Skip to content

Fix GitHub search queries with labels containing spaces (Issue #48) - #101

Merged
konard merged 7 commits into
mainfrom
issue-48-ad9abe01
Sep 5, 2026
Merged

Fix GitHub search queries with labels containing spaces (Issue #48)#101
konard merged 7 commits into
mainfrom
issue-48-ad9abe01

Conversation

@konard

@konard konard commented Sep 9, 2025

Copy link
Copy Markdown
Member

Summary

Fixes #48 — GitHub search queries with labels containing spaces failed because command-stream did not perform full POSIX quote removal when handing arguments to commands.

The canonical repro:

const label = 'help wanted';
await $`gh issue list --label "${label}"`;

Before this change, command-stream only stripped quotes from words that were wholly quoted. A word with an embedded quote — the common shape after interpolation, e.g. label:"help wanted" or label:'help wanted' — kept its quotes or was split at the space, diverging from how /bin/sh behaves.

Root cause

POSIX "quote removal" allows quotes to appear anywhere inside a word; the shell strips the quote characters and concatenates the quoted and unquoted pieces into a single argument (label:'help wanted'label:help wanted). command-stream's parser only handled the fully-wrapped case ("…" / '…'), so mid-word quotes leaked through.

Approach

Per the maintainer request, the behavior now follows /bin/sh (least surprise vs. competitors), verified by diffing against /bin/sh across 22 cases.

  • POSIX quote removal per argument in every parse path via removeShellQuotes (JS) / remove_shell_quotes (Rust):
    • '…' is fully literal;
    • "…" honors \ escapes only for $ ` " \ and newline;
    • an unquoted \ escapes the next character;
    • quoted and unquoted pieces concatenate into one argument.
  • raw is preserved for each argument (the original word with its quotes) so re-serializing a command back to a real shell round-trips exactly — quotes are only removed for values delivered to built-in/virtual commands.
  • Rust virtual-command dispatch now uses quote-aware split_command_words instead of naive split_whitespace, which previously split inside quotes.
  • Tokenizer infinite-loop fix: a lone & (as in 2>&1, or backgrounding) matched neither the && operator nor advanced the word scanner (it was in the word stop-set), so it spun forever. This was latent until the virtual path began tokenizing raw commands. The tokenizer now always makes progress, and the virtual path skips tokenizing commands already routed to a real shell.

JavaScript and Rust implementations are kept in parity (the language-parity check passes).

How to reproduce / verify

node experiments/issue-48-quote-removal-parity.mjs
# 21/22 match /bin/sh; the only divergence is the malformed unterminated-quote
# input "it's", where /bin/sh itself errors (out of scope).

Tests

  • JSjs/tests/github-search-escaping.test.mjs: unit tests for removeShellQuotes, the exact issue shape (--label "value"), spawned-process argv assertions, /bin/sh word-splitting parity, and a custom virtual command receiving quote-removed args.
  • Rust — unit tests for remove_shell_quotes / split_command_words and tokenizer termination (lone &); integration tests for embedded-quote echo.
  • Experimentexperiments/issue-48-quote-removal-parity.mjs diffs 22 quoting cases against /bin/sh (built-in and spawned paths).

Before / after

const label = 'help wanted';
await $`echo label:"${label}"`;
// Before: label:"help wanted"   (quotes leaked through)
// After:  label:help wanted     (matches /bin/sh)

A changeset is included (js/.changeset/gh-search-label-spaces.md, patch).

Resolves #48

🤖 Generated with Claude Code

Adding CLAUDE.md with task information for AI processing.
This file will be removed when the task is complete.

Issue: #48
@konard konard self-assigned this Sep 9, 2025
konard and others added 2 commits September 9, 2025 19:53
Root cause: GitHub CLI expects search terms as separate arguments, not as a single quoted string. The previous quote() function was over-quoting values, causing nested quote issues in template literals.

Changes:
- Modified quote() function to prefer double quotes for simple spaced strings
- This eliminates nested single-quote-inside-double-quote problems
- Maintains backward compatibility for complex strings with quotes
- Updated existing tests to reflect improved quoting behavior

Technical details:
- Simple spaced strings like "help wanted" now use double quotes: "help wanted"
- Strings with single quotes still use traditional escaping: 'it'\''s'
- Strings with double quotes use single quotes: 'has"quotes'
- No change for safe strings without spaces: nospaces

Examples that now work:
- await $`gh search issues repo:owner/repo label:${labelWithSpaces}`
- await $`gh issue list --label "${label}"`

Fixes #48

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
@konard konard changed the title [WIP] GitHub search queries with labels containing spaces fail Fix GitHub search queries with labels containing spaces (Issue #48) Sep 9, 2025
@konard
konard marked this pull request as ready for review September 9, 2025 17:11
@konard

konard commented Sep 5, 2026

Copy link
Copy Markdown
Member Author

We need to get latest changes from default branch.

We should double check more cases similar like this, make sure we have test coverage similar to all our competitors, and select behavior closer to how it would behave in sh scripts or with least surprise based on best practices from competitors. If there multiple options we should allow to configure, and use closer to sh behavior by default.

@konard
konard marked this pull request as draft September 5, 2026 13:57
@konard

konard commented Sep 5, 2026

Copy link
Copy Markdown
Member Author

🤖 AI Work Session Started

Starting automated work session at 2026-09-05T13:57:57.983Z

The PR has been converted to draft mode while work is in progress.

This comment marks the beginning of an AI work session. Please wait for the session to finish, and provide your feedback.

The repository was restructured (js/ and rust/ workspaces) and main now has
quote-context aware interpolation (issue #49), which supersedes the earlier
attempt in this branch to make quote() prefer double quotes. Resolved all
conflicts in favour of main; the fix for issue #48 is re-implemented on top
of the current code.
GitHub search queries such as `$`gh issue list --label "${label}"`` (with
`label = "help wanted"`) failed because command-stream only stripped quotes
from wholly-quoted words. Mid-word quotes like `label:"help wanted"` were left
intact or split incorrectly, diverging from POSIX `sh`.

Changes:
- Add POSIX-style quote removal applied per argument in every parse path
  (`removeShellQuotes` / `remove_shell_quotes`): single quotes are literal,
  double quotes honor `\` escapes for `$ ` " \`, and backslash escapes outside
  quotes. Quoted and unquoted pieces concatenate into one argument, matching
  `/bin/sh`.
- Preserve the original word as `raw` so re-serializing a command back to a
  real shell round-trips exactly.
- Route virtual-command dispatch through quote-aware word splitting
  (`split_command_words`) in Rust instead of naive `split_whitespace`, which
  split inside quotes.
- Fix a latent tokenizer infinite loop on a lone `&` (e.g. `2>&1` or
  backgrounding): the word scanner listed `&` in its stop set but no operator
  arm consumed it, so `i` never advanced. Surfaced once the virtual path began
  tokenizing raw commands.
- Guard the Rust virtual path from tokenizing commands already destined for a
  real shell (empty command name).

Tests:
- JS: js/tests/github-search-escaping.test.mjs (unit + spawned-process + sh
  parity + custom virtual command).
- Rust: unit tests for remove_shell_quotes / split_command_words and tokenizer
  termination; integration tests for embedded-quote echo.
- experiments/issue-48-quote-removal-parity.mjs diffs 22 cases against /bin/sh.
Comment on lines +129 to +133
const shOut = execFileSync(
'/bin/sh',
['-c', `${process.execPath} ${argprint} label:"${label}"`],
{ encoding: 'utf8' }
);
@konard
konard marked this pull request as ready for review September 5, 2026 14:55
The unquoted-backslash-as-escape rule in quote removal corrupted Windows
paths (e.g. `cd C:\Users\foo`), breaking the cd invocation isolation
test on windows-latest. Apply POSIX unquoted-backslash escaping only on
non-Windows platforms; keep it literal on Windows where it is the path
separator. Double-quote inner escapes and single-quote handling are
unchanged. Extract readDoubleQuotedSegment in JS to keep complexity within
the lint limit. Tests gated per platform in both languages.
@konard

konard commented Sep 5, 2026

Copy link
Copy Markdown
Member Author

Working session summary

I'll wait for the background CI poller to complete and notify me.


This summary was automatically extracted from the AI working session output.

@konard

konard commented Sep 5, 2026

Copy link
Copy Markdown
Member Author

🤖 Solution Draft Log

This log file contains the complete execution trace of the AI solution draft process.

💰 Cost: $16.147203

📊 Context and tokens usage:

Claude Opus 5: (3 sub-sessions)

  1. 116.7K / 1M (12%) input tokens, 44.2K / 128K (35%) output tokens
  2. 114.7K / 1M (11%) input tokens, 43.4K / 128K (34%) output tokens
  3. 88.4K / 1M (9%) input tokens, 29.7K / 128K (23%) output tokens

Total: (94 new + 82.9K cache writes + 2.6M cache reads) input tokens, 22.3K output tokens, $2.692297 cost

Claude Opus 4.8:

  • 116.7K / 1M (12%) input tokens, 108.2K / 128K (85%) output tokens

Total: (7.0K new + 288.9K cache writes + 15.7M cache reads) input tokens, 108.2K output tokens, $13.454907 cost

🤖 Models used:

  • Tool: Anthropic Claude Code
  • Requested: opus (claude-opus-5)
  • Thinking level: high (~23999 tokens)
  • Main model: Claude Opus 5 (claude-opus-5)
  • Additional models:
    • Claude Opus 4.8 (claude-opus-4-8)

📎 Log file uploaded as Gist (7176KB)


Now working session is ended, feel free to review and add any feedback on the solution draft.

@konard
konard merged commit 753d739 into main Sep 5, 2026
34 checks passed
@konard

konard commented Sep 5, 2026

Copy link
Copy Markdown
Member Author

🎉 Auto-merged

This pull request has been automatically merged by hive-mind.

  • All CI checks have passed

Auto-merged by hive-mind with --auto-merge flag

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

GitHub search queries with labels containing spaces fail

2 participants