Skip to content

fix(history): harden GC and delete follow-ups, query-aware Home/End - #1477

Merged
Alan-TheGentleman merged 1 commit into
mainfrom
fix/history-reliability-followups
Sep 26, 2026
Merged

Alan-TheGentleman merged 1 commit into
mainfrom
fix/history-reliability-followups

Conversation

@Alan-TheGentleman

@Alan-TheGentleman Alan-TheGentleman commented Sep 26, 2026 •

Copy link
Copy Markdown
Collaborator

Closes #1476

Summary

  • Hardens prompt-history GC and delete paths flagged by native-review advisories, and makes Home/End query-aware in the selector.
  • Advisories judged by design or unreachable are documented in the task notes (ownerless migration lock needs manual recovery; lock rmdir failure is caught by the caller; sweep ENOENT during GC never reports a false delete).

Changes

File Change
extensions/history/store.ts Carry late complete lines into the compact before removing a claim; compact only when at least two files can merge (idempotent GC); failed sweep carry appends go to a .carry-*.jsonl sibling that drains like any store file.
extensions/history/index.ts Delete notice chosen after the tombstone write; malformed-preference disabled message; Home/End go to the search caret when the query is non-empty.
extensions/history/selector-helpers.ts storeDeleteNotice and the combined partial-failure text.
docs/prompt-history.md Selector keys, GC idempotence, carry sibling, lock recovery, notices.
tests/… Regression coverage for every fix, including a new selector key suite; session-writer test isolated from the real home.

Test plan

  • node --experimental-strip-types --test tests/*.test.ts → 3826 pass, 0 fail, 43 skipped.
  • node scripts/check-types.mjs → no regressions against the recorded baseline.
  • Native review approved.

Summary by CodeRabbit

  • Bug Fixes

    • History capture warnings now distinguish invalid or unreadable preferences from an explicitly disabled setting.
    • Home and End move through the history list when search is empty, and move the caret when a query is present.
    • Deleting history now reports combined sweep and hide failures more clearly.
    • History updates made during deletion or compaction are preserved more reliably, and compaction avoids unnecessary rewrites when only one file can be merged.
  • Documentation

    • Clarified history selector behavior, migration locking, deletion outcomes, and compaction requirements.

@Alan-TheGentleman Alan-TheGentleman added the type:bug Bug fix label Sep 26, 2026
@coderabbitai

coderabbitai Bot commented Sep 26, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 289e29af-25d6-44dd-a41b-2707238aff5b

📥 Commits

Reviewing files that changed from the base of the PR and between d615b2e and 7f2d968.

📒 Files selected for processing (12)
  • docs/prompt-history.md
  • extensions/history/index.ts
  • extensions/history/selector-helpers.ts
  • extensions/history/store.ts
  • odd/tasks/history-followups.md
  • tests/history-command-registration.test.ts
  • tests/history-delete-confirm.test.ts
  • tests/history-gc.test.ts
  • tests/history-off-path.test.ts
  • tests/history-scope-delete.test.ts
  • tests/history-search-caret-keys.test.ts
  • tests/history-session-writer.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.


📝 Walkthrough

Walkthrough

The changes update prompt-history capture warnings and selector key handling, combine store deletion outcomes into a notice, and adjust concurrent-write handling and compaction eligibility. Tests and documentation cover these changes.

Changes

Prompt history

Layer / File(s) Summary
Capture status and search keys
extensions/history/index.ts, tests/history-command-registration.test.ts, tests/history-off-path.test.ts, tests/history-search-caret-keys.test.ts, tests/history-session-writer.test.ts, docs/prompt-history.md
The selector identifies malformed or unreadable capture preferences. Home and End move the search caret when the query contains text; with an empty query, they retain list-navigation behavior. Tests cover these cases and use an isolated config home for capture checks.
Store deletion notices
extensions/history/index.ts, extensions/history/selector-helpers.ts, tests/history-delete-confirm.test.ts, docs/prompt-history.md
The history command selects a deletion notice after the tombstone attempt. The notice helper accounts for sweep failures and hide failures. Tests cover clean and partial outcomes.
Sweep and compaction reliability
extensions/history/store.ts, tests/history-gc.test.ts, tests/history-scope-delete.test.ts, docs/prompt-history.md, odd/tasks/history-followups.md
Sweeps carry late bytes into the rewritten file or a sibling file if needed. Compaction requires at least two mergeable files and copies complete late lines before removing claims. Tests cover concurrent writes, failure handling, and repeated GC. Documentation and task notes record the related reliability changes.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix

Suggested reviewers: carolitascl

Merge Risk: ⚪ Minimal · up to 7f2d9

The history changes are mergeable after normal checks. The remaining double-write failure was possible before this PR and is less likely with the new fallback.

Security Architecture Review

Security architecture risk: 🟡 Moderate · up to 7f2d9

Deletion handling is better at reporting partial failures, but the new compaction rule can leave a hidden prompt on disk longer. In a narrow high-volume case, that prompt could become visible again after its deletion record expires.

Retained concerns

  • Medium · security · inferred: Skipping compaction when only one file is eligible can retain tombstoned prompt text that the former single-file rewrite would have removed. If its bounded tombstone is later evicted, that surviving copy can reappear in history.
Security review details

Security Blast Radius

  • inferred — The identified exposure is a surviving prompt copy in a local project history store and its later visibility through history selection. The examined changes do not establish a new remote, cross-tenant, or privileged entrypoint.

Security Findings and Attack Paths

  • inferred — If a deleted prompt has a surviving or newly captured store copy in the sole eligible compaction file, the new guard leaves that copy untouched. Eviction of its bounded tombstone can then make it visible again. Successful physical sweeps remove existing copies, limiting this case.

Trust Boundaries and Controls

  • observed — The confirmed selector action remains provenance-gated. On a partial sweep it attempts a tombstone, and the resulting notice does not claim hiding succeeded when that write failed.

Resilience and Maintainability Implications

  • inferred — Store rewrites and tombstone persistence remain separate steps, so interruption between them is not a crash-atomic delete. Deferring the notice does not change that pre-existing ordering.

Hardening Proposals

  • proposed — Preserve a bounded way to purge tombstoned lines from a lone eligible file without rewriting an unchanged file on every shutdown, or ensure its tombstone cannot expire while that copy remains.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main changes: hardened history garbage collection and delete follow-ups, plus query-aware Home/End behavior.
Linked Issues check ✅ Passed Issue [#1476] requires fixes for history GC race and compaction behavior, failed carry handling, post-tombstone delete notices, malformed preference reporting, isolated session-writer tests, and query…
Out of Scope Changes check ✅ Passed The changed source files implement the reliability and selector behavior required by [#1476]. The added tests verify those changes. The documentation and task-note updates describe the same behaviors …
Docstring Coverage ✅ Passed Docstring coverage is 81.25% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 16 functions across 10 files. (2 skipped: 2…
✨ Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@Alan-TheGentleman
Alan-TheGentleman merged commit 5167c5c into main Sep 26, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type:bug Bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(history): reliability follow-ups and query-aware Home/End

1 participant