diff --git a/docs/plans/2026-09-26-accept-codex-0157.md b/docs/plans/2026-09-26-accept-codex-0157.md new file mode 100644 index 000000000..6b4bf14c0 --- /dev/null +++ b/docs/plans/2026-09-26-accept-codex-0157.md @@ -0,0 +1,30 @@ +# Accept Codex 0.157.1 (#234, codex-headless#16), with its known gaps filed + +Size: short. This is a compatibility review; the only code change is the accepted version. + +## Evidence (verified 2026-09-26) +- **Release notes, 0.149.1 → 0.157.1** (19 releases), scanned for rollouts, session/resume, the TUI composer and footer, approvals/trust, the Responses stream, proxy and config: + - 0.157.0 #47096 keeps TUI hints "immediately above the composer". The 0.157.1 PTY recording (`codex-headless testing/fixtures/composer-0157/tall-draft-ctrlc.json`) still shows the idle `? for shortcuts` hint on the row under the status row, which is what the composer classifier reads. + - 0.157.0 #47113 persists thread-creator identity in rollouts, and 0.153 #41912 persists token usage. Both are checked in the corpus below. + - 0.152/0.153 add rollout compression for shared lineages. None of the 1,850 files on disk is compressed. +- **Rollout corpus** (`~/.codex/sessions`, read line by line). The counts are a 2026-09-26 snapshot; the corpus grows while agents run (the reviewers counted 2,396–2,407 files, 219–233 of them 0.157.x). Record and payload types per minor version: + - The only top-level type in 0.150–0.157 not seen in 0.144–0.149 is `token_usage_record` (from 0.153). It is opaque to the parser's classifier: agent-transcript-parser#38. + - 0.157 writes no `event_msg:user_message`/`agent_message`. The prompt is a role-user `response_item` plus `event_msg:item_completed` (`UserMessage`). Fresh-rollout ownership (`codex-headless FreshRolloutClaim`) matches every durable user observation, including `response_item`, so it still finds the prompt. +- **Known gaps found in review.** Each was filed; all but #1289 are confirmed inherited from the already-accepted 0.149.1. **Status on 2026-09-27:** #1362, #1363 and #1289 are closed (#1363 by #1407). The two surviving prompt-extraction mutations from round-2 review b (the transcript reader's role-user `response_item`, and `foldCodexRecord`'s `response_item` branch) are both KILLED on origin/main `6dd23a49`. Disabling either fails a test there (16 transcript-reader tests; "lists prompts for a sampled rollout newest first"), so no new issue is needed: + - codex-headless#59: `listCodexSessions` summaries show the injected AGENTS/environment block (Agent Code's picker reads Codex's own index and is unaffected); + - codex-headless#62: the semantic tool lifecycle ignores `item_completed` (`CommandExecution`/`McpToolCall`), the only tool form since 0.149; + - #1362: `read_agent_transcript` drops `custom_tool_call` exec commands; + - #1363: the conversation source's no-index fallback reads only `event_msg:user_message`, so 0.157 prompts are lost when `state_N.sqlite` is unusable; + - #1289 (existing): 0.157 `compacted` payloads have no `type` and never render a compaction boundary; + - codex-headless#60: the recorded rollout-ownership tests run past 5 s under load. +- **Screen:** 0.157.0 and 0.157.1 raw PTY recordings pin the composer classifier: idle, a draft and Ctrl+C in 0.157.0; a 20-line draft in 0.157.1 (`tall-draft-ctrlc.json`, from codex-headless #57). This PR does not move the app's codex-headless pointer, but the pointer it already has (`d42cc1da`) contains both files, and `codexSession.nativeComposer.test.ts` replays the tall recording (round-2 reviews a and b). + - 0.157.0 #47178 **enabled the fullscreen transcript by default**. The 0.157.1 recording uses the alternate screen (`\x1b[?1049h`), so the classifier is pinned in that mode, and Agent Code passes no override. + - Trust and approval overlays have no 0.157 recording. They are unverified in the new default mode: a stated residual. +- **Runtime:** Agent Code has driven codex-cli 0.157.1 panes daily since 2026-09-25 (the #1319/#1327 work used them); the proxy recordings in `~/.config/agent-code/proxy` come from 0.157.1 sessions. + +## Change +- `support/upstream-versions.json`: Codex `accepted` 0.130.0 → 0.157.1, with `checkedAt` 2026-09-26. +- The same in codex-headless (0.149.1 → 0.157.1), with notes pointing at this evidence. + +## Out of scope +Cataloguing `token_usage_record` (agent-transcript-parser#38). diff --git a/docs/plans/2026-09-27-lane-port-probe-settle.md b/docs/plans/2026-09-27-lane-port-probe-settle.md new file mode 100644 index 000000000..8013aa372 --- /dev/null +++ b/docs/plans/2026-09-27-lane-port-probe-settle.md @@ -0,0 +1,142 @@ +# Lane port watcher: stop probing short-lived test servers (#1409) + +## Problem + +`LanePortWatcher` probes, with one `GET /`, every TCP listener it finds in a +watched lane's process tree. It does this on the FIRST scan that sees the +listener (`LanePortWatcher.runScan` → `probeOnce` → `lanePortsIo.probe`). +Agents run test suites inside their lanes, so test suites' loopback servers +get an unsolicited request. Tests that count requests flake, only on developer +machines: CI has no watching app. #1187/#1406 is the confirmed case. The +runtime harness saw `GET /` with `user-agent: node` about 1.6 s after its +server started. #1406 excused "any `GET /`" in that one harness, and the +comment it left admits the residual: a runtime escape that requests exactly +`GET /` is excused too. + +## Evidence this plan rests on + +- **Every exposed listener in #1409's inventory binds port 0:** + - `serviceLanListener.test.ts:40` + - `proxy-harness.mts:136` + - `netFetch.test.ts:25,165` + - `netPolicy.test.ts:43` + - `playwrightActions.system.test.ts:18` + - `record-fixtures.mts:164` + - `runtimeHarness.ts:87` + + They all live for one test or one harness run. +- **The recorded machine** (`__fixtures__/*.agents-and-tmux-terminal.*`, + replayed through `attributePorts`) has four attributed listeners: + - 4173 and 5292: vite, fixed ports; + - 62678: the recorder's own page server, `listen(0)`; + - 62679: the tmux pane's Python server, which is also ephemeral. + +## Rejected directions + +- **Skip ephemeral ports (≥ 49152 on macOS).** This would hide the recorded + tmux dev server (62679) and real tools that fall back to a random free port + (`serve` does, when 3000 is busy). It trades a test flake for a product + miss. +- **`HEAD` or another path.** It still reaches test handlers (#1409). +- **Read process command lines to spot test runners.** That undoes the + watcher's privacy rule: `ps` reads PID/PPID only. +- **Test-side excuses only.** They fix only the tests we know about. Every + future counting test inside a lane would rediscover the flake. + +## Change + +1. **Settle before probing (product).** The watcher records when it first saw + each `pid:port`. It probes, and lists, a listener only once it has SEEN it + listening for `PROBE_SETTLE_MS` (5 s), measured from the lsof answer that + first reported it. That is conservative: a server that was already up + before the watcher saw it still waits the full window. The first-seen entry is dropped when + the listener disappears, just as the probe cache already is. + - A test server that lives for less than the window is never contacted. + - A dev server's chip appears one or two scans later: about 6–9 s after + start at the 3 s scan floor, instead of about 0–3 s. + - Side benefit: short-lived test servers no longer flash chips in the lane + header. + - **UNCONFIRMED product call:** 5 s, and hiding a listener until it + settles. The alternative would be listing it unprobed as `other` at + once. That brings back the chip flicker, so I rejected it. + - Scheduling: while any listener is unsettled, the next scan runs at + `max(SCAN_FLOOR_MS, time until the earliest one settles)` instead of the + full back-off. Without that, a slow machine's 20 × back-off could delay + a chip for a minute. +2. **The probe identifies itself (product plus tests).** + - `lanePortsIo.probe` sends `User-Agent: AgentCode-LanePortProbe/1`, from + one exported constant (`LANE_PORT_PROBE_USER_AGENT`). + - A long-lived counting test that outlives the window can then excuse + exactly the probe, not "any `GET /`" or "any node fetch". + - A developer who sees the request in their server log can tell what sent + it. +3. **Tests that count requests (test side).** Each excuses exactly the probe's + User-Agent: + - `runtimeHarness.ts` is DEFERRED to a follow-up once #1436 merges. #1436 + also edits that file, and #1406's `GET /` excuse already keeps the + harness green. The follow-up narrows the excuse to `GET /` AND the probe + UA, closing #1406's documented residual. + - `serviceLanListener.test.ts` is NOT changed (decided during the fix). + Each of its servers lives for one test, far under the window. The LAN + listener also forwards only an allow-list of headers + (`LAN_FORWARDED_HEADERS`, no `user-agent`), and that is correct product + behavior, so a forwarded probe could not be told apart upstream anyway. + The settle window is its fix. + - `scripts/proxy-harness.mts` (`requestCount`). + + The latent listeners (netFetch/netPolicy, LanTransport/Cloudflared) count + nothing and stay unchanged. + +## Test plan (fail-first from recorded data) + +- New `LanePortWatcher` tests use the recorded listeners. + - The recorder's `listen(0)` page server (62678) appears in one scan and is + gone by the next. It must never be probed. On main it is probed on the + first scan, so this test fails there. + - A recorded dev server (4173) is not probed before the window and is + probed and listed after it. + - While a listener is unsettled, the next scan is scheduled for when it + settles, not after the back-off. +- Existing tests that expect a chip after one scan now advance the fake clock + past the window and scan again. Their assertions stay the same. +- A `lanePortsIo.probe` test against a real loopback server: the request + carries the probe UA. +- (Follow-up after #1436) runtimeHarness: an untagged `GET /` IS counted, + extending #1406's positive control. + +## Review round 1 (a, b): what changed and what was corrected + +- **Corrected claim (b):** the settle window is a MITIGATION, not a guarantee. + Test servers and dev servers both live for arbitrary lengths, and a stalled + run can hold a test server past 5 s. Counting tests therefore get + deterministic guards as well: + - `serviceLanListener.test.ts` DOES change, reversing the earlier decision + above. Its upstream ignores exactly `GET /`, and `send()` never uses `/`. + A probe forwarded through the LAN listener loses its User-Agent, so shape + is the only thing the upstream can check. A real-socket test replays that + sequence. + - `proxy-harness.mts` excuses `GET /` plus the UA, not the UA alone, so a + `POST /responses` carrying that UA is still counted and forwarded. +- **Window timing (a):** + - The age now starts when lsof returned the listener, not at scan start. + Otherwise a slow lsof, or a scan straddling a plan change, shortened the + window. + - An empty plan and `stop()` forget ages and probe answers. + - A failed lsof (timeout, signal, missing binary) throws instead of reading + as empty, so a settled chip is not pruned and hidden for another window. + - Known limit, documented at `PROBE_SETTLE_MS`: a close and rebind on the + same pid:port BETWEEN scans is invisible to sampling. +- **Escalated product alternative (b), OWNER/MANAGER DECISION:** stop + automatic HTTP probes entirely. Publish owned listening ports unverified, + and request a port only when the user clicks it or an agent opens it. That + gives zero unsolicited requests and immediate discovery. The costs: the + html/other classification the chip relies on (`LanePortChip` shows html + ports only), and non-page listeners shown as candidates, which the settle + window could still debounce. It is out of scope here: it changes what the + chip shows. + +## Decision (B6 q129, owner proxy) + +Accepted as a MITIGATION: the PR says `Refs #1409`, not `Fixes`. The settle +window's residual and the roughly 6–9 s chip latency are accepted. The passive, +probe-free discovery alternative is #1458. diff --git a/docs/plans/2026-09-27-opencode-launch-without-db-wait-bump.md b/docs/plans/2026-09-27-opencode-launch-without-db-wait-bump.md new file mode 100644 index 000000000..ff163cbba --- /dev/null +++ b/docs/plans/2026-09-27-opencode-launch-without-db-wait-bump.md @@ -0,0 +1,51 @@ +# App side of launching OpenCode without waiting on `opencode db path` (#1114) + +Short plan: a pointer bump plus the host contract the package needs. The design, evidence and review history are in the package: `packages/opencode-terminal-headless/docs/plans/2026-09-27-launch-without-db-path-wait.md` (opencode-terminal-headless#10, merged `7a009541`). + +## Outcome +An OpenCode pane's TUI spawns without waiting up to 20 s for the database-path lookup during a restore storm. The durable (committed-transcript) channel opens when the lookup lands. Nothing committed in the meantime is lost silently: it is proven absent or reported as a possible gap, which the renderer heals by re-reading history (#1117). + +## Change +- `packages/opencode-terminal-headless` goes to `7a009541`. No lockfile change: the app resolves the package through a path alias, not a `file:` dependency, and the package's own dependencies did not change. +- **`OpencodeTerminalSession`: the host contract (recheck2 a/b).** + 1. `tuiOutput` is latched in the PTY data subscription made right after spawn, and passed to the headless as `tuiOutputSeen`. + 2. Terminal input (`write`: keystrokes, pastes) is **held** until the TUI's first output, then written in order. Programmatic prompts go through the server and the package gates them. So nothing that can make OpenCode commit reaches the PTY before it paints, and the package's "no output yet, nothing committed" proof holds. Held input for a TUI that never paints is dropped on stop. +- **Renderer test harness:** `AdapterPty` drops the data subscription it duplicated (the package's `FakePty` has it now, and the two private fields conflicted), and its launch-shape docs are refreshed. + +## Tests +`opencodeTerminalSession.test.ts`, with the real headless recording its options. Each test fails on main's adapter: +- the latch is false until the first output, then true; +- input written before paint is held, then written in order, with later input passing through; +- held input is dropped when the pane stops. + +## Verification boundary +Unit and system tests with fake PTYs and the real package. The app is not launched. The real TUI's "paint before reading input" ordering is the package's stated assumption (recorded sessions support it). Holding input makes the host side of it true by construction. + +## Review round 1 and steering q97 +- **b and c (blocker, package):** after a ladder recovery the late report fired before a BUSY-deferred reader positioned, so the app's one heal could run too early and a turn was lost. Fixed in the package (opencode-terminal-headless#11: the report comes from `onPositioned`). This PR bumps to that merge. +- **b (major, app): the pre-paint hold was unbounded, and exit did not clear it.** + - **Bounded:** 256 chunks (the renderer's own pre-attach queue cap) and 64 KiB. Past either bound the NEW input is refused, so what was accepted stays in order. + - **Refused, never silently dropped:** `OpencodeTerminalSession.write` returns `false`, `AgentSession.write` may return `false`, and `SessionManager.write` reports it. `AgentTerminalLeaf` shows a coalesced pane toast ("That input didn't reach the agent…") when `sendInput` answers `false`. The same toast now covers the older refusals keystrokes were silently dropped for (no backend, a prompt delivery holding the composer). + - **Cleared on exit** as well as on stop. + - Tests (each red on the previous head): a 64 KiB paste refused while earlier input is kept; the 257th chunk refused; exit clears the hold; the manager reports a refusal; the leaf toasts once. + +## Steering q100: the pre-attach flush and neutral copy +- **The pre-attach flush ignored `sendInput` → false.** It is the pane's largest single write (up to 256 queued chunks), so it is the one most likely to exceed the bounded pre-paint hold, and it was dropped silently. + - It now goes through the same coalesced refusal reporter as the forwarder. +- **Ruling: a refusal is final, not retried.** A retry could land after newer keystrokes, out of order. The user is told which input failed: "What you typed while the terminal was attaching didn't reach the agent." +- **The forwarder's copy is neutral:** "That input didn't reach the agent." Main answers only a boolean (no backend, a delivery reservation, a full pre-paint hold), so naming any one cause would be false for the others. +- **Test:** a Submit queued before attach, with `sendInput` resolving false on flush, shows the flush message. Red with the flush reverted. + +## Review round 1, reviewer a (MERGE-READY, minors) +- **P3 (package):** the reader's `onError` gate release was unpinned. Pinned in opencode-terminal-headless#11. +- **A3–A5 (app):** the per-spawn latch reset, the stop-time clear and the `onData` generation guard were unpinned. `start()` after `stop()` is allowed, so each is reachable. + - Pinned by a restart test: a fresh hold, and the dead PTY's late paint is ignored. + - Pinned by a stop test: the hold is empty after stop. + - Each test fails under its mutation. +- **Stale doc:** the `deliverPromptText` doc claimed a PTY paste. It now says HTTP, held until the durable reader positions. +- **Unbounded hold:** already bounded (q97). +- **A6 (latch set after the flush):** left alone, as a says; the flush is synchronous. + +## Pointer bump to opencode-terminal-headless#11's merge (3935a3bb) +- The pointer moves from 7a009541 to 3935a3bb, the merge of #11, which contains 7a009541. That brings in round 1's blocker fix: the late database-path recovery is reported from `onPositioned`, so the app's one heal waits for the reader. +- The package's `package.json` and `package-lock.json` are unchanged between the two commits, so no lockfile resync is needed. diff --git a/docs/plans/2026-09-27-worktree-timeout-consumers.md b/docs/plans/2026-09-27-worktree-timeout-consumers.md new file mode 100644 index 000000000..d2d8be004 --- /dev/null +++ b/docs/plans/2026-09-27-worktree-timeout-consumers.md @@ -0,0 +1,180 @@ +# A timed-out worktree list is never read as "no family" (#1430) + +Size: short plan. It is a follow-up of #1429, which is merged; each +consumer's fix is small and bounded. + +## Outcome + +When `git worktree list` times out, none of the four remaining consumers +treats the empty answer as "this checkout has no siblings". Each either +says it or stays unknown, and none caches or records the wrong family. + +## Evidence (verified 2026-09-27 on origin/main after #1429, do not re-derive) + +- `src/main/ipc/git.ts`: `listWorktreesForCwd` returns `[]` on a timeout. + `listWorktreesForCwdDetailed` returns `{ worktrees, timedOut }` but is not + exported. Timed-out results are never cached (#1429). +- **Conversations:** `family.ts` `resolveFamily` falls back to the cwd + alone when the list is empty, so from a linked checkout the main + checkout's conversations drop out. `service.ts` `discover` caches that + family's discovery for `DISCOVERY_FRESH_MS` (3 s). The response + (`ConversationListResponse.family`) says nothing. +- **Worktree activity:** `ipc/worktreeActivity.ts` throws "not a git + worktree" on `[]` and answers `{ ok: false }`, which `loadWorktreeDump` + shows as "Agent activity: unavailable", the same as a non-repository. +- **Agent activity repo root:** `index.ts` `resolveRepoRoot` is + `listWorktreesForCwd(cwd).then(w => w[0]?.path ?? cwd)`. On a timeout it + records the cwd as the repo root in `AgentActivityRecorder` context, so + a worktree's activity is filed under the worktree, not its repository. +- **Renderer history:** `initialHistory.ts:301-303` and `history.ts:111-112` + map any non-ok `gitWorktrees` answer (including `timedOut`) to `[]`, and + feed it into `ingestWorktreeRawEvent`, which attributes against an empty + family. + +## Change + +- `git.ts`: export `listWorktreesForCwdDetailed`. +- **Conversations:** + - the `ListWorktrees` dependency returns `{ worktrees, timedOut }`; + - `RepositoryFamily.gitTimedOut: boolean`; + - a discovery whose family timed out is returned but NOT kept as + `this.discovery`, so the next request asks git again; + - `ConversationListResponse.family.gitTimedOut?: true`, and the picker + shows a muted line: "Git didn't answer in time. Conversations from this + repository's other worktrees may be missing." +- **Worktree activity:** `{ ok: false, timedOut: true }` on a timeout, and + the preload type matches. `loadWorktreeDump` gets `activityTimedOut`, and + the dump line reads "unavailable (Git timed out)". +- **Repo root:** `resolveRepoRootAfterGit(listDetailed, cwd)` lives in + `agentActivity/`. + - It retries once when the list timed out: the git queue is the usual + cause, and it drains. + - If it times out again it throws `RepoRootUnknown`. The recorder then + records the interval's repository as UNKNOWN (`''`, the store's + existing "no repository" value, which the summary labels Unknown) and + warns. The worktree row keeps its `cwd`, and the next interval asks git + again. + - Steering q126: the first version fell back to the cwd. The store + persisted it, and summarize grouped by it, so a worktree folder became + a repository of its own that no later interval could fold back. It + never healed. + - Ruling: one retry, never a loop. The recorder awaits this on every + interval open. +- **Renderer history:** a `gitWorktrees` answer with `timedOut` skips + worktree attribution for that chunk, and `workActivity`/`workContext` + stay as they were. "Unknown" stays unknown; the live reconciler fills it + in later. A non-repository (`ok: false` without `timedOut`) keeps + today's `[]`. + +## Tests (fail-first, with #1429's execFile timeout fake where main reads git) + +- `family`/`service`: a timed-out list gives `gitTimedOut` on the family + and response, and a second request re-asks git (it isn't cached). + Before: a cwd-only family, cached. +- `worktreeActivity` IPC: timeout → `{ ok: false, timedOut: true }`. + Before: `{ ok: false }`. +- `resolveRepoRootAfterGit`: + - timeout then success → the main checkout; + - two timeouts → throws; + - a success first → no retry. +- `AgentActivityRecorder` (steering q126), the real interval path and a real + store: two timeouts, then success. The first interval is under Unknown, + the second under the repository, and there is never a bucket keyed by the + worktree folder. Before: a `/dev/agent-code/.worktrees/fix` repository. +- `loadWorktreeDump` / `formatWorktreeDump`: the activity line says the + git timeout. +- `initialHistory`: a `timedOut` worktrees answer leaves `workActivity` + untouched. Before: it was ingested against `[]`. + +## Verification + +`npx tsc -b` and the scoped vitest runs. The app is not launched. + +## Out of scope + +`listWorktreesForCwd`'s other callers (MCP read paths) already go through +#1429's surfaces. + +## Review round 1 (a, b: FIX-BEFORE-MERGE), each fix fail-first + +- **a (major): pages from two families.** Page 1 was built while git timed + out (the cwd alone), and page 2 after git recovered (the whole + repository). Appending lost the rows the recovered order puts before the + cursor, and page 2's family cleared the warning. + - Fix: `useConversationList` compares the page's family (root, roots, + `gitTimedOut`) with what it appends to. On a change it discards the page + and reloads page 1. + - Pinned: a picker test drives ArrowDown paging across recovery. +- **a + b (major): skipped history lost its worktree evidence.** Skipping + attribution on a timeout meant the reconciler, which replays only what it + observed, never saw the chunk. A quiet session stayed on the launch folder + after git recovered. + - Fix: `WorkspaceRefs.worktreeReconcilerRef` publishes the live + reconciler. Both history loaders hand a timed-out chunk to it + (`handHistoryToReconciler`: `observe` plus `refresh`), and its bounded + window replays the chunk when a later refresh gets the catalog. Failed + probes are not cached, so the next refresh retries. + - Pinned: the real reconciler with the recorded `codex-0151` window + reaches `.../worktree-2` after git recovers, and a loader-level test + shows the timed-out chunk is handed over. Removing that call turns it + red. +- **b (minor): the note showed in Everywhere,** where the family removes no + rows. It now shows only in Repository scope. Pinned. +- **a (surviving mutation): the repository-unknown warning.** It is now + asserted. +- **c (MERGE-READY), minors:** + - The older-history loader's hand-off and its null guard are pinned by a + `loadOlderHistory` test with git timing out. Both of c's mutations are + now red. + - The dead `listWorktreesForCwd` export is removed. + - The body's counts are corrected. + - Residual, accepted: the production publish of `worktreeReconcilerRef` + in `useIpcSubscriptions` is unasserted, because mounting that hook is + heavy. Every loader and reconciler test injects the ref. + +## Verification pass (a, b: FIX-BEFORE-MERGE), each fix fail-first + +Both findings are gaps in the round-1 hand-off. + +- **a (major): a fresh cached catalog never repainted.** A live event had + already cached the catalog, and only the history's own `gitWorktrees` + call timed out. `refresh()` answered `cached` and never called + `onCatalogReady`, so the handed-over chunk sat in the window. + - Fix: `LiveWorktreeReconciler.replayCachedCatalog(cwd)` replays the + retained evidence against a real cached catalog. It does nothing for + the empty placeholder an in-flight probe writes. + `handHistoryToReconciler` calls it when `refresh` answers `cached`. + - Pinned: the recorded `codex-0151` window, with the catalog loaded + first, reaches `worktree-2`. Before the fix it stayed on the main + checkout. +- **b (major): an older page could replace a newer context.** The + reconciler appends what it observes as the newest evidence. + - Fix: the older-history loader hands a page over only while + `workContext` is unknown. This is the same recency rule as its + answered-git backfill. + - Pinned: a pane with a known context hands nothing over and keeps it. + - Ruling: initial history needs no such guard. It is the transcript's + tail, the newest evidence there is. + +## Manager verification (B6 at f5fc3585: FIX), fixed fail-first + +- **A scroll-up during a git timeout outranked the newest chunk.** The + sequence: the initial history (worktree-1) is handed over during a + timeout, then the user scrolls up while git still times out, and the + older page (worktree-2) is handed over too. It was appended as the + newest evidence, and once git recovered the pane landed on worktree-2. + - Fix: `observe(..., position)`. An older page enters at the OLD end of + the window (`retainOlder`), under the same 2 × limit bound. + - Overflow is the oldest evidence there is, so it is dropped rather than + folded when a catalog is cached. The folded baseline holds newer + records, and folding on top of them would make the page newest again. + - `handHistoryToReconciler(..., 'older')` is used by the older-history + loader. + - Pinned: the recorded `codex-0151` window as the older page, with the + same records moved to worktree-1 and 1 h later as the newest chunk, + and the recorded three-worktree catalog. It now lands on worktree-1. + - Mutations: appending instead of prepending, and the loader passing + newest, are each 1 red. +- The `resolveRepoRoot.ts` header now says Unknown, not the cwd. +- Filed separately by B6, out of scope here: a git failure that is not a + timeout is still read as "not a repository". diff --git a/packages/agent-transcript-parser b/packages/agent-transcript-parser index 68dbfff7e..7e8a67c7c 160000 --- a/packages/agent-transcript-parser +++ b/packages/agent-transcript-parser @@ -1 +1 @@ -Subproject commit 68dbfff7e11e833e843805dabf7a7942e7f75ca0 +Subproject commit 7e8a67c7cff453e23712a6f6aad8e53c8197754b diff --git a/packages/claude-code-headless b/packages/claude-code-headless index f52fc82d9..1cfa8c92b 160000 --- a/packages/claude-code-headless +++ b/packages/claude-code-headless @@ -1 +1 @@ -Subproject commit f52fc82d9d185314992cd845dafce42a723b2248 +Subproject commit 1cfa8c92b70e15c860e07e26d3db94a3e0c652e7 diff --git a/packages/opencode-terminal-headless b/packages/opencode-terminal-headless index 75536312c..3935a3bb4 160000 --- a/packages/opencode-terminal-headless +++ b/packages/opencode-terminal-headless @@ -1 +1 @@ -Subproject commit 75536312cb74920f4f9ea6bbb2e4df05b0c4a431 +Subproject commit 3935a3bb48b514896f0519872d1552a259fc9dea diff --git a/packages/workflow-mcp b/packages/workflow-mcp index ef995af00..513374d5f 160000 --- a/packages/workflow-mcp +++ b/packages/workflow-mcp @@ -1 +1 @@ -Subproject commit ef995af00b201e8ee535f3740c196f90074d2dd3 +Subproject commit 513374d5ff4514a402036086ade356915bbc36f3 diff --git a/scripts/proxy-harness.mts b/scripts/proxy-harness.mts index 960a42c8f..42bea9e67 100644 --- a/scripts/proxy-harness.mts +++ b/scripts/proxy-harness.mts @@ -21,6 +21,8 @@ import { mkdirSync, writeFileSync, createWriteStream, readFileSync, existsSync } import { join } from 'node:path' import { homedir } from 'node:os' +import { LANE_PORT_PROBE_USER_AGENT } from '../src/main/browserPocket/lanePortsIo.ts' + const prompt = process.argv[2] ?? 'say hi in three words' const ts = new Date().toISOString().replace(/[:.]/g, '-') @@ -50,6 +52,21 @@ console.error(`[harness] auth_mode=${authMode}, upstream=${upstreamBase}`) let requestCount = 0 const server = createServer(async (req: IncomingMessage, res: ServerResponse) => { + // The harness runs for as long as Codex does, usually well past the lane + // port watcher's settle window. When an agent runs it inside an Agent Code + // lane, this proxy is in the lane's process tree and receives the watcher's + // one `GET /` (#1409). Counting it made `runCodex()` report that Codex + // reached the proxy when only the watcher had. Only the probe's exact shape + // is excused: `GET /` AND its User-Agent. Anything else still counts, + // including a bare `GET /` and, above all, a `POST /responses` that somehow + // carried that User-Agent (#1452 review b), which must be counted and + // forwarded. + if (req.method === 'GET' && req.url === '/' && req.headers['user-agent'] === LANE_PORT_PROBE_USER_AGENT) { + res.statusCode = 404 + res.end() + console.error(`[harness] ignored the Agent Code lane port probe: ${req.method} ${req.url}`) + return + } const startedAt = Date.now() const reqId = ++requestCount console.error(`[proxy#${reqId}] ${req.method} ${req.url}`) diff --git a/src/main/agentActivity/AgentActivityRecorder.test.ts b/src/main/agentActivity/AgentActivityRecorder.test.ts index 3cd497f12..a4addb140 100644 --- a/src/main/agentActivity/AgentActivityRecorder.test.ts +++ b/src/main/agentActivity/AgentActivityRecorder.test.ts @@ -474,3 +474,60 @@ describe('AgentActivityRecorder', () => { expect(summary.totals.agentMs).toBe(10 * MINUTE) }) }) + +// #1430 / steering q126: git timing out twice made resolveRepoRoot throw, the +// recorder's catch answered the CWD, and the store persisted the worktree folder +// as the interval's repository. summarize groups by that stored key, so the +// worktree became a repository of its own and a later successful interval could +// never fold the earlier one back. An unresolved repository is recorded as +// UNKNOWN ('' — the store's existing "no repository" value, labelled Unknown), +// never as a false one; the cwd still names the worktree row. +describe('AgentActivityRecorder when git times out resolving the repository', () => { + it('files the interval under Unknown, never under the worktree folder, and later intervals under the repository', async () => { + const { resolveRepoRootAfterGit } = await import('@main/agentActivity/resolveRepoRoot.js') + let gitTimingOut = true + const manager = new EventEmitter() + const recorder = new AgentActivityRecorder({ + manager: manager as unknown as Pick, + store: new AgentActivityStore(dir), + // The REAL retry-once policy over a git lister that times out, then answers. + resolveRepoRoot: cwd => resolveRepoRootAfterGit(async () => gitTimingOut + ? { worktrees: [], timedOut: true } + : { worktrees: [{ path: '/dev/agent-code' }, { path: '/dev/agent-code/.worktrees/fix' }], timedOut: false }, cwd), + identityOf: () => undefined, + }) + recorders.push(recorder) + await recorder.start() + recorder.updateWorkspace(windows(), { 'name-1': 'Ada' }) + manager.emit('started', { sessionId: 'child', kind: 'codex' }) + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + try { + manager.emit('semantic-event', { sessionId: 'child', event: { type: 'stream_phase', phase: 'responding' } }) + vi.setSystemTime(T0 + HOUR) + manager.emit('semantic-event', { sessionId: 'child', event: { type: 'stream_phase', phase: 'idle' } }) + await vi.waitFor(async () => expect((await recorder.summary('24h')).totals.agentMs).toBe(HOUR)) + + gitTimingOut = false + vi.setSystemTime(T0 + 2 * HOUR) + manager.emit('semantic-event', { sessionId: 'child', event: { type: 'stream_phase', phase: 'responding' } }) + vi.setSystemTime(T0 + 3 * HOUR) + manager.emit('semantic-event', { sessionId: 'child', event: { type: 'stream_phase', phase: 'idle' } }) + vi.setSystemTime(T0 + 4 * HOUR) + await vi.waitFor(async () => expect((await recorder.summary('24h')).totals.agentMs).toBe(2 * HOUR)) + + // The unknown interval is said once in main's log, not silently filed + // (review a: the warning was not asserted). + expect(warn).toHaveBeenCalledWith(expect.stringContaining('repository unknown for this interval'), expect.anything()) + const [project] = (await recorder.summary('24h')).projects + const repositories = project!.repositories.map(r => [r.repoRoot, r.label, r.agentMs, r.worktrees.map(w => w.cwd)]) + expect(repositories).toEqual(expect.arrayContaining([ + ['', 'Unknown', HOUR, ['/dev/agent-code/.worktrees/fix']], + ['/dev/agent-code', 'agent-code', HOUR, ['/dev/agent-code/.worktrees/fix']], + ])) + // The false repository — the worktree folder as its own repository — never exists. + expect(project!.repositories.some(r => r.repoRoot === '/dev/agent-code/.worktrees/fix')).toBe(false) + } finally { + warn.mockRestore() + } + }) +}) diff --git a/src/main/agentActivity/AgentActivityRecorder.ts b/src/main/agentActivity/AgentActivityRecorder.ts index 97d614639..8c8fbc8ab 100644 --- a/src/main/agentActivity/AgentActivityRecorder.ts +++ b/src/main/agentActivity/AgentActivityRecorder.ts @@ -250,7 +250,18 @@ export class AgentActivityRecorder { provider: placement?.kind ?? entry.kind ?? 'unknown', tabId: placement?.tabId ?? null, tabTitle: placement?.tabTitle ?? null, - repoRoot: cwd ? await this.deps.resolveRepoRoot(cwd).catch(() => cwd) : '', + // #1430 / steering q126: a rejection means the repository is UNKNOWN (git + // timed out twice). It is recorded as '' — the store's existing "no + // repository" value, which the summary labels Unknown — and NEVER as the + // cwd. The cwd used to be persisted as this interval's repository: the + // store keeps it, summarize groups by it, so a worktree folder became a + // repository of its own that no later, correctly resolved interval could + // fold back. `cwd` below still names the worktree row, and the next + // interval asks git again. + repoRoot: cwd ? await this.deps.resolveRepoRoot(cwd).catch((error: unknown) => { + console.warn('[agent-activity] repository unknown for this interval (recorded as Unknown):', error instanceof Error ? error.message : error) + return '' + }) : '', cwd, } } diff --git a/src/main/agentActivity/resolveRepoRoot.test.ts b/src/main/agentActivity/resolveRepoRoot.test.ts new file mode 100644 index 000000000..a8dd846df --- /dev/null +++ b/src/main/agentActivity/resolveRepoRoot.test.ts @@ -0,0 +1,35 @@ +import { describe, expect, it, vi } from 'vitest' + +import { RepoRootUnknown, resolveRepoRootAfterGit } from './resolveRepoRoot' + +// #1430: a timed-out `git worktree list` used to file an agent's activity under +// its worktree folder instead of its repository (resolveRepoRoot read `[]` as +// "not a repository" and answered the cwd). +const MAIN = '/repo' +const WORKTREE = '/repo/.worktrees/feature' +const answered = { worktrees: [{ path: MAIN }, { path: WORKTREE }], timedOut: false } +const timeout = { worktrees: [], timedOut: true } + +describe('resolveRepoRootAfterGit', () => { + it('answers the main checkout when git answers', async () => { + const list = vi.fn(async () => answered) + expect(await resolveRepoRootAfterGit(list, WORKTREE)).toBe(MAIN) + expect(list).toHaveBeenCalledTimes(1) + }) + + it('retries a timeout once, and files under the repository when the retry answers', async () => { + const list = vi.fn().mockResolvedValueOnce(timeout).mockResolvedValueOnce(answered) + expect(await resolveRepoRootAfterGit(list, WORKTREE)).toBe(MAIN) + expect(list).toHaveBeenCalledTimes(2) + }) + + it('throws after two timeouts instead of answering the worktree folder', async () => { + const list = vi.fn(async () => timeout) + await expect(resolveRepoRootAfterGit(list, WORKTREE)).rejects.toBeInstanceOf(RepoRootUnknown) + expect(list).toHaveBeenCalledTimes(2) + }) + + it('keeps the folder for a real non-repository (git answered, no worktrees)', async () => { + expect(await resolveRepoRootAfterGit(async () => ({ worktrees: [], timedOut: false }), '/tmp/plain')).toBe('/tmp/plain') + }) +}) diff --git a/src/main/agentActivity/resolveRepoRoot.ts b/src/main/agentActivity/resolveRepoRoot.ts new file mode 100644 index 000000000..b6aed618b --- /dev/null +++ b/src/main/agentActivity/resolveRepoRoot.ts @@ -0,0 +1,36 @@ +/** + * The repository an agent's activity is filed under: the main checkout, i.e. + * the first entry of `git worktree list` (#1430). + * + * WHY a timeout is retried once and then THROWN rather than answered with the + * cwd: the recorder files each interval under the root this returns, so a + * timed-out (empty) list used to file a worktree's activity under the worktree + * itself — a silent, persisted mis-grouping. A timeout is almost always the + * git queue being busy (#1429's shared queue of five), which drains, so one + * retry usually answers. Two timeouts are "unknown", and unknown is thrown: + * the recorder records that one interval's repository as UNKNOWN (`''`, the + * store's "no repository" value) and warns, and the next interval asks again. + * Not the cwd (steering q126): the store persisted it, and a worktree folder + * became a repository of its own that no later interval could fold back. + * Never a loop — the recorder awaits this on every interval open. + * + * A non-repository (git answered, no worktrees) is not a timeout: the cwd is + * then the honest root, exactly as before. + */ +export class RepoRootUnknown extends Error { + constructor(cwd: string) { + super(`git worktree list timed out twice for ${cwd}`) + this.name = 'RepoRootUnknown' + } +} + +export async function resolveRepoRootAfterGit( + listDetailed: (cwd: string) => Promise<{ worktrees: ReadonlyArray<{ path: string }>; timedOut: boolean }>, + cwd: string, +): Promise { + for (let attempt = 0; attempt < 2; attempt += 1) { + const { worktrees, timedOut } = await listDetailed(cwd) + if (!timedOut) return worktrees[0]?.path ?? cwd + } + throw new RepoRootUnknown(cwd) +} diff --git a/src/main/browserPocket/LanePortWatcher.test.ts b/src/main/browserPocket/LanePortWatcher.test.ts index e22430d5c..592f1acd6 100644 --- a/src/main/browserPocket/LanePortWatcher.test.ts +++ b/src/main/browserPocket/LanePortWatcher.test.ts @@ -2,7 +2,7 @@ import { readFileSync } from 'node:fs' import { join } from 'node:path' import { describe, expect, it, vi } from 'vitest' -import { LanePortWatcher, SCAN_FLOOR_MS, type LanePortWatcherDeps } from './LanePortWatcher' +import { LanePortWatcher, PROBE_SETTLE_MS, SCAN_FLOOR_MS, type LanePortWatcherDeps } from './LanePortWatcher' import { parseLsofListen, parsePsTable, parseTmuxPanesAll } from './core/lanePorts' // Replays the Stage-1 recording of a live machine: two apps' agents, a @@ -38,14 +38,25 @@ function harness(agents: Record) { now: () => (t += 5), setTimer: (fn, ms) => { timers.push({ fn, ms }); return () => {} }, } - return { watcher: new LanePortWatcher(deps), timers, listListeners, listTmuxPanes, probe, broadcast } + // `advance` plays wall time passing between scans, which is what the settle + // window (#1409) measures; `now()` alone only ticks 5 ms per read. + const advance = (ms: number) => { t += ms } + // One scan to discover the listeners, then one once they have settled + // (#1409). The attribution tests below assert on what a settled scan shows. + const settledScan = async (watcher: LanePortWatcher) => { + await watcher.scan() + advance(PROBE_SETTLE_MS) + await watcher.scan() + } + const h = { watcher: new LanePortWatcher(deps), timers, listListeners, listTmuxPanes, probe, broadcast, advance } + return { ...h, settle: () => settledScan(h.watcher) } } describe('LanePortWatcher on the recorded machine', () => { it('reports each lane its own dev server and nothing of its neighbour\'s', async () => { const h = harness({ a: ancestorClaude(4173), b: ancestorClaude(5292) }) h.watcher.setSessions([{ sessionId: 'a', tmuxNames: [], terminalSessionIds: [] }, { sessionId: 'b', tmuxNames: [], terminalSessionIds: [] }]) - await h.watcher.scan() + await h.settle() const out = h.broadcast.mock.calls.at(-1)![0] const ports = (id: string) => (out[id] ?? []).map((p: { port: number }) => p.port) expect(ports('a')).toContain(4173) @@ -59,7 +70,7 @@ describe('LanePortWatcher on the recorded machine', () => { it('finds a tmux terminal\'s server through its pane, attributed to the lane that owns the terminal', async () => { const h = harness({ a: ancestorClaude(4173) }) h.watcher.setSessions([{ sessionId: 'a', tmuxNames: [recTmux[0]], terminalSessionIds: [] }]) - await h.watcher.scan() + await h.settle() const ports = h.broadcast.mock.calls.at(-1)![0].a.map((p: { port: number }) => p.port) expect(ports).toContain(listeners.find(l => l.pid === recTmux[1])!.port) expect(ports).toContain(4173) @@ -77,7 +88,7 @@ describe('LanePortWatcher on the recorded machine', () => { it('probes only owned listeners, and each pid:port once across scans', async () => { const h = harness({ a: ancestorClaude(4173) }) h.watcher.setSessions([{ sessionId: 'a', tmuxNames: [], terminalSessionIds: [] }]) - await h.watcher.scan() + await h.settle() await h.watcher.scan() expect(h.probe.mock.calls.map(c => c[0])).toEqual([4173]) }) @@ -110,9 +121,11 @@ describe('scheduling and cost', () => { it('unchanged results are not re-broadcast', async () => { const h = harness({ a: ancestorClaude(4173) }) h.watcher.setSessions([{ sessionId: 'a', tmuxNames: [], terminalSessionIds: [] }]) + // Unsettled scan broadcasts no chips; the settled one broadcasts 4173. + await h.settle() + expect(h.broadcast).toHaveBeenCalledTimes(2) await h.watcher.scan() - await h.watcher.scan() - expect(h.broadcast).toHaveBeenCalledTimes(1) + expect(h.broadcast).toHaveBeenCalledTimes(2) }) }) @@ -120,11 +133,16 @@ describe('review A #8 / surviving mutations', () => { it('a server that went away and came back on the same pid:port is probed again', async () => { const h = harness({ a: ancestorClaude(4173) }) h.watcher.setSessions([{ sessionId: 'a', tmuxNames: [], terminalSessionIds: [] }]) - await h.watcher.scan() + await h.settle() const listen = h.listListeners.getMockImplementation()! h.listListeners.mockImplementation(async () => []) await h.watcher.scan() h.listListeners.mockImplementation(listen) + // The returning server settles afresh: its predecessor's age must not + // carry over (it could be a test server reusing a freed port). + await h.watcher.scan() + expect(h.probe.mock.calls.map(c => c[0])).toEqual([4173]) + h.advance(PROBE_SETTLE_MS) await h.watcher.scan() expect(h.probe.mock.calls.map(c => c[0])).toEqual([4173, 4173]) }) @@ -162,3 +180,226 @@ describe('review A #8 / surviving mutations', () => { expect(broadcast.mock.calls.map(c => c[0])).toEqual([{}]) }) }) + +// #1409: agents run test suites inside their lanes, and those suites' loopback +// servers (every one in the issue's inventory binds port 0 and lives for one +// test) received the watcher's unsolicited `GET /`. The recorder's own page +// server in the recording is exactly such a listener: `listen(0)` on 62678, +// under claude 81647. +describe('#1409: short-lived listeners are never contacted', () => { + const RECORDER_PAGE_SERVER = 62678 + + it('a loopback server that is gone by the next scan is never probed nor listed', async () => { + const h = harness({ a: ancestorClaude(RECORDER_PAGE_SERVER) }) + h.watcher.setSessions([{ sessionId: 'a', tmuxNames: [], terminalSessionIds: [] }]) + await h.watcher.scan() + h.listListeners.mockImplementation(async () => []) + h.advance(PROBE_SETTLE_MS) + await h.watcher.scan() + expect(h.probe).not.toHaveBeenCalled() + for (const [bySession] of h.broadcast.mock.calls) expect(bySession.a ?? []).toEqual([]) + }) + + it('a dev server is probed and listed only once it has listened for the settle window', async () => { + const h = harness({ a: ancestorClaude(4173) }) + h.watcher.setSessions([{ sessionId: 'a', tmuxNames: [], terminalSessionIds: [] }]) + await h.watcher.scan() + expect(h.probe).not.toHaveBeenCalled() + expect(h.broadcast.mock.calls.at(-1)?.[0].a ?? []).toEqual([]) + h.advance(PROBE_SETTLE_MS) + await h.watcher.scan() + expect(h.probe.mock.calls.map(c => c[0])).toEqual([4173]) + expect(h.broadcast.mock.calls.at(-1)![0].a.map((p: { port: number }) => p.port)).toEqual([4173]) + }) + + it('while a listener is unsettled the next scan comes when it settles, not after a longer back-off', async () => { + const timers: number[] = [] + let t = 0 + const watcher = new LanePortWatcher({ + // 400 ms per scan ⇒ a 20 × 400 = 8 s back-off, longer than the window. + listProcesses: async () => { t += 400; return parentOf }, + listListeners: async pids => listeners.filter(l => pids.includes(l.pid)), listTmuxPanes: async () => [], + probe: async port => probes.get(port) ?? { status: null, contentType: null }, + agentPid: () => ancestorClaude(4173), terminalPid: () => null, broadcast: () => {}, + now: () => t, setTimer: (_fn, ms) => { timers.push(ms); return () => {} }, + }) + watcher.setSessions([{ sessionId: 'a', tmuxNames: [], terminalSessionIds: [] }]) + await watcher.scan() + const next = timers.at(-1)! + expect(next).toBeGreaterThanOrEqual(SCAN_FLOOR_MS) + expect(next).toBeLessThanOrEqual(PROBE_SETTLE_MS) + }) + + it('pulling the next scan in for a settling listener never goes below the floor', async () => { + const h = harness({ a: ancestorClaude(4173) }) + h.watcher.setSessions([{ sessionId: 'a', tmuxNames: [], terminalSessionIds: [] }]) + await h.watcher.scan() + // About 1 s left in the window: the rescan still waits the floor. + h.advance(PROBE_SETTLE_MS - 1000) + await h.watcher.scan() + expect(h.probe).not.toHaveBeenCalled() + expect(h.timers.at(-1)!.ms).toBe(SCAN_FLOOR_MS) + }) +}) + +// #1452 review A: what "listening for PROBE_SETTLE_MS" is measured from. These +// use a clock that only moves when a test moves it (the shared harness ticks +// 5 ms on every `now()` read, which blurs exact boundaries). Each scenario is +// one the reviewer reproduced against the first version of the window. +function preciseHarness(agent: number) { + const clock = { t: 0 } + const timers: number[] = [] + const probe = vi.fn(async (port: number) => probes.get(port) ?? { status: null, contentType: null }) + const broadcast = vi.fn() + const owned = async (pids: number[]) => listeners.filter(l => pids.includes(l.pid)) + const listListeners = vi.fn(owned) + const listProcesses = vi.fn(async () => parentOf) + const watcher = new LanePortWatcher({ + listProcesses, listListeners, listTmuxPanes: async () => [], probe, broadcast, + agentPid: () => agent, terminalPid: () => null, + now: () => clock.t, setTimer: (_fn, ms) => { timers.push(ms); return () => {} }, + }) + const plan = [{ sessionId: 'a', tmuxNames: [], terminalSessionIds: [] }] + return { clock, timers, probe, broadcast, listListeners, listProcesses, owned, watcher, plan } +} + +describe('#1452: the settle window counts observed time only', () => { + it('a slow lsof does not shorten the window: age starts when the listener was observed', async () => { + const h = preciseHarness(ancestorClaude(4173)) + h.watcher.setSessions(h.plan) + // lsof answers 4.9 s into the scan; that is when 4173 was first seen. + h.listListeners.mockImplementationOnce(async pids => { h.clock.t += 4900; return h.owned(pids) }) + await h.watcher.scan() + h.clock.t += SCAN_FLOOR_MS + await h.watcher.scan() + expect(h.probe).not.toHaveBeenCalled() + }) + + it('a scan that straddles a plan change does not age what it finds', async () => { + const h = preciseHarness(ancestorClaude(4173)) + let release!: () => void + const gate = new Promise(r => { release = r }) + h.listProcesses.mockImplementationOnce(async () => { await gate; return parentOf }) + h.watcher.setSessions(h.plan) + const first = h.watcher.scan() + h.clock.t = 5100 + h.watcher.setSessions([...h.plan]) + release() + await first + // The generation-fenced immediate rescan, still at t = 5100. + await h.watcher.scan() + expect(h.probe).not.toHaveBeenCalled() + }) + + it('an empty plan forgets ages: time spent unwatched is not settled time', async () => { + const h = preciseHarness(ancestorClaude(4173)) + h.watcher.setSessions(h.plan) + await h.watcher.scan() + h.watcher.setSessions([]) + h.clock.t = PROBE_SETTLE_MS + h.watcher.setSessions(h.plan) + await h.watcher.scan() + expect(h.probe).not.toHaveBeenCalled() + }) + + it('probes at exactly PROBE_SETTLE_MS of observed life, not a millisecond before', async () => { + const h = preciseHarness(ancestorClaude(4173)) + h.watcher.setSessions(h.plan) + await h.watcher.scan() + h.clock.t = PROBE_SETTLE_MS - 1 + await h.watcher.scan() + expect(h.probe).not.toHaveBeenCalled() + h.clock.t = PROBE_SETTLE_MS + await h.watcher.scan() + expect(h.probe.mock.calls.map(c => c[0])).toEqual([4173]) + }) + + it('a failed lsof keeps a settled chip on screen and does not restart its window', async () => { + const h = preciseHarness(ancestorClaude(4173)) + h.watcher.setSessions(h.plan) + await h.watcher.scan() + h.clock.t = PROBE_SETTLE_MS + await h.watcher.scan() + const settled = h.broadcast.mock.calls.at(-1)![0] + expect(settled.a.map((p: { port: number }) => p.port)).toEqual([4173]) + const broadcasts = h.broadcast.mock.calls.length + h.listListeners.mockRejectedValueOnce(Object.assign(new Error('lsof timed out'), { killed: true, signal: 'SIGTERM', code: null })) + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + await h.watcher.scan() + warn.mockRestore() + expect(h.broadcast.mock.calls.length).toBe(broadcasts) + h.clock.t += SCAN_FLOOR_MS + await h.watcher.scan() + // Still listed, from the probe cache: no resettle, no second probe. + expect(h.broadcast.mock.calls.length).toBe(broadcasts) + expect(h.probe.mock.calls.map(c => c[0])).toEqual([4173]) + }) +}) + +// #1452 round-2 review A: a scan that was waiting on lsof when the plan was +// emptied must not write its listeners' ages back after the clear. +describe('#1452: an obsolete scan writes nothing', () => { + it('a scan in flight across an empty plan does not restore listener age', async () => { + const h = preciseHarness(ancestorClaude(4173)) + let release!: () => void + const gate = new Promise(r => { release = r }) + h.listListeners.mockImplementationOnce(async pids => { await gate; return h.owned(pids) }) + h.watcher.setSessions(h.plan) + const inFlight = h.watcher.scan() + // Empty the plan only once the scan has captured the old roots and is + // inside lsof; emptying it earlier leaves the scan nothing to look up. + await vi.waitFor(() => expect(h.listListeners).toHaveBeenCalledTimes(1)) + h.watcher.setSessions([]) + release() + await inFlight + h.clock.t = PROBE_SETTLE_MS + h.watcher.setSessions(h.plan) + await h.watcher.scan() + expect(h.probe).not.toHaveBeenCalled() + }) + + it('a probe answer that lands after the plan was emptied is not cached', async () => { + const h = preciseHarness(ancestorClaude(4173)) + h.watcher.setSessions(h.plan) + await h.watcher.scan() + h.clock.t = PROBE_SETTLE_MS + let answer!: () => void + const gate = new Promise(r => { answer = r }) + h.probe.mockImplementationOnce(async port => { await gate; return probes.get(port)! }) + const inFlight = h.watcher.scan() + await vi.waitFor(() => expect(h.probe).toHaveBeenCalledTimes(1)) + h.watcher.setSessions([]) + answer() + await inFlight + // Restored and settled again: the old answer was not kept, so it is asked again. + h.watcher.setSessions(h.plan) + await h.watcher.scan() + h.clock.t = 2 * PROBE_SETTLE_MS + await h.watcher.scan() + expect(h.probe).toHaveBeenCalledTimes(2) + }) +}) + +// #1452 round-3 review A: stop() must invalidate a scan in flight the same way +// an empty plan does. The user-visible effect is a chip broadcast after the +// watcher was stopped. A cached answer or age left behind is not observable +// through the API (a stopped watcher never scans again), so the broadcast is +// what this pins. +describe('#1452: stop() fences a scan in flight', () => { + it('a probe answer that lands after stop() is not broadcast', async () => { + const h = preciseHarness(ancestorClaude(4173)) + h.watcher.setSessions(h.plan) + await h.watcher.scan() + h.clock.t = PROBE_SETTLE_MS + let answer!: () => void + const gate = new Promise(r => { answer = r }) + h.probe.mockImplementationOnce(async port => { await gate; return probes.get(port)! }) + const inFlight = h.watcher.scan() + await vi.waitFor(() => expect(h.probe).toHaveBeenCalledTimes(1)) + const broadcasts = h.broadcast.mock.calls.length + h.watcher.stop() + answer() + await inFlight + expect(h.broadcast.mock.calls.length).toBe(broadcasts) + }) +}) diff --git a/src/main/browserPocket/LanePortWatcher.ts b/src/main/browserPocket/LanePortWatcher.ts index ce1054771..b066e248f 100644 --- a/src/main/browserPocket/LanePortWatcher.ts +++ b/src/main/browserPocket/LanePortWatcher.ts @@ -17,6 +17,8 @@ import { attributePorts, classifyProbe, type Listener, type ProbeResult, type Se * - Scans back off with their own cost: max(3 s, 20 × last scan) — VS Code's * auto-forward formula — so a slow machine scans less, not more. * - A probe answer is cached per pid:port; a dev server is probed once. + * - A listener is probed (and listed) only after the watcher has SEEN it + * listening for PROBE_SETTLE_MS; see that constant for why (#1409). */ export type LanePortWatcherDeps = { /** pid → parent pid for every process (`ps -axo pid=,ppid=`). */ @@ -37,12 +39,69 @@ export type LanePortWatcherDeps = { export const SCAN_FLOOR_MS = 3000 +/** + * How long a listener must have been seen before the watcher sends it its one + * `GET /` and shows it as a chip (#1409). + * + * WHY a settle window at all: agents run test suites inside their lanes, and + * a suite's loopback server is in the lane's process tree like any dev + * server. Probing on the first scan that saw it sent those servers an + * unsolicited request. Tests that count requests then flaked, only on + * developer machines, since CI has no watching app. #1187 was one: the probe + * reached the extension runtime harness's egress server about 1.6 s after it + * started. Every exposed server in #1409's inventory binds port 0 and lives + * for one test or one harness run. Dev servers stay up. So "has it stayed up" + * is the discriminator that needs no process names: `ps` reads PID/PPID + * only, a privacy rule. + * + * WHY not the obvious alternatives (plan + * docs/plans/2026-09-27-lane-port-probe-settle.md): + * - Skipping ephemeral ports (≥ 49152) would hide the recorded tmux Python + * server (62679) and tools that fall back to a random free port. + * - `HEAD` or another path still reaches test handlers. + * - Listing an unsettled listener unprobed, as "other", would flash a chip for + * every test server. + * + * WHY 5 s (an UNCONFIRMED product call): it is about three times the 1.6 s + * observed in #1187 and longer than a typical single test's server. A dev + * server's chip still appears about 6–9 s after it starts, at the 3 s scan + * floor. A counting test whose server outlives the window is still reachable; + * those excuse exactly LANE_PORT_PROBE_USER_AGENT (lanePortsIo.ts). + * + * WHAT "listening for 5 s" means, and its limits (#1452 review A): + * - The age starts when lsof RETURNED the listener (`observedAt`), not when + * the scan started. `ps` and `lsof` may each take up to 3 s under the load + * that #1409 is about. Timing from the scan start once let a listener that + * lsof reported 4.9 s into the scan get probed about 3 s after it was + * first seen. The same start-of-scan timestamp also aged a listener found + * by a scan that straddled a plan change. + * - The age is only as continuous as our sampling. A server that closes and a + * new one that binds the same pid:port BETWEEN two scans look like one + * listener to `lsof`. Nothing short of watching sockets can tell them + * apart, so a fixed-port test server can still inherit a predecessor's age. + * When observation truly stops (an empty plan, stop()), the ages are + * dropped, so a gap of unbounded length never counts as "listening". + * - A failed lsof (timeout, signal, missing binary) is an unknown, not an + * empty answer. It throws, and the scan keeps the last broadcast and the + * ages, instead of pruning a settled dev server and hiding its chip for + * another window (lanePortsIo.listListeners). + * - Cost: a lane whose listeners keep churning (a test runner starting a new + * server every scan) keeps pulling the next scan in to the settle time, so + * the 20 x back-off does not grow for it. That is bounded (never below + * SCAN_FLOOR_MS) and only lasts while something is actually settling. + */ +export const PROBE_SETTLE_MS = 5000 + export class LanePortWatcher { private sessions: PortWatchSession[] = [] private cancel: (() => void) | null = null private inFlight: Promise | null = null private lastScanMs = 0 private probeCache = new Map() + /** pid:port → the `now()` of the scan that first saw it listening. Pruned + * with the probe cache, so a server that restarts on the same pid:port + * settles again rather than inheriting its predecessor's age. */ + private firstSeen = new Map() private lastBroadcast = '' private stopped = false /** Bumped by every setSessions. A scan that started under an older plan @@ -61,6 +120,12 @@ export class LanePortWatcher { if (sessions.length === 0) { // Clear chips immediately rather than leaving the last scan on screen. this.emit({}) + // No scan runs while the plan is empty, so nothing is being observed. + // Keeping the ages would let a listener that reappears after an + // unwatched gap of any length count that gap as settled time, and would + // reuse a probe answer from a server that may since have been replaced + // (#1452 review A). + this.forgetListeners() return } // A plan change (a lane now shows a different agent) deserves an answer @@ -70,8 +135,20 @@ export class LanePortWatcher { stop(): void { this.stopped = true + // Invalidate a scan already in flight, exactly as a plan change does: the + // obsolete-scan fence in runScan compares generations, and without this + // bump a scan awaiting lsof or a probe would, after stop(), still cache + // its answer, record ages into the maps cleared below, and broadcast a + // chip (#1452 round-3 review A). + this.planGeneration++ this.cancel?.() this.cancel = null + this.forgetListeners() + } + + private forgetListeners(): void { + this.firstSeen.clear() + this.probeCache.clear() } /** One scan. Exposed for tests; concurrent callers share the scan in flight. */ @@ -85,6 +162,9 @@ export class LanePortWatcher { if (this.stopped || this.sessions.length === 0) return const started = this.deps.now() const generation = this.planGeneration + // The earliest moment an unsettled listener becomes probeable, so the next + // scan can be pulled in to meet it (see the finally block). + let nextSettleAt: number | null = null try { const [parentOf, panes] = await Promise.all([this.deps.listProcesses(), this.hasTmux() ? this.deps.listTmuxPanes() : Promise.resolve([])]) const panesByName = new Map() @@ -115,13 +195,38 @@ export class LanePortWatcher { queue.push(...(children.get(pid) ?? [])) } const listeners = pids.size ? await this.deps.listListeners([...pids]) : [] + // When these listeners were actually observed; see PROBE_SETTLE_MS for + // why this is not `started`. + const observedAt = this.deps.now() + // WHY a scan from an older plan writes NOTHING from here on (#1452 + // round-2 review A): setSessions([]) forgets every age and probe + // answer, but a scan that was already waiting on lsof resumed after + // that clear and wrote its listeners' ages back, dated before the + // unwatched gap. A listener then counted the gap as settled time and was + // probed on the first scan of the restored plan. So an obsolete scan + // neither records ages, probes, caches nor prunes. Its only effect is + // the immediate rescan the finally block schedules for the new plan. + const current = () => generation === this.planGeneration + if (!current()) return const attributed = attributePorts({ listeners, parentOf, roots }) const out: Record = {} for (const [sessionId, ports] of Object.entries(attributed)) { const rows: LanePort[] = [] for (const p of ports) { - const kind = classifyProbe(await this.probeOnce(p.pid, p.port)) + // Probes await, so the plan can change mid-loop too. + if (!current()) return + const key = `${p.pid}:${p.port}` + const seenAt = this.firstSeen.get(key) ?? observedAt + this.firstSeen.set(key, seenAt) + if (observedAt - seenAt < PROBE_SETTLE_MS) { + // Not contacted, not listed: a test server that is gone before it + // settles never learns the watcher exists (#1409). + const settleAt = seenAt + PROBE_SETTLE_MS + nextSettleAt = nextSettleAt === null ? settleAt : Math.min(nextSettleAt, settleAt) + continue + } + const kind = classifyProbe(await this.probeOnce(p.pid, p.port, current)) if (kind === 'ignore') continue rows.push({ port: p.port, pid: p.pid, url: `http://localhost:${p.port}/`, kind }) } @@ -130,16 +235,27 @@ export class LanePortWatcher { } // Probe cache entries for listeners that are gone would otherwise pin // a restarted server's old answer to a reused port. + if (!current()) return const live = new Set(listeners.map(l => `${l.pid}:${l.port}`)) for (const key of this.probeCache.keys()) if (!live.has(key)) this.probeCache.delete(key) - if (generation === this.planGeneration) this.emit(out) + for (const key of this.firstSeen.keys()) if (!live.has(key)) this.firstSeen.delete(key) + this.emit(out) } catch (error) { + // Nothing is pruned or broadcast on failure: the chips and the ages + // from the last good scan stand until a scan succeeds. console.warn('[browser-pocket] port scan failed:', error instanceof Error ? error.message : error) } finally { - this.lastScanMs = this.deps.now() - started + const ended = this.deps.now() + this.lastScanMs = ended - started // A plan that changed during this scan is answered right away. const stale = generation !== this.planGeneration - if (!this.stopped && this.sessions.length) this.schedule(stale ? 0 : Math.max(SCAN_FLOOR_MS, 20 * this.lastScanMs)) + let next = Math.max(SCAN_FLOOR_MS, 20 * this.lastScanMs) + // While a listener is waiting out the settle window, do not let a slow + // machine's 20 x back-off push its chip out by up to a minute: rescan + // when it settles. The floor still holds, so this never scans faster + // than an idle watcher would. + if (nextSettleAt !== null) next = Math.min(next, Math.max(SCAN_FLOOR_MS, nextSettleAt - ended)) + if (!this.stopped && this.sessions.length) this.schedule(stale ? 0 : next) } } @@ -147,12 +263,14 @@ export class LanePortWatcher { return this.sessions.some(s => s.tmuxNames.length > 0) } - private async probeOnce(pid: number, port: number): Promise { + private async probeOnce(pid: number, port: number, current: () => boolean): Promise { const key = `${pid}:${port}` const cached = this.probeCache.get(key) if (cached) return cached const result = await this.deps.probe(port).catch((): ProbeResult => ({ status: null, contentType: null })) - this.probeCache.set(key, result) + // The plan may have been emptied (and the cache cleared) while the probe + // was in flight; see `current` in runScan. + if (current()) this.probeCache.set(key, result) return result } diff --git a/src/main/browserPocket/lanePortsIo.test.ts b/src/main/browserPocket/lanePortsIo.test.ts new file mode 100644 index 000000000..f66e785b3 --- /dev/null +++ b/src/main/browserPocket/lanePortsIo.test.ts @@ -0,0 +1,44 @@ +import { createServer, type Server } from 'node:http' +import { afterEach, describe, expect, it } from 'vitest' + +import { LANE_PORT_PROBE_USER_AGENT, listenersFromLsofError, probe } from './lanePortsIo' + +// #1409: the probe names itself, so a developer who finds it in a server log +// can tell what sent it, and a long-lived test that counts requests can +// excuse exactly this request instead of "any GET /" or "any node fetch". +// A real loopback socket, because the header is what reaches the server. +let server: Server | null = null +afterEach(async () => { + await new Promise(resolve => server ? server.close(() => resolve()) : resolve()) + server = null +}) + +it('sends GET / with the lane port probe User-Agent', async () => { + const seen: Array<{ method?: string; url?: string; ua?: string }> = [] + server = createServer((request, response) => { + seen.push({ method: request.method, url: request.url, ua: request.headers['user-agent'] }) + response.setHeader('content-type', 'text/html') + response.end('

