docs(plans): implementation-ready plans for roadmap Wave 1-4 features (8, parallel-generated)
This commit is contained in:
Yaojia Wang
155
docs/plans/w1-approval-preview.md
Normal file
155
docs/plans/w1-approval-preview.md
Normal file
@@ -0,0 +1,155 @@
|
||||
# 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.
|
||||
143
docs/plans/w1-clickable-links.md
Normal file
143
docs/plans/w1-clickable-links.md
Normal file
@@ -0,0 +1,143 @@
|
||||
# Clickable URLs & file paths in the terminal
|
||||
|
||||
**Status of premise (read first).** Two independent capabilities are bundled here:
|
||||
|
||||
1. **URLs** — already *90% shipped.* `public/terminal-session.ts:148` already does `this.term.loadAddon(new WebLinksAddon())`, and `@xterm/addon-web-links@^0.12.0` is already in `package.json`. Remaining work is a security hardening pass on link activation (scheme allowlist + `noopener`). Pure frontend.
|
||||
|
||||
2. **File paths → editor** — needs a **custom link provider** *and* a **small, additive server change.** The task brief says "POST to the existing `/open-in-editor` … no server change," but the existing route **cannot** open a file at a line: `openInEditor` (`src/http/editor.ts:31`) rejects anything that is not an **absolute existing _directory_** (lines 35, 45–47) and spawns `code <dir>` with **no `--goto`/line** (line 51). Feeding it `src/app.ts:42` fails three validators at once. See the **Decision** callout below — I recommend a tiny additive `openFileInEditor` alongside the untouched `openInEditor`. A strict "frontend-only" fallback exists but degrades to "open the containing folder, no line jump."
|
||||
|
||||
`src/types.ts` is **not** touched — this is an HTTP JSON body, not a WS protocol message; no shared-contract change. The byte-shuttle terminal stream is untouched.
|
||||
|
||||
---
|
||||
|
||||
## Decision: how file-path clicks reach the editor
|
||||
|
||||
| Option | What clicking `src/app.ts:42` does | Server change | Recommendation |
|
||||
|---|---|---|---|
|
||||
| **A — additive `openFileInEditor` (recommended)** | Opens the file at line 42 (`code --goto /abs/src/app.ts:42`) | +~35 lines in `src/http/editor.ts`, +1 branch in the `server.ts:384` route. Backward-compatible; `openInEditor` + its tests untouched | **Yes.** Only option that delivers the actual feature (jump to file:line). Additive and low-risk. |
|
||||
| **B — strict frontend-only** | Resolves to the file's parent dir and calls existing `openInEditor` → opens the *repo/folder*, no file, no line | None | Fallback only. Poor UX; loses the whole point (line jump). Document but don't ship as primary. |
|
||||
|
||||
The rest of this plan assumes **Option A**. The frontend work is identical either way; only the POST body and server branch differ.
|
||||
|
||||
---
|
||||
|
||||
## Contract
|
||||
|
||||
### Route (extended, backward-compatible)
|
||||
`POST /open-in-editor` (`src/server.ts:384`) — unchanged guards: `express.json({ limit: '4kb' })`, `requireAllowedOrigin` (CSRF, `src/server.ts:352`). New branch on body shape:
|
||||
|
||||
- **Existing (directory)** — `{ "path": "<abs dir>" }` → `openInEditor(cfg, body.path)` (unchanged; Projects panel keeps working).
|
||||
- **New (file+line)** — `{ "file": "<abs file>", "line": <int?>, "column": <int?> }` → `openFileInEditor(cfg, body.file, body.line)`.
|
||||
- Response envelope unchanged: `204` on success; `{ error }` + `4xx/5xx` on failure (mirrors `src/server.ts:388–392`).
|
||||
|
||||
Body is a discriminated request: **`file` present ⇒ file mode; else path mode.** If neither present → `400 { error: 'path or file is required' }`.
|
||||
|
||||
### New server function (`src/http/editor.ts`)
|
||||
```
|
||||
export async function openFileInEditor(
|
||||
cfg: Config, rawFile: unknown, rawLine?: unknown
|
||||
): Promise<OpenEditorResult>
|
||||
```
|
||||
Reuses the existing `OpenEditorResult` type (`editor.ts:20`). Validates: string + non-empty (`400`), `path.isAbsolute` (`400`), `fs.stat` exists (`404`), `stat.isFile()` (`400 'path is not a file'`), and `line` (when present) is an integer in `1..1_000_000` else `400`. Spawns via `execFile(cfg.editorCmd, args)` (no shell, same as line 51) where:
|
||||
- `args = isGotoEditor(cfg.editorCmd) && line !== undefined ? ['--goto', `${file}:${line}`] : [file]`
|
||||
- `isGotoEditor` = basename ∈ `{code, code-insiders, codium, cursor, windsurf}` (the editors that accept `--goto`). Unknown editors open the bare file (never pass a bogus `--goto` argv).
|
||||
|
||||
### New frontend module (`public/link-paths.ts`) — pure, node-testable
|
||||
```
|
||||
export interface PathMatch {
|
||||
text: string // exact matched substring (e.g. "src/app.ts:42")
|
||||
path: string // "src/app.ts"
|
||||
line?: number
|
||||
column?: number
|
||||
startX: number // 1-based column of first char (xterm range.start.x)
|
||||
endX: number // 1-based column of last char (xterm range.end.x, inclusive)
|
||||
}
|
||||
export function findPathMatches(lineText: string): PathMatch[]
|
||||
```
|
||||
Matching rules (concrete): a token is a path candidate iff it has a filename with a dot-extension, optionally preceded by `./`, `../`, or `dir/…/` segments, optionally suffixed `:line` and `:line:col`. A candidate becomes a match iff **(has a `/` separator) OR (has a `:line` suffix) OR (extension ∈ `CODE_EXT` allowlist)** — this links `src/app.ts:42`, `README.md`, `main.rs:10` while rejecting `example.com`, `v1.2`, `foo.bar`. Reject candidates immediately preceded by `/` or `:` (avoids grabbing the tail of a `https://host/path.html` URL, which `WebLinksAddon` owns). `CODE_EXT` = a named const set (`ts tsx js jsx mjs cjs py go rs rb java kt c h cpp hpp cc cs php swift css scss html json yaml yml toml md txt sh sql vue svelte` …).
|
||||
|
||||
### Env vars
|
||||
**None new.** `EDITOR_CMD` (default `'code'`) already exists (`src/config.ts:49`, `src/types.ts:42`) and is reused.
|
||||
|
||||
### WS protocol / `src/types.ts`
|
||||
**No change.** No new client→server or server→client message types.
|
||||
|
||||
---
|
||||
|
||||
## Files to change
|
||||
|
||||
| Path | Change |
|
||||
|---|---|
|
||||
| `public/link-paths.ts` | **New.** Pure `findPathMatches` + `CODE_EXT` const + `PathMatch` type. No DOM. (Keeps matcher unit-testable in node and file <150 lines.) |
|
||||
| `public/terminal-session.ts` | Replace bare `new WebLinksAddon()` at **line 148** with a hardened handler (scheme allowlist + `window.open(uri,'_blank','noopener,noreferrer')`). After `term.open` (line 149) register a path link provider via `this.term.registerLinkProvider(...)`; store the returned `IDisposable`. Add `private openPath(m: PathMatch)` (resolve rel→abs via `this.cwdValue`, in-flight guard, `fetch('/open-in-editor', …)`, error→`statusLine` toast). Dispose the provider in `dispose()` (line 452). Small helpers `openWebLink`, `makePathLinkProvider`. |
|
||||
| `src/http/editor.ts` | **Add** `openFileInEditor` + `isGotoEditor` helper. **`openInEditor` unchanged** (Projects panel + its tests keep passing). |
|
||||
| `src/server.ts` | In the `/open-in-editor` handler (**line 384–393**) branch: `body.file` present → `openFileInEditor(cfg, body.file, body.line)`; else existing `openInEditor(cfg, body.path)`. |
|
||||
| `src/types.ts` | **Not touched** — noted here only to confirm no shared-contract edit is required. |
|
||||
|
||||
---
|
||||
|
||||
## TDD steps (ordered)
|
||||
|
||||
> Run with `npm test` (vitest). New frontend-logic tests are **node** (pure matcher); DOM-wiring tests reuse the existing **jsdom** harness in `test/terminal-session.test.ts`.
|
||||
|
||||
1. **`test/link-paths.test.ts` (new, node).** RED→GREEN for `findPathMatches`:
|
||||
- `'see src/app.ts:42 for'` → one match, `path:'src/app.ts', line:42`, `startX/endX` correct (1-based, `startX = index+1`, `endX = index+len`).
|
||||
- `'./a/b.tsx:10:5'` → `line:10, column:5`.
|
||||
- `'README.md'` (allowlisted ext, no slash) → matched; `'example.com'` and `'v1.2.3'` → **no** match.
|
||||
- URL guard: `'https://host/path.html'` → **no** path match (preceded-by-`/` rule).
|
||||
- Two paths on one line → two matches with disjoint ranges.
|
||||
Then implement `public/link-paths.ts` to green.
|
||||
|
||||
2. **`test/editor.test.ts` (extend, node — mirror existing spawn-a-harmless-process style, `editorCmd:'true'`).** Add a `describe('openFileInEditor')`:
|
||||
- relative → `400`; missing → `404`; **directory** → `400 'is not a file'`; non-int/`0`/`1e9+` line → `400`.
|
||||
- success on a real temp file (editorCmd `'true'`) → `status 204`.
|
||||
- **argv assertion:** write a tiny recorder script into the temp dir (`#!/bin/sh; printf '%s\n' "$@" > "$ARGS_OUT"`), set `editorCmd` to it, call with `line:42`, assert the recorded argv is `--goto`, `<file>:42`; call an unknown editor name → argv is just `<file>` (no `--goto`).
|
||||
Then implement `openFileInEditor` + `isGotoEditor` to green.
|
||||
|
||||
3. **`test/integration/server.test.ts` (extend; harness at line 229).** Boot the app, `POST /open-in-editor`:
|
||||
- `{file:<abs tmp file>, line:3}` with a valid `Origin` → `204`.
|
||||
- foreign/missing `Origin` → `403` (proves the CSRF guard still covers the new branch).
|
||||
- `{path:<abs tmp dir>}` still `204` (regression: directory mode intact).
|
||||
Wire the `server.ts` branch to green.
|
||||
|
||||
4. **`test/terminal-session.test.ts` (extend, jsdom).** Extend `FakeTerminal` (line 14) with `registerLinkProvider = vi.fn(p => { this.captured = p; return {dispose:vi.fn()} })` and a `buffer = { active: { getLine: (y)=>({ translateToString:()=> this.lineText }) } }`. Keep the `WebLinksAddon` mock (line 48) but assert the constructor **received a handler fn**. Tests:
|
||||
- after construct, a link provider is registered; feeding a line with `src/app.ts:42` → `provideLinks` callback yields one `ILink` whose `text/range` match `findPathMatches`.
|
||||
- calling `link.activate(mouseEvent, text)` when `cwd` is set (drive an OSC-7 via the captured handler, or set via a resolved path) → `fetch` (stub via `vi.stubGlobal('fetch', …)` as in `test/preview-grid.test.ts:140`) called once with `'/open-in-editor'`, method `POST`, body `{file:<abs>, line:42}`.
|
||||
- relative path **with null cwd** → **no** fetch; a `statusLine` is written to the terminal (assert `term.write`).
|
||||
- in-flight guard: two rapid `activate` calls → **one** fetch.
|
||||
- `openWebLink('javascript:alert(1)')` → `window.open` **not** called; `openWebLink('https://x')` → `window.open('https://x','_blank','noopener,noreferrer')` called.
|
||||
Wire `terminal-session.ts` to green.
|
||||
|
||||
**Coverage:** the matcher (branch-heavy) is fully covered by the node test; the DOM wiring by jsdom; the server branch + validators by editor/integration tests. This keeps the 80% gate comfortably.
|
||||
|
||||
---
|
||||
|
||||
## Edge cases & failure modes
|
||||
|
||||
- **No cwd yet** (OSC-7 never fired, `this.cwdValue === null`, `terminal-session.ts:97`) → relative paths are unresolvable → **skip activation**, write a one-line `statusLine('cannot open <path>: working dir unknown')`. Absolute paths still work.
|
||||
- **Path doesn't exist / is a dir** → server returns `404`/`400`; frontend shows a non-blocking `statusLine` toast, no throw (mirror the existing `openProjectInEditor` catch that only `console.error`s).
|
||||
- **Wide (CJK) glyphs before a path** shift xterm columns vs JS string index. Paths are ASCII, but a preceding CJK run offsets `startX`. Acceptable v1 caveat; note it. (Fixable later by walking cells; YAGNI now.)
|
||||
- **URL/path overlap** (`https://host/a.ts:5`) — `WebLinksAddon` links the URL; the `/`-preceded guard stops the path provider from double-linking the tail. Verify the two providers don't both underline.
|
||||
- **`provideLinks` line indexing** — pass `terminal.buffer.active.getLine(bufferLineNumber - 1)` and set `range.{start,end}.y = bufferLineNumber` (xterm gives a 1-based buffer row). **Verify once in a real browser** — the single indexing footgun.
|
||||
- **Rapid clicks** spawn N detached GUI processes on the host → in-flight boolean guard (+ optional 500 ms cooldown) on the frontend.
|
||||
- **Non-`code` editor** without `--goto` support → `isGotoEditor` returns false → open bare file (never inject a stray `--goto` argv the editor would treat as a filename).
|
||||
- **Line-only false positives** like `12:34` (a timestamp) — filtered because the token needs a filename-with-extension before the `:line`.
|
||||
|
||||
---
|
||||
|
||||
## Security
|
||||
|
||||
- **Origin/CSRF:** unchanged. `/open-in-editor` stays behind `requireAllowedOrigin` (`src/server.ts:385`, `:352`). The frontend POST is same-origin, so the browser sends `Origin` and passes; a foreign page's no-preflight POST is rejected `403`. Integration test #3 asserts this on the new branch.
|
||||
- **No shell / no injection:** `openFileInEditor` uses `execFile` with an **argv array** (as `editor.ts:51`); the file path and `<file>:<line>` are argv elements, never a command line. `line` is validated to an integer before interpolation, so `${file}:${line}` can't smuggle shell metacharacters via the line field.
|
||||
- **Path containment:** the resolved path is validated **absolute + existing + `isFile()`** server-side. Terminal output is attacker-influenced (a malicious repo could print `../../etc/hosts:1`), but opening a file the user could already `cat` in the shell this app *already grants* adds no privilege (threat model: LAN, no auth, full shell). Optional hardening (note, not required v1): reject resolved paths that escape the session `cwd` root.
|
||||
- **URL activation hardening (the real new surface):** custom `WebLinksAddon` handler (replacing the default at line 148) **allowlists schemes** to `http:/https:/mailto:` and opens with `window.open(uri, '_blank', 'noopener,noreferrer')` — blocks `javascript:`/`data:`/`file:` URIs and prevents reverse-tabnabbing. Activation is gated on the click (a genuine user gesture); no hover/auto-open.
|
||||
- **Rate-limit:** frontend in-flight guard caps editor-spawn fan-out; the route's `express.json({limit:'4kb'})` bounds body size. No new secrets, no logging of paths beyond the existing `console.error` on failure.
|
||||
|
||||
---
|
||||
|
||||
## Effort & dependencies
|
||||
|
||||
- **Effort:** ~**1.5–2 days.** Matcher + tests (0.5d), frontend wiring + jsdom tests + hardened URL handler (0.5d), server `openFileInEditor` + editor/integration tests (0.5d), manual browser verify of link indexing/overlap (0.25d).
|
||||
- **Depends on:** nothing — OSC-7 `cwd` capture (`terminal-session.ts:155–159`), the `/open-in-editor` route, and the `WebLinksAddon` dependency all already exist. Self-contained, W1.
|
||||
- **Unlocks / synergy:** the **Approval preview (W1, task #7)** and **diff viewer (W4, task #13)** can reuse `findPathMatches` + `openFileInEditor` to make paths in a diff/command preview clickable. `link-paths.ts` is deliberately a standalone pure module for that reuse.
|
||||
- **Scope flag for the orchestrator:** Option A adds a ~35-line server function (contradicting the brief's "no server change"). It is additive and backward-compatible, but it *is* a deviation — record it in `PROGRESS_LOG.md`. If the "no server change" constraint is hard, ship Option B (folder-open fallback) and defer file:line jump.
|
||||
138
docs/plans/w2-pty-inject-queue.md
Normal file
138
docs/plans/w2-pty-inject-queue.md
Normal file
@@ -0,0 +1,138 @@
|
||||
# Server-side PTY-inject + idle-queued follow-up prompt
|
||||
|
||||
**Feature id:** `w2-pty-inject-queue` · **Branch base:** `develop`
|
||||
|
||||
A thin, Origin/CSRF-guarded HTTP route writes text into a live session's PTY, plus a **bounded per-session queue** whose head entry fires **once** when Claude next goes idle (Stop/SessionEnd), after a short **settle delay**. This is the unlocking primitive for templated launches, auto-continue, and issue-intake.
|
||||
|
||||
Grounding facts from the code I read:
|
||||
- `writeInput(session, data)` — `src/session/session.ts:201` — no-op after PTY exit (L4); today called only from the WS input handler at `src/server.ts:876`.
|
||||
- Idle is observable in the hook side-channel: the Stop/SessionEnd branch is `src/server.ts:414` (`if (ev.eventClass === 'Stop' || ev.eventClass === 'SessionEnd')`), immediately after `manager.handleHookEvent(...)` at `:412`. `handleHookEvent` (`src/session/manager.ts:223`) sets `session.claudeStatus` and broadcasts.
|
||||
- `broadcast(session, msg)` — `src/session/session.ts:52` — fan-out to all `session.clients`.
|
||||
- CSRF helper `requireAllowedOrigin(req, res)` — `src/server.ts:352`; per-IP `createRateLimiter(max, windowMs)` — `src/server.ts:109`; `isLoopback` — `src/server.ts:151`; `SESSION_ID_RE` (UUID v4, M7) — `src/protocol.ts:22`.
|
||||
- Timer/hold precedent: `pendingApprovals` map + `setTimeout` live in **server.ts** (`:232`, `:459`), not the manager — the manager owns session state; the server owns wiring/timers. The queue follows the same split.
|
||||
- Session shape `src/types.ts:201`; `LiveSessionInfo` `:246`; `ServerMessage` union `:109`; `SessionManager` iface `:314`. `timeline`/`stuckNotified`/`telemetry` are the precedent for "mutable runtime handle on immutable meta, replaced wholesale".
|
||||
|
||||
---
|
||||
|
||||
## Contract
|
||||
|
||||
### New HTTP routes (all in `src/server.ts`, registered near the `/live-sessions/:id/preview` block ~`:334`)
|
||||
|
||||
| Method / path | Guard | Body | Success | Errors |
|
||||
|---|---|---|---|---|
|
||||
| `POST /live-sessions/:id/queue` | `requireAllowedOrigin` (CSRF) + per-IP rate limit + `SESSION_ID_RE` check | `{ text: string, appendEnter?: boolean }` (`express.json({limit:'16kb'})`) | `200 { length }` | `400` bad id / empty text / non-string; `413` text > `QUEUE_ITEM_MAX_BYTES`; `404` unknown or exited session; `409` queue at `QUEUE_MAX_ITEMS`; `429` rate; `503` when `QUEUE_ENABLED=false` |
|
||||
| `GET /live-sessions/:id/queue` | none (read-only, same threat model as `/live-sessions`) | — | `200 { length, items: string[] }` | `404` unknown |
|
||||
| `DELETE /live-sessions/:id/queue` | `requireAllowedOrigin` | — | `200 { length: 0 }` (escape hatch: cancel all pending) | `404` unknown |
|
||||
|
||||
Not loopback-gated (unlike `/hook`): these are LAN-device actions, so Origin-guarded like `/open-in-editor` (`:384`).
|
||||
|
||||
### `src/types.ts` (coordination edit — the frozen shared-contract source)
|
||||
|
||||
- **`ServerMessage`** — add a variant so all attached devices see pending count live:
|
||||
`| { type: 'queue'; length: number }`
|
||||
- **`Session`** — add a mutable runtime handle (precedent: `timeline`):
|
||||
`queue: readonly string[]` (verbatim byte strings, head fires first; replaced wholesale, never mutated in place).
|
||||
- **`LiveSessionInfo`** — add optional `readonly queueLength?: number;` (additive/optional like `lastOutputAt` at `:261`) so `/live-sessions` and the manage grid show depth.
|
||||
- **`SessionManager`** — add three methods:
|
||||
- `enqueueFollowup(id: string, text: string): EnqueueResult`
|
||||
- `drainOne(id: string): string | null` *(pops head, writes to PTY, broadcasts; null if none/exited)*
|
||||
- `clearQueue(id: string): boolean`
|
||||
- New result type:
|
||||
`export type EnqueueResult = { ok: true; length: number } | { ok: false; reason: 'unknown' | 'full' | 'exited' };`
|
||||
|
||||
### `src/config.ts` env vars (add to `Config` in types.ts `:21` block **and** `loadConfig`)
|
||||
|
||||
| Env | Field | Default | Parser (existing helper) |
|
||||
|---|---|---|---|
|
||||
| `QUEUE_ENABLED` | `queueEnabled: boolean` | `true` | `parseBool` (`config.ts:89`) |
|
||||
| `QUEUE_MAX_ITEMS` | `queueMaxItems: number` | `10` | `parseNonNegativeInt` (`:73`) |
|
||||
| `QUEUE_ITEM_MAX_BYTES` | `queueItemMaxBytes: number` | `4096` | `parseNonNegativeInt` |
|
||||
| `QUEUE_SETTLE_MS` | `queueSettleMs: number` | `1500` | `parseNonNegativeInt` |
|
||||
|
||||
### Client→server protocol
|
||||
|
||||
No new `ClientMessage`. Enqueue is HTTP POST (Origin-guarded), deliberately **not** a WS frame — it must survive "walked away, zero tabs open" and be usable from the manage page for any session, not just the WS-bound one. (Contrast: quick-reply chips send *immediately* over the active WS via `sendToActive`; the queue is *deferred* + cross-session, so HTTP is the right seam.)
|
||||
|
||||
---
|
||||
|
||||
## Files to change
|
||||
|
||||
| Path | Concrete change |
|
||||
|---|---|
|
||||
| `src/types.ts` | **Coordination edit.** Add `queue` to `Session`; `queueLength?` to `LiveSessionInfo`; `{type:'queue';length}` to `ServerMessage`; 4 `Config` fields; 3 `SessionManager` methods; `EnqueueResult` type. |
|
||||
| `src/config.ts` | Parse the 4 new env vars in `loadConfig` (helpers already exist); include in returned `Config`. |
|
||||
| `src/session/session.ts` | Init `queue: Object.freeze([])` in the `createSession` session literal (~`:137`, beside `timeline`). No other logic here — reuse existing `writeInput` (`:201`) and `broadcast` (`:52`). |
|
||||
| `src/session/manager.ts` | Implement `enqueueFollowup` / `drainOne` / `clearQueue`; add `queueLength: s.queue.length` to `list()` map (`:184`); import `writeInput` from `./session.js` (add to the existing import at `:43`). Add the three names to the returned object (`:337`). |
|
||||
| `src/server.ts` | Register the 3 routes; add a `QUEUE_RATE_MAX` const + a `createRateLimiter` instance (beside `:218`); in the Stop/SessionEnd branch (`:414`) call a new local `scheduleDrain(sessionId)` that debounces a `setTimeout(cfg.queueSettleMs)` (map `drainTimers: Map<string, Timeout>`, `.unref()`); on fire, re-check idle + stable `lastOutputAt`, then `manager.drainOne(id)`. Clear all `drainTimers` in `doShutdown` (`:930`). |
|
||||
| `public/queue.ts` **(new)** | Tiny FE module: `enqueueFollowup(sessionId, text, appendEnter): Promise<Result>` — POSTs `/live-sessions/:id/queue` (same-origin), never throws; `clearQueue(sessionId)`; parse `{type:'queue'}` frames to update a badge. |
|
||||
| `public/tabs.ts` | Handle the incoming `{type:'queue'}` frame (near the existing status/telemetry frame handling) to show a "N queued" badge on the tab; add an "Queue…" affordance (long-press on the quick-reply `+`, or a small button) that calls `enqueueFollowup(this.activeSessionId(), text)` instead of `sendToActive` (`:875`). |
|
||||
| `public/manage.*` (grid) | Optional: render `queueLength` per card and a cancel (DELETE) button. |
|
||||
|
||||
---
|
||||
|
||||
## TDD steps (ordered — RED → GREEN, matching repo style)
|
||||
|
||||
1. **`src/types.ts`** — make the coordination edit first so everything compiles. Update **every** test `CFG` fixture that lists all Config fields (`test/manager.test.ts:47`, and the same literal in `test/session.test.ts`, plus any others — `grep -rl "worktreeTimeoutMs" test/`) to add the 4 new fields. (Compile gate, no assertion.)
|
||||
|
||||
2. **`test/config.test.ts`** (node) — RED: assert defaults (`queueEnabled=true`, `queueMaxItems=10`, `queueItemMaxBytes=4096`, `queueSettleMs=1500`), env overrides parse, and invalid (`QUEUE_MAX_ITEMS=-1`) throws (fail-fast, like existing `parseNonNegativeInt` tests). → GREEN in `src/config.ts`.
|
||||
|
||||
3. **`test/manager.test.ts`** (node, node-pty mocked via `createMockPty`) — RED then GREEN in `src/session/manager.ts`:
|
||||
- `enqueueFollowup` appends → returns `{ok:true,length:1}` and **broadcasts** `{type:'queue',length:1}` to a stub `WebSocketLike` (assert `ws.send` payload via `serialize`). Second call → `length:2`.
|
||||
- Cap: with `queueMaxItems:2`, third enqueue → `{ok:false,reason:'full'}`, no broadcast, queue unchanged (immutability).
|
||||
- Unknown id → `{ok:false,reason:'unknown'}`.
|
||||
- `drainOne` on a 2-item queue → returns head string, asserts **`mockPty.write` called with that exact string**, queue now length 1, broadcasts `{type:'queue',length:1}`.
|
||||
- `drainOne` empty queue → `null`, no write. Exited session (`session.exitedAt` set) → `null` (double-guards L4).
|
||||
- `clearQueue` → empties + broadcasts `length:0`, returns true.
|
||||
- `list()` includes `queueLength`.
|
||||
*(Use the existing mock-pty `write` spy pattern from `test/session.test.ts:364`.)*
|
||||
|
||||
4. **`test/integration/queue.test.ts`** (new, node, real `startServer` + `fetch`, PTY-gated with the `itPty` helper at `server.test.ts:51`) — RED then GREEN for the routes in `src/server.ts`:
|
||||
- `POST /live-sessions/:id/queue` → `403` foreign Origin; `403`/missing Origin default-deny (mirror `server.test.ts:770`).
|
||||
- Allowed Origin, malformed id → `400`; empty `text` → `400`; `text` of `queueItemMaxBytes+1` → `413`; unknown session → `404`; over rate → `429`.
|
||||
- Happy path on a **real attached** session (open WS, attach, capture `sessionId`): `200 {length:1}`, then `GET /live-sessions` shows `queueLength:1`; `DELETE …/queue` → `queueLength:0`.
|
||||
- **Idle-drain wiring** (`itPty` + `vi.useFakeTimers`): attach real session, enqueue `"echo QUEUED_MARKER\r"`, POST `/hook` (loopback) with `{hook_event_name:'Stop', ...}` and header `x-webterm-session`, advance timers past `queueSettleMs`, assert the client WS receives an `output` frame containing `QUEUED_MARKER` (the shell echoes it). Also assert a *second* enqueue does **not** fire until the next Stop (one-per-idle pacing).
|
||||
- Settle guard: enqueue, POST Stop, then before `queueSettleMs` push a `/hook` event that produces output (changes `lastOutputAt`) → advance timers → assert **no** drain (Claude still active). *(This targets the `scheduleDrain` cursor check.)*
|
||||
|
||||
5. **`test/queue.test.ts`** (new, **jsdom**, mocked `fetch`) — RED then GREEN in `public/queue.ts`:
|
||||
- `enqueueFollowup` POSTs to the right URL/body, returns parsed result; on non-2xx returns `{ok:false}` and **never throws**; on network reject returns `{ok:false}` (mirrors quick-reply's never-throw discipline, `quick-reply.ts:99`).
|
||||
- A `{type:'queue',length:3}` frame → badge helper returns/sets 3.
|
||||
|
||||
6. **`test/tabs.test.ts`** — extend: a `{type:'queue',length:N}` server frame updates the active tab's badge; the enqueue affordance calls `enqueueFollowup` with `activeSessionId()` (not `sendToActive`).
|
||||
|
||||
**Coverage:** manager + config + routes are node-testable deterministically (queue mutation, caps, broadcasts, drain, rate/Origin/validation all hit without a real PTY). The FE module is small and fully jsdom-mockable. Only the real echo-through-PTY assertion is `itPty`-gated (auto-skips in sandbox, runs in CI) — keeps the 80% gate.
|
||||
|
||||
---
|
||||
|
||||
## Edge cases & failure modes
|
||||
|
||||
- **Idle flapping / repeated Stop:** `scheduleDrain` **debounces** — clears any existing `drainTimers` entry and restarts the settle timer on each Stop; only fires once the window elapses.
|
||||
- **New output during settle window:** capture `outputCursor = session.lastOutputAt` at schedule time; on timer fire, drain **only if** `session.lastOutputAt === outputCursor` **and** `claudeStatus === 'idle'` **and** `exitedAt === null`. Otherwise skip (a later genuine Stop reschedules). Prevents injecting mid-render.
|
||||
- **One-per-idle pacing (intended):** `drainOne` fires exactly one entry; the injected prompt makes Claude work again → its next Stop drains the next entry. If Claude errors and never emits Stop, remaining items **wait** (no spamming).
|
||||
- **Session exits with items queued:** `drainOne` guards on `exitedAt` (returns null); on session removal (`onSessionExit` L2 / `killById`) the queue dies with the session. Server clears its `drainTimers` entry in `doShutdown`; stale timers are harmless (drain returns null) and `.unref()`ed.
|
||||
- **Queue full → `409`** (never silently drop). **Oversized text → `413`.** Both actionable to the caller.
|
||||
- **Concurrent enqueue from two devices:** single-threaded, immutable array replace → both land; `{type:'queue'}` broadcast keeps every device's badge consistent.
|
||||
- **Enqueue to exited/unknown session → `404`** (checked before append).
|
||||
- **`QUEUE_ENABLED=false`** → routes `503` (graceful disable, like `/push/vapid-key:478`); Stop branch skips scheduling.
|
||||
- **Verbatim bytes / Enter:** queue stores the exact string (byte-shuttle invariant). FE decides `appendEnter` (append `\r`) at enqueue time, mirroring quick-reply's `appendEnter` (`quick-reply.ts:24`). No server-side text parsing.
|
||||
|
||||
---
|
||||
|
||||
## Security
|
||||
|
||||
- **CSRF:** `POST`/`DELETE …/queue` are state-changing and cause **shell input**, so they carry `requireAllowedOrigin` (`:352`) — the same guard as the DELETE-session routes. Without it a foreign page could inject commands into a running Claude. `GET` is read-only (queue length + prompt text the user themselves queued) → no guard, consistent with `/live-sessions`.
|
||||
- **Not loopback-gated:** intentionally Origin-gated (LAN device), not `isLoopback` — `/hook` is loopback (host-only) but enqueue must work from the phone.
|
||||
- **Path/ID containment:** validate `:id` against `SESSION_ID_RE` (`protocol.ts:22`) before any Map lookup or PTY write; reject non-UUID with `400`. The id is only a Map key — never touches argv/fs.
|
||||
- **Input validation at the boundary:** `text` must be a non-empty `string`; byte length (`Buffer.byteLength`) ≤ `queueItemMaxBytes` → else `413`. `appendEnter` coerced to boolean. Body capped by `express.json({limit:'16kb'})`. Bytes are passed **verbatim** to the PTY (raw keyboard bytes — do not filter content, per the protocol rule), but bounded in size and count.
|
||||
- **Rate limit:** dedicated per-IP `createRateLimiter(QUEUE_RATE_MAX, RATE_LIMIT_WINDOW_MS)` (e.g. 20/min) → `429`, matching the `DECISION_RATE_MAX`/`SUBSCRIBE_RATE_MAX` pattern (`:75`). Bounds injection-flood risk.
|
||||
- **DoS bounds:** `queueMaxItems` caps depth; `queueItemMaxBytes` caps size; drain writes one entry per idle → no unbounded PTY write burst.
|
||||
- **No capability tokens needed** — this reuses the app's existing LAN/Origin trust boundary (same as every other control route); it does **not** widen it. No secrets logged; sanitize any queued text before logging via existing `sanitizeForLog` (`:162`).
|
||||
- **Loopback drain source:** the drain trigger is the Stop hook, which is already loopback-gated at `/hook` (`:399`) — so the *timing* signal can't be forged remotely; only the *content* (Origin-gated) and it fires against the caller's own session.
|
||||
|
||||
---
|
||||
|
||||
## Effort & dependencies
|
||||
|
||||
- **Rough effort:** ~2–3 dev-days. Backend (types + config + manager methods + 3 routes + settle-timer wiring) ~1.5 d incl. tests; FE (`public/queue.ts` + tabs badge/affordance) ~0.5–1 d.
|
||||
- **Depends on:** nothing new — builds entirely on shipped primitives (`writeInput`, `broadcast`, `handleHookEvent` idle branch, `requireAllowedOrigin`, `createRateLimiter`, `LiveSessionInfo`). No schema migrations, no new deps.
|
||||
- **Coordination:** the `src/types.ts` edit is the only cross-cutting change — freeze it first (it touches `Config`, so every all-fields test `CFG` fixture must be updated in the same commit).
|
||||
- **Unlocks (roadmap):** templated launches / auto-continue (queue a follow-up prompt on kickoff), issue-intake (external POST that enqueues), and any "when Claude finishes, do X" automation. The manage-page `queueLength` surface also feeds the multi-session workbench view.
|
||||
131
docs/plans/w3-diff-vs-base.md
Normal file
131
docs/plans/w3-diff-vs-base.md
Normal file
@@ -0,0 +1,131 @@
|
||||
# Diff against a base branch (?base=<rev>)
|
||||
|
||||
Adds an optional `base` revision to the read-only git-diff side-channel so the viewer can compare a whole branch against `main` (or any commit-ish), not just the working tree / index. This lands the `FR-B1.9` deferral called out in `src/http/diff.ts:18-19`, using the exact mitigation named there: a `git rev-parse --verify` allow-list before any revision reaches the diff CLI. The diff **parsers and the render core stay untouched**; only a two-stage revision guard (backend), a reflected `base` field, and a toolbar picker (frontend) are added.
|
||||
|
||||
---
|
||||
|
||||
## Contract
|
||||
|
||||
### Route (unchanged path, one new optional query param)
|
||||
`GET /projects/diff` (`src/server.ts:661-678`)
|
||||
|
||||
| Param | Type | Notes |
|
||||
|---|---|---|
|
||||
| `path` | string (required) | absolute git dir; validated by `isValidGitDir` (`src/server.ts:122-133`) — unchanged |
|
||||
| `staged` | `0`\|`1` (optional) | current behavior; **ignored when `base` is present** |
|
||||
| `base` | string (optional) | a commit-ish (branch/tag/sha/`HEAD~N`). When present → three-dot diff `git diff <base>... --`; untracked files are not listed |
|
||||
|
||||
Response is the existing `DiffResult` JSON, now with an optional reflected `base`:
|
||||
- `200` structured `DiffResult` (with `base` echoed when supplied)
|
||||
- `400 {error}` — missing `path`, **or** a `base` that fails the syntactic pre-check (flag injection / junk)
|
||||
- `404 {error}` — path not a git dir (unchanged)
|
||||
- Best-effort: a syntactically-valid but **unknown/unrelated** `base` (rev-parse miss, no merge-base) yields `200` with `files: []` — consistent with the module's "git failure → empty, never throw" house style (`src/http/diff.ts:14`, `:346`).
|
||||
|
||||
### Message / data types — `src/types.ts` (coordination edit)
|
||||
Extend `DiffResult` (`src/types.ts:475-479`) with one **optional** field so the shape stays backward-compatible and the viewer's required-field validation is unaffected:
|
||||
```
|
||||
export interface DiffResult {
|
||||
files: DiffFile[];
|
||||
staged: boolean;
|
||||
truncated: boolean;
|
||||
base?: string; // NEW — echoed when the diff was against a base revision
|
||||
}
|
||||
```
|
||||
No other shared type changes. `GetDiffOptions` lives in `src/http/diff.ts:260-263` (not `types.ts`) and gains `base?: string`.
|
||||
|
||||
### Env vars — `src/config.ts`
|
||||
**None required.** rev-parse + diff reuse the existing `diffTimeoutMs` / `diffMaxBytes` bounds (`src/config.ts:347-362`). *(Optional kill-switch `DIFF_BASE_ENABLED` (default true) could be added mirroring `worktreeEnabled` at `src/config.ts:372` if a runtime disable is wanted — deferred, not needed for correctness.)*
|
||||
|
||||
### New/changed function signatures — `src/http/diff.ts`
|
||||
```
|
||||
export function isPlausibleRev(base: string): boolean // pure boundary check
|
||||
async function resolveBaseRev(cwd, base, timeoutMs, maxBytes): Promise<string | null> // rev-parse --verify → canonical sha | null
|
||||
export interface GetDiffOptions { staged: boolean; base?: string; cfg: Pick<Config,...> } // +base
|
||||
export async function getDiff(repoPath, opts): Promise<DiffResult> // branches on opts.base
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Files to change
|
||||
|
||||
| Path | Concrete change |
|
||||
|---|---|
|
||||
| `src/types.ts` | **Coordination edit.** Add optional `base?: string` to `DiffResult` (`:475-479`). |
|
||||
| `src/http/diff.ts` | Add exported pure `isPlausibleRev` (charset + no-`..` + no-leading-`-` + length≤250). Add `resolveBaseRev` (runs `git rev-parse --verify --quiet --end-of-options <base>^{commit}` via `runGit` `:289-309`; return trimmed `/^[0-9a-f]{7,64}$/` sha or `null`). Add `base?` to `GetDiffOptions` (`:260-263`). In `getDiff` (`:347-365`): if `opts.base` set → `resolved = resolveBaseRev(...)`; `null` → `{files:[],staged:false,truncated:false,base:opts.base}`; else run `git diff --no-color <resolved>... --` and `git diff --numstat <resolved>... --`, **skip** `listUntracked` (`:322-340`), set `staged:false`, echo `base:opts.base`. Working-tree path unchanged. |
|
||||
| `src/server.ts` | Diff route (`:661-678`): read `base` (`typeof q==='string' && q!=='' ? q : undefined`); if present and `!isPlausibleRev(base)` → `400 {error:'invalid base revision'}`; else pass `base` into `getDiff(target,{staged,base,cfg})`. Import `isPlausibleRev` from `./http/diff.js`. Route stays no-Origin-guard (read-only, unchanged threat model). |
|
||||
| `public/diff.ts` | `fetchDiff` (`:110-120`): change signature to `fetchDiff(repoPath, opts:{staged?:boolean; base?:string})`; build URL with `&base=<enc>` (omit `staged`) when `base` set, else `&staged=`. `normalizeDiffResult` (`:38-51`): pass through optional `base` (`typeof o['base']==='string' ? o['base'] : undefined`; keep other fields required). `MountDiffViewerOpts` (`:240-243`): add `bases?: string[]`. `mountDiffViewer` (`:253-347`): add a `<select>` "compare-base" control to the toolbar (`:265-272`) — first option `Working tree` (base=null), then one option per `bases[]`; track `base: string \| null`; when a base is chosen disable/grey the Working/Staged tabs and `loadDiff` calls `fetchDiff(repoPath,{base})`; back on "Working tree" restores `fetchDiff(repoPath,{staged})`. **Render core (`renderDiff`/`renderDiffFile`/`renderLine`/`renderHunk`) untouched.** |
|
||||
| `public/projects.ts` | `buildDiffSection` (`:611-643`): add `bases: string[]` param; pass `{ bases, onClose }` into `mountDiffViewer` (`:625`). `renderProjectDetail` (`:672`, call site `:709`): derive `bases` = unique of `detail.worktrees.map(w=>w.branch)` (`WorktreeInfo.branch`, `src/types.ts:292`) ∪ `[detail.branch]`, filtered to defined strings; pass into `buildDiffSection(detail.path, diffRef, bases)`. This is the "reuse worktree/branch data" wiring. |
|
||||
| `test/http/diff.test.ts` | unit `isPlausibleRev` + `getDiff` base integration (below). |
|
||||
| `test/integration/worktree.test.ts` | route-level base tests (real `startServer`). |
|
||||
| `test/diff.test.ts` | jsdom `fetchDiff`/`normalizeDiffResult`/picker tests. |
|
||||
| `test/worktree-form.test.ts` | assert `bases` reach the `mountDiffViewer` mock. |
|
||||
|
||||
---
|
||||
|
||||
## TDD steps (ordered)
|
||||
|
||||
**1. Pure guard — `test/http/diff.test.ts` (node)** — add a `describe('isPlausibleRev')`:
|
||||
- ✅ accepts `main`, `feature/x`, `HEAD~3`, `v1.2.0`, a 40-hex sha, `main^`, `HEAD@{1}`.
|
||||
- ❌ rejects `''`, a 300-char string, `-rf`/`--output=x` (leading `-`), `a..b`, `x y` (whitespace), `` `id` `` / `$(x)` / `;` (metachars), `\x00`.
|
||||
- Implement `isPlausibleRev` → GREEN. (Matches the existing pure-parser layer at `:34-266`.)
|
||||
|
||||
**2. `getDiff` with base — `test/http/diff.test.ts` (node, real repo)** — extend the `describe('getDiff (real git repo)')` block (`:274`). In a `beforeAll`-style setup, commit on `main`, then `git(repo,'checkout','-b','feature')`, commit a change:
|
||||
- `getDiff(repo,{staged:false,base:'main',cfg:LIMITS})` → `result.base==='main'`, `result.staged===false`, the feature-only change present, **no `untracked` entries**, counts numstat-consistent (mirror `:292-302`).
|
||||
- `base:'main'` while HEAD===main → `files:[]` (empty, no changes).
|
||||
- `base:'no-such-branch'` → `{files:[], truncated:false}` (rev-parse miss → empty; assert never throws, like `:340-347`).
|
||||
- `base:'-x'` never reaches here (route-guarded) but assert `getDiff` still returns empty (defense-in-depth) — optional.
|
||||
- Implement `resolveBaseRev` + the base branch in `getDiff` → GREEN.
|
||||
|
||||
**3. Route — `test/integration/worktree.test.ts` (node, `startServer`)** — this file already covers `GET /projects/diff` (header comment `:2-11`); add, using its `itGit` + temp-repo + two-branch setup:
|
||||
- `GET /projects/diff?path=<repo>&base=feature` → `200`, `DiffResult` with `base:'feature'`, verbatim content.
|
||||
- `GET /projects/diff?path=<repo>&base=-rf` → `400`.
|
||||
- `GET /projects/diff?path=<repo>&base=ghost-branch` → `200` with `files:[]`.
|
||||
- Wire `base` parse + `isPlausibleRev` 400 into the route → GREEN.
|
||||
|
||||
**4. Frontend fetch/normalize — `test/diff.test.ts` (jsdom)** — the file mocks `fetch`; add:
|
||||
- `fetchDiff('/repo',{base:'main'})` builds `/projects/diff?path=%2Frepo&base=main` (no `staged=`); `fetchDiff('/repo',{staged:true})` builds the current `&staged=true` URL (update the existing signature-based tests).
|
||||
- `normalizeDiffResult({files:[],staged:false,truncated:false,base:'main'})` → `.base==='main'`; a payload without `base` → `.base===undefined` and still valid.
|
||||
- Implement `fetchDiff` opts + `normalizeDiffResult` pass-through → GREEN.
|
||||
|
||||
**5. Base picker — `test/diff.test.ts` (jsdom)** — extend `describe('mountDiffViewer')` (`:353`):
|
||||
- `mountDiffViewer(container,'/repo',{bases:['main','dev']})` renders a `<select>` with options `Working tree` + `main` + `dev`.
|
||||
- Selecting `main` (dispatch `change`) triggers a fetch whose URL contains `base=main` and disables the Working/Staged tabs; selecting `Working tree` restores a `staged`-mode fetch.
|
||||
- Empty/absent `bases` → no `<select>` (backward-compatible with existing tests that call `mountDiffViewer(container,'/repo',{})`).
|
||||
- Implement toolbar select + state → GREEN.
|
||||
|
||||
**6. Wiring — `test/worktree-form.test.ts` (jsdom)** — it already mocks `mountDiffViewer` (`:31`) and tests `renderProjectDetail`/`buildDiffSection` (`:402-421`). Add: give a `ProjectDetail` with `worktrees:[{branch:'main',...},{branch:'feat',...}]`, click "View Diff", assert the `mockMountDiffViewer` was called with `bases` containing `main` and `feat`. Implement `buildDiffSection` + `renderProjectDetail` derivation → GREEN.
|
||||
|
||||
**7. Refactor / coverage** — `npm test`; confirm the 80% gate holds (every new branch — `isPlausibleRev` both arms, `resolveBaseRev` hit/miss, `getDiff` base/no-base, route 400/200, picker on/off — is exercised above).
|
||||
|
||||
---
|
||||
|
||||
## Edge cases & failure modes
|
||||
|
||||
- **`base=''`** → treated as absent (route coerces to `undefined`) → normal working-tree diff.
|
||||
- **`base` + `staged=1` both set** → `base` wins; staged silently ignored; result `staged:false`. (Documented; the picker disables the staged tab in base mode so the UI can't send both.)
|
||||
- **Flag injection** (`base=-rf`, `--output=/etc/passwd`) → rejected by `isPlausibleRev` (leading `-`) → `400`; even if it slipped through, `--end-of-options` in rev-parse and the trailing `--` in `git diff` neutralize it.
|
||||
- **Range injection** (`base=a..b`, `base=a...b`) → `isPlausibleRev` rejects `..`; we construct the `...` ourselves from a single resolved sha.
|
||||
- **Unknown ref** (typo, deleted branch) → rev-parse `--verify` miss → `resolveBaseRev` returns `null` → empty `DiffResult` (no crash). Rare in practice since the picker only offers real worktree branches.
|
||||
- **Unrelated histories** (no merge-base for `<base>...HEAD`) → `git diff` errors → `runGit` returns empty (`:302-308`) → empty result.
|
||||
- **`base` peels to a tree/tag-of-tree, not a commit** → `^{commit}` peel fails → `null` → empty.
|
||||
- **Detached HEAD in the repo** → `HEAD` still resolves; three-dot works.
|
||||
- **Huge branch diff** → existing `diffMaxFiles`/`diffMaxBytes`/timeout truncation applies unchanged (`:360-364`).
|
||||
- **Rename/binary/new/deleted across the base range** → handled by the untouched `parseUnifiedDiff`/`parseNumstat` (numstat is authoritative for counts, `:187-205`).
|
||||
- **Old git without `--end-of-options`** (pre-2.24) → not a concern in 2026, but since `isPlausibleRev` already blocks leading `-`, the flag can be dropped without loss if a legacy git is hit.
|
||||
- **jsdom picker with `bases:[]` or omitted** → no select rendered; existing `mountDiffViewer(container,'/repo',{})` tests keep passing.
|
||||
|
||||
## Security
|
||||
|
||||
- **Revision allow-list (the core mitigation, `src/http/diff.ts:18-19`)** — two stages, both before the diff CLI: (1) `isPlausibleRev` — a pure boundary check (`/^[A-Za-z0-9][A-Za-z0-9._/@^~{}-]{0,249}$/`, reject `..`) rejecting flag-injection/junk fast with a `400`; (2) `git rev-parse --verify --quiet --end-of-options <base>^{commit}` — git itself is the authoritative allow-list, and its output (a canonical 40/64-hex sha) is what's passed to `git diff`, fully decoupling the raw user string from the diff invocation.
|
||||
- **No shell** — all git calls stay `execFile('git',[...])` (`:296`), args as an array; trailing `--` terminates options on every diff command (`:352-353`), matching the file's SEC note (`:11-12`).
|
||||
- **Read-only** — rev-parse and `git diff` are read-only; `base` introduces **no** write path, so the route keeps its no-Origin-guard status (same threat model as `/projects`, `:660`). No new state-changing surface → no `requireAllowedOrigin` / CSRF change needed.
|
||||
- **Path containment** — unchanged: `isValidGitDir` three-prong (`:122-133`) still gates `path`; `base` cannot escape the repo (rev-parse resolves inside `cwd`).
|
||||
- **DoS bounds** — the extra rev-parse spawn reuses `diffTimeoutMs`/`diffMaxBytes` via `runGit` (`:289-301`); no unbounded work added.
|
||||
- **Frontend XSS** — `base` is echoed and rendered only via `textContent`/`<option>.textContent`; the SEC-H4 "zero innerHTML" invariant of `public/diff.ts` (`:8-9`) is preserved (render core untouched).
|
||||
- **Rate-limit** — parity with the existing diff route (no per-route limiter today); base adds one bounded read-only spawn per request, no new amplification. If the route is later rate-limited, this feature needs no change.
|
||||
|
||||
## Effort & dependencies
|
||||
|
||||
- **Effort:** ~1.5–2 days. Backend guard + `getDiff` branch (~0.5d incl. tests), route wiring (~0.25d), FE picker + `projects.ts` wiring (~0.75d incl. jsdom tests), polish/coverage (~0.25d).
|
||||
- **Depends on:** the shipped B1 diff stack — `src/http/diff.ts`, `public/diff.ts`, the `/projects/diff` route, and B3 worktree/branch data in `ProjectDetail.worktrees` (`src/types.ts:290-312`) which the picker reuses. No new features required.
|
||||
- **Unlocks / adjacent:** W13 "Stage / commit / push from the diff viewer" (a base-vs-branch view is the natural surface for review-before-push) and W10 "PR + CI status chip" (comparing a feature branch against its PR base). Keeping the render core and parsers unchanged means those build on the same `DiffResult` without churn.
|
||||
181
docs/plans/w3-pr-ci-chip.md
Normal file
181
docs/plans/w3-pr-ci-chip.md
Normal file
@@ -0,0 +1,181 @@
|
||||
# PR + CI/checks status chip via gh
|
||||
|
||||
A per-project chip in the project-detail view that shows, for the repo's current branch: **PR state** (open / draft / merged / closed / none), **N checks passing** (from `statusCheckRollup`), and **mergeable** (clean / conflicting). It is a read-only, out-of-band side-channel — exactly like `getDiff` (`src/http/diff.ts`): `execFile('gh', …)` (no shell), timeout + `maxBuffer` bound, parses `gh`'s `--json` output, and **capability-degrades** (chip explains itself) when `gh` is missing, unauthenticated, or the branch has no PR. Cached at module scope with a short TTL (reuses `cfg.projectScanTtlMs`) so opening/refreshing a project detail doesn't hammer the GitHub API.
|
||||
|
||||
Grounding: `buildProjectDetail` (`src/http/projects.ts:420`) surfaces branch/dirty/worktrees but nothing PR. `getDiff`/`runGit` (`src/http/diff.ts:289`) is the runner+degrade pattern to mirror. `isValidGitDir` (`src/server.ts:123`) + the `/projects/diff` route (`src/server.ts:661`) are the exact route mirror. The FE detail header is `renderProjectDetail` (`public/projects.ts:672`, header at lines 696-706); `buildDiffSection` (`public/projects.ts:612`) and `public/diff.ts` `fetchDiff`/`normalizeDiffResult` (lines 110/38) are the FE fetch+degrade+textContent pattern.
|
||||
|
||||
---
|
||||
|
||||
## Contract
|
||||
|
||||
### New route
|
||||
|
||||
`GET /projects/pr?path=<abs-repo-dir>` — read-only, **no** Origin guard (same threat model as `/projects` and `/projects/diff`; see `src/server.ts:660` comment).
|
||||
|
||||
- `400 {error}` — `path` missing/empty (mirror `src/server.ts:662-666`).
|
||||
- `404 {error:'project not found'}` — `!isValidGitDir(target)` (mirror `src/server.ts:667-670`, SEC-H7 three-prong).
|
||||
- `200 PrStatus` — **always** on a valid git dir, including all degrade cases (the availability lives in the body, not the HTTP status — so the FE renders one chip regardless). `500 {error}` only on an unexpected throw (mirror `src/server.ts:674-677`).
|
||||
|
||||
### New message type — `src/types.ts` (coordination edit, next to `DiffResult` at line 475)
|
||||
|
||||
```ts
|
||||
/* ── W3 PR + CI status chip (gh) ── */
|
||||
|
||||
/** Why a PrStatus has (or lacks) PR data. Drives the FE chip's degraded text. */
|
||||
export type PrAvailability =
|
||||
| 'ok' // a PR exists for the current branch; fields below are populated
|
||||
| 'no-pr' // gh works but the branch has no PR (or no remote/default repo)
|
||||
| 'not-installed' // `gh` binary not found on PATH (ENOENT)
|
||||
| 'unauthenticated' // gh present but not logged in (needs `gh auth login`)
|
||||
| 'disabled' // GH_ENABLED=0 — feature off, never spawns gh
|
||||
| 'error'; // gh spawned but failed for another reason (timeout, etc.)
|
||||
|
||||
/** Rolled-up CI check counts from gh's statusCheckRollup (CheckRun + StatusContext). */
|
||||
export interface PrCheckSummary {
|
||||
total: number;
|
||||
passing: number; // CheckRun conclusion SUCCESS/NEUTRAL/SKIPPED | StatusContext SUCCESS
|
||||
failing: number; // FAILURE/TIMED_OUT/CANCELLED/ACTION_REQUIRED | ERROR/FAILURE
|
||||
pending: number; // QUEUED/IN_PROGRESS/WAITING | PENDING/EXPECTED
|
||||
}
|
||||
|
||||
/** GET /projects/pr result. Only present-when-'ok' fields are optional. */
|
||||
export interface PrStatus {
|
||||
availability: PrAvailability;
|
||||
number?: number;
|
||||
title?: string;
|
||||
url?: string;
|
||||
state?: 'open' | 'closed' | 'merged'; // lower-cased from gh OPEN/CLOSED/MERGED
|
||||
isDraft?: boolean;
|
||||
mergeable?: 'mergeable' | 'conflicting' | 'unknown'; // lower-cased from gh
|
||||
headRefName?: string;
|
||||
baseRefName?: string;
|
||||
checks?: PrCheckSummary;
|
||||
}
|
||||
```
|
||||
|
||||
### `gh` invocation (single spawn — KISS)
|
||||
|
||||
One command; `statusCheckRollup` already carries per-check state, so no second `gh pr checks` spawn:
|
||||
|
||||
```
|
||||
gh pr view --json number,state,title,url,isDraft,mergeable,headRefName,baseRefName,statusCheckRollup
|
||||
```
|
||||
|
||||
Run with `cwd = repoPath`. gh resolves the PR from the current branch. `statusCheckRollup` items are a mix of `{__typename:'CheckRun', status, conclusion}` and `{__typename:'StatusContext', state}` — the pure parser handles both.
|
||||
|
||||
### New env vars — `src/config.ts` + `Config` in `src/types.ts` (coordination edit)
|
||||
|
||||
| Env var | Default | Purpose |
|
||||
|---|---|---|
|
||||
| `GH_ENABLED` | `true` (`parseBool`) | Feature flag. `false` → route returns `{availability:'disabled'}`, never spawns gh. |
|
||||
| `GH_TIMEOUT_MS` | `8000` (`parseNonNegativeInt`) | Hard-kill timeout for the gh spawn. Larger than `diffTimeoutMs` (2 s) because gh hits the network. |
|
||||
|
||||
Cache TTL **reuses** `cfg.projectScanTtlMs` (`src/config.ts:311`, default 10 000 ms) — no new TTL var. Add the two fields to the assembled object in `loadConfig` (`src/config.ts:390-438`) and to the `Config` interface.
|
||||
|
||||
---
|
||||
|
||||
## Files to change
|
||||
|
||||
| Path | Concrete change |
|
||||
|---|---|
|
||||
| `src/types.ts` | **Coordination edit.** Add `PrAvailability`, `PrCheckSummary`, `PrStatus` near `DiffResult` (line 475). Add `ghEnabled: boolean` + `ghTimeoutMs: number` to the `Config` interface (find `Config` — used by `loadConfig`). |
|
||||
| `src/config.ts` | Add `const DEFAULT_GH_TIMEOUT_MS = 8000` near line 65; parse `GH_ENABLED` via `parseBool(env['GH_ENABLED'], true)` and `GH_TIMEOUT_MS` via `parseNonNegativeInt(...)`; add both to the frozen object (lines 413-438). |
|
||||
| `src/http/gh.ts` | **New file** (~180 lines), mirrors `diff.ts` structure: a `runGh` runner (`execFileAsync('gh', args, {cwd, timeout, maxBuffer})` that captures `stdout`/`stderr`/`code`/spawn-ENOENT), a **pure** `parsePrView(json): PrStatus`-core + `summarizeChecks(rollup): PrCheckSummary`, a `classifyGhFailure(exec): PrAvailability`, module-scope short-TTL cache mirroring `discoverCache` (`projects.ts:239-317`) with in-flight dedupe, `getPrStatus(repoPath, cfg): Promise<PrStatus>`, and a `_clearPrCache()` test hook (mirror `_clearProjectCache`, `projects.ts:314`). |
|
||||
| `src/server.ts` | Add `import { getPrStatus } from './http/gh.js'` (next to line 41). Add `GET /projects/pr` route immediately after `/projects/diff` (after line 678), copying the `path`-missing → 400 and `!isValidGitDir` → 404 guards, then `res.json(await getPrStatus(target, cfg))` in a try/catch → 500 (mirror lines 671-677). |
|
||||
| `public/gh-chip.ts` | **New file** (~120 lines), render-only, mirrors `public/diff.ts`: `normalizePrStatus(raw): PrStatus \| null`, `fetchPrStatus(repoPath): Promise<PrStatus \| null>` (mirror `fetchDiff`, `diff.ts:110`), `chipText(status): {label, cls, title}` (pure, unit-tested), `renderPrChip(status): HTMLElement` (all text via **`textContent`**, zero `innerHTML` — SEC-H4), `mountPrChip(container, repoPath): {destroy()}` that shows a loading placeholder then swaps in the resolved chip. |
|
||||
| `public/projects.ts` | Add `import { mountPrChip } from './gh-chip.js'` (near line 27). In `renderProjectDetail`, after the dirty indicator (line 704) inside the `if (detail.isGit)` guard, append the chip host and mount it; track the handle in a `prRef` and `destroy()` it in the `back` click handler (lines 683-688) alongside `diffRef.h?.destroy()`. |
|
||||
| `public/styles.css` (or the existing project-detail CSS file) | Add `.proj-pr-chip` + state modifier classes (`.proj-pr-open/.draft/.merged/.closed/.none/.unavailable`, `.proj-pr-checks-ok/.fail/.pending`, `.proj-pr-conflict`). Reuse the existing `.proj-branch` chip look (line 699) as the base. |
|
||||
| `test/http/gh.test.ts` | **New** (node) — pure parsers + `getPrStatus` classification. |
|
||||
| `test/integration/pr-status.test.ts` | **New** (node) — real `startServer`, `gh` stubbed via a PATH shim. |
|
||||
| `test/gh-chip.test.ts` | **New** (jsdom) — `normalizePrStatus`/`chipText`/`renderPrChip`/`mountPrChip`. |
|
||||
|
||||
---
|
||||
|
||||
## TDD steps
|
||||
|
||||
Ordered; each "write test → run RED → implement → GREEN". Backend-pure first (cheap, deterministic), then route, then FE. Keeps the 80 % gate because the pure parser + classifier are the bulk of the logic and are fully covered without spawning gh.
|
||||
|
||||
**Backend — `test/http/gh.test.ts`** (node; mirror `test/http/diff.test.ts:1-33`). Import from `../../src/http/gh.js`.
|
||||
|
||||
1. `summarizeChecks` — canned `statusCheckRollup` arrays:
|
||||
- CheckRun `{status:'COMPLETED',conclusion:'SUCCESS'}` → passing++. Implement `summarizeChecks`.
|
||||
- CheckRun `conclusion:'FAILURE'` → failing++; `TIMED_OUT`/`CANCELLED`/`ACTION_REQUIRED` → failing.
|
||||
- CheckRun `status:'IN_PROGRESS'`/`'QUEUED'` (null conclusion) → pending.
|
||||
- StatusContext `{state:'SUCCESS'}` → passing; `'PENDING'` → pending; `'FAILURE'/'ERROR'` → failing.
|
||||
- `NEUTRAL`/`SKIPPED` → passing (don't block). Empty/`undefined` rollup → all-zero. Unknown shape → counted in `total` only, treated as pending. Assert `total === passing+failing+pending`.
|
||||
2. `parsePrView` — feed a full canned JSON string:
|
||||
- Valid PR JSON → `availability:'ok'`, lower-cased `state`/`mergeable`, `number`/`title`/`url`/`isDraft`/`headRefName`/`baseRefName` mapped, `checks` from `summarizeChecks`. Implement `parsePrView` (never throws — `try/JSON.parse`; malformed → `{availability:'error'}`, mirroring `diff.ts` "never throws" house style).
|
||||
- `isDraft:true` still `availability:'ok'` (FE decides the "draft" label); `mergeable:'UNKNOWN'` → `'unknown'`.
|
||||
- Malformed / non-object JSON → `{availability:'error'}`.
|
||||
- **Security assert**: a PR `title` containing `<script>alert(1)</script>` survives verbatim in `PrStatus.title` (proves no parsing-side mangling; FE renders it inert).
|
||||
3. `classifyGhFailure` — given synthetic exec results:
|
||||
- spawn ENOENT (`code:'ENOENT'`) → `'not-installed'`.
|
||||
- stderr containing `gh auth login` / `not logged` / `authentication` / `HTTP 401` → `'unauthenticated'`.
|
||||
- stderr containing `no pull requests found` / `no default remote` / `no git remote` → `'no-pr'`.
|
||||
- other non-zero exit → `'error'`. Implement `classifyGhFailure` (regex on lower-cased stderr).
|
||||
4. `getPrStatus` cache/dedupe — inject a fake runner (or spy) so no real gh spawns:
|
||||
- `ghEnabled:false` in cfg → resolves `{availability:'disabled'}` **without** invoking the runner.
|
||||
- Two rapid calls for the same path share one in-flight run (assert runner called once); after `_clearPrCache()`, it runs again. Mirror `projects.ts:289-311`. Implement the module cache + `_clearPrCache`.
|
||||
- Cache key includes the current branch (cheap read of `.git/HEAD` like `readBranch`, `projects.ts:88`) so a branch switch busts the cache before TTL. Test: same path, different HEAD branch → runner re-invoked.
|
||||
|
||||
> To keep `getPrStatus` unit-testable without gh, factor the spawn into an injectable `runGh` (default real, overridable in tests) — same seam idea as `getDiff`'s `runGit`. `parsePrView`/`summarizeChecks`/`classifyGhFailure` stay pure and exported.
|
||||
|
||||
**Route — `test/integration/pr-status.test.ts`** (node; mirror `test/integration/projects-endpoint.test.ts:1-55`). Use `getFreePort` + a temp dir with a fake `.git` repo (reuse `makeFakeGitRepo` shape). Stub gh with a **PATH shim**: write an executable script named `gh` into a temp `bin/` that echoes canned JSON (or exits 1 with a canned stderr), then set `process.env.PATH = binDir + ':' + process.env.PATH` before `startServer` (execFile resolves `gh` via PATH). Restore PATH + `_clearPrCache()` in `afterEach`.
|
||||
|
||||
5. Missing `path` → **400**. Implement route guard 1.
|
||||
6. Non-git dir path → **404** (`isValidGitDir` fails). Implement guard 2.
|
||||
7. gh shim emits valid PR JSON → **200** with `availability:'ok'`, correct `checks`. Wire `res.json(await getPrStatus(...))`.
|
||||
8. gh shim exits 1 with `no pull requests found` on stderr → **200 `{availability:'no-pr'}`**.
|
||||
9. `GH_ENABLED='0'` env → **200 `{availability:'disabled'}`**, and (assert via a shim that writes a marker file) gh is never spawned.
|
||||
|
||||
**Frontend — `test/gh-chip.test.ts`** (jsdom; mirror `test/diff.test.ts:1-14`, `// @vitest-environment jsdom`, dynamic `await import('../public/gh-chip.js')`, `vi.stubGlobal('fetch', …)`).
|
||||
|
||||
10. `normalizePrStatus` — valid object round-trips; non-object / bad `availability` → `null` (mirror `normalizeDiffResult`, `diff.ts:38`). Implement.
|
||||
11. `chipText(status)` — pure map: `ok`+open → `"PR #12 ✓ 5/5"`; failing checks → `"PR #12 ✕ 3/5"`; `mergeable:'conflicting'` adds a `⚠ conflicts` marker/class; `no-pr` → `"No PR"`; `not-installed` → `"gh not installed"` + `title` link to cli.github.com; `unauthenticated` → `"gh auth login"`; `disabled` → chip hidden (returns null/`display:none`). Implement.
|
||||
12. `renderPrChip` — **SEC-H4 assert**: a `title` of `<img src=x onerror=...>` appears as literal text (`el.textContent` contains it, `el.querySelector('img')` is null). Zero `innerHTML`.
|
||||
13. `mountPrChip` — `fetch` stubbed to resolve `ok` JSON → placeholder replaced by the chip; `fetch` rejects → degrades to an `error` chip (no throw); `destroy()` removes the node. Mirror `mountDiffViewer` (`diff.ts:253-`).
|
||||
|
||||
**Frontend wiring — extend `test/projects.test.ts`** (jsdom; `renderProjectDetail` is already exported/tested there).
|
||||
|
||||
14. `renderProjectDetail` with `detail.isGit:true` → a `.proj-pr-chip` host is present in the header; with `isGit:false` → absent. (Mock `gh-chip`'s `mountPrChip` via `vi.mock` so the DOM assertion doesn't depend on fetch.) Implement the `renderProjectDetail` edit + `prRef.destroy()` in the back handler.
|
||||
|
||||
Run `npm test` after each GREEN. The pure backend parser tests (steps 1-3) carry most of the coverage weight for `gh.ts`; FE steps 10-13 cover `gh-chip.ts`.
|
||||
|
||||
---
|
||||
|
||||
## Edge cases & failure modes
|
||||
|
||||
- **`gh` not installed** → spawn ENOENT → `'not-installed'`. Chip shows "gh not installed" (never a 500, never a stack trace).
|
||||
- **`gh` present, not authenticated** → stderr auth pattern → `'unauthenticated'` → "gh auth login".
|
||||
- **No PR for branch / no remote / detached HEAD** → `'no-pr'` → "No PR". (Detached HEAD: `.git/HEAD` isn't `ref:` → cache-key branch is `null`; gh itself errors → `no-pr`.)
|
||||
- **PR exists but checks not started / all pending** → `checks.total>0, passing=0, pending=total` → "⧗ 0/N".
|
||||
- **`mergeable:'UNKNOWN'`** (GitHub computes mergeability async right after a push) → `'unknown'` → neutral marker, not a red "conflict". Only `'conflicting'` shows the ⚠.
|
||||
- **Draft PR** → `isDraft:true` → "Draft" styling; still `availability:'ok'`.
|
||||
- **Merged/closed PR still on the branch** → `state:'merged'/'closed'` badge (gh returns the most recent PR).
|
||||
- **gh timeout** (network hang) → `execFileAsync` kills at `ghTimeoutMs` → `'error'` → generic "PR status unavailable". Bounded, never hangs the request.
|
||||
- **Huge `statusCheckRollup`** (100s of checks / monorepo) → bounded by `maxBuffer` (reuse `diffMaxBytes`); overflow → `'error'` (don't try to parse a truncated JSON). Counts are aggregate so the chip stays tiny.
|
||||
- **Malformed `--json` output** (gh version drift) → `parsePrView` `JSON.parse` throws → caught → `'error'`.
|
||||
- **Branch switch within TTL** → cache key includes HEAD branch, so it busts immediately rather than showing the previous branch's PR for up to 10 s.
|
||||
- **FE fetch/network error** → `fetchPrStatus` returns `null` → chip renders an `'error'` state, never throws (mirror `fetchDiff`, `diff.ts:117`).
|
||||
|
||||
---
|
||||
|
||||
## Security
|
||||
|
||||
- **No shell**: `execFile('gh', [fixed argv])` — identical guarantee to `runGit` (`diff.ts:296`, SEC-M9). The **only** user-influenced input reaching gh is `cwd`, which is the already-validated `repoPath`. No untrusted string is ever placed in argv (gh derives the PR from the branch; we never pass a branch/base/rev). This sidesteps the `?base=` deferral rationale in `diff.ts:18-20`.
|
||||
- **Path containment**: route calls `isValidGitDir(target)` (`server.ts:123`) before spawning — absolute + is-dir + has `.git` (SEC-H7). Path traversal / arbitrary-cwd is blocked exactly as `/projects/diff`.
|
||||
- **DoS bounds**: `timeout: ghTimeoutMs` (hard kill) + `maxBuffer: cfg.diffMaxBytes` bound a slow/huge gh. Module-scope TTL cache + in-flight dedupe cap outbound GitHub-API calls to ≈1 per repo per `projectScanTtlMs` even under rapid detail-view refreshes (the panel auto-refreshes every 5 s, `projects.ts:33`).
|
||||
- **Network egress note**: unlike every other side-channel (all local), gh talks to GitHub's API using the host's existing `gh`/`GH_TOKEN` credential. The endpoint **never accepts or forwards a token** — it only triggers gh's own auth. Document this in the route comment and in TECH_DOC §7 (this is the first feature to make an outbound call on behalf of a LAN client; the `GH_ENABLED=0` kill-switch lets a cautious operator disable it entirely).
|
||||
- **No secret leakage**: never log gh **stdout** (may contain private PR titles) or the token. If logging a failure, log only `availability` + `sanitizeForLog(stderr.slice(0,200))` (reuse `server.ts:162`) — never raw stderr, mirroring the worktree audit line (`server.ts:714`) and SEC-M10 "never raw git stderr".
|
||||
- **Origin/CSRF**: GET is read-only and non-mutating → no Origin guard, consistent with `/projects` and `/projects/diff` (`server.ts:660`). It spawns a subprocess but performs no state change, so CSWH/CSRF risk is limited to triggering a cached, rate-bounded read.
|
||||
- **XSS**: PR `title` is attacker-controllable (anyone who can open a PR on a repo the host has access to). It is carried verbatim server-side and rendered **only via `textContent`** in `gh-chip.ts` (SEC-H4, same discipline as `diff.ts` line-rendering). Explicit jsdom test (step 12) asserts no element injection.
|
||||
- **Rate-limit**: no per-IP limiter needed (read-only, same as `/projects/diff`); the TTL cache is the effective throttle. If desired later, the `createRateLimiter` helper (`server.ts:109`) is available.
|
||||
|
||||
---
|
||||
|
||||
## Effort & dependencies
|
||||
|
||||
- **Effort**: ~1.5–2 days. Backend `gh.ts` + route ≈ 0.75 day (the runner/cache pattern is a near-copy of `diff.ts`/`projects.ts`; the pure `parsePrView`/`summarizeChecks` classifier is the real work). FE `gh-chip.ts` + wiring + CSS ≈ 0.5 day. Tests (3 files) ≈ 0.5 day, with the PATH-shim integration harness the only novel piece.
|
||||
- **Depends on**: nothing hard-blocking — `Config`/`src/types.ts`/`config.ts` coordination edits and `isValidGitDir` all exist today. Requires `gh` on the host for the live path, but the feature is designed to degrade cleanly without it (so it ships regardless).
|
||||
- **Unlocks**: the **W3 "Quick wins" chip** (task #11 — sync/ahead-behind chip, recent commits) can reuse the same `gh.ts`/`gh-chip.ts` side-channel + short-TTL-cache scaffold (e.g. `gh pr status`, `git rev-list --count @{u}...HEAD`). The PATH-shim gh-integration harness is reusable by any future gh-backed feature.
|
||||
- **Interacts with (no conflict)**: **W3 `?base=` diff** (task #9) touches `diff.ts`/`/projects/diff` only; this feature is an independent file (`gh.ts`) and route (`/projects/pr`). Both add a chip/section to the same `renderProjectDetail` header — coordinate the header layout (place the PR chip after the branch chip, before or beside the diff toggle) but they do not edit the same functions.
|
||||
174
docs/plans/w3-quick-wins.md
Normal file
174
docs/plans/w3-quick-wins.md
Normal file
@@ -0,0 +1,174 @@
|
||||
# Quick wins: sync chip + cost guard + reconnect digest + recent commits
|
||||
|
||||
Four small, independent features that ride existing seams. None touches the byte-shuttle WS stream; all new HTTP routes are side-channel and follow the established `/projects/*` / `/hook/*` patterns. Read-only routes get no Origin guard (same threat model as `/projects` — `src/server.ts:281`); the cost guard rides the already-injected `NotifyService` DI seam and the `stuckNotified` one-shot-latch pattern (`src/session/manager.ts:271-287`).
|
||||
|
||||
Shared coordination edit up front: **`src/types.ts`** gains `ProjectInfo.ahead/behind/lastCommitMs` (§a), `NotifyClass` `'budget'` + `Session.budgetNotified` + `Config.costBudgetUsd` + `UiConfig.costBudgetUsd` (§b), `DigestResult`/`DigestSession` (§c), `CommitLogEntry`/`GitLogResult` (§d). Do all type edits in one commit so the four sub-features can then proceed in parallel.
|
||||
|
||||
---
|
||||
|
||||
## Contract
|
||||
|
||||
### (a) Sync chip — no new route
|
||||
Folded into the existing `GET /projects` payload. `ProjectInfo` gains three optional fields, populated in the cached per-repo metadata pass:
|
||||
|
||||
```ts
|
||||
// src/types.ts — extend ProjectInfo (currently line 279-287)
|
||||
export interface ProjectInfo {
|
||||
name: string
|
||||
path: string
|
||||
isGit: boolean
|
||||
branch?: string
|
||||
dirty?: boolean
|
||||
lastActiveMs?: number
|
||||
ahead?: number // commits on HEAD not on @{u} (git rev-list, right count)
|
||||
behind?: number // commits on @{u} not on HEAD (git rev-list, left count)
|
||||
lastCommitMs?: number // git log -1 --format=%ct * 1000 (HEAD commit time)
|
||||
sessions: ProjectSessionRef[]
|
||||
}
|
||||
```
|
||||
`@{u}` with no upstream / non-git → all three `undefined` (best-effort, never throws). No new env var: gated by the existing `projectDirtyCheck` (which already means "spend git subprocess time per repo").
|
||||
|
||||
### (b) Cost budget guard + push alert
|
||||
- **New env** `COST_BUDGET_USD` (dollars, float ≥ 0; default `0` = disabled) → `Config.costBudgetUsd: number` (`src/types.ts` Config, alongside B2 `statuslineTtlMs` at line 65).
|
||||
- **New `NotifyClass` member** `'budget'` (`src/types.ts:376`): `'needs-input' | 'done' | 'stuck' | 'budget'`.
|
||||
- **New `Session` field** `budgetNotified: boolean` (`src/types.ts` Session, next to `stuckNotified` at line 227-228) — one-shot latch, **never re-armed** (cost is monotonic).
|
||||
- **`GET /config/ui`** payload gains `costBudgetUsd?: number` so the FE can derive warn-styling client-side:
|
||||
```ts
|
||||
export interface UiConfig { allowAutoMode: boolean; costBudgetUsd?: number }
|
||||
```
|
||||
- **No new ServerMessage variant.** The "warning broadcast" is the *existing* `{type:'telemetry', telemetry}` frame (already broadcast on every statusLine, `manager.ts:260`); the warn is derived on the client (`costUsd >= costBudgetUsd`). The distinct new server action on threshold crossing is a single `notifyService.notify(session, 'budget')` push. (Rationale: keeps the frozen `ServerMessage` contract in `types.ts:109-120` untouched — KISS/YAGNI. A dedicated frame was considered and rejected: cost overage is not a `ClaudeStatus`.)
|
||||
- `renderTelemetryGauge` (`public/preview-grid.ts:197`) gains a 4th param `costBudgetUsd?: number`; the cost chip (line 224-227) gets class `tg-cost-warn` when `telemetry.costUsd >= budget`, mirroring the ctx>80% path at line 219.
|
||||
|
||||
### (c) While-you-were-away reconnect digest
|
||||
**New read-only route** `GET /digest?since=<epochMs>` (no Origin guard; same as `/live-sessions`). Aggregates `manager.list()`:
|
||||
|
||||
```ts
|
||||
export interface DigestSession {
|
||||
id: string
|
||||
title?: string // last cwd segment
|
||||
status: ClaudeStatus
|
||||
costUsd?: number // telemetry.costUsd
|
||||
lastOutputAt?: number
|
||||
finished: boolean // status==='idle' && lastOutputAt > since
|
||||
needsInput: boolean // status==='waiting'
|
||||
stuck: boolean // status==='stuck'
|
||||
}
|
||||
export interface DigestResult {
|
||||
since: number
|
||||
generatedAt: number
|
||||
total: number
|
||||
finished: number
|
||||
needsInput: number
|
||||
stuck: number
|
||||
working: number
|
||||
totalCostUsd: number
|
||||
sessions: DigestSession[]
|
||||
}
|
||||
```
|
||||
Response: `200` + `DigestResult` (empty aggregate when no sessions). `since` clamped to a finite non-negative number (bad/absent → `0`, i.e. "everything is new").
|
||||
|
||||
### (d) Recent-commits log per project
|
||||
**New read-only route** `GET /projects/log?path=<abs>&n=<int>` (no Origin guard; guarded by `isValidGitDir`, `src/server.ts:123`, exactly like `/projects/diff` at line 661-678).
|
||||
|
||||
```ts
|
||||
export interface CommitLogEntry { hash: string; at: number; subject: string } // at = %ct*1000
|
||||
export interface GitLogResult { commits: CommitLogEntry[]; truncated: boolean }
|
||||
```
|
||||
- `path` missing/empty → `400`; not a valid git dir → `404` (three-prong `isValidGitDir`); git failure → `500`.
|
||||
- `n` parsed to int, clamped to `[1, GIT_LOG_MAX]` (const `50`; default `20`).
|
||||
- Git command (NUL-record, US-field delimited so subjects with tabs/newlines can't corrupt parsing):
|
||||
```
|
||||
git log --no-color -z -n <n> --format=%h%x1f%ct%x1f%s (cwd: repoPath, timeout, maxBuffer)
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Files to change
|
||||
|
||||
| Path | Change |
|
||||
|---|---|
|
||||
| `src/types.ts` | **Coordination edit.** Add `ProjectInfo.ahead/behind/lastCommitMs`; `NotifyClass` `'budget'`; `Session.budgetNotified`; `Config.costBudgetUsd`; `UiConfig.costBudgetUsd`; new `DigestResult`/`DigestSession`, `CommitLogEntry`/`GitLogResult`. |
|
||||
| `src/config.ts` | Add `parseNonNegativeFloat(raw,label,fallback)` helper; parse `COST_BUDGET_USD`→`costBudgetUsd` (near B2 block line 365-369); add `costBudgetUsd` to the frozen object (spread block line 415-437). |
|
||||
| `src/http/projects.ts` | Add `readSync(repoPath)` helper (2 execFile calls); extend `MakeProjectArgs` (line 121) + `makeProject` (line 129) with `ahead/behind/lastCommitMs`; call `readSync` in `runDiscovery`'s per-repo `mapWithConcurrency` (line 276-280), gated by `cfg.projectDirtyCheck`. |
|
||||
| `src/http/git-log.ts` | **New.** `parseGitLog(stdout,max)` (pure) + `getGitLog(repoPath,{n,timeoutMs})` (async, execFile, no shell). Modelled on `src/http/diff.ts` / `readDirty` (`projects.ts:98`). |
|
||||
| `src/http/digest.ts` | **New.** `buildDigest(live: readonly LiveSessionInfo[], since: number): DigestResult` — pure, injected list (mirrors `buildProjects` injection, `projects.ts:387`). |
|
||||
| `src/session/manager.ts` | In `handleStatusLine` (line 256): after storing/broadcasting telemetry, run the budget-latch check → `notifyService?.notify(session,'budget')`. |
|
||||
| `src/session/session.ts` | Init `budgetNotified: false` in `createSession` object literal (~line 138). **Do not** re-arm (leave line 150 as-is). |
|
||||
| `src/server.ts` | Add `GET /projects/log` (after `/projects/diff`, line 678); add `GET /digest` (after `/live-sessions`, line 279); extend `GET /config/ui` (line 728-731) with `costBudgetUsd`. Import `getGitLog`, `buildDigest`. |
|
||||
| `public/preview-grid.ts` | `renderTelemetryGauge` gains `costBudgetUsd?` param; add `tg-cost-warn` class on the cost chip (line 224-227). |
|
||||
| `public/projects.ts` | `normalizeProject` (line 236): pass through numeric `ahead/behind/lastCommitMs`. `makeProjectCard` (line 454): add sync chip after branch chip. `renderProjectDetail` (line 709 area, git repos only): mount recent-commits section. |
|
||||
| `public/git-log.ts` | **New.** `mountGitLog(container, repoPath)` — fetch `GET /projects/log`, render inert rows via `textContent` only. |
|
||||
| `public/digest.ts` | **New.** `mountDigest(host)` — read `localStorage` last-seen, fetch `GET /digest?since=`, render dismissible banner, update last-seen. |
|
||||
| `public/tabs.ts` | In `loadUiConfig` (line 248-261): also read `costBudgetUsd`; store on the instance; pass to `renderTelemetryGauge` call (line 1391). |
|
||||
| `public/main.ts` | `mountDigest(...)` in the toolbar/app wiring block (~line 51-119). |
|
||||
| `public/style.css` | Add `.tg-cost-warn`, `.proj-sync`, `.proj-commitlog` rows, `.wya-banner` (mirror `.tg-ctx-warn`, `.proj-branch`). |
|
||||
|
||||
---
|
||||
|
||||
## TDD steps (ordered)
|
||||
|
||||
Repo style: backend unit tests are **node** env (default), FE tests declare `// @vitest-environment jsdom` at the top (`test/projects-panel.test.ts:1`) and mock `@xterm/xterm`; route tests spin a real server on a free port (`test/integration/projects-endpoint.test.ts`). `test/manager.test.ts` uses a mock `NotifyService` recording `notify(session,cls,token)` (line 100-107) and a `parseSent(ws)` helper.
|
||||
|
||||
**0. Types (RED→GREEN, compile-only)**
|
||||
- [ ] Edit `src/types.ts` with all shapes above. `npx tsc --noEmit` fails where callers/mocks lack new required fields → gives the worklist. `Session.budgetNotified` is required → `test/manager.test.ts` + `src/session/session.ts` must init it.
|
||||
|
||||
**(b) Cost guard — highest security value, do first**
|
||||
- [ ] `test/config.test.ts`: add defaults block — `loadConfig({}).costBudgetUsd === 0`; override `COST_BUDGET_USD:'5.50'`→`5.5`; `throw` for `'abc'` and `'-1'` (mirror line 156-181). Implement `parseNonNegativeFloat` + wire in `src/config.ts`.
|
||||
- [ ] `test/manager.test.ts` (extend `describe('handleStatusLine')` line 803): with `cfg.costBudgetUsd=1`, feed telemetry `costUsd=0.5` → `notify` **not** called, `session.budgetNotified===false`; feed `costUsd=1.2` → `notify` called once with `(s,'budget')`, latch `true`; feed `costUsd=2` again → **not** called again. With `costBudgetUsd=0` → never called. Implement the latch in `manager.handleStatusLine`.
|
||||
- [ ] `test/session.test.ts`: assert `createSession(...).budgetNotified === false`. Implement init in `session.ts`.
|
||||
- [ ] `test/preview-grid.test.ts` (jsdom): `renderTelemetryGauge(c, {costUsd:6,at:now}, ttl, 5)` → cost chip has class `tg-cost-warn`; with budget `0`/`undefined` or `costUsd<budget` → no warn class. Implement param.
|
||||
- [ ] Route: extend `test/integration` (or `test/http`) — `GET /config/ui` returns `costBudgetUsd`. Implement in `server.ts:728`.
|
||||
|
||||
**(d) Recent commits**
|
||||
- [ ] `test/http/git-log.test.ts` (new): `parseGitLog` — NUL-record + US-field splitting; empty stdout→`[]`; malformed record (missing field) skipped; subject truncated at cap; `truncated` flag when records===max. (Pure, no spawn.)
|
||||
- [ ] `test/http/git-log.test.ts`: `getGitLog` against a real temp repo made with `git init` + 3 commits (pattern from `test/http/diff.test.ts` / worktrees test) → 3 entries newest-first, `n=2`→2 + `truncated:true`.
|
||||
- [ ] `test/integration/projects-log-endpoint.test.ts` (new, model on `projects-endpoint.test.ts`): `GET /projects/log?path=<repo>` → 200 array; missing `path`→400; non-git temp dir→404; `?n=999`→clamped. Implement route in `server.ts`.
|
||||
- [ ] FE `test/git-log.test.ts` (jsdom): `mountGitLog` renders rows via `textContent`; a commit subject containing `<img onerror>` appears verbatim (no HTML injection); fetch failure → empty/error inert text. Implement `public/git-log.ts` + detail-section wiring in `public/projects.ts`.
|
||||
|
||||
**(a) Sync chip**
|
||||
- [ ] `test/projects.test.ts` (extend, node): real temp repo (uses `execFileP` already imported line ~20) with an upstream branch ahead/behind → `buildProjects` yields `ahead`/`behind`/`lastCommitMs`; repo with **no** upstream → those `undefined`, no throw; `projectDirtyCheck:false` → sync skipped (undefined). Implement `readSync` + fold into `runDiscovery`.
|
||||
- [ ] `test/projects-panel.test.ts` (jsdom): `normalizeProject` passes numeric `ahead/behind/lastCommitMs`, drops non-numbers; `makeProjectCard` renders `↑2 ↓1` chip when set, omits when `undefined`/`0`. Implement FE.
|
||||
|
||||
**(c) Reconnect digest**
|
||||
- [ ] `test/http/digest.test.ts` (new, node): `buildDigest([...LiveSessionInfo], since)` — counts finished (`idle` & `lastOutputAt>since`), needsInput (`waiting`), stuck, working; `totalCostUsd` sums `telemetry.costUsd`; empty list → zeroes; `since` in the future → 0 finished.
|
||||
- [ ] `test/integration/digest-endpoint.test.ts` (new): server up, `GET /digest?since=0` → 200 `DigestResult`; malformed `since` → treated as 0. Implement route.
|
||||
- [ ] FE `test/digest.test.ts` (jsdom): `mountDigest` fetches with the stored last-seen; renders banner only when `finished+needsInput+stuck>0`; dismiss updates localStorage last-seen and hides; fetch failure → no banner (best-effort). Implement `public/digest.ts` + `main.ts` mount.
|
||||
|
||||
**Coverage:** every new pure function (`parseGitLog`, `buildDigest`, `readSync` via `buildProjects`, budget latch, gauge warn) has a direct unit test; routes have integration tests. Keeps the ≥80% gate — the new code is mostly pure/tested; the thin `server.ts` wiring is exercised by the integration tests.
|
||||
|
||||
---
|
||||
|
||||
## Edge cases & failure modes
|
||||
|
||||
- **(a)** No upstream (`@{u}` fatal) → catch → `ahead/behind` undefined; chip hidden. Detached HEAD → `git log -1 --format=%ct` still works (lastCommitMs set), `@{u}` fails (sync hidden). Empty repo (no commits) → both git calls fail → all undefined. Slow git → 2s timeout kill (reuse `GIT_STATUS_TIMEOUT_MS`, `projects.ts:36`). Adds ≤2 spawns/repo bounded by `GIT_CONCURRENCY=8`; gated off entirely when `projectDirtyCheck=false`. `ahead=behind=0` (in sync) → chip omitted (only render when >0).
|
||||
- **(b)** `costUsd` undefined in a telemetry frame → skip guard (no crossing). Budget `0` → disabled. Latch persists across statusLine frames; a **new** session gets its own latch (per-`Session`). DND/`notifyDone` interplay: `'budget'` is neither `'done'` nor gated, so `shouldSend` sends it unless global DND is on (matches stuck). Late-joining device: gets current telemetry via `manager.ts:139-140`, derives warn from `/config/ui` budget — no missed styling. Push disabled (no VAPID) → latch still flips, broadcast still happens, just no push (graceful, like stuck).
|
||||
- **(c)** `since` absent/NaN/negative → `0`. No sessions → all-zero `DigestResult` (banner suppressed client-side). Clock skew / `lastOutputAt` in future → still counted as finished if `idle` (acceptable; coarse "what happened" view). First-ever visit (no localStorage) → `since=0`, banner may list everything → set last-seen to `generatedAt` after first render so it doesn't re-nag.
|
||||
- **(d)** Binary/huge subjects: `maxBuffer` cap + subject truncation. Non-repo/deleted path → 404 (isValidGitDir). Repo with 0 commits → `[]`. Merge commits/unusual chars in subject → US-field + NUL-record delimiters immune to embedded whitespace. `n` non-numeric → clamp to default.
|
||||
|
||||
---
|
||||
|
||||
## Security
|
||||
|
||||
- **Path containment:** `/projects/log` reuses `isValidGitDir` (`server.ts:123`) — absolute + isDirectory + has `.git` (SEC-H7 three-prong), identical to `/projects/diff`. `readSync` runs only against paths already discovered by the bounded BFS scan (`scanRepos`, symlink/dotdir/`node_modules`-skipping, `projects.ts:148`).
|
||||
- **No shell, ever:** all git calls use `execFile('git', [...])` with `timeout` + `maxBuffer` (mirrors `readDirty` line 100). `repoPath` is passed as `cwd`, never interpolated into argv. `n` is coerced to an int and clamped before reaching argv.
|
||||
- **Output is untrusted:** commit subjects and digest labels rendered via `textContent` only (SEC-H5, as in `preview-grid.ts` and `projects.ts` worktree/CLAUDE.md rendering) — zero `innerHTML`. FE `normalizeProject`-style narrowing for the new numeric fields (drop non-numbers) and for `/digest`/`/projects/log` responses (never trust the API shape).
|
||||
- **Origin/CSRF:** all four routes are **read-only GETs** → no Origin guard, consistent with `/projects`, `/live-sessions`, `/projects/diff`. `GET /config/ui` stays read-only. No state-changing surface is added, so no new `requireAllowedOrigin` / rate-limit needed. (The budget **push** goes out the existing `pushService` seam — no new inbound route.)
|
||||
- **Secrets:** cost budget is a non-secret number; `costBudgetUsd` is safe to expose in `/config/ui`. Push payload for `'budget'` carries only sessionId + cwd-basename label (no cost figure, no terminal bytes) via the existing `buildPayload` (`push-service.ts:88`) — SEC-C5 byte-shuttle boundary preserved.
|
||||
- **DoS:** sync adds bounded spawns (concurrency 8, 2s timeout, gated by `projectDirtyCheck`); `/projects/log` `n` clamped ≤50; `/digest` is O(sessions) over an already-capped table.
|
||||
|
||||
---
|
||||
|
||||
## Effort & dependencies
|
||||
|
||||
**~3–4 dev-days total** (four independent slices; can be parceled to parallel builders after the shared `src/types.ts` edit lands):
|
||||
|
||||
| Sub | Effort | Notes |
|
||||
|---|---|---|
|
||||
| (a) sync chip | ~0.5–0.75d | Backend `readSync` + fold-in + 2 FE renders. |
|
||||
| (b) cost guard | ~0.75d | Config float parser, manager latch, gauge warn, `/config/ui` + tabs.ts wiring. Highest test surface. |
|
||||
| (c) reconnect digest | ~1d | New pure aggregate + route + new FE banner module + main.ts mount + localStorage last-seen. |
|
||||
| (d) recent commits | ~1d | New backend git-log module + route + new FE render module + detail-section wiring. |
|
||||
|
||||
**Dependencies (into these):** all four build only on already-shipped infra — `buildProjects` cache pass (v0.6), `NotifyService` DI + `stuckNotified` latch (A5/A1), `renderTelemetryGauge` (B2), `isValidGitDir` + `getDiff` pattern (B1), `/config/ui` seam (review #4). No dependency on other roadmap items.
|
||||
|
||||
**Unlocks / synergy:** (d)'s `getGitLog` + NUL-parsing helper and (a)'s upstream detection are reusable by **#9 "diff against a base branch"** (upstream/`@{u}` resolution) and **#10 "PR + CI status chip"** (a per-repo git/`gh` metadata pass can fold into the same `runDiscovery` concurrency slot as the sync chip). (c)'s `buildDigest` gives **#8 idle-queued follow-up** a ready read-side "which sessions are idle/waiting" aggregate. `NotifyClass 'budget'` establishes the pattern for any future threshold alerts.
|
||||
135
docs/plans/w4-commit-push.md
Normal file
135
docs/plans/w4-commit-push.md
Normal file
@@ -0,0 +1,135 @@
|
||||
# Stage / commit / push from the diff viewer
|
||||
|
||||
`id: w4-commit-push` — the highest-risk git **write** set. Scope is bounded to the MVP the task defines: **per-file stage/unstage toggle + commit + push-current-branch**. Discard / checkout / restore-working-tree are explicitly **deferred** (they are the only destructive-to-working-tree ops; nothing in this feature ever touches file contents on disk).
|
||||
|
||||
This mirrors the existing read-only diff channel (`src/http/diff.ts`) and the one existing git-write channel (`src/http/worktrees.ts` + the `POST /projects/worktree` route at `src/server.ts:699–725`). The server stays a byte-shuttle; these are out-of-band side-channel routes.
|
||||
|
||||
---
|
||||
|
||||
## Contract
|
||||
|
||||
### New routes (all in `src/server.ts`, all `express.json`, all `requireAllowedOrigin` + `isValidGitDir`)
|
||||
|
||||
Every route: `requireAllowedOrigin(req, res)` (CSRF, `src/server.ts:352`) → `gitOpsEnabled` gate (403 if off) → per-IP rate limit → validate `path` with the in-file `isValidGitDir` (three-prong: absolute + dir + `.git`, `src/server.ts:123`) → delegate to `src/http/git-ops.ts`. The delegate re-validates and **realpath-contains** every path (defense in depth — the route’s `isValidGitDir` does not follow symlinks).
|
||||
|
||||
| Method + path | Body | Success | Notes |
|
||||
|---|---|---|---|
|
||||
| `POST /projects/git/stage` | `{ path: string, files: string[], stage?: boolean }` | `200 {ok:true, staged:boolean, count:number}` | `stage` default `true` → `git add`; `false` → unstage (`git restore --staged`). `stage` is an **addition** to the task’s bare `{path,files[]}` so one endpoint serves the toggle in both directions. |
|
||||
| `POST /projects/git/commit` | `{ path: string, message: string }` | `200 {ok:true, commit:string}` (short SHA) | Commits **staged** changes only. `message` capped at `commitMsgMaxLen`. |
|
||||
| `POST /projects/git/push` | `{ path: string }` | `200 {ok:true, branch:string, remote:string}` | Pushes the **current branch** to its existing upstream, else `-u <sole-remote> <branch>`. Remote/branch are **derived server-side**, never taken from the client. |
|
||||
|
||||
Error shape (mirrors the worktree route at `src/server.ts:724`): `res.status(result.status).json({ error: result.error })` where `error` is a **safe** message (never raw git stderr, SEC-M10 — see `classifyWorktreeError`, `src/http/worktrees.ts:193`).
|
||||
|
||||
Status codes: `400` invalid input / detached HEAD / no remote / identity unset; `403` disabled or bad Origin; `404` not a git dir; `409` nothing staged / non-fast-forward / index.lock held; `401` push auth required; `429` rate-limited; `500` unclassified; `200` success.
|
||||
|
||||
### `src/types.ts` (coordination edit — frozen shared-contract source)
|
||||
|
||||
Add, next to `CreateWorktreeResult` (`src/types.ts:485`):
|
||||
|
||||
```
|
||||
export interface GitOpResult {
|
||||
ok: boolean;
|
||||
status?: number; // HTTP status on failure
|
||||
error?: string; // SAFE message only (never raw git stderr)
|
||||
// success payloads (route-specific, all optional):
|
||||
staged?: boolean; // stage: direction applied
|
||||
count?: number; // stage: files affected
|
||||
commit?: string; // commit: short SHA
|
||||
branch?: string; // push: branch pushed
|
||||
remote?: string; // push: remote pushed to
|
||||
}
|
||||
```
|
||||
|
||||
One interface keeps the coordination churn minimal (like `CreateWorktreeResult`). The FE narrows with an `isGitOpResult` guard mirroring `isWorktreeResult` (`public/projects.ts:545`).
|
||||
|
||||
### `src/config.ts` env vars (4 new, mirroring the B3 worktree group at `src/config.ts:371–378`)
|
||||
|
||||
| Env | Field | Default | Purpose |
|
||||
|---|---|---|---|
|
||||
| `GIT_OPS_ENABLED` | `gitOpsEnabled: boolean` | `true` | Master kill-switch (mirrors `worktreeEnabled`, `src/config.ts:372`). Off → all three routes 403. |
|
||||
| `GIT_OPS_TIMEOUT_MS` | `gitOpsTimeoutMs: number` | `10_000` | stage/commit exec timeout (mirrors `DEFAULT_WORKTREE_TIMEOUT_MS`, `src/config.ts:65`). |
|
||||
| `GIT_PUSH_TIMEOUT_MS` | `gitPushTimeoutMs: number` | `120_000` | push is network-bound → longer bound. |
|
||||
| `COMMIT_MSG_MAX_LEN` | `commitMsgMaxLen: number` | `5_000` | commit-message length cap. |
|
||||
|
||||
Parsed with the existing `parseBool` / `parseNonNegativeInt` helpers and appended to the frozen object (`src/config.ts:413–438`). File-count cap for `stage` reuses the existing `diffMaxFiles` (`src/config.ts:358`, default 300) — no new var. `src/types.ts` `Config` interface gains the 4 fields (same coordination note the file already carries at `src/config.ts:15`).
|
||||
|
||||
---
|
||||
|
||||
## Files to change
|
||||
|
||||
| Path | Change |
|
||||
|---|---|
|
||||
| **`src/http/git-ops.ts`** (new) | The whole write engine. Mirrors `diff.ts`/`worktrees.ts` execFile pattern: no shell, `timeout`+`maxBuffer`, `--` terminator, `env:{...process.env, GIT_TERMINAL_PROMPT:'0'}`. Exports (all never-throw, return `GitOpResult`): `stageFiles(repoPath, files, stage, opts)`, `commit(repoPath, message, opts)`, `push(repoPath, opts)`, plus pure `classifyGitError(stderr): {status,error}` and `validateRepoFiles(repoRealPath, files): string[] \| null`. |
|
||||
| **`src/http/git-path.ts`** (new, small) | Extracts the containment helper so it isn’t duplicated: `resolveRealPath(target)` (copy of `worktrees.ts:118`) + `isContained(realBase, realCandidate): boolean`. Used by `git-ops.ts`. (Optional follow-up: refactor `worktrees.ts:141` `computeWorktreeDir` onto it — **deferred**, out of this task’s lane.) |
|
||||
| **`src/types.ts`** | Coordination edit: add `GitOpResult` (above) + 4 `Config` fields. |
|
||||
| **`src/config.ts`** | Parse the 4 new env vars; append to the frozen config object. |
|
||||
| **`src/server.ts`** | Register the 3 routes after the worktree route (`:725`). Add two module-level rate constants + two limiters via `createRateLimiter` (`:109`). Reuse in-file `isValidGitDir`, `requireAllowedOrigin`, `sanitizeForLog` (`:162`) for the commit/push audit log. Import from `./http/git-ops.js`. |
|
||||
| **`public/diff.ts`** | `mountDiffViewer` (`:253`) gains: per-file **Stage/Unstage** toggle button in each file row, and a bottom **commit/push bar** (message `<textarea>` + Commit + Push). Add optional `onToggleStage` to the render path; add POST helpers `postStage/postCommit/postPush`. All text via `textContent`/`el()` (SEC-H4, `:139` note). Re-`loadDiff()` after any successful write. |
|
||||
| **`public/projects.ts`** | No logic change required — `buildDiffSection` (`:612`) already mounts `mountDiffViewer` and gets the new UI for free. Only add CSS-class hooks if styling inline. |
|
||||
| **`public/style.css`** (or wherever `df-*` classes live) | Styles for `df-file-stage`, `df-commitbar`, `df-commit-msg`, `df-commit-btn`, `df-push-btn`, `df-op-error`, `df-op-busy`. |
|
||||
|
||||
---
|
||||
|
||||
## TDD steps (ordered)
|
||||
|
||||
Backend uses **node env + real throwaway git repos** in `os.tmpdir()` exactly like `test/http/diff.test.ts:277` and `test/http/worktrees-create.test.ts:42`. FE uses **jsdom + mocked `fetch`** like `test/diff.test.ts`.
|
||||
|
||||
**Pure classifier first (fast, deterministic):**
|
||||
1. `test/http/git-ops.test.ts` → `describe('classifyGitError')`: feed canned stderr strings, assert `{status,error}` and that `error` never contains `fatal:` (SEC-M10, mirror `worktrees-create.test.ts:193`). Cases: `nothing to commit`→409; `Please tell me who you are`→400; `! [rejected]`/`non-fast-forward`/`fetch first`→409; `could not read from Username`/`Authentication failed`/`terminal prompts disabled`/`Permission denied (publickey)`→401; `index.lock`→409; unknown→500. → then implement `classifyGitError`.
|
||||
2. `describe('validateRepoFiles')`: rejects `[]`, absolute paths, leading-`-`, `../escape`, > `diffMaxFiles` entries; accepts in-repo relative paths (incl. a **deleted** file whose path no longer exists on disk — `resolveRealPath` resolves the existing prefix). Symlink-escape: pre-plant a symlink inside the repo pointing outside, assert rejection (mirror `worktrees-create.test.ts:137`). → implement `validateRepoFiles` on `git-path.ts`.
|
||||
|
||||
**Integration against real repos:**
|
||||
3. `describe('stageFiles')`: init repo + commit; modify a tracked file + add an untracked file; `stageFiles(repo,[file],true)` → assert `git diff --cached --name-only` lists them; `stage:false` → assert unstaged. Assert non-git path → `{ok:false,status:404}`. → implement `stageFiles`.
|
||||
4. `describe('commit')`: stage a change → `commit(repo,'msg')` → `{ok:true, commit}` and `git log -1 --format=%H` starts with it. Empty index → `{ok:false,status:409}`. Repo with `user.name` unset → 400. Message length > cap → 400 (route-level; test the length guard where it lives). → implement `commit`.
|
||||
5. `describe('push')`: create a **bare** repo as `origin` (`git init --bare`), `git remote add origin`, first push sets upstream via `-u`; assert bare repo received the ref (`git --git-dir=<bare> rev-parse <branch>`). Detached HEAD → 400. Zero remotes → 400. Two remotes + no upstream → 409. Second push (upstream now set) → plain `git push`. → implement `push`.
|
||||
|
||||
**Config + route wiring:**
|
||||
6. `test/config.test.ts`: the 4 new vars default correctly and parse/validate (reuse existing patterns).
|
||||
7. `test/integration/server.test.ts` (extend, using the `startServer` + `fetch` + `Origin` pattern at `:783`): for each of the 3 routes — foreign Origin → 403, **no** Origin → 403 (default-deny, mirror `:778`), non-git path → 404, missing field → 400, `gitOpsEnabled:false` → 403, and a happy-path 200 against a temp repo (push against a temp bare remote). Confirm `error` bodies carry no `fatal:`.
|
||||
|
||||
**Frontend (jsdom):**
|
||||
8. `test/diff.test.ts` (extend): stub `global.fetch`. Assert each file row renders a Stage button on the Working view and Unstage on the Staged view; clicking POSTs to `/projects/git/stage` with the right `{path,files,stage}` body and re-fetches. Assert the commit bar disables Commit when the message is empty, POSTs `/projects/git/commit`, and shows a failure via `textContent` (SEC-H4 — assert zero `innerHTML`, mirror the file’s existing security assertions). Assert Push POSTs `/projects/git/push` and reflects busy/disabled state. → implement the `mountDiffViewer` changes to pass.
|
||||
|
||||
Coverage: the pure `classifyGitError`/`validateRepoFiles` + real-repo integration cover `git-ops.ts` branches; steps 7–8 cover the new server + `diff.ts` lines. Keeps the 80% gate.
|
||||
|
||||
---
|
||||
|
||||
## Edge cases & failure modes
|
||||
|
||||
- **Nothing staged → commit**: git exits non-zero (`nothing to commit`) → 409 "Nothing staged to commit." (not a 500).
|
||||
- **Author identity unset**: `commit` fails (`Please tell me who you are`) → 400 with a safe hint. Do **not** auto-inject a fake identity.
|
||||
- **Unborn HEAD (no commits yet)**: unstage via `git restore --staged` errors → classify to a safe 409/400; commit still works (creates the first commit). Documented limitation.
|
||||
- **Detached HEAD on push**: `git rev-parse --abbrev-ref HEAD` → `HEAD` → refuse 400 "Cannot push a detached HEAD."
|
||||
- **No upstream, one remote**: `git push -u <remote> <branch>`. **No upstream, ≥2 remotes**: 409 "Set an upstream first." **Zero remotes**: 400.
|
||||
- **Non-fast-forward / remote ahead**: `! [rejected]` → 409 "Push rejected — pull/rebase first." (never force-push).
|
||||
- **Credential prompt hang**: `GIT_TERMINAL_PROMPT=0` (+ optional `GIT_SSH_COMMAND='ssh -o BatchMode=yes'`) makes auth fail fast → 401 instead of hanging; the exec `timeout` is the backstop.
|
||||
- **`index.lock` held (concurrent op)**: → 409 "Another git operation is in progress." (Optional hardening: a per-realpath in-memory promise-chain mutex in `server.ts` to serialize writes; git’s own lock is the correctness backstop, so this is MEDIUM, not required for MVP.)
|
||||
- **Deleted file staged**: path absent on disk → `resolveRealPath` resolves the existing prefix and re-appends the basename; `git add -- <path>` records the deletion. **Renamed file**: FE sends both `oldPath` and `newPath` so the deletion+addition both stage.
|
||||
- **Huge `files[]`**: capped at `diffMaxFiles` → 400.
|
||||
- **maxBuffer overflow / timeout**: caught, returned as a safe 500 (never a thrown rejection — house style, `diff.ts:302`).
|
||||
- **Commit message with newlines / leading `-` / emoji**: safe — passed as the single value of `-m` via argv (no shell), length-capped, never a pathspec.
|
||||
- **Write succeeds but re-fetch fails**: FE shows the diff error state but the write already happened; surface a non-blocking notice.
|
||||
|
||||
---
|
||||
|
||||
## Security
|
||||
|
||||
- **Origin/CSRF**: all 3 routes call `requireAllowedOrigin` (`src/server.ts:352`) — this is a **write** channel, unlike read-only `/projects/diff` which has no guard. Tested for foreign-Origin **and** missing-Origin (default-deny).
|
||||
- **No shell, ever**: `execFile('git', argv, …)` only — mirrors `diff.ts:296` / `worktrees.ts:242`. Untrusted strings (message, file paths) are argv elements, never interpolated. `--` terminates options before any pathspec so a path can’t become a flag; file paths additionally rejected if they start with `-`.
|
||||
- **Path containment (highest-risk)**: `isValidGitDir` at the route (absolute+dir+`.git`) **plus** `realpath`-based containment of the repo and of **every** `files[]` entry inside `git-ops.ts` (`resolveRealPath` + `startsWith(realBase+sep)`, the M2 pattern from `worktrees.ts:141`). Defeats `../` traversal and pre-planted-symlink escapes even though the diff route only does the lighter three-prong check.
|
||||
- **Server-derived remote/branch**: push never accepts a remote or refspec from the client — both are read back from the repo — eliminating arg-injection and push-to-arbitrary-URL risk.
|
||||
- **Safe error classification**: `classifyGitError` maps stderr substrings to safe messages; raw git stderr is never returned (SEC-M10). Tested that responses contain no `fatal:`.
|
||||
- **Rate limiting**: reuse `createRateLimiter` (`src/server.ts:109`, 60 s window) — `gitWriteLimiter` (stage+commit) ≈ 30/min/IP, `gitPushLimiter` ≈ 6/min/IP; over-limit → 429. Fixed policy constants beside the existing ones (`src/server.ts:74–76`).
|
||||
- **Audit log**: commit/push log actor+path via `sanitizeForLog` (`src/server.ts:162`, control-char strip + truncate), like the worktree audit at `:714`.
|
||||
- **Kill-switch**: `gitOpsEnabled=false` disables all three (mirrors `worktreeEnabled`, `src/server.ts:701`) for locked-down deployments.
|
||||
- **Body size**: `express.json({ limit: '4kb' })` for commit/push, `'64kb'` for stage (large `files[]`) — matching the existing routes’ caps.
|
||||
- **FE injection**: all rendered git output/errors via `textContent`/`el()` — zero `innerHTML` (SEC-H4, asserted in `test/diff.test.ts`).
|
||||
|
||||
---
|
||||
|
||||
## Effort & dependencies
|
||||
|
||||
- **Estimate**: ~3–4 days. Backend `git-ops.ts` + classifier + containment ≈ 1.5 d; route wiring + config + integration tests ≈ 0.75 d; FE toggles + commit/push bar + jsdom tests ≈ 1 d; polish/edge-cases ≈ 0.5 d.
|
||||
- **Depends on**: the existing B1 diff channel (`src/http/diff.ts`, `public/diff.ts:253` `mountDiffViewer`) and B3 worktree channel (`src/http/worktrees.ts`) — both shipped; this reuses their execFile + realpath-containment + error-classification patterns directly. No new dependency on other roadmap items.
|
||||
- **Shares infra with** `w4-worktree-remove` (task #12): both are git-write routes under `requireAllowedOrigin` + `classifyGitError`; landing the shared `git-path.ts` + `classifyGitError` here de-risks that one. Extractable-later: a generic `runGitWrite()` wrapper could later back `worktrees.ts` too (deferred — out of lane).
|
||||
- **Unlocks**: an in-browser commit loop for the walk-away workflow (stage → commit → push from a phone), and a natural home for a future "recent commits" chip (task #11).
|
||||
147
docs/plans/w4-worktree-lifecycle.md
Normal file
147
docs/plans/w4-worktree-lifecycle.md
Normal file
@@ -0,0 +1,147 @@
|
||||
# Worktree remove / prune
|
||||
|
||||
Delete losing worktrees and prune stale ones from any device. Backend adds `removeWorktree`/`pruneWorktrees` to `src/http/worktrees.ts` (same execFile-no-shell + timeout + containment machinery as the shipped `createWorktree`), two Origin-guarded routes in `src/server.ts`, and the FE makes the already-rendered `locked`/`prunable`/main tags in `public/projects.ts` `makeWorktreeRow` (lines 492–502) actionable.
|
||||
|
||||
## Contract
|
||||
|
||||
### New backend functions — `src/http/worktrees.ts`
|
||||
|
||||
Reuse the private helpers already in the file: `isGitRepo` (line 168), `listWorktrees`/`parseWorktrees` (lines 29, 69), `extractStderr` (line 180), `resolveRealPath` (line 118). Match `createWorktree`'s structured-result-never-throws contract (line 219).
|
||||
|
||||
```ts
|
||||
export interface RemoveWorktreeOptions { readonly force?: boolean; readonly timeoutMs: number }
|
||||
export async function removeWorktree(
|
||||
repoPath: string, targetPath: string, opts: RemoveWorktreeOptions,
|
||||
): Promise<RemoveWorktreeResult>
|
||||
|
||||
export interface PruneWorktreesOptions { readonly timeoutMs: number }
|
||||
export async function pruneWorktrees(
|
||||
repoPath: string, opts: PruneWorktreesOptions,
|
||||
): Promise<PruneWorktreesResult>
|
||||
```
|
||||
|
||||
`removeWorktree` algorithm (the security spine):
|
||||
1. `isGitRepo(repoPath)` false → `{ ok:false, status:404, error:'Not a git repository.' }`.
|
||||
2. `typeof targetPath !== 'string'` or empty → `{ ok:false, status:400, error:'Worktree path is required.' }`.
|
||||
3. `listWorktrees(repoPath)` → find the entry whose **realpath equals** `resolveRealPath(targetPath)` (canonical compare, not raw-string compare, to defeat symlink tricks). No match → `{ ok:false, status:404, error:'That path is not a worktree of this repository.' }`.
|
||||
4. Matched entry `.isMain === true` → `{ ok:false, status:400, error:'Cannot remove the main worktree.' }`.
|
||||
5. Matched entry `.locked` → `{ ok:false, status:409, error:'This worktree is locked; unlock it in a terminal first.' }` (never double-force `-f -f`).
|
||||
6. Run `git worktree remove` **with the canonical path from git's own list entry** (`match.path`), never the raw user string: `['worktree','remove', ...(force?['--force']:[]), '--', match.path]` via `execFileAsync` (cwd `repoPath`, `timeout: opts.timeoutMs`, `maxBuffer: WORKTREE_MAX_BUFFER`). On success `{ ok:true, path: match.path }`.
|
||||
7. On error → `classifyRemoveError(err)` (new; sibling of `classifyWorktreeError` line 193): stderr containing `contains modified or untracked files` / `use --force` / `is dirty` → `{ ok:false, status:409, error:'Worktree has uncommitted changes — force required.' }`; `not a working tree`/`is not a working tree` → `{ ok:false, status:404, ... }`; else `{ ok:false, status:500, error:'Failed to remove the worktree.' }`. Never leak raw stderr (SEC-M10).
|
||||
|
||||
`pruneWorktrees` algorithm: `isGitRepo` gate (404), then `git worktree prune -v` via execFile (timeout/maxBuffer). Parse verbose lines (`Removing worktrees/<name>: <reason>`, emitted on stdout/stderr — capture both) into `pruned: string[]` (best-effort; empty when nothing prunable — idempotent). Error → `{ ok:false, status:500, error:'Failed to prune worktrees.' }`.
|
||||
|
||||
### Changed message types — `src/types.ts` (coordination edit)
|
||||
|
||||
Add next to `CreateWorktreeResult` (line 485):
|
||||
|
||||
```ts
|
||||
export interface RemoveWorktreeResult { ok: boolean; path?: string; status?: number; error?: string }
|
||||
export interface PruneWorktreesResult { ok: boolean; pruned?: string[]; status?: number; error?: string }
|
||||
```
|
||||
|
||||
`WorktreeInfo` (line 290), `Config` worktree fields (lines 67–69) unchanged. Option interfaces stay local to `worktrees.ts` (mirrors `CreateWorktreeOptions`, line 207).
|
||||
|
||||
### New routes — `src/server.ts`
|
||||
|
||||
Insert both immediately after the create route (ends line 725), reusing `requireAllowedOrigin` (line 352), `sanitizeForLog` (line 162), `cfg.worktreeEnabled`, `cfg.worktreeTimeoutMs`. Add `removeWorktree, pruneWorktrees` to the import at line 43.
|
||||
|
||||
| Route | Body / gate | Response |
|
||||
|---|---|---|
|
||||
| `DELETE /projects/worktree` | `express.json({limit:'4kb'})`; `{ path, worktreePath, force? }`. Guard order: `requireAllowedOrigin` → `worktreeEnabled` (403) → `path`+`worktreePath` present (400). Audit-log via `sanitizeForLog`. | `result.ok` → `200 {ok:true,path}`; else `result.status ?? 500` + `{error}` |
|
||||
| `POST /projects/worktree/prune` | `express.json({limit:'4kb'})`; `{ path }`. Same guard order (path required → 400). | `200 {ok:true,pruned}` or `result.status` + `{error}` |
|
||||
|
||||
`force` coerced strictly: `const force = body['force'] === true`.
|
||||
|
||||
### Env vars — `src/config.ts`
|
||||
|
||||
**None new.** Reuse `worktreeEnabled` (line 372, gates all worktree writes), `worktreeTimeoutMs` (line 374) for remove/prune timeouts.
|
||||
|
||||
### FE — `public/projects.ts`
|
||||
|
||||
- New module-level fetch helpers (siblings of `killSession`, line 275): `removeWorktreeReq(repoPath, worktreePath, force)` → `DELETE /projects/worktree` with JSON body, returns `{ok, status, error}`; `pruneWorktreesReq(repoPath)` → `POST /projects/worktree/prune`, returns `{ok, pruned?, error?}`. Both same-origin (Origin guard passes), best-effort catch.
|
||||
- `makeWorktreeRow` (line 492) gains an optional `actions?: { onRemove:(wt)=>void }` param. When `actions` present and **not** `wt.isMain` and **not** `wt.locked`: append a `proj-wt-remove` `✕` button (`aria-label` "Remove worktree", `title` "Remove this worktree"). Locked rows keep the `locked` tag but no remove button (tooltip explains). Prunable rows still get remove.
|
||||
- `DetailCallbacks` (line 666) gains `onRemoveWorktree:(worktreePath:string)=>void` and `onPruneWorktrees:()=>void`. `renderProjectDetail` (line 672) threads `actions` into the worktree-list loop (line 721) and, when `detail.worktrees.some(w=>w.prunable)`, renders a section-level `Prune stale worktrees` button beside the "Worktrees" title (line 712) wired to `cb.onPruneWorktrees`.
|
||||
- `mountProjects` (line 820) implements the confirm→force→refresh flow (encapsulated here, like `killAndRefresh` line 897), passed into `renderDetail` (line 959):
|
||||
- `onRemoveWorktree`: `confirm("Remove worktree at <path>? This deletes the working tree.")` → `removeWorktreeReq(detailPath, wtPath, false)`; on `409` → second `confirm("Uncommitted changes will be lost. Force-remove?")` → retry with `force:true`; on other failure show error via `textContent` (never innerHTML, SEC-L3/H6); then `void refresh()`.
|
||||
- `onPruneWorktrees`: `confirm("Prune worktrees whose folders are gone?")` → `pruneWorktreesReq(detailPath)` → `refresh()`.
|
||||
- `public/diff.ts`: **no change** — `grep` confirms it renders no worktree rows/tags; the task's mention is covered entirely by `projects.ts`. (Note this deviation from the task wording in the log.)
|
||||
|
||||
## Files to change
|
||||
|
||||
| Path | Change |
|
||||
|---|---|
|
||||
| `src/types.ts` | **Coordination edit:** add `RemoveWorktreeResult`, `PruneWorktreesResult` after line 491. |
|
||||
| `src/http/worktrees.ts` | Add `removeWorktree`, `pruneWorktrees`, `RemoveWorktreeOptions`, `PruneWorktreesOptions`, `classifyRemoveError`; reuse existing `isGitRepo`/`listWorktrees`/`resolveRealPath`/`extractStderr`. |
|
||||
| `src/server.ts` | Import (line 43) + two routes after line 725 (`DELETE /projects/worktree`, `POST /projects/worktree/prune`). |
|
||||
| `public/projects.ts` | `removeWorktreeReq`/`pruneWorktreesReq` helpers; extend `makeWorktreeRow` (492), `DetailCallbacks` (666), `renderProjectDetail` (672), `mountProjects` (820). |
|
||||
| `test/http/worktrees-remove.test.ts` | **New** — node, real temp repos; unit + integration of the two functions. |
|
||||
| `test/integration/worktree.test.ts` | Extend with `DELETE`/prune route cases. |
|
||||
| `test/worktree-form.test.ts` | **New describe blocks** — jsdom FE row/confirm/prune tests. |
|
||||
|
||||
## TDD steps (ordered)
|
||||
|
||||
Backend pure/logic — `test/http/worktrees-remove.test.ts` (node env, mirror `worktrees-create.test.ts`: `makeRepo()` helper, `gitAvailable` guard, real temp repos, no network):
|
||||
|
||||
1. Test `removeWorktree` returns `{ok:false,status:404}` for a non-git dir → implement `isGitRepo` gate.
|
||||
2. Test empty/missing `targetPath` → `{ok:false,status:400}` → implement guard.
|
||||
3. Test a path not in `git worktree list` → `{ok:false,status:404}` → implement realpath-match against `listWorktrees`.
|
||||
4. Test main worktree (the repo root) → `{ok:false,status:400,error:/main/}` → implement `isMain` reject.
|
||||
5. Test happy path: create a worktree via `createWorktree`, then `removeWorktree` (clean tree) → `{ok:true}`; assert `git worktree list` no longer contains it.
|
||||
6. Test dirty worktree (write an untracked file into it) → `removeWorktree(force:false)` → `{ok:false,status:409}`; then `force:true` → `{ok:true}` → implement `classifyRemoveError` + force arg.
|
||||
7. Test error message never contains `fatal:`/`error:` (SEC-M10) → assert on the 409 case.
|
||||
8. Test locked worktree (`git worktree lock`) → `{ok:false,status:409,error:/locked/}` → implement locked reject.
|
||||
9. Test symlink alias: pass a symlink pointing at a real worktree as `targetPath` → still matches via realpath and removes git's canonical path → verify (M2-style containment).
|
||||
10. Test `pruneWorktrees` on a repo with a manually-`rm -rf`'d worktree dir → `{ok:true, pruned:[...]}` length ≥1; on a clean repo → `{ok:true, pruned:[]}` (idempotent). Non-git → `{ok:false,status:404}`.
|
||||
|
||||
Backend route — extend `test/integration/worktree.test.ts` (reuse `spawnServer`, `makeRealRepo`, `itGit`):
|
||||
|
||||
11. `DELETE /projects/worktree` foreign Origin → 403; missing Origin → 403.
|
||||
12. `WORKTREE_ENABLED=0` → 403.
|
||||
13. Missing `worktreePath` → 400.
|
||||
14. Attempt to remove main worktree → 400.
|
||||
15. `itGit`: create worktree via `POST /projects/worktree`, then `DELETE` it (clean) → 200 `{ok:true}`; `git worktree list` no longer lists it.
|
||||
16. `itGit`: dirty worktree → DELETE without force → 409; with `force:true` → 200.
|
||||
17. `POST /projects/worktree/prune`: Origin 403, disabled 403, and `itGit` prune-after-manual-delete → 200 with `pruned`.
|
||||
|
||||
FE — `test/worktree-form.test.ts` (jsdom; extend, reuse `makeDetail`, `makeHooks`, `makeCbs`, stubbed `fetch`):
|
||||
|
||||
18. `makeWorktreeRow` for a non-main non-locked wt with `actions` → contains a `.proj-wt-remove` button; main and locked rows → no button.
|
||||
19. `renderProjectDetail` with a prunable worktree → a `Prune stale worktrees` button exists; without → absent.
|
||||
20. Clicking Remove: stub `window.confirm=()=>true`, stub `fetch` → `{ok:true}`; assert `fetch` called `DELETE /projects/worktree` with `force:false` in body.
|
||||
21. Dirty retry: `fetch` first resolves `{ok:false,status:409}` then `{ok:true}`; `confirm` returns true twice → assert second `fetch` body has `force:true`.
|
||||
22. `confirm` returns false → assert `fetch` **not** called (no accidental deletion).
|
||||
23. Error render: `fetch` → `{ok:false,status:500,error:'boom'}` → error shown via `textContent` (assert `.textContent`, no HTML injection).
|
||||
|
||||
Run `npm test`; keep the 80% gate — the new functions and both routes carry direct + error-path coverage; FE branches (button presence, confirm true/false, force retry, error) are all exercised.
|
||||
|
||||
## Edge cases & failure modes
|
||||
|
||||
- **Main worktree** — always rejected (400); the repo root can never be deleted.
|
||||
- **Not-a-worktree path** — arbitrary FS path (e.g. `/etc`) never matches the list → 404, git never invoked against it.
|
||||
- **Dirty tree** (modified tracked or untracked files) — git refuses without `--force`; surfaced as 409 → explicit second confirm before force.
|
||||
- **Locked worktree** — 409 with a "unlock first" message; UI hides the remove button; never auto-escalate to `-f -f`.
|
||||
- **Removing the worktree you're viewing** (`isCurrent` but not `isMain`) — allowed; after refresh the detail path may 404 → detail shows "Project not found" (existing null branch, line 691). Acceptable.
|
||||
- **Already-removed / concurrent delete** — git errors "not a working tree" → 404 safe message; the follow-up `refresh()` reconciles the UI.
|
||||
- **Prune with nothing prunable** — `{ok:true, pruned:[]}`, no error (idempotent).
|
||||
- **git binary missing / timeout** — execFile rejects → 500 safe message; timeout bounded by `worktreeTimeoutMs`.
|
||||
- **Path with control chars / flag-like leading `-`** — never reaches argv as-is: we pass git's own canonical list path, and `--` terminates options; audit log runs through `sanitizeForLog`.
|
||||
- **DELETE-with-body stripped by an intermediary** — same-origin fetch, no proxy in the LAN threat model; body reliably delivered. (If ever a concern, mirror as query params — noted, not implemented.)
|
||||
|
||||
## Security
|
||||
|
||||
- **Origin/CSRF:** both routes call `requireAllowedOrigin` first (line 352) — destructive state change, mandatory (SEC-C3). Integration tests 11–12, 17 assert 403 for foreign/missing Origin.
|
||||
- **Feature gate:** `cfg.worktreeEnabled` (403 when off) governs remove/prune exactly as it governs create.
|
||||
- **No-shell exec:** `execFile('git', [...])` only; never a shell string. `timeout` + `maxBuffer` bound resource use.
|
||||
- **Path containment (the core defense):** the target is accepted **only** if its realpath matches an entry git itself reports in `worktree list`, and the command runs against **git's canonical path**, not the user string — so no traversal/symlink/arbitrary-path deletion is reachable (M2-consistent). `--` belt-and-suspenders before the path arg.
|
||||
- **Main-worktree protection:** `isMain` reject prevents deleting the repository itself.
|
||||
- **Safe error messages:** `classifyRemoveError`/prune mapping return fixed strings; raw git stderr never returned (SEC-M10) — asserted in test 7.
|
||||
- **FE injection:** error/label rendering uses `textContent` only (SEC-L3/H6), asserted in test 23.
|
||||
- **Destructive-intent confirmation:** browser `confirm()` before any delete, plus a **second** confirm before `force` on a dirty tree — no single-click data loss.
|
||||
- **Rate-limit:** inherits the app's per-connection posture; these are Origin-gated same-origin calls. (No new limiter added — consistent with the create route.)
|
||||
|
||||
## Effort & dependencies
|
||||
|
||||
- **Effort:** ~1.5–2 days (backend + routes ~0.75d, FE wiring + confirm flow ~0.5d, tests ~0.5d).
|
||||
- **Depends on:** W4 *create worktree* (shipped `createWorktree`, whose `isGitRepo`/`listWorktrees`/`resolveRealPath`/`classify*` are reused) and the v0.6 project-detail worktree list (`makeWorktreeRow`).
|
||||
- **Unlocks:** full worktree lifecycle from any device (create → work → **remove/prune losers**), and pairs naturally with W4 *stage/commit/push from the diff viewer* (issue #13) to close the "spin up a worktree, land the winner, delete the rest" loop.
|
||||
Reference in New Issue
Block a user