From 5149d5d013ba32df2fb6c9eabed1f15fb5de04a3 Mon Sep 17 00:00:00 2001 From: Manas Raghuwanshi Date: Wed, 5 Aug 2026 19:38:50 +0530 Subject: [PATCH 1/5] docs(readme): show how the context work was measured The benchmark integration, the replay harness and the compaction result were only visible in source comments and CLAUDE.md. The README described the product and said nothing about how its defaults were chosen. States the finding that decides the default: compaction cuts peak prompt characters by 36-43% and is off, because implicit caching stopped entirely when it ran - 18.1M cached tokens of 23.2M became 1.1M of 11.6M. Numbers are taken from runtime/compaction.ts, not recomputed. Also states what the harness cannot answer, matching what it prints: cache rates belong to the original recordings and a modified assembly cannot inherit them. --- README.md | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/README.md b/README.md index 3e84a91..246448d 100644 --- a/README.md +++ b/README.md @@ -97,6 +97,47 @@ Only user and assistant messages are persisted, capped at the most recent messag History written by a version before sessions existed is imported once into a `legacy` bucket, reachable from the `/resume` picker with Ctrl+A. Resume it and take a turn and it becomes that project's session; open it only to read and it stays put. +## How it is measured + +Woopcode is benchmarked on [Harbor](https://github.com/laude-institute/harbor)'s +`terminal-bench-2` as an installed agent: the harness installs the published CLI +into the task container and runs `woopcode -p` once per task, so what is measured +is the thing users get rather than a bespoke harness build. `harbor_woopcode/` +holds the integration. + +Context changes are measured before they ship, against ten recorded benchmark +trajectories rather than against intuition: + +```bash +bun run replay:baseline +``` + +The harness replays each trajectory's prompt assembly and reports peak size and +what a given budget would have done — 932 iterations, no API calls, nothing spent. + +**The measurements decide the defaults, including against the obvious answer.** +Tool history is the only part of the prompt that grows: across the corpus, peak +prompt size ran from 22,639 to 219,179 characters while the system prompt, +repository context and conversation stayed flat. Compacting it works by the +character count — 36–43% off peak prompts at matched depth, confirmed live — and +it is **off by default**, because the same benchmark run cost a task that had +been passing. Reading the provider's own token counts back out of both runs +explained why: implicit caching stopped entirely, 18.1M cached tokens of 23.2M +becoming 1.1M of 11.6M, because rewriting the older messages moves the cache +prefix on every request. At iteration 200, uncompacted, only 16k of a 96k prompt +was billed at full rate; compacted, all 29k was. Peak characters fell by two +thirds for roughly no saving. + +The code, its tests and the measurements all stay — `WOOPCODE_TOOL_HISTORY_BUDGET` +enables it — but the default follows the billing, not the character count. +`runtime/compaction.ts` carries the full numbers and the two variants worth +trying next. + +What the harness cannot tell you is stated where it runs: it reports cache rates +observed for the original recordings, and a modified prompt assembly cannot +inherit them. The fixtures reconstruct prompt *sizes* faithfully; they are not a +conversation that can be replayed against a live provider. + ## Built-in tools Woopcode ships with a fixed set of tools, grouped by what they touch. From 44725bf4fd94ac0c91b7d90fb5b4c8fb738e3aa3 Mon Sep 17 00:00:00 2001 From: Manas Raghuwanshi Date: Wed, 5 Aug 2026 19:38:57 +0530 Subject: [PATCH 2/5] refactor(tui): drop the unreachable /exit branch in the prompt handleSlashCommand returns handled: true for every /-prefixed input - an unknown command included, which is answered with a message rather than falling through. /exit is a registered command (commands/slash/commands.ts), so the literal check for it below the handled guard could never run. Removes it and says at the call site what reaches the agent instead. --- tui/src/prompt.tsx | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/tui/src/prompt.tsx b/tui/src/prompt.tsx index ebc07ae..47f0ab5 100644 --- a/tui/src/prompt.tsx +++ b/tui/src/prompt.tsx @@ -198,7 +198,6 @@ export function Prompt({ // neither is a prompt the user would want ↑ to bring back. history.current.push(prompt); - // 🔥 Slash command interception const context = { controller, onExit, @@ -207,6 +206,8 @@ export function Prompt({ }, }; + // Every slash command, `/exit` included, is resolved by the registry. What + // reaches the agent below is what the registry did not claim. const result = await handleSlashCommand(prompt, context); if (result.handled) { @@ -214,20 +215,13 @@ export function Prompt({ return; } - // Original flow - if (prompt === "/exit") { - await onExit(); - return; - } - onValueChange(""); - - // Run the agent with error handling to keep app alive + try { await controller.run(prompt); } catch (error) { - // Error already displayed via callbacks.onError - // Just catch it here to prevent app crash + // The failure has already been rendered by callbacks.onError. Swallowed + // here only so a provider error cannot unmount the app mid-session. if (process.env.DEBUG) { console.error("Prompt handler caught error:", error); } From e4805dac7a38248edd62aa7ec1ae25ad5b773ee2 Mon Sep 17 00:00:00 2001 From: Manas Raghuwanshi Date: Wed, 5 Aug 2026 19:39:22 +0530 Subject: [PATCH 3/5] chore: drop comments that restate the code About fifty comments named what the next line already said - // Generate unified diff, // Check if path is a directory, // Format output. They carry nothing a reader does not have, and they crowd out the comments that record a decision. Removed, and a few replaced with the reason the code is shaped that way: the write in editFile and writeFile sits below both approval exits, and a logged-out provider cannot be left as the default. Also removes a commented-out console.log in the onDone callback, and hoists glob's dynamic import of statSync to the top of the file. --- commands/agent.tsx | 5 ++--- commands/slash/commands.ts | 7 ++----- commands/slash/parser.ts | 3 ++- commands/slash/registry.ts | 2 -- config/paths.ts | 1 - onboarding/index.ts | 1 - onboarding/setupWizard.tsx | 1 - tools/editFile.ts | 6 ++---- tools/glob.ts | 30 ++++++++++++------------------ tools/question.ts | 1 - tools/readFile.ts | 1 - tools/runTests.ts | 3 ++- tools/webFetch.ts | 23 ++++++----------------- tools/writeFile.ts | 7 ++----- tui/src/components/CodeBlock.tsx | 2 -- tui/src/components/Markdown.tsx | 1 - 16 files changed, 30 insertions(+), 64 deletions(-) diff --git a/commands/agent.tsx b/commands/agent.tsx index c723c06..84876b7 100644 --- a/commands/agent.tsx +++ b/commands/agent.tsx @@ -293,10 +293,10 @@ async function runInteractive( modelOverride?: string, session: InitializeOptions = { continueLatest: true }, ) { - // Register slash commands registerCommands(); - // Ensure provider is configured (launches onboarding if needed) + // Launches onboarding when nothing is configured, so this may not return + // immediately on a first run. const { provider, apiKey } = await ensureProviderConfigured(); const config = await getConfig(); @@ -375,7 +375,6 @@ async function runInteractive( }, onDone() { - //console.log("onDone received"); store.finishAssistantMessage(); store.setStatus("Ready"); }, diff --git a/commands/slash/commands.ts b/commands/slash/commands.ts index 4a7ce42..2847ab3 100644 --- a/commands/slash/commands.ts +++ b/commands/slash/commands.ts @@ -346,7 +346,6 @@ const loginCommand: SlashCommand = { return unsupportedProviderMessage(provider); } - // Validate API key const { loginProvider } = await import("../../config/authProvider"); const isValid = await loginProvider(provider, apiKey); @@ -358,7 +357,6 @@ const loginCommand: SlashCommand = { return `Cannot change provider while the agent is running. Press Esc to cancel first.`; } - // Save the API key config.providers[provider].apiKey = apiKey; config.defaultProvider = provider; @@ -408,12 +406,11 @@ const logoutCommand: SlashCommand = { return `Cannot log out of the active provider while the agent is running. Press Esc to cancel first.`; } - // Remove API key delete config.providers[provider].apiKey; - // If logging out from default provider, clear default + // Leaving a logged-out provider as the default would send the next turn at + // a provider with no key, so hand the default to one that still has one. if (config.defaultProvider === provider) { - // Find another logged-in provider const otherProvider = Object.entries(config.providers).find( ([name, details]: [string, any]) => name !== provider && details.apiKey && isProviderEnabled(name) diff --git a/commands/slash/parser.ts b/commands/slash/parser.ts index 4187c2b..fa9b64d 100644 --- a/commands/slash/parser.ts +++ b/commands/slash/parser.ts @@ -3,7 +3,8 @@ import type { ParsedCommand } from "./types"; export function parseInput(input: string): ParsedCommand { const trimmed = input.trim(); - //discovery mode + // A bare slash lists what is available rather than failing as an unknown + // command, which is what makes the commands discoverable at all. if (trimmed === "/") { return { type: "discovery", originalInput: input }; } diff --git a/commands/slash/registry.ts b/commands/slash/registry.ts index b9ac183..84ce8fb 100644 --- a/commands/slash/registry.ts +++ b/commands/slash/registry.ts @@ -27,7 +27,6 @@ export class SlashCommandRegistry { return this.getAll().filter((cmd) => cmd.category === category); } - // Auto-generated help generateHelp(): string { const categories = { session: "Session", @@ -55,7 +54,6 @@ export class SlashCommandRegistry { return output.trim(); } - // Discovery list generateDiscoveryList(): string { return this.getAll() .map((cmd) => `/${cmd.name}`) diff --git a/config/paths.ts b/config/paths.ts index 9766297..4c49ea6 100644 --- a/config/paths.ts +++ b/config/paths.ts @@ -36,7 +36,6 @@ export function getConfigDir(): string { configDir = join(xdgConfigHome, "woopcode"); } - // Ensure directory exists if (!existsSync(configDir)) { mkdirSync(configDir, { recursive: true }); } diff --git a/onboarding/index.ts b/onboarding/index.ts index 6a88b4e..ceac304 100644 --- a/onboarding/index.ts +++ b/onboarding/index.ts @@ -100,7 +100,6 @@ function runOnboarding(): Promise { }), ); - // Handle Ctrl+C gracefully const handleExit = () => { if (!hasCompleted) { unmount(); diff --git a/onboarding/setupWizard.tsx b/onboarding/setupWizard.tsx index 818171b..09bf348 100644 --- a/onboarding/setupWizard.tsx +++ b/onboarding/setupWizard.tsx @@ -71,7 +71,6 @@ export function SetupWizard({ onComplete, onError }: SetupWizardProps) { return; } - // Save configuration const config = await getConfig(); config.defaultProvider = selectedProvider.id; // A provider chosen in the wizard may have no entry yet. diff --git a/tools/editFile.ts b/tools/editFile.ts index fe61406..6819bce 100644 --- a/tools/editFile.ts +++ b/tools/editFile.ts @@ -102,12 +102,10 @@ export const editFileTool: Tool = { return `No changes needed for ${path}`; } - // Generate unified diff const diff = createTwoFilesPatch(path, path, content, updated, "", "", { context: 3, }); - // Create pending edit const pendingEdit: PendingEdit = { id: crypto.randomUUID(), filePath: path, @@ -117,7 +115,6 @@ export const editFileTool: Tool = { toolCallId: crypto.randomUUID(), }; - // Request approval from UI let approved: boolean; try { approved = await store.setPendingEdit(pendingEdit); @@ -133,7 +130,8 @@ export const editFileTool: Tool = { return message; } - // Write file after approval + // The only write in this tool, and it sits below both exits above. Nothing + // may move above them: the diff review is the product's whole guarantee. await Bun.write(path, updated); return outcome.replacements > 1 diff --git a/tools/glob.ts b/tools/glob.ts index c5ee215..830a8b3 100644 --- a/tools/glob.ts +++ b/tools/glob.ts @@ -1,5 +1,6 @@ import type { Tool } from "../config/types"; import path from "path"; +import { statSync } from "fs"; import { resolveWorkspacePath } from "./workspace"; export const globTool: Tool = { @@ -35,32 +36,27 @@ Returns up to 100 matching file paths.`, throw new Error("Pattern is required"); } - // Resolve search path const resolvedPath = await resolveWorkspacePath(searchPath, { mustExist: true }); - // Check if path is a file (not a directory) try { - const { statSync } = await import("fs"); - const stats = statSync(resolvedPath); - - if (stats.isFile()) { + if (statSync(resolvedPath).isFile()) { throw new Error(`glob path must be a directory: ${resolvedPath}`); } } catch (err: any) { - // If error is "glob path must be a directory", re-throw it + // Only the directory check is worth failing on. Anything else stat can + // raise means the path is not readable yet, which Glob reports itself as + // an empty scan. if (err.message?.includes("glob path must be a directory")) { throw err; } - // Otherwise, path might not exist yet, which is okay - Glob will handle it } - // Perform glob search with limit const LIMIT = 100; const files: string[] = []; - + try { const glob = new Bun.Glob(pattern); - + for await (const file of glob.scan({ cwd: resolvedPath, onlyFiles: true, @@ -68,23 +64,21 @@ Returns up to 100 matching file paths.`, if (files.length >= LIMIT) { break; } - - // Resolve to absolute path - const absolutePath = path.resolve(resolvedPath, file); - files.push(absolutePath); + + files.push(path.resolve(resolvedPath, file)); } } catch (error) { throw new Error(`Glob search failed: ${error instanceof Error ? error.message : String(error)}`); } - // Format output const output: string[] = []; - + if (files.length === 0) { output.push("No files found"); } else { output.push(...files); - + + if (files.length === LIMIT) { output.push(""); output.push( diff --git a/tools/question.ts b/tools/question.ts index 8b72fcd..2b14769 100644 --- a/tools/question.ts +++ b/tools/question.ts @@ -35,7 +35,6 @@ Examples: throw new Error("At least one question is required"); } - // Validate questions const validQuestions = questions.filter( (q) => typeof q === "string" && q.trim().length > 0 ); diff --git a/tools/readFile.ts b/tools/readFile.ts index 53e6f09..bbfafda 100644 --- a/tools/readFile.ts +++ b/tools/readFile.ts @@ -96,7 +96,6 @@ export const readFileTool: Tool = { throw Error(`File ${path} does not exist`); } - // Check if path is a directory try { const stats = statSync(path); if (stats.isDirectory()) { diff --git a/tools/runTests.ts b/tools/runTests.ts index 7ae49c8..040dc5d 100644 --- a/tools/runTests.ts +++ b/tools/runTests.ts @@ -24,7 +24,8 @@ export const runTestsTool: Tool = { return "Command rejected by user. It was not run."; } - // Warn about server commands + // A server never exits, so it would hold the tool open until the timeout + // rather than failing. Refused here instead, with the reason. if (command.includes("run src/index") || command.includes("run index") || command.includes("start")) { return "Error: This command appears to start a server. Use run_tests only for test suites, not for starting servers. Servers run indefinitely and will cause timeouts."; } diff --git a/tools/webFetch.ts b/tools/webFetch.ts index b629a18..9e423cd 100644 --- a/tools/webFetch.ts +++ b/tools/webFetch.ts @@ -43,7 +43,6 @@ Default timeout: 30 seconds`, const format = (args.format as string) || "markdown"; const timeoutSeconds = Math.min((args.timeout as number) || 30, 120); - // Validation if (!url || (!url.startsWith("http://") && !url.startsWith("https://"))) { throw new Error("URL must start with http:// or https://"); } @@ -81,27 +80,23 @@ Default timeout: 30 seconds`, throw new Error(`HTTP ${response.status}: ${response.statusText}`); } - // Check content length const contentLength = response.headers.get("content-length"); if (contentLength && parseInt(contentLength) > MAX_SIZE) { throw new Error("Response too large (exceeds 5MB limit)"); } const contentType = response.headers.get("content-type") || ""; - - // Handle images + if (contentType.includes("image/")) { return `Image fetched from ${url}\nContent-Type: ${contentType}\n\nNote: Image content cannot be displayed in text format.`; } - // Get content const text = await response.text(); if (text.length > MAX_SIZE) { throw new Error("Response too large (exceeds 5MB limit)"); } - // Process based on format let output = text; if (format === "text" && contentType.includes("text/html")) { @@ -152,13 +147,9 @@ function extractTextFromHTML(html: string): string { .replace(/<\/li>/gi, "\n") .replace(/<\/tr>/gi, "\n"); - // Remove all remaining HTML tags text = text.replace(/<[^>]+>/g, ""); - - // Decode HTML entities text = decodeHTMLEntities(text); - // Clean up whitespace text = text .replace(/\n\s*\n\s*\n/g, "\n\n") // Remove excessive newlines .replace(/[ \t]+/g, " ") // Normalize spaces @@ -167,10 +158,12 @@ function extractTextFromHTML(html: string): string { return text; } +/** + * A deliberately small subset of HTML, not a conformant converter: the output is + * read by a model, so an unhandled tag costs a little fidelity rather than + * breaking anything. `turndown` is the upgrade if that stops being true. + */ function convertHTMLToMarkdown(html: string): string { - // Basic HTML to Markdown conversion - // For production, consider using a library like 'turndown' - let md = html; // Remove script, style tags @@ -211,13 +204,9 @@ function convertHTMLToMarkdown(html: string): string { md = md.replace(/<\/p>/gi, "\n\n"); md = md.replace(//gi, "\n"); - // Remove remaining HTML tags md = md.replace(/<[^>]+>/g, ""); - - // Decode HTML entities md = decodeHTMLEntities(md); - // Clean up whitespace md = md .replace(/\n\s*\n\s*\n/g, "\n\n") .replace(/[ \t]+/g, " ") diff --git a/tools/writeFile.ts b/tools/writeFile.ts index 219f4f3..8d372d9 100644 --- a/tools/writeFile.ts +++ b/tools/writeFile.ts @@ -51,7 +51,6 @@ export const writeFileTool: Tool = { throw new Error(`File not found: ${path}`); } - // Read current content const oldContent = await file.text(); // If content is identical, skip diff preview @@ -59,12 +58,10 @@ export const writeFileTool: Tool = { return `No changes needed for ${path}`; } - // Generate unified diff const diff = createTwoFilesPatch(path, path, oldContent, content, "", "", { context: 3, }); - // Create pending edit const pendingEdit: PendingEdit = { id: crypto.randomUUID(), filePath: path, @@ -74,7 +71,6 @@ export const writeFileTool: Tool = { toolCallId: crypto.randomUUID(), }; - // Request approval from UI let approved: boolean; try { approved = await store.setPendingEdit(pendingEdit); @@ -90,7 +86,8 @@ export const writeFileTool: Tool = { return outcome; } - // Write file after approval + // The only write in this tool, and it sits below both exits above. Nothing + // may move above them: the diff review is the product's whole guarantee. await Bun.write(path, content); return `Updated ${path}`; diff --git a/tui/src/components/CodeBlock.tsx b/tui/src/components/CodeBlock.tsx index 25b8915..86dfa51 100644 --- a/tui/src/components/CodeBlock.tsx +++ b/tui/src/components/CodeBlock.tsx @@ -29,7 +29,6 @@ export function CodeBlock({ code, language }: CodeBlockProps) { } const lines = highlighted.split("\n"); - // Use theme colors for borders const borderColor = chalk.hex(colors.borderBase); const termWidth = Math.max((process.stdout.columns || 80) - 6, 30); @@ -49,7 +48,6 @@ export function CodeBlock({ code, language }: CodeBlockProps) { // ╰────────────────────╯ const bottom = borderColor(`╰${"─".repeat(boxWidth - 2)}╯`); - // empty padding row const empty = borderColor("│") + " ".repeat(boxWidth - 2) + borderColor("│"); // code rows — truncate long lines at inner width so the box never breaks diff --git a/tui/src/components/Markdown.tsx b/tui/src/components/Markdown.tsx index 71b51fb..088b7b9 100644 --- a/tui/src/components/Markdown.tsx +++ b/tui/src/components/Markdown.tsx @@ -275,7 +275,6 @@ function renderInline(tokens: Token[], md: MarkdownPalette): ReactNode[] { case "link": { const t = token as Tokens.Link; - // Render link text with accent color return ( {renderInline(t.tokens, md)} From e1c41562f8195ebba864727aa079b6ddaef7d421 Mon Sep 17 00:00:00 2001 From: Manas Raghuwanshi Date: Wed, 5 Aug 2026 19:42:39 +0530 Subject: [PATCH 4/5] refactor(tui): settle a pending edit through one path Three of the store's four modal flows resolve through a private resolvePendingX helper. The edit flow predated it and repeated the same resolver lookup, state clear and emit in approve, reject and clear. Routes all three through resolvePendingEdit. The outcome is named rather than boolean because cancelling rejects where declining resolves false, and editFile reads the difference: a rejection is reported as cancelled, a false as 'Edit rejected ... Do not claim this edit was completed'. Nothing covered that distinction. Mutating the rejection to resolve false left the whole suite green - the overlay tests call clearPendingEdit only as teardown and swallow the rejection. Adds two tests that fail under exactly that mutation, for cancellation and for clearTimeline. --- tui/src/store/ui-store.test.ts | 45 +++++++++++++++++++++++++++++ tui/src/store/ui-store.ts | 52 +++++++++++++--------------------- 2 files changed, 65 insertions(+), 32 deletions(-) diff --git a/tui/src/store/ui-store.test.ts b/tui/src/store/ui-store.test.ts index b31f594..87b7c1a 100644 --- a/tui/src/store/ui-store.test.ts +++ b/tui/src/store/ui-store.test.ts @@ -138,6 +138,51 @@ describe("UIStore edit approvals", () => { expect(store.getState().pendingEdit).toBeNull(); }); + /** + * Cancelling and declining are two different answers, and only the rejection + * distinguishes them. `editFile` catches it to report "Edit cancelled"; a + * resolved false takes the other branch and tells the model "Edit rejected + * ... Do not claim this edit was completed". Collapsing the two would report + * an interrupted turn as a refusal the user never made. + * + * The dismissal test below covers `dismissTopModal`, which declines. Nothing + * covered this path: the overlay tests call `clearPendingEdit` only as + * teardown and swallow the rejection, so resolving false here stayed green. + */ + test("cancelling a turn rejects the waiting edit rather than declining it", async () => { + const store = new UIStore(); + const decision = store.setPendingEdit({ + id: "edit-cancel", + filePath: "src/example.ts", + oldContent: "old", + newContent: "new", + diff: "@@ -1 +1 @@\n-old\n+new", + toolCallId: "tool-1", + }); + + store.clearPendingEdit(); + + await expect(decision).rejects.toThrow("Edit cancelled"); + expect(store.getState().pendingEdit).toBeNull(); + }); + + test("clearing the timeline cancels an edit left waiting on the screen", async () => { + const store = new UIStore(); + const decision = store.setPendingEdit({ + id: "edit-clear", + filePath: "src/example.ts", + oldContent: "old", + newContent: "new", + diff: "@@ -1 +1 @@\n-old\n+new", + toolCallId: "tool-1", + }); + + store.clearTimeline(); + + await expect(decision).rejects.toThrow("Edit cancelled"); + expect(store.getState().pendingEdit).toBeNull(); + }); + test("marks a failed tool instead of leaving a spinner running", () => { const store = new UIStore(); store.startTool({ id: "tool-1", name: "edit_file", arguments: {} }); diff --git a/tui/src/store/ui-store.ts b/tui/src/store/ui-store.ts index 4fc0fa5..c687a88 100644 --- a/tui/src/store/ui-store.ts +++ b/tui/src/store/ui-store.ts @@ -503,7 +503,6 @@ export class UIStore { this.emit(); } - // Pending Edit Management setPendingEdit(edit: PendingEdit): Promise { if (this.nonInteractive) { return Promise.resolve(this.nonInteractiveApproval); @@ -523,48 +522,37 @@ export class UIStore { } approvePendingEdit() { - const edit = this.state.pendingEdit; - if (!edit) return; - - const resolver = this.editResolvers.get(edit.id); - if (resolver) { - resolver.resolve(true); - this.editResolvers.delete(edit.id); - } - - this.state = { - ...this.state, - pendingEdit: null, - pendingEditScrollOffset: 0, - }; - this.emit(); + this.resolvePendingEdit("approved"); } rejectPendingEdit() { - const edit = this.state.pendingEdit; - if (!edit) return; - - const resolver = this.editResolvers.get(edit.id); - if (resolver) { - resolver.resolve(false); - this.editResolvers.delete(edit.id); - } - - this.state = { - ...this.state, - pendingEdit: null, - pendingEditScrollOffset: 0, - }; - this.emit(); + this.resolvePendingEdit("rejected"); } clearPendingEdit() { + this.resolvePendingEdit("cancelled"); + } + + /** + * Settles the pending edit and takes it off the screen. + * + * Cancelling rejects where rejecting resolves false, and the two are not + * interchangeable: `editFile` catches the rejection to report that the edit + * was cancelled, and returns the rather firmer "Edit rejected ... Do not + * claim this edit was completed" on a false. Collapsing them would tell the + * model a dismissed dialog was a refusal. + */ + private resolvePendingEdit(outcome: "approved" | "rejected" | "cancelled") { const edit = this.state.pendingEdit; if (!edit) return; const resolver = this.editResolvers.get(edit.id); if (resolver) { - resolver.reject(new Error("Edit cancelled")); + if (outcome === "cancelled") { + resolver.reject(new Error("Edit cancelled")); + } else { + resolver.resolve(outcome === "approved"); + } this.editResolvers.delete(edit.id); } From 7721587b363699464a59dadb294119d47ab94454 Mon Sep 17 00:00:00 2001 From: Manas Raghuwanshi Date: Wed, 5 Aug 2026 19:47:10 +0530 Subject: [PATCH 5/5] refactor(config): read, change and write the session index in one place The index read-modify-write was spelled out at five call sites - the sequence that has already lost a row once, per the count check in summariesFor that heals it. updateIndex holds it once. The fallback is a parameter rather than a default because the five did not agree, and the disagreement is load-bearing: saving rebuilds from the files so the new row joins the others, pruning starts from empty, and removing does nothing at all, since writing an index for a project with no sessions would create the directory lazy creation exists to avoid. None of the three was covered. Collapsing rebuild to empty, and skip to empty, both left the whole suite green. Adds a test per fallback, asserted against the index on disk rather than through listSessions, which would have healed all three and hidden them. Each fails under its own mutation. This does not close the window between the read and the write. Two processes still interleave; summariesFor is still what catches it. --- config/sessions.ts | 70 +++++++++++++++++++------- packages/tests/config/sessions.test.ts | 52 +++++++++++++++++++ 2 files changed, 103 insertions(+), 19 deletions(-) diff --git a/config/sessions.ts b/config/sessions.ts index 5d42296..1a29db0 100644 --- a/config/sessions.ts +++ b/config/sessions.ts @@ -361,6 +361,44 @@ async function writeIndex(slug: string, index: SessionIndex): Promise { await writeJsonAtomic(getSessionIndexPath(slug), index); } +/** + * What a caller means when the index it wanted to change is not there. + * + * Not a detail to default: saving a record rebuilds from the files on disk so + * the new row joins the existing ones rather than replacing them; pruning + * starts from empty because it is about to write the rows it kept; and removing + * a session does nothing at all, since there is no row to take out and writing + * an index here would create the directory lazy creation exists to avoid. + */ +type MissingIndex = "rebuild" | "empty" | "skip"; + +/** + * Reads a project's index, applies `mutate`, writes the result back. + * + * Every caller wants those three steps and no caller wants two of them, but + * each spelled the sequence out itself — five copies of a read-modify-write + * that has already lost a row once. This does not close the window between the + * read and the write; two processes still interleave, which is why + * `summariesFor` compares the row count against the files on disk and rebuilds + * when they disagree. It puts the pattern in one place, so the next change to + * it is one edit rather than five. + */ +async function updateIndex( + slug: string, + onMissing: MissingIndex, + mutate: (index: SessionIndex) => SessionIndex, +): Promise { + const existing = await readIndex(slug); + if (!existing && onMissing === "skip") return; + + const index = existing ?? { + version: SESSION_VERSION, + sessions: onMissing === "rebuild" ? await rebuildIndex(slug) : [], + }; + + await writeIndex(slug, mutate(index)); +} + /** Session files on disk for a project, ignoring the index entirely. */ function sessionFileCount(slug: string): number { const directory = getProjectSessionsDir(slug); @@ -481,16 +519,15 @@ async function writeSessionRecord(record: SessionRecord): Promise mkdirSync(getProjectSessionsDir(slug), { recursive: true }); await writeJsonAtomic(getSessionPath(slug, trimmed.id), trimmed); - const index = (await readIndex(slug)) ?? { - version: SESSION_VERSION, - sessions: await rebuildIndex(slug), - }; const summary = summarize(trimmed, slug); - const sessions = [ - summary, - ...index.sessions.filter((session) => session.id !== trimmed.id), - ].sort(byRecency); - await writeIndex(slug, { ...index, version: SESSION_VERSION, sessions }); + await updateIndex(slug, "rebuild", (index) => ({ + ...index, + version: SESSION_VERSION, + sessions: [ + summary, + ...index.sessions.filter((session) => session.id !== trimmed.id), + ].sort(byRecency), + })); return trimmed; } @@ -540,13 +577,10 @@ async function removeFromProject(slug: string, id: string): Promise { const path = getSessionPath(slug, id); if (existsSync(path)) rmSync(path, { force: true }); - const index = await readIndex(slug); - if (!index) return; - - await writeIndex(slug, { + await updateIndex(slug, "skip", (index) => ({ ...index, sessions: index.sessions.filter((session) => session.id !== id), - }); + })); } catch { // See above: leaving it listed is the safer failure. } @@ -666,13 +700,12 @@ export async function pruneSessions( } if (removedHere > 0) { - const index = (await readIndex(slug)) ?? { version: SESSION_VERSION, sessions: [] }; - await writeIndex(slug, { + await updateIndex(slug, "empty", (index) => ({ ...index, version: SESSION_VERSION, lastPrunedAt: now, sessions: kept, - }); + })); } } @@ -709,8 +742,7 @@ export async function pruneIfDue( // Stamped even when nothing was removed, or a store with no expired sessions // would rescan every launch. - const current = (await readIndex(slug)) ?? { version: SESSION_VERSION, sessions: [] }; - await writeIndex(slug, { ...current, lastPrunedAt: now }); + await updateIndex(slug, "empty", (index) => ({ ...index, lastPrunedAt: now })); return removed; } diff --git a/packages/tests/config/sessions.test.ts b/packages/tests/config/sessions.test.ts index 0c01d75..658e018 100644 --- a/packages/tests/config/sessions.test.ts +++ b/packages/tests/config/sessions.test.ts @@ -420,6 +420,58 @@ describe("an index that has fallen behind the files", () => { }); }); +/** + * A missing index means something different to each caller that writes one, and + * the three answers are not interchangeable. Asserted against the file on disk + * rather than through listSessions, because listSessions heals a wrong index by + * comparing its row count to the files and rebuilding — which would hide every + * one of these. + */ +describe("writing an index that is not there", () => { + test("saving a session rebuilds the other rows instead of replacing them", async () => { + const first = await seed(); + rmSync(join(projectDir(), "index.json"), { force: true }); + + const second = await seed(); + + const index = JSON.parse(await Bun.file(join(projectDir(), "index.json")).text()); + const ids = index.sessions.map((entry: any) => entry.id); + expect(ids).toContain(second.id); + expect(ids).toContain(first.id); + }); + + test("moving a session out of a project does not create an index there", async () => { + mkdirSync(configDir, { recursive: true }); + writeFileSync( + join(configDir, "conversation.json"), + JSON.stringify([{ role: "user", content: "legacy work" }]), + ); + resetSessionStoreForTests(); + const imported = (await migrateLegacyConversation())!; + + const legacyIndex = join(sessionsDir, LEGACY_SLUG, "index.json"); + rmSync(legacyIndex, { force: true }); + + await adoptSession(imported); + + // Nothing is left in that bucket to list, so an index recording zero rows + // is state the feature promises not to keep. + expect(existsSync(legacyIndex)).toBe(false); + }); + + test("stamping a prune keeps the sessions it did not remove", async () => { + const saved = await seed(); + const indexPath = join(projectDir(), "index.json"); + rmSync(indexPath, { force: true }); + + await pruneIfDue(30); + + const index = JSON.parse(await Bun.file(indexPath).text()); + expect(index.lastPrunedAt).toBeGreaterThan(0); + expect(index.sessions.map((entry: any) => entry.id)).toEqual([saved.id]); + }); +}); + describe("retention", () => { test("removes sessions past the cutoff and keeps the rest", async () => { const day = 24 * 60 * 60 * 1000;