From f6ec59e05a14644979a705112a4ee8c4d3b455f0 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 21 Aug 2026 21:54:08 +0000 Subject: [PATCH 1/3] check_delimiters.py: extend cross-line paren tracking to rem comments (Item 61) rem lines were previously fully opaque to the paren-tracking checker (skipped via `continue`), even though cmd.exe's own block-boundary parser counts '('/ ')' characters inside rem text exactly like it does inside echo text -- the same hazard class that broke 6 CI lanes once already (PR #408) and a rem-text sibling a second time (PR #445, Item 52). Routes rem lines through the same character scan and cross-line-close check echo lines already had (StackItem's bool is_echo_open generalized to Optional[str] prose_kind). Making this work correctly against the real run_setup.bat required two more general (not rem-specific) fixes, found only by running the extended checker against it: cmd.exe's own '^' escape character in front of a bracket was not recognized (so the file's own established '^(' / '^)' hazard-defusing convention was itself flagged), and a bare apostrophe was treated as a string-quote delimiter on .bat/.cmd lines with no such concept in real cmd.exe, corrupting cross-line tracking for any rem prose containing an ordinary contraction or possessive. Running the fixed checker against run_setup.bat surfaces 26 genuine, previously-invisible cross-line rem pairs already in the file (not audited here -- flagged as the concrete next follow-up in CLAUDE.md's Item 61 entry, per this repo's one-slice-at-a-time discipline for run_setup.bat). One existing line's own metacharacter listing ("(&, |, ^)") was reworded to resolve the sole false positive the new caret-escape heuristic itself produced, distinguishing a literal example caret from an escape prefix. check_delimiters.py is advisory-only (not wired into any CI gate), so this does not affect the GitHub Actions pipeline. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_017SQ1rvJxDbE71pTXJ4QvLV --- CLAUDE.md | 116 +++++++++++++++--------- docs/agent-lessons-learned.md | 24 +++-- run_setup.bat | 2 +- tests/test_check_delimiters_import.py | 101 +++++++++++++++++++++ tools/check_delimiters.py | 123 +++++++++++++++++++++----- 5 files changed, 292 insertions(+), 74 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 444dfa76..750be170 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1038,47 +1038,75 @@ way (no live Windows execution available here), that is noted explicitly rather OLD rules before running the bootstrapper, asserting the file ends up with `-text` and no leftover `eol=crlf` line for `*.bat`/`*.cmd`. -- **Item 61: `check_delimiters.py` does not catch a cross-line `(`/`)` pair inside `rem` comment - text, even though cmd.exe's own parser is just as vulnerable to it as it is for `echo` text -- - a real gap that caused a genuine CI-breaking regression across all 8 lanes (PR #445, Item 52's - own fix).** `docs/agent-lessons-learned.md`'s "A literal `(`/`)` inside `echo` text..." entry - documents both the original 2026-07 echo-text incident AND this newly-confirmed `rem`-text - sibling -- read that entry for the full mechanism and incident trace before starting this item. - - **Root cause, already diagnosed**: `check_delimiters.py`'s `.bat`/`.cmd` handling treats a `rem` - line as fully opaque (`if upper.startswith("REM ") or ...: continue`, skipping it from - paren-scanning entirely) instead of scanning its characters the way the `echo`-line path does - (`is_bat_echo_line`, tracked via `is_echo_open` on the bracket stack, flagging a `(` that opens - on an echo line already nested inside a real block and closes on a LATER line). Real cmd.exe - does not distinguish `rem` from `echo` for this purpose -- its block-closing search is a raw - character scan across the whole block's text regardless of which command a given line belongs - to -- so `rem` comments are exactly as exposed to this hazard as `echo` text, but the checker - only defends the `echo` case today. - - **High-level fix**: extend the existing `is_echo_open`/bracket-stack machinery to also apply to - `rem` lines -- drop the current `continue`-and-skip shortcut for REM lines, route them through - the same character scan `echo` lines already get, and reuse the identical "already nested inside - an open bracket, closes on a different line" flagging logic (scoped the same way, so a harmless - top-level `rem` header block with no enclosing `if`/`for` doesn't false-positive -- this repo's - own file header, `run_setup.bat` lines 1-40ish, has several legitimately-balanced-per-line or - intentionally `^`-escaped parens that must stay clean). - - **Scope note, NOT yet done as part of the Item 52 fix that surfaced this**: a full audit of - every PRE-EXISTING cross-line `rem`-comment paren pair already in `run_setup.bat` (there are - many, scattered throughout the file's ~5300 lines) was explicitly NOT performed -- Item 52's own - fix only reworded the ONE `rem` block it had just introduced and broken. Whether any of the - pre-existing ones are ALSO genuinely hazardous (nested inside a real open block, not just a - top-level header) is unknown and unverified; the checker fix above would surface them - automatically once implemented -- do not assume the file is currently clean of other latent - instances of this same bug just because CI has been green so far (an existing hazard only - manifests when the SPECIFIC surrounding code happens to also be reached/reparsed in a way that - exposes it, exactly as this one sat undetected until Item 52 added new code near it). - - **Coverage gap to close in the same slice**: `tests/test_check_delimiters_import.py`'s existing - `test_paren_*` cases cover the `echo`-line hazard; add an analogous `rem`-line case (a `rem` - block whose `(` opens on one line and matching `)` closes on a later line, nested inside a real - `if`/`for` block) proving the extended checker catches it, plus a negative case (a top-level - `rem` header block with no enclosing bracket) proving it doesn't false-positive. +- **Item 61 (checker fix landed; audit of newly-surfaced findings remains open): `check_delimiters.py` + now catches a cross-line `(`/`)` pair inside `rem` comment text, not just `echo` text -- but + turning that on surfaced 26 genuine, previously-invisible findings already in `run_setup.bat` + that still need their own audit before `check_delimiters.py run_setup.bat` reports clean again.** + `docs/agent-lessons-learned.md`'s "A literal `(`/`)` inside `echo` text..." entry documents the + original 2026-07 echo-text incident and its `rem`-text sibling (PR #445, Item 52) that motivated + this fix -- read that entry for the full mechanism and incident trace. + + **Fix shipped**: `check_delimiters.py`'s `.bat`/`.cmd` handling no longer treats a `rem` line as + fully opaque -- it now routes through the same character scan and `prose_kind`-tracked bracket + stack `echo` lines already used (the field was generalized from a bool `is_echo_open` to a + `Optional[str] prose_kind`, so the same cross-line-close check flags either kind and the error + message names which one). Scoped identically to the echo case: a paren opened on a rem/echo line + is only flagged if it was ALREADY nested inside a real open bracket when pushed, so a harmless + top-level rem header block with no enclosing `if`/`for` does not false-positive. + + **Two additional, necessary correctness fixes found only by running the extended checker against + the real `run_setup.bat` -- neither was anticipated by the original fix description above, and + either one alone made the rem-line extension actively counterproductive (68+ and later 82 mostly- + bogus findings on first attempt, cascading from a handful of root causes) rather than useful:** + 1. **cmd.exe's own `^` escape character was not recognized at all.** `^(` / `^)` is this file's + own established, already-documented convention for defusing this exact hazard (see the error + message's own suggested fix) -- `run_setup.bat`'s file-header rem block (lines ~43-58) uses it + extensively ("Windows ^(CRLF^)", etc.). Without recognizing it, the checker flagged the very + construct that fixes the hazard, and every mistracked bracket corrupted the stack for + everything scanned afterward. Fixed: a bracket character on any `.bat`/`.cmd` line preceded by + an ODD count of `^` (via the pre-existing `count_preceding` helper, mirroring how string-escape + already used it) is now treated as a literal, non-special character. + 2. **A bare apostrophe (`'`) was treated as a string-quote delimiter on `.bat`/`.cmd` lines, and a + standalone `"` in rem/echo PROSE could open a persistent, incorrectly cross-line "string."** + cmd.exe has no single-quote-string concept at all, and rem/echo text (unlike real code) has no + "quoted argument" concept either -- an ordinary contraction/possessive ("doesn't", "user's", + "cmd.exe's") or a standalone `"` describing the quote character itself (`'...a literal " would + close the quote.'`, a real line in `run_setup.bat`) would silently swallow every later + character -- including real parens on subsequent lines -- as fake "string content" until some + unrelated, later quote happened to "close" it. Fixed: `'` is now always inert on `.bat`/`.cmd` + lines; `"` is inert specifically on rem/echo prose lines (still fully meaningful on a real, + non-prose `.bat`/`.cmd` code line, e.g. `set "VAR=..."`). + + Both fixes are general (not `rem`-specific) and apply equally to `echo` lines, closing the + identical latent gap there too -- it just never manifested for `echo` because this codebase's own + echo output text is comparatively sparse and formal (rarely uses contractions or `^`-escaping) + compared to the much higher volume of informal, dev-facing `rem` prose. + + **Coverage added**: `tests/test_check_delimiters_import.py` gained 4 new tests -- the positive + (`rem` cross-line pair nested inside a real block, flagged) and negative (top-level `rem` header, + not flagged) cases originally scoped for this item, plus two regression tests for the two + correctness fixes above (a `^`-escaped rem-line pair is not flagged; an apostrophe/standalone-quote + rem line does not corrupt paren tracking). All 13 tests in that file, and the full pytest suite, + pass. + + **Remaining scope, concretely bounded (NOT closed by this fix -- this is the actual follow-up + work, not a hypothetical): 26 genuine, previously-invisible findings are now surfaced in + `run_setup.bat` itself** (run `python tools/check_delimiters.py run_setup.bat` for the current, + authoritative list -- do not copy the list here, since any future edit to the file shifts every + line number). Each is a real cross-line `(`/`)` pair inside `rem` prose, nested inside a real + `if`/`for` block, that predates this fix and was invisible to the checker until now (an existing + hazard only manifests when the specific surrounding code happens to also be reached/reparsed in a + way that exposes it -- do not assume `run_setup.bat` was clean of this bug just because CI has + been green; `check_delimiters.py` is advisory-only, not wired into any `.github/workflows/*.yml` + gate, so this has not broken CI, only the local/agent-facing sanity-sweep discipline). **Whoever + picks this up next**: audit each one individually (most are very likely simple, low-risk prose + rewording -- see the single `(^, &, or |)` reordering this same PR applied to the ONE genuine + false-positive the caret-escape heuristic itself produced, at the original `(&, |, ^)` listing -- + as the template fix shape), a few at a time rather than all 26 in one sweep, per this repo's own + "EXTREME CAUTION, one slice at a time" convention for anything touching `run_setup.bat`. Live- + cmd.exe verification of a representative sample (not necessarily all 26) would raise confidence + that "nested cross-line rem pair" is a real hazard class here and not merely theoretical, per this + repo's own standing distrust of pure static reasoning for this hazard class (see below). **Scope WIDENED, same PR (#445), via a second real CI incident on the SAME code block: a SAME-LINE, self-contained, balanced `(`/`)` pair -- not just cross-line pairs -- can ALSO corrupt @@ -1098,9 +1126,9 @@ way (no live Windows execution available here), that is noted explicitly rather `test_paren_pair_on_redirected_echo_line_deeply_nested_is_a_known_false_negative` for a regression fixture documenting the checker's current false-negative on this exact shape. - **Revised item scope**: the high-level fix above (extend `is_echo_open`-style tracking to `rem` - lines) is necessary but NOT sufficient on its own -- it still only catches CROSS-line pairs. - Whoever picks up this item should ALSO investigate whether extending the same-line-pair + **Revised item scope, still open**: the cross-line fix above was necessary but is NOT sufficient + on its own -- it still only catches CROSS-line pairs. Whoever picks up this item next should ALSO + investigate whether extending the same-line-pair "always safe" assumption is correct at all once genuinely nested (vs. top-level), and if not, design a check for that case too (e.g. flag ANY `(`/`)` pair -- same-line or cross-line -- found inside `echo`/`rem` text that is already nested inside a real open bracket, not just cross-line diff --git a/docs/agent-lessons-learned.md b/docs/agent-lessons-learned.md index 8c331966..8ad8583b 100644 --- a/docs/agent-lessons-learned.md +++ b/docs/agent-lessons-learned.md @@ -475,14 +475,22 @@ list; the recurring traps that have actually bitten us: relative to `echo` text: it will report a clean file even when a `rem` block contains exactly this hazard. **Fix applied to the specific instance**: reworded the comment to avoid the literal `(`/`)` characters entirely (` -- ` in place of the parenthetical), per the same rule as the echo - case. **The general gap in `check_delimiters.py` itself is NOT yet closed** -- extending its - existing `is_echo_open`-style tracking to also cover `rem` lines (dropping the current - `continue`-and-skip shortcut, replacing it with the same character-scan-plus-cross-line-check the - echo path already has) would close this class of bug the same way the echo fix did in 2026-07 -- - flagged as a candidate follow-up item, not implemented as part of this fix (this fix only needed - to unblock the one broken instance, not audit or re-armor every pre-existing `rem` block in the - file -- a large, separate undertaking on a ~5300-line file with many other cross-line `rem` - parens whose actual nesting-safety was not individually re-verified here). + case. **The general gap in `check_delimiters.py` itself is now closed (CLAUDE.md Item 61)** -- + `StackItem`'s bool `is_echo_open` field was generalized to `Optional[str] prose_kind`, and `rem` + lines now route through the same character scan/cross-line-close check `echo` lines already had + (the `REM `/`::` `continue`-and-skip shortcut now only applies to `::`). Reusing that mechanism + for `rem` prose required two ADDITIONAL, general (not `rem`-specific) fixes, found only by running + the extended checker against the real `run_setup.bat`: (1) a bracket preceded by an odd count of + `^` (this file's own established escape convention for defusing this exact hazard, e.g. `^(CRLF^)` + in its own header) is now treated as literal, not a real delimiter; (2) a bare `'` is now always + inert on `.bat`/`.cmd` lines (cmd.exe has no single-quote-string concept at all), and `"` is inert + specifically on rem/echo PROSE lines (an ordinary contraction/possessive, or a standalone `"` + describing the quote character itself, was previously opening a persistent, incorrectly + cross-line "string" that swallowed later real parens as fake string content). See CLAUDE.md's + Item 61 entry for the full fix trace, the 4 new regression tests, and the still-open follow-up: + running the fixed checker against `run_setup.bat` surfaced 26 genuine, previously-invisible + cross-line `rem` pairs already in the file that need their own audit (not attempted in this same + slice, per this repo's own one-slice-at-a-time discipline for anything touching `run_setup.bat`). **The rem-comment fix above did NOT fully resolve the regression -- a SECOND, independent paren hazard in the SAME code block was found only via a second round of live CI evidence, after the diff --git a/run_setup.bat b/run_setup.bat index 0273d906..be7e0e4c 100644 --- a/run_setup.bat +++ b/run_setup.bat @@ -4455,7 +4455,7 @@ if %HP_DLL_ITER% GEQ 3 ( goto :dll_bundle_recover_done ) rem Append via file content (type), never via %VAR% expansion on an echo/set line -- -rem a DLL basename can legally contain a space or a cmd.exe metacharacter (&, |, ^), +rem a DLL basename can legally contain a space or a cmd.exe metacharacter (^, &, or |), rem which would corrupt or split the command line if routed through argv/echo text. type "~next_dll.txt">>"~dll_bundle_tried.txt" echo.>>"~dll_bundle_tried.txt" diff --git a/tests/test_check_delimiters_import.py b/tests/test_check_delimiters_import.py index e2875e93..5c42e86a 100644 --- a/tests/test_check_delimiters_import.py +++ b/tests/test_check_delimiters_import.py @@ -189,3 +189,104 @@ def test_paren_pair_on_redirected_echo_line_deeply_nested_is_a_known_false_negat # instead of loosening it. assert result == 0 assert "No delimiter issues found." in captured.out + + +# derived requirement: these tests close the cross-line half of CLAUDE.md Item 61 -- +# a "rem" comment was previously fully opaque to check_delimiters.py (skipped from +# paren-scanning entirely, unlike "echo" lines), so it never caught the identical +# PR #408 hazard class when it hit a "rem" block instead of an "echo" one, as it +# genuinely did in PR #445 (see docs/agent-lessons-learned.md's "rem needs a space +# after it" entry's sibling incident). The fix routes rem lines through the same +# character scan + is_echo_open-style stack tracking echo lines already get. +def test_paren_split_across_rem_lines_inside_block_is_flagged(tmp_path, capsys): + bat = tmp_path / "sample.bat" + bat.write_text( + "@echo off\r\n" + "if defined HP_NO_INTERPRETER (\r\n" + " rem method (uv, conda, a fresh download, a\r\n" + " rem local virtual environment) failed -- usually\r\n" + " exit /b 0\r\n" + ")\r\n" + "echo done\r\n", + encoding="ascii", + ) + result = check_delimiters.main([str(bat)]) + captured = capsys.readouterr() + + assert result == 1 + assert "does not close until line" in captured.out + assert "counts parens in rem text too" in captured.out + + +def test_paren_split_across_rem_lines_at_top_level_is_not_flagged(tmp_path, capsys): + # Same textual pattern as above, but with no enclosing if/for block -- a real + # instance of this shape exists in run_setup.bat's own file header (a top-level + # "rem" block, no enclosing bracket) and must not false-positive, mirroring the + # existing top-level-echo negative case above. + bat = tmp_path / "sample.bat" + bat.write_text( + "@echo off\r\n" + "rem and it exited with an error just now (see the status line\r\n" + "rem above) -- so we cannot tell what happened.\r\n" + "exit /b 0\r\n", + encoding="ascii", + ) + result = check_delimiters.main([str(bat)]) + captured = capsys.readouterr() + + assert result == 0 + assert "No delimiter issues found." in captured.out + + +# derived requirement: two real, necessary correctness bugs found while implementing +# the fix above, both discovered only by running the extended checker against the +# real run_setup.bat (not by reasoning about the fixtures alone) -- each one alone +# was enough to make the rem-line extension actively counterproductive (flagging or +# corrupting far more than it fixed), since real "rem" prose in this heavily- +# documented codebase routinely contains both patterns. +def test_caret_escaped_paren_on_rem_line_is_not_flagged(tmp_path, capsys): + # cmd.exe's own escape character ('^') in front of a bracket makes it a literal + # character there, not a real block delimiter -- and '^(' / '^)' is this repo's + # own established convention for defusing exactly this hazard (see the error + # message's own suggested fix, and run_setup.bat's real file-header rem block, + # which uses this pattern extensively). Without this check, the very construct + # that FIXES the hazard was itself flagged as if it were the hazard. + bat = tmp_path / "sample.bat" + bat.write_text( + "@echo off\r\n" + "if defined HP_NO_INTERPRETER (\r\n" + " rem this file's line endings are Windows ^(CRLF^) by construction\r\n" + " rem and enforced elsewhere ^(see the docs^), but a stale copy can differ\r\n" + " exit /b 0\r\n" + ")\r\n", + encoding="ascii", + ) + result = check_delimiters.main([str(bat)]) + captured = capsys.readouterr() + + assert result == 0 + assert "No delimiter issues found." in captured.out + + +def test_apostrophe_and_standalone_quote_in_rem_text_do_not_corrupt_paren_tracking(tmp_path, capsys): + # cmd.exe has no concept of a single-quote string delimiter at all, and rem/echo + # PROSE text (unlike real code) has no "quoted argument" concept either -- so an + # ordinary contraction/possessive ("doesn't", "user's") or a standalone '"' + # describing the quote character itself must never open a persistent "string" + # that swallows later, unrelated characters (including real parens) until some + # later, unrelated quote happens to "close" it. Confirmed directly against real + # run_setup.bat prose ("cmd.exe's", "GitHub's", '...a literal " would close...'). + # A balanced same-line pair on its OWN line must still be harmless either way. + bat = tmp_path / "sample.bat" + bat.write_text( + "@echo off\r\n" + "rem cmd.exe's own quoting rules mean a literal \" would close the quote.\r\n" + "rem This is just an ordinary sentence that doesn't need any escaping here.\r\n" + "echo done\r\n", + encoding="ascii", + ) + result = check_delimiters.main([str(bat)]) + captured = capsys.readouterr() + + assert result == 0 + assert "No delimiter issues found." in captured.out diff --git a/tools/check_delimiters.py b/tools/check_delimiters.py index 273b5cd9..734b49b2 100644 --- a/tools/check_delimiters.py +++ b/tools/check_delimiters.py @@ -37,7 +37,9 @@ class StackItem: char: str line: int column: int - is_echo_open: bool = False + # None = not opened on an echo/rem prose line; "echo" / "rem" = which command's + # text it was opened on (see the cross-line-close check in pop() below). + prose_kind: Optional[str] = None class LineCursor: @@ -142,8 +144,8 @@ def __init__(self, path: pathlib.Path) -> None: def add_issue(self, line: int, column: int, message: str) -> None: self.issues.append(Issue(self.path, line, column, message)) - def push(self, char: str, line: int, column: int, is_echo_open: bool = False) -> None: - self.stack.append(StackItem(char, line, column, is_echo_open)) + def push(self, char: str, line: int, column: int, prose_kind: Optional[str] = None) -> None: + self.stack.append(StackItem(char, line, column, prose_kind)) def pop(self, expected: str, line: int, column: int, actual: str) -> None: if not self.stack: @@ -157,7 +159,7 @@ def pop(self, expected: str, line: int, column: int, actual: str) -> None: f"Mismatched '{actual}' (expected to close '{last.char}' from line {last.line}, column {last.column})", ) return - if last.char == "(" and last.is_echo_open and line != last.line: + if last.char == "(" and last.prose_kind and line != last.line: # derived requirement: cmd.exe's parenthesized-block parser counts '(' / ')' # characters inside plain "echo" text too -- it has no concept of "this paren # is just prose." A '(' opened on one echo line and closed on a LATER echo line @@ -167,16 +169,19 @@ def pop(self, expected: str, line: int, column: int, actual: str) -> None: # if(...)/for(...) block, the stray pair can still corrupt cmd.exe's own block- # closing search. See docs/agent-lessons-learned.md's "A literal (/) inside echo # text is NOT invisible..." entry for the real regression this closes (PR #408, - # commit fd52a3f: "failed was unexpected at this time.", 6 CI lanes broken). + # commit fd52a3f: "failed was unexpected at this time.", 6 CI lanes broken) and + # its "rem" comment sibling (PR #445, Item 52 -- rem lines were fully skipped by + # this checker and hit the identical hazard undetected until real Windows CI). self.add_issue( last.line, last.column, - f"Batch: '(' opened on this 'echo' line does not close until line {line}; " - "cmd.exe's parenthesized-block parser counts parens in echo text too, so a " - "cross-line split can corrupt an enclosing if/for block's structure even " - "though the pair is individually balanced. Keep the pair on one line, avoid " - "literal parens in wrapped prose (prefer ' -- ' or ','), or escape both as " - "'^(' / '^)' if they are structurally necessary.", + f"Batch: '(' opened on this '{last.prose_kind}' line does not close until " + f"line {line}; cmd.exe's parenthesized-block parser counts parens in " + f"{last.prose_kind} text too, so a cross-line split can corrupt an enclosing " + "if/for block's structure even though the pair is individually balanced. " + "Keep the pair on one line, avoid literal parens in wrapped prose (prefer " + "' -- ' or ','), or escape both as '^(' / '^)' if they are structurally " + "necessary.", ) def check(self) -> List[Issue]: @@ -242,13 +247,27 @@ def check(self) -> List[Issue]: stripped = line.lstrip() is_bat_echo_line = False + is_bat_rem_line = False if lower_suffix in {".bat", ".cmd"}: upper = stripped.upper() - if upper.startswith("REM ") or upper == "REM" or stripped.startswith("::"): + if stripped.startswith("::"): continue - # derived requirement: matches "echo", "echo.", "echo(", "echo message" -- - # anything cmd.exe itself treats as the echo command -- but not "echofoo". - is_bat_echo_line = re.match(r"echo\b", stripped, re.IGNORECASE) is not None + if upper.startswith("REM ") or upper == "REM": + # derived requirement (CLAUDE.md Item 61, PR #445 Item 52 incident): a + # "rem" line is NOT opaque to cmd.exe's own parenthesized-block parser -- + # it counts '(' / ')' characters inside rem comment text exactly the same + # way it does inside echo text (see the cross-line-close check in pop() + # above). Route rem lines through the same character scan echo lines + # already get instead of skipping them outright, so a cross-line paren + # pair inside rem prose, nested inside a real enclosing if/for block, is + # caught the same way an echo-text one already is. "::" stays fully + # skipped -- a real label token, not prose text, and out of this item's + # scope. + is_bat_rem_line = True + else: + # derived requirement: matches "echo", "echo.", "echo(", "echo message" -- + # anything cmd.exe itself treats as the echo command -- but not "echofoo". + is_bat_echo_line = re.match(r"echo\b", stripped, re.IGNORECASE) is not None while True: ch = cursor.current() @@ -328,15 +347,43 @@ def check(self) -> List[Issue]: self.here_string = "'@" break + if ( + lower_suffix in {".bat", ".cmd"} + and ch in "(){}[]" + and count_preceding(line, cursor.index, "^") % 2 == 1 + ): + # derived requirement (found while extending the rem-line cross-line-paren + # check, CLAUDE.md Item 61): cmd.exe's own escape character ('^') in front + # of a bracket makes it a literal character there, not a real block + # delimiter -- and this repo's own established convention for defusing the + # cross-line-paren hazard is exactly to write it as '^(' / '^)' (see + # docs/agent-lessons-learned.md). Failing to recognize the escape here would + # make the checker flag the very construct that fixes the hazard -- confirmed + # directly: run_setup.bat's own file-header rem block (lines ~43-58) uses + # "CRLF ^)" / "^(no goto/call...breaks^)" style escaping extensively, and + # without this check every one of those was mis-tracked as a real, + # structurally significant bracket, corrupting the stack for the rest of the + # file. Applies to all four bracket characters (not just parens) and to every + # .bat/.cmd line (not only echo/rem text), since '^' escaping is general + # cmd.exe syntax, not a prose-specific convention. + cursor.advance() + continue + if ch in "({[": # derived requirement: only the case where this paren is ALREADY nested # inside another open bracket (a real enclosing if/for block) is actually - # hazardous -- a top-level echo statement with a self-contained paren pair - # split across two otherwise-independent echo COMMANDS (no enclosing block - # for cmd.exe to misparse) is harmless, confirmed against a real instance in - # this file (:print_fastpath_ambiguous_note) that would otherwise false-flag. - echo_open = ch == "(" and is_bat_echo_line and bool(self.stack) - self.push(ch, line_no, cursor.column(), is_echo_open=echo_open) + # hazardous -- a top-level echo/rem line with a self-contained paren pair + # split across two otherwise-independent lines (no enclosing block for + # cmd.exe to misparse) is harmless, confirmed against real instances in + # this file (:print_fastpath_ambiguous_note for echo; a top-level rem + # header block for rem) that would otherwise false-flag. + prose_kind: Optional[str] = None + if ch == "(" and bool(self.stack): + if is_bat_echo_line: + prose_kind = "echo" + elif is_bat_rem_line: + prose_kind = "rem" + self.push(ch, line_no, cursor.column(), prose_kind=prose_kind) cursor.advance() continue @@ -346,6 +393,40 @@ def check(self) -> List[Issue]: cursor.advance() continue + if lower_suffix in {".bat", ".cmd"} and ch in "'\"": + if ch == "'": + # derived requirement (found while extending the rem-line + # cross-line-paren check, CLAUDE.md Item 61): cmd.exe has no concept + # of a single-quote string delimiter at all -- only '"' is meaningful + # to it, in real code. The generic quote-tracking below previously + # treated a bare apostrophe in .bat/.cmd text as opening a string, + # which was harmless for the small amount of real .bat CODE this + # scanner used to see (code rarely contains a stray apostrophe) but + # corrupts everything once rem/echo PROSE routes through here too -- + # an ordinary contraction like "doesn't" or a possessive like + # "user's" would silently swallow every character (including real + # parens) up to the NEXT apostrophe as fake "string content". + # Confirmed directly: run_setup.bat's own file-header rem block hits + # this via "gitattributes'", "GitHub's", "user's", "cmd.exe's", etc. + # Always inert, on every .bat/.cmd line -- matches real cmd.exe + # semantics, where a bare "'" is never special anywhere in the file. + cursor.advance() + continue + if is_bat_echo_line or is_bat_rem_line: + # derived requirement: unlike a real command line (where '"' groups + # an argument), an echo/rem line's text has no "quoted argument" + # concept at all to cmd.exe -- the whole remainder of the line is + # just text. Prose can legitimately contain an ODD count of '"' + # (documentation describing the quote character itself), which would + # otherwise open a persistent "string" that incorrectly swallows + # every following character -- including real parens on LATER lines + # -- until some unrelated, later '"' happens to "close" it. Confirmed + # directly: run_setup.bat's own rem text (line ~1036: "...(a literal + # \" would close the cmd-level quote)."). '"' stays fully meaningful + # on a real (non-prose) .bat/.cmd code line, e.g. `set "VAR=..."`. + cursor.advance() + continue + if ch in "'\"": triple = False if lower_suffix == ".py" and is_python_triple_quote(line, cursor.index, ch): From 978db636b80707dd5cd8f4fd7676cf38eb164c82 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 21 Aug 2026 23:26:42 +0000 Subject: [PATCH 2/3] Address CodeRabbit review: tab-delimited rem, doc wording, stronger test - tools/check_delimiters.py: recognize "rem" followed by a tab (not just a space) as a real rem line in both .bat/.cmd scan passes, via a single shared REM_LINE_RE used at both call sites so they cannot drift apart. cmd.exe treats a tab exactly like a space after "rem"; the previous literal "REM " check silently left such a line's parens untracked by the cross-line-paren hazard check (Major finding, verified by CodeRabbit's own scripted repro before and after the fix). - tests/test_check_delimiters_import.py: added a tab-delimited regression test, and strengthened the apostrophe/standalone-quote regression test to nest inside a real block with a later cross-line rem pair that must still be flagged -- the original fixture had no parens after the quote characters, so a regressed implementation could pass it without proving normal scanning actually resumes. - CLAUDE.md: cite run_setup.bat's file-header block by its stable "LINE-ENDING SELF-CHECK" label instead of approximate line numbers, and correct the remaining-scope wording -- the hazard surfaces from cmd.exe parsing an enclosing block's raw text, not from the block's own condition evaluating true. - docs/agent-lessons-learned.md: mark the preceding paragraph's "does NOT catch it" as explicitly historical (before Item 61) so it no longer reads as contradicting the fix documented immediately after it. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_017SQ1rvJxDbE71pTXJ4QvLV --- CLAUDE.md | 14 ++++--- docs/agent-lessons-learned.md | 7 ++-- tests/test_check_delimiters_import.py | 54 ++++++++++++++++++++++++--- tools/check_delimiters.py | 14 +++++-- 4 files changed, 71 insertions(+), 18 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 750be170..e5a997b2 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1060,7 +1060,8 @@ way (no live Windows execution available here), that is noted explicitly rather bogus findings on first attempt, cascading from a handful of root causes) rather than useful:** 1. **cmd.exe's own `^` escape character was not recognized at all.** `^(` / `^)` is this file's own established, already-documented convention for defusing this exact hazard (see the error - message's own suggested fix) -- `run_setup.bat`'s file-header rem block (lines ~43-58) uses it + message's own suggested fix) -- `run_setup.bat`'s file-header rem block (the `LINE-ENDING + SELF-CHECK` block) uses it extensively ("Windows ^(CRLF^)", etc.). Without recognizing it, the checker flagged the very construct that fixes the hazard, and every mistracked bracket corrupted the stack for everything scanned afterward. Fixed: a bracket character on any `.bat`/`.cmd` line preceded by @@ -1094,10 +1095,13 @@ way (no live Windows execution available here), that is noted explicitly rather `run_setup.bat` itself** (run `python tools/check_delimiters.py run_setup.bat` for the current, authoritative list -- do not copy the list here, since any future edit to the file shifts every line number). Each is a real cross-line `(`/`)` pair inside `rem` prose, nested inside a real - `if`/`for` block, that predates this fix and was invisible to the checker until now (an existing - hazard only manifests when the specific surrounding code happens to also be reached/reparsed in a - way that exposes it -- do not assume `run_setup.bat` was clean of this bug just because CI has - been green; `check_delimiters.py` is advisory-only, not wired into any `.github/workflows/*.yml` + `if`/`for` block, that predates this fix and was invisible to the checker until now (cmd.exe + parses an enclosing `if`/`for` block's full raw text to find its closing paren BEFORE it ever + evaluates the block's own condition, so the hazard can surface purely from cmd.exe reaching and + parsing that block -- whenever the file is invoked and control flow reaches that point -- even + if the condition itself would have evaluated false and the body never executed; do not assume + `run_setup.bat` was clean of this bug just because CI has been green; `check_delimiters.py` is + advisory-only, not wired into any `.github/workflows/*.yml` gate, so this has not broken CI, only the local/agent-facing sanity-sweep discipline). **Whoever picks this up next**: audit each one individually (most are very likely simple, low-risk prose rewording -- see the single `(^, &, or |)` reordering this same PR applied to the ONE genuine diff --git a/docs/agent-lessons-learned.md b/docs/agent-lessons-learned.md index 8ad8583b..482b0d60 100644 --- a/docs/agent-lessons-learned.md +++ b/docs/agent-lessons-learned.md @@ -456,9 +456,10 @@ list; the recurring traps that have actually bitten us: enclosing block, confirmed against `:print_fastpath_ambiguous_note`) doesn't false-positive. Tests: `tests/test_check_delimiters_import.py`'s three `test_paren_*` cases. - **The identical hazard applies to `rem` comment text too, and `check_delimiters.py` does NOT - catch it -- confirmed as a second real shipped regression, PR #445 (Item 52), all 8 CI lanes - broken simultaneously.** A `rem` comment explaining "check the highest threshold first" split a + **The identical hazard applies to `rem` comment text too, and `check_delimiters.py` did NOT + catch it at the time (before CLAUDE.md Item 61, see below) -- confirmed as a second real shipped + regression, PR #445 (Item 52), all 8 CI lanes broken simultaneously.** A `rem` comment explaining + "check the highest threshold first" split a parenthetical remark's `(`/`)` across three separate `rem` lines, nested three levels deep inside real `if (...)` blocks (`if exist ( if not errorlevel 1 ( if errorlevel 1 ( ... rem lines here ... )`). Symptom matched the echo-hazard bug exactly: every CI lane that runs a real diff --git a/tests/test_check_delimiters_import.py b/tests/test_check_delimiters_import.py index 5c42e86a..de52faa4 100644 --- a/tests/test_check_delimiters_import.py +++ b/tests/test_check_delimiters_import.py @@ -276,17 +276,59 @@ def test_apostrophe_and_standalone_quote_in_rem_text_do_not_corrupt_paren_tracki # that swallows later, unrelated characters (including real parens) until some # later, unrelated quote happens to "close" it. Confirmed directly against real # run_setup.bat prose ("cmd.exe's", "GitHub's", '...a literal " would close...'). - # A balanced same-line pair on its OWN line must still be harmless either way. + # + # derived requirement (CodeRabbit review, PR #449): the original version of this + # fixture had no '(' or ')' anywhere AFTER the apostrophe/standalone-quote lines, + # so a REGRESSED implementation that still corrupts string-state tracking (and + # therefore never resumes normal scanning at all) could pass this test purely by + # having nothing left to scan. Nested inside a real enclosing block, with a real + # cross-line rem paren pair immediately afterward that MUST still be flagged -- + # proving the fix genuinely resumes correct paren tracking, not just that it + # avoids an immediate crash/false-positive on the quote characters themselves. bat = tmp_path / "sample.bat" bat.write_text( "@echo off\r\n" - "rem cmd.exe's own quoting rules mean a literal \" would close the quote.\r\n" - "rem This is just an ordinary sentence that doesn't need any escaping here.\r\n" - "echo done\r\n", + "if defined HP_NO_INTERPRETER (\r\n" + " rem cmd.exe's own quoting rules mean a literal \" would close the quote.\r\n" + " rem This is just an ordinary sentence that doesn't need any escaping here.\r\n" + " rem method (uv, conda, a fresh download, a\r\n" + " rem local virtual environment) failed -- usually\r\n" + " exit /b 0\r\n" + ")\r\n", encoding="ascii", ) result = check_delimiters.main([str(bat)]) captured = capsys.readouterr() - assert result == 0 - assert "No delimiter issues found." in captured.out + assert result == 1 + assert "does not close until line" in captured.out + assert "counts parens in rem text too" in captured.out + + +def test_tab_delimited_rem_line_inside_block_is_flagged(tmp_path, capsys): + # derived requirement (CodeRabbit review, PR #449, Major): cmd.exe treats a TAB + # exactly like a space as the word separator after "rem" -- "rem\tsomething" is + # just as much a real comment as "rem something". The original REM_LINE_RE-less + # classifier (a literal "REM " startswith check) missed this: a tab-delimited rem + # line matched neither the rem branch nor the echo branch, so its parens were + # scanned WITHOUT prose_kind tagging and a cross-line pair on such a line was + # silently never flagged. Same fixture shape as + # test_paren_split_across_rem_lines_inside_block_is_flagged above, but with a tab + # after "rem" instead of a space, proving both rem-detection call sites recognize + # it identically. + bat = tmp_path / "sample.bat" + bat.write_text( + "@echo off\r\n" + "if defined HP_NO_INTERPRETER (\r\n" + " rem\tmethod (uv, conda, a fresh download, a\r\n" + " rem\tlocal virtual environment) failed -- usually\r\n" + " exit /b 0\r\n" + ")\r\n", + encoding="ascii", + ) + result = check_delimiters.main([str(bat)]) + captured = capsys.readouterr() + + assert result == 1 + assert "does not close until line" in captured.out + assert "counts parens in rem text too" in captured.out diff --git a/tools/check_delimiters.py b/tools/check_delimiters.py index 734b49b2..101fae1e 100644 --- a/tools/check_delimiters.py +++ b/tools/check_delimiters.py @@ -20,6 +20,14 @@ ".json", } +# derived requirement (CodeRabbit review, PR #449): cmd.exe treats a TAB exactly like a +# space as the word separator after "rem" -- `rem\tsomething` is just as much a real +# comment as `rem something`. A literal `"REM "` (space only) prefix check misses it, +# silently leaving such a line's parens untracked by the cross-line-paren hazard check. +# Shared by BOTH .bat/.cmd rem-detection call sites in this file so they cannot drift +# apart -- do not inline a second, differently-spelled check. +REM_LINE_RE = re.compile(r"rem(?:[ \t]|$)", re.IGNORECASE) + @dataclass class Issue: @@ -249,10 +257,9 @@ def check(self) -> List[Issue]: is_bat_echo_line = False is_bat_rem_line = False if lower_suffix in {".bat", ".cmd"}: - upper = stripped.upper() if stripped.startswith("::"): continue - if upper.startswith("REM ") or upper == "REM": + if REM_LINE_RE.match(stripped): # derived requirement (CLAUDE.md Item 61, PR #445 Item 52 incident): a # "rem" line is NOT opaque to cmd.exe's own parenthesized-block parser -- # it counts '(' / ')' characters inside rem comment text exactly the same @@ -457,8 +464,7 @@ def check(self) -> List[Issue]: for line_no, raw_line in enumerate(lines, start=1): line = raw_line.rstrip("\n\r") stripped = line.lstrip() - upper = stripped.upper() - if upper.startswith("REM ") or upper == "REM" or stripped.startswith("::"): + if REM_LINE_RE.match(stripped) or stripped.startswith("::"): continue scan_from = None if not self._bat_in_backtick: From 2c9e14f0b3db67e6f49d48436544437f68578270 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 21 Aug 2026 23:28:40 +0000 Subject: [PATCH 3/3] Fix stale test-count references after the tab-delimited-rem follow-up CodeRabbit caught this: CLAUDE.md and docs/agent-lessons-learned.md still said "4 new tests" / "13 tests total" after the previous commit's follow-up added a 5th test (tab-delimited rem detection), bringing the real total to 14 tests / 5 added. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_017SQ1rvJxDbE71pTXJ4QvLV --- CLAUDE.md | 13 ++++++++----- docs/agent-lessons-learned.md | 2 +- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index e5a997b2..b7c046d2 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1083,12 +1083,15 @@ way (no live Windows execution available here), that is noted explicitly rather echo output text is comparatively sparse and formal (rarely uses contractions or `^`-escaping) compared to the much higher volume of informal, dev-facing `rem` prose. - **Coverage added**: `tests/test_check_delimiters_import.py` gained 4 new tests -- the positive + **Coverage added**: `tests/test_check_delimiters_import.py` gained 5 new tests -- the positive (`rem` cross-line pair nested inside a real block, flagged) and negative (top-level `rem` header, - not flagged) cases originally scoped for this item, plus two regression tests for the two - correctness fixes above (a `^`-escaped rem-line pair is not flagged; an apostrophe/standalone-quote - rem line does not corrupt paren tracking). All 13 tests in that file, and the full pytest suite, - pass. + not flagged) cases originally scoped for this item, two regression tests for the two correctness + fixes above (a `^`-escaped rem-line pair is not flagged; an apostrophe/standalone-quote rem line, + nested inside a real block with a genuine cross-line pair immediately after, still gets the pair + flagged -- proving normal scanning resumes, not just that the quote characters themselves don't + crash it), plus one more from a CodeRabbit review round on this same PR (a tab-delimited `rem` + line is recognized identically to a space-delimited one). All 14 tests in that file, and the + full pytest suite, pass. **Remaining scope, concretely bounded (NOT closed by this fix -- this is the actual follow-up work, not a hypothetical): 26 genuine, previously-invisible findings are now surfaced in diff --git a/docs/agent-lessons-learned.md b/docs/agent-lessons-learned.md index 482b0d60..4da22441 100644 --- a/docs/agent-lessons-learned.md +++ b/docs/agent-lessons-learned.md @@ -488,7 +488,7 @@ list; the recurring traps that have actually bitten us: specifically on rem/echo PROSE lines (an ordinary contraction/possessive, or a standalone `"` describing the quote character itself, was previously opening a persistent, incorrectly cross-line "string" that swallowed later real parens as fake string content). See CLAUDE.md's - Item 61 entry for the full fix trace, the 4 new regression tests, and the still-open follow-up: + Item 61 entry for the full fix trace, the 5 new regression tests, and the still-open follow-up: running the fixed checker against `run_setup.bat` surfaced 26 genuine, previously-invisible cross-line `rem` pairs already in the file that need their own audit (not attempted in this same slice, per this repo's own one-slice-at-a-time discipline for anything touching `run_setup.bat`).