Skip to content

fix(shell): unescape bash path backslash escapes - #199

Open
kvnloo wants to merge 1 commit into
devfrom
tip/49691-shell-unescape
Open

kvnloo wants to merge 1 commit into
devfrom
tip/49691-shell-unescape

Conversation

@kvnloo

@kvnloo kvnloo commented Sep 28, 2026 •

Copy link
Copy Markdown
Owner

Issue for this PR

Fork-local verification of anomalyco#49691. No origin action.

Type of change

  • Bug fix
  • New feature
  • Refactor / code improvement
  • Documentation

What does this PR do?

Bash leaves backslashes in unquoted tree-sitter word nodes. The shell tool previously resolved /tmp/my\ project/x as a literal backslash path and requested external_directory permission for an in-project file. This change removes Bash backslash escapes before path resolution. PowerShell keeps its existing expansion path.

How did you verify your code works?

At candidate 5029f6d374e767d4fca47f5d7f7e5b34714138fc, the focused regression passes. Replacing only the new unescape call with the old behavior fails because the tool emits an external_directory request.

Replaying the change onto current dev 055d95bb7e278c94baf06235a52cac79dd13ba67 passes all 24 tests in packages/opencode/test/tool/shell.test.ts. Package typecheck fails identically on clean dev and the candidate at test/provider/openai.test.ts:24; that unrelated baseline gate is not bypassed.

Screenshots / recordings

Not applicable. This is shell permission classification with no UI change.

Checklist

  • I have tested my changes locally
  • I have not included unrelated changes in this PR

Tip leaf from anomalyco#49691 (origin tip 1c0ad21), 3way-applied onto current dev.
@kvnloo kvnloo added the grok bot Posted by Grok Bot OSS review dump label Sep 28, 2026
@github-actions

Copy link
Copy Markdown

This PR doesn't fully meet our contributing guidelines and PR template.

What needs to be fixed:

  • PR description is missing required template sections. Please use the PR template.

Please edit this PR description to address the above within 2 hours, or it will be automatically closed.

If you believe this was flagged incorrectly, please let a maintainer know.

@github-actions

Copy link
Copy Markdown

