refactor(sandbox): remove Shields from core - #10722
Conversation
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
|
Important Review skippedToo many files! This PR contains 604 files, which is 304 over the limit of 300. To get a review, reduce the PR to 300 files or fewer by splitting it into smaller PRs or changing its base branch. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (604)
You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
|
🌿 Preview your docs: https://nvidia-preview-pr-10722.docs.buildwithfern.com/nemoclaw |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall line coverage in commit fff3bca in the TypeScript / code-coverage/cliThe overall line coverage in commit fff3bca in the Show a line coverage summary of the most impacted files.
Updated |
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Restore the Hermes mutable cross-identity root before gateway import. Repair the stale image fixture and CodeQL annotations. Enable trusted BuildKit E2E prebuilds and pin the exact Pi candidate receipts. Co-authored-by: Prekshi Vyas <prekshiv@nvidia.com> Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Preserve current advisor and Hermes E2E fixes while keeping the removal atomic. Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Keep the newly landed Shields implementation and tests removed while retaining current E2E fixes. Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Retire legacy Shields state only after an exact sandbox-identity proof. Verify rebuilt Hermes config through pinned descriptors before deleting upgrade state. Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Exercise valid, read-only, and linked config fixtures. Clarify when legacy recovery artifacts block existing sandboxes. Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Avoid a redundant agent-definition lookup after the recreated Hermes identity is already verified. Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Bind DCode and Pi retirement to exact replacement identity and successful restore. Require complete generic post-restore verification before deleting legacy state. Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Keep the atomic removal branch aligned with current main before final qualification. Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Read replacement and staged fixtures through no-follow regular-file descriptors. Keep intentional stale-handle tests without filesystem race alerts. Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Keep retired Shields commands and tests removed. Retain current external gateway and E2E pruning changes. Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
cjagwani
left a comment
There was a problem hiding this comment.
Exact head reviewed: fff3bcae772657bb5e9438868a7453633bf0223f.
P0
- None.
P1
- The recovery allow flag is appended after a variable-width rest tuple, so the ordinary path places it in
preparedBuildContextand leavesallowRemovedImmutabilityStateRecordfalse. - Mutable-config posture checks require the legacy Docker argv builder and fail on the supported Podman provider.
- The documented sub-agent flow persists a real upstream key in agent-readable/agent-writable OpenClaw state.
- The Pi receipt guard compares against
HEAD^2, excluding current-base inputs from the candidate it attests and allowing stale image receipts to pass.
The 7580a670..fff3bcae delta only hardens three loaded-runner test deadlines; those focused tests pass and it changes none of these findings. All 99 commits are Verified. CodeRabbit is terminal (skipped because the PR exceeds its file cap); all nine exact-head Advisor specialists terminated without artifacts because of infrastructure failures. Exact-head CI evidence was collected separately and does not change these four reproducible findings.
| @@ -3231,6 +3228,7 @@ async function runOnboard(opts: OnboardOptions = {}): Promise<void> { | |||
| dashboardPortReservationScope, | |||
| hermesApiPortReservationScope, | |||
| ...createArgs, | |||
| opts.allowRemovedImmutabilityStateRecord === true, | |||
There was a problem hiding this comment.
[P1] Keep the migration allow flag in a fixed argument slot
createArgs is variable-width: on the ordinary non-deferred path the wrapper supplies 16 arguments and appends the prepared build context as argument 17. Appending this boolean after ...createArgs therefore places it in preparedBuildContext, while allowRemovedImmutabilityStateRecord retains its false default. The accepted legacy-state recovery is rejected, and true is also consumed as a build context. Thread this through an explicit fixed slot (or preserve the missing placeholders) and cover the real no-callback recovery path through runOnboard.
| } | ||
|
|
||
| function privilegedExecCapture(sandboxName: string, command: string[]): string { | ||
| return dockerExecFileSync(privilegedSandboxExecArgv(sandboxName, command, false, true), { |
There was a problem hiding this comment.
[P1] Route mutable-posture commands through provider-native execution
privilegedSandboxExecArgv requires the optional Docker-compatibility buildLegacyDockerArgv, but the supported Podman provider exposes native resolveTarget/execute without that legacy builder. Every call through this helper, plus the direct repair and Hermes-probe calls below, therefore throws before inspecting or repairing the sandbox. Use capturePrivilegedSandboxCommand / executePrivilegedSandboxCommand so the selected provider owns execution, and add coverage with a provider that lacks the Docker compatibility builder.
| docker exec --user root "$SANDBOX_CTR" chown sandbox:sandbox /sandbox/.openclaw/openclaw.json /sandbox/.openclaw/.config-hash | ||
| docker exec --user root "$SANDBOX_CTR" chmod 444 /sandbox/.openclaw/openclaw.json | ||
| docker exec --user root "$SANDBOX_CTR" chmod 444 /sandbox/.openclaw/.config-hash | ||
| docker exec --user root "$SANDBOX_CTR" chmod 660 /sandbox/.openclaw/openclaw.json |
There was a problem hiding this comment.
[P1] Keep the upstream provider key out of agent-controlled state
This procedure acknowledges that /tmp/openclaw.updated.json contains the real API key, uploads it into openclaw.json, and then makes that file readable and writable by the sandbox identity. Sandbox code can therefore read and exfiltrate the upstream credential, contrary to the OpenShell provider/placeholder boundary. Replace the recipe with a supported host-side provider and policy flow; if direct auxiliary-provider credentials are unsupported, state that limitation instead of publishing a runnable raw-key upload path.
| if (process.env.GITHUB_ACTIONS !== "true" || process.env.GITHUB_EVENT_NAME !== "pull_request") { | ||
| return "HEAD"; | ||
| } | ||
| const result = git(["rev-parse", "--verify", "HEAD^2"]); |
There was a problem hiding this comment.
[P1] Validate receipts against the exact merge candidate
On pull_request, HEAD is the synthetic merge while HEAD^2 is only the PR branch head. Comparing to HEAD^2 drops base-only Pi inputs: current main changed nemoclaw-blueprint/blueprint.yaml, agents/pi/Dockerfile copies that whole directory, and these receipts still attest the older source tree. The exact synthetic merge differs in that copied input, yet this head's static-checks passes. Compare against the exact merge candidate and add a divergent-base regression so current-base changes also force both architecture receipts to be refreshed together.
Maintainer override under the existing approval.
## Outcome NemoClaw now inventories direct and transitive live E2E assertions without executing live test modules. A checked-in baseline and PR growth guard prevent assertion growth while Epic #10920 reduces the suite. ## Reason The live E2E suite had no deterministic assertion inventory. Assertions could move into companion modules or change syntax without making the increased failure surface visible. ### Related issues Closes #10934 Part of #10920 ## Changes - Add a TypeScript-parser census for Vitest matchers, Node assertions, named assertion helpers, failure guards, object-field assertions, and detectable generated probes. - Resolve live companion imports statically and report direct, transitive, per-file, and unique-suite totals without loading live modules. - Add a compact current-main baseline that preserves the fixed Epic #10920 reference and records the lower post-#10722 main state. - Run the ratchet from `npm run checks:repository` and reject baseline increases through the existing PR growth guard. - Document the assertion-disposition and baseline-update workflow in `test/README.md`. The new mechanism is required by Epic #10920. Repository checks and E2E-remediation PRs consume it. A direct assertion count is insufficient because it can be reduced by moving assertions into helpers or other assertion forms. `test/repository/e2e-assertion-census.test.ts` protects the parser, import graph, path controls, census, and ratchet. ## Verification - `npx vitest run --project integration test/repository/e2e-assertion-census.test.ts test/repository/checks-runner.test.ts test/automation/pull-requests/growth-guardrails.test.ts` — 3 files and 66 tests passed. - `npm run typecheck:cli` — passed. - `npm run checks:repository` — passed, including the new 1,977-direct-expect baseline across 86 live test files. - Commit hooks — pre-commit, commit-msg, and pre-push passed. The published commit is GitHub Verified. - No live E2E run applies because this change analyzes source and does not change live behavior. - The diff contains no secrets, API keys, or credentials. ## Review notes `npm run check` completed every pre-commit and repository-wide governance check, then one existing CLI coverage worker did not exit after the active workers finished. The run was stopped and is not claimed as passing. `npm run test:changed` passed its targeted growth-guard test, then selected 1,682 unrelated CLI, plugin, and E2E-support files. It reported 62 failures from existing host state, unavailable macOS commands such as `ip`, and parallel timeout paths. None involved the changed census, ratchet, runner-registration, or growth-guard tests. Required GitHub CI remains the authoritative broad gate for this PR. --- 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 tooling to scan end-to-end tests and track assertion counts. - Added configurable assertion budgets with reporting, JSON export, validation, and update commands. - Added safeguards for assertion growth, missing files, renamed tests, and budget violations. - **CI & Quality** - Integrated assertion-budget checks into repository validation and growth guardrails. - Added baseline metadata covering current end-to-end test assertion limits. - Added comprehensive coverage for budget enforcement and diagnostic reporting. - **Documentation** - Documented the live end-to-end assertion ratchet process and approved budget updates. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
…pts removed upstream Upstream removed the Shields/state-mutation subsystem (NVIDIA#10722), deleting scripts/state-dir-guard.py, runtime-state-mutation-{control,startup-gate}.py, runtime_state_mutation_hermes_publisher.py, and their state-lock-plan / runtime-state-mutation-publisher-v1.json manifests. The fork's hermes Dockerfile and start/vm entrypoints still COPY and invoke them at runtime, so a bare rebase dropped them and broke build-nemoclaw-hermes. Restore the six files from the pre-rebase fork HEAD so the hermes image keeps its state-guard posture.
…pts removed upstream Upstream removed the Shields/state-mutation subsystem (NVIDIA#10722), deleting scripts/state-dir-guard.py, runtime-state-mutation-{control,startup-gate}.py, runtime_state_mutation_hermes_publisher.py, and their state-lock-plan / runtime-state-mutation-publisher-v1.json manifests. The fork's hermes Dockerfile and start/vm entrypoints still COPY and invoke them at runtime, so a bare rebase dropped them and broke build-nemoclaw-hermes. Restore the six files from the pre-rebase fork HEAD so the hermes image keeps its state-guard posture.
…pts removed upstream Upstream removed the Shields/state-mutation subsystem (NVIDIA#10722), deleting scripts/state-dir-guard.py, runtime-state-mutation-{control,startup-gate}.py, runtime_state_mutation_hermes_publisher.py, and their state-lock-plan / runtime-state-mutation-publisher-v1.json manifests. The fork's hermes Dockerfile and start/vm entrypoints still COPY and invoke them at runtime, so a bare rebase dropped them and broke build-nemoclaw-hermes. Restore the six files from the pre-rebase fork HEAD so the hermes image keeps its state-guard posture.
…pts removed upstream Upstream removed the Shields/state-mutation subsystem (NVIDIA#10722), deleting scripts/state-dir-guard.py, runtime-state-mutation-{control,startup-gate}.py, runtime_state_mutation_hermes_publisher.py, and their state-lock-plan / runtime-state-mutation-publisher-v1.json manifests. The fork's hermes Dockerfile and start/vm entrypoints still COPY and invoke them at runtime, so a bare rebase dropped them and broke build-nemoclaw-hermes. Restore the six files from the pre-rebase fork HEAD so the hermes image keeps its state-guard posture.
…pts removed upstream Upstream removed the Shields/state-mutation subsystem (NVIDIA#10722), deleting scripts/state-dir-guard.py, runtime-state-mutation-{control,startup-gate}.py, runtime_state_mutation_hermes_publisher.py, and their state-lock-plan / runtime-state-mutation-publisher-v1.json manifests. The fork's hermes Dockerfile and start/vm entrypoints still COPY and invoke them at runtime, so a bare rebase dropped them and broke build-nemoclaw-hermes. Restore the six files from the pre-rebase fork HEAD so the hermes image keeps its state-guard posture.
…pts removed upstream Upstream removed the Shields/state-mutation subsystem (NVIDIA#10722), deleting scripts/state-dir-guard.py, runtime-state-mutation-{control,startup-gate}.py, runtime_state_mutation_hermes_publisher.py, and their state-lock-plan / runtime-state-mutation-publisher-v1.json manifests. The fork's hermes Dockerfile and start/vm entrypoints still COPY and invoke them at runtime, so a bare rebase dropped them and broke build-nemoclaw-hermes. Restore the six files from the pre-rebase fork HEAD so the hermes image keeps its state-guard posture.
…pts removed upstream Upstream removed the Shields/state-mutation subsystem (NVIDIA#10722), deleting scripts/state-dir-guard.py, runtime-state-mutation-{control,startup-gate}.py, runtime_state_mutation_hermes_publisher.py, and their state-lock-plan / runtime-state-mutation-publisher-v1.json manifests. The fork's hermes Dockerfile and start/vm entrypoints still COPY and invoke them at runtime, so a bare rebase dropped them and broke build-nemoclaw-hermes. Restore the six files from the pre-rebase fork HEAD so the hermes image keeps its state-guard posture.
…pts removed upstream Upstream removed the Shields/state-mutation subsystem (NVIDIA#10722), deleting scripts/state-dir-guard.py, runtime-state-mutation-{control,startup-gate}.py, runtime_state_mutation_hermes_publisher.py, and their state-lock-plan / runtime-state-mutation-publisher-v1.json manifests. The fork's hermes Dockerfile and start/vm entrypoints still COPY and invoke them at runtime, so a bare rebase dropped them and broke build-nemoclaw-hermes. Restore the six files from the pre-rebase fork HEAD so the hermes image keeps its state-guard posture.
<!-- markdownlint-disable MD041 --> ## Outcome Legacy OpenClaw upgrades now preserve the prepared rebuild context in its intended create-call position instead of treating it as a DCode context. The supported Shields-era migration selects the exact candidate managed image while retaining the released workload receipt as previous authority. ## Reason Four legacy upgrade targets failed after destructive preparation because an omitted optional callback shifted the prepared context into the wrong positional argument. The v0.0.115 Shields migration also rejected the intentional transition from its released receipt revision to the exact candidate revision. ### Related issues Fixes #10964 Fixes #10970 Refs #10722 ## Changes - Preserve the optional verified-create callback slot at the onboarding machine boundary and update its direct call-shape tests. - Use the exact GitHub Actions qualification revision for the replacement workload while keeping the old receipt in the rebuild handoff. - Remove the unrelated Hermes, messaging, cron, WeChat, redaction, recovery, npm-remediation, and Pi changes from this PR. ## Verification - `npx vitest run --project cli <nine affected handler and rebuild test files>` — 174/174 tests passed. - `npx vitest run --project cli src/lib/onboard/prepared-dcode-rebuild.test.ts` — 17/17 tests passed. - `npx vitest run --project cli src/lib/onboard/sandbox-workload-rebuild.test.ts` — 25/25 tests passed after the final test correction. - `npx vitest run --project cli src/lib/onboard/machine/handlers/sandbox-dcode-selection.test.ts src/lib/onboard/sandbox-workload-rebuild.test.ts` — 39/39 tests passed after the final review wording corrections. - `npm run typecheck:cli` — passed. - `npm run docs` — passed with 0 errors and 5 existing Fern warnings; documentation review found no public documentation change required. - Normal `pre-commit` and `commit-msg` hooks — passed, including repository checks, ShellCheck, Hadolint, gitleaks, E2E phase plans, and commitlint. - [Exact-head managed-image build](https://github.com/NVIDIA/NemoClaw/actions/runs/33924466936) — passed for `9f697dc9c1b9caefc01bd5ac47e6aed5e246b9a0`, including direct OpenClaw, Hermes, and Deep Agents Code startup and exact all-agent activation. - [Exact-head live E2E](https://github.com/NVIDIA/NemoClaw/actions/runs/33926108097) — the v0.0.55 ARM64, v0.0.74 x86-64, v0.0.89 x86-64, and v0.0.115 Shields migrations passed. The v0.0.55 x86-64 target completed the repaired image rebuild and sandbox replacement, then hit a downstream OpenClaw gateway health timeout after the sandbox inference route passed; see the related intermittent recreation-health tracker #10977. - The diff contains no secrets, API keys, or credentials. ## Review notes This reduces the PR from 48 files and 941 changed lines to 12 files and 129 changed lines. The previous Hermes, WeChat, redaction, selected-runtime recovery, npm-remediation, and Pi findings no longer apply because those changes are absent from the final diff. One local prepared-context integration fixture stopped before the changed code because this host reported a `packaged-service` gateway while the fixture recorded `standalone`; the direct prepared-context unit boundary passed. Exact-head live E2E remains the authoritative lifecycle evidence. The v0.0.55 x86-64 E2E no longer reports `A prepared DCode build context cannot be used for this sandbox target.` It reaches `Sandbox 'e2e-gw-survivor' created`, verifies the compatible endpoint through `inference.local`, and then times out waiting for the resumed OpenClaw gateway. The matching ARM64 target and the other required x86-64 upgrades pass. The failed job cleaned up every registered resource. The migration remains fail-closed outside GitHub Actions. During qualification, the old receipt remains immutable previous authority and only the exact candidate revision selects the replacement image. --- Signed-off-by: Aaron Erickson <aerickson@nvidia.com> Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> --------- Signed-off-by: Aaron Erickson <aerickson@nvidia.com> Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> Co-authored-by: Prekshi Vyas <34834085+prekshivyas@users.noreply.github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Prekshi Vyas <prekshiv@nvidia.com>
<!-- markdownlint-disable MD041 --> ## Outcome Adds the canonical dated documentation entry for v0.0.120 and records the release's material user-facing changes before tag planning. The Hermes rebuild guide now also documents the fail-closed immutable-base requirement for legacy sandboxes without an image hint. ## Reason Release planning requires a merged `docs/changelog/2026-09-04.mdx` containing exactly one `## v0.0.120` heading. The existing automation draft does not contain that required changelog and does not cover the full release scope, so this PR provides a fresh, independently reviewed release-docs update. ### Related issues Relates to #10919 ## Changes - Add three release-note lead paragraphs and detailed, user-facing v0.0.120 changes with canonical documentation routes. - Cover configuration export and doctor (#11015, #11012); Hermes runtime, recovery, and Discord policy (#10595, #11071, #11024, #10927, #10983, #10988, #10999, #11019, #10682); Shields retirement (#10722, #10996); OpenShell forwarding and runtime authority (#10695, #10814, #10815, #10810); onboarding and recovery (#10690, #10900, #11046, #10882, #10864); inference behavior (#10956, #10910, #11070); Deep Agents MCP projection safety (#10911, #10909); and provider-profile validation (#10884, #10895). - Scope the legacy Hermes immutable-base rebuild guidance to the Hermes-rendered recovery page. ## Verification - `npx vitest run --project integration test/generation/check-docs-links.test.ts test/generation/check-docs-published-routes.test.ts test/generation/post-merge-docs.test.ts` — 3 files and 125 tests passed. - `npm run docs` — passed with 0 errors and 5 existing Fern warnings. - Independent documentation audit — reconciled all 71 commits in `v0.0.119..origin/main`, validated all 29 PR links and published routes, and found no unsupported product claims or remaining corrections. - Normal `pre-commit`, `commit-msg`, and `pre-push` hooks — passed. - `git diff --check` — passed. - GitHub commit verification — `a22fe0989fd72c7daaa9b2e7a4734a3edc069aba` is Verified with reason `valid`. - Secret review — the diff contains no secrets, API keys, or credentials. ## Review notes The existing automation draft #10919 is intentionally left untouched. This PR supersedes its release-docs content with the complete canonical changelog and a variant-correct Hermes recovery update. --- Signed-off-by: Charan Jagwani <cjagwani@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Documentation** - Added release notes covering verified configuration export, host and gateway diagnostics, service forwarding, sandbox recovery, onboarding safeguards, inference retries, MCP projection safety, provider setup, and Discord runtime policy. - Clarified sandbox rebuild behavior, including use of the release-pinned immutable base image when required. - Documented that rebuilds stop before modifying sandbox data when the required image cannot be resolved or validated. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Outcome
Removes Shields from NemoClaw core in one atomic change: the CLI, plugin, runtime-provider contract, policies, manifests, docs, tests, and compiled command surface no longer expose a Shields capability. Existing installations with legacy Shields state receive an explicit retirement notice and are routed through fail-closed rebuild, recreation, or destruction recovery before ordinary mutation.
Reason
NemoClaw should not own post-provisioning immutability as an inherent product concept. A future ecosystem product or composable blueprint can provide that capability on top of NemoClaw without coupling every supported agent and runtime provider to a built-in Shields state machine.
Related issues
Fixes #10696
Changes
shields up,shields down, andshields statuscommands, plugin status affordance,src/lib/shields, permissive-policy and state-lock-plan assets, runtime-providerstateMutationcontract, Hermes mutation publisher/controller/gate, and Shields-specific E2E and unit infrastructure.Verification
npm run validate:pr— passed on the published commit against the refreshed canonical comparison ref.npm run build:cliandnpm run typecheck:cli— passed.npm --prefix nemoclaw run buildandnpm --prefix nemoclaw run typecheck— passed.npm run validate:configs— 52/52 configuration files passed.npm run checks:repository— architecture, imports, project membership, source-shape, and growth checks passed.npm run test:e2e-phases:check— 132 semantic E2E phase tests across 87 files passed.npm --prefix tools/mcp-tool-discovery-runtime run bundle:reviewed:check— passed.npm run docs— passed with 0 errors and 2 unrelated existing Fern warnings.sandboxandgatewayidentities.gitleaksand the pre-commit private-key check passed; the diff contains no secrets, API keys, or credentials.Review notes
npm testandnpm run test:changedattempts were not green on this loaded macOS host. The observed failures included fixed-timeout/load behavior, a known invalid host-local gateway registry row, macOS/private/varnormalization, and a snapshot-sanitizer hardlink case that reproduced intermittently on cleanmain. A serial replay of the initially red changed set passed 301/308; its remaining seven failures are in unchanged files and reduce to that host registry plus one macOS temp/timing fixture. The implementation-owned gates and focused suites above pass, and clean PR CI remains authoritative.Signed-off-by: Aaron Erickson aerickson@nvidia.com