Repository navigation
🐛 Delegated decision on a capability gap: model the blocker, stop the un-park/re-park loop, name the missing App permission - #15134
Conversation
kubestellar-prow
Bot
added
the
dco-signoff: yes
|
[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 |
kubestellar-prow
Bot
added
the
size/XXL
|
@clubanderson is auto-promoted for contributor-lane merging on this PR: the last 5 merged PRs by this author in this hive's repositories have no revert or sentinel-alert evidence. CI, reviewer verdict, diff fences, soak, holds and branch protection still apply. |
Open PR overlap checkBase branch: Conflicts are checked with every PR merged onto the current tip of
|
github-actions
Bot
added
the
conflicts-with-open-pr
hivecommons-hive
Bot
added
author/human
|
@clubanderson this PR is held at Failing checks:
Next: the hive does not push to your branch. Push a new head (or reply here); CI and the review swarm re-run automatically and the PR returns to the normal path. |
hivecommons-hive
Bot
added
stage/050-review-pending
hivecommons-hive
Bot
added
stage/071-needs-author
|
@clubanderson this PR is held at Failing checks:
Next: the hive does not push to your branch. Push a new head (or reply here); CI and the review swarm re-run automatically and the PR returns to the normal path. |
hivecommons-hive
Bot
added
stage/050-review-pending
|
@clubanderson this PR is held at Failing checks:
Next: the hive does not push to your branch. Push a new head (or reply here); CI and the review swarm re-run automatically and the PR returns to the normal path. |
hivecommons-hive
Bot
removed
the
stage/071-needs-author
|
@clubanderson this PR is held at Why:
Next: the hive does not push to your branch. Push a new head (or reply here); CI and the review swarm re-run automatically and the PR returns to the normal path. |
![hivecommons-hive[bot]](https://avatars.githubusercontent.com/in/3568013?s=60&v=4){kind=small}
There was a problem hiding this comment.
Intent alignment vs #15099 — the diff covers every remaining item of the issue (blocker model, no-effect + options, loop guard, label reconcile, pill, /api/status.capabilities), but it was cut before #15127 landed on v5 as the partial fix for the same issue, and the PR is currently CONFLICTING.
1. Two mechanisms now answer the same "capability gap" case with different verdicts (in-scope).
This branch's merge-base 8785546 is 69 commits behind #15127's merge 77d935f. On v5, src/pkg/delegatedecision/policy.go already has CapabilityGap(in) → recommendOnly("capability-gap", …) and a capability-gap glossary rule. This PR adds, in src/pkg/dashboard/api_repo_item_decide_delegated.go:484-499, an earlier short-circuit that returns VerdictRefused with Rule: "blocker:"+b.Kind and the new blocker glossary reason (src/pkg/delegatedecision/decision_glossary.go:186-188). After rebase, which of the two fires depends on whether pkg/blocker.Infer (src/pkg/blocker/blocker.go:173-197) or capabilityGapRE matches first — same situation, two rule names, two verdicts, two popover texts. Neither the PR body nor changelog.d/fixed-capability-blocker-decision.md mentions #15127, and the fragment re-describes the loop fix that changelog.d/fixed-let-hive-decide-capability-gap.md already shipped.
Suggested remedy: rebase onto v5, then make the policy rule defer to the structured blocker so there is one path — e.g. in policy.go:
- if CapabilityGap(in) {
- return recommendOnly("capability-gap", …)
- }
+ // Capability gaps are decided before policy runs, from the structured
+ // blocker (pkg/blocker); this rule only catches prose the inferrer
+ // does not recognise and reuses its glossary family.
+ if CapabilityGap(in) {
+ return recommendOnly("blocker:capability", …)
+ }(or drop CapabilityGap and move its regex into pkg/blocker's inference table), and trim the fragment to the delta over #15127 (structured blocker, re-check endpoint, label reconcile, pill, capabilities).
2. Advisory — deliberate deviation from issue item 4. #15099 says "otherwise approved-direction wins"; src/pkg/stageengine/derive_issue.go issueParkClear keeps 045 and clears approved-direction in both cases. The PR body states and justifies this (the un-park removes needs-human in the same write, so a needs-human seen beside the ack is the newer signal). The reasoning holds; just make sure the docs row in labels-and-control-signals.md says this rather than the issue's wording. No change requested.
— hive: agent=adjudicator-3 backend=copilot model=claude-fable-5.1 copilot=1.0.88
…stop re-parking loops An issue the scanner parks because the GitHub App cannot push .github/workflows/* (no workflows permission) is a capability gap, not a question: a direction decision cannot clear it, and "Let hive decide" un-parked #14976 only for the sweep to re-apply the same needs-human text an hour later. - pkg/blocker: structured blocker kinds, the <!-- hive:blocker kind=… --> marker, prose inference, and a persisted ledger with a delegated-unpark loop guard and a cleared-at staleness guard. - Record blockers where comments are read: issue_comment webhook, task-list sweep (adds the marker to the remainder comment), gh-wrapper (adds the marker to a needs-human comment that names a missing capability). - Stage engine: blocker parks 045 with "blocked: <short>"; needs-human beside approved-direction keeps 045 and clears the ack (needs-human is the newer signal by construction). - Dashboard: decision_needed.blocker/options populated with real handlers (App settings link, Re-check permissions, close as partially addressed, wait; patch hand-off disabled with its reason), delegated decision short-circuits to no-effect with the glossary 'blocker' reason, the row pill reads "Blocked · App lacks workflows permission", ⚙ → Security and /api/status.capabilities name missing permissions and blocked counts. - Tests: Go (blocker, ledger, webhook, sweep marker, stage facts/derive, dashboard handlers, glossary) and node fixtures (pill, options, popover, capability text); gh-wrapper bash tests. - Docs: labels-and-control-signals (blocker marker + kinds + reconcile rule), dashboard-sections, operator-reference (App permissions table), api-reference; changelog fragment. Fixes #15099 Signed-off-by: Andrew Anderson <andy@clubanderson.com>
Follows #15087's option consequences so a blocked row's ways out say what each one does. Signed-off-by: Andrew Anderson <andy@clubanderson.com>
Signed-off-by: Andrew Anderson <andy@clubanderson.com>
Signed-off-by: Andrew Anderson <andy@clubanderson.com>
…call sites, import ratchet - pkg/github already has a test helper named blocker; import pkg/blocker as itemblocker there. - Ledger.Record drove the loop guard off the preserved first-report time, so a capability re-report after a delegated un-park never tripped it; use the observation time. Observe returns changed=false for a report older than the entry's cleared-at. - stageTrackerDecisionNeededWith test calls pass the clock. - Allow pkg/dashboard → pkg/blocker in the import ratchet (the blocker ledger is the seam the tracker, delegate and status read). Signed-off-by: Andrew Anderson <andy@clubanderson.com>
Signed-off-by: Andrew Anderson <andy@clubanderson.com>
… duplicate JS const Signed-off-by: Andrew Anderson <andy@clubanderson.com>
…ases; resync api-reference citations after rebase Signed-off-by: Andrew Anderson <andy@clubanderson.com>
…the write-back ratchet Signed-off-by: Andrew Anderson <andy@clubanderson.com>
Signed-off-by: Andrew Anderson <andy@clubanderson.com>
1a0e000
ℹ️ Sentinel notice — informational; author is trusted (owner), merge is not blockedHive flagged this PR (author @clubanderson, head
Hive did not add the |
There was a problem hiding this comment.
Security — delta re-review at 1a0e000 (previous head a35b45a). The earlier security finding is still present, and the delta adds a second ingestion path with the same gap.
Blocker ledger trusts comment text from any author (medium, in-scope).
pkg/blocker.Ledger.Observe (src/pkg/blocker/ledger.go:128-146) records a capability blocker or loop-guard reason from whatever text it is handed. Both callers added in this delta hand it every comment with no author check:
src/pkg/github/webhook_cache.go:495-502—issue_commentcreated/edited →itemblocker.Shared().Observe(configured, n, body, at).p.Comment.User.Loginis parsed (webhook_cache.go:449-451) but never read.src/pkg/dashboard/api_repo_item_decide_delegated_plan.godelegatedCommentTexts—Issues.ListComments→commentText{Body, At};c.GetUser()is dropped, anddelegatedBlocker(src/pkg/dashboard/item_blocker.go:210-216) re-observes all of them before every delegated decision.
Consequence: anyone who can comment on a public repo issue can write "needs-human: the App cannot push to .github/workflows" (matches Infer, src/pkg/blocker/blocker.go:211-217) and thereby (a) make Let hive decide return no-effect on that issue — decideDelegatedOne short-circuits on the ledger for every KindIssue regardless of labels (api_repo_item_decide_delegated.go:492-500); (b) flip the global /api/status.capabilities / ⚙ Security card to "missing workflows · N items blocked" (capabilitiesStatus, item_blocker.go:240-250, reads the ledger with no label or author filter); (c) after a delegated un-park, trip the human-only:re-asked loop guard by repeating the recorded reason (ledger.go:180-204); (d) have up to 280 chars of their text (ReasonSentence, blocker.go:233) quoted under the hive's identity in the close-partial comment (item_blocker.go:394-403). No write to GitHub is gained, so this is control-plane DoS/spoofing, not privilege escalation — but it is a new trust boundary this PR introduces for the blocker model #15099 asks for.
Suggested fix — gate both callers on a hive-authored comment (the reports the PR describes all come from agents via bin/gh-wrapper.sh, the task-list sweep, or the hive bot login):
--- a/src/pkg/github/webhook_cache.go
+++ b/src/pkg/github/webhook_cache.go
@@ func (c *Client) HandleWebhookEvent(event string, payload []byte) (WebhookEventR
- if body, at, ok := webhookIssueCommentBody(event, p); ok {
+ if body, at, ok := webhookIssueCommentBody(event, p); ok && p.Comment.User != nil && c.IsHiveAuthorLogin(p.Comment.User.Login) {--- a/src/pkg/dashboard/api_repo_item_decide_delegated_plan.go
+++ b/src/pkg/dashboard/api_repo_item_decide_delegated_plan.go
@@ func (s *Server) delegatedCommentTexts(ctx context.Context, repoFull string, number int) []commentText {
for _, c := range comments {
+ if !s.deps.GHClient.IsHiveAuthorLogin(c.GetUser().GetLogin()) {
+ continue
+ }
out = append(out, commentText{Body: c.GetBody(), At: c.GetCreatedAt().Time})
}If maintainer-written needs-human reports must also count, use the repo permission check already in gatherDelegatedInput (api_repo_item_decide_delegated.go:653-657) instead of accepting all authors. Please add a test with a non-hive commenter asserting Blocks() stays false.
Other delta items checked from this perspective, no findings: blocker/recheck and blocker/close-partial are gated by canToggleRepoHold + X-Hive-User and audited (item_blocker.go:277-291, 336-350); InstallationInfo.Permissions exposes only permission levels (src/pkg/github/app.go:642-663); _blocker_kind in bin/gh-wrapper.sh:1188-1215 only pattern-matches the body as a regex subject — no expansion of user text.
— hive: agent=adjudicator-3 backend=copilot model=claude-fable-5.1 copilot=1.0.88
There was a problem hiding this comment.
Intent-alignment re-review at head 1a0e000d (rebased onto v5 past #15127; the earlier merge-conflict finding is resolved). Judged against #15099.
1. issueParkClear strips a legitimately recorded approved-direction (still present; deviates from issue item 4) — src/pkg/stageengine/derive_issue.go:82-88 clears the ack whenever needs-human + 045 are present. The comment above it says the ack is "by construction" stale because "the un-park that applies the ack removes needs-human in the same write". That holds for /hive approve and the delegate (DecisionLiftsNeedsHuman always returns true, facts_sink.go:93-96), but not for the label-approval path: tryLabelApproval adds approved-direction while deliberately keeping a hive-added needs-human (src/pkg/github/issue_unpark_command.go:556-571; label_approval.go:282-285,394 — "The approval is still recorded, but the issue is not released while one is present", and the reply tells the maintainer "needs-human (held for a person; only a person removes it)"). The next stage pass now deletes that recorded approval, and f.Acted (label_approval.go:401, timeline-based) stops it being re-applied. #15099 item 4 says "otherwise approved-direction wins"; the PR argues for the opposite to stop the loop, but the loop guard already records human-only:re-asked as a blocker in that case, so gating the clear on a blocker keeps the fix and stops the regression:
if !f.Park.NeedsHuman || label == "" || stage != stagelabels.IssueNeedsDecision || strings.TrimSpace(is.Blocker) == "" {
(and drop the "by construction" paragraph in the doc comment; adjust the derive_issue_test.go plain-case expectation.)
2. UI/doc text promises an automatic re-check that is not implemented — src/pkg/dashboard/item_blocker.go:121 ("the hive re-tries on its own once it sees …: write"), :170, :322 ("The hive re-tries on its next pass"), src/docs/operator-reference.md:1813 ("The hive re-checks on its own on the next stage pass"), src/docs/dashboard-sections.md:729. The only callers of InstallationInfo.Granted and Ledger.ClearKind are inside handleRepoItemBlockerRecheck (item_blocker.go:277,317,319); no stage pass or cadence tick probes permissions. Either add that probe or reword to "press Re-check permissions once the permission is granted".
3. Advisory (no change required): #15099 was already closed as completed by #15127 (capability-gap regex rule, delegatedecision/policy.go:374), which still runs as a fallback after the new blocker short-circuit at api_repo_item_decide_delegated.go:493-504. The PR body and changelog.d/fixed-capability-blocker-decision.md describe the whole fix without mentioning #15127; a one-clause "completes the remainder of #15127's partial fix" would keep the record straight.
— hive: agent=adjudicator-4 backend=copilot model=claude-fable-5.1 copilot=1.0.88
|
@clubanderson this PR is held at Why:
Next: the hive does not push to your branch. Push a new head (or reply here); CI and the review swarm re-run automatically and the PR returns to the normal path. |
There was a problem hiding this comment.
Correctness — delta re-review at 1a0e000d (previous head a35b45a6; the compare is a rebase onto v5 plus fix commits 2dbd1fd…1a0e000d). Both earlier correctness findings are still present; the new commits don't touch either mechanism.
1. Still present — issueParkClear strips an approved-direction the label-approval path deliberately leaves next to needs-human (src/pkg/stageengine/derive_issue.go:82-89)
The label-approval sweep classifies needs-human as a kept park label (src/pkg/github/label_approval.go:282-285, :393-394), still adds approved-direction while it is present (src/pkg/github/issue_unpark_command.go:559-567 — Blocked() only looks at BlockingParkLabels, :322), and posts "Left in place: needs-human (held for a person; only a person removes it)" (label_approval.go:486-487). The next stage pass then clears that ack because f.Park.NeedsHuman && label != "" — no blocker, no loop guard involved. The posted comment and the label state now contradict each other, and the person's recorded acceptance is lost once they later remove needs-human.
The loop you are stopping already surfaces in the ledger as a blocker (human-only:re-asked is set as e.Blocker in src/pkg/blocker/ledger.go:195-198), so the reconcile can be scoped to it without losing the fix:
if !f.Park.NeedsHuman || label == "" || stage != stagelabels.IssueNeedsDecision || is.Blocker == "" {
return nil
}
(and update derive_issue_test.go's plain-case expectation accordingly.)
2. Still present — a recorded blocker is never released when the gap is resolved out-of-band (src/pkg/dashboard/item_blocker.go:53-58, :210-216)
delegatedBlocker returns whatever the ledger holds; the only writers that drop a blocker are close-partial (item_blocker.go:384) and a granted recheck (:319). Observe of a later plain needs-human report keeps the existing Blocker (ledger.go:180-203 only sets e.Blocker when nil). So if a person pushes the workflow change themselves and removes needs-human, and an agent later parks the issue for an unrelated reason, "Let hive decide" still answers no-effect "App has no workflows permission" (api_repo_item_decide_delegated.go:493-501), and the stage pass still reports blocked: App lacks workflows permission (derive_issue.go:135-141). Nothing clears the entry on issue close either (Clear( has one caller), so /api/status.capabilities.blocked_items keeps counting closed issues.
Concrete fix: when a newer needs-human report carries no detectable kind, treat it as superseding the capability blocker —
--- a/src/pkg/blocker/ledger.go
+++ b/src/pkg/blocker/ledger.go
@@ func (l *Ledger) RecordReason(repo string, number int, reason string, at time.Time) Entry {
e.Repo, e.Number = repo, number
+ // A later park that names no capability supersedes a recorded one: the
+ // gap was resolved out-of-band and the issue is held for a new reason.
+ if e.Blocker != nil && e.Blocker.Source != SourceLoopGuard && at.After(e.ReportedAt) && reason != e.Reason {
+ e.Blocker = nil
+ e.Reapplied = false
+ }
e.Reason = reason
e.ReportedAt = atplus a blockerLedger().Clear(repo, n) from the closed-issue sweep (src/pkg/github/closed_issue_stage_sweep.go) so closed items leave the capabilities count.
Other perspectives' earlier findings (docs, intent/#15127 overlap, webhook author gate, close-partial body) were not re-judged here.
— hive: agent=adjudicator backend=copilot model=claude-fable-5.1 copilot=1.0.88
|
merged by Hive trusted-author auto-merge: author @clubanderson holds role owner and GitHub permission admin on repo; CI green at 1a0e000. |
|
Thank you for your contribution! Your PR has been merged. We'd love to hear how your experience was: share feedback |
Fixes #15099
What happened on hive's hive
#14976 was parked
needs-humanby the scanner because the remaining retry wrappers live in.github/workflows/*, "which the App token cannot push (noworkflowspermission)". The operator ran Let hive decide → the delegate appliedapproved-directionand un-parked it → the scanner re-appliedneeds-humanwith the same text an hour later → "I tried 'let hive decide' and it still needs human." The delegated decision answered a question nobody asked: the blocker is a capability gap, which no direction can clear, and the un-park → re-park loop would repeat forever.stage/045-needs-decisionandapproved-directionwere also both on the issue.What this PR does
pkg/blocker): kindscapability:workflows-permission,capability:repo-write,capability:branch-protection,capability:secret,human-only:cla,human-only:re-asked; a standard marker<!-- hive:blocker kind=… -->; prose inference for reports written without it; a persisted ledger (/data/item-blockers.json). Recorded at every comment path:issue_commentwebhook, task-list sweep (adds the marker to the remainder comment it writes), andbin/gh-wrapper.sh(adds the marker to a needs-human comment that names a missing capability).no-effect(reasonblocker, in the glossary) and writes nothing: "Hive can't do this one: the GitHub App has noworkflowspermission, so it cannot push changes to .github/workflows/*."decision_needed.optionscarries the ways out with real handlers: (a) Grant the App workflows permission → App settings link + Re-check permissions (POST …/blocker/recheckprobes the installation and releases everything blocked on that permission); (b) Prepare the patch for a person to push — disabled with its reason (no human-authorised push identity path exists today; checkedshared_uid_read_token/ operator PAT paths, both read-only or hive-identity); (c) Mark partially addressed and close (POST …/blocker/close-partial); (d) Keep waiting. "Let hive decide" stays listed but disabled with the sentence, not greyed silently.human-only:re-askedwhen no capability is named) and disables the delegate until the reason changes. Acleared_atguard stops re-observed old comments from re-blocking after a permission grant.approved-directionnever coexist. Rule:needs-humannext toapproved-directionis by construction the newer signal (the un-park removesneeds-humanwhen it applies the ack), so the stage pass keeps 045 and clearsapproved-direction, in both the blocker and plain cases. This deviates from "approved-direction wins" in the issue on purpose: a person or sweep that re-parks after a delegated decision must never be silently un-parked again — that is exactly the loop./api/status.capabilities = {missing: [...], blocked_items: N, by_permission: {...}}; ⚙ → Security shows GitHub App permissions: missingworkflows· N items blocked with the settings link.Tests
Go:
pkg/blocker(marker parse, inference, ledger persist/loop guard/cleared-at),pkg/github(webhook issue_comment → blocker, sweep marker, stage facts →blocked: …+ ack cleared),pkg/stageengine(derive reconcile),pkg/dashboard(decisionForBlocker options, delegated no-effect with no GitHub writes, loop guard, recheck 404/409/401/403/missing/granted/probe-failure, close-partial, status payload), glossary family lookup + SPA mirror. Node fixtures via stdin: pill word, blocker actions/options/url/role gating, popover HTML, request shapes,capabilityGapsText/securityCapabilitiesHtml.bin/gh-wrapper.test.sh: marker added / not duplicated / plain needs-human untouched (94 pass).Docs
labels-and-control-signals.md(Blockers section: marker, kinds, reconcile rule; glossary row),dashboard-sections.md(Blocked pill — capability gaps),operator-reference.md(GitHub App permissions table and what each unblocks),api-reference.md(new endpoints,capabilities,blockeron tracker items),changelog.d/fixed-capability-blocker-decision.md.