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

  1. Pick a group with no Ai::ToolRule rows and confirm duo_workflow_local_tool_governance is disabled.
  2. Add a rule for a GitLab read tool, for example list_issues → Deny, on the web surface only.
  3. 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.
  4. Enable duo_workflow_local_tool_governance for the group and repeat. The local_access value 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_spec

RuboCop 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 failures

Also 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
  • #622602, the issue
  • #622884, the process gap that asked for the spec guard
  • #613544, duo_workflow_local_tool_governance rollout
  • !251602 (merged), blocked on this; it inherits the same fallback in clamp_surface
  • !251850 (closed), closed; an earlier attempt that changed the read_only_files default
  • #624032, two latent inconsistencies found here, neither currently reachable
Edited by Luke Duncalfe

Merge request reports

Loading
Loading