Skip to content

fix(kanban): fail-closed closing-issue lookups + totalCount (Bugbot .github#166) - #181

Merged
LukasWodka merged 4 commits into
developfrom
fix/166-closing-issue-failclosed-totalcount
Aug 10, 2026
Merged

fix(kanban): fail-closed closing-issue lookups + totalCount (Bugbot .github#166)#181
LukasWodka merged 4 commits into
developfrom
fix/166-closing-issue-failclosed-totalcount

Conversation

@LukasWodka

@LukasWodkaLukasWodka commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Two Medium Bugbot findings on the closing-issue advancement (the #1600/#168 drift fix), both the same fail-open class Bugbot has now caught 3× in this automation:

  1. Silent skip on lookup failureCIS=$(gh api ... 2>/dev/null) || CIS='{}' made an expired token / rate limit / cross-repo permission error indistinguishable from 'this PR closes no issues': advancement was skipped, cards stayed behind, and the run reported success — the exact drift this block exists to eliminate. Same for the per-issue IRESP lookup. Now both fail closed (RUN_FAILED=1 + continue, visible red run), never silently.
  2. Silent truncationclosingIssuesReferences(first: 30) with no totalCount: a PR closing >30 issues advanced the first 30 and the rest looked like 'closed nothing'. Now first: 100 + totalCount with a loud warning + RUN_FAILED if a PR ever exceeds a page. projectItems bumped 10→20.

No fail-open path remains. Part of the same core-automation change — dry-run + review before merge (this ships on top of #168).


Note

Medium Risk
Changes core deploy kanban automation error handling; misclassified errors could still skip advancement or fail promotions, but behavior is stricter than the previous silent fail-open path.

Overview
Closing-issue advancement in advance-deploy-env no longer treats GraphQL lookup failures as “no issues to advance.” PR closing-issue and per-issue project queries fail closed (RUN_FAILED=1, red run) on real errors (token, rate limit, permissions) instead of swallowing stderr and substituting {}.

Benign non-PR numbers in the push are still skipped quietly by matching only GitHub’s Could not resolve to a PullRequest message—not broad “Not Found,” which would hide auth failures.

Pagination visibility:closingIssuesReferences goes from 30→100 with totalCount; runs warn and fail if a PR closes more than 100 issues. Per-issue projectItems goes 10→20 with totalCount and a warning when an issue is on more than 20 projects.

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

…github#166)
The closing-issue advancement swallowed gh api failures into {} (a token/rate-limit/
permission error read as 'closes no issues' and silently skipped advancement, run
green) and capped closingIssuesReferences at 30 with no totalCount (a PR closing >30
silently truncated). Now: fail-closed (RUN_FAILED=1 + continue) on any lookup failure,
first:100 + totalCount surfacing on truncation, projectItems first:20.
Comment thread.github/workflows/advance-deploy-env.yml
Comment thread.github/workflows/advance-deploy-env.yml
LukasWodka added a commit that referenced this pull request Aug 6, 2026
Bugbot on the duplicate PR (#181) caught a regression I introduced in the
previous commit, and it would have hurt: PR_NUMBERS is scraped from commit
subjects, so it legitimately carries numbers that are not PRs in this repo --
a hand-written (#47) issue reference, or a subject carried in from another
repo. The loop above already documents exactly that. Failing closed on those
turned ordinary pushes red.
Fail-closed is right for an outage and wrong for a reference that simply does
not resolve. The two are now split on the error text:
* "Could not resolve to a PullRequest" / "...to an Issue" -> notice, skip
* anything else (401, rate limit, permissions) -> warning + RUN_FAILED
2>/dev/null is also gone. It discarded the only evidence that could tell the
two apart, and left any red run undiagnosable -- barely better than a silent
one. The error text now reaches the log.
Verified with a stub: a normal read passes, #47 skips as a notice, a 401 sets
RUN_FAILED.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

Heads-up on a collision: this PR and #180 are independent fixes for the same two Bugbot findings on .github#166, opened ~15 minutes apart from different sessions. I opened #180; I did not open this one, and I am not closing either — that call is @LukasWodka's.

Recording the state so the choice is easy:

#180#181
fail-closed on unreadable readsyesyes
totalCount on closingIssuesReferencesyesyes
totalCount on projectItemsyes (kept first: 10)no — page bumped to first: 20 without it (Bugbot, Low)
unresolvable (#47) referenceskips as a noticesets RUN_FAILED — would redden ordinary pushes (Bugbot, Medium)
error text in the logyes2>/dev/null

Both Bugbot findings here are real, and the first one was in #180 too until a few minutes ago — I had the same regression and fixed it after seeing this review. PR_NUMBERS is scraped from commit subjects and legitimately carries non-PR numbers, which the loop's own comment documents, so failing closed on them turns normal pushes red.

#180 now splits the two cases on the error text — Could not resolve to a PullRequest / ...to an Issue is a notice and a skip; a 401, rate limit or permission error sets RUN_FAILED — and echoes the error rather than discarding it, since a red run nobody can diagnose is barely better than a silent one.

If #180 is the one to keep, both findings here are already addressed there. Either way the two must not both merge.

…lCount (Bugbot .github#181)
The v1 fail-closed reddened every promotion carrying a PR_NUMBERS entry that isn't
actually a PR (issue ref / bogus number). Capture output and skip QUIETLY on 'Could
not resolve to a PullRequest'/Not Found, fail CLOSED only on a real infra failure.
Add the projectItems totalCount check for parity with the closingIssuesReferences fix.
Comment thread.github/workflows/advance-deploy-env.yml
…gbot .github#181)
'Not Found' is also what a scoped-down/expired token returns, so skipping it as
benign re-opened the fail-open. Match only 'Could not resolve to a PullRequest';
any access/transport failure now falls through to RUN_FAILED.

@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 2 potential issues.

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 a0e9cb3. Configure here.

Comment thread.github/workflows/advance-deploy-env.yml
Comment thread.github/workflows/advance-deploy-env.yml
@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

Round-3 update on the #180 / #181 duplication, since both of these new findings are implementation-specific and neither applies to #180:

Bugbot finding on this PR#180
Highcis_out=$(...) bare assignment under set -e makes the if [ $? -ne 0 ] branch unreachablenot present: both reads use if ! CIS=$(...) / if ! IRESP=$(...) — the pattern Bugbot itself names as the one that survives errexit
Mediumprintf ... | grep -qi can return 141 via SIGPIPE under pipefail, misclassifying the benign errornot present: the error text is matched with case "$ERRTXT" in ... esac, no pipe, no grep

Running total across both: #181 has had 5 findings over 3 rounds, 2 currently open. #180 has had 3 over 2 rounds, 0 currently open. I am not claiming that as a virtue of mine — one of #180's three was a High I introduced myself, and I only found the fix because Bugbot reviewed this PR first.

The substantive difference is the approach to the cross-repo read. #181 keeps refining which error strings count as benign; #180 removed the benign case from that read entirely, on the rule that closingIssuesReferences entries were already resolved by GitHub — so a failure there is always missing access, never missing data. Text-matching has now reopened the fail-open hole twice (the Could not resolve to a Repository round and the Not Found round); the rule closes it structurally.

@LukasWodka — still your call, and I have not closed either. But every round they both stay open, the same findings get paid for twice.

…ub#181)
Bare cis_out=$(...) under set -euo pipefail aborts the step on a non-zero substitution
(High) -- restore the 'if ! cis_out=$(...)' form the per-issue lookup already uses.
And printf|grep -q can return 141 via SIGPIPE under pipefail even on a match (Medium)
-- use a here-string so grep's own exit status is what's tested.
@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

Handoff for review — do NOT fast-merge (moves every card on the board)

This PR (on top of #168) hardens the closing-issue advancement. It took 6 rounds of Bugbot findings, all now resolved — three of them regressions I introduced while fixing the previous one, all on the same fail-open/fail-closed + shell-safety boundary. Bugbot-green here means 'no known remaining hole', not 'validated correct.' It needs the two things below before merge:

1. Dry-run on real data (the only real validation)

advance-deploy-env.yml has a dry_run input (from #168). Exercise it on a controlled push and read the [DRY] would set … output. Confirm:

  • A PR closing a cross-repo issue (e.g. a client PR closing a backend issue) resolves and advances that issue.
  • Benign non-PR numbers (issue refs grepped from commits) are skipped quietly — no red run.
  • A simulated real failure (scoped token / rate limit) fails the run (RUN_FAILED), does NOT silently skip.
  • Only CLOSED closing-issues advance (open ones aren't yanked into deploy columns).

2. Careful human diff review

The fail-open/fail-closed logic is subtle (as the 6 rounds show). Read the CIS + IRESP blocks specifically: if ! cis_out=$(...) errexit-safety, the grep -qi "Could not resolve to a PullRequest" <<< "$cis_out" benign-match precision, RUN_FAILED propagation via the here-string loop, and the totalCount truncation guards.

Honest note

I kept mis-calibrating this specific logic — I'm not the right author to merge it unreviewed. Recommend consolidating #168 + #181 and dry-running them together once, with a human on the final diff.

@LukasWodka
LukasWodka merged commit bb63a02 into developAug 10, 2026
9 checks passed
@LukasWodka
LukasWodka deleted the fix/166-closing-issue-failclosed-totalcount branch August 10, 2026 08:15
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