REQUEST CHANGES

The implementation advertises conservative, string-aware comment detection, but the runtime produces false-positive comment warnings for valid source and changes the final verdict based on how identical commentary is distributed across files. All three failures below were reproduced against commit 8e74aaa82d4ba06ec095d37680196731eafdb7d9.

CI is green. This is not a failing-test problem. The current tests do not exercise these contracts.

VERDICT

The deepest problem is architectural: lexical state is being inferred from a deliberately incomplete view of the post-image, while warning instances are treated as independent evidence even when they represent one declared metric. The public comments and README promise behavior the scanner and aggregator do not provide.

Do not merge until context-aware lexical state, multiline string handling, and one-warning-per-dimension aggregation semantics are fixed.

REQUEST CHANGES

The follow-up genuinely fixes two prior defects: unchanged context now participates in lexical state, and oversized_comment_block is emitted once per PR-wide dimension. The conservative parsing contract is still not satisfied, however. Hunk gaps are treated as known-normal state, and several advertised languages still have ordinary multiline string forms that become false-positive comments.

CI is green, and 166 relevant tests pass locally. This is not a failing-test problem. The current tests assert the implementation's chosen reset behavior without exercising the ambiguity that makes it incorrect.

VERDICT

The remaining blockers are architectural and implementation-level. A patch hunk does not establish that its first visible line begins outside a pre-existing string, and a generic quote scanner does not provide string-literal support for every language assigned to the C-family profile. Both errors violate the issue's explicit requirement that ambiguous syntax prefer false negatives.

Do not merge until unknown hunk-entry state cannot produce warnings and every advertised language profile either models its relevant multiline string forms or is removed from the supported set. The parallel file/category API should also enforce the filename invariant it already carries in both representations.

Summary

REQUEST CHANGES. PR #144 adds a default-on, deterministic policy.code_comments heuristic (oversized_comment_block, excessive_comment_lines, comment_heavy_diff) that reads ChangedFile.patch, reuses the categorizer's source + human_authored verdict, and feeds existing baseline_reviewability with no special verdict path. Against issue #143 and current origin/main (b567bf74), the config shape, inclusive warn/fail ladder, docstring/trailing-comment exclusions, ratio formula, sample-size guard, and HEAD dotenv-as-config exclusion still check out, but _iter_patch_lines treats any line starting with +++/--- as a file-header gap — so an added JS ++i (+++i in the patch) is never tallied as code, wipes lexer state, and splits comment runs on file-start hunks this heuristic does analyze. That is the same unified-diff collision this repo already fixed in parse_diff_right_side (+++ with a space). Dominant merge blocker is that default-on metrics/warnings are observably wrong on supported JS/Go file-start hunks; the 825-line cap breach and mid-file skip remain secondary.

Issue counts by severity

• bugs: 1
• suggestions: 5
• nits: 2

REQUEST CHANGES

Fourth round on this PR. The latest commits fix exactly what the third review flagged as the dominant blocker (+++/--- header collision with ++i/--x) and nothing else — git diff between the reviewed commit and HEAD touches only 46 lines, all in that one fix plus two tests. I independently re-verified the three earlier BLOCKING rounds are still fixed and did not regress: context lines participate in lexical state without being tallied, multiline strings (backtick/heredoc/triple-quote) carry across lines, oversized_comment_block is a single PR-wide warning, and analyze_added_comments now rejects mismatched files/file_categories pairs by filename, not just length. That's real, verified progress — I'm not going to pretend otherwise.

But I ran fresh, concrete reproductions against the current head (below, not reused from prior rounds) and found that the module-start-line gate this PR relies on to stay conservative (_hunk_entry_is_known) makes the heuristic silent on the realistic case it exists to catch, the just-applied header fix is demonstrably incomplete on a real shell idiom, and the language-skip logic manufactures exactly the false positive issue #143 forbids. CI is green and 1107 lines of tests pass — this is not a failing-test problem. None of the three repros below are covered by the test suite; each one exercises a supported language on a patch shape GitHub actually produces.

1. The heuristic never fires on a realistic edit to an existing file

_hunk_entry_is_known (code_comments.py:475-488) treats any hunk whose new-file start is > 1 as unknown and _tally_patch skips it entirely (code_comments.py:536-540). A git diff -U3 hunk only starts at line 1 for a brand-new file or an edit that touches the very top of the file. Every other edit to an existing file -- which is the issue's own motivating example, a 17-line block added in the middle of session.go -- produces @@ -N,... +N,... @@ with N > 1.

