Skip to content

πŸ› fix: preserve owner intent through PR decision paths - #14822

Closed
thatguywob wants to merge 3 commits into
hivecommons:v5from
thatguywob:fix/14816-decision-path
Closed

thatguywob wants to merge 3 commits into
hivecommons:v5from
thatguywob:fix/14816-decision-path

Conversation

@thatguywob

Copy link
Copy Markdown
Contributor

Fixes #14816

Owner approvals with rationale now stay approvals instead of scheduling a cosmetic fix that invalidates the approved head. The full rationale remains in the durable decision record. Commands beginning with approve, approved, lgtm, or yes approve; change requests start with the instruction.

  • Read /hive decision at 070 as well as 085, and let a later trusted APPROVED review on the same head supersede the stored request so metadata-only fixes need no new commit. Approval selection uses each reviewer's latest opinion across all review pages.
  • Preserve non-closing references when a Hive decision reply is recorded or the PR body has been edited. Treat edited Refs conservatively as intentional, including a downgrade from Closes, rather than restoring automatic issue closure. Pasted decision markers from unrelated commenters do not count.
  • Read trusted head-scoped contributor change requests before admission/verdict early returns, so requires_human yields to 072 and the existing author nudge path.

Added regression coverage for rationale parsing and retention, same-head 070 release, stale/untrusted/wrong-head approvals, superseded reviews, owner-directed references, and contributor routing. Updated maintainer command documentation and added a changelog fragment.

Validation: gofmt -l on all changed Go files returned no output. Tests and lint are reserved for CI by AGENTS.md and were not run locally. Merge gates and trusted-merger requirements remain in force.

β€” hive: backend=codex


🐝 Hive Agent: contributor | SHA: 6c8d05c7e

Recognize approval rationale, preserve intentional non-closing issue links, release metadata-only fixes on same-head approval, and route trusted contributor change requests before requires_human admission returns.

Hive-Run: hivecommons#14816
Hive-Plan: issue-14816/decision-path
Hive-Spec: issue-14816#1-4
Co-authored-by: clubanderson <407614+clubanderson@users.noreply.github.com>
Signed-off-by: thatguywob <thatguywob@users.noreply.github.com>
@kubestellar-prow kubestellar-prow Bot added dco-signoff: yes Indicates the PR's author has signed the DCO. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 28, 2026
@hivecommons-hive hivecommons-hive Bot added author/clanker A contributor's own hive authored this PR under their login dco All commits are DCO signed-off role/trusted Author is trusted for unattended merge (trusted-author role or registry/claim tier in require_tiers) stage/050-review-pending Review swarm requested; awaiting a verdict for the current head stage/072-returned-to-contributor Contributor PR: CI red, changes requested or conflicting; diagnosis posted, author must act and removed stage/050-review-pending Review swarm requested; awaiting a verdict for the current head labels Oct 28, 2026
@hivecommons-hive

Copy link
Copy Markdown
Contributor

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

Failing checks:

  • test (rest 1/4)

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.

Queued in your relay fix queue (1 waiting): fixes are handed out before new work as soon as you have capacity.

Include the automated author kind when checking contributor stage routing, and keep the ignored-stage regression outside both supported decision stages (070 and 085).

Signed-off-by: thatguywob <thatguywob@users.noreply.github.com>
@hivecommons-hive hivecommons-hive Bot added stage/050-review-pending Review swarm requested; awaiting a verdict for the current head and removed stage/072-returned-to-contributor Contributor PR: CI red, changes requested or conflicting; diagnosis posted, author must act labels Oct 28, 2026

@hivecommons-hive hivecommons-hive Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Read the diff against v5 plus the callers (automerge_sweep.go:1646, automerge_review_gate.go, review/human_change_request.go Record/For, pr_request_watcher.go:501). The four changes do what #14816 asks and I found no defect. Looks correct to me.

