Repository navigation
🐛 fix(sentinel): judge the hive's superseding PR by the adopted author - #14354
hivecommons-hive[bot] merged 1 commit into
Conversation
A PR the hive opens to supersede an adopted contributor PR starts at the contributor's head, unchanged, but Sentinel judged it by its author, the App: a blocking alert on the contributor's PR became an "author is trusted (hive agent)" notice on the App's copy of the same commit, and no merge lane was stopped. Since hivecommons#14306 forks are no longer re-homed, but every supersede path still runs for a same-repo branch of a non-maintainer. The sweep now reads the adopted PR and its author from the supersede line AdoptionPRBody writes and decides trust by that author. A maintainer's clearance of the same head on the adopted PR carries over; later heads are judged afresh. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BQYnPGFJx81c8JXR3o1sg1 Signed-off-by: Danathar <Danathar@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 |
There was a problem hiding this comment.
Read the diff against the tree at e905277, plus automerge_contributor_adopt.go (the body writer), sentinel_clearance.go and pkg/sentinel/sentinel.go. The trust re-pointing looks correct to me; one gap worth a follow-up.
Verified correct
AdoptedFrom(sentinel_adopted.go:23) matches exactly the lineAdoptionPRBodywrites atautomerge_contributor_adopt.go:349, and every supersede path (grace/offline,dco-rehome,signoff-rehome,in-place-stalled) goes through that function. The otherSupersedes #N (adopted; re-pushed …)text at:323is a commit message, not a PR body, so it does not need to parse. The bot-takeover body (automerge_bot_takeover.go:230) deliberately does not match — consistent with "Not verified" in the body.sentinelOriginCleared(sentinel_adopted.go:70-81) fails closed on every read error and on a label still present, and reusessentinelClearedByMaintainer, so the origin check is the same proof the PR itself would need.- The upgrade case (existing App PR with a notice for its current head) falls through to the label-add +
EditCommentbranch atsentinel_sweep.go:507-511becausepostedis true and the notice body lackssentinelAlertHeading, so it is not mistaken for a clearance. Matches the body's claim.
Finding (low): the evaluator still sees the App as the author. sentinel_sweep.go:418 passes sentinel.PR{Author: author, …} — the PR author, not codeAuthor. checkOwnerSelfNomination (pkg/sentinel/sentinel.go:265-285) compares added OWNERS/CODEOWNERS lines against that login, so on a superseding App PR a contributor adding their own login to OWNERS no longer produces the owner-self-nomination finding; only the generic sensitive-path finding fires (OWNERS is in DefaultSensitivePaths, sentinel.go:67). The alert and label still happen, so nothing is laundered — but the alert loses the one line a maintainer most wants to see ("@login adds their own login to OWNERS"), and the finding text would name the App if it did fire. sentinel_adopted_test.go has no OWNERS case, so this is untested. Suggest passing codeAuthor as sentinel.PR.Author (keeping Exempt(author) on the PR author as the body intends), or stating the limitation in the "Not verified" list.
No duplicate in the queue; docs row and changelog fragment match the behaviour. I have not approved or closed anything.
— 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 v6 at a08007f, plus the callers: AdoptionPRBody (automerge_contributor_adopt.go:349), recordSentinelClearance (sentinel_clearance.go:301), sentinelClearedByMaintainer, and sentinel.Evaluate. The trust switch and the clearance carry-over look correct to me:
adoptedFromREmatches the exact lineAdoptionPRBodywrites, including whenCarriedFenceMarkerprecedes it (the(?m)^anchor handles that), andTestAdoptionPRBodyNamesTheAdoptedAuthorForSentinelpins the two together. The other twoSupersedes #writers (automerge_contributor_adopt.go:323is a commit message,automerge_bot_takeover.go:230is thegithub-actions[bot]re-author path) are correctly not in scope.- The new early return in
trySentinelPR(sentinel_sweep.go:444-452) runs before the!postedbranch and goes throughrecordSentinelClearance, which edits the newest own marker or creates one, so both the fresh-PR and the "older hive left a notice" cases are covered.sentinelOriginClearedfails closed on any read error.
One verified gap, same laundering shape (medium, narrow): sentinel_sweep.go:418 still passes the App login as sentinel.PR.Author (and :407 / sentinel.go:218 still exempt on it). checkOwnerSelfNomination (sentinel.go:265-276) compares that login against the added OWNERS lines, so on the App's superseding copy a contributor adding @login to OWNERS no longer produces owner-self-nomination — only the generic sensitive-path hit. With default sentinel.sensitive_paths the PR is still labelled, just with a weaker headline; but sensitive_paths replaces the defaults when set (operator-reference.md:1093), so an operator whose list omits OWNERS gets no alert at all on the App copy, which is exactly the "author is trusted" escape this PR closes for the trust decision. The body lists "the findings themselves" as deliberately unchanged, so this may be intended scope — but passing codeAuthor into sentinel.Evaluate at :418 is a one-token change that makes the judgement consistent (and would make exempt_logins follow the code author too, which seems the right reading of "judged by the adopted PR's author"). Fine as a follow-up if you'd rather keep this PR tight; I'd note it in the fragment either way.
No other findings. Docs row and changelog fragment match the behaviour. I have not approved or changed anything.
— hive: agent=reviewer backend=copilot model=claude-fable-5.1 copilot=1.0.88
Summary
The hive can supersede a flagged contributor PR with a PR of its own that starts at the same commit. Sentinel then judged that PR by its author, the App, so a blocking alert turned into an "author is trusted (hive agent)" notice and nothing stopped a merge lane.
Live case, on a hive running 92878ca, 2026-10-26:
44428f3,sentinel-alertadded.44428f3and on every later head. Only the carried workflow-path fence kept it out of an unattended merge.That shape came from a fork. Since #14306 a fork PR is no longer re-homed into the base repository, so it does not recur. Every other supersede path still exists for a same-repo branch whose author is not a repository maintainer (#14305 excludes maintainers):
.github/workflows/);dco-rehome;signoff-rehome;in-place-stalled.Every one of them opens an App-authored PR through
supersedeAdoptedPRand gets the same notice. An in-place adoption is unaffected, because the PR keeps its contributor author.The adoption code already guards the same laundering for the contributor lane's diff fences (
automerge_adoption_fence.go: "a fenced diff re-homed into an App-authored PR that the self-authored lane then merges on its own"). The Sentinel decision had no such guard.Change
trySentinelPR: on a PR the App opened whose body carries the supersede lineAdoptionPRBodywrites (Supersedes #N (…; original commits by @login preserved).), the trust decision uses the adopted PR's author (sentinelCodeAuthor,AdoptedFromin the newsentinel_adopted.go).sentinel-alerton the App's PR as well.sentinelOriginCleared, the proofsentinelClearedByMaintaineralready uses) instead of asking again.sentinel.trusted_authors_blockrow inoperator-reference.mddescribes the rule.Unchanged:
exempt_logins(still read for the PR author);Testing
Tests run from
src/, with-count=1:go test ./pkg/github/... ./pkg/sentinel/... ./cmd/hive/...: allok.go vet ./pkg/github/is clean.gofmtis clean.src/scripts/check-changelog-fragment.sh v6passes.New tests in
sentinel_adopted_test.gorun the real sweep against the fake GitHub:TestSweepSentinelAdoptedPRJudgedByAdoptedAuthorsentinel-alertand an alert naming #930 and its author. A second case replaces a notice an older hive already posted for that head.TestSweepSentinelAdoptedPRCarriesOriginClearanceForSameHeadTestSweepSentinelAdoptedPRUnchangedCasesTestAdoptedFrom[bot]authors. A quoted line, prose, and the bot-takeover body are not matched.TestAdoptionPRBodyNamesTheAdoptedAuthorForSentinel(automerge)AdoptionPRBodywrites parses back to the adopted PR and author, so the two cannot drift apart.Mutation checks
Each part of the fix was broken in turn and restored from a backup, and each break failed the expected tests:
Not verified
automerge_bot_takeover.go), which re-authors a PR a workflow opened withGITHUB_TOKEN.Contributor checklist
changelog.d/fixed-sentinel-adopted-pr-judged-by-adopted-author.md.v6.🤖 Generated with Claude Code
https://claude.ai/code/session_01BQYnPGFJx81c8JXR3o1sg1