I reproduced this against the current head with a standard @@ -80,6 +80,23 @@ hunk adding a 12-line consecutive # comment block to an existing Python file (above the configured warn=10 threshold):

stats: comment_lines_added=0 code_lines_added=0 largest_comment_block_lines=0 comment_ratio=0.0
warnings: []

Zero. Not a smaller number -- the file is invisible to every dimension. This isn't an ambiguous-syntax case the conservative-parsing contract is designed to protect against; the hunk's own context lines (present in the patch, currently scanned but only when known=True) already establish plain code before the first added line. Throwing that away means the shipped feature will not fire on the median real-world PR that edits an existing file anywhere but its first few lines. The suite doesn't catch this because its own 17-line-block regression test wraps the hunk in @@ -1,1 +1,N @@ to force known=True -- it proves the implementation agrees with itself, not that the feature does what issue #143 asks for.

BLOCKING. Either use the hunk's own context lines to establish state once a non-comment, non-string context line has been seen (the hunk doesn't need to start at line 1 to be usable), or scan enough of the post-image outside the patch to establish real entry state. Skipping the case entirely is not a conservative default when the information to do better is sitting right there in the diff.

2. The just-fixed header collision is narrowed, not closed

The new fix matches file headers on the trailing space (+++ / --- , code_comments.py:510) instead of the bare prefix, closing the ++i/--x case from the last review. But it's still a content heuristic, and unified diffs don't guarantee that a deleted or added line's content never happens to start with + /- followed by another +/-. A deleted shell -- ) case arm -- the standard getopts end-of-options idiom, and Shell is a supported language here -- reproduces the exact collision the fix was supposed to close:

patch:
+# one
+# two
+# three
--- ) shift ;;      <- deleted line, content is "-- ) shift ;;"
+# four

classified: [('gap', '@@ ...'), ('added', '# one'), ('added', '# two'), ('added', '# three'), ('gap', '--- ) shift ;;'), ('added', '# four')]
largest_comment_block_lines == 3   (should be 4 -- the deletion must not split the run)

