fix(approve): report a jq abort instead of collapsing it into a decline (BLO-32267)

[allyblockcast]

Thinking Path

• Paperclip is the open source app people use to manage AI agents for work
• Its release path gates paperclip-api on a ValidatingAdmissionPolicy that only admits digests listed in a bounded approval ring, rotated by scripts/approve-paperclip-api-digest.sh
• serving_replicaset_image() is the reader that recovers the last-healthy digest from the serving ReplicaSet when the Deployment's spec.template names one that never carried traffic (fix(approve): recover the last-healthy digest from the serving ReplicaSet (BLO-31842) #1655, fix(approve): recover the older digest when two ReplicaSets are serving (BLO-32101) #1676) — it is the object a rollback depends on
• That reader ran its election program as image="$(jq … 2>/dev/null)" || image="", which maps a deliberate decline and a jq program abort onto the same empty string and discards the one line that says which
• Downstream both read as "no rollback target", so a future edit that breaks the jq surfaces as a silently missing recovery digest rather than a diagnosable error
• This is not hypothetical: BLO-32101's first malformed-revision case passed with and without the guard it was written to prove, precisely because an abort and a clean decline are indistinguishable from outside — that test had to be reshaped around the conflation
• This pull request routes jq's stderr through a capture and warns once, in the same idiom the kubectl get replicasets branch two above it already uses
• The benefit is that a broken election program is reported as an error instead of quietly costing a rollback its target

Linked Issues or Issue Description

• Refs BLO-32267 — the tracking issue for this change

• Follow-up to fix(approve): recover the older digest when two ReplicaSets are serving (BLO-32101) #1676 (BLO-32101), raised there by Ally as a non-blocking Suggestion at head 96766a79. Filed separately rather than folded in because the conflation is pre-existing and this is a diagnosability improvement, not a correctness defect in what shipped — and fix(approve): recover the older digest when two ReplicaSets are serving (BLO-32101) #1676 was already reviewed clean at that head

• Builds on fix(approve): recover the last-healthy digest from the serving ReplicaSet (BLO-31842) #1655 (BLO-31842) and fix(release): pin the running digest in the approval window (BLO-28483) #1639 (BLO-28483), which introduced this reader and the pinned running-digest slot it feeds

• I searched the GitHub PR list (open and recently closed) for duplicate or related PRs and linked them above

Searched approve-paperclip-api-digest, serving_replicaset_image, jq abort, and 2>/dev/null: no duplicate. #1682 (BLO-32213), #1686 (BLO-32210) and #1689 (BLO-32352) are open against the same file but cover the write harnesses, citation-range pinning, and a raced approve() rejection respectively — different functions, no overlap with this change.

What Changed

• serving_replicaset_image() captures jq's stderr into a $jq_err file instead of sending it to /dev/null, and on a non-zero jq exit warns once per script run — the reader is re-entered on every 409 retry, so warn-once is what keeps a persistent abort from becoming per-rotation noise.
• The warning carries jq's own message, and the function still degrades to empty rather than failing the release. This stays an availability safeguard, not a gate.
• Why the split is safe: every decline this function makes is expressed by the program succeeding and emitting "" (no serving ReplicaSet, a missing or non-integer revision, tied revisions, containers that disagree, another repository). So a non-zero jq exit can only mean the program itself is broken.
• scripts/approve-paperclip-api-digest.test.js gains two cases — a jq abort is reported rather than returned as a silent empty, and a repeated abort warns once rather than once per rotation.
• One test-only extra: the rotation case's assertion message now names which side of 1 it landed on. 0 (warned not at all — the pre-fix behaviour) and 3 (warned per rotation) are opposite defects, and a message reading "more than once" for both repeated in miniature the exact conflation this change removes from the script.

No change to which digest is elected. Ownership → readiness → deployment.kubernetes.io/revision ordering, the decline-on-missing and tie guards, and no-fallthrough-to-the-runner-up are untouched, as is the RBAC degradation path. MAX_APPROVED_DIGESTS=3 is unchanged, so the maxApprovedApiDigests CEL variable in paperclip/paperclip-public-tools.yaml needs no change.

Verification

Full suite green at head 03d576ee, rebased onto master 75c01c162:

$ node --test scripts/approve-paperclip-api-digest.test.js
ℹ tests 95
ℹ pass 95
ℹ fail 0
ℹ duration_ms 10328

93 → 95 tests. The policy job — where this file is gated (.github/workflows/pr.yml) — is success at this head.

Negative control — run and pasted, not assumed

