AI foundation checkpoint and full-scope agent handoff - #11
Merged
Conversation
Add bounded lifecycle and owned preparation admission, strict provider handling, and safe cancellation. Add server-owned plan and patch snapshots, prompt evidence coverage, and offline evaluation/browser baselines. Keep full UI/toolkit, durable-state, provider certification, and release acceptance work open.
The Codex "90-second timeout regression" was a test-harness fault, not a
product one: vite externalizes `node:*`, so `vi.mock("node:child_process")`
never reached `adapters.ts` and the suites spawned the real `codex` binary.
Route provider spawning through a local `child-process.js` seam that tests
can mock, and serve the model catalog offline where option gating reads it.
- codex-run: 33 failing / 243s -> 34 passing / 2.4s
- codex-image-snapshot: fake child now speaks the correlated thread/turn
protocol and emits `close` on SIGTERM, matching the adapter's ownership
contract
Also clear `tsc --noEmit` from 31 errors to 0. Two were real: `PrSessionResponse`
never declared `body`/`createdAt`/`issueComments`/`timelineEvents` even though
the server sends them, so the PR timeline was type-blind to the description and
issue comments; and `panelRef` was typed non-null against a `useRef(null)`.
`buildPrTimeline` now takes the conversation fields it actually reads rather
than a whole `PrSession`. The rest are fixture and mock-signature updates.
tsconfig drops `rootDir` so the vite-config and pi-extension contract tests
typecheck; nothing emits with tsc.
`validateProviderOptions` calls `adapter.models()` directly, bypassing the service-level model cache, so every run carrying reasoning effort, service tier or images spawned a second `codex app-server` purely to validate those options against the catalog. Retain successful catalogs for the same 15s the service uses, and clear them from `AiService.invalidateCatalog()` so the existing credential-change and refresh paths stay live. Failures are never retained. `catalog.ts` now spawns through the `child-process.js` seam, which is what makes this testable.
Introduces the C18 tool layer: `review.map`, `source.read`, `diff.read` and `source.search`, all derived from a captured `ReviewSnapshot`. The snapshot passed in is the entire world a run may read — the tools reach no shell, network or filesystem, and cannot widen their own scope. - cursors on the map and on search results - batch reads with a shared byte budget, per-item errors, and identical ranges read once and shared rather than re-billed - an exhausted budget is reported, never silently truncated - reading is separated by representation, so patch offsets can never be handed back as original-file line numbers - `ReviewSnapshot.search` locates matches without returning their text, so searching issues no evidence and leaves returned-line coverage honest; the query is a literal substring, never a caller-supplied regular expression Still outstanding for C18: definitions, references, history, discussion, snapshot/version queries, cache authorization and rehydration.
The typed tools had no caller: a run captured its evidence inline and threw it
away, leaving external callers nothing to address. Retain each capture in a
bounded, in-memory store and serve it on the existing surfaces.
- `SnapshotStore`: LRU + TTL retention, no persistence. A caller may pin the
revision it captured against; a mismatch is reported stale rather than
silently served a different generation.
- `/api/ai/evidence[/:id/{map,read,search}]`, mirrored by the `ai_evidence_*`
MCP tools, all annotated read-only.
- Listing and mapping are not reading, so neither adds to returned-line
coverage; search returns positions only, so a match must be read before it
can be cited.
Documents the tools in the MCP schema reference.
Definitions and references need a real language server, so this adds one, discovered on PATH like the provider runtimes and reporting itself unavailable rather than pretending when absent. The client speaks only initialize, definition and references. It correlates every response by id, bounds frames, total bytes, pending requests, locations and time, and never grants the server authority: a server-initiated request is answered "method not found", its notifications are dropped, and no workspace edit, command or file write is accepted. A server that dies mid-request fails the session instead of surfacing an unhandled stream error. `locateInSnapshot` maps returned locations onto the capture. A language server may point anywhere in the repository, so a location outside the captured evidence is named but marked out of scope and never read — navigation cannot widen a run's scope. Non-file URIs, paths outside the repository root, and traversal segments all resolve to out-of-scope, and a patch source is never resolved, since its lines are not file lines.
The published skills carry a tool and route catalog that the contract test holds to the server and MCP registrations. Adding the evidence surfaces without updating it broke that contract. Documents `ai_evidence_*` and the four `/api/ai/evidence*` routes, corrects the note that claimed every AI surface is HTTP-only, and mirrors the file into the repo-local skill tree the byte-identical check compares against.
Adds the configuration, pooling and surfaces that make the LSP client usable. - `settings.aiLanguageServers` maps a file extension to a server. It is empty by default: no toolchain is presumed, so lookups report themselves unavailable until one is configured. The settings file is user-editable, so entries are validated on load and a malformed one is dropped rather than repaired; the command is resolved on PATH and never run through a shell. - `LanguageServers` starts a server at most once per command and reuses it, since a cold start costs far more than a query, and shuts it down once idle so a review does not leave servers running. Its idle timers are unref'd. - `lookupSymbols` maps every returned location back onto the capture and reports how many fell outside it. An absent or failing server yields an explicit `unavailable` result rather than an empty one, so a caller never reads "no server" as "no references". A patch source is refused outright, since its offsets are not file positions. - `POST /api/ai/evidence/:id/symbols` and the `ai_evidence_symbols` MCP tool, both read-only, documented in the catalog and the settings reference.
Completes the addressable half of the evidence surface. `review.map` now names the generation it describes, so a caller can tell which local, PR or plan capture it is holding rather than inferring it from a revision string alone. `verifyCitation` re-checks a stored citation against a capture: one this capture never issued is invalid, and one held against another generation is stale. Neither is silently accepted, so an agent that saved evidence can confirm it still anchors instead of trusting it. Exposed as `POST /api/ai/evidence/:id/verify` and the `ai_evidence_verify` MCP tool. Also adds authorization coverage for every evidence route, asserted against the real app rather than a stand-in, and updates the settings fixture for the new `aiLanguageServers` default.
The outgoing URI was concatenated, so a path containing a space or other character needing percent-encoding produced a malformed URI that no language server would resolve, while returned locations were already being decoded. Build it with pathToFileURL so both directions agree.
Completes the read-only tool list. `sourceHistory` lists commits touching a captured source. It is addressed by snapshot key, never by a caller-supplied path, so a run can only ask about files it was already given, and it returns commit metadata only — the git invocation passes `--no-patch` and separates the path with `--` after `--end-of-options`, so history cannot become a way to read content the capture withheld. A failing invocation is reported as missing rather than leaking git's diagnostics. Records use ASCII unit/record separators, so a subject containing one is still parsed intact, and an unparsable record is rejected rather than turned into an invented commit. `sourceDiscussion` lists review threads anchored to captured paths. Threads on paths outside the capture are counted but never returned, bodies are bounded and marked when truncated, and comment text is treated as human-written data rather than instructions to the run. Both are exposed as read-only routes and MCP tools, paged by cursor, and documented in the catalog.
Evidence built from a patch alone cannot cite a real file line, so the local capture now reads the originals behind each changed path at the exact revisions it identified, and `resolveDiffSnapshot` carries them alongside the patch. Revision correctness, proven against real git rather than hand-written patches: staged reads the index and ignores a dirtied working tree, working reads the tree, revision mode reads each side at its own commit, and a rename reads each side at its own path. Additions have no old side and deletions no new side. Honesty rules: - A git object read is `recorded`; a working-tree read is `reconstructed`, since it is optimistic. Those contents are hashed at capture and re-read in `assertFresh`, because the index hash does not cover them. - `mixed` mode still asserts no single old/new pair, so originals are omitted rather than attributed to a revision that does not exist. - Binary, oversized and unreadable paths are omitted explicitly, on the source and in the capture's omission list. - The patch sources keep saying their offsets are not file lines, and the capture stops claiming no original coverage once originals exist. Fixes a real defect found by the repeated-path fixture: a path occurring twice in one patch produced colliding snapshot keys, which `ReviewSnapshot` rejects. The first occurrence now wins and later ones are not re-read.
Asserts the invariants the evidence surface depends on, rather than one code path, so a change that widens authority has to break a test to land. - Scope: a source outside the capture cannot be read, a line range cannot reach past what was captured, out-of-repository and traversal URIs and non-file schemes never become readable, discussion outside the capture is counted not returned, and the git path comes from the capture rather than the caller's key. - Diagnostics: snapshot and language-server messages come from fixed sets, and no caller-supplied key, query, cursor or git failure text is echoed back. - Authority: the evidence modules reference no filesystem, process or network API; history runs execFile with an argv, `--no-patch` and `--end-of-options` and never a shell; every `ai_evidence_*` MCP tool is annotated read-only; and the language-server client advertises no mutating capability. - Freshness: a citation from another generation is stale, one minted against a different capture is invalid, and a patch source cannot be read as file lines.
Parity was documented but never enforced, so a tool could drift from the route it calls, or a route could exist that no client can reach. Every `ai_evidence_*` tool must call a route registered in server.ts with a matching method, no evidence route may be unreachable from MCP, and no evidence route may be registered with a mutating verb.
The legacy baseline buys its byte reduction by discarding evidence: on the large fixture it drops the `unchanged-conversion` line the review needs. That was documented as "not an acceptable optimization" and left there. Measures the alternative the typed tools make possible — map the capture, locate each required text, read only those lines — over the same fixtures so the two are comparable rather than separately reported. Measured, on identical fixtures: - Small: 1,764 bytes retrieved versus 6,627 prompt; evidence preserved. - Large: 1,794 bytes retrieved versus 101,214 prompt; evidence preserved, including the line the legacy prompt drops. `returnedLines` counts lines a read actually returned, so it is evidence coverage and never a claim about model attention. These remain measurements on synthetic fixtures: `humanApprovedThresholds` stays false, browser and UI responsiveness are still unmeasured, and no threshold is asserted as passed.
Driver spike first, since the plan required one before choosing: `node:sqlite` does not exist on Node 20, which `engines` still supports, and better-sqlite3 is a native module needing prebuilds for all seven packaged platforms. Neither is acceptable for a local-first CLI today, so this is built on the append-only journal the rest of the product already persists with, behind a narrow interface so a SQLite driver can replace it without touching callers. Each durability property is tested against a real filesystem: - Idempotent: a repeated key returns the stored record and writes nothing, across a reload and under concurrent duplicates of the same key. - Concurrent: appends are serialized, so lines never interleave and one failure does not stall or lose the writes after it. - Replayable and recoverable: a torn trailing line is discarded and reported by line number, everything before it is kept, appends continue afterward, and a record whose checksum does not match its payload is not trusted. - Non-destructive: a journal from a newer version is refused and left byte for byte intact, and compaction keeps the previous journal alongside. - Honest under disk failure: an I/O error is typed, leaves no half-applied record, and never leaks a path or errno into its message. Single-writer by design, which the existing server lock already guarantees. Not yet wired into the run path; legacy conversation data is untouched.
- `aiEvidenceTools` gates the evidence routes and, through them, the MCP tools that call them. It is read per request, so rollback takes effect without a restart, and answers not-found so a disabled build does not advertise the routes. Disabling it leaves model listing and runs untouched. - Default-provider gate: the five shipped providers must stay exactly Codex, Claude Code, OpenCode, Cursor and Grok, and the dormant direct OpenAI and Anthropic paths must stay out of the defaults. Drift now fails a test. - Packaging gate: the declared Node floor stays `>=20`, no SQLite or language server becomes a runtime dependency, and no AI module imports `node:sqlite`, which does not exist on that floor. - `docs/ai-operations.md` covers the flags, language-server configuration, storage rollback/recovery/migration, provider defaults, the verification commands and an incident checklist — including that an unavailable language server is not the same as no references, and that the baseline runners are not acceptance gates.
Wires the storage engine into the run path, so a run's boundaries survive a restart instead of living only in memory. `streamAiRun` records a `request` when the run starts and a `result` or `error` when it terminates, keyed `<runId>:<kind>` so a retry cannot duplicate. Deltas are deliberately not journaled — one record per token would be a firehose, and the terminal record already establishes what the run produced. A failure that never reached a terminal event is still recorded. Journalling is best-effort: a storage failure is swallowed so it can never take down a live run, which is asserted by test. `createApp` takes the store as an optional dependency and journals nothing without it, so an embedded app — and the test suite — never writes to a user's storage directory. `startServer` supplies the real per-repo store. An earlier draft of this constructed the store inside `createApp`, which wrote a journal into `~/.diffing` during the test run.
A PR capture held only the unified patch, so evidence built from it could not cite a real file line — the gap left when local originals landed. Reuses the local originals capture in revision mode with a GitHub-backed blob reader, so renames, additions, deletions, omissions and deduplication behave identically on both surfaces. The old side reads the merge base when one is known, since that is what the PR is diffed against. Every read costs a network round trip, so this is stricter than local: at most 20 files, an explicit omission for anything past that instead of refusing the capture, and a byte-bounded cache keyed by host/owner/repo/revision/path so a repeated capture of the same PR re-fetches nothing. A fetch failure or binary content is an absent original recorded as incomplete, never a failed capture and never empty content presented as real. Also lets the originals bound be lowered per capture, with local keeping its existing refusal since a local read is cheap.
Parity was HTTP and MCP only, and the catalog said so. `diffing evidence <list|map|read|search|symbols|verify|history|discussion>` now reaches the same routes, so all three transports read the same bounded evidence. Batch reads are mirrored properly: `--range KEY:START:END` repeats, so one invocation can read several ranges under the shared budget rather than losing the batching the tool layer provides. The key is split from the right, so a key containing a colon still parses. The parity test now asserts three-way coverage — every registered route must be reachable from MCP and documented as a CLI resource — and the usage text must list every resource. Usage errors exit 5 with a specific message. Because the CLI calls the HTTP routes, `aiEvidenceTools: false` disables it along with the MCP tools; the operations doc and catalog say so.
Findings, proposals and questions that cannot assert what the capture does not support. Every entry carries at least one citation; each cited range is re-read and its quote compared byte for byte, and each citation is verified against the snapshot that issued it. A fabricated quote, a citation minted against another capture, and a range reaching past the captured lines are all rejected rather than stored and later rendered as if they were evidence. Two separations the tests pin down: - Authoring is not deciding. A run may write entries; only `decide` records an outcome, and it records who decided it and when, without mutating the notebook it was given. - Coverage is not quality. What is reported is the lines reads returned, with the basis stated verbatim. Verifying a quote re-reads lines already returned, so the coverage union cannot be inflated by verification — asserted by test. Explicit links must resolve to a real entry and cannot be self-referential, so a notebook never implies a relationship to something absent. Stale re-review reports a notebook built on a replaced capture as wholly stale, and otherwise names only the entries whose cited text has drifted. This is the validated model. Authoring surfaces and storage are deliberately not wired: who may write entries is a product decision, not one to guess.
The file-mention dropdown intercepted ArrowDown, ArrowUp, Enter, Tab and Escape whenever it was open, with no composition guard. While an input method is composing, those keys belong to it: arrows move through the candidate list and Enter commits a candidate. Intercepting them replaced a CJK user's chosen candidate with a file mention — silent input corruption, not a cosmetic issue. The handler now yields while composing, honouring both `isComposing` and the legacy `keyCode === 229` signal browsers use when it is unavailable, and resumes handling once composition ends. The tests first written for this passed vacuously: the dropdown was never opened, so nothing was asserted. They now assert it is open, and reverting the guard fails six of them.
Eight call sites passed `behavior: "smooth"` to scrollIntoView or scrollTo. The global `prefers-reduced-motion` stylesheet rule cannot help there: an explicit behavior option takes precedence over the CSS `scroll-behavior` property, so a user who asked for reduced motion still got animated scrolling in the diff minimap, plan and PR review panes, theme and font pickers, comment navigation and plan line sync. Routes them through a `scrollBehavior()` helper that resolves the preference, treats a missing or throwing `matchMedia` as no preference rather than failing, and never upgrades an explicitly instant scroll. `useScrollToNextFile` already checked the preference correctly; it now shares the helper rather than carrying a second copy of the query. A sweep test fails if any `src/ui` source reintroduces a literal smooth behavior, so this cannot silently regress. It skips type declarations such as `behavior: "smooth" | "auto"`, which are not calls.
The rail clamped to a fixed 320–720px regardless of the window, so a width persisted on a wide display overflowed a narrow one and left no room for the diff the rail exists to discuss. Nothing re-clamped when the window resized either, so narrowing a window kept the old width. `clampRailWidth` resolves a requested width against the viewport, always leaving a diff gutter, and lets a narrow window override the usual minimum rather than overflowing. When no usable rail fits it returns null so the caller collapses instead of rendering something unreadable. It is idempotent, so re-clamping on every resize event is stable, and it falls back to the minimum for a non-finite width rather than producing NaN geometry. The rail now re-clamps on mount and on window resize.
C22's safety property is testable without a browser: a run must never start unprompted, and the AI surface must never be able to publish. - No module under src/lib/ai or src/ui/ai may reference a GitHub mutation helper — merge, state change, metadata update, suggestion apply, or pending review comments — so no run path can reach publication however it is prompted. - The only inference trigger anywhere in the AI surface is `user`. Introducing a background trigger fails this test, which was verified by doing so. - The server-side refusal of a non-user trigger stays in both the request validator and the service.
I had been deferring this as an unspecified product decision. It is not: the approved plan defines all three activities and their bounds, so this implements that spec rather than inventing one. - Ask answers with citations and explicit unknowns; Investigate returns findings, questions and coverage; Propose drafts a comment, reply, summary, code suggestion or plan revision. - Investigate is the only iterative activity, so it is the only one with a step budget above one. Each is bounded in duration, steps and output, and an exhausted budget names which bound stopped it, so partial work is never presented as complete. - A proposal is inert. `requiresHumanApproval` is true for every proposal kind, the admitted shape is plain data exposing no operation, and a proposal must say what it drafts while a non-proposal may not. - Only an explicit user trigger admits an activity. Also asserts the plan's stated retrieval target as a test: at least 50% fewer serialized evidence bytes on the focused large diff, and only while every labeled evidence item survives — the target is not permission to drop context. Meeting it is still not a human-approved threshold, which the test also pins.
Another item I had called unspecified that the plan spells out. It enumerates the state matrix — empty/disconnected, queued/preparing, tool activity, streaming, interrupted, canceled, retry — and states the honesty rules for the activity list: report actual reads, outcomes, elapsed time and coverage, and do not invent percentages, pretend to expose private model reasoning, or animate a false success. Those rules are enforced structurally rather than by convention: - No progress fraction is computed, so a caller cannot render one. A test asserts no progress/percentage key or reasoning channel exists. - `succeeded` is true only after a terminal complete event actually arrived. A stream that merely stops is interrupted, never quietly successful. - Partial text from a canceled or interrupted run is preserved and labelled partial, rather than discarded or presented as the answer. - A retry is a new attempt with its own state and does not overwrite the one before it. - Elapsed time comes from an injected clock and never goes negative on skew.
The plan requires importing legacy history "without overwriting originals" and using "revisions and idempotency keys rather than whole-transcript last-write-wins saves". Both are enforced rather than intended. - The legacy store is only ever read. A test asserts it is byte for byte identical after an import, so a run can be repeated, interrupted or abandoned without damaging the history it came from. - Every turn is journaled under a key derived from its conversation, position and id, so a re-run adds nothing and an interrupted import resumes by writing only what never landed. Position is part of the key because legacy turns need not carry an id and two may be textually identical. - A turn that cannot be trusted is skipped and counted, never rewritten into something that would import cleanly, and an undated turn inherits the conversation's timestamp rather than inventing one. The report distinguishes imported from already-present, so an incomplete import is visible instead of reading as a clean one.
Completes the notebook: entries are now authored, stored and decided through the same surfaces as evidence. With an external agent as the producer — the architecture chosen for this PR — that agent can record cited findings and a person can decide them. - Entries are journaled on the durable store, so they inherit its idempotency, replay and recovery. Validation runs before the write, so an entry that cannot cite the capture is never stored, not stored then rejected. - Deciding is a separate record from authoring, so a writer cannot smuggle an outcome in with the entry it wrote. The latest decision applies and earlier ones stay journaled: changing your mind is a new record, never an erasure. - Read, author and decide are reachable over HTTP, MCP and the CLI, keeping the three-way parity the contract test enforces. Two of my own tests caught mistakes in this change and both were right: - The parity test failed because I added HTTP routes without the MCP and CLI mirrors. - The adversarial test failed because I put two writing tools in the `ai_evidence_*` namespace, which is asserted read-only. Renamed them to `ai_notebook_*` rather than weakening the invariant, and added an assertion that the writers are idempotent, non-destructive mutations.
The rail re-rendered and re-parsed every completed turn on each streamed token. The plan forbids exactly this — "avoid reparsing the entire transcript per token" and "memoize completed turns and completed Markdown blocks" — and on a long conversation it is the difference between a smooth stream and a stuttering one. Completed assistant turns render through a memoized component whose comparator is explicit: a turn re-renders only when its own id, text or copied state changes, never because a sibling is streaming. A test drives five tokens through a transcript and asserts the completed turns are not re-parsed once. The copy handler is now stable across renders too. Without that the memo would have bought nothing, since a fresh handler each render fails the comparator — the kind of detail that makes a memoization look right and do nothing.
The plan specifies both the card's contents — claim, uncertainty, evidence references, verification status — and the rule that governs them: "invalid citations are rejected or shown as unverified, not converted into authoritative findings." Verification is therefore presented, never assumed: - A citation absent from the verification map is treated as unverified, so omission cannot pass for confirmation. - An unverified citation is still shown, since dropping it would conceal what the claim rests on, and the card states how many could not be verified. - The card marks itself unverified structurally, with a border weight as well as a label, so it does not depend on color alone. Styling uses Gridline tokens rather than literal colors, which a test enforces so a card cannot drift from the theme or break in dark mode. Long quotes scroll inside the card rather than overflowing the rail.
The plan's compact, expandable account of what a run actually did, with its prohibition enforced rather than intended: "do not invent percentages, pretend to expose private model reasoning, or animate a false success." - No progress fraction can reach the DOM, because the activity model computes none. A test asserts no percentage text, no `progress` element and no progressbar role is rendered. - A terminal phase that is not success states itself — interrupted, canceled and failed each carry their own border weight as well as a label, so an interrupted run does not read like a finished one at a glance. - Partial output is labelled partial, so a reader is not left assuming the visible text is the whole answer, and a retry names its attempt number. - Steps report the reads and tool outcomes that actually happened, with failures distinguished, and an empty list says so rather than implying work. Status is announced through a visually-hidden live region, so assistive technology hears the phase change without rereading the transcript on every token.
The assistant's thinking indicator loops infinitely and nothing anywhere observed page visibility, so it kept animating in a background tab. The plan requires activity animation to pause "when inactive/hidden"; browsers throttle such animations but do not stop them. CSS cannot observe visibility, so the state is reflected onto the root element and the stylesheet pauses on that. Reflecting state rather than stopping each animation means one added later is covered without anyone remembering to opt in, and pseudo-elements are included for the same reason. A page that is already hidden when observation starts is marked immediately, so a first paint in a background tab is paused rather than animating until the first visibility change. Installed before the app renders, which a test pins. No visible change while the page is visible.
The resize handle advertised aria-valuemin 320 and aria-valuemax 720. Those were correct until this branch clamped the rail to the viewport; since then a narrow window has had a lower real maximum and can force a minimum below 320, so the announced range no longer matched what a resize could produce. A screen-reader user was told a range that does not exist. `railWidthBounds` derives the announced range from the same rule the clamp uses, and a test asserts every width the clamp can return falls inside the announced bounds, so the two cannot drift apart again. This was a defect introduced earlier in this branch, not a pre-existing one.
My earlier clamping fix covered only the keyboard path. Dragging the resize handle still used the hardcoded 320-720 range, so the more commonly used path could still overflow a narrow window and squeeze out the diff — an incomplete fix that looked complete because the keyboard test passed. Both paths now share the same rule. A test drives each of them in a 900px window and asserts no persisted width exceeds what the viewport allows, and reverting the drag clamp fails it. Also asserts the handle announces that window's real maximum, so the ARIA range and the two resize paths cannot drift apart independently.
Puts the reviewed parts together — completed turns, the active response, run activity, cited findings, retry — so there is something concrete for the human visual review the plan requires before rollout. Deliberately not wired into the rail. The plan says review "must confirm that the result still looks and feels like diffing before rollout", so replacing what users see today is not mine to do unprompted. A test asserts the rail does not reference it, so this cannot become the shipped path by accident. Composition rules that are not cosmetic: - Completed turns render through the memoized component, so a streamed token does not re-parse the transcript above it. - The active response is kept out of the completed list, so a stream cannot rewrite settled history. - Retry appears only after a terminal failure, and says it starts a new attempt and keeps the previous one, matching the model's behaviour. I first added an `aiTranscriptShell` setting to gate this and then removed it: nothing reads it, and a setting no code consumes is worse than no setting.
Responsiveness measured as an algorithmic property rather than a stopwatch. Wall-clock timings in jsdom say nothing about a real browser, but the shape of the work does: if a streamed token re-parses completed turns, cost grows with conversation length and a long review degrades. Counting parses measures that directly and does not flake. Streaming ten tokens into transcripts of 5, 50 and 200 turns re-parses nothing in every case, and ten times the tokens costs the same as one — so the work is bounded by the active response, not by history. This is the honest half of C24's UI target: it proves the scaling property without claiming browser latency numbers this environment cannot produce.
The review UI now renders completed turns, the in-flight response, run activity, cited findings, and retry-after-failure on the shipped rail instead of leaving TranscriptShell unwired.
Cancel and error used to leave the rail in a terminal phase that kept the New conversation, switch, and delete buttons enabled while those handlers still required idle, so they no-op'd. They now match start(): clear the pending attempt and run from a terminal-failure phase.
Browser tests now drive ask, stream, retry and unverified findings on diff, PR-diff and plan. Live compatibility is a real discovery/ping probe that never flips liveVerified. Latency and heap are sampled; ceilings stay unapproved.
A Tip: footer wiped the Cursor model list, and thinking events aborted live pings as protocol errors. Those are product bugs. Claude 429 and a missing Grok key are still environmental; liveVerified stays false.
The CSRF Origin check required an exact match with the backend URL, so pnpm dev (UI on :5173, API on :3433) 403'd every /api call. Loopback http(s) origins are accepted when the server itself is loopback-bound; off-loopback Origins still have to match.
Replace the boxed 3x3 dots with ⠋⠙⠹⠸⠼⠴⠦⠧⠇⠏ and drop the thinking chip background so it sits in the transcript instead of a pill.
Add license, metadata, and user_invocable so skills.sh validate passes. Wire evidence/notebook tools through the router and review recipe, note the Vite loopback Origin rule, and keep published and repo-local trees byte-identical.
ahmedragab20
marked this pull request as ready for review
September 8, 2026 12:01
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
AI review foundation (full scope)
Reviewer's guide
45 commits sit on top of the original checkpoint (head
e5e4436). Humanreview of the five decisions and the shipped rail is in. All 14 acceptance
criteria are treated as covered for merge. Read the groups; the commit
messages carry the reasoning.
Read these first — decisions made without you. Each is reversible and each
is where I'd most want to be corrected:
b249754b2c2dc3e088497node:sqlitedoes not exist on Node 20, whichenginesdeclares;better-sqlite3needs prebuilds for seven platforms. Behind a narrow interface, so a driver swap does not touch callers.593d18faiEvidenceToolsto dark-ship instead; it is read per request, no restart.bef86e8Defects fixed, with the failure each would have caused. These are the
commits most worth spot-checking, because they change behaviour users see:
3b80babcodexbinary — vitest 4 does not propagatevi.mock("node:child_process"). 33 failing/243s → 34 passing/2.4s.07af36f85b826dprefers-reduced-motion; the option overrides the CSS reset.2edc8f4,4b2338e76c830fReviewSnapshotrejects.28b029b81df71b05f2119:5173403'd every/apicall (request Origin is not allowed).1fa456b--list-modelsTip:footer wiped the catalog; thinking frames aborted live pings.c5a0703Three of those were mine, introduced earlier in this same branch and caught
by later audit: the stale ARIA resize range (
98e226d), the clamp that coveredonly the keyboard path (
4b2338e), and a memoization that would have donenothing because the handler identity changed each render (
28b029b). Thedefect rate in what I added is not zero, which is the honest argument for
reading the diff rather than trusting the test count.
Guards worth understanding, because they will fail on you later. Each
exists because it caught something real:
ec62075(scope/egress/authority),0bc940f(HTTP/MCP/CLI parity — it failed when I added routes withoutmirrors),
a4a75da(no automatic inference, no reachable publication),593d18f(provider defaults and Node floor).Shipped on the rail:
TranscriptShellis mounted byAiAssistantRailon diff, PR-diff, and plan. Completed turns, the in-flight response, run
activity, cited findings, and retry-after-failure are one column. A streamed
token does not re-parse completed turns. Unverified citations stay unverified.
Thinking uses a braille spinner (
⠋⠙⠹⠸⠼⠴⠦⠧⠇⠏) with no chip background(
061e28c). Human visual review received.Agent cookbook (
e5e4436):skills/is skills.sh-valid (license,metadata,user_invocable) and byte-identical with.agents/skills/.Evidence/notebook tools, the Vite loopback Origin rule, and “never
POST /api/ai/run” are in the router and review recipes.Status
Ready for review. All 14 acceptance criteria are covered. C16 live ping is Codex/OpenCode/Cursor ok; Claude 429 and a missing Grok key are waived. C24 measurements are recorded and not CI gates (
humanApprovedThresholdsstays false). C27 is waived — synthetic replay is not a live quality score, and the human has closed the item.Verification (current)
pnpm exec tsc --noEmitpnpm test:ai:e2epnpm --silent compat:ai -- --pingliveVerified: false./skills.sh validateThe single failure is
src/__tests__/native-bundle.test.ts— untouched by this branch (git diff main..HEADis empty for it). It assertsnpm_execpathis set and needs seven prebuilt native TUI binaries, so it is environmental, not a code defect. It must still pass in CI where those binaries exist.The previously deferred Codex execution regression / 90-second timeout is resolved and was not a product bug. Under vitest 4,
vi.mock("node:child_process")does not propagate into imported modules because vite externalizesnode:*, socodex-run.test.tswas spawning the realcodexbinary and hanging. Provider spawning now goes through a localchild-process.jsseam that tests can mock: that suite went from 33 failing over 243s to 34 passing in 2.4s, andcodex-image-snapshot.test.tsfrom 2 failing to 2 passing.What this branch delivers
Lifecycle and providers (C15 met / C16 partial). Typed requests/events, terminal/error handling, cleanup, bounded streams, deadlines and cancellation ownership, and run-preparation admission. Capability/protocol/catalog safeguards for Codex, Claude Code, OpenCode, Cursor and direct Grok; defaults unchanged, dormant OpenAI/Anthropic paths disabled, unsupported settings gated. Live ping: Codex/OpenCode/Cursor ok; Claude 429; Grok disconnected (no key).
liveVerifiedstays false.Snapshots (C17, complete). Local/PR/plan identity, hashes, canonical patches, coverage, stale checks, explicit drafts and omissions. Local capture reads actual old/new originals under real-Git fixtures (staged, working, revision, rename, delete, add, repeated paths). PR captures read theirs at the merge base and head behind a bounded cache.
Typed read-only tools (C18, complete).
review.map,source.read,diff.read,source.search, definitions/references, history, discussion, and snapshot/version queries — with cursors, batch reads, a shared byte budget, per-item errors, deduplication and honest exhaustion. Design decisions worth review:review.mapandsource.searchissue no evidence and add nothing to returned-line coverage; search returns positions only, so a match must be read before it can be cited. Search is a literal substring, never a caller-supplied regex...traversal all resolve to out-of-scope.--no-patch, path separated by--after--end-of-options— so it cannot become a content-read backdoor. A failing invocation reports missing rather than leaking git diagnostics.verifyCitationrejects a citation this capture never issued as invalid and one held against another generation as stale; neither is silently accepted.External surfaces (C25). Captures are retained in a bounded LRU+TTL
SnapshotStoreand served at/api/ai/evidence[/:id/{map,read,search,symbols,verify,history,discussion}], mirrored by theai_evidence_*MCP tools and thediffing evidenceCLI, all read-only. Revision pinning returns stale rather than silently serving another generation. Every route sits behind the existing session auth, asserted against the real app.Configuration.
settings.aiLanguageServersmaps a file extension to a server, empty by default — no toolchain is presumed and lookups report unavailable until one is configured. Entries are validated on load, a malformed one is dropped rather than repaired, and the command is resolved on PATH and never run through a shell.Efficiency. Every codex run carrying reasoning effort, service tier or images previously spawned a second
codex app-serverpurely to validate those options. Successful catalogs are now retained for the same 15s the service uses and cleared through the existing invalidation path.Acceptance criteria
liveVerifiedstays false in the product.pnpm test:ai:e2enow covers ask/stream/retry/unverified-findings on diff, PR-diff, and plan across desktop, narrow, and reduced-motion, plus the typing/scroll baseline. Fixture-hosted, no live inference.humanApprovedThresholdsstays false.diffing evidence, none mutating, all documentedaiEvidenceToolsrollback flag, storage migration/rollback, default-provider drift gate, anddocs/ai-operations.mdnot-adjudicated;qualityThresholdsApprovedstays false.Notes for the next agent
Do not restart the old autonomous goal. Do not delete or skip Codex tests. Keep the five provider defaults unchanged and dormant OpenAI/Anthropic disabled. No GitHub apply/publish.
liveVerifiedstays false.No remaining acceptance-criteria blockers. Do not un-wire
TranscriptShell.liveVerifiedandhumanApprovedThresholdsstay false in code. After pulling, restartpnpm dev:serverso the Vite Origin fix is live.Verification: