Skip to content

Actually fix the two collapsed runbook snippets from #129, and guard the class - #136

Open
jonfroehlich wants to merge 2 commits into
mainfrom
docs/fix-vistas-snippets
Open

Actually fix the two collapsed runbook snippets from #129, and guard the class#136
jonfroehlich wants to merge 2 commits into
mainfrom
docs/fix-vistas-snippets

Conversation

@jonfroehlich

Copy link
Copy Markdown
Member

Review finding 14 on #129 was reported fixed and was not. Correcting the record along with the code.

What happened. The patch script that fixed it was 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 output was the same collapsed line it was meant to repair. The --op-threshold correction in the same replacement did apply, so the diff looked right.

Both snippets now carry real continuations (written via chr(92), so no layer can eat them):

python scripts/model_comparison/compare.py benchmark/richmond \
    --models rampnet,vistas:curb-cut,vistas:curb-cut+curb

The guard matters more than the two lines. A collapsed continuation still executes, still reads as fine, and its only symptom is that a reader copying the wrapped form gets a broken command — so nothing catches it, including a careful review. test_no_runbook_snippet_has_a_collapsed_line_continuation 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 carry their backslash. It found nothing beyond these two.

Suite 1,238 → 1,239.

🤖 Generated with Claude Code (claude-opus-5[1m], effort: high)

…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) <noreply@anthropic.com>
@jonfroehlich

Copy link
Copy Markdown
Member Author

Deep review

