feat(release): surface validated Brev image evidence - #8852
Conversation
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
📝 WalkthroughWalkthroughThe release workflow now uses ordinary E2E evidence for the exact candidate ledger and validates release images separately. Trusted v2 receipts, durable evidence paths, image provenance, ancestry, commit distance, and maintainer image selection are enforced. ChangesRelease evidence qualification
Estimated code review effort: 4 (Complex) | ~60 minutes 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 d675d79 in the TypeScript / code-coverage/cliThe overall coverage in commit d675d79 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.agents/skills/nemoclaw-maintainer-e2e/SKILL.md (1)
336-370: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDerive
EVIDENCE_MODEbefore validation.Lines 353-370 use the legacy full-mode validator when
EVIDENCE_MODEis unset. Lines 400-407 derive the mode only after that command. A valid selective Launchable or push artifact can therefore fail the full-mode selector checks before image validation starts.Read
dispatch.jsonafter artifact download, deriveEVIDENCE_MODE, and pass--modein the same validation procedure.Also applies to: 398-407
🤖 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 @.agents/skills/nemoclaw-maintainer-e2e/SKILL.md around lines 336 - 370, The validation flow must derive EVIDENCE_MODE before invoking validate-full-e2e-evidence. After downloading the artifact and before constructing VALIDATOR_MODE_ARGS or running the validator, read FULL_E2E_DIR/dispatch.json and determine whether the evidence is full, launchable, or push; then pass the derived --mode value for selective Launchable and push candidates while preserving the legacy full-run behavior when no mode applies. Remove or relocate the later derivation near the existing validation logic.
🧹 Nitpick comments (1)
tools/e2e/workflow-boundary.mts (1)
4214-4219: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winEnforce boolean serialization in the boundary check.
The workflow uses
--argjsonfor these fields on Lines 744-746, but this validator only requires the object properties. A future change to--argwould serialize"false"or"true"as strings and still pass this check. The supplied receipt fixture intest/openshell-e2e-qualification.test.tsexpects JSON booleans. Require the three--argjsonfragments here and add a regression case that rejects--arg.Proposed validator guard
for (const required of [ 'kind: "nemoclaw-e2e-dispatch-v1"', "candidateSha: $candidateSha", "eventName: $eventName", "workflowRunId: $workflowRunId", "workflowRunAttempt: $workflowRunAttempt", + '--argjson allowDgxSparkRunnerQueue "$ALLOW_DGX_SPARK_RUNNER_QUEUE"', + '--argjson allowJetsonDispatch "$ALLOW_JETSON_DISPATCH"', + '--argjson allowJetsonRunnerQueue "$ALLOW_JETSON_RUNNER_QUEUE"', "allowDgxSparkRunnerQueue: $allowDgxSparkRunnerQueue", "allowJetsonDispatch: $allowJetsonDispatch", "allowJetsonRunnerQueue: $allowJetsonRunnerQueue",Also applies to: 4242-4244
🤖 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/e2e/workflow-boundary.mts` around lines 4214 - 4219, Update the boundary validator checks around the ALLOW_DGX_SPARK_RUNNER_QUEUE, ALLOW_JETSON_DISPATCH, and ALLOW_JETSON_RUNNER_QUEUE entries to require their corresponding jq invocations use --argjson, not --arg, so serialized values remain JSON booleans. Apply the same requirement to the additional matching entries noted in the comment, and add a regression case in the relevant qualification test that rejects --arg for these fields.
🤖 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.
Outside diff comments:
In @.agents/skills/nemoclaw-maintainer-e2e/SKILL.md:
- Around line 336-370: The validation flow must derive EVIDENCE_MODE before
invoking validate-full-e2e-evidence. After downloading the artifact and before
constructing VALIDATOR_MODE_ARGS or running the validator, read
FULL_E2E_DIR/dispatch.json and determine whether the evidence is full,
launchable, or push; then pass the derived --mode value for selective Launchable
and push candidates while preserving the legacy full-run behavior when no mode
applies. Remove or relocate the later derivation near the existing validation
logic.
---
Nitpick comments:
In `@tools/e2e/workflow-boundary.mts`:
- Around line 4214-4219: Update the boundary validator checks around the
ALLOW_DGX_SPARK_RUNNER_QUEUE, ALLOW_JETSON_DISPATCH, and
ALLOW_JETSON_RUNNER_QUEUE entries to require their corresponding jq invocations
use --argjson, not --arg, so serialized values remain JSON booleans. Apply the
same requirement to the additional matching entries noted in the comment, and
add a regression case in the relevant qualification test that rejects --arg for
these fields.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a26d6771-6a96-4d86-836d-60e8a5094b7b
📒 Files selected for processing (14)
.agents/skills/nemoclaw-maintainer-cut-release-tag/SKILL.md.agents/skills/nemoclaw-maintainer-cut-release-tag/scripts/release-e2e-evidence.mts.agents/skills/nemoclaw-maintainer-e2e/SKILL.md.agents/skills/nemoclaw-maintainer-e2e/scripts/validate-full-e2e-evidence.mts.agents/skills/nemoclaw-maintainer-evening/SKILL.md.agents/skills/nemoclaw-maintainer-policies/references/release-train.md.github/workflows/e2e.yamltest/brev-launchable-e2e.test.tstest/e2e/README.mdtest/maintainer-e2e-skill.test.tstest/maintainer-skills-policy.test.tstest/release-e2e-evidence.test.tstools/e2e/brev-launchable-e2e.shtools/e2e/workflow-boundary.mts
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests. Since last review: 1 prior item resolved · 0 still apply · 0 new items found 3 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. A maintainer can dispatch the default E2E suite for the commit under review. Recommended E2E: None Manual-only E2E: 1 optional E2E recommendation
This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
prekshivyas
left a comment
prekshivyas
left a comment
There was a problem hiding this comment.
Blocking procedure finding on latest PR commit d675d7928.
The release-image path invokes validate-full-e2e-evidence.mts at .agents/skills/nemoclaw-maintainer-e2e/SKILL.md:353-366 before the procedure derives EVIDENCE_MODE from the downloaded receipt at lines 398-407. A maintainer following the executable block with the variable unset enters the legacy full-run validator; valid selective Launchable and push receipts are then evaluated under the wrong event/selector contract. The intended stricter direct-main validation is also not selected by the block itself.
Please derive and validate the mode from run-$RUN_ID.json plus dispatch.json before constructing the validator arguments, or provide one command that derives and validates the mode atomically. Add contract coverage for the documented execution order for full, selective Launchable, and push receipts.
Security rubric: 1 PASS (no credential flow added); 2 PASS (receipt fields are constrained); 3 PASS (trusted-project and direct-main authority is modeled); 4 PASS (no dependency change); 5 PASS (invalid receipts fail closed); 6 PASS (no cryptography change); 7 PASS (workflow defaults remain restrictive); 8 WARNING (tests do not exercise the documented command sequence); 9 WARNING (the mode control is ordered after its consumer). Files reviewed: the complete 14-file diff.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/brev-launchable-e2e.test.ts (1)
422-427: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert that the SSH readiness probe ran.
This test can pass if an earlier failure emits
"host SSH readiness timed out"without executing an SSH readiness probe. Assert at least one recorded SSH readiness attempt before checking that E2E did not run.Proposed test change
expect(result.status).not.toBe(0); expect(result.stderr).toContain("host SSH readiness timed out"); + expect(fs.readFileSync(calls, "utf8")).toContain("ssh host readiness attempt 1:"); expect(fs.readFileSync(calls, "utf8")).not.toMatch(/brev exec|full-e2e\.test\.ts/u);As per path instructions, review tests for behavioral confidence rather than implementation lock-in.
🤖 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/brev-launchable-e2e.test.ts` around lines 422 - 427, Update the test around fixture() and run() to assert that the recorded calls contain at least one SSH readiness probe before asserting that E2E execution did not occur. Use the existing calls log and its established readiness-probe signature, preserving the current timeout and workspace-cleanup assertions without coupling the test to unrelated implementation details.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.
Outside diff comments:
In `@test/brev-launchable-e2e.test.ts`:
- Around line 422-427: Update the test around fixture() and run() to assert that
the recorded calls contain at least one SSH readiness probe before asserting
that E2E execution did not occur. Use the existing calls log and its established
readiness-probe signature, preserving the current timeout and workspace-cleanup
assertions without coupling the test to unrelated implementation details.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f22955c8-8e58-4c60-bd31-f33dad8f3a87
📒 Files selected for processing (2)
test/brev-launchable-e2e.test.tstools/e2e/brev-launchable-e2e.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- tools/e2e/brev-launchable-e2e.sh
prekshivyas
left a comment
prekshivyas
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 0fc45b8696 after the merge commit. The blocking procedure defect remains unchanged: the executable block invokes validate-full-e2e-evidence.mts using EVIDENCE_MODE before the later selection procedure derives that mode from dispatch.json. A maintainer following the documented order can still validate a selective Launchable or push receipt under the legacy full contract.
Derive and validate full, launchable, or push from the trusted run and dispatch receipts before constructing VALIDATOR_MODE_ARGS, or make derivation and validation one atomic command. Add contract coverage for the documented execution order. Exact-head CI is green, but this code-path blocker prevents approval.
wscurran
added
area: ci
prekshivyas
left a comment
prekshivyas
left a comment
There was a problem hiding this comment.
Re-reviewed remote head 0fc45b869 against current main at 93461f3fa.
The documented execution-order defect remains on the remote head: the procedure consumes EVIDENCE_MODE before deriving it. I validated a minimal local correction that derives full, launchable, or push before validator argument construction; its focused policy, workflow-boundary, and Brev tests pass.
That correction cannot be pushed safely onto the current base. Since this PR was last synchronized, main merged the release-qualification and E2E consolidation work. The current base deletes both evidence scripts modified here and moves their responsibilities into the consolidated workflow. A base sync now produces semantic conflicts across the release skills, E2E workflow, validator, and tests.
Maintainer direction is required before further implementation: decide whether this historical Brev image-evidence feature remains part of the consolidated release design and, if so, define its current owner and contract. Do not reintroduce the deleted scripts or document the old flow as canonical without that decision. The PR remains changes requested and is not ready for approval.
senthilr-nv
left a comment
senthilr-nv
left a comment
There was a problem hiding this comment.
Review of commit 0fc45b8696a1498f56114a07e4e53ab1b2c8a590: this PR is not approval-ready.
The release-evidence design is based on an older workflow and maintainer-skill contract. The branch now conflicts with current main, and the required changes result is skipped. Resolving these conflicts would require choosing the current release evidence policy and cannot be treated as a mechanical refresh.
Reconstruct the change from current main only after confirming the accepted release evidence contract, then rerun the complete workflow, test, documentation, security, CodeRabbit, and advisor gates. Do not merge the current conflicting branch.
4 participants
Summary
Release preparation now separates the exact-candidate ordinary E2E ledger from historically validated Launchable image evidence. Maintainers see immutable GCP identity, workflow and job timestamps, and the commit distance to the planned release before selecting that evidence or requesting a fresh selective Launchable run. This change is evidence-only: the current
lkgpath still rebuilds, and exact promotion requires downstream support plus a liveREADYand identity check.Changes
launchable-e2e.jsonartifact.mainLaunchable evidence from full, selective, and push runs while rejecting PR-shaped, stale-identity, malformed, and incomplete receipts.mainv2 receipt for that ledger.git rev-listcommit distance, offer prior evidence or a fresh selective run, and repeat discovery and choice iforigin/mainmoves.lkgworkflow.Type of Change
Quality Gates
mainreceipt identity, exact trusted-project image provenance, successful Launchable E2E, and verified cleanup; it also states that historical evidence is not proof of current GCP image existence or promotion.Documentation Writer Review
docs-updated.agents/skills/nemoclaw-maintainer-cut-release-tag/SKILL.md,.agents/skills/nemoclaw-maintainer-e2e/SKILL.md,.agents/skills/nemoclaw-maintainer-evening/SKILL.md,.agents/skills/nemoclaw-maintainer-policies/references/release-train.md, andtest/e2e/README.md.DGX 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 required for this focused internal workflow, evidence-validator, and maintainer-procedure change.npm run checks:repositorypassed.npm run docsbuilds without warnings (doc changes only) — result: Passed with 0 errors; Fern reported two existing hidden warnings.Signed-off-by: Julie Yaunches jyaunches@nvidia.com
Summary by CodeRabbit
Release Workflow
Validation
Documentation