fix(runtime): close prior review debt - #7976
Conversation
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
📝 WalkthroughWalkthroughThe change centralizes messaging channel selection, normalizes channel IDs, and expands plan validation across nested references. It also hardens the runtime entrypoint guard and isolates environment variables in entrypoint tests. ChangesMessaging plan flow
Runtime entrypoint test isolation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Plan
participant ChannelSelection
participant BuildSelection
Plan->>ChannelSelection: normalize and filter enabled channels
ChannelSelection->>BuildSelection: provide enabled channel IDs
BuildSelection->>Plan: return matching post-agent-install build files
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit 771f48c in the TypeScript / code-coverage/cliThe overall coverage in commit 771f48c in the Show a code coverage summary of the most impacted files.
Updated |
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported 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: This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
|
@coderabbitai review |
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
✅ Action performedReview finished.
|
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 `@src/lib/messaging/post-agent-install-selection.ts`:
- Around line 91-112: Update selectEnabledPostAgentInstallBuildFiles so an
unresolved step.hookId is excluded rather than treated as unrestricted:
distinguish a missing hook returned by find from a resolved hook, and return
true only when the matching hook exists and its phase is "post-agent-install"
(or otherwise satisfies the intended phase rule). Add a regression test covering
a hookId that does not match any hook on the matched channel.
🪄 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: 99fdd735-17c2-4887-b0c4-715e121faadc
📒 Files selected for processing (7)
src/lib/messaging/applier/plan-filter.tssrc/lib/messaging/plan-validation.test.tssrc/lib/messaging/plan-validation.tssrc/lib/messaging/post-agent-install-selection.test.tssrc/lib/messaging/post-agent-install-selection.tssrc/lib/onboard/managed-startup/image-runtime.tstest/entrypoint-env-wrapper.test.ts
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
6b5842b
into
feat/buildless-managed-contract-hardening
## Summary Adds the inert, runtime-parameterized E2E contract needed to qualify buildless and future native runtimes without introducing Podman switches into central orchestration. The catalog compiles Docker and a fixture-only MXC-style provider through the same open provider identity, scenario, obligation, execution, and parity-evidence contracts. It keeps OpenClaw, Hermes, and DCode together and models amd64/arm64 plus CPU/GPU execution dimensions. Local-inference, recovery, installer, user-facing documentation, and protected-E2E qualification remain deferred; this slice does not activate or advertise runtime support. ## Related Issue Part of #7744 ## Changes - Add an open branded execution-provider identity and immutable execution-profile registry rather than a closed Docker/Podman union. - Add runtime-neutral scenarios, support obligations, bounded host lanes, exact preparation ownership, and normalized parity evidence. - Add a fixture-local provider adapter seam that owns environment, lifecycle, state observation, and exact scenario assertions. - Compile all-agent OpenClaw, Hermes, and DCode scenarios for Docker and a fixture-only MXC-style provider with exact adapter/provider/scenario ownership checks. - Validate the inspected workload identity before obligations, preserve execution and cleanup failures, require non-empty provider receipts, and allowlist persisted receipt and scenario fields. - Prove profile construction does not register either fixture provider through the canonical runtime-profile query and public live-matrix boundary. - Preserve the canonical live matrix, workflow plan, risk plan, and existing Docker fixture behavior. - Keep the slice inert: no production provider registration, Podman command, workflow target, support claim, or runtime activation. - Document the cross-runtime foundation and the deferred activation obligations. ## 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 inert slice changes no user-visible behavior. `test/e2e/docs/README.md` documents the contributor-facing E2E contract. - [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: Independent exact-diff review found no P0/P1/P2. Provider identity stays open and runtime-neutral, every matrix case has exact provider/scenario/obligation ownership, preparation is bounded and atomic, inspected identity is validated before obligations, and observed lifecycle/state must conform exactly before parity evidence is accepted. - [ ] 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 changes - Result: `docs-updated` - Evidence: `test/e2e/docs/README.md` accurately documents the inert cross-runtime foundation, provider-neutral contracts, public non-registration boundary, and deferred activation. No additional user-facing documentation is required. - Agent: Codex Desktop <!-- docs-review-head-sha: 4788d28 --> <!-- docs-review-agents-blob-sha: c052d60 --> ## 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 validate:pr` passed after refreshing `origin/main` 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: The 26-test focused hardening suite, 36-test changed-file suite, 12-test registry/foundation suite, `typecheck`, `typecheck:cli`, source-shape policy, test-size policy, `git diff --check`, and full `npm run validate:pr` passed on `4788d287b`. - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: Exact-head required CI, advisors, CodeRabbit, multiarch, and protected E2E are running for `4788d287b8672be1b44999e78e094b2221303bd1`. - [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) ## Stack - Base: PR3.5b #7976 branch `feat/buildless-coderabbit-debt` at `771f48c47c0aaa1d51511d172fffc691e6c9ac76`. - This slice: PR3.5c branch `feat/buildless-runtime-e2e-foundation` at `4788d287b8672be1b44999e78e094b2221303bd1`. - Next: PR3.6 adds the driver-neutral lifecycle bundle registry and sandbox-action parity; it is not part of this review diff. - Production buildless and Podman support remain disabled until the full all-agent, amd64/arm64, GPU, local Ollama/NIM/vLLM, recovery, installer, documentation, and protected-E2E gates pass. Signed-off-by: Aaron Erickson <aerickson@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes * **Documentation** * Added comprehensive documentation for cross-runtime end-to-end test foundation. * **New Features** * Introduced cross-runtime test execution planning with support for multiple execution profiles and platforms. * Added execution profile system with validation for platforms, architectures, and capabilities. * Implemented runtime matrix compilation and resolution for deterministic test case distribution. * Added test fixture infrastructure for provider-neutral scenario definitions. * **Tests** * Added cross-runtime compatibility and parity verification tests. * Added comprehensive runtime matrix validation test suite. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
<!-- markdownlint-disable MD041 --> ## Summary Adds the inert, provider-neutral managed-workload rebuild transaction for the incremental buildless stack. The exact old workload and registry row remain authoritative through replacement preparation, readiness, state restore, and provider rebind. Only one exact compare-and-swap publishes the replacement, and old-runtime retirement happens afterward through provider-owned opaque handles. This slice does not wire a production rebuild caller or activate buildless onboarding. Snapshot/backup and durable recovery ownership remain tracked in #7744 and are required before activation. ## Related Issue Part of #7744 ## Changes - Capture a deep-frozen rebuild plan bound to the exact provider, shipped agent, platform, prior managed receipt, full durable-row revision, lifecycle generation, and live identity fingerprint. - Pre-render and validate the exact replacement image, startup profile, receipt, and safe metadata before provider mutation. - Define provider-neutral prepare, create, readiness, restore, provider-rebind, rollback, abort-preparation, and retire-previous phases using opaque exact handles rather than sandbox-name deletion. - Keep partial prepare/create cleanup transaction-idempotent and run abort cleanup even when post-prepare registry revalidation throws. - Publish only through exact old-authority CAS; reconcile ambiguous persistence against either the exact replacement or exact old row. - Preserve the staged replacement and return an immutable recovery task when publication is indeterminate, avoiding rollback of a replacement that may already be durable. - Retire the exact old runtime only after publication; return a durable-owner recovery task if retirement remains pending. - Bind replacement contracts and startup profiles to OpenClaw, Hermes, or DCode authority and reject provider, agent, platform, receipt, generation, or identity drift. - Reject malformed provider artifacts at every transition, stop before later phases, and prove exact transaction abort or exact staged-handle rollback. - Document the shared backup boundary and the durable recovery ownership tracked in #7744 before activation. ## Type of Change - [x] 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: The transaction is inert with no production caller or support claim; the internal README records ownership boundaries for later slices. - [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: Exact-head audit covers immutable authority, pre-mutation validation, CAS ambiguity, abort cleanup, exact-handle rollback, and deferred recovery ownership. - [ ] 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 changes - Result: `docs-updated` - Evidence: The reviewed 23-file, `+4,272/-0` slice remains byte-identical after the append-only current-main refresh to `0de2789608a86e580d787991e81c03c5f0b14dbf` through `e97ecce48c7fcc1dfb398e1cfae8c81a859b7dcd`; stable patch ID remains `dd1c4a899fd9a62954a00d4e2e61da445a306e03`. The only documentation path is `src/lib/onboard/managed-workload/rebuild/README.md`. It accurately states that the transaction is dormant, has no CLI command or production-action importer, and does not activate buildless rebuilds. It assigns ambiguous publication and pending retirement to durable recovery, links recovery and snapshot/backup ownership to the live accepted epic #7744, and requires normalized backup manifests, restore validation, durable reconciliation, and protected qualification for OpenClaw, Hermes, and LangChain Deep Agents Code before activation. Production-import and command/action diff scans found no activation caller. Markdownlint passed with zero issues on the exact refreshed head. The append-only parent refresh to `362a70cda` preserves the exact reviewed slice diff and changes no reviewed documentation. - Agent: Codex Desktop <!-- docs-review-head-sha: 362a70c --> <!-- docs-review-agents-blob-sha: 3dd7c24 --> ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Tested commit: - Station profile/scenario: - Result: - Supporting evidence: ## Verification - Exact locally validated head/base: `e97ecce48c7fcc1dfb398e1cfae8c81a859b7dcd` / `0de2789608a86e580d787991e81c03c5f0b14dbf` - Review budget: 23 files, `+4,272/-0`. - Stable exact-slice patch ID: `dd1c4a899fd9a62954a00d4e2e61da445a306e03`. - [x] The six implementation/review commits and both maintainer refresh commits are SSH-signed and contain DCO trailers; GitHub-generated conflict-resolution merge commits preserve append-only branch history. - [x] `npm run validate:pr` passed on the exact clean head with Node 22.16.0. - [x] 132 focused rebuild transaction, workload authority, registry CAS, and source-boundary tests passed again on the exact refreshed head; CLI typecheck and repository checks also passed; changed test files add zero `if` statements. - [x] `npm run build:cli`, CLI typecheck through `validate:pr`, exact-base pre-commit, commitlint, and pre-push gates passed. - [x] Failure tests prove prepare/create ambiguity aborts exact transaction resources, staged failures roll back only exact staging authority, and indeterminate CAS never rolls back. - [x] Agent-binding tests reject cross-agent image/profile drift for all shipped managed-image agents. - [x] No snapshot manifest dependency, production rebuild callsite, runtime selection change, or public activation exists in this slice. - [ ] Applicable broad gate passed — exact-head required CI, advisors, CodeRabbit, multiarch, and protected E2E are the broad remote gates. ## Stack - Base: live `main` at `0de2789608a86e580d787991e81c03c5f0b14dbf`; PR3.1 through PR3.6 content is already landed, with #7976, #7988, and #7990 carried once through the final #7973 aggregate tree. - This slice: PR3.7 branch `feat/managed-workload-rebuild-parity` at `e97ecce48c7fcc1dfb398e1cfae8c81a859b7dcd`. - Epic #7744 tracks shared snapshot, backup, restore, and durable recovery ownership before activation. - Buildless support remains disabled until OpenClaw, Hermes, and DCode plus required multiarch and protected qualification pass together. Signed-off-by: Aaron Erickson <aerickson@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added managed workload rebuild workflows with staged replacement, validation, rollback, recovery, and atomic commit handling. * Added authority validation for managed workloads, including receipt, image, platform, and startup configuration checks. * Added safe cloning and deep-freezing for supported immutable data. * Added safeguards against stale, conflicting, or incomplete workload state during rebuilds. * **Documentation** * Documented rebuild recovery behavior and activation requirements. * **Tests** * Added comprehensive coverage for rebuild transactions, authority validation, rollback, persistence reconciliation, and immutable data handling. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Aaron Erickson <aerickson@nvidia.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Summary
Closes the five still-valid unresolved CodeRabbit findings from the lower buildless stack and the same-surface advisor findings found during exact-head qualification without rewriting its green heads. Managed messaging selection now honors disabled channels through one normalized shared contract, the image-runtime entry guard is safe under ESM test imports, and dashboard-port tests are hermetic against ambient host values.
Related Issue
Part of #7744
Changes
NEMOCLAW_DASHBOARD_PORTandCHAT_UI_URLfrom wrapper scenarios before applying explicit test inputs.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 unavailablebuild:cli,typecheck:cli, and exact-headvalidate:prpassed.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result: Exact-head required GitHub CI and protected E2E are the broad gate for this small dormant follow-up; they are running for locally validated head771f48c47c0aaa1d51511d172fffc691e6c9ac76.npm run docsbuilds without warnings (doc changes only)Stack
feat/buildless-managed-contract-hardeningat7d51947c870b8f1de2caa1d33e6e9d6e98d11346.feat/buildless-coderabbit-debtat771f48c47c0aaa1d51511d172fffc691e6c9ac76.Signed-off-by: Aaron Erickson aerickson@nvidia.com
Summary by CodeRabbit