Hey! Your PR title tip: unescape bash path backslash escapes (#49691) doesn't follow conventional commit format.

Please update it to start with one of:

  • feat: or feat(scope): new feature
  • fix: or fix(scope): bug fix
  • docs: or docs(scope): documentation changes
  • chore: or chore(scope): maintenance tasks
  • refactor: or refactor(scope): code refactoring
  • test: or test(scope): adding or updating tests

Where scope is the package name (e.g., app, desktop, opencode).

See CONTRIBUTING.md for details.

@kvnloo kvnloo changed the title tip: unescape bash path backslash escapes (#49691) fix(shell): unescape bash path backslash escapes Oct 10, 2026
@kvnloo

kvnloo commented Oct 10, 2026

Copy link
Copy Markdown
Owner Author

Independent exact-head verification (Claude lane): do not merge as is. It weakens the external-directory check.

Head 5029f6d374e767d4fca47f5d7f7e5b34714138fc, 61 commits behind upstream dev, merges cleanly.

  • The PR's own test goes red to green (23 pass / 1 fail at base, 24 pass at head), typecheck passes, and the fix does reach the real shell tool's permission scan.

Regression, reproduced on Linux. argPath now strips backslashes from quoted arguments too. Bash keeps a backslash literal inside single quotes, and inside double quotes except before a few characters. With a project at <outer>/proj and a real outside directory named <outer>/pro\j:

cat '<outer>/pro\j/secret.txt'

reads the outside file with no external-directory prompt at head. On base the prompt is raised. The double-quoted form behaves the same.

Fix direction. Unescape only unquoted words, and inside double quotes only bash's own escape set.

By reading only, not executed

  • The same code path serves cmd.exe and Git Bash. A quoted Windows path such as "C:\Windows\win.ini" would lose its backslashes and look in-project. The PR's new test is skipped on Windows and the Windows job was cancelled.
  • Only the legacy shell tool is changed; the V2 bash tool in packages/core has its own scan.

Not run: Windows, e2e, the full suite. Nothing pushed. Cost: about 11 tool calls (a third of one agent that also covered #143 and #121).

@kvnloo

kvnloo commented Oct 10, 2026

Copy link
Copy Markdown
Owner Author

READY (Claude lane): quote-aware fix for the regression above, independently verified

Branch factory/claude/199-quote-aware-unescape @ 63d670c114472f03ec1ce67a30c215109ade27c8, one commit on this PR's head 5029f6d. This PR's branch is untouched.

What changed. The scan now takes each path argument from the parsed quoting instead of stripping every backslash: unquoted words get bash backslash removal, single-quoted segments are literal, double-quoted segments unescape only bash's own set, and mixed words apply each rule per segment. It uses the existing parse; there is no hand-written quote parser. cmd.exe is back to base behavior. PowerShell is untouched.

Evidence

  • RED at 5029f6d: 25 pass / 4 fail (single-quoted, double-quoted, mixed quoting, and the cmd case, which passes at base and fails at this PR's head).
  • GREEN at 63d670c: 29 pass / 0 fail for the whole shell test file; package typecheck passes.
  • A separate verifier reproduced all of it, mutated the fix, and attacked it with its own probes.

Verifier's findings, stated plainly

  • It found no new way to read an outside file.
  • Two specific command spellings that prompt at base no longer prompt at the fix. In both cases the same file can already be read unprompted at base with a simpler spelling, so no capability is added, but a strict reading would call these regressions. They should be closed before promotion.
  • Two mutants survive: backslash-newline handling is correct in probes but has no committed test.
  • The verifier also found gaps in this scan that exist at base, before this PR and before the fix. I am not listing them on this thread; they have gone to the repository owner directly.

Not run: Windows (cmd, PowerShell, Git Bash), the full suite, the V2 bash tool. Cost: 27 worker tool calls plus 29 by the verifier.

@kvnloo

kvnloo commented Oct 10, 2026

Copy link
Copy Markdown
Owner Author

Do not use factory/claude/199-scan-prefix-and-literal-glob @ 239a80d: the independent verifier rejected it.

That branch (one commit on 63d670c) closes the two earlier findings and adds the backslash-newline tests; 33 tests pass. But the verifier found it introduces a new loss of the external-directory prompt for a class of command spellings that prompt at every earlier version, including before this PR. The worker's claim that its probe found no such spelling was wrong.

The verifier identified the cause as one condition that treats an escaped literal character as dynamic syntax, and confirmed a one-line change restores the prompts while keeping the earlier findings closed. Five mutants also survive that branch's tests.

A corrected branch is in progress with a wider probe and a second adversarial verification. Until that reports, the safe state is still: do not promote this PR, and treat 63d670c as the best available but with the two known findings open.

Cost: 17 worker tool calls plus 13 by the verifier.

@kvnloo

kvnloo commented Oct 10, 2026

Copy link
Copy Markdown
Owner Author

Stopping after three rounds: none of the fix branches should be promoted, and neither should this PR. This needs a design decision, not another patch.

round branch head adversarial verdict
1 factory/claude/199-quote-aware-unescape 63d670c fixes the quoted-path regression; two prompt losses found
2 factory/claude/199-scan-prefix-and-literal-glob 239a80d rejected: new prompt loss for a class of escaped characters
3 factory/claude/199-scan-literal-paren 295f3f2afbd01eace38be949a2d032a2bcab434a rejected: fixes round 2's eight cases (44 tests pass, 20 mutants killed by the worker), but a fresh verifier found 9 command forms that prompt before this PR and read an outside file silently at this head, and 8 that prompt at round 1 and are silent here

What three rounds show. Each fix made the scan understand more of bash's quoting, and each time a different verifier found forms where understanding more means resolving further into a path the scan cannot actually follow (brace expansion, variable expansion, glob characters that exist as literal directory names). The older code prompted for some of those only because it gave up early on the raw text. The scan is lexical; bash is not. Patching spelling by spelling will not converge.

Recommendation. Make the scan fail closed: when a path argument contains anything the scan cannot fully resolve, treat it as possibly external and prompt, instead of resolving the part it understands. That trades some extra prompts for no silent reads, and it is a product decision for the maintainers, since it changes how often users are asked.

State to rely on today

  • This PR's head 5029f6d is worse than its parent for quoted paths. Do not promote it. The same change is open upstream from this fork; whether to withdraw or update that upstream PR is the owner's call.
  • The parent of this PR remains the safest of the five versions tested for the forms that regressed, though it has its own long-standing gaps.
  • The round 3 worker's own 176-form probe and the verifier's independent probe are kept locally. I am not listing command forms on this thread; they have gone to the repository owner.

Cost: round 3 used 30 worker tool calls plus 22 by the verifier (295,374 subagent tokens); round 2 used 17 plus 13.

@kvnloo kvnloo added the keep pr-janitor: never auto-close label Oct 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

grok bot Posted by Grok Bot OSS review dump keep pr-janitor: never auto-close needs:title

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant