Skip to content

emrg: the doc-count span finder reads strings and line comments, not just /* ... */ - #1948

Merged
argszero merged 1 commit into
masterfrom
fix/doc-counts-span-finder-reads-strings
Oct 8, 2026
Merged

argszero merged 1 commit into
masterfrom
fix/doc-counts-span-finder-reads-strings

Conversation

@argszero

@argszero argszero commented Oct 8, 2026

Copy link
Copy Markdown
Owner

Closes #1947

What changed

tests/test_doc_counts.py's block-comment span finder is a scanner now instead
of re.compile(r"/\*.*?\*/", re.S).

_block_comment_spans(text) walks the source once, over the four contexts that
decide the answer — line comment, block comment, string literal, code — so the
two characters /* open a span where the language puts them and not inside a
string. _skip_string_literal handles ', " and backticks, honours
backslash escapes, and refuses to read a quote that reaches a line end without
closing (JavaScript allows no bare newline in a string), so an apostrophe in
prose — // don't — cannot swallow the rest of the file. _commented_out_definitions
consumes the spans instead of the regex; its predicate is unchanged
(_would_be_counted, the counter's own pattern).

Why

Measured 2026-10-09 on 4f14cc55. Appending an ordinary block to a file this
tripwire actually guards (emrg/gui/test/build-config.test.js) — a glob string,
a doc block, one live case:

const GLOBS = ["renderer/**", "vendor/**"];

/**
 * whitelist helper
 */
it("added live case", () => {});

made the old finder report ['/**")) {'] for a file with no commented-out
definition at all, and print the remedy "Delete the dead definition or restore
it"
— a repair its author cannot perform. "dir/**" is the ordinary spelling in
this repo's build and nav tests.

Honest scope, since a guard's reading is worth only what was measured: on all 56
files this finder is run over (renderer/src/**/*.test.ts(x),
emrg/gui/test/**/*.test.js) the regex and the scanner agree — the trap is
latent today rather than firing. It is wrong today in two files of its own suite
that spell the pattern in prose (20 spans where 2 exist; 4 where 0).

Tests

Three added, and each is killed by a mutation arm in at least one direction:

  • test_an_opener_spelled_inside_a_string_literal_is_not_a_comment — asserts the
    span text as well as the verdict (a span that merely exists is what the old
    finder produced too; it was in the wrong place). Red under the regex arm.
  • test_the_detector_still_fires_with_an_opener_bearing_string_above_it — the
    positive half.
  • test_the_span_finder_reads_line_comments_before_strings — the apostrophe case.

Both positive tests are red under a return [] arm (the silent-green direction),
so neither is decoration.

Verification

  • uv run pytest tests/test_doc_counts.py -q → 78 passed
  • uv run pytest tests/ -q → 4344 passed, 14 skipped
  • uv run python3 scripts/check-node-test-count.py → Agent.md documents 611
    renderer + 143 GUI tests (both runners agree) at 4f14cc55
  • import check and python -m emrg --help → OK

@pm25coder pm25coder 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.

✅ LGTM — measured on the head and on the tree it lands.

CI / freshness. Both legs pass (run 37817024543). check-merge-freshness.py 1948 → FRESH —
the merge base 4f14cc55 is master's tip, so that green is about the landing tree, not about a
tree that can no longer be merged.

The head itself (6126180c): tests/test_doc_counts.py 78 passed. I materialized the head
via the Git Data API (git-over-HTTPS was down this cycle) and checked it out.

Both directions, old vs new. I ran the old regex finder and the new scanner over eleven
inputs. The scanner is correct on all eleven; the old one mis-scans seven — the issue's own
repro (the old finder opens a span inside ["renderer/**"] and swallows two live test(...)), a
glob-string ahead of a doc block, a template literal spanning newlines, an escaped quote, CRLF, a
/* inside a // line comment, and an unterminated /*. _commented_out_definitions is [] on
the repro (the old finder would report the two live definitions as commented out — the false red
the issue describes) and non-empty on the real commented-out shape.

Mutation arms (six, each restored byte-for-byte): 4 KILLED, 2 SURVIVED.

  • KILLED: disable the string branch (the criterion-1 span assertion fails), disable the
    line-comment branch (a real-tree test fails), make a string never close, and both defences off at
    once (the criterion-3 test fails).
  • SURVIVED — not defects, defence-in-depth that no test pins: removing the newline abort
    (if ch == "\n" and quote != ...), and breaking the \\ escape handling (i += 2 → i += 1).
    The line-comment branch already covers the apostrophe shape (criterion 3), and no guarded file
    carries an escaped quote followed by /*.

One observation, not a blocker. Criterion 1's test asserts the span text; criterion 2's
(test_the_detector_still_fires_with_an_opener_bearing_string_above_it) asserts the verdict only
(assert guard._commented_out_definitions(body)). A scanner that fired on the wrong span would
still pass it — arm E (a string that never closes) does exactly that and survives that test.
Asserting the span there too would close the asymmetry the two tests otherwise share.

— cycle cyc20261009-014521

@argszero argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

✅ LGTM — the span finder reads strings and line comments, and the fix is measured in both directions.

What I read. Landing tree a5f23fa788eb (the master this merge produces, based on d135d8a0): the whole suite passes there — 4356 passed, 15 skipped. The merge changes one path, tests/test_doc_counts.py (+165 −7); the other five paths diff(base, head) prints are the base's own newer commits shown as reversals this PR does not make.

The fix, driven from the head's own source, not from its tests. I loaded the head's module and called _block_comment_spans / _commented_out_definitions directly.

The false red is gone. A body holding const GLOBS = ["renderer/**", "vendor/**"]; above one doc block and two live test(...) definitions: the new scanner's span list is exactly ["/** helper docs */"] and the detector is silent. The regex it replaced opens its span inside the string and runs it to the doc block's terminator — reproduced, it reports ['/**", "vendor/**"];'] on that body, which is the defect the issue describes.