Re-ran from the branch head (41facb3) in a clean detached worktree. The two snippets are fixed, the guard catches exactly the class it claims to, and the PR is still needed: main is 23 commits ahead of this branch and both collapsed lines are still there (docs/model_comparison.md:1192 and :1349 on main). Checks that came back clean:

  • Full suite: 1239 passed, 1 skipped, as the body says.
  • git merge-tree against origin/main: clean. Against origin/exp/vistas-parity-126 (Vistas at resolution parity: the handicap was real, and it was recall (#126) #137, which rewrites 270 lines of the same doc): clean. The merged tree passes the guard.
  • The guard's exceptions are exercised, not decorative: across the four walked docs it scans 150 shell lines, 25 of which carry an aligned trailing comment and 32 of which end in a backslash.
  • Run against main's copy of the doc, the test fails on line 1192 (the first collapsed snippet); run against the branch's copy, it passes. Synthetic fixtures: a 3-space and a 5-space collapse are both caught, a collapse followed by an aligned comment is caught, a kept continuation with aligned arguments passes, an aligned trailing comment passes.
  • The fixed --models rampnet,vistas:curb-cut,vistas:curb-cut+curb parses on both the branch and main: parse_model_spec is partition(":"), VISTAS_CLASS_SETS has curb-cut+curb, and roster.label_for resolves both specs on both refs. The slurm snippet's PYTHON / MODELS / BUNDLE are the variables run_open_models.slurm reads on both refs.
  • "It found nothing beyond these two" is true for the four docs the test walks.

Everything below is about how far the guard reaches, not about what it does on the lines it reaches.

1. Medium-low — the guard walks four docs; the repo has eighteen with shell blocks

The test names model_comparison.md, replication.md, operating_point.md and adding_a_benchmark_city.md. On main those hold 25 of the 53 bash fences in the repo. Not walked: README.md (5 blocks, the front door), docs/stage2_epoch_curve_84.md (4), data/inventories/README.md (4), scripts/model_comparison/yolo_baseline/README.md (2), docs/tillicum.md (2), docs/stage2_training_cost.md (2), docs/data_provenance.md (2), the three HF card templates, scripts/analysis/README.md, docs/stage1_generation_cost.md, docs/seam.md, docs/model_scoreboard.md. A collapsed snippet in any of those is as invisible as the two this PR fixes were.

Running the same logic over every .md on main flags exactly one line outside the walked set, and it is a false positive: data/inventories/README.md:45 is echo "OK $(basename "$f")", padding inside a quoted string. So broadening the walk costs one carve-out. Blanking quoted spans before the regex ("…" and '…') removes that hit, and it also closes the hole in finding 2. The sweep then stays at zero on main, on the branch, and on the merged tree.

Fix: walk every tracked *.md instead of four names; blank quoted spans before matching; keep the sweep at zero.

2. Low — split(" #", 1) truncates at a # inside quotes

A quoted argument containing # hides everything after it, so python x.py --tag 'v #1' --b c passes. No line in the walked docs has that shape today. Blanking quoted spans first (finding 1) fixes it without a second exception.

3. Low — an indented fence is never entered

Fence detection is line.startswith("```"), so a fenced block inside a list item (two-space indent) is not scanned at all, opening or closing. docs/replication.md:683 on main is one such block, a single line with an aligned comment, currently fine. Fix: line.lstrip().startswith("```") for both the opening and closing fence.

4. Low — the scanner is not itself testable

The scan is inline in the test body, so the only way to check that it still catches the class is to plant a defect in a committed doc. Fix: move the scan into a helper that takes text and returns (line_no, line) hits, and pin it with three inline fixtures: a 5-space collapse reports the right line number; an aligned trailing comment reports nothing; a kept backslash with aligned arguments reports nothing.

Known limits, not fixes

  • The 3-space threshold is right for the repo's 4-space continuation indent (a swallowed \ leaves one trailing space plus the indent). A continuation that was flush-left collapses to one space and is indistinguishable from an ordinary line; a 2-space indent collapses to two. Neither is used in these docs. Worth one sentence in the docstring so the number is not read as arbitrary.
  • console fences are not scanned. shell fences are, because startswith("```sh") matches them. Neither appears in the repo.
  • A heredoc body with aligned columns inside a shell block would fire. None exists in any .md on main.

Decisions for Jon, not fixes

  • Placement. tests/test_roster.py already holds two doc-consistency tests (test_the_roster_table_in_the_docs_matches_the_registry, test_no_doc_still_hardcodes_the_old_roster_count), so this one is not out of place. If the walk broadens to every .md (finding 1), a tests/test_docs.py is the natural home, and the two existing doc tests could move with it. Not worth doing on its own.

What holds up

  • Both snippets carry real continuations. The diff is exactly the two lines plus the test; no prose changed.
  • The guard catches the defect on the real artifact (main's doc) and on synthetic 3- and 5-space collapses, and its two exceptions are the right two: aligned comments and lines that still carry their backslash.
  • Merge health is clean against main and against Vistas at resolution parity: the handicap was real, and it was recall (#126) #137, which is the branch most likely to conflict.
  • The fixed command runs as written on both refs; no argument-parser drift since the branch was cut.

Fix list

  • [F1] Medium-low — Walk every tracked *.md (via git ls-files or an rglob that skips .venv, .claude, node_modules) instead of the four names; blank out "…" and '…' spans before the regex so data/inventories/README.md:45 does not fire and a collapse after a quoted # is not hidden; assert the sweep is zero. — files: tests/test_roster.py
  • [F2] Low — Detect fences with line.lstrip().startswith("```") for both opening and closing, so list-nested blocks (e.g. docs/replication.md:683 on main) are scanned. — files: tests/test_roster.py
  • [F3] Low — Extract the scan into a helper returning (line_no, line) hits and pin it with three inline fixtures (5-space collapse hits the right line; aligned comment and kept backslash with aligned arguments do not). Add one docstring sentence deriving the 3-space threshold from the 4-space continuation indent. — files: tests/test_roster.py

🤖 Generated with Claude Code (claude-fable-5-1)

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 <noreply@anthropic.com>
@jonfroehlich

Copy link
Copy Markdown
Member Author

Review fixes

All three in tests/test_roster.py, commit 3c16513.

  • F1 — fixed. The guard walked four runbook names; it now walks every tracked *.md (git ls-files, not a filesystem walk, because the checkout can contain nested worktrees and virtualenvs). Quoted "…" and '…' spans are blanked before the regex, which drops the one false positive broadening would otherwise add (data/inventories/README.md:45, padding inside a quoted string) and closes F2 at the same time.
  • F2 — fixed. split(" #", 1) no longer truncates at a # inside quotes, since the split now runs on the quote-blanked line. Fences are detected with lstrip() on both the opening and closing line, so blocks nested in a list item are scanned; there are 3 such fences in the merged tree.
  • F3 — fixed. The scan is a module-level helper _collapsed_continuations(text) yielding (line_no, line), pinned by test_the_collapsed_continuation_scanner_catches_the_shape_it_claims with three inline fixtures: a 5-space collapse reports line 2, an aligned trailing comment reports nothing, a kept backslash with aligned arguments reports nothing. Its docstring derives the 3-space threshold from the 4-space continuation indent.

Not applied: the placement question under "Decisions for Jon" — the test stays in tests/test_roster.py.

Sweep results, with the broadened walk:

  • This branch: zero (25 tracked docs).
  • Merge with origin/main: zero. Checked in a throwaway detached worktree of origin/main merged with this branch's head — 27 docs, 404 shell lines, merge clean.
  • Run against origin/main alone it reports exactly two lines, docs/model_comparison.md:1192 and :1349 — the two this PR fixes, and nothing else across all of main's docs.

Full suite: 1240 passed, 1 skipped (one more than before, the new scanner test).

🤖 Generated with Claude Code (claude-opus-5)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant