Uh oh!
There was an error while loading. Please reload this page.
fix(scripts): branch ownership comes from the PR author, never the tip commit (backend#2365) - #323
Conversation
…p commit (backend#2365) A "my branches" list built from `%(authorname)` on the ref answers "who touched it last", not "whose work is this". Pushing a review fixup onto someone else's branch rewrites the tip author and changes nothing about who owns it -- and the next command in that workflow is `push origin --delete`. Measured: `client` PRs #395 and #592 are @shujaatTracebloc's and both showed Lukas as tip author. backend#2001's per-person table is NOT affected -- checked first, and it attributed by PR author already (`inventory.py`: "merged/closed PR author wins, else the tip's git identity"). The defect is that the rule lived in a throwaway script, so every later query re-derives it and the obvious derivation is wrong. `branch_owner.py` is the one place that answers it: PR author first (exact head oid, then head name), the oldest commit not on the default branch for a branch with no PR, and `unattributable` otherwise. `attribute()` takes no tip-author parameter, so no path can return one. Fails closed -- an unread or truncated PR list, an ambiguous head name and a deleted PR author all refuse rather than guess -- and `origin/HEAD` is verified against the remote before the first-commit signal is used, because a stale default corrupts that signal exactly. `CreateEvent` is not used for the no-PR case despite the ticket proposing it: /repos/{repo}/events caps at 300 events, which on backend spans under four hours and holds two branch creations. 71 assertions and 26 mutations, wired into the already-required `selftests` context; three mutations were uncaught on the first run and the tests were strengthened rather than the mutations dropped. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
… not a mismatch (backend#2365) Both of Bugbot's findings on #323, and both are the same shape as the defect already fixed in the first commit: a message asserting a comparison that never ran. Reproduced by construction before changing anything. 1. The multi-author reuse refusal ended "and none is at the current tip" even with no tip in hand -- reachable for a branch named on the command line that this clone has not fetched, whose head name two people have used. The single -author `pr` arm already split those two sentences; this arm now does too. 2. When the default branch could not be confirmed, `main` withheld the first-commit signal by passing "" and `attribute` then reported "no commit on this branch that is not already on the default branch" -- a fact about the branch, from a check nobody ran. Appending the reason did not retract it. `attribute` takes `first_commit_problem` now, so a withheld signal is reported as unmeasured with its reason, and the measured-and-empty wording is pinned apart from it. 77 assertions, 29 mutations, 0 stale, 0 uncaught. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
LukasWodka
commented
Aug 24, 2026
Both Bugbot findings were real and are fixed in 9102520. Reproduced by construction first — they are the same shape as the defect the first commit already fixed (a message asserting a comparison that never ran), so the class is now pinned in three places rather than two. 1. 2. Three new mutations cover both (the reuse tail unconditional, the withheld arm disabled, and the two wordings collapsed into one) — all three redden. Suite now 77 assertions, 29 mutations, 0 stale, 0 uncaught; |
Uh oh!
There was an error while loading. Please reload this page.
saqlainsyed007
left a comment
There was a problem hiding this comment.
Reviewed the core attribute() logic and it's excellent — fail-closed at every ambiguity, never returns the tip author. Requesting changes on @cursor's Medium, which I verified and which is a real inconsistency with this module's own contract:
remote_branches fails open (scripts/branch_owner.py:357). A failed git for-each-ref does return [] with no problem signal, so main renders it as a successful empty inventory — header, 0 branch(es), exit 0. Every other seam (pull_requests, default_branch) refuses with an explicit problem string, and the module's own docstring is emphatic that a silent empty "looks like coverage" and that "cannot tell" must be a finding, not a pass. This one read violates that: an enumeration that couldn't run is indistinguishable from "no branches to attribute."
Make it signal the failure the way the other seams do — raise (or return a sentinel main turns into a non-zero exit with the git error) on rc != 0, rather than []. Everything else is good to go.
…one (backend#2365) Bugbot's medium-severity finding on 9102520, and it is the sharpest one yet because it is the fail-closed rule turned on this module's own third seam. `remote_branches` collapsed a failed `git for-each-ref` into `[]` with no problem string, and `main` printed that as `0 branch(es)`, exit 0 -- a clean bill of health from a read that never happened, while the other two seams (`pull_requests`, `default_branch`) refuse explicitly. "I could not read the branch list" and "this clone has no branches" are different answers and only one of them is a fact about the clone. It returns `(refs, problem)` now and `main` refuses with exit 2, because without the ref list there is no tip for any branch and no way to tell whether a named one exists -- so every verdict below it would be weaker than it looked. A genuinely empty remote still reports no problem, pinned by its own case so the two cannot collapse back together. Bugbot's other comment on that head is stale: it restates the absent-tip finding already fixed in 9102520 and its own footer names commit 6f45cdc. Verified by construction on the current tree -- the refusal reads "and no tip was supplied to break the tie with". 80 assertions, 30 mutations, 0 stale, 0 uncaught. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
LukasWodka
commented
Aug 24, 2026
Round 2: one real finding, fixed in 7a87b83; the other comment is stale.
It returns
Suite now 80 assertions, 30 mutations, 0 stale, 0 uncaught; ruff, house-rules and That is 6 defects this review has caught between us, and every one was the same shape: a message asserting a check that never ran. Worth a line in |
…e seam and in main (backend#2365) Saqlain's requested change on #323 landed in 7a87b83, before his review arrived. This is the rest of what he asked for, and it found a fourth instance he had not seen. AUDITED ALL FOUR READS rather than fixing only the reported one. `pull_requests` and `default_branch` already refuse with a problem string. `remote_branches` was fixed in 7a87b83. `first_commit_author` had the SAME defect and worse: its condition was `if rc != 0 or not out`, conflating a FAILED `git log` with a branch that genuinely has no commits off the default branch, and `attribute` then reported the second one -- "no commit on this branch that is not already on the default branch" -- for both. A fact about the branch, from a read that never ran. It returns `(author, problem)` now, and `main` carries that reason through. THE TWO EMPTIES ARE PINNED APART IN BOTH DIRECTIONS, for both reads: collapse the failure into the success and the mutation reddens; report the genuine empty AS a failure and it reddens too. Asserting only "something empty came back" would have passed in a world where both refuse identically. AND `main` IS NOW TESTED, which is what made this worth doing. Dropping the one argument that carries a seam's problem into the rule reverted the whole fix while every isolated case stayed green -- the mutation run caught it as UNCAUGHT, so the entry point gets driven directly: a failed history read renders as unmeasured, a genuine empty renders as no-unique-commits, a failed enumeration exits 2 with no rows, and a genuinely empty remote exits 0. 92 assertions, 35 mutations, 0 stale, 0 uncaught. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 7a87b83. Configure here.
Uh oh!
There was an error while loading. Please reload this page.
LukasWodka
commented
Aug 24, 2026
@saqlainsyed007 — you were right, and the fix for the exact thing you flagged had already landed in Your finding generalised, and caught a fourth seam you hadn't seen. Rather than fix only the reported read I audited all four (derived from the call sites, not from memory —
Both empties are now pinned apart in both directions, for both reads. You'd get away with a test that only asserted "something empty came back", so each pair is mutated twice: collapse the failure into the success (reddens) and report the genuine empty as a failure (reddens). Same trap as the two indistinguishable no-author refusal arms earlier in this PR. And 92 assertions, 35 mutations, 0 stale, 0 uncaught. ruff, That's 7 defects this review has caught between the three of us, every one the same shape: a message asserting a check that never ran. If it recurs once more it belongs in Re-requesting your review; not merging. |
LukasWodka
commented
Aug 24, 2026
bugbot run |
…ot N failed ranges (backend#2365) Bugbot's `Failed first-commit read looks empty` names the seam already fixed in 6665d13, and two sub-cases with it. Both were verified by construction: an unfetched default branch and a CLI-named branch absent from the clone now report "the oldest-commit signal was not measured", naming the failed command, rather than "no commit not already on the default branch". The root cause behind the first one was still open, though. `default_branch` trusted `gh` for the NAME and never asked whether this clone has the ref, so a checkout that never fetched the remote's default produced one failed `default..branch` range per branch -- honest since 6665d13, but blaming N ranges for one missing ref, and unreadable on a repo with hundreds of branches. It is verified with `rev-parse --verify` now and reported once, as its own fact, with the fetch that fixes it. 95 assertions, 36 mutations, 0 stale, 0 uncaught. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
LukasWodka
commented
Aug 24, 2026
The seam. Your two sub-cases, verified by construction rather than assumed — both were already covered by that fix, and I checked rather than claiming it: Neither says "no unique commits" any more. But your first sub-case had a root cause still open, and that is the better catch. Also worth recording because it is the most useful thing this round produced: chasing your finding is what got 95 assertions, 36 mutations, 0 stale, 0 uncaught. Both empties pinned apart in both directions for both reads, so "could not read" and "nothing there" cannot collapse. bugbot run |
saqlainsyed007
left a comment
There was a problem hiding this comment.
Approving. The fail-open I flagged (and @cursor did) is fixed and verified: remote_branches now returns (list, problem) and signals a failed git for-each-ref instead of collapsing it to [], so main can no longer render an unreadable enumeration as a clean "0 branch(es) / exit 0". That brings the last seam in line with the module's own fail-closed contract, which the core attribute() logic already followed rigorously (PR-author precedence, never the tip author, refuse at every ambiguity). CI green, thread resolved, no open threads.
Uh oh!
There was an error while loading. Please reload this page.

Closes tracebloc/backend#2365.
The five-minute check first, because half the ticket is retracted by it
backend#2365's first task was to establish how backend#2001's per-person branch
table was derived, on the suspicion that it used the tip-commit author and was
therefore aiming each engineer's
--deleteat other people's branches.It did not. That table is attributed by PR author, and needs no rebuilding.
The two generators are recoverable from the 2026-08-14 session that produced them
(
inventory.py, thenperperson.py). The decisive line isinventory.py's,comment and all:
and
perperson.pygroups onr["login"] or r["email"] or r["aname"]— the PRauthor when there is a PR, a git identity only when there is none.
That is also visible in the published artifacts without reading the code, which is
how it was found: in
stale-remote-branches.tsv, all 1,165 rows with a PR numbercarry a GitHub login and all 28 rows without one carry an email address. A
perfect split on "does a PR exist", not on "is this identity resolvable" — and
design-system Fl-app-page-header, whose committer email is the perfectlyresolvable
…+waqaskhanroghani@users.noreply.github.com, still shows as the rawemail because it has no PR. So the fallback keys on PR existence, exactly as the
code says.
stale-remote-branches.mdeven labels its own summary "By author(branch tip / merged-PR author)".
So the destructive-policy half of the ticket is demoted, with evidence: no row of
#2001's table belongs to the wrong person, and nobody was sent to reap somebody
else's branches by it. The "never had a PR" column in that table is exactly the
set where the weaker signal was used, and it is 23 branches.
What is real, and what this PR fixes
The reporter's own incident stands, and reproduces today:
client)fix/bugbot-tier0-helm-prereqslukas@tracebloc.iofix/583-wire-ca-proxylukas@tracebloc.ioBoth are Shujaat's; a review fixup pushed onto each rewrote the tip author. The
list that nearly deleted them was a fresh ad-hoc query, not #2001's pipeline — and
that is the actual defect: the rule was got right once, in a throwaway script,
and nothing carries it forward. The next
for-each-refone-liner re-derives it,and the obvious re-derivation is the wrong one.
So this adds the one place that answers it.
scripts/branch_owner.pySignals, in priority order, each named in the output so a caller can see which one
decided:
pr-exact— the PR author, where the PR's head oid is still the branch tip.pr— the PR author, where later pushes have overtaken the PR head.first-commit— no PR: the author of the oldest commit not on the defaultbranch. (On
fix/583-wire-ca-proxythat is Shujaat while the tip is Lukas, soeven the fallback beats the tip.)
unattributable— said out loud, with the reason.The tip author is not merely deprioritised, it is unreachable:
attribute()takes no tip-author parameter, so no code path in the module can return one. A
selftest case asserts the signature, so re-adding the parameter reddens before any
behaviour test has to.
Fails closed, in four places. "No PR exists" is only a fact when the PR list
was read, completely — so an absent, unauthenticated or failing
gh, anunparseable answer, or a list that hit its own
--limitcap makes every branchunattributablerather than falling through to a commit author. That fall-throughis the same misattribution, arrived at from a clean read of the wrong thing.
Ambiguity is a refusal too: two PRs on one reused head name by two different
people, or a PR whose author is a deleted account, name the candidates instead of
picking one.
origin/HEADis read but never trusted, for the reasongit-reap's headeralready documents (stale on 9 of this org's 19 clones). It cannot affect signal 1 —
the PR query is not filtered by base — but it corrupts signal 3 exactly, because
main..branchthen includes commits that are ondevelop, whose oldest belongs towhoever landed them. So the remote is asked, and when it cannot be, signal 3 is
withheld rather than used.
Failure modes are stated in the module, not implied
Co-authored work (a PR has one author;
Co-Authored-By:is invisible here),cherry-picks and rebases (git preserves the original author, so an imported first
commit attributes to its origin), handovers (the PR author proposed it; the board
assignee owns it now, RFC-BACKEND-0008 D31), and bot/agent identities (returned
verbatim — mapping identities to people is a roster, and a hand-written roster is
what this module refuses to be).
CreateEventis not signal 2, though the ticket proposed itMeasured on
tracebloc/backend, 2026-08-24:/repos/{repo}/eventscaps at 300events (page 4 → HTTP 422), that whole window spanned 07:30Z–11:24Z, and it
held 2 branch
CreateEvents. An active repo pushes branch creations out of thewindow in hours; these branches are months old. A signal that answers for almost
nothing looks like coverage, so the module records why it is not used.
Eight defects found before review finished, all fixed and pinned
Three from the first live run, four from Bugbot, one from an audit Saqlain's
review prompted (below). Every one is the same
shape: a message asserting a check that never ran.
origin/HEAD's git short name is bareorigin, so stripping the remoteprefix leaves the remote itself and it appeared in the output as a branch called
origin. Not hypothetical: that is the same bug that put one bogus row per repointo the 2026-08-14 inventory (1212 rows vs 1193 real branches), and a sweep
built on it would try to delete a branch named after the remote.
subprocess.runraises whenghis not on PATH — a traceback out of the seam, on the machine most likely tolack
gh, is not the documented refusal. It is an exit code now.all (a branch named on the command line that this clone never fetched). A
claim about the branch, made where there was nothing to compare. The two now
read differently, and a note on stderr names any branch that is not in the
clone's refs.
Verification
scripts/tests/branch-owner-selftest.py— 95 assertions, 0 failed. Nonetwork, no git, no gh: the rule is pure and both seams are stubbed. Case 1 is
the ticket's own measured pair, by name.
scripts/tests/branch-owner-mutations.py— 36 mutations, 0 stale, 0uncaught. Every anchor is asserted to match exactly once,
--dryresolves themin
make lint, and the module is restored on every path including a crash.Three mutations were uncaught on the first run and the tests were strengthened
rather than the mutations deleted:
let control fall to the name-reuse arm and refuse for the wrong reason. The
arms are now asserted apart — a refusal test that cannot say which refusal is
a coin toss.
UNATTRIBUTABLE, whichagrees with itself whatever the module sets it to, so changing it to
"unknown"passed the whole suite. It is now written down independently.question was asked, so the remote query could be replaced by any command at
all. The command is asserted now.
Saqlain's review + Bugbot round 3 (the fourth and fifth defects). Saqlain
verified Cursor's Medium on
remote_branches— the fix had already landed in7a87b83, so his line numbers were one commit stale, but his ask was right andit generalised. Auditing all four reads (derived from the call sites, not from
memory) found
first_commit_authorwith the same defect and worse:if rc != 0 or not outmade a failedgit logand a genuinely empty range the samereturn value, so
attributereported "no commit not already on the defaultbranch" for both. It returns
(author, problem)now. Bugbot found the same seamindependently, plus a root cause behind it:
default_branchtrustedghforthe name and never checked this clone has the ref, so an unfetched default
produced one failed range per branch — blaming N ranges for one missing ref. It
is
rev-parse --verify'd and reported once now.main()is tested as a result, which is the part worth flagging. Every casebefore this exercised
attributeor a seam in isolation, so dropping the singleargument that carries a seam's problem into the rule reverted the whole fix while
the suite stayed green — the mutation run reported it
UNCAUGHT. The entry pointis now driven directly for all four outcomes.
Both empties are pinned apart in both directions, for both reads that have
them: collapse the failure into the success and report the genuine empty as
a failure — each reddens. Asserting only "something empty came back" would have
passed in a world where both refuse identically.
Bugbot round 2 found the sharpest one, fixed in the third commit:
remote_branchescollapsed a failedgit for-each-refinto[]with noproblem string, and
mainprinted that as0 branch(es), exit 0 — a clean billof health from a read that never happened, while the module's other two seams
refuse explicitly. It returns
(refs, problem)now andmainrefuses with exit2; a genuinely empty remote still reports no problem, pinned by its own case so
the two cannot collapse. (Bugbot's second comment on that head was stale — it
restated the absent-tip finding already fixed, and its own footer names the
earlier commit. Verified by construction against the current tree.)
Bugbot round 1 found 2 more of the same class, both real, both fixed in
9102520 — the multi-author reuse refusal claimed "none is at the current tip"
with no tip in hand, and a withheld first-commit signal was reported as "no
commit not already on the default branch", a fact about the branch from a check
nobody ran.
attributetakesfirst_commit_problemnow, so a withheld signalsays it was not measured; the measured-and-empty wording is pinned apart from it.
Three further mutations cover them.
Wired into
make selftestsandmake mutations, so both tiers run inside thealready-required
selftestscontext — no branch-protection edit, and nothinglands as a red gate. The wiring is itself mutation-proved: unwiring
selftest-branch-ownermakesselftests-coverfail withbranch-owner-selftest.py is not run by any target in this Makefile.make lintgreen: ruff 0.15.20 at CI's exact--isolated --select E4,E7,E9,F,house-rules --all(no findings), action-pins, mint-scope, actionlint,mutations-dry.
make selftestsgreen (all 17 suites).End-to-end on the real repo, which is where the three defects above came from:
and across all of
client: 32pr-exact, 1pr, 1first-commit, 0unattributable.
org-standards.mdOne line, next to the existing "your branches are yours to clean up" rule, saying
what "yours" means and pointing at the tool. It reaches every repo's CLAUDE.md
through the existing sync, so humans and AI sessions read the same rule — which
matters here, because the wrong rule is the one that is easier to type.
Not done, deliberately
branch_owner.pyonlyreads.
git-reapis untouched. It reaps the caller's own local branches, soownership never arises there; a helper it does not call would be dead code in a
shipping tool. Its header is where the fail-closed posture used here came from.
house-rulesgrep for%(authorname)was considered and skipped: zero currenthits, and
%(authorname)is legitimate for "who touched this last". The rulelives in the module and in
org-standards.mdinstead.🤖 Generated with Claude Code
Note
Medium Risk
New ownership helper that other cleanup/sweep scripts are expected to call; a wrong attribution could target another engineer’s unlanded branch. Mitigated by fail-closed refusals, no delete path, and a mutation-proved selftest.
Overview
Defines one ownership rule so a “my branches” list cannot aim
--deleteat someone else’s work after a review fixup rewrote the tip author (backend#2365).branch_owner.pyattributes in order: PR author at the current tip (pr-exact), PR author after later pushes (pr), oldest commit not on the default branch (first-commit), orunattributablewith a reason. The tip author is unreachable by construction (attribute()has no such parameter). Unreadable or truncatedghoutput, reused head names with different authors, deleted PR authors, and untrusted default-branch caches refuse rather than guess.Wired into
make selftests/make mutationswith a stubbed selftest and mutation harness.org-standards.mdnow states that “yours” means the PR you opened and points callers at this script. Read-only;git-reapis unchanged.Reviewed by Cursor Bugbot for commit ba2dada. Bugbot is set up for automated code reviews on this repo. Configure here.