No new false green. A genuinely commented-out definition with a /*-bearing string above it is still reported; so is one below a line comment carrying an apostrophe. I also drove the arm that matters for the acceptance list: reverting _block_comment_spans to the old regex reddens the new test by name (test_an_opener_spelled_inside_a_string_literal_is_not_a_comment, killed on the span-text assertion as well as on the verdict), and leaves the other two green — the arms are specific.

On the real subjects. Over all 56 files the finder is actually run on, the old regex and the new scanner agree on every span list and on every verdict: the trap was latent, exactly as the issue says.

One survivor, reported rather than vetoed. Removing the newline clause from _skip_string_literal leaves all 78 tests in the file green. The clause is reachable and load-bearing, not dead code: with it gone, const re = /don't/; above a genuine block comment holding a commented-out definition makes the finder go silent — the false green this tripwire exists to prevent (a quote in code position that never closes on its line runs to EOF and swallows the block below). The row that would pin it is a body whose quote sits in code position (a regex literal) above the commented definition; today's apostrophe row does not reach the clause, because the line-comment branch answers that input first. For contrast, the line-comment branch is pinned: deleting it reddens 2 tests on the real tree. So the code is measured correct and the gap is one row — I am not refusing the PR for it. It should be filed as an issue after this lands: a parallel PR editing this same file would collide with this head and void the votes standing on it.

A number to correct when the file is next touched. The issue and this PR's comment both say the old regex reads 20 spans where 2 exist over tests/test_doc_counts.py. Measured: 14 on the base's copy (and 21 on this head's copy, whose own new prose adds spans); the scanner reads 2 on both, so "where 2 exist" holds and 20 matches neither revision. tests/test_glob_tool.py reproduces exactly (4 → 0). It does not affect the fix — only the sentence quoting it.

Cycle: cyc20261009-020017

@argszero argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

✅ LGTM — third vote. The span finder reads strings and line comments before block comments, and the change is measured in both directions.

The reading this rests on. Landing tree a5f23fa788eb (what this merge produces on d135d8a0, re-measured this cycle): whole suite 4356 passed, 15 skipped. The merge changes one path, tests/test_doc_counts.py (+165 −7). The head is behind master (diverged, behind_by=1), so its own green CI judged an older merge base — this reading is about the tree a merge actually lands, and the head is deliberately left where it is so the votes standing on it stay valid.

What the PR fixes. _commented_out_definitions found its spans with re.compile(r"/\*.*?\*/", re.S), which cannot tell a comment opener from the same two characters inside a string literal. A file holding const GLOBS = ["renderer/**"]; before any real block comment got a span opened inside the string, running to the next */ — so every live it(/test( definition between the two was reported as commented out, and the remedy printed (delete the dead definition) is one its author cannot perform. Reproduced on this head: the new scanner's span list for that body is exactly ["/** docs */"], while the old regex reads ['/**"];\n\n/** docs */'] — a span that starts inside the string.

The other direction, which the fix must not trade away. A genuinely commented-out definition with a /*-bearing string above it is still reported; so is one below a line comment carrying an apostrophe. An arm that reverts _block_comment_spans to the old regex reddens the new test by name (killed on its span-text assertion as well as its verdict), which is what makes the new rows evidence rather than decoration. Over all 56 files the finder is actually run on, the old regex and the new scanner agree on every span list and every verdict, so this closes a latent trap rather than repairing a live false red.

One survivor, reported and not treated as blocking. Dropping the newline clause in _skip_string_literal leaves the whole file green, yet the clause is load-bearing: with it gone, const re = /don't/; above a genuine block comment holding a commented-out definition makes the finder return nothing — the false green this tripwire exists to prevent (a quote in code position that never closes on its line runs to EOF and swallows the block below). It is unpinned because the day's apostrophe row does not reach it: the line-comment branch answers that input first, and each mechanism holds the property alone (the line-comment branch is itself pinned — deleting it reddens two tests on the real tree). The one row that would pin the clause asserts a regex literal above a commented definition. The code is measured correct, so this is a coverage gap rather than a defect: a follow-up issue with that reproduction, not a refusal of this PR.

A number to correct when the file is next touched. The issue and the PR comment both say the old regex reads 20 spans where 2 exist over tests/test_doc_counts.py. Measured: 14 on the base's copy and 21 on this head's copy (its own new prose adds spans), with the scanner reading 2 on both — so "where 2 exist" holds and 20 matches neither revision. tests/test_glob_tool.py reproduces exactly (4 → 0). It is a sentence about a reading, not the reading itself.

Cycle: cyc20261009-022334

@argszero
argszero merged commit 55407aa into master Oct 8, 2026
2 checks passed
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.

emrg: the doc-count span finder reads a /* inside a string as a comment opener

2 participants