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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze
This commit is contained in:
Edward Langley
@@ -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
|
||||
|
||||
@@ -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.
|
||||
|
||||
+121
-4
@@ -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 <job>`")
|
||||
|
||||
|
||||
_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",
|
||||
|
||||
Reference in New Issue
Block a user