diff --git a/docs/agent-lessons-learned.md b/docs/agent-lessons-learned.md index 137fbde4..cbe29b4f 100644 --- a/docs/agent-lessons-learned.md +++ b/docs/agent-lessons-learned.md @@ -455,16 +455,23 @@ list; the recurring traps that have actually bitten us: `failed was unexpected at this time.` This shipped in a new `:preflight_compile` message block (the item-14 fix, see `docs/agent-closed-backlog.md`'s Item 14 entry for the full trace) and broke every CI lane whose own self-tests reach that branch in the same run (6 lanes failed - simultaneously on one commit) -- caught by real CI, not by any local check: `python - tools/check_delimiters.py run_setup.bat` passed clean on the broken version, a confirmed gap in - that tool (its paren-balance logic does not currently walk `echo`/prose text inside a block the - way cmd.exe's own parser does). **Rule of thumb: never let a `(` and its matching `)` land on - different lines inside any parenthesized `.bat` block, even inside `echo`/prose text that looks - purely cosmetic.** Either keep the pair on the same line, avoid literal parens in wrapped prose - entirely (prefer ` -- ` or `,`), or escape both with `^(`/`^)` if the parens are structurally - necessary. When manually reviewing a new multi-line `echo` block inside an `if`/`for` block, - count parens per-line as part of the review, not just per-block -- `check_delimiters.py`'s - current implementation will not catch an imbalance introduced this way. + simultaneously on one commit) -- caught by real CI, not by any local check at the time: `python + tools/check_delimiters.py run_setup.bat` passed clean on the broken version, since a stray `(...)` + pair inside echo text is individually balanced (one open, one close), so a whole-file LIFO + paren-count scan sees nothing wrong -- the hazard is specifically about a cross-line split + landing inside an ALREADY-open enclosing block, not a raw count mismatch. **Rule of thumb: never + let a `(` and its matching `)` land on different lines inside any parenthesized `.bat` block, + even inside `echo`/prose text that looks purely cosmetic.** Either keep the pair on the same + line, avoid literal parens in wrapped prose entirely (prefer ` -- ` or `,`), or escape both with + `^(`/`^)` if the parens are structurally necessary. **Gap closed same day**: + `check_delimiters.py` now tracks, for `.bat`/`.cmd` files, whether each `(` was opened on an + `echo` line while ALREADY nested inside another open bracket (a real enclosing `if`/`for` + block) -- if so, and its matching `)` closes on a different source line, it's flagged. Scoped + to "already nested" specifically because a top-level echo statement with no enclosing block + (confirmed against a real, harmless instance in this file, `:print_fastpath_ambiguous_note`) has + no block-closing search for cmd.exe to corrupt, so flagging it would be a pure false positive. + Regression tests: `tests/test_check_delimiters_import.py`'s three `test_paren_*` cases (flagged + when nested, not flagged at top level, not flagged for a same-line pair). - **Avoid `EnableDelayedExpansion`; if unavoidable, wrap it tightly.** `!` becomes special under delayed expansion, and a parent shell launched with `/V:ON` causes `!`-collisions. `tests/harness.ps1` `batch.bang.scan` enforces "no `!` in live batch code lines." diff --git a/tests/test_check_delimiters_import.py b/tests/test_check_delimiters_import.py index d8637b67..9df3386e 100644 --- a/tests/test_check_delimiters_import.py +++ b/tests/test_check_delimiters_import.py @@ -72,3 +72,71 @@ def test_rem_prefixed_word_outside_bat_file_is_not_flagged(tmp_path, capsys): assert result == 0 assert "No delimiter issues found." in captured.out + + +# derived requirement: these three tests are a regression guard for a second real bug +# that shipped in run_setup.bat and broke 6 CI lanes simultaneously (see +# docs/agent-lessons-learned.md's "A literal (/) inside echo text is NOT invisible..." +# entry, PR #408 commit fd52a3f). A "(" opened on one "echo" line and closed by its +# matching ")" on a LATER "echo" line, both inside an enclosing if(...) block, is +# balanced from a pure count-matching perspective (which is why the pre-existing +# unclosed/mismatched checks missed it) but corrupts cmd.exe's own block-closing +# search at runtime -- "failed was unexpected at this time." check_delimiters.py did +# not catch this at the time; this dedicated check closes that gap. +def test_paren_split_across_echo_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" + " echo *** method (uv, conda, a fresh download, a ***\r\n" + " echo *** 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 echo text too" in captured.out + + +def test_paren_split_across_echo_lines_at_top_level_is_not_flagged(tmp_path, capsys): + # Same textual pattern as above, but with no enclosing if/for block -- each echo + # line is an independent top-level command, so there is no block-closing search + # for cmd.exe to corrupt. A real instance of this shape exists in run_setup.bat + # (:print_fastpath_ambiguous_note) and must not false-positive. + bat = tmp_path / "sample.bat" + bat.write_text( + "@echo off\r\n" + "echo and it exited with an error just now (see the status line\r\n" + "echo 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 + + +def test_paren_pair_on_same_echo_line_is_not_flagged(tmp_path, capsys): + # A balanced pair on a single echo line (common, e.g. a parenthetical aside) is + # always safe regardless of block nesting -- only a CROSS-line split is risky. + bat = tmp_path / "sample.bat" + bat.write_text( + "@echo off\r\n" + "if defined FOO (\r\n" + " echo *** see the docs (specifically the README) for details ***\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 diff --git a/tools/check_delimiters.py b/tools/check_delimiters.py index b3fd8acb..273b5cd9 100644 --- a/tools/check_delimiters.py +++ b/tools/check_delimiters.py @@ -37,6 +37,7 @@ class StackItem: char: str line: int column: int + is_echo_open: bool = False class LineCursor: @@ -141,8 +142,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) -> None: - self.stack.append(StackItem(char, line, column)) + 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 pop(self, expected: str, line: int, column: int, actual: str) -> None: if not self.stack: @@ -155,6 +156,28 @@ def pop(self, expected: str, line: int, column: int, actual: str) -> None: column, 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: + # 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 + # is syntactically balanced from a pure count-matching perspective (which is why + # this specific case needs its own check, separate from the generic unclosed/ + # mismatched checks above), but if that echo line sits inside a real + # 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). + 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.", + ) def check(self) -> List[Issue]: try: @@ -218,10 +241,14 @@ def check(self) -> List[Issue]: cursor.advance(idx + 2) stripped = line.lstrip() + is_bat_echo_line = False if lower_suffix in {".bat", ".cmd"}: upper = stripped.upper() if upper.startswith("REM ") or upper == "REM" or 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 while True: ch = cursor.current() @@ -302,7 +329,14 @@ def check(self) -> List[Issue]: break if ch in "({[": - self.push(ch, line_no, cursor.column()) + # 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) cursor.advance() continue