[Danathar]

Summary

The hold guard keeps held PRs mergeable by merging their base in (#10437). On every hive running the default convergence.mode (shadow), it could make that update once per PR and then never again. After that, it logged hold guard: could not confirm new head after keeping held PR mergeable every pass, and the branch stayed behind.

Cause

maintainHeldPRMergeability called UpdateBranch, whose mutation-journal identity is only repo + branch_update + PR number: no head, no base. Once the first update was journaled as applied, every later update for that PR replayed as "external mutation already applied; treating as idempotent success". GitHub was never called, so the head never moved, and the guard warned and tried again the next pass.

Live evidence (hive on a08007f, then fcf0e78):

• fix: scan printer apps for known security flaws projectbluefin/fsdk-containers#474 (fork PR):
  • 11:10Z: the guard's update landed as fc68f8a, committed at 11:10:05Z. It still warned at 11:10:07Z, because it read the commit list before the merge showed up. This is the race described below.
  • 12:41Z: main moved, with no workflow-file change.
  • 12:46Z to 17:45Z: 21 more WARNs. Each came about half a second after external mutation already applied; treating as idempotent success … kind=branch_update target=474. There was no new commit, and the PR merged at 17:56Z.
• chore(deps): update taiki-e/install-action digest to 5488ac5 projectbluefin/ghostscript-printer-app#152 and chore(deps): update taiki-e/install-action digest to 5488ac5 projectbluefin/ps-printer-app#136 (same-repo Renovate branches): the smaller race on its own. GitHub answered 202, and its merge commits are stamped 18:34:44Z and 18:34:48Z. The guard read the commit list before they showed up and warned at 18:34:46Z and 18:34:50Z. The update landed, but because it was journaled as applied, those PRs would have hit the first problem the next time main moved.

Causes ruled out for these PRs

• Workflows permission: no real request was made after 11:10 on fix: unify tooltip font and size across dashboard #474, and main's only change in that window touched no workflow file. The 403 workflows case is already handled since 🐛 fix(automerge): park workflows-permission branch-update refusals #14183.
• Fork without push access: the App pushed to fix: unify tooltip font and size across dashboard #474's fork at 11:10.

Both still get a stop here, because they are refusals the same token would get again.

Change

Journal identity

• The guard now calls UpdateBranchExpectedHead with the PR's head and base SHA. Each (head, base) is its own journaled attempt, the same way PR follow-up and supersession already call it. The update is also pinned to the head the guard saw.
• Held PRs now carry BaseSHA from enumeration, as actionable PRs already did.

Accepted (202) but not visible yet

• The attempt is recorded as requested (Info, not WARN) and waited on, not re-sent.
• When the head later moves, the request settles. The baseline is not advanced, because between ticks the new head is not provably the hive's own merge. The guard has already recorded that head as seen while held (🐛 [v6] hold guard re-holds a PR that a maintainer reviewed at its current head, because a rebase happened earlier during the hold #13603), so the lift check judges it.
• If the head has not moved after holdguard.UpdateConfirmWindow (30 minutes), the guard stops with reason update-not-landed.

Refusals

• Refusals are classified with the automerge sweep's existing classifier, moved to pkg/github (ClassifyBranchUpdateError). The automerge sweep keeps using it through aliases, unchanged.
• These stop attempts for that head and base until either moves:
  • workflow-permission
  • fork-not-editable: also decided up front, with no call, when "allow edits by maintainers" is off
  • conflict
  • up-to-date
• The reason is stored on the record (UpdateStopped*) and logged with the head and base.
• Transient failures (5xx, rate limit, a fenced journal entry) are still retried on the next pass.

Person-facing note

• When the guard stops for workflow-permission, fork-not-editable or update-not-landed, it posts one comment per head and reason. The comment names the reason and says to use Update branch; for a fork, it says to ask the author.
• A conflict keeps the existing conflict notice.
• A stale behind that GitHub calls up to date gets no note.

Returned to contributor (072)

• The guard no longer merges the base into a PR at stage/072-returned-to-contributor. That stage means the author's hive is expected to push to the branch. A merge commit pushed under them only makes their next push fail until they pull, and the PR cannot merge before they act anyway.
• A PR the hive adopted (074) is still kept up to date.

Docs citation

client.go gains one line (BaseSHA), which moves RankActionableIssues down a line, so src/docs/scale-envelope.md:37 now cites client.go:4712-4722.

Legacy records

• Records written before this change with WorkflowRefusedHeadSHA still stop on that head. The field is no longer written.

Testing

Tests run from src/ under /var/tmp, with -count=1:

• go test ./pkg/holdguard/ ./pkg/github/... ./pkg/convergence/... ./cmd/hive/: all ok (holdguard, github, automerge, requestwatch, the convergence packages and cmd/hive).
• go vet ./cmd/hive/ is clean, and gofmt is clean on the changed files.
• src/scripts/check-changelog-fragment.sh v6, check-docs-citations.py and check-docs-links.py pass.
• pkg/holdguard coverage is 99.6% on its own tests (the gate's floor is 90%; it was 75.3% when the new methods were only tested from cmd/hive).

New tests in cmd/hive/hold_guard_update_retry_test.go:

Test	Covers
…AsksGitHubAgainAfterAJournaledUpdate	The #474 cause, with the journal a hive runs by default (shadow mode). After a first update, a new head and base reaches GitHub again and advances the baseline.
…WaitsOnAnAcceptedUpdate	A slow 202: no re-request inside the window. After the window, a not-landed stop with one note and no further calls. A moved base tries again.
…SettlesAnUpdateThatLandsLater	The update lands between ticks: the request settles and the baseline is not advanced.
…StopsOnAForkItCannotPushTo	Fork with maintainer edits off (no call), a 422 maintainer-edits refusal, and a 403 on a fork: each stops with one note.
…RetriesRefusedHeadOnlyAfterHeadOrBaseMoves	A refused head is not asked again until main or the head moves. A 502 is retried each pass.
…HonoursLegacyWorkflowRefusal	Old records still stop on their head.
…LeavesAReturnedPRAlone	072 is not updated.
TestEnforceHoldGuardSettledUpdateIsNotDriftAtLift	An update that lands between ticks, then a human lift: no drift note when the hive saw the merge while held; a re-hold when the lift came first.

New tests in pkg/holdguard/update_state_test.go: an awaited update is matched on its own head and base, expires after UpdateConfirmWindow, and settles once the head moves without moving the baseline. AdvanceHygiene clears it. A stop holds for one head and base and survives a reload, and the legacy workflows record still stops. The note is per head and reason, and the comment names each reason and what to do.

Changed existing tests in hold_guard_mergeability_test.go:

• Workflows refusal test: it expected no comment. It now expects one note naming the workflows permission and Update branch. It also checks that a moved base retries without a second note for the same head.
• Conflict test: it retried the same head and base after a conflict. It now asserts that the same head and base are not asked again, and resolves the conflict through a base move.

Mutation checks

Each part of the fix was broken in turn and restored from a backup, and each break failed the expected tests:

• Old no-input UpdateBranch: the journal test fails.

• No wait on an accepted update: the 202 test fails.

• No up-front fork check: the fork test fails.

• No 072 skip: the 072 test fails.

• No stop check: five tests fail.

• No note dedupe: three tests fail.

• pkg/holdguard, one break at a time: pending ignores the base; settle on the same head; a stop keeps the request; a stop ignores the base; the note ignores the reason. Each fails one of the new package tests.

• No observed-held-head step (store.Touch in the snapshot step): the settled-update lift test fails.

Review question: is a settled update drift at lift time?

The hive's review asked whether the merge commit from an update that lands between ticks, which SettleUpdateRequest leaves out of the baseline's commit set, reads as new commits when a person lifts the hold. It does not, as long as the hive saw it while the PR was held. Each tick, the snapshot step records the held head (store.Touch, ObservedHeldHeadSHA) before the mergeability step settles the request. At lift, LiftCoversObservedHead accepts that head (#13603), and the commit set is only used to describe drift that has already been found. If the lift lands before the hive sees the new head, the PR is re-held with a drift note. This PR does not change that case, and it errs toward a fresh review. TestEnforceHoldGuardSettledUpdateIsNotDriftAtLift covers both orders.

Not verified

• No live hive. Verified against the fake GitHub only.
• base.sha freshness. The retry-on-main-moved rule relies on base.sha in the pulls list changing when main moves. PR follow-up already relies on the same field for the same purpose. I have not confirmed how quickly GitHub refreshes it in the list response.
• Fork refusal text. The 422 and 403 fork-refusal messages in the tests follow the automerge classifier's patterns. I have not captured them live.
• Merge-request watcher. pkg/github/merge_request_watcher.go still calls the no-input UpdateBranch for merge requests with update_branch, so a second request on the same PR would replay the first update the same way. Left out of this PR, because pinning it to the requested head changes that path's behaviour.

Contributor checklist

• Changelog: changelog.d/fixed-hold-guard-branch-update-retries.md.
• Targets v6.
• The title uses the emoji convention.
• Commits include DCO sign-off.
• No docs describe this path.
• No secrets, credentials or local runtime state are committed.

🤖 Generated with Claude Code

https://claude.ai/code/session_01BQYnPGFJx81c8JXR3o1sg1

[kubestellar-prow]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign danathar for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[hivecommons-hive]

The diagnosis holds and the fix looks correct to me; one open question for the author, no defects found.

Verified on this head (d059167):

• Root cause is real. UpdateBranch calls updateBranch(ctx, repo, number, "", "") (src/pkg/github/pullrequest.go:559-561), which leaves inputs == nil (pullrequest.go:580-583), and the mutation journal's logical ID is derived from the claim's Inputs (src/pkg/convergence/mutation/boundary.go:299-316, journal.go:175). So the identity really was repo + branch_update + PR number, and every later update for that PR replayed as applied. UpdateBranchExpectedHead adds expected_head_sha/observed_base_sha to the inputs, which gives one journal entry per (head, base) as the body describes.
• UpdateBranchExpectedHead and PullRequest.BaseSHA already exist on v6 (pullrequest.go:568, client.go:986); this PR only starts populating BaseSHA in fetchPRsWithDetails. go build ./cmd/hive/ ./pkg/github/... ./pkg/holdguard/ passes on this head.
• SettleUpdateRequest runs before the behind|dirty filter (main_helpers.go:305-308), so a 202 that lands between ticks is cleared even when the PR is no longer behind; UpdatePending cannot then re-match because the head differs.
• ClassifyBranchUpdateError is a verbatim move out of automerge_sweep.go (type alias + const aliases keep the sweep's call sites unchanged), so the sweep's behaviour is not altered.
• WorkflowRefusedHeadSHA written by older builds is still honoured by UpdateStoppedFor (holdguard.go:376-378), so no retry storm on upgrade.

Open question (not a defect): when an accepted update lands between ticks, SettleUpdateRequest clears the request but deliberately does not AdvanceHygiene (holdguard.go:441-447), so the hive's own merge commit is never added to the baseline CommitSHAs. The lift check is covered by ObservedHeldHeadSHA as the comment says — but does the snapshot/drift step treat that App-authored merge commit as new commits on a held PR and post the drift notice? If drift is already keyed off ObservedHeldHeadSHA rather than CommitSHAs, ignore this; if not, the 202 race turns into a spurious drift comment instead of a spurious WARN. I did not trace the snapshot step far enough to settle it.

— hive: agent=reviewer-2 backend=copilot model=claude-fable-5.1 copilot=1.0.88

[hivecommons-hive]

@Danathar this PR is back in your court (stage/072-returned-to-contributor). The hive will not merge it until the head changes.

Failing checks:

• check-failure

Reply here or push a new head to this branch (same sign-off); CI and the review swarm re-run automatically and the PR returns to the normal path.

Your relay session received this as a fix-pr task; your current task is parked (claim kept alive) until this PR is green.

[hivecommons-hive]

The only red required check, relative links and anchors resolve, is PR-local: this PR adds lines to src/pkg/github/client.go above RankActionableIssues, so the line-cited anchor in the docs drifted:

DRIFT src/docs/scale-envelope.md:37 src/pkg/github/client.go:4707-4717 -> lacks anchor(s) near cited range: AgeMinutes

On head d059167 the function now spans client.go:4712-4722 (sort by AgeMinutes at 4720). Fix in one line of src/docs/scale-envelope.md:37:

-     (`RankActionableIssues`, `src/pkg/github/client.go:4707-4717`, descending `AgeMinutes`).
+     (`RankActionableIssues`, `src/pkg/github/client.go:4712-4722`, descending `AgeMinutes`).

(The branch lives in your fork, so the hive cannot push this for you.)

---

🐝 Hive Agent: scanner | Instance: hosted-available-oke-11-placeholder-r05x | SHA: unknown

— hive: agent=scanner backend=copilot model=claude-fable-5.1 copilot=1.0.88

[github-actions]

Open PR overlap check

Base branch: v6

Conflicts are checked with every PR merged onto the current tip of v6, so a PR that only needs a rebase is not reported as conflicting with its siblings.

PR	Title	Relation	Files
#14399	🌱 chore: forward-merge v5 → v6 (slice 1 of 2: through 25dd844, before the pkg/agent→crux/agent rename)	CONFLICT	src/docs/scale-envelope.md

[hivecommons-hive]

hive/ci-report (head d059167): the coverage check is PR-local — the gate reports holdguard: 75.3% (floor 90%) (job 121788688554); coverage is not red on v5.

Cause: the PR adds ~10 exported/unexported funcs to src/pkg/holdguard/holdguard.go (UpdateStoppedFor, UpdateStopNoted, MarkUpdateRequested, UpdatePending, SettleUpdateRequest, MarkUpdateStopped, MarkUpdateStopNoted, UpdateStoppedCommentBody, clearUpdateRequested) but all new tests live in src/cmd/hive/hold_guard_*_test.go. The gate scores per package without -coverpkg, so coverage exercised from cmd/hive does not count toward pkg/holdguard.

Fix: add table tests in src/pkg/holdguard/holdguard_test.go covering those functions directly — including the no-op paths (empty head, untracked PR), the legacy WorkflowRefusedHeadSHA branch of UpdateStoppedFor, UpdatePending before/after UpdateConfirmWindow, SettleUpdateRequest with a moved vs unmoved head, and each UpdateStop* reason through UpdateStoppedCommentBody. That should clear the 90% floor without touching the cmd/hive tests.

---

🐝 Hive Agent: scanner | Instance: hosted-available-oke-11-placeholder-r05x | SHA: unknown

— hive: agent=scanner backend=copilot model=claude-fable-5.1 copilot=1.0.88

[hivecommons-hive]

hive/check-report: relative links and anchors resolve fails on this PR's own diff (job 121784248884):

DRIFT src/docs/scale-envelope.md:37 src/pkg/github/client.go:4707-4717 -> src/pkg/github/client.go lacks anchor(s) near cited range: AgeMinutes

The one-line insertion in src/pkg/github/client.go shifts RankActionableIssues; on this branch the function spans 4712–4720 (func RankActionableIssues at 4712, the AgeMinutes comparator at 4720), so the cited range no longer covers the anchor. Fix: in src/docs/scale-envelope.md:37 change src/pkg/github/client.go:4707-4717 → src/pkg/github/client.go:4712-4720 (re-run bash src/scripts/check-doc-citations.sh or whichever citation checker the relative links and anchors resolve job calls to confirm). The coverage job on the same run fails fast at the shard-artifact stage and should be re-read after the next push.

---

🐝 Hive Agent: scanner | Instance: hosted-available-oke-11-placeholder-r05x | SHA: b05e7ba6f

— hive: agent=scanner backend=copilot model=claude-fable-5.1 copilot=1.0.88

[Danathar]

Pushed a13a0c6, on top of a merge of current v6:

• Docs citation: src/docs/scale-envelope.md:37 now cites client.go:4712-4722. check-docs-citations.py passes.
• Coverage: new pkg/holdguard/update_state_test.go tests the update request and stop methods inside the package. Coverage is 99.6%, against the 90% floor.
• Review question (settled update vs. drift at lift): no false drift note when the hive saw the merge while the PR was held. Each tick the snapshot step records the held head (store.Touch) before the request settles, and LiftCoversObservedHead accepts that head at lift (🐛 [v6] hold guard re-holds a PR that a maintainer reviewed at its current head, because a rebase happened earlier during the hold #13603). A lift before the hive sees the merge still re-holds, as it did before this PR. TestEnforceHoldGuardSettledUpdateIsNotDriftAtLift covers both orders. Details are in the PR description.

[hivecommons-hive]

Reviewed — no findings from this perspective.

— hive: agent=reviewer backend=copilot model=claude-fable-5.1 copilot=1.0.88

[hivecommons-hive]

Hive filed review findings that do not block this PR as backlog items:

• Review backlog (intent-alignment): Legacy WorkflowRefusedHeadSHA records stop on head alone, ignoring base moves #14533 — Review backlog (intent-alignment): Legacy WorkflowRefusedHeadSHA records stop on head alone, ignoring base moves (Review backlog (intent-alignment): Legacy WorkflowRefusedHeadSHA records stop on head alone, ignoring base moves #14533)

Cap: 10 backlog item(s) per PR per day.

[hivecommons-hive]

Reviewed — no findings from this perspective.

— hive: agent=ci-maintainer backend=copilot model=claude-fable-5.1 copilot=1.0.88

[hivecommons-hive]

Correctness review (read at a13a0c6, plus pkg/github/pullrequest.go, client.go enrichment, and the lift path in cmd/hive/main_helpers.go). No blocker; two small in-scope notes.

1. Stale expected_head_sha 422 is recorded as a "conflict". The guard now pins the update to the enumerated head (src/cmd/hive/main_helpers.go:328, via UpdateBranchExpectedHead → PullRequestBranchUpdateOptions{ExpectedHeadSHA} at src/pkg/github/pullrequest.go:594-596). When the author pushes between enumeration and the call, GitHub answers 422 "expected head sha didn't match current head ref"; ClassifyBranchUpdateError has no case for that text and its 422 fallthrough returns BranchUpdateConflict (src/pkg/github/branch_update_refusal.go:55). The guard then MarkUpdateStopped(..., "conflict"), logs a WARN "stopped keeping held PR mergeable" with reason=conflict (main_helpers.go:374-380), and on a dirty PR posts ConflictCommentBody (main_helpers.go:398-407). It self-heals next tick because the head moved, but the stop/log/comment are for a race, not a conflict. The old call had no expected head, so this path is new. Suggested fix in the shared classifier (the sweep also passes a head, so it benefits too):

	case http.StatusUnprocessableEntity:
		switch {
		case strings.Contains(msg, "expected head sha"):
			// The head moved under us; the next pass sees the new head.
			return BranchUpdateOther
		case strings.Contains(msg, "maintainer") || strings.Contains(msg, "permission") || strings.Contains(msg, "not authorized"):

(src/pkg/github/branch_update_refusal.go:47-48; BranchUpdateOther lands in the guard's transient branch at main_helpers.go:339-345, which only logs.)

2. Legacy records now get a comment. UpdateStoppedFor maps an old WorkflowRefusedHeadSHA to UpdateStopWorkflowPermission (src/pkg/holdguard/holdguard.go:383-385), and the stopped path calls noteHeldPRUpdateStopped every pass (main_helpers.go:314-316), so every pre-upgrade refused PR still behind will receive one UpdateStoppedCommentBody on the first pass after deploy. Reasonable, but the PR body's "Legacy records" section says only that they still stop — worth a line there so nobody reads the burst of notes as a bug.

Checked and fine: actionable.PRs.Held is enriched through EnrichCIStatus → enrichPRCI (client.go:2477-2492), so the up-front FromFork && !MaintainerCanModify stop reads the full-PR value, not the list endpoint's absent field. The settled-update/no-baseline-advance behaviour matches what the old code already did in the 202 race (warn, never advance), so lifts by a bot/hive actor behave as before.

— hive: agent=adjudicator backend=copilot model=claude-fable-5.1 copilot=1.0.88

[hivecommons-hive]

Reviewed (style/maintainability) — no blockers; one advisory.

ClassifyBranchUpdateError now lives in src/pkg/github/branch_update_refusal.go:32-58, but its only table test is still TestClassifyUpdateBranchError in src/pkg/github/automerge/automerge_update_branch_error_test.go:21, which exercises the one-line forwarder classifyUpdateBranchError (automerge_sweep.go:2473-2475) and the const aliases. Tests normally sit beside the code they cover, and the pkg/github floor in .github/workflows/v2-tests.yml:1632 is scored without -coverpkg, so the moved body counts as uncovered there. Suggest moving (or copying) the table into src/pkg/github/branch_update_refusal_test.go against BranchUpdate*, and then either dropping the automerge aliases (single call site at automerge_sweep.go:2438) or leaving them — your call; the alias layer is fine as-is if you prefer the smaller diff.

— hive: agent=reviewer-2 backend=copilot model=claude-fable-5.1 copilot=1.0.88

[hivecommons-hive]

Reviewed — no findings from this perspective.

— hive: agent=adjudicator backend=copilot model=claude-fable-5.1 copilot=1.0.88

[hivecommons-hive]

Evidence bundle `#14401a13a0c6` sha256:ddfa0bfb7e9e4a6745f06c356736d250561986447fcd8384724cbceb2ccb663c — download from the dashboard or `hivectl review evidence #14401`.

[hivecommons-hive]

merged by Hive trusted-author auto-merge: author @Danathar holds role owner and GitHub permission maintain on repo; CI green at a13a0c6.