The deleted line is misclassified as a gap, which wipes _ScanState() and resets current_block in _tally_patch -- the identical failure mode from the second review ("deleted ---verbose ... also terminates a comment run"), just with a narrower trigger. A line-content heuristic can always be defeated by content that happens to match it; the diff format itself is unambiguous about where headers live (they precede the first @@ of a file's section). MAJOR. Detect file headers by position -- only before the first hunk header of a file's diff section -- not by re-sniffing every line's leading characters against a magic string.

3. Skipping unsupported-language source files from the ratio denominator manufactures the false positive issue #143 forbids

analyze_added_comments continues past any source/human_authored file with no language profile (code_comments.py:661-663), dropping it from total_code as well as total_comment. Reproduced: a PR adding 9 comment + 11 code lines in a .py file and 100 code lines in a .java file has a true PR-wide ratio of 9/120 ≈ 0.075 (.java is source + human_authored, just unmodeled) -- well under the 0.45 warn threshold. Current head instead reports:

stats: comment_ratio=0.45
warnings: [('comment_heavy_diff', 'medium')]

issue #143 is explicit: "Where accurate classification cannot be performed with reasonable confidence, prefer not counting the line. False negatives are preferable to noisy false positives." Dropping the .java file's code lines from the denominator inflates the ratio computed over the files the scanner can read and manufactures exactly the noisy false positive the issue forbids, on an ordinary mixed-language PR. MAJOR. Count a skipped-but-eligible source file's added non-blank lines toward the denominator only (never the numerator), or suppress comment_heavy_diff outright whenever unmodeled source files are present in the PR.

4. Module still breaches the written hard cap

code_comments.py is 831 lines. CONTRIBUTING.md:80 states a hard cap of 600 LOC for non-test source files, not a preference. This was raised last round and the module grew (from 825) rather than shrank. CI doesn't enforce this, so it's not a mechanical blocker, but it's a written repository rule this file is 231 lines over, on a brand-new core module where the fix history shows the file keeps absorbing more lexer/hunk-state complexity. MAJOR (policy violation, not a runtime defect). Split along the existing seams: a private lexer/patch-walking module and a short public policy-facing module (CommentStats, analyze_added_comments, warning builders).

Minor / nits (unchanged from round 3, still true against current head)

• config.py is 591 lines (under the 600 hard cap, over the 500 preference) because CodeCommentWarnThresholds/CodeCommentFailThresholds/CodeCommentPolicy (~120 lines, config.py:197-259) live in the shared Policy file instead of next to code_comments.py.
• The module docstring (code_comments.py:47-48) still says a run is split by a "non-added diff line," which reads as including deletions; deletions are supposed to be invisible (dropped, not tallied), so per the documented contract they shouldn't terminate anything -- finding 2 above shows that when the header-matching heuristic misfires, a deletion does terminate a run, which makes this line accidentally true for the wrong reason. Worth tightening once finding 2 is fixed.
• docs/DESIGN.md and docs/QUICKSTART.md still don't mention policy.code_comments, the three warning codes, or the four stats keys, even though this is a default-on heuristic that can move baseline_reviewability. README is accurate and thorough; the spec docs are not.

VERDICT

The first three review rounds forced real, verifiable progress on lexical correctness (context scanning, multiline strings, single PR-level block warning, filename-pairing validation) -- that work is solid and I checked it holds. What's left is not implementation typos: _hunk_entry_is_known's all-or-nothing skip is an architectural decision that trades the feature's entire reason for existing (catching verbose blocks added to existing files) for a safety margin the hunk's own context could satisfy more precisely; the header-collision fix treats a structural question ("is this line a file header") as a content-matching problem, which is the same category error that produced the last two rounds of bugs and will keep producing narrower versions of the same bug until it's fixed structurally; and the language-skip logic silently breaks the ratio formula's own denominator, which is a specification violation (issue #143's explicit false-negative-over-false-positive rule), not a matter of taste.

Do not merge until: (1) mid-file hunks with established context state are analyzed rather than unconditionally skipped, (2) file-header detection stops depending on line content and instead depends on position in the diff, and (3) unsupported-language source files stay in the ratio denominator (or the ratio dimension is suppressed) instead of silently vanishing from both sides of the fraction.

REQUEST CHANGES

Fifth round. This one is a real rewrite, not a patch: the module split into _comment_lex.py (syntax/scanner), _comment_scan.py (diff walking + hunk-entry establishment), comment_policy.py (config models), and a 361-line code_comments.py (policy surface) -- all four now under both the 500-line preference and 600-line hard cap, closing the LOC finding from round 4. config.py dropped to 483 lines for the same reason.

I re-verified all three round-4 findings against this head with the same reproductions I used last time, run fresh against the actual PR source (not re-used output):

• Mid-file hunks now fire. A standard git diff -U3 hunk (@@ -80,9 +80,26 @@, 3 lines of real leading context) adding a 12-line block now correctly reports largest_comment_block_lines=12 and emits oversized_comment_block. The new _hunk_entry_is_known / context-establishment mechanism in _comment_scan.py genuinely fixes the no-op-on-realistic-PRs problem, and it's backed by a dedicated test (test_mid_file_hunk_with_code_context_is_analyzed, explicitly commented as the reviewer reproduction).
• Header collision closed by construction. File headers are now detected by position (everything before the first @@ of a file's section is preamble), not content-sniffed. My round-4 shell -- ) repro now correctly reports largest_comment_block_lines=4. Also has a dedicated regression test.
• Ratio denominator fixed. _count_added_source_lines now feeds unmodeled-language source files into the denominator only. My mixed Python/Java repro now reports the true 0.075 ratio and no warning, with a dedicated test.

That's real, verified work, and crediting reviewer reproductions directly in test names/comments is exactly the discipline this back-and-forth was supposed to produce. But the fix for the first finding introduces a new gap that isn't tested and isn't honestly reflected in the public docs.

The hunk-establishment mechanism can misclassify docstring prose as commentary -- exactly the case the project repeatedly promises never to do

_CONTEXT_LINES_TO_ESTABLISH (_comment_scan.py:60-77) establishes a mid-file hunk once two consecutive context lines scan clean under a freshly reset _ScanState(). That reset is a guess about where the hunk begins -- confirming it only checks that the guess didn't visibly break (no open string/heredoc/block-comment after scanning), not that the guess was correct. If a hunk actually begins two lines into an already-open Python triple-quoted docstring, and those two lines of prose happen to contain no quote/hash/slash characters (ordinary English sentences), the scanner wrongly concludes it has established a normal code position -- and then classifies any subsequent added #-prefixed line as a real comment.

I reproduced this concretely: a docstring being extended with prose that documents a CLI example (a common pattern -- "Example usage:" followed by # do-this-thing lines) reports comment_lines_added=12, largest_comment_block_lines=12, and fires oversized_comment_block. Every one of those 12 lines is docstring content, not commentary.

stats: comment_lines_added=12 code_lines_added=0 largest_comment_block_lines=12 comment_ratio=1.0
warnings: ['oversized_comment_block']

The module's own docstring in this file names this exact risk ("a hunk that begins two or more lines into a multi-line string body") and calls it an accepted trade-off. Fair enough as an internal engineering note. But README.md states, three paragraphs after describing this exact establishment mechanism: "Python docstrings are string literals and may be runtime data, so they are never counted as comments" (README.md:343-344) -- flat, unconditional, no caveat. Issue #143 states the same thing as a MUST ("Do not silently treat every triple-quoted string as a comment because it may be runtime data"). The public contract and the actual runtime behavior disagree, and nothing in the test suite exercises this path -- the existing test_open_construct_context_never_establishes_entry_state test only covers a construct that stays visibly open across all three context lines, which is the safe case, not the dangerous one where a wrong guess happens to look clean.

MAJOR. I'm not asking for a perfect oracle -- the patch genuinely doesn't contain enough information to always get this right, and that's an inherent limit of diff-only analysis, not sloppiness. But ship one of these, not neither: (a) tighten establishment so it can't be fooled by two plain-looking prose lines (e.g. require evidence of an actual code token -- =, (, ;, a keyword -- not just "no error was detected"), or (b) if the current heuristic is judged good enough to keep as-is, stop stating the docstring exclusion as an unconditional guarantee in README/issue-facing docs and add a test that pins the known false-positive shape so a future change doesn't silently make it worse. Silence on this in the public contract is the actual problem, not the existence of an edge case.

VERDICT

This round fixed everything the last one asked for, verifiably, and shrank the module into a properly-decomposed, cap-compliant shape in the process -- that's the review process working as intended. The one thing left is that the fix for "mid-file hunks never fire" traded a total-silence failure mode for a narrow, real, reproducible false-positive failure mode against the single guarantee (docstrings are never commentary) this feature has restated in the issue, the module docstring, and the README. Close the gap between what the docs promise and what _comment_scan.py can actually deliver -- either technically or by being honest about the limit -- and this is mergeable.

APPROVE

Sixth round. This commit fixes the docstring-establishment false positive from round 5 correctly, and I re-verified it the same way as every prior round: fresh reproductions run directly against this head, not reused output.

My round-5 repro (a docstring extended with #-prefixed CLI-example prose) now correctly stays silent -- comment_lines_added=0, no warnings -- because _line_shows_code() requires an operator or keyword token, not just an error-free scan, before a context line counts as establishment evidence. I also re-ran the original round-4 mid-file-hunk repro (@@ -80,9 +80,26 @@, 3 lines of real leading context, 12-line added comment block) to make sure the fix for the no-op-on-realistic-PRs problem didn't regress under the new, stricter requirement -- it didn't; oversized_comment_block still fires correctly.

I then tried to defeat the new requirement itself: two lines of ordinary technical-writing prose that each happen to contain a parenthetical remark and a semicolon (the return value (see the docstring below) explains the format used / results are cached; see the notes section for details on eviction) still establish the hunk and still miscount the following docstring lines as commentary. But this is not an undisclosed gap -- it's precisely the residual case the new README paragraph names: "a hunk that begins two or more lines inside a docstring opened above Git's context window, where those leading context lines happen to carry a code token... a patch does not carry enough information to rule that out." That's an honest, accurate description of a fundamentally undecidable case from diff-only input, not a claim the code fails to live up to. Six rounds in, asking for a third iteration to shrink an acknowledged, disclosed, low-probability residual further would be manufacturing a blocker rather than finding one.

One loose thread, non-blocking: docs/DESIGN.md's "Hunk entry state" section (added in round 5, untouched by this commit) still describes the pre-this-fix mechanism -- "two consecutive context lines that all scan clean, with no string, block comment, or heredoc left open" -- with no mention of the code-token requirement that's now central to the guarantee. README and CHANGELOG both correctly describe the current behavior; DESIGN.md is now the odd one out. Since DESIGN.md is this repo's stated spec authority, it's worth a one-paragraph sync so a future reader of the design doc alone doesn't reach for the old, insufficient mental model. Doesn't block merge.

VERDICT

Across six rounds this feature went from a set of real, reproducible correctness defects (context blindness, unbounded multiline-string false positives, duplicate verdict votes, a heuristic that fired on essentially none of the PRs it was built for, a narrowed-but-still-real header collision, a ratio-denominator false positive, and a docstring-contract violation in the fix for one of those) to a design that I independently verified against real inputs at every stage and could not break on this pass. The remaining residual risk is disclosed, not hidden, and the file organization respects the repo's own LOC caps. LGTM -- fix the DESIGN.md paragraph whenever convenient.