Skip to content

fix(fr-gate): fail closed on an unreadable or absent Status - #71

Merged
LukasWodka merged 2 commits into
developfrom
fix/1266-fr-gate-fail-closed
Jul 26, 2026
Merged

fix(fr-gate): fail closed on an unreadable or absent Status#71
LukasWodka merged 2 commits into
developfrom
fix/1266-fr-gate-fail-closed

Conversation

@LukasWodka

@LukasWodkaLukasWodka commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Closes tracebloc/backend#1266. RFC-BACKEND-0008 D27-L2/L6.

The defect

fr-gate.yml treated "could not evaluate this item" as "this item is fine":

STATUS=$(gh api graphql ... 2>/dev/null | jq ...)|| STATUS=""if [ -z"$STATUS" ];thenecho" ⚪ #$num — not on kanban, skipping"
MISSING="$MISSING #$num"continue# ← not a blockfi

2>/dev/null and || STATUS="" collapsed every failure into a silent pass — missing/expired
PROJECTS_KANBAN_TOKEN, a repo secret shadowing the org one, a transient 5xx, or the item genuinely
absent. If every item was unreadable, the gate printed ✓ PASSED. The only promotion gate disarmed
itself, and failure looked exactly like success.

It was a race, not an edge case

fr-gate-caller.yml and add-to-kanban.yml both fire on pull_request: opened and run
concurrently, so the gate regularly queried the board before the card existed. On
tracebloc/.github#61 the gate finished at 16:48:09 and the card was created at 16:48:14 — it
won by one second and passed. Measured on real promotion runs: 4 of 32 items skipped this way, 3 of
20, 3 of 17.

The fix

  • resolve_status() retries up to 5× (2s, 4s, 6s, 8s backoff) and returns a distinct code for
    an API failure (1) vs a PR genuinely not on the board (2). The empty-result retry is the half
    that closes the race.
  • The verdict fails closed on BLOCKED, MISSING, orUNREADABLE, each with its own
    actionable message — an infra failure now reads differently from a review failure.

Also closes L6 (delete card → add label → pass): the label trigger re-runs the gate,
add-to-kanban doesn't recreate the card, and the retry now waits for it rather than passing on its
absence.

Verification

  • Return-code contract exercised under a stubbed gh: card → 0, no-card → 2, API-failure → 1.
  • actionlint (now a required check on this repo) passes — shellcheck included.

⚠️ Overlaps #68

#68 ("make the FR gate self-clearing") also rewrites fr-gate.yml. Whichever merges first, the
other must rebase.
They are not independent. I'd also flag that #68's approach — automating the
manual retry — needs reconciling with this: the retry-passes-96%-of-the-time pattern it automates is
in several cases the card being moved between attempts, i.e. the L3 bypass, not a transient blip.
This PR retries the read, not the verdict, which is the safe half.

I could not run the full workflow end-to-end (no kanban token locally); the logic is unit-tested as
above and CI is the real integration run.


Note

Medium Risk
Changes promotion merge-blocking behavior for staging/main; misconfigured tokens or slow kanban card creation could block merges until retry succeeds or override is used, which is intentional but operationally sensitive.

Overview
The reusable FR gate workflow no longer treats “could not read kanban Status” as a pass. It adds resolve_status(), which retries GraphQL reads up to five times with backoff (including empty results from racing add-to-kanban on pull_request: opened), and separates API failure from PR not on the board.

Items that stay unreadable or missing after retries now fail the check via new UNREADABLE and MISSING handling, with targeted ::error:: messages (token/infra vs missing card vs wrong column). The old path that skipped off-board PRs and could print PASSED when every item was unevaluated is removed. Jq parsing is hardened so transient parse/pipe errors do not abort retries under set -euo pipefail.

Reviewed by Cursor Bugbot for commit 5817ae0. Bugbot is set up for automated code reviews on this repo. Configure here.

