Skip to content

fix(closure-router): a manual issue close must not invent a deploy state - #67

Merged
LukasWodka merged 1 commit into
developfrom
fix/kanban-manual-close-no-prod
Jul 26, 2026
Merged

fix(closure-router): a manual issue close must not invent a deploy state#67
LukasWodka merged 1 commit into
developfrom
fix/kanban-manual-close-no-prod

Conversation

@LukasWodka

@LukasWodkaLukasWodka commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Summary

Closing an issue by hand as completed moved its kanban card to Prod. This fixes that: with no closing PR, the router now leaves Status alone.

The mechanism (not a guess — traced end to end)

In the issues + state_reason=completed branch, the router asks GraphQL for the base branch of the PR that closed the issue:

--jq '... .nodes[0].baseRefName // ""' → "" when nothing closed it

That empty string then fell into the case statement's last arm:

case"$CLOSING_PR_BASE"in
main|master) status=Prod ;;
staging) status=FR on staging ;;
develop) status=FR on dev ;;
*) status=Prod ;; # ← empty base landed hereesac

So it was an explicit default, not a fallthrough accident and not a mis-detected closing PR. The same arm also swallowed a failed lookup — the gh call ends in || CLOSING_PR_BASE="", so a transient GraphQL error produced Prod too.

Evidence

backend#1171#1176 were closed by hand as completed on 2026-07-25 09:21 UTC. All six were moved to Prod, though their code had been merged only to develop and never promoted. They had to be corrected manually afterwards.

Confirmed against the live API and the run logs:

  • closedByPullRequestsReferences(first: 5){"nodes":[]} for each issue
  • run log: Routing decision: status=Prod→ #1171 → Status=Prod

Why this is more than untidy

fr-gate.yml is a required status check. It lets a promotion PR merge only when every contained item is in Ready for staging (→ staging) or Ready for prod (→ main/master). An item sitting falsely in Prodsatisfies that gate without anyone having reviewed it. A hand-closed issue could therefore let unreviewed work through the exact gate built to stop it.

The fix

Split the two cases that were collapsed into one arm:

CaseBeforeAfter
Issue completed, no closing PRProdno change
Issue completed, closing-PR lookup failedProdno change
Issue completed, closing PR → unrecognised baseProdFR on dev

With no closing PR the step emits an empty status, and the update step — already guarded by if: steps.target.outputs.status != '' — skips entirely. The card keeps whatever column it was in, which is the honest answer: ticking an issue off by hand tells you nothing about where its code is deployed. advance-deploy-env.yml still moves the card when the code actually ships, and kanban-reconcile.yml still catches drift.

No new column was invented.

One judgement call worth flagging

The third row above is a small behaviour change beyond the reported bug. An issue closed by a PR merged to an unrecognised base used to become Prod, while the pull_request branch maps that same base to FR on dev. The documented contract is "an issue mirrors its closing PR", so the two disagreeing was itself a defect — and Prod is the same invent-a-deploy-state failure, reachable through a different input. It now mirrors the PR route. Happy to revert this one hunk if you'd rather keep the change strictly to the reported case.

Deliberately not changed (flagging, not fixing)

closedByPullRequestsReferences defaults to includeClosedPrs: false, which still returns open PRs that say "Closes #N". So an issue closed by hand while an open promotion PR targeting main references it will mirror to Prod — same bug class, different input. Filtering the query to merged PRs only would close it. I left it alone because it changes the documented PR-mirroring path rather than the manual-close path, and this workflow is consumed by ~17 repos. Worth a follow-up.

What was deliberately left alone

  • All four PR-merge routes (main/masterProd, staging, develop, other base) — byte-identical.
  • PR closed unmerged → Cancelled.
  • Issue not_planned and issue with no state_reasonCancelled.
  • The Update Status on the kanban step — untouched.
  • The four pre-existing SC2016 findings in that second step: they belong to ci(actionlint): lint this repo's own workflows + clear the 29 pre-existing findings #66, which exists to clear them. Not duplicated here to keep the diffs disjoint.

The run: block was refactored to assign a STATUS variable and write $GITHUB_OUTPUT once at the end. Same values, but "no change" becomes expressible, and it removes the old grep status= "$GITHUB_OUTPUT" | tail -1 | cut -d= -f2 log line, which would have broken as soon as a branch wrote no status at all.

Verification

Route matrix, old script vs new, run under a stubbed gh — 13 of 16 identical, and the 3 that changed are precisely the bug class:

CASE | OLD | NEW |
------------------------------------------+---------------+---------------+
PR merged → main / master | Prod | Prod | same
PR merged → staging | FR on staging | FR on staging | same
PR merged → develop | FR on dev | FR on dev | same
PR merged → feature/x (odd base) | FR on dev | FR on dev | same
PR closed unmerged | Cancelled | Cancelled | same
issue completed, closing PR → main | Prod | Prod | same
issue completed, closing PR → master | Prod | Prod | same
issue completed, closing PR → staging | FR on staging | FR on staging | same
issue completed, closing PR → develop | FR on dev | FR on dev | same
issue completed, closing PR → feature/x | Prod | FR on dev | CHANGED
issue completed, NO closing PR (the bug) | Prod | (no change) | CHANGED
issue completed, gh lookup FAILS | Prod | (no change) | CHANGED
issue not_planned | Cancelled | Cancelled | same
issue closed, no state_reason | Cancelled | Cancelled | same
unexpected event (push) | (no change) | (no change) | same

