Repository navigation
Part of #4366: fix(read): use window size as tracking baseline for --head-lines/--tail-lines - #4367
TechWizard9999 wants to merge 20 commits into
Conversation
- Add fs::create_dir_all for .claude dir in patch_settings_json_command and run_default_mode (fixes rtk-ai#2519, rtk-ai#4046 Windows regression) - Hoist OpenCode plugin installation before Unchanged early return in run_claude_md_mode_with - Fixes rtk-ai#4046 regressions.
The previous fix created the .claude directory unconditionally, but dry-run mode must not create directories. Only create the directory when not in dry-run mode. Fixes test: test_run_default_mode_dry_run_does_not_create_missing_claude_dir
- README.md: change --opencode description to 'Claude setup + OpenCode plugin' - supported-agents.md: clarify OpenCode installs Claude setup + plugin - troubleshooting.md: change --opencode label to 'Claude Code + OpenCode plugin' - main.rs: install Claude even when --opencode is passed (additive) - mod.rs: update help text to 'Claude setup + OpenCode plugin' Ports docs edits from rtk-ai#4052 to rtk-ai#4323.
…il-lines fast path
The function is used in tests but no longer in the main code path after the tracking baseline fix for --head-lines/--tail-lines.
📊 Automated PR Analysis
SummaryFixes gain token tracking for Review Checklist
Linked issues: #4366, #2519, #4046 Analyzed automatically by wshm · This is an automated analysis, not a human review. |
The test was using fields (original_cmd, input_tokens) that don't exist on CommandRecord. Fixed to use available fields: rtk_cmd and saved_tokens.
- Add back NamedTempFile import (used by other tests) - Remove unused InitContext import - Remove unused ctx variable in test - Remove #[expect(dead_code)] from regular_file_len (used in tests)
The test module imports Tracker and TempDir which are used in test functions, but clippy with --all-targets incorrectly flags them as unused.
Without stdin inheritance, cmd.output() gives the child an immediate-EOF stdin, causing curl to read an empty body for -d @-, -T -, --data-binary @-, -F field=@-, -K -. This mirrors the existing pattern in npm_cmd.rs / wc_cmd.rs (RunOptions::inherit_stdin). Fixes rtk-ai#4084.
The import was unused because the code used the fully qualified path std::process::Stdio::inherit() instead of the imported Stdio.
Resolved conflict in src/cmds/system/read.rs test imports.
KuSh
left a comment
There was a problem hiding this comment.
Round 1 of 3 — CHANGES REQUESTED
Claim: rtk read <file> --head-lines N / --tail-lines N books the N-line window, not the whole file, so gain stops claiming savings for rewritten head/tail (#4366).
Scope: accept if narrowed to "rtk read <file> --head-lines N / --tail-lines N is tracked as head -N / tail -N with the N-line window as its baseline, on every path of run()". Rebase on develop with only the read.rs commits: the curl stdin commit duplicates #4085 (and its Fixes #4084 would close that issue on merge), and the init/docs commits are already on develop through #4323. Make it Part of #4366, since the pipeline half of that issue (grep -n sample big.txt | head -3, still 7386 tokens saved) is not in this PR. #4153 changes the same track calls; this narrow fix lands first. (round 1, frozen)
Ran: develop 6f5503d, head 4c292b2 and the merge with develop × 20 rows under the hook (rtk rewrite, then the rewritten command) on the issue's 400-line file, plus 2 typed stdin rows; stdout and exit codes are identical between develop and the head on all 20 rows; invariants: under the hook, non-default targets (CRLF, empty file, .rs, N above the line count), accounting per row, every decision; savings: n/a, this PR fixes accounting, and the correct booking for a head/tail window is 0
Pins: 6 decisions, 6 mutated, 6 survived the suite (fast-path baseline, fast-path label, window-path label → cat, window-path head/tail swap, fallback label → cat, fallback head/tail swap)
Matrix: executed this round; pin check: executed this round
CI @4c292b2f: 3 failing: test (ubuntu-latest), test (macos-latest), test (windows-latest) — test_head_tail_tracking_baseline_is_window_not_full_file panics at read.rs:775 (index out of bounds: the len is 0 but the index is 0); the same panic locally
Blocks merge (2, frozen at round 1)
-
src/cmds/system/read.rs:67— ahead/taillabel over a whole-file baseline (3 track sites inrun(), and only the fast path is fixed). On the byte-window path the label now saystail -N <file>, but the baseline is still&bytes, the whole file. The hook reaches this path for everytail -n N <file>.Command (under the hook) develophead head -5 big.txtcat big.txt6673 → 82, saved 6591head -5 big.txt82 → 82, saved 0 ✓tail -n 5 big.txtcat big.txt6673 → 84, saved 6589tail -5 big.txt6673 → 84, saved 6589tail -n 5 crlf.txt298 → 30, saved 268 tail -5 crlf.txt, saved 268tail -n 5 big.rs774 → 26, saved 748 tail -5 big.rs, saved 748The PR description's tail row ("83 tokens") is not what the binary books. The filtered and numbered fallback (
-l aggressive,-n, typed only) has the same shape:rtk read big.txt -n --head-lines 5bookshead -5 big.txt7473 → 89, andrtk read big.rs --head-lines 5 -l aggressivebookshead -5 big.rs774 → 29. That label wascatbefore, so this PR makes those rows contradict themselves. A fix has to satisfy these rows: everytail -n Nrow above books input equal to the N-line window (saved 0 at--level none), the fallback'shead -N/tail -Nrows book the raw N-line window as input, andcat big.txtstill books 6673 → 6673. -
src/cmds/system/read.rs:758— unpinned behaviour, and the PR's own test is red on CI.run()records throughTimedExecution→Tracker::new(), never into the test'sTracker::new_in_memory(), sorecent[0]indexes an empty list on all three runners. Even reading the right DB,rtk_cmdis always"rtk read", socontains("head")cannot pass, andCommandRecorddoesn't carryoriginal_cmd, the label this PR changes. Evidence: each of the 6 mutations above leaves the failing set unchanged (26 pass, only this test fails), so nothing pins any of them. The test must run the issue's 400-line file throughrun()insidetest_isolation::with_rootonce per path (--head-linesand--tail-linesat--level none, plus a filtered or-nwindow) and once as a plaincat. It must readoriginal_cmdandinput_tokensback fromtest_isolation::db_path()and assert the label and that input equals the window's tokens (the whole file for thecatcontrol). Each of the six mutations, and putting&bytesback as the tail baseline, must turn it red.
Optional — will not hold merge
regular_file_lenhas no production caller left. Delete it andtest_regular_file_len_only_answers_for_a_regular_filerather than keeping it under#[allow(dead_code)].#[allow(unused_imports)]on the whole test module hides the duplicate imports (the in-functionuselines repeat the module-level ones). Drop the allow and the duplicates, and useexpect("…")instead ofunwrap()in the test.- The
head / tail / catlabel is built three times. On the byte-window path theheadarm can never run, because the fast path returns first for every head read at--level nonewithout-n. One label (and one baseline) computed once would keep the three paths from drifting apart again.
Filed as follow-ups
rtk read - --head-lines N/--tail-lines Non stdin still books all of stdin ascat - (stdin)(6673 → 84, saved 6589). It's outside this PR's hunks, and the hook rewrites none ofcat f | head -5,head -5 < fortail -n 5 < f, so only typed calls reach it. Absorbed by the existing #4366, comment added.
Checked and correct — no need to re-verify
- The
--head-linesfast path books the window on every shape tried: 82 → 82 on the issue's file, CRLF 29 → 29,.rs22 → 22, empty file 0 → 0, and N = 500 on a 400-line file 6673 → 6673. - Output is unchanged: stdout bytes, md5 and exit code are identical between
developand the head on all 20 rows, and the merge withdevelopbooks the same rows as the head. cat big.txtstill books 6673 → 6673, and--max-linesreads keep theircatlabel.
Your questions
- None asked yet.
Next
Rebase on develop with only the read.rs change, make it Part of #4366, give the byte-window and fallback paths the window as their baseline (rows in item 1), and replace the test with one that reads original_cmd and input_tokens from the test DB and that the six mutations turn red (item 2).
Rounds: 1/3. Threads: 2 open (waiting on the author), 0 resolved this round.
| }; | ||
| timer.track( | ||
| &format!("cat {}", file.display()), | ||
| &original_cmd, |
There was a problem hiding this comment.
Blocks merge 1: a tail label over a whole-file baseline. This path is what the hook reaches for every tail -n N <file>. The label is now tail -N <file>, but the baseline is still &bytes, the whole file. With the issue's file, tail -n 5 big.txt books 6673 → 84, saving 6589, the same as develop. Only the label moved from cat big.txt to tail -5 big.txt. The baseline here has to be the window (saved 0 at --level none), and the filtered/-n fallback at L128-135 needs the same rule: it now labels head -5 big.txt over 7473 → 89. The rows are in the review body, item 1.
| /// not the full file size. When `head -5 big.txt` is rewritten to `rtk read --head-lines 5`, | ||
| /// the tracking baseline should be the 5-line window, not the full file. | ||
| #[test] | ||
| fn test_head_tail_tracking_baseline_is_window_not_full_file() -> Result<()> { |
There was a problem hiding this comment.
Blocks merge 2: this test is red on all three runners and pins nothing. run() records through TimedExecution → Tracker::new(), so the Tracker::new_in_memory() read here is always empty and recent[0] panics (read.rs:775, index out of bounds). Even with the right DB, rtk_cmd is always "rtk read" and CommandRecord has no original_cmd, so the label can't be checked this way. Run run() inside test_isolation::with_root, read original_cmd and input_tokens from test_isolation::db_path(), and cover --head-lines and --tail-lines at --level none, a filtered or -n window, and a plain cat control. All six mutations listed in the body currently pass the suite; each has to turn this test red.
- Fix fast path (--head-lines): use window.len() instead of file size - Fix byte-window path: use window.len() and correct original_cmd - Fix fallback path: use actual command (head/tail/cat) for tracking - Fix test to use test_isolation::with_root and verify against test DB - Remove dead code (regular_file_len and its test) - Clean up test imports (remove unused TempDir)
- Fix byte-window path (--tail-lines): use window.len() instead of full file size - Fix fallback path: use window for tracking when head/tail lines specified - Fix test to use test_isolation::with_root and query test DB directly - Add rusqlite import for test
|
Sorry got corrupted , Tried to trigger CI but couldnt will create an new pr |
Summary
Part of #4366: Fix tracking baseline for
head -N/tail -Nrewrites inrtk read.Changes
Fast path (
--head-lines N): Changed tracking baseline fromcat {file}with full file size tohead -N {file}withwindow.len()(the actual N-line window size).Byte-window path (
--tail-linesor--head-lineswith-n): Fixed tracking baseline fromcat {file}with full file content to actual command (head -N/tail -N) withwindow.len().Fallback path: Updated tracking to use actual command (
head -N/tail -N/cat) instead of alwayscat.Added regression test:
test_head_tail_tracking_baseline_is_window_not_full_fileuses in-memory tracker to verify the fix.Removed dead code: Removed
regular_file_lenfunction and its test (no longer used).Fixed Regressions
head -5 big.txtcat big.txt(6673 tokens)head -5 big.txt(81 tokens)tail -5 big.txtcat big.txt(6673 tokens)tail -5 big.txt(83 tokens)grep ...(7846 tokens)Testing
test_head_tail_tracking_baseline_is_window_not_full_fileaddedTracker::new_in_memory()to avoid polluting developer's DBtest_isolation::with_rootfor proper database isolationFixes #4366