fix: the clean-board assert compares against the server's own count

[LukasWodka]

Bugbot High on .github#341, and correct. The post-archive assertion added in #339 treats a fully paginated nodes list as the whole board, and never asks the server how many items the connection actually holds.

Two ways it passes while the board is dirty

• nodes|length renders a missing or null path as 0. A query that resolved to nothing at all counts as "no terminal items left" and the job reports clean.
• Items omitted from nodes while totalCount still counts them are invisible to every check in the step. Both the first read and the re-read are the same credential's, so they agree on the same subset — or on zero — and agree confidently.

The second is the exact defect #339 was written to close, one layer further in.

I wrote the sentence that describes this, then built on it anyway

The file already says:

"Both reads are this credential's, so this cannot see what neither can."

…and the assertion still rested on those two reads. Comparing a read against another read by the same blind credential is the test-a-list-against-itself trap, and #339's own commit message claimed to have avoided it by using a second read. A second read with the same credential is not an independent one.

Why totalCount is the fix

It is the only number in this job that does not come from this credential's view of nodes. Three guards, all failing closed, matching the rest of the step:

1. the connection must resolve to an array — an unreadable read fails rather than counting as zero;
2. totalCount must be present and numeric, or completeness is unestablished;
3. paginated count must equaltotalCount, or items are being omitted and the run cannot claim a clean board.

Type

fix

Test plan

• actionlint clean; yaml.safe_load parses.
• Behaviour is only observable against the live board, which is the point. On the next scheduled run one of two things happens, and both are informative:
  • counts agree → the read is complete, and the remaining terminal count is real;
  • counts disagree → this credential is being served a partial nodes list, which is the mechanism behind the 668 un-archived terminal cards and has never been provable until now.

Checklist

• Targets develop
• One self-contained change (one workflow)
• Assignee set, one reviewer requested
• No secrets, tokens or customer data

---

Note

Low Risk
CI-only workflow logic with stricter verification; no application runtime, auth, or data-path changes.

Overview
Hardens the kanban archive workflow’s “Assert the board is clean” step so a green run cannot mean “we saw zero terminals” when the GraphQL read was incomplete or empty.

ProjectV2 items queries now request totalCount on the initial terminal collect and on the post-archive re-read. The assert loop adds fail-closed checks: the re-read must return a real nodes array (so a missing path cannot count as zero items), totalCount must be present and numeric, and the sum of paginated nodes must equal that totalCount. If pagination yields fewer rows than the server reports, the job errors instead of treating a partial credential view as a clean board—addressing the same-credential “test a list against itself” blind spot called out for backend#2528.

^{Reviewed by Cursor Bugbot for commit c143e4b. Bugbot is set up for automated code reviews on this repo. Configure here.}

[shujaatTracebloc]

The diagnosis is right and the fix is the right shape — totalCount genuinely is the only number in this job that doesn't come from the same credential's view of nodes, and "comparing a read against another read by the same blind credential" is exactly the trap #339 fell into. But there's a defect in the implementation that stops the new check from ever running.

[ -z "$declared_total" ] && declared_total="$page_tc" aborts the step on page 2

Line 285, inside the while :; do loop, under the set -euo pipefail at line 216.

On the first iteration declared_total is empty, the test succeeds, the assignment runs, and the AND-list exits 0. On the second iteration declared_total is now set, so [ -z … ] returns 1, the right-hand side doesn't run, and the whole AND-list's exit status is 1. Under set -e that terminates the step immediately — no ::error::, no message, just a failed job with nothing to read.

This is the standard set -e gotcha with A && B as a bare statement: set -e ignores non-zero status inside an && list only for the non-final commands, and exempts the list entirely only in a condition context (if, while, !, or another &&/||). A standalone list that ends non-zero is not exempt.

It fires on every real run. Page size is 100 (items(first: 100)), and this PR's own body cites 668 un-archived terminal cards — so pagination is certain, and page 2 always happens.

The consequence is the part that matters: the step dies partway through the re-read, so reread_total -ne declared_total at line 332 — the entire new capability this PR adds — never executes. The job would go from "passes while the board is dirty" to "dies with no diagnosis," which is fail-closed in the narrow sense but surfaces nothing, and specifically not the credential-blindness you're trying to prove.

Either of these is exit-status-safe:

if [ -z"$declared_total" ];then declared_total="$page_tc";fi

:"${declared_total:=$page_tc}"

Worth noting it's the only [ … ] && assignment statement in the file — I grepped. So this isn't a house idiom that works elsewhere; it arrived with this change.

The rest holds up

The three guards are well-ordered and each fails closed for a distinct reason:

• array check before counting — this is the real fix for nodes|length rendering a missing path as 0, and it has to come first, because every count below it is meaningless otherwise.
• totalCount present and numeric — the case with *[!0-9]* catches null, empty, and anything non-numeric in one shape, which is more robust than a -eq comparison that would itself error on garbage.
• paginated count equals totalCount — the actual new signal.

