Skip to content

Commit 14ef4c0

Browse files
waveywavesclaude
andcommitted
fix(crafter): address review feedback - revert PR metadata, simplify AI config filename
- Revert collector_prmetadata.go: PR metadata intentionally uses PR number for naming; deduplication within the same PR is expected - Restore collector_prmetadata_test.go deleted by mistake - Simplify AI config temp filename to a constant name per agent (ai-agent-config-{agentName}.json) instead of appending the full 64-character ConfigHash Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Vibhav Bobade <vibhav.bobde@gmail.com>
1 parent 8f05279 commit 14ef4c0

3 files changed

Lines changed: 98 additions & 19 deletions

File tree

pkg/attestation/crafter/collector_aiagentconfig.go

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -100,17 +100,15 @@ func (c *AIAgentConfigCollector) uploadAgentConfig(
100100
return fmt.Errorf("marshaling AI agent config for %s: %w", agentName, err)
101101
}
102102

103-
// Use a deterministic filename derived from the config hash so that retries
104-
// produce the same Artifact.Name (via fileStats -> os.Stat().Name()) and
105-
// avoid duplicate CAS uploads. PR #2917 fixed the primary root cause
106-
// (non-deterministic content from captured_at); this is additional hardening.
107-
tmpPath := filepath.Join(os.TempDir(), fmt.Sprintf("ai-agent-config-%s-%s.json", agentName, data.ConfigHash))
103+
// Use a constant filename per agent so retries produce the same
104+
// Artifact.Name (via fileStats -> os.Stat().Name()).
105+
materialName := fmt.Sprintf("ai-agent-config-%s", agentName)
106+
tmpPath := filepath.Join(os.TempDir(), fmt.Sprintf("%s.json", materialName))
108107
if err := os.WriteFile(tmpPath, jsonData, 0o600); err != nil {
109108
return fmt.Errorf("writing temp file: %w", err)
110109
}
111110
defer os.Remove(tmpPath)
112111

113-
materialName := fmt.Sprintf("ai-agent-config-%s", agentName)
114112
if _, err := cr.AddMaterialContractFree(ctx, attestationID, schemaapi.CraftingSchema_Material_CHAINLOOP_AI_AGENT_CONFIG.String(), materialName, tmpPath, casBackend, nil); err != nil {
115113
return fmt.Errorf("adding AI agent config material for %s: %w", agentName, err)
116114
}

pkg/attestation/crafter/collector_prmetadata.go

Lines changed: 28 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -17,12 +17,9 @@ package crafter
1717

