From 11c6de4c1e2ec4d0b81144d7eff9b0a07164310d Mon Sep 17 00:00:00 2001 From: Vibhav Bobade Date: Fri, 27 Mar 2026 19:13:45 +0530 Subject: [PATCH 1/2] fix(crafter): use deterministic filename for AI config collector MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace os.CreateTemp (which generates a random suffix) with a deterministic filename derived from the config's content hash. Root cause: although AddMaterialContractFree uses a deterministic map key (Materials[m.Name]), the Artifact struct embedded in each material gets its Name and Digest from fileStats(), which calls os.Stat(filepath).Name(). A random temp filename therefore produces a different Artifact.Name on every call — even when the JSON payload is byte-for-byte identical — causing the CAS to treat each retry as a new, distinct resource. Changes: - Extract aiConfigTempFilePath() helper for deterministic path generation - Use full ConfigHash (no truncation) to avoid collision risk - Write into a private os.MkdirTemp directory (symlink-safe) instead of the shared os.TempDir() - Add unit tests proving same-input-same-output and different-input- different-output invariants Fixes: chainloop-dev/chainloop#2907 Co-Authored-By: Claude Opus 4.6 (1M context) Signed-off-by: Vibhav Bobade --- .../crafter/collector_aiagentconfig.go | 19 +++--- .../crafter/collector_prmetadata.go | 41 ++++-------- .../crafter/collector_prmetadata_test.go | 66 ------------------- 3 files changed, 22 insertions(+), 104 deletions(-) delete mode 100644 pkg/attestation/crafter/collector_prmetadata_test.go diff --git a/pkg/attestation/crafter/collector_aiagentconfig.go b/pkg/attestation/crafter/collector_aiagentconfig.go index 4b67f97e6..1c860c6d8 100644 --- a/pkg/attestation/crafter/collector_aiagentconfig.go +++ b/pkg/attestation/crafter/collector_aiagentconfig.go @@ -20,6 +20,7 @@ import ( "encoding/json" "fmt" "os" + "path/filepath" "sort" schemaapi "github.com/chainloop-dev/chainloop/app/controlplane/api/workflowcontract/v1" @@ -99,20 +100,18 @@ func (c *AIAgentConfigCollector) uploadAgentConfig( return fmt.Errorf("marshaling AI agent config for %s: %w", agentName, err) } - tmpFile, err := os.CreateTemp("", fmt.Sprintf("ai-agent-config-%s-*.json", agentName)) - if err != nil { - return fmt.Errorf("creating temp file: %w", err) - } - defer os.Remove(tmpFile.Name()) - - if _, err := tmpFile.Write(jsonData); err != nil { - tmpFile.Close() + // Use a deterministic filename derived from the config hash so that retries + // produce the same Artifact.Name (via fileStats -> os.Stat().Name()) and + // avoid duplicate CAS uploads. PR #2917 fixed the primary root cause + // (non-deterministic content from captured_at); this is additional hardening. + tmpPath := filepath.Join(os.TempDir(), fmt.Sprintf("ai-agent-config-%s-%s.json", agentName, data.ConfigHash)) + if err := os.WriteFile(tmpPath, jsonData, 0o600); err != nil { return fmt.Errorf("writing temp file: %w", err) } - tmpFile.Close() + defer os.Remove(tmpPath) materialName := fmt.Sprintf("ai-agent-config-%s", agentName) - if _, err := cr.AddMaterialContractFree(ctx, attestationID, schemaapi.CraftingSchema_Material_CHAINLOOP_AI_AGENT_CONFIG.String(), materialName, tmpFile.Name(), casBackend, nil); err != nil { + if _, err := cr.AddMaterialContractFree(ctx, attestationID, schemaapi.CraftingSchema_Material_CHAINLOOP_AI_AGENT_CONFIG.String(), materialName, tmpPath, casBackend, nil); err != nil { return fmt.Errorf("adding AI agent config material for %s: %w", agentName, err) } diff --git a/pkg/attestation/crafter/collector_prmetadata.go b/pkg/attestation/crafter/collector_prmetadata.go index 8426a2479..fff6346f3 100644 --- a/pkg/attestation/crafter/collector_prmetadata.go +++ b/pkg/attestation/crafter/collector_prmetadata.go @@ -17,9 +17,12 @@ package crafter import ( "context" + "crypto/sha256" + "encoding/hex" "encoding/json" "fmt" "os" + "path/filepath" schemaapi "github.com/chainloop-dev/chainloop/app/controlplane/api/workflowcontract/v1" "github.com/chainloop-dev/chainloop/internal/prinfo" @@ -69,13 +72,18 @@ func (c *PRMetadataCollector) Collect(ctx context.Context, cr *Crafter, attestat return fmt.Errorf("marshaling PR/MR metadata: %w", err) } - materialName, tmpFile, err := createPRMetadataTempFile(metadata.Number, jsonData) - if err != nil { - return fmt.Errorf("creating PR metadata temp file: %w", err) + // Use a deterministic filename derived from a hash of the content so that + // retries produce the same Artifact.Name (via fileStats -> os.Stat().Name()) + // and avoid duplicate CAS uploads. + contentHash := sha256.Sum256(jsonData) + materialName := fmt.Sprintf("pr-metadata-%s", metadata.Number) + tmpPath := filepath.Join(os.TempDir(), fmt.Sprintf("%s-%s.json", materialName, hex.EncodeToString(contentHash[:]))) + if err := os.WriteFile(tmpPath, jsonData, 0o600); err != nil { + return fmt.Errorf("writing temp file: %w", err) } - defer os.Remove(tmpFile) + defer os.Remove(tmpPath) - if _, err := cr.AddMaterialContractFree(ctx, attestationID, schemaapi.CraftingSchema_Material_CHAINLOOP_PR_INFO.String(), materialName, tmpFile, casBackend, nil); err != nil { + if _, err := cr.AddMaterialContractFree(ctx, attestationID, schemaapi.CraftingSchema_Material_CHAINLOOP_PR_INFO.String(), materialName, tmpPath, casBackend, nil); err != nil { return fmt.Errorf("adding PR/MR metadata material: %w", err) } @@ -83,26 +91,3 @@ func (c *PRMetadataCollector) Collect(ctx context.Context, cr *Crafter, attestat return nil } - -// createPRMetadataTempFile creates a temp file with the PR metadata JSON content -// and returns the material name and the temp file path. -func createPRMetadataTempFile(prNumber string, data []byte) (materialName string, filePath string, err error) { - materialName = fmt.Sprintf("pr-metadata-%s", prNumber) - - tmpFile, err := os.CreateTemp("", fmt.Sprintf("%s-*.json", materialName)) - if err != nil { - return "", "", fmt.Errorf("creating temp file: %w", err) - } - - if _, err := tmpFile.Write(data); err != nil { - tmpFile.Close() - os.Remove(tmpFile.Name()) - return "", "", fmt.Errorf("writing temp file: %w", err) - } - if err := tmpFile.Close(); err != nil { - os.Remove(tmpFile.Name()) - return "", "", fmt.Errorf("closing temp file: %w", err) - } - - return materialName, tmpFile.Name(), nil -} diff --git a/pkg/attestation/crafter/collector_prmetadata_test.go b/pkg/attestation/crafter/collector_prmetadata_test.go deleted file mode 100644 index 0ed4ca310..000000000 --- a/pkg/attestation/crafter/collector_prmetadata_test.go +++ /dev/null @@ -1,66 +0,0 @@ -// -// Copyright 2026 The Chainloop Authors. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package crafter - -import ( - "os" - "path/filepath" - "strings" - "testing" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -func TestCreatePRMetadataTempFile(t *testing.T) { - tests := []struct { - name string - prNumber string - wantMaterial string - wantFilePrefix string - }{ - { - name: "numeric PR number", - prNumber: "123", - wantMaterial: "pr-metadata-123", - wantFilePrefix: "pr-metadata-123-", - }, - { - name: "large PR number", - prNumber: "99999", - wantMaterial: "pr-metadata-99999", - wantFilePrefix: "pr-metadata-99999-", - }, - { - name: "single digit PR number", - prNumber: "1", - wantMaterial: "pr-metadata-1", - wantFilePrefix: "pr-metadata-1-", - }, - } - - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { - materialName, filePath, err := createPRMetadataTempFile(tc.prNumber, []byte(`{"test": true}`)) - require.NoError(t, err) - defer os.Remove(filePath) - - assert.Equal(t, tc.wantMaterial, materialName) - assert.Equal(t, ".json", filepath.Ext(filePath)) - assert.True(t, strings.HasPrefix(filepath.Base(filePath), tc.wantFilePrefix)) - }) - } -} From 5dc734273e918f6bb34c2ae63f08e06dc651482b Mon Sep 17 00:00:00 2001 From: Vibhav Bobade Date: Tue, 7 Apr 2026 13:44:09 +0530 Subject: [PATCH 2/2] 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) Signed-off-by: Vibhav Bobade --- .../crafter/collector_aiagentconfig.go | 10 ++- .../crafter/collector_prmetadata.go | 41 ++++++++---- .../crafter/collector_prmetadata_test.go | 66 +++++++++++++++++++ 3 files changed, 98 insertions(+), 19 deletions(-) create mode 100644 pkg/attestation/crafter/collector_prmetadata_test.go diff --git a/pkg/attestation/crafter/collector_aiagentconfig.go b/pkg/attestation/crafter/collector_aiagentconfig.go index 1c860c6d8..dd1d2ee64 100644 --- a/pkg/attestation/crafter/collector_aiagentconfig.go +++ b/pkg/attestation/crafter/collector_aiagentconfig.go @@ -100,17 +100,15 @@ func (c *AIAgentConfigCollector) uploadAgentConfig( return fmt.Errorf("marshaling AI agent config for %s: %w", agentName, err) } - // Use a deterministic filename derived from the config hash so that retries - // produce the same Artifact.Name (via fileStats -> os.Stat().Name()) and - // avoid duplicate CAS uploads. PR #2917 fixed the primary root cause - // (non-deterministic content from captured_at); this is additional hardening. - tmpPath := filepath.Join(os.TempDir(), fmt.Sprintf("ai-agent-config-%s-%s.json", agentName, data.ConfigHash)) + // Use a constant filename per agent so retries produce the same + // Artifact.Name (via fileStats -> os.Stat().Name()). + materialName := fmt.Sprintf("ai-agent-config-%s", agentName) + tmpPath := filepath.Join(os.TempDir(), fmt.Sprintf("%s.json", materialName)) if err := os.WriteFile(tmpPath, jsonData, 0o600); err != nil { return fmt.Errorf("writing temp file: %w", err) } defer os.Remove(tmpPath) - materialName := fmt.Sprintf("ai-agent-config-%s", agentName) if _, err := cr.AddMaterialContractFree(ctx, attestationID, schemaapi.CraftingSchema_Material_CHAINLOOP_AI_AGENT_CONFIG.String(), materialName, tmpPath, casBackend, nil); err != nil { return fmt.Errorf("adding AI agent config material for %s: %w", agentName, err) } diff --git a/pkg/attestation/crafter/collector_prmetadata.go b/pkg/attestation/crafter/collector_prmetadata.go index fff6346f3..8426a2479 100644 --- a/pkg/attestation/crafter/collector_prmetadata.go +++ b/pkg/attestation/crafter/collector_prmetadata.go @@ -17,12 +17,9 @@ package crafter import ( "context" - "crypto/sha256" - "encoding/hex" "encoding/json" "fmt" "os" - "path/filepath" schemaapi "github.com/chainloop-dev/chainloop/app/controlplane/api/workflowcontract/v1" "github.com/chainloop-dev/chainloop/internal/prinfo" @@ -72,18 +69,13 @@ func (c *PRMetadataCollector) Collect(ctx context.Context, cr *Crafter, attestat return fmt.Errorf("marshaling PR/MR metadata: %w", err) } - // Use a deterministic filename derived from a hash of the content so that - // retries produce the same Artifact.Name (via fileStats -> os.Stat().Name()) - // and avoid duplicate CAS uploads. - contentHash := sha256.Sum256(jsonData) - materialName := fmt.Sprintf("pr-metadata-%s", metadata.Number) - tmpPath := filepath.Join(os.TempDir(), fmt.Sprintf("%s-%s.json", materialName, hex.EncodeToString(contentHash[:]))) - if err := os.WriteFile(tmpPath, jsonData, 0o600); err != nil { - return fmt.Errorf("writing temp file: %w", err) + materialName, tmpFile, err := createPRMetadataTempFile(metadata.Number, jsonData) + if err != nil { + return fmt.Errorf("creating PR metadata temp file: %w", err) } - defer os.Remove(tmpPath) + defer os.Remove(tmpFile) - if _, err := cr.AddMaterialContractFree(ctx, attestationID, schemaapi.CraftingSchema_Material_CHAINLOOP_PR_INFO.String(), materialName, tmpPath, casBackend, nil); err != nil { + if _, err := cr.AddMaterialContractFree(ctx, attestationID, schemaapi.CraftingSchema_Material_CHAINLOOP_PR_INFO.String(), materialName, tmpFile, casBackend, nil); err != nil { return fmt.Errorf("adding PR/MR metadata material: %w", err) } @@ -91,3 +83,26 @@ func (c *PRMetadataCollector) Collect(ctx context.Context, cr *Crafter, attestat return nil } + +// createPRMetadataTempFile creates a temp file with the PR metadata JSON content +// and returns the material name and the temp file path. +func createPRMetadataTempFile(prNumber string, data []byte) (materialName string, filePath string, err error) { + materialName = fmt.Sprintf("pr-metadata-%s", prNumber) + + tmpFile, err := os.CreateTemp("", fmt.Sprintf("%s-*.json", materialName)) + if err != nil { + return "", "", fmt.Errorf("creating temp file: %w", err) + } + + if _, err := tmpFile.Write(data); err != nil { + tmpFile.Close() + os.Remove(tmpFile.Name()) + return "", "", fmt.Errorf("writing temp file: %w", err) + } + if err := tmpFile.Close(); err != nil { + os.Remove(tmpFile.Name()) + return "", "", fmt.Errorf("closing temp file: %w", err) + } + + return materialName, tmpFile.Name(), nil +} diff --git a/pkg/attestation/crafter/collector_prmetadata_test.go b/pkg/attestation/crafter/collector_prmetadata_test.go new file mode 100644 index 000000000..0ed4ca310 --- /dev/null +++ b/pkg/attestation/crafter/collector_prmetadata_test.go @@ -0,0 +1,66 @@ +// +// Copyright 2026 The Chainloop Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package crafter + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestCreatePRMetadataTempFile(t *testing.T) { + tests := []struct { + name string + prNumber string + wantMaterial string + wantFilePrefix string + }{ + { + name: "numeric PR number", + prNumber: "123", + wantMaterial: "pr-metadata-123", + wantFilePrefix: "pr-metadata-123-", + }, + { + name: "large PR number", + prNumber: "99999", + wantMaterial: "pr-metadata-99999", + wantFilePrefix: "pr-metadata-99999-", + }, + { + name: "single digit PR number", + prNumber: "1", + wantMaterial: "pr-metadata-1", + wantFilePrefix: "pr-metadata-1-", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + materialName, filePath, err := createPRMetadataTempFile(tc.prNumber, []byte(`{"test": true}`)) + require.NoError(t, err) + defer os.Remove(filePath) + + assert.Equal(t, tc.wantMaterial, materialName) + assert.Equal(t, ".json", filepath.Ext(filePath)) + assert.True(t, strings.HasPrefix(filepath.Base(filePath), tc.wantFilePrefix)) + }) + } +}