Skip to content

ci(2212): the fixtures drift check must fail when it cannot run - #536

Merged
LukasWodka merged 2 commits into
developfrom
fix/2212-fixtures-drift-fail-closed
Aug 20, 2026
Merged

ci(2212): the fixtures drift check must fail when it cannot run#536
LukasWodka merged 2 commits into
developfrom
fix/2212-fixtures-drift-fail-closed

Conversation

@LukasWodka

@LukasWodkaLukasWodka commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Why

Backend fixtures drift check is being armed as a required context (tracebloc/backend#2212). Its activation-phase fail-open has to go first.

When BACKEND_CONTRACTS_TOKEN was unreadable, the step printed a warning and exit 0 — a check that never executed reported as a passing one. "Inert, not red" was the right call while the secret did not exist. It has existed since 2026-08-05, and once the context is required, exiting 0 when unable is strictly worse than an advisory guard, because it also looks solved. That is the shape that let a stale R8 manifest merge in backend#2183.

cli is public, so this needs three branches, not two

The two reasons the token can be missing are not the same thing:

ConditionBeforeNow
token presentrun the checkrun the check
absent, fork PRexit 0 + warningfail — maintainer verifies by hand, applies skip-fixtures-drift
absent, same-repoexit 0 + warningfail — rotated/removed secret is a misconfiguration

GitHub withholds repo secrets from forks by design, so on a fork PR the check genuinely cannot run. Passing those silently would fail open on exactly the contributions that deserve the most scrutiny — so it fails, and a maintainer who has checked internal/api/testdata/*.json by hand applies skip-fixtures-drift. The label is a permanent artifact on the PR, the same model as the existing skip-fr-gate. Created on this repo with a matching description.

The trap this repo has already hit once

types: [..., labeled, unlabeled] is added to the pull_request trigger. Without it the defaults are opened/synchronize/reopened, so applying the override label would change nothing until the next push — making the override look broken. That is precisely the defect Bugbot caught on version-bump-gate-caller.yml's skip-version-gate.

Mutation proof

All five paths, running the actual step body against a stubbed sync script:

override label present exit 0 [WARN: Backend fixtures drift OVERRIDDEN]
token present (normal path) exit 0 [real check ran with --check]
token absent, FORK PR exit 1 [ERROR: Backend fixtures drift could not run]
token absent, same-repo (misconfig) exit 1 [ERROR: BACKEND_CONTRACTS_TOKEN is missing]
override wins over missing token exit 0 [WARN: ... OVERRIDDEN]

Plus the case that would be easy to break while "fixing" this one:

token present, script reports drift exit 3 [drift detected!]

exec replaces the shell, so a real drift failure still fails the step rather than being swallowed by the wrapper.

Security

Every ${{ }} goes through env: — none reaches the run: body. IS_FORK and OVERRIDE are workflow-context booleans, and the token stays a secret reference.

Test plan

  • YAML parses; trigger types, step env keys and the absence of ${{ }} in run: all asserted
  • Five-way decision logic mutation-proved above
  • CI on this PR exercises the token-present path (same-repo PR, secret readable), which should stay green
  • The two failing paths are proved by the harness rather than by unsetting a live secret

Refs tracebloc/backend#2212

🤖 Generated with Claude Code


Note

Medium Risk
Changes required CI gate semantics and fork/Dependabot bypass paths; misconfiguration or missing override labels can block PRs, but avoids fail-open on a security-sensitive contract check.

Overview
Arms the Backend fixtures drift check for use as a required CI context by replacing the activation-era warn and exit 0 path when BACKEND_CONTRACTS_TOKEN is unreadable with fail-closed behavior, so a check that never ran cannot report green.

The step now branches on token presence, PR source, and override: with a token it execssync-backend-fixtures.sh --check (real drift failures still fail the job); skip-fixtures-drift exits 0 with a warning and leaves the label as the audit record; Dependabot PRs without the token pass with a notice; fork PRs and same-repo misconfiguration exit 1 with targeted errors. Workflow context (IS_FORK, PR_AUTHOR, OVERRIDE) is passed only via env:, not inline in the shell script.

pull_request triggers now include labeled / unlabeled (and ready_for_review) so applying or removing the override label re-runs the gate without waiting for another push.

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

`Backend fixtures drift check` is being armed as a required context
(backend#2212). Its activation-phase fail-open has to go first: when
BACKEND_CONTRACTS_TOKEN was unreadable the step printed a warning and exited 0,
so a check that never executed reported as a passing one. Inert-not-red was the
right call while the secret did not exist; the secret has existed since
2026-08-05, and once the context is required an exit-0-when-unable is strictly
worse than an advisory guard, because it also looks solved (backend#2183).
`cli` is PUBLIC, so the two reasons the token can be missing are different
things and the step now splits three ways:
token present -> run the check
absent, fork PR -> FAIL. GitHub withholds repo secrets from forks by
design, so the check genuinely cannot run. A maintainer
verifies internal/api/testdata/*.json by hand and
applies `skip-fixtures-drift` -- a permanent artifact on
the PR, the same model as skip-fr-gate. Silently passing
forks would fail open on exactly the contributions that
deserve the most scrutiny.
absent, same-repo -> FAIL. Rotated, removed or expired: a misconfiguration
that used to read as a clean run.
`types: [.., labeled, unlabeled]` added to the pull_request trigger, because
without them the default opened/synchronize/reopened means applying the override
label changes nothing until the next push -- the same defect Bugbot caught on
version-bump-gate-caller.yml's skip-version-gate.
Every ${{ }} goes through env:, none into the run: body.
Mutation-proved, all five paths, by running the step body against a stubbed
sync script:
override label present exit 0 (OVERRIDDEN warning)
token present exit 0 (real check ran)
token absent, fork PR exit 1 (could not run)
token absent, same-repo exit 1 (secret missing)
token present, script reports drift exit 3 (exec propagates the status)
The last one matters: `exec` replaces the shell, so a real drift failure still
fails the step rather than being swallowed.
Label `skip-fixtures-drift` created on this repo.
Refs tracebloc/backend#2212
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@LukasWodkaLukasWodka self-assigned this Aug 20, 2026
@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

bugbot run

Comment thread.github/workflows/backend-fixtures-drift.yml
I claimed in chat that this PR was unaffected by the Dependabot finding on
averaging-service#367. Wrong, and this repo is the worse case of the two.
Dependabot branches live in THIS repo, not a fork, so
`github.event.pull_request.head.repo.fork` is FALSE on them -- verified on the
real #530: head.repo.fork=false, head.repo.full_name=tracebloc/cli. Their runs
still receive only Dependabot-scoped secrets, so BACKEND_CONTRACTS_TOKEN is
empty. Under the previous commit that combination landed in the "absent,
same-repo -> misconfiguration -> FAIL" branch, which would have blocked every
Dependabot PR once the context is armed.
Not theoretical: this repo has 4 Dependabot PRs, #530 is OPEN right now, and it
currently reports `Backend fixtures drift check: success` -- the fail-open
passing vacuously on a live PR today.
So Dependabot gets a fourth branch, passing with a ::notice::. Safe for the same
structural reason as averaging-service#367, via a different always-running
guard: a dependency bump cannot alter internal/api/testdata/*.json, and if it
did, internal/api/contracts_test.go replays every fixture through the real
decode paths under the REQUIRED `Test` check with no token. Drift against the
pinned backend ref is re-checked by the push run on develop/main, where Actions
secrets are available.
Mutation-proved, all five:
Dependabot PR (fork=false, no token) exit 0 (notice: deferred)
fork PR, no token exit 1 (could not run)
human same-repo, no token exit 1 (secret missing)
token present exit 0 (real check ran)
override label exit 0 (OVERRIDDEN warning)
PR_AUTHOR uses github.event.pull_request.user.login, not github.actor, so it
stays correct across re-runs.
Refs tracebloc/backend#2212
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

Self-correction, pushed. I said in chat that this PR was unaffected by the Dependabot finding on tracebloc/averaging-service#367. Wrong — and cli is the worse of the two cases.

Dependabot branches live in this repo, not a fork, so github.event.pull_request.head.repo.fork is false on them. Verified against the real #530:

PR #530: head.repo.fork=false head.repo.full_name=tracebloc/cli

Their runs still get only Dependabot-scoped secrets, so BACKEND_CONTRACTS_TOKEN is empty. Under the first commit that combination fell into absent, same-repo → misconfiguration → FAIL, which would have blocked every Dependabot PR once this context is armed.

Not theoretical: this repo has 4 Dependabot PRs, #530 is open right now, and it currently reports Backend fixtures drift check: success — the fail-open passing vacuously on a live PR today. Arming on top of the first commit would have turned that green into a permanent block.

Fixed with a fourth branch that passes with a ::notice::. Safe for the same structural reason as #367 but via a different always-running guard: a dependency bump cannot alter internal/api/testdata/*.json, and if it did, internal/api/contracts_test.go replays every fixture through the real decode paths under the requiredTest check with no token. Drift against the pinned backend ref is re-checked by the push run on develop/main, where Actions secrets are available.

All five branches mutation-proved:

Dependabot PR (fork=false, no token) exit 0 notice: deferred
fork PR, no token exit 1 could not run
human same-repo, no token exit 1 secret missing
token present exit 0 real check ran
override label exit 0 OVERRIDDEN

@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

bugbot run

@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.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 3e3429c. Configure here.

@saqlainsyed007saqlainsyed007 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Correctness review — approving.

Arms the backend-fixtures-drift check for use as a required context by replacing the activation-era "warn + exit 0" fail-open with fail-closed behavior. Traced every path:

  • No unintended fail-open remains. The only exit 0 branches are the explicit skip-fixtures-drift label, the real check via exec (a drift/non-zero exit still fails the step), and the Dependabot deferral. Anything else falls through to exit 1 — no fall-through green.
  • exec fails closed — if sync-backend-fixtures.sh were missing/non-executable, a non-interactive bash shell exits non-zero rather than continuing to a passing branch.
  • Branch ordering is right — Dependabot (head.repo.fork=false, no Actions secret) is handled before the fork/misconfig split, so weekly bumps aren't blocked, and the deferral is safe because a dependency bump can't touch internal/api/testdata/*.json.
  • set -uo pipefail is safe — every referenced var is declared in env:, so push/workflow_dispatch runs (null pull_request context) resolve to empty/false without tripping -u.
  • Security — all ${{ }} interpolation goes through env:, none into the run: body; the dependabot[bot] login can't be spoofed from a fork, and forks are caught by IS_FORK regardless.
  • types: [..., labeled, unlabeled] correctly added so applying/removing the override label actually re-runs the gate.

The earlier Bugbot finding (Dependabot PRs landing in the misconfig branch) is already fixed by the fourth branch and its thread is resolved. CI is green, including the token-present path of this check on this same-repo PR. LGTM.

@LukasWodka
LukasWodka merged commit c28c120 into developAug 20, 2026
26 checks passed
@LukasWodka
LukasWodka deleted the fix/2212-fixtures-drift-fail-closed branch August 20, 2026 08:35
@saadqbal

Copy link
Copy Markdown
Collaborator

/fr-pass

CI/tooling-only change, no product surface. Verified by this repo's own checks; the staging journey does not exercise workflow code.

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.

3 participants

@LukasWodka@saadqbal@saqlainsyed007