fix(shields): keep UP when Hermes config path is unsafe (#8804)

[aarav1109s]

Summary

shields down could reject an unsafe Hermes config symlink and still leave Shields DOWN with a timer. It now refuses unsafe config paths before weakening policy, and if unlock fails after a provisional DOWN record, it restores the restrictive policy and clears that provisional state so status stays UP.

Related Issue

Fixes #8804

Changes

• Add a preflight path-safety check for Hermes config paths before shields down starts the timer, writes provisional DOWN, or applies permissive policy. Keep the config directory fd open and open protected files relative to it.
• Clear provisional DOWN on an unsafe-path re-lock failure only when unlock never completed. After a successful unlock, require verified re-lock or report manual intervention.
• Document the refusal and rollback behavior in docs/reference/commands.mdx.
• Cover preflight, unlock-fail, and unlock-then-unsafe re-lock paths with a shared test helper that keeps the changed test file if-count flat.

Type of Change

• 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

• Tests added or updated for changed behavior
• Existing tests cover changed behavior — justification:
• Tests not applicable — justification:
• 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: fix(shields): keep UP when Hermes config path is unsafe (#8804) #8830 (comment)
• Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue:

Documentation Writer Review

• Documentation writer subagent reviewed the completed changes
• Result: docs-updated
• Evidence: docs/reference/commands.mdx
• Agent: Cursor

DGX Station Hardware Evidence

• Tested on DGX Station
• Tested commit:
• Station profile/scenario:
• Result:
• Supporting evidence:

Verification

• PR description includes a Signed-off-by: line and every commit appears as Verified in GitHub
• Normal pre-commit, commit-msg, and pre-push hooks passed, or npm run validate:pr passed after refreshing origin/main 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: npx vitest run --project cli src/lib/shields/openclaw-transition.test.ts -t '8804' → 3 passed; npx vitest run --project cli src/lib/shields/policy-transition.test.ts -t '8198' → 3 passed; npm run test-size:check and npm run checks:repository passed
• Applicable broad gate passed — npm test for broad runtime/test-harness changes; npm run check for repo-wide validation/coverage changes — command/result:
• Quality Gates section completed with required justifications or waivers
• No secrets, API keys, or credentials committed
• npm run docs builds without warnings (doc changes only)
• Doc pages follow the style guide (doc changes only)
• New doc pages include SPDX header and frontmatter (new pages only)

Change size

This is a large review surface: 714 additions and 4 deletions across six files. Most of the increase is the typed adversarial test harness and regression coverage.

---

Signed-off-by: Aarav Sharma aarav1109s@gmail.com

Summary by CodeRabbit

• New Features

  • Added safeguards when turning Shields down to reject unsafe configuration paths, including symlinks and invalid files or directories.
  • Shields remains up when validation fails, preventing policy weakening and temporary state changes.

• Bug Fixes

  • Improved recovery after failed transitions by restoring restrictive policies and clearing temporary state.
  • Retains Shields down and requires manual intervention if an unsafe path cannot be re-locked after unlocking.

• Documentation

  • Documented the additional shields down safeguards and failure behavior.

[copy-pr-bot]

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

[coderabbitai]

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

shields down now validates Hermes configuration paths before changing policy or state. Unsafe paths are rejected, and failed transitions restore or retain the correct Shields state. Tests and command documentation cover these cases.

Changes

Hermes Shields safety

Layer / File(s)	Summary
Config-path preflight src/lib/shields/index.ts	The command checks the Hermes config directory and protected files with no-follow opens. It rejects unsafe paths before policy, state, timer, or permission mutations.
Unsafe-path rollback src/lib/shields/index.ts	Rollback tracks timer authority and config restoration. It retains recovery metadata when re-lock verification or timer revocation remains incomplete.
Transition validation and documentation test/helpers/hermes-unsafe-config-shields-harness.ts, src/lib/shields/openclaw-transition.test.ts, docs/reference/commands.mdx	The harness simulates unsafe-path failures. Tests cover preflight rejection and rollback outcomes. The command reference documents fail-closed behavior.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant ShieldsDown
  participant ConfigPathPreflight
  participant SandboxFilesystem
  participant ShieldState
  ShieldsDown->>ConfigPathPreflight: Validate Hermes configuration paths
  ConfigPathPreflight->>SandboxFilesystem: Open protected paths without following symlinks
  SandboxFilesystem-->>ConfigPathPreflight: Return path safety result
  ConfigPathPreflight-->>ShieldsDown: Return validation result
  ShieldsDown->>ShieldState: Apply or retain transition state

Loading

Possibly related PRs

• NVIDIA/NemoClaw#8658 — Modifies Hermes Shields transition handling in src/lib/shields/index.ts.
• NVIDIA/NemoClaw#8668 — Adds related no-follow symlink protections for Hermes filesystem paths.
• NVIDIA/NemoClaw#8845 — Validates Hermes configuration writes and lifecycle behavior.

Suggested reviewers: cv, ericksoa

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The changes address #8804 by rejecting unsafe paths before mutation and preserving UP state through rollback failures.
Out of Scope Changes check	✅ Passed	The documentation, tests, and harness changes directly support the unsafe Hermes config-path safeguards.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly identifies the Shields behavior fix for unsafe Hermes config paths and matches the main change.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

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/index.ts`:
- Around line 835-849: Update the config validation flow around open_nofollow to
keep the trusted config_dir descriptor open through protected-file checks, and
open each file using descriptor-relative no-follow traversal rather than
absolute paths. Close the directory descriptor only after all checks complete,
preserving existing rejection of non-directories and non-regular files; add
coverage for replacing config_dir with a symlink while its final file remains
regular.

🪄 Autofix

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: 2a8037ca-aa4e-4224-bc96-45a69fc91776

📥 Commits

Reviewing files that changed from the base of the PR and between 9ac22da and 4cef3a0.

📒 Files selected for processing (3)

• docs/reference/commands.mdx
• src/lib/shields/index.ts
• src/lib/shields/openclaw-transition.test.ts

[github-actions]

PR Review Advisor — No blocking findings reported

Advisor assessment: No blocking advisor findings reported
Next action: No advisor follow-up needed.
Findings: 0 blockers · 0 warnings · 0 suggestions

Model lanes

• GPT-5.6 Terra (primary): Completed · medium confidence · 0 blockers · 0 warnings · 0 suggestions
• Nemotron 3 Ultra (second opinion): Completed · high confidence · 1 blocker · 3 warnings · 0 suggestions
• Model comparison: normalized findings differ; normalized terminology decisions differ; normalized E2E selections differ; Nemotron reported 1 more blocker, 3 more warnings, the same number of suggestions.

1 additional E2E selection from the second opinion

Advisory only. The primary lane did not select these E2E jobs or targets.

• shields-config: The completed second-opinion lane identified E2E coverage that the primary lane omitted.

Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests.

3 semantic terminology decisions

Terminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.

• established — unsafe config path at src/lib/shields/index.ts:803: Use `unsafe config path` for this path-safety failure class.
• established — provisional Shields down record at docs/reference/commands.mdx:1388: Use `provisional Shields down record` for the incomplete persisted transition state.
• justified — config protection at docs/reference/commands.mdx:1390: Keep `config protection` where the text requires verified filesystem protection rather than recorded posture.

E2E guidance

Advisory only. A maintainer can dispatch the default E2E suite for the commit under review.

Recommended E2E: None

Manual-only E2E: cloud-inference, security-posture
The manual PR workflow does not run these selectors for the commit under review. Run them from reviewed code on main.

1 optional E2E recommendation

• hermes-shields-config

Workflow run details

This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge.

[cv]

test/helpers/hermes-unsafe-config-shields-harness.ts:99-100 still injects the expected symlink refusal instead of executing the Python no-follow preflight added in src/lib/shields/index.ts. This bypasses the component that enforces the new filesystem security boundary. A regression in descriptor-relative traversal, O_NOFOLLOW, or file-type validation could pass src/lib/shields/openclaw-transition.test.ts:789 and let shields down weaken policy for an unsafe path. Add a filesystem-backed regression test that executes the embedded preflight against a real Hermes config symlink and a replaced config-directory symlink. Verify that policy, timer, audit, and Shields state remain unchanged.

[coderabbitai]

Actionable comments posted: 4

🧹 Nitpick comments (1)

test/helpers/hermes-unsafe-config-shields-harness.ts (1)

309-316: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Reuse the existing convergence spy.

beforeEachHook already spies on waitForHermesInferenceRouteConvergence. Update that spy with mockReturnValue instead of calling vi.spyOn again.

🤖 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/helpers/hermes-unsafe-config-shields-harness.ts` around lines 309 - 316,
Update failHermesInferenceConvergence to reuse the
waitForHermesInferenceRouteConvergence spy created by beforeEachHook, calling
mockReturnValue on that existing spy instead of invoking vi.spyOn again.
Preserve the current failed convergence response values.

🤖 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/openclaw-transition.test.ts`:
- Around line 818-823: Update the rollback test around
expectHermesShieldsUpRecord to also assert getShieldsPosture("hermes-shields",
false), matching the preflight test, and verify that Hermes is restored to a
locked, immutable posture while retaining the existing state-record and cleanup
assertions.

In `@test/helpers/hermes-unsafe-config-shields-harness.ts`:
- Around line 56-64: Update isPathPreflight and the related preflight handling
in the harness so detection uses a shared production marker or an explicit
assertion of the expected marker, rather than silently falling through when the
marker changes. Ensure marker mismatches fail the relevant scenarios, including
unlock-symlink and unlock-ok-relock-symlink, instead of allowing prior(cmd) to
return a default successful result.
- Around line 173-175: Update the cache cleanup in the harness teardown hooks to
use requireSource.cache with requireSource.resolve(...) instead of bare
require.cache, including the cleanup around indexModule and the corresponding
location near line 291. Ensure afterEachHook also removes timer-bound-lock.js
and transition-lock.js so all harness modules are cleared after each test.
- Around line 242-271: Update the vi.spyOn(childProcess, "fork") mock to satisfy
the ChildProcess return type by either adding the required members such as
stdin, stdout, stderr, and stdio to the fake or explicitly casting the
intentional test double to ChildProcess. Preserve the existing pid, lifecycle
methods, and send authorization behavior.

---

Nitpick comments:
In `@test/helpers/hermes-unsafe-config-shields-harness.ts`:
- Around line 309-316: Update failHermesInferenceConvergence to reuse the
waitForHermesInferenceRouteConvergence spy created by beforeEachHook, calling
mockReturnValue on that existing spy instead of invoking vi.spyOn again.
Preserve the current failed convergence response values.

🪄 Autofix

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: 63070be8-b83e-4ecc-905f-9dd57b93c120

📥 Commits

Reviewing files that changed from the base of the PR and between 4cef3a0 and bb11c95.

📒 Files selected for processing (4)

• docs/reference/commands.mdx
• src/lib/shields/index.ts
• src/lib/shields/openclaw-transition.test.ts
• test/helpers/hermes-unsafe-config-shields-harness.ts

🚧 Files skipped from review as they are similar to previous changes (2)

• docs/reference/commands.mdx
• src/lib/shields/index.ts

[aarav1109s]

Good catch, Addressed the requested changes in commit 5b37e7e1a.

• The harness now executes the embedded Python preflight against a real config-file symlink and a replaced config-directory symlink. Both cases verify that policy, timer, audit, Shields state, and locked posture remain unchanged.
• Preflight error handling now reads stderr before the command text, so it reports the actual Python refusal.
• The rollback posture assertion, preflight marker assertion, module-cache cleanup, ChildProcess mock type, and convergence-spy reuse are fixed.

Validation passed: four focused Hermes security tests, strict CLI type checking, repository checks, source-shape checks, and test-size checks. GitHub growth guardrails pass, and CodeRabbit reports no new actionable comments.

[aarav1109s]

The remaining Nemotron check failed because the NVIDIA inference endpoint returned HTTP 429 (shared rate limit exhausted), not because of the PR changes. Could a maintainer please rerun the failed job once the limit resets?

[aarav1109s]

I inspected the failed advisor artifact before retrying. Although the advisor itself failed canonical-receipt validation, it preserved one actionable finding: missing sensitive-file preflight coverage. Commit 1089696 adds a real .env symlink fixture and verifies Shields down fails before policy, timer, state, or audit mutation. Focused Hermes tests pass (5/5), both TypeScript builds pass, and repository checks pass.

[prekshivyas]

The new configUnlocked flag proves only that unlockAgentConfig() returned; false does not prove that no mutation occurred. The unlock path can fail after a state-directory or top-level mutation has started, and its internal rollback can also fail when a protected path is replaced or disappears. rollbackShieldsDown() then sees configUnlocked=false plus an unsafe-path relock error, restores the pre-transition state record, and reports UP without independently proving that every protected path is locked. Keep the provisional DOWN/manual-intervention state unless lockdown is positively re-verified, or return explicit rollback proof from the unlock transaction. Add a regression where the unsafe replacement occurs after mutation begins and verify status never reports UP for a partially unlocked tree.

[aarav1109s]

Addressed the rollback-proof concern in d067893. The provisional DOWN state is now cleared only after re-lock succeeds and is re-confirmed; an unlock exception is no longer treated as proof that no mutation occurred. If rollback remains incomplete, the transition is persisted as active so status remains DOWN/manual-intervention instead of being reinterpreted as UP. Added a regression covering an unsafe replacement after state-directory mutation begins, including failed internal rollback and failed outer re-lock. Focused #8804 tests pass (6/6), CLI typecheck, repository checks, formatting, and test-size checks pass.

[coderabbitai]

🧹 Nitpick comments (1)

src/lib/shields/index.ts (1)

5104-5122: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Extract incomplete-transition persistence into a helper.

Lines 5104-5122 add nested transition persistence to shieldsDownWithoutHostLock. This function already coordinates policy, timer, state, and provider operations. Move this branch into a focused helper that returns or reports the persistence result.

As per coding guidelines, **/*.{ts,tsx,js,jsx} requires: “Keep function complexity low.”

🤖 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/shields/index.ts` around lines 5104 - 5122, Extract the guarded
incomplete-transition persistence branch from shieldsDownWithoutHostLock into a
focused helper that accepts the required transition, timer authority, rollback,
and sandbox context, performs both freshness assertions and
writeShieldsDownTransition calls, and returns or reports success/failure.
Replace the inline branch with a call to this helper while preserving the
existing conditions and critical error reporting.

Source: Coding guidelines

🤖 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/shields/index.ts`:
- Around line 5104-5122: Extract the guarded incomplete-transition persistence
branch from shieldsDownWithoutHostLock into a focused helper that accepts the
required transition, timer authority, rollback, and sandbox context, performs
both freshness assertions and writeShieldsDownTransition calls, and returns or
reports success/failure. Replace the inline branch with a call to this helper
while preserving the existing conditions and critical error reporting.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 2632cd9b-14e2-4793-82d7-c31e558c8c36

📥 Commits

Reviewing files that changed from the base of the PR and between 1089696 and d067893.

📒 Files selected for processing (4)

• docs/reference/commands.mdx
• src/lib/shields/index.ts
• src/lib/shields/openclaw-transition.test.ts
• test/helpers/hermes-unsafe-config-shields-harness.ts

🚧 Files skipped from review as they are similar to previous changes (2)

• docs/reference/commands.mdx
• test/helpers/hermes-unsafe-config-shields-harness.ts

[aarav1109s]

The Nemotron PR Review Advisor failed because its SDK exceeded the 15-minute timeout after an earlier connection-refused transport error. The GPT-5.6 advisor, advisor publishing, CodeRabbit, and growth guardrails all passed, so this appears to be CI/infrastructure-related rather than a PR failure. Could a maintainer please rerun the failed Nemotron job?

The latest PR commit executes the production preflight against real unsafe-path fixtures and verifies that protected state does not change. Dismissing this resolved review before a full current-commit review.

[apurvvkumaria]

Review-advisor follow-up:

• Added a missing Hermes config.yaml regression case.
• The test confirms that shields down stops before sandbox execution, audit writes, timer creation, or posture weakening.
• The focused Shields suite passed all 33 tests.
• CLI type checking and the applicable repository, secret, source-shape, and test-size checks passed.
• The documentation writer confirmed that the existing Shields command update covers this refusal. No additional documentation change is required.
• GitHub marks the new signed commit as Verified.

Fresh CI and automated review are running. Security review, human approval, and all required checks remain required.

[apurvvkumaria]

The failed CLI shards exposed two test fixtures that did not model the new Shields preflight boundary.

• The policy-transition fixture now supplies the privileged execution and Docker preflight results before exercising its original assertions.
• The concurrent-down fixture now recognizes the preflight command before inspecting transition state.
• All 91 focused Shields tests, CLI type checking, applicable repository hooks, and diff checks pass.
• The static-check failure occurred while downloading shfmt after the remote end closed the connection; that failure is infrastructure-related, and the new branch activity will exercise the check again.
• Documentation review found no user-facing behavior change from this test-only update.

[apurvvkumaria]

Follow-up for the growth guardrail: the preflight setup now uses the test file’s existing linear switch pattern, and the repository conditional scanner passes.

The 91 focused Shields tests pass with ordinary host process access. Their failures under the restricted local runner were caused by denied child-process identity inspection; no production or additional test behavior was changed in response.

[apurvvkumaria]

The remaining static-check failure was PR-related: the unsafe-path preflight added two points to an already constrained Shields transition function. I moved the existing failure handling into a dedicated helper without changing the preflight order, emitted guidance, or termination behavior.

Biome now passes, the three focused Shields suites pass (99 tests), repository hooks pass, and the documentation follow-up found no user-facing change. The shard 7 failure was a timeout in an unrelated OpenClaw transition hook after prolonged runner activity; this push provides a fresh run rather than retrying that stale workflow.

[apurvvkumaria]

Sensitive-path security review completed for commit under review da7e90135: no findings.

• Authentication and authorization boundaries are unchanged.
• The preflight receives only manifest-validated canonical paths and invokes Python with an argument array, not a shell command.
• The config directory is opened without following symlinks, protected files are opened relative to that directory descriptor, and each descriptor is verified as the required file type.
• The existing Hermes guard remains the authoritative mutation boundary after preflight. If the path changes during the operation and re-lock cannot be verified, NemoClaw preserves the Shields down posture and timer authority and reports manual intervention instead of claiming the config is protected.
• No credential value is read, returned, or logged; diagnostics contain only fixed path and failure text.
• No dependency, network, cryptographic, privilege, or secret-source change is introduced.
• Regression coverage includes config-file symlinks, config-directory symlinks, a missing config, a sensitive-file symlink, replacement during unlock, partial mutation followed by failed rollback, and failed re-lock after unlock.

Validation: all required CI checks pass; the focused Shields suites pass 99 tests; CLI type checking, repository checks, source/test budgets, documentation validation, and secret scanning pass.

Size flag: this is a large review surface at 714 additions and 4 deletions across six files. Most of the increase is the typed adversarial test harness and regression coverage.

[prekshivyas]

Reviewed the current head, config-path preflight change, required CI, advisor feedback, and resolved threads. Required checks are green and no blocking code findings remain. The incomplete second-opinion lane is a nonblocking maintainer-disposition item.