The gate treated "could not evaluate this item" as "this item is fine". An
unreadable or missing Status hit `continue`, and if every item was unreadable
the gate printed "PASSED". A missing or expired PROJECTS_KANBAN_TOKEN, a
repo-level secret shadowing the org one, or a transient GraphQL 5xx all
collapsed into a silent pass — the only promotion gate disarming itself, with
failure indistinguishable from success.
Two changes:
- resolve_status() retries up to 5 times and distinguishes an API failure
(rc=1) from a PR genuinely not on the board (rc=2). The empty-result retry is
the important half: the empty case is usually a RACE, not an absence.
fr-gate-caller.yml and add-to-kanban.yml both fire on `pull_request: opened`
and run concurrently, so the gate routinely queries the board before the card
exists. Observed on #61 — gate finished at 16:48:09, card
created at 16:48:14, and it passed on "not on kanban". Measured 4/32, 3/20,
3/17 items skipped this way on real promotion runs.
- The verdict blocks on BLOCKED, MISSING, or UNREADABLE, each with its own
actionable message, instead of only on BLOCKED.
This also closes the delete-card-then-add-a-label bypass: the label trigger
re-runs the gate, add-to-kanban does not recreate the card, and the retry now
gives the card time to reappear rather than passing on its absence.
Verified: the return-code contract holds under a stubbed gh (card -> 0,
no-card -> 2, API-failure -> 1), and actionlint passes.
Refs RFC-BACKEND-0008 D27-L2/L6 (tracebloc/backend#1266)
@LukasWodkaLukasWodka self-assigned this Jul 26, 2026
@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

👋 Heads-up — Code review queue is at 41 / 30

Above the WIP limit. The team convention is to review existing PRs before opening new work.

Open PRs currently in Code review (oldest first):

Pull from review before opening new work. (This is a nudge from the kanban WIP check, not a block.)

@cursorcursorBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 336c175. Configure here.

Comment thread.github/workflows/fr-gate.yml
Bugbot on #71 caught two robustness bugs I introduced:
- The gh success capture used 2>&1, so any stderr warning was merged into the
JSON body and fed to jq — a spurious parse failure on an otherwise good call.
Capture stdout only (2>/dev/null).
- The jq|head pipeline was unguarded under set -euo pipefail. A jq error, or a
SIGPIPE from head closing the pipe early, would abort resolve_status mid-loop,
skip the remaining retries, and return an ambiguous code that the caller then
mislabelled as MISSING with a '5 attempts' message that never ran. Parse is
now non-fatal (2>/dev/null on jq, || true on the pipeline) so a failed parse
falls through to the next retry.
Also made the end-of-loop sleep an explicit if, not a '[ ] && sleep' list,
which is a set -e footgun as a bare statement.
Return-code contract re-verified under a stubbed gh, including a case where gh
writes to stderr but returns 0 — the JSON now parses cleanly where 2>&1 would
have broken it.
@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

Bugbot finding addressed. It was right, and both bugs were mine from this PR:

  1. 2>&1 on the gh capture merged stderr into the JSON body → jq parsed garbage on an otherwise-good call. Now 2>/dev/null (stdout only).
  2. The jq | head pipeline was unguarded under set -euo pipefail, so a parse error or head SIGPIPE aborted resolve_status mid-loop — skipping the retries and returning an ambiguous code the caller mislabelled as MISSING. Parse is now non-fatal.

Re-verified the return-code contract, including a new case where gh writes to stderr but returns 0 (proves fix #1). actionlint green.

bugbot run

@LukasWodka
LukasWodka merged commit cc34368 into developJul 26, 2026
2 checks passed
LukasWodka added a commit that referenced this pull request Jul 26, 2026
Picks up #66 (actionlint gate + the 29 shellcheck cleanups), #67 (closure
router), #69 (CODEOWNERS), and #71 (fr-gate fail-closed). Without these the
code-quality run here was linting the pre-#66 workflow copies and failing on
findings already fixed on develop.
LukasWodka added a commit that referenced this pull request Jul 26, 2026
* fix(fr-gate): gate the diff, not just commit subjects (re-land #72 onto develop)
#72 was reviewed and merged, but into its stacked base (fix/1266) rather than
develop — #71 merged to develop first and the child was never retargeted, so
the attribution code never reached develop (stacked-PR merge-order hazard).
This ports exactly that delta onto develop: the only change vs develop's
current fr-gate.yml is the additive attribution block + its wiring (verified by
diff — all additions, nothing removed). develop already carries #71's
fail-closed + robustness fixes via the earlier squash.
Attribution: every non-merge commit in BASE..HEAD must carry a (#N) or be
introduced by a PR merge (in M^1..M^2), else the gate fails closed. Fixture-
tested incl. the Bugbot #72 false-negative (direct push absorbed by a feature
branch). actionlint clean.
Refs RFC-BACKEND-0008 D27-L1 (tracebloc/backend#1265), re-lands #72
* fix(fr-gate): only PR-merges may vouch — close the sync-merge laundering hole
Bugbot #73: the attribution loop vouched a commit via ANY merge in range whose
M^1..M^2 contained it. A routine 'Merge branch develop into feature' sync merge
is itself in range, and a direct push on develop sits in that sync merge's
S^1..S^2 — so the push was laundered into attribution, reopening the exact
fail-open the #72 ^1 check closed, on merge-commit workflows.
Fix: a merge may vouch only if its own subject carries a (#N) or
'Merge pull request #N'. Sync merges (no PR ref) no longer vouch.
Reproduced with a fixture where the feature branches before the direct push and
syncs develop in: the pre-fix logic MISSES the push (the hole), the fix FLAGS
it. Earlier fixtures (basic direct push, feature-absorbs-base) still flag
correctly. actionlint clean.
Refs RFC-BACKEND-0008 D27-L1 (tracebloc/backend#1265)
LukasWodka added a commit that referenced this pull request Jul 26, 2026
promote: FR gate fail-closed (#71) to main
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@LukasWodka