fix(discover): count hook-rewritten commands as coverage, not misses

[albatrossflyon-coder]

Summary

Fixes #3148.

rtk discover reads commands from Claude Code transcripts as the model emitted them — before the PreToolUse hook rewrites them. It classified every Supported command as a missed opportunity regardless of whether the hook was actually installed and had already rewritten that exact command at runtime, producing a real undercount of RTK usage (the issue reports ~19.5 points on a 30-day/359-transcript window).

Fix: re-derive per-command whether an installed hook would have covered this exact instance, using the same rewrite engine the hook itself calls (registry::rewrite_command, same function backing rtk hook check). This respects exclude_commands/transparent_prefixes config and the same unattestable-construct defer the hook applies (heredocs, command substitution, etc.) — so commands the hook genuinely couldn't/wouldn't touch still report as misses, only commands it actually would have rewritten move to "already using RTK."

Secondary fix, same issue: rtk proxy <cmd> deliberately runs the raw command unfiltered, but was being counted as coverage just because it starts with "rtk ". Excluded explicitly — the escape hatch shouldn't flatter the audit it's supposed to be exempt from.

Did not address the issue's third (explicitly lower-priority) finding — $(cat …) command substitution shim detection — to keep this PR scoped to the two miscounts the issue calls the actual bug.

Test plan

• Added covered_by_hook/is_already_rtk as small pure functions (matching the existing pattern in this codebase — hook_check.rs, rewrite_cmd.rs) with 7 new unit tests covering: hook not installed, rewritable command covered, unattestable construct still a miss, config-excluded command still a miss, plain rtk prefix counted, rtk proxy excluded, unrelated command not counted
• cargo test — 2478 passed, 0 failed
• cargo fmt --check — clean
• cargo clippy --all-targets — no issues

[pszymkowiak]

Verified on a compiled build — unit tests (7) cover all branches of covered_by_hook (hook-not-installed → miss, rewritable → covered, unattestable-construct → miss, config-excluded → miss) and is_already_rtk (incl. the rtk proxy exclusion). Also ran it end-to-end with a synthetic transcript via CLAUDE_CONFIG_DIR:

hook absent   → Already using RTK: 0 (0%)    ; grep / git status counted as MISS
hook present  → Already using RTK: 2 (50%)   ; git status $(whoami) stays a MISS, rtk proxy not counted

So rewritable commands correctly move from "missed" to "covered" when the hook is installed, while the unattestable-construct case still reports as a genuine miss (the hook would defer) and rtk proxy never flatters the count. Reusing registry::rewrite_command — the same engine the hook calls — is the right call for consistency with rtk hook check.

One minor semantic note (non-blocking): hook_installed reflects the hook's current state, not whether it was installed when each transcript was recorded, so very old pre-install transcripts could be over-counted as covered. Given discover is a heuristic and defaults to a 30-day window (where hook state is usually stable), and the pre-rewrite form is all the transcript preserves, this is a reasonable trade-off — maybe worth a line in the help/docs.

LGTM 👍

[KuSh]

Nice catch on the root cause (transcripts recording pre-rewrite commands), and reusing the real rewrite engine instead of the starts_with("rtk ") heuristic is the right call. But I think this reintroduces the same class of bug in the other direction:

hook_installed (src/discover/mod.rs:80) and the excluded/transparent-prefix config are snapshotted once from current state and then applied uniformly to every transcript entry across the whole since_days window (up to 30 days). Two concrete failure modes:

1. Hook installed after the history was recorded. If someone installs the hook today and runs rtk discover --since-days 30, every rewritable command from the prior 29 days — which genuinely ran unfiltered, since no hook existed yet — gets classified as already_rtk instead of a miss. That's not a hypothetical edge case; running discover right after installing rtk is probably the single most common trigger for this command.
2. Registry support added after the history was recorded. covered_by_hook() calls registry::rewrite_command() against whatever rtk version is installed now. If command support for X shipped after some of the scanned history was generated, those historical invocations get retroactively marked as covered even though the hook (if it existed at all at that point) had no rewrite for X yet.

Both stem from the same thing: ExtractedCommand (src/discover/provider.rs:13) carries no per-entry timestamp, and there's no stored record of when the hook was installed or which rtk version was active at any given point in the scan window — so there's no way to bound the "would this have been covered" check to the state that actually existed at that time.

Given the PR's own motivation is fixing a systematic ~19.5-point miscount, I'd want this to not trade one systematic miscount for another. Suggestions, roughly in order of effort:

• Minimal: bound the covered_by_hook check by hook-install mtime (if resolvable) so history predating install still counts as a miss, and document that command-registry-support drift isn't handled.
• Better: have the hook itself log its rewrite decision (allow/ask/deny/passthrough + rtk version) at the moment it runs, so discover reads ground truth for any window covered by that log instead of re-deriving it from current state. Happy to sketch this separately if useful — it'd also incidentally let us report whether "ask" rewrites were actually accepted, which today isn't tracked anywhere.

Not blocking if there's appetite to land the transcript-parsing fix now and treat the time-travel issue as a fast-follow, but I think it should at least be called out in the PR description / a tracking issue so it doesn't get read as "coverage stats are now trustworthy" when they're only trustworthy for windows where the hook + registry haven't changed.

[KuSh]

it seems this one should also be filtered by covered_by_hook even if less important

[albatrossflyon-coder]

Good catch, and you're right that this is a real time-travel bug, not just a theoretical edge case — hook_status() (src/hooks/hook_check.rs) only checks whether a hook is installed right now, there's genuinely no stored record of when it was installed or which rtk version was active at any point in the past.

For a minimal fix I can bound the scan by settings.json's mtime (resolve_claude_dir().join(SETTINGS_JSON)) — treat any transcript entry older than that as unknown/pre-hook and count it as a miss regardless of what covered_by_hook() says today. It's an approximation (mtime moves on any settings edit, not just the hook being added, and it does nothing for the registry-version-drift case you raised — a command whose rewrite support shipped after the fact still gets misclassified) but it directly fixes failure mode #1, which you called the most common trigger.

I'd rather not try to solve #2 (registry drift) in this PR — that really needs the hook to log its own rewrite decision + rtk version at runtime, which is a bigger persistence change than a bug-fix PR should carry. I'll open a tracking issue for that and link it here, and call out both limitations explicitly in this PR's description so nobody reads the merged stats as "fully time-accurate."

Will push the mtime bound + doc update to this branch — let me know if you'd rather see it split into its own follow-up PR instead of amending this one.

[KuSh]

@albatrossflyon-coder I've pushed a complete fix in #3206 that should superseed your PR. Could you check it before spending time updating your PR?

[albatrossflyon-coder]

Thanks for pushing through on this — #3206 is the right fix, not just a good-enough one. It solves both gaps you originally flagged (pre-install history via the real hook_decisions log, and registry/permission drift via measuring the actual PermissionVerdict at the moment the hook ran) instead of the mtime-bound approximation I was about to add here, which only ever addressed the first. The tool_use_id join and the hook-decision logging table are a materially better foundation than re-deriving coverage from current state, and the validation (hook-absent vs hook-installed-with-real-decisions, direct sqlite checks) is thorough.

Closing this in favor of #3206 — no need to duplicate review effort. Thanks for catching the gap and doing the deeper fix.