Uh oh!
There was an error while loading. Please reload this page.
ci(set-status): add the missing card instead of losing the race (backend#2731) - #370
Conversation
…end#2731) set-status and add-to-kanban fire on the same pull_request event and start together. set-status polled 5 x 5s for the card and failed when it was not there; add-to-kanban frequently won seconds later, so the card ended CORRECT and the check ended RED. On .github#362 the board was already in the exact state set-status would have written -- the job did its work and reported failure anyway, which is the shape that teaches people to merge past red. The old comment held the retry budget at 5 x 5s deliberately, to measure how often the race is lost rather than guess a window. That measurement arrived (#361 and #362, both within seconds), so this ends the race instead of widening the window: the job already holds a token with organization-projects: write, and addProjectV2ItemById is idempotent, so it adds the card itself when it is absent. add-to-kanban does exactly one thing -- an actions/add-to-project step -- so there is nothing to duplicate. Still fail-closed: an unresolvable PR node, or an add that returns no item id, goes red exactly as before. Only "not yet" stops reporting as "missing". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ckend#2731) Nothing covered set-pr-status.yml_s status-write block. The closing-ref suite already owns assertions about that file, so the four new checks live there. Scope stated in the file rather than implied: these read the workflow SOURCE, so they cannot prove the fallback works against the real API -- and this repo_s own caller pins @main, so the PR that lands the fix does not exercise it either. What they catch is the regression that matters: deleting the fallback, or softening the refusal back to a pass. Both are registered as WORKFLOW_MUTATIONS, per that list_s own note that rule 5 does not exempt a guard for living in YAML. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
saadqbal
left a comment
There was a problem hiding this comment.
"The card ended correct and the check ended red… nothing is broken, so the habit it teaches is to merge past red." That is the right reason to treat this as urgent rather than cosmetic. I saw the red on #361 two days ago and read it as a real failure; a check that fails while its purpose is achieved costs more than one that never ran.
The part I most want to credit is that this honours the prior decision's terms instead of overriding them. The 5×5s budget was held deliberately, with its own comment saying "widening it in the same change would hide how often the race is actually lost… the measurement tells us what the window should be — guessing does not." The measurement then arrived — #361 and #362, both lost within seconds — and the conclusion drawn from it is that a bigger number is still a guess and a longer wait still waits on a sibling workflow that may never have run. A deferred decision that actually got measured and then resolved on the evidence is rarer than it should be.
The three safety claims are each stated as checked rather than assumed, which is the right form, and I verified the one that carries the weight:
Fail-closed holds on both paths. An unresolvable PR node exits 1; an add that returns no item id exits 1. And the structure is better than the description promised — the re-check is a separate block outside the add path, so one guard covers "no add was attempted and the card is still absent" and "the add ran and produced nothing" rather than trusting the mutation's return inline. Both spellings of absence are tested (-z and = "null"), which matters because jq -r prints the literal string null and // empty does not catch that.
"The board write cannot proceed; this is not a race" in the error text is a small thing worth keeping: the log distinguishes itself from the case this PR fixes, so the next person reading a red set-status knows immediately which of the two they have.
And the scope claim is the right one to have made — an existing organization-projects: write grant, no widening, mint-scope still 0 unscoped, 0 findings. Taking over a sibling's work is only safe when it needs no new permission to do it, and checking that add-to-kanban does exactly one thing closes the duplication question the ticket itself raised.
Green, no threads. 👍
Uh oh!
There was an error while loading. Please reload this page.
What
set-statusandadd-to-kanbanfire on the samepull_requestevent and start together.set-statuspolled 5×5s for the PR's card and failed when it was not there;add-to-kanbanfrequently won seconds later. So the card ended correct and the check ended red.On
.github#362the board was already in the exact stateset-statuswould have written — the job's purpose was achieved and only its verdict was wrong. That is the worst shape for a check to fail in: nothing is broken, so the habit it teaches is to merge past red.Why end the race rather than widen the window
The retry budget's own comment held it at 5×5s deliberately, to find out how often the race is actually lost:
That measurement arrived (backend#2731:
#361and#362, both within seconds). A bigger number would still be a guess, and a longer wait is still a wait on a sibling workflow that may simply never have run.So
set-statusadds the card itself when it is absent. Three things make that safe, and I checked each rather than assuming:permission-organization-projects: writeandpermission-pull-requests: read— no permission change, andmint-scopestill reports0 unscoped, 0 findings.addProjectV2ItemByIdis idempotent — adding an item already on the board returns the existing item id.add-to-kanbandoes exactly one thing (anactions/add-to-projectstep and nothing else), so there is nothing to duplicate. This was the ticket's own "check first" caveat.Still fail-closed. backend#2037's guarantee is unchanged: an unresolvable PR node, or an add that returns no item id, goes red exactly as before. Only "not yet" stops reporting as "missing". Per the ticket, the failure was not downgraded to a warning.
Tests
Nothing covered this file's status-write block. The closing-ref suite already owns assertions about
set-pr-status.yml, so four went there (156 → 160 assertions), plus twoWORKFLOW_MUTATIONS— per that list's own note that rule 5 does not exempt a guard for living in YAML.Both mutations are caught, each by its own named assertion (
55 mutation(s): 0 stale, 0 uncaught):They were UNCAUGHT on the first attempt, and that is the part worth reading. Two separate causes, and I had asserted the fix was covered when it was not:
"addProjectV2ItemById" in HOSTpasses under the mutation that deletes the call, because the identifier also appears in the comment above it explaining that the add is idempotent — documenting the fix keeping the assertion green, the mirror image of the backend#2632 trap. And a bare"exit 1" in HOSTis satisfied by any other refusal in the file.Both now pin the call, not a token:
"addProjectV2ItemById(input: {projectId:", and a regex tying the refusal message to itsexit 1as one sequence. The harness reporting UNCAUGHT is the only reason either was found.Verification
make selftests→ rc=0make mint-scope→0 unscoped, 0 exempted, 0 findingspython3 scripts/tests/closing-ref-gate-mutations.py→ 55 mutations, 0 stale, 0 uncaughtactionlint+yaml.safe_loadon the changed workflow → cleanstandards-sync.pymutated — the backend#2441 corruption shape; restored and re-run from a verified-clean baseline before the result above was taken)What I could NOT verify, and it matters
This PR does not exercise the new path.
.github's ownset-pr-status-caller.ymlpinsset-pr-status.yml@main, so the running job on this PR ismain's copy. The fallback only takes effect after promotion tomain, and its first real exercise is the first PR that loses the race afterwards. The four assertions read the workflow source; they cannot prove the API call succeeds.Per the ticket's Done when: the first half ("two consecutive PRs land with
set-statusgreen on the first attempt") is only observable post-promotion. The second half ("a PR whose card genuinely never appears still fails") is what the fail-closed mutation pins.Part of tracebloc/backend#2731
Note
Medium Risk
Touches CI board writes (GraphQL add + status update) on every PR event; mistakes could duplicate cards or mask genuine board failures, though idempotent add and unchanged fail-closed paths limit blast radius.
Overview
Fixes false-red
set-statuschecks whenadd-to-kanbanadds the card a few seconds later: after the existing 5×5s poll, the job now adds the PR to the org project via GraphQL (pullRequestnode id →addProjectV2ItemById) instead of treating “not yet” as “missing.” The 5×5s window is unchanged; the race is ended by doing the add locally with the token that already hasorganization-projects: write, relying on idempotent project adds.Fail-closed behavior is preserved for real failures (unresolvable PR node, add returns no item id); only the error copy distinguishes “could not be added” from the old race wording.
Tests:
closing-ref-gate-selftest.pygains four workflow-source pins on the mutation call, PR node lookup, and the refusal/exit 1pair;closing-ref-gate-mutations.pyadds two YAML mutations so removing the fallback or softening the final refusal reddens the suite.Reviewed by Cursor Bugbot for commit ef02239. Bugbot is set up for automated code reviews on this repo. Configure here.