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")), []),