From 7f2d96820665754136e970dc9cf6692ff7fb18b9 Mon Sep 17 00:00:00 2001 From: Alan Buscaglia Date: Sat, 26 Sep 2026 22:00:54 +0200 Subject: [PATCH] fix(history): harden GC and delete follow-ups, query-aware Home/End --- docs/prompt-history.md | 43 +++++- extensions/history/index.ts | 55 ++++++-- extensions/history/selector-helpers.ts | 24 ++++ extensions/history/store.ts | 68 +++++++-- odd/tasks/history-followups.md | 13 +- tests/history-command-registration.test.ts | 2 +- tests/history-delete-confirm.test.ts | 35 ++++- tests/history-gc.test.ts | 154 +++++++++++++++++++++ tests/history-off-path.test.ts | 29 ++++ tests/history-scope-delete.test.ts | 50 +++++++ tests/history-search-caret-keys.test.ts | 142 +++++++++++++++++++ tests/history-session-writer.test.ts | 22 +-- 12 files changed, 593 insertions(+), 44 deletions(-) create mode 100644 tests/history-search-caret-keys.test.ts diff --git a/docs/prompt-history.md b/docs/prompt-history.md index 432c4e374..80139a0aa 100644 --- a/docs/prompt-history.md +++ b/docs/prompt-history.md @@ -44,7 +44,9 @@ Env values are trimmed and case-insensitive. While the variable forces a value, the Customize rows show `env override` and the preview says the variable overrides the preference; a selection is still saved and takes effect once the variable stops forcing a value. A malformed preference file is -reported and never rewritten by Customize: fix or remove it by hand. +reported and never rewritten by Customize: fix or remove it by hand. The +history selector names that file and says the preference is invalid or +unreadable, instead of asking you to turn capture on in Customize. - The check runs per prompt: changing the preference or the variable stops or starts new captures immediately. @@ -60,7 +62,10 @@ after the extension loads, or at its first delivered prompt if that comes first. The selector reads the store but does not initiate import. With capture off, both capture and the selector leave the store untouched. Failed migration reads can be retried on a later session; untrusted deletion -records defer transcript bootstrap until they can be read safely. +records defer transcript bootstrap until they can be read safely. Migration +holds the `history-global.jsonl.migration-lock` directory while it runs; if a +pi process dies in that window, the lock stays and migration is skipped until +you remove that directory by hand. An import creates **new searchable copies** under `~/.pi/agent/history`. The source transcripts stay untouched and read-only. Turning capture off again @@ -113,6 +118,18 @@ rm -rf ~/.pi/agent/history # whole store rm -rf ~/.pi/agent/history/projects/ # one project (see registry.json) ``` +## Selector keys + +`Home` and `End` depend on the search box: + +- **Search box empty:** they move the list selection. `Home` selects the + newest prompt; `End` loads every remaining prompt and selects the oldest. +- **Any text in the search box** (whitespace included): they move the search + caret to the start or end of the query, like the other editing keys. They + never move the list, and `End` does not load the remaining prompts. + +As with other editing keys, the list selection returns to the first match. + ## Delete The selector's delete key (`ctrl+shift+backspace`) is a two-step y/n @@ -135,7 +152,8 @@ prompt is affected — prompts that merely share a beginning stay. `history-global.jsonl`. Each affected file is rewritten atomically (temp file + rename). Files are never removed, even when they end up empty. Lines that another pi instance appends while a file is being rewritten - are carried over into the new file. + are carried over into the new file; if that append fails, they are kept + in a sibling `.carry--.jsonl` store file instead. 2. **A tombstone is written** to `hidden.json`, so the prompt stays hidden everywhere the selector reads, and a later transcript bootstrap does not import it again. The session transcripts themselves are never modified. @@ -175,6 +193,13 @@ Failures surface an error notification and never report a clean delete: - If the tombstone write fails after store copies were removed, the prompt may reappear from session transcripts ("Deleted from the store, but hiding failed — the prompt may reappear from session transcripts."). +- If both happen — some files cannot be rewritten and the tombstone write + fails — a single notice says so and never claims the prompt is hidden + ("Some history files could not be rewritten and hiding failed — the + prompt may reappear from those files or from session transcripts."). + +The notice is chosen after the tombstone write, so it always describes the +final state. `hidden.json` fails closed: if it exists but cannot be trusted (unreadable, corrupt, or not an array), history is blocked with a recovery warning @@ -200,6 +225,10 @@ in total. Then: `compact--.jsonl`, which is written completely (temp file + rename) before any merged file is removed. Earlier compact files are merged again like any other file. +- Compaction runs only when at least **two** files can be merged. Merging a + single file cannot reduce the file count, so a directory that stays above + a threshold after compaction (for example, because compaction never + lowers the entry count) is not rewritten again at every shutdown. - Never merged: `seed.jsonl` (while it exists, the transcript import does not run again) and the capture file of the session that is shutting down. `history-global.jsonl` sits outside the project directories and is never @@ -211,9 +240,11 @@ in total. Then: Other pi instances may still be appending to the files being merged. Each file is renamed to a claim name before it is read (`.gc--.jsonl`), -so a later append by path starts a fresh file under the original name, and -bytes written to the claimed file after it was read are moved into the -compact file once the claim is removed. +so a later append by path starts a fresh file under the original name. +Complete lines written to the claimed file after it was read are appended to +the compact file **before** the claim is removed; if that append fails, the +claim stays on disk with every byte. The claim is read once more after its +removal for a write that landed in between. Failures never lose prompts: a file that cannot be read is left untouched, and if the compact file cannot be written, the claimed files stay on disk and diff --git a/extensions/history/index.ts b/extensions/history/index.ts index 839de9918..fa3a31af5 100644 --- a/extensions/history/index.ts +++ b/extensions/history/index.ts @@ -42,6 +42,7 @@ import { gentlePiConfigHome } from "../../lib/agent-home.ts"; import { historyCaptureEnabled, historyCaptureEnvOverride, + resolveHistoryCapturePolicy, } from "../../lib/history-capture-policy.ts"; import { hidePrompt } from "./hide-prompts.ts"; import { @@ -68,7 +69,6 @@ import { deleteConfirmFooterText, deleteConfirmStep, deletionActionsFor, - EDITOR_HIDE_FAILED_TEXT, filterPrompts, getVisiblePromptRecords, initialLoadedCount, @@ -81,6 +81,7 @@ import { shouldGrowWindow, STORE_DELETE_FAILED_TEXT, storeDeleteFollowUp, + storeDeleteNotice, withExpandedHistoryGlobals, type PiHistoryGlobals, type PromptEntry, @@ -151,11 +152,23 @@ export function captureEnabled( return historyCaptureEnabled({ env, gentlePiConfigHome: configHome }); } -/** Why capture is off, naming the control that actually decides it. */ -function captureDisabledMessage(env: NodeJS.ProcessEnv): string { - return historyCaptureEnvOverride(env) === "off" - ? "Prompt history is disabled by GENTLE_PI_HISTORY_CAPTURE, which overrides the Gentle → Customize → History preference." - : "Prompt history is disabled. Turn on \"Prompt history capture\" in Gentle → Customize → History, or set GENTLE_PI_HISTORY_CAPTURE=1."; +/** + * Why capture is off, naming the control that actually decides it. A + * malformed or unreadable preference is reported as such: Customize refuses + * to rewrite it, so "turn it on in Customize" would not help. + */ +function captureDisabledMessage( + env: NodeJS.ProcessEnv, + configHome: string, +): string { + if (historyCaptureEnvOverride(env) === "off") { + return "Prompt history is disabled by GENTLE_PI_HISTORY_CAPTURE, which overrides the Gentle → Customize → History preference."; + } + const preference = resolveHistoryCapturePolicy({ gentlePiConfigHome: configHome }); + if (preference.malformed) { + return `Prompt history is disabled because the Gentle → Customize → History preference is invalid or unreadable: ${preference.globalFile}. Fix or remove that file, or set GENTLE_PI_HISTORY_CAPTURE=1.`; + } + return "Prompt history is disabled. Turn on \"Prompt history capture\" in Gentle → Customize → History, or set GENTLE_PI_HISTORY_CAPTURE=1."; } // --------------------------------------------------------------------------- @@ -362,12 +375,14 @@ class PromptHistorySelector extends Container implements Focusable { match: (_d, kb) => kb.matches(_d, "tui.select.cancel"), handler: () => this.onCancel(), }, + // Home/End jump the list only while the search box is empty; with any + // text they fall through to the search input and move its caret. { - match: (d, _kb) => matchesKey(d, "home"), + match: (d, _kb) => matchesKey(d, "home") && this.listOwnsHomeEnd(), handler: () => this.jumpToFirst(), }, { - match: (d, _kb) => matchesKey(d, "end"), + match: (d, _kb) => matchesKey(d, "end") && this.listOwnsHomeEnd(), handler: () => this.jumpToLast(), }, { @@ -688,12 +703,12 @@ class PromptHistorySelector extends Container implements Focusable { // actions via the pure planner over the injected store root/cwd. const actions = deletionActionsFor(selected.source ?? "editor"); + let sweep: SweepResult | null = null; if (actions.deleteFromEditorStore) { // Store path: physically remove EVERY copy from the JSONL store, one // atomic rewrite per file. A thrown store failure is contained here // (PR #1393): toast + abort — nothing was removed and no tombstone is // written, so the delete never lies about state. - let sweep: SweepResult; try { sweep = this.scope === "global" @@ -704,9 +719,9 @@ class PromptHistorySelector extends Container implements Focusable { return; } // Files that could not be read or rewritten may still hold a copy: - // say so, and still write the tombstone that hides them. + // still write the tombstone that hides them; the notice waits for + // the hide result below. const followUp = storeDeleteFollowUp(sweep); - if (followUp.notice) this.onNotify?.(followUp.notice, "error"); if (!followUp.proceed) return; } @@ -721,7 +736,12 @@ class PromptHistorySelector extends Container implements Focusable { this.onNotify?.(hide.message, "error"); return; } - this.onNotify?.(EDITOR_HIDE_FAILED_TEXT, "error"); + } + // One notice for the editor path, stating both halves: a partial sweep + // only says "hidden" when the tombstone was actually written. + if (sweep) { + const notice = storeDeleteNotice(sweep, hide.status === "error"); + if (notice) this.onNotify?.(notice, "error"); } // Remove from the master records array so a subsequent filter doesn't // bring it back. @@ -870,6 +890,15 @@ class PromptHistorySelector extends Container implements Focusable { this.rebuildPreview(); } + /** + * An empty search box has no caret to move, so Home/End keep their list + * jumps; any text (whitespace included) routes them to the caret, and End + * then never loads the whole list. + */ + private listOwnsHomeEnd(): boolean { + return this.searchInput.getValue().length === 0; + } + private jumpToFirst(): void { if (this.filteredRecords.length === 0) return; this.selectedIndex = 0; @@ -1130,7 +1159,7 @@ function createOpenFlow( // Capture gate (#1390) FIRST: with capture off the selector is a no-op — // no registry writes, no writer init, no store reads, no overlay. if (!captureEnabled(env, configHome)) { - ctx.ui.notify(captureDisabledMessage(env), "warning"); + ctx.ui.notify(captureDisabledMessage(env, configHome), "warning"); return; } diff --git a/extensions/history/selector-helpers.ts b/extensions/history/selector-helpers.ts index 7cb29ce78..42a0a3998 100644 --- a/extensions/history/selector-helpers.ts +++ b/extensions/history/selector-helpers.ts @@ -331,6 +331,13 @@ export const EDITOR_HIDE_FAILED_TEXT = export const STORE_DELETE_PARTIAL_TEXT = "Some history files could not be rewritten; the prompt is hidden, but copies may remain on disk."; +/** + * Toast copy when some store files could not be rewritten AND the tombstone + * write failed: nothing hides the copies that may remain on disk. + */ +export const STORE_DELETE_PARTIAL_HIDE_FAILED_TEXT = + "Some history files could not be rewritten and hiding failed — the prompt may reappear from those files or from session transcripts."; + /** The counts a scope delete reports (structural twin of store's SweepResult). */ export interface StoreSweepCounts { filesAffected: number; @@ -353,6 +360,23 @@ export function storeDeleteFollowUp( return { proceed: counts.removed > 0 }; } +/** + * The one error notice of an editor-path delete, chosen AFTER the tombstone + * write so it never claims the prompt is hidden when hiding failed. No + * notice for a clean sweep with a written tombstone. + */ +export function storeDeleteNotice( + counts: StoreSweepCounts, + hideFailed: boolean, +): string | undefined { + if (counts.failed > 0) { + return hideFailed + ? STORE_DELETE_PARTIAL_HIDE_FAILED_TEXT + : STORE_DELETE_PARTIAL_TEXT; + } + return hideFailed ? EDITOR_HIDE_FAILED_TEXT : undefined; +} + export function getVisiblePromptRecords( records: PromptRecord[], selectedIndex: number, diff --git a/extensions/history/store.ts b/extensions/history/store.ts index 24ce85c94..08ac7a98b 100644 --- a/extensions/history/store.ts +++ b/extensions/history/store.ts @@ -474,8 +474,11 @@ function readFrom(fd: number, position: number): Buffer { * the descriptor kept open on the replaced inode supplies everything * appended after the read — including a torn last line — which is carried * over verbatim into the new file after the rename. Kept lines are copied - * byte-for-byte. On failure the tmp file is removed, the original stays in - * place unless the rename already happened, and the error is rethrown. + * byte-for-byte. If that carry-over append fails, the raced bytes are + * written to a sibling `.carry--.jsonl` store file instead + * of vanishing with the replaced inode. On failure the tmp file is removed, + * the original stays in place unless the rename already happened, and the + * error is rethrown. */ function sweepFile(file: string, key: string): number { const fd = fs.openSync(file, "r"); @@ -502,7 +505,7 @@ function sweepFile(file: string, key: string): number { tmp = null; // Lines another instance appended to the replaced inode since the read. const carried = readFrom(fd, complete); - if (carried.length > 0) fs.appendFileSync(file, carried); + if (carried.length > 0) carryIntoRewrite(file, carried); return removed; } catch (error) { if (tmp !== null) { @@ -518,6 +521,25 @@ function sweepFile(file: string, key: string): number { } } +/** + * Append raced bytes to the rewritten file; if that fails, keep them in a + * sibling store file (read like any other) so they are never lost. A torn + * last line is completed so the sibling stays parseable. Throws only when + * both writes fail. + */ +function carryIntoRewrite(file: string, carried: Buffer): void { + try { + fs.appendFileSync(file, carried); + } catch { + const text = carried.toString("utf8"); + fs.writeFileSync( + `${file}.carry-${process.pid}-${Date.now()}.jsonl`, + text.endsWith("\n") ? text : `${text}\n`, + { flag: "wx" }, + ); + } +} + /** * Remove every line whose prompt identity matches `text` from each file in * `files`, one atomic rewrite per affected file (see sweepFile). Files whose @@ -834,7 +856,9 @@ export interface GcOptions { * Compaction consolidates files: it merges the oldest store files of ONE * project into a single `compact--.jsonl` and removes the merged * originals. It is not a retention limit — every visible prompt is copied; - * only tombstoned prompts (already deleted by the user) are dropped. + * only tombstoned prompts (already deleted by the user) are dropped. It + * runs only when at least two files can be merged, so a repeated GC over + * an already compacted dir rewrites nothing. * * Never merged: `seed.jsonl` (its presence is the bootstrap gate, so * removing it would re-seed deleted prompts from transcripts), the files @@ -881,7 +905,10 @@ export function gcProjectDir( .slice(opts.keepNewest ?? GC_KEEP_NEWEST) .reverse() // oldest first: the merged output is chronological .map((c) => c.file); - if (tail.length === 0) return none; + // Merging a single file cannot reduce the file count: rewriting it + // would repeat on every shutdown while a threshold stays exceeded + // (compaction never lowers the entry count), so GC stays idempotent. + if (tail.length < 2) return none; return compactFiles(dir, tail, hidden.keys); } catch { return none; @@ -941,9 +968,11 @@ function claimFile(file: string): ClaimedFile | null { * atomically (tmp + rename) BEFORE any claim is removed. A failure before * the compact file lands leaves every claim in place (still a readable * store file); a claim that cannot be removed survives as a harmless - * duplicate (drains dedupe by identity). After each removal the claim's - * descriptor is drained once more and any late bytes are appended to the - * compact file (or written back under the claim name if that fails). + * duplicate (drains dedupe by identity). Complete lines that reached a + * claim after its read are appended to the compact file BEFORE the claim + * is removed — if that fails, the claim stays with every byte. After the + * removal the descriptor is drained once more for a write that landed in + * between (written back under the claim name if its append fails). */ function compactFiles( dir: string, @@ -979,6 +1008,7 @@ function compactFiles( } for (const c of claimed) { try { + carryCompleteLines(c, compact, hidden); fs.rmSync(c.claim); } catch { // the claim keeps every byte; it is merged again by a later GC @@ -990,7 +1020,27 @@ function compactFiles( return { compacted: true, merged: claimed.length }; } -/** Move bytes that reached a removed claim after it was read. */ +/** + * Append the complete lines that reached a still-present claim after it + * was read, advancing its cursor; a torn last line waits for the final + * drain. Throws when the append fails, so the caller keeps the claim. + */ +function carryCompleteLines( + c: ClaimedFile, + compact: string, + hidden: ReadonlySet, +): void { + const late = readFrom(c.fd, c.consumed); + const complete = late.lastIndexOf(NEWLINE) + 1; + if (complete === 0) return; + fs.appendFileSync( + compact, + keepVisibleLines(late.subarray(0, complete).toString("utf8"), hidden), + ); + c.consumed += complete; +} + +/** Move bytes that reached a claim between its last carry and its removal. */ function carryOver( c: ClaimedFile, compact: string, diff --git a/odd/tasks/history-followups.md b/odd/tasks/history-followups.md index aded676c9..b13905e88 100644 --- a/odd/tasks/history-followups.md +++ b/odd/tasks/history-followups.md @@ -20,7 +20,7 @@ Delegated direct: each task touches 2+ non-trivial files; one writer at a time. ## Tasks - [x] F1: Persisted History capture toggle in Customize with env precedence, docs and tests. Route: delegated (Customize + history + docs). -- [ ] F2: History reliability follow-ups: GC carry-over after unlink, perpetual line-threshold compaction, still-valid review advisories, `Home`/`End` search caret. Route: delegated. +- [x] F2: History reliability follow-ups: GC carry-over after unlink, perpetual line-threshold compaction, still-valid review advisories, `Home`/`End` search caret. Route: delegated. - [ ] F3: Restore responsive header and sidebar-aware overlay margin from `e2cca1f9b^` onto current main with tests. Route: delegated. - [ ] F4: Remove temporary worktrees from the chain work. Route: inline. @@ -40,5 +40,14 @@ Delegated direct: each task touches 2+ non-trivial files; one writer at a time. - Docs: `docs/prompt-history.md` quick path + precedence table. `docs/readme-reference.md` has no Customize option list, so unchanged; the Customize paragraph lives in `README.md` (outside F1 edit surface) and does not yet mention the History category — proposed follow-up. - Size: ~310 insertions/52 deletions tracked plus ~200 lines in two new files (above the advisory ~400 heuristic because of tests; not split). +2026-09-26 F2 (delegated writer, branch `fix/history-reliability-followups` from main `d615b2ed2`): advisories evaluated against current code; fixes uncommitted; `Home`/`End` blocked on a product decision. +- Fixed (strict TDD): (a) GC carry-over: complete late lines are appended to the compact file BEFORE the claim is removed (a failed append keeps the whole claim), then one post-removal drain; (b) GC idempotence: compaction runs only when >= 2 files can be merged, so a dir that stays above a threshold is not rewritten every shutdown; (c) `sweepFile`: a failed carry-over append after the rename keeps the raced bytes in a sibling `.carry--.jsonl` store file instead of losing them; (d) `executeDelete`: the partial-sweep notice is chosen after `hidePrompt` via `storeDeleteNotice`, with a new combined `STORE_DELETE_PARTIAL_HIDE_FAILED_TEXT`, so it never claims "hidden" when the tombstone failed; (e) #1475: the disabled warning reports a malformed/unreadable preference as invalid and names the file; (f) `tests/history-session-writer.test.ts` read the ambient home's Customize preference in `captureEnabled is a strict opt-in` (failed under a temp `HOME` with the preference on); every call now uses an empty config home. +- Not fixed: ownerless migration crash lock stays fail-closed by design (pinned by `an ownerless crash lock fails closed...`); documented manual recovery instead. `finally rmdirSync` throwing is contained by the caller's catch. Sweep ENOENT on a file GC just claimed stays a quiet no-op (never a false "deleted" claim). +- RED observed: 4 gc tests (late line absent at claim removal; claim held only the late line; second GC `merged: 1`; single-candidate `merged: 1`), 1 sweep test (`failed: 1`, raced line lost), 2 off-path tests (generic "turn on" copy), delete-confirm import error (missing exports), session-writer under temp HOME (`actual: true`). GREEN: all pass. +- Checks: `node --experimental-strip-types --test tests/*.test.ts` 3865 tests, 3822 pass, 0 fail, 43 skipped; `node scripts/check-types.mjs` exit 0, 188 baseline, no regressions; `git diff --check` clean. +- Docs: `docs/prompt-history.md` (invalid preference warning, migration lock recovery, sweep carry sibling, combined delete notice, two-file compaction minimum, carry-before-removal order). +- `Home`/`End` conflict (search input always focused vs. list jumps pinned by §B2/§D7) resolved by user choice 2, "by query": with any text in the search box (whitespace included) `Home`/`End` fall through to the search input and move its caret, and `End` never jumps or loads the list; with an empty search box the §B2/§D7 list jumps stay. Implemented as a `listOwnsHomeEnd()` guard on the two existing dispatch entries (12 entries, order, and handlers unchanged). New behavior suite `tests/history-search-caret-keys.test.ts` drives the real selector through the `history` command with a fake overlay host. RED: 3 failed (`Home` then "p" pasted nothing; caret-to-end then "3" pasted nothing; `End` with a query or whitespace-only query selected `p00`); the empty-query case passed before and after (pins existing jumps). GREEN: 4/4, plus `history-dispatch` and `history-lazy-windowing` unchanged and passing. Docs: new "Selector keys" section in `docs/prompt-history.md`. +- Checks after `Home`/`End`: `node --experimental-strip-types --test tests/*.test.ts` 3869 tests, 3826 pass, 0 fail, 43 skipped; `node scripts/check-types.mjs` exit 0, 188 baseline, no regressions; `git diff --check` clean (the new untracked test file also has no trailing whitespace). Uncommitted; no review invoked by the writer. + ## Next step -Parent: review F1, decide on the README.md Customize mention, commit F1 as a work-unit commit. +Parent: review F2 and close it with a work-unit commit. diff --git a/tests/history-command-registration.test.ts b/tests/history-command-registration.test.ts index 867ab9b74..2d42ab456 100644 --- a/tests/history-command-registration.test.ts +++ b/tests/history-command-registration.test.ts @@ -93,7 +93,7 @@ test("the capture gate precedes every store touch in the open flow (#1390)", () "the drain must run only after the capture gate passes", ); assert.ok( - body.includes("captureDisabledMessage(env)"), + body.includes("captureDisabledMessage(env, configHome)"), "the disabled warning explains which control decides", ); const message = source.slice( diff --git a/tests/history-delete-confirm.test.ts b/tests/history-delete-confirm.test.ts index 758b52987..f50759d68 100644 --- a/tests/history-delete-confirm.test.ts +++ b/tests/history-delete-confirm.test.ts @@ -8,8 +8,10 @@ import { deletionActionsFor, EDITOR_HIDE_FAILED_TEXT, STORE_DELETE_FAILED_TEXT, + STORE_DELETE_PARTIAL_HIDE_FAILED_TEXT, STORE_DELETE_PARTIAL_TEXT, storeDeleteFollowUp, + storeDeleteNotice, } from "../extensions/history/selector-helpers.ts"; // Slice-05 delete-confirm tests (PR #1393 follow-up): the delete @@ -365,17 +367,42 @@ test("storeDeleteFollowUp: any failed file proceeds to hide AND surfaces an erro assert.ok(STORE_DELETE_PARTIAL_TEXT.includes("hidden")); }); -test("executeDelete routes the sweep result through storeDeleteFollowUp before hiding", () => { +test("executeDelete reports a partial sweep only after the hide result is known", () => { const body = executeDeleteBody(); const followAt = body.indexOf("storeDeleteFollowUp("); - const noticeAt = body.indexOf('this.onNotify?.(followUp.notice, "error")'); const hideAt = body.indexOf("hidePrompt("); + const noticeAt = body.indexOf("storeDeleteNotice("); + const spliceAt = body.indexOf("this.records.splice("); assert.ok(followAt >= 0, "the sweep result must be interpreted"); - assert.ok(noticeAt > followAt, "a partial failure must surface as an error"); - assert.ok(hideAt > noticeAt, "the tombstone still follows a partial failure"); + assert.ok(hideAt > followAt, "the tombstone still follows a partial failure"); + assert.equal( + body.includes("followUp.notice"), + false, + "the partial notice must not claim the prompt is hidden before hidePrompt runs", + ); + assert.ok(noticeAt > hideAt, "the store notice is chosen from the hide result"); + assert.ok( + body.includes('this.onNotify?.(notice, "error")'), + "the chosen notice surfaces as an error", + ); + assert.ok(noticeAt < spliceAt, "the notice is decided before the row leaves the list"); assert.equal( body.includes("if (removed === 0) return;"), false, "a zero-removal partial failure must not skip the tombstone", ); }); + +test("storeDeleteNotice states exactly what remains after the sweep and the hide", () => { + const clean = { filesAffected: 1, removed: 1, failed: 0 }; + const partial = { filesAffected: 1, removed: 1, failed: 1 }; + assert.equal(storeDeleteNotice(clean, false), undefined); + assert.equal(storeDeleteNotice(clean, true), EDITOR_HIDE_FAILED_TEXT); + assert.equal(storeDeleteNotice(partial, false), STORE_DELETE_PARTIAL_TEXT); + // Both halves failed: one notice, and it never claims the prompt is hidden. + assert.equal(storeDeleteNotice(partial, true), STORE_DELETE_PARTIAL_HIDE_FAILED_TEXT); + assert.equal(STORE_DELETE_PARTIAL_HIDE_FAILED_TEXT.includes("is hidden"), false); + assert.ok(STORE_DELETE_PARTIAL_HIDE_FAILED_TEXT.includes("could not be rewritten")); + assert.ok(STORE_DELETE_PARTIAL_HIDE_FAILED_TEXT.includes("hiding failed")); + assert.ok(STORE_DELETE_PARTIAL_HIDE_FAILED_TEXT.includes("may reappear")); +}); diff --git a/tests/history-gc.test.ts b/tests/history-gc.test.ts index 7a03863dc..3c9923deb 100644 --- a/tests/history-gc.test.ts +++ b/tests/history-gc.test.ts @@ -676,3 +676,157 @@ test("a torn last line in a merged file is carried over, not dropped", () => { assert.ok(texts.includes("torn")); assert.ok(texts.includes("r2.jsonl-1")); }); + +// Carry-over ordering (#1394 advisory "carryover-after-unlink"): bytes that +// reach a claim after its merge read are copied into the compact file +// BEFORE the claim is removed, so a failed copy leaves the claim — and every +// byte in it — on disk instead of depending on a post-removal fallback. + +/** + * Replace fs.renameSync for the duration of fn; `after` runs right after a + * rename whose destination is a compact-*.jsonl file lands. + */ +function withAfterCompactRename(after: () => void, fn: () => void): void { + type RenameSync = (from: string, to: string) => void; + const target = fs as unknown as { renameSync: RenameSync }; + const realRename = fs.renameSync.bind(fs) as RenameSync; + target.renameSync = (from: string, to: string) => { + realRename(from, to); + if (path.basename(to).startsWith("compact-")) after(); + }; + try { + fn(); + } finally { + target.renameSync = realRename; + } +} + +test("late bytes reach the compact file before their claim is removed", () => { + const root = makeRoot(); + const dir = projectRoot(root); + fs.mkdirSync(dir, { recursive: true }); + const idle = writeFile(dir, "idle.jsonl", 2, 1000); + for (let i = 2; i <= 4; i++) { + writeFile(dir, `k${i}.jsonl`, 2, i * 1000); + } + // A writer that opened the file before the claim writes after the merge. + const fd = fs.openSync(idle, "a"); + let lateInCompactAtRemoval: boolean | null = null; + try { + withAfterCompactRename( + () => { + fs.writeSync(fd, `${JSON.stringify({ v: 1, text: "late-by-fd" })}\n`); + }, + () => { + withRmSyncPatched( + (file, rmSync) => { + if (path.basename(file).startsWith("idle.jsonl.gc-")) { + lateInCompactAtRemoval = compactTexts(dir).includes("late-by-fd"); + } + rmSync(file); + }, + () => { + const result = gcProjectDir(root, CWD, { + fileThreshold: 2, + lineThreshold: 100000, + keepNewest: 1, + }); + assert.deepEqual(result, { compacted: true, merged: 3 }); + }, + ); + }, + ); + } finally { + fs.closeSync(fd); + } + assert.equal(lateInCompactAtRemoval, true); + assert.equal(dirTexts(dir).filter((t) => t === "late-by-fd").length, 1); +}); + +test("a failed carry-over keeps the whole claim on disk", () => { + const root = makeRoot(); + const dir = projectRoot(root); + fs.mkdirSync(dir, { recursive: true }); + const idle = writeFile(dir, "idle.jsonl", 2, 1000); + for (let i = 2; i <= 4; i++) { + writeFile(dir, `m${i}.jsonl`, 2, i * 1000); + } + const fd = fs.openSync(idle, "a"); + type AppendFileSync = (file: string, data: string | Buffer) => void; + const target = fs as unknown as { appendFileSync: AppendFileSync }; + const realAppend = fs.appendFileSync.bind(fs) as AppendFileSync; + target.appendFileSync = (file: string, data: string | Buffer) => { + if (path.basename(String(file)).startsWith("compact-")) { + throw Object.assign(new Error("simulated ENOSPC"), { code: "ENOSPC" }); + } + realAppend(file, data); + }; + try { + withAfterCompactRename( + () => { + fs.writeSync(fd, `${JSON.stringify({ v: 1, text: "late-by-fd" })}\n`); + }, + () => { + gcProjectDir(root, CWD, { + fileThreshold: 2, + lineThreshold: 100000, + keepNewest: 1, + }); + }, + ); + } finally { + target.appendFileSync = realAppend; + fs.closeSync(fd); + } + // The claim survives intact: its merged lines AND the late line. + const claim = fs + .readdirSync(dir) + .find((f) => f.startsWith("idle.jsonl.gc-")); + assert.ok(claim, "the claim must stay on disk when its carry-over fails"); + const claimTexts = fs + .readFileSync(path.join(dir, claim), "utf8") + .trim() + .split("\n") + .map((l) => (JSON.parse(l) as { text: string }).text); + assert.deepEqual(claimTexts, ["idle.jsonl-0", "idle.jsonl-1", "late-by-fd"]); +}); + +// Idempotence (#1394 advisory "line-threshold-perpetual"): merging a single +// file cannot reduce the file count, so GC must not rewrite it — otherwise +// every shutdown above a threshold rewrites the compact file again. + +test("a second GC over an already compacted dir rewrites nothing", () => { + const root = makeRoot(); + const dir = projectRoot(root); + fs.mkdirSync(dir, { recursive: true }); + for (let i = 1; i <= 12; i++) { + writeFile(dir, `n${String(i).padStart(2, "0")}.jsonl`, 5, i * 1000); + } + const opts = { fileThreshold: 10, lineThreshold: 100000, keepNewest: 10 }; + assert.deepEqual(gcProjectDir(root, CWD, opts), { compacted: true, merged: 2 }); + // 10 newest + 1 compact = 11 files: still above the file threshold. + const before = fs.readdirSync(dir).sort(); + assert.equal(before.length, 11); + const compactBefore = compactTexts(dir); + assert.deepEqual(gcProjectDir(root, CWD, opts), { compacted: false, merged: 0 }); + assert.deepEqual(fs.readdirSync(dir).sort(), before); + assert.deepEqual(compactTexts(dir), compactBefore); +}); + +test("the line threshold with a single merge candidate rewrites nothing", () => { + const root = makeRoot(); + const dir = projectRoot(root); + fs.mkdirSync(dir, { recursive: true }); + // 3 files x 4000 lines = 12000 > 10000, but keepNewest 2 leaves one file. + for (let i = 1; i <= 3; i++) { + writeFile(dir, `l${i}.jsonl`, 4000, i * 1000); + } + const before = fs.readdirSync(dir).sort(); + const result = gcProjectDir(root, CWD, { + fileThreshold: 10, + lineThreshold: 10000, + keepNewest: 2, + }); + assert.deepEqual(result, { compacted: false, merged: 0 }); + assert.deepEqual(fs.readdirSync(dir).sort(), before); +}); diff --git a/tests/history-off-path.test.ts b/tests/history-off-path.test.ts index 2ff92a14e..72a6f75e8 100644 --- a/tests/history-off-path.test.ts +++ b/tests/history-off-path.test.ts @@ -139,3 +139,32 @@ test("an explicit env off names the override instead of the Customize fix", asyn assert.match(notifyCalls[0][0], /disabled by GENTLE_PI_HISTORY_CAPTURE, which overrides the Gentle → Customize → History preference/); assert.deepEqual(fs.readdirSync(root), []); }); + +test("a malformed Customize preference is reported as invalid, not just off", async () => { + const root = makeRoot(); + const configHome = fs.mkdtempSync(path.join(os.tmpdir(), "pi-history-off-config-")); + const preference = path.join(configHome, "history-capture.json"); + fs.writeFileSync(preference, "{not json", "utf8"); + const { commandHandler } = loadWithCommand({}, root, configHome); + const notifyCalls: Array<[string, string]> = []; + await commandHandler([], fakeCtx(notifyCalls)); + assert.equal(notifyCalls.length, 1); + assert.equal(notifyCalls[0][1], "warning"); + assert.match(notifyCalls[0][0], /Gentle → Customize → History preference is invalid or unreadable/); + assert.ok(notifyCalls[0][0].includes(preference), `the warning must name the file, got: ${notifyCalls[0][0]}`); + assert.ok(notifyCalls[0][0].includes("GENTLE_PI_HISTORY_CAPTURE=1")); + // Reporting never repairs: the malformed file and the store stay untouched. + assert.equal(fs.readFileSync(preference, "utf8"), "{not json"); + assert.deepEqual(fs.readdirSync(root), []); +}); + +test("an env value that defers to a malformed preference still reports it", async () => { + const root = makeRoot(); + const configHome = fs.mkdtempSync(path.join(os.tmpdir(), "pi-history-off-config-")); + fs.writeFileSync(path.join(configHome, "history-capture.json"), "[]", "utf8"); + const { commandHandler } = loadWithCommand({ GENTLE_PI_HISTORY_CAPTURE: "maybe" }, root, configHome); + const notifyCalls: Array<[string, string]> = []; + await commandHandler([], fakeCtx(notifyCalls)); + assert.equal(notifyCalls.length, 1); + assert.match(notifyCalls[0][0], /preference is invalid or unreadable/); +}); diff --git a/tests/history-scope-delete.test.ts b/tests/history-scope-delete.test.ts index a2e7dd7df..b638da35e 100644 --- a/tests/history-scope-delete.test.ts +++ b/tests/history-scope-delete.test.ts @@ -7,6 +7,7 @@ import { appendSessionCapture, deleteFromGlobal, deleteFromProject, + drainProject, globalSeedPath, openSessionWriter, projectHash, @@ -245,3 +246,52 @@ test("a failed rewrite is counted, keeps the original, and leaves no tmp file", const leftovers = fs.readdirSync(dir).filter((f) => f.includes(".tmp-")); assert.deepEqual(leftovers, []); }); + +// A failed carry-over append (#1393 advisory): the rewrite already replaced +// the file, so the raced lines exist only in the kept descriptor. They must +// land in a sibling store file instead of vanishing with the old inode. +test("raced lines survive a failed carry-over append in a sibling store file", () => { + const root = makeRoot(); + const state = openSessionWriter(root, PROJECT_A, "other-instance"); + appendSessionCapture(state, "victim"); + appendSessionCapture(state, "keeper"); + const realAppend = fs.appendFileSync; + let carryFailed = false; + fs.appendFileSync = (( + file: fs.PathOrFileDescriptor, + data: string | Uint8Array, + options?: fs.WriteFileOptions, + ) => { + // The sweep carries a Buffer; the simulated instance appends a string. + if (String(file) === state.filePath && Buffer.isBuffer(data)) { + carryFailed = true; + throw Object.assign(new Error("simulated EIO"), { code: "EIO" }); + } + return realAppend(file, data, options); + }) as typeof fs.appendFileSync; + let result; + try { + withAppendBeforeRename( + state.filePath, + `${JSON.stringify({ v: 1, text: "raced in" })}\n`, + () => { + result = deleteFromProject(root, PROJECT_A, "victim"); + }, + ); + } finally { + fs.appendFileSync = realAppend; + } + assert.equal(carryFailed, true); + assert.deepEqual(result, { filesAffected: 1, removed: 1, failed: 0 }); + assert.deepEqual(fileTexts(state.filePath), ["keeper"]); + const drained = drainProject(root, PROJECT_A); + assert.equal(drained.status, "ok"); + if (drained.status === "ok") { + assert.deepEqual([...drained.prompts].sort(), ["keeper", "raced in"]); + } + const dir = path.dirname(state.filePath); + assert.deepEqual( + fs.readdirSync(dir).filter((f) => f.includes(".tmp-")), + [], + ); +}); diff --git a/tests/history-search-caret-keys.test.ts b/tests/history-search-caret-keys.test.ts new file mode 100644 index 000000000..7c0b727de --- /dev/null +++ b/tests/history-search-caret-keys.test.ts @@ -0,0 +1,142 @@ +import { test } from "node:test"; +import assert from "node:assert/strict"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import promptHistoryExtension from "../extensions/history/index.ts"; +import { projectHash } from "../extensions/history/store.ts"; + +// Home/End routing in the history selector: with text in the search box +// they move the search caret (End never jumps or loads the list); with an +// empty search box they keep the documented list jumps (§B2/§D7). The real +// selector is driven through the history command with a fake overlay host; +// every fixture lives under os.tmpdir(), never the user's real ~/.pi. + +const CWD = "/pi-history-fixtures/project-caret-keys"; +const HOME = "\x1b[H"; +const END = "\x1b[F"; +const LEFT = "\x1b[D"; +const ENTER = "\r"; +// 15 prompts, oldest p00 .. newest p14: more than the initial 10-row window. +const PROMPT_COUNT = 15; + +interface Selector { + handleInput(data: string): void; +} + +function makeRoot(): string { + const root = fs.mkdtempSync(path.join(os.tmpdir(), "pi-history-caret-")); + const dir = path.join(root, "projects", projectHash(CWD)); + fs.mkdirSync(dir, { recursive: true }); + const lines = Array.from({ length: PROMPT_COUNT }, (_, i) => + JSON.stringify({ + v: 1, + text: `p${String(i).padStart(2, "0")}`, + ts: 1_700_000_000_000 + i, + }), + ); + fs.writeFileSync(path.join(dir, "store.jsonl"), `${lines.join("\n")}\n`); + return root; +} + +/** + * Open the selector, feed `keys` one at a time, and return the prompt the + * selector pasted into the editor (null when it pasted nothing). + */ +async function selectAfter(keys: string[]): Promise { + const root = makeRoot(); + const commands: Array<[string, { handler: unknown }]> = []; + promptHistoryExtension( + { + on: () => {}, + registerShortcut: () => {}, + registerCommand: (name: string, def: { handler: unknown }) => { + commands.push([name, def]); + }, + } as never, + { + env: { GENTLE_PI_HISTORY_CAPTURE: "1" }, + gentlePiConfigHome: fs.mkdtempSync( + path.join(os.tmpdir(), "pi-history-caret-config-"), + ), + root, + cwd: CWD, + instanceId: "inst-caret", + agentDir: path.join(root, "agent"), + sessionsRoot: path.join(root, "sessions"), + }, + ); + const command = commands.find(([name]) => name === "history"); + assert.ok(command, "the history command must be registered"); + const handler = command[1].handler as ( + args: unknown, + ctx: unknown, + ) => Promise; + + let pasted: string | null = null; + const plain = (_color: string, text: string) => text; + const ctx = { + ui: { + notify: (message: string) => { + assert.fail(`unexpected notification: ${message}`); + }, + pasteToEditor: (text: string) => { + pasted = text; + }, + custom: ( + factory: ( + tui: unknown, + theme: unknown, + keybindings: unknown, + done: (result: unknown) => void, + ) => Selector, + ) => + new Promise((resolve) => { + const selector = factory( + { requestRender: () => {} }, + { fg: plain, bg: plain, bold: (text: string) => text }, + undefined, + resolve, + ); + for (const key of keys) selector.handleInput(key); + // Enter selects the highlighted row; with no match it resolves + // nothing, so close explicitly to finish the command. + selector.handleInput(ENTER); + resolve(null); + }), + }, + }; + await handler([], ctx); + // The paste path schedules one render tick; let it drain. + await new Promise((resolve) => setTimeout(resolve, 0)); + return pasted; +} + +test("with a query, Home moves the search caret instead of the list", async () => { + // Typed "1", caret to the start, then "p": the query is "p1", whose + // newest match is p14. A list jump would leave the query as "1p". + assert.equal(await selectAfter(["1", HOME, "p"]), "p14"); +}); + +test("with a query, End moves the search caret instead of the list", async () => { + // Caret moved to the start, End brings it back: "p1" + "3" = "p13". + // A list jump would leave the caret at 0 and type "3p1". + assert.equal(await selectAfter(["p", "1", LEFT, LEFT, END, "3"]), "p13"); +}); + +test("with a query, End neither jumps to the last match nor loads the list", async () => { + // A real query: the highlighted row stays the newest match, not p00. + assert.equal(await selectAfter(["p", END]), "p14"); + // A whitespace-only query filters nothing and keeps the 10-row window: + // End must not load every record and select the oldest one. + assert.equal(await selectAfter([" ", END]), "p14"); +}); + +test("with an empty query, Home and End keep the list jumps", async () => { + // End loads every record and selects the oldest one (§D7). + assert.equal(await selectAfter([END]), "p00"); + // Home jumps back to the newest. + assert.equal(await selectAfter([END, HOME]), "p14"); + // Emptying the query hands Home/End back to the list. + assert.equal(await selectAfter(["p", "\x7f", END]), "p00"); +}); diff --git a/tests/history-session-writer.test.ts b/tests/history-session-writer.test.ts index 735c62764..634cf136f 100644 --- a/tests/history-session-writer.test.ts +++ b/tests/history-session-writer.test.ts @@ -193,16 +193,20 @@ test("the extension entry registers exactly the slice-6 wiring surface", () => { }); test("captureEnabled is a strict opt-in", () => { - assert.equal(captureEnabled({}, makeConfigHome()), false); - assert.equal(captureEnabled({ GENTLE_PI_HISTORY_CAPTURE: "0" }), false); - assert.equal(captureEnabled({ GENTLE_PI_HISTORY_CAPTURE: "false" }), false); - assert.equal(captureEnabled({ GENTLE_PI_HISTORY_CAPTURE: "off" }), false); - assert.equal(captureEnabled({ GENTLE_PI_HISTORY_CAPTURE: "yes" }), false); - assert.equal(captureEnabled({ GENTLE_PI_HISTORY_CAPTURE: " 1 " }), true); - assert.equal(captureEnabled({ GENTLE_PI_HISTORY_CAPTURE: "TRUE" }), true); - assert.equal(captureEnabled({ GENTLE_PI_HISTORY_CAPTURE: "On" }), true); + // Every call gets an empty config home: an env value that defers to the + // preference must never read the developer's real Customize setting. + const configHome = makeConfigHome(); + const enabled = (env: NodeJS.ProcessEnv) => captureEnabled(env, configHome); + assert.equal(enabled({}), false); + assert.equal(enabled({ GENTLE_PI_HISTORY_CAPTURE: "0" }), false); + assert.equal(enabled({ GENTLE_PI_HISTORY_CAPTURE: "false" }), false); + assert.equal(enabled({ GENTLE_PI_HISTORY_CAPTURE: "off" }), false); + assert.equal(enabled({ GENTLE_PI_HISTORY_CAPTURE: "yes" }), false); + assert.equal(enabled({ GENTLE_PI_HISTORY_CAPTURE: " 1 " }), true); + assert.equal(enabled({ GENTLE_PI_HISTORY_CAPTURE: "TRUE" }), true); + assert.equal(enabled({ GENTLE_PI_HISTORY_CAPTURE: "On" }), true); // The unshipped rename from the contributor branch is not a switch. - assert.equal(captureEnabled({ [`GENTLE_PI_HISTORY_${"ENABLE"}`]: "1" }), false); + assert.equal(enabled({ [`GENTLE_PI_HISTORY_${"ENABLE"}`]: "1" }), false); }); test("the capture handler is a no-op unless the user opts in", () => {