refactor(ci): resolve the docs-only scope once and gate the contract - #3237
Conversation
|
Claude finished @kyle-sexton's task in 1m 32s —— View job Security review (
|
|
Last security-reviewed head: |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 66e100acdf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Claude finished @kyle-sexton's task
|
The docs-only short-circuit let six jobs each run scripts/check-docs-only.sh
as their own `id: scope` step and gate their inner work on
`steps.scope.outputs.docs_only != 'true'`. That put the contract at 48
references in one file, every one of them an inverse-polarity test against a
string flag. Each consumer had to independently know that the value is the
string `true`, that the safe direction is to RUN the suite, and that the
negation must be spelled that exact way. Nothing executable held that
knowledge; a prose comment did.
The asymmetry is what made it worth fixing. A consumer that wrote
`== 'false'` where it meant `!= 'true'` would skip its lane whenever the
detector emitted anything unexpected — a lane reporting success without
running, which is the false-green shape docs/conventions/liveness-assertion/
names. A consumer that forgot the gate entirely would merely run always.
A new `scope` job now runs the detector once and publishes `run_full`. The
output is positive on purpose: a consumer writes `run_full == 'true'` to do
the work and `run_full == 'false'` to report it not applicable. Both are
plain equality against a value that `${{ <expr> != 'true' }}` can render only
as `true` or `false`, so the two forms are exact complements and a new lane
has no negation to spell wrong.
The fail-safe direction is preserved. The detector already emitted
`docs_only=false` for every condition it could resolve but not classify, and
follows every emit with an immediate `exit 0`, so a run that cannot complete
leaves the flag unset. `run_full` renders an unset flag as `true`, and the
full suite runs; the same path covers push events, where the detect step does
not run at all. `continue-on-error` on that step contributes the other half —
it keeps a failed detector from failing the resolving job and skipping every
consumer. The self-test is deliberately NOT `continue-on-error`, so a
detector nobody verified still turns the lane red.
scripts/check-docs-only-gate.sh asserts nine properties against ci.yml rather
than restating them in a comment: the detector is resolved exactly once; the
published expression is exactly the fail-closed one; the step that absorbs
failure is the step that actually invokes the detector; the self-test is
unweakened; every consumer reference is one of two whole sanctioned shapes;
every aggregator-feed override names a step that actually carries the gate;
no consumer carries a job-level condition; every reader declares the `needs`
edge; and at least one consumer actually reads the output.
The last three are the ones that are easy to miss. An override on an UNGATED
step would replace a real failure with `success` on every docs-only diff —
forgetting an override is fail-closed, adding an unpaired one is not. A
job-level `if: always()` on a consumer lets it run when the resolver did not
succeed, where the output is empty, both sanctioned forms are false, and
every gated step skips alongside its own not-applicable reporter: green
having run nothing. A missing `needs` edge produces the same empty output by
a different route.
The gate is deliberately hostile to being satisfied by anything other than
the real thing. Comments never stand in for the expressions they quote.
Block-scalar bodies are read for content but never for structure, so a `run:`
script can neither masquerade as a step condition nor hide a second
resolution. References are detected loosely — any mention of the output, in
any spelling or case — and only then held to the exact sanctioned shape, so a
spelling the gate does not model reads as unsanctioned rather than becoming
invisible to it. Zero references is a defect, not a vacuous pass. Any shape
it cannot parse exits 2 rather than 0.
Its suite breaks each property in turn and requires the gate to name it, and
proves against the real detector that both an outright abort and an
unwritable output file leave the flag unset — the half of the guarantee that
lives below the workflow.
Writing that last case turned up a false sentence in the detector's own
header, which claimed a non-zero exit on an unwritable output file. The
behavior is correct and the guarantee is unaffected — it rests on the flag
being unset, not on an exit status — but a change whose whole subject is
replacing a load-bearing comment with something executable should not ship
beside a comment known to be false, so that header is corrected here. It is
the only edit to that file and it changes no behavior.
Behavior is unchanged in which jobs run and which steps they execute. The
aggregator is untouched: the intentional docs-only step skip is still mapped
to `success` in the CHECK_RESULTS feed, now keyed on the same single output
as the gates it mirrors.
Refs #2914
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XtbWChCVfUWAv1Pi5Qk2hA
Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
66e100a to
4607369
Compare
|
Claude finished @cursor[bot]'s task in 3m 22s —— View job Code review (
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
|
||
| case "$kind" in | ||
| stepif) | ||
| if [[ "$bare" != "$WORK_FORM" && "$bare" != "$SKIP_FORM" ]]; then |
There was a problem hiding this comment.
Confirming Codex's P2 finding here (originally posted against 66e100a; still applies at the current HEAD 4607369, so I'm re-anchoring it to today's line numbers).
The stepif branch treats WORK_FORM (== 'true') and SKIP_FORM (== 'false') as equally sanctioned for any step — it has no notion of which of the two a given step is supposed to be. The only place that distinction gets checked is §5b (THE FEED MIRRORS A REAL GATE, lines 591-615), and that only fires for steps that also have a CHECK_RESULTS feed entry — i.e. hygiene's steps. For jobs with no feed at all (plugin-gate, miro-plugin, video-extraction, ai-briefing-build, course-digest-extraction), a step that's supposed to run the real work (if: ... == 'true') can be silently flipped to == 'false' and --check still exits 0, because SKIP_FORM is unconditionally accepted as sanctioned syntax regardless of which step carries it.
I verified this by reading the parser rather than re-running the harness (this lane's Bash execution is sandboxed to a narrow allowlist that doesn't include ad-hoc script execution here), but the logic is unambiguous: nothing outside §5b ties a step's identity to which of the two forms it's allowed to carry, and §5b's gated_ordinals is only populated from WORK_FORM matches, so a step that got flipped away from WORK_FORM simply drops out of that set rather than tripping a defect.
Concretely: flip plugin-gate's "Set up Node" step (line 1115) from run_full == 'true' to run_full == 'false' and check-docs-only-gate.sh --check should still report the contract satisfied, while the lane now only sets up Node on docs-only diffs — the inverse of what the step needs.
This doesn't undermine the PR's core claim (the contract — one resolution, fail-closed default, no negation to misspell — is real and well-verified), but it's a gap in what the gate can prove: it verifies syntactic conformance to one of two legal shapes, not that a given step picked the correct one of the two. Worth either a follow-up gate property (e.g., a per-job invariant that at least one WORK_FORM step exists whenever the job also has a SKIP_FORM reporter, or an explicit reporter/work-step naming convention the gate can key on) or an explicit call-out in the header's "WHAT IS CHECKED" list that this class of error is out of scope.
|
Claude has reviewed this PR 1 time. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
1 participant
Closes #3159
Summary
The docs-only short-circuit lets the heavy CI lanes report an honest evaluated-and-not-applicable success on a diff confined to the documentation allowlist. That contract lived as a step output re-derived independently inside six jobs —
hygiene,plugin-gate,miro-plugin,video-extraction,ai-briefing-build,course-digest-extraction— each runningscripts/check-docs-only.shas its ownid: scopestep and gating its work onsteps.scope.outputs.docs_only != 'true'.That put the contract at 48 references in one file, every one an inverse-polarity test against a string flag. Each consumer had to independently know three things: that the value is the string
true, that the safe direction is to run the suite, and that the negation must be spelled that exact way. Nothing executable held that knowledge; a prose comment did.The asymmetry is what made it worth fixing. A consumer that wrote
== 'false'where it meant!= 'true'would skip its lane whenever the detector emitted anything unexpected — a lane reporting success without having run, the false-green shapedocs/conventions/liveness-assertion/names. A consumer that forgot the gate entirely would merely run always, which is only wasteful. The comment existed because of that asymmetry, and 48 hand-maintained copies of a comment is the wrong mechanism for it.Fix
A new
scopejob resolves the diff once per workflow and publishesrun_full. Every former reference now readsneeds.scope.outputs.run_full; the six duplicated detector invocations and their six duplicated self-test steps are gone.The output is positive on purpose. A consumer writes
run_full == 'true'to do the work andrun_full == 'false'to report it not applicable. Both are plain equality against a value that${{ <expr> != 'true' }}can render only astrueorfalse, so the two forms are exact complements and a new lane has no negation to spell wrong. This is the fork the triage brief left open, resolved the way the brief defaulted it.The fail-safe direction is preserved, and now checked rather than asserted:
docs_only=truerun_full=falsedocs_only=falserun_full=true$GITHUB_OUTPUT, and!= 'true'renders an unset flag astruecontinue-on-errorscripts/check-docs-only.sh's behavior is unchanged: it still emitsdocs_only=falsefor every condition it can resolve but cannot classify. Its header comment is corrected, though — see Verification.scripts/check-docs-only-gate.shis a new gate asserting nine properties againstci.yml: the detector is resolved exactly once; the published expression is exactly the fail-closed one; the step that absorbs failure is the step that actually invokes the detector; the self-test is unweakened; every consumer reference is one of two whole sanctioned shapes; every aggregator-feed override names a step that actually carries the gate; no consumer carries a job-level condition; every reader declares theneedsedge; and at least one consumer actually reads the output. It runs in its owndocs-only-gatelane, wired intoci-status.needs.Three of those are worth spelling out, because they are the asymmetric ones — the direction that fails open is not the direction you would guess:
skippedreaches the aggregator and reds it. Adding one for a step that is not gated is fail-open — it replaces that step's real outcome withsuccesson every docs-only diff. Only the second direction needed a check.needsand therefore runs only when the resolver succeeded. Anif: always()orif: ${{ !cancelled() }}— an idiom this very file uses on the aggregate — lets the job run anyway, with an empty output. Both forms are then false, so every gated step skips alongside its own not-applicable reporter, and the lane reports success having run nothing. The condition does not have to mention the output to do this, so the check does not read what it says.needsedge produces the same empty output by a different route.The gate is deliberately hostile to being satisfied by anything other than the real thing, because a gate that can be evaded is the same nominal closure it was written to remove:
run:script can neither masquerade as a step condition nor hide a second resolution.check-lane-coverage.sh.Behavior is unchanged in which jobs run and which steps they execute. The aggregator is untouched: the intentional docs-only step skip is still mapped to
successin theCHECK_RESULTSfeed, now keyed on the same single output as the gates it mirrors.The trade-off this makes, stated plainly. Six previously independent jobs now declare
needs: [scope], so they are no longer fault-isolated from each other: ifscopefails, all six are skipped rather than running and failing on their own terms. That is the unavoidable cost of resolving the scope once, and it was weighed rather than overlooked:skipped, whichci-statusrejects, andci-statuscarriesif: ${{ !cancelled() }}so a failed dependency does not skip the aggregate itself — it still runs and turns red. There is no input under which this change turns a red gate green.scopeis a checkout, onecontinue-on-errordetector call, and the detector's own suite. The only thing that can fail it is the self-test, and a broken detector should stop the lanes that would otherwise trust it.The cost is real, and if it proves annoying in practice the honest fix is to make
scopecheaper still, not to re-scatter the resolution.Verification
scripts/check-docs-only-gate.test.shscripts/check-docs-only.test.sh(detector behavior unchanged)scripts/check-lane-coverage.test.shscripts/check-manifest-duplicate-keys.py,scripts/check-hook-wiring-liveness.shscripts/check-lane-coverage.sh --checkci-status.needsscripts/aggregate-hygiene-results.sh --self-testmainactionlintonci.ymlshellcheck -xon both new scriptsscripts/check-shell-portability.sh --paths(both new scripts)scripts/check-silent-skips.shEach of the gate's 46 cases breaks exactly one property of a known-good fixture and requires the gate to name that property, so the gate is proven to detect what it claims rather than merely passing. The known-good fixture deliberately carries the shapes a structural reader is fragile on — a comment inside a job body, a job key with a trailing comment, a block scalar whose body looks like workflow structure, a quoted flow
needs:, a reusable-workflow job — because a gate only ever tested against tidy input is only ever known to work on tidy input.Two cases are behavioral rather than structural and close the half of the fail-closed guarantee that lives below the workflow: against the real
check-docs-only.sh, both an outright usage abort and an unwritable$GITHUB_OUTPUTleavedocs_onlygenuinely unset, which is exactly whatrun_fullrelies on.Writing that second case turned up a false claim in the detector's own header: it said the script exits non-zero on an unwritable output file. It does not —
emit()does not check its redirect and the script does not useset -e. The behavior is correct and the fail-closed guarantee is unaffected, because the guarantee rests on the flag being unset rather than on an exit status; only the comment was wrong. Since this change is precisely about replacing a load-bearing comment with something executable, shipping it next to a comment known to be false would be self-defeating, so the header is corrected here — the only edit to that file, and a comment-only one. The property it now describes is the one the new test asserts.actionlintis also what caught the missingneedsedges during development — it rejectsneeds.scope.*from a job with no such edge — so that class cannot reach a run silently.What local runs cannot prove. GitHub expression evaluation and job-level
needssemantics do not execute locally. Thatneeds.scope.outputs.run_fullrenders as expected on a real pull-request event, on a push-to-main event where the detect step does not run, and on a genuinely docs-only diff is argued from the documented semantics and asserted structurally by the new gate — it is not observed until this PR's own CI run. This PR is not itself a docs-only diff, so its own run does not exercise the short-circuit branch either; that path is covered by the detector's suite and the gate's fixtures rather than by a live run. Reviewers should treat the first green run on this PR as the evidence for the run-full branch, and the fixtures as the evidence for the rest.Independent review. Three fresh-context reviewers checked the change with the author's rationale withheld, on two different models to decorrelate blind spots: two successive passes on the new gate's parser soundness and test quality, and one on GitHub Actions semantics and behavioral equivalence.
The semantics review returned nothing blocking, and its evidence is worth repeating because it is stronger than a reading: per-job gate counts are byte-identical across the refactor (34 work gates before and after, 6 not-applicable reporters, 6 feed overrides), so no step gained or lost a gate. It also confirmed the one genuine behavior change is in the safe direction — a detector hard-exit used to red the owning job and now runs the full suite instead — and that every abnormal
scopeoutcome resolves the merge gate red. Its two should-fix findings are addressed below and in the follow-up note.Both gate reviews found the gate evadable, and both were right. The first draft could be satisfied by prose: a commented-out output expression standing in for the real one, a sibling step lending its
continue-on-errorto the detect step, zero references reading as "all references well formed". It was rewritten so that attribution happens during a single structural pass, consumer forms are whitelisted as whole shapes, and the liveness floor is explicit.The second review then broke the rewrite, and found the deeper error. A whitelist that only fires on the spellings it recognizes does not reject the ones it does not — it cannot see them, which is the opposite of rejecting them.
needs['scope']['outputs']['run_full']andneeds.SCOPE.outputs.run_fullare both legal Actions, both resolve at runtime, and both were invisible: a lane could be switched to an inverted-polarity gate in either spelling and sail through while the innocent lanes kept the reference count healthy. Detection is now deliberately loose — any mention of the output, in any spelling or case — and only then held to the exact sanctioned shape, so an unmodelled spelling lands in the "not sanctioned" bucket instead of vanishing. The same review found a commented-out detector call satisfying the "the detector actually runs" property, an expression-valuedcontinue-on-errorevading the self-test guarantee, and two false positives that would have red-lined legitimate edits.Every one of those cases is now a named test. That churn is the point of the exercise: a gate written to replace a load-bearing comment is worth nothing if it can be satisfied by a comment.
The semantics review then found two more, both now closed and both listed among the nine properties above: the feed-override check was validating only the template's shape and never that the step it names is actually gated, which is the fail-open direction; and the "no polarity to get wrong" claim was silently conditional on
scopesucceeding, since a consumer carryingif: always()would run with an empty output and skip every branch. Two comments that overstated what was mechanically pinned were corrected rather than left to be believed.Related
Sequencing. #3158 (hoist
fetch-depthinto a checkout wrapper) touched the same file and landed first. This branch is rebased on it: thescopejob uses the./.github/actions/checkout-with-basecomposite that change introduced, rather than carrying its ownfetch-depth. Nothing here re-litigates a checkout call site.Left for a follow-up, deliberately — and it is worth someone's time. The six consumer lanes still run the
Fetch basecomposite (git fetch --prune --unshallow --no-tagsplus a base fetch) even though, after this change, the detector was their only consumer of the base ref. The semantics reviewer checked the four pure-build lanes independently and found no remaining need:plugins/knowledge/skills/video-digest/scripts/run-tests.sh,plugins/ai-briefing/skills/generate/scripts/run-tests.shandplugins/knowledge/skills/course-digest/scripts/run-tests.shcontain zero git /origin//BASE_REFreferences, and miro's four npm scripts aretsc,vitest,biomeand a bundle check.That matters more than it looks: the unshallow is the expensive step in each of those lanes, so this refactor removed the cheap per-lane duplicate (a self-test and a detector call, seconds) and left the costly one, while adding one serialized job that pays the same unshallow up front. Dropping
Fetch base— or passingfetch-base: 'false'— on those four lanes is what would pay for the new serialization.plugin-gatelooks equally dead but spans the wholeplugins/**/*.test.shcorpus and should be verified before touching.hygienegenuinely needs history, for the git-history secret scan among others.None of that is done here: it changes checkout call sites, which is a different concern from the scope contract and squarely the surface #3158 just reworked. Filing it as its own item with the evidence above is the right next step.
Generated by Claude Code