fix(shields): support nested OpenShell PID namespaces - #6568
Conversation
Signed-off-by: Chengjie Wang <chengjiew@nvidia.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe guard now parses ChangesNested Namespace Topology Matching
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant openclaw_config_guard as openclaw-config-guard.py
participant procfs as /proc
participant readiness as readiness predicate
openclaw_config_guard->>procfs: read cmdline and namespace state
procfs-->>openclaw_config_guard: NUL-separated args and PID topology
openclaw_config_guard->>readiness: require cmdline matches and topology_matches
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/startup-process-identity.test.ts (1)
103-109: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winValidate the namespace selector in the harness.
The optional tuple tail silently falls back to
namespace_pathfor any value other than exactly["nested"]. Tighten this so typoed or accidentally omitted nested selectors fail loudly instead of weakening these regression scenarios.As per path instructions, “Migration tests must prove the superseded path is unreachable or removed, not merely prove that the new path also works.”
Proposed harness hardening
- pid, start_time, cmdline, effective_uid, inner_pid, parent_pid, *namespace = process + pid, start_time, cmdline, effective_uid, inner_pid, parent_pid, *namespace = process + if namespace not in ([], ["nested"]): + raise AssertionError(f"unsupported namespace selector: {namespace!r}") write_process( proc_root, pid, start_time, cmdline, - nested_namespace_path if namespace == ["nested"] else namespace_path, + nested_namespace_path if namespace == ["nested"] else namespace_path,Also applies to: 179-184
🤖 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/startup-process-identity.test.ts` around lines 103 - 109, The test harness in the process fixture currently treats any namespace tail other than exactly ["nested"] as a fallback to namespace_path, which can mask typos or missing selectors. Tighten the selector logic used in the process loop and write_process call so only the expected nested namespace value is accepted and any other non-empty or unexpected tail causes the test to fail loudly, keeping the regression scenarios in startup-process-identity.test.ts strict.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 `@test/startup-process-identity.test.ts`:
- Around line 103-109: The test harness in the process fixture currently treats
any namespace tail other than exactly ["nested"] as a fallback to
namespace_path, which can mask typos or missing selectors. Tighten the selector
logic used in the process loop and write_process call so only the expected
nested namespace value is accepted and any other non-empty or unexpected tail
causes the test to fail loudly, keeping the regression scenarios in
startup-process-identity.test.ts strict.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ca3dda7c-d907-4a71-b477-6bd690415f81
📒 Files selected for processing (2)
scripts/openclaw-config-guard.pytest/startup-process-identity.test.ts
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.
|
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage remains at 96%, unchanged from the TypeScript / code-coverage/cliThe overall coverage in the Show a code coverage summary of the most impacted files.
Updated |
E2E Advisor RecommendationRequired E2E: Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
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
|
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. |
Signed-off-by: Chengjie Wang <chengjiew@nvidia.com>
E2E Target Results — ✅ All selected jobs passedRun: 29032896048
|
E2E Target Results — ❌ Some jobs failedRun: 29032896112
|
Accept only canonical direct or bash nemoclaw-start launch shapes. Require both procfs command-line reads to remain identical. Cover same-namespace and nested-PID extra-argument spoof attempts. Co-authored-by: Chengjie Wang <chengjiew@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
E2E Target Results — ❌ Some jobs failedRun: 29032896112
|
E2E Target Results — ✅ All selected jobs passedRun: 29033682489
|
E2E Target Results — ❌ Some jobs failedRun: 29033682473
|
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
|
Maintainer follow-up on exact head
The first strict two-argument version was caught by exact-head Local verification is green: focused process-identity and Hermes guard suites pass 26/26, Python compilation succeeds, and The security review verdict remains warning, no demonstrated code-blocking vulnerability under the supported OpenShell v0.0.72 threat model. The nested path relies on kernel-owned Fresh exact-head runtime validation is running in: Approval remains gated on those runs, settled current-head CI/advisors, and the normal merge gate. The final documentation review found no durable docs change: standard Docker ENTRYPOINT/CMD composition and the existing command, troubleshooting, runtime-control, and security guidance remain accurate. This compatibility/security fix belongs in the v0.0.79 release notes. |
E2E Target Results — ❌ Some jobs failedRun: 29034109789
|
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
E2E Target Results — ❌ Some jobs failedRun: 29033682473
|
E2E Target Results — ✅ All selected jobs passedRun: 29034314857
|
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
E2E Target Results —
|
| Job | Result |
|---|---|
| cloud-onboard | |
| shields-config |
E2E Target Results — ✅ All requested jobs passedRun: 29034503546
|
E2E Target Results — ✅ All selected jobs passedRun: 29034503592
|
E2E Target Results —
|
| Job | Result |
|---|---|
| live |
E2E Target Results —
|
| Job | Result |
|---|---|
| hermes-e2e | |
| hermes-gpu-startup | |
| hermes-shields-config | |
| shields-config |
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
|
Exact-head maintainer disposition for All ordinary CI is green, including the production-image non-root smoke that caught the earlier Docker Required runtime evidence is green:
I audited the uploaded artifacts rather than relying on job conclusions alone. OpenClaw shields reported all 10 config/readiness/drift/audit assertions true. Hermes shields reported config preservation, the fresh non-root trigger, and both down/up cycles true. OpenClaw and Hermes security-posture artifacts both reported Disposition of the secondary Nemotron findings:
No further source change is warranted from those secondary findings. Approval remains subject to the final deterministic post-report merge gate; this comment does not merge the PR. |
E2E Target Results — ✅ All requested jobs passedRun: 29035702418
|
cv
left a comment
There was a problem hiding this comment.
Approved on exact head e69f149a8ff622a8e077400e34eee67f4cea46d3 after the Docker CMD compatibility regression was caught and fixed. All ordinary CI, the primary merge_as_is advisor, focused 26/26 identity/guard tests, audited OpenClaw and Hermes shields/security-posture runs, and audited Hermes image secret-boundary/root-entrypoint smokes are green. Secondary advisor dispositions and the accepted scoped threat-model rationale are recorded in the exact-head maintainer comment.
E2E Target Results — ✅ All selected jobs passedRun: 29035917175
|
E2E Target Results — ✅ All requested jobs passedRun: 29035917181
|
<!-- markdownlint-disable MD041 --> ## Summary Adds the pre-tag v0.0.79 release notes entry to `docs/about/release-notes.mdx` so the release plan can be generated after docs merge. The entry summarizes the merged v0.0.79 release train across inference, diagnostics, runtime hardening, policies, onboarding recovery, and release validation. ## Changes - Added the v0.0.79 release notes section with linked follow-up documentation for OpenRouter onboarding, managed vLLM changes, completion and logging, Deep Agents runtime limits, policy updates, onboarding recovery, and release validation. - Source summary: - #6461 -> `docs/about/release-notes.mdx`: Documents OpenRouter onboarding support and links to inference/provider references. - #6271 and #6272 -> `docs/about/release-notes.mdx`: Documents shell completion and structured logging highlights. - #6465, #6539, #6570, and #6528 -> `docs/about/release-notes.mdx`: Documents status route-drift, orphaned sandbox, gateway cleanup, and DGX Spark express-install diagnostics. - #6523, #6551, #6484, #6488, #6324, and #6542 -> `docs/about/release-notes.mdx`: Documents managed vLLM, Qwen3.6 tool parser, compaction, and timeout/readiness improvements. - #6559, #6538, #6560, #6568, #6552, #6567, and #6587 -> `docs/about/release-notes.mdx`: Documents runtime, credential, proxy, PID namespace, TOML, and provider-state hardening. - #6541, #5415, #6246, #6496, and #6573 -> `docs/about/release-notes.mdx`: Documents GitHub policy, Gmail policy, MCP allowlist, WhatsApp, and messaging-variant updates. - #6253, #6572, #6444, #6536, and #5860 -> `docs/about/release-notes.mdx`: Documents onboarding resume and create-step recovery improvements. - #6508, #6527, #5506, #6588, #6446, #6447, #6582, #6296, #6367, #6397, and #6505 -> `docs/about/release-notes.mdx`: Documents docs, release-risk, and E2E validation updates. ## 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 exactly one tests line and one docs line. Check other lines when applicable. Add every requested justification or approval reference. --> - [ ] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [x] Tests not applicable — justification: Release-note prose only. - [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 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 - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — command/result or justification: Tests not applicable, release-note prose only. - [ ] 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) - [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) Docs validation note: `npm run docs:check-agent-variants && npm run docs:check-routes && git diff --check` passed. Full `npm run docs` is currently blocked before Fern validation because the pinned `fern-api@5.65.2` package is unavailable from npm (`ETARGET No matching version found`). --- <!-- 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 * **Documentation** * Added release notes for v0.0.79 with a new summary of recent improvements, including onboarding and inference options, operator/CLI diagnostics, sandbox recovery hardening, runtime limits, network policy behavior, and release validation updates. * Added updated references and links for the latest release. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
<!-- markdownlint-disable MD041 --> ## Summary Fix OpenClaw shields config-guard startup detection when `openshell-sandbox` is PID 1 and `nemoclaw-start` is PID 1 inside OpenShell's nested workload PID namespace. The guard now recognizes that authenticated topology without weakening the existing process identity checks. ## Related Issue Fixes NVIDIA#6565 ## Changes - Accept a stable nested workload PID namespace when the pinned direct child reports `NSpid` 1, including when Landlock hides namespace inode links. - Preserve the existing UID, exact command line, direct-parent, double-read, and pinned-proc identity checks. - Add OpenClaw regression coverage for readable and Landlock-restricted namespace evidence while retaining Hermes and spoofing negative controls. ## 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: Restores the intended OpenShell runtime topology without changing commands, configuration, or documented workflow. - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: Focused security review confirmed the new branch retains pinned proc identity, two stable reads, PPID 1, effective UID, exact command line, and duplicate-process rejection; only the authenticated nested PID namespace shape is added. - [ ] 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 - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — command/result or justification: 46 integration tests and 54 shields CLI tests passed. - [ ] 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: Chengjie Wang <chengjiew@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved supervised startup process identity detection across same-namespace and nested PID-namespace scenarios, including more reliable command-line matching and additional validation to confirm correct runtime topology. * Updated nested PID-namespace access-denial behavior to align with actual observed results. * **Tests** * Expanded startup-process identity coverage for nested PID namespaces, including added supervised entrypoint and argv-spoof scenarios. * Updated parameterized expectations and added new OpenShell nested variants (including cross-namespace and landlock denial cases). <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Chengjie Wang <chengjiew@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com> Signed-off-by: Carlos Villela <cvillela@nvidia.com> Co-authored-by: Apurv Kumaria <akumaria@nvidia.com> Co-authored-by: Carlos Villela <cvillela@nvidia.com>
<!-- markdownlint-disable MD041 --> ## Summary Adds the pre-tag v0.0.79 release notes entry to `docs/about/release-notes.mdx` so the release plan can be generated after docs merge. The entry summarizes the merged v0.0.79 release train across inference, diagnostics, runtime hardening, policies, onboarding recovery, and release validation. ## Changes - Added the v0.0.79 release notes section with linked follow-up documentation for OpenRouter onboarding, managed vLLM changes, completion and logging, Deep Agents runtime limits, policy updates, onboarding recovery, and release validation. - Source summary: - NVIDIA#6461 -> `docs/about/release-notes.mdx`: Documents OpenRouter onboarding support and links to inference/provider references. - NVIDIA#6271 and NVIDIA#6272 -> `docs/about/release-notes.mdx`: Documents shell completion and structured logging highlights. - NVIDIA#6465, NVIDIA#6539, NVIDIA#6570, and NVIDIA#6528 -> `docs/about/release-notes.mdx`: Documents status route-drift, orphaned sandbox, gateway cleanup, and DGX Spark express-install diagnostics. - NVIDIA#6523, NVIDIA#6551, NVIDIA#6484, NVIDIA#6488, NVIDIA#6324, and NVIDIA#6542 -> `docs/about/release-notes.mdx`: Documents managed vLLM, Qwen3.6 tool parser, compaction, and timeout/readiness improvements. - NVIDIA#6559, NVIDIA#6538, NVIDIA#6560, NVIDIA#6568, NVIDIA#6552, NVIDIA#6567, and NVIDIA#6587 -> `docs/about/release-notes.mdx`: Documents runtime, credential, proxy, PID namespace, TOML, and provider-state hardening. - NVIDIA#6541, NVIDIA#5415, NVIDIA#6246, NVIDIA#6496, and NVIDIA#6573 -> `docs/about/release-notes.mdx`: Documents GitHub policy, Gmail policy, MCP allowlist, WhatsApp, and messaging-variant updates. - NVIDIA#6253, NVIDIA#6572, NVIDIA#6444, NVIDIA#6536, and NVIDIA#5860 -> `docs/about/release-notes.mdx`: Documents onboarding resume and create-step recovery improvements. - NVIDIA#6508, NVIDIA#6527, NVIDIA#5506, NVIDIA#6588, NVIDIA#6446, NVIDIA#6447, NVIDIA#6582, NVIDIA#6296, NVIDIA#6367, NVIDIA#6397, and NVIDIA#6505 -> `docs/about/release-notes.mdx`: Documents docs, release-risk, and E2E validation updates. ## 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 exactly one tests line and one docs line. Check other lines when applicable. Add every requested justification or approval reference. --> - [ ] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [x] Tests not applicable — justification: Release-note prose only. - [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 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 - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — command/result or justification: Tests not applicable, release-note prose only. - [ ] 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) - [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) Docs validation note: `npm run docs:check-agent-variants && npm run docs:check-routes && git diff --check` passed. Full `npm run docs` is currently blocked before Fern validation because the pinned `fern-api@5.65.2` package is unavailable from npm (`ETARGET No matching version found`). --- <!-- 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 * **Documentation** * Added release notes for v0.0.79 with a new summary of recent improvements, including onboarding and inference options, operator/CLI diagnostics, sandbox recovery hardening, runtime limits, network policy behavior, and release validation updates. * Added updated references and links for the latest release. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Summary
Fix OpenClaw shields config-guard startup detection when
openshell-sandboxis PID 1 andnemoclaw-startis PID 1 inside OpenShell's nested workload PID namespace. The guard now recognizes that authenticated topology without weakening the existing process identity checks.Related Issue
Fixes #6565
Changes
NSpid1, including when Landlock hides namespace inode links.Type of Change
Quality Gates
Verification
Verifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailablenpm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result:npm run docsbuilds without warnings (doc changes only)Signed-off-by: Chengjie Wang chengjiew@nvidia.com
Summary by CodeRabbit
Bug Fixes
Tests