Skip to content

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

Merged
abnegate merged 2 commits into
mainfrom
fix/bound-recovery-to-head
Aug 21, 2026
Merged

(fix): bound untagged recovery to the head of main#92
abnegate merged 2 commits into
mainfrom
fix/bound-recovery-to-head

Conversation

@abnegate

Copy link
Copy Markdown
Member

Explains run 32448110295, which recovered a release that had been deliberately abandoned, and blocks the retry of the dependency release.

The defect

RecoverySelector::select treats any automation merge with no tag pointing at it as an interrupted release:

foreach ($mergesas$merge) {
if (!isset($targets[$merge->target]) && MergeValidator::isAutomation($merge)) {
$candidates[] = newCandidate(tag: null, target: $merge->target, ...);
}
}

There is no bound on how old that merge may be, and mergedPullRequests() paginates the entire closed-PR history (state=closed&base=main, all pages). So an automation merge that never got a tag stays a recovery candidate forever.

That is what happened: #89 was merged by the automation, its release failed, and the tag and draft were deleted. The merge remained, untagged, so the next dispatch read it as unfinished and began re-tagging 8a93696.

Why this blocks the retry specifically

After #91 reverts #89, commit 8a93696 is still in history and still untagged. Every subsequent run would re-tag it and publish an image built from the commit whose changes were just reverted — quietly undoing the point of the revert.

The rule

Recovery is warranted in exactly one window: between the automation merge landing and its tag being created. In that window the merge is still the tip of main. Once main has moved on, the release was abandoned rather than interrupted, and resurrecting it is wrong.

An untagged automation merge is therefore recoverable only while merge->target === head-of-main. The tagged path is untouched — a tag that exists but was never released still recovers as before, regardless of age.

This required a new Repository::head(); the GitHub adapter reads repos/{repo}/commits/main.

Verification

  • test_ignores_an_untagged_merge_main_has_moved_past is the regression test. Confirmed red with the bound removed and green with it.
  • Two existing parity contracts covered the untagged path and were adapted rather than dropped:
    • test_resumes_proven_merge_when_cancelled_before_tag_creation now passes the merge target as head, preserving its intent — an interrupted release at the tip still recovers.
    • test_fails_closed_for_ambiguous_proven_untagged_merges previously used two merges with different targets, which the tip rule makes structurally impossible. It now uses two automation merges claiming the same tip commit, which still exercises fail-closed on ambiguity under the new rule.
  • composer verify: Pint, PHPStan max, 153 tests / 1812 assertions, 91 parity contracts.

Not verified

  • No live run has exercised this. Repository::head() is new and its GitHub call is only covered by a mock.
  • The tip rule fails closed in one scenario worth naming: if a human merges anything to main in the seconds between the automation merge and its tag, the automation merge stops being the tip and its genuinely-interrupted release becomes unrecoverable. It would then need finishing by hand. The workflow's concurrency group prevents concurrent automation runs but cannot prevent a human merge.

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 <noreply@anthropic.com>
@abnegate
abnegateforce-pushed the fix/bound-recovery-to-head branch from 562c9f6 to e1c92f0CompareAugust 21, 2026 05:00
@abnegate

Copy link
Copy Markdown
MemberAuthor

@greptileai review

@greptile-apps

greptile-appsBot commented Aug 21, 2026

Copy link
Copy Markdown

Greptile Summary

The PR limits untagged recovery to automation merges at the current tip of main by adding a lazy repository-head lookup. The lookup remains reachable during tagged recovery when an eligible untagged merge coexists, so failure of that request can still block the tagged recovery.

  • Adds Repository::head() and its GitHub implementation.
  • Filters untagged recovery candidates against the head of main.
  • Adds regression and parity coverage for tip-bounded recovery.

Confidence Score: 4/5

The PR is not yet safe to merge because an otherwise valid tagged recovery can still be aborted by failure of the new head lookup.

Recovery selection continues into untagged-merge processing after collecting a tagged candidate, so a coexisting eligible untagged merge invokes Repository::head() and can lose the tagged recovery if that request fails.

Files Needing Attention: .github/scripts/src/Automation/RecoverySelector.php and .github/scripts/tests/Unit/Automation/RecoveryTest.php

Important Files Changed

FilenameOverview
.github/scripts/src/Automation/RecoverySelector.phpAdds tip filtering for untagged merges, but still invokes the head callback when a tagged candidate coexists with an eligible untagged merge.
.github/scripts/src/Automation/Orchestrator.phpPasses the repository head lookup lazily into recovery selection, avoiding eager evaluation but not all tagged-recovery dependencies.
.github/scripts/src/Automation/Repository/GitHub.phpAdds a validated GitHub API lookup for the SHA at the head of main.
.github/scripts/tests/Unit/Automation/RecoveryTest.phpCovers ordinary tagged and untagged paths, but the tagged-recovery test bypasses the callback through a matching merge target.
.github/scripts/src/Automation/Repository.phpExtends the repository abstraction with the head lookup required for tip-bounded recovery.

Fix all with GreploopFix All in Claude CodeFix All in Codex

Prompt To Fix All With AI
### Issue 1
.github/scripts/src/Automation/RecoverySelector.php:96
**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.

Reviews (2): Last reviewed commit: "(fix): read the head only when recovery ..." | Re-trigger Greptile

Comment thread.github/scripts/src/Automation/Orchestrator.php Outdated
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 <noreply@anthropic.com>
@abnegate

Copy link
Copy Markdown
MemberAuthor

Greptile P2 (unnecessary head lookup dependency) — fixed in 70b8259. The thread auto-resolved when the push moved its anchor, so recording the reasoning here.

Agreed, and it mattered more than a nit in this spot. recover() is the mechanism that repairs interrupted releases, and I had given its tagged path a dependency it never uses. A transient failure of the head request would have aborted recovery of a release that was already tagged and only needed finishing — exactly the situation recovery exists to handle. Adding a new way for the repair mechanism to fail is the wrong direction.

The head is now resolved lazily, on the first untagged automation merge that actually needs comparing. RecoverySelector::select takes a callable(): string rather than a resolved string, so the tagged path never invokes it and no API call is made unless an untagged automation merge exists.

Verification:

  • test_does_not_read_the_head_for_tagged_recovery passes a resolver that throws if called and asserts tagged recovery still returns its candidate. Confirmed it errors when the lookup is made eager again, and passes with the lazy version.
  • test_ignores_an_untagged_merge_main_has_moved_past still confirmed red with the tip bound removed.
  • composer verify: PHPStan max, 156 tests / 1818 assertions, 91 parity contracts.

Note this PR was rebased onto current main earlier (after #90 and #91 merged) to resolve a CHANGES.md conflict; both changelog entries were kept.

continue;
}

$tip ??= $head();

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

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.

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.

@abnegate
abnegate merged commit 3a654ca into mainAug 21, 2026
12 checks passed
@abnegate
abnegate deleted the fix/bound-recovery-to-head branch August 21, 2026 05:15
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@abnegate