Skip to content

fix(shell): close external_directory gaps in the command path scan - #40153

Open
0xSemizzz wants to merge 1 commit into
anomalyco:devfrom
0xSemizzz:fix/shell-external-directory-coverage
Open

fix(shell): close external_directory gaps in the command path scan#40153
0xSemizzz wants to merge 1 commit into
anomalyco:devfrom
0xSemizzz:fix/shell-external-directory-coverage

Conversation

@0xSemizzz

@0xSemizzz 0xSemizzz commented Aug 2, 2026

Copy link
Copy Markdown

The docs say external_directory fires whenever a tool touches paths outside the project directory. The file tools do that for every call. The shell tool only did it for a short allowlist of command names, and threw away any argument containing a $.

So tee ~/.ssh/authorized_keys never prompted, because tee was not scanned. And rm $HOME/.ssh/id_rcause although rmis scanned,dynamic()` discarded the argument before it could be resolved.

Add the common readers and writers that take their target as a plain positional argument, and expand hells the way expand()` already does for PowerShell.

Issue for this PR

Closes #40159

Type of change

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

What does this PR do?

Two changes in tool/shell.ts.

FILES gains the file commands that were missing: tee, dd, ln, rsync, shred, split, truncate, install, unlink on the write side, and head, tail, sed, sort, wc, diff, cmp, less, more on the read
side. Only commands whose target is a plain positional argument, since that is what pathArgs() can aready inside the project resolves and is skipped as before, so this only prompts for paths thatgenuinely leave the project.

argPath() now runs POSIX arguments through a new posixExpand() that substitutes $HOME and $PWD (both $VAR and ${VAR} forms) before dynamic() sees them. Only those two, because they are the only ones resolvable without running the shell. $OTHER, $(cmd), and backticks still fall through dynamic() and get skipped exactly as they do today. $HOMEBREW deliberately does not match, because the pattern requires a / or end of string after the name.

Redirection targets such as echo x > /etc/foo are still not scanned, because parts() skips redirection nodes and handling them means walking redirected_statement children. That is a bigger change and worth a follow-up.

How did you verify your code works?

Two tests in packages/opencode/test/tool/shell.test.ts, POSIX only. echo hi | tee /etc/... expects an external_directory request for /etc/*, and rm $HOME/.ssh/... expects one for <home>/.ssh/*. Both stop at the permissioncall so nothing runs.

I also ran posixExpand against $HOME/x, ${HOME}/x, "$HOME/x", $PWD/../x, $HOMEBREW/x, $OTHER/x, $(whoami)/x, ~/x, and a plain relative path, to confirm the first four expand and the rest stay dynamic.

cd packages/opencode && bun test test/tool/shell.test.ts

Screenshots / recordings

N/A, no UI change.

Checklist

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

`external_directory` is documented as firing whenever a tool touches paths
outside the project directory, and the file tools honour that for every call.
The shell tool only honoured it for a small allowlist of command names, and
dropped any argument containing a `$`.

Two gaps followed:

  tee ~/.ssh/authorized_keys   -> `tee` was not scanned, so no prompt
  rm $HOME/.ssh/id_rsa         -> `rm` is scanned, but `dynamic()` discarded
                                  the argument before it could be resolved

Add the common file readers and writers that take their target as a plain
positional argument, and expand `$HOME`/`$PWD` for POSIX shells the way
`expand()` already does for PowerShell. Anything still unresolvable after
expansion continues to fall through `dynamic()` untouched.

Redirection targets (`echo x > /etc/foo`) are still not scanned; `parts()`
skips redirection nodes. Left for a follow-up.
@github-actions github-actions Bot added needs:compliance This means the issue will auto-close after 2 hours. needs:issue labels Aug 2, 2026
@github-actions

github-actions Bot commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

Thanks for your contribution!

This PR doesn't have a linked issue. All PRs must reference an existing issue.

Please:

  1. Open an issue describing the bug/feature (if one doesn't exist)
  2. Add Fixes #<number> or Closes #<number> to this PR description

See CONTRIBUTING.md for details.

@github-actions github-actions Bot removed needs:compliance This means the issue will auto-close after 2 hours. needs:issue labels Aug 2, 2026
@github-actions

github-actions Bot commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

Thanks for updating your PR! It now meets our contributing guidelines. 👍

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.

external_directory does not fire for many bash commands that write outside the project

1 participant