From cee61b94b93d0d49cc7211c8913255ca4c4c5228 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Wed, 23 Sep 2026 08:06:15 -0700 Subject: [PATCH 01/12] tooling: check-ci-graph fails when a checkers.sh suite runs in no CI step improvise-dak5: ci.yml names its checker suites by hand, so a new scripts/X-test.sh ran locally and never in CI. check-ci-graph.py now asks `checkers.sh --list` for the suites, so the naming rule keeps one home, and reads every checkers.sh invocation in ci.yml. A bare run covers every suite, --self-test covers `checkers`, and a YAML comment is never a run. A suite planted in the real tree fails the check, naming it, and adding it to the step clears it. Seven new self-test cases. Rivals: counting comments fails the comment case; answering "everything runs" fails five. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/code/tooling.md | 11 +++ context/test-evidence/tooling.md | 31 +++++++++ scripts/check-ci-graph.py | 114 +++++++++++++++++++++++++++++-- 3 files changed, 152 insertions(+), 4 deletions(-) diff --git a/context/code/tooling.md b/context/code/tooling.md index 6b09fc0f..37c5af36 100644 --- a/context/code/tooling.md +++ b/context/code/tooling.md @@ -259,6 +259,17 @@ the deploy's and pass. `README.md` therefore states the rule and deliberately **does not list the jobs**: a second copy with nothing holding it to the first is what went stale, twice. +It holds a second rule of the same kind: every suite `scripts/checkers.sh +--list` names must run in some CI step (improvise-dak5). `ci.yml` names its +suites by hand, because they need different jobs and environments (`kb` runs +under a nix trealla, `node-test` in the `js` job), so a single bare +`checkers.sh` was not an option. The check does not re-derive what a suite is. +It asks `checkers.sh --list`, so the naming rule keeps its one home. It then +reads every `checkers.sh` invocation in the workflow. A bare one covers every +suite, and `--self-test` covers `checkers`. A YAML comment is skipped, because a +bare mention there would otherwise read as "everything runs", which is the +failure that looks like success. + ### pr-watch and mergeability (improvise-edpl, improvise-zlj2) `scripts/pr-watch` is what the orchestrator's merge procedure stands on, and diff --git a/context/test-evidence/tooling.md b/context/test-evidence/tooling.md index 3304bd5e..9d869c3e 100644 --- a/context/test-evidence/tooling.md +++ b/context/test-evidence/tooling.md @@ -763,3 +763,34 @@ to spare a harness directory. `opfs.spec.js` instead asserts `/opfs/index.html` is served, naming the fix. Injected by running `web/assemble.sh harness/serve` alone: 4 chromium failures, each carrying that message, where it used to fail with no hint of the cause. + +### Every checkers.sh suite runs in CI — `scripts/check-ci-graph.py` (improvise-dak5) + +**What was unguarded.** `ci.yml` names its `checkers.sh` suites by hand. A new +`scripts/X-test.sh` beside `scripts/X` is picked up by `checkers.sh` locally +and never runs in CI unless someone adds it to a step. Found on #226, where +`bd-audit-ids` had to be added by hand. + +**Planted in the real tree.** `scripts/zz-planted.sh` plus +`scripts/zz-planted-test.sh` appear in `checkers.sh --list`, and +`check-ci-graph.py` then exits 1 with *`zz-planted` is a checkers.sh suite that +no CI step runs; add it to ci.yml*. As a control, appending `zz-planted` to the +named step clears it (exit 0, "all 9 checkers.sh suites run in CI"). Both files +and the `ci.yml` edit were then removed, and the real run is back to 8 suites, +exit 0. + +**Seven self-test suite lists.** They cover: a suite no step runs; suites named +across several steps (one inside `nix shell … --command`); a bare run covering +everything; `--self-test` covering `checkers` and nothing else; a bare mention +in a YAML comment, which must *not* count as a bare run; names continued onto +the next line with a backslash; and a step naming a suite that does not exist. + +**Rivals, each run against `--self-test`:** +- **YAML comments no longer skipped:** exactly the comment case fails, because + the bare mention there reads as "everything runs" and the missing suite + passes. +- **`suites_run` always answering "everything"** (the vacuous shape): five + cases fail, every one that expects a suite to be named. + +The script was restored after each (`cmp` against a saved copy), and the +self-test is back to "5 graphs and 7 suite lists judged". diff --git a/scripts/check-ci-graph.py b/scripts/check-ci-graph.py index c3fc9911..bc17019f 100755 --- a/scripts/check-ci-graph.py +++ b/scripts/check-ci-graph.py @@ -6,10 +6,15 @@ README name three of seven while the rest read as ungated (improvise-x7cq). Stating it was not enough -- a new job was added during review of PR #83 and nothing noticed, so the rule is a gate. See context/code/tooling.md. +Second rule: every suite `scripts/checkers.sh --list` knows must run in some +CI step. ci.yml names its suites by hand, so a new `*-test.sh` ran locally and +never in CI until someone remembered to add it (improvise-dak5). + `--self-test` checks the checker: each case is a graph this must refuse. """ import re +import subprocess import sys WORKFLOW = ".github/workflows/ci.yml" @@ -60,6 +65,95 @@ def verdict(jobs, needs): return out +_CHECKERS = re.compile(r"(?:^|[\s/])scripts/checkers\.sh\b(.*)$") +ALL = object() + + +def suites_run(text): + """The suite names CI runs, or ALL when some step runs checkers.sh bare. + + A YAML comment is skipped: a bare mention there must not read as "every + suite runs". A trailing backslash continues onto the next line. + """ + lines = text.split("\n") + names, i = set(), 0 + while i < len(lines): + raw = lines[i] + i += 1 + if raw.lstrip().startswith("#"): + continue + m = _CHECKERS.search(raw) + if not m: + continue + rest = m.group(1) + while rest.rstrip().endswith("\\") and i < len(lines): + rest = rest.rstrip()[:-1] + " " + lines[i] + i += 1 + rest = re.split(r"&&|\|\||[;|#]", rest)[0] + args = rest.split() + if "--self-test" in args: + names.add("checkers") + picked = [a for a in args if not a.startswith("-")] + if not picked and "--self-test" not in args: + return ALL + names.update(picked) + return names + + +def suite_verdict(suites, ran): + """The complaints about suites CI never runs, or runs by a wrong name.""" + if ran is ALL: + return [] + out = [f"`{s}` is a checkers.sh suite that no CI step runs; add it to ci.yml" + for s in sorted(set(suites) - ran)] + out += [f"ci.yml runs checkers.sh `{s}`, which is not a suite" + for s in sorted(ran - set(suites))] + return out + + +def list_suites(): + """The suites, from checkers.sh itself, so its naming rule has one home.""" + out = subprocess.run(["scripts/checkers.sh", "--list"], capture_output=True, + text=True, check=True).stdout + return [s for s in out.split("\n") if s.strip()] + + +# (name, suites checkers.sh knows, workflow text, the complaints it must draw). +SUITE_SELF_TEST = [ + ("a suite no step runs is named", + ["kb", "zz-new"], " run: ./scripts/checkers.sh kb\n", + ["`zz-new` is a checkers.sh suite that no CI step runs; add it to ci.yml"]), + + ("every suite named across steps is accepted", + ["kb", "pr-watch", "checkers"], + " run: nix shell x --command ./scripts/checkers.sh kb\n" + " run: ./scripts/checkers.sh --self-test\n" + " run: ./scripts/checkers.sh pr-watch\n", []), + + ("a bare run covers every suite", + ["kb", "zz-new"], " run: ./scripts/checkers.sh\n", []), + + ("--self-test covers the checkers suite and nothing else", + ["checkers", "kb"], " run: ./scripts/checkers.sh --self-test\n", + ["`kb` is a checkers.sh suite that no CI step runs; add it to ci.yml"]), + + ("a bare mention in a comment does not count as running everything", + ["kb", "zz-new"], + " # Every suite goes through scripts/checkers.sh\n" + " run: ./scripts/checkers.sh kb\n", + ["`zz-new` is a checkers.sh suite that no CI step runs; add it to ci.yml"]), + + ("names continued onto the next line count, and are not a bare run", + ["kb", "pr-watch", "zz-new"], + " run: ./scripts/checkers.sh \\\n kb pr-watch\n", + ["`zz-new` is a checkers.sh suite that no CI step runs; add it to ci.yml"]), + + ("a step naming a suite that does not exist is caught", + ["kb"], " run: ./scripts/checkers.sh kb ghost\n", + ["ci.yml runs checkers.sh `ghost`, which is not a suite"]), +] + + # (name, workflow text, the complaints it must draw). SELF_TEST = [ ("a job nobody gates is named", @@ -95,28 +189,40 @@ def self_test(): got = verdict(jobs, needs) if got != want: failures.append(f" {name}: expected {want}, saw {got}") + for name, suites, text, want in SUITE_SELF_TEST: + got = suite_verdict(suites, suites_run(text)) + if got != want: + failures.append(f" {name}: expected {want}, saw {got}") if failures: print("ci-graph self-test FAILED:", file=sys.stderr) print("\n".join(failures), file=sys.stderr) return 1 - print(f"ci-graph self-test: {len(SELF_TEST)} graphs judged") + print(f"ci-graph self-test: {len(SELF_TEST)} graphs and " + f"{len(SUITE_SELF_TEST)} suite lists judged") return 0 def main(): if "--self-test" in sys.argv[1:]: return self_test() - jobs, needs = read_graph(open(WORKFLOW, encoding="utf8").read()) + text = open(WORKFLOW, encoding="utf8").read() + jobs, needs = read_graph(text) if len(jobs) < 2: print(f"error: {WORKFLOW}: found {len(jobs)} jobs; the parse is wrong", file=sys.stderr) return 2 - complaints = verdict(jobs, needs) + suites = list_suites() + if not suites: + print("error: checkers.sh --list named no suites; the listing is wrong", + file=sys.stderr) + return 2 + complaints = verdict(jobs, needs) + suite_verdict(suites, suites_run(text)) if complaints: for c in complaints: print(f"ci-graph: {c}", file=sys.stderr) return 1 - print(f"ci-graph: {DEPLOY} needs all {len(jobs) - 1} other jobs") + print(f"ci-graph: {DEPLOY} needs all {len(jobs) - 1} other jobs; " + f"all {len(suites)} checkers.sh suites run in CI") return 0 From c1417359776ba212fa6e987f58ac8a8c61e64ff1 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Wed, 23 Sep 2026 08:36:13 -0700 Subject: [PATCH 02/12] tooling: check-ci-graph asks checkers.sh what a step runs (improvise-maax) The #228 review and Codex's P1 found the suite check failing open. A bare scripts/checkers.sh after an inline comment, in a step name or as echo's argument read as "every suite runs". --list alone read as every suite, and --self-test's discarded names, and --list's, counted as run. checkers.sh gains --would-run, honoured only as the first argument. It reports what the rest of the arguments would run, from the real argument loop and name resolution. check-ci-graph.py only finds invocations: in run: text, comments stripped by bash's rule, command position, every command on the line, quotes removed. It then asks --would-run about each. An unparseable line contributes nothing and fails closed. 16 suite self-test cases, 8 of which the old parser gets wrong. Six rivals each fail a case only they can. checkers-test.sh gains four --would-run cases. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/code/tooling.md | 22 +++- context/test-evidence/tooling.md | 59 +++++++++ scripts/check-ci-graph.py | 215 +++++++++++++++++++++++-------- scripts/checkers-test.sh | 29 +++++ scripts/checkers.sh | 30 +++-- 5 files changed, 287 insertions(+), 68 deletions(-) diff --git a/context/code/tooling.md b/context/code/tooling.md index 37c5af36..5f1aa084 100644 --- a/context/code/tooling.md +++ b/context/code/tooling.md @@ -264,11 +264,23 @@ It holds a second rule of the same kind: every suite `scripts/checkers.sh suites by hand, because they need different jobs and environments (`kb` runs under a nix trealla, `node-test` in the `js` job), so a single bare `checkers.sh` was not an option. The check does not re-derive what a suite is. -It asks `checkers.sh --list`, so the naming rule keeps its one home. It then -reads every `checkers.sh` invocation in the workflow. A bare one covers every -suite, and `--self-test` covers `checkers`. A YAML comment is skipped, because a -bare mention there would otherwise read as "everything runs", which is the -failure that looks like success. +It asks `checkers.sh --list`, so the naming rule keeps its one home. + +**Nor does it re-derive what an invocation runs** (improvise-maax). The first +version did, and got `checkers.sh`'s argument rules wrong in the direction that +passes. It read `--list` as "run everything", when `--list` runs nothing. It +counted the names after `--self-test`, which `checkers.sh` throws away. And it +read any mention of `scripts/checkers.sh` as a command: in a trailing comment, +a step `name:`, or `echo`'s argument. Now `checkers.sh --would-run ` +answers what `` would run, from the same argument loop and name +resolution the real run uses. `--would-run` is honoured only as the first +argument, so a CI step that spells it anywhere is a usage error rather than a +green step that ran nothing. The check itself only finds invocations. It reads +the shell text of `run:` values alone, strips a comment by bash's rule (a `#` +at the start of a word, outside quotes, so `nixpkgs#trealla` survives), splits +at `;`, `&&` and `|`, and counts a command only when `checkers.sh` is its +command word or what `nix shell … --command` runs. A line `shlex` cannot parse +contributes nothing, which fails closed. ### pr-watch and mergeability (improvise-edpl, improvise-zlj2) diff --git a/context/test-evidence/tooling.md b/context/test-evidence/tooling.md index 8d321b3a..c05731ec 100644 --- a/context/test-evidence/tooling.md +++ b/context/test-evidence/tooling.md @@ -841,3 +841,62 @@ the next line with a backslash; and a step naming a suite that does not exist. The script was restored after each (`cmp` against a saved copy), and the self-test is back to "5 graphs and 7 suite lists judged". + +### The suite check asks checkers.sh what a step runs — `scripts/check-ci-graph.py` (improvise-maax) + +**What broke.** The adversarial review of #228 at `cee61b94`, and ChatGPT +Codex's P1 on it, found the first version failing open in four shapes. Each let +the check pass while a suite never ran: +- a bare `scripts/checkers.sh` outside a command read as "every suite runs": + after an inline `#` (Codex), in a step `name:`, or as `echo`'s argument; +- `checkers.sh --list` alone read as every suite, though `--list` runs nothing; +- `--self-test zz-new kb` counted `zz-new` and `kb`, though `--self-test` + discards them; +- `--list kb zz-new` counted both. + +Two more were loud but wrong: only the first invocation on a line counted, and +quoted names kept their quotes. + +**The fix moves the argument rules back into `checkers.sh`.** It gains +`--would-run`, which is honoured only as the first argument and reports rather +than runs. `check-ci-graph.py` finds invocations: `run:` text only, comments +stripped by bash's rule, command position only. It then asks `--would-run` +about each one. + +**Every new case run against the OLD parser** (`/tmp/ccg-old.py`, the file at +`cee61b94`): eight of the sixteen suite cases give the wrong answer there. The +five false passes are `--list` alone, the inline comment (Codex), the non-run +block scalar, the step `name:` and `echo`'s argument. Wrong in a noisier way: +`--self-test zz-new kb` (the old parser counts `kb`, so it reports only a +bogus "zz-new is not a suite"), `--list kb zz-new` (the same), and `a && b` +(the second suite is missed). + +**A vacuity fixed before the rivals ran.** As first written, the bare-mention +cases required `zz-new`, which no real `checkers.sh` ever runs. A false bare +run then still left exactly `zz-new` unrun, the expected answer, so the +any-word-position rival passed all sixteen. The cases now require `kb` and +`pr-watch`, and the real step runs only `kb`. A false bare run is then the only +way `pr-watch` counts as run. + +**Rivals, one function swapped in at a time and each run against the real +`--self-test`:** +- **no comment stripping:** fails only "a suite named inside a trailing shell + comment is not run"; +- **every line scanned** (run: prefixes still recognised): fails "a + command-shaped line in a non-run block scalar" and the block-run case; +- **`checkers.sh` accepted in any word position:** fails only "a bare mention + as echo's argument"; +- **argv modelled naively** (names, else everything): fails the `--self-test` + case and both `--list` cases; +- **first invocation per line only:** fails "every invocation on a line + counts" and the block-run case; +- **`--would-run` honoured in any position**, in `checkers.sh` itself: fails + `checkers-test.sh`'s "--would-run anywhere but first is a usage error" (exit + 0, wanted 4) and the parser's "a step spelling --would-run runs nothing". + +One more rival crashed instead: an early every-line rival hit `shlex`'s +"No escaped character" on a lone trailing backslash. So a line that cannot be +parsed now contributes no invocation, which fails closed. Everything was +restored (`cmp` for `checkers.sh`) and re-run: `--self-test` reports 16 suite +lists and `checkers-test.sh` 17/17. A planted `zz-planted` suite still fails +the real run, naming it. diff --git a/scripts/check-ci-graph.py b/scripts/check-ci-graph.py index bc17019f..b70048eb 100755 --- a/scripts/check-ci-graph.py +++ b/scripts/check-ci-graph.py @@ -8,12 +8,14 @@ nothing noticed, so the rule is a gate. See context/code/tooling.md. Second rule: every suite `scripts/checkers.sh --list` knows must run in some CI step. ci.yml names its suites by hand, so a new `*-test.sh` ran locally and -never in CI until someone remembered to add it (improvise-dak5). +never in CI until someone remembered to add it (improvise-dak5). What a step +runs is asked of `checkers.sh --would-run`, never re-derived (improvise-maax). `--self-test` checks the checker: each case is a graph this must refuse. """ import re +import shlex import subprocess import sys @@ -65,49 +67,116 @@ def verdict(jobs, needs): return out -_CHECKERS = re.compile(r"(?:^|[\s/])scripts/checkers\.sh\b(.*)$") -ALL = object() +_RUN = re.compile(r"^(\s*)(?:-\s+)?run:\s?(.*)$") +_ASSIGN = re.compile(r"^[A-Za-z_][A-Za-z0-9_]*=") +_OPERATOR = set(";&|()") + + +def run_scripts(text): + """The shell text of every `run:` value. Nothing else in ci.yml is a + command, so a step `name:` or a YAML comment never reads as one.""" + lines, out, i = text.split("\n"), [], 0 + while i < len(lines): + m = _RUN.match(lines[i]) + i += 1 + if not m: + continue + indent, value = len(m.group(1)), m.group(2).strip() + if value[:1] not in ("|", ">"): + out.append(value) + continue + body = [] + while i < len(lines) and (not lines[i].strip() or + len(lines[i]) - len(lines[i].lstrip()) > indent): + body.append(lines[i]) + i += 1 + out.append("\n".join(body)) + return out + + +def strip_comment(line): + """Bash's rule: `#` opens a comment only at the start of a word and outside + quotes, so `nixpkgs#trealla` survives and `echo x # checkers.sh` does not.""" + quote, prev = None, " " + for j, c in enumerate(line): + if quote: + if c == quote: + quote = None + elif c in "'\"": + quote = c + elif c == "#" and (prev.isspace() or prev in _OPERATOR): + return line[:j] + prev = c + return line + + +def simple_commands(script): + """Each simple command in a script, as its words with quotes removed.""" + for line in script.replace("\\\n", " ").split("\n"): + lexer = shlex.shlex(strip_comment(line), posix=True, punctuation_chars=True) + lexer.whitespace_split = True + lexer.commenters = "" + try: + tokens = list(lexer) + except ValueError: + continue # unparseable: no invocation, so it fails closed + words = [] + for tok in tokens: + if set(tok) <= _OPERATOR: + if words: + yield words + words = [] + else: + words.append(tok) + if words: + yield words + + +def checkers_invocations(text): + """The arguments of every command that IS checkers.sh: the command word + after any VAR=value, or the program `nix shell ... --command` runs.""" + for script in run_scripts(text): + for words in simple_commands(script): + k = 0 + while k < len(words) and _ASSIGN.match(words[k]): + k += 1 + if "--command" in words[k:]: + k = words.index("--command", k) + 1 + if k < len(words) and words[k].endswith("scripts/checkers.sh"): + yield words[k + 1:] + + +def would_run(args): + """What checkers.sh itself says these arguments run. Its argument rules are + not restated here: --list and --self-test mean what the script makes them.""" + out = subprocess.run(["scripts/checkers.sh", "--would-run", *args], + capture_output=True, text=True).stdout + ran, missing = set(), set() + for line in out.split("\n"): + if line.startswith("would-run: "): + ran.add(line[len("would-run: "):]) + elif line.startswith("missing: "): + missing.add(line[len("missing: "):]) + return ran, missing def suites_run(text): - """The suite names CI runs, or ALL when some step runs checkers.sh bare. - - A YAML comment is skipped: a bare mention there must not read as "every - suite runs". A trailing backslash continues onto the next line. - """ - lines = text.split("\n") - names, i = set(), 0 - while i < len(lines): - raw = lines[i] - i += 1 - if raw.lstrip().startswith("#"): - continue - m = _CHECKERS.search(raw) - if not m: - continue - rest = m.group(1) - while rest.rstrip().endswith("\\") and i < len(lines): - rest = rest.rstrip()[:-1] + " " + lines[i] - i += 1 - rest = re.split(r"&&|\|\||[;|#]", rest)[0] - args = rest.split() - if "--self-test" in args: - names.add("checkers") - picked = [a for a in args if not a.startswith("-")] - if not picked and "--self-test" not in args: - return ALL - names.update(picked) - return names + """(suites some step runs, names a step asks for that are not suites).""" + ran, missing = set(), set() + for args in checkers_invocations(text): + r, m = would_run(args) + ran |= r + missing |= m + return ran, missing -def suite_verdict(suites, ran): - """The complaints about suites CI never runs, or runs by a wrong name.""" - if ran is ALL: - return [] +def suite_verdict(suites, run): + """The complaints about suites CI never runs, or asks for by a wrong name.""" + ran, missing = run out = [f"`{s}` is a checkers.sh suite that no CI step runs; add it to ci.yml" for s in sorted(set(suites) - ran)] out += [f"ci.yml runs checkers.sh `{s}`, which is not a suite" - for s in sorted(ran - set(suites))] + for s in sorted(missing)] return out @@ -118,35 +187,71 @@ def list_suites(): return [s for s in out.split("\n") if s.strip()] -# (name, suites checkers.sh knows, workflow text, the complaints it must draw). +# (name, suites to require, workflow text, the complaints it must draw). Every +# case asks the REAL checkers.sh. A false "bare run" must be the ONLY way pr-watch runs. +_UNRUN = "`zz-new` is a checkers.sh suite that no CI step runs; add it to ci.yml" +_NO_PW = "`pr-watch` is a checkers.sh suite that no CI step runs; add it to ci.yml" +_KB_PW = ["kb", "pr-watch", "zz-new"] +_REQ = ["kb", "pr-watch"] +_ONLY_KB = " run: ./scripts/checkers.sh kb\n" SUITE_SELF_TEST = [ ("a suite no step runs is named", - ["kb", "zz-new"], " run: ./scripts/checkers.sh kb\n", - ["`zz-new` is a checkers.sh suite that no CI step runs; add it to ci.yml"]), + _KB_PW, " run: ./scripts/checkers.sh kb pr-watch\n", [_UNRUN]), - ("every suite named across steps is accepted", + ("suites named across steps, one inside nix shell --command, are accepted", ["kb", "pr-watch", "checkers"], - " run: nix shell x --command ./scripts/checkers.sh kb\n" + " run: nix shell nixpkgs#trealla --command ./scripts/checkers.sh kb\n" " run: ./scripts/checkers.sh --self-test\n" - " run: ./scripts/checkers.sh pr-watch\n", []), + " - run: ./scripts/checkers.sh pr-watch\n", []), - ("a bare run covers every suite", - ["kb", "zz-new"], " run: ./scripts/checkers.sh\n", []), + ("a bare run covers every real suite", + ["kb", "pr-watch", "checkers"], " run: ./scripts/checkers.sh\n", []), - ("--self-test covers the checkers suite and nothing else", - ["checkers", "kb"], " run: ./scripts/checkers.sh --self-test\n", + ("--self-test runs checkers and DISCARDS the names beside it", + ["checkers", "kb"], " run: ./scripts/checkers.sh --self-test zz-new kb\n", ["`kb` is a checkers.sh suite that no CI step runs; add it to ci.yml"]), - ("a bare mention in a comment does not count as running everything", - ["kb", "zz-new"], - " # Every suite goes through scripts/checkers.sh\n" - " run: ./scripts/checkers.sh kb\n", - ["`zz-new` is a checkers.sh suite that no CI step runs; add it to ci.yml"]), + ("--list alone runs nothing", + ["kb"], " run: ./scripts/checkers.sh --list\n", + ["`kb` is a checkers.sh suite that no CI step runs; add it to ci.yml"]), - ("names continued onto the next line count, and are not a bare run", - ["kb", "pr-watch", "zz-new"], - " run: ./scripts/checkers.sh \\\n kb pr-watch\n", - ["`zz-new` is a checkers.sh suite that no CI step runs; add it to ci.yml"]), + ("--list with names runs nothing either", + ["kb"], " run: ./scripts/checkers.sh --list kb zz-new\n", + ["`kb` is a checkers.sh suite that no CI step runs; add it to ci.yml"]), + + ("a bare mention after an inline shell comment is not a run (Codex, #228)", + _REQ, " run: echo skipped # ./scripts/checkers.sh\n" + _ONLY_KB, [_NO_PW]), + + ("a suite named inside a trailing shell comment is not run", + _REQ, " run: ./scripts/checkers.sh kb # pr-watch\n", [_NO_PW]), + + ("a command-shaped line in a non-run block scalar is not a run", + _REQ, + " - name: notes\n with:\n notes: |\n" + " ./scripts/checkers.sh\n " + _ONLY_KB, [_NO_PW]), + + ("a bare mention in a step name is not a run", + _REQ, " - name: Runs scripts/checkers.sh\n " + _ONLY_KB, [_NO_PW]), + + ("a bare mention as echo's argument is not a run", + _REQ, " run: echo scripts/checkers.sh\n" + _ONLY_KB, [_NO_PW]), + + ("a bare mention in a YAML comment is not a run", + _REQ, " # Every suite goes through scripts/checkers.sh\n" + _ONLY_KB, + [_NO_PW]), + + ("every invocation on a line counts, and quotes are removed", + _REQ, + " run: ./scripts/checkers.sh kb && ./scripts/checkers.sh 'pr-watch'\n", []), + + ("a block run is read, continuation lines included", + _REQ, + " run: |\n set -e\n ./scripts/checkers.sh \\\n" + " kb pr-watch\n - name: next\n", []), + + ("a step spelling --would-run runs nothing, wherever it puts it", + _REQ, _ONLY_KB + " run: ./scripts/checkers.sh pr-watch --would-run\n" + " run: ./scripts/checkers.sh --would-run pr-watch\n", [_NO_PW]), ("a step naming a suite that does not exist is caught", ["kb"], " run: ./scripts/checkers.sh kb ghost\n", diff --git a/scripts/checkers-test.sh b/scripts/checkers-test.sh index 01995e07..6c9b8c37 100755 --- a/scripts/checkers-test.sh +++ b/scripts/checkers-test.sh @@ -176,5 +176,34 @@ listed="$(tr '\n' ' ' < "$work/out")" && ok "an X-test.sh with no X beside it is not a suite" \ || bad "an X-test.sh with no X beside it is not a suite" "got [$listed]" +# --------------------------------------------------------------------------- +# 8. --would-run reports what the rest of the arguments run, and runs nothing. +# check-ci-graph.py asks it, so each argument rule below is answered HERE. +d="$(sandbox wouldrun)" +for n in alpha beta; do + suite "$d" "$n" < "$work/out" 2>&1 +got="$(tr '\n' ' ' < "$work/out")" +[ "$got" = "would-run: alpha would-run: beta " ] && [ ! -e "$work/ran-alpha" ] \ + && ok "--would-run with no names reports every suite and runs none" \ + || bad "--would-run with no names reports every suite and runs none" "got [$got]" +"$d/checkers.sh" --would-run --self-test alpha > "$work/out" 2>&1 +got="$(tr '\n' ' ' < "$work/out")" +[ "$got" = "missing: checkers " ] \ + && ok "--would-run --self-test reports checkers and discards the names" \ + || bad "--would-run --self-test reports checkers and discards the names" "got [$got]" +"$d/checkers.sh" --would-run --list alpha > "$work/out" 2>&1 +grep -q '^would-run:' "$work/out" \ + && bad "--would-run --list reports nothing as run" "$(tr '\n' ' ' < "$work/out")" \ + || ok "--would-run --list reports nothing as run" +expect "--would-run anywhere but first is a usage error" 4 "$d/checkers.sh" alpha --would-run +[ ! -e "$work/ran-alpha" ] && ok "and it runs nothing" \ + || bad "and it runs nothing" "alpha ran" + printf '\ncheckers-test: %d passed, %d failed\n' "$pass" "$fail" [ "$fail" -eq 0 ] diff --git a/scripts/checkers.sh b/scripts/checkers.sh index f598cfdc..49b5c06a 100755 --- a/scripts/checkers.sh +++ b/scripts/checkers.sh @@ -27,6 +27,10 @@ usage: checkers.sh [--list] [--self-test] [name ...] --list print the suite names and exit --self-test run scripts/checkers-test.sh, which injects the faults this runner exists to catch + --would-run as the FIRST argument only: run nothing, and print + `would-run: ` (or `missing: `) for each suite + the rest of the arguments would run. check-ci-graph.py + asks this, so the argument rules have one home exit 0 every suite passed exit 1 a suite failed, or a shebang is not `#!/usr/bin/env bash` @@ -107,8 +111,10 @@ run_suite() { } main() { - local self_test=0 + local self_test=0 would_run=0 local -a names=() + # Only as the FIRST argument, so a CI step spelling it anywhere is an error. + if [ "${1:-}" = --would-run ]; then would_run=1; shift; fi while [ $# -gt 0 ]; do case "$1" in --list) suites; return 0 ;; @@ -120,6 +126,21 @@ main() { shift done + if [ "$self_test" = 1 ]; then + names=(checkers) + elif [ ${#names[@]} -eq 0 ]; then + while IFS= read -r line; do names+=("$line"); done < <(suites) + fi + + local n file + if [ "$would_run" = 1 ]; then + for n in ${names[@]+"${names[@]}"}; do + if [ -f "$HERE/$n-test.sh" ]; then printf 'would-run: %s\n' "$n" + else printf 'missing: %s\n' "$n"; fi + done + return 0 + fi + local pinned; pinned="$(find_modern_bash)" if [ -z "$pinned" ]; then echo 'checkers: no bash >= 5 on PATH and nix cannot supply one.' >&2 @@ -132,13 +153,6 @@ main() { local rc=0 check_shebangs || rc=1 - if [ "$self_test" = 1 ]; then - names=(checkers) - elif [ ${#names[@]} -eq 0 ]; then - while IFS= read -r line; do names+=("$line"); done < <(suites) - fi - - local n file for n in ${names[@]+"${names[@]}"}; do file="$HERE/$n-test.sh" if [ ! -f "$file" ]; then From d6706774a22e8c63fc4a7b6f1a872c6fd12793ea Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Wed, 23 Sep 2026 08:59:18 -0700 Subject: [PATCH 03/12] tooling: CI runs checkers.sh --ci ; the gate reads a table, not shell improvise-st8q, improvise-tvte, from the #228 re-review at c1417359. The maax parser still failed open. A bare checkers.sh inside a heredoc or a folded run: value counted as every suite, and step and job if: were ignored. Every round of parsing met another shell or YAML form, so the parsing is gone. checkers.sh holds a job-to-suites table (ci_suites), and each CI job makes one call, `checkers.sh --ci `. check-ci-graph.py counts a step only when all of these hold: it sits inside steps: at step-key indentation, its whole run: value is that call, the call names its own job, and no if:, continue-on-error:, shell: or defaults: covers it. Every other mention of scripts/checkers.sh is reported. A key with an inline value owns deeper lines, and an open quote owns lines until it closes, so heredocs, folds and quoted values are content, never structure. There are 19 suite self-test cases and 7 rivals, each failing a case only it can. The reviewer's heredoc, fold and if: attacks on the real ci.yml now exit 1. checkers-test.sh gains five --ci cases. context/code/tooling.md corrects two overstated claims ("--would-run anywhere is a usage error", "fails closed"). Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- .github/workflows/ci.yml | 29 +-- context/code/tooling.md | 59 +++-- context/test-evidence/tooling.md | 62 ++++++ scripts/check-ci-graph.py | 367 +++++++++++++++++++------------ scripts/checkers-test.sh | 13 ++ scripts/checkers.sh | 29 ++- 6 files changed, 375 insertions(+), 184 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 4646ac2d..0e1b9526 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -43,7 +43,7 @@ jobs: # assembly still exited 0 (improvise-w8ih). It guards the assemble step # below, so it fails under this job's name rather than another's. - name: The stamp gate refuses what it claims to - run: ./scripts/checkers.sh assemble-stamp + run: ./scripts/checkers.sh --ci build - uses: dtolnay/rust-toolchain@stable with: targets: wasm32-unknown-unknown @@ -248,7 +248,7 @@ jobs: # The guard above is only worth its line if its refusals are themselves # checked -- the step it replaces was green while asserting nothing. - name: The node test runner refuses what it claims to - run: ./scripts/checkers.sh node-test + run: ./scripts/checkers.sh --ci js kb: name: Knowledge base consistency @@ -263,27 +263,12 @@ jobs: run: | nix shell nixpkgs#trealla --command tpl prolog-sessions/kb.pl \ -g "(kb_consistent -> halt(0) ; (kb_report, halt(1)))" - # scripts/kb is the only thing that writes kb.pl, so its refusals are as - # load-bearing as kb_consistent itself. Without this the replacement - # writer could regress green -- the review that found the directive - # bypass also found that nothing ran this suite. - - name: The kb tool refuses what it claims to - run: nix shell nixpkgs#trealla --command ./scripts/checkers.sh kb # Every suite goes through scripts/checkers.sh, which pins a bash >= 5 - # and scrubs the environment, so the shell and the variables a suite - # passes under here are the ones it passes under locally (improvise-f9qf). - - name: The checker runner refuses what it claims to - run: ./scripts/checkers.sh --self-test - # pr-watch is driven entirely from fixtures. The release preflight is the - # one guard whose absence is invisible until a tag is public and - # immutable. pr-reviews stands between a merge and an unread review, and - # was blind to inline comments for as long as it existed (improvise-xgra). - # fmt-workspaces is here because its own failure mode is the bug it - # exists for: a usage error wearing a formatting failure's exit code. - # bd-audit-ids is the dead-ID audit, whose recipe once read a bd that - # never ran as a clean audit (improvise-52eq). - - name: The remaining checkers refuse what they claim to - run: ./scripts/checkers.sh pr-watch release-preflight pr-reviews fmt-workspaces bd-audit-ids + # and scrubs the environment (improvise-f9qf). Which suites this job runs + # is the `kb` row of the CI table in checkers.sh, deliberately not listed + # here. check-ci-graph.py holds that table to `checkers.sh --list`. + - name: Every checker in scripts/ refuses what it claims to + run: nix shell nixpkgs#trealla --command ./scripts/checkers.sh --ci kb # --------------------------------------------------------------------------- # Lints the one crate the `test` job cannot reach. diff --git a/context/code/tooling.md b/context/code/tooling.md index 5f1aa084..c81e2f13 100644 --- a/context/code/tooling.md +++ b/context/code/tooling.md @@ -266,21 +266,50 @@ under a nix trealla, `node-test` in the `js` job), so a single bare `checkers.sh` was not an option. The check does not re-derive what a suite is. It asks `checkers.sh --list`, so the naming rule keeps its one home. -**Nor does it re-derive what an invocation runs** (improvise-maax). The first -version did, and got `checkers.sh`'s argument rules wrong in the direction that -passes. It read `--list` as "run everything", when `--list` runs nothing. It -counted the names after `--self-test`, which `checkers.sh` throws away. And it -read any mention of `scripts/checkers.sh` as a command: in a trailing comment, -a step `name:`, or `echo`'s argument. Now `checkers.sh --would-run ` -answers what `` would run, from the same argument loop and name -resolution the real run uses. `--would-run` is honoured only as the first -argument, so a CI step that spells it anywhere is a usage error rather than a -green step that ran nothing. The check itself only finds invocations. It reads -the shell text of `run:` values alone, strips a comment by bash's rule (a `#` -at the start of a word, outside quotes, so `nixpkgs#trealla` survives), splits -at `;`, `&&` and `|`, and counts a command only when `checkers.sh` is its -command word or what `nix shell … --command` runs. A line `shlex` cannot parse -contributes nothing, which fails closed. +**What CI runs is a table in `checkers.sh`, not something read out of shell** +(improvise-st8q, improvise-tvte). Two rounds of review each found the gate +passing while a suite never ran, and every finding was the gate +*interpreting* `ci.yml`. It read `--list` as "run everything". It counted the +names after `--self-test`. Then it read a bare `scripts/checkers.sh` inside a +heredoc, a folded `>` value or a multi-line string as a command, and it ignored +`if:`. Each fix added parsing, and shell and YAML always had another form. So +the parsing went. `checkers.sh` holds a job→suites table, `ci_suites`, and each +CI job has exactly one step whose whole `run:` value is +`./scripts/checkers.sh --ci `. A `nix shell --command` prefix is +allowed. `--ci` runs that row and nothing else, and `--ci` beside a suite name +or `--self-test` is a usage error. `checkers.sh --would-run --ci ` reports +the row without running it, so the table has one home. + +The gate reads no shell. It counts a step only when all of these hold: +- the step sits in a job's `steps:` sequence, at step-key indentation; +- its `run:` value is exactly that call; +- the call names its own job; +- neither the step nor the job carries `if:`, `continue-on-error:` or `shell:`; +- no `defaults:` block, at workflow or job level, covers it, since that can + change the shell the step runs under. + +The only YAML it models is what makes a line structure rather than content. A +key with an inline value owns every following line indented deeper, and an +open quote owns every line until it closes. So a heredoc, a folded or plain +continuation, a quoted value or a whole fake job inside a top-level block +scalar is skipped as content. Any line outside a YAML comment that mentions +`scripts/checkers.sh` and is not a counted step is **reported**, not ignored. +The union of the counted jobs' rows must cover `checkers.sh --list`. + +**What this does not close, stated rather than guarded.** Once a counted step +is reached, the gate trusts that it runs the script it names. An earlier step +in the same job that rewrote `scripts/checkers.sh` or its table would defeat +that. Shell control flow *around* the call cannot hide in the counted step, +because its value is exactly the call. But the gate cannot see a job-level +`needs:` whose failure skips the job, which is why `pages-deploy` must still +`needs:` every job. + +**Two claims earlier versions of this section made were wrong.** "`--would-run` +anywhere but first is a usage error" was false: `checkers.sh --list +--would-run` exits 0 at `--list` before it reaches the second flag. And "a line +`shlex` cannot parse fails closed" was true of that one line, but read as a +property of the gate, which at that point still passed on heredocs and folded +values. ### pr-watch and mergeability (improvise-edpl, improvise-zlj2) diff --git a/context/test-evidence/tooling.md b/context/test-evidence/tooling.md index ba9f01af..bc0da11b 100644 --- a/context/test-evidence/tooling.md +++ b/context/test-evidence/tooling.md @@ -937,3 +937,65 @@ parsed now contributes no invocation, which fails closed. Everything was restored (`cmp` for `checkers.sh`) and re-run: `--self-test` reports 16 suite lists and `checkers-test.sh` 17/17. A planted `zz-planted` suite still fails the real run, naming it. + +### The suite gate reads a table, not shell — `scripts/check-ci-graph.py` (improvise-st8q, improvise-tvte) + +**What broke, twice over.** The #228 re-review at `c1417359` showed the maax +parser still failing open. On the real `ci.yml`, two changes each turned +exit 1 into exit 0: +- a `GITHUB_STEP_SUMMARY` heredoc holding a bare `./scripts/checkers.sh`; +- folding the kb step as `run: >`. + +That is because `run:` values were read one line at a time. `"--command" in +words` also matched any command at all. And step and job `if:` were ignored +(tvte). Each round of parsing had been answered by another shell or YAML form. + +**The structural fix.** `checkers.sh` owns a job→suites table, and each CI job +makes one call, `checkers.sh --ci `. The gate parses no shell. It counts a +step only when all of these hold: +- the step sits at step-key indentation inside `steps:`; +- its whole `run:` value is that call; +- the call names its own job; +- the step carries no `if:`, `continue-on-error:`, `shell:` or covering + `defaults:`. + +Every other line mentioning `scripts/checkers.sh` is reported. A key with an +inline value owns deeper lines, and an open quote owns lines until it closes, +so content is never read as structure. + +**The reviewer's attacks replayed on the real `ci.yml`** (kb step mutated, +restored after each): +- the heredoc: exit 1, naming the line and the seven unrun suites; +- `run: >`: exit 1, the same; +- `if:` on the kb step: exit 1, "conditional (if)". + +**Self-test: 19 suite lists.** Every case asks the real `checkers.sh` table. + +**Rivals, each a source edit loaded and run against the real `--self-test`, +each failing a case only it can:** +- **No quote tracking:** fails "an exact run: line inside a multi-line quoted + value does not count" and nothing else. The first quoted case could not + isolate this rule, because its content line was not `run:`-shaped anyway. + It was replaced. +- **No owner rule:** fails "a whole job inside a top-level block scalar does not + count". +- **Disqualifiers ignored:** fails the step `if:`, job `if:`, job `if:` after + its steps, `continue-on-error` and both `defaults:` cases. +- **Any job may run any row:** fails "a job running another job's row is + refused". +- **The call may be embedded in other shell:** fails "shell control flow around + the call does not count". +- **Stray mentions not reported:** fails the nine cases that expect a line to + be named. +- **Steps counted outside `steps:`:** fails "a step-shaped item under a job key + other than steps: does not count". The first form of this rival passed, + because it changed only where a step starts while the step-key match still + required `steps:`. The rival was rewritten to treat every job key with an + empty value as opening steps. + +**`checkers-test.sh` gains five `--ci` cases.** The rival that drops "`--ci` +takes a job and nothing else" fails two of them, `--ci` beside a suite name +and beside `--self-test`, each exit 0 where 4 was wanted. + +**Stated, not guarded:** a step earlier in the same job that rewrites +`scripts/checkers.sh` or its table (`context/code/tooling.md`). diff --git a/scripts/check-ci-graph.py b/scripts/check-ci-graph.py index b70048eb..bdbc57b4 100755 --- a/scripts/check-ci-graph.py +++ b/scripts/check-ci-graph.py @@ -8,14 +8,14 @@ nothing noticed, so the rule is a gate. See context/code/tooling.md. Second rule: every suite `scripts/checkers.sh --list` knows must run in some CI step. ci.yml names its suites by hand, so a new `*-test.sh` ran locally and -never in CI until someone remembered to add it (improvise-dak5). What a step -runs is asked of `checkers.sh --would-run`, never re-derived (improvise-maax). +never in CI until someone remembered to add it (improvise-dak5). Each job runs +`checkers.sh --ci `, whose table lives in checkers.sh; no shell is parsed +here, and what a row runs is asked of `--would-run` (improvise-maax, st8q). `--self-test` checks the checker: each case is a graph this must refuse. """ import re -import shlex import subprocess import sys @@ -67,88 +67,113 @@ def verdict(jobs, needs): return out -_RUN = re.compile(r"^(\s*)(?:-\s+)?run:\s?(.*)$") -_ASSIGN = re.compile(r"^[A-Za-z_][A-Za-z0-9_]*=") -_OPERATOR = set(";&|()") +_JOB_KEY = re.compile(r"^ ([A-Za-z_][\w-]*):(.*)$") +_STEP_START = re.compile(r"^ - ") +_STEP_KEY = re.compile(r"^ (?:- | )([A-Za-z_][\w-]*):(.*)$") +_ANY_KEY = re.compile(r"^(\s*)(?:- )?([A-Za-z_][\w-]*):(.*)$") +_CI_CALL = re.compile(r"^(?:nix shell \S+ --command )?\./scripts/checkers\.sh --ci ([\w-]+)$") +_DISQUALIFY = ("if", "continue-on-error", "shell") +_JOB_DISQUALIFY = _DISQUALIFY + ("defaults",) -def run_scripts(text): - """The shell text of every `run:` value. Nothing else in ci.yml is a - command, so a step `name:` or a YAML comment never reads as one.""" - lines, out, i = text.split("\n"), [], 0 - while i < len(lines): - m = _RUN.match(lines[i]) - i += 1 - if not m: - continue - indent, value = len(m.group(1)), m.group(2).strip() - if value[:1] not in ("|", ">"): - out.append(value) - continue - body = [] - while i < len(lines) and (not lines[i].strip() or - len(lines[i]) - len(lines[i].lstrip()) > indent): - body.append(lines[i]) - i += 1 - out.append("\n".join(body)) - return out +def _inline_value(rest): + """A key's inline value, without a YAML comment (`#` after whitespace).""" + return re.sub(r"\s+#.*$", "", rest).strip() -def strip_comment(line): - """Bash's rule: `#` opens a comment only at the start of a word and outside - quotes, so `nixpkgs#trealla` survives and `echo x # checkers.sh` does not.""" - quote, prev = None, " " - for j, c in enumerate(line): - if quote: - if c == quote: +def _opens_quote(value): + """True when a quoted inline value is not closed on its own line.""" + if value[:1] not in ("'", '"') or len(value) < 2: + return value[:1] in ("'", '"') + q, body = value[0], value[1:] + if q == "'": + return not body.replace("''", "").endswith("'") + return not re.sub(r'\\.', "", body).endswith('"') + + +def ci_steps(text): + """(job, value, disqualifiers) for every step of every job, and every line + mentioning scripts/checkers.sh that is not a step's whole `run:` value. + + Nothing is read as shell. A key with an inline value owns every following + line indented deeper, which is how a block scalar, a folded value, a plain + continuation or a heredoc inside one is skipped whole (improvise-st8q).""" + steps, stray = [], [] + job, in_steps, job_flags, step = None, False, set(), None + workflow_flags = set() + owner, quote = None, None + + def close_step(): + if step is not None: + steps.append(step) + + for n, raw in enumerate(text.split("\n"), 1): + indent = len(raw) - len(raw.lstrip()) + if quote is not None: + if quote in raw: quote = None - elif c in "'\"": - quote = c - elif c == "#" and (prev.isspace() or prev in _OPERATOR): - return line[:j] - prev = c - return line - - -def simple_commands(script): - """Each simple command in a script, as its words with quotes removed.""" - for line in script.replace("\\\n", " ").split("\n"): - lexer = shlex.shlex(strip_comment(line), posix=True, punctuation_chars=True) - lexer.whitespace_split = True - lexer.commenters = "" - try: - tokens = list(lexer) - except ValueError: - continue # unparseable: no invocation, so it fails closed - words = [] - for tok in tokens: - if set(tok) <= _OPERATOR: - if words: - yield words - words = [] + if "scripts/checkers.sh" in raw: + stray.append(n) + continue + if owner is not None and (not raw.strip() or indent > owner): + if "scripts/checkers.sh" in raw: + stray.append(n) + continue + owner = None + if not raw.strip() or raw.lstrip().startswith("#"): + continue + if raw.startswith("defaults:"): + workflow_flags.add("workflow defaults") + m = _JOB.match(raw) + if m: + close_step() + step = None + job, in_steps, job_flags = m.group(1), False, set() + continue + m = _JOB_KEY.match(raw) + if m and job is not None: + close_step() + step = None + key, value = m.group(1), _inline_value(m.group(2)) + in_steps = key == "steps" and value == "" + if key in _JOB_DISQUALIFY: + job_flags.add(key) + for s in steps: + if s["job"] == job: + s["flags"].add("job " + key) + if in_steps and _STEP_START.match(raw): + close_step() + step = {"job": job, "run": None, "flags": {"job " + f for f in job_flags}, + "line": n} + m = _STEP_KEY.match(raw) if in_steps and step is not None else None + if m: + key, value = m.group(1), _inline_value(m.group(2)) + if key == "run": + step["run"] = value + if "scripts/checkers.sh" in raw and not _CI_CALL.match(value): + stray.append(n) + elif key in _DISQUALIFY: + step["flags"].add(key) + elif "scripts/checkers.sh" in raw: + stray.append(n) + elif "scripts/checkers.sh" in raw: + stray.append(n) + k = _ANY_KEY.match(raw) + if k and _inline_value(k.group(3)): + value = _inline_value(k.group(3)) + if _opens_quote(value): + quote = value[0] else: - words.append(tok) - if words: - yield words - - -def checkers_invocations(text): - """The arguments of every command that IS checkers.sh: the command word - after any VAR=value, or the program `nix shell ... --command` runs.""" - for script in run_scripts(text): - for words in simple_commands(script): - k = 0 - while k < len(words) and _ASSIGN.match(words[k]): - k += 1 - if "--command" in words[k:]: - k = words.index("--command", k) + 1 - if k < len(words) and words[k].endswith("scripts/checkers.sh"): - yield words[k + 1:] + owner = len(k.group(1)) + (2 if raw.lstrip().startswith("- ") else 0) + close_step() + for s in steps: + s["flags"] |= workflow_flags + return steps, stray def would_run(args): - """What checkers.sh itself says these arguments run. Its argument rules are - not restated here: --list and --self-test mean what the script makes them.""" + """What checkers.sh itself says these arguments run. Its argument rules and + its CI table are not restated here (improvise-maax).""" out = subprocess.run(["scripts/checkers.sh", "--would-run", *args], capture_output=True, text=True).stdout ran, missing = set(), set() @@ -160,22 +185,33 @@ def would_run(args): return ran, missing -def suites_run(text): - """(suites some step runs, names a step asks for that are not suites).""" - ran, missing = set(), set() - for args in checkers_invocations(text): - r, m = would_run(args) - ran |= r - missing |= m - return ran, missing +def _unrun(s): + return f"`{s}` is a checkers.sh suite that no CI step runs; add it to ci.yml" -def suite_verdict(suites, run): - """The complaints about suites CI never runs, or asks for by a wrong name.""" - ran, missing = run - out = [f"`{s}` is a checkers.sh suite that no CI step runs; add it to ci.yml" - for s in sorted(set(suites) - ran)] - out += [f"ci.yml runs checkers.sh `{s}`, which is not a suite" +def suite_verdict(suites, text): + """The complaints: suites no counted step runs, and every step or line that + does not count, each named so the fix is obvious rather than silent.""" + steps, stray = ci_steps(text) + out, ran, missing = [], set(), set() + for s in steps: + m = _CI_CALL.match(s["run"] or "") + if not m: + continue + if m.group(1) != s["job"]: + out.append(f"job `{s['job']}` runs `checkers.sh --ci {m.group(1)}`; " + "a job runs only its own row of the CI table") + elif s["flags"]: + out.append(f"job `{s['job']}`'s `checkers.sh --ci` step is conditional " + f"({', '.join(sorted(s['flags']))}), so it does not count") + else: + r, mi = would_run(["--ci", s["job"]]) + ran |= r + missing |= mi + out += [f"ci.yml line {n} mentions scripts/checkers.sh outside a step whose " + "whole `run:` is `./scripts/checkers.sh --ci `" for n in stray] + out += [_unrun(s) for s in sorted(set(suites) - ran)] + out += [f"checkers.sh's CI table names `{s}`, which is not a suite" for s in sorted(missing)] return out @@ -187,75 +223,118 @@ def list_suites(): return [s for s in out.split("\n") if s.strip()] +def _wf(*jobs): + return "jobs:\n" + "".join(jobs) + + +def _job(name, *steps, head=""): + return f" {name}:\n runs-on: x\n{head} steps:\n" + "".join(steps) + + +_BUILD = _job("build", " - run: ./scripts/checkers.sh --ci build\n") +_REQ = ["assemble-stamp", "pr-watch"] +_NO_PW = [_unrun("pr-watch")] + + +def _stray(n): + return (f"ci.yml line {n} mentions scripts/checkers.sh outside a step whose " + "whole `run:` is `./scripts/checkers.sh --ci `") + + # (name, suites to require, workflow text, the complaints it must draw). Every -# case asks the REAL checkers.sh. A false "bare run" must be the ONLY way pr-watch runs. -_UNRUN = "`zz-new` is a checkers.sh suite that no CI step runs; add it to ci.yml" -_NO_PW = "`pr-watch` is a checkers.sh suite that no CI step runs; add it to ci.yml" -_KB_PW = ["kb", "pr-watch", "zz-new"] -_REQ = ["kb", "pr-watch"] -_ONLY_KB = " run: ./scripts/checkers.sh kb\n" +# case asks the REAL checkers.sh table, where `kb`'s row holds pr-watch. SUITE_SELF_TEST = [ - ("a suite no step runs is named", - _KB_PW, " run: ./scripts/checkers.sh kb pr-watch\n", [_UNRUN]), + ("two jobs each running their own row are accepted", _REQ, + _wf(_BUILD, _job("kb", " - run: nix shell nixpkgs#trealla --command " + "./scripts/checkers.sh --ci kb # the kb row\n")), []), - ("suites named across steps, one inside nix shell --command, are accepted", - ["kb", "pr-watch", "checkers"], - " run: nix shell nixpkgs#trealla --command ./scripts/checkers.sh kb\n" - " run: ./scripts/checkers.sh --self-test\n" - " - run: ./scripts/checkers.sh pr-watch\n", []), + ("a job that never calls its row leaves its suites unrun", _REQ, + _wf(_BUILD, _job("kb", " - run: echo nothing\n")), _NO_PW), - ("a bare run covers every real suite", - ["kb", "pr-watch", "checkers"], " run: ./scripts/checkers.sh\n", []), + ("a call inside a heredoc in a block run: does not count (st8q)", _REQ, + _wf(_BUILD, _job("kb", " - run: |\n cat >> x <<'EOF'\n" + " ./scripts/checkers.sh --ci kb\n EOF\n")), + [_stray(11)] + _NO_PW), - ("--self-test runs checkers and DISCARDS the names beside it", - ["checkers", "kb"], " run: ./scripts/checkers.sh --self-test zz-new kb\n", - ["`kb` is a checkers.sh suite that no CI step runs; add it to ci.yml"]), + ("a folded run: does not count (st8q)", _REQ, + _wf(_BUILD, _job("kb", " - run: >\n ./scripts/checkers.sh --ci kb\n")), + [_stray(10)] + _NO_PW), - ("--list alone runs nothing", - ["kb"], " run: ./scripts/checkers.sh --list\n", - ["`kb` is a checkers.sh suite that no CI step runs; add it to ci.yml"]), + ("a plain continuation line does not count", _REQ, + _wf(_BUILD, _job("kb", " - run: echo\n ./scripts/checkers.sh --ci kb\n")), + [_stray(10)] + _NO_PW), - ("--list with names runs nothing either", - ["kb"], " run: ./scripts/checkers.sh --list kb zz-new\n", - ["`kb` is a checkers.sh suite that no CI step runs; add it to ci.yml"]), + ("an exact run: line inside a multi-line quoted value does not count", _REQ, + _wf(_BUILD, _job("kb", " - name: \"a\n" + " run: ./scripts/checkers.sh --ci kb\n b\"\n")), + [_stray(10)] + _NO_PW), - ("a bare mention after an inline shell comment is not a run (Codex, #228)", - _REQ, " run: echo skipped # ./scripts/checkers.sh\n" + _ONLY_KB, [_NO_PW]), + ("a whole job inside a top-level block scalar does not count", _REQ, + _wf(_BUILD) + "notes: |\n kb:\n steps:\n" + " - run: ./scripts/checkers.sh --ci kb\n", + [_stray(9)] + _NO_PW), - ("a suite named inside a trailing shell comment is not run", - _REQ, " run: ./scripts/checkers.sh kb # pr-watch\n", [_NO_PW]), + ("a step-shaped line inside a job-level block scalar does not count", _REQ, + _wf(_BUILD, _job("kb", " - run: echo\n", + head=" notes: |\n - run: ./scripts/checkers.sh --ci kb\n")), + [_stray(9)] + _NO_PW), - ("a command-shaped line in a non-run block scalar is not a run", - _REQ, - " - name: notes\n with:\n notes: |\n" - " ./scripts/checkers.sh\n " + _ONLY_KB, [_NO_PW]), + ("a step-shaped item under a job key other than steps: does not count", _REQ, + _wf(_BUILD, " kb:\n services:\n - run: ./scripts/checkers.sh --ci kb\n" + " steps:\n - run: echo\n"), + [_stray(8)] + _NO_PW), - ("a bare mention in a step name is not a run", - _REQ, " - name: Runs scripts/checkers.sh\n " + _ONLY_KB, [_NO_PW]), + ("a step under if: does not count (tvte)", _REQ, + _wf(_BUILD, _job("kb", " - if: false\n run: ./scripts/checkers.sh --ci kb\n")), + ["job `kb`'s `checkers.sh --ci` step is conditional (if), so it does not count"] + + _NO_PW), - ("a bare mention as echo's argument is not a run", - _REQ, " run: echo scripts/checkers.sh\n" + _ONLY_KB, [_NO_PW]), + ("a job under if: does not count (tvte)", _REQ, + _wf(_BUILD, _job("kb", " - run: ./scripts/checkers.sh --ci kb\n", + head=" if: false\n")), + ["job `kb`'s `checkers.sh --ci` step is conditional (job if), so it does not count"] + + _NO_PW), - ("a bare mention in a YAML comment is not a run", - _REQ, " # Every suite goes through scripts/checkers.sh\n" + _ONLY_KB, - [_NO_PW]), + ("a job-level if: after its steps still disqualifies them", _REQ, + _wf(_BUILD, " kb:\n steps:\n - run: ./scripts/checkers.sh --ci kb\n" + " if: false\n"), + ["job `kb`'s `checkers.sh --ci` step is conditional (job if), so it does not count"] + + _NO_PW), - ("every invocation on a line counts, and quotes are removed", - _REQ, - " run: ./scripts/checkers.sh kb && ./scripts/checkers.sh 'pr-watch'\n", []), + ("a job's defaults: can change the shell, so it does not count", _REQ, + _wf(_BUILD, _job("kb", " - run: ./scripts/checkers.sh --ci kb\n", + head=" defaults:\n run:\n shell: python {0}\n")), + ["job `kb`'s `checkers.sh --ci` step is conditional (job defaults), " + "so it does not count"] + _NO_PW), - ("a block run is read, continuation lines included", - _REQ, - " run: |\n set -e\n ./scripts/checkers.sh \\\n" - " kb pr-watch\n - name: next\n", []), + ("workflow-level defaults: disqualifies every step", ["assemble-stamp"], + "defaults:\n run:\n shell: python {0}\n" + _wf(_BUILD), + ["job `build`'s `checkers.sh --ci` step is conditional (workflow defaults), " + "so it does not count", _unrun("assemble-stamp")]), - ("a step spelling --would-run runs nothing, wherever it puts it", - _REQ, _ONLY_KB + " run: ./scripts/checkers.sh pr-watch --would-run\n" - " run: ./scripts/checkers.sh --would-run pr-watch\n", [_NO_PW]), + ("continue-on-error does not count", _REQ, + _wf(_BUILD, _job("kb", " - run: ./scripts/checkers.sh --ci kb\n" + " continue-on-error: true\n")), + ["job `kb`'s `checkers.sh --ci` step is conditional (continue-on-error), " + "so it does not count"] + _NO_PW), - ("a step naming a suite that does not exist is caught", - ["kb"], " run: ./scripts/checkers.sh kb ghost\n", - ["ci.yml runs checkers.sh `ghost`, which is not a suite"]), + ("shell control flow around the call does not count", _REQ, + _wf(_BUILD, _job("kb", " - run: true || ./scripts/checkers.sh --ci kb\n")), + [_stray(9)] + _NO_PW), + + ("a job running another job's row is refused", _REQ, + _wf(_BUILD, _job("kb", " - run: ./scripts/checkers.sh --ci build\n")), + ["job `kb` runs `checkers.sh --ci build`; a job runs only its own row of the CI table"] + + _NO_PW), + + ("a bare mention in a step name is refused", _REQ, + _wf(_BUILD, _job("kb", " - name: runs scripts/checkers.sh\n" + " run: ./scripts/checkers.sh --ci kb\n")), + [_stray(9)]), + + ("a YAML comment mentioning checkers.sh is fine", _REQ, + _wf(_BUILD, _job("kb", " # every suite goes through scripts/checkers.sh\n" + " - run: ./scripts/checkers.sh --ci kb\n")), []), ] @@ -295,7 +374,7 @@ def self_test(): if got != want: failures.append(f" {name}: expected {want}, saw {got}") for name, suites, text, want in SUITE_SELF_TEST: - got = suite_verdict(suites, suites_run(text)) + got = suite_verdict(suites, text) if got != want: failures.append(f" {name}: expected {want}, saw {got}") if failures: @@ -321,7 +400,7 @@ def main(): print("error: checkers.sh --list named no suites; the listing is wrong", file=sys.stderr) return 2 - complaints = verdict(jobs, needs) + suite_verdict(suites, suites_run(text)) + complaints = verdict(jobs, needs) + suite_verdict(suites, text) if complaints: for c in complaints: print(f"ci-graph: {c}", file=sys.stderr) diff --git a/scripts/checkers-test.sh b/scripts/checkers-test.sh index 6c9b8c37..2eb4a30a 100755 --- a/scripts/checkers-test.sh +++ b/scripts/checkers-test.sh @@ -205,5 +205,18 @@ expect "--would-run anywhere but first is a usage error" 4 "$d/checkers.sh" alph [ ! -e "$work/ran-alpha" ] && ok "and it runs nothing" \ || bad "and it runs nothing" "alpha ran" +# --------------------------------------------------------------------------- +# 9. --ci runs that job's row of the CI table and nothing else. The +# sandbox has none of the real suites, so a row reads back as `missing:`. +"$d/checkers.sh" --would-run --ci js > "$work/out" 2>&1 +got="$(tr '\n' ' ' < "$work/out")" +[ "$got" = "missing: node-test " ] && ok "--ci reads that job's row of the table" \ + || bad "--ci reads that job's row of the table" "got [$got]" +expect "--ci with a job the table does not have is exit 4" 4 "$d/checkers.sh" --would-run --ci nope +expect "--ci with nothing after it is exit 4" 4 "$d/checkers.sh" --would-run --ci +expect "--ci beside a suite name is exit 4, not a union" 4 \ + "$d/checkers.sh" --would-run --ci js alpha +expect "--ci beside --self-test is exit 4" 4 "$d/checkers.sh" --would-run --self-test --ci js + printf '\ncheckers-test: %d passed, %d failed\n' "$pass" "$fail" [ "$fail" -eq 0 ] diff --git a/scripts/checkers.sh b/scripts/checkers.sh index 49b5c06a..e7ec3b7c 100755 --- a/scripts/checkers.sh +++ b/scripts/checkers.sh @@ -19,7 +19,7 @@ HERE="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" usage() { cat >&2 <<'EOF' -usage: checkers.sh [--list] [--self-test] [name ...] +usage: checkers.sh [--list] [--self-test] [--ci ] [name ...] name a suite to run, spelled without the -test.sh suffix (e.g. `pr-reviews` for scripts/pr-reviews-test.sh). @@ -27,6 +27,9 @@ usage: checkers.sh [--list] [--self-test] [name ...] --list print the suite names and exit --self-test run scripts/checkers-test.sh, which injects the faults this runner exists to catch + --ci run the suites the CI table gives that job of + .github/workflows/ci.yml, and nothing else. Each CI job runs + exactly this, so the table is the one list of what CI runs --would-run as the FIRST argument only: run nothing, and print `would-run: ` (or `missing: `) for each suite the rest of the arguments would run. check-ci-graph.py @@ -110,8 +113,19 @@ run_suite() { env "${env_args[@]}" "$pinned" "$file" } +# The CI table: the suites each job of .github/workflows/ci.yml runs, by that +# job's `checkers.sh --ci ` step. check-ci-graph.py holds it to --list. +ci_suites() { + case "$1" in + build) echo assemble-stamp ;; + js) echo node-test ;; + kb) echo kb checkers pr-watch release-preflight pr-reviews fmt-workspaces bd-audit-ids ;; + *) return 1 ;; + esac +} + main() { - local self_test=0 would_run=0 + local self_test=0 would_run=0 ci_job="" local -a names=() # Only as the FIRST argument, so a CI step spelling it anywhere is an error. if [ "${1:-}" = --would-run ]; then would_run=1; shift; fi @@ -119,6 +133,7 @@ main() { case "$1" in --list) suites; return 0 ;; --self-test) self_test=1 ;; + --ci) ci_job="${2:-}"; [ -n "$ci_job" ] || { usage; return 4; }; shift ;; -h|--help) usage; return 4 ;; -*) printf 'checkers: unknown option %s\n' "$1" >&2; usage; return 4 ;; *) names+=("$1") ;; @@ -126,7 +141,15 @@ main() { shift done - if [ "$self_test" = 1 ]; then + if [ -n "$ci_job" ]; then + if [ "$self_test" = 1 ] || [ ${#names[@]} -gt 0 ]; then + echo 'checkers: --ci takes a job and nothing else' >&2; return 4 + fi + local table + table="$(ci_suites "$ci_job")" || { + printf 'checkers: no CI job %s in the table\n' "$ci_job" >&2; return 4; } + read -r -a names <<< "$table" + elif [ "$self_test" = 1 ]; then names=(checkers) elif [ ${#names[@]} -eq 0 ]; then while IFS= read -r line; do names+=("$line"); done < <(suites) From 1501def5c29b359492e2709b24e10e419adb2748 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Wed, 23 Sep 2026 11:06:47 -0700 Subject: [PATCH 04/12] check-ci-graph: a continued run: or a skipped need does not count A plain scalar folds a deeper-indented next line into run:, so `|| true` there made the counted call fail-open (improvise-1be2). A job that needs a job under if: is skipped with it, directly or transitively (improvise-bbwa). The unrun message now points at the ci_suites row. tooling.md names the checkout-with-ref hole. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/code/tooling.md | 20 +++++++--- context/test-evidence/tooling.md | 39 +++++++++++++++++++ scripts/check-ci-graph.py | 64 ++++++++++++++++++++++++++++++-- 3 files changed, 113 insertions(+), 10 deletions(-) diff --git a/context/code/tooling.md b/context/code/tooling.md index c81e2f13..14cb4d25 100644 --- a/context/code/tooling.md +++ b/context/code/tooling.md @@ -282,11 +282,17 @@ the row without running it, so the table has one home. The gate reads no shell. It counts a step only when all of these hold: - the step sits in a job's `steps:` sequence, at step-key indentation; -- its `run:` value is exactly that call; +- its `run:` value is exactly that call, on one line. A plain scalar folds a + deeper-indented next line into the value, so `|| true` there would become part + of the command. A `run:` that continues onto another line is not counted + (improvise-1be2); - the call names its own job; - neither the step nor the job carries `if:`, `continue-on-error:` or `shell:`; - no `defaults:` block, at workflow or job level, covers it, since that can - change the shell the step runs under. + change the shell the step runs under; +- no job it `needs:`, directly or through other jobs, carries a job-level + `if:`. GitHub skips a job whose need was skipped, so a false `if:` two jobs + away skips the call as surely as one on its own job (improvise-bbwa). The only YAML it models is what makes a line structure rather than content. A key with an inline value owns every following line indented deeper, and an @@ -299,10 +305,12 @@ The union of the counted jobs' rows must cover `checkers.sh --list`. **What this does not close, stated rather than guarded.** Once a counted step is reached, the gate trusts that it runs the script it names. An earlier step in the same job that rewrote `scripts/checkers.sh` or its table would defeat -that. Shell control flow *around* the call cannot hide in the counted step, -because its value is exactly the call. But the gate cannot see a job-level -`needs:` whose failure skips the job, which is why `pages-deploy` must still -`needs:` every job. +that. So would an earlier `actions/checkout` step with a `ref:`, which +replaces the tree, so the counted step runs another revision's `checkers.sh` +and its table. Shell control flow *around* the call cannot hide in the counted +step, because its value is exactly the call. A needed job that *fails* also +skips the job, and the gate does not model that. The failure is itself red in +CI, and it is why `pages-deploy` must still `needs:` every job. **Two claims earlier versions of this section made were wrong.** "`--would-run` anywhere but first is a usage error" was false: `checkers.sh --list diff --git a/context/test-evidence/tooling.md b/context/test-evidence/tooling.md index bc0da11b..a95786ad 100644 --- a/context/test-evidence/tooling.md +++ b/context/test-evidence/tooling.md @@ -999,3 +999,42 @@ and beside `--self-test`, each exit 0 where 4 was wanted. **Stated, not guarded:** a step earlier in the same job that rewrites `scripts/checkers.sh` or its table (`context/code/tooling.md`). + +### A continued run: and a skipped need do not count — `scripts/check-ci-graph.py` (improvise-1be2, improvise-bbwa) + +**What broke (1be2, P2, fail-open).** The gate matched `run:` against its own +physical line only. A plain YAML scalar folds a deeper-indented next line into +the value, so a kb step whose `run:` continued onto a line reading `|| true` +ran `./scripts/checkers.sh --ci kb || true`. It counted as the exact call, and +the gate reported "all 9 suites run". A failing suite could then never fail CI. + +**What broke (bbwa, P3).** The gate disqualified `if:` on the step and on its +own job, but not on a job it `needs:`. GitHub skips a job whose need was +skipped, so `if: false` on a needed job meant the call never ran, while the +gate counted it. + +**Fix.** A `run:` whose value owns a following non-blank line is not counted, +and the step is named. `needs:` is read in all three forms (`[a, b]`, a +scalar, a block list) and walked transitively. A job-level `if:` anywhere in +that closure disqualifies the step as "conditional (needs K, which has if)". +The "no CI step runs" message now names the fix site: a row of `ci_suites` in +`scripts/checkers.sh`, not `ci.yml`. + +**Red first.** The three new cases (a continuation line, `needs:` inline and +as a block list) each failed with `saw []`, which is the false pass. + +**Self-test: 23 suite lists.** One of the 23 is the two-hop case, added after +the rival below survived the first 22. + +**Rivals, each a source edit run against the real `--self-test`:** +- **The continuation is not marked:** fails the 1be2 case only. +- **No needs flag:** fails the three needs cases. +- **Block-list `needs:` not read:** fails "the same, with needs: written as a + list" only. +- **No transitive walk:** passed all 22 cases at first, because every needs + case was one hop. "The same, two needs: away" (`kb` needs `mid`, `mid` needs + `gate`) was added, and the rival now fails it alone. + +**Stated, not guarded:** a needed job that *fails* also skips the job, and an +earlier `actions/checkout` with `ref:` replaces the tree +(`context/code/tooling.md`). diff --git a/scripts/check-ci-graph.py b/scripts/check-ci-graph.py index bdbc57b4..2debf16f 100755 --- a/scripts/check-ci-graph.py +++ b/scripts/check-ci-graph.py @@ -101,7 +101,8 @@ def ci_steps(text): steps, stray = [], [] job, in_steps, job_flags, step = None, False, set(), None workflow_flags = set() - owner, quote = None, None + owner, owner_step, quote = None, None, None + needs, if_jobs, in_needs = {}, set(), False def close_step(): if step is not None: @@ -118,8 +119,10 @@ def ci_steps(text): if owner is not None and (not raw.strip() or indent > owner): if "scripts/checkers.sh" in raw: stray.append(n) + if owner_step is not None and raw.strip(): + owner_step["multiline"] = True continue - owner = None + owner, owner_step = None, None if not raw.strip() or raw.lstrip().startswith("#"): continue if raw.startswith("defaults:"): @@ -128,7 +131,10 @@ def ci_steps(text): if m: close_step() step = None - job, in_steps, job_flags = m.group(1), False, set() + job, in_steps, job_flags, in_needs = m.group(1), False, set(), False + continue + if in_needs and re.match(r"^ - ", raw): + needs[job].add(_inline_value(raw.strip()[2:])) continue m = _JOB_KEY.match(raw) if m and job is not None: @@ -136,6 +142,11 @@ def ci_steps(text): step = None key, value = m.group(1), _inline_value(m.group(2)) in_steps = key == "steps" and value == "" + in_needs = key == "needs" and value == "" + if key == "needs": + needs[job] = {x.strip() for x in value.strip("[]").split(",") if x.strip()} + if key == "if": + if_jobs.add(job) if key in _JOB_DISQUALIFY: job_flags.add(key) for s in steps: @@ -165,12 +176,25 @@ def ci_steps(text): quote = value[0] else: owner = len(k.group(1)) + (2 if raw.lstrip().startswith("- ") else 0) + owner_step = step if m and m.group(1) == "run" else None close_step() for s in steps: s["flags"] |= workflow_flags + s["flags"] |= {f"needs {j}, which has if" for j in _needed(s["job"], needs) & if_jobs} return steps, stray +def _needed(job, needs): + """Every job `job` needs, transitively: a skipped one skips it too (improvise-bbwa).""" + seen, todo = set(), list(needs.get(job, ())) + while todo: + j = todo.pop() + if j not in seen: + seen.add(j) + todo += needs.get(j, ()) + return seen + + def would_run(args): """What checkers.sh itself says these arguments run. Its argument rules and its CI table are not restated here (improvise-maax).""" @@ -186,7 +210,8 @@ def would_run(args): def _unrun(s): - return f"`{s}` is a checkers.sh suite that no CI step runs; add it to ci.yml" + return (f"`{s}` is a checkers.sh suite that no CI step runs; add it to a row of " + "ci_suites in scripts/checkers.sh") def suite_verdict(suites, text): @@ -201,6 +226,9 @@ def suite_verdict(suites, text): if m.group(1) != s["job"]: out.append(f"job `{s['job']}` runs `checkers.sh --ci {m.group(1)}`; " "a job runs only its own row of the CI table") + elif s.get("multiline"): + out.append(f"job `{s['job']}`'s `checkers.sh --ci` step's run: continues " + "onto another line, so it does not count") elif s["flags"]: out.append(f"job `{s['job']}`'s `checkers.sh --ci` step is conditional " f"({', '.join(sorted(s['flags']))}), so it does not count") @@ -284,6 +312,34 @@ SUITE_SELF_TEST = [ " steps:\n - run: echo\n"), [_stray(8)] + _NO_PW), + ("the call with a continuation line does not count (1be2)", _REQ, + _wf(_BUILD, _job("kb", " - run: ./scripts/checkers.sh --ci kb\n" + " || true\n")), + ["job `kb`'s `checkers.sh --ci` step's run: continues onto another line, " + "so it does not count"] + _NO_PW), + + ("a job that needs a job under if: does not count (bbwa)", _REQ, + _wf(_BUILD, " gate:\n if: false\n steps:\n - run: echo\n", + _job("kb", " - run: ./scripts/checkers.sh --ci kb\n", + head=" needs: [build, gate]\n")), + ["job `kb`'s `checkers.sh --ci` step is conditional (needs gate, which has if), " + "so it does not count"] + _NO_PW), + + ("the same, with needs: written as a list", _REQ, + _wf(_BUILD, " gate:\n if: false\n steps:\n - run: echo\n", + _job("kb", " - run: ./scripts/checkers.sh --ci kb\n", + head=" needs:\n - gate\n")), + ["job `kb`'s `checkers.sh --ci` step is conditional (needs gate, which has if), " + "so it does not count"] + _NO_PW), + + ("the same, two needs: away", _REQ, + _wf(_BUILD, " gate:\n if: false\n steps:\n - run: echo\n", + " mid:\n needs: gate\n steps:\n - run: echo\n", + _job("kb", " - run: ./scripts/checkers.sh --ci kb\n", + head=" needs: [mid]\n")), + ["job `kb`'s `checkers.sh --ci` step is conditional (needs gate, which has if), " + "so it does not count"] + _NO_PW), + ("a step under if: does not count (tvte)", _REQ, _wf(_BUILD, _job("kb", " - if: false\n run: ./scripts/checkers.sh --ci kb\n")), ["job `kb`'s `checkers.sh --ci` step is conditional (if), so it does not count"] From 3eafd4dec7bc6dc83e6a3de6dcfed2f26aadd57f Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Wed, 23 Sep 2026 17:02:13 -0700 Subject: [PATCH 05/12] check-ci-graph: refuse a key or needs: it cannot read plainly (w0j4) Quoted, anchored and multi-line needs: values, and quoted, spaced or explicit ? keys, all skipped kb on a PR while the gate reported every suite run. Rather than parse more YAML, every structural line inside jobs: must take one plain form or the gate names it and fails. Also a 1be2 case at the one-column boundary, which the owner+1 rival passed. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/code/tooling.md | 21 ++++++ context/test-evidence/tooling.md | 51 +++++++++++++ scripts/check-ci-graph.py | 125 ++++++++++++++++++++++++++++++- 3 files changed, 193 insertions(+), 4 deletions(-) diff --git a/context/code/tooling.md b/context/code/tooling.md index 14cb4d25..543d36c9 100644 --- a/context/code/tooling.md +++ b/context/code/tooling.md @@ -302,6 +302,27 @@ scalar is skipped as content. Any line outside a YAML comment that mentions `scripts/checkers.sh` and is not a counted step is **reported**, not ignored. The union of the counted jobs' rows must cover `checkers.sh --list`. +**A key the gate cannot read plainly is refused, not parsed** (improvise-w0j4). +YAML has many spellings of one key or one list. `"if":`, `'continue-on-error':`, +`if :`, and `? if` then `: false` all mean the same key. `needs: ["gate"]`, +`needs: 'gate'`, `- "gate"`, `needs: &d [gate]`, and a flow list split over +two lines all name `gate`. The gate read none of them. Each one skipped `kb` +on a PR while the gate reported every suite run. + +Teaching it each spelling is the treadmill st8q stepped off. So every +structural line inside `jobs:` must take one plain form, or the gate names the +line and fails: +- a top-level key; +- a job name; +- a job key; +- a step key; +- a `needs:` value that is empty (a block list follows), one bare name, or a + one-line flow list of bare names; +- a block-list item that is a bare name. + +Content under a job key other than `steps:` and `needs:`, such as `env:` or +`strategy:`, is not structure the gate reads, so it is not checked. + **What this does not close, stated rather than guarded.** Once a counted step is reached, the gate trusts that it runs the script it names. An earlier step in the same job that rewrote `scripts/checkers.sh` or its table would defeat diff --git a/context/test-evidence/tooling.md b/context/test-evidence/tooling.md index a95786ad..cce90568 100644 --- a/context/test-evidence/tooling.md +++ b/context/test-evidence/tooling.md @@ -1038,3 +1038,54 @@ the rival below survived the first 22. **Stated, not guarded:** a needed job that *fails* also skips the job, and an earlier `actions/checkout` with `ref:` replaces the tree (`context/code/tooling.md`). + +### The gate refuses a key or needs: it cannot read plainly — `scripts/check-ci-graph.py` (improvise-w0j4) + +**What broke (P2, fail-open).** Found in the delta review of #228 at +`2aafff3b`. The bbwa fix read `needs:` names as raw text, and it recognised job +and step keys only in the bare `key:` form. The review put a `gate` job with +`if: false` in the real `ci.yml` and gave `kb` a `needs:` on it. Every one of +these spellings still exited 0 with "all 9 suites run": +- `needs: ["gate"]`, `needs: 'gate'`, and a block item `- "gate"`; +- `needs: &d [gate]`, and `needs: &d` followed by a block list; +- a flow list split over two lines. + +yq read `kb` as needing `gate` in every case. The disqualifier keys had the +same gap: `"if":`, `'continue-on-error':`, `if :`, and `? if` then `: false`. + +**Fix: the review's suggestion, refuse rather than parse.** Every structural +line inside `jobs:` must be one plain form. That means a job name, a job key, a +step key, a `needs:` value that is empty, one bare name or a one-line flow list +of bare names, or a bare block item. Top-level keys must be plain everywhere. +Anything else is reported, naming the line, and the gate fails. + +**Red first.** Eleven refusal cases were added. All eleven failed with `saw []` +against `2aafff3b`, which is the false pass. Two controls, `needs: [ gate ] # +c` and `needs: gate`, still read `gate` and are conditional. Two more cases, +a quoted job name and a quoted top-level key, were added with the fix. + +**Replayed on the real `ci.yml`.** Each attack was mutated in, run, and +restored, with a byte comparison after the last. All eleven spellings now exit +1, and the plain `needs: [gate]` control exits 1 as conditional. + +**The 1be2 boundary.** The earlier case put the continuation two columns past +the key. So a rival that marks multiline only past `owner + 1` passed all 23 +cases. On the real `ci.yml`, a 9-space `|| true` under the kb step's `run:` at +column 8 then exited 0. The new case continues exactly one column past the +key. The review's rival fails it and nothing else. On the real `ci.yml` the +9-space continuation now exits 1. + +**Rivals, each a source edit run against the real `--self-test` (39 cases):** +- `needs:` value accepted as anything: fails the five inline or anchored or + split `needs:` cases. +- Block items accepted as anything: fails the quoted-item case. +- Step keys not checked: fails the quoted step key. +- Job keys not checked: fails `"if":`, `'continue-on-error':`, `if :`, and `?`. +- Job names not checked: fails the quoted job name. +- Top-level keys not checked: fails the quoted top-level key. +- Refusals collected but not reported: fails all thirteen. +- The review's `owner + 1` rival: fails the boundary case alone. + +**Stated, not guarded.** Content under job keys other than `steps:` and +`needs:` is not checked. Neither is anything deeper than a step's own keys. +The gate reads no structure there. diff --git a/scripts/check-ci-graph.py b/scripts/check-ci-graph.py index 2debf16f..bc7f0a9a 100755 --- a/scripts/check-ci-graph.py +++ b/scripts/check-ci-graph.py @@ -76,6 +76,28 @@ _DISQUALIFY = ("if", "continue-on-error", "shell") _JOB_DISQUALIFY = _DISQUALIFY + ("defaults",) +_NAME = r"[A-Za-z_][\w-]*" +_TOP_KEY = re.compile(rf"^{_NAME}:(\s|$)") +_NEEDS_VALUE = re.compile(rf"^(|{_NAME}|\[\s*{_NAME}(\s*,\s*{_NAME})*\s*\])$") +_NEEDS_ITEM = re.compile(rf"^ - {_NAME}$") + + +def _readable(line, indent, in_steps, in_needs): + """Whether a structural line inside `jobs:` is one this gate can read. Anything + else is refused, not parsed: context/code/tooling.md (improvise-w0j4).""" + if indent == 2: + return bool(_JOB.match(line)) + if indent == 4: + m = _JOB_KEY.match(line) + return bool(m) and (m.group(1) != "needs" + or bool(_NEEDS_VALUE.match(_inline_value(m.group(2))))) + if in_needs: + return bool(_NEEDS_ITEM.match(line)) + if in_steps: + return indent in (6, 8) and bool(_STEP_KEY.match(line)) + return indent > 4 + + def _inline_value(rest): """A key's inline value, without a YAML comment (`#` after whitespace).""" return re.sub(r"\s+#.*$", "", rest).strip() @@ -98,8 +120,9 @@ def ci_steps(text): Nothing is read as shell. A key with an inline value owns every following line indented deeper, which is how a block scalar, a folded value, a plain continuation or a heredoc inside one is skipped whole (improvise-st8q).""" - steps, stray = [], [] + steps, stray, unread = [], [], [] job, in_steps, job_flags, step = None, False, set(), None + in_jobs = False workflow_flags = set() owner, owner_step, quote = None, None, None needs, if_jobs, in_needs = {}, set(), False @@ -125,6 +148,13 @@ def ci_steps(text): owner, owner_step = None, None if not raw.strip() or raw.lstrip().startswith("#"): continue + line = re.sub(r"\s+#.*$", "", raw) + if indent == 0: + in_jobs = line.startswith("jobs:") + if not _TOP_KEY.match(line): + unread.append(n) + elif in_jobs and indent <= 8 and not _readable(line, indent, in_steps, in_needs): + unread.append(n) if raw.startswith("defaults:"): workflow_flags.add("workflow defaults") m = _JOB.match(raw) @@ -181,7 +211,7 @@ def ci_steps(text): for s in steps: s["flags"] |= workflow_flags s["flags"] |= {f"needs {j}, which has if" for j in _needed(s["job"], needs) & if_jobs} - return steps, stray + return steps, stray, unread def _needed(job, needs): @@ -214,11 +244,16 @@ def _unrun(s): "ci_suites in scripts/checkers.sh") +def _unread(n): + return (f"ci.yml line {n} is a key or `needs:` value this gate cannot read as a " + "plain name, so it cannot trust the graph; spell it plainly") + + def suite_verdict(suites, text): """The complaints: suites no counted step runs, and every step or line that does not count, each named so the fix is obvious rather than silent.""" - steps, stray = ci_steps(text) - out, ran, missing = [], set(), set() + steps, stray, unread = ci_steps(text) + out, ran, missing = [_unread(n) for n in unread], set(), set() for s in steps: m = _CI_CALL.match(s["run"] or "") if not m: @@ -269,6 +304,19 @@ def _stray(n): "whole `run:` is `./scripts/checkers.sh --ci `") +_GATE = " gate:\n if: false\n steps:\n - run: echo\n" +_KB_RUN = " - run: ./scripts/checkers.sh --ci kb\n" +_NEEDS_GATE = ["job `kb`'s `checkers.sh --ci` step is conditional (needs gate, which has if), " + "so it does not count"] + _NO_PW + + +def _unreadable(name, text, *markers, rest=()): + """A case whose workflow spells a key or a `needs:` value the gate must refuse.""" + lines = text.split("\n") + at = [next(i for i, l in enumerate(lines, 1) if m in l) for m in markers] + return (name, _REQ, text, [_unread(n) for n in at] + list(rest)) + + # (name, suites to require, workflow text, the complaints it must draw). Every # case asks the REAL checkers.sh table, where `kb`'s row holds pr-watch. SUITE_SELF_TEST = [ @@ -332,6 +380,75 @@ SUITE_SELF_TEST = [ ["job `kb`'s `checkers.sh --ci` step is conditional (needs gate, which has if), " "so it does not count"] + _NO_PW), + ("the 1be2 continuation exactly one column past the key does not count (w0j4)", _REQ, + _wf(_BUILD, _job("kb", _KB_RUN + " || true\n")), + ["job `kb`'s `checkers.sh --ci` step's run: continues onto another line, " + "so it does not count"] + _NO_PW), + + ("control: a padded flow list with a comment is still read (w0j4)", _REQ, + _wf(_BUILD, _GATE, _job("kb", _KB_RUN, head=" needs: [ gate ] # c\n")), + _NEEDS_GATE), + + ("control: a plain scalar needs: is still read (w0j4)", _REQ, + _wf(_BUILD, _GATE, _job("kb", _KB_RUN, head=" needs: gate\n")), + _NEEDS_GATE), + + _unreadable("a quoted name in a flow needs: is refused (w0j4)", + _wf(_BUILD, _GATE, _job("kb", _KB_RUN, head=' needs: ["gate"]\n')), + 'needs: ["gate"]'), + + _unreadable("a quoted scalar needs: is refused (w0j4)", + _wf(_BUILD, _GATE, _job("kb", _KB_RUN, head=" needs: 'gate'\n")), + "needs: 'gate'"), + + _unreadable("a quoted item in a block needs: is refused (w0j4)", + _wf(_BUILD, _GATE, _job("kb", _KB_RUN, head=' needs:\n - "gate"\n')), + '- "gate"'), + + _unreadable("an anchored flow needs: is refused (w0j4)", + _wf(_BUILD, _GATE, _job("kb", _KB_RUN, head=" needs: &d [gate]\n")), + "needs: &d"), + + _unreadable("an anchored block needs: is refused (w0j4)", + _wf(_BUILD, _GATE, _job("kb", _KB_RUN, head=" needs: &d\n - gate\n")), + "needs: &d"), + + _unreadable("a flow needs: over two lines is refused (w0j4)", + _wf(_BUILD, _GATE, _job("kb", _KB_RUN, head=" needs: [build,\n gate]\n")), + "needs: [build,"), + + _unreadable("a quoted job key is refused (w0j4)", + _wf(_BUILD, _job("kb", _KB_RUN, head=' "if": false\n')), + '"if"'), + + _unreadable("a quoted step key is refused (w0j4)", + _wf(_BUILD, _job("kb", " - \"if\": false\n" + " run: ./scripts/checkers.sh --ci kb\n")), + '"if"'), + + _unreadable("a single-quoted continue-on-error is refused (w0j4)", + _wf(_BUILD, _job("kb", _KB_RUN, head=" 'continue-on-error': true\n")), + "'continue-on-error'"), + + _unreadable("a key with a space before its colon is refused (w0j4)", + _wf(_BUILD, _job("kb", _KB_RUN, head=" if : false\n")), + "if : false"), + + _unreadable("a quoted job name is refused (w0j4)", + _wf(_BUILD, ' "gate":\n if: false\n steps:\n - run: echo\n', + _job("kb", _KB_RUN)), + '"gate"', + rest=["job `build`'s `checkers.sh --ci` step is conditional (job if), " + "so it does not count", _unrun("assemble-stamp")]), + + _unreadable("a quoted top-level key is refused (w0j4)", + '"defaults":\n run:\n shell: bash\n' + _wf(_BUILD, _job("kb", _KB_RUN)), + '"defaults"'), + + _unreadable("an explicit ? key is refused (w0j4)", + _wf(_BUILD, _job("kb", _KB_RUN, head=" ? if\n : false\n")), + "? if", ": false"), + ("the same, two needs: away", _REQ, _wf(_BUILD, " gate:\n if: false\n steps:\n - run: echo\n", " mid:\n needs: gate\n steps:\n - run: echo\n", From 5738e8e0c05695d8f642846b194bc0005005337a Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Wed, 23 Sep 2026 17:34:06 -0700 Subject: [PATCH 06/12] check-ci-graph: a job's keys start at indent 4, or its lines are refused (rjaa) w0j4 read any line deeper than 4 outside steps:/needs: as content, so a needed job whose own keys sat at indent 5 or 6 (valid YAML) had its if: and needs: missed and the gate exited 0. A deeper line is now content only once its job has a 4-space key. tooling.md names the three valid styles the gate refuses loudly. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/code/tooling.md | 15 +++++++++++++++ context/test-evidence/tooling.md | 32 ++++++++++++++++++++++++++++++++ scripts/check-ci-graph.py | 32 +++++++++++++++++++++++++++----- 3 files changed, 74 insertions(+), 5 deletions(-) diff --git a/context/code/tooling.md b/context/code/tooling.md index 543d36c9..d96c6b34 100644 --- a/context/code/tooling.md +++ b/context/code/tooling.md @@ -323,6 +323,21 @@ line and fails: Content under a job key other than `steps:` and `needs:`, such as `env:` or `strategy:`, is not structure the gate reads, so it is not checked. +**But a line is content only once its job has a 4-space key** (improvise-rjaa). +The first version accepted any line deeper than 4 outside `steps:` and +`needs:`. So a *needed* job whose own keys sat at indent 5 or 6, which is valid +YAML, had its `if:` and `needs:` read as content. The gate exited 0 with `kb` +skipped. Now a job's first key must be at indent 4. After that, a deeper line +can only be the content of an empty-valued key, because a key with an inline +value already owns every deeper line. + +Three valid styles are refused loudly rather than read: +- an indentless block list (`needs:` then `- gate` at indent 4); +- a block list at indent 8; +- a quoted top-level key such as `"on":`. + +Each exits 1 and names its line. Spell it the plain way. + **What this does not close, stated rather than guarded.** Once a counted step is reached, the gate trusts that it runs the script it names. An earlier step in the same job that rewrote `scripts/checkers.sh` or its table would defeat diff --git a/context/test-evidence/tooling.md b/context/test-evidence/tooling.md index cce90568..40ec45f9 100644 --- a/context/test-evidence/tooling.md +++ b/context/test-evidence/tooling.md @@ -1089,3 +1089,35 @@ key. The review's rival fails it and nothing else. On the real `ci.yml` the **Stated, not guarded.** Content under job keys other than `steps:` and `needs:` is not checked. Neither is anything deeper than a step's own keys. The gate reads no structure there. + +### A job's keys start at indent 4, or its lines are refused — `scripts/check-ci-graph.py` (improvise-rjaa) + +**What broke (P2, fail-open).** Found in the delta review of #228 at +`3eafd4de`. w0j4's `_readable` accepted any line deeper than 4 outside `steps:` +and `needs:` as content. A *needed* job whose own keys sat at indent 5 or 6, +which is valid YAML, therefore had its `if:` and `needs:` read as content. The +review's three plants each exited 0 with `kb` skipped: +- a 6-space `gate` job, with `kb` given `needs: gate`; +- a 5-space `gate` job, with `kb` given `needs: [gate]`; +- a 6-space `mid` job holding `needs: gate`, with `kb` given `needs: mid`. + +**Fix, as the review suggested.** A job's first key must be at indent 4. After +that, a line at indent 5 to 8 outside `steps:` and `needs:` is content only +once its job has a 4-space key. Such a line can only be the content of an +empty-valued key, because a key with an inline value already owns every deeper +line. + +**Red first.** All three plants, as self-test cases, failed with `saw []`. A +control, `env:` then `FOO: bar` at indent 6, stays accepted as content. + +**Replayed on the real `ci.yml`.** Each plant was mutated in, run, and +restored. All three now exit 1, naming the first misplaced line. + +**Rivals, each run against the real `--self-test` (43 cases):** +- The pre-fix rule (any line past 4 is content): fails the three plants. +- `job_keyed` never reset at a new job: fails the same three. The previous + job's key would otherwise license the next job's misplaced lines. + +**Refused loudly, not read:** an indentless block list, a block list at indent +8, and a quoted top-level key such as `"on":`. Each is valid YAML, and each is +named by line. diff --git a/scripts/check-ci-graph.py b/scripts/check-ci-graph.py index bc7f0a9a..58ddcc60 100755 --- a/scripts/check-ci-graph.py +++ b/scripts/check-ci-graph.py @@ -82,9 +82,9 @@ _NEEDS_VALUE = re.compile(rf"^(|{_NAME}|\[\s*{_NAME}(\s*,\s*{_NAME})*\s*\])$") _NEEDS_ITEM = re.compile(rf"^ - {_NAME}$") -def _readable(line, indent, in_steps, in_needs): +def _readable(line, indent, in_steps, in_needs, job_keyed): """Whether a structural line inside `jobs:` is one this gate can read. Anything - else is refused, not parsed: context/code/tooling.md (improvise-w0j4).""" + else is refused, not parsed: context/code/tooling.md (improvise-w0j4, rjaa).""" if indent == 2: return bool(_JOB.match(line)) if indent == 4: @@ -95,7 +95,7 @@ def _readable(line, indent, in_steps, in_needs): return bool(_NEEDS_ITEM.match(line)) if in_steps: return indent in (6, 8) and bool(_STEP_KEY.match(line)) - return indent > 4 + return indent > 4 and job_keyed def _inline_value(rest): @@ -122,7 +122,7 @@ def ci_steps(text): continuation or a heredoc inside one is skipped whole (improvise-st8q).""" steps, stray, unread = [], [], [] job, in_steps, job_flags, step = None, False, set(), None - in_jobs = False + in_jobs, job_keyed = False, False workflow_flags = set() owner, owner_step, quote = None, None, None needs, if_jobs, in_needs = {}, set(), False @@ -153,7 +153,8 @@ def ci_steps(text): in_jobs = line.startswith("jobs:") if not _TOP_KEY.match(line): unread.append(n) - elif in_jobs and indent <= 8 and not _readable(line, indent, in_steps, in_needs): + elif in_jobs and indent <= 8 and not _readable(line, indent, in_steps, in_needs, + job_keyed): unread.append(n) if raw.startswith("defaults:"): workflow_flags.add("workflow defaults") @@ -162,12 +163,14 @@ def ci_steps(text): close_step() step = None job, in_steps, job_flags, in_needs = m.group(1), False, set(), False + job_keyed = False continue if in_needs and re.match(r"^ - ", raw): needs[job].add(_inline_value(raw.strip()[2:])) continue m = _JOB_KEY.match(raw) if m and job is not None: + job_keyed = True close_step() step = None key, value = m.group(1), _inline_value(m.group(2)) @@ -445,6 +448,25 @@ SUITE_SELF_TEST = [ '"defaults":\n run:\n shell: bash\n' + _wf(_BUILD, _job("kb", _KB_RUN)), '"defaults"'), + ("control: a job key's block content at indent 6 is content (rjaa)", _REQ, + _wf(_BUILD, _job("kb", _KB_RUN, head=" env:\n FOO: bar\n")), []), + + _unreadable("a needed job whose keys sit at indent 6 is refused (rjaa)", + _wf(_BUILD, " gate:\n if: false\n steps:\n - run: echo\n", + _job("kb", _KB_RUN, head=" needs: gate\n")), + " if: false", " steps:", " - run: echo"), + + _unreadable("a needed job whose keys sit at indent 5 is refused (rjaa)", + _wf(_BUILD, " gate:\n if: false\n steps:\n - run: echo\n", + _job("kb", _KB_RUN, head=" needs: [gate]\n")), + " if: false", " steps:", " - run: echo"), + + _unreadable("a job between, with its needs: at indent 6, is refused (rjaa)", + _wf(_BUILD, _GATE, + " mid:\n needs: gate\n steps:\n - run: echo\n", + _job("kb", _KB_RUN, head=" needs: mid\n")), + " needs: gate", " steps:", " - run: echo"), + _unreadable("an explicit ? key is refused (w0j4)", _wf(_BUILD, _job("kb", _KB_RUN, head=" ? if\n : false\n")), "? if", ": false"), From 5036b4483a900c4a1cf300e0557e160f2bc36249 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Wed, 23 Sep 2026 17:56:28 -0700 Subject: [PATCH 07/12] check-ci-graph: a comment on a line the gate reads is refused, and the stripper is gone (j49r) WIP; tooling.md and evidence follow. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- scripts/check-ci-graph.py | 59 ++++++++++++++++++++++++++++++--------- 1 file changed, 46 insertions(+), 13 deletions(-) diff --git a/scripts/check-ci-graph.py b/scripts/check-ci-graph.py index 58ddcc60..29a9bd5b 100755 --- a/scripts/check-ci-graph.py +++ b/scripts/check-ci-graph.py @@ -23,6 +23,7 @@ WORKFLOW = ".github/workflows/ci.yml" DEPLOY = "pages-deploy" _JOB = re.compile(r"^ ([A-Za-z0-9_-]+):$") +_JOB_OPEN = re.compile(r"^ ([A-Za-z0-9_-]+):(\s|$)") _NEEDS = re.compile(r"^ needs:\s*\[(.*)\]\s*$") @@ -31,7 +32,9 @@ def read_graph(text): Both come off indentation, which is what a workflow's own shape is. A job key is two spaces; `needs:` is four INSIDE the job it belongs to, so the - current job is tracked rather than the first `needs:` taken. + current job is tracked rather than the first `needs:` taken. A job opens on + `_JOB_OPEN`, wider than `_JOB`: this reader refuses nothing, so it reads wide + (improvise-j49r). """ jobs, needs, current = [], None, None in_jobs = False @@ -41,7 +44,7 @@ def read_graph(text): continue if not in_jobs: continue - m = _JOB.match(raw) + m = _JOB_OPEN.match(raw) if m: current = m.group(1) jobs.append(current) @@ -80,6 +83,7 @@ _NAME = r"[A-Za-z_][\w-]*" _TOP_KEY = re.compile(rf"^{_NAME}:(\s|$)") _NEEDS_VALUE = re.compile(rf"^(|{_NAME}|\[\s*{_NAME}(\s*,\s*{_NAME})*\s*\])$") _NEEDS_ITEM = re.compile(rf"^ - {_NAME}$") +_COMMENT = re.compile(r"\s#") def _readable(line, indent, in_steps, in_needs, job_keyed): @@ -99,8 +103,9 @@ def _readable(line, indent, in_steps, in_needs, job_keyed): def _inline_value(rest): - """A key's inline value, without a YAML comment (`#` after whitespace).""" - return re.sub(r"\s+#.*$", "", rest).strip() + """A key's inline value. No comment is stripped: a line carrying one is + refused, so every matcher reads the same string (improvise-j49r).""" + return rest.strip() def _opens_quote(value): @@ -148,13 +153,13 @@ def ci_steps(text): owner, owner_step = None, None if not raw.strip() or raw.lstrip().startswith("#"): continue - line = re.sub(r"\s+#.*$", "", raw) if indent == 0: - in_jobs = line.startswith("jobs:") - if not _TOP_KEY.match(line): - unread.append(n) - elif in_jobs and indent <= 8 and not _readable(line, indent, in_steps, in_needs, - job_keyed): + in_jobs = raw.startswith("jobs:") + readable = bool(_TOP_KEY.match(raw)) + else: + readable = not in_jobs or indent > 8 or _readable(raw, indent, in_steps, + in_needs, job_keyed) + if _COMMENT.search(raw) or not readable: unread.append(n) if raw.startswith("defaults:"): workflow_flags.add("workflow defaults") @@ -325,7 +330,11 @@ def _unreadable(name, text, *markers, rest=()): SUITE_SELF_TEST = [ ("two jobs each running their own row are accepted", _REQ, _wf(_BUILD, _job("kb", " - run: nix shell nixpkgs#trealla --command " - "./scripts/checkers.sh --ci kb # the kb row\n")), []), + "./scripts/checkers.sh --ci kb\n")), []), + + _unreadable("a trailing comment on the counted run: line is refused (j49r)", + _wf(_BUILD, _job("kb", _KB_RUN.rstrip("\n") + " # the kb row\n")), + "# the kb row", rest=[_stray(9)] + _NO_PW), ("a job that never calls its row leaves its suites unrun", _REQ, _wf(_BUILD, _job("kb", " - run: echo nothing\n")), _NO_PW), @@ -388,8 +397,8 @@ SUITE_SELF_TEST = [ ["job `kb`'s `checkers.sh --ci` step's run: continues onto another line, " "so it does not count"] + _NO_PW), - ("control: a padded flow list with a comment is still read (w0j4)", _REQ, - _wf(_BUILD, _GATE, _job("kb", _KB_RUN, head=" needs: [ gate ] # c\n")), + ("control: a padded flow list is still read (w0j4)", _REQ, + _wf(_BUILD, _GATE, _job("kb", _KB_RUN, head=" needs: [ gate ]\n")), _NEEDS_GATE), ("control: a plain scalar needs: is still read (w0j4)", _REQ, @@ -527,6 +536,25 @@ SUITE_SELF_TEST = [ " run: ./scripts/checkers.sh --ci kb\n")), [_stray(9)]), + _unreadable("a job header with a trailing comment is refused (j49r)", + _wf(_BUILD, " test:\n runs-on: x\n", + " gate: # skipped on purpose\n if: false\n steps:\n - run: echo\n", + _job("kb", _KB_RUN, head=" needs: gate\n")), + "gate: # skipped"), + + _unreadable("a closed quote followed by a comment is refused (j49r)", + _wf(_BUILD, _job("kb", _KB_RUN, head=" env:\n A: \"x\" # note\n") + + " if: false\n", " after:\n steps:\n - run: echo \"z\"\n"), + '"x" # note', rest=[_stray(11)] + _NO_PW), + + _unreadable("a # inside a quoted value, below indent 8, is refused (j49r)", + _wf(_BUILD, _job("kb", _KB_RUN + " - uses: x\n with:\n" + " foo: \"x # y\"\n") + + " if: false\n", " after:\n steps:\n - run: echo \"z\"\n"), + '"x # y"', + rest=["job `kb`'s `checkers.sh --ci` step is conditional (job if), " + "so it does not count"] + _NO_PW), + ("a YAML comment mentioning checkers.sh is fine", _REQ, _wf(_BUILD, _job("kb", " # every suite goes through scripts/checkers.sh\n" " - run: ./scripts/checkers.sh --ci kb\n")), []), @@ -552,6 +580,11 @@ SELF_TEST = [ "jobs:\n test:\n x: 1\n pages-deploy:\n x: 1\n", [f"{DEPLOY} has no `needs:` list, so nothing gates the deploy"]), + ("a job header with a trailing comment is still a job (j49r)", + "jobs:\n test:\n x: 1\n gate: # skipped on purpose\n x: 1\n" + " pages-deploy:\n needs: [test]\n", + [f"{DEPLOY} does not need `gate`, so a failing gate cannot stop a deploy"]), + # `needs:` belongs to the job it is indented under. Taking the first one # seen would read ANOTHER job's list as the deploy's and pass. ("another job's needs is not mistaken for the deploy's", From 588094b7bb54b25ff4d2e8d5e1cad803a9e72dfb Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Wed, 23 Sep 2026 18:02:08 -0700 Subject: [PATCH 08/12] check-ci-graph: a ' #' on a read line is refused, one string per line, read_graph opens wide (j49r) A job header with a trailing comment was accepted by the readability check (comment-stripped line) and never opened (raw line), so its if: went to the previous job and read_graph never listed it. The audit found the stripper also cut a quoted "x # y" open, hiding a later job-level if:. Both exited 0 on the real ci.yml. Refusing the comment makes stripped == raw on every accepted line, so the stripper is deleted. Argument: context/code/tooling.md. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/code/tooling.md | 33 +++++++++++++++++++ context/test-evidence/tooling.md | 56 ++++++++++++++++++++++++++++++++ scripts/check-ci-graph.py | 21 +++++++----- 3 files changed, 101 insertions(+), 9 deletions(-) diff --git a/context/code/tooling.md b/context/code/tooling.md index d96c6b34..d1c09a7a 100644 --- a/context/code/tooling.md +++ b/context/code/tooling.md @@ -338,6 +338,39 @@ Three valid styles are refused loudly rather than read: Each exits 1 and names its line. Spell it the plain way. +**A ` #` on any line the gate reads is refused, and every matcher reads the +same string** (improvise-j49r). The gate used to strip a trailing comment for +the readability check and match the raw line everywhere else. So +` gate: # skipped on purpose` was accepted as a plain job header by one and +never opened as a job by the other. Its `if: false` was credited to the job +before it, and `read_graph` did not list `gate` at all. Both rules passed with +`kb` skipped and the deploy ungated by `gate`. An audit of every match found +the stripper wrong in a second way: it cut `A: "x # y"` to `A: "x`, the quote +tracker then held the next lines open, and a job-level `if:` after it vanished. +That also exited 0 on the real `ci.yml`. + +There were two ways to make the matchers agree: +- **strip everywhere.** A correct stripper has to know where a quoted value + starts and ends, because `"x # y"` has no comment and `"x" # y` has one. That + is parsing again, the thing st8q and w0j4 gave up. +- **Refuse the comment.** This was chosen. Once every line containing ` #` is + refused, the stripped and raw forms of every accepted line are the same + string, so no two matchers can read different versions of it. So the + stripper is deleted, not kept consistent. + +It costs one valid style: a trailing comment, or a ` #` inside a quoted value, +on any line that is not block-scalar or quoted content. The real `ci.yml` has +neither. A full-line comment is still skipped, and a `#` with no space before +it, as in `nixpkgs#trealla`, is not a comment. After a refused line only the +exit is guaranteed. What the gate reads next may be garbled: a refused +`"x" # note` still opens the quote tracker. The exit is 1 either way. + +`read_graph` refuses nothing, so it cannot rely on refusal. It opens a job on +any ` name:` line followed by whitespace or the end of the line, which is +wider than the exact header `ci_steps` accepts. Reading too wide only adds jobs +`pages-deploy` must need, which fails closed. Reading too narrow is what lost +`gate`. + **What this does not close, stated rather than guarded.** Once a counted step is reached, the gate trusts that it runs the script it names. An earlier step in the same job that rewrote `scripts/checkers.sh` or its table would defeat diff --git a/context/test-evidence/tooling.md b/context/test-evidence/tooling.md index 40ec45f9..e489adda 100644 --- a/context/test-evidence/tooling.md +++ b/context/test-evidence/tooling.md @@ -1121,3 +1121,59 @@ restored. All three now exit 1, naming the first misplaced line. **Refused loudly, not read:** an indentless block list, a block list at indent 8, and a quoted top-level key such as `"on":`. Each is valid YAML, and each is named by line. + +### A comment on a read line is refused, and the matchers share one string — `scripts/check-ci-graph.py` (improvise-j49r) + +**What broke (P2, fail-open).** Found in the delta review of #228 at +`5738e8e0`. `_readable` checked the comment-stripped line, while `_JOB.match` +in both `ci_steps` and `read_graph` ran on the raw line. A header +` gate: # skipped on purpose` was therefore accepted and never opened. Its +`if: false` went to the previous job, and `read_graph` did not list `gate`. +Planted before ` wasm:` in the real `ci.yml`, with `kb` given `needs: gate`, +the gate exited 0: "pages-deploy needs all 7 other jobs; all 9 suites run". + +**The audit found a second instance.** The stripper cut `A: "x # y"` to +`A: "x`, so `_opens_quote` held the following lines open, and a job-level +`if: false` after them was never read. Planted under `kb` in the real +`ci.yml`, this also exited 0 at `5738e8e0`. + +**Fix.** A line containing ` #` is refused, with its own message, and the +stripper is deleted, so every matcher reads the raw line. `read_graph` opens a +job on `_JOB_OPEN` (` name:` followed by whitespace or the end of the line). +That is wider than `_JOB`, because it refuses nothing. The argument is in +`context/code/tooling.md`. + +**Red first** against `5738e8e0`: +- the suite case for the header: `saw []`; +- the `read_graph` case for the header: `saw []`; +- the `"x # y"` case at indent 10: `saw []`. + +The `"x" # note` case read correctly at `5738e8e0`, because the stripper cut it +cleanly. It guards the alternative of reading raw lines without refusing: under +that alternative the raw value `"x" # note` opens the quote tracker. Two old +cases were changed. The positive "own row" case lost its trailing comment, and +that form is now its own refusal case. The w0j4 padded-list control lost its +`# c` and is still a control. + +**Replayed on the real `ci.yml`.** Each plant was mutated in, run and +restored, and the file was compared byte for byte (sha1 `a00288a8…`). Four +plants were run: j49r as reviewed, j49r with `gate` also in pages-deploy's +`needs`, `"x # y"` under `kb`, and `"x" # note` under `kb`. At `5738e8e0` the +first and third exited 0. Now all four exit 1 and name the line. + +**Rivals, each a source edit run against the real `--self-test` (47 cases).** +The review's eleven still fail where the review thread said they would. The +continuation rival fails two cases, the needs flag five, block-list one, +transitive one, flow-only two, `owner + 1` one, `if_jobs` kb-only five, +`_readable` always True fifteen, quoted `_NEEDS_VALUE` one, and each rjaa rival +three. Four rivals are new: +- Raw lines, no refusal: fails the run-line, `"x" # note` and `"x # y"` cases. +- The stripped line everywhere, no refusal (the other option): fails all four + suite cases. +- Refusal only inside `jobs:` at indent 8 or less: fails the indent-10 + `"x # y"` case alone. +- `read_graph` opening on the exact `_JOB`: fails the `read_graph` case alone. + +The header suite case does not separate the raw-without-refusal rival. There +the exact `_JOB` on the raw line refuses the header anyway, and the complaint +has the same text. diff --git a/scripts/check-ci-graph.py b/scripts/check-ci-graph.py index 29a9bd5b..eb6ee127 100755 --- a/scripts/check-ci-graph.py +++ b/scripts/check-ci-graph.py @@ -160,7 +160,7 @@ def ci_steps(text): readable = not in_jobs or indent > 8 or _readable(raw, indent, in_steps, in_needs, job_keyed) if _COMMENT.search(raw) or not readable: - unread.append(n) + unread.append((n, bool(_COMMENT.search(raw)))) if raw.startswith("defaults:"): workflow_flags.add("workflow defaults") m = _JOB.match(raw) @@ -252,7 +252,10 @@ def _unrun(s): "ci_suites in scripts/checkers.sh") -def _unread(n): +def _unread(n, comment=False): + if comment: + return (f"ci.yml line {n} has a ` #`, which YAML may read as a comment; this gate " + "reads no comments, so put it on a line of its own") return (f"ci.yml line {n} is a key or `needs:` value this gate cannot read as a " "plain name, so it cannot trust the graph; spell it plainly") @@ -261,7 +264,7 @@ def suite_verdict(suites, text): """The complaints: suites no counted step runs, and every step or line that does not count, each named so the fix is obvious rather than silent.""" steps, stray, unread = ci_steps(text) - out, ran, missing = [_unread(n) for n in unread], set(), set() + out, ran, missing = [_unread(n, c) for n, c in unread], set(), set() for s in steps: m = _CI_CALL.match(s["run"] or "") if not m: @@ -318,11 +321,11 @@ _NEEDS_GATE = ["job `kb`'s `checkers.sh --ci` step is conditional (needs gate, w "so it does not count"] + _NO_PW -def _unreadable(name, text, *markers, rest=()): +def _unreadable(name, text, *markers, rest=(), comment=False): """A case whose workflow spells a key or a `needs:` value the gate must refuse.""" lines = text.split("\n") at = [next(i for i, l in enumerate(lines, 1) if m in l) for m in markers] - return (name, _REQ, text, [_unread(n) for n in at] + list(rest)) + return (name, _REQ, text, [_unread(n, comment) for n in at] + list(rest)) # (name, suites to require, workflow text, the complaints it must draw). Every @@ -334,7 +337,7 @@ SUITE_SELF_TEST = [ _unreadable("a trailing comment on the counted run: line is refused (j49r)", _wf(_BUILD, _job("kb", _KB_RUN.rstrip("\n") + " # the kb row\n")), - "# the kb row", rest=[_stray(9)] + _NO_PW), + "# the kb row", rest=[_stray(9)] + _NO_PW, comment=True), ("a job that never calls its row leaves its suites unrun", _REQ, _wf(_BUILD, _job("kb", " - run: echo nothing\n")), _NO_PW), @@ -540,18 +543,18 @@ SUITE_SELF_TEST = [ _wf(_BUILD, " test:\n runs-on: x\n", " gate: # skipped on purpose\n if: false\n steps:\n - run: echo\n", _job("kb", _KB_RUN, head=" needs: gate\n")), - "gate: # skipped"), + "gate: # skipped", comment=True), _unreadable("a closed quote followed by a comment is refused (j49r)", _wf(_BUILD, _job("kb", _KB_RUN, head=" env:\n A: \"x\" # note\n") + " if: false\n", " after:\n steps:\n - run: echo \"z\"\n"), - '"x" # note', rest=[_stray(11)] + _NO_PW), + '"x" # note', rest=[_stray(11)] + _NO_PW, comment=True), _unreadable("a # inside a quoted value, below indent 8, is refused (j49r)", _wf(_BUILD, _job("kb", _KB_RUN + " - uses: x\n with:\n" " foo: \"x # y\"\n") + " if: false\n", " after:\n steps:\n - run: echo \"z\"\n"), - '"x # y"', + '"x # y"', comment=True, rest=["job `kb`'s `checkers.sh --ci` step is conditional (job if), " "so it does not count"] + _NO_PW), From 9dc8a643b2ea08957acb1fbea6cea53762f123af Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Wed, 23 Sep 2026 18:20:36 -0700 Subject: [PATCH 09/12] check-ci-graph: a multi-line quoted value is refused, and the quote tracker is deleted (6zf1) The tracker closed a multi-line quote on an escaped \" or a doubled '', so a later quote reopened it over a job's real if:. Both styles exited 0 on the real ci.yml. The opening line is refused instead; _opens_quote's escape handling is now load-bearing and has a case per style. Argument: context/code/tooling.md. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/code/tooling.md | 43 +++++++++++++---- context/test-evidence/tooling.md | 66 +++++++++++++++++++++++++ scripts/check-ci-graph.py | 83 +++++++++++++++++++++----------- 3 files changed, 154 insertions(+), 38 deletions(-) diff --git a/context/code/tooling.md b/context/code/tooling.md index d1c09a7a..d23ffa4e 100644 --- a/context/code/tooling.md +++ b/context/code/tooling.md @@ -295,10 +295,10 @@ The gate reads no shell. It counts a step only when all of these hold: away skips the call as surely as one on its own job (improvise-bbwa). The only YAML it models is what makes a line structure rather than content. A -key with an inline value owns every following line indented deeper, and an -open quote owns every line until it closes. So a heredoc, a folded or plain -continuation, a quoted value or a whole fake job inside a top-level block -scalar is skipped as content. Any line outside a YAML comment that mentions +key with an inline value owns every following line indented deeper. So a +heredoc, a folded or plain continuation, or a whole fake job inside a +top-level block scalar is skipped as content. A quoted value must close on its +own line, or the line is refused (improvise-6zf1, below). Any line outside a YAML comment that mentions `scripts/checkers.sh` and is not a counted step is **reported**, not ignored. The union of the counted jobs' rows must cover `checkers.sh --list`. @@ -358,12 +358,35 @@ There were two ways to make the matchers agree: string, so no two matchers can read different versions of it. So the stripper is deleted, not kept consistent. -It costs one valid style: a trailing comment, or a ` #` inside a quoted value, -on any line that is not block-scalar or quoted content. The real `ci.yml` has -neither. A full-line comment is still skipped, and a `#` with no space before -it, as in `nixpkgs#trealla`, is not a comment. After a refused line only the -exit is guaranteed. What the gate reads next may be garbled: a refused -`"x" # note` still opens the quote tracker. The exit is 1 either way. +This refuses some valid styles, on any line that is not block-scalar content: +- a trailing comment on a step, such as `- uses: actions/checkout@v7 # pin`; +- a one-line `run: echo "a # b"`; +- a quoted `with:` value containing ` #`. + +This costs future readability, not correctness. The real `ci.yml` has none of +them. A full-line comment is still skipped, and a `#` with no space before it, +as in `nixpkgs#trealla`, is not a comment. After a refused line only the exit +is guaranteed: what the gate reads next may be garbled, and the exit is 1 +either way. + +**A quoted value that does not close on its own line is refused, and the quote +tracker is deleted** (improvise-6zf1). The tracker held a multi-line quoted +value open until the next line containing its quote character. YAML does not +close on an escaped `\"` inside double quotes, or on a doubled `''` inside +single quotes. So the tracker closed early, a later content line such as +`b: "` opened a phantom string, and that string swallowed the job's real +`if:`. Both quote styles exited 0 on the real `ci.yml`. This was the fourth +review round whose fail-open lived in quote or comment handling. Closing the +tracker correctly would have meant modelling YAML's quote rules one more time, +so the gate stops reading multi-line quotes at all. The real `ci.yml` has +none. + +The opening line is still judged by `_opens_quote`, and that check is now +load-bearing: a value it wrongly calls closed is not refused, and its +continuation lines are read as structure. A `name: "a\"` followed by +`needs: ok` would then overwrite a job's real `needs: gate`. So `_opens_quote` +drops `\\.` before looking for the closing `"`, and `''` before looking for +the closing `'`, and a case covers each. `read_graph` refuses nothing, so it cannot rely on refusal. It opens a job on any ` name:` line followed by whitespace or the end of the line, which is diff --git a/context/test-evidence/tooling.md b/context/test-evidence/tooling.md index e489adda..ef5ffeb7 100644 --- a/context/test-evidence/tooling.md +++ b/context/test-evidence/tooling.md @@ -1177,3 +1177,69 @@ three. Four rivals are new: The header suite case does not separate the raw-without-refusal rival. There the exact `_JOB` on the raw line refuses the header anyway, and the complaint has the same text. + +### A multi-line quoted value is refused, and the quote tracker is gone — `scripts/check-ci-graph.py` (improvise-6zf1) + +**What broke (P2, fail-open, predates j49r).** Found in the delta review of #228 +at `588094b7`. The tracker closed a multi-line quoted value at the first line +containing its quote character, including an escaped `\"` or a doubled `''`, +which YAML does not treat as a close. A later content line such as `b: "` then +opened a phantom string that swallowed the job's real `if: false`. The review +planted a `gate` job whose `name:` spans lines, with `kb` given `needs: gate` +and `gate` added to pages-deploy's `needs`. yq-go read `gate.if == false`, and +the gate exited 0 in both quote styles. + +**Fix.** A line whose quoted value does not close on its own line is refused, +with its own message. The tracker is deleted. The real `ci.yml` has no +multi-line quoted value (scanned with `_ANY_KEY` and `_opens_quote`: 0). + +**Red first** against `588094b7`: both plant cases (double and single) gave +`saw []`. + +**New cases, and why each is there.** +- The two plants. Each now draws the refusal, and also "conditional (needs + gate, which has if)", because with the tracker gone the job's `if:` is read. +- An opening line ending in an escaped `\"`, and one ending in a doubled `''`, + each followed by `needs: ok` and a readable closing line (`x: y"`). yq-go + reads each as one string, with `kb.needs == "gate"`. If `_opens_quote` + called either value closed, nothing would be refused, and the gate would + read `needs: ok` in place of kb's real need. These cases are what make + `_opens_quote`'s escape handling load-bearing and tested. +- Two refused styles the review listed: a trailing comment on a step, and + `run: echo "a # b"`. Both were already refused by the j49r rule, so these + cases document behaviour rather than fix a bug. The third style, a quoted + `with:` value containing ` #`, was already the j49r indent-10 case. + +**Changed cases.** +- "an exact run: line inside a multi-line quoted value does not count" is now + "a run: line inside a multi-line quoted value: the quote is refused". It + draws the refusal of the opening line and a plain refusal of the closing + `b"`. Only the exit is guaranteed after a refused line. +- "a closed quote followed by a comment is refused (j49r)" now reads the + job-level `if:` correctly. Its expectation is the refusal plus "conditional + (job if)", which is what the j49r evidence wanted but could not have while + the tracker existed. + +**Replayed on the real `ci.yml`.** I planted the review's three shapes: double +quote, single quote, and double quote with the reopening line at indent 4. At +`588094b7` each exited 0. Now each exits 1, naming the opening line. All seven +plants from both rounds were restored byte for byte (sha1 `a00288a8…`). + +**Rivals, each a source edit run against the real `--self-test` (53 cases).** +All 18 fail: +- the review's eleven; +- the four from j49r; +- three new ones: + - a multi-line quote is not refused: fails the five 6zf1 cases; + - `_opens_quote` ignores `\"`: fails the escaped-`\"` case alone; + - `_opens_quote` ignores `''`: fails the doubled-`''` case alone. + +The j49r rival "no comment refusal" now fails four cases, not three: the +`"x" # note` case is also caught by the quote refusal, because the raw value +`"x" # note` does not end in its quote. + +**Checked, not a finding.** A flow value (`{…}` or `[…]`) that continues onto +another line is not refused. yq-go rejects a continuation at a lesser +indentation (`did not find expected ',' or '}'`), which is loud. At the key's +own indentation the continuation is read as a key, which fails closed: a +spurious `if:`, or a `needs:` value the exact `_NEEDS_VALUE` refuses. diff --git a/scripts/check-ci-graph.py b/scripts/check-ci-graph.py index eb6ee127..524a8944 100755 --- a/scripts/check-ci-graph.py +++ b/scripts/check-ci-graph.py @@ -129,7 +129,7 @@ def ci_steps(text): job, in_steps, job_flags, step = None, False, set(), None in_jobs, job_keyed = False, False workflow_flags = set() - owner, owner_step, quote = None, None, None + owner, owner_step = None, None needs, if_jobs, in_needs = {}, set(), False def close_step(): @@ -138,12 +138,6 @@ def ci_steps(text): for n, raw in enumerate(text.split("\n"), 1): indent = len(raw) - len(raw.lstrip()) - if quote is not None: - if quote in raw: - quote = None - if "scripts/checkers.sh" in raw: - stray.append(n) - continue if owner is not None and (not raw.strip() or indent > owner): if "scripts/checkers.sh" in raw: stray.append(n) @@ -159,8 +153,11 @@ def ci_steps(text): else: readable = not in_jobs or indent > 8 or _readable(raw, indent, in_steps, in_needs, job_keyed) - if _COMMENT.search(raw) or not readable: - unread.append((n, bool(_COMMENT.search(raw)))) + k = _ANY_KEY.match(raw) + opens = bool(k) and _opens_quote(_inline_value(k.group(3))) + if _COMMENT.search(raw) or opens or not readable: + unread.append((n, "comment" if _COMMENT.search(raw) else + "quote" if opens else "plain")) if raw.startswith("defaults:"): workflow_flags.add("workflow defaults") m = _JOB.match(raw) @@ -207,14 +204,9 @@ def ci_steps(text): stray.append(n) elif "scripts/checkers.sh" in raw: stray.append(n) - k = _ANY_KEY.match(raw) if k and _inline_value(k.group(3)): - value = _inline_value(k.group(3)) - if _opens_quote(value): - quote = value[0] - else: - owner = len(k.group(1)) + (2 if raw.lstrip().startswith("- ") else 0) - owner_step = step if m and m.group(1) == "run" else None + owner = len(k.group(1)) + (2 if raw.lstrip().startswith("- ") else 0) + owner_step = step if m and m.group(1) == "run" else None close_step() for s in steps: s["flags"] |= workflow_flags @@ -252,8 +244,11 @@ def _unrun(s): "ci_suites in scripts/checkers.sh") -def _unread(n, comment=False): - if comment: +def _unread(n, kind="plain"): + if kind == "quote": + return (f"ci.yml line {n} opens a quoted value that continues onto another line; " + "this gate reads no multi-line quotes, so keep it on one line") + if kind == "comment": return (f"ci.yml line {n} has a ` #`, which YAML may read as a comment; this gate " "reads no comments, so put it on a line of its own") return (f"ci.yml line {n} is a key or `needs:` value this gate cannot read as a " @@ -321,11 +316,11 @@ _NEEDS_GATE = ["job `kb`'s `checkers.sh --ci` step is conditional (needs gate, w "so it does not count"] + _NO_PW -def _unreadable(name, text, *markers, rest=(), comment=False): +def _unreadable(name, text, *markers, rest=(), kind="plain"): """A case whose workflow spells a key or a `needs:` value the gate must refuse.""" lines = text.split("\n") at = [next(i for i, l in enumerate(lines, 1) if m in l) for m in markers] - return (name, _REQ, text, [_unread(n, comment) for n in at] + list(rest)) + return (name, _REQ, text, [_unread(n, kind) for n in at] + list(rest)) # (name, suites to require, workflow text, the complaints it must draw). Every @@ -337,7 +332,7 @@ SUITE_SELF_TEST = [ _unreadable("a trailing comment on the counted run: line is refused (j49r)", _wf(_BUILD, _job("kb", _KB_RUN.rstrip("\n") + " # the kb row\n")), - "# the kb row", rest=[_stray(9)] + _NO_PW, comment=True), + "# the kb row", rest=[_stray(9)] + _NO_PW, kind="comment"), ("a job that never calls its row leaves its suites unrun", _REQ, _wf(_BUILD, _job("kb", " - run: echo nothing\n")), _NO_PW), @@ -355,10 +350,10 @@ SUITE_SELF_TEST = [ _wf(_BUILD, _job("kb", " - run: echo\n ./scripts/checkers.sh --ci kb\n")), [_stray(10)] + _NO_PW), - ("an exact run: line inside a multi-line quoted value does not count", _REQ, - _wf(_BUILD, _job("kb", " - name: \"a\n" - " run: ./scripts/checkers.sh --ci kb\n b\"\n")), - [_stray(10)] + _NO_PW), + _unreadable("a run: line inside a multi-line quoted value: the quote is refused (6zf1)", + _wf(_BUILD, _job("kb", " - name: \"a\n" + " run: ./scripts/checkers.sh --ci kb\n b\"\n")), + '- name: "a', kind="quote", rest=[_unread(11)]), ("a whole job inside a top-level block scalar does not count", _REQ, _wf(_BUILD) + "notes: |\n kb:\n steps:\n" @@ -543,21 +538,53 @@ SUITE_SELF_TEST = [ _wf(_BUILD, " test:\n runs-on: x\n", " gate: # skipped on purpose\n if: false\n steps:\n - run: echo\n", _job("kb", _KB_RUN, head=" needs: gate\n")), - "gate: # skipped", comment=True), + "gate: # skipped", kind="comment"), _unreadable("a closed quote followed by a comment is refused (j49r)", _wf(_BUILD, _job("kb", _KB_RUN, head=" env:\n A: \"x\" # note\n") + " if: false\n", " after:\n steps:\n - run: echo \"z\"\n"), - '"x" # note', rest=[_stray(11)] + _NO_PW, comment=True), + '"x" # note', kind="comment", + rest=["job `kb`'s `checkers.sh --ci` step is conditional (job if), " + "so it does not count"] + _NO_PW), _unreadable("a # inside a quoted value, below indent 8, is refused (j49r)", _wf(_BUILD, _job("kb", _KB_RUN + " - uses: x\n with:\n" " foo: \"x # y\"\n") + " if: false\n", " after:\n steps:\n - run: echo \"z\"\n"), - '"x # y"', comment=True, + '"x # y"', kind="comment", rest=["job `kb`'s `checkers.sh --ci` step is conditional (job if), " "so it does not count"] + _NO_PW), + _unreadable("a trailing comment on a step is refused (j49r)", + _wf(_BUILD, _job("kb", " - uses: actions/checkout@v7 # pin\n" + _KB_RUN)), + "# pin", kind="comment"), + + _unreadable("a one-line run: with a ` #` inside quotes is refused (j49r)", + _wf(_BUILD, _job("kb", " - run: echo \"a # b\"\n" + _KB_RUN)), + '"a # b"', kind="comment"), + + _unreadable("a multi-line double-quoted value is refused, escaped quote and all (6zf1)", + _wf(_BUILD, " gate:\n runs-on: x\n name: \"a\n it\\\" is\n b: \"\n" + " if: false\n steps:\n - run: echo \"z\"\n", + _job("kb", _KB_RUN, head=" needs: gate\n")), + 'name: "a', kind="quote", rest=_NEEDS_GATE), + + _unreadable("a multi-line single-quoted value is refused, doubled quote and all (6zf1)", + _wf(_BUILD, " gate:\n runs-on: x\n name: 'a\n it''s\n b: '\n" + " if: false\n steps:\n - run: echo 'z'\n", + _job("kb", _KB_RUN, head=" needs: gate\n")), + "name: 'a", kind="quote", rest=_NEEDS_GATE), + + _unreadable("an opening line ending in an escaped \\\" still opens a quote (6zf1)", + _wf(_BUILD, _GATE, _job("kb", _KB_RUN, head=" needs: gate\n name: \"a\\\"\n" + " needs: ok\n x: y\"\n")), + 'name: "a', kind="quote"), + + _unreadable("an opening line ending in a doubled '' still opens a quote (6zf1)", + _wf(_BUILD, _GATE, _job("kb", _KB_RUN, head=" needs: gate\n name: 'it''\n" + " needs: ok\n x: y'\n")), + "name: 'it", kind="quote"), + ("a YAML comment mentioning checkers.sh is fine", _REQ, _wf(_BUILD, _job("kb", " # every suite goes through scripts/checkers.sh\n" " - run: ./scripts/checkers.sh --ci kb\n")), []), From 7ae871cb1e2f7a9ca5e3ccf5e8e1b970ee85ae57 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Wed, 23 Sep 2026 18:38:35 -0700 Subject: [PATCH 10/12] check-ci-graph: every value it reads is one of a few one-line shapes, or refused (0wn3) A multi-line flow value (- name: [a, "b ... run: ... c: x"]) is one string to go-yaml, PyYAML and Psych, but the gate counted the inner run:. Rather than refuse one more construct, values are now an allowlist: empty, a block header, a closed quote, a one-line flow with no quotes or nesting, or a plain scalar not starting with an indicator. The real ci.yml needs nothing else. Argument: context/code/tooling.md. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/code/tooling.md | 51 ++++++++++++++++++++- context/test-evidence/tooling.md | 61 +++++++++++++++++++++++++ scripts/check-ci-graph.py | 77 +++++++++++++++++++++++++++----- 3 files changed, 176 insertions(+), 13 deletions(-) diff --git a/context/code/tooling.md b/context/code/tooling.md index d23ffa4e..b5bc44e7 100644 --- a/context/code/tooling.md +++ b/context/code/tooling.md @@ -297,8 +297,9 @@ The gate reads no shell. It counts a step only when all of these hold: The only YAML it models is what makes a line structure rather than content. A key with an inline value owns every following line indented deeper. So a heredoc, a folded or plain continuation, or a whole fake job inside a -top-level block scalar is skipped as content. A quoted value must close on its -own line, or the line is refused (improvise-6zf1, below). Any line outside a YAML comment that mentions +top-level block scalar is skipped as content. Every other line's value must be +one of a few shapes that end on their own line, or the line is refused +(improvise-0wn3, below). Any line outside a YAML comment that mentions `scripts/checkers.sh` and is not a counted step is **reported**, not ignored. The union of the counted jobs' rows must cover `checkers.sh --list`. @@ -388,6 +389,52 @@ continuation lines are read as structure. A `name: "a\"` followed by drops `\\.` before looking for the closing `"`, and `''` before looking for the closing `'`, and a case covers each. +**Every value the gate reads must be one of a few shapes, each ending on its +own line** (improvise-0wn3). The 6zf1 fix refused a multi-line *quote* and +left a multi-line *flow* value (`[`, `{`) alone. The argument for leaving it +was that a flow continuation line is read as a key, so it fails closed. That +argument was wrong. It covered the closing line it imagined, not one written +to look like a step key: + +``` + - name: [a, "b + run: ./scripts/checkers.sh --ci kb + c: x"] +``` + +go-yaml, PyYAML and Psych all read this step as one `name` string with no +`run:`. The gate read the middle line as the step's `run:` and counted it. So +did the single-quoted form, a flow map, a flow opened on a line of its own, and +an anchored or tagged flow (`&a [x, "b`, `!!seq [x, "b`). All exited 0. + +This was the fifth review round in a row, and each fail-open was a construct +the gate did not *recognise* but also did not *refuse*. So the check is now an +allowlist over values, not a list of refused constructs. `_line_value` takes the +value a line carries: a key's, a list item's, or a whole line of its own. That +value must be one of: +- empty (a block follows); +- a block-scalar header (`|`, `>`, with chomping and indentation indicators), + whose body the owner rule skips; +- a quoted scalar that closes on the same line (`_opens_quote`); +- a one-line flow with no quotes, no nesting and no `#` inside it, such as + `needs: [test, wasm]`; +- a plain scalar that does not start with a YAML indicator (`[ ] { } & * ! | > + % @ \` ? ,`). + +Anything else is refused, whatever it is. The real `ci.yml` needs nothing +more: 153 plain values, 41 empty, 2 one-line flows and 5 block headers, with 0 +refused. + +The obvious narrower fix, "a flow value must end in its closer", is not enough. +`[a, "b]` ends in `]` while its quote is still open, and a case pins that. + +This is an allowlist over *values*, and it applies on every line the gate +reads, at any depth. The *keys* at indent 8 or less inside `jobs:` were already +allowlisted by `_readable` (w0j4, rjaa). A deeper key is not shape-checked, +because its value is. So a quoted key deep in a job, such as `"FOO": bar` under +`env:`, is now refused, because the gate cannot tell it from an unclosed quote. +That is a valid style refused loudly, beside the others in this section. + `read_graph` refuses nothing, so it cannot rely on refusal. It opens a job on any ` name:` line followed by whitespace or the end of the line, which is wider than the exact header `ci_steps` accepts. Reading too wide only adds jobs diff --git a/context/test-evidence/tooling.md b/context/test-evidence/tooling.md index ef5ffeb7..969f4450 100644 --- a/context/test-evidence/tooling.md +++ b/context/test-evidence/tooling.md @@ -1243,3 +1243,64 @@ another line is not refused. yq-go rejects a continuation at a lesser indentation (`did not find expected ',' or '}'`), which is loud. At the key's own indentation the continuation is read as a key, which fails closed: a spurious `if:`, or a `needs:` value the exact `_NEEDS_VALUE` refuses. + +### Every value is one of a few one-line shapes — `scripts/check-ci-graph.py` (improvise-0wn3) + +**What broke (P2, fail-open).** Found in the delta review of #228 at +`9dc8a643`. A multi-line flow value was not refused: + +``` + - name: [a, "b + run: nix shell nixpkgs#trealla --command ./scripts/checkers.sh --ci kb + c: x"] +``` + +go-yaml, PyYAML 6.0.3 and Psych 5.2.2 read this step as a single `name` string +with no `run:`. The gate read the middle line as the step's `run:` and counted +it. The single-quoted form and a flow map did the same. All three exited 0 on +the real `ci.yml`. + +**This retracts the 6zf1 entry's "Checked, not a finding" note above.** That +note argued a flow continuation "fails closed", but it only considered closing +lines that are not spelled like a step key. + +**Fix: an allowlist over values, not one more refused construct.** Every value +the gate reads must be one of: empty; a block-scalar header; a quoted scalar +closed on its line; a one-line flow with no quotes, nesting or `#`; or a plain +scalar not starting with a YAML indicator. That covers a key's value, a list +item's, or a line of its own. Anything else is refused. The argument is in +`context/code/tooling.md`. + +The real `ci.yml` has 153 plain values, 41 empty, 2 one-line flows and 5 block +headers, and none is refused. When a line is both an unreadable key and an +unreadable value, the key refusal's message wins, so w0j4's cases keep their +diagnosis. + +**Red first** against `9dc8a643`. All seven new cases gave `saw []`: +- the review's three: `[` double-quoted, `[` single-quoted, `{`; +- `[a, "b]`, which ends in its closer with its quote still open. This is what + the review's proposed fix ("does not close on the same line") would pass if + it were read as "ends in `]`"; +- a flow opened on a line of its own, under an empty `name:`. yq-go reads it + as one string too; +- an anchored flow (`&a [x, "b`) and a tagged one (`!!seq [x, "b`), each + confirmed with yq-go to swallow the `run:`. + +**Replayed on the real `ci.yml`.** I replaced kb's checkers step with each of +the review's three plants. At `9dc8a643` all three exit 0. Now all three exit +1, naming the opening line. All ten plants from the four rounds exit 1, and +the file was restored byte for byte (sha1 `a00288a8…`). + +**Rivals, each a source edit run against the real `--self-test` (60 cases).** +All 22 fail. The review's sixteen are my 01–11, the comment rival, the +`read_graph` rival, the old quote-refusal rival (now "value allowlist removed", +which fails 12 cases), and both `_opens_quote` rivals. The four new ones: +- a flow need only end in its closer: fails `[a, "b]` alone; +- any flow start allowed: fails the five flow cases; +- only a key's value is checked: fails the own-line flow case alone; +- anchors and tags read as plain: fails the anchored and tagged cases. + +**A valid style newly refused.** A quoted key below the `_readable` depth, such +as `"FOO": bar` under `env:`, is refused, because the gate cannot tell it from +an unclosed quote. Checked directly: refused, and the plain `FOO: bar` control +is accepted. diff --git a/scripts/check-ci-graph.py b/scripts/check-ci-graph.py index 524a8944..d6abc87d 100755 --- a/scripts/check-ci-graph.py +++ b/scripts/check-ci-graph.py @@ -108,6 +108,29 @@ def _inline_value(rest): return rest.strip() +_FLOW_LINE = re.compile(r"""^(\[[^\[\]{}"'#]*\]|\{[^\[\]{}"'#]*\})$""") +_BLOCK_HEADER = re.compile(r"^[|>][-+0-9]*$") + + +def _plain_value(value): + """Whether a value is one of the few shapes the gate reads, all ending on their own + line; anything else is refused: context/code/tooling.md (improvise-0wn3).""" + if value == "" or _BLOCK_HEADER.match(value) or _FLOW_LINE.match(value): + return True + if value[0] in "\"'": + return not _opens_quote(value) + return value[0] not in "[]{}&*!|>%@`?," + + +def _line_value(raw): + """The value a line carries: a key's, a list item's, or the whole line's.""" + k = _ANY_KEY.match(raw) + if k: + return _inline_value(k.group(3)) + body = raw.strip() + return body[2:].strip() if body.startswith("- ") else body + + def _opens_quote(value): """True when a quoted inline value is not closed on its own line.""" if value[:1] not in ("'", '"') or len(value) < 2: @@ -154,10 +177,10 @@ def ci_steps(text): readable = not in_jobs or indent > 8 or _readable(raw, indent, in_steps, in_needs, job_keyed) k = _ANY_KEY.match(raw) - opens = bool(k) and _opens_quote(_inline_value(k.group(3))) - if _COMMENT.search(raw) or opens or not readable: + shaped = _plain_value(_line_value(raw)) + if _COMMENT.search(raw) or not shaped or not readable: unread.append((n, "comment" if _COMMENT.search(raw) else - "quote" if opens else "plain")) + "plain" if not readable else "value")) if raw.startswith("defaults:"): workflow_flags.add("workflow defaults") m = _JOB.match(raw) @@ -245,9 +268,10 @@ def _unrun(s): def _unread(n, kind="plain"): - if kind == "quote": - return (f"ci.yml line {n} opens a quoted value that continues onto another line; " - "this gate reads no multi-line quotes, so keep it on one line") + if kind == "value": + return (f"ci.yml line {n} has a value that is not one plain line (a multi-line " + "quote or flow, a nested or quoted flow, an anchor, alias or tag); spell " + "it plainly") if kind == "comment": return (f"ci.yml line {n} has a ` #`, which YAML may read as a comment; this gate " "reads no comments, so put it on a line of its own") @@ -312,6 +336,7 @@ def _stray(n): _GATE = " gate:\n if: false\n steps:\n - run: echo\n" _KB_RUN = " - run: ./scripts/checkers.sh --ci kb\n" +_INNER_RUN = " run: ./scripts/checkers.sh --ci kb\n" _NEEDS_GATE = ["job `kb`'s `checkers.sh --ci` step is conditional (needs gate, which has if), " "so it does not count"] + _NO_PW @@ -353,7 +378,7 @@ SUITE_SELF_TEST = [ _unreadable("a run: line inside a multi-line quoted value: the quote is refused (6zf1)", _wf(_BUILD, _job("kb", " - name: \"a\n" " run: ./scripts/checkers.sh --ci kb\n b\"\n")), - '- name: "a', kind="quote", rest=[_unread(11)]), + '- name: "a', kind="value", rest=[_unread(11)]), ("a whole job inside a top-level block scalar does not count", _REQ, _wf(_BUILD) + "notes: |\n kb:\n steps:\n" @@ -567,23 +592,53 @@ SUITE_SELF_TEST = [ _wf(_BUILD, " gate:\n runs-on: x\n name: \"a\n it\\\" is\n b: \"\n" " if: false\n steps:\n - run: echo \"z\"\n", _job("kb", _KB_RUN, head=" needs: gate\n")), - 'name: "a', kind="quote", rest=_NEEDS_GATE), + 'name: "a', kind="value", rest=_NEEDS_GATE), _unreadable("a multi-line single-quoted value is refused, doubled quote and all (6zf1)", _wf(_BUILD, " gate:\n runs-on: x\n name: 'a\n it''s\n b: '\n" " if: false\n steps:\n - run: echo 'z'\n", _job("kb", _KB_RUN, head=" needs: gate\n")), - "name: 'a", kind="quote", rest=_NEEDS_GATE), + "name: 'a", kind="value", rest=_NEEDS_GATE), _unreadable("an opening line ending in an escaped \\\" still opens a quote (6zf1)", _wf(_BUILD, _GATE, _job("kb", _KB_RUN, head=" needs: gate\n name: \"a\\\"\n" " needs: ok\n x: y\"\n")), - 'name: "a', kind="quote"), + 'name: "a', kind="value"), _unreadable("an opening line ending in a doubled '' still opens a quote (6zf1)", _wf(_BUILD, _GATE, _job("kb", _KB_RUN, head=" needs: gate\n name: 'it''\n" " needs: ok\n x: y'\n")), - "name: 'it", kind="quote"), + "name: 'it", kind="value"), + + _unreadable("a flow list whose quote runs onto the next lines is refused (0wn3)", + _wf(_BUILD, _job("kb", " - name: [a, \"b\n" + _INNER_RUN + " c: x\"]\n")), + 'name: [a, "b', kind="value"), + + _unreadable("the same with a single-quoted string (0wn3)", + _wf(_BUILD, _job("kb", " - name: [a, 'b\n" + _INNER_RUN + " c: x']\n")), + "name: [a, 'b", kind="value"), + + _unreadable("the same as a flow map (0wn3)", + _wf(_BUILD, _job("kb", " - name: {a: \"b\n" + _INNER_RUN + " c: x\"}\n")), + 'name: {a: "b', kind="value"), + + _unreadable("a flow list that ends in its closer with a quote still open is refused (0wn3)", + _wf(_BUILD, _job("kb", " - name: [a, \"b]\n" + _INNER_RUN + " c: x\"]\n")), + 'name: [a, "b]', kind="value"), + + _unreadable("a flow value opened on a line of its own is refused too (0wn3)", + _wf(_BUILD, _job("kb", " - name:\n [a, \"b\n" + _INNER_RUN + + " c: x\"]\n")), + '[a, "b', kind="value"), + + _unreadable("an anchored multi-line flow is refused (0wn3)", + _wf(_BUILD, _job("kb", " - name: &a [x, \"b\n" + _INNER_RUN + " c: x\"]\n")), + 'name: &a', kind="value"), + + _unreadable("a tagged multi-line flow is refused (0wn3)", + _wf(_BUILD, _job("kb", " - name: !!seq [x, \"b\n" + _INNER_RUN + + " c: x\"]\n")), + 'name: !!seq', kind="value"), ("a YAML comment mentioning checkers.sh is fine", _REQ, _wf(_BUILD, _job("kb", " # every suite goes through scripts/checkers.sh\n" From 1bec7c0fb9f5b3c68d3c5088f815536929d5a5bd Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Wed, 23 Sep 2026 19:00:08 -0700 Subject: [PATCH 11/12] check-ci-graph: a line holding a YAML-only line break or a control character is refused (xpzh) The gate split on \n alone while YAML also breaks at U+0085, U+2028 and U+2029, so an if: after one was invisible and the gate exited 0. Every line, comments and block-scalar bodies included, is now checked for C0 (but tab), DEL, C1 and U+2028/2029. CR is folded by the one read path, read_workflow, and file-based controls pin that. Argument: context/code/tooling.md. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/code/tooling.md | 31 ++++++++++++++++++ context/test-evidence/tooling.md | 48 +++++++++++++++++++++++++++ scripts/check-ci-graph.py | 56 ++++++++++++++++++++++++++++++-- 3 files changed, 132 insertions(+), 3 deletions(-) diff --git a/context/code/tooling.md b/context/code/tooling.md index b5bc44e7..943409f5 100644 --- a/context/code/tooling.md +++ b/context/code/tooling.md @@ -435,6 +435,37 @@ because its value is. So a quoted key deep in a job, such as `"FOO": bar` under `env:`, is now refused, because the gate cannot tell it from an unclosed quote. That is a valid style refused loudly, beside the others in this section. +**A line holding a line break YAML knows and the gate does not, or a control +character, is refused** (improvise-xpzh). The gate splits the text on `\n` +only. YAML also breaks lines at U+0085 (NEL), U+2028 and U+2029, so +`runs-on: x if: false` was one line to the gate and two to go-yaml. +The job's `if:` was invisible, and the gate exited 0. The same break inside a +full-line comment, or inside a `run: |` body, ends that line for YAML too. A +body line `echo a if: false` makes the `if:` a real job key. + +So the check runs on **every** line, before the comment and owner skips, and +refuses: +- C0 controls other than tab (CR included); +- DEL; +- the whole C1 block, which contains U+0085; +- U+2028 and U+2029. + +CR is the one break the read folds. `read_workflow` opens with Python's +universal newlines, which turn CR and CRLF into `\n`, and a lone CR is a line +break to YAML too, so the two agree. A CR that survives to the split can only +come from a read that did not fold it, and it is refused. Two self-test +controls write real files, one CRLF and one lone CR, and read them back +through `read_workflow`. The code path is the point: a string handed straight +to `suite_verdict` never meets the fold. + +**Why not refuse all non-ASCII,** which would be the simpler allowlist. The +real `ci.yml` has 27 non-ASCII characters (em dash, `§`, arrows) in its +comments, so it would have to be rewritten to pass. The set of characters that +changes line structure is not open-ended, though. It is YAML's own `b-char`: +LF and CR, plus NEL, LS and PS in YAML 1.1. So refusing that set, and the +controls around it, is complete by the spec's definition, not a list that +grows with each review. + `read_graph` refuses nothing, so it cannot rely on refusal. It opens a job on any ` name:` line followed by whitespace or the end of the line, which is wider than the exact header `ci_steps` accepts. Reading too wide only adds jobs diff --git a/context/test-evidence/tooling.md b/context/test-evidence/tooling.md index 969f4450..c3b04189 100644 --- a/context/test-evidence/tooling.md +++ b/context/test-evidence/tooling.md @@ -1304,3 +1304,51 @@ which fails 12 cases), and both `_opens_quote` rivals. The four new ones: as `"FOO": bar` under `env:`, is refused, because the gate cannot tell it from an unclosed quote. Checked directly: refused, and the plain `FOO: bar` control is accepted. + +### A line break YAML knows and the gate does not is refused — `scripts/check-ci-graph.py` (improvise-xpzh) + +**What broke (P2, fail-open).** Found in the delta review of #228 at +`7ae871cb`. The gate splits on `\n` only, but YAML also breaks lines at U+0085, +U+2028 and U+2029. `runs-on: ubuntu-latest if: false` on the real +`kb` job is one line to the gate and two to go-yaml. yq-go reads +`kb.if == false` with each of the three characters, and the gate exited 0. + +**Fix.** `_UNSAFE_CHAR` is checked on every line, before the comment and owner +skips. It covers C0 except tab, DEL, all of C1 (which holds U+0085), U+2028 and +U+2029. The workflow is read through `read_workflow`, whose universal newlines +fold CR and CRLF. The argument, including why all non-ASCII is not refused, is +in `context/code/tooling.md`. The real `ci.yml` has 0 CR bytes, no BOM and 27 +printable non-ASCII characters, none of them in the refused set. + +**Red first** against `7ae871cb`. All five new suite cases gave `saw []`: +- one per break character, each on a `runs-on:` line; +- a U+2028 inside a full-line comment; +- a U+2028 inside a `run: |` body, on the job's last step. yq-go ends the + block there and reads the `if:` as a job key. + +**Controls.** Two cases write a real file and read it back through +`read_workflow`: +- CRLF line ends are accepted with no complaint; +- a lone CR is a line break, so the following `if:` makes kb conditional, as + YAML reads it. + +**Replayed on the real `ci.yml`.** The three plants, one per character, on +kb's `runs-on:`, exit 0 at `7ae871cb`. Now all three exit 1, naming line 255. +All 13 plants from the five rounds exit 1. The file was written with +`newline=""` so the characters landed literally, and it was restored byte for +byte (sha1 `a00288a8…`). + +**Rivals, each a source edit run against the real `--self-test` (65 suite +lists, 2 files).** All 27 fail. The 22 from earlier rounds fail as before. +Five are new: + +| Rival | Fails | +|---|---| +| The character check removed | the five new cases | +| C1 allowed | the U+0085 case alone | +| Comments not checked | the comment case alone | +| Owned content not checked | the block-scalar case, and the comment case | +| The read does not fold CR (`newline=""`) | both file controls | + +The owned-content rival also fails the comment case because `owner` still +holds the preceding `runs-on:` key's column when that line is checked. diff --git a/scripts/check-ci-graph.py b/scripts/check-ci-graph.py index d6abc87d..ae60bcc3 100755 --- a/scripts/check-ci-graph.py +++ b/scripts/check-ci-graph.py @@ -15,9 +15,11 @@ here, and what a row runs is asked of `--would-run` (improvise-maax, st8q). `--self-test` checks the checker: each case is a graph this must refuse. """ +import os import re import subprocess import sys +import tempfile WORKFLOW = ".github/workflows/ci.yml" DEPLOY = "pages-deploy" @@ -84,6 +86,7 @@ _TOP_KEY = re.compile(rf"^{_NAME}:(\s|$)") _NEEDS_VALUE = re.compile(rf"^(|{_NAME}|\[\s*{_NAME}(\s*,\s*{_NAME})*\s*\])$") _NEEDS_ITEM = re.compile(rf"^ - {_NAME}$") _COMMENT = re.compile(r"\s#") +_UNSAFE_CHAR = re.compile("[\x00-\x08\x0b-\x1f\x7f-\x9f\u2028\u2029]") def _readable(line, indent, in_steps, in_needs, job_keyed): @@ -160,6 +163,9 @@ def ci_steps(text): steps.append(step) for n, raw in enumerate(text.split("\n"), 1): + if _UNSAFE_CHAR.search(raw): + unread.append((n, "char")) + continue indent = len(raw) - len(raw.lstrip()) if owner is not None and (not raw.strip() or indent > owner): if "scripts/checkers.sh" in raw: @@ -268,6 +274,9 @@ def _unrun(s): def _unread(n, kind="plain"): + if kind == "char": + return (f"ci.yml line {n} holds a control character or a line break YAML sees and " + "this gate does not (U+0085, U+2028, U+2029); remove it") if kind == "value": return (f"ci.yml line {n} has a value that is not one plain line (a multi-line " "quote or flow, a nested or quoted flow, an anchor, alias or tag); spell " @@ -309,6 +318,12 @@ def suite_verdict(suites, text): return out +def read_workflow(path): + """The workflow's text. Universal newlines fold CR and CRLF into the one break the + gate splits on; every other break YAML knows is refused (improvise-xpzh).""" + return open(path, encoding="utf8").read() + + def list_suites(): """The suites, from checkers.sh itself, so its naming rule has one home.""" out = subprocess.run(["scripts/checkers.sh", "--list"], capture_output=True, @@ -336,6 +351,7 @@ def _stray(n): _GATE = " gate:\n if: false\n steps:\n - run: echo\n" _KB_RUN = " - run: ./scripts/checkers.sh --ci kb\n" +_YAML_ONLY_BREAKS = ("\x85", "\u2028", "\u2029") _INNER_RUN = " run: ./scripts/checkers.sh --ci kb\n" _NEEDS_GATE = ["job `kb`'s `checkers.sh --ci` step is conditional (needs gate, which has if), " "so it does not count"] + _NO_PW @@ -640,12 +656,38 @@ SUITE_SELF_TEST = [ + " c: x\"]\n")), 'name: !!seq', kind="value"), + *[_unreadable(f"a U+{ord(sep):04X} inside a line is refused (xpzh)", + _wf(_BUILD, " kb:\n runs-on: x" + sep + " if: false\n steps:\n" + _KB_RUN), + sep, kind="char") + for sep in _YAML_ONLY_BREAKS], + + _unreadable("a line break inside a block scalar's body is refused too (xpzh)", + _wf(_BUILD, _job("kb", _KB_RUN + " - run: |\n echo a\u2028 if: false\n")), + "echo a", kind="char"), + + _unreadable("a line break inside a full-line comment is refused too (xpzh)", + _wf(_BUILD, " kb:\n runs-on: x\n # note\u2028 if: false\n steps:\n" + + _KB_RUN), "# note", kind="char"), + ("a YAML comment mentioning checkers.sh is fine", _REQ, _wf(_BUILD, _job("kb", " # every suite goes through scripts/checkers.sh\n" " - run: ./scripts/checkers.sh --ci kb\n")), []), ] +# (name, file bytes as text, the complaints it must draw), read back through +# read_workflow, because what a CR means depends on that read (improvise-xpzh). +FILE_SELF_TEST = [ + ("control: CRLF line ends are folded by the read and accepted (xpzh)", + _wf(_BUILD, _job("kb", _KB_RUN)).replace("\n", "\r\n"), []), + + ("control: a lone CR is a line break, as YAML reads it (xpzh)", + _wf(_BUILD, " kb:\n runs-on: x\r if: false\n steps:\n" + _KB_RUN), + ["job `kb`'s `checkers.sh --ci` step is conditional (job if), so it does not count"] + + _NO_PW), +] + + # (name, workflow text, the complaints it must draw). SELF_TEST = [ ("a job nobody gates is named", @@ -690,19 +732,27 @@ def self_test(): got = suite_verdict(suites, text) if got != want: failures.append(f" {name}: expected {want}, saw {got}") + for name, content, want in FILE_SELF_TEST: + with tempfile.TemporaryDirectory() as d: + path = os.path.join(d, "ci.yml") + with open(path, "w", encoding="utf8", newline="") as f: + f.write(content) + got = suite_verdict(_REQ, read_workflow(path)) + if got != want: + failures.append(f" {name}: expected {want}, saw {got}") if failures: print("ci-graph self-test FAILED:", file=sys.stderr) print("\n".join(failures), file=sys.stderr) return 1 - print(f"ci-graph self-test: {len(SELF_TEST)} graphs and " - f"{len(SUITE_SELF_TEST)} suite lists judged") + print(f"ci-graph self-test: {len(SELF_TEST)} graphs, " + f"{len(SUITE_SELF_TEST)} suite lists and {len(FILE_SELF_TEST)} files judged") return 0 def main(): if "--self-test" in sys.argv[1:]: return self_test() - text = open(WORKFLOW, encoding="utf8").read() + text = read_workflow(WORKFLOW) jobs, needs = read_graph(text) if len(jobs) < 2: print(f"error: {WORKFLOW}: found {len(jobs)} jobs; the parse is wrong", From 901f7fadc5e9f346c387a3da0b108c276c902d67 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Wed, 23 Sep 2026 23:13:36 -0700 Subject: [PATCH 12/12] check-ci-graph: a defaults: run: shell: no longer disqualifies a step (the user's ruling) Main's PR #287 added a workflow-level defaults shell, and the refusal made every suite read as unrun. The two cases that pinned it are deleted; one shows a step under workflow and job defaults is counted. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/code/tooling.md | 13 +++++++++++-- context/test-evidence/tooling.md | 22 ++++++++++++++++++++++ scripts/check-ci-graph.py | 21 +++++---------------- 3 files changed, 38 insertions(+), 18 deletions(-) diff --git a/context/code/tooling.md b/context/code/tooling.md index 943409f5..8458afee 100644 --- a/context/code/tooling.md +++ b/context/code/tooling.md @@ -288,8 +288,6 @@ The gate reads no shell. It counts a step only when all of these hold: (improvise-1be2); - the call names its own job; - neither the step nor the job carries `if:`, `continue-on-error:` or `shell:`; -- no `defaults:` block, at workflow or job level, covers it, since that can - change the shell the step runs under; - no job it `needs:`, directly or through other jobs, carries a job-level `if:`. GitHub skips a job whose need was skipped, so a false `if:` two jobs away skips the call as surely as one on its own job (improvise-bbwa). @@ -472,6 +470,17 @@ wider than the exact header `ci_steps` accepts. Reading too wide only adds jobs `pages-deploy` must need, which fails closed. Reading too narrow is what lost `gate`. +**A `defaults: run: shell:` does not disqualify a step** (the user's ruling, +PR #228). An earlier version refused to count any step under a workflow- or +job-level `defaults:`, because a default shell can change what the call runs +under. The user called that "a silly check". It also broke on the day +`ci.yml` gained a real one: PR #287 set a workflow-level +`shell: nix develop .#ci --command bash …`, and every suite then read as run by +no CI step. A default shell is how the workflow chooses its environment, not a +way to skip a call. A shell that failed to run the call would fail the step, red +in CI. A step's own `shell:` still disqualifies it. That rule was not part of +the ruling. + **What this does not close, stated rather than guarded.** Once a counted step is reached, the gate trusts that it runs the script it names. An earlier step in the same job that rewrote `scripts/checkers.sh` or its table would defeat diff --git a/context/test-evidence/tooling.md b/context/test-evidence/tooling.md index c3b04189..8d5e1c00 100644 --- a/context/test-evidence/tooling.md +++ b/context/test-evidence/tooling.md @@ -1352,3 +1352,25 @@ Five are new: The owned-content rival also fails the comment case because `owner` still holds the preceding `runs-on:` key's column when that line is checked. + +### A `defaults: run: shell:` no longer disqualifies a step — `scripts/check-ci-graph.py` (the user's ruling on #228) + +**What changed, and why.** The gate refused to count any step under a +workflow- or job-level `defaults:` block. The user ruled it "a silly check". +It also broke on contact with main: PR #287 added a workflow-level +`defaults: run: shell: nix develop .#ci --command bash …`, and on the merged +tree every suite read as run by no CI step. The refusal is removed. + +**Cases.** Two cases pinned the refusal: "a job's defaults: can change the +shell, so it does not count" and "workflow-level defaults: disqualifies every +step". Both are deleted. The new case "a step under a defaults: run: shell: is +counted" carries both a workflow-level default in #287's shape and a +job-level one, and expects no complaint. + +**Red first.** Against the code before the removal, the new case drew both +steps as "conditional (workflow defaults)" and kb's as "(job defaults, workflow +defaults)", so both rows went unrun. + +**Kept.** A step's own `shell:` still disqualifies it; the ruling did not cover +that. The w0j4 case refusing a quoted `"defaults":` key is about key spelling, +and it stands. diff --git a/scripts/check-ci-graph.py b/scripts/check-ci-graph.py index ae60bcc3..88ef1510 100755 --- a/scripts/check-ci-graph.py +++ b/scripts/check-ci-graph.py @@ -78,7 +78,6 @@ _STEP_KEY = re.compile(r"^ (?:- | )([A-Za-z_][\w-]*):(.*)$") _ANY_KEY = re.compile(r"^(\s*)(?:- )?([A-Za-z_][\w-]*):(.*)$") _CI_CALL = re.compile(r"^(?:nix shell \S+ --command )?\./scripts/checkers\.sh --ci ([\w-]+)$") _DISQUALIFY = ("if", "continue-on-error", "shell") -_JOB_DISQUALIFY = _DISQUALIFY + ("defaults",) _NAME = r"[A-Za-z_][\w-]*" @@ -154,7 +153,6 @@ def ci_steps(text): steps, stray, unread = [], [], [] job, in_steps, job_flags, step = None, False, set(), None in_jobs, job_keyed = False, False - workflow_flags = set() owner, owner_step = None, None needs, if_jobs, in_needs = {}, set(), False @@ -187,8 +185,6 @@ def ci_steps(text): if _COMMENT.search(raw) or not shaped or not readable: unread.append((n, "comment" if _COMMENT.search(raw) else "plain" if not readable else "value")) - if raw.startswith("defaults:"): - workflow_flags.add("workflow defaults") m = _JOB.match(raw) if m: close_step() @@ -211,7 +207,7 @@ def ci_steps(text): needs[job] = {x.strip() for x in value.strip("[]").split(",") if x.strip()} if key == "if": if_jobs.add(job) - if key in _JOB_DISQUALIFY: + if key in _DISQUALIFY: job_flags.add(key) for s in steps: if s["job"] == job: @@ -238,7 +234,6 @@ def ci_steps(text): owner_step = step if m and m.group(1) == "run" else None close_step() for s in steps: - s["flags"] |= workflow_flags s["flags"] |= {f"needs {j}, which has if" for j in _needed(s["job"], needs) & if_jobs} return steps, stray, unread @@ -544,16 +539,10 @@ SUITE_SELF_TEST = [ ["job `kb`'s `checkers.sh --ci` step is conditional (job if), so it does not count"] + _NO_PW), - ("a job's defaults: can change the shell, so it does not count", _REQ, - _wf(_BUILD, _job("kb", " - run: ./scripts/checkers.sh --ci kb\n", - head=" defaults:\n run:\n shell: python {0}\n")), - ["job `kb`'s `checkers.sh --ci` step is conditional (job defaults), " - "so it does not count"] + _NO_PW), - - ("workflow-level defaults: disqualifies every step", ["assemble-stamp"], - "defaults:\n run:\n shell: python {0}\n" + _wf(_BUILD), - ["job `build`'s `checkers.sh --ci` step is conditional (workflow defaults), " - "so it does not count", _unrun("assemble-stamp")]), + ("a step under a defaults: run: shell: is counted (the user's ruling)", _REQ, + "defaults:\n run:\n shell: nix develop .#ci --command bash -e {0}\n" + + _wf(_BUILD, _job("kb", _KB_RUN, head=" defaults:\n run:\n shell: bash\n")), + []), ("continue-on-error does not count", _REQ, _wf(_BUILD, _job("kb", " - run: ./scripts/checkers.sh --ci kb\n"