fix(shields): reconcile startup API key on first Hermes shields down (#6381) - #6384
Closed
yanyunl1991 wants to merge 6 commits into
Closed
fix(shields): reconcile startup API key on first Hermes shields down (#6381)#6384yanyunl1991 wants to merge 6 commits into
yanyunl1991 wants to merge 6 commits into
Conversation
…6381) On a freshly built, OpenShell-managed (non-root) Hermes sandbox, `nemohermes <sandbox> shields down` always failed with "strict hash verification failed for Hermes restart seal", and the aborted transition left /sandbox/.hermes in a broken state that then blocked shields up too. Root cause: OpenShell launches the Hermes entrypoint as the sandbox user. That non-root startup mints the per-sandbox API_SERVER_KEY into .env and refreshes the in-tree compatibility hash anchor, but it cannot advance the root-owned strict anchor at /etc/nemoclaw/hermes.config-hash, which stays at its build-time digest. The sealed shields "mutable" transition (shields down) verifies the current config against that stale strict anchor and fails on every fresh non-root sandbox. The guard already knows how to reconcile exactly this drift: seal_restart's `config-write` purpose calls _reconcile_nonroot_startup_api_key_hash, which advances the strict anchor only when the sole difference is the single generated API_SERVER_KEY, config.yaml is unchanged, and the compat anchor corroborates the frozen inputs — refusing every other config or env change. This change lets the `shields-mutable` purpose use that same reconciliation: begin-shields-transition now accepts --expected-config-sha256 and threads it to seal_restart, and the host passes the current config.yaml digest when starting a mutable transition. Without the digest the stale anchor still fails closed, so the reconciliation stays opt-in and every existing refusal is preserved. Note: shields up on a fresh non-root Hermes sandbox has a separate, pre-existing host-side lock-verification ordering issue (it reports "parent dir mode=755 (expected 1775)" even though the locked posture is ultimately applied). That is independent of this strict-hash fix and is not addressed here. Fixes #6381 Signed-off-by: Yanyun Liao <yanyunl@nvidia.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughHermes shields transitions now accept and validate an expected config SHA-256 for mutable starts, pass it through the CLI and client wiring, adjust post-finish parent verification, and add tests for reconciliation success and failure cases. ChangesShields-down reconciliation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant CLI as nemohermes CLI
participant Client as beginHermesConfigShields
participant Sandbox as privileged sandbox
participant Guard as runtime-config-guard.py
CLI->>Client: begin shields transition (mutable)
Client->>Sandbox: hash target.configPath
Sandbox-->>Client: SHA-256 digest
Client->>Guard: begin-shields-transition --hash-file --expected-config-sha256
Guard->>Guard: seal_restart() checks StrictHashMismatchError
alt purpose=shields-mutable and digest present
Guard-->>Client: reconcile
else digest missing or config drift
Guard-->>Client: fail closed
end
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in the Show a code coverage summary of the most covered files.
TypeScript / code-coverage/cliThe overall coverage in the Show a code coverage summary of the most covered files.
Updated |
Contributor
E2E Advisor RecommendationRequired E2E: Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
Contributor
E2E Target RecommendationRequired E2E targets: None Full E2E target advisor summaryE2E Target AdvisorBase: Required E2E targets
Optional E2E targets
Relevant changed files
|
Contributor
PR Review Advisor — Changes requestedMerge posture: Do not merge yet Action checklist
Findings index
Review findings by urgency: 0 required fixes, 4 items to resolve/justify, 1 in-scope improvement
|
Contributor
PR Review Advisor (Nemotron Ultra) — Changes requestedMerge posture: Do not merge yet Action checklist
Findings index
Review findings by urgency: 0 required fixes, 2 items to resolve/justify, 3 in-scope improvements
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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/hermes/runtime-config-guard.py (1)
2656-2678: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
shields-lockedstill bypasses startup-hash reconciliation.begin_shields_transition(..., mode="locked")routes to_seal_shields_locked(), which never acceptsexpected_config_sha256, so a fresh sandbox whose first root transaction is shields-up will still fail on the startup-minted.envdrift. Thread the expected hash through the locked path or make shields-up-first impossible.🤖 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/hermes/runtime-config-guard.py` around lines 2656 - 2678, The locked shields transition still skips startup-hash reconciliation, so a fresh sandbox can fail on the startup-minted .env drift. Update the `begin_shields_transition` / `_seal_shields_locked` flow to either accept and pass through `expected_config_sha256` for the `mode="locked"` path or explicitly prevent shields-up-first when that reconciliation cannot happen. Keep the reconciliation logic aligned with `_reconcile_nonroot_startup_api_key_hash` and `_verify_strict_hash` so the locked path handles the same startup hash drift as `config-write`.
🤖 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/hermes/runtime-config-guard.py`:
- Around line 2656-2678: The locked shields transition still skips startup-hash
reconciliation, so a fresh sandbox can fail on the startup-minted .env drift.
Update the `begin_shields_transition` / `_seal_shields_locked` flow to either
accept and pass through `expected_config_sha256` for the `mode="locked"` path or
explicitly prevent shields-up-first when that reconciliation cannot happen. Keep
the reconciliation logic aligned with `_reconcile_nonroot_startup_api_key_hash`
and `_verify_strict_hash` so the locked path handles the same startup hash drift
as `config-write`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 37e82bec-481c-4dbd-b93e-5b0edfeefae1
📒 Files selected for processing (3)
agents/hermes/runtime-config-guard.pysrc/lib/shields/index.tstest/hermes-nonroot-strict-hash-reconciliation.test.ts
11 tasks
Complements the shields-down fix: the first shields up on a fresh OpenShell-managed Hermes sandbox failed with "Config not locked: parent dir mode=755 (expected 1775), parent dir owner=root:root (expected root:sandbox)". For the sealed Hermes transaction the parent (/sandbox) posture — 1775 root:sandbox — is deliberately the last persistent change, applied by finish-shields-transition; the guard keeps /sandbox root-owned as its crash-consistency orphan marker until finish. lockAgentConfigUnderMutationLock verified parent protection between apply and finish, so it always observed the frozen 755 root:root posture and reported a false lock failure (the catch path then committed the correct posture but still surfaced exit 1 and left shields state DOWN). Defer parent-protection verification for the sealed Hermes path to a post-finish re-verify, where the 1775 root:sandbox posture is in place. Locked files, config-dir mode, and chattr are still checked before finish; OpenClaw and legacy-Hermes paths (no sealed transaction) keep the inline check. A full shields down -> up -> down -> up cycle now completes on a fresh sandbox. Fixes #6381 Signed-off-by: Yanyun Liao <yanyunl@nvidia.com>
…rict-hash-reconcile-6381
…rict-hash-reconcile-6381
Replace the `if (expectedDigest !== undefined)` push in the test helper with a ternary spread so the changed test file adds no `if` statements, satisfying the codebase-growth-guardrails "no added if statements" gate. No behavioral change. Signed-off-by: Yanyun Liao <yanyunl@nvidia.com>
21 tasks
Contributor
|
SWQA E2E follow-up: #6398 adds a dedicated CPU-only live regression for this fix. It performs a fresh non-root Hermes onboard followed by Against current Failing run: https://github.com/NVIDIA/NemoClaw/actions/runs/28874817718 #6398 is intentionally draft/blocked on this PR. After #6384 lands, we will rerun the same lane and expect both shields cycles to pass. No GPU or hosted inference secret is required. |
cv
requested changes
Jul 7, 2026
cv
left a comment
cv
left a comment
Collaborator
There was a problem hiding this comment.
The implementation looks security-correct, and I do not think the CodeRabbit shields-locked suggestion applies: locked mode intentionally routes through _seal_shields_locked, freezes the namespace, republishes bounded root-owned inputs, and synthesizes fresh strict/compat anchors so stale mutable state cannot veto containment. Please do not thread mutable reconciliation into that path.
Two in-scope host contracts still need deterministic coverage before approval:
- Add a TypeScript/host regression proving the mutable transition reads a valid
sha256sumand forwards it as--expected-config-sha256tobegin-shields-transition(plus a malformed/missing hash fail-closed assertion if practical). The new Python tests bypass this wiring. - Add a stateful host regression proving parent protection is skipped before
finish-shields-transition, then enforced after finish, with finish occurring before the final parent check. The current tests do not cover this ordering.
Please also link a passing #6398 run on a combined/exact-fix head. Its current run 28874817718 reproduces the failure on the test-only head without #6384, but does not yet validate this fix. These are trust-anchor and sandbox lifecycle changes, so the exact host wiring and live cycle should be evidenced.
jyaunches
added a commit
that referenced
this pull request
Jul 7, 2026
<!-- markdownlint-disable MD041 --> ## Summary Adds a CPU-only live E2E that reproduces the fresh Hermes shields lifecycle regression from #6381. The test onboards a new non-root Hermes sandbox, runs shields down/up twice, and preserves the failure as a dedicated E2E lane until #6384 lands. ## Related Issue Relates to #6381. Depends on #6384. ## Changes - Add a fresh Hermes onboard and two-cycle shields down/up live regression test. - Assert the mutable and locked ownership/mode contracts after each transition. - Add a dedicated `hermes-shields-config` workflow job that requires no GPU or hosted inference secret. - Extend the E2E artifact workflow boundary for the new job. ## 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 <!-- Check exactly one tests line and one docs line. Check other lines when applicable. Add every requested justification or approval reference. --> - [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: test and CI coverage only; no user-facing behavior changes - [ ] 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: - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Verification <!-- Check each applicable item only when supported by the requested evidence. Run targeted tests once per relevant change set and rerun after later edits or hook autofixes that can affect the tested behavior. Do not rerun hook-covered checks. --> - [x] PR description includes the DCO sign-off declaration 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 - [ ] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — command/result or justification: E2E workflow support tests passed (24/24), and the live target collects successfully; the live run is intentionally expected to reproduce #6381 on current `main` - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/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) --- <!-- DCO sign-off is required in this PR description, and every commit must appear as Verified in GitHub. Run: git config user.name && git config user.email --> Signed-off-by: Julie Yaunches <jyaunches@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added a new Hermes live end-to-end check that runs repeated shields cycles in a fresh non-root sandbox. * Updated PR reporting so the new live job is included in the results summary. * **Bug Fixes** * Updated E2E artifact upload workflow boundary validations to match the current number of expected E2E execution jobs and default callers. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
jyaunches
self-requested a review
July 7, 2026 16:53
Contributor
E2E Target Results — ✅ All requested jobs passedRun: 28883593471
|
Contributor
|
✅ SWQA follow-up: after updating this branch with current
This is the same test that reproduced |
wscurran
added
area: sandbox
21 tasks
Collaborator
|
Closing as superseded by current The mainline implementation is the safer final form: for the first mutable transition, the root-owned runtime guard parses the expected I attempted a current-main merge to assess salvage. All three conflicts were these independently landed changes, and resolving toward this branch would restore redundant host digest forwarding instead of improving main. The contributor work and live validation remain reflected in the mainline fix; this PR no longer has a distinct safe delta for v0.0.78. |
cv
added a commit
that referenced
this pull request
Jul 8, 2026
…6431) <!-- markdownlint-disable MD041 --> ## Summary Replaces NemoClaw's build-time mutation of the released Deep Agents bootstrap with a first-party `deepagents.harness_profiles` plugin for `deepagents-code==0.1.34` / `deepagents==0.7.0a6`. The two managed OpenAI-compatible model keys continue to receive the released native Nemotron 3 Ultra profile, with exact version/source gates and no third-party source changes. ## Related Issue Fixes #6424 ## Changes - Add and install `nemoclaw-deepagents-profile==0.1.0` through Deep Agents' supported profile entry-point lifecycle. - Register only the two NemoClaw-managed aliases against the released canonical Ultra profile, atomically and idempotently. - Fail the image build on missing or unimportable dependencies, mismatched distribution/package roots, copied/installed adapter-source drift, or released-profile/bootstrap drift. - Run a DCode-only negative Docker build from the current hash-locked base, strip both upstream distributions, and prove failure occurs at the isolated import gate before the later dependency check. - Build and install a real unreviewed-version plugin wheel and prove the actual validator rejects it. - Verify entry-point discovery, all 12 middleware entries, unrelated-model isolation, graph compilation, and allowed/denied execute-dispatch parity against the official wheels. - Split image/runtime and credential-boundary contracts into balanced 756/755-line suites with a 113-line shared helper, preserving all 75 original tests and substantial per-file size headroom. - Remove the installed-bootstrap patcher and document that the adapter must be removed, not rehashed, once reviewed dependencies provide both exact aliases. - Preserve the merged DCode hardening and paced `/agents` first-run TUI behavior from #6410 / #6418. ## Automated review dispositions - **License metadata:** the production package keeps the PEP 639 SPDX string and builds unchanged with lock-pinned `setuptools==82.0.1`; the production validator now requires exact installed-wheel `License-Expression: Apache-2.0` metadata, with a negative metadata-drift test. The legacy conversion is a localized offline wrong-version fixture with explicit source-boundary and removal-condition documentation. Remove the fixture-only conversion once runner setuptools accepts PEP 639 strings; production never uses it. - **Plain-progress build output:** plain progress remains necessary to prove the exact import-failure marker. Before Docker runs, the gate now rejects every Docker `ARG` name outside a complete reviewed allowlist, while tests pin the only passed build arguments to the two public `BASE_IMAGE` references. Behavior tests inject unreviewed uppercase, lowercase, and continued ARG declarations across all three Dockerfiles and prove rejection occurs before any build; the targeted DCode E2E job runs the same script with real Docker before live tests. - **Adapter build-layer retention:** Docker can retain the copied project tree in an image layer or failed local build cache. This is accepted because it contains only public, first-party Apache-2.0 source and metadata, while the installed Python module necessarily ships the same source; revisit if any adapter input becomes secret-bearing or non-public. - **Credential redaction parity:** `PASS`/`PASSWD`, quoted/space-separated assignments, punctuation-bearing values, and bounded camel/acronym aliases now share the same fail-closed policy across the Bash wrapper, managed Python runtime, observability scrubber, config filter, full/sensitive-text redactors, structured-log classifier, TUI sanitizer, and E2E redactors. The separator lookbehind is capped at 32 horizontal characters to prevent attacker-controlled scans; private-key blocks are scrubbed before assignment matching. Positive tests cover `customPass`, `DBPass`, and known secret `*Key` families, while `COMPASS`/`BYPASS`, `TOPSECRET`/`SUBTOKEN`, pass-rate fields, `publicKey`, and `customKey` remain untouched. - **OpenShell TLS key provenance:** the canonical mounted path is intentionally accepted only from the supervisor-owned runtime environment and rejected from the mutable DCode `.env`. The split credential suite now proves both sides explicitly, matching the existing wrapper-identity coverage; allowing it in `.env` would weaken the boundary. - **Docker auth cleanup:** the shared workflow validator requires exactly one canonical cleanup with `if: always()` as the final job step. A DCode-specific mutation test now also rejects moving cleanup before the import gate. - **Private-key and fixture helpers:** multiline private-key matching is consolidated into the live generic matcher with a required-newline mode, preserving comment behavior while removing 12 lines. Profile-hash fixture replacement is now whitespace/quote tolerant while still requiring one exact reviewed constant and digest. A focused regression covers both formatting variants and duplicate-definition rejection; use an AST transform only if the current two-constant scope grows. ## 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: public CLI, configuration, model IDs, and user-visible behavior are unchanged; the existing DCode quickstart is implementation-neutral. - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — the prior security and supply-chain approval is #6431 (review); fresh exact-head re-review will be requested after the current full fan-out because the head changed. - [x] Non-success, skipped, or missing CI check accepted by maintainer — `e2e-all` baseline failures accepted in #6431 (review); follow-ups #6381/#6384 and #6467/#6474. ## Verification - [x] PR description includes the DCO sign-off declaration 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 — `npm run check:diff` passed on `e78d3ef7`; the independently runnable image/runtime and credential-boundary suites passed 18 of 18 and 123 of 123; the final cross-surface security/parity audit passed 96 of 96; fresh-cache real-wheel validation and the isolated three-package import probe passed. - [ ] Applicable broad gate passed — exact-head focused DCode run [28971629788](https://github.com/NVIDIA/NemoClaw/actions/runs/28971629788) and cloud-onboard run [28971629739](https://github.com/NVIDIA/NemoClaw/actions/runs/28971629739) passed on `e78d3ef7`; full fan-out run [28972330045](https://github.com/NVIDIA/NemoClaw/actions/runs/28972330045) is in progress. The prior full run [28919686103](https://github.com/NVIDIA/NemoClaw/actions/runs/28919686103) passed 77 of 79 applicable jobs; its two failures reproduced identically on retry and `main` run [28911441118](https://github.com/NVIDIA/NemoClaw/actions/runs/28911441118), with the prior maintainer waiver recorded [here](#6431 (review)). - [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) ## Exact-head advisor evidence - [`ubuntu-repo-cloud-langchain-deepagents-code` run 28971629788](https://github.com/NVIDIA/NemoClaw/actions/runs/28971629788) passed on exact head `e78d3ef7f43f3242486dedc4b5b2b42e0585d041`. The production-image validator covered plugin discovery and installed-distribution binding, official source hashes, both aliases and all 12 middleware entries, unrelated-model isolation, graph compilation, and allowed/denied execute dispatch parity. - The same exact-head run passed the real-Docker stripped-dependency import gate before live E2E, then passed image version checks (`deepagents-code==0.1.34`, `deepagents==0.7.0a6`), direct and login-shell headless `PONG`, and interactive TUI acceptance with the optional name prompt and no model picker. - The advisor-required [`cloud-onboard` run 28971629739](https://github.com/NVIDIA/NemoClaw/actions/runs/28971629739) also passed on that exact SHA. - CodeRabbit is green with no unresolved threads. Exact-head advisor run 28971565095 reported zero GPT findings but requested the runtime evidence above; Nemotron's two attempts were non-advisory JSON-parse failures. Both advisors will be rerun against this updated evidence. - The localized import-gate removal condition is tracked in #6424 rather than a new cleanup issue. - Exact-head full fan-out run [28972330045](https://github.com/NVIDIA/NemoClaw/actions/runs/28972330045) is in progress; the prior baseline waiver remains applicable only if the same two unrelated failures recur. --- 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 a first-party Nemotron 3 Ultra profile plugin that registers managed model aliases. * **Bug Fixes / Security** * Strengthened fail-closed validation for the released profile, including integrity checks and managed vs native dispatch parity (with denied-shell behavior). * Hardened secret/credential detection and redaction so `PASS`-keyed values are treated as sensitive. * **CI / Quality** * Added build-time and workflow-boundary checks ensuring images reject missing base dependencies. * **Tests** * Expanded plugin/profile-contract, image behavior, and end-to-end/workflow coverage. * **Chores** * Updated the container build flow to install and validate the plugin artifact at build time, removing the standalone patch approach. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Aaron Erickson <aerickson@nvidia.com> Signed-off-by: Carlos Villela <cvillela@nvidia.com> Signed-off-by: cjagwani <cjagwani@nvidia.com> Co-authored-by: Carlos Villela <cvillela@nvidia.com> Co-authored-by: cjagwani <cjagwani@nvidia.com>
Hadar301
pushed a commit
to Hadar301/NemoClaw-OpenShift
that referenced
this pull request
Jul 12, 2026
<!-- markdownlint-disable MD041 --> ## Summary Adds a CPU-only live E2E that reproduces the fresh Hermes shields lifecycle regression from NVIDIA#6381. The test onboards a new non-root Hermes sandbox, runs shields down/up twice, and preserves the failure as a dedicated E2E lane until NVIDIA#6384 lands. ## Related Issue Relates to NVIDIA#6381. Depends on NVIDIA#6384. ## Changes - Add a fresh Hermes onboard and two-cycle shields down/up live regression test. - Assert the mutable and locked ownership/mode contracts after each transition. - Add a dedicated `hermes-shields-config` workflow job that requires no GPU or hosted inference secret. - Extend the E2E artifact workflow boundary for the new job. ## 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 <!-- Check exactly one tests line and one docs line. Check other lines when applicable. Add every requested justification or approval reference. --> - [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: test and CI coverage only; no user-facing behavior changes - [ ] 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: - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Verification <!-- Check each applicable item only when supported by the requested evidence. Run targeted tests once per relevant change set and rerun after later edits or hook autofixes that can affect the tested behavior. Do not rerun hook-covered checks. --> - [x] PR description includes the DCO sign-off declaration 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 - [ ] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — command/result or justification: E2E workflow support tests passed (24/24), and the live target collects successfully; the live run is intentionally expected to reproduce NVIDIA#6381 on current `main` - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/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) --- <!-- DCO sign-off is required in this PR description, and every commit must appear as Verified in GitHub. Run: git config user.name && git config user.email --> Signed-off-by: Julie Yaunches <jyaunches@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added a new Hermes live end-to-end check that runs repeated shields cycles in a fresh non-root sandbox. * Updated PR reporting so the new live job is included in the results summary. * **Bug Fixes** * Updated E2E artifact upload workflow boundary validations to match the current number of expected E2E execution jobs and default callers. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Hadar301
pushed a commit
to Hadar301/NemoClaw-OpenShift
that referenced
this pull request
Jul 12, 2026
…VIDIA#6431) <!-- markdownlint-disable MD041 --> ## Summary Replaces NemoClaw's build-time mutation of the released Deep Agents bootstrap with a first-party `deepagents.harness_profiles` plugin for `deepagents-code==0.1.34` / `deepagents==0.7.0a6`. The two managed OpenAI-compatible model keys continue to receive the released native Nemotron 3 Ultra profile, with exact version/source gates and no third-party source changes. ## Related Issue Fixes NVIDIA#6424 ## Changes - Add and install `nemoclaw-deepagents-profile==0.1.0` through Deep Agents' supported profile entry-point lifecycle. - Register only the two NemoClaw-managed aliases against the released canonical Ultra profile, atomically and idempotently. - Fail the image build on missing or unimportable dependencies, mismatched distribution/package roots, copied/installed adapter-source drift, or released-profile/bootstrap drift. - Run a DCode-only negative Docker build from the current hash-locked base, strip both upstream distributions, and prove failure occurs at the isolated import gate before the later dependency check. - Build and install a real unreviewed-version plugin wheel and prove the actual validator rejects it. - Verify entry-point discovery, all 12 middleware entries, unrelated-model isolation, graph compilation, and allowed/denied execute-dispatch parity against the official wheels. - Split image/runtime and credential-boundary contracts into balanced 756/755-line suites with a 113-line shared helper, preserving all 75 original tests and substantial per-file size headroom. - Remove the installed-bootstrap patcher and document that the adapter must be removed, not rehashed, once reviewed dependencies provide both exact aliases. - Preserve the merged DCode hardening and paced `/agents` first-run TUI behavior from NVIDIA#6410 / NVIDIA#6418. ## Automated review dispositions - **License metadata:** the production package keeps the PEP 639 SPDX string and builds unchanged with lock-pinned `setuptools==82.0.1`; the production validator now requires exact installed-wheel `License-Expression: Apache-2.0` metadata, with a negative metadata-drift test. The legacy conversion is a localized offline wrong-version fixture with explicit source-boundary and removal-condition documentation. Remove the fixture-only conversion once runner setuptools accepts PEP 639 strings; production never uses it. - **Plain-progress build output:** plain progress remains necessary to prove the exact import-failure marker. Before Docker runs, the gate now rejects every Docker `ARG` name outside a complete reviewed allowlist, while tests pin the only passed build arguments to the two public `BASE_IMAGE` references. Behavior tests inject unreviewed uppercase, lowercase, and continued ARG declarations across all three Dockerfiles and prove rejection occurs before any build; the targeted DCode E2E job runs the same script with real Docker before live tests. - **Adapter build-layer retention:** Docker can retain the copied project tree in an image layer or failed local build cache. This is accepted because it contains only public, first-party Apache-2.0 source and metadata, while the installed Python module necessarily ships the same source; revisit if any adapter input becomes secret-bearing or non-public. - **Credential redaction parity:** `PASS`/`PASSWD`, quoted/space-separated assignments, punctuation-bearing values, and bounded camel/acronym aliases now share the same fail-closed policy across the Bash wrapper, managed Python runtime, observability scrubber, config filter, full/sensitive-text redactors, structured-log classifier, TUI sanitizer, and E2E redactors. The separator lookbehind is capped at 32 horizontal characters to prevent attacker-controlled scans; private-key blocks are scrubbed before assignment matching. Positive tests cover `customPass`, `DBPass`, and known secret `*Key` families, while `COMPASS`/`BYPASS`, `TOPSECRET`/`SUBTOKEN`, pass-rate fields, `publicKey`, and `customKey` remain untouched. - **OpenShell TLS key provenance:** the canonical mounted path is intentionally accepted only from the supervisor-owned runtime environment and rejected from the mutable DCode `.env`. The split credential suite now proves both sides explicitly, matching the existing wrapper-identity coverage; allowing it in `.env` would weaken the boundary. - **Docker auth cleanup:** the shared workflow validator requires exactly one canonical cleanup with `if: always()` as the final job step. A DCode-specific mutation test now also rejects moving cleanup before the import gate. - **Private-key and fixture helpers:** multiline private-key matching is consolidated into the live generic matcher with a required-newline mode, preserving comment behavior while removing 12 lines. Profile-hash fixture replacement is now whitespace/quote tolerant while still requiring one exact reviewed constant and digest. A focused regression covers both formatting variants and duplicate-definition rejection; use an AST transform only if the current two-constant scope grows. ## 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: public CLI, configuration, model IDs, and user-visible behavior are unchanged; the existing DCode quickstart is implementation-neutral. - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — the prior security and supply-chain approval is NVIDIA#6431 (review); fresh exact-head re-review will be requested after the current full fan-out because the head changed. - [x] Non-success, skipped, or missing CI check accepted by maintainer — `e2e-all` baseline failures accepted in NVIDIA#6431 (review); follow-ups NVIDIA#6381/NVIDIA#6384 and NVIDIA#6467/NVIDIA#6474. ## Verification - [x] PR description includes the DCO sign-off declaration 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 — `npm run check:diff` passed on `e78d3ef7`; the independently runnable image/runtime and credential-boundary suites passed 18 of 18 and 123 of 123; the final cross-surface security/parity audit passed 96 of 96; fresh-cache real-wheel validation and the isolated three-package import probe passed. - [ ] Applicable broad gate passed — exact-head focused DCode run [28971629788](https://github.com/NVIDIA/NemoClaw/actions/runs/28971629788) and cloud-onboard run [28971629739](https://github.com/NVIDIA/NemoClaw/actions/runs/28971629739) passed on `e78d3ef7`; full fan-out run [28972330045](https://github.com/NVIDIA/NemoClaw/actions/runs/28972330045) is in progress. The prior full run [28919686103](https://github.com/NVIDIA/NemoClaw/actions/runs/28919686103) passed 77 of 79 applicable jobs; its two failures reproduced identically on retry and `main` run [28911441118](https://github.com/NVIDIA/NemoClaw/actions/runs/28911441118), with the prior maintainer waiver recorded [here](NVIDIA#6431 (review)). - [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) ## Exact-head advisor evidence - [`ubuntu-repo-cloud-langchain-deepagents-code` run 28971629788](https://github.com/NVIDIA/NemoClaw/actions/runs/28971629788) passed on exact head `e78d3ef7f43f3242486dedc4b5b2b42e0585d041`. The production-image validator covered plugin discovery and installed-distribution binding, official source hashes, both aliases and all 12 middleware entries, unrelated-model isolation, graph compilation, and allowed/denied execute dispatch parity. - The same exact-head run passed the real-Docker stripped-dependency import gate before live E2E, then passed image version checks (`deepagents-code==0.1.34`, `deepagents==0.7.0a6`), direct and login-shell headless `PONG`, and interactive TUI acceptance with the optional name prompt and no model picker. - The advisor-required [`cloud-onboard` run 28971629739](https://github.com/NVIDIA/NemoClaw/actions/runs/28971629739) also passed on that exact SHA. - CodeRabbit is green with no unresolved threads. Exact-head advisor run 28971565095 reported zero GPT findings but requested the runtime evidence above; Nemotron's two attempts were non-advisory JSON-parse failures. Both advisors will be rerun against this updated evidence. - The localized import-gate removal condition is tracked in NVIDIA#6424 rather than a new cleanup issue. - Exact-head full fan-out run [28972330045](https://github.com/NVIDIA/NemoClaw/actions/runs/28972330045) is in progress; the prior baseline waiver remains applicable only if the same two unrelated failures recur. --- 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 a first-party Nemotron 3 Ultra profile plugin that registers managed model aliases. * **Bug Fixes / Security** * Strengthened fail-closed validation for the released profile, including integrity checks and managed vs native dispatch parity (with denied-shell behavior). * Hardened secret/credential detection and redaction so `PASS`-keyed values are treated as sensitive. * **CI / Quality** * Added build-time and workflow-boundary checks ensuring images reject missing base dependencies. * **Tests** * Expanded plugin/profile-contract, image behavior, and end-to-end/workflow coverage. * **Chores** * Updated the container build flow to install and validate the plugin artifact at build time, removing the standalone patch approach. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Aaron Erickson <aerickson@nvidia.com> Signed-off-by: Carlos Villela <cvillela@nvidia.com> Signed-off-by: cjagwani <cjagwani@nvidia.com> Co-authored-by: Carlos Villela <cvillela@nvidia.com> Co-authored-by: cjagwani <cjagwani@nvidia.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
On a freshly built, OpenShell-managed (non-root) Hermes sandbox,
nemohermes <sandbox> shields downalways failed with[SECURITY] strict hash verification failed for Hermes restart seal, andshields upthen failed withConfig not locked: parent dir mode=755 (expected 1775), blocking the entire Hermes shields lifecycle that MCP mutations depend on. This PR fixes both so a fulldown → upcycle works on a fresh sandbox.Closes #6381.
Reproduction
On our Ubuntu 24.04 x86_64 test host (no GPU), against a fresh Hermes sandbox onboarded non-interactively from latest
main:Environment
main; Hermes agent, OpenShell-managed (non-root) topology (no/run/nemoclaw/hermes-root-lifecyclemarker).Smoking gun (captured before running shields down) — the root-owned strict anchor
/etc/nemoclaw/hermes.config-hashcarries the stale build-time.envdigest, while the in-tree compat anchor.config-hashand the actual.envagree on the current digest;config.yamlis byte-identical across both anchors. The only difference is the single 64-hexAPI_SERVER_KEYminted at first startup.Observed on
main(before fix)Observed on
fix/...(after fix) — a full cycle succeeds:Analysis
OpenShell launches the Hermes entrypoint as the
sandboxuser (agents/hermes/start.shtakes the non-root branch; the root-only/run/nemoclaw/hermes-root-lifecyclemarker is never written). That non-root startup mints the per-sandboxAPI_SERVER_KEYinto.envand refreshes the in-tree compat hash anchor, but cannot write the root-owned strict anchor/etc/nemoclaw/hermes.config-hash, which keeps its build-time digest.begin-shields-transition(as root) →seal_restart(purpose="shields-mutable"), which verifies the current config/env against the stale strict anchor via_verify_strict_hashand fails on every fresh non-root sandbox. The guard already reconciles this exact drift for theconfig-writepurpose via_reconcile_nonroot_startup_api_key_hash, butshields-mutablewas not wired to use it./sandbox) posture1775 root:sandboxis deliberately the last persistent change and is applied byfinish-shields-transition(the guard keeps/sandboxroot-owned as its crash-consistency orphan marker until finish).lockAgentConfigUnderMutationLockverified parent protection between apply and finish, so it always saw the frozen755 root:rootposture and reported a false lock failure.Fix
agents/hermes/runtime-config-guard.py: allowseal_restart'sStrictHashMismatchErrorrecovery for theshields-mutablepurpose (not justconfig-write) when an expected config digest is supplied; thread a new optionalexpected_config_sha256throughbegin_shields_transition; accept/validate--expected-config-sha256(64-hex) in the CLI handler. The reconciliation itself is unchanged — it advances the strict anchor only when the compat anchor corroborates the frozen inputs,config.yamlis unchanged, and the sole.envdelta is the single generatedAPI_SERVER_KEY; every other difference is refused, and the posture must be mutable/never-locked.src/lib/shields/index.ts:mutableHermes transition, read the currentconfig.yamldigest and pass it as--expected-config-sha256. Absent the digest the stale anchor still fails closed, so reconciliation is opt-in.1775 root:sandboxposture is actually in place. Locked files, config-dir mode, and chattr are still checked before finish; OpenClaw and legacy-Hermes paths (no sealed transaction) keep the inline check.Tests:
test/hermes-nonroot-strict-hash-reconciliation.test.tsgains three shields-down cases (reconciles the startup key and completes the transition; still fails closed with no digest; refuses config drift), reusing the existing fixture that already locks down the reconciliation's other refusals. The shields-up ordering fix is validated end-to-end on the test host (fulldown → up → down → upcycle) and against the existing shields unit suite (no regressions).Changes
agents/hermes/runtime-config-guard.py: enable the existing non-root API-key reconciliation for theshields-mutabletransition.src/lib/shields/index.ts: pass the config digest tobegin-shields-transitionfor mutable transitions; verify Hermes parent lock posture after finish.test/hermes-nonroot-strict-hash-reconciliation.test.ts: cover the shields-down reconcile, fail-closed, and config-drift-refusal cases.Type of Change
Verification
npx prek run --all-filespasses (on changed files)npm testpasses (touched files at minimum)make docsbuilds without warnings (doc changes only)AI Disclosure
Signed-off-by: Yanyun Liao yanyunl@nvidia.com
Summary by CodeRabbit
New Features
Bug Fixes
Tests