diff --git a/experiments/issue-46-redirection-parity.mjs b/experiments/issue-46-redirection-parity.mjs new file mode 100644 index 00000000..007267d3 --- /dev/null +++ b/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, ''); + const csOut = readAll(dirCs).replaceAll(dirCs, ''); + + const same = + expected.code === actual.code && + expected.stdout.replaceAll(dirSh, '') === + actual.stdout.replaceAll(dirCs, '') && + 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); diff --git a/js/.changeset/issue-46-redirection-sh-parity.md b/js/.changeset/issue-46-redirection-sh-parity.md new file mode 100644 index 00000000..5dc7641e --- /dev/null +++ b/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`. diff --git a/js/src/$.process-runner-execution.mjs b/js/src/$.process-runner-execution.mjs index fcf31b00..468e9c86 100644 --- a/js/src/$.process-runner-execution.mjs +++ b/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', diff --git a/js/src/shell-parser.mjs b/js/src/shell-parser.mjs index 33cf8e37..0705346f 100644 --- a/js/src/shell-parser.mjs +++ b/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; diff --git a/js/tests/redirection-silent-failure.test.mjs b/js/tests/redirection-silent-failure.test.mjs new file mode 100644 index 00000000..4113a51d --- /dev/null +++ b/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 < b"')).toBe(false); + expect(needsRealShell("echo 'a < b'")).toBe(false); + }); +}); diff --git a/rust/changelog.d/20260905_095834_redirection_sh_parity.md b/rust/changelog.d/20260905_095834_redirection_sh_parity.md new file mode 100644 index 00000000..9240254c --- /dev/null +++ b/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`. diff --git a/rust/src/lib.rs b/rust/src/lib.rs index 849e8269..1081fadb 100644 --- a/rust/src/lib.rs +++ b/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("") diff --git a/rust/src/shell_parser.rs b/rust/src/shell_parser.rs index a7dd4891..00c3d0c6 100644 --- a/rust/src/shell_parser.rs +++ b/rust/src/shell_parser.rs @@ -409,31 +409,29 @@ pub fn parse_shell_command(command: &str) -> Option { parser.parse() } -/// Check if a command needs shell features we don't handle +/// Check if a command needs shell features we don't handle. +/// +/// Redirection counts as such a feature. Virtual commands receive a plain +/// argument list built by splitting on whitespace, 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. Handing the whole command to +/// the system shell is the only way to get exactly the POSIX result +/// (issue #46). pub fn needs_real_shell(command: &str) -> bool { // Check for features we don't handle yet let unsupported = [ - "`", // Command substitution - "$(", // Command substitution - "${", // Variable expansion - "~", // Home expansion (at start of word) - "*", // Glob patterns - "?", // Glob patterns - "[", // Glob patterns - "2>", // stderr redirection - "&>", // Combined redirection - ">&", // File descriptor duplication - "<<", // Here documents - "<<<", // Here strings + '`', // Command substitution + '$', // Command substitution and variable expansion + '~', // Home expansion (at start of word) + '*', // Glob patterns + '?', // Glob patterns + '[', // Glob patterns + '>', // Output redirection, in every form (>, >>, 2>, &>, >&) + '<', // Input redirection, in every form (<, <<, <<<) ]; - for feature in &unsupported { - if command.contains(feature) { - return true; - } - } - - false + command.chars().any(|c| unsupported.contains(&c)) } #[cfg(test)] diff --git a/rust/tests/redirection_silent_failure.rs b/rust/tests/redirection_silent_failure.rs new file mode 100644 index 00000000..102e168b --- /dev/null +++ b/rust/tests/redirection_silent_failure.rs @@ -0,0 +1,195 @@ +//! Issue #46: a command whose first word is also a virtual command used to be +//! dispatched to that virtual command with the shell operators still in the +//! argument list. Virtual command arguments come from splitting on whitespace, +//! so `echo hello > out.txt` printed `hello > out.txt` and wrote no file, and +//! `git push ... 2>&1` reported success while nothing had been pushed. +//! +//! These tests mirror js/tests/redirection-silent-failure.test.mjs: every case +//! is compared against `/bin/sh`, which is the contract. + +#![cfg(unix)] + +use command_stream::{needs_real_shell, ProcessRunner, RunOptions}; +use std::path::Path; +use std::process::Command; +use tempfile::TempDir; + +/// Run `command` in `dir` through /bin/sh and return (exit code, stdout). +fn run_in_sh(command: &str, dir: &Path) -> (i32, String) { + let output = Command::new("/bin/sh") + .arg("-c") + .arg(command) + .current_dir(dir) + .output() + .expect("failed to run /bin/sh"); + ( + output.status.code().unwrap_or(-1), + String::from_utf8_lossy(&output.stdout).into_owned(), + ) +} + +/// List the directory as sorted `name:contents` pairs. +fn snapshot(dir: &Path) -> Vec { + let mut entries: Vec = std::fs::read_dir(dir) + .unwrap() + .map(|entry| { + let entry = entry.unwrap(); + let name = entry.file_name().to_string_lossy().into_owned(); + let contents = std::fs::read_to_string(entry.path()).unwrap_or_default(); + format!("{}:{}", name, contents) + }) + .collect(); + entries.sort(); + entries +} + +/// Create a scratch directory holding the `seed.txt` the cases read from. +fn scratch() -> TempDir { + let dir = TempDir::new().unwrap(); + std::fs::write(dir.path().join("seed.txt"), "seeded\n").unwrap(); + dir +} + +async fn assert_matches_sh(command: &str) { + let sh_dir = scratch(); + let cs_dir = scratch(); + + let (expected_code, expected_stdout) = run_in_sh(command, sh_dir.path()); + + let mut runner = ProcessRunner::new( + command, + RunOptions { + mirror: false, + cwd: Some(cs_dir.path().to_path_buf()), + ..Default::default() + }, + ); + let result = runner.run().await.unwrap(); + + assert_eq!( + result.stdout, expected_stdout, + "stdout mismatch for {:?}", + command + ); + assert_eq!( + result.code, expected_code, + "exit code mismatch for {:?}", + command + ); + assert_eq!( + snapshot(cs_dir.path()), + snapshot(sh_dir.path()), + "written files mismatch for {:?}", + command + ); +} + +/// Every command starts with a word that is also a virtual command, which is +/// exactly the shape that used to bypass the shell. +#[tokio::test] +async fn redirection_on_virtual_commands_matches_sh() { + for command in [ + "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>&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'", + ] { + assert_matches_sh(command).await; + } +} + +/// Expansions reached the virtual commands through the same gap. +#[tokio::test] +async fn expansions_on_virtual_commands_match_sh() { + for command in [ + "echo $HOME", + "echo *", + "echo ~", + "echo `echo hi`", + "echo $(echo hi)", + ] { + assert_matches_sh(command).await; + } +} + +#[tokio::test] +async fn failing_git_push_reports_the_failure_through_redirection() { + // A local path that is not a repository fails the same way everywhere and + // keeps the test off the network. + let dir = TempDir::new().unwrap(); + let repo = dir.path().join("repo"); + std::fs::create_dir(&repo).unwrap(); + + for args in [ + vec!["init", "-q"], + vec!["config", "user.email", "test@example.com"], + vec!["config", "user.name", "Test"], + ] { + Command::new("git") + .args(&args) + .current_dir(&repo) + .output() + .unwrap(); + } + std::fs::write(repo.join("file.txt"), "content\n").unwrap(); + Command::new("git") + .args(["add", "file.txt"]) + .current_dir(&repo) + .output() + .unwrap(); + Command::new("git") + .args(["commit", "-q", "-m", "initial"]) + .current_dir(&repo) + .output() + .unwrap(); + Command::new("git") + .args(["remote", "add", "origin"]) + .arg(dir.path().join("no-such-remote")) + .current_dir(&repo) + .output() + .unwrap(); + + let mut runner = ProcessRunner::new( + "git push origin HEAD 2>&1", + RunOptions { + mirror: false, + cwd: Some(repo.clone()), + ..Default::default() + }, + ); + let result = runner.run().await.unwrap(); + + // Before the fix this was code 0 with an empty stdout. + assert_ne!(result.code, 0, "git push should have failed"); + assert!( + result.stdout.contains("fatal:"), + "expected git's error on stdout, got {:?}", + result.stdout + ); +} + +#[test] +fn needs_real_shell_recognises_redirection() { + assert!(needs_real_shell("echo hello > out.txt")); + assert!(needs_real_shell("echo hello >> out.txt")); + assert!(needs_real_shell("cat < in.txt")); + assert!(needs_real_shell("git push origin main 2>&1")); + assert!(needs_real_shell("cat <