Skip to content

fix(read): adopt upstream #3377 — --max-lines as exact contiguous head count - #92

Merged
kylehgc merged 16 commits into
developfrom
fix/read-head-exact-contiguous
Aug 4, 2026
Merged

kylehgc merged 16 commits into
developfrom
fix/read-head-exact-contiguous

Conversation

@kylehgc

@kylehgc kylehgc commented Aug 4, 2026

Copy link
Copy Markdown
Owner

Closes #50.

What this adopts

Adoption of upstream rtk-ai/rtk#3377 by Mohammed Elkarsh (@mohammedelkarsh): replaces smart_truncate with a plain contiguous head_truncate in read.rs, making --max-lines N (the hook's head -N mapping) return the literal first N content lines plus the [K more lines] marker.

Why rtk-ai#3377 instead of the ticketed rtk-ai#2964 + rtk-ai#3166

Issue #50 named upstream rtk-ai#2964 (exact budget) + rtk-ai#3166 (contiguous window), which conflict with each other by construction and disagree on whether the marker counts against the budget (rtk-ai#2964: N content lines; rtk-ai#3166: N−1). During Phase 2 the upstream sweep surfaced #3377 (opened 2026-08-03), which supersedes both in one coherent change:

Adopting rtk-ai#3377 is one clean cherry-pick with no hand-authored conflict resolution; adopting the ticketed pair would have required rewriting rtk-ai#3166's diff during the pick. Verified: smart_truncate's only caller was read.rs:182; the FUNC_SIGNATURE/IMPORT_PATTERN regexes remain in use by AggressiveFilter (no dead code).

Commits

Commit Author Kind
653fb56 fix(read): honor --max-lines N as exact head count Mohammed Elkarsh Cherry-pick, byte-identical to upstream 91bec76
34bc096 test(read): pin head window to a literal contiguous prefix (rtk-ai#3046 shape) kylehgc Fork amendment

Amendment rationale: rtk-ai#3377's tests use plain a..f lines, so a re-introduced importance heuristic would pass them. The amendment reproduces the original rtk-ai#3046 corruption shape directly (60 plain lines, bare } at line 61, 30-line window).

Repro (debug binary, Windows x64)

Before (develop @ d08ba98) — both defects in one view:

$ rtk read plain10.txt --max-lines 4        # 10 plain lines
line1
line2
[8 more lines]                              # asked for 4, got 2

$ rtk read brace.txt --max-lines 30         # 62 lines, bare } at line 61
alias thing_1='do stuff 1'
...
alias thing_15='do stuff 15'
}                                           # pulled from line 61, false adjacency
[46 more lines]                             # 16 content lines for a 30-line ask

After (this branch):

$ rtk read plain10.txt --max-lines 4
line1
line2
line3
line4
[6 more lines]

$ rtk read brace.txt --max-lines 30 | tail -2
alias thing_30='do stuff 30'
[32 more lines]                             # 30 contiguous lines, no phantom }

Quality gate

cargo fmt --all (no-op) · cargo clippy --all-targets 0 warnings · cargo test --all 2641 passed / 0 failed across 18 suites (x86_64-pc-windows-msvc host toolchain).

