Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
104 changes: 104 additions & 0 deletions experiments/issue-48-quote-removal-parity.mjs
Original file line number Diff line number Diff line change
@@ -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);
9 changes: 9 additions & 0 deletions js/.changeset/gh-search-label-spaces.md
Original file line number Diff line number Diff line change
@@ -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.
22 changes: 8 additions & 14 deletions js/src/$.process-runner-execution.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import {
parseShellCommand,
needsRealShell,
hasShellEscapes,
removeShellQuotes,
} from './shell-parser.mjs';
import {
createExitPromise,
Expand Down Expand Up @@ -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' };
Expand Down Expand Up @@ -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 };
Expand Down
7 changes: 6 additions & 1 deletion js/src/$.process-runner-orchestration.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -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}`;
Expand Down
6 changes: 5 additions & 1 deletion js/src/$.process-runner-pipeline.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -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}"`);
Expand Down
120 changes: 105 additions & 15 deletions js/src/shell-parser.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 = {
Expand Down
Loading
Loading