What I checked:

  • Approval parsing (src/pkg/github/issue_unpark_command.go:102-115): first-word match on DecisionText() is the semantics #14816 Β§1 proposed; the only caller is prDecisionModeFor (pr_unpark_decision.go:139), so issue-side /hive decision is unaffected.
  • Same-head approval releasing 070 (automerge_human_changes_requested.go:101-116): Record keeps the standing changes-kind entry (different SourceID, same head) and For returns the newest RecordedAt, so the approve-kind record outranks it on the next pass exactly like the dashboard direction: approve path already does. The After(standing.RecordedAt) guard is sound now that review-sourced requests stamp RecordedAt from SubmittedAt (line 133).
  • Shared latestTrustedHumanReview: latestTrustedHumanApproval now uses latest-opinion-per-login, so an approval later superseded by the same reviewer's CHANGES_REQUESTED no longer counts. That is a tightening of the review gate, and a correct one.
  • lastEditedAt == "" guard (pr_issue_link.go:331): the attribution trailer is appended at creation (pr_request_watcher.go:501), not by a post-open edit, so freshly opened hive PRs still get the Refsβ†’Closes repair; only edited bodies are left alone, as the body states.

One low-severity note, not a blocker:

  • issue_unpark_command.go:110 keeps yes in the prefix set. Under the old exact-match BareApproval that was harmless; as a prefix it turns /hive decision yes, but also change X into mode=approve and silently drops the instruction. #14816 Β§1 proposed approve/approved/lgtm only. Suggest dropping yes from the prefix list (or keeping it bare-only) so a hedged answer still reaches the fix lane.

Open question I could not verify from the tree: #14816 Β§4 (crux#22 staying at 085) is addressed by the early return at automerge_contributor_lane.go:1222, but the issue also floated crux being outside the trusted-merger config. If it is, this change will not move that PR β€” worth a look at the crux lane config after merge rather than a change here.

β€” hive: agent=reviewer-2 backend=copilot model=claude-fable-5.1 copilot=1.0.88

@hivecommons-hive hivecommons-hive Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Read the tree at 09d61ee (not just the diff): automerge_human_changes_requested.go, automerge_contributor_lane.go:1210-1265, pkg/review/human_change_request.go (Record/For), issue_unpark_command.go, pr_issue_link.go. Per #14816 item 1 the first-word approval rule is the owner's stated choice, so I judged against that, not against the old bare-only behaviour.

What checks out

  • latestTrustedHumanReview keeps the newest of APPROVED|CHANGES_REQUESTED|DISMISSED per login before filtering, so an older approval can no longer shadow a later change request on the same head β€” the right fix for item 4.
  • The approval-clears-standing-request path is sound: HumanChangeRequests.For returns the newest record by RecordedAt (human_change_request.go:219), and the approve record is stamped with the review's SubmittedAt, which the guard already required to be after standing.RecordedAt, so the next pass sees IsApproval() and stops scheduling a fix.
  • No textual conflict with #14821 even though both touch pr_unpark_decision.go (git merge-tree of the two heads is clean).

Two things worth fixing before this lands

  1. yes as a first word now approves and discards the rest β€” src/pkg/github/issue_unpark_command.go, Approval() case list ("approve", "approved", "lgtm", "yes"). /hive decision yes, but rename the flag to --foo first was changes_requested with the text kicked to the author; it is now approve, and the instruction becomes inert rationale. #14816 proposes only approve/approved/lgtm for the prefix form; yes is far more often the opener of a conditional answer. Suggest keeping yes bare-only. (medium)
  2. A trailing comma defeats the new rule β€” same function, strings.TrimRight(words[0], ".:"). /hive decision approve, the body change is applied yields words[0] == "approve," β†’ not matched β†’ changes_requested β†’ fix kick, which is exactly the loop item 1 describes. Add ,; (and the em dash the tests use after a space) to the trim set, or split on strings.FieldsFunc. (low)

Open question, not a defect: pr_issue_link.go now skips the Refs→Closes repair whenever lastEditedAt != "". Any body edit — including the hive's own at pr_contributor_link.go:109 and pr_supersession_resolve.go:303, or an agent editing its PR description — permanently disables that repair for the PR. It fails toward leaving the issue open, which is the safe side, so fine if intended; flagging so it is a choice rather than a surprise.

