diff --git a/CHANGELOG.md b/CHANGELOG.md index 74b48aa..482c7a9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,7 +1,30 @@ # Changelog + + ## Unreleased +## v0.5.2 (2026-10-09) + +Release focused on terminal replay resumption after a mid-stream drop (issue #94), a position-independent `node --test` case-count guard (issue #95), a worktree-delete refusal that names the dirty state and offers a force exit (issue #102), and post-v0.5.1 UI/transport hardening (issues #98, #101). + +- **docs(terminal): discussion draft on terminal instance survival and restart reattach** — records a discussion (no decision, no code change) on what happens to pty terminal instances and their inner processes (e.g. opencode cli) when the myworktree daemon exits, and what it would take to keep them alive, traceable and re-attachable after a restart: the terminal dies with the daemon via PTY hangup, not an explicit kill — myworktree holds the pty master fd in the per-instance Handle (`internal/instance/pty/driver.go`) and `Shutdown` deliberately does not stop pty kinds (`internal/app/app.go`), so process exit closes the master and the kernel SIGHUPs the foreground process group and session leader, while nohup/setsid-escaped processes become untracked orphans (no `PDEATHSIG` anywhere). The framework's existing `RestartSurvivor`/Reattach path (`ReconcileRunningOnStartup`) is implemented by reasonix only, and pty is excluded because its in-memory stdin/stdout bindings and ring buffer cannot resume — the single necessary and sufficient condition to survive a restart is to move PTY master fd ownership out of myworktree (tmux with an isolated socket is the mature form; a self-built supervisor the alternative). The draft also records the honest boundaries of "uninterrupted", the long-term unreclaimed-risk list, and the open decision forks (scrollback depth, hard dependency vs fallback, Shutdown semantics, orphan reaping), and flags an adjacent pre-existing hole: dsh-web never implements Reattach, so a `kill -9` daemon leaves dsh processes orphaned on their ports. Discussion draft: `docs/TERMINAL_DETACH_REATTACH_DISCUSSION.md`. + +- **feat(terminal): the streaming handshake replay now announces its real START offset ahead of the first chunk, so a client whose connection dies mid-replay resumes from what it already painted instead of re-pulling the whole replay (issue #94)** — #87's streaming replay (`ttyHandshakeReplayBudget` = 8 MB, ~128 × 64 KB frames) widened the "replay painted, `sync` not yet arrived" death window from one ≤64 KB frame to the full budget: the client's `handshakeSynced` latch deliberately refused to advance the cursor frame-by-frame until the closing `sync`, because `RingBuffer.ReadSince` silently clamps a stale `since` to the oldest live byte and accumulating from the request could publish a cursor over bytes never delivered — so a drop mid-replay re-pulled everything from the old cursor, which under sustained network degradation does not converge. Fix (the issue's minimal form, landed one step cheaper): the catch-up loop now brackets the replay with TWO sync frames — `{"type":"sync","offset":S,"start":true}` ahead of the FIRST binary chunk, where S = the first chunk's `next` − len(body), so S and the chunk come from the SAME `ReadSince` call (the clamped oldest-live value whenever clamping happened, never the raw request; no new `Manager` surface), and the unchanged closing sync after the last chunk. The frame REUSES the `"sync"` type instead of adding a frame type plus a capability gate: any recipient provably parses `"sync"` already (the `caps=sync` token or the inferred explicit-`since` opt-in, issue #98 — and every catch-up request presents `since` by definition, so no catch-up client can be a pre-whitelist page), and the shipped client's existing handler — adopt the ABSOLUTE offset, lift the latch — is precisely the wanted semantics, so the client needed ZERO code changes and even a never-refreshed v0.5.1 page benefits from a new daemon. The `"start":true` marker exists for consumers that must know WHEN the replay ends (the test harness stops at the sync WITHOUT it and fails if a start frame ever arrives after a chunk); the UI deliberately ignores the field, treating both sync frames alike, and S + Σ(frame bytes) equals the closing offset unless the ring rolls a full cycle mid-replay and evicts bytes the loop had not sent yet — then a later chunk clamps to the new oldest live byte, the sum lands short, and the closing sync, the absolute authority, jumps the gap (those bytes are undeliverable anyway, the same rule the read→subscribe window already lives by). The beyond-head tail degrade (`since` past head — defensive) announces its tail's start the same way (S = head − len), so every replay addressed to a cursor-bearing client carries the frame; only the CURSOR-LESS first-connect tail (a single ≤64 KB frame whose client may not parse `sync` at all — that path's `syncCap` is a genuine gate, not an inference), `since=-2` (zero binary frames by design, the #86 contract untouched) and the caught-up-at-head consult exit still send the closing sync ALONE, and `completeHandshake` stays single-shot per connection. Pinned server-side by `TestHandleInstanceTTYWS_ReplayStartOffsetPublishesClampedStart` (a `since` below the oldest live byte publishes the CLAMPED start, never the raw request), `TestHandleInstanceTTYWS_MidReplayResumeContinuesFromRenderedBytes` (a drop after the start sync plus two chunks reconnects from start+painted and receives EXACTLY the un-rendered remainder), `TestHandleInstanceTTYWS_StartSyncOnlyForChunkedReplaysWithACursor` (the closing sync arrives ALONE only on the three paths with no cursor or no chunks: cursor-less tail / `-2` / caught-up), the extended 8 MB budget test (start 0 + delivered budget == closing offset; the re-request's start IS the published cursor) and the extended `TestHandleInstanceTTYWS_CursorAheadOfHeadFallsBackToTail` (the beyond-head degrade announces the tail's real start, S = head − len); client-side by the new issue-#94 `node --test` case inside the 68-case total below, which drives start sync → replay frames → a mid-replay drop → and pins the reconnect's `since` at the rendered cursor. Contract documented in `docs/API.md` (TTY WS) and `docs/ARCHITECTURE.md`. + +- **test(ui): the `node --test` case-count guard now checks EVERY occurrence of the coverage-claim phrase instead of the first match (issue #95)** — `TestTerminalStatusChangelogCount` located the claim with `FindSubmatch`, which returns the first "Pinned by N `node --test` cases" in the file; since entries are prepended in reverse chronological order, the anchor was positional — any future entry using the phrase and landing above the #80 entry would silently hijack the guard (a stale number above plus drift in the real claim would still pass, and entries quoting subset counts carried no constraint at all). Of the issue's two proposals this takes the stricter one: every occurrence of the phrase must equal the derived total (`grep -c '^test('`), making the claim position-independent — the phrase now means "the file's total case count" wherever it appears, and subset counts keep their distinct phrasing (e.g. #83's "fourteen new #83 cases"). Verified by injection: a stale claim above the anchor fails, and a correct claim above plus a corrupted #80 claim fails — the exact silent-failure mode the issue named. Pinned by `TestTerminalStatusChangelogCount`. + + +- **fix(worktree): the delete refusal now says WHAT is dirty, warns about the gitignored files force would destroy, and offers a force exit (issue #102)** — reported on v0.5.1-beta.3: deleting a worktree was refused with the flat "worktree has uncommitted or untracked changes; delete is refused", yet the user had confirmed the workspace clean beforehand — in fact 133 tracked source files (two whole top-level directories) had vanished BEFORE the delete attempt, and the honest-but-undetailed refusal presented a damaged workspace as an everyday "you have changes" prompt, with no force exit short of a manual `git worktree remove --force` the daemon never noticed (its state record survived). Three fixes, matching the issue's P1–P3 (the file disappearance itself was investigated in the issue and is NOT a myworktree bug): **(P1)** `Manager.Delete` is now `Delete(id, force)` and the refusal is a structured `DirtyWorktreeError` — counts by category (deleted/modified/untracked always, added/renamed when nonzero; exactly one bucket per porcelain line by delete > rename > add > modify priority, so the buckets always sum to the total), up to 5 first paths, and the FULL `git status --porcelain` output; the refusal wording is now e.g. "… refused: 133 entries (133 deleted, 0 modified, 0 untracked); first paths: firmware/CMakeLists.txt, …". **(P2)** the force exit exists everywhere the delete does: `POST /api/worktrees/delete` takes `force`, the MCP `worktree_delete` verb takes `force`, and the CLI grew `myworktree worktree delete --force `; force passes `--force` to `git worktree remove`. **(P3)** the refusal separately counts the gitignored entries (`git status --porcelain --ignored`, the "!!" lines the blocking check never sees — 89 gitignored files, 64 of them single-copy, would have silently vanished in the report) and warns that force destroys them unrecoverably. API: the refusal answers **409** with the contract code `worktree_dirty` + `message` (one-line summary for text clients) + the structured `dirty` breakdown. UI: `deleteWorktree` matches the code and opens a dialog with the summary, a collapsible full git status output, the gitignored-file danger warning, and a two-step-confirm Force delete button that resends with `force: true`. The issue's P4 side observation (state.json's recorded branch can lag a worktree-internal `git switch`) needed no change: `Manager.List` already re-resolves the live branch per worktree for display, and the stored value is only a fallback. Pinned by `TestDeleteDirtyRefusalCarriesDetails` / `TestDeleteForceRemovesDirtyWorktree` (real-git integration), `TestDirtyWorktreeErrorSummarizePorcelain` / `TestCountIgnoredPorcelain` / `TestDirtyWorktreeErrorMessage` (unit), `TestHandleWorktreeDeleteDirtyRefusalIsStructured` / `TestHandleWorktreeDeleteForceDeletes` (API contract), and `TestWorktreeDirtyDeleteDialog` (UI anchors). Contract documented in `docs/API.md`. Review follow-ups (two independent reviews, both verified locally before fixing): **(R1)** `myworktree worktree delete --force` silently dropped the flag — Go's `flag` parsing stops at the first positional — so the CLI now scans for `--force`/`-force` manually and rejects extra positionals with a usage error; **(R2)** `force` no longer gates on `git status` — the probe only builds the refusal, and on a damaged worktree (corrupt index) it fails while `git worktree remove --force` still succeeds, so force skips it entirely (pinned by `TestDeleteForceSkipsStatusProbe`); **(R3)** the MCP `worktree_delete` dirty refusal now answers the same 409 + `worktree_dirty` + structured `dirty` breakdown as the HTTP API (was a flat 400; pinned by `TestHandleMCPWorktreeDeleteDirtyRefusalIsStructured`); **(R4)** a worktree whose only at-risk contents are gitignored files still deletes without a refusal, but the delete now returns the destroyed gitignored count — surfaced as `ignored_destroyed` in the API/MCP success responses, a post-delete dashboard notice, and a CLI stderr note (pinned by `TestDeleteCleanWorktreeReportsIgnoredDestroyed` / `TestHandleWorktreeDeleteCleanReportsIgnoredDestroyed`); **(R5)** both status probes run with `-uall` so untracked/ignored directories count file-by-file instead of collapsing to one entry, and with `-c core.quotePath=false` so non-ASCII paths stay literal (pinned by `TestDeleteUntrackedDirCountsFilesIndividually` / `TestDeleteRefusalKeepsNonASCIIPathsLiteral`); the carried `porcelain` payload is capped at 200 lines with a truncation marker. UI nits from the same round: a failed force delete shows the body's human `message` instead of the raw `worktree_dirty` code, and Esc closing the dialog resets its dangling state via a `cancel` listener. Second review round (both reviews confirmed every first-round fix and found no new defect; their remaining nits): `ignored_destroyed` is now present ONLY when counted — a forced success omits the field rather than answering a 0 that read as "none destroyed" while meaning "not counted"; the CLI's hand-rolled scan also honors the `--force=` spelling and the `--` terminator, with a garbage value failing loudly (`TestRunWorktreeDeleteForceEqualsForms` / `_InvalidForceValueFails` / `_DashDashTerminator`); and the doc-comment/pluralization nits around the 200-line porcelain cap are fixed (`TestCapDirtyPorcelain`). +- **fix(terminal): the TEXT `{"type":"ping"}` heartbeat is now opt-in via a handshake capability, so stale pre-refresh pages no longer paint it into the terminal every 10 s (issue #98)** — reported on v0.5.1: SOME terminal tabs showed a literal `{"type":"ping"}` line every 10 s while others on the same daemon never did; refreshing an affected tab made it stop. The split was each page's LOAD TIME, not the instance: #83 had introduced the TEXT heartbeat and the client's `parseTTYControlMessage` whitelist in the same commit, so a tab opened on v0.5.0 (whose whitelist has only `ready`/`resize`) and never refreshed ran old JS against the v0.5.1 daemon, and every heartbeat text frame fell through to the paint-everything branch. The root cause is a design orientation, not the ping frame itself: any non-whitelisted text frame is terminal output, so EVERY future server control frame would leak the same way onto stale pages — and already-open tabs never re-fetch JS, so no Cache-Control header can reach them. Fix (the issue's preferred direction): the TEXT heartbeat now rides behind an explicit client capability. The tty/ws handshake accepts `caps=`; the server sends the `{"type":"ping"}` text frame ONLY when the list carries `ping` as a WHOLE token (`ttyClientHasCap` — `pinger` is not a match), and `connectTTY` declares `&caps=ping,sync` (its whitelist already handles `ping`/`pong`/`sync`). A client that does not opt in — the stale v0.5.0 page, any non-browser client — degrades to the pre-#83 behaviour it always had: the RFC 6455 ping still flows to everyone (`onmessage` never fires for control frames, so nothing is ever painted), only the observable text mirror is withheld. An old daemon ignores the unknown parameter, so the version mix is safe in both directions, and the client-probe `{"type":"ping"}` → `{"type":"pong"}` round trip is unchanged (a client that sends a probe understands the answer). The same leak class had a second, lower-rate carrier, closed in the same pass: the `{"type":"sync"}` offset echo (#87) post-dates the v0.5.0 whitelist too, so an un-refreshed v0.5.0 page painted it into the terminal once per connect; the echo now rides behind `caps=sync` OR an inferred opt-in — any client presenting an explicit `since` cursor receives it, because the cursor parameter and the sync whitelist shipped in the same fix (#87; v0.5.0's client never sends `since` on this endpoint at all, so presenting one provably implies parsing the other). The bare caps gate alone would have frozen every pre-caps v0.5.1 page's cursor — the `handshakeSynced` latch never sets without the echo — and re-replayed output on every reconnect until a manual refresh, neither bounded to a tick nor self-healing on the live paths (the poll's `ensureTerminalLiveTransport` deliberately never re-runs `loadLog`); review round 2 caught this before merge. The residual degradation shrinks to a pre-caps page whose cursor is still the unknown/zero sentinel: it sends no `since`, gets no echo, and its reconnects re-replay the ≤64KB tail until a loadLog re-pin or refresh lands a cursor — the pre-#87 behaviour that page shipped with. In `since=-2` mode the echo is the ONLY cursor handoff (the replay is empty by design), and the explicit-`since` inference keeps it flowing there too. The `caps` parameter itself may be repeated (`?caps=pong&caps=ping` — every value is scanned, not `url.Values.Get`'s first only), and matching stays whole-token and case-sensitive (`caps=PING` opts nothing in). Pinned by `TestHandleInstanceTTYWS_HeartbeatTextPingRequiresOptIn` (no caps / `caps=pong` / `caps=pinger` / `caps=PING` receive zero text pings over 6+ heartbeat ticks while the RFC pings keep flowing; `caps=resize,ping` and the repeated `caps=pong&caps=ping` opt in), `TestHandleInstanceTTYWS_SyncFrameRequiresOptIn` (no caps / `caps=ping` / `caps=SYNC` receive the binary replay but never a sync frame; `caps=sync` and the repeated `caps=pong&caps=sync` receive it — as do explicit-`since` clients via the inferred #87-era opt-in: `since=5`, `since=-2`, even unparsable), the `caps=ping,sync` dials in `TestHandleInstanceTTYWS_HeartbeatEmitsPingControlFrames` / `_ResponsivePeerSurvivesReadDeadline`, and the two-sided source anchors in `TestTerminalStatusLivenessHeartbeatContract`. Contract documented in `docs/API.md` (TTY WS). Upgrade note for the release: tabs opened before the upgrade still need ONE refresh to pick up the whitelisted client — say so in the release notes. +- **fix(ui): switching to an instance-less worktree left the previous worktree's dsh-web iframe on screen, covering the empty state (issue #101)** — with a dsh-web instance active on worktree A, selecting a worktree B with ZERO instances kept A's iframe visible; because `#dsh-panel` sits after `#empty-state` in the DOM, it painted over the "No instances" hint, the UI read as B while keystrokes still reached A's session — an invitation to run commands in the wrong worktree. Three defects combined, all required: (A) `renderWorkspace()`'s no-active-instance branch hid only `#opencode-panel` / `#reasonix-panel` and forgot `#dsh-panel` — whose CSS keeps it full-size until `[hidden]` is set — while the only line hiding it lived in `selectInstance()`, unreachable from the worktree-switch path; (B) `selectWorktree()` never called the previous renderer's `deactivate()` — `maybeDestroyInactiveStoppedSession`'s terminal-bucket guard returns early for web instances — so the panel stayed visible AND the old instance's ready/scope polling kept running in the background after the switch; (C) `selectInstance(null)` early-returns on the empty-worktree path (`selectWorktree` had already nulled `activeInst`), so its panel-hiding lines never execute there — correct de-duplication, but it meant the path could no longer be relied on for cleanup. Fix: one `hideWebPanels()` helper hides ALL three panels as the single source of truth — shared by `renderWorkspace` (the empty branch AND the non-web active branch, which also lacked the dsh hide) and `selectInstance` — so the next web kind cannot be forgotten by one of the call sites; and `selectWorktree` now deactivates the previous instance's renderer unconditionally (deliberately WITHOUT `selectInstance`'s `prevRenderer !== renderer` guard: a worktree switch is always a full leave, never a same-renderer re-activation), which also stops the leaked polling. opencode-web / reasonix were only accidentally correct on this path and are now covered by the same helper; `selectInstance(null)`'s early return (defect C) is left alone by design. Pinned by `TestEmptyWorktreeSwitchHidesAllWebPanels` — asset-text assertions in the style of `TestWebRenderersStoppedSwitchHidesAllFrames` covering the helper's three panel ids, both `renderWorkspace` call sites, the shared-helper call in `selectInstance`, and the guard-less `deactivate()` in `selectWorktree`. Review follow-ups (second round): the leave cleanup is now a shared `deactivateInstanceRenderer()` helper — kind-agnostic and unconditional, skipping an unresolvable id (concurrent deletion) instead of falling back to `'pty'` and deactivating the wrong renderer — used by BOTH worktree-switch entry points: `selectWorktreeByID` (the create/import flows) previously set `activeWT` and fired an async `refresh()` without deactivating the previous renderer or clearing `activeInst`, so the old worktree's web iframe stayed visible — and kept polling — over the new worktree until the refresh round-trip finished; it now deactivates, clears the selection and re-renders synchronously before refreshing. The test pins the deactivate BEFORE the `state.activeWT = id;` / `render();` sequencing in both entry points (position, not mere presence — a deactivate after render() flashes the empty state over the still-visible iframe), and the function slicer derives its end-marker indentation from the declaration line instead of a hardcoded 8 spaces, so a re-indented script block fails loudly rather than silently extending every slice to end-of-file. + ## v0.5.1 (2026-10-06) Release focused on dsh 0.2.x support (issue #84), terminal WebSocket reliability (issues #80–#83, #86, #87), a beta prerelease channel off `develop`, and a round of UI/installer fixes — three of them found by the v0.5.1 betas themselves (issues #96, macOS overlay L2, alias-started daemon detection). @@ -17,7 +40,7 @@ Release focused on dsh 0.2.x support (issue #84), terminal WebSocket reliability - **fix(terminal): a throttled browser tab lost all terminal output, silently and forever (issue #82)** — a backgrounded tab, a slow link or a laptop sleep left the terminal frozen on a stale screen while the shell kept producing output, and neither side could notice. Root cause: `pty.broadcast` fanned each chunk out with `select { case ch <- chunk: default: }` and the `default:` arm was **empty** — the chunk was dropped. `ws.Conn.writeFrame` blocks inside `rw.Flush()` on the hijacked TCP socket and `Upgrade` sets no write deadline, so as soon as the client stops reading, `conn.WriteBinary` blocks, the handler's select loop stops draining, the subscriber's 64-slot channel fills, and from that instant **every** chunk was discarded — unboundedly, with no counter, no signal to the client and no disconnect — so one missed clear-screen or cursor move corrupted everything rendered afterwards. Fix: an overflowing subscriber is now **removed, closed and drained** rather than kept and skipped, so the handler reaches `!ok` (bounded: the backlog never reaches the socket writer, though a parked receiver can still win a few buffered chunks) and answers with a `1013 subscriber overflow: slow consumer` close frame and returns; the browser's `ws.onclose` (which since #87 already logs abnormal close codes and reasons) prints the cause in the console with no client change, reconnects, and resumes **incrementally from its own cursor** on the #87 offset contract already shipped in #87. The channel capacity is deliberately unchanged — a longer queue only postpones the disconnect of a consumer that cannot keep up, and the disconnect is the correct outcome. The threshold is concrete rather than "a bit slow": `pumpLogs` reads the PTY in 1024-byte chunks and each subscriber queue holds 64 of them, so a consumer is disconnected after roughly **64 KB** of output it has not taken, and recovery costs the client the browser's hard-coded 5 s reconnect (`index.html`) plus an incremental resume from its own cursor. The teardown close write is also now **bounded**: `conn.SetWriteDeadline(5s)` immediately before it, on that failure path only, because this branch exists precisely because the socket stalled — an unbounded close would have moved the stall from `WriteBinary` to `WriteClose` and parked the handler forever, which would not have fixed #82 at all. Stated exactly what that bound covers, because an earlier wording here overstated it: the deadline bounds **only this teardown close-frame write**; a handler already blocked inside a live `conn.WriteBinary` never reaches this branch and is reclaimed only when that write returns — `Upgrade` sets no write deadline, and issue #83 — since merged, on the liveness side — added only a READ deadline and deliberately scoped a write deadline out; a write deadline for every connection remains unimplemented, by design, not by oversight. What actually keeps the failure path from flushing the backlog onto the stalled socket is upstream: Go delivers a closed channel's **buffered** values with `ok == true` before it ever reports `!ok`, so the overflow arm now **drains the closed subscriber's queue at the source** (`for range` over the collected subscribers, **after `subsMu` is released** — never inside it: that loop terminates only because the channel it ranges over is already closed, which is a convention this code keeps rather than something the compiler enforces, and `subsMu` is process-global, so draining under it and dropping or reordering the one `closeLocked()` line that precedes it would wedge every OTHER instance's broadcast at `subsMu.Lock()` with no panic, no log and no timeout — a daemon-wide silent freeze traded for the old loud local `panic: send on closed channel`; so `closeLocked` now reports whether it performed the close and only those subscribers are collected, which makes "we drain only channels this critical section closed" provable instead of assumed), making the handler's very next receive the resync signal instead of up to 64 queued chunks (~64 KB) of `WriteBinary`; discarding that backlog loses nothing **that is still in the ring buffer**, because `pumpLogs` writes every chunk to the ring buffer *before* broadcasting it (that ordering is now commented as load-bearing) and each drained chunk comes back on the reconnect's replay from the client's #87 cursor — the whole premise of #82 — with the one exception both `docs/API.md` and this entry now name: a chunk broadcast after `Manager.dropBuffer` closed and dropped the ring was never stored at all (`RingBuffer.Close` sets `closed=true, data=nil, used=0` and does not reset `head`, and the buffer is no longer reachable through the Manager), so it is unreplayable and a still-connected client would have received it on the wire pre-drain — the same post-swap loss ARCHITECTURE §4.1 already declares intentional. The overflow is also now **logged server-side** (`s.logger.Printf`, nil-guarded, matching the existing tty-resize precedent in `app.go`): this disconnect is user-visible and the client retries on a hard-coded 5 s timer with no backoff, so without the log line a repeatedly-overflowing consumer was a silent reconnect+replay loop whose only trace was a browser-console warning. That nil guard is defensive against a state production cannot reach — `app.New` rejects a nil logger — and it is no longer dead inside the suite: `ttyWSTestServer` injects a real logger into the `Server` it mounts, so the overflow test asserts the line fired with the instance id in it (until then every TTY WS test ran with `s.logger == nil`, and deleting the line left `internal/app` green, which made this sentence and `docs/API.md`'s "the server logs the overflow" unverified). That frame is best-effort by design and resync never depends on it arriving: the browser reconnects on any socket close, reasoned or not. A write deadline covering healthy connections (which would make the whole thing gentler) remains unimplemented — issue #83 shipped without one, deliberately scoping itself to read-deadline liveness. That close gave the channel a second closer, so `subs` now holds a `*subscriber` (its channel plus a `closed` flag) whose `closeLocked` does the check-and-close under `subsMu`; moving `close(ch)` inside the lock would **not** have been enough, because `cancel`'s `delete` is a no-op on a channel the overflow already removed and it would still double-close — `panic: close of closed channel`, in `pumpLogs`' goroutine, taking the daemon down. `SubscribeOutput`'s public signature is unchanged (the framework `outputSub` assertion and the dsh-web bridge keep working), and an id key now leaves `subs` as soon as its last subscriber overflows, preserving the "key exists iff it has a live subscriber" invariant `cancel` already kept — now pinned at the registry level, not just per id, by a `registeredInstanceCount` delta assertion in `TestBroadcast_OverflowDisconnectsSubscriber` (the per-id `liveSubscribers` check answers 0 for a stale empty set exactly as happily as for a removed key, so before that assertion the `delete(subs, id)` in the overflow arm — and this sentence's claim about it — was unpinned: removing the delete left the whole pty package green). Pinned by `TestBroadcast_OverflowDisconnectsSubscriber`, `TestBroadcast_WithinCapacitySubscriberIsNeverDisconnected`, `TestSubscribeOutput_CancelAfterOverflowDoesNotPanic` and `TestSubscribeOutput_CancelRacesOverflow` in `internal/instance/pty/driver_test.go` (clean under `-race -count=10`), and end to end by `TestHandleInstanceTTYWS_SubscriberOverflowClosesConnectionWith1013` in `internal/app/tty_ws_test.go`, which drives the same condition over a real hijacked WebSocket — the client goes quiet after the handshake while the producer outruns the subscriber's queue — and pins everything downstream of the close: the 1013 reason on the wire, nothing served afterwards, and the registry drained. That test's kind is a double of the driver, so the double now mirrors the drain too — delete, close, then drain after its own lock, with `closeLocked` reporting the close — and the test counts the live binary frames before the close against the exact arithmetic (`published - 16 - 1` reached the handler, so exactly that many may reach the wire): 16 more and the backlog was flushed onto the stalled socket, which is the behaviour the drain exists to remove. Without the mirror the e2e test would still have been pinning the *pre*-drain shape, and reverting the production drain would have left `internal/app` green. Stated exactly, as the test's own comment now says: it does **not** reproduce the TCP stall (a publish is a mutex plus a non-blocking send against the handler's 32 KB `write()` syscall, so the flood outruns the drain by orders of magnitude and the socket buffers never become the binding constraint), and its deferred-cancel check pins the **deregistration** only, because net/http recovers handler panics into stderr — the exactly-once close is pinned in the pty package, where a panic does fail the test. Deliberately **not** done here: a write deadline on the TTY socket, which would turn TCP backpressure into a bounded stall but changes behaviour for every connection including healthy ones, remains unimplemented — issue #83 (since merged) scoped itself to read-deadline liveness and deliberately left the write deadline out. Contract documented in `docs/ARCHITECTURE.md` §4.1, and in `docs/API.md` (TTY WS: the `1013 subscriber overflow: slow consumer` close code in the server→client outcomes and the handshake protocol's step 8). - **feat(dsh-web): adapt to the dsh 0.2.x RPC wire + `--no-open` (issue #84)** — the embedded integration now targets **dsh 0.2.x only**; 0.1.x support is dropped rather than dual-branched — a user still on dsh 0.1.x now hits a Spawn that fails at the hard version gate with a readable error, and the frontend says 0.1.x is unsupported instead of offering the "use it from the local 127.0.0.1 environment" fallback (dropped from the version-gated branches as dead advice, yet deliberately kept in the version-unknown `cap._error` branch, as the follow-up fix bullet below details). Two breaking upstream changes are implemented, both verified by hand against the installed `dsh 0.2.0-rc.2`: (1) **endpoint segments became `/`** — `POST /api/workspace/create`, `/api/session/create`, `/api/subagents/prompt` (the subagent namespace is **plural**), with the envelope's `method` still required to equal the URL-path endpoint (bare `/api` still 404s); the 0.1.x dotted names (`workspace.create`) 404 on 0.2. (2) **every verb's single object argument is nested at `payload.args.request`** — `{type:'client-request', rpcId, method:'workspace/create', payload:{args:{request:{path:…}}}}`; the envelope and the `{ok, value}` answer shape are unchanged, and the `value` shapes (`{workspace:{workspaceId,…}, created}` / `{sessionId, agentPreset?}`) are unchanged. Code: `bootstrap.go` gained a `requestPayload` wrapper and the two bootstrap calls use the slash endpoints; `proxy.go` matches `/api/session/create` for the session-id tee, `ownSessionWriteMethods` / `ownSessionIDsInBody` / `classifyRPCBody` switched to the slash names with a shared `decodeArgsRequest` unwrapping `payload.args.request` (no legacy fallback — the real gateway rejects a flat payload with "Remote payload must contain exactly one plain-object args field"). Gates: `minVersion 0.2.0`, `supportedMaxVersion 0.3.0` (exclusive), `minRemoteVersion 0.2.0`, `NpxPin 0.2.0-rc.2` (npm publishes it as both `latest` and `next`, and it is the exact build every finding was verified against); `probeAndGate`'s error now names the slash endpoints and `--no-open`. Frontend: the two hard-coded `0.1.5` remote-floor fallbacks and the "(0.1.x)" warning text in `kinds/dsh_web.js` (plus `index.html`) became `0.2.0` / `(0.2.x)`. **Issue #84**: the launcher appends **`--no-open`** — dsh 0.2.x opens the host's default browser on every boot, and since the SPA is embedded in an iframe each Start spawned a stray browser tab; the ready line still prints (`printUrl` stays true), so the ready-line parse is unaffected. It is appended **after `--port 0`**, grouped with the web-app options — launcher flags (`--patch`) must stay first (`allowUnknownOption` + `passThroughOptions`; `TestWebArgs` pins `--no-open` after both `--patch` and `--host`). Verified unchanged and deliberately untouched: the restrict overlay rows (`storage-json` / `directory-picker` / `client-hmr` + `insert: directory-picker-browse` all compose on 0.2.0 per `--dump-config`, and a live instance's `pluginInventory/list` shows no enabled non-active entry with no `dsh-client-ui-directory-picker-*` surface loaded), the auth relay (`GET /?token=` → 303 + `dsh-auth-*` cookie), `/api/remote.mux`, and the injected `static/dsh-bridge.js`. Test mocks (`testdata/dsh-mock.go`, `testdata/dsh-mock-modern.go`) now report `0.2.0-rc.2`, answer the slash endpoints, and reject a payload without `args.request`. Docs: ARCHITECTURE §9 / PRD §7 / API §5.14–5.15 / README (+zh-CN) reworded, and `docs/plans/dsh-native-ui/PLAN.md` gained a marked dsh-0.2.x amendment block (the 0.1.x history is kept as history). - **fix(dsh-web): the review-round fixes on top of the 0.2.x adaptation** — four follow-ups forced by two independent code reviews of that same change (the adaptation itself is documented above and not repeated here). **fix(dsh-web): the advisory version gate could not classify a prerelease dsh** — `isSupportedVersion` / `hardVersionOK` compared the RAW `dsh --version` string, so a `-rc.N` suffix made `splitVersion`'s `Atoi` fail and `versionLess` answer `false` for BOTH bounds. The npx launch mode is the only caller that passes the raw pin (`driver.go` assigns `version = NpxPin`), so every npx-launched instance reported `version_supported:false` and the frontend painted the permanent red "dsh version is too new or the restrict overlay is not effective" bar even though `overlay_verified` was true. Both gates now core-parse through `parseVersion` exactly like `isRemoteCapable`, so `0.2.0-rc.2` classifies as core `0.2.0` while `0.1.9-rc.1` is still refused by the hard gate. Pinned by the prerelease cases in `version_test.go`, `TestVersionGatesAgreeOnPrereleaseCore`, and `TestSpawnNpxModeReportsVersionSupported` (a real npx-mode `Spawn` asserting `VersionSupported == true`). **fix(dsh-web): the mock toolchain accepted any argv, so a rejected flag stayed invisible** — both `testdata/dsh-mock*.go` doubles ignored unknown options, so a dsh build that ever dropped `--no-open` (or one where `--patch` moved behind an app-level option and `allowUnknownOption` + `passThroughOptions` forwarded it to the web app, killing the boot) would leave every test green while every real Start failed. Both mocks now validate the recognised web flag set (`--patch`, `--host`, `--port`, `--no-open`, `--trusted-host`, plus the launcher-level `--dump-config`) and fail like commander does (`unknown option ''` on stderr, exit 1); `TestMockDshWebFlagContract` pins both mocks across positive (`webArgs` boots, `dumpArgs` dumps, `--trusted-host`'s value token is skipped) and negative (`--no-browser`, `--allow-origin`) cases. **test(dsh-web): all eight write-driving RPC verbs are now pinned, not three** — `ownSessionWriteMethods` carries the dsh 0.2 slash endpoints (`session/prompt`, `session/cancel`, `session/fork`, `session/rename`, `session/selectModel`, `session/attachment`, `session/updateQueue`, plus the plural-namespaced `subagents/prompt`), but only three were exercised, so a typo in any of the other five silently disabled own-session attribution for that verb — a session this daemon actually drives would be reported as foreign activity — with every test green. `TestProxySessionOwnMarking` is now table-driven over all eight with distinct ids, an exact-set comparison, and a bidirectional coverage guard that fails if the map and the table drift apart. **frontend/docs corrections carried out in the same review round** — the "use it from the local 127.0.0.1 environment" fallback was dropped from the two VERSION-GATED dsh capability warnings (`cap.missing` / `!cap.remote_capable` — dead advice once `minRemoteVersion == minVersion == 0.2.0`, since `hardVersionOK` refuses a sub-0.2.0 dsh in EVERY mode, loopback included) and is deliberately kept in the `cap._error` branch, where the capability fetch itself failed so the installed dsh version is UNKNOWN and a local Start may well succeed; the stale `/api/events.mux` / `/api/events.host` carrier paths became the real `/api/remote.mux` in `docs/ARCHITECTURE.md` §9, `docs/PRD.md` §7 and `docs/plans/dsh-native-ui/FEASIBILITY.md`; the false "dsh has no authentication surface" claim was corrected wherever it survived (dsh 0.2.x DOES carry browser-session auth — launch token → `dsh-auth-*` cookie — and the per-instance proxy is its sole credential holder); `docs/API.md` §5.14 now shows the reachable `version` (`0.2.0`, the parsed core) with `missing_dsh` split into its own failed-instance example; and `docs/plans/dsh-native-ui/pr5-remote-e2e.sh` now drives the real `/api/remote.mux` handshake and FAILS on a non-101 instead of recording it. -- **fix(terminal): a new instance could show `[Process Stopped]` over a live PTY (issue #80)** — a just-created terminal sometimes rendered the red `[Process Stopped]` banner, greyed-out styling, and no visible input or output, while blind typing still reached the process and produced correct results; switching tabs or pressing Refresh "fixed" it. Root cause: the frontend collapsed the server's **seven-state** status enum into a `status === 'running'` boolean. `Manager.Start` persists a new record as `starting` and flips it to `running` on the kind's ready signal in a separate goroutine (a store version conflict costs 3 × 10 ms of retries), so an instance created moments earlier is routinely observed as `starting` at selection time — and the activation path treated that transient state as terminal: it disconnected the transport, replayed the log with the banner, and never reconnected. Worse, nothing could recover from that: `selectInstance()` returns early when the tab is already selected, and the 2 s poll's `renderTerminalSessions()` only toggles visibility, so `activate()` runs exactly once per selection change. The instance then stayed transport-less forever: input silently degraded to the buffered HTTP fallback (which is why typing "worked") while output, published only to WS/SSE subscribers, never came back. Fix: both activation paths (`kinds/pty.js` and index.html's legacy no-renderer fallback) now classify the status into three buckets — **live** (`running`, `unhealthy`: the process is alive and takes I/O; `unhealthy` means a probe failed, not that the process died), **pending** (`starting`, `stopping`: transient, nothing concluded yet) and **terminal** (`stopped`, `failed`, `exited`) — and only the terminal bucket paints the banner, greys out the terminal, refuses input, or blocks the connect. Re-activation moved from selection-driven to poll-driven: `reconcileTerminalSessions()` runs on every `refresh()` with fresh state and **promotes** the active session as soon as its status turns live (`connectTTY` only, guarded against a CONNECTING socket so the poll cannot flap it; no `loadLog`, since the WS handshake replays the screen state itself — the tail on a first connect, and only the bytes after the client's `since` cursor once it holds one (issue #87, shipped with this branch)), and replays the log once on entry into the terminal bucket, tracked per session via `lastKnownStatus`, so a crashed or failed instance still gets its banner. An `unhealthy` instance also keeps its transport and its reconnect-on-close retry instead of being reported as stopped (note: `unhealthy` and `stopping` are reachable only through the enum — `runLifecycle` persists only `running` and the terminal statuses, and a kind's health-probe result is discarded, so those two branches cannot be exercised against a live backend today; tracked separately). The status-bar text for the renderer-owned web kinds (`reasonix`, `dsh-web`) follows the buckets too, so a freshly created instance reports `starting...` where it used to read `stopped` for the whole `starting` window — next to an iframe that `kinds/reasonix.js` keeps navigating. Both activation paths also refuse an id `state.instances` cannot resolve (the server accepts that socket and closes it with 1013, which surfaced as a dropped connection), `activate()` shares the promotion's in-flight guard so a tab switch mid-bring-up no longer opens a second socket, and a failed terminal replay is retried with an exponential backoff instead of once per 2s tick forever (a re-select bypasses it), and an explicit tab re-selection now takes over a queued reconnect instead of freezing for the rest of the 5s window — the poll-driven promotion still waits, because taking over from it every tick would flap the connection. The takeover cancels the queued retry itself rather than leaving it to `disconnectTTY`, which activation only reaches after its `loadLog` settles: a retry firing in that window opened a socket and replayed the tail, and the connect that followed opened a second and replayed it again. Pinned by 67 `node --test` cases (`testdata/terminal_status.test.mjs`, which slices the shipped `index.html` / `kinds/pty.js` and runs them against stubbed page state, wired into `go test` via `TestTerminalStatusHandling`, whose count `TestTerminalStatusChangelogCount` re-derives from the file rather than trusting this sentence) plus the source-contract `TestTerminalStatusBucketsReplaceRunningEquality`. Contract documented in `docs/ARCHITECTURE.md` §5.3 / §5.4 / §5.6 / §5.8 and `docs/API.md` — the Start endpoint's `201` body is `status: "starting"`, not `"running"`, and the doc said otherwise, which is what invited the bug. +- **fix(terminal): a new instance could show `[Process Stopped]` over a live PTY (issue #80)** — a just-created terminal sometimes rendered the red `[Process Stopped]` banner, greyed-out styling, and no visible input or output, while blind typing still reached the process and produced correct results; switching tabs or pressing Refresh "fixed" it. Root cause: the frontend collapsed the server's **seven-state** status enum into a `status === 'running'` boolean. `Manager.Start` persists a new record as `starting` and flips it to `running` on the kind's ready signal in a separate goroutine (a store version conflict costs 3 × 10 ms of retries), so an instance created moments earlier is routinely observed as `starting` at selection time — and the activation path treated that transient state as terminal: it disconnected the transport, replayed the log with the banner, and never reconnected. Worse, nothing could recover from that: `selectInstance()` returns early when the tab is already selected, and the 2 s poll's `renderTerminalSessions()` only toggles visibility, so `activate()` runs exactly once per selection change. The instance then stayed transport-less forever: input silently degraded to the buffered HTTP fallback (which is why typing "worked") while output, published only to WS/SSE subscribers, never came back. Fix: both activation paths (`kinds/pty.js` and index.html's legacy no-renderer fallback) now classify the status into three buckets — **live** (`running`, `unhealthy`: the process is alive and takes I/O; `unhealthy` means a probe failed, not that the process died), **pending** (`starting`, `stopping`: transient, nothing concluded yet) and **terminal** (`stopped`, `failed`, `exited`) — and only the terminal bucket paints the banner, greys out the terminal, refuses input, or blocks the connect. Re-activation moved from selection-driven to poll-driven: `reconcileTerminalSessions()` runs on every `refresh()` with fresh state and **promotes** the active session as soon as its status turns live (`connectTTY` only, guarded against a CONNECTING socket so the poll cannot flap it; no `loadLog`, since the WS handshake replays the screen state itself — the tail on a first connect, and only the bytes after the client's `since` cursor once it holds one (issue #87, shipped with this branch)), and replays the log once on entry into the terminal bucket, tracked per session via `lastKnownStatus`, so a crashed or failed instance still gets its banner. An `unhealthy` instance also keeps its transport and its reconnect-on-close retry instead of being reported as stopped (note: `unhealthy` and `stopping` are reachable only through the enum — `runLifecycle` persists only `running` and the terminal statuses, and a kind's health-probe result is discarded, so those two branches cannot be exercised against a live backend today; tracked separately). The status-bar text for the renderer-owned web kinds (`reasonix`, `dsh-web`) follows the buckets too, so a freshly created instance reports `starting...` where it used to read `stopped` for the whole `starting` window — next to an iframe that `kinds/reasonix.js` keeps navigating. Both activation paths also refuse an id `state.instances` cannot resolve (the server accepts that socket and closes it with 1013, which surfaced as a dropped connection), `activate()` shares the promotion's in-flight guard so a tab switch mid-bring-up no longer opens a second socket, and a failed terminal replay is retried with an exponential backoff instead of once per 2s tick forever (a re-select bypasses it), and an explicit tab re-selection now takes over a queued reconnect instead of freezing for the rest of the 5s window — the poll-driven promotion still waits, because taking over from it every tick would flap the connection. The takeover cancels the queued retry itself rather than leaving it to `disconnectTTY`, which activation only reaches after its `loadLog` settles: a retry firing in that window opened a socket and replayed the tail, and the connect that followed opened a second and replayed it again. Pinned by 68 `node --test` cases (`testdata/terminal_status.test.mjs`, which slices the shipped `index.html` / `kinds/pty.js` and runs them against stubbed page state, wired into `go test` via `TestTerminalStatusHandling`, whose count `TestTerminalStatusChangelogCount` re-derives from the file rather than trusting this sentence) plus the source-contract `TestTerminalStatusBucketsReplaceRunningEquality`. Contract documented in `docs/ARCHITECTURE.md` §5.3 / §5.4 / §5.6 / §5.8 and `docs/API.md` — the Start endpoint's `201` body is `status: "starting"`, not `"running"`, and the doc said otherwise, which is what invited the bug. - **fix(terminal): log replay served the oldest buffered bytes instead of the newest (issue #81)** — re-selecting an idle terminal instance replayed the *oldest* 64 KB of the ring buffer, so the UI showed stale session-start output; only a window resize (SIGWINCH → TUI repaint) recovered the current screen. The kind refactor had rewired `Manager.Tail` to `ReadLogs(handle, 0, n)` — `ReadSince(0, n)` clamps to the oldest live byte — orphaning the correct `RingBuffer.Tail`. `Kind.ReadLogs` now defines a negative `since` as tail semantics (newest bytes, cursor at the end offset); the pty driver serves it from `RingBuffer.Tail`, `Manager.Tail` requests it and clamps non-conforming cursors, the log endpoints route an omitted/negative `since` to the tail, `GET /api/instances/log` returns the end offset in `X-Log-Offset`, and the browser cursor became a real byte offset. Follow-ups: #82 (broadcast drop / resync), #83 (weak WS liveness check), #86 (follow-from-live-end streaming), #87 (WS handshake replay duplication). ## v0.5.0 (2026-09-11) diff --git a/README.md b/README.md index 02673e8..adef406 100644 --- a/README.md +++ b/README.md @@ -93,7 +93,7 @@ PATH is auto-appended to `~/.zshrc` / `~/.bashrc` (open a new shell to pick it u Pin a version, change the install location, or skip PATH modification: ```bash -curl -fsSL .../install.sh | bash -s -- -v v0.5.1 # pin a version +curl -fsSL .../install.sh | bash -s -- -v v0.5.2 # pin a version INSTALL_ALIAS=mwt bash install.sh # avoid the Debian/Ubuntu `mw` clash INSTALL_DIR=~/bin bash install.sh # install elsewhere curl -fsSL .../install.sh | bash -s -- --no-modify-path # do not touch rc files @@ -132,17 +132,17 @@ Example: ```bash # Pick the archive that matches your platform, then verify and unpack it. # macOS Apple Silicon: -curl -LO https://github.com/linletian/myworktree/releases/download/v0.5.1/myworktree_v0.5.1_macOS_arm64.tar.gz +curl -LO https://github.com/linletian/myworktree/releases/download/v0.5.2/myworktree_v0.5.2_macOS_arm64.tar.gz # macOS Intel (replace arch in the filename): -# curl -LO .../myworktree_v0.5.1_macOS_amd64.tar.gz +# curl -LO .../myworktree_v0.5.2_macOS_amd64.tar.gz # Linux amd64: -# curl -LO .../myworktree_v0.5.1_Linux_amd64.tar.gz +# curl -LO .../myworktree_v0.5.2_Linux_amd64.tar.gz # Linux arm64: -# curl -LO .../myworktree_v0.5.1_Linux_arm64.tar.gz +# curl -LO .../myworktree_v0.5.2_Linux_arm64.tar.gz -curl -LO https://github.com/linletian/myworktree/releases/download/v0.5.1/checksums.txt +curl -LO https://github.com/linletian/myworktree/releases/download/v0.5.2/checksums.txt shasum -a 256 -c checksums.txt --ignore-missing -tar -xzf myworktree_v0.5.1_macOS_arm64.tar.gz +tar -xzf myworktree_v0.5.2_macOS_arm64.tar.gz # Optional: install into PATH sudo install -m 755 ./mw /usr/local/bin/mw @@ -161,7 +161,7 @@ mw --version > Or open **System Settings → Privacy & Security** and click "Allow Anyway" for the > blocked binaries. -Start from `v0.5.1` or newer for public release binaries. The earlier `v0.1.0` GitHub Release assets were withdrawn after post-release validation uncovered severe terminal interaction issues, and `v0.5.1` is the current recommended public release. +Start from `v0.5.2` or newer for public release binaries. The earlier `v0.1.0` GitHub Release assets were withdrawn after post-release validation uncovered severe terminal interaction issues, and `v0.5.2` is the current recommended public release. Each release archive contains `mw`, `myworktree`, `README.md`, `LICENSE`, and `CHANGELOG.md`. If you need a platform we do not publish, follow the source build steps below. diff --git a/README.zh-CN.md b/README.zh-CN.md index 9ac7b42..5d93c5f 100644 --- a/README.zh-CN.md +++ b/README.zh-CN.md @@ -94,7 +94,7 @@ curl -fsSL https://raw.githubusercontent.com/linletian/myworktree/main/scripts/i 可指定版本、改安装路径、跳过 PATH 修改： ```bash -curl -fsSL .../install.sh | bash -s -- -v v0.5.1 # 指定版本 +curl -fsSL .../install.sh | bash -s -- -v v0.5.2 # 指定版本 INSTALL_ALIAS=mwt bash install.sh # 避开 Debian/Ubuntu 的 `mw` 占用 INSTALL_DIR=~/bin bash install.sh # 装到自定义目录 curl -fsSL .../install.sh | bash -s -- --no-modify-path # 不改 rc 文件 @@ -130,17 +130,17 @@ curl -fsSL .../install.sh | bash -s -- -v vX.Y.Z-beta.N ```bash # 根据你的平台选择对应压缩包，然后校验并解压 # macOS Apple Silicon： -curl -LO https://github.com/linletian/myworktree/releases/download/v0.5.1/myworktree_v0.5.1_macOS_arm64.tar.gz +curl -LO https://github.com/linletian/myworktree/releases/download/v0.5.2/myworktree_v0.5.2_macOS_arm64.tar.gz # macOS Intel（替换文件名中的架构字段）： -# curl -LO .../myworktree_v0.5.1_macOS_amd64.tar.gz +# curl -LO .../myworktree_v0.5.2_macOS_amd64.tar.gz # Linux amd64： -# curl -LO .../myworktree_v0.5.1_Linux_amd64.tar.gz +# curl -LO .../myworktree_v0.5.2_Linux_amd64.tar.gz # Linux arm64： -# curl -LO .../myworktree_v0.5.1_Linux_arm64.tar.gz +# curl -LO .../myworktree_v0.5.2_Linux_arm64.tar.gz -curl -LO https://github.com/linletian/myworktree/releases/download/v0.5.1/checksums.txt +curl -LO https://github.com/linletian/myworktree/releases/download/v0.5.2/checksums.txt shasum -a 256 -c checksums.txt --ignore-missing -tar -xzf myworktree_v0.5.1_macOS_arm64.tar.gz +tar -xzf myworktree_v0.5.2_macOS_arm64.tar.gz # 可选：安装到 PATH sudo install -m 755 ./mw /usr/local/bin/mw @@ -157,7 +157,7 @@ mw --version > ``` > 或在 **系统设置 → 隐私与安全性** 中为被阻止的二进制文件点击"仍要打开"。 -建议从 `v0.5.1` 或更新版本开始使用公开发布版二进制。更早的 `v0.1.0` GitHub Release 资产在补充实测中发现严重终端交互问题后已撤回，而 `v0.5.1` 是当前推荐的公开发布版本。 +建议从 `v0.5.2` 或更新版本开始使用公开发布版二进制。更早的 `v0.1.0` GitHub Release 资产在补充实测中发现严重终端交互问题后已撤回，而 `v0.5.2` 是当前推荐的公开发布版本。 每个发布压缩包内都包含 `mw`、`myworktree`、`README.md`、`LICENSE` 和 `CHANGELOG.md`。 如果你的平台暂无对应产物，就直接使用下面的源码编译步骤。 diff --git a/RELEASING.md b/RELEASING.md index ca2ea27..9f6c1ad 100644 --- a/RELEASING.md +++ b/RELEASING.md @@ -48,7 +48,7 @@ One commit, `chore(release): prepare vX.Y.Z`, touching **only**: | File | Change | |---|---| -| `CHANGELOG.md` | Rename `## Unreleased` → `## vX.Y.Z (YYYY-MM-DD)` with a one-line release summary under it; keep an empty `## Unreleased` placeholder at the top. **Audit the delta for PRs that shipped without a CHANGELOG entry** (`git log --oneline ..develop` / merged PR list) and add the missing entries in house style (bold title — em-dash — detail — `Pinned by TestName`). | +| `CHANGELOG.md` | Rename `## Unreleased` → `## vX.Y.Z (YYYY-MM-DD)` with a one-line release summary under it; keep an empty `## Unreleased` placeholder at the top. **Audit the delta for PRs that shipped without a CHANGELOG entry** (`git log --oneline ..develop` / merged PR list) and add the missing entries in house style (bold title — em-dash — detail — `Pinned by TestName`). House rule (issue #95, also in an HTML comment at the top of the file): the phrase "Pinned by N `node --test` cases" declares the TOTAL case count of `internal/ui/testdata/terminal_status.test.mjs` and EVERY occurrence must equal it — when the count grows, bump them all; use different phrasing for subset counts. | | `README.md` | Bump every `vA.B.C` string: download URLs, example tarball names, `-v` pin example, "current recommended public release" wording. | | `README.zh-CN.md` | Same sweep as `README.md`. | | `scripts/install.sh` | Bump the `-v` example version strings in the header/usage comments. | diff --git a/docs/API.md b/docs/API.md index 145d6dc..a0bbbce 100644 --- a/docs/API.md +++ b/docs/API.md @@ -88,18 +88,78 @@ Body: Response (201): same as create. -### Delete (strict: refuses if dirty) +### Delete (strict: refuses if dirty, unless forced) `POST /api/worktrees/delete` Body: ```json -{ "id": "" } +{ "id": "", "force": false } ``` +- `force` (optional, default `false`, issue #102): delete even with + uncommitted or untracked changes. The refusal response lists everything + force would destroy — including gitignored files the dirty check never + blocks on — so a client that resends with `force: true` has seen the cost. + Force delete is unrecoverable: tracked changes AND every gitignored file + vanish with the directory. + Response: ```json -{ "status": "ok" } +{ "status": "ok", "ignored_destroyed": 0 } +``` +`ignored_destroyed` counts the gitignored entries the delete destroyed with +the directory (issue #102 review): a worktree whose ONLY at-risk contents are +gitignored files deletes without a refusal — the blocking check never sees +them — so the success response carries the count and the dashboard surfaces +it after the delete. **The field is present only when counted**: the force +path skips the status probe (a damaged worktree can fail `git status` while +`git worktree remove --force` still succeeds — force must not gate on the +probe), so a forced success OMITS the field rather than answering a 0 that +would read as "none destroyed" while really meaning "not counted" (second +review round). Counting itself is best-effort: a probe failure (e.g. the +10 s timeout on a pathologically large ignored tree) never blocks a clean +delete and leaves the count at 0. + +Refusal (409, dirty worktree and no force): the `error` code +`"worktree_dirty"` is part of the API contract — the dashboard matches on it +to open the dirty-details dialog instead of a generic alert. `message` +carries the one-line summary for text-only clients; `dirty` carries the full +breakdown (issue #102: the old flat "delete is refused" message made a +beforehand-damaged workspace indistinguishable from a leftover scratch file): +```json +{ + "error": "worktree_dirty", + "message": "worktree has uncommitted or untracked changes; delete is refused: 3 entries (1 deleted, 0 modified, 2 untracked); first paths: .gitignore, README.md, scratch.txt; additionally 1 gitignored entry present, which a force delete would destroy unrecoverably; retry with force to delete anyway", + "dirty": { + "entries": 3, + "deleted": 1, + "modified": 0, + "added": 0, + "renamed": 0, + "untracked": 2, + "ignored": 1, + "first_paths": [".gitignore", "README.md", "scratch.txt"], + "porcelain": "?? .gitignore\n D README.md\n?? scratch.txt" + } +} ``` +`ignored` counts `git status --porcelain --ignored` "!!" entries — gitignored +files/dirs that do NOT block the delete (the blocking check runs without +`--ignored`) but a force delete destroys unrecoverably. Both status probes +run with `-uall` (untracked/ignored directories count file-by-file instead of +collapsing to one entry) and `-c core.quotePath=false` (non-ASCII paths stay +literal); the carried `porcelain` is capped at 200 lines with a truncation +marker, while counts always reflect the full output. The MCP +`worktree_delete` verb takes the same `force` flag and answers a dirty +refusal with the SAME 409 + `worktree_dirty` + `dirty` breakdown shape +(issue #102 review — it previously returned a flat 400 MCP clients could not +recognize programmatically); its success result carries `ignored_destroyed` +under the same present-only-when-counted rule (omitted on force). The CLI +spelling is `myworktree worktree delete [--force] ` — `--force` may +appear before or after the id (issue #102 review: Go's flag parsing would +otherwise silently drop a trailing flag), the `--force=` spelling and +the `--` terminator work as in the flag-based subcommands, and a clean +delete prints the destroyed gitignored count to stderr. ### Open Terminal (host macOS) `POST /api/worktrees/open-terminal` @@ -443,7 +503,7 @@ Body: ``` ### Web TTY stream (WebSocket) -`GET /api/instances/tty/ws?id=[&since=]` +`GET /api/instances/tty/ws?id=[&since=][&caps=]` Bi-directional stream for terminal output/input with PTY support. @@ -455,7 +515,9 @@ Bi-directional stream for terminal output/input with PTY support. holds the tail and only its end offset is unknown (a reverse proxy stripped `X-Log-Offset` off `GET /api/instances/log`). The handshake replays NOTHING — no binary frame at all — and answers with - `{"type":"sync","offset":}` and then live output only. Sending + `{"type":"sync","offset":}` (`since=-2` is itself an explicit + `since`, so any client in this mode receives the echo) and + then live output only. Sending the tail here would paint it a second time under the screen that already shows it, which was the bug. This is the browser's `CURSOR_FOLLOW_LIVE_END` (-2), mirrored by `sinceFollowLiveEnd` in @@ -466,19 +528,57 @@ Bi-directional stream for terminal output/input with PTY support. OLDEST live byte (the #81 symptom); on this WS endpoint the catch-up loop reads forward from 0 and streams every chunk it reads straight to the socket, so the effect there is a replay of everything still live in - the ring — up to the 8 MB handshake budget — with `sync` at the end of - the last chunk actually written. The shipped UI never sends `0`; + the ring — up to the 8 MB handshake budget — with the closing `sync` at + the end of the last chunk actually written (the replay is bracketed by + the start/closing sync pair described below). The shipped UI never sends + `0`; it omits `since` entirely when its cursor is unknown (`CURSOR_UNKNOWN`) and sends `-2` when the screen is painted but the offset is not. - **`since>0`** → the server replays the bytes at or after that offset, in full, so a reconnecting client that still holds its rendered screen does not receive the tail a second time. +`caps` is an optional, comma-separated capability list (issue #98), and the +parameter may be **repeated** (`?caps=pong&caps=ping` — every value is +scanned, not just the first). Matching is by whole, case-sensitive token: +`caps=pinger` and `caps=PING` do NOT opt in. Two capabilities are defined: +- **`ping`** — the client whitelists the TEXT `{"type":"ping"}` heartbeat + (stamps its liveness clock, never renders it), so the server may send that + frame. A client that does NOT opt in — a page loaded before the whitelist + shipped, any non-browser client that did not ask — is never sent the TEXT + heartbeat, because it would render it as terminal output every 10 s. The + RFC 6455 ping on the same tick is unaffected: control frames never reach + page JavaScript, so it is sent to every client. +- **`sync`** — the client whitelists the `{"type":"sync"}` offset echo and + drives its reconnect cursor off it (issue #87), so the handshake sends it. + The echo is also sent to any client that presents an explicit `since` + parameter — the **inferred opt-in**: the cursor parameter and the sync + whitelist shipped in the same fix (#87, v0.5.1), and v0.5.0 never sends + `since` on this endpoint at all, so a client presenting one provably + parses the other. This keeps pre-caps v0.5.1 pages on the cursor contract: + without the echo their per-connection cursor latch never sets, the cursor + freezes, and every reconnect would re-replay from it. A client with + NEITHER — v0.5.0, a fresh non-browser probe — is never sent the frame, + because it would paint it into the terminal once per connect. Residual: a + pre-caps page whose cursor is still the unknown/zero sentinel sends no + `since` either, so it gets no echo and its reconnects re-replay the ≤64KB + tail until a loadLog re-pin or a refresh lands a cursor — the pre-#87 + behaviour that page shipped with. In `since=-2` mode the echo is the ONLY + cursor handoff (the replay is empty by design) — which is exactly why the + explicit-`since` inference matters: a pre-caps page in that mode still + receives it. +The rule is deliberately opt-IN rather than version-gated: the server can +never again leak a future visible control frame to a client that did not +declare it. Older servers ignore unknown query parameters, so a new client +against an old daemon behaves exactly as before (the old daemon sends the +TEXT ping unconditionally, and the new client whitelists it). + Each replay read is capped at 64KB. When the delta since `since` exceeds 64KB, the server loops reads until it is caught up and **streams each chunk to the socket as it reads it** instead of retaining one, so the whole delta is delivered and peak memory stays ~64KB (one chunk) regardless of the ring -cap. The `sync` offset is the end of the **last chunk actually written** to +cap. The closing `sync`'s offset is the end of the **last chunk actually +written** to this socket — never an offset over bytes the client did not receive **and can still receive**, because a client cursor only moves forward and would never ask for them again. The two deliberate exceptions both publish the ring's real @@ -509,6 +609,42 @@ still in the ring buffer is silently clamped to the oldest live byte; a tail (beyond head, defensive) — the client's own number is never echoed back as authoritative. +On that streaming catch-up path the replay is bracketed by TWO sync frames +(issue #94). Ahead of the FIRST binary chunk the server sends +`{"type":"sync","offset":,"start":true}`, where S is the replay's real +start offset: the first chunk's end offset minus its length, so S and the +first chunk come from the same read — the requested `since` when no clamp +happened, the clamped oldest live byte when it did, never the raw request +value. The closing sync after the last chunk is unchanged. A client that +adopts S on arrival may advance its cursor by every replay frame's wire +length from chunk 1 on, so a connection that dies mid-replay resumes from +the bytes it already rendered instead of re-pulling the whole (up to 8 MB) +replay from its old cursor — under sustained network degradation the pre-#94 +behaviour does not converge. The frame REUSES the `"sync"` type rather than +inventing a new one: every client that receives it already provably parses +`"sync"` (the caps token, or the inferred explicit-`since` opt-in above — +any catch-up request presents `since` by definition), its existing handler — +adopt the absolute offset, lift the per-connection latch — is exactly the +wanted semantics, and a new frame type would have to fight the stale-page +leak class (issue #98) with a new capability gate. The `"start":true` field +exists for consumers that must know WHEN the replay ends (test harnesses, +third-party clients); the shipped UI deliberately ignores it and treats both +sync frames alike. S + Σ(replay frame bytes) equals the closing offset +UNLESS the ring rolls a full cycle mid-replay and evicts past the loop's +cursor between two reads — a later chunk then clamps to the new oldest live +byte, the sum lands short, and the closing sync jumps the gap (truthfully: +those bytes are undeliverable anyway, the same rule the read→subscribe +window already lives by). The beyond-head tail degrade (`since` past head — +defensive, unreachable through normal flows) announces its tail's start the +same way, S = head − len(tail): the client presented a cursor, so EVERY +replay addressed to one carries a start frame. The closing sync arrives +ALONE only where there is no cursor to advance: the cursor-less +first-connect tail replay (a single ≤64KB frame whose death window is the +pre-#87 one — and a cursor-less client may not parse `sync` at all, which is +why THAT path's `syncCap` is a genuine gate, not an inference), the +`since=-2` handshake (zero binary frames by design) and the caught-up-at-head +exit (nothing to announce). + In `since=-2` mode the published offset comes from `Manager.EndOffset` — the head read with a zero-length body, because there is no replay to attach it to. Two windows surround that read and they behave differently, so do not @@ -535,14 +671,30 @@ conflate them: 1. Server sends `{"type":"ready"}` immediately after connection 2. Client should wait for this message before sending resize 3. Client sends `{"type":"resize","cols":80,"rows":24}` to start data flow -4. Server sends initial log (the tail, or only the bytes after `since`) as binary frames — and in `since=-2` mode sends NO binary frame at all -5. Server sends `{"type":"sync","offset":}` (text frame) — the end +4. Server sends `{"type":"sync","offset":,"start":true}` (text frame, + issue #94) announcing the replay's real start offset S — the requested + `since`, or the clamped oldest live byte when `since` predated it — + ahead of the FIRST binary frame. Sent on every replay addressed to a + cursor-bearing client (the streamed catch-up AND the beyond-head tail + degrade); skipped only by the cursor-less first-connect tail and by + empty replays. Same opt-in as the closing sync in step 6 — every client + that receives it provably parses `"sync"`, and a client that ignores + the `start` marker is merely back to the pre-#94 behaviour of waiting + for the closing sync +5. Server sends the initial log (the tail, or only the bytes after + `since`) as binary frames — and in `since=-2` mode sends NO binary + frame at all +6. Server sends `{"type":"sync","offset":}` (text frame, **only to + clients that opted in via `caps=sync` or presented an explicit `since`** + — issue #98) — the CLOSING sync: the end offset of that replay (in `since=-2` mode, the live head it refused to replay); the client stores it and sends it back as `since` - on its next reconnect -6. Real-time output continues as binary frames -7. Client receives first data and triggers second resize (50ms delay) for TUI redraw -8. If the client stops draining the live stream, the server closes the + on its next reconnect. On the streaming catch-up path this is the + second sync frame — the start announcement of step 4 preceded the + replay; everywhere else it is the only one +7. Real-time output continues as binary frames +8. Client receives first data and triggers second resize (50ms delay) for TUI redraw +9. If the client stops draining the live stream, the server closes the connection with **`1013` / reason `subscriber overflow: slow consumer`** (issue #82) instead of silently dropping output — see Close codes below; the client reconnects with the `since` cursor it already holds, exactly @@ -563,8 +715,8 @@ conflate them: *Server → Client:* - Ready: `{"type":"ready"}` (text frame) - Output: binary frames (terminal output chunks) -- Sync: `{"type":"sync","offset":}` (text frame, sent after the handshake replay — even when that replay was empty. The client latches it per connection: replay frames received BEFORE the sync never touch the cursor — the sync publishes the authoritative end of the whole replay in one step; binary frames received AFTER it advance the cursor by their wire byte count — every one of them is ring-buffer output, as the server closes the connection rather than writing diagnostics as binary) -- Heartbeat: `{"type":"ping"}` (text frame, every 10 s, issue #83) — the application-level mirror of the RFC 6455 ping the server sends on the same tick. Browsers answer the protocol ping automatically from their network stack (which refreshes the server's 45 s read deadline); this text frame is the heartbeat browser JavaScript can observe, since `onmessage` never fires for control frames. Clients MUST whitelist `ping` (and `pong`) as control types and MUST NOT render them as terminal output. +- Sync: `{"type":"sync","offset":}` (text frame, **opt-in via `caps=sync` — or inferred from an explicit `since` parameter — since issue #98**), the cursor handoff of the handshake, sent even when the replay was empty. Since issue #94 a replay addressed to a cursor-bearing client is bracketed by TWO of these: `{"type":"sync","offset":,"start":true}` ahead of the FIRST replay chunk carries the replay's real (possibly clamped) start offset, and the closing sync after the last chunk carries its end offset — every other path (the cursor-less tail, `since=-2`, an empty replay) sends the closing sync alone. The client latches per connection: a replay frame advances the cursor by its wire byte count only once SOME sync has arrived — against a post-#94 server that is the start sync, so frames count from chunk 1 on (S and chunk 1 come from the same server-side read, so the base is truthful); against a pre-#94 server the latch lifts only at the closing sync, which publishes the end of the whole replay in one step. Either way the closing sync is the absolute authority — whenever the summed frames and it drift (a ring rollover mid-replay can evict unsent bytes, landing the sum short), it wins. Counting binary frames is sound at all because every one of them is ring-buffer output: the server closes the connection rather than writing diagnostics as binary. The `"start":true` marker exists for consumers that must know WHEN the replay ends; a client that ignores it and treats both syncs as absolute offsets to adopt implements the full contract) +- Heartbeat: `{"type":"ping"}` (text frame, every 10 s, issue #83; **opt-in via `caps=ping` since issue #98**) — the application-level mirror of the RFC 6455 ping the server sends on the same tick. Browsers answer the protocol ping automatically from their network stack (which refreshes the server's 45 s read deadline); this text frame is the heartbeat browser JavaScript can observe, since `onmessage` never fires for control frames. Clients MUST whitelist `ping` (and `pong`) as control types and MUST NOT render them as terminal output — and the server only sends this frame to clients that declared `ping` in the handshake `caps` list, so a client without the whitelist never receives it. **Liveness (issue #83):** half-open TCP sockets (laptop sleep, NAT/proxy idle timeout) keep `readyState === OPEN` without ever firing `onclose`, so liveness rides on heartbeat traffic in both directions. Server: pings every 10 s; arms a 45 s read deadline before every read and reaps a peer that has sent nothing for that long (the browser's automatic Pong refreshes it). **Non-browser clients get no automatic Pong** — `internal/ws`'s own client returns `opPing` as an ordinary message and installs no responder — so any Go or embedded client of this endpoint MUST answer protocol pings with Pong and/or send `{"type":"ping"}` periodically, or it will be reaped at the 45 s deadline. Client: stamps the arrival of every frame, probes `{"type":"ping"}` every 5 s once READY, and reconnects after 30 s without heartbeat traffic (3 × the ping interval, under the server's 45 s backstop). The web UI's **primary detector is the 2 s poll** — `ensureTerminalLiveTransport()` notices the stale stamp on the active session at ~30 s and queues the reconnect (returning `false`: a queued reconnect is not yet a live transport); the 5 s watchdog is the fallback that also covers sessions the poll does not promote. Liveness is never inferred from the absence of program output — a prompt, `vim` or `top` emit zero bytes for hours and stay connected. A write deadline was deliberately **not** part of issue #83: normal-traffic writes on this socket carry none (the only bounded write here is the 5 s deadline on #82's overflow close-frame), so a handler blocked mid-write is reclaimed when that write fails or returns, not by the read deadline. @@ -616,9 +768,17 @@ Client Server |-- {"type":"resize", --->| Notify terminal size | "cols":80,"rows":24} | | | + |<-- {"type":"sync", ----| Start offset of the replay (issue #94): + | "offset":1024, | only on a replay addressed to a cursor- + | "start":true} | bearing client (streamed catch-up and the + | | beyond-head tail degrade); S is the + | | requested `since` or the clamped oldest + | | live byte, never the raw request |<-- binary output -------| Replay: tail, or bytes after `since` - |<-- {"type":"sync", ----| End offset of that replay - | "offset":4096} | + |<-- {"type":"sync", ----| CLOSING sync: end offset of that replay — + | "offset":4096} | only when the client opted in via + | | ?caps=sync or presented ?since + | | (issue #98) | | |--- (50ms delay) -------| | | @@ -628,9 +788,11 @@ Client Server |--- input bytes -------->| User input |<-- binary output -------| Process output | | - |<-- {"type":"ping"} -----| Heartbeat every 10s (issue #83; the same - | | tick also writes an RFC 6455 ping, which - | | the browser answers invisibly to JS) + |<-- {"type":"ping"} -----| Heartbeat every 10s (issue #83; only when + | | the client opted in via ?caps=ping — issue + | | #98. The same tick also writes an RFC 6455 + | | ping, which the browser answers invisibly + | | to JS, caps or not) |-- {"type":"ping"} ------->| Client probe every 5s once READY |<-- {"type":"pong"} ------| Answered - never typed into the PTY ``` diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 1826fc0..56a05dc 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -113,7 +113,7 @@ Each running instance owns a bounded, **in-memory** ring buffer (`internal/frame **Cursor semantics (`since` / `next`).** `head` is a monotonic total-bytes-written counter for the instance. Clients pass it as `since` to read incrementally; the server returns the bytes plus an advanced cursor. When no new data is available, the cursor is returned unchanged (preserves the SSE 1 s poll-loop contract). When `since` points to data already evicted from the ring (oldest live byte > since), the read silently clamps to the oldest live byte. A negative or omitted `since` requests **tail semantics**: the newest `maxBytes` are returned with the cursor set to the current end offset (the bare `GET /api/instances/log` response and the MCP `instance_log_tail` tool drive this path; kinds without log capture return an empty body and the manager clamps the cursor to `0`). **`since=-2` is the one negative value that is NOT tail semantics** (issue #86): the *follow-from-the-live-end* sentinel, for a client whose screen already holds the tail but whose `X-Log-Offset` was stripped by a proxy. Both stream endpoints branch on it explicitly, ahead of their `since < 0` tail branch — `parseInt64Default` passes any negative value through untouched, so an unbranched `-2` would be indistinguishable from the omitted `since` that caused the duplication. It means **send nothing, publish the current end offset, then stream only new data**: the SSE stream emits one empty `log` frame carrying the head, the WS handshake emits no binary frame at all and puts the head in `sync`. The head comes from `Manager.EndOffset` — `Tail(id, 0)`, the same lookup, kind delegation and cursor clamps, but a zero-length read, because copying 64 KB to throw it away on every stripped-header connect is pure waste (and it reports `0` for a non-running instance exactly as `Tail` returns `("", 0, nil)`). The browser names the two unknown states: `CURSOR_UNKNOWN` (-1: nothing painted, so `since` is omitted and the tail arrives) and `CURSOR_FOLLOW_LIVE_END` (-2: painted but unoffset, so no replay) — and the -2 claim is gated on content actually reaching the screen, since `writeSanitizedTerminalOutput` returns the number of characters it wrote (`.length`, not bytes) and sanitizing can empty a non-empty tail, so an empty paint keeps -1 and the replay stays available instead of hiding output. `TestFollowLiveEndSentinelAgreesAcrossTheWire` pins the `-2` against `sinceFollowLiveEnd` because no compiler sits between `internal/app/app.go` and the page. `kinds/pty.js` owns its own **prefixed** `PTY_CURSOR_UNKNOWN` and deliberately does not declare the other: every script in the page is a classic script sharing ONE global lexical environment, so a second top-level `const CURSOR_UNKNOWN` would throw `SyntaxError` in the second script and take the entire inline application down with it. That hazard is guarded, not remembered — `testdata/terminal_status.test.mjs` collects the top-level declarations of every non-vendor classic script plus the inline block and fails on any name two of them declare. The sentinel also has a scope: the non-streaming `GET /api/instances/log` answers any negative `since` with the tail, which is the sentinel's opposite, so that endpoint returns **400** for `-2` rather than silently inverting it. What `-2` costs is a window, and it is the window BEFORE the head read, not after it: the client painted the tail up to some head `H1`, the sentinel publishes `H2 >= H1`, the client adopts `H2`, so `[H1, H2)` is in no replay, no painted body and no live frame while the cursor already sits past it — permanently unrecoverable for that client, milliseconds normally and seconds if the WS handshake times out first. -The WS handshake (`completeHandshake`, issue #87) drives **tail only for a cursor-less first connect**; a reconnect with `since >= 0` loops `Manager.ReadSince` until caught up, **streaming every 64KB chunk straight to the socket as it reads it** instead of retaining one, so the whole delta is delivered and peak memory stays ~64KB (one chunk) regardless of the ring cap, and the `sync` offset it publishes equals the end of the **last chunk actually written** to that socket — never an offset over bytes the client never received **and could still receive**, because a client cursor only moves forward and would never ask for them again; on a caught-up exit that offset is the final read's own `next` (the ring's head), which is the one deliberate exception and is truthful precisely because an exhausted or closed ring can no longer deliver what sits behind it — the alternative, publishing a zero, would force a full tail replay of bytes the client already has. One handshake replays at most 8 MB (`ttyHandshakeReplayBudget`), counted in bytes already written and checked before the next read; once it is spent the loop stops, leaving the undelivered remainder **ahead** of the published cursor so the next reconnect re-requests exactly those bytes — deferred, never skipped. Bytes produced between the last chunk written and the live subscription sit behind the published cursor and are re-requested on the next reconnect (self-healing contiguity, not absolute coverage). A zero-progress first read consults `Tail` for the real head: cursor exactly at head is the normal caught-up exit (empty replay, sync == cursor == head), while a cursor past head — unreachable through normal flows since Restart mints a new id — degrades to the first-connect tail rather than echoing a bogus cursor. +The WS handshake (`completeHandshake`, issue #87) drives **tail only for a cursor-less first connect**; a reconnect with `since >= 0` loops `Manager.ReadSince` until caught up, **streaming every 64KB chunk straight to the socket as it reads it** instead of retaining one, so the whole delta is delivered and peak memory stays ~64KB (one chunk) regardless of the ring cap, and the `sync` offset it publishes equals the end of the **last chunk actually written** to that socket — never an offset over bytes the client never received **and could still receive**, because a client cursor only moves forward and would never ask for them again; on a caught-up exit that offset is the final read's own `next` (the ring's head), which is the one deliberate exception and is truthful precisely because an exhausted or closed ring can no longer deliver what sits behind it — the alternative, publishing a zero, would force a full tail replay of bytes the client already has. The streamed replay is bracketed by TWO sync frames (issue #94): `{"type":"sync","offset":S,"start":true}` ahead of the FIRST chunk — S = first chunk's `next` − length, the clamped oldest-live value whenever the request's `since` predated it, never the raw request — lifts the client's per-connection cursor latch at chunk 1 so a mid-replay drop resumes from the bytes already painted instead of re-pulling the whole replay (the death window #87 widened from one ≤64KB frame to up to 8MB), and the closing sync after the last chunk re-pins the end absolutely — the beyond-head tail degrade (`since` past head) announces its tail's start (head − len) the same way, so EVERY replay addressed to a cursor-bearing client carries the frame; only the cursor-less first-connect tail goes without; reusing the `"sync"` type means every recipient provably parses it (the caps token or the inferred `since` opt-in, issue #98) and needs no client change, while `"start":true` lets consumers that must know when the replay ends tell the two apart. One handshake replays at most 8 MB (`ttyHandshakeReplayBudget`), counted in bytes already written and checked before the next read; once it is spent the loop stops, leaving the undelivered remainder **ahead** of the published cursor so the next reconnect re-requests exactly those bytes — deferred, never skipped. Bytes produced between the last chunk written and the live subscription sit behind the published cursor and are re-requested on the next reconnect (self-healing contiguity, not absolute coverage). A zero-progress first read consults `Tail` for the real head: cursor exactly at head is the normal caught-up exit (empty replay, sync == cursor == head), while a cursor past head — unreachable through normal flows since Restart mints a new id — degrades to the first-connect tail rather than echoing a bogus cursor. **Lifecycle.** - `Start` resolves the cap with budget enforcement, creates the buffer, adds it to `Manager.buffers[id]` (an `*atomic.Pointer[RingBuffer]`), and atomically increments `Manager.totalBufBytes`. diff --git a/docs/TERMINAL_DETACH_REATTACH_DISCUSSION.md b/docs/TERMINAL_DETACH_REATTACH_DISCUSSION.md new file mode 100644 index 0000000..c6d88a7 --- /dev/null +++ b/docs/TERMINAL_DETACH_REATTACH_DISCUSSION.md @@ -0,0 +1,203 @@ +# 终端实例存活与重启拉回 — 讨论稿 + +> 状态：讨论稿，未决策，不含实现承诺。记录一次关于 pty 终端实例生命周期的 +> 讨论：myworktree 关闭后终端及其内部进程的存亡、能否让它们存活且重启后 +> 原样拉回、以及长期未回收的风险。 +> +> 所有代码事实均标注文件行号，基于写稿时的 main/develop 代码。 + +## 1. 问题 + +- Q1：myworktree 进程关闭后，终端实例（pty kind）及其中的进程（如 opencode + cli）是否自动一起被杀掉？ +- Q2：有没有可能让终端实例和内部进程继续存活，并且（a）可追溯是 myworktree + 启动的，（b）下次 myworktree 启动后原样拉回，（c）拉回过程中断为零？ +- Q3：如果允许长期存活，长期未回收的风险有哪些？ + +## 2. 现状：终端实例的生命周期（代码事实） + +### 2.1 进程拓扑 + +- 每个终端实例是一个 `zsh -f -i`，经 `creack/pty.Start()` 启动 + （`internal/instance/pty/driver.go:87,104`）。creack/pty v1.1.24 的 + `Start` 语义是"新会话 + 控制终端"（Setsid + Setctty）：zsh 是会话首领， + PTY slave 是它的控制终端。 +- **PTY master fd（`h.ptmx`）持有在 myworktree 进程内**，保存在 per-instance + Handle 里（`internal/instance/pty/driver.go:56`）。Resize / SendInput / + pumpLogs 全部通过这个 fd 进行（`driver.go:251,259,262`）。 +- 用户在终端里跑的前台程序（如 opencode cli）因交互 shell 的 job control + 而拥有独立进程组。 + +结论：**终端的命脉（master fd）握在 myworktree 手里**。这是后续一切结论的 +根源。 + +### 2.2 关闭链路：Q1 的答案是"必死" + +| 场景 | 机制 | 结果 | +|---|---|---| +| 优雅退出 | `Server.Shutdown()`（`internal/app/app.go:697-709`）**不显式杀 pty**（代码注释原文：tty instances "die when their PTY hangs up"），只显式 `StopAllKind(reasonix/dsh)`。myworktree 进程退出 → 内核关闭 ptmx | PTY hangup：内核对前台进程组与会话首领发 SIGHUP(+SIGCONT)。opencode cli（TUI，默认不捕获 HUP）直接终止；zsh 收到 HUP 后退出并把自己的作业 hup 掉 | +| kill -9 / 崩溃 / OOM | 进程死亡 → 所有 fd 关闭 | 同上，同一 hangup 链路 | + +即：**终端实例与其中的 opencode cli 会随 myworktree 自动一起死，且不依赖 +myworktree 的主动清理**——这是内核级 PTY 语义（谁持有 master fd，谁就是终端 +的命脉），不是代码里某个显式 kill 的结果。 + +**逃逸口（已知）**：终端内自行 `nohup` / `setsid` / `& disown` 的进程会活 +下来，但成为**无人追踪的孤儿**——全仓库无任何 `Pdeathsig` 设置（grep 确认）， +这些进程躲过 SIGHUP 后彻底失联。这正是 Q3 风险的现成入口。 + +### 2.3 Reattach 机制现状：Q2 的可行性底子 + +框架已具备一套"进程存活 + 重启拉回"机制，但 pty 被刻意排除： + +- `framework.Kind` 有可选的 `RestartSurvivor` 接口：`Reattach(ctx, id)` + （`internal/framework/kind.go:356-361`）。同处注释明确写着 "Kinds whose + processes die with the server (PTY) do NOT implement this interface"。 +- `Manager.ReconcileRunningOnStartup()`（`internal/framework/methods.go:241`） + 在启动时对每条 running/starting 记录询问对应 kind：实现了 + `RestartSurvivor` 且 `Reattach` 成功 → 记录保持 running；否则标记 + stopped。 +- **唯一实现者是 reasonix**（`internal/instance/reasonix/kind.go:192`）： + serve 子进程用 `Setpgid` 启动（`reasonix/driver.go:274`），pid/port 落盘， + `Health` = `kill(pid,0)` + TCP 探测（`driver.go:531,555`）。myworktree 被 + kill -9 后 serve 活着，重启后拉回，框架照常管理它。 +- pty 不实现的原因（`docs/ARCHITECTURE.md:90`）：in-memory stdin/stdout 绑定 + 无法在进程重启后恢复；且 PTY 输出 ring buffer 是纯内存态 + （`ARCHITECTURE.md:100-102,129`），重启即丢，无法回放。 + +对照：reasonix 能活，是因为它**不需要 myworktree 持有的 PTY**（自己的日志 +文件 + 端口 + pid 文件）；pty 的生死绑在 master fd 上，问题无解于"重启后 +拉回"层，只能解于"master fd 所有权"层。 + +### 2.4 既存的孤儿面（顺带发现） + +- dsh-web kind：`Setpgid` 管道子进程（`dsh_web/driver.go:325`），Stop 杀进程 + 组（`driver.go:404-422`），但**没有实现 Reattach**（全仓库仅 reasonix 有）。 + myworktree 被 kill -9 后，dsh 进程会作为孤儿存活，重启时 + ReconcileRunningOnStartup 只能把记录标 stopped——进程仍占着端口和凭证。 +- 终端内 nohup/setsid 逃逸的进程：完全失联（见 2.2）。 + +这两类就是当前架构下"长期未回收"的真实存量。 + +## 3. 需求收敛 + +讨论收敛为一条：**myworktree 重启后，终端会话原样恢复、运行不中断**—— +包含 zsh 本体（环境变量、cwd、历史）、前台 TUI 程序（opencode cli）、屏幕 +内容与 scrollback。 + +## 4. 充要条件 + +只有一个根条件，其余都是它的推论： + +> **把 PTY master fd 的所有权从 myworktree 移到一个比 myworktree 命长的持 +> 有者。** + +推论为六条验收条件： + +| # | 条件 | 说明 | +|---|---|---| +| 1 | master 所有权转移 | 持有者必须 setsid、忽略 SIGHUP、独立于用户登录会话，命长于 myworktree | +| 2 | 会话身份可持久化 | 会话名 = 实例 id（如 `mw-`），可探活；写入 KindBlob（现 pty blob 为 `{}`，`pty/driver.go:237`） | +| 3 | 屏幕原样回放 | 回放 pane 内容 + scrollback（带属性，如 tmux `capture-pane -p -e -S -N`）喂进现有 RingBuffer；前端 `since` 游标重放契约（`ARCHITECTURE.md:231`）不变 | +| 4 | 字节流续接 | 重连后 output 继续喂 `pumpLogs`→RingBuffer→广播；input → 按键注入；resize → 设窗口尺寸 | +| 5 | Shutdown 语义反转 | 现在 reasonix/dsh 是显式杀（`app.go:707-709`）；pty 要改为 **退出 = 全体客户端断开，会话侧保留** | +| 6 | 前端零改动 | pty Kind 本就是 opaque Kind + Resize/SendInput/SubscribeOutput 扩展接口（`pty/driver.go:251,259,262`），换后端不动 WS/SSE/游标/心跳协议 | + +另需说明 `PDEATHSIG` 的方向：它保证"父死子必须死"（且内核关闭 fd 先于 +PDEATHSIG 投递），与内核 hangup 现在扮演的角色相同——只能用来消灭孤儿， +不能用来保活。保活必须移走 master，没有捷径。 + +## 5. 候选方案 + +### A. tmux（隔离 socket）— 推荐讨论主线 + +- 形态：`tmux -L mw-` 独立 socket（绝不碰用户自己的 tmux server）； + spawn = `new-session -d -s mw- zsh -f -i`，150ms 后 `send-keys` 注入 + tag 命令（对齐 `pty/driver.go:139-144` 现有行为）；runtime = control mode + （`tmux -C attach`）attach，`%output` 喂现有管道；restart = Reattach = + `has-session` 探活 + `capture-pane` 回放 + 重进 control mode。 +- 天然满足三条需求：**存活**（tmux server setsid、忽略 SIGHUP、不占任何终 + 端，myworktree 生死只是客户端来去）、**可追溯**（session 命名 + `tmux ls`）、 + **可拉回**（控制模式重连 + scrollback 持久化，顺带解决"ring buffer 重启 + 即丢"）。 +- Reattach 形状与 `reasonix.Kind.Reattach`（`reasonix/kind.go:192`）一致， + 只是探活从 pid+TCP 换成 tmux 查询。框架这条路已跑通过一次。 +- 代价：新增二进制依赖；control-mode 协议解析（`%begin/%end/%error`、 + `%output`、会话通知）是最大工程面；按键/括号粘贴映射；tmux 对输出字节做 + 了自身解释，个别 passthrough/sixel/OSC-8 特性与直连 pty 有差异。 +- 依赖管理：写稿时本开发机未安装 tmux（`which tmux` 为空）。需像 dsh 的 + `exec.LookPath` 预检一样处理缺失（结构化错误 + 前端引导），不建议保留 + in-process pty 做双后端 fallback（两套后端的维护成本高于一个预检）。 + +### B. 自研 terminal supervisor + +每实例一个 setsid 守护进程持有 PTY，myworktree 经 unix socket 转发 I/O。形态 +上是 tmux 的最小子集，但 scrollback 持久化、多客户端、会话回收全部自踩一遍 +tmux 踩过的坑。仅当需要 tmux 给不了的深度集成（如 ring buffer 与实例状态强 +绑定）才考虑，否则不推荐。 + +### C. 保持现状 + 回收闭环（不改存活语义） + +承认"终端随 daemon 生死"是设计语义，只补回收：注入 `MYWORKTREE_INSTANCE=` +env（`/proc/*/environ` 可反查归属）；启动时对持久化 pid 做 starttime +校验（防 pid 复用）后杀进程组；Shutdown 显式 `StopAllKind(pty)` 把逃逸口焊 +死；dsh-web 补 Reattach 或启动时强制清理。长期跑 agent 的需求走 dsh-web +kind（detached + reverse proxy 模式）。 + +### D. systemd --user scope / cgroup（作为 A/C 的补充） + +解决不了 master fd 问题，但给每实例独立 scope/cgroup：按名字批量杀、资源 +计量、崩溃后可寻回。是"可追溯 + 可回收"的 Linux 原生答案，与 A/C 正交。 + +## 6. "不中断"的边界 + +**能保证**（方案 A）：zsh 本体（环境变量、cwd、历史）、前台 opencode TUI、 +其网络连接全部存活——终端里的 shell 不依赖 myworktree 的任何东西，这正是终 +端比 reasonix serve（需注入 env/token）更容易做存活的原因。myworktree 优雅 +退出与被 kill -9 在方案 A 下等价（都是摘除客户端）。 + +**不能保证**： +- 无客户端期间窗口尺寸停在旧值（重连时统一 resize，ncurses 通常自愈）； +- control-mode 的 `%output` 是 tmux 解释渲染后的字节流，个别 + passthrough/sixel/OSC-8 特性与直连 pty 有细微差异； +- 粘贴中途、半截输入等瞬时状态； +- 依赖 myworktree 自身环境/连接的进程例外（终端场景不存在此问题）； +- 登出即杀全部用户进程的环境（systemd `KillUserProcesses=yes`）下 tmux + server 同样活不了——那种场景退回现状行为，可接受。 + +## 7. 长期未回收风险（若允许存活） + +1. **失控代理**：opencode cli 无人值守继续烧 token、改文件、动 git worktree， + 而用户以为"关了"。会话活着 ≠ 用户知道它活着。 +2. **日志账**：跨重启可回看就得把 ring buffer 落盘——项目刚因 10MB 文件截断 + 造成 ~100,000× 写放大而删掉磁盘日志（`ARCHITECTURE.md:100-102`），不能 + 重蹈覆辙。 +3. **单点与升级 skew**：supervisor/tmux server 成为单点；老会话被新版本 + attach 的协议兼容、resize/redaction 语义差异。 +4. **僵尸会话**：shell 已死但会话还在，Status 报 running 实则无人。 +5. **Stop 路径必须穿过持有者**：否则 `kill -9 myworktree` 后再没人管这棵 + 树——detached 架构最经典的泄漏。 +6. **与 worktree 生命周期打架**：删 worktree 时里面有活进程 → git 删不掉、 + 进程 hold 着已删除的 cwd 与软链凭证（reasonix 文档中 + "restart-then-delete 把持凭证的 serve 变孤儿"是缩小版）。 +7. **假 reattach**：pid 复用导致探活误判——pidfd 或 `/proc//stat` + starttime 校验是必配。 +8. **socket 安全**：隔离 socket 的目录权限（多用户机器必须私有化）。 + +## 8. 待决策分叉点（本文档不决策） + +1. "原样"的边界：只要求活着的 shell + TUI + 当前屏幕（`capture-pane` 够 + 用），还是包含完整 scrollback 历史（直接从持有者侧取，别让 myworktree 的 + RingBuffer/RAM 预算扛）？——决定回放源设计。 +2. tmux 是硬依赖 + 预检，还是保留 in-process pty 做 fallback（双后端）？ +3. Shutdown 改 detach 后，Stop/Delete/Restart 对会话侧的新语义（谁负责 + `kill-session`）。 +4. 孤儿回收策略：shell 已死的会话、用户手动 `kill-server` 后的状态漂移。 +5. C 方案（现状 + 闭环）是否作为 A 落地前的过渡step先做——尤其 dsh-web 的 + Reattach 缺口本身就是一个应当独立修复的缺陷。 + +## 9. 附注 + +- 写稿时开发环境未安装 tmux（`which tmux` 为空），依赖可用性是方案 A 的第 + 一道 gate。 +- 本文档为纯讨论记录，不修改任何代码。 diff --git a/internal/app/app.go b/internal/app/app.go index ae48004..9544670 100644 --- a/internal/app/app.go +++ b/internal/app/app.go @@ -1131,16 +1131,59 @@ func (s *Server) handleWorktreeDelete(w http.ResponseWriter, r *http.Request) { } var req struct { ID string `json:"id"` + // Force deletes even with uncommitted/untracked changes (issue + // #102). The refusal answer lists what force would destroy — + // including gitignored files the dirty check never blocks on — + // so a client that resends with force has seen the cost. + Force bool `json:"force"` } if err := readJSON(r.Body, &req); err != nil { writeErr(w, http.StatusBadRequest, err) return } - if err := s.worktreeMgr.Delete(req.ID); err != nil { + ignoredDestroyed, err := s.worktreeMgr.Delete(req.ID, req.Force) + if err != nil { + var dirty *worktree.DirtyWorktreeError + if errors.As(err, &dirty) { + writeWorktreeDirtyErr(w, dirty) + return + } writeErr(w, http.StatusBadRequest, err) return } - writeJSON(w, http.StatusOK, map[string]string{"status": "ok"}) + // ignored_destroyed is present only when the count is meaningful: the + // force path skips the status probe, so there a 0 would read as "none + // destroyed" while really meaning "not counted" (second review round). + resp := map[string]any{"status": "ok"} + if !req.Force { + resp["ignored_destroyed"] = ignoredDestroyed + } + writeJSON(w, http.StatusOK, resp) +} + +// writeWorktreeDirtyErr writes the structured 409 answer for a refused +// worktree delete (issue #102). The "error" code "worktree_dirty" is part +// of the API contract — the dashboard matches on it to open the +// dirty-details dialog (summary, collapsible full git status output, +// gitignored-file warning, and the force-delete exit) instead of a +// generic alert. "message" carries the same one-line summary for clients +// that only surface error text. +func writeWorktreeDirtyErr(w http.ResponseWriter, dirty *worktree.DirtyWorktreeError) { + writeJSON(w, http.StatusConflict, map[string]any{ + "error": "worktree_dirty", + "message": dirty.Error(), + "dirty": map[string]any{ + "entries": dirty.Entries, + "deleted": dirty.Deleted, + "modified": dirty.Modified, + "added": dirty.Added, + "renamed": dirty.Renamed, + "untracked": dirty.Untracked, + "ignored": dirty.Ignored, + "first_paths": dirty.FirstPaths, + "porcelain": dirty.Porcelain, + }, + }) } // resolveWorktreePath returns the filesystem path for a worktree id. @@ -2359,6 +2402,43 @@ func (s *Server) handleInstanceTTYWSLiveness(w http.ResponseWriter, r *http.Requ // OTHER unknown-cursor state — the screen IS painted, only its end // offset is unknown — and is handled inside completeHandshake. since := parseInt64Default(r.URL.Query().Get("since"), -1) + // Client capability flags (issue #98), a comma-separated list. The + // parameter may be REPEATED (?caps=pong&caps=ping) — url.Values.Get + // would return only the first value, so the raw slice is scanned. + // Defined flags: + // "ping": the client whitelists the TEXT {"type":"ping"} heartbeat + // in parseTTYControlMessage (stamps its liveness clock, never + // paints it), so the server may send it. A client that does NOT + // opt in — a page loaded before the whitelist shipped (v0.5.0), + // any non-browser client — is never sent the text frame, because + // such a client would paint it into the terminal as literal + // output every ttyPingInterval. The RFC 6455 ping below is + // unaffected: control frames never reach page JS, so it is + // always safe. + // "sync": the client whitelists the {"type":"sync"} offset echo + // and drives its reconnect cursor off it (issue #87). The echo + // has a second, INFERRED opt-in: an explicit `since` parameter. + // The cursor parameter and the sync whitelist shipped in the + // same fix (#87, v0.5.1) — v0.5.0 never puts `since` on this + // endpoint at all — so a client presenting one provably parses + // the other, and the inference costs exactly nothing against + // the one population that would paint the frame. It keeps + // pre-caps v0.5.1 pages on the cursor contract: without the + // echo their handshakeSynced latch never sets, the cursor + // freezes, and every reconnect re-replays from it — a + // duplicate-paint regression strictly worse than the leak + // (review round 2). Residual: a pre-caps page whose cursor is + // still the unknown/zero sentinel sends no `since` either, so + // it gets no echo and its reconnects re-replay the ≤64KB tail + // until a loadLog re-pin or a refresh lands a cursor — the + // pre-#87 behaviour that page shipped with. + // This is deliberately opt-IN, not version-gated: the server can + // never again leak a future visible control frame to a client that + // did not declare it understands it. + caps := r.URL.Query()["caps"] + textPingCap := ttyClientHasCap(caps, "ping") + _, sincePresent := r.URL.Query()["since"] + syncCap := ttyClientHasCap(caps, "sync") || sincePresent conn, err := ws.Upgrade(w, r) if err != nil { return @@ -2445,6 +2525,21 @@ func (s *Server) handleInstanceTTYWSLiveness(w http.ResponseWriter, r *http.Requ // cursor parsed from the query string) rather than taking a parameter; // both call sites below pass that same variable. // + // On the streaming catch-up path the replay is bracketed by TWO sync + // frames (issue #94): one carrying the replay's real start offset S + // ahead of the FIRST binary chunk — S = first chunk's next - len, the + // clamped value whenever ReadSince clamped a stale cursor, never the + // raw request — and the closing one carrying the end offset of the last + // chunk written. The start frame lifts the client's handshakeSynced + // latch at chunk 1, so a mid-replay drop resumes from the bytes already + // painted instead of re-pulling the whole replay; the closing frame + // keeps the #87 invariant below and corrects any drift absolutely. The + // beyond-head tail degrade (cursor > head) announces its tail's start + // the same way — the client presented a cursor, so every replay chunk + // addressed to one is preceded by a start frame; only the CURSOR-LESS + // first-connect tail goes without (its single ≤64KB frame has the + // pre-#87 death window, and such a client may not parse sync at all). + // // Three cursors, three modes: a real `since >= 0` replays only [since, // head); the -1 sentinel (nothing painted) replays the tail; and the // sinceFollowLiveEnd sentinel (-2, painted but offset unknown) replays @@ -2547,6 +2642,45 @@ func (s *Server) handleInstanceTTYWSLiveness(w http.ResponseWriter, r *http.Requ cursor := since first := true var streamed int64 + // startPublished (issue #94) marks that the replay's real + // START offset has been announced to the client, so it can + // advance its cursor frame-by-frame from there instead of + // waiting for the closing sync frame. It is deliberately NOT + // `first`: the FIX-F consult below clears `first` before any + // chunk has been written, while the start offset must be + // published exactly once, ahead of the first CHUNK on the + // wire. + startPublished := false + // emitStartSync announces a replay's real START offset + // (issue #94) ahead of its first binary chunk, lifting the + // client's handshakeSynced latch at chunk 1 so a mid-replay + // drop resumes from the bytes already painted instead of + // re-pulling the whole (up to 8MB) replay. The frame REUSES + // the "sync" type rather than inventing a new one: the + // client's whitelist (parseTTYControlMessage) paints any + // unknown text frame into the terminal (the issue #98 leak + // class), and the existing handler — set cursor to the + // ABSOLUTE offset, lift the latch — is exactly the semantics + // wanted here. The "start":true marker lets a consumer that + // must know WHEN the replay ends (a test harness, a + // third-party client) tell this announcement apart from the + // closing echo; the in-repo client deliberately ignores it — + // every sync is an absolute offset to adopt. + // + // syncCap is PROVABLY true at both call sites — they sit in + // the catch-up branch, which requires since >= 0, i.e. an + // explicit `since` in the query, already the inferred opt-in + // half of syncCap (issue #98; review Minor-1). The guard + // stays so a future change to that inference can never leak + // the frame to a client that did not opt in — defensive + // depth, not an option any client has today. + emitStartSync := func(start int64) bool { + if !syncCap { + return true + } + payload := []byte(`{"type":"sync","offset":` + strconv.FormatInt(start, 10) + `,"start":true}`) + return conn.WriteText(payload) == nil + } for { if streamed >= ttyHandshakeReplayBudget { // Budget exhausted — the ONE budget guard, checked @@ -2614,8 +2748,19 @@ func (s *Server) handleInstanceTTYWSLiveness(w http.ResponseWriter, r *http.Requ // ever appeared it must degrade to the // first-connect tail, never to a cursor // the ring never held. Write the tail - // before claiming head over it. + // before claiming head over it. Unlike + // the CURSOR-LESS tail replay above, this + // tail rides the issue #94 contract: the + // client presented a cursor, so announce + // the tail's real start S = head - len + // ahead of the chunk and let its latch + // lift at a truthful absolute base — the + // closing sync then republishes the same + // head the frames summed to. if tail != "" { + if !emitStartSync(head - int64(len(tail))) { + return false + } if err := conn.WriteBinary([]byte(tail)); err != nil { return false } @@ -2657,6 +2802,32 @@ func (s *Server) handleInstanceTTYWSLiveness(w http.ResponseWriter, r *http.Requ // Write as we read: the chunk is on the wire now, so the // cursor published below is never ahead of it, and nothing // older than the next read is retained in memory. + if !startPublished { + startPublished = true + // S = next - len(body) is the true start of THIS + // chunk — the requested `since` when no clamp + // happened, the clamped oldest live byte when it + // did — because S and the first chunk come from the + // SAME ReadSince call, so no new Manager surface is + // needed and a stale `since` can never be echoed + // raw. The closing sync below still arrives with + // the end offset, so a client that only understands + // sync-at-the-end is merely back to the pre-#94 + // death window, and a misbehaving kind that returns + // a body without advancing the cursor (the + // defensive break below) is corrected by that same + // final absolute value. One corner on THAT + // defensive path (review Minor-2): next <= cursor + // there makes S = next - len(body) dip BELOW the + // client's own `since`, so a drop between this + // frame and the first chunk reconnects with a + // regressed cursor and re-renders [S, since) — + // bounded, self-healing, and still strictly better + // than the pre-#94 full re-pull it replaces. + if !emitStartSync(next - int64(len(body))) { + return false + } + } if err := conn.WriteBinary([]byte(body)); err != nil { return false } @@ -2683,9 +2854,20 @@ func (s *Server) handleInstanceTTYWSLiveness(w http.ResponseWriter, r *http.Requ // and every other empty-replay path set a truthful head above. // Built with strconv (not json.Marshal) so there is no // unreachable error branch. - syncPayload := []byte(`{"type":"sync","offset":` + strconv.FormatInt(endOffset, 10) + `}`) - if err := conn.WriteText(syncPayload); err != nil { - return false + // + // OPT-IN (the issue #98 orientation): the echo is itself visible + // control traffic that post-dates the v0.5.0 whitelist, so an + // un-refreshed v0.5.0 page would paint it into the terminal on + // every connect — the same leak as the TEXT ping, once per + // connect instead of once per tick. It goes only to a client + // that declared "sync" in caps or presented an explicit `since` + // cursor (the two shipped together in #87 — see the handshake + // comment above); v0.5.0 does neither and never receives it. + if syncCap { + syncPayload := []byte(`{"type":"sync","offset":` + strconv.FormatInt(endOffset, 10) + `}`) + if err := conn.WriteText(syncPayload); err != nil { + return false + } } ch, cancelFn, err := s.instanceMgr.SubscribeOutput(id) @@ -2771,15 +2953,23 @@ func (s *Server) handleInstanceTTYWSLiveness(w http.ResponseWriter, r *http.Requ // Invisible to page JS (onmessage never fires for control // frames), hence frame 2: // 2. TEXT {"type":"ping"} — the heartbeat the client can - // actually observe and stamp lastDataAt from. + // actually observe and stamp lastDataAt from. OPT-IN ONLY + // (issue #98): sent solely to clients whose `caps` + // handshake parameter listed "ping" — a client without + // the whitelist (a pre-refresh v0.5.0 page, any older or + // non-browser client) would paint this frame into the + // terminal every 10s, so it gets frame 1 only and + // degrades to the pre-#83 behaviour it always had. // A failed write means the peer is gone: return and let the // existing defers unwind (same treatment as the binary output // write below). if err := conn.WritePing(nil); err != nil { return } - if err := conn.WriteText([]byte(`{"type":"ping"}`)); err != nil { - return + if textPingCap { + if err := conn.WriteText([]byte(`{"type":"ping"}`)); err != nil { + return + } } case <-handshakeTimer.C: @@ -3101,16 +3291,35 @@ func (s *Server) handleMCPCall(w http.ResponseWriter, r *http.Request) { case "worktree_delete": var args struct { ID string `json:"id"` + // Force: same semantics as /api/worktrees/delete (issue + // #102) — delete even with uncommitted/untracked changes. + Force bool `json:"force"` } if err := decodeArgs(req.Args, &args); err != nil { writeErr(w, http.StatusBadRequest, err) return } - if err := s.worktreeMgr.Delete(args.ID); err != nil { + ignoredDestroyed, err := s.worktreeMgr.Delete(args.ID, args.Force) + if err != nil { + // Same structured refusal as /api/worktrees/delete (409 + + // worktree_dirty + breakdown), so MCP clients can recognize + // the dirty state programmatically instead of parsing a flat + // 400 message (issue #102 review). + var dirty *worktree.DirtyWorktreeError + if errors.As(err, &dirty) { + writeWorktreeDirtyErr(w, dirty) + return + } writeErr(w, http.StatusBadRequest, err) return } - writeJSON(w, http.StatusOK, map[string]any{"result": map[string]string{"status": "ok"}}) + // As in the HTTP handler, ignored_destroyed is present only when + // counted (the force path skips the probe). + result := map[string]any{"status": "ok"} + if !args.Force { + result["ignored_destroyed"] = ignoredDestroyed + } + writeJSON(w, http.StatusOK, map[string]any{"result": result}) case "branch_list": def, out, err := s.listTopBranches() if err != nil { @@ -3408,6 +3617,23 @@ func parseInt64Default(s string, def int64) int64 { return v } +// ttyClientHasCap reports whether the tty WS handshake's `caps` query +// parameter — a comma-separated capability list (issue #98) that may be +// REPEATED, ?caps=pong&caps=ping — contains name as a WHOLE, +// case-sensitive token. Substring matching is deliberately not enough: +// "pinger" must not opt a client into the TEXT {"type":"ping"} heartbeat, +// and "PING" names no capability. +func ttyClientHasCap(caps []string, name string) bool { + for _, v := range caps { + for _, tok := range strings.Split(v, ",") { + if strings.TrimSpace(tok) == name { + return true + } + } + } + return false +} + func decodeArgs(raw json.RawMessage, out any) error { if len(raw) == 0 { raw = []byte("{}") diff --git a/internal/app/app_worktree_delete_test.go b/internal/app/app_worktree_delete_test.go new file mode 100644 index 0000000..bb47323 --- /dev/null +++ b/internal/app/app_worktree_delete_test.go @@ -0,0 +1,229 @@ +package app + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" + + "myworktree/internal/store" + "myworktree/internal/worktree" +) + +// setupDeleteTestWorktree builds a real git repo with one managed worktree +// registered in a state file — the worktree delete handler shells out to +// git, so only a real repo exercises the refusal path honestly. +func setupDeleteTestWorktree(t *testing.T) (srv *Server, fs store.FileStore, wtPath string) { + t.Helper() + if _, err := exec.LookPath("git"); err != nil { + t.Skip("git is required") + } + repo := t.TempDir() + runGitDeleteTest(t, repo, "init") + runGitDeleteTest(t, repo, "config", "user.name", "Test User") + runGitDeleteTest(t, repo, "config", "user.email", "test@example.com") + if err := os.WriteFile(filepath.Join(repo, "README.md"), []byte("init\n"), 0o600); err != nil { + t.Fatalf("write seed file: %v", err) + } + runGitDeleteTest(t, repo, "add", "README.md") + runGitDeleteTest(t, repo, "commit", "-m", "init") + + wtPath = filepath.Join(t.TempDir(), "wt") + runGitDeleteTest(t, repo, "worktree", "add", wtPath, "-b", "wt-branch") + + dataDir := t.TempDir() + fs = store.FileStore{Path: filepath.Join(dataDir, "state.json")} + if err := fs.Save(store.State{ + Worktrees: []store.ManagedWorktree{{ID: "wt1", Name: "wt1", Path: wtPath, Branch: "wt-branch"}}, + }); err != nil { + t.Fatalf("save state: %v", err) + } + srv = &Server{worktreeMgr: worktree.Manager{GitRoot: repo, DataDir: dataDir, Store: fs}} + return srv, fs, wtPath +} + +func runGitDeleteTest(t *testing.T, dir string, args ...string) { + t.Helper() + cmd := exec.Command("git", args...) + cmd.Dir = dir + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("git %v failed: %v (%s)", args, err, out) + } +} + +// makeDeleteTestWorktreeDirty reproduces the issue #102 shape: a tracked +// file deleted, an untracked scratch file, and a gitignored file that the +// blocking check must not count but the refusal must warn about. +func makeDeleteTestWorktreeDirty(t *testing.T, wtPath string) { + t.Helper() + if err := os.Remove(filepath.Join(wtPath, "README.md")); err != nil { + t.Fatalf("remove tracked file: %v", err) + } + if err := os.WriteFile(filepath.Join(wtPath, "scratch.txt"), []byte("x\n"), 0o600); err != nil { + t.Fatalf("write scratch: %v", err) + } + if err := os.WriteFile(filepath.Join(wtPath, ".gitignore"), []byte("*.log\n"), 0o600); err != nil { + t.Fatalf("write .gitignore: %v", err) + } + if err := os.WriteFile(filepath.Join(wtPath, "debug.log"), []byte("log\n"), 0o600); err != nil { + t.Fatalf("write ignored file: %v", err) + } +} + +// TestHandleWorktreeDeleteDirtyRefusalIsStructured pins the issue #102 API +// contract: a dirty-worktree refusal answers 409 with error +// "worktree_dirty" and the full breakdown the dialog renders — counts by +// category, first paths, the full porcelain output, and the gitignored +// force-risk count. The flat "delete is refused" message made a damaged +// workspace indistinguishable from a leftover scratch file. +func TestHandleWorktreeDeleteDirtyRefusalIsStructured(t *testing.T) { + srv, fs, wtPath := setupDeleteTestWorktree(t) + makeDeleteTestWorktreeDirty(t, wtPath) + + req := httptest.NewRequest(http.MethodPost, "/api/worktrees/delete", strings.NewReader(`{"id":"wt1"}`)) + w := httptest.NewRecorder() + srv.handleWorktreeDelete(w, req) + + if w.Code != http.StatusConflict { + t.Fatalf("status = %d, want 409: %s", w.Code, w.Body.String()) + } + var body map[string]any + if err := json.Unmarshal(w.Body.Bytes(), &body); err != nil { + t.Fatalf("invalid JSON: %v (%s)", err, w.Body.String()) + } + if body["error"] != "worktree_dirty" { + t.Fatalf("error = %v, want worktree_dirty (the dashboard matches on this code)", body["error"]) + } + if msg, _ := body["message"].(string); !strings.Contains(msg, "3 entries") || !strings.Contains(msg, "retry with force") { + t.Fatalf("message = %q, want the one-line summary with counts and the force exit", msg) + } + dirty, ok := body["dirty"].(map[string]any) + if !ok { + t.Fatalf("dirty details missing: %s", w.Body.String()) + } + // " D README.md" is the deletion; "?? .gitignore" + "?? scratch.txt" + // are untracked; debug.log is ignored — a force risk, NOT a blocker. + num := func(key string) float64 { + v, _ := dirty[key].(float64) + return v + } + if num("entries") != 3 || num("deleted") != 1 || num("untracked") != 2 || num("ignored") != 1 { + t.Fatalf("dirty = entries:%v deleted:%v untracked:%v ignored:%v, want 3/1/2/1", + num("entries"), num("deleted"), num("untracked"), num("ignored")) + } + if paths, _ := dirty["first_paths"].([]any); len(paths) == 0 { + t.Fatalf("first_paths must name example paths: %v", dirty) + } + if porc, _ := dirty["porcelain"].(string); !strings.Contains(porc, "README.md") { + t.Fatalf("porcelain must carry the full git status output: %q", porc) + } + + // The refusal changed nothing: the state record survives. + st, err := fs.Load() + if err != nil { + t.Fatalf("load state: %v", err) + } + if len(st.Worktrees) != 1 { + t.Fatalf("refused delete must keep the state record, got %#v", st.Worktrees) + } +} + +// TestHandleWorktreeDeleteForceDeletes pins the force exit (issue #102 P2): +// resending with force:true removes the dirty worktree and drops its +// record — previously the only way out was a manual `git worktree remove +// --force` the daemon never noticed. +func TestHandleWorktreeDeleteForceDeletes(t *testing.T) { + srv, fs, wtPath := setupDeleteTestWorktree(t) + makeDeleteTestWorktreeDirty(t, wtPath) + + req := httptest.NewRequest(http.MethodPost, "/api/worktrees/delete", strings.NewReader(`{"id":"wt1","force":true}`)) + w := httptest.NewRecorder() + srv.handleWorktreeDelete(w, req) + + if w.Code != http.StatusOK { + t.Fatalf("status = %d, want 200: %s", w.Code, w.Body.String()) + } + // The force path skips the status probe, so ignored_destroyed must be + // ABSENT (not 0): 0 would read as "none destroyed" while really + // meaning "not counted" (second review round). + var okBody map[string]any + if err := json.Unmarshal(w.Body.Bytes(), &okBody); err != nil { + t.Fatalf("invalid JSON: %v (%s)", err, w.Body.String()) + } + if _, present := okBody["ignored_destroyed"]; present { + t.Fatalf("force success must omit ignored_destroyed (the probe was skipped), got %s", w.Body.String()) + } + if _, err := os.Stat(wtPath); !os.IsNotExist(err) { + t.Fatalf("force delete should remove the path, err=%v", err) + } + st, err := fs.Load() + if err != nil { + t.Fatalf("load state: %v", err) + } + if len(st.Worktrees) != 0 { + t.Fatalf("force delete should drop the state record, got %#v", st.Worktrees) + } +} + +// TestHandleWorktreeDeleteCleanReportsIgnoredDestroyed pins the issue #102 +// review follow-up: a worktree whose ONLY at-risk contents are gitignored +// files deletes without a refusal (nothing blocks), but the success response +// must carry the destroyed gitignored count so no client lets it pass +// silently. +func TestHandleWorktreeDeleteCleanReportsIgnoredDestroyed(t *testing.T) { + srv, _, wtPath := setupDeleteTestWorktree(t) + if err := os.WriteFile(filepath.Join(wtPath, ".gitignore"), []byte("*.log\n"), 0o600); err != nil { + t.Fatalf("write .gitignore: %v", err) + } + runGitDeleteTest(t, wtPath, "add", ".gitignore") + runGitDeleteTest(t, wtPath, "commit", "-m", "add gitignore") + if err := os.WriteFile(filepath.Join(wtPath, "debug.log"), []byte("log\n"), 0o600); err != nil { + t.Fatalf("write ignored: %v", err) + } + + req := httptest.NewRequest(http.MethodPost, "/api/worktrees/delete", strings.NewReader(`{"id":"wt1"}`)) + w := httptest.NewRecorder() + srv.handleWorktreeDelete(w, req) + + if w.Code != http.StatusOK { + t.Fatalf("status = %d, want 200: %s", w.Code, w.Body.String()) + } + var body map[string]any + if err := json.Unmarshal(w.Body.Bytes(), &body); err != nil { + t.Fatalf("invalid JSON: %v (%s)", err, w.Body.String()) + } + if n, _ := body["ignored_destroyed"].(float64); n != 1 { + t.Fatalf("ignored_destroyed = %v, want 1 (debug.log) — a clean delete must not erase gitignored files silently", body["ignored_destroyed"]) + } +} + +// TestHandleMCPWorktreeDeleteDirtyRefusalIsStructured pins the issue #102 +// review fix: the MCP worktree_delete verb answers a dirty refusal with the +// SAME 409 + worktree_dirty + breakdown shape as /api/worktrees/delete, not +// a flat 400 MCP clients cannot recognize programmatically. +func TestHandleMCPWorktreeDeleteDirtyRefusalIsStructured(t *testing.T) { + srv, _, wtPath := setupDeleteTestWorktree(t) + makeDeleteTestWorktreeDirty(t, wtPath) + + req := httptest.NewRequest(http.MethodPost, "/mcp/call", strings.NewReader(`{"tool":"worktree_delete","args":{"id":"wt1"}}`)) + w := httptest.NewRecorder() + srv.handleMCPCall(w, req) + + if w.Code != http.StatusConflict { + t.Fatalf("status = %d, want 409 (same as the HTTP API): %s", w.Code, w.Body.String()) + } + var body map[string]any + if err := json.Unmarshal(w.Body.Bytes(), &body); err != nil { + t.Fatalf("invalid JSON: %v (%s)", err, w.Body.String()) + } + if body["error"] != "worktree_dirty" { + t.Fatalf("error = %v, want worktree_dirty", body["error"]) + } + if _, ok := body["dirty"].(map[string]any); !ok { + t.Fatalf("MCP refusal must carry the structured dirty breakdown: %s", w.Body.String()) + } +} diff --git a/internal/app/tty_ws_test.go b/internal/app/tty_ws_test.go index e90de57..1407967 100644 --- a/internal/app/tty_ws_test.go +++ b/internal/app/tty_ws_test.go @@ -447,16 +447,34 @@ func expectCloseFrame(t *testing.T, c *ws.Conn, code uint16, wantReason string) } // dialHandshakeFrames dials, completes the handshake with the first -// resize, and collects the replay up to the `sync` echo — but keeps the -// binary frames SEPARATE, so a test can pin how the replay was chunked +// resize, and collects the replay up to the CLOSING `sync` echo — but keeps +// the binary frames SEPARATE, so a test can pin how the replay was chunked // (each frame is one streamed ring-buffer read, never the whole delta // re-buffered) without an extra concatenation copy. func dialHandshakeFrames(t *testing.T, addr, path string) (c *ws.Conn, frames [][]byte, syncOffset int64) { + t.Helper() + c, frames, _, syncOffset = dialHandshakeStart(t, addr, path) + return c, frames, syncOffset +} + +// dialHandshakeStart is dialHandshakeFrames plus the replay's START offset +// from the issue #94 start sync frame (-1 when none arrived: the tail +// replay, the empty-replay consult exits and the sinceFollowLiveEnd path +// emit only the closing sync). On the streaming catch-up path the replay is +// bracketed by TWO sync frames — {"type":"sync","offset":S,"start":true} +// ahead of the first binary chunk and the plain closing echo after the last +// one — so the loop below stops only at a sync WITHOUT the start marker, +// and pins the wire invariant that the start announcement precedes every +// replay chunk. The "start" field itself is ignored by the in-repo client +// (every sync carries an absolute offset to adopt); it exists so consumers +// that must know WHEN the replay ends can tell the two frames apart. +func dialHandshakeStart(t *testing.T, addr, path string) (c *ws.Conn, frames [][]byte, startOffset, syncOffset int64) { t.Helper() conn := dialTTY(t, addr, path) sendResize(t, conn) + startOffset = -1 sawSync := false for !sawSync { op, p := readFrame(t, conn, 5*time.Second) @@ -473,12 +491,23 @@ func dialHandshakeFrames(t *testing.T, addr, path string) (c *ws.Conn, frames [] var ctl struct { Type string `json:"type"` Offset int64 `json:"offset"` + Start bool `json:"start"` } if err := json.Unmarshal(p, &ctl); err != nil { t.Fatalf("control frame %q is not JSON: %v", p, err) } switch ctl.Type { case "sync": + if ctl.Start { + if startOffset != -1 { + t.Fatalf("duplicate start sync frame: %q", p) + } + if len(frames) != 0 { + t.Fatalf("start sync arrived AFTER %d replay chunks — it must precede the first binary frame: %q", len(frames), p) + } + startOffset = ctl.Offset + continue + } syncOffset = ctl.Offset sawSync = true case "resize": @@ -494,7 +523,7 @@ func dialHandshakeFrames(t *testing.T, addr, path string) (c *ws.Conn, frames [] t.Fatalf("unexpected opcode %d during handshake", op) } } - return conn, frames, syncOffset + return conn, frames, startOffset, syncOffset } // dialHandshake dials the TTY endpoint, consumes the ready frame, sends @@ -542,7 +571,7 @@ func TestHandleInstanceTTYWS_ReconnectWithSinceDoesNotRedeliverSeenBytes(t *test addr, _, instID, _ := ttyWSTestServer(t, k) k.buf.WriteString("HELLO-TAIL") // 10 bytes - c1, replay1, sync1 := dialHandshake(t, addr, ttyWSPath(instID, "")) + c1, replay1, sync1 := dialHandshake(t, addr, ttyWSPath(instID, "caps=sync")) if replay1 != "HELLO-TAIL" { t.Fatalf("first handshake replay = %q, want HELLO-TAIL", replay1) } @@ -555,7 +584,7 @@ func TestHandleInstanceTTYWS_ReconnectWithSinceDoesNotRedeliverSeenBytes(t *test _ = c1.WriteClose(ws.CloseMessage(1000, "bye")) _ = c1.Close() - c2, replay2, sync2 := dialHandshake(t, addr, ttyWSPath(instID, "since=10")) + c2, replay2, sync2 := dialHandshake(t, addr, ttyWSPath(instID, "since=10&caps=sync")) _ = c2.WriteClose(ws.CloseMessage(1000, "bye")) _ = c2.Close() if strings.Contains(replay2, "HELLO-TAIL") { @@ -578,7 +607,7 @@ func TestHandleInstanceTTYWS_OmittedSinceReplaysTail(t *testing.T) { addr, _, instID, _ := ttyWSTestServer(t, k) k.buf.WriteString("tail-body") // 9 bytes - for _, query := range []string{"", "since=", "since=bogus"} { + for _, query := range []string{"caps=sync", "since=&caps=sync", "since=bogus&caps=sync"} { replay, syncOffset := func() (string, int64) { c, replay, off := dialHandshake(t, addr, ttyWSPath(instID, query)) _ = c.WriteClose(ws.CloseMessage(1000, "bye")) @@ -610,7 +639,7 @@ func TestHandleInstanceTTYWS_StaleSinceClampsSilently(t *testing.T) { k.buf.WriteString(full.String()) // head 100, oldest live byte at 36 replay, syncOffset := func() (string, int64) { - c, replay, off := dialHandshake(t, addr, ttyWSPath(instID, "since=10")) + c, replay, off := dialHandshake(t, addr, ttyWSPath(instID, "since=10&caps=sync")) _ = c.WriteClose(ws.CloseMessage(1000, "bye")) _ = c.Close() return replay, off @@ -646,7 +675,7 @@ func TestHandleInstanceTTYWS_LargeDeltaStreamsEveryByteSinceCursor(t *testing.T) content := full.String() k.buf.WriteString(content[:100]) - c1, replay1, sync1 := dialHandshake(t, addr, ttyWSPath(instID, "")) + c1, replay1, sync1 := dialHandshake(t, addr, ttyWSPath(instID, "caps=sync")) if replay1 != content[:100] || sync1 != 100 { t.Fatalf("first handshake: replay %d bytes / sync %d, want 100/100", len(replay1), sync1) } @@ -656,7 +685,7 @@ func TestHandleInstanceTTYWS_LargeDeltaStreamsEveryByteSinceCursor(t *testing.T) _ = c1.WriteClose(ws.CloseMessage(1000, "bye")) _ = c1.Close() - c2, frames2, sync2 := dialHandshakeFrames(t, addr, ttyWSPath(instID, "since=100")) + c2, frames2, sync2 := dialHandshakeFrames(t, addr, ttyWSPath(instID, "since=100&caps=sync")) _ = c2.WriteClose(ws.CloseMessage(1000, "bye")) _ = c2.Close() @@ -723,10 +752,17 @@ func TestHandleInstanceTTYWS_ReplayBudgetTruncatesAndPublishesLastWrittenOffset( k.buf.Write(delta) // head = budget + 4 chunks head := k.buf.Offset() - c, frames, syncOffset := dialHandshakeFrames(t, addr, ttyWSPath(instID, "since=0")) + c, frames, startOffset, syncOffset := dialHandshakeStart(t, addr, ttyWSPath(instID, "since=0&caps=sync")) _ = c.WriteClose(ws.CloseMessage(1000, "bye")) _ = c.Close() + // The start sync (issue #94) opened the replay at the requested cursor — + // no clamp below the oldest live byte applies here — and the client's + // frame-by-frame advance from it must land exactly on the closing offset. + if startOffset != 0 { + t.Fatalf("start sync offset = %d, want 0 (the replay starts at the requested cursor)", startOffset) + } + var delivered int64 for i, f := range frames { if int64(len(f)) > 64*1024 { @@ -747,6 +783,9 @@ func TestHandleInstanceTTYWS_ReplayBudgetTruncatesAndPublishesLastWrittenOffset( if len(frames) != int(ttyHandshakeReplayBudget/(64*1024)) { t.Fatalf("replay arrived as %d frames, want %d full 64KB chunks", len(frames), ttyHandshakeReplayBudget/(64*1024)) } + if startOffset+delivered != syncOffset { + t.Fatalf("start %d + delivered %d != sync offset %d — advancing the cursor per frame from the start offset must land exactly on the closing offset", startOffset, delivered, syncOffset) + } // The published cursor is the end of the LAST chunk written, behind // head: the remainder stays re-requestable. if syncOffset != ttyHandshakeReplayBudget { @@ -758,12 +797,20 @@ func TestHandleInstanceTTYWS_ReplayBudgetTruncatesAndPublishesLastWrittenOffset( // Self-healing: a reconnect from the published cursor re-requests the // bytes the budget deferred — the last 4 chunks — and catches up to - // head, so nothing was lost, only deferred. - c2, replay2, sync2 := dialHandshake(t, addr, ttyWSPath(instID, "since="+strconv.FormatInt(syncOffset, 10))) + // head, so nothing was lost, only deferred. Its start sync must open + // exactly AT the published cursor: resuming, not restarting. + c2, frames2, start2, sync2 := dialHandshakeStart(t, addr, ttyWSPath(instID, "since="+strconv.FormatInt(syncOffset, 10)+"&caps=sync")) _ = c2.WriteClose(ws.CloseMessage(1000, "bye")) _ = c2.Close() + if start2 != syncOffset { + t.Fatalf("start sync of the re-request = %d, want the published cursor %d — the replay must resume where the truncated one stopped", start2, syncOffset) + } + var replay2 []byte + for _, f := range frames2 { + replay2 = append(replay2, f...) + } want2 := delta[ttyHandshakeReplayBudget:] - if replay2 != string(want2) { + if !bytes.Equal(replay2, want2) { t.Fatalf("re-request after truncation = %d bytes, want the %d deferred bytes — the budget must defer, never skip", len(replay2), len(want2)) } if sync2 != head { @@ -771,6 +818,188 @@ func TestHandleInstanceTTYWS_ReplayBudgetTruncatesAndPublishesLastWrittenOffset( } } +// TestHandleInstanceTTYWS_ReplayStartOffsetPublishesClampedStart pins AC2 of +// issue #94: when the requested `since` predates the oldest live byte, +// ReadSince silently clamps to that byte — and the start sync must publish +// the CLAMPED value, never the raw request. Publishing the request would +// land the client's cursor PAST bytes it never received once it advances +// per frame — strictly worse than the pre-#94 re-pull this frame removes. +func TestHandleInstanceTTYWS_ReplayStartOffsetPublishesClampedStart(t *testing.T) { + t.Parallel() + // A 64-byte ring with 100 bytes written holds offsets [36, 100): a + // request from since=5 is clamped to 36 by the first ReadSince. + payload := make([]byte, 100) + for i := range payload { + payload[i] = byte('A' + i%26) + } + k := newTTYHandshakeKind(64) + addr, _, instID, _ := ttyWSTestServer(t, k) + k.buf.Write(payload) + + c, frames, startOffset, syncOffset := dialHandshakeStart(t, addr, ttyWSPath(instID, "since=5&caps=sync")) + _ = c.WriteClose(ws.CloseMessage(1000, "bye")) + _ = c.Close() + + if startOffset != 36 { + t.Fatalf("start sync offset = %d, want 36 (the clamped oldest live byte) — echoing the raw since=5 would strand the cursor past bytes the client never received", startOffset) + } + var replay []byte + for _, f := range frames { + replay = append(replay, f...) + } + if !bytes.Equal(replay, payload[36:]) { + t.Fatalf("replay = %d bytes starting at %v, want the live window payload[36:] (%d bytes)", len(replay), replay[:1], len(payload[36:])) + } + if syncOffset != 100 { + t.Fatalf("sync offset = %d, want 100 (the ring head)", syncOffset) + } + if startOffset+int64(len(replay)) != syncOffset { + t.Fatalf("start %d + replay %d != sync %d — the frame-by-frame advance from the clamped start must close exactly on the end offset", startOffset, len(replay), syncOffset) + } +} + +// TestHandleInstanceTTYWS_MidReplayResumeContinuesFromRenderedBytes pins AC1 +// of issue #94: a connection that dies MID-REPLAY — after the start sync and +// two chunks, before the closing sync — has already advanced its cursor to +// start+painted on the client, so the reconnect resumes with exactly the +// un-rendered remainder instead of re-pulling the whole replay from the old +// cursor (the pre-#94 death window, up to 8MB under a degrading link). +func TestHandleInstanceTTYWS_MidReplayResumeContinuesFromRenderedBytes(t *testing.T) { + t.Parallel() + // A 200KB delta in a ring that holds it comfortably: a multi-chunk + // streaming replay (64KB reads), long enough to die inside. + delta := make([]byte, 200*1024) + for i := range delta { + delta[i] = byte('a' + i%26) + } + k := newTTYHandshakeKind(int64(len(delta)) + 1024) + addr, _, instID, _ := ttyWSTestServer(t, k) + k.buf.Write(delta) + + // First connection: "dies" two chunks into the replay. Read frames + // manually — dialHandshakeStart would consume the closing sync this + // connection never lives to see. The client-side cursor at the drop is + // the start sync's S plus the wire bytes already painted. + c := dialTTY(t, addr, ttyWSPath(instID, "since=0&caps=sync")) + sendResize(t, c) + startOffset := int64(-1) + var painted int64 + for painted < 2*64*1024 { + op, p := readFrame(t, c, 5*time.Second) + switch op { + case wsOpBinary: + if startOffset == -1 { + t.Fatalf("binary replay chunk before the start sync — the client would advance its cursor from a base it never learned") + } + if !bytes.Equal(p, delta[painted:painted+int64(len(p))]) { + t.Fatalf("replay chunk at offset %d is not the delta's bytes there", painted) + } + painted += int64(len(p)) + case wsOpPing: + // Protocol heartbeat: not part of the replay. + case wsOpText: + var ctl struct { + Type string `json:"type"` + Offset int64 `json:"offset"` + Start bool `json:"start"` + } + if err := json.Unmarshal(p, &ctl); err != nil { + t.Fatalf("control frame %q is not JSON: %v", p, err) + } + switch ctl.Type { + case "sync": + if !ctl.Start { + t.Fatalf("closing sync arrived %d bytes into a 200KB replay — it must follow the LAST chunk", painted) + } + startOffset = ctl.Offset + case "resize", "ping", "pong": + // Size echo / app-level heartbeat: background traffic. + default: + t.Fatalf("unexpected control frame %q during the replay", p) + } + default: + t.Fatalf("unexpected opcode %d during the replay", op) + } + } + if startOffset != 0 { + t.Fatalf("start sync offset = %d, want 0 (the requested cursor, no clamp on a fresh ring)", startOffset) + } + // The drop: no closing sync was consumed, the cursor stands at the + // rendered byte count. _ = c.Close() via cleanup would also do, but the + // explicit close IS the mid-replay death this test is about. + _ = c.Close() + + // The reconnect sends the rendered cursor as `since`; the resume must + // deliver EXACTLY the un-rendered remainder — neither the two painted + // chunks again (the pre-#94 duplication) nor anything past head. + cursor := startOffset + painted + c2, frames2, start2, sync2 := dialHandshakeStart(t, addr, ttyWSPath(instID, "since="+strconv.FormatInt(cursor, 10)+"&caps=sync")) + _ = c2.WriteClose(ws.CloseMessage(1000, "bye")) + _ = c2.Close() + if start2 != cursor { + t.Fatalf("resume start sync = %d, want the rendered cursor %d — resuming must continue where the dropped replay died", start2, cursor) + } + var replay2 []byte + for _, f := range frames2 { + replay2 = append(replay2, f...) + } + if !bytes.Equal(replay2, delta[cursor:]) { + t.Fatalf("resume replay = %d bytes, want exactly the %d un-rendered bytes — no re-delivery of what was already painted", len(replay2), len(delta[cursor:])) + } + if sync2 != int64(len(delta)) { + t.Fatalf("resume sync offset = %d, want head %d", sync2, len(delta)) + } +} + +// TestHandleInstanceTTYWS_StartSyncOnlyForChunkedReplaysWithACursor pins the +// boundaries of the issue #94 frame. The contract is: EVERY replay CHUNK +// addressed to a cursor-bearing client is preceded by a start frame — the +// streamed catch-up (pinned by the resume/budget tests above) AND the +// single-frame beyond-head degrade (pinned by +// TestHandleInstanceTTYWS_CursorAheadOfHeadFallsBackToTail). What this test +// pins is the other side, the three paths that must send the closing sync +// ALONE: the CURSOR-LESS tail replay (no cursor to advance — and such a +// client may not parse `sync` at all, so that path's syncCap is a genuine +// gate), the sinceFollowLiveEnd path and the caught-up-at-head consult +// exit (both cursor-bearing, but zero chunks to announce). +func TestHandleInstanceTTYWS_StartSyncOnlyForChunkedReplaysWithACursor(t *testing.T) { + t.Parallel() + k := newTTYHandshakeKind(1024) + addr, _, instID, _ := ttyWSTestServer(t, k) + k.buf.WriteString("tail-body") // head = 9 + + // Tail path: no `since` at all — the one CURSOR-LESS replay. + c1, _, start1, sync1 := dialHandshakeStart(t, addr, ttyWSPath(instID, "caps=sync")) + _ = c1.Close() + if start1 != -1 { + t.Fatalf("tail replay announced a start offset %d — the client holds no cursor to advance (and may not parse sync at all), so the single ≤64KB frame keeps the pre-#87 death window", start1) + } + if sync1 != 9 { + t.Fatalf("tail replay sync = %d, want head 9", sync1) + } + + // Follow from the live end (-2): no replay at all, the closing sync IS + // the cursor handoff. + c2, _, start2, sync2 := dialHandshakeStart(t, addr, ttyWSPath(instID, "since=-2&caps=sync")) + _ = c2.Close() + if start2 != -1 { + t.Fatalf("follow-live-end announced a start offset %d — the -2 path emits zero binary frames and must stay untouched (issue #94 AC)", start2) + } + if sync2 != 9 { + t.Fatalf("follow-live-end sync = %d, want head 9", sync2) + } + + // Caught up exactly at head: the empty-replay consult exit. + c3, frames3, start3, sync3 := dialHandshakeStart(t, addr, ttyWSPath(instID, "since=9&caps=sync")) + _ = c3.Close() + if start3 != -1 { + t.Fatalf("caught-up reconnect announced a start offset %d — an empty replay has no first chunk to start at", start3) + } + if len(frames3) != 0 || sync3 != 9 { + t.Fatalf("caught-up reconnect frames=%d sync=%d, want an empty replay and head 9", len(frames3), sync3) + } +} + // TestHandleInstanceTTYWS_EmptyReadAfterConsultPublishesHeadNotZero pins // review finding B3, the FIX-F counterpart. The scripted queue drives the // same shape TestHandleInstanceTTYWS_ConsultSeesNewBytesDeliversThem uses @@ -797,7 +1026,7 @@ func TestHandleInstanceTTYWS_EmptyReadAfterConsultPublishesHeadNotZero(t *testin scriptedRead{body: "", next: 16}, ) - c, replay, syncOffset := dialHandshake(t, addr, ttyWSPath(instID, "since=10")) + c, replay, syncOffset := dialHandshake(t, addr, ttyWSPath(instID, "since=10&caps=sync")) _ = c.WriteClose(ws.CloseMessage(1000, "bye")) _ = c.Close() @@ -821,7 +1050,7 @@ func TestHandleInstanceTTYWS_SyncFrameCarriesEndOffset(t *testing.T) { addr, _, instID, _ := ttyWSTestServer(t, k) // Empty ring buffer: no binary frame, but still a sync with offset 0. - c, replay, syncOffset := dialHandshake(t, addr, ttyWSPath(instID, "")) + c, replay, syncOffset := dialHandshake(t, addr, ttyWSPath(instID, "caps=sync")) if replay != "" { t.Fatalf("replay on empty buffer = %q, want no bytes", replay) } @@ -862,7 +1091,7 @@ func TestHandleInstanceTTYWS_SyncFrameCarriesEndOffset(t *testing.T) { _ = c.WriteClose(ws.CloseMessage(1000, "bye")) _ = c.Close() replay2, sync2 := func() (string, int64) { - c2, replay, off := dialHandshake(t, addr, ttyWSPath(instID, "since=10")) + c2, replay, off := dialHandshake(t, addr, ttyWSPath(instID, "since=10&caps=sync")) _ = c2.WriteClose(ws.CloseMessage(1000, "bye")) _ = c2.Close() return replay, off @@ -875,6 +1104,105 @@ func TestHandleInstanceTTYWS_SyncFrameCarriesEndOffset(t *testing.T) { } } +// TestHandleInstanceTTYWS_SyncFrameRequiresOptIn pins the caps=sync half of +// the issue #98 contract: the {"type":"sync"} offset echo is sent ONLY to +// clients whose handshake `caps` list carries "sync" as a whole, +// case-sensitive token — or that present an explicit `since` cursor, the +// INFERRED opt-in: the cursor parameter and the sync whitelist shipped in +// the same fix (#87, v0.5.1), and v0.5.0 never puts `since` on this +// endpoint at all, so a client presenting one provably parses the other. +// Without that inference a pre-caps v0.5.1 page's cursor latch never sets +// and every reconnect re-replays from the frozen cursor (review round 2). +// The frame post-dates the v0.5.0 parseTTYControlMessage whitelist, so an +// un-refreshed v0.5.0 page painted it into the terminal once per connect — +// the same leak class as the every-10s TEXT ping. Caps gate only the +// visible control frame: the binary replay flows regardless. +func TestHandleInstanceTTYWS_SyncFrameRequiresOptIn(t *testing.T) { + t.Parallel() + + // Negative cases: the replay still arrives, but no sync frame may ever + // appear — for a client with no caps at all, for one that declared + // only "ping" (one capability does not imply the other), and for a + // case-mismatched token ("SYNC" names no capability). The fast + // heartbeat tick keeps frames flowing through the window so the reads + // never stall. + for _, query := range []string{"", "caps=ping", "caps=SYNC"} { + k := newTTYHandshakeKind(1024) + addr, _, _, instID, _ := ttyWSTestServerWithLiveness(t, k, 100*time.Millisecond, 30*time.Second) + k.buf.WriteString("tail-body") // 9 bytes + c := dialTTY(t, addr, ttyWSPath(instID, query)) + sendResize(t, c) + + sawReplay := false + deadline := time.Now().Add(650 * time.Millisecond) + for time.Now().Before(deadline) { + op, p := readFrame(t, c, 2*time.Second) + switch op { + case wsOpBinary: + if string(p) == "tail-body" { + sawReplay = true + } + case wsOpPing: + // Protocol heartbeat: control frame, invisible to page JS. + case wsOpText: + var ctl struct { + Type string `json:"type"` + } + if err := json.Unmarshal(p, &ctl); err != nil { + t.Fatalf("query %q: control frame %q is not JSON: %v", query, p, err) + } + if ctl.Type == "sync" { + t.Fatalf("query %q: {\"type\":\"sync\"} reached a client that did not opt in — a pre-whitelist page would paint it into the terminal on every connect (issue #98)", query) + } + // Anything else (the resize echo, the opted-in text ping + // for the caps=ping case) is legitimate traffic here. + default: + t.Fatalf("query %q: unexpected opcode %d", query, op) + } + } + if !sawReplay { + t.Fatalf("query %q: no replay arrived — caps must gate only the sync control frame, never the binary output", query) + } + _ = c.WriteClose(ws.CloseMessage(1000, "bye")) + _ = c.Close() + } + + // Positive cases: the exact caps token opts in — including from a + // REPEATED caps parameter (url.Values.Get would read only the first + // value) — and so does an explicit `since`, the inferred #87-era + // opt-in: incremental (`since=5`), follow-live-end (`since=-2`, + // where the echo IS the only cursor handoff), and even an + // unparsable value (presence, not parseability, is the proof — + // v0.5.0 never sends the parameter at all). + for _, tc := range []struct { + query string + wantReplay string + wantOffset int64 + }{ + {"caps=sync", "tail-body", 9}, + {"caps=pong&caps=sync", "tail-body", 9}, + {"since=5", "body", 9}, + {"since=-2", "", 9}, + {"since=bogus", "tail-body", 9}, + } { + k := newTTYHandshakeKind(1024) + addr, _, instID, _ := ttyWSTestServer(t, k) + k.buf.WriteString("tail-body") + replay, syncOffset := func() (string, int64) { + c, replay, off := dialHandshake(t, addr, ttyWSPath(instID, tc.query)) + _ = c.WriteClose(ws.CloseMessage(1000, "bye")) + _ = c.Close() + return replay, off + }() + if replay != tc.wantReplay { + t.Fatalf("query %q: replay = %q, want %q", tc.query, replay, tc.wantReplay) + } + if syncOffset != tc.wantOffset { + t.Fatalf("query %q: sync offset = %d, want %d", tc.query, syncOffset, tc.wantOffset) + } + } +} + // TestHandleInstanceTTYWS_ReplayReadFailureClosesConnection pins the // handshake error path: a failing replay read must CLOSE the connection // with 1013, never smuggle the error text into a binary frame — every @@ -887,9 +1215,12 @@ func TestHandleInstanceTTYWS_ReplayReadFailureClosesConnection(t *testing.T) { k.buf.WriteString("some-bytes") k.failRead.Store(true) - // Both branches fail the same way: the tail branch (no since) and - // the catch-up loop (since=2) hit the error on their first read. - for _, query := range []string{"", "since=2"} { + // Both branches fail the same way: the tail branch (no since) and the + // catch-up loop (since=2) hit the error on their first read. caps=sync + // rides along so the "never a sync" assertion below keeps its teeth — + // without the opt-in the server withholds the frame regardless of the + // error path. + for _, query := range []string{"caps=sync", "since=2&caps=sync"} { c := dialTTY(t, addr, ttyWSPath(instID, query)) sendResize(t, c) // The resize that triggers the handshake also queues a shared-size @@ -949,7 +1280,7 @@ func TestHandleInstanceTTYWS_NonRunningInstanceDegradesToFirstConnect(t *testing t.Fatalf("Stop: %v", err) } - c, replay, syncOffset := dialHandshake(t, addr, ttyWSPath(instID, "since=5")) + c, replay, syncOffset := dialHandshake(t, addr, ttyWSPath(instID, "since=5&caps=sync")) if replay != "" { t.Fatalf("replay on stopped instance = %q, want nothing", replay) } @@ -976,11 +1307,19 @@ func TestHandleInstanceTTYWS_CursorAheadOfHeadFallsBackToTail(t *testing.T) { addr, _, instID, _ := ttyWSTestServer(t, k) k.buf.WriteString("HELLO-RING") // head = 10 - c, replay, syncOffset := dialHandshake(t, addr, ttyWSPath(instID, "since=999")) + c, frames, startOffset, syncOffset := dialHandshakeStart(t, addr, ttyWSPath(instID, "since=999&caps=sync")) _ = c.WriteClose(ws.CloseMessage(1000, "bye")) _ = c.Close() - if replay != "HELLO-RING" { - t.Fatalf("replay = %q, want the tail HELLO-RING (cursor ahead of head must fall back to first-connect tail)", replay) + if len(frames) != 1 || string(frames[0]) != "HELLO-RING" { + t.Fatalf("replay = %d frames %q, want the single tail HELLO-RING (cursor ahead of head must fall back to first-connect tail)", len(frames), frames) + } + // The degrade announced the tail's real start like every replay + // addressed to a cursor-bearing client (issue #94 review): S = head 10 + // − len 10 = 0. A genuine 0, distinct from the harness's -1 "no start + // frame" sentinel — and exactly the value the client's latch needs to + // count the tail frame up to the closing sync. + if startOffset != 0 { + t.Fatalf("start sync offset = %d, want 0 (head 10 − tail 10) — the degrade announces the tail's real start like any other replay", startOffset) } if syncOffset != 10 { t.Fatalf("sync offset = %d, want 10 (the tail's end offset, not the client's bogus 999)", syncOffset) @@ -1024,7 +1363,7 @@ func TestHandleInstanceTTYWS_ConsultSeesNewBytesDeliversThem(t *testing.T) { scriptedRead{body: "ABCDEF", next: 16}, ) - c, replay, syncOffset := dialHandshake(t, addr, ttyWSPath(instID, "since=10")) + c, replay, syncOffset := dialHandshake(t, addr, ttyWSPath(instID, "since=10&caps=sync")) _ = c.WriteClose(ws.CloseMessage(1000, "bye")) _ = c.Close() if replay != "ABCDEF" { @@ -1147,7 +1486,7 @@ func TestHandleInstanceTTYWS_SubscriberOverflowClosesConnectionWith1013(t *testi k := newTTYHandshakeKind(1024) addr, _, instID, logOut := ttyWSTestServer(t, k) - c, _, _ := dialHandshake(t, addr, ttyWSPath(instID, "")) + c, _, _ := dialHandshake(t, addr, ttyWSPath(instID, "caps=sync")) k.waitSubscribers(t, 5*time.Second) // Outrun the queue: the client is not reading, so every chunk the @@ -1185,13 +1524,16 @@ func TestHandleInstanceTTYWS_SubscriberOverflowClosesConnectionWith1013(t *testi // rather than treat the session as finished; // - its reason names the overflow, so the console explains the drop // instead of showing an unexplained disconnect; - // - no second `sync` precedes it. `sync` is written exactly once, in - // completeHandshake, before SubscribeOutput (app.go), so a second one - // here would advertise a cursor past bytes the client never received - // and its next reconnect would silently skip them. The resize echo the - // handler's own handshake queued is a TEXT frame on a different - // channel, so it may land anywhere in this sequence; it is not a - // cursor and is allowed through. + // - no `sync` precedes it. The handshake's sync frames — since issue + // #94 up to TWO of them, the start announcement ahead of the first + // replay chunk and the closing echo after the last — are ALL written + // inside completeHandshake, before SubscribeOutput (app.go), so one + // arriving HERE, on the live stream, would advertise a cursor past + // bytes the client never received and its next reconnect would + // silently skip them. The resize echo the handler's own handshake + // queued is a TEXT frame on a different channel, so it may land + // anywhere in this sequence; it is not a cursor and is allowed + // through. for { op, p := readFrame(t, c, 10*time.Second) if op == wsOpClose { @@ -1283,7 +1625,11 @@ func TestHandleInstanceTTYWS_HeartbeatEmitsPingControlFrames(t *testing.T) { k := newTTYHandshakeKind(1024) addr, _, _, instID, _ := ttyWSTestServerWithLiveness(t, k, 100*time.Millisecond, 30*time.Second) - c, _, _ := dialHandshake(t, addr, ttyWSPath(instID, "")) + // caps=ping (issue #98): only a client that opts in via the handshake + // capability list is sent the TEXT {"type":"ping"} heartbeat — the + // RFC 6455 ping goes to everyone, the text frame is opt-in because a + // client without the parseTTYControlMessage whitelist would paint it. + c, _, _ := dialHandshake(t, addr, ttyWSPath(instID, "caps=ping,sync")) sawRFCPing := false deadline := time.Now().Add(3 * time.Second) // 30 × the test tick @@ -1317,6 +1663,84 @@ func TestHandleInstanceTTYWS_HeartbeatEmitsPingControlFrames(t *testing.T) { t.Fatal(`no {"type":"ping"} heartbeat frame within the deadline`) } +// TestHandleInstanceTTYWS_HeartbeatTextPingRequiresOptIn pins the issue #98 +// contract: the TEXT {"type":"ping"} heartbeat is sent ONLY to clients whose +// handshake `caps` list carries "ping" as a whole token. A page loaded before +// the parseTTYControlMessage whitelist shipped (v0.5.0) connected to a +// v0.5.1+ daemon painted the text frame into the terminal every 10s — the +// server must never emit a visible control frame its client did not declare. +// The RFC 6455 ping is unaffected: onmessage never fires for control frames, +// so it is always safe and keeps flowing to every client. +func TestHandleInstanceTTYWS_HeartbeatTextPingRequiresOptIn(t *testing.T) { + t.Parallel() + + // expectNoTextPing runs one connection for ~6 ping ticks: RFC 6455 + // pings MUST keep arriving (invisible to page JS, always safe), but no + // TEXT {"type":"ping"} may ever appear. The window is deliberately + // generous — 650ms against a 100ms tick with a >=3 threshold leaves + // ~350ms of scheduling slack, so a loaded CI runner cannot flake it + // (a 450ms window left only ~150ms over the third tick). dialTTY + + // sendResize rather than dialHandshake: these queries carry no "sync" + // capability, so no sync frame ever terminates a dialHandshake wait — + // the resize completes the handshake server-side on its own. + expectNoTextPing := func(t *testing.T, query string) { + t.Helper() + k := newTTYHandshakeKind(1024) + addr, _, _, instID, _ := ttyWSTestServerWithLiveness(t, k, 100*time.Millisecond, 30*time.Second) + c := dialTTY(t, addr, ttyWSPath(instID, query)) + defer func() { _ = c.Close() }() + sendResize(t, c) + + rfcPings := 0 + deadline := time.Now().Add(650 * time.Millisecond) // > 6 ticks + for time.Now().Before(deadline) { + op, p := readFrame(t, c, 2*time.Second) + switch op { + case wsOpPing: + rfcPings++ + case wsOpText: + if strings.Contains(string(p), `"type":"ping"`) { + t.Fatalf("query %q: TEXT {\"type\":\"ping\"} reached a client that did not opt in — it would be painted into the terminal every tick (issue #98)", query) + } + // Anything else (the post-handshake resize echo) is + // legitimate background traffic here. + } + } + if rfcPings < 3 { + t.Fatalf("query %q: only %d RFC 6455 pings in the window — the always-on heartbeat must flow regardless of caps", query, rfcPings) + } + } + + expectNoTextPing(t, "") // no caps parameter at all + expectNoTextPing(t, "caps=pong") // a different capability is not ping + expectNoTextPing(t, "caps=pinger") // a substring is not a token match + expectNoTextPing(t, "caps=PING") // the match is case-sensitive + + // expectTextPing: the exact token anywhere in the comma-separated list + // opts in. (caps=sync rides along so dialHandshake's sync wait + // terminates; it does not bear on the ping gate.) + expectTextPing := func(t *testing.T, query string) { + t.Helper() + k := newTTYHandshakeKind(1024) + addr, _, _, instID, _ := ttyWSTestServerWithLiveness(t, k, 100*time.Millisecond, 30*time.Second) + c, _, _ := dialHandshake(t, addr, ttyWSPath(instID, query)) + defer func() { _ = c.Close() }() + deadline := time.Now().Add(2 * time.Second) + for time.Now().Before(deadline) { + op, p := readFrame(t, c, 2*time.Second) + if op == wsOpText && strings.Contains(string(p), `"type":"ping"`) { + return // the opted-in client received the text heartbeat + } + } + t.Fatalf(`query %q: no {"type":"ping"} within the deadline — the whole-token capability must opt in`, query) + } + + expectTextPing(t, "caps=resize,ping,sync") + // A REPEATED caps parameter is scanned too — url.Values.Get would + // return only the first value and miss the ping in the second. + expectTextPing(t, "caps=pong&caps=ping,sync") +} + // TestHandleInstanceTTYWS_PingProbeAnsweredAndNotTypedIntoShell pins the // client-probe round trip (issue #83): a client-sent {"type":"ping"} must // be answered with {"type":"pong"} AND must never reach SendInput — the @@ -1336,7 +1760,7 @@ func TestHandleInstanceTTYWS_PingProbeAnsweredAndNotTypedIntoShell(t *testing.T) // resize echo and the probe's own answer — zero interleaving noise. addr, _, _, instID, _ := ttyWSTestServerWithLiveness(t, k, time.Hour, time.Hour) - c, _, _ := dialHandshake(t, addr, ttyWSPath(instID, "")) + c, _, _ := dialHandshake(t, addr, ttyWSPath(instID, "caps=sync")) readAnswer := func(what string) { t.Helper() @@ -1398,7 +1822,7 @@ func TestHandleInstanceTTYWS_DeadPeerReapedByReadDeadline(t *testing.T) { // loaded or -race'd runner breaks that assumption and flakes. Deriving // the floor from the configured deadline keeps the bound honest. start := time.Now() - c, _, _ := dialHandshake(t, addr, ttyWSPath(instID, "")) + c, _, _ := dialHandshake(t, addr, ttyWSPath(instID, "caps=sync")) // Bound the wait so a never-reaping server fails the test instead of // hanging it: the reap must land ~readDeadline after the dial, long @@ -1448,7 +1872,9 @@ func TestHandleInstanceTTYWS_ResponsivePeerSurvivesReadDeadline(t *testing.T) { const readDeadline = 300 * time.Millisecond addr, srv, _, instID, _ := ttyWSTestServerWithLiveness(t, k, pingInterval, readDeadline) - c, _, _ := dialHandshake(t, addr, ttyWSPath(instID, "")) + // caps=ping opts into the TEXT heartbeat (issue #98), so both heartbeat + // frames are expected below. + c, _, _ := dialHandshake(t, addr, ttyWSPath(instID, "caps=ping,sync")) windowStart := time.Now() surviveUntil := windowStart.Add(3 * readDeadline) @@ -1645,7 +2071,7 @@ func TestHandleInstanceTTYWS_FollowLiveEndReplaysNothing(t *testing.T) { addr, _, instID, _ := ttyWSTestServer(t, k) k.buf.WriteString("HELLO-TAIL") // head = 10 - c, replay, syncOffset := dialHandshake(t, addr, ttyWSPath(instID, "since=-2")) + c, replay, syncOffset := dialHandshake(t, addr, ttyWSPath(instID, "since=-2&caps=sync")) if replay != "" { t.Fatalf("sentinel handshake replayed %q, want no binary frame at all — the client's screen already holds the tail", replay) } @@ -1686,7 +2112,7 @@ func TestHandleInstanceTTYWS_FollowLiveEndOnEmptyRingPublishesZero(t *testing.T) k := newTTYHandshakeKind(1024) addr, _, instID, _ := ttyWSTestServer(t, k) - c, replay, syncOffset := dialHandshake(t, addr, ttyWSPath(instID, "since=-2")) + c, replay, syncOffset := dialHandshake(t, addr, ttyWSPath(instID, "since=-2&caps=sync")) if replay != "" { t.Fatalf("replay on an empty ring = %q, want nothing", replay) } @@ -1726,7 +2152,7 @@ func TestHandleInstanceTTYWS_FollowLiveEndOnStoppedInstanceCloses(t *testing.T) t.Fatalf("Stop: %v", err) } - c := dialTTY(t, addr, ttyWSPath(instID, "since=-2")) + c := dialTTY(t, addr, ttyWSPath(instID, "since=-2&caps=sync")) sendResize(t, c) for { op, p := readFrame(t, c, 5*time.Second) @@ -1769,7 +2195,7 @@ func TestHandleInstanceTTYWS_FollowLiveEndReadFailureClosesConnection(t *testing k.buf.WriteString("some-bytes") k.failRead.Store(true) - c := dialTTY(t, addr, ttyWSPath(instID, "since=-2")) + c := dialTTY(t, addr, ttyWSPath(instID, "since=-2&caps=sync")) sendResize(t, c) for { op, p := readFrame(t, c, 5*time.Second) diff --git a/internal/cli/cli.go b/internal/cli/cli.go index 542c7a0..073a7bc 100644 --- a/internal/cli/cli.go +++ b/internal/cli/cli.go @@ -10,6 +10,7 @@ import ( "os" "os/signal" "path/filepath" + "strconv" "strings" "syscall" @@ -209,10 +210,53 @@ func worktreeCmd(logger *log.Logger, args []string) error { return nil case "delete": - if len(args) < 2 { - return fmt.Errorf("usage: myworktree worktree delete ") + // Scan for --force manually: Go's flag package stops parsing at + // the first positional arg, so `worktree delete --force` + // would silently drop the flag and the user would hit the same + // refusal the message just told them to retry with force + // (issue #102 review). The --force= spelling and the -- + // terminator keep this consistent with the flag-based + // subcommands (second review round). + var force bool + var positional []string + literal := false + for _, a := range args[1:] { + if literal { + positional = append(positional, a) + continue + } + switch { + case a == "--": + literal = true + case a == "--force" || a == "-force": + force = true + case strings.HasPrefix(a, "--force=") || strings.HasPrefix(a, "-force="): + v := a[strings.Index(a, "=")+1:] + b, err := strconv.ParseBool(v) + if err != nil { + return fmt.Errorf("invalid --force value %q (want true/false); usage: myworktree worktree delete [--force] ", v) + } + force = b + default: + positional = append(positional, a) + } + } + if len(positional) != 1 { + return fmt.Errorf("usage: myworktree worktree delete [--force] ") } - return mgr.Delete(args[1]) + ignoredDestroyed, err := mgr.Delete(positional[0], force) + if err != nil { + return err + } + if ignoredDestroyed > 0 { + word := "entries" + if ignoredDestroyed == 1 { + word = "entry" + } + fmt.Fprintf(os.Stderr, "note: the delete destroyed %d gitignored %s with the directory (build caches, logs, backups that existed only in this worktree)\n", + ignoredDestroyed, word) + } + return nil default: return fmt.Errorf("unknown worktree subcommand: %s", args[0]) diff --git a/internal/cli/cli_test.go b/internal/cli/cli_test.go index e568fac..fe6cea6 100644 --- a/internal/cli/cli_test.go +++ b/internal/cli/cli_test.go @@ -6,10 +6,12 @@ import ( "io" "log" "os" + "os/exec" "path/filepath" "testing" "myworktree/internal/config" + "myworktree/internal/store" "myworktree/internal/version" ) @@ -301,3 +303,146 @@ func captureStdin(t *testing.T, input string) (restore func()) { r.Close() } } + +// setupWorktreeDeleteCLI builds a real git repo with one registered managed +// worktree and points the CLI at it (cwd + XDG_CONFIG_HOME), returning the +// worktree path. The delete subcommand shells out to git, so only a real +// repo exercises it honestly. +func setupWorktreeDeleteCLI(t *testing.T) string { + t.Helper() + if _, err := exec.LookPath("git"); err != nil { + t.Skip("git is required") + } + repo := t.TempDir() + // macOS: t.TempDir() lives under /var, a symlink to /private/var, while + // the CLI resolves the repo through `git rev-parse --show-toplevel` + // (which returns the REAL path) and keys the state dir off it — so the + // test must write state.json under the resolved path too, or the CLI + // looks up a different HashPath and never finds the worktree. + if resolved, err := filepath.EvalSymlinks(repo); err == nil { + repo = resolved + } + runGitCLI := func(args ...string) { + t.Helper() + cmd := exec.Command("git", args...) + cmd.Dir = repo + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("git %v failed: %v (%s)", args, err, out) + } + } + runGitCLI("init") + runGitCLI("config", "user.name", "Test User") + runGitCLI("config", "user.email", "test@example.com") + if err := os.WriteFile(filepath.Join(repo, "README.md"), []byte("init\n"), 0o600); err != nil { + t.Fatalf("write seed: %v", err) + } + runGitCLI("add", "README.md") + runGitCLI("commit", "-m", "init") + wtDir := t.TempDir() + if resolved, err := filepath.EvalSymlinks(wtDir); err == nil { + wtDir = resolved + } + wtPath := filepath.Join(wtDir, "wt") + runGitCLI("worktree", "add", wtPath, "-b", "wt-branch") + + t.Setenv("XDG_CONFIG_HOME", t.TempDir()) + dataDir, err := projectDataDir(repo) + if err != nil { + t.Fatalf("projectDataDir: %v", err) + } + if err := os.MkdirAll(dataDir, 0o755); err != nil { + t.Fatalf("mkdir dataDir: %v", err) + } + fs := store.FileStore{Path: filepath.Join(dataDir, "state.json")} + if err := fs.Save(store.State{ + Worktrees: []store.ManagedWorktree{{ID: "wt1", Name: "wt1", Path: wtPath, Branch: "wt-branch"}}, + }); err != nil { + t.Fatalf("save state: %v", err) + } + + oldWd, err := os.Getwd() + if err != nil { + t.Fatalf("getwd: %v", err) + } + if err := os.Chdir(repo); err != nil { + t.Fatalf("chdir: %v", err) + } + t.Cleanup(func() { _ = os.Chdir(oldWd) }) + return wtPath +} + +// TestRunWorktreeDeleteTrailingForceFlag pins the issue #102 review fix: +// `myworktree worktree delete --force` must actually force — Go's flag +// package stops parsing at the first positional arg, so the previous +// FlagSet-based parsing silently dropped a trailing --force and the user hit +// the very refusal the message told them to retry past. +func TestRunWorktreeDeleteTrailingForceFlag(t *testing.T) { + wtPath := setupWorktreeDeleteCLI(t) + // Dirty the worktree so only a forced delete can remove it. + if err := os.WriteFile(filepath.Join(wtPath, "scratch.txt"), []byte("x\n"), 0o600); err != nil { + t.Fatalf("write scratch: %v", err) + } + + if code := Run([]string{"myworktree", "worktree", "delete", "wt1", "--force"}, log.New(io.Discard, "", 0)); code != 0 { + t.Fatalf("trailing --force delete should succeed, exit code %d", code) + } + if _, err := os.Stat(wtPath); !os.IsNotExist(err) { + t.Fatalf("trailing --force should delete the worktree, stat err=%v", err) + } +} + +// TestRunWorktreeDeleteExtraPositionalFails pins the usage guard: anything +// beyond the single id is a usage error, not a silently ignored argument. +func TestRunWorktreeDeleteExtraPositionalFails(t *testing.T) { + setupWorktreeDeleteCLI(t) + if code := Run([]string{"myworktree", "worktree", "delete", "wt1", "wt2"}, log.New(io.Discard, "", 0)); code == 0 { + t.Fatal("extra positional args must fail with a usage error") + } +} + +// TestRunWorktreeDeleteForceEqualsForms pins the second-review-round parsing +// consistency: the hand-rolled scan must honor the --force= spelling +// the flag-based subcommands accept, in any position. +func TestRunWorktreeDeleteForceEqualsForms(t *testing.T) { + wtPath := setupWorktreeDeleteCLI(t) + if err := os.WriteFile(filepath.Join(wtPath, "scratch.txt"), []byte("x\n"), 0o600); err != nil { + t.Fatalf("write scratch: %v", err) + } + + // --force=false must NOT force: the dirty delete is refused. + if code := Run([]string{"myworktree", "worktree", "delete", "wt1", "--force=false"}, log.New(io.Discard, "", 0)); code == 0 { + t.Fatal("--force=false should not force the delete of a dirty worktree") + } + if _, err := os.Stat(wtPath); err != nil { + t.Fatalf("refused delete must keep the path: %v", err) + } + + // --force=true must force, trailing the id. + if code := Run([]string{"myworktree", "worktree", "delete", "wt1", "--force=true"}, log.New(io.Discard, "", 0)); code != 0 { + t.Fatalf("--force=true delete should succeed, exit code %d", code) + } + if _, err := os.Stat(wtPath); !os.IsNotExist(err) { + t.Fatalf("--force=true should delete the worktree, stat err=%v", err) + } +} + +// TestRunWorktreeDeleteInvalidForceValueFails pins that a garbage +// --force= is a loud error, never a silent default. +func TestRunWorktreeDeleteInvalidForceValueFails(t *testing.T) { + setupWorktreeDeleteCLI(t) + if code := Run([]string{"myworktree", "worktree", "delete", "wt1", "--force=maybe"}, log.New(io.Discard, "", 0)); code == 0 { + t.Fatal("--force=maybe must fail loudly") + } +} + +// TestRunWorktreeDeleteDashDashTerminator pins that `--` ends flag +// handling: everything after it is positional. +func TestRunWorktreeDeleteDashDashTerminator(t *testing.T) { + wtPath := setupWorktreeDeleteCLI(t) + if code := Run([]string{"myworktree", "worktree", "delete", "--", "wt1"}, log.New(io.Discard, "", 0)); code != 0 { + t.Fatalf("delete -- wt1 should succeed on a clean worktree, exit code %d", code) + } + if _, err := os.Stat(wtPath); !os.IsNotExist(err) { + t.Fatalf("delete -- wt1 should remove the worktree, stat err=%v", err) + } +} diff --git a/internal/ui/static/index.html b/internal/ui/static/index.html index fb2500f..d10f806 100644 --- a/internal/ui/static/index.html +++ b/internal/ui/static/index.html @@ -1640,6 +1640,37 @@ + + +
+ Worktree has uncommitted changes + +
+
+

