fix(state): restore Hermes cron scripts before enabling restored jobs - #7880
fix(state): restore Hermes cron scripts before enabling restored jobs#7880laitingsheng wants to merge 34 commits into
Conversation
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
|
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 (6)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughHermes now persists ChangesSandbox restore flow
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested labels: Sequence Diagram(s)sequenceDiagram
participant SandboxRestore
participant RestoreCronGuard
participant HermesGateway
participant HermesState
SandboxRestore->>RestoreCronGuard: Begin drain
RestoreCronGuard->>HermesGateway: Stop dispatch and wait for active work
SandboxRestore->>HermesState: Stage scripts and cron state
SandboxRestore->>RestoreCronGuard: Validate enabled script references
RestoreCronGuard-->>SandboxRestore: Allow publication or fail closed
SandboxRestore->>HermesState: Publish scripts before cron
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
|
🌿 Preview your docs: https://nvidia-preview-pr-7880.docs.buildwithfern.com/nemoclaw |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit 61b91a3 in the TypeScript / code-coverage/cliThe overall coverage in commit 61b91a3 in the Show a code coverage summary of the most impacted files.
Updated |
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
7 terminology differences from the second opinionAdvisory only. These are normalized differences from the primary terminology receipt.
Second-opinion terminology and E2E selections are advisory. They do not change the primary assessment or E2E / PR Gate. 4 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: 1 optional E2E recommendation
This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
Signed-off-by: Tinson Lai <tinsonl@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: 2
🤖 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/state/sandbox-staged-restore.test.ts`:
- Around line 46-48: Update the fixture setup in the sandbox staged-restore test
to create the scripts directory if necessary and always write digest.sh,
removing the options.stateDirs.includes("scripts") conditional. Leave archive
selection controlled by the manifest.
In `@src/lib/state/sandbox.ts`:
- Around line 1377-1388: Update the restore command construction around the
commands array so staging cleanup runs when extraction or any state-directory
move fails, not only on successful completion. Add failure-safe shell cleanup,
such as an exit trap, for the staging path while preserving the existing
successful restore flow, and add coverage for failed extraction or move cleanup.
🪄 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: 00a0a7d7-3a7b-414a-96ef-f29fbed89c70
📒 Files selected for processing (5)
agents/hermes/manifest.yamldocs/manage-sandboxes/backup-restore.mdxdocs/reference/commands.mdxsrc/lib/state/sandbox-staged-restore.test.tssrc/lib/state/sandbox.ts
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/lib/state/sandbox-staged-restore.test.ts (1)
42-46:⚠️ Potential issue | 🟠 MajorStill create
backup/scriptsbefore the unconditional write.Lines 42-44 create only configured state directories, so Line 46 throws
ENOENTwheneverstateDirsomits"scripts". Create the fixture directory independently; let the manifest control whether it is restored.Proposed fix
for (const stateDir of options.stateDirs) { fs.mkdirSync(path.join(backupPath, stateDir), { recursive: true }); } + fs.mkdirSync(path.join(backupPath, "scripts"), { recursive: true }); fs.writeFileSync(path.join(backupPath, "cron", "jobs.json"), '{"jobs":[{"enabled":true}]}\n'); fs.writeFileSync(path.join(backupPath, "scripts", "digest.sh"), "#!/bin/bash\necho ok\n");This is the same unresolved fixture-setup issue reported previously.
🤖 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/state/sandbox-staged-restore.test.ts` around lines 42 - 46, Update the fixture setup around the stateDirs loop and scripts/digest.sh write to always create the backup/scripts directory independently before writing the file. Keep stateDirs controlling only which directories are configured for restoration, while ensuring the unconditional scripts fixture write cannot fail when scripts is omitted.
🤖 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.
Duplicate comments:
In `@src/lib/state/sandbox-staged-restore.test.ts`:
- Around line 42-46: Update the fixture setup around the stateDirs loop and
scripts/digest.sh write to always create the backup/scripts directory
independently before writing the file. Keep stateDirs controlling only which
directories are configured for restoration, while ensuring the unconditional
scripts fixture write cannot fail when scripts is omitted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ead92611-e2b1-4770-b6af-cfc925049245
📒 Files selected for processing (1)
src/lib/state/sandbox-staged-restore.test.ts
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/state/sandbox.ts (1)
1377-1390: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake failed publication rollback-safe, not only staging-clean.
The command still removes each live state directory before moving its staged replacement. If a
mvfails, the new EXIT trap removes staging but cannot restore the live directory already deleted; earlier directories may also have been published. A transient publication failure can therefore leave the sandbox partially restored or missing state. Use a rollback/transactional swap and extend the failure test to seed existing state and assert it survives.As per path instructions, destructive lifecycle operations must validate before mutation, preserve state/backup invariants, and cover failure/recovery/rebuild/resume behavior.
🤖 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/state/sandbox.ts` around lines 1377 - 1390, Update the restore command construction around orderStateDirsForRestore so publication is rollback-safe: preserve each existing live state directory in a backup, publish staged replacements transactionally, and restore the original state if any removal or move fails before cleanup. Keep staging cleanup via the EXIT trap, and extend the failure test to seed existing state and verify it remains intact after a failed publication.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 `@src/lib/state/sandbox-staged-restore.test.ts`:
- Around line 80-83: The movesFail shim in the staged-restore test must record
that publication was attempted before exiting. Update the injected script in the
movesFail branch to write a failure marker, then assert that marker together
with success === false and staging cleanup in the affected test cases.
---
Outside diff comments:
In `@src/lib/state/sandbox.ts`:
- Around line 1377-1390: Update the restore command construction around
orderStateDirsForRestore so publication is rollback-safe: preserve each existing
live state directory in a backup, publish staged replacements transactionally,
and restore the original state if any removal or move fails before cleanup. Keep
staging cleanup via the EXIT trap, and extend the failure test to seed existing
state and verify it remains intact after a failed publication.
🪄 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: 9b7dc759-dc37-43fb-99aa-9c06ac5ca2bb
📒 Files selected for processing (2)
src/lib/state/sandbox-staged-restore.test.tssrc/lib/state/sandbox.ts
laitingsheng
added
NV QA
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
|
Babysitting review for exact head
No base refresh is requested. |
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/state/sandbox.ts (1)
1352-1393: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake staged publication rollback-safe.
Removing a live directory before its staged
mvsucceeds turns a failed restore into data loss; the EXIT trap only removes staging. Preserve every replaced live directory in a rollback location, restore all prior directories if any publish step fails, and delete rollback data only after the full ordered publication succeeds. Add failed-mvcoverage proving existingscriptsandcronsurvive and restoredcronis never exposed without its scripts.As per path instructions, destructive lifecycle operations must preserve state/backup invariants and cover failure, recovery, rebuild, and resume behavior without bypassing the public action boundary.
🤖 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/state/sandbox.ts` around lines 1352 - 1393, Update buildStagedRestoreCommand and its restore tests to make publication rollback-safe: move each existing live state directory into a rollback location before replacing it, and on any failed publish restore all previously replaced directories in the original order. Remove rollback data only after every ordered directory publish succeeds, while retaining staging cleanup. Add failed-mv coverage proving existing scripts and cron are preserved and cron is never visible without its restored scripts.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.
Outside diff comments:
In `@src/lib/state/sandbox.ts`:
- Around line 1352-1393: Update buildStagedRestoreCommand and its restore tests
to make publication rollback-safe: move each existing live state directory into
a rollback location before replacing it, and on any failed publish restore all
previously replaced directories in the original order. Remove rollback data only
after every ordered directory publish succeeds, while retaining staging cleanup.
Add failed-mv coverage proving existing scripts and cron are preserved and cron
is never visible without its restored scripts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 67464f1b-80a9-46ba-bb13-4d77aee86011
📒 Files selected for processing (3)
docs/manage-sandboxes/backup-restore.mdxdocs/reference/commands.mdxsrc/lib/state/sandbox.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/manage-sandboxes/backup-restore.mdx
- docs/reference/commands.mdx
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
…edger-e2e Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
apurvvkumaria
left a comment
apurvvkumaria
left a comment
There was a problem hiding this comment.
Blocking: current head 7824b3f is syntactically invalid. The merge-resolution change removed the closing brace of buildStagedRestoreCommand in src/lib/state/sandbox.ts after its return and before snapshotManifestAuthority. Parsing the exact blob reaches EOF with TypeScript diagnostic TS1005 at 2615:1: closing brace expected, so the CLI cannot type-check or build and the feature cannot serve its stated purpose.\n\nPlease restore the missing closing brace and run npm run typecheck:cli plus the staged-restore tests and required CI. This is a direct, reproducible current-revision build break.
apurvvkumaria
left a comment
apurvvkumaria
left a comment
There was a problem hiding this comment.
Re-reviewed exact head a5a698f. The blocking syntax defect remains in src/lib/state/sandbox.ts: buildStagedRestoreCommand closes its return expression with ); but never closes the function body before function snapshotManifestAuthority begins. TypeScript therefore parses the following declarations inside a function and the supported CLI build/typecheck cannot succeed. Please add the missing closing brace immediately after the return expression and add or run a focused syntax/typecheck regression on this exact revision. No other blocking finding is asserted here.
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@agents/hermes/restore-cron-guard.py`:
- Around line 108-139: Update validate_enabled_scripts to validate each enabled
script’s readability using the gateway user’s permissions rather than os.access
under the restore process; perform the open/readability check through the
existing privilege-switching mechanism and retain the current missing/unreadable
ValueError behavior. Add a negative test covering a sandbox-owned 0600 script
that the restore process can access but gateway cannot.
- Around line 50-84: Update begin_drain and _release_owned_marker to use atomic
create-if-absent and clear-if-token-matches operations. Always acquire an owned
drain marker before checking or waiting on the gateway PID, including when it is
initially inactive, and return “inactive” only after safely releasing that owned
marker. Ensure the owned token is released on successful completion as well as
timeout, commit, or rollback, while never clearing a replacement marker.
In `@docs/security/best-practices.mdx`:
- Line 347: In the Hermes rebuild restore description, split the two sentences
onto separate source lines while preserving their wording and paragraph
formatting.
🪄 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: bdce40c8-3a99-4ed7-aab7-38e057a478e2
📒 Files selected for processing (7)
agents/hermes/Dockerfileagents/hermes/manifest.yamlagents/hermes/restore-cron-guard.pydocs/manage-sandboxes/backup-restore.mdxdocs/reference/commands.mdxdocs/security/best-practices.mdxscripts/state-dir-guard.py
🚧 Files skipped from review as they are similar to previous changes (2)
- agents/hermes/manifest.yaml
- docs/reference/commands.mdx
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Claim an exclusive restore drain marker before the gateway PID check so a gateway that starts during publication still sees the drain, release only a marker the restore owns, and validate enabled cron scripts against the gateway account rather than the restore process. Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
|
Exact-head CI follow-up for Please update the committed Hermes restore-guard digest to the exact current script bytes (and its contract assertion, if applicable), then let the new head rerun the managed-image and protected E2E gates. I have not edited the author branch or rerun the failing SHA. |
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
The image build and the final-image layout contract both compare agents/hermes/restore-cron-guard.py against the committed digest, and the pinned value still described an earlier revision of the script, so buildx stopped at the sha256sum guard before startup. Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
agents/hermes/restore-cron-guard.py (1)
55-59: 🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy liftMake drain-marker ownership operations atomic.
Lines 96-100 can overwrite an operator drain that appears after
drain_requested(). Lines 55-59 can clear an operator drain that replaces this restore marker after the ownership check. The local ownership file does not protect the shared gateway marker.Add create-if-absent and clear-if-token-matches operations to
drain_control. Add an interleaving test for both replacement races. This violates the required operator-drain preservation.Also applies to: 96-100
🤖 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 `@agents/hermes/restore-cron-guard.py` around lines 55 - 59, Make drain-marker ownership atomic across the restore flow: add drain_control operations that create the shared marker only if absent and clear it only when its token still matches, then update the marker acquisition around drain_requested() and _release_owned_marker to use them. Ensure operator drains inserted between either ownership check and mutation are preserved, and add interleaving coverage for both replacement races.
🧹 Nitpick comments (2)
src/lib/state/sandbox-staged-restore.test.ts (1)
158-162: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMake the unset failure-target case explicit.
If
options.failPublishingDiris undefined, line 160 evaluatestarget.endsWith("/"). This is false for current production targets, so the shim behaves correctly today. The guard depends on targets never carrying a trailing slash.Compare against a sentinel that cannot match instead.
♻️ Proposed change
-if ( - ${JSON.stringify(options.movesFail === true)} || - (isPublish && target.endsWith("/" + ${JSON.stringify(options.failPublishingDir ?? "")})) || - (isRollback && target.endsWith("/" + ${JSON.stringify(options.failRollingBackDir ?? "")})) -) process.exit(1); +const failPublishingDir = ${JSON.stringify(options.failPublishingDir ?? null)}; +const failRollingBackDir = ${JSON.stringify(options.failRollingBackDir ?? null)}; +if ( + ${JSON.stringify(options.movesFail === true)} || + (isPublish && failPublishingDir !== null && target.endsWith("/" + failPublishingDir)) || + (isRollback && failRollingBackDir !== null && target.endsWith("/" + failRollingBackDir)) +) process.exit(1);🤖 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/state/sandbox-staged-restore.test.ts` around lines 158 - 162, Update the failure-target checks in the generated shim around options.failPublishingDir and options.failRollingBackDir so an unset directory compares against an explicitly non-matching sentinel instead of constructing a trailing-slash suffix. Preserve the existing publishing and rollback failure behavior when a directory is configured.test/helpers/vitest-watch-triggers.ts (1)
87-90: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
test/hermes-final-image-layout.test.tsto this trigger. It compares the SHA-256 ofagents/hermes/restore-cron-guard.pywithNEMOCLAW_HERMES_RESTORE_CRON_GUARD_SHA256; keeptest/hermes-restore-cron-guard.test.tsand update its watch-trigger expectation.🤖 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/vitest-watch-triggers.ts` around lines 87 - 90, Add test/hermes-final-image-layout.test.ts to the restore-cron-guard entry in the watch-trigger configuration, while retaining test/hermes-restore-cron-guard.test.ts. Update the corresponding watch-trigger expectation so both tests run when agents/hermes/restore-cron-guard.py changes.
🤖 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 `@agents/hermes/restore-cron-guard.py`:
- Around line 148-160: Update _readable_by_gateway() to validate gateway search
(execute) permission on every directory from scripts_dir through
script_path.parent, in addition to the script file’s read permission. Preserve
the existing identity and ownership checks, and add a nested 0700-directory test
covering the scheduler’s validation path.
In `@src/lib/state/sandbox-staged-restore.test.ts`:
- Around line 448-459: Update the test case “rejects an enabled restored job
whose script is absent” to assert the guard-event sequence used by its sibling
test around line 441, specifically proving that the validate guard rejects the
missing restored script. Keep the existing rollback and state-preservation
assertions unchanged.
In `@test/hermes-restore-cron-guard.test.ts`:
- Line 29: Update the parent test suite title in the “Hermes restore cron guard”
describe block to append the linked issue suffix “(`#7806`)”, leaving the child
test titles unchanged.
- Around line 192-214: Update the acceptance test around runGuardModule and the
digest.sh fixture so its readability check is independent of the host account:
either stub _gateway_identity() explicitly to the test user or make the fixture
searchable by gateway and change digest.sh to a gateway-readable mode. Preserve
the existing enabled/disabled job assertions and successful validation outcome.
---
Duplicate comments:
In `@agents/hermes/restore-cron-guard.py`:
- Around line 55-59: Make drain-marker ownership atomic across the restore flow:
add drain_control operations that create the shared marker only if absent and
clear it only when its token still matches, then update the marker acquisition
around drain_requested() and _release_owned_marker to use them. Ensure operator
drains inserted between either ownership check and mutation are preserved, and
add interleaving coverage for both replacement races.
---
Nitpick comments:
In `@src/lib/state/sandbox-staged-restore.test.ts`:
- Around line 158-162: Update the failure-target checks in the generated shim
around options.failPublishingDir and options.failRollingBackDir so an unset
directory compares against an explicitly non-matching sentinel instead of
constructing a trailing-slash suffix. Preserve the existing publishing and
rollback failure behavior when a directory is configured.
In `@test/helpers/vitest-watch-triggers.ts`:
- Around line 87-90: Add test/hermes-final-image-layout.test.ts to the
restore-cron-guard entry in the watch-trigger configuration, while retaining
test/hermes-restore-cron-guard.test.ts. Update the corresponding watch-trigger
expectation so both tests run when agents/hermes/restore-cron-guard.py changes.
🪄 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: f077068a-46d4-483d-a08a-57959952d9af
📒 Files selected for processing (20)
agents/hermes/Dockerfileagents/hermes/manifest.yamlagents/hermes/restore-cron-guard.pydocs/manage-sandboxes/backup-restore.mdxdocs/reference/commands.mdxdocs/security/best-practices.mdxscripts/state-dir-guard.pysrc/lib/onboard/managed-startup/profile.tssrc/lib/shields/state-dir-lock.tssrc/lib/state/sandbox-staged-restore.test.tssrc/lib/state/sandbox.tstest/helpers/vitest-watch-triggers.tstest/hermes-doctor-config-hash.test.tstest/hermes-final-image-layout.test.tstest/hermes-restore-cron-guard.test.tstest/sandbox-provisioning.test.tstest/sandbox-rlimit-hooks.test.tstest/shields-up-runtime-perms.test.tstest/state-dir-guard.test.tstest/vitest-watch-triggers.test.ts
🚧 Files skipped from review as they are similar to previous changes (11)
- src/lib/onboard/managed-startup/profile.ts
- scripts/state-dir-guard.py
- agents/hermes/manifest.yaml
- test/hermes-final-image-layout.test.ts
- docs/security/best-practices.mdx
- docs/reference/commands.mdx
- test/sandbox-provisioning.test.ts
- agents/hermes/Dockerfile
- src/lib/state/sandbox.ts
- test/hermes-doctor-config-hash.test.ts
- docs/manage-sandboxes/backup-restore.mdx
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Dismissed as stale because commit 6949d3e restored the missing buildStagedRestoreCommand closing brace, and the current head passes the CLI build/typecheck path. This dismissal addresses only the obsolete syntax defect; newer review threads and check status remain independent.
The restore guard only checked the script's own permission bits, so a script inside a directory the gateway cannot search passed validation and then failed when the scheduler opened it. Validation now walks every directory from the scripts root to the script's parent, and the pinned image digest follows the changed script. Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
7 participants
Summary
Hermes confines the scripts that
no_agentcron jobs run toHERMES_HOME/scripts, but that directory was absent from the Hermes state contract. A rebuild could therefore restore enabled cron definitions without restoring the scripts they call. This PR includesscriptsin Hermes state, publishes restored directories as complete units withcronafterscripts, and rolls live state back if staged publication fails instead of leaving a partial restore or deleting the prior state.Before either live scheduled-work directory moves, NemoClaw now asks the running Hermes gateway to enter its native external-drain state and waits for messaging, API, and cron activity to reach zero. It validates every enabled restored script reference before releasing only the drain marker NemoClaw created. Operator-owned drains remain in place, and rollback failure preserves both the recovery tree and the drain.
Related Issue
Fixes #7806
Changes
scriptsto the Hermes state contract and the high-risk state-directory lock inventory..nemoclaw-restore-rollbackand refuse a later restore if rollback or post-commit cleanup cannot finish, so a retry cannot overwrite the recovery copy.scripts/crontransition; wait forgateway_state=drainingand zero active messaging, API, and cron work.HERMES_HOME/scriptsbefore resuming dispatch. Hermes invokes those files through an explicit Bash/Python interpreter, so readability is the pinned runtime permission contract.Type of Change
Quality Gates
88f6b0cfa9c69976ba0df1b0fd0b0fad56c84b35. Secrets, input/path validation, authorization, dependencies, logging, cryptography, configuration scope, rollback/concurrency integrity, and regression coverage pass. The root-owned image helper is hash-bound; every dynamic shell path is quoted; enabled scripts must remain under the resolved scripts root; operator drains are not intentionally released; and failed rollback preserves the drain and recovery tree. The follow-up CI repair only adds the helper to Docker replay fixtures and classifies its SHA-256 build argument as an integrity pin. Exact diff fingerprint:d93aa2010392d6b11ba46b87da530791f32e03b997cd6743a92ff25718a272ea(git diff --binary origin/main...HEAD | shasum -a 256). Fresh exact-head CI/E2E and automated review remain required.Documentation Writer Review
docs-updateddocs/manage-sandboxes/backup-restore.mdx,docs/reference/commands.mdx, anddocs/security/best-practices.mdx; the exact-head CI repair is test/contract-only and needs no additional docs.npm run docscompleted with 0 errors and 2 pre-existing warnings.DGX Station Hardware Evidence
scripts/prepare-dgx-station-host.shis unchanged.Verification
Signed-off-by:line and every new commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, andnpm run validate:prpassed against currentorigin/mainnpm run checks, and fullnpm run validate:prpass. The config-hash path also passes 3/3 in a clean Linux container with isolated PyYAML; the macOS-only permission result is an environment limitation, not a product failure.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result: Fresh exact-head GitHub CI, E2E, CodeRabbit, and Advisor gates are running.npm run docsbuilds without warnings (doc changes only) — 0 errors and 2 pre-existing Fern warningsSigned-off-by: Tinson Lai tinsonl@nvidia.com
Summary by CodeRabbit
New Features
Bug Fixes
Documentation