Repository navigation
fix: allow tee mode to record recall stats (fixes #4219) - #4263
2 commits merged into
Conversation
- Modify record_tee_elision() to create the recall store if needed - Modify record_tee_recall_with() to create the recall store if needed - Update misleading message in gain --recalls to mention tee mode This allows tee-only users to see their recall stats in 'rtk gain --recalls'. The stats table is lightweight and separate from the full recall store. Fixes rtk-ai#4219.
📊 Automated PR Analysis
SummaryFixes tee-only users never having recall stats recorded because record_tee_elision() and record_tee_recall_with() only opened an existing SQLite store rather than creating one. The functions now use open() instead of open_existing(), and the misleading CLI message in show_recall_stats() is updated to mention tee mode support. Review Checklist
Linked issues: #4219 Analyzed automatically by wshm · This is an automated analysis, not a human review. |
|
@KuSh — Could you please review this |
The two commits that let the tee path create the recall store fix rtk-ai#4219, which rtk-ai#4263 carries on its own. rtk-ai#4219 is an open design question (a tee install currently leaves no sqlite artifact behind), so it is decided there, and this branch stays on rtk-ai#4218. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
KuSh
left a comment
KuSh
left a comment
There was a problem hiding this comment.
Round 1 of 3 — CHANGES REQUESTED
Claim: tee-only installs get their recall stats in rtk gain --recalls: the tee stats writers create the recall store when it is missing, and the empty-state message says stats appear in sqlite or tee mode.
Scope: accept if narrowed to "rtk gain --recalls stops telling a tee-only install that stats are pending; tee mode keeps writing stats only into a store that already exists". Whether tee mode should create a store at all is the open question in #4219 (goal 1 vs goal 2) and stays there. (round 1, frozen)
Ran: rtk grep -rn alpha . over 60 files × 30 lines (9 elisions per run), reached through rtk rewrite first, plus the hook rewrite of a cat <tee log> read, × modes tee / sqlite / disabled / tee + RTK_RECALL=0 / tee + custom database_path / sqlite + database_path in a shared 775 dir, × develop c75f159 vs head 637a2d1 (binaries differ); invariants: under the hook, non-default targets, every decision (all three modes + kill switch); split streams and over-cap do not apply (filter output is 211 lines on both sides, unchanged); savings unchanged by this PR.
Matrix: executed this round
CI @637a2d18: all green (11/11)
Blocks merge (1, frozen at round 1)
src/core/retriever.rs:389(and:433) — the tee path now creates the recall store, which reverses a shipped guarantee: 473edaf in #3278 ("keep tee mode free of any sqlite artifact", listed in the v0.49.0 changelog). The change works as intended. On develop a tee-mode grep leaveshistory.db tee/, and on this head it also leaves a 36 KBrecall.dbholding the full store schema (recall,recall_stats,tee_reads). The same switch also sends tee mode intoopen()'screate_private_dir(parent). Withdatabase_pathin a shared directory, the directory's mode goes from775on develop to700on this head in tee mode (sqlite mode already does this on develop, filed as #4280).
The new comment "The stats table is lightweight and separate from the full recall store" does not matchinit_schema.recall_statsis created in the same file, by the same batch as the output table.
To merge on the narrowed claim, the change needs to meet these rows:- tee mode, no
recall.db, after the grep above and a hook read of a tee log:recall.dbis still absent (as on develop), andrtk gain --recallssays that tee mode records per-filter stats only into an existing recall store, so none are recorded here. It also namesrtk config recall sqliteas the way to get them. The current "appear once filters elide output" line is the wrong one in this state. - tee mode with a store that already exists: stats are recorded as today, and the table renders unchanged.
- sqlite mode, empty store: today's wording is correct, keep it.
- disabled mode: the message says recovery is disabled, rather than promising stats.
- a unit test in
retriever.rsthatrecord_tee_elisionandrecord_tee_recall_withwithmode: Teeand no existing store leave no db file. Nothing pins 473edaf today, which is why the suite stays green on this head.
- tee mode, no
Optional — will not hold merge
- With
RTK_RECALL=0the header showsMode: tee/sqlitewhile nothing is recorded. The same message branch could say that recovery is disabled by the environment. Same wording on develop; your call.
Filed as follow-ups
- #4280 recall: a configured database_path chmods its existing parent directory to 0700 — pre-existing in sqlite mode on develop, outside this PR's hunks once narrowed; filed.
Checked and correct — no need to re-verify
- sqlite mode: identical on both binaries (9 entries stored,
grep|sqlite|8|0,grep_skipped|sqlite|1|0, same hints). - disabled mode and tee +
RTK_RECALL=0: norecall.dband no tee files on either binary. - the hook read of a tee log is recorded once per file (
grep|tee|8|1on head), and a customdatabase_pathis honoured. cargo fmt --check,cargo clippy --all-targets(0 warnings),cargo test --allgreen locally; CI 11/11.
Your questions
- None asked beyond the review request.
Next
Drop the retriever.rs hunk, meet the message rows above with the no-store test, and change fixes #4219 to Part of #4219 so the goal-1-vs-goal-2 question stays open there. #4262 carries this same change plus unrelated init/OpenCode edits, so could you close it in favour of this one?
Rounds: 1/3. Threads: 1 open (waiting on the narrowing), 0 resolved this round.
| if let Ok(Some(conn)) = open_existing(cfg) { | ||
| // Create the store if needed to record tee elision stats. The stats table | ||
| // is lightweight and separate from the full recall store. | ||
| if let Ok(conn) = open(cfg) { |
There was a problem hiding this comment.
This switches the tee path from open_existing to open, which undoes 473edaf (#3278, v0.49.0: "keep tee mode free of any sqlite artifact"). On this head a tee-mode grep leaves a recall.db with the full store schema, and a shared database_path parent goes from 775 to 700. The acceptance rows are in the review body.
Summary
This PR fixes issue #4219 where tee-only users couldn't see their recall stats in
rtk gain --recalls.Changes
src/core/retriever.rs: Modified
record_tee_elision()andrecord_tee_recall_with()to create the recall store (SQLite database) if needed when recording stats. Previously, these functions usedopen_existing()which only opens an existing database, so tee-only users never had their stats recorded.src/analytics/gain.rs: Updated the misleading message in
show_recall_stats()to mention that stats work in both sqlite and tee modes.Root Cause
In tee mode, the recall store was intentionally never created (to "leave no sqlite artifact behind"). However, the
recall_statstable schema supports "tee" mode, andrtk gain --recallsdisplays tee stats - but tee-only users could never populate them because the database was never created.The fix allows the stats table to be created on-demand when the first stat needs to be recorded. The stats table is lightweight (just counters) and separate from the full recall store that stores command outputs.