+ +
+ Full git status output + 

+            

+            

+                
+                
+            

+            
+        

+    
+
     
     
         

@@ -2771,11 +2802,25 @@
             dragState.targetId = null;
         }
 
+        // Hide every embedded web-ui panel. The panels are mutually
+        // exclusive, and EVERY "no web instance on screen" path must hide
+        // ALL of them: hiding only the older kinds leaves the newest
+        // kind's iframe visible over whatever should be on screen instead
+        // (issue #101 — #dsh-panel survived a switch to an instance-less
+        // worktree and covered its empty state). Single source of truth
+        // so the next kind cannot be forgotten here.
+        function hideWebPanels() {
+            const ocPanel = document.getElementById('opencode-panel');
+            const rxPanel = document.getElementById('reasonix-panel');
+            const dshPanel = document.getElementById('dsh-panel');
+            if (ocPanel) ocPanel.hidden = true;
+            if (rxPanel) rxPanel.hidden = true;
+            if (dshPanel) dshPanel.hidden = true;
+        }
+
         function renderWorkspace() {
             const empty = document.getElementById("empty-state");
             const termContainer = document.getElementById("terminal-container");
-            const ocPanel = document.getElementById("opencode-panel");
-            const rxPanel = document.getElementById("reasonix-panel");
             const actions = document.getElementById("instance-actions");
             const controls = document.getElementById("terminal-controls");
 
@@ -2802,8 +2847,7 @@
                 // renderer and hide the iframe.
                 if (!isWeb) {
                     termContainer.style.display = "block";
-                    if (ocPanel) ocPanel.hidden = true;
-                    if (rxPanel) rxPanel.hidden = true;
+                    hideWebPanels();
                 }
                 actions.classList.add("visible");
 
@@ -2826,8 +2870,10 @@
                 empty.style.display = "block";
                 empty.textContent = state.activeWT ? "No instances. Click + to start one." : "Select a worktree.";
                 termContainer.style.display = "none";
-                if (ocPanel) ocPanel.hidden = true;
-                if (rxPanel) rxPanel.hidden = true;
+                // No active instance: every web panel must be hidden, or a
+                // previous worktree's keep-alive iframe covers the empty
+                // state (issue #101).
+                hideWebPanels();
                 if (controls) controls.classList.toggle("oc-hidden", true);
                 actions.classList.remove("visible");
             }
