fix(shields): accept private mutable Hermes root - #7033
Conversation
Signed-off-by: Prekshi Vyas <prekshiv@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:
📝 WalkthroughWalkthroughHermes config unlock verification now detects workload topology before accepting mode ChangesHermes topology-aware unlock
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant UnlockVerification
participant HermesSandbox
participant HermesConfigDirectory
UnlockVerification->>HermesSandbox: inspect lifecycle marker
HermesSandbox-->>UnlockVerification: return Hermes topology
UnlockVerification->>HermesConfigDirectory: inspect directory mode
HermesConfigDirectory-->>UnlockVerification: return mode
UnlockVerification->>UnlockVerification: validate topology and mode
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
PR Review Advisor — InformationalAdvisor assessment: Informational / high confidence Model lanes
Nemotron output stays in workflow artifacts and does not change the assessment above. E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage remains at 96%, unchanged from the Updated |
apurvvkumaria
added
bug-fix
Co-authored-by: Prekshi Vyas <prekshiv@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
Security/correctness review at exact head 291aa0d confirms PRA-1. The exception is unconditional for every Hermes topology, but the retained root-entrypoint topology launches Hermes as the separate gateway UID in the sandbox group (agents/hermes/start.sh) and relies on 03770 traversal/write access. Treating 0700 sandbox:sandbox as a completed unlock can therefore report success while that gateway cannot traverse the config root. The new live test explicitly covers only managed non-root/same-UID startup, so it does not close this case. Please either bind the 0700 allowance to an attested same-UID/non-root topology, or restore/verify 03770 before finish where a separate gateway UID exists, and add a regression that rejects or repairs 0700 in the root-separated topology. Categories 1–6 PASS; configuration/holistic posture FAIL for the cross-topology exception; security testing WARNING until that negative topology is covered. I am withholding trusted E2E authorization until this blocker is fixed.
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/shields/legacy-hermes-compat.test.ts`:
- Around line 151-152: Replace the command mock branch keyed by
HERMES_ROOT_LIFECYCLE_MARKER with focused tests that exercise the real
lifecycle-marker probe predicate and topology mapping. Cover missing, existing,
and symlink marker paths, and add negative security-boundary cases proving
bypass attempts are rejected rather than classified as hermesTopology.
🪄 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: b486a2df-332f-4758-8219-616de010f750
📒 Files selected for processing (2)
src/lib/shields/index.tssrc/lib/shields/legacy-hermes-compat.test.ts
|
Re-audit at exact head |
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
…ate-shields-root Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
|
🌿 Preview your docs: https://nvidia-preview-pr-7033.docs.buildwithfern.com/nemoclaw |
|
Superseded by #7049. The replacement preserves all seven original commits from this PR in its ancestry and credits Prekshi Vyas as co-author of the maintainer remediation commit. Closing this PR without merge so the descriptor-safe marker and topology attestation can proceed through the normal gate. |
<!-- markdownlint-disable MD041 --> ## Summary Supersedes #7033 with an append-only maintainer follow-up that preserves every original commit by Prekshi Vyas. It keeps the intended private Hermes root support while requiring descriptor-safe lifecycle-marker and process-topology attestation before accepting or repairing that state. ## Related Issue Refs #6381 ## Changes - Preserve all seven commits from #7033 and their original authorship. - Authenticate the root-separated lifecycle marker by pinned descriptor, exact ownership, mode, link count, and content. - Attest the pinned process topology before distinguishing root-separated and same-UID runtimes. - Re-sample lifecycle and startup-ready state after the procfs proof and fail closed if topology changes during attestation. - Restrict accepted same-UID modes and repair root-separated mode inside the sealed transaction. - Fail closed on malformed, symlinked, hard-linked, changed, or raced marker and topology state while retaining rollback. - Exercise real root-owned repair and exact rollback in a pinned, network-isolated Linux container independent of the host UID. - Assert the rollback protocol order: prepare abort, restore the locked state directory, then commit abort. - Credit Prekshi Vyas as co-author of both maintainer remediation commits. ## 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: an independent documentation audit confirmed this tightens an internal sandbox guard without changing supported commands, configuration, or operator 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: independent exact-head maintainer review covered topology transitions, filesystem authority, rollback order, test isolation, and the automated findings; the remaining analyzer comments were verified as duplicate false positives or inapplicable to the explicitly same-UID E2E fixture. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Verification - [x] PR description includes the DCO sign-off declaration and every pushed commit appears as `Verified` in GitHub - [x] Normal pre-commit, commit-msg, and pre-push hooks passed - [x] 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) - [ ] 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) Targeted evidence: the topology suite passed (3 host tests; 2 platform-gated), the Docker-backed real-root rollback case passed independently, the legacy shields suite passed (14/14), Python compilation and CLI typecheck passed, and changed-file hooks plus title, size, source-shape, Biome, Docker-abstraction, and diff checks passed. Fresh exact-head CI and trusted E2E are pending. --- Signed-off-by: Apurv Kumaria <akumaria@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Security & Reliability** * Strengthened Hermes runtime topology attestation using a root lifecycle marker. * Added stricter fail-closed behavior for missing/unsafe marker states and unknown topologies. * Reconciles/repairs mutable private Hermes directory metadata (mode/ownership) before finalization. * Added durability checks by introducing a directory metadata fsync barrier when required. * **Bug Fixes** * Relaxed non-final Hermes config-directory mode matching (while keeping owner validation strict), with clearer expected-mode failures during in-progress transactions. * **Tests** * Added new Hermes runtime-config-guard topology and reconciliation test coverage (local + Docker), including race/drift and transition recovery scenarios. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com> Co-authored-by: Prekshi Vyas <prekshiv@nvidia.com>
4 participants
Summary
Hermes can tighten its mutable home from mode 03770 to the private mode 0700 after the dashboard starts. Shields now accepts either sandbox-owned mutable posture instead of rolling back a successful second unlock, while retaining exact ownership and protected-file checks.
Related Issue
Refs #6381
Changes
Type of Change
Quality Gates
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailablenpx vitest run --project cli src/lib/shields/legacy-hermes-compat.test.ts(12 passed);npm run test:changed(32 files, 310 tests passed);npm run typecheck:clipassed.npm 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: Prekshi Vyas prekshiv@nvidia.com
Summary by CodeRabbit
700directory permissions, now specifically allowing it only in the managed non-root topology.700or3770for sandbox permissions via a more flexible match.