Skip to content

fix(2214): audit the branch a repo ships from, not any branch named develop - #289

Merged
LukasWodka merged 3 commits into
developfrom
fix/2214-audit-the-shipping-branch
Aug 20, 2026
Merged

fix(2214): audit the branch a repo ships from, not any branch named develop#289
LukasWodka merged 3 commits into
developfrom
fix/2214-audit-the-shipping-branch

Conversation

@LukasWodka

@LukasWodkaLukasWodka commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Closes backend#2214. Found by the backend#2157 sweep — the first change ever made to a copied workflow file.

The bug

caller-drift preferred develop wherever that branch existed. rfcs is the only repo in the org whose default is main, so the sweep landed there — where rfcs actually ships — and the audit read a develop that lags:

refblob
rfcs/main (default, where it ships)2a3a432b3d01
rfcs/develop (what the audit read)07283e30b80b

It then reported drift against a change that was correctly in place.

It agreed by coincidence for months

That's the part worth recording. Both refs held the same blob because nothing had ever changed that file, so a wrong ref and a right ref were indistinguishable. A resolver pointing at the wrong branch produces a correct-looking result for exactly as long as the two branches happen to match, and emits a false finding the first time real work lands.

The rule is now derived, not listed

Develop-first exists because train repos default to main/master while work lands on develop, so a default-branch audit under-reports work in flight. A non-train repo has no promotion pipeline: its default branch is where it ships, and preferring a stray develop audits a branch nobody merges to.

on_train=bool(meta.get("release_train"))
ifon_trainand"develop"inbranches: ... # train: develop-firstelifmeta.get("default_branch") inbranches: ... # else: the shipping branch

The discriminator is release_train, which the inventory already carries and which load_release_trainalready verifies against release-train/repos.yml — so the audited ref is derived from a fact this guard independently checks, rather than from a hand-maintained audit_ref field or an exception row for rfcs. An override would just be a second place to be wrong.

Measured across all 19 repos before writing it: the new rule changes exactly one answerrfcs, developmain. Nothing else moves.

audit_branch is renamed develop-firstdevelop-first-on-train in both the inventory and SUPPORTED_AUDIT_BRANCH, because the schema check compares them and the semantics changed. The description above that key said "develop where that branch exists, else the default" — precisely the behaviour being removed.

Verification

Both directions are pinned, because neither case can fail alone: assert only the train side and the non-train path is untested; assert only the non-train side and a resolver that always uses the default branch passes.

mutationresultcaught by
reverts to plain develop-first193/1the non-train case
always the default branch192/2the train case
train flag read inverted191/3the train case + two more
audit_branch no longer enforced193/1the inventory positive control

And one fix to the suite itself

The _good positive control raised AssertionError when the tree was fetched off develop — which aborts the whole run. Two of those mutations first reported CRASH and hid every other case they also broke. It now returns an empty tree and fails cleanly. A suite that dies on the first surprise can't tell you the shape of a regression.

The fixture META also gained release_train: True, now load-bearing: without it every existing case would have silently exercised the non-train path while being written about develop-first.

make check green — 194 caller-drift cases, all selftests, coverage gate, actionlint.

Not fixed here, because it isn't in this repo

The workspace CLAUDE.md says "Verified 2026-08-06: every active repo's default branch is now develop (all 20)". That's false for rfcs, and it's the line a reader would use to conclude this guard reads the right branch. It lives outside any git repo so it can't ride a PR — flagged on backend#2214 instead.


Note

Cursor Bugbot is generating a summary for commit f7c7bcc. Configure here.

