test/fetch-guard-patch-regression.test.ts (2)

87-92: ⚡ Quick win

Run the generated patch script in strict bash mode.

Line 88 writes a script without set -euo pipefail; intermediate failures inside a multi-command block can be missed and still report success.

Proposed change

-    fs.writeFileSync(scriptPath, ["#!/usr/bin/env bash", command].join("\n"), { mode: 0o700 });
+    fs.writeFileSync(
+      scriptPath,
+      ["#!/usr/bin/env bash", "set -euo pipefail", command].join("\n"),
+      { mode: 0o700 },
+    );

🤖 Prompt for AI Agents

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

In `@test/fetch-guard-patch-regression.test.ts` around lines 87 - 92, The
generated test script is missing strict bash options, so update the script
content written by fs.writeFileSync (the script created at scriptPath) to
include a strict mode header (e.g., add "set -euo pipefail" immediately after
the shebang) so any intermediate failures in multi-command blocks cause the
script to fail; keep the shebang (#!/usr/bin/env bash) and then insert the
strict flags before writing the command string that is later executed via
spawnSync.

---

83-87: ⚡ Quick win

Use replaceAll for the OpenClaw dist-path rewrite.

Line 86 only rewrites the first occurrence. If the Dockerfile patch block gains another matching path, the test can hit the real absolute path unexpectedly.

Proposed change

-    ).replace("/usr/local/lib/node_modules/openclaw/dist", dist);
+    ).replaceAll("/usr/local/lib/node_modules/openclaw/dist", dist);

🤖 Prompt for AI Agents

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

In `@test/fetch-guard-patch-regression.test.ts` around lines 83 - 87, The test
constructs the Docker command into the variable command using
dockerRunCommandBetween and then calls .replace to rewrite the OpenClaw dist
path, but .replace only replaces the first occurrence; change that call to use a
global replace (e.g., replaceAll) so every instance of
"/usr/local/lib/node_modules/openclaw/dist" is rewritten to the test-local dist
value; update the expression that assigns command (the code calling .replace) to
use replaceAll with the same search string and the dist variable so no stray
absolute paths remain.

test/sandbox-provisioning.test.ts (1)

166-170: 💤 Low value

Consider making whitespace normalization more robust.

The .replace('python3 -c " ', 'python3 -c "') fix depends on exact Dockerfile formatting. If the Dockerfile's line continuation style changes, this could silently break the test.

Consider normalizing whitespace more broadly in dockerRunCommandBetween, or adding a comment explaining why this specific fix is needed.

🤖 Prompt for AI Agents

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

In `@test/sandbox-provisioning.test.ts` around lines 166 - 170, The test's fragile
post-processing uses a specific string replace after calling
dockerRunCommandBetween, which will break if spacing/line-continuation in the
Dockerfile changes; instead update dockerRunCommandBetween to normalize
whitespace and line-continuations (collapse consecutive spaces, trim
leading/trailing spaces, and join backslash-continuation lines) so returned
command is stable, or at minimum replace the ad-hoc .replace call with a broader
normalization (e.g., collapse multiple spaces around python3 -c and remove stray
space after the opening quote) and add a short comment in the test referencing
dockerRunCommandBetween to explain why normalization is required.

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

202-238: 🏗️ Heavy lift

These tests still key off nemoclaw-start.sh comment anchors.

Both cases locate a block with indexOf(...) and execute src.slice(...). That means a benign reorder or comment rewrite in the entrypoint can fail the test without any behavioral regression, which keeps the suite coupled to source shape. Prefer driving the real script with controlled env/paths, or extracting these shell helpers into directly invokable files.

Also applies to: 323-367

🤖 Prompt for AI Agents

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

In `@test/service-env.test.ts` around lines 202 - 238, The test currently relies
on fragile string anchors (start/end, src.slice) and writes a temporary shell
snippet (tmpFile) to assert SSL_CERT_FILE handling; instead, change the test to
drive the real entrypoint or extracted helper directly by executing the actual
nemoclaw-start.sh (or refactor the shell helper into a committed, invokable
script) with controlled environment and temporary paths, replacing the
indexOf/src.slice logic and temporary concatenation (writeFileSync/execFileSync)
with a direct execFileSync call against the real script (passing a fake CA
bundle path via env and asserting output), or load the shell helper file and
invoke it as a standalone script so the test verifies behavior not source
layout.

