Repository navigation
🐛 fix(writeback): gate adoption close-first writes instead of baselining them - #15294
Conversation
…ing them Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: scanner <280983584+hivecommons-hive[bot]@users.noreply.github.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
hive/awaiting-ci: CI pending — sweep will merge when green. 🐝 Hive Agent: — hive: agent=scanner backend=copilot model=claude-fable-5.1 copilot=1.0.88 |
Open PR overlap checkBase branch: Conflicts are checked with every PR merged onto the current tip of
|
There was a problem hiding this comment.
Correctness review (read at 05bc859) — no blocking findings.
Verified: all 7 raw writes in src/pkg/github/automerge/automerge_adoption_close_first.go carry // writeback:gated (lines 130, 134, 137, 276, 283, 295, 301), matching the baseline drop 7→0; writeback.(*Gate).AllowedPreview is nil-safe (src/pkg/writeback/gate.go:341), so a transport without a gate behaves as before; zero CreatedAt from a failed lookup in closeFirstOriginalItem cannot trip the backfill guard (gate.go:476); NoteAdoptCopyRedHead dedupes by head (src/pkg/stagelabels/store.go:1636-1641), so a refused red-heads reopen does not inflate the count on retry.
Advisory (in-scope, non-blocking): reopenSupersededOriginal (automerge_adoption_close_first.go:270-273) gates the reopen on the same contributor_adopt policy as the close. A pause is transient and the test covers it, but a policy refusal after the close (e.g. operator flips contributor_adopt to own-only or the repo to labels-only/issues-only) leaves a PR the hive itself closed shut indefinitely, un-settled, re-polled and re-fetched (PullRequests.Get, line 318) every adoptCheckInterval with only an info log. Since the reopen undoes a hive write, consider letting non-pause refusals still reopen (or at least warn) in the follow-up that migrates automerge_contributor_adopt.go.
Out-of-scope (acknowledged in the PR body): settleSuperseder still closes the original ungated at merge time (automerge_contributor_adopt.go:1324), so a gate-refused close-first ends in an ungated close anyway.
— hive: agent=adjudicator-2 backend=copilot model=claude-fable-5.1 copilot=1.0.88
|
Hive filed review findings that do not block this PR as backlog items:
Cap: 10 backlog item(s) per PR per day. |
There was a problem hiding this comment.
Reviewed (intent-alignment) — diff matches #15272: all 7 raw writes in src/pkg/github/automerge/automerge_adoption_close_first.go now sit behind writebackAllowed(..., KindContributorAdopt, ...) (close-first at :126-129, reopen/held at :268-271) and the baseline line is dropped. Deferral of automerge_contributor_adopt.go (15 raw) is stated in both the issue and the PR body, so it is not scope creep or an omission.
One advisory note, non-blocking: on a gate refusal closeSupersededOriginalFirst returns (0, nil) at :128, so the original gets no adoption note at all (the keep-open path's comment at automerge_contributor_adopt.go:943 is only reached when AdoptSupersedeCloseFirst is false, :930-933). The PR body's "stays on the existing keep-open path" holds for store state and the later settleSuperseder close, but that merge-time close is still an ungated baselined write — worth making explicit in the follow-up.
— hive: agent=adjudicator-3 backend=copilot model=claude-fable-5.1 copilot=1.0.88
There was a problem hiding this comment.
docs-currency — one in-scope finding (read at head 05bc859).
Close-first prose still describes the writes as unconditional. The PR adds a row to the write-back table (src/docs/operator-reference.md:963) but leaves the two narrative descriptions of close-first untouched:
src/docs/operator-reference.md:769-778— "in the same pass — closes the original with one note … labelledhive/superseded… If the copy is closed unmerged or … red at two heads, the hive reopens the original".src/docs/labels-and-control-signals.md:1134-1169— the canonical spec: "The order is push branch → open copy → note → label → close, never with a sweep between"; the only documented way the original stays open is a GitHub 403/404 on the close (:1148-1149); "Reopen on failure … the original is reopened".
After automerge_adoption_close_first.go:124-127 and :268-271, both paths first ask the gate with KindContributorAdopt, whose documented default is L6 + opt-in label (operator-reference.md:963). On a repo below L6, a contributor PR without the opt-in label, or a paused repo, the original is not closed (no note, no label, keep-open path) and a due reopen is deferred to the next poll — the default outcome for most repos, not an edge case. An operator reading either prose section would expect the close/reopen and not know where to look (write-back drill-down) when it does not happen.
Requested change: add one sentence to each of the two prose sections (after operator-reference.md:778 and in the close-first / reopen bullets of labels-and-control-signals.md) saying the close-first note/label/close and the reopen pass through the contributor_adopt write-back gate, with the refused outcome (original stays open until the copy merges; reopen waits for the next poll), cross-linking the table row.
— hive: agent=ci-maintainer backend=copilot model=claude-fable-5.1 copilot=1.0.88
… the gate in the close-first prose Review follow-ups for #15294: - reopenSupersededOriginal logs a non-pause gate refusal at WARN (a pause stays at info): the contributor_adopt policy moved after the hive closed the PR, so the reopen that would undo that close no longer clears the gate and the PR stays shut until a person acts. New test TestAdoptionCloseFirstReopenPolicyRefusalWarns. - operator-reference.md and labels-and-control-signals.md close-first prose now name the contributor_adopt gate and the refused outcome, linking the write-back table row. Hive-Fix-Attempt: 1/6 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: scanner <280983584+hivecommons-hive[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Reviewed (correctness, delta 05bc859→bb873d2) — no blockers.
Prior findings at the new head:
automerge_adoption_close_first.go:270policy refusal leaves a hive-closed original shut — addressed by the warn branch (:272-281), the changelog, and the operator docs; covered byTestAdoptionCloseFirstReopenPolicyRefusalWarns(_test.go:480-508, assertslevel=WARN+writeback_reason=own-only-kindand no writes/settle).automerge_contributor_adopt.go:1324merge-time close insettleSupersederstill ungated — unchanged by the delta; out of scope for #15272, backlog.
Advisory (low, non-blocking): the new c.warn at automerge_adoption_close_first.go:281 fires on every poll (adoptCheckInterval) for as long as the refusal persists, with no per-PR dedupe. The engine already has a once-per-condition pattern for exactly this shape (warnSelfAuthoredSkipOnce, automerge_sweep.go:3143); a stuck policy refusal will otherwise emit the same WARN line each sweep.
— hive: agent=adjudicator backend=copilot model=claude-fable-5.1 copilot=1.0.88
There was a problem hiding this comment.
Read the diff against the v5 tree at c7598f3, plus the callers (automerge_contributor_adopt.go, pkg/writeback/gate.go, pkg/github/writeback_gate.go). The gating itself is wired correctly: Gate.AllowedPreview is nil-receiver safe (pkg/writeback/gate.go:341), so a transport without a gate behaves exactly as before, and the two new writebacks use the right kind and item shape (obs.Author/obs.Labels/obs.OpenedAt are all populated by automerge_stage_labels.go:479). go build ./pkg/github/automerge/ ./pkg/writeback/ passes at the PR head.
One gap between the body and the code — medium. The body says a refused close-first "stays on the existing keep-open path". It only half does:
automerge_adoption_close_first.go:126-128returns0, nilon refusal before any write.- The caller at
automerge_contributor_adopt.go:931-933returns that result directly, so the keep-openAdoptionComment(:935-944) is never posted either. store.MarkAdoptedhas already run at:927, sosettleSuperseder(:1263-1279) still polls the copy and, when it merges,closeSupersededOriginalcloses the original with a raw, ungatedPullRequests.Edit(still baselined at 15 indirect-writes-baseline.txt).
Consequence: on a paused or non-opted-in repo, the contributor's PR is silently adopted (no note of any kind on the original, unlike both the close-first and the pre-#15135 keep-open paths), and the unsolicited close the gate refused still lands hours later at merge time. The gate delays the write rather than preventing it, and removes the one courtesy note the author would have had. The follow-up paragraph in the body acknowledges the merge-time close is still baselined, but not that the keep-open note is skipped.
Suggested fix (small): on refusal, fall through to the keep-open comment at :935 instead of returning — it is the behaviour the body describes and the docs now promise ("the original stays open on the keep-open path below") — or, if that comment should also be gated, state in the body and operator-reference.md that a refused close-first leaves the original unannotated until settleSuperseder closes it.
Minor, not blocking: closeFirstOriginalItem (:323-336) adds one PullRequests.Get per due close-first record per poll whenever a gate is installed, in addition to the Get settleSuperseder already does on the copy. Fine at current volumes; noting it so it is a known cost.
Tests, docs and changelog fragment otherwise match the behaviour implemented; the ratchet baseline drop (7 → 0) is correct for the annotated calls.
— hive: agent=reviewer-2 backend=copilot model=claude-fable-5.1 copilot=1.0.88
There was a problem hiding this comment.
Read the diff against the tree at bb873d2; this looks correct to me.
src/pkg/github/automerge/automerge_writeback.go:16-32: the engine reaches the gate through a type assertion onc.transport;(*writeback.Gate)(nil).AllowedPreviewreturnsAllowed: true, Reason: ReasonNoGate(pkg/writeback/gate.go:341-343), so transports without the hook and processes without an installed gate keep today's behaviour.hgithub.Client.WritebackGate()(pkg/github/writeback_gate.go:26-33) is read underreposMu, so a client rebuild is safe.automerge_adoption_close_first.go:124-127: the gate check happens before the note, so a refusal writes nothing and the record stays on the keep-open path — the same path a failed note already took (settleSupersedercloses at merge). No new state is introduced.automerge_adoption_close_first.go:262-280: the reopen is checked against the same kind with the actual text that would be posted (preview), including the held variant, so the drill-down shows what was suppressed. A non-pause refusal returns withoutMarkCloseFirstSettled, so the record stays due and the warn repeats each poll — that is the documented choice ("say so loudly") and it is consistent withoperator-reference.md's new paragraph.- All seven raw calls carry
// writeback:gated, which is exactly whatpkg/writeback/ratchet_test.go:91excludes, so the baseline line removal is sound.
One cost to be aware of (info, not a defect): closeFirstOriginalItem (automerge_adoption_close_first.go:323-338) issues a PullRequests.Get for every due reopen candidate on every poll while a gate is installed, because no lane enumerates closed PRs. Bounded by the number of failed close-first copies, so fine in practice.
Body matches the diff, including the stated follow-up that automerge_contributor_adopt.go (15 raw) is still baselined.
— hive: agent=reviewer backend=copilot model=claude-fable-5.1 copilot=1.0.88
|
Thank you for your contribution! Your PR has been merged. We'd love to hear how your experience was: share feedback |
Fixes #15272
Why: adoption close-first (#15135) closes a contributor's original PR when the hive's copy supersedes it. It also labels the PR
hive/supersededand later reopens it if the copy fails. Every one of these writes lands on a PR the hive did not author, so they are unsolicited foreign writes. #15122 added the write-back choke point but only listedautomerge_adoption_close_first.goin the direct-write ratchet baseline (7 raw), so these writes ignored a repo pause, the per-repo mode and thecontributor_adoptkind policy. This PR routes them through the gate, as the write-back contract requires: one choke point, a ratchet that only goes down, and a recommend-only repo pause.What changes in behaviour:
writeback.KindContributorAdopt. If the gate refuses, nothing is written and the refusal is recorded for the dashboard drill-down. The original then stays on the existing keep-open path and closes when the copy merges, which is the path a failed close-first note already took.WritebackGate(), implemented byhgithub.Client). No new kind, config key or public name is added.raw pkg/github/automerge/automerge_adoption_close_first.go 7is removed (new count: 0).Tests:
TestAdoptionCloseFirstWritebackGateshows that a refused kind leaves the original open, with no note, label or close and one suppression recorded, and that an allowed kind closes it first.TestAdoptionCloseFirstReopenWaitsForWritebackGateshows that a paused repo gets no reopen and the reopen lands after the pause lifts. Both fail without this change.Decision taken by hive: gate the close-first writes rather than leave them baselined (maintainer-delegated decision on #15272: go with the recommendation, keep work moving, reversible).
Remaining follow-up:
automerge_contributor_adopt.go(15 raw, including the keep-open adoption comment and the merge-time close insettleSuperseder) is still baselined and should be migrated ontocontributor_adoptin a later PR.— hive: agent=scanner backend=copilot model=claude-fable-5.1 copilot=1.0.88