Minor: the contributor lane now lists reviews three times per PR per tick (latestTrustedHumanApproval, then the new early latestTrustedHumanChangesRequested, then hasOutstandingChangesRequested). Not blocking; could reuse one read.

I have not approved, merged, or changed anything.

β€” hive: agent=reviewer backend=copilot model=claude-fable-5.1 copilot=1.0.88

@hivecommons-hive hivecommons-hive Bot added stage/060-reviewed Review swarm verdict approve; a gate is still pending (CI running, soak not started) and removed stage/050-review-pending Review swarm requested; awaiting a verdict for the current head labels Oct 28, 2026
@github-actions

github-actions Bot commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

Open PR overlap check

Base branch: v5

This PR conflicts with v5 itself. Rebase it first: until it merges cleanly into the base, conflicts with other open PRs cannot be told apart from base drift, so only same-file overlaps are listed.

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

PR Title Relation Files
#15233 πŸ› fix: preserve owner intent through PR decision paths (adopted) same files src/docs/maintainer-commands.md
src/pkg/github/automerge/automerge_contributor_lane.go
src/pkg/github/automerge/automerge_contributor_lane_test.go
src/pkg/github/automerge/automerge_human_changes_requested.go
src/pkg/github/automerge/automerge_human_changes_requested_test.go
src/pkg/github/automerge/automerge_review_gate.go
src/pkg/github/issue_unpark_command.go
src/pkg/github/issue_unpark_command_test.go
src/pkg/github/pr_issue_link.go
src/pkg/github/pr_issue_link_test.go
src/pkg/github/pr_unpark_decision.go
src/pkg/github/pr_unpark_decision_test.go
#15186 πŸ› Activity Monitor: one coherent post-decision state for Let hive decide rows; 060 reviewed in CI / Review same files src/pkg/github/issue_unpark_command.go
#15159 🌱 chore: remove unreachable MITM cancellation guard same files .github/workflows/v2-ci.yml
#15104 πŸ› Activity Monitor: Rebase, then merge as maintainer; fixer owns parked conflicting PRs; dismiss self-inflicted changes_requested same files src/pkg/github/issue_unpark_command.go
src/pkg/github/pr_unpark_decision.go
src/pkg/github/pr_unpark_decision_test.go
#15216 πŸ› Sentinel: a dashboard Approve releases the head for good; skip Prow-rejected /lgtm + /approve same files src/pkg/github/automerge/automerge_contributor_lane_test.go
#15226 ✨ feature: enqueue eligible PRs into GitHub native merge queues same files src/pkg/github/automerge/automerge_contributor_lane.go
src/pkg/github/automerge/automerge_contributor_lane_test.go

@hivecommons-hive hivecommons-hive Bot added stage/072-returned-to-contributor Contributor PR: CI red, changes requested or conflicting; diagnosis posted, author must act and removed stage/060-reviewed Review swarm verdict approve; a gate is still pending (CI running, soak not started) labels Oct 28, 2026
@hivecommons-hive

Copy link
Copy Markdown
Contributor

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

Failing checks:

  • overlayfs-exec-guard

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.

Queued in your relay fix queue (1 waiting): fixes are handed out before new work as soon as you have capacity.

Match the docker matrix timeout so slow backend package downloads do not cancel the job before its overlayfs exec and stale-binary assertions run. Keep the 90-second exec timeout and all guard assertions unchanged.

Signed-off-by: thatguywob <thatguywob@users.noreply.github.com>
@hivecommons-hive hivecommons-hive Bot added stage/050-review-pending Review swarm requested; awaiting a verdict for the current head and removed stage/072-returned-to-contributor Contributor PR: CI red, changes requested or conflicting; diagnosis posted, author must act stage/050-review-pending Review swarm requested; awaiting a verdict for the current head labels Oct 28, 2026

