feat(runtime): implement durable Docker state mutation - #8658
Conversation
Signed-off-by: Aaron Erickson <aerickson@nvidia.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:
📝 WalkthroughWalkthroughThe pull request adds Hermes runtime-state mutation control across Docker provider contracts, durable lifecycle state, startup gating, publisher execution, Shields recovery, timer authorization, and privileged execution leases. It also adds Docker authority validation, image metadata checks, integration tests, and operational documentation. ChangesHermes runtime mutation
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Shields
participant DockerProvider
participant LifecycleLedger
participant HermesPublisher
participant HermesStartup
Shields->>DockerProvider: acquire and fence runtime target
DockerProvider->>LifecycleLedger: persist intent and target claim
DockerProvider->>HermesPublisher: publish or rollback posture
HermesPublisher-->>DockerProvider: receipt and activation proof
DockerProvider->>LifecycleLedger: finalize release
HermesStartup->>HermesStartup: validate checkpoint and release
HermesStartup-->>Shields: startup readiness
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit b0bfb67 in the TypeScript / code-coverage/cliThe overall coverage in commit b0bfb67 in the Show a code coverage summary of the most impacted files.
Updated |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
PR Review Advisor — InformationalAdvisor assessment: Informational / low confidence Model lanes
Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests. E2E guidanceAdvisory only. A maintainer can dispatch the default E2E suite for the commit under review. Recommended E2E: Manual-only E2E: This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (25)
scripts/runtime_state_mutation_hermes_publisher.py (1)
844-907: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace the
locals()test with an explicit descriptor variable.Line 906 uses
"config_fd" in locals()to decide whether to close the descriptor. That works today, but it couples cleanup to a name lookup rather than to a value. A rename ofconfig_fdinside thetryblock leaks the descriptor with no diagnostic.Initialize the descriptor before the
tryblock and test the value.♻️ Proposed change
def _verify_state_posture(posture: str, plan_json: str) -> None: guard = _load_module( STATE_DIR_GUARD_PATH, "_nemoclaw_runtime_state_mutation_state_dir_guard" ) + config_fd = -1 try: plan = guard.parse_agent_state_lock_plan(plan_json) identity = guard._production_identity() config_fd = guard._open_absolute_dir_nofollow(HERMES_DIR) @@ except Exception: _fail("publisher-state-posture-invalid") finally: - if "config_fd" in locals(): - os.close(config_fd) + if config_fd >= 0: + os.close(config_fd)🤖 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 `@scripts/runtime_state_mutation_hermes_publisher.py` around lines 844 - 907, Update _verify_state_posture to initialize the configuration descriptor variable before entering the try block, then close it in finally only when its value indicates a successfully opened descriptor. Remove the "config_fd" in locals() check while preserving cleanup on all paths.src/lib/onboard/runtime-provider/state-mutation.test.ts (2)
521-528: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winThis case depends on an implicit check-ordering inside the planner.
The override sets
readOnlyRoots: ["scripts", "workspace-main"], but the fixture selectors contain no{ kind: "path", path: "workspace-main" }. Two checks can reject this input: the root-and-prefix overlap check insidenormalizeStateLockPlan, and the selector-scope check inrequireStateLockPlanScope. The test asserts the overlap message, so it passes only becausenormalizeStateLockPlanruns first.Add the matching path selector to the override. The case then isolates the overlap rule and stays correct if the planner reorders its checks.
♻️ Proposed change
[ "overlapping root and prefix policies", { readOnlyRoots: ["scripts", "workspace-main"], readOnlyPrefixes: ["workspace-"], }, /root and prefix policies must not overlap/u, ],Then extend the
it.eachbody to add the selector for the overlapping root, or move this case into its ownitthat passesselectors: [...base.selectors, { kind: "path", path: "workspace-main" }].🤖 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/onboard/runtime-provider/state-mutation.test.ts` around lines 521 - 528, Update the “overlapping root and prefix policies” case in the relevant it.each fixture to include { kind: "path", path: "workspace-main" } in its selectors, while preserving the existing readOnlyRoots, readOnlyPrefixes, and expected overlap error. Ensure the test still exercises the overlap validation in normalizeStateLockPlan rather than failing selector-scope validation in requireStateLockPlanScope.
332-463: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse field-specific diagnostics or document the limitation.
requireExactKeysreports only${label} fields are unsupported, so these assertions prove rejection but not the rejection reason. A defect that rejects an input for a different unsupported field can pass. Add the offending key to the error, or document that these tests provide rejection-only coverage.🤖 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/onboard/runtime-provider/state-mutation.test.ts` around lines 332 - 463, Update the rejection cases in the parameterized test around prepareRuntimeProviderStateMutationPlan to assert field-specific diagnostics from requireExactKeys, including the offending key in each expected error where applicable. Ensure each malformed input cannot pass because of an unrelated unsupported-field rejection; if field-specific diagnostics are not available, document that these assertions intentionally provide rejection-only coverage.Source: Path instructions
scripts/runtime-state-mutation-startup-gate.py (3)
485-498: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
raw is Noneis unreachable here.
_read_atis called withoutmissing=True, so a missing candidate raisesgate-file-missinginstead of returningNone. Theraw is Nonetest at line 497 never runs. That changes the refusal code an operator sees for a missing candidate fromrelease-candidate-mismatchtogate-file-missing.If
release-candidate-mismatchis the intended code, passmissing=True. Otherwise drop the dead test.♻️ Proposed change
raw = _read_at( directory_fd, CANDIDATE_NAME, uid=os.geteuid(), gid=os.getegid(), mode=0o600, + missing=True, )🤖 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 `@scripts/runtime-state-mutation-startup-gate.py` around lines 485 - 498, Update _verify_release_candidate’s _read_at call to pass missing=True if missing candidates must produce the existing release-candidate-mismatch refusal code; otherwise remove the unreachable raw is None check and preserve the gate-file-missing behavior.
605-638: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
directory_fdleaks when_active_existsrefuses.
_active_directory()returns an open descriptor at line 606. If_active_existsraisesGateError("active-gate-invalid"), that descriptor is never closed, because thetry/finallystarts at line 612. The process exits right aftermain(), so this does not leak in production. It does leak when the module is exercised in-process by a test or by the Dockerfile import probe.Move the
_active_existscall inside thetryblock.🤖 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 `@scripts/runtime-state-mutation-startup-gate.py` around lines 605 - 638, Move the _active_exists check in _run inside the existing try/finally block so any GateError or early inactive return still closes directory_fd. Preserve the current handling for None descriptors and the existing action flow; ensure the finally does not attempt to close a None descriptor.
336-401: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe canonical round-trip check depends on
PERMIT_KEYS,RELEASE_KEYS, andRETRY_KEYSfield order.Line 399 compares
rawagainst_canonical(normalized) + b"\n".normalizedis built in a fixed order:schemaVersion,protocol,transactionId,nonce, the protocol binding fields,start,candidateDirectory. That order currently matches each key tuple, so valid receipts pass. A future reorder of a key tuple, or of the producer's field order, silently rejects every receipt asgate-receipt-invalidand holds startup.Add a short comment that binds the two orders, or derive the emitted order from the key tuple.
🤖 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 `@scripts/runtime-state-mutation-startup-gate.py` around lines 336 - 401, Update _binding so the canonical round-trip validation remains explicitly tied to the ordering defined by PERMIT_KEYS, RELEASE_KEYS, and RETRY_KEYS. Prefer deriving normalized field emission from the selected keys, or add a concise comment documenting that normalized’s construction order must match each protocol key tuple; preserve the existing receipt validation behavior.src/lib/actions/sandbox/snapshot-restore-test-fixture.ts (1)
276-277: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTwo hand-maintained copies of the
timer-controlmock surface. The productiontimer-controlmodule gainedisProcessAliveandreadProcessStartIdentity, and both test entry points now mirror that surface by hand. A future export must be added to both files, and a missed site fails only when that path is exercised.
src/lib/actions/sandbox/snapshot-restore-test-fixture.ts#L276-L277: export a single sharedtimer-controlmock factory from this fixture, and restore both new implementations inresetSnapshotRestoreMocks.src/lib/actions/sandbox/snapshot-auto-create-failure.test.ts#L100-L104: consume that shared factory in thevi.mockcall instead of restating the three functions.🤖 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/snapshot-restore-test-fixture.ts` around lines 276 - 277, The timer-control mock surface is duplicated across two test entry points. In src/lib/actions/sandbox/snapshot-restore-test-fixture.ts#L276-L277, export one shared timer-control mock factory, include both isProcessAlive and readProcessStartIdentity in resetSnapshotRestoreMocks, and in src/lib/actions/sandbox/snapshot-auto-create-failure.test.ts#L100-L104 use that factory from the vi.mock call instead of restating the three mocked functions.src/lib/adapters/sandbox/command-transport.test.ts (1)
75-77: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
vi.resetAllMocks()is redundant with the project-level isolation.The
cliVitest project importstest/helpers/vitest-state-isolation.tsand enablesclearMocks,restoreMocks,unstubEnvs, andunstubGlobals. Every dependency mock in this file is also rebuilt per test bycreateDependencies(), and the hoisted mocks receive a freshmockImplementationin each test that uses them. This hook adds no isolation.Remove it, or keep it only if a specific hoisted mock must lose its implementation between tests.
Based on the learning that Vitest files under
srcrun in thecliproject withclearMocks,restoreMocks,unstubEnvs, andunstubGlobalsalready enabled, so suite hooks should only clean up resources Vitest does not manage.🤖 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/adapters/sandbox/command-transport.test.ts` around lines 75 - 77, Remove the redundant beforeEach hook calling vi.resetAllMocks() from this test suite. Rely on the cli Vitest project’s configured isolation and the per-test createDependencies() and mockImplementation setup; retain a suite hook only if a specific hoisted mock requires cleanup beyond Vitest’s managed resets.Source: Learnings
src/lib/onboard/runtime-provider/docker-operation-authority.test.ts (1)
29-32: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the redundant
vi.unstubAllEnvs()call.The
cliVitest project enablesunstubEnvs, so Vitest restores stubbed environment variables between tests. Keep only the temporary-directory cleanup in this hook.Based on learnings: Vitest test files under
srcare executed by thecliVitest project, which enablesclearMocks,restoreMocks,unstubEnvs, andunstubGlobals; suite-level teardown should only clean resources Vitest does not manage.🤖 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/onboard/runtime-provider/docker-operation-authority.test.ts` around lines 29 - 32, Remove the redundant vi.unstubAllEnvs() call from the afterEach hook in the docker-operation authority tests, leaving only the temporary-directory cleanup for roots.Source: Learnings
src/lib/onboard/runtime-provider/state-mutation.ts (1)
416-434: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
selectorIdentityinstead of repeating its expression.
selectorIdentityis defined at Line 211. The samepath:/prefix:expression is written again at Lines 421-423 and Lines 483-484. Three copies can drift, and the identity string is the deduplication and scope-check key.♻️ Proposed refactor
const identities = new Set( - selectors.map((selector) => - selector.kind === "path" ? `path:${selector.path}` : `prefix:${selector.prefix}`, - ), + selectors.map(selectorIdentity), );for (const selector of selectors) { - const identity = - selector.kind === "path" ? `path:${selector.path}` : `prefix:${selector.prefix}`; - uniqueSelectors.set(identity, selector); + uniqueSelectors.set(selectorIdentity(selector), selector); }Also applies to: 481-486
🤖 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/onboard/runtime-provider/state-mutation.ts` around lines 416 - 434, Update requireStateLockPlanScope and the other affected scope-check logic to reuse the existing selectorIdentity helper instead of rebuilding path:/prefix: identity strings inline. Replace both selector and required-root/prefix identity construction with selectorIdentity-compatible inputs, preserving the current deduplication and exact-selector validation behavior.src/lib/onboard/runtime-provider/docker-operation-authority.ts (1)
425-432: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffConfirm the per-command context re-inspection cost is acceptable.
The context-branch guard calls
resolvedDockerContextEndpointon every guarded command. That runs an extradocker context inspectchild process before each capture and each spawn. Lifecycle flows that issue many commands therefore double their Docker subprocess count. TheDOCKER_HOSTbranch has no equivalent cost because its guard only re-verifies the executable.If the re-check must stay on every command, consider a short time-bounded cache of the qualified endpoint so a burst of commands re-inspects once.
🤖 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/onboard/runtime-provider/docker-operation-authority.ts` around lines 425 - 432, Reduce the per-command Docker context re-inspection overhead in the guard around executable.guard and resolvedDockerContextEndpoint. Add a short-lived, time-bounded cache for the qualified context endpoint so bursts of guarded commands reuse one recent inspection while still detecting endpoint changes promptly; preserve the existing retry error when a fresh validation differs from qualifiedEndpoint.src/lib/onboard/runtime-provider/docker-llama-cpp-operation.ts (1)
68-73: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winApply the same argument validation on the injected spawner path.
authority.spawnvalidates arguments withfixedDockerSpawnArgumentsand pinscwd,env, andshell: false. ThespawnCommandbranch only prefixes the endpoint arguments. An injected spawner therefore receives unvalidated arguments and no fixed environment. The parameter is reachable from the exportedcreateDockerLlamaCppHostLocalOperation, so the two paths should enforce the same bound.Consider exporting a validation helper from
docker-operation-authority.tsand calling it beforespawnCommand, or restrictingspawnCommandto tests only.🤖 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/onboard/runtime-provider/docker-llama-cpp-operation.ts` around lines 68 - 73, Update the injected spawner branch in the operation’s spawn method to apply the same fixedDockerSpawnArguments validation and pinned cwd, env, and shell:false settings used by authority.spawn. Reuse or export the existing validation helper from docker-operation-authority.ts, then pass its normalized arguments and options to spawnCommand while preserving assertAuthority and endpoint argument prefixing.src/lib/shields/flow.test.ts (1)
31-49: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the shared timer authorization proof helper.
test/helpers/hermes-shields-provider-consumer-harness.tsalready exportswriteTimerAuthorizationProof, andsrc/lib/shields/legacy-hermes-compat.test.tsimports it. This local copy builds the identical proof payload. Two copies of the same security fixture will drift when the proof schema changes, and one suite will then silently stop proving authorization.Import the exported helper and pass
requireDistas itsloadSourceargument, or move the shared implementation to a helper both suites import.🤖 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/flow.test.ts` around lines 31 - 49, Remove the local writeTimerAuthorizationProof implementation and import the shared helper from test/helpers/hermes-shields-provider-consumer-harness.ts. Update its call site to pass requireDist as the loadSource argument, preserving the existing authorization-proof setup behavior.src/lib/shields/legacy-hermes-compat.test.ts (2)
655-656: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the bare block around the new
describe.The
describe("Hermes Shields runtime-provider consumer", …)block is wrapped in a bare block statement{ … }. The block creates a lexical scope that nothing in the file uses, and its indentation suggests the suite is nested inside the preceding suite when it is not.♻️ Proposed change
-{ - describe("Hermes Shields runtime-provider consumer", () => { +describe("Hermes Shields runtime-provider consumer", () => {- }); -} +});Re-indent the enclosed body by two spaces.
Also applies to: 1454-1455
🤖 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/legacy-hermes-compat.test.ts` around lines 655 - 656, Remove the unnecessary bare block surrounding the `describe("Hermes Shields runtime-provider consumer", …)` suite and the corresponding occurrence near the later test section. Re-indent each enclosed test body by two spaces while leaving the `describe` suites and test behavior unchanged.
1002-1091: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSplit the timer-replacement case out of the forward-policy parameterization.
This
it.eachcovers two unrelated failures.missingandtamperedassert that nothing runs.timer-replacedasserts the opposite:runSpyandtransitionSpyare called,routeSpyruns once, and the transition file is removed. The arrange step also branches at Line 1059, and the expected error at Line 1076 is an alternation of two different messages, so the test passes for either failure text.The title claims the recovered forward policy is invalid, which does not describe the timer-replacement case. Split
timer-replacedinto its ownitwith a single exact error expectation, and keep theit.eachformissingandtamperedwith their shared assertions.As per path instructions: "Flag copied production algorithms, broad mocks that bypass the behavior under test, and conditionals that make a test pass without exercising its claim."
🤖 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/legacy-hermes-compat.test.ts` around lines 1002 - 1091, Split the timer-replaced scenario out of the parameterized test around the failureMode setup. Keep the it.each limited to missing and tampered, with shared assertions that no recovery actions run and an exact forward-policy error expectation; add a dedicated test for timer replacement with its specific arrangement, exact auto-restore-authority/timer-generation error expectation, and assertions for runSpy, transitionSpy, routeSpy, and transition cleanup. Remove the conditional setup and alternated expectation from the shared test.Source: Path instructions
src/lib/sandbox/agent-config.ts (1)
67-72: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign the error text with the condition it detects.
agentis loaded fromsandbox.agenton the preceding line, so this branch does not detect a concurrent change. It detects a manifest whosenamefield differs from the requested agent name. The current message will send an operator looking for a race.♻️ Proposed wording change
const agent = resolveCurrentAgentDefinition(sandbox.agent); if (agent.name !== sandbox.agent) { throw new Error( - `Sandbox '${sandboxName}' agent-definition authority changed during resolution`, + `Sandbox '${sandboxName}' resolved agent manifest '${agent.name}' does not match its registered agent '${sandbox.agent}'`, ); }🤖 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/sandbox/agent-config.ts` around lines 67 - 72, Update the error message in the resolveCurrentAgentDefinition validation to describe a manifest name mismatch: report that the resolved agent definition’s name differs from the requested sandbox.agent value, rather than attributing it to an authority change during resolution.src/lib/sandbox/privileged-exec.test.ts (1)
105-117: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the real directory constant in the mock.
PERSISTED_ENGINE_LIFECYCLE_DIRECTORYis duplicated as a string literal here, and the tests at Line 241 and Line 270 create the directory with the same literal. If the real constant changes,assertNoActiveStateMutationTargetwill look at a different path, thelstatSyncguard will return early, and these tests will pass without exercising the gate.Import the constant from
../onboard/runtime-provider/persisted-engine-lifecyclebefore the module is replaced in the require cache, and reuse it in both the mock and the directory setup.🤖 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/sandbox/privileged-exec.test.ts` around lines 105 - 117, Update the privileged-exec test setup to import PERSISTED_ENGINE_LIFECYCLE_DIRECTORY from ../onboard/runtime-provider/persisted-engine-lifecycle before replacing that module in requireCache, then reuse the imported constant in the mocked exports and both directory-creation setups instead of duplicated string literals.src/lib/shields/index.ts (2)
2781-2796: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winProvider posture repair discards the first attempt's error in both directions. Both the unlock and the lock provider branches run a same-posture transition, verify the live posture, and on failure retry with the opposite rollback. Each catch block discards the original error, so the reason the live posture disagreed with the state-derived authority is never reported.
src/lib/shields/index.ts#L2781-L2796: record the caught verification error withconsole.errorbefore running themutable/lockedrepair transition.src/lib/shields/index.ts#L3229-L3239: record the caught error fromverifyProviderLockedPosturewithconsole.errorbefore running thelocked/mutablerepair transition.🤖 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 2781 - 2796, The caught verification errors are discarded before posture repair transitions. In src/lib/shields/index.ts lines 2781-2796, update the catch around verifyHermesProviderMutablePosture to pass the caught error to console.error before runHermesProviderProtectionTransition performs the mutable/locked repair; likewise, in lines 3229-3239, log the caught verifyProviderLockedPosture error with console.error before the locked/mutable repair transition.
4819-4825: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the unreachable
timerAuthorityguards.
FreshShieldsDownTimerStartdeclarestransitionandtimerAuthorityas required, andstartFreshShieldsDownTimereither returns both or throws. After the destructuring at Line 4819 both values are always defined.The guards at Line 4823, Line 4846, Line 4948, Line 4981, Line 5014, and Line 5016 therefore never take their false branch. The throw at Line 5017 ("Fresh Shields down lost its timer authorization proof") is unreachable. The dead branches make the control flow in this function harder to audit than the invariant it enforces.
Drop the conditionals and call
assertFreshShieldsDownAuthoritydirectly.♻️ Proposed simplification
- let { transition } = timerStart; - const { timerAuthority, policyPathForApply } = timerStart; + let transition: ShieldsDownTransition = timerStart.transition; + const { timerAuthority, policyPathForApply } = timerStart; try { - if (transition && timerAuthority) { - assertFreshShieldsDownAuthority(sandboxName, timerAuthority, transition, "preparing"); - } + assertFreshShieldsDownAuthority(sandboxName, timerAuthority, transition, "preparing");- if (!timerAuthority) { - throw new Error("Fresh Shields down lost its timer authorization proof"); - } assertFreshShieldsDownAuthority(sandboxName, timerAuthority, transition, "preparing");Based on learnings: "avoid adding 'defensive' error handling (e.g., try/catch wrappers, fallbacks, or extra validation) around internal helper logic when there is no realistic throwing path or failure mode."
Also applies to: 5016-5019
🤖 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 4819 - 4825, Remove the unreachable timerAuthority and transition guards in the Fresh Shields down timer flow. In the function containing the destructuring of timerStart, call assertFreshShieldsDownAuthority directly at the guarded sites, including the paths around lines 4846, 4948, 4981, 5014, and 5016, and delete the unreachable “lost its timer authorization proof” throw while preserving the existing authorization arguments and control flow.Source: Learnings
src/lib/onboard/runtime-provider/persisted-engine-lifecycle.ts (1)
696-746: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider limiting the repair scan to publication paths.
readPrivateFilenow callsrepairInterruptedExclusivePublicationon every read. That adds onereaddirSyncplus twolstatSynccalls per candidate entry for each private-file read.loadTransactionreads several phase files, the intent, the release receipt, and lease files, andlistUnfinishedTransactionsrepeats that for every transaction directory and every claim. The directories are small today, so this is not a correctness problem, but the cost grows with the number of unfinished transactions.Consider running the repair once per directory per store operation, or only from the paths that can observe a post-link crash window (
loadPhase, claim loading, and receipt loading), instead of from the generic read helper.🤖 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/onboard/runtime-provider/persisted-engine-lifecycle.ts` around lines 696 - 746, Limit repairInterruptedExclusivePublication to publication-sensitive loading paths instead of invoking it from the generic readPrivateFile helper. Remove the unconditional repair call from readPrivateFile, then invoke repair once per directory or only within loadPhase, claim-loading, and receipt-loading flows, ensuring each store operation preserves interrupted-publication recovery without rescanning for every private-file read.test/helpers/shields-flow-harness.ts (2)
236-247: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBoth harnesses copy the production timer-authorization proof serialization.
src/lib/shields/timer-control.tsexportswriteTimerAuthorizationProofForMarker, but that helper requiresmarker.pid === process.pid, so neither harness can call it for a simulated timer. Each harness therefore re-implements the proof payload by hand. The shared root cause is the missing reusable payload builder intimer-control.ts. If a field is added toTimerAuthorizationProof,hasExactTimerAuthorizationProofstarts rejecting harness-written proofs and every dependent suite fails for a reason unrelated to the behavior under test.Export a pure
buildTimerAuthorizationProof(marker): TimerAuthorizationProoffromsrc/lib/shields/timer-control.ts, havewriteTimerAuthorizationProofForMarkeruse it, then call it from both harnesses.
test/helpers/shields-flow-harness.ts#L236-L247: replace the inline object literal passed tofs.writeFileSyncwithJSON.stringify(timerControl.buildTimerAuthorizationProof(marker)).test/helpers/hermes-shields-provider-consumer-harness.ts#L120-L131: replace the inline object literal inwriteTimerAuthorizationProofwithJSON.stringify(timerControl.buildTimerAuthorizationProof(marker)).As per path instructions, tests must be reviewed for behavioral confidence rather than implementation lock-in, and copied production algorithms must be flagged.
🤖 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/shields-flow-harness.ts` around lines 236 - 247, Extract a pure buildTimerAuthorizationProof(marker): TimerAuthorizationProof helper in src/lib/shields/timer-control.ts, and make writeTimerAuthorizationProofForMarker reuse it. In test/helpers/shields-flow-harness.ts lines 236-247 and test/helpers/hermes-shields-provider-consumer-harness.ts lines 120-131, replace each duplicated proof object with JSON.stringify(timerControl.buildTimerAuthorizationProof(marker)); review tests for behavioral confidence without coupling them to the implementation.Source: Path instructions
429-457: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the second-occurrence failure semantics.
Both injection branches fail on the second matching rename, not the first. The
++stateWrites === 2and++transitionWrites === 2conditions encode that rule as a bare literal. A reader cannot tell which write is the first one and which one is the target.Add a short comment that names the first rename and the targeted rename. That prevents a future change to the write sequence from silently retargeting the injected failure.
♻️ Proposed change
const originalRenameSync = fs.renameSync.bind(fs); + // The first rename to each destination is the initial shields-down write. + // The failure is injected on the second rename, which is the policy + // rejection rollback that the tests assert against. let stateWrites = 0; let transitionWrites = 0;🤖 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/shields-flow-harness.ts` around lines 429 - 457, Add concise comments in the renameSync mock branches for statePath and transitionPrefix documenting that the first matching rename succeeds and the second matching rename is the injected failure. Keep the existing ++stateWrites === 2 and ++transitionWrites === 2 behavior unchanged.src/lib/shields/timer-control.ts (1)
352-360: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winThread the process-inspection deadline into the new start-identity check.
verifyTimerMarkerIdentitycallsreadProcessStartIdentity(marker.pid)without a deadline. That call starts a fresh inspection budget and can fall back toexecFileSync("ps", ...)when/procis unavailable.Two consequences appear on the authority path in
src/lib/shields/index.ts:
isExactLiveFutureTimerAuthority(Lines 221-233) builds one deadline withprocessInspectionDeadlineAfter(...)and passes it toisProcessAliveandreadProcessStartIdentity. It then callsverifyTimerMarkerIdentity(marker), which escapes that budget and can spawnpsa second time.- The same call site already compared
readProcessStartIdentity(marker.pid, deadline)againstmarker.timerProcessStartIdentity. The new block repeats that identical comparison.Add an optional
deadlineparameter toverifyTimerMarkerIdentityand forward it to bothreadProcessStartIdentityandreadProcessCommandLine. The callers can then bound the total inspection cost.♻️ Proposed change
-function verifyTimerMarkerIdentity(marker: ShieldsTimerMarker): { +function verifyTimerMarkerIdentity( + marker: ShieldsTimerMarker, + deadline = processInspectionDeadline(), +): { verified: boolean; warning?: string; } { if ( marker.timerProcessStartIdentity !== undefined && - readProcessStartIdentity(marker.pid) !== marker.timerProcessStartIdentity + readProcessStartIdentity(marker.pid, deadline) !== marker.timerProcessStartIdentity ) {Run the following script to list every caller and check whether a deadline is already in scope:
#!/bin/bash set -euo pipefail rg -nP --type=ts -C4 '\bverifyTimerMarkerIdentity\s*\(' src test rg -nP --type=ts -C2 '\bprocessInspectionDeadline(After)?\s*\(' src/lib/shields🤖 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/timer-control.ts` around lines 352 - 360, Update verifyTimerMarkerIdentity to accept an optional deadline and pass it to both readProcessStartIdentity and readProcessCommandLine. Update every caller, especially isExactLiveFutureTimerAuthority, to forward the existing process-inspection deadline so all checks share one bounded budget; avoid repeating the already-performed start-identity comparison when applicable.test/runtime-state-mutation-hermes-publisher.test.ts (1)
282-289: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRun the Python harness once in
beforeAll.Each of the three tests calls
runHarness(), and every call re-runs all three scenario blocks of the harness. The harness is deterministic and produces one result map that contains the keys for all three tests. The sibling suitetest/runtime-state-mutation-control.test.tsalready usesbeforeAllfor the same pattern at lines 1193-1200. Hoisting removes two redundant Python runs, each with a 20-second timeout budget.♻️ Proposed refactor to run the harness once
-function runHarness(): Record<string, unknown> { +let harnessResult: Record<string, unknown>; + +beforeAll(() => { const result = spawnSync("python3", ["-I", "-c", HARNESS, PUBLISHER, STATE_PLAN], { encoding: "utf8", timeout: 20_000, }); expect(result.status, result.stderr).toBe(0); - return JSON.parse(result.stdout) as Record<string, unknown>; -} + harnessResult = JSON.parse(result.stdout) as Record<string, unknown>; +});Update the import on line 7 to include
beforeAll, then replace eachconst result = runHarness();with a reference toharnessResult.Also applies to: 292-293, 320-321, 333-334
🤖 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/runtime-state-mutation-hermes-publisher.test.ts` around lines 282 - 289, Run runHarness() once in a beforeAll hook, storing its result in a shared harnessResult variable, and import beforeAll. Replace each test’s local runHarness() invocation with harnessResult while preserving the existing key-specific assertions.test/runtime-state-mutation-control.test.ts (1)
15-1189: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftConsider extracting the embedded Python harness to a fixture file.
HARNESSholds about 1170 lines of Python inside a TypeScript template literal. No Python linter, formatter, or type checker inspects it. Escaping rules also force workarounds such as thedollarvariable at line 1064. Moving the harness to a fixture file, for exampletest/fixtures/runtime-state-mutation-control-harness.py, and passing the path tospawnSyncwould restore Python tooling and simplify the string handling. The same applies to the smaller harnesses in the sibling runtime-state-mutation suites.🤖 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/runtime-state-mutation-control.test.ts` around lines 15 - 1189, Extract the embedded HARNESS Python template and the smaller harnesses in the sibling runtime-state-mutation suites into dedicated fixture files under test/fixtures, preserving their behavior and imports. Update the corresponding test setup and spawnSync invocations to pass each fixture path instead of constructing Python source from template literals, and remove string-escaping workarounds such as the dollar variable.
🤖 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/start.sh`:
- Around line 71-89: The startup loop around
nemoclaw_runtime_state_mutation_gate must distinguish an active-mutation retry
from gate inspection failures. Update the gate result contract and its handling
in main() so retry-wait retains the one-second retry behavior, while
GateError/OSError or other refusal cases return a separate status that logs an
accurate inspection-failure message instead of repeatedly claiming an active
mutation.
In `@scripts/runtime_state_mutation_hermes_publisher.py`:
- Line 71: Add a CI test that loads the Hermes AgentDefinition via
loadAgent("hermes"), derives selectors from its configPaths including
shieldsFiles, and compares the resulting set with the publisher contract
represented by TOP_SELECTORS. Remove independent selector literals from the test
and fail clearly when the sets diverge, including
publisher-plan-selector-mismatch coverage.
In `@src/lib/onboard/runtime-provider/docker-operation-authority.ts`:
- Around line 151-164: Update fixedDockerCommandEnvironment to read PATH
exclusively from the supplied env object, removing the fallback to
process.env.PATH. Preserve the existing missing-PATH error so curated
environments fail closed without mixing ambient and caller-provided values.
In `@src/lib/onboard/runtime-provider/docker-state-mutation.test.ts`:
- Around line 560-563: Remove await from synchronous state-mutation calls
acquire, assertFenced, publish, rollback, activate, release, and recover, and
remove async from the enclosing it callbacks at the referenced tests. Preserve
the existing assertions and test behavior while ensuring these tests directly
exercise the synchronous contract.
- Around line 102-363: Remove conditional statements from all three affected
test files to satisfy the growth guardrails: in
src/lib/onboard/runtime-provider/docker-state-mutation.test.ts lines 102-363,
move harness, acquireMarker, ownerThatStopsAfterPrepare, and
replayDeferredAcquire into a dedicated fixture module and make the afterHelper
behavior a fixture option; in
src/lib/onboard/runtime-provider/docker-operation-authority.test.ts lines 34-48,
replace args.includes("inspect") branches in contextCapture and the inline
capture with precomputed responses or a command-keyed lookup; in
src/lib/shields/hermes-runtime-state-mutation.test.ts lines 296-344, use it.each
data columns for failure-stage overrides and a scripted-result queue helper for
the call-counter behavior at the cited sites.
In `@src/lib/onboard/runtime-provider/docker-state-mutation.ts`:
- Around line 174-184: Remove the VITEST/NEMOCLAW_TEST_STATE_DIR branch from
resolveDockerStateMutationStateDir so production state always resolves through
resolveShieldsStateDir. Update tests, including the environment-branch case in
docker-state-mutation.test.ts, to provide the temporary state directory via
DockerStateMutationSurfaceOptions.resolveStateDir instead.
In `@src/lib/onboard/runtime-provider/persisted-engine-lifecycle.ts`:
- Around line 1882-1904: Update the privileged-exec consumer to import the
persisted-engine mutation gate through the runtime-provider/access module,
matching access.ts’s re-export, instead of binding directly from
persisted-engine-lifecycle.ts. Preserve use of
hasActivePersistedEngineStateMutationTarget while ensuring the Hermes harness
spy can intercept it.
In `@src/lib/sandbox/privileged-exec-exclusion.test.ts`:
- Around line 41-47: Remove all four if statements from the privileged-exec test
file, including the conditionals in waitForExit and the setup/teardown sections.
Preserve the existing behavior by moving conditional handling into a shared
helper under test/helpers, or replace the guards with the documented
unconditional operations where those operations are safe.
In `@src/lib/shields/index.ts`:
- Around line 5422-5429: Update the temporarily_unlocked handling around
assertTimedAuthority so a legacy timed-DOWN state with a valid marker but no
transition artifact is migrated before provider-state-mutation-v2 is enabled, or
accepted through a bounded compatibility path. Preserve strict validation for
missing/invalid markers and non-active transitions, and ensure shields status no
longer reports this legacy state as drifted solely because transition is null.
In `@src/lib/shields/timer.ts`:
- Around line 204-205: Cache the current process start identity once at module
scope or on first use, then update markerRecordMatchesCurrentTimer to compare
against that cached value instead of calling
readProcessStartIdentity(process.pid) on every invocation. Preserve the existing
undefined-identity matching behavior and retain the comparison for markers with
a stored identity.
In `@test/runtime-state-mutation-control.test.ts`:
- Around line 905-906: Update the waitpid check in the guardian writer-stop
assertion to poll until the stop is reported or the established deadline
expires, matching the existing polling pattern in the last-resort block. Keep
using WUNTRACED|WNOHANG and set results["guardian_writer_stopped"] from the
final selected PID and WIFSTOPPED(status) result.
---
Nitpick comments:
In `@scripts/runtime_state_mutation_hermes_publisher.py`:
- Around line 844-907: Update _verify_state_posture to initialize the
configuration descriptor variable before entering the try block, then close it
in finally only when its value indicates a successfully opened descriptor.
Remove the "config_fd" in locals() check while preserving cleanup on all paths.
In `@scripts/runtime-state-mutation-startup-gate.py`:
- Around line 485-498: Update _verify_release_candidate’s _read_at call to pass
missing=True if missing candidates must produce the existing
release-candidate-mismatch refusal code; otherwise remove the unreachable raw is
None check and preserve the gate-file-missing behavior.
- Around line 605-638: Move the _active_exists check in _run inside the existing
try/finally block so any GateError or early inactive return still closes
directory_fd. Preserve the current handling for None descriptors and the
existing action flow; ensure the finally does not attempt to close a None
descriptor.
- Around line 336-401: Update _binding so the canonical round-trip validation
remains explicitly tied to the ordering defined by PERMIT_KEYS, RELEASE_KEYS,
and RETRY_KEYS. Prefer deriving normalized field emission from the selected
keys, or add a concise comment documenting that normalized’s construction order
must match each protocol key tuple; preserve the existing receipt validation
behavior.
In `@src/lib/actions/sandbox/snapshot-restore-test-fixture.ts`:
- Around line 276-277: The timer-control mock surface is duplicated across two
test entry points. In
src/lib/actions/sandbox/snapshot-restore-test-fixture.ts#L276-L277, export one
shared timer-control mock factory, include both isProcessAlive and
readProcessStartIdentity in resetSnapshotRestoreMocks, and in
src/lib/actions/sandbox/snapshot-auto-create-failure.test.ts#L100-L104 use that
factory from the vi.mock call instead of restating the three mocked functions.
In `@src/lib/adapters/sandbox/command-transport.test.ts`:
- Around line 75-77: Remove the redundant beforeEach hook calling
vi.resetAllMocks() from this test suite. Rely on the cli Vitest project’s
configured isolation and the per-test createDependencies() and
mockImplementation setup; retain a suite hook only if a specific hoisted mock
requires cleanup beyond Vitest’s managed resets.
In `@src/lib/onboard/runtime-provider/docker-llama-cpp-operation.ts`:
- Around line 68-73: Update the injected spawner branch in the operation’s spawn
method to apply the same fixedDockerSpawnArguments validation and pinned cwd,
env, and shell:false settings used by authority.spawn. Reuse or export the
existing validation helper from docker-operation-authority.ts, then pass its
normalized arguments and options to spawnCommand while preserving
assertAuthority and endpoint argument prefixing.
In `@src/lib/onboard/runtime-provider/docker-operation-authority.test.ts`:
- Around line 29-32: Remove the redundant vi.unstubAllEnvs() call from the
afterEach hook in the docker-operation authority tests, leaving only the
temporary-directory cleanup for roots.
In `@src/lib/onboard/runtime-provider/docker-operation-authority.ts`:
- Around line 425-432: Reduce the per-command Docker context re-inspection
overhead in the guard around executable.guard and resolvedDockerContextEndpoint.
Add a short-lived, time-bounded cache for the qualified context endpoint so
bursts of guarded commands reuse one recent inspection while still detecting
endpoint changes promptly; preserve the existing retry error when a fresh
validation differs from qualifiedEndpoint.
In `@src/lib/onboard/runtime-provider/persisted-engine-lifecycle.ts`:
- Around line 696-746: Limit repairInterruptedExclusivePublication to
publication-sensitive loading paths instead of invoking it from the generic
readPrivateFile helper. Remove the unconditional repair call from
readPrivateFile, then invoke repair once per directory or only within loadPhase,
claim-loading, and receipt-loading flows, ensuring each store operation
preserves interrupted-publication recovery without rescanning for every
private-file read.
In `@src/lib/onboard/runtime-provider/state-mutation.test.ts`:
- Around line 521-528: Update the “overlapping root and prefix policies” case in
the relevant it.each fixture to include { kind: "path", path: "workspace-main" }
in its selectors, while preserving the existing readOnlyRoots, readOnlyPrefixes,
and expected overlap error. Ensure the test still exercises the overlap
validation in normalizeStateLockPlan rather than failing selector-scope
validation in requireStateLockPlanScope.
- Around line 332-463: Update the rejection cases in the parameterized test
around prepareRuntimeProviderStateMutationPlan to assert field-specific
diagnostics from requireExactKeys, including the offending key in each expected
error where applicable. Ensure each malformed input cannot pass because of an
unrelated unsupported-field rejection; if field-specific diagnostics are not
available, document that these assertions intentionally provide rejection-only
coverage.
In `@src/lib/onboard/runtime-provider/state-mutation.ts`:
- Around line 416-434: Update requireStateLockPlanScope and the other affected
scope-check logic to reuse the existing selectorIdentity helper instead of
rebuilding path:/prefix: identity strings inline. Replace both selector and
required-root/prefix identity construction with selectorIdentity-compatible
inputs, preserving the current deduplication and exact-selector validation
behavior.
In `@src/lib/sandbox/agent-config.ts`:
- Around line 67-72: Update the error message in the
resolveCurrentAgentDefinition validation to describe a manifest name mismatch:
report that the resolved agent definition’s name differs from the requested
sandbox.agent value, rather than attributing it to an authority change during
resolution.
In `@src/lib/sandbox/privileged-exec.test.ts`:
- Around line 105-117: Update the privileged-exec test setup to import
PERSISTED_ENGINE_LIFECYCLE_DIRECTORY from
../onboard/runtime-provider/persisted-engine-lifecycle before replacing that
module in requireCache, then reuse the imported constant in the mocked exports
and both directory-creation setups instead of duplicated string literals.
In `@src/lib/shields/flow.test.ts`:
- Around line 31-49: Remove the local writeTimerAuthorizationProof
implementation and import the shared helper from
test/helpers/hermes-shields-provider-consumer-harness.ts. Update its call site
to pass requireDist as the loadSource argument, preserving the existing
authorization-proof setup behavior.
In `@src/lib/shields/index.ts`:
- Around line 2781-2796: The caught verification errors are discarded before
posture repair transitions. In src/lib/shields/index.ts lines 2781-2796, update
the catch around verifyHermesProviderMutablePosture to pass the caught error to
console.error before runHermesProviderProtectionTransition performs the
mutable/locked repair; likewise, in lines 3229-3239, log the caught
verifyProviderLockedPosture error with console.error before the locked/mutable
repair transition.
- Around line 4819-4825: Remove the unreachable timerAuthority and transition
guards in the Fresh Shields down timer flow. In the function containing the
destructuring of timerStart, call assertFreshShieldsDownAuthority directly at
the guarded sites, including the paths around lines 4846, 4948, 4981, 5014, and
5016, and delete the unreachable “lost its timer authorization proof” throw
while preserving the existing authorization arguments and control flow.
In `@src/lib/shields/legacy-hermes-compat.test.ts`:
- Around line 655-656: Remove the unnecessary bare block surrounding the
`describe("Hermes Shields runtime-provider consumer", …)` suite and the
corresponding occurrence near the later test section. Re-indent each enclosed
test body by two spaces while leaving the `describe` suites and test behavior
unchanged.
- Around line 1002-1091: Split the timer-replaced scenario out of the
parameterized test around the failureMode setup. Keep the it.each limited to
missing and tampered, with shared assertions that no recovery actions run and an
exact forward-policy error expectation; add a dedicated test for timer
replacement with its specific arrangement, exact
auto-restore-authority/timer-generation error expectation, and assertions for
runSpy, transitionSpy, routeSpy, and transition cleanup. Remove the conditional
setup and alternated expectation from the shared test.
In `@src/lib/shields/timer-control.ts`:
- Around line 352-360: Update verifyTimerMarkerIdentity to accept an optional
deadline and pass it to both readProcessStartIdentity and
readProcessCommandLine. Update every caller, especially
isExactLiveFutureTimerAuthority, to forward the existing process-inspection
deadline so all checks share one bounded budget; avoid repeating the
already-performed start-identity comparison when applicable.
In `@test/helpers/shields-flow-harness.ts`:
- Around line 236-247: Extract a pure buildTimerAuthorizationProof(marker):
TimerAuthorizationProof helper in src/lib/shields/timer-control.ts, and make
writeTimerAuthorizationProofForMarker reuse it. In
test/helpers/shields-flow-harness.ts lines 236-247 and
test/helpers/hermes-shields-provider-consumer-harness.ts lines 120-131, replace
each duplicated proof object with
JSON.stringify(timerControl.buildTimerAuthorizationProof(marker)); review tests
for behavioral confidence without coupling them to the implementation.
- Around line 429-457: Add concise comments in the renameSync mock branches for
statePath and transitionPrefix documenting that the first matching rename
succeeds and the second matching rename is the injected failure. Keep the
existing ++stateWrites === 2 and ++transitionWrites === 2 behavior unchanged.
In `@test/runtime-state-mutation-control.test.ts`:
- Around line 15-1189: Extract the embedded HARNESS Python template and the
smaller harnesses in the sibling runtime-state-mutation suites into dedicated
fixture files under test/fixtures, preserving their behavior and imports. Update
the corresponding test setup and spawnSync invocations to pass each fixture path
instead of constructing Python source from template literals, and remove
string-escaping workarounds such as the dollar variable.
In `@test/runtime-state-mutation-hermes-publisher.test.ts`:
- Around line 282-289: Run runHarness() once in a beforeAll hook, storing its
result in a shared harnessResult variable, and import beforeAll. Replace each
test’s local runHarness() invocation with harnessResult while preserving the
existing key-specific assertions.
🪄 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: aab0811b-7317-4b94-966e-af305a9f4bc2
📒 Files selected for processing (65)
agents/hermes/Dockerfileagents/hermes/runtime-state-mutation-publisher-v1.jsonagents/hermes/start.shscripts/runtime-state-mutation-control.pyscripts/runtime-state-mutation-startup-gate.pyscripts/runtime_state_mutation_hermes_publisher.pysrc/lib/actions/sandbox/process-recovery-temp-ssh.test.tssrc/lib/actions/sandbox/process-recovery.tssrc/lib/actions/sandbox/snapshot-auto-create-failure.test.tssrc/lib/actions/sandbox/snapshot-restore-test-fixture.tssrc/lib/adapters/sandbox/command-transport.test.tssrc/lib/adapters/sandbox/command-transport.tssrc/lib/onboard/runtime-provider/access.tssrc/lib/onboard/runtime-provider/contract.tssrc/lib/onboard/runtime-provider/docker-llama-cpp-operation.tssrc/lib/onboard/runtime-provider/docker-operation-authority.test.tssrc/lib/onboard/runtime-provider/docker-operation-authority.tssrc/lib/onboard/runtime-provider/docker-state-mutation.test.tssrc/lib/onboard/runtime-provider/docker-state-mutation.tssrc/lib/onboard/runtime-provider/docker.tssrc/lib/onboard/runtime-provider/persisted-engine-lifecycle.test.tssrc/lib/onboard/runtime-provider/persisted-engine-lifecycle.tssrc/lib/onboard/runtime-provider/registry.tssrc/lib/onboard/runtime-provider/runtime-provider-contract.test.tssrc/lib/onboard/runtime-provider/state-mutation.test.tssrc/lib/onboard/runtime-provider/state-mutation.tssrc/lib/sandbox/agent-config.tssrc/lib/sandbox/config.tssrc/lib/sandbox/privileged-exec-exclusion.test.tssrc/lib/sandbox/privileged-exec.test.tssrc/lib/sandbox/privileged-exec.tssrc/lib/shields/flow.test.tssrc/lib/shields/hermes-runtime-state-mutation.test.tssrc/lib/shields/hermes-runtime-state-mutation.tssrc/lib/shields/index.test.tssrc/lib/shields/index.tssrc/lib/shields/legacy-hermes-compat.test.tssrc/lib/shields/mutable-config-repair.tssrc/lib/shields/openclaw-transition.test.tssrc/lib/shields/policy-transition.test.tssrc/lib/shields/timer-bound-lock.test.tssrc/lib/shields/timer-bound-lock.tssrc/lib/shields/timer-control.tssrc/lib/shields/timer-process.test.tssrc/lib/shields/timer.tssrc/lib/shields/transition-lock.tssrc/lib/state/mcp-lifecycle-lock/shields-timer-authority.tstest/config-set-nested-ssrf.test.tstest/config-set-prompt-error.test.tstest/helpers/hermes-shields-provider-consumer-harness.tstest/helpers/shields-flow-harness.tstest/hermes-config-transaction-wiring.test.tstest/hermes-final-image-layout.test.tstest/hermes-gateway-supervisor-recovery.test.tstest/hermes-start.test.tstest/package-contract/cli/config-set-prompt-eof.test.tstest/repro-2681-group-writable.test.tstest/runtime-provider-source-shape.test.tstest/runtime-state-mutation-control.test.tstest/runtime-state-mutation-hermes-publisher.test.tstest/runtime-state-mutation-startup-gate.test.tstest/sandbox-provisioning.test.tstest/sandbox-rlimit-hooks.test.tstest/shields-up-runtime-perms.test.tstest/support/hermes-shell-harness.ts
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
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)
scripts/runtime-state-mutation-startup-gate.py (1)
443-489: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPrevent concurrent publication from replacing a conflicting payload.
Each function checks for an existing file before it calls
os.replace. Two concurrent writers can both observe no file. The lateros.replacethen silently replaces the first payload. This bypassescandidate-conflictorretry-ack-conflictand can persist a receipt that does not match the first validated binding.Use a no-replace publication primitive, or serialize writers. On a destination collision, reread the final file and accept only byte-identical content.
scripts/runtime-state-mutation-startup-gate.py#L443-L489: publishstartup-complete.jsonwithout replacing an existing candidate.scripts/runtime-state-mutation-startup-gate.py#L555-L611: publishretry-ack.jsonwithout replacing an existing acknowledgement.🤖 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 `@scripts/runtime-state-mutation-startup-gate.py` around lines 443 - 489, Prevent concurrent publication races in the startup-complete.json path at scripts/runtime-state-mutation-startup-gate.py:443-489 and the retry-ack.json path at scripts/runtime-state-mutation-startup-gate.py:555-611. Replace the unconditional os.replace publication with a no-replace or serialized operation; when publication collides, reread the final file and accept only byte-identical payloads, otherwise invoke the existing candidate-conflict or retry-ack-conflict handling. Update both affected publication flows consistently.
🧹 Nitpick comments (1)
test/helpers/docker-state-mutation-harness.ts (1)
345-355: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
replayDeferredAcquireskips the response hooks that the capture path applies.The capture path runs
options.afterHelperand thelostAcquireResponsesRemainingcounter after it builds an acquire receipt.replayDeferredAcquirereproduces onlyacquireMarker. If a test combinesdeferAcquireOncewithafterHelperorloseAcquireResponseOnce, the replayed acquire and the captured acquire diverge, and the test asserts against a state the real helper path never produces.Route the replay through the same tail as the capture path.
♻️ Proposed refactor
const replayDeferredAcquire = () => { if (deferredAcquireRequest === null) throw new Error("No deferred acquire request exists."); const serializedRequest = deferredAcquireRequest; deferredAcquireRequest = null; helperActions.push("acquire"); acquireRequests.push(serializedRequest); const request = JSON.parse(serializedRequest) as Record<string, unknown>; const conflict = acquireMarker(request); if (conflict) throw new Error(conflict.stderr); + options.afterHelper?.("acquire", state); return marker; };🤖 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/docker-state-mutation-harness.ts` around lines 345 - 355, Update replayDeferredAcquire to execute the same post-receipt tail as the capture path, including options.afterHelper and lostAcquireResponsesRemaining handling after acquireMarker succeeds. Reuse the existing capture-path helper or shared logic rather than duplicating behavior, while preserving the deferred request reset and conflict validation.
🤖 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 `@scripts/runtime-state-mutation-startup-gate.py`:
- Around line 443-489: Prevent concurrent publication races in the
startup-complete.json path at
scripts/runtime-state-mutation-startup-gate.py:443-489 and the retry-ack.json
path at scripts/runtime-state-mutation-startup-gate.py:555-611. Replace the
unconditional os.replace publication with a no-replace or serialized operation;
when publication collides, reread the final file and accept only byte-identical
payloads, otherwise invoke the existing candidate-conflict or retry-ack-conflict
handling. Update both affected publication flows consistently.
---
Nitpick comments:
In `@test/helpers/docker-state-mutation-harness.ts`:
- Around line 345-355: Update replayDeferredAcquire to execute the same
post-receipt tail as the capture path, including options.afterHelper and
lostAcquireResponsesRemaining handling after acquireMarker succeeds. Reuse the
existing capture-path helper or shared logic rather than duplicating behavior,
while preserving the deferred request reset and conflict validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8b8e63ec-5ba1-41f6-9446-1068fa8a40c5
📒 Files selected for processing (22)
scripts/runtime-state-mutation-control.pyscripts/runtime-state-mutation-startup-gate.pysrc/lib/onboard/runtime-provider/docker-operation-authority.test.tssrc/lib/onboard/runtime-provider/docker-operation-authority.tssrc/lib/onboard/runtime-provider/docker-state-mutation.test.tssrc/lib/onboard/runtime-provider/persisted-engine-lifecycle.test.tssrc/lib/onboard/runtime-provider/state-mutation.test.tssrc/lib/sandbox/privileged-exec-exclusion.test.tssrc/lib/sandbox/privileged-exec.test.tssrc/lib/shields/flow.test.tssrc/lib/shields/hermes-runtime-state-mutation.test.tssrc/lib/shields/legacy-hermes-compat.test.tstest/helpers/docker-operation-authority-test-helpers.tstest/helpers/docker-state-mutation-harness.tstest/helpers/hermes-runtime-state-mutation-test-helpers.tstest/helpers/hermes-shields-provider-consumer-harness.tstest/helpers/privileged-exec-test-helpers.tstest/helpers/runtime-provider-state-mutation-test-helpers.tstest/helpers/shields-flow-harness.tstest/hermes-doctor-config-hash.test.tstest/runtime-state-mutation-control.test.tstest/runtime-state-mutation-startup-gate.test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- src/lib/sandbox/privileged-exec-exclusion.test.ts
- src/lib/sandbox/privileged-exec.test.ts
- src/lib/shields/hermes-runtime-state-mutation.test.ts
- src/lib/onboard/runtime-provider/state-mutation.test.ts
- test/runtime-state-mutation-control.test.ts
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
|
🌿 Preview your docs: https://nvidia-preview-pr-8658.docs.buildwithfern.com/nemoclaw |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/lib/onboard/runtime-provider/docker-operation-authority.ts (2)
264-271: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRequire a regular file in
executableCandidate.
fs.accessSync(candidate, fs.constants.X_OK)also succeeds for a directory that carries the execute bit. A directory namedsshordocker-credential-*onPATHis therefore selected, andqualifiedExecutableMetadatathen throws "Docker operation executable is not one regular file." That aborts all Docker authority qualification instead of skipping the entry.Align the candidate filter with the later requirement.
♻️ Proposed fix
function executableCandidate(candidate: string): boolean { try { + if (!fs.statSync(candidate).isFile()) return false; fs.accessSync(candidate, fs.constants.X_OK); return true; } catch { return false; } }🤖 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/onboard/runtime-provider/docker-operation-authority.ts` around lines 264 - 271, Update executableCandidate to require that candidate is a regular file in addition to having execute permission, using filesystem stat information; return false for directories or other non-regular entries so qualifiedExecutableMetadata can skip them.
572-576: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winPer-command guards repeat full qualification. The binding guard re-runs every qualification step on each guarded Docker command, so filesystem scans and an extra Docker invocation are paid per command.
src/lib/onboard/runtime-provider/docker-operation-authority.ts#L572-L576: bound the re-qualification, for example with a freshness window or one re-qualification per operation, instead of runningexecutable.guard(),delegatedCommands.guard(), andendpointGuard()on each command.src/lib/onboard/runtime-provider/docker-operation-authority.ts#L618-L634: reuse the bounded result for the context re-inspection, and report an inspection failure separately from an identity mismatch.🤖 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/onboard/runtime-provider/docker-operation-authority.ts` around lines 572 - 576, In docker-operation-authority.ts, update the binding guard around the guard callback at lines 572-576 to bound re-qualification instead of invoking executable.guard(), delegatedCommands.guard(), and endpointGuard() for every command, using a freshness window or once-per-operation result. At lines 618-634, reuse that bounded qualification result for context re-inspection and distinguish inspection failures from identity mismatches in the reported outcome.src/lib/onboard/runtime-provider/docker-operation-authority.test.ts (1)
187-209: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSplit the two independent rejection claims into separate tests.
This test asserts two unrelated rejections: relative
PATHentries and env-based delegated interpreters. A failure in either assertion reports the same test name, so the failing claim is not identifiable from the report. Twoitblocks give exact failure attribution and let each claim fail independently.🤖 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/onboard/runtime-provider/docker-operation-authority.test.ts` around lines 187 - 209, Split the combined test into two independent it blocks: keep the relative PATH rejection assertion in one, and move the credential-helper setup and env-based delegated interpreter assertion into another. Give each test a specific name identifying its rejection behavior, while preserving the existing inputs and expected errors.
🤖 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/inference/llama-cpp/host-local-runtime.ts`:
- Around line 374-386: Require and validate bindings.hostPort in
buildLlamaCppRequestGuardDockerArgv before constructing the --publish argument,
failing closed when it is undefined instead of emitting an empty host-port
field. Preserve the existing validated-port publication format for defined
hostPort values.
In `@test/helpers/privileged-exec-test-helpers.ts`:
- Around line 43-47: Update releaseAndStopChild so failures from
createReleaseMarker do not bypass child termination: preserve the marker error,
attempt the existing conditional SIGKILL and waitForChildExit cleanup, then
rethrow the original error after cleanup completes.
- Around line 33-37: Update the release block around createReleaseMarker so
released is assigned true only after marker creation completes successfully.
Keep the existing guard and ensure a failure from createReleaseMarker leaves
released false, allowing later calls to retry marker creation.
---
Nitpick comments:
In `@src/lib/onboard/runtime-provider/docker-operation-authority.test.ts`:
- Around line 187-209: Split the combined test into two independent it blocks:
keep the relative PATH rejection assertion in one, and move the
credential-helper setup and env-based delegated interpreter assertion into
another. Give each test a specific name identifying its rejection behavior,
while preserving the existing inputs and expected errors.
In `@src/lib/onboard/runtime-provider/docker-operation-authority.ts`:
- Around line 264-271: Update executableCandidate to require that candidate is a
regular file in addition to having execute permission, using filesystem stat
information; return false for directories or other non-regular entries so
qualifiedExecutableMetadata can skip them.
- Around line 572-576: In docker-operation-authority.ts, update the binding
guard around the guard callback at lines 572-576 to bound re-qualification
instead of invoking executable.guard(), delegatedCommands.guard(), and
endpointGuard() for every command, using a freshness window or
once-per-operation result. At lines 618-634, reuse that bounded qualification
result for context re-inspection and distinguish inspection failures from
identity mismatches in the reported outcome.
🪄 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: 55d0c51a-ae1f-4a08-ba46-f04d9d0a54dd
📒 Files selected for processing (20)
agents/hermes/Dockerfiledocs/inference/set-up-llama-cpp.mdxdocs/manage-sandboxes/runtime-controls.mdxdocs/reference/commands.mdxdocs/reference/host-files-and-state.mdxdocs/reference/troubleshooting.mdxdocs/security/tcb-boundary.mdxscripts/checks/run-llama-cpp-dgx-spark-qualification.mtsscripts/install.shsrc/lib/inference/llama-cpp/host-local-runtime.test.tssrc/lib/inference/llama-cpp/host-local-runtime.tssrc/lib/onboard/runtime-provider/docker-operation-authority.test.tssrc/lib/onboard/runtime-provider/docker-operation-authority.tssrc/lib/sandbox/privileged-exec-exclusion.test.tssrc/lib/shields/hermes-runtime-state-mutation.test.tssrc/lib/shields/hermes-runtime-state-mutation.tstest/helpers/hermes-shields-provider-consumer-harness.tstest/helpers/privileged-exec-test-helpers.tstest/install-preflight.test.tstest/llama-cpp-dgx-spark-qualification-runner.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- agents/hermes/Dockerfile
- src/lib/shields/hermes-runtime-state-mutation.ts
- src/lib/shields/hermes-runtime-state-mutation.test.ts
- test/helpers/hermes-shields-provider-consumer-harness.ts
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
✅ Action performedReview finished.
|
|
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: 6
🧹 Nitpick comments (19)
src/lib/shields/legacy-hermes-compat.test.ts (4)
1341-1346: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the capability-probe absence predicate.
The same inline
os.lstatprobe predicate is repeated in four tests. A shared helper, for exampleexpectNoCapabilityProbe(commands), removes the duplication and keeps the intent in one place.Also applies to: 1364-1369, 1414-1419, 1439-1444
🤖 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/legacy-hermes-compat.test.ts` around lines 1341 - 1346, Extract the repeated inline predicate checking that no command contains the “-c” os.lstat capability probe into a shared helper such as expectNoCapabilityProbe(commands). Replace the four duplicated assertions in the affected tests with calls to this helper, preserving the existing false-expectation behavior.
715-715: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the bare block around the new
describe.
describealready creates its own scope. The wrapping block at Line 715 and Line 1457 adds indentation without value.Also applies to: 1456-1457
🤖 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/legacy-hermes-compat.test.ts` at line 715, Remove the unnecessary bare block surrounding the new describe section in the legacy Hermes compatibility tests, including the matching closing block at the corresponding end. Keep the describe suite and its contents unchanged while adjusting indentation to match the surrounding file.
1030-1032: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename the parameterized title so it covers the timer fixture.
The
timer-replacedfixture rejects a replaced auto-restore timer authority, not an invalid forward policy. The current title prints "the recovered forward policy is timer-replaced", which misstates the behavior under test.♻️ Proposed change
- )("fails closed when the recovered forward policy is %s", (_failureMode, arrangeFailure, expectedError, assertSideEffects) => { + )("fails closed on recovered timed DOWN when %s", (_failureMode, arrangeFailure, expectedError, assertSideEffects) => {As per coding guidelines: "Use behavior-oriented test titles".
🤖 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/legacy-hermes-compat.test.ts` around lines 1030 - 1032, Update the parameterized test title in the it.each block for forwardPolicyFailureFixtures so it describes the recovered authorization/restore state or failure condition rather than specifically calling every case an invalid forward policy. Ensure the timer-replaced fixture is accurately represented while preserving the existing parameterized cases and assertions.Source: Coding guidelines
889-897: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the phase assertions out of the mock implementations.
routeSpyandauditSpythrow assertion errors insideshields.shieldsDown. If the production path catches, wraps, or rethrows a different error, the ordering claim is not proven and the test can still pass. The same pattern exists at Lines 1002-1008.Record the observed phase in the mock and assert after the call.
♻️ Proposed refactor
+ const observedPhases: Record<string, string> = {}; routeSpy.mockImplementation(() => { - expect(JSON.parse(fs.readFileSync(transitionPath, "utf-8")).phase).toBe("preparing"); + observedPhases.route = JSON.parse(fs.readFileSync(transitionPath, "utf-8")).phase; events.push("route"); return { ok: true, attempts: 1, httpStatus: 200 }; }); auditSpy.mockImplementation(() => { - expect(JSON.parse(fs.readFileSync(transitionPath, "utf-8")).phase).toBe("active"); + observedPhases.audit = JSON.parse(fs.readFileSync(transitionPath, "utf-8")).phase; events.push("audit"); });Then assert after
shields.shieldsDown(...):expect(observedPhases).toEqual({ route: "preparing", audit: "active" });As per path instructions: "Flag copied production algorithms, broad mocks that bypass the behavior under test, and conditionals that make a test pass without exercising its claim."
🤖 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/legacy-hermes-compat.test.ts` around lines 889 - 897, Move the phase assertions out of the routeSpy and auditSpy mock implementations in both affected test sections. Have each mock record the phase observed from transitionPath, then assert the collected phases after shields.shieldsDown completes, preserving the expected route="preparing" and audit="active" ordering without assertions being swallowed by production error handling.Source: Path instructions
scripts/runtime-state-mutation-startup-gate.py (1)
618-624: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueClose
directory_fdif_active_existsrefuses.
_active_existscan raiseGateErrorfor an invalidactive.json. Thetry/finallystarts only at Line 625, so the directory descriptor stays open on that path. The process exits soon after, so the effect is bounded, but the guard is simple.♻️ Proposed fix
def _run(action: str) -> str: directory_fd = _active_directory() - if not _active_exists(directory_fd): - if directory_fd is not None: - os.close(directory_fd) - return "inactive" - assert directory_fd is not None + try: + active = _active_exists(directory_fd) + except BaseException: + if directory_fd is not None: + os.close(directory_fd) + raise + if not active: + if directory_fd is not None: + os.close(directory_fd) + return "inactive" + assert directory_fd is not None try:🤖 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 `@scripts/runtime-state-mutation-startup-gate.py` around lines 618 - 624, Ensure _run closes directory_fd when _active_exists raises GateError, before the existing try/finally cleanup scope begins; preserve the current inactive return behavior and avoid closing a nonexistent descriptor.test/runtime-state-mutation-hermes-publisher.test.ts (2)
282-289: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRun the Python harness once for all three tests.
runHarnessspawnspython3and executes the complete harness, which builds every result key. Lines 293, 321, and 334 each call it again. The three tests therefore repeat the same work three times, with a 20-second timeout each.Compute the result once in
beforeAlland read the shared value in each test.♻️ Proposed fix
+let harness: Record<string, unknown>; + describe("Hermes runtime state mutation publisher", () => { + beforeAll(() => { + harness = runHarness(); + }); + it("publishes and rolls back only the exact installed full plan (`#7744`)", () => { - const result = runHarness(); + const result = harness;Import
beforeAllfromvitestand apply the same change in the other two tests.Also applies to: 292-293, 320-321, 333-334
🤖 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/runtime-state-mutation-hermes-publisher.test.ts` around lines 282 - 289, Update the runtime-state mutation tests to execute runHarness once in a beforeAll hook, storing its Record<string, unknown> result in shared test-scope state; have all three tests read that value instead of calling runHarness individually, and import beforeAll from vitest.
371-378: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the source-text assertions with behavior assertions.
Lines 372-375 assert on the raw text of
runtime-state-mutation-startup-gate.py, and Lines 376-378 assert on the raw text of the publisher.test/runtime-state-mutation-startup-gate.test.tsalready proves thepermitted,activation-ready, andretryexit codes by executing the gate. Source-text assertions lock the implementation and pass even if the constants are unused.Keep the
start.shwiring assertions, which check shell wiring that has no other observable boundary here. Drop the duplicated gate and publisher constant checks.As per path instructions: "Prefer observable outcomes through the public boundary over source-text, private-shape, or mock-call assertions."
🤖 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/runtime-state-mutation-hermes-publisher.test.ts` around lines 371 - 378, In the runtime-state-mutation Hermes publisher test, remove the raw source-text assertions for the startup gate’s DURABLE_DIRECTORY and exit-code constants and the publisher’s PYTHON_PATH constant. Retain the existing start.sh wiring assertions, relying on the executable gate behavior covered by runtime-state-mutation-startup-gate.test.ts for observable exit-code validation.Source: Path instructions
test/runtime-state-mutation-startup-gate.test.ts (1)
83-93: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd a negative test for
gate-start-mismatch.The harness fixes
_capture_parentto a constantstartand writes permits that always carry the same value. No case writes a permit whosestartdiffers from the captured parent.gate-start-mismatchat_bindingis the control that binds the permit to the exactnemoclaw-startparent process, and it is the primary guarantee of this helper.Add one permit with a modified
startfield and assert the refusal.As per path instructions: "Flag ... conditionals that make a test pass without exercising its claim."
🧪 Sketch of the added harness case
write(os.path.join(durable, gate.PERMIT_NAME), permit, 0o444) results["admitted"] = gate._run("admit") + + foreign = {**permit, "start": {**start, "pid": 42}} + write(os.path.join(durable, gate.PERMIT_NAME), foreign, 0o444) + results["foreign_start"] = code(lambda: gate._run("admit")) + write(os.path.join(durable, gate.PERMIT_NAME), permit, 0o444)Then assert
foreign_start: "gate-start-mismatch"in thetoMatchObjectblock.🤖 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/runtime-state-mutation-startup-gate.test.ts` around lines 83 - 93, Extend the startup-gate harness around `_run("admit")` to write a permit whose `start` differs from the constant captured by `_capture_parent`, then assert admission is refused with `foreign_start: "gate-start-mismatch"` in the existing result match. Keep the existing matching-start case unchanged so the test covers both the valid binding and the mismatch path in `_binding`.Source: Path instructions
scripts/runtime_state_mutation_hermes_publisher.py (1)
806-809: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConvert
os.statfailures to the fixed publisher error code.Lines 806-807 call
os.statoutside atry. If/sandboxor/sandbox/.hermesis missing or unreadable, anOSErrorescapesapply_plan_posture. Every other failure path in this module raisesPublisherErrorwith a fixed, non-sensitive code. Keep the error contract uniform for the controller.♻️ Proposed fix
- parent = os.stat(os.path.dirname(HERMES_DIR), follow_symlinks=False) - root = os.stat(HERMES_DIR, follow_symlinks=False) + try: + parent = os.stat(os.path.dirname(HERMES_DIR), follow_symlinks=False) + root = os.stat(HERMES_DIR, follow_symlinks=False) + except OSError: + _fail("publisher-top-posture-invalid") if not stat.S_ISDIR(parent.st_mode) or not stat.S_ISDIR(root.st_mode): _fail("publisher-top-posture-invalid")🤖 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 `@scripts/runtime_state_mutation_hermes_publisher.py` around lines 806 - 809, Wrap the os.stat calls in the posture validation block around parent and root in exception handling, converting any OSError into the fixed publisher error code via _fail("publisher-top-posture-invalid"). Preserve the existing directory-mode checks and ensure no raw filesystem exception escapes apply_plan_posture.test/helpers/shields-flow-harness.ts (1)
113-136: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse one authorization-proof writer.
writeShieldsTimerAuthorizationProofat Lines 113-136 and the inline block at Lines 259-273 build the same proof payload. The two copies differ only in which token and sandbox name they read. If the proof schema gains a field, one copy can drift and a harness path will silently produce an invalid proof.Extract the payload construction into one function that accepts the marker and the process token.
♻️ Sketch of the shared writer
+function writeProof( + timerControl: typeof import("../../src/lib/shields/timer-control.js"), + marker: { pid: number; sandboxName: string; timerProcessStartIdentity: string }, + sandboxName: string, + processToken: string, +): void { + fs.writeFileSync( + timerControl.timerAuthorizationProofPath(sandboxName, processToken), + JSON.stringify({ + schemaVersion: 1, + pid: marker.pid, + sandboxName, + processToken, + timerProcessStartIdentity: marker.timerProcessStartIdentity, + authoritySha256: timerControl.timerAuthoritySha256(marker), + }), + { mode: 0o600 }, + ); +}Then call
writeProof(...)from bothwriteShieldsTimerAuthorizationProofand thesendhandler.Also applies to: 259-273
🤖 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/shields-flow-harness.ts` around lines 113 - 136, Extract the duplicated authorization-proof payload and file-writing logic into a shared `writeProof` helper that accepts the timer marker and process token. Update both `writeShieldsTimerAuthorizationProof` and the `send` handler’s inline proof-writing block to call `writeProof`, preserving each caller’s existing sandbox and token selection while keeping schema fields and file permissions centralized.src/lib/onboard/runtime-provider/persisted-engine-lifecycle.test.ts (1)
69-76: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the claim path from production code instead of recomputing the digest.
runtimeTargetClaimPathreimplements the target-claim identity assha256("mxc" \0 targetId). If production changes that derivation, these tests stop pointing at the real claim file. Assertions such asexpect(fs.existsSync(runtimeTargetClaimPath(...))).toBe(false)would then pass for the wrong reason.Export a path helper from
persisted-engine-lifecycle.tsand call it here, so the test tracks the real layout.As per path instructions: "Flag copied production algorithms, broad mocks that bypass the behavior under test, and conditionals that make a test pass without exercising its claim."
🤖 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/onboard/runtime-provider/persisted-engine-lifecycle.test.ts` around lines 69 - 76, Replace the duplicated SHA-256 derivation in runtimeTargetClaimPath with the exported production path helper from persisted-engine-lifecycle.ts. Update the test to call that helper so claim-file assertions follow the real production layout and identity derivation.Source: Path instructions
src/lib/onboard/runtime-provider/runtime-provider-contract.test.ts (1)
475-501: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd validation cases for
acquire,assertFenced,activate, andrecover. The registry validates all seven v2 operations, but this table covers only three.🤖 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/onboard/runtime-provider/runtime-provider-contract.test.ts` around lines 475 - 501, Extend the parameterized validation test around createRuntimeProviderBundleRegistry to cover the remaining stateMutation v2 operations acquire, assertFenced, activate, and recover alongside the existing entries. Preserve the current deletion and expected error-message assertion for each operation.src/lib/onboard/runtime-provider/persisted-engine-lifecycle.ts (4)
1580-1593: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the execution-authority predicate into one function.
Lines 1580-1593 in
acquireMutationExecutionand Lines 1677-1690 inassertMutationExecutioncompute the same acceptance condition. Both derivependingStateMutationReleaseandpreparedStateMutation, then apply the same composite test. Only the failure message differs.This predicate decides whether a caller may execute against a durable mutation. Two copies can drift, and a divergence would let
acquireMutationExecutiongrant a lease thatassertMutationExecutionlater rejects mid-mutation, or the reverse.Extract one helper that takes the loaded record and returns the decision, then call it from both sites with the site-specific message.
Also applies to: 1677-1690
🤖 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/onboard/runtime-provider/persisted-engine-lifecycle.ts` around lines 1580 - 1593, The execution-authority predicate is duplicated between acquireMutationExecution and assertMutationExecution. Extract a shared helper that accepts the loaded mutation record, computes pendingStateMutationRelease and preparedStateMutation, and returns the complete acceptance decision; replace both inline conditions with calls to this helper while preserving each site’s existing failure message.
1333-1385: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftShare one implementation of the abandoned-lease recovery protocol.
recoverExecutionArtifactsForRetirementrepeats the recovery sequence thatacquireMutationExecutionperforms at Lines 1607-1663: load the recovery marker, reject a live owner, publish an exclusive recovery marker, re-check liveness, remove the abandoned lease, then remove the recovery marker in afinallyblock.This sequence is what prevents two processes from owning one mutation. Two copies can drift. A correction to the ordering or to the liveness re-check in one copy would not reach the other.
Extract the shared loop into one helper and call it from both sites.
🤖 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/onboard/runtime-provider/persisted-engine-lifecycle.ts` around lines 1333 - 1385, Extract the duplicated abandoned-lease recovery loop into a shared helper, preserving its marker loading, transaction validation, live-owner rejection, exclusive publication, liveness re-check, lease removal, and finally-based marker cleanup. Replace the recovery sequence in both recoverExecutionArtifactsForRetirement and acquireMutationExecution with calls to that helper, keeping each caller’s surrounding behavior unchanged.
1000-1018: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared private-directory preamble.
Lines 1004-1015 duplicate the directory resolution,
lstatSync, symlink/directory check, andverifyPrivateDirectorycall thatloadTransactionperforms at Lines 1023-1034. The two copies are identical.This preamble is the private-directory security check for the ledger. If one copy is relaxed later, the intent loader reads from a directory that the other loader would reject. Extract one helper that returns the verified directory or
null, then use it in both loaders.♻️ Proposed refactor
+function verifiedTransactionDirectory(root: string, transactionId: string): string | null { + const directory = transactionDirectory(root, transactionId); + let metadata: fs.Stats; + try { + metadata = fs.lstatSync(directory); + } catch (error) { + if ((error as NodeJS.ErrnoException).code === "ENOENT") return null; + throw error; + } + if (!metadata.isDirectory() || metadata.isSymbolicLink()) { + fail("transaction path must be a private real directory"); + } + verifyPrivateDirectory(directory); + return directory; +} + function loadStateMutationIntentFromRoot( root: string, transactionId: string, ): PersistedEngineStateMutationIntent | null { - const directory = transactionDirectory(root, transactionId); - let metadata: fs.Stats; - try { - metadata = fs.lstatSync(directory); - } catch (error) { - if ((error as NodeJS.ErrnoException).code === "ENOENT") return null; - throw error; - } - if (!metadata.isDirectory() || metadata.isSymbolicLink()) { - fail("transaction path must be a private real directory"); - } - verifyPrivateDirectory(directory); + const directory = verifiedTransactionDirectory(root, transactionId); + if (directory === null) return null; return loadStateMutationIntentArtifact(directory, transactionId); }🤖 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/onboard/runtime-provider/persisted-engine-lifecycle.ts` around lines 1000 - 1018, Extract the duplicated transaction-directory validation from loadStateMutationIntentFromRoot and loadTransaction into a shared helper that resolves the transaction path, returns null for ENOENT, rejects non-directory or symlink paths, and calls verifyPrivateDirectory before returning the verified directory. Replace both loaders’ inline preambles with this helper while preserving their existing artifact-loading behavior.
198-200: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winUse one canonical state-mutation plan size bound.
Export
MAX_PLAN_BYTESfromstate-mutation.tsand import it inpersisted-engine-lifecycle.ts. Both checks apply to the sameserializedPlanUTF-8 byte limit. Otherwise, future changes can makeprepareRuntimeProviderStateMutationPlanaccept a plan that intent publication rejects.🤖 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/onboard/runtime-provider/persisted-engine-lifecycle.ts` around lines 198 - 200, Use the canonical MAX_PLAN_BYTES exported by state-mutation.ts in persisted-engine-lifecycle.ts, importing it and removing the duplicate MAX_STATE_MUTATION_PLAN_BYTES constant. Update the serializedPlan byte-limit check in prepareRuntimeProviderStateMutationPlan and the intent publication validation to use this shared bound, while leaving the intent and nonce limits unchanged.src/lib/onboard/runtime-provider/state-mutation.ts (1)
420-424: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
selectorIdentityfor the two inline identity expressions.
selectorIdentityat Line 211 already builds thepath:/prefix:identity. Lines 420-424 and Lines 483-486 repeat that expression. Three copies of the identity scheme can drift. If the namespace changes in one place,requireStateLockPlanScopecan pass against a stale namespace while the dedup map uses the new one.♻️ Proposed refactor
const identities = new Set( - selectors.map((selector) => - selector.kind === "path" ? `path:${selector.path}` : `prefix:${selector.prefix}`, - ), + selectors.map((selector) => selectorIdentity(selector)), );const uniqueSelectors = new Map<string, RuntimeProviderStateMutationSelector>(); for (const selector of selectors) { - const identity = - selector.kind === "path" ? `path:${selector.path}` : `prefix:${selector.prefix}`; - uniqueSelectors.set(identity, selector); + uniqueSelectors.set(selectorIdentity(selector), selector); }Also applies to: 481-486
🤖 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/onboard/runtime-provider/state-mutation.ts` around lines 420 - 424, Replace the inline path/prefix identity expressions in the Set construction and the corresponding logic around the second occurrence with calls to the existing selectorIdentity helper. Preserve the current deduplication behavior while ensuring both sites use the centralized identity scheme.src/lib/onboard/runtime-provider/docker-llama-cpp-operation.ts (1)
28-40: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse shared error types or exported prefixes for Docker error translation.
docker-operation-authority.tsproduces both"Docker operation "and"Docker context endpoint changed "errors. A wording change can bypass translation and expose the raw Docker error. Export the prefixes or use a typed error withinstanceof.🤖 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/onboard/runtime-provider/docker-llama-cpp-operation.ts` around lines 28 - 40, Update withManagedLlamaCppError to rely on shared exported Docker error prefixes or a shared typed error from docker-operation-authority.ts instead of duplicating message-string checks. Ensure both Docker operation and Docker context endpoint changed errors continue translating to Managed llama.cpp errors even if their wording changes.test/helpers/privileged-exec-test-helpers.ts (1)
23-41: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExpress the shared-buffer requirement in the
waitBuffertype.
Atomics.waitrequires anInt32Arraybacked by aSharedArrayBuffer. TypeScript6.0.3supports this generic type and rejects an ordinaryInt32Arrayat the call site.♻️ Proposed refactor
export function createOrdinaryExecReleaseSleeper( releasePath: string, - waitBuffer: Int32Array, + waitBuffer: Int32Array<SharedArrayBuffer>, ): {🤖 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/privileged-exec-test-helpers.ts` around lines 23 - 41, Update createOrdinaryExecReleaseSleeper so waitBuffer is typed as an Int32Array backed by SharedArrayBuffer, satisfying Atomics.wait’s shared-buffer requirement while preserving the existing sleep and release behavior.
🤖 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/inference/llama-cpp/host-local-runtime.ts`:
- Around line 350-370: Bind authority consumption to issued-object identity: in
qualifyDockerLoopbackPublishAuthority, register each returned authority in a
private WeakSet, and in consumeDockerLoopbackPublishAuthority require membership
before marking it consumed, rejecting branded clones; update
test/llama-cpp-dgx-spark-qualification-runner.test.ts lines 146-155 to add a
regression test confirming a cloned valid authority is rejected.
In `@src/lib/onboard/runtime-provider/persisted-engine-lifecycle.ts`:
- Around line 2349-2361: Update all three
releaseCompletedPersistedEngineStateMutation signatures to return
PersistedEngineStateMutationReleaseResult<T> instead of
PersistedEngineStateMutationResult<T>. In the finalized recovery branch, remove
the `as T` cast from the undefined value so the optional value is represented by
the result type.
In `@test/runtime-state-mutation-control.test.ts`:
- Around line 1031-1043: Reset deadline to a fresh two-second monotonic deadline
immediately before the last-resort waitpid loop that monitors writer_pid and
WIFSTOPPED(status). Keep the existing parked_path deadline and result assignment
unchanged, matching the guardian block’s reset behavior.
- Around line 1226-1233: Increase the spawnSync timeout in the beforeAll harness
setup from 15 seconds to accommodate the harness’s bounded waits under loaded CI
runners, and raise the corresponding Vitest testTimeout for this suite so the
test framework allows the same longer execution window.
- Around line 868-892: Update both forked child branches, including the
writer/controller branches and the children in the last-resort block, to wrap
their child-only logic in try/finally and call os._exit() from finally. Ensure
exceptions or loop termination cannot unwind the parent’s TemporaryDirectory
teardown, while preserving each child’s existing setup and behavior.
In `@test/support/hermes-shell-harness.ts`:
- Around line 99-111: Update the prelude extraction logic using the startScript
source and start/end marker indices to throw an error when either indexOf call
returns -1, before calling slice. Preserve the existing replacement behavior
only when both markers are found.
---
Nitpick comments:
In `@scripts/runtime_state_mutation_hermes_publisher.py`:
- Around line 806-809: Wrap the os.stat calls in the posture validation block
around parent and root in exception handling, converting any OSError into the
fixed publisher error code via _fail("publisher-top-posture-invalid"). Preserve
the existing directory-mode checks and ensure no raw filesystem exception
escapes apply_plan_posture.
In `@scripts/runtime-state-mutation-startup-gate.py`:
- Around line 618-624: Ensure _run closes directory_fd when _active_exists
raises GateError, before the existing try/finally cleanup scope begins; preserve
the current inactive return behavior and avoid closing a nonexistent descriptor.
In `@src/lib/onboard/runtime-provider/docker-llama-cpp-operation.ts`:
- Around line 28-40: Update withManagedLlamaCppError to rely on shared exported
Docker error prefixes or a shared typed error from docker-operation-authority.ts
instead of duplicating message-string checks. Ensure both Docker operation and
Docker context endpoint changed errors continue translating to Managed llama.cpp
errors even if their wording changes.
In `@src/lib/onboard/runtime-provider/persisted-engine-lifecycle.test.ts`:
- Around line 69-76: Replace the duplicated SHA-256 derivation in
runtimeTargetClaimPath with the exported production path helper from
persisted-engine-lifecycle.ts. Update the test to call that helper so claim-file
assertions follow the real production layout and identity derivation.
In `@src/lib/onboard/runtime-provider/persisted-engine-lifecycle.ts`:
- Around line 1580-1593: The execution-authority predicate is duplicated between
acquireMutationExecution and assertMutationExecution. Extract a shared helper
that accepts the loaded mutation record, computes pendingStateMutationRelease
and preparedStateMutation, and returns the complete acceptance decision; replace
both inline conditions with calls to this helper while preserving each site’s
existing failure message.
- Around line 1333-1385: Extract the duplicated abandoned-lease recovery loop
into a shared helper, preserving its marker loading, transaction validation,
live-owner rejection, exclusive publication, liveness re-check, lease removal,
and finally-based marker cleanup. Replace the recovery sequence in both
recoverExecutionArtifactsForRetirement and acquireMutationExecution with calls
to that helper, keeping each caller’s surrounding behavior unchanged.
- Around line 1000-1018: Extract the duplicated transaction-directory validation
from loadStateMutationIntentFromRoot and loadTransaction into a shared helper
that resolves the transaction path, returns null for ENOENT, rejects
non-directory or symlink paths, and calls verifyPrivateDirectory before
returning the verified directory. Replace both loaders’ inline preambles with
this helper while preserving their existing artifact-loading behavior.
- Around line 198-200: Use the canonical MAX_PLAN_BYTES exported by
state-mutation.ts in persisted-engine-lifecycle.ts, importing it and removing
the duplicate MAX_STATE_MUTATION_PLAN_BYTES constant. Update the serializedPlan
byte-limit check in prepareRuntimeProviderStateMutationPlan and the intent
publication validation to use this shared bound, while leaving the intent and
nonce limits unchanged.
In `@src/lib/onboard/runtime-provider/runtime-provider-contract.test.ts`:
- Around line 475-501: Extend the parameterized validation test around
createRuntimeProviderBundleRegistry to cover the remaining stateMutation v2
operations acquire, assertFenced, activate, and recover alongside the existing
entries. Preserve the current deletion and expected error-message assertion for
each operation.
In `@src/lib/onboard/runtime-provider/state-mutation.ts`:
- Around line 420-424: Replace the inline path/prefix identity expressions in
the Set construction and the corresponding logic around the second occurrence
with calls to the existing selectorIdentity helper. Preserve the current
deduplication behavior while ensuring both sites use the centralized identity
scheme.
In `@src/lib/shields/legacy-hermes-compat.test.ts`:
- Around line 1341-1346: Extract the repeated inline predicate checking that no
command contains the “-c” os.lstat capability probe into a shared helper such as
expectNoCapabilityProbe(commands). Replace the four duplicated assertions in the
affected tests with calls to this helper, preserving the existing
false-expectation behavior.
- Line 715: Remove the unnecessary bare block surrounding the new describe
section in the legacy Hermes compatibility tests, including the matching closing
block at the corresponding end. Keep the describe suite and its contents
unchanged while adjusting indentation to match the surrounding file.
- Around line 1030-1032: Update the parameterized test title in the it.each
block for forwardPolicyFailureFixtures so it describes the recovered
authorization/restore state or failure condition rather than specifically
calling every case an invalid forward policy. Ensure the timer-replaced fixture
is accurately represented while preserving the existing parameterized cases and
assertions.
- Around line 889-897: Move the phase assertions out of the routeSpy and
auditSpy mock implementations in both affected test sections. Have each mock
record the phase observed from transitionPath, then assert the collected phases
after shields.shieldsDown completes, preserving the expected route="preparing"
and audit="active" ordering without assertions being swallowed by production
error handling.
In `@test/helpers/privileged-exec-test-helpers.ts`:
- Around line 23-41: Update createOrdinaryExecReleaseSleeper so waitBuffer is
typed as an Int32Array backed by SharedArrayBuffer, satisfying Atomics.wait’s
shared-buffer requirement while preserving the existing sleep and release
behavior.
In `@test/helpers/shields-flow-harness.ts`:
- Around line 113-136: Extract the duplicated authorization-proof payload and
file-writing logic into a shared `writeProof` helper that accepts the timer
marker and process token. Update both `writeShieldsTimerAuthorizationProof` and
the `send` handler’s inline proof-writing block to call `writeProof`, preserving
each caller’s existing sandbox and token selection while keeping schema fields
and file permissions centralized.
In `@test/runtime-state-mutation-hermes-publisher.test.ts`:
- Around line 282-289: Update the runtime-state mutation tests to execute
runHarness once in a beforeAll hook, storing its Record<string, unknown> result
in shared test-scope state; have all three tests read that value instead of
calling runHarness individually, and import beforeAll from vitest.
- Around line 371-378: In the runtime-state-mutation Hermes publisher test,
remove the raw source-text assertions for the startup gate’s DURABLE_DIRECTORY
and exit-code constants and the publisher’s PYTHON_PATH constant. Retain the
existing start.sh wiring assertions, relying on the executable gate behavior
covered by runtime-state-mutation-startup-gate.test.ts for observable exit-code
validation.
In `@test/runtime-state-mutation-startup-gate.test.ts`:
- Around line 83-93: Extend the startup-gate harness around `_run("admit")` to
write a permit whose `start` differs from the constant captured by
`_capture_parent`, then assert admission is refused with `foreign_start:
"gate-start-mismatch"` in the existing result match. Keep the existing
matching-start case unchanged so the test covers both the valid binding and the
mismatch path in `_binding`.
🪄 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: 747c82d8-b702-4d45-816a-10a21c38f963
📒 Files selected for processing (88)
agents/hermes/Dockerfileagents/hermes/runtime-state-mutation-publisher-v1.jsonagents/hermes/start.shdocs/inference/set-up-llama-cpp.mdxdocs/manage-sandboxes/runtime-controls.mdxdocs/reference/commands.mdxdocs/reference/host-files-and-state.mdxdocs/reference/troubleshooting.mdxdocs/security/tcb-boundary.mdxscripts/checks/run-llama-cpp-dgx-spark-qualification.mtsscripts/install.shscripts/runtime-state-mutation-control.pyscripts/runtime-state-mutation-startup-gate.pyscripts/runtime_state_mutation_hermes_publisher.pysrc/lib/actions/sandbox/process-recovery-temp-ssh.test.tssrc/lib/actions/sandbox/process-recovery.tssrc/lib/actions/sandbox/snapshot-auto-create-failure.test.tssrc/lib/actions/sandbox/snapshot-restore-test-fixture.tssrc/lib/adapters/sandbox/command-transport.test.tssrc/lib/adapters/sandbox/command-transport.tssrc/lib/domain/uninstall/shims.test.tssrc/lib/domain/uninstall/shims.tssrc/lib/inference/llama-cpp/host-local-runtime.test.tssrc/lib/inference/llama-cpp/host-local-runtime.tssrc/lib/onboard/runtime-provider/access.tssrc/lib/onboard/runtime-provider/contract.tssrc/lib/onboard/runtime-provider/docker-llama-cpp-managed-lifecycle.test.tssrc/lib/onboard/runtime-provider/docker-llama-cpp-operation.tssrc/lib/onboard/runtime-provider/docker-operation-authority.test.tssrc/lib/onboard/runtime-provider/docker-operation-authority.tssrc/lib/onboard/runtime-provider/docker-state-mutation.test.tssrc/lib/onboard/runtime-provider/docker-state-mutation.tssrc/lib/onboard/runtime-provider/docker.tssrc/lib/onboard/runtime-provider/persisted-engine-lifecycle.test.tssrc/lib/onboard/runtime-provider/persisted-engine-lifecycle.tssrc/lib/onboard/runtime-provider/registry.tssrc/lib/onboard/runtime-provider/runtime-provider-contract.test.tssrc/lib/onboard/runtime-provider/state-mutation.test.tssrc/lib/onboard/runtime-provider/state-mutation.tssrc/lib/sandbox/agent-config.tssrc/lib/sandbox/config.tssrc/lib/sandbox/privileged-exec-exclusion.test.tssrc/lib/sandbox/privileged-exec.test.tssrc/lib/sandbox/privileged-exec.tssrc/lib/shields/flow.test.tssrc/lib/shields/hermes-runtime-state-mutation.test.tssrc/lib/shields/hermes-runtime-state-mutation.tssrc/lib/shields/index.test.tssrc/lib/shields/index.tssrc/lib/shields/legacy-hermes-compat.test.tssrc/lib/shields/mutable-config-repair.tssrc/lib/shields/openclaw-transition.test.tssrc/lib/shields/policy-transition.test.tssrc/lib/shields/timer-bound-lock.test.tssrc/lib/shields/timer-bound-lock.tssrc/lib/shields/timer-control.tssrc/lib/shields/timer-process.test.tssrc/lib/shields/timer.test.tssrc/lib/shields/timer.tssrc/lib/shields/transition-lock.tssrc/lib/state/mcp-lifecycle-lock/shields-timer-authority.tstest/config-set-nested-ssrf.test.tstest/config-set-prompt-error.test.tstest/helpers/docker-operation-authority-test-helpers.tstest/helpers/docker-state-mutation-harness.tstest/helpers/hermes-runtime-state-mutation-test-helpers.tstest/helpers/hermes-shields-provider-consumer-harness.tstest/helpers/privileged-exec-test-helpers.tstest/helpers/runtime-provider-state-mutation-test-helpers.tstest/helpers/shields-flow-harness.tstest/hermes-config-transaction-wiring.test.tstest/hermes-doctor-config-hash.test.tstest/hermes-final-image-layout.test.tstest/hermes-gateway-supervisor-recovery.test.tstest/hermes-start.test.tstest/install-npm-resolution.test.tstest/install-preflight.test.tstest/llama-cpp-dgx-spark-qualification-runner.test.tstest/package-contract/cli/config-set-prompt-eof.test.tstest/repro-2681-group-writable.test.tstest/runtime-provider-source-shape.test.tstest/runtime-state-mutation-control.test.tstest/runtime-state-mutation-hermes-publisher.test.tstest/runtime-state-mutation-startup-gate.test.tstest/sandbox-provisioning.test.tstest/sandbox-rlimit-hooks.test.tstest/shields-up-runtime-perms.test.tstest/support/hermes-shell-harness.ts
🚧 Files skipped from review as they are similar to previous changes (51)
- test/package-contract/cli/config-set-prompt-eof.test.ts
- test/shields-up-runtime-perms.test.ts
- src/lib/actions/sandbox/snapshot-auto-create-failure.test.ts
- src/lib/adapters/sandbox/command-transport.ts
- src/lib/actions/sandbox/process-recovery-temp-ssh.test.ts
- test/runtime-provider-source-shape.test.ts
- test/helpers/runtime-provider-state-mutation-test-helpers.ts
- src/lib/state/mcp-lifecycle-lock/shields-timer-authority.ts
- src/lib/onboard/runtime-provider/docker.ts
- src/lib/shields/timer-process.test.ts
- src/lib/onboard/runtime-provider/registry.ts
- scripts/install.sh
- src/lib/shields/mutable-config-repair.ts
- test/hermes-start.test.ts
- test/hermes-config-transaction-wiring.test.ts
- docs/reference/host-files-and-state.mdx
- src/lib/shields/transition-lock.ts
- src/lib/actions/sandbox/snapshot-restore-test-fixture.ts
- agents/hermes/runtime-state-mutation-publisher-v1.json
- src/lib/sandbox/privileged-exec.test.ts
- test/config-set-prompt-error.test.ts
- src/lib/sandbox/privileged-exec.ts
- docs/security/tcb-boundary.mdx
- src/lib/onboard/runtime-provider/access.ts
- docs/inference/set-up-llama-cpp.mdx
- src/lib/shields/index.test.ts
- test/hermes-final-image-layout.test.ts
- src/lib/onboard/runtime-provider/state-mutation.test.ts
- src/lib/shields/hermes-runtime-state-mutation.ts
- docs/reference/troubleshooting.mdx
- test/helpers/docker-operation-authority-test-helpers.ts
- src/lib/shields/policy-transition.test.ts
- test/helpers/hermes-runtime-state-mutation-test-helpers.ts
- src/lib/shields/hermes-runtime-state-mutation.test.ts
- test/helpers/docker-state-mutation-harness.ts
- src/lib/shields/timer-bound-lock.test.ts
- agents/hermes/Dockerfile
- src/lib/onboard/runtime-provider/contract.ts
- test/hermes-gateway-supervisor-recovery.test.ts
- test/sandbox-provisioning.test.ts
- docs/reference/commands.mdx
- src/lib/onboard/runtime-provider/docker-state-mutation.ts
- src/lib/sandbox/agent-config.ts
- src/lib/shields/timer.ts
- test/config-set-nested-ssrf.test.ts
- test/repro-2681-group-writable.test.ts
- agents/hermes/start.sh
- docs/manage-sandboxes/runtime-controls.mdx
- src/lib/actions/sandbox/process-recovery.ts
- src/lib/sandbox/config.ts
- src/lib/shields/timer-bound-lock.ts
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
…ation-consumer Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
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)
test/support/hermes-shell-harness.ts (1)
61-61: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winClean up the temporary directory when fixture setup fails.
tmpDiris created at Line 61, but thetryblock starts at Line 126. If fixture setup fails before Line 126, thefinallyblock does not run. Repeated failed test runs can leavenemoclaw-hermes-init-path-*directories in the system temporary directory.Start the
tryblock immediately afterfs.mkdtempSyncand keep all fixture setup inside it.Also applies to: 126-140
🤖 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/support/hermes-shell-harness.ts` at line 61, Move the existing try block to immediately follow creation of tmpDir in the fixture setup, enclosing all setup operations before the current try location. Keep the existing finally cleanup logic paired with this broader try so failures during any fixture setup remove the temporary directory.
🤖 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 `@test/support/hermes-shell-harness.ts`:
- Line 61: Move the existing try block to immediately follow creation of tmpDir
in the fixture setup, enclosing all setup operations before the current try
location. Keep the existing finally cleanup logic paired with this broader try
so failures during any fixture setup remove the temporary directory.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f87ddcfa-8723-42f3-b7ab-da25c5d08da5
📒 Files selected for processing (6)
src/lib/inference/llama-cpp/host-local-runtime.tssrc/lib/onboard/runtime-provider/persisted-engine-lifecycle.test.tssrc/lib/onboard/runtime-provider/persisted-engine-lifecycle.tstest/llama-cpp-dgx-spark-qualification-runner.test.tstest/runtime-state-mutation-control.test.tstest/support/hermes-shell-harness.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- src/lib/inference/llama-cpp/host-local-runtime.ts
- src/lib/onboard/runtime-provider/persisted-engine-lifecycle.test.ts
- test/runtime-state-mutation-control.test.ts
- test/llama-cpp-dgx-spark-qualification-runner.test.ts
- src/lib/onboard/runtime-provider/persisted-engine-lifecycle.ts
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
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 `@test/managed-image-protected-runtime-contract.test.ts`:
- Line 20: Extend the protected runtime contract test to build a managed-image
fixture and invoke the public onboarding flow, asserting that its returned or
consumed expectedSupervisorArgv matches MANAGED_IMAGE_OPENSHELL_SUPERVISOR_ARGV.
Cover the boundary through scripts/checks/run-managed-image-openshell-e2e.ts or
onboard-managed-workload orchestration rather than only inspecting the constant;
retain the frozen-array assertion only if that immutability is an independent
contract.
🪄 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: c25dde41-53ba-437b-be45-ce095c09c9fe
📒 Files selected for processing (4)
scripts/checks/run-managed-image-openshell-e2e.tssrc/lib/onboard/managed-workload/onboard-orchestration.tssrc/lib/onboard/sandbox-create-launch.tstest/managed-image-protected-runtime-contract.test.ts
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
Issue #7744 explicitly accepts B4-C2: a durable Docker stateMutation implementation with one named production consumer and no Podman registration. The complete exact-base comparison implements that scope without a competing B4-C2 PR. The provider binds the lifecycle ledger, Docker authority, exact runtime and mount identity, declarative plan, writer exclusion, activation proof, rollback, release, and restart recovery. Hermes capability selection preserves older-image compatibility and fails closed on invalid capability state. Documentation matches the user-visible behavior. No actionable correctness, compatibility, security, regression, or documentation finding remains.
4 participants
Summary
Implements the B4-C2 slice of #7744 by replacing Docker's unsupported
stateMutationsurface with a durable, provider-fenced implementation. Hermes Shields is the named managed-image consumer, with fencing retained across publication, validation, rollback, activation, and controller restart; Podman remains unregistered.Related Issue
Refs #7744
Changes
Extends the runtime-provider
stateMutationcontract to plan schema v2 and registers a real Docker implementation. Hermes Shields requires a canonical AgentDefinition protection projection, exact serialized plan digest, phase-bearing fence, rollback posture, activation proof, and explicit release. A direct filesystem mutation cannot prove that it applies to the current sandbox lifecycle, runtime, mount namespace, state root, or configuration projection. This behavior is covered bystate-mutation.test.ts,runtime-provider-contract.test.ts, anddocker-state-mutation.test.ts.Adds durable persisted-engine lifecycle claims, exact request/receipt validation, Docker operation authority, and exclusion of ordinary privileged container execution for the full SSH or OpenShell/fallback operation lifetime. Process-local locking or a check immediately before spawning is insufficient because provider publication, subprocess execution, fallback, cleanup, and controller recovery can cross process boundaries. This behavior is covered by
persisted-engine-lifecycle.test.ts,docker-operation-authority.test.ts,command-transport.test.ts,privileged-exec.test.ts, andprivileged-exec-exclusion.test.ts.Packages fixed, root-owned mutation control, startup-gate, and Hermes publisher helpers plus an exact capability descriptor in the managed image. The image-side controller validates the durable request, publishes or rolls back the protection posture, records activation evidence, and gates gateway startup and recovery. Host-only mutation is insufficient because publication and restart recovery must be enforced inside the image that owns the state mount. This behavior is covered by
runtime-state-mutation-control.test.ts,runtime-state-mutation-startup-gate.test.ts,runtime-state-mutation-hermes-publisher.test.ts,hermes-final-image-layout.test.ts, andhermes-gateway-supervisor-recovery.test.ts.Wires Hermes Shields as the named production consumer, including retained-fence recovery, rollback publication, activation proof handling, timer process identity, and transition-lock coordination. Only a current managed Hermes Docker image with the exact root-owned capability uses the provider protocol; images without it retain the existing compatibility path. A global switch is insufficient while older images remain valid deployments. This behavior is covered by
hermes-runtime-state-mutation.test.ts,flow.test.ts,legacy-hermes-compat.test.ts,openclaw-transition.test.ts,policy-transition.test.ts,timer-bound-lock.test.ts, andtimer-process.test.ts.Hardens Docker operation authority by binding the qualified executable, interpreter chain, credential and SSH helpers, endpoint, PATH semantics, and privileged-execution lease through every capture and spawn. The installer now preserves already-present user-local OpenShell and npm PATH entries so persisted authority survives status and cleanup without weakening real helper or endpoint drift checks.
Restricts qualification-only Docker loopback publication to a freshly queried live Docker Engine >= 28.3.3 authority. Registry and model-server publications each consume a separate single-use authority; ordinary managed llama.cpp onboarding retains the private bridge, disables contradictory inherited image healthchecks, and performs no Docker port publication.
Documents provider selection, durable fencing and phases, restart hold and recovery, rollback, ledger state, host files, troubleshooting, and the runtime trust boundary.
Type of Change
Quality Gates
Documentation Writer Review
docs-updatedDGX Station Hardware Evidence
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailablenpm run validate:pr, timer tests (22/22), mutation controller/startup-gate/publisher tests (16/16), and protected-readiness support tests (49/49) passed through exact head 2cd934e against runtime base e677fcb; pre-push hooks also passed.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result: Exact-head canonical CI run 31421039363 and managed-image all-agent activation run 31421040652 passed at prior head d4271fa. Current exact-head canonical CI run 31447285073, managed-image run 31447285090, and trusted protected E2E run 31447308972 are running at candidate 2cd934e against runtime base e677fcb and trusted workflow 2afba61.npm run docscompleted with 0 errors. Fern reported two non-PR warnings: the unauthenticated redirects check was skipped, and the existing light-mode accent contrast is 2.41:1.Signed-off-by: Aaron Erickson aerickson@nvidia.com
Summary by CodeRabbit