The standing bar on this line of work (per #1639 / #1655 / #1676). With the shipping .sh reverted to master's version — routing back to 2>/dev/null, capture removed — 93/95:

case	result
a jq abort is reported as an error, not returned as a silent empty	FAIL — The input did not match /jq program failed/. Input: ''
a repeated jq abort warns once, not once per rotation	FAIL — expected exactly one jq-abort warning across 5 rotations, got 0
missing revision · tied revisions · ownership · repository · containers-disagree	green
a lone serving ReplicaSet is pinned even when its revision annotation is malformed	green
a failed ReplicaSet list warns, naming the grant and carrying kubectl's reason (RBAC degradation)	green

A case that passes both with and without the change is not evidence — that is the exact failure this PR is about, so the control is the point rather than a formality. The new case neuters the guard in the shipping source rather than a restated copy, and asserts the anchor still matched, so a rewrite of that guard cannot silently turn the mutation into a no-op that tests nothing.

Risks

Low. The change is confined to one function's error plumbing and adds no new failure mode: jq's exit status was already consulted (|| image=""), so the only new behaviour is emitting a warning line that was previously discarded.

• Election is unchanged — no ordering, guard, or fallthrough was touched, and the decline cases stay silent, so the ordinary "no rollback target" path gains no noise.
• Cannot fail a release — a jq abort still degrades to empty exactly as before; the warning is diagnostic output, not a gate.
• RBAC degradation untouched — a failed kubectl get replicasets still warns once and degrades to empty.
• The one behavioural change an operator will notice is a new stderr line when the election program is genuinely broken, which is the intent.

Model Used

Claude Opus 5 (claude-opus-5, 1M context window) via Claude Code, with extended thinking and tool use.

Checklist

• I have included a thinking path that traces from project context to this change
• I have specified the model used (with version and capability details)

🤖 Generated with Claude Code

[allyblockcast]

🔗 Paperclip issue: BLO-32267
🔗 Paperclip issue: BLO-32101

[allyblockcast]

Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention:

Missing or incomplete:

• Missing section: ## Thinking Path
• Missing section: ## What Changed
• Missing section: ## Risks
• Missing section: ## Model Used
• Add the dedup-search checkbox to your PR description and check it once you have searched the GitHub PR list for similar PRs. See the PR template at .github/PULL_REQUEST_TEMPLATE.md and CONTRIBUTING.md → "Before You Start: Search First".

Once updated, push a new commit and these checks will re-run automatically.

— commitperclip

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 03d576e

Clean on correctness. The split between "the program declined" and "the program could not run" is sound, and the reasoning that makes it safe — every decline is expressed by jq succeeding and emitting "", so a non-zero exit can only be a broken program — holds against the actual jq at scripts/approve-paperclip-api-digest.sh:1035-1069.

I did not take the PR's verification on trust. Checked out both files at this head and ran the suite: 95/95. Then reproduced the stated negative control by replacing only the shipping .sh with master's: 93/95, failing exactly a jq abort is reported as an error, not returned as a silent empty and a repeated jq abort warns once, not once per rotation. The claim in the description is accurate.

Critical Issues (0)

None.

Important Issues (0)

None.

Suggestions (2)

• [pr-review-toolkit/tests] scripts/approve-paperclip-api-digest.test.js:1330 — the jq stderr capture leaves no temp file behind cannot fail for the reason its comment gives. Measured: deleting rm -f "$jq_err" (approve-paperclip-api-digest.sh:1101) leaves all three jq cases green. jq_err lives inside state_dir, and state_dir is rm -rf'd either by the reader itself when it minted one, or by the harness's rm -rf "$rs_state_dir" when the caller owns it — so the file can never survive into readdirSync(tmpRoot) whether or not it is reclaimed.

  • In fairness this is a duplicated blind spot, not a new one: the pre-existing sibling at :1413 has the identical property — deleting rm -f "$rs_err" also stays green. And the shipping cleanup is correct; only the assertion about it is inert.
  • Worth either asserting against state_dir's own contents on the caller-owned path (where the harness mints it and can list it before teardown), or dropping the case. In a change whose whole thesis is that two different outcomes must not present identically, a test that passes either way is the one place that reads louder than it measures.

• [gstack/review] scripts/approve-paperclip-api-digest.sh:983-984 — the selector and uid reads still run 2>/dev/null and || return 0, which is the same conflation this PR removes, one layer up and strictly quieter: that path returns before the warned-selector branch, so an abort there produces no warning at all. Near-unreachable in practice — neither program can abort on the valid JSON kubectl get deployment -o json produces — so this is genuine follow-up material rather than something to fold in here, in the same spirit this PR was split out of #1676.

Strengths

• image="" inside the abort branch (:1088) is load-bearing, not belt-and-braces: on abort jq may have already written partial output into the command substitution, so the assignment is what stops a truncated value reaching the caller as a digest. Easy to drop as redundant later — the comment above it earns its place.
• && jq_status=0 || jq_status=$? (:1069) is set -e-safe under this file's set -euo pipefail and mirrors the rs_status idiom at :1017 exactly, so there is one capture pattern in this function rather than two.
• The three degradation paths keep distinct markers — warned-selector, warned-list, warned-jq — so none can mask another across the rotate loop's re-entries. The warn-once semantics are then pinned by their own case rather than assumed.
• Neutering the guard in the shipping source rather than a restated copy, plus the notEqual assertion that the anchor still matched, is the right shape: a rewrite of the guard fails the test loudly instead of turning it into a no-op.
• The reader's core contract is preserved — it degrades, it never fails a release. The harness asserts exit 0 on every case including both abort paths.

Recommended Action

1. No Critical or Important issues; nothing blocks merge on the diff.
2. Consider the two Suggestions opportunistically — the first is the one with real value, since it removes a false coverage signal from a file that otherwise holds a high bar for discriminating tests.
3. Unrelated to this diff, worth a glance before merge: the review check-run reports failure at this head, from its Fail if quality gates failed rollup step, while General tests, Typecheck + Release Registry, e2e and Canary Dry Run were still queued/in_progress at review time. I did not diagnose it further; re-read it once the suite settles rather than treating it as a verdict on this change.