From 13e1407827455812dba765457e048d66aa6defce Mon Sep 17 00:00:00 2001 From: Alan Buscaglia Date: Sat, 26 Sep 2026 22:41:17 +0200 Subject: [PATCH] fix(history): resolve review advisories on carry, notices and layout --- docs/prompt-history.md | 5 +- extensions/history/index.ts | 6 +- extensions/history/store.ts | 61 +++++++++++-- odd/tasks/history-review-advisories.md | 39 +++++++++ tests/history-delete-confirm.test.ts | 52 +++++++++++ tests/history-header-layout.test.ts | 13 +++ tests/history-overlay-margin.test.ts | 51 +++++++++++ tests/history-scope-delete.test.ts | 114 +++++++++++++++++++++++++ 8 files changed, 334 insertions(+), 7 deletions(-) create mode 100644 odd/tasks/history-review-advisories.md diff --git a/docs/prompt-history.md b/docs/prompt-history.md index 9d9053a03..b798794b6 100644 --- a/docs/prompt-history.md +++ b/docs/prompt-history.md @@ -174,7 +174,10 @@ prompt is affected — prompts that merely share a beginning stay. 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; if that append fails, they are kept - in a sibling `.carry--.jsonl` store file instead. + in a sibling `.carry--.jsonl` store file instead. Carry + files belong to the same scope as the file they came from (a + `history-global.jsonl.carry-*.jsonl` file is part of the global scope), + so the selector reads them and later deletes sweep them. 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. diff --git a/extensions/history/index.ts b/extensions/history/index.ts index 4e7a5c7b3..66b1814a1 100644 --- a/extensions/history/index.ts +++ b/extensions/history/index.ts @@ -553,8 +553,12 @@ class PromptHistorySelector extends Container implements Focusable { const radioFull = scopeRadioText(this.scope, false); // Radio label compaction is fit-driven too: abbreviate only when the // full radio cannot fit the row it would occupy (user-directed paste). + // Stacked and compact rows print it after one leading space; inline + // mode always fits it by construction. const radioText = - width >= radioFull.length ? radioFull : scopeRadioText(this.scope, true); + width >= radioFull.length + 1 + ? radioFull + : scopeRadioText(this.scope, true); const mode = planHeaderLayout( width, leftWidth, diff --git a/extensions/history/store.ts b/extensions/history/store.ts index 08ac7a98b..aaba0a693 100644 --- a/extensions/history/store.ts +++ b/extensions/history/store.ts @@ -400,13 +400,36 @@ export function drainProject( ); } +/** + * Carry siblings a failed sweep carry-over wrote next to the global seed + * (`history-global.jsonl.carry-*.jsonl`, see carryIntoRewrite). They live + * in the store root, outside every project dir, so the global drain and + * delete list them explicitly. + */ +function listGlobalSeedCarries(root: string): string[] { + const prefix = `${path.basename(globalSeedPath(root))}.carry-`; + let entries: fs.Dirent[]; + try { + entries = fs.readdirSync(root, { withFileTypes: true }); + } catch { + return []; + } + return entries + .filter( + (e) => + e.isFile() && e.name.startsWith(prefix) && e.name.endsWith(".jsonl"), + ) + .map((e) => path.join(root, e.name)); +} + /** * Drain the GLOBAL scope: every project dir's files, mtime-newest-first, * deduped, capped — with the legacy global seed appended LAST (deliberate: * it is the least specific, migrated source, so per-project entries win - * recency and keep-first dedup favors them). With a `stateDir`, the - * tombstone filter applies and fails closed: an untrusted hidden.json - * blocks the drain (see DrainResult). + * recency and keep-first dedup favors them). The seed's carry siblings hold + * bytes appended to it after a sweep's read, so they drain right before + * it. With a `stateDir`, the tombstone filter applies and fails closed: an + * untrusted hidden.json blocks the drain (see DrainResult). */ export function drainGlobal( root: string, @@ -431,6 +454,7 @@ export function drainGlobal( ); } const sorted = sortFilesForDrain(files); + sorted.push(...sortFilesForDrain(listGlobalSeedCarries(root))); if (fs.existsSync(globalSeed)) sorted.push(globalSeed); // legacy last return drainWithHidden(sorted, limit, stateDir); } @@ -523,7 +547,10 @@ 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 + * sibling store file (read like any other) so they are never lost. A failed + * append may have written part of the bytes: that fragment is trimmed back + * to the last newline first, so the owner's next append cannot merge with + * it into one corrupt line (the sibling holds every carried byte). A torn * last line is completed so the sibling stays parseable. Throws only when * both writes fail. */ @@ -531,6 +558,7 @@ function carryIntoRewrite(file: string, carried: Buffer): void { try { fs.appendFileSync(file, carried); } catch { + trimTornTail(file); const text = carried.toString("utf8"); fs.writeFileSync( `${file}.carry-${process.pid}-${Date.now()}.jsonl`, @@ -540,6 +568,25 @@ function carryIntoRewrite(file: string, carried: Buffer): void { } } +/** + * Truncate `file` after its last newline, dropping a newline-less fragment + * left by a partially written append. Bytes before the last newline are + * complete lines and are never touched. Best effort: never throws. + */ +function trimTornTail(file: string): void { + let fd: number | null = null; + try { + fd = fs.openSync(file, "r+"); + const bytes = readFrom(fd, 0); + const complete = bytes.lastIndexOf(NEWLINE) + 1; + if (complete < bytes.length) fs.ftruncateSync(fd, complete); + } catch { + // the carry sibling still keeps every raced byte + } finally { + if (fd !== null) fs.closeSync(fd); + } +} + /** * Remove every line whose prompt identity matches `text` from each file in * `files`, one atomic rewrite per affected file (see sweepFile). Files whose @@ -578,11 +625,15 @@ export function deleteFromProject( ); } -/** Delete every copy of a prompt from the GLOBAL scope (all projects + seed). */ +/** + * Delete every copy of a prompt from the GLOBAL scope (all projects, the + * seed, and the seed's carry siblings). + */ export function deleteFromGlobal(root: string, text: string): SweepResult { const files: string[] = []; const globalSeed = globalSeedPath(root); if (fs.existsSync(globalSeed)) files.push(globalSeed); + files.push(...listGlobalSeedCarries(root)); let projectDirs: fs.Dirent[]; try { projectDirs = fs.readdirSync(path.join(root, "projects"), { diff --git a/odd/tasks/history-review-advisories.md b/odd/tasks/history-review-advisories.md new file mode 100644 index 000000000..135166f2c --- /dev/null +++ b/odd/tasks/history-review-advisories.md @@ -0,0 +1,39 @@ +# History review advisories + +## Objective / authorization +Resolve the five non-blocking native-review advisories left on #1477 and #1480, in one PR, then merge. User authorized: "pr y merge" → "si". + +## Advisories +- [x] A1 (#1477, WARNING, `extensions/history/store.ts` ~535-539): sweep carry sibling for the global seed (`history-global.jsonl.carry-*.jsonl`) is not drained by `drainGlobal`. + - Outcome: REAL defect, fixed. The carry sibling lives in the store root, outside every project dir, so neither `drainGlobal` nor `deleteFromGlobal` listed it. New `listGlobalSeedCarries(root)` feeds both: the drain orders the carries (newest first) after every project file and right before the seed (their bytes are newer than the seed, legacy stays last); tombstones apply through the shared `drainWithHidden`. `deleteFromGlobal` sweeps them too, so a delete reaches every copy. + - Evidence: `tests/history-scope-delete.test.ts` "the global scope drains and deletes the global seed's carry sibling". RED: drained `['project-newest','legacy-keep']`, expected `['project-newest','raced in','legacy-keep']`. GREEN: passes (hidden raced line filtered, order kept, second delete removes the carry copy). +- [x] A2 (#1477, WARNING, `extensions/history/index.ts` ~739-744): hide-failure notice on the non-store (session-derived) delete path. + - Outcome: NOT a defect; no code change. The non-store branch of `executeDelete` notifies `hide.message` and returns before the splice. `hidePrompt` only returns two error messages ("Could not write the hide file; the prompt may reappear." and the hidden.json recovery message); neither claims the prompt is hidden. The branch is also unreachable from the UI: open-flow records come from drains as strings (no `source`, treated as `editor`), `deleteCurrent` drops session rows before arming, and the armed confirm is modal. + - Evidence: `tests/history-delete-confirm.test.ts` "a failed hide on the non-store path reports the hide error and keeps the row" (source-parse of the branch + both real `hidePrompt` error results: rename failure and corrupt hidden.json). Passes on unchanged code; no RED applies because no behavior changes. +- [x] A3 (#1477, SUGGESTION, `extensions/history/store.ts` ~1036-1040): a partial append during carry can leave a torn fragment. + - Outcome: REAL defect, fixed on the writer side. A failed `appendFileSync` could leave part of the carried bytes without a trailing newline; the owner's next capture merged with it into one corrupt line (lost prompt). `carryIntoRewrite` now calls `trimTornTail(file)` before writing the carry sibling: it truncates only bytes after the last newline (complete lines are never touched; best effort, never throws). The sibling still keeps every carried byte. Reader-side recovery was rejected: it would contradict the existing assertion in `tests/history-multi-reader.test.ts` that a torn fragment plus merged entry parses to null. + - Evidence: `tests/history-scope-delete.test.ts` "a partially written carry-over leaves no fragment for the next append". RED: file lines parsed to `['keeper', null]`. GREEN: `['keeper','after-carry']`, and the drain holds `after-carry`, `keeper`, `raced in`. +- [x] A4 (#1480, WARNING, `extensions/history/index.ts` ~1206-1220): live `overlayOptions` margin/visible function behavior is not proven by tests. + - Outcome: coverage gap, closed with tests; the behavior was already correct (no source change). New tests in `tests/history-overlay-margin.test.ts`: "the open picker's margin follows the real sidebar across the breakpoint and teardown" (real `installSidebar`, opened at 139 columns: no margin; 140: rail margin 54; 139: none; 140 then sidebar teardown: none) and "the margin getter reports the value refreshed by the last visible() pass". + - Evidence: characterization tests pass on current code. Sensitivity checked by a temporary mutation (removed the `rightMargin` refresh in `visible()`): both new tests and the existing live-margin test failed (3 failures); mutation reverted, 14/14 pass. +- [x] A5 (#1480, WARNING, `extensions/history/index.ts` ~556-557): scope radio fit check may be off by one column. + - Outcome: REAL defect, fixed. Stacked and compact rows print the radio after one leading space, but the fit check compared `width >= radioFull.length`; at exactly 34 columns the full radio was chosen and truncated with an ellipsis. The check is now `width >= radioFull.length + 1` (inline mode always fits it by construction). + - Evidence: `tests/history-header-layout.test.ts` "the full radio shows exactly when its row, leading space included, fits" (width == needed: full radio, exact row; needed - 1: abbreviated, no truncated header row). RED: `' ◉ Current project | ○ All projec…'`. GREEN: passes; the existing `RADIO - 1` test is unchanged and passes. + +## Constraints +Strict TDD by configuration (`node --experimental-strip-types --test`). Fix only still-valid defects; document any advisory found invalid with evidence. Route: delegated writer (2+ non-trivial files). + +## Checks +`node --experimental-strip-types --test tests/*.test.ts`, `node scripts/check-types.mjs`, `git diff --check`, native review, five required CI checks. + +## Progress +2026-09-26: branch `fix/history-review-advisories` from main `50b2af778`. +2026-09-26: A1-A5 resolved by the delegated writer (route: delegated, writer trigger: `store.ts` + `index.ts` + four test files). Docs: `docs/prompt-history.md` delete section now states that carry files belong to their source file's scope (A1 is user-visible). Not committed; commit, native review, PR, and merge remain with the parent. + +## Verification evidence (writer) +- `node --experimental-strip-types --test tests/*.test.ts`: 3897 tests, 3854 pass, 0 fail, 43 skipped. +- `node scripts/check-types.mjs`: exit 0 (188 recorded diagnostics, no regressions). +- `git diff --check`: clean. + +## Next step +Parent: work-unit commit(s), RDD assessment/review, PR, five required CI checks, merge. diff --git a/tests/history-delete-confirm.test.ts b/tests/history-delete-confirm.test.ts index f50759d68..3a95664c0 100644 --- a/tests/history-delete-confirm.test.ts +++ b/tests/history-delete-confirm.test.ts @@ -1,7 +1,10 @@ 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 { fileURLToPath } from "node:url"; +import { hidePrompt } from "../extensions/history/hide-prompts.ts"; import { deleteConfirmFooterText, deleteConfirmStep, @@ -406,3 +409,52 @@ test("storeDeleteNotice states exactly what remains after the sweep and the hide assert.ok(STORE_DELETE_PARTIAL_HIDE_FAILED_TEXT.includes("hiding failed")); assert.ok(STORE_DELETE_PARTIAL_HIDE_FAILED_TEXT.includes("may reappear")); }); + +// Review advisory A2 (#1477): the non-store (session-derived) delete path +// must never claim a prompt is hidden when the tombstone write failed. +// Evidence that it does not: that branch surfaces hidePrompt's own error +// message and returns before the row leaves the list, and every error +// message hidePrompt returns says the hide did NOT happen. The branch is +// also unreachable from the UI: deleteCurrent drops session rows before +// arming, and the armed confirm is modal. +test("a failed hide on the non-store path reports the hide error and keeps the row", () => { + const body = executeDeleteBody(); + const branchAt = body.indexOf('if (hide.status === "error") {'); + const noticeAt = body.indexOf("if (sweep) {"); + assert.ok(branchAt >= 0 && noticeAt > branchAt); + const branch = body.slice(branchAt, noticeAt); + const nonStoreAt = branch.indexOf("if (!actions.deleteFromEditorStore) {"); + assert.ok(nonStoreAt >= 0, "the non-store path handles its own hide failure"); + const notifyAt = branch.indexOf('this.onNotify?.(hide.message, "error");'); + const returnAt = branch.indexOf("return;", notifyAt); + assert.ok(notifyAt > nonStoreAt, "it surfaces hidePrompt's own message"); + assert.ok(returnAt > notifyAt, "and aborts before the splice"); + assert.ok(body.indexOf("this.records.splice(") > branchAt + returnAt); + + const messages: string[] = []; + const unwritable = fs.mkdtempSync(path.join(os.tmpdir(), "pi-history-a2-")); + const realRename = fs.renameSync; + fs.renameSync = ((from: fs.PathLike, to: fs.PathLike) => { + if (String(to) === path.join(unwritable, "hidden.json")) { + throw Object.assign(new Error("simulated EACCES"), { code: "EACCES" }); + } + return realRename(from, to); + }) as typeof fs.renameSync; + try { + const failed = hidePrompt(unwritable, "secret"); + assert.equal(failed.status, "error"); + if (failed.status === "error") messages.push(failed.message); + } finally { + fs.renameSync = realRename; + } + const corrupt = fs.mkdtempSync(path.join(os.tmpdir(), "pi-history-a2-")); + fs.writeFileSync(path.join(corrupt, "hidden.json"), "{not json", "utf8"); + const refused = hidePrompt(corrupt, "secret"); + assert.equal(refused.status, "error"); + if (refused.status === "error") messages.push(refused.message); + + for (const message of messages) { + assert.doesNotMatch(message, /\b(is|was) hidden\b/i, message); + assert.match(message, /may (then )?reappear/, message); + } +}); diff --git a/tests/history-header-layout.test.ts b/tests/history-header-layout.test.ts index e188aac99..d0c703e36 100644 --- a/tests/history-header-layout.test.ts +++ b/tests/history-header-layout.test.ts @@ -243,6 +243,19 @@ test("compact abbreviates the radio only when the full radio cannot fit", async assert.equal(lines[3]!.trimEnd(), ` ${SCOPE_RADIO_COMPACT_PROJECT}`); }); +test("the full radio shows exactly when its row, leading space included, fits", async () => { + // Stacked and compact rows print the radio after one leading space, so + // the full radio needs RADIO + 1 columns (review advisory A5). + const fits = await renderAt(RADIO + 1); + assert.equal(fits[3], ` ${SCOPE_RADIO_FULL_PROJECT}`); + const tight = await renderAt(RADIO); + assert.equal(tight[3]!.trimEnd(), ` ${SCOPE_RADIO_COMPACT_PROJECT}`); + assert.ok( + tight.slice(1, 4).every((line) => !line.includes("…")), + "no header row is truncated at the boundary width", + ); +}); + test("the list wheel band follows the header mode", async () => { // Inline: row 5 is the first list row — wheel down selects p13. assert.equal(await wheelAt(5, INLINE_WIDTH), "p13"); diff --git a/tests/history-overlay-margin.test.ts b/tests/history-overlay-margin.test.ts index 428dab14d..21fd22ac3 100644 --- a/tests/history-overlay-margin.test.ts +++ b/tests/history-overlay-margin.test.ts @@ -273,3 +273,54 @@ test("the picker margin stays live while the overlay is open", async () => { assert.equal(options.visible?.(140, 50), true); assert.deepEqual(options.margin, { right: 54 }); }); + +// Review advisory A4 (#1480): the live margin must follow the REAL installed +// sidebar after the overlay opened, not only a hand-built state object. +test("the open picker's margin follows the real sidebar across the breakpoint and teardown", async (t) => { + const { host, tui, root } = sidebarHost(139); + let uninstall = installSidebar(tui, shellTheme); + t.after(() => uninstall()); + root[NODE](); + const options = await captureOverlayOptions(host.terminal); + assert.equal(options.margin, undefined, "opened below the breakpoint: no rail"); + + // Widening across the breakpoint paints the rail while the picker is open. + host.terminal.columns = 140; + root[NODE](); + assert.equal(options.visible?.(140, 50), true); + assert.deepEqual(options.margin, { + right: SIDEBAR_RAIL_COLUMNS + SIDEBAR_OVERLAY_PADDING, + }); + + // Narrowing below it removes the rail again. + host.terminal.columns = 139; + root[NODE](); + assert.equal(options.visible?.(139, 50), true); + assert.equal(options.margin, undefined); + + // Back at the breakpoint, then the sidebar is torn down: the rail + // disappears and the picker returns to the full window. + host.terminal.columns = 140; + root[NODE](); + assert.equal(options.visible?.(140, 50), true); + assert.deepEqual(options.margin, { + right: SIDEBAR_RAIL_COLUMNS + SIDEBAR_OVERLAY_PADDING, + }); + uninstall(); + uninstall = () => {}; + assert.equal(options.visible?.(140, 50), true); + assert.equal(options.margin, undefined); +}); + +test("the margin getter reports the value refreshed by the last visible() pass", async () => { + const state = { ...OWNING }; + const options = await captureOverlayOptions(terminalWithState(state)); + assert.deepEqual(options.margin, { right: 54 }); + // pi-tui re-reads margin several times per layout; between visible() + // passes every read agrees, and the next pass picks up the change. + state.active = false; + assert.deepEqual(options.margin, { right: 54 }); + assert.equal(options.visible?.(139, 50), true); + assert.equal(options.margin, undefined); + assert.equal(options.margin, undefined); +}); diff --git a/tests/history-scope-delete.test.ts b/tests/history-scope-delete.test.ts index b638da35e..907414153 100644 --- a/tests/history-scope-delete.test.ts +++ b/tests/history-scope-delete.test.ts @@ -7,11 +7,14 @@ import { appendSessionCapture, deleteFromGlobal, deleteFromProject, + drainGlobal, drainProject, globalSeedPath, openSessionWriter, + parseStoreLine, projectHash, } from "../extensions/history/store.ts"; +import { hidePrompt } from "../extensions/history/hide-prompts.ts"; // Scope delete (design v2): sweepFiles' atomic per-file rewrite semantics // plus the project/global delete entry points. Synthetic project cwds — @@ -295,3 +298,114 @@ test("raced lines survive a failed carry-over append in a sibling store file", ( [], ); }); + +/** Fail the sweep's Buffer carry-over append to `target`; `partial` bytes land first. */ +function withFailedCarry(target: string, partial: number, run: () => void): void { + const realAppend = fs.appendFileSync; + let failed = false; + fs.appendFileSync = (( + file: fs.PathOrFileDescriptor, + data: string | Uint8Array, + options?: fs.WriteFileOptions, + ) => { + if (String(file) === target && Buffer.isBuffer(data)) { + failed = true; + if (partial > 0) realAppend(file, data.subarray(0, partial)); + throw Object.assign(new Error("simulated ENOSPC"), { code: "ENOSPC" }); + } + return realAppend(file, data, options); + }) as typeof fs.appendFileSync; + try { + run(); + } finally { + fs.appendFileSync = realAppend; + } + assert.ok(failed, "the simulated carry-over failure must fire"); +} + +// Review advisory A1 (#1477): a failed carry-over on the GLOBAL seed writes +// `history-global.jsonl.carry-*.jsonl` next to it, outside every project +// dir. The global drain and delete must reach it like any store file. +test("the global scope drains and deletes the global seed's carry sibling", () => { + const root = makeRoot(); + const stateDir = makeRoot(); + const seed = globalSeedPath(root); + writeLines(seed, ["victim", "legacy-keep"]); + const dir = path.join(root, "projects", projectHash(PROJECT_A)); + writeLines(path.join(dir, "s.jsonl"), ["project-newest"]); + withFailedCarry(seed, 0, () => { + withAppendBeforeRename( + seed, + `${JSON.stringify({ v: 1, text: "hidden raced" })}\n` + + `${JSON.stringify({ v: 1, text: "raced in" })}\n`, + () => { + const result = deleteFromGlobal(root, "victim"); + assert.deepEqual(result, { filesAffected: 1, removed: 1, failed: 0 }); + }, + ); + }); + const carries = fs + .readdirSync(root) + .filter((f) => f.startsWith("history-global.jsonl.carry-")); + assert.equal(carries.length, 1, "the raced lines live in one carry sibling"); + assert.equal(hidePrompt(stateDir, "hidden raced").status, "written"); + + // Tombstones apply; the carry holds bytes newer than the seed, so it + // drains right before it, after every project file (legacy last). + const drained = drainGlobal(root, 1000, stateDir); + assert.equal(drained.status, "ok"); + if (drained.status === "ok") { + assert.deepEqual(drained.prompts, [ + "project-newest", + "raced in", + "legacy-keep", + ]); + } + + const result = deleteFromGlobal(root, "raced in"); + assert.deepEqual(result, { filesAffected: 1, removed: 1, failed: 0 }); + const after = drainGlobal(root, 1000, stateDir); + assert.equal(after.status, "ok"); + if (after.status === "ok") { + assert.deepEqual(after.prompts, ["project-newest", "legacy-keep"]); + } +}); + +// Review advisory A3 (#1477): a carry-over append that fails after writing +// part of its bytes must not leave a newline-less fragment in the rewritten +// file, where the owner's next append would merge into one corrupt line. +test("a partially written carry-over leaves no fragment for the next append", () => { + const root = makeRoot(); + const state = openSessionWriter(root, PROJECT_A, "other-instance"); + appendSessionCapture(state, "victim"); + appendSessionCapture(state, "keeper"); + withFailedCarry(state.filePath, 7, () => { + withAppendBeforeRename( + state.filePath, + `${JSON.stringify({ v: 1, text: "raced in" })}\n`, + () => { + const result = deleteFromProject(root, PROJECT_A, "victim"); + assert.deepEqual(result, { filesAffected: 1, removed: 1, failed: 0 }); + }, + ); + }); + appendSessionCapture(state, "after-carry"); + const lines = fs + .readFileSync(state.filePath, "utf8") + .split("\n") + .filter((l) => l.length > 0); + assert.deepEqual( + lines.map((l) => parseStoreLine(l)?.text ?? null), + ["keeper", "after-carry"], + "every line of the rewritten file parses", + ); + const drained = drainProject(root, PROJECT_A); + assert.equal(drained.status, "ok"); + if (drained.status === "ok") { + assert.deepEqual([...drained.prompts].sort(), [ + "after-carry", + "keeper", + "raced in", + ]); + } +});