From db34bef451ed53b7bbcdd8029d7441c9a3f6f526 Mon Sep 17 00:00:00 2001 From: konard Date: Tue, 9 Sep 2025 20:23:44 +0300 Subject: [PATCH 1/4] Initial commit with task details for issue #46 Adding CLAUDE.md with task information for AI processing. This file will be removed when the task is complete. Issue: https://github.com/link-foundation/command-stream/issues/46 --- CLAUDE.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 CLAUDE.md diff --git a/CLAUDE.md b/CLAUDE.md new file mode 100644 index 0000000..4ddf566 --- /dev/null +++ b/CLAUDE.md @@ -0,0 +1,5 @@ +Issue to solve: https://github.com/link-foundation/command-stream/issues/46 +Your prepared branch: issue-46-f237aa54 +Your prepared working directory: /tmp/gh-issue-solver-1757438618176 + +Proceed. \ No newline at end of file From 93e2b75c79947a8c9e51ff0d64036187f0182aae Mon Sep 17 00:00:00 2001 From: konard Date: Tue, 9 Sep 2025 20:24:02 +0300 Subject: [PATCH 2/4] Remove CLAUDE.md - PR created successfully --- CLAUDE.md | 5 ----- 1 file changed, 5 deletions(-) delete mode 100644 CLAUDE.md diff --git a/CLAUDE.md b/CLAUDE.md deleted file mode 100644 index 4ddf566..0000000 --- a/CLAUDE.md +++ /dev/null @@ -1,5 +0,0 @@ -Issue to solve: https://github.com/link-foundation/command-stream/issues/46 -Your prepared branch: issue-46-f237aa54 -Your prepared working directory: /tmp/gh-issue-solver-1757438618176 - -Proceed. \ No newline at end of file From 5e8e4e42c34ce3dc998175fef1124c2df211cf90 Mon Sep 17 00:00:00 2001 From: konard Date: Tue, 9 Sep 2025 20:34:45 +0300 Subject: [PATCH 3/4] Fix git push silent failure when using 2>&1 redirection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Issue: Git push commands with 2>&1 redirection were returning exit code 0 with empty output instead of proper error codes and messages, causing silent failures in CI/CD pipelines and repository creation scripts. Root Cause: Commands containing shell features like 2>&1 redirection were incorrectly parsed by the virtual command system. The parser would identify 'cd' as a virtual command and pass shell operators (&&, 2>&1) as arguments, causing the rest of the command (git push) to be ignored. Solution: - Add needsRealShell() check before virtual command execution - When a command requires real shell features, bypass virtual commands - Ensure commands with redirections execute in actual shell environment Changes: - Modified ProcessRunner._doStartAsync() to check needsRealShell() first - Added comprehensive test suite for git push scenarios - Added debug examples for reproducing and analyzing the issue Tests: All existing tests pass, new tests verify the fix works correctly. ๐Ÿค– Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude --- examples/debug-shell-parser.mjs | 33 +++++ examples/test-git-push-output-comparison.mjs | 122 +++++++++++++++++++ examples/test-git-push-silent-failure.mjs | 87 +++++++++++++ examples/test-git-push-with-errexit.mjs | 78 ++++++++++++ examples/test-simple-git-push.mjs | 83 +++++++++++++ examples/test-verbose-git-push.mjs | 78 ++++++++++++ src/$.mjs | 14 ++- tests/git-push-silent-failure.test.mjs | 101 +++++++++++++++ 8 files changed, 593 insertions(+), 3 deletions(-) create mode 100644 examples/debug-shell-parser.mjs create mode 100755 examples/test-git-push-output-comparison.mjs create mode 100755 examples/test-git-push-silent-failure.mjs create mode 100755 examples/test-git-push-with-errexit.mjs create mode 100755 examples/test-simple-git-push.mjs create mode 100755 examples/test-verbose-git-push.mjs create mode 100644 tests/git-push-silent-failure.test.mjs diff --git a/examples/debug-shell-parser.mjs b/examples/debug-shell-parser.mjs new file mode 100644 index 0000000..3c2dd7b --- /dev/null +++ b/examples/debug-shell-parser.mjs @@ -0,0 +1,33 @@ +#!/usr/bin/env node + +// Test to check how shell parser handles 2>&1 +import { parseShellCommand, needsRealShell } from '../src/shell-parser.mjs'; + +function testShellParser() { + console.log('๐Ÿ”ง Testing shell parser with different commands...\n'); + + const testCommands = [ + 'git push origin main', + 'git push origin main 2>&1', + 'git push origin main 2>error.log', + 'git push origin main &>output.log', + 'git push origin main >&2', + 'ls -la', + 'echo hello | grep hello' + ]; + + for (const cmd of testCommands) { + console.log(`Command: ${cmd}`); + console.log(` needsRealShell: ${needsRealShell(cmd)}`); + + try { + const parsed = parseShellCommand(cmd); + console.log(` parsed:`, JSON.stringify(parsed, null, 2)); + } catch (error) { + console.log(` parsing error:`, error.message); + } + console.log(''); + } +} + +testShellParser(); \ No newline at end of file diff --git a/examples/test-git-push-output-comparison.mjs b/examples/test-git-push-output-comparison.mjs new file mode 100755 index 0000000..b4141ed --- /dev/null +++ b/examples/test-git-push-output-comparison.mjs @@ -0,0 +1,122 @@ +#!/usr/bin/env node + +// Test to compare command-stream vs native execSync for git push +import { $ } from '../src/$.mjs'; +import fs from 'fs/promises'; +import path from 'path'; +import { tmpdir } from 'os'; +import { execSync } from 'child_process'; + +async function testGitPushOutputComparison() { + console.log('๐Ÿ”ง Comparing command-stream vs execSync for git operations...\n'); + + // Create a temporary directory for testing + const testDir = path.join(tmpdir(), `git-output-test-${Date.now()}`); + + try { + console.log(`๐Ÿ“ Creating test directory: ${testDir}`); + await $`mkdir -p ${testDir}`; + + // Initialize git repository + console.log('๐Ÿ”„ Initializing git repository...'); + await $`cd ${testDir} && git init`; + + // Configure git user (required for commits) + await $`cd ${testDir} && git config user.email "test@example.com"`; + await $`cd ${testDir} && git config user.name "Test User"`; + + // Create a test file and commit + console.log('๐Ÿ“ Creating test file and committing...'); + await fs.writeFile(path.join(testDir, 'test.txt'), 'Test content for git push issue'); + await $`cd ${testDir} && git add test.txt`; + await $`cd ${testDir} && git commit -m "Test commit"`; + + // Add a remote (this one doesn't exist, so push will fail) + await $`cd ${testDir} && git remote add origin https://github.com/nonexistent/test-repo.git`; + + console.log('\n๐Ÿ” Test 1: Using command-stream $ for git push...'); + try { + const commandStreamResult = await $`cd ${testDir} && git push -u origin main 2>&1`; + console.log('Command-stream results:'); + console.log(' Exit code:', commandStreamResult.code); + console.log(' Stdout length:', commandStreamResult.stdout?.length || 0); + console.log(' Stderr length:', commandStreamResult.stderr?.length || 0); + console.log(' Stdout:', JSON.stringify(commandStreamResult.stdout || '')); + console.log(' Stderr:', JSON.stringify(commandStreamResult.stderr || '')); + } catch (error) { + console.log('โŒ Command-stream threw error:', error.message); + console.log(' Error code:', error.code); + } + + console.log('\n๐Ÿ” Test 2: Using execSync for git push...'); + try { + const execSyncOutput = execSync('git push -u origin main 2>&1', { + encoding: 'utf8', + cwd: testDir + }); + console.log('ExecSync results:'); + console.log(' Output length:', execSyncOutput.length); + console.log(' Output:', JSON.stringify(execSyncOutput)); + } catch (error) { + console.log('โŒ ExecSync threw error (expected):', error.message.split('\n')[0]); + console.log(' Exit code:', error.status); + console.log(' Error output:', JSON.stringify(error.output?.toString() || error.stdout?.toString() || '')); + } + + // Test with dry-run which shouldn't fail + console.log('\n๐Ÿ” Test 3: Using command-stream $ for git push --dry-run...'); + try { + const dryRunResult = await $`cd ${testDir} && git push --dry-run origin main 2>&1`; + console.log('Command-stream dry-run results:'); + console.log(' Exit code:', dryRunResult.code); + console.log(' Stdout length:', dryRunResult.stdout?.length || 0); + console.log(' Stderr length:', dryRunResult.stderr?.length || 0); + console.log(' Stdout:', JSON.stringify(dryRunResult.stdout || '')); + console.log(' Stderr:', JSON.stringify(dryRunResult.stderr || '')); + } catch (error) { + console.log('โŒ Command-stream dry-run threw error:', error.message); + } + + console.log('\n๐Ÿ” Test 4: Using execSync for git push --dry-run...'); + try { + const execSyncDryRun = execSync('git push --dry-run origin main 2>&1', { + encoding: 'utf8', + cwd: testDir + }); + console.log('ExecSync dry-run results:'); + console.log(' Output length:', execSyncDryRun.length); + console.log(' Output:', JSON.stringify(execSyncDryRun)); + } catch (error) { + console.log('โŒ ExecSync dry-run threw error:', error.message.split('\n')[0]); + console.log(' Output:', JSON.stringify(error.output?.toString() || error.stdout?.toString() || '')); + } + + // Test a successful git command for comparison + console.log('\n๐Ÿ” Test 5: Git status comparison...'); + const statusCommand = await $`cd ${testDir} && git status`; + console.log('Command-stream git status:'); + console.log(' Exit code:', statusCommand.code); + console.log(' Stdout length:', statusCommand.stdout?.length || 0); + console.log(' Stdout preview:', (statusCommand.stdout || '').slice(0, 100)); + + const statusExecSync = execSync('git status', { encoding: 'utf8', cwd: testDir }); + console.log('ExecSync git status:'); + console.log(' Output length:', statusExecSync.length); + console.log(' Output preview:', statusExecSync.slice(0, 100)); + + } catch (error) { + console.log('โŒ Test failed with error:', error.message); + } finally { + // Cleanup + console.log('\n๐Ÿงน Cleaning up test directory...'); + try { + await $`rm -rf ${testDir}`; + console.log('โœ… Cleanup completed'); + } catch (cleanupError) { + console.log('โš ๏ธ Cleanup failed:', cleanupError.message); + } + } +} + +// Run the test +testGitPushOutputComparison().catch(console.error); \ No newline at end of file diff --git a/examples/test-git-push-silent-failure.mjs b/examples/test-git-push-silent-failure.mjs new file mode 100755 index 0000000..f303981 --- /dev/null +++ b/examples/test-git-push-silent-failure.mjs @@ -0,0 +1,87 @@ +#!/usr/bin/env node + +// Test script to reproduce git push silent failure issue +import { $ } from '../src/$.mjs'; +import fs from 'fs'; +import path from 'path'; +import { tmpdir } from 'os'; + +async function testGitPushSilentFailure() { + console.log('๐Ÿ”ง Testing git push silent failure issue...\n'); + + // Create a temporary directory for testing + const testDir = path.join(tmpdir(), `git-push-test-${Date.now()}`); + + try { + console.log(`๐Ÿ“ Creating test directory: ${testDir}`); + await $`mkdir -p ${testDir}`; + + // Initialize git repository + console.log('๐Ÿ”„ Initializing git repository...'); + await $`cd ${testDir} && git init`; + + // Configure git user (required for commits) + await $`cd ${testDir} && git config user.email "test@example.com"`; + await $`cd ${testDir} && git config user.name "Test User"`; + + // Create a test file and commit + console.log('๐Ÿ“ Creating test file and committing...'); + await $`cd ${testDir} && echo "test content" > test.txt`; + await $`cd ${testDir} && git add test.txt`; + await $`cd ${testDir} && git commit -m "Initial commit"`; + + // Test 1: Try to push to a non-existent remote (this should fail) + console.log('\n๐Ÿ” Test 1: Pushing to non-existent remote...'); + try { + const result = await $`cd ${testDir} && git remote add origin https://github.com/nonexistent/repo.git`; + console.log('โœ… Remote added successfully'); + + const pushResult = await $`cd ${testDir} && git push -u origin main`; + console.log('๐Ÿšจ POTENTIAL ISSUE: Push appeared successful when it should have failed!'); + console.log('Exit code:', pushResult.code); + console.log('Stdout:', pushResult.stdout); + console.log('Stderr:', pushResult.stderr); + + } catch (error) { + console.log('โœ… Push correctly failed with error:', error.message); + console.log('Error code:', error.code); + } + + // Test 2: Try to push to an invalid URL + console.log('\n๐Ÿ” Test 2: Pushing to invalid URL...'); + try { + await $`cd ${testDir} && git remote set-url origin https://invalid-url-that-does-not-exist.com/repo.git`; + const pushResult = await $`cd ${testDir} && git push origin main`; + console.log('๐Ÿšจ POTENTIAL ISSUE: Push to invalid URL appeared successful!'); + console.log('Exit code:', pushResult.code); + console.log('Stdout:', pushResult.stdout); + console.log('Stderr:', pushResult.stderr); + + } catch (error) { + console.log('โœ… Push to invalid URL correctly failed:', error.message); + console.log('Error code:', error.code); + } + + // Test 3: Check git status after failed push + console.log('\n๐Ÿ” Test 3: Checking git status after push attempt...'); + const statusResult = await $`cd ${testDir} && git status`; + console.log('Git status output:'); + console.log(statusResult.stdout); + + } catch (error) { + console.log('โŒ Test failed with error:', error.message); + console.log('Error details:', error); + } finally { + // Cleanup + console.log('\n๐Ÿงน Cleaning up test directory...'); + try { + await $`rm -rf ${testDir}`; + console.log('โœ… Cleanup completed'); + } catch (cleanupError) { + console.log('โš ๏ธ Cleanup failed:', cleanupError.message); + } + } +} + +// Run the test +testGitPushSilentFailure().catch(console.error); \ No newline at end of file diff --git a/examples/test-git-push-with-errexit.mjs b/examples/test-git-push-with-errexit.mjs new file mode 100755 index 0000000..adf259a --- /dev/null +++ b/examples/test-git-push-with-errexit.mjs @@ -0,0 +1,78 @@ +#!/usr/bin/env node + +// Test script to reproduce git push silent failure with errexit enabled +import { $, shell } from '../src/$.mjs'; +import fs from 'fs'; +import path from 'path'; +import { tmpdir } from 'os'; + +async function testGitPushWithReachExit() { + console.log('๐Ÿ”ง Testing git push with errexit enabled...\n'); + + // Create a temporary directory for testing + const testDir = path.join(tmpdir(), `git-push-errexit-test-${Date.now()}`); + + try { + console.log(`๐Ÿ“ Creating test directory: ${testDir}`); + await $`mkdir -p ${testDir}`; + + // Initialize git repository + console.log('๐Ÿ”„ Initializing git repository...'); + await $`cd ${testDir} && git init`; + + // Configure git user (required for commits) + await $`cd ${testDir} && git config user.email "test@example.com"`; + await $`cd ${testDir} && git config user.name "Test User"`; + + // Create a test file and commit to main branch + console.log('๐Ÿ“ Creating test file and committing to main...'); + await $`cd ${testDir} && git checkout -b main`; + await $`cd ${testDir} && echo "test content" > test.txt`; + await $`cd ${testDir} && git add test.txt`; + await $`cd ${testDir} && git commit -m "Initial commit"`; + + console.log('\n๐Ÿ”ง Test without errexit (default behavior)...'); + shell.errexit(false); + try { + const result = await $`cd ${testDir} && git remote add origin https://github.com/nonexistent/repo.git`; + const pushResult = await $`cd ${testDir} && git push -u origin main`; + console.log('โœ… Command executed without throwing'); + console.log('Exit code:', pushResult.code); + console.log('Stderr length:', pushResult.stderr?.length || 0); + console.log('Has error in stderr:', pushResult.stderr?.includes('error:') || false); + } catch (error) { + console.log('โŒ Command threw error (unexpected):', error.message); + } + + console.log('\n๐Ÿ”ง Test with errexit enabled...'); + shell.errexit(true); + try { + const pushResult = await $`cd ${testDir} && git push origin main`; + console.log('๐Ÿšจ Command completed without throwing (this is the bug!)'); + console.log('Exit code:', pushResult.code); + console.log('Stderr:', pushResult.stderr); + } catch (error) { + console.log('โœ… Command correctly threw error:', error.message); + console.log('Error code:', error.code); + } + + } catch (error) { + console.log('โŒ Test failed with error:', error.message); + console.log('Error details:', error); + } finally { + // Reset errexit + shell.errexit(false); + + // Cleanup + console.log('\n๐Ÿงน Cleaning up test directory...'); + try { + await $`rm -rf ${testDir}`; + console.log('โœ… Cleanup completed'); + } catch (cleanupError) { + console.log('โš ๏ธ Cleanup failed:', cleanupError.message); + } + } +} + +// Run the test +testGitPushWithReachExit().catch(console.error); \ No newline at end of file diff --git a/examples/test-simple-git-push.mjs b/examples/test-simple-git-push.mjs new file mode 100755 index 0000000..c9fa160 --- /dev/null +++ b/examples/test-simple-git-push.mjs @@ -0,0 +1,83 @@ +#!/usr/bin/env node + +// Test to isolate the git push issue without redirection +import { $ } from '../src/$.mjs'; +import fs from 'fs/promises'; +import path from 'path'; +import { tmpdir } from 'os'; + +async function testSimpleGitPush() { + console.log('๐Ÿ”ง Testing simple git push without redirection...\n'); + + // Create a temporary directory for testing + const testDir = path.join(tmpdir(), `simple-git-test-${Date.now()}`); + + try { + console.log(`๐Ÿ“ Creating test directory: ${testDir}`); + await $`mkdir -p ${testDir}`; + + // Initialize git repository + console.log('๐Ÿ”„ Initializing git repository...'); + await $`cd ${testDir} && git init`; + + // Configure git user (required for commits) + await $`cd ${testDir} && git config user.email "test@example.com"`; + await $`cd ${testDir} && git config user.name "Test User"`; + + // Create a test file and commit to the correct branch + console.log('๐Ÿ“ Creating test file and committing...'); + await fs.writeFile(path.join(testDir, 'test.txt'), 'Test content'); + await $`cd ${testDir} && git add test.txt`; + await $`cd ${testDir} && git commit -m "Test commit"`; + + // Check which branch we're on + const branchResult = await $`cd ${testDir} && git branch --show-current`; + console.log('Current branch:', branchResult.stdout.trim()); + + // Add a remote that doesn't exist + await $`cd ${testDir} && git remote add origin https://github.com/nonexistent/test-repo.git`; + + console.log('\n๐Ÿ” Test 1: git push without redirection...'); + const pushResult = await $`cd ${testDir} && git push -u origin ${branchResult.stdout.trim()}`; + console.log('Results:'); + console.log(' Exit code:', pushResult.code); + console.log(' Stdout length:', pushResult.stdout?.length || 0); + console.log(' Stderr length:', pushResult.stderr?.length || 0); + console.log(' Stdout:', JSON.stringify(pushResult.stdout || '')); + console.log(' Stderr:', JSON.stringify(pushResult.stderr || '')); + + console.log('\n๐Ÿ” Test 2: git push with explicit 2>&1 redirection...'); + const pushRedirectResult = await $`cd ${testDir} && git push -u origin ${branchResult.stdout.trim()} 2>&1`; + console.log('Results:'); + console.log(' Exit code:', pushRedirectResult.code); + console.log(' Stdout length:', pushRedirectResult.stdout?.length || 0); + console.log(' Stderr length:', pushRedirectResult.stderr?.length || 0); + console.log(' Stdout:', JSON.stringify(pushRedirectResult.stdout || '')); + console.log(' Stderr:', JSON.stringify(pushRedirectResult.stderr || '')); + + console.log('\n๐Ÿ” Test 3: git push to a different fake remote...'); + await $`cd ${testDir} && git remote set-url origin https://fake-host-that-does-not-exist.com/repo.git`; + const pushFakeResult = await $`cd ${testDir} && git push origin ${branchResult.stdout.trim()}`; + console.log('Results:'); + console.log(' Exit code:', pushFakeResult.code); + console.log(' Stdout length:', pushFakeResult.stdout?.length || 0); + console.log(' Stderr length:', pushFakeResult.stderr?.length || 0); + console.log(' Stdout:', JSON.stringify(pushFakeResult.stdout || '')); + console.log(' Stderr:', JSON.stringify(pushFakeResult.stderr || '')); + + } catch (error) { + console.log('โŒ Test failed with error:', error.message); + } finally { + // Cleanup + console.log('\n๐Ÿงน Cleaning up test directory...'); + try { + await $`rm -rf ${testDir}`; + console.log('โœ… Cleanup completed'); + } catch (cleanupError) { + console.log('โš ๏ธ Cleanup failed:', cleanupError.message); + } + } +} + +// Run the test +testSimpleGitPush().catch(console.error); \ No newline at end of file diff --git a/examples/test-verbose-git-push.mjs b/examples/test-verbose-git-push.mjs new file mode 100755 index 0000000..5f4e672 --- /dev/null +++ b/examples/test-verbose-git-push.mjs @@ -0,0 +1,78 @@ +#!/usr/bin/env node + +// Test git push with verbose logging to trace execution +import { $ } from '../src/$.mjs'; +import fs from 'fs/promises'; +import path from 'path'; +import { tmpdir } from 'os'; + +// Enable verbose logging +process.env.COMMAND_STREAM_VERBOSE = 'true'; + +async function testVerboseGitPush() { + console.log('๐Ÿ”ง Testing git push with verbose logging enabled...\n'); + + // Create a temporary directory for testing + const testDir = path.join(tmpdir(), `verbose-git-test-${Date.now()}`); + + try { + console.log(`๐Ÿ“ Creating test directory: ${testDir}`); + await $`mkdir -p ${testDir}`; + + // Initialize git repository + console.log('๐Ÿ”„ Initializing git repository...'); + await $`cd ${testDir} && git init`; + + // Configure git user (required for commits) + await $`cd ${testDir} && git config user.email "test@example.com"`; + await $`cd ${testDir} && git config user.name "Test User"`; + + // Create a test file and commit + console.log('๐Ÿ“ Creating test file and committing...'); + await fs.writeFile(path.join(testDir, 'test.txt'), 'Test content'); + await $`cd ${testDir} && git add test.txt`; + await $`cd ${testDir} && git commit -m "Test commit"`; + + // Check which branch we're on + const branchResult = await $`cd ${testDir} && git branch --show-current`; + const branch = branchResult.stdout.trim(); + console.log('Current branch:', branch); + + // Add a remote that doesn't exist + await $`cd ${testDir} && git remote add origin https://github.com/nonexistent/test-repo.git`; + + console.log('\n๐Ÿ” Test 1: git push without redirection (verbose logging enabled)...'); + console.log('='.repeat(80)); + const pushResult1 = await $`cd ${testDir} && git push -u origin ${branch}`; + console.log('='.repeat(80)); + console.log('Results:'); + console.log(' Exit code:', pushResult1.code); + console.log(' Stdout:', JSON.stringify(pushResult1.stdout || '')); + console.log(' Stderr:', JSON.stringify(pushResult1.stderr || '')); + + console.log('\n๐Ÿ” Test 2: git push WITH 2>&1 redirection (verbose logging enabled)...'); + console.log('='.repeat(80)); + const pushResult2 = await $`cd ${testDir} && git push -u origin ${branch} 2>&1`; + console.log('='.repeat(80)); + console.log('Results:'); + console.log(' Exit code:', pushResult2.code); + console.log(' Stdout:', JSON.stringify(pushResult2.stdout || '')); + console.log(' Stderr:', JSON.stringify(pushResult2.stderr || '')); + + } catch (error) { + console.log('โŒ Test failed with error:', error.message); + console.log('Error stack:', error.stack); + } finally { + // Cleanup + console.log('\n๐Ÿงน Cleaning up test directory...'); + try { + await $`rm -rf ${testDir}`; + console.log('โœ… Cleanup completed'); + } catch (cleanupError) { + console.log('โš ๏ธ Cleanup failed:', cleanupError.message); + } + } +} + +// Run the test +testVerboseGitPush().catch(console.error); \ No newline at end of file diff --git a/src/$.mjs b/src/$.mjs index 46c7258..e33455e 100755 --- a/src/$.mjs +++ b/src/$.mjs @@ -1667,8 +1667,16 @@ class ProcessRunner extends StreamEmitter { command: this.spec.command.slice(0, 100) }, null, 2)}`); - // Only use enhanced parser when appropriate - if (!this.options._bypassVirtual && shouldUseShellOperators && !needsRealShell(this.spec.command)) { + // Check if command needs real shell first (for features like 2>&1 redirection) + if (!this.options._bypassVirtual && needsRealShell(this.spec.command)) { + trace('ProcessRunner', () => `Command needs real shell, executing in shell | ${JSON.stringify({ + command: this.spec.command.slice(0, 50), + reason: 'needsRealShell' + }, null, 2)}`); + // Execute in real shell instead of using virtual commands + // Fall through to normal shell execution below + } else if (!this.options._bypassVirtual && shouldUseShellOperators && !needsRealShell(this.spec.command)) { + // Only use enhanced parser when appropriate and doesn't need real shell const enhancedParsed = parseShellCommand(this.spec.command); if (enhancedParsed && enhancedParsed.type !== 'simple') { trace('ProcessRunner', () => `Using enhanced parser for shell operators | ${JSON.stringify({ @@ -1700,7 +1708,7 @@ class ProcessRunner extends StreamEmitter { commandCount: parsed.commands?.length }, null, 2)}`); return await this._runPipeline(parsed.commands); - } else if (parsed.type === 'simple' && virtualCommandsEnabled && virtualCommands.has(parsed.cmd) && !this.options._bypassVirtual) { + } else if (parsed.type === 'simple' && virtualCommandsEnabled && virtualCommands.has(parsed.cmd) && !this.options._bypassVirtual && !needsRealShell(this.spec.command)) { // For built-in virtual commands that have real counterparts (like sleep), // skip the virtual version when custom stdin is provided to ensure proper process handling const hasCustomStdin = this.options.stdin && diff --git a/tests/git-push-silent-failure.test.mjs b/tests/git-push-silent-failure.test.mjs new file mode 100644 index 0000000..6785b4b --- /dev/null +++ b/tests/git-push-silent-failure.test.mjs @@ -0,0 +1,101 @@ +import { test, expect, describe, beforeEach, afterEach } from 'bun:test'; +import './test-helper.mjs'; // Automatically sets up beforeEach/afterEach cleanup +import { $, shell } from '../src/$.mjs'; +import fs from 'fs/promises'; +import path from 'path'; +import { tmpdir } from 'os'; + +describe('Git push silent failure fix (Issue #46)', () => { + let testDir; + + beforeEach(async () => { + // Create temp directory for each test + testDir = path.join(tmpdir(), `git-test-${Date.now()}-${Math.random().toString(36).slice(2)}`); + await $`mkdir -p ${testDir}`; + + // Initialize git repo + await $`cd ${testDir} && git init`; + await $`cd ${testDir} && git config user.email "test@example.com"`; + await $`cd ${testDir} && git config user.name "Test User"`; + + // Create and commit a test file + await fs.writeFile(path.join(testDir, 'test.txt'), 'Test content'); + await $`cd ${testDir} && git add test.txt`; + await $`cd ${testDir} && git commit -m "Test commit"`; + + // Add non-existent remote + await $`cd ${testDir} && git remote add origin https://github.com/nonexistent/test-repo.git`; + + // Get current branch name + const branchResult = await $`cd ${testDir} && git branch --show-current`; + global.testBranch = branchResult.stdout.trim(); + }); + + afterEach(async () => { + if (testDir) { + try { + await $`rm -rf ${testDir}`; + } catch (e) { + // Ignore cleanup errors + } + } + }); + + test('git push without redirection should return proper exit code and stderr', async () => { + const result = await $`cd ${testDir} && git push origin ${global.testBranch}`; + + expect(result.code).not.toBe(0); // Should fail + expect(result.stderr).toContain('fatal:'); // Should have error in stderr + expect(result.stdout).toBe(''); // stdout should be empty + }); + + test('git push with 2>&1 redirection should return proper exit code and stdout', async () => { + const result = await $`cd ${testDir} && git push origin ${global.testBranch} 2>&1`; + + expect(result.code).not.toBe(0); // Should fail (not return 0 like before the fix) + expect(result.stdout).toContain('fatal:'); // Error should be in stdout due to redirection + expect(result.stderr).toBe(''); // stderr should be empty due to redirection + }); + + test('git push with errexit enabled should throw on failure', async () => { + shell.errexit(true); + + try { + await expect(async () => { + await $`cd ${testDir} && git push origin ${global.testBranch}`; + }).toThrow(); + } finally { + shell.errexit(false); + } + }); + + test('git push with 2>&1 and errexit should throw on failure', async () => { + shell.errexit(true); + + try { + await expect(async () => { + await $`cd ${testDir} && git push origin ${global.testBranch} 2>&1`; + }).toThrow(); + } finally { + shell.errexit(false); + } + }); + + test('complex command with 2>&1 should not trigger virtual cd command bug', async () => { + // This was the specific case that caused the bug: + // The command was incorrectly parsed as a virtual `cd` command with all the rest as args + const result = await $`cd ${testDir} && git push origin ${global.testBranch} 2>&1`; + + expect(result.code).not.toBe(0); // Should fail, not return 0 from virtual cd + expect(result.stdout).toContain('fatal:'); // Should contain actual git error + expect(result.stdout).not.toContain('cd:'); // Should not contain cd command errors + }); + + test('other shell features that need real shell still work', async () => { + // Test that other needsRealShell features still work + const result = await $`cd ${testDir} && echo "test" > output.txt && cat output.txt`; + + expect(result.code).toBe(0); + expect(result.stdout.trim()).toBe('test'); + }); +}); \ No newline at end of file From d4fbd500134939d21a4d5c7a89b9aaaecd8cddd6 Mon Sep 17 00:00:00 2001 From: konard Date: Sat, 5 Sep 2026 09:59:22 +0000 Subject: [PATCH 4/4] fix: send redirections and expansions to the real shell (#46) needsRealShell() was only consulted when the command also contained one of `&&`, `||`, `;`, `&` or `(`, and redirection characters are not part of that operator set. Any command whose first word is a built-in was therefore dispatched in-process with the operators left in the argument list, which is split on whitespace: `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 tell was that `echo $(echo hi)` worked while `echo $HOME` did not, purely because `(` happens to be in hasShellOperators(). - js: ask needsRealShell() unconditionally in handleShellMode(). - js: treat unquoted `<` and `>` as unsupported features, which covers `>`, `>>`, `2>`, `&>`, `>&`, `<`, `<<` and `<<<` in one rule. - rust: run the real-shell check before virtual command dispatch, and match the same character set in needs_real_shell(). Both new test files compare every case against /bin/sh, which is the contract, and cover the quoted forms (`echo "a > b"`) so the fix does not over-reach. Removes the stale examples and test written against the pre-monorepo layout; the old git-push test also required network access. --- experiments/issue-46-redirection-parity.mjs | 108 ++++++++++ .../issue-46-redirection-sh-parity.md | 17 ++ js/examples/debug-shell-parser.mjs | 33 --- .../test-git-push-output-comparison.mjs | 122 ----------- js/examples/test-git-push-silent-failure.mjs | 87 -------- js/examples/test-git-push-with-errexit.mjs | 78 ------- js/examples/test-simple-git-push.mjs | 83 -------- js/examples/test-verbose-git-push.mjs | 78 ------- js/src/$.process-runner-execution.mjs | 9 +- js/src/shell-parser.mjs | 23 ++- js/tests/git-push-silent-failure.test.mjs | 101 --------- js/tests/redirection-silent-failure.test.mjs | 174 ++++++++++++++++ .../20260905_095834_redirection_sh_parity.md | 14 ++ rust/src/lib.rs | 7 +- rust/src/shell_parser.rs | 38 ++-- rust/tests/redirection_silent_failure.rs | 195 ++++++++++++++++++ 16 files changed, 552 insertions(+), 615 deletions(-) create mode 100644 experiments/issue-46-redirection-parity.mjs create mode 100644 js/.changeset/issue-46-redirection-sh-parity.md delete mode 100644 js/examples/debug-shell-parser.mjs delete mode 100755 js/examples/test-git-push-output-comparison.mjs delete mode 100755 js/examples/test-git-push-silent-failure.mjs delete mode 100755 js/examples/test-git-push-with-errexit.mjs delete mode 100755 js/examples/test-simple-git-push.mjs delete mode 100755 js/examples/test-verbose-git-push.mjs delete mode 100644 js/tests/git-push-silent-failure.test.mjs create mode 100644 js/tests/redirection-silent-failure.test.mjs create mode 100644 rust/changelog.d/20260905_095834_redirection_sh_parity.md create mode 100644 rust/tests/redirection_silent_failure.rs diff --git a/experiments/issue-46-redirection-parity.mjs b/experiments/issue-46-redirection-parity.mjs new file mode 100644 index 0000000..007267d --- /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 0000000..5dc7641 --- /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/examples/debug-shell-parser.mjs b/js/examples/debug-shell-parser.mjs deleted file mode 100644 index 3c2dd7b..0000000 --- a/js/examples/debug-shell-parser.mjs +++ /dev/null @@ -1,33 +0,0 @@ -#!/usr/bin/env node - -// Test to check how shell parser handles 2>&1 -import { parseShellCommand, needsRealShell } from '../src/shell-parser.mjs'; - -function testShellParser() { - console.log('๐Ÿ”ง Testing shell parser with different commands...\n'); - - const testCommands = [ - 'git push origin main', - 'git push origin main 2>&1', - 'git push origin main 2>error.log', - 'git push origin main &>output.log', - 'git push origin main >&2', - 'ls -la', - 'echo hello | grep hello' - ]; - - for (const cmd of testCommands) { - console.log(`Command: ${cmd}`); - console.log(` needsRealShell: ${needsRealShell(cmd)}`); - - try { - const parsed = parseShellCommand(cmd); - console.log(` parsed:`, JSON.stringify(parsed, null, 2)); - } catch (error) { - console.log(` parsing error:`, error.message); - } - console.log(''); - } -} - -testShellParser(); \ No newline at end of file diff --git a/js/examples/test-git-push-output-comparison.mjs b/js/examples/test-git-push-output-comparison.mjs deleted file mode 100755 index b4141ed..0000000 --- a/js/examples/test-git-push-output-comparison.mjs +++ /dev/null @@ -1,122 +0,0 @@ -#!/usr/bin/env node - -// Test to compare command-stream vs native execSync for git push -import { $ } from '../src/$.mjs'; -import fs from 'fs/promises'; -import path from 'path'; -import { tmpdir } from 'os'; -import { execSync } from 'child_process'; - -async function testGitPushOutputComparison() { - console.log('๐Ÿ”ง Comparing command-stream vs execSync for git operations...\n'); - - // Create a temporary directory for testing - const testDir = path.join(tmpdir(), `git-output-test-${Date.now()}`); - - try { - console.log(`๐Ÿ“ Creating test directory: ${testDir}`); - await $`mkdir -p ${testDir}`; - - // Initialize git repository - console.log('๐Ÿ”„ Initializing git repository...'); - await $`cd ${testDir} && git init`; - - // Configure git user (required for commits) - await $`cd ${testDir} && git config user.email "test@example.com"`; - await $`cd ${testDir} && git config user.name "Test User"`; - - // Create a test file and commit - console.log('๐Ÿ“ Creating test file and committing...'); - await fs.writeFile(path.join(testDir, 'test.txt'), 'Test content for git push issue'); - await $`cd ${testDir} && git add test.txt`; - await $`cd ${testDir} && git commit -m "Test commit"`; - - // Add a remote (this one doesn't exist, so push will fail) - await $`cd ${testDir} && git remote add origin https://github.com/nonexistent/test-repo.git`; - - console.log('\n๐Ÿ” Test 1: Using command-stream $ for git push...'); - try { - const commandStreamResult = await $`cd ${testDir} && git push -u origin main 2>&1`; - console.log('Command-stream results:'); - console.log(' Exit code:', commandStreamResult.code); - console.log(' Stdout length:', commandStreamResult.stdout?.length || 0); - console.log(' Stderr length:', commandStreamResult.stderr?.length || 0); - console.log(' Stdout:', JSON.stringify(commandStreamResult.stdout || '')); - console.log(' Stderr:', JSON.stringify(commandStreamResult.stderr || '')); - } catch (error) { - console.log('โŒ Command-stream threw error:', error.message); - console.log(' Error code:', error.code); - } - - console.log('\n๐Ÿ” Test 2: Using execSync for git push...'); - try { - const execSyncOutput = execSync('git push -u origin main 2>&1', { - encoding: 'utf8', - cwd: testDir - }); - console.log('ExecSync results:'); - console.log(' Output length:', execSyncOutput.length); - console.log(' Output:', JSON.stringify(execSyncOutput)); - } catch (error) { - console.log('โŒ ExecSync threw error (expected):', error.message.split('\n')[0]); - console.log(' Exit code:', error.status); - console.log(' Error output:', JSON.stringify(error.output?.toString() || error.stdout?.toString() || '')); - } - - // Test with dry-run which shouldn't fail - console.log('\n๐Ÿ” Test 3: Using command-stream $ for git push --dry-run...'); - try { - const dryRunResult = await $`cd ${testDir} && git push --dry-run origin main 2>&1`; - console.log('Command-stream dry-run results:'); - console.log(' Exit code:', dryRunResult.code); - console.log(' Stdout length:', dryRunResult.stdout?.length || 0); - console.log(' Stderr length:', dryRunResult.stderr?.length || 0); - console.log(' Stdout:', JSON.stringify(dryRunResult.stdout || '')); - console.log(' Stderr:', JSON.stringify(dryRunResult.stderr || '')); - } catch (error) { - console.log('โŒ Command-stream dry-run threw error:', error.message); - } - - console.log('\n๐Ÿ” Test 4: Using execSync for git push --dry-run...'); - try { - const execSyncDryRun = execSync('git push --dry-run origin main 2>&1', { - encoding: 'utf8', - cwd: testDir - }); - console.log('ExecSync dry-run results:'); - console.log(' Output length:', execSyncDryRun.length); - console.log(' Output:', JSON.stringify(execSyncDryRun)); - } catch (error) { - console.log('โŒ ExecSync dry-run threw error:', error.message.split('\n')[0]); - console.log(' Output:', JSON.stringify(error.output?.toString() || error.stdout?.toString() || '')); - } - - // Test a successful git command for comparison - console.log('\n๐Ÿ” Test 5: Git status comparison...'); - const statusCommand = await $`cd ${testDir} && git status`; - console.log('Command-stream git status:'); - console.log(' Exit code:', statusCommand.code); - console.log(' Stdout length:', statusCommand.stdout?.length || 0); - console.log(' Stdout preview:', (statusCommand.stdout || '').slice(0, 100)); - - const statusExecSync = execSync('git status', { encoding: 'utf8', cwd: testDir }); - console.log('ExecSync git status:'); - console.log(' Output length:', statusExecSync.length); - console.log(' Output preview:', statusExecSync.slice(0, 100)); - - } catch (error) { - console.log('โŒ Test failed with error:', error.message); - } finally { - // Cleanup - console.log('\n๐Ÿงน Cleaning up test directory...'); - try { - await $`rm -rf ${testDir}`; - console.log('โœ… Cleanup completed'); - } catch (cleanupError) { - console.log('โš ๏ธ Cleanup failed:', cleanupError.message); - } - } -} - -// Run the test -testGitPushOutputComparison().catch(console.error); \ No newline at end of file diff --git a/js/examples/test-git-push-silent-failure.mjs b/js/examples/test-git-push-silent-failure.mjs deleted file mode 100755 index f303981..0000000 --- a/js/examples/test-git-push-silent-failure.mjs +++ /dev/null @@ -1,87 +0,0 @@ -#!/usr/bin/env node - -// Test script to reproduce git push silent failure issue -import { $ } from '../src/$.mjs'; -import fs from 'fs'; -import path from 'path'; -import { tmpdir } from 'os'; - -async function testGitPushSilentFailure() { - console.log('๐Ÿ”ง Testing git push silent failure issue...\n'); - - // Create a temporary directory for testing - const testDir = path.join(tmpdir(), `git-push-test-${Date.now()}`); - - try { - console.log(`๐Ÿ“ Creating test directory: ${testDir}`); - await $`mkdir -p ${testDir}`; - - // Initialize git repository - console.log('๐Ÿ”„ Initializing git repository...'); - await $`cd ${testDir} && git init`; - - // Configure git user (required for commits) - await $`cd ${testDir} && git config user.email "test@example.com"`; - await $`cd ${testDir} && git config user.name "Test User"`; - - // Create a test file and commit - console.log('๐Ÿ“ Creating test file and committing...'); - await $`cd ${testDir} && echo "test content" > test.txt`; - await $`cd ${testDir} && git add test.txt`; - await $`cd ${testDir} && git commit -m "Initial commit"`; - - // Test 1: Try to push to a non-existent remote (this should fail) - console.log('\n๐Ÿ” Test 1: Pushing to non-existent remote...'); - try { - const result = await $`cd ${testDir} && git remote add origin https://github.com/nonexistent/repo.git`; - console.log('โœ… Remote added successfully'); - - const pushResult = await $`cd ${testDir} && git push -u origin main`; - console.log('๐Ÿšจ POTENTIAL ISSUE: Push appeared successful when it should have failed!'); - console.log('Exit code:', pushResult.code); - console.log('Stdout:', pushResult.stdout); - console.log('Stderr:', pushResult.stderr); - - } catch (error) { - console.log('โœ… Push correctly failed with error:', error.message); - console.log('Error code:', error.code); - } - - // Test 2: Try to push to an invalid URL - console.log('\n๐Ÿ” Test 2: Pushing to invalid URL...'); - try { - await $`cd ${testDir} && git remote set-url origin https://invalid-url-that-does-not-exist.com/repo.git`; - const pushResult = await $`cd ${testDir} && git push origin main`; - console.log('๐Ÿšจ POTENTIAL ISSUE: Push to invalid URL appeared successful!'); - console.log('Exit code:', pushResult.code); - console.log('Stdout:', pushResult.stdout); - console.log('Stderr:', pushResult.stderr); - - } catch (error) { - console.log('โœ… Push to invalid URL correctly failed:', error.message); - console.log('Error code:', error.code); - } - - // Test 3: Check git status after failed push - console.log('\n๐Ÿ” Test 3: Checking git status after push attempt...'); - const statusResult = await $`cd ${testDir} && git status`; - console.log('Git status output:'); - console.log(statusResult.stdout); - - } catch (error) { - console.log('โŒ Test failed with error:', error.message); - console.log('Error details:', error); - } finally { - // Cleanup - console.log('\n๐Ÿงน Cleaning up test directory...'); - try { - await $`rm -rf ${testDir}`; - console.log('โœ… Cleanup completed'); - } catch (cleanupError) { - console.log('โš ๏ธ Cleanup failed:', cleanupError.message); - } - } -} - -// Run the test -testGitPushSilentFailure().catch(console.error); \ No newline at end of file diff --git a/js/examples/test-git-push-with-errexit.mjs b/js/examples/test-git-push-with-errexit.mjs deleted file mode 100755 index adf259a..0000000 --- a/js/examples/test-git-push-with-errexit.mjs +++ /dev/null @@ -1,78 +0,0 @@ -#!/usr/bin/env node - -// Test script to reproduce git push silent failure with errexit enabled -import { $, shell } from '../src/$.mjs'; -import fs from 'fs'; -import path from 'path'; -import { tmpdir } from 'os'; - -async function testGitPushWithReachExit() { - console.log('๐Ÿ”ง Testing git push with errexit enabled...\n'); - - // Create a temporary directory for testing - const testDir = path.join(tmpdir(), `git-push-errexit-test-${Date.now()}`); - - try { - console.log(`๐Ÿ“ Creating test directory: ${testDir}`); - await $`mkdir -p ${testDir}`; - - // Initialize git repository - console.log('๐Ÿ”„ Initializing git repository...'); - await $`cd ${testDir} && git init`; - - // Configure git user (required for commits) - await $`cd ${testDir} && git config user.email "test@example.com"`; - await $`cd ${testDir} && git config user.name "Test User"`; - - // Create a test file and commit to main branch - console.log('๐Ÿ“ Creating test file and committing to main...'); - await $`cd ${testDir} && git checkout -b main`; - await $`cd ${testDir} && echo "test content" > test.txt`; - await $`cd ${testDir} && git add test.txt`; - await $`cd ${testDir} && git commit -m "Initial commit"`; - - console.log('\n๐Ÿ”ง Test without errexit (default behavior)...'); - shell.errexit(false); - try { - const result = await $`cd ${testDir} && git remote add origin https://github.com/nonexistent/repo.git`; - const pushResult = await $`cd ${testDir} && git push -u origin main`; - console.log('โœ… Command executed without throwing'); - console.log('Exit code:', pushResult.code); - console.log('Stderr length:', pushResult.stderr?.length || 0); - console.log('Has error in stderr:', pushResult.stderr?.includes('error:') || false); - } catch (error) { - console.log('โŒ Command threw error (unexpected):', error.message); - } - - console.log('\n๐Ÿ”ง Test with errexit enabled...'); - shell.errexit(true); - try { - const pushResult = await $`cd ${testDir} && git push origin main`; - console.log('๐Ÿšจ Command completed without throwing (this is the bug!)'); - console.log('Exit code:', pushResult.code); - console.log('Stderr:', pushResult.stderr); - } catch (error) { - console.log('โœ… Command correctly threw error:', error.message); - console.log('Error code:', error.code); - } - - } catch (error) { - console.log('โŒ Test failed with error:', error.message); - console.log('Error details:', error); - } finally { - // Reset errexit - shell.errexit(false); - - // Cleanup - console.log('\n๐Ÿงน Cleaning up test directory...'); - try { - await $`rm -rf ${testDir}`; - console.log('โœ… Cleanup completed'); - } catch (cleanupError) { - console.log('โš ๏ธ Cleanup failed:', cleanupError.message); - } - } -} - -// Run the test -testGitPushWithReachExit().catch(console.error); \ No newline at end of file diff --git a/js/examples/test-simple-git-push.mjs b/js/examples/test-simple-git-push.mjs deleted file mode 100755 index c9fa160..0000000 --- a/js/examples/test-simple-git-push.mjs +++ /dev/null @@ -1,83 +0,0 @@ -#!/usr/bin/env node - -// Test to isolate the git push issue without redirection -import { $ } from '../src/$.mjs'; -import fs from 'fs/promises'; -import path from 'path'; -import { tmpdir } from 'os'; - -async function testSimpleGitPush() { - console.log('๐Ÿ”ง Testing simple git push without redirection...\n'); - - // Create a temporary directory for testing - const testDir = path.join(tmpdir(), `simple-git-test-${Date.now()}`); - - try { - console.log(`๐Ÿ“ Creating test directory: ${testDir}`); - await $`mkdir -p ${testDir}`; - - // Initialize git repository - console.log('๐Ÿ”„ Initializing git repository...'); - await $`cd ${testDir} && git init`; - - // Configure git user (required for commits) - await $`cd ${testDir} && git config user.email "test@example.com"`; - await $`cd ${testDir} && git config user.name "Test User"`; - - // Create a test file and commit to the correct branch - console.log('๐Ÿ“ Creating test file and committing...'); - await fs.writeFile(path.join(testDir, 'test.txt'), 'Test content'); - await $`cd ${testDir} && git add test.txt`; - await $`cd ${testDir} && git commit -m "Test commit"`; - - // Check which branch we're on - const branchResult = await $`cd ${testDir} && git branch --show-current`; - console.log('Current branch:', branchResult.stdout.trim()); - - // Add a remote that doesn't exist - await $`cd ${testDir} && git remote add origin https://github.com/nonexistent/test-repo.git`; - - console.log('\n๐Ÿ” Test 1: git push without redirection...'); - const pushResult = await $`cd ${testDir} && git push -u origin ${branchResult.stdout.trim()}`; - console.log('Results:'); - console.log(' Exit code:', pushResult.code); - console.log(' Stdout length:', pushResult.stdout?.length || 0); - console.log(' Stderr length:', pushResult.stderr?.length || 0); - console.log(' Stdout:', JSON.stringify(pushResult.stdout || '')); - console.log(' Stderr:', JSON.stringify(pushResult.stderr || '')); - - console.log('\n๐Ÿ” Test 2: git push with explicit 2>&1 redirection...'); - const pushRedirectResult = await $`cd ${testDir} && git push -u origin ${branchResult.stdout.trim()} 2>&1`; - console.log('Results:'); - console.log(' Exit code:', pushRedirectResult.code); - console.log(' Stdout length:', pushRedirectResult.stdout?.length || 0); - console.log(' Stderr length:', pushRedirectResult.stderr?.length || 0); - console.log(' Stdout:', JSON.stringify(pushRedirectResult.stdout || '')); - console.log(' Stderr:', JSON.stringify(pushRedirectResult.stderr || '')); - - console.log('\n๐Ÿ” Test 3: git push to a different fake remote...'); - await $`cd ${testDir} && git remote set-url origin https://fake-host-that-does-not-exist.com/repo.git`; - const pushFakeResult = await $`cd ${testDir} && git push origin ${branchResult.stdout.trim()}`; - console.log('Results:'); - console.log(' Exit code:', pushFakeResult.code); - console.log(' Stdout length:', pushFakeResult.stdout?.length || 0); - console.log(' Stderr length:', pushFakeResult.stderr?.length || 0); - console.log(' Stdout:', JSON.stringify(pushFakeResult.stdout || '')); - console.log(' Stderr:', JSON.stringify(pushFakeResult.stderr || '')); - - } catch (error) { - console.log('โŒ Test failed with error:', error.message); - } finally { - // Cleanup - console.log('\n๐Ÿงน Cleaning up test directory...'); - try { - await $`rm -rf ${testDir}`; - console.log('โœ… Cleanup completed'); - } catch (cleanupError) { - console.log('โš ๏ธ Cleanup failed:', cleanupError.message); - } - } -} - -// Run the test -testSimpleGitPush().catch(console.error); \ No newline at end of file diff --git a/js/examples/test-verbose-git-push.mjs b/js/examples/test-verbose-git-push.mjs deleted file mode 100755 index 5f4e672..0000000 --- a/js/examples/test-verbose-git-push.mjs +++ /dev/null @@ -1,78 +0,0 @@ -#!/usr/bin/env node - -// Test git push with verbose logging to trace execution -import { $ } from '../src/$.mjs'; -import fs from 'fs/promises'; -import path from 'path'; -import { tmpdir } from 'os'; - -// Enable verbose logging -process.env.COMMAND_STREAM_VERBOSE = 'true'; - -async function testVerboseGitPush() { - console.log('๐Ÿ”ง Testing git push with verbose logging enabled...\n'); - - // Create a temporary directory for testing - const testDir = path.join(tmpdir(), `verbose-git-test-${Date.now()}`); - - try { - console.log(`๐Ÿ“ Creating test directory: ${testDir}`); - await $`mkdir -p ${testDir}`; - - // Initialize git repository - console.log('๐Ÿ”„ Initializing git repository...'); - await $`cd ${testDir} && git init`; - - // Configure git user (required for commits) - await $`cd ${testDir} && git config user.email "test@example.com"`; - await $`cd ${testDir} && git config user.name "Test User"`; - - // Create a test file and commit - console.log('๐Ÿ“ Creating test file and committing...'); - await fs.writeFile(path.join(testDir, 'test.txt'), 'Test content'); - await $`cd ${testDir} && git add test.txt`; - await $`cd ${testDir} && git commit -m "Test commit"`; - - // Check which branch we're on - const branchResult = await $`cd ${testDir} && git branch --show-current`; - const branch = branchResult.stdout.trim(); - console.log('Current branch:', branch); - - // Add a remote that doesn't exist - await $`cd ${testDir} && git remote add origin https://github.com/nonexistent/test-repo.git`; - - console.log('\n๐Ÿ” Test 1: git push without redirection (verbose logging enabled)...'); - console.log('='.repeat(80)); - const pushResult1 = await $`cd ${testDir} && git push -u origin ${branch}`; - console.log('='.repeat(80)); - console.log('Results:'); - console.log(' Exit code:', pushResult1.code); - console.log(' Stdout:', JSON.stringify(pushResult1.stdout || '')); - console.log(' Stderr:', JSON.stringify(pushResult1.stderr || '')); - - console.log('\n๐Ÿ” Test 2: git push WITH 2>&1 redirection (verbose logging enabled)...'); - console.log('='.repeat(80)); - const pushResult2 = await $`cd ${testDir} && git push -u origin ${branch} 2>&1`; - console.log('='.repeat(80)); - console.log('Results:'); - console.log(' Exit code:', pushResult2.code); - console.log(' Stdout:', JSON.stringify(pushResult2.stdout || '')); - console.log(' Stderr:', JSON.stringify(pushResult2.stderr || '')); - - } catch (error) { - console.log('โŒ Test failed with error:', error.message); - console.log('Error stack:', error.stack); - } finally { - // Cleanup - console.log('\n๐Ÿงน Cleaning up test directory...'); - try { - await $`rm -rf ${testDir}`; - console.log('โœ… Cleanup completed'); - } catch (cleanupError) { - console.log('โš ๏ธ Cleanup failed:', cleanupError.message); - } - } -} - -// Run the test -testVerboseGitPush().catch(console.error); \ No newline at end of file diff --git a/js/src/$.process-runner-execution.mjs b/js/src/$.process-runner-execution.mjs index fcf31b0..468e9c8 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 33cf8e3..0705346 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/git-push-silent-failure.test.mjs b/js/tests/git-push-silent-failure.test.mjs deleted file mode 100644 index 6785b4b..0000000 --- a/js/tests/git-push-silent-failure.test.mjs +++ /dev/null @@ -1,101 +0,0 @@ -import { test, expect, describe, beforeEach, afterEach } from 'bun:test'; -import './test-helper.mjs'; // Automatically sets up beforeEach/afterEach cleanup -import { $, shell } from '../src/$.mjs'; -import fs from 'fs/promises'; -import path from 'path'; -import { tmpdir } from 'os'; - -describe('Git push silent failure fix (Issue #46)', () => { - let testDir; - - beforeEach(async () => { - // Create temp directory for each test - testDir = path.join(tmpdir(), `git-test-${Date.now()}-${Math.random().toString(36).slice(2)}`); - await $`mkdir -p ${testDir}`; - - // Initialize git repo - await $`cd ${testDir} && git init`; - await $`cd ${testDir} && git config user.email "test@example.com"`; - await $`cd ${testDir} && git config user.name "Test User"`; - - // Create and commit a test file - await fs.writeFile(path.join(testDir, 'test.txt'), 'Test content'); - await $`cd ${testDir} && git add test.txt`; - await $`cd ${testDir} && git commit -m "Test commit"`; - - // Add non-existent remote - await $`cd ${testDir} && git remote add origin https://github.com/nonexistent/test-repo.git`; - - // Get current branch name - const branchResult = await $`cd ${testDir} && git branch --show-current`; - global.testBranch = branchResult.stdout.trim(); - }); - - afterEach(async () => { - if (testDir) { - try { - await $`rm -rf ${testDir}`; - } catch (e) { - // Ignore cleanup errors - } - } - }); - - test('git push without redirection should return proper exit code and stderr', async () => { - const result = await $`cd ${testDir} && git push origin ${global.testBranch}`; - - expect(result.code).not.toBe(0); // Should fail - expect(result.stderr).toContain('fatal:'); // Should have error in stderr - expect(result.stdout).toBe(''); // stdout should be empty - }); - - test('git push with 2>&1 redirection should return proper exit code and stdout', async () => { - const result = await $`cd ${testDir} && git push origin ${global.testBranch} 2>&1`; - - expect(result.code).not.toBe(0); // Should fail (not return 0 like before the fix) - expect(result.stdout).toContain('fatal:'); // Error should be in stdout due to redirection - expect(result.stderr).toBe(''); // stderr should be empty due to redirection - }); - - test('git push with errexit enabled should throw on failure', async () => { - shell.errexit(true); - - try { - await expect(async () => { - await $`cd ${testDir} && git push origin ${global.testBranch}`; - }).toThrow(); - } finally { - shell.errexit(false); - } - }); - - test('git push with 2>&1 and errexit should throw on failure', async () => { - shell.errexit(true); - - try { - await expect(async () => { - await $`cd ${testDir} && git push origin ${global.testBranch} 2>&1`; - }).toThrow(); - } finally { - shell.errexit(false); - } - }); - - test('complex command with 2>&1 should not trigger virtual cd command bug', async () => { - // This was the specific case that caused the bug: - // The command was incorrectly parsed as a virtual `cd` command with all the rest as args - const result = await $`cd ${testDir} && git push origin ${global.testBranch} 2>&1`; - - expect(result.code).not.toBe(0); // Should fail, not return 0 from virtual cd - expect(result.stdout).toContain('fatal:'); // Should contain actual git error - expect(result.stdout).not.toContain('cd:'); // Should not contain cd command errors - }); - - test('other shell features that need real shell still work', async () => { - // Test that other needsRealShell features still work - const result = await $`cd ${testDir} && echo "test" > output.txt && cat output.txt`; - - expect(result.code).toBe(0); - expect(result.stdout.trim()).toBe('test'); - }); -}); \ No newline at end of file diff --git a/js/tests/redirection-silent-failure.test.mjs b/js/tests/redirection-silent-failure.test.mjs new file mode 100644 index 0000000..4113a51 --- /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 0000000..9240254 --- /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 849e826..1081fad 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 a7dd489..00c3d0c 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 0000000..102e168 --- /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 <