From 41facb30c33a111b15b88a1caf1cd6342effce55 Mon Sep 17 00:00:00 2001 From: Jon Froehlich Date: Tue, 18 Aug 2026 14:33:42 -0700 Subject: [PATCH 1/2] Actually fix the two collapsed runbook snippets, and guard the class (#126) Review finding 14 on #129 was reported fixed and was not: the patch script that "fixed" it was written as a quoted bash heredoc, which strips one level of backslash, so the replacement text's trailing "\" + newline became a Python line continuation and the newline was removed. The result was the same collapsed line it was meant to repair -- with the --op-threshold correction applied around it, which is why the diff looked plausible. Both snippets now carry real continuations, written via chr(92) so no layer can eat them. The guard is the point: a collapsed continuation still runs, still looks fine at a glance, and its only symptom is that someone copying the wrapped form gets a broken command -- so nothing catches it. The new test walks every shell block in the runbook docs and rejects run-on spaces mid-command, excluding the two forms that legitimately use them (aligned trailing comments, and lines that still have their backslash). It found nothing else, so these two were the only ones. Co-Authored-By: Claude Opus 5 (1M context) --- docs/model_comparison.md | 6 ++++-- tests/test_roster.py | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 2 deletions(-) diff --git a/docs/model_comparison.md b/docs/model_comparison.md index e1fe7a91..4dbf851a 100644 --- a/docs/model_comparison.md +++ b/docs/model_comparison.md @@ -1155,7 +1155,8 @@ driveway aprons should appear as a characteristic false-positive mode — which recall hides on the other side of it is measurable: ```bash -python scripts/model_comparison/compare.py benchmark/richmond --models rampnet,vistas:curb-cut,vistas:curb-cut+curb +python scripts/model_comparison/compare.py benchmark/richmond \ + --models rampnet,vistas:curb-cut,vistas:curb-cut+curb ``` The spec's `model_id` slot carries the **class set**, not a model id — the checkpoint comes @@ -1312,7 +1313,8 @@ No new launcher; the arm needs nothing beyond the `transformers` + `torchvision` models already use, and Mask2Former is in-library (no `trust_remote_code`). ```bash -PYTHON=$ENVPY MODELS=rampnet,vistas:curb-cut BUNDLE=benchmark/richmond sbatch -A scripts/model_comparison/run_open_models.slurm +PYTHON=$ENVPY MODELS=rampnet,vistas:curb-cut BUNDLE=benchmark/richmond \ + sbatch -A scripts/model_comparison/run_open_models.slurm ``` ## Status diff --git a/tests/test_roster.py b/tests/test_roster.py index d4c6104e..9682d0d5 100644 --- a/tests/test_roster.py +++ b/tests/test_roster.py @@ -85,6 +85,40 @@ def _table_lines(text): yield line +def test_no_runbook_snippet_has_a_collapsed_line_continuation(): + """A shell snippet whose trailing backslash was lost still *looks* fine -- the + wrap becomes a run of spaces inside one long line -- and it still runs, so + nothing catches it. But the exact-commands-in-order rule is what these blocks + exist to satisfy, and a reader copying the wrapped form gets a broken command. + + Two of them shipped in #126 because a patch script ate the backslashes. + """ + import re + for name in ("model_comparison.md", "replication.md", "operating_point.md", + "adding_a_benchmark_city.md"): + path = REPO / "docs" / name + if not path.exists(): + continue + in_shell = False + for n, line in enumerate(path.read_text("utf-8").splitlines(), 1): + if line.startswith("```"): + in_shell = line.startswith("```bash") or line.startswith("```sh") + continue + if not in_shell or line.lstrip().startswith("#"): + continue + # An aligned trailing comment legitimately uses run-on spaces. + line = line.split(" #", 1)[0].rstrip() + # A line that still HAS its continuation is correct by definition, + # whatever spacing it uses to align its arguments. + if line.endswith("\\"): + continue + # Three or more spaces mid-command is what a swallowed trailing + # backslash plus newline leaves behind. + hit = re.search(r"\S {3,}\S", line) + assert hit is None, ( + f"docs/{name}:{n} looks like a collapsed line continuation: {line.strip()!r}") + + def test_no_doc_still_hardcodes_the_old_roster_count(): """These exact phrases were the drift. Catch them coming back. From 3c165130ee51dfdfa43435c168fb4343a3035010 Mon Sep 17 00:00:00 2001 From: Jon Froehlich Date: Thu, 3 Sep 2026 06:20:11 -0700 Subject: [PATCH 2/2] Broaden the collapsed-continuation guard to every tracked doc (#126) Review findings F1-F3. F1: the guard walked four runbooks; it now walks every tracked .md via git ls-files, and blanks quoted spans before matching so padding inside a string (data/inventories/README.md:45) is not a hit and a `#` inside quotes cannot hide the rest of the line. F2: fences are detected after lstrip, so a block nested in a list item is scanned. F3: the scan is a module-level helper returning (line_no, line), pinned by three inline fixtures, and the docstring derives the three-space threshold from the four-space continuation indent. The sweep is zero on this branch and on the merge with main; run against main alone it reports exactly the two lines this PR fixes. Co-Authored-By: Claude Opus 5 --- tests/test_roster.py | 110 +++++++++++++++++++++++++++++++++---------- 1 file changed, 86 insertions(+), 24 deletions(-) diff --git a/tests/test_roster.py b/tests/test_roster.py index 9682d0d5..ab9d85ce 100644 --- a/tests/test_roster.py +++ b/tests/test_roster.py @@ -6,6 +6,8 @@ """ import json import pathlib +import re +import subprocess import pytest @@ -85,6 +87,72 @@ def _table_lines(text): yield line +def _blank_quoted(line): + """Replace the contents of "..." and '...' spans with x, keeping the width. + + Quoted text is data, not command structure: a run of spaces or a ``#`` + inside it says nothing about a lost continuation, and left alone the first + produces a false positive and the second hides everything after it.""" + out = [] + quote = None + for ch in line: + if quote is None: + out.append(ch) + if ch in "\"'": + quote = ch + else: + out.append(ch if ch == quote else "x") + if ch == quote: + quote = None + return "".join(out) + + +def _collapsed_continuations(text): + """Yield (line_no, line) for every shell line that looks like a lost backslash. + + Three spaces is the threshold because these runbooks indent continuations by + four, so a swallowed trailing backslash leaves at least that indent inside + one line; a flush-left or two-space continuation would collapse below three + and is not used here. + """ + in_shell = False + for n, line in enumerate(text.splitlines(), 1): + stripped = line.lstrip() + if stripped.startswith("```"): + # lstrip, so a fence nested in a list item is entered and left. + in_shell = stripped.startswith("```bash") or stripped.startswith("```sh") + continue + if not in_shell or stripped.startswith("#"): + continue + scan = _blank_quoted(line) + # An aligned trailing comment legitimately uses run-on spaces. + scan = scan.split(" #", 1)[0].rstrip() + # A line that still HAS its continuation is correct by definition, + # whatever spacing it uses to align its arguments. + if scan.endswith("\\"): + continue + # Three or more spaces mid-command is what a swallowed trailing + # backslash plus newline leaves behind. + if re.search(r"\S {3,}\S", scan): + yield n, line + + +def test_the_collapsed_continuation_scanner_catches_the_shape_it_claims(): + """The scanner is the guard; these fixtures are the guard on the guard. + + Planting a defect in a committed doc is the only other way to check that it + still catches the class, and that is not a thing to leave lying around.""" + collapsed = "```bash\npython x.py --a b --c d\n```\n" + assert list(_collapsed_continuations(collapsed)) == [ + (2, "python x.py --a b --c d")] + + aligned_comment = "```bash\npython x.py --a b # what b is for\n```\n" + assert list(_collapsed_continuations(aligned_comment)) == [] + + kept_continuation = "```bash\npython x.py --a b \\\n --c d\n```\n" + assert list(_collapsed_continuations(kept_continuation)) == [] + + def test_no_runbook_snippet_has_a_collapsed_line_continuation(): """A shell snippet whose trailing backslash was lost still *looks* fine -- the wrap becomes a run of spaces inside one long line -- and it still runs, so @@ -92,31 +160,25 @@ def test_no_runbook_snippet_has_a_collapsed_line_continuation(): exist to satisfy, and a reader copying the wrapped form gets a broken command. Two of them shipped in #126 because a patch script ate the backslashes. + + Every tracked .md is walked, not the four runbooks the two defects happened + to be in: a collapsed snippet in README.md would be just as invisible. """ - import re - for name in ("model_comparison.md", "replication.md", "operating_point.md", - "adding_a_benchmark_city.md"): - path = REPO / "docs" / name - if not path.exists(): - continue - in_shell = False - for n, line in enumerate(path.read_text("utf-8").splitlines(), 1): - if line.startswith("```"): - in_shell = line.startswith("```bash") or line.startswith("```sh") - continue - if not in_shell or line.lstrip().startswith("#"): - continue - # An aligned trailing comment legitimately uses run-on spaces. - line = line.split(" #", 1)[0].rstrip() - # A line that still HAS its continuation is correct by definition, - # whatever spacing it uses to align its arguments. - if line.endswith("\\"): - continue - # Three or more spaces mid-command is what a swallowed trailing - # backslash plus newline leaves behind. - hit = re.search(r"\S {3,}\S", line) - assert hit is None, ( - f"docs/{name}:{n} looks like a collapsed line continuation: {line.strip()!r}") + hits = [] + for path in _tracked_docs(): + rel = path.relative_to(REPO).as_posix() + hits += [f"{rel}:{n} {line.strip()!r}" + for n, line in _collapsed_continuations(path.read_text("utf-8"))] + assert hits == [], ( + "these lines look like collapsed line continuations:\n" + "\n".join(hits)) + + +def _tracked_docs(): + """Tracked .md paths. git ls-files, not a walk: the checkout can contain + nested worktrees and virtualenvs that a walk would descend into.""" + listing = subprocess.run(["git", "-C", str(REPO), "ls-files", "-z", "*.md"], + capture_output=True, text=True, check=True) + return [REPO / p for p in listing.stdout.split("\0") if p] def test_no_doc_still_hardcodes_the_old_roster_count():