[hivecommons-hive]

Closes #12962

Summary

A review-swarm requires_human (or reject) hold stayed in the owner's "your turn" inbox and in /api/review/pipeline human_hold after the owner approved the PR or replied /hive approve. The self-merge sweep already treated those as a release (#12374), so the board and the merger disagreed. The board had no input for a person's release: pipeline.ReviewHolds / ownershipEnv.reviewHolds never saw review or comment evidence.

Both surfaces now read one shared predicate, github.ReleasesReviewHold. It follows the pattern of HumanReviewBlocksMerge (#12378): an APPROVED review or a /hive approve reply, from a person, after the verdict's RecordedAt. The sweep records each release it finds, and the pipeline and inbox read that record. A newer blocking verdict, for example one on a later head, holds the PR again.

Decision taken by hive: the release evidence reaches the board through a small durable ledger, /data/review-hold-releases.json. The self-merge sweep writes it at its review verdict gate, where it already lists the PR's reviews and comments. This is additive and backward compatible: there is no API call per render, and the dashboard enumeration is unchanged. A missing or unreadable ledger leaves every hold in place (fail closed). Known limitation: the release reaches the board only after the sweep reaches its verdict gate. That means the PR must be otherwise mergeable (green, not conflicting) and authored by the App or a trusted author, because those are the only PRs the sweep merges.

Changes

• pkg/github/review_hold_release.go (new): ReleasesReviewHold (the shared predicate) and IsReviewHoldReleaseCommand (moved from automerge). It also adds the review-hold-releases.json ledger (LoadReviewHoldReleases, RecordReviewHoldRelease, ReviewHoldReleaseFor), modelled on review-links.json. The ledger keeps only the command, never the comment text, and ignores anything the predicate would refuse.
• pkg/github/automerge: personReleasedReviewHold builds the evidence and asks ReleasesReviewHold. reviewVerdictGate hands any release it finds to the new Options.RecordReviewHoldRelease hook. The gate's own behaviour is unchanged.
• cmd/hive: wires the hook to the ledger for both sweep call sites.
• pkg/review/pipeline: new Inputs.HoldRelease. In reviewSwarmHolds, a recorded release after the verdict lifts the verdict hold. When the blocking verdict is on the current head, it also lifts the dispatch hold that verdict placed; fix-cap and no-fixer holds stay. A released current-head verdict now shows as approved. The carried-hold reason now reads "no approving verdict or person's approval since".
• pkg/dashboard: /api/review/pipeline and the your-turn decision both load the ledger and pass HoldRelease.
• Docs: dashboard.md, maintainer-commands.md and review-swarm.md describe the release. Changelog fragment changelog.d/fixed-12962.md.

Tests

• pkg/github/review_hold_release_test.go: the predicate table covers a person's review or /hive approve after the verdict (releases); the same before or at the verdict, a bot, a plain comment, a COMMENTED review, or a missing login (all hold). It also covers the command parser and a ledger round trip.
• pkg/review/pipeline/review_holds_test.go TestReviewHolds_PersonRelease12962: a requires_human on the current head plus a person's APPROVED review after RecordedAt gives no hold and the approved stage. The test also covers /hive approve, reject, a carried hold, and the verdict's dispatch hold being released. A comment before the verdict, a bot approval, a plain comment, a newer requires_human on a later head, or an unrelated dispatch hold still holds.
• pkg/dashboard/ownership_review_holds_12377_test.go TestReviewHoldReleasedByAPerson12962: with the ledger loaded, there is no your-turn row and no human_hold; a release before the verdict or a bot's approval still gives review_requires_human.
• pkg/github/automerge/automerge_review_hold_12962_test.go TestReviewVerdictGateAgreesWithPipeline12962: an agreement test. For one fixture, the sweep's reviewVerdictGate (through trySweepSelfAuthoredPR) and pipeline.ReviewHolds, which the ownership decision reads, agree on held or released, using the release the sweep recorded.

Tests were not run locally, per AGENTS.md; CI is the verdict. gofmt -l reports no issues.

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

[hivecommons-hive]

hive/awaiting-ci: CI pending — sweep will merge when green.

---

🐝 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.

No conflicting open PRs or same-file overlaps were found.

[hivecommons-hive]

hive/awaiting-ci: rebased onto v6 after #12960 merged (docs-only conflict in review-swarm.md, kept both paragraphs). CI pending — sweep will merge when green.

---

🐝 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]

Read at head 1f9171b.

Release is judged against RecordedAt on the board but FirstRecorded() in the sweep — the drift this PR sets out to close reopens on the next artifact refresh (medium, in-scope).

• Sweep side: reviewVerdictGate passes hold.RecordedAt (automerge_review_hold.go:69), which review.LatestVerdictHold fills from latest.FirstRecorded() (pkg/review/verdict_hold.go:66, comment at :20-23 citing #12957).
• Board side, current-head verdict: reviewSwarmHolds builds the hold with Since: current.RecordedAt (pkg/review/pipeline/review_holds.go:63,65) and the new release check reads out[0].Since (review_holds.go:73); approvalReasons likewise calls ReleasesReviewHold(current.RecordedAt, *r) (pkg/review/pipeline/pipeline.go:468).
• CollectAndMerge restamps RecordedAt = now on every collect, and only FirstRecordedAt is preserved for an unchanged verdict (pkg/review/review.go:680-686, 714-722).

Sequence: verdict first recorded T0 → owner approves at T1 → sweep records the release (At: T1) → next collect sets RecordedAt = T2 > T1. ReleasesReviewHold(T2, release@T1) is false, so the PR drops back into human_hold / "your turn" while the sweep still treats it as released. This is the current-head case the body and review-swarm.md describe as fixed; the carried-hold path is fine because carriedVerdictHold uses h.RecordedAt = FirstRecorded() (review_holds.go:112). Tests don't catch it because every fixture leaves FirstRecordedAt zero so FirstRecorded() == RecordedAt.

Requested change: use current.FirstRecorded() for Since at review_holds.go:63,65 and in the comparison at pipeline.go:468 (and add a case with FirstRecordedAt < At < RecordedAt to TestReviewHolds_PersonRelease12962).

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

[hivecommons-hive]

Security perspective — no exploitable vulnerability, injection, secret or permission regression found. One in-scope defect that defeats the contract (#12962: board and sweep must agree), plus a body/diff gap.

1. Current-head release is judged against RecordedAt, which is restamped on every collect; the sweep uses FirstRecorded(). src/pkg/review/pipeline/review_holds.go:63,65 set Since: current.RecordedAt, and :73 calls ReleasesReviewHold(out[0].Since, …); src/pkg/review/pipeline/pipeline.go:468 likewise uses current.RecordedAt. The sweep's hold comes from review.LatestVerdictHold → RecordedAt: latest.FirstRecorded() (src/pkg/review/verdict_hold.go:66, src/cmd/hive/merge_eligibility.go:224), and src/pkg/review/review.go:680 states RecordedAt is restamped on every collect. So after the next artifact refresh current.RecordedAt moves past release.At, ReleasesReviewHold returns false, and the inbox/human_hold holds the PR again while the sweep still treats it as released — the exact #12957-style drift this PR exists to remove. The carried-hold path is fine (it goes through LatestVerdictHold). The agreement test (automerge_review_hold_12962_test.go) uses a fixture with no FirstRecordedAt and a single timestamp, so it cannot see this. Fix: use current.FirstRecorded() at review_holds.go:63,65 and pipeline.go:468, and add a case with RecordedAt after the release and FirstRecordedAt before it.

2. Body says fix-cap / no-fixer dispatch holds stay; code lifts any dispatch hold on the head. review_holds.go:74-76 sets hold = nil whenever blocksOnHead(current), without checking hold.Reason. The test "a release does not lift a dispatch hold the verdict did not place" passes only because it has no verdict. Low impact (board-only), but either gate on the reason the verdict placed or drop the claim from the PR body.

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

[hivecommons-hive]

Style/maintainability review (head 1f9171b)

Current-head release compares against the restamped RecordedAt, not FirstRecorded() — the board and the sweep still drift (in-scope, changes requested)

The sweep's "since" is latest.FirstRecorded() (src/pkg/review/verdict_hold.go:66, wired through src/cmd/hive/merge_eligibility.go:224), and the pipeline's carried hold inherits that via LatestVerdictHold (src/pkg/review/pipeline/review_holds.go:112). But the current-head path uses the raw field:

• src/pkg/review/pipeline/review_holds.go:64,66 set Since: current.RecordedAt, and the new release check at :73 is ReleasesReviewHold(out[0].Since, …).
• src/pkg/review/pipeline/pipeline.go:468 is ReleasesReviewHold(current.RecordedAt, *r).

Per src/pkg/review/review.go:201-208 (and this PR's own review-swarm.md text), re-collecting the same verdict on an artifact refresh moves recorded_at; first_recorded_at is the stable instant. So for a requires_human on the current head — the primary case in #12962 — the sweep records the release, the board shows approved, and on the next artifact refresh current.RecordedAt passes release.At and the board holds again while the sweep stays released. That is the drift #12962 asks to close, and the #12957 fix is bypassed on this path.

TestReviewVerdictGateAgreesWithPipeline12962 misses it because the fixture leaves FirstRecordedAt zero so FirstRecorded() == RecordedAt.

Requested change: use current.FirstRecorded() at review_holds.go:64,66 and pipeline.go:468 (the Since the carried path already uses), and add an agreement-test case with FirstRecordedAt earlier than RecordedAt and the release between them.

Advisory (no action needed): review_holds.go:74-76 drops the dispatch hold by position whenever blocksOnHead(current), while the comment at :57-58 says "the dispatcher's hold, which that verdict placed". Today that holds because fix-cap/no-fixer holds only arise under changes_requested (src/pkg/review/dispatch.go:740,744), but the coupling is implicit; checking hold.Reason against the "review verdict " prefix from dispatch.go:767-769 would make it explicit.

— hive: agent=adjudicator 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 (style): Lead doc sentence of busyNoActivityStall still describes only the Working case #12922 — Review backlog (style): Dispatch hold is released by position, not by reason, despite the comment saying 'which that verdict placed' (Review backlog (style): Lead doc sentence of busyNoActivityStall still describes only the Working case #12922)

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

[hivecommons-hive]

Addressed review: the board now judges a person's release against FirstRecorded() (reviewSwarmHolds Since and approvalReasons), matching the sweep, so a later RecordedAt restamp no longer reopens the hold. Added a FirstRecordedAt < At < RecordedAt case to TestReviewHolds_PersonRelease12962. The dispatch hold is now cleared only when the verdict placed it (new HumanReviewHold.PlacedByVerdict, built on humanReason), so fix-cap/no-fixer holds survive; the existing fix-cap case covers it. hive/awaiting-ci: CI pending — sweep will merge when green.

---

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

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

[hivecommons-hive]

Addressed review: the board now judges a person's release against FirstRecorded() (reviewSwarmHolds Since and approvalReasons), matching the sweep, so a later RecordedAt restamp no longer reopens the hold. Added a FirstRecordedAt < At < RecordedAt case to TestReviewHolds_PersonRelease12962. The dispatch hold is now cleared only when the verdict placed it (new HumanReviewHold.PlacedByVerdict, built on humanReason), so fix-cap/no-fixer holds survive; the existing fix-cap case covers it. hive/awaiting-ci: CI pending — sweep will merge when green.

---

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

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

[Danathar]

/lgtm
/approve

[kubestellar-prow]

LGTM label has been added.

Details

Git tree hash: 36ca2451e3050e00cd50c70b3527e9e3ab135bde

[kubestellar-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Danathar

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [Danathar]

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

[github-actions]

Thank you for your contribution! Your PR has been merged.

We'd love to hear how your experience was: share feedback