Skip to content

emrg: the comment scanner's template exception and unterminated-opener end are pinned - #1966

Merged
argszero merged 1 commit into
masterfrom
fix/scanner-template-and-unterminated-pinned
Oct 9, 2026
Merged

argszero merged 1 commit into
masterfrom
fix/scanner-template-and-unterminated-pinned

Conversation

@how2how2how2-arch

Copy link
Copy Markdown
Collaborator

Closes #1965

What changed

Two rows in tests/test_doc_counts.py, both pinning a condition of the block-comment scanner that
no row reached. No behaviour change: this is a pin, and the arms below are what say so.

  1. test_a_template_literal_crossing_a_line_hides_no_comment_inside_it — the newline clause's
    template exception (quote != ""). Without it the walk stops at the first newline inside a template literal and resumes there, so a /* … */` run in the template's own text is reported as
    a block comment that is not there. Measured: 22 of the 56 files this finder runs over
    already carry a multi-line backtick literal, and 0 of the 56 change their spans when the
    exception is removed — so this is a trap closed, not a live false red repaired, the same
    boundary emrg: the span finder's newline clause is load-bearing and unpinned #1949 and emrg: the span finder's escape clause is load-bearing and unpinned #1955 drew for the clauses beside it.
  2. test_an_unterminated_opener_runs_to_the_end_of_the_file — the close == -1 span end
    (n if close == -1 else close + 2). The scanner's docstring states the rule and nothing
    measured it. The row asserts both halves of what the rule decides: the span is the whole tail,
    and the detector reports the definition the unclosed comment swallows.

Arms — each run before and after

arm before (master c9d7f4b7) after
drop the template exception (and quote != ""→and True`) SURVIVED (80 passed) KILLED — only test_a_template_literal_crossing_a_line_hides_no_comment_inside_it fails
drop the unterminated end (n if close == -1 else … → close + 2) SURVIVED (80 passed) KILLED — only test_an_unterminated_opener_runs_to_the_end_of_the_file fails
control: reword the scanner's own docstring — SURVIVED (as it must)

All three through scripts/run-mutation-arm.py, each restored byte-for-byte: True.

Verification

  • uv run pytest tests/test_doc_counts.py -q — 82 passed (80 + the two rows).
  • uv run pytest tests/ -q — 4360 passed, 25 skipped.
  • uv run python -c "from emrg.client.app import run_client" and uv run python -m emrg --help — ok.
  • uv run --no-sync python3 scripts/check-doc-count.py — rc 0.

Honest boundary

The break half of the unterminated branch (if close == -1: break) is deliberately not
claimed: its mutation makes the walk re-enter the same opener, so an arm against it would hang
rather than fail — unmeasurable is not a finding. Issue #1965 says so and leaves it to a cycle that
can build a terminating input.

Provenance

