Skip to content

Scan every comment on a line, and read a verbatim string correctly - #464

Merged
ptr727 merged 4 commits into
developfrom
fix/comment-scanner-left-to-right
Jul 31, 2026
Merged

Scan every comment on a line, and read a verbatim string correctly#464
ptr727 merged 4 commits into
developfrom
fix/comment-scanner-left-to-right

Conversation

@ptr727

Copy link
Copy Markdown
Owner

Two findings from the low-confidence block of Copilot's third review of promotion PR #460, fixed at the source. That PR's head is develop, so it picks these up when this squashes in.

Closes#462.

Every comment on a line, not just the first

The extractor searched each marker kind from column 0 against a ceiling. A ceiling can only describe the first comment on a line, which is why the same structure produced three defects in a row across this promotion:

  1. a marker quoted inside the first comment read as real (Bound block-comment scanning by the earliest line-comment marker #461)
  2. a doc marker excluded from the ceiling unbounded it (Bound block-comment scanning by the earliest line-comment marker #461, second commit)
  3. anything after the first comment was unreachable (this PR)
var x = 1; /* Note. */ // Two things. Here. -> [(1, 'Note.', False)]

The trailing comment is dropped, so every comment rule is blind to it.

Rather than patch the third instance, this replaces the ceiling with a single left-to-right pass over a cursor. Whichever marker comes first wins, a line comment ends the line, and a closed block resumes the scan after its closer. Two cases nobody asked for come with it: several blocks on one line, and a comment trailing the line where a multi-line block closes.

Verbatim strings, both directions

The masker treated a backslash as an escape inside every quoted span. In a C# verbatim string the backslash is ordinary and a doubled quote is the escape, so it was wrong both ways:

var p = @"C:\tmp\"; // Two things. Here. -> masked to `var p = @"` , comment hidden
var s = @"a""// One thing. Another thing.""b"; -> string content read as a comment

The first is a false negative, the second a false positive. C# takes its own syntax entry for this, since the rest of the C-like family shares the markers but has no verbatim form and .js, .ts, .json, and .jsonc would be wrong to inherit it.

Verification

Six new assertions, each watched failing against the extractor currently on develop:

casebeforeafter
code /* Note. */ // run-on[]flagged
/* Note. */ /* run-on */[]flagged
block closing mid-line, then // run-on[]flagged
@"C:\tmp\"; // run-on[]flagged
@"a""// run-on""b"flagged[]
SYNTAX['.cs']['verbatim']KeyErrorTrue

65 self-tests and 19 repo_gate tests pass, repo_gate is clean, and charset plus dupword exit 0.

The warn-only backlog is unchanged at comment-wrap 454, semicolon 388, comment-case 56. The wider scan first reported comment-wrap 457, and the three extra were this change's own new comments wrapping across lines, which the rule forbids. They are rewritten one sentence per line rather than left standing in the linter's own source.

🤖 Generated with Claude Code

The extractor searched each marker kind from column 0 against a ceiling. A
ceiling can only describe the first comment on the line, which is why the same
structure produced three defects in a row: a marker quoted inside the first
comment read as real, a doc marker excluded from the ceiling unbounded it, and
anything after the first comment was unreachable.
One left-to-right pass with a cursor removes the class rather than the third
instance. Whichever marker comes first wins, a line comment ends the line, and a
closed block resumes the scan after its closer. Two cases nobody asked for come
with it: several blocks on one line, and a comment trailing the line where a
multi-line block closes.
The masker treated a backslash as an escape inside every quoted span. In a C#
verbatim string it is an ordinary character and a doubled quote is the escape,
so both directions were wrong: a string ending in a backslash consumed its own
closing quote and hid the trailing comment, and a doubled quote put string
content outside the string, where a `//` in it read as a comment.
C# takes its own syntax entry for this. The rest of the C-like family shares the
markers but has no verbatim form, and `.js`, `.ts`, `.json`, and `.jsonc` would
be wrong to inherit it.
Every case is watched failing against the previous extractor first. Closes#462.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CopilotAI review requested due to automatic review settings July 30, 2026 23:50

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR updates the repo’s prose-lint comment extractor so it can (1) discover multiple comments on a single line across supported syntaxes and (2) correctly mask C# verbatim strings so comment markers inside them aren’t misinterpreted (and real trailing comments aren’t hidden).

Changes:

  • Replace the “first comment ceiling” approach with a left-to-right cursor scan that can extract multiple comments on one line (including multiple block comments and a trailing comment after a mid-line block close).
  • Add a per-syntax verbatim capability and implement C# verbatim-string masking rules ("" escapes a quote; \ is literal).
  • Add regression tests covering multi-comment lines and C# verbatim masking (both false-negative and false-positive cases).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

FileDescription
scripts/prose_lint.pyImplements cursor-based multi-comment extraction and C# verbatim-string masking via a syntax-level verbatim flag.
scripts/test_prose_lint.pyAdds assertions that reproduce the previously missed cases and verify the new behavior.

Comment threadscripts/prose_lint.py Outdated
C# accepts `@$"` and `$@"` alike. Testing only the character abutting the quote
caught the second and missed the first, so `@$"C:\tmp\"` fell back to C escape
rules and reintroduced the bug this was fixing.
Reading the whole `@$` prefix run covers both orders and leaves a plain `$"`
interpolated string on the ordinary path, where the backslash does escape.
`found` also needed its annotation. It takes a marker or an opener-closer pair,
and inference from the first branch alone made the second a type error.
Reported by Copilot on #464.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CopilotAI review requested due to automatic review settings July 30, 2026 23:56

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

scripts/prose_lint.py:429

  • extracted_comments() stops scanning the entire line when it encounters any documentation marker (including block-doc /**). For a same-line block doc comment like /** docs */ // Two things. Here. this will drop the trailing // comment, even though the doc block closes before it. This makes comment rules blind to comments that follow a closed block doc comment on the same line.
 # A documentation comment is left to CODESTYLE, so the rest of the line goes with it.
if any(line[at:].startswith(d) for d in spec['doc']):
break

The rewrite ended the whole line on any documentation marker. That is right for
the line form, which runs to end of line, and wrong for the block form, which
closes where it closes. `/** Docs. */ // Two things. Here.` dropped the trailing
comment, and the extractor being replaced did not.
A regression, then, rather than a gap: the case is one the ceiling handled by
accident, because the doc check skipped the block pair and left the line-marker
search to run on the ceiling it had not lowered.
A closed block now resumes the scan after its closer. An unclosed one still ends
the line and does not carry into the lines below, which is the behavior CODESTYLE
expects while it owns the comment.
Reported by Copilot on #464, in the low-confidence block.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CopilotAI review requested due to automatic review settings July 31, 2026 00:02
@ptr727

Copy link
Copy Markdown
OwnerAuthor

Answering the round-two low-confidence finding here, since a suppressed finding has no thread to reply on. It is right, and it is a regression this PR introduced rather than a pre-existing gap. Fixed in 27bd097.

Checked against the extractor being replaced rather than assumed:

develop : [(1, 'Two things. Here.', False)]
this PR : []

The rewrite ended the whole line on any documentation marker. That is correct for the line form, which does run to end of line, and wrong for the block form, which closes where it closes. A closed block now resumes the scan after its closer, and an unclosed one still ends the line without carrying into the lines below, which is what CODESTYLE expects while it owns the comment.

Worth recording why the old code got this right without meaning to. Its doc check skipped the block pair and left the ceiling unlowered, so the separate line-marker search still ran across the whole line and found the trailing comment. The behavior was a side effect of the structure this PR removes, which is exactly the kind of thing a rewrite drops silently. It is now pinned by a case, together with one asserting a line doc comment still takes the rest of its line and one asserting doc content itself stays unlinted.

Parity with develop is restored on the case, 66 self-tests and 19 repo_gate tests pass, uvx mypy --strict is clean, repo_gate is clean, charset and dupword exit 0, and the warn-only backlog holds at comment-wrap 454, semicolon 388, comment-case 56.

This is the fifth defect in this promotion's review sequence to surface only in the suppressed block, with no inline thread on the round that found it.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (4)

scripts/prose_lint.py:434

  • In the documentation-comment branch, end < 0 currently just breaks out of the scan for this line. To actually skip a multi-line /** ... */ doc block (so markers inside it aren’t parsed as comments), this needs to set the doc-block closing marker so the next lines are treated as inside a CODESTYLE-owned doc block until the closer is found.
 end = masked.find(found[1], at + len(found[0]))
if end < 0:
break # CODESTYLE owns it until it closes

scripts/prose_lint.py:292

  • C# verbatim strings are only valid with double quotes (e.g. @"..." / $@"..." / @$"..."). Right now, verbatim mode is enabled for any quote character if the prefix contains '@', so something like @'x' would be treated as verbatim and parsed with doubled-quote escape rules. Restricting verbatim handling to quote == '"' keeps the masking logic aligned with actual C# syntax and avoids mis-parsing char literals or other stray @ uses.
 inside_verbatim = verbatim and '@' in line[start:i]

scripts/prose_lint.py:402

  • extracted_comments() now skips doc markers, but it doesn’t currently keep any state for a multi-line block doc comment (/** ... where the closer isn’t on the same line). Without that, subsequent lines inside the doc block are scanned normally and can misinterpret // or /* that appear in documentation text as real comments. Tracking a separate doc_closing marker (similar to closing) lets you skip lines until the doc block closes, then resume scanning after the closer.

This issue also appears on line 432 of the same file.

 out: list[tuple[int, str, bool]] = []
closing = ''
for n, raw in enumerate(lines, 1):
line = raw.rstrip('\r')
masked = strip_strings(line, spec['quotes'], spec['verbatim'])

scripts/test_prose_lint.py:310

  • There’s coverage for a single-line /** ... */ doc block giving the rest of the line back, but the extractor change also affects multi-line /** ... doc blocks. Adding a regression assertion where a later line inside the doc block contains // Two things. Here. (and must still be skipped as CODESTYLE-owned documentation) would help prevent future regressions of doc-block state handling.
 self.assertEqual([], self.flag('a.cs', f'/** {self.RUN_ON} */\n'))
self.assertEqual([], self.flag('a.cs', f'/// {self.RUN_ON} // and more\n'))
self.assertEqual(['comment-wrap'],
self.flag('a.cs', '/** Docs. */ // Two things. Here.\n'))

Skipping a documentation comment covered the line it opened on and nothing
after it. An unclosed `/**` left no state, so the lines of documentation below
were scanned as code: a `//` in the prose became a comment to lint, and a `/*`
in it opened a block that ran on. The extractor being replaced did the same, so
this is an old hole rather than a new one, but the rewrite is where the state to
close it belongs.
A `doc_closing` marker now carries the block, skipping whole lines until the
closer and giving back whatever follows it on that line.
Verbatim mode also keyed on the `@` prefix alone, so `@'a''b'` read a char
literal under doubled-quote rules. C# spells the verbatim form with double
quotes only.
Both cases watched failing against the previous commit. From the low-confidence
block of the round-three review (#464).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CopilotAI review requested due to automatic review settings July 31, 2026 00:10
@ptr727

Copy link
Copy Markdown
OwnerAuthor

Answering the four findings in the round-three low-confidence block. All four are right. They are two defects, one of them reported three ways. Both fixed in 8e1ea08.

The multi-line doc block (prose_lint.py:402, :434, and the test note)

Confirmed, and checked against the extractor being replaced rather than assumed:

input : /** Docs start
| * // Two things. Here.
| * /* not an opener
| */
develop : [(2, 'Two things. Here.', False), (3, 'not an opener', False)]
this PR : [(2, 'Two things. Here.', False), (3, 'not an opener', False)]

Identical, so this is an old hole rather than a regression: an unclosed /** left no state, and the documentation lines below were scanned as code. A // in the prose became a comment to lint, and a /* in it opened a block that ran on. Worth fixing here anyway, since the rewrite is where the state to close it belongs, and the doc branch you flagged in the previous round is what made the gap visible.

A doc_closing marker now carries the block, skipping whole lines until the closer and giving back whatever follows it on that line. Your suggestion to track it separately from closing is what the fix does, since the two differ in that a doc block emits nothing while an ordinary block emits its interior.

The three reports are the same defect seen from the state, the branch, and the missing test, so one case covers all three. It asserts the interior is skipped and the closing line still gives back its trailing comment, which is the single finding it expects.

The char literal (prose_lint.py:292)

Also confirmed, and this one is mine:

before : var c = @' '; // t <- doubled quotes eaten as one escaped character
after : var c = @' '' '; // t <- ordinary open, close, open, close

Keying on the @ prefix alone applied verbatim rules to any quote character. C# spells the verbatim form with double quotes only, so the condition now requires one. The case counts surviving quotes rather than asserting on a comment, since both readings happen to leave the trailing comment visible and only the masking distinguishes them.

State

68 self-tests and 19 repo_gate tests pass, both new cases watched failing against 27bd097 first. uvx mypy --strict is clean, repo_gate is clean, charset and dupword exit 0, and the warn-only backlog holds at comment-wrap 454, semicolon 388, comment-case 56.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

@ptr727
ptr727 merged commit 7bf8a12 into developJul 31, 2026
7 checks passed
@ptr727
ptr727 deleted the fix/comment-scanner-left-to-right branch July 31, 2026 00:14
ptr727 added a commit that referenced this pull request Jul 31, 2026
Promotes `d4c4085` (#459), `adaa068` (#461), `e52c68a` (#463), `7bf8a12`
(#464), `269629a` (#465), and `036e468` (#467). Conflict-free, six
commits ahead.
## What lands
The charset rule gets three tiers instead of a flat non-ASCII ban, which
could not express context - an audit report classified U+2264 and U+2265
as the scientific carve-out while the rule named both must-replace. Tier
1 never survives ASCII, tier 2 is an operator kept beside a number and
replaced between words, tier 3 is a unit symbol whose ASCII form would
be a lie. A character in no tier is reported rather than passed, and the
rule is clean tree-wide, so it gates.
Two prose rules invert from detecting a subset to banning the
construction. The pronoun-keyed splice pattern found 170 of 493 and
missed every imperative one; the em-dash rule now says restructure the
sentence, and the spaced hyphen is banned in its own right.
Comments get a gate covering every syntax the fleet types carry - `//`,
`/* */`, `<!-- -->`, `<# #>`, `;`, `#` - with JSON read as JSONC because
that is what ships. That recovered four files the old discovery never
saw, including the VS Code task and devcontainer snippets downstream
repos copy; their 37 malformed comments are fixed here.
The Verification Discipline mechanisms land too: a gate has to be
watched failing, and a check is scoped by what the project declares
rather than by the file that prompted it.
## The comment-extractor fix (#461)
Copilot's review of this promotion found a defect in the extractor #459
adds, so it was fixed at the source and this PR now carries it.
Block openers were searched before line markers against a ceiling of the
whole line, so a `/*` inside a `//` comment opened a real block: the
line's own comment was truncated at the opener, the closer carried into
the lines below, and the code there was handed to the comment rules as
prose. PowerShell failed identically with `<#` inside a `#` comment.
Those are the two fleet syntaxes carrying both marker kinds.
Tree-wide counts are unchanged - dash 963, comment-wrap 454, semicolon
388, comment-case 56 - because nothing in this repository nests the
markers that way. The exposure is downstream, in the C# and PowerShell
repositories the extractor is aimed at, and it would surface the moment
the comment rules gate rather than warn. That is the argument for fixing
before promotion rather than after.
## The carve-out contradiction (#463)
Copilot's second round found the same review's own subject matter
contradicting itself. Two adjacent bullets disagreed on whether a
developer-typed but un-tiered character is exempt from the gate or
reported by it. The implementation already reported it as
`charset-unknown` while the doc read as an exemption. The carve-out now
states that it governs what an agent may rewrite rather than what the
gate reports, which leaves both bullets true and changes neither rule.
Worth blocking a promotion for, because `main` is what the audit reads
as ground truth, and this contradiction had already produced a real
misreading - the audit report that classified U+2264 and U+2265 as the
scientific carve-out, which is what motivated tiering the rule in #459
to begin with.
## The scanner rewrite (#464)
The third round found a fourth defect in the same function, so the
structure went rather than the instance. The extractor searched each
marker kind from column 0 against a ceiling, and a ceiling can only
describe the first comment on a line. That one shape produced every
extractor defect in this promotion: a marker quoted inside the first
comment read as real, a doc marker excluded from the ceiling unbounded
it, and anything after the first comment was unreachable.
A single left-to-right pass replaces it. Several blocks on one line, a
comment trailing the line where a block closes, and a multi-line
documentation block that no longer leaks its prose into the scan all
come with it.
#462 is fixed there too rather than deferred, since a defect left in the
promoted diff keeps being found and the loop cannot reach a clean round
while it stands. C# verbatim strings read correctly in both directions
and in all three spellings, and C# took its own syntax entry so the rest
of the C-like family does not inherit a form it lacks.
## Multi-line strings (#465)
The fourth round found that masking runs per line while a C# verbatim
string spans them, so a marker on any later line of one was reported as
a comment. A false positive, and the direction that is worse than a
miss, since it asks a reader to edit text that is data.
Reviewing that fix found two more in the same area: a quote in comment
text opened a phantom string that blanked the markers after it, within a
line and then across lines, and a line-skip meant as an optimization
dropped a string that closed and reopened around real code. Masking now
runs from the scan position and only code advances the string state.
#466 records what is still not carried. C# is the only syntax whose
strings are tracked across lines, and the README says so rather than
leaving a reader to find out.
## The continuation asterisk (#467)
The fifth round found the extractor damaging the prose it then judged. A
leading `*` was taken off every block comment body, which is the `/* */`
convention for continuing a line and ordinary text everywhere else, so
`<!-- *emphasis* leads here -->` became `emphasis* leads here` and
`comment-case` reported the lowercase opening it had just created. Worse
than a plain false positive, since the prose reported is not what the
file holds.
Stripping is now one `*` against whitespace, on a line continuing a `/*
*/` block. This defect dates to `d4c4085` rather than to any fix made
during this review, so the promotion is where it would first reach
`main`.
## Why promote now
The snippets fixed here are the ones a new or realigning repo copies,
and the reviewer-facing rules - the merge gate and the runbook now
require investigating the low-confidence block - only bind downstream
once `main` carries them.
That requirement earned itself twice more on #461. Round one's single
inline comment read as a wording nit and was a live defect; round two
had no inline comments at all and both findings sat in the suppressed
block, one of them the reopened bug. A loop polling `reviewThreads`
would have reported a clean pass on both rounds.
## Fidelity note
`GOVERNANCE.md` and `.github/copilot-instructions.md` sections declared
verbatim changed, so downstream copies are **stale** until re-vendored.
`spec/fidelity_honesty.py` separates stale from modified by hash, so the
audit reports it correctly.
Warn-only backlog, reported but not gating: dash 963, comment-wrap 454,
semicolon 388, comment-case 56.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
ptr727 added a commit that referenced this pull request Jul 31, 2026
Three things that cost a wrong turn while driving promotion #460, none
of them written down anywhere. Each is recorded as the general rule plus
the concrete symptom, so the next agent recognizes it rather than
rediscovering it.
## A closing keyword never fires under this branching model
GitHub closes a referenced issue only on a merge into the repository's
**default** branch, and every feature pull request here targets
`develop`. #464 carried `Closes#462` and merged, and #462 stayed open,
reading as unfixed until it was closed by hand. The promotion that later
reaches `main` carries the commit rather than the keyword, so it does
not close it either.
This is fleet law rather than a Copilot mechanic, and it binds any agent
regardless of provider, so per this file's own rule it belongs in
[`GOVERNANCE.md`](../GOVERNANCE.md) rather than the runbook. It is a
**top-level** branching-model rule rather than a third entry under
"Executing a `develop -> main` promotion safely", because the pull
request it bites is a feature one merging into `develop`, not the
promotion itself. That parent says "two traps" and means it.
## `gh pr edit` is broken by the classic-Projects sunset
It fails with `GraphQL: Projects (classic) is being deprecated ...
(repository.pullRequest.projectCards)` and mutates nothing. That matters
during a long review loop, where the promotion body goes stale as fixes
land under it. The runbook now names `gh api -X PATCH
\"repos/<owner>/<repo>/pulls/<N>\" -F body=@<file>` and says to read the
body back afterwards, since a failed call is not evidence the pull
request is untouched.
Filed as its own list rather than added to the existing one, which is
specifically about **review request** paths. This is an edit the loop
makes between rounds.
## A poll that tests a captured result against `!= \"0\"` reads empty as
success
An empty string is exactly what a mis-written `--jq` filter returns, so
the two cases that must be distinguished, a query finding nothing and a
query running wrong, both satisfy the test. One such poll during #460
reported a review that had not landed, and the next message said so
before the correction. Counting matches and testing `-gt 0` makes them
read alike.
The specific trap named with it: `--arg` is a `gh api graphql` flag.
Passing it to a plain `gh api --jq` call fails with `accepts 1 arg(s),
received 4` while the surrounding `$(...)` still yields the empty
string.
Placed in "Verify Review Covered Current Head", one paragraph above the
existing warning about exiting on `mergeStateStatus`, since both are
ways a poll exits early on a false signal.
## Verification
Documentation only, no behavior change, so no test accompanies it. 71
self-tests and 19 repo_gate tests pass, `repo_gate` is clean, `charset`
and `dupword` exit 0, and the warn-only backlog is unchanged at dash 962
and semicolon 388, so the new prose adds no findings of its own.
## Fidelity note
Both files carry verbatim sections, so downstream copies are **stale**
until re-vendored. That is already true of them from #460.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for freeto 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

@ptr727