fix(history): resolve review advisories on carry, notices and layout

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 `<name>.carry-<pid>-<ts>.jsonl` store file instead.
+   in a sibling `<name>.carry-<pid>-<ts>.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.

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,

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,14 +547,18 @@ 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.
  */
 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"), {

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.

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);
+  }
+});

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");

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);
+});