Skip to content
6 changes: 6 additions & 0 deletions src/lib/shields/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ const { verifyShieldsLockState }: typeof import("./verify-lock") = require("./ve
const { relockAndReconfirm }: typeof import("./relock-reconfirm") = require("./relock-reconfirm");
const {
inspectShieldsTransitionLockOwner,
isShieldsTransitionLockUnavailable,
takeoverShieldsTransitionLock,
withShieldsTransitionLock,
}: typeof import("./transition-lock") = require("./transition-lock");
Expand Down Expand Up @@ -785,6 +786,11 @@ function failShieldsCommand(message: string, _shouldThrow?: boolean): never {
}

function completeDeferredShieldsExit(error: unknown, shouldThrow = false): never {
if (isShieldsTransitionLockUnavailable(error)) {
console.error(` ${error.summary}`);
if (error.recovery) console.error(` Recovery: ${error.recovery}`);
return failShieldsCommand(error.summary, shouldThrow);
}
if (error instanceof DeferredShieldsExit && !shouldThrow) {
process.exit(error.exitCode);
}
Expand Down
61 changes: 61 additions & 0 deletions src/lib/shields/transition-lock.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,10 @@ import path from "node:path";
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";

import {
isShieldsTransitionLockUnavailable,
ShieldsTransitionLockManager,
type ShieldsTransitionLockOwner,
type ShieldsTransitionLockUnavailableError,
shieldsTransitionLockPath,
} from "./transition-lock";

Expand Down Expand Up @@ -1031,6 +1033,65 @@ describe("host shields transition lock", () => {
expect(fs.readFileSync(lockPath, "utf8")).toBe("{incomplete");
});

it("refuses an old malformed owner record without waiting out the timeout (#8108)", () => {
const lockPath = writeOwner("alpha", "{incomplete");
fs.utimesSync(lockPath, new Date(1_000), new Date(1_000));
let nowMs = 60_000;
let sleepCalls = 0;
const locker = manager({
now: () => nowMs,
sleep: (milliseconds) => {
sleepCalls += 1;
nowMs += milliseconds;
},
});

let caught: unknown;
try {
locker.withShieldsTransitionLock("alpha", "nemoclaw alpha shields status", () => undefined, {
waitTimeoutMs: 30_000,
pollIntervalMs: 50,
malformedStaleMs: 30_000,
});
} catch (error) {
caught = error;
}

expect(sleepCalls).toBe(0);
expect(nowMs).toBe(60_000);
expect(isShieldsTransitionLockUnavailable(caught)).toBe(true);
const failure = caught as ShieldsTransitionLockUnavailableError;
expect(failure.summary).toContain("Cannot acquire shields transition lock");
expect(failure.summary).toContain("the owner record is incomplete");
expect(failure.summary).not.toContain("will not remove");
expect(failure.recovery).toContain(`remove '${lockPath}' manually`);
expect(failure.lockPath).toBe(lockPath);
expect(fs.readFileSync(lockPath, "utf8")).toBe("{incomplete");
});

it("refuses an old malformed owner record on the async path (#8108)", async () => {
const lockPath = writeOwner("alpha", "{incomplete");
fs.utimesSync(lockPath, new Date(1_000), new Date(1_000));
let sleepCalls = 0;
const locker = manager({
now: () => 60_000,
sleepAsync: async () => {
sleepCalls += 1;
},
});

await expect(
locker.withShieldsTransitionLockAsync(
"alpha",
"nemoclaw alpha shields status",
async () => undefined,
{ waitTimeoutMs: 30_000, pollIntervalMs: 50, malformedStaleMs: 30_000 },
),
).rejects.toThrow(/Cannot acquire shields transition lock/);
expect(sleepCalls).toBe(0);
expect(fs.readFileSync(lockPath, "utf8")).toBe("{incomplete");
});

it.skipIf(process.platform === "win32")("rejects symbolic-link lock paths", () => {
const target = path.join(root, "target");
fs.writeFileSync(target, "{}", { mode: 0o600 });
Expand Down
91 changes: 80 additions & 11 deletions src/lib/shields/transition-lock.ts
Original file line number Diff line number Diff line change
Expand Up @@ -318,28 +318,82 @@ function staleOwnerRecovery(lockPath: string): string {
return `NemoClaw could not safely recover the stale lock automatically. ${manualRecovery(lockPath)}`;
}

function formatWaitReason(reason: WaitReason | null, lockPath: string): string {
if (!reason) return "the lock changed during inspection; retry the command";
interface WaitReasonDescription {
reason: string;
recovery: string;
}

function describeWaitReason(reason: WaitReason | null, lockPath: string): WaitReasonDescription {
if (!reason) {
return { reason: "the lock changed during inspection; retry the command", recovery: "" };
}
if (reason.kind === "recent-malformed") {
return `the owner record is incomplete and only ${Math.max(0, Math.floor(reason.ageMs))}ms old. Retry after the writer finishes`;
return {
reason: `the owner record is incomplete and only ${Math.max(0, Math.floor(reason.ageMs))}ms old.`,
recovery: "Retry after the writer finishes",
};
}
if (reason.kind === "stale-malformed") {
return `the owner record is incomplete and ${Math.max(0, Math.floor(reason.ageMs))}ms old. ${malformedStaleRecovery(lockPath)}`;
return {
reason: `the owner record is incomplete and ${Math.max(0, Math.floor(reason.ageMs))}ms old.`,
recovery: malformedStaleRecovery(lockPath),
};
}
const owner = reason.owner;
if (reason.kind === "dead") {
return `recorded owner PID ${String(owner.pid)} is not running (${owner.command}). ${staleOwnerRecovery(lockPath)}`;
return {
reason: `recorded owner PID ${String(owner.pid)} is not running (${owner.command}).`,
recovery: staleOwnerRecovery(lockPath),
};
}
if (reason.kind === "pid-reused") {
return `recorded owner PID ${String(owner.pid)} now has process-start identity '${reason.currentProcessStartIdentity}' instead of '${owner.processStartIdentity}' (${owner.command}). ${staleOwnerRecovery(lockPath)}`;
return {
reason: `recorded owner PID ${String(owner.pid)} now has process-start identity '${reason.currentProcessStartIdentity}' instead of '${owner.processStartIdentity}' (${owner.command}).`,
recovery: staleOwnerRecovery(lockPath),
};
}
if (reason.kind === "identity-unavailable") {
return `PID ${String(owner.pid)} is alive but its process-start identity cannot be verified (${owner.command}). Verify the active process and retry`;
return {
reason: `PID ${String(owner.pid)} is alive but its process-start identity cannot be verified (${owner.command}).`,
recovery: "Verify the active process and retry",
};
}
if (reason.kind === "same-process") {
return `another async chain in this process still owns the lock (${owner.command}). Wait for that operation to finish and retry`;
return {
reason: `another async chain in this process still owns the lock (${owner.command}).`,
recovery: "Wait for that operation to finish and retry",
};
}
return `PID ${String(owner.pid)} is still running (${owner.command}). Wait for that operation to finish and retry`;
return {
reason: `PID ${String(owner.pid)} is still running (${owner.command}).`,
recovery: "Wait for that operation to finish and retry",
};
}

export class ShieldsTransitionLockUnavailableError extends Error {
readonly lockPath: string;
readonly summary: string;
readonly recovery: string;

constructor(summary: string, recovery: string, lockPath: string) {
super(recovery ? `${summary} ${recovery}` : summary);
this.name = "ShieldsTransitionLockUnavailableError";
this.lockPath = lockPath;
this.summary = summary;
this.recovery = recovery;
}
}

export function isShieldsTransitionLockUnavailable(
error: unknown,
): error is ShieldsTransitionLockUnavailableError {
if (!error || typeof error !== "object") return false;
const candidate = error as { name?: unknown; summary?: unknown; recovery?: unknown };
return (
candidate.name === "ShieldsTransitionLockUnavailableError" &&
typeof candidate.summary === "string" &&
typeof candidate.recovery === "string"
);
}

export function shieldsTransitionLockPath(
Expand Down Expand Up @@ -817,6 +871,7 @@ export class ShieldsTransitionLockManager {
if (!observed) continue;
lastWaitReason = observed;
if (this.recoveredObservedStaleOwner(sandboxName, observed)) continue;
this.failFastOnUnrecoverableOwner(state, observed);
}
this.sleep(this.waitDuration(state, lastWaitReason));
}
Expand Down Expand Up @@ -848,6 +903,7 @@ export class ShieldsTransitionLockManager {
if (!observed) continue;
lastWaitReason = observed;
if (this.recoveredObservedStaleOwner(sandboxName, observed)) continue;
this.failFastOnUnrecoverableOwner(state, observed);
}
await this.sleepAsync(this.waitDuration(state, lastWaitReason));
}
Expand Down Expand Up @@ -954,12 +1010,25 @@ export class ShieldsTransitionLockManager {
private enforceWaitTimeout(state: AcquisitionState, reason: WaitReason | null): void {
const elapsedMs = Math.max(0, this.now() - state.startedAtMs);
if (elapsedMs >= state.waitTimeoutMs) {
throw new Error(
`Timed out after ${String(state.waitTimeoutMs)}ms waiting for shields transition lock '${state.lockPath}': ${formatWaitReason(reason, state.lockPath)}`,
const described = describeWaitReason(reason, state.lockPath);
throw new ShieldsTransitionLockUnavailableError(
`Timed out after ${String(state.waitTimeoutMs)}ms waiting for shields transition lock '${state.lockPath}': ${described.reason}`,
described.recovery,
state.lockPath,
);
}
}

private failFastOnUnrecoverableOwner(state: AcquisitionState, reason: WaitReason): void {
if (reason.kind !== "stale-malformed") return;
const described = describeWaitReason(reason, state.lockPath);
throw new ShieldsTransitionLockUnavailableError(
`Cannot acquire shields transition lock '${state.lockPath}': ${described.reason}`,
described.recovery,
state.lockPath,
);
}

private enforceWaitTimeoutBeforeRetry(
state: AcquisitionState,
reason: WaitReason | null,
Expand Down
69 changes: 69 additions & 0 deletions test/cli/shields-transition-lock.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
// SPDX-License-Identifier: Apache-2.0

import fs from "node:fs";
import path from "node:path";

import { describe, expect, test as it } from "../helpers/owned-test-resources";

import { runWithEnv, testTimeoutOptions, writeSandboxRegistry } from "./helpers";

const REFUSAL_BUDGET_MS = 15_000;

function writeCorruptTransitionLock(home: string, sandboxName: string): string {
const stateDir = path.join(home, ".nemoclaw", "state");
fs.mkdirSync(stateDir, { recursive: true });
const lockPath = path.join(stateDir, `shields-transition-lock-${sandboxName}.json`);
fs.writeFileSync(lockPath, "", { mode: 0o600 });
fs.utimesSync(lockPath, new Date(1_000), new Date(1_000));
return lockPath;
}

function expectCleanRefusal(out: string): void {
expect(out).toContain("the owner record is incomplete");
expect(out).toContain("Recovery:");
expect(out).toContain("manually");
expect(out).not.toContain("ShieldsTransitionLockManager");
expect(out).not.toMatch(/^\s+at /m);
expect(out).not.toContain("Node.js v");
}

describe("shields commands with a corrupt transition lock", () => {
it(
"refuses read-only status without a raw stack trace (#8108)",
testTimeoutOptions(30_000),
({ testHome }) => {
const { home } = testHome;
writeSandboxRegistry(home);
const lockPath = writeCorruptTransitionLock(home, "alpha");

const startedAt = Date.now();
const status = runWithEnv("alpha shields status 2>&1", testHome.environment());
const elapsedMs = Date.now() - startedAt;

expect(status.code).toBe(1);
expectCleanRefusal(status.out);
expect(elapsedMs).toBeLessThan(REFUSAL_BUDGET_MS);
expect(fs.readFileSync(lockPath, "utf8")).toBe("");
},
);

it(
"refuses shields down without a raw stack trace (#8108)",
testTimeoutOptions(30_000),
({ testHome }) => {
const { home } = testHome;
writeSandboxRegistry(home);
const lockPath = writeCorruptTransitionLock(home, "alpha");

const startedAt = Date.now();
const down = runWithEnv("alpha shields down --reason test 2>&1", testHome.environment());
const elapsedMs = Date.now() - startedAt;

expect(down.code).toBe(1);
expectCleanRefusal(down.out);
expect(elapsedMs).toBeLessThan(REFUSAL_BUDGET_MS);
expect(fs.readFileSync(lockPath, "utf8")).toBe("");
},
);
});
Comment on lines +31 to +88

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Add corrupt-lock coverage for shields up.

This suite covers shields status and shields down only. Add a shields up case that verifies exit code 1, clean recovery output, prompt refusal, and preservation of the malformed lock file.

As per path instructions, tests must prove that public entrypoints reach the new path. The PR objective includes shields up, shields down, and shields status.

🧰 Tools
🪛 ast-grep (0.45.0)

[warning] 46-46: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFileSync(lockPath, "utf8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').

(detect-non-literal-fs-filename-typescript)


[warning] 65-65: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFileSync(lockPath, "utf8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').

(detect-non-literal-fs-filename-typescript)

🤖 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/cli/shields-transition-lock.test.ts` around lines 31 - 69, Add a
corrupt-transition-lock test alongside the existing cases in the “shields
commands with a corrupt transition lock” suite, invoking the public `alpha
shields up` command and verifying exit code 1, clean refusal output via
`expectCleanRefusal`, completion within `REFUSAL_BUDGET_MS`, and unchanged
malformed lock contents. Keep the setup consistent with the existing `shields
status` and `shields down` tests.

Source: Path instructions

Loading