@@ -2869,9 +2915,39 @@
         }
 
         // Interaction
+        // Deactivate the renderer of a (previously active) instance on a
+        // full leave. Kind-agnostic by design: web-ui kinds hide their
+        // panel AND stop their background polling, terminal kinds hide
+        // their host. Unconditional — no same-renderer guard like
+        // selectInstance's: a worktree switch is always a full leave,
+        // never a same-renderer re-activation. An unresolvable id
+        // (concurrent deletion) is skipped rather than falling back to
+        // 'pty': which renderer is actually on screen is unknowable, and
+        // the caller's render() hides every panel regardless (issue #101).
+        function deactivateInstanceRenderer(id) {
+            if (!id) return;
+            const inst = state.instances.find(i => i.id === id);
+            if (!inst) return;
+            const renderer = (window.KindRenderers || {})[inst.kind || 'pty'];
+            if (renderer && typeof renderer.deactivate === 'function') {
+                renderer.deactivate();
+            }
+        }
+
         function selectWorktree(id) {
             if (state.activeWT === id) return;
             const previousID = state.activeInst;
+            // Leaving the worktree also leaves its active instance:
+            // deactivate its renderer BEFORE clearing the selection and
+            // re-rendering — renderWorkspace reads the just-nulled
+            // state.activeInst and paints the new worktree's empty state,
+            // so a deactivate after render() would flash that empty state
+            // over the still-visible old iframe. maybeDestroyInactiveStoppedSession
+            // below cannot do it — its terminal-bucket guard returns early for
+            // web instances, so without this call a dsh-web iframe stayed
+            // visible over the next (instance-less) worktree's empty state and
+            // its renderer kept polling in the background (issue #101).
+            deactivateInstanceRenderer(previousID);
             state.activeWT = id;
             state.activeInst = null; // Reset instance selection
             maybeDestroyInactiveStoppedSession(previousID);
@@ -2889,8 +2965,20 @@
             gitChangesTimer = setInterval(refreshGitChanges, 10000);
         }
 