The self-critical framing in the body is accurate and worth keeping: the file already said "Both reads are this credential's, so this cannot see what neither can" and the assertion was built on those two reads anyway. That sentence being present and disregarded is more useful to the next reader than a clean write-up would have been.

One design question, not a blocker

declared_total is pinned to the first page's totalCount. On a board being mutated concurrently, a later page could legitimately report a different total, and the comparison would then fail for a reason that isn't credential blindness. That's still fail-closed and arguably correct for this job — but the error message would name the wrong cause, and on a board this size a concurrent change during a multi-page read isn't far-fetched. Worth a sentence in the message acknowledging the two possible causes, or a deliberate note that a mid-read mutation is itself disqualifying.

Happy to re-review as soon as the && is unwound — everything else here I'd approve.

[saadqbal]

Correct, and I approved the assertion this fixes, so let me be specific about what I got wrong rather than just stamping the repair.

On #339 I checked the shrink guard and satisfied myself it couldn't false-positive: both first_total and reread_total count every item regardless of isArchived, so a successful archive leaves them equal. That was true, and it was the wrong question. The right one was whether those two numbers were independent — and they aren't, both being the same credential's paginated nodes. My approval even said "only a genuine loss of visibility trips it", which names the hole precisely without noticing it: a credential blind in both reads shows no change between them, trips nothing, and reports clean. The bug #339 existed to fix was a visibility bug — 63 of 668 — and I blessed a fix whose assertion rested on that same visibility.

totalCount is the right answer for the reason you give: it is the only number in the job that doesn't come from this credential's view of nodes. And the three guards all fail closed, matching the rest of the step — a non-array nodes is an error rather than a zero, a missing totalCount means completeness can't be established rather than assumed, and reread_total != declared_total says items are being omitted rather than that none exist.

The nodes|length → 0 path is the sharper of the two failure modes, because it needs no partial visibility at all: a query that resolves to nothing whatsoever reads as "no terminal items left". That's the same fail-open shape as an empty scan passing a subset check, which this org has now hit in at least four places.

"I wrote the sentence that describes this, then built on it anyway" — the file already said both reads are this credential's, so this cannot see what neither can. Writing the constraint down and then not applying it is worth recording as its own lesson; it's the failure mode that survives review precisely because the reviewer reads the sentence as evidence the author handled it. I did exactly that.

Green, no threads. Approving.

[shujaatTracebloc]

Retracting my change request — the finding was wrong, and the approval over it was right.

I claimed [ -z "$declared_total" ] && declared_total="$page_tc" (line 285) would abort the step on page 2 under the set -euo pipefail at line 216, killing the run before the new reread_total -ne declared_total check could execute. That does not happen. I tested it instead of continuing to reason about it:

set -e
x=1
[ -z"$x" ] && x=2
echo REACHED # → prints REACHED, exit 0

set -e explicitly exempts commands in an &&/|| list except the one following the final operator. The test [ -z "$x" ] is exempt, and when it fails the assignment never runs, so no non-exempt command failed and the shell does not exit. Reproduced against the actual loop shape — three iterations, declared_total set once on the first, loop completes, exit 0.

What I confused it with. The idiom genuinely does bite, but only in two shapes, neither of which is this one:

• as the last line of a script — the failing list becomes the script's exit status (exit=1);
• as the last line of a function whose status is then evaluated under set -e (exit=1).

Line 285 is mid-loop with a dozen statements after it, so it's the safe case. I generalised a real gotcha to a position where it doesn't apply, and I should have run the three-line check before filing rather than after being overruled.

(Verified on bash 3.2; the exemption is POSIX-specified and unchanged in bash 5, which is what the runners use.)

While I was in there I also checked the loop tail — [ "$HAS_NEXT" = "true" ] || break — for the mirror-image problem. It's safe in both directions: when the test passes the list returns 0, and when it fails break runs.

So the substance stands as merged, and it's good. The three guards are well-ordered and each fails closed for a distinct reason: the array check has to come first because every count below it is meaningless otherwise; the case with *[!0-9]* catches null, empty and non-numeric in one shape rather than letting a -eq error on garbage; and totalCount really is the only number in the job that doesn't come from the same credential's view of nodes.

The one thing from my review I'd still leave with you, unchanged and non-blocking: declared_total pins to the first page's totalCount, so a board mutated mid-read can fail the comparison for a reason that isn't credential blindness, and the error message would name the wrong cause. On a 668-item board that's seven pages of wall-clock. Worth a sentence in the message allowing for both causes — or a deliberate note that a mid-read mutation is itself disqualifying, which is a defensible position and arguably the one you're already taking.

@saadqbal — you were right to approve over me.