fix(gpu): prefer native OpenShell injection#6142
Merged
Merged
Conversation
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
ericksoa
added
v0.0.72
Contributor
📝 WalkthroughWalkthroughThe PR tightens Hermes startup and environment-boundary checks, updates Docker GPU patch selection and diagnostics, adds Hermes GPU startup proof/live E2E coverage, wires a new workflow job into validation, and updates related docs and tests. ChangesHermes GPU startup validation
Estimated code review effort: 5 (Critical) | ~120 minutes 🚥 Pre-merge checks | ✅ 5✅ 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 branch is 96%. Coverage data for the branch is not yet available. Show a code coverage summary of the most covered files.
TypeScript / code-coverage/cliThe overall coverage in the branch is 69%. Coverage data for the branch is not yet available. Show a code coverage summary of the most covered files.
Updated |
Contributor
PR Review Advisor — No blocking findingsMerge posture: No blocking advisor findings Action checklist
Test follow-ups to resolve or justifyIf these cover changed behavior, prefer adding them in this PR; otherwise state why existing coverage is enough or link the follow-up.
This is an automated, non-binding review; it still expects maintainers and agents to respond to each required or warning item. Treat suggestions as current-PR improvements when they touch changed code; defer only with maintainer rationale or a linked follow-up. A human maintainer must make the final merge decision. |
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: Dispatch required E2E targets:
Full E2E target advisor summaryE2E Target AdvisorBase: Required E2E targets
Optional E2E targets
Relevant changed files
|
Contributor
PR Review Advisor (Nemotron Ultra) — Changes requestedMerge posture: Do not merge yet Action checklist
Findings index
🚨 Required before mergeAddress these before merging unless a maintainer explicitly overrides the advisor with rationale.
|
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Contributor
Vitest E2E Target Results —
|
| Job | Result |
|---|---|
| hermes-gpu-startup | |
| hermes-root-entrypoint-smoke |
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Contributor
Vitest E2E Target Results — ❌ Some jobs failedRun: 28548219046
|
1 similar comment
Contributor
Vitest E2E Target Results — ❌ Some jobs failedRun: 28548219046
|
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
test/e2e/live/hermes-gpu-startup-proof.ts (2)
65-78: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCombined multi-assertion script obscures which invariant failed.
The
startupConfigscript chains four distinct checks (API-key format regex, twosha256sum -chash validations, and a startup-guard log grep) into a single exit code /OKstring. When it fails, the only signal is exit code and stdout — you can't tell from the assertion which specific invariant broke without manually digging into captured stderr/artifacts. Splitting these into separateexecShellcalls with individualexpectassertions would make regressions immediately actionable.
[recommended_refactor: better failure diagnosability for a regression-proof test]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/live/hermes-gpu-startup-proof.ts` around lines 65 - 78, The startupConfig check in hermes-gpu-startup-proof currently combines multiple independent invariants into one shell command, making failures hard to diagnose. Split the checks in trustedSandboxShellScript into separate sandbox.execShell calls and add individual expect assertions for the API_SERVER_KEY regex, /etc/nemoclaw/hermes.config-hash, /sandbox/.hermes/.config-hash, and the /tmp/nemoclaw-start.log guard grep so each failure points to the exact invariant that broke.
79-141: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueTwo sequential
docker pscalls introduce a small TOCTOU window.The running-containers check (lines 79-104) and the all-containers check (lines 118-140) query Docker separately; a container could theoretically stop/restart between the two calls, causing a spurious length mismatch. Low risk in this deterministic sandbox-lifecycle test, but worth noting if flakiness is observed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/live/hermes-gpu-startup-proof.ts` around lines 79 - 141, The test in hermes-gpu-startup-proof.ts has a small TOCTOU window because it calls docker ps twice, once for running containers and once for all containers, so the container state can change between checks. Update the assertions around the host.command docker queries to reuse a single snapshot of container listings, or otherwise combine the checks so the container identity and state are validated from the same point in time. Use the existing runningContainers, allContainers, and containerState logic to keep the verification atomic.tools/e2e/hermes-gpu-startup-workflow-boundary.mts (1)
22-23: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated explicit-only selector format.
EXPECTED_SELECTORhardcodes the samecontains(format(...))pattern thatexplicitOnlyFreeStandingJobIfalready builds for other explicit-only jobs (used forjetson-nvmap-gpu,sandbox-rlimits-connect, etc. inworkflow-boundary.mts). Duplicating the literal string risks drift if the shared helper's format ever changes.♻️ Suggested refactor
-const EXPECTED_SELECTOR = - "${{ contains(format(',{0},', inputs.jobs), ',hermes-gpu-startup,') || contains(format(',{0},', inputs.targets), ',hermes-gpu-startup,') }}"; +const EXPECTED_SELECTOR = explicitOnlyFreeStandingJobIf(JOB_NAME, JOB_NAME);(requires exporting
explicitOnlyFreeStandingJobIffromworkflow-boundary.mtsand importing it here.)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/e2e/hermes-gpu-startup-workflow-boundary.mts` around lines 22 - 23, `EXPECTED_SELECTOR` is duplicating the explicit-only selector template instead of reusing the shared builder. Export `explicitOnlyFreeStandingJobIf` from `workflow-boundary.mts` and import it into `hermes-gpu-startup-workflow-boundary.mts`, then construct the hermes selector through that helper using the hermes job name. Keep the existing `EXPECTED_SELECTOR` constant only as the derived result, not a hardcoded `contains(format(...))` literal, so it stays aligned with the other explicit-only job selectors.
🤖 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.
Nitpick comments:
In `@test/e2e/live/hermes-gpu-startup-proof.ts`:
- Around line 65-78: The startupConfig check in hermes-gpu-startup-proof
currently combines multiple independent invariants into one shell command,
making failures hard to diagnose. Split the checks in trustedSandboxShellScript
into separate sandbox.execShell calls and add individual expect assertions for
the API_SERVER_KEY regex, /etc/nemoclaw/hermes.config-hash,
/sandbox/.hermes/.config-hash, and the /tmp/nemoclaw-start.log guard grep so
each failure points to the exact invariant that broke.
- Around line 79-141: The test in hermes-gpu-startup-proof.ts has a small TOCTOU
window because it calls docker ps twice, once for running containers and once
for all containers, so the container state can change between checks. Update the
assertions around the host.command docker queries to reuse a single snapshot of
container listings, or otherwise combine the checks so the container identity
and state are validated from the same point in time. Use the existing
runningContainers, allContainers, and containerState logic to keep the
verification atomic.
In `@tools/e2e/hermes-gpu-startup-workflow-boundary.mts`:
- Around line 22-23: `EXPECTED_SELECTOR` is duplicating the explicit-only
selector template instead of reusing the shared builder. Export
`explicitOnlyFreeStandingJobIf` from `workflow-boundary.mts` and import it into
`hermes-gpu-startup-workflow-boundary.mts`, then construct the hermes selector
through that helper using the hermes job name. Keep the existing
`EXPECTED_SELECTOR` constant only as the derived result, not a hardcoded
`contains(format(...))` literal, so it stays aligned with the other
explicit-only job selectors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ab4d5bbc-8aa6-4d72-adc3-c742e2b09674
📒 Files selected for processing (6)
.github/workflows/e2e.yamltest/e2e/live/hermes-gpu-startup-proof.tstest/e2e/live/hermes-gpu-startup.test.tstest/e2e/support/hermes-workflow-boundary.test.tstools/e2e/hermes-gpu-startup-workflow-boundary.mtstools/e2e/workflow-boundary.mts
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/e2e.yaml
Contributor
Vitest E2E Target Results — ❌ Some jobs failedRun: 28549714069
|
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Contributor
Vitest E2E Target Results — ❌ Some jobs failedRun: 28550860758
|
…uardrail codebase-growth-guardrails rejects new if statements in test files. Replace the diagnostic capture if-branch with a ternary so the guardrail passes without changing behaviour. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
ericksoa
changed the title
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Contributor
Vitest E2E Target Results —
|
| Job | Result |
|---|---|
| hermes-gpu-startup | |
| hermes-root-entrypoint-smoke | ✅ success |
cv
approved these changes
Jul 1, 2026
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Contributor
Vitest E2E Target Results — ❌ Some jobs failedRun: 28552985970
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/e2e/live/hermes-gpu-startup-proof.ts (1)
76-76: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake the PID 1 argv rejection catch embedded
nemoclaw-starttokens.Line 76 only rejects exact argv items, so a polluted supervisor argument such as
--command=/usr/local/bin/nemoclaw-startwould pass while still carrying the superseded startup path.Proposed test hardening
- String.raw`python3 -c 'import json; from pathlib import Path; argv=[item.decode("utf-8", "strict") for item in Path("/proc/1/cmdline").read_bytes().split(b"\0") if item]; print(json.dumps({"argv0": argv[0] if argv else "", "has_nemoclaw_start": any(item in ("nemoclaw-start", "/usr/local/bin/nemoclaw-start") for item in argv)}))'`, + String.raw`python3 - <<'PY' +import json +from pathlib import Path + +argv = [ + item.decode("utf-8", "strict") + for item in Path("/proc/1/cmdline").read_bytes().split(b"\0") + if item +] +print(json.dumps({ + "argv0": argv[0] if argv else "", + "has_nemoclaw_start": any("nemoclaw-start" in item for item in argv), +})) +PY`,As per path instructions, migration tests must prove the superseded path is unreachable or removed, not merely prove that the new path also works.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/live/hermes-gpu-startup-proof.ts` at line 76, The PID 1 argv check in hermes-gpu-startup-proof is too strict because it only matches exact argv entries, so embedded superseded startup paths can slip through. Update the command in the test to detect any argv item containing the legacy nemoclaw-start path/token, and keep the check anchored around the existing /proc/1/cmdline parsing and has_nemoclaw_start logic so the migration proof fails if the old path is merely hidden inside another argument.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/e2e/support/hermes-gpu-startup-integrity.test.ts`:
- Around line 70-87: runProof() currently inherits the full process.env, so bash
-c can pick up host-specific variables and make the proof behavior
non-deterministic. Update runProof to use a scrubbed environment instead of
process.env, then explicitly add only the variables each test case needs; keep
the PYTHONPATH override scoped to the shadowing scenario. Use the runProof
helper and its spawnSync/bash -c invocation as the place to apply the minimal
env.
In `@test/e2e/support/hosted-inference.test.ts`:
- Around line 429-444: The request assertions in hosted-inference.test.ts only
verify one forbidden-marker occurrence and can miss extra leaks because the fake
server records matches via forbiddenMarkerMatches rather than the raw marker
string. Tighten the test around fake.requests() by asserting the total
forbidden-marker count across all recorded requests, not just arrayContaining
entries or JSON text. Use the existing request shape fields such as auth, path,
stream, and forbiddenMarkerMatches to locate the relevant assertion block and
make the expectation fail if any authenticated request includes the marker.
---
Outside diff comments:
In `@test/e2e/live/hermes-gpu-startup-proof.ts`:
- Line 76: The PID 1 argv check in hermes-gpu-startup-proof is too strict
because it only matches exact argv entries, so embedded superseded startup paths
can slip through. Update the command in the test to detect any argv item
containing the legacy nemoclaw-start path/token, and keep the check anchored
around the existing /proc/1/cmdline parsing and has_nemoclaw_start logic so the
migration proof fails if the old path is merely hidden inside another argument.
🪄 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: 5d535b9e-e2e0-409c-a1a5-d19b3a3559fc
📒 Files selected for processing (22)
agents/hermes/Dockerfileagents/hermes/validate-env-secret-boundary.pydocs/reference/commands-nemohermes.mdxdocs/reference/commands.mdxdocs/reference/troubleshooting.mdxsrc/lib/onboard.tssrc/lib/onboard/docker-gpu-local-inference.test.tssrc/lib/onboard/docker-gpu-local-inference.tssrc/lib/onboard/docker-gpu-patch.tssrc/lib/onboard/docker-gpu-pre-rollback-diagnostics.test.tssrc/lib/onboard/docker-gpu-pre-rollback-diagnostics.tssrc/lib/onboard/docker-gpu-sandbox-create.test.tstest/e2e/fixtures/fake-openai-compatible.tstest/e2e/lib/fake-openai-compatible-api.mtstest/e2e/live/hermes-gpu-startup-integrity.tstest/e2e/live/hermes-gpu-startup-proof.tstest/e2e/live/hermes-gpu-startup.test.tstest/e2e/support/hermes-gpu-startup-integrity.test.tstest/e2e/support/hosted-inference.test.tstest/e2e/support/jetson-workflow-boundary.test.tstest/hermes-gateway-wrapper.test.tstest/hermes-openshell-runtime-env-boundary.test.ts
✅ Files skipped from review due to trivial changes (2)
- test/e2e/support/jetson-workflow-boundary.test.ts
- docs/reference/commands-nemohermes.mdx
🚧 Files skipped from review as they are similar to previous changes (8)
- src/lib/onboard/docker-gpu-local-inference.ts
- src/lib/onboard/docker-gpu-pre-rollback-diagnostics.test.ts
- src/lib/onboard/docker-gpu-local-inference.test.ts
- test/e2e/live/hermes-gpu-startup.test.ts
- src/lib/onboard.ts
- src/lib/onboard/docker-gpu-pre-rollback-diagnostics.ts
- src/lib/onboard/docker-gpu-sandbox-create.test.ts
- src/lib/onboard/docker-gpu-patch.ts
Contributor
Vitest E2E Target Results — ✅ All requested jobs passedRun: 28568959028
|
Contributor
Vitest E2E Target Results — ❌ Some jobs failedRun: 28568490864
|
Contributor
Vitest E2E Target Results — ❌ Some jobs failedRun: 28568069499
|
Contributor
Vitest E2E Target Results — ✅ All requested jobs passedRun: 28568954862
|
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Contributor
|
🌿 Preview your docs: https://nvidia-preview-pr-6142.docs.buildwithfern.com/nemoclaw |
Contributor
Vitest E2E Target Results — ✅ All requested jobs passedRun: 28597329591
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/lib/onboard/docker-gpu-diagnostic-redaction.ts (2)
52-69: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winNo minimum length on discovered sensitive values before blanket substring redaction.
discoverDockerGpuDiagnosticSensitiveValuesonly filtersvalue.length > 0, andredactTextdoes a globalsplit(value).join("<REDACTED>")for every discovered value. A short env value (e.g. a 1–2 character token/flag matchingSENSITIVE_ENV_KEYor anEXTRA_PLACEHOLDER_KEYSentry) would get replaced everywhere it appears in every diagnostic artifact, including inside unrelated timestamps, exit codes, or container-id fragments — degrading the usefulness of the whole bundle for a single short value.♻️ Suggested guard
.filter( ([key, value]) => - (SENSITIVE_ENV_KEY.test(key) || extraPlaceholderKeys.has(key)) && value.length > 0, + (SENSITIVE_ENV_KEY.test(key) || extraPlaceholderKeys.has(key)) && value.length >= 6, )Also applies to: 76-86
🤖 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 `@src/lib/onboard/docker-gpu-diagnostic-redaction.ts` around lines 52 - 69, `discoverDockerGpuDiagnosticSensitiveValues` is collecting any non-empty matched env value, which can cause `redactText` to blanket-replace very short strings across unrelated diagnostics. Update the filtering in `discoverDockerGpuDiagnosticSensitiveValues` (and keep it consistent with any callers like `redactText`) to require a reasonable minimum length before returning values for redaction, so short flags/tokens from `SENSITIVE_ENV_KEY` or `EXTRA_PLACEHOLDER_KEYS_ENV` are not redacted globally.
7-9: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win
SENSITIVE_ENV_KEYpattern misses generic*_KEYsecrets.The regex catches
api_?key/private_?keybut not a barekeysuffix (e.g.ENCRYPTION_KEY,SIGNING_KEY,MASTER_KEY). Values under such names bypass both this heuristic and theNEMOCLAW_EXTRA_PLACEHOLDER_KEYSallowlist unless a caller explicitly opts them in, so they could be written to the on-disk diagnostics bundle unredacted.🔒 Suggested widening
-const SENSITIVE_ENV_KEY = - /(?:api_?key|token|secret|password|credential|authorization|cookie|private_?key|proxy)/i; +const SENSITIVE_ENV_KEY = + /(?:api_?key|_?key$|token|secret|password|credential|authorization|cookie|proxy)/i;🤖 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 `@src/lib/onboard/docker-gpu-diagnostic-redaction.ts` around lines 7 - 9, The redaction heuristic in SENSITIVE_ENV_KEY is too narrow and misses generic secret names ending in KEY, so update the pattern in docker-gpu-diagnostic-redaction.ts to also match bare *_KEY-style environment names such as ENCRYPTION_KEY, SIGNING_KEY, and MASTER_KEY. Keep the existing handling around EXTRA_PLACEHOLDER_KEYS, but widen the matcher used by the redaction logic so these values are treated as sensitive by default and do not get written unredacted by the diagnostics bundle.src/lib/onboard/docker-gpu-pre-rollback-diagnostics.test.ts (1)
173-174: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClock stub hardcodes internal call count.
The fixed 8-element
clockarray ties the test to the exact number ofDate.now()invocations made internally byboundedDiagnosticsDeps/priming/snapshot beforecollectDockerGpuPatchDiagnostics's own inspect loop runs. Any future change adding/removing one timing check would silently shift which call the test lands on, without a clear failure signal pointing at the real cause.Consider driving elapsed time from the actual budget/timeout constants (e.g. mock
Date.nowto return a monotonically increasing value derived fromPRE_ROLLBACK_DIAGNOSTICS_TOTAL_BUDGET_MS) rather than a fixed-length array, so the test communicates intent independent of internal call count.
As per path instructions, "Prefer observable outcomes ... over source-text, private-shape, or mock-call assertions."🤖 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 `@src/lib/onboard/docker-gpu-pre-rollback-diagnostics.test.ts` around lines 173 - 174, The test for collectDockerGpuPatchDiagnostics uses a fixed 8-value Date.now() clock stub, which is tied to an internal call count in boundedDiagnosticsDeps/priming/snapshot. Replace the array-based mock with a monotonically increasing time source driven by the actual budget/timeout constants (for example, based on PRE_ROLLBACK_DIAGNOSTICS_TOTAL_BUDGET_MS) so the test asserts observable elapsed-time behavior rather than the exact number of Date.now() calls. Keep the change localized to the Date.now spy setup in docker-gpu-pre-rollback-diagnostics.test.ts and ensure the inspect loop still exercises the intended timeout path.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/lib/onboard/docker-gpu-diagnostic-redaction.ts`:
- Around line 52-69: `discoverDockerGpuDiagnosticSensitiveValues` is collecting
any non-empty matched env value, which can cause `redactText` to blanket-replace
very short strings across unrelated diagnostics. Update the filtering in
`discoverDockerGpuDiagnosticSensitiveValues` (and keep it consistent with any
callers like `redactText`) to require a reasonable minimum length before
returning values for redaction, so short flags/tokens from `SENSITIVE_ENV_KEY`
or `EXTRA_PLACEHOLDER_KEYS_ENV` are not redacted globally.
- Around line 7-9: The redaction heuristic in SENSITIVE_ENV_KEY is too narrow
and misses generic secret names ending in KEY, so update the pattern in
docker-gpu-diagnostic-redaction.ts to also match bare *_KEY-style environment
names such as ENCRYPTION_KEY, SIGNING_KEY, and MASTER_KEY. Keep the existing
handling around EXTRA_PLACEHOLDER_KEYS, but widen the matcher used by the
redaction logic so these values are treated as sensitive by default and do not
get written unredacted by the diagnostics bundle.
In `@src/lib/onboard/docker-gpu-pre-rollback-diagnostics.test.ts`:
- Around line 173-174: The test for collectDockerGpuPatchDiagnostics uses a
fixed 8-value Date.now() clock stub, which is tied to an internal call count in
boundedDiagnosticsDeps/priming/snapshot. Replace the array-based mock with a
monotonically increasing time source driven by the actual budget/timeout
constants (for example, based on PRE_ROLLBACK_DIAGNOSTICS_TOTAL_BUDGET_MS) so
the test asserts observable elapsed-time behavior rather than the exact number
of Date.now() calls. Keep the change localized to the Date.now spy setup in
docker-gpu-pre-rollback-diagnostics.test.ts and ensure the inspect loop still
exercises the intended timeout path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d22ab199-ad6b-4e0c-9add-ffd77fdf44ad
📒 Files selected for processing (7)
src/lib/onboard/docker-gpu-diagnostic-redaction.test.tssrc/lib/onboard/docker-gpu-diagnostic-redaction.tssrc/lib/onboard/docker-gpu-patch.tssrc/lib/onboard/docker-gpu-pre-rollback-diagnostics.test.tssrc/lib/onboard/docker-gpu-pre-rollback-diagnostics.tstest/e2e/support/hermes-gpu-startup-integrity.test.tstest/e2e/support/hosted-inference.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- test/e2e/support/hermes-gpu-startup-integrity.test.ts
- test/e2e/support/hosted-inference.test.ts
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Contributor
|
🌿 Preview your docs: https://nvidia-preview-pr-6142.docs.buildwithfern.com/nemoclaw |
Contributor
Vitest E2E Target Results — ✅ All requested jobs passedRun: 28599180561
|
Contributor
Vitest E2E Target Results — ❌ Some jobs failedRun: 28597318221
|
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Contributor
Vitest E2E Target Results —
|
| Job | Result |
|---|---|
| credential-sanitization | |
| gpu-e2e | |
| hermes-gpu-startup | |
| hermes-root-entrypoint-smoke | |
| hermes-sandbox-secret-boundary | |
| messaging-providers |
Contributor
|
🌿 Preview your docs: https://nvidia-preview-pr-6142.docs.buildwithfern.com/nemoclaw |
Contributor
Vitest E2E Target Results — ✅ All requested jobs passedRun: 28601348023
|
Contributor
Vitest E2E Target Results — ✅ All requested jobs passedRun: 28601346031
|
Contributor
Author
|
Exact-head disposition for Candidate and runtime evidence
Nemotron disposition
CodeRabbit disposition
All six inline GitHub review threads are marked resolved. The non-binding Nemotron findings above are dispositioned for maintainer acceptance; the PR has not been merged. Reporter-class DGX Spark aarch64 / DGX Station GB300 NVIDIA Endpoints validation is still unavailable in declared repository runners, so this PR does not claim issue #6110 resolved. |
21 tasks
cv
added a commit
that referenced
this pull request
Jul 2, 2026
<!-- markdownlint-disable MD041 --> ## Summary This PR prepares the user-facing documentation for v0.0.73 before the release plan is frozen. It adds release notes for the merged runtime changes and closes documentation gaps around DNS-backed HTTPS endpoint validation and LangChain Deep Agents Code proxy recovery. ## Changes - Add the `v0.0.73` release-note section with links to the detailed command, inference, recovery, lifecycle, platform, and setup documentation. - Correct the custom endpoint guidance so DNS-backed HTTPS rejection and the supported alternatives match the fail-closed runtime behavior. - Document the managed `inference.local` proxy boundary and rebuild requirement for existing LangChain Deep Agents Code sandboxes. - Add troubleshooting guidance for the DNS-backed HTTPS validation error. - Validate with `npm run docs:sync-agent-variants` and `npm run docs`; Fern completed with 0 errors and 2 existing warnings. - Source summary: - [#6139](#6139) -> `docs/about/release-notes.mdx`, `docs/inference/inference-options.mdx`, `docs/reference/commands.mdx`, `docs/reference/commands-nemohermes.mdx`, and `docs/reference/troubleshooting.mdx`: Document fail-closed DNS-backed HTTPS endpoint handling and recovery options. - [#6142](#6142) -> `docs/about/release-notes.mdx`: Summarize native OpenShell GPU injection and compatibility-path diagnostics. - [#6197](#6197) -> `docs/about/release-notes.mdx`: Summarize agent-aware messaging preset rejection. - [#6199](#6199) -> `docs/about/release-notes.mdx`: Summarize the unreachable-sandbox backup opt-in, restore behavior, and data-loss boundary. - [#6204](#6204) and [#6206](#6206) -> `docs/about/release-notes.mdx` and `docs/get-started/quickstart-langchain-deepagents-code.mdx`: Document the corrected managed proxy contract and required sandbox rebuild. - [#6213](#6213) -> `docs/about/release-notes.mdx`: Summarize the merged setup, recovery, and host-state documentation. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [x] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates <!-- Check all that apply. For any "covered by existing tests", "not applicable", or waiver entry, add a brief justification on the same line or in the Changes section. --> - [ ] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [x] Tests not applicable — justification: documentation-only release preparation; the Fern docs build validates the changed pages and routes. - [x] Docs updated for user-facing behavior changes - [ ] Docs not applicable — justification: - [ ] 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 item you ran and confirmed. Leave unchecked items you skipped. Doc-only changes do not require npm test unless you ran it. --> - [x] PR description includes the DCO sign-off declaration and every commit appears as `Verified` in GitHub - [x] Git hooks passed during commit and push, or `npx prek run --from-ref main --to-ref HEAD` passes - [ ] Targeted tests pass for changed behavior - [ ] Full `npm test` passes (broad runtime changes only) - [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) - [x] 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: Carlos Villela <cvillela@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Added a new **v0.0.73** release notes section with six highlights at the top of the changelog. * Expanded **Custom Endpoint URL Validation** guidance in inference option docs, including explicit acceptance/rejection rules for HTTP vs DNS-backed HTTPS and how validated IPs are stored. * Updated command references (`nemohermes inference set`, `$$nemoclaw inference set`) to match the new validation behavior. * Added troubleshooting documentation for unsupported **DNS-backed HTTPS endpoints**, plus clarified Deep Agents Code routing and post-upgrade sandbox rebuild guidance. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Contributor
|
Hi @ericksoa @cv |
23 tasks
2 tasks
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
Routes ordinary native CDI Linux through OpenShell 0.0.71's native
--gpupath instead of the legacy Docker container swap. The legacy path remains available for WSL, Jetson, and explicitNEMOCLAW_DOCKER_GPU_PATCH=1, with its OpenShell supervisor command boundary and rollback diagnostics hardened.Related Issue
Related to #6110
Changes
NEMOCLAW_DOCKER_GPU_PATCHauto, forced-legacy, and native-routing behavior for ordinary native Linux, Docker Desktop WSL, and Jetson/Tegra.NEMOCLAW_DOCKER_GPU_PATCH=1as the explicit legacy-swap force control and=0as the existing native opt-out; Docker Desktop WSL still ignores=0, and Jetson keeps its compatibility default.OPENSHELL_SANDBOX_COMMAND..enventries remain rejected.Type of Change
Quality Gates
Verification
Verifiedin GitHubnpx prek run --from-ref main --to-ref HEADpassesnpm testpasses (broad runtime changes only)npm run docsbuilds without warnings (doc changes only)Local exact-candidate verification at
d76f1647a5f354ba04737a6a049b82bfbf6d5454:*_KEYand custom-placeholder canaries, JSON-validity, inspect-before-write, exhausted-budget, and collector-owned top regressions pass.a1fc52c7TypeScript entrypoints fail through the CommonJS preload withERR_UNKNOWN_FILE_EXTENSION; exact-head Linux CI remains required.npm run docs:sync-agent-variants,npm run docs:check-agent-variants, andnpm run docspass; Fern reports 0 errors and 2 existing warnings.Live A/B evidence:
97e3e7e1: run 28554699811 reproduced sustained OpenShellErrorfollowed by safe rollback. It also exposed and now closes a lifecycle instrumentation gap: the post-createensureApplied()branch bypassed pre-rollback capture.15f50182: run 28555110558 completed onboarding with exit 0, reachedPhase: Ready, reportedCUDA verified, and sent authenticated Hermes chat-completions requests to the hermetic inference endpoint. The job's only failure was the now-fixed test regex not stripping ANSI aroundReady.7cb219d9: full run 28559814959 and second pass 28559816026 both reached Ready/CUDA with clean runtime and teardown; their Hermes proof stopped on the now-fixed ANSI matcher before downstream assertions.a1fc52c7plus workflow-only child6d0cf6a5: run 28561607207 selected the legacy swap and rolled back cleanly, but failed because the Hermes boundary rejected the driver-owned OpenShellOPENSHELL_TLS_KEYpath. Candidate54cf259dadds an exact runtime-only allowance with negative boundary tests.a1fc52c7: run 28561548749 proved the GPU and security companion jobs, while Hermes GPU stopped at a sandbox-user/procpermission probe after Ready/CUDA. Candidate54cf259dkeeps the same proof but runs it as root and restricts the match to the exactnemoclaw-startprocess.a1fc52c7: run 28561555945 reproduced only the same harness permission failure after Ready/CUDA, correct PID 1 topology, authenticated inference, zero forbidden-marker matches, and clean teardown.54cf259d: run 28564960504 exposed the stale Dockerfile validator digest and was canceled before runtime proof. Candidate970803a4updates the integrity pin, retained by final headc5a67c4c.54cf259d: run 28564973806 was canceled during pre-cleanup and supplies no acceptance evidence.54cf259dplus child69f4e1b2: run 28564983760 was canceled during pre-cleanup and supplies no acceptance evidence.970803a4: run 28565197328 was canceled before acceptance execution when the canonical placeholder-format advisor fix advanced the head.970803a4: run 28565207911 was canceled before runner assignment and supplies no acceptance evidence.970803a4plus childb4d5679e: run 28565222881 was canceled before runner assignment and supplies no acceptance evidence.7335903b: run 28565576066 was intentionally canceled when the documentation gap advanced the candidate; the root-entrypoint smoke passed, but the remaining lanes provide no complete acceptance proof.7335903b: run 28565587094 was canceled before acceptance execution and supplies no acceptance evidence.7335903bplus child091e16fd: run 28565603460 was canceled before acceptance execution and supplies no acceptance evidence.c5a67c4c: run 28566083673 reached the native GPU/runtime proofs before the obsolete raw strict-hash assertion failed; messaging independently hit the process-probe self-match fixed by merged test(e2e): prevent process-token probe self-matches #6167. GPU, root-entrypoint, secret-boundary, and credential companion lanes passed.c5a67c4c: run 28566083641 proved native routing, Ready/CUDA,nvidia-smi,/proc,cuInit(0)=0, PID 1, authenticated inference, and cleanup, then failed only the obsolete raw strict-hash assertion.c5a67c4cplus child48c46a7f: run 28566083589 proved the same runtime boundary on the legacy route, then failed only the obsolete raw strict-hash assertion.a04a70ac: run 28568069499 exposed a pre-onboarding harness defect: direct Vitest import of the production cleanup helper could not resolve its lazy CommonJS TypeScript dependencies. Companion results do not count as final-head evidence.a04a70ac: run 28568069558 failed at the same pre-onboarding cleanup boundary and supplies no runtime acceptance evidence.a04a70acplus child085a3b7d: run 28568069530 failed at the same pre-onboarding cleanup boundary and supplies no runtime acceptance evidence.6ac4ebc8: run 28568490864 exposed a clean-runner preinstall edge: the compiled cleanup child was invoked before OpenShell existed and failed before onboarding. Companion results do not count as final-head evidence.6ac4ebc8: run 28568494928 failed at the same pre-onboarding cleanup boundary and supplies no runtime acceptance evidence.6ac4ebc8plus child5d3742a7: run 28568501430 failed at the same pre-onboarding cleanup boundary and supplies no runtime acceptance evidence.65b06d64: run 28568954862 passed all six jobs, but the candidate advanced to close the advisor-confirmed shared diagnostic-redaction boundary and two proof-hardening review threads.65b06d64: run 28568959028 passed the full native runtime proof but is not final-head evidence.65b06d64plus child2c6dca1b: run 28568966557 passed but is not final-parent evidence.d76f1647: run 28601346031 passed all six requested jobs. Native GPU, Hermes startup, root-entrypoint, secret-boundary, credential-sanitization, and messaging proofs are green; all 21 messaging raw-token surface probes areABSENT, and every cleanup record has zero failures.d76f1647: run 28601348023 passed 9/9 assertions. Artifact8043702467(sha256:0ef929fa2478f5c9579ad2f282c46de8fe4d6eefb04acb062de1af29b4eb002c) proves native routing, Ready/CUDA,nvidia-smi,/proc, successfulcuInit(0), OpenShell PID 1, one container/no backup, authenticated inference with zero forbidden-marker matches, and clean teardown.d76f1647from signed workflow-only child4c49b5bc: run 28602166456 passed. Artifact8044114375(sha256:b5cff9cc7e7cf361f55e074a265c8cfefd3e6dc21ad0d300b140d2a749cde00b) records clone exit 137 withfailure_kind=patched_container_failedandrolled_back=nobefore finalize, thenrolled_back=yes, exactly one running original container, no backup leak, guard-observed clone removal, clean canary scans, and clean fixture teardown.d76f1647plus signed workflow-only child078a372d: run 28603335692 passed 9/9 assertions. Artifact8044550700(sha256:56d97c5caa8536ebff735ff600399ac7fafa2fed71206de1f5470e7d15549f6f) provesgpuRoute=legacy-patch,--device nvidia.com/gpu=all, Ready/CUDA with all three GPU probes, correct OpenShell PID 1/command envelope, one container/no backup, integrity and negative guard checks, two authenticated inference requests with zero forbidden-marker matches, a clean artifact canary scan, and clean teardown.Source-of-truth review for the retained compatibility path:
Error.OPENSHELL_SANDBOX_COMMAND=env ...envelope; the live test pins native and legacy runtime topology separately.NEMOCLAW_DOCKER_GPU_PATCH=0; routing tests lock that behavior. Remove it when Docker Desktop exposes usablenvidia.com/gpuCDI devices to the WSL distro./dev/nvmapand/dev/nvhost-*device ownership requires host group propagation for the non-root sandbox user; group-add tests lock that behavior. Remove it only when the platform/runtime supplies equivalent access without the compatibility recreate.Diagnostic redaction boundary:
collectDockerGpuPatchDiagnostics()constructs the trusted per-bundle redactor, discovers conventional and custom-placeholder values from every known/discovered full inspect before writing, recursively redacts JSON values, and publishes every summary, Docker, OpenShell, and pre-rollback top artifact through that boundary.Advisor architecture and follow-up rationale:
docker-gpu-patch.tsgrows 58 lines to keep token validation before container mutation and bounded failed-clone capture before rollback. Splitting this security-critical ordering during the release-blocker fix would add cross-module state transfer; extract it when the legacy swap is retired after WSL and Jetson native proof.docker-gpu-local-inference.test.tsgrows 32 lines so bridge-probe routing assertions stay beside the behavior under test. Extract a bridge-probe module and focused test file if that surface grows again.docker-gpu-local-inference.tsgrows 15 lines to keep the bridge-probe/host-network decision beside its caller-facing contract; extract it with the tests if that surface grows again.docker-gpu-patch.test.tsgrows 13 lines and remains below 1,350 lines; split the mode-routing cases on the next growth.OPENSHELL_TLS_KEYtests prove exact runtime acceptance, arbitrary/PEM/relative/near-miss rejection, persisted.envrejection, and continued rejection of supervisor identity tokens. The exact allowed path is sourced fromNVIDIA/OpenShell@v0.0.71(a242f84bb367d6df7d4d133e95a93857406c67f7), wheredriver_utils.rs::TLS_KEY_MOUNT_PATHdefines/etc/openshell/tls/client/tls.keyand the Docker/Podman drivers inject it.This PR does not claim #6110 resolved until the reporter-class DGX Spark aarch64 or DGX Station GB300 NVIDIA Endpoints path passes. No such runner is declared in this repository, and organization runner inventory is not visible with the current permissions. Missing reporter hardware is an external acceptance blocker, not a passing result.
The #6155 docs regression fix and current
mainthrough9fe45362are integrated. A refreshed pairwise merge-tree audit atd76f1647is clean with #5595 and #6153. #5876 directly conflicts ine2e.yamland related Hermes/docs/workflow-boundary files; its resolution must unionhermes-gpu-startupandmcp-bridge-devselectors/result summaries and recompute uploader validation. #6020 already conflicts with currentmainand also overlaps #6142 outsidee2e.yaml; #6053 is mergeable withmainbut conflicts pairwise in the uploader boundary. Those later branches must preserve #6142's explicit-only inventory and artifact contracts during retargeting; #6142 itself remains mergeable/CLEAN.Signed-off-by: Aaron Erickson aerickson@nvidia.com
Summary by CodeRabbit
NEMOCLAW_DOCKER_GPU_PATCHguidance across native Linux, Docker Desktop WSL, and Jetson/Tegra.