Skip to content

fix(read): only treat /* at line start as block comment opener - #2397

Merged
KuSh merged 2 commits into
rtk-ai:developfrom
hgunduzoglu:fix/minimal-filter-comment-detection
Sep 25, 2026
Merged

KuSh merged 2 commits into
rtk-ai:developfrom
hgunduzoglu:fix/minimal-filter-comment-detection

Conversation

@hgunduzoglu

@hgunduzoglu hgunduzoglu commented Jun 11, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Part of #2385 — MinimalFilter dropped valid code lines containing /* as a substring, and worse: when no */ followed on the same line, in_block_comment leaked and silently swallowed the rest of the file.

Root cause

In src/core/filter.rs, the block-comment opener detection used trimmed.contains(start) instead of starts_with(start). Any line with /* anywhere — string literals, globs ("src/*.rs"), regex patterns — entered block-comment state and was dropped; since such lines rarely contain a closing */, every subsequent line was dropped too.

While reproducing, I narrowed the issue's reported scope in two ways (verified empirically on develop):

  • URL lines with // were NOT affected — line-comment detection already uses starts_with, so repro lines 1–2 from the issue pass through unchanged even before this fix. The real blast radius is /*-in-string + the state leak.
  • The same-line /* ... */ case (issue's bug 2) resolves itself once openers require line-start: only genuine comment lines enter block state now, and a comment-only /* inline */ line being dropped is the filter working as intended (consistent with the issue's "Expected" section: drop only when markers start the trimmed line).

Repro on develop vs this branch:

# develop — glob line dropped AND everything after it swallowed
$ rtk read example.rs --level minimal
const API: &str = "http://example.com/v1";  // endpoint
let re = regex::Regex::new(r"https?://\w+").unwrap();
fn foo() {}

# this branch — full file view, only the real comment line stripped
$ rtk read example.rs --level minimal
const API: &str = "http://example.com/v1";  // endpoint
let re = regex::Regex::new(r"https?://\w+").unwrap();
fn foo() {}
let glob = "src/*.rs";
fn bar() {}
fn baz() {}

Change

One-word fix (contains → starts_with) in the opener detection, with a comment documenting the constraint. Side benefit: Python inline triple-quote assignments (x = """inline""") are no longer dropped either (same contains path).

Testing

  • TDD: 5 new unit tests written first (2 red on develop):
    • /* in string literal kept + no state leak past it (the truncation regression)
    • Python x = """inline""" assignment kept
    • same-line /* inline */ comment dropped without leaking state
    • multi-line /* ... */ blocks still stripped (existing behavior guarded)
    • URL + trailing // comment line kept (documents that the issue's URL claim was already safe)
  • cargo fmt --all --check && cargo clippy --all-targets && cargo test — clean, 2155 passed.
  • Manual verification with the issue's repro file (above).

MinimalFilter used trimmed.contains() to detect block-comment openers,
so any code line containing /* as a substring (string literals, globs
like "src/*.rs", regex patterns) was dropped — and when no */ followed
on the same line, in_block_comment leaked and silently swallowed the
rest of the file. Match openers with starts_with() so only genuine
comment lines enter block-comment state.

Fixes rtk-ai#2385

@KuSh KuSh left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Round 1 of 3 — APPROVED, follow-ups filed

Claim: MinimalFilter opens a block comment only when /* (or the language's block opener) starts the trimmed line, so a /* inside a string or glob no longer drops that line and every line after it.
Scope: accept (round 1, frozen). I changed the issue link to Part of #2385 (it was Fixes #2385), because two shapes from #2714, which was closed into #2385, still reproduce (see "Filed as follow-ups").
Ran: rtk read -l minimal and -l aggressive on 19,822 real files (2,598 Rust, 2,984 C/C++, 2,495 JS/TS, 1,983 Go, 9,690 Python). Four builds, each with its own target dir and a distinct md5: merge-base 6785a6c, head 2b08f52, develop c50ff30, and develop + this PR. The scoring oracle for non-Python files is a lexer whose comment stripping matches gcc -fpreprocessed on 300/300 sampled headers; Python is scored against a tokenize reference. Also run: the PR's repros, #3425's repros, #2714's repros, Ruby =begin, CSS and extensionless files, and stdin. Invariants: under the hook (rtk rewrite "rtk read -l minimal …" returns the command unchanged; a plain cat rewrites to --level none, which does not reach this filter), every decision (Rust/C/JS/Go/Python/Ruby/Unknown/Data, /** doc exemption, the empty-output fallback), develop output ⊆ merge output checked on every file. Bash-output reduction at minimal: C 24.4 → 20.9%, Go 21.9 → 20.4%, JS 14.0 → 12.3%, Rust 6.8 → 5.8%, Python 6.5 → 6.4%. All of the drop comes from code lines that are no longer deleted. Three languages were already below the 20% floor (CONTRIBUTING.md) on develop (#3260), so this is not a finding against this PR.
Matrix: never — running now (this round)
CI @2b08f52b: all green, 11/11 (fmt, clippy, test ubuntu/macos/windows, Security Scan, semgrep, benchmark, doc review, test presence, CLA). The run is from 2026-06-26. The head merged onto current develop c50ff30 is clean and passes cargo fmt --all --check, cargo clippy --all-targets (0 warnings) and cargo test --all (3,960 passed, 0 failed) locally.

Blocks merge (0, frozen at round 1)

None.

What the change does to code loss (lines the lexer oracle counts as code that -l minimal drops), develop → develop + #2397:

files losing code code lines lost
C/C++ 1,072 → 53 20,354 → 764
Go 109 → 48 9,206 → 437
JS/TS 210 → 17 5,915 → 29
Rust 72 → 9 9,312 → 63
Python 1,562 → 202 5,342 → 1,029

On every one of the 19,822 files, develop's output is a subsequence of the merge's output. There are 4 apparent exceptions, each a whitespace trim(), raw-fallback or trailing-newline artifact, checked by hand. One of them is linecount.go: develop dropped the whole 5.4 KB file and fell back to raw, and the merge prints it correctly. rtk's own src/core/toml_filter.rs lost 2,100 of its lines on develop to a src/filters/*.toml doc comment; with the fix it reads in full. No line that develop kept is dropped.

Optional — will not hold merge

  • The Python branch of the block-comment check is now dead: block_start == doc_block_start == """, so starts_with(start) && !starts_with(doc_block_start) can never be true. It does no harm and #3336 replaces the Python path, so it is not worth a change here.
  • A block opened mid-line (foo(); /* start … */) now leaves its continuation lines in the output as comment text. Develop dropped them together with the foo(); line, so this is the right direction: comment kept, no code lost. It is part of the #2385 follow-up below.
  • Ruby's closer is contains("=end") while the opener is now starts_with, so an embedded =end inside a =begin block ends it early. This leaves comment text in the output and loses no code.