…evelop
`caller-drift` preferred `develop` wherever that branch existed. `rfcs` is the only
repo in the org whose default is `main`, so the backend#2157 sweep landed there --
where rfcs actually ships -- and the audit read a `develop` that lags, then reported
drift against a change that was correctly in place:
rfcs/main 2a3a432 the default branch, where the change landed
rfcs/develop 07283e3 what the audit read
IT AGREED BY COINCIDENCE FOR MONTHS, and that is the part worth recording. Both
refs held the same blob because nothing had ever changed that file, so a wrong ref
and a right ref were indistinguishable. A resolver pointing at the wrong branch
produces a correct-looking result for exactly as long as the two branches match,
then emits a false finding the first time real work lands.
THE RULE IS NOW DERIVED, NOT LISTED. Develop-first exists because TRAIN repos
default to main/master while work lands on develop, so a default-branch audit
under-reports work in flight. A NON-TRAIN repo has no promotion pipeline: its
default branch IS where it ships, and preferring a stray `develop` audits a branch
nobody merges to. The discriminator is `release_train`, which the inventory already
carries and which `load_release_train` already verifies against
release-train/repos.yml -- so the audited ref is derived from a fact this guard
independently checks, rather than from a hand-maintained `audit_ref` field or an
exception row for rfcs. A second place to be wrong is what an override would buy.
Measured across all 19 repos before writing it: the new rule changes exactly ONE
answer, rfcs develop -> main. Nothing else moves.
`audit_branch` is renamed develop-first -> develop-first-on-train in the inventory
AND in SUPPORTED_AUDIT_BRANCH, because the schema check compares them and the
semantics changed. The one-line description above the key said "develop where that
branch exists, else the default" -- exactly the behaviour being removed -- and now
states the rule and names the discriminator.
BOTH DIRECTIONS ARE PINNED, because neither case can fail alone: assert only the
train side and the non-train path is untested; assert only the non-train side and a
resolver that always uses the default branch passes. Each reddens a different
mutation. 4 mutations, all applied and caught:
reverts to plain develop-first 193/1 the non-train case
always the default branch 192/2 the train case
the train flag read inverted 191/3 the train case + two more
audit_branch no longer enforced 193/1 the inventory positive control
AND ONE FIX TO THE SUITE ITSELF. The `_good` positive control RAISED
AssertionError when the tree was fetched off develop, which aborts the whole run --
so two of those mutations first reported "CRASH" and hid every other case they also
broke. It now returns an empty tree, failing cleanly. A suite that dies on the
first surprise cannot tell you the shape of a regression.
The fixture META also gained `release_train: True`, which is now load-bearing:
without it every existing case would have silently exercised the non-train path
while being written about develop-first.
NOT FIXED HERE, because it is not in this repo: the workspace CLAUDE.md states
"Verified 2026-08-06: every active repo's default branch is now `develop` (all
20)". That is false for rfcs and is the line a reader would use to conclude this
guard reads the right branch. It lives outside any git repo, so it cannot ride a
PR; flagged on backend#2214.
make check green: 194 caller-drift cases, all selftests, coverage gate, actionlint.
Closes backend#2214.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@LukasWodkaLukasWodka self-assigned this Aug 20, 2026

@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 f7c7bcc. Configure here.

Comment threadscripts/caller-drift.py
LukasWodkaand others added 2 commits August 20, 2026 04:59
Bugbot, High, and it makes the previous commit worse than a no-op. `read_repo` read
`meta.get("release_train")`, but production `meta` comes from `list_active_repos`,
which returns ONLY:
{"visibility": ..., "default_branch": ...}
No `release_train`. So the flag was always None -> False, EVERY repo was audited on
its default branch, and develop-first was silently deleted for the train repos it
exists for. Today that happens to be harmless -- every train repo currently defaults
to `develop`, measured -- but the policy was gone, and the next repo to default to
main/master would have under-reported work in flight with nothing saying so.
THE SELFTEST PASSED BECAUSE THE FIXTURE CARRIED A KEY PRODUCTION NEVER SETS. I added
`release_train: True` to META in the same commit that started reading it from there.
A fixture richer than the real payload is a test asserting its author's assumption,
and this is the THIRD time that shape has appeared in this epic -- `is_bot` on #282,
the look-alike action name on #287, this.
THE FIX IS A PARAMETER, NOT A LOOKUP. `on_train` is required and positional, so a
caller that forgets it raises TypeError; defaulting it to False would reproduce the
bug with better manners. `main()` passes `bool(entry.get("release_train"))` from the
INVENTORY row -- schema-required, and cross-checked against release-train/repos.yml
by load_release_train -- so the ref this guard reads is derived from a fact it
independently verifies.
META IS NOW EXACTLY THE PRODUCER'S SHAPE, and a case asserts that by reading
`list_active_repos`'s own source rather than a list written in the test: it writes
`visibility` and `default_branch` and nothing else. That case reddens if the producer
starts setting `release_train`, which is the only honest way to keep the two in step.
AND THE WIRING IS PINNED SEPARATELY, because the behavioural cases could not see it.
They call read_repo directly, so mutations hardcoding the argument to True or False
left all 196 green -- and #289's bug WAS the wiring, not the resolver. A source
assertion on main() closes it. Weaker than behavioural and said so in the comment:
driving main() needs the org listing, the inventory and the train file stubbed
together, which this suite has no harness for.
That assertion was ALSO wrong on its first attempt: a paren-matching regex allowing
one level of nesting could not match `bool(entry.get(...))` across two lines, so it
found nothing and failed on the correct code while every mutation "passed". An
extractor that cannot find the thing reports the same as a defect. Line-based now.
7 mutations, all applied and caught:
on_train read from meta again 194/2 the train-branch case
call site passes False 196/1 the wiring case
call site passes True 196/1 the wiring case
call site reverts to meta 196/1 the wiring case
plain develop-first restored 193/1 the non-train case
always the default branch 192/2 the train case
list_active_repos sets release_train 194/2 the producer-shape case
197 cases green. make check green.
Addresses Bugbot on .github#289.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three ruff errors (E402 import-not-at-top, two E741 ambiguous `l`) shipped in the
commit before this one. They shipped because I ran
make check 2>&1 | tail -3 && git commit && git push
and a pipeline exits with the status of its LAST command. `tail` returned 0, so the
`&&` chain treated a failed `make check` as a pass and pushed anyway. The output was
even visible -- "make: *** [ruff] Error 1" was in the three lines I printed -- and
the chain ran on regardless.
Recorded rather than quietly fixed, because it is the same defect this repo keeps
finding in its own guards: a check whose result nothing actually reads. Mine was in
the shell, one layer out from the code.
`make check` now run with its exit status captured (exit=0), not piped.
197 selftest cases green, 41 suites green.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

