From e1c92f02afbce78be6bb7d34de2ab9e0366fd3e7 Mon Sep 17 00:00:00 2001 From: Jake Barnby Date: Fri, 21 Aug 2026 16:58:42 +1200 Subject: [PATCH 1/2] (fix): bound untagged recovery to the head of main An automation merge with no tag was read as an interrupted release no matter how old it was, and the merged-pull-request lookup walks the whole closed-PR history. A release that was deliberately abandoned therefore stayed recoverable forever, and the next run would tag and publish it from a commit main had long moved past. The window where recovery is warranted is the one between the merge and its tag, and in that window the merge is still the tip. Once main has moved on, the release was abandoned rather than interrupted, so leave it alone. Co-Authored-By: Claude Opus 5 --- .../scripts/src/Automation/Orchestrator.php | 1 + .../src/Automation/RecoverySelector.php | 2 + .github/scripts/src/Automation/Repository.php | 2 + .../src/Automation/Repository/GitHub.php | 16 +++++ .../Unit/Automation/OrchestratorTest.php | 1 + .../tests/Unit/Automation/RecoveryTest.php | 60 +++++++++++++++---- CHANGES.md | 1 + 7 files changed, 73 insertions(+), 10 deletions(-) diff --git a/.github/scripts/src/Automation/Orchestrator.php b/.github/scripts/src/Automation/Orchestrator.php index 8b1129a..a0df20a 100644 --- a/.github/scripts/src/Automation/Orchestrator.php +++ b/.github/scripts/src/Automation/Orchestrator.php @@ -45,6 +45,7 @@ public function recover(): ?Candidate $tags, $this->repository->releases($tags), $this->repository->mergedPullRequests(), + $this->repository->head(), ); } diff --git a/.github/scripts/src/Automation/RecoverySelector.php b/.github/scripts/src/Automation/RecoverySelector.php index 66f3daf..41ae0a8 100644 --- a/.github/scripts/src/Automation/RecoverySelector.php +++ b/.github/scripts/src/Automation/RecoverySelector.php @@ -15,6 +15,7 @@ public static function select( array $tags, array $releases, array $merges, + string $head, ): ?Candidate { $released = []; $published = []; @@ -85,6 +86,7 @@ public static function select( foreach ($merges as $merge) { if ( !isset($targets[$merge->target]) + && $merge->target === $head && MergeValidator::isAutomation($merge) ) { $candidates[] = new Candidate( diff --git a/.github/scripts/src/Automation/Repository.php b/.github/scripts/src/Automation/Repository.php index 535cc3c..586e1aa 100644 --- a/.github/scripts/src/Automation/Repository.php +++ b/.github/scripts/src/Automation/Repository.php @@ -6,6 +6,8 @@ interface Repository { + public function head(): string; + /** * @return list */ diff --git a/.github/scripts/src/Automation/Repository/GitHub.php b/.github/scripts/src/Automation/Repository/GitHub.php index 6807ca2..fead792 100644 --- a/.github/scripts/src/Automation/Repository/GitHub.php +++ b/.github/scripts/src/Automation/Repository/GitHub.php @@ -396,6 +396,22 @@ public function createTag(string $name, string $target): Tag throw new RuntimeException("Tag {$name} is missing after creation"); } + #[Override] + public function head(): string + { + $result = $this->api( + 'GET', + "repos/{$this->repository}/commits/main", + [['--jq', '.sha']], + ); + $head = trim($result->output); + if (preg_match('/\A[0-9a-f]{40}\z/', $head) !== 1) { + throw new RuntimeException('Unable to read the head of main'); + } + + return $head; + } + #[Override] public function draft(int $identifier): Recovery { diff --git a/.github/scripts/tests/Unit/Automation/OrchestratorTest.php b/.github/scripts/tests/Unit/Automation/OrchestratorTest.php index 3935f05..e6b8cab 100644 --- a/.github/scripts/tests/Unit/Automation/OrchestratorTest.php +++ b/.github/scripts/tests/Unit/Automation/OrchestratorTest.php @@ -253,6 +253,7 @@ public function test_recovers_merge_cancelled_before_tag_on_next_no_diff_run(): state: 'merged', ), ]); + $repository->method('head')->willReturn($target); $candidate = $this->orchestrator($repository)->recover(); diff --git a/.github/scripts/tests/Unit/Automation/RecoveryTest.php b/.github/scripts/tests/Unit/Automation/RecoveryTest.php index 233eaf1..2ea5bce 100644 --- a/.github/scripts/tests/Unit/Automation/RecoveryTest.php +++ b/.github/scripts/tests/Unit/Automation/RecoveryTest.php @@ -21,7 +21,7 @@ public function test_resumes_draft_after_publish_failure(): void $target = str_repeat('a', 40); $this->assertCandidate( - RecoverySelector::select( + self::select( [new Tag(name: '1.4.5', target: $target)], [ $this->release(), @@ -46,7 +46,7 @@ public function test_resumes_tag_when_draft_creation_failed_and_next_run_has_no_ $target = str_repeat('a', 40); $this->assertCandidate( - RecoverySelector::select( + self::select( [new Tag(name: '1.4.5', target: $target)], [ $this->release( @@ -70,7 +70,7 @@ public function test_resumes_proven_merge_when_cancelled_before_tag_creation(): $target = str_repeat('d', 40); $this->assertCandidate( - RecoverySelector::select( + self::select( [ new Tag( name: '1.4.4', @@ -79,6 +79,7 @@ public function test_resumes_proven_merge_when_cancelled_before_tag_creation(): ], [$this->release(tag: '1.4.4', draft: false)], [$this->merge(number: 76, target: $target)], + head: $target, ), tag: null, target: $target, @@ -92,7 +93,7 @@ public function test_does_not_resume_merge_of_an_untested_base(): void { self::assertSame( null, - RecoverySelector::select( + self::select( [ new Tag( name: '1.4.4', @@ -116,7 +117,7 @@ public function test_fails_closed_for_ambiguous_proven_untagged_merges(): void { $this->expectException(RecoveryException::class); - RecoverySelector::select( + self::select( [ new Tag( name: '1.4.4', @@ -131,9 +132,29 @@ public function test_fails_closed_for_ambiguous_proven_untagged_merges(): void ), $this->merge( number: 77, - target: str_repeat('e', 40), + target: str_repeat('d', 40), ), ], + head: str_repeat('d', 40), + ); + } + + #[Test] + public function test_ignores_an_untagged_merge_main_has_moved_past(): void + { + self::assertSame( + null, + self::select( + [ + new Tag( + name: '1.4.4', + target: str_repeat('a', 40), + ), + ], + [$this->release(tag: '1.4.4', draft: false)], + [$this->merge(number: 76, target: str_repeat('d', 40))], + head: str_repeat('f', 40), + ), ); } @@ -142,7 +163,7 @@ public function test_ignores_unrelated_orphan_tag(): void { self::assertSame( null, - RecoverySelector::select( + self::select( [ new Tag( name: '9.9.9', @@ -162,7 +183,7 @@ public function test_ignores_tag_for_unmarked_or_multi_file_pull_request(): void self::assertSame( null, - RecoverySelector::select( + self::select( [new Tag(name: '1.4.5', target: $target)], [$this->release(tag: '1.4.4', draft: false)], [ @@ -180,7 +201,7 @@ public function test_fails_closed_for_multiple_recoverable_releases(): void $second = str_repeat('b', 40); $this->expectException(RecoveryException::class); - RecoverySelector::select( + self::select( [ new Tag(name: '1.4.5', target: $first), new Tag(name: '1.4.6', target: $second), @@ -199,7 +220,7 @@ public function test_does_not_resume_wrong_target_draft(): void $target = str_repeat('a', 40); $this->expectException(RecoveryException::class); - RecoverySelector::select( + self::select( [new Tag(name: '1.4.5', target: $target)], [ $this->release(tag: '1.4.4', draft: false), @@ -281,4 +302,23 @@ private function assertCandidate( self::assertSame($pull, $candidate->pull); self::assertSame($draft, $candidate->draft); } + + /** + * @param list $tags + * @param list $releases + * @param list $merges + */ + private static function select( + array $tags, + array $releases, + array $merges, + ?string $head = null, + ): ?Candidate { + return RecoverySelector::select( + $tags, + $releases, + $merges, + $head ?? str_repeat('a', 40), + ); + } } diff --git a/CHANGES.md b/CHANGES.md index fb89ce9..f06bfff 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -16,6 +16,7 @@ * `git ls-remote --tags --refs` returns the *annotated tag object*, not the commit it points at — `refs/tags/6.3.0` on phpredis is `aa4302d`, while the commit is `df4fab2`. Resolving references from that output would have replaced correct commit pins with tag-object SHAs. The resolver now reads the peeled `^{}` entry when a tag is annotated and falls back to the object for lightweight tags. * The release step could never succeed. `createDraft` asks GitHub for `generate_release_notes=true`, so the returned body is the automation's body *plus* the generated changelog — and both `assertDraft` and `validateDraft` then required the body to equal what was sent. Every run died at `Draft release is unsafe` after tagging and drafting. Both checks now require the body to *open with* the automation markers, which is what the safety property actually depends on; `RecoverySelector::matches` already worked this way. * `Draft release is unsafe` named none of the six fields it compared, so diagnosing it needed the API and the source side by side. It now says which ones mismatched. +* Recovery treated *any* automation merge without a tag as an unfinished release, and `mergedPullRequests()` paginates the entire closed-PR history — so an abandoned release stayed recoverable forever. A merge whose release was deliberately dropped would be re-tagged and published on the next run, from a commit main had already moved past. An untagged automation merge is now recoverable only while it is still the tip of `main`; once main has moved on, the release was abandoned, not interrupted. * A reference that has drifted from its version is now corrected on the next run even when the version itself is unchanged, so a hand-edited or stale pin self-heals instead of persisting. Every reference is resolved from upstream unconditionally — the peeled commit for git, a fresh hash of the selected tarball for PECL — rather than carrying forward whatever the file already held. A pinned tag that upstream no longer publishes now fails the run instead of passing silently. From 70b825939dca99df31aa2ee7296720ad45c4404c Mon Sep 17 00:00:00 2001 From: Jake Barnby Date: Fri, 21 Aug 2026 17:07:01 +1200 Subject: [PATCH 2/2] (fix): read the head only when recovery needs it Fetching the head of main before every selection gave the tagged recovery path a dependency it never uses, so a failure of that one request would abort recovery of a release that was already tagged and only needed finishing. Recovery is the mechanism that repairs interrupted releases; adding a new way for it to fail is the wrong direction. Resolve the head lazily, on the first untagged automation merge that actually needs comparing. Co-Authored-By: Claude Opus 5 --- .../scripts/src/Automation/Orchestrator.php | 2 +- .../src/Automation/RecoverySelector.php | 28 ++++++++++++------- .../tests/Unit/Automation/RecoveryTest.php | 23 ++++++++++++++- CHANGES.md | 2 +- 4 files changed, 42 insertions(+), 13 deletions(-) diff --git a/.github/scripts/src/Automation/Orchestrator.php b/.github/scripts/src/Automation/Orchestrator.php index a0df20a..12e339b 100644 --- a/.github/scripts/src/Automation/Orchestrator.php +++ b/.github/scripts/src/Automation/Orchestrator.php @@ -45,7 +45,7 @@ public function recover(): ?Candidate $tags, $this->repository->releases($tags), $this->repository->mergedPullRequests(), - $this->repository->head(), + fn (): string => $this->repository->head(), ); } diff --git a/.github/scripts/src/Automation/RecoverySelector.php b/.github/scripts/src/Automation/RecoverySelector.php index 41ae0a8..947a3ef 100644 --- a/.github/scripts/src/Automation/RecoverySelector.php +++ b/.github/scripts/src/Automation/RecoverySelector.php @@ -10,12 +10,13 @@ * @param list $tags * @param list $releases * @param list $merges + * @param callable(): string $head */ public static function select( array $tags, array $releases, array $merges, - string $head, + callable $head, ): ?Candidate { $released = []; $published = []; @@ -83,19 +84,26 @@ public static function select( ); } + $tip = null; foreach ($merges as $merge) { if ( - !isset($targets[$merge->target]) - && $merge->target === $head - && MergeValidator::isAutomation($merge) + isset($targets[$merge->target]) + || !MergeValidator::isAutomation($merge) ) { - $candidates[] = new Candidate( - tag: null, - target: $merge->target, - pull: $merge->number, - draft: null, - ); + continue; + } + + $tip ??= $head(); + if ($merge->target !== $tip) { + continue; } + + $candidates[] = new Candidate( + tag: null, + target: $merge->target, + pull: $merge->number, + draft: null, + ); } $unique = []; diff --git a/.github/scripts/tests/Unit/Automation/RecoveryTest.php b/.github/scripts/tests/Unit/Automation/RecoveryTest.php index 2ea5bce..486840d 100644 --- a/.github/scripts/tests/Unit/Automation/RecoveryTest.php +++ b/.github/scripts/tests/Unit/Automation/RecoveryTest.php @@ -139,6 +139,27 @@ public function test_fails_closed_for_ambiguous_proven_untagged_merges(): void ); } + #[Test] + public function test_does_not_read_the_head_for_tagged_recovery(): void + { + $target = str_repeat('a', 40); + + $candidate = RecoverySelector::select( + [new Tag(name: '1.4.5', target: $target)], + [ + $this->release(), + $this->release(identifier: 9, tag: '1.4.4', draft: false), + ], + [$this->merge()], + static fn (): string => throw new RecoveryException( + 'head must not be read for tagged recovery', + ), + ); + + self::assertNotNull($candidate); + self::assertSame('1.4.5', $candidate->tag); + } + #[Test] public function test_ignores_an_untagged_merge_main_has_moved_past(): void { @@ -318,7 +339,7 @@ private static function select( $tags, $releases, $merges, - $head ?? str_repeat('a', 40), + static fn (): string => $head ?? str_repeat('a', 40), ); } } diff --git a/CHANGES.md b/CHANGES.md index f06bfff..201debf 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -16,7 +16,7 @@ * `git ls-remote --tags --refs` returns the *annotated tag object*, not the commit it points at — `refs/tags/6.3.0` on phpredis is `aa4302d`, while the commit is `df4fab2`. Resolving references from that output would have replaced correct commit pins with tag-object SHAs. The resolver now reads the peeled `^{}` entry when a tag is annotated and falls back to the object for lightweight tags. * The release step could never succeed. `createDraft` asks GitHub for `generate_release_notes=true`, so the returned body is the automation's body *plus* the generated changelog — and both `assertDraft` and `validateDraft` then required the body to equal what was sent. Every run died at `Draft release is unsafe` after tagging and drafting. Both checks now require the body to *open with* the automation markers, which is what the safety property actually depends on; `RecoverySelector::matches` already worked this way. * `Draft release is unsafe` named none of the six fields it compared, so diagnosing it needed the API and the source side by side. It now says which ones mismatched. -* Recovery treated *any* automation merge without a tag as an unfinished release, and `mergedPullRequests()` paginates the entire closed-PR history — so an abandoned release stayed recoverable forever. A merge whose release was deliberately dropped would be re-tagged and published on the next run, from a commit main had already moved past. An untagged automation merge is now recoverable only while it is still the tip of `main`; once main has moved on, the release was abandoned, not interrupted. +* Recovery treated *any* automation merge without a tag as an unfinished release, and `mergedPullRequests()` paginates the entire closed-PR history — so an abandoned release stayed recoverable forever. A merge whose release was deliberately dropped would be re-tagged and published on the next run, from a commit main had already moved past. An untagged automation merge is now recoverable only while it is still the tip of `main`; once main has moved on, the release was abandoned, not interrupted. The head lookup is lazy, so recovering an already-tagged release never depends on it. * A reference that has drifted from its version is now corrected on the next run even when the version itself is unchanged, so a hand-edited or stale pin self-heals instead of persisting. Every reference is resolved from upstream unconditionally — the peeled commit for git, a fresh hash of the selected tarball for PECL — rather than carrying forward whatever the file already held. A pinned tag that upstream no longer publishes now fails the run instead of passing silently.