From 40282e2fecef278a8fbb280d904a1f48d5025b1e Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Sun, 9 Aug 2026 20:40:43 -0700 Subject: [PATCH] Harden Wave 2 picker and compaction review follow-ups Model accept now uses the filtered row id only, empty filter sentinels cannot toggle favorites, and re-read stubbing only considers kept turns with path+offset+limit identity so chunked reads stay whole. Docs, changelog, and executable bit for the vendored patch ledger catch up. --- CHANGELOG.md | 14 ++++ bin/vendor-patch-diff | 0 docs/TUI.md | 8 +-- src/session/compactor.ts | 93 +++++++++++++++++------- src/tui/product-host.test.ts | 67 +++++++++++++++++- src/tui/product-host.ts | 24 +++---- src/tui/shell.ts | 4 +- tests/unit/compactor-pairing.test.ts | 102 ++++++++++++++++++++++++++- 8 files changed, 261 insertions(+), 51 deletions(-) mode change 100644 => 100755 bin/vendor-patch-diff diff --git a/CHANGELOG.md b/CHANGELOG.md index 7acca143e..88f61e913 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -37,6 +37,20 @@ Format loosely follows [Keep a Changelog](https://keepachangelog.com/). Versions mouse back for native terminal selection; Alt+C remains the keyboard copy path for whole messages, tool outputs, and diffs. +### Session + +- **Superseded read stubs.** When compaction keeps more than one successful + `read_file` of the same path (or same path+offset+limit range), older results + become a one-line stub and the newest stays whole. Errors stay verbatim. + Dedup only considers turns that survive compaction, so a summarized re-read + cannot hollow a kept older body. + +### Tooling + +- **Vendored patch ledger.** `bin/vendor-patch-diff` and + `vendor/intx-inference/PATCHES.md` site markers prove local patches against + upstream without a manual re-sync checklist. + ## [0.2.95] - 2026-08-09 Tool-only auto-pause that no longer stops healthy work, resume the last session diff --git a/bin/vendor-patch-diff b/bin/vendor-patch-diff old mode 100644 new mode 100755 diff --git a/docs/TUI.md b/docs/TUI.md index 8665d7442..a308e958f 100644 --- a/docs/TUI.md +++ b/docs/TUI.md @@ -424,6 +424,9 @@ narrows the list in place (printable keys claimed by the filter row, same pattern as the command palette); Enter selects. Escape closes the picker. The row matching the session's live active model gets a `(current)` suffix. Alt+F on a model row still toggles favorite when a favorite hook is wired. +While type-to-filter is active, bare `j`/`k` type into the filter rather than +moving the highlight — use arrow keys (or the filtered list's navigation) to +move. Onboarding (the standalone provider-setup screen, `provider-setup.ts`) and the satellite pickers used for session resume and session-mode selection @@ -663,11 +666,6 @@ asserted as fact: tiny-terminal, sub-24-row path) has a corresponding test that pins the exact row counts, or whether some of that path is only exercised indirectly. -- Whether the `(current)` marking on a provider *group* row - (`withGroupMark` in `openModels`, `product-host.ts`) is reachable and - correct in every case where the active model's provider itself has no - favorites/recents entry — the code path exists but was not traced through - a live picker session. - Full coverage of which chords are guaranteed deliverable on every terminal emulator Corbits Code targets (Shift+Enter and Alt+letter reporting depend on kitty-protocol negotiation the harness cannot test — see Test-harness diff --git a/src/session/compactor.ts b/src/session/compactor.ts index 3120401ce..4198023d8 100644 --- a/src/session/compactor.ts +++ b/src/session/compactor.ts @@ -207,7 +207,7 @@ export type CompactorConfig = { }; const DEFAULT_COMPACTOR_CONFIG: CompactorConfig = { - keepRecentTurns: 5, + keepRecentTurns: 6, summaryMaxChars: 2000, maxAnchorTurns: 8, }; @@ -231,11 +231,17 @@ const ANCHOR_SCORE_THRESHOLD = 5; // Tool names whose results are path-keyed for re-read dedup during compaction. const READ_TOOLS = new Set(["read_file"]); -// Call-id index for stub rendering (name + path). Not path-keyed — that is -// buildPathToReads below. +// Call-id index for stub rendering (name + path). Dedup keys live on `readKey`. type ToolCallInfo = { name: string; + /** Display path for stubs (always the raw path arg when present). */ pathArg?: string; + /** + * Dedup identity for re-read stubbing. Full-file reads share the path alone; + * ranged reads (offset/limit) get a distinct key so chunked reads of the same + * file do not hollow each other. + */ + readKey?: string; }; type PathRead = { @@ -245,7 +251,20 @@ type PathRead = { isError: boolean; }; -function pathArgFromArguments(raw: unknown): string | undefined { +function scalarArg(value: unknown): string { + if (typeof value === "number" && Number.isFinite(value)) return String(value); + if (typeof value === "string") return value; + return ""; +} + +/** + * Extract path + re-read identity from a tool_call's arguments. + * Identity is path alone for full-file reads; path+offset+limit when either + * range arg is present so partial reads don't supersede each other. + */ +function readIdentityFromArguments( + raw: unknown, +): { path: string; readKey: string } | undefined { let args: unknown = raw ?? {}; if (typeof args === "string") { try { @@ -255,8 +274,16 @@ function pathArgFromArguments(raw: unknown): string | undefined { } } if (args === null || typeof args !== "object" || Array.isArray(args)) return undefined; - const path = (args as Record)["path"]; - return typeof path === "string" && path.length > 0 ? path : undefined; + const rec = args as Record; + const path = rec["path"]; + if (typeof path !== "string" || path.length === 0) return undefined; + const offsetPart = scalarArg(rec["offset"]); + const limitPart = scalarArg(rec["limit"]); + const readKey = + offsetPart === "" && limitPart === "" + ? path + : `${path}\0${offsetPart}\0${limitPart}`; + return { path, readKey }; } // callId → tool name/path for readable stubs. Inverse of path-to-reads. @@ -266,8 +293,11 @@ function buildCallIndex(turns: readonly ConversationTurn[]): Map): Set { const superseded = new Set(); @@ -494,7 +530,7 @@ export function createPruningCompactor( return { name: "pruning-compactor", - version: "1.3.0", + version: "1.3.1", async apply( turns: ConversationTurn[], _ctx: StrategyContext, @@ -520,12 +556,9 @@ export function createPruningCompactor( }; } - // callId → name/path for stubs; path → ordered reads for re-read dedup. - // Only older successful reads of a path re-read later are stubbed — not a - // blanket strip of every kept tool_result (see CL-5595 / CL-4374). + // callId → name/path for stubs. Built over the full transcript so a kept + // result can still name its path even when its call turn was summarized. const callIndex = buildCallIndex(aged.turns); - const pathToReads = buildPathToReads(aged.turns, callIndex); - const supersededReads = supersededReadCallIds(pathToReads); const keepCount = Math.min(cfg.keepRecentTurns, aged.turns.length - 1); const keepFrom = aged.turns.length - keepCount; @@ -568,6 +601,12 @@ export function createPruningCompactor( const anchorTurns = sortedAnchorIndices.map((i) => olderTurns[i]!); const summarizedTurns = olderTurns.filter((_, i) => !anchorIndices.has(i)); + // Path-dedup only among turns that survive. Supersession over the full + // transcript would hollow a kept older read when the newer re-read is only + // in the summary (CL-4374 review follow-up). + const pathToReads = buildPathToReads([...anchorTurns, ...recentTurns], callIndex); + const supersededReads = supersededReadCallIds(pathToReads); + const summary = cfg.summarize !== undefined ? await cfg.summarize(summarizedTurns) : buildTurnSummary(summarizedTurns, cfg.summaryMaxChars, anchorTurns.length); @@ -584,11 +623,11 @@ export function createPruningCompactor( }; // Anchors and recent turns stay contentful except for path-dedup: when the - // same file was read successfully more than once, older results become a - // one-line stub and the newest stays whole. Error results are never - // stubbed. SummarizedTurns lose content wholesale via the summary above. - // Anchors are already image-aged (outside the recent window). Recent turns - // keep live base64 so a just-pasted screenshot still reaches the model. + // same file was read successfully more than once among kept turns, older + // results become a one-line stub and the newest stays whole. Error results + // are never stubbed. SummarizedTurns lose content wholesale via the summary + // above. Anchors are already image-aged (outside the recent window). Recent + // turns keep live base64 so a just-pasted screenshot still reaches the model. const process = (t: ConversationTurn): ConversationTurn => stubSupersededReads(t, supersededReads, callIndex); const output = coalesceAdjacentTextTurns([ diff --git a/src/tui/product-host.test.ts b/src/tui/product-host.test.ts index 56d4380df..b233baa02 100644 --- a/src/tui/product-host.test.ts +++ b/src/tui/product-host.test.ts @@ -333,9 +333,9 @@ describe("mountProductHost", () => { }) }) -describe("provider-first model picker", () => { - // Mirrors the bug-report shape: several providers, one (codex) with three - // accounts, plus a favorite so the top level has a reachable-without-descending pick. +describe("flat type-to-filter model picker", () => { + // Several providers, one (codex) with three accounts, plus a favorite so the + // top of the flat list has a reachable pick without typing. const providers = { "codex/abk-labs": { models: ["gpt-5.5", "gpt-5.6-sol"] }, "codex/dirtroad": { models: ["gpt-5.5", "gpt-5.6-sol"] }, @@ -524,6 +524,67 @@ describe("provider-first model picker", () => { harness.destroy() } }) + + test("Enter on a no-matches filter does not apply a model", async () => { + const { harness, host, selected } = await mountPicker() + try { + host.openModels?.() + await harness.renderOnce() + for (const ch of "zzzz-no-such-model") { + harness.pressKey(ch) + } + await harness.renderOnce() + expect(host.shell.overlayItems).toEqual(["(no matches)"]) + acceptOverlaySelection(host.shell) + expect(selected).toEqual([]) + } finally { + host.dispose() + harness.destroy() + } + }) + + test("filtered accept uses the filtered row id, not the unfiltered catalog index", async () => { + // Catalog order puts favorites/recents first; after filtering to "grok", + // index 0 is the grok row — accepting must still apply the grok id, never + // the catalog's index-0 favorite. + const { harness, host, selected } = await mountPicker() + try { + host.openModels?.() + await harness.renderOnce() + for (const ch of "grok") { + harness.pressKey(ch) + } + await harness.renderOnce() + // Accept whatever is focused after filter (should be the sole match). + acceptOverlaySelection(host.shell) + expect(selected).toEqual(["xai/thegreataxios:grok-4.5"]) + } finally { + host.dispose() + harness.destroy() + } + }) + + test("Alt+F on the no-matches sentinel does not toggle a favorite", async () => { + const favorites: string[] = [] + const { harness, host } = await mountPicker({ + onFavoriteToggle: (id) => favorites.push(id), + }) + try { + host.openModels?.() + await harness.renderOnce() + for (const ch of "zzzz-no-such-model") { + harness.pressKey(ch) + } + await harness.renderOnce() + expect(host.shell.overlayItems).toEqual(["(no matches)"]) + harness.pressKey("f", { meta: true }) + await harness.renderOnce() + expect(favorites).toEqual([]) + } finally { + host.dispose() + harness.destroy() + } + }) }) describe("mount failure", () => { diff --git a/src/tui/product-host.ts b/src/tui/product-host.ts index a4ca90d9e..14de84619 100644 --- a/src/tui/product-host.ts +++ b/src/tui/product-host.ts @@ -86,13 +86,9 @@ export type ProductHostDeliver = ( ) => void /** - * `section` groups rows for the provider-first picker: "recent" and - * "favorites" stay flat at the top (already single models, reachable without - * descending); "provider" rows are grouped into one top-level entry per - * provider (or per account, since each configured provider entry is already - * account-scoped — `codex/abk-labs`, `codex/dirtroad`); "unconnected" stays - * flat as a "connect →" row. Omitted (from a caller not using - * buildModelsFirstCatalog) falls back to one flat list, unwrapped. + * `section` tags catalog rows for grouping/ordering in `buildModelsFirstCatalog` + * (recent and favorites first, then provider models, then unconnected connect + * rows). The live picker is flat + type-to-filter — it does not nest by section. */ export type ProductHostModelOption = { readonly id: string @@ -131,7 +127,7 @@ export type ProductHostConfig = { * `models`/`describeModel` via `setModels` and reopens the picker. */ readonly onConnectProvider?: (providerName: string) => void - /** `f` on a focused model/provider row; absent rows (connect →) are skipped by the caller. */ + /** Alt+F on a focused model row; connect rows are skipped by the caller. Bare `f` is claimed by type-to-filter. */ readonly onFavoriteToggle?: (itemId: string) => void /** Command palette catalog (registry-backed). */ readonly commands?: readonly PaletteCommand[] @@ -519,8 +515,11 @@ export async function mountProductHost( // Flat list: type to narrow rather than drill into a provider pane. typeToFilter: true, onAccept: (sel) => { - const id = sel.id ?? items[sel.index]?.id - if (!id) return + // Prefer the stable id from the (possibly filtered) row. Do not fall + // back to `items[sel.index]` — that index is into the filtered list, + // not the unfiltered catalog, so it would pick the wrong model. + const id = sel.id + if (id === undefined || id.length === 0) return const providerName = id.startsWith("connect:") ? id.slice("connect:".length) : null if (providerName !== null) { onConnect?.(providerName) @@ -535,7 +534,8 @@ export async function mountProductHost( // Alt+F, never bare f — type-to-filter claims printable keys. const name = typeof key.name === "string" ? key.name.toLowerCase() : "" if (name !== "f" || key.ctrl || !(key.meta || key.option)) return false - if (itemId.startsWith("connect:")) return false + // Empty id is the "(no matches)" filter sentinel — not a model. + if (itemId.length === 0 || itemId.startsWith("connect:")) return false onFavoriteToggle(itemId) return true }, @@ -543,8 +543,6 @@ export async function mountProductHost( : {}), }) } - ;(shell as AppShell & { __openModels?: () => void }).__openModels = - openModels } const setModels = ( models: readonly ProductHostModelOption[], diff --git a/src/tui/shell.ts b/src/tui/shell.ts index c3c5116ac..6fbc982ad 100644 --- a/src/tui/shell.ts +++ b/src/tui/shell.ts @@ -1,8 +1,8 @@ /** * OpenTUI app shell — sticky transcript, prompt chrome, inset overlay. * - * Wave 3 product skin on the Wave 2 platform. Functional wrappers around - * @opentui/core class renderables. Not wired to production CLI; Ink remains production. + * Functional wrappers around @opentui/core class renderables. This is the + * production interactive CLI surface (Ink is no longer the live path). */ import { homedir } from "node:os" diff --git a/tests/unit/compactor-pairing.test.ts b/tests/unit/compactor-pairing.test.ts index 5a349d59f..62bf76bdf 100644 --- a/tests/unit/compactor-pairing.test.ts +++ b/tests/unit/compactor-pairing.test.ts @@ -99,7 +99,7 @@ describe("pruning compactor preserves tool_call/tool_result pairing", () => { }); }); -// CL-4374: when the same path is read more than once and both results survive +// When the same path is read more than once and both results survive // compaction (recent window / anchors), older successful reads become one-line // stubs; the newest successful read stays whole; error results stay whole. function assistantRead(id: string, path: string): ConversationTurn { @@ -192,4 +192,104 @@ describe("pruning compactor stubs superseded file reads (CL-4374)", () => { expect(resultText(output, "r1")).toBe(errBody); expect(resultText(output, "r2")).toBe(okBody); }); + + test("does not stub a sole kept successful read when a later re-read was summarized", async () => { + // Supersession is computed only over kept turns. A re-read that lands only + // in the summary must not hollow the surviving body. + const soleBody = "SOLE_KEPT_" + "s".repeat(200); + const discardedBody = "DISCARDED_" + "d".repeat(200); + const turns: ConversationTurn[] = [ + userText("start"), + assistantRead("old", "src/hot.ts"), + userReadResult("old", discardedBody), + userText("m1"), + userText("m2"), + userText("m3"), + userText("m4"), + assistantRead("kept", "src/hot.ts"), + userReadResult("kept", soleBody), + userText("end"), + ]; + // keep=3 → recent is kept call + kept result + end; the older pair summarizes. + const compactor = createPruningCompactor({ keepRecentTurns: 3, maxAnchorTurns: 0 }); + const { output } = await compactor.apply(turns, {} as never); + expect(resultText(output, "old")).toBeUndefined(); + expect(resultText(output, "kept")).toBe(soleBody); + }); + + test("does not stub ranged reads of the same path with different offset/limit", async () => { + const body1 = "RANGE_0_" + "a".repeat(200); + const body2 = "RANGE_50_" + "b".repeat(200); + const turns: ConversationTurn[] = [ + userText("start"), + userText("a"), + userText("b"), + userText("c"), + { + role: "assistant", + content: [ + { + type: "tool_call", + id: "r1", + name: "read_file", + arguments: { path: "src/hot.ts", offset: 0, limit: 40 }, + }, + ], + timestamp: 1, + }, + userReadResult("r1", body1), + { + role: "assistant", + content: [ + { + type: "tool_call", + id: "r2", + name: "read_file", + arguments: { path: "src/hot.ts", offset: 50, limit: 40 }, + }, + ], + timestamp: 1, + }, + userReadResult("r2", body2), + userText("d"), + userText("e"), + ]; + const compactor = createPruningCompactor({ keepRecentTurns: 6, maxAnchorTurns: 2 }); + const { output } = await compactor.apply(turns, {} as never); + expect(resultText(output, "r1")).toBe(body1); + expect(resultText(output, "r2")).toBe(body2); + }); + + test("stubs repeated identical-range reads of the same path", async () => { + const oldBody = "OLD_RANGE_" + "a".repeat(200); + const newBody = "NEW_RANGE_" + "b".repeat(200); + const rangeArgs = { path: "src/hot.ts", offset: 10, limit: 20 }; + const turns: ConversationTurn[] = [ + userText("start"), + userText("a"), + userText("b"), + userText("c"), + { + role: "assistant", + content: [{ type: "tool_call", id: "r1", name: "read_file", arguments: rangeArgs }], + timestamp: 1, + }, + userReadResult("r1", oldBody), + { + role: "assistant", + content: [{ type: "tool_call", id: "r2", name: "read_file", arguments: rangeArgs }], + timestamp: 1, + }, + userReadResult("r2", newBody), + userText("d"), + userText("e"), + ]; + const compactor = createPruningCompactor({ keepRecentTurns: 6, maxAnchorTurns: 2 }); + const { output } = await compactor.apply(turns, {} as never); + const older = resultText(output, "r1"); + expect(resultText(output, "r2")).toBe(newBody); + expect(older).toBeDefined(); + expect(older).not.toBe(oldBody); + expect(older).toMatch(/omitted|chars/); + }); });