bugbot run

@saadqbalsaadqbal left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Verified the interesting part first: list_active_repos does build each entry with exactly visibility and default_branch (caller-drift.py:1055-1056), so the earlier meta.get("release_train") really was always None — every repo audited on its default branch, develop-first silently deleted for the train repos it exists for. Good catch, and the diagnosis of why the selftest missed it is the more valuable half: a fixture that constructs the meta it wishes for cannot fail on the producer's real shape.

The recurrence guard is the right shape. Rather than another fixture, the suite now parses list_active_repos' own source and asserts the keys it writes, plus that main() derives on_train from the inventory entry — so if the producer changes, the test moves with it. Confirmed by mutation, 197/0 baseline:

  • Read train membership from meta again (the original High) → 2 failures, led by a TRAIN repo is audited on develop even when it defaults to main. That it's now caught behaviourally and not only by the source checks is what makes this closed rather than papered over.
  • Drop the discriminator (plain develop-first) → 1 failure: a NON-TRAIN repo is audited on its default branch, not a stray develop.

Making on_train required and positional is the right call over a defaulted keyword — = False would have reproduced the identical bug with better manners.

On the rule itself: deriving from release_train, which load_release_train already cross-checks against release-train/repos.yml, beats a per-repo audit_ref or an rfcs exception row. A hand-maintained override is a second place to be wrong, and it would have needed a human to notice rfcs was the odd one out — which nobody did for months. Measuring that the new rule changes exactly one answer across all 19 before writing it is the part that makes this safe to land.

The "it agreed by coincidence" note is worth having written down. A resolver pointing at the wrong branch produces correct-looking output for exactly as long as the two refs happen to match, then emits a false finding the first time real work lands — and a false finding on this guard blocks merges. That's a failure mode with no signal until it bites.

@LukasWodka
LukasWodka merged commit 31349db into developAug 20, 2026
12 checks passed
@LukasWodka
LukasWodka deleted the fix/2214-audit-the-shipping-branch branch August 20, 2026 09:46
LukasWodka added a commit that referenced this pull request Aug 20, 2026
saadqbal approved with one fix, and it is the file's own rule turned on itself.
THE PROSE SAID 13 IN TWO PLACES while `EXEMPT` holds 12. His diagnosis is right:
13 is the mint STEP count the audit reports -- 12 unscoped plus one already scoped
-- so the two are different populations, and the smaller one drifts the moment a row
is burnt down. Which is the point of the guard.
And his recommended fix is the right one: not "write 12", but let the number come
from `len(_exempt())`, which the run already prints. A hardcoded tally sitting
directly above the list it counts is the exact pattern backend#1729 is cited for --
in the file that cites it. Both prose sites now carry no number, and the docstring
says why, so the next person does not helpfully add one back.
MERGE CONFLICT resolved against develop, which moved three times underneath this
branch (#288, #289, #291). All three hunks wanted BOTH sides, not one:
Makefile `lint` mint-scope AND mutation-house-rules-dry
SELFTEST_TARGETS selftest-mint-scope AND selftest-house-rules
selftests.yml the mint-scope audit step AND the house-rules mutation step
Makefile CI map one line naming all three, since the required `selftests`
context now runs all of them
repo-inventory.yml kept OURS deliberately: develop still carries the
pre-release-train#93 wording ("the header alone"), which is now false -- #93 added
the scoping, so the divergence really is two comment blocks and nothing else, and
ours is the version with the checkable claim in it.
Two comments in selftests.yml also lost their case counts on the way through, for
the same reason as the docstring: `mint-scope` prints its own totals.
make check exit 0. mint-scope: 13 steps, 12 exempted, 0 findings. 41 house-rules
cases. mint-scope selftest 13/13.
Refs backend#2157.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

/fr-pass

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.

2 participants

@LukasWodka@saadqbal