Removed tests: the five smart_truncate unit tests died with the function (upstream's own diff); their invariant (kept + reported == total) is re-asserted by the adopted test_head_truncate_plain_text_exact_count and the fork amendment.

Deliberately not fixed

🤖 Generated with Claude Code

KuSh and others added 16 commits July 27, 2026 13:59
…AskRewrite

handle_vscode (rtk hook copilot's PascalCase path, shared by VS Code Copilot
Chat and Copilot CLI's Claude-compat entry) always set permissionDecision to
"allow" or "ask", diverging from process_claude_payload which only asserts
"allow" for an explicit user-configured Allow rule and stays silent otherwise.
Asserting "ask" is what caused rtk-ai#3037: Copilot CLI 1.0.66+ treats it as
authoritative and forces a blocking dialog with no "remember" option on every
rewritten command.

84aa4d6/0df6929 patched the wrong path (the camelCase native handler, which
never had this problem) with an auto-allow heuristic gated on an `explicit`
flag, instead of fixing handle_vscode itself. Replace both with the same rule
Claude's own hook and Droid's hook already use: never assert a decision for
AskRewrite, regardless of whether the verdict was Default or an explicit Ask
rule. This removes the Copilot-specific heuristic (and the now-unused
`explicit` field) in favor of one behavior shared across all hosts.

Also extracts vscode_response_from_decision as a pure, testable function
(mirroring copilot_cli_response_from_decision), closing a prior gap where
handle_vscode's decision output had no direct unit test coverage.
…ok config

rtk init --copilot registered both a PascalCase PreToolUse entry and a
camelCase preToolUse entry in the same rtk-rewrite.json, on the assumption
that VS Code Copilot Chat needs the former and Copilot CLI needs the latter.

Live testing showed Copilot CLI treats PreToolUse/preToolUse as two
independent, sequentially-run hooks — a redundant second `rtk hook copilot`
process spawn per tool call, chaining the first hook's rewrite into the
second's input (confirmed via raw stdin capture, and confirmed independent
of declaration order in the file). Also confirmed Copilot CLI honors the
PascalCase-only schema perfectly well on its own, receiving the same
tool_name/tool_input.command shape either way — so the camelCase entry buys
nothing for Copilot CLI, while adding process overhead and an extra,
harder-to-reason-about execution path.

Drop the camelCase preToolUse entry, keeping the single PascalCase
PreToolUse entry shared by both hosts. Existing installs are not upgraded
automatically — re-running `rtk init --copilot` / `rtk init --global
--copilot` overwrites the old dual-schema file with the new one
(write_if_changed overwrites unconditionally on content diff), verified
by test_copilot_init_upgrades_old_dual_schema_install and
test_copilot_global_install_upgrades_old_dual_schema_install, which seed
the old dual-schema content and assert it gets replaced.
detect_format only matched tool_name values "runTerminalCommand", "Bash",
and "bash" for the VsCode hook format. Live payload capture from a real VS
Code Copilot Chat session (agent mode, GitHub.copilot-chat) showed it
actually sends "run_in_terminal" — none of the previously recognized
values — so detect_format fell through to PassThrough and the hook never
fired at all for VS Code Copilot Chat: no rewrite, no permissionDecision,
nothing.

Add "run_in_terminal" to the recognized tool_name set. Verified against the
real captured payload end-to-end: a compound "cd <dir> && git status
--short --branch" command now correctly rewrites only the git segment to
"rtk git status --short --branch", leaving "cd" untouched, with no
permissionDecision asserted (consistent with the Default-verdict behavior
fixed in 042aeaf).
…ot generated

The doc comments described the camelCase toolName/toolArgs schema as "GitHub
Copilot CLI"'s format, which was accurate when rtk init --copilot registered
it. Now that only the PascalCase schema is generated, this path is reachable
only via not-yet-upgraded installs' leftover registration, or hosts that use
this schema under a different toolName value on their own (JetBrains/
IntelliJ's Copilot plugin sends "run_in_terminal", tracked in rtk-ai#2443/rtk-ai#3093).
Note both call sites accordingly so the code isn't mistaken for dead weight.
…owershell

Contributor testing on Windows 11 with Copilot CLI 1.0.73 (rtk-ai#3179) confirmed
Copilot CLI remaps its native bash/powershell shell tool to tool_name: "Bash"
under the PascalCase PreToolUse schema, and that its updatedInput is honored
end-to-end there. Since rtk init --copilot now only registers that schema,
Windows already works standalone through it — the camelCase toolName
"powershell" case (rtk-ai#3178/rtk-ai#3179) becomes legacy-only, relevant solely to
un-upgraded installs. Document that on both HookFormat variants.

Also fixes a HookDecision::AskRewrite struct-pattern leftover in
copilot_ide_response_from_decision (added by rtk-ai#3093, merged into develop
after this branch's own struct-to-tuple AskRewrite refactor), surfaced as a
compile error by rebasing onto develop to pick up rtk-ai#3179.
Coding agents routinely emit multi-line Bash blocks of sequential
commands. The rewriter tokenized newlines as plain whitespace, so a
block was treated as one command: the rtk prefix landed on the first
line and every following line ran raw and unfiltered.

Split multi-line input at the newline tokens the quote-aware lexer
emits (newlines inside quoted strings are never split points) and
rewrite each line through the existing single-line path. Blank lines,
comment lines, indentation, and CRLF separators are preserved verbatim.

Per-line rewriting only applies when every line is an independent
command. The whole block passes through untouched when:
- a shell keyword opens control flow (for/if/while/case/...)
- a list or pipeline continues across the line break (&&, ||, |, |&
  at a line edge)
- a subshell or group spans lines
- the block contains a heredoc (existing gate)

Ref rtk-ai#1243
Hardening from adversarial review of the multi-line rewrite:

- If any newline byte was swallowed by quote state (raw \n/\r count vs
  emitted newline tokens), pass the whole block through. The lexer has
  no comment awareness, so an apostrophe in a # comment opens quote
  state and hides subsequent lines; rewriting such a block would act on
  lines no permission verdict accounted for. Passthrough hands the
  original command to native permission handling. Quoted multi-line
  strings (commit messages) forgo their rewrite as the safe trade.
- Bail when (( or )) sits at a line edge: arithmetic spanning lines
  must not get a command prefix spliced into arithmetic context.
- Strip trailing unquoted comments before the independence checks:
  'git log | # keep pipeline' continues the pipeline across the
  newline even though the line ends in comment text, and rewriting
  the next line would rebuild a pipeline whose final stage the
  pipeline-safety guard may specifically reject (e.g. grep -f).
- Bail when a line's unquoted ()/{} don't balance: array literals
  (arr=(one), function bodies (foo() {), and groups span lines, so
  surrounding lines are not independent commands. Subsumes the
  previous single-char edge checks.
- Bail on any block containing $'...': inside ANSI-C quoting bash
  treats \' as a literal quote that does not close the string, but
  the lexer thinks it does — emitting a split point bash would never
  honor, the inverse of the swallowed-newline case the count check
  catches.
…rmission-decision

fix(hooks): stop Copilot from silently deciding permission on unconfigured commands
smart_truncate kept only max/2 plain-text lines, so hook rewrites of
head -N under-delivered half the requested content. Use head_truncate
with a recoverable [K more lines] marker instead.

Fixes rtk-ai#3370
… shape)

Fork amendment to the adopted upstream rtk-ai#3377: its tests use plain a..f
lines, so a re-introduced importance heuristic (bare }, exports pulled
from past the window) would slip through. This test reproduces the
original rtk-ai#3046 corruption shape directly.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@kylehgc

kylehgc commented Aug 4, 2026

Copy link
Copy Markdown
Owner Author

Review round (pre-merge, /code-review two-axis)

Standards axis: no hard violations. Judgement calls noted: (1) no ≥60% savings assertion — deliberate, this is an explicit-detail flag path where CONTRIBUTING.md's correctness-over-savings principle governs; (2) synthetic 3000-line fixture in the adopted exact-count test — acceptable per cli-testing.md inline-fixture guidance; (3) minor two-instance collect/slice/join duplication between the tail path and head_truncate — extraction would cost more than it saves; (4) deleting the unused _lang param and heuristic path flagged as a positive (Speculative Generality removed).

Spec axis: fully implemented. Independently verified the cherry-pick 653fb56 is byte-identical to upstream 91bec76 and the amendment 34bc096 is tests-only. One nuance recorded: on truncation the output joins with \n (CRLF files lose \r, no trailing newline before the marker) — identical to the pre-existing tail path, so consistent module semantics, not a regression.

No changes required; merging as reviewed.

@kylehgc
kylehgc merged commit 22f43ee into develop Aug 4, 2026
11 checks passed
@kylehgc
kylehgc deleted the fix/read-head-exact-contiguous branch August 4, 2026 15:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Adopt upstream #2964 + #3166: read - head-rewrite fidelity (exact budget, contiguous window)

5 participants