ok

') + }) + await new Promise(resolve => server!.listen(0, '127.0.0.1', resolve)) + const port = (server.address() as { port: number }).port + expect(await probe(port)).toEqual({ status: 200, contentType: 'text/html' }) + expect(seen).toEqual([{ method: 'GET', url: '/', ua: LANE_PORT_PROBE_USER_AGENT }]) + expect(LANE_PORT_PROBE_USER_AGENT).toMatch(/^AgentCode-LanePortProbe\//) +}) + +// #1452 review A: only lsof's "nothing matched" (exit 1) is an answer. A +// timeout or missing binary must not read as "every server stopped". +describe('listenersFromLsofError', () => { + it('exit status 1 is an answer: its stdout is parsed', () => { + expect(listenersFromLsofError({ code: 1, killed: false, signal: null, stdout: '' })).toEqual([]) + }) + it.each([ + ['a timeout (execFile kills the child)', { code: null, killed: true, signal: 'SIGTERM', stdout: '' }], + ['a signal', { code: null, killed: false, signal: 'SIGKILL', stdout: '' }], + ['a missing binary', { code: 'ENOENT', stdout: undefined }], + ['any other exit status', { code: 2, killed: false, signal: null, stdout: '' }], + ])('%s throws', (_label, error) => { + expect(() => listenersFromLsofError(error)).toThrow() + }) +}) diff --git a/src/main/browserPocket/lanePortsIo.ts b/src/main/browserPocket/lanePortsIo.ts index 106a1045c..927e9321b 100644 --- a/src/main/browserPocket/lanePortsIo.ts +++ b/src/main/browserPocket/lanePortsIo.ts @@ -28,12 +28,39 @@ export async function listListeners(pids: number[]): Promise { const { stdout } = await run('/usr/sbin/lsof', ['-nP', '-a', '-iTCP', '-sTCP:LISTEN', '-F', 'pcn', '-p', pids.join(',')], { timeout: 3000, maxBuffer: 8 * 1024 * 1024 }) return parseLsofListen(stdout) } catch (error) { - // Exit status 1 with empty output is lsof's "nothing matched" (recorded), - // not a failure. - return parseLsofListen((error as { stdout?: string }).stdout ?? '') + return listenersFromLsofError(error) } } +/** + * Exit status 1 is lsof's "nothing matched" (recorded), not a failure, and + * its stdout is still the answer. Anything else is a failed observation: a + * timeout (execFile kills the child, so `killed`/`signal` is set and `code` + * is null), a signal, or a missing binary (`code: 'ENOENT'`). Those throw. + * Before #1452 they became `[]`, which LanePortWatcher read as "every server + * stopped". That pruned a settled dev server, and with the settle window its + * chip then stayed hidden for another 5 s after lsof recovered (review A). + */ +export function listenersFromLsofError(error: unknown): Listener[] { + const e = error as { code?: unknown; killed?: boolean; signal?: unknown; stdout?: string } + if (e.code === 1 && !e.killed && !e.signal) return parseLsofListen(e.stdout ?? '') + throw error +} + +/** + * The User-Agent every lane port probe sends (#1409). + * + * WHY name ourselves: the probe lands in a developer's own server. A log line + * that says `AgentCode-LanePortProbe` explains itself; Node's default `node` + * does not. It is also the one exact thing a long-lived test that counts + * requests can excuse. The settle window (PROBE_SETTLE_MS in + * LanePortWatcher.ts) keeps short tests from ever seeing the probe, but a + * harness that outlives the window still can. Excusing "any `GET /`" (#1406) + * would also excuse a real regression that requests `/`. Keep this stable: + * tests match it verbatim. + */ +export const LANE_PORT_PROBE_USER_AGENT = 'AgentCode-LanePortProbe/1' + /** * One `GET /` on loopback. Only ever called for listeners inside a watched * lane's own process tree; `redirect: 'manual'` so a redirect is classified, @@ -42,7 +69,7 @@ export async function listListeners(pids: number[]): Promise { */ export async function probe(port: number): Promise { try { - const res = await fetch(`http://127.0.0.1:${port}/`, { redirect: 'manual', signal: AbortSignal.timeout(1000) }) + const res = await fetch(`http://127.0.0.1:${port}/`, { redirect: 'manual', headers: { 'user-agent': LANE_PORT_PROBE_USER_AGENT }, signal: AbortSignal.timeout(1000) }) await res.body?.cancel().catch(() => {}) return { status: res.status, contentType: res.headers.get('content-type') } } catch { diff --git a/src/main/conversations/catalog/listing.ts b/src/main/conversations/catalog/listing.ts index bfbc7b158..5d4427009 100644 --- a/src/main/conversations/catalog/listing.ts +++ b/src/main/conversations/catalog/listing.ts @@ -67,7 +67,7 @@ export function buildListing(input: BuildListingInput): ConversationListResponse total, hiddenChildren, nextCursor: start + limit < visible.length && last ? encodeCursor(last) : null, - family: { repoRoot: family.root, roots: family.roots }, + family: { repoRoot: family.root, roots: family.roots, ...(family.gitTimedOut ? { gitTimedOut: true as const } : {}) }, timing: { ms: Math.max(0, Date.now() - input.startedAt) }, } } diff --git a/src/main/conversations/family.ts b/src/main/conversations/family.ts index 5c5c095e7..bf76ea899 100644 --- a/src/main/conversations/family.ts +++ b/src/main/conversations/family.ts @@ -37,11 +37,24 @@ export type RepositoryFamily = { * project directory name from the literal cwd, so a lowercased root * would name a directory that does not exist. */ rawRoots: string[] + /** `git worktree list` timed out (#1430), so the siblings are UNKNOWN, not + * absent: `roots` fell back to the cwd alone and may be missing the main + * checkout and other worktrees. The service does not cache such a family, + * and the picker says so. False when git answered (or is simply not a + * repository here). */ + gitTimedOut: boolean matches(candidate: string | null | undefined): boolean } +/** A plain list means git answered. The detailed form (main's + * listWorktreesForCwdDetailed) can also say the list timed out (#1430); + * both are accepted so fixtures that hand in a known list stay as they are. */ +export type ListedWorktrees = + | ReadonlyArray<{ path: string }> + | { worktrees: ReadonlyArray<{ path: string }>; timedOut: boolean } + export type FamilyDeps = { - listWorktrees(cwd: string): Promise> + listWorktrees(cwd: string): Promise } /** `path.resolve` collapses `..` and trailing slashes; darwin and win32 file @@ -76,8 +89,12 @@ export async function resolveFamily( const cwdForms = forms(cwd) const cwdRoots = cwdForms.map(normalizeCwd) let worktreeForms: string[][] = [] + let gitTimedOut = false try { - worktreeForms = (await deps.listWorktrees(cwd)).map(w => forms(w.path)) + const listed = await deps.listWorktrees(cwd) + const worktrees = 'timedOut' in listed ? listed.worktrees : listed + gitTimedOut = 'timedOut' in listed && listed.timedOut === true + worktreeForms = worktrees.map(w => forms(w.path)) } catch { worktreeForms = [] } @@ -122,6 +139,7 @@ export async function resolveFamily( root, roots, rawRoots, + gitTimedOut, matches(candidate) { if (scope === 'everywhere') return true if (!candidate) return false diff --git a/src/main/conversations/service.system.test.ts b/src/main/conversations/service.system.test.ts index 1ef00d6e3..76d7cc04d 100644 --- a/src/main/conversations/service.system.test.ts +++ b/src/main/conversations/service.system.test.ts @@ -139,3 +139,44 @@ describe('ConversationService', () => { }) }) +// #1430 (found by #1429 review b): `git worktree list` timing out answered `[]`, +// resolveFamily fell back to the cwd alone, and the service cached that guess +// for DISCOVERY_FRESH_MS. From a linked checkout the main checkout's +// conversations dropped out of the repository picker and nothing said so. +describe('ConversationService when git times out listing worktrees', () => { + it('says the family is incomplete, does not cache it, and recovers when git answers', async () => { + const porcelain = await corpusWorktreesPorcelain() + const worktrees = porcelain.split('\n').filter(l => l.startsWith('worktree ')).map(l => ({ path: l.slice('worktree '.length) })) + let timedOut = true + const calls: string[] = [] + const claudeHistory = new ClaudeHistoryIndex(join(corpus.claudeConfigDir, 'history.jsonl')) + const s = new ConversationService({ + sources: [ + new ClaudeConversationSource({ projectsDir: join(corpus.claudeConfigDir, 'projects'), history: claudeHistory }), + new CodexConversationSource({ codexHome: corpus.codexHome }), + new OpencodeConversationSource({ dataDir: corpus.opencodeDataDir }), + ], + ledger: null, + // The shape main's listWorktreesForCwdDetailed answers. + listWorktrees: async cwd => { + calls.push(cwd) + return timedOut ? { worktrees: [], timedOut: true } : { worktrees, timedOut: false } + }, + claudeHistory, + }) + const cwd = '/fixture/repo/.worktrees/extension-platform' + + const slow = await s.list({ cwd, scope: 'repository', limit: 500 }) + expect(slow.family.gitTimedOut).toBe(true) + // Asked again at once: a guessed family is never served from the cache. + await s.list({ cwd, scope: 'repository', limit: 500 }) + expect(calls).toHaveLength(2) + + timedOut = false + const answered = await s.list({ cwd, scope: 'repository', limit: 500 }) + expect(answered.family.gitTimedOut).toBeUndefined() + expect(answered.family.repoRoot).toBe('/fixture/repo') + // The main checkout's rows are back: the guessed family was missing some. + expect(answered.total).toBeGreaterThan(slow.total) + }) +}) diff --git a/src/main/conversations/service.ts b/src/main/conversations/service.ts index 5253f1ba4..938aa4aae 100644 --- a/src/main/conversations/service.ts +++ b/src/main/conversations/service.ts @@ -11,7 +11,7 @@ import { performanceService } from '@main/performance/PerformanceService.js' import { buildListing, HIDDEN_KINDS } from './catalog/listing.js' import { normalizeConversation } from './catalog/normalize.js' import { unwrapUserText } from './catalog/unwrap.js' -import { resolveFamily, type RepositoryFamily } from './family.js' +import { resolveFamily, type ListedWorktrees, type RepositoryFamily } from './family.js' import type { ConversationLedger } from './ledger/ledger.js' import type { LedgerRow } from './ledger/types.js' import { ClaudeConversationSource } from './sources/claude.js' @@ -56,7 +56,7 @@ const SEARCH_BYTES_PER_ROW = 128 * 1024 type Discovery = { at: number; key: string; family: RepositoryFamily; sources: SourceConversation[] } -export type ListWorktrees = (cwd: string) => Promise> +export type ListWorktrees = (cwd: string) => Promise export class ConversationService { private discovery: Discovery | null = null @@ -95,7 +95,10 @@ export class ConversationService { return [] as SourceConversation[] }))) const discovery: Discovery = { at: Date.now(), key, family, sources: perSource.flat() } - this.discovery = discovery + // #1430: a family built while git timed out is a guess (the cwd alone), so it answers this + // request and is not kept — the next one asks git again instead of serving the guess for + // DISCOVERY_FRESH_MS. + if (!family.gitTimedOut) this.discovery = discovery this.discoveries++ span.end({ rows: discovery.sources.length }) return discovery diff --git a/src/main/extensions/serviceLanListener.test.ts b/src/main/extensions/serviceLanListener.test.ts index c8f97f7d5..14eee6a26 100644 --- a/src/main/extensions/serviceLanListener.test.ts +++ b/src/main/extensions/serviceLanListener.test.ts @@ -26,13 +26,26 @@ afterEach(async () => { upstream = null }) +// WHY `GET /` is not recorded (#1409 / #1452 review b): when this suite runs +// inside an Agent Code lane, the browser pocket's LanePortWatcher finds both +// sockets in the lane's process tree. Once one has listened for its 5 s settle +// window (only a stalled run gets that far), the watcher sends it one `GET /`. +// Aimed at the LAN listener, that request is forwarded here WITHOUT its +// identifying User-Agent, because the listener's header allow-list drops it, +// correctly. So the upstream cannot tell the probe by header. It can tell it +// by shape: `send()` below never uses `/`, so no request this suite makes is +// `GET /`. Residual: a forwarding regression that emits an extra `GET /` +// would be excused. Any other extra request still fails `toHaveLength(1)`. +const WATCHER_PROBE_PATH = '/' +const TEST_PATH = '/lan-contract' + async function wire(responseHeaders: Record = {}): Promise<{ seen: Seen[]; port: number }> { const seen: Seen[] = [] upstream = createServer((req, res) => { let body = '' req.on('data', chunk => { body += chunk }) req.on('end', () => { - seen.push({ method: req.method, url: req.url, headers: req.headers, body }) + if (!(req.method === 'GET' && req.url === WATCHER_PROBE_PATH)) seen.push({ method: req.method, url: req.url, headers: req.headers, body }) res.writeHead(200, { 'content-type': 'application/json', ...responseHeaders }) res.end('{"ok":true}') }) @@ -46,7 +59,7 @@ async function wire(responseHeaders: Record = {}): Promise<{ see * forwarding headers, and a hostile LAN peer is under no such restriction. */ function send(port: number, options: { method?: string; path?: string; headers?: Record; body?: string }) { return new Promise<{ status: number; headers: IncomingHttpHeaders; body: string }>((resolve, reject) => { - const req = httpRequest({ host: '127.0.0.1', port, method: options.method ?? 'GET', path: options.path ?? '/', headers: options.headers }, res => { + const req = httpRequest({ host: '127.0.0.1', port, method: options.method ?? 'GET', path: options.path ?? TEST_PATH, headers: options.headers }, res => { let body = '' res.on('data', chunk => { body += chunk }) res.on('end', () => resolve({ status: res.statusCode ?? 0, headers: res.headers, body })) @@ -94,6 +107,16 @@ describe('service LAN listener forwarding contract', () => { expect(headers['x-evil']).toBeUndefined() }) + // #1452 review b's failure sequence, on real sockets: the lane port watcher's + // probe reaches the LAN listener mid-test, loses its User-Agent in + // forwarding, and must not turn the contract's one request into two. + it('a lane port watcher probe forwarded mid-test does not count as a contract request', async () => { + const { seen, port } = await wire() + await send(port, { method: 'GET', path: '/', headers: { 'user-agent': 'AgentCode-LanePortProbe/1' } }) + await send(port, { method: 'POST', body: '{"kind":"check"}', headers: { 'content-type': 'application/json' } }) + expect(seen.map(s => `${s.method} ${s.url}`)).toEqual(['POST /lan-contract']) + }) + // The security property the whole contract rests on: a guest must not be // able to claim it is the host's own frame, or pose as another address. it('a LAN peer cannot forge the attestation or forwarding facts', async () => { diff --git a/src/main/index.ts b/src/main/index.ts index 39c137d5f..7842601c2 100644 --- a/src/main/index.ts +++ b/src/main/index.ts @@ -119,7 +119,8 @@ import { WorkspaceFileStore } from '@main/storage/workspaceFileStore.js' import type { PersistedWindow } from '@main/storage/workspaceFile.js' import { ConversationLedger, readAgentNameAssignments } from '@main/conversations/ledger/ledger.js' import { createConversationService } from '@main/conversations/service.js' -import { listWorktreesForCwd } from '@main/ipc/git.js' +import { listWorktreesForCwdDetailed } from '@main/ipc/git.js' +import { resolveRepoRootAfterGit } from '@main/agentActivity/resolveRepoRoot.js' import { AGENT_NAMES_FILE } from '@main/agentNames/ipc.js' import { RemoteWorkspaceProjection } from '@main/remote/workspaceProjection.js' import { tldrIdentitiesInUse } from '@main/tldr/identitiesInUse.js' @@ -1442,7 +1443,8 @@ async function startApp(): Promise { store: new AgentActivityStore(AGENT_ACTIVITY_DIR), // The first worktree entry is the main checkout, so every worktree of one // repository folds into it (the conversations picker's family rule). - resolveRepoRoot: cwd => listWorktreesForCwd(cwd).then(worktrees => worktrees[0]?.path ?? cwd), + // #1430: a timed-out list is retried once, then thrown (see resolveRepoRootAfterGit). + resolveRepoRoot: cwd => resolveRepoRootAfterGit(listWorktreesForCwdDetailed, cwd), identityOf: sessionId => builtInMcpHost.sessionTldrIdentity(sessionId), }) const projectActivity = (windows: readonly PersistedWindow[]) => { @@ -1559,7 +1561,8 @@ async function startApp(): Promise { // the ledger, so it is constructed after the ledger. The control host above // was built before the workspace store opened and holds a getter for it; // its handlers only run on requests, long after this line. - const conversationService = createConversationService({ ledger: conversationLedger, listWorktrees: listWorktreesForCwd }) + // The detailed lister (#1430): a timed-out list is a family the service must not cache. + const conversationService = createConversationService({ ledger: conversationLedger, listWorktrees: listWorktreesForCwdDetailed }) registerAllIpc({ manager, sessionFeedTap: feedTap, diff --git a/src/main/ipc/git.ts b/src/main/ipc/git.ts index 3ffd69f38..51fba7867 100644 --- a/src/main/ipc/git.ts +++ b/src/main/ipc/git.ts @@ -237,13 +237,14 @@ function parseWorktreePorcelain(out: string): WorktreeIdentity[] { return worktrees } -export async function listWorktreesForCwd(cwd: string): Promise { - return (await listWorktreesForCwdDetailed(cwd)).worktrees -} +// No plain `listWorktreesForCwd` any more (#1430, review c): it answered `[]` +// for a timeout as well as for "not a repository", and every consumer that +// read it that way was moved to the detailed answer below. A new caller that +// only needs the list must still decide what a timeout means to it. /** The list plus whether `git worktree list` timed out, which an empty list * alone cannot tell apart from "not a git repository" (#1250 row 11). */ -async function listWorktreesForCwdDetailed(cwd: string): Promise<{ worktrees: WorktreeIdentity[]; timedOut: boolean }> { +export async function listWorktreesForCwdDetailed(cwd: string): Promise<{ worktrees: WorktreeIdentity[]; timedOut: boolean }> { const now = Date.now() const cached = worktreeListCache.get(cwd) if (cached && (!cached.settled || cached.expiresAt > now)) { @@ -564,7 +565,7 @@ async function inspectSubmodule( export function registerGitIpc(): void { // WHY both worktree handlers carry `gitMissing` exactly like git:status - // (#508 review): a machine without a working git makes listWorktreesForCwd + // (#508 review): a machine without a working git makes the worktree lister // return [] (the '' swallow), which these handlers collapse into a bare // { ok:false } — and the Worktrees panel then renders "not a git // repository", the same lie A5 fixed on GitBar. The flag is read AFTER diff --git a/src/main/ipc/userMcp.ts b/src/main/ipc/userMcp.ts index a8f8479f0..c57005c76 100644 --- a/src/main/ipc/userMcp.ts +++ b/src/main/ipc/userMcp.ts @@ -57,6 +57,15 @@ export function registerUserMcpIpc(service: UserMcpService): void { return service.setSecret(id, inputId, value) }) + // User-only (q114): the Settings dialog's "Confirm for this server". Not on + // the agent tool surface. + ipcMain.handle('user-mcp:confirm-secret', (_evt, id: unknown, inputId: unknown) => { + if (!isUserMcpServerId(id) || typeof inputId !== 'string') { + throw new Error('user-mcp:confirm-secret: invalid arguments') + } + return service.confirmSecret(id, inputId) + }) + ipcMain.handle('user-mcp:import', (_evt, text: unknown, fallbackName: unknown) => { if (typeof text !== 'string' || text.length > MAX_IMPORT_CHARS) throw new Error('user-mcp:import: invalid text') return service.importConfig(text, typeof fallbackName === 'string' ? fallbackName : undefined) diff --git a/src/main/ipc/worktreeActivity.test.ts b/src/main/ipc/worktreeActivity.test.ts new file mode 100644 index 000000000..8a6974e76 --- /dev/null +++ b/src/main/ipc/worktreeActivity.test.ts @@ -0,0 +1,39 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +// #1430: the REAL worktree-activity handler; Electron's ipcMain is captured and +// the git lister is the edge. A timed-out `git worktree list` used to throw +// "not a git worktree" here and answer `{ ok: false }` — indistinguishable from +// a non-repository, and shown as a missing activity index. +const handlers = new Map Promise>() +vi.mock('electron', () => ({ ipcMain: { handle: (channel: string, handler: (...args: unknown[]) => Promise) => handlers.set(channel, handler) } })) +const git = vi.hoisted(() => ({ listWorktreesForCwdDetailed: vi.fn() })) +vi.mock('@main/ipc/git.js', () => git) + +import { registerWorktreeActivityIpc } from './worktreeActivity' + +const getSummary = vi.fn(async () => ({ summaries: [], status: { lastIndexedAt: null } })) +beforeEach(() => { + handlers.clear() + getSummary.mockClear() + registerWorktreeActivityIpc({ getSummary } as never) +}) +const summary = (cwd: string) => handlers.get('worktree-activity:summary')!({}, cwd, false) + +describe('worktree-activity:summary', () => { + it('says a git timeout instead of answering "not a repository"', async () => { + git.listWorktreesForCwdDetailed.mockResolvedValue({ worktrees: [], timedOut: true }) + expect(await summary('/repo')).toEqual({ ok: false, timedOut: true }) + expect(getSummary).not.toHaveBeenCalled() + }) + + it('keeps a plain non-repository a plain failure', async () => { + git.listWorktreesForCwdDetailed.mockResolvedValue({ worktrees: [], timedOut: false }) + expect(await summary('/tmp/plain')).toEqual({ ok: false }) + }) + + it('summarises a repository git answered for', async () => { + git.listWorktreesForCwdDetailed.mockResolvedValue({ worktrees: [{ path: '/repo' }], timedOut: false }) + expect(await summary('/repo')).toMatchObject({ ok: true, summaries: [] }) + expect(getSummary).toHaveBeenCalledWith({ worktrees: [{ path: '/repo' }], refresh: false }) + }) +}) diff --git a/src/main/ipc/worktreeActivity.ts b/src/main/ipc/worktreeActivity.ts index b7d54482c..ca5ff1771 100644 --- a/src/main/ipc/worktreeActivity.ts +++ b/src/main/ipc/worktreeActivity.ts @@ -1,6 +1,6 @@ import { ipcMain } from 'electron' -import { listWorktreesForCwd } from '@main/ipc/git.js' +import { listWorktreesForCwdDetailed } from '@main/ipc/git.js' import type { WorktreeActivityIndex } from '@main/worktreeActivity/WorktreeActivityIndex.js' export function registerWorktreeActivityIpc(index: WorktreeActivityIndex): void { @@ -8,7 +8,11 @@ export function registerWorktreeActivityIpc(index: WorktreeActivityIndex): void 'worktree-activity:summary', async (_evt, cwd: string, refresh?: boolean) => { try { - const worktrees = await listWorktreesForCwd(cwd) + const { worktrees, timedOut } = await listWorktreesForCwdDetailed(cwd) + // #1430: an empty list from a TIMED-OUT git is "unknown", not "not a + // repository". Said as its own answer so the panel and agents reading + // it (worktrees.read) do not report the activity index as missing. + if (timedOut) return { ok: false as const, timedOut: true as const } if (worktrees.length === 0) throw new Error('not a git worktree') const result = await index.getSummary({ worktrees, diff --git a/src/main/sessionManager.lifecycle.test.ts b/src/main/sessionManager.lifecycle.test.ts index 1f67dfac7..86e65f761 100644 --- a/src/main/sessionManager.lifecycle.test.ts +++ b/src/main/sessionManager.lifecycle.test.ts @@ -199,6 +199,21 @@ describe('SessionManager lifecycle journal', () => { // the failure is silent in exactly the way that made #683 take a full // journal dig to diagnose. + // #1114: a session may refuse input (OpenCode Terminal's bounded pre-paint + // hold). The refusal reaches the caller as `false`, like a missing backend, + // so the renderer can say so. + it('reports a session\'s refusal to the caller', async () => { + const { SessionManager } = await import('./sessionManager') + const spy = journalSpy() + const manager = new SessionManager(null, null, spy.journal as never) + await manager.recover({ sessionId: 's1', kind: 'claude', cwd: '/tmp/project' }) + const entry = (manager as unknown as { sessions: Map unknown } }> }).sessions.get('s1')! + entry.session.write = () => false + expect(manager.write('s1', 'refused')).toBe(false) + entry.session.write = () => undefined + expect(manager.write('s1', 'accepted')).toBe(true) + }) + it('defaults an unlabelled write to renderer rather than inventing an origin', async () => { const { SessionManager } = await import('./sessionManager') const spy = journalSpy() diff --git a/src/main/sessionManager.ts b/src/main/sessionManager.ts index 7e24d19cb..d4019a441 100644 --- a/src/main/sessionManager.ts +++ b/src/main/sessionManager.ts @@ -4115,8 +4115,11 @@ export class SessionManager extends EventEmitter { // The origin the caller supplies is as precise as the boundary can be // without a contract change; see InputWriteOrigin. this.recordInputWrite(sessionId, data, origin) - entry.session.write(data) - return true + // A session may refuse input it cannot take (#1114: OpenCode Terminal's + // bounded hold before its TUI paints). Refused input was not written, so + // the caller is told, exactly as for a missing backend above. The journal + // row above still says the write was attempted, which it was. + return entry.session.write(data) !== false } private writeReserved( diff --git a/src/main/userMcp/secrets.ts b/src/main/userMcp/secrets.ts index 16de0ca24..9b506415c 100644 --- a/src/main/userMcp/secrets.ts +++ b/src/main/userMcp/secrets.ts @@ -1,4 +1,4 @@ -import { mkdir, readFile, readdir, rename, rm, writeFile } from 'node:fs/promises' +import { mkdir, readFile, readdir, rename, rm, stat, writeFile } from 'node:fs/promises' import { join } from 'node:path' import type { SecretCodec } from '@main/keyVault/vaultStore.js' @@ -26,6 +26,71 @@ import type { UserMcpSecretState } from '@shared/userMcp/types.js' * ids and input ids are both validated to `[A-Za-z0-9_-]`, so they are safe * path segments by construction. */ +/** + * Every record is BOUND to the destination it was saved for (#1304, q113). + * + * The plaintext that gets encrypted is `BOUND_PREFIX + JSON({ d, v })`: `d` is + * the server's destination identity (`userMcpDestination` of its entry) at the + * moment the secret was saved, `v` the secret. A read supplies the server's + * CURRENT destination and gets the value only if `d` matches; anything else + * reads as "not set", so launch refuses to attach the server. + * + * WHY at read time and not by write order: a destination change is several + * writes (the document, then each blob), and a crash or a failed rollback or + * restore can stop between any two. Two reviews found such a window in each + * direction: the NEW address with the OLD token, then the OLD address with + * the NEW token. Any ordering leaves one open. A binding check at every read + * holds whatever state is left on disk: a token only ever reaches the + * destination it was entered for. + * + * Records written before binding existed (plain value, no prefix) are NEVER + * bound automatically (q114). An old version could have crashed between + * publishing a new destination and clearing the old token, leaving document B + * next to a token entered for A; binding legacy records to "the destination + * the document names" would stamp that token as B's and launch it. So an + * unbound record is kept on disk (never deleted) but reads as not set, and + * Settings asks the user to confirm it for the current destination + * (`confirm`) or re-enter it. That user action is the proof that binds + * it. Upgrading users pay a one-time confirmation per stored secret. + */ +const BOUND_PREFIX = 'agent-code/user-mcp-secret/v1:' + +/** + * What a record is bound to (B6 R3 at 4d5c79ab). `destination` alone was not + * enough: the entry text can stay identical while the VALUE of another input + * moves the request (`API_BASE_URL=${input:svc-API_BASE_URL}`, or the host in + * `https://${input:host}/mcp?key=${input:tok}`), and an agent can set such a + * value with mcp_servers_set_secret. So a record also carries `inputs`, a + * digest of the values of EVERY other input the entry references (q127: no + * classifier; a key's name does not prove its role). See service.ts + * bindingsFor. A record's own value is never in its digest, so the record for + * a rotated token stays bound; the user's edit rebinds its siblings + * (service.ts writeValues). + */ +export type SecretBinding = { destination: string; inputs: string } + +function bindValue(binding: SecretBinding, value: string): string { + return BOUND_PREFIX + JSON.stringify({ d: binding.destination, x: binding.inputs, v: value }) +} + +type SecretRecord = + | { kind: 'unbound'; value: string } + // `inputs` is absent on records written before B6 R3 (never merged, so only + // dev data); it then matches nothing and the record needs confirmation. + | { kind: 'bound'; destination: string; inputs: string | undefined; value: string } + +function parseRecord(plaintext: string): SecretRecord | null { + if (plaintext === '') return null + if (!plaintext.startsWith(BOUND_PREFIX)) return { kind: 'unbound', value: plaintext } + try { + const record = JSON.parse(plaintext.slice(BOUND_PREFIX.length)) as { d?: unknown; x?: unknown; v?: unknown } + if (typeof record.d !== 'string' || typeof record.v !== 'string') return null + return { kind: 'bound', destination: record.d, inputs: typeof record.x === 'string' ? record.x : undefined, value: record.v } + } catch { + return null + } +} + export class UserMcpSecretStore { constructor( private readonly dir: string, @@ -40,7 +105,83 @@ export class UserMcpSecretStore { } } - async get(serverId: string, inputId: string): Promise { + private async record(serverId: string, inputId: string): Promise { + const plaintext = await this.decrypted(serverId, inputId) + return plaintext === null ? null : parseRecord(plaintext) + } + + /** The secret, only if it was saved for exactly `binding` (see BOUND_PREFIX). */ + async get(serverId: string, inputId: string, binding: SecretBinding): Promise { + const record = await this.record(serverId, inputId) + if (record?.kind !== 'bound' || record.value === '') return null + return record.destination === binding.destination && record.inputs === binding.inputs ? record.value : null + } + + /** + * Whether a blob EXISTS for this input, decryptable or not (q130). Only a + * verified-absent file (ENOENT) is "no secret". Any other failure throws, + * so a caller deciding whether it may overwrite or delete fails closed. A + * present blob that cannot be decrypted (a key mismatch, a corrupt file) + * still holds the user's secret and must not be treated as empty. + */ + async present(serverId: string, inputId: string): Promise { + try { + await stat(this.path(serverId, inputId)) + return true + } catch (error) { + if ((error as NodeJS.ErrnoException).code === 'ENOENT') return false + throw error + } + } + + /** + * The input ids that have a blob on disk for this server, whether or not the + * server still defines them (r3 round 3: an agent edit that drops an input + * keeps its blob). Strict like snapshotServer: only a missing directory + * means none; any other failure throws, so a guard built on it fails closed. + */ + async storedInputIds(serverId: string): Promise { + let files: string[] + try { + files = await readdir(join(this.dir, serverId)) + } catch (error) { + if ((error as NodeJS.ErrnoException).code === 'ENOENT') return [] + throw error + } + return files.filter(file => file.endsWith('.bin')).map(file => file.slice(0, -'.bin'.length)) + } + + /** + * The stored value whatever it is bound to, ONLY for computing other + * records' bindings (service.ts bindingFor): the digest has to see the value + * the next launch would substitute. Never handed to a launch. + */ + async storedValue(serverId: string, inputId: string): Promise { + const record = await this.record(serverId, inputId) + return record ? record.value : null + } + + /** + * Bind a withheld record to `binding` because the USER confirmed it + * (q114, B6 R3). Two cases only: + * - an unbound record from an earlier version: it carries no proof of its + * destination, and the user's confirmation is that proof; + * - a record for the SAME destination whose other input values changed + * (an agent set a new base URL): the user confirms it may go there. + * A record bound to a DIFFERENT destination is never confirmable: that is + * the q113 crash window (a token next to a document it was not saved for), + * and only re-entering it may bind it. Returns false when there is nothing + * to confirm, leaving the bytes untouched. + */ + async confirm(serverId: string, inputId: string, binding: SecretBinding): Promise { + const record = await this.record(serverId, inputId) + if (!record || record.value === '') return false + if (record.kind === 'bound' && (record.destination !== binding.destination || record.inputs === binding.inputs)) return false + await this.write(serverId, inputId, bindValue(binding, record.value)) + return true + } + + private async decrypted(serverId: string, inputId: string): Promise { if (!this.available()) return null let ciphertext: Buffer try { @@ -49,19 +190,23 @@ export class UserMcpSecretStore { return null } try { - const value = this.codec.decrypt(ciphertext) - return value === '' ? null : value + return this.codec.decrypt(ciphertext) } catch { // Left in place on purpose: if the keyring comes back, so does the value. return null } } - async set(serverId: string, inputId: string, value: string): Promise { + /** Save `value` bound to `binding`, what it is for. */ + async set(serverId: string, inputId: string, value: string, binding: SecretBinding): Promise { if (value === '') { await this.clear(serverId, inputId) return } + await this.write(serverId, inputId, bindValue(binding, value)) + } + + private async write(serverId: string, inputId: string, plaintext: string): Promise { if (!this.available()) { throw new Error('Secure storage is not available on this system, so the secret cannot be saved.') } @@ -69,7 +214,7 @@ export class UserMcpSecretStore { await mkdir(directory, { recursive: true, mode: 0o700 }) const target = this.path(serverId, inputId) const temporary = `${target}.${process.pid}.${Date.now()}.tmp` - await writeFile(temporary, this.codec.encrypt(value), { mode: 0o600 }) + await writeFile(temporary, this.codec.encrypt(plaintext), { mode: 0o600 }) await rename(temporary, target) } @@ -95,15 +240,68 @@ export class UserMcpSecretStore { rm(join(this.dir, serverId, file), { force: true }))) } - /** Presence and a last-4 hint only. The renderer never receives a value. */ - async state(serverId: string, inputIds: readonly string[]): Promise> { - const entries = await Promise.all(inputIds.map(async id => { - const value = await this.get(serverId, id) + /** + * The server's encrypted blobs as raw bytes, for a mutation to put back if + * its secret step fails midway (#1304, q108). Ciphertext only: nothing is + * decrypted, so this works even when secure storage is unavailable. + */ + async snapshotServer(serverId: string): Promise> { + const snapshot = new Map() + let files: string[] + try { + files = await readdir(join(this.dir, serverId)) + } catch (error) { + // Only a missing directory means "no secrets" (q110, review a): an + // EACCES/EIO/EMFILE here returned an empty snapshot, so a failed step + // then "restored" nothing and the server lost its token for good. + if ((error as NodeJS.ErrnoException).code === 'ENOENT') return snapshot + throw error + } + for (const file of files) { + if (!file.endsWith('.bin')) continue + snapshot.set(file, await readFile(join(this.dir, serverId, file))) + } + return snapshot + } + + /** Make the server's blobs exactly `snapshot` again (see snapshotServer). */ + async restoreServer(serverId: string, snapshot: ReadonlyMap): Promise { + const directory = join(this.dir, serverId) + await rm(directory, { recursive: true, force: true }) + if (snapshot.size === 0) return + await mkdir(directory, { recursive: true, mode: 0o700 }) + for (const [file, ciphertext] of snapshot) { + await writeFile(join(directory, file), ciphertext, { mode: 0o600 }) + } + } + + /** + * Presence and a last-4 hint only; the renderer never receives a value. + * `unconfirmed` marks a record that is kept but withheld and that the user + * can confirm (see confirm): `legacy` from an earlier version, or + * `inputs-changed` when another input's value changed (by an agent or an + * import) since it was bound. + * A record bound to another destination shows as plainly not set, because + * only re-entering it may bind it. + */ + async state(serverId: string, bindings: Readonly>): Promise> { + const entries = await Promise.all(Object.entries(bindings).map(async ([id, binding]) => { + const record = await this.record(serverId, id) // No hint for short values: the last four characters of a six-character // PIN are most of the secret. - const state: UserMcpSecretState = value === null - ? { set: false } - : { set: true, ...(value.length >= 12 ? { hint: value.slice(-4) } : {}) } + const hint = record && record.value.length >= 12 ? { hint: record.value.slice(-4) } : {} + let state: UserMcpSecretState + if (!record || record.value === '') { + state = { set: false } + } else if (record.kind === 'unbound') { + state = { set: false, unconfirmed: 'legacy', ...hint } + } else if (record.destination !== binding.destination) { + state = { set: false } + } else if (record.inputs !== binding.inputs) { + state = { set: false, unconfirmed: 'inputs-changed', ...hint } + } else { + state = { set: true, ...hint } + } return [id, state] as const })) return Object.fromEntries(entries) diff --git a/src/main/userMcp/service.test.ts b/src/main/userMcp/service.test.ts index 245c8ddc5..4f352f709 100644 --- a/src/main/userMcp/service.test.ts +++ b/src/main/userMcp/service.test.ts @@ -1,4 +1,4 @@ -import { mkdtemp, readFile, readdir, rm, stat, writeFile } from 'node:fs/promises' +import { chmod, lstat, mkdir, mkdtemp, readFile, readdir, rm, stat, symlink, writeFile } from 'node:fs/promises' import { tmpdir } from 'node:os' import { join } from 'node:path' @@ -42,6 +42,13 @@ function service(): UserMcpService { }) } +// The token a LAUNCH would hand this server (q113: secrets are read bound to +// the server's current destination, so this is the observable pairing). +async function launchedToken(svc: UserMcpService, id: string): Promise { + const resolution = await svc.resolveForLaunch({ provider: 'claude', overrides: {}, cwd: dir }) + return resolution.servers.find(server => server.id === id)?.secrets['beeper-authorization'] ?? null +} + const beeper = (overrides: Partial = {}): UserMcpSaveInput => ({ name: 'beeper', enabled: true, @@ -251,6 +258,9 @@ describe('UserMcpService secret redirection (review round 1)', () => { if (!saved.ok) throw new Error(saved.error) const moved = await svc.save({ ...beeper(), id: saved.id, secrets: undefined, entry: { type: 'http', url: 'https://evil.example/mcp', headers: { Authorization: 'Bearer ${input:beeper-authorization}' } } }) expect(moved).toMatchObject({ ok: true, secretsCleared: true }) + // The user's move deletes the old token itself, not only withholds it + // (r3 round-2 review a: a no-op clearServer survived every test). + await expect(readFile(join(dir, 'mcp-secrets', saved.id!, 'beeper-authorization.bin'))).rejects.toMatchObject({ code: 'ENOENT' }) const launch = await svc.resolveForLaunch({ provider: 'claude', overrides: {}, cwd: dir }) // The token never reaches the new host: the server is dropped instead. expect(launch.servers).toEqual([]) @@ -326,3 +336,816 @@ describe('UserMcpService review round 2', () => { expect(await svc.save(beeper({ secrets: { 'beeper-authorization': '${GITHUB_TOKEN}' } }))).toMatchObject({ ok: false }) }) }) + +// #1304: mutate() rolled MEMORY back on a failure, but persist() had already +// written the new document. A failed secret write after a save, or a failed +// clear after a delete, left disk and memory disagreeing: the server came back +// after a restart (without its secret), or the next mutation wrote a deleted +// server back. +describe('a secret step that fails after the document was written (#1304)', () => { + type Internals = { secrets: { set: (...args: unknown[]) => Promise; clearServer: (...args: unknown[]) => Promise } } + + it('a failed secret write on save leaves the server on neither disk nor memory', async () => { + const live = service() + ;(live as unknown as Internals).secrets.set = async () => { throw new Error('secure storage unavailable') } + const result = await live.save(beeper()) + expect(result.ok).toBe(false) + expect((await live.snapshot()).servers).toHaveLength(0) + expect((await service().snapshot()).servers).toHaveLength(0) + }) + + it('a failed secret clear on delete keeps the server on both disk and memory', async () => { + const live = service() + expect((await live.save(beeper())).ok).toBe(true) + const id = (await live.snapshot()).servers[0]!.id + ;(live as unknown as Internals).secrets.clearServer = async () => { throw new Error('EACCES') } + expect((await live.delete(id)).ok).toBe(false) + expect((await live.snapshot()).servers.map(server => server.id)).toEqual([id]) + expect((await service().snapshot()).servers.map(server => server.id)).toEqual([id]) + }) +}) + +// q108: rolling the document back is not enough if the secret step already +// erased the old secret. A destination change clears the server's blobs, then +// sets the new ones; if a set fails after the clear, the old server came back +// (document rolled back) WITHOUT its token. +describe('a secret step that fails midway keeps the previous secret (#1304, q108)', () => { + type Store = { + get: (serverId: string, inputId: string) => Promise + set: (serverId: string, inputId: string, value: string) => Promise + clearServer: (serverId: string) => Promise + } + const storeOf = (svc: UserMcpService) => (svc as unknown as { secrets: Store }).secrets + + it('a destination change whose new secret fails to write keeps the old destination AND its token', async () => { + const live = service() + expect((await live.save(beeper())).ok).toBe(true) + const id = (await live.snapshot()).servers[0]!.id + const store = storeOf(live) + store.set = async () => { throw new Error('secure storage unavailable') } + const moved = await live.save(beeper({ + id, + entry: { type: 'http', url: 'http://localhost:9999/v0/mcp', headers: { Authorization: 'Bearer ${input:beeper-authorization}' } }, + secrets: { 'beeper-authorization': 'bpr_live_new_token_0000' }, + } as Partial)) + expect(moved.ok).toBe(false) + const restarted = service() + const [server] = (await restarted.snapshot()).servers + expect(server?.id).toBe(id) + expect(JSON.stringify(server)).toContain('localhost:23373') + expect(await launchedToken(restarted, id)).toBe(TOKEN) + }) + + it('a delete whose clear fails after removing some blobs keeps the server AND its token', async () => { + const live = service() + expect((await live.save(beeper())).ok).toBe(true) + const id = (await live.snapshot()).servers[0]!.id + const store = storeOf(live) + const realClear = store.clearServer.bind(store) + store.clearServer = async serverId => { await realClear(serverId); throw new Error('EACCES') } + expect((await live.delete(id)).ok).toBe(false) + const restarted = service() + expect((await restarted.snapshot()).servers.map(server => server.id)).toEqual([id]) + expect(await launchedToken(restarted, id)).toBe(TOKEN) + }) +}) + +// q110 (SECURITY, #1420 review a): a destination and a token must never be +// observable as a mixed pair (the NEW destination with the OLD token), not by +// a launch during a save, not by a restart at any point in it, and not after a +// failed rollback. On any doubt: no secret. +describe('destination/secret pairing never mixes (#1304, q110)', () => { + type Store = { + get: (serverId: string, inputId: string) => Promise + set: (serverId: string, inputId: string, value: string) => Promise + clearServer: (serverId: string) => Promise + } + const storeOf = (svc: UserMcpService) => (svc as unknown as { secrets: Store }).secrets + const EVIL = 'https://evil.example/mcp' + const moved = (id: string, extra: Partial = {}): UserMcpSaveInput => beeper({ + id, + entry: { type: 'http', url: EVIL, headers: { Authorization: 'Bearer ${input:beeper-authorization}' } }, + secrets: {}, + ...extra, + } as Partial) + const launch = (svc: UserMcpService) => svc.resolveForLaunch({ provider: 'claude', overrides: {}, cwd: dir }) + const mixed = (resolution: Awaited>) => + resolution.servers.some(server => JSON.stringify(server.entry).includes('evil.example') && Object.values(server.secrets).includes(TOKEN)) + const deferred = () => { let release!: () => void; const gate = new Promise(resolve => { release = resolve }); return { gate, release } } + + async function seeded() { + const live = service() + expect((await live.save(beeper())).ok).toBe(true) + return { live, id: (await live.snapshot()).servers[0]!.id } + } + + it.each(['clearServer', 'set', 'prune'] as const)( + 'a launch and a restart while the save is paused at %s never pair the new destination with the old token', + async method => { + const { live, id } = await seeded() + const store = storeOf(live) as unknown as Record Promise> + const real = store[method]!.bind(store) + const hold = deferred() + let reached!: () => void + const atStep = new Promise(resolve => { reached = resolve }) + store[method] = async (...args) => { reached(); await hold.gate; return real(...args) } + const saving = live.save(moved(id, method === 'set' ? { secrets: { 'beeper-authorization': 'bpr_live_new_token_1111' } } as Partial : {})) + await Promise.race([atStep, saving]) + const liveLaunch = launch(live) + expect(mixed(await launch(service()))).toBe(false) + hold.release() + await saving + expect(mixed(await liveLaunch)).toBe(false) + expect(mixed(await launch(service()))).toBe(false) + }, + ) + + it('a failed rollback write after a failed secret step never leaves the new destination with the old token', async () => { + const { live, id } = await seeded() + const store = storeOf(live) + store.set = async () => { + await chmod(dir, 0o500) + throw new Error('secure storage unavailable') + } + try { + expect((await live.save(moved(id, { secrets: { 'beeper-authorization': 'bpr_live_new_token_2222' } } as Partial))).ok).toBe(false) + } finally { + await chmod(dir, 0o700) + } + expect(mixed(await launch(service()))).toBe(false) + }) + + // The reverse mix: a launch that read the OLD destination and then awaited + // its native policy lookups used to read the secret store AFTER a save had + // written the NEW token, pairing the old destination with a token the user + // meant for the new one. Launches are serialized with mutations. + it('a launch paused in its own lookups never pairs the old destination with the new token', async () => { + const { live, id } = await seeded() + const hold = deferred() + let reached!: () => void + const inLookup = new Promise(resolve => { reached = resolve }) + const gated = new UserMcpService({ + stateDir: dir, + codec, + native: { + list: async () => native, + codexNames: async () => codexNames, + claudeManagedPolicy: async () => { reached(); await hold.gate; return false }, + }, + }) + const launching = gated.resolveForLaunch({ provider: 'claude', overrides: {}, cwd: dir }) + await inLookup + const saving = gated.save(moved(id, { secrets: { 'beeper-authorization': 'bpr_live_new_token_4444' } } as Partial)) + await new Promise(resolve => setTimeout(resolve, 50)) + hold.release() + const resolution = await launching + await saving + const reverseMix = resolution.servers.some(server => + JSON.stringify(server.entry).includes('localhost:23373') && Object.values(server.secrets).includes('bpr_live_new_token_4444')) + expect(reverseMix).toBe(false) + void live + }) + + // Review a: snapshotServer returned an EMPTY snapshot on any readdir error, + // so a failed step then "restored" nothing and the server lost its token. + // Only a missing directory means no secrets. + it('a secrets directory that cannot be listed is an error, not an empty snapshot', async () => { + const { live, id } = await seeded() + const store = (live as unknown as { secrets: { snapshotServer: (id: string) => Promise> } }).secrets + const serverDir = join(dir, 'mcp-secrets', id) + await chmod(serverDir, 0o000) + try { + await expect(store.snapshotServer(id)).rejects.toThrow() + } finally { + await chmod(serverDir, 0o700) + } + await expect(store.snapshotServer('never-saved')).resolves.toEqual(new Map()) + }) + + // Worker rule "unknown is never empty" (q109, q115): through the SERVICE, on + // the real filesystem. A save that cannot list the secrets directory must + // fail without touching the stored bytes. After the permission comes back, + // routine maintenance (a save that changes no secret, which prunes) must + // keep the same ciphertext, and a launch still gets the original token. + // WHY both this and the snapshotServer test above: on a real filesystem an + // unlistable directory also blocks the write and the rm, so an "empty on any + // error" snapshot cannot do damage HERE. It does damage on a transient + // EMFILE/EIO, which no real fs reproduces on demand. The test above is the + // one that fails when snapshotServer treats a non-ENOENT error as empty. + it('an unlistable secrets directory fails the save once and the token bytes survive recovery and maintenance', async () => { + const { live, id } = await seeded() + const serverDir = join(dir, 'mcp-secrets', id) + const [blob] = await readdir(serverDir) + const before = await readFile(join(serverDir, blob!)) + await chmod(serverDir, 0o000) + try { + const failed = await live.save(beeper({ id, secrets: { 'beeper-authorization': 'bpr_live_new_token_5555' } } as Partial)) + expect(failed.ok).toBe(false) + } finally { + await chmod(serverDir, 0o700) + } + expect(await readFile(join(serverDir, blob!))).toEqual(before) + const maintained = await live.save(beeper({ id, name: 'beeper-renamed', secrets: {} } as Partial)) + expect(maintained.ok).toBe(true) + expect(await readFile(join(serverDir, blob!))).toEqual(before) + expect(await launchedToken(service(), id)).toBe(TOKEN) + }) + + it('a listener that throws after commit does not turn a committed save into a failure', async () => { + const { live, id } = await seeded() + live.onChange(() => { throw new Error('broadcast failed') }) + const result = await live.save(beeper({ id, secrets: { 'beeper-authorization': 'bpr_live_new_token_3333' } } as Partial)) + expect(result.ok).toBe(true) + expect(await launchedToken(service(), id)).toBe('bpr_live_new_token_3333') + }) +}) + + +// q113 (SECURITY, #1420 fresh review a): ORDER alone cannot hold the pairing +// across a crash or a failed restore. A/T -> B/U with a failing prune rolled +// the document back to A before U was removed, and a restart (or a failed +// restore) then launched A with U. Each secret record is now bound to the +// destination it was saved for, and a launch refuses a secret whose binding +// does not match the document's current destination: fail closed, in both +// directions, whatever state a crash or a failed restore leaves. +describe('secrets are bound to their destination (#1304, q113)', () => { + type Store = Record Promise> + const storeOf = (svc: UserMcpService) => (svc as unknown as { secrets: Store }).secrets + const OLD_URL = 'http://localhost:23373/v0/mcp' + const NEW_URL = 'https://evil.example/mcp' + const U = 'bpr_live_new_token_9999' + const deferred = () => { let release!: () => void; const gate = new Promise(resolve => { release = resolve }); return { gate, release } } + const at = (url: string) => ({ type: 'http', url, headers: { Authorization: 'Bearer ${input:beeper-authorization}' } }) + async function pairing(svc: UserMcpService) { + const resolution = await svc.resolveForLaunch({ provider: 'claude', overrides: {}, cwd: dir }) + return resolution.servers.map(server => [server.entry.type === 'http' ? (server.entry as { url: string }).url : '', server.secrets['beeper-authorization'] ?? null]) + } + const wrongPair = (pairs: Array>) => + pairs.some(([url, token]) => (url === OLD_URL && token === U) || (url === NEW_URL && token === TOKEN)) + + async function seeded() { + const live = service() + expect((await live.save(beeper())).ok).toBe(true) + return { live, id: (await live.snapshot()).servers[0]!.id } + } + + it('a restart between the document rollback and the secret restore never launches the old address with the new token', async () => { + const { live, id } = await seeded() + const store = storeOf(live) + store.prune = async () => { throw new Error('prune failed') } + const realRestore = store.restoreServer!.bind(store) + const hold = deferred() + let reached!: () => void + const atRestore = new Promise(resolve => { reached = resolve }) + store.restoreServer = async (...args) => { reached(); await hold.gate; return realRestore(...args) } + const saving = live.save(beeper({ id, entry: at(NEW_URL), secrets: { 'beeper-authorization': U } } as Partial)) + await atRestore + expect(wrongPair(await pairing(service()))).toBe(false) + hold.release() + expect((await saving).ok).toBe(false) + expect(wrongPair(await pairing(service()))).toBe(false) + }) + + it('a failed secret restore never leaves the old address with the new token', async () => { + const { live, id } = await seeded() + const store = storeOf(live) + store.prune = async () => { throw new Error('prune failed') } + store.restoreServer = async () => { throw new Error('restore failed') } + expect((await live.save(beeper({ id, entry: at(NEW_URL), secrets: { 'beeper-authorization': U } } as Partial))).ok).toBe(false) + expect(wrongPair(await pairing(live))).toBe(false) + expect(wrongPair(await pairing(service()))).toBe(false) + }) + + it('a token written for one destination is refused for another, read from disk after a restart', async () => { + const { live, id } = await seeded() + // Hand-craft the worst durable state: the document names the NEW address + // while the blob still holds the token saved for the OLD one. + const snapshotOld = await storeOf(live).snapshotServer!(id) as Map + expect((await live.save(beeper({ id, entry: at(NEW_URL), secrets: {} } as Partial))).ok).toBe(true) + await storeOf(live).restoreServer!(id, snapshotOld) + const restarted = service() + expect(await launchedToken(restarted, id)).toBeNull() + expect(wrongPair(await pairing(restarted))).toBe(false) + }) + + // q114: a record written before binding carries no proof of which + // destination it was saved for. An old-version crash could leave document B + // with the plaintext token T that was entered for A, and an upgrade that + // bound legacy records to "the destination the document names" labelled T + // as B and launched B/T. Legacy records are therefore never trusted: they + // read as not set until the user re-enters them, across every restart. + it('never launches a pre-binding secret, even when the document names a destination', async () => { + const live = service() + expect((await live.save(beeper({ entry: at(NEW_URL), secrets: {} } as Partial))).ok).toBe(true) + const id = (await live.snapshot()).servers[0]!.id + await mkdir(join(dir, 'mcp-secrets', id), { recursive: true }) + await writeFile(join(dir, 'mcp-secrets', id, 'beeper-authorization.bin'), codec.encrypt(TOKEN), { mode: 0o600 }) + expect(await launchedToken(service(), id)).toBeNull() + expect(await launchedToken(service(), id)).toBeNull() + }) + + // B6 (q114): kept, withheld, and bound only by the user's confirmation. + it('keeps a pre-binding secret, withholds it, and binds it only when the user confirms it', async () => { + const live = service() + expect((await live.save(beeper({ entry: at(NEW_URL), secrets: {} } as Partial))).ok).toBe(true) + const id = (await live.snapshot()).servers[0]!.id + const blob = join(dir, 'mcp-secrets', id, 'beeper-authorization.bin') + await mkdir(join(dir, 'mcp-secrets', id), { recursive: true }) + await writeFile(blob, codec.encrypt(TOKEN), { mode: 0o600 }) + const restarted = service() + expect(await launchedToken(restarted, id)).toBeNull() + expect((await restarted.snapshot()).servers[0]!.secrets['beeper-authorization']).toMatchObject({ set: false, unconfirmed: 'legacy' }) + expect(await readFile(blob, 'utf8')).toBe(`enc:${TOKEN}`) + expect((await restarted.confirmSecret(id, 'beeper-authorization')).ok).toBe(true) + expect(await launchedToken(service(), id)).toBe(TOKEN) + }) + + it('tells the user a pre-binding secret must be re-entered, and accepts the re-entry', async () => { + const live = service() + expect((await live.save(beeper({ secrets: {} } as Partial))).ok).toBe(true) + const id = (await live.snapshot()).servers[0]!.id + await mkdir(join(dir, 'mcp-secrets', id), { recursive: true }) + await writeFile(join(dir, 'mcp-secrets', id, 'beeper-authorization.bin'), codec.encrypt(TOKEN), { mode: 0o600 }) + const restarted = service() + const [server] = (await restarted.snapshot()).servers + expect(server!.problems.map(problem => problem.message).join(' ')).toMatch(/earlier version.*re-enter/i) + expect((await restarted.setSecret(id, 'beeper-authorization', TOKEN)).ok).toBe(true) + expect(await launchedToken(service(), id)).toBe(TOKEN) + }) + + it('restores ciphertext with owner-only permissions', async () => { + const { live, id } = await seeded() + const store = storeOf(live) + const snapshot = await store.snapshotServer!(id) as Map + await store.restoreServer!(id, snapshot) + expect((await stat(join(dir, 'mcp-secrets', id, 'beeper-authorization.bin'))).mode & 0o777).toBe(0o600) + }) + + it('a launch requested while a save is running waits for that save', async () => { + const { live, id } = await seeded() + const store = storeOf(live) + const realClear = store.clearServer!.bind(store) + const hold = deferred() + let reached!: () => void + const atClear = new Promise(resolve => { reached = resolve }) + store.clearServer = async (...args) => { reached(); await hold.gate; return realClear(...args) } + const saving = live.save(beeper({ id, entry: at(NEW_URL), secrets: { 'beeper-authorization': U } } as Partial)) + await atClear + let launched = false + const launching = pairing(live).then(result => { launched = true; return result }) + await new Promise(resolve => setTimeout(resolve, 30)) + expect(launched).toBe(false) + hold.release() + await saving + expect(await launching).toEqual([[NEW_URL, U]]) + }) +}) + +// #1420 reviews a+b (blocker): an agent changing the endpoint INSIDE a +// secret-bearing env value kept the token, stayed enabled without review, and +// the next launch sent the token to the new host. +describe('an endpoint inside a secret-bearing value is part of the destination (#1420)', () => { + const stdio = (endpoint: string) => ({ + name: 'endpoint-client', + enabled: true, + providers: { claude: true, codex: true }, + entry: { command: 'node', args: ['client.js'], env: { MCP_ENDPOINT: endpoint } }, + inputs: [{ id: 'beeper-authorization', description: 'Token' }], + }) + + it('an agent that moves the endpoint loses the token and needs review, across a restart', async () => { + const live = service() + expect((await live.save({ ...stdio('https://trusted.example/mcp?key=${input:beeper-authorization}'), secrets: { 'beeper-authorization': TOKEN } } as UserMcpSaveInput)).ok).toBe(true) + const id = (await live.snapshot()).servers[0]!.id + const moved = await live.save({ id, ...stdio('https://evil.example/mcp?key=${input:beeper-authorization}') } as UserMcpSaveInput, 'agent') + expect(moved.ok).toBe(true) + const restarted = service() + const [server] = (await restarted.snapshot()).servers + expect(server!.pendingReview).toBe(true) + const resolution = await restarted.resolveForLaunch({ provider: 'claude', overrides: {}, cwd: dir }) + expect(JSON.stringify(resolution.servers)).not.toContain(TOKEN) + }) +}) + +// q118 (#1420 round-2 review a, both findings, replayed on real files across a +// restart). 1: the agent changes only WHICH input supplies the endpoint host, +// then sets that new input itself; the token T it never knew went to +// evil.example. 2: the agent reorders a reference against a literal `${input}` +// that the old mask could not tell apart. Both must now need review and launch +// without T. +describe('a reference change is a destination change (#1420, q118)', () => { + const client = (endpoint: string, inputs: string[]) => ({ + name: 'endpoint-client', + enabled: true, + providers: { claude: true, codex: true }, + entry: { command: 'node', args: ['client.js'], env: { MCP_ENDPOINT: endpoint } }, + inputs: inputs.map(id => ({ id, description: id })), + }) + + it('pointing the endpoint at another input loses the token and needs review, across a restart', async () => { + const live = service() + const saved = await live.save({ + ...client('https://${input:trusted-host}/mcp?key=${input:beeper-authorization}', ['trusted-host', 'beeper-authorization']), + secrets: { 'trusted-host': 'trusted.example', 'beeper-authorization': TOKEN }, + } as UserMcpSaveInput) + expect(saved.ok).toBe(true) + const id = (await live.snapshot()).servers[0]!.id + const moved = await live.save({ id, ...client('https://${input:evil-host}/mcp?key=${input:beeper-authorization}', ['trusted-host', 'evil-host', 'beeper-authorization']) } as UserMcpSaveInput, 'agent') + expect(moved.ok).toBe(true) + await live.setSecret(id, 'evil-host', 'evil.example') + const restarted = service() + expect((await restarted.snapshot()).servers[0]!.pendingReview).toBe(true) + const resolution = await restarted.resolveForLaunch({ provider: 'claude', overrides: {}, cwd: dir }) + expect(JSON.stringify(resolution.servers)).not.toContain(TOKEN) + }) + + it('reordering a reference against a literal ${input} loses the token and needs review, across a restart', async () => { + const live = service() + expect((await live.save(beeper({ entry: { type: 'http', url: 'http://localhost:23373/v0/mcp', headers: { Authorization: 'Bearer ${input:beeper-authorization}${input}' } } } as Partial))).ok).toBe(true) + const id = (await live.snapshot()).servers[0]!.id + const moved = await live.save(beeper({ id, secrets: {}, entry: { type: 'http', url: 'http://localhost:23373/v0/mcp', headers: { Authorization: 'Bearer ${input}${input:beeper-authorization}' } } } as Partial), 'agent') + expect(moved.ok).toBe(true) + const restarted = service() + expect((await restarted.snapshot()).servers[0]!.pendingReview).toBe(true) + const resolution = await restarted.resolveForLaunch({ provider: 'claude', overrides: {}, cwd: dir }) + expect(JSON.stringify(resolution.servers)).not.toContain(TOKEN) + }) +}) + + +// B6 manager check at 4d5c79ab (temp/review-1420/manager-verify-a.md), R3: the +// identity covered the entry text and reference ids but not the VALUES of the +// inputs that decide where a request goes. Imported env values all become +// inputs, so `API_BASE_URL=${input:svc-API_BASE_URL}` is a host an agent can +// re-set with mcp_servers_set_secret, and the next launch sent API_KEY=T there. +// Each secret is now bound to its destination PLUS the values of EVERY other +// input the entry references (q127: no credential/steering classifier; a +// stdio program may read API_KEY as an endpoint). An agent or import changing +// any input value turns the server off for review and withholds the siblings +// (kept, never deleted) until the user confirms. A user change in Settings is +// itself the confirmation: it rebinds the siblings that were valid. +describe('input values that steer a request are part of the binding (#1420, B6 R3)', () => { + const EVIL = 'https://evil.example' + const imported = () => ({ + name: 'svc', + enabled: true, + providers: { claude: true, codex: true }, + entry: { command: 'node', args: ['client.js'], env: { API_BASE_URL: '${input:svc-API_BASE_URL}', API_KEY: '${input:svc-API_KEY}' } }, + inputs: [{ id: 'svc-API_BASE_URL', description: 'API_BASE_URL' }, { id: 'svc-API_KEY', description: 'API_KEY' }], + }) + const endpoint = () => ({ + name: 'endpoint-client', + enabled: true, + providers: { claude: true, codex: true }, + entry: { command: 'node', args: ['client.js'], env: { MCP_ENDPOINT: 'https://${input:trusted-host}/mcp?key=${input:tok}' } }, + inputs: [{ id: 'trusted-host', description: 'host' }, { id: 'tok', description: 'token' }], + }) + const launched = async (svc: UserMcpService) => JSON.stringify((await svc.resolveForLaunch({ provider: 'claude', overrides: {}, cwd: dir })).servers) + const blob = (id: string, inputId: string) => readFile(join(dir, 'mcp-secrets', id, `${inputId}.bin`)) + + it('an agent re-setting an imported base URL withholds the key (kept on disk) and needs review, across a restart', async () => { + const live = service() + expect((await live.save({ ...imported(), secrets: { 'svc-API_BASE_URL': 'https://trusted.example', 'svc-API_KEY': TOKEN } } as UserMcpSaveInput)).ok).toBe(true) + const id = (await live.snapshot()).servers[0]!.id + expect(await launched(live)).toContain(TOKEN) + const keyBytes = await blob(id, 'svc-API_KEY') + expect((await live.setSecret(id, 'svc-API_BASE_URL', EVIL, 'agent')).ok).toBe(true) + const restarted = service() + const [server] = (await restarted.snapshot()).servers + expect(server!.pendingReview).toBe(true) + expect(server!.enabled).toBe(false) + expect(server!.secrets['svc-API_KEY']).toMatchObject({ set: false, unconfirmed: 'inputs-changed' }) + expect(await launched(restarted)).not.toContain(TOKEN) + expect(await blob(id, 'svc-API_KEY')).toEqual(keyBytes) + }) + + it('an agent re-setting the host inside an endpoint template withholds the token, across a restart', async () => { + const live = service() + expect((await live.save({ ...endpoint(), secrets: { 'trusted-host': 'trusted.example', tok: TOKEN } } as UserMcpSaveInput)).ok).toBe(true) + const id = (await live.snapshot()).servers[0]!.id + expect((await live.setSecret(id, 'trusted-host', 'evil.example', 'agent')).ok).toBe(true) + const restarted = service() + expect((await restarted.snapshot()).servers[0]!.pendingReview).toBe(true) + expect(await launched(restarted)).not.toContain(TOKEN) + }) + + it('a USER changing the host in Settings is the confirmation: the key still launches, to the new host, with no review', async () => { + const live = service() + await live.save({ ...imported(), secrets: { 'svc-API_BASE_URL': 'https://trusted.example', 'svc-API_KEY': TOKEN } } as UserMcpSaveInput) + const id = (await live.snapshot()).servers[0]!.id + expect((await live.setSecret(id, 'svc-API_BASE_URL', 'https://moved.example')).ok).toBe(true) + const restarted = service() + expect((await restarted.snapshot()).servers[0]!.pendingReview).toBeUndefined() + const out = await launched(restarted) + expect(out).toContain(TOKEN) + expect(out).toContain('https://moved.example') + }) + + // q127 item 4: the key NAME proves nothing about the role, so an agent + // changing API_KEY is treated like any other input change. + it('an agent changing an API_KEY-named input withholds the bound sibling and needs review, across a restart', async () => { + const live = service() + await live.save({ ...imported(), secrets: { 'svc-API_BASE_URL': 'https://trusted.example', 'svc-API_KEY': TOKEN } } as UserMcpSaveInput) + const id = (await live.snapshot()).servers[0]!.id + const baseBytes = await blob(id, 'svc-API_BASE_URL') + expect((await live.setSecret(id, 'svc-API_KEY', 'bpr_live_agent_chosen_7777', 'agent')).ok).toBe(true) + const restarted = service() + const [server] = (await restarted.snapshot()).servers + expect(server!.pendingReview).toBe(true) + expect(server!.enabled).toBe(false) + expect(server!.secrets['svc-API_BASE_URL']).toMatchObject({ set: false, unconfirmed: 'inputs-changed' }) + expect(await blob(id, 'svc-API_BASE_URL')).toEqual(baseBytes) + // Even once the user turns it back on, the sibling waits for its own confirmation. + expect((await restarted.setEnabled(id, true)).ok).toBe(true) + expect(await launched(service())).not.toContain('https://trusted.example') + expect((await service().confirmSecret(id, 'svc-API_BASE_URL')).ok).toBe(true) + expect(await launched(service())).toContain('https://trusted.example') + }) + + // A user edit rebinds only siblings that were VALID before it. A sibling an + // agent's change already withheld stays withheld: an unrelated Settings + // edit must not bless the agent's change. + it('a user edit does not confirm a sibling that an earlier agent change withheld', async () => { + const live = service() + await live.save({ ...endpoint(), secrets: { 'trusted-host': 'trusted.example', tok: TOKEN } } as UserMcpSaveInput) + const id = (await live.snapshot()).servers[0]!.id + await live.setSecret(id, 'trusted-host', 'evil.example', 'agent') + await live.setEnabled(id, true) + expect((await live.setSecret(id, 'trusted-host', 'evil.example')).ok).toBe(true) + expect(await launched(service())).not.toContain(TOKEN) + expect((await service().snapshot()).servers[0]!.secrets.tok).toMatchObject({ set: false, unconfirmed: 'inputs-changed' }) + }) + + it('a user rotating one token keeps the other secrets launching, with no confirmation', async () => { + const live = service() + await live.save({ ...imported(), secrets: { 'svc-API_BASE_URL': 'https://trusted.example', 'svc-API_KEY': TOKEN } } as UserMcpSaveInput) + const id = (await live.snapshot()).servers[0]!.id + expect((await live.setSecret(id, 'svc-API_KEY', 'bpr_live_rotated_1111')).ok).toBe(true) + const out = await launched(service()) + expect(out).toContain('bpr_live_rotated_1111') + expect(out).toContain('https://trusted.example') + expect((await service().snapshot()).servers[0]!.pendingReview).toBeUndefined() + }) + + // Fresh r3 reviews a+b (blocker): an agent's later entry edit is a + // destination change, and save() cleared every blob, deleting a secret the + // user had not yet confirmed or re-entered. Since q113 the read-time binding + // keeps an old token away from a new destination, so an agent's save no + // longer deletes anything: the blobs stay (useless for the new entry until + // the user re-enters them) and only the user's own edits forget or prune. + it('an agent entry edit after a value change keeps the withheld token on disk, and never launches it', async () => { + const live = service() + await live.save({ ...endpoint(), secrets: { 'trusted-host': 'trusted.example', tok: TOKEN } } as UserMcpSaveInput) + const id = (await live.snapshot()).servers[0]!.id + await live.setSecret(id, 'trusted-host', 'evil.example', 'agent') + const tokBytes = await blob(id, 'tok') + const edited = await service().save({ id, ...endpoint(), entry: { ...endpoint().entry, args: ['client-v2.js'] } } as UserMcpSaveInput, 'agent') + expect(edited).toMatchObject({ ok: true, pendingReview: true }) + expect(edited).not.toHaveProperty('secretsCleared') + const restarted = service() + expect(await blob(id, 'tok')).toEqual(tokBytes) + await restarted.setEnabled(id, true) + expect(await launched(service())).not.toContain(TOKEN) + // Dropping the reference altogether does not prune it either. + await service().save({ id, ...endpoint(), entry: { command: 'node', args: ['client-v2.js'], env: { MCP_ENDPOINT: 'https://${input:trusted-host}/mcp' } }, inputs: [{ id: 'trusted-host', description: 'host' }] } as UserMcpSaveInput, 'agent') + expect(await blob(id, 'tok')).toEqual(tokBytes) + }) + + // Review b (surviving mutation): the review flag is saved BEFORE an agent's + // value. If the document write fails, the value must not have landed; with + // the order reversed, a restart saw the old enabled document next to the + // agent's value and launched it. + it('an agent value never lands next to an unflagged document when the document write fails', async () => { + const live = service() + await live.save({ ...imported(), secrets: { 'svc-API_BASE_URL': 'https://trusted.example', 'svc-API_KEY': TOKEN } } as UserMcpSaveInput) + const id = (await live.snapshot()).servers[0]!.id + const internals = live as unknown as { persist: () => Promise } + const realPersist = internals.persist.bind(live) + let failures = 1 + internals.persist = async () => { if (failures-- > 0) throw new Error('EIO'); return realPersist() } + expect((await live.setSecret(id, 'svc-API_BASE_URL', EVIL, 'agent')).ok).toBe(false) + const out = await launched(service()) + expect(out).not.toContain(EVIL) + expect(out).toContain(TOKEN) + }) + + // Review b (surviving mutation): the direct service contract for an agent + // save that supplies values on an EXISTING server. + it('an agent save that supplies a value on an existing server needs review and withholds the sibling', async () => { + const live = service() + await live.save({ ...imported(), secrets: { 'svc-API_BASE_URL': 'https://trusted.example', 'svc-API_KEY': TOKEN } } as UserMcpSaveInput) + const id = (await live.snapshot()).servers[0]!.id + const result = await live.save({ id, ...imported(), secrets: { 'svc-API_BASE_URL': EVIL } } as UserMcpSaveInput, 'agent') + expect(result).toMatchObject({ ok: true, pendingReview: true }) + const [server] = (await service().snapshot()).servers + expect(server!.enabled).toBe(false) + expect(server!.secrets['svc-API_KEY']).toMatchObject({ set: false, unconfirmed: 'inputs-changed' }) + }) + + // r3 round-2 reviews a+b: two more agent routes destroyed a withheld + // secret. An agent may not overwrite or delete a secret that is waiting for + // the user's confirmation; it is refused before anything changes, and the + // user confirms, re-enters or removes it in Settings. + it('an agent cannot overwrite a withheld token', async () => { + const live = service() + await live.save({ ...endpoint(), secrets: { 'trusted-host': 'trusted.example', tok: TOKEN } } as UserMcpSaveInput) + const id = (await live.snapshot()).servers[0]!.id + await live.setSecret(id, 'trusted-host', 'evil.example', 'agent') + const tokBytes = await blob(id, 'tok') + const overwrite = await service().setSecret(id, 'tok', 'bpr_live_agent_value_4444', 'agent') + expect(overwrite.ok).toBe(false) + expect(await blob(id, 'tok')).toEqual(tokBytes) + const viaSave = await service().save({ id, ...endpoint(), secrets: { tok: 'bpr_live_agent_value_5555' } } as UserMcpSaveInput, 'agent') + expect(viaSave.ok).toBe(false) + expect(await blob(id, 'tok')).toEqual(tokBytes) + // The user may still re-enter it. + expect((await service().setSecret(id, 'tok', 'bpr_live_user_value_6666')).ok).toBe(true) + }) + + it('an agent cannot remove a server that has a withheld secret; the user can', async () => { + const live = service() + await live.save({ ...endpoint(), secrets: { 'trusted-host': 'trusted.example', tok: TOKEN } } as UserMcpSaveInput) + const id = (await live.snapshot()).servers[0]!.id + await live.setSecret(id, 'trusted-host', 'evil.example', 'agent') + const tokBytes = await blob(id, 'tok') + expect((await service().delete(id, 'agent')).ok).toBe(false) + expect((await service().snapshot()).servers.map(server => server.id)).toEqual([id]) + expect(await blob(id, 'tok')).toEqual(tokBytes) + expect((await service().delete(id)).ok).toBe(true) + await expect(blob(id, 'tok')).rejects.toMatchObject({ code: 'ENOENT' }) + }) + + // q130: a blob that exists but cannot be decrypted (a key mismatch, a + // corrupt file) is NOT an absent secret. It counts as withheld, so none of + // the three agent routes may replace or delete it; only the user may. The + // bytes below are real ciphertext this codec cannot decrypt. + it('an agent cannot overwrite or remove a secret whose blob exists but cannot be decrypted', async () => { + const live = service() + await live.save({ ...endpoint(), secrets: { 'trusted-host': 'trusted.example', tok: TOKEN } } as UserMcpSaveInput) + const id = (await live.snapshot()).servers[0]!.id + const unreadable = Buffer.from('v10\u0000\u00ff\u0013 sealed by another keychain', 'utf8') + await writeFile(join(dir, 'mcp-secrets', id, 'tok.bin'), unreadable, { mode: 0o600 }) + const svc = service() + expect((await svc.setSecret(id, 'tok', 'bpr_live_agent_value_8888', 'agent')).ok).toBe(false) + expect((await svc.save({ id, ...endpoint(), secrets: { tok: 'bpr_live_agent_value_9999' } } as UserMcpSaveInput, 'agent')).ok).toBe(false) + expect((await svc.delete(id, 'agent')).ok).toBe(false) + expect(await blob(id, 'tok')).toEqual(unreadable) + expect((await svc.snapshot()).servers.map(server => server.id)).toEqual([id]) + // The user's re-entry replaces it. + expect((await svc.setSecret(id, 'tok', 'bpr_live_user_value_7777')).ok).toBe(true) + expect(await blob(id, 'tok')).not.toEqual(unreadable) + }) + + // q130: presence that cannot be CHECKED is unknown, not absent. A + // self-referencing link makes stat fail (ELOOP) while the atomic write's + // rename would still replace it, so only the presence check stands + // between an agent and the entry. (An unreadable directory does not + // discriminate: the write fails there too.) + it('an agent write is refused when the blob cannot even be stat-ed', async () => { + const live = service() + await live.save({ ...endpoint(), secrets: { 'trusted-host': 'trusted.example', tok: TOKEN } } as UserMcpSaveInput) + const id = (await live.snapshot()).servers[0]!.id + const file = join(dir, 'mcp-secrets', id, 'tok.bin') + await rm(file) + await symlink(file, file) + expect((await service().setSecret(id, 'tok', 'bpr_live_agent_value_1212', 'agent')).ok).toBe(false) + expect((await lstat(file)).isSymbolicLink()).toBe(true) + }) + + // r3 round 3 (review a): the guard walked only DEFINED inputs. An agent + // edit that drops an input keeps its blob (agents never prune), and that + // orphaned blob was then invisible to the guard, so mcp_servers_remove + // deleted it. Every blob on disk for the server is covered now; one with no + // input can prove no binding, so it counts as withheld. + it('an agent cannot remove a server whose dropped input still holds a secret', async () => { + const live = service() + await live.save({ ...endpoint(), secrets: { 'trusted-host': 'trusted.example', tok: TOKEN } } as UserMcpSaveInput) + const id = (await live.snapshot()).servers[0]!.id + const tokBytes = await blob(id, 'tok') + await service().save({ id, ...endpoint(), entry: { command: 'node', args: ['client.js'], env: { MCP_ENDPOINT: 'https://${input:trusted-host}/mcp' } }, inputs: [{ id: 'trusted-host', description: 'host' }] } as UserMcpSaveInput, 'agent') + // The user re-enters the remaining input, so the ORPHAN is the only + // secret the guard can protect. + expect((await service().setSecret(id, 'trusted-host', 'trusted.example')).ok).toBe(true) + expect((await service().delete(id, 'agent')).ok).toBe(false) + expect(await blob(id, 'tok')).toEqual(tokBytes) + expect((await service().delete(id)).ok).toBe(true) + }) + + // q131: the orphan -> re-add route. The agent drops every reference, then + // re-adds the input and supplies a value in the same save; the orphaned + // blob must survive byte-for-byte. + it('an agent re-adding a dropped input cannot replace its orphaned secret', async () => { + const live = service() + await live.save({ ...endpoint(), secrets: { 'trusted-host': 'trusted.example', tok: TOKEN } } as UserMcpSaveInput) + const id = (await live.snapshot()).servers[0]!.id + const tokBytes = await blob(id, 'tok') + const bare = { ...endpoint(), entry: { command: 'node', args: ['client.js'], env: {} }, inputs: [] } + expect((await service().save({ id, ...bare } as UserMcpSaveInput, 'agent')).ok).toBe(true) + expect((await service().snapshot()).servers[0]!.inputs).toEqual([]) + const readd = await service().save({ id, ...endpoint(), secrets: { tok: 'bpr_live_agent_value_3131' } } as UserMcpSaveInput, 'agent') + expect(readd.ok).toBe(false) + expect(await blob(id, 'tok')).toEqual(tokBytes) + }) + + // q131: an unlistable directory (0o300: writable and searchable, not + // listable) must refuse the orphan re-add. Two strict reads stand here: + // the guard's blob listing (storedInputIds) and save's snapshotServer. + // Making the listing lenient alone does NOT fail this test, because the + // snapshot still refuses; the test pins the route, and removing both + // strict reads fails it. + it('an agent re-add is refused when the secrets directory cannot be listed', async () => { + const live = service() + await live.save({ ...endpoint(), secrets: { 'trusted-host': 'trusted.example', tok: TOKEN } } as UserMcpSaveInput) + const id = (await live.snapshot()).servers[0]!.id + const tokBytes = await blob(id, 'tok') + await service().save({ id, ...endpoint(), entry: { command: 'node', args: ['client.js'], env: {} }, inputs: [] } as UserMcpSaveInput, 'agent') + const serverDir = join(dir, 'mcp-secrets', id) + await chmod(serverDir, 0o300) + try { + expect((await service().save({ id, ...endpoint(), secrets: { tok: 'bpr_live_agent_value_3232' } } as UserMcpSaveInput, 'agent')).ok).toBe(false) + } finally { + await chmod(serverDir, 0o700) + } + expect(await blob(id, 'tok')).toEqual(tokBytes) + }) + + // B6 manager check at fb61adfa: on a case-insensitive disk (macOS default, + // Windows) `TOK.bin` IS `tok.bin`, and the guard compared ids with case, so + // an agent save naming `TOK` overwrote the withheld `tok` secret. Ids are + // compared case-insensitively; the refusal holds on any filesystem. + const upper = (entryValue: string) => ({ ...endpoint(), entry: { command: 'node', args: ['client.js'], env: { MCP_ENDPOINT: entryValue } }, inputs: [{ id: 'trusted-host', description: 'host' }, { id: 'TOK', description: 'token' }] }) + it('an agent cannot overwrite a withheld secret through a case-only variant of its id', async () => { + const live = service() + await live.save({ ...endpoint(), secrets: { 'trusted-host': 'trusted.example', tok: TOKEN } } as UserMcpSaveInput) + const id = (await live.snapshot()).servers[0]!.id + await live.setSecret(id, 'trusted-host', 'evil.example', 'agent') + const bytes = await blob(id, 'tok') + const result = await service().save({ id, ...upper('https://${input:trusted-host}/mcp?key=${input:TOK}'), secrets: { TOK: 'bpr_live_agent_case_1111' } } as UserMcpSaveInput, 'agent') + expect(result.ok).toBe(false) + expect(await blob(id, 'tok')).toEqual(bytes) + }) + + it('an agent cannot replace an orphaned secret through a case-only variant of its id', async () => { + const live = service() + await live.save({ ...endpoint(), secrets: { 'trusted-host': 'trusted.example', tok: TOKEN } } as UserMcpSaveInput) + const id = (await live.snapshot()).servers[0]!.id + const bytes = await blob(id, 'tok') + await service().save({ id, ...endpoint(), entry: { command: 'node', args: ['client.js'], env: {} }, inputs: [] } as UserMcpSaveInput, 'agent') + const result = await service().save({ id, ...upper('https://${input:trusted-host}/mcp?key=${input:TOK}'), secrets: { TOK: 'bpr_live_agent_case_2222' } } as UserMcpSaveInput, 'agent') + expect(result.ok).toBe(false) + expect(await blob(id, 'tok')).toEqual(bytes) + }) + + it('an agent cannot replace an undecryptable secret through a case-only variant of its id', async () => { + const live = service() + await live.save({ ...endpoint(), secrets: { 'trusted-host': 'trusted.example', tok: TOKEN } } as UserMcpSaveInput) + const id = (await live.snapshot()).servers[0]!.id + const unreadable = Buffer.from('sealed by another keychain', 'utf8') + await writeFile(join(dir, 'mcp-secrets', id, 'tok.bin'), unreadable, { mode: 0o600 }) + const result = await service().save({ id, ...upper('https://${input:trusted-host}/mcp?key=${input:TOK}'), secrets: { TOK: 'bpr_live_agent_case_3333' } } as UserMcpSaveInput, 'agent') + expect(result.ok).toBe(false) + expect(await blob(id, 'tok')).toEqual(unreadable) + }) + + // Gap 2 (B6): both guards had no committed test. + it('confirm refuses a record bound to ANOTHER destination, and Settings shows it as not set', async () => { + const live = service() + await live.save(beeper()) + const id = (await live.snapshot()).servers[0]!.id + const oldBlob = await blob(id, 'beeper-authorization') + // Crafted: the document moves elsewhere while the old destination's blob + // stays on disk (the q113 crash window). + await live.save(beeper({ id, secrets: {}, entry: { type: 'http', url: 'https://evil.example/mcp', headers: { Authorization: 'Bearer ${input:beeper-authorization}' } } } as Partial)) + await mkdir(join(dir, 'mcp-secrets', id), { recursive: true }) + await writeFile(join(dir, 'mcp-secrets', id, 'beeper-authorization.bin'), oldBlob, { mode: 0o600 }) + const restarted = service() + expect((await restarted.snapshot()).servers[0]!.secrets['beeper-authorization']).toEqual({ set: false }) + expect((await restarted.confirmSecret(id, 'beeper-authorization')).ok).toBe(false) + expect(await blob(id, 'beeper-authorization')).toEqual(oldBlob) + }) + + // The destination guard must hold on its own: here the steering value moved + // too, so the "already bound" check cannot be what refuses it. + it('confirm refuses a token saved for another destination even when a steering value also changed', async () => { + const live = service() + await live.save({ ...imported(), secrets: { 'svc-API_BASE_URL': 'https://trusted.example', 'svc-API_KEY': TOKEN } } as UserMcpSaveInput) + const id = (await live.snapshot()).servers[0]!.id + const oldKey = await blob(id, 'svc-API_KEY') + const moved = { ...imported(), entry: { ...imported().entry, args: ['other-client.js'] } } + expect(await live.save({ id, ...moved, secrets: { 'svc-API_BASE_URL': 'https://other.example' } } as UserMcpSaveInput)).toMatchObject({ ok: true, secretsCleared: true }) + await writeFile(join(dir, 'mcp-secrets', id, 'svc-API_KEY.bin'), oldKey, { mode: 0o600 }) + const restarted = service() + expect((await restarted.snapshot()).servers[0]!.secrets['svc-API_KEY']).toEqual({ set: false }) + expect((await restarted.confirmSecret(id, 'svc-API_KEY')).ok).toBe(false) + expect(await launched(service())).not.toContain(TOKEN) + }) + + it('confirm has nothing to do for a record already bound to the current binding, and leaves it byte-identical', async () => { + const live = service() + await live.save(beeper()) + const id = (await live.snapshot()).servers[0]!.id + const bytes = await blob(id, 'beeper-authorization') + expect((await live.confirmSecret(id, 'beeper-authorization')).ok).toBe(false) + expect(await blob(id, 'beeper-authorization')).toEqual(bytes) + }) +}) diff --git a/src/main/userMcp/service.ts b/src/main/userMcp/service.ts index cc9a4ef57..811004c09 100644 --- a/src/main/userMcp/service.ts +++ b/src/main/userMcp/service.ts @@ -1,4 +1,4 @@ -import { randomUUID } from 'node:crypto' +import { createHash, randomUUID } from 'node:crypto' import { join } from 'node:path' import type { SecretCodec } from '@main/keyVault/vaultStore.js' @@ -43,9 +43,11 @@ import { codexShellPolicyStyle, readNativeMcpServers, } from './nativeServers.js' -import { UserMcpSecretStore } from './secrets.js' +import { UserMcpSecretStore, type SecretBinding } from './secrets.js' import { loadUserMcpDocument, saveUserMcpDocument } from './store.js' +type PendingSecretRestore = { run: () => Promise; safeWithNewDocument: boolean } + export type UserMcpLaunchResolution = { servers: ResolvedUserMcpServer[] attachedIds: string[] @@ -76,6 +78,22 @@ export type UserMcpServiceDeps = { * deleted or switched off. The renderer contributes only the pane's explicit * per-agent choices; everything else is read here at launch. */ +/** + * Whether `inputId` names one of `ids`, ignoring letter case (B6 check at + * fb61adfa). Secrets live in `.bin`, and on a case-insensitive disk + * (macOS by default, Windows) `TOK.bin` IS `tok.bin`: an agent writing `TOK` + * overwrote the withheld `tok`. Case-folding refuses it on every filesystem, + * which is the fail-closed side (a user can still rename or re-enter). + */ +function sameInputId(ids: readonly string[], inputId: string): boolean { + const folded = inputId.toLowerCase() + return ids.some(id => id.toLowerCase() === folded) +} + +/** Why an agent's write or removal was refused (see withheldInputIds). */ +const WITHHELD_REFUSAL = (inputId: string): string => + `Secret "${inputId}" is withheld until the user confirms or re-enters it in Settings → MCP, so an agent cannot replace or remove it.` + export class UserMcpService { private document: UserMcpDocument = { version: 1, servers: [] } private storeProblem: string | undefined @@ -155,6 +173,11 @@ export class UserMcpService { const problem = value === '' ? null : secretValueProblem(value) if (problem) return { ok: false, error: problem } } + if (actor === 'agent' && existing) { + const withheld = await this.withheldInputIds(existing) + const overwrite = Object.keys(input.secrets ?? {}).find(inputId => sameInputId(withheld, inputId)) + if (overwrite) return { ok: false, error: WITHHELD_REFUSAL(overwrite) } + } const entry = normalizeEntry(input.entry) const destinationChanged = existing !== undefined && userMcpDestination(existing.entry) !== userMcpDestination(entry) // An agent proposes, the user approves (review round 2): a server an @@ -163,7 +186,12 @@ export class UserMcpService { // prompt-injected agent could install a command that every future agent // runs. A user save of an existing server clears the flag only by // turning it on (setEnabled); saving it off keeps the flag visible. - const agentNeedsReview = actor === 'agent' && (existing === undefined || destinationChanged || existing.pendingReview === true) + // An agent (or an agent's import) supplying ANY input value can move + // where the other secrets go without touching the entry, so it needs + // review exactly like an entry change (B6 R3, q127: no classifier, the + // key name proves nothing about how a program uses the value). + const agentSetsValue = Object.keys(input.secrets ?? {}).length > 0 + const agentNeedsReview = actor === 'agent' && (existing === undefined || destinationChanged || agentSetsValue || existing.pendingReview === true) const enabled = actor === 'agent' ? (agentNeedsReview ? false : existing!.enabled && input.enabled) : input.enabled @@ -183,43 +211,90 @@ export class UserMcpService { // and launch already refuses to attach the server until it is set. const problems = validateServer(server, others) if (problems.length > 0) return { ok: false, error: problems[0]!.message, problems } - // Changing WHERE a server connects forgets its stored secrets (review - // round 1). Otherwise an edit — or an agent's mcp_servers_update after a - // prompt injection — could keep `${input:token}` and point the entry at + // Changing WHERE a server connects must not carry its tokens along + // (review round 1): otherwise an agent's mcp_servers_update after a + // prompt injection could keep `${input:token}`, point the entry at // another host or command, and the next launch would hand the token to - // it: exfiltration without ever reading a secret. Secrets supplied in + // it. Since q113 that is enforced at READ time by the destination + // binding. A user's move also forgets the old secrets (their intent is a + // new server); an agent's move keeps the blobs, withheld, so it cannot + // delete credentials either (fresh r3 reviews a+b). Secrets supplied in // this same save are set afterwards, so an intentional move that // re-enters the token still works in one step. + // SECURITY INVARIANT (q110, #1420 review a): a destination is never + // observable, by a launch or by a restart at any point, paired with a + // token that was not saved for it. The order below is what holds it: + // 1. snapshot the server's current secrets (strictly: an unreadable + // directory aborts here, before anything changes); + // 2. on a USER destination change, CLEAR the old secrets while the old + // destination is still the published one, on disk and in memory + // (an agent's change clears nothing; see forgetsSecrets below. The + // q113 read-time binding is what holds the invariant either way); + // 3. only then publish the new document (memory, then disk); + // 4. write the new secrets and (user only) prune. + // A crash anywhere leaves at worst a server with NO secret: fail closed. + // The earlier order (document first, then clear) had a window, and a + // failed rollback made it durable, in which the NEW destination sat on + // disk with the OLD token, and resolveForLaunch could read it. + // + // On failure, mutate() rolls the document back first and restores this + // snapshot only if the old document is back on disk, or if the + // destination did not change (then the old secrets still match the + // document that is on disk). Otherwise the secrets stay cleared. + const previousSecrets = existing + ? await this.secrets.snapshotServer(server.id) + : new Map() + // Only the USER's edits forget secrets (fresh r3 reviews a+b). An agent's + // edit used to clear too, which deleted a secret already WITHHELD for + // the user's confirmation: a prompt-injected agent could erase a + // credential without review. Since q113 the read-time binding keeps an + // old token away from a new destination, so the agent's edit leaves the + // blobs on disk (unusable for the new entry until the user re-enters + // them, usable again if the edit is reverted) and deletes nothing. + const forgetsSecrets = destinationChanged && actor === 'user' + this.pendingSecretRestore = { + run: () => this.secrets.restoreServer(server.id, previousSecrets), + safeWithNewDocument: !forgetsSecrets, + } + if (forgetsSecrets) await this.secrets.clearServer(server.id) this.document = { version: 1, servers: existing ? this.document.servers.map(candidate => candidate.id === server.id ? server : candidate) : [...this.document.servers, server], } - // Document first, secret blobs after (review round 2): a failed persist - // rolls the document back in mutate(), and blobs cleared before it could - // not be rolled back, so a failed destination edit used to lose the - // server's token for good. await this.persist() - if (destinationChanged) await this.secrets.clearServer(server.id) - for (const [inputId, value] of Object.entries(input.secrets ?? {})) { - if (server.inputs.some(candidate => candidate.id === inputId)) { - await this.secrets.set(server.id, inputId, value) - } - } - await this.secrets.prune(server.id, server.inputs.map(candidate => candidate.id)) + const supplied = Object.fromEntries(Object.entries(input.secrets ?? {}) + .filter(([inputId]) => server.inputs.some(candidate => candidate.id === inputId))) + await this.writeValues(server, supplied, actor) + // Pruning orphans is the user's housekeeping for the same reason: an + // agent dropping a reference must not delete that secret. + if (actor === 'user') await this.secrets.prune(server.id, server.inputs.map(candidate => candidate.id)) return { ok: true, id: server.id, - ...(destinationChanged ? { secretsCleared: true } : {}), + ...(forgetsSecrets ? { secretsCleared: true } : {}), ...(pendingReview ? { pendingReview: true } : {}), } }) } - delete(id: string): Promise { + /** An agent may not remove a server holding a withheld secret (see withheldInputIds). */ + delete(id: string, actor: UserMcpActor = 'user'): Promise { return this.mutate(async () => { - if (!this.document.servers.some(server => server.id === id)) return { ok: false, error: 'That server no longer exists.' } + const existing = this.document.servers.find(server => server.id === id) + if (!existing) return { ok: false, error: 'That server no longer exists.' } + if (actor === 'agent') { + const [withheld] = await this.withheldInputIds(existing) + if (withheld) return { ok: false, error: `${WITHHELD_REFUSAL(withheld)} Only the user can remove this server now (Settings → MCP).` } + } + // Snapshot strictly first (q110), so a failed clear can restore exactly + // what was there once the document is back (see save()). + const previousSecrets = await this.secrets.snapshotServer(id) + this.pendingSecretRestore = { + run: () => this.secrets.restoreServer(id, previousSecrets), + safeWithNewDocument: false, + } this.document = { version: 1, servers: this.document.servers.filter(server => server.id !== id) } await this.persist() await this.secrets.clearServer(id) @@ -243,18 +318,155 @@ export class UserMcpService { return this.update(id, server => ({ ...server, providers: { ...server.providers, [provider]: enabled } })) } - setSecret(id: string, inputId: string, value: string): Promise { + /** + * The USER confirms that a secret saved by an earlier version is for this + * server's current destination (q114), which binds it. Deliberately NOT on + * the agent tool surface (userMcpTools): an agent confirming an old token + * for a destination it just set would be the exfiltration binding prevents. + */ + confirmSecret(id: string, inputId: string): Promise { + return this.mutate(async () => { + const server = this.document.servers.find(candidate => candidate.id === id) + if (!server) return { ok: false, error: 'That server no longer exists.' } + if (!server.inputs.some(input => input.id === inputId)) return { ok: false, error: `No secret named "${inputId}".` } + const confirmed = await this.secrets.confirm(id, inputId, (await this.bindingsFor(server))[inputId]!) + if (!confirmed) return { ok: false, error: `Secret "${inputId}" has nothing to confirm.` } + return { ok: true } + }) + } + + /** + * Store one secret. An AGENT setting ANY input turns the server off and + * flags it for review first (B6 R3, q127): the value may move where the + * other secrets go, and those are withheld by their bindings until the user + * confirms. The user path (Settings IPC) is itself the confirmation; see + * writeValues. + */ + setSecret(id: string, inputId: string, value: string, actor: UserMcpActor = 'user'): Promise { return this.mutate(async () => { const problem = value === '' ? null : secretValueProblem(value) if (problem) return { ok: false, error: problem } const server = this.document.servers.find(candidate => candidate.id === id) if (!server) return { ok: false, error: 'That server no longer exists.' } if (!server.inputs.some(input => input.id === inputId)) return { ok: false, error: `No secret named "${inputId}".` } - await this.secrets.set(id, inputId, value) - return { ok: true } + if (actor === 'agent' && sameInputId(await this.withheldInputIds(server), inputId)) { + return { ok: false, error: WITHHELD_REFUSAL(inputId) } + } + // q127: ANY input an agent changes, clearing included. + const needsReview = actor === 'agent' + if (needsReview) { + // Published BEFORE the value is written: a crash between the two + // leaves a reviewed-off server with the old value, never an enabled + // one with the new. + const flagged: UserMcpServer = { ...server, enabled: false, pendingReview: true } + this.document = { version: 1, servers: this.document.servers.map(candidate => candidate.id === id ? flagged : candidate) } + await this.persist() + } + await this.writeValues(server, { [inputId]: value }, actor) + return { ok: true, ...(needsReview ? { pendingReview: true } : {}) } }) } + /** + * What each of the server's secrets is bound to (#1420, B6 R3, q127): its + * destination identity plus a digest of the values of EVERY other input the + * entry references. `pending` overlays values about to be written, so a + * record is bound to the values it will be launched with. + * + * WHY every other input and no credential/steering classifier (q127): a + * key's NAME and a value's shape do not prove its role; a stdio program can + * read API_KEY as an endpoint. A record's own value is left out, so the + * record for a rotated token is still bound to what it was saved for. + * Values are read raw (storedValue): the digest must see what a launch would + * substitute, bound or not. + */ + private async bindingsFor(server: UserMcpServer, pending: Readonly> = {}): Promise> { + const destination = userMcpDestination(server.entry) + const referenced = referencedInputIds(server.entry).sort() + const values = new Map() + for (const inputId of referenced) { + values.set(inputId, inputId in pending ? (pending[inputId] || null) : await this.secrets.storedValue(server.id, inputId)) + } + const bindings: Record = {} + for (const input of server.inputs) { + const others = referenced.filter(inputId => inputId !== input.id).map(inputId => [inputId, values.get(inputId) ?? null]) + bindings[input.id] = { destination, inputs: createHash('sha256').update(JSON.stringify(others)).digest('hex') } + } + return bindings + } + + /** + * Inputs whose stored secret is WITHHELD: a blob exists but does not prove + * its current binding (legacy, bound before an agent changed another input, + * or not decryptable at all, q130). The user can still confirm, re-enter or + * recover them. + * + * WHY an agent may not overwrite or delete one (r3 round-2 reviews a+b): a + * withheld secret is kept precisely so the USER decides what happens to it. + * An agent overwriting it (mcp_servers_set_secret, save with values) or + * removing its server (mcp_servers_remove) destroyed the ciphertext before + * that decision, so a prompt-injected agent could erase a credential it can + * never read. A valid secret may still be replaced by an agent (a user + * asking in chat to rotate a token), which already needs review. + */ + private async withheldInputIds(server: UserMcpServer): Promise { + const bindings = await this.bindingsFor(server) + const withheld: string[] = [] + // Defined inputs AND every blob on disk (r3 round 3, review a): an agent + // edit that drops an input keeps its blob, and a guard walking only the + // defined inputs let mcp_servers_remove delete it. A blob with no input + // can prove no binding, so it is withheld. + const ids = [...new Set([...server.inputs.map(input => input.id), ...await this.secrets.storedInputIds(server.id)])] + for (const id of ids) { + // Raw presence, not a decrypted value (q130): a blob that exists but + // cannot be decrypted is a secret the user may still recover, so it + // counts as withheld. Only ENOENT means there is nothing to protect. + if (!(await this.secrets.present(server.id, id))) continue + const binding = bindings[id] + if (!binding || await this.secrets.get(server.id, id, binding) === null) withheld.push(id) + } + return withheld + } + + /** + * Write input values, bound to the values they will launch with. + * + * USER (Settings IPC; q127): the user's own edit is the confirmation, so + * every sibling that was VALID just before the edit is rebound to the new + * digest. Rotating one token therefore never locks out the others. A sibling + * that was already withheld (an earlier agent change) stays withheld: an + * unrelated Settings edit must not bless that change; only an explicit + * confirm or a re-entry does. + * + * AGENT (MCP tools, imports): no sibling is rebound, so every sibling whose + * digest included the changed value is withheld until the user confirms it. + * The agent tools can only reach this with actor 'agent' + * (userMcpTools.ts); the user actor is only ever passed by the Settings IPC. + * + * Written value first, siblings after: a crash between them leaves a + * sibling withheld (fail closed), never a sibling bound to values it was not + * confirmed for. + */ + private async writeValues(server: UserMcpServer, supplied: Readonly>, actor: UserMcpActor): Promise { + if (Object.keys(supplied).length === 0) return + const valid: Array<[string, string]> = [] + if (actor === 'user') { + const before = await this.bindingsFor(server) + for (const input of server.inputs) { + if (input.id in supplied) continue + const value = await this.secrets.get(server.id, input.id, before[input.id]!) + if (value !== null) valid.push([input.id, value]) + } + } + const after = await this.bindingsFor(server, supplied) + for (const [inputId, value] of Object.entries(supplied)) { + await this.secrets.set(server.id, inputId, value, after[inputId]!) + } + for (const [inputId, value] of valid) { + await this.secrets.set(server.id, inputId, value, after[inputId]!) + } + } + /** * Copy a CLI-native server into Agent Code. * @@ -299,7 +511,23 @@ export class UserMcpService { * "it's attached" while the agent has no such tools. * A dropped server never fails the launch. */ - async resolveForLaunch(params: { + /** + * Launch reads are serialized with mutations (q110). Reading + * `this.document` and the secret store while a save was between its steps + * could see a half-applied change; now a launch runs strictly before or + * after each mutation, never inside one. + */ + resolveForLaunch(params: { + provider: string + overrides: Readonly> + cwd: string + }): Promise { + const run = this.tail.then(() => this.resolveForLaunchNow(params)) + this.tail = run.then(() => {}, () => {}) + return run + } + + private async resolveForLaunchNow(params: { provider: string overrides: Readonly> cwd: string @@ -344,8 +572,11 @@ export class UserMcpService { } const secrets: Record = {} let missing: string | null = null + const bindings = await this.bindingsFor(server) for (const inputId of referencedInputIds(server.entry)) { - const value = await this.secrets.get(server.id, inputId) + // Bound read (q113, B6 R3): only a secret saved for this destination + // AND for the current values of the server's other inputs. + const value = bindings[inputId] ? await this.secrets.get(server.id, inputId, bindings[inputId]!) : null if (value === null) { missing = inputId break @@ -413,33 +644,92 @@ export class UserMcpService { this.storeProblem = loaded.problem this.readFailed = false } + this.persistedInMutation = false + this.pendingSecretRestore = null + let outcome: Awaited> try { - const outcome = await operation() - if (!outcome.ok) return outcome - const snapshot = await this.snapshot() - for (const listener of this.listeners) listener(snapshot) - return { - ok: true, - snapshot, - ...(outcome.id ? { id: outcome.id } : {}), - ...(outcome.secretsCleared ? { secretsCleared: true } : {}), - ...(outcome.pendingReview ? { pendingReview: true } : {}), - } + outcome = await operation() } catch (error) { // Review round 1: a failed persist must not leave memory ahead of // disk, or the snapshot shows a server that a restart will lose and a // retry is refused as a duplicate. (Secrets written before the failure // are orphaned blobs at worst; the next save of that server prunes them.) - this.document = before + // + // #1304: operations persist the document BEFORE their secret step + // (round 2 of that review, so a failed persist cannot lose a token). + // A secret step that throws after that point used to roll back only + // memory, leaving disk ahead of it: a saved server came back after a + // restart without its secret, and a deleted one was written back by + // the next mutation. Roll the FILE back too. If that write fails as + // well, disk still holds the new document, so memory keeps it: the + // two must agree either way. + let oldDocumentOnDisk: boolean + if (this.persistedInMutation) { + try { + await saveUserMcpDocument(this.file, before) + this.document = before + oldDocumentOnDisk = true + } catch { + // Disk holds the persisted document; memory already matches it. + oldDocumentOnDisk = false + } + } else { + this.document = before + oldDocumentOnDisk = true + } + // q110: put the previous secrets back only where they pair with the + // document that is actually on disk. A restore that itself fails is + // not retried: the server is left without (some of) its secrets, + // which launch refuses to attach. That is the fail-closed direction. + // Read through a method: TypeScript narrows the field to null from the + // assignment above and cannot see that the operation set it. + const restore = this.takePendingSecretRestore() + if (restore && (oldDocumentOnDisk || restore.safeWithNewDocument)) { + await restore.run().catch(() => {}) + } return { ok: false, error: error instanceof Error ? error.message : String(error) } } + if (!outcome.ok) return outcome + // Committed. Nothing after this line may report failure for a change + // that is on disk (q110, review a finding 4): a listener that threw used + // to land in the rollback path, which reverted the document but not + // the secrets it had just written. Each listener is isolated. + const snapshot = await this.snapshot() + for (const listener of this.listeners) { + try { + listener(snapshot) + } catch (error) { + console.warn('[user-mcp] change listener failed:', error) + } + } + return { + ok: true, + snapshot, + ...(outcome.id ? { id: outcome.id } : {}), + ...(outcome.secretsCleared ? { secretsCleared: true } : {}), + ...(outcome.pendingReview ? { pendingReview: true } : {}), + } }) this.tail = run.catch(() => {}) return run } + /** The secret restore the current operation registered before touching + * secrets (q110); mutate() decides whether it may run. */ + private pendingSecretRestore: PendingSecretRestore | null = null + + private takePendingSecretRestore(): PendingSecretRestore | null { + const restore = this.pendingSecretRestore + this.pendingSecretRestore = null + return restore + } + + /** Set by persist() during the current mutate() operation (#1304). */ + private persistedInMutation = false + private async persist(): Promise { await saveUserMcpDocument(this.file, this.document) + this.persistedInMutation = true // A successful write supersedes whatever made the old file unreadable. this.storeProblem = undefined } @@ -452,11 +742,18 @@ export class UserMcpService { ): Promise { const transport = transportOf(server.entry) const others = this.document.servers.filter(other => other.id !== server.id) - const secrets = await this.secrets.state(server.id, server.inputs.map(input => input.id)) + const secrets = await this.secrets.state(server.id, await this.bindingsFor(server)) const problems = validateServer(server, others) for (const inputId of referencedInputIds(server.entry)) { if (secrets[inputId] && !secrets[inputId]!.set) { - problems.push({ kind: 'secret-missing', message: `Secret "${inputId}" is not set` }) + problems.push({ + kind: 'secret-missing', + message: secrets[inputId]!.unconfirmed === 'legacy' + ? `Secret "${inputId}" was saved by an earlier version. Confirm it is for ${summarizeEntry(server.entry)}, or re-enter it` + : secrets[inputId]!.unconfirmed === 'inputs-changed' + ? `Secret "${inputId}" is withheld because an agent changed another value this server uses. Confirm it may be sent to ${summarizeEntry(server.entry)} with the new values, or re-enter it` + : `Secret "${inputId}" is not set`, + }) } } if (server.pendingReview) { diff --git a/src/mcp/runtime/userMcpTools.test.ts b/src/mcp/runtime/userMcpTools.test.ts index e10d56d52..cf6c6257c 100644 --- a/src/mcp/runtime/userMcpTools.test.ts +++ b/src/mcp/runtime/userMcpTools.test.ts @@ -126,6 +126,75 @@ describe('mcp_servers built-in domain', () => { await close() }) + // #1420 q127: the agent tool is the AGENT path, never the user path. A + // value it sets turns the server off for review and withholds the sibling + // secret; if the tool ever passed the user actor, the sibling would be + // rebound and keep launching. + it('set_secret from an agent turns a reviewed server off and withholds its other secret', async () => { + const saved = await service.save({ + name: 'svc', + enabled: true, + providers: { claude: true, codex: true }, + entry: { command: 'node', args: ['client.js'], env: { API_BASE_URL: '${input:base}', API_KEY: '${input:key}' } }, + inputs: [{ id: 'base', description: 'base' }, { id: 'key', description: 'key' }], + secrets: { base: 'https://trusted.example', key: TOKEN }, + }) + expect(saved.ok).toBe(true) + const id = (await service.snapshot()).servers[0]!.id + const { call, close } = await connect(['mcp_servers']) + await call('mcp_servers_set_secret', { id, inputId: 'key', value: 'agent-chosen-value-9999' }) + const [server] = (await service.snapshot()).servers + expect(server!.pendingReview).toBe(true) + expect(server!.enabled).toBe(false) + expect(server!.secrets.base).toMatchObject({ set: false, unconfirmed: 'inputs-changed' }) + await close() + }) + + // r3 round-2 reviews a+b: the remove tool is the AGENT path, so a server + // holding a withheld secret cannot be removed through it. + it('remove refuses a server whose secret is withheld for the user', async () => { + await service.save({ + name: 'svc', + enabled: true, + providers: { claude: true, codex: true }, + entry: { command: 'node', args: ['client.js'], env: { API_BASE_URL: '${input:base}', API_KEY: '${input:key}' } }, + inputs: [{ id: 'base', description: 'base' }, { id: 'key', description: 'key' }], + secrets: { base: 'https://trusted.example', key: TOKEN }, + }) + const id = (await service.snapshot()).servers[0]!.id + await service.setSecret(id, 'base', 'https://evil.example', 'agent') + const { call, close } = await connect(['mcp_servers']) + const removed = await call('mcp_servers_remove', { id }) + expect(removed.isError).toBe(true) + expect((await service.snapshot()).servers.map(server => server.id)).toEqual([id]) + await close() + }) + + // q131 through the exposed tools: mcp_servers_update drops every reference + // (the server keeps no inputs), then mcp_servers_remove. The orphaned + // secrets are the user's, so the removal is refused and the bytes stay. + it('update dropping every reference, then remove, cannot delete the orphaned secrets', async () => { + await service.save({ + name: 'svc', + enabled: true, + providers: { claude: true, codex: true }, + entry: { command: 'node', args: ['client.js'], env: { API_BASE_URL: '${input:base}', API_KEY: '${input:key}' } }, + inputs: [{ id: 'base', description: 'base' }, { id: 'key', description: 'key' }], + secrets: { base: 'https://trusted.example', key: TOKEN }, + }) + const id = (await service.snapshot()).servers[0]!.id + const keyFile = join(dir, 'mcp-secrets', id, 'key.bin') + const keyBytes = await readFile(keyFile) + const { call, close } = await connect(['mcp_servers']) + const updated = await call('mcp_servers_update', { id, entry: { command: 'node', args: ['client.js'] } }) + expect(updated.isError).toBe(false) + expect((await service.snapshot()).servers[0]!.inputs).toEqual([]) + const removed = await call('mcp_servers_remove', { id }) + expect(removed.isError).toBe(true) + expect(await readFile(keyFile)).toEqual(keyBytes) + await close() + }) + it('cannot turn a server on (review round 2)', async () => { const { call, close } = await connect(['mcp_servers']) await call('mcp_servers_add', { config: '{"url":"https://x.dev/mcp"}', name: 'x' }) diff --git a/src/mcp/runtime/userMcpTools.ts b/src/mcp/runtime/userMcpTools.ts index 9132df6c9..1b95d15d9 100644 --- a/src/mcp/runtime/userMcpTools.ts +++ b/src/mcp/runtime/userMcpTools.ts @@ -122,7 +122,7 @@ export function registerUserMcpTools( server.registerTool('mcp_servers_update', { title: 'Update an MCP server', - description: 'Change one server: its name, its config entry (the full entry object, same shape as mcp_servers_add accepts; keep ${input:…} references for secrets), which providers new agents get it on, or turn it OFF. Changing the entry\'s URL, command, arguments or environment forgets its stored secrets and switches it off until the user reviews it. You cannot turn a server on.', + description: 'Change one server: its name, its config entry (the full entry object, same shape as mcp_servers_add accepts; keep ${input:…} references for secrets), which providers new agents get it on, or turn it OFF. Changing the entry\'s URL, command, arguments or environment switches it off until the user reviews it; its stored secrets are kept but not used with the changed config until the user enters them again. You cannot turn a server on.', inputSchema: { id: z.string().min(1).max(64), name: z.string().min(1).max(64).optional(), @@ -161,13 +161,13 @@ export function registerUserMcpTools( server.registerTool('mcp_servers_remove', { title: 'Remove an MCP server', - description: 'Delete one of the user\'s MCP servers and its stored secrets. Only when the user\'s current request asks to remove that server.', + description: 'Delete one of the user\'s MCP servers and its stored secrets. Only when the user\'s current request asks to remove that server. A server with a secret waiting for the user\'s confirmation can only be removed by the user.', inputSchema: { id: z.string().min(1).max(64) }, annotations: { readOnlyHint: false, destructiveHint: true, idempotentHint: true, openWorldHint: false }, }, async ({ id }) => { try { const name = await nameOf(id) - const result = await service().delete(id) + const result = await service().delete(id, 'agent') if (result.ok) changed(`An agent removed MCP server ${name}`) return mutation(result) } catch (error) { @@ -177,7 +177,7 @@ export function registerUserMcpTools( server.registerTool('mcp_servers_set_secret', { title: 'Set an MCP server secret', - description: 'Store one secret (a ${input:id} the server config references) encrypted. Only use a value the user gave you in this conversation; never invent one. The value is never returned by any tool.', + description: 'Store one secret (a ${input:id} the server config references) encrypted. Only use a value the user gave you in this conversation; never invent one. The value is never returned by any tool. Any value you set switches the server off until the user reviews it, and its other secrets wait for the user to confirm them.', inputSchema: { id: z.string().min(1).max(64), inputId: z.string().min(1).max(64), @@ -187,8 +187,12 @@ export function registerUserMcpTools( }, async ({ id, inputId, value }) => { try { const name = await nameOf(id) - const result = await service().setSecret(id, inputId, value) - if (result.ok) changed(`An agent set a secret for MCP server ${name}`) + const result = await service().setSecret(id, inputId, value, 'agent') + if (result.ok) { + changed(result.pendingReview + ? `An agent set a secret for MCP server ${name} (off until you review it)` + : `An agent set a secret for MCP server ${name}`) + } return mutation(result) } catch (error) { return failure(error) diff --git a/src/preload/api/git.ts b/src/preload/api/git.ts index e2c80ef5c..b65ac5654 100644 --- a/src/preload/api/git.ts +++ b/src/preload/api/git.ts @@ -43,7 +43,9 @@ export const gitApi = { summaries: WorktreeActivitySummary[] status: WorktreeActivityIndexStatus } - | { ok: false } + // `timedOut` (#1430): git worktree list did not answer in time; the + // activity is unknown, not absent. + | { ok: false; timedOut?: true } > => ipcRenderer.invoke('worktree-activity:summary', cwd, refresh), gitStatus: (cwd: string): Promise => diff --git a/src/preload/api/userMcp.ts b/src/preload/api/userMcp.ts index 85d8f648b..e0df1069a 100644 --- a/src/preload/api/userMcp.ts +++ b/src/preload/api/userMcp.ts @@ -26,6 +26,8 @@ export const userMcpApi = { ipcRenderer.invoke('user-mcp:set-provider', id, provider, enabled), userMcpSetSecret: (id: string, inputId: string, value: string): Promise => ipcRenderer.invoke('user-mcp:set-secret', id, inputId, value), + userMcpConfirmSecret: (id: string, inputId: string): Promise => + ipcRenderer.invoke('user-mcp:confirm-secret', id, inputId), userMcpImport: (text: string, fallbackName?: string): Promise => ipcRenderer.invoke('user-mcp:import', text, fallbackName), userMcpCopyNative: (provider: UserMcpProvider, name: string): Promise => diff --git a/src/providers/opencode/runtime/opencodeTerminalSession.test.ts b/src/providers/opencode/runtime/opencodeTerminalSession.test.ts index d47143d0c..ea4997fcc 100644 --- a/src/providers/opencode/runtime/opencodeTerminalSession.test.ts +++ b/src/providers/opencode/runtime/opencodeTerminalSession.test.ts @@ -14,6 +14,19 @@ vi.mock('node-pty', () => ({ spawn: ptyState.spawn })) vi.mock('./opencodeCliSessions.js', () => ({ createEmptyOpencodeSession: ptyState.createEmptySession, })) +// The REAL headless, subclassed only to record what the adapter hands it +// (#1114: the `tuiOutputSeen` latch). Behaviour is unchanged. +const headlessState = vi.hoisted(() => ({ options: [] as Array<{ tuiOutputSeen?: () => boolean }> })) +vi.mock('opencode-terminal-headless', async importOriginal => { + const actual = await importOriginal() + class RecordingHeadless extends actual.OpencodeTerminalHeadless { + constructor(options: ConstructorParameters[0]) { + headlessState.options.push(options) + super(options) + } + } + return { ...actual, OpencodeTerminalHeadless: RecordingHeadless } +}) import { OpencodeTerminalSession } from './opencodeTerminalSession.js' @@ -141,6 +154,131 @@ describe('OpencodeTerminalSession', () => { expect(pty.write).not.toHaveBeenCalled() }) + // #1114 / opencode-terminal-headless#10 (recheck2 a/b): the package can + // prove that nothing was committed behind its reader's starting head only if + // the host (1) latches the TUI's first output from spawn and passes it as + // `tuiOutputSeen`, and (2) lets nothing commit-capable reach the PTY before + // that output. Both are pinned here. + it('hands the headless a first-output latch that is false until the TUI paints', async () => { + const pty = fakePty() + ptyState.spawn.mockReturnValue(pty) + const { session } = create({ cwd: '/workspace', resumeSessionId: 'ses_123' }) + await session.start() + const latch = headlessState.options.at(-1)?.tuiOutputSeen + expect(latch).toBeTypeOf('function') + expect(latch?.()).toBe(false) + pty.emitData('\x1b[?1049h') + expect(latch?.()).toBe(true) + }) + + it('holds terminal input written before the TUI paints, then writes it in order', async () => { + const pty = fakePty() + ptyState.spawn.mockReturnValue(pty) + const { session } = create({ cwd: '/workspace', resumeSessionId: 'ses_123' }) + await session.start() + session.write('hel') + session.write('lo\r') + // Nothing reaches the TUI while it cannot have painted. + expect(pty.write).not.toHaveBeenCalled() + pty.emitData('\x1b[?1049h') + expect(pty.write.mock.calls.map(call => call[0])).toEqual(['hel', 'lo\r']) + // After the first output, input passes straight through. + session.write('x') + expect(pty.write.mock.calls.map(call => call[0])).toEqual(['hel', 'lo\r', 'x']) + }) + + it('drops input held for a TUI that never painted when the pane stops', async () => { + const pty = fakePty() + ptyState.spawn.mockReturnValue(pty) + const { session } = create({ cwd: '/workspace', resumeSessionId: 'ses_123' }) + await session.start() + session.write('typed early') + await session.stop() + pty.emitData('late paint') + expect(pty.write).not.toHaveBeenCalled() + }) + + // Steering q97: the pre-paint hold is bounded, and what it cannot hold is + // REFUSED (write returns false, which main reports), never silently dropped. + it('refuses a paste larger than the pre-paint hold, and keeps what it already held', async () => { + const pty = fakePty() + ptyState.spawn.mockReturnValue(pty) + const { session } = create({ cwd: '/workspace', resumeSessionId: 'ses_123' }) + await session.start() + expect(session.write('typed')).toBe(true) + expect(session.write('x'.repeat(64 * 1024))).toBe(false) + pty.emitData('\x1b[?1049h') + expect(pty.write.mock.calls.map(call => call[0])).toEqual(['typed']) + }) + + it('refuses the 257th held chunk', async () => { + const pty = fakePty() + ptyState.spawn.mockReturnValue(pty) + const { session } = create({ cwd: '/workspace', resumeSessionId: 'ses_123' }) + await session.start() + for (let i = 0; i < 256; i += 1) expect(session.write('k')).toBe(true) + expect(session.write('k')).toBe(false) + }) + + // #1397 review a (survivors A3-A5): `start()` after `stop()` is allowed, so + // the hold must start over per spawn. A latch left `true` from the first + // TUI would let input reach an unpainted second TUI; input held for the + // first would be flushed into the second; a late paint from the dead first + // PTY would latch and flush the second's hold. + it('starts the hold over for a restarted TUI and ignores the old PTY', async () => { + const first = fakePty() + ptyState.spawn.mockReturnValue(first) + const { session } = create({ cwd: '/workspace', resumeSessionId: 'ses_123' }) + await session.start() + first.emitData('\x1b[?1049h') + await session.stop() + const firstOnData = first.onData.mock.calls[0]?.[0] as (data: string) => void + const second = fakePty() + ptyState.spawn.mockReturnValue(second) + await session.start() + // Held input from a previous generation must not survive into this one. + session.write('for the second TUI') + expect(second.write).not.toHaveBeenCalled() + expect(headlessState.options.at(-1)?.tuiOutputSeen?.()).toBe(false) + // The dead first PTY's listener firing late must not release the hold. + firstOnData('late paint from the first TUI') + expect(second.write).not.toHaveBeenCalled() + second.emitData('\x1b[?1049h') + expect(second.write.mock.calls.map(call => call[0])).toEqual(['for the second TUI']) + }) + + // #1397 review a (survivor A4): stop clears the hold itself, not only the + // PTY subscription. The per-spawn reset also covers a restart, so the + // stop-time clear is pinned directly: a stopped pane must not keep up to + // 64 KiB of typed text alive for its lifetime. + it('clears input held before a stop, and never replays it into a restarted TUI', async () => { + const first = fakePty() + ptyState.spawn.mockReturnValue(first) + const { session } = create({ cwd: '/workspace', resumeSessionId: 'ses_123' }) + await session.start() + session.write('meant for the first TUI') + await session.stop() + expect((session as unknown as { heldInput: string[] }).heldInput).toEqual([]) + const second = fakePty() + ptyState.spawn.mockReturnValue(second) + await session.start() + second.emitData('\x1b[?1049h') + expect(second.write).not.toHaveBeenCalled() + }) + + it('clears held input when the TUI exits before painting', async () => { + const pty = fakePty() + ptyState.spawn.mockReturnValue(pty) + const { session } = create({ cwd: '/workspace', resumeSessionId: 'ses_123' }) + await session.start() + session.write('x'.repeat(1024)) + pty.emitExit({ exitCode: 1, signal: 0 }) + await vi.waitFor(() => expect(session.isExited()).toBe(true)) + expect((session as unknown as { heldInput: string[] }).heldInput).toEqual([]) + // No backend any more: refused, not held. + expect(session.write('after exit')).toBe(false) + }) + it('reports a degraded durable channel instead of failing the pane', async () => { const pty = fakePty() ptyState.spawn.mockReturnValue(pty) diff --git a/src/providers/opencode/runtime/opencodeTerminalSession.ts b/src/providers/opencode/runtime/opencodeTerminalSession.ts index 3b0594e75..47334d1fc 100644 --- a/src/providers/opencode/runtime/opencodeTerminalSession.ts +++ b/src/providers/opencode/runtime/opencodeTerminalSession.ts @@ -19,6 +19,14 @@ import type { } from '@shared/types/session.js' const TUI_READY_GRACE_MS = 250 +// Bounds on terminal input held before the TUI's first output (#1114, +// steering q97). A TUI that never paints (the recorded port conflict neither +// paints nor exits) must not turn pastes into unbounded growth of Electron's +// main process. 256 chunks matches the renderer's own pre-attach input queue; +// 64 KiB is far more than anyone types into a pane that has not painted, and +// small next to a paste worth refusing loudly. +const HELD_INPUT_MAX_CHUNKS = 256 +const HELD_INPUT_MAX_CHARS = 64 * 1024 class OpencodeTerminalNotReadyError extends Error { readonly code = 'opencode-terminal-not-ready' @@ -111,6 +119,24 @@ export class OpencodeTerminalSession extends EventEmitter implements AgentSessio private ptyDataSubscription: { dispose(): void } | null = null private providerSessionId: string | null = null private readinessTimer: ReturnType | null = null + // #1114: the TUI now spawns before OpenCode's database path is known, and + // the package's durable reader positions at the database's head only when + // the lookup lands. Rows committed before that are behind the head. The + // package can prove there were none only if two things hold, and both are + // this adapter's job (opencode-terminal-headless#10, recheck2 a/b): + // 1. `tuiOutput` is latched from SPAWN (a data subscription does not + // replay, and the headless is built after the spawn), and handed to + // the headless as `tuiOutputSeen`; + // 2. nothing that can make OpenCode commit reaches the PTY before the + // TUI's first output. Programmatic prompts go through the server and + // the package gates them; terminal input (keystrokes, pastes) comes + // through `write`, which HOLDS it until then (`heldInput`). + // OpenCode takes input only after it paints, so with both, "no output yet" + // proves nothing was committed. Without them a gap is reported as possible + // and the renderer re-reads history (#1117). + private tuiOutput = false + private heldInput: string[] = [] + private heldInputChars = 0 private readonly cwd: string private readonly cols: number @@ -209,9 +235,21 @@ export class OpencodeTerminalSession extends EventEmitter implements AgentSessio env: launch.env, }) this.pty = pty + this.tuiOutput = false + this.heldInput = [] + this.heldInputChars = 0 this.ptyDataSubscription = pty.onData(data => { if (generation !== this.startGeneration || this.pty !== pty || this.exited) return + if (!this.tuiOutput) { + // Latched BEFORE the input is released, so the package never sees + // "no output" once the TUI could have received any. + this.tuiOutput = true + const held = this.heldInput + this.heldInput = [] + this.heldInputChars = 0 + for (const chunk of held) pty.write(chunk) + } // SessionManager already owns a capped attach/replay buffer for agent PTY // bytes. Forwarding the native stream through that channel is what makes // a TUI launched before React mounts appear complete instead of blank. @@ -243,6 +281,8 @@ export class OpencodeTerminalSession extends EventEmitter implements AgentSessio // storm gets whatever a sibling has since resolved for free, and only a // genuinely unresolved path costs another process start. resolveDbPath: () => resolveOpencodeDbPath({ binary: this.binary, env, cwd: this.cwd }), + // Latched from spawn above (see `tuiOutput`). + tuiOutputSeen: () => this.tuiOutput, }) this.headless = headless this.forwardHeadless(headless, pty, launch.server.url) @@ -348,6 +388,9 @@ export class OpencodeTerminalSession extends EventEmitter implements AgentSessio this.pty = null this.headless = null this.exited = true + // Input held for a TUI that exited before painting goes nowhere. + this.heldInput = [] + this.heldInputChars = 0 this.ptyDataSubscription?.dispose() this.ptyDataSubscription = null this.clearReadinessTimer() @@ -357,8 +400,29 @@ export class OpencodeTerminalSession extends EventEmitter implements AgentSessio }) } - write(data: string): void { - this.pty?.write(data) + /** + * `false` means the input was REFUSED: not written, not held. The caller + * (SessionManager.write, then the renderer) tells the user (steering q97: + * never a silent drop). + */ + write(data: string): boolean { + if (!this.pty) return false + // Held until the TUI's first output (see `tuiOutput`): keystrokes typed + // into a pane that has not painted yet must not be able to commit rows + // the package's reader cannot see. They are written, in order, the moment + // the TUI paints. + if (!this.tuiOutput) { + // Bounded (see HELD_INPUT_MAX_*). Past the bound the NEW input is + // refused rather than an old chunk dropped: what was already accepted + // stays in order, and the refusal is reported for exactly the input it + // concerns. + if (this.heldInput.length >= HELD_INPUT_MAX_CHUNKS || this.heldInputChars + data.length > HELD_INPUT_MAX_CHARS) return false + this.heldInput.push(data) + this.heldInputChars += data.length + return true + } + this.pty.write(data) + return true } /** @@ -460,6 +524,9 @@ export class OpencodeTerminalSession extends EventEmitter implements AgentSessio this.importAbort = null this.ptyDataSubscription?.dispose() this.ptyDataSubscription = null + // Input held for a TUI that never painted goes nowhere now. + this.heldInput = [] + this.heldInputChars = 0 this.exited = true this.clearReadinessTimer() const headless = this.headless diff --git a/src/renderer/src/features/conversations/ui/ConversationsPicker.renderer.test.tsx b/src/renderer/src/features/conversations/ui/ConversationsPicker.renderer.test.tsx index 7756ecae8..31dad3c5b 100644 --- a/src/renderer/src/features/conversations/ui/ConversationsPicker.renderer.test.tsx +++ b/src/renderer/src/features/conversations/ui/ConversationsPicker.renderer.test.tsx @@ -410,3 +410,57 @@ describe('ConversationsPicker', () => { }) }) + +// #1430: git timed out listing the repository's worktrees, so main built the +// list from this folder alone. The rows are complete for this folder but may +// be missing the siblings' conversations — said in a muted status line. +describe('ConversationsPicker when git timed out', () => { + it('says conversations from other worktrees may be missing', async () => { + install(vi.fn(async () => response({ family: { repoRoot: '/fixture/repo/.worktrees/extension-platform', roots: ['/fixture/repo/.worktrees/extension-platform'], gitTimedOut: true } }))) + render() + const note = await screen.findByText("Git didn't answer in time. Conversations from this repository's other worktrees may be missing.") + expect(note).toHaveAttribute('role', 'status') + }) + + it('says nothing when git answered', async () => { + install() + render() + await screen.findByText('Project context bootstrapping') + expect(screen.queryByText(/Git didn't answer in time/)).toBeNull() + }) +}) + +// #1430 review a/b. +describe('ConversationsPicker paging across a git timeout (#1430)', () => { + it('starts over from page 1 when git recovers between pages, instead of appending a page from another family', async () => { + const timedOut = { repoRoot: '/fixture/repo/.worktrees/extension-platform', roots: ['/fixture/repo/.worktrees/extension-platform'], gitTimedOut: true as const } + const recovered = { repoRoot: '/fixture/repo', roots: ['/fixture/repo', '/fixture/repo/.worktrees/extension-platform'] } + const worktreeRow = row({ nativeId: 'feature-first', label: 'feature first', cwd: '/fixture/repo/.worktrees/extension-platform' }) + const mainNewest = row({ nativeId: 'main-newest', label: 'main newest' }) + const list = vi.fn(async (request: { cursor?: string | null }) => { + if (list.mock.calls.length === 1) return response({ rows: [worktreeRow], total: 1, hiddenChildren: 0, nextCursor: 'after-feature', family: timedOut }) + if (request.cursor) return response({ rows: [row({ nativeId: 'main-second', label: 'main second' })], total: 3, hiddenChildren: 0, nextCursor: null, family: recovered }) + return response({ rows: [mainNewest, worktreeRow], total: 2, hiddenChildren: 0, nextCursor: null, family: recovered }) + }) + install(list) + render() + // Page 1 (built while git timed out) is on screen; moving down pages in. + expect(await screen.findByText('feature first')).toBeInTheDocument() + expect(screen.getByText(/Git didn't answer in time/)).toBeInTheDocument() + fireEvent.keyDown(screen.getByRole('dialog'), { key: 'ArrowDown' }) + // The recovered page 1 replaces the timed-out one; the row page 1 missed is there. + expect(await screen.findByText('main newest')).toBeInTheDocument() + expect(list.mock.calls.map(c => c[0].cursor ?? null)).toEqual([null, 'after-feature', null]) + await waitFor(() => expect(list.mock.calls.at(-1)?.[0]).toMatchObject({ cursor: null })) + expect(screen.queryByText('main second')).toBeNull() + expect(screen.queryByText(/Git didn't answer in time/)).toBeNull() + }) + + it('says nothing about missing worktrees in Everywhere, where the family removes no rows', async () => { + install(vi.fn(async () => response({ family: { repoRoot: null, roots: [], gitTimedOut: true } }))) + render() + await screen.findByText('Project context bootstrapping') + fireEvent.click(screen.getByRole('button', { name: 'Everywhere' })) + await waitFor(() => expect(screen.queryByText(/Git didn't answer in time/)).toBeNull()) + }) +}) diff --git a/src/renderer/src/features/conversations/ui/ConversationsPicker.tsx b/src/renderer/src/features/conversations/ui/ConversationsPicker.tsx index b5cf77943..502b72330 100644 --- a/src/renderer/src/features/conversations/ui/ConversationsPicker.tsx +++ b/src/renderer/src/features/conversations/ui/ConversationsPicker.tsx @@ -37,6 +37,11 @@ const SCOPES: Array<{ id: ConversationScope; label: string }> = [ { id: 'everywhere', label: 'Everywhere' }, ] +// Fixed words (#1430). Only in 'repository' scope (review b): 'cwd' never +// wants siblings, and 'everywhere' matches every folder, so a timed-out +// sibling list cannot remove a row from either. +export const GIT_TIMED_OUT_NOTE = "Git didn't answer in time. Conversations from this repository's other worktrees may be missing." + export function ConversationsPicker({ open, focusSearch, workspace, onClose }: Props) { const [query, setQuery] = useState('') const [scope, setScope] = useState('repository') @@ -282,6 +287,15 @@ export function ConversationsPicker({ open, focusSearch, workspace, onClose }: P {banner &&
{banner}
} + {/* #1430: git timed out listing this repository's worktrees, so the + list was built from this folder alone (and main did not cache it). + Said, because rows are MISSING, not absent: a muted status line, + not an error, since the next open asks git again. */} + {response?.family.gitTimedOut && scope === 'repository' && ( +
+ {GIT_TIMED_OUT_NOTE} +
+ )}
(null) const [response, setResponse] = useState(null) // The parameters `response` was fetched for, set with it. const [responseKey, setResponseKey] = useState(null) @@ -57,6 +60,18 @@ export function useConversationList(params: ConversationListParams): { includeChildren, query: query.trim() || undefined, cursor, limit: PAGE, }) if (request !== version.current) return + // #1430 review a: a page is only an append of the pages before it when + // both came from the SAME family. Page 1 built while git timed out + // (the cwd alone) and a page 2 built after git recovered (the whole + // repository) interleave differently: the recovered order can place + // rows BEFORE the cursor that page 1 never had, so appending loses them + // for good — and page 2's family (no gitTimedOut) would clear the + // warning while rows are missing. Start over from page 1 instead. + const current = responseRef.current + if (cursor && current && !sameFamily(current.family, next.family)) { + void run(null) + return + } setResponse(prev => (cursor && prev ? { ...next, rows: [...prev.rows, ...next.rows] } : next)) setResponseKey(requestKey) } catch { @@ -73,6 +88,7 @@ export function useConversationList(params: ConversationListParams): { const hasResponse = useRef(false) hasResponse.current = response !== null + responseRef.current = response useEffect(() => { if (!open) { version.current += 1 @@ -98,3 +114,8 @@ export function useConversationList(params: ConversationListParams): { return { response, loading, error, needsPane, loadMore, stale } } + +/** Same family = same repository root, same roots, same git-timeout answer. */ +function sameFamily(a: ConversationListResponse['family'], b: ConversationListResponse['family']): boolean { + return a.repoRoot === b.repoRoot && a.gitTimedOut === b.gitTimedOut && a.roots.length === b.roots.length && a.roots.every((root, i) => root === b.roots[i]) +} diff --git a/src/renderer/src/features/mcp/ui/McpServerDialog.tsx b/src/renderer/src/features/mcp/ui/McpServerDialog.tsx index 724e6f1c1..ce8b3657a 100644 --- a/src/renderer/src/features/mcp/ui/McpServerDialog.tsx +++ b/src/renderer/src/features/mcp/ui/McpServerDialog.tsx @@ -22,7 +22,7 @@ import type { UserMcpServerEntry, UserMcpServerView, } from '@shared/userMcp/types' -import { USER_MCP_PROVIDERS } from '@shared/userMcp/types' +import { USER_MCP_PROVIDERS, type UserMcpSecretState } from '@shared/userMcp/types' import { providerSupportForEntry, transportOf, userMcpDestination } from '@shared/userMcp/validate' const PROVIDER_LABEL: Record = { claude: 'Claude', codex: 'Codex' } @@ -486,6 +486,11 @@ function EditServer({ server, onDone, onCancel, onDirty, onSaving }: { server: U values={secretEdits} states={secretStates} onChange={setSecretEdits} + onConfirm={destinationChanged ? undefined : async inputId => { + setError(null) + const result = await window.api.userMcpConfirmSecret(server.id, inputId) + if (!result.ok) setError(result.error) + }} /> ) : null} {destinationChanged && inputs.length > 0 ? ( @@ -531,11 +536,14 @@ function SecretFields({ values, states, onChange, + onConfirm, }: { inputs: UserMcpInput[] values: Record - states: Record + states: Record onChange: (values: Record) => void + /** Confirm a withheld secret for this server as it is now (q114, #1420 B6 R3). */ + onConfirm?: (inputId: string) => void }) { return (
@@ -545,6 +553,8 @@ function SecretFields({ const edited = values[input.id] const placeholder = edited === '' ? 'cleared on save' + : state?.unconfirmed + ? `${state.unconfirmed === 'legacy' ? 'saved by an earlier version' : 'withheld: an agent changed another value'}${state.hint ? ` (…${state.hint})` : ''}: confirm or re-enter` : state?.set ? `set${state.hint ? ` (…${state.hint})` : ''} — type to replace` : 'not set' @@ -568,6 +578,9 @@ function SecretFields({ className="h-7 flex-1" aria-label={`Secret ${input.id}`} /> + {state?.unconfirmed && edited === undefined && onConfirm ? ( + + ) : null} {state?.set && edited === undefined ? ( ) : null} diff --git a/src/renderer/src/features/worktrees/control.renderer.test.ts b/src/renderer/src/features/worktrees/control.renderer.test.ts index 9d9751994..385699376 100644 --- a/src/renderer/src/features/worktrees/control.renderer.test.ts +++ b/src/renderer/src/features/worktrees/control.renderer.test.ts @@ -36,3 +36,31 @@ describe('worktrees.read and a git timeout', () => { expect(await read({ ok: false, gitMissing: false })).toMatchObject({ gitUnavailable: true, gitMissing: false, gitTimedOut: false }) }) }) + +// #1430: git answered the status, but listing worktrees for the activity index +// then timed out. That used to read as "activity unavailable", the same as a +// missing index; it now says the git timeout, to agents and in the dump. +describe('worktrees.read when the activity lookup times out', () => { + async function readWithActivity(activity: unknown) { + const gitWorktreeStatus = vi.fn(async () => ({ ok: true, worktrees: [] })) + Object.defineProperty(window, 'api', { configurable: true, value: { gitWorktreeStatus, worktreeActivitySummary: vi.fn(async () => activity) } }) + useAppStore.setState({ workspaceState: { ...originalStore.workspaceState, sessions: { agent: { cwd: '/repo', kind: 'claude' } } } } as never) + const workspace = { state: { tabs: [], sessions: {}, pinnedSessionIds: [] }, runtimes: {} } as unknown as Workspace + const [capability] = worktreeControlCapabilities(() => workspace) + const result = await capability!.execute({ sessionId: 'agent' }, {} as never) + expect(result.ok).toBe(true) + return (result as { value: Record }).value + } + + it('says a git timeout apart from a missing activity index', async () => { + expect(await readWithActivity({ ok: false, timedOut: true })).toMatchObject({ activityUnavailable: true, activityTimedOut: true }) + expect(await readWithActivity({ ok: false })).toMatchObject({ activityUnavailable: true, activityTimedOut: false }) + }) + + it('and the text dump says it too', async () => { + const { formatWorktreeDump } = await import('./lib/formatWorktreeDump') + const base = { cwd: '/repo', generatedAt: 0, rows: [], indexStatus: null, gitUnavailable: false, gitMissing: false, activityUnavailable: true } + expect(formatWorktreeDump({ ...base, activityTimedOut: true } as never)).toContain('- Agent activity: unavailable (Git timed out)') + expect(formatWorktreeDump(base as never)).toContain('- Agent activity: unavailable\n') + }) +}) diff --git a/src/renderer/src/features/worktrees/control.ts b/src/renderer/src/features/worktrees/control.ts index 443a1cc67..a48aebb33 100644 --- a/src/renderer/src/features/worktrees/control.ts +++ b/src/renderer/src/features/worktrees/control.ts @@ -8,7 +8,7 @@ export function worktreeControlCapabilities(getWorkspace: () => Workspace) { return [defineCapability({ id: 'worktrees.read', title: 'Read worktree status and agent activity', execution: 'window', effect: 'read', target: { kind: 'session', field: 'sessionId' }, description: 'Read the Worktrees panel data for an exact agent’s repository, including branch/path, Git status, indexed activity and associated live agents. Bounded pages use a revision; changing Git/activity state requires a fresh read. Explicitly reports missing Git, a Git timeout (gitTimedOut: slow, not a verdict on the repository), non-repository and activity-index unavailability. Does not create, delete or change worktrees, or wake agents.', input: z.object({ sessionId: z.string(), refreshActivity: z.boolean().default(false), ...pageInput }).strict(), - output: pageSchema(z.json()).extend({ cwd: z.string(), generatedAt: z.number(), gitUnavailable: z.boolean(), gitMissing: z.boolean(), gitTimedOut: z.boolean(), activityUnavailable: z.boolean(), indexStatus: z.json().nullable() }), + output: pageSchema(z.json()).extend({ cwd: z.string(), generatedAt: z.number(), gitUnavailable: z.boolean(), gitMissing: z.boolean(), gitTimedOut: z.boolean(), activityUnavailable: z.boolean(), activityTimedOut: z.boolean(), indexStatus: z.json().nullable() }), handler: async input => { const workspace = getWorkspace(), meta = useAppStore.getState().workspaceState.sessions[input.sessionId] if (!meta) throw new ControlError('unavailable', 'Agent no longer exists') @@ -18,6 +18,8 @@ export function worktreeControlCapabilities(getWorkspace: () => Workspace) { // #1429 review a, b: without it a timed-out list reached agents as // "not a repository", the exact misreading the panel now avoids. gitTimedOut: dump.gitTimedOut === true, activityUnavailable: dump.activityUnavailable, + // #1430: the activity index was not missing; git timed out listing worktrees. + activityTimedOut: dump.activityTimedOut === true, indexStatus: z.json().parse(JSON.parse(JSON.stringify(dump.indexStatus))) } }, })] diff --git a/src/renderer/src/features/worktrees/lib/formatWorktreeDump.ts b/src/renderer/src/features/worktrees/lib/formatWorktreeDump.ts index ebcc738ad..7093bee50 100644 --- a/src/renderer/src/features/worktrees/lib/formatWorktreeDump.ts +++ b/src/renderer/src/features/worktrees/lib/formatWorktreeDump.ts @@ -45,7 +45,7 @@ export function formatWorktreeDump(dump: WorktreeDump): string { lines.push(`- Patch-equivalent: ${countRows(dump.rows, row => row.category === 'patch-equivalent')}`) lines.push(`- Cleanup/merged: ${countRows(dump.rows, row => row.category === 'cleanup-merged')}`) lines.push(`- Detached: ${countRows(dump.rows, row => row.detached)}`) - lines.push(`- Agent activity: ${dump.activityUnavailable ? 'unavailable' : 'available'}`) + lines.push(`- Agent activity: ${activityLabel(dump)}`) if (dump.indexStatus?.lastIndexedAt) { // Same #495 A15 rationale as the Generated line above. lines.push(`- Activity index updated: ${new Date(dump.indexStatus.lastIndexedAt).toISOString()}`) @@ -143,3 +143,9 @@ export function providerLabel(kind: SessionKind): string { function formatLiveAgent(agent: WorktreeDumpRow['liveAgents'][number]): string { return `${providerLabel(agent.kind)} ${agent.live ? 'active' : 'open'} in "${agent.tabTitle}"` } + +/** #1430: a git timeout is said as one, never as a missing activity index. */ +function activityLabel(dump: WorktreeDump): string { + if (!dump.activityUnavailable) return 'available' + return dump.activityTimedOut ? 'unavailable (Git timed out)' : 'unavailable' +} diff --git a/src/renderer/src/features/worktrees/lib/loadWorktreeDump.ts b/src/renderer/src/features/worktrees/lib/loadWorktreeDump.ts index d4d8f84dd..609098924 100644 --- a/src/renderer/src/features/worktrees/lib/loadWorktreeDump.ts +++ b/src/renderer/src/features/worktrees/lib/loadWorktreeDump.ts @@ -39,6 +39,9 @@ export type WorktreeDump = { * (#1250 row 11). */ gitTimedOut?: boolean activityUnavailable: boolean + /** activityUnavailable because git timed out while the activity index asked + * for this repository's worktrees (#1430), not because there is no index. */ + activityTimedOut?: boolean } export async function loadWorktreeDump(params: { @@ -92,6 +95,7 @@ export async function loadWorktreeDump(params: { gitUnavailable: false, gitMissing: false, activityUnavailable: !activityResult.ok, + activityTimedOut: !activityResult.ok && 'timedOut' in activityResult && activityResult.timedOut === true, } } diff --git a/src/renderer/src/workspace/hook/actions/agentIndexNavigation.renderer.test.tsx b/src/renderer/src/workspace/hook/actions/agentIndexNavigation.renderer.test.tsx index e97bac952..7ae60a6c6 100644 --- a/src/renderer/src/workspace/hook/actions/agentIndexNavigation.renderer.test.tsx +++ b/src/renderer/src/workspace/hook/actions/agentIndexNavigation.renderer.test.tsx @@ -44,6 +44,7 @@ function makeRefs(state: WorkspaceState): WorkspaceRefs { seenUuidsRef: ref({}), historyWindowsRef: { current: {} } as never, historyAwaitingTurnStartRef: { current: new Set() } as never, + worktreeReconcilerRef: { current: null }, undoStackRef: ref(new UndoCloseStack()), bootstrapTimersRef: ref(new Map()), persistedFeedDebugIdRef: ref({}), diff --git a/src/renderer/src/workspace/hook/actions/history.renderer.test.tsx b/src/renderer/src/workspace/hook/actions/history.renderer.test.tsx index 61ed5db2a..37895d710 100644 --- a/src/renderer/src/workspace/hook/actions/history.renderer.test.tsx +++ b/src/renderer/src/workspace/hook/actions/history.renderer.test.tsx @@ -206,3 +206,76 @@ describe('what an older-history request reports', () => { expect(answer).toBe('skipped') }) }) + +// #1430 review c: the older-history loader's half of the round-1 fix was +// untested. A page read while `git worktree list` timed out must reach the live +// reconciler (so a recovered catalog replays it), and must NOT be attributed +// against a null family (ingestWorktreeRawEvent would throw on it). +async function loadOlderPageWhileGitTimesOut(initial: Partial = {}) { + let runtimes: Record = { + session: { ...emptyRuntime(), hasOlderHistory: true, historyOldestMarker: 'anchor', ...initial }, + } + const observed: unknown[][] = [] + const positions: Array = [] + const refresh = vi.fn(async () => 'failed' as const) + const refs = { + stateRef: ref({ sessions: { session: { kind: 'claude', cwd: '/tmp/project', providerSessionId: 'provider-session' } } }), + latestRuntimesRef: ref(runtimes), + seenUuidsRef: ref({}), + worktreeReconcilerRef: ref({ + observe: (_s: string, _c: string, entries: Array<{ entry: unknown }>, projection: unknown, position?: string) => { observed.push(entries.map(e => e.entry)); positions.push(position); return projection }, + refresh, + replayCachedCatalog: vi.fn(), + }), + } as unknown as WorkspaceRefs + const setRuntimes: WorkspaceSetRuntimes = next => { + runtimes = typeof next === 'function' ? next(runtimes) : next + refs.latestRuntimesRef.current = runtimes + } + const updateRuntime = (id: string, patch: Partial) => { + setRuntimes(prev => ({ ...prev, [id]: { ...prev[id]!, ...patch } })) + } + const older = { + type: 'assistant', + uuid: 'older-1', + timestamp: '2026-09-20T09:05:00.000Z', + message: { role: 'assistant', content: [{ type: 'tool_use', id: 'toolu_1', name: 'Write', input: { file_path: '/tmp/project/.worktrees/x/a.ts' } }] }, + } + Object.defineProperty(window, 'api', { configurable: true, value: { + loadOlderHistory: vi.fn().mockResolvedValue({ entries: [{ entries: [older], historyMarker: 'older-1' }], hasMore: false }), + gitWorktrees: vi.fn(async () => ({ ok: false, gitMissing: false, timedOut: true })), + } }) + const { result } = renderHook(() => useHistoryActions(setRuntimes, refs, updateRuntime, ipcSessionFeed)) + let outcome: unknown + await act(async () => { outcome = await result.current.loadOlderHistory('session') }) + return { outcome, observed, positions, refresh, older, runtime: () => runtimes.session } +} + +describe('an older page read while git timed out (#1430)', () => { + it('hands the page to the reconciler, asks it to refresh, and attributes nothing', async () => { + const { outcome, observed, positions, refresh, older, runtime } = await loadOlderPageWhileGitTimesOut() + + expect(outcome).not.toBe('failed') + expect(observed).toEqual([[{ entries: [older], historyMarker: 'older-1' }]]) + // As the OLDEST evidence, never the newest (#1450 B6 verify). + expect(positions).toEqual(['older']) + expect(refresh).toHaveBeenCalledWith('/tmp/project') + // Nothing was attributed against the unknown family: no activity folded + // from the page, no work context derived from it. + expect(runtime()?.workActivity).toBeNull() + expect(runtime()?.workContext).toBeNull() + }) + + it('never hands an older page over when the pane already knows a newer context (#1450 verification b)', async () => { + // The answered-git path backfills only an UNKNOWN context: older records + // must never replace fresher evidence. Handing the page to the reconciler + // appended it as if it were the newest evidence, so a recovered catalog + // moved the pane back to the older worktree. + const known = { worktreePath: '/tmp/project/.worktrees/newer' } as unknown as SessionRuntime['workContext'] + const { observed, refresh, runtime } = await loadOlderPageWhileGitTimesOut({ workContext: known }) + + expect(observed).toEqual([]) + expect(refresh).not.toHaveBeenCalled() + expect(runtime()?.workContext).toBe(known) + }) +}) diff --git a/src/renderer/src/workspace/hook/actions/history.ts b/src/renderer/src/workspace/hook/actions/history.ts index d41f1d92c..d341484f9 100644 --- a/src/renderer/src/workspace/hook/actions/history.ts +++ b/src/renderer/src/workspace/hook/actions/history.ts @@ -26,6 +26,8 @@ import type { WorkspaceSetRuntimes } from '@renderer/workspace/hook/context' import type { WorkspaceRefs } from '@renderer/workspace/hook/refs' import * as perf from '@renderer/performance/client' import type { SessionFeed } from '@shared/sessionFeed/SessionFeed' +import { worktreesForAttribution } from '@renderer/workspace/work-context/worktreesForAttribution' +import { handHistoryToReconciler } from '@renderer/workspace/hook/actions/initialHistory' // Older history loader — called by Feed's scroll handler when the // user scrolls near the top. @@ -109,7 +111,17 @@ export function useHistoryActions( } const prepend: Entry[] = [] const worktreesResult = await window.api.gitWorktrees(meta.cwd) - const worktrees = worktreesResult.ok ? worktreesResult.worktrees : [] + // #1430: null = git timed out, family unknown → skip attribution (see + // worktreesForAttribution), and hand the page to the reconciler so a + // recovered catalog can still attribute it (handHistoryToReconciler). + // + // Only while the context is still UNKNOWN, the same recency rule as + // the answered-git backfill below (#1450 verification b): the + // reconciler appends what it observes as the NEWEST evidence, so an + // older page handed over while the pane already knew a newer + // worktree moved it back to the older one once git recovered. + const worktrees = worktreesForAttribution(worktreesResult) + if (worktrees === null && !runtime.workContext) handHistoryToReconciler(refs, sessionId, meta.cwd, chunk.entries, 'older') let workActivity = runtime.workActivity let workContext = runtime.workContext let oldestMarker: string | null = runtime.historyOldestMarker @@ -130,7 +142,7 @@ export function useHistoryActions( // Older-history pagination walks records that predate the current // tail. Use them only to backfill an unknown badge; never let old // worktree evidence replace fresher live/current context. - if (!workContext) { + if (!workContext && worktrees !== null) { workActivity = ingestWorktreeRawEvent({ state: workActivity, raw: rawEntry, diff --git a/src/renderer/src/workspace/hook/actions/historyWorktreeTimeout.renderer.test.ts b/src/renderer/src/workspace/hook/actions/historyWorktreeTimeout.renderer.test.ts new file mode 100644 index 000000000..d60f6ddd0 --- /dev/null +++ b/src/renderer/src/workspace/hook/actions/historyWorktreeTimeout.renderer.test.ts @@ -0,0 +1,122 @@ +import { readFileSync } from 'node:fs' +import { resolve } from 'node:path' + +import { describe, expect, it } from 'vitest' + +import { emptyRuntime, type SessionRuntime } from '@renderer/session-runtime/state' +import { LiveWorktreeReconciler } from '@renderer/workspace/work-context/LiveWorktreeReconciler' +import { handHistoryToReconciler } from './initialHistory' + +// #1430 review a/b: a history chunk read while `git worktree list` timed out +// was skipped for worktree attribution, and nothing else ever saw it: the live +// reconciler replays only what it observed, so a quiet session whose writes were +// in a linked worktree stayed on the launch folder after git recovered. The +// loaders now hand such a chunk to the reconciler (handHistoryToReconciler), +// whose window replays it when the catalog answers. +// +// Recorded data: the Codex worktree window (13 records, main +// /fixture/project-1, the agent's actual worktree .../worktree-2) and the +// recorded `git worktree list` identities. +const fixtureDir = resolve(process.cwd(), 'testing', 'fixtures', 'worktree-live-attribution') +const codex = JSON.parse(readFileSync(resolve(fixtureDir, 'codex-0151-worktree-window.json'), 'utf8')) as { + git: { main: { path: string }; ui?: { path: string; branch: string } } + records: unknown[] +} +const catalog = (JSON.parse(readFileSync(resolve(fixtureDir, 'git-worktree-identities.json'), 'utf8')) as { + worktrees: Array<{ path: string; branch: string; detached: boolean }> +}).worktrees.map(worktree => ({ ...worktree, head: null })) + +describe('history read while git timed out (#1430 review a/b)', () => { + it('reaches the worktree the agent wrote in once git answers', async () => { + let gitAnswers = false + let runtime: SessionRuntime = emptyRuntime() + let reconciler!: LiveWorktreeReconciler + reconciler = new LiveWorktreeReconciler({ + loadWorktrees: async () => gitAnswers ? { ok: true, worktrees: catalog } : { ok: false, gitMissing: false, timedOut: true } as never, + onCatalogReady: cwd => { + const projection = reconciler.project({ sessionId: 'resumed', cwd, projection: runtime }) + runtime = { ...runtime, ...projection } + }, + }) + const refs = { worktreeReconcilerRef: { current: reconciler }, latestRuntimesRef: { current: { resumed: runtime } } } + + // The initial history load: git timed out, so the chunk is handed over. + handHistoryToReconciler(refs as never, 'resumed', codex.git.main.path, codex.records) + // The refresh the loader asked for still sees git time out; a failed probe + // is not cached, so the next refresh (any live batch or catalog event) + // asks again — and git has recovered by then. + expect(await reconciler.refresh(codex.git.main.path)).toBe('failed') + expect(runtime.workContext).toBeNull() + gitAnswers = true + expect(await reconciler.refresh(codex.git.main.path)).toBe('ready') + + expect(runtime.workContext?.worktreePath).toBe(codex.git.ui?.path) + }) + + it('repaints at once when the reconciler already holds a fresh catalog (#1450 verification a)', async () => { + // A live event loaded the catalog before this pane's history arrived; only + // the history's own gitWorktrees call timed out. refresh() then answers + // 'cached' and never calls onCatalogReady, so without a replay the chunk + // sat in the window and the pane stayed on the launch folder. + let runtime: SessionRuntime = emptyRuntime() + let reconciler!: LiveWorktreeReconciler + reconciler = new LiveWorktreeReconciler({ + loadWorktrees: async () => ({ ok: true, worktrees: catalog }), + onCatalogReady: cwd => { + const projection = reconciler.project({ sessionId: 'resumed', cwd, projection: runtime }) + runtime = { ...runtime, ...projection } + }, + }) + expect(await reconciler.refresh(codex.git.main.path)).toBe('ready') + expect(runtime.workContext?.worktreePath).not.toBe(codex.git.ui?.path) + const refs = { worktreeReconcilerRef: { current: reconciler }, latestRuntimesRef: { current: { resumed: runtime } } } + + handHistoryToReconciler(refs as never, 'resumed', codex.git.main.path, codex.records) + await Promise.resolve() + await Promise.resolve() + + expect(runtime.workContext?.worktreePath).toBe(codex.git.ui?.path) + }) + + it('an older page read during the timeout never outranks the newest chunk (#1450 B6 verify)', async () => { + // B6's sequence: the initial history (newest) is handed over while git + // times out; the user scrolls up while it still times out, and the older + // page is handed over too; git recovers. The older page must stay OLDER + // evidence: the pane belongs where the newest records put it. + // + // Older page = the recorded worktree-2 window. Newest chunk = the same + // recorded records moved to worktree-1 and 1 h later (only cwd and + // timestamp change, so the record shape is the recorded one). + const worktree1 = catalog.find(w => w.path.endsWith('/worktree-1'))!.path + const newest = codex.records.map(record => { + const r = structuredClone(record) as { timestamp?: string; payload?: { item?: { cwd?: string } } } + if (r.timestamp) r.timestamp = new Date(Date.parse(r.timestamp) + 3_600_000).toISOString() + if (r.payload?.item?.cwd) r.payload.item.cwd = `file://${worktree1}` + return r + }) + let gitAnswers = false + let runtime: SessionRuntime = emptyRuntime() + let reconciler!: LiveWorktreeReconciler + reconciler = new LiveWorktreeReconciler({ + loadWorktrees: async () => gitAnswers ? { ok: true, worktrees: catalog } : { ok: false, gitMissing: false, timedOut: true } as never, + onCatalogReady: cwd => { + const projection = reconciler.project({ sessionId: 'resumed', cwd, projection: runtime }) + runtime = { ...runtime, ...projection } + }, + }) + const refs = { worktreeReconcilerRef: { current: reconciler }, latestRuntimesRef: { current: { resumed: runtime } } } + + handHistoryToReconciler(refs as never, 'resumed', codex.git.main.path, newest) + expect(await reconciler.refresh(codex.git.main.path)).toBe('failed') + handHistoryToReconciler(refs as never, 'resumed', codex.git.main.path, codex.records, 'older') + expect(await reconciler.refresh(codex.git.main.path)).toBe('failed') + gitAnswers = true + expect(await reconciler.refresh(codex.git.main.path)).toBe('ready') + + expect(runtime.workContext?.worktreePath).toBe(worktree1) + }) + + it('does nothing without a reconciler or without records', () => { + expect(() => handHistoryToReconciler({ worktreeReconcilerRef: { current: null }, latestRuntimesRef: { current: {} } } as never, 's', '/x', [{}])).not.toThrow() + }) +}) diff --git a/src/renderer/src/workspace/hook/actions/initialHistory.renderer.test.tsx b/src/renderer/src/workspace/hook/actions/initialHistory.renderer.test.tsx index 965cf8898..ff5ba1d6c 100644 --- a/src/renderer/src/workspace/hook/actions/initialHistory.renderer.test.tsx +++ b/src/renderer/src/workspace/hook/actions/initialHistory.renderer.test.tsx @@ -97,3 +97,48 @@ describe('the initial-history loader under failure and repetition', () => { expect(gated).toHaveBeenCalledTimes(3) }) }) + +// #1430 review a/b: when `git worktree list` times out during the initial +// load, the chunk is not attributed against an empty family — and it is not +// dropped either: the loader hands its records to the live reconciler, whose +// window replays them once git answers (handHistoryToReconciler). +describe('the initial-history loader when git times out (#1430)', () => { + it('hands the chunk to the worktree reconciler and asks it to refresh', async () => { + const { history } = emptySession() + const pane = rehydratedPane() + const observed: unknown[][] = [] + const refresh = vi.fn(async () => 'failed' as const) + pane.refs.worktreeReconcilerRef.current = { + observe: (_sessionId, _cwd, entries, projection) => { observed.push(entries.map(e => e.entry)); return projection }, + refresh, + replayCachedCatalog: vi.fn(), + } + scope.extendApi({ + gitWorktrees: async () => ({ ok: false, gitMissing: false, timedOut: true }), + loadInitialHistory: async (request: { cwd: string; providerSessionId: string; limit: number }) => { + const chunk = await history.loadInitialHistory(request) + return { ...chunk, entries: [{ type: 'recorded-row', n: 1 }, ...chunk.entries] } + }, + }) + await loadInitialHistoryForSession({ sessionId: SESSION_ID, meta: pane.meta, refs: pane.refs, setRuntimes: pane.setRuntimes }) + expect(observed).toHaveLength(1) + expect(observed[0]![0]).toEqual({ type: 'recorded-row', n: 1 }) + expect(refresh).toHaveBeenCalledWith(pane.meta.cwd) + }) + + it('does not hand anything over when git answered', async () => { + const { history } = emptySession() + const pane = rehydratedPane() + const observe = vi.fn() + pane.refs.worktreeReconcilerRef.current = { observe, refresh: vi.fn(), replayCachedCatalog: vi.fn() } + scope.extendApi({ + gitWorktrees: async () => ({ ok: true, worktrees: [] }), + loadInitialHistory: async (request: { cwd: string; providerSessionId: string; limit: number }) => { + const chunk = await history.loadInitialHistory(request) + return { ...chunk, entries: [{ type: 'recorded-row', n: 1 }, ...chunk.entries] } + }, + }) + await loadInitialHistoryForSession({ sessionId: SESSION_ID, meta: pane.meta, refs: pane.refs, setRuntimes: pane.setRuntimes }) + expect(observe).not.toHaveBeenCalled() + }) +}) diff --git a/src/renderer/src/workspace/hook/actions/initialHistory.ts b/src/renderer/src/workspace/hook/actions/initialHistory.ts index ebcb2fca7..32a225291 100644 --- a/src/renderer/src/workspace/hook/actions/initialHistory.ts +++ b/src/renderer/src/workspace/hook/actions/initialHistory.ts @@ -1,6 +1,7 @@ import { DEFAULT_PROVIDER, isAgentProviderKind } from '@shared/types/providerKind' import type { Entry } from '@shared/types/transcript' import { emptyRuntime } from '@renderer/session-runtime/state' +import { worktreesForAttribution } from '@renderer/workspace/work-context/worktreesForAttribution' import type { SessionRuntime } from '@renderer/session-runtime/state' import type { SessionId, SessionMeta } from '@renderer/workspace/types' import { getRendererProviderCapabilities } from '@providers/registry.renderer.capabilities' @@ -300,11 +301,14 @@ export async function loadInitialHistoryForSession({ historyRead, window.api.gitWorktrees(meta.cwd), ]) - const worktrees = worktreesResult.ok ? worktreesResult.worktrees : [] + // #1430: null = git timed out, family unknown → skip attribution for this + // chunk (see worktreesForAttribution). + const worktrees = worktreesForAttribution(worktreesResult) if (superseded()) { span.end({ fetched: chunk.entries.length, hasMore: chunk.hasMore, superseded: true }) return settleSuperseded() } + if (worktrees === null) handHistoryToReconciler(refs, sessionId, meta.cwd, chunk.entries) setRuntimes(prev => { const current = prev[sessionId] @@ -339,13 +343,15 @@ export async function loadInitialHistoryForSession({ const toolResultIndex = current.toolResultIndex for (const [rawIndex, raw] of chunk.entries.entries()) { - workActivity = ingestWorktreeRawEvent({ - state: workActivity, - raw, - worktrees, - sessionCwd: meta.cwd, - }) - workContext = deriveAgentWorkContext(workActivity) + if (worktrees !== null) { + workActivity = ingestWorktreeRawEvent({ + state: workActivity, + raw, + worktrees, + sessionCwd: meta.cwd, + }) + workContext = deriveAgentWorkContext(workActivity) + } const { entries: mapped, historyMarker: marker } = mapper.map(raw) // Marker policy (site-owned): the FIRST kept line of the @@ -586,3 +592,39 @@ export function reconcileStuckTranscriptLoads({ } return reKicked } + +/** + * A history chunk read while `git worktree list` timed out (#1430 review a/b). + * + * WHY hand it to the live reconciler instead of skipping it: skipping avoided + * a wrong attribution (against an empty family) but threw the chunk's worktree + * evidence away for good — the reconciler only replays what it observed, so a + * quiet session whose writes were in a linked worktree stayed on the launch + * folder after git recovered. observe() keeps the chunk's RELEVANT records in + * its bounded window (deferred while no catalog is cached) and refresh() asks + * git again; when the catalog lands, onCatalogReady replays the window against + * it and repaints the pane. Outside any setState updater, because observe is + * a side effect and an updater may run twice. The current runtime is the + * baseline, as for a live batch. + */ +export function handHistoryToReconciler( + refs: Pick, + sessionId: SessionId, + cwd: string, + entries: readonly unknown[], + // 'older' for an older-history page: it enters the reconciler's window as + // the OLDEST evidence, so a scroll-up during a git timeout never outranks + // the newest chunk (#1450 B6 verify). + position: 'newest' | 'older' = 'newest', +): void { + const reconciler = refs.worktreeReconcilerRef.current + if (!reconciler || entries.length === 0) return + reconciler.observe(sessionId, cwd, entries.map(entry => ({ entry })), refs.latestRuntimesRef.current[sessionId] ?? emptyRuntime(), position) + // 'cached' means a fresh catalog was already there, so no onCatalogReady is + // coming: replay now or the chunk waits for an unrelated event (#1450 + // verification a). 'ready' already replayed; 'failed' is retried by the + // next refresh, which notifies when git answers. + void reconciler.refresh(cwd).then(outcome => { + if (outcome === 'cached') reconciler.replayCachedCatalog(cwd) + }) +} diff --git a/src/renderer/src/workspace/hook/actions/sessionReplacementHandoff.renderer.test.tsx b/src/renderer/src/workspace/hook/actions/sessionReplacementHandoff.renderer.test.tsx index ffdf3e637..14151d937 100644 --- a/src/renderer/src/workspace/hook/actions/sessionReplacementHandoff.renderer.test.tsx +++ b/src/renderer/src/workspace/hook/actions/sessionReplacementHandoff.renderer.test.tsx @@ -71,6 +71,7 @@ describe('renderer session replacement handoff', () => { seenUuidsRef: ref({}), historyWindowsRef: { current: {} } as never, historyAwaitingTurnStartRef: { current: new Set() } as never, + worktreeReconcilerRef: { current: null }, undoStackRef: ref(new UndoCloseStack()), bootstrapTimersRef: ref(new Map()), persistedFeedDebugIdRef: ref({}), diff --git a/src/renderer/src/workspace/hook/actions/testing/paneActionsHarness.tsx b/src/renderer/src/workspace/hook/actions/testing/paneActionsHarness.tsx index ae67475c5..a0fa57d73 100644 --- a/src/renderer/src/workspace/hook/actions/testing/paneActionsHarness.tsx +++ b/src/renderer/src/workspace/hook/actions/testing/paneActionsHarness.tsx @@ -43,6 +43,7 @@ export function makeRefs(state: WorkspaceState): WorkspaceRefs { seenUuidsRef: ref({}), historyWindowsRef: { current: {} } as never, historyAwaitingTurnStartRef: { current: new Set() } as never, + worktreeReconcilerRef: { current: null }, undoStackRef: ref(new UndoCloseStack()), bootstrapTimersRef: ref(new Map()), persistedFeedDebugIdRef: ref({}), diff --git a/src/renderer/src/workspace/hook/ipc/testing/opencodeTerminalPane.tsx b/src/renderer/src/workspace/hook/ipc/testing/opencodeTerminalPane.tsx index 33cad2510..d0463ea72 100644 --- a/src/renderer/src/workspace/hook/ipc/testing/opencodeTerminalPane.tsx +++ b/src/renderer/src/workspace/hook/ipc/testing/opencodeTerminalPane.tsx @@ -89,18 +89,14 @@ import { makeWorkspaceRefsForTest } from './workspaceRefsForTest' const USERNAME = 'opencode' const PASSWORD = 'renderer-replay' -/** A caller-owned PTY that can also paint, like the TUI's first frame. */ +/** A caller-owned PTY that can also paint, like the TUI's first frame. The + * data subscription is the package's own `FakePty` one since + * opencode-terminal-headless#10; this used to duplicate it. */ export class AdapterPty extends FakePty { readonly kill = vi.fn(() => this.exit(0, 15)) - private readonly dataListeners = new Set<(data: string) => void>() - - onData(listener: (data: string) => void): { dispose(): void } { - this.dataListeners.add(listener) - return { dispose: () => { this.dataListeners.delete(listener) } } - } paint(data: string): void { - for (const listener of [...this.dataListeners]) listener(data) + this.output(data) } } @@ -232,7 +228,12 @@ export type RecordedPaneOptions = { * Where the durable channel reads. Omitted: a fresh database the replay * writes into. A string: that file as it is (a database the store refuses, * say). `{ error }`: OpenCode could not tell the launch where its database - * is, which is how `prepareOpencodeTerminalLaunch` reports it. + * is, as a host-built launch reports it (`dbPath: null` + `dbPathError`). + * NOTE: since opencode-terminal-headless#10 (#1114) the real + * `prepareOpencodeTerminalLaunch` never sets `dbPathError`; it returns the + * lookup as `dbPathPending` and a failure is that promise's rejection. This + * harness builds its own launch, so it keeps the older shape, which the + * package still honours for host-built launches. */ database?: string | { error: string } /** @@ -251,6 +252,9 @@ export type RecordedPaneOptions = { * Launch DARK and let the path turn up later (#1114, #1117): the launch * could not resolve OpenCode's database path, so the durable channel starts * closed and the package retries `resolveOpencodeDbPath` on this ladder. + * (Modelled as the pre-#10 launch shape: a known failure rather than a + * pending lookup. The package runs the same ladder after a pending lookup + * rejects.) * The fresh database is still created and written by the replay, exactly as * the TUI keeps committing to it while the pane cannot read it. The test * controls when the resolver starts answering (it mocks the package's diff --git a/src/renderer/src/workspace/hook/ipc/testing/workspaceRefsForTest.ts b/src/renderer/src/workspace/hook/ipc/testing/workspaceRefsForTest.ts index 981814b13..534d7adf8 100644 --- a/src/renderer/src/workspace/hook/ipc/testing/workspaceRefsForTest.ts +++ b/src/renderer/src/workspace/hook/ipc/testing/workspaceRefsForTest.ts @@ -22,6 +22,7 @@ export function makeWorkspaceRefsForTest(state: WorkspaceState): WorkspaceRefs { seenUuidsRef: ref({}), historyWindowsRef: { current: {} } as never, historyAwaitingTurnStartRef: { current: new Set() } as never, + worktreeReconcilerRef: { current: null } as never, undoStackRef: ref(new UndoCloseStack()), bootstrapTimersRef: ref(new Map()), persistedFeedDebugIdRef: ref({}), diff --git a/src/renderer/src/workspace/hook/ipc/useIpcSubscriptions.ts b/src/renderer/src/workspace/hook/ipc/useIpcSubscriptions.ts index 256b8b013..d6004812c 100644 --- a/src/renderer/src/workspace/hook/ipc/useIpcSubscriptions.ts +++ b/src/renderer/src/workspace/hook/ipc/useIpcSubscriptions.ts @@ -616,6 +616,10 @@ export function useIpcSubscriptions( refs.seenUuidsRef.current, ) }, MEMORY_GAUGE_INTERVAL_MS) + // Published for the history loaders (#1430 review a/b): a chunk read while + // git timed out is handed here instead of being dropped. See + // WorkspaceRefs.worktreeReconcilerRef. + refs.worktreeReconcilerRef.current = worktreeReconciler const refreshWorktrees = (cwd: string | null | undefined): void => { // The reconciler owns failure/retry/coalescing. This listener intentionally // does not await decorative Git metadata and therefore cannot delay a @@ -2830,6 +2834,7 @@ export function useIpcSubscriptions( }) return () => { + if (refs.worktreeReconcilerRef.current === worktreeReconciler) refs.worktreeReconcilerRef.current = null worktreeReconciler.dispose() window.clearInterval(orphanSweepTimer) window.clearInterval(memoryGaugeTimer) diff --git a/src/renderer/src/workspace/hook/persistence/codexLiveContinuity.renderer.test.tsx b/src/renderer/src/workspace/hook/persistence/codexLiveContinuity.renderer.test.tsx index dd18bdbd0..709d2dd4f 100644 --- a/src/renderer/src/workspace/hook/persistence/codexLiveContinuity.renderer.test.tsx +++ b/src/renderer/src/workspace/hook/persistence/codexLiveContinuity.renderer.test.tsx @@ -219,6 +219,7 @@ function makeRefs(state: WorkspaceState, runtimes: Record> + /** The live worktree reconciler, owned by useIpcSubscriptions' effect and + * published here (null outside it). #1430 review a/b: a history chunk read + * while `git worktree list` timed out cannot be attributed, and dropping it + * lost its worktree evidence for good. The loaders hand such a chunk to + * the reconciler instead, whose bounded window replays it when the + * catalog answers. */ + worktreeReconcilerRef: MutableRefObject | null> undoStackRef: MutableRefObject bootstrapTimersRef: MutableRefObject>> persistedFeedDebugIdRef: MutableRefObject> @@ -104,6 +112,7 @@ export function useWorkspaceRefs( const seenUuidsRef = useRef>>({}) const historyWindowsRef = useRef>({}) const historyAwaitingTurnStartRef = useRef>(new Set()) + const worktreeReconcilerRef = useRef | null>(null) const undoStackRef = useRef(new UndoCloseStack()) const bootstrapTimersRef = useRef>>(new Map()) const pendingAdoptionWindowIdsRef = useRef([]) @@ -141,6 +150,7 @@ export function useWorkspaceRefs( seenUuidsRef, historyWindowsRef, historyAwaitingTurnStartRef, + worktreeReconcilerRef, // Latest screen per session — mirrored from state into a ref so // the Enter handler in TileLeaf can capture a baseline diff --git a/src/renderer/src/workspace/tile-tree/AgentTerminalLeaf.submit.renderer.test.tsx b/src/renderer/src/workspace/tile-tree/AgentTerminalLeaf.submit.renderer.test.tsx index 3c48e6f8e..5ae28c317 100644 --- a/src/renderer/src/workspace/tile-tree/AgentTerminalLeaf.submit.renderer.test.tsx +++ b/src/renderer/src/workspace/tile-tree/AgentTerminalLeaf.submit.renderer.test.tsx @@ -166,6 +166,7 @@ describe('AgentTerminalLeaf Mouse Mode Submit', () => { resize.mockClear() sendInput.mockClear() workspace.acknowledgeSession = vi.fn() + workspace.showPaneToast = vi.fn() vi.stubGlobal('requestAnimationFrame', (callback: FrameRequestCallback) => { const id = ++nextFrameId @@ -209,6 +210,47 @@ describe('AgentTerminalLeaf Mouse Mode Submit', () => { expect(sendInput).toHaveBeenCalledWith('session-1', '\r') }) + // #1114 / steering q97: main answers `false` when the session refused the + // input (an OpenCode terminal's full pre-paint hold, a missing backend). The + // pane says so once, instead of dropping it silently. + it('tells the user, once, when the agent refuses terminal input', async () => { + settings.mouseModeEnabled = true + sendInput.mockResolvedValue(false) + render(leaf()) + act(() => flushAnimationFrames()) + await act(async () => { + attach.resolve('') + await attach.promise + }) + const button = screen.getByRole('button', { name: 'Send' }) + fireEvent.click(button) + fireEvent.click(button) + await act(async () => { await Promise.resolve(); await Promise.resolve() }) + expect(workspace.showPaneToast).toHaveBeenCalledTimes(1) + expect(workspace.showPaneToast).toHaveBeenCalledWith('session-1', "That input didn't reach the agent.") + sendInput.mockResolvedValue(undefined) + }) + + // Steering q100: input queued BEFORE attach is flushed as one write, the + // largest the pane makes; its refusal was dropped silently. + it('tells the user when input typed before attach is refused on flush', async () => { + settings.mouseModeEnabled = true + sendInput.mockResolvedValue(false) + render(leaf()) + act(() => flushAnimationFrames()) + // Queued before attach. + fireEvent.click(screen.getByRole('button', { name: 'Send' })) + expect(sendInput).not.toHaveBeenCalled() + await act(async () => { + attach.resolve('') + await attach.promise + }) + await act(async () => { await Promise.resolve(); await Promise.resolve() }) + expect(sendInput).toHaveBeenCalledWith('session-1', '\r') + expect(workspace.showPaneToast).toHaveBeenCalledWith('session-1', "What you typed while the terminal was attaching didn't reach the agent.") + sendInput.mockResolvedValue(undefined) + }) + it('queues a pre-attach Submit and delivers it once attach lands', async () => { settings.mouseModeEnabled = true render(leaf()) diff --git a/src/renderer/src/workspace/tile-tree/AgentTerminalLeaf.tsx b/src/renderer/src/workspace/tile-tree/AgentTerminalLeaf.tsx index cd64530b7..cde23a78c 100644 --- a/src/renderer/src/workspace/tile-tree/AgentTerminalLeaf.tsx +++ b/src/renderer/src/workspace/tile-tree/AgentTerminalLeaf.tsx @@ -318,8 +318,27 @@ export function AgentTerminalLeaf({ // Replay-aware, coalescing outgoing path — see terminalInputForwarder.ts // (#745) for why replies xterm generates while parsing the replay must // never reach the provider and why same-tick chunks share one IPC call. + // WHY a refusal is shown (#1114, steering q97): main says `false` when + // the input was not taken: an OpenCode terminal whose TUI has not + // painted and whose bounded pre-paint hold is full, no backend, or a + // prompt delivery holding the composer. Dropping that silently was the + // one thing the bound must not do. The copy names no cause (steering + // q100): main answers only a boolean, and any single reason would be + // false for the others. Coalesced: a held key or a burst of refused + // chunks is one message, not a stack of them. A refusal is final, not + // retried: a retry could land after newer keystrokes, out of order. + let lastRefusalToastAt = 0 + const sendReportingRefusal = (data: string, refusedMessage: string) => { + void window.api.sendInput(sessionId, data).then(accepted => { + if (accepted !== false || disposed) return + const now = Date.now() + if (now - lastRefusalToastAt < 3000) return + lastRefusalToastAt = now + showPaneToastRef.current(sessionId, refusedMessage) + }, () => {}) + } const forwarder = createTerminalInputForwarder(data => { - void window.api.sendInput(sessionId, data) + sendReportingRefusal(data, "That input didn't reach the agent.") }) offTextPaste = registerTerminalPasteTarget(sessionId, { isActive: () => !disposed && focusedRef.current && dimensionActiveRef.current, @@ -493,7 +512,11 @@ export function AgentTerminalLeaf({ } } if (pendingInput.length > 0) { - void window.api.sendInput(sessionId, pendingInput.join('')) + // Through the same refusal report (steering q100): this batch is + // typically the largest single write the pane makes (up to 256 + // queued chunks), so it is the one most likely to exceed a bounded + // hold, and it used to vanish silently when refused. + sendReportingRefusal(pendingInput.join(''), "What you typed while the terminal was attaching didn't reach the agent.") pendingInput.length = 0 } if (focusedRef.current) liveTerm.focus() diff --git a/src/renderer/src/workspace/work-context/LiveWorktreeReconciler.ts b/src/renderer/src/workspace/work-context/LiveWorktreeReconciler.ts index bf5a72669..9fc2e5f72 100644 --- a/src/renderer/src/workspace/work-context/LiveWorktreeReconciler.ts +++ b/src/renderer/src/workspace/work-context/LiveWorktreeReconciler.ts @@ -109,6 +109,12 @@ export class LiveWorktreeReconciler { cwd: string, entries: ReadonlyArray<{ entry: unknown }>, projection: WorktreeRuntimeProjection, + // 'older' = an older-history page (#1450 B6 verify). Its records predate + // everything already retained, so they enter at the OLD end of the + // window. Appended like a live batch, a scroll-up during a git timeout + // became the newest evidence and moved the pane to the older worktree + // once git recovered: folding is in window order, and the last write wins. + position: 'newest' | 'older' = 'newest', ): WorktreeRuntimeProjection { if (this.disposed) return projection let evidence = this.evidenceBySession.get(sessionId) @@ -160,6 +166,13 @@ export class LiveWorktreeReconciler { const relevantRaw = entries .map(({ entry }) => entry) .filter(entry => extractWorktreeActivityEvents(entry, this.now()).length > 0) + if (position === 'older') { + this.retainOlder(cwd, evidence, relevantRaw) + this.evidenceBySession.set(sessionId, evidence) + const next = this.rebuild(cwd, evidence) + evidence.lastEmitted = next + return next + } evidence.recentRaw.push(...relevantRaw) // Length is not a generation: after eviction this window stays at 500 // while its contents keep changing. Irrelevant transport batches do not @@ -198,6 +211,60 @@ export class LiveWorktreeReconciler { return next } + /** + * Replay retained evidence against the catalog already cached for `cwd`. + * + * WHY (#1450 verification a): refresh() notifies only when git ANSWERS. A + * history chunk handed over while its own gitWorktrees call timed out can + * land after a live event already cached a fresh catalog; refresh() then + * answers 'cached' and never notifies, so the chunk sat in the window and + * the pane stayed on the launch folder until an unrelated event. Only with + * a real catalog (refreshedAt > 0): the placeholder an in-flight probe + * writes is empty, and replaying against it is the wrong-family read #1430 + * removes. + */ + replayCachedCatalog(cwd: string): void { + if (this.disposed) return + const cached = this.cache.get(cwd) + if (!cached || cached.refreshedAt <= 0) return + this.onCatalogReady(cwd) + } + + /** + * Put an older page's relevant records at the OLD end of the retained + * evidence, under the same 2 × recentRawLimit bound as live batches. + * + * Order of age, oldest first: deferredRaw, then recentRaw. The page predates + * both, so it goes in front of whichever is non-empty first. What does not + * fit is the oldest evidence there is, so it is what gets dropped: + * - With a cached catalog the baseline already holds folded records that are + * NEWER than this page, and folding the page on top of them would make it + * the newest again. So the overflow is dropped, never folded; the window + * is full of newer evidence anyway. + * - Without a catalog the overflow moves into deferredRaw's front, and what + * deferredRaw cannot hold is counted in droppedBeforeCatalog, as for live + * evictions. + */ + private retainOlder(cwd: string, evidence: SessionEvidence, relevantRaw: unknown[]): void { + if (relevantRaw.length === 0) return + evidence.revision += 1 + if (evidence.deferredRaw.length > 0) { + evidence.deferredRaw.unshift(...relevantRaw) + } else { + evidence.recentRaw.unshift(...relevantRaw) + if (evidence.recentRaw.length > this.recentRawLimit) { + const overflow = evidence.recentRaw.splice(0, evidence.recentRaw.length - this.recentRawLimit) + const cached = this.cache.get(cwd) + if (!(cached && cached.refreshedAt > 0)) evidence.deferredRaw.unshift(...overflow) + } + } + if (evidence.deferredRaw.length > this.recentRawLimit) { + const overflow = evidence.deferredRaw.length - this.recentRawLimit + evidence.deferredRaw.splice(0, overflow) + evidence.droppedBeforeCatalog += overflow + } + } + forgetSession(sessionId: SessionId): void { this.evidenceBySession.delete(sessionId) } diff --git a/src/renderer/src/workspace/work-context/worktreesForAttribution.test.ts b/src/renderer/src/workspace/work-context/worktreesForAttribution.test.ts new file mode 100644 index 000000000..bc395540c --- /dev/null +++ b/src/renderer/src/workspace/work-context/worktreesForAttribution.test.ts @@ -0,0 +1,24 @@ +import { describe, expect, it } from 'vitest' + +import { worktreesForAttribution } from './worktreesForAttribution' + +// #1430: history chunks attributed a TIMED-OUT worktree list as `[]` (no +// worktrees), recording the pane's work as outside any worktree. The three +// answers `git:worktrees` gives (see src/preload/api/git.ts) map to three +// different things. +describe('worktreesForAttribution', () => { + const worktrees = [{ path: '/repo' }, { path: '/repo/.worktrees/a' }] + + it('attributes against the list git answered', () => { + expect(worktreesForAttribution({ ok: true, worktrees })).toBe(worktrees) + }) + + it('skips attribution when git timed out: the family is unknown, not empty', () => { + expect(worktreesForAttribution({ ok: false, gitMissing: false, timedOut: true } as never)).toBeNull() + }) + + it('attributes against no worktrees for a real non-repository or missing git', () => { + expect(worktreesForAttribution({ ok: false, gitMissing: false } as never)).toEqual([]) + expect(worktreesForAttribution({ ok: false, gitMissing: true } as never)).toEqual([]) + }) +}) diff --git a/src/renderer/src/workspace/work-context/worktreesForAttribution.ts b/src/renderer/src/workspace/work-context/worktreesForAttribution.ts new file mode 100644 index 000000000..278ecbe20 --- /dev/null +++ b/src/renderer/src/workspace/work-context/worktreesForAttribution.ts @@ -0,0 +1,16 @@ +/** + * Which worktree list a history chunk's events are attributed against (#1430). + * + * WHY a TIMED-OUT list answers `null` (skip attribution) and not `[]`: `[]` + * is what a non-repository really has, and attributing against it is right + * there. For a timeout the family is UNKNOWN — attributing against `[]` would + * record the pane's work as outside any worktree, a wrong answer the live + * reconciler then has to overwrite. Skipping leaves the pane's work context as + * it was; the live reconciler fills it in when git answers. + */ +export function worktreesForAttribution( + result: { ok: true; worktrees: W[] } | { ok: false; timedOut?: boolean }, +): W[] | null { + if (result.ok) return result.worktrees + return 'timedOut' in result && result.timedOut === true ? null : [] +} diff --git a/src/shared/conversations/types.ts b/src/shared/conversations/types.ts index f629fd256..17300b67e 100644 --- a/src/shared/conversations/types.ts +++ b/src/shared/conversations/types.ts @@ -77,7 +77,10 @@ export type ConversationListResponse = { total: number hiddenChildren: number nextCursor: string | null - family: { repoRoot: string | null; roots: string[] } + /** `gitTimedOut` (#1430): `git worktree list` did not answer in time, so the + * repository's other worktrees could not be included; the rows may be + * missing some. Absent when git answered. */ + family: { repoRoot: string | null; roots: string[]; gitTimedOut?: true } timing: { ms: number } } diff --git a/src/shared/types/session.ts b/src/shared/types/session.ts index 61399e9f7..1e62e15a5 100644 --- a/src/shared/types/session.ts +++ b/src/shared/types/session.ts @@ -575,7 +575,11 @@ export interface AgentSessionEmitter { export interface AgentSession extends AgentSessionEmitter { start(): Promise<{ projectDir?: string } | void> stop(): Promise - write(data: string): void + /** `false` = the session refused the input (nothing was written or held); + * anything else = accepted. Sessions that never refuse return nothing. + * SessionManager.write reports a refusal to its caller, which tells the + * user (OpenCode Terminal's bounded pre-paint hold, #1114). */ + write(data: string): void | boolean resize(cols: number, rows: number): void /** Optional: does the underlying process have a pid we can display / @@ -677,8 +681,11 @@ export interface AgentSession extends AgentSessionEmitter { ): Promise /** Optional (OpenCode today): deliver a user prompt through the concrete - * runtime's transport. Structured OpenCode uses HTTP; OpenCode Terminal - * performs a readiness-gated bracketed paste into its PTY. Keeping both + * runtime's transport. Both OpenCode runtimes deliver over the server's + * HTTP API (Terminal since #877, never a PTY paste); OpenCode Terminal's + * headless additionally holds delivery until its durable reader has + * positioned (#1114), so a prompt cannot commit behind the reader's + * starting head. Keeping both * behind one provider-owned capability lets SessionManager stay ignorant * of which OpenCode runtime was selected. Claude/Codex leave it undefined * and their provider policies write through io.write instead. */ diff --git a/src/shared/userMcp/types.ts b/src/shared/userMcp/types.ts index 150c7302a..24bf0620d 100644 --- a/src/shared/userMcp/types.ts +++ b/src/shared/userMcp/types.ts @@ -111,7 +111,16 @@ export type UserMcpProblem = export type UserMcpSupport = { ok: true } | { ok: false; reason: string } -export type UserMcpSecretState = { set: boolean; hint?: string } +/** `unconfirmed`: a secret saved by an earlier version (before secrets were + * bound to their destination). It is kept but withheld from launches until + * the user confirms it for the current destination or re-enters it (q114). */ +/** + * `unconfirmed`: a stored secret that is kept but withheld until the user + * confirms it (#1420): `legacy` = saved by an earlier version (q114); + * `inputs-changed` = an agent or an import changed another input of this + * server since it was saved (B6 R3, q127). A user's own edit rebinds instead. + */ +export type UserMcpSecretState = { set: boolean; hint?: string; unconfirmed?: 'legacy' | 'inputs-changed' } /** What crosses IPC to the renderer. Never contains a secret value. */ export type UserMcpServerView = UserMcpServer & { diff --git a/src/shared/userMcp/userMcp.test.ts b/src/shared/userMcp/userMcp.test.ts index 6d34ddc08..9d76e394b 100644 --- a/src/shared/userMcp/userMcp.test.ts +++ b/src/shared/userMcp/userMcp.test.ts @@ -203,9 +203,36 @@ describe('review round 2 model rules', () => { expect(summarizeEntry({ command: 'srv', args: ['--api-key', 'd6f8g2h9', '--port', '8080'] })).toBe('srv … … --port 8080') }) - it('counts every literal as part of where secrets go, but not which secret a value references', () => { + // q118 (#1420 round-2 review a): WHICH input a value references is part of + // where secrets go. An endpoint built from `${input:trusted-host}` becomes + // another host when the agent points it at `${input:evil-host}`, so a + // reference change must be a destination change, like any literal edit. + it('counts every literal AND every reference id as part of where secrets go', () => { const base = { command: 'npx', env: { T: '${input:a}' } } - expect(userMcpDestination(base)).toBe(userMcpDestination({ command: 'npx', env: { T: '${input:b}' } })) + expect(userMcpDestination(base)).not.toBe(userMcpDestination({ command: 'npx', env: { T: '${input:b}' } })) expect(userMcpDestination(base)).not.toBe(userMcpDestination({ command: 'npx', env: { T: '${input:a}', NODE_OPTIONS: '--require x' } })) + // Key order is not a destination. + expect(userMcpDestination({ command: 'npx', env: { A: '1', B: '${input:a}' } })) + .toBe(userMcpDestination({ command: 'npx', env: { B: '${input:a}', A: '1' } })) + }) + + // q118 (round-2 review a, finding 2): the old mask wrote every reference as + // `${input}`, which is also literal text substitution leaves alone, so + // `${input:t}${input}` and `${input}${input:t}` looked identical while the + // resolved header changed. + it('never confuses a literal `${input}` with a reference', () => { + const header = (auth: string) => ({ type: 'http', url: 'https://api.example/mcp', headers: { Authorization: auth } }) + expect(userMcpDestination(header('Bearer ${input:t}${input}'))).not.toBe(userMcpDestination(header('Bearer ${input}${input:t}'))) + }) + + // #1420 reviews a+b: masking the WHOLE value that contains a reference let + // the literal around it (the endpoint) change without changing the + // identity, so the token went to a host it was not saved for. + it('counts the literal text around a secret reference as part of the destination', () => { + const at = (url: string) => ({ command: 'node', args: ['client.js'], env: { MCP_ENDPOINT: url } }) + expect(userMcpDestination(at('https://trusted.example/mcp?key=${input:t}'))) + .not.toBe(userMcpDestination(at('https://evil.example/mcp?key=${input:t}'))) + const header = (auth: string) => ({ type: 'http', url: 'https://api.example/mcp', headers: { Authorization: auth } }) + expect(userMcpDestination(header('Bearer ${input:t}'))).not.toBe(userMcpDestination(header('Basic ${input:t}'))) }) }) diff --git a/src/shared/userMcp/validate.ts b/src/shared/userMcp/validate.ts index 59eda5178..321b01857 100644 --- a/src/shared/userMcp/validate.ts +++ b/src/shared/userMcp/validate.ts @@ -223,25 +223,40 @@ export function providerSupportForEntry(entry: unknown): Record = { ...entry, type: transportOf(entry) } + const canonical: Record = { ...entry, type: transportOf(entry) } for (const field of ['env', 'headers'] as const) { if (!isStringRecord(entry[field])) continue - masked[field] = Object.fromEntries(Object.entries(entry[field] as Record) - .map(([key, value]) => [key, hasInputReference(value) ? '' : value]) - .sort(([a], [b]) => (a as string).localeCompare(b as string))) + canonical[field] = Object.fromEntries(Object.entries(entry[field] as Record) + .sort(([a], [b]) => a.localeCompare(b))) } - return JSON.stringify(Object.keys(masked).sort().map(key => [key, masked[key]])) + return JSON.stringify(Object.keys(canonical).sort().map(key => [key, canonical[key]])) } /** diff --git a/support/upstream-versions.json b/support/upstream-versions.json index b796b2cde..35ad28be2 100644 --- a/support/upstream-versions.json +++ b/support/upstream-versions.json @@ -12,8 +12,8 @@ "label": "Codex", "pkg": "@openai/codex", "changelog": "https://github.com/openai/codex/releases", - "accepted": "0.130.0", - "checkedAt": "2026-05-16" + "accepted": "0.157.1", + "checkedAt": "2026-09-26" }, "opencode": { "label": "OpenCode",