@hanthor hanthor left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2: Reconcile persisted review-backed approvals after dismissal (src/pkg/github/automerge/automerge_human_changes_requested.go:104-110). A later same-head APPROVED review is now persisted as an approval-kind durable decision. If that review is dismissed before merge, live review selection rejects it, but humanChangesRequestedForHead returns early for standing.IsApproval(), recordedHumanApprovalForHead accepts the persisted record without revalidating its source review, and stage folding continues treating requires_human as answered. The revoked approval can therefore suppress needs-decision and prevent the prior instruction reaching the fixer. Reconcile review-backed records against current review state while preserving durable explicit comment/dashboard decisions; cover approval -> dismissal on the unchanged head.

Owner/workflow intent is already recorded for this head; this is not a request to override it. Before merge, fix the above, rebase onto current v5, obtain a successful overlayfs-exec-guard verdict (the current-head run exceeded its new 45-minute budget), and renew the head-scoped workflow approval after new commits. Preserve existing delegated-decision guards; do not waive the workflow credential fence or close this as superseded without an actual replacement.

@hivecommons-hive

Copy link
Copy Markdown
Contributor

Hive decided close this PR and open a fresh hive-authored PR with the same commits at 2026-10-28T13:01:29Z, but it hasn't taken effect.

What should have happened: The hive closes this PR and opens a fresh hive-authored PR with the same commits and sign-off, then finishes that one.
Why it hasn't: The branch has merge conflicts with the base branch; it needs a rebase before anything can merge.
What you can do: Merge as maintainer (on GitHub) Β· Re-run decision Β· Close as superseded Β· Send back for a new commit.

Posted by the hive as the owner's delegate; the rule was: the author's hive hasn't been connected since the nudge (contributor-offline).

clubanderson added a commit that referenced this pull request Oct 29, 2026
…dopted or could-not-adopt, routes workflow-path PRs to the workflows capability blocker, and re-adoption survives the mutation journal

Let hive decide β†’ adopt-supersede settled the operator-action ledger as
applied ("done") as soon as the adoption/supersede request label landed,
and the βœ“ hover kept saying so while nothing happened.

- #14822 edits .github/workflows: the adoption engine's
  fence refused the supersession (adoption-stalled:contributor-lane:
  workflow-path) and the row fell back to a bare Needs human decision.
  The policy now decides such PRs as capability-gap (recommend-only),
  the ledger records blocked, and the badge names the
  capability:workflows-permission blocker with what a person can do.
- #14947 was adopted (copy #14977), the copy was closed by
  the duplicate sweep, and the re-adoption's branch_push /
  pull_request_create claims had the same inputs as the first attempt, so
  the mutation journal replayed them as idempotent success without running
  the effect: created == nil β†’ "error=None" β†’ adoption reverted. The
  claims now carry prior_copy, and a replayed provenance URL is resolved
  and reused while the copy is still open.
- settledDelegatedOutcome: adopt-supersede β†’ executing, dedup β†’ no-effect,
  capability-gap β†’ blocked. The tracker derives the βœ“ state from the
  stage record (adopted as #N / could not adopt / still executing / stuck)
  and exposes progress, blocker and human to the SPA hover.

Fixes #15188

Signed-off-by: Andrew Anderson <andy@clubanderson.com>
@clubanderson

Copy link
Copy Markdown
Member

Adopted into #15233 per operator decision (adopt-supersede); your commits are preserved with authorship. Thanks @thatguywob!

@clubanderson

Copy link
Copy Markdown
Member

Superseded by #15233 (adopted per operator decision; your commits are preserved with authorship). Closing so the tracker follows the live PR. Thanks @thatguywob!

clubanderson added a commit that referenced this pull request Oct 29, 2026
…dopted or could-not-adopt, routes workflow-path PRs to the workflows capability blocker

Let hive decide β†’ adopt-supersede settled the operator-action ledger as
applied ("done") as soon as the adoption/supersede request label landed,
and the βœ“ hover kept saying so while nothing happened.

- #14822 edits .github/workflows: the adoption engine's
  fence refused the supersession (adoption-stalled:contributor-lane:
  workflow-path) and the row fell back to a bare Needs human decision.
  The policy now decides such PRs as capability-gap (recommend-only),
  the ledger records blocked, and the badge names the
  capability:workflows-permission blocker with what a person can do.
- #14947: the re-adoption after the copy #14977 was
  closed was replayed by the mutation journal; that fix landed in
  #15209 (prior_copy claim input, replayed provenance
  resolved). This PR layers the ledger / hover states over it.
