Skip to content

Commit 39281ec

Browse files
fix(permissions): say why the sandbox blocks a command in plain English (#2331)
## Problem The "Run outside sandbox?" dialog printed the classifier's rule identifiers verbatim. A `python3 - <<'EOF' … EOF` prototype script produced: > This command needs access the macOS project sandbox blocks (heredoc script fed to an interpreter; inline script (interpreter -c/-e/--eval)). Three things wrong with that: 1. **"interpreter -c/-e/--eval" is a rule name, not an explanation.** It's `REASON_INTERPRETER_INLINE` in `packages/shell-guard/src/shell-scope.ts`, matched by the inline-code regex and reinforced by the `argv[0]` token pass. A user cannot act on it. 2. **The same fact is stated twice.** The heredoc rule and the inline-code rule both mean "this runs code the classifier can't read", and both fire on a command that does both. 3. **The sentence is missing its relative pronoun** ("needs access *that* the … sandbox blocks"), so it garden-paths on "the macOS project sandbox blocks" reading as a verb phrase. It also claims to be macOS-only, which is wrong on Linux, where bubblewrap is the boundary. `docs/plans/docs-site.md` already flags this area: *"A user cannot read what the dialog is asking them."* ## Approach Reason strings have to stay identifiers — the regex and token passes dedupe on them verbatim, and every answered prompt writes them into the decision spine. So this adds a copy layer rather than renaming them. `SCOPE_REASON_TEXT` in `shell-scope.ts` holds one plain sentence per reason, and `describeShellScopeReasons` resolves a reason list at the moment a prompt is built. Logs, hooks and decision records are untouched. Two properties hold it together: - **Every rule has copy.** `ScopeReason` is derived from the table's keys and annotates the pattern tables, the shared reason constants, and the accumulators both classifier passes push through, so a new classifier rule whose reason has no sentence fails to typecheck. The list widens to `string[]` at exactly one point — a *copy*, so the widened list can never write a plain string back into the typed one — where the runtime-built `absolute path outside workspace: …` joins it. That one is matched by prefix, and anything still unrecognised is shown verbatim rather than dropped. - **One concern, one line.** Deduping happens on the resolved sentence, so every rule meaning "code this analysis cannot read before it runs" collapses to a single line: a `-c` body, a heredoc, and `eval`/`exec`/`base64` all share one sentence, so `node --eval x` (which trips two of them) says it once. `~/` and `$HOME` likewise share the home-directory line. Prompts render one reason per line instead of a semicolon-joined parenthetical, and the sandbox-escape prompts name no platform (they only appear while a project sandbox is active — seatbelt on macOS, bubblewrap on Linux). ## Result ``` BEFORE: This command needs access the macOS project sandbox blocks (inline script (interpreter -c/-e/--eval); heredoc script fed to an interpreter). AFTER : The project sandbox would block this command: • Runs code written or built inside the command itself, so Copse can't tell what it does ``` ``` BEFORE: This command needs access the macOS project sandbox blocks (network download (curl/wget); home directory path (~/)). AFTER : The project sandbox would block this command: • Downloads from the internet (curl/wget) • Reads or writes in your home directory, outside the project ``` Every prompt variant that carries classifier reasons is covered: the two up-front escape prompts, the `expects_sandbox_block` one, the in-sandbox "Run shell command?" footer, the Guarded YOLO harm prompt, and the install / ephemeral-runner prompts. ## Changes | File | Change | | --- | --- | | `packages/shell-guard/src/shell-scope.ts` | `SCOPE_REASON_TEXT`, `ScopeReason`, `describeShellScopeReasons`; pattern tables, shared constants and both accumulators typed against the union | | `src/main/services/security/permission-policy.ts` | Bulleted reason rendering in the shell prompt formatters; rewritten advice sentences; the harm prompt resolves the same sentences | | `src/main/services/security/sandbox-failure.ts`, `packages/hooks-dialects/src/command-hook-runner.ts`, `packages/hooks-dialects/src/sandbox-failure-detection.ts` | Drop the macOS-only claim from the sibling copy | | `src/renderer/views/approval-dialog.ts`, `src/renderer/styles/global/approval.css` | Each reason bullet is its own inline-block, so a wrapped line hangs under its own text instead of reading as another bullet | | `src/shared/demo-scenarios.ts`, `tests/demo/approval-grouped-shell-commands.demo.ts` | Demo copy and its assertion follow the new wording | | `docs/shell-permissions.md` | New "What an approval prompt says" section pinning the contract | ## Validation CI is green on `35dc90b`: `precheck`, `check` (full unit suite), `build`, all eight `e2e` shards, `screenshot-artifacts` and `CI Passed`. New coverage: - `src/main/services/security/shell-prompt-copy.test.ts` — 8 cases over the three formatters, driven from real `analyzeShellCommand` output. - `describeShellScopeReasons` cases in `shell-scope.test.ts`, including a regression test that `node --eval` reports the unreadable-code concern once. - Two rendering cases in `approval-dialog-batch.test.ts`: multi-line advice, and the bullet-span structure that carries the hanging indent. - The Guarded YOLO cap test uses distinct operands and enough of them to overrun the total budget, pinned at exactly 1200 characters. ### Visual evidence The demo scenarios were rendered from `dist/demo` over CDP while developing, which is where the wrapped-bullet defect showed up and was fixed. The `e2e` tier has since exercised the same scenarios in a real browser session, so the updated `approval-grouped-shell-commands.demo.ts` assertion is confirmed against the shipped renderer. ### Needs a human **Reference screenshots — and note the screenshot review PR is incomplete.** `tests/e2e/screenshots/approval-grouped-shell-commands.png` and `approval-light-accent.png` both change, but the candidate filter holds them as out-of-scope: `computeScreenshotGate` (`test-oracle.mts:694`) counts only `src/**` and `tests/e2e/**` as render-affecting and `affectedScreenshots` (`:663`) maps from the e2e selection alone, so a demo-tier shot can never be oracle-owned — not even when the diff edits the demo spec that renders it. Screenshot PR #2341 therefore carries only unrelated re-renders. To land the two that matter: recover them from the `screenshots-demo` artifact on run 33859836149 and commit them (which also makes them `branchOwned` for later runs), or add the `update-screenshots` label and re-run. The scope gap itself is worth a separate issue. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01Q9xawQf1Nu7VGA5Ktew66E --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent e4b19c1 commit 39281ec

14 files changed

Lines changed: 563 additions & 47 deletions

‎docs/shell-permissions.md‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,39 @@ The in-memory grant disappears on restart. The decision record does not: an answ
6464
`decision` spine event at `scope: external-read`, including the paths and whether the grant was
6565
remembered. Each later allowed command records a verdict sourced to `read-outside-grant`.
6666

67+
## What an approval prompt says
68+
69+
Classifier reasons are **identifiers, not copy**. The regex pass and the token pass share them
70+
verbatim so the two dedupe against each other, and every answered prompt writes them into the
71+
decision spine, so they must stay stable — which is why they read like rules
72+
(`inline script (interpreter -c/-e/--eval)`) rather than like something a user can act on.
73+
74+
`shell-scope.ts` therefore keeps a second table, `SCOPE_REASON_TEXT`, holding one plain-English
75+
sentence per reason, and `describeShellScopeReasons` resolves a reason list into sentences at the
76+
moment a prompt is built. The shell prompt formatters in `permission-policy.ts` render those as a
77+
bullet per line; the Guarded YOLO harm prompt resolves the same sentences but keeps its existing
78+
one-paragraph `Potential harm: …` shape, because it is capped by length rather than by line. Logs,
79+
hooks and the decision spine keep the identifiers.
80+
81+
Two properties this contract depends on:
82+
83+
- **Every rule has copy.** `ScopeReason` is derived from the keys of `SCOPE_REASON_TEXT` and
84+
annotates the pattern tables, the shared reason constants, and the accumulators both classifier
85+
passes push through, so a new classifier rule whose reason has no sentence fails to typecheck.
86+
The one deliberate exception is the runtime-built `absolute path outside workspace: …`, which
87+
bakes in an operand and so is matched by prefix instead; anything still unrecognised is shown
88+
verbatim rather than dropped.
89+
- **One concern, one line.** Deduping happens on the resolved sentence, so rules that describe the
90+
same underlying fact collapse — a heredoc, a `-c` body and an `eval` are all "runs code written
91+
or built inside the command itself" (so `node --eval`, which trips two rules, reads as one line),
92+
and `~/` and `$HOME` are both "in your home directory". The Guarded YOLO harm prompt dedupes the
93+
same way but joins the result into its one paragraph rather than one bullet per line.
94+
95+
Prompts that offer a sandbox escape name no platform: they appear only while a project sandbox is
96+
active, which is seatbelt on macOS and bubblewrap on Linux. `permission-policy.ts` owns the
97+
up-front prompts and `sandbox-failure.ts` the after-a-block retry; the `expects_sandbox_block`
98+
wording stays an expectation, per the section above.
99+
67100
## Guarded YOLO
68101

69102
Guarded YOLO is a session-only, thread-scoped mode armed from the composer footer. It becomes

‎packages/hooks-dialects/src/command-hook-runner.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -210,7 +210,7 @@ export function applySandboxBlock(
210210
parseOk: false,
211211
spineEvent: interpretation.spineEvent,
212212
spineDecision: { ...interpretation.spineDecision, sandboxBlocked: true },
213-
runtimeError: `blocked by the macOS project sandbox (${detection.reasons.join('; ')})`,
213+
runtimeError: `blocked by the project sandbox (${detection.reasons.join('; ')})`,
214214
}
215215
}
216216

