155 lines
16 KiB
Markdown
155 lines
16 KiB
Markdown
# Approval preview — command/diff on the approval bar
|
||
|
||
## Summary & grounding
|
||
|
||
Today `/hook/permission` (`src/server.ts:423`) reads the hook body, extracts only `tool_name` (`server.ts:430`), derives a `gate` (`server.ts:454`), parks the held `res` in `pendingApprovals` (`server.ts:464`), and broadcasts a bare `waiting` status via `manager.handleHookEvent(sessionId, 'waiting', tool, true, gate)` (`server.ts:471`). The rich `tool_input` — which `parseHookEvent` already knows how to pass through verbatim (`src/http/hook.ts:104-105`) — is dropped on this route. The status `ServerMessage` (`src/types.ts:113-119`) has no field to carry a preview, so the approval bar in `public/tabs.ts:360-377` can only say *"Claude wants to use Bash"*.
|
||
|
||
This feature derives a **bounded, sanitized preview** from `tool_input` server-side, threads it through the same broadcast + late-joiner re-send paths the `gate` already uses, and renders it above the Approve/Reject buttons — reusing the render-only diff renderer (`public/diff.ts:139` `renderDiffFile`) and the `sanitizeField` pattern (`src/session/timeline.ts:50`).
|
||
|
||
Design decision: the derive logic lives in a **new pure module** `src/http/approval-preview.ts` (not inlined in `hook.ts`) so it is unit-testable in isolation and keeps `hook.ts` focused; `hook.ts` is cited only as the proof that `tool_input` is already available on the hook body. The `/hook/permission` route calls it directly (it does not go through `parseHookEvent`).
|
||
|
||
---
|
||
|
||
## Contract
|
||
|
||
### New message field — `src/types.ts` (coordination edit)
|
||
|
||
Add a new exported type and extend the `status` variant of `ServerMessage` (currently `src/types.ts:113-119`). Additive + optional → older clients ignore it; the frontend exhaustiveness check (`terminal-session.ts:349-355`) is unaffected.
|
||
|
||
```ts
|
||
/** A5-adjacent: compact, BOUNDED preview of what a held approval will run.
|
||
* Derived server-side from the hook tool_input, sanitized + byte-capped.
|
||
* Discriminated on `kind`:
|
||
* - 'command' → a shell command string (Bash). Newlines PRESERVED; all other
|
||
* control/ANSI chars stripped. Rendered in a <pre> via textContent.
|
||
* - 'diff' → ONE synthetic DiffFile (Edit/Write/MultiEdit/NotebookEdit),
|
||
* rendered by public/diff.ts renderDiffFile (textContent-only, SEC-H4).
|
||
* `truncated` = the source exceeded the line/byte cap and was clipped. */
|
||
export type ApprovalPreview =
|
||
| { kind: 'command'; text: string; truncated?: boolean }
|
||
| { kind: 'diff'; file: DiffFile; truncated?: boolean };
|
||
```
|
||
|
||
Extend the status variant:
|
||
|
||
```ts
|
||
| {
|
||
type: 'status';
|
||
status: ClaudeStatus;
|
||
detail?: string;
|
||
pending?: boolean;
|
||
gate?: PermissionGate;
|
||
preview?: ApprovalPreview; // NEW — present only on a held (pending) waiting status
|
||
}
|
||
```
|
||
|
||
Extend `SessionManager.handleHookEvent` (currently `src/types.ts:331-339`) with a trailing optional param (appended last so the existing positional `/hook` call at `server.ts:412` is untouched):
|
||
|
||
```ts
|
||
handleHookEvent(
|
||
sessionId: string,
|
||
status: ClaudeStatus,
|
||
detail?: string,
|
||
pending?: boolean,
|
||
gate?: PermissionGate,
|
||
eventClass?: string,
|
||
toolName?: string,
|
||
preview?: ApprovalPreview, // NEW
|
||
): void;
|
||
```
|
||
|
||
### New route behavior
|
||
|
||
No new route. `POST /hook/permission` (`server.ts:423`) gains: derive a preview from `body['tool_input']` + `tool`, store it on the `PendingApproval`, pass it into `handleHookEvent`. `GET`/other routes unchanged.
|
||
|
||
### New env vars — `src/config.ts`
|
||
|
||
**None.** The bounds are security limits, not user knobs (loosening them is a DoS/broadcast-bloat vector), so they are module constants in `approval-preview.ts`, not config. (Documented as a deliberate choice; if a knob is later wanted, `APPROVAL_PREVIEW_BYTES` slots into `Config` next to the existing `previewBytes` field.)
|
||
|
||
### Bounds (module constants in `src/http/approval-preview.ts`)
|
||
|
||
| Constant | Value | Purpose |
|
||
|---|---|---|
|
||
| `PREVIEW_MAX_LINES` | 40 | max diff/command lines emitted |
|
||
| `PREVIEW_MAX_LINE_LEN` | 200 | per-line char cap (= `sanitizeField` default) |
|
||
| `PREVIEW_MAX_BYTES` | 4096 | hard total-byte cap on the serialized preview payload |
|
||
| `EDIT_TOOLS` | `Edit`,`Write`,`MultiEdit`,`NotebookEdit` | reuse the set semantics from `timeline.ts:17` |
|
||
|
||
---
|
||
|
||
## Files to change
|
||
|
||
| Path | Change |
|
||
|---|---|
|
||
| `src/types.ts` | **Coordination edit.** Add `ApprovalPreview` type; add `preview?` to the `status` `ServerMessage` variant (`:113-119`); add trailing `preview?` param to `SessionManager.handleHookEvent` (`:331-339`). |
|
||
| `src/http/approval-preview.ts` | **New file (pure, no DOM, never throws).** `deriveApprovalPreview(toolName: string \| undefined, toolInput: unknown): ApprovalPreview \| null`. Bash→`{kind:'command'}`; Edit/Write/MultiEdit/NotebookEdit→`{kind:'diff', file}`; unknown tool / malformed input→`null`. Imports `sanitizeField` from `../session/timeline.js`; defines a `sanitizeLine` wrapper and per-line splitting so `\n`/`\t` survive but ANSI/control chars don't. |
|
||
| `src/server.ts` | In `/hook/permission` (`:423`): after computing `tool` (`:430`), call `deriveApprovalPreview(tool, body['tool_input'])`. Add `preview?: ApprovalPreview` to the `PendingApproval` interface (`:225-231`); store it at creation (`:464`). Pass `preview` into `handleHookEvent(...)` (`:471`). In the late-joiner re-send (`:858-864`) include `preview: heldApproval.preview`. Import `deriveApprovalPreview` + `ApprovalPreview`. |
|
||
| `src/session/manager.ts` | `handleHookEvent` (`:223-249`): accept trailing `preview?` param; set `msg.preview = preview` when defined, before `broadcast(session, msg)` (`:245-249`). (The Case-2 late-join re-send at `:142` stays bare — the server layer owns pending/preview re-send, per the comment at `:137-138`.) |
|
||
| `public/terminal-session.ts` | Add `private pendingPreviewValue: ApprovalPreview \| null = null` near `:98-106`; getter `get pendingPreview()` near `:185-207`; in `case 'status'` (`:313-322`) set `this.pendingPreviewValue = nextPending ? (msg.preview ?? null) : null`; clear it in the two disconnect resets near `:385-389`. Import `ApprovalPreview` from `../src/types.js`. |
|
||
| `public/tabs.ts` | In `updateApprovalBar` (`:360-377`): after the `label`, if `session.pendingPreview` is set, build and insert a preview node **before** the buttons. Add `private renderApprovalPreview(p: ApprovalPreview): HTMLElement` — `kind:'command'`→`<pre class="approval-cmd">` via `textContent`; `kind:'diff'`→`renderDiffFile(p.file)` wrapped in a scroll container; append a "… truncated" note when `p.truncated`. Import `renderDiffFile` from `./diff.js` and `ApprovalPreview` from `../src/types.js`. |
|
||
| `public/style.css` | Add `.approval-preview` (scroll container: `max-height`, `overflow:auto`, `overflow-x:auto`), `.approval-cmd` (`white-space:pre-wrap`, monospace), `.approval-truncated` under the existing `#approvalbar` block (`:1084`). Reuse existing `.df-*` styling for the diff. |
|
||
| `test/http/approval-preview.test.ts` | **New (node).** Unit tests for `deriveApprovalPreview`. |
|
||
| `test/manager.test.ts` | Extend: `handleHookEvent` with a `preview` arg puts it on the broadcast status msg. |
|
||
| `test/terminal-session.test.ts` | Extend (jsdom): status frame with `preview` sets `pendingPreview`; cleared when `pending` false / on disconnect. |
|
||
| `test/tabs.test.ts` | Extend (jsdom): `FakeTerminalSession` gains `pendingPreview`; `updateApprovalBar` renders command / diff / truncated note; no-preview → name-only bar (regression). |
|
||
| `test/integration/server.test.ts` | Extend: `POST /hook/permission` with `tool_input` → broadcast `waiting` status carries `preview`; a late-joining WS gets the preview on attach. |
|
||
|
||
---
|
||
|
||
## TDD steps (ordered)
|
||
|
||
Repo style: pure/back-end tests are plain vitest (node) — see `test/hook.test.ts` (`import { describe, it, expect }`, `expect.objectContaining`). Front-end tests carry `// @vitest-environment jsdom` and mock `TerminalSession` (`test/tabs.test.ts`) or mock `WebSocket` + stub xterm (`test/terminal-session.test.ts`). Diff render tests run in jsdom asserting `textContent` (`test/diff.test.ts`).
|
||
|
||
1. **RED — `test/http/approval-preview.test.ts` (node).** Write, before any impl:
|
||
- Bash: `deriveApprovalPreview('Bash', { command: 'ls -la', description: 'x' })` → `{ kind:'command', text:'ls -la' }`.
|
||
- Bash multi-line: `command:'a\nb'` → `text` still contains the `\n` (newlines preserved).
|
||
- Bash with ANSI/control injection: `command:'\x1b[31mrm\x1b[0m\x07'` → ESC/BEL stripped, no `\x1b`/`\x07` in `text`.
|
||
- Edit: `{ file_path:'/p/f.ts', old_string:'a\nb', new_string:'c' }` → `{ kind:'diff', file }` where `file.newPath` sanitized, hunk has `removed` lines `a`,`b` and `added` line `c`, `file.removed===2`, `file.added===1`.
|
||
- Write: `{ file_path, content:'x\ny' }` → all-added hunk, `removed===0`.
|
||
- MultiEdit: `{ file_path, edits:[{old_string,new_string},{...}] }` → one hunk per edit.
|
||
- Truncation: `content` with >`PREVIEW_MAX_LINES` lines → `truncated:true`, ≤ cap lines emitted; a >`PREVIEW_MAX_BYTES` blob → `truncated:true` and serialized size ≤ cap.
|
||
- Unknown tool (`'WebFetch'`) → `null`; missing/`null`/array/number `toolInput` → `null` and **never throws** (mirrors `hook.ts` SEC-M7 style).
|
||
2. **GREEN — implement `src/http/approval-preview.ts`.** Pure, `unknown`-narrowed, `sanitizeField`-per-line, byte-clamped. Run the suite to green.
|
||
3. **RED — `test/manager.test.ts`.** Add: calling `handleHookEvent(id,'waiting','Bash',true,'tool',undefined,undefined,{kind:'command',text:'ls'})` broadcasts a status msg whose `preview` deep-equals the arg (assert via the fake WS `send` capture already used in this file). Add: omitting `preview` → no `preview` key on the msg.
|
||
4. **GREEN — `src/types.ts` param + `src/session/manager.ts`.** Add the trailing param and `msg.preview` assignment.
|
||
5. **RED — `test/integration/server.test.ts`.** Add a test (follow the `itPty` pattern of ⑧ at `:608-655`): attach a WS, `POST /hook/permission` with `{ tool_name:'Bash', tool_input:{ command:'echo hi' } }`, assert the broadcast `pending===true` status has `preview.kind==='command'` and `preview.text` containing `echo hi`. Add a **late-joiner** assertion: open a 2nd WS to the same `sessionId` while held → its first `waiting/pending` status includes `preview`.
|
||
6. **GREEN — `src/server.ts`.** Wire `deriveApprovalPreview` into `/hook/permission`, store on `PendingApproval`, pass to `handleHookEvent`, include in the `:858-864` re-send.
|
||
7. **RED — `test/terminal-session.test.ts` (jsdom).** Feed a `status` frame with `pending:true, gate:'tool', preview:{kind:'command',text:'ls'}` → `session.pendingPreview` set; a follow-up `status` with `pending:false` clears it to `null`; disconnect clears it.
|
||
8. **GREEN — `public/terminal-session.ts`.** Add field, getter, set/clear.
|
||
9. **RED — `test/tabs.test.ts` (jsdom).** Extend `FakeTerminalSession` with `pendingPreview`. Assert `updateApprovalBar`: command preview → a `.approval-cmd` node whose `textContent` equals the command; diff preview → a `.df-file` present (renderDiffFile output); `truncated:true` → `.approval-truncated` present; `pendingPreview:null` → bar shows label + buttons only (regression, matches `:373-374`). Assert **zero `innerHTML`** — content via `textContent` only.
|
||
10. **GREEN — `public/tabs.ts` + `public/style.css`.** Add `renderApprovalPreview`, insert before buttons, style the container.
|
||
11. **Coverage gate.** The new pure module is branch-dense and fully unit-covered (helps the 80% gate); FE branches covered by jsdom tests. Run full `npm test` + coverage; confirm no regression in `hook.test.ts`/`diff.test.ts`/`manager.test.ts`.
|
||
|
||
---
|
||
|
||
## Edge cases & failure modes
|
||
|
||
- **Unknown / non-preview tool** (WebSearch, Task, MCP tools, `ExitPlanMode`): `deriveApprovalPreview` returns `null` → status carries no `preview` → `updateApprovalBar` falls back to today's name-only bar (`:373`). Plan-gate (`gate:'plan'`, `:369-371`) also gets no preview (ExitPlanMode has no reviewable command/diff) — unchanged.
|
||
- **Missing/partial `tool_input`**: Bash without `command`, Edit without `old_string`, `tool_input` present but `null` (the `'tool_input' in b` passthrough at `hook.ts:104` can yield `null`) → return `null`, never throw.
|
||
- **Huge command / whole-file Write**: clipped at `PREVIEW_MAX_LINES` then `PREVIEW_MAX_BYTES`; `truncated:true` shows the "… truncated" note. Prevents a multi-MB status frame from being broadcast to every client and retained in `pendingApprovals`.
|
||
- **Newlines vs control chars**: `sanitizeField` (`timeline.ts:50`) strips `\x00-\x1f` which includes `\n`/`\t`/`\r` — so it is applied **per line after splitting**, never to the whole multi-line blob, preserving structure while still killing ANSI ESC (`\x1b`) and BEL.
|
||
- **MultiEdit with many edits**: hunks accumulate until the line/byte cap, then stop + `truncated:true`.
|
||
- **Late joiner after approval already resolved**: `pendingApprovals.get()` returns `undefined` (`server.ts:858`) → no preview re-sent (correct; nothing is held).
|
||
- **Approve/reject clears preview**: `handleHookEvent(...,'working',...,false)` (`server.ts:887,890`) broadcasts a non-pending status → `terminal-session` sets `pendingPreview = null` → bar hides (existing `:362` guard).
|
||
- **Multi-device**: preview broadcasts to all clients via `broadcast` and re-sends to each new attach — every mirror shows the same preview.
|
||
- **Binary/odd content in Write**: rendered as literal text via `textContent` (no interpretation), `binary:false` on the synthetic file (we don't attempt binary detection — out of scope).
|
||
|
||
---
|
||
|
||
## Security
|
||
|
||
- **Trust boundary**: `tool_input` is Claude-controlled content arriving on the loopback-only `/hook/permission` route (`isLoopback` guard at `server.ts:424`). It is untrusted for *content*: treat as `unknown`, narrow every field (`typeof x === 'string'`), never index without a guard. `deriveApprovalPreview` must **never throw** (SEC-M7 discipline, mirroring `parseHookEvent`).
|
||
- **Sanitization (SEC-H6 reuse)**: every emitted string passes through `sanitizeField`/`sanitizeLine` — strips `\x00-\x1f` (incl. ANSI ESC `\x1b`), truncates to `PREVIEW_MAX_LINE_LEN`. This neutralizes terminal-escape / cursor-hijack payloads in a filename or command before they reach the DOM.
|
||
- **XSS (SEC-H4 reuse)**: the frontend renders **only** via `textContent` / `el()` / `renderDiffFile` (which is already innerHTML-free — `diff.ts:9`, `:191-194`). No `innerHTML` anywhere in the new FE code; asserted in tests. `<script>`, `&`, `<img onerror>` appear as literal characters.
|
||
- **DoS / resource containment**: `PREVIEW_MAX_LINES` + `PREVIEW_MAX_BYTES` cap the payload that is (a) broadcast to N clients and (b) retained in `pendingApprovals` for the held-decision lifetime. No unbounded growth from a hostile/huge `tool_input`.
|
||
- **No new route, no new capability token**: the existing per-decision token (`server.ts:457`), Origin/loopback guards, and rate limiters are untouched. Preview is pure display data attached to an already-authorized held decision.
|
||
- **No path egress / no argv**: file paths from `tool_input` are only sanitized + displayed as text; they are never passed to `execFile`, `fs`, or a shell. No path-traversal surface is added.
|
||
|
||
---
|
||
|
||
## Effort & dependencies
|
||
|
||
- **Effort**: ~1.5–2 days. Breakdown: pure `approval-preview.ts` + its tests ~0.5d (the bulk of logic + coverage); server/manager/types threading ~0.25d; FE (terminal-session field + tabs render + CSS) + jsdom tests ~0.5d; integration test + polish ~0.25d.
|
||
- **Depends on** (all already shipped): H3 held-approval gate + `pendingApprovals` (`server.ts:423-472`); B4 `gate` re-send plumbing to late joiners (`server.ts:858-864`, `manager.ts:137-142`) — this feature rides the exact same rails; B1 `public/diff.ts` `renderDiffFile` + the `DiffFile`/`DiffLine` types (reused, not modified); A4 `sanitizeField` (`timeline.ts:50`, reused).
|
||
- **Unlocks / synergizes**: the diff-render path here is the same one W3 "Diff against a base branch" and W4 "Stage/commit/push from the diff viewer" extend — a shared, security-reviewed `DiffFile` render surface. Also complements A1 lock-screen approvals: a future enhancement can put a one-line preview summary into the push `detail` (`PushPayload.detail`, `types.ts:381`) so the phone shows *what* is being approved (not planned here, but the derive function is the reusable source).
|
||
- **Isolation**: `src/http/approval-preview.ts` is a new owned file (no conflict); `src/types.ts` is the one coordination edit (additive-optional, low collision risk); `server.ts`/`manager.ts`/`terminal-session.ts`/`tabs.ts` edits are localized to the cited line ranges. |