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 25 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 (1)
📝 WalkthroughWalkthroughHermes now persists ChangesSandbox restore flow
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 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 |
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. 3 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: 3 optional E2E recommendations
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>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
Exact-head babysitting update for |
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. |
6 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