diff --git a/experiments/issue-48-quote-removal-parity.mjs b/experiments/issue-48-quote-removal-parity.mjs new file mode 100644 index 00000000..59915659 --- /dev/null +++ b/experiments/issue-48-quote-removal-parity.mjs @@ -0,0 +1,104 @@ +#!/usr/bin/env node +// Issue #48: labels/search terms containing spaces reach built-in commands +// with their quotes still attached. +// +// A POSIX shell performs *quote removal* on every word: quotes may appear +// anywhere in a word, and the quoted and unquoted pieces are concatenated into +// a single argument (`label:'help wanted'` is one word, `label:help wanted`). +// command-stream's own tokenizer only stripped quotes when the *whole* word was +// wrapped in them, so `gh search issues label:${label}` (which interpolates as +// `label:'help wanted'`) kept the quotes as literal characters. +// +// This script diffs command-stream against /bin/sh, printing each argument the +// command actually received, so any divergence in word splitting or quote +// removal is visible. +import { spawnSync } from 'child_process'; +import { fileURLToPath } from 'url'; +import { dirname, join } from 'path'; +import { $ } from '../js/src/$.mjs'; + +const here = dirname(fileURLToPath(import.meta.url)); +const argprint = join(here, '..', 'js', 'tests', 'fixtures', 'argprint.mjs'); + +// Each case is the argument list appended to a command; `echo` exercises +// command-stream's built-in path, argprint the spawned-process path. +const ARGS = [ + // The shape from the issue: an interpolated value inside a search term. + "label:'help wanted'", + 'label:"help wanted"', + "repo:o/r label:'help wanted' is:open", + "--label='help wanted'", + '--label="help wanted"', + // Fully quoted words (these already worked). + "'help wanted'", + '"help wanted"', + // Quotes in the middle / at the end of a word. + "a'b c'd", + 'a"b c"d', + "pre'post'", + "'pre'post", + // Adjacent quoted sections concatenate into one word. + "'a''b'", + '\'a\'"b"', + "a''b", + // Empty words. + "''", + '""', + "x '' y", + // A quote character inside the other kind of quotes stays literal. + '"it\'s"', + '\'say "hi"\'', + "it's", + // Multiple quoted words on one line. + "'a b' 'c d'", + // The POSIX idiom for a literal single quote, as produced by quote(). + "'it'\\''s here'", +]; + +let failures = 0; +for (const args of ARGS) { + const sh = spawnSync('/bin/sh', ['-c', `node ${argprint} ${args}`], { + encoding: 'utf8', + }); + const expected = sh.stdout; + + let viaEcho; + try { + const r = await $({ mirror: false })`${{ raw: `echo ${args}` }}`; + viaEcho = r.stdout; + } catch (e) { + viaEcho = `ERROR ${e.message}`; + } + const shEcho = spawnSync('/bin/sh', ['-c', `echo ${args}`], { + encoding: 'utf8', + }).stdout; + + let viaSpawn; + try { + const r = await $({ + mirror: false, + })`${{ raw: `node ${argprint} ${args}` }}`; + viaSpawn = r.stdout; + } catch (e) { + viaSpawn = `ERROR ${e.message}`; + } + + const sameEcho = shEcho === viaEcho; + const sameSpawn = expected === viaSpawn; + const same = sameEcho && sameSpawn; + if (!same) { + failures++; + } + console.log(`${same ? 'OK ' : 'DIFF'} ${JSON.stringify(args)}`); + if (!sameEcho) { + console.log(` echo sh: ${JSON.stringify(shEcho)}`); + console.log(` echo cs: ${JSON.stringify(viaEcho)}`); + } + if (!sameSpawn) { + console.log(` argv sh: ${JSON.stringify(expected)}`); + console.log(` argv cs: ${JSON.stringify(viaSpawn)}`); + } +} + +console.log(`\n${failures} divergence(s) out of ${ARGS.length}`); +process.exit(failures === 0 ? 0 : 1); diff --git a/js/.changeset/gh-search-label-spaces.md b/js/.changeset/gh-search-label-spaces.md new file mode 100644 index 00000000..4563f38d --- /dev/null +++ b/js/.changeset/gh-search-label-spaces.md @@ -0,0 +1,9 @@ +--- +'command-stream': patch +--- + +Fix shell quote removal so interpolated values containing spaces (e.g. GitHub search labels) reach commands as a single argument. + +Previously `$\`gh issue list --label "${label}"\``(with`label = "help wanted"`) failed because command-stream only stripped quotes from wholly-quoted words. Mid-word quotes such as `label:"help wanted"`were left intact or split incorrectly, diverging from POSIX`sh`. + +The parser now performs POSIX-style quote removal per argument (single quotes are literal; double quotes honor `\` escapes for ``$ ` " \``; backslash escapes outside quotes on POSIX platforms), so quoted and unquoted pieces concatenate into one argument — matching `/bin/sh`. Virtual commands (`echo`, custom handlers) receive the quote-removed value, while the original word is preserved for faithful re-serialization to a real shell. On Windows an unquoted backslash is kept literal so paths such as `cd C:\Users\foo` survive intact. The Rust implementation is updated in parity. diff --git a/js/src/$.process-runner-execution.mjs b/js/src/$.process-runner-execution.mjs index 468e9c86..89504426 100644 --- a/js/src/$.process-runner-execution.mjs +++ b/js/src/$.process-runner-execution.mjs @@ -19,6 +19,7 @@ import { parseShellCommand, needsRealShell, hasShellEscapes, + removeShellQuotes, } from './shell-parser.mjs'; import { createExitPromise, @@ -1361,13 +1362,10 @@ export function attachExecutionMethods(ProcessRunner, deps) { const cmd = parts[0]; const args = parts.slice(1).map((arg) => { - if ( - (arg.startsWith('"') && arg.endsWith('"')) || - (arg.startsWith("'") && arg.endsWith("'")) - ) { - return { value: arg.slice(1, -1), quoted: true, quoteChar: arg[0] }; - } - return { value: arg, quoted: false }; + // POSIX quote removal so `label:'help wanted'` reaches a built-in as the + // single argument `label:help wanted`, not with literal quotes (#48). + const { value, quoted, quoteChar } = removeShellQuotes(arg); + return { value, quoted, quoteChar: quoteChar ?? undefined, raw: arg }; }); return { cmd, args, type: 'simple' }; @@ -1411,13 +1409,9 @@ export function attachExecutionMethods(ProcessRunner, deps) { const cmd = parts[0]; const args = parts.slice(1).map((arg) => { - if ( - (arg.startsWith('"') && arg.endsWith('"')) || - (arg.startsWith("'") && arg.endsWith("'")) - ) { - return { value: arg.slice(1, -1), quoted: true, quoteChar: arg[0] }; - } - return { value: arg, quoted: false }; + // POSIX quote removal (see _parseCommand) applied per pipeline stage. + const { value, quoted, quoteChar } = removeShellQuotes(arg); + return { value, quoted, quoteChar: quoteChar ?? undefined, raw: arg }; }); return { cmd, args }; diff --git a/js/src/$.process-runner-orchestration.mjs b/js/src/$.process-runner-orchestration.mjs index e3979801..84f89b58 100644 --- a/js/src/$.process-runner-orchestration.mjs +++ b/js/src/$.process-runner-orchestration.mjs @@ -102,7 +102,12 @@ async function handleRedirects(result, redirects, cwd) { function buildCommandString(cmd, args, redirects) { let commandStr = cmd; for (const arg of args) { - if (arg.quoted && arg.quoteChar) { + // `raw` preserves the original word (with its quotes) so re-serializing to a + // real shell round-trips exactly; fall back to the older re-quoting only for + // args produced without it. + if (arg.raw !== undefined) { + commandStr += ` ${arg.raw}`; + } else if (arg.quoted && arg.quoteChar) { commandStr += ` ${arg.quoteChar}${arg.value}${arg.quoteChar}`; } else if (arg.value !== undefined) { commandStr += ` ${arg.value}`; diff --git a/js/src/$.process-runner-pipeline.mjs b/js/src/$.process-runner-pipeline.mjs index cbe2a66b..413d8905 100644 --- a/js/src/$.process-runner-pipeline.mjs +++ b/js/src/$.process-runner-pipeline.mjs @@ -81,7 +81,11 @@ function buildCommandParts(command) { const parts = [cmd]; for (const arg of args) { if (arg.value !== undefined) { - if (arg.quoted) { + // `raw` preserves the word exactly as written (quotes and all), so + // handing it back to a real shell round-trips without re-quoting guesses. + if (arg.raw !== undefined) { + parts.push(arg.raw); + } else if (arg.quoted) { parts.push(`${arg.quoteChar}${arg.value}${arg.quoteChar}`); } else if (arg.value.includes(' ')) { parts.push(`"${arg.value}"`); diff --git a/js/src/shell-parser.mjs b/js/src/shell-parser.mjs index 0705346f..324affa6 100644 --- a/js/src/shell-parser.mjs +++ b/js/src/shell-parser.mjs @@ -22,6 +22,105 @@ const TokenType = { EOF: 'eof', }; +/** + * Perform POSIX quote removal on a single already-tokenized word. + * + * A shell word may carry quotes anywhere inside it, not just wrapped around + * the whole thing: `label:'help wanted'`, `--flag="a b"` and `a'b c'd` are all + * one word each. The shell strips the quote characters and concatenates the + * quoted and unquoted pieces into a single argument. Our tokenizer keeps the + * quotes in the word (so it can split correctly and hand a valid command back + * to a real shell when needed); this function turns that raw word into the + * literal value a built-in command should receive, exactly as `/bin/sh` would. + * + * Rules mirrored from POSIX: + * - Outside quotes, a backslash escapes the next character (it becomes + * literal and loses any quoting role). On Windows the backslash is the + * path separator, so an unquoted backslash is kept literal there - eating + * it would corrupt paths such as `cd C:\Users\foo`. + * - Inside '...', every character is literal, including backslash. + * - Inside "...", a backslash only escapes $, `, ", \ and newline; before + * anything else it stays a literal backslash. + * + * @param {string} word - Raw word including any quote characters + * @returns {{value: string, quoted: boolean, quoteChar: (string|null)}} + * `value` is the quote-removed text, `quoted` is true when any quoting or + * escaping was applied, and `quoteChar` is the first quote character seen + * (kept for callers that re-serialize simple wholly-quoted words). + */ +/** + * Read the body of a double-quoted segment starting just after the opening + * quote. Backslash only escapes the small POSIX set inside double quotes; any + * other backslash stays literal. + * + * @param {string} word - The full word being scanned + * @param {number} start - Index of the first character inside the quotes + * @returns {{value: string, next: number}} The unescaped body and the index + * just past the closing quote (or the end of the word if unterminated). + */ +function readDoubleQuotedSegment(word, start) { + let value = ''; + let i = start; + while (i < word.length && word[i] !== '"') { + if (word[i] === '\\' && isDoubleQuoteEscape(word[i + 1])) { + value += word[i + 1]; + i += 2; + continue; + } + value += word[i]; + i++; + } + return { value, next: i + 1 }; +} + +export function removeShellQuotes(word) { + let value = ''; + let quoted = false; + let quoteChar = null; + let i = 0; + + while (i < word.length) { + const char = word[i]; + + if (char === "'") { + quoted = true; + if (quoteChar === null) { + quoteChar = "'"; + } + i++; + while (i < word.length && word[i] !== "'") { + value += word[i]; + i++; + } + i++; // skip the closing quote (if any) + continue; + } + + if (char === '"') { + quoted = true; + if (quoteChar === null) { + quoteChar = '"'; + } + const segment = readDoubleQuotedSegment(word, i + 1); + value += segment.value; + i = segment.next; // position past the closing quote (if any) + continue; + } + + if (char === '\\' && i + 1 < word.length && process.platform !== 'win32') { + quoted = true; + value += word[i + 1]; + i += 2; + continue; + } + + value += char; + i++; + } + + return { value, quoted, quoteChar }; +} + /** * Parse a word token from the command string, handling quotes and escapes * @param {string} command - The command string @@ -353,21 +452,12 @@ class ShellParser { const cmd = words[0]; const args = words.slice(1).map((word) => { - // Remove quotes if present - if ( - (word.startsWith('"') && word.endsWith('"')) || - (word.startsWith("'") && word.endsWith("'")) - ) { - return { - value: word.slice(1, -1), - quoted: true, - quoteChar: word[0], - }; - } - return { - value: word, - quoted: false, - }; + // POSIX quote removal: strip quotes wherever they appear in the word and + // concatenate the pieces, so `label:'help wanted'` becomes one argument + // `label:help wanted` (issue #48). `raw` keeps the original text for the + // paths that re-serialize the command back to a real shell. + const { value, quoted, quoteChar } = removeShellQuotes(word); + return { value, quoted, quoteChar: quoteChar ?? undefined, raw: word }; }); const result = { diff --git a/js/tests/github-search-escaping.test.mjs b/js/tests/github-search-escaping.test.mjs new file mode 100644 index 00000000..defc468e --- /dev/null +++ b/js/tests/github-search-escaping.test.mjs @@ -0,0 +1,179 @@ +#!/usr/bin/env node + +// Regression tests for issue #48: +// "GitHub search queries with labels containing spaces fail due to multiple +// layers of escaping issues when passed through command-stream." +// +// The canonical repro is: +// const label = 'help wanted'; +// await $`gh issue list --label "${label}"`; +// The label must arrive at the command as a single argument `help wanted` +// (quotes removed), matching how POSIX `sh` performs quote removal. + +import { test, expect, describe, beforeEach } from 'bun:test'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { execFileSync } from 'node:child_process'; +import { + $, + shell, + enableVirtualCommands, + register, + unregister, +} from '../src/$.mjs'; +import { removeShellQuotes } from '../src/shell-parser.mjs'; +import { isWindows } from './test-helper.mjs'; + +const fixturesDir = path.join( + path.dirname(fileURLToPath(import.meta.url)), + 'fixtures' +); +const argprint = path.join(fixturesDir, 'argprint.mjs'); + +function setup() { + shell.errexit(false); + shell.verbose(false); + shell.xtrace(false); + enableVirtualCommands(); +} + +describe('removeShellQuotes (POSIX quote removal)', () => { + test('removes quotes wrapping a whole word', () => { + expect(removeShellQuotes("'help wanted'").value).toBe('help wanted'); + expect(removeShellQuotes('"help wanted"').value).toBe('help wanted'); + }); + + test('removes quotes embedded mid-word and concatenates', () => { + expect(removeShellQuotes("label:'help wanted'").value).toBe( + 'label:help wanted' + ); + expect(removeShellQuotes('label:"help wanted"').value).toBe( + 'label:help wanted' + ); + expect(removeShellQuotes("--label='help wanted'").value).toBe( + '--label=help wanted' + ); + expect(removeShellQuotes("a'b c'd").value).toBe('ab cd'); + }); + + // Unquoted backslash escaping is POSIX-only. On Windows the backslash is the + // path separator, so it is kept literal there (see the Windows path test). + test.skipIf(isWindows)('handles the POSIX single-quote escape idiom', () => { + expect(removeShellQuotes("'it'\\''s here'").value).toBe("it's here"); + }); + + test.skipIf(isWindows)('handles unquoted backslash escapes', () => { + expect(removeShellQuotes('a\\ b').value).toBe('a b'); + }); + + test('handles double-quoted backslash escapes', () => { + // Inside double quotes a backslash only escapes a small set; this is the + // same on every platform. + expect(removeShellQuotes('"a\\"b"').value).toBe('a"b'); + expect(removeShellQuotes('"a\\nb"').value).toBe('a\\nb'); + }); + + test.skipIf(!isWindows)( + 'keeps unquoted backslashes literal on Windows', + () => { + // A Windows path handed to a virtual command (e.g. `cd C:\Users\foo`) must + // survive intact rather than losing its separators. + expect(removeShellQuotes('C:\\Users\\foo').value).toBe('C:\\Users\\foo'); + expect(removeShellQuotes('"C:\\Users\\foo"').value).toBe( + 'C:\\Users\\foo' + ); + } + ); + + test('reports quoting flags', () => { + const q = removeShellQuotes("'x'"); + expect(q.value).toBe('x'); + expect(q.quoted).toBe(true); + expect(q.quoteChar).toBe("'"); + + const plain = removeShellQuotes('plain'); + expect(plain.value).toBe('plain'); + expect(plain.quoted).toBe(false); + expect(plain.quoteChar).toBe(null); + }); +}); + +describe('GitHub search escaping (issue #48)', () => { + beforeEach(() => { + setup(); + }); + + test('virtual echo receives a spaced label as one quote-removed argument', async () => { + const label = 'help wanted'; + const result = await $({ mirror: false })`echo label:"${label}"`; + expect(result.stdout.trimEnd()).toBe('label:help wanted'); + }); + + test('virtual echo handles the exact issue shape (--label "value")', async () => { + const label = 'help wanted'; + const result = await $({ mirror: false })`echo --label "${label}"`; + expect(result.stdout.trimEnd()).toBe('--label help wanted'); + }); + + test.skipIf(isWindows)( + 'spawned process receives the spaced label as a single argv entry', + async () => { + const label = 'help wanted'; + const result = await $({ + mirror: false, + })`${process.execPath} ${argprint} --label "${label}"`; + expect(result.stdout).toBe('ARG[--label]\nARG[help wanted]\n'); + } + ); + + test.skipIf(isWindows)( + 'label containing a single quote survives round-trip', + async () => { + const label = "it's complicated"; + const result = await $({ + mirror: false, + })`${process.execPath} ${argprint} --label "${label}"`; + expect(result.stdout).toBe("ARG[--label]\nARG[it's complicated]\n"); + } + ); + + test.skipIf(isWindows)( + 'matches /bin/sh word-splitting for a spaced, embedded label', + async () => { + const label = 'help wanted'; + const csResult = await $({ + mirror: false, + })`${process.execPath} ${argprint} label:"${label}"`; + + const shOut = execFileSync( + '/bin/sh', + ['-c', `${process.execPath} ${argprint} label:"${label}"`], + { encoding: 'utf8' } + ); + + expect(csResult.stdout).toBe(shOut); + expect(csResult.stdout).toBe('ARG[label:help wanted]\n'); + } + ); +}); + +describe('custom virtual command receives quote-removed args (issue #48)', () => { + beforeEach(() => { + setup(); + }); + + test('space-containing argument arrives as a single value', async () => { + const received = []; + register('capture48', async ({ args }) => { + received.push(...args); + return { stdout: '', stderr: '', code: 0 }; + }); + try { + const label = 'help wanted'; + await $({ mirror: false })`capture48 --label "${label}"`; + expect(received).toEqual(['--label', 'help wanted']); + } finally { + unregister('capture48'); + } + }); +}); diff --git a/rust/changelog.d/20260905_145543_quote-removal-label-spaces.md b/rust/changelog.d/20260905_145543_quote-removal-label-spaces.md new file mode 100644 index 00000000..598143f0 --- /dev/null +++ b/rust/changelog.d/20260905_145543_quote-removal-label-spaces.md @@ -0,0 +1,14 @@ +--- +bump: patch +--- + +### Fixed + +- Perform POSIX quote removal per argument so interpolated values containing + spaces (e.g. `gh issue list --label "help wanted"`) reach virtual commands as + a single argument with quotes stripped, matching `/bin/sh` behavior (#48). +- Fix a tokenizer infinite loop on a lone `&` (as in `2>&1` or backgrounding), + which the word scanner previously neither consumed nor treated as an operator. +- Keep an unquoted backslash literal on Windows so virtual commands such as + `cd C:\Users\foo` still receive a valid path (POSIX backslash escaping is + unchanged on other platforms). diff --git a/rust/src/lib.rs b/rust/src/lib.rs index 1081fadb..8476144a 100644 --- a/rust/src/lib.rs +++ b/rust/src/lib.rs @@ -418,9 +418,19 @@ impl ProcessRunner { return None; } - // Parse args from command string - let parts: Vec<&str> = self.command.split_whitespace().collect(); - let args: Vec = parts.iter().skip(1).map(|s| s.to_string()).collect(); + // An empty command name means the caller already decided this command + // must go to a real shell (redirection, expansions, escapes). Bail out + // before tokenizing so we neither waste work nor parse shell syntax we + // deliberately delegate. + if cmd_name.is_empty() { + return None; + } + + // Parse args from command string, respecting quotes and performing + // POSIX quote removal so `echo label:'help wanted'` reaches the built-in + // as the single argument `label:help wanted` (issue #48). + let words = shell_parser::split_command_words(&self.command); + let args: Vec = words.into_iter().skip(1).collect(); let ctx = CommandContext { args, diff --git a/rust/src/pipeline.rs b/rust/src/pipeline.rs index ef5a4c20..1ed333cf 100644 --- a/rust/src/pipeline.rs +++ b/rust/src/pipeline.rs @@ -293,8 +293,10 @@ impl Pipeline { cwd: Option<&PathBuf>, env: Option<&HashMap>, ) -> Option { - let parts: Vec<&str> = full_cmd.split_whitespace().collect(); - let args: Vec = parts.iter().skip(1).map(|s| s.to_string()).collect(); + // Respect quotes and perform POSIX quote removal (issue #48), matching + // the non-pipeline virtual-command path. + let words = crate::shell_parser::split_command_words(full_cmd); + let args: Vec = words.into_iter().skip(1).collect(); let ctx = crate::commands::CommandContext { args, diff --git a/rust/src/shell_parser.rs b/rust/src/shell_parser.rs index 00c3d0c6..dc813b2a 100644 --- a/rust/src/shell_parser.rs +++ b/rust/src/shell_parser.rs @@ -57,6 +57,107 @@ pub struct ParsedArg { pub value: String, pub quoted: bool, pub quote_char: Option, + /// The original word exactly as written, including any quote characters, so + /// callers that re-serialize the command back to a real shell round-trip it + /// without having to re-quote from `value`. + pub raw: String, +} + +/// Perform POSIX quote removal on a single already-tokenized word. +/// +/// A shell word may carry quotes anywhere inside it, not just wrapped around +/// the whole thing: `label:'help wanted'`, `--flag="a b"` and `a'b c'd` are all +/// one word each. The shell strips the quote characters and concatenates the +/// quoted and unquoted pieces into a single argument. Our tokenizer keeps the +/// quotes in the word (so it can split correctly and hand a valid command back +/// to a real shell when needed); this function turns that raw word into the +/// literal value a built-in command should receive, exactly as `/bin/sh` would +/// (issue #48). +/// +/// Rules mirrored from POSIX: +/// - Outside quotes, a backslash escapes the next character (it becomes +/// literal and loses any quoting role). On Windows the backslash is the +/// path separator, so an unquoted backslash is kept literal there — eating +/// it would corrupt paths such as `cd C:\Users\foo`. +/// - Inside `'...'`, every character is literal, including backslash. +/// - Inside `"..."`, a backslash only escapes `$`, `` ` ``, `"`, `\` and +/// newline; before anything else it stays a literal backslash. +/// +/// Returns the quote-removed value, whether any quoting/escaping was applied, +/// and the first quote character seen. +pub fn remove_shell_quotes(word: &str) -> (String, bool, Option) { + let chars: Vec = word.chars().collect(); + let mut value = String::new(); + let mut quoted = false; + let mut quote_char: Option = None; + let mut i = 0; + + while i < chars.len() { + let c = chars[i]; + + if c == '\'' { + quoted = true; + if quote_char.is_none() { + quote_char = Some('\''); + } + i += 1; + while i < chars.len() && chars[i] != '\'' { + value.push(chars[i]); + i += 1; + } + i += 1; // skip the closing quote (if any) + continue; + } + + if c == '"' { + quoted = true; + if quote_char.is_none() { + quote_char = Some('"'); + } + i += 1; + while i < chars.len() && chars[i] != '"' { + if chars[i] == '\\' + && i + 1 < chars.len() + && matches!(chars[i + 1], '$' | '`' | '"' | '\\' | '\n') + { + value.push(chars[i + 1]); + i += 2; + continue; + } + value.push(chars[i]); + i += 1; + } + i += 1; // skip the closing quote (if any) + continue; + } + + if c == '\\' && i + 1 < chars.len() && !cfg!(windows) { + quoted = true; + value.push(chars[i + 1]); + i += 2; + continue; + } + + value.push(c); + i += 1; + } + + (value, quoted, quote_char) +} + +/// Split a simple command string into its words, respecting quotes and applying +/// POSIX quote removal to each word. Operators are ignored, so this is meant for +/// simple commands (the virtual-command dispatch path). Mirrors the JS parser's +/// per-argument quote removal so `echo label:'help wanted'` yields +/// `["echo", "label:help wanted"]` rather than splitting inside the quotes. +pub fn split_command_words(command: &str) -> Vec { + tokenize(command) + .into_iter() + .filter_map(|token| match token.token_type { + TokenType::Word(w) => Some(remove_shell_quotes(&w).0), + _ => None, + }) + .collect() } /// Types of parsed commands @@ -102,6 +203,15 @@ pub fn tokenize(command: &str) -> Vec { value: "&&".to_string(), }); i += 2; + } else if chars[i] == '&' { + // A lone `&` (backgrounding, or the fd-duplication form in `2>&1`) + // is not modeled by this parser. Consume it so the tokenizer always + // makes progress: the word branch below lists `&` in its stop set, + // so without this arm it would break without advancing `i` and spin + // forever. Commands that truly rely on `&`/redirection are routed to + // a real shell (needs_real_shell) before we tokenize for virtual + // command dispatch, so dropping the token here is safe. + i += 1; } else if chars[i] == '|' && i + 1 < chars.len() && chars[i + 1] == '|' { tokens.push(Token { token_type: TokenType::Or, @@ -376,21 +486,17 @@ impl ShellParser { let args: Vec = words .into_iter() .map(|word| { - // Remove quotes if present - if (word.starts_with('"') && word.ends_with('"')) - || (word.starts_with('\'') && word.ends_with('\'')) - { - ParsedArg { - value: word[1..word.len() - 1].to_string(), - quoted: true, - quote_char: Some(word.chars().next().unwrap()), - } - } else { - ParsedArg { - value: word, - quoted: false, - quote_char: None, - } + // POSIX quote removal: strip quotes wherever they appear in the + // word and concatenate the pieces, so `label:'help wanted'` + // becomes one argument `label:help wanted` (issue #48). `raw` + // keeps the original text for paths that re-serialize the + // command back to a real shell. + let (value, quoted, quote_char) = remove_shell_quotes(&word); + ParsedArg { + value, + quoted, + quote_char, + raw: word, } }) .collect(); @@ -553,4 +659,144 @@ mod tests { _ => panic!("Expected Sequence with Subshell"), } } + + // ------------------------------------------------------------------------ + // Quote removal (issue #48) + // ------------------------------------------------------------------------ + + #[test] + fn test_remove_shell_quotes_whole_word() { + assert_eq!(remove_shell_quotes("'help wanted'").0, "help wanted"); + assert_eq!(remove_shell_quotes("\"help wanted\"").0, "help wanted"); + } + + #[test] + fn test_remove_shell_quotes_embedded() { + // The shape from the issue: an interpolated label inside a search term. + assert_eq!( + remove_shell_quotes("label:'help wanted'").0, + "label:help wanted" + ); + assert_eq!( + remove_shell_quotes("label:\"help wanted\"").0, + "label:help wanted" + ); + assert_eq!( + remove_shell_quotes("--label='help wanted'").0, + "--label=help wanted" + ); + } + + #[test] + fn test_remove_shell_quotes_concatenation() { + assert_eq!(remove_shell_quotes("a'b c'd").0, "ab cd"); + assert_eq!(remove_shell_quotes("pre'post'").0, "prepost"); + assert_eq!(remove_shell_quotes("'a''b'").0, "ab"); + assert_eq!(remove_shell_quotes("a''b").0, "ab"); + } + + #[test] + fn test_remove_shell_quotes_escapes() { + // Inside double quotes, backslash only escapes a small set (this is the + // same on every platform). + assert_eq!(remove_shell_quotes("\"a\\\"b\"").0, "a\"b"); + assert_eq!(remove_shell_quotes("\"a\\nb\"").0, "a\\nb"); + + // Unquoted backslash escaping is POSIX-only. On Windows the backslash is + // the path separator, so it stays literal (see the Windows path test). + #[cfg(not(windows))] + { + // POSIX single-quote idiom produced by quote() for a quoted value. + assert_eq!(remove_shell_quotes("'it'\\''s here'").0, "it's here"); + // Backslash escapes a space outside quotes. + assert_eq!(remove_shell_quotes("a\\ b").0, "a b"); + } + } + + // On Windows an unquoted backslash must be preserved so that virtual + // commands like `cd C:\Users\foo` still receive a valid path. + #[cfg(windows)] + #[test] + fn test_remove_shell_quotes_windows_path() { + assert_eq!( + remove_shell_quotes("C:\\Users\\foo").0, + "C:\\Users\\foo".to_string() + ); + // A quoted Windows path is likewise preserved. + assert_eq!( + remove_shell_quotes("\"C:\\Users\\foo\"").0, + "C:\\Users\\foo".to_string() + ); + } + + #[test] + fn test_remove_shell_quotes_flags() { + let (value, quoted, quote_char) = remove_shell_quotes("'x'"); + assert_eq!(value, "x"); + assert!(quoted); + assert_eq!(quote_char, Some('\'')); + + let (value, quoted, quote_char) = remove_shell_quotes("plain"); + assert_eq!(value, "plain"); + assert!(!quoted); + assert_eq!(quote_char, None); + } + + #[test] + fn test_tokenize_terminates_on_lone_ampersand() { + // Regression: a lone `&` (fd duplication `2>&1`, or backgrounding) used + // to spin the tokenizer forever because it matched neither the `&&` + // operator nor advanced the word scanner. It must now terminate. + let tokens = tokenize("git push origin HEAD 2>&1"); + let words: Vec = tokens + .into_iter() + .filter_map(|t| match t.token_type { + TokenType::Word(w) => Some(w), + _ => None, + }) + .collect(); + assert_eq!(words, vec!["git", "push", "origin", "HEAD", "2", "1"]); + } + + #[test] + fn test_split_command_words_terminates_on_background() { + // `echo a & echo b` is not caught by needs_real_shell, so it can reach + // the tokenizer with a lone `&`; it must terminate rather than hang. + assert_eq!( + split_command_words("echo a & echo b"), + vec![ + "echo".to_string(), + "a".to_string(), + "echo".to_string(), + "b".to_string() + ] + ); + } + + #[test] + fn test_split_command_words_quote_removal() { + assert_eq!( + split_command_words("echo label:'help wanted' is:open"), + vec![ + "echo".to_string(), + "label:help wanted".to_string(), + "is:open".to_string() + ] + ); + } + + #[test] + fn test_parse_simple_command_embedded_quotes() { + let cmd = parse_shell_command("gh search issues label:'help wanted'").unwrap(); + match cmd { + ParsedCommand::Simple { cmd, args, .. } => { + assert_eq!(cmd, "gh"); + assert_eq!(args.last().unwrap().value, "label:help wanted"); + assert!(args.last().unwrap().quoted); + // `raw` keeps the original word for re-serialization. + assert_eq!(args.last().unwrap().raw, "label:'help wanted'"); + } + _ => panic!("Expected Simple command"), + } + } } diff --git a/rust/tests/virtual_commands.rs b/rust/tests/virtual_commands.rs index 624283bf..a52dafbb 100644 --- a/rust/tests/virtual_commands.rs +++ b/rust/tests/virtual_commands.rs @@ -122,6 +122,36 @@ async fn test_execute_virtual_echo() { assert!(result.stdout.contains("Hello World")); } +// Quote removal for embedded quotes (issue #48): a search term whose value +// contains a space, e.g. `gh issue list --label "help wanted"`, must reach the +// virtual command as a single argument with the quotes removed. +#[tokio::test] +async fn test_execute_virtual_echo_embedded_single_quotes() { + let _guard = lock_virtual_commands().await; + enable_virtual_commands(); + let result = run("echo label:'help wanted'").await.unwrap(); + assert!(result.is_success()); + assert_eq!(result.stdout.trim_end(), "label:help wanted"); +} + +#[tokio::test] +async fn test_execute_virtual_echo_embedded_double_quotes() { + let _guard = lock_virtual_commands().await; + enable_virtual_commands(); + let result = run("echo label:\"help wanted\"").await.unwrap(); + assert!(result.is_success()); + assert_eq!(result.stdout.trim_end(), "label:help wanted"); +} + +#[tokio::test] +async fn test_execute_virtual_echo_multiple_embedded_terms() { + let _guard = lock_virtual_commands().await; + enable_virtual_commands(); + let result = run("echo label:'help wanted' is:open").await.unwrap(); + assert!(result.is_success()); + assert_eq!(result.stdout.trim_end(), "label:help wanted is:open"); +} + #[tokio::test] async fn test_execute_virtual_pwd() { let _guard = lock_virtual_commands().await;