- settledDelegatedOutcome: adopt-supersede β†’ executing, dedup β†’ no-effect,
  capability-gap β†’ blocked. The tracker derives the βœ“ state from the
  stage record (adopted as #N / could not adopt / still executing / stuck)
  and exposes progress, blocker and human to the SPA hover.

Fixes #15188

Signed-off-by: Andrew Anderson <andy@clubanderson.com>
clubanderson added a commit that referenced this pull request Oct 29, 2026
… as instructions

Address review on #14822:
- A same-head APPROVED review persisted as an approval-kind decision is
  dropped again when that review is dismissed or the reviewer's latest
  opinion on the head is no longer APPROVED, so a revoked approval cannot
  keep answering requires_human or shadow the earlier instruction.
  Comment/dashboard decisions stay durable. Adds HumanChangeRequestStore.Forget.
- `yes` approves only when bare; `yes, but …` stays an instruction.
- Trailing punctuation after the approval word (approve, / lgtm;) no longer
  defeats the first-word rule.
- Changelog fragment lives at repo-root changelog.d/.

Signed-off-by: Andrew Anderson <andy@clubanderson.com>
clubanderson added a commit that referenced this pull request Oct 29, 2026
… as instructions

Address review on #14822:
- A same-head APPROVED review persisted as an approval-kind decision is
  dropped again when that review is dismissed or the reviewer's latest
  opinion on the head is no longer APPROVED, so a revoked approval cannot
  keep answering requires_human or shadow the earlier instruction.
  Comment/dashboard decisions stay durable. Adds HumanChangeRequestStore.Forget.
- `yes` approves only when bare; `yes, but …` stays an instruction.
- Trailing punctuation after the approval word (approve, / lgtm;) no longer
  defeats the first-word rule.
- Changelog fragment lives at repo-root changelog.d/.

Signed-off-by: Andrew Anderson <andy@clubanderson.com>
clubanderson added a commit that referenced this pull request Oct 29, 2026
…dopted or could-not-adopt, routes workflow-path PRs to the workflows capability blocker

Let hive decide β†’ adopt-supersede settled the operator-action ledger as
applied ("done") as soon as the adoption/supersede request label landed,
and the βœ“ hover kept saying so while nothing happened.

- #14822 edits .github/workflows: the adoption engine's
  fence refused the supersession (adoption-stalled:contributor-lane:
  workflow-path) and the row fell back to a bare Needs human decision.
  The policy now decides such PRs as capability-gap (recommend-only),
  the ledger records blocked, and the badge names the
  capability:workflows-permission blocker with what a person can do.
- #14947: the re-adoption after the copy #14977 was
  closed was replayed by the mutation journal; that fix landed in
  #15209 (prior_copy claim input, replayed provenance
  resolved). This PR layers the ledger / hover states over it.
- settledDelegatedOutcome: adopt-supersede β†’ executing, dedup β†’ no-effect,
  capability-gap β†’ blocked. The tracker derives the βœ“ state from the
  stage record (adopted as #N / could not adopt / still executing / stuck)
  and exposes progress, blocker and human to the SPA hover.

Fixes #15188

Signed-off-by: Andrew Anderson <andy@clubanderson.com>
clubanderson added a commit that referenced this pull request Oct 29, 2026
)

* πŸ› fix: preserve owner intent through PR decision paths

Recognize approval rationale, preserve intentional non-closing issue links, release metadata-only fixes on same-head approval, and route trusted contributor change requests before requires_human admission returns.