test/sandbox-init.test.ts (1)

512-543: 🏗️ Heavy lift

This still depends on nemoclaw-start.sh source layout.

Locating the block via _SANDBOX_INIT= and # Harden: limit process count means a harmless refactor of the entrypoint can break the test even if it still sources sandbox-init.sh correctly. That keeps the test coupled to production text shape; a more robust version would invoke the real script path (or a thin wrapper around it) and assert the sentinel effect without slicing by marker text.

scripts/find-source-shape-tests.ts (1)

77-85: ⚡ Quick win

Use readdirSync(..., { withFileTypes: true }) to avoid an extra statSync per entry.

This script runs in local hooks/CI paths; cutting one syscall per directory entry improves scan latency with no behavior change.

Suggested refactor

-  for (const entry of readdirSync(dir)) {
-    const abs = join(dir, entry);
+  for (const entry of readdirSync(dir, { withFileTypes: true })) {
+    const abs = join(dir, entry.name);
     if (isSkippedPath(abs)) continue;

-    const stats = statSync(abs);
-    if (stats.isDirectory()) {
+    if (entry.isDirectory()) {
       yield* walkFiles(abs);
-    } else if (stats.isFile()) {
+    } else if (entry.isFile()) {
       yield abs;
     }
   }

🤖 Prompt for AI Agents

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

In `@scripts/find-source-shape-tests.ts` around lines 77 - 85, The loop in
walkFiles uses readdirSync(dir) plus statSync(abs) per entry; replace it by
calling readdirSync(dir, { withFileTypes: true }) and iterate Dirent objects so
you can avoid statSync: compute abs with join(dir, dirent.name), continue when
isSkippedPath(abs), then use dirent.isDirectory() to recurse with yield*
walkFiles(abs) or dirent.isFile() to yield abs. Update references in
scripts/find-source-shape-tests.ts (walkFiles) accordingly and remove the
now-unnecessary statSync call.

test/credential-exposure.test.ts (1)

145-167: ⚡ Quick win

Extend the secret non-forwarding assertion to plugin env builder too.

Line 153 only verifies buildCliSubprocessEnv(). If plugin env filtering regresses to include NVIDIA_API_KEY, this test still passes. Please assert both builders here, similar to the TLS parity tests.

Suggested patch

   try {
     process.env.NVIDIA_API_KEY = "nvapi-secret-should-not-leak";
     process.env.PATH = `/tmp/nemoclaw-fake-bin:${process.env.PATH || ""}`;
-    const env = buildCliSubprocessEnv();
-    expect(env.NVIDIA_API_KEY).toBeUndefined();
-    expect(env.PATH).toContain("/tmp/nemoclaw-fake-bin");
+    for (const buildSubprocessEnv of [buildCliSubprocessEnv, buildPluginSubprocessEnv]) {
+      const env = buildSubprocessEnv();
+      expect(env.NVIDIA_API_KEY).toBeUndefined();
+      expect(env.PATH).toContain("/tmp/nemoclaw-fake-bin");
+    }
   } finally {

Based on learnings: Security-sensitive code paths (credential sanitization, SSRF validation, policy enforcement) require extra test coverage to maintain security integrity.

🤖 Prompt for AI Agents

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

In `@test/credential-exposure.test.ts` around lines 145 - 167, The current test
only checks buildCliSubprocessEnv for leaking NVIDIA_API_KEY; update the test to
also call buildPluginSubprocessEnv() and assert the same expectations
(expect(env.NVIDIA_API_KEY).toBeUndefined() and
expect(env.PATH).toContain("/tmp/nemoclaw-fake-bin")) so both subprocess env
builders are covered; locate the assertions around buildCliSubprocessEnv in the
"subprocess env builder does not spread full process.env into subprocesses" test
and duplicate the checks for buildPluginSubprocessEnv.

Fix all unresolved CodeRabbit comments on this PR:

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

Fix all unresolved CodeRabbit comments on this PR:

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

test/shellquote-sandbox.test.ts (1)

163-169: 💤 Low value

Consider simplifying type narrowing with non-null assertion.

The as unknown as double cast is a workaround for TypeScript not narrowing after Vitest's expect().not.toBeNull(). Since the assertion on line 163 already validates non-null, you can use the non-null assertion operator directly for cleaner code.

♻️ Proposed simplification

       expect(captured).not.toBeNull();
-      const capturedRun = captured as unknown as {
-        command: string[];
-        opts: { ignoreError?: boolean };
-      };
-      expect(capturedRun.command).toEqual(["/tmp/custom-openshell", "--version"]);
-      expect(capturedRun.opts.ignoreError).toBe(true);
+      expect(captured!.command).toEqual(["/tmp/custom-openshell", "--version"]);
+      expect(captured!.opts.ignoreError).toBe(true);

🤖 Prompt for AI Agents

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

In `@test/shellquote-sandbox.test.ts` around lines 163 - 169, The test uses a
double cast "as unknown as" to coerce `captured` into the `capturedRun` shape
after `expect(captured).not.toBeNull()`; replace that double cast by using the
non-null assertion operator to simplify narrowing (i.e., assign `capturedRun`
from `captured!` and keep the shape `{ command: string[]; opts: { ignoreError?:
boolean } }`), so subsequent assertions on `capturedRun.command` and
`capturedRun.opts.ignoreError` remain valid.

Fix all unresolved CodeRabbit comments on this PR:

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

test/wsl2-probe-timeout.test.ts (1)

79-80: ⚡ Quick win

Fail fast when scripted probe responses are exhausted.

Line 79 and Line 155 currently default to success/last-result when extra calls happen. That can mask unintended retries. Throwing when the scripted queue is exceeded makes these tests much more regression-sensitive.

Proposed patch

-        const status = statuses[index++] ?? 0;
+        const status = statuses[index++];
+        if (status === undefined) {
+          throw new Error(`Unexpected extra probe invocation at call ${index}`);
+        }
@@
-        return results[index++] ?? results[results.length - 1];
+        const next = results[index++];
+        if (!next) {
+          throw new Error(`Unexpected extra probe invocation at call ${index}`);
+        }
+        return next;

Also applies to: 155-156

🤖 Prompt for AI Agents

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

In `@test/wsl2-probe-timeout.test.ts` around lines 79 - 80, The test currently
masks extra probe calls by defaulting to a success/last-result when the scripted
arrays are exhausted; update the logic around the scripted response consumption
(the statuses array access using index and the results/lastResult access later)
to throw an error when index is out of bounds instead of using the
nullish/default fallback. Concretely, replace the expressions that use
"statuses[index++] ?? 0" and the similar "results/lastResult" fallback with an
explicit bounds check that throws a clear error like "No more scripted probe
responses" when index >= statuses.length (and similarly for results.length)
before advancing the index and returning the scripted value.

test/nemoclaw-start.test.ts (1)

576-582: 💤 Low value

Consider consolidating duplicate helper functions.

extractShellFunction (lines 576-582, 678-684) and extractFunction (lines 1221-1227) duplicate the top-level extractShellFunctionFromSource (lines 44-50). You could reuse the existing helper:

-  function extractShellFunction(name: string): string {
-    const match = src.match(new RegExp(`${name}\\(\\) \\{([\\s\\S]*?)^\\}`, "m"));
-    if (!match) {
-      throw new Error(`Expected ${name} in scripts/nemoclaw-start.sh`);
-    }
-    return `${name}() {${match[1]}\n}`;
-  }
+  const extractShellFunction = (name: string) => extractShellFunctionFromSource(src, name);

Regarding the static analysis ReDoS warnings: these are false positives since name always comes from hardcoded string literals in the test code (e.g., "apply_model_override", "_read_gateway_token").

🤖 Prompt for AI Agents

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

In `@test/nemoclaw-start.test.ts` around lines 576 - 582, There are duplicate
helpers—extractShellFunction and extractFunction—that replicate
extractShellFunctionFromSource; refactor tests to call the existing
extractShellFunctionFromSource instead of redefining those functions (search for
extractShellFunction and extractFunction and replace their bodies/calls with
calls to extractShellFunctionFromSource), and leave a comment near the call
sites noting that the ReDoS static-analysis warnings are false positives because
name is a hardcoded literal (e.g., "apply_model_override",
"_read_gateway_token").

Fix all unresolved CodeRabbit comments on this PR:

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