From af4f99570b49e00c78d248253a5f55a74ba22ca6 Mon Sep 17 00:00:00 2001 From: Miguel Martinez Trivino Date: Sat, 14 Mar 2026 09:46:12 +0100 Subject: [PATCH 1/5] fix: PR metadata temp file missing .json extension Add wildcard to os.CreateTemp pattern so the random suffix is inserted before the extension instead of appended after it. Closes #2845 Signed-off-by: Miguel Martinez Trivino --- pkg/attestation/crafter/crafter.go | 2 +- pkg/attestation/crafter/prmetadata_test.go | 15 ++++++++++++++- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/pkg/attestation/crafter/crafter.go b/pkg/attestation/crafter/crafter.go index 5918e0648..9646001da 100644 --- a/pkg/attestation/crafter/crafter.go +++ b/pkg/attestation/crafter/crafter.go @@ -546,7 +546,7 @@ func (c *Crafter) AutoCollectPRMetadata(ctx context.Context, attestationID strin // Create a temporary file for the metadata materialName := fmt.Sprintf("pr-metadata-%s", metadata.Number) - tmpFile, err := os.CreateTemp("", fmt.Sprintf("%s.json", materialName)) + tmpFile, err := os.CreateTemp("", fmt.Sprintf("%s*.json", materialName)) if err != nil { return fmt.Errorf("failed to create temp file: %w", err) } diff --git a/pkg/attestation/crafter/prmetadata_test.go b/pkg/attestation/crafter/prmetadata_test.go index 472234b2f..0031b64cd 100644 --- a/pkg/attestation/crafter/prmetadata_test.go +++ b/pkg/attestation/crafter/prmetadata_test.go @@ -1,5 +1,5 @@ // -// Copyright 2024-2025 The Chainloop Authors. +// Copyright 2024-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. @@ -16,6 +16,8 @@ package crafter import ( + "fmt" + "os" "path/filepath" "testing" @@ -23,6 +25,17 @@ import ( "github.com/stretchr/testify/require" ) +func TestPRMetadataTempFileHasJSONExtension(t *testing.T) { + materialName := fmt.Sprintf("pr-metadata-%s", "123") + tmpFile, err := os.CreateTemp("", fmt.Sprintf("%s*.json", materialName)) + require.NoError(t, err) + defer os.Remove(tmpFile.Name()) + tmpFile.Close() + + assert.Equal(t, ".json", filepath.Ext(tmpFile.Name()), + "temp file should have .json extension, got: %s", tmpFile.Name()) +} + func TestExtractGitHubPRMetadata(t *testing.T) { testCases := []struct { name string From 7f0e966bb1d0172d5115bf638f0ae26e1add1d84 Mon Sep 17 00:00:00 2001 From: Miguel Martinez Trivino Date: Sat, 14 Mar 2026 18:20:17 +0100 Subject: [PATCH 2/5] fix: move PR metadata temp file test to collector and update CLAUDE.md Move the temp file JSON extension test to collector_prmetadata_test.go using table-driven tests, matching the new collector-based pattern. Add development guidelines for table-driven tests, TDD, and issue repo. Signed-off-by: Miguel Martinez Trivino --- CLAUDE.md | 3 + .../crafter/collector_prmetadata_test.go | 70 +++++++++++++++++++ pkg/attestation/crafter/prmetadata_test.go | 13 ---- 3 files changed, 73 insertions(+), 13 deletions(-) create mode 100644 pkg/attestation/crafter/collector_prmetadata_test.go diff --git a/CLAUDE.md b/CLAUDE.md index c2a6f82ff..0f6d148eb 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -259,6 +259,8 @@ Code reviews are required for all submissions via GitHub pull requests. - I do not want you to be in the co-author signoff - when the schema is changed, run make generate, do not create a migration explicitly - If you are writing go code, adhere to best practices such as the ones in effective-go, or others. This could include, error handling patterns, interface design, package organization, concurrency patterns, etc. +- When writing tests, use table-driven tests whenever possible +- When implementing new functionality, follow TDD: write failing tests first, then implement the code to make them pass - do not change previous migrations, they are immutable - if you add any new dependency to a constructor, remember to run wire ./... - when adding new inedexes, make sure to update the generated sql migraiton files and make them CREATE INDEX CONCURRENTLY and set -- atlas:txmode none at the top @@ -272,3 +274,4 @@ Code reviews are required for all submissions via GitHub pull requests. - any call to authorization Enforce done from the biz or svc layer must be done using biz.AuthzUseCase - if you modify a schema, remember to run `make migration_sync` - after changing Helm chart source code (`deployment/chainloop/`), bump the **patch** version (not minor, not major) in the chart's `Chart.yaml` +- when asked to create a GitHub issue, create it in the `chainloop-dev/chainloop` repository diff --git a/pkg/attestation/crafter/collector_prmetadata_test.go b/pkg/attestation/crafter/collector_prmetadata_test.go new file mode 100644 index 000000000..350bd8de8 --- /dev/null +++ b/pkg/attestation/crafter/collector_prmetadata_test.go @@ -0,0 +1,70 @@ +// +// 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 ( + "fmt" + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestPRMetadataCollectorTempFileHasJSONExtension(t *testing.T) { + tests := []struct { + name string + prNumber string + wantPrefix string + wantExtension string + }{ + { + name: "numeric PR number", + prNumber: "123", + wantPrefix: "pr-metadata-123-", + wantExtension: ".json", + }, + { + name: "large PR number", + prNumber: "99999", + wantPrefix: "pr-metadata-99999-", + wantExtension: ".json", + }, + { + name: "single digit PR number", + prNumber: "1", + wantPrefix: "pr-metadata-1-", + wantExtension: ".json", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + materialName := fmt.Sprintf("pr-metadata-%s", tc.prNumber) + tmpFile, err := os.CreateTemp("", fmt.Sprintf("%s-*.json", materialName)) + require.NoError(t, err) + defer os.Remove(tmpFile.Name()) + tmpFile.Close() + + fileName := filepath.Base(tmpFile.Name()) + assert.Equal(t, tc.wantExtension, filepath.Ext(fileName)) + assert.True(t, len(fileName) > len(tc.wantPrefix), + "filename %q should be longer than prefix %q", fileName, tc.wantPrefix) + assert.Equal(t, tc.wantPrefix, fileName[:len(tc.wantPrefix)]) + }) + } +} diff --git a/pkg/attestation/crafter/prmetadata_test.go b/pkg/attestation/crafter/prmetadata_test.go index 0031b64cd..1e6636839 100644 --- a/pkg/attestation/crafter/prmetadata_test.go +++ b/pkg/attestation/crafter/prmetadata_test.go @@ -16,8 +16,6 @@ package crafter import ( - "fmt" - "os" "path/filepath" "testing" @@ -25,17 +23,6 @@ import ( "github.com/stretchr/testify/require" ) -func TestPRMetadataTempFileHasJSONExtension(t *testing.T) { - materialName := fmt.Sprintf("pr-metadata-%s", "123") - tmpFile, err := os.CreateTemp("", fmt.Sprintf("%s*.json", materialName)) - require.NoError(t, err) - defer os.Remove(tmpFile.Name()) - tmpFile.Close() - - assert.Equal(t, ".json", filepath.Ext(tmpFile.Name()), - "temp file should have .json extension, got: %s", tmpFile.Name()) -} - func TestExtractGitHubPRMetadata(t *testing.T) { testCases := []struct { name string From e97b997add7d4c5dd101126c89f5ea1b2a0f207b Mon Sep 17 00:00:00 2001 From: Miguel Martinez Trivino Date: Sat, 14 Mar 2026 18:32:22 +0100 Subject: [PATCH 3/5] fix: extract createPRMetadataTempFile for testable file naming Extract a single helper that creates the temp file and returns the material name and file path, replacing inline formatting. Signed-off-by: Miguel Martinez Trivino --- .../crafter/collector_prmetadata.go | 34 +++++++++---- .../crafter/collector_prmetadata_test.go | 50 +++++++++---------- 2 files changed, 46 insertions(+), 38 deletions(-) diff --git a/pkg/attestation/crafter/collector_prmetadata.go b/pkg/attestation/crafter/collector_prmetadata.go index f683a2116..36acb8289 100644 --- a/pkg/attestation/crafter/collector_prmetadata.go +++ b/pkg/attestation/crafter/collector_prmetadata.go @@ -68,20 +68,13 @@ func (c *PRMetadataCollector) Collect(ctx context.Context, cr *Crafter, attestat return fmt.Errorf("marshaling PR/MR metadata: %w", err) } - materialName := fmt.Sprintf("pr-metadata-%s", metadata.Number) - tmpFile, err := os.CreateTemp("", fmt.Sprintf("%s-*.json", materialName)) + materialName, tmpFile, err := createPRMetadataTempFile(metadata.Number, jsonData) 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() - return fmt.Errorf("writing temp file: %w", err) + return fmt.Errorf("creating PR metadata temp file: %w", err) } - tmpFile.Close() + defer os.Remove(tmpFile) - if _, err := cr.AddMaterialContractFree(ctx, attestationID, schemaapi.CraftingSchema_Material_CHAINLOOP_PR_INFO.String(), materialName, tmpFile.Name(), 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) } @@ -89,3 +82,22 @@ 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() + return "", "", fmt.Errorf("writing temp file: %w", err) + } + tmpFile.Close() + + return materialName, tmpFile.Name(), nil +} diff --git a/pkg/attestation/crafter/collector_prmetadata_test.go b/pkg/attestation/crafter/collector_prmetadata_test.go index 350bd8de8..0ed4ca310 100644 --- a/pkg/attestation/crafter/collector_prmetadata_test.go +++ b/pkg/attestation/crafter/collector_prmetadata_test.go @@ -16,55 +16,51 @@ package crafter import ( - "fmt" "os" "path/filepath" + "strings" "testing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) -func TestPRMetadataCollectorTempFileHasJSONExtension(t *testing.T) { +func TestCreatePRMetadataTempFile(t *testing.T) { tests := []struct { - name string - prNumber string - wantPrefix string - wantExtension string + name string + prNumber string + wantMaterial string + wantFilePrefix string }{ { - name: "numeric PR number", - prNumber: "123", - wantPrefix: "pr-metadata-123-", - wantExtension: ".json", + name: "numeric PR number", + prNumber: "123", + wantMaterial: "pr-metadata-123", + wantFilePrefix: "pr-metadata-123-", }, { - name: "large PR number", - prNumber: "99999", - wantPrefix: "pr-metadata-99999-", - wantExtension: ".json", + name: "large PR number", + prNumber: "99999", + wantMaterial: "pr-metadata-99999", + wantFilePrefix: "pr-metadata-99999-", }, { - name: "single digit PR number", - prNumber: "1", - wantPrefix: "pr-metadata-1-", - wantExtension: ".json", + 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 := fmt.Sprintf("pr-metadata-%s", tc.prNumber) - tmpFile, err := os.CreateTemp("", fmt.Sprintf("%s-*.json", materialName)) + materialName, filePath, err := createPRMetadataTempFile(tc.prNumber, []byte(`{"test": true}`)) require.NoError(t, err) - defer os.Remove(tmpFile.Name()) - tmpFile.Close() + defer os.Remove(filePath) - fileName := filepath.Base(tmpFile.Name()) - assert.Equal(t, tc.wantExtension, filepath.Ext(fileName)) - assert.True(t, len(fileName) > len(tc.wantPrefix), - "filename %q should be longer than prefix %q", fileName, tc.wantPrefix) - assert.Equal(t, tc.wantPrefix, fileName[:len(tc.wantPrefix)]) + assert.Equal(t, tc.wantMaterial, materialName) + assert.Equal(t, ".json", filepath.Ext(filePath)) + assert.True(t, strings.HasPrefix(filepath.Base(filePath), tc.wantFilePrefix)) }) } } From 774848e8c240ba94a1bf55c2bdc7ac5870364b08 Mon Sep 17 00:00:00 2001 From: Miguel Martinez Trivino Date: Sat, 14 Mar 2026 18:36:20 +0100 Subject: [PATCH 4/5] fix: clean up temp file on write error in createPRMetadataTempFile Signed-off-by: Miguel Martinez Trivino --- pkg/attestation/crafter/collector_prmetadata.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/attestation/crafter/collector_prmetadata.go b/pkg/attestation/crafter/collector_prmetadata.go index 36acb8289..4c155b8a7 100644 --- a/pkg/attestation/crafter/collector_prmetadata.go +++ b/pkg/attestation/crafter/collector_prmetadata.go @@ -95,6 +95,7 @@ func createPRMetadataTempFile(prNumber string, data []byte) (materialName string if _, err := tmpFile.Write(data); err != nil { tmpFile.Close() + os.Remove(tmpFile.Name()) return "", "", fmt.Errorf("writing temp file: %w", err) } tmpFile.Close() From 6217705afb8a30f383437d21d419992db9168d72 Mon Sep 17 00:00:00 2001 From: Miguel Martinez Trivino Date: Sat, 14 Mar 2026 18:43:13 +0100 Subject: [PATCH 5/5] fix: check Close() error in createPRMetadataTempFile Signed-off-by: Miguel Martinez Trivino --- pkg/attestation/crafter/collector_prmetadata.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/pkg/attestation/crafter/collector_prmetadata.go b/pkg/attestation/crafter/collector_prmetadata.go index 4c155b8a7..2d75cc8d5 100644 --- a/pkg/attestation/crafter/collector_prmetadata.go +++ b/pkg/attestation/crafter/collector_prmetadata.go @@ -98,7 +98,10 @@ func createPRMetadataTempFile(prNumber string, data []byte) (materialName string os.Remove(tmpFile.Name()) return "", "", fmt.Errorf("writing temp file: %w", err) } - tmpFile.Close() + if err := tmpFile.Close(); err != nil { + os.Remove(tmpFile.Name()) + return "", "", fmt.Errorf("closing temp file: %w", err) + } return materialName, tmpFile.Name(), nil }