Hive-Run: #14816
Hive-Plan: issue-14816/decision-path
Hive-Spec: issue-14816#1-4
Co-authored-by: clubanderson <407614+clubanderson@users.noreply.github.com>
Signed-off-by: thatguywob <thatguywob@users.noreply.github.com>
Signed-off-by: Andrew Anderson <andy@clubanderson.com>
Signed-off-by: Andrew Anderson <clubanderson@users.noreply.github.com>

* πŸ› fix: align decision routing tests with stage inputs

Include the automated author kind when checking contributor stage routing, and keep the ignored-stage regression outside both supported decision stages (070 and 085).

Signed-off-by: thatguywob <thatguywob@users.noreply.github.com>
Signed-off-by: Andrew Anderson <andy@clubanderson.com>
Signed-off-by: Andrew Anderson <clubanderson@users.noreply.github.com>

* 🌱 chore: move changelog fragment to src/changelog.d

Signed-off-by: Andrew Anderson <andy@clubanderson.com>

* πŸ› fix: reconcile review-backed approvals and keep conditional answers as instructions

Address review on #14822:
- A same-head APPROVED review persisted as an approval-kind decision is
  dropped again when that review is dismissed or the reviewer's latest
  opinion on the head is no longer APPROVED, so a revoked approval cannot
  keep answering requires_human or shadow the earlier instruction.
  Comment/dashboard decisions stay durable. Adds HumanChangeRequestStore.Forget.
- `yes` approves only when bare; `yes, but …` stays an instruction.
- Trailing punctuation after the approval word (approve, / lgtm;) no longer
  defeats the first-word rule.
- Changelog fragment lives at repo-root changelog.d/.

Signed-off-by: Andrew Anderson <andy@clubanderson.com>

---------

Signed-off-by: thatguywob <thatguywob@users.noreply.github.com>
Signed-off-by: Andrew Anderson <andy@clubanderson.com>
Signed-off-by: Andrew Anderson <clubanderson@users.noreply.github.com>
Co-authored-by: thatguywob <thatguywob@users.noreply.github.com>
Co-authored-by: clubanderson <407614+clubanderson@users.noreply.github.com>
clubanderson added a commit that referenced this pull request Oct 29, 2026
…dopted or could-not-adopt, routes workflow-path PRs to the workflows capability blocker (#15206)

Let hive decide β†’ adopt-supersede settled the operator-action ledger as
applied ("done") as soon as the adoption/supersede request label landed,
and the βœ“ hover kept saying so while nothing happened.

- #14822 edits .github/workflows: the adoption engine's
  fence refused the supersession (adoption-stalled:contributor-lane:
  workflow-path) and the row fell back to a bare Needs human decision.
  The policy now decides such PRs as capability-gap (recommend-only),
  the ledger records blocked, and the badge names the
  capability:workflows-permission blocker with what a person can do.
- #14947: the re-adoption after the copy #14977 was
  closed was replayed by the mutation journal; that fix landed in
  #15209 (prior_copy claim input, replayed provenance
  resolved). This PR layers the ledger / hover states over it.
- settledDelegatedOutcome: adopt-supersede β†’ executing, dedup β†’ no-effect,
  capability-gap β†’ blocked. The tracker derives the βœ“ state from the
  stage record (adopted as #N / could not adopt / still executing / stuck)
  and exposes progress, blocker and human to the SPA hover.

Fixes #15188

Signed-off-by: Andrew Anderson <andy@clubanderson.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author/clanker A contributor's own hive authored this PR under their login dco All commits are DCO signed-off dco-signoff: yes Indicates the PR's author has signed the DCO. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. role/trusted Author is trusted for unattended merge (trusted-author role or registry/claim tier in require_tiers) size/L Denotes a PR that changes 100-499 lines, ignoring generated files. stage/085-needs-decision A human decision is owed: sentinel alert, hold/needs-decision label, human-only rule or parked issue

Projects

None yet

3 participants