src/lib/provider-models.ts-36-38 (1)

36-38: ⚠️ Potential issue | 🟡 Minor

Guard null payloads before reading parsed.data.

If the body is "null", parsed.data throws a TypeError before your custom “Unexpected model catalog response” error path runs.

Proposed minimal fix

 function parseModelIds(body: string, itemKeys: Array<keyof ModelCatalogItem> = ["id"]): string[] {
-  const parsed = parseJson<ModelCatalogResponse>(body);
-  if (!Array.isArray(parsed.data)) {
+  const parsed = parseJson<ModelCatalogResponse | null>(body);
+  if (!parsed || !Array.isArray(parsed.data)) {
     throw new Error("Unexpected model catalog response: expected a top-level data array");
   }
   return parsed.data

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/lib/provider-models.ts` around lines 36 - 38, Guard against parsed being
null before accessing parsed.data in parseModelIds: after calling
parseJson<ModelCatalogResponse>(body) (in function parseModelIds), check whether
parsed is non-null (or use parsed?.data) and only then test
Array.isArray(parsed.data); if parsed is null throw or handle the same
"Unexpected model catalog response" error path so a body of "null" does not
cause a TypeError when referencing parsed.data.

scripts/type-safety-hotspots.ts-1356-1358 (1)

1356-1358: ⚠️ Potential issue | 🟡 Minor

--root/--project can incorrectly consume another flag as value.

requireDefined only rejects undefined, so inputs like --root --json treat --json as a path and silently skip the JSON mode flag.

💡 Proposed fix

+function requireCliValue(flag: string, value: string | undefined): string {
+  if (value === undefined || value.startsWith("-")) {
+    throw new Error(`Missing value for ${flag}`);
+  }
+  return value;
+}
+
 export function parseArgs(argv: string[]): CliOptions {
@@
       case "--root":
-        options.rootDir = path.resolve(
-          requireDefined(argv[index + 1], "Missing value for --root"),
-        );
+        options.rootDir = path.resolve(requireCliValue("--root", argv[index + 1]));
         index += 1;
         break;
       case "--project":
-        projectPaths.push(requireDefined(argv[index + 1], "Missing value for --project"));
+        projectPaths.push(requireCliValue("--project", argv[index + 1]));
         index += 1;
         break;

Also applies to: 1362-1362

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@scripts/type-safety-hotspots.ts` around lines 1356 - 1358, The code sets
options.rootDir using path.resolve(requireDefined(argv[index + 1], ...)) but
requireDefined only checks for undefined so flags like "--json" can be consumed
as a value; update the logic around argv[index + 1] (used when handling --root
and similarly for --project) to validate that the next token exists and does not
start with "-" before accepting it as the value, and if it is missing or looks
like a flag, throw or call requireDefined with a clear error; locate the
handling that assigns options.rootDir and the analogous assignment for --project
and add this extra check using argv, index, and requireDefined to prevent flags
being interpreted as values.

nemoclaw/src/index.ts-67-75 (1)

67-75: ⚠️ Potential issue | 🟡 Minor

Avoid deriving endpoint from provider fallback.

At Line 73, endpoint: endpoint ?? provider ?? "" can populate endpoint with a provider label/value. That can hide a missing endpoint and prevent the intended default fallback at Line 352.

Suggested fix

     return {
-      endpoint: endpoint ?? provider ?? "",
+      endpoint: endpoint ?? "",
       provider: provider ?? "",
       model: model ?? "",
     };

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@nemoclaw/src/index.ts` around lines 67 - 75, The returned config must not
assign endpoint from provider; change the return to set endpoint to endpoint ??
"" (remove the provider fallback) while keeping provider: provider ?? "" and
model: model ?? ""; in other words, stop using endpoint: endpoint ?? provider ??
"" and ensure any downstream logic that relies on a missing endpoint uses the
intended default fallback rather than a provider value—locate the return in the
parsing routine that uses parsedObject/readStringProperty and update that single
expression.

test/credential-rotation.test.ts-69-73 (1)

69-73: ⚠️ Potential issue | 🟡 Minor

The "Fresh imports" comment is misleading—require() returns cached modules without explicit cache clearing.

Calling the loaders in beforeEach doesn't bypass CommonJS caching. While vi.restoreAllMocks() clears mocks between tests, the module instances themselves remain cached. Consider clarifying the comment to reflect actual behavior, and consolidate mock cleanup into a single afterEach hook for clarity:

Suggested adjustment

-import { describe, expect, it, vi, beforeEach } from "vitest";
+import { describe, expect, it, vi, beforeEach, afterEach } from "vitest";
...
 beforeEach(() => {
-  // Fresh imports to avoid cross-test contamination
+  // Re-capture function references for each test
   ({ hashCredential, detectMessagingCredentialRotation } = loadCredentialRotationInternals());
   registry = loadRegistryModule();
 });
+
+afterEach(() => {
+  vi.restoreAllMocks();
+});

Then remove individual vi.restoreAllMocks() calls from each test block. If module state isolation is actually required, also add delete require.cache[require.resolve("../dist/lib/onboard.js")]; and corresponding cache deletion in beforeEach.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/credential-rotation.test.ts` around lines 69 - 73, The "Fresh imports"
comment is inaccurate because loadCredentialRotationInternals() and
loadRegistryModule() still return cached CommonJS modules; update the comment to
say "Reinitialize references to module internals (modules remain cached unless
cache is cleared)". Move vi.restoreAllMocks() calls into a single afterEach hook
to centralize mock cleanup and remove per-test restores. If you need true
module-state isolation, add explicit cache invalidation before re-requiring
(delete require.cache[require.resolve("../dist/lib/onboard.js")] and any other
target modules) in beforeEach, then re-import via the existing loaders
(loadCredentialRotationInternals and loadRegistryModule) so hashCredential and
detectMessagingCredentialRotation refer to fresh module instances.

src/lib/openshell.test.ts-29-29 (1)

29-29: ⚠️ Potential issue | 🟡 Minor

Correct the stub to match Node.js SpawnSyncReturns.output structure.

The output array returned by Node's spawnSync has the shape [null, stdout, stderr] (where output[0] is null for stdin, output[1] is stdout, output[2] is stderr). The current stub incorrectly omits the null element at index 0, creating a shape mismatch that could cause confusion or bugs in future tests that depend on correct array indexing.

Fix

-    output: [spec.stdout, spec.stderr],
+    output: [null, spec.stdout, spec.stderr],

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/lib/openshell.test.ts` at line 29, The test stub for
SpawnSyncReturns.output is missing the leading null for stdin; update the stub
used in src/lib/openshell.test.ts so the output array matches Node's spawnSync
shape (SpawnSyncReturns.output) by including a null as the first element so it
becomes [null, spec.stdout, spec.stderr], ensuring any code/tests that index
output[1]/output[2] behave correctly.

src/lib/config-io.ts-128-141 (1)

128-141: ⚠️ Potential issue | 🟡 Minor

Duplicate fallback branches silently mask JSON parse errors.

Lines 137 and 139 both return fallback. The first handles ENOENT (file not found), but line 139 catches all other errors—including SyntaxError from malformed JSON—without logging. This could mask corrupted config files.

🛡️ Consider warning on parse errors

     if (isErrnoException(errorObject) && errorObject.code === "ENOENT") {
       return fallback;
     }
+    // Log warning for unexpected errors (e.g., malformed JSON)
+    console.warn(`Warning: Failed to parse config file ${filePath}, using defaults`);
     return fallback;
   }

Alternatively, if silent fallback is intentional, remove the redundant if check:

     if (isErrnoException(errorObject) && errorObject.code === "ENOENT") {
       return fallback;
     }
-    return fallback;
+    throw error; // Surface unexpected errors like malformed JSON
   }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/lib/config-io.ts` around lines 128 - 141, readConfigFile currently
returns fallback for all errors (duplicate fallback branches) which silences
JSON parse errors; update readConfigFile to distinguish ENOENT (return fallback)
from JSON parse errors thrown by parseJson and either log a warning or rethrow a
ConfigParseError (or ConfigPermissionError only for permission cases).
Specifically, keep the permission check using isPermissionError and throw
ConfigPermissionError as today, keep the isErrnoException && code === "ENOENT"
branch to return fallback, but do not fall through to a silent generic fallback:
detect parse failures from parseJson (e.g., SyntaxError) and either throw a new
ConfigParseError including toError(errorObject) or call a logger.warn with
filePath and error details before returning fallback, and ensure
parseJson/readConfigFile references are updated accordingly so parse errors are
surfaced (use function name readConfigFile and helpers
isPermissionError/isErrnoException/toError in your change).

test/onboard.test.ts-3198-3198 (1)

3198-3198: ⚠️ Potential issue | 🟡 Minor

Add TypeScript reference for Node.js types to resolve NodeJS.ProcessEnv.

The NodeJS.ProcessEnv type annotation at lines 3198, 3527, 3664, and 3795 triggers ESLint's no-undef rule because the test ESLint configuration lacks TypeScript integration. Although @types/node is installed, the ESLint parser for test/**/*.ts has no parserOptions or projectService configuration to resolve the NodeJS namespace.

Add a triple-slash reference directive at the top of the file:

Suggested fix

+/// <reference types="node" />
 // SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
 // SPDX-License-Identifier: Apache-2.0

Alternatively, use typeof process.env instead of NodeJS.ProcessEnv (which is already defined in ESLint globals).

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/onboard.test.ts` at line 3198, The TypeScript NodeJS namespace isn't
resolved for the tests: either add a triple-slash directive /// <reference
types="node" /> at the very top of this test file (before any imports) so
NodeJS.ProcessEnv is available, or replace the explicit type annotations (e.g.,
the declarations named env using NodeJS.ProcessEnv at locations like the const
env variables) with the already-resolved typeof process.env; update occurrences
in functions/blocks referencing NodeJS.ProcessEnv (the const env declarations
around lines where env is defined) to one of these fixes so ESLint no-undef is
eliminated.

test/cli.test.ts-44-44 (1)

44-44: ⚠️ Potential issue | 🟡 Minor

ESLint reports 'NodeJS' is not defined — add explicit type import or use inline annotation.

The NodeJS namespace is a TypeScript global from @types/node, but ESLint's no-undef rule doesn't recognize it without explicit configuration. Consider either:

1. Adding /// <reference types="node" /> at the top
2. Using inline type: env: Record<string, string | undefined> = {}

🔧 Proposed fix using inline type

 function runWithEnv(
   args: string,
-  env: NodeJS.ProcessEnv = {},
+  env: Record<string, string | undefined> = {},
   timeout: number = Number(process.env.NEMOCLAW_EXEC_TIMEOUT || 10000),
 ): CliRunResult {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/cli.test.ts` at line 44, ESLint flags the use of the NodeJS namespace
for the variable env in test/cli.test.ts; fix by either adding a triple-slash
reference to `@types/node` at the top of the file (/// <reference types="node" />)
so NodeJS.ProcessEnv is recognized, or change the type of env (the declaration
`env: NodeJS.ProcessEnv = {}`) to an inline type such as Record<string, string |
undefined> to avoid the NodeJS global; update the env declaration accordingly
and run the linter to verify the error is resolved.

nemoclaw/src/blueprint/state.ts-31-33 (1)

31-33: ⚠️ Potential issue | 🟡 Minor

Reject malformed state fields instead of merging them through.

isStatePatch() only checks that the JSON root is an object. A persisted file with wrong field types still gets spread into NemoClawState, so this boundary can return "true" for shieldsDown, {} for updatedAt, etc. Build the patch from validated keys or add per-field guards before merging.

Also applies to: 78-80

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@nemoclaw/src/blueprint/state.ts` around lines 31 - 33, isStatePatch currently
only checks for a non-null object and lets malformed fields pass through; update
it (and the similar logic around lines referenced) to validate individual
keys/types and construct a safe Partial<NemoClawState> from only the known,
correctly-typed fields instead of returning true for any object. Specifically,
in isStatePatch (and the corresponding merge code at the other location)
whitelist expected keys from NemoClawState (e.g., shieldsDown: boolean,
updatedAt: string|Date, etc.), perform per-field guards/coercions/validation,
and return/build a patch object containing only validated properties so
merging/spreading cannot introduce wrong-typed fields into the state.

nemoclaw/src/blueprint/runner.ts-459-460 (1)

459-460: ⚠️ Potential issue | 🟡 Minor

Preserve the raw CLI token for the unknown-action error.

With the new isAction() check, nemoclaw foo now falls through as undefined, so the final error reports Unknown action 'undefined' instead of the actual bad token. Keeping argv[0] separately makes the message actionable again.

Proposed fix

 export async function main(argv: string[] = process.argv.slice(2)): Promise<void> {
-  const action = isAction(argv[0]) ? argv[0] : undefined;
+  const rawAction = argv[0];
+  const action = isAction(rawAction) ? rawAction : undefined;
@@
     case undefined:
     default:
-      throw new Error(`Unknown action '${String(action)}'. Use: plan, apply, status, rollback`);
+      throw new Error(
+        `Unknown action '${String(rawAction)}'. Use: plan, apply, status, rollback`,
+      );
   }
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@nemoclaw/src/blueprint/runner.ts` around lines 459 - 460, The CLI now loses
the original token when using isAction(argv[0]) so unknown-action errors show
'undefined'; capture the raw first (e.g. const raw = argv[0]) and then compute
action with isAction(raw) ? raw : undefined, and update any error reporting that
currently uses action to reference the preserved raw token (raw) in the "Unknown
action" message so the actual bad token is displayed; look for usages in
function main and any downstream error/throw that references action.

src/lib/services.ts (1)

106-107: Avoid dual source-of-truth for service literals.

ServiceName and SERVICE_NAMES currently duplicate the same literal, which can drift when new services are added. Consider deriving ServiceName from SERVICE_NAMES to keep one authoritative list.

Suggested refactor

-type ServiceName = "cloudflared";
-const SERVICE_NAMES: readonly ServiceName[] = ["cloudflared"];
+const SERVICE_NAMES = ["cloudflared"] as const;
+type ServiceName = (typeof SERVICE_NAMES)[number];

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/lib/services.ts` around lines 106 - 107, Currently the service literals
are duplicated in ServiceName and SERVICE_NAMES which can drift; make
SERVICE_NAMES the single source of truth (declare it as a readonly "as const"
tuple) and derive the ServiceName type from it using the indexed lookup (e.g.,
type ServiceName = typeof SERVICE_NAMES[number]); update any usages of
ServiceName or SERVICE_NAMES accordingly so only the array literal needs editing
when adding/removing services.

test/e2e/brev-e2e.test.ts (2)

124-135: ssh function uses INSTANCE_NAME directly without validation.

The function uses INSTANCE_NAME (line 131) which can be undefined. While the test suite is gated by hasRequiredVars, this function could be called before that check or in isolation. For consistency with the PR's type-safety goals, consider using requireInstanceName().

🛡️ Defensive fix

 function ssh(
   cmd: string,
   { timeout = 120_000, stream = false }: { timeout?: number; stream?: boolean } = {},
 ): string {
+  const instanceName = requireInstanceName();
   const escaped = cmd.replace(/'/g, "'\\''");
   const stdio = stream ? STREAM_STDIO : CAPTURE_STDIO;
   const result = execSync(
-    `ssh -o StrictHostKeyChecking=no -o LogLevel=ERROR "${INSTANCE_NAME}" '${escaped}'`,
+    `ssh -o StrictHostKeyChecking=no -o LogLevel=ERROR "${instanceName}" '${escaped}'`,
     { encoding: "utf-8", timeout, stdio },
   );
   return stream ? "" : result.trim();
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/e2e/brev-e2e.test.ts` around lines 124 - 135, The ssh helper uses the
global INSTANCE_NAME directly which can be undefined; change ssh to call
requireInstanceName() to retrieve a validated instance (replace references to
INSTANCE_NAME inside ssh) so the command string always gets a non-null value;
ensure you import/require the same requireInstanceName function used elsewhere
and use its return in the SSH command construction inside function ssh.

---

86-92: JSON.parse result lacks runtime validation.

JSON.parse returns unknown, but the code directly returns it as Array<{ name: string; status?: string }> without validating the shape. If brev ls --json returns an unexpected format, the callers may fail with confusing errors.

Consider adding a basic runtime check or using a type guard.

🛡️ Optional: Add minimal shape validation

 function listBrevInstances(): Array<{ name: string; status?: string }> {
   try {
-    return JSON.parse(brev("ls", "--json"));
+    const parsed: unknown = JSON.parse(brev("ls", "--json"));
+    if (!Array.isArray(parsed)) return [];
+    return parsed.filter(
+      (item): item is { name: string; status?: string } =>
+        typeof item === "object" && item !== null && typeof (item as Record<string, unknown>).name === "string"
+    );
   } catch {
     return [];
   }
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/e2e/brev-e2e.test.ts` around lines 86 - 92, The function
listBrevInstances currently returns JSON.parse(brev("ls", "--json")) cast as
Array<{name:string; status?:string}> without runtime validation; update
listBrevInstances to parse the JSON and then perform a minimal shape check (use
Array.isArray(result) and for each item verify typeof item.name === "string" and
(item.status === undefined || typeof item.status === "string")) or implement a
small type guard helper used by listBrevInstances; if validation fails, return
an empty array (or handle error consistently) so callers never receive an
unexpected shape from brev("ls","--json").

scripts/validate-configs.ts (1)

79-83: Schema parsing assumes object root without validation.

JSON.parse could return an array or primitive if the schema file is malformed. Consider adding a guard or letting TypeScript infer the type.

♻️ Add runtime validation or remove type annotation

 function loadSchema(repoRelative: string): object {
   const abs = join(REPO_ROOT, repoRelative);
-  const schema: object = JSON.parse(readFileSync(abs, "utf-8"));
-  return schema;
+  const schema: unknown = JSON.parse(readFileSync(abs, "utf-8"));
+  if (typeof schema !== "object" || schema === null || Array.isArray(schema)) {
+    throw new Error(`Schema ${repoRelative} must be a JSON object`);
+  }
+  return schema;
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@scripts/validate-configs.ts` around lines 79 - 83, The loadSchema function
currently calls JSON.parse and asserts the result as object, which may be an
array or primitive for malformed files; modify loadSchema to parse into an
unknown (or any) first, then perform a runtime type guard to verify the parsed
value is a non-null object and not an array (e.g., typeof result === "object" &&
result !== null && !Array.isArray(result")); if the check fails, throw a
descriptive Error mentioning the repoRelative path so callers know the file is
invalid; keep the function name loadSchema and the same return behavior when
validation passes.

test/setup-jetson.test.ts (1)

126-126: Redundant coercion before calling getExecErrorOutput.

The function signature getExecErrorOutput(error: Error | string | null | undefined) already handles all these types. The inline coercion error instanceof Error ? error : String(error) is unnecessary since String(error) for an Error object would lose the stderr property.

♻️ Simplify by passing the error directly

       } catch (error) {
-        output = getExecErrorOutput(error instanceof Error ? error : String(error));
+        output = getExecErrorOutput(error as Error | string | null | undefined);
       }

Alternatively, if you want to avoid the cast, broaden the catch variable type annotation or use a type guard inline.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/setup-jetson.test.ts` at line 126, The call to getExecErrorOutput is
coercing the caught value with error instanceof Error ? error : String(error),
which can strip Error-specific fields like stderr; instead, pass the caught
value directly to getExecErrorOutput (i.e., call getExecErrorOutput(error)) and,
if TypeScript complains, widen the catch variable type or add a local type guard
so the catch binding is typed as Error | string | null | undefined; update the
assignment to output accordingly and remove the redundant coercion.

nemoclaw/src/onboard/config.ts (1)

63-65: Consider making isOptionalString a type guard.

While functionally correct in this context, converting to a type guard would make the function reusable and more idiomatic.

♻️ Convert to type guard

-function isOptionalString(value: string | null | undefined): boolean {
-  return value === undefined || typeof value === "string";
+function isOptionalString(value: unknown): value is string | undefined {
+  return value === undefined || typeof value === "string";
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@nemoclaw/src/onboard/config.ts` around lines 63 - 65, Change isOptionalString
into a proper type guard so it can be reused: update the signature to accept
unknown (e.g., isOptionalString(value: unknown): value is string | null |
undefined) and adjust the body to explicitly allow undefined, null, or typeof
value === "string" (add the null check). Keep the function name isOptionalString
and use this new type predicate wherever the old helper is used.

src/lib/registry.ts (1)

76-78: Redundant null/object checks before isErrnoException.

The isErrnoException function already verifies error !== null && typeof error === "object", so the outer checks are duplicative.

♻️ Simplify the condition

     } catch (error) {
-      if (!(typeof error === "object" && error !== null && isErrnoException(error)) || error.code !== "EEXIST") {
+      if (!isErrnoException(error) || error.code !== "EEXIST") {
         throw error;
       }

Apply the same simplification to lines 89-92, 126-128, and 133-135.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/lib/registry.ts` around lines 76 - 78, Remove the redundant typeof/null
guards and rely on isErrnoException to perform those checks: change the
condition that currently reads if (!(typeof error === "object" && error !== null
&& isErrnoException(error)) || error.code !== "EEXIST") to use only
isErrnoException(error) (and the error.code check) so the branch becomes based
on !isErrnoException(error) || error.code !== "EEXIST". Apply the same
simplification to the other occurrences mentioned (the conditions at the blocks
around lines 89-92, 126-128, and 133-135) so all checks uniformly call
isErrnoException(error) without duplicating null/typeof checks; keep the
error.code comparisons (e.g., "EEXIST") intact and reference the same error
variable and isErrnoException helper.

test/snapshot.test.ts (1)

24-26: Type definitions work but could be consolidated.

The recursive types BackupValue (line 25) and BackupManifestOverrides (line 49) are mutually dependent. TypeScript hoists type declarations, so this works, but defining them adjacently would improve readability.

♻️ Suggested consolidation

-type BackupScalar = string | number | boolean | null | undefined;
-type BackupValue = BackupScalar | BackupManifestOverrides | BackupValue[];
-
-function isSandboxStateModule(
+type BackupScalar = string | number | boolean | null | undefined;
+type BackupManifestOverrides = { [key: string]: BackupValue };
+type BackupValue = BackupScalar | BackupManifestOverrides | BackupValue[];
+
+function isSandboxStateModule(

And remove the duplicate definition at line 49.

Also applies to: 49-49

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/snapshot.test.ts` around lines 24 - 26, The two mutually recursive type
aliases BackupValue and BackupManifestOverrides should be defined together for
clarity: move or recreate the BackupManifestOverrides type adjacent to the
BackupValue declaration and keep the recursive references (BackupValue[] and
BackupManifestOverrides) intact so TypeScript's recursion still works; then
remove the duplicate BackupManifestOverrides definition elsewhere in the file to
avoid redundancy.

test/presets-checkbox.test.ts (1)

33-39: Edge case: parseResult throws if stdout is empty.

If stdout is empty or contains only whitespace, lines will be [], and lines[lines.length - 1] evaluates to undefined, causing JSON.parse(undefined) to throw. While this surfaces test failures appropriately in this context, adding a guard would make debugging easier.

♻️ Defensive check

 function parseResult(stdout: string): string[] {
   const lines = stdout.trim().split("\n").filter(Boolean);
+  if (lines.length === 0) {
+    return [];
+  }
   const parsed: Array<string | null> = JSON.parse(lines[lines.length - 1]);
   return Array.isArray(parsed)
     ? parsed.filter((entry): entry is string => typeof entry === "string")
     : [];
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/presets-checkbox.test.ts` around lines 33 - 39, The parseResult function
can call JSON.parse on undefined when stdout is empty; update parseResult to
first trim/split into lines (as done), then if lines.length === 0 return an
empty array (or throw a clear Error) instead of accessing lines[lines.length -
1]; only call JSON.parse on a defined string, keep the existing type-filtering
logic (parsed and the entry type guard) unchanged so the function still returns
string[] when valid JSON is present.

src/lib/usage-notice.ts (1)

51-53: parseJson<T> provides type annotation without runtime validation.

The generic parseJson<T> function returns the parsed value typed as T, but doesn't validate the shape at runtime. For hasAcceptedUsageNotice (line 105), if the file contains null or an unexpected shape, the access to saved.acceptedVersion could throw—though the surrounding catch handles this gracefully.

Consider adding shape validation similar to the readStringProperty helpers, or documenting that callers must handle invalid shapes via try/catch.

♻️ Validate shape before access

 export function hasAcceptedUsageNotice(version: string): boolean {
   try {
-    const saved = parseJson<{ acceptedVersion?: string }>(
-      fs.readFileSync(getUsageNoticeStateFile(), "utf8"),
-    );
-    return saved.acceptedVersion === version;
+    const saved = JSON.parse(fs.readFileSync(getUsageNoticeStateFile(), "utf8"));
+    return typeof saved === "object" && saved !== null && saved.acceptedVersion === version;
   } catch {
     return false;
   }
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/lib/usage-notice.ts` around lines 51 - 53, The parseJson<T> helper
currently asserts a generic type without runtime validation; update code so
parsing never falsely types unexpected shapes: either change parseJson<T> to
return unknown (e.g., parseJson(text): unknown) and have callers validate
shapes, or add a lightweight runtime shape check in hasAcceptedUsageNotice to
ensure the parsed object has a non-null object with a numeric/string
acceptedVersion before accessing saved.acceptedVersion; reference parseJson and
hasAcceptedUsageNotice when making the change and keep existing try/catch
behavior for malformed JSON.

src/lib/config-io.ts (1)

18-20: isErrnoException check is minimal but potentially fragile.

The guard only checks for "code" in error, which could match non-ErrnoException objects. Consider also verifying typeof error.code === "string" for stricter validation.

♻️ Stricter check

 function isErrnoException(error: object | null): error is NodeJS.ErrnoException {
-  return error !== null && "code" in error;
+  return error !== null && "code" in error && typeof (error as { code: unknown }).code === "string";
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/lib/config-io.ts` around lines 18 - 20, The type guard is too permissive:
update isErrnoException to ensure the found "code" property is actually a string
(and that error is non-null/typeof "object") before narrowing; locate the
isErrnoException function and change its predicate to check error !== null &&
typeof error === "object" && "code" in error && typeof (error as any).code ===
"string" so only real ErrnoExceptions pass the guard.

test/validate-blueprint.test.ts (1)

68-70: loadYaml<T> is a cast without runtime validation.

Unlike the loadYAML/loadJSON helpers in validate-config-schemas.test.ts that include isLooseObject guards, this generic loadYaml<T> function directly casts the parsed YAML to T. This is acceptable here since:

1. The tests validate structure via assertions
2. Schema validation in validate-config-schemas.test.ts covers structural correctness
3. Invalid YAML would cause test failures

However, for consistency with the PR's type-safety goals, consider adding a shape guard or documenting that callers must validate.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/validate-blueprint.test.ts` around lines 68 - 70, The generic
loadYaml<T>(path: URL) currently casts YAML.parse(...) to T without runtime
validation; update loadYaml to perform a loose shape guard (e.g., reuse the
isLooseObject predicate used by loadYAML/loadJSON in
validate-config-schemas.test.ts) and return the parsed value only if the guard
passes (or throw a clear error), or alternatively add a comment/docstring on
loadYaml<T> that callers must perform their own validation—change the
implementation of loadYaml<T> or its documentation accordingly and keep the
function name loadYaml<T> so callers are easy to find.

src/nemoclaw.ts (1)

514-520: Redundant || undefined fallback.

Map.get() already returns undefined for missing keys, so || undefined is unnecessary.

Remove redundant fallback

   const liveNames = Array.from<string>(parseLiveSandboxNames(liveList.output));
   for (const name of liveNames) {
-    const metadata = metadataByName.get(name) || undefined;
+    const metadata = metadataByName.get(name);
     if (upsertRecoveredSandbox(name, metadata)) {
       recoveredFromGateway += 1;
     }
   }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/nemoclaw.ts` around lines 514 - 520, Remove the redundant "|| undefined"
when retrieving metadata from the Map: change the assignment to const metadata =
metadataByName.get(name); (leave the loop and the call to
upsertRecoveredSandbox(name, metadata) and increment of recoveredFromGateway
unchanged) so that metadata is naturally undefined for missing keys without the
pointless fallback.

test/credentials.test.ts (1)

12-19: Consider using in operator instead of Reflect.get for type guards.

While Reflect.get works, using the in operator is more idiomatic for property existence checks in type guards and avoids potential prototype chain issues.

Use `in` operator for property checks

 function isCredentialsModule(value: object | null): value is CredentialsModule {
   return (
     value !== null &&
-    typeof Reflect.get(value, "loadCredentials") === "function" &&
-    typeof Reflect.get(value, "getCredential") === "function" &&
-    typeof Reflect.get(value, "saveCredential") === "function"
+    "loadCredentials" in value && typeof value.loadCredentials === "function" &&
+    "getCredential" in value && typeof value.getCredential === "function" &&
+    "saveCredential" in value && typeof value.saveCredential === "function"
   );
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/credentials.test.ts` around lines 12 - 19, The type guard
isCredentialsModule should use the `in` operator instead of Reflect.get to check
property existence; update the implementation to first ensure value !== null
(and typeof value === "object"), then use `"loadCredentials" in value &&
"getCredential" in value && "saveCredential" in value` and finally verify each
is a function with typeof (value as any).loadCredentials === "function" etc.,
referencing the existing isCredentialsModule function and the
loadCredentials/getCredential/saveCredential property names.

src/lib/credentials.ts (1)

41-44: Redundant type checks in error guard.

The condition typeof error === "object" && error !== null && isErrnoException(error) is redundant since isErrnoException already performs the error !== null && typeof error === "object" checks internally (line 17).

Simplify the condition

   } catch (error) {
-    if (!(typeof error === "object" && error !== null && isErrnoException(error)) || error.code !== "ENOENT") {
+    if (!isErrnoException(error) || error.code !== "ENOENT") {
       throw error;
     }
   }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/lib/credentials.ts` around lines 41 - 44, The catch block’s guard is
overly verbose: remove the redundant typeof/null checks and rely on
isErrnoException(error) which already performs them; update the condition in the
catch (currently using `typeof error === "object" && error !== null &&
isErrnoException(error) || error.code !== "ENOENT"`) to simply check
`!isErrnoException(error) || error.code !== "ENOENT"` so that
non-ErrnoExceptions or errors with a code not equal to "ENOENT" are re-thrown;
locate this change in the catch handling around the `isErrnoException` call in
src/lib/credentials.ts.

src/lib/credential-filter.ts (1)

15-17: parseJson<T> is an unchecked cast — acceptable here given downstream validation.

The wrapper casts JSON.parse output directly to T without runtime validation. This is acceptable in this file because sanitizeConfigFile immediately calls isConfigObject(parsed) to validate the shape. However, document the expectation that callers must validate the result.

📝 Optional: Add JSDoc clarifying the unsafe cast

+/**
+ * Parse JSON text as type T. Callers must validate the returned shape
+ * since this performs an unchecked cast.
+ */
 function parseJson<T>(text: string): T {
   return JSON.parse(text);
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/lib/credential-filter.ts` around lines 15 - 17, The parseJson<T> wrapper
performs an unchecked cast of JSON.parse output to T; update its documentation
(JSDoc) to state this is an unsafe cast and that callers must perform runtime
validation (e.g., callers such as sanitizeConfigFile should call
isConfigObject(parsed) before using the result) so future maintainers know
validation is required before trusting the typed value.

src/lib/onboard-session.ts (1)

32-32: Consider using Set<StepStatus> for stricter typing.

VALID_STEP_STATES is typed as Set<string> but could be Set<StepStatus> since it's derived from STEP_STATES. This would provide better type inference when checking membership.

♻️ Optional stricter typing

-const VALID_STEP_STATES = new Set<string>(STEP_STATES);
+const VALID_STEP_STATES: Set<StepStatus> = new Set(STEP_STATES);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/lib/onboard-session.ts` at line 32, VALID_STEP_STATES is declared as
Set<string> but should use the stronger type Set<StepStatus>; update the
declaration to const VALID_STEP_STATES = new Set<StepStatus>(STEP_STATES as
StepStatus[]) or, better, ensure STEP_STATES is typed as readonly StepStatus[]
and then write const VALID_STEP_STATES = new Set<StepStatus>(STEP_STATES) so
membership checks against VALID_STEP_STATES infer StepStatus instead of string;
reference the symbols VALID_STEP_STATES and STEP_STATES when making this change.

scripts/ts-migration-bulk-fix-prs.ts (1)

119-147: Validate gh JSON before treating it as typed PR metadata.

listTargetPrs() and getPrMetadata() still trust JSON.parse() at the top level, so a non-array/non-object response can still break targets.map(...) or skew the base/maintainer checks before readPrFiles() sanitizes anything. Parsing to unknown and narrowing the outer shape would make this boundary actually type-safe.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@scripts/ts-migration-bulk-fix-prs.ts` around lines 119 - 147, listTargetPrs
and getPrMetadata currently call JSON.parse(...) and assume the result matches
PrListItem[] / PrListItem; change them to parse into unknown and validate the
outer shape before returning: for listTargetPrs, JSON.parse into unknown, assert
it's an array with each item being an object containing a numeric "number"
property (and optionally validate
"title","headRefName","baseRefName","url","maintainerCanModify"), otherwise
throw a clear error; for getPrMetadata, parse into unknown and assert it's an
object with a numeric "number" (and the expected fields, including "files")
before returning; implement small type-guard helpers (e.g., isPrListArray,
isPrObject) and use them in listTargetPrs and getPrMetadata to avoid passing
malformed data downstream from runCapture("gh", ...).

src/lib/sandbox-build-context.ts (1)

8-16: Consider exporting the new types for external consumers.

BuildContextStageResult and BuildContextStats are defined but not exported. If callers need to annotate variables with these return types, they cannot import them directly.

♻️ Suggested change to export types

-type BuildContextStageResult = {
+export type BuildContextStageResult = {
   buildCtx: string;
   stagedDockerfile: string;
 };

-type BuildContextStats = {
+export type BuildContextStats = {
   fileCount: number;
   totalBytes: number;
 };

Alternatively, add them to the existing export block:

 export {
   collectBuildContextStats,
   stageLegacySandboxBuildContext,
   stageOptimizedSandboxBuildContext,
+  type BuildContextStageResult,
+  type BuildContextStats,
 };

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/lib/sandbox-build-context.ts` around lines 8 - 16, The two new types
BuildContextStageResult and BuildContextStats are declared but not exported;
update the file so external callers can import them by exporting these types
(e.g., add export to their declarations or include them in the module's export
block), ensuring the exact type names BuildContextStageResult and
BuildContextStats are exported for use in annotations and return type imports.

test/service-env.test.ts (1)

421-421: ⚠️ Potential issue | 🔴 Critical

Fix unresolved type on Line 421 (BufferEncoding is undefined in this file).

Use the Node child_process options type directly for execFileSync string output instead of BufferEncoding here; this currently fails lint/type checks.

Suggested patch

-import { execSync, execFileSync } from "node:child_process";
+import { execSync, execFileSync, type ExecFileSyncOptionsWithStringEncoding } from "node:child_process";
@@
-        const runOpts: { encoding: BufferEncoding } = { encoding: "utf-8" };
+        const runOpts: ExecFileSyncOptionsWithStringEncoding = { encoding: "utf-8" };

#!/bin/bash
# Verify unresolved symbol usage and expected replacement in this file.
rg -n --type=ts '\bBufferEncoding\b|ExecFileSyncOptionsWithStringEncoding|from "node:child_process"' test/service-env.test.ts

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/service-env.test.ts` at line 421, The test uses an unresolved
BufferEncoding type for runOpts; replace it with the proper Node child_process
options type used by execFileSync (ExecFileSyncOptionsWithStringEncoding) and
import that type from the Node child_process module, then declare runOpts:
ExecFileSyncOptionsWithStringEncoding = { encoding: "utf-8" } so execFileSync
calls get the correct typing; update the import to include
ExecFileSyncOptionsWithStringEncoding and adjust the runOpts symbol accordingly.

test/onboard.test.ts (1)

3288-3288: ESLint configuration does not recognize TypeScript's NodeJS namespace.

The static analysis reports 'NodeJS' is not defined on lines 3288, 3617, 3754, and 3885. These are valid TypeScript type annotations—NodeJS.ProcessEnv is provided by @types/node. The ESLint no-undef rule flags this because it doesn't understand TypeScript's global namespace declarations.

Consider updating ESLint configuration to use @typescript-eslint/no-undef or disable no-undef for TypeScript files (TypeScript's own compiler handles undefined variable detection). Alternatively, you can use typeof process.env as the type annotation to avoid the linting error while maintaining type safety.

Alternative type annotation (optional)

-      const env: NodeJS.ProcessEnv = {
+      const env: typeof process.env = {

Also applies to: 3617-3617, 3754-3754, 3885-3885

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/onboard.test.ts` at line 3288, The lint error comes from using the
TypeScript-only global type NodeJS.ProcessEnv for the test variable env; either
update ESLint to stop using the JS no-undef rule for TS (replace "no-undef" with
"@typescript-eslint/no-undef" or disable "no-undef" for TypeScript files) or
change the type annotation in the tests from "NodeJS.ProcessEnv" to a
TS-friendly alternative such as "typeof process.env" (locate the env
declarations using the "env" variable and the NodeJS.ProcessEnv annotation in
the test file and apply the chosen fix consistently across all instances).

src/lib/onboard-session.ts (1)

298-301: Preserve string-only runtime guarantees in createSession array fields.

At Lines 298-301, array contents are copied without validating element types. Since this is an exported API, runtime callers can still pass mixed arrays and violate the Session contract.

🛡️ Proposed hardening

-    policyPresets: Array.isArray(overrides.policyPresets) ? [...overrides.policyPresets] : null,
-    messagingChannels: Array.isArray(overrides.messagingChannels)
-      ? [...overrides.messagingChannels]
-      : null,
+    policyPresets: readStringArray(overrides.policyPresets ?? undefined),
+    messagingChannels: readStringArray(overrides.messagingChannels ?? undefined),

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/lib/onboard-session.ts` around lines 298 - 301, The current createSession
handling copies overrides.policyPresets and overrides.messagingChannels without
validating element types, which can violate the Session string-only arrays;
update the logic in createSession to sanitize these arrays by checking
Array.isArray(overrides.policyPresets) /
Array.isArray(overrides.messagingChannels) and creating new arrays that only
include string elements (e.g., filter each array with typeof item === 'string')
or set the field to null if no valid strings remain, ensuring the resulting
session object upholds the Session contract.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

test/snapshot.test.ts (1)

40-42: ⚠️ Potential issue | 🟠 Major

Use pathToFileURL(...).href for cross-platform ESM dynamic import.

On Line 41, import(path.join(...)) can fail on Windows (ERR_UNSUPPORTED_ESM_URL_SCHEME) when the path contains a drive-letter absolute path. Convert to a file URL first.

Suggested fix

 import fs from "node:fs";
 import os from "node:os";
 import path from "node:path";
+import { pathToFileURL } from "node:url";
 import { describe, it, expect, afterAll, beforeEach } from "vitest";
@@
 const loadedSandboxState = await import(
-  path.join(REPO_ROOT, "dist", "lib", "sandbox-state.js"),
+  pathToFileURL(path.join(REPO_ROOT, "dist", "lib", "sandbox-state.js")).href,
 );

In Node.js ESM, does dynamic import of a Windows absolute filesystem path require converting the path with pathToFileURL(...).href?

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/snapshot.test.ts` around lines 40 - 42, The dynamic ESM import using
import(path.join(REPO_ROOT, "dist", "lib", "sandbox-state.js")) can fail on
Windows; change the import to convert the filesystem path to a file:// URL first
by calling pathToFileURL on the path you build (using
pathToFileURL(path.join(REPO_ROOT, "dist", "lib", "sandbox-state.js")).href)
before passing it into import so loadedSandboxState receives a proper ESM file
URL; ensure you import pathToFileURL from the 'url' module and use its .href
when calling import.

nemoclaw/src/commands/migration-state.test.ts (1)

932-958: Split malformed externalRoots and malformed warnings into separate tests.

At Line 951, bindings: [1] can fail validation before Line 954 is evaluated, so warnings: [null] may not be independently exercised. Separate tests would make branch coverage and failure diagnosis clearer.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@nemoclaw/src/commands/migration-state.test.ts` around lines 932 - 958, The
test "rejects malformed externalRoots and warnings entries" conflates two
validation failures; split it into two focused tests: one that crafts a snapshot
JSON with the malformed externalRoots (e.g., bindings: [1]) while keeping
warnings valid, and one that crafts a snapshot JSON with valid externalRoots but
malformed warnings (e.g., warnings: [null]); in both tests use addFile to write
the snapshot and call loadSnapshotManifest("/snapshots/snap1") with
expect(...).toThrow(/Invalid snapshot manifest/) to assert rejection so each
validation branch (externalRoots and warnings) is exercised independently.

src/lib/sandbox-state.ts (1)

42-44: Narrow parseJson to unknown to prevent future unsafe casts.

The generic T implies runtime shape safety that this function does not provide. Currently, the only call site (line 777) already uses parseJson<unknown>, which is safe. However, narrowing the function signature to return unknown directly prevents accidental unsafe usage in future changes.

Proposed change

-function parseJson<T>(text: string): T {
+function parseJson(text: string): unknown {
   return JSON.parse(text);
 }
@@
-    const parsed = parseJson<unknown>(readFileSync(manifestPath, "utf-8"));
+    const parsed = parseJson(readFileSync(manifestPath, "utf-8"));

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/lib/sandbox-state.ts` around lines 42 - 44, The parseJson generic should
be narrowed to return unknown to avoid implying runtime type safety: change the
parseJson<T>(...) signature in src/lib/sandbox-state.ts to parseJson(...):
unknown (remove the generic parameter), update its implementation to return
unknown, and adjust any callers if they assumed a concrete T (current call
already uses parseJson<unknown> so no change expected); ensure exported
types/signature reflect the non-generic function and update any tests or call
sites that performed implicit casts to instead explicitly narrow the unknown to
the expected type.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

src/nemoclaw.ts (1)

572-575: Return type inconsistency: function returns empty string but declares string | null.

getActiveGatewayName returns "" when no match is found, but the return type includes | null. While callers handle this correctly (using truthiness checks), the type contract is misleading.

♻️ Align implementation with declared type

 function getActiveGatewayName(output = ""): string | null {
   const match = stripAnsi(output).match(/^\s*Gateway:\s+(.+?)\s*$/m);
-  return match ? match[1].trim() : "";
+  return match ? match[1].trim() : null;
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/nemoclaw.ts` around lines 572 - 575, The function getActiveGatewayName
currently returns an empty string when no match is found but is typed to return
string | null; change the implementation to return null on no match (keep the
declared return type) so callers relying on null vs empty-string behavior remain
correct—locate getActiveGatewayName, keep the stripAnsi(output).match(...) logic
and return match ? match[1].trim() : null.

src/lib/onboard-session.ts (1)

163-167: Minor: ErrnoLike type has an unreachable union member.

The { code?: string | number } branch is never matched because isErrnoException requires error instanceof Error. The type could be simplified, but the current implementation is correct and safe.

♻️ Optional simplification

-type ErrnoLike = Error | { code?: string | number } | null;
-
-function isErrnoException(error: ErrnoLike): error is NodeJS.ErrnoException {
+function isErrnoException(error: unknown): error is NodeJS.ErrnoException {
   return error !== null && error instanceof Error && "code" in error;
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/lib/onboard-session.ts` around lines 163 - 167, The ErrnoLike union
contains an unreachable `{ code?: string | number }` branch because
isErrnoException checks instanceof Error; replace the alias with a single,
accurate type such as NodeJS.ErrnoException | null (or Error | null if you
intentionally want non-errno Errors), update the ErrnoLike declaration
accordingly, and ensure uses of ErrnoLike (and the isErrnoException guard)
continue to compile and narrow correctly; see ErrnoLike and isErrnoException for
the locations to change.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

test/install-preflight.test.ts (1)

1346-1355: Consider extracting a shared env type alias for helper signatures.

The same inline Record<string, string | undefined> appears across multiple helpers. A single alias would reduce repetition and keep updates localized.

♻️ Suggested small refactor

+type EnvOverrides = Record<string, string | undefined>;
+
-function callResolveReleaseTag(fakeBin: string, env: Record<string, string | undefined> = {}) {
+function callResolveReleaseTag(fakeBin: string, env: EnvOverrides = {}) {

-function callInstallerFn(fnCall: string, env: Record<string, string | undefined> = {}) {
+function callInstallerFn(fnCall: string, env: EnvOverrides = {}) {

-function callEnsureSupportedRuntime(fakeBin: string, env: Record<string, string | undefined> = {}) {
+function callEnsureSupportedRuntime(fakeBin: string, env: EnvOverrides = {}) {

Also applies to: 1576-1585, 1851-1864

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/install-preflight.test.ts` around lines 1346 - 1355, Extract a shared
type alias for the environment object (currently repeated as Record<string,
string | undefined>) and use it in helper function signatures like
callResolveReleaseTag (and the other helpers at the noted ranges) to avoid
repetition; add e.g. type Env = Record<string, string | undefined> at the top of
the test file and replace the inline type in callResolveReleaseTag's env
parameter and the other helper functions so all signatures use the new Env
alias.

test/runner.test.ts (1)

70-83: Consider extracting shared spawnSync mock lifecycle boilerplate.

Setup/restore/call-cache-reset logic repeats across multiple tests; consolidating it would reduce noise and lower maintenance risk.

♻️ Possible refactor sketch

+function withMockedSpawnSync(
+  impl: typeof childProcess.spawnSync,
+  runTest: () => void,
+): void {
+  const originalSpawnSync = childProcess.spawnSync;
+  // `@ts-expect-error` intentional partial mock for tests
+  childProcess.spawnSync = impl;
+  try {
+    runTest();
+  } finally {
+    childProcess.spawnSync = originalSpawnSync;
+    delete require.cache[require.resolve(runnerPath)];
+  }
+}

Then each test can focus on assertions instead of repeating lifecycle plumbing.

Also applies to: 92-104, 167-188, 197-218, 504-530

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/runner.test.ts` around lines 70 - 83, Extract the repeated spawnSync
mock setup/teardown into a reusable helper that encapsulates creating the call
array, monkey-patching childProcess.spawnSync via captureSpawnCall, clearing
require.cache for runnerPath, and restoring the original spawnSync; update tests
that use captureSpawnCall, childProcess.spawnSync, runnerPath and require.cache
(e.g., the blocks around run/runInteractive) to call this helper at the start
and a corresponding restore/cleanup function (or use a single
withMockedSpawnSync(fn) wrapper) so each test only focuses on assertions and no
longer repeats lifecycle boilerplate.

src/lib/usage-notice.ts (2)

99-107: Use the new property readers when constructing NoticeConfig.

Lines [99]-[107] still trust imported JSON shape directly for referenceUrl, body, and links. This bypasses the new runtime extraction helpers and can leak malformed values into rendering logic.

Proposed fix

 export function loadUsageNoticeConfig(): NoticeConfig {
-  const rawConfig: NoticeConfigSource = noticeConfig;
+  const rawConfig: object | null =
+    typeof noticeConfig === "object" && noticeConfig !== null ? noticeConfig : null;
   return {
-    version: rawConfig.version || "",
-    title: rawConfig.title || "",
-    referenceUrl: rawConfig.referenceUrl,
-    body: rawConfig.body,
-    links: rawConfig.links,
-    interactivePrompt: rawConfig.interactivePrompt || "",
+    version: readStringProperty(rawConfig, "version") ?? "",
+    title: readStringProperty(rawConfig, "title") ?? "",
+    referenceUrl: readStringProperty(rawConfig, "referenceUrl"),
+    body: readStringArrayProperty(rawConfig, "body"),
+    links: readLinksProperty(rawConfig, "links"),
+    interactivePrompt: readStringProperty(rawConfig, "interactivePrompt") ?? "",
   };
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/lib/usage-notice.ts` around lines 99 - 107, The code constructs a
NoticeConfig by reading fields directly from rawConfig (see
rawConfig.version/rawConfig.title/rawConfig.referenceUrl/rawConfig.body/rawConfig.links/interactivePrompt);
replace direct reads of referenceUrl, body and links with the new runtime
property readers (the helper functions introduced for extracting/validating
referenceUrl, body, and links) so that NoticeConfig uses the sanitized values
instead of rawConfig.referenceUrl/rawConfig.body/rawConfig.links; keep
version/title/interactivePrompt fallback logic as-is but call the readers for
the three properties when building the returned object in the function that
currently returns the NoticeConfig.

---

58-60: Avoid unchecked generic casting in JSON parsing.

Line [58] currently returns JSON.parse(text) as arbitrary T, which is still an unchecked type escape hatch. Prefer unknown and narrow at the call site.

Proposed fix

-function parseJson<T>(text: string): T {
+function parseJson(text: string): unknown {
   return JSON.parse(text);
 }

 export function hasAcceptedUsageNotice(version: string): boolean {
   try {
-    const saved = parseJson<{ acceptedVersion?: string }>(
-      fs.readFileSync(getUsageNoticeStateFile(), "utf8"),
-    );
-    return saved.acceptedVersion === version;
+    const saved = parseJson(fs.readFileSync(getUsageNoticeStateFile(), "utf8"));
+    const acceptedVersion =
+      typeof saved === "object" && saved !== null
+        ? readStringProperty(saved, "acceptedVersion")
+        : undefined;
+    return acceptedVersion === version;
   } catch {
     return false;
   }
 }

Also applies to: 112-115

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/lib/usage-notice.ts` around lines 58 - 60, The parseJson<T> function
currently returns JSON.parse(text) cast to T which is an unchecked generic;
change its signature to return unknown (e.g., parseJson(text: string): unknown)
and remove the generic cast so parsing returns an unknown that must be narrowed
by callers; update all other places that used parseJson<T> (the other occurrence
at lines 112-115) to explicitly validate/transform the unknown into the expected
type (using type guards, schema validation, or explicit casts after checks) so
no unchecked generic escape hatch remains.

src/lib/agent-onboard.ts (1)

18-20: Narrow OnboardContext callback payload types instead of LooseObject.

startRecordedStep is now typed as arbitrary object, but its concrete implementation in src/lib/onboard.ts (Line 7189-7210) expects a small fixed key set. This weakens boundary checks and hides mismatches.

Proposed tightening

 type LooseScalar = string | number | boolean | null | undefined;
 type LooseValue = LooseScalar | LooseObject | LooseValue[];
 type LooseObject = { [key: string]: LooseValue };

+type AgentSetupStepUpdates = {
+  sandboxName?: string | null;
+  provider?: string | null;
+  model?: string | null;
+  policyPresets?: string[] | null;
+};
+
 export interface OnboardContext {
   step: (current: number, total: number, message: string) => void;
   runCaptureOpenshell: (args: string[], opts?: { ignoreError?: boolean }) => string | null;
   openshellShellCommand: (args: string[], options?: { openshellBinary?: string }) => string;
   openshellBinary: string;
   buildSandboxConfigSyncScript: (config: LooseObject) => string;
   writeSandboxConfigSyncFile: (script: string) => string;
   cleanupTempDir: (file: string, prefix: string) => void;
-  startRecordedStep: (stepName: string, updates: LooseObject) => void;
+  startRecordedStep: (stepName: string, updates: AgentSetupStepUpdates) => void;
   skippedStepMessage: (stepName: string, sandboxName: string) => void;
 }

Also applies to: 27-30

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/lib/agent-onboard.ts` around lines 18 - 20, The callback payload types
for OnboardContext are too permissive (using LooseObject) while the concrete
implementation of startRecordedStep expects a fixed key set; define a specific
interface (e.g., StartRecordedStepPayload) describing the exact keys used by the
implementation and replace LooseObject with that interface in the OnboardContext
signature for startRecordedStep, and likewise create/narrow explicit payload
types for the other callbacks mentioned (lines 27-30) instead of reusing
LooseObject; update references to startRecordedStep and OnboardContext so the
type names match the expected fields from src/lib/onboard.ts (lines ~7189-7210).

test/e2e/brev-e2e.test.ts (1)

130-139: Apply requireInstanceName() consistently in command paths

You introduced a strong guard (requireInstanceName()), but ssh, brev create, and rsync still interpolate INSTANCE_NAME directly. Using the guard in these call paths keeps behavior deterministic and avoids accidental "undefined" targeting if preconditions change.

Proposed refactor

 function ssh(
   cmd: string,
   { timeout = 120_000, stream = false }: { timeout?: number; stream?: boolean } = {},
 ): string {
+  const instanceName = requireInstanceName();
   const escaped = cmd.replace(/'/g, "'\\''");
   const stdio = stream ? STREAM_STDIO : CAPTURE_STDIO;
   const result = execSync(
-    `ssh -o StrictHostKeyChecking=no -o LogLevel=ERROR "${INSTANCE_NAME}" '${escaped}'`,
+    `ssh -o StrictHostKeyChecking=no -o LogLevel=ERROR "${instanceName}" '${escaped}'`,
     { encoding: "utf-8", timeout, stdio },
   );
   return stream ? "" : result.trim();
 }

 function createBrevInstance(elapsed: () => string): void {
+  const instanceName = requireInstanceName();
   // ...
   execSync(
     `brev search cpu --min-vcpu ${BREV_MIN_VCPU} --min-ram ${BREV_MIN_RAM} --min-disk ${BREV_MIN_DISK} --provider ${BREV_PROVIDER} --sort price | ` +
-      `brev create ${INSTANCE_NAME} --startup-script @${setupScriptPath} --detached`,
+      `brev create ${instanceName} --startup-script @${setupScriptPath} --detached`,
     { encoding: "utf-8", timeout: 180_000, stdio: PIPE_INPUT_STDIO },
   );

-  execSync(
+  const instanceName = requireInstanceName();
+  execSync(
     `rsync -az --delete --exclude node_modules --exclude .git --exclude dist --exclude .venv "${REPO_DIR}/" "${INSTANCE_NAME}:${resolvedRemoteDir}/"`,
+    `rsync -az --delete --exclude node_modules --exclude .git --exclude dist --exclude .venv "${REPO_DIR}/" "${instanceName}:${resolvedRemoteDir}/"`,
     { encoding: "utf-8", timeout: 120_000 },
   );

Also applies to: 367-369, 412-413

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/e2e/brev-e2e.test.ts` around lines 130 - 139, The ssh wrapper (function
ssh) and other call sites that interpolate INSTANCE_NAME directly (e.g., the ssh
command, brev create invocation, and rsync usage) must call
requireInstanceName() and use its return value instead of the raw INSTANCE_NAME
to ensure the guard is applied; update the ssh function to call const target =
requireInstanceName() and use target in the ssh command string, and similarly
replace direct INSTANCE_NAME uses in the brev create and rsync call sites with
requireInstanceName() to make behavior deterministic and prevent accidental
"undefined" targets.

src/lib/deploy.ts (1)

10-20: Keep the injected process APIs aligned with Node’s real types.

ExecLikeOptions plus spawnSync(...): void reintroduces a broad escape hatch here: misspelled options still type-check, and the injected spawnSync no longer exposes status/error/stdout. That works against the PR’s type-tightening goal. Prefer the Node option/result types directly, or a narrow Pick of them, on these injected dependencies.

Also applies to: 64-65

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/lib/deploy.ts` around lines 10 - 20, The injected types are too loose:
replace the ad-hoc ExecLikeValue/ExecLikeOptions and the void-returning
spawnSync with Node’s real child_process types so misspelled options fail and
results expose status/error/stdout/stderr; specifically, change uses of
ExecLikeOptions to reference SpawnSyncOptions (or
SpawnSyncOptionsWithStringEncoding) and make the injected spawnSync have the
signature returning SpawnSyncReturns<Buffer | string> (or a narrowed Pick of
that type exposing status, error, stdout, stderr) so functions like spawnSync,
ExecLikeOptions, and ExecLikeValue use the built-in Node types rather than
generic object/string unions.

src/lib/sandbox-state.ts (1)

423-427: Consider using imported unlinkSync instead of require("node:fs").

The file already imports from node:fs (line 13-23) but uses require("node:fs").unlinkSync here. For consistency with the rest of the file, consider adding unlinkSync to the destructured imports.

♻️ Proposed fix

Add unlinkSync to the imports at the top of the file:

 import {
   chmodSync,
   existsSync,
   lstatSync,
   mkdirSync,
   readdirSync,
   readFileSync,
   readlinkSync,
   rmSync,
+  unlinkSync,
   writeFileSync,
 } from "node:fs";

Then use it directly:

-          try {
-            require("node:fs").unlinkSync(fullPath);
-          } catch {
+          try {
+            unlinkSync(fullPath);
+          } catch {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/lib/sandbox-state.ts` around lines 423 - 427, Replace the inline
require("node:fs").unlinkSync usage with the imported unlinkSync for
consistency: add unlinkSync to the existing destructured imports from node:fs at
the top of the file, then change the try block that currently calls
require("node:fs").unlinkSync(fullPath) to call unlinkSync(fullPath) instead
(keeping the existing try/catch and fullPath variable intact).

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes