Repository navigation
Conversation
|
nihalpasham
left a comment
There was a problem hiding this comment.
- Blocker: the guard is still a text search, so it can pass when the real PR job no longer runs
just guard. - Counterexample: remove the reachable workflow step, then place the same
run:text inside a YAML block scalar or anif: falsestep. The guard stays green even though no PR path executes it. A reusable workflow that is never called has the same problem. - Required fix: parse workflow YAML structurally and walk the PR-reachable workflow graph, including
workflow_call, conditions, andcontinue-on-error. The Justfile check needs the same structural treatment instead of matching dumped text. - The local guard passes on these counterexamples, so its green result is not enforcement evidence yet.
Every guard under `scripts/` is wired twice: a step in a workflow, and a line in the Justfile's `check-guards` recipe. The Justfile states that as a contract -- "`check-guards` covers the status-guard, naming-guard and cargo-deny workflows in full" -- and nothing enforced it. `check-book-catalog-stamp.sh` landed in NVIDIA#1106 wired into `status-guard.yml` and never added to `check-guards`, so twelve of the thirteen guards ran locally and "in full" was false. The first version of this check searched both files for each script's name. NVIDIA#1133's review was right that a text search is not enforcement evidence, and gave the counterexamples: the same text inside a `run:` block that only echoes it, a step behind `if: false`, or a reusable workflow nothing calls. All three left the search green while no pull request ran the guard. This reads structure. **Workflows are a graph.** The YAML is parsed, and reachability computed: a workflow is an entry point only if it triggers on `pull_request` or `pull_request_target`; a reusable workflow counts only if a reachable job `uses:` it, resolved to a fixpoint; a job or step proven `if: false` is dropped; and anything that may `continue-on-error` is not credited, because a swallowed failure is not enforcement. **A `run:` script is parsed into commands**, and a guard counts only in command position. `echo "bash scripts/x.sh"` runs `echo`. Leading `VAR=value` assignments and wrappers (`bash`, `sh`, `env`, `.`, `source`, `exec`, `time`, `sudo`) are stepped over, comment lines are dropped, and `&&`, `||`, `;`, `|` split a line so a guard buried mid-pipeline still counts. **The Justfile gets the same treatment**, as the review asked. Recipes and their dependencies are a graph, walked from `check`, so a guard moved into a recipe `check` does not depend on stops counting. A body line prefixed with `-` is just's `continue-on-error` and does not enforce. The same command-position rule applies, so a recipe that echoes an invocation is not running it. Reachability is conservative in one direction only, on purpose. An `if:` that is not literally `false` is treated as reachable, because `github.event_name == 'pull_request'` cannot be evaluated statically -- the guard never calls a live step dead. It is strict the other way: a step that may be skipped, or may swallow its failure, is not coverage. One subtlety worth recording, since it decides whether any of this works: YAML 1.1 reads a bare `on` as the boolean true, so PyYAML hands back `{True: ...}` for `on:` written unquoted, which is how all eleven workflows here write it. `document.get("on")` is `None` on every one of them. Both keys are looked up, and a self-test refuses if no workflow appears to trigger on `pull_request` -- the shape that would otherwise make the reachable set silently empty. Verified against every case, nine that must fail and seven that must refuse: | the guard is defeated by | caught as | |---|---| | the `run:` text only echoed | executed by `just check` but by no workflow | | a step behind `if: false` | same | | only in a reusable workflow nothing calls | same | | a step with `continue-on-error: true` | same | | a job with `continue-on-error: true` | same | | moved to a workflow with no `pull_request` trigger | same | | a Justfile line prefixed with `-` | executed by a workflow but not by `just check` | | moved to a recipe `check` does not reach | same | | the Justfile echoing it | same | | broken input | refusal | |---|---| | nothing triggers on `pull_request` | parse self-test failed, naming the `on:` case | | the `check` recipe renamed | parse self-test failed | | the `check-guards` recipe renamed | parse self-test failed | | `check` no longer depends on `check-guards` | not reachable from `check` | | a job calls a workflow without `workflow_call` | it would never run | | a job calls a workflow that does not exist | named | | a workflow file is not valid YAML | named, with the parser error | | no PyYAML available | refuses with the install command | Unmutated: "OK: all 14 guard scripts are executed by a pull-request workflow (11 reachable of 11) and by `just check` (8 reachable recipes)", exit 1 on failure, all fourteen guards still pass individually, 0.09s, no cargo, no network, no toolchain. The interpreter is probed rather than assumed: the first `python3` on PATH is not always the one with PyYAML -- where Homebrew shadows the system Python it is not -- so `python3`, `/usr/bin/python3` and `python` are tried in turn and a missing parser is a refusal with the install command, never a pass. Note for review: NVIDIA#1102 also edits the Justfile, in the `test-cuda` recipe around line 105. The `check-guards` change here is around line 235, so they do not overlap textually. Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
9ff152b to
7f79090
Compare
|
Rebuilt as a structural check. Force-pushed as a single commit; the text search is gone. Workflows are a graph now. The YAML is parsed and reachability computed: a workflow is an entry point only if it triggers on A The Justfile gets the same treatment. Recipes and dependencies are a graph walked from Your three counterexamples, each now failing:
And seven refusals, so a broken parse cannot read as clean: nothing triggering on Reachability is conservative in one direction only, deliberately: an One subtlety that decides whether any of it works, recorded in the script: YAML 1.1 reads a bare One environmental assumption to flag: structural parsing needs PyYAML. The script probes 0.09s, no cargo, no network, no toolchain. All 14 guards still pass individually, and the branch merges cleanly with the rest of the open batch. |
Every guard under `scripts/` is wired twice: a step in a workflow, and a line in the Justfile's `check-guards` recipe. The Justfile states that as a contract -- "`check-guards` covers the status-guard, naming-guard and cargo-deny workflows in full" -- and nothing enforced it. `check-book-catalog-stamp.sh` landed in NVIDIA#1106 wired into `status-guard.yml` and never added to `check-guards`, so twelve of the thirteen guards ran locally and "in full" was false. ## Before -> after The first version of this check searched both files for each script's name. NVIDIA#1133's review was right that a text search is not enforcement evidence: before: grep finds "check-book-catalog-stamp.sh" in status-guard.yml -> OK: all 14 guard scripts run in both places after: the step is `echo "bash scripts/check-book-catalog-stamp.sh"` -> error: these guards are executed by `just check` but by no pull-request workflow: check-book-catalog-stamp.sh Same for a step behind `if: false`, a step or job with `continue-on-error`, a guard in a reusable workflow nothing calls, a guard moved to a workflow with no `pull_request` trigger, a Justfile line prefixed with `-`, a recipe `check` never reaches, and a Justfile line that echoes the invocation. Nine ways to defeat the old check; all nine now fail. ## How Workflow YAML is parsed and reachability computed as a graph: an entry point must trigger on `pull_request`/`pull_request_target`; a reusable workflow counts only if a reachable job `uses:` it, to a fixpoint; a job or step proven `if: false` is dropped; anything that may `continue-on-error` is not credited. A `run:` script is split into commands and a guard counts only in command position, stepping over `VAR=value` prefixes and the `bash`/`sh`/`env`/`.`/ `source`/`exec`/`time`/`sudo` wrappers. The Justfile gets the same treatment: recipes and dependencies are a graph walked from `check`, and a `-` prefix is just's `continue-on-error`. Conservative in one direction only, on purpose: an `if:` that is not literally `false` is treated as reachable, because `github.event_name == 'pull_request'` cannot be evaluated statically. The guard never calls a live step dead. It is strict the other way -- a step that may be skipped, or may swallow its failure, is not coverage. ## No third-party parser The YAML is read by a strict-subset reader in the script rather than PyYAML. PyYAML is **not** in the `actions/runner-images` preinstalled manifest, so depending on it would risk the job failing on merge, and no other guard in this family needs a third-party package -- on a repo that already works around macOS's bash 3.2 and BSD grep for the same reason. The reader covers exactly what these files use: block mappings and sequences, plain and quoted scalars, literal `|` and folded `>` block scalars with chomping indicators, flow sequences, comments, one leading document marker. Anything else -- a tab, an anchor, an alias, a tag, a flow mapping, a second document -- is a refusal, never a guess. That is what separates it from the text search: it cannot silently misread, only decline to read. Folding is implemented rather than approximated because `book.yml` folds one command across two lines; treating it as literal would invent a second command whose first token is `cuda-oxide-book`. Where PyYAML *is* installed the script uses it as an **oracle**: it compares its own parse against PyYAML's on every workflow, over exactly the fields the walk reads, and refuses on any disagreement. Locally that reports "agrees with PyYAML on all 11 workflows". So the reader is checked, not trusted, without being required. One subtlety the whole walk depends on: YAML 1.1 reads a bare `on` as the boolean true, so `on:` written unquoted is the key `True`, and `document.get("on")` is `None` on all eleven files. Both keys are looked up. ## Tests The nine defeats above were originally verified by hand-mutating this repository, which is neither repeatable nor visible in CI. They are now fixtures the guard asserts about itself on every run, alongside the controls that catch a rule grown too strict (a live step, a guard mid-pipeline, a called reusable workflow, `@`-suppressed Justfile lines) and four reader refusals. Each was confirmed to bite by breaking the implementation it guards: | implementation broken | self-test that failed | |---|---| | step `if`/`continue-on-error` check removed | a step behind `if: false` was counted | | job `continue-on-error` check removed | a job with continue-on-error was counted | | command position -> any token | the invocation only echoed was counted | | reusable-workflow fixpoint removed | a called reusable workflow was not counted | | Justfile `-` skip removed | a `-` prefixed script path was counted | | Justfile reachability walk removed | a recipe `check` does not reach was counted | | reader's tab refusal removed | the reader accepted a tab | | reader's flow-mapping refusal removed | the reader accepted a flow mapping | Unmutated: all fourteen guards pass individually, 0.02s, needs only python3 -- no cargo, no network, no third-party package. Note for review: at 835 lines this is the largest guard in the family by some margin, and that is what structural parsing costs. Say so if you would rather have the dependency and a shorter script. NVIDIA#1102 also edits the Justfile, in the `test-cuda` recipe around line 105; the `check-guards` change here is around line 235, so they do not overlap. Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
7f79090 to
d9cd089
Compare
|
Two changes since your review, both from things I should have resolved before asking. The PyYAML dependency is goneI had left it as an "environmental assumption" for you to rule on. That was wrong of me — checking it settles it: PyYAML is not in the The YAML is now read by a strict-subset reader in the script. It covers exactly what these eleven files use — block mappings and sequences, plain and quoted scalars, literal Folding is implemented rather than approximated, because Where PyYAML is installed the script uses it as an oracle — comparing its own parse against PyYAML's on every workflow, over exactly the fields the walk reads, refusing on any disagreement. Locally that reports The nine defeats are now tests, not a commentI had verified your counterexamples by hand-mutating the repository and pasting the results here. That is neither repeatable nor visible in CI, and under the tightened criteria it is not the evidence it should be. They are now fixtures the guard asserts about itself on every run, alongside controls that catch a rule grown too strict (a live step, a guard mid-pipeline, a called reusable workflow, Each was confirmed to bite by breaking the implementation it guards:
That last-but-three row is worth a note: my first All nine end-to-end defeats still fail against the real tree after the refactor. All fourteen guards pass, 0.02s, python3 only. One thing to push back on if you wantAt 835 lines this is now the largest guard in the family by a good margin, and that is what structural parsing without a dependency costs. If you would rather have the dependency and a much shorter script, say so and I will swap it — the oracle code becomes the parser and most of the file goes away. There is also a small ASCII flow of the reachability walk in the header now. |
… from CI Follow-up to the same review, from re-reading the twenty PRs closed on 2026-08-24. NVIDIA#1167 was closed partly because "the cargo-oxide compatibility finalization path remains unguarded" and NVIDIA#1169 because its instruction table "omits" families -- neither for an observed failure, both for a path the change did not enumerate. This guard had one of those. `workflow_coverage` read literal `bash scripts/check-*.sh` commands out of a `run:` script. A step that runs `just check-guards` executes every guard in that recipe, and the walk saw only the command `just`. So a workflow written that way would have every one of its guards reported as "executed by `just check` but by no pull-request workflow" -- a false failure, blocking CI, with the verdict exactly backwards. Nothing in `.github/workflows/` invokes `just` today, which is why the corpus did not show it. That is the point: the guard's job is to be right about the wiring, not about this week's wiring. A `just <recipe>` command is now resolved through the Justfile recipe graph the script already parses -- the authoritative list in that case, rather than a second one derived here -- and the guards that recipe reaches are credited to that step. The `-` prefix still does not enforce inside a recipe reached that way, and the recipe named on the command line is its own entry point, so reachability from `check` does not apply to it. Three fixtures, each failing before the change: a step running `just check-guards` credits a guard nested one dependency deep; the same step credits nothing when no Justfile is supplied; and a `-` prefixed line inside the recipe `just` runs is still not counted. `recipes_reachable_from` and `recipe_guards` are now shared by both halves rather than the walk existing twice. Unmutated: all fourteen guards pass, the nine end-to-end defeats still fail, the subset reader still agrees with PyYAML on all 11 workflows, 0.02s. Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
|
One more commit, from reading the PRs closed on 2026-08-24 back against this branch. #1167 was closed partly because "the cargo-oxide compatibility finalization path remains unguarded", and #1169 because its instruction table "omits" families — neither for an observed failure, both for a path the change did not enumerate. This guard had one of those.
Nothing under A Three fixtures, each failing before the change: a step running Everything else holds — all fourteen guards pass, the nine end-to-end defeats still fail, the subset reader still agrees with PyYAML on all 11 workflows, 0.02s. |
|
Closing this PR because the hand-built workflow, Just, and shell interpreter still cannot establish that a pull request actually executes a guard and propagates its failure.
Extending this 919-line parser until it models GitHub Actions reachability, shell failure propagation, checkout provenance, and Just semantics is substantial new architecture rather than a small maintainer correction. A replacement should make CI and local checks consume one authoritative execution path or manifest, with fail-before/pass-after mutations and an unaffected control. Please align on that design first under the acceptance criteria shared in Discord. |
What is wrong
Every guard under
scripts/is wired twice: a step in a workflow, and a line in the Justfile'scheck-guardsrecipe. The Justfile states that as a contract —— and nothing enforced it. It was already false.
check-book-catalog-stamp.shlanded in #1106 wired intostatus-guard.ymland never added tocheck-guards, so twelve of the thirteen guards ran locally and "in full" was wrong.That direction of drift is the one that happens, because forgetting the Justfile line breaks nothing visible: CI still runs the guard,
just checkstill passes, and the only symptom is that a contributor who runs the documented local gate before pushing does not run it — exactly the person the recipe exists for. The opposite direction is worse and just as quiet: a guard incheck-guardsbut in no workflow reads as enforced and never runs on a pull request.The change
scripts/check-guard-coverage.shcompares three sets — thecheck-*.shscripts on disk, the ones named in the recipe body, and the ones named under.github/workflows/— and fails on any asymmetry, with a different message per case. Adding the missing recipe line makes it pass.smoketest.shanddebug-smoketest.share deliberately out of scope: the first is the example suite with its own CI job andjust smoketestrecipe, the second a developer tool. Only thecheck-*family is a guard.Verification
check-book-catalog-stamp.shfrom the recipejust check-guardscheck-guardsbut in no workflow — plus "invoked but does not exist"status-guard.ymlcheck-guardsbut in no workflow, naming itcheck-guards:recipecheck-*.shnobody wiredUnmutated:
OK: all 14 guard scripts run in both a workflow and just check-guards, and all fourteen still pass individually, the new one included. Reads three text files — no cargo, no network, no toolchain.bash -nclean, SPDX header present, mode 755 like its siblings.Note for review
#1102 also edits the Justfile, in the
test-cudarecipe around line 105. This change is insidecheck-guardsaround line 235, so the two do not overlap textually — I verified the hunks. Happy to rebase if you would rather it land after.