diff --git a/docs/plans/2026-09-26-proxy-events-retention.md b/docs/plans/2026-09-26-proxy-events-retention.md new file mode 100644 index 000000000..aeec6fcc1 --- /dev/null +++ b/docs/plans/2026-09-26-proxy-events-retention.md @@ -0,0 +1,101 @@ +# Bound a live Claude session's proxy-events.jsonl (#1273 residual) + +## Problem + +#1284 / claude-code-headless#62 stopped writing request bodies past 256 MiB per events file. What the +issue still has open: + +1. The file is still unbounded. Responses, stream chunks and body-less request records keep + appending — about 7 % of the old byte rate, ~170 MB per 2.4 GB of old-rate traffic — for as long + as the session lives. +2. A live file is never reclaimed: `debugRetention.ts` skips any run written in the last 10 minutes + (`ACTIVE_GRACE_MS`), and a session that stays open for days keeps its file "live" all that time. + +## Evidence + +- Composition of a real post-#62 events file (owner's machine, run + `…/resume-fa48baff…/2026-09-25T17-34-30-657Z`): `request` lines 98.1 % (bodies until the budget), + `response-chunk` 1.7 %, `response` 0.2 %. Past the budget, chunks/responses/body-less requests are + what keeps growing. +- The file is not just a log: it is the **transport** from mitmdump to the app. `ProxyServer` + polls it every 200 ms from a byte offset and emits each complete line as a live event; the + adapter builds the transcript view from the `response-chunk` lines. So "stop writing chunks past a + budget" would break the live view, and truncating in place would drop or duplicate events. + +## Decisions (defaults) + +- **Rotate in the addon, keep one previous generation.** When the events file reaches + `PROXY_EVENTS_ROTATE_BYTES` (default 512 MiB), the addon renames it to `proxy-events.1.jsonl` + (atomically replacing the older generation) and the next write starts a fresh + `proxy-events.jsonl`. The **events file** therefore holds at most ~2 × 512 MiB instead of growing + for the life of the session. The run directory is NOT fully bounded by this change: the latest-body + sidecar is ~21.3 MiB on disk (16 MiB raw, base64; briefly twice that during its atomic replace), and + `sslkeylog.log` still grows without bound (#1380). See the review outcome under Delivery. Rotation failure is non-fatal: the addon + keeps appending (forensics must never disturb the proxy). + - WHY 512 MiB and one generation: the body budget (256 MiB) still applies per file, so each + generation keeps ~100 turns of bodies plus a long stretch of body-less traffic; the debug bundle + only ever ships the last 5 MiB. Deleting generation 2 is the point of the change. + - WHY rotate on the writer side: mitmdump is the single writer, runs single-threaded, and writes + each line with open-append-close, so after `os.replace` no write can land in the old inode. + Rotating from the app would race the writer. +- **The tail holds the generation it reads open** (revised after review of claude-code-headless#64, + steering q53). + - The first version stat()ed the path and later open()ed it by name. A rotation between the two made + it read the new file with the old offset; reviewers reproduced lost and duplicated events. + - `EventsFileTail` now keeps a `FileHandle` on the current generation. Its size comes from `fstat` on + that handle, and rotation is detected when the *path's* inode changes. + - On rotation, the tail finishes the held generation, **opens the live file first**, and then reads + whole any unseen generation at `.1` before the live one. Anything between the two held handles can + only be at `.1`. + - Pinned by a test that rotates before every path-level await point. +- **Gap policy: a bounded, reported gap, not an acknowledgement protocol** (q53 asked us to choose). + - Each generation created by a rotation carries its number in a header line, `{"kind":"generation","generation":n}`, created atomically with the live file. Generation 0 (the run's first file) has none; the tail reads a missing header as generation 0 and strips the line when present. + - The bound holds under the default `PROXY_EVENTS_ROTATE_BYTES`; setting it to 0 (the forensic override) disables rotation. + - This replaced a first design with a `proxy-events.rotations` counter file, which a reader could pair with the wrong generation (round 2 of #64). + - When the poller stalls through more rotations than it can hold or drain (about 1 GiB of traffic at the default), the generations deleted unread are counted as `lostGenerations`. `ProxyServer` surfaces them as a `transport-gap` event plus one warning. + - An ack protocol would need a second writer in the app. A stalled or dead app would then let the proxy's disk grow without bound again, which is this issue. +- **Known limitation** (accepted by the manager under the final-pass cap, stated in claude-code-headless#64): + - A process crash between renaming the live file to `.1` and publishing the next header is repaired when the addon restarts (`2218918`). + - If the addon is never restarted, the rotated generation's unread events are not delivered and **not** reported as a gap. +- **Addon hardening** (same review): + - `_write` never raises out of a mitmproxy hook, since the stream tap carries the user's live + response; + - a crashed partial line is terminated at startup, so the next event is not glued to it; + - the live file is recreated in its own step. +- **Debug bundle reader: owned by W1 in #1332** (steering q54). This branch's own `readEventsTail` + change was reverted (`0a462fad`). #1332 makes one provider-neutral, rotation-safe reader: one + handle, `bytesRead` honoured, filling from `.1`. This branch merges main after #1332 and keeps only + Claude-specific wiring, if any is still needed. +- **Retention: one change.** Run detection now also counts a directory holding only + `proxy-events.1.jsonl` (found in review). With rotation the events file is bounded, so the 10-minute + grace no longer lets one session's events fill the disk (`sslkeylog.log` is #1380); old oversized + files from before this change age out normally. + +## Tests + +- Package, real addon + real tailer (see the PR body for the final list, incl. the per-await-point + rotation cases and the ProxyServer wiring test): drive the real `mitmAddon.py` (`request` / `response` / + chunk hooks, a small `PROXY_EVENTS_ROTATE_BYTES`) through several rotations while an + `EventsFileTail` polls between writes; assert every event is emitted exactly once and in order, + only one previous generation exists, and the live file stays under the threshold + one line. + Fail-first: without rotation the file exceeds the bound; without the poller's rotation handling, + events written between the last poll and the rename are lost (asserted by name). +- Poll ordering: rename observed while the new file is still absent, and a rotation that happens + while the old generation ended mid-poll — both orders pinned. +- App (added after the #1376 review): `claudeSession.suspension.test.ts` pins that both the `event` + and `transport-gap` channels are forwarded and detached; `sessionManager.proxyGap.test.ts` pins the + `claude.proxy_transport_gap` incident, its re-emit, and that a replaced session's late gap is + ignored; `debugRetention.test.ts` pins that a run holding only `proxy-events.1.jsonl` is counted. + The bundle reader's own tests live in W1's #1332 (q54). + +## Delivery + +claude-code-headless#64 (addon + tailer): three reviews, verification, and a final round 3, MERGED (`f52fc82`). This agent-code PR bumps the pointer and, after its review, adds the app-side +wiring below (gap forwarding and incident, `.1`-only retention); it `Refs #1273` rather than fixing it. #1332 (W1's rotation-safe bundle reader) is already on main. +- **No lockfile resync is needed.** The app consumes claude-code-headless from source (tsconfig and Vite aliases), not as a `file:` dependency, and its runtime dependencies (`chokidar`, `@xterm/headless`) are already root dependencies. The bump changes only the package's own devDependencies; `npm install --package-lock-only` leaves `package-lock.json` unchanged. +- **The generation header** is one more JSON line with an unknown `kind` to the app's only direct reader (the bundle reader, which ships raw bytes). The addon recreates `proxy-events.jsonl` immediately, so `debugRetention`'s run detection is unaffected. +- **Review of #1376 (a, b, c):** + - **The app dropped the gap signal.** `ClaudeSession` subscribed to the proxy's `event` channel only, so `transport-gap` never left the package. It now subscribes to both (`attachProxyServer`/`detachProxyServer`) and re-emits `proxy-transport-gap`. `SessionManager` records a `claude.proxy_transport_gap` incident and re-emits it with the session id. This is diagnostic only (steering q87): no missing-span marker is rendered yet, so the user-visible gap is #1381. The normal `event` wiring is now pinned too; deleting it used to pass every test. + - **A run holding only `proxy-events.1.jsonl` was invisible to retention.** `collectProxyRunDirs` now counts it. + - **`sslkeylog.log` also grows without bound** in every live run directory, and holds TLS secrets. This is outside this bump, so it is filed as #1380. The PR says `Refs #1273`, not `Fixes`: the events file is bounded, but a live run directory is not fully bounded until #1380 lands. + - **The latest-body sidecar** is ≤ 16 MiB raw, about 21.3 MiB on disk after base64 encoding. During the atomic replace, twice that is briefly possible. diff --git a/docs/plans/2026-09-27-extension-runtime-harness-egress.md b/docs/plans/2026-09-27-extension-runtime-harness-egress.md new file mode 100644 index 000000000..71e1cfc20 --- /dev/null +++ b/docs/plans/2026-09-27-extension-runtime-harness-egress.md @@ -0,0 +1,24 @@ +# Extension runtime harness: count only runtime egress (#1187) + +## Evidence +- **Reproduced locally under CPU load:** 2 of 10 runs of `node scripts/check-extension-frames.mjs --runtime-only` failed with exactly the issue's output. +- **Temporary request logging on the harness's egress server** showed one `GET /` in EVERY run, failing or passing, about 1.6 s after startup. Its headers were `user-agent: node`, `accept-language: *` and `sec-fetch-mode: cors`, which is Node's undici `fetch`, not the extension renderer. +- **Source:** the running Agent Code app's browser-pocket `LanePortWatcher` (`src/main/browserPocket/lanePortsIo.ts:45`, `probe(port)`) walks each agent lane's process tree, finds listening sockets with `lsof`, and probes them with `GET /`. The test ran inside an agent lane, so the harness's server is in that tree. Normally the probe lands after the egress assertion; under load it lands before. +- **The other two log lines are not the bug.** The `ZodError ... "extensionId"` is the harness's deliberate forgery probe (`runtimeHarness.ts`, the `sandbox` command expects `spoofDenied: true`). `No handler registered for 'extensions:runtime-api'` also appears twice in every passing run during disposal. + +## Change +Every request that reaches the harness's egress server counts as runtime egress EXCEPT the watcher's exact shape, `GET /`, which is recorded and ignored. + +**Superseded first version:** it counted only `/private-state` and ignored every other path. Review b showed that this would miss a leak to any other path (`/leak`, encoded or query variants). + +**Positive control:** right after binding, main fetches both `/private-state` and `/egress-control`, and both must be counted before the journey starts (review c). + +## Verification +- **Under load:** 0 of 10 runs fail with the fix; 2 of 10 failed before it. +- **Mutations, each failing the run:** + - an "ignore everything" classifier fails the control; + - excusing `/private-state` fails the control; + - a main-side `GET /leak` after the control fails the egress assertion; + - removing `will-navigate`, `will-frame-navigate` and the `webRequest` block fails the egress assertion. + +**Residual:** a runtime escape that requests exactly `GET /` is excused. The renderer probes all target `/private-state`. diff --git a/docs/plans/2026-09-27-load-older-history-failure.md b/docs/plans/2026-09-27-load-older-history-failure.md new file mode 100644 index 000000000..9803a0a96 --- /dev/null +++ b/docs/plans/2026-09-27-load-older-history-failure.md @@ -0,0 +1,52 @@ +# A failed "load older history" is said, not just un-spun (#1250 row 12) + +Short plan: a bug with a known root cause. The row comes from `temp/quality-loop/hunt-c3.md` (row 12, P3). #1250 is a batch issue, so this PR is `Refs #1250`. + +## Outcome +Scrolling to the top of an agent's feed pages in older history. When that page fails to load (an IPC error, or an unreadable transcript), the user is told, and learns how to retry. Today the catch in `useHistoryActions.loadOlderHistory` only clears `loadingOlderHistory`. The feed looks as if it simply has nothing older, although `hasOlderHistory` is still true. + +## Root cause (verified in source, origin/main) +- `src/renderer/src/workspace/hook/actions/history.ts`: on catch, it records a perf failure, warns to the console, and patches `loadingOlderHistory: false`. It returns `void` whatever happened, so no caller can tell a failure from a load. +- `TileLeaf.loadOlderHistory` awaits it and discards the result. `Feed` retries only on a NEW scroll event within 160 px of the top. + +## Design (contract) +- **`loadOlderHistory(sessionId): Promise<'loaded' | 'skipped' | 'failed'>`** (`OlderHistoryLoadResult`, exported from `history.ts`). + - `failed` only from the catch; `skipped` from every early return; `loaded` after the merge. +- **`TileLeaf`** shows a pane toast on `failed`: "Couldn't load older messages. Scroll up again to retry." + - Fixed text (q22): the error can carry a transcript path. + - Coalesced to one per 5 s per pane. While the scroller stays near the top, every scroll tick retries, and each failure must not stack another toast. + - It is a PANE toast because the failure belongs to this feed. +- `AgentFeed` and `Feed` keep `onLoadOlderHistory: () => Promise`. The phone mounts the same `AgentFeed` and is untouched. + +## Tests +- **`history.renderer.test.tsx`:** a rejecting `loadOlderHistory` IPC gives `failed` and leaves `hasOlderHistory` true (a retry stays possible). A successful page gives `loaded`; a missing marker gives `skipped`. Red on main: the result is `undefined`. +- **New `TileLeaf.olderHistory.renderer.test.tsx`:** with Feed probed for its `onLoadOlderHistory` prop and `workspace.loadOlderHistory` answering `failed`, the pane toast shows the fixed text once across two quick failures, and not at all on `loaded`. Red on main: no toast. + +## Verification boundary +Renderer tests drive the real hook and the real TileLeaf. The app is not launched. + +## Out of scope +- #1250's other rows. +- A persistent inline "retry" affordance in the feed header (a design call; the toast plus the existing scroll retry is enough to end the silence). + +## Steering q106 +- The dispatch layout re-renders the SAME TileLeaf with another agent's `sessionId` when a lane switches. One timestamp per mounted leaf therefore let agent A's toast silence agent B's first failure for 5 s. +- The cooldown is now keyed by `sessionId`. Test: same-leaf rerender, A fails and toasts, the lane switches to B, B fails and toasts, and B's repeat still coalesces. It fails with the per-leaf timestamp. + +## Review round 1 (a, b: FIX-BEFORE-MERGE) +- **a (Major): an unreadable transcript was paged as an empty page with `hasMore: false`.** The main-side `readOlderTranscriptWindow` swallowed stat/open/read failures, so the renderer dropped "older history exists" and returned `loaded`. + - **Ruling:** this reader serves only older paging, so a failure now rejects. The page is reported `failed` and stays retryable. An empty file (stat succeeded, size 0) is still an honest empty page, and the initial-chunk reader is unchanged. + - Test in `historyLoader.test.ts`: the first page loads, the file is removed, the older page rejects with ENOENT, and an empty file stays empty. It fails on the old loader. + - This landed in `e905104e`, whose message names only q106. +- **a, b (Major): "Scroll up again to retry" was impossible at the top.** At `scrollTop` 0 an upward gesture fires no scroll event, and Feed triggered only on scroll. An upward wheel at the top now makes the same request. + - Test: the real Feed, two upward wheels at 0, two requests (red on the old Feed). A downward wheel makes none. It also kills a's survivor (`loadingOlderRef` left true). +- **b (Major): the cooldown was shared across agents in one lane.** This is q106, fixed. +- **b (Minor): the raw error still reaches `console.warn` and the perf span.** Declined: these are developer diagnostics, not user-visible text (q22 governs what the user sees). The perf journal already records file paths on this path by design (`finishOlderChunk`). +- **c (MERGE-READY; four test gaps, each pinned):** + - every early return answers `skipped`; + - a retry 1 s into the window stays suppressed; + - the toast text is asserted literally; + - the toast goes to this pane. + c's mutations M7 (no-older → failed) and T3 (5 s → 500 ms) now fail. +- **Verification b (Major): the touch form of the gesture was still missing.** A downward finger drag at the top now makes the same request. Test with the real Feed: an upward drag makes none, a downward one makes one. It fails on the previous Feed. +- **Verification a (Major): a Codex rollout the resolver could no longer find still made an older page an empty `hasMore: false`.** `loadOlderHistoryChunk` returned that before reaching the now-strict reader. An older page with an unresolvable transcript now rejects. The perf span fails, and the phone's `RemoteServer` already turns a throw into a structured `ok: false`. Test in `historyLoader.providerOwned.test.ts` (Codex, resolver returns null), red on the previous loader. diff --git a/packages/claude-code-headless b/packages/claude-code-headless index 728cb4945..f52fc82d9 160000 --- a/packages/claude-code-headless +++ b/packages/claude-code-headless @@ -1 +1 @@ -Subproject commit 728cb49454fb31c9a57b8927d9a4130c0d9c5f4e +Subproject commit f52fc82d9d185314992cd845dafce42a723b2248 diff --git a/src/main/extensions/testing/runtimeHarness.ts b/src/main/extensions/testing/runtimeHarness.ts index 5901bf133..0a2a0534c 100644 --- a/src/main/extensions/testing/runtimeHarness.ts +++ b/src/main/extensions/testing/runtimeHarness.ts @@ -52,12 +52,43 @@ async function until(predicate: () => Promise, label: string): Promise< void (async () => { await app.whenReady() + // Every request that reaches this server is runtime egress EXCEPT one exact + // shape: `GET /` (#1187). When the suite runs inside an Agent Code agent + // lane, the host app's browser-pocket LanePortWatcher finds this listener in + // the lane's process tree and probes it once with `GET /` + // (src/main/browserPocket/lanePortsIo.ts, Node fetch). Under load that probe + // landed before the assertion below and failed the run with `['/']`, which + // the issue read as a runtime-API race; request logging showed it in every + // local run, about 1.6 s after startup, and CI has no watching app. + // + // WHY ignore exactly `GET /` and not "anything but the probed path" (review + // b): a regression that leaked to any other path (`/leak`, an encoded or + // query variant) must still fail. Only the watcher's shape is excused, and + // the positive control below proves a non-probe path is counted. Residual: a + // runtime escape that requests exactly `GET /` would be excused too; the + // renderer probes all target EGRESS_PATH, so that would need a new probe. + const EGRESS_PATH = '/private-state' const hits: string[] = [] - const server = createServer((request, response) => { hits.push(request.url ?? ''); response.end('unexpected egress') }) + const ignoredWatcherProbes: string[] = [] + const server = createServer((request, response) => { + const url = request.url ?? '' + if (request.method === 'GET' && url === '/') ignoredWatcherProbes.push(`${request.headers['user-agent'] ?? ''}`) + else hits.push(url) + response.end('unexpected egress') + }) await new Promise((resolve, reject) => { server.once('error', reject); server.listen(0, '127.0.0.1', resolve) }) const address = server.address() assert.ok(address && typeof address !== 'string') - const endpoint = `http://127.0.0.1:${address.port}/private-state` + const endpoint = `http://127.0.0.1:${address.port}${EGRESS_PATH}` + // Positive control: a non-probe path IS counted (review b's surviving + // mutation replaced the classifier and the journey stayed green). + // Both the path every renderer probe uses and an arbitrary one (review c: + // a regression that excused EGRESS_PATH itself kept an /egress-control-only + // control green while blinding the real check). + await fetch(endpoint).then(response => response.text()) + await fetch(`http://127.0.0.1:${address.port}/egress-control`).then(response => response.text()) + assert.deepEqual(hits, [EGRESS_PATH, '/egress-control'], 'the egress server must count the probed path and any other non-watcher path') + hits.length = 0 const statuses: RuntimeStatus[] = [] const lifecycle: string[] = [] const projectRoot = join(root!, 'project') @@ -228,7 +259,7 @@ void (async () => { await command('navigate') await new Promise(resolve => setTimeout(resolve, 100)) assert.equal(object(await command('snapshot')).activations, 1, 'renderer navigation must be refused') - assert.deepEqual(hits, []) + assert.deepEqual(hits, [], `runtime egress reached the server (ignored watcher probes: ${ignoredWatcherProbes.length})`) console.log('PASS runtime: command errors, host/preload isolation, forged namespaces and direct network/navigation denial') await views.attach(66, 'update-view', 'engine', revision, 'engine.main') diff --git a/src/main/incident/journalTypes.ts b/src/main/incident/journalTypes.ts index 3c1dd640c..4dedb304f 100644 --- a/src/main/incident/journalTypes.ts +++ b/src/main/incident/journalTypes.ts @@ -120,6 +120,7 @@ export type AppRunIncidentKind = // the child was adopted — but it ran work nobody was waiting on, so the // timeout budget or the renderer's responsiveness is worth looking at (#926). | 'orchestration.late_response_adopted' + | 'claude.proxy_transport_gap' | 'orchestration.prompt_delivery_failed' | 'mcp.host_start_failed' // Remote mobile companion (src/main/remote/) — declared here because this diff --git a/src/main/sessionManager.proxyGap.test.ts b/src/main/sessionManager.proxyGap.test.ts new file mode 100644 index 000000000..2be1d10fd --- /dev/null +++ b/src/main/sessionManager.proxyGap.test.ts @@ -0,0 +1,111 @@ +import { EventEmitter } from 'node:events' +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const { createSession, createTerminalSession } = vi.hoisted(() => ({ + createSession: vi.fn(), + createTerminalSession: vi.fn(), +})) + +vi.mock('@main/workspaceDirectory.js', () => ({ + // These suites spawn into synthetic paths ('/tmp/project', '/recorded/worktree') + // that intentionally do not exist on disk. The real spawn-path guard stats the + // cwd, so it is stubbed here; workspaceDirectory.test.ts covers the guard + // itself, and the missing-folder case below overrides this mock to prove the + // manager surfaces it. + MissingWorkspaceDirectoryError: class MissingWorkspaceDirectoryError extends Error { + constructor(readonly cwd: string) { + super(`Workspace folder is missing: ${cwd}`) + this.name = 'MissingWorkspaceDirectoryError' + } + }, + assertWorkspaceDirectoryExists: vi.fn(async () => {}), +})) + +vi.mock('@providers/registry.main.js', () => ({ + getMainProvider: () => ({ + name: 'Claude', + createSession, + createTerminalSession, + }), +})) + +vi.mock('@main/setup/toolchain.js', () => ({ + getToolPath: () => '/usr/bin/true', +})) + +vi.mock('@main/performance/PerformanceService.js', () => ({ + performanceService: { + mark: vi.fn(), + record: vi.fn(), + error: vi.fn(), + metric: vi.fn(), + span: () => ({ end: vi.fn(), fail: vi.fn() }), + }, +})) + +vi.mock('@main/storage/feedDebugLog.js', () => ({ + forgetFeedDebugSession: vi.fn(), +})) + +class FakeAgentSession extends EventEmitter { + async start(): Promise { + this.emit('started', { projectDir: '/tmp/project' }) + } + + async stop(): Promise {} + + write(): void {} + + resize(): void {} +} + +// Review of #1376 (a, b, c): claude-code-headless#64 reports proxy generations deleted before the +// app read them as `transport-gap`; ClaudeSession re-emits it as `proxy-transport-gap`. This pins +// the manager's half: an always-on incident for the session, and the event re-emitted with its id. +describe('a Claude proxy transport gap', () => { + beforeEach(() => { + createSession.mockReset() + }) + + it('is recorded as an incident and re-emitted with the session id', async () => { + const { SessionManager } = await import('./sessionManager') + const session = new FakeAgentSession() + createSession.mockImplementationOnce(() => session) + const incidents: Array<{ kind: string; context?: Record }> = [] + const journal = { recordIncident: (incident: { kind: string; context?: Record }) => { incidents.push(incident) }, record: vi.fn(), recordError: vi.fn() } + const manager = new SessionManager(null, null, journal as never) + const gaps: unknown[] = [] + manager.on('proxy-transport-gap', gap => { gaps.push(gap) }) + + await manager.recover({ sessionId: 's1', kind: 'claude', cwd: '/tmp/project' }) + session.emit('proxy-transport-gap', { lostGenerations: 3 }) + + expect(gaps).toEqual([{ sessionId: 's1', lostGenerations: 3 }]) + expect(incidents).toContainEqual(expect.objectContaining({ + kind: 'claude.proxy_transport_gap', + context: { sessionId: 's1', lostGenerations: 3 }, + })) + }) + + // Focused review of #1376 (c): a replaced session's late gap must not be recorded against the + // session id its successor now owns. + it('ignores a gap from a session that has been replaced', async () => { + const { SessionManager } = await import('./sessionManager') + const first = new FakeAgentSession() + const second = new FakeAgentSession() + createSession.mockImplementationOnce(() => first).mockImplementationOnce(() => second) + const incidents: Array<{ kind: string }> = [] + const journal = { recordIncident: (incident: { kind: string }) => { incidents.push(incident) }, record: vi.fn(), recordError: vi.fn() } + const manager = new SessionManager(null, null, journal as never) + const gaps: unknown[] = [] + manager.on('proxy-transport-gap', gap => { gaps.push(gap) }) + + await manager.recover({ sessionId: 's1', kind: 'claude', cwd: '/tmp/project' }) + first.emit('exit', { exitCode: 0 }) + await manager.recover({ sessionId: 's1', kind: 'claude', cwd: '/tmp/project' }) + first.emit('proxy-transport-gap', { lostGenerations: 1 }) + + expect(gaps).toEqual([]) + expect(incidents.filter(incident => incident.kind === 'claude.proxy_transport_gap')).toEqual([]) + }) +}) diff --git a/src/main/sessionManager.ts b/src/main/sessionManager.ts index 82d2fa218..d74cc0d27 100644 --- a/src/main/sessionManager.ts +++ b/src/main/sessionManager.ts @@ -195,6 +195,7 @@ type ManagerEvents = { observation?: AgentTranscriptObservationMetadata }] 'jsonl-error': [{ sessionId: string; error: Error }] + 'proxy-transport-gap': [{ sessionId: string; lostGenerations: number }] /** Durable-history generation boundary (grok). Never completion or idle; * consumers apply renderer/session-runtime/historyBoundary.ts decisions. */ 'history-boundary': [{ sessionId: string; type: 'reset' | 'caught-up'; generation: number; snapshotByteLength: number; byteOffset?: number; complete?: boolean; file: string }] @@ -569,6 +570,11 @@ export type ResolveConditionResult = failedAtStep?: string } +/** The one Claude-only event SessionManager subscribes to (ClaudeSessionEvents declares it). */ +type ProxyGapSource = { + on(event: 'proxy-transport-gap', listener: (gap: { lostGenerations: number }) => void): unknown +} + export class SessionManager extends EventEmitter { private readonly monitorResponses = new ResponseTracker(mainOperations) private readonly sessions = new Map() @@ -3342,6 +3348,21 @@ export class SessionManager extends EventEmitter { } this.emit('jsonl-error', { sessionId, error }) }) + // claude-code-headless#64 / review of #1376: proxy events deleted before the app read them. + // Recorded as an always-on incident (debug bundles carry the journal) and re-emitted, so a + // hole in the live Claude feed is never silent. + // Claude-only (its ClaudeSessionEvents declares it; other providers have no proxy tail), so it + // is subscribed through that type rather than widening every provider's event map. + if (kind === 'claude') (session as unknown as ProxyGapSource).on('proxy-transport-gap', (gap: { lostGenerations: number }) => { + if (!ownsEntry()) return + this.journal?.recordIncident({ + kind: 'claude.proxy_transport_gap', + severity: 'warn', + reason: 'events_deleted_unread', + context: { sessionId, lostGenerations: gap.lostGenerations }, + }) + this.emit('proxy-transport-gap', { sessionId, lostGenerations: gap.lostGenerations }) + }) session.on('transcript-diagnostic', (diagnostic: unknown) => { if (!ownsEntry()) return if (kind === 'codex' && this.observeCodexTranscriptDiagnostic( diff --git a/src/main/sessions/historyLoader.providerOwned.test.ts b/src/main/sessions/historyLoader.providerOwned.test.ts index 145304af2..c9c416816 100644 --- a/src/main/sessions/historyLoader.providerOwned.test.ts +++ b/src/main/sessions/historyLoader.providerOwned.test.ts @@ -57,6 +57,18 @@ describe('historyLoader with a provider-owned history source', () => { expect(span.end).not.toHaveBeenCalled() }) + // #1413 verification a: a Codex rollout the resolver can no longer find + // (removed, or unreadable) made an OLDER page an empty `hasMore: false`, + // so the pane dropped "older history exists" with nothing said. An older + // page is only asked for after one loaded; it now rejects, which the + // renderer reports as a failed, retryable page. + it('rejects an older page whose transcript can no longer be resolved', async () => { + registry.resolveTranscriptPath.mockResolvedValue(null) + await expect(loadOlderHistoryChunk({ kind: 'codex', cwd: '/w', providerSessionId: 'thread-1', beforeMarker: 'm', limit: 5 })) + .rejects.toThrow('could not be found') + expect(span.fail).toHaveBeenCalledOnce() + }) + it('keeps file-backed providers on the JSONL path', async () => { registry.resolveTranscriptPath.mockResolvedValue(null) // The assertion that matters is the ROUTING: a provider without its own diff --git a/src/main/sessions/historyLoader.test.ts b/src/main/sessions/historyLoader.test.ts index 66f147ac3..881bd07fa 100644 --- a/src/main/sessions/historyLoader.test.ts +++ b/src/main/sessions/historyLoader.test.ts @@ -292,6 +292,22 @@ describe('readInitialTranscriptTail', () => { }) describe('readOlderTranscriptWindow', () => { + // #1413 review a: the older page of a transcript that became unreadable + // after its first page loaded came back as an empty page with + // `hasMore: false`. The renderer then dropped "older history exists" and + // said nothing. It now rejects, so the page is reported as failed and can + // be retried; a genuinely empty file stays an honest empty page. + it('rejects an older page of a transcript that became unreadable, but not of an empty one', async () => { + const file = writeClaude('vanishing.jsonl', 30) + const first = await loadInitialHistoryChunkFromFile(file, 12) + expect(first.hasMore).toBe(true) + rmSync(file) + await expect(loadOlderHistoryChunkFromFile(file, { kind: 'claude', beforeMarker: 'u-18', limit: 12 })).rejects.toThrow(/ENOENT/) + const empty = join(root, 'empty-older.jsonl') + writeFileSync(empty, '') + expect(await loadOlderHistoryChunkFromFile(empty, { kind: 'claude', beforeMarker: 'u-18', limit: 12 })).toEqual({ entries: [], hasMore: false }) + }) + it('pages older history identically to the forward marker scan at every depth, with and without an offset', async () => { const file = writeClaude('pages.jsonl', 300) const bytes = readFileSync(file) diff --git a/src/main/sessions/historyLoader.ts b/src/main/sessions/historyLoader.ts index 458af40ed..a05a1f777 100644 --- a/src/main/sessions/historyLoader.ts +++ b/src/main/sessions/historyLoader.ts @@ -550,7 +550,14 @@ async function readOlderTranscriptWindow( entries: Record[] offsets: number[] }> { - const size = await stat(filePath).then(s => s.size).catch(() => 0) + // WHY an unreadable transcript THROWS here (#1413 review a): this reader + // serves only older-history paging, where the renderer already holds a + // page from this file. A failed stat/open/read used to come back as an + // empty page with `hasMore: false`, so the feed dropped "older history + // exists" for good and the user was told nothing. A rejection reaches + // the renderer as a failed page, which is said and can be retried. An + // EMPTY file (stat succeeded, size 0) is still an honest empty page. + const size = (await stat(filePath)).size const empty = { bytes: size, tailBytes: 0, @@ -571,13 +578,7 @@ async function readOlderTranscriptWindow( : extractCodexHistoryMarker const limit = Math.max(0, params.limit) - let handle: FileHandle - try { - handle = await open(filePath, 'r') - } catch (error) { - if (params.beforeRecordHash) throw error - return empty - } + const handle: FileHandle = await open(filePath, 'r') const stats = parseStats() try { let parseErrors = 0 @@ -642,9 +643,6 @@ async function readOlderTranscriptWindow( return false }) return finishWindow(size, tailBytes, parseErrors, parsed, found, found ? 'marker' : 'tail', kept, limit) - } catch (error) { - if (params.beforeRecordHash) throw error - return empty } finally { observeParse(stats) await handle.close().catch(() => {}) @@ -708,10 +706,17 @@ export async function loadOlderHistoryChunk( // + catch scaffolding in both). const filePath = await resolveHistoryTranscriptPath(params) if (!filePath) { + // WHY a rejection and not an empty page (#1413 verification a): an older + // page is only ever asked for after a page of this transcript loaded, so + // "no file now" means it vanished or cannot be located, not that there is + // nothing older. An empty `hasMore: false` made the renderer drop "older + // history exists" for good, with nothing said; a rejection is reported as + // a failed page and stays retryable. + const error = new Error('The transcript for this session could not be found') performanceService .span('historyLoader.loadOlderChunk', { kind: params.kind, limit: params.limit }) - .end({ result: 'missing-file' }) - return { entries: [], hasMore: false } + .fail(error, { result: 'missing-file' }) + throw error } return loadOlderHistoryChunkFromFile(filePath, params) } diff --git a/src/main/storage/debugRetention.test.ts b/src/main/storage/debugRetention.test.ts index e4c0683f7..600c599af 100644 --- a/src/main/storage/debugRetention.test.ts +++ b/src/main/storage/debugRetention.test.ts @@ -4,7 +4,7 @@ import { join } from 'node:path' import { afterEach, beforeEach, describe, expect, it } from 'vitest' -import { collectSessionRecordingDirs, runPrunePasses } from './debugRetention.js' +import { collectProxyRunDirs, collectSessionRecordingDirs, runPrunePasses } from './debugRetention.js' import type { DebugStorageArtifact, DebugStorageBucket, @@ -226,3 +226,19 @@ describe('runPrunePasses', () => { expect(result).toEqual({ removed: 0, bytesFreed: 0, remainingBytes: 500 }) }) }) + +// Review of #1376 (a): claude-code-headless#64 rotates proxy-events.jsonl to proxy-events.1.jsonl +// before creating the next live file. A run left holding only `.1` (the creation failed, or the +// addon died in between) was invisible to every retention pass. +describe('collectProxyRunDirs', () => { + it('counts a run that holds only the rotated generation', async () => { + const live = join(root, 'project', 'session', 'run-live') + const rotated = join(root, 'project', 'session', 'run-rotated') + mkdirSync(live, { recursive: true }) + mkdirSync(rotated, { recursive: true }) + writeFileSync(join(live, 'proxy-events.jsonl'), '{}\n') + writeFileSync(join(rotated, 'proxy-events.1.jsonl'), '{}\n') + const runs = (await collectProxyRunDirs(root)).map(artifact => artifact.path).sort() + expect(runs).toEqual([live, rotated]) + }) +}) diff --git a/src/main/storage/debugRetention.ts b/src/main/storage/debugRetention.ts index 8bdaace51..ee54f4b53 100644 --- a/src/main/storage/debugRetention.ts +++ b/src/main/storage/debugRetention.ts @@ -655,7 +655,18 @@ async function loadManualLegacyBundlePaths(): Promise> { return manual } -async function collectProxyRunDirs(root: string): Promise { +/** + * A proxy run is a directory holding the live events file OR only its rotated generation. + * + * WHY `.1` too (review of #1376, a): claude-code-headless#64 renames the live file to + * `proxy-events.1.jsonl` before creating the next one. If that creation fails, or the addon dies in + * between, the run holds only `.1`; matching the live name alone made such a run invisible to + * TTL, cap and budget alike, so stranded runs could accumulate. The bundle reader already treats + * `.1` as a run (proxyEventsReader.ts). + */ +const PROXY_RUN_MARKERS = new Set(['proxy-events.jsonl', 'proxy-events.1.jsonl']) + +export async function collectProxyRunDirs(root: string): Promise { const out: Artifact[] = [] async function walk(dir: string, depth: number): Promise { let entries @@ -664,7 +675,7 @@ async function collectProxyRunDirs(root: string): Promise { } catch { return } - if (entries.some(entry => entry.isFile() && entry.name === 'proxy-events.jsonl')) { + if (entries.some(entry => entry.isFile() && PROXY_RUN_MARKERS.has(entry.name))) { const artifact = await collectDirArtifact(dir, 'proxy') if (artifact) out.push(artifact) return diff --git a/src/providers/claude/runtime/claudeSession.suspension.test.ts b/src/providers/claude/runtime/claudeSession.suspension.test.ts index 70b692614..46d42f896 100644 --- a/src/providers/claude/runtime/claudeSession.suspension.test.ts +++ b/src/providers/claude/runtime/claudeSession.suspension.test.ts @@ -1,3 +1,5 @@ +import { EventEmitter } from 'node:events' + import { describe, expect, it, vi } from 'vitest' import { ClaudeSession } from './claudeSession.js' @@ -51,3 +53,45 @@ describe('ClaudeSession.readComposer', () => { expect(session.readComposer()).toEqual({ screen: '❯ typed', attributes }) }) }) + +// Review of #1376 (a, b, c): the app subscribed to the proxy's `event` channel only, so the +// `transport-gap` claude-code-headless#64 reports (generations rotated away unread) never left the +// package — and deleting even the `event` subscription passed every Claude runtime test. +describe('ClaudeSession proxy wiring', () => { + function wired() { + const session = new ClaudeSession() + const proxy = new EventEmitter() + const handleProxyTransportEvent = vi.fn() + const internals = session as unknown as { + proxyServer: unknown + headless: unknown + attachProxyServer(): void + detachProxyServer(): void + } + internals.proxyServer = proxy + internals.headless = { handleProxyTransportEvent } + internals.attachProxyServer() + return { session, proxy, handleProxyTransportEvent, detach: () => internals.detachProxyServer() } + } + + it('forwards every proxy event to the adapter', () => { + const { proxy, handleProxyTransportEvent } = wired() + const chunk = { kind: 'response-chunk', flow_id: 'f1', chunk_b64: 'eA==' } + proxy.emit('event', chunk) + expect(handleProxyTransportEvent).toHaveBeenCalledWith(chunk) + }) + + it('surfaces a transport gap as a session event', () => { + const { session, proxy } = wired() + const gaps: unknown[] = [] + session.on('proxy-transport-gap', gap => { gaps.push(gap) }) + proxy.emit('transport-gap', { lostGenerations: 2 }) + expect(gaps).toEqual([{ lostGenerations: 2 }]) + }) + + it('detaches both channels', () => { + const { proxy, detach } = wired() + detach() + expect(proxy.listenerCount('event') + proxy.listenerCount('transport-gap')).toBe(0) + }) +}) diff --git a/src/providers/claude/runtime/claudeSession.ts b/src/providers/claude/runtime/claudeSession.ts index 361e97aa8..eb6760db3 100644 --- a/src/providers/claude/runtime/claudeSession.ts +++ b/src/providers/claude/runtime/claudeSession.ts @@ -97,6 +97,7 @@ export type ClaudeSessionEvents = { // Declared for the provider-neutral AgentSession contract. Claude currently // emits no transcript-discovery diagnostics, so this event never fires. 'transcript-diagnostic': [unknown] + 'proxy-transport-gap': [{ lostGenerations: number }] // Optional status: the spinner verb ("Cogitating…", "Cascading…", // …) so the renderer can label its activity indicator with what CC // is actually doing rather than a generic "thinking…" placeholder. @@ -168,6 +169,7 @@ export class ClaudeSession extends EventEmitter { // proxy shutdown path drop the emitter isn't enough — the closure // captures `this.headless` and delays GC of the session object. private proxyEventHandler: ((ev: unknown) => void) | null = null + private proxyGapHandler: ((gap: { lostGenerations: number }) => void) | null = null private exited = false /** Gate for the committed `tool_result` bridge. False until the * JSONL tailer's initial replay has quiesced (250 ms without a new @@ -512,16 +514,7 @@ export class ClaudeSession extends EventEmitter { // the EventEmitter keeps a reference to the closure (which closes // over `this.headless`) even after teardown, preventing GC of // the session object until the proxy process itself is collected. - if (this.proxyServer) { - this.proxyEventHandler = ev => { - this.headless?.handleProxyTransportEvent( - ev as Parameters< - NonNullable['handleProxyTransportEvent'] - >[0], - ) - } - this.proxyServer.on('event', this.proxyEventHandler) - } + this.attachProxyServer() this.pty.onData((data: string) => this.emit('pty-data', data)) @@ -1165,12 +1158,47 @@ export class ClaudeSession extends EventEmitter { // a leak-relevance (a live mitmdump keeps its port bound), and two // hand-maintained copies of it is exactly how the rollback path missed // it in the first place (#495 A9). - private async teardownProxy(): Promise { + /** + * Subscribe to the proxy's two channels. Only when useProxy was set (`proxyServer` is otherwise + * null), and AFTER headless exists, so the adapter is there before any chunk arrives. + * + * WHY `transport-gap` too (review of #1376, a/b/c): claude-code-headless#64 rotates the events file + * and, when the poller stalls through rotations, reports generations deleted unread as + * `transport-gap`. The app listened to `event` only, so a whole span of chunks could vanish with + * nothing but a main-process console line — the "every event exactly once, or an explicit gap" + * contract stopped at the package boundary. The gap is re-emitted as `proxy-transport-gap`, which + * SessionManager records as an always-on incident for this session. + */ + private attachProxyServer(): void { + if (!this.proxyServer) return + this.proxyEventHandler = ev => { + this.headless?.handleProxyTransportEvent( + ev as Parameters< + NonNullable['handleProxyTransportEvent'] + >[0], + ) + } + this.proxyGapHandler = gap => { this.emit('proxy-transport-gap', gap) } + this.proxyServer.on('event', this.proxyEventHandler) + this.proxyServer.on('transport-gap', this.proxyGapHandler) + } + + /** Detach both proxy channels; see attachProxyServer. */ + private detachProxyServer(): void { if (!this.proxyServer) return if (this.proxyEventHandler) { this.proxyServer.off('event', this.proxyEventHandler) this.proxyEventHandler = null } + if (this.proxyGapHandler) { + this.proxyServer.off('transport-gap', this.proxyGapHandler) + this.proxyGapHandler = null + } + } + + private async teardownProxy(): Promise { + if (!this.proxyServer) return + this.detachProxyServer() try { // WHY a deadline around the package's stop(): ProxyServer.stop() // awaits `child.once('exit')` after SIGTERM/SIGKILL, and that promise diff --git a/src/renderer/src/features/feed/ui/Feed.olderHistory.renderer.test.tsx b/src/renderer/src/features/feed/ui/Feed.olderHistory.renderer.test.tsx new file mode 100644 index 000000000..543174d36 --- /dev/null +++ b/src/renderer/src/features/feed/ui/Feed.olderHistory.renderer.test.tsx @@ -0,0 +1,49 @@ +import { act, fireEvent, render, screen } from '@testing-library/react' +import { describe, expect, it, vi } from 'vitest' + +import { Feed } from '@renderer/features/feed/ui/Feed' + +// #1413 review a and b: after a failed older-history page, the pane says +// "Scroll up again to retry". The real Feed used to request a page only on a +// `scroll` event, and at scrollTop 0 an upward wheel moves nothing, so no +// event fired: the retry the toast promised could not happen at the top. + +describe('Feed older-history trigger', () => { + it('retries a failed page on an upward wheel at the very top', async () => { + const onLoadOlderHistory = vi.fn(async () => {}) + render() + const scroller = screen.getByRole('region', { name: 'Conversation' }) + expect(scroller.scrollTop).toBe(0) + // First request (as a scroll to the top makes it) fails in the hook. + await act(async () => { fireEvent.wheel(scroller, { deltaY: -40 }) }) + expect(onLoadOlderHistory).toHaveBeenCalledTimes(1) + // The user does what the toast says: another upward gesture, still at 0. + await act(async () => { fireEvent.wheel(scroller, { deltaY: -40 }) }) + expect(onLoadOlderHistory).toHaveBeenCalledTimes(2) + }) + + // Verification b: the touch form of the same gesture. + it('retries on a downward finger drag at the very top, and not on an upward one', async () => { + const onLoadOlderHistory = vi.fn(async () => {}) + render() + const scroller = screen.getByRole('region', { name: 'Conversation' }) + await act(async () => { + fireEvent.touchStart(scroller, { touches: [{ clientY: 100 }] }) + fireEvent.touchMove(scroller, { touches: [{ clientY: 60 }] }) + }) + expect(onLoadOlderHistory).not.toHaveBeenCalled() + await act(async () => { + fireEvent.touchStart(scroller, { touches: [{ clientY: 100 }] }) + fireEvent.touchMove(scroller, { touches: [{ clientY: 140 }] }) + }) + expect(onLoadOlderHistory).toHaveBeenCalledTimes(1) + }) + + it('does not load on a downward wheel', async () => { + const onLoadOlderHistory = vi.fn(async () => {}) + render() + const scroller = screen.getByRole('region', { name: 'Conversation' }) + await act(async () => { fireEvent.wheel(scroller, { deltaY: 40 }) }) + expect(onLoadOlderHistory).not.toHaveBeenCalled() + }) +}) diff --git a/src/renderer/src/features/feed/ui/Feed.tsx b/src/renderer/src/features/feed/ui/Feed.tsx index 0be4a571c..f5ec71a41 100644 --- a/src/renderer/src/features/feed/ui/Feed.tsx +++ b/src/renderer/src/features/feed/ui/Feed.tsx @@ -475,6 +475,33 @@ function FeedImpl({ useEffect(() => { const el = scrollerRef.current if (!el) return + const loadOlderNearTop = () => { + if ( + el.scrollTop < 160 && + hasOlderHistory && + !loadingOlderHistory && + !loadingOlderRef.current && + !tailMode && + onLoadOlderHistory + ) { + loadingOlderRef.current = true + const beforeHeight = el.scrollHeight + const beforeTop = el.scrollTop + void onLoadOlderHistory() + .then(() => { + requestAnimationFrame(() => { + const next = scrollerRef.current + if (!next) return + const delta = next.scrollHeight - beforeHeight + next.scrollTop = beforeTop + Math.max(0, delta) + lastScrollTopRef.current = next.scrollTop + }) + }) + .finally(() => { + loadingOlderRef.current = false + }) + } + } const onScroll = () => { if (tailMode) { el.scrollTop = el.scrollHeight @@ -519,34 +546,41 @@ function FeedImpl({ onScrollInfo({ fraction }) } - if ( - el.scrollTop < 160 && - hasOlderHistory && - !loadingOlderHistory && - !loadingOlderRef.current && - !tailMode && - onLoadOlderHistory - ) { - loadingOlderRef.current = true - const beforeHeight = el.scrollHeight - const beforeTop = el.scrollTop - void onLoadOlderHistory() - .then(() => { - requestAnimationFrame(() => { - const next = scrollerRef.current - if (!next) return - const delta = next.scrollHeight - beforeHeight - next.scrollTop = beforeTop + Math.max(0, delta) - lastScrollTopRef.current = next.scrollTop - }) - }) - .finally(() => { - loadingOlderRef.current = false - }) - } + loadOlderNearTop() } el.addEventListener('scroll', onScroll, { passive: true }) - return () => el.removeEventListener('scroll', onScroll) + // WHY a wheel trigger too (#1413 review a, b): the loader used to fire only + // on a `scroll` event. At scrollTop 0 an upward wheel or trackpad gesture + // moves nothing, so no scroll event fires, and after a failed page the + // pane's "Scroll up again to retry" could not be done without first + // scrolling down. An upward wheel AT the top is the same request. + const onWheel = (event: WheelEvent) => { + if (event.deltaY < 0 && el.scrollTop <= 0) loadOlderNearTop() + } + el.addEventListener('wheel', onWheel, { passive: true }) + // The touch form of the same gesture (#1413 verification b): a finger + // dragging DOWN at the top asks for older content and, at the boundary, + // fires no scroll event either. + let touchStartY: number | null = null + const onTouchStart = (event: TouchEvent) => { + touchStartY = event.touches[0]?.clientY ?? null + } + const onTouchMove = (event: TouchEvent) => { + const y = event.touches[0]?.clientY + if (touchStartY === null || y === undefined) return + if (y > touchStartY && el.scrollTop <= 0) { + touchStartY = null + loadOlderNearTop() + } + } + el.addEventListener('touchstart', onTouchStart, { passive: true }) + el.addEventListener('touchmove', onTouchMove, { passive: true }) + return () => { + el.removeEventListener('scroll', onScroll) + el.removeEventListener('wheel', onWheel) + el.removeEventListener('touchstart', onTouchStart) + el.removeEventListener('touchmove', onTouchMove) + } }, [ sessionId, onScrollInfo, diff --git a/src/renderer/src/workspace/hook/actions/history.renderer.test.tsx b/src/renderer/src/workspace/hook/actions/history.renderer.test.tsx index f1958a8fa..61ed5db2a 100644 --- a/src/renderer/src/workspace/hook/actions/history.renderer.test.tsx +++ b/src/renderer/src/workspace/hook/actions/history.renderer.test.tsx @@ -126,3 +126,83 @@ describe('older history and the ingest watermark (#915)', () => { }) }) }) + +// #1250 row 12: a failed page used to clear the spinner and nothing else, and +// returned nothing, so no caller could tell the user. The hook now answers +// what happened, and a failure leaves `hasOlderHistory` set so the next +// scroll to the top retries. +describe('what an older-history request reports', () => { + function harness(runtime: Partial) { + let runtimes: Record = { session: { ...emptyRuntime(), ...runtime } } + const refs = { + stateRef: ref({ sessions: { session: { kind: 'claude', cwd: '/tmp/project', providerSessionId: 'provider-session' } } }), + latestRuntimesRef: ref(runtimes), + seenUuidsRef: ref({}), + } as unknown as WorkspaceRefs + const setRuntimes: WorkspaceSetRuntimes = next => { + runtimes = typeof next === 'function' ? next(runtimes) : next + refs.latestRuntimesRef.current = runtimes + } + const updateRuntime = (id: string, patch: Partial) => { + setRuntimes(prev => ({ ...prev, [id]: { ...prev[id]!, ...patch } })) + } + const { result } = renderHook(() => useHistoryActions(setRuntimes, refs, updateRuntime, ipcSessionFeed)) + return { load: () => result.current.loadOlderHistory('session'), runtime: () => runtimes.session!, refs } + } + + it('answers failed when the page cannot be read, and leaves a retry possible', async () => { + vi.spyOn(console, 'warn').mockImplementation(() => {}) + Object.defineProperty(window, 'api', { configurable: true, value: { + loadOlderHistory: vi.fn(async () => { throw new Error("ENOENT: no such file or directory, open '/Users/someone/.claude/projects/x.jsonl'") }), + gitWorktrees: vi.fn(async () => ({ ok: true, worktrees: [] })), + } }) + const { load, runtime } = harness({ hasOlderHistory: true, historyOldestMarker: 'anchor' }) + let answer: unknown + await act(async () => { answer = await load() }) + expect(answer).toBe('failed') + expect(runtime()).toMatchObject({ hasOlderHistory: true, loadingOlderHistory: false }) + vi.restoreAllMocks() + }) + + // #1413 review c: every early return is `skipped`, never `failed`. A + // `failed` here would toast "Couldn't load older messages" on every scroll + // tick of a feed that simply has nothing older. + it.each([ + ['no older history', { hasOlderHistory: false, historyOldestMarker: 'anchor' }], + ['a load already running', { hasOlderHistory: true, loadingOlderHistory: true, historyOldestMarker: 'anchor' }], + ] as const)('answers skipped for %s', async (_name, runtime) => { + const loadOlderHistory = vi.fn() + Object.defineProperty(window, 'api', { configurable: true, value: { loadOlderHistory, gitWorktrees: vi.fn() } }) + const { load } = harness(runtime) + let answer: unknown + await act(async () => { answer = await load() }) + expect(answer).toBe('skipped') + expect(loadOlderHistory).not.toHaveBeenCalled() + }) + + it('answers skipped for a session it cannot page (no meta, no provider session)', async () => { + Object.defineProperty(window, 'api', { configurable: true, value: { loadOlderHistory: vi.fn(), gitWorktrees: vi.fn() } }) + const { load, refs } = harness({ hasOlderHistory: true, historyOldestMarker: 'anchor' }) + let answer: unknown + ;(refs.stateRef.current as { sessions: Record }).sessions = { session: { kind: 'claude', cwd: '/tmp/project' } } + await act(async () => { answer = await load() }) + expect(answer).toBe('skipped') + ;(refs.stateRef.current as { sessions: Record }).sessions = {} + await act(async () => { answer = await load() }) + expect(answer).toBe('skipped') + }) + + it('answers loaded for a page, and skipped when nothing was asked', async () => { + Object.defineProperty(window, 'api', { configurable: true, value: { + loadOlderHistory: vi.fn(async () => ({ entries: [], hasMore: false })), + gitWorktrees: vi.fn(async () => ({ ok: true, worktrees: [] })), + } }) + const loaded = harness({ hasOlderHistory: true, historyOldestMarker: 'anchor' }) + let answer: unknown + await act(async () => { answer = await loaded.load() }) + expect(answer).toBe('loaded') + const noMarker = harness({ hasOlderHistory: true, historyOldestMarker: null }) + await act(async () => { answer = await noMarker.load() }) + expect(answer).toBe('skipped') + }) +}) diff --git a/src/renderer/src/workspace/hook/actions/history.ts b/src/renderer/src/workspace/hook/actions/history.ts index c49cfb353..d41f1d92c 100644 --- a/src/renderer/src/workspace/hook/actions/history.ts +++ b/src/renderer/src/workspace/hook/actions/history.ts @@ -35,6 +35,12 @@ import type { SessionFeed } from '@shared/sessionFeed/SessionFeed' // markers so paged response_items get the same ownership metadata as // entries that arrived live. +/** What one older-history request did. `failed` means the page could not be + * read (the IPC rejected, or the transcript could not be parsed); `skipped` + * means nothing was asked (no provider session, nothing older, a load + * already running). Only `failed` is worth telling the user about. */ +export type OlderHistoryLoadResult = 'loaded' | 'skipped' | 'failed' + export function useHistoryActions( setRuntimes: WorkspaceSetRuntimes, refs: WorkspaceRefs, @@ -45,17 +51,17 @@ export function useHistoryActions( // and its one caller already holds the feed from context. feed: Pick, ): { - loadOlderHistory: (sessionId: SessionId) => Promise + loadOlderHistory: (sessionId: SessionId) => Promise } { const loadOlderHistory = useCallback( - async (sessionId: SessionId) => { + async (sessionId: SessionId): Promise => { const span = perf.span('workspace.history.loadOlder', { sessionId }) const currentState = refs.stateRef.current const meta = currentState.sessions[sessionId] const runtime = refs.latestRuntimesRef.current[sessionId] ?? emptyRuntime() if (!meta) { span.end({ skipped: 'missing-meta' }) - return + return 'skipped' } const kind = meta.kind ?? DEFAULT_PROVIDER @@ -65,19 +71,19 @@ export function useHistoryActions( // does page it gets the same answer. if (!isAgentProviderKind(kind) || !meta.providerSessionId) { span.end({ skipped: 'unsupported-or-missing-provider-session', kind }) - return + return 'skipped' } if (!runtime.hasOlderHistory || runtime.loadingOlderHistory) { span.end({ skipped: runtime.loadingOlderHistory ? 'already-loading' : 'no-older-history', kind, }) - return + return 'skipped' } if (!runtime.historyOldestMarker) { updateRuntime(sessionId, { hasOlderHistory: false, loadingOlderHistory: false }) span.end({ skipped: 'missing-marker', kind }) - return + return 'skipped' } updateRuntime(sessionId, { loadingOlderHistory: true }) @@ -246,10 +252,15 @@ export function useHistoryActions( prepended: prepend.length, hasMore: chunk.hasMore, }) + return 'loaded' } catch (err) { span.fail(err, { kind }) console.warn('[history] load older failed', err) + // `hasOlderHistory` is left as it was, so the next scroll to the top + // retries. The caller is TOLD (#1250 row 12): this used to be the + // whole of it, and the feed then looked as if it had nothing older. updateRuntime(sessionId, { loadingOlderHistory: false }) + return 'failed' } }, [refs.latestRuntimesRef, refs.seenUuidsRef, refs.stateRef, setRuntimes, updateRuntime, feed], diff --git a/src/renderer/src/workspace/tile-tree/TileLeaf.olderHistory.renderer.test.tsx b/src/renderer/src/workspace/tile-tree/TileLeaf.olderHistory.renderer.test.tsx new file mode 100644 index 000000000..d74287a97 --- /dev/null +++ b/src/renderer/src/workspace/tile-tree/TileLeaf.olderHistory.renderer.test.tsx @@ -0,0 +1,108 @@ +import { act, cleanup, render } from '@testing-library/react' +import { afterEach, beforeEach, expect, it, vi } from 'vitest' + +import { useAppStore } from '@renderer/app-state/store' +import { emptyRuntime } from '@renderer/session-runtime/state' +import type { Workspace } from '@renderer/workspace/hook' +import type { OlderHistoryLoadResult } from '@renderer/workspace/hook/actions/history' +import { AgentTerminalOwnerVisibilityProvider } from '@renderer/workspace/terminal/AgentTerminalOwnership' +import { OLDER_HISTORY_FAILED, TileLeaf } from './TileLeaf' + +// #1250 row 12: a failed older-history page cleared its spinner and said +// nothing, so the feed looked as if it had nothing older. TileLeaf is where +// the answer from the hook meets the pane, so the wire is probed here: Feed is +// replaced by a probe that hands back the REAL `onLoadOlderHistory` prop +// TileLeaf passes down (through the real AgentFeed), and calling it is exactly +// what a scroll to the top does. + +let loadOlder: (() => Promise) | undefined +vi.mock('@renderer/features/feed/ui/Feed', () => ({ + Feed: ({ onLoadOlderHistory }: { onLoadOlderHistory?: () => Promise }) => { + loadOlder = onLoadOlderHistory + return
+ }, +})) +vi.mock('@renderer/features/sessionFeed/SessionFeedContext', () => ({ useSessionFeed: () => ({}) })) +vi.mock('@renderer/features/feed/ledger/useLedgerFeedItems', () => ({ useLedgerFeedItems: () => ({ items: [] }) })) +vi.mock('@renderer/features/usage-limit/useUsageLimitActions', () => ({ useUsageLimitActions: () => ({}) })) +vi.mock('@renderer/features/workflows/model/useSessionWorkflowViews', () => ({ + useSessionWorkflowViews: () => ({ references: [], allReferences: [], selectedReference: null }), +})) +vi.mock('./TileLeaf/useComposerKeybinds', () => ({ useComposerKeybinds: () => ({ onKeyDown: vi.fn(), slashMode: false, submitCurrentDraft: vi.fn() }) })) +vi.mock('./TileLeaf/useComposerDictation', () => ({ useComposerDictation: () => ({ handleShortcut: () => false }) })) +vi.mock('./TileLeaf/PaneHeader', () => ({ PaneHeader: () => null })) +vi.mock('./TileLeaf/QueueStrip', () => ({ QueueStrip: () => null })) +vi.mock('./TileLeaf/PaneToast', () => ({ PaneToast: () => null })) +vi.mock('./TileLeaf/ComposerInput', () => ({ ComposerInput: () => null })) +vi.mock('./TileLeaf/ComposerActions', () => ({ ComposerActions: () => null })) +vi.mock('@providers/shared/renderer/conditions/ProviderConditionOutlet', () => ({ ProviderConditionOutlet: () => null })) + +const original = useAppStore.getState() +beforeEach(() => { loadOlder = undefined }) +afterEach(() => { cleanup(); vi.useRealTimers(); useAppStore.setState(original, true) }) + +function mount(answers: OlderHistoryLoadResult[]) { + const paneToasts: string[] = [] + const toastSessions: string[] = [] + const workspace = { + state: { sessions: { agent: { kind: 'claude', cwd: '/trial' }, other: { kind: 'claude', cwd: '/trial' } } }, + acknowledgeSession: vi.fn(), + setDraftInput: vi.fn(), + loadOlderHistory: vi.fn(async () => answers.shift() ?? 'skipped'), + showPaneToast: (sessionId: string, message: string) => { paneToasts.push(message); toastSessions.push(sessionId) }, + } as unknown as Workspace + const leaf = (sessionId: string) => ( + + + + ) + const view = render(leaf('agent')) + // A prop TileLeaf never passed would make every assertion below vacuous. + expect(loadOlder).toBeTypeOf('function') + return { paneToasts, toastSessions, workspace, switchTo: (sessionId: string) => view.rerender(leaf(sessionId)) } +} + +it('says a failed page in fixed words, once per burst of retries', async () => { + vi.useFakeTimers({ toFake: ['Date'] }) + const { paneToasts, toastSessions } = mount(['failed', 'failed', 'failed', 'failed']) + await act(async () => { await loadOlder!() }) + // The literal words (#1413 review c): comparing against the exported + // constant would let a reworded or interpolated message pass. And on THIS + // pane. + expect(paneToasts).toEqual(["Couldn't load older messages. Scroll up again to retry."]) + expect(toastSessions).toEqual(['agent']) + expect(OLDER_HISTORY_FAILED).toBe(paneToasts[0]) + // Every scroll tick near the top retries; a burst is one toast. + await act(async () => { await loadOlder!() }) + expect(paneToasts).toHaveLength(1) + // Still inside the window a second later (review c: a window shrunk to + // 500 ms would toast here). + vi.setSystemTime(Date.now() + 1_000) + await act(async () => { await loadOlder!() }) + expect(paneToasts).toHaveLength(1) + // After the coalescing window, a new failure is said again. + vi.setSystemTime(Date.now() + 4_001) + await act(async () => { await loadOlder!() }) + expect(paneToasts).toEqual([OLDER_HISTORY_FAILED, OLDER_HISTORY_FAILED]) +}) + +it('says nothing for a page that loaded or a request that was skipped', async () => { + const { paneToasts } = mount(['loaded', 'skipped']) + await act(async () => { await loadOlder!() }) + await act(async () => { await loadOlder!() }) + expect(paneToasts).toEqual([]) +}) + +// Steering q106: the dispatch layout re-renders the SAME leaf with another +// agent when a lane switches. Agent A's toast must not silence agent B's +// first failure; repeated failures of one agent still coalesce. +it('does not let one agent\'s toast silence another agent in the same leaf', async () => { + vi.useFakeTimers({ toFake: ['Date'] }) + const { paneToasts, toastSessions, switchTo } = mount(['failed', 'failed', 'failed']) + await act(async () => { await loadOlder!() }) + switchTo('other') + await act(async () => { await loadOlder!() }) + expect(toastSessions).toEqual(['agent', 'other']) + await act(async () => { await loadOlder!() }) + expect(paneToasts).toEqual([OLDER_HISTORY_FAILED, OLDER_HISTORY_FAILED]) +}) diff --git a/src/renderer/src/workspace/tile-tree/TileLeaf.tsx b/src/renderer/src/workspace/tile-tree/TileLeaf.tsx index 598b85ffb..10f3d027c 100644 --- a/src/renderer/src/workspace/tile-tree/TileLeaf.tsx +++ b/src/renderer/src/workspace/tile-tree/TileLeaf.tsx @@ -116,6 +116,10 @@ type Props = { showWorktreeBadges?: boolean } + +export const OLDER_HISTORY_FAILED = "Couldn't load older messages. Scroll up again to retry." +const OLDER_HISTORY_TOAST_COALESCE_MS = 5_000 + export function TileLeaf({ sessionId, runtime, @@ -508,9 +512,26 @@ export function TileLeaf({ } }, [acknowledgeSession, feed, runtime.conditions, sessionId, showToast]) + // WHY a pane toast on a failed page (#1250 row 12): the loader used to + // clear its spinner and nothing else, so the feed looked as if it had + // nothing older. Fixed words (q22): the error can name a transcript path. + // Coalesced, because while the scroller sits near the top every scroll + // tick retries, and each failure must not stack another toast. + // + // Keyed by session, not per mounted leaf (steering q106): the dispatch + // layout re-renders the SAME TileLeaf with another agent's sessionId when + // a lane switches, so one timestamp let agent A's toast silence agent B's + // first failure for the rest of the window. + const lastOlderHistoryToastAtRef = useRef(new Map()) const loadOlderHistory = useCallback(async () => { - await workspace.loadOlderHistory(sessionId) - }, [sessionId, workspace.loadOlderHistory]) + const result = await workspace.loadOlderHistory(sessionId) + if (result !== 'failed') return + const now = Date.now() + const last = lastOlderHistoryToastAtRef.current.get(sessionId) ?? 0 + if (now - last < OLDER_HISTORY_TOAST_COALESCE_MS) return + lastOlderHistoryToastAtRef.current.set(sessionId, now) + workspace.showPaneToast(sessionId, OLDER_HISTORY_FAILED) + }, [sessionId, workspace.loadOlderHistory, workspace.showPaneToast]) const appendRenderDebug = useCallback((entry: Parameters[1]) => { workspace.appendFeedDebug(sessionId, entry)