fix: send redirections and expansions to the real shell (#46)

experiments/issue-46-redirection-parity.mjs

@@ -0,0 +1,108 @@
+#!/usr/bin/env node
+// Issue #46: "silent failure" class of bugs.
+//
+// The report was about `git push ... 2>&1` returning exit code 0 with no
+// output. The root cause is broader: a command whose first word is a built-in
+// (virtual) command is dispatched to that built-in with the shell operators
+// left in place as literal arguments, so the redirection silently does
+// nothing. This script diffs command-stream against /bin/sh so any divergence
+// in exit code, stdout, or files written is visible.
+import { execSync, spawnSync } from 'child_process';
+import { mkdtempSync, rmSync, writeFileSync } from 'fs';
+import { tmpdir } from 'os';
+import { join } from 'path';
+import { $ } from '../js/src/$.mjs';
+
+const CASES = [
+  // The originally reported shape.
+  'exit 3 2>&1',
+  'sh -c "echo err 1>&2; exit 7" 2>&1',
+  'cd . && sh -c "exit 5" 2>&1',
+  'git push origin nonexistent-remote-branch 2>&1',
+  // Output redirection on a built-in.
+  'echo hello > out.txt',
+  'echo hello >> out.txt',
+  'echo hello 1> out.txt',
+  'pwd > out.txt',
+  'true > out.txt',
+  'seq 1 3 > out.txt',
+  'basename /a/b > out.txt',
+  // stderr redirection on a built-in.
+  'echo hello 2> err.txt',
+  'ls /definitely/missing/path 2>/dev/null',
+  'ls /definitely/missing/path 2>&1',
+  'cat /definitely/missing/path 2>/dev/null',
+  // Input redirection.
+  'cat < seed.txt',
+  'cat 0< seed.txt',
+  // Redirection combined with operators.
+  'echo a > out.txt && echo b >> out.txt',
+  'false > out.txt || echo fallback > out.txt',
+  // Redirection targets that must not be treated as arguments.
+  'echo one two > out.txt',
+  // Quoted redirection characters are literal, not operators.
+  'echo "a > b"',
+  "echo 'a > b'",
+];
+
+let failures = 0;
+for (const cmd of CASES) {
+  const dirSh = mkdtempSync(join(tmpdir(), 'sh-'));
+  const dirCs = mkdtempSync(join(tmpdir(), 'cs-'));
+  for (const d of [dirSh, dirCs]) {
+    writeFileSync(join(d, 'seed.txt'), 'seeded\n');
+  }
+
+  const sh = spawnSync('/bin/sh', ['-c', cmd], {
+    cwd: dirSh,
+    encoding: 'utf8',
+  });
+  const expected = { code: sh.status, stdout: sh.stdout };
+
+  let actual;
+  try {
+    const r = await $({ cwd: dirCs, mirror: false })`${{ raw: cmd }}`;
+    actual = { code: r.code, stdout: r.stdout };
+  } catch (e) {
+    actual = { code: e.code, stdout: e.stdout };
+  }
+
+  const shFiles = execSync('ls -1', { cwd: dirSh, encoding: 'utf8' }).trim();
+  const csFiles = execSync('ls -1', { cwd: dirCs, encoding: 'utf8' }).trim();
+  // The two runs use differently-named temp dirs, so `pwd` output must be
+  // normalised before the captured files can be compared.
+  const shOut = readAll(dirSh).replaceAll(dirSh, '<CWD>');
+  const csOut = readAll(dirCs).replaceAll(dirCs, '<CWD>');
+
+  const same =
+    expected.code === actual.code &&
+    expected.stdout.replaceAll(dirSh, '<CWD>') ===
+      actual.stdout.replaceAll(dirCs, '<CWD>') &&
+    shFiles === csFiles &&
+    shOut === csOut;
+  if (!same) {
+    failures++;
+  }
+  console.log(`${same ? 'OK  ' : 'DIFF'} ${JSON.stringify(cmd)}`);
+  if (!same) {
+    console.log(
+      `      sh: code=${expected.code} stdout=${JSON.stringify(expected.stdout)} files=${JSON.stringify(shFiles)} contents=${JSON.stringify(shOut)}`
+    );
+    console.log(
+      `      cs: code=${actual.code} stdout=${JSON.stringify(actual.stdout)} files=${JSON.stringify(csFiles)} contents=${JSON.stringify(csOut)}`
+    );
+  }
+  rmSync(dirSh, { recursive: true, force: true });
+  rmSync(dirCs, { recursive: true, force: true });
+}
+
+function readAll(dir) {
+  return execSync('for f in *; do echo "== $f"; cat "$f"; done', {
+    cwd: dir,
+    encoding: 'utf8',
+    shell: '/bin/sh',
+  });
+}
+
+console.log(`\n${failures} divergence(s) out of ${CASES.length}`);
+process.exit(failures === 0 ? 0 : 1);

js/.changeset/issue-46-redirection-sh-parity.md

@@ -0,0 +1,17 @@
+---
+'command-stream': patch
+---
+
+Route redirections and expansions to the system shell so they are no longer
+silently swallowed by built-in commands (#46).
+
+`needsRealShell()` was only consulted when the command also contained `&&`,
+`||`, `;`, `&` or `(`, and redirection characters are not part of that operator
+set. A command whose first word is a built-in (`echo`, `cat`, `true`, `ls`,
+`seq`, ...) was therefore dispatched in-process with the operators passed
+through as literal arguments: `echo hello > out.txt` printed `hello > out.txt`
+and wrote no file, and `git push origin main 2>&1` reported exit code 0 with
+empty output even when the push had failed. The verdict is now independent of
+the operator set, and `>`, `>>`, `<`, `2>`, `&>`, `>&` and `<<` are recognised
+as requiring a real shell, matching `/bin/sh` behaviour. Redirection characters
+inside quotes stay literal, as in `sh`.

js/src/$.process-runner-execution.mjs

@@ -863,9 +863,12 @@ async function handleShellMode(runner, deps) {
   const useShellOps = shouldUseShellOperators(runner, command);
   // Backslash escapes are removed by a real shell but not by our lightweight
   // tokenizer, so such commands always go to the system shell rather than to
-  // the built-in commands (issue #49).
-  const requiresRealShell =
-    (useShellOps && needsRealShell(command)) || hasShellEscapes(command);
+  // the built-in commands (issue #49). needsRealShell() is asked
+  // unconditionally: gating it on useShellOps made the verdict depend on
+  // whether the command happened to also contain `&&`, `||`, `;`, `&` or `(`,
+  // so `echo hi > f` handed `>` and `f` to the built-in `echo` as two literal
+  // arguments while `echo $(echo hi) > f` did not (issue #46).
+  const requiresRealShell = needsRealShell(command) || hasShellEscapes(command);
 
   trace(
     'ProcessRunner',

js/src/shell-parser.mjs

@@ -545,21 +545,26 @@ function isSingleAmpersand(command, index) {
 
 function isUnsupportedUnquotedFeature(command, index) {
   const char = command[index];
-  if ('`$~*?['.includes(char) || isSingleAmpersand(command, index)) {
+  if ('`$~*?[<>'.includes(char) || isSingleAmpersand(command, index)) {
     return true;
   }
 
-  const remainder = command.slice(index);
-  return (
-    remainder.startsWith('2>') ||
-    remainder.startsWith('&>') ||
-    remainder.startsWith('>&') ||
-    remainder.startsWith('<<')
-  );
+  return false;
 }
 
 /**
- * Check if a command needs shell features we don't handle
+ * Check if a command needs shell features we don't handle.
+ *
+ * Redirection (`>`, `>>`, `<`, `2>`, `&>`, `<<`, ...) counts as such a
+ * feature. The built-in (virtual) commands take a plain argument list, so a
+ * redirection left in that list is passed through as a literal argument:
+ * `echo hello > out.txt` would print `hello > out.txt` and write no file, and
+ * `git push ... 2>&1` would report success while nothing was pushed. Sending
+ * the whole command to the system shell is the only way to get exactly the
+ * POSIX result (issue #46).
+ *
+ * @param {string} command - The full command string
+ * @returns {boolean} true when the command must be run by a real shell
  */
 export function needsRealShell(command) {
   let quote = null;

js/tests/redirection-silent-failure.test.mjs

@@ -0,0 +1,174 @@
+// Issue #46: a command whose first word is a built-in (virtual) command used to
+// be dispatched to that built-in with the shell operators still in the argument
+// list. `git push ... 2>&1` reported exit code 0 and no output while nothing was
+// pushed, and `echo hello > out.txt` printed `hello > out.txt` instead of
+// writing the file.
+//
+// needsRealShell() already recognised those constructs, but the caller only
+// consulted it when the command also contained `&&`, `||`, `;`, `&` or `(`.
+// These tests pin the behaviour to /bin/sh, which is the contract: anything the
+// built-ins cannot reproduce exactly goes to the system shell.
+import { test, expect, describe, beforeEach, afterEach } from 'bun:test';
+import { beforeTestCleanup, afterTestCleanup } from './test-cleanup.mjs';
+import { isWindows } from './test-helper.mjs';
+import { $ } from '../src/$.mjs';
+import { needsRealShell } from '../src/shell-parser.mjs';
+import { spawnSync } from 'child_process';
+import { promises as fs } from 'fs';
+import path from 'path';
+import os from 'os';
+
+/** Run `command` in a scratch directory and report what /bin/sh does with it. */
+function runInSh(command, cwd) {
+  const sh = spawnSync('/bin/sh', ['-c', command], { cwd, encoding: 'utf8' });
+  return { code: sh.status, stdout: sh.stdout, stderr: sh.stderr };
+}
+
+/** List the scratch directory as `name:contents` pairs, sorted by name. */
+async function snapshot(dir) {
+  const names = (await fs.readdir(dir)).sort();
+  const entries = await Promise.all(
+    names.map(
+      async (name) =>
+        `${name}:${await fs.readFile(path.join(dir, name), 'utf8')}`
+    )
+  );
+  return entries;
+}
+
+describe('Redirection is never handed to a built-in as an argument (issue #46)', () => {
+  let shDir;
+  let csDir;
+
+  beforeEach(async () => {
+    await beforeTestCleanup();
+    const base = await fs.mkdtemp(path.join(os.tmpdir(), 'issue46-'));
+    shDir = path.join(base, 'sh');
+    csDir = path.join(base, 'cs');
+    await fs.mkdir(shDir);
+    await fs.mkdir(csDir);
+    await fs.writeFile(path.join(shDir, 'seed.txt'), 'seeded\n');
+    await fs.writeFile(path.join(csDir, 'seed.txt'), 'seeded\n');
+  });
+
+  afterEach(async () => {
+    await afterTestCleanup();
+    if (shDir) {
+      await fs.rm(path.dirname(shDir), { recursive: true, force: true });
+    }
+  });
+
+  // Every command here starts with a word that is also a built-in, which is
+  // exactly the shape that used to bypass the shell.
+  const parityCases = [
+    'echo hello > out.txt',
+    'echo hello >> out.txt',
+    'echo hello 1> out.txt',
+    'echo one two > out.txt',
+    'echo hello 2> err.txt',
+    'true > out.txt',
+    'seq 1 3 > out.txt',
+    'basename /a/b > out.txt',
+    'cat < seed.txt',
+    'cat 0< seed.txt',
+    'cat /definitely/missing/path 2>/dev/null',
+    'ls /definitely/missing/path 2>/dev/null',
+    'ls /definitely/missing/path 2>&1',
+    'exit 3 2>&1',
+    'echo a > out.txt && echo b >> out.txt',
+    'false > out.txt || echo fallback > out.txt',
+    // Quoted redirection characters are literal in sh, so they must stay
+    // literal here too - the fix must not over-reach.
+    'echo "a > b"',
+    "echo 'a > b'",
+  ];
+
+  for (const command of parityCases) {
+    test.skipIf(isWindows)(`matches /bin/sh for: ${command}`, async () => {
+      const expected = runInSh(command, shDir);
+
+      let actual;
+      try {
+        const result = await $({
+          cwd: csDir,
+          mirror: false,
+        })`${{ raw: command }}`;
+        actual = { code: result.code, stdout: result.stdout };
+      } catch (error) {
+        actual = { code: error.code, stdout: error.stdout };
+      }
+
+      expect(actual.stdout).toBe(expected.stdout);
+      expect(actual.code).toBe(expected.code);
+      expect(await snapshot(csDir)).toEqual(await snapshot(shDir));
+    });
+  }
+
+  // Expansions reached the built-ins through the same gap: `echo $(echo hi)`
+  // worked only because `(` counted as a shell operator, while `echo $HOME`
+  // printed the literal text.
+  const expansionCases = [
+    'echo $HOME',
+    'echo *',
+    'echo ~',
+    'echo `echo hi`',
+    'echo $(echo hi)',
+  ];
+
+  for (const command of expansionCases) {
+    test.skipIf(isWindows)(`expands like /bin/sh for: ${command}`, async () => {
+      const expected = runInSh(command, shDir);
+      const result = await $({
+        cwd: csDir,
+        mirror: false,
+      })`${{ raw: command }}`;
+      expect(result.stdout).toBe(expected.stdout);
+      expect(result.code).toBe(expected.code);
+    });
+  }
+
+  test.skipIf(isWindows)(
+    'a failing git push reports the failure through 2>&1',
+    async () => {
+      // A local path that is not a repository fails the same way everywhere and
+      // keeps the test off the network.
+      const repo = path.join(csDir, 'repo');
+      await fs.mkdir(repo);
+      await $({ cwd: repo, mirror: false })`git init -q`;
+      await $({
+        cwd: repo,
+        mirror: false,
+      })`git config user.email test@example.com`;
+      await $({ cwd: repo, mirror: false })`git config user.name Test`;
+      await fs.writeFile(path.join(repo, 'file.txt'), 'content\n');
+      await $({ cwd: repo, mirror: false })`git add file.txt`;
+      await $({ cwd: repo, mirror: false })`git commit -q -m initial`;
+      await $({
+        cwd: repo,
+        mirror: false,
+      })`git remote add origin ${path.join(csDir, 'no-such-remote')}`;
+
+      const result = await $({
+        cwd: repo,
+        mirror: false,
+      })`git push origin HEAD 2>&1`;
+
+      // Before the fix this was code 0 with an empty stdout: the whole command
+      // had been swallowed by the virtual `git`-less dispatch path.
+      expect(result.code).not.toBe(0);
+      expect(result.stdout).toContain('fatal:');
+    }
+  );
+
+  test('needsRealShell recognises redirection outside quotes only', () => {
+    expect(needsRealShell('echo hello > out.txt')).toBe(true);
+    expect(needsRealShell('echo hello >> out.txt')).toBe(true);
+    expect(needsRealShell('cat < in.txt')).toBe(true);
+    expect(needsRealShell('git push origin main 2>&1')).toBe(true);
+    expect(needsRealShell('cat <<EOF')).toBe(true);
+
+    expect(needsRealShell('echo hello')).toBe(false);
+    expect(needsRealShell('echo "a > b"')).toBe(false);
+    expect(needsRealShell("echo 'a < b'")).toBe(false);
+  });
+});

rust/changelog.d/20260905_095834_redirection_sh_parity.md

@@ -0,0 +1,14 @@
+---
+bump: patch
+---
+
+### Fixed
+
+- Redirections and expansions are no longer swallowed by virtual commands
+  (#46). `ProcessRunner` dispatched to a virtual command before checking
+  whether the command needed a real shell, and arguments came from splitting on
+  whitespace, so `echo hello > out.txt` printed `hello > out.txt` instead of
+  writing the file and `git push origin main 2>&1` reported success while
+  nothing had been pushed. `needs_real_shell` now recognises `>` and `<` in all
+  their forms, and the real-shell check runs before virtual dispatch, matching
+  `/bin/sh`.

rust/src/lib.rs

@@ -268,7 +268,12 @@ impl ProcessRunner {
         // Check if this is a virtual command. Backslash escapes are removed by a
         // real shell but not by the whitespace splitting used for virtual
         // command args, so such commands always go to the system shell (#49).
-        let first_word = if has_shell_escapes(&self.command) {
+        // The same applies to redirection and expansions: whitespace splitting
+        // would hand `>`, `out.txt` to the virtual command as two ordinary
+        // arguments, so `echo hello > out.txt` printed the redirection instead
+        // of writing the file, and `git push ... 2>&1` reported success while
+        // nothing was pushed (#46).
+        let first_word = if has_shell_escapes(&self.command) || needs_real_shell(&self.command) {
             ""
         } else {
             self.command.split_whitespace().next().unwrap_or("")