1818
import (
1919
"context"
20-
"crypto/sha256"
21-
"encoding/hex"
2220
"encoding/json"
2321
"fmt"
2422
"os"
25-
"path/filepath"
2623

2724
schemaapi "github.com/chainloop-dev/chainloop/app/controlplane/api/workflowcontract/v1"
2825
"github.com/chainloop-dev/chainloop/internal/prinfo"
@@ -72,22 +69,40 @@ func (c *PRMetadataCollector) Collect(ctx context.Context, cr *Crafter, attestat
7269
return fmt.Errorf("marshaling PR/MR metadata: %w", err)
7370
}
7471

75-
// Use a deterministic filename derived from a hash of the content so that
76-
// retries produce the same Artifact.Name (via fileStats -> os.Stat().Name())
77-
// and avoid duplicate CAS uploads.
78-
contentHash := sha256.Sum256(jsonData)
79-
materialName := fmt.Sprintf("pr-metadata-%s", metadata.Number)
80-
tmpPath := filepath.Join(os.TempDir(), fmt.Sprintf("%s-%s.json", materialName, hex.EncodeToString(contentHash[:])))
81-
if err := os.WriteFile(tmpPath, jsonData, 0o600); err != nil {
82-
return fmt.Errorf("writing temp file: %w", err)
72+
materialName, tmpFile, err := createPRMetadataTempFile(metadata.Number, jsonData)
73+
if err != nil {
74+
return fmt.Errorf("creating PR metadata temp file: %w", err)
8375
}
84-
defer os.Remove(tmpPath)
76+
defer os.Remove(tmpFile)
8577

86-
if _, err := cr.AddMaterialContractFree(ctx, attestationID, schemaapi.CraftingSchema_Material_CHAINLOOP_PR_INFO.String(), materialName, tmpPath, casBackend, nil); err != nil {
78+
if _, err := cr.AddMaterialContractFree(ctx, attestationID, schemaapi.CraftingSchema_Material_CHAINLOOP_PR_INFO.String(), materialName, tmpFile, casBackend, nil); err != nil {
8779
return fmt.Errorf("adding PR/MR metadata material: %w", err)
8880
}
8981

9082
cr.Logger.Info().Msg("successfully collected and attested PR/MR metadata")
9183

9284
return nil
9385
}
86+
87+
// createPRMetadataTempFile creates a temp file with the PR metadata JSON content
88+
// and returns the material name and the temp file path.
89+
func createPRMetadataTempFile(prNumber string, data []byte) (materialName string, filePath string, err error) {
90+
materialName = fmt.Sprintf("pr-metadata-%s", prNumber)
91+
92+
tmpFile, err := os.CreateTemp("", fmt.Sprintf("%s-*.json", materialName))
93+
if err != nil {
94+
return "", "", fmt.Errorf("creating temp file: %w", err)
95+
}
96+
97+
if _, err := tmpFile.Write(data); err != nil {
98+
tmpFile.Close()
99+
os.Remove(tmpFile.Name())
100+
return "", "", fmt.Errorf("writing temp file: %w", err)
101+
}
102+
if err := tmpFile.Close(); err != nil {
103+
os.Remove(tmpFile.Name())
104+
return "", "", fmt.Errorf("closing temp file: %w", err)
105+
}
106+
107+
return materialName, tmpFile.Name(), nil
108+
}
Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,66 @@
1+
//
2+
// Copyright 2026 The Chainloop Authors.
3+
//
4+
// Licensed under the Apache License, Version 2.0 (the "License");
5+
// you may not use this file except in compliance with the License.
6+
// You may obtain a copy of the License at
7+
//
8+
// http://www.apache.org/licenses/LICENSE-2.0
9+
//
10+
// Unless required by applicable law or agreed to in writing, software
11+
// distributed under the License is distributed on an "AS IS" BASIS,
12+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
// See the License for the specific language governing permissions and
14+
// limitations under the License.
15+
16+
package crafter
17+
18+
import (
19+
"os"
20+
"path/filepath"
21+
"strings"
22+
"testing"
23+
24+
"github.com/stretchr/testify/assert"
25+
"github.com/stretchr/testify/require"
26+
)
27+
28+
func TestCreatePRMetadataTempFile(t *testing.T) {
29+
tests := []struct {
30+
name string
31+
prNumber string
32+
wantMaterial string
33+
wantFilePrefix string
34+
}{
35+
{
36+
name: "numeric PR number",
37+
prNumber: "123",
38+
wantMaterial: "pr-metadata-123",
39+
wantFilePrefix: "pr-metadata-123-",
40+
},
41+
{
42+
name: "large PR number",
43+
prNumber: "99999",
44+
wantMaterial: "pr-metadata-99999",
45+
wantFilePrefix: "pr-metadata-99999-",
46+
},
47+
{
48+
name: "single digit PR number",
49+
prNumber: "1",
50+
wantMaterial: "pr-metadata-1",
51+
wantFilePrefix: "pr-metadata-1-",
52+
},
53+
}
54+
55+
for _, tc := range tests {
56+
t.Run(tc.name, func(t *testing.T) {
57+
materialName, filePath, err := createPRMetadataTempFile(tc.prNumber, []byte(`{"test": true}`))
58+
require.NoError(t, err)
59+
defer os.Remove(filePath)
60+
61+
assert.Equal(t, tc.wantMaterial, materialName)
62+
assert.Equal(t, ".json", filepath.Ext(filePath))
63+
assert.True(t, strings.HasPrefix(filepath.Base(filePath), tc.wantFilePrefix))
64+
})
65+
}
66+
}

0 commit comments

Comments
 (0)