perf(ci): shorten main and E2E feedback loops - #7262
Conversation
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
|
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:
📝 WalkthroughWalkthroughThe PR updates exact-diff E2E gate handling, Hermes sandbox image workflows, Brev provisioning retries, CI sharding and timeouts, and related tests and documentation. The former required polling job is removed in favor of direct required-check seeding and validation. ChangesE2E gate lifecycle
Hermes sandbox workflow
Brev provisioning
CI execution and reliability
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
PR Review Advisor — InformationalAdvisor assessment: Informational / medium confidence Model lanes
Nemotron output stays in workflow artifacts and does not change the assessment above. 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/sandbox-images-and-e2e.yaml (1)
213-266: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winConsumer job timeout has zero headroom beyond the two probe budgets.
test-hermes-sandbox-imagekeepstimeout-minutes: 75while the two probe steps alone are budgeted at 45 (secret boundary) + 30 (root entrypoint) = 75 minutes. That leaves no headroom for checkout,Set up Node,Install root dependencies, artifact download/load, and both artifact-upload steps. If either probe runs anywhere near its own budget, the job will hit the overall timeout and get killed mid-upload even though the probes themselves stayed within their allotted time — a spurious CI failure that blocks merges.Since
tools/e2e/sandbox-images-workflow-boundary.mts(Lines 676-678, 705-707, 711-712) pins these exact numbers, bumping the job timeout to give real headroom (e.g. 85-90 minutes) requires updating the validator's expected constant in the same PR.🤖 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/sandbox-images-and-e2e.yaml around lines 213 - 266, Increase the test-hermes-sandbox-image job’s timeout-minutes above the combined 45- and 30-minute probe budgets, providing headroom for setup, image handling, and artifact uploads. Update the corresponding expected timeout constant in the sandbox-images-workflow-boundary validator so validation matches the new job timeout.
🤖 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 @.agents/skills/nemoclaw-maintainer-day/scripts/check-gates.ts:
- Line 342: Update the check selection logic around checkNames and the
exactChecks.length guard to prefer the current “E2E / PR Gate” match, falling
back to “E2E / PR Gate Coordination” only when no current match exists. Preserve
exact-diff validation for the selected match, and add a regression test covering
both names with the same external_id.
In `@test/e2e/brev-e2e.test.ts`:
- Around line 569-570: Update the attempt-count normalization near
configuredAttempts and maxAttempts to accept only finite positive integers; fall
back to the default of two attempts for zero, negative, fractional, or otherwise
invalid BRE V_PROVISION_ATTEMPTS values, while preserving valid configured
counts.
- Around line 175-201: Update inspectBrevInstances so JSON is marked
authoritative only when parsed is a recognized instance array or contains a
recognized workspaces array; for unrecognized shapes such as {}, return
authoritative: false or fall back to parseBrevListOutput(brev("ls")). Preserve
authoritative inventories for recognized shapes so Brev branch validation can
safely make early-stop decisions.
---
Outside diff comments:
In @.github/workflows/sandbox-images-and-e2e.yaml:
- Around line 213-266: Increase the test-hermes-sandbox-image job’s
timeout-minutes above the combined 45- and 30-minute probe budgets, providing
headroom for setup, image handling, and artifact uploads. Update the
corresponding expected timeout constant in the sandbox-images-workflow-boundary
validator so validation matches the new job timeout.
🪄 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: CHILL
Plan: Enterprise
Run ID: fa68826b-321c-4b42-b6eb-5b22f3311e8c
📒 Files selected for processing (35)
.agents/skills/nemoclaw-maintainer-day/scripts/check-gates.ts.github/workflows/e2e.yaml.github/workflows/main.yaml.github/workflows/platform-vitest-main.yaml.github/workflows/pr-e2e-gate.yaml.github/workflows/sandbox-images-and-e2e.yamlsrc/lib/actions/uninstall/hermes-forward-watcher-installer.test.tssrc/lib/shields/flow.test.tstest/brev-provisioning-state.test.tstest/e2e/README.mdtest/e2e/brev-e2e.test.tstest/e2e/docs/README.mdtest/e2e/support/e2e-workflow.test.tstest/e2e/support/hermes-secret-boundary-workflow.test.tstest/e2e/support/sandbox-images-workflow-boundary.test.tstest/helpers/vitest-watch-triggers.tstest/hermes-runtime-config-guard-topology.test.tstest/platform-vitest-main-workflow.test.tstest/pr-e2e-gate-fork-skip.test.tstest/pr-e2e-gate-lifecycle.test.tstest/pr-e2e-gate-retry-history.test.tstest/pr-e2e-gate-workflow.test.tstest/pr-e2e-gate.test.tstest/pr-e2e-required.test.tstest/pr-risk-plan.test.tstest/pr-workflow-contract.test.tstest/release-latest-tag.test.tstest/skills/check-gates-evidence.test.tstest/skills/check-gates-test-fixtures.tstest/vitest-watch-triggers.test.tstools/advisors/risk-plan.mtstools/e2e/brev-provisioning.mtstools/e2e/pr-e2e-gate.mtstools/e2e/pr-e2e-required.mtstools/e2e/sandbox-images-workflow-boundary.mts
💤 Files with no reviewable changes (4)
- test/pr-e2e-required.test.ts
- tools/advisors/risk-plan.mts
- test/vitest-watch-triggers.test.ts
- tools/e2e/pr-e2e-required.mts
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit 423ad38 in the TypeScript / code-coverage/cliThe overall coverage in commit 423ad38 in the Show a code coverage summary of the most impacted files.
Updated |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/brev-provisioning-state.test.ts`:
- Around line 47-74: Extend the test case for observeBrevProvisioningProgress to
invoke attempt 3 with the same failure state, assert it returns the expected
result, and verify inspect has been called twice. Preserve the existing attempt
1 inspection and attempt 2 skip assertions.
🪄 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: CHILL
Plan: Enterprise
Run ID: 414cd60d-44d8-40a3-bf33-9edaed97dea7
📒 Files selected for processing (4)
.github/workflows/e2e.yamltest/brev-provisioning-state.test.tstest/e2e/brev-e2e.test.tstools/e2e/brev-provisioning.mts
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/e2e.yaml
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
|
Addressed all automated review findings in 3da8b5b and 7766a8e. In addition to the inline fixes, the Hermes consumer timeout is now 90 minutes, leaving 15 minutes of setup/artifact headroom beyond its 45- and 30-minute probes; the workflow validator, tests, and internal E2E docs were updated together. Focused review regressions pass (132 tests), and npm run check:diff is green. |
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 `@test/skills/check-gates-rollout.test.ts`:
- Around line 23-26: Update the test assertion around runGate to verify the
selected check identity, not only allPass and the CI pass status. Assert
trustedCustomCheckId equals 8000, or expose and assert the chosen check name/ID
through the fixture, while preserving the existing success assertions.
In `@tools/e2e/brev-provisioning.mts`:
- Around line 38-74: Remove the local parseBrevJsonInventory and
parseBrevProvisioningAttempts declarations from the E2E flow and import the
shared exports from the provisioning helpers. Update evaluator and observer
callers similarly so all paths use the shared implementations and their tested
parsing and retry behavior.
🪄 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: CHILL
Plan: Enterprise
Run ID: 1d2ae93c-91b1-450e-89b5-1c79b9e6aa21
📒 Files selected for processing (10)
.agents/skills/nemoclaw-maintainer-day/scripts/check-gates.ts.github/workflows/sandbox-images-and-e2e.yamltest/brev-provisioning-state.test.tstest/e2e/README.mdtest/e2e/brev-e2e.test.tstest/e2e/docs/README.mdtest/e2e/support/sandbox-images-workflow-boundary.test.tstest/skills/check-gates-rollout.test.tstools/e2e/brev-provisioning.mtstools/e2e/sandbox-images-workflow-boundary.mts
🚧 Files skipped from review as they are similar to previous changes (5)
- test/e2e/README.md
- test/e2e/brev-e2e.test.ts
- .agents/skills/nemoclaw-maintainer-day/scripts/check-gates.ts
- test/e2e/support/sandbox-images-workflow-boundary.test.ts
- tools/e2e/sandbox-images-workflow-boundary.mts
wscurran
added
area: ci
dfernandez365-rgb
left a comment
dfernandez365-rgb
left a comment
There was a problem hiding this comment.
Security review — fail-open test gate.
dockerAvailable maps any Linux docker info failure—missing CLI, daemon outage, permission denial, or timeout—to false, and it.skipIf(!dockerAvailable) then lets this security-annotated rollback contract disappear while the aggregate Linux gate remains green. The required Ubuntu shards have no Docker preflight, and the WSL root-only step plus Hermes root-entrypoint E2E exercise different contracts.
Please scope the exception to WSL without Docker using the existing isWsl() helper (or an explicit WSL-lane marker) and preserve native-Linux fail-closed behavior.
|
A verified reference patch for the requested fail-closed narrowing is available at dfernandez365-rgb@94db91d. Evidence on current main d5fa194:
I have not opened a duplicate PR. |
|
A separate #7259 correctness gap remains in this PR. Changing the Shields test timeout from 10 seconds to 20 seconds does not remove its 1200 ms sentinel, 1400 ms wait, or load-dependent wall-clock oracle. The verified issue candidate removes that timer boundary and, at the mocked policy-restore call, proves the captured child PID/start identity is gone or zombie while preserving owner-stop-before-enumeration, transition cleanup, and policy-set assertions: Native macOS evidence for that exact head: https://github.com/dfernandez365-rgb/NemoClaw/actions/runs/29774224429/job/88459619040 — 22/22 Shields tests, 1/1 watcher test, and 18,811 tests across 1,605 files passed. This PR's green macos-e2e job runs three unrelated integration tests and does not execute either #7259 file. Please adopt the exact child-lifecycle assertion/removal of the timer oracle. The watcher test should also retain the explicit absolute-path assertion rather than relying only on equality with realpathSync. |
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
dfernandez365-rgb
left a comment
dfernandez365-rgb
left a comment
There was a problem hiding this comment.
Exact-head review: cc900d695a38b5a6ae5417c7bc2dd7b27370ff8b.
Two blockers remain on this head:
-
The Hermes root-topology security contract still fails open when Docker is unavailable.
test/hermes-runtime-config-guard-topology.test.ts:73-75maps every Linuxdocker infofailure (missing CLI, daemon outage, permission denial, or timeout) todockerAvailable=false, and line 513 then skips the root repair/rollback test. That is only a legitimate skip for the explicitly Docker-less WSL lane. On native required Linux, loss of Docker must fail setup/the test rather than make this security contract disappear behind a green shard. Scope the skip to WSL (using the existing platform helper or an explicit lane marker) and preserve fail-closed native-Linux behavior. -
The current head deterministically fails its own maintainer policy suite. The merge-gate prose changed both safety statements from
coordination identitytorequired-check identity, buttest/maintainer-skills-policy.test.ts:271-275still requires two matches of the old sentence. Upstream CI job 88506901242 fails there, and I reproduced it locally on this exact head: 14 passed, 1 failed. Update the assertion to the new direct-check identity contract and rerun the focused integration test plus the affected CLI shard.
The known reviewed-npm audit failure is separate; these two findings are specific to this PR's diff and current head.
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
<!-- markdownlint-disable MD041 --> ## Summary Internal PRs that change credential-bearing E2E trust boundaries now request an exact-SHA protected-environment review automatically after ordinary CI. Repository-configured E2E reviewers can approve that environment without merge rights, while fork PR code still never receives repository secrets. ## Required Repository Configuration Before merging or enabling this workflow, a repository administrator must: 1. Create the environment `approve-credentialed-e2e-for-internal-pr`. 2. Configure one or more required reviewers. Prefer an organization team such as `nemoclaw-e2e-approvers`; members need repository read access but do not need merge rights. Individuals can also be listed directly. 3. Enable **Prevent self-review**. 4. Disable administrator bypass. 5. Restrict the environment to protected `main`. 6. Do not add environment secrets, variables, or custom protection apps. Only one configured reviewer is required to approve a run. The approval must cover the exact test plan. Complete this configuration before merging or enabling the workflow. ## Changes - Add `approve-credentialed-e2e-for-internal-pr` as a protected, non-deployment environment gate before internal credentialed E2E dispatch. - Bind approval to the first trusted controller attempt, exact workflow SHA, PR head SHA, base SHA, deterministic plan, and one approval naming only the expected environment. - Treat protected-environment reviewer configuration as the delegated authority for both internal execution approval and fork skip approval; retain `maintain` or `admin` checks on both manual fallback operations. - Fail closed without dispatch when approval history, environment configuration, run identity, PR identity, or the live revision does not match. - Document rollout requirements: required reviewer allowlists, **Prevent self-review**, protected `main`, no environment secrets, variables, or custom protection apps, and exact plan review. - Keep the existing manual `run-control-plane` and `approve-fork-e2e-skip` operations as maintainer fallbacks. This intentionally excludes the feedback-loop and observer-removal changes in #7262. Both PRs touch the gate lifecycle, so whichever merges second will require security-sensitive conflict resolution rather than an automatic merge. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [x] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [ ] Docs updated for user-facing behavior changes - [x] Docs not applicable — justification: this is a maintainer-only repository E2E authorization operation; `test/e2e/README.md` documents the operational change and no shipped user surface changes. - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [ ] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: pending maintainer security review. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Tested commit: not applicable - Station profile/scenario: not applicable - Result: not applicable - Supporting evidence: not applicable ## Verification - [x] PR description includes a `Signed-off-by:` line and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run check:diff` passed when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — `npx vitest run test/pr-e2e-gate-command.test.ts test/pr-e2e-gate-workflow.test.ts test/pr-e2e-gate-fork-skip.test.ts test/pr-e2e-gate-internal-approval.test.ts test/pr-e2e-gate-retry-history.test.ts test/pr-e2e-required.test.ts` (94 passed); `npm run typecheck:cli`; `npm run test-size:check`. - [ ] Applicable broad gate passed — `npm test` completed with 18,865 passed, 201 skipped, and 20 failures in untouched local macOS/tooling-dependent tests. Failures include `/private/var` path normalization, missing GNU `timeout` and `stat -c`, Bash-version assumptions, Docker timing/memory, Python build tooling, and unrelated timeouts. Approval-specific tests are green; CI must provide the authoritative Linux broad result. - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only) - [ ] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) --- Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> --------- Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
|
@dfernandez365-rgb The two blockers from your exact-head review are addressed on the current head: native Linux now fails closed when Docker is unavailable, and the maintainer policy assertion follows the direct required-check identity. I also refreshed onto current main and reconciled the merged delegated-approval work by preserving protected environment authorization while moving it to the direct E2E / PR Gate check. Please re-review when the new GitHub checks settle. |
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
Approved on exact head 2348422 against base 7e8945a. Product-scope gate passes: this is internal CI/E2E maintenance and creates no supported product integration or recipe. Nine-category security review found no findings; protected authorization, exact-diff/GitHub-App validation, least-privilege permissions, secret scoping, local-only image loading, fail-closed native-Linux coverage, and bounded/authoritative Brev parsing are preserved. The stale human blockers and all CodeRabbit requests are addressed. Approval remains contingent on the current GitHub checks and exact-SHA E2E gate completing successfully.
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
Re-approved on refreshed exact head 916b824 against base 7e8945a after the documentation-writer follow-up. The sole new commit is Verified/DCO and brings the maintainer merge gate in line with the already-reviewed workflow: protected approve-credentialed-e2e-for-internal-pr authorization and the current E2E reviewer authorization required to run E2E title first, with the existing exact-head/base manual run-control-plane fallback. It does not change executable behavior or the prior product-scope/security conclusions. Approval remains contingent on fresh GitHub CI and exact-SHA E2E evidence.
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
Re-approved on refreshed exact head a625749 against base 02cf73d. The merge from current main was conflict-free and adds only the already-merged Quickstart documentation/test ownership changes; the PR-specific diff, protected-authorization runbook correction, and prior product-scope/security conclusions are unchanged. Fresh hosted CI and new exact-SHA E2E evidence remain required.
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
Re-approved on refreshed exact head 6d56aeb against base 1bb15a0. The #7343 merge was conflict-free; its documentation-receipt workflow/package/test changes are inherited from main and disappear from the three-dot PR diff. The PR-specific file set, runbook fix, product scope, and nine-category security assessment remain unchanged. Fresh hosted CI and new exact-SHA E2E evidence are required.
|
Pre-merge repository-configuration audit: the required environment Please keep this PR unmerged until a repository administrator applies and validates those settings. No repository environment policy was changed during this review. |
|
Coordination note for the runner-stability work in #7146: recent hosted-runner shutdowns (including runs 29964501381 and 29965049603) use a Jobs API shape that the current classifier misses: a completed/failed hosted job, one completed/cancelled live step, skipped cleanup, and a final successful A focused classifier fix plus a strictly authorized one-time whole-plan retry is being developed separately; #7262 does not duplicate that behavior. I am keeping the core controller work independent from the current observer so it can rebase cleanly if this PR lands first. Integration points for #7262:
This is a convergence note only. No repository configuration or code on #7262 was changed. |
|
Current E2E failure diagnosis: run 29964960386, Hermes security-posture job 89074421115, lost standard hosted runner The last semantic-progress sample reported phase The Jobs API terminalized the live step as cancelled and the job as failed, which is exactly the current shape being added to #7146 classification and one-time retry coverage. #7391 already routes this Hermes lane to a larger runner when enabled, but repository variable |
<!-- markdownlint-disable MD041 --> ## Summary Final-main E2E previously fanned out even when the newest applicable base-image publication was missing, partial, or failed. This change adds a trusted, fail-closed preflight that proves all required publishers succeeded before matrix generation, while preserving the controller-selected PR path without checking out PR code. ## Related Issue Fixes #7372 ## Changes - Add a dependency-free publication verifier that derives publisher inputs from the checked-in workflow, resolves the applicable first-parent commit, bounded-polls paginated GitHub evidence, validates workflow/run/job provenance across rerun attempts, and reconfirms the selected run. - Add a least-privilege `base-image-publication` job before `generate-matrix`; trusted main schedule/manual runs perform the check, controller-selected PR dispatches bypass it before checkout, and invalid invocation modes fail closed. - Add focused behavioral and workflow-boundary coverage for pagination, retries, batched pushes, merge history, partial/failed evidence, diagnostic context, controller bypass, and trust-boundary mutations. - Keep this change independent of #7100's progress artifacts and #7262's matrix timeout; preserve those adjacent lines if either PR lands first. - Scope this fix to publication sequencing. Strict digest pinning and removal of downstream mutable `latest` fallback remain follow-up work. ## Type of Change - [x] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [ ] Docs updated for user-facing behavior changes - [x] Docs not applicable — justification: This changes internal GitHub Actions sequencing and provenance validation only; CLI, configuration, runtime, installation, and user-facing image behavior are unchanged. - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: A final fail-closed trust-boundary review found one diagnostic-context gap, which was fixed and regression-tested; no other blocking findings remained. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Documentation Writer Review - [x] Documentation writer subagent reviewed the completed implementation - Result: `no-docs-needed` - Evidence: The change adds an automatic internal E2E publication-provenance gate without changing CLI, configuration, runtime, installation, or user-facing image behavior. - Agent: Codex Desktop - PR: #7373 <!-- docs-review-head-sha: 10a2123 --> <!-- docs-review-agents-blob-sha: 73549ba --> ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Tested commit: - Station profile/scenario: - Result: - Supporting evidence: ## Verification - [x] PR description includes a `Signed-off-by:` line and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run check:diff` passed when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — command/result or justification: `npx vitest run --project e2e-support test/e2e/support/base-image-publication.test.ts test/e2e/support/base-image-publication-workflow-boundary.test.ts test/e2e/support/e2e-workflow.test.ts` — 95 passed. - [x] Applicable broad gate passed — `npm run checks`, `npm run typecheck:cli`, and `npx prek run --from-ref origin/main --to-ref HEAD` passed. - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only) - [ ] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) --- Signed-off-by: Apurv Kumaria <akumaria@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added a base-image publication verification gate that determines whether publication evidence is required before running E2E workflows. * Updated E2E matrix generation and PR E2E reporting to wait for this gate’s result. * Strengthened the E2E workflow boundary validation and approval authorization checks to rely on stable, trusted attributes. * **Tests** * Added end-to-end boundary coverage for the base-image publication policy, including classifier behavior and drift detection. * Added evidence validation tests for workflow/run selection, publisher job evidence rules, pagination safety, and deterministic polling/timeout behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Apurv Kumaria <akumaria@nvidia.com> Signed-off-by: Carlos Villela <cvillela@nvidia.com> Co-authored-by: Carlos Villela <cvillela@nvidia.com>
Preserve the direct PR gate while integrating the authenticated hosted-runner-loss retry and rejecting malformed Brev inventory entries. Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
|
Closing this PR as superseded by #7451. The issue records the focused PR boundaries, merge dependencies, validation requirements, and repository-configuration blocker. Rebuild each change from fresh |
7 participants
Summary
Shortens the critical path for
main, removes a long-lived PR-gate polling runner, and makes macOS/WSL feedback faster and actionable. It also isolates and caches the Hermes image build, adds bounded state-aware Brev provisioning retries, rejects malformed authoritative Brev inventory entries, and incorporates a strictly authenticated one-time retry for confirmed GitHub-hosted runner loss.Changes
checksgate.E2E / PR Gateas the exact-diff custom required check directly and delete the 170-minute polling/mirroring job and its observer implementation. Maintainer gate inspection keeps rollout compatibility with the formerE2E / PR Gate Coordinationname.Required Repository Configuration
Before merging or enabling this workflow, a repository administrator must:
approve-credentialed-e2e-for-internal-pr.such as
nemoclaw-e2e-approvers; members need repository read accessbut do not need merge rights. Individuals may be listed directly instead.
main.Only one configured reviewer is required to approve a run. The environment
must be configured before this workflow is merged or enabled, and approval
must cover the exact test plan.
Type of Change
Quality Gates
Documentation Writer Review
docs-updated.agents/skills/nemoclaw-maintainer-day/MERGE-GATE.md,test/e2e/README.md, andtest/e2e/docs/README.mddocument the direct required check, authorization and runner-loss retry paths, 330/140-minute retry budgets, rollout compatibility, platform sharding, Hermes image reuse, and Brev provisioning retries.Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailablenpm run check:diffpassed.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result:npm testcompleted with 18,758 passed, 199 skipped, and 21 failures in unchanged macOS-local assumptions (Bash 3, missing GNUtimeout, Python 3.14 wheel tooling, locale, and load-sensitive tests). The changede2e-report-to-prcontract passed in isolation; Linux CI remains required.npm run docsbuilds without warnings (doc changes only)Signed-off-by: Prekshi Vyas prekshiv@nvidia.com
Summary by CodeRabbit
New Features
E2E / PR Gaterequired check, with improved exact-diff verification and fork skip-approval handling.Bug Fixes
Documentation