diff --git a/docs/plans/2026-09-26-control-history-retention.md b/docs/plans/2026-09-26-control-history-retention.md new file mode 100644 index 000000000..d1303b5e9 --- /dev/null +++ b/docs/plans/2026-09-26-control-history-retention.md @@ -0,0 +1,72 @@ +# Control history keeps a bounded window (#1274) + +## Problem +`FileControlHistory` appends every control call (external control, MCP, application tasks) to `control-history/events.jsonl`, with each prompt and result as a `payloads/.json` file. Nothing is ever removed, and `open()` loads the whole journal into an in-memory array that every `history.events()` call copies. + +## Evidence (owner's store, 2026-09-27) +- 129 MB on disk after 22 days: `events.jsonl` has 6,089 rows over 1,894 calls; `payloads/` holds 3,499 files for 3,457 referenced digests (42 orphans from failed appends). +- 105 MB of the payloads are `transcripts.page` results, which are read-only and unkeyed. `mcp.tools/list` adds 7 MB, `mcp.tools/call` 2.8 MB, and `agents.read` 1.5 MB. +- Keyed calls, the executor's dedupe ledger (#1240), are small: 815 rows, 220 `received` (221 calls carry a key; corrected in review C3). Their keys come from `dispatch.configure`, `commands.run`, `operations.start/finish`, `agents.prompt`, and others. +- Every call in the journal has a `result` row (0 in flight). +- Simulated retention for unkeyed finished calls: 7 days keeps 507 calls (1,910 rows) and 17 MB of payloads; 14 days keeps 881 calls and 48 MB; 30 days keeps everything. + +## The boundary this must not break +The journal is the executor's idempotency ledger (see the #1240 class comment and `docs/plans/2026-09-25-control-history-recovery.md`). A keyed retry must find its `received` row and stored result forever, because request keys have no lifetime in the contract. Retention may therefore only remove calls that can never be looked up for dedupe. + +## Decisions (defaults) +1. **Prune whole calls, never single rows.** A call is removed only when ALL of these hold: + - no row of the call carries a `requestKey`, so it is not dedupe evidence; + - it has a `result` row, so it is not in flight (`history.read` would otherwise show a call lost mid-way); + - no kept row names it as `reusedCallId`; + - its newest row is older than the retention window. + Removing whole calls keeps the per-call key consistency that `analyze()` checks, so a pruned ledger reloads as clean. +2. **Window: 7 days (UNCONFIRMED default).** On the owner's rate this is about 20–35 MB steady state. `history.read` and `history.list` answer for the last week; an agent inspecting what it did is interested in hours, not weeks. The window is a constructor option so a product change is one line. +3. **Pruning runs on load, after recovery,** as one atomic rewrite of `events.jsonl` (temp file, fsync, rename, directory fsync, the same `writeAtomic` recovery uses), with sequences renumbered (process-local cursors, as in recovery). The app restarts often (updates, relaunch), and load is the one point where no append is in flight. A process that runs for weeks keeps growing until its next launch; that residual is stated. +4. **Payload GC after the rewrite.** Delete every `payloads/*.json` not referenced by a kept row, which also removes orphans left by failed appends. This runs only after the journal rewrite is durable, so a crash in between leaves extra files, never a row pointing at a missing payload. A digest shared by a pruned row and a kept row stays. +5. **`history.list` drops "never silently truncated".** It becomes a stated window: "keyed calls are kept; other finished calls are kept for 7 days". +6. **A damaged ledger is not pruned in the same launch.** When recovery ran, the rewritten ledger is still under an unaccepted block; pruning is skipped so the operator reconciles what they saw. The next clean launch prunes. + +## Tests (fail-first, real rows) +A fixture of real journal rows, recorded from the owner's store: ids, timestamps, callers, capability ids and digests only (no prompts, per the #1240 plan). It covers old unkeyed calls, old keyed calls, a reuse pair, and recent calls. Payload files are written by the test under the recorded digests. +- An old unkeyed finished call is pruned, with its payloads, and orphan payloads are removed. Old keyed calls, the calls they reuse, and recent calls are kept, byte-identical apart from renumbered sequences. +- After pruning, a keyed retry of an old key still replays its stored result and never re-dispatches (executor round-trip). +- A call without a `result` row is kept however old it is. +- A payload shared by a pruned and a kept row survives. +- A load that ran recovery does not prune. + +## Review round 1 (#1330) and steering q47/q49 +All three reviewers returned FIX-BEFORE-MERGE. Whatever window the owner picks, retention must never delete evidence someone can still ask for. A finished, unkeyed, unreused, old call is now also KEPT when: +- **It is a lifecycle task origin** (a `task.*` step) (A1, C1). `operations.read`/`finish` look a task up by its original call id, and the tool contract says task results persist across restarts. The owner has 12 such calls. +- **Its result is not proven settled** (A3). Settled means: an ok result with status `completed`/`ui_opened`, a `not_started` refusal, or an MCP transport echo. `outcome_unknown` (26 on the owner's machine) and `pending` (157, accepted `agents.prompt` operations) are kept, and so is a missing or unreadable payload (q40). +- **An unaccepted recovery quarantine names it** (A2). Its call ids, and every payload digest the quarantine names, stay until the operator accepts that digest in `recovery-accepted.json`. + +Payloads are read only for calls that are otherwise expired, so after the first launch each launch reads about a day's worth. + +Other changes: +- **Rewrite failure (B1).** A failed rewrite no longer fails the load, and payload GC never runs after one. Sequences are renumbered on copies, so the rows served still match the unchanged file. +- **Survivors pinned.** Reuse-only target (A, B2), unknown-age row (A, B3), non-digest temp file (B4), and the window at its boundary, 7 days ± 1 minute (C). +- **Fixture metadata (B5).** The shared digest is on the `dispatched` rows. + +**Owner question (unchanged, still open):** the window length (default 7 days). With the retained roots above, what a window deletes is only settled, unkeyed, non-task calls: exact request/result copies of reads and completed mutations, which `history.read`/`list` then no longer find. On the owner's store today: 7 days keeps ~520 calls and ~20 MB of payloads; 14 days keeps ~884 calls and ~52 MB. + +## Review round 2 (#1330) +- **A GC failure after a successful rewrite (a, b, c; Blocker).** The whole prune rejected, and `open()` served the pre-rewrite rows. The next append numbered itself from that longer list, and the gap got the journal quarantined with keyed calls blocked. Once the rewrite lands, the rewritten rows are always what is served. Each payload deletion fails on its own (warned, retried next launch). +- **A torn-tail quarantine (a, b, c; Major).** It has no guard (it blocks no keyed call), so it was never scanned, and the payload only its torn line names (an outcome fsynced before the append tore) was deleted on the next launch. Evidence is now read from **every** quarantine file whose digest is not accepted. A damaged line still yields its digests, and its call id when visible. An unreadable quarantine file skips retention for that launch. +- **Task detection by byte prefix (a, b, c).** It missed a valid step with another key order and treated a corrupted step as ordinary. It now parses the step through the integrity-checked `payload()`, as the task store does. Unreadable or not an object means kept. +- **First-launch cost (b, c; Minor): accepted as a residual.** Classifying the backlog reads each old result payload once. On a clone of the owner's store the first load took 3.2–6.9 s, and later loads take about 35 ms. It runs on the first control-history use after the upgrade, not at app start. A byte-sniffing shortcut was rejected: that is exactly the kind of guess the task-prefix bug was. + +Tests: 3 new cases (red on `65175615`). They also kill round 2's surviving mutations: the task guard is now exercised with a settled result, and quarantine-only digests with a torn tail. + +## Owner decision (2026-09-27) +**The window is 90 days** (`temp/manager/owner-decisions-2026-09-27.md`: "#1330, control history: 90 days", with the standing preference "infrequent deletion"). This replaces the UNCONFIRMED 7-day default. +- `CONTROL_HISTORY_RETENTION_MS = 90 days`, pinned by value in the boundary test. +- Both `history.read` / `history.list` descriptions now say 90 days. +- **Tests:** the recorded-store retention cases move "now" 83 days later, the amount the window grew. Every recorded call keeps its position relative to the edge, so the same rows are pruned and kept. +- **Cost:** on the owner's store today, 30 days already kept everything, so the 90-day window deletes nothing yet. At ~5 MB of payloads a day, steady state is up to ~450 MB. What stays bounded is long-run growth of a journal held whole in memory. + +## Verification (b: FIX-BEFORE-MERGE; c: MERGE-READY) +- **b (Blocker): the same served/disk divergence through the rewrite's directory sync**, which runs after the rename. EIO there rejected the prune, and open() served the pre-rewrite rows. + - **Ruling:** the rows on disk change at the rename. `writeAtomic` reports when the rename landed. A failure after that point serves the renumbered rows, with a warning, and skips payload GC for this launch. + - **Why skip GC:** a power loss could undo the unsynced directory entry and bring back the old journal, which names the expired calls' payloads. The orphans are collected by the next launch. + - Test: EIO injected at the directory sync after the rename. The served rows match the file, the expired call's payloads remain, the next append continues the file, and the next launch reports no recovery. The test fails on the previous head. +- **b (survivor): the GC's quarantine digests were unpinned alongside a real prune.** Test: a torn-tail quarantine, then another old settled call pruned in the same launch; the quarantine-only payload survives. The test fails with the digests removed from GC's `referenced` set. diff --git a/docs/plans/2026-09-27-codex-compaction-boundary.md b/docs/plans/2026-09-27-codex-compaction-boundary.md new file mode 100644 index 000000000..a8466cac6 --- /dev/null +++ b/docs/plans/2026-09-27-codex-compaction-boundary.md @@ -0,0 +1,24 @@ +# Codex compaction never renders (#1289) + +## Evidence (verified 2026-09-27 on the local corpus, about 1,500 `compacted` lines) +1. **The branch never runs.** `mapCodexRolloutToFeedEntries` returns early when `payload.type` is not a string. A `compacted` line's payload has no `type`, so the `entry.type === 'compacted'` branch never runs and Codex compaction never renders. +2. **The boundary has no timestamp.** The boundary entry is built without one, and `rendering/model/order.ts` sorts timestamp-less rows to the end of their phase. So enabling the branch as-is would paint "Conversation compacted" at the bottom of the feed, not where it happened. +3. **`replacement_history` is not new conversation.** It is the context Codex retains across the compaction: developer instructions, the AGENTS.md block, earlier user prompts and (0.155+) a `compaction` item whose summary is `encrypted_content` (13–23 KB). Mapping it repaints prompts already in the feed (#1289: 17,326 of 20,343 sampled replacement messages duplicate an earlier user message). +4. **The summary `message` is empty in every 0.15x rollout.** Only 56 of about 1,500 compactions carry readable text, all from older CLIs. +5. **The boundary stores the whole payload as `compactMetadata`.** That is `replacement_history`, the retained user prompts, resume metadata and the encrypted blob, kept on a feed entry and in every debug bundle, although nothing reads more than identity. + +## Change +- Handle `compacted` before the `payload.type` guard. +- The boundary carries the line's timestamp, so it sorts where compaction happened. +- `replacement_history` is never mapped. The summary entry is emitted only when `message` is non-empty (older CLIs). +- **No `compactMetadata`.** Nothing in the app reads it for Codex, and a varying metadata object also made every boundary a different rendering shape. +- Catalog: add Codex durable shapes for the boundary and the summary. They are the same shared.compaction dispositions Claude's entries have, pinned by curated fixtures. + +## Tests +Two real `compacted` lines: a 0.157.0 one (empty message, `compaction` item) and an older one with a readable message. Texts are redacted at equal length; the structure and keys are verbatim. +- Both map to a timestamped boundary. +- Only the older one also maps to a summary. +- No replacement-history entry is emitted. +- The boundary carries no retained history. +- Red on main, where both map to `[]`. +- The dispatcher renders the boundary through `shared.compaction`, and the catalog classifies it `known-claimed`. diff --git a/docs/plans/2026-09-27-floating-surface-layers.md b/docs/plans/2026-09-27-floating-surface-layers.md new file mode 100644 index 000000000..4724fd0b4 --- /dev/null +++ b/docs/plans/2026-09-27-floating-surface-layers.md @@ -0,0 +1,42 @@ +# Floating-surface layer: named layers, side-panel shell (#512) + +## Where #512 stands on main (inventory, 2026-09-27) +- **Done by earlier work:** shared Radix `dialog.tsx` for modals, shared `dropdown-menu.tsx` (AppearanceMenu and the Skills menu moved onto it), one toast (CaffeinateToast forwards to `GlobalToast`), and the shared side-panel header `PanelHeader`. +- **Left:** + 1. **No named layer scale.** The bands are real but spelled as magic numbers in about ten files: + - pane overlays z-40/50; + - pane dialog z-[60..62]; + - dialog z-[1100]; + - menus z-[1150]; + - toast/dictation z-[1200]; + - debug highlight z-[10000]. + Comments in `registry.tsx`, `App.tsx` and `surfaces/types.ts` still describe the old "everything z-50, DOM order breaks ties" model. + 2. **No shared side-panel shell.** Git and Worktrees are identical; Agent Status is near-identical. The six debug panels copy their own shell and header, and their closes have no accessible name. + 3. **RemotePanel is registered as a side panel but renders a centered Dialog.** + 4. **No dismiss stack / `anySurfaceOpen()`.** Escape arbitration is Radix's own layer stack, plus the central `useKeybinds` branches, plus per-component handlers. + 5. **Three hand-rolled anchored popovers remain:** + - CommandSortControl: keeps focus in the palette input; + - PathInput suggestions: a combobox; + - ExplorerPane context menu: pointer-anchored, with a WHY for not moving. + +## This PR +- **`ui/layers.ts`:** ONE table of named layers as literal Tailwind classes (Tailwind only emits classes it sees spelled out, the rule `PANE_DIALOG_LAYERS` already follows). Every magic z value in the bands above moves onto it, and `PANE_DIALOG_LAYERS` becomes a view of it. The stale comments are corrected. +- **`components/ui/side-panel.tsx` ``:** the shared outer shell (`aside`, fixed width, border, surface, column, overflow). Git, Worktrees, Agent Status and the debug panels render through it. The debug panels also move to `PanelHeader`, which gives their closes a name. +- **RemotePanel** moves from `sidePanelSurfaces` to the modal surfaces it actually is. +## Sequencing (steering q95) +W1's open #1394 edits `useKeybinds.ts` and its ownership tests, and owner keyboard work (#1221 follow-ups) is in flight. Until #1394 merges, this PR does NOT touch `useKeybinds.ts` or its tests. + +## The dismiss stack moved out of this PR (review b) +A first version shipped an unwired `ui/dismissStack.ts`. Review showed its wiring contract could not work as written: +- **Capture phase is too early.** A document capture-phase Escape listener runs BEFORE the inner handlers that implement two-phase dismissal (PathInput suggestions, then the modal; palette sub-modes), so it would close the modal first. +- **Effect order is not stacking order.** Passive-effect registration order is not visual stacking order: a parent and child that mount in the same commit register child-first, so Escape would close the parent. + +Both need a different arbitration design, done together with the `useKeybinds.ts` wiring on a fresh origin/main after #1394 merges. Shipping an unwired module with a known-wrong contract would mislead that work. + +## Not in this PR, and why +The three remaining anchored popovers stay custom. Each has a documented reason a menu primitive does not fit: +- the palette input must keep DOM focus; +- the path field is a combobox; +- the explorer menu is anchored at the pointer. + +A popover primitive shaped around one of them would be fitted to one caller. The PR therefore says `Refs #512`, not `Fixes`. #512 stays open for the dismiss stack plus its useKeybinds wiring, and for the popover decision. diff --git a/docs/plans/2026-09-27-setup-state-write-failures.md b/docs/plans/2026-09-27-setup-state-write-failures.md new file mode 100644 index 000000000..97ce8b67f --- /dev/null +++ b/docs/plans/2026-09-27-setup-state-write-failures.md @@ -0,0 +1,86 @@ +# A failed setup-state write is reported and not half-applied (#1250 rows 6 and 13) + +Short plan: bugs with a known root cause. The rows come from `temp/quality-loop/hunt-c3.md`. #1250 is a batch issue, so this PR is `Refs #1250`; the other rows stay open. + +## Outcome +Changing a provider's enablement (row 6), the OpenCode usage source, or the CLI update behaviour (row 13) either takes effect, or tells the user it did not and leaves everything as it was. Today a failed write (a full disk, a read-only or permission-broken state directory) is invisible: +- the switch or card flips back with no message, or stays flipped until restart; +- main keeps the unwritten value in memory, so the app behaves as if it were saved, then silently reverts on the next launch. + +## Root cause (verified in source, origin/main) +- **`saveSetupState` (`src/main/setup/setupState.ts`)** assigns `cache` to the new state BEFORE the queued write, and never restores it when the write rejects. Every later `loadSetupState()` returns the unwritten value: + - `providerEnablement.mutate` re-resolves from it on the next poll; + - the CLI-update orchestrator is not told (the IPC handler throws first), while the cache says otherwise. +- **`ProviderEnablementRow`** toggle and reset use `try/finally` with no `catch`, called via `void`: an unhandled rejection with nothing shown. +- **`OpencodeUsageSourceRow`** catches, but shows the raw IPC `error.message`, which for a filesystem failure can carry a path (q22: user-visible text is curated). +- **`setCliUpdateBehavior` (renderer store)** has no rejection handler at all, and `CliUpdateBehaviorRow` has nowhere to say it failed. + +## Design (contract) +- **`saveSetupState`:** keep the previous cache. If the write rejects and no newer save has replaced the cache since (`cache === snapshot`), restore it, then rethrow. + - **Ruling:** a newer save built from the failed state is left alone. It would persist the failed value too, which is what the user asked for, and restoring under it would discard a write that succeeded. Cost: in that narrow race, a change reported as failed lands later. +- **Renderer:** each row catches and shows a fixed sentence: "Couldn't save this change. Nothing was changed." The row's own value comes from main's snapshot, so it shows the unchanged state. + - `setCliUpdateBehavior` returns its promise. + - `CliUpdateBehaviorRow` holds an error, like `UpdateChannelRow`. + - No IPC or filesystem text is shown. + +## Tests +- **`setupState.test.ts` (new), real filesystem in a scratch `STATE_DIR`:** a directory at `setup.json` makes the rename fail. Then: + - the save rejects; + - `loadSetupState()` returns the previous value, not the unwritten one; + - a later good save still lands. + + Red on main: the cache keeps the unwritten value. +- **Renderer:** a rejecting `providerEnablementSet` / `providerEnablementReset` / usage-source call / `cliUpdatesSetBehavior` shows the fixed sentence and no raw text, and there is no unhandled rejection. Red on main for rows 6 and 13. + +## Out of scope +- #1250 rows 7–12, 14 and 15. +- Row 15 needs `src/main/index.ts`, which open PR #1216 also edits; it is raised with the manager. + +## Review round 1 (a, b: FIX-BEFORE-MERGE; c: MERGE-READY with minors) +- **The restore design was wrong (a, b, c).** Every save was built from the optimistic cache: + - two queued failures made the second "restore" the first one's unwritten state; + - a failure followed by a good save carried the failed value to disk while the row said "Nothing was changed". + The `cache === snapshot` ruling above is **withdrawn**. + - **Ruling:** a save is an UPDATE function (`updateSetupState(update)`). At write time it is applied to `durable` (the last state known on disk), never to another save's unwritten result. `cache` is `durable` plus the still-pending updates, recomputed as each one settles, so a failed update drops out of memory and out of every later write. + - Readers still see a change synchronously once the state is loaded. + - `saveSetupState(next)` stays as a whole-state update. + - The first read is shared, so a late duplicate read cannot reset `durable`. + - **Cost if wrong:** none known. Updates are pure functions of the state. +- **Provider toggles build per key** (`setProviderEnablementOverride(kind, enabled | null)`), not from a whole map computed off the optimistic cache. +- **Reset persisted, then rejected (a, b).** + - `mutate` rejects only when the write fails. + - A refresh failure after a landed write resolves: the saved state is shown against the last detection that succeeded, or fails open to "all installed", the same as `enabledAgentProviderKindsSync`. + - Found on the way: a rejected detection probe stayed "in flight" for the process lifetime. It is now cleared in `finally`. +- **SetupGate (b), in scope because it writes the same file:** + - a failed manual path shows the fixed sentence, not the raw IPC error; + - a failed skip or acknowledgment (the panel still closes, per #1047) is said after the close as a toast; + - a failed check stores a fixed sentence (the raw error goes to the console). +- **Test gaps (b, c):** the reset alert is tested on its own render; clearing after a later success is pinned for all three rows; each ordered queue case has a real-filesystem test with a one-shot rename fault. +- **c (minor):** the body's test count is corrected. + +Tests, each red on `6707e7cf` (verified by swapping in that file): +- `setupState.test.ts`: 4 queue cases. +- `providerEnablement.test.ts` (new): a reset with a failing re-probe, and the stuck in-flight probe. +- `firstRun.renderer.test.tsx`: the skip toast and the manual path. +- The row tests kill b's four surviving mutations. + +## Verification (a, b: FIX-BEFORE-MERGE) +- **b (Major): a saved setup answer reported as unsaved.** Every setup IPC saves the answer, then runs `checkPrerequisites`, whose tool-path write-back hits the same file. When only the write-back failed, the IPC rejected, and SetupGate said the answer was not saved. + - **Ruling:** the write-back is best effort (warned). It persists a cache of the probe, and the result returned is the probe's own answer. + - **Cost:** the toolchain keeps its last persisted paths until a later write-back lands. + - This also stops Install and a provider reset from rejecting after their own work succeeded. + - Test: `src/main/ipc/setup.test.ts`, with the real handlers, real setup state and real check. Skip, acknowledgment and manual path each resolve with the answer on disk; a failing answer write still rejects. Red with the old `prerequisites.ts`. +- **b (Major): Install rendered the raw rejection.** It now shows "Could not install ." The installer's own output on a non-ok result is unchanged. Test in `firstRun.renderer.test.tsx`, red on the old SetupGate. +- **a (Major): an older provider refresh could overwrite and broadcast a newer one** (a pre-existing race in the touched path). Refreshes are numbered; only one started after the applied refresh may replace it. Test: the first row's credential probe is held while the second row's refresh finishes; the cache and the last broadcast keep Claude off. It fails without the ordering. +- **Survivors, each pinned:** + - the shared first read: a held first read that finishes after a save no longer resets the durable baseline; + - the fallback uses the last good detection, not "all installed". +- **c (MERGE-READY):** its two survivors are the two pinned above. Its suspicion, an update that throws staying pending forever, is closed: the update leaves `pending` on every path. Test added. + +## Recheck (a, b: FIX-BEFORE-MERGE) +- **b and a2 (Major): a provider snapshot published a toggle whose write then failed.** A refresh read the optimistic cache, which already held another toggle still in flight (from a second toggle's refresh, or from an overlapping `get`). + - **Ruling:** published provider snapshots are built from the DURABLE state (`loadDurableSetupState`). Every refresh runs after its own write has landed, so nothing optimistic is needed there. + - Two tests: a successful toggle during a failing one, and an overlapping read. Both fail with the optimistic read. +- **a1 (Major): a failed write-back left the toolchain on the last persisted, possibly dead, path while the check said "found at X".** `refreshToolchainFromState(unsaved)` applies the probed paths in memory when they could not be persisted, so the check and a launch agree in this process. + - Test: new `prerequisites.test.ts`, with the real check, toolchain and setup state. It fails without the overlay. + - It also kills a's survivor (the refresh removed). diff --git a/docs/plans/2026-09-27-wall-clock-tests.md b/docs/plans/2026-09-27-wall-clock-tests.md new file mode 100644 index 000000000..f0b711ae0 --- /dev/null +++ b/docs/plans/2026-09-27-wall-clock-tests.md @@ -0,0 +1,17 @@ +# Tests whose outcome depends on load or build state (#1107, remaining sites) + +## Remaining sites (from the issue and its correction comment) +| Site | Cause | Decision | +|---|---|---| +| `lspServerCreation.system.test.ts` | fixed `setTimeout(20)` | already fixed in #1108 | +| `lazy-prose/index.renderer.test.tsx` (#700) | The first compile of the lazily imported Markdown chunk is measured by a 5 s `findByText`, which equals Vitest's 5 s test default, so its useful message ("never appeared") can never fire | **Load the chunk in `beforeAll`**, awaiting the import itself. `findByText` then measures only the render, with its own default and a real message. | +| `sessionManager.terminalReplay.test.ts` | Dynamic `import()`s of `@xterm/headless` and `sessionManager`'s whole module graph run inside the test body, against the 5 s test budget | **Static imports.** `vi.mock` is hoisted, so the mocks still apply. Module loading moves to collection, outside the test's budget. | +| `workflows/control.system.test.ts` | Forks the BUILT workflow worker (`packages/workflow-mcp/dist/workflowWorker.js`). In a fresh worktree `dist` doesn't exist; the package throws `worker-missing`, the test only sees the operation never completing, and it times out after 5 s with no reason. | **A precondition** that fails at once, saying the worker must be built (`npm run build` in `packages/workflow-mcp`). CI builds it, so CI is unaffected. The test does not build it itself: a build inside a unit of the suite would be slow, and racy across parallel workers. | + +Rules held: +- No budget is widened. +- Each wait is either on its condition or on the real module load. + +## Evidence +- The fresh worktree has no `packages/workflow-mcp/dist`. On `origin/main` the control test fails after 5 s with a `waitFor` timeout; after the fix, it fails immediately with the precondition message. +- For the other two, the load moves out of the test body. The PR records the measured load time that used to count against the budget. diff --git a/src/main/control/history/FileControlHistory.test.ts b/src/main/control/history/FileControlHistory.test.ts index 79934ab5e..d2c420609 100644 --- a/src/main/control/history/FileControlHistory.test.ts +++ b/src/main/control/history/FileControlHistory.test.ts @@ -1,20 +1,28 @@ import { mkdtemp, readFile, readdir, appendFile, writeFile, rm, stat, chmod, access } from 'node:fs/promises' import { tmpdir } from 'node:os' import { join } from 'node:path' -import { randomUUID } from 'node:crypto' -import { afterEach, describe, expect, it } from 'vitest' +import { createHash, randomUUID } from 'node:crypto' +import { afterEach, describe, expect, it, vi } from 'vitest' import { z } from 'zod' import { createControlExecutor, createControlRegistry } from '../../../control-sdk/host' import { defineCapability, type ControlHistory, type ControlResult } from '@control-sdk' -import { FileControlHistory } from './FileControlHistory' +import { CONTROL_HISTORY_RETENTION_MS, FileControlHistory } from './FileControlHistory' +import { taskHistoryCapabilities } from './tasks' import { historyCapabilities } from './control' +// The recorded rows in real-rows-2026-09.json were written on 2026-09-05. The +// history's retention clock (#1274) is pinned there, so a recorded unkeyed +// call is "recent" as it was when recorded, not pruned for being weeks old by +// the wall clock. Rows these tests append are stamped with the wall clock, +// which is later still, so they are recent too. Retention itself is tested +// against its own clock at the end of the file. +const RECORDED_AT = () => new Date('2026-09-05T09:00:00.000Z') const directories: string[] = [] afterEach(async () => { await Promise.all(directories.splice(0).map(directory => rm(directory, { recursive: true, force: true }))) }) async function setup() { const directory = await mkdtemp(join(tmpdir(), 'ac-control-history-')) directories.push(directory) - return { directory, history: new FileControlHistory(directory) } + return { directory, history: new FileControlHistory(directory, { now: RECORDED_AT }) } } const caller = { kind: 'external' as const, id: 'trial-client' } function executor(history: ControlHistory, handler: () => Promise = async () => 'done') { @@ -150,7 +158,7 @@ async function seeded() { type Recovery = { kind: string; quarantinePath: string; sha256: string; keyedCallsBlocked: boolean } function reopen(directory: string) { const reports: Recovery[] = [] - return { history: new FileControlHistory(directory, { onRecovered: report => reports.push(report as Recovery) }), reports } + return { history: new FileControlHistory(directory, { now: RECORDED_AT, onRecovered: report => reports.push(report as Recovery) }), reports } } // The real keyed row as its own external caller would retry it. const realCaller = { kind: 'external' as const, id: realRows.keyedReceived.caller.replace(/^external:/, '') } @@ -458,3 +466,376 @@ describe('damaged history recovery (#1240) keeps request keys idempotent', () => await access(path) }) }) + +// #1274: the journal and its payloads grew forever (129 MB after 22 days on +// the owner's machine, 105 MB of it read-only transcripts.page results) and +// the whole journal lived in memory. Retention may only drop calls that can +// never be looked up for dedupe (steering q12): unkeyed, finished, not reused. +const retentionRows = JSON.parse(await readFile(join(import.meta.dirname, + '../../../../testing/fixtures/control-history/retention-rows-2026-09-27.json'), 'utf8')) as { + rows: Array<{ sequence: number; callId: string; kind: string; payload?: string; requestKey?: string }> +} +// "Now" for retention cases. It was 2026-09-27 against a 7-day window; the +// owner's 90-day window (2026-09-27) is 83 days longer, so "now" moves 83 days +// later. Every recorded call then sits exactly where it did relative to the +// window's edge, and the same rows are pruned and kept. +const RETENTION_NOW = new Date('2026-12-19T12:00:00.000Z') +const OLD_UNKEYED = ['95caa49c-eb60-44e2-9b62-938ce2243d11', '5ea10842-a1c8-463c-aee9-5d0e238c56e1', '37763f2f-c479-49b7-8d79-4c990bc0784c'] +const KEPT = ['95c97fce-bca0-4bc7-9901-90b45d988d1c', '13e43d53-5120-41ae-88d7-820dc9088728', 'a346f752-9eec-4b37-b7df-145dfaf1aaf5', + 'e8a21d19-b139-4012-8e09-6139b5643bb5', '2ebe82c0-c877-4bb6-9877-56ded38d4739'] +// The old unkeyed externalControl.status and transcripts.page calls' +// `dispatched` rows name the same digest as the keyed app.windowFocus call's +// `dispatched` row (#1330 review B5 corrected which rows); pruning the first +// two must not take it away from the third. +const SHARED_DIGEST = 'e5624e8c0ef7518948b17f88486be4658cdb3ba0e9c92a02aea6ad365bb92fd1' +const ORPHAN_DIGEST = 'f'.repeat(64) + +describe('control history retention (#1274)', () => { + // Payload CONTENTS are not recorded (they hold prompts and results), and + // retention now READS result and step payloads (#1330 review: an unknown + // outcome or a task origin must be kept), through the same digest check + // every reader uses. So each recorded digest is replaced, here and not in + // the fixture, by the digest of a body of the right shape for the row + // that first names it: a settled `completed` result, an ordinary owner + // step, or an opaque input. One recorded digest maps to one body, so rows + // that shared a payload still share one. Everything else is the recording. + const bodies = new Map() + for (const row of retentionRows.rows) { + if (!row.payload || bodies.has(row.payload)) continue + bodies.set(row.payload, JSON.stringify(row.kind === 'result' + ? { ok: true, value: { recorded: row.payload }, operation: { callId: row.callId, instanceId: 'recorded', status: 'completed' } } + : row.kind === 'step' ? { step: 'resolve-owner', recorded: row.payload } : { recorded: row.payload })) + } + const digestOf = (body: string) => createHash('sha256').update(body).digest('hex') + const remap = new Map([...bodies].map(([recorded, body]) => [recorded, digestOf(body)])) + const rowsWithBodies = retentionRows.rows.map(row => row.payload ? { ...row, payload: remap.get(row.payload)! } : row) + const SHARED = remap.get(SHARED_DIGEST)! + async function writePayload(directory: string, body: string): Promise { + const { mkdir } = await import('node:fs/promises') + await mkdir(join(directory, 'payloads'), { recursive: true }) + const digest = digestOf(body) + await writeFile(join(directory, 'payloads', `${digest}.json`), body) + return digest + } + async function seededJournal(rows: Array & { callId: string; kind: string; payload?: string }> = rowsWithBodies) { + const { directory } = await setup() + await writeFile(join(directory, 'events.jsonl'), rows.map((row, index) => `${JSON.stringify({ ...row, sequence: index + 1 })}\n`).join(''), { mode: 0o600 }) + for (const body of bodies.values()) await writePayload(directory, body) + // An orphan, as a failed append leaves, and a temp file of a write that + // never renamed: GC takes the first and must never touch the second. + const { mkdir } = await import('node:fs/promises') + await mkdir(join(directory, 'payloads'), { recursive: true }) + await writeFile(join(directory, 'payloads', `${ORPHAN_DIGEST}.json`), '{}') + await writeFile(join(directory, 'payloads', `${ORPHAN_DIGEST}.json.tmp`), '{}') + return directory + } + const open = (directory: string, onRecovered?: () => void) => + new FileControlHistory(directory, { now: () => RETENTION_NOW, onRecovered }) + + it('drops old unkeyed finished calls and their payloads, and keeps every keyed, reused and recent call', async () => { + const directory = await seededJournal() + const events = await open(directory).events() + expect([...new Set(events.map(event => event.callId))].sort()).toEqual([...KEPT].sort()) + // Kept rows are the recorded rows, in order, with only sequences renumbered. + const expected = rowsWithBodies.filter(row => KEPT.includes(row.callId)) + expect(events.map(({ sequence: _s, ...rest }) => rest)).toEqual(expected.map(({ sequence: _s, ...rest }) => rest)) + expect(events.map(event => event.sequence)).toEqual(expected.map((_, index) => index + 1)) + const payloads = new Set(await readdir(join(directory, 'payloads'))) + const keptDigests = new Set(expected.map(row => row.payload!).filter(Boolean)) + expect(payloads).toEqual(new Set([...[...keptDigests].map(digest => `${digest}.json`), `${ORPHAN_DIGEST}.json.tmp`])) + expect(payloads.has(`${SHARED}.json`)).toBe(true) + expect(payloads.has(`${ORPHAN_DIGEST}.json`)).toBe(false) + // The rewrite is durable and clean: a second launch prunes nothing and + // reports no recovery. + const reports: unknown[] = [] + expect(await open(directory, () => reports.push(1)).events()).toEqual(events) + expect(reports).toEqual([]) + }) + + it('keeps a call with no result row however old it is', async () => { + const rows = rowsWithBodies.filter(row => !(row.callId === OLD_UNKEYED[2] && row.kind === 'result')) + const directory = await seededJournal(rows) + const events = await open(directory).events() + expect(events.some(event => event.callId === OLD_UNKEYED[2])).toBe(true) + expect(events.some(event => event.callId === OLD_UNKEYED[0])).toBe(false) + }) + + it('does not prune in a launch that had to recover the journal', async () => { + const directory = await seededJournal() + await appendFile(join(directory, 'events.jsonl'), '{"sequence":31,"kind":"resu') + const reports: unknown[] = [] + const events = await open(directory, () => reports.push(1)).events() + expect(reports).toHaveLength(1) + expect(events.some(event => event.callId === OLD_UNKEYED[0])).toBe(true) + }) + + it('still replays an old keyed call after its unkeyed neighbours were pruned', async () => { + const { directory } = await setup() + const then = new Date('2026-09-01T00:00:00.000Z') + let effects = 0 + const at = (clock: () => Date, history: ControlHistory) => { + const registry = createControlRegistry() + registry.register({ kind: 'main', generation: 'trial' }, [defineCapability({ + id: 'trial.act', title: 'Harmless trial', description: 'Exercise durable admission', + execution: 'main', effect: 'mutation', input: z.object({ text: z.string() }), output: z.unknown(), + handler: async () => { effects++; return 'sent' }, + })]) + return createControlExecutor({ history, instanceId: randomUUID(), id: randomUUID, now: () => clock().toISOString(), + catalog: () => registry.list(), dispatch: (req, context) => registry.invoke(req, context) }) + } + const old = new FileControlHistory(directory, { now: () => then }) + const first = await at(() => then, old).invoke(request, caller) + await at(() => then, old).invoke({ ...request, requestKey: undefined, input: { text: 'unkeyed' } }, caller) + expect(effects).toBe(2) + + const later = new FileControlHistory(directory, { now: () => RETENTION_NOW }) + const replay = await at(() => RETENTION_NOW, later).invoke(request, caller) + expect(replay).toMatchObject({ ok: true, value: 'sent', operation: { reusedCallId: first.operation?.callId } }) + expect(effects).toBe(2) + const kept = await new FileControlHistory(directory, { now: () => RETENTION_NOW }).events() + expect(kept.every(event => event.requestKey === request.requestKey)).toBe(true) + }) + + // Rows shaped exactly like the executor's and the task writer's, for one + // unkeyed call ending at `at`, each with a real hashed payload. + type Built = { callId: string; rows: Array & { callId: string; kind: string; payload?: string }> } + async function call(directory: string, options: { at: string; result?: unknown; steps?: unknown[]; reusedCallId?: string; callId?: string }): Promise { + const callId = options.callId ?? randomUUID() + const base = { at: options.at, instanceId: 'recorded', callId, capabilityId: 'agents.resume', caller: 'external:agent-code-control' } + const rows: Built['rows'] = [ + { ...base, kind: 'received', payload: await writePayload(directory, JSON.stringify({ input: { callId } })) }, + { ...base, kind: 'dispatched', payload: await writePayload(directory, JSON.stringify({ owner: { kind: 'main', generation: 'g' } })) }, + ] + for (const step of options.steps ?? []) rows.push({ ...base, kind: 'step', payload: await writePayload(directory, JSON.stringify(step)) }) + if (options.result !== undefined) rows.push({ ...base, kind: 'result', payload: await writePayload(directory, JSON.stringify(options.result)), ...(options.reusedCallId ? { reusedCallId: options.reusedCallId } : {}) }) + return { callId, rows } + } + async function journal(directory: string, calls: Built[]) { + await writeFile(join(directory, 'events.jsonl'), calls.flatMap(built => built.rows).map((row, index) => `${JSON.stringify({ ...row, sequence: index + 1 })}\n`).join(''), { mode: 0o600 }) + } + const OLD = '2026-09-01T00:00:00.000Z' + const settled = (status = 'completed') => ({ ok: true, value: {}, operation: { callId: 'x', instanceId: 'recorded', status } }) + const kept = async (directory: string, ids: string[]) => { + const present = new Set((await open(directory).events()).map(event => event.callId)) + return ids.map(id => present.has(id)) + } + + // #1330 review A1/C1, q49: an unkeyed task's original call is the task + // store's lookup key. Pruning it turned operations.read into not_found and + // made operations.finish unable to find its origin. + it('keeps an unkeyed task origin, so operations.read still answers after the window', async () => { + const { directory } = await setup() + const owner = { kind: 'main' as const, generation: 'g' } + const task = await call(directory, { at: OLD, result: settled('pending'), steps: [ + { step: 'task.started', owner }, + { step: 'task.finished', result: { ok: true, value: { newSessionId: 'new' } } }, + ] }) + const plain = await call(directory, { at: OLD, result: settled() }) + await journal(directory, [task, plain]) + const history = open(directory) + const read = taskHistoryCapabilities(history, () => false).find(item => item.descriptor.id === 'operations.read')! + const context = { requestId: 'read', owner, caller: { kind: 'external' as const, id: 'operator' } } + expect(await read.execute({ callId: task.callId }, context)).toMatchObject({ ok: true, value: { status: 'completed', result: { ok: true, value: { newSessionId: 'new' } } } }) + expect(await kept(directory, [task.callId, plain.callId])).toEqual([true, false]) + }) + + // #1330 review A3, q49: an unknown outcome is the evidence that the effect + // may have run; `pending` is still open. Only a proven-settled result goes. + it('keeps results that are not proven settled, and prunes settled ones', async () => { + const { directory } = await setup() + const unknown = await call(directory, { at: OLD, result: { ok: false, error: { code: 'unavailable', message: 'lost', outcome: 'unknown' }, operation: { callId: 'x', instanceId: 'recorded', status: 'outcome_unknown' } } }) + const pending = await call(directory, { at: OLD, result: settled('pending') }) + const refused = await call(directory, { at: OLD, result: { ok: false, error: { code: 'unavailable', message: 'no', outcome: 'not_started' }, operation: { callId: 'x', instanceId: 'recorded', status: 'blocked' } } }) + const done = await call(directory, { at: OLD, result: settled() }) + const opened = await call(directory, { at: OLD, result: settled('ui_opened') }) + const transport = await call(directory, { at: OLD, result: { direction: 'outbound', payload: { jsonrpc: '2.0' } } }) + await journal(directory, [unknown, pending, refused, done, opened, transport]) + expect(await kept(directory, [unknown, pending, refused, done, opened, transport].map(built => built.callId))) + .toEqual([true, true, false, false, false, false]) + // The unknown outcome's payload is still readable for reconciliation. + const history = open(directory) + const row = (await history.events()).find(event => event.callId === unknown.callId && event.kind === 'result')! + expect(await history.payload(row.payload!)).toMatchObject({ error: { outcome: 'unknown' } }) + }) + + // #1330 review A2, q49: an unaccepted recovery's quarantine names rows and + // payloads the operator must still be able to read. The first launch + // recovers (no prune); the second must not prune what the quarantine + // names; once the operator accepts the digest, it may go. + it('keeps what an unaccepted recovery quarantine names, until it is accepted', async () => { + const { directory } = await setup() + const old = await call(directory, { at: OLD, result: settled() }) + await journal(directory, [old]) + await appendFile(join(directory, 'events.jsonl'), 'not json\n') + const reports: Array<{ sha256: string }> = [] + await new FileControlHistory(directory, { now: () => RETENTION_NOW, onRecovered: report => reports.push(report) }).events() + expect(reports).toHaveLength(1) + const second = open(directory) + const events = await second.events() + const result = events.find(event => event.callId === old.callId && event.kind === 'result') + expect(result).toBeDefined() + expect(await second.payload(result!.payload!)).toMatchObject({ ok: true }) + await writeFile(join(directory, 'recovery-accepted.json'), JSON.stringify({ accepted: [reports[0]!.sha256] })) + expect(await kept(directory, [old.callId])).toEqual([false]) + }) + + // #1330 review B1: payload GC must only follow a durable rewrite. A + // rewrite that fails (here: the directory refuses the temp file) leaves the + // old journal naming every payload, and the load itself still works. + it('leaves every payload and serves the unpruned rows when the rewrite fails', async () => { + const directory = await seededJournal() + const before = new Set(await readdir(join(directory, 'payloads'))) + await chmod(directory, 0o500) + try { + const events = await open(directory).events() + expect(new Set(events.map(event => event.callId))).toEqual(new Set(retentionRows.rows.map(row => row.callId))) + expect(new Set(await readdir(join(directory, 'payloads')))).toEqual(before) + } finally { await chmod(directory, 0o700) } + // Writable again, the next launch prunes as usual. + expect(await kept(directory, OLD_UNKEYED)).toEqual([false, false, false]) + }) + + // #1330 review A/B survivors: each rule on its own. + it('keeps an old unkeyed call a kept duplicate reuses, and a call of unknown age', async () => { + const { directory } = await setup() + const target = await call(directory, { at: OLD, result: settled() }) + const reuser = await call(directory, { at: RETENTION_NOW.toISOString(), result: settled(), reusedCallId: target.callId }) + const unknownAge = await call(directory, { at: 'not a time', result: settled() }) + await journal(directory, [target, reuser, unknownAge]) + expect(await kept(directory, [target.callId, reuser.callId, unknownAge.callId])).toEqual([true, true, true]) + }) + + // #1330 review C: the window is pinned at its boundary, not only by rows + // three weeks apart. + it('prunes a call exactly past the window and keeps one just inside it', async () => { + const { directory } = await setup() + const minute = 60_000 + const inside = await call(directory, { at: new Date(RETENTION_NOW.getTime() - CONTROL_HISTORY_RETENTION_MS + minute).toISOString(), result: settled() }) + const outside = await call(directory, { at: new Date(RETENTION_NOW.getTime() - CONTROL_HISTORY_RETENTION_MS - minute).toISOString(), result: settled() }) + // Exactly at the edge is still inside (#1330 verification: `>=` vs `>` + // survived every other case). With infrequent deletion as the owner's + // standing preference, the boundary instant is kept. + const edge = await call(directory, { at: new Date(RETENTION_NOW.getTime() - CONTROL_HISTORY_RETENTION_MS).toISOString(), result: settled() }) + await journal(directory, [inside, outside, edge]) + expect(await kept(directory, [inside.callId, outside.callId, edge.callId])).toEqual([true, false, true]) + // The owner's 90-day window (2026-09-27); pinned by value, not only through the constant. + expect(CONTROL_HISTORY_RETENTION_MS).toBe(90 * 24 * 60 * 60 * 1000) + }) + + // #1330 round 2 (a, b, c; Blocker): once the rewrite has landed, a failed + // payload deletion must not put the pre-rewrite rows back in memory. Those + // made the next append number itself past the end of the shorter file, and + // the gap got the journal quarantined with every keyed call blocked. + it('serves the rewritten rows when a payload deletion fails after the rewrite', async () => { + const { directory } = await setup() + const old = await call(directory, { at: OLD, result: settled() }) + const recent = await call(directory, { at: RETENTION_NOW.toISOString(), result: settled() }) + await journal(directory, [old, recent]) + // A digest-named DIRECTORY in payloads/: rm() without `recursive` fails. + const { mkdir } = await import('node:fs/promises') + await mkdir(join(directory, 'payloads', `${'e'.repeat(64)}.json`)) + const history = open(directory) + const events = await history.events() + expect(new Set(events.map(event => event.callId))).toEqual(new Set([recent.callId])) + expect(events.map(event => event.sequence)).toEqual(recent.rows.map((_, index) => index + 1)) + // The next append continues the file it is really appending to. + const appended = await history.append({ callId: 'next', instanceId: 'recorded', capabilityId: 'agents.read', caller: 'external:agent-code-control', kind: 'received', at: RETENTION_NOW.toISOString() }) + expect(appended.sequence).toBe(recent.rows.length + 1) + const reports: unknown[] = [] + await open(directory, () => reports.push(1)).events() + expect(reports).toEqual([]) + }) + + // #1330 verification b (Blocker): the same divergence through the rewrite's + // directory sync, which runs AFTER the rename. It failed with EIO, the + // prune rejected, and open() served the six pre-rewrite rows over a + // three-row file; the next append wrote sequence 7 and the next launch + // quarantined the journal. + it('serves the rewritten rows when syncing the directory fails after the rename', async () => { + const { directory } = await setup() + const old = await call(directory, { at: OLD, result: settled() }) + const recent = await call(directory, { at: RETENTION_NOW.toISOString(), result: settled() }) + await journal(directory, [old, recent]) + const sync = vi.spyOn(FileControlHistory.prototype as unknown as { syncDirectory: () => Promise }, 'syncDirectory') + .mockRejectedValueOnce(Object.assign(new Error('EIO: i/o error, fsync'), { code: 'EIO' })) + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + try { + const history = open(directory) + const events = await history.events() + expect(new Set(events.map(event => event.callId))).toEqual(new Set([recent.callId])) + // GC is skipped while the rewrite is unsynced: a power loss could bring + // back the old journal, which names the expired call's payloads. + for (const row of old.rows) await expect(access(join(directory, 'payloads', `${row.payload}.json`))).resolves.toBeUndefined() + const onDisk = (await readFile(join(directory, 'events.jsonl'), 'utf8')).split('\n').filter(Boolean) + expect(events).toHaveLength(onDisk.length) + const appended = await history.append({ callId: 'next', instanceId: 'recorded', capabilityId: 'agents.read', caller: 'external:agent-code-control', kind: 'received', at: RETENTION_NOW.toISOString() }) + expect(appended.sequence).toBe(recent.rows.length + 1) + const reports: unknown[] = [] + await open(directory, () => reports.push(1)).events() + expect(reports).toEqual([]) + } finally { + sync.mockRestore() + warn.mockRestore() + } + }) + + // #1330 round 2 (a, b, c): a torn tail blocks no keyed call, so it has no + // guard, but its torn line can be the only row naming a result payload + // fsynced before the append tore. Kept until the operator accepts it. + it('keeps a payload that only an unaccepted torn-tail quarantine names', async () => { + const { directory } = await setup() + const torn = await call(directory, { at: OLD }) + const unknownResult = await writePayload(directory, JSON.stringify({ ok: false, error: { code: 'unavailable', message: 'lost', outcome: 'unknown' }, operation: { callId: torn.callId, instanceId: 'recorded', status: 'outcome_unknown' } })) + await journal(directory, [torn]) + await appendFile(join(directory, 'events.jsonl'), JSON.stringify({ sequence: 3, at: OLD, instanceId: 'recorded', callId: torn.callId, kind: 'result', capabilityId: 'agents.resume', caller: 'external:agent-code-control', payload: unknownResult }).slice(0, -2)) + const reports: Array<{ sha256: string; kind: string }> = [] + await new FileControlHistory(directory, { now: () => RETENTION_NOW, onRecovered: report => reports.push(report as never) }).events() + expect(reports).toEqual([expect.objectContaining({ kind: 'torn-tail' })]) + await open(directory).events() + expect(await open(directory).payload(unknownResult)).toMatchObject({ error: { outcome: 'unknown' } }) + // Accepted, it is ordinary garbage (no kept row names it). + await writeFile(join(directory, 'recovery-accepted.json'), JSON.stringify({ accepted: [reports[0]!.sha256] })) + await open(directory).events() + await expect(open(directory).payload(unknownResult)).rejects.toThrow() + }) + + // #1330 verification b (survivor): the torn call above is kept for having + // no result, so nothing was expired and GC never had to honour the + // quarantine's digests. Here another old settled call IS pruned in the same + // launch, so GC runs, and the digest only the torn line names must survive. + it('keeps a quarantine-only payload while pruning another call in the same launch', async () => { + const { directory } = await setup() + const torn = await call(directory, { at: OLD }) + const unknownResult = await writePayload(directory, JSON.stringify({ ok: false, error: { code: 'unavailable', message: 'lost', outcome: 'unknown' }, operation: { callId: torn.callId, instanceId: 'recorded', status: 'outcome_unknown' } })) + await journal(directory, [torn]) + await appendFile(join(directory, 'events.jsonl'), JSON.stringify({ sequence: 3, at: OLD, instanceId: 'recorded', callId: torn.callId, kind: 'result', capabilityId: 'agents.resume', caller: 'external:agent-code-control', payload: unknownResult }).slice(0, -2)) + await open(directory).events() + // A clean journal now, plus an old settled call that retention expires. + const expiring = await call(directory, { at: OLD, result: settled() }) + await journal(directory, [torn, expiring]) + expect(await kept(directory, [torn.callId, expiring.callId])).toEqual([true, false]) + expect(await open(directory).payload(unknownResult)).toMatchObject({ error: { outcome: 'unknown' } }) + }) + + // #1330 round 2 (a, b, c): a task origin is recognised by what its step + // payload SAYS (the task store parses it), not by its first bytes, and a + // step that cannot be read is unknown, so kept. Settled results here, so + // only the task rule can keep these calls. + it('keeps a settled task origin whose step has another key order, or cannot be read', async () => { + const { directory } = await setup() + const owner = { kind: 'main' as const, generation: 'g' } + const reordered = await call(directory, { at: OLD, result: settled(), steps: [ + { owner, step: 'task.started' }, + { result: { ok: true, value: { newSessionId: 'new' } }, step: 'task.finished' }, + ] }) + const corrupted = await call(directory, { at: OLD, result: settled(), steps: [{ step: 'task.started', owner }] }) + await journal(directory, [reordered, corrupted]) + const stepDigest = corrupted.rows.find(row => row.kind === 'step')!.payload! + await writeFile(join(directory, 'payloads', `${stepDigest}.json`), '{ "tampered": true }') + const history = open(directory) + const read = taskHistoryCapabilities(history, () => false).find(item => item.descriptor.id === 'operations.read')! + expect(await read.execute({ callId: reordered.callId }, { requestId: 'read', owner, caller: { kind: 'external', id: 'operator' } })) + .toMatchObject({ ok: true, value: { status: 'completed' } }) + expect(await kept(directory, [reordered.callId, corrupted.callId])).toEqual([true, true]) + }) +}) diff --git a/src/main/control/history/FileControlHistory.ts b/src/main/control/history/FileControlHistory.ts index 16066cd3e..cff4bcfb5 100644 --- a/src/main/control/history/FileControlHistory.ts +++ b/src/main/control/history/FileControlHistory.ts @@ -56,6 +56,17 @@ const EXECUTOR_KINDS = new Set(['received', 'dispatched', 'result', 'dup // A malformed recovery.json preserved aside when a new recovery must rewrite // the marker: its block cannot be read, so it stays a global block of its own. const INVALID_MARKER = /^recovery\.invalid-.+\.json$/ +// How long a call that can never be looked up for dedupe stays readable +// (#1274). OWNER DECISION (2026-09-27, temp/manager/owner-decisions-2026-09-27.md): +// 90 days, with the standing preference "infrequent deletion": default to +// longer retention. The PR first proposed 7 days (an agent inspecting its +// own work looks back hours, not weeks). The owner chose the longer window. +// Cost, on the owner's rate (~5 MB of payloads a day, nearly all +// transcripts.page reads): up to ~450 MB at steady state. Today's store is +// under 30 days old, so this window deletes nothing yet. What it bounds is +// the long run: the journal, which is held whole in memory, no longer grows +// without limit. +export const CONTROL_HISTORY_RETENTION_MS = 90 * 24 * 60 * 60 * 1000 const recoveryFileSchema = z.object({ quarantines: z.array(z.object({ file: z.string().refine(name => QUARANTINE.test(name) || INVALID_MARKER.test(name)), @@ -105,7 +116,12 @@ export class FileControlHistory implements ControlHistory { private guards: Guard[] = [] constructor( private readonly directory: string, - private readonly options: { onRecovered?: (recovery: ControlHistoryRecovery) => void } = {}, + private readonly options: { + onRecovered?: (recovery: ControlHistoryRecovery) => void + /** Clock for retention; injectable so tests can age a real journal. */ + now?: () => Date + retentionMs?: number + } = {}, ) {} private load(): Promise { @@ -120,15 +136,213 @@ export class FileControlHistory implements ControlHistory { if ((error as NodeJS.ErrnoException).code !== 'ENOENT') throw error } let events: HistoryEvent[] = [] + let recovered = false if (bytes) { const analysis = analyze(bytes.toString('utf8')) events = analysis.events - if (analysis.torn || analysis.damaged) await this.recover(bytes, analysis) + recovered = analysis.torn || analysis.damaged + if (recovered) await this.recover(bytes, analysis) } this.guards = await this.readGuards() + // WHY a launch that recovered does not also prune: the operator is about + // to reconcile a damaged ledger against what it showed at recovery time, + // and a second rewrite in the same launch would change it under them. + // The next clean launch prunes. + if (!recovered) { + // WHY a failed prune does not fail the load (#1330 review B1): retention + // is housekeeping, and the journal is the dedupe ledger every control + // call needs. A read-only or full disk must leave control working on the + // unpruned rows, and a failed rewrite throws before any payload is + // deleted, so the old journal never names a missing payload. + events = await this.prune(events).catch(error => { + console.warn('[control-history] retention skipped this launch:', (error as NodeJS.ErrnoException).code ?? 'error') + return events + }) + } return events } + // Retention (#1274). WHY on load and nowhere else: load is the one moment + // no append is in flight (appends wait on `load()`), so the rewrite cannot + // race a writer, and the app relaunches often (updates, restarts). A + // process running for weeks grows until its next launch; that is the + // accepted residual, bounded by the same rate. + // + // WHY whole calls, and only these (steering q12): the journal is the + // executor's dedupe ledger and request keys have no lifetime in the + // contract, so any call carrying a key is kept forever (815 of 6,089 rows + // on the owner's machine). A call with no `result` may still be in flight + // or is evidence of an interrupted one; a call a kept `duplicate` names + // via reusedCallId is where that duplicate's answer lives. Dropping WHOLE + // calls keeps the per-call key consistency analyze() checks, so the + // pruned ledger reloads as clean instead of being quarantined. + private async prune(events: HistoryEvent[]): Promise { + const cutoff = (this.options.now?.() ?? new Date()).getTime() - (this.options.retentionMs ?? CONTROL_HISTORY_RETENTION_MS) + const calls = new Map() + const reused = new Set() + for (const event of events) { + const call = calls.get(event.callId) ?? { keyed: false, finished: false, newest: 0, rows: [] } + if (event.requestKey !== undefined) call.keyed = true + if (event.kind === 'result') call.finished = true + // An unparseable time counts as NEW: unknown age is never a reason to + // delete evidence. + const at = Date.parse(event.at) + call.newest = Math.max(call.newest, Number.isFinite(at) ? at : Number.POSITIVE_INFINITY) + call.rows.push(event) + calls.set(event.callId, call) + if (event.reusedCallId) reused.add(event.reusedCallId) + } + // What unaccepted recovery evidence names stays until the operator + // accepts it (#1330 review A2, q49): the quarantine copy holds the rows, + // but only this directory holds their payloads, and reconciling a damaged + // ledger means reading exactly those. + const evidence = await this.quarantineEvidence() + const expired = new Set() + for (const [id, call] of calls) { + if (call.keyed || !call.finished || reused.has(id) || call.newest >= cutoff || evidence.callIds.has(id)) continue + if (await this.mustOutliveRetention(call.rows)) continue + expired.add(id) + } + if (expired.size === 0 && !(await this.hasUnreferencedPayloads(events, evidence.digests))) return events + const kept = events.filter(event => !expired.has(event.callId)) + // Renumbered COPIES: if the rewrite fails, the caller keeps serving the + // original rows with the sequences the unchanged file still has. + const renumbered = kept.map((event, index) => ({ ...event, sequence: index + 1 })) + if (expired.size > 0) { + // WHY the rename is tracked (#1330 verification b; Blocker): the rows on + // disk change at the RENAME, not when `writeAtomic` resolves. Its + // directory sync comes after, and when that failed (EIO) the whole + // prune rejected, so open() served the pre-rewrite rows over a journal + // that no longer had them: the next append numbered itself from the + // longer list and the gap got the journal quarantined. Once the rename + // landed, the renumbered rows are served whatever fails next. + // + // An unsynced directory entry can still be undone by a power loss, + // bringing the OLD journal back. Its extra rows name the expired calls' + // payloads, so GC is skipped for this launch (`unsynced`): the old + // journal must never come back naming deleted files. The orphans are + // collected by the next launch whose rewrite syncs. + const renamed = { landed: false } + try { + await this.writeAtomic(join(this.directory, JOURNAL), renumbered.map(event => `${JSON.stringify(event)}\n`).join(''), renamed) + } catch (error) { + if (!renamed.landed) throw error + console.warn('[control-history] retention rewrite landed, but syncing its directory failed:', (error as NodeJS.ErrnoException).code ?? 'error') + return renumbered + } + } + // Payload GC runs only AFTER the rewrite is durable (a failed rewrite + // throws above, so GC never runs; see open()): a crash in between leaves + // unreferenced files, collected next launch, never a kept row pointing + // at a deleted payload. It also collects orphans from appends that stored + // a payload and then failed. A digest any kept row or unaccepted + // quarantine names stays. + // + // WHY GC can no longer fail the prune (#1330 round 2, all three; Blocker): + // once the rewrite has landed, the rows on disk ARE `renumbered`, so they + // are what must be served whatever happens next. A failed `rm` used to + // reject the whole prune, open() then served the pre-rewrite rows, the + // next append numbered itself from that longer list, and the gap got the + // journal quarantined with every keyed call blocked. Each deletion now + // fails on its own, and a leftover file is collected next launch. + const served = expired.size > 0 ? renumbered : events + const referenced = new Set([...kept.flatMap(event => event.payload ? [event.payload] : []), ...evidence.digests].map(digest => `${digest}.json`)) + const names = await readdir(join(this.directory, 'payloads')).catch(() => [] as string[]) + for (const name of names) { + // Only finished payload files: a `.tmp` belongs to a write that has not + // renamed yet (none can be in flight here, but that is the writer's + // business, not GC's). + if (!/^[a-f0-9]{64}\.json$/.test(name) || referenced.has(name)) continue + await rm(join(this.directory, 'payloads', name), { force: true }).catch(error => { + console.warn('[control-history] could not remove an unreferenced payload; retrying next launch:', (error as NodeJS.ErrnoException).code ?? 'error') + }) + } + return served + } + + // Whether an old, unkeyed, finished call must be kept anyway: its evidence + // is still the only answer to a question someone can ask (#1330 review + // A1/A3/C1, steering q49). + // - A TASK ORIGIN: operations.read/finish look the task up by the original + // call's id and read its `task.*` step payloads. Its tool contract says + // task results persist across restarts, and nothing expires a task, so + // these are kept (12 of 1,894 calls on the owner's machine). + // - A result that is not PROVEN settled: `outcome_unknown` means the + // effect may have run and must not be blindly retried; `pending` is an + // accepted operation whose outcome is still open. Only a completed or + // ui_opened success, a not_started refusal, or an MCP transport echo + // (which reports no operation) is settled. A missing or unreadable + // payload is unknown, and unknown is kept (q40). + // Payloads are read only for calls that are otherwise expired, so steady + // state reads about a day's worth per launch. + private async mustOutliveRetention(rows: HistoryEvent[]): Promise { + for (const row of rows) { + if (row.kind === 'step' && row.payload) { + // Parsed through the same integrity-checked read the task store uses + // (#1330 round 2): a byte-prefix test missed a valid step written + // with another key order, and treated a corrupted step as an ordinary + // one. Unreadable or not an object: unknown, so kept. + const step = await this.payload(row.payload).catch(() => undefined) as { step?: unknown } | undefined + if (!step || typeof step !== 'object' || (typeof step.step === 'string' && step.step.startsWith('task.'))) return true + } + if (row.kind === 'result') { + if (!row.payload) return true + const result = await this.payload(row.payload).catch(() => undefined) as { + ok?: unknown; error?: { outcome?: unknown }; operation?: { status?: unknown }; direction?: unknown + } | undefined + if (!result || typeof result !== 'object') return true + const settled = result.ok === true + ? result.operation?.status === 'completed' || result.operation?.status === 'ui_opened' + : result.ok === false + ? result.error?.outcome === 'not_started' + : 'direction' in result && !('operation' in result) + if (!settled) return true + } + } + return false + } + + // Call ids and payload digests named by every quarantine file whose digest + // is not accepted. WHY every file and not `this.guards` (#1330 round 2, all + // three): guards drop a torn-tail quarantine because it blocks no keyed + // call, but its torn last line can be the ONLY row naming a result payload + // that was fsynced before the append tore, and that payload is the outcome + // the operator reconciles against. Evidence retention asks "has the + // operator accepted this file", not "does it block anything". Rows in a + // quarantine may be damaged, so each line is parsed on its own and a line + // that does not parse still yields every digest-shaped token in it. + private async quarantineEvidence(): Promise<{ callIds: Set; digests: Set }> { + const callIds = new Set() + const digests = new Set() + const accepted = await this.acceptedDigests() + for (const name of await readdir(this.directory)) { + if (!QUARANTINE.test(name)) continue + const bytes = await readFile(join(this.directory, name)).catch(() => null) + // Unreadable evidence cannot be scanned, and unknown must not delete: + // skip retention for this launch entirely. + if (bytes === null) throw new Error('quarantine evidence unreadable') + if (accepted.has(createHash('sha256').update(bytes).digest('hex'))) continue + for (const line of bytes.toString('utf8').split('\n')) { + for (const digest of line.match(/[a-f0-9]{64}/g) ?? []) digests.add(digest) + try { + const row = JSON.parse(line) as { callId?: unknown } + if (typeof row.callId === 'string') callIds.add(row.callId) + } catch { + // A damaged line: its digests were collected above, and a call id + // is recovered from its text when one is visible. + const callId = /"callId":"([^"]+)"/.exec(line)?.[1] + if (callId) callIds.add(callId) + } + } + } + return { callIds, digests } + } + + private async hasUnreferencedPayloads(events: HistoryEvent[], protectedDigests: Set): Promise { + const referenced = new Set([...events.flatMap(event => event.payload ? [event.payload] : []), ...protectedDigests].map(digest => `${digest}.json`)) + return (await readdir(join(this.directory, 'payloads'))).some(name => /^[a-f0-9]{64}\.json$/.test(name) && !referenced.has(name)) + } + private async recover(bytes: Buffer, analysis: Analysis): Promise { const sha256 = createHash('sha256').update(bytes).digest('hex') const stamp = new Date().toISOString().replace(/[:.]/g, '-') @@ -207,12 +421,16 @@ export class FileControlHistory implements ControlHistory { // that analysis lifted a block nobody accepted (#1254 review A). The // record's own digest is what accepts it. guards.push(...records) - let accepted = new Set() + const accepted = await this.acceptedDigests() + return guards.filter(guard => (guard.keyedCallsBlocked || guard.blockedPairs.length > 0) && !accepted.has(guard.sha256)) + } + + private async acceptedDigests(): Promise> { try { const parsed = JSON.parse(await readFile(join(this.directory, ACCEPTED), 'utf8')) as { accepted?: unknown } - if (Array.isArray(parsed.accepted)) accepted = new Set(parsed.accepted.filter((id): id is string => typeof id === 'string')) + if (Array.isArray(parsed.accepted)) return new Set(parsed.accepted.filter((id): id is string => typeof id === 'string')) } catch { /* absent or unreadable: nothing accepted, the conservative reading */ } - return guards.filter(guard => (guard.keyedCallsBlocked || guard.blockedPairs.length > 0) && !accepted.has(guard.sha256)) + return new Set() } private refusal(write: HistoryWrite): string | null { @@ -283,12 +501,14 @@ export class FileControlHistory implements ControlHistory { try { await file.sync() } finally { await file.close() } } - private async writeAtomic(path: string, text: string): Promise { + private async writeAtomic(path: string, text: string, renamed?: { landed: boolean }): Promise { const temporary = `${path}.${randomUUID()}.tmp` try { const file = await open(temporary, 'wx', 0o600) try { await file.writeFile(text); await file.sync() } finally { await file.close() } await rename(temporary, path) + // From here the new contents ARE the file; see the retention rewrite. + if (renamed) renamed.landed = true await this.syncDirectory(this.directory) } finally { await rm(temporary, { force: true }) } } diff --git a/src/main/control/history/control.ts b/src/main/control/history/control.ts index 75bda3119..9d0d5aae1 100644 --- a/src/main/control/history/control.ts +++ b/src/main/control/history/control.ts @@ -5,7 +5,7 @@ export function historyCapabilities(history: ControlHistory) { return [ defineCapability({ id: 'history.read', title: 'Inspect one control call', execution: 'main', effect: 'read', - description: 'Return every durable event and payload reference for a call, including retries and unresolved outcomes. Retrieve each payload with history.payloadRead.', + description: 'Return every durable event and payload reference for a call, including retries and unresolved outcomes. A settled call without a request key that finished more than 90 days ago (not a task origin, not an unknown or pending outcome) reads as not_found. Retrieve each payload with history.payloadRead.', input: z.object({ callId: z.string().min(1).describe('operation.callId from a tool result, or callId from history.list.') }).strict(), output: z.object({ callId: z.string(), events: z.array(historyEventSchema), relatedCalls: z.array(z.string()), state: z.enum(['recorded', 'outcome_unknown', 'not_found']) }), @@ -18,9 +18,12 @@ export function historyCapabilities(history: ControlHistory) { }), defineCapability({ id: 'history.list', title: 'List control history', execution: 'main', effect: 'read', - description: 'Read durable invocation events. Carry snapshot through paging so reading history does not chase its own new records.', + // The retention window is part of what this tool promises (#1274): a page + // is never clipped, but calls outside the window are gone from the + // journal, and an agent reading an empty result must know why. + description: 'Read durable invocation events. Kept indefinitely: calls with a request key, calls without a result or whose outcome is unknown or still pending, lifecycle task origins, calls a kept retry reuses, and calls named by unaccepted recovery evidence. Other finished calls are kept for 90 days. Carry snapshot through paging so reading history does not chase its own new records.', input: z.object({ after: z.number().int().nonnegative().default(0).describe('Exclusive event sequence boundary; use nextAfter from the previous page.'), snapshot: z.number().int().nonnegative().optional().describe('Keep the first page’s snapshot unchanged to finish a finite history read while new calls are recorded.'), - limit: z.number().int().min(1).max(200).default(50).describe('Maximum events per page; call history is never silently truncated.'), callId: z.string().optional().describe('Optional exact call ID to filter events.') }).strict(), + limit: z.number().int().min(1).max(200).default(50).describe('Maximum events per page; a page is never silently truncated.'), callId: z.string().optional().describe('Optional exact call ID to filter events.') }).strict(), output: z.object({ events: z.array(historyEventSchema), snapshot: z.number().int(), nextAfter: z.number().int().nullable(), complete: z.boolean() }), handler: async ({ after, snapshot, limit, callId }, context) => { const events = await history.events() diff --git a/src/main/ipc/setup.test.ts b/src/main/ipc/setup.test.ts new file mode 100644 index 000000000..e756856e1 --- /dev/null +++ b/src/main/ipc/setup.test.ts @@ -0,0 +1,92 @@ +import { mkdtemp, readFile, rm } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { join } from 'node:path' + +import { afterEach, beforeEach, expect, it, vi } from 'vitest' + +// #1403 verification b: every setup answer is saved FIRST, then the IPC runs +// the prerequisite check, which writes its probed tool paths back to the same +// setup.json. When only that second write failed, the IPC rejected and the +// renderer said the answer was not saved, while it was on disk and took +// effect on the next launch. +// +// The REAL handlers, the real setup-state persistence (scratch STATE_DIR) and +// the real prerequisite check. Replaced edges only: Electron's ipcMain (to +// capture the handlers), the login-shell/PATH probes and bundled-archive +// lookups (machine-dependent), the toolchain refresh (process env), and the +// Homebrew installer (not exercised). The write failure is a real rename +// rejection injected into fs/promises, planned per write. + +const paths = vi.hoisted(() => ({ STATE_DIR: '' })) +vi.mock('@main/storage/paths.js', () => paths) +const handlers = vi.hoisted(() => new Map Promise>()) +vi.mock('electron', () => ({ + ipcMain: { handle: (channel: string, handler: (...args: unknown[]) => Promise) => handlers.set(channel, handler) }, +})) +vi.mock('@main/setup/binaryResolver.js', () => ({ + resolveToolPath: async () => null, + isExecutable: async () => false, + classifyExecutable: async () => 'ok', +})) +vi.mock('@main/setup/runtimeTools.js', () => ({ isBundledArchiveAvailable: async () => false })) +vi.mock('@main/setup/toolchain.js', () => ({ refreshToolchainFromState: async () => {} })) +vi.mock('@main/setup/homebrewInstaller.js', () => ({ installWithHomebrew: vi.fn() })) +// Each rename in order: `true` fails it. The answer's write is the first, the +// check's write-back the second. +const renamePlan = vi.hoisted(() => ({ fail: [] as boolean[] })) +vi.mock('fs/promises', async importOriginal => { + const actual = await importOriginal() + return { + ...actual, + rename: async (from: string, to: string) => { + if (renamePlan.fail.shift()) throw Object.assign(new Error('ENOSPC: no space left on device'), { code: 'ENOSPC' }) + return await actual.rename(from, to) + }, + } +}) + +let dir: string +beforeEach(async () => { + dir = await mkdtemp(join(tmpdir(), 'setup-ipc-')) + paths.STATE_DIR = dir + handlers.clear() + renamePlan.fail = [] + vi.resetModules() + vi.spyOn(console, 'warn').mockImplementation(() => {}) + const { registerSetupIpc } = await import('./setup.js') + registerSetupIpc() +}) +afterEach(async () => { + vi.restoreAllMocks() + await rm(dir, { recursive: true, force: true }) +}) + +const invoke = (channel: string, ...args: unknown[]) => handlers.get(channel)!({}, ...args) +const onDisk = async () => JSON.parse(await readFile(join(dir, 'setup.json'), 'utf8')) as Record + +it('answers a saved skip even when the check cannot write its tool paths back', async () => { + renamePlan.fail = [false, true] + const check = await invoke('setup:skip-optional', 'mitmdump') as { tools: Record } + expect(check.tools.mitmdump!.skipped).toBe(true) + expect(await onDisk()).toMatchObject({ skippedOptionalTools: { mitmdump: true } }) +}) + +it('answers a saved no-provider acknowledgment even when the check cannot write back', async () => { + renamePlan.fail = [false, true] + const check = await invoke('setup:acknowledge-no-providers') as { noProvidersAcknowledged: boolean } + expect(check.noProvidersAcknowledged).toBe(true) + expect(await onDisk()).toMatchObject({ acknowledgedNoProviders: true }) +}) + +it('answers a saved manual path even when the check cannot write back', async () => { + renamePlan.fail = [false, true] + const result = await invoke('setup:set-tool-path', 'claude', '/opt/bin/claude') as { ok: boolean } + expect(result.ok).toBe(true) + expect(await onDisk()).toMatchObject({ manualToolPaths: { claude: '/opt/bin/claude' } }) +}) + +// The rejection path is kept for what it means: the answer itself was not saved. +it('rejects a skip whose own write fails', async () => { + renamePlan.fail = [true] + await expect(invoke('setup:skip-optional', 'mitmdump')).rejects.toThrow('ENOSPC') +}) diff --git a/src/main/sessionManager.terminalReplay.test.ts b/src/main/sessionManager.terminalReplay.test.ts index 943e8da31..9470e1e85 100644 --- a/src/main/sessionManager.terminalReplay.test.ts +++ b/src/main/sessionManager.terminalReplay.test.ts @@ -1,6 +1,9 @@ import { EventEmitter } from 'node:events' +import { Terminal } from '@xterm/headless' import { expect, it, vi } from 'vitest' +import { SessionManager } from './sessionManager' + // #843 / #1041 review: the SHELL attach path (attachTerminal) must replay the // modes its evicted bytes set, like the agent path. Reverting it to read() // passed every test until this one. A full-screen program run from a shell @@ -26,9 +29,11 @@ vi.mock('@main/performance/PerformanceService.js', () => ({ })) vi.mock('@main/storage/feedDebugLog.js', () => ({ forgetFeedDebugSession: vi.fn() })) +// WHY static imports (#1107): the dynamic import()s that stood here loaded SessionManager's whole +// module graph INSIDE the test body, against the 5 s test budget, so the test passed alone and +// timed out under full-suite parallelism. vi.mock is hoisted above these imports, so the mocks +// still apply; the load now happens at collection, outside any test's budget. it('a remounted shell terminal gets the alternate screen and mouse mode its evicted bytes set', async () => { - const { Terminal } = await import('@xterm/headless') - const { SessionManager } = await import('./sessionManager') // No tmux: a direct PTY terminal, the case every machine without tmux runs. const manager = new SessionManager({ isAvailable: () => false, getBinary: () => null } as never) const { sessionId } = await manager.spawn({ kind: 'terminal', cwd: '/tmp/project' }) diff --git a/src/main/setup/prerequisites.test.ts b/src/main/setup/prerequisites.test.ts new file mode 100644 index 000000000..caa5442a3 --- /dev/null +++ b/src/main/setup/prerequisites.test.ts @@ -0,0 +1,77 @@ +import { mkdir, mkdtemp, readFile, rm, writeFile } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { join } from 'node:path' + +import { afterEach, beforeEach, expect, it, vi } from 'vitest' + +// #1403 recheck a: when the check's tool-path write-back fails, the check +// still answers with what it probed. The toolchain must then launch what the +// check says it found, not the last persisted (possibly dead) path. +// +// The REAL check, toolchain and setup-state persistence in a scratch +// STATE_DIR. Replaced edges: the machine's shell/PATH probes and the bundled +// archives (machine-dependent). The failure is a real rename rejection. + +const paths = vi.hoisted(() => ({ STATE_DIR: '' })) +vi.mock('@main/storage/paths.js', () => paths) +vi.mock('@main/setup/binaryResolver.js', () => ({ + resolveToolPath: async (tool: string) => (tool === 'codex' ? '/fresh/codex' : null), + isExecutable: async (path: string) => path === '/fresh/codex', + classifyExecutable: async () => 'ok', +})) +vi.mock('@main/setup/runtimeTools.js', () => ({ + isBundledArchiveAvailable: async () => false, + resolveBundledTool: async () => null, +})) +const renameFaults = vi.hoisted(() => ({ failNext: 0 })) +vi.mock('fs/promises', async importOriginal => { + const actual = await importOriginal() + return { + ...actual, + rename: async (from: string, to: string) => { + if (renameFaults.failNext > 0) { + renameFaults.failNext -= 1 + throw Object.assign(new Error('ENOSPC: no space left on device'), { code: 'ENOSPC' }) + } + return await actual.rename(from, to) + }, + } +}) + +let dir: string +const originalPath = process.env.PATH +beforeEach(async () => { + dir = await mkdtemp(join(tmpdir(), 'prerequisites-')) + paths.STATE_DIR = dir + renameFaults.failNext = 0 + vi.resetModules() + vi.spyOn(console, 'warn').mockImplementation(() => {}) + await mkdir(dir, { recursive: true }) + // A codex that moved: the persisted path is gone, the probe finds a new one. + await writeFile(join(dir, 'setup.json'), JSON.stringify({ version: 1, toolPaths: { codex: '/gone/codex' } })) +}) +afterEach(async () => { + vi.restoreAllMocks() + process.env.PATH = originalPath + await rm(dir, { recursive: true, force: true }) +}) + +it('launches what the check found when its write-back fails', async () => { + const { checkPrerequisites } = await import('./prerequisites.js') + const { getToolPath } = await import('./toolchain.js') + renameFaults.failNext = 1 + const check = await checkPrerequisites() + expect(check.tools.codex).toMatchObject({ found: true, path: '/fresh/codex' }) + // Not persisted... + expect(JSON.parse(await readFile(join(dir, 'setup.json'), 'utf8')).toolPaths.codex).toBe('/gone/codex') + // ...but what a launch uses in this process agrees with what the panel says. + expect(getToolPath('codex', '')).toBe('/fresh/codex') +}) + +it('launches the persisted probe when the write-back lands', async () => { + const { checkPrerequisites } = await import('./prerequisites.js') + const { getToolPath } = await import('./toolchain.js') + await checkPrerequisites() + expect(JSON.parse(await readFile(join(dir, 'setup.json'), 'utf8')).toolPaths.codex).toBe('/fresh/codex') + expect(getToolPath('codex', '')).toBe('/fresh/codex') +}) diff --git a/src/main/setup/prerequisites.ts b/src/main/setup/prerequisites.ts index 814db09cb..e719995cd 100644 --- a/src/main/setup/prerequisites.ts +++ b/src/main/setup/prerequisites.ts @@ -159,12 +159,28 @@ export async function checkPrerequisites(): Promise { ) const tools = Object.fromEntries(entries) as Record - await updateToolPaths( - Object.fromEntries( - entries.map(([tool, status]) => [tool, status.path]), - ) as Partial>, - ) - await refreshToolchainFromState() + // WHY the write-back is best effort (#1403 verification b): it persists a + // CACHE of what this probe just found, and every caller of the check had + // already saved what the user asked for (skip a helper, continue without a + // provider, a manual path, a provider reset) before running it. When this + // write failed (a full disk, a read-only state dir), the whole check + // rejected, and the renderer then said the answer was not saved although it + // was on disk. The result below is the probe's own answer either way, and + // the toolchain applies the probed paths in memory when they could not be + // persisted (#1403 recheck a), so "found at X" is also what a launch uses. + // The cost: the next launch starts from the last persisted paths until a + // check's write-back lands. + const probed = Object.fromEntries( + entries.map(([tool, status]) => [tool, status.path]), + ) as Partial> + let unsaved: typeof probed | undefined + try { + await updateToolPaths(probed) + } catch (error) { + console.warn('[setup] could not persist probed tool paths:', (error as NodeJS.ErrnoException).code ?? 'error') + unsaved = probed + } + await refreshToolchainFromState(unsaved) // No `ready`/`blocking` any more (#995): nothing blocks launch. The policy // that replaced them is in readiness.ts, and it runs here so every diff --git a/src/main/setup/providerEnablement.test.ts b/src/main/setup/providerEnablement.test.ts new file mode 100644 index 000000000..ce5bcd4ac --- /dev/null +++ b/src/main/setup/providerEnablement.test.ts @@ -0,0 +1,174 @@ +import { mkdir, mkdtemp, readFile, rm } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { join } from 'node:path' + +import { afterEach, beforeEach, expect, it, vi } from 'vitest' + +// #1403 review a and b: what the Providers settings row hears must match what +// is on disk. The row shows "Couldn't save this change. Nothing was changed." +// on ANY rejection, so a rejection must mean the change was not saved. +// +// Real setup-state persistence in a scratch STATE_DIR. Only the edges are +// replaced: the login-shell provider probe (`checkPrerequisites`), the z.ai +// credential probe and the usage cache, none of which this contract is about. + +const paths = vi.hoisted(() => ({ STATE_DIR: '' })) +vi.mock('@main/storage/paths.js', () => paths) +const probe = vi.hoisted(() => ({ checkPrerequisites: vi.fn() })) +vi.mock('@main/setup/prerequisites.js', () => probe) +const zai = vi.hoisted(() => ({ probeZaiCredential: vi.fn(async () => false) })) +vi.mock('@main/usage/zaiUsage.js', () => zai) +vi.mock('@main/usage/usageService.js', () => ({ invalidateUsageSnapshotCache: () => {} })) +// The real rename, with an optional hook per call (in order): a test can hold +// one write open and then fail it. +const renames = vi.hoisted(() => ({ hooks: [] as Array<(() => Promise) | null> })) +vi.mock('fs/promises', async importOriginal => { + const actual = await importOriginal() + return { + ...actual, + rename: async (from: string, to: string) => { + const hook = renames.hooks.shift() + if (hook) await hook() + return await actual.rename(from, to) + }, + } +}) + +let dir: string +beforeEach(async () => { + dir = await mkdtemp(join(tmpdir(), 'provider-enablement-')) + paths.STATE_DIR = dir + probe.checkPrerequisites.mockReset() + probe.checkPrerequisites.mockResolvedValue({ usableProviders: ['claude', 'codex'] }) + zai.probeZaiCredential.mockReset() + zai.probeZaiCredential.mockResolvedValue(false) + renames.hooks = [] + vi.resetModules() + vi.spyOn(console, 'warn').mockImplementation(() => {}) +}) +afterEach(async () => { + vi.restoreAllMocks() + await rm(dir, { recursive: true, force: true }) +}) + +const overridesOnDisk = async (): Promise => + (JSON.parse(await readFile(join(dir, 'setup.json'), 'utf8')) as { providerEnablementOverrides: unknown }) + .providerEnablementOverrides + +// A reset clears detection first, then writes. When the re-probe failed after +// the write had landed, the reset rejected: the row said "Nothing was +// changed" about an override already removed on disk. +it('resolves a reset whose write landed even when re-detection then fails', async () => { + const enablement = await import('./providerEnablement.js') + await enablement.setProviderEnabled('codex', false) + expect(await overridesOnDisk()).toEqual({ codex: false }) + + probe.checkPrerequisites.mockRejectedValue(new Error('login shell timed out')) + const snapshot = await enablement.resetProviderEnablement('codex') + expect(await overridesOnDisk()).toEqual({}) + // Shown against the last detection that succeeded: codex was installed. + expect(snapshot.entries.find(entry => entry.kind === 'codex')).toMatchObject({ + enabled: true, + because: 'detected', + }) + + // A failed probe must not stay "in flight" for the process lifetime: the + // next resolve probes again and sees the real answer. + probe.checkPrerequisites.mockResolvedValue({ usableProviders: ['claude'] }) + await enablement.resetProviderEnablement('codex') + const after = await enablement.getProviderEnablementSnapshot() + expect(after.entries.find(entry => entry.kind === 'codex')).toMatchObject({ enabled: false, installed: false }) +}) + +// The rejection path is kept for what it means: nothing was saved. +it('rejects a toggle whose write fails, and changes nothing', async () => { + const enablement = await import('./providerEnablement.js') + await enablement.setProviderEnabled('codex', true) + await rm(join(dir, 'setup.json')) + await mkdir(join(dir, 'setup.json')) + await expect(enablement.setProviderEnabled('codex', false)).rejects.toThrow() + const snapshot = await enablement.getProviderEnablementSnapshot() + expect(snapshot.entries.find(entry => entry.kind === 'codex')).toMatchObject({ enabled: true, because: 'user' }) +}) + +// #1403 verification a (survivor): the fallback is the last detection that +// SUCCEEDED, not "everything installed". Only Claude was detected here, so a +// reset Codex whose re-probe fails is shown as not installed, and so off. +it('shows a reset against the last good detection, not against everything', async () => { + probe.checkPrerequisites.mockResolvedValue({ usableProviders: ['claude'] }) + const enablement = await import('./providerEnablement.js') + await enablement.setProviderEnabled('codex', true) + probe.checkPrerequisites.mockRejectedValue(new Error('login shell timed out')) + const snapshot = await enablement.resetProviderEnablement('codex') + expect(snapshot.entries.find(entry => entry.kind === 'codex')).toMatchObject({ enabled: false, installed: false }) +}) + +// #1403 verification a: two rows write at once. The first row's refresh +// pauses in the credential probe; the second row's refresh finishes first. +// The first must not then overwrite, and broadcast, a snapshot read before +// the second write: the disk says Claude is off, so every reader must too. +it('never lets an older refresh overwrite a newer one', async () => { + const enablement = await import('./providerEnablement.js') + await enablement.getProviderEnablementSnapshot() + let releaseFirst!: () => void + zai.probeZaiCredential.mockImplementationOnce(() => new Promise(resolve => { releaseFirst = () => resolve(false) })) + const broadcasts: boolean[] = [] + enablement.onProviderEnablementChanged(snapshot => { + broadcasts.push(snapshot.entries.find(entry => entry.kind === 'claude')!.enabled) + }) + const first = enablement.setProviderEnabled('codex', false) + await vi.waitFor(() => expect(releaseFirst).toBeTypeOf('function')) + await enablement.setProviderEnabled('claude', false) + releaseFirst() + await first + expect(await overridesOnDisk()).toEqual({ codex: false, claude: false }) + expect(enablement.getCachedProviderEnablement()!.entries.find(entry => entry.kind === 'claude')!.enabled).toBe(false) + // The last broadcast is what subscribers keep. + expect(broadcasts.at(-1)).toBe(false) +}) + +// #1403 recheck b: two toggles in flight. The first saves and refreshes while +// the second's write is still open; that write then fails. The refresh read +// the optimistic state, which already held the second toggle, so it broadcast +// Claude as off while the second row said "Nothing was changed" and the disk +// never had it. Published snapshots come from what is on disk. +it('never publishes a toggle whose own write then fails', async () => { + const enablement = await import('./providerEnablement.js') + await enablement.getProviderEnablementSnapshot() + let failSecond!: () => void + const secondHeld = new Promise((_resolve, reject) => { + failSecond = () => reject(Object.assign(new Error('ENOSPC: no space left on device'), { code: 'ENOSPC' })) + }) + // Handled here too: it may reject before the held rename awaits it. + secondHeld.catch(() => {}) + renames.hooks = [null, () => secondHeld] + const first = enablement.setProviderEnabled('codex', false) + const second = enablement.setProviderEnabled('claude', false) + const snapshot = await first + failSecond() + await expect(second).rejects.toThrow('ENOSPC') + expect(await overridesOnDisk()).toEqual({ codex: false }) + expect(snapshot.entries.find(entry => entry.kind === 'claude')!.enabled).toBe(true) + expect(enablement.getCachedProviderEnablement()!.entries.find(entry => entry.kind === 'claude')!.enabled).toBe(true) +}) + +// #1403 recheck a: the same leak through an overlapping READ. A `get` (or a +// usage refresh) while a toggle's write is pending must not publish that +// toggle; the write then fails and the disk never had it. +it('never publishes a pending toggle through an overlapping read', async () => { + const enablement = await import('./providerEnablement.js') + await enablement.getProviderEnablementSnapshot() + let failWrite!: () => void + const held = new Promise((_resolve, reject) => { + failWrite = () => reject(Object.assign(new Error('ENOSPC: no space left on device'), { code: 'ENOSPC' })) + }) + held.catch(() => {}) + renames.hooks = [() => held] + const toggle = enablement.setProviderEnabled('claude', false) + const read = enablement.getProviderEnablementSnapshot() + const snapshot = await read + failWrite() + await expect(toggle).rejects.toThrow('ENOSPC') + expect(snapshot.entries.find(entry => entry.kind === 'claude')!.enabled).toBe(true) + expect(enablement.getCachedProviderEnablement()!.entries.find(entry => entry.kind === 'claude')!.enabled).toBe(true) +}) diff --git a/src/main/setup/providerEnablement.ts b/src/main/setup/providerEnablement.ts index b2d8e24f5..afcaaa3dd 100644 --- a/src/main/setup/providerEnablement.ts +++ b/src/main/setup/providerEnablement.ts @@ -13,9 +13,9 @@ import { import { checkPrerequisites } from '@main/setup/prerequisites.js' import { invalidateUsageSnapshotCache } from '@main/usage/usageService.js' import { - loadSetupState, + loadDurableSetupState, setOpencodeUsageSource as persistOpencodeUsageSource, - setProviderEnablementOverrides, + setProviderEnablementOverride, } from '@main/setup/setupState.js' type Listener = (snapshot: ProviderEnablementSnapshot) => void @@ -27,32 +27,65 @@ const listeners = new Set() let cachedDetected: ReadonlySet | null = null let inFlightDetection: Promise> | null = null let cachedSnapshot: ProviderEnablementSnapshot | null = null +// The last detection that succeeded, kept across a reset (which clears +// `cachedDetected` to force a fresh probe). It is what a saved change is +// shown against when the fresh probe fails; see `mutate`. +let lastDetected: ReadonlySet | null = null function detectInstalledKinds(): Promise> { if (cachedDetected) return Promise.resolve(cachedDetected) if (inFlightDetection) return inFlightDetection - inFlightDetection = checkPrerequisites().then(result => { - // usableProviders is the exact resolution the first-run SetupGate uses - // (manual override → PATH probe → bundled archive), so Settings → - // Providers and the gate can never disagree about "installed". - cachedDetected = new Set( - (result.usableProviders ?? []).filter(kind => AGENT_PROVIDER_KINDS.includes(kind)), - ) - inFlightDetection = null - return cachedDetected - }) + inFlightDetection = checkPrerequisites() + .then(result => { + // usableProviders is the exact resolution the first-run SetupGate uses + // (manual override → PATH probe → bundled archive), so Settings → + // Providers and the gate can never disagree about "installed". + cachedDetected = new Set( + (result.usableProviders ?? []).filter(kind => AGENT_PROVIDER_KINDS.includes(kind)), + ) + lastDetected = cachedDetected + return cachedDetected + }) + // WHY in `finally` (#1403 review): cleared only on success, a rejected + // probe stayed in flight forever, so every later resolve returned the + // same rejection until restart. + .finally(() => { + inFlightDetection = null + }) return inFlightDetection } -async function resolveAndCache(): Promise { - const state = await loadSetupState() - const detected = await detectInstalledKinds() - cachedSnapshot = { +// WHY refreshes are ordered (#1403 verification a): a refresh reads setup +// state, then awaits detection and the credential probe. Two rows can write +// at once (each disables only its own switch), so an OLDER refresh could +// finish last and overwrite, and broadcast, a snapshot built before the newer +// write: the user disables Claude, disk says disabled, and pickers show it +// enabled until some later refresh. Only a refresh started after the one +// already applied may replace it; a stale one answers with the newer snapshot. +let refreshesStarted = 0 +let appliedRefresh = 0 + +async function resolveAndCache( + detection: () => Promise> = detectInstalledKinds, +): Promise { + const refresh = ++refreshesStarted + // DURABLE state, not the optimistic cache (#1403 recheck b): two toggles in + // flight, the first saves and refreshes while the second is still pending, + // and the cache already holds the second. The second then failed to write, + // and its row said "Nothing was changed" while this refresh had broadcast + // it. Every refresh here runs after its own write landed, so the durable + // state already includes everything it needs to show. + const state = await loadDurableSetupState() + const detected = await detection() + const next: ProviderEnablementSnapshot = { entries: resolveProviderEnablement(state.providerEnablementOverrides, detected), opencodeUsageSource: state.opencodeUsageSource, zaiCredentialPresent: await probeZaiCredential(), } - return cachedSnapshot + if (refresh < appliedRefresh && cachedSnapshot) return cachedSnapshot + appliedRefresh = refresh + cachedSnapshot = next + return next } /** Fail-open before the first resolve: hiding a user's providers because a @@ -77,13 +110,28 @@ function emit(snapshot: ProviderEnablementSnapshot): void { } async function mutate(action: () => Promise): Promise { + // A rejection here is a change that was NOT saved; the renderer says so. await action() + // WHY a refresh failure no longer rejects (#1403 review a and b): once the + // write landed, the change IS saved and takes effect. Rejecting made the row + // say "Nothing was changed" about a change that was on disk (a reset clears + // detection first, so a failing re-probe hit this directly). Show the saved + // state against the last detection that succeeded, or fail open to "all + // installed" before any has, the same fail-open as + // `enabledAgentProviderKindsSync`. The next resolve probes again. + let snapshot: ProviderEnablementSnapshot + try { + snapshot = await resolveAndCache() + } catch (error) { + console.warn('[provider-enablement] saved, but re-detecting providers failed:', error) + const fallback = lastDetected ?? new Set(AGENT_PROVIDER_KINDS) + snapshot = await resolveAndCache(async () => fallback) + } // Resolve the NEW enablement BEFORE invalidating the usage cache (review // finding #4): the old order invalidated first, so a usage fetch landing // in that window recomposed from the PREVIOUS snapshot and re-cached the // just-disabled provider. Generation-guarded cache writes cover the fetch // already in flight; this closes the window for the next one. - const snapshot = await resolveAndCache() invalidateUsageSnapshotCache() emit(snapshot) return snapshot @@ -93,22 +141,17 @@ export async function setProviderEnabled( kind: AgentProviderKind, enabled: boolean, ): Promise { - const state = await loadSetupState() - const overrides = { ...state.providerEnablementOverrides, [kind]: enabled } - return await mutate(() => setProviderEnablementOverrides(overrides)) + return await mutate(() => setProviderEnablementOverride(kind, enabled)) } export async function resetProviderEnablement( kind: AgentProviderKind, ): Promise { - const state = await loadSetupState() - const overrides = { ...state.providerEnablementOverrides } - delete overrides[kind] // Reset must also redo detection: "installed" is the display hint on the // settings row, and a stale cached detection would keep showing the state // from before any install that happened while the app was closed. cachedDetected = null - return await mutate(() => setProviderEnablementOverrides(overrides)) + return await mutate(() => setProviderEnablementOverride(kind, null)) } export async function setOpencodeUsage( diff --git a/src/main/setup/setupState.test.ts b/src/main/setup/setupState.test.ts new file mode 100644 index 000000000..3c3a989fb --- /dev/null +++ b/src/main/setup/setupState.test.ts @@ -0,0 +1,187 @@ +import { mkdir, mkdtemp, readFile, rm } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { join } from 'node:path' + +import { afterEach, beforeEach, expect, it, vi } from 'vitest' + +// #1250 rows 6 and 13: a setup-state write that fails must leave main's +// in-memory state as it was. `saveSetupState` used to assign the new state to +// its cache before writing and never restore it, so after a failed write the +// app behaved as if the change were saved (provider enablement re-resolved +// from it, the next save persisted it) and then reverted on the next launch. +// +// Real filesystem in a scratch STATE_DIR. The failure is real too: a directory +// sits where `setup.json` goes, so the temp file's rename onto it fails. + +const paths = vi.hoisted(() => ({ STATE_DIR: '' })) +vi.mock('@main/storage/paths.js', () => paths) + +// The real rename, with an optional one-shot failure. The directory trick +// above fails EVERY write until it is removed, and removing it between two +// queued writes races the queue; the ordered cases below need "this write +// fails, the next one lands" exactly. +const renameFaults = vi.hoisted(() => ({ failNext: 0 })) +// Holds the NEXT read of setup.json until released: the first-load race. +const readGate = vi.hoisted(() => ({ hold: null as Promise | null })) +vi.mock('fs/promises', async importOriginal => { + const actual = await importOriginal() + return { + ...actual, + readFile: (async (...args: Parameters) => { + const hold = readGate.hold + readGate.hold = null + const result = actual.readFile(...args) + if (hold) await hold + return await result + }) as typeof actual.readFile, + rename: async (from: string, to: string) => { + if (renameFaults.failNext > 0) { + renameFaults.failNext -= 1 + throw Object.assign(new Error('ENOSPC: no space left on device'), { code: 'ENOSPC' }) + } + return await actual.rename(from, to) + }, + } +}) + +let dir: string +beforeEach(async () => { + dir = await mkdtemp(join(tmpdir(), 'setup-state-')) + paths.STATE_DIR = dir + renameFaults.failNext = 0 + readGate.hold = null + vi.resetModules() +}) +afterEach(async () => { + await rm(dir, { recursive: true, force: true }) +}) + +it('restores the previous state when the write fails, and a later good save still lands', async () => { + const setup = await import('./setupState.js') + await setup.setCliUpdateBehavior('notify') + expect((await setup.loadSetupState()).cliUpdateBehavior).toBe('notify') + + // Break the next write: a directory where the file must be renamed to. + await rm(join(dir, 'setup.json')) + await mkdir(join(dir, 'setup.json')) + await expect(setup.setCliUpdateBehavior('off')).rejects.toThrow() + // Not "off": nothing was written, so nothing may act as if it had been. + expect((await setup.loadSetupState()).cliUpdateBehavior).toBe('notify') + + await rm(join(dir, 'setup.json'), { recursive: true }) + await setup.setCliUpdateBehavior('automatic') + expect(JSON.parse(await readFile(join(dir, 'setup.json'), 'utf8')).cliUpdateBehavior).toBe('automatic') +}) + +it('restores provider-enablement overrides after a failed write', async () => { + const setup = await import('./setupState.js') + await setup.setProviderEnablementOverrides({ codex: true }) + await rm(join(dir, 'setup.json')) + await mkdir(join(dir, 'setup.json')) + await expect(setup.setProviderEnablementOverrides({ codex: false })).rejects.toThrow() + expect((await setup.loadSetupState()).providerEnablementOverrides).toEqual({ codex: true }) +}) + +const onDisk = async (): Promise> => + JSON.parse(await readFile(join(dir, 'setup.json'), 'utf8')) as Record + +// #1403 review a and c: two overlapping saves that both fail. The second used +// to "restore" the first one's unwritten state, so main kept acting on a +// value no write ever landed. +it('keeps the durable state when two queued saves both fail', async () => { + const setup = await import('./setupState.js') + await setup.setCliUpdateBehavior('notify') + renameFaults.failNext = 2 + const results = await Promise.allSettled([ + setup.setCliUpdateBehavior('off'), + setup.setOpencodeUsageSource('zai'), + ]) + expect(results.map(result => result.status)).toEqual(['rejected', 'rejected']) + const state = await setup.loadSetupState() + expect(state.cliUpdateBehavior).toBe('notify') + expect(state.opencodeUsageSource).toBe('none') +}) + +// #1403 review a and b: a failed save followed by a good one. The good one's +// snapshot was built from the failed one's unwritten state and carried it to +// disk, while the renderer had just said "Nothing was changed". +it('never lets a later save carry a failed one to disk', async () => { + const setup = await import('./setupState.js') + await setup.setCliUpdateBehavior('notify') + renameFaults.failNext = 1 + const first = setup.setCliUpdateBehavior('off') + // The reviewers' sequence: the second save starts AFTER the first one's + // change is visible (a few microtasks; its write is still on real I/O). + for (let i = 0; i < 5; i += 1) await Promise.resolve() + expect((await setup.loadSetupState()).cliUpdateBehavior).toBe('off') + const second = setup.setOpencodeUsageSource('zai') + const results = await Promise.allSettled([first, second]) + expect(results.map(result => result.status)).toEqual(['rejected', 'fulfilled']) + expect(await onDisk()).toMatchObject({ cliUpdateBehavior: 'notify', opencodeUsageSource: 'zai' }) + expect(await setup.loadSetupState()).toMatchObject({ cliUpdateBehavior: 'notify', opencodeUsageSource: 'zai' }) +}) + +// The other order: a good save still pending when a later one fails keeps +// its value, in memory and on disk. (The first fix's `cache === snapshot` +// guard existed for this case and no test exercised it; review c.) +it('keeps an earlier good save when a later queued save fails', async () => { + const setup = await import('./setupState.js') + await setup.setCliUpdateBehavior('notify') + const first = setup.setCliUpdateBehavior('off') + const second = setup.setOpencodeUsageSource('zai') + // Readers see both at once, before either write settles. + expect(await setup.loadSetupState()).toMatchObject({ cliUpdateBehavior: 'off', opencodeUsageSource: 'zai' }) + await first + renameFaults.failNext = 1 + await expect(second).rejects.toThrow('ENOSPC') + expect(await onDisk()).toMatchObject({ cliUpdateBehavior: 'off', opencodeUsageSource: 'none' }) + expect(await setup.loadSetupState()).toMatchObject({ cliUpdateBehavior: 'off', opencodeUsageSource: 'none' }) +}) + +// Per-provider overrides apply to the durable map: a failed toggle cannot +// ride along with a later toggle of another provider. +it('does not persist a failed provider toggle with a later one', async () => { + const setup = await import('./setupState.js') + renameFaults.failNext = 1 + const results = await Promise.allSettled([ + setup.setProviderEnablementOverride('codex', false), + setup.setProviderEnablementOverride('claude', false), + ]) + expect(results.map(result => result.status)).toEqual(['rejected', 'fulfilled']) + expect((await onDisk()).providerEnablementOverrides).toEqual({ claude: false }) + expect((await setup.loadSetupState()).providerEnablementOverrides).toEqual({ claude: false }) +}) + +// #1403 verification b (survivor): two first loads share ONE read. A second +// read that started later but finished after a save's write would reset the +// durable baseline to the file as it was before that write, and the next +// save would then drop the saved change. +it('shares the first read, so a slow read cannot undo a save made meanwhile', async () => { + const seed = await import('./setupState.js') + await seed.setCliUpdateBehavior('notify') + vi.resetModules() + const setup = await import('./setupState.js') + let release!: () => void + readGate.hold = new Promise(resolve => { release = resolve }) + // The first read reads the file NOW ('notify') and is then held. + const slowLoad = setup.loadSetupState() + // A save started meanwhile. With a second read of its own it would land + // while the first read is still held; sharing the read, it waits for it. + const save = setup.setCliUpdateBehavior('off') + await Promise.race([save, new Promise(resolve => setTimeout(resolve, 300))]) + // Only now does the first read finish, with the contents it read before. + release() + await Promise.all([slowLoad, save]) + await setup.setOpencodeUsageSource('zai') + expect(await onDisk()).toMatchObject({ cliUpdateBehavior: 'off', opencodeUsageSource: 'zai' }) +}) + +// #1403 verification c (suspicion): an update that throws is not left +// pending, where the cache would show it forever. +it('drops an update that throws, leaving the state as it was', async () => { + const setup = await import('./setupState.js') + await setup.setCliUpdateBehavior('notify') + await expect(setup.updateSetupState(() => { throw new Error('bad update') })).rejects.toThrow('bad update') + await setup.setOpencodeUsageSource('zai') + expect(await setup.loadSetupState()).toMatchObject({ cliUpdateBehavior: 'notify', opencodeUsageSource: 'zai' }) +}) diff --git a/src/main/setup/setupState.ts b/src/main/setup/setupState.ts index 0123e296c..563e1cb48 100644 --- a/src/main/setup/setupState.ts +++ b/src/main/setup/setupState.ts @@ -97,15 +97,55 @@ const DEFAULT_SETUP_STATE: PersistedSetupState = { updatedAt: 0, } +// WHY saves are UPDATES applied to the last DURABLE state (#1250 rows 6 and +// 13, #1403 review round 1): the first fix kept a whole next-state per save +// and restored the previous cache on failure. Reviewers a, b and c showed +// that cannot be made true, because every save was BUILT from the optimistic +// cache: +// - two queued saves that both fail: the second "restored" the first one's +// unwritten state, so main acted on a value no write ever landed; +// - a failing save followed by a good one: the good one's snapshot carried +// the failed value to disk, while the renderer had just said "Nothing was +// changed". +// So a save is now a function of the state it applies to. At write time it is +// applied to `durable` (what is known to be on disk), never to another save's +// unwritten result, and a failed update simply drops out. Readers still see +// every change at once: `cache` is `durable` with the still-pending updates +// applied in order, recomputed whenever one settles. +// +// Invariant: `cache` === fold(pending, durable) after every settle, and +// nothing written to disk ever contains an update whose own write failed. +export type SetupStateUpdate = (state: PersistedSetupState) => PersistedSetupState + +let durable: PersistedSetupState | null = null let cache: PersistedSetupState | null = null +const pending: SetupStateUpdate[] = [] let writeQueue: Promise = Promise.resolve() +function recomputeCache(): PersistedSetupState { + const next = pending.reduce((state, update) => update(state), durable ?? DEFAULT_SETUP_STATE) + cache = next + return next +} + +// One first read, shared. Two concurrent first loads used to read the file +// twice; with a durable baseline that matters, because a read that finishes +// AFTER a save's write would reset `durable` to the pre-write file. +let loading: Promise | null = null + export async function loadSetupState(): Promise { if (cache) return cache + loading ??= readDurable() + await loading + // Folds in any update queued while the first read was in flight. + return recomputeCache() +} + +async function readDurable(): Promise { try { const raw = await readFile(SETUP_STATE_FILE, 'utf8') const parsed = JSON.parse(raw) as Partial - cache = { + durable = { version: 1, toolPaths: parsed.toolPaths ?? {}, manualToolPaths: parsed.manualToolPaths ?? {}, @@ -129,54 +169,105 @@ export async function loadSetupState(): Promise { updatedAt: typeof parsed.updatedAt === 'number' ? parsed.updatedAt : 0, } } catch { - cache = DEFAULT_SETUP_STATE + durable = DEFAULT_SETUP_STATE } - return cache + recomputeCache() } -export async function saveSetupState( - next: PersistedSetupState, -): Promise { - cache = { ...next, version: 1, updatedAt: Date.now() } - const snapshot = cache - writeQueue = writeQueue +/** The state as last written to disk: no pending update included. For a + * reader that PUBLISHES state (#1403 recheck b): provider enablement is + * broadcast to every picker, and a snapshot folded from another save still + * in flight would publish that save even if its write then failed. */ +export async function loadDurableSetupState(): Promise { + await loadSetupState() + return durable ?? DEFAULT_SETUP_STATE +} + +/** Apply `update` and persist the result. Rejects when the write fails, and + * then the update is gone: neither the cache nor any later write carries it. */ +export async function updateSetupState(update: SetupStateUpdate): Promise { + // Only the very first save awaits the read. Once loaded, the update is + // visible to readers synchronously, as the old whole-state assignment was. + if (!cache) await loadSetupState() + pending.push(update) + // An update that throws must never stay pending (#1403 verification c): + // the cache would show forever a change no write could apply. + try { + recomputeCache() + } catch (error) { + pending.splice(pending.indexOf(update), 1) + recomputeCache() + throw error + } + const write = writeQueue .catch(() => {}) .then(async () => { - await mkdir(STATE_DIR, { recursive: true }) - // WHY setup state uses the same temp+rename discipline as workspace - // state even though the single-process lock should prevent concurrent - // app mains: - // - // Setup paths are user-visible configuration. A failed write should not - // leave `setup.json` truncated and force the user through tool discovery - // again. Temp+rename gives atomic visibility to readers; it is not a full - // fsync durability protocol for power-loss recovery, which would be a - // separate requirement. - const tmp = `${SETUP_STATE_FILE}.${process.pid}.${Date.now()}.${Math.random() - .toString(36) - .slice(2)}.tmp` + // Settle bookkeeping INSIDE the queue step, so the next queued update + // is applied to this one's outcome and never to its unwritten result. + const settle = (): void => { + pending.splice(pending.indexOf(update), 1) + recomputeCache() + } + let snapshot: PersistedSetupState try { - await writeFile(tmp, JSON.stringify(snapshot, null, 2), 'utf8') - await rename(tmp, SETUP_STATE_FILE) + // Inside the try, so a throwing update settles like a failed write. + snapshot = { + ...update(durable ?? DEFAULT_SETUP_STATE), + version: 1, + updatedAt: Date.now(), + } + await mkdir(STATE_DIR, { recursive: true }) + // WHY setup state uses the same temp+rename discipline as workspace + // state even though the single-process lock should prevent concurrent + // app mains: + // + // Setup paths are user-visible configuration. A failed write should not + // leave `setup.json` truncated and force the user through tool discovery + // again. Temp+rename gives atomic visibility to readers; it is not a full + // fsync durability protocol for power-loss recovery, which would be a + // separate requirement. + const tmp = `${SETUP_STATE_FILE}.${process.pid}.${Date.now()}.${Math.random() + .toString(36) + .slice(2)}.tmp` + try { + await writeFile(tmp, JSON.stringify(snapshot, null, 2), 'utf8') + await rename(tmp, SETUP_STATE_FILE) + } catch (err) { + await rm(tmp, { force: true }).catch(() => undefined) + throw err + } } catch (err) { - await rm(tmp, { force: true }).catch(() => undefined) + settle() throw err } + durable = snapshot + settle() }) - await writeQueue - return cache + writeQueue = write + await write + return recomputeCache() +} + +/** Whole-state replacement. Kept for callers that already hold a complete + * state; prefer `updateSetupState` so the change is applied to what is on + * disk rather than to a state read before other saves settled. */ +export async function saveSetupState( + next: PersistedSetupState, +): Promise { + return await updateSetupState(() => next) } export async function updateToolPaths( paths: Partial>, ): Promise { - const state = await loadSetupState() - const toolPaths = { ...state.toolPaths } - for (const [tool, path] of Object.entries(paths) as Array<[SetupToolId, string | null]>) { - if (path) toolPaths[tool] = path - else delete toolPaths[tool] - } - return await saveSetupState({ ...state, toolPaths }) + return await updateSetupState(state => { + const toolPaths = { ...state.toolPaths } + for (const [tool, path] of Object.entries(paths) as Array<[SetupToolId, string | null]>) { + if (path) toolPaths[tool] = path + else delete toolPaths[tool] + } + return { ...state, toolPaths } + }) } // Records a user-supplied override from setup:set-tool-path. Writes BOTH @@ -191,32 +282,29 @@ export async function setManualToolPath( tool: SetupToolId, path: string, ): Promise { - const state = await loadSetupState() - return await saveSetupState({ + return await updateSetupState(state => ({ ...state, manualToolPaths: { ...state.manualToolPaths, [tool]: path }, toolPaths: { ...state.toolPaths, [tool]: path }, - }) + })) } export async function markOptionalSkipped( tool: SetupToolId, skipped: boolean, ): Promise { - const state = await loadSetupState() - return await saveSetupState({ + return await updateSetupState(state => ({ ...state, skippedOptionalTools: { ...state.skippedOptionalTools, [tool]: skipped, }, - }) + })) } /** Records that the user chose to continue with no provider installed. */ export async function markNoProvidersAcknowledged(): Promise { - const state = await loadSetupState() - return await saveSetupState({ ...state, acknowledgedNoProviders: true }) + return await updateSetupState(state => ({ ...state, acknowledgedNoProviders: true })) } /** Persist the user's CLI auto-update preference. Written by the setting @@ -225,26 +313,41 @@ export async function markNoProvidersAcknowledged(): Promise { - const state = await loadSetupState() - return await saveSetupState({ ...state, cliUpdateBehavior: behavior }) + return await updateSetupState(state => ({ ...state, cliUpdateBehavior: behavior })) } -/** Replace the provider-enablement override map (#1102). Whole-map write: - * the caller (main's providerEnablement module) computed the next map from - * the state it just loaded, so partial merges here would only re-race it. */ +/** Replace the provider-enablement override map (#1102). Whole-map write, + * for callers that own the entire map; per-provider changes go through + * `setProviderEnablementOverride`. */ export async function setProviderEnablementOverrides( overrides: UserProviderOverrides, ): Promise { - const state = await loadSetupState() - return await saveSetupState({ ...state, providerEnablementOverrides: overrides }) + return await updateSetupState(state => ({ ...state, providerEnablementOverrides: overrides })) +} + +/** Set (or with `null`, clear) ONE provider's enablement override. WHY + * per key (#1403 review): the settings row used to compute the whole next + * map from the optimistic cache, so a map built while an earlier toggle's + * write was still pending carried that toggle to disk even when its own + * write failed. Applied at write time to the durable map, a failed toggle + * cannot ride along with a later one. */ +export async function setProviderEnablementOverride( + kind: keyof UserProviderOverrides, + enabled: boolean | null, +): Promise { + return await updateSetupState(state => { + const overrides = { ...state.providerEnablementOverrides } + if (enabled === null) delete overrides[kind] + else overrides[kind] = enabled + return { ...state, providerEnablementOverrides: overrides } + }) } /** Persist the selected OpenCode usage source (#1102/#1104). */ export async function setOpencodeUsageSource( opencodeUsageSource: OpencodeUsageSource, ): Promise { - const state = await loadSetupState() - return await saveSetupState({ ...state, opencodeUsageSource }) + return await updateSetupState(state => ({ ...state, opencodeUsageSource })) } /** Persist a successful latest-version probe. Called after every non-error @@ -256,9 +359,8 @@ export async function updateCliUpdateCache( cli: CliUpdateKind, entry: CliUpdateCacheEntry, ): Promise { - const state = await loadSetupState() - return await saveSetupState({ + return await updateSetupState(state => ({ ...state, cliUpdateCache: { ...state.cliUpdateCache, [cli]: entry }, - }) + })) } diff --git a/src/main/setup/toolchain.ts b/src/main/setup/toolchain.ts index 2112765fb..057744eec 100644 --- a/src/main/setup/toolchain.ts +++ b/src/main/setup/toolchain.ts @@ -65,9 +65,23 @@ export async function initializeToolchain(): Promise { }) } -export async function refreshToolchainFromState(): Promise { +/** + * `unsaved`: paths a prerequisite check just probed but could not persist + * (#1403 recheck a). They are applied over the persisted ones for THIS + * process, so the check's "found at X" and the path a launch then uses are + * the same X. Without them, a failed write-back left the toolchain on the + * last persisted path, possibly a dead one, while the setup panel said the + * tool was found somewhere else. `null` clears, as in `updateToolPaths`. + */ +export async function refreshToolchainFromState( + unsaved?: Partial>, +): Promise { const state = await loadSetupState() cachedPaths = { ...state.toolPaths } + for (const [tool, path] of Object.entries(unsaved ?? {}) as Array<[SetupToolId, string | null]>) { + if (path) cachedPaths[tool] = path + else delete cachedPaths[tool] + } await refreshBundledOverrides(state) applyToolEnv() } diff --git a/src/main/workflows/control.system.test.ts b/src/main/workflows/control.system.test.ts index 6f6f857a7..e69bdf3e2 100644 --- a/src/main/workflows/control.system.test.ts +++ b/src/main/workflows/control.system.test.ts @@ -1,3 +1,4 @@ +import { existsSync } from 'node:fs' import { mkdtemp, mkdir, copyFile, rm } from 'node:fs/promises' import { tmpdir } from 'node:os' import { join } from 'node:path' @@ -10,9 +11,18 @@ import { taskHistoryCapabilities } from '@main/control/history/tasks' import { workflowControlCapabilities } from './control' // This starts the real isolated workflow worker, so it belongs to the system // tier even though the workflow fixture never launches a provider agent. +// +// WHY a precondition (#1107): the worker is forked from the BUILT package +// (packages/workflow-mcp/dist/workflowWorker.js; see workerFilePath in runWorkflow.ts). A fresh git +// worktree has no dist, the package throws `worker-missing` inside the run, and all this test used +// to see was an operation that never completed — a 5 s waitFor timeout that looked like load. It +// now fails at once and says why. It does not build the package itself: a build inside one test is +// slow and races parallel workers; CI builds it before the suite. +const BUILT_WORKER = 'packages/workflow-mcp/dist/workflowWorker.js' const directories: string[] = [], services: WorkflowService[] = [] afterEach(async () => { await Promise.all(services.splice(0).map(service => service.quiesce())); await Promise.all(directories.splice(0).map(path => rm(path, { recursive: true, force: true }))) }) it('admits a main-host task before source approval and persists external ownership through a real existing workflow run', async () => { + expect(existsSync(BUILT_WORKER), `${BUILT_WORKER} is missing: run \`npm run build\` in packages/workflow-mcp (this test forks the built worker)`).toBe(true) const cwd = await mkdtemp(join(tmpdir(), 'ac-workflow-operator-')); directories.push(cwd) await mkdir(join(cwd, '.claude/workflows'), { recursive: true }) await copyFile('packages/workflow-mcp/test/fixtures/workflow-corpus/minimal.js', join(cwd, '.claude/workflows/minimal.js')) diff --git a/src/providers/codex/renderer/compaction.committed.evidence.renderer.test.tsx b/src/providers/codex/renderer/compaction.committed.evidence.renderer.test.tsx new file mode 100644 index 000000000..973562548 --- /dev/null +++ b/src/providers/codex/renderer/compaction.committed.evidence.renderer.test.tsx @@ -0,0 +1,74 @@ +import { render } from '@testing-library/react' +import { describe, expect, it } from 'vitest' + +import fixture from '../../../../testing/fixtures/rendering-shapes/codex/compaction/committed-compacted.json' + +import { renderCodexDurableEntry } from '@providers/codex/renderer/entries/dispatch' +import { CODEX_RENDER_SHAPES } from '@providers/codex/renderer/shapes' +import { mapCodexRolloutToFeedEntries } from '@providers/codex/renderer/transcript/rollout' +import { buildFingerprintIndex, classifySighting } from '@renderer/rendering/evidence/catalogCoverage' +import { fingerprintRenderShape } from '@renderer/rendering/evidence/shapeFingerprint' +import type { Entry } from '@shared/types/transcript' + +// #1289: a Codex `compacted` rollout line has no payload.type, so the mapper's +// early return swallowed it and Codex compaction never rendered. The fixture is +// two real lines (see its `evidence`): a 0.157.0 one with an empty message and +// an encrypted `compaction` item, and an older one with a readable summary. +const [modern, legacy] = fixture.records as Array & { timestamp: string }> +const catalogIndex = buildFingerprintIndex([CODEX_RENDER_SHAPES]) + +const kinds = (entries: Entry[]) => entries.map(entry => + (entry as { subtype?: string }).subtype ?? ((entry as { isCompactSummary?: boolean }).isCompactSummary ? 'compact_summary' : entry.type)) + +describe('Codex committed compaction (#1289)', () => { + it('keeps the curated carrier identical to what the mapper produces', () => { + // The catalog coverage gate sweeps `cases`; pinning them to the mapper's + // output means the gate and these tests read the same shapes. + expect(fixture.cases).toEqual(fixture.records.flatMap(record => + mapCodexRolloutToFeedEntries(record as Record).map(transcriptEntry => ({ transcriptEntry })))) + }) + + it('maps a 0.157 compaction to one timestamped boundary and nothing else', () => { + const entries = mapCodexRolloutToFeedEntries(modern!) + // No summary (the message is empty) and no replay of replacement_history, + // which only restates context and prompts already in the feed. + expect(kinds(entries)).toEqual(['compact_boundary']) + // A timestamp-less row sorts to the bottom of the feed (order.ts). + expect(entries[0]!.timestamp).toBe(modern!.timestamp) + }) + + it('carries none of the retained history on the boundary', () => { + // The boundary used to embed the whole payload as compactMetadata: + // retained prompts, instructions and a 13–23 KB encrypted summary. + const [boundary] = mapCodexRolloutToFeedEntries(modern!) + expect(boundary).not.toHaveProperty('compactMetadata') + expect(JSON.stringify(boundary).length).toBeLessThan(300) + }) + + it('maps an older compaction with a readable message to boundary then summary', () => { + const entries = mapCodexRolloutToFeedEntries(legacy!) + expect(kinds(entries)).toEqual(['compact_boundary', 'compact_summary']) + for (const entry of entries) expect(entry.timestamp).toBe(legacy!.timestamp) + }) + + it('renders both through shared.compaction, and the catalog claims them', () => { + const entries = [...mapCodexRolloutToFeedEntries(modern!), ...mapCodexRolloutToFeedEntries(legacy!)] + expect(entries).toHaveLength(3) + for (const entry of entries) { + const decision = renderCodexDurableEntry({ entry } as Parameters[0]) + if (decision?.action !== 'render') throw new Error('expected a rendered compaction entry') + const eventType = entry.type === 'system' ? `system:${(entry as { subtype: string }).subtype}` : entry.type + const fingerprint = fingerprintRenderShape({ provider: 'codex', plane: 'transcript-entry', eventType, payload: entry }).fingerprint + const definition = catalogIndex.byFingerprint.get(fingerprint) + expect({ eventType, claimed: definition?.id }).toEqual({ eventType, claimed: expect.stringMatching(/^codex\.entry\./) }) + expect(classifySighting({ + structuralFingerprint: fingerprint, + lifecycle: 'durable', + outcome: { kind: 'specialized', shapeId: definition!.id, rendererId: decision.receipt.rendererId, protocolId: decision.receipt.protocolId }, + } as Parameters[0], catalogIndex)).toEqual({ kind: 'known-claimed', shapeId: definition!.id }) + const view = render(decision.node) + expect(view.container.textContent?.length).toBeGreaterThan(0) + view.unmount() + } + }) +}) diff --git a/src/providers/codex/renderer/shapes.ts b/src/providers/codex/renderer/shapes.ts index e1d722e15..7424fa4d4 100644 --- a/src/providers/codex/renderer/shapes.ts +++ b/src/providers/codex/renderer/shapes.ts @@ -111,6 +111,43 @@ export const CODEX_RENDER_SHAPES = defineRenderShapeCatalog('codex', { disposition: { kind: 'specialized', rendererId: 'shared.compaction', protocolId: 'compaction.live' }, why: 'GRADUATED Phase 10 from the captured structured Codex semantic item: prefix/final lifecycle paints compaction progress without screen parsing, and the later durable compacted rollout remains the replay source of truth for boundary + summary.', }), + // #1289: the committed compaction a `compacted` rollout line maps to. It + // never rendered before: the mapper returned early on the line's typeless + // payload. Same shared.compaction dispositions as Claude's durable entries. + 'codex.entry.system-compact-boundary.v1': defineRenderShape({ + id: 'codex.entry.system-compact-boundary.v1', + provider: 'codex', + fingerprints: ["fp2-c5b90915"], + eventTypes: ["system:compact_boundary"], + planes: ["transcript-entry"] as const, + lifecycles: ["durable"] as const, + observed: { + providerVersions: ['0.157.0'], + models: [], + firstSeen: '2026-07-13', + lastSeen: '2026-09-25', + }, + fixtures: { final: ["rendering-shapes/codex/compaction/committed-compacted.json"], prefixes: [] }, + disposition: { kind: 'specialized', rendererId: 'shared.compaction', protocolId: 'compaction.boundary' }, + why: 'Codex durable-entry dispatch owns the boundary a committed `compacted` line maps to (timestamped, no retained history) and delegates only the provider-neutral grammar to shared.compaction. The durable rollout line is the replay source of truth.', + }), + 'codex.entry.compact-summary.v1': defineRenderShape({ + id: 'codex.entry.compact-summary.v1', + provider: 'codex', + fingerprints: ["fp2-85ed68b2"], + eventTypes: ["user"], + planes: ["transcript-entry"] as const, + lifecycles: ["durable"] as const, + observed: { + providerVersions: [], + models: [], + firstSeen: '2026-07-13', + lastSeen: '2026-07-13', + }, + fixtures: { final: ["rendering-shapes/codex/compaction/committed-compacted.json"], prefixes: [] }, + disposition: { kind: 'specialized', rendererId: 'shared.compaction', protocolId: 'compaction.summary' }, + why: 'Only older Codex CLIs write a readable compaction `message` (56 of about 1,500 local compactions); 0.15x encrypts the summary. When present it is the committed summary, painted by shared.compaction.', + }), 'codex.semantic.exec.v1': defineRenderShape({ id: 'codex.semantic.exec.v1', provider: 'codex', diff --git a/src/providers/codex/renderer/transcript/rollout.ts b/src/providers/codex/renderer/transcript/rollout.ts index 395ce7458..c175d461b 100644 --- a/src/providers/codex/renderer/transcript/rollout.ts +++ b/src/providers/codex/renderer/transcript/rollout.ts @@ -196,19 +196,61 @@ function codexConversationEntryFromMessageItem( } } +// WHY no `compactMetadata` (#1289): the boundary used to carry the whole +// `compacted` payload, including `replacement_history` (the retained developer +// instructions, AGENTS.md, earlier user prompts, and an encrypted summary of +// 13–23 KB). That kept a second copy of that text in memory and in every debug +// bundle, and nothing in the app reads a Codex boundary's metadata; its uuid +// already identifies the rollout line. A varying metadata object also made +// each boundary a different rendering shape. function codexCompactBoundaryEntry( uuid: string, - payload: Record, + timestamp: string | undefined, ): Entry { return { type: 'system', subtype: 'compact_boundary', content: 'Conversation compacted', uuid, - compactMetadata: payload, + // WHY a timestamp (#1289): rendering/model/order.ts sorts timestamp-less + // rows to the end of their phase, so without it the boundary painted at + // the bottom of the feed instead of where the compaction happened. + timestamp, } } +// A committed Codex compaction: a timestamped boundary, then the summary when +// the CLI wrote a readable one (#1289). +// +// WHY `replacement_history` is not mapped here: in the common case it is not +// new conversation. It is the context Codex keeps across the compaction +// (developer instructions, the AGENTS.md block, earlier user prompts, and from +// 0.155 an encrypted `compaction` summary item). Mapped, it repainted prompts +// already in the feed: 17,326 of 20,343 sampled replacement messages +// duplicated an earlier user message. +// +// KNOWN GAP (review a of #1386, follow-up #1393): it is NOT +// always a duplicate. 82 local rollouts (68 sessions) are resumed files that +// START with a `compacted` line, so its retained user prompts are the only +// copy of that earlier conversation in the file. This line-at-a-time mapper +// cannot tell that case apart (a paged older-history load can also start a +// page with a `compacted` line that has predecessors in the previous page), +// so the fix belongs where the loader knows it is mapping from file offset 0. +// Before #1386 no `compacted` line rendered at all, so this is not a +// regression. WHY the summary is conditional: `message` is empty in every +// 0.15x rollout, where the summary is encrypted; only 56 of about 1,500 local +// compactions (older CLIs) carry readable text. +function mapCodexCompacted( + uuid: string, + timestamp: string | undefined, + payload: Record, +): Entry[] { + const out: Entry[] = [codexCompactBoundaryEntry(`${uuid}:compact-boundary`, timestamp)] + const message = typeof payload.message === 'string' ? payload.message.trim() : '' + if (message) out.push(codexCompactSummaryEntry(`${uuid}:compact-summary`, timestamp, message)) + return out +} + function codexCompactSummaryEntry( uuid: string, timestamp: string | undefined, @@ -295,6 +337,11 @@ function mapCodexRolloutToFeedEntriesUnstamped(entry: Record): const timestamp = typeof entry.timestamp === 'string' ? entry.timestamp : undefined + // WHY before the payload.type guard (#1289): a `compacted` line's payload + // has no `type` (every one of about 1,500 local lines), so that guard + // returned [] for all of them and Codex compaction never rendered. + if (entry.type === 'compacted' && payload) return mapCodexCompacted(uuid, timestamp, payload) + if (!payload || typeof payload.type !== 'string') return [] if (entry.type === 'event_msg') { @@ -397,33 +444,6 @@ function mapCodexRolloutToFeedEntriesUnstamped(entry: Record): return [] } - if (entry.type === 'compacted') { - const out: Entry[] = [ - codexCompactBoundaryEntry(`${uuid}:compact-boundary`, payload), - ] - - const message = typeof payload.message === 'string' ? payload.message.trim() : '' - if (message) { - out.push(codexCompactSummaryEntry(`${uuid}:compact-summary`, timestamp, message)) - } - - const replacementHistory = Array.isArray(payload.replacement_history) - ? payload.replacement_history - : [] - for (let i = 0; i < replacementHistory.length; i += 1) { - const item = asRecord(replacementHistory[i]) - if (!item) continue - const mapped = codexConversationEntryFromMessageItem( - `${uuid}:replacement:${i}`, - timestamp, - item, - ) - if (mapped) out.push(mapped) - } - - return out - } - if (entry.type !== 'response_item') return [] const conversationEntry = codexConversationEntryFromMessageItem(uuid, timestamp, payload) diff --git a/src/providers/shared/renderer/components/lazy-prose/index.renderer.test.tsx b/src/providers/shared/renderer/components/lazy-prose/index.renderer.test.tsx index cbfb3dd10..10d6d8e51 100644 --- a/src/providers/shared/renderer/components/lazy-prose/index.renderer.test.tsx +++ b/src/providers/shared/renderer/components/lazy-prose/index.renderer.test.tsx @@ -1,16 +1,26 @@ import { render, screen } from '@testing-library/react' -import { describe, expect, it } from 'vitest' +import { beforeAll, describe, expect, it } from 'vitest' import { LazyTextProse } from '@providers/shared/renderer/components/lazy-prose' describe('LazyTextProse browser boundary', () => { + // WHY load the split chunk here (#1107, #700): its first transform under Vitest can take seconds + // on a loaded machine, and the test used to measure that with a 5 s findByText — the same number + // as Vitest's 5 s test default, so the test-level timeout always won and the useful message + // ("the element never appeared") could never fire. Awaiting the import itself (not a timer) moves + // compiler warm-up out of the assertion; production reuses a loaded chunk the same way. The lazy + // boundary is still exercised: LazyTextProse resolves its own import() on render. + beforeAll(async () => { + await import('@renderer/features/feed/ui/markdown') + }) + it('loads the bounded Markdown surface on demand in a renderer environment', async () => { render() - // The first transform of the split Markdown chunk can exceed Testing - // Library's one-second default in CI/dev Vitest; production reuses the - // loaded chunk. This timeout tests eventual ownership without turning - // compiler warm-up into a renderer failure. - const strong = await screen.findByText('Evidence-backed', {}, { timeout: 5_000 }) + // The boundary itself (review of #1377, a): even with the chunk preloaded, React.lazy suspends on + // the first render and shows the fallback. An eager import would render the prose at once and + // skip it, so this assertion is what fails if the component stops loading on demand. + expect(screen.getByRole('status')).toHaveTextContent('Formatting content') + const strong = await screen.findByText('Evidence-backed') expect(strong.tagName).toBe('STRONG') expect(strong.parentElement).toHaveTextContent('Evidence-backed rendering') }) diff --git a/src/renderer/src/app/App.tsx b/src/renderer/src/app/App.tsx index 716e8b5c7..a6a640322 100644 --- a/src/renderer/src/app/App.tsx +++ b/src/renderer/src/app/App.tsx @@ -143,21 +143,21 @@ export default function App() { RetainedWorkspaceSurface: a that is ever detached from the DOM is destroyed, so no layout change or takeover may own it. Each guest is its own fixed element at z-index 20 — above the lanes - it overlays, below every z-50 overlay and modal after this line. */} + it overlays, below every pane overlay and app layer + (ui/layers.ts). */} - {/* Mount order here IS the z-order contract: overlays and modals - are fixed-position siblings and mostly share z-50, so DOM - order is the paint-order tiebreaker. Overlays render first - (paint under); anything that must sit at a specific height - within the modal stack lives in modalSurfaces at an explicit - index — see app/surfaces/registry.tsx. Do not swap these. */} + {/* Stacking is by named layer (ui/layers.ts), and within the dialog + layer by OPEN order: each Radix Dialog portals into when it + opens, so the one opened last paints on top. Render order here only + breaks same-commit ties; see the note in app/surfaces/registry.tsx. + Keep overlays before modals so those ties stay as they were. */} {/* Voice-dictation guide modal, opened by the "Configure Voice - Dictation" nudge and by a future command-palette entry. Sits - after GlobalModals in DOM order so it paints on top of the - other overlay stacks without competing with the modalSurfaces - registry — the guide is entirely local, no z-index tricks. */} + Dictation" nudge and by a future command-palette entry. It is a + dialog like the registry's, so it paints above them when opened + after them (open order, see above); its position here only breaks + a same-commit tie. */} diff --git a/src/renderer/src/app/surfaces/GlobalModals.tsx b/src/renderer/src/app/surfaces/GlobalModals.tsx index 858d09dd0..3676d033f 100644 --- a/src/renderer/src/app/surfaces/GlobalModals.tsx +++ b/src/renderer/src/app/surfaces/GlobalModals.tsx @@ -3,9 +3,8 @@ import { sortSurfacesByLayer } from './types' export function GlobalModals() { // Sorted by layer before render. First-party entries omit `layer`, so the stable - // sort leaves their documented order untouched; an extension-contributed surface - // (EXTENSION_SURFACE_LAYER) is lifted into its own band above the first-party - // stack instead of tie-breaking into it. See SurfaceEntry.layer. + // sort leaves their documented order untouched. This orders RENDER only: + // dialogs paint by open order within LAYERS.dialog (see SurfaceEntry.layer). return ( <> {sortSurfacesByLayer(modalSurfaces).map(entry => ( diff --git a/src/renderer/src/app/surfaces/registry.tsx b/src/renderer/src/app/surfaces/registry.tsx index 974273041..80973fbff 100644 --- a/src/renderer/src/app/surfaces/registry.tsx +++ b/src/renderer/src/app/surfaces/registry.tsx @@ -44,35 +44,38 @@ import { AgentMcpServersSurface } from '@renderer/features/mcp/surfaces/AgentMcp // in the owning feature's surfaces/ folder + add ONE import + ONE array // entry here. App.tsx is never edited. // -// ORDER MATTERS within each array, AND the mount order of the groups in -// App.tsx (overlays → modals) is part of the same contract: together they -// define the DOM sibling order at the app root, which IS the paint order -// whenever z-indexes tie. Most of these surfaces are `position: fixed` -// z-50, so "which array, at which index" decides what covers what. The -// order below is the exact order App.tsx rendered these surfaces before -// the extraction — keep new entries at the END unless you have a stacking -// reason and write it down. +// HOW STACKING ACTUALLY WORKS (#512, corrected in review): the layers are +// named in ui/layers.ts. Almost every entry here renders the shared Radix +// Dialog (LAYERS.dialog; the caffeinate entry renders nothing and forwards to +// the app toast). A Dialog's content portals into when it OPENS, so +// between two open dialogs the one OPENED LATER paints on top, whatever their +// order in this array. Array order only decides between dialogs that open in +// the same React commit. A surface that must always sit above another needs +// an explicit mechanism (its own layer in ui/layers.ts), not an array index. +// +// The order below is still the exact order App.tsx rendered these surfaces +// before the extraction; keep new entries at the END so same-commit ties do +// not move. /** Rendered at the app root, after the overlays. */ export const modalSurfaces: SurfaceEntry[] = [ { id: 'command-palette', Component: CommandPaletteSurface }, { id: 'path-picker', Component: PathPickerSurface }, - // ⚠ Two non-modal surfaces interleaved into the modal stack ON PURPOSE. - // Pre-refactor App.tsx rendered them exactly here — after the palette - // and path picker, before the tile-tabs..usage modals — and that DOM - // position is load-bearing because all three of palette / dispatch-count - // / toast are fixed z-50, so sibling order is the only tiebreaker: - // - tiled-dispatch-count must paint ABOVE the command palette. Tiled - // dispatch can fire while the palette is open (native menu; the - // palette deliberately stays open for keepPaletteOpen-style flows), - // and the count prompt is the thing awaiting input — burying it - // behind the palette soft-locks the flow. - // - both must stay BELOW the later modals (a modal opened over the - // toast dims it, as before). - // The first cut of this registry put these two in overlaySurfaces - // (rendered before the modals group), which silently reversed the - // palette/count-prompt stacking — codex review of PR #505 caught it. - // Grouping by semantic kind is NOT safe here; group by paint order. + // The next three entries were interleaved here ON PURPOSE when they were + // fixed z-50 siblings of the palette, and pre-refactor App.tsx rendered + // them exactly here. Today: + // - tiled-dispatch-count and dispatch-row-project are Dialogs in + // LAYERS.dialog. The count prompt paints above the command palette + // because it OPENS after it (tiled dispatch fires from an open + // palette, and the prompt is what awaits input; burying it would + // soft-lock the flow). Any dialog opened after either of them paints + // over it, whatever the array says. + // - caffeinate-toast renders nothing and forwards to the app toast + // (LAYERS.toast). + // Their position here only decides a same-commit tie. History: the first + // cut of this registry moved the two prompts into overlaySurfaces, which + // reversed the palette/count-prompt stacking while they were z-50 siblings + // (codex review of PR #505). { id: 'tiled-dispatch-count', Component: TiledDispatchCountSurface }, { id: 'dispatch-row-project', Component: DispatchRowProjectSurface }, { id: 'caffeinate-toast', Component: CaffeinateToastSurface }, @@ -93,8 +96,8 @@ export const modalSurfaces: SurfaceEntry[] = [ { id: 'rewind-to-prompt', Component: RewindToPromptSurface }, { id: 'agent-title-prompt', Component: AgentTitlePromptSurface }, { id: 'usage', Component: UsageModalSurface }, - // New modals append so their z-50 sibling order cannot accidentally move an - // established surface below one it used to cover; see the registry contract. + // New modals append so a same-commit tie cannot move an established + // surface; see the stacking note above. { id: 'provider-switch-picker', Component: ProviderSwitchPickerSurface }, { id: 'key-vault', Component: KeyVaultModalSurface }, // Appended per the contract above. It is only opened from a command, which @@ -103,16 +106,19 @@ export const modalSurfaces: SurfaceEntry[] = [ { id: 'new-agent-in', Component: NewAgentInSurface }, // Appended per the contract above. Opened only from a session command that // closes the palette first; it must paint over every established modal so - // the warning is never hidden behind the surface it is warning about. + // the warning is never hidden behind the surface it is warning about, and + // it does because it opens after them (open order). { id: 'root-management-confirm', Component: RootManagementConfirmSurface }, // Appended per the contract above; opened only from a command that closes // the palette first (#913). { id: 'merge-project-tabs', Component: MergeProjectTabsSurface }, // Appended per the contract above. Opened only from a session command that - // closes the palette first, so it stacks over established modals by order. + // closes the palette first; it paints over anything already open because it + // opens later (open order, see the stacking note above). { id: 'tldr-history', Component: ReportHistorySurface }, // Appended per the contract above (#964). Opened only from a command that - // closes the palette first, so it stacks over established modals by order. + // closes the palette first; it paints over anything already open because it + // opens later. { id: 'agent-analytics', Component: AgentAnalyticsSurface }, // Appended per the contract above (#1143); both are opened from commands // that close the palette first. The per-agent picker can hand off to the @@ -123,32 +129,38 @@ export const modalSurfaces: SurfaceEntry[] = [ { id: 'mcp-server-dialog', Component: McpServerDialogSurface }, // Appended per the contract above (#1161). Opened from the Skills grid, the // "Add Skill…" command (which closes the palette first) and an external - // skill's "Manage with Agent Code"; it stacks over Settings by order. + // skill's "Manage with Agent Code"; it paints over Settings because it opens + // after it. { id: 'add-skill-dialog', Component: AddSkillDialogSurface }, - // Built-in apps host. Last in the array, which per the paint-order contract - // above means it paints above every modal already mounted. That placement is - // reasoned, not defaulted: an app is always user-initiated from the palette and - // is the thing awaiting input for as long as it is open, so nothing already on - // screen has a claim to cover it. No app has a reason to sit *under* another - // modal — if one ever does, that is a signal it should not be an app. + // Built-in apps host. An app is always user-initiated and is the thing + // awaiting input while open, so it should cover what is already on screen, + // and it does: it opens after them (open order, see the stacking note above). + // Its position here only decides a same-commit tie. A dialog opened + // AFTER an app paints over it; no app has a reason to sit under another + // modal, and if one ever does, that is a signal it should not be an app. { id: 'app-host', Component: AppHostSurface }, // The shared in-app confirm (replaced window.confirm, keyboard-first plan - // D8). LAST, after even app-host, because a confirm is always a question - // ABOUT the surface underneath it — the Conventions editor asking "discard - // changes?", Key Vault asking "delete key?" — so it must paint above - // whichever surface asked. It renders nothing until requestConfirm queues - // a request. + // D8). A confirm is always a question ABOUT the surface underneath it (the + // Conventions editor asking "discard changes?", Key Vault asking "delete + // key?"), so it must paint above whichever surface asked. It does, because + // it opens after that surface (open order); its position here only decides + // a same-commit tie. It renders nothing until requestConfirm queues a request. { id: 'confirm-dialog', Component: ConfirmHost }, + // #512: RemotePanel renders a centred Radix Dialog, but was registered as a + // side panel, so it mounted inside the main row and only painted as a modal + // because DialogContent portals out. It is a modal; it lives here, appended + // per the contract above (its portal stacks by open order either way). + { id: 'remote-panel', Component: RemotePanelSurface }, ] /** * Rendered at the app root, after the main row, BEFORE the modals — so * everything in this array paints UNDER the modal stack when z-indexes - * tie. Only surfaces that must never cover a modal belong here (voice - * dictation is z-40, below the z-50 stack regardless). A z-50 surface - * that needs a specific position relative to the modals goes into - * modalSurfaces at an explicit index instead — see the interleaved - * entries there for why. + * tie. Voice dictation's chip is in LAYERS.toast, above every dialog on + * purpose (dictating into a dialog must stay visible), so its position here + * no longer decides its stacking. A surface that must sit at a fixed height + * relative to the dialogs needs its own named layer in ui/layers.ts; a + * position in either array only breaks same-commit ties. */ export const overlaySurfaces: SurfaceEntry[] = [ { id: 'voice-dictation', Component: VoiceDictationSurface }, @@ -159,6 +171,5 @@ export const sidePanelSurfaces: SurfaceEntry[] = [ { id: 'git-bar', Component: GitBarSurface }, { id: 'worktrees-bar', Component: WorktreesBarSurface }, { id: 'agent-status-panel', Component: AgentStatusPanelSurface }, - { id: 'remote-panel', Component: RemotePanelSurface }, { id: 'debug-surfaces', Component: DebugSurfaces }, ] diff --git a/src/renderer/src/app/surfaces/types.ts b/src/renderer/src/app/surfaces/types.ts index 3e21530fa..7bab76358 100644 --- a/src/renderer/src/app/surfaces/types.ts +++ b/src/renderer/src/app/surfaces/types.ts @@ -25,21 +25,24 @@ export type SurfaceEntry = { */ Component: ComponentType /** - * Paint band. Entries are stably sorted by `layer` (default 0) before render, - * so within a band the array order still decides sibling/paint order exactly as - * before — every first-party entry omits `layer` and keeps its documented - * position. The field exists so a NON-first-party surface (an extension one, - * WS7) can be given a distinct band it cannot escape: it can never tie-break - * into the first-party z-50 stack and silently reorder it, which is the exact - * class of bug PR #505 hit. First-party entries should not set it. + * RENDER band, not a paint band. Entries are stably sorted by `layer` + * (default 0) before render, which fixes their React render order and + * nothing else. Corrected in #512's review: every modal is a Radix Dialog + * in LAYERS.dialog that portals into when it OPENS, so between open + * dialogs the one opened last paints on top whatever its `layer`. A surface + * that must really sit above the first-party dialogs needs its own z layer + * in ui/layers.ts, and no entry has one today (EXTENSION_SURFACE_LAYER has + * no consumer). First-party entries should not set this. */ layer?: number } /** - * The band all extension-contributed surfaces sit in — above the first-party stack, - * so an extension surface always paints over app chrome (it is user-initiated and - * awaiting input) but cannot reorder first-party surfaces among themselves. + * The render band reserved for extension-contributed surfaces. It orders their + * React render after the first-party entries; it does NOT make them paint + * above an open first-party dialog (see SurfaceEntry.layer). No surface uses + * it yet; giving extension surfaces a real paint band means adding a layer to + * ui/layers.ts when the first one lands. */ export const EXTENSION_SURFACE_LAYER = 100 diff --git a/src/renderer/src/components/ui/dialog.tsx b/src/renderer/src/components/ui/dialog.tsx index fce4786a0..3cc225913 100644 --- a/src/renderer/src/components/ui/dialog.tsx +++ b/src/renderer/src/components/ui/dialog.tsx @@ -9,6 +9,7 @@ import { } from '@renderer/components/ui/pane-dialog' import { APP_INTERACTION_OWNER_ATTRIBUTE } from '@renderer/lib/interaction-ownership' import { cn } from '@renderer/lib/utils' +import { LAYERS } from '@renderer/ui/layers' // Adapted from https://ui.shadcn.com/docs/components/dialog. // @@ -52,7 +53,8 @@ const DialogOverlay = React.forwardRef< ref={ref} data-slot="dialog-overlay" className={cn( - 'fixed inset-0 z-[1100] bg-overlay-scrim-strong', + 'fixed inset-0 bg-overlay-scrim-strong', + LAYERS.dialog, className, )} {...props} @@ -157,7 +159,8 @@ const DialogContent = React.forwardRef< // the TRACK minimum lets the child shrink first; each child still owns // whether its content truncates, wraps, or scrolls. className={cn( - 'fixed left-1/2 top-1/2 z-[1100] grid grid-cols-[minmax(0,1fr)] -translate-x-1/2 -translate-y-1/2 rounded-float border border-border-hi bg-surface text-ink shadow-[0_16px_48px_var(--theme-shadow-color)] outline-none', + LAYERS.dialog, + 'fixed left-1/2 top-1/2 grid grid-cols-[minmax(0,1fr)] -translate-x-1/2 -translate-y-1/2 rounded-float border border-border-hi bg-surface text-ink shadow-[0_16px_48px_var(--theme-shadow-color)] outline-none', dialogSizes[size], // The corner `× ⎋` (below) sits over the header's right end, so the // header must leave room for it. Owned HERE (UI pass, G-20): five diff --git a/src/renderer/src/components/ui/dropdown-menu.tsx b/src/renderer/src/components/ui/dropdown-menu.tsx index 179cee6f9..d4270b516 100644 --- a/src/renderer/src/components/ui/dropdown-menu.tsx +++ b/src/renderer/src/components/ui/dropdown-menu.tsx @@ -2,6 +2,7 @@ import * as DropdownMenuPrimitive from '@radix-ui/react-dropdown-menu' import * as React from 'react' import { cn } from '@renderer/lib/utils' +import { LAYERS } from '@renderer/ui/layers' // Adapted from https://ui.shadcn.com/docs/components/dropdown-menu. // @@ -51,7 +52,8 @@ const DropdownMenuContent = React.forwardRef< data-slot="dropdown-menu-content" sideOffset={sideOffset} className={cn( - 'modal-pop z-[1150] min-w-[180px] overflow-hidden rounded-float border border-popover-border bg-popover-bg p-1 font-code text-ink shadow-[0_8px_24px_var(--theme-shadow-color)] outline-none', + LAYERS.menu, + 'modal-pop min-w-[180px] overflow-hidden rounded-float border border-popover-border bg-popover-bg p-1 font-code text-ink shadow-[0_8px_24px_var(--theme-shadow-color)] outline-none', className, )} {...props} diff --git a/src/renderer/src/components/ui/pane-dialog.tsx b/src/renderer/src/components/ui/pane-dialog.tsx index 453a574a7..21a663ad9 100644 --- a/src/renderer/src/components/ui/pane-dialog.tsx +++ b/src/renderer/src/components/ui/pane-dialog.tsx @@ -3,6 +3,7 @@ import { createPortal } from 'react-dom' import { PANE_INTERACTION_OWNER_ATTRIBUTE } from '@renderer/lib/interaction-ownership' import { cn } from '@renderer/lib/utils' +import { LAYERS } from '@renderer/ui/layers' // Pane-scoped dialogs (#713, keyboard-first plan X1). // @@ -55,9 +56,9 @@ import { cn } from '@renderer/lib/utils' // leaves the pane. /** - * The pane's stacking levels while a pane dialog is up, as literal Tailwind - * classes (Tailwind only emits classes it can see spelled out). ONE table so - * the order cannot drift between files: + * The pane's stacking levels while a pane dialog is up. The classes are + * spelled in the app-wide layer table (ui/layers.ts, #512); this names the + * three a pane dialog uses: * scrim covers the pane, including the composer it must not let you type into; * content the dialog itself; * feedback the pane's own status toast. #713's second half was a refusal @@ -67,9 +68,9 @@ import { cn } from '@renderer/lib/utils' * composer stays covered. */ export const PANE_DIALOG_LAYERS = { - scrim: 'z-[60]', - content: 'z-[61]', - feedback: 'z-[62]', + scrim: LAYERS.paneDialogScrim, + content: LAYERS.paneDialogContent, + feedback: LAYERS.paneDialogFeedback, } as const export type PaneDialogHost = { diff --git a/src/renderer/src/components/ui/side-panel.renderer.test.tsx b/src/renderer/src/components/ui/side-panel.renderer.test.tsx new file mode 100644 index 000000000..8ca83fa3e --- /dev/null +++ b/src/renderer/src/components/ui/side-panel.renderer.test.tsx @@ -0,0 +1,32 @@ +import { render, screen } from '@testing-library/react' +import { expect, it, vi } from 'vitest' + +import { DebugPanelHeader } from '@renderer/features/debug/ui/DebugPanelHeader' +import { SidePanel } from './side-panel' + +// #512: one shell for the docked side-panel slot, and a named close on the +// debug-family header (five copies drew a bare "×" with no accessible name). +it('renders a named complementary landmark with its content, width and column layout', () => { + render(

