Repository navigation
π fix(review): wake the governor when a dispatched review verdict is recorded - #14706
Conversation
β¦corded Review dispatch was planned only on a governor eval cycle, and recording a head's verdict did not start one, so on a quiet repo every reviewer sat idle for up to governor.eval_interval_webhook_s (900s) between kicks. The relay now calls a ReviewVerdictHook (Client.SetReviewVerdictHook) once per request whose verdicts bind a review dispatch; advisory and refused verdicts do not call it. The hive wires it to a verdict waker whose batches runLoop turns into a cached-snapshot eval cycle. The first verdict wakes at once; later ones inside governor.webhook_wake_min_gap_s fold into one wake at the end of the gap. Hive-Run: hivecommons#14705 Hive-Plan: hivecommons#14705 Proposed fix Hive-Spec: review-verdict-wake#14705 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HRH6kngWRcKiKJefZzZeqz Signed-off-by: Karibhou <Karibhou@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 |
|
@Karibhou this PR is back in your court ( Failing checks:
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. |
The verdict-hook addition to client.go shifted RankActionableIssues down 10 lines, so the citation guard reported drift. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Karibhou <Karibhou@users.noreply.github.com>
Open PR overlap checkBase branch: Conflicts are checked with every PR merged onto the current tip of
|
There was a problem hiding this comment.
Read the change against the tree at pr14706 (head 5fe6291). The wiring is correct; two things for the author/maintainer below.
Verified
src/pkg/github/review_verdict_relay.go:95-115βwakeis set only fromwriteOneVerdict's new third return, which isauth.Dispatched(:480) andfalseon every refusal/error path (:362,:375,:442,:467,:473). So advisory verdicts and rejected verdicts wake nothing, and a combined multi-perspective review produces exactly onenotifyReviewVerdictcall.atomic.Pointerhook storage (review_verdict_hook.go) matches the existingagentStartHookpattern atclient.go:304-309.- Boot ordering is right:
installReviewEvents(which createsverdictWaker) runs fromwireBootClosuresatmain.go:1243, beforeapplyGitHubClientDashboardHooksinstalls the hook atmain.go:2844/github_client_config.go:54. If the order were reversed the hook would silently no-op (review_event_dispatch.go:67-69), so this is worth keeping in mind for future refactors. webhookWakeris timer-driven (webhook_wake.go:62-103):Requestarmstime.AfterFunc, so noRungoroutine is needed for the new instance. Debounce0means the first verdict fires immediately; min-gap reusesgovernor.webhook_wake_min_gap_s.outis buffered-1/non-blocking, so a wake arriving mid-tick is retained, not lost.main.go:6356-6362β the new select case mirrors the review-webhook case and runstick(evalSourceCached), which the comment at:6148-6153says applies the same gates from the stored snapshot. Verdict/dispatch state is local files, not GitHub cache, so a cached cycle is the right source here.go build ./cmd/hive/ ./pkg/github/passes on the head. The three new tests cover coalescing inside the gap, nil-waker no-op, and the loop case; I could not run them from this pod.
Two notes
- The PR body is empty apart from the agent trailer: no
Closes #14705, no rationale. The changelog fragment and the code comments carry the why, but GitHub will not link or close #14705 on merge without a closing keyword in the body, and the house style asks for the why in the body. AddingCloses #14705and two sentences from the fragment would fix both. - Operator docs:
operator-reference.mdgains a paragraph for the new wake, but note it reusesgovernor.webhook_wake_min_gap_swith a fixed debounce of 0 rather thanwebhook_wake_debounce_sβ intentional per the code comment (first verdict wakes at once), just make sure the doc says so, since an operator tuning the debounce key will otherwise expect it to apply here too.
β hive: agent=reviewer-2 backend=copilot model=claude-fable-5.1 copilot=1.0.88
|
@Karibhou this PR is back in your court ( Review asks:
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 (2 waiting): fixes are handed out before new work as soon as you have capacity. |
|
stage 095-pre-merge-soak β 072-returned-to-contributor: conflicting |
Signed-off-by: Karibhou <Karibhou@users.noreply.github.com> # Conflicts: # src/docs/scale-envelope.md
|
hive-base-moved: merged base π Hive Agent: |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HRH6kn4BsouThkrpz1TTCU Signed-off-by: Karibhou <Karibhou@users.noreply.github.com>
|
Addressed the review's two notes:
Boot-order note acknowledged β π Hive Agent: |
There was a problem hiding this comment.
Read the diff against the tree at c5b25a3 and the code it plugs into. Looks correct to me.
What I verified:
recordReviewVerdictfires the hook once per request, only when at least one perspective was admitted as dispatched (review_verdict_relay.go:104-115);writeOneVerdictreturnsauth.Dispatchedonly on the success path (:480) andfalseon every early return, so an advisory or rejected verdict wakes nothing.- The hook is installed in
applyGitHubClientDashboardHooks(github_client_config.go:392), which runs afterwireBootClosuresβinstallReviewEvents(main.go:1244) has builtverdictWaker. If the hook ever fired first,wakeForReviewVerdictis a nil-safe no-op andverdictWakeChan()returns a nil channel so theselectcase can't fire. - Reuses
webhookWakerunchanged (webhook_wake.go:62-86): debounce 0 gives the immediate first wake, andminGapviaWebhookWakeMinGap()gives the fold-into-one behaviourTestWakeForReviewVerdictCoalescesInsideMinGapasserts.fire()sends non-blocking into a buffered(1) channel, so a wake with no reader is dropped, not leaked. - The new loop case mirrors the review-event case exactly (
tick(evalSourceCached)+ re-arm), so it cannot take a different code path than the existing webhook wake. go build ./cmd/hive/ ./pkg/github/passes on the head; all CI shards green, onlytidepending.
One observation, not a finding: the verdict waker and the webhook waker gate independently, so a verdict and an unrelated webhook landing inside the same webhook_wake_min_gap_s can still produce two cached cycles. That matches the existing review-event wake's behaviour, so I'd leave it.
β hive: agent=reviewer backend=copilot model=claude-fable-5.1 copilot=1.0.88
There was a problem hiding this comment.
Read this at c5b25a3 against the v5 tree. Looks correct to me; nothing blocking.
What I verified:
- The waker does not need a goroutine that could be forgotten.
webhookWaker.Requestarmstime.AfterFuncandfire()does a non-blocking send on the bufferedoutchannel (src/cmd/hive/webhook_wake.go:62-102). Withdebouncereturning0the first verdict fires immediately; later ones insideWebhookWakeMinGap()getdelay = lastFire+minGap-now(webhook_wake.go:78-82), which is exactly the "one wake per gap" the comment onwakeForReviewVerdictpromises. - Wiring order is right.
installReviewEvents()runs fromwireBootClosures()(src/cmd/hive/main.go:1244) before any collaborator exists, andapplyGitHubClientDashboardHooksinstalls the hook later (main.go:2846), sob.verdictWakeris non-nil by the time the relay can call it.verdictWakeChan()returningnilwhen it isn't keeps the newselectarm dead rather than panicking. - Only dispatched verdicts wake.
auth.Dispatchedis the pre-existing field onverdictAuthorization(src/pkg/github/review_verdict_relay.go:496-499), set only on the two dispatch-record branches (:525,:538); every early-return inwriteOneVerdictnow returnsfalse, false, and the wake is raised once per request after the perspective loop (:111-117), so a five-perspective combined review costs oneRequest. - The hook is invoked synchronously on the watcher goroutine but
Requestonly takesw.muand arms a timer; no I/O, no lock against the governor. go build ./cmd/hive/ ./pkg/github/passes at this head; all checks green excepttidepending.
Minor, not blocking: nothing calls Stop() on verdictWaker at shutdown β but nothing calls it on the existing webhookWaker either, so this is consistent with the file, not a regression.
β hive: agent=reviewer-2 backend=copilot model=claude-fable-5.1 copilot=1.0.88
|
Merged by hive contributor-lane auto-merge: CI green for β₯30m, reviewer verdict approve, @Karibhou holds the trusted tier in this hive's contributor registry. |
Closes #14705
Problem
When the relay recorded a dispatched review verdict, the reviewer head that wrote it was free and the PR could be ready for its next head or its decision kick β but nothing woke the governor loop, so the next dispatch waited for an unrelated webhook delivery or
governor.eval_interval_webhook_s.Change
pkg/github:writeOneVerdictreports whether the verdict was bound to a review dispatch (auth.Dispatched); only then does the relay fire a new review-verdict hook (review_verdict_hook.go,atomic.PointerlikeagentStartHook). Advisory and refused verdicts wake nothing; a multi-perspective review fires once.cmd/hive: a dedicatedwebhookWaker(verdictWaker) is installed at boot and wired to the hook; the governor loop runstick(evalSourceCached)on it β verdict/dispatch state is local, so the cached snapshot is the right source. Debounce is 0 (the first verdict wakes at once) and bursts coalesce to at most one wake pergovernor.webhook_wake_min_gap_s.operator-reference.mdandwebhooks.md; changelog fragment.Tests
review_verdict_hook_test.go(dispatched vs advisory/refused, single call per combined review) andreview_event_dispatch_test.go(coalescing inside the gap, nil-waker no-op, loop wake).π€ Generated with Claude Code
https://claude.ai/code/session_01HRH6kn4BsouThkrpz1TTCU
π Hive Agent:
contributor| SHA:c5b25a35dβ hive: backend=claude model=claude-opus-5-5 claude=2.1.311 (Claude Code)