fix(hermes): precreate .hermes_history so shields-up doesn't lock TUI#4708
Merged
Conversation
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR ensures /sandbox/.hermes/.hermes_history exists with sandbox:sandbox ownership and 660 permissions at image build and at startup; adds ensure_hermes_history_file and invokes it during repair; adds an E2E history-writable probe and wires it into suites; and extends unit tests to cover pre-existing history states. ChangesHermes History File Safety and Writability
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
E2E Advisor RecommendationRequired E2E: Auto-dispatched E2E: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
|
Contributor
E2E Scenario Advisor RecommendationRequired scenario E2E: Dispatch required scenario E2E:
Full scenario advisor summaryE2E Scenario AdvisorBase: Required scenario E2E
Optional scenario E2E
Relevant changed files
|
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 26875066276
|
Contributor
PR Review AdvisorFindings: 2 needs attention, 2 worth checking, 0 nice ideas Review findings🛠️ Needs attention
🔎 Worth checking
🌱 Nice ideas
Since last review detailsCurrent findings:
This is an automated advisory review. A human maintainer must make the final merge decision. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/hermes-start.test.ts (1)
462-475: ⚡ Quick winAssert the symlink target was not written through.
The symlink refusal test only checks
historyKind === "symlink"and the stderr message. But a regression whereensure_hermes_history_filefollowed the symlink (e.g. opening/truncating the target) would still leave the path classified as asymlink, so this case would pass while the actual security property—no write-through to the attacker-controlled target—is broken. Capturing and asserting the target content closes that detection gap.♻️ Capture and assert the symlink target content
In
runHermesGatewayRuntimeCleanup, hoist the target path and return its content:const historyPath = path.join(hermesHome, ".hermes_history"); + const historyTarget = path.join(tmpDir, "history-target"); if (opts.preExistingHistory === "regular") { fs.writeFileSync(historyPath, "pre-existing\n", { mode: 0o600 }); } else if (opts.preExistingHistory === "symlink") { - const target = path.join(tmpDir, "history-target"); - fs.writeFileSync(target, "attacker\n"); - fs.symlinkSync(target, historyPath); + fs.writeFileSync(historyTarget, "attacker\n"); + fs.symlinkSync(historyTarget, historyPath); } else if (opts.preExistingHistory === "directory") { fs.mkdirSync(historyPath); }historyMode, historyKind, historyContent, + historySymlinkTargetContent: fs.existsSync(historyTarget) + ? fs.readFileSync(historyTarget, "utf-8") + : "",Then assert in the symlink test:
expect(run.historyKind).toBe("symlink"); + expect(run.historySymlinkTargetContent).toBe("attacker\n"); expect(run.result.stderr).toContain( "Refusing Hermes layout repair because", );🤖 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/hermes-start.test.ts` around lines 462 - 475, The symlink test must also assert the symlink target was not written-through: modify the helper runHermesGatewayRuntimeCleanup to record/return the symlink target path and its file content (or absence) when preExistingHistory === "symlink", then in the test that calls runHermesGatewayRuntimeCleanup (the case with preExistingHistory: "symlink" and expecting historyKind === "symlink") add an assertion that the target file's content equals the original sentinel content (or remains untouched/uncreated) to ensure ensure_hermes_history_file did not follow/truncate the symlink; reference runHermesGatewayRuntimeCleanup and ensure_hermes_history_file to locate the code to change.test/e2e-scenario/validation_suites/hermes/01-history-writable.sh (1)
46-46: 💤 Low valuePrefer the canonical
e2e_sandbox_exechelper over directopenshell sandbox exec.
test/e2e-scenario/validation_suites/sandbox-exec.shexists specifically to absorb the recurring drift (quoting, exit-code propagation, dry-run handling) of hand-rolled sandbox exec calls across migrated suite steps. These three direct invocations reintroduce that drift. Sourcing and usinge2e_sandbox_exec "${sandbox_name}" -- <cmd>would keep this step on the shared contract.This is optional given dry-run is already short-circuited at the top of the script.
Also applies to: 70-71, 79-80
🤖 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-scenario/validation_suites/hermes/01-history-writable.sh` at line 46, Replace the direct calls to "openshell sandbox exec" in this script with the canonical helper "e2e_sandbox_exec" to ensure consistent quoting, exit-code propagation, and dry-run handling; specifically, where the script currently captures metadata into the "meta" variable using "openshell sandbox exec --name \"${sandbox_name}\" -- stat -c '%F|%U:%G|%a' \"${HISTORY_PATH}\"" and the two subsequent direct invocations, call e2e_sandbox_exec "${sandbox_name}" -- stat -c '%F|%U:%G|%a' "${HISTORY_PATH}" and adapt other similar lines to use e2e_sandbox_exec "${sandbox_name}" -- <original command>, preserving variable usage (sandbox_name, HISTORY_PATH) and capturing output into the same variables.
🤖 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-scenario/validation_suites/hermes/01-history-writable.sh`:
- Around line 91-116: The shields_status_state helper currently swallows command
errors with "|| true" and returns "unknown" on parse failure; change it so the
nemoclaw call is not suppressed (remove "|| true") so failures propagate, and
after computing initial_state check for "unknown" and fail loudly (print an
explanatory error to stderr and exit 1) before calling probe_history_writable;
reference shields_status_state and initial_state to locate the change and ensure
the script exits when shields parsing fails rather than continuing silently.
---
Nitpick comments:
In `@test/e2e-scenario/validation_suites/hermes/01-history-writable.sh`:
- Line 46: Replace the direct calls to "openshell sandbox exec" in this script
with the canonical helper "e2e_sandbox_exec" to ensure consistent quoting,
exit-code propagation, and dry-run handling; specifically, where the script
currently captures metadata into the "meta" variable using "openshell sandbox
exec --name \"${sandbox_name}\" -- stat -c '%F|%U:%G|%a' \"${HISTORY_PATH}\""
and the two subsequent direct invocations, call e2e_sandbox_exec
"${sandbox_name}" -- stat -c '%F|%U:%G|%a' "${HISTORY_PATH}" and adapt other
similar lines to use e2e_sandbox_exec "${sandbox_name}" -- <original command>,
preserving variable usage (sandbox_name, HISTORY_PATH) and capturing output into
the same variables.
In `@test/hermes-start.test.ts`:
- Around line 462-475: The symlink test must also assert the symlink target was
not written-through: modify the helper runHermesGatewayRuntimeCleanup to
record/return the symlink target path and its file content (or absence) when
preExistingHistory === "symlink", then in the test that calls
runHermesGatewayRuntimeCleanup (the case with preExistingHistory: "symlink" and
expecting historyKind === "symlink") add an assertion that the target file's
content equals the original sentinel content (or remains untouched/uncreated) to
ensure ensure_hermes_history_file did not follow/truncate the symlink; reference
runHermesGatewayRuntimeCleanup and ensure_hermes_history_file to locate the code
to change.
🪄 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: 36dc62b4-a126-4be6-a94c-c2b78afde323
📒 Files selected for processing (8)
agents/hermes/Dockerfileagents/hermes/Dockerfile.baseagents/hermes/start.shtest/e2e-scenario/scenarios/assertions/registry.tstest/e2e-scenario/scenarios/migration-inventory.tstest/e2e-scenario/validation_suites/hermes/01-history-writable.shtest/e2e-scenario/validation_suites/suites.yamltest/hermes-start.test.ts
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
…fig seal Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 26904869296
|
cv
approved these changes
Jun 3, 2026
12 tasks
cv
added a commit
that referenced
this pull request
Jun 3, 2026
## Summary Hardens the Hermes `.hermes_history` repair added in #4708 so startup no longer performs privileged check-then-use mutations on a sandbox-writable pathname. It also records the shields-up history-file exception in the central Hermes/shields contracts and preserves history through Hermes state backups. ## Related Issue Follow-up to #4708; refs #2432. ## Changes - Replace path-based `.hermes_history` repair in `agents/hermes/start.sh` with an `O_NOFOLLOW` fd-based Python helper that validates, `fchown`s, and `fchmod`s the opened inode. - Document the intentional `sandbox:sandbox 0660` `.hermes_history` exception in `src/lib/shields/state-dir-lock.ts`, `agents/hermes/manifest.yaml`, and `agents/hermes/policy-additions.yaml`. - Add `.hermes_history` to Hermes `state_files` and update snapshot coverage so history is backed up and restored. - Clarify that the scenario E2E history append probe is the accepted proxy for the `/exit` symptom from #2432. ## 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) ## Verification - [x] `npx prek run --all-files` passes - [x] `npm test` passes - [x] Tests added or updated for new or changed behavior - [x] No secrets, API keys, or credentials committed - [ ] Docs updated for user-facing behavior changes - [ ] `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) --- Signed-off-by: Carlos Villela <cvillela@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes * **New Features** * Hermes history file is now included in backup and restore operations, ensuring command history is preserved across sessions. * **Bug Fixes** * Enhanced reliability of history file handling with improved validation and robustness during repair operations. * **Tests** * Expanded test coverage for history file backup and restore scenarios. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Carlos Villela <cvillela@nvidia.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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
Hermes TUI uses
prompt_toolkit.FileHistoryrooted at$HERMES_HOME/.hermes_history. Upstream hardcodes the path with no env override. Undershields up,/sandbox/.hermesis re-owned toroot:root 0755and the sandbox user can no longer create the missing history file, so every keypress emits "Permission denied" and blocks/exit. Precreating the file as a sandbox-owned regular file means the lockdown only restricts the parent dir, while file-level perms still letopen("ab")succeed.Related Issue
Fixes #2432
Changes
/sandbox/.hermes/.hermes_historyassandbox:sandbox 0660inDockerfile.base+ the Hermes derivedDockerfile.ensure_hermes_history_file()inagents/hermes/start.shand wire it intorepair_hermes_startup_layoutso runtime repair restores the file when it's missing or has drifted ownership. The helper refuses symlinks, non-regular paths, and hard-linked targets (a hard link toconfig.yaml/.envwould let the subsequentchmod 660walk the shared inode and silently undo the shields-uproot:root 0444lock afterverify_config_integrityhas already passed).test/hermes-start.test.tswith cases covering: unlocked + create, pre-existing regular file mode re-asserted, symlink refused with no write-through, directory refused, locked-root + create, locked-root + symlink, locked-root + directory, locked-root + hard-link-to-config.yaml(assertsconfig.yamlperms + content unchanged).hermes/01-history-writable.shE2E probe in thehermes-specificsuite. Reproduces the exactprompt_toolkitopen("ab")call against the running sandbox in both shields-down and shields-up states (forces shields-up when the scenario starts shields-down so the regression cannot slip through).Type of Change
Verification
Signed-off-by: Tinson Lai tinsonl@nvidia.com
Summary by CodeRabbit
Bug Fixes
Tests