fix(hermes): prevent cron dispatch during rebuild restore - #7871
fix(hermes): prevent cron dispatch during rebuild restore#7871HOYALIM wants to merge 10 commits into
Conversation
Signed-off-by: Ho Lim <subhoya@gmail.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughHermes cron rebuilds now validate restored job scripts, drain the gateway during state restoration, verify gateway identity and cron readiness, and release dispatch only after successful validation. The controller is packaged in the Hermes image, and cron scripts are preserved in rebuild backups. ChangesHermes cron restore
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant RebuildPipeline
participant BackupValidator
participant HermesCronControl
participant HermesGateway
RebuildPipeline->>BackupValidator: validate cron backup
RebuildPipeline->>HermesCronControl: begin restore gate
HermesCronControl->>HermesGateway: request draining
HermesGateway-->>HermesCronControl: draining with zero active agents
RebuildPipeline->>RebuildPipeline: restore durable state
RebuildPipeline->>HermesCronControl: validate restore
HermesCronControl->>HermesGateway: verify identity and drain
HermesCronControl-->>RebuildPipeline: validation receipt
RebuildPipeline->>HermesCronControl: release restore gate
HermesCronControl->>HermesGateway: clear drain request
HermesGateway-->>HermesCronControl: running
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
PR Review Advisor — Blocking findings reportedAdvisor assessment: Blockers require maintainer review Model lanes
Second-opinion terminology and E2E selections are advisory. They do not change the primary assessment or E2E / PR Gate. 1 semantic terminology decisionTerminology 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: 2 optional E2E recommendations
Blockers
|
![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
🧹 Nitpick comments (3)
src/lib/actions/sandbox/rebuild-pipeline.ts (1)
360-379: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the gated-restore IIFE into a named helper.
The inline
(() => { try {...} catch {...} })()embedded in a ternary is hard to scan at a glance and mixes two concerns (transaction execution + error reporting) inline with the main rebuild flow.♻️ Suggested refactor
- const restored = hermesCronRestorePlan?.requiresDispatchGate - ? (() => { - try { - return runHermesCronRestoreTransaction(sandboxName, restore, (state, identity) => { - log( - `Hermes cron restore gate ${state}: pid=${String(identity.pid)}, startTime=${String(identity.start_time)}`, - ); - }); - } catch (error) { - console.error(""); - console.error( - error instanceof HermesCronRestoreIncompleteError - ? " Hermes cron dispatch remains drained because state restore was incomplete." - : ` Hermes cron restore could not prove safe reactivation: ${rebuildFailureDetail(error)}`, - ); - console.error(` Backup is preserved at: ${backup.backupManifest?.backupPath}`); - return bail("Hermes cron restore validation failed; dispatch was not re-enabled."); - } - })() - : restore(); + const runGatedHermesCronRestore = (): typeof restore extends () => infer T ? T : never => { + try { + return runHermesCronRestoreTransaction(sandboxName, restore, (state, identity) => { + log( + `Hermes cron restore gate ${state}: pid=${String(identity.pid)}, startTime=${String(identity.start_time)}`, + ); + }); + } catch (error) { + console.error(""); + console.error( + error instanceof HermesCronRestoreIncompleteError + ? " Hermes cron dispatch remains drained because state restore was incomplete." + : ` Hermes cron restore could not prove safe reactivation: ${rebuildFailureDetail(error)}`, + ); + console.error(` Backup is preserved at: ${backup.backupManifest?.backupPath}`); + return bail("Hermes cron restore validation failed; dispatch was not re-enabled."); + } + }; + const restored = hermesCronRestorePlan?.requiresDispatchGate + ? runGatedHermesCronRestore() + : restore();🤖 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/actions/sandbox/rebuild-pipeline.ts` around lines 360 - 379, Extract the inline gated-restore IIFE from the `restored` assignment into a named helper near the rebuild flow. Have the helper execute `runHermesCronRestoreTransaction`, preserve the existing gate-state logging and error-reporting behavior, and return `bail(...)` on failure; then call the helper from the `hermesCronRestorePlan?.requiresDispatchGate` branch while leaving the `restore()` branch unchanged.src/lib/actions/sandbox/rebuild-hermes-cron-restore/backup.ts (1)
4-13: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffDirect filesystem I/O inside an actions-layer module.
This file performs raw
fssyscalls (lstatSync,readdirSync,readFileSync,realpathSync) directly insidesrc/lib/actions/**, interleaved with the validity decisions (symlink checks, script-escape rules, size limits). Per the repo's layering rules, host-boundary work belongs in an adapter (injectable for tests) while the pure accept/reject decisions belong in a domain module; this file currently owns both.♻️ Suggested direction
Split into: an adapter that exposes
lstat/readdir/readFile/realpathprimitives (mockable in tests), and a domain module that takes the resulting metadata and returns accept/reject decisions — with this file reduced to orchestrating the two.As per path instructions for
src/lib/{actions,domain,adapters,state}/**: "actions orchestrate, domain modules make pure decisions, adapters own host/process/network boundaries" (src/lib/README.md).Also applies to: 182-212
🤖 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/actions/sandbox/rebuild-hermes-cron-restore/backup.ts` around lines 4 - 13, Split the filesystem and validation responsibilities currently combined in the backup flow: move lstat/readdir/readFile/realpath calls behind an injectable adapter, move symlink, script-escape, and size-limit accept/reject logic into a pure domain module, and reduce the action entrypoint to orchestrating those components while preserving existing behavior. Use the existing backup-related functions and module symbols to identify the orchestration, adapter, and domain boundaries.Source: Path instructions
test/hermes-final-image-layout.test.ts (1)
239-239: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd the matching permission assertions
The test only covers the newCOPYentry; add thechmod 755andchown root:rootexpectations forhermes-cron-restore-control.pytoo so the new script’s ownership and mode changes stay covered.🤖 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-final-image-layout.test.ts` at line 239, Add assertions in the Hermes final image layout test for the copied hermes-cron-restore-control.py entry, verifying it receives chmod 755 and chown root:root alongside the existing COPY expectation. Keep the assertions scoped to this script’s permission and ownership configuration.
🤖 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/actions/sandbox/rebuild-hermes-cron-restore/backup.ts`:
- Around line 131-133: Replace the permission-bit-only check in the rebuild
preflight with an `isReadableSync` helper that calls `accessSync(path,
constants.R_OK)` and returns false on errors. Use this helper when validating
the resolved script path so the preflight matches the Python `_validate_script`
readability gate and throws the existing unreadable-script error before deleting
the old sandbox.
In `@test/hermes-cron-restore-control.test.ts`:
- Around line 17-71: Extend the Hermes cron restore control tests beyond
validateTree to directly cover begin_drain, validate_restore, and release_drain
in cron-restore-control.py. Add successful lifecycle coverage plus failure cases
for invalid gateway identity and missing or incorrect drain markers, asserting
each command’s status and relevant output while preserving the existing
temporary-home setup and cleanup.
---
Nitpick comments:
In `@src/lib/actions/sandbox/rebuild-hermes-cron-restore/backup.ts`:
- Around line 4-13: Split the filesystem and validation responsibilities
currently combined in the backup flow: move lstat/readdir/readFile/realpath
calls behind an injectable adapter, move symlink, script-escape, and size-limit
accept/reject logic into a pure domain module, and reduce the action entrypoint
to orchestrating those components while preserving existing behavior. Use the
existing backup-related functions and module symbols to identify the
orchestration, adapter, and domain boundaries.
In `@src/lib/actions/sandbox/rebuild-pipeline.ts`:
- Around line 360-379: Extract the inline gated-restore IIFE from the `restored`
assignment into a named helper near the rebuild flow. Have the helper execute
`runHermesCronRestoreTransaction`, preserve the existing gate-state logging and
error-reporting behavior, and return `bail(...)` on failure; then call the
helper from the `hermesCronRestorePlan?.requiresDispatchGate` branch while
leaving the `restore()` branch unchanged.
In `@test/hermes-final-image-layout.test.ts`:
- Line 239: Add assertions in the Hermes final image layout test for the copied
hermes-cron-restore-control.py entry, verifying it receives chmod 755 and chown
root:root alongside the existing COPY expectation. Keep the assertions scoped to
this script’s permission and ownership configuration.
🪄 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: 9637835a-e6fe-4b49-b92b-c42d404fc276
📒 Files selected for processing (10)
agents/hermes/Dockerfileagents/hermes/cron-restore-control.pyagents/hermes/manifest.yamlsrc/lib/actions/sandbox/rebuild-hermes-cron-restore.test.tssrc/lib/actions/sandbox/rebuild-hermes-cron-restore/backup.tssrc/lib/actions/sandbox/rebuild-hermes-post-restore.tssrc/lib/actions/sandbox/rebuild-pipeline.tssrc/lib/actions/sandbox/rebuild-post-restore-phase.tstest/hermes-cron-restore-control.test.tstest/hermes-final-image-layout.test.ts
Signed-off-by: Ho Lim <subhoya@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/actions/sandbox/rebuild-hermes-cron-restore/backup.ts`:
- Around line 5-6: Remove the direct filesystem and JSON persistence handling
from the backup restore action, including the accessSync/constants imports, and
move backup-tree traversal and persisted-state reads into the appropriate
state-layer API under state modules. Update the restore-validation orchestration
in the action to call that API directly, without retaining a forwarding
duplicate.
🪄 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: 749e83f4-60cf-4834-82f5-6440c656de8a
📒 Files selected for processing (2)
src/lib/actions/sandbox/rebuild-hermes-cron-restore/backup.tstest/hermes-cron-restore-control.test.ts
| accessSync, | ||
| constants, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Move persisted backup-tree I/O into the state layer.
These imports support direct metadata and JSON reads in an action module. Keep this action as the restore-validation orchestrator, but move backup filesystem traversal and persisted-state reads to src/lib/state/** and call that API here—without retaining a forwarding duplicate.
As per path instructions, “state modules own persisted files and state I/O.”
🤖 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/actions/sandbox/rebuild-hermes-cron-restore/backup.ts` around lines 5
- 6, Remove the direct filesystem and JSON persistence handling from the backup
restore action, including the accessSync/constants imports, and move backup-tree
traversal and persisted-state reads into the appropriate state-layer API under
state modules. Update the restore-validation orchestration in the action to call
that API directly, without retaining a forwarding duplicate.
Source: Path instructions
Signed-off-by: Ho Lim <subhoya@gmail.com>
Signed-off-by: Ho Lim <subhoya@gmail.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)
agents/hermes/Dockerfile (1)
62-62: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winInclude the cron restore controller in the image integrity gate.
This security-critical controller is copied into the image and hardened, but it is not covered by the Dockerfile’s SHA-256 verification block. A modified build-context script could therefore change the fail-closed restore policy while the existing integrity checks still pass. Add a committed hash argument and
sha256sum -cverification for/usr/local/lib/nemoclaw/hermes-cron-restore-control.py.As per path instructions,
agents/**is a security boundary requiring fail-closed handling and least privilege.Also applies to: 216-217
🤖 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/Dockerfile` at line 62, Add the committed SHA-256 hash argument for hermes-cron-restore-control.py alongside the existing integrity values, and extend the Dockerfile’s verification block to run sha256sum -c against /usr/local/lib/nemoclaw/hermes-cron-restore-control.py. Ensure the check fails the build on any mismatch and retains the existing fail-closed behavior.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 `@agents/hermes/Dockerfile`:
- Line 62: Add the committed SHA-256 hash argument for
hermes-cron-restore-control.py alongside the existing integrity values, and
extend the Dockerfile’s verification block to run sha256sum -c against
/usr/local/lib/nemoclaw/hermes-cron-restore-control.py. Ensure the check fails
the build on any mismatch and retains the existing fail-closed behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a57dcd5f-7d50-4fb5-8f17-6b8d30514c7f
📒 Files selected for processing (3)
agents/hermes/Dockerfilesrc/lib/actions/sandbox/rebuild-pipeline.tssrc/lib/actions/sandbox/rebuild-preflight-phase.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/actions/sandbox/rebuild-preflight-phase.ts
Signed-off-by: Ho Lim <subhoya@gmail.com>
wscurran
added
area: sandbox
|
✨ Thanks for the PR. This fixes the cron dispatch regression by making Hermes restoration fail closed until scripts are validated against the same gateway identity. Maintainers will review the drain contract reuse, backup preservation, and test coverage for both restore paths. Related open issues: Related open issues: |
Signed-off-by: Ho Lim <subhoya@gmail.com>
apurvvkumaria
left a comment
apurvvkumaria
left a comment
There was a problem hiding this comment.
Blocking: the new Hermes restore controller can erase a pre-existing drain owned by another workflow. begin_drain unconditionally writes the nemoclaw-cron-restore marker over any active current-epoch marker, and release_drain later clears that marker without proving this operation acquired it. Reproduction: seed a current-epoch drain marker with principal operator, run begin followed by release, and observe that the operator drain is gone and cron dispatch can resume even though the operator still requires it. The focused tests pass because they model drain state as a boolean and do not cover pre-existing ownership.\n\nPlease preserve an already-active drain, record an ownership receipt or token only when this controller acquires the marker, and clear only when the current marker still matches that ownership. Add success and failure-path tests showing an operator-owned marker survives begin, release, and rollback. This is required because the current behavior breaks the supported scheduling-safety contract.
Signed-off-by: Ho Lim <subhoya@gmail.com>
apurvvkumaria
left a comment
apurvvkumaria
left a comment
There was a problem hiding this comment.
Re-reviewed the current revision after the requested drain-ownership fix. Begin now preserves a pre-existing external drain and records a random ownership token only when this controller acquires the marker. Validation and release require the current marker to match that token; release without an acquired token preserves the external marker, and rollback will not overwrite a replacement operator marker. The new tests reproduce pre-existing, replacement, and failed-release cases and confirm the operator marker survives. The prior scheduling-safety blocker is resolved, and I found no new blocking correctness, security, compatibility, or regression defect.
|
Non-blocking fast-follow recommendation: make drain marker acquisition and release compare-and-set operations. The pinned Hermes helpers currently implement unconditional replace/unlink, so an operator marker written in the narrow gaps between the controller's ownership check and its write, clear, or rollback write can still be overwritten or removed. The sequential supported-path blocker is fixed and this does not change the approval outcome. A narrowly scoped follow-up should add create-only and token-matched-clear primitives (or equivalent serialization) in Hermes, then cover concurrent operator replacement at begin, release, and rollback. |
Signed-off-by: Ho Lim <subhoya@gmail.com>
Signed-off-by: Ho Lim <subhoya@gmail.com>
4817a2f to
06a6b6e
Compare
apurvvkumaria
left a comment
apurvvkumaria
left a comment
There was a problem hiding this comment.
Approve — re-reviewed exact head 06a6b6e after the main refresh. The previously approved external-drain ownership fix is unchanged. The conflict resolution correctly composes the cron restore controller into the current Hermes runtime payload and integrity gate, including copy, root ownership and mode, committed SHA-256 verification, and layout assertions. Exact-head focused suites passed 20 of 20 tests. I also reviewed the timed-out second-opinion artifact: its duplicate-PR coordination concern and optional content-hash hardening do not demonstrate PR-attributable breakage, while live script existence and permission validation runs after restore with dispatch still drained. No blocking defect found.
|
Release-queue follow-up: approved head 06a6b6e is now DIRTY against main. Its E2E controller timed out waiting for the trusted verdict rather than reporting a branch-owned test failure. Please update from current main, resolve the conflict with a signed/verified commit, and let the required CI, advisor, and E2E evidence regenerate on the new exact head. |
Signed-off-by: Ho Lim <subhoya@gmail.com>
Summary
Make Hermes cron restoration fail closed across sandbox rebuilds. Script-backed jobs now remain undispatchable until the backed-up cron tree and profile-local scripts are restored, validated, and acknowledged against the same gateway process identity.
Related Issue
Closes #7806.
Changes
Verification
npx vitest run --project cli --project integration src/lib/actions/sandbox/rebuild-hermes-cron-restore.test.ts test/hermes-cron-restore-control.test.tsnpx vitest run --project cli --project integration src/lib/actions/sandbox/rebuild-flow.test.ts src/lib/actions/sandbox/rebuild-hermes-post-restore.test.ts test/hermes-final-image-layout.test.tsnpm run typecheck:cli -- --incrementalnpm run check:diffSigned-off-by: Ho Lim subhoya@gmail.com
Summary by CodeRabbit