Skip to content

(fix): bound untagged recovery to the head of main#92

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
abnegate merged 2 commits into from
Aug 21, 2026
Merged

(fix): bound untagged recovery to the head of main #92

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e1c92f0
(fix): bound untagged recovery to the head of main
abnegate Aug 21, 2026
70b8259
(fix): read the head only when recovery needs it
abnegate Aug 21, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/scripts/src/Automation/Orchestrator.php
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -45,6 +45,7 @@ public function recover(): ?Candidate
$tags,
$this->repository->releases($tags),
$this->repository->mergedPullRequests(),
fn (): string => $this->repository->head(),
);
}

Expand Down
26 changes: 18 additions & 8 deletions .github/scripts/src/Automation/RecoverySelector.php
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -10,11 +10,13 @@
* @param list<Tag> $tags
* @param list<Recovery> $releases
* @param list<Merge> $merges
* @param callable(): string $head
*/
public static function select(
array $tags,
array $releases,
array $merges,
callable $head,
): ?Candidate {
$released = [];
$published = [];
Expand DownExpand Up@@ -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();

@greptile-appsgreptile-appsBotAug 21, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1Tagged recovery still reads head

When a valid tagged candidate coexists with an eligible untagged automation merge, this loop invokes head() before returning either candidate. A failure of that additional GitHub request therefore aborts recovery of the already-tagged release.

Prompt To Fix With AI
This is a comment left during a code review.
Path: .github/scripts/src/Automation/RecoverySelector.php
Line: 96
Comment:
**Tagged recovery still reads head**
When a valid tagged candidate coexists with an eligible untagged automation merge, this loop invokes `head()` before returning either candidate. A failure of that additional GitHub request therefore aborts recovery of the already-tagged release.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Fix in Claude CodeFix in Codex

@abnegateabnegateAug 21, 2026

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refuting this one, with the caveat that the underlying concern is fair — I just do not think it identifies a new failure mode.

The claim is that a failed head() can abort an otherwise valid tagged recovery. That is true, but recover() already depends on three unconditional API requests, each of which aborts tagged recovery the same way if it fails:

$tags = $this->repository->tags(); // paginated API callreturn RecoverySelector::select(
$tags,
$this->repository->releases($tags), // paginated API call$this->repository->mergedPullRequests(), // paginated API callfn (): string => $this->repository->head(), // only invoked if an untagged automation merge exists
);

If releases() throws, the tagged candidate is lost in exactly the same way. Recovery has never been resilient to API failure and is not designed to be — every adapter method throws, the step fails, and the next scheduled or dispatched run retries from scratch. head() is now the most conditional of the four, not an outlier.

On the specific interleaving: when a tagged candidate and an eligible untagged merge coexist and head()succeeds, the untagged merge is either the tip — giving two candidates and a deliberate RecoveryException — or it is not, and it is filtered out. So the only cost of a failed head() is one retry of a run that would otherwise have proceeded.

I also considered making a head() failure non-fatal by skipping untagged candidates, and rejected it: that would suppress genuine ambiguity detection and let a tagged recovery proceed while an untagged one also existed. Failing closed is the safer behaviour and is what the code does now.

Happy to reconsider if you are pointing at something narrower that I have missed — in particular if there is a case where head() is invoked and no untagged automation merge exists, since that would be a real regression and is what the laziness change was meant to rule out.

@greptile-appsgreptile-appsBotAug 21, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your rebuttal is correct on all counts. The ??= ensures head() is only invoked when the loop body actually reaches an untagged automation merge — if no such merge exists, $tip remains null and head() is never called. So the concern I raised (that a failing head() could abort an already-valid tagged recovery) only materialises when an untagged automation merge exists alongside the tagged candidate, which is precisely the ambiguous state where failing closed is the right call anyway.

The symmetry argument is also solid: head() is now the most conditional of the four calls, not an outlier. I was incorrect to frame it as a new failure mode. Withdrawing the comment.

Tip: You can customize Greptile's behavior for this repo with .greptile/rules.md and .greptile/config.json.

if ($merge->target !== $tip) {
continue;
}

$candidates[] = new Candidate(
tag: null,
target: $merge->target,
pull: $merge->number,
draft: null,
);
}

$unique = [];
Expand Down
2 changes: 2 additions & 0 deletions .github/scripts/src/Automation/Repository.php
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -6,6 +6,8 @@

interface Repository
{
public function head(): string;

/**
* @return list<Tag>
*/
Expand Down
16 changes: 16 additions & 0 deletions .github/scripts/src/Automation/Repository/GitHub.php
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -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
{
Expand Down
1 change: 1 addition & 0 deletions .github/scripts/tests/Unit/Automation/OrchestratorTest.php
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -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();

Expand Down
81 changes: 71 additions & 10 deletions .github/scripts/tests/Unit/Automation/RecoveryTest.php
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -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(),
Expand All@@ -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(
Expand All@@ -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',
Expand All@@ -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,
Expand All@@ -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',
Expand All@@ -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',
Expand All@@ -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),
),
);
}

Expand All@@ -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',
Expand All@@ -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)],
[
Expand All@@ -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),
Expand All@@ -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),
Expand DownExpand Up@@ -281,4 +323,23 @@ private function assertCandidate(
self::assertSame($pull, $candidate->pull);
self::assertSame($draft, $candidate->draft);
}

/**
* @param list<Tag> $tags
* @param list<Recovery> $releases
* @param list<Merge> $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),
);
}
}
1 change: 1 addition & 0 deletions CHANGES.md
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -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 <id> 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 <id> 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.

Expand Down
Loading