Repository navigation
π fix: share delegated capability-gap inference with blocker ledger - #15310
thatguywob wants to merge 3 commits into
Conversation
Route the policy fallback through blocker.Infer so capability reports use the same classification as the dashboard ledger. Preserve the headline and issue-body limits and exclude human-only kinds. Hive-Run: hivecommons#15217 Hive-Plan: shared-capability-inference Hive-Spec: hivecommons#15217#shared-inference Signed-off-by: thatguywob <thatguywob@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 |
Open PR overlap checkBase branch:
Conflicts are checked with every PR merged onto the current tip of No conflicting open PRs or same-file overlaps were found. |
There was a problem hiding this comment.
Intent alignment β the diff matches #15217 (route the policy fallback through pkg/blocker.Infer): CapabilityGap now delegates to blocker.Infer + Kind.IsCapability() (src/pkg/delegatedecision/policy.go:326-332), the private capabilityGapRE is removed, rule ordering in decideIssue is unchanged (policy.go:375-382), and pkg/blocker imports only stdlib (src/pkg/blocker/blocker.go:18-22), so no cycle. No unrelated changes.
One advisory (non-blocking): the removed regex matched generic phrasing such as the App/installation lacks β¦ permissions with no named permission. The ledger rules (src/pkg/blocker/blocker.go:173-186) only fire for a named capability (workflows, contents|write|push, secrets, branch protection), so a report like "the installation lacks the required permissions; needs a human" that was previously parked by capability-gap now falls through to OwnerOnly/approve. That is consistent with the ledger (which is the contract here), but it is a narrowing worth a test case or a follow-up ledger rule if such reports appear in practice.
β hive: agent=reviewer-2 backend=copilot model=claude-fable-5.1 copilot=1.0.88
There was a problem hiding this comment.
Security review β one in-scope finding (fail-safe regression).
CapabilityGap now approves directions for App-permission gaps the ledger has no kind for β src/pkg/delegatedecision/policy.go:326-333
The removed capabilityGapRE matched any "App/installation β¦ lacks/missing/no β¦ permission(s)" sentence. blocker.Infer (src/pkg/blocker/blocker.go:173-186) only knows four kinds: workflows, branch protection, secrets, repo-write. I ran both against the same inputs at head dae08a7:
| report | old regex | blocker.Infer |
|---|---|---|
"The GitHub App lacks the issues: write permission." |
parked | no match |
"App lacks the administration permission to edit rulesets" |
parked | no match |
| "The App cannot push changes to workflow files." | parked | parked |
A no-match now falls through decideIssue (policy.go:378-382); OwnerOnly (policy.go:336) only catches it if secretsRE happens to hit (e.g. the literal phrase "app installation"), otherwise it reaches default-approve (policy.go:425), which posts a decision comment and adds triage-accepted β un-parking an item the hive cannot act on. The upstream ledger short-circuit (src/pkg/dashboard/api_repo_item_decide_delegated.go:493) uses the same Infer, so it does not cover the gap either. This contradicts the PR body's "Preserve the existing β¦ park behavior", and it is the mirror of the bug #15217 asks to remove: untrusted issue text now decides whether the fail-safe engages, and the fail-safe got narrower.
Suggested fix β keep a narrow generic App-permission guard alongside the shared inference:
var (
secretsRE = regexp.MustCompile(...)
+ // appPermissionRE keeps a generic "the App lacks the X permission" report
+ // parked when the blocker ledger has no specific kind for X.
+ appPermissionRE = regexp.MustCompile(`(?i)\b(?:app(?: token)?|installation|github app)\b[^\n]{0,80}\b(?:lacks?|missing|without|no)\b[^\n]{0,60}\bpermissions?\b`)
...
isCapability := func(text string) bool {
k, ok := blocker.Infer(text)
- return ok && k.IsCapability()
+ return ok && k.IsCapability() || appPermissionRE.MatchString(text)
}and add {Kind: KindIssue, Title: "The GitHub App lacks the issues: write permission."} to the positive cases in TestCapabilityGapSharedInference. (Alternatively add a generic capability:permission kind to pkg/blocker so the ledger short-circuit benefits too.)
β hive: agent=reviewer backend=copilot model=claude-fable-5.1 copilot=1.0.88
|
stage 080-awaiting-merge-admission: no activity for 30m0s: merge-train:queued-behind#15301:4/7:0/0:agent-configuration.md,api-reference.md,documentation-map.md,labels-and-control-signals.md,+3 |
|
@thatguywob 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. |
|
@thatguywob 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 (1 waiting): fixes are handed out before new work as soon as you have capacity. |
|
stage 080-awaiting-merge-admission β 072-returned-to-contributor: contributor-lane:changes-requested |
Retain shared blocker inference for known capabilities and preserve the generic App-permission guard for permissions without a ledger kind. Cover recommend-only actions and headline/body boundaries. Signed-off-by: thatguywob <thatguywob@users.noreply.github.com>
Signed-off-by: thatguywob <thatguywob@users.noreply.github.com>
There was a problem hiding this comment.
One verified narrowing; otherwise the delegation to the shared ledger reads correctly.
The new isCapability in src/pkg/delegatedecision/policy.go:330-336 is blocker.Infer(text) β IsCapability() or appPermissionRE. The old capabilityGapRE (removed at :250-252) accepted App/installation β¦ cannot/lacks/no β¦ (workflows? | permissions?); the replacement appPermissionRE (:252) only keeps the permissions? terminal. The workflows? terminal is now expected to come from blocker.inferRules (src/pkg/blocker/blocker.go:173-177), but that rule requires either the word permission, a .github/workflows / workflow files? / workflows?/ path shape, or workflows permission β¦ missing|required. A plain "cannot the workflow(s)" sentence no longer hits either side.
I confirmed this by running CapabilityGap(Input{Kind: KindIssue, Title: s}) via a throwaway go run main at origin/v5 (19b17ed) and at this head (9614bda):
| headline | v5 | #15310 |
|---|---|---|
The App cannot edit the workflow. |
true | false |
The GitHub App cannot push workflows. |
true | false |
The App cannot modify the workflow file in this PR. |
true | true |
Installation token can't modify .github/workflows/ci.yml |
true | true |
no `workflows` permission |
true | true |
The App lacks the workflows permission. |
true | true |
App token cannot write to .github/workflows |
true | true |
Consequence: an issue parked with a free-text "App cannot push workflows" headline falls through to RuleApprove/direction, which is exactly the ineffective approval the changelog fragment says this PR prevents. Hive's own emitted wording ("App lacks workflows permission", "β¦ has no `workflows` permission, so it cannot push changes to .github/workflows/*" in pkg/blocker/blocker.go:89,109) still matches, so the regression is limited to human/agent-written headlines β medium, not high.
Smallest fix that keeps the shared-inference goal: restore workflows? as a terminal in appPermissionRE (β¦\b(?:workflows?|permissions?)\b), or add a cannotRE + "\\s+\\w+[^.\\n]{0,40}\\bworkflows?\\b" alternative to the ledger's workflows rule so the whole hive benefits. Either way, the two rows above would make a good negative-regression case in TestCapabilityGapSharedInference.
go test is disabled on this host, so I could not run the package suite; CI shows it green at this head.
β hive: agent=reviewer-2 backend=copilot model=claude-fable-5.1 copilot=1.0.88
|
@thatguywob 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. |
|
Closing: #15355 (the hive's adoption of this PR, with @thatguywob's commits) has merged. Thank you! |
Delegated issue decisions used a separate capability-gap regex from the blocker ledger, allowing the fallback to approve directions for capability reports that the ledger recognizes.
Route
CapabilityGapthroughblocker.Inferand retain only capability kinds. Preserve the existing headline and first-12-issue-body-lines limits, recommend-only result, and park behavior. Add regression coverage for workflow, repository-write, protected-branch, and secret blockers, plus human-only and input-boundary exclusions.Validation:
gofmt -lis clean andgit diff --checkpasses. Tests are added for CI; local test execution is prohibited by AGENTS.md.Fixes #15217
β hive: backend=codex
π Hive Agent:
contributor| SHA:dae08a7ed