Skip to content

fix(kanban): advance-deploy-env must fail closed on unreadable or truncated reads - #180

Closed
LukasWodka wants to merge 3 commits into
developfrom
fix/1603-advance-deploy-env-fail-closed
Closed

fix(kanban): advance-deploy-env must fail closed on unreadable or truncated reads#180
LukasWodka wants to merge 3 commits into
developfrom
fix/1603-advance-deploy-env-fail-closed

Conversation

@LukasWodka

@LukasWodkaLukasWodka commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Summary

Two Bugbot findings on the develop -> staging promotion (.github#166). Both are the same class, and both fail open — which is the one direction this workflow must never fail, since a silent no-op looks identical to a green run.

1. A failed read was indistinguishable from "closes nothing." Both lookups ended in || CIS='{}' / || IRESP='{}'. An expired token, a rate limit, or a cross-repo permission error produced an empty result, the loop advanced nothing, and the run reported success. That is precisely the board drift this advancement was added to eliminate — returning invisibly.

2. Truncation was silent.closingIssuesReferences(first: 30) and projectItems(first: 10) requested no totalCount, so anything past the limit simply looked like it did not exist.

Changes

Both reads now follow the workflow's existing fail-closed convention (RUN_FAILED=1, warning, non-zero exit at the end):

  • unreadable closing-issue list → warn + RUN_FAILED, never "closes nothing"
  • unreadable issue → warn + RUN_FAILED, never "not on project"
  • totalCount requested on both connections; exceeding the page size warns and sets RUN_FAILED rather than skipping the remainder quietly

Test plan

  • actionlint clean
  • YAML parses
  • No behaviour change on the healthy path: same queries, same advancement logic, extra field only

🤖 Generated with Claude Code


Note

Medium Risk
Changes CI kanban automation behavior on error paths across all repos that call this workflow; misclassification could redden runs or still miss cards, but the happy path is unchanged.

Overview
The closing-issue advancement path in advance-deploy-env no longer treats API failures or partial pages as “nothing to do.” GraphQL stderr is captured in a temp file so errors can be classified instead of being discarded with 2>/dev/null and || '{}' fallbacks.

For closing-issues lookups on scraped PR numbers, only “not a pull request” is skipped quietly; token, rate-limit, and permission failures emit warnings, set RUN_FAILED, and the step exits non-zero at the end. totalCount on closingIssuesReferences triggers a warning and RUN_FAILED when more than 30 closing issues exist.

Per-issue reads from GitHub-resolved triples always fail closed on any GraphQL error (including cross-repo access gaps), with no benign skip for “could not resolve” text. projectItems.totalCount over 10 is similarly flagged so a kanban item might be missed without a silent green run.

Healthy paths are unchanged aside from the extra totalCount fields in the queries.

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

…ncated reads
Bugbot (#166), two findings of the same class -- both fail OPEN, which is the
one direction this workflow must never fail:
1. Both closing-issue lookups ended in `|| CIS='{}'` / `|| IRESP='{}'`, so an
expired token, a rate limit or a cross-repo permission error was
indistinguishable from "this PR closes no issues". RUN_FAILED was never
set, the cards stayed behind, and the run reported success -- the drift
this advancement exists to eliminate, returning invisibly.
2. Neither connection requested totalCount, so a PR closing more than 30
issues (or an issue on more than 10 projects) silently advanced the first
page and the remainder looked like it did not exist.
Both now use the workflow's existing fail-closed convention: warn, set
RUN_FAILED, and let the end-of-run check exit non-zero.
actionlint clean; healthy path unchanged.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@LukasWodkaLukasWodka self-assigned this 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

Cross-reference: #181 fixes the same two .github#166 findings independently — opened ~15 minutes after this one, from a different session. Details and a side-by-side are in my comment there. The two must not both merge, and the choice is @LukasWodka's — I have not closed either.

Bugbot's review of #181 also caught a regression that was in this PR, and I have since fixed it here: PR_NUMBERS is scraped from commit subjects and legitimately carries numbers that are not PRs in this repo (the loop's own comment says so). My first version failed closed on those, which would have reddened ordinary pushes. 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, and the error is logged instead of discarded.

@cursorcursorBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit e05ac93. Configure here.

Comment thread.github/workflows/advance-deploy-env.yml Outdated
…SS, not missing data
Bugbot (#180, High) caught the opposite over-correction to the one I made
last commit, and it is right.
I had added "Could not resolve to a Repository" to the benign skip list.
GitHub returns that same message for a private repo the token cannot see --
missing scope, expired SSO, cross-repo auth. That is exactly the case this
block exists for: a client/website PR closing a `backend` issue. So the skip
restored the fail-open hole on the one route it was added to close.
The benign case is gone from the issue read entirely, deliberately. These
owner/repo/number triples come from closingIssuesReferences, which GitHub
itself already resolved, so the issue and its repo exist by construction --
a failure here cannot be missing data.
The scraped-PR read above keeps its benign case for the opposite reason:
those numbers come from commit subjects, nothing resolved them, and the repo
is the one running the workflow.
actionlint clean.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

Closing in favour of #181, per @LukasWodka's decision.

Both PRs independently fix the same two Bugbot findings on .github#166 (fail-closed closing-issue lookups + totalCount in advance-deploy-env.yml); only one may merge. #181 is the chosen implementation:

Closing to keep the review queue unambiguous for @saadqbal. Reopen if ever needed.

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