Do not resolve tool rules for an ungoverned local surface
Why this is needed now
This is a prerequisite for re-landing ai-assist!6491 and closing #608968.
!6491 (merged) was reverted because it produced approval prompts on every local file read. Those prompts came from the defect below, not from the ceiling. Re-landing !6491 (merged) without this fix reproduces the incident, so this has to land first.
What is actually broken in production today
Being straight about urgency, because the headline symptom is currently gone:
| live now? | |
|---|---|
| Ask prompts on local reads | no — !6491 (merged) is reverted, so nothing clamps the row |
local_access rules written to the row while the flag is off |
only if an admin configured local rules with the flag off |
| A web-only Deny hiding a tool in IDE and CLI | yes |
The deny leak is the one live defect. In DWS, if policies.deny: sits outside the governance_active block, so denies apply whether or not the ceiling exists. It predates !6491 (merged) entirely: 57cbf31f7082 hardcoded surface: :web, and a7e9e83091c8 preserved that. Reverting !6491 (merged) does not address it. It is probably unreported because few namespaces have explicit web Deny rules yet.
The defect
GovernanceSurface.for returned nil for an ide or chat session while duo_workflow_local_tool_governance was off. nil already meant "no special surface, use the caller's default", so every caller resolved rules anyway, against web_access. The flag gated surface selection, not whether governance applied.
Closes #622602.
Adds GovernanceSurface::UNGOVERNED so the two meanings are separable. nil keeps its original meaning. ResolutionService refuses the sentinel, because ToolRule#access_for falls through to local_access for an unrecognised surface, which is the defect itself.
Why a sentinel rather than a check at the call site
Four callers read the nil, with three different fallbacks. Fixing one would have left the shape intact.
| caller | fallback | effect for ide with the flag off |
|---|---|---|
workflows.rb (/ws claim) |
|| :web |
claim built from web_access rules |
workflow_context_generation_service (token claim) |
|| :web |
claim built from web_access rules |
build_resolution_service |
|| degraded_surface → raw 'ide' |
local_access rules written to the workflow row |
clamp_surface, UpdateAgentPrivilegesService |
|| :web |
reachable only if their duplicated flag checks are removed |
Before and after
Surface resolution
The only behavioural change. Everything below follows from this one cell.
| environment | local flag | before | after |
|---|---|---|---|
ide / chat / chat_partial |
off | nil |
:ungoverned |
ide / chat / chat_partial |
on | :ide / :chat / :chat_partial |
unchanged |
web / ambient, no flow |
either | nil |
unchanged |
web / ambient + allowlisted flow, background flag on |
either | :background |
unchanged |
external |
either | nil |
unchanged |
absent / nil |
either | nil |
unchanged |
Call sites, for a local surface with the flag off
| call site | before | after |
|---|---|---|
/ws claim |
web_access rules resolved into the claim |
{allow: [], deny: []} |
| workflow-token claim | web_access rules resolved into the claim |
no resolution, empty payload |
| row privileges (server-set) | local_access rules written to the row |
DEFAULT_PRIVILEGES, no resolution |
| row clamp (client-supplied) | unreachable, flag-guarded | unreachable; the guard lives in the caller, not in clamp_surface |
What a user sees
| symptom | before | after |
|---|---|---|
approval prompt on read_file, grep, list_dir |
prompts, once the DWS row-clamp is deployed | silent |
| a web-only Deny hides a tool in IDE/CLI | tool absent, no prompt, live today | tool present |
local_access rules applied while the flag is off |
applied | not applied |
everything on web, ambient, external, background |
unchanged | |
| any local session with the flag on | unchanged |
"No tool rules configured" is not the same as "ungoverned". With gitlab_duo_governance_settings on and no rules, governance still resolves: registry defaults put the read_only_gitlab tools into allow, so the claim is non-empty. Only a genuinely ungoverned session sends all three lists empty. Conflating the two is what produced this incident.
This does not unblock #613544
Enabling duo_workflow_local_tool_governance on a namespace with no tool rules will produce approval prompts on local file reads, because read_only_files defaults to ask per !244013 (merged).
That is unchanged by this MR. With the flag on, GovernanceSurface.for has always returned a real surface, so the flag-on path never had the nil-collapse bug. This MR fixes governance running ahead of the flag; @rbarnwal1's follow-up, coercing an unset read_only_files to allow on the local surface, is what makes the flag safe to turn on. Both are needed before rollout.
| any local session once the flag is on | | unchanged |
The deny leak is the one that is live in production right now. It predates the ceiling work: 57cbf31f7082 hardcoded surface: :web, and a7e9e83091c8 preserved that when GovernanceSurface was introduced. Reverting ai-assist!6491 does not address it.
Why the ungoverned claim is empty
{allow: [], deny: []} is deliberate and must not carry the MCP pre-approved names. DWS reads empty allow and deny as governance inactive and leaves the client's own pre-approvals in place:
# ai-assist main, abstract_workflow.py:354
governance_active = (
bool(policies.allow) or bool(policies.deny) or bool(self._ask_tools)
)
if governance_active:
self._preapproved_tools = list(...)Verified against ai-assist main at 551365aca, and the ceiling revert 72b41d7c3 does not touch that line (it comes from d4e5c6bc6, !6594 (merged)). So an empty claim reads as inactive both with the ceiling present and with it reverted. I cannot verify every deployed DWS version from this repo; if a reviewer knows of an older deployed build that reads the claim differently, that would need checking before rollout.
A non-empty allow would activate the ceiling and replace those pre-approvals with the MCP list alone, so every non-MCP tool would start prompting. This is also exactly what the existing gitlab_duo_governance_settings-off branch already sends.
The contract callers must honour
GovernanceSurface.for now has three outcomes, not two:
| return | meaning | caller must |
|---|---|---|
| a surface symbol | govern against that surface | resolve normally |
nil |
no special surface | default to :web, as before |
UNGOVERNED |
this session must not be governed | skip resolution entirely |
All four callers check GovernanceSurface.ungoverned? before resolving. clamp_surface and resolution_service still end in || :web; they are safe because their callers guard first, not because those methods are sentinel-aware.
ResolutionService is the backstop, not the guard. Handed the sentinel it fails loudly in development and test via track_and_raise_for_dev_exception, and in production returns ServiceResponse.error(reason: :ungoverned_surface). Every caller already handles an unsuccessful result, so a future caller that forgets the check lands on its existing fail-closed path rather than raising out of a request. resolve_governance_with_retry short-circuits that reason, since retrying cannot change a surface.
Anyone adding a call site, including !251602 (merged), must add the guard. The backstop degrades gracefully; it does not make the guard optional.
Known inconsistency left in place
degraded_surface passes an unrecognised environment through as a raw string, and ToolRule#access_for treats anything outside WEB_SURFACES as local_access. So an external workflow's row privileges resolve against local rules while its claims resolve against web rules.
Same class of fall-through as the bug fixed here, but pre-existing and it needs a decision on what external should mean rather than a wiring change. Not fixed here; the comment near clamp_surface now names it instead of claiming consistency. Tracked in #624032, along with gitlab_duo_governance_settings being checked against three different actor types across these call sites.
Not in scope, and the agreed follow-up
read_only_files defaulting to ask is deliberate, set in !244013 (merged) on least-privilege grounds and documented in the matrix. It is unchanged here.
Separately, @rbarnwal1 and @nrosandich have agreed a follow-up on #622602: coerce an unset read_only_files permission to allow on the local surface only, inside ResolutionService#fallback_permission, mirroring what background_effective already does for background flows. Write and destroy stay at ask, and a namespace that wants strict local reads writes an explicit rule, which is the opt-in without a new setting.
That is what makes #613544 rollable: today the flag cannot be enabled without recreating this week's prompts. This MR stops governance running ahead of that flag; the follow-up makes the flag safe to turn on. Both are needed before the rollout.
How to set up and validate locally
- Pick a group with no
Ai::ToolRulerows and confirmduo_workflow_local_tool_governanceis disabled. - Add a rule for a GitLab read tool, for example
list_issues→ Deny, on the web surface only. - Start an IDE or CLI Duo session against a project in that group and ask the agent to list issues. Before this change the tool is missing entirely; after it, the tool works.
- Enable
duo_workflow_local_tool_governancefor the group and repeat. Thelocal_accessvalue now governs, as before.
Testing
733 examples, 0 failures, 15 pending
workflows_spec, governance_surface_spec, resolution_service_spec,
workflow_context_generation_service_spec, create_workflow_service_spec,
update_agent_privileges_service_spec, tool_rules_resolver_specRuboCop clean on all changed files.
Three existing specs asserted the old behaviour as correct, and two of them could not have caught this. "resolves web rules, ignoring the local-only allow" checked only that allow excluded one tool, which held whether or not policies were sent. All three now assert that nothing is resolved.
Every new behavioural spec was verified to fail with return UNGOVERNED reverted to return:
mutated: 635 examples, 8 failures
restored: 733 examples, 0 failuresAlso adds a surface resolution matrix over environment, definition and both governance flags, so adding an environment forces the author to state what an unconfigured user on that surface should experience. That is the spec-level guard requested in #622884.
Merge request checklist
- Tests added for the changed behaviour, and verified to fail without the fix
- Documentation added/updated, if needed
- Reviewed by the DAP governance owners
Related
- #622602, the issue
- #622884, the process gap that asked for the spec guard
- #613544,
duo_workflow_local_tool_governancerollout - !251602 (merged), blocked on this; it inherits the same fallback in
clamp_surface - !251850 (closed), closed; an earlier attempt that changed the
read_only_filesdefault - #624032, two latent inconsistencies found here, neither currently reachable