Repository navigation
π fix(sec-check): request Dependabot alerts read permission and document it - #17442
Conversation
β¦ent it The sec-check policy calls the dependabot/alerts endpoint, but the contributor-tier App token never requested vulnerability_alerts, so the call returned 403 and the permission was undocumented. Request it as optional (retry without it when the installation has not accepted it) and add it to the App permissions table. Signed-off-by: scanner <280983584+hivecommons-hive[bot]@users.noreply.github.com> Co-authored-by: williamdalessandro <231250207+williamdalessandro@users.noreply.github.com>
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. |
|
Hive status β updated in place; edits do not notify. Full history is in the Activity Monitor. hive/awaiting-ci: CI pending β sweep will merge when green. (The vulnerability_alerts fallback now retries only on GitHub's 422 permission refusal, so a hung endpoint keeps its single bounded mint window and a repo-scope 422 still costs exactly one mint attempt.) π Hive Agent: β hive: agent=scanner backend=copilot model=claude-fable-5.1 copilot=1.0.88 Review findings from Code reads correct to me; I am routing it to a maintainer only because it changes what the GitHub App asks for, which is a permission decision rather than a code one. What I verified in the tree, not just the diff:
Decision for a maintainer: the App manifest itself must declare Dependabot alerts: Read-only and every existing installation must re-accept it once (the token request is optional and degrades to a 403 on the alerts call, so nothing breaks before that). That is an App-wide grant even though only the sec-check token requests it. If that is acceptable, I see nothing in the code that should hold this. Evidence bundle `#174420a08d9a` sha256:0f40e100d5d1c877d0c6b665beecc75191303d79ffb88b5d8481c2c589720e6c β download from the dashboard or `hivectl review evidence #17442`. |
Mutation testing (advisory)No surviving mutants in changed files (or the run hit its time budget before reporting any). Advisory only; this job never blocks the PR (#17349). |
There was a problem hiding this comment.
Decision needed: @clubanderson should decide whether vulnerability_alerts: read must also be requested at the trusted/merger tiers (src/pkg/github/app.go:342-357), since a sec-check agent on an ISSUES_PRS_MERGE (L5) hive mints trusted and would still 403, or whether contributor-only is the intended scope.
Mechanically correct, but the fix only covers one of the two tiers sec-check mints at. Routing to a maintainer because this widens the App's requested permission set.
What I verified at ace9ea1:
src/pkg/github/app.go:424-440βopts.Permissionspoints at the same*InstallationPermissionsasperms, so clearingperms.VulnerabilityAlertsbefore the retry does change the second mint request. The new test asserts exactly that (two requests, second without the key,pull_requestsstillwrite).- The retry gate
perms.Workflows == nilkeeps it from colliding with the Workflows fallback; today onlycontributorsetsVulnerabilityAlerts, so it is correct, if a little indirect. installationPermissionMapgains the key, so the test helper and any permission diffing see it.
Gap (medium): the token tier is derived from agent mode, not role β src/crux/agentmode/agentmode.go:124-129 returns m.TokenTier() for every role except the advisory reviewer. The linked #17426 reproduces at ISSUES_AND_PRS (β contributor), which this PR fixes, but the same report says sec-check is run "at ACMM L4 or L5"; at L5 the agent mints trusted (src/pkg/github/app.go:342-357), which this PR leaves without vulnerability_alerts, so the gh api repos/<org>/<repo>/dependabot/alerts call in sec-check-holdgated.md still 403s there. The docs row (src/docs/github-app-setup.md:96) states the permission unlocks sec-check unconditionally, which would not be true for L5 hives.
Not a defect, noting only: the docs table already has a blank line before the Workflows row (pre-existing, not introduced here), so the new row renders as the last row of the first table.
I have not approved or changed anything.
β hive: agent=reviewer-2 backend=copilot model=claude-fable-5.1 copilot=1.0.88
β¦on refusal Signed-off-by: scanner <280983584+hivecommons-hive[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Decision needed: @clubanderson should decide whether the hive App may request Dependabot alerts read (vulnerability_alerts: read) on every contributor-tier token β all ISSUES_AND_PRS agents, not only sec-check β given every installation must re-accept the App permission for it to take effect.
Routing to a maintainer: the code is correct, but it widens what the App token asks for, and that is a permission decision, not a code one.
What I verified in the tree at 3a5882d:
src/pkg/github/app.go:329-338addsVulnerabilityAlerts: readto the contributor tier only. ISSUES_AND_PRS agents mint at the contributor tier (src/pkg/scheduler/workflow_push_ceiling_test.go:10), and sec-check is ISSUES_AND_PRS (src/pkg/config/packs/level-5.yaml:221-232), so the fix reaches the agent #17426 describes. It also reaches every other contributor-tier agent β the grant is per-tier, not per-role. Read-only, but broader than the issue's stated need.- The fallback at
app.go:432-440fires only onisPermissionNotGrantedMintError(app.go:470-477: HTTP 422 and "permission" in the message). A repo-scope 422 ("β¦does not exist or is not accessibleβ¦") does not match, so the hub's one-mint-per-repo accounting thatIsRepoScopeMintError(app.go:487-494) feeds is preserved.TestScopedToken_ContributorRepoScopeErrorDoesNotRetrypins that;TestScopedToken_ContributorVulnerabilityAlertsWithFallbackpins the degrade path. Theperms.Workflows == nilguard keeps it from colliding with the existing Workflows retry on trusted tiers. installationPermissionMap(app.go:828) gains the key, so the permission advisory/diff surfaces see it.- Docs row added to
src/docs/github-app-setup.md:96; changelog fragment present. Issue #17426 isCloses-linked.
Operator consequence a human should weigh: until each installation re-accepts the App permission, every contributor-tier mint takes two round-trips (first 422, then retry) and logs a warning per mint. That is the same trade the Workflows grant made, but it is now on the most common tier.
I have not approved or changed anything.
β hive: agent=reviewer backend=copilot model=claude-fable-5.1 copilot=1.0.88
|
Needs a decision, not a merge:
Head: verdict requires_human @3a5882d, 66/67 checks green, 1 pending, DCO yes, soak 0s of 10m0s. What to do: approve this head ( How to release it:
Clear the cause and the hive re-evaluates on its next pass; the stage moves to |
β¦le token Least privilege (#17426 reviewer decision): only an ISSUES_AND_PRS agent with role sec-check mints the new sec-check tier (the contributor set plus Dependabot alerts read), via the same role-refinement path the reviewer tier uses. Every other contributor-tier token is unchanged. The App itself still declares the permission, so each installation re-accepts it once. Signed-off-by: Andrew Anderson <andy@clubanderson.com>
|
/hive decision scope vulnerability_alerts:read to the sec-check role token only; App-level permission still needs the one-time re-accept |
clubanderson
left a comment
There was a problem hiding this comment.
Approved: scoped to sec-check token; App permission re-accept is one-time.
|
LGTM label has been added. DetailsGit tree hash: 0983632212f81f9e3ccfc5de32073e7967a68485 |
There was a problem hiding this comment.
Read the diff against the tree at c79ec2d, then the issue. This looks correct to me.
What I verified:
- Role plumbing is real, not just a constant.
src/crux/agent/manager_tokens.go:969andmanager_sandbox.go:179both callTokenTierForRole(m.agentMode(a), a.Config.Role), and the quickstart agent isrole: sec-check(src/deploy/hive-quickstart.yaml:119), so the new branch inagentmode.go:128actually fires for the shipped sec-check agent. - Default mode coverage.
crux.DefaultAgentMode(src/crux/agentmode.go:39-54) putssec-checkatISSUES_AND_PRSfor ACMM L4, L5 and L6, which is the mode the issue reports (L4/L5 holdgated). An operator who overridesmode: ISSUES_PRS_MERGEon sec-check gets the plaintrustedtoken and still 403s β the test atreviewer_tier_test.go:85pins that as intentional and the docs row says "ISSUES_AND_PRS agent with role sec-check", so it is consistent; just noting it as the one configuration the fix does not reach. - No fall-through to a read-only tier.
mint.ScopesForTier(src/pkg/mint/agent.go:57-62) falls back toadvisorscopes for an unknown tier, which would have silently demoted the hive-side mint token; the PR adds the"sec-check"entry atagent.go:50.ModeForTokenTier(agentmode.go:166) maps it back toISSUES_AND_PRS. - Retry is bounded to the right failure.
isPermissionNotGrantedMintError(app.go:484) requires a 422 whose message mentions "permission", and the guard atapp.go:443also requiresperms.Workflows == nil, so the repo-scope 422 (#7869) and the existing Workflows fallback never earn a second mint;opts.Permissionsis the samepermspointer, so clearingVulnerabilityAlertsbefore the retry takes effect. Covered by the three new tests inworkflows_permission_test.go. - Least privilege. Only the
sec-checktier setsVulnerabilityAlerts(app.go:345-351);TestScopedToken_OnlySecCheckTierRequestsVulnerabilityAlertsasserts every other tier omits it.
Intent: #17426 is triage/accepted and asks for exactly this permission plus the docs row, so the App-level widening is the maintainer's stated ask, not a new decision; the PR narrows it further to one role. Body matches the diff. Operator-facing: existing installations must re-accept the permission once, which the docs row and changelog both say.
β hive: agent=reviewer backend=copilot model=claude-fable-5.1 copilot=1.0.88
There was a problem hiding this comment.
Read the diff against the tree at 0a08d9a, then the callers. This looks correct to me.
What I verified
src/crux/agentmode/agentmode.go:128-130β the newsec-checktier is only yielded forModeIssuesAndPRs+ rolesec-check, andModeForTokenTiermaps it back toModeIssuesAndPRs(agentmode.go:166), so the mode ladder is unchanged. Every shippedsec-checkagent runs atmode: ISSUES_AND_PRS(src/pkg/config/packs/level-{4,5,6}.yaml), so the tier actually reaches the mint viacrux/agent/manager_tokens.go:969βScopedTokenForRepos.src/pkg/github/app.go:338-351β thesec-checkpermission set is thecontributorset plusVulnerabilityAlerts: read, nothing wider.installationPermissionMap(app.go:839) learns the key so the permission advisory can report it.src/pkg/github/app.go:443-451β the fallback mint mutatesperms, whichopts.Permissionspoints at (app.go:433), so the retry really does drop the grant. The guardisPermissionNotGrantedMintError(app.go:480-489) requires a 422 whose message contains "permission"; the repo-scope 422 ("not accessible to the parent installation") does not match, so that path still returns on the first mint and keeps the hub's one-mint-per-repo accounting intact.TestScopedToken_SecCheckRepoScopeErrorDoesNotRetrypins this.src/pkg/mint/agent.go:50β hive-side scopes forsec-checkare identical tocontributor, so the only widening is the GitHub-side read on Dependabot alerts, exactly what #17426 asks for.
For the operator merging this: the App itself has to declare Dependabot alerts: Read-only and every installation must re-accept once β the PR documents this in src/docs/github-app-setup.md:96 and the fallback keeps minting until then, so the change is safe to land before the App setting is flipped.
β hive: agent=reviewer backend=copilot model=claude-fable-5.1 copilot=1.0.88
|
/lgtm |
|
LGTM label has been added. DetailsGit tree hash: 2d2def50c7ec7b940ad1caae40a02141d379ba71 |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: clubanderson The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Thank you for your contribution! Your PR has been merged. We'd love to hear how your experience was: share feedback |
β¦the ask as a pill (#17457) * π activity monitor: light a needs-decision PR's real stage and carry the ask as a pill A PR parked at stage/085-needs-decision whose head is short of merge-ready (checks running, red or unreported; review verdict pending or asking for a person) forced a 'Needs human Β· decision' chevron into the PR in progress column while the CI and Review chevrons spun dim (#17442 under #17426; #17416 read 'Needs human Β· review' the same way). The lit chevron is now the PR's real stage: the CI track (with motion while checks run) or the Review track, wearing an amber ham-human-gated marker; PR in progress is passed and Merge queue stays empty. The ask rides as the needs-decision pill (human icon, question in the hover, decision popover on click) β the server re-adds the modifier once the head's tracks are known, and the page derives it for older payloads β and the lit track's tooltip ends 'Β· needs a person: <question>'. One lit stage per row. Only a merge-ready, queued or merging 085 keeps the gate. Fixes #17453 Signed-off-by: Andrew Anderson <andy@clubanderson.com> * fix(dashboard): sync shared anchor asset; needs-decision fallback yields to human pills Signed-off-by: Andrew Anderson <andy@clubanderson.com> * fix(dashboard): pad the needs-decision popover pill to the phone tap target Signed-off-by: Andrew Anderson <andy@clubanderson.com> * fix(dashboard): keep the needs-decision pill on one line in nested-PR heads Signed-off-by: Andrew Anderson <andy@clubanderson.com> * fix(dashboard): ask pill wears the person icon and folds to it in the stacked head Signed-off-by: Andrew Anderson <andy@clubanderson.com> * test(dashboard): track the ask pill's trigger call in the popover source guard Signed-off-by: Andrew Anderson <andy@clubanderson.com> --------- Signed-off-by: Andrew Anderson <andy@clubanderson.com>
Closes #17426
The sec-check policy calls
gh api repos/<org>/<repo>/dependabot/alerts, but the contributor-tier App token never requestedvulnerability_alertsand the permission was absent from the setup docs, so operators had no path to fix the 403.The mint requests it as optional: if the installation has not accepted it, the mint retries without it (same pattern as Workflows), so hives without sec-check are unaffected. Existing installations must re-accept the permission. No App-manifest builder or permission-advisory checklist exists in code, so nothing was added there.
β hive: agent=scanner backend=copilot model=claude-fable-5.1 copilot=1.0.88 requested_by=@williamdalessandro