Repository navigation
fix(ci): a skipped smoke-test must not satisfy the required context (#17126) - #17388
Conversation
…17126) Owner rule, 2026-09-19: nothing lands on main without passing the Docker smoke test. `smoke-test` is a required status check published by two mutually exclusive jobs -- the real build in docker-smoke-test.yml and the path-filtered shim in docker-smoke-required-context.yml. Both jobs `needs: changes`, so a `changes` job that failed or was cancelled SKIPPED both of them. GitHub counts a skipped check run as satisfying a required status check, so the requirement was met with no smoke test run at all. Both files carried a comment asserting the opposite: "if `changes` errors, `needs` skips this job rather than reporting green ... which blocks the pull request". Correct about the skip, wrong about what a skip means, and written in the one place that cannot be falsified. Same shape as #17354's `global` channel comment and #17384's floor prose -- a claim parked where nothing can test it. The shim now runs on `always()` and REFUSES when its own detector did not answer, before reporting anything. The real build now also runs when its own `changes` did not succeed: two `changes` jobs exist, one per workflow, and the shim can only speak for its own. When nothing can say whether an image could have changed, the honest answer is to build it -- a full run on a rare infrastructure failure is the correct direction for a gate nothing is allowed to land without. The three cases stay mutually exclusive, so exactly one check run named `smoke-test` ever carries a conclusion: changes ok, docker != true shim succeeds (legitimate green) changes ok, docker == true shim skips, real build reports changes NOT ok shim fails; the real workflow builds `repo_tests/docker_smoke_required_context_17126_test.py` moves the claim somewhere it can fail: 14 tests over both workflows' `if:` expressions, the shim's refusal ORDER (an `exit 1` after the success echo would report green first), and the env-var seam. With positive controls, because every assertion is a substring check against a live file and those pass for two different reasons. Mutation proof, each fix reverted in turn with the mutation confirmed applied: shim `if:` back to the pre-#17126 form -> 3 tests red by name refusal moved after the success echo -> the order test red real build's `if:` back -> 1 test red by name all reverted -> 14 passed Closes #17126
|
Warning Review limit reachedNext included review available in 45 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Repository: mrveiss/AutoBot-AI/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ SSOT Configuration Compliance: Passing🎉 No new hardcoded values of either class — Known backlog in |
Review pass (requested by coordination, not the author)Verdict: FIX-FIRST — nothing broken, but one decision needs an explicit Verified clean, by re-deriving rather than trusting the PR's own claims: the 1. AC2 as written is not what is implemented, and I think the code is right#17126 AC2 says, verbatim:
On
So a pass is producible on detector failure. The workflow comment argues the But the issue's checkbox still says the opposite, and nothing records the
I would take (a). Flagging rather than deciding. 2. The AC2 deviation and the two-publisher case are the same scenarioThese were two separate findings in my pass and they are one. On detector I could not verify how GitHub's branch-protection resolution treats that. It Stated as unverified rather than guessed. Worth either a one-line note in the 3. No test stops
|
Update to finding 2 — it is no longer "unverified"I said I could not verify how branch protection resolves two conclusions on one The repo's position is that the conflict cannot arise. So the question "what does GitHub do with two conclusions?" is one this repo has It is enforced for one of five.
There are five shims — Why that matters specifically hereIt means the scenario in my finding 2 is reachable not because GitHub's
I still recommend the implemented reading — it verifies where AC2-as-written A live instance already happened todayOn PR #17379, one SHA carried two entries for The general gap is filed as #17391. Nothing here blocks this PR beyond the |
…17126) A pre-merge review found the comment asserting something the code does not do, in the one place a future reader would trust it -- on a PR whose entire subject is a gate that reported green while checking nothing. The claim was "the three cases are mutually exclusive, so exactly one check run named `smoke-test` ever carries a conclusion". The first two are exclusive. The third is not a case: there are TWO `changes` jobs, one per workflow, and each job can only see its own. When both detectors fail, the shim reports failure AND the real build runs and reports whatever the build does -- two check runs, one context, possibly opposite, resolved by whichever finished last. The narrower invariant is the one #17126 needs and it does hold: THE SHIM CANNOT REPORT GREEN UNLESS ITS OWN DETECTOR SAID NO DOCKER PATHS CHANGED. So a disagreement between the two detectors can block a PR whose image build passed -- conservative, self-resolving on a re-run -- and can never admit one no build covered. Both comments now say that, and two tests assert it: the refusal precedes any success message, and it is conditioned on the shim's own result. The complement assumption the two detectors rest on is #17391's subject, named rather than left implicit. Refs #17126
…read the file (#17126) A pre-merge review found two weaknesses in this PR's own guard, both of the class the guard exists to catch. THE CONNECTIVE WAS NEVER ASSERTED. Every check was `"clause" in condition`, so swapping `||` for `&&` between the disjuncts would make each job's `if` false in exactly the case it must run -- reopening #17126 -- while every assertion kept passing, because each token is still present verbatim. The condition is now pinned WHOLE for both workflows. Mutation proof, applied to the real file and confirmed by `git diff --stat` before the verdict was read: `||` -> `&&` in the shim's if: FAILED test_the_condition_is_pinned_whole_not_clause_by_clause[...] 1 failed, 18 passed <- the substring tests all still passed reverted: 19 passed That the older assertions stayed green under the mutation is the finding, not a side note. THE always() CONTROL COULD NOT FAIL. It asserted against two string literals defined in the test file and never read the workflow, so it reported coverage of the exact bug class #17126 fixed while exercising nothing. It now also checks the live condition. Also documented: the `always()` widening reaches every trigger, so a `changes` failure on `merge_group` now runs the full build where it previously skipped -- a real cost at the last gate before main, and still the right direction. The shim's trigger is pull_request only, so nothing double-reports there. Refs #17126
…oes not carry (#17126) I corrected this comment once already -- it claimed the three cases were mutually exclusive, which is false -- and the correction then claimed the shim's green 'can never admit one where no build ran', which is ALSO false. Both were overstatements in the one place a reader would trust them, on a PR about a gate that reported green while checking nothing. Derived from the two live conditions, on a pull request with both detectors succeeding (S = shim's detector, R = the real build's): S.docker R.docker publishers true true real only correct false false shim only correct true false NEITHER no context is created at all false true BOTH shim green + the build's result Row 3 is #17126's hole by another door: a required check satisfies the gate by never existing, with nothing failed and nothing cancelled. Row 4 is worse than conservative -- the shim's green does not mean a build ran, and completion order decides the context. Neither row is a defect in either condition; each is correct for the value its own detector computed. One question, two detectors, an assumed agreement. Both files now carry the table, and the walk-through with proposed criteria is on #17391 -- the single-publisher problem, not closable in either condition alone. Refs #17126, #17391
…ard (#17126) Two review findings on #17388, both fixed here. `test_swapping_the_connective_would_be_caught` compared two of this module's own literals -- `_EXPECTED_CONDITIONS[_SHIM].replace("||", "&&")` against `_EXPECTED_CONDITIONS[_SHIM]` -- which is a tautology the moment the pin contains an `||`, and it never opened a workflow file. It also sat ABOVE the positive-controls banner, so its name and its position both read as a live guard while it was asserting nothing about the live condition. Renamed to say what it does, moved down with the other controls, and pointed at `_condition(_job(_SHIM))`. Reading the live file is what makes it non-vacuous: if the shim's condition ever stopped joining its disjuncts with `||`, the swap becomes a no-op and the inequality fails. That is the property being claimed -- that the whole-condition pin can see a connective where a substring check cannot. This is the same class as the finding the previous round fixed in `test_the_always_check_rejects_the_shape_that_caused_17126`: a control that never reads its subject reads as coverage and is not. Finding it twice in one file says the first fix was applied to the instance rather than to the pattern. `DOCKER_CHANGED` was declared in the step's `env:` and never read -- wired into both log paths instead of dropped, so a refusal now names the filter's answer alongside the detector's result, which is what a reader debugging a red shim needs and had to go to the job summary for. Closes #17126
1 participant
Thinking Path
Owner rule, 2026-09-19: nothing lands on
mainwithout passing the Docker smoke test.smoke-testis a required status check published by two mutually exclusive jobs — the real build in
docker-smoke-test.yml, and the path-filtered shim indocker-smoke-required-context.ymlfor pullrequests that cannot reach an image.
Both jobs
needs: changes. Achangesjob that failed or was cancelled therefore skipped both ofthem, and GitHub counts a skipped check run as satisfying a required status check. The
requirement was met with no smoke test run at all.
The part worth reading is that both files said the opposite. Each carried a comment asserting
fail-closed:
Correct that the job is skipped. Wrong about what a skip means. And written in the one place that
cannot be falsified — the same shape as #17354's
global-channel comment ("carries no per-tenantpayload of its own") and the floor prose in #17384: a claim parked where nothing can test it, which
then gets read as enforcement.
Why the fix touches the real workflow too. There are two
changesjobs, one per workflow, andeach job can only see its own. Making the shim refuse covers the case where the shim's detector
fails. If the real workflow's detector fails while the shim's succeeds with
docker == 'true',the shim correctly stands down and only the real job's condition prevents an all-skipped context. So
the real build now also runs when its own
changesdid not succeed: when nothing can say whether animage could have changed, the honest answer is to build it. That costs a full run on a rare
infrastructure failure, which is the right direction for a gate nothing is allowed to land without.
The three cases stay mutually exclusive, so exactly one check run named
smoke-testever carries aconclusion:
What Changed
.github/workflows/docker-smoke-required-context.ymlalways() && (result != 'success' || docker != 'true')and its step refuses before reporting, with the detector result passed throughenv:rather than interpolated into the shell.github/workflows/docker-smoke-test.ymlchangesdid not succeed; the stale fail-closed comment is replaced with what the platform actually doesrepo_tests/docker_smoke_required_context_17126_test.pyif:expressions, the refusal's ORDER, the env-var seam, and that both jobs still publish the one context nameVerification
The pre-push hook additionally ran every guard whose declared globs match the changed workflow —
21 repo_tests files — and reported
[pre-push OK] pytest: all relevant tests pass.Mutation proof, each fix reverted in turn, with the mutation confirmed applied (
git diff --stat)before the verdict was read:
if:back to the pre-#17126needs.changes.outputs.docker != 'true'test_the_shim_runs_even_when_its_detector_failed,..._runs_on_the_detector_failure_path,..._refuses_rather_than_reporting_greenif:back to skipping on detector failuretest_the_real_build_runs_when_its_own_detection_failedThe order assertion is the one I would not have thought to write before today: an
exit 1anywherein the script satisfies a naive check, and an
exit 1after the success echo reports the requiredcontext green and then exits non-zero.
Stated boundary, in the test's own docstring: this reads workflow YAML. It cannot prove GitHub's
required-context resolution and does not try — the platform behaviour is the premise, not the
assertion. What it holds is that neither job can reach a state where a skip is the only thing
reported.
ci-gate'sMUST_RUNlist independently refuses an all-skippedsmoke-testat mergetime, and that belt stays: a guard on the workflow text and a guard at merge time fail for different
reasons.
Process note, since it cost real work: after the mutation proof I reverted with
git checkout -- .github/workflows/, which discarded my own uncommitted fixes along with themutations. Re-applied and re-verified before committing. The rule that would have prevented it is to
commit the fix before mutating it — reverting a mutation from git only works when what is
underneath it is committed.
AC2 has been amended on the issue, so
Closesis honest — and I read the amended text beforeswitching back to it rather than taking the amendment on report. The old criterion asked for
"neither job can produce a pass" on a detector failure; the amended one asks the shim to refuse and
the real build to run, "because a detector that cannot answer is not evidence that nothing
changed." That is what this PR implements. AC1 and AC2 are ticked on the issue; AC3 is deliberately
left unticked there until this merges, since a criterion met on a branch is not met in the tree.
The residual, which is NOT in AC2's scope and is filed rather than hidden: the amended criterion
covers detector failure. Two detectors that both succeed and disagree produce either no
publisher (the context is never created — #17126's hole by another door) or two (a shim green with no
build behind it). The four-case table is now in both workflows' comments, and the walk-through with
proposed acceptance criteria is on #17391, the single-publisher problem. It is not closable inside
either condition: each is individually correct for the value its own detector computed.
I corrected my own comment twice getting to that. It first claimed the three cases were mutually
exclusive ("exactly one check run ever carries a conclusion" — false), then claimed the shim's green
"can never admit a PR no build covered" — also false, row 4. Both were overstatements in the one
place a reader would trust them, on a PR about a gate that reported green while checking nothing.
Closes #17126, not Closes — and the reason is this PR's own subject.**
AC2 reads, verbatim on the issue: "When path detection fails, neither job can produce a pass."
This implementation deliberately does the opposite — when detection fails the real build RUNS, and a
green then means a smoke test actually ran, which is the stronger guarantee. Two other sessions
independently reached the same conclusion: the code is right and the criterion should be amended.
The amendment has not happened, so
Closeswould tick a criterion the implementation contradicts —on an issue about a gate that reported green while checking nothing. The proposed amendment is on the
issue with its reasoning; the owner's criterion stays theirs to change.
Refs #17126
Model Used
Claude Opus 5 (1M context)