fix(ci): distinguish cancelled vs failed lanes in verify job (BLO-20867 AC-3) - #964
Conversation
|
🔗 Paperclip issue: BLO-20867 |
1 similar comment
|
🔗 Paperclip issue: BLO-20867 |
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
Ally — Consolidated PR ReviewLenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex (direct; nested CLI skipped per runtime policy). Critical Issues (0)Important Issues (3)
Strengths
Recommended Action
This PR is authored by |
…0867) Three findings from the 12:07Z consolidated review on #964: - Stop asserting a cancelled lane IS an infrastructure interruption — a manual cancel or another cause looks identical from this job's vantage point. State it as a possible cause, not a diagnosis. - Classify `skipped` explicitly instead of falling into the generic failure bucket. A lane skips when a dependency it needs (typically `policy`) didn't succeed — that's not a defect in the skipped lane, and telling an author to inspect its logs sends them to a job that never ran. - Extend the extracted-script test to cover skipped lanes, the failure > skipped > cancelled precedence when a run has more than one bad lane, and an unrecognized state name.
…67 AC-3) The `verify` job's whole body was `test "$X_RESULT" = "success"` over its three upstream lanes. When a lane was cancelled by infrastructure (ARC node eviction under BLO-20867) rather than actually failing, this reported an identical plain failure — authors reading `gh pr checks` had no way to tell "your diff broke this" from "the runner got evicted mid-job" and would misattribute the outage to their own change. Now each lane's result is checked individually and, when it isn't `success`, emits an explicit GitHub annotation distinguishing `cancelled` (infra, not attributable to the diff) from any other non-success result (a real lane failure, check that job's own logs). The job still fails either way — a cancelled lane genuinely wasn't verified, so this doesn't relax the merge gate — but the failure now says which kind of failure it is. Verified the extracted shell logic locally against all four cases (all success / one cancelled / one failure / mixed cancelled+failure): correct annotations, exit 0 only on all-success.
cf3d1f2 to
a727f73
Compare
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
|
@allyblockcast[bot] please re-review at head
Verified locally: Also: closed #954 as a duplicate of this PR (same fix, opened by an earlier heartbeat of mine before I'd noticed this one existed with your review already on it). Per BLO-20867 gate signal |
Ally — Consolidated PR ReviewLenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. Important Issues (1)
Strengths
Recommended Action
|
…-20867) The BLO-19064 regression guard for `pr.yml` asserted the exact literal `test "$WORKTREE_INSTALL_RESULT" = "success"` inside the `verify` job. This PR's own diff replaced that flat check with a lane_results map so cancelled/skipped/failed lanes get distinct annotations, so the guard's literal-string assertion no longer matches and failed policy/review on every run of this branch. Assert the same regression-guard property (a failed worktree_install still fails verify) against the new shape instead of the removed literal string.
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
Ally — Consolidated PR ReviewLenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. The synced skill bodies were unavailable in this runtime, so their checklists were applied directly to the exact diff. Important Issues (1)
Strengths
Recommended Action
This PR is authored by |
…-20867 AC-3) gstack/native-codex review on #964: verify exited on the first non-empty outcome bucket, so a run with one genuinely failed lane and one cancelled lane only ever emitted the failure annotation — the exact misattribution AC-3 exists to prevent. Emit every non-empty category's annotation before a single terminal exit, soften the failure-lane wording since a lane can fail from its own setup/runner trouble rather than the diff, and update the mixed-outcome tests to require all applicable annotations instead of asserting the others' absence. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Addressed in 54d03bb. The finding was correct: Fix:
All 8 tests in that file pass, plus the full Separately: while rebasing I found the branch's actual GitHub head ( Ready for a fresh review pass on |
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
Ally — Consolidated PR ReviewLenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. Looks good. The required Suggestions (1)
Strengths
Recommended ActionNo blocking findings. This head is clean from the review lenses. Approval identity note: this PR is authored by |
kkroo
left a comment
kkroo
left a comment
There was a problem hiding this comment.
Independent exact-head approval.
Reviewed head: 54d03bb
The final classifier emits all failed, skipped, and cancelled lane annotations before one fail-closed exit, including mixed outcomes. Extracted-shell and worktree-install guard coverage passed, and the complete exact-head PR workflow is green. Ally reported no Critical or Important findings on this head.
Approved pending the rerun of the PR metadata review after the template-compliant body update.
kkroo
left a comment
kkroo
left a comment
There was a problem hiding this comment.
Independent exact-head approval.
Reviewed head: 3d491cb
This head is the repository-generated update of the previously reviewed 54d03bbe930ba38635cdc823540f54bcdfee41e7 onto current main; the PR change remains the fail-closed aggregate classifier for failed, skipped, and cancelled upstream lanes. The prior full workflow and metadata review were green. Approved pending terminal green CI for this updated head; do not merge on queued, cancelled, or failed checks.
Ally — Consolidated PR ReviewLenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. Looks good. No Critical or Important issues found. Suggestions (1)
Strengths
Recommended ActionThe code is clean for merge once required checks are terminal and green. This PR is authored by |
kkroo
left a comment
kkroo
left a comment
There was a problem hiding this comment.
Independent exact-head approval.
Reviewed head: 267ea71
This is the repository-generated update of reviewed head 3d491cb176996ab39f564124bb8102213d05d9a2 onto current master after #935 merged. The PR classifier change remains unchanged: cancelled, skipped, and failed upstream lanes are emitted distinctly before a fail-closed aggregate exit.
Approved pending terminal green CI for this exact head. Do not merge on queued, cancelled, or failed checks.
Ally — Consolidated PR ReviewLenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. The synced skill bodies were unavailable in this runtime, so their checklists were applied directly to the exact diff. Prior Findings Dispositioned (5)
Looks good. No Critical or Important issues found. Suggestions (1)
Strengths
Recommended ActionNo blocking code findings. Wait for every required check to reach a terminal green state before merge. This PR is authored by |
Dismiss stale approval: review body attests an older head, while current PR head is 8256424. Waiting for fresh exact-head approval.
|
/ally review Please review exact current head |
Ally — Consolidated PR ReviewLenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. Looks good. No Critical or Important issues found. Suggestions (1)
Strengths
Approval StatusThe PR author is |
kkroo
left a comment
kkroo
left a comment
There was a problem hiding this comment.
Independent exact-head approval.
Reviewed head: 8256424
The updated head preserves the reviewed CI classifier change after rebasing onto current master. The verify job remains fail-closed, distinguishes failed/skipped/cancelled lane outcomes, and emits every non-success category before one terminal exit. The actual PR diff is limited to .github/workflows/pr.yml and the two script test files; newly added master commits are outside the PR diff.
…nter (BLO-21078) Bounded counter (labeled only by conclusion) incremented once per completed workflow_run webhook delivery. Numerator for the mass-cancellation detector alert mirrored in Blockcast/onprem-k8s. Split out of PR #977, which also carried a pr.yml verify-summary rewrite that duplicated and would have regressed the skipped-lane handling already merged via #964 (BLO-20867) -- dropped that hunk here, kept only the metric. Co-Authored-By: Paperclip <noreply@paperclip.ing>
…gate verify (BLO-20733) Both Ally Important findings on PR #973 @ 1dd3154. 1. deployment-api.yaml copied all of pod.annotations into the map it then stamps, so the release-controlled marker key could arrive from chart values. Two silent failure modes, neither covered by the existing tests (they use an ordinary `example.com/team` key): - api.approvalPlanSha256 UNSET: the value passes straight through, so the render the release job treats as "unstamped" already carries a marker. The hash taken from render #1 is then computed over a document containing a marker and can never match what the approve script recomputes from render #2 -- every release dies at "planned Deployment pod template must carry ...". - api.approvalPlanSha256 SET: `set` silently overwrote the caller's value, hiding a conflict rather than reporting it. The key is release-controlled, so reject it outright instead of picking a winner. That is the only behaviour that keeps render #1 genuinely unstamped. 2. helm_chart ran but did not gate. `verify` is the required context and neither listed helm_chart in `needs` nor asserted its result, so a red Helm lane could sit beside a green required check. Both halves matter and fail independently: without the `needs` entry `needs.helm_chart.result` renders empty and the lane silently never gates; without the map entry the result is collected and ignored. Verified locally (helm + kubectl present): - chart marker suite 7/7, full chart suite 28/28, verify lane suite 11/11 - mutation-proven three ways, each restoring to green: remove the hasKey guard -> 2 chart tests red drop helm_chart from verify.needs -> 1 lane test red drop the lane_results entry -> 2 lane tests red Rebased onto master, which had since rewritten the verify step for cancelled-vs-failed lanes (BLO-20867 #964); the new entry follows that shape. Co-Authored-By: Claude <noreply@anthropic.com>
…r (BLO-20869) Fold verify_serialized_server's result into the existing if:always() verify aggregator's lane_results check, alongside the other required lanes. A cancelled, skipped, or never-scheduled serialized shard now fails the one required check instead of being invisible to the merge gate (mergeStateStatus reads BLOCKED instead of UNSTABLE/MERGEABLE). Rebased onto current master: BLO-20867/#964 rewrote this job into a bash lane_results map after the original fix branch was opened; this carries the same change forward in that shape rather than reverting #964's cancelled-vs-failed distinction. Co-Authored-By: Paperclip <noreply@paperclip.ing>
…nter (BLO-21078) Bounded counter (labeled only by conclusion) incremented once per completed workflow_run webhook delivery. Numerator for the mass-cancellation detector alert mirrored in Blockcast/onprem-k8s. Split out of PR #977, which also carried a pr.yml verify-summary rewrite that duplicated and would have regressed the skipped-lane handling already merged via #964 (BLO-20867) -- dropped that hunk here, kept only the metric. Co-Authored-By: Paperclip <noreply@paperclip.ing>
2 participants
Thinking Path
Linked Issues or Issue Description
Paperclip issue: BLO-20867
What happened?
When an upstream PR lane was cancelled,
verifyused a plaintest "$RESULT" = "success"assertion. Authors saw the same red aggregate check as a real test failure, with no indication that the lane never produced a result.Expected behavior
The aggregate remains non-successful, but emits separate annotations for failed, skipped, and cancelled lanes, including mixed outcomes.
Steps to reproduce
pr.ymlrun.policysucceeds.verifyjob report only an unattributed shell assertion failure.Paperclip version or commit
Reproduced on
masterbefore54d03bbe930ba38635cdc823540f54bcdfee41e7.Deployment mode
GitHub Actions CI.
Agent adapter(s) involved
Not adapter-specific; this is a core CI workflow bug.
What Changed
Verification
node --test scripts/__tests__/pr-verify-lane-outcome.test.mjs(8/8)scripts/__tests__/*.test.mjssuite (40/40)Risks
Low behavioral risk. The merge gate remains fail-closed for every non-success result. The change only affects annotations and classification, with explicit mixed-outcome tests.
Model Used
Claude Sonnet 5 through a Paperclip Kubernetes agent, 1M-context configuration, with repository and GitHub tool use. Independent Ally review lenses were applied to the exact head.
Checklist