test(core): cover env-prefix substitution + buildShellExecWarnings dual-check (#4386 R4)

LaZzyMan · LaZzyMan · commit 456291fd4576 · 2026-05-25T12:01:53.000+08:00
Round-4 test-coverage findings from wenshao: 1. **env-prefix integration test in `shell.test.ts`** (cid 3293075622) — the `command substitution warning (issue #4093)` describe block in `shell.test.ts` covered `$()`, backticks, `<()`, `>()`, and the no-warning case, but had no test for the shape where `stripShellWrapper` strips the env-prefix + `bash -c` wrapper to yield a substitution-free inner command (`echo ok`) while the raw command has substitution in the env assignment (`FOO=$(cat secret.txt) bash -c 'echo ok'`). This is the exact shape that exercises the `|| detectCommandSubstitution(rawCommand)` branch of `buildShellExecWarnings` via integration through `getConfirmationDetails`. Without this test, removing the `||` clause wouldn't regress any case here. 2. **`buildShellExecWarnings` unit tests in `shell-utils.test.ts`** (cid 3293078758 second half) — round 3 extracted `buildShellExecWarnings` as an exported helper but added no direct unit test. Added a 5-case describe block covering: no substitution, stripped-form substitution, the env-prefix dual-check case (with a sanity-check that the stripped form actually lacks `$(` to make the dual-check load-bearing), backticks, and process substitution. These complement the integration tests in shell.test.ts / monitor.test.ts which exercise the helper through the tool confirmation paths. Closes wenshao R4 findings: env-prefix integration test (cid 3293075622), `buildShellExecWarnings` direct unit coverage (cid 3293078758 — second half).
diff --git a/packages/core/src/tools/shell.test.ts b/packages/core/src/tools/shell.test.ts
@@ -5035,6 +5035,26 @@ describe('ShellTool', () => {
         // `warnings` should be omitted entirely when there's nothing to flag.
         expect(details.warnings).toBeUndefined();
       });
+
+      // Regression coverage for PR #4386 R4 (cid 3293075622): the
+      // `|| detectCommandSubstitution(rawCommand)` branch of
+      // `buildShellExecWarnings` only fires for shapes where
+      // `stripShellWrapper` yields a substitution-free inner command
+      // (here `echo ok`) but the raw command has substitution in the
+      // env-prefix. Without this case, removing the `||` clause would
+      // not regress any test in this describe block.
+      it('surfaces a warning for substitution in the env-prefix of a shell wrapper', async () => {
+        const invocation = shellTool.build({
+          command: `FOO=$(cat secret.txt) bash -c 'echo ok'`,
+          is_background: false,
+        });
+        const details = (await invocation.getConfirmationDetails(
+          new AbortController().signal,
+        )) as { warnings?: string[] };
+
+        expect(details.warnings).toBeDefined();
+        expect(details.warnings?.[0]).toMatch(/command substitution/i);
+      });
     });
   });
 
diff --git a/packages/core/src/utils/shell-utils.test.ts b/packages/core/src/utils/shell-utils.test.ts
@@ -6,8 +6,10 @@
 
 import { expect, describe, it, beforeEach, vi, afterEach } from 'vitest';
 import {
+  buildShellExecWarnings,
   checkArgumentSafety,
   checkCommandPermissions,
+  COMMAND_SUBSTITUTION_WARNING,
   escapeShellArg,
   getCommandRoots,
   getShellConfiguration,
@@ -1098,3 +1100,54 @@ describe('checkArgumentSafety', () => {
     });
   });
 });
+
+// Regression coverage for PR #4386 R4 (cid 3293078758): the dual-check
+// branch of `buildShellExecWarnings` — where the stripped form has no
+// substitution but the raw command does (e.g. env-prefix wrapped in
+// `bash -c`) — was untested. Without coverage, removing the
+// `|| detectCommandSubstitution(rawCommand)` clause would not regress
+// any test in this file.
+describe('buildShellExecWarnings', () => {
+  it('returns undefined when neither stripped nor raw command has substitution', () => {
+    expect(
+      buildShellExecWarnings('npm install', 'npm install'),
+    ).toBeUndefined();
+  });
+
+  it('returns the substitution warning when the stripped command has $()', () => {
+    const result = buildShellExecWarnings(
+      'echo $(cat secret)',
+      'echo $(cat secret)',
+    );
+    expect(result).toEqual([COMMAND_SUBSTITUTION_WARNING]);
+  });
+
+  it('returns the substitution warning when the raw command has substitution but the stripped form does not (env-prefix wrapper case)', () => {
+    // `stripShellWrapper("FOO=$(cat secret) bash -c 'echo ok'")` yields
+    // `echo ok` — no substitution — so the `|| rawCommand` branch is the
+    // only thing that fires the warning here.
+    const raw = `FOO=$(cat secret) bash -c 'echo ok'`;
+    const stripped = stripShellWrapper(raw);
+    // Sanity-check the precondition before asserting on the helper.
+    expect(stripped).not.toContain('$(');
+
+    expect(buildShellExecWarnings(stripped, raw)).toEqual([
+      COMMAND_SUBSTITUTION_WARNING,
+    ]);
+  });
+
+  it('returns the warning for backtick substitution in either input', () => {
+    expect(buildShellExecWarnings('echo `whoami`', 'echo `whoami`')).toEqual([
+      COMMAND_SUBSTITUTION_WARNING,
+    ]);
+  });
+
+  it('returns the warning for process substitution <(...)', () => {
+    expect(
+      buildShellExecWarnings(
+        'diff <(ls /a) <(ls /b)',
+        'diff <(ls /a) <(ls /b)',
+      ),
+    ).toEqual([COMMAND_SUBSTITUTION_WARNING]);
+  });
+});