panel body

) + const panel = screen.getByRole('complementary', { name: 'Git' }) + expect(panel).toContainElement(screen.getByText('panel body')) + // The shell's layout contract: a full-height, non-shrinking column that + // clips its own overflow so each panel scrolls inside itself. + for (const cls of ['w-[280px]', 'border-l', 'h-full', 'flex-shrink-0', 'flex', 'flex-col', 'overflow-hidden', 'bg-surface']) { + expect(panel.className.split(/\s+/)).toContain(cls) + } +}) + +it('lets a panel override the border colour instead of stacking two', () => { + render(body) + const panel = screen.getByRole('complementary', { name: 'Rendering debug' }) + expect(panel.className).toContain('border-red-500/60') + expect(panel.className).not.toMatch(/(^|\s)border-border(\s|$)/) +}) + +it('gives the debug header close an accessible name', () => { + const onClose = vi.fn() + render() + screen.getByRole('button', { name: 'Close dev debug' }).click() + expect(onClose).toHaveBeenCalledTimes(1) +}) diff --git a/src/renderer/src/components/ui/side-panel.tsx b/src/renderer/src/components/ui/side-panel.tsx new file mode 100644 index 000000000..a4d8c575e --- /dev/null +++ b/src/renderer/src/components/ui/side-panel.tsx @@ -0,0 +1,46 @@ +import * as React from 'react' + +import { cn } from '@renderer/lib/utils' + +/** + * The outer shell of a docked side panel: the column that sits in the main + * row to the right of the workspace (Git, Worktrees, Agent Status, the debug + * panels). Pair it with `PanelHeader` for the header (#512). + * + * WHY a shell component: nine panels carried copies of the same class string + * (`h-full w-[N] flex-shrink-0 border-l border-border bg-surface flex flex-col + * overflow-hidden`), which had already drifted (one `aside` among `div`s, one + * without the code font, one with its own border colour). The slot is one + * thing in the window, so its chrome is one component. + * + * WHY width stays a className (`w-[280px]`) and not a number prop: Tailwind + * only emits classes it sees spelled out, so a runtime `w-[${n}px]` would + * produce no CSS. Each panel keeps its width because each was sized for its + * content (HTML wraps badly under 540px, for one). + * + * WHY no z-index: side panels are in normal flow in the main row, never + * floating. A panel that needs to float is not a side panel (RemotePanel was + * registered here while rendering a centred dialog; it now lives with the + * modals). + */ +export function SidePanel({ + label, + className, + children, + ...props +}: React.ComponentProps<'aside'> & { + /** The panel's name for assistive technology ("Git"); the aside is a + * landmark, so it needs one. */ + label: string +}) { + return ( + + ) +} diff --git a/src/renderer/src/features/agent-status/ui/AgentStatusPanel.tsx b/src/renderer/src/features/agent-status/ui/AgentStatusPanel.tsx index fa19295fd..e7a17ca72 100644 --- a/src/renderer/src/features/agent-status/ui/AgentStatusPanel.tsx +++ b/src/renderer/src/features/agent-status/ui/AgentStatusPanel.tsx @@ -14,6 +14,7 @@ import { } from '@renderer/features/agent-status/model/formatAgentStatus' import type { AgentStatusField } from '@renderer/features/agent-status/model/formatAgentStatus' import type { Workspace } from '@renderer/workspace/workspaceStore' +import { SidePanel } from '@renderer/components/ui/side-panel' type Props = { sessionId: string @@ -29,12 +30,7 @@ export function AgentStatusPanel({ sessionId, workspace, onClose }: Props) { ) return ( - + ) } diff --git a/src/renderer/src/features/browser-pocket/ui/PocketStrip.tsx b/src/renderer/src/features/browser-pocket/ui/PocketStrip.tsx index bf051e75c..27da7c846 100644 --- a/src/renderer/src/features/browser-pocket/ui/PocketStrip.tsx +++ b/src/renderer/src/features/browser-pocket/ui/PocketStrip.tsx @@ -9,6 +9,7 @@ import { attachPocket, setPocketView } from '../actions' import { requestPocket } from '../state/pocketBus' import { usePocketLive, usePocketLiveStore } from '../state/pocketLiveStore' import { useLanePorts } from '../state/lanePortsStore' +import { LAYERS } from '@renderer/ui/layers' /** * The 22 px collapsed pocket in a lane (spec §4.1): glanceable, never a @@ -96,7 +97,7 @@ export function PocketStrip({ sessionId, workspace }: { sessionId: SessionId; wo role="tooltip" // Popover chrome like every floating menu (theme shadow; the old // hard rgba shadow was a smear on light themes). - className="rounded-float pointer-events-none absolute bottom-[24px] left-2 z-40 flex w-[320px] flex-col gap-1 overflow-hidden border border-popover-border bg-popover-bg p-1.5 text-[11px] text-ink shadow-[0_8px_24px_var(--theme-shadow-color)]" + className={`rounded-float pointer-events-none absolute bottom-[24px] left-2 ${LAYERS.paneOverlay} flex w-[320px] flex-col gap-1 overflow-hidden border border-popover-border bg-popover-bg p-1.5 text-[11px] text-ink shadow-[0_8px_24px_var(--theme-shadow-color)]`} > {live.thumbnail ? : null} {details.map(line => ( diff --git a/src/renderer/src/features/cli-updates/CliUpdateBehaviorRow.renderer.test.tsx b/src/renderer/src/features/cli-updates/CliUpdateBehaviorRow.renderer.test.tsx index fc5ffccec..c17ee1a52 100644 --- a/src/renderer/src/features/cli-updates/CliUpdateBehaviorRow.renderer.test.tsx +++ b/src/renderer/src/features/cli-updates/CliUpdateBehaviorRow.renderer.test.tsx @@ -1,4 +1,4 @@ -import { act, cleanup, fireEvent, render, screen, within } from '@testing-library/react' +import { act, cleanup, fireEvent, render, screen, waitFor, within } from '@testing-library/react' import { afterEach, describe, expect, it, vi } from 'vitest' import { CliUpdateBehaviorRow } from './CliUpdateBehaviorRow' @@ -24,4 +24,24 @@ describe('CliUpdateBehaviorRow', () => { fireEvent.keyDown(checked, { key: 'ArrowRight' }) expect(setBehavior).toHaveBeenCalledTimes(1) }) + + // #1250 row 13: the write had no rejection handler at all. + it('says a failed write did not save, and keeps showing main\'s value', async () => { + const setBehavior = vi.fn(async () => { throw new Error("EACCES: permission denied, open '/Users/someone/setup.json'") }) + Object.assign(window, { api: { ...window.api, cliUpdatesSetBehavior: setBehavior } }) + act(() => { useCliUpdateStore.setState({ snapshot: { ...useCliUpdateStore.getState().snapshot, behavior: 'notify' } }) }) + render() + const group = screen.getByRole('radiogroup', { name: 'CLI Update Behavior' }) + const off = within(group).getAllByRole('radio').find(radio => radio.textContent?.includes('Off'))! + fireEvent.click(off) + expect(await screen.findByRole('alert')).toHaveTextContent("Couldn't save this change. Nothing was changed.") + expect(document.body.textContent).not.toContain('EACCES') + const checked = within(group).getAllByRole('radio').find(radio => radio.getAttribute('aria-checked') === 'true')! + expect(checked.textContent).toContain('Notify Only') + + // A later choice that saves clears the message (#1403 review b). + setBehavior.mockImplementation(async () => ({ ...useCliUpdateStore.getState().snapshot, behavior: 'off' }) as never) + fireEvent.click(off) + await waitFor(() => expect(screen.queryByRole('alert')).toBeNull()) + }) }) diff --git a/src/renderer/src/features/cli-updates/CliUpdateBehaviorRow.tsx b/src/renderer/src/features/cli-updates/CliUpdateBehaviorRow.tsx index a7c9cc06b..13232bfd3 100644 --- a/src/renderer/src/features/cli-updates/CliUpdateBehaviorRow.tsx +++ b/src/renderer/src/features/cli-updates/CliUpdateBehaviorRow.tsx @@ -1,6 +1,8 @@ +import { useState } from 'react' import type { CliUpdateBehavior } from '@shared/types/cliUpdate.js' import { OptionCards } from '@renderer/components/ui/option-cards' import { setCliUpdateBehavior, useCliUpdateStore } from '@renderer/features/cli-updates/store' +import { SETUP_WRITE_FAILED } from '@renderer/features/settings/setupWriteFailed' // Settings row for CLI auto-update behavior. // @@ -44,9 +46,21 @@ export function CliUpdateBehaviorRow() { // Subscribe to just the behavior slice — the banner state churn (updating // → updated etc.) should not re-render this row. const behavior = useCliUpdateStore(state => state.snapshot.behavior) + // #1250 row 13: a failed write is said here, in the same fixed words as the + // provider rows (setup.json is the same file). The cards keep showing + // main's value: a save whose write failed never becomes main's state + // (`updateSetupState`). A later choice clears the message. + const [failed, setFailed] = useState(false) + const choose = (next: CliUpdateBehavior) => { + setFailed(false) + void setCliUpdateBehavior(next).catch(() => setFailed(true)) + } return ( - // The shared radio cards (UI pass, G-17): this row announced NO state - // (plain buttons), was square, and had no focus ring. - +
+ {/* The shared radio cards (UI pass, G-17): this row announced NO state + (plain buttons), was square, and had no focus ring. */} + + {failed ?
{SETUP_WRITE_FAILED}
: null} +
) } diff --git a/src/renderer/src/features/cli-updates/store.ts b/src/renderer/src/features/cli-updates/store.ts index f8630bda4..a871e8074 100644 --- a/src/renderer/src/features/cli-updates/store.ts +++ b/src/renderer/src/features/cli-updates/store.ts @@ -121,12 +121,15 @@ export function useCliUpdateSync(): void { }, []) } -/** Change the CLI-update behavior. Fire-and-forget; the returned snapshot - * from main flows back through the state channel. Exposed as a plain - * function (not a hook) so the settings-row callback can call it - * without threading a hook through several component layers. */ -export function setCliUpdateBehavior(behavior: CliUpdateBehavior): void { - void window.api.cliUpdatesSetBehavior(behavior).then((snapshot) => { - useCliUpdateStore.getState().setSnapshot(snapshot) - }) +/** Change the CLI-update behavior. The returned snapshot from main also + * flows back through the state channel. Exposed as a plain function (not a + * hook) so the settings-row callback can call it without threading a hook + * through several component layers. + * + * Returns the promise (#1250 row 13): it used to be fire-and-forget with no + * rejection handler, so a failed write was an unhandled rejection and the + * row showed nothing. The caller reports it. */ +export async function setCliUpdateBehavior(behavior: CliUpdateBehavior): Promise { + const snapshot = await window.api.cliUpdatesSetBehavior(behavior) + useCliUpdateStore.getState().setSnapshot(snapshot) } diff --git a/src/renderer/src/features/command-palette/ui/CommandSortControl.tsx b/src/renderer/src/features/command-palette/ui/CommandSortControl.tsx index 6c458e702..eded9aea6 100644 --- a/src/renderer/src/features/command-palette/ui/CommandSortControl.tsx +++ b/src/renderer/src/features/command-palette/ui/CommandSortControl.tsx @@ -5,6 +5,7 @@ import { COMMAND_SORT_MODE_LABELS, } from '@renderer/features/command-palette/lib/sortCommands' import type { CommandSortMode } from '@renderer/features/command-palette/lib/sortCommands' +import { LAYERS } from '@renderer/ui/layers' // The palette's browse-order picker. // @@ -203,12 +204,12 @@ export function CommandSortControl({ role="menu" // Right-anchored: the control sits at the right edge of the header, // so a left-anchored menu would hang off the dialog. - className="rounded-float overflow-hidden - absolute right-0 top-[calc(100%+4px)] z-50 + className={`rounded-float overflow-hidden + absolute right-0 top-[calc(100%+4px)] ${LAYERS.inSurfacePopover} min-w-[168px] border border-popover-border bg-popover-bg shadow-[0_8px_24px_var(--theme-shadow-color)] - " + `} > {COMMAND_SORT_MODES.map((candidate, index) => ( - +
@@ -402,7 +405,7 @@ export function RenderingDebugInspector({ sessionId, provider, onSave, onClose }
)} - + ) } diff --git a/src/renderer/src/features/debug/ui/DebugPanel.tsx b/src/renderer/src/features/debug/ui/DebugPanel.tsx index 3570477dd..1d1b3b1bd 100644 --- a/src/renderer/src/features/debug/ui/DebugPanel.tsx +++ b/src/renderer/src/features/debug/ui/DebugPanel.tsx @@ -5,6 +5,8 @@ import type { SessionRuntime } from '@renderer/workspace/workspaceStore' import type { Entry } from '@shared/types/transcript' import { AgentInlineTerminal } from '@renderer/features/debug/ui/AgentInlineTerminal' import { useScreenLease } from '@renderer/features/debug/useScreenLease' +import { DebugPanelHeader } from './DebugPanelHeader' +import { SidePanel } from '@renderer/components/ui/side-panel' // DebugPanel — inline diagnostic overlay showing the raw state of the // focused pane. Toggled via "Toggle Debug Panel" in the command palette. @@ -66,30 +68,9 @@ export function DebugPanel({ }, [runtime.entries]) return ( -
+ {/* Header */} -
- debug — {kind} session - -
+
{/* State flags */} @@ -185,7 +166,7 @@ export function DebugPanel({ 
{runtime.projectDir ?? '(none)'}
-
+ ) } diff --git a/src/renderer/src/features/debug/ui/DebugPanelHeader.tsx b/src/renderer/src/features/debug/ui/DebugPanelHeader.tsx new file mode 100644 index 000000000..010a15222 --- /dev/null +++ b/src/renderer/src/features/debug/ui/DebugPanelHeader.tsx @@ -0,0 +1,47 @@ +import type { ReactNode } from 'react' + +/** + * The header shared by the debug-family side panels (#512): a red uppercase + * title, optional actions, and one close. + * + * WHY its own header and not PanelHeader: these are diagnostic tools, and the + * red title is the deliberate cue that the panel reads raw session state + * rather than being product UI (ProxyDebugPanel's note). What they must share + * with PanelHeader is a close with an accessible name: all five copies of + * this header drew a bare "×" that a screen reader announced as nothing. + */ +export function DebugPanelHeader({ + title, + closeLabel, + actions, + onClose, +}: { + title: ReactNode + /** Accessible name of the close button ("Close debug logs"). */ + closeLabel: string + actions?: ReactNode + onClose: () => void +}) { + return ( +
+ {title} +
+ {actions} + +
+
+ ) +} diff --git a/src/renderer/src/features/debug/ui/DevDebugPanel.tsx b/src/renderer/src/features/debug/ui/DevDebugPanel.tsx index b48a2be1d..bb167bf82 100644 --- a/src/renderer/src/features/debug/ui/DevDebugPanel.tsx +++ b/src/renderer/src/features/debug/ui/DevDebugPanel.tsx @@ -8,6 +8,8 @@ import type { } from '@renderer/features/debug/devModules/types' import type { Workspace } from '@renderer/workspace/workspaceStore' import type { SessionRuntime } from '@renderer/session-runtime/state' +import { DebugPanelHeader } from './DebugPanelHeader' +import { SidePanel } from '@renderer/components/ui/side-panel' const STORAGE_KEY = 'agent-code:dev-debug:enabled-modules' @@ -79,27 +81,8 @@ export function DevDebugPanel({ sessionId, runtime, kind, workspace, onClose }: } return ( -
-
- dev debug - -
+ +
@@ -198,7 +181,7 @@ export function DevDebugPanel({ sessionId, runtime, kind, workspace, onClose }: )) )}
-
+ ) } diff --git a/src/renderer/src/features/debug/ui/FeedDebugPanel.tsx b/src/renderer/src/features/debug/ui/FeedDebugPanel.tsx index 7fca4e096..14a9cf72a 100644 --- a/src/renderer/src/features/debug/ui/FeedDebugPanel.tsx +++ b/src/renderer/src/features/debug/ui/FeedDebugPanel.tsx @@ -1,6 +1,8 @@ import { useCallback, useEffect, useMemo, useRef, useState } from 'react' import type { FeedDebugEntry, FeedDebugLayer, SessionRuntime } from '@renderer/session-runtime/state' +import { DebugPanelHeader } from './DebugPanelHeader' +import { SidePanel } from '@renderer/components/ui/side-panel' type Props = { sessionId: string @@ -55,27 +57,8 @@ export function FeedDebugPanel({ sessionId, runtime, kind, onClose }: Props) { }, []) return ( -
-
- debug logs — {kind} pane - -
+ +
session @@ -154,7 +137,7 @@ export function FeedDebugPanel({ sessionId, runtime, kind, onClose }: Props) { }) )}
-
+ ) } diff --git a/src/renderer/src/features/debug/ui/HtmlDebugPanel.tsx b/src/renderer/src/features/debug/ui/HtmlDebugPanel.tsx index 497bda106..01872c5a4 100644 --- a/src/renderer/src/features/debug/ui/HtmlDebugPanel.tsx +++ b/src/renderer/src/features/debug/ui/HtmlDebugPanel.tsx @@ -1,6 +1,8 @@ import { useCallback, useEffect, useMemo, useState } from 'react' import { sanitizeHtml } from '@renderer/lib/sanitizeHtml' +import { DebugPanelHeader } from './DebugPanelHeader' +import { SidePanel } from '@renderer/components/ui/side-panel' // HtmlDebugPanel — grabs the focused pane's `outerHTML` and shows // it as copy-pasteable text. Fourth in the debug-panel family @@ -188,25 +190,17 @@ export function HtmlDebugPanel({ sessionId, kind, onClose }: Props) { : `no pane found for ${sessionId.slice(0, 8)}` return ( -
+ {/* Header — matches the existing debug panels: red uppercase title, row of action buttons on the right, × close. Width of 540px (not 380 like DebugPanel) because HTML wraps badly at narrower widths — FeedDebugPanel landed at 540 for the same reason. */} -
- debug — html ({kind}) -
+ - -
-
+ } + /> {/* Mode toggle row. Inline tab-style: two text buttons with the active one underlined/accented. Placed as its own @@ -298,7 +284,7 @@ export function HtmlDebugPanel({ sessionId, kind, onClose }: Props) {
)}
- + ) } diff --git a/src/renderer/src/features/debug/ui/ProxyDebugPanel.tsx b/src/renderer/src/features/debug/ui/ProxyDebugPanel.tsx index 405c0ec2f..ae6cbea4d 100644 --- a/src/renderer/src/features/debug/ui/ProxyDebugPanel.tsx +++ b/src/renderer/src/features/debug/ui/ProxyDebugPanel.tsx @@ -1,6 +1,8 @@ import { useMemo } from 'react' import { useAppStore } from '@renderer/app-state/hooks' import { emptySemanticRuntime } from '@renderer/session-runtime/state' +import { DebugPanelHeader } from './DebugPanelHeader' +import { SidePanel } from '@renderer/components/ui/side-panel' // ProxyDebugPanel — live inspector for the semantic stream and any // provider transport attribution that survives into the shared runtime. @@ -65,35 +67,12 @@ export function ProxyDebugPanel({ sessionId, kind, onClose }: Props) { }, [state.currentTurn]) return ( -
+ {/* Header — same chrome as DebugPanel. Kept red title for uniformity across debug-family panels (they are all diagnostic tools that read session state, not user-facing UI). */} -
- proxy debug — {kind} semantic stream -
- -
-
+
{/* Flow attribution */} @@ -251,7 +230,7 @@ export function ProxyDebugPanel({ sessionId, kind, onClose }: Props) {
-
+ ) } diff --git a/src/renderer/src/features/editor/ui/ExplorerPane.tsx b/src/renderer/src/features/editor/ui/ExplorerPane.tsx index 30756fec2..21d06ef84 100644 --- a/src/renderer/src/features/editor/ui/ExplorerPane.tsx +++ b/src/renderer/src/features/editor/ui/ExplorerPane.tsx @@ -6,6 +6,7 @@ import { FileIcon, FolderIcon } from '@renderer/features/editor/lib/fileIcon' // shape. See @shared/types/editorFs. import type { EditorFsEntry } from '@shared/types/editorFs' import { withVisibleControls } from '@shared/text/visibleControls' +import { LAYERS } from '@renderer/ui/layers' type TreeNode = { entry: EditorFsEntry @@ -779,7 +780,7 @@ export function ExplorerPane({ // pointer or the tree row (Shift+F10), which Radix DropdownMenu can // only fake with a virtual anchor, and its own roving focus, Escape // and Tab-closes behaviour are already keyboard-complete (M4). - className="fixed z-[1150] min-w-[160px] rounded-float overflow-hidden border border-popover-border bg-popover-bg py-1 shadow-[0_8px_24px_var(--theme-shadow-color)]" + className={`fixed ${LAYERS.menu} min-w-[160px] rounded-float overflow-hidden border border-popover-border bg-popover-bg py-1 shadow-[0_8px_24px_var(--theme-shadow-color)]`} style={{ left: menu.x, top: menu.y }} onMouseDown={event => event.stopPropagation()} onBlur={event => { diff --git a/src/renderer/src/features/git/ui/GitBar.tsx b/src/renderer/src/features/git/ui/GitBar.tsx index 3e98067fe..cbbf78b0c 100644 --- a/src/renderer/src/features/git/ui/GitBar.tsx +++ b/src/renderer/src/features/git/ui/GitBar.tsx @@ -11,6 +11,7 @@ import type { GitRecentCommit, GitSubmoduleStatus, } from '@shared/types/gitStatus' +import { SidePanel } from '@renderer/components/ui/side-panel' // GitBar — a narrow right-edge panel showing git state for the // focused pane's cwd: current branch, latest 5 commits, and the @@ -82,13 +83,7 @@ export function GitBar({ cwd, onClose }: Props) { const totalDel = data?.files.reduce((s, f) => s + f.deletions, 0) ?? 0 return ( -
+ {/* The shared side-panel header (UI pass, G-26). Its close had no accessible name and no focus style. */} @@ -195,7 +190,7 @@ export function GitBar({ cwd, onClose }: Props) { {!data && !error && (
loading…
)} -
+ ) } diff --git a/src/renderer/src/features/goal-loop/GoalLoopPane.tsx b/src/renderer/src/features/goal-loop/GoalLoopPane.tsx index 46c14b9f5..07b2f3d80 100644 --- a/src/renderer/src/features/goal-loop/GoalLoopPane.tsx +++ b/src/renderer/src/features/goal-loop/GoalLoopPane.tsx @@ -8,6 +8,7 @@ import type { GoalLoopControlAction, GoalLoopState } from '@shared/types/goalLoo import { dismissGoalLoop, useGoalLoopView } from './viewState' import { withVisibleControls } from '@shared/text/visibleControls' import { useSwapFocus } from '@renderer/lib/useSwapFocus' +import { LAYERS } from '@renderer/ui/layers' const PHASE_LABEL: Record = { active: 'active', paused: 'paused', ended: 'ended', @@ -79,7 +80,7 @@ function GoalLoopOverlay({ children, takeFocus }: { children: ReactNode; takeFoc data-goal-loop-active={takeFocus ? '' : undefined} role="dialog" aria-label="Agent goal loop" - className="absolute inset-0 z-50 bg-canvas text-ink" + className={`absolute inset-0 ${LAYERS.paneTakeover} bg-canvas text-ink`} onMouseDown={event => { event.preventDefault(); event.stopPropagation() }} onClick={event => event.stopPropagation()} onKeyDown={event => { @@ -204,7 +205,7 @@ export function GoalLoopPane({ sessionId, focused = true }: { sessionId: string; // also a genuine blocking surface, may do the same. const strip =
event.stopPropagation()} onClick={event => event.stopPropagation()} > diff --git a/src/renderer/src/features/path-picker/ui/PathInput.renderer.test.tsx b/src/renderer/src/features/path-picker/ui/PathInput.renderer.test.tsx index dc097b98f..b65a65b97 100644 --- a/src/renderer/src/features/path-picker/ui/PathInput.renderer.test.tsx +++ b/src/renderer/src/features/path-picker/ui/PathInput.renderer.test.tsx @@ -1,4 +1,4 @@ -import { act, cleanup, fireEvent, render, screen } from '@testing-library/react' +import { cleanup, fireEvent, render, screen } from '@testing-library/react' import { useState } from 'react' import { afterEach, describe, expect, it, vi } from 'vitest' @@ -31,9 +31,11 @@ describe('PathInput', () => { }, }) render() - await act(async () => { await new Promise(resolve => setTimeout(resolve, 100)) }) + // Wait for the suggestions themselves, not a 100 ms guess (#1107, review of #1377): the lookup + // runs after a 60 ms debounce, so a loaded runner could still be before it when a fixed sleep + // ended. findByRole polls until the listbox exists. + const listbox = await screen.findByRole('listbox') const field = screen.getByRole('combobox') - const listbox = screen.getByRole('listbox') expect(field).toHaveAttribute('aria-expanded', 'true') expect(field).toHaveAttribute('aria-controls', listbox.id) expect(field.getAttribute('aria-activedescendant')).toBe(screen.getAllByRole('option')[0]!.id) diff --git a/src/renderer/src/features/path-picker/ui/PathInput.tsx b/src/renderer/src/features/path-picker/ui/PathInput.tsx index 44153a5aa..033da4848 100644 --- a/src/renderer/src/features/path-picker/ui/PathInput.tsx +++ b/src/renderer/src/features/path-picker/ui/PathInput.tsx @@ -1,6 +1,7 @@ import { useCallback, useEffect, useMemo, useRef, useState, useId } from 'react' import type { CSSProperties } from 'react' import { containsInvisibleControls, withVisibleControls } from '@shared/text/visibleControls' +import { LAYERS } from '@renderer/ui/layers' // PathInput — the path picker's path-with-completion input. // @@ -284,7 +285,7 @@ export function PathInput({ id={listboxId} role="listbox" className={` - absolute left-0 right-0 top-full mt-1 z-50 + absolute left-0 right-0 top-full mt-1 ${LAYERS.inSurfacePopover} bg-popover-bg border border-popover-border rounded-float max-h-[280px] overflow-auto shadow-[0_8px_24px_var(--theme-shadow-color)] diff --git a/src/renderer/src/features/providers/ui/ProviderEnablementRow.renderer.test.tsx b/src/renderer/src/features/providers/ui/ProviderEnablementRow.renderer.test.tsx index 3291b0ece..5f892928b 100644 --- a/src/renderer/src/features/providers/ui/ProviderEnablementRow.renderer.test.tsx +++ b/src/renderer/src/features/providers/ui/ProviderEnablementRow.renderer.test.tsx @@ -62,6 +62,58 @@ describe('ProviderEnablementRow', () => { expect(setMock).toHaveBeenCalledWith('grok', true) }) + // #1250 row 6: a failed write was an unhandled rejection with nothing on + // screen. It is said, in fixed words (never the raw IPC text, which can + // carry a filesystem path: q22), and the switch keeps main's value. + it('says a failed toggle or reset did not save, without raw error text', async () => { + setMock.mockRejectedValueOnce(new Error("EACCES: permission denied, open '/Users/someone/.config/agent-code/setup.json'")) + await setup() + const grokSwitch = screen.getAllByRole('switch').find(el => el.getAttribute('aria-label') === 'Enable Grok')! + fireEvent.click(grokSwitch) + expect(await screen.findByRole('alert')).toHaveTextContent("Couldn't save this change. Nothing was changed.") + expect(document.body.textContent).not.toContain('EACCES') + expect(grokSwitch.getAttribute('aria-checked')).toBe('false') + + // A later toggle that saves clears the message (#1403 review b: a stale + // alert after a good retry was unpinned). + fireEvent.click(grokSwitch) + await vi.waitFor(() => expect(screen.queryByRole('alert')).toBeNull()) + }) + + // #1403 review b: this used to be checked after a failed toggle whose alert + // was still up, so a silent reset passed. Its own render, its own alert. + it('says a failed reset did not save, without raw error text', async () => { + resetMock.mockRejectedValueOnce(new Error('EROFS: read-only file system')) + await setup() + fireEvent.click(screen.getByText('Reset to Detection')) + expect(await screen.findByRole('alert')).toHaveTextContent("Couldn't save this change. Nothing was changed.") + expect(document.body.textContent).not.toContain('EROFS') + }) + + it('says a failed usage-source write did not save, without raw error text', async () => { + const usageMock = vi.fn() + .mockRejectedValueOnce(new Error("ENOSPC: no space left on device, write '/Users/someone/setup.json'")) + .mockResolvedValue({ ...snapshot, opencodeUsageSource: 'zai', zaiCredentialPresent: true }) + Object.defineProperty(window, 'api', { + configurable: true, + value: { providerEnablementSet: setMock, providerEnablementReset: resetMock, providerEnablementSetOpencodeUsageSource: usageMock }, + }) + const { ProviderEnablementRow } = await import('./ProviderEnablementRow') + const { useProviderEnablementStore } = await import('@renderer/features/providers/store') + useProviderEnablementStore.getState().setSnapshot({ + ...snapshot, + entries: snapshot.entries.map(entry => entry.kind === 'opencode' ? { ...entry, enabled: true } : entry), + zaiCredentialPresent: true, + }) + render() + fireEvent.change(screen.getByLabelText('OpenCode usage source'), { target: { value: 'zai' } }) + expect(await screen.findByText("Couldn't save this change. Nothing was changed.")).toBeTruthy() + expect(document.body.textContent).not.toContain('ENOSPC') + // A later choice that saves clears the message (#1403 review b). + fireEvent.change(screen.getByLabelText('OpenCode usage source'), { target: { value: 'zai' } }) + await vi.waitFor(() => expect(screen.queryByText("Couldn't save this change. Nothing was changed.")).toBeNull()) + }) + it('reset link appears only for user overrides and calls the API', async () => { await setup() const resetButtons = screen.getAllByText('Reset to Detection') diff --git a/src/renderer/src/features/providers/ui/ProviderEnablementRow.tsx b/src/renderer/src/features/providers/ui/ProviderEnablementRow.tsx index c392b38b9..67c5a776c 100644 --- a/src/renderer/src/features/providers/ui/ProviderEnablementRow.tsx +++ b/src/renderer/src/features/providers/ui/ProviderEnablementRow.tsx @@ -4,6 +4,7 @@ import { Select } from '@renderer/components/ui/select' import { getRendererProviderCapabilities } from '@providers/registry.renderer.capabilities' import { useProviderEnablementStore } from '@renderer/features/providers/store' +import { SETUP_WRITE_FAILED } from '@renderer/features/settings/setupWriteFailed' import { OPENCODE_USAGE_SOURCES, type OpencodeUsageSource, type ProviderEnablementEntry } from '@shared/types/providerEnablement' @@ -15,14 +16,21 @@ function hintFor(entry: ProviderEnablementEntry): string { function EntryRow({ entry }: { entry: ProviderEnablementEntry }) { const capabilities = getRendererProviderCapabilities(entry.kind) const [pending, setPending] = useState(false) + // A failed write used to be an unhandled rejection with nothing on screen + // (#1250 row 6): the switch simply did not move, or looked moved until the + // next launch. + const [failed, setFailed] = useState(false) const toggle = async () => { setPending(true) + setFailed(false) try { // The returned snapshot also arrives via the push channel; applying it // here too keeps the switch snappy instead of waiting a broadcast hop. const snapshot = await window.api.providerEnablementSet(entry.kind, !entry.enabled) useProviderEnablementStore.getState().setSnapshot(snapshot) + } catch { + setFailed(true) } finally { setPending(false) } @@ -30,9 +38,12 @@ function EntryRow({ entry }: { entry: ProviderEnablementEntry }) { const reset = async () => { setPending(true) + setFailed(false) try { const snapshot = await window.api.providerEnablementReset(entry.kind) useProviderEnablementStore.getState().setSnapshot(snapshot) + } catch { + setFailed(true) } finally { setPending(false) } @@ -42,6 +53,7 @@ function EntryRow({ entry }: { entry: ProviderEnablementEntry }) {
{capabilities.shortLabel}
+ {failed ?
{SETUP_WRITE_FAILED}
: null}
{hintFor(entry)} {entry.because === 'user' ? ( @@ -99,10 +111,12 @@ function OpencodeUsageSourceRow() { try { const next = await window.api.providerEnablementSetOpencodeUsageSource(value) useProviderEnablementStore.getState().setSnapshot(next) - } catch (error) { + } catch { // An IPC failure must surface, not become an unhandled renderer - // rejection with a silently unchanged dropdown (final review #7). - setChoiceError(error instanceof Error ? error.message : 'Could not save the usage source.') + // rejection with a silently unchanged dropdown (final review #7). In + // fixed words: the raw message of a failed write can carry a + // filesystem path (q22; #1250). + setChoiceError(SETUP_WRITE_FAILED) } finally { setPending(false) } } diff --git a/src/renderer/src/features/settings/setupWriteFailed.ts b/src/renderer/src/features/settings/setupWriteFailed.ts new file mode 100644 index 000000000..3b808b4e2 --- /dev/null +++ b/src/renderer/src/features/settings/setupWriteFailed.ts @@ -0,0 +1,20 @@ +// What a failed setup-state write says, wherever the renderer writes main's +// `setup.json` (#1250 rows 6 and 13, #1403 review b). +// +// WHY fixed text: the IPC error of a failed write can carry a filesystem path, +// and user-visible text is curated (q22). "Nothing was changed" is true +// because main applies each save to the last state known to be on disk and +// drops a save whose write failed (`updateSetupState`): neither main's state +// nor a later save carries it. Main rejects only when the write itself +// failed; a refresh that fails after a landed write resolves instead +// (`providerEnablement.mutate`). +export const SETUP_WRITE_FAILED = "Couldn't save this change. Nothing was changed." + +// The setup panel's answer (skip an optional tool, continue with no provider) +// closes the panel whatever happens (#1047: a panel that cannot be answered is +// a lockout), so its failure is said after the close, as a toast. +export const SETUP_ANSWER_NOT_SAVED = "Couldn't save your setup answer. Setup may ask again next launch." + +// The prerequisite check failed (an IPC error, a probe that could not run). +// Its raw error can name paths from the login shell. +export const SETUP_CHECK_FAILED = "Couldn't check which tools are installed." diff --git a/src/renderer/src/features/setup/firstRun.renderer.test.tsx b/src/renderer/src/features/setup/firstRun.renderer.test.tsx index b27d075fb..dfcc23560 100644 --- a/src/renderer/src/features/setup/firstRun.renderer.test.tsx +++ b/src/renderer/src/features/setup/firstRun.renderer.test.tsx @@ -13,6 +13,8 @@ import { import type { SessionSpawnOptions } from '@preload/api/types' import type { SetupCheckResult } from '@shared/types/setup' import type { CommandContext } from '@renderer/features/command-palette/types' +import { GlobalToastContext } from '@renderer/ui/GlobalToastContext' +import { SETUP_ANSWER_NOT_SAVED, SETUP_WRITE_FAILED } from '@renderer/features/settings/setupWriteFailed' // The first run, end to end in the renderer (#995 stage 6): the REAL // workspace hook with its REAL bootstrap, the REAL SetupGate and the setup @@ -32,9 +34,15 @@ vi.mock('@renderer/performance/client', () => ({ measure: (_name: string, fn: () => T | Promise) => fn(), })) +// What the desktop's GlobalToastProvider would have been asked to show. +const showToast = vi.fn() + const originalStore = useAppStore.getState() const originalApi = Object.getOwnPropertyDescriptor(window, 'api') -beforeEach(() => resetSetupStoreForTests()) +beforeEach(() => { + resetSetupStoreForTests() + showToast.mockReset() +}) afterEach(() => { cleanup() resetSetupStoreForTests() @@ -78,7 +86,7 @@ function mountMachine(checks: SetupCheckResult[], options: { failKinds?: string[ appendFeedDebugLog: async () => undefined, } }) const hook = renderHook(() => useWorkspace()) - render() + render() return { hook, spawnSession, setupCheck, api: window.api } } @@ -206,6 +214,50 @@ describe('the setup panel never strands the first run (#1047 review)', () => { fireEvent.click(await screen.findByRole('button', { name: 'Continue with a Terminal' })) await waitFor(() => expect(projects()).toBe(1)) expect(spawnedKinds(spawnSession)).toEqual(['terminal']) + // #1403 review b: the error used to land in the panel's alert just as the + // panel closed, so nobody saw that the answer was not saved. Said after + // the close, in fixed words (the raw error names a device or a path). + await waitFor(() => expect(showToast).toHaveBeenCalledWith(SETUP_ANSWER_NOT_SAVED)) + expect(JSON.stringify(showToast.mock.calls)).not.toContain('ENOSPC') + }) + + it('says an install failed in fixed words, never the raw IPC error', async () => { + // #1403 verification b (q22): Install's IPC rejects when the follow-up + // check fails, and its message can carry the state file's path; the + // dialog rendered it verbatim. + const check = withoutMachineWideInstalls(loadFirstRunCheck('clean-machine')) + const installable: SetupCheckResult = { + ...check, + tools: { ...check.tools, mitmdump: { ...check.tools.mitmdump, found: false, path: null, source: undefined, installable: true, skipped: false } }, + } + const { api } = mountMachine([installable]) + Object.assign(api, { + setupInstall: vi.fn(async () => { throw new Error("EACCES: permission denied, open '/Users/someone/Library/Application Support/agent-code/setup.json'") }), + }) + fireEvent.click(await screen.findByRole('button', { name: 'Install' })) + expect(await screen.findByText('Could not install mitmproxy.')).toBeTruthy() + expect(screen.queryByText(/EACCES|setup\.json/)).toBeNull() + fireEvent.click(screen.getByRole('button', { name: 'Continue with a Terminal' })) + await waitFor(() => expect(projects()).toBe(1)) + }) + + it('says a manual path was not saved in fixed words, never the raw write error', async () => { + // #1403 review b (q22): a failed setup.json write rejects the IPC with a + // message that can carry the state directory's path; it was rendered + // verbatim under the path field. + const { api } = mountMachine([withoutMachineWideInstalls(loadFirstRunCheck('clean-machine'))]) + Object.assign(api, { + setupSetToolPath: vi.fn(async () => { throw new Error("EACCES: permission denied, open '/Users/someone/Library/Application Support/agent-code/setup.json'") }), + }) + fireEvent.click((await screen.findAllByRole('button', { name: 'Enter Path Manually…' }))[0]!) + const [field] = screen.getAllByRole('textbox') + fireEvent.change(field!, { target: { value: '/opt/bin/claude' } }) + fireEvent.click(screen.getByRole('button', { name: 'Set' })) + expect(await screen.findByText(SETUP_WRITE_FAILED)).toBeTruthy() + expect(screen.queryByText(/EACCES|setup\.json/)).toBeNull() + // Answer the panel so the parked bootstrap does not leak into the next test. + fireEvent.click(screen.getByRole('button', { name: 'Continue with a Terminal' })) + await waitFor(() => expect(projects()).toBe(1)) }) it('records the provider-less answer, so the panel stops opening by itself', async () => { diff --git a/src/renderer/src/features/setup/store.ts b/src/renderer/src/features/setup/store.ts index efab5b3d5..bcdffa1e6 100644 --- a/src/renderer/src/features/setup/store.ts +++ b/src/renderer/src/features/setup/store.ts @@ -3,6 +3,7 @@ import { create } from 'zustand' import { AGENT_PROVIDER_KINDS, DEFAULT_PROVIDER, type AgentProviderKind } from '@shared/types/providerKind' import type { SetupCheckResult } from '@shared/types/setup' +import { SETUP_CHECK_FAILED } from '@renderer/features/settings/setupWriteFailed' /** * The renderer's one copy of the setup check (#995). @@ -74,7 +75,10 @@ export function refreshSetupCheck(): Promise { .then(() => window.api.setupCheck()) .then(check => { store.setCheck(check); return check }) .catch((err: unknown) => { - store.setError(err instanceof Error ? err.message : String(err)) + // Fixed text on screen (q22, #1403 review b): the raw error can name + // login-shell paths. The detail stays in the console for debugging. + console.warn('[setup] prerequisite check failed:', err) + store.setError(SETUP_CHECK_FAILED) return null }) .finally(() => { inFlight = null }) diff --git a/src/renderer/src/features/setup/ui/SetupGate.tsx b/src/renderer/src/features/setup/ui/SetupGate.tsx index 960e4495e..422931743 100644 --- a/src/renderer/src/features/setup/ui/SetupGate.tsx +++ b/src/renderer/src/features/setup/ui/SetupGate.tsx @@ -13,6 +13,8 @@ import { DialogHeader, DialogTitle, } from '@renderer/components/ui/dialog' +import { SETUP_ANSWER_NOT_SAVED, SETUP_WRITE_FAILED } from '@renderer/features/settings/setupWriteFailed' +import { useGlobalToast } from '@renderer/ui/GlobalToastContext' import { Button } from '@renderer/components/ui/button' import { DialogActions } from '@renderer/components/ui/dialog-actions' import { Input } from '@renderer/components/ui/input' @@ -50,6 +52,7 @@ export function SetupGate() { const panelRef = useRef(null) const [busy, setBusy] = useState('check') const [actionError, setActionError] = useState(null) + const { showToast } = useGlobalToast() const refresh = useCallback(async () => { setBusy('check') @@ -103,8 +106,11 @@ export function SetupGate() { const result = await window.api.setupInstall(target) useSetupStore.getState().setCheck(result.check) if (!result.ok) setActionError(result.output || `Could not install ${target}.`) - } catch (err) { - setActionError(err instanceof Error ? err.message : String(err)) + } catch { + // Fixed words (q22, #1403 verification b): a rejection here is an IPC + // or filesystem error, whose message can carry the state file's path. + // `result.output` above is the installer's own report. + setActionError(`Could not install ${target}.`) } finally { setBusy(null) } @@ -123,8 +129,11 @@ export function SetupGate() { if (!result.ok) return result.reason useSetupStore.getState().setCheck(result.check) return null - } catch (err) { - return err instanceof Error ? err.message : String(err) + } catch { + // A rejection here is main failing to record the path (its setup.json + // write), and its message can carry a filesystem path (q22, #1403 + // review b). `result.reason` above is main's own curated refusal. + return SETUP_WRITE_FAILED } finally { setBusy(null) } @@ -152,6 +161,7 @@ export function SetupGate() { ? missingOptional.filter(tool => tool.installable && !tool.skipped) : [] setBusy('check') + let answerNotSaved = false try { for (const tool of skippedTools) { useSetupStore.getState().setCheck(await window.api.setupSkipOptional(tool.id)) @@ -161,13 +171,18 @@ export function SetupGate() { if (automatic && noProvider) { useSetupStore.getState().setCheck(await window.api.setupAcknowledgeNoProviders()) } - } catch (err) { - setActionError(err instanceof Error ? err.message : String(err)) + } catch { + // WHY a toast (#1403 review b): the error used to go into the panel's + // own alert, and the `finally` below closed the panel in the same + // tick, so nobody ever saw that the answer was not saved. The close + // must stay (see above); the message goes where it survives it. + answerNotSaved = true } finally { setBusy(null) useSetupStore.getState().close() } - }, [automatic, missingOptional, noProvider]) + if (answerNotSaved) showToast(SETUP_ANSWER_NOT_SAVED) + }, [automatic, missingOptional, noProvider, showToast]) if (!shouldShow || !check) return null diff --git a/src/renderer/src/features/tldr/TldrOverlay.tsx b/src/renderer/src/features/tldr/TldrOverlay.tsx index dcde1b5cc..c530569eb 100644 --- a/src/renderer/src/features/tldr/TldrOverlay.tsx +++ b/src/renderer/src/features/tldr/TldrOverlay.tsx @@ -6,6 +6,7 @@ import { TldrFreshness } from './TldrFreshness' import { isPreviewVisible, useTldrView } from './viewState' import { useAgentTerminalOwnerVisible } from '@renderer/workspace/terminal/AgentTerminalOwnership' import type { PreviewKind } from './viewState' +import { LAYERS } from '@renderer/ui/layers' // What differs between the TLDR and Goal peeks is only where the text comes // from and what it is called. Sharing the overlay keeps the subscription race @@ -101,7 +102,7 @@ export function TldrOverlay({ kind = 'tldr', identity, enabled, runtime, provide // follows the active theme (light themes included) and reads in the same // canvas and ink as the rest of the UI. Opaque canvas keeps the pane's own // text from bleeding through behind the summary. - className="absolute inset-0 z-50 bg-canvas text-center text-ink" + className={`absolute inset-0 ${LAYERS.paneTakeover} bg-canvas text-center text-ink`} onMouseDown={event => { event.preventDefault(); event.stopPropagation() }} onClick={event => event.stopPropagation()} > diff --git a/src/renderer/src/features/voice-dictation/ui/VoiceDictationOverlay.tsx b/src/renderer/src/features/voice-dictation/ui/VoiceDictationOverlay.tsx index 1678a8611..ec1a5f95f 100644 --- a/src/renderer/src/features/voice-dictation/ui/VoiceDictationOverlay.tsx +++ b/src/renderer/src/features/voice-dictation/ui/VoiceDictationOverlay.tsx @@ -1,4 +1,5 @@ import { useDictationOverlayState } from '@renderer/features/voice-dictation/dictationStatusStore' +import { LAYERS } from '@renderer/ui/layers' // VoiceDictationOverlay — terminal-mode floating chip rendered at App root. // Rendered Agent mode keeps the old inline composer mic affordance; only @@ -53,7 +54,7 @@ export function VoiceDictationOverlay() { return (
{ diff --git a/src/renderer/src/features/worktrees/ui/WorktreesBar.tsx b/src/renderer/src/features/worktrees/ui/WorktreesBar.tsx index 459ab8c9e..c9aa817bd 100644 --- a/src/renderer/src/features/worktrees/ui/WorktreesBar.tsx +++ b/src/renderer/src/features/worktrees/ui/WorktreesBar.tsx @@ -11,6 +11,7 @@ import type { } from '@renderer/features/worktrees/lib/loadWorktreeDump' import { worktreeColorForIdentity } from '@renderer/workspace/tile-tree/TileLeaf/worktreeBadgeColor' import type { Workspace } from '@renderer/workspace/workspaceStore' +import { SidePanel } from '@renderer/components/ui/side-panel' type Props = { cwd: string | null @@ -231,7 +232,7 @@ export function WorktreesBar({ cwd, workspace, onClose }: Props) { }, [rows]) return ( -
+ {/* The shared side-panel header (UI pass, G-26): Title Case ghost actions instead of lowercase text links, and a named close (the × had no accessible name). */} @@ -295,7 +296,7 @@ export function WorktreesBar({ cwd, workspace, onClose }: Props) { ))}
)} -
+ ) } diff --git a/src/renderer/src/rendering/evidence/bundleShapeSweep.ts b/src/renderer/src/rendering/evidence/bundleShapeSweep.ts index 52c28f7cd..413aaaf65 100644 --- a/src/renderer/src/rendering/evidence/bundleShapeSweep.ts +++ b/src/renderer/src/rendering/evidence/bundleShapeSweep.ts @@ -240,10 +240,19 @@ export function sweepCuratedShapeFixture( } const transcriptEntry = asRecord(carrier.transcriptEntry) if (transcriptEntry && typeof transcriptEntry.type === 'string') { + // Same event-type rule as the bundle sweep above (EntryRow's + // `type:subtype` for non-conversation entries). The curated path used + // the bare type, which no fixture exercised until the first curated + // system entry (Codex's compact boundary, #1289); a catalog entry for + // `system:compact_boundary` then could never match its own fixture. + const isConversation = transcriptEntry.type === 'user' || transcriptEntry.type === 'assistant' + const subtype = !isConversation && typeof transcriptEntry.subtype === 'string' + ? `:${transcriptEntry.subtype}` + : '' observe( 'transcript-entry', 'durable', - transcriptEntry.type, + `${transcriptEntry.type}${subtype}`, transcriptEntry, ) } diff --git a/src/renderer/src/session-runtime/activity.ts b/src/renderer/src/session-runtime/activity.ts index 9315740a4..728a9278a 100644 --- a/src/renderer/src/session-runtime/activity.ts +++ b/src/renderer/src/session-runtime/activity.ts @@ -42,7 +42,10 @@ export type SessionActivity = { * * There is no "system record" case, though an earlier draft claimed one: the * transcript mapper drops every system/meta record before it reaches - * `entries`, and compact boundaries carry no timestamp at all. + * `entries`, and Claude's compact boundaries carry no timestamp at all. (A + * Codex compact boundary does carry one since #1386; the tail fallback reads + * the newest entry of any type, so a compaction counts as transcript time, + * which is correct: it is a real committed record.) * * The footer's rule is the one kept, because it includes the ingest * watermark: `lastJsonlEntryAt` is the newest thing the transcript reader has diff --git a/src/renderer/src/session-runtime/ingest/committedRecords.ts b/src/renderer/src/session-runtime/ingest/committedRecords.ts index 3d8bd4a95..72a0f6f9d 100644 --- a/src/renderer/src/session-runtime/ingest/committedRecords.ts +++ b/src/renderer/src/session-runtime/ingest/committedRecords.ts @@ -143,8 +143,10 @@ export function isPaginationAnchor(mapped: readonly Entry[], marker: string | nu * ghost `_atp.updatedAt`, and the ownership ledger's collapsed-running rule * compares it with the live turn — both are "when the producer observed * this", so a resumed session compares yesterday against yesterday. Entries - * without a usable `timestamp` (compact boundaries, queue ops) leave the + * without a usable `timestamp` (Claude compact boundaries, queue ops) leave the * cursor where it was: they are not evidence the committed channel is alive. + * A Codex compact boundary carries its rollout line's timestamp (#1386), so it + * does advance the cursor, correctly: the `compacted` line was committed. * * Shared since #1177: the phone used to hand the ledger a constant 0, so the * collapsed-running rule could never fire there (ARCHITECTURE §8.3). diff --git a/src/renderer/src/ui/GlobalToast.tsx b/src/renderer/src/ui/GlobalToast.tsx index cb9b671fc..86fb6dae8 100644 --- a/src/renderer/src/ui/GlobalToast.tsx +++ b/src/renderer/src/ui/GlobalToast.tsx @@ -3,6 +3,7 @@ import { useCallback, useEffect, useRef, useState } from 'react' import { useAppStore } from '@renderer/app-state/hooks' import { managedSkillsUnavailableMessage } from '@shared/types/tldr' import { GlobalToastContext } from '@renderer/ui/GlobalToastContext' +import { LAYERS } from '@renderer/ui/layers' // GlobalToast — app-wide toast system rendered in the top-right corner. // @@ -146,7 +147,7 @@ export function GlobalToastProvider({ children }: { children: React.ReactNode }) The focus ring's offset gap is what makes it visible on the accent fill, because the built-in focus ring IS the accent (Claude review of #1221, reviewer C F1). */} -
+
{toast && (