From f2c5b2b26939a91c9329178e40a882d7b8c88386 Mon Sep 17 00:00:00 2001 From: shiny-code-bot Date: Sat, 26 Sep 2026 19:27:22 -0400 Subject: [PATCH 1/2] Build merge-train candidates before publishing CI refs --- control_plane/merge_train_github.py | 88 ++++++- docs/merge-train-policy.md | 50 ++-- docs/style/testing.md | 14 +- tests/test_merge_train_github.py | 340 ++++++++++++++++++++-------- tests/test_workflow_invariants.py | 8 + 5 files changed, 381 insertions(+), 119 deletions(-) diff --git a/control_plane/merge_train_github.py b/control_plane/merge_train_github.py index 514ae65de..30b2af19c 100644 --- a/control_plane/merge_train_github.py +++ b/control_plane/merge_train_github.py @@ -1,4 +1,6 @@ import json +from hashlib import sha256 +import logging from time import sleep from typing import TYPE_CHECKING, Callable, Literal, Protocol, TypeVar from urllib.error import HTTPError, URLError @@ -50,6 +52,8 @@ from control_plane.merge_train import MergeTrainPullRequestState from control_plane.merge_admission import GuardedMergeAdmission, MergeAdmissionDeniedError +logger = logging.getLogger(__name__) + if TYPE_CHECKING: from control_plane.tenant_admission_controller import TenantAdmissionTechnicalChecks @@ -361,9 +365,13 @@ def build_batch_candidate( ) -> MergeTrainBatchCandidate: resolved_effect_executor = effect_executor or self.semantic_effect_executor repository_path = _repository_path(candidate.repository) - candidate_branch = _branch_name_from_ref(candidate.candidate_ref) + construction_ref = ( + "refs/heads/launchplane/construct/" + + sha256(candidate.candidate_ref.encode("utf-8")).hexdigest() + ) + candidate_branch = _branch_name_from_ref(construction_ref) if checkpoint is not None: - checkpoint(candidate, None, "reset_candidate_ref") + checkpoint(candidate, None, "reset_construction_ref") resolved_effect_executor.prepare_candidate_ref( CandidateRefPrepareEffect( lineage=MergeTrainEffectLineage( @@ -371,12 +379,12 @@ def build_batch_candidate( base_branch=candidate.base_branch, batch_id=candidate.batch_id, ), - candidate_ref=candidate.candidate_ref, + candidate_ref=construction_ref, base_sha=candidate.base_sha, ) ) if checkpoint is not None: - checkpoint(candidate, None, "candidate_ref_ready") + checkpoint(candidate, None, "construction_ref_ready") base_identity = _git_commit_identity( transport=self.transport, repository_path=repository_path, @@ -408,7 +416,7 @@ def build_batch_candidate( base_branch=candidate.base_branch, batch_id=candidate.batch_id, ), - candidate_ref=candidate.candidate_ref, + candidate_ref=construction_ref, rolling_parent_sha=parent_sha, pull_request_number=entry.pull_request_number, head_sha=entry.head_sha, @@ -512,6 +520,48 @@ def build_batch_candidate( f"candidate_entry_merged:{entry_index}", ) candidate = progress_candidate + if checkpoint is not None: + checkpoint(candidate, None, "publish_candidate_ref") + resolved_effect_executor.prepare_candidate_ref( + CandidateRefPrepareEffect( + lineage=MergeTrainEffectLineage( + repository=candidate.repository, + base_branch=candidate.base_branch, + batch_id=candidate.batch_id, + ), + candidate_ref=candidate.candidate_ref, + # Publication points at the completed candidate, never an intermediate base. + base_sha=candidate_sha, + ) + ) + _verify_candidate_publication( + transport=self.transport, + repository_path=repository_path, + candidate_ref=candidate.candidate_ref, + expected_sha=candidate_sha, + ) + if checkpoint is not None: + checkpoint(candidate, None, "candidate_ref_published") + try: + resolved_effect_executor.delete_candidate_ref( + CandidateRefDeleteEffect( + lineage=MergeTrainEffectLineage( + repository=candidate.repository, + base_branch=candidate.base_branch, + batch_id=candidate.batch_id, + ), + candidate_ref=construction_ref, + expected_ref_sha=candidate_sha, + ) + ) + except MergeTrainGitHubError as error: + # Rebuilding here would replace a verified publication and start duplicate CI. + logger.warning( + "Published candidate retained; construction ref cleanup failed for %s " + "(GitHub status %s).", + construction_ref, + error.status_code, + ) return _validated_model_update(candidate, status="ready_for_checks") def observe_batch_candidate_checks( @@ -2269,6 +2319,34 @@ def _base_branch_sha( return _required_text(commit.get("sha"), "GitHub branch commit requires sha.") +def _verify_candidate_publication( + *, + transport: MergeTrainGitHubTransport, + repository_path: str, + candidate_ref: str, + expected_sha: str, +) -> None: + for delay_seconds in (0.0, *MERGE_REF_READ_DELAYS_SECONDS): + if delay_seconds: + sleep(delay_seconds) + try: + observed_sha = _base_branch_sha( + transport=transport, + repository_path=repository_path, + base_branch=_branch_name_from_ref(candidate_ref), + ) + except MergeTrainGitHubError as error: + if error.status_code != 404: + raise + continue + if observed_sha == expected_sha: + return + raise MergeTrainGitHubStaleHeadError( + "GitHub published candidate ref did not resolve to the completed candidate.", + status_code=409, + ) + + def _wait_for_branch_sha( *, transport: MergeTrainGitHubTransport, diff --git a/docs/merge-train-policy.md b/docs/merge-train-policy.md index 14689afa6..572798b07 100644 --- a/docs/merge-train-policy.md +++ b/docs/merge-train-policy.md @@ -285,12 +285,27 @@ A batch candidate represents: base branch + queued PR #1 + queued PR #2 + ... + queued PR #N ``` -The candidate is built in deterministic queue order. If any pull request cannot -be applied cleanly, candidate construction stops at that pull request and the -worker records a blocker. The first implementation should use an explicit -temporary candidate ref or branch so GitHub Actions can run checks against a -real commit SHA. The exact ref naming and cleanup policy are part of the batch -train implementation, not the repository policy TOML. +The candidate is built in deterministic queue order on a temporary +`launchplane/construct/` branch. The digest is the SHA-256 of the +canonical candidate ref, so retries use the same construction branch. Only +after every entry's rolling commit and tree are verified does the native +GitHub adapter publish `launchplane/train/**` at the completed candidate SHA. +Base and intermediate construction pushes therefore do not start required +workflows. The canonical candidate ref, persisted candidate identity, and +rolling provenance remain the inputs to checks and landing. + +Publication has a bounded exact-SHA readback, including temporary 404s while a +new branch becomes visible. A failed or interrupted publication never returns +`ready_for_checks`. Retrying a build resets the construction branch and +reconstructs the candidate; a crash after publication but before persistence +can still require a new publication and checks. + +After verified publication, the adapter deletes the construction ref through +the semantic effect executor. An already missing ref is clean. Other cleanup +failures are logged with the ref and HTTP status without discarding the verified +candidate or restarting its CI; the retained ref has no landing authority. +Failed or interrupted builds retain their construction ref as recovery evidence. +Ref naming is an implementation detail, not mutable repository policy. After GitHub creates a candidate merge commit, Launchplane performs a bounded read-after-write convergence check before declaring the candidate ref stale. @@ -306,8 +321,11 @@ Launchplane must fail closed when candidate check evidence is missing, pending, failed, stale, or attached to a different commit SHA. Repositories using batch candidates must run their required workflows for -pushes to `launchplane/train/**`. Aggregate required-check jobs must also run on -those push events and treat the candidate as same-repository work when no pull +pushes to `launchplane/train/**` and exclude `launchplane/construct/**` from +those triggers. A workflow matching every branch would still run intermediate +checks and could supply check evidence before the final train push registers. +Aggregate required-check jobs must also run on those push events and treat the +candidate as same-repository work when no pull request payload exists. Otherwise the candidate has no exact-SHA check evidence and remains fail-closed in `ready_for_checks`. @@ -334,11 +352,12 @@ structural proof still blocks. This does not claim to observe GitHub's strict setting and does not grant, change, or bypass provider protection; GitHub's guarded merge endpoint continues to enforce its own policy. -Candidate-ref workflow concurrency must keep create/force-reset pushes separate -from normal construction pushes. Normal intermediate pushes cancel each other -for the same ref, while the reset run retains its own SHA-keyed group so a -cancelled duplicate does not replace the protected base commit's successful -required-check evidence. Candidate-specific cancellation must not broaden a +Candidate-ref workflow concurrency keeps create/force-reset pushes separate +from ordinary ref updates. Native construction now publishes only the completed +candidate; existing concurrency rules remain for publication retries and +previously created refs. Create/reset runs retain their SHA-keyed group so a +cancelled duplicate cannot replace the protected base commit's successful +required-check evidence. Candidate-specific cancellation does not broaden a workflow's cancellation policy for ordinary base-branch pushes. ### PR-Native Landing @@ -415,8 +434,9 @@ the failed candidate batch lineage and plan a replacement candidate from the fresh snapshot. Candidate construction can fail before required checks run when GitHub rejects -one rolling merge entry as stale or conflicting. The controller persists that -candidate as `failed`, reports the exact pull request reached by the build, and +one rolling merge entry as stale or conflicting. Its partial state is retained +on the construction ref; no new canonical train ref is published. The controller +persists that candidate as `failed`, reports the exact pull request reached by the build, and releases the controller lease without replaying the rejected merge. The same queue-change rule then governs replacement planning; an unchanged queue remains stopped for operator attention. diff --git a/docs/style/testing.md b/docs/style/testing.md index 958975914..2789dd622 100644 --- a/docs/style/testing.md +++ b/docs/style/testing.md @@ -80,16 +80,20 @@ For pushes to `main` and `launchplane/train/**`, the `verified-tree` job can reuse a completed, successful GitHub Actions `ci-gate` on the exact pushed commit. Its check suite must identify that same commit on main without a PR merge context; conflicting pending or failed gates veto reuse. This avoids full -work on already-tested train base creation/reset without trusting a fork or -retargeted PR's merge-ref checks. New candidate commits still run full CI. +work when an all-no-op batch publishes the already-tested base, without trusting +a fork or retargeted PR's merge-ref checks. Native construction uses +`launchplane/construct/**`, outside the required workflows' push filters, and +publishes the canonical train ref only after every entry is verified. New +completed candidate commits still run full CI. Main retains its existing PR-tree reuse, tightened to require that the base is an ancestor of the PR head as well as matching trees and a successful gate. The train does not extend that shortcut: a historical PR gate alone cannot identify the merge-ref tree tested before a retarget. PR events, unrelated -branches, and missing API evidence run the full suite. Candidate construction, -concurrency and required checks are unchanged, and every final candidate SHA -still receives its own gate. +branches, and missing API evidence run the full suite. Construction branches +must remain outside required workflow triggers. Concurrency and required checks +are unchanged, and every published final candidate SHA still receives its own +gate. Same-repo CI currently uses 12 unittest shards with a 20-test/30-second split threshold to keep large app and service targets under the tool wall-clock diff --git a/tests/test_merge_train_github.py b/tests/test_merge_train_github.py index 0a8098ecc..52abf2405 100644 --- a/tests/test_merge_train_github.py +++ b/tests/test_merge_train_github.py @@ -582,103 +582,44 @@ def test_merge_stack_child_into_parent_allows_main_when_custom_base_is_protected self.assertEqual(merge_commit_sha, "parent-after-child") - def test_build_batch_candidate_creates_ref_and_merges_heads_in_order(self) -> None: + def test_build_batch_candidate_publishes_only_after_all_heads_are_merged(self) -> None: candidate = _batch_candidate() transport = RecordingMergeTrainGitHubTransport( - responses=( - {"ref": candidate.candidate_ref, "object": {"sha": "base-main"}}, - _git_commit("base-main", "tree-base"), - _git_commit("head-1", "tree-head-1"), - _git_commit("head-2", "tree-head-2"), - _merge_commit("candidate-after-1", "tree-candidate-1"), - _github_branch(sha="candidate-after-1", tree_sha="tree-candidate-1"), - _git_commit( - "candidate-after-1", - "tree-candidate-1", - parents=("base-main", "head-1"), - ), - _merge_commit("candidate-after-2", "tree-candidate-2"), - _github_branch(sha="candidate-after-2", tree_sha="tree-candidate-2"), - _git_commit( - "candidate-after-2", - "tree-candidate-2", - parents=("candidate-after-1", "head-2"), - ), - ) + responses=(*_candidate_build_responses(), {}, _published_candidate_branch(), None) ) - - built_candidate = GitHubMergeTrainClient(transport=transport).build_batch_candidate( + built = GitHubMergeTrainClient(transport=transport).build_batch_candidate( candidate=candidate ) - self.assertEqual(built_candidate.status, "ready_for_checks") - self.assertEqual(built_candidate.candidate_sha, "candidate-after-2") + self.assertEqual(built.status, "ready_for_checks") + self.assertEqual(built.candidate_sha, "candidate-after-2") + first_body = transport.requests[0].body + assert first_body is not None + construction_ref = str(first_body["ref"]) + self.assertTrue(construction_ref.startswith("refs/heads/launchplane/construct/")) + merges = [request for request in transport.requests if request.path.endswith("/merges")] self.assertEqual( - [(request.method, request.path, request.body) for request in transport.requests], - [ - ( - "POST", - "/repos/example/merge-train-repo/git/refs", - {"ref": candidate.candidate_ref, "sha": "base-main"}, - ), - ( - "GET", - "/repos/example/merge-train-repo/git/commits/base-main", - None, - ), - ( - "GET", - "/repos/example/merge-train-repo/git/commits/head-1", - None, - ), - ( - "GET", - "/repos/example/merge-train-repo/git/commits/head-2", - None, - ), - ( - "POST", - "/repos/example/merge-train-repo/merges", - { - "base": "launchplane/train/example/merge-train-repo/main/" - f"{candidate.batch_id}", - "head": "head-1", - "commit_message": f"Launchplane merge train {candidate.batch_id}: merge PR #1", - }, - ), - ( - "GET", - "/repos/example/merge-train-repo/branches/launchplane%2Ftrain%2Fexample%2Fmerge-train-repo%2Fmain%2F" - f"{candidate.batch_id}", - None, - ), - ( - "GET", - "/repos/example/merge-train-repo/git/commits/candidate-after-1", - None, - ), - ( - "POST", - "/repos/example/merge-train-repo/merges", - { - "base": "launchplane/train/example/merge-train-repo/main/" - f"{candidate.batch_id}", - "head": "head-2", - "commit_message": f"Launchplane merge train {candidate.batch_id}: merge PR #2", - }, - ), - ( - "GET", - "/repos/example/merge-train-repo/branches/launchplane%2Ftrain%2Fexample%2Fmerge-train-repo%2Fmain%2F" - f"{candidate.batch_id}", - None, - ), - ( - "GET", - "/repos/example/merge-train-repo/git/commits/candidate-after-2", - None, - ), - ], + [request.body["head"] for request in merges if request.body], ["head-1", "head-2"] + ) + self.assertEqual( + {request.body["base"] for request in merges if request.body}, + {construction_ref.removeprefix("refs/heads/")}, + ) + publications = [ + request + for request in transport.requests + if request.body and request.body.get("ref") == candidate.candidate_ref + ] + self.assertEqual(len(publications), 1) + self.assertEqual( + publications[0].body, {"ref": candidate.candidate_ref, "sha": built.candidate_sha} + ) + self.assertLess( + transport.requests.index(merges[-1]), transport.requests.index(publications[0]) + ) + self.assertEqual(transport.requests[-1].method, "DELETE") + self.assertTrue( + transport.requests[-1].path.endswith(construction_ref.removeprefix("refs/")) ) def test_build_batch_candidate_routes_writes_through_injected_semantic_executor( @@ -705,6 +646,7 @@ def test_build_batch_candidate_routes_writes_through_injected_semantic_executor( "tree-candidate-2", parents=("candidate-after-1", "head-2"), ), + _github_branch(sha="candidate-after-2", tree_sha="tree-candidate-2"), ) ) @@ -718,7 +660,13 @@ def test_build_batch_candidate_routes_writes_through_injected_semantic_executor( self.assertTrue(all(request.method == "GET" for request in transport.requests)) self.assertEqual( [type(effect) for effect in executor.effects], - [CandidateRefPrepareEffect, CandidateHeadMergeEffect, CandidateHeadMergeEffect], + [ + CandidateRefPrepareEffect, + CandidateHeadMergeEffect, + CandidateHeadMergeEffect, + CandidateRefPrepareEffect, + CandidateRefDeleteEffect, + ], ) first_merge = executor.effects[1] assert isinstance(first_merge, CandidateHeadMergeEffect) @@ -728,6 +676,15 @@ def test_build_batch_candidate_routes_writes_through_injected_semantic_executor( second_merge = executor.effects[2] assert isinstance(second_merge, CandidateHeadMergeEffect) self.assertEqual(second_merge.rolling_parent_sha, "candidate-after-1") + publication = executor.effects[3] + assert isinstance(publication, CandidateRefPrepareEffect) + self.assertEqual(publication.candidate_ref, candidate.candidate_ref) + self.assertEqual(publication.base_sha, built.candidate_sha) + self.assertNotEqual(first_merge.candidate_ref, publication.candidate_ref) + cleanup = executor.effects[4] + assert isinstance(cleanup, CandidateRefDeleteEffect) + self.assertEqual(cleanup.candidate_ref, first_merge.candidate_ref) + self.assertEqual(cleanup.expected_ref_sha, built.candidate_sha) def test_build_batch_candidate_resets_existing_ref(self) -> None: candidate = _batch_candidate() @@ -754,6 +711,9 @@ def test_build_batch_candidate_resets_existing_ref(self) -> None: "tree-candidate-2", parents=("candidate-after-1", "head-2"), ), + {}, # Publish only the completed candidate. + _github_branch(sha="candidate-after-2", tree_sha="tree-candidate-2"), + None, # Delete the construction ref. ) ) @@ -764,8 +724,8 @@ def test_build_batch_candidate_resets_existing_ref(self) -> None: self.assertEqual(transport.requests[1].method, "PATCH") self.assertEqual( transport.requests[1].path, - "/repos/example/merge-train-repo/git/refs/heads/launchplane/train/example/merge-train-repo/main/" - f"{candidate.batch_id}", + "/repos/example/merge-train-repo/git/refs/" + + str((transport.requests[0].body or {})["ref"]).removeprefix("refs/"), ) self.assertEqual(transport.requests[1].body, {"sha": "base-main", "force": True}) @@ -790,6 +750,9 @@ def test_build_batch_candidate_retries_eventually_consistent_ref_read(self) -> N "tree-candidate-2", parents=("candidate-after-1", "head-2"), ), + {}, # Publish only the completed candidate. + _github_branch(sha="candidate-after-2", tree_sha="tree-candidate-2"), + None, # Delete the construction ref. ) ) @@ -803,7 +766,7 @@ def test_build_batch_candidate_retries_eventually_consistent_ref_read(self) -> N sleep_mock.assert_called_once_with(0.25) self.assertEqual( [request.method for request in transport.requests].count("POST"), - 3, + 4, ) def test_build_batch_candidate_accepts_expected_sha_on_final_ref_read(self) -> None: @@ -832,6 +795,9 @@ def test_build_batch_candidate_accepts_expected_sha_on_final_ref_read(self) -> N "tree-candidate-2", parents=("candidate-after-1", "head-2"), ), + {}, # Publish only the completed candidate. + _github_branch(sha="candidate-after-2", tree_sha="tree-candidate-2"), + None, # Delete the construction ref. ) ) @@ -927,6 +893,12 @@ def test_build_batch_candidate_reports_conflict_without_landing_prs(self) -> Non [request.method for request in transport.requests], ["POST", "GET", "GET", "GET", "POST", "GET", "GET", "POST"], ) + self.assertFalse( + any( + request.body and request.body.get("ref") == candidate.candidate_ref + for request in transport.requests + ) + ) def test_build_batch_candidate_records_github_204_as_no_op_step(self) -> None: candidate = _batch_candidate() @@ -947,6 +919,9 @@ def test_build_batch_candidate_records_github_204_as_no_op_step(self) -> None: "tree-candidate-2", parents=("base-main", "head-2"), ), + {}, # Publish only the completed candidate. + _github_branch(sha="candidate-after-2", tree_sha="tree-candidate-2"), + None, # Delete the construction ref. ) ) @@ -982,6 +957,159 @@ def test_build_batch_candidate_rejects_unexplained_merge_parent(self) -> None: with self.assertRaisesRegex(MergeTrainGitHubStaleHeadError, "parents"): GitHubMergeTrainClient(transport=transport).build_batch_candidate(candidate=candidate) + def test_all_no_op_entries_publish_the_base_after_construction_reads(self) -> None: + candidate = _batch_candidate() + no_op = ( + None, + _github_branch(sha="base-main", tree_sha="tree-base"), + _git_commit("base-main", "tree-base"), + {"status": "ahead"}, + ) + transport = RecordingMergeTrainGitHubTransport( + responses=( + {}, + _git_commit("base-main", "tree-base"), + _git_commit("head-1", "tree-head-1"), + _git_commit("head-2", "tree-head-2"), + *no_op, + *no_op, + {}, + _github_branch(sha="base-main", tree_sha="tree-base"), + None, + ) + ) + built = GitHubMergeTrainClient(transport=transport).build_batch_candidate( + candidate=candidate + ) + + self.assertEqual(built.candidate_sha, "base-main") + self.assertEqual(built.status, "ready_for_checks") + assert built.structural_provenance is not None + self.assertTrue( + all( + step.kind == "no_op_already_contained" for step in built.structural_provenance.steps + ) + ) + containment_reads = [r for r in transport.requests if "/compare/" in r.path] + self.assertEqual(len(containment_reads), 2) + self.assertTrue(all("launchplane%2Fconstruct%2F" in r.path for r in containment_reads)) + self.assertEqual( + [ + r.body + for r in transport.requests + if r.body and r.body.get("ref") == candidate.candidate_ref + ], + [{"ref": candidate.candidate_ref, "sha": "base-main"}], + ) + + def test_publication_replaces_an_existing_ref_only_with_the_completed_sha(self) -> None: + candidate = _batch_candidate() + for status_code in (409, 422): + with self.subTest(status_code=status_code): + transport = RecordingMergeTrainGitHubTransport( + responses=( + *_candidate_build_responses(), + MergeTrainGitHubError("reference exists", status_code=status_code), + {}, + _published_candidate_branch(), + None, + ) + ) + built = GitHubMergeTrainClient(transport=transport).build_batch_candidate( + candidate=candidate + ) + updates = [r for r in transport.requests if r.method == "PATCH"] + self.assertEqual(len(updates), 1) + self.assertTrue( + updates[0].path.endswith(candidate.candidate_ref.removeprefix("refs/")) + ) + self.assertEqual(updates[0].body, {"sha": built.candidate_sha, "force": True}) + + def test_publication_waits_for_a_new_or_stale_branch_read(self) -> None: + for stale_read in ( + MergeTrainGitHubError("not found", status_code=404), + _github_branch(sha="old-partial-build", tree_sha="old-tree"), + ): + with self.subTest(stale_read=stale_read): + transport = RecordingMergeTrainGitHubTransport( + responses=( + *_candidate_build_responses(), + {}, + stale_read, + _published_candidate_branch(), + None, + ) + ) + with patch("control_plane.merge_train_github.sleep") as wait: + built = GitHubMergeTrainClient(transport=transport).build_batch_candidate( + candidate=_batch_candidate() + ) + self.assertEqual(built.status, "ready_for_checks") + wait.assert_called_once_with(0.25) + + def test_unverified_publication_retains_construction_and_fails_closed(self) -> None: + for read in ( + MergeTrainGitHubError("not found", status_code=404), + _github_branch(sha="wrong-sha", tree_sha="wrong-tree"), + ): + with self.subTest(read=read): + transport = RecordingMergeTrainGitHubTransport( + responses=(*_candidate_build_responses(), {}, *(read for _ in range(6))) + ) + with patch("control_plane.merge_train_github.sleep"): + with self.assertRaisesRegex( + MergeTrainGitHubStaleHeadError, "published candidate" + ): + GitHubMergeTrainClient(transport=transport).build_batch_candidate( + candidate=_batch_candidate() + ) + self.assertFalse(any(r.method == "DELETE" for r in transport.requests)) + + def test_failed_publication_keeps_construction_for_recovery(self) -> None: + transport = RecordingMergeTrainGitHubTransport( + responses=( + *_candidate_build_responses(), + MergeTrainGitHubError("provider unavailable", status_code=503), + ) + ) + with self.assertRaisesRegex(MergeTrainGitHubError, "provider unavailable"): + GitHubMergeTrainClient(transport=transport).build_batch_candidate( + candidate=_batch_candidate() + ) + self.assertFalse(any(r.method == "DELETE" for r in transport.requests)) + + def test_cleanup_failure_does_not_replace_a_verified_candidate(self) -> None: + transport = RecordingMergeTrainGitHubTransport( + responses=( + *_candidate_build_responses(), + {}, + _published_candidate_branch(), + MergeTrainGitHubError("provider unavailable", status_code=503), + ) + ) + with self.assertLogs("control_plane.merge_train_github", level="WARNING") as logs: + built = GitHubMergeTrainClient(transport=transport).build_batch_candidate( + candidate=_batch_candidate() + ) + self.assertEqual(built.status, "ready_for_checks") + self.assertEqual(built.candidate_sha, "candidate-after-2") + self.assertIn("construction ref cleanup failed", logs.output[0]) + self.assertEqual(transport.requests[-1].method, "DELETE") + + def test_missing_construction_ref_is_already_clean(self) -> None: + transport = RecordingMergeTrainGitHubTransport( + responses=( + *_candidate_build_responses(), + {}, + _published_candidate_branch(), + MergeTrainGitHubError("not found", status_code=404), + ) + ) + built = GitHubMergeTrainClient(transport=transport).build_batch_candidate( + candidate=_batch_candidate() + ) + self.assertEqual(built.status, "ready_for_checks") + def test_observe_batch_candidate_checks_marks_passed_candidate(self) -> None: candidate = _batch_candidate().model_copy( update={"candidate_sha": "candidate-sha", "status": "ready_for_checks"} @@ -1761,6 +1889,9 @@ def test_actual_landing_evidence_drives_recorded_rolling_evaluator(self) -> None "tree-candidate-2", parents=("candidate-after-1", "head-2"), ), + {}, # Publish only the completed candidate. + _github_branch(sha="candidate-after-2", tree_sha="tree-candidate-2"), + None, # Delete the construction ref. ) ) built = GitHubMergeTrainClient(transport=build_transport).build_batch_candidate( @@ -2833,6 +2964,27 @@ def _protected_branch_with_checks( return {"protected": True, "protection": {"required_status_checks": payload}} +def _candidate_build_responses() -> tuple[object, ...]: + return ( + {}, + _git_commit("base-main", "tree-base"), + _git_commit("head-1", "tree-head-1"), + _git_commit("head-2", "tree-head-2"), + _merge_commit("candidate-after-1", "tree-candidate-1"), + _github_branch(sha="candidate-after-1", tree_sha="tree-candidate-1"), + _git_commit("candidate-after-1", "tree-candidate-1", parents=("base-main", "head-1")), + _merge_commit("candidate-after-2", "tree-candidate-2"), + _published_candidate_branch(), + _git_commit( + "candidate-after-2", "tree-candidate-2", parents=("candidate-after-1", "head-2") + ), + ) + + +def _published_candidate_branch() -> dict[str, object]: + return _github_branch(sha="candidate-after-2", tree_sha="tree-candidate-2") + + def _batch_candidate() -> MergeTrainBatchCandidate: repository = "example/merge-train-repo" base_branch = "main" diff --git a/tests/test_workflow_invariants.py b/tests/test_workflow_invariants.py index 9be0ed93b..bf5ffadf8 100644 --- a/tests/test_workflow_invariants.py +++ b/tests/test_workflow_invariants.py @@ -1,3 +1,4 @@ +from fnmatch import fnmatchcase import unittest from pathlib import Path from tempfile import TemporaryDirectory @@ -27,6 +28,13 @@ def test_required_workflows_run_for_merge_train_candidate_refs(self) -> None: self.assertIsInstance(branches, list) assert isinstance(branches, list) self.assertIn("launchplane/train/**", branches) + self.assertFalse( + any( + fnmatchcase("launchplane/construct/example-batch", str(pattern)) + for pattern in branches + ), + f"{workflow_path} must not run required checks on construction refs", + ) for workflow_path in ( ".github/workflows/ci.yml", From adc417aa602fed5c6d3c43f30a2b894fb7e5079d Mon Sep 17 00:00:00 2001 From: shiny-code-bot Date: Sat, 26 Sep 2026 19:35:33 -0400 Subject: [PATCH 2/2] Keep construction evidence visible after candidate build failure --- control_plane/merge_train_controller_run_once.py | 13 +++++++++++++ control_plane/merge_train_github.py | 11 ++++++----- docs/merge-train-policy.md | 7 +++++++ tests/support/merge_train.py | 10 ++++++---- tests/test_http_app_merge_train.py | 11 ++++++++++- tests/test_merge_train_github.py | 1 - 6 files changed, 42 insertions(+), 11 deletions(-) diff --git a/control_plane/merge_train_controller_run_once.py b/control_plane/merge_train_controller_run_once.py index 45f64428f..52db44e56 100644 --- a/control_plane/merge_train_controller_run_once.py +++ b/control_plane/merge_train_controller_run_once.py @@ -65,6 +65,7 @@ MergeTrainGitHubStaleHeadError, MergeTrainGitHubTransport, UrllibMergeTrainGitHubTransport, + merge_train_construction_ref, ) from control_plane.merge_train_stack_collapse import ( MergeTrainStackCollapsePlanRecordStore, @@ -1706,6 +1707,15 @@ def _advance_active_candidate_record( return reflow_result candidate_build_error: MergeTrainGitHubStaleHeadError | None = None + construction_evidence = ( + { + "construction_ref": merge_train_construction_ref( + active_candidate_record.candidate.candidate_ref + ) + } + if active_candidate_record.ordinary_job_binding is None + else {} + ) if active_candidate_record.candidate.status in {"planned", "building"}: controller_action = "build_candidate" if request.mutate: @@ -1738,6 +1748,7 @@ def checkpoint_candidate_progress( "candidate_ref": progress_candidate.candidate_ref, "candidate_sha": progress_candidate.candidate_sha, "completed_entry_count": (int(phase.split(":", 1)[1]) if ":" in phase else 0), + **construction_evidence, }, ) @@ -1795,6 +1806,7 @@ def checkpoint_candidate_progress( result["details"] = { "github_status_code": candidate_build_error.status_code, "failed_pull_request_number": lease.record.active_pull_request_number, + **construction_evidence, } if request.mutate: updated_candidate_record = build_merge_train_batch_candidate_record( @@ -1815,6 +1827,7 @@ def checkpoint_candidate_progress( "candidate_ref": candidate.candidate_ref, "candidate_sha": candidate.candidate_sha, "candidate_status": candidate.status, + **construction_evidence, }, ) result["candidate"] = candidate.model_dump(mode="json") diff --git a/control_plane/merge_train_github.py b/control_plane/merge_train_github.py index 30b2af19c..494dceed6 100644 --- a/control_plane/merge_train_github.py +++ b/control_plane/merge_train_github.py @@ -365,10 +365,7 @@ def build_batch_candidate( ) -> MergeTrainBatchCandidate: resolved_effect_executor = effect_executor or self.semantic_effect_executor repository_path = _repository_path(candidate.repository) - construction_ref = ( - "refs/heads/launchplane/construct/" - + sha256(candidate.candidate_ref.encode("utf-8")).hexdigest() - ) + construction_ref = merge_train_construction_ref(candidate.candidate_ref) candidate_branch = _branch_name_from_ref(construction_ref) if checkpoint is not None: checkpoint(candidate, None, "reset_construction_ref") @@ -551,7 +548,6 @@ def build_batch_candidate( batch_id=candidate.batch_id, ), candidate_ref=construction_ref, - expected_ref_sha=candidate_sha, ) ) except MergeTrainGitHubError as error: @@ -2319,6 +2315,11 @@ def _base_branch_sha( return _required_text(commit.get("sha"), "GitHub branch commit requires sha.") +def merge_train_construction_ref(candidate_ref: str) -> str: + """Locate native construction evidence from the canonical candidate identity.""" + return "refs/heads/launchplane/construct/" + sha256(candidate_ref.encode("utf-8")).hexdigest() + + def _verify_candidate_publication( *, transport: MergeTrainGitHubTransport, diff --git a/docs/merge-train-policy.md b/docs/merge-train-policy.md index 572798b07..04edbcb5d 100644 --- a/docs/merge-train-policy.md +++ b/docs/merge-train-policy.md @@ -305,6 +305,9 @@ the semantic effect executor. An already missing ref is clean. Other cleanup failures are logged with the ref and HTTP status without discarding the verified candidate or restarting its CI; the retained ref has no landing authority. Failed or interrupted builds retain their construction ref as recovery evidence. +The native controller checkpoint records its exact `construction_ref`, and a +failed build returns that locator with the provider status. A ref locator is +not proof that the ref still exists. Ref naming is an implementation detail, not mutable repository policy. After GitHub creates a candidate merge commit, Launchplane performs a bounded @@ -441,6 +444,10 @@ releases the controller lease without replaying the rejected merge. The same queue-change rule then governs replacement planning; an unchanged queue remains stopped for operator attention. +An exhausted final-publication readback also fails closed, with no individual +failed pull request: its checkpoint identifies the publication phase and the +retained construction ref. + ## Example Policy Entries The example below is documentation/import material only. It is not packaged as a diff --git a/tests/support/merge_train.py b/tests/support/merge_train.py index b3d929d14..164120c3a 100644 --- a/tests/support/merge_train.py +++ b/tests/support/merge_train.py @@ -111,8 +111,8 @@ def build_batch_candidate( ) = None, ) -> MergeTrainBatchCandidate: if checkpoint is not None: - checkpoint(candidate, None, "reset_candidate_ref") - checkpoint(candidate, None, "candidate_ref_ready") + checkpoint(candidate, None, "reset_construction_ref") + checkpoint(candidate, None, "construction_ref_ready") for entry_index, entry in enumerate(candidate.entries, start=1): checkpoint(candidate, entry, "merge_candidate_entry") checkpoint( @@ -120,6 +120,8 @@ def build_batch_candidate( entry, f"candidate_entry_merged:{entry_index}", ) + checkpoint(candidate, None, "publish_candidate_ref") + checkpoint(candidate, None, "candidate_ref_published") return candidate.model_copy( update={"candidate_sha": "candidate-built", "status": "ready_for_checks"} ) @@ -273,8 +275,8 @@ def build_batch_candidate( ) = None, ) -> MergeTrainBatchCandidate: if checkpoint is not None: - checkpoint(candidate, None, "reset_candidate_ref") - checkpoint(candidate, None, "candidate_ref_ready") + checkpoint(candidate, None, "reset_construction_ref") + checkpoint(candidate, None, "construction_ref_ready") checkpoint(candidate, candidate.entries[0], "merge_candidate_entry") raise MergeTrainGitHubStaleHeadError( "Candidate entry conflicts with the rolling merge base.", status_code=409 diff --git a/tests/test_http_app_merge_train.py b/tests/test_http_app_merge_train.py index 6d5df4b01..8d2191b45 100644 --- a/tests/test_http_app_merge_train.py +++ b/tests/test_http_app_merge_train.py @@ -29,6 +29,7 @@ ) from control_plane.merge_train import MergeTrainDryRunSnapshot from control_plane.merge_train_controller_run_once import MERGE_TRAIN_CONTROLLER_ACTIVE_ACTION +from control_plane.merge_train_github import merge_train_construction_ref from control_plane.service_auth import ( BearerIdentityConfig, LaunchplaneAuthzPolicy, @@ -2847,7 +2848,13 @@ async def test_reflows_candidate_after_build_stale_state(self) -> None: ) self.assertEqual( failed_payload["result"]["details"], - {"failed_pull_request_number": 1, "github_status_code": 409}, + { + "failed_pull_request_number": 1, + "github_status_code": 409, + "construction_ref": merge_train_construction_ref( + failed_payload["result"]["candidate"]["candidate_ref"] + ), + }, ) self.assertEqual(reflow_response.status_code, 202) self.assertEqual(reflow_payload["result"]["controller_action"], "plan_candidate") @@ -3629,6 +3636,8 @@ def capture_idempotency(record: object) -> object: self.assertEqual(response.status_code, 202) self.assertIn("merge_candidate_entry", observed_phases) self.assertIn("candidate_entry_merged", observed_phases) + self.assertIn("publish_candidate_ref", observed_phases) + self.assertIn("candidate_ref_published", observed_phases) self.assertEqual(idempotency_controller_statuses, ["running"]) self.assertEqual(final_state.status, "idle") diff --git a/tests/test_merge_train_github.py b/tests/test_merge_train_github.py index 52abf2405..91d964b39 100644 --- a/tests/test_merge_train_github.py +++ b/tests/test_merge_train_github.py @@ -684,7 +684,6 @@ def test_build_batch_candidate_routes_writes_through_injected_semantic_executor( cleanup = executor.effects[4] assert isinstance(cleanup, CandidateRefDeleteEffect) self.assertEqual(cleanup.candidate_ref, first_merge.candidate_ref) - self.assertEqual(cleanup.expected_ref_sha, built.candidate_sha) def test_build_batch_candidate_resets_existing_ref(self) -> None: candidate = _batch_candidate()