Skip to content

Commit 5f92696

Browse files
authored
fix(onboard): accept Hermes startup CMD in root helper (#9511)
<!-- markdownlint-disable MD041 --> ## Summary Hermes managed startup could reach the root helper with Docker-appended image `CMD` arguments, but the helper rejected that valid process shape before recording its durable fence. This change accepts trailing arguments only after the fixed `nemoclaw-start` position while preserving complete process identity binding. ## Related Issue Fixes #9485 ## Changes - Authenticate the exact `/usr/local/bin/nemoclaw-start` path when it is direct or immediately interpreted by an allowed Bash path, including Docker-appended trailing `CMD` arguments. - Continue hashing and fencing the complete argument vector, and reject bare, prefixed, or reordered startup-script forms. - Model Docker named-volume state in the mutation harness and verify recovery when the durable-volume acquire succeeds but its response is lost. - Documentation writer review found no documentation change is needed because this restores the existing managed Hermes topology without changing commands, configuration, defaults, or user workflow. ## Type of Change - [x] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates <!-- Check one tests line. Check other lines when applicable. Add every requested justification or approval reference. --> - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: Complete diff reviewed for exact process identity, parent and UID checks, full argument hashing, durable-volume recovery, and fail-closed negative forms. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## DGX Station Hardware Evidence <!-- Required only when scripts/prepare-dgx-station-host.sh changes. Maintainers must review the linked evidence before approving or merging. This is human-reviewed evidence, not authenticated hardware provenance. Exceptional bypasses use existing repository governance and must be documented on the PR. --> - [ ] Tested on DGX Station - Tested commit: - Station profile/scenario: - Result: - Supporting evidence: ## Verification <!-- Check each applicable item only when supported by the requested evidence. Run targeted tests once per relevant change set and rerun after later edits or hook autofixes that can affect the tested behavior. Do not rerun hook-covered checks. --> - [x] PR description includes a `Signed-off-by:` line and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run validate:pr` passed after refreshing `origin/main` when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — command/result or justification: `vitest` root-helper integration, 11/11 passed; Docker, Podman, and persisted-engine lifecycle suite, 83/83 passed; `npm run build:cli` and `npm run typecheck:cli` passed. - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only) - [ ] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) --- <!-- DCO sign-off is required in this PR description, and every commit must appear as Verified in GitHub. Run: git config user.name && git config user.email --> Signed-off-by: Rebecca Sliter <571084+rsliter@users.noreply.github.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Improved recognition of supported `nemoclaw-start` launch commands, including Docker-appended arguments and approved Bash invocation paths. - Tightened validation to reject unsupported or reordered command formats. - Improved recovery handling when durable-volume state acquisition responses are lost. - **Tests** - Expanded coverage for bind-mounted and managed-volume state configurations. - Added validation across multiple runtime command layouts and volume metadata scenarios. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: Rebecca Sliter <571084+rsliter@users.noreply.github.com>
1 parent 96e05c7 commit 5f92696

4 files changed

Lines changed: 47 additions & 17 deletions

File tree

scripts/runtime-state-mutation-control.py

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -105,6 +105,7 @@
105105
STARTUP_RETRY_ACK_PROTOCOL = "nemoclaw-runtime-state-mutation-retry-ack-v1"
106106
OPENSHELL_ARGV0 = b"/opt/openshell/bin/openshell-sandbox"
107107
NEMOCLAW_START_PATH = b"/usr/local/bin/nemoclaw-start"
108+
BASH_ARGV0 = (b"bash", b"/bin/bash", b"/usr/bin/bash")
108109
HERMES_GATEWAY_PATHS = (b"/usr/local/bin/hermes", b"/usr/local/bin/hermes.real")
109110
HERMES_INTERNAL_PORT = 18642
110111
HERMES_HEALTH_PATH = "/health"
@@ -2402,10 +2403,12 @@ def _is_openshell_supervisor(process: ProcessIdentity) -> bool:
24022403

24032404

24042405
def _is_nemoclaw_start(process: ProcessIdentity, sandbox_uid: int) -> bool:
2405-
direct = process.command == (NEMOCLAW_START_PATH,)
2406+
# Docker appends image CMD arguments after ENTRYPOINT. Authenticate the
2407+
# fixed startup-script position, then bind the complete argv to the fence.
2408+
direct = bool(process.command) and process.command[0] == NEMOCLAW_START_PATH
24062409
interpreted = bool(
2407-
len(process.command) == 2
2408-
and process.command[0].rsplit(b"/", 1)[-1] == b"bash"
2410+
len(process.command) >= 2
2411+
and process.command[0] in BASH_ARGV0
24092412
and process.command[1] == NEMOCLAW_START_PATH
24102413
)
24112414
return bool(

src/lib/onboard/runtime-provider/docker-state-mutation.test.ts

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -534,8 +534,13 @@ describe("Docker state mutation owner", () => {
534534
expect(runtime.lifecycleStore.listUnfinished()[0]?.phase).toBe("fence-established");
535535
});
536536

537-
it("converges when an orphan acquire writes its marker before recovery", () => {
538-
const runtime = harness({ loseAcquireResponseOnce: true });
537+
it("recovers a durable-volume fence when acquire succeeds after its response is lost (#9485)", () => {
538+
const runtime = harness({ loseAcquireResponseOnce: true, stateMountType: "volume" });
539+
expect(runtime.state).toMatchObject({
540+
mountDriver: "local",
541+
mountName: "nemoclaw-hermes-alpha-state",
542+
mountType: "volume",
543+
});
539544

540545
expect(() => runtime.owner.acquire({ ...runtime.context, plan: plan() })).toThrow(
541546
"root helper acquire did not complete successfully",

test/helpers/docker-state-mutation-harness.ts

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -139,11 +139,15 @@ export interface DockerStateMutationHarnessOptions {
139139
readonly lifecycleGeneration?: string;
140140
readonly loseAcquireResponseOnce?: boolean;
141141
readonly loseReleaseResponseOnce?: boolean;
142+
readonly stateMountType?: "bind" | "volume";
142143
}
143144

144145
export interface DockerStateMutationHarnessState {
146+
mountDriver: string | null;
147+
mountName: string | null;
145148
runtimePid: number;
146149
mountSource: string;
150+
mountType: "bind" | "volume";
147151
sandboxId: string;
148152
pidMode: string;
149153
privileged: boolean;
@@ -156,9 +160,16 @@ function createContainerStateMutationHarness(
156160
) {
157161
const lifecycleGeneration =
158162
options.lifecycleGeneration ?? DOCKER_STATE_MUTATION_LIFECYCLE_GENERATION;
163+
const stateMountType = options.stateMountType ?? "bind";
164+
const usesManagedVolume = stateMountType === "volume";
159165
const state: DockerStateMutationHarnessState = {
166+
mountDriver: usesManagedVolume ? "local" : null,
167+
mountName: usesManagedVolume ? "nemoclaw-hermes-alpha-state" : null,
160168
runtimePid: 4812,
161-
mountSource: "/var/lib/openshell/alpha/hermes",
169+
mountSource: usesManagedVolume
170+
? "/var/lib/docker/volumes/nemoclaw-hermes-alpha-state/_data"
171+
: "/var/lib/openshell/alpha/hermes",
172+
mountType: stateMountType,
162173
sandboxId: SANDBOX_ID,
163174
pidMode: "",
164175
privileged: false,
@@ -229,9 +240,11 @@ function createContainerStateMutationHarness(
229240
state.privileged,
230241
[
231242
{
232-
Type: "bind",
243+
Type: state.mountType,
233244
Source: state.mountSource,
245+
...(state.mountName === null ? {} : { Name: state.mountName }),
234246
Destination: DOCKER_STATE_MUTATION_STATE_ROOT,
247+
...(state.mountDriver === null ? {} : { Driver: state.mountDriver }),
235248
Mode: "",
236249
RW: true,
237250
Propagation: "rprivate",

test/runtime-state-mutation-control.test.ts

Lines changed: 19 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -187,15 +187,14 @@ def process(pid, state, parent, start, uid, command, inode):
187187
188188
root_uid = control.ROOT_UID
189189
pid1 = process(1, "S", 0, "100", root_uid, (control.OPENSHELL_ARGV0,), 101)
190-
start = process(
191-
10,
192-
"S",
193-
1,
194-
"200",
195-
1001,
196-
(b"/bin/bash", control.NEMOCLAW_START_PATH),
197-
110,
198-
)
190+
def start_process(pid, command):
191+
return process(pid, "S", 1, str(190 + pid), 1001, command, 100 + pid)
192+
193+
start = start_process(10, (b"/bin/bash", control.NEMOCLAW_START_PATH, b"/bin/bash"))
194+
prefixed_start = start_process(11, (b"/bin/bash", b"--noprofile", control.NEMOCLAW_START_PATH))
195+
reordered_start = start_process(12, (b"/bin/bash", b"/bin/bash", control.NEMOCLAW_START_PATH))
196+
bare_direct_start = start_process(13, (b"nemoclaw-start", b"/bin/bash"))
197+
bare_interpreted_start = start_process(14, (b"/bin/bash", b"nemoclaw-start", b"/bin/bash"))
199198
gateway = process(
200199
77,
201200
"S",
@@ -483,6 +482,11 @@ control._supported_writer_uids = lambda: (1000, 1001)
483482
control._sandbox_uid = lambda: 1001
484483
control._capture_process = lambda pid: {1: pid1, 10: start}.get(pid)
485484
control._capture_writer_processes = lambda _uids: (start, gateway)
485+
results["managed_start_with_cmd"] = control._is_nemoclaw_start(start, 1001)
486+
results["prefixed_start"] = control._is_nemoclaw_start(prefixed_start, 1001)
487+
results["reordered_start"] = control._is_nemoclaw_start(reordered_start, 1001)
488+
results["bare_direct_start"] = control._is_nemoclaw_start(bare_direct_start, 1001)
489+
results["bare_interpreted_start"] = control._is_nemoclaw_start(bare_interpreted_start, 1001)
486490
real_readlink = os.readlink
487491
os.readlink = lambda selected: "mnt:[401]" if selected == control.MOUNT_NAMESPACE_PATH else real_readlink(selected)
488492
try:
@@ -1273,10 +1277,15 @@ describe("runtime state mutation controller", () => {
12731277
});
12741278
});
12751279

1276-
it("holds exact OpenShell and entrypoint identities through recovery (#7744)", () => {
1280+
it("holds exact OpenShell and managed-image entrypoint identities through recovery (#9485)", () => {
12771281
expect(harnessResult).toMatchObject({
12781282
acquire: "fenced",
12791283
assert: "fenced",
1284+
managed_start_with_cmd: true,
1285+
prefixed_start: false,
1286+
reordered_start: false,
1287+
bare_direct_start: false,
1288+
bare_interpreted_start: false,
12801289
acquire_fence: {
12811290
supervisor: { pid: 1, startIdentity: "100" },
12821291
start: { pid: 10, startIdentity: "200", parentPid: 1 },

0 commit comments

Comments
 (0)