Filed as follow-ups

  • #2385 — kept open (link now Part of #2385); comment added. Two shapes from #2714 are still dropped whole on develop + #2397, and on develop + #3336 + #2397: /* multi-line\n comment */ int z = 3; and /* implicit */ Ctor(int k);. The /* x */ code shape is 749 of the 764 C code lines still lost. Both need span removal, not a line-level prefix test. Pre-existing, not made worse.
  • #1323 — absorbed; comment added. AggressiveFilter counts braces in any text Minimal passes through. Now that a mid-line-opened comment body reaches it, fn f() {\n a(); /* e.g. { {\n still */\n b();\n}\nconst TOP: u32 = 1; drops the top-level const at -l aggressive, where develop kept it. This reproduces only with a constructed input. On the 10,131 real non-Python files, aggressive output differs in 25 files outside the raw-fallback cases, and every hidden line is a brace or // ... implementation marker caused by real code that now reaches the aggressive pass. No const/static/signature/import line is lost because of comment text, and 11 comment lines in 2 files are newly kept.
  • #4249 rtk read -l minimal applies C comment syntax to extensionless files — outside this PR's hunks (Language::Unknown); filed. .gitignore = node_modules\n/*.log\n!/src\ndist/ prints only node_modules on develop and on this branch. The line-start /* is gitignore's anchored glob, and the output is not empty, so no fallback fires. This PR does fix the Makefile case (cp src/*.c /tmp/).
  • #3038 — absorbed. printf '/* x\nfoo\n' | rtk read -l minimal - prints nothing, exit 0, on both develop and this branch, because the stdin path has no empty-output fallback.

Checked and correct — no need to re-verify

  • The PR's own repro, #3425 finding 1 (Glob::new("src/*.rs") no longer swallows the file) and finding 2 (QUERY = """ keeps its line), and #2714's first case (int x = 5; /* inline comment */ keeps its line).
  • Mutation: reverting starts_with to contains fails exactly test_minimal_keeps_code_with_inline_block_marker and test_minimal_keeps_python_inline_triple_quote_assignment, as the PR body says. The other three tests pass on develop and guard behaviour that already worked.
  • Ruby s = "=begin", CSS url("img/*.png"), /** doc blocks and /// doc lines containing /* (e.g. /// https://docs.rs/bitflags/*/…, which develop dropped) are all now kept.
  • It is a logic induction, confirmed on the corpus: the merge's block state implies develop's block state line by line, so the change cannot drop a line develop kept.
  • The combination with #3336 (develop + #3336 + #2397): textually clean in both orders, non-Python output byte-identical to develop + #2397 on all 10,131 files, Python loss 8 lines in 2 files, cargo test --all 3,965 passed.

Hypotheses built and dropped

  • "A real /* … */ block no longer closes": dropped. Merge in-block implies develop in-block (induction plus the subsequence check on all 19,822 files), so no block can stay open longer than on develop.
  • "Aggressive loses top-level declarations because comment text now reaches it": reproduced only on a constructed input. It was dropped as a blocker after the real-file count above, run under the exact condition (rtk read -l aggressive <file>, both binaries), found 0 hits, and it went to #1323.
  • "Python docstring toggle inverts after a multi-line string assignment": reproduced, and develop inverts in the same way. #1322 and #3336 cover it.

Your questions

None asked.

Next

Nothing — approving. I changed the issue link in the description to Part of #2385, so the merge leaves it open for the two #2714 shapes. #3336 can land after this with no conflict. #3335 will then need a two-sided keep in the test block, because both PRs append tests after the same test.
Rounds: 1/3. Threads: 0 open, 0 resolved this round.

@KuSh
KuSh merged commit f3d4d9b into rtk-ai:develop Sep 25, 2026
12 checks passed
@rtk-release-bot rtk-release-bot Bot mentioned this pull request Sep 25, 2026
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.

2 participants