+        // Programmatic worktree switch used by the create/import flows.
+        // Same leave semantics as selectWorktree: deactivate the previous
+        // instance's renderer and clear the selection SYNCHRONOUSLY, or
+        // the old worktree's web iframe stays visible (and keeps polling)
+        // over the new worktree until the refresh round-trip finishes
+        // (issue #101 review).
         function selectWorktreeByID(id) {
+            if (state.activeWT === id) return;
+            const previousID = state.activeInst;
+            deactivateInstanceRenderer(previousID);
             state.activeWT = id;
+            state.activeInst = null;
+            maybeDestroyInactiveStoppedSession(previousID);
+            render();
             refresh();
         }
 
@@ -2921,12 +3009,7 @@
             // dispatching so the activated renderer's panel is the only
             // visible one (prevents the stacked half/half layout when
             // switching between web-ui kinds).
-            const ocPanel = document.getElementById('opencode-panel');
-            const rxPanel = document.getElementById('reasonix-panel');
-            const dshPanel = document.getElementById('dsh-panel');
-            if (ocPanel) ocPanel.hidden = true;
-            if (rxPanel) rxPanel.hidden = true;
-            if (dshPanel) dshPanel.hidden = true;
+            hideWebPanels();
             // Deactivate the previously-active renderer first so it
             // can release any listeners / hide its panel cleanly.
             if (previousID) {
@@ -3276,10 +3359,106 @@
             ev.stopPropagation();
             if (!confirm("Delete worktree?")) return;
             try {
-                await api("/api/worktrees/delete", { method: "POST", body: JSON.stringify({ id }) });
+                const res = await api("/api/worktrees/delete", { method: "POST", body: JSON.stringify({ id }) });
                 if (state.activeWT === id) state.activeWT = null;
                 await refresh();
-            } catch (e) { alert(e); }
+                // Issue #102 review: a clean-looking delete still destroys
+                // every gitignored file with the directory — surface the
+                // count instead of letting it vanish silently.
+                if (res && res.ignored_destroyed > 0) {
+                    alert("Worktree deleted. Note: " + res.ignored_destroyed + " gitignored " +
+                        (res.ignored_destroyed === 1 ? "entry" : "entries") +
+                        " (build caches, logs, backups that existed only in this worktree) were destroyed with the directory.");
+                }
+            } catch (e) {
+                // Issue #102: a dirty-worktree refusal is structured — open
+                // the details dialog (breakdown, full git status output,
+                // gitignored-file risk, force exit) instead of a flat alert.
+                if (e && e.body && e.body.error === "worktree_dirty") {
+                    showWorktreeDirtyDialog(id, e.body);
+                    return;
+                }
+                alert(e);
+            }
+        }
+
+        // Worktree dirty-delete dialog (issue #102). The refusal used to
+        // say only "delete is refused" — indistinguishable between "a
+        // scratch file was left" and "the workspace was damaged
+        // beforehand" (133 tracked files missing in the report). The
+        // server now sends the breakdown; this dialog renders it and
+        // offers the force-delete exit behind a two-step confirm.
+        let dirtyDeleteID = null;
+
+        // Esc closes a  natively without running
+        // closeWorktreeDirtyDialog — reset the dangling state the same way
+        // (issue #102 review).
+        document.getElementById('modal-wt-dirty').addEventListener('cancel', () => {
+            dirtyDeleteID = null;
+        });
+
+        function showWorktreeDirtyDialog(id, body) {
+            dirtyDeleteID = id;
+            const d = (body && body.dirty) || {};
+            const entries = d.entries || 0;
+            let summary = entries + " uncommitted entr" + (entries === 1 ? "y" : "ies") +
+                " (" + (d.deleted || 0) + " deleted, " + (d.modified || 0) + " modified, " + (d.untracked || 0) + " untracked" +
+                (d.added ? ", " + d.added + " added" : "") + (d.renamed ? ", " + d.renamed + " renamed" : "") + ").";
+            if (d.first_paths && d.first_paths.length) {
+                summary += " First paths: " + d.first_paths.join(", ") + ".";
+            }
+            document.getElementById('wt-dirty-summary').textContent = summary;
+            const ignoredEl = document.getElementById('wt-dirty-ignored');
+            if (d.ignored > 0) {
+                ignoredEl.textContent = "⚠ " + d.ignored + " gitignored entr" + (d.ignored === 1 ? "y" : "ies") +
+                    " (build caches, logs, backups) exist only in this worktree — the dirty check does not block on them, but force delete destroys them unrecoverably.";
+                ignoredEl.hidden = false;
+            } else {
+                ignoredEl.hidden = true;
+            }
+            document.getElementById('wt-dirty-porcelain').textContent = d.porcelain || "";
+            const errorEl = document.getElementById('wt-dirty-error');
+            errorEl.hidden = true;
+            // Reset the two-step confirm on every open.
+            const btn = document.getElementById('wt-dirty-force');
+            btn.dataset.confirm = "";
+            btn.textContent = "Force delete";
+            document.getElementById('modal-wt-dirty').showModal();
+        }
+
+        function closeWorktreeDirtyDialog() {
+            const dlg = document.getElementById('modal-wt-dirty');
+            if (dlg && dlg.open) dlg.close();
+            dirtyDeleteID = null;
+        }
+
+        async function forceDeleteDirtyWorktree() {
+            const id = dirtyDeleteID;
+            if (!id) return;
+            const btn = document.getElementById('wt-dirty-force');
+            // Two-step confirm (same pattern as the dsh install button):
+            // force delete is unrecoverable — tracked changes AND every
+            // gitignored file vanish with the directory.
+            if (btn.dataset.confirm !== "1") {
+                btn.dataset.confirm = "1";
+                btn.textContent = "Confirm force delete? All changes and gitignored files are destroyed";
+                return;
+            }
+            const errorEl = document.getElementById('wt-dirty-error');
+            try {
+                await api("/api/worktrees/delete", { method: "POST", body: JSON.stringify({ id, force: true }) });
+                closeWorktreeDirtyDialog();
+                if (state.activeWT === id) state.activeWT = null;
+                await refresh();
+            } catch (e) {
+                btn.dataset.confirm = "";
+                btn.textContent = "Force delete";
+                errorEl.hidden = false;
+                // A structured refusal body carries the human summary in
+                // body.message — e.message alone is the raw error CODE
+                // ("worktree_dirty") for those (issue #102 review).
+                errorEl.textContent = (e && e.body && e.body.message) || (e && e.message) || String(e);
+            }
         }
 
         async function doStartInstance() {
@@ -4440,6 +4619,20 @@
                 if (session.ttyOffset > 0) url += `&since=${session.ttyOffset}`;
                 else if (session.ttyOffset === CURSOR_FOLLOW_LIVE_END) url += `&since=${CURSOR_FOLLOW_LIVE_END}`;
 
+                // Capability opt-in (issue #98): this page's
+                // parseTTYControlMessage whitelists the TEXT
+                // {"type":"ping"} heartbeat and the {"type":"sync"}
+                // offset echo (issue #87), so declare both in caps and
+                // the server sends them; a v0.5.0 page that opts into
+                // neither is never sent either text frame, because it
+                // would paint them into the terminal. (Pre-caps pages
+                // that DO carry a `since` cursor still receive the sync
+                // echo — the parameter and the sync whitelist shipped
+                // together in #87, so presenting one proves parsing the
+                // other.) Older servers ignore the parameter, so this
+                // is safe against every daemon version.
+                url += `&caps=ping,sync`;
+
                 resetTTYOutputDecoder(session);
                 const ws = new WebSocket(url);
                 ws.binaryType = "arraybuffer";
@@ -4455,14 +4648,32 @@
                 // first of them and could land PAST bytes the client never
                 // received — strictly worse than duplicating them. The sync
                 // frame is the only frame carrying an authoritative ABSOLUTE
-                // offset, so it is the one that moves the cursor, publishing
-                // the end offset of the whole replay in a single step. Blast
-                // radius if the socket dies in the replay→sync gap: the cursor
-                // never advanced, so the reconnect re-requests from where the
-                // client really was and re-paints up to
-                // ttyHandshakeReplayBudget (8MB) now that the replay streams,
-                // instead of a single 64KB frame — bounded duplication that
-                // self-corrects on that reconnect.
+                // offset, so it is the one that moves the cursor. A post-#94
+                // server sends it TWICE on the streaming catch-up path: a
+                // START sync ahead of the first replay chunk carrying the
+                // replay's real (clamped) start offset S, which lifts this
+                // latch at chunk 1 so the cursor advances frame-by-frame
+                // from S — a socket that dies mid-replay then resumes from
+                // the bytes already painted instead of re-pulling the whole
+                // replay — and the CLOSING sync after the last chunk
+                // carrying the end offset, which re-pins the cursor
+                // absolutely. Against a pre-#94 server only the closing
+                // sync arrives, and the blast radius of dying in the
+                // replay→sync gap is the old one: the cursor never advanced,
+                // so the reconnect re-requests from where the client really
+                // was and re-paints up to ttyHandshakeReplayBudget (8MB) —
+                // bounded duplication that self-corrects on that reconnect.
+                // Recorded trade-off of the start sync (issue #94 review):
+                // a drop landing between a frame that ends mid-UTF-8-
+                // sequence and the next one discards those 1-3 lead bytes
+                // (resetTTYOutputDecoder runs in ws.onclose) while the
+                // cursor has already stepped past them, so the character
+                // renders as U+FFFD where the pre-#94 full re-pull
+                // redelivered it. "Either way" names the two paths that
+                // have ALREADY stepped the cursor past those lead bytes
+                // — this replay path post-#94 and the live path, which
+                // has always had the same trade-off: cosmetic only, and
+                // the split character is unrecoverable on both.
                 let handshakeSynced = false;
                 let handshakeTimeout = null;
                 let connectTimeout = setTimeout(() => {
@@ -4640,6 +4851,23 @@
                         // Handshake offset echo (issue #87): the server tells
                         // us the end offset of the replay it just sent, so a
                         // later reconnect can resume incrementally with it.
+                        // Since issue #94 the streaming catch-up path sends
+                        // this frame TWICE — a START sync (the replay's real,
+                        // possibly clamped, start offset) ahead of the first
+                        // binary chunk, then the CLOSING sync (the end
+                        // offset) after the last one. This handler needs no
+                        // distinction: every sync carries an ABSOLUTE offset,
+                        // so adopting it and lifting the latch is correct for
+                        // both — the start sync makes the per-frame advance
+                        // below truthful from chunk 1 on, and the closing
+                        // sync re-pins the value the frames summed to. The
+                        // sum can land SHORT only if the ring rolled a full
+                        // cycle mid-replay and evicted bytes the loop had
+                        // not sent yet (a later chunk then clamps above the
+                        // running cursor): the closing sync jumps the gap,
+                        // which is truthful — those bytes are undeliverable
+                        // anyway, the same rule the read→subscribe window
+                        // already lives by.
                         if (Number.isFinite(controlMsg.offset) && controlMsg.offset >= 0) session.ttyOffset = controlMsg.offset;
                         handshakeSynced = true;
                         return;
@@ -4668,9 +4896,15 @@
                             // broadcast, so wire bytes and buffered bytes are
                             // the same bytes. BEFORE the latch the frames are
                             // still replay bytes and must not touch the
-                            // cursor: the sync frame that follows on the same
-                            // TCP stream publishes the authoritative end
-                            // offset of the entire replay in one step.
+                            // cursor: the replay need not start where we asked
+                            // (the stale-cursor clamp), so only an ABSOLUTE
+                            // offset may move it. Post-#94 that is the START
+                            // sync, which arrives ahead of the first replay
+                            // chunk, so the latch is normally already lifted
+                            // here and replay frames advance the cursor as
+                            // they paint; against a pre-#94 server the latch
+                            // lifts only at the closing sync, which publishes
+                            // the end offset of the entire replay in one step.
                             if (handshakeSynced) session.ttyOffset += u8.length;
                         }
                     } else if (session.term) {
diff --git a/internal/ui/terminal_status_test.go b/internal/ui/terminal_status_test.go
index ca7a41d..36bec53 100644
--- a/internal/ui/terminal_status_test.go
+++ b/internal/ui/terminal_status_test.go
@@ -36,6 +36,18 @@ func TestTerminalStatusHandling(t *testing.T) {
 // a file holding 27) — a hand-maintained count in prose is a claim nobody
 // checks. Derive it here instead: count the cases, then require the entry to
 // say the same thing.
+//
+// Issue #95: EVERY occurrence of the claim phrase must equal the derived
+// count, not just the first match. The previous first-match lookup silently
+// re-anchored the guard to whichever entry happened to sit highest in the
+// file (entries are prepended in reverse chronological order), so a new
+// entry quoting the phrase with a stale number would both hijack the
+// constraint and leave the real claim unchecked. Requiring every occurrence
+// to match makes the phrase position-independent — it now means "the file's
+// total case count" wherever it appears (subset counts use different
+// phrasing, e.g. #83's "fourteen new #83 cases"). Of the issue's two
+// proposals this was chosen over per-entry anchoring as the simpler
+// invariant to keep unambiguous as entries accumulate.
 func TestTerminalStatusChangelogCount(t *testing.T) {
 	src, err := os.ReadFile("testdata/terminal_status.test.mjs")
 	if err != nil {
@@ -50,13 +62,18 @@ func TestTerminalStatusChangelogCount(t *testing.T) {
 	if err != nil {
 		t.Fatalf("read CHANGELOG.md: %v", err)
 	}
-	claimed := regexp.MustCompile(`Pinned by (\d+) ` + "`" + `node --test` + "`" + ` cases`).FindSubmatch(changelog)
-	if claimed == nil {
+	claims := regexp.MustCompile(`Pinned by (\d+) `+"`"+`node --test`+"`"+` cases`).FindAllSubmatchIndex(changelog, -1)
+	if len(claims) == 0 {
 		t.Fatal("CHANGELOG.md no longer says \"Pinned by N `node --test` cases\"")
 	}
-	if got, err := strconv.Atoi(string(claimed[1])); err != nil || got != cases {
-		t.Fatalf("CHANGELOG.md claims %s `node --test` cases but testdata/terminal_status.test.mjs has %d",
-			claimed[1], cases)
+	for _, loc := range claims {
+		num := changelog[loc[2]:loc[3]]
+		// Report the line so a multi-occurrence violation needs no grep.
+		line := 1 + strings.Count(string(changelog[:loc[0]]), "\n")
+		if got, err := strconv.Atoi(string(num)); err != nil || got != cases {
+			t.Errorf("CHANGELOG.md:%d claims %s `node --test` cases but testdata/terminal_status.test.mjs has %d — EVERY occurrence of the phrase must state the file's total (issue #95)",
+				line, num, cases)
+		}
 	}
 }
 
@@ -87,6 +104,26 @@ func TestTerminalStatusLivenessHeartbeatContract(t *testing.T) {
 	if !strings.Contains(index, `msg.type !== "ping" && msg.type !== "pong"`) {
 		t.Fatal(`parseTTYControlMessage must whitelist both "ping" and "pong" (issue #83)`)
 	}
+	// Issue #98: the TEXT ping heartbeat and the sync offset echo are
+	// opt-in on both halves of the wire — the client must DECLARE the
+	// capabilities and the server must GATE on them (a client without the
+	// whitelist would paint the ping frame every 10s and the sync frame
+	// once per connect).
+	if !strings.Contains(index, "&caps=ping,sync") {
+		t.Fatal(`connectTTY must declare caps=ping,sync so the server sends the TEXT ping heartbeat and the sync offset echo (issue #98)`)
+	}
+	if !strings.Contains(string(appSrc), `ttyClientHasCap(caps, "ping")`) {
+		t.Fatal("the server must gate the TEXT ping heartbeat on the client's caps opt-in (issue #98)")
+	}
+	if !strings.Contains(string(appSrc), `ttyClientHasCap(caps, "sync")`) {
+		t.Fatal("the server must gate the sync offset echo on the client's caps opt-in (issue #98)")
+	}
+	if !strings.Contains(string(appSrc), `r.URL.Query()["since"]`) {
+		t.Fatal("the sync echo must also flow to a client presenting an explicit since cursor — the inferred #87-era opt-in (issue #98 review round 2)")
+	}
+	if !strings.Contains(string(appSrc), `r.URL.Query()["caps"]`) {
+		t.Fatal(`caps must be scanned across ALL repeated parameter values (Query()["caps"]), not Get's first value only`)
+	}
 	// The stamp must be the FIRST thing ws.onmessage does - before any
 	// branch - so EVERY frame (control or binary) refreshes it. Anchoring on
 	// the following line proves nothing was reordered between them.
diff --git a/internal/ui/testdata/terminal_status.test.mjs b/internal/ui/testdata/terminal_status.test.mjs
index 5418af0..9e93d92 100644
--- a/internal/ui/testdata/terminal_status.test.mjs
+++ b/internal/ui/testdata/terminal_status.test.mjs
@@ -1070,6 +1070,55 @@ test("issue #87: binary frames move the cursor only after the sync latch (FIX-B)
   assert.equal(h.api.parseTTYControlMessage('{"type":"bogus"}'), null);
 });
 
+test("issue #94: the start sync lifts the latch at chunk 1, so a mid-replay drop resumes", () => {
+  const h = ttyWsHarness({ ttyOffset: 13 });
+  h.connect();
+  const ws = h.sockets[0];
+  assert.ok(ws.url.includes("&since=13"), "the reconnect carries the cursor");
+  ws.readyState = 1;
+  ws.onopen();
+  ws.onmessage({ data: '{"type":"ready"}' });
+
+  // A post-#94 server brackets a streaming replay with TWO sync frames: the
+  // start sync ahead of chunk 1 (the replay's real, possibly clamped, start
+  // offset — S and chunk 1 come from the same server-side read) and the
+  // closing sync after the last chunk. The "start" field is only a marker
+  // for consumers that must tell the two apart; the handler treats every
+  // sync as an absolute offset to adopt.
+  ws.onmessage({ data: '{"type":"sync","offset":13,"start":true}' });
+  assert.equal(h.session.ttyOffset, 13, "the start sync adopts the replay's real start offset");
+
+  // Replay chunks after the start sync are safe to count frame-by-frame:
+  // the base is truthful (never the raw, maybe-clamped `since`), unlike the
+  // pre-sync frames pinned by the FIX-B case above.
+  ws.onmessage({ data: h.enc.encode("chunk-1-").buffer }); // 8 bytes
+  ws.onmessage({ data: h.enc.encode("chunk-2").buffer }); // 7 bytes
+  assert.equal(h.session.ttyOffset, 28, "replay frames advance the cursor once the start offset is known");
+  assert.deepEqual(h.writes, ["chunk-1-", "chunk-2"], "the bytes still render in order");
+
+  // Die MID-REPLAY — before the closing sync. The cursor already stands at
+  // the rendered byte count, so the reconnect RESUMES from it instead of
+  // re-pulling the whole replay from 13 (the pre-#94 death window, up to
+  // ttyHandshakeReplayBudget under a degrading link).
+  ws.close();
+  h.fireTimers(5000);
+  assert.equal(h.sockets.length, 2, "the reconnect opened a new socket");
+  assert.ok(h.sockets[1].url.includes("&since=28"),
+    `a mid-replay drop must resume from the rendered cursor, got ${h.sockets[1].url}`);
+
+  // The resume's own replay is bracketed the same way, and its closing sync
+  // re-pins the absolute end — the same value the frames summed to.
+  const ws2 = h.sockets[1];
+  ws2.readyState = 1;
+  ws2.onopen();
+  ws2.onmessage({ data: '{"type":"ready"}' });
+  ws2.onmessage({ data: '{"type":"sync","offset":28,"start":true}' });
+  ws2.onmessage({ data: h.enc.encode("rest").buffer }); // 4 bytes
+  assert.equal(h.session.ttyOffset, 32, "the resume advances from its own start offset");
+  ws2.onmessage({ data: '{"type":"sync","offset":32}' });
+  assert.equal(h.session.ttyOffset, 32, "the closing sync republishes the value the frames already summed to");
+});
+
 test("issue #87: the cursor survives the drop and the reconnect resumes from it", () => {
   const h = ttyWsHarness();
   h.connect();
diff --git a/internal/ui/ui_test.go b/internal/ui/ui_test.go
index 1f1b4a9..bc79da9 100644
--- a/internal/ui/ui_test.go
+++ b/internal/ui/ui_test.go
@@ -304,6 +304,172 @@ func TestWebRenderersStoppedSwitchHidesAllFrames(t *testing.T) {
 	}
 }
 
+// sliceJSFunction returns index.html's top-level `function name(...) {…}`,
+// from the declaration to the start of the next (async) function declared
+// at the SAME indentation. The indent is read off the declaration line
+// rather than hardcoded: a re-indented script block must not silently
+// extend every slice to the end of the file, where assertions would match
+// text belonging to a different function and keep passing.
+func sliceJSFunction(t *testing.T, js, name string) string {
+	t.Helper()
+	start := strings.Index(js, "function "+name+"(")
+	if start < 0 {
+		t.Fatalf("index.html: function %s not found", name)
+	}
+	lineStart := strings.LastIndex(js[:start], "\n") + 1
+	indent := js[lineStart:start]
+	if strings.Trim(indent, " \t") != "" {
+		t.Fatalf("index.html: function %s is not declared at line start — cannot derive a slice end marker", name)
+	}
+	rest := js[start:]
+	end := len(rest)
+	for _, marker := range []string{"\n" + indent + "function ", "\n" + indent + "async function "} {
+		if i := strings.Index(rest, marker); i >= 0 && i < end {
+			end = i
+		}
+	}
+	return rest[:end]
+}
+
+// TestEmptyWorktreeSwitchHidesAllWebPanels pins the issue #101 fix: switching
+// to a worktree with ZERO instances must not leave the previous worktree's
+// dsh-web iframe on screen. Three defects combined: (A) renderWorkspace's
+// no-active-instance branch hid only the opencode/reasonix panels and forgot
+// #dsh-panel (whose CSS keeps it full-size until [hidden] is set); (B)
+// selectWorktree never called the previous renderer's deactivate()
+// (maybeDestroyInactiveStoppedSession's terminal-bucket guard returns early
+// for web instances), so the dsh-web panel stayed visible AND its background
+// polling kept running; (C) selectInstance(null) early-returns on the empty
+// worktree path, so its panel-hiding lines never execute there. The fix
+// extracts one hideWebPanels() helper as the single source of truth used by
+// renderWorkspace and selectInstance, and makes worktree switches deactivate the
+// previous renderer through the shared deactivateInstanceRenderer helper
+// (without selectInstance's prevRenderer !== renderer guard — a worktree
+// switch is always a full leave), synchronously, in BOTH switch entry
+// points: selectWorktree and selectWorktreeByID (create/import flows).
+func TestEmptyWorktreeSwitchHidesAllWebPanels(t *testing.T) {
+	indexJS := fetchStaticAsset(t, "/")
+
+	// (A) One helper hides ALL three web panels — adding a kind and hiding
+	// only the older panels is exactly how #dsh-panel was forgotten.
+	helper := sliceJSFunction(t, indexJS, "hideWebPanels")
+	for _, panelID := range []string{"'opencode-panel'", "'reasonix-panel'", "'dsh-panel'"} {
+		if !strings.Contains(helper, panelID) {
+			t.Fatalf("hideWebPanels() does not hide %s — a forgotten panel stays visible over the empty state (issue #101)", panelID)
+		}
+	}
+
+	// renderWorkspace's no-active-instance branch must hide every web panel
+	// through the shared helper (this is the branch the empty worktree lands
+	// on, and it is the visual backstop for every path into it).
+	rw := sliceJSFunction(t, indexJS, "renderWorkspace")
+	emptyStart := strings.Index(rw, `empty.style.display = "block";`)
+	if emptyStart < 0 {
+		t.Fatal("renderWorkspace lost its no-active-instance branch (empty.style.display = \"block;\")")
+	}
+	emptyEnd := strings.Index(rw[emptyStart:], "renderTerminalSessions();")
+	if emptyEnd < 0 {
+		t.Fatal("renderWorkspace empty branch does not end in renderTerminalSessions(); anymore — re-check the slice")
+	}
+	emptyBranch := rw[emptyStart : emptyStart+emptyEnd]
+	if !strings.Contains(emptyBranch, "hideWebPanels()") {
+		t.Fatal("renderWorkspace's empty branch does not hide all web panels — a previous worktree's web iframe covers the empty state (issue #101)")
+	}
+	// The non-web active-instance branch hides panels too; it must use the
+	// same helper rather than an inline subset that can forget a kind.
+	if !strings.Contains(rw[:emptyStart], "hideWebPanels()") {
+		t.Fatal("renderWorkspace's non-web branch no longer hides web panels via hideWebPanels()")
+	}
+
+	// selectInstance hides panels before dispatching to the kind renderer;
+	// it must go through the same helper, not an inline copy that can drift.
+	si := sliceJSFunction(t, indexJS, "selectInstance")
+	if !strings.Contains(si, "hideWebPanels()") {
+		t.Fatal("selectInstance no longer hides web panels via hideWebPanels() — the two call sites can drift (issue #101)")
+	}
+
+	// (B) selectWorktree must deactivate the previous instance's renderer so
+	// web kinds hide their panel and stop polling. It must NOT carry
+	// selectInstance's prevRenderer !== renderer guard: a worktree switch is
+	// always a full leave, and the guard would skip deactivation whenever the
+	// auto-selected instance of the target worktree shares the kind.
+	// Assert the POSITION, not just presence: render() → renderWorkspace
+	// reads the just-nulled state.activeInst and paints the empty state, so
+	// a deactivate() that ran after render() would flash that empty state
+	// over the still-visible old iframe — the exact #101 symptom class.
+	sw := sliceJSFunction(t, indexJS, "selectWorktree")
+	deactIdx := strings.Index(sw, "deactivateInstanceRenderer(previousID)")
+	if deactIdx < 0 {
+		t.Fatal("selectWorktree does not deactivate the previous renderer — web iframes stay visible and keep polling across worktree switches (issue #101)")
+	}
+	assignIdx := strings.Index(sw, "state.activeWT = id;")
+	if assignIdx < 0 || deactIdx > assignIdx {
+		t.Fatal("selectWorktree deactivates the previous renderer only after state.activeWT = id — render() then paints the new worktree's empty state over the still-visible old iframe (issue #101)")
+	}
+	if strings.Contains(sw, "prevRenderer !== renderer") {
+		t.Fatal("selectWorktree must deactivate unconditionally — the prevRenderer !== renderer guard skips same-kind leaves (issue #101)")
+	}
+
+	// The shared leave helper deactivates through the renderer registry and
+	// skips unresolvable ids: on a concurrent deletion the 'pty' fallback
+	// would deactivate the wrong renderer while a web renderer is on screen.
+	leave := sliceJSFunction(t, indexJS, "deactivateInstanceRenderer")
+	if !strings.Contains(leave, ".deactivate()") {
+		t.Fatal("deactivateInstanceRenderer does not call renderer.deactivate() (issue #101)")
+	}
+	if !strings.Contains(leave, "if (!inst) return;") {
+		t.Fatal("deactivateInstanceRenderer lost its unresolvable-id skip — the 'pty' fallback deactivates the wrong renderer (issue #101 review)")
+	}
+
+	// selectWorktreeByID (the create/import flows' switch) must run the
+	// same synchronous leave cleanup: without it the old worktree's web
+	// iframe stays visible — and keeps polling — over the new worktree
+	// until the refresh round-trip finishes.
+	swb := sliceJSFunction(t, indexJS, "selectWorktreeByID")
+	for _, want := range []string{"deactivateInstanceRenderer(previousID)", "state.activeInst = null", "render();"} {
+		if !strings.Contains(swb, want) {
+			t.Fatalf("selectWorktreeByID is missing %q — it bypasses the worktree-switch cleanup and strands the previous renderer (issue #101 review)", want)
+		}
+	}
+	// Same ordering pin as selectWorktree's above: the deactivate must run
+	// BEFORE render(), or render() paints the new worktree's empty state
+	// over the still-visible old iframe — presence alone does not pin this.
+	swbDeact := strings.Index(swb, "deactivateInstanceRenderer(previousID)")
+	swbRender := strings.Index(swb, "render();")
+	if swbDeact < 0 || swbRender < 0 || swbDeact > swbRender {
+		t.Fatal("selectWorktreeByID deactivates the previous renderer only after render() — the empty state flashes over the still-visible old iframe (issue #101 review)")
+	}
+}
+
+// TestWorktreeDirtyDeleteDialog pins the issue #102 UI contract: a
+// dirty-worktree delete refusal is STRUCTURED (error code "worktree_dirty"
+// plus a breakdown), and the dashboard must open the details dialog —
+// summary, collapsible full git status output, the gitignored force-risk
+// warning — with the force-delete exit behind a two-step confirm, instead
+// of the old flat alert that hid what force would destroy.
+func TestWorktreeDirtyDeleteDialog(t *testing.T) {
+	indexJS := fetchStaticAsset(t, "/")
+
+	checks := []struct{ what, anchor string }{
+		{"the dirty-details dialog exists", `id="modal-wt-dirty"`},
+		{"deleteWorktree matches the structured refusal code", `e.body.error === "worktree_dirty"`},
+		{"the refusal routes to the dialog", "showWorktreeDirtyDialog(id, e.body)"},
+		{"the force exit resends with force:true", `{ id, force: true }`},
+		{"the collapsible full git status output is rendered", "wt-dirty-porcelain"},
+		{"the gitignored force-risk warning is rendered", "wt-dirty-ignored"},
+		{"the force delete is a two-step confirm", "Confirm force delete?"},
+		{"the confirm state resets per open", `btn.dataset.confirm = ""`},
+		{"a failed force delete shows the human summary, not the raw error code", `(e && e.body && e.body.message) || (e && e.message)`},
+		{"Esc (cancel) resets the dangling dialog state", `.addEventListener('cancel', () => {`},
+		{"a clean delete surfaces the destroyed gitignored count", "res.ignored_destroyed > 0"},
+	}
+	for _, c := range checks {
+		if !strings.Contains(indexJS, c.anchor) {
+			t.Fatalf("index.html: %s — anchor %q not found (issue #102)", c.what, c.anchor)
+		}
+	}
+}
+
 func TestReasonixRendererKeepsFrameAlive(t *testing.T) {
 	mux := http.NewServeMux()
 	if err := Register(mux, "myworktree", nil); err != nil {
diff --git a/internal/worktree/worktree.go b/internal/worktree/worktree.go
index 56a0855..365b425 100644
--- a/internal/worktree/worktree.go
+++ b/internal/worktree/worktree.go
@@ -318,10 +318,150 @@ func (m Manager) worktreesRoot() (root string, legacy bool, err error) {
 	return filepath.Join(m.GitRoot, v), false, nil
 }
 
-func (m Manager) Delete(id string) error {
+// DirtyWorktreeError is returned by Delete when the worktree carries
+// uncommitted or untracked changes and force was not set (issue #102). It
+// carries the full breakdown so API clients can render the details: the
+// previous flat message — "worktree has uncommitted or untracked changes;
+// delete is refused" — gave no way to tell "I left a scratch file" (low
+// risk) from "133 tracked source files vanished BEFORE the delete attempt"
+// (the workspace is damaged; restore it instead of deleting), two
+// situations that demand OPPOSITE reactions.
+type DirtyWorktreeError struct {
+	Entries    int      // total git status --porcelain lines
+	Deleted    int      // lines with D in either status column
+	Modified   int      // remaining modification lines (M/T/U in either column)
+	Added      int      // lines with A in either status column
+	Renamed    int      // lines with R/C in either status column
+	Untracked  int      // "??" lines
+	Ignored    int      // gitignored entries present — invisible to the blocking check (which runs without --ignored) but destroyed unrecoverably by a force delete
+	FirstPaths []string // up to maxDirtyFirstPaths raw path fields, in status order
+	Porcelain  string   // the git status --porcelain -uall output for UI expansion, capped at maxDirtyPorcelainLines lines
+}
+
+// maxDirtyFirstPaths caps how many paths the refusal message names inline;
+// the remaining paths are in Porcelain (itself capped at
+// maxDirtyPorcelainLines lines).
+const maxDirtyFirstPaths = 5
+
+// maxDirtyPorcelainLines bounds the status output carried in the refusal.
+// The probe runs with -uall so untracked directories count file-by-file —
+// an untracked dependency dir (node_modules) would otherwise expand the
+// API/UI payload to tens of thousands of lines.
+const maxDirtyPorcelainLines = 200
+
+// capDirtyPorcelain truncates the carried status output; bucket counts and
+// FirstPaths are always computed from the FULL output before capping.
+func capDirtyPorcelain(out string) string {
+	lines := strings.Split(out, "\n")
+	if len(lines) <= maxDirtyPorcelainLines {
+		return out
+	}
+	rest := len(lines) - maxDirtyPorcelainLines
+	word := "lines"
+	if rest == 1 {
+		word = "line"
+	}
+	return fmt.Sprintf("%s\n… (%d more %s truncated)", strings.Join(lines[:maxDirtyPorcelainLines], "\n"), rest, word)
+}
+
+func (e *DirtyWorktreeError) Error() string {
+	entryWord := "entries"
+	if e.Entries == 1 {
+		entryWord = "entry"
+	}
+	var b strings.Builder
+	fmt.Fprintf(&b, "worktree has uncommitted or untracked changes; delete is refused: %d %s (%d deleted, %d modified, %d untracked",
+		e.Entries, entryWord, e.Deleted, e.Modified, e.Untracked)
+	if e.Added > 0 {
+		fmt.Fprintf(&b, ", %d added", e.Added)
+	}
+	if e.Renamed > 0 {
+		fmt.Fprintf(&b, ", %d renamed", e.Renamed)
+	}
+	b.WriteString(")")
+	if len(e.FirstPaths) > 0 {
+		fmt.Fprintf(&b, "; first paths: %s", strings.Join(e.FirstPaths, ", "))
+	}
+	if e.Ignored > 0 {
+		ignoredWord := "entries"
+		if e.Ignored == 1 {
+			ignoredWord = "entry"
+		}
+		fmt.Fprintf(&b, "; additionally %d gitignored %s present, which a force delete would destroy unrecoverably",
+			e.Ignored, ignoredWord)
+	}
+	b.WriteString("; retry with force to delete anyway")
+	return b.String()
+}
+
+// summarizePorcelain classifies `git status --porcelain` (v1) lines into the
+// DirtyWorktreeError buckets. Every classified line counts in EXACTLY ONE
+// bucket — the two status columns collapse by priority
+// (delete > rename > add > modify) — so the buckets always sum to Entries.
+// "??" lines are untracked; "!!" ignored lines are skipped outright (they
+// never occur here in production — the blocking check runs without
+// --ignored). FirstPaths is capped at maxDirtyFirstPaths.
+func (e *DirtyWorktreeError) summarizePorcelain(out string) {
+	for _, line := range strings.Split(out, "\n") {
+		if strings.TrimSpace(line) == "" {
+			continue
+		}
+		if strings.HasPrefix(line, "!!") {
+			// Ignored entries never reach this summary in production
+			// (the blocking check runs WITHOUT --ignored); skip them
+			// defensively so a caller passing the --ignored output
+			// cannot misclassify them as modifications.
+			continue
+		}
+		e.Entries++
+		xy, path := line, ""
+		if len(line) >= 3 {
+			xy = line[:2]
+			path = strings.TrimSpace(line[3:])
+		}
+		switch {
+		case xy == "??":
+			e.Untracked++
+		case strings.ContainsAny(xy, "D"):
+			e.Deleted++
+		case strings.ContainsAny(xy, "RC"):
+			e.Renamed++
+		case strings.ContainsAny(xy, "A"):
+			e.Added++
+		default:
+			e.Modified++
+		}
+		if path != "" && len(e.FirstPaths) < maxDirtyFirstPaths {
+			e.FirstPaths = append(e.FirstPaths, path)
+		}
+	}
+}
+
+// countIgnoredPorcelain counts the "!!" entries of a
+// `git status --porcelain --ignored` run — the gitignored files/dirs the
+// plain dirty check never sees (issue #102: 89 gitignored files, 64 of them
+// single-copy, survived the dirty check unnoticed in the report).
+func countIgnoredPorcelain(out string) (n int) {
+	for _, line := range strings.Split(out, "\n") {
+		if strings.HasPrefix(line, "!!") {
+			n++
+		}
+	}
+	return n
+}
+
+// Delete removes the worktree and its state record. It refuses a dirty
+// worktree with a *DirtyWorktreeError unless force is set. The returned
+// count reports how many gitignored entries the delete destroyed with the
+// directory — the blocking dirty check never sees them, so surfacing the
+// number keeps a clean-looking delete from silently erasing build caches,
+// logs, or backups that exist only in this worktree (issue #102 review).
+// The count is 0 on the force path, where the status probe is skipped
+// entirely (see below).
+func (m Manager) Delete(id string, force bool) (int, error) {
 	st, err := m.Store.Load()
 	if err != nil {
-		return err
+		return 0, err
 	}
 	idx := -1
 	var wt store.ManagedWorktree
@@ -333,39 +473,93 @@ func (m Manager) Delete(id string) error {
 		}
 	}
 	if idx == -1 {
-		return fmt.Errorf("unknown worktree id: %s", id)
+		return 0, fmt.Errorf("unknown worktree id: %s", id)
 	}
 
+	ignoredDestroyed := 0
 	if _, err := os.Stat(wt.Path); err != nil {
 		if !errors.Is(err, os.ErrNotExist) {
-			return fmt.Errorf("stat worktree failed: %w", err)
+			return 0, fmt.Errorf("stat worktree failed: %w", err)
 		}
 
 		cmd := gitx.GitCommand(10*time.Second, m.GitRoot, "worktree", "prune", "--expire", "now")
 		out, err := cmd.CombinedOutput()
 		if err != nil {
-			return fmt.Errorf("git worktree prune failed: %w: %s", err, strings.TrimSpace(string(out)))
+			return 0, fmt.Errorf("git worktree prune failed: %w: %s", err, strings.TrimSpace(string(out)))
 		}
 	} else {
-		// Strict: refuse to delete if dirty.
-		cmdStatus := gitx.GitCommand(10*time.Second, wt.Path, "status", "--porcelain")
-		statusOut, err := cmdStatus.Output()
-		if err != nil {
-			return fmt.Errorf("git status failed: %w", err)
-		}
-		if strings.TrimSpace(string(statusOut)) != "" {
-			return errors.New("worktree has uncommitted or untracked changes; delete is refused")
+		// Strict: refuse to delete if dirty (unless force). The refusal
+		// carries the full breakdown — counts by category, first paths,
+		// and the gitignored-file risk a force delete would destroy
+		// (issue #102): the flat "delete is refused" message made a
+		// damaged workspace (tracked files missing) indistinguishable
+		// from a leftover scratch file.
+		//
+		// The probe runs ONLY without force: its output builds the
+		// refusal, and gating the escape hatch on it would strand exactly
+		// the workspaces force exists for — on a damaged worktree (e.g.
+		// a corrupt index) git status fails while git worktree remove
+		// --force still succeeds (issue #102 review).
+		if !force {
+			// -uall: untracked/ignored DIRECTORIES count file-by-file —
+			// the default collapse reports "?? node_modules/" as one
+			// entry while a delete would destroy thousands of files.
+			// -c core.quotePath=false: keep non-ASCII paths literal
+			// instead of C-style octal escapes in the refusal.
+			cmdStatus := gitx.GitCommand(10*time.Second, wt.Path, "-c", "core.quotePath=false", "status", "--porcelain", "-uall")
+			statusOut, err := cmdStatus.Output()
+			if err != nil {
+				return 0, fmt.Errorf("git status failed: %w", err)
+			}
+			porcelain := strings.TrimSpace(string(statusOut))
+			if porcelain != "" {
+				dirty := &DirtyWorktreeError{Porcelain: capDirtyPorcelain(porcelain)}
+				dirty.summarizePorcelain(porcelain)
+				// Ignored files do NOT block the delete (the check above
+				// runs without --ignored), so name the risk explicitly: a
+				// force delete destroys them unrecoverably (issue #102 P3).
+				// A failed count is not worth failing the refusal over —
+				// the dirty entries alone justify it.
+				cmdIgnored := gitx.GitCommand(10*time.Second, wt.Path, "-c", "core.quotePath=false", "status", "--porcelain", "--ignored", "-uall")
+				if ignoredOut, ignoredErr := cmdIgnored.Output(); ignoredErr == nil {
+					dirty.Ignored = countIgnoredPorcelain(string(ignoredOut))
+				}
+				return 0, dirty
+			}
+			// Clean per the blocking check — but the delete still
+			// destroys every gitignored file with the directory. Count
+			// them so callers can surface the loss; ignoring the count's
+			// error keeps a broken probe from blocking a clean delete.
+			// Edge case (second review round): on a pathologically large
+			// ignored tree the probe can hit the 10 s GitCommand timeout,
+			// which leaves the count at 0 — accepted, because blocking a
+			// legitimate clean delete on a counting failure is worse than
+			// occasionally missing the note.
+			cmdIgnored := gitx.GitCommand(10*time.Second, wt.Path, "-c", "core.quotePath=false", "status", "--porcelain", "--ignored", "-uall")
+			if ignoredOut, ignoredErr := cmdIgnored.Output(); ignoredErr == nil {
+				ignoredDestroyed = countIgnoredPorcelain(string(ignoredOut))
+			}
 		}
 
-		cmd := gitx.GitCommand(10*time.Second, m.GitRoot, "worktree", "remove", wt.Path)
+		args := []string{"worktree", "remove"}
+		if force {
+			// `git worktree remove` refuses a dirty worktree on its own;
+			// --force is the explicit override the caller opted into.
+			args = append(args, "--force")
+		}
+		args = append(args, wt.Path)
+		cmd := gitx.GitCommand(10*time.Second, m.GitRoot, args...)
 		removeOut, err := cmd.CombinedOutput()
 		if err != nil {
-			return fmt.Errorf("git worktree remove failed: %w: %s", err, strings.TrimSpace(string(removeOut)))
+			return 0, fmt.Errorf("git worktree remove failed: %w: %s", err, strings.TrimSpace(string(removeOut)))
 		}
 	}
 
 	st.Worktrees = append(st.Worktrees[:idx], st.Worktrees[idx+1:]...)
-	return m.Store.Save(st)
+	if err := m.Store.Save(st); err != nil {
+		return 0, err
+	}
+	return ignoredDestroyed, nil
 }
 
 var (
diff --git a/internal/worktree/worktree_integration_test.go b/internal/worktree/worktree_integration_test.go
index a710b19..58f5e0e 100644
--- a/internal/worktree/worktree_integration_test.go
+++ b/internal/worktree/worktree_integration_test.go
@@ -1,9 +1,11 @@
 package worktree
 
 import (
+	"errors"
 	"os"
 	"os/exec"
 	"path/filepath"
+	"strings"
 	"testing"
 
 	"myworktree/internal/llm"
@@ -43,7 +45,7 @@ func TestCreateDeleteWorktreeIntegration(t *testing.T) {
 		t.Fatalf("created worktree path should exist: %v", err)
 	}
 
-	if err := m.Delete(wt.ID); err != nil {
+	if _, err := m.Delete(wt.ID, false); err != nil {
 		t.Fatalf("Delete failed: %v", err)
 	}
 	st, err := fs.Load()
@@ -80,7 +82,7 @@ func TestDeleteMissingWorktreeRemovesState(t *testing.T) {
 		t.Fatalf("worktree path should be gone, got err=%v", err)
 	}
 
-	if err := m.Delete(wt.ID); err != nil {
+	if _, err := m.Delete(wt.ID, false); err != nil {
 		t.Fatalf("Delete failed for missing worktree: %v", err)
 	}
 
@@ -117,6 +119,117 @@ func initGitRepo(t *testing.T) string {
 	return repo
 }
 
+// makeWorktreeDirty reproduces the issue #102 shape inside a managed
+// worktree: a tracked file deleted, an untracked scratch file, and a
+// gitignored file (which the blocking dirty check must NOT count, but the
+// refusal must WARN about — force would destroy it unrecoverably).
+func makeWorktreeDirty(t *testing.T, wtPath string) {
+	t.Helper()
+	if err := os.Remove(filepath.Join(wtPath, "README.md")); err != nil {
+		t.Fatalf("remove tracked file failed: %v", err)
+	}
+	if err := os.WriteFile(filepath.Join(wtPath, "scratch.txt"), []byte("scratch\n"), 0o600); err != nil {
+		t.Fatalf("write untracked file failed: %v", err)
+	}
+	if err := os.WriteFile(filepath.Join(wtPath, ".gitignore"), []byte("*.log\n"), 0o600); err != nil {
+		t.Fatalf("write .gitignore failed: %v", err)
+	}
+	if err := os.WriteFile(filepath.Join(wtPath, "debug.log"), []byte("log\n"), 0o600); err != nil {
+		t.Fatalf("write ignored file failed: %v", err)
+	}
+}
+
+func TestDeleteDirtyRefusalCarriesDetails(t *testing.T) {
+	if _, err := exec.LookPath("git"); err != nil {
+		t.Skip("git is required")
+	}
+	repo := initGitRepo(t)
+	dataDir := t.TempDir()
+	fs := store.FileStore{Path: filepath.Join(dataDir, "state.json")}
+	m := Manager{
+		GitRoot: repo,
+		DataDir: dataDir,
+		Store:   fs,
+	}
+
+	wt, err := m.Create("dirty work", "HEAD")
+	if err != nil {
+		t.Fatalf("Create failed: %v", err)
+	}
+	makeWorktreeDirty(t, wt.Path)
+
+	_, err = m.Delete(wt.ID, false)
+	var dirty *DirtyWorktreeError
+	if !errors.As(err, &dirty) {
+		t.Fatalf("Delete should refuse with DirtyWorktreeError, got %v", err)
+	}
+	// " D README.md" is the one deletion; "?? .gitignore" + "?? scratch.txt"
+	// are the two untracked entries; debug.log is ignored and must NOT
+	// block — only surface as the force-risk count.
+	if dirty.Entries != 3 || dirty.Deleted != 1 || dirty.Modified != 0 || dirty.Untracked != 2 {
+		t.Fatalf("dirty breakdown = entries:%d deleted:%d modified:%d untracked:%d, want 3/1/0/2",
+			dirty.Entries, dirty.Deleted, dirty.Modified, dirty.Untracked)
+	}
+	if dirty.Ignored != 1 {
+		t.Fatalf("Ignored = %d, want 1 (debug.log) — the gitignored file must be NAMED as a force risk even though it does not block", dirty.Ignored)
+	}
+	if len(dirty.FirstPaths) == 0 || dirty.Porcelain == "" {
+		t.Fatalf("refusal must carry first paths and the full porcelain output, got %#v", dirty)
+	}
+	msg := err.Error()
+	for _, want := range []string{"3 entries", "1 deleted", "2 untracked", "1 gitignored entry", "retry with force"} {
+		if !strings.Contains(msg, want) {
+			t.Fatalf("refusal message missing %q:\n%s", want, msg)
+		}
+	}
+
+	// The refusal changed nothing: state keeps the record, the path lives.
+	st, err := fs.Load()
+	if err != nil {
+		t.Fatalf("load state failed: %v", err)
+	}
+	if len(st.Worktrees) != 1 {
+		t.Fatalf("refused delete must keep the state record, got %#v", st.Worktrees)
+	}
+	if _, err := os.Stat(wt.Path); err != nil {
+		t.Fatalf("refused delete must keep the path: %v", err)
+	}
+}
+
+func TestDeleteForceRemovesDirtyWorktree(t *testing.T) {
+	if _, err := exec.LookPath("git"); err != nil {
+		t.Skip("git is required")
+	}
+	repo := initGitRepo(t)
+	dataDir := t.TempDir()
+	fs := store.FileStore{Path: filepath.Join(dataDir, "state.json")}
+	m := Manager{
+		GitRoot: repo,
+		DataDir: dataDir,
+		Store:   fs,
+	}
+
+	wt, err := m.Create("force me", "HEAD")
+	if err != nil {
+		t.Fatalf("Create failed: %v", err)
+	}
+	makeWorktreeDirty(t, wt.Path)
+
+	if _, err := m.Delete(wt.ID, true); err != nil {
+		t.Fatalf("force Delete failed: %v", err)
+	}
+	if _, err := os.Stat(wt.Path); !os.IsNotExist(err) {
+		t.Fatalf("force delete should remove the path, got err=%v", err)
+	}
+	st, err := fs.Load()
+	if err != nil {
+		t.Fatalf("load state failed: %v", err)
+	}
+	if len(st.Worktrees) != 0 {
+		t.Fatalf("force delete should drop the state record, got %#v", st.Worktrees)
+	}
+}
+
 func runGit(t *testing.T, dir string, args ...string) {
 	t.Helper()
 	cmd := exec.Command("git", args...)
@@ -126,3 +239,196 @@ func runGit(t *testing.T, dir string, args ...string) {
 		t.Fatalf("git %v failed: %v (%s)", args, err, string(out))
 	}
 }
+
+// worktreeGitDir resolves a managed worktree's real git dir from the
+// "gitdir:" pointer file (a linked worktree's .git is a file, not a dir).
+func worktreeGitDir(t *testing.T, wtPath string) string {
+	t.Helper()
+	raw, err := os.ReadFile(filepath.Join(wtPath, ".git"))
+	if err != nil {
+		t.Fatalf("read .git pointer failed: %v", err)
+	}
+	line := strings.TrimSpace(string(raw))
+	if !strings.HasPrefix(line, "gitdir: ") {
+		t.Fatalf("unexpected .git pointer content: %q", line)
+	}
+	return strings.TrimPrefix(line, "gitdir: ")
+}
+
+// TestDeleteForceSkipsStatusProbe pins the issue #102 review fix: force must
+// NOT gate on `git status` — on a damaged worktree (corrupt index) the probe
+// fails while `git worktree remove --force` still succeeds, and force is the
+// escape hatch for exactly that workspace.
+func TestDeleteForceSkipsStatusProbe(t *testing.T) {
+	if _, err := exec.LookPath("git"); err != nil {
+		t.Skip("git is required")
+	}
+	repo := initGitRepo(t)
+	dataDir := t.TempDir()
+	fs := store.FileStore{Path: filepath.Join(dataDir, "state.json")}
+	m := Manager{
+		GitRoot: repo,
+		DataDir: dataDir,
+		Store:   fs,
+	}
+
+	wt, err := m.Create("damaged", "HEAD")
+	if err != nil {
+		t.Fatalf("Create failed: %v", err)
+	}
+	if err := os.WriteFile(filepath.Join(worktreeGitDir(t, wt.Path), "index"), []byte("garbage"), 0o600); err != nil {
+		t.Fatalf("corrupt index failed: %v", err)
+	}
+	// Sanity: the probe really is broken now.
+	if out, statErr := exec.Command("git", "-C", wt.Path, "status", "--porcelain").CombinedOutput(); statErr == nil {
+		t.Fatalf("precondition failed: git status should fail on the corrupt index, got %s", out)
+	}
+
+	if _, err := m.Delete(wt.ID, true); err != nil {
+		t.Fatalf("force Delete must not gate on git status (corrupt index): %v", err)
+	}
+	if _, err := os.Stat(wt.Path); !os.IsNotExist(err) {
+		t.Fatalf("force delete should remove the path, got err=%v", err)
+	}
+	// The non-force path must still surface the probe failure.
+	wt2, err := m.Create("damaged again", "HEAD")
+	if err != nil {
+		t.Fatalf("Create failed: %v", err)
+	}
+	if err := os.WriteFile(filepath.Join(worktreeGitDir(t, wt2.Path), "index"), []byte("garbage"), 0o600); err != nil {
+		t.Fatalf("corrupt index failed: %v", err)
+	}
+	if _, err := m.Delete(wt2.ID, false); err == nil {
+		t.Fatal("non-force Delete should fail when the status probe fails")
+	}
+	_ = os.RemoveAll(wt2.Path)
+}
+
+// TestDeleteUntrackedDirCountsFilesIndividually pins the -uall probe (issue
+// #102 review): the default porcelain output collapses an untracked
+// DIRECTORY to one "?? dir/" line, so a dependency dir with a thousand
+// files would read as "1 untracked" in the refusal.
+func TestDeleteUntrackedDirCountsFilesIndividually(t *testing.T) {
+	if _, err := exec.LookPath("git"); err != nil {
+		t.Skip("git is required")
+	}
+	repo := initGitRepo(t)
+	dataDir := t.TempDir()
+	fs := store.FileStore{Path: filepath.Join(dataDir, "state.json")}
+	m := Manager{
+		GitRoot: repo,
+		DataDir: dataDir,
+		Store:   fs,
+	}
+
+	wt, err := m.Create("untracked dir", "HEAD")
+	if err != nil {
+		t.Fatalf("Create failed: %v", err)
+	}
+	dir := filepath.Join(wt.Path, "vendorlibs")
+	if err := os.MkdirAll(dir, 0o755); err != nil {
+		t.Fatalf("mkdir failed: %v", err)
+	}
+	for _, name := range []string{"a.so", "b.so", "c.so"} {
+		if err := os.WriteFile(filepath.Join(dir, name), []byte("x\n"), 0o600); err != nil {
+			t.Fatalf("write failed: %v", err)
+		}
+	}
+
+	_, err = m.Delete(wt.ID, false)
+	var dirty *DirtyWorktreeError
+	if !errors.As(err, &dirty) {
+		t.Fatalf("Delete should refuse with DirtyWorktreeError, got %v", err)
+	}
+	if dirty.Untracked != 3 {
+		t.Fatalf("Untracked = %d, want 3 — the untracked dir must count file-by-file (-uall), not collapse to one entry", dirty.Untracked)
+	}
+}
+
+// TestDeleteRefusalKeepsNonASCIIPathsLiteral pins core.quotePath=false on the
+// probe (issue #102 review): git's default C-style quoting would show a
+// Chinese filename as octal escapes in the refusal and the dialog.
+func TestDeleteRefusalKeepsNonASCIIPathsLiteral(t *testing.T) {
+	if _, err := exec.LookPath("git"); err != nil {
+		t.Skip("git is required")
+	}
+	repo := initGitRepo(t)
+	dataDir := t.TempDir()
+	fs := store.FileStore{Path: filepath.Join(dataDir, "state.json")}
+	m := Manager{
+		GitRoot: repo,
+		DataDir: dataDir,
+		Store:   fs,
+	}
+
+	wt, err := m.Create("unicode", "HEAD")
+	if err != nil {
+		t.Fatalf("Create failed: %v", err)
+	}
+	if err := os.WriteFile(filepath.Join(wt.Path, "中文文件.txt"), []byte("x\n"), 0o600); err != nil {
+		t.Fatalf("write failed: %v", err)
+	}
+
+	_, err = m.Delete(wt.ID, false)
+	var dirty *DirtyWorktreeError
+	if !errors.As(err, &dirty) {
+		t.Fatalf("Delete should refuse with DirtyWorktreeError, got %v", err)
+	}
+	if !strings.Contains(dirty.Porcelain, "中文文件.txt") {
+		t.Fatalf("porcelain should carry the literal non-ASCII path, got:\n%s", dirty.Porcelain)
+	}
+	if len(dirty.FirstPaths) != 1 || dirty.FirstPaths[0] != "中文文件.txt" {
+		t.Fatalf("FirstPaths = %#v, want the literal non-ASCII path (no octal escapes)", dirty.FirstPaths)
+	}
+}
+
+// TestDeleteCleanWorktreeReportsIgnoredDestroyed pins the issue #102 review
+// follow-up: a worktree whose ONLY contents at risk are gitignored files is
+// deleted without a refusal (nothing blocks), but the caller must learn how
+// many gitignored entries vanished with the directory instead of it passing
+// silently. Ignored directories count file-by-file (-uall).
+func TestDeleteCleanWorktreeReportsIgnoredDestroyed(t *testing.T) {
+	if _, err := exec.LookPath("git"); err != nil {
+		t.Skip("git is required")
+	}
+	repo := initGitRepo(t)
+	dataDir := t.TempDir()
+	fs := store.FileStore{Path: filepath.Join(dataDir, "state.json")}
+	m := Manager{
+		GitRoot: repo,
+		DataDir: dataDir,
+		Store:   fs,
+	}
+
+	wt, err := m.Create("ignored only", "HEAD")
+	if err != nil {
+		t.Fatalf("Create failed: %v", err)
+	}
+	if err := os.WriteFile(filepath.Join(wt.Path, ".gitignore"), []byte("*.log\nbuildcache/\n"), 0o600); err != nil {
+		t.Fatalf("write .gitignore failed: %v", err)
+	}
+	// Commit .gitignore on the worktree's branch so the only remaining
+	// status entries are the ignored ones.
+	runGit(t, wt.Path, "add", ".gitignore")
+	runGit(t, wt.Path, "commit", "-m", "add gitignore")
+	if err := os.WriteFile(filepath.Join(wt.Path, "debug.log"), []byte("log\n"), 0o600); err != nil {
+		t.Fatalf("write failed: %v", err)
+	}
+	cacheDir := filepath.Join(wt.Path, "buildcache")
+	if err := os.MkdirAll(cacheDir, 0o755); err != nil {
+		t.Fatalf("mkdir failed: %v", err)
+	}
+	for _, name := range []string{"x.o", "y.o"} {
+		if err := os.WriteFile(filepath.Join(cacheDir, name), []byte("o\n"), 0o600); err != nil {
+			t.Fatalf("write failed: %v", err)
+		}
+	}
+
+	ignored, err := m.Delete(wt.ID, false)
+	if err != nil {
+		t.Fatalf("clean Delete failed: %v", err)
+	}
+	if ignored != 3 {
+		t.Fatalf("ignoredDestroyed = %d, want 3 (debug.log + buildcache/x.o + buildcache/y.o — the ignored dir must count file-by-file)", ignored)
+	}
+}
diff --git a/internal/worktree/worktree_test.go b/internal/worktree/worktree_test.go
index a162c47..a1ff9a1 100644
--- a/internal/worktree/worktree_test.go
+++ b/internal/worktree/worktree_test.go
@@ -43,3 +43,114 @@ func TestSlugify(t *testing.T) {
 		t.Fatalf("slug should be truncated to 48 chars, got len=%d", len(got))
 	}
 }
+
+// TestDirtyWorktreeErrorSummarizePorcelain pins the classification of the
+// issue #102 refusal summary: every porcelain line lands in EXACTLY ONE
+// bucket (priority delete > rename > add > modify), so the buckets sum to
+// Entries; "!!" lines never appear here in production (the blocking check
+// runs without --ignored) and are skipped defensively.
+func TestDirtyWorktreeErrorSummarizePorcelain(t *testing.T) {
+	out := strings.Join([]string{
+		" D firmware/CMakeLists.txt", // worktree delete
+		"D  tools/gone.txt",          // staged delete
+		" M docs/changed.md",         // worktree modify
+		"M  staged.txt",              // staged modify
+		"MM both.txt",                // both columns — still ONE bucket
+		"T  typechange.txt",          // typechange counts as modify
+		"A  added.txt",               // staged add
+		"R  old.txt -> new.txt",      // rename (path field kept raw)
+		"C  src.txt -> copy.txt",     // copy counts as rename
+		"?? scratch.log",             // untracked
+		"!! debug.log",               // defensive: ignored lines are skipped
+	}, "\n")
+
+	d := &DirtyWorktreeError{}
+	d.summarizePorcelain(out)
+
+	if d.Entries != 10 {
+		t.Fatalf("Entries = %d, want 10 (the !! line is skipped)", d.Entries)
+	}
+	if d.Deleted != 2 || d.Modified != 4 || d.Added != 1 || d.Renamed != 2 || d.Untracked != 1 {
+		t.Fatalf("buckets = %dD/%dM/%dA/%dR/%dU, want 2D/4M/1A/2R/1U — and they must sum to Entries",
+			d.Deleted, d.Modified, d.Added, d.Renamed, d.Untracked)
+	}
+	if got := d.Deleted + d.Modified + d.Added + d.Renamed + d.Untracked; got != d.Entries {
+		t.Fatalf("buckets sum to %d, want Entries=%d", got, d.Entries)
+	}
+	if len(d.FirstPaths) != maxDirtyFirstPaths {
+		t.Fatalf("FirstPaths = %d entries, want the %d-entry cap", len(d.FirstPaths), maxDirtyFirstPaths)
+	}
+	if d.FirstPaths[0] != "firmware/CMakeLists.txt" {
+		t.Fatalf("FirstPaths[0] = %q, want the first path field without its status prefix", d.FirstPaths[0])
+	}
+}
+
+// TestCountIgnoredPorcelain pins the "!!" count used for the gitignored
+// risk warning in the refusal message (issue #102 P3).
+func TestCountIgnoredPorcelain(t *testing.T) {
+	out := " M tracked.txt\n?? scratch.txt\n!! build/\n!! debug.log\n!! nv_backup.bin\n"
+	if n := countIgnoredPorcelain(out); n != 3 {
+		t.Fatalf("countIgnoredPorcelain = %d, want 3", n)
+	}
+	if n := countIgnoredPorcelain(""); n != 0 {
+		t.Fatalf("countIgnoredPorcelain(\"\") = %d, want 0", n)
+	}
+}
+
+// TestDirtyWorktreeErrorMessage pins the refusal wording shape (issue #102
+// P1/P3): counts by category, first paths, the gitignored-file warning,
+// and the force exit — the flat "delete is refused" gave none of these.
+func TestDirtyWorktreeErrorMessage(t *testing.T) {
+	d := &DirtyWorktreeError{
+		Entries: 133, Deleted: 133,
+		FirstPaths: []string{"firmware/CMakeLists.txt", "firmware/main.c"},
+		Ignored:    89,
+	}
+	msg := d.Error()
+	for _, want := range []string{
+		"133 entries", "(133 deleted, 0 modified, 0 untracked)",
+		"first paths: firmware/CMakeLists.txt, firmware/main.c",
+		"89 gitignored entries", "force delete would destroy unrecoverably",
+		"retry with force",
+	} {
+		if !strings.Contains(msg, want) {
+			t.Fatalf("refusal message missing %q:\n%s", want, msg)
+		}
+	}
+
+	// Singular forms and the optional buckets.
+	d = &DirtyWorktreeError{Entries: 1, Modified: 1, Added: 2, Renamed: 1, Ignored: 1}
+	msg = d.Error()
+	for _, want := range []string{"1 entry", ", 2 added", ", 1 renamed", "1 gitignored entry present"} {
+		if !strings.Contains(msg, want) {
+			t.Fatalf("refusal message missing %q:\n%s", want, msg)
+		}
+	}
+}
+
+func TestCapDirtyPorcelain(t *testing.T) {
+	make := func(n int) string {
+		lines := make([]string, n)
+		for i := range lines {
+			lines[i] = "?? f"
+		}
+		return strings.Join(lines, "\n")
+	}
+
+	// At or under the cap the output is carried verbatim.
+	if got := capDirtyPorcelain(make(maxDirtyPorcelainLines)); got != make(maxDirtyPorcelainLines) {
+		t.Fatalf("output at the cap must pass through unchanged, got %d lines", strings.Count(got, "\n")+1)
+	}
+	// One line over: singular "line", and the carried body stays capped.
+	got := capDirtyPorcelain(make(maxDirtyPorcelainLines + 1))
+	if !strings.HasSuffix(got, "… (1 more line truncated)") {
+		t.Fatalf("1-over truncation marker should be singular, got suffix %q", got[len(got)-40:])
+	}
+	if strings.Count(got, "?? f") != maxDirtyPorcelainLines {
+		t.Fatalf("carried output must be capped at %d lines", maxDirtyPorcelainLines)
+	}
+	// Two over: plural.
+	if got := capDirtyPorcelain(make(maxDirtyPorcelainLines + 2)); !strings.HasSuffix(got, "… (2 more lines truncated)") {
+		t.Fatalf("2-over truncation marker should be plural, got suffix %q", got[len(got)-40:])
+	}
+}
diff --git a/scripts/install.sh b/scripts/install.sh
index 11e6460..074bc36 100755
--- a/scripts/install.sh
+++ b/scripts/install.sh
@@ -3,7 +3,7 @@
 #
 # Usage:
 #   curl -fsSL https://raw.githubusercontent.com/linletian/myworktree/main/scripts/install.sh | bash
-#   curl -fsSL .../install.sh | bash -s -- -v v0.5.1
+#   curl -fsSL .../install.sh | bash -s -- -v v0.5.2
 #   curl -fsSL .../install.sh | bash -s -- -v vX.Y.Z-beta.N   # pin a beta
 #   curl -fsSL .../install.sh | bash -s -- --beta             # latest beta
 #   curl -fsSL .../install.sh | bash -s -- --no-modify-path
@@ -50,7 +50,7 @@ Usage: install.sh [options]
 
 Options:
   -h, --help              show this help
-  -v, --version      install a specific version (e.g., v0.5.1, 0.5.0,
+  -v, --version      install a specific version (e.g., v0.5.2, 0.5.1,
                           or a beta like vX.Y.Z-beta.N)
       --beta              install the latest beta (prerelease) instead of
                           the latest stable release
@@ -65,7 +65,7 @@ Env vars:
 
 Examples:
   curl -fsSL https://raw.githubusercontent.com/${REPO}/main/scripts/install.sh | bash
-  curl -fsSL .../install.sh | bash -s -- -v v0.5.1
+  curl -fsSL .../install.sh | bash -s -- -v v0.5.2
   INSTALL_ALIAS=mwt bash install.sh     # avoid conflict with the Debian/Ubuntu 'mw' package
   INSTALL_DIR=~/bin bash install.sh     # install into a custom dir
 EOF