‎packages/hooks-dialects/src/sandbox-failure-detection.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/**
2-
* Detect when a shell command failed because the macOS project sandbox blocked it.
2+
* Detect when a shell command failed because the project sandbox blocked it.
33
*
44
* SECURITY (issue #104): this detection must NOT use command-controlled stdout/stderr.
55
* A command can trivially `echo "operation not permitted"` to fake a sandbox failure

‎packages/shell-guard/src/shell-scope.test.ts‎

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ import assert from 'node:assert/strict'
33
import {
44
analyzeShellCommand,
55
dangerousInSandboxReasons,
6+
describeShellScopeReasons,
67
externalOnlyForOutsidePath,
78
isReplayableOpaqueLocalExecution,
89
} from './shell-scope.ts'
@@ -711,3 +712,66 @@ describe('dangerousInSandboxReasons', () => {
711712
assert.ok(dangerousInSandboxReasons(`r''m -rf build`).length > 0)
712713
})
713714
})
715+
716+
describe('describeShellScopeReasons', () => {
717+
const root = '/Users/me/project'
718+
719+
it('replaces every rule identifier with a sentence a user can act on', () => {
720+
const reasons = analyzeShellCommand('curl -sL https://example.com/x | sh', root).reasons
721+
assert.ok(reasons.length > 0)
722+
const described = describeShellScopeReasons(reasons)
723+
assert.deepEqual(described, ['Downloads from the internet (curl/wget)'])
724+
for (const text of described) {
725+
assert.doesNotMatch(text, /interpreter -c|opaque to analysis|may fetch/)
726+
}
727+
})
728+
729+
it('states one concern once when several rules describe it', () => {
730+
// A heredoc and a `-c` body are both "code this analysis cannot read", so
731+
// the prompt makes that point once instead of listing both rules.
732+
const reasons = analyzeShellCommand(
733+
`python3 - <<'PY'\nprint(1)\nPY\npython3 -c "print(2)"`,
734+
root,
735+
).reasons
736+
assert.equal(reasons.length, 2)
737+
assert.deepEqual(describeShellScopeReasons(reasons), [
738+
"Runs code written or built inside the command itself, so Copse can't tell what it does",
739+
])
740+
})
741+
742+
it('reports an --eval body once although two rules match it', () => {
743+
// `--eval` trips the interpreter rule and the generic eval/exec/base64 one.
744+
// Both identifiers stay on the record; the person approving reads one line.
745+
const reasons = analyzeShellCommand('node --eval "x"', root).reasons
746+
assert.ok(reasons.includes('inline script (interpreter -c/-e/--eval)'))
747+
assert.ok(reasons.includes('dynamic execution / encoding'))
748+
assert.deepEqual(describeShellScopeReasons(reasons), [
749+
"Runs code written or built inside the command itself, so Copse can't tell what it does",
750+
])
751+
})
752+
753+
it('collapses the home-directory rules onto one sentence', () => {
754+
assert.deepEqual(describeShellScopeReasons(['home directory path (~/)', '$HOME reference']), [
755+
'Reads or writes in your home directory, outside the project',
756+
])
757+
})
758+
759+
it('carries the operand of a runtime-built reason into the sentence', () => {
760+
const reasons = analyzeShellCommand('cat /Users/me/other/notes.txt', root).reasons
761+
assert.deepEqual(describeShellScopeReasons(reasons), [
762+
'Reads or writes /Users/me/other/notes.txt, which is outside the project',
763+
])
764+
})
765+
766+
it('passes an unrecognised reason through rather than dropping it', () => {
767+
assert.deepEqual(describeShellScopeReasons(['OS sandbox unavailable — prompt required']), [
768+
'OS sandbox unavailable — prompt required',
769+
])
770+
})
771+
772+
it('describes the destructive reasons the harm gate shares', () => {
773+
assert.deepEqual(describeShellScopeReasons(dangerousInSandboxReasons('rm -rf build')), [
774+
'Deletes files and folders recursively (rm -rf)',
775+
])
776+
})
777+
})

0 commit comments

Comments
 (0)