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:
📝 WalkthroughWalkthroughAdds Hermes runtime state mutation support across Python helpers, Docker provider lifecycle APIs, persisted fencing, startup checkpoints, Shields recovery, timer authorization, and privileged execution leases. It also adds Docker authority qualification, loopback publication controls, integration coverage, and final-image metadata validation. ChangesHermes runtime mutation and containment
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Shields
participant PersistedLifecycle
participant DockerStateMutation
participant HermesPublisher
participant HermesStartupGate
Shields->>PersistedLifecycle: Prepare and claim runtime target
Shields->>DockerStateMutation: Acquire and establish fence
DockerStateMutation->>HermesPublisher: Publish target posture
HermesPublisher->>HermesStartupGate: Coordinate startup checkpoint
HermesStartupGate-->>HermesPublisher: Return checkpoint or retry receipt
HermesPublisher-->>DockerStateMutation: Return activation receipt
DockerStateMutation-->>Shields: Return activation proof and release receipt
Possibly related issues
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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| return result | ||
|
|
||
|
|
||
| def _canonical(value: object) -> bytes: |
| return parent, start | ||
|
|
||
|
|
||
| def _parse_uids(raw: bytes) -> tuple[int, int, int, int]: |
| raise PublisherError(code) | ||
|
|
||
|
|
||
| def _canonical(value: object) -> bytes: |
| return result | ||
|
|
||
|
|
||
| def _parse_json(raw: bytes, maximum: int, code: str) -> object: |
| os.close(fd) | ||
| try: | ||
| os.unlink(temporary, dir_fd=directory_fd) | ||
| except FileNotFoundError: |
| _atomic_write(directory_fd, JOURNAL_NAME, journal) | ||
|
|
||
|
|
||
| def _acquire_lock(directory_fd: int) -> int: |
| _fail("publisher-guard-untrusted") | ||
|
|
||
|
|
||
| def _run_guard(action: str, arguments: list[str]) -> str: |
| def _continue_forward( | ||
| directory_fd: int, | ||
| journal: dict[str, object], | ||
| operation: dict[str, object], | ||
| normalized: dict[str, object], | ||
| ) -> dict[str, object]: |
| def _continue_abort( | ||
| directory_fd: int, | ||
| journal: dict[str, object], | ||
| operation: dict[str, object], | ||
| normalized: dict[str, object], | ||
| ) -> dict[str, object]: |
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 against this exact revision. Recommended 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
| while :; do | ||
| if nemoclaw_runtime_state_mutation_gate admit; then | ||
| break | ||
| else | ||
| _nemoclaw_runtime_state_mutation_gate_status=$? | ||
| fi | ||
| case "$_nemoclaw_runtime_state_mutation_gate_status" in | ||
| 10) break ;; | ||
| 75) | ||
| printf '%s\n' '[SECURITY] Hermes startup held by an active runtime state mutation.' >&2 | ||
| /bin/sleep 1 || true | ||
| ;; | ||
| *) | ||
| printf '%s\n' '[SECURITY] Runtime state mutation startup gate failed.' >&2 | ||
| exit 1 | ||
| ;; | ||
| esac | ||
| done | ||
| unset _nemoclaw_runtime_state_mutation_gate_status |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Exit code 75 conflates "retry-wait" with any refusal, so a wedged gate logs a misleading cause forever.
main() in scripts/runtime-state-mutation-startup-gate.py returns 75 for the retry-wait state and also for every GateError and OSError. This loop maps 75 to a one-second retry with the message "held by an active runtime state mutation". A corrupt permit, a bad directory mode, or an unreadable receipt therefore produces an endless 1 Hz log line that names the wrong cause.
Holding is the intended behavior, so keep the loop. Separate the diagnosis instead: return a distinct code for retry-wait, or rate-limit the message and state that the gate refused inspection.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@agents/hermes/start.sh` around lines 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.
| "abort-state-applied", | ||
| } | ||
| ) | ||
| TOP_SELECTORS = (".config-hash", ".env", "config.yaml") |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check whether an existing test binds the Hermes AgentDefinition selectors to the publisher TOP_SELECTORS tuple.
set -euo pipefail
rg -n -C 5 'TOP_SELECTORS'
rg -n -C 5 'config-hash' --glob '**/*.test.ts' --glob '**/*.test.py'
fd -i 'hermes.*state.*mutation.*test' --type fRepository: NVIDIA/NemoClaw
Length of output: 153
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- publisher ---'
sed -n '1,130p' scripts/runtime_state_mutation_hermes_publisher.py
printf '%s\n' '--- planner and related symbols ---'
rg -n -C 8 'prepareAgentDefinitionProtectionTransitionPlan|configPaths|publisher-plan-selector-mismatch|AgentDefinition' src scripts --glob '*.ts' --glob '*.py'
printf '%s\n' '--- candidate tests ---'
git ls-files | rg '(^|/)(.*test.*|.*spec.*)$' | rg 'runtime|state|mutation|hermes|onboard' || trueRepository: NVIDIA/NemoClaw
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- exact selector references ---'
rg -n -C 12 'TOP_SELECTORS|publisher-plan-selector-mismatch|prepareAgentDefinitionProtectionTransitionPlan|shieldsFiles|configFile:|envFile:' \
scripts/runtime_state_mutation_hermes_publisher.py \
src/lib/onboard/runtime-provider/state-mutation.ts \
src/lib/agent/defs.ts \
2>/dev/null || true
printf '%s\n' '--- relevant tracked files ---'
git ls-files | rg 'runtime-provider|state-mutation|hermes.*publisher|publisher.*hermes|hermes.*test|state.*mutation.*test' || true
printf '%s\n' '--- selector and planner tests ---'
rg -l 'TOP_SELECTORS|publisher-plan-selector-mismatch|prepareAgentDefinitionProtectionTransitionPlan|shieldsFiles' . \
--glob '*.test.ts' --glob '*.spec.ts' --glob '*.test.py' --glob '*.py' \
| head -100 || trueRepository: NVIDIA/NemoClaw
Length of output: 30671
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- Hermes publisher tests ---'
rg -n -C 10 'TOP_SELECTORS|config-hash|config.yaml|publisher-plan-selector-mismatch|selectors|projection' \
test/runtime-state-mutation-hermes-publisher.test.ts
printf '%s\n' '--- state mutation tests ---'
rg -n -C 12 'prepareAgentDefinitionProtectionTransitionPlan|configFile|envFile|shieldsFiles|selectors|Hermes|hermes' \
src/lib/onboard/runtime-provider/state-mutation.test.ts
printf '%s\n' '--- Hermes definition/config sources ---'
git ls-files agents src | rg 'hermes.*(json|ya?ml|ts)$|agent.*hermes|defs'
rg -n -C 8 '"config_file"|"env_file"|"shields_files"|config_file|env_file|shields_files' \
agents src/lib/agent --glob '*.json' --glob '*.yaml' --glob '*.yml' --glob '*.ts'Repository: NVIDIA/NemoClaw
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- Hermes manifest configuration ---'
rg -n -C 6 '^(config:| dir:| config_file:| env_file:| format:| shields_files:|state_lock_plan)' \
agents/hermes/manifest.yaml
printf '%s\n' '--- publisher test setup and assertions ---'
sed -n '1,180p' test/runtime-state-mutation-hermes-publisher.test.ts
sed -n '260,330p' test/runtime-state-mutation-hermes-publisher.test.ts
printf '%s\n' '--- agent loader and manifest test helpers ---'
sed -n '1,130p' src/lib/agent/defs.ts
rg -n -C 8 'loadAgent\\("hermes"|manifest.yaml|configShieldsFiles' src/lib/agent/defs.test.ts src/lib/agent/defs.tsRepository: NVIDIA/NemoClaw
Length of output: 15451
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import ast
import re
from pathlib import Path
publisher = Path("scripts/runtime_state_mutation_hermes_publisher.py").read_text()
module = ast.parse(publisher)
top = None
for node in module.body:
if isinstance(node, ast.Assign) and any(
isinstance(target, ast.Name) and target.id == "TOP_SELECTORS"
for target in node.targets
):
top = ast.literal_eval(node.value)
break
manifest = Path("agents/hermes/manifest.yaml").read_text()
config = re.search(
r"(?ms)^config:\n(?P<body>(?:^[ \t]+.*\n?)*)",
manifest,
)
if not config:
raise SystemExit("Hermes config block not found")
body = config.group("body")
config_file = re.search(r"(?m)^[ \t]+config_file:\s*([^\s#]+)", body).group(1)
env_file = re.search(r"(?m)^[ \t]+env_file:\s*([^\s#]+)", body).group(1)
shields = re.findall(r"(?m)^[ \t]+- ([^\s#]+)", body.split("shields_files:", 1)[1].split("auth_file:", 1)[0])
projection = tuple(dict.fromkeys((config_file, ".config-hash", env_file, *shields)))
tests = "\n".join(
str(path.read_text())
for path in Path("test").rglob("*.test.ts")
if "runtime-state-mutation" in path.name
)
print({
"publisher_TOP_SELECTORS": top,
"manifest_projection_top_paths": projection,
"values_match": tuple(top) == projection,
"publisher_test_references_loadAgent_hermes": 'loadAgent("hermes")' in tests,
"publisher_test_has_literal_selector_tuple": '(".config-hash", ".env", "config.yaml")' in tests,
})
PYRepository: NVIDIA/NemoClaw
Length of output: 423
Bind Hermes selector coverage to AgentDefinition
TOP_SELECTORS hard-codes the current paths, while prepareAgentDefinitionProtectionTransitionPlan derives them from configPaths, including shieldsFiles. The current tests use independent literals and do not load loadAgent("hermes"). If configPaths changes, a Shields transition fails with publisher-plan-selector-mismatch at runtime. Add a CI test that compares the derived Hermes selector set with the publisher contract.
🤖 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` at 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.
| export function hasActivePersistedEngineStateMutationTarget( | ||
| lifecycleStore: PersistedEngineLifecycleStore, | ||
| sandboxName: string, | ||
| runtimeId?: string, | ||
| ): boolean { | ||
| const exactSandboxName = exactName(sandboxName, "sandbox name"); | ||
| if (runtimeId !== undefined && !RUNTIME_ID.test(runtimeId)) { | ||
| fail("runtime identity is malformed"); | ||
| } | ||
| const exactRuntimeId = runtimeId; | ||
| return lifecycleStore | ||
| .listUnfinished() | ||
| .some( | ||
| (record) => | ||
| record.action === "state-mutation" && | ||
| record.sandboxName === exactSandboxName && | ||
| record.resources.some( | ||
| (resource) => | ||
| resource.role === "target" && | ||
| (exactRuntimeId === undefined || resource.runtimeId === exactRuntimeId), | ||
| ), | ||
| ); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Resolve every definition and consumer of the active state-mutation target gate.
set -euo pipefail
rg -nP --type=ts -C3 '\bhasActivePersistedEngineStateMutationTarget\b'
fd -t f 'access.ts' src/lib/onboard/runtime-provider --exec sed -n '1,120p'Repository: NVIDIA/NemoClaw
Length of output: 153
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- matching files ---'
git ls-files | rg '(^|/)(persisted-engine-lifecycle\.ts|access\.ts|privileged-exec\.ts|hermes-shields-provider-consumer-harness\.ts)$' || true
printf '%s\n' '--- symbol references ---'
rg -n -C4 'hasActivePersistedEngineStateMutationTarget|runtime-provider/access|privileged-exec|hermes-shields-provider-consumer-harness' . \
-g '*.ts' -g '*.js' -g '*.mjs' -g '*.tsx' || true
printf '%s\n' '--- runtime-provider files ---'
fd -t f . | rg 'onboard/runtime-provider' || trueRepository: NVIDIA/NemoClaw
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- access.ts exports and imports ---'
rg -n -C5 'persisted-engine-lifecycle|hasActivePersistedEngineStateMutationTarget|export' \
src/lib/onboard/runtime-provider/access.ts
printf '%s\n' '--- privileged-exec import and call ---'
sed -n '1,32p;200,228p' src/lib/sandbox/privileged-exec.ts
printf '%s\n' '--- harness loader and spy ---'
sed -n '110,205p' test/helpers/hermes-shields-provider-consumer-harness.ts
printf '%s\n' '--- all non-test direct gate references ---'
rg -n --glob '*.ts' --glob '!**/*.test.ts' \
'hasActivePersistedEngineStateMutationTarget' src testRepository: NVIDIA/NemoClaw
Length of output: 9653
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- test harness gate spy usage ---'
rg -n -C6 'lifecycleGateSpy|hasActivePersistedEngineStateMutationTarget' \
test/helpers/hermes-shields-provider-consumer-harness.ts \
src/lib/shields/*.test.ts
printf '%s\n' '--- compiler and test module settings ---'
rg -n -C3 '"(type|module|moduleResolution|transform|test|build)"|moduleResolution|module":|commonjs|esbuild|tsx|vitest' \
package.json tsconfig*.json vite.config.* vitest.config.* 2>/dev/null || true
printf '%s\n' '--- relevant import/export graph ---'
sed -n '1,55p' src/lib/onboard/runtime-provider/access.ts
sed -n '1,30p;240,265p' src/lib/shields/hermes-runtime-state-mutation.ts
sed -n '1,48p;1975,2050p' src/lib/onboard/runtime-provider/docker-state-mutation.tsRepository: NVIDIA/NemoClaw
Length of output: 41522
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- source loader implementation ---'
rg -n -C8 'function (source|load)|loadSource|sourceLoaderNodeOptions|createRequire|requireDist' \
test/helpers vitest.config.ts
printf '%s\n' '--- CommonJS output assumptions ---'
sed -n '1,45p' tsconfig.src.json
sed -n '1,120p' test/helpers/source-loader-options.ts 2>/dev/null || true
printf '%s\n' '--- direct imports of the gate ---'
rg -n -C2 'hasActivePersistedEngineStateMutationTarget' \
src/lib/sandbox/privileged-exec.ts \
src/lib/shields/hermes-runtime-state-mutation.ts \
src/lib/onboard/runtime-provider/docker-state-mutation.ts \
src/lib/onboard/runtime-provider/access.tsRepository: NVIDIA/NemoClaw
Length of output: 50371
Route the privileged-exec gate through runtime-provider/access.
access.ts re-exports the gate, but privileged-exec.ts imports it directly from persisted-engine-lifecycle.ts. The Hermes harness spy does not replace this direct binding. Use the same module path in both consumers, or spy on the direct module.
🤖 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
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.
| const marker = mode === "temporarily_unlocked" ? readTimerMarker(sandboxName) : null; | ||
| const assertTimedAuthority = () => { | ||
| if (mode !== "temporarily_unlocked") return; | ||
| if (!marker || !transition || transition.phase !== "active") { | ||
| throw new Error("Timed mutable status lost its exact active auto-restore authority"); | ||
| } | ||
| assertFreshShieldsDownAuthority(sandboxName, marker, transition, "active"); | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Determine whether a timed DOWN record can persist without a transition artifact.
set -euo pipefail
rg -nP --type=ts -C5 'readTimerBoundShieldsDownTransition' src/lib/shields
rg -nP --type=ts -C8 'forwardPolicy' src/lib/shields/index.ts | head -80
rg -nP --type=ts -C5 'shields-transition-' src/lib/shieldsRepository: NVIDIA/NemoClaw
Length of output: 29612
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- timed-DOWN creation, marker lifecycle, and transition cleanup ---'
rg -n -C8 'startFreshShieldsDownTimer|writeTimerMarker|clearTimer|readTimerMarker|clearShieldsDownTransition|phase: "active"|shieldsDown: true' src/lib/shields/index.ts
printf '%s\n' '--- protocol selection and Hermes status path ---'
rg -n -C12 'inspectHermesShieldsProtocol|provider-state-mutation-v2|verifyHermesProviderMutableStatus|getShieldsPostureWithoutHostLock|temporarily_unlocked' src/lib/shields/index.ts
printf '%s\n' '--- rebuild and persisted state handling ---'
rg -n -C10 'rebuild|recreate|managed image|capability|shields.*state|shieldsDown' src/lib/shields src/lib | head -300Repository: NVIDIA/NemoClaw
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- protocol capability and active-provider detection ---'
sed -n '850,970p' src/lib/shields/index.ts
rg -n -C12 'function hasActiveRuntimeProviderStateMutation|hasActiveRuntimeProviderStateMutation\(' src/lib/shields/index.ts src/lib
printf '%s\n' '--- posture and Hermes status implementation ---'
sed -n '1478,1510p' src/lib/shields/index.ts
sed -n '5398,5498p' src/lib/shields/index.ts
printf '%s\n' '--- rebuild/state preservation paths ---'
rg -n -C8 'clearShieldsStateWithoutHostLock|rebuild|lifecycleGeneration|state file|shields-timer' src/lib --glob '*.ts' | head -240
printf '%s\n' '--- timed status and legacy compatibility tests ---'
rg -n -C10 'temporarily_unlocked|timed|transition.*null|missing.*transition|provider-state-mutation-v2|rebuild' src/lib/shields/*.test.ts | head -300Repository: NVIDIA/NemoClaw
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- runtime-provider mutation state helper ---'
rg -n -C20 'hasActiveHermesRuntimeProviderStateMutation|runtime-provider-lifecycle|active.*mutation|provider.*state.*mutation' src --glob '*.ts' | head -260
printf '%s\n' '--- sandbox rebuild lifecycle entrypoints ---'
rg -n -C15 'upgrade.*sandbox|rebuild.*sandbox|rebuildSandbox|recreate.*sandbox|destroy.*rebuild|lifecycleGeneration|clearShieldsStateWithoutHostLock' src --glob '*.ts' | head -320
printf '%s\n' '--- timer marker persistence and timer child behavior ---'
sed -n '1,260p' src/lib/shields/timer-control.ts
rg -n -C12 'timerMarkerPath|writeTimerMarkerAtomic|clearTimerMarker|processToken|shieldsDown' src/lib/shields/timer-control.ts src/lib/shields/index.ts | head -260Repository: NVIDIA/NemoClaw
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- rebuild action files ---'
git ls-files 'src/lib/actions/**' 'src/lib/**' | rg '/(rebuild|destroy|upgrade)(/|\.|-)'
fd -i 'rebuild|destroy|upgrade' src/lib/actions src/lib/commands src/commands
printf '%s\n' '--- rebuild implementation and callers ---'
rg -n -C18 'export async function rebuildSandbox|function rebuildSandbox|rebuildSandbox\(|destroySandbox\(|clearShieldsStateWithoutHostLock|recoverSandboxWithHermesCronRestore' src/lib/actions src/commands src/lib --glob '*.ts' | head -400
printf '%s\n' '--- state cleanup helpers and lifecycle-store cleanup ---'
rg -n -C15 'clear.*Runtime|remove.*Lifecycle|delete.*Lifecycle|PERSISTED_ENGINE_LIFECYCLE_DIRECTORY|destroy.*state|clear.*state' src/lib/onboard src/lib/state src/lib/actions --glob '*.ts' | head -300Repository: NVIDIA/NemoClaw
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- rebuild Shields phase ---'
sed -n '1,280p' src/lib/actions/sandbox/rebuild-shields.ts
sed -n '1,260p' src/lib/actions/sandbox/rebuild-shields-phase.ts
printf '%s\n' '--- rebuild destroy phase ---'
sed -n '1,300p' src/lib/actions/sandbox/rebuild-destroy-phase.ts
sed -n '1,260p' src/lib/actions/sandbox/destroy-execution.ts
printf '%s\n' '--- rebuild orchestration call sites ---'
rg -n -C12 'rebuildShields|runRebuildShields|rebuild-shields|destroyPhase|executeSandboxDestroy|wipeSandboxState|cleanupShieldsDestroyArtifacts' src/lib/actions/sandbox/rebuild*.ts src/lib/actions/sandbox/destroy*.ts
printf '%s\n' '--- state wipe implementation ---'
sed -n '380,455p' src/lib/actions/sandbox/destroy.ts
rg -n -C12 'function removeShieldsState|export.*removeShieldsState|runtime-provider-lifecycle|shields-forward-policy|shields-transition' src/lib/actions/sandbox src/lib/state src/lib/shields --glob '*.ts' | head -220Repository: NVIDIA/NemoClaw
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- backup Shields window implementation ---'
sed -n '1,360p' src/lib/actions/sandbox/backup-shields-window.ts
printf '%s\n' '--- rebuild state helper ---'
rg -n -C25 'openBackupShieldsWindowForState|rebuildShieldsWindow|staleSandboxWasLocked|wasLocked|temporarily_unlocked|isShieldsDown' src/lib/actions/sandbox/rebuild-flow-helpers.ts src/lib/actions/sandbox/backup-shields-window.ts
printf '%s\n' '--- tests for already-down rebuild behavior ---'
rg -n -C18 'already.*down|temporarily_unlocked|wasLocked|openBackupShieldsWindowForState|rebuild.*down|timer.*rebuild' src/lib/actions/sandbox/*rebuild*.test.ts src/lib/actions/sandbox/*backup*.test.tsRepository: NVIDIA/NemoClaw
Length of output: 34147
Handle timed-DOWN state without a transition artifact.
openBackupShieldsWindow leaves an existing temporarily_unlocked state unchanged. A rebuild can therefore retain its host timer marker and Shields state while replacing the sandbox image. If that state predates the transition artifact, transition is null; assertTimedAuthority then throws and shields status reports DOWN (DRIFTED …) with exit code 2. Migrate this legacy state before enabling provider-state-mutation-v2, or handle the missing transition as a bounded compatibility case.
🤖 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 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.
| (marker.timerProcessStartIdentity === undefined || | ||
| marker.timerProcessStartIdentity === readProcessStartIdentity(process.pid)) && |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Cache the current process start identity instead of reading it on every match.
markerRecordMatchesCurrentTimer now calls readProcessStartIdentity(process.pid) on each invocation. The authorityPoll interval at Line 685 calls this function every AUTO_RESTORE_AUTHORITY_POLL_MS, and the restore timeout calls it again.
The start identity of the current process cannot change while the process runs. Each call reads /proc/self/stat, and on a host without /proc it falls back to execFileSync("ps", ...), which forks a process on every poll tick.
Resolve the identity once at module scope or on first use, then compare against the cached value.
♻️ Proposed change
+let currentProcessStartIdentity: string | null | undefined;
+
+function selfStartIdentity(): string | null {
+ if (currentProcessStartIdentity === undefined) {
+ currentProcessStartIdentity = readProcessStartIdentity(process.pid);
+ }
+ return currentProcessStartIdentity;
+}
+
function markerRecordMatchesCurrentTimer(
marker: ShieldsTimerMarker | null,
args: TimerArgs,
): boolean {
@@
(marker.timerProcessStartIdentity === undefined ||
- marker.timerProcessStartIdentity === readProcessStartIdentity(process.pid)) &&
+ marker.timerProcessStartIdentity === selfStartIdentity()) &&📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| (marker.timerProcessStartIdentity === undefined || | |
| marker.timerProcessStartIdentity === readProcessStartIdentity(process.pid)) && | |
| let currentProcessStartIdentity: string | null | undefined; | |
| function selfStartIdentity(): string | null { | |
| if (currentProcessStartIdentity === undefined) { | |
| currentProcessStartIdentity = readProcessStartIdentity(process.pid); | |
| } | |
| return currentProcessStartIdentity; | |
| } | |
| function markerRecordMatchesCurrentTimer( | |
| marker: ShieldsTimerMarker | null, | |
| args: TimerArgs, | |
| ): boolean { | |
| (marker.timerProcessStartIdentity === undefined || | |
| marker.timerProcessStartIdentity === selfStartIdentity()) && |
🤖 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.ts` around lines 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.
| selected, status = os.waitpid(writer_pid, os.WUNTRACED | os.WNOHANG) | ||
| results["guardian_writer_stopped"] = selected == writer_pid and os.WIFSTOPPED(status) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Poll waitpid until the stop is reported, to avoid a flaky assertion.
Line 905 calls os.waitpid once with WNOHANG. If the kernel has not yet reported the stop that guardian_hold sent at line 857, waitpid returns (0, 0) and guardian_writer_stopped becomes false. Line 1402 then fails intermittently. The last-resort block at lines 1003-1007 already polls until a deadline for the same condition. Use the same form here.
🐛 Proposed fix to poll for the stop report
- selected, status = os.waitpid(writer_pid, os.WUNTRACED | os.WNOHANG)
- results["guardian_writer_stopped"] = selected == writer_pid and os.WIFSTOPPED(status)
+ selected = 0
+ status = 0
+ deadline = time.monotonic() + 2
+ while time.monotonic() < deadline:
+ selected, status = os.waitpid(writer_pid, os.WUNTRACED | os.WNOHANG)
+ if selected == writer_pid and os.WIFSTOPPED(status):
+ break
+ time.sleep(0.01)
+ results["guardian_writer_stopped"] = selected == writer_pid and os.WIFSTOPPED(status)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| selected, status = os.waitpid(writer_pid, os.WUNTRACED | os.WNOHANG) | |
| results["guardian_writer_stopped"] = selected == writer_pid and os.WIFSTOPPED(status) | |
| selected = 0 | |
| status = 0 | |
| deadline = time.monotonic() + 2 | |
| while time.monotonic() < deadline: | |
| selected, status = os.waitpid(writer_pid, os.WUNTRACED | os.WNOHANG) | |
| if selected == writer_pid and os.WIFSTOPPED(status): | |
| break | |
| time.sleep(0.01) | |
| results["guardian_writer_stopped"] = selected == writer_pid and os.WIFSTOPPED(status) |
🤖 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 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.
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| return directory_access | os.O_DIRECTORY | os.O_NOFOLLOW | os.O_CLOEXEC | ||
|
|
||
|
|
||
| def _open_absolute_directory(path: str, *, readable_final: bool = False) -> int: |
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
| export function buildLlamaCppRequestGuardDockerArgv( | ||
| contract: LlamaCppHostLocalLaunchContract, | ||
| bindings: LlamaCppHostLocalRuntimeBindings, | ||
| loopbackPublishAuthority: DockerLoopbackPublishAuthority, | ||
| ): string[] { | ||
| consumeDockerLoopbackPublishAuthority(loopbackPublishAuthority); | ||
| validateContract(contract); | ||
| validateBindings(contract, bindings); | ||
| const { limits } = contract.serve; | ||
| return [ | ||
| ...buildLlamaCppHostLocalDockerRunArgv(contract, bindings), | ||
| "--publish", | ||
| `127.0.0.1:${bindings.hostPort === undefined ? "" : String(bindings.hostPort)}:${String(contract.serve.port)}`, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Require hostPort before you build the loopback publish specification.
When bindings.hostPort is undefined, line 386 emits --publish 127.0.0.1::8081. Docker reads that as a request for an ephemeral host port on loopback. The container starts on an unpredictable port instead of the documented port 8081.
validateBindings treats hostPort as optional (lines 290-292), and the downstream caller buildServerContainerArgv in scripts/checks/run-llama-cpp-dgx-spark-qualification.mts spreads hostPort only when it is defined. The undefined path is therefore reachable from the qualification runner.
This function exists to fence loopback publication. A missing host port must fail closed, not select a random port silently.
🐛 Proposed fix to require the host port
export function buildLlamaCppRequestGuardDockerArgv(
contract: LlamaCppHostLocalLaunchContract,
bindings: LlamaCppHostLocalRuntimeBindings,
loopbackPublishAuthority: DockerLoopbackPublishAuthority,
): string[] {
consumeDockerLoopbackPublishAuthority(loopbackPublishAuthority);
validateContract(contract);
validateBindings(contract, bindings);
+ if (bindings.hostPort === undefined) {
+ throw new Error(
+ "llama.cpp request-guard loopback publishing requires an explicit host port.",
+ );
+ }
const { limits } = contract.serve;
return [
...buildLlamaCppHostLocalDockerRunArgv(contract, bindings),
"--publish",
- `127.0.0.1:${bindings.hostPort === undefined ? "" : String(bindings.hostPort)}:${String(contract.serve.port)}`,
+ `127.0.0.1:${String(bindings.hostPort)}:${String(contract.serve.port)}`,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function buildLlamaCppRequestGuardDockerArgv( | |
| contract: LlamaCppHostLocalLaunchContract, | |
| bindings: LlamaCppHostLocalRuntimeBindings, | |
| loopbackPublishAuthority: DockerLoopbackPublishAuthority, | |
| ): string[] { | |
| consumeDockerLoopbackPublishAuthority(loopbackPublishAuthority); | |
| validateContract(contract); | |
| validateBindings(contract, bindings); | |
| const { limits } = contract.serve; | |
| return [ | |
| ...buildLlamaCppHostLocalDockerRunArgv(contract, bindings), | |
| "--publish", | |
| `127.0.0.1:${bindings.hostPort === undefined ? "" : String(bindings.hostPort)}:${String(contract.serve.port)}`, | |
| export function buildLlamaCppRequestGuardDockerArgv( | |
| contract: LlamaCppHostLocalLaunchContract, | |
| bindings: LlamaCppHostLocalRuntimeBindings, | |
| loopbackPublishAuthority: DockerLoopbackPublishAuthority, | |
| ): string[] { | |
| consumeDockerLoopbackPublishAuthority(loopbackPublishAuthority); | |
| validateContract(contract); | |
| validateBindings(contract, bindings); | |
| if (bindings.hostPort === undefined) { | |
| throw new Error( | |
| "llama.cpp request-guard loopback publishing requires an explicit host port.", | |
| ); | |
| } | |
| const { limits } = contract.serve; | |
| return [ | |
| ...buildLlamaCppHostLocalDockerRunArgv(contract, bindings), | |
| "--publish", | |
| `127.0.0.1:${String(bindings.hostPort)}:${String(contract.serve.port)}`, |
🤖 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/inference/llama-cpp/host-local-runtime.ts` around lines 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.
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>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
2 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
DGX 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 unavailableSigned-off-by: Aaron Erickson aerickson@nvidia.com
Summary by CodeRabbit