feat: add ccrecall import --repair-gaps to fix stale-tail sessions - #212
Conversation
…nd add find_repairable_sessions
…gs from ship-time challenge review Fixes a data-integrity bug where multi-file repair candidates were force-reimported file-by-file, silently unlinking branch_messages for sibling files while reporting success. Also fixes a PID-guard concurrency gap, a batch-aborting SAVEPOINT failure path, a session-id-churn message-count bug, and outcome-bucketing/observability gaps.
Post-implementation cleanup for the stale-tail import repair feature (016), surfaced by the clean-code pass and known-issues walkthrough at the end of orchestration: - Hoist duplicated test helpers (`write_four_turns`, `age_past_grace_window`, `delete_message`) into `tests/conftest.py`; tie the grace-window offset to `STALE_TAIL_SECONDS` instead of a bare magic number - Reuse the shared `_DB` CLI parameter alias in `cmd_import`/`cmd_status` instead of redefining it inline - Extract `_SESSION`/`_PROJECT`/`_PATH` shared CLI parameter aliases used by `cmd_recent`/`cmd_search`/`cmd_search_messages`, replacing duplicated inline `Annotated` definitions - Annotate `_db_coverage_fingerprint`/`_cached_ok_fingerprint`/ `_record_ok_fingerprint`'s `cursor` parameter as `sqlite3.Cursor` No behavior change. Also resolves stale/completed known-issues.md entries (KI-001, KI-003, KI-004, KI-005), files the remaining structural decomposition and `_noop()` dedup follow-ups as GitHub issues (#210, #211), and archives the feature's task files now that all tasks are complete. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KQiGktKrBfB5Tx5Gjrjy32
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: 📝 WalkthroughWalkthroughThe change adds an opt-in ChangesStale-tail repair
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~60 minutes Change: Feature · Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant CLI
participant ImportRun
participant IngestionStatus
participant ImportRepair
participant Database
CLI->>ImportRun: invoke import with repair_gaps
ImportRun->>IngestionStatus: find repairable sessions
IngestionStatus-->>ImportRun: return stale_tail and ingestion_gap candidates
ImportRun->>ImportRepair: repair candidates
ImportRepair->>Database: savepoint and force-import
ImportRepair->>IngestionStatus: reclassify session
ImportRepair-->>ImportRun: return repair totals
ImportRun-->>CLI: print summary and exit status
Merge Risk: 🟡 Moderate · up to The repair command can miss repairable damage or stop before processing remaining sessions, and an initialization failure can weaken concurrent-import protection. These issues should be corrected before merge. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Out of Scope Changes checkExplanation Most changes support issue Full details: Docstring CoverageExplanation Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 86 functions across 10 files. (2 skipped: 2 unsupported.) ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
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. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 45462279d0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
A failed RELEASE SAVEPOINT during --repair-gaps left the savepoint open on the connection instead of discarding it, so the next candidate's SAVEPOINT nested inside the "failed" one rather than starting its own independent, durably-committing transaction — breaking the per-candidate durability model documented in import_repair.py's module docstring. Roll back the candidate's writes and release the savepoint explicitly before continuing the batch. If that recovery itself fails (a persistent, not transient, problem), escalate to the same abort-the-batch handling already used for genuine force-reimport infrastructure failures, with an explicit log entry — rather than let an unhandled exception surface without ever recording what actually happened. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KQiGktKrBfB5Tx5Gjrjy32
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1506a1a407
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Advanced
Run ID: bae02dce-4789-48fb-9ad4-b927a3d11097
📒 Files selected for processing (12)
design/specs/016-stale-tail-import-repair/design.mddesign/specs/016-stale-tail-import-repair/known-issues.mdsrc/ccrecall/cli/commands.pysrc/ccrecall/hooks/import_conversations.pysrc/ccrecall/hooks/import_repair.pysrc/ccrecall/ingestion_status.pysrc/ccrecall/session_ops.pytests/conftest.pytests/test_cli_smoke.pytests/test_import_pipeline.pytests/test_import_repair.pytests/test_ingestion_status.py
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
repair_sessions() called sort_session_files(filepaths) before the per-candidate try/except/SAVEPOINT block. A file that disappears between find_repairable_sessions()'s snapshot and this call raised outside that guard, aborting the whole remaining batch instead of counting just this candidate as failed. import_session_group() already sorts its own input internally, and a single-file candidate has nothing to sort, so the pre-sort was both redundant and unguarded. Use filepaths directly instead. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KQiGktKrBfB5Tx5Gjrjy32
run()'s finally block deleted PID_KEY_IMPORT unconditionally unless _run() explicitly reported the acquisition was denied. But when repair_gaps=True and _run() raises before it even reaches its own try_acquire_pid_file() attempt (e.g. load_settings()/setup_logging() fail first), repair_lock_denied never got updated from its default False — so finally deleted a marker this invocation never acquired, potentially un-guarding a genuinely live holder such as the SessionStart-spawned background import. Default repair_lock_denied to repair_gaps instead of False: whenever repair_gaps=True, assume 'not yet acquired' until _run() proves otherwise by returning. repair_gaps=False is unaffected — _run() never touches PID_KEY in that case, so unconditional deletion on exit stays the existing, correct contract with _spawn_background. If _run() does acquire the lock and then raises later, this same default causes this invocation's own marker to be left behind too — deliberately not distinguished, since try_acquire_pid_file's liveness probe reaps a dead PID's stale marker on the next acquisition attempt. Adds a regression test simulating the early-exception race. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KQiGktKrBfB5Tx5Gjrjy32
Two related robustness fixes to session classification: - _db_coverage_fingerprint()'s link half only counted branch_messages rows per active branch, so a same-count substitution (one link deleted, a different one added) left the fingerprint unchanged and hid the corruption behind a cached 'ok' verdict — the exact class of regression (Finding 1/6) this fingerprint exists to catch. Switched to fingerprinting actual per-branch linked-message UUID membership. A follow-up fix closes a gap in that first pass: the membership query is an INNER JOIN starting from branch_messages, so an active branch with zero links (including a newly created one) contributed no row and was invisible to the fingerprint. Added a separate branch_part listing active-branch ids independently of link membership. - classify_sessions() read transcript files again (_expected_uuids()) and stat'd them (_is_contiguous_suffix branch) after _source_fingerprint() had already confirmed they existed — a file deleted in that window raised FileNotFoundError uncaught, aborting the whole classification generator and therefore every session summarize_ingestion()/find_repairable_sessions()/reclassify_session() was still trying to classify, not just the one with the vanished file. Now contained per-session, yielding missing_source and continuing, matching _source_fingerprint()'s existing pattern. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KQiGktKrBfB5Tx5Gjrjy32
design.md's Status is archived, so its Approach section body is frozen per repo convention rather than rewritten to match the final implementation. Appended a dated Addendum entry instead, noting two places the shipped control flow diverges from what the doc describes: savepoint-recovery failures also abort the repair batch (not just force-reimport failures), and the PID guard is acquired before the per-project import loop, not after it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KQiGktKrBfB5Tx5Gjrjy32
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 24cf4e9cf7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
classify_sessions()'s existing_msg_uuids check compared expected active-path UUIDs against any message row for the session (SELECT uuid FROM messages WHERE session_id = ?), not messages actually linked to the active branch via branch_messages. A message row surviving while its active-branch link was dropped or substituted — the exact corruption class find_repairable_sessions() exists to catch — still counted as 'present', so the session classified as ok and got cached that way, permanently hiding the corruption from --repair-gaps. existing_msg_uuids is now scoped through branch_messages -> branches (is_active = 1) -> messages, matching the join pattern _db_coverage_fingerprint() already uses for the same reason. Required updating test_ingestion_status.py's shared _seed_session() fixture to actually create a matching active branch + links (it previously inserted only into messages, so no test in the file could distinguish 'row exists' from 'row is reachable from the active branch') — added link_active_branch=False plus two small helpers (_link_active_branch, _link_messages_to_branch) for tests that build custom branch topology, switched two tests to conftest.py's existing delete_message() (a bare DELETE now violates the branch_messages FK), and rewrote test_branch_link_substitution_invalidates_ok_cache to model a realistic same-count/different-membership corruption. Added test_present_but_unlinked_message_is_not_counted_as_ok as a direct regression test. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KQiGktKrBfB5Tx5Gjrjy32
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6c48ab19bd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…nt, dangling refs - Check PID file ownership before cleanup: run()'s finally block now compares the marker's stored PID against os.getpid() so a concurrent holder's marker is preserved instead of unconditionally deleted. - Contain reclassify_session failures per-candidate: a transcript that becomes unreadable between import and reclassification counts as failed instead of aborting the remaining batch. sqlite3.OperationalError still escalates to abort, matching the force-reimport's own policy. - Strip dangling 'Finding N' cross-references from comments/docstrings across src/ and tests/. The explanatory text is self-contained without them. - Add tests for PID marker preservation during plain import, reclassify failure containment, and SAVEPOINT acquisition failure. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KQiGktKrBfB5Tx5Gjrjy32
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 658b2dc046
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
🤖 I have created a release *beep* *boop* --- ## [0.25.0](v0.24.2...v0.25.0) (2026-09-17) ### Features * add ccrecall import --repair-gaps to fix stale-tail sessions ([#212](#212)) ([6e60dcb](6e60dcb)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). --------- Co-authored-by: nodejsmith-release-please[bot] <273458268+nodejsmith-release-please[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
1 participant
ccrecall import --repair-gapsccrecall import's fast-path skip check trusts a stat match (file_size/file_mtimeequal to the storedimport_logrow) as proof the DB already holds every message from a transcript. When a session's DB rows fall behind its JSONL for any other reason, that stat match is a false positive —ccrecall importsilently no-ops forever, even thoughccrecall status --check-ingestionalready knows exactly which sessions arestale_tailoringestion_gap. Confirmed live against a real DB: 16 sessions, 334 turns unrepaired after runningccrecall import.ingestion_status.pygainsclassify_sessions(), a generator extracted fromsummarize_ingestion()'s per-session loop, so the aggregate counts and the new per-session repair candidate list can't drift on what counts as recoverable.find_repairable_sessions()filters that generator tostale_tail/ingestion_gapcandidates (session id, project id, on-disk file paths);reclassify_session()re-checks a single session's outcome after a repair attempt without a full-DB rescan.hooks/import_repair.py(repair_sessions()) force-reimports each candidate, bypassing the stat/hash skip gate, and classifies the outcome into three distinct buckets: repaired (nowok), unrepairable (stillstale_tail/ingestion_gapafter a full force-reimport — the source transcript genuinely lacks the missing messages, so re-attempting won't help), and failed (an operational error during processing). Each candidate gets its ownSAVEPOINT, so a poison transcript at candidate 12 of 16 doesn't lose the other 15 repairs.ccrecall importgains an opt-in--repair-gapsflag, wired throughcli/commands.pyandhooks/import_conversations.py. It shares the same PID guard as the SessionStart background auto-import and skips (with a printed notice, not an error) if that guard is already held —--repair-gapsholds a longer transaction than a normal import, so racing a concurrent import raises the odds of abusy_timeoutexpiry mid-batch.--project, and runs even when the project-scoped import step in the same invocation fails or exits early (e.g. an unsafe--projectpath) — the two are independent steps by design.Known issues carried forward
repair_sessions(),_run(),_sync_branches_and_messages(),classify_sessions()/summarize_ingestion()) exceed the 50-line guideline; deferred to a dedicated refactor with its own pinned-behavior tests rather than riskingrepair_sessions()'s carefully-reasoned durability model under time pressure._noop()is duplicated betweenimport_conversations.pyandimport_repair.py; de-duplicating requires resolving a circular import, which needs its own architectural call.find_repairable_sessions()snapshots classification once and doesn't re-validate a candidate's freshness immediately before its force-reimport. Low severity: force-reimport is idempotent andreclassify_session()re-checks the actual outcome after the fact, so a stale snapshot can't corrupt data or misreport a failed repair as successful.Housekeeping
Annotatedaliases, andcursortype hints surfaced during the clean-code pass.Closes #206
Summary by CodeRabbit
New Features
ccrecall import --repair-gapsoption to recover sessions with stale or missing ingestion data.Documentation