Uh oh!
There was an error while loading. Please reload this page.
fix(2243): one branch→Status mapping — and it surfaced a column that does not exist - #295
Conversation
…t does not exist Two workflows decide a card Status from the branch a merge landed on, and they held THREE copies of the rule between them: advance-deploy-env had one and READ the per-repo `.kanban.yml` override; kanban-closure-router had two and IGNORED it. Both write Status, and a PR merged to develop fires both -- the router on pull_request closed, advance on the push -- so with a `.kanban.yml` present they would write DIFFERENT statuses for the same merge and run ordering decided which stuck. The router could not have honoured the override even in principle: it never checks the caller out. That is why the fix is one shared mapping rather than a second copy of the yq read -- the override has to be FETCHED. WHAT THIS FOUND, which is the part worth reading. `Staging (human review)` was an accepted override value in advance-deploy-env and is NOT a column on the board -- measured against project #2, whose Status options are Backlog, North Stars, Ready, In progress, Code review, On dev, Staging (agent review), FR on staging, Ready for prod, Prod, Done, Cancelled. A repo that had used it would have had its write rejected for naming a column that does not exist. Nothing caught it because the vocabulary lived in a shell `case` that kanban-columns-check.py never read. So kanban-columns-check now IMPORTS the mapping instead of regex-scraping two workflows for it. That is strictly stronger -- it reads the data structure rather than a rendering of it -- and it SHRINKS the regex surface rather than growing it. Both rewired workflows come out of WRITERS with the reason stated, since their literals are legitimately gone. The fold happens in `written_names`, not in `main`: the selftest substitutes `written_names` to control its input against a fake board, and folding into main silently widened what the selftest could not see. Caught by that selftest going red. A GUARD SO THE MAPPING CANNOT BE RE-COPIED. Its first version matched any `STATUS="On dev"` and flagged three innocent sites -- a sibling-merge holding state and two no-base-ref floors. Those are policy defaults for cases where there IS no branch, not copies of the mapping, and a guard that cannot tell the difference gets argued with and then switched off. Narrowed to a branch-keyed case arm, with both directions asserted. 22 selftest cases, 10/10 kanban-columns, all 10 .github selftests green, ruff clean, actionlint clean, make check green. backend#2243 Co-Authored-By: Claude Opus 5 <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.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
saadqbal
left a comment
There was a problem hiding this comment.
The consolidation is right and finding the phantom column is a genuinely good catch. But there are five open Bugbot threads under a Cursor Bugbot=neutral check — two High — and I verified three of them. One of the Highs I'd fix differently to how it's framed, and that matters because the obvious reading resurrects two closed incidents.
High branch_status_map.py:108 — confirmed, and the repo already settled this
try:
importyamldoc=yaml.safe_load(raw)
exceptExceptionasexc:
sys.stderr.write(f"::warning::.kanban.yml in {repo} did not parse ({exc}); ...")
return {}ImportError lands in the same arm as a parse failure. Two things follow: the override silently doesn't apply, and the diagnostic lies — on a missing module the file parsed fine, so did not parse sends the reader after the wrong thing.
What makes this more than a nit is that three sibling scripts in this repo already decided the opposite way, and one of them wrote the argument down:
mint-scope.py ::error::PyYAML is required: python3 -m pip install pyyaml
caller-drift.py ::error::PyYAML is not importable. The guard parses workflows as YAML …
standards-sync.py die("PyYAML is not importable; there is no trustworthy degraded mode")
Plus bricked-prs.yml carries an explicit Install PyYAML step, and the Makefile has a guard-pyyaml target noting the module "hard-fails without PyYAML by design. Measured with the module blocked". There is no trustworthy degraded mode is exactly the situation: an empty override map isn't a partial read, it's the wrong answer wearing a warning. Hard-fail like its siblings, and add the install step to both rewired workflows.
High kanban-closure-router.yml:101 — real, but do not fix it the way it reads
Bugbot is right that the case overwrites the mapper. It is wrong to imply the case is the mistake. That block is load-bearing and the comment says why:
A PR merged into a SIBLING feature branch deploys nothing by itself … this fallthrough silently stranded 6 such cards (backend#1437 mechanism 1) … The Status write is deliberately KEPT: skipping it lets the project's built-in "Item closed" automation set closed items to Cancelled, archiving shipped-via-parent work as abandoned (Bugbot, .github#157).
Delete or bypass that and you reintroduce both. The actual defect is ordering: the sibling rule fires on "branch is not one of the four stock names", which is also true of every legitimate non-stock override key — so it wins unconditionally over an explicit .kanban.yml, including the rfcs case this PR exists to unblock.
The mapper already knows the difference; it just doesn't say so. It computes it for the notice and then drops it:
ifoverride.get(branch):
sys.stderr.write(f"::notice::.kanban.yml overrides {branch} -> {status}\n")
...
print(json.dumps({"status": status, "env": env}))Emit it — {"status": …, "env": …, "overridden": bool(override.get(branch))} — and gate the fallthrough on it, so the sibling rule applies only where nobody stated an intent:
case"$BASE_REF"in
main|master|staging|develop) : ;;
*) if [ "$OVERRIDDEN"!="true" ];then STATUS="On dev"; SIBLING="true";fi ;;
esacThat keeps both prior fixes intact and lets a declared override through, which is the whole point of the change.
Medium kanban-columns-check.py:56 — confirmed, and it is this PR's own thesis one step out
The router was dropped from WRITERS on the grounds that its Status literals are gone. They are not — there are six: On dev at :113, :190, :196, Cancelled at :118, :208, and Done at :204. Cancelled and Done aren't in the imported mapping, so the guard can go green while those writes name columns the board may not have.
Read that against the guard's own docstring:
A written name that does not resolve means the card is not moved — and until .github#246 that was a
::warning::on a GREEN run, so the board silently stopped tracking the pipeline. The board freezing and the board working looked identical.
And against this PR's headline finding: Staging (human review) went undetected precisely "because the vocabulary lived in a shell case that kanban-columns-check.py never read". Removing the router from WRITERS while six literals remain recreates that blind spot for three more names, in the same PR that documents how expensive it was the first time. Either keep the router in WRITERS, or move those six literals into the shared mapping so the import genuinely covers them.
The rest
I didn't independently verify the two remaining Mediums (kanban-reconcile.yml's separate closer map, and kanban-columns.ymlpaths: not including the new mapping file), but both are the same shape as what I did confirm — a second reader of one rule, and a guard that doesn't run on the file it now depends on. The paths: one looks cheap and I'd take it in this PR.
The core change is right and I'd approve it with those closed. Three copies of a branch→Status rule with only one honouring .kanban.yml, where a develop merge fires both writers and run ordering decided which stuck, is a real latent conflict — and "no repo has one today, which is the only reason this never fired: the documented feature has never executed, and the first repo to adopt it inherits the bug" is the right way to characterise a dormant defect. Noticing that the router never checks the caller out, so the override has to be fetched rather than read, is the insight that makes one shared mapping the correct shape rather than a second yq.
Worth flagging separately: this is the second PR today where Cursor Bugbot reported neutral with open findings beneath it — five here, two on release-train#104. A neutral in the rollup reads as "fine" at a glance, and it isn't.
Bugbot on .github#295. Two Highs, and both meant the override still never applied -- so the PR would have shipped a refactor with the bug intact. HIGH 1 -- PyYAML is not on the runner. `read_override` imported `yaml` lazily and treated ImportError exactly like a parse miss: a warning and an empty override. Neither rewired workflow installs it, so a real `.kanban.yml` would have been silently ignored and both writers would have kept the defaults -- failing open in the one environment that matters. Parsed with `yq` now, which is what advance-deploy-env used before this refactor, so it is a dependency this path already had. A present file with no parser REFUSES rather than defaulting: an unread override is not "no override". HIGH 2 -- the router `case` clobbered any non-stock override. Both sites called the mapper and then a `case` over the four stock branch names overwrote ANY other result with the On dev floor. So a `.kanban.yml` key for a branch outside those four was computed and discarded on the same run -- including the rfcs override this change exists to unblock. Keyed on whether the mapper ANSWERED now, which also removes the last branch-name list from that workflow. MEDIUM -- kanban-reconcile held the second copy, justified by "identical, deliberately" (.github#126). backend#2243 made that justification unsafe: once the router honours an override, an identical copy is no longer identical, and the weekly sweep would move an overridden card BACK onto a deploy column -- the slower one silently undoing the faster one, exactly as that comment predicted, through the mechanism it was written to prevent. It calls the mapper now; the Status-name to option-id case that remains is a different relation, not a second copy of the rule. MEDIUM -- a mapping-only PR never ran the board-name check, because `branch_status_map.py` was not in kanban-columns.yml `paths:` even though the check now imports it. Added, with its own selftest assertion: WRITERS does not name it (it is not a workflow), so the existing WRITERS-vs-paths case could never have covered it. MEDIUM -- I dropped kanban-closure-router from WRITERS on the claim its literals were gone. Only its BRANCH MAPPING moved; it still writes Cancelled, Done and On dev, and Cancelled/Done are not in the imported mapping -- so the conformance check could have gone green while those two named columns the board no longer has. Restored, with the reason. advance-deploy-env stays out because it now writes no literal at all, which is checkable rather than asserted. Also fixed a shell syntax break of my own: replacing the `case` heads left their arms and `esac` behind. actionlint caught it. 11/11 kanban-columns, 22/22 branch-status-map, all 10 .github selftests green, ruff and actionlint clean. backend#2243 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
LukasWodka
commented
Aug 21, 2026
bugbot run |
saadqbal
left a comment
There was a problem hiding this comment.
All three of the findings I raised are fixed, two of them better than what I suggested, and all five threads are closed. Not converting to an approval yet only because selftest and Cursor Bugbot are still running — nothing else is outstanding from me.
PyYAML — you removed the question instead of answering it. I said hard-fail like mint-scope.py / standards-sync.py / caller-drift.py and add an install step. You dropped the dependency entirely and went to yq, which that path already had, and hard-fail on both modes:
exceptFileNotFoundError:
# CANNOT TELL, LOUDLY. A present `.kanban.yml` and no parser is not "no# override" -- it is an unread override, and quietly defaulting is how the# first version failed. Refuse so somebody fixes the runner.raiseSystemExit(1)
except (subprocess.CalledProcessError, json.JSONDecodeError):
# refusing rather than applying part of itraiseSystemExit(1)That's the better shape — no install step to forget in a fourth workflow later, and the diagnostic no longer says "did not parse" about a missing module.
The router clobber — your fix is cleaner than my overridden flag. I proposed a new field on the mapper's JSON plus a condition in the router. You keyed it on whether the mapper answered at all:
# KEYED ON WHETHER THE MAPPER ANSWERED, not on a list of branch namesif [ -z"$STATUS" ];thenThe mapper's silence is the signal, so no new field and no second place to keep in step — and "the last branch-name list in this workflow is gone with it" is the part that actually retires the duplication rather than moving it.
I checked the risk that inverts, because gating on emptiness makes the sibling protection depend on the mapper going quiet at the right times, and a silent failure there re-strands the six cards from backend#1437. resolve() does DEFAULT_MAP.get(branch, ("", "")), so all three cases land correctly:
| case | mapper returns | sibling logic |
|---|---|---|
| stock branch, no override | its default | skipped ✓ |
| non-stock branch with override | the override | skipped ✓ (the fix) |
| non-stock branch, no override | "" | fires ✓ (protection intact) |
resolve()'s new docstring is a good addition too — an override naming an unknown Status keeping the default environment, because "the Status is the operator's call, the environment is a derived fact, and deriving it from an unknown is a guess."
The router is back in WRITERS with the reason recorded inline (it still writes Cancelled, Done and On dev directly, and the first two aren't in the imported mapping), and the LITERAL scanner covers those. declared_names() importing from branch_status_map.py rather than regexing its source — "a regex over the module's source would be the same scrape-a-rendering mistake one layer along" — is the right call and closes the blind spot that let Staging (human review) through in the first place.
Ping me when CI lands, or I'll pick it up next pass.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
…rent door Bugbot on .github#295 round two. HIGH -- reconcile had no checkout. My own note said "branch_status_map.py is local: this job only ever runs in tracebloc/.github" -- true of the REPOSITORY and false of the WORKSPACE. A hosted runner starts empty, so the stage-derivation step would have failed on a missing file and taken the weekly backstop down mid-sweep. A comment that is confidently right about the wrong noun. HIGH -- read_override failed open on a fetch failure. It caught every `gh api` error and returned an empty map, identical to "no .kanban.yml". So a present override behind a 403, a 5xx or a rate limit was silently ignored and both writers applied the defaults: the exact override-ignore defect this change exists to close, reached by a different door. The parse and missing-`yq` paths already refused; this one did not. 404 is now the only failure that means "no override", matched on the message the way promote-repo.sh does it since `gh` exits non-zero for both. Everything else refuses. Four cases, and they assert `SystemExit` specifically rather than "it raised" -- a different exception would mean a different path, which is the rule this repo has on bare assertRaises. One of those cases failed on its first run for a reason worth keeping: the loop set the stub AFTER the call, so the 403 iteration ran against the 404 stub from the case above and reported a false failure. The test caught its own off-by-one. 26/26 branch-status-map, all 10 .github selftests green, ruff and actionlint clean. backend#2243 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
LukasWodka
commented
Aug 21, 2026
bugbot run |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
saadqbal
left a comment
There was a problem hiding this comment.
My three original findings are still fixed at 0fedbd39 — I re-checked rather than carrying forward last pass's verification, since the head moved. But two new Bugbot Highs are open, both real, and the second one is partly my fault.
Cursor Bugbot reports neutral again with two Highs beneath it. Third time on this PR set today. Worth saying once more because a neutral in the rollup reads as "fine" and I nearly approved on the strength of last pass's clean thread count.
branch_status_map.py:156 — real, and the fix is caller-specific
No caller passes the third ref argument:
advance-deploy-env:83 branch_status_map.py "$BRANCH" "$GITHUB_REPOSITORY"
kanban-closure-router:98 branch_status_map.py "$BASE_REF" "$REPO_FULL"
kanban-reconcile:588 branch_status_map.py "$CLOSER_BASE" "$ORG/$REPO"
So every override read resolves at HEAD — the repo's default branch — rather than the branch being acted on. For advance-deploy-env that's a genuine regression: it previously read .kanban.yml off the checked-out pushed branch, so a push to main used main's mapping; now it uses the default branch's.
But don't fix this by passing the ref everywhere. The router runs on pull_request: closed, where the branch may already be deleted — your own read_override docstring says exactly that, which is why HEAD is the right default there. So:
advance-deploy-env— push event, the branch provably exists, pass"$BRANCH"as the third argument- router — keep
HEAD, and the existing docstring already justifies it - reconcile — a backstop sweeping historical closures, so
HEADis also right
A blanket change would break the router on exactly the deleted-branch case it was written to survive.
kanban-reconcile.yml:588 — real, and this one is on my review
DEST=$(python3 scripts/branch_status_map.py "$CLOSER_BASE""$ORG/$REPO"| jq -r '.status')Bare, inside a while loop, under set -euo pipefail. So the raise SystemExit(1)I asked you to add now propagates through pipefail into errexit and truncates the weekly sweep mid-loop on one unreadable .kanban.yml — 403, 5xx, rate limit, bad YAML. Bugbot's observation that the sibling closer lookups in the same loop already soft-fail is the tell: this is the one call in there that doesn't.
I was right about the writers and wrong to state it as a blanket rule. "There is no trustworthy degraded mode" holds at a decision point — advance and the router are about to write a Status, and silently applying the default mapping there produces a wrong board with no signal. It does not hold for a batch backstop, where the unit of failure should be the card, not the sweep.
So don't revert to soft-fail — that reinstates the silent default. Isolate per item instead:
if! DEST=$(python3 scripts/branch_status_map.py "$CLOSER_BASE""$ORG/$REPO"2>&1| jq -r '.status');thenecho"::warning::skipping $ORG/$REPO#$NUM — override unreadable"
SKIPPED=$((SKIPPED +1))continuefiand report SKIPPED at the end so a repo that keeps failing is visible rather than absorbed. That keeps "never silently apply the default" while letting the other cards through — which is the property a weekly backstop exists to provide.
Still good
Re-verified at this head: raise SystemExit(1) on both yq paths, the router gated on [ -z "$STATUS" ] at both sites, resolve() still returning ("", "") for an unknown branch so the sibling protection fires, and kanban-closure-router.yml still in WRITERS. None of that regressed.
…uld not survive it Bugbot on .github#295, two Highs. HIGH -- `read_override` defaulted to the API HEAD, i.e. the repo DEFAULT branch, and no caller passed a ref. So an override present on `develop` but not yet on `main` was silently ignored by both writers. And that is a REGRESSION rather than a new limitation: advance-deploy-env previously read `.kanban.yml` off the checked-out PUSHED branch. My own docstring argued for the default on the grounds that a closed PR head may be gone -- true, and irrelevant, because every branch these callers map is a BASE branch that persists. Right reasoning, wrong input. All four call sites pass the ref now, and the selftest asserts it by reading the call sites: the failure is a MISSING argument, which no unit call can show. Verified non-vacuous by dropping one ref and watching it redden. HIGH -- the mapper call could take the weekly sweep down. Making read_override REFUSE on an unreadable `.kanban.yml` was right for a single card and wrong inside a loop: a bare command substitution under `set -euo pipefail` let ONE unreadable repo truncate the whole backstop mid-sweep, while the sibling closer lookups beside it already soft-fail. It soft-fails now and SKIPS the item rather than defaulting. Falling back to the default mapping would be the fail-open this PR removed, one layer out -- the router already moved that card correctly, and this sweep exists to fix misses, not to overrule with a guess. 33/33 branch-status-map, all 10 .github selftests green, actionlint and ruff clean. backend#2243 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
LukasWodka
commented
Aug 21, 2026
bugbot run |
Uh oh!
There was an error while loading. Please reload this page.
saadqbal
left a comment
There was a problem hiding this comment.
Both Highs fixed, and I was over-cautious on one of them — you were right. The remaining Medium is real, and there's a sharper way to state it than Bugbot did.
The ref fix: you passed it in all three callers, where I said keep HEAD for the router. My reason was that on pull_request: closed the branch may be gone. Your docstring answers it:
Every branch these callers map is a BASE branch (
develop/staging/main, or a closing PR's base), which persists after a merge — so passing it is safe even though a merged head may be gone.
That's correct and I had it wrong: I was thinking of the deleted head, and every ref these callers pass is a base. Passing it is both safe and strictly better than reading the default branch. Good that the reasoning is in the docstring rather than only in the commit.
The fetch arm is a fix I didn't ask for and it's the best part of this round. Distinguishing 404 ("genuinely no override") from 403 / 5xx / rate-limit ("refuse, something may exist") closes the same defect through the door nobody was watching — "the exact override-ignore defect this change exists to close, reached by a different door" is the right description. That was reachable and silent before.
Reconcile isolation is exactly right:
if! DEST=$(python3 scripts/branch_status_map.py "$CLOSER_BASE""$ORG/$REPO""$CLOSER_BASE"2>/dev/null | jq -r '.status');thenecho"$REPO#$NUM: the branch->Status mapping could not be read … - leaving it alone rather than guessing a column"continuefiPer-card, names the card and the base, and "leaving it alone rather than guessing a column" is the distinction I was trying to draw — skip the item, don't degrade to a default. My "no trustworthy degraded mode" line holds at the writers and this is what it should look like in a sweep.
The remaining Medium at :153 — and the strongest argument for it is your own docstring.
returndocifisinstance(doc, dict) else {}Bugbot frames it as "another silent ignore", which is true. But it's more than an inconsistency with the other arms: it contradicts the documented contract two lines up.
An unreadable or absent file is an EMPTY override, not an error … A malformed one is also empty, and says so on stderr — silently applying half a parsed map would be worse than ignoring it.
"Says so on stderr" is true for a YAML/JSON parse failure — that arm raises with ::error::. It is false for a branch_status_map that parses fine but isn't a mapping: no message, no exit code, {} returned. So the file now has three refuse-loudly paths (non-404 fetch, missing yq, parse failure), one legitimate empty (404), and one path that is silently empty while the docstring says it announces itself.
One line closes it and makes the docstring true:
ifnotisinstance(doc, dict):
sys.stderr.write(f"::error::.kanban.yml in {repo} has a `branch_status_map` that is "f"{type(doc).__name__}, not a mapping; refusing rather than ignoring it\n")
raiseSystemExit(1)
returndocThat's the fourth instance of the same principle in this file, which is worth noticing on its own: every time someone looks, another door onto "fetched, unusable, silently defaulted" turns up. The set is now enumerable — fetch, parser-missing, parse, shape — so this should be the last one.
Nothing else outstanding from me.
…id otherwise
Bugbot Medium, and saadqbal made the argument better than Bugbot did: this
is not merely an inconsistency with the other arms, it CONTRADICTS the
contract documented two lines above it.
The docstring claimed "a malformed one is also empty, and SAYS SO on
stderr". True of a YAML/JSON parse failure, which raises with ::error::.
False of a `branch_status_map` that parses fine and is a list: no message,
no exit code, {} returned. So the file had three refuse-loudly paths, one
legitimate empty, and one that was silently empty while the docs said it
announced itself.
A documented contract that one path quietly breaks is worse than an
undocumented one, because the next reader trusts it -- and I was that
reader, twice, on this PR.
The arm refuses now, and the docstring is a TABLE of all five outcomes
rather than prose that drifted from three of them. The cases assert the five
as a SET, not one at a time, because the defect was an inconsistency BETWEEN
them. Plus the inverse -- a real mapping still comes back -- so the guard
cannot degenerate into "refuse anything".
37/37, all 10 .github selftests green, ruff clean.
backend#2243
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>LukasWodka
commented
Aug 21, 2026
Closed. And thank you for the correction on the ref — it is worth being explicit that you retracted a review point of your own and that I had it right, because that is the harder direction for a reviewer to go. The part I want on the record is your reframing of the That is the second time on this PR that my own comment was the thing that was wrong: 37/37, all 10 selftests green, ruff and actionlint clean. |
LukasWodka
commented
Aug 21, 2026
bugbot run |
Uh oh!
There was an error while loading. Please reload this page.
saadqbal
left a comment
There was a problem hiding this comment.
The non-dict arm is fixed — and ad1a7f3d's subject is the right way to describe it. Bugbot's new Medium is real and understated, and I think I can tell you why the removal happened and what the clean fix is.
It isn't an unverified claim — the names are already there, and there are thirteen
Bugbot says a new literal write in advance-deploy-env.yml would slip past. The stronger fact is that thirteen Status names are in that file right now, in rank():
Backlog · North Stars · Ready · In progress · Code review · On dev
Staging (agent review) · FR on staging · Ready for prod · Prod · Done · Cancelled
Staging (human review)
And the file documents its own failure mode at rank 7:
An unknown Status returns "" here, the guard below fails, and evaluation falls through to strict equality: the card BLOCKS every prod promotion carrying it. That is the backend#1411 shape … a live landmine waiting for the first card to land in it.
So a board rename doesn't just mis-sort a card here — it blocks every prod promotion carrying it. That is squarely what kanban-columns-check.py exists to prevent ("the board freezing and the board working looked identical"), and dropping the file from WRITERS took all thirteen outside the check.
This is also the secondWRITERS removal on this PR that turned out to be wrong. I flagged the router earlier (six literals, restored). Two hand-removals, two errors, is enough to say the curated tuple is the wrong shape — but see below, because the naive fix doesn't work either.
Why it was removed, and the fix that actually closes it
WRITERS is a write-side name, and rank() is read-side: it consumes a Status rather than emitting one. On that reading the removal is technically defensible — which is probably how it happened. But the check's purpose is "every Status column name … must exist on the board", and a read-side name that doesn't resolve breaks just as loudly.
And adding the file back naively would fail the check — which is the part worth knowing before someone tries it. Twelve of those thirteen are exactly the live board options this PR measured. The thirteenth, Staging (human review), is the phantom column this PR's own headline finding identified as not existing. It's in rank() deliberately:
Both names rank the same so the monotonic guard is stable across the #1592 rename instant.
So the clean fix is two steps, and the first one is overdue anyway:
- Retire the
"Staging (human review)"arm. The rename instant has passed — your own measurement against project #2 is the evidence, and D7-style rename shims are exactly the thing this PR is removing elsewhere. Keeping a shim for a rename that completed is howadvance-deploy-env.ymlcame to accept an override value naming a nonexistent column in the first place. - Then put the file back in
WRITERS, where its remaining twelve names get checked against the board like everyone else's.
And the structural point, for a follow-up
Curating WRITERS by hand is the root cause of both misses. Derived is one loop:
# Any workflow naming a Status column is subject to the board check, whether it# writes one or ranks one — an unresolved name breaks both.forwfinsorted((ROOT/".github/workflows").glob("*.yml")):
ifSTATUS_LITERAL.search(wf.read_text()):
assertwf.nameinWRITERS, f"{wf.name} names a Status column but is not checked"That's the same shape as the in-step-clipper test on tracebloc-engine#689 — discover the set rather than enumerate it — and it would have caught both the router and this one without anybody reasoning about write-side versus read-side.
Nothing else outstanding from me. Fifth round with Cursor Bugbot at neutral over an open finding, which is now less a coincidence than a thing to fix in how that check reports.
…rusted saadqbal on .github#295, and his framing is stronger than Bugbot Medium. Bugbot said a NEW literal in advance-deploy-env.yml would slip past. The fact is thirteen Status names are in that file RIGHT NOW, in `rank()`, and dropping it from WRITERS took all thirteen outside the check. The file documents its own failure mode: an unknown Status returns "" from rank(), the guard falls through to strict equality, and the card then BLOCKS every prod promotion carrying it. A read-side name that does not resolve breaks at least as loudly as a write-side one -- so "WRITERS is a write-side name" was technically defensible and wrong about what the check is for. That was the SECOND wrong removal on this PR; the router was the first. And the naive fix would have failed the check, which is worth knowing before someone tries it: twelve of the thirteen are live board options, and the thirteenth is `Staging (human review)` -- the phantom column this PR own headline finding identified. So, in order: 1. RETIRE THE SHIM. It ranked alongside `FR on staging` to keep the monotonic guard stable across the #1592 rename INSTANT, and that instant has passed -- measured against project #2. Keeping a shim for a completed rename is how this same file came to accept an override naming a nonexistent column. 2. Restore the file to WRITERS, where its remaining twelve get checked. Two hand-removals, two errors, is enough: `unlisted_namers()` derives against the tuple now. Any workflow naming a board column on a CODE line while absent from WRITERS is a finding, with a two-row exemption list carrying reasons. It found two on its first run, which is the argument for it existing: kanban-archive.yml:104 selects the three terminal columns to archive, and wip-limit-check.yml:47 defaults its column input to `Code review`. Neither writes a Status; neither name was checked. A rename would have left the archiver archiving nothing and the WIP check counting an empty column -- both indistinguishable from a quiet board. Three things the tests caught while doing this, all mine: the guard fired inside the stubbed-board cases and short-circuited main() (stubbed in run(), like cross_check, with the reason written down); my own cases got that stub because run() patches permanently, so they use kcc_fresh as the cross_check cases already did; and the paths-filter regex matched only CONSECUTIVE list items, so my interleaved comment truncated the block and produced a false "uncovered" that took two rounds to read as a parser artefact. 16/16, all 10 selftests green, ruff and actionlint clean. backend#2243 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
LukasWodka
commented
Aug 21, 2026
bugbot run |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Bugbot High + Medium on .github#295, and both are about claims I wrote one commit earlier. HIGH -- putting advance-deploy-env.yml back in WRITERS covered NONE of rank()s twelve names. `LITERAL` matches an assignment idiom and a `case` arm has no `=`; WRITERS membership also makes `unlisted_namers` SKIP the file. So all twelve stayed invisible -- and `Backlog`, `North Stars` and `Ready` were collected from nowhere at all -- while the comment I had just added claimed they were covered. Measured, not argued: 12 of 12 unattributed before, 0 after. `written_names` now collects every board column a WRITERS file NAMES, not only the ones it assigns, through one `names_in` helper that `unlisted_namers` also uses. Two copies of "does this file name a column" is the drift this file keeps finding in itself. MEDIUM -- the stale-exemption check the docstring promised did not exist. `unlisted_namers` said stale entries "are reported by the caller for the same reason mint-scope.py reports its own", and main() never diffed them. It does now, with three expiry conditions asserted separately because they are different facts: the file is gone, it joined WRITERS, or it names no column any more. Plus the inverse, so a live exemption is not a permanent red. The exempt dict moved to module level so both functions read ONE copy. AND FIXING THE HIGH NEARLY CREATED ITS TWIN. The staleness guard tested `if not found`, and `found` now has three contributors -- so case-arm names alone could have satisfied it while `LITERAL` matched nothing: the guard disarmed by the very change that broadened the collection. It is keyed on the assignment pass now, and I broke the pattern to watch it fire. 7 cases (23/23), all 10 selftests green, make check 41/41, ruff clean. backend#2243 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
LukasWodka
commented
Aug 21, 2026
bugbot run |
Uh oh!
There was an error while loading. Please reload this page.
saadqbal
left a comment
There was a problem hiding this comment.
My finding is closed, and the derived check earned itself on the first run — which is the best outcome I could have asked for. The new High is real, and I think it's the third downstream consequence of a recommendation I made, so let me name the pattern rather than just the instance.
The guard found two files nobody had looked at
unlisted_namers() exists now, and its own comment records what it caught:
kanban-archive.yml:104SELECTS the three terminal columns (Prod/Cancelled/Done) to archive, andwip-limit-check.yml:47defaults its column input toCode review— and neither name was being checked against the board. A rename would have left the archiver silently archiving nothing and the WIP check counting an empty column, both of which look exactly like a quiet board.
Neither Bugbot nor I flagged those. That's the argument for deriving a set rather than curating one, made by the check itself on its first execution. And opt_either("Staging (human review)", "FR on staging") is a better answer to the alias than my "retire the shim" — it lets the pair be legal during a rename and illegal after, which is the property you actually want.
The new High is real, and the no-write path here is not neutral
Bugbot is right that a read_override refusal exits the target step before any Status is published, and that the update step's non-empty guard then skips. What makes it serious is something this file already documents about itself:
skipping it lets the project's built-in "Item closed" automation set closed items to Cancelled, archiving shipped-via-parent work as abandoned (Bugbot, .github#157)
So the built-in automation acts on the close independently of this workflow. A loud red run doesn't protect the card — the automation still wins the race. Writing nothing isn't abstention here; it's delegating the decision to something that decides wrongly.
And this is the third consequence of a line I wrote. I told you "there is no trustworthy degraded mode" and to hard-fail rather than silently default. That was right for the mapping, and it has now cost three separate fixes: the reconcile sweep truncating mid-loop, then the ref-resolution regression, now this. The principle holds, but I stated it too broadly, and the missing qualifier is this:
"Refuse rather than guess" assumes that doing nothing is safe. At a decision point whose default is supplied by another system, refusing has to be expressed as a write, not as an absence.
The file already contains the right pattern for exactly this — the sibling-branch arm deliberately keeps its Status write for this precise reason:
STATUS="On dev"
SIBLING="true"So an override-read failure wants the same treatment: write the non-terminal holding state, label it for the weekly pass, and surface the failure as a warning — rather than exiting and letting Cancelled land. That keeps "never silently apply the default mapping" (the holding state isn't the mapping's answer, it's an explicit we could not tell) while denying the built-in automation the opening.
One process observation, six rounds in
Every round on this PR has closed its findings and surfaced new ones, and all of them have landed in the same place: what happens when the override read fails. The mapping consolidation and the two guards look genuinely settled now — unlisted_namers(), opt_either, the [ -z "$STATUS" ] gate, the hard-fail arms all verified across rounds. The failure-mode design is where the cost keeps recurring, and that's separable.
Worth considering landing the mapping plus the guards, and taking the override-read failure semantics as its own change — one place to reason about all three consumers' no-write paths at once, instead of discovering them one workflow at a time. Entirely your call; it's an observation about where the findings cluster, not a request.
Bugbot High on .github#295, and it is NEW -- the old in-file `case` could not fail this way. `read_override` refuses on an unreadable `.kanban.yml`, and a bare command substitution under `set -euo pipefail` then exits the target step before any Status is published. The update step non-empty guard skips, and the project built-in "Item closed" automation sets `Cancelled` and archives shipped-via-parent work -- the .github#157 no-write path this same file documents, reached by a door I opened. And it fires exactly when the first repo adopts the override this change exists to unblock. THE POLICY DIFFERS BY CALLER, and that is the substance rather than a detail. reconcile SKIPS the item: it is a backstop that fixes misses, so writing a guessed column would overrule a router that already got it right. The router writes the DEFAULT: publishing nothing is strictly worse than ignoring an override for one run, because one is recoverable and the other leaves a card Cancelled. So `read_override` still refuses, and `--no-override` is how a caller asks for the answer it can safely fall back to. Neither caller gets a silent default -- the router logs a ::warning:: naming the repo, the branch and the reason. The fallback is proven to consult NOTHING by running it with `gh` removed from PATH, and the same call WITHOUT the flag is asserted to refuse -- so the flag is doing the work rather than a silent default. The call-site assertion failed on its own first run and the fix is the interesting part: it counted the new fallback calls and demanded a ref of them. A `--no-override` call consults no file by definition, so demanding a ref would demand the opposite of its purpose. It now distinguishes the two shapes, and additionally asserts the router has a fallback in BOTH arms while reconcile has NONE -- the per-caller policy, pinned. 47/47, all 10 selftests green, make check 41/41, actionlint and ruff clean. backend#2243 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
LukasWodka
commented
Aug 21, 2026
bugbot run |
There was a problem hiding this comment.
✅ 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 94740a5. Configure here.
…he default saadqbal on .github#295, correcting my own fix from one commit ago -- and the qualifier he adds is the part worth keeping: "Refuse rather than guess" assumes that doing nothing is safe. At a decision point whose default is supplied by ANOTHER system, refusing has to be expressed AS A WRITE, not as an absence. My first fix wrote the DEFAULT MAPPING on an unreadable override. That denies the built-in Item-closed automation its opening, which was the point -- but it claims a promotion happened on a read that failed, which is exactly the "silently apply the default mapping" this PR removed everywhere else. The router writes the non-terminal HOLDING STATE now (`On dev`) and labels the card `override-unreadable` for the weekly pass, modelled on the sibling-merge arm three lines down, which keeps its Status write for precisely this reason. The holding state claims nothing: it is an explicit WE COULD NOT TELL. And it is still a write, because the built-in automation acts on the close independently of this workflow -- a loud red run does not protect the card, the automation wins the race. `--no-override` is gone from the router. It stays in the CLI because the flag itself is right and tested; nothing calls it today, which the selftest now asserts rather than leaving to be discovered. THE THREE CONSUMERS NO-WRITE PATHS ARE PINNED AS A SET, because that is what the finding is about -- three callers, three different correct answers, each for a stated reason: router writes the holding state + label; another system would decide reconcile skips the item; it fixes misses, nothing else acts on its silence advance no fallback; a push has no competing automation Reverting one arm to the old default-mapping fallback drops the suite to 48/2, so the distinction is checked rather than described. 50/50, all 10 selftests green, make check 41/41, actionlint clean. backend#2243 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
LukasWodka
commented
Aug 21, 2026
Taken — the holding state is in, and the qualifier you added is the thing I want on the record:
My fix wrote the default mapping. That denied the built-in automation its opening, which was the point — but it claims a promotion happened on a read that failed, which is the exact "silently apply the default mapping" this PR removes everywhere else. Yours claims nothing: the holding state is an explicit we could not tell. So the router writes The three consumers' no-write paths are pinned as a set, because that's what the finding is actually about — three callers, three different correct answers, each for a stated reason:
Reverting one arm to the old default-mapping fallback drops the suite to 48/2, so the distinction is checked rather than described. On "three consequences of a line I wrote"I'd put it differently: the line was right and the scope was mine to get right. You gave me a principle about the mapping; I applied it uniformly across three callers with different blast radii without asking what "doing nothing" cost in each. Reconcile truncating mid-loop, the ref regression and this one are all one mistake made three times — not three consequences of your advice. On landing the mapping and taking failure semantics separatelyYour read of where the findings cluster matches mine, and I'd normally take that split. I'm going to argue for landing this as one PR anyway, and it's a close call: The three no-write paths are now decided and asserted together, which is the thing a separate PR would have been for. Splitting now would mean either reverting working code to re-land it, or landing a mapping whose three consumers have a known-wrong failure mode with a follow-up ticket — and a known-wrong path with a ticket attached is how the What I take from your observation instead: the clustering was a signal I should have read three rounds ago as "stop fixing instances, go enumerate the consumers". That enumeration is what the table above is. Your call if you'd still rather split it — it's your review, and if you want the mapping in today I'll cut the failure semantics out onto its own branch rather than argue further. 50/50, all 10 selftests green, |
LukasWodka
commented
Aug 21, 2026
bugbot run |
There was a problem hiding this comment.
✅ 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 8ac1bf7. Configure here.
saadqbal
left a comment
There was a problem hiding this comment.
This is fixed, and fixed better than I suggested. Two things I want to credit specifically before the one thing I'd still change.
You rejected the fix I implied and were right to. I said the answer was "patterned by the file's own sibling case", which reads as write the default mapping. 94740a54 did that; 8ac1bf7c changed it to the holding state, with the reason recorded:
Writing the default would be "silently apply the mapping we could not confirm" -- it claims a promotion happened on a read that failed. The holding state claims nothing: it is an explicit WE COULD NOT TELL.
That's the correct distinction and I didn't draw it. "On dev" on a card whose PR merged to main understates progress, which is recoverable; the default mapping would have asserted a promotion nobody verified. Understating beats asserting-on-a-failed-read, and it's the direction the rest of this repo fails in.
The three consumers are now pinned as a set, each with its own stated answer rather than one standing in for all three — router writes the holding state and labels, reconcile skips, advance-deploy does its own thing. That's the part I asked for in the process note and it's better executed as a test than it would have been as a separate PR. And eq("the router does NOT fall back to the default mapping", "--no-override" in _router, False) is a negative assertion pinning your own self-correction, so the 94740a54 version can't quietly come back.
The PATH="/nonexistent" mutation is the right way to prove --no-override touches no network, and asserting that the same call without the flag refuses is what makes it non-vacuous — without that pair, a silent fallback would pass both.
One thing left: 2>/dev/null throws away the only thing a human needs
:115 and :225 both redirect the mapper's stderr to /dev/null. That stderr is not noise — branch_status_map.py writes five distinct cause-naming annotations, and the script's own comment at :106 treats saying so on stderr as a property that matters:
:130 ::error::.kanban.yml in {repo} could not be fetched …
:135 ::error::could not run `gh` to read .kanban.yml in {repo} …
:157 ::error::.kanban.yml is present but `yq` is not installed …
:162 ::error::.kanban.yml in {repo} did not parse ({exc}) …
:169 ::error:: …
All five collapse into one message that says only "the override for X could not be read". Those causes need opposite responses: yq missing is a runner-image problem affecting every repo at once, a parse failure is one repo owner's typo, and a fetch failure may be transient and self-healing. The card gets labelled either way, so whoever works the weekly pass is the audience for this text — and it can't tell them which of the three they're looking at, with the answer having been written and then discarded one line earlier.
This is the standard you applied in e2e-test-agent#184, in almost the same words:
A missing binary, an absent namespace and an RBAC denial would all have been recorded as a wedged apiserver, and triage routes on that text.
In fairness, the file does discard stderr elsewhere — :192 and :331 — so this isn't a departure from local convention. But those two feed sentinels (CLOSER="ERROR|", RESP='{}') into logic that handles them; this one's entire output is a human-facing triage message, which makes the cause the payload rather than a detail.
And I think I can see why the redirect is there, which is why I'd suggest a specific shape rather than just "delete it": those are ::error:: annotations, so letting them through would put error annotations on a run that deliberately does not fail — making a correct, self-healing outcome look broken in the UI. That's a real concern and worth keeping. Capturing rather than discarding satisfies both:
map_err=$(mktemp)if! STATUS=$(python3 .kanban-map/scripts/branch_status_map.py \"$BASE_REF""$REPO_FULL""$BASE_REF"2>"$map_err" \| jq -r '.status');thenecho"::warning::… Writing the non-terminal holding state … The mapper said:"
sed 's/^::error:://; s/^/ /'"$map_err"
STATUS="On dev"
UNREADABLE_OVERRIDE="true"fi
rm -f "$map_err"Stripping the ::error:: prefix keeps it out of the annotations list while putting the cause in the log next to the warning that sent someone looking. Same fix in both arms.
Not blocking — the card is protected either way, which was the actual defect, and that's closed. Approving; this is worth a follow-up commit whenever you're next in the file.
LukasWodka
commented
Aug 21, 2026
/fr-pass |
Closes backend#2243.
The defect
Two workflows decide a card's
Statusfrom the branch a merge landed on, and theyheld three copies of the rule between them:
.kanban.yml?advance-deploy-env.ymlkanban-closure-router.ymlBoth write
Status, and a PR merged todevelopfires both — the router onpull_request: closed, advance on thepush. So with a.kanban.ymlpresent theywould write different statuses for the same merge, and run ordering decided which
stuck. No repo has one today, which is the only reason this never fired: the
documented feature has never executed, and the first repo to adopt it inherits the bug.
The router could not have honoured the override even in principle — it never checks
the caller out. That is why the fix is one shared mapping rather than a second
yqread: the override has to be fetched.
What this found
Staging (human review)is not a column on the board. It was an accepted overridevalue in
advance-deploy-env.yml. Measured against project #2, the Status options are:A repo that had used it would have had its write rejected for naming a column that
does not exist. Nothing caught it because the vocabulary lived in a shell
casethatkanban-columns-check.pynever read.So that check now imports the mapping instead of regex-scraping two workflows for
it. Strictly stronger — it reads the data structure rather than a rendering of it —
and it shrinks the regex surface rather than growing it. Both rewired workflows come
out of
WRITERSwith the reason stated, because their literals are legitimately gone.Two things the tests caught in this PR
The fold has to be in
written_names, notmain. The selftest substituteswritten_namesto control its input against a fake board; folding the declaredvocabulary into
mainsilently widened what the selftest could not see. It went redand that is how I found out.
The re-copy guard's first version was too broad. It matched any
STATUS="On dev"and flagged three innocent sites — a sibling-merge holding state andtwo "this closer has no base ref" floors. Those are policy defaults for cases where
there is no branch, not copies of the mapping, and a guard that cannot tell the
difference gets argued with and then switched off. Narrowed to a branch-keyed
casearm, with both directions asserted: it matches the real shape and ignores the
innocent ones.
Verification
scripts/tests/branch-status-map-selftest.py— 22 cases, input domain derivedfrom
DEFAULT_MAPrather than hand-listedkanban-columns-selftest.py— 10/10.githubselftests green,ruffclean,actionlintclean on bothrewired workflows,
make checkgreen (41/41)Follow-up, not this PR
kanban-reconcile.ymlandfr-gate.ymlalso mention column names, but asrank orderings rather than branch mappings — a different rule that deserves its own
look, not a rushed merge into this one.
Unblocks backend#2242 option 3 (routing
rfcsoff deploy states), which needed aworking override on both writers.
🤖 Generated with Claude Code
Note
High Risk
Changes how Status is chosen on PR/issue close, deploy pushes, and the weekly reconcile sweep. A mapping or fallback bug can Cancel, park, or overwrite kanban cards across the org.
Overview
Unifies the branch→Status rule so
advance-deploy-env,kanban-closure-router, andkanban-reconcileno longer disagree when a repo has.kanban.yml. Previously only advance read the override; the other two sites ignored it, so the first adopter would get racey Status writes.New
branch_status_map.pyis the single mapping. It fetches.kanban.ymlvia the API (the router never checks the caller out) at the mapped branch ref, not the repo default. 404 means no override; 403/5xx/bad YAML/wrong type refuse. Callers then differ on purpose: the router writes the On dev holding state and labelsoverride-unreadable(so GitHub’s Item-closed automation cannot set Cancelled); reconcile skips that item; advance has no fallback.Also drops the dead
Staging (human review)rank shim, imports the mapping intokanban-columns-check.pyinstead of scraping YAML, and guardsWRITERSso unnamed column users cannot slip past.Reviewed by Cursor Bugbot for commit 8ac1bf7. Bugbot is set up for automated code reviews on this repo. Configure here.