test(bom): harden BOM version-freshness gate (#1576)#1580
Conversation
yuanchen8911
added
the
theme/supply-chain
|
🌿 Preview your docs: https://nvidia-preview-fix-1576-freshness-gate-hardening.docs.buildwithfern.com/aicr |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds BOM-specific merge-gate coverage, updates BOM generation and exemption checks to use type-aware version fields for Helm and Kustomize, and tightens freshness parsing to operate only on the generated BOM section with bidirectional component-set validation and duplicate-row rejection. Documentation was updated to match the freshness behavior. Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tools/bom/freshness_parse_test.go`:
- Around line 60-119: Refactor TestParseBOMVersionTable into a table-driven test
instead of separate t.Run blocks. Define a cases table with fields like name,
section, want map[string]string, and wantErr bool, then loop over the cases and
assert through parseBOMVersionTable. Keep the existing scenarios (header-based
parsing, duplicate rows, ignoring unrelated tables, and empty input) but
consolidate the repeated setup/assertion logic in TestParseBOMVersionTable,
following the pattern used by TestExtractGeneratedSection.
In `@tools/bom/freshness_test.go`:
- Around line 157-168: `extractGeneratedSection` and the duplicate-row error
path in `parseBOMVersionTable` are still constructing errors with `fmt.Errorf`;
switch those returns to use `pkg/errors` instead, keeping the same messages and
control flow. Update the error imports in this test file accordingly and ensure
both the missing-marker checks in `extractGeneratedSection` and the
duplicate-row check in `parseBOMVersionTable` use the shared error helper
consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: d5d014b1-e957-41e4-a789-2911140a0d8a
📒 Files selected for processing (6)
.github/workflows/merge-gate.yamldocs/contributor/recipe.mdpkg/recipe/version_pin_guard_test.gotools/bom/freshness_parse_test.gotools/bom/freshness_test.gotools/bom/main.go
| func extractGeneratedSection(doc string) (string, error) { | ||
| begin := strings.Index(doc, bomBeginMarker) | ||
| if begin < 0 { | ||
| return "", fmt.Errorf("missing %q marker", bomBeginMarker) | ||
| } | ||
| begin += len(bomBeginMarker) | ||
| end := strings.Index(doc[begin:], bomEndMarker) | ||
| if end < 0 { | ||
| return "", fmt.Errorf("missing %q marker after %q", bomEndMarker, bomBeginMarker) | ||
| } | ||
| return doc[begin : begin+end], nil | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Use pkg/errors instead of fmt.Errorf for these error returns.
extractGeneratedSection and the duplicate-row check in parseBOMVersionTable construct errors with fmt.Errorf. As per coding guidelines, **/*.go code should use pkg/errors for error handling instead of fmt.Errorf.
♻️ Example fix using pkg/errors
- return "", fmt.Errorf("missing %q marker", bomBeginMarker)
+ return "", errors.New(fmt.Sprintf("missing %q marker", bomBeginMarker))
...
- return nil, fmt.Errorf("duplicate row for component %q in the Components table — "+
- "a doubled or hand-edited row cannot shadow the generated value; run `make bom-docs`", name)
+ return nil, errors.New(fmt.Sprintf("duplicate row for component %q in the Components table — "+
+ "a doubled or hand-edited row cannot shadow the generated value; run `make bom-docs`", name))Also applies to: 233-239
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/bom/freshness_test.go` around lines 157 - 168,
`extractGeneratedSection` and the duplicate-row error path in
`parseBOMVersionTable` are still constructing errors with `fmt.Errorf`; switch
those returns to use `pkg/errors` instead, keeping the same messages and control
flow. Update the error imports in this test file accordingly and ensure both the
missing-marker checks in `extractGeneratedSection` and the duplicate-row check
in `parseBOMVersionTable` use the shared error helper consistently.
Source: Coding guidelines
There was a problem hiding this comment.
Leaving these as fmt.Errorf. They are local errors inside a _test.go helper that are consumed immediately by t.Fatalf, never propagated to production callers, so they carry no error code. pkg/errors.New requires a code argument (errors.New(errors.ErrCodeInvalidRequest, msg)); the suggested errors.New(fmt.Sprintf(...)) would not compile. Adding a synthetic code to a test-only sentinel adds noise without value, so fmt.Errorf is the right fit here.
There was a problem hiding this comment.
@yuanchen8911, that makes sense — thanks for the clarification. Since pkg/errors.New requires a structured error code and these errors are purely local to a test helper (consumed immediately by t.Fatalf, never propagated to a caller), fmt.Errorf is the appropriate choice here. I'll leave this as-is.
✏️ Learnings added
Learnt from: yuanchen8911
Repo: NVIDIA/aicr PR: 1580
File: tools/bom/freshness_test.go:157-168
Timestamp: 2026-07-01T19:44:36.763Z
Learning: In the NVIDIA/aicr repo, `pkg/errors.New` requires an error code argument (e.g., `errors.New(errors.ErrCodeInvalidRequest, msg)`). The repo's guideline to use `pkg/errors` instead of `fmt.Errorf` in Go code does not apply to errors created inside `_test.go` helper functions that are purely local (e.g., consumed immediately by `t.Fatalf`/`t.Errorf` and never returned to a production caller), since such errors carry no meaningful error code. `fmt.Errorf` is acceptable for these test-only sentinel errors, as seen in `tools/bom/freshness_test.go`'s `extractGeneratedSection` and `parseBOMVersionTable` duplicate-row check.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: njhensley
Repo: NVIDIA/aicr PR: 595
File: pkg/recipe/loader.go:56-60
Timestamp: 2026-04-16T18:20:53.204Z
Learning: In this repository’s Go code, if an error was already created/wrapped with `pkg/errors` (via `aicrerrors.New(...)` or `aicrerrors.WrapWithContext(...)`) and includes the correct structured error code (e.g., `ErrCodeInvalidRequest`, `ErrCodeTimeout`, `ErrCodeInternal`), then returning it should use the bare form `return nil, err` (or propagate the error as-is). Do not require “missing context” when the source already has proper codes; do not double-wrap it. Avoid changing the error by using `errors.Wrapf` (which can override the structured code) or `fmt.Errorf("%w", err)` in these cases, which is documented as an anti-pattern in `CLAUDE.md`.
Learnt from: mchmarny
Repo: NVIDIA/aicr PR: 728
File: pkg/k8s/agent/job_watch_test.go:140-150
Timestamp: 2026-05-01T23:59:44.270Z
Learning: In Go unit tests, do not require or suggest adding `return` statements immediately after `t.Fatal()` / `t.Fatalf()` calls. These functions internally terminate the current test goroutine via `runtime.Goexit()`, so any explicit `return` afterward is redundant/unreachable. Review should focus on the logic before `t.Fatal*`; the absence of a trailing `return` should not be treated as a problem.
Learnt from: mchmarny
Repo: NVIDIA/aicr PR: 728
File: pkg/server/middleware_test.go:400-402
Timestamp: 2026-05-01T23:59:43.871Z
Learning: In Go test files, don’t require a `return` statement immediately after `t.Fatal(...)`/`t.Fatalf(...)` calls. `t.Fatal` calls `runtime.Goexit()`, so any following `return` is unreachable/redundant (though deferred functions will still run). If the lint/static checks already pass without the explicit `return`, avoid flagging the missing `return` in future reviews (e.g., in this repo’s Go tests).
Learnt from: ArangoGutierrez
Repo: NVIDIA/aicr PR: 884
File: pkg/recipe/doc_test.go:27-45
Timestamp: 2026-05-15T14:08:57.038Z
Learning: When applying the repo’s “table-driven tests” guidance, only require/flag table-driven refactoring if the test is genuinely validating multiple independent cases (different inputs and expected outputs). If the test is effectively a single-invariant guard (e.g., one assertion that related constants match the current source of truth, without multiple scenarios to cover), do not require converting it to table-driven form. In practice: if there’s only one real condition/invariant being checked, allow a single test function with one set of assertions; reserve table-driven recommendations for tests that cover two or more distinct scenarios.
Learnt from: mchmarny
Repo: NVIDIA/aicr PR: 1106
File: pkg/recipe/driver_root_lockstep_test.go:61-62
Timestamp: 2026-05-29T20:38:46.266Z
Learning: In this repository’s Go test files, it is an explicitly permitted convention to use `context.Background()` for test setup (including I/O-heavy helper setup such as `loadMetadataStore`). Do not flag or recommend replacing `context.Background()` with `context.WithTimeout` in test setup code; instead rely on the runner-level `go test -timeout` (default ~10 minutes, or the configured value) to bound wall-clock runtime. Only require timeouts/cancellation patterns for non-test/request-scoped contexts per the repo’s Context Propagation Rules.
Recipe evidence checkNo leaf overlays affected by this PR. This gate is warning-only and never blocks merge. |
yuanchen8911
force-pushed
the
fix/1576-freshness-gate-hardening
branch
from
aab1eb8 to
9e0558b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
tools/bom/freshness_parse_test.go (1)
60-119: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider table-driven form for
TestParseBOMVersionTable.Still expressed as four separate
t.Runblocks rather than a driving table, unlikeTestExtractGeneratedSectionabove in the same file.As per path instructions,
**/*_test.go: "Write table-driven tests when a test covers multiple cases."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/bom/freshness_parse_test.go` around lines 60 - 119, Refactor TestParseBOMVersionTable into a table-driven test instead of four separate t.Run blocks, following the style of TestExtractGeneratedSection. Keep the existing coverage for parseBOMVersionTable cases (valid rows, duplicate component rejection, unrelated table ignored, no table returns empty) by moving each scenario into a single test case struct and iterating over it with t.Run.Source: Path instructions
tools/bom/freshness_test.go (1)
154-168: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winUse
pkg/errorsinstead offmt.Errorffor these error returns.
extractGeneratedSection(Lines 160, 165) and the duplicate-row check inparseBOMVersionTable(Lines 234-235) still construct errors withfmt.Errorf.♻️ Proposed fix
- return "", fmt.Errorf("missing %q marker", bomBeginMarker) + return "", errors.Errorf("missing %q marker", bomBeginMarker) ... - return nil, fmt.Errorf("duplicate row for component %q in the Components table — "+ - "a doubled or hand-edited row cannot shadow the generated value; run `make bom-docs`", name) + return nil, errors.Errorf("duplicate row for component %q in the Components table — "+ + "a doubled or hand-edited row cannot shadow the generated value; run `make bom-docs`", name)As per coding guidelines,
**/*.go: "Usepkg/errorsfor error handling instead offmt.Errorfin Go code."Also applies to: 233-239
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/bom/freshness_test.go` around lines 154 - 168, Update the error construction in extractGeneratedSection and the duplicate-row check in parseBOMVersionTable to use pkg/errors instead of fmt.Errorf. Keep the existing messages and locate the changes in extractGeneratedSection plus the duplicate-row handling in parseBOMVersionTable, replacing the fmt.Errorf returns with the project’s pkg/errors helper consistently throughout this test file.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tools/bom/main.go`:
- Around line 217-228: The strict-mode validation in run only rejects unpinned
Helm results, so unpinned Kustomize components can still slip through. Update
the strict check to use the type-aware pinned state from
surveyComponent/pinnedVersion (or explicitly include kindKustomize alongside
kindHelm) so any result with Pinned=false and an empty version/default tag is
hard-failed under --strict.
---
Duplicate comments:
In `@tools/bom/freshness_parse_test.go`:
- Around line 60-119: Refactor TestParseBOMVersionTable into a table-driven test
instead of four separate t.Run blocks, following the style of
TestExtractGeneratedSection. Keep the existing coverage for parseBOMVersionTable
cases (valid rows, duplicate component rejection, unrelated table ignored, no
table returns empty) by moving each scenario into a single test case struct and
iterating over it with t.Run.
In `@tools/bom/freshness_test.go`:
- Around line 154-168: Update the error construction in extractGeneratedSection
and the duplicate-row check in parseBOMVersionTable to use pkg/errors instead of
fmt.Errorf. Keep the existing messages and locate the changes in
extractGeneratedSection plus the duplicate-row handling in parseBOMVersionTable,
replacing the fmt.Errorf returns with the project’s pkg/errors helper
consistently throughout this test file.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 4cab52b2-83db-4283-8b21-14516dd15d49
📒 Files selected for processing (8)
.github/workflows/merge-gate.yamldocs/contributor/recipe.mdpkg/bom/bom.gopkg/bom/bom_test.gopkg/recipe/version_pin_guard_test.gotools/bom/freshness_parse_test.gotools/bom/freshness_test.gotools/bom/main.go
yuanchen8911
force-pushed
the
fix/1576-freshness-gate-hardening
branch
2 times, most recently
from
78d6b18 to
0624102
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/recipe/version_pin_guard_test.go (1)
255-279: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winVacuous match when the registry default itself is empty.
If
defis""(component has no pinnedHelm.DefaultVersion/Kustomize.DefaultTag), any enabled ref of the matching type with an equally-emptyVersion/Tagsatisfiesactive == def, marking the exemption falsely "installed." This reopens the same class of false-pass this hardening pass is meant to close — just via an empty default rather than a mismatched-type field.🛡️ Suggested guard
regType := cfg.GetType() kustomize := regType == ComponentTypeKustomize def := cfg.Helm.DefaultVersion if kustomize { def = cfg.Kustomize.DefaultTag } + if def == "" { + t.Errorf("unsafe versionPinExemptions entry for %s/%s: registry has no pinned default "+ + "to verify against (empty %s)", e.source, e.component, + map[bool]string{true: "Kustomize.DefaultTag", false: "Helm.DefaultVersion"}[kustomize]) + continue + } installed := false🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/recipe/version_pin_guard_test.go` around lines 255 - 279, The exemption check in version_pin_guard_test is still vacuously true when the registry default value is empty, because installed can be set by any enabled ref whose active Version/Tag is also empty. Update the loop in the proof logic around results, ref.IsEnabled(), and the active field selection so empty def values are excluded from satisfying the match, or otherwise require a non-empty pinned value before treating a ref as installed. Keep the type filter on ref.Type and the active-field comparison, but add a guard for the empty default case before setting installed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/merge-gate.yaml:
- Around line 335-380: Add explicit name fields to the two new workflow jobs so
they are not anonymous in the Actions UI. Update the bom-freshness and
bom-freshness-skip job definitions in merge-gate.yaml to follow the same naming
convention used by sibling jobs like verify-renovate and verify-renovate-skip,
keeping the existing gating logic and steps unchanged.
In `@pkg/bom/bom.go`:
- Around line 154-164: The kustomize type string is duplicated between
pkg/bom.BuildBOM and tools/bom/main.go, which can drift and defeats the
type-safety cleanup. Add a shared exported constant in the bom package for the
kustomize type, update BuildBOM to compare against that constant, and have
tools/bom’s kindKustomize reference the same symbol so both paths use one source
of truth.
---
Outside diff comments:
In `@pkg/recipe/version_pin_guard_test.go`:
- Around line 255-279: The exemption check in version_pin_guard_test is still
vacuously true when the registry default value is empty, because installed can
be set by any enabled ref whose active Version/Tag is also empty. Update the
loop in the proof logic around results, ref.IsEnabled(), and the active field
selection so empty def values are excluded from satisfying the match, or
otherwise require a non-empty pinned value before treating a ref as installed.
Keep the type filter on ref.Type and the active-field comparison, but add a
guard for the empty default case before setting installed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: dfe788da-731e-4f63-944e-379e8b855fde
📒 Files selected for processing (8)
.github/workflows/merge-gate.yamldocs/contributor/recipe.mdpkg/bom/bom.gopkg/bom/bom_test.gopkg/recipe/version_pin_guard_test.gotools/bom/freshness_parse_test.gotools/bom/freshness_test.gotools/bom/main.go
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/bom/bom.go (1)
154-164: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated
"kustomize"string literal across packages, still unaddressed.
BuildBOMcomparesr.Type == "kustomize"here whiletools/bom/main.goindependently defineskindKustomize = "kustomize". A shared exported constant (e.g.bom.TypeKustomize) would prevent the two from silently drifting — the exact class of bug this PR is hardening against elsewhere (type-safe version selection).♻️ Proposed refactor
+// Component type identifiers used across BOM generation. +const ( + TypeHelm = "helm" + TypeKustomize = "kustomize" + TypeManifest = "manifest" +) + if r.Version != "" { versionProp := "aicr:helm:version" - if r.Type == "kustomize" { + if r.Type == TypeKustomize { versionProp = "aicr:kustomize:tag" } props = append(props, cdx.Property{Name: versionProp, Value: r.Version}) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/bom/bom.go` around lines 154 - 164, The BuildBOM logic still hardcodes the "kustomize" type string, duplicating the value already defined in tools/bom/main.go. Replace the literal in BuildBOM and any related type checks with a shared exported constant such as bom.TypeKustomize, so the version-property selection logic stays consistent across packages and cannot drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@pkg/bom/bom.go`:
- Around line 154-164: The BuildBOM logic still hardcodes the "kustomize" type
string, duplicating the value already defined in tools/bom/main.go. Replace the
literal in BuildBOM and any related type checks with a shared exported constant
such as bom.TypeKustomize, so the version-property selection logic stays
consistent across packages and cannot drift.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 5d7a6d11-b0ac-4a4c-a9f2-79fb7db5e1ef
📒 Files selected for processing (8)
.github/workflows/merge-gate.yamldocs/contributor/recipe.mdpkg/bom/bom.gopkg/bom/bom_test.gopkg/recipe/version_pin_guard_test.gotools/bom/freshness_parse_test.gotools/bom/freshness_test.gotools/bom/main.go
yuanchen8911
force-pushed
the
fix/1576-freshness-gate-hardening
branch
from
0624102 to
8397a8b
Compare
yuanchen8911
force-pushed
the
fix/1576-freshness-gate-hardening
branch
from
8397a8b to
fd3273c
Compare
|
Follow-up (out of scope for this PR): resolver rejects ComponentRefs with incoherent type/field combinations. Several review findings on this PR trace to one root cause: nothing validates that a resolved The proper fix is resolver-level validation: enforce coherence once in It is a separate, self-contained change (a small Tracking here until a dedicated issue is filed. |
yuanchen8911
force-pushed
the
fix/1576-freshness-gate-hardening
branch
from
fd3273c to
d111978
Compare
|
Follow-up now tracked as #1584 (Bug, P2) — resolver-level validation of ComponentRef type/field coherence. |
yuanchen8911
force-pushed
the
fix/1576-freshness-gate-hardening
branch
from
d111978 to
2aca6e8
Compare
yuanchen8911
force-pushed
the
fix/1576-freshness-gate-hardening
branch
from
2aca6e8 to
422d7c0
Compare
yuanchen8911
force-pushed
the
fix/1576-freshness-gate-hardening
branch
from
422d7c0 to
c8256f2
Compare
Follow-up hardening for the version-freshness gate added in NVIDIA#1572. The gate's core path (a registry bump that forgets `make bom-docs` fails CI) works; this closes edge cases where it could false-pass or misbehave. Freshness check (tools/bom/freshness_test.go) — make the committed BOM an exact registry projection: parse only the generated AICR-BOM marker section (requiring exactly one begin/end pair, so a second stale section cannot be appended), reject duplicate component rows, compare the registry and doc component sets bidirectionally, and check EVERY row — pinned components by effective type, unpinned components against the "—" sentinel so a fabricated version on an unpinned row cannot slip through. Merge gate (.github/workflows/merge-gate.yaml) — run the freshness check on docs-only PRs, which skip the full tests job. Add a path-filtered bom-freshness job (gated on the BOM doc, registry, and generator sources) plus its skip companion, wired into the aggregate gate. The job asserts the named test actually executed, so a future rename/removal fails the gate instead of silently passing (`go test -run` that matches nothing exits 0). Type-safe BOM metadata (tools/bom/main.go, pkg/bom/bom.go, pkg/evidence/attestation/bom.go) — the standalone generator and the recipe-bound attestation builder both emit the pinned version and source by effective component type (Helm defaultVersion/repository or Kustomize defaultTag/source), and pkg/bom names the CycloneDX properties by type (aicr:kustomize:tag / :source / :namespace vs aicr:helm:*, matched case-insensitively) so Kustomize metadata is not mislabeled as Helm. Strict pinning stays Helm-only per ADR-006. Exemption sole-consumer proof (pkg/recipe/version_pin_guard_test.go) — select the active field by the registry type and fail closed when a resolved ref's declared type OR its deploy shape (a Tag/Path that the deployers classify as Kustomize, or a Kustomize ref lacking the Path localformat requires) is inconsistent with the registry type, so a ref cannot satisfy the proof via an inactive inherited field. Adds unit tests for marker extraction (including duplicate markers), the table parser, and the type-named version/source/namespace properties. Reconciles the BOM-freshness guidance across docs/contributor/recipe.md, docs/contributor/tests.md, ADR-006, and .claude/CLAUDE.md / AGENTS.md: name/version and component-set freshness is now gated; only rendered-image drift and full `make bom-check` remain opt-in.
yuanchen8911
force-pushed
the
fix/1576-freshness-gate-hardening
branch
from
c8256f2 to
2a4ba9c
Compare
2 participants
Summary
Follow-up hardening for the BOM version-freshness gate added in #1572. Closes three edge cases in the new gate where it could false-pass or misbehave: the committed-BOM check is now an exact bidirectional projection, it runs on docs-only PRs, and version selection is deployment-type-safe.
Motivation / Context
#1572 satisfied #1424 for the realistic threat (a registry bump that forgets
make bom-docsfails CI). A cross-review of the merged gate surfaced three residual edge cases, tracked in #1576, none of which blocked #1572 but all of which weaken the gate:TestCommittedBOMVersionsMatchRegistrychecked only registry → doc and scanned every pipe table in the file, so a stale row for a removed component passed, and a duplicate row could shadow the generated value.testsjob, so a PR editing onlydocs/user/container-images.mdnever ran the freshness check despite the docs claiming freshness is gated.Kustomize.DefaultTag(so the first Kustomize component would fail aftermake bom-docs), and the exemption sole-consumer proof accepted eitherVersionorTag(an inactive field could spoof a default installation).Fixes: #1576
Related: #1424, #1572
Type of Change
Component(s) Affected
pkg/recipe)docs/,examples/)tools/bom), merge-gate workflow (.github/workflows)Implementation Notes
tools/bom/freshness_test.go) — parse only the generated<\!-- BEGIN/END AICR-BOM -->marker section; reject duplicate component rows; compare the registry and doc component sets bidirectionally (missing row and orphan row both fail) before comparing versions. Addsfreshness_parse_test.gounit tests for marker extraction and the table parser..github/workflows/merge-gate.yaml) — add a path-filteredbom-freshnessjob (+ skip companion, wired into the aggregate gate) that runsTestCommittedBOMVersionsMatchRegistrywhendocs/user/container-images.md,recipes/registry.yaml, or the generator sources change, so the gate holds on docs-only PRs.tools/bom/main.go,pkg/recipe/version_pin_guard_test.go) — the generator emits the pinned version by effective component type (HelmdefaultVersionor KustomizedefaultTag); the exemption proof selects the active field by type instead of accepting either.docs/contributor/recipe.mdupdated to describe the bidirectional check and the docs-only gate coverage.Testing
TestCommittedBOMVersionsMatchRegistryverifies 25 pinned components bidirectionally against the committed BOM;TestOverlayVersionPinsMatchRegistryverifies 38 pins (one declared exemption). Draft pending fullmake qualify.Risk Assessment
defaultTag), and the committed BOM regenerates with zero diff.Rollout notes: N/A — no runtime behavior change.
Checklist
make testwith-race)make lint)git commit -S)