actionlint (actionlint -no-color -oneline -shellcheck shellcheck, v1.7.12 + shellcheck 0.11.0, same invocation as #66's job) on this file:

Zero findings are attributable to this diff. The one SC2016 inside the step I edited is now suppressed at the line with a reason, using the same wording as #66 so the two resolve trivially. YAML parses; both script versions pass bash -n.

Notes for the reviewer

  • Merge conflict with ci(actionlint): lint this repo's own workflows + clear the 29 pre-existing findings #66 is expected in the first hunk — that PR adds a # shellcheck disable=SC2016 on the exact line this rewrites. Resolution is to keep this PR's block, which already carries the directive.
  • "No change" really is no change: the project's built-in Item closed workflow is disabled on project chore: add auto-add to engineer kanban workflow #2 (verified via the API — workflow #7, enabled: false), so nothing else claims the card on close. The in-code comment records this, because if it is ever re-enabled this branch needs revisiting. That also makes the stale warning in the existing env: comment worth a second look someday.
  • A comment block at the site spells out why there is no default, so the next reader doesn't helpfully restore one.

Also hardens ${{ github.event.issue.number }} out of the run: body into env:, matching the convention the rest of the file already follows.

🤖 Generated with Claude Code


Note

Medium Risk
Changes promotion-gate-related kanban routing across many repos; wrong behavior could block or mis-route releases, though the fix corrects a known false-Prod path.

Overview
Kanban closure routing no longer treats a manually completed issue (or a failed closing-PR GraphQL lookup) as shipped: Status stays unchanged because the router leaves status empty and the existing Update Status step skips when output is blank.

Previously, an empty CLOSING_PR_BASE hit the case default and forced Prod, which could satisfy fr-gate.yml without real functional review. The header comment and an in-script note document that deliberate “no default” behavior and the dependency on the disabled Item closed project workflow.

The Determine target Status script is refactored to set a single STATUS variable and write GITHUB_OUTPUT once; ISSUE_NUMBER moves into env for the gh call. Issues closed by a PR with an unrecognised base now map to FR on dev (matching the pull_request merge path) instead of Prod.

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

An issue closed by hand as completed was routed to Prod. The closing-PR
lookup returns an empty base when no PR closed the issue, and the case
statement's `*)` arm mapped that empty string to "Prod". This was an
explicit default, not an accident of fallthrough, and it also swallowed
the case where the GraphQL lookup simply failed.
Observed live: backend#1171-#1176 were ticked off by hand on 2026-07-25
and all six were moved to Prod, despite their code having been merged
only to develop. The run logs show "Routing decision: status=Prod" while
closedByPullRequestsReferences returned an empty list for each.
This matters beyond tidiness. fr-gate.yml is a required status check that
lets a promotion PR merge only when every contained item sits in Ready
for staging or Ready for prod. An item falsely parked in Prod satisfies
that gate with nobody having reviewed it, so this bug can let unreviewed
work through the very gate designed to stop it.
Fix: separate "no closing PR" from "closing PR with an unrecognised
base". With no closing PR the step now emits an empty status, and the
update step (already guarded on a non-empty status) skips entirely,
leaving the card in whatever column it was already in. Closing an issue
by hand tells us nothing about where the code is deployed, so the honest
answer is to change nothing. not_planned and a missing state_reason still
route to Cancelled, and every PR-merge route is unchanged.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

bugbot run

@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

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

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

Open PRs currently in Code review (oldest first):

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

@cursorcursorBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

✅ 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 cf86578. Configure here.

@LukasWodkaLukasWodka self-assigned this Jul 25, 2026
@LukasWodka
LukasWodka merged commit 49d38ff into developJul 26, 2026
4 checks passed
LukasWodka added a commit that referenced this pull request Jul 26, 2026
* fix(fr-gate): gate the diff, not just commit subjects
Item discovery greps commit SUBJECTS for (#N). Any commit whose subject has no
marker — a hand-written commit, a rebase-merged commit, or the commits folded
inside a merge — was invisible to the gate. design-system#70 shipped 12 such
commits to main under a single blessed (#67): the gate saw one item, passed,
and the other 12 were never evaluated.
Adds an attribution pass over every non-merge commit in BASE..HEAD. A commit is
attributable if its own subject carries a (#N), OR it is reachable from the 2nd
parent of a merge in range (i.e. it entered via a PR merge). Whatever is left is
code that reached the range without a reviewed PR — a direct push, or a release
branch edited after its last PR — and it now fails the gate closed.
Squash- and rebase-merges to develop each produce a single (#N)-bearing commit,
so the normal flow attributes cleanly; only genuinely unreviewed commits flag.
Verified against a git fixture: a subject-ref commit, two commits vouched for by
their PR merge, and one direct-push commit — only the direct push is flagged.
actionlint passes.
Refs RFC-BACKEND-0008 D27-L1 (tracebloc/backend#1265)
* fix(fr-gate): vouch a commit only if a merge introduced it (M^1..M^2)
Bugbot on #72: the merge-vouching path treated any ancestor of a merge's 2nd
parent as attributable. A direct push to the base branch becomes an ancestor
of ^2 once a feature branch merges the base in and is then PR-merged, so it was
silently vouched despite never being in a PR.
Tighten to: reachable from ^2 AND NOT from ^1 — i.e. among the commits the
merge actually added. Verified against a fixture reproducing exactly that
pattern: the old check flagged nothing, the new check flags the direct push,
and the normal feature-merge fixture still flags only its own direct push (no
false positives introduced).
LukasWodka added a commit that referenced this pull request Jul 26, 2026
Picks up #66 (actionlint gate + the 29 shellcheck cleanups), #67 (closure
router), #69 (CODEOWNERS), and #71 (fr-gate fail-closed). Without these the
code-quality run here was linting the pre-#66 workflow copies and failing on
findings already fixed on develop.
@LukasWodka
LukasWodka deleted the fix/kanban-manual-close-no-prod branch August 1, 2026 21:45
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@LukasWodka