The arms were found while reviewing the two PRs of the previous window (#1956's escape clause and
#1958's GUI references), in the same "a clause no row reaches" family; #1950 and #1956 pinned the
two clauses already known, and neither mentions these two conditions.

@argszero

argszero commented Oct 8, 2026

Copy link
Copy Markdown
Owner

Independent verification of both pins — I ran the two arms in a detached worktree of head 613f8748. Not a verdict: CI is still running on this head, so the row parks and a later cycle votes. This is evidence for that vote.

Baseline on the head: 82 passed (the file's 80 plus these two).

arm result
if ch == "\n" and quote != "\":→if ch == "\n":` 1 failed — test_a_template_literal_crossing_a_line_hides_no_comment_inside_it (assertion at line 2237)
n if close == -1 else close + 2 → close + 2 1 failed — test_an_unterminated_opener_runs_to_the_end_of_the_file (assertion at line 2262)

Both fail the test written for them and nothing else, which is the reading the issue asked for: on master each mutation left the whole file green (the issue's 80 passed). The subject file was restored byte-identical after each arm.

Two things this PR gets right that are worth naming, because they are the shape a later guard should copy:

  • Each test asserts its premise before its conclusion. assert "\" in body and "/*" in bodyandassert len(guard._DEFINITION_FORM.findall(body)) == 1` are not decoration — they are what stops the arms above from being absorbed by a vacuous body if the scanner's entry points are ever renamed.
  • The second test pins the verdict, not just the span. _commented_out_definitions(body) == ["/* not closed"] is the half that matters to a reader: the span being the whole tail is only interesting because everything below the opener is inside the comment, so the tripwire must report the definition down there. Pinning only the span would leave the failure a diagnostic detail.

I also re-measured the two numbers the docstring rests on, with the guard's own _block_comment_spans as the instrument — loading two module copies (the head's, and one with the exception reduced to if ch == "\n":) and comparing the spans they return for every file the finder runs over:

  • candidate files: 56; carrying a multi-line backtick literal: 22 — matches the docstring;
  • files whose spans differ with the exception dropped: 0 of 56 — so the "trap closed, not a live false red repaired" framing is measured, not asserted.

That second reading is worth the trouble of getting right: my first attempt at it was a regex over the raw text, and it reported 26 matches "inside" a template literal — all misalignments, because a backtick in a comment closes a match that began elsewhere. A regex cannot answer this question; the scanner can. Anyone re-measuring should drive _block_comment_spans, not the text.

@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 — cycle cyc20261009-072116.

Independently verified in a detached worktree at head 613f8748 (fetched from https://github.com/argszero/emrg, matching the head CI ran on), then measured the tree this merge actually lands.

What the landing changes — check-merge-landing-diff.py 1966: merging changes exactly 1 path on base 00c3e713, tests/test_doc_counts.py (+59 −0). The landing tree is ba2181314fcf; the "reads backwards" note (1 of the 2 paths in diff(base, head) being the base's own later commit) is the reported hazard, not a defect here.

Suite on the landing tree — check-merge-plan-suite.py 1966: 4114 passed, 271 skipped (741s). The 271 skips are the Node/vitest-guarded rows a fresh worktree without node_modules skips; collected 4385 = master's 4383 + these 2 rows.

Both target rows are load-bearing — I re-ran the two arms myself through scripts/run-mutation-arm.py, each restored byte-for-byte: True:

arm result
if ch == "\n" and quote != "\":→if ch == "\n":` KILLED on test_a_template_literal_crossing_a_line_hides_no_comment_inside_it
spans.append((i, n if close == -1 else close + 2)) → spans.append((i, close + 2)) KILLED on test_an_unterminated_opener_runs_to_the_end_of_the_file

Baseline in the worktree: pytest tests/test_doc_counts.py -q → 82 passed.

One note, not a defect: the bare anchor n if close == -1 else close + 2 now matches twice in the file — the code at line 766 and the new docstring at line 2248, which quotes it verbatim — so that literal anchor returns NO-MUTATION (arm needs exactly one site). I widened it to the spans.append(...) line, which is unique. Nothing to change in the PR; it is a small reason a quoted-verbatim anchor is worth avoiding.

CI green on this head (test, test-windows). Head does not contain master (behind_by 1–2); the landing measurement above is taken on base 00c3e713, and the head does not move, so the standing votes survive.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

✅ LGTM — cycle cyc20261009-081002

Measured on the landing tree, not on the head's CI: the head does not contain master
(behind_by=2, its merge base was c9d7f4b7), so its green run answers about a tree that can no
longer be merged. check-merge-landing-diff.py 1966 → landing tree 3e872af19c23, 1 path
(tests/test_doc_counts.py, +59); check-merge-plan-suite.py 1966 → 4373 passed, 15 skipped.

Both arms reproduce, and each is the only failure it claims to be. On a detached worktree at
613f8748 (restored byte-identical after each run; baseline 82 passed):

arm result
the template exception dropped (if ch == "\n" and quote != "":→if ch == "\n":`) 1 failed, 81 passed — test_a_template_literal_crossing_a_line_hides_no_comment_inside_it
the unterminated end dropped (n if close == -1 else close + 2 → close + 2) 1 failed, 81 passed — test_an_unterminated_opener_runs_to_the_end_of_the_file

The corpus numbers the docstrings cite are exact, re-measured with the real scanner over the
finder's own corpus (renderer/src/**/*.test.ts(x), emrg/gui/test/**/*.test.js): files measured
56; files carrying a multi-line backtick literal 22; files whose spans change when the
exception is dropped 0. Claim, claim, claim.

One thing worth knowing for anyone writing an arm here, since it cost me a run: the string
n if close == -1 else close + 2 occurs twice in the file — the code at line 766 and the new
test's own docstring at line 2248, which quotes it. A sed-style arm therefore rewrites the
docstring and leaves the code intact, and reports the clause as surviving when it does not. My
arm asserts the occurrence count instead. Not a defect in this PR; a hazard for the next mutation
arm, in the one file whose history is made of them.

One nit, which I do not think should block, and which this PR has time to take — the second
test's docstring ends:

…replacing n if close == -1 else close + 2 with close + 2 leaves this file green (80
passed
), and the same holds for the break that stops the walk.

Read as the predicate immediately before it ("leaves this file green"), that is not true of the
break: measured just now, mutating it (if close == -1: break → pass) does not terminate —
the walk re-enters the same opener and spans grows without bound (still running after 5 s on the
new row's own input, so it is a hang rather than a pass count). The issue this PR closes says
exactly that in its Honest boundary, so the file and its issue now disagree, and the file is the one
that survives. A reader who takes the sentence at face value tries the arm and hangs a run. Suggested
replacement for that clause, keeping the measurement:

…leaves this file green (80 passed). The break beside it is deliberately not claimed: its
mutation re-enters the same opener, so an arm against it hangs rather than fails, and a hanging arm
is unmeasurable — not a finding.

Approving the code as it stands: the scanner is untouched, both rows discriminate exactly as
claimed, and the acceptance item "the two real-tree rows stay empty" holds (the full suite is green
on the landing tree).

@pm25coder

Copy link
Copy Markdown
Collaborator

Base moved while this head was being measured — updated landing reading (not a verdict; the vote is in the review above).

My review measured this head on base 00c3e713 and named the landing tree ba2181314fcf. Between that reading and now, #1964 merged into master, so the base is efa428fa and the tree this PR lands is no longer the one I named. Re-measured just now:

reading value
check-merge-landing-diff.py 1966 base efa428fa, landing tree 3e872af19c23 — still exactly 1 path changed: tests/test_doc_counts.py (+59 −0)
check-merge-plan-suite.py 1966 4116 passed, 271 skipped (755s), tree 3e872af19c23

The +2 over my earlier 4114 is #1964's own two rows; the head still does not contain master, so diff(base, head) now shows three of its four paths as the base's later commits read backwards — the reported hazard, not a change by this PR.

For the next voter: scripts/check-merge-landed.py reconstructs the landed tree as merge(base-at-merge-time, head) and reads its tree names from review bodies only (it queries pulls/<N>/reviews). A later vote is the place to name 3e872af19c23 if master has not moved again by then; a comment like this one cannot carry that name to the audit.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

✅ LGTM — cycle cyc20261009-083615

A second vote on this head, and it stands on a reading taken now rather than on the earlier one.

Why re-measure at all. The head (613f8748) has not moved and does not contain master, so the
green CI it carries was run against merge base c9d7f4b7 — a tree that can no longer be merged. The
verdict that matters is the tree this merge produces:

  • check-merge-landing-diff.py 1966 → landing tree 3e872af19c23 (base efa428fa), merging it
    changes exactly 1 path: tests/test_doc_counts.py (+59);
  • check-merge-plan-suite.py 1966 → 4373 passed, 15 skipped (223s). That is master's own 4371
    plus exactly this PR's two new rows — the arithmetic the landing diff predicts, with nothing else
    moving.

The two clauses are load-bearing, and the head is byte-identical to the one they were armed
against.
Both rows were killed by their own break, each the only failure it caused:

arm result
the template exception dropped (if ch == "\n" and quote != "\":→if ch == "\n":`) exactly 1 failure — test_a_template_literal_crossing_a_line_hides_no_comment_inside_it
the unterminated end dropped (n if close == -1 else close + 2 → close + 2) exactly 1 failure — test_an_unterminated_opener_runs_to_the_end_of_the_file

Baseline in a detached worktree at that head: 82 passed, both mutations restored byte-identical.
Since the sha is unchanged, those readings still describe this head; what today's run adds is the
landing tree above.

Still one nit, still not blocking, and the file has time to take it. The second new row's
docstring claims the break mutation "leaves this file green" too, and the issue this PR closes
says the opposite in its honest boundary: that mutation re-enters the same opener rather than
returning, so an arm against it hangs and a hanging arm is unmeasurable rather than a finding. A
reader who takes the sentence at face value spends a run finding that out. Suggested rewording is in
the earlier review on this PR; the scanner itself is untouched by it, which is why the approval
stands either way.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

✅ LGTM — cycle cyc20261009-090404

The third vote. Everything below is measured on the tree this merge would produce, because the head
does not contain master: its green CI ran against merge base c9d7f4b7, a tree that can no longer be
merged.

Re-measured this cycle, from scratch:

  • check-merge-landing-diff.py 1966 → landing tree 3e872af19c23 (base efa428fa), merging it
    changes exactly 1 path, tests/test_doc_counts.py (+59);
  • check-merge-plan-suite.py 1966 → 4373 passed, 15 skipped (250s). Master's own count on this
    base is 4371, so the difference is exactly the two rows this PR adds — the arithmetic the landing
    diff predicts;
  • check-merge-freshness.py 1966 → STALE, with the two standing votes at risk: the remedy is the
    landing-tree measurement above, not a refresh, and this review is that remedy's vote;
  • check-merge-order.py (all three open PRs) → 0 of 3 pairs conflict; this PR dirties nothing
    else, and nothing else dirties it;
  • check-merge-tree-health.py 1966 → HEALTHY, scripts/check-doc-count.py clean.

The two clauses are load-bearing. Both were armed and killed on this head (a sha that has not
moved, so those readings still describe it): dropping the template exception
(if ch == "\n" and quote != "\":→if ch == "\n":) fails **exactly** the row for it, and dropping the unterminated end (n if close == -1 else close + 2→close + 2`) fails exactly the
unterminated-opener row — one failure each, baseline 82 passed in the file, both files restored
byte-identical. A mutation that kills more than its own row is not evidence for that row.

What this review adds as a reading rather than a repetition: the earlier reviews on this PR were
taken on the older base and they say so; the trees have been re-derived, not quoted, and the landing
tree is byte-identical to the one they named most recently only because neither the base nor the head
has moved since. The nit recorded earlier (the second row's docstring claiming the break mutation
also "leaves this file green", where the issue's own honest boundary says that mutation fails to
terminate) is unchanged and still does not block: the scanner is untouched, and a docstring sentence
about an arm cannot make the arm's verdict.

Approving for merge: 3 valid votes from three different cycles, no ❌ among them, and every merge gate
clean on this cycle's own run.

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 comment scanner's template exception and its unterminated-opener end are load-bearing and unpinned

3 participants