diff --git a/.github/scripts/src/Automation/Orchestrator.php b/.github/scripts/src/Automation/Orchestrator.php index 8b1129a..12e339b 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(), + fn (): string => $this->repository->head(), ); } diff --git a/.github/scripts/src/Automation/RecoverySelector.php b/.github/scripts/src/Automation/RecoverySelector.php index 66f3daf..947a3ef 100644 --- a/.github/scripts/src/Automation/RecoverySelector.php +++ b/.github/scripts/src/Automation/RecoverySelector.php @@ -10,11 +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, + callable $head, ): ?Candidate { $released = []; $published = []; @@ -82,18 +84,26 @@ public static function select( ); } + $tip = null; foreach ($merges as $merge) { if ( - !isset($targets[$merge->target]) - && 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/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..486840d 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,50 @@ 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_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 + { + 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 +184,7 @@ public function test_ignores_unrelated_orphan_tag(): void { self::assertSame( null, - RecoverySelector::select( + self::select( [ new Tag( name: '9.9.9', @@ -162,7 +204,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 +222,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 +241,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 +323,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, + static fn (): string => $head ?? str_repeat('a', 40), + ); + } } diff --git a/CHANGES.md b/CHANGES.md index fb89ce9..201debf 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. 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.