fix(images): resolve provenance-wrapped base manifests - #8257
Conversation
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
apurvvkumaria
added
area: security
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe workflow stages multi-platform image candidates under run-specific tags, validates source provenance and managed indexes, exports verified platform digests, and promotes tags only when the published digest matches the validated candidate. Tests cover provenance failures, workflow ordering, and watch triggers. ChangesManaged image publication
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant BaseImageWorkflow
participant ContainerRegistry
participant Validator
participant ManagedImageContract
BaseImageWorkflow->>ContainerRegistry: Create candidate manifest
ContainerRegistry-->>BaseImageWorkflow: Return candidate digest
BaseImageWorkflow->>Validator: Validate candidate and source indexes
Validator-->>BaseImageWorkflow: Return verified platform digests
BaseImageWorkflow->>ManagedImageContract: Export verified digests
BaseImageWorkflow->>ContainerRegistry: Promote candidate to final tags
ContainerRegistry-->>BaseImageWorkflow: Return published digest
BaseImageWorkflow->>BaseImageWorkflow: Compare published and candidate digests
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit f5e6b97 in the TypeScript / code-coverage/cliThe overall coverage in commit f5e6b97 in the Show a code coverage summary of the most impacted files.
Updated |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (5)
scripts/checks/validate-managed-base-index.sh (1)
134-137: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDerive the emitted platform map from
platform_digestsinstead of repeating the architecture list.The architecture inventory now appears three times in this file: line 30, line 110, and lines 135-136. A future third architecture requires three edits, and a missed edit is silent for the output map. Build the output from the associative array.
♻️ Proposed refactor
-jq -cn \ - --arg amd64 "${platform_digests[amd64]}" \ - --arg arm64 "${platform_digests[arm64]}" \ - '{"linux/amd64": $amd64, "linux/arm64": $arm64}' +platform_map='{}' +for arch in "${!platform_digests[@]}"; do + platform_map="$( + jq -cn \ + --argjson map "$platform_map" \ + --arg platform "linux/$arch" \ + --arg digest "${platform_digests[$arch]}" \ + '$map + {($platform): $digest}' + )" +done +printf '%s\n' "$platform_map"As per path instructions for
scripts/checks/**: "Derive inventories and limits from a canonical source where possible; flag duplicated lists that can silently drift."🤖 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 `@scripts/checks/validate-managed-base-index.sh` around lines 134 - 137, Update the JSON construction following the platform_digests population to derive the emitted platform map directly from the associative array, rather than hard-coding amd64 and arm64 in separate jq arguments. Preserve the existing digest-to-platform mapping and ensure newly added platform entries are included automatically.Source: Path instructions
test/dcode-base-image-workflow.test.ts (1)
480-487: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo workflow test covers the published-digest guard, which is the safety property this PR adds. Both suites assert that staging precedes validation and that validation precedes promotion, but neither asserts the comparison that rejects a published digest different from the validated candidate. Deleting that comparison from
.github/workflows/base-image.yamlwould keep both suites green.
test/dcode-base-image-workflow.test.ts#L480-L487: addexpect(openClawManifestScript).toContain("published_digest=")and an assertion for the inequality guard, then repeat both inside the Hermes and Deep Agents Code loop at lines 606-630.test/managed-image-publication-workflow.test.ts#L381-L386: add the same two assertions againstmanifestRunfor every expected publisher.As per path instructions for
**/*.test.{ts,js,mts,mjs,cts,cjs}: "Flag copied production algorithms, broad mocks that bypass the behavior under test, and conditionals that make a test pass without exercising its claim."🤖 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 `@test/dcode-base-image-workflow.test.ts` around lines 480 - 487, The workflow tests do not verify the published-digest mismatch guard. In test/dcode-base-image-workflow.test.ts:480-487, add assertions that openClawManifestScript contains published_digest= and the inequality comparison, then add the same assertions inside the Hermes and Deep Agents Code loop at lines 606-630. In test/managed-image-publication-workflow.test.ts:381-386, add both assertions against manifestRun for every expected publisher.Source: Path instructions
.github/workflows/base-image.yaml (2)
539-544: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe staged candidate tag is never removed.
Line 539 creates a durable registry tag
base-candidate-${GITHUB_RUN_ID}-${GITHUB_RUN_ATTEMPT}. Nothing deletes it after promotion. Every run and every retry adds one permanent tag per image, for three images. The GHCR package tag list grows without bound, and the candidate tags become an alternative pull surface that no consumer contract covers.Add a cleanup step that deletes the candidate tag after the digest-preserving promotion check succeeds, and run it with
if: always()so failed runs also clean up. The same gap exists in the Deep Agents Code and OpenClaw jobs; I will list all three sites in the consolidated comment.🤖 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 @.github/workflows/base-image.yaml around lines 539 - 544, The workflow must delete the staged candidate tag after promotion verification, including when the job fails. Add an `if: always()` cleanup step for the candidate created near `candidate_tag`, and apply the same cleanup to the corresponding candidate-tag flows in the Deep Agents Code and OpenClaw jobs, using the existing tag and registry authentication context.
561-572: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftOne staging, validation, and promotion sequence is copied into three publisher jobs. The three
runbodies are identical except for the agent name in two error messages, so any change to the promotion contract needs three identical edits and three identical test-string updates.
.github/workflows/base-image.yaml#L561-L572: move the Hermes staging, validation, digest-extraction, and promotion sequence into a shared script such asscripts/publish-managed-base-index.sh, and call it withAGENT,IMAGE, and the tag list..github/workflows/base-image.yaml#L728-L739: call the same shared script from the Deep Agents Code job..github/workflows/base-image.yaml#L897-L908: call the same shared script from the OpenClaw job.Do you want me to draft the shared script and the three call sites?
🤖 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 @.github/workflows/base-image.yaml around lines 561 - 572, Extract the duplicated Hermes staging, validation, digest extraction, and promotion sequence into a shared scripts/publish-managed-base-index.sh accepting AGENT, IMAGE, and the tag list, then replace the inline logic at .github/workflows/base-image.yaml lines 561-572, 728-739, and 897-908 with calls to that script; update each job’s agent-specific error messages and test strings as needed.test/validate-managed-base-index.test.ts (1)
120-198: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the changed argument contract.
This PR changes the validator arguments to
<index-reference> <linux-amd64-source-digest> <linux-arm64-source-digest>and adds the source-digest format check. No test exercises those branches. A caller that swaps or malforms an argument stays undetected by this suite.runValidatorcurrently hardcodes the arguments, so the tests cannot reach the guards at lines 16-25 ofscripts/checks/validate-managed-base-index.sh.Let
runValidatoraccept argument overrides, then assert the two rejection messages.♻️ Proposed test additions
+ it("rejects a mutable managed base reference (`#7744`)", () => { + const mutable = runValidator({ reference: `${image}:latest` }); + + expect(mutable.status).not.toBe(0); + expect(mutable.stderr).toContain("managed base index reference must be immutable"); + }); + + it("rejects a malformed platform source digest (`#7744`)", () => { + const malformed = runValidator({ amd64SourceDigestArgument: "sha256:not-a-digest" }); + + expect(malformed.status).not.toBe(0); + expect(malformed.stderr).toContain("managed base platform source digest is invalid"); + });As per path instructions for
scripts/checks/**: "Require focused tests for both detection and false-positive behavior."🤖 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 `@test/validate-managed-base-index.test.ts` around lines 120 - 198, Extend the runValidator test helper to accept index-reference and linux/amd64/linux/arm64 source-digest argument overrides instead of hardcoding them. Add focused tests asserting malformed source-digest arguments trigger the validator’s source-digest format rejection and swapped source-digest arguments trigger the appropriate mismatch rejection, while preserving the existing valid invocation coverage.Source: Path instructions
🤖 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.
Nitpick comments:
In @.github/workflows/base-image.yaml:
- Around line 539-544: The workflow must delete the staged candidate tag after
promotion verification, including when the job fails. Add an `if: always()`
cleanup step for the candidate created near `candidate_tag`, and apply the same
cleanup to the corresponding candidate-tag flows in the Deep Agents Code and
OpenClaw jobs, using the existing tag and registry authentication context.
- Around line 561-572: Extract the duplicated Hermes staging, validation, digest
extraction, and promotion sequence into a shared
scripts/publish-managed-base-index.sh accepting AGENT, IMAGE, and the tag list,
then replace the inline logic at .github/workflows/base-image.yaml lines
561-572, 728-739, and 897-908 with calls to that script; update each job’s
agent-specific error messages and test strings as needed.
In `@scripts/checks/validate-managed-base-index.sh`:
- Around line 134-137: Update the JSON construction following the
platform_digests population to derive the emitted platform map directly from the
associative array, rather than hard-coding amd64 and arm64 in separate jq
arguments. Preserve the existing digest-to-platform mapping and ensure newly
added platform entries are included automatically.
In `@test/dcode-base-image-workflow.test.ts`:
- Around line 480-487: The workflow tests do not verify the published-digest
mismatch guard. In test/dcode-base-image-workflow.test.ts:480-487, add
assertions that openClawManifestScript contains published_digest= and the
inequality comparison, then add the same assertions inside the Hermes and Deep
Agents Code loop at lines 606-630. In
test/managed-image-publication-workflow.test.ts:381-386, add both assertions
against manifestRun for every expected publisher.
In `@test/validate-managed-base-index.test.ts`:
- Around line 120-198: Extend the runValidator test helper to accept
index-reference and linux/amd64/linux/arm64 source-digest argument overrides
instead of hardcoding them. Add focused tests asserting malformed source-digest
arguments trigger the validator’s source-digest format rejection and swapped
source-digest arguments trigger the appropriate mismatch rejection, while
preserving the existing valid invocation coverage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 9a3f2714-9165-46b6-9e1d-7732a1af6986
📒 Files selected for processing (7)
.github/workflows/base-image.yamlscripts/checks/validate-managed-base-index.shtest/dcode-base-image-workflow.test.tstest/helpers/vitest-watch-triggers.tstest/managed-image-publication-workflow.test.tstest/validate-managed-base-index.test.tstest/vitest-watch-triggers.test.ts
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
Second-opinion terminology and E2E selections are advisory. They do not change the primary assessment or E2E / PR Gate. 2 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: 1 optional E2E recommendation
This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
2 participants
Summary
Corrects base-image publication for provenance-bearing single-platform indexes. The workflow now validates runnable descriptors and provenance before it updates consumer tags.
Changes
The shared validator is required by the OpenClaw, Hermes, and Deep Agents Code publishers. One implementation prevents the three publication paths from applying different provenance rules.
Type of Change
Quality Gates
Documentation Writer Review
no-docs-neededDGX Station Hardware Evidence
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailablenpm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result: Not applicable to this internal workflow correction.npm run checks:repositorypassed.npm run docsbuilds without warnings (doc changes only)Failure evidence: first affected base-image run and release-blocking base-image run.
Signed-off-by: Apurv Kumaria akumaria@nvidia.com