Repository navigation
fix: allow clean-run summaries to bypass never_worse guard for injected JSON (fixes #4218) - #4266
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.
…ed JSON
Add empty_json_patterns to RunOptions so stdout-only filters that inject
JSON format can register their clean-run empty output (e.g., ruff's [],
pint's {"files":[]}). When raw stdout matches, the filter's human summary
is preferred over the raw JSON.
Fixes rtk-ai#4218.
📊 Automated PR Analysis
SummaryFixes issue #4218 where rtk's never_worse guard rejected the filter's human-readable summary in favor of raw injected JSON output (e.g. '[]') because the raw output was shorter, by letting ruff/pint register known clean-run empty-JSON patterns that bypass the guard. The diff also includes an unrelated change to src/core/retriever.rs that reverses a previously documented invariant (tee mode no longer avoids creating a sqlite artifact), which is not mentioned anywhere in the PR description. Review Checklist
Linked issues: #4218 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>
`[]` is a clean run only when rtk chose `--output-format=json`. A user who asked for JSON must get `[]` back, so `rtk ruff check --output-format=json | jq` keeps working on a clean tree; register the pattern only when rtk injected the format. Move the choice between the raw capture and the filter summary into guard_stdout so it can be tested, list the exemption in guard.rs next to the one for `rtk diff`, and drop the pint registration: pint runs under a tee label, which never reads it, and a clean pint run prints a ~140-byte JSON that already loses to its summary. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
KuSh
left a comment
There was a problem hiding this comment.
Round 1 of 3 — APPROVED
Claim: when rtk injects --output-format=json into rtk ruff check and the tree is clean, show the filter's Ruff: No issues found instead of ruff's raw [], which never_worse picked because it is shorter.
Scope: accept if narrowed to "the #4218 fix for rtk ruff check" (round 1, frozen). The two #4219 commits (9833d664, 637a2d18: the tee path creating the recall store) are the whole of #4263 and are reviewed there; #4219 is an open design question, since today a tee install leaves no sqlite artifact behind. The pint registration is dropped: pint never had the symptom.
Ran: rtk ruff (ruff 0.16.6) × 11 invocation forms (clean, dirty, user --output-format=json in both spellings, --output-format=gitlab, --exit-zero, --fix --exit-non-zero-on-fix, missing path, config error, bare check, format --check --output-format=json) × develop c75f1590 / head 7a446f14 / fixup bff334ca; rtk pint --test on pint 1.32.1's real clean JSON, captured in Docker and replayed; rtk grep in tee mode for recall.db creation. Invariants checked: under the hook (ruff check and uv run ruff check rewrite to rtk ruff check), every branch of the new decision (injected clean, user JSON, other formats, non-check subcommands, tee label), split streams (config error on stderr), and the accounting row. The clean row now books 1 → 6 tokens (−5). That is the intended trade: tracking keeps negative savings as they are. Every other row is unchanged against develop. An independent run of the fixup covered about 35 more forms (every --output-format value, --statistics, --diff, --add-noqa, -o, format --diff, stdin, stderr warnings): the fixup differs from develop only where stdout is exactly an injected [].
Matrix: executed this round
CI @bff334ca: all green (11/11; check-target skipped)
Blocks merge (2, frozen at round 1): both closed by the fixup
src/cmds/python/ruff_cmd.rs:114: the bypass fires for JSON the user asked for.rtk ruff check --output-format=json clean.py | jq lengthprints0on develop and fails withjq: parse erroron the head, because[]is swapped for the summary even when the user chose the format. Fixed by registering[]only whenis_check && !user_set_output_format, i.e. only when rtk injected the format. Both spellings are back to[]on the fixup.src/core/retriever.rs:387,433: out-of-scope behaviour change. In tee mode,rtk grepnow createsrecall.db(develop: no sqlite file). This reverses the invariant documented in the comment it replaces, and it is exactly #4263's change, so the fixup reverts it here and #4219 is decided on #4263.
Optional: will not hold merge
-
The pint registration
{"files":[]}was never read: pint runs with.tee("pint"), and the tee branch goes throughprint_with_hint, which never looks at the new field. Pint also never had the symptom: a clean pint 1.32.1 run prints a ~140-byte JSON (about,files,time,memory) that already loses topint: ok. Dropped in the fixup, and the field doc now says it is not consulted under a tee label. -
The decision moved into
guard_stdoutand got two tests. A mutation in each direction fails the matching test.guard.rsnow lists this exemption next to thertk diffone. -
After
--fixhas fixed everything, the summary readsRuff: No issues found. Develop printed[], and ruff's own "N fixed" line is lost on both sides in JSON mode. The exit code is propagated. A filter wording such as "no remaining issues" under--fixwould be more exact; that is a nit for a follow-up, not this PR. -
Every clean
rtk ruff checknow books a negative savings row (1 → 6 tokens). That is the intended trade: tracking keeps negative rows as they are. -
The option is renamed
clean_outputsto match what it holds (exact outputs, not patterns).
Filed as follow-ups
- #4218 stays open (the body now reads "Part of #4218"):
rtk lint ruffon a clean tree still prints[](it goes throughemit_guarded, not the runner), and the other injectors the issue lists are unchecked. Evidence added to #4218 (comment).
Checked and correct — no need to re-verify
- Dirty tree, missing path,
--exit-zero, barecheck: same output and exit code as develop. - A config error (stdout empty, error on stderr) is forwarded unchanged.
--output-format=gitlabandruff format --check --output-format=jsonprint[]on a clean tree and are unchanged: they are not registered, and their filters echo it anyway.- CRLF:
[]\r\ntrims to the pattern (pinned by a test).
Your questions
- (none; the review request is this round)
Next
fixup f9f40924 + bff334ca pushed: reverts the #4219 commits (kept on #4263), limits the bypass to the format rtk injected, drops the pint no-op and adds tests. Approving on green CI.
Rounds: 1/3. Threads: 0 open, 0 resolved this round.
Summary
Part of #4218. This PR fixes the case where
rtk ruff checkon a clean tree prints the raw injected JSON[]instead of the filter's human summary "Ruff: No issues found".Root Cause
When rtk injects
--output-format=json(or similar), the tool's empty-run output is extremely short (e.g.,[]= 2 bytes). Thenever_worseguard compares token estimates against this injected output and rejects the filter's informative summary becauseceil(21/4)=6 > ceil(2/4)=1.Changes
src/core/runner.rs: Added
empty_json_patternsfield toRunOptionsand logic to prefer the filter's summary when raw stdout matches a registered empty-run pattern.src/cmds/python/ruff_cmd.rs: Registered
[]as ruff's clean-run empty JSON pattern.src/cmds/php/pint_cmd.rs: RegisteredDropped in the maintainer fixup: pint runs under a tee label, and its clean output already loses to{"files":[]}as pint's clean-run empty JSON pattern.pint: ok.Testing