ci: a merge that waits ends the resolver green, not red (ADR-1128) - #964

Claude finished @cdeust's task in 2m 55s —— View job

Reviewed PR #964

• Gather context (diff, resolver, downstream job conditions)
• Run the five review agents (quality, performance, tests, docs, security)
• Verify findings and post the review

The review is posted: #964 (comment)

• Blockers: none.
• Notes (3):
  • The match is on the message text, so the same "head advanced" message in _confirm_pr_merge, after the merge, would end green. That case can't be reached today.
  • ADR-1127 doesn't say that ADR-1128 amends it.
  • No test runs main() through the real resolve().
• I couldn't run pytest in this job because I don't have permission. The findings come from reading the code.

Verdict: approve

Review 1 of 3 — PR #964

No Blockers. The waiting refusals are the four fixed merge held: ... literals (pipeline-post-merge.yml:264,384,397,398) plus PR head advanced after testing (:206). They contain no interpolated data, so no API or PR text reaches the ::notice. Every merge held: refusal is raised before the merge PUT (:478). No output is written, so contract (run_tests == 'true', :705) and the whole test/tag chain stay skipped. The new pr_number != '' gate on close-issues is required: without it, int("") at :656 would turn every waiting run red again. The workflow condition and ci_pipeline_profiles.conditions() are held equal by check_runtime_contract, through test_ci_pipeline_routes.py:36, so the ADR's test citation is correct.

Notes, no action before merge

• The waiting match is on the message, not the call site. _confirm_pr_merge (:493) also raises PR head advanced after testing through _pr_identity, after the PUT. That would end green with the merge commit untested. GitHub freezes a merged PR's head and the PUT is pinned to sha, so this is unreachable today. A MergeHeld(PostMergeError) raised only before the merge would close it for good.
• ADR-1127's Status and Negative consequence still say a held wake-up "ends in failure". An "amended by ADR-1128" line would match how ADR-1097, ADR-1104 and ADR-1106 record their amendments.
• run_main stubs resolve(), so no test sends a real refusal raised inside resolve() through main().

Five reviewers ran: quality, performance, tests, docs and security. Performance and security found nothing actionable. I could not run pytest in this job (no permission); the findings come from reading the code.

View job run

Verdict: approve

PR #964 was updated after the trigger (dd62ec5 -> fb8e59e). Push again or mention Claude to run on the latest version.

Claude finished @cdeust's task in 2m 17s —— View job

Reviewed PR #964 (review 2 of 3)

• Check the Blockers from review 1 (there were none)

• Scope: commits after 6f27dca, which are dd62ec5 (merge of main) and fb8e59e (CHANGELOG conflict fix)

• Run the five review agents (quality, performance, tests, docs, security)

• Post the review: ci: a merge that waits ends the resolver green, not red (ADR-1128) #964 (comment)

• Blockers: none.

• Since review 1:

  • The merge left conflict markers in CHANGELOG.md, and fb8e59e removed them, keeping both entries.
  • The PR's own files are the same as at 6f27dca.
  • Main's changes to claude-review.yml don't affect the resolver.

• Notes (3):

  • A PR that hits the review cap now waits without a red run to show it.
  • Two notes are still open from review 1: the "head advanced" message also matches after the merge, and ADR-1127 has no "amended by ADR-1128" line.

• I read the code but did not run pytest in this job.

Verdict: approve

Review 2 of 3 — PR #964

Review 1 found no Blockers. Two commits came after 6f27dca: dd62ec5, which merges main, and fb8e59e, which fixes the CHANGELOG conflict. I reviewed only those two.

No Blockers.

• The merge committed conflict markers, and the next commit removed them. dd62ec5 left <<<<<<</>>>>>>> in CHANGELOG.md. fb8e59e keeps both entries under [Unreleased] → ### Changed: ADR-1128 first, then main's ADR-1119 entry, with no duplicates. No conflict markers are left anywhere in the tree.
• The PR's own files are the same as at 6f27dca. That covers pipeline-post-merge.yml, ci_pipeline_profiles.py, the new test, both ADR-1128 pages and wiki/manifest.json. ADR-1128 is still the only ADR with that number; main's ci: admit the Claude reviews of one pull request one at a time #963 adds no new one.
• Main's claude-review.yml changes don't affect the resolver. Those changes are the budget job and the claude-review-<PR> concurrency group. The job is still named Claude review and the step PR Review, which is what _ran_the_review (pipeline-post-merge.yml:353-361) looks for. A queued review that gets replaced ends cancelled, so it can never supply a verdict (:359, :381, :392). test_ci_pipeline_routes.py:36 and check_runtime_contract still agree with the close-issues and contract conditions.

Notes, no action before merge

• When ADR-1119's budget skips a fourth review, the resolver holds with merge held: no Claude review ran on this head (:384). With this PR that hold ends green with a notice, so a PR over the cap no longer shows a red run.
• Open from review 1: _confirm_pr_merge (:493) raises the same PR head advanced after testing text after the merge PUT, and the message match (:537, :562) would turn it green. It can't happen today; a MergeHeld raised only before the merge would rule it out.
• Open from review 1: ADR-1127's Status and Negative consequence still say a held wake-up "ends in failure", with no "amended by ADR-1128" line.

Five reviewers ran: quality, performance, tests, docs and security. Quality, performance and docs found nothing. I read the code but did not run pytest in this job.

View job run

Verdict: approve