Keep per-tool allows when one rule demotes the group

Abhimanyu Singhrequested to merge
duo/630369-per-tool-allows-survive-group-ask into master

What does this MR do and why?

A single explicit ask or deny rule on one tool in read_only_gitlab currently removes every tool in that group from pre_approved_tools, including the ones the admin left at their default of Allow.

Measured on a namespace with no other rules, surface :web: one Ask on get_issue takes the allow list from 75 tools to 0.

The settings page still shows the other tools as Allow. ToolRulesResolver#build_tool_hash computes web_access per tool with no group logic, and registry.rb states the invariant this breaks, in a comment on default_permission_of: "Deriving it here keeps the displayed default equal to the enforced one."

Why this happens

resolve_group correctly demotes a mixed group out of pre_approved_agent_privileges. That is #606359 working as intended. The problem is that allowed_tools_for_group is only called inside when :allow, so when a group resolves :ask nothing reaches the per-tool allow list except tools carrying an explicit allow rule.

This predates !250707 (merged). Before it, resolve_group returned :ask via permissions.include?(ASK), which gives the same verdict here; !250707 (merged) left the Ask path unchanged and improved the Deny path. The when :allow gate dates to 57cbf31f (2026-05-22). #606359 and !250707 (merged) both reasoned about read_write_gitlab, whose default is ask, where dropping unconfigured tools is correct. read_only_gitlab is the only group in DEFAULT_PREAPPROVED_GROUPS and inverts the outcome.

Why it matters now

Latent for chat today: nothing intersects the claim's allow list with the row's pre-approvals, and chat_agent.py treats the two as an OR, so web chat still pre-approves through the row and one Ask gives one prompt.

Not latent for v1 flows. Only the chat workflow reads the allow list, so developer/v1 builds pre-approval from the row alone. With local governance on, one Ask already makes every GitLab read prompt in the Duo CLI, and this MR alone does not change that. ai-assist!7063 passes the allow list into ToolsRegistry for v1 flows, and the two together close it. Reported by @viktomas.

ai-assist!6749 makes the allow list a ceiling. After it lands, one Ask rule strips all 56 GitLab read tools DWS grants through privilege group 2, plus every entry in a flow or catalog agent's pre_approved_tools: config.

The change

Two edits in ee/app/services/ai/tool_rules/resolution_service.rb:

  1. Call allowed_tools_for_group on :ask as well as :allow, never on :deny.
  2. Point allowed_tools_for_group at Registry.governed_tool_names_for(group_name: group) instead of the raw PRIVILEGE_GROUP_MAPPING.

The second is required, not cosmetic. resolve_group already filters to catalogued names; allowed_tools_for_group did not. Doing only the first newly pre-approves evaluate_vuln_fp_status, ascp_list_scans and ascp_list_components, which have no catalog entry and so cannot be denied by any admin rule, and it fails the existing spec at resolution_service_spec.rb:183.

In a default-ask group the fallback is ask on the web and local surfaces, so allowed_tools_for_group returns only the explicitly-allowed tools, which is exactly what the unconditional concat already emitted. The first edit is a no-op for every group except read_only_gitlab, on those two surfaces.

On the background surface, fallback_permission coerces the ask default to allow for every group. So a single deny rule leaves the group resolved to :ask, and the first edit now pre-approves the group's other tools per tool, where before it added none. This matches the displayed default: the settings page shows those tools as Allow on background. The background surface itself is only reached behind the duo_workflow_background_tool_governance feature flag, and only for the developer flow.

Measured on the background surface:

namespace config (surface :background) before after
no rules 102 94
one Deny on update_issue 71 93

The 102 to 94 baseline drop is the 8 uncatalogued names (3 read, 5 write) that governed_tool_names_for now excludes. The 71 to 93 jump is the 24 other catalogued write tools in the group, now kept pre-approved per tool instead of dropping out with the whole group.

Behaviour change today

The baseline moving from 75 to 72 is real, not preparation. It drops ascp_list_components, ascp_list_scans and evaluate_vuln_fp_status from the allow list.

The same change also removes five write tools from read_write_gitlab, but only on the background surface: post_secret_fp_analysis_to_gitlab, set_form_permissions, ascp_create_scan, ascp_create_component and ascp_create_security_context. The background zero-rule baseline goes from 102 to 94. On web these never had a chance to be pre-approved: the group's fallback there is ask, and an uncatalogued name can never carry a rule, so it always resolves to ask regardless of the group's own resolution. Consumers checked in the workflow service repo: two versions of the ambient secrets_fp_detection flow and the experimental ambient business_context_security_guidelines flow reference four of them, and the chat-partial duo_permissions_assistant flow references the fifth. None of the four configs sets require_tool_approval or pre_approved_tools. The ambient flows never reach the background surface (outside GovernanceSurface::ALLOWLISTED_FLOWS), and the chat-partial flow runs on web, so no consumer observes this change today. Where all eight uncatalogued names land in the catalog is tracked in https://gitlab.com/gitlab-org/gitlab/-/issues/630349.

Consumers checked: ascp_list_components in business_context_security_guidelines/1.0.0.yml, evaluate_vuln_fp_status in resolve_sast_vulnerability/1.0.0.yml and 1.0.1.yml. Both are Sidekiq flows with require_tool_approval false, so pre-approval is never consulted and this is a no-op for them. After !6749 (closed) those three cannot be pre-approved at all, which is the decision in #630349.

Before and after

Measured on a zero-rule namespace, surface :web:

namespace config before after
no rules 75 72
one Ask on get_issue 0 70
one Deny on get_issue 0 70
one Allow on create_issue (default-ask group) 78 75
one Deny on create_issue 75 72
Allow create_issue + Deny create_epic 78 75
requested project denies get_issue, workflow runs elsewhere (tightening path from MR 6492) 0 reads all reads but get_issue

The default-ask rows move by the same 3 as the baseline and are otherwise unchanged, so #606359's before/after table is preserved.

Tests

No existing spec asserted the collapsing behaviour, so nothing had to be rewritten. Added:

  • a default-allow group with one ask rule keeps its other tools in pre_approved_tools, excludes the asked tool, and reports it in ask_tools
  • the same for a deny rule
  • a mapped tool with no catalog entry never appears in pre_approved_tools
  • the request-spec example from MR 6492 for a project-level deny on one read tool now asserts that the rest of the group stays pre-approved; on master it fails, so it is this MR's request-level regression guard

The existing "excludes the group from pre_approved_agent_privileges" example is untouched, so #606359 stays pinned.

References

How to set up and validate locally

bundle exec rspec ee/spec/services/ai/tool_rules/resolution_service_spec.rb

Then, in rails console, on a namespace with no rules:

g = Group.find_by_full_path('your-group')
svc = ->(){ Ai::ToolRules::ResolutionService.new(namespace: g, surface: :web).execute.payload }
svc.call[:pre_approved_tools].uniq.size
Ai::ToolRule.create!(namespace: g, tool_name: 'get_issue', web_access: :ask)
svc.call[:pre_approved_tools].uniq.size

Before this change the second count is 0. After it, the other catalogued read tools survive.

MR acceptance checklist

Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

Edited by Pratyaksh Golash

Merge request reports