feat(mcp): classify every v1 RPC and lint the classification complete (1b-1) - #21180

Every v1 RPC now carries mcp_method_class, and CI fails on one that does not. That last clause is the point of the PR: five rounds of this series have found MCP-reachable methods one batch at a time, and a new RPC could always arrive unclassified. It cannot now.

Only FORBIDDEN is enforced. READ, WRITE and EXCLUDED are recorded classifications with no request-time reader — the gate that selects between them is 1b-2. The enum comment says so, the interceptor comment says so, and the interceptor's dispatch test pins that all three still dispatch. The exception is FORBIDDEN, which the existing interceptor enforces the moment it is annotated; see the behavior change below.

The classification

backend/api/v1/testdata/mcp_method_classification.md renders it row by row. It is a rendering, not the source of truth — the annotations are — and a stale file fails CI. Regenerate with:

MCP_INVENTORY=write go test ./backend/api/v1/ -run TestMCPClassificationInventory

Seeded from the closed #21141's inventory, re-verified against current main handler by handler. Its ai.op.* identifiers and capability universe are not revived.

Admin-class exclusion is a fourth class, not FORBIDDEN

EXCLUDED is new. It means no phase-1 mode serves the method, and it is deliberately not FORBIDDEN, because the two differ in whether they can ever be undone. An admin-capable ceiling — if one is ever built — could legitimately serve an EXCLUDED method; that is a product decision nobody has made. It could never serve a FORBIDDEN one, because FORBIDDEN names a mechanism that breaks the MCP boundary itself. Folding them together would put the ordinary admin API into the same durable-never set as credential minting, and any future widening would have to re-litigate both at once.

mcp_exclusion_reason is required on every EXCLUDED row, so an exclusion nobody wrote down cannot happen:

Counterargument, pre-written: a fourth value is one more thing the gate has to switch on, and today EXCLUDED and FORBIDDEN behave identically — nothing serves either. YAGNI says use FORBIDDEN and split later. Against that: they do not behave identically now — FORBIDDEN denies at the interceptor and EXCLUDED dispatches, which the dispatch test pins — and every FORBIDDEN row carries a reason describing a credential or boundary mechanism. Ninety-three admin rows have no such mechanism, so folding them in would mean either inventing a false reason or leaving them reasonless, which the lint refuses.

The conservative exclusions

Method granularity means excluding these costs nothing elsewhere, so where a method is dangerous behind an innocuous permission it is out.

• RolloutService/GetTaskRunSession — OPENS_AN_ADMIN_CONNECTION. Shares bb.taskRuns.list with four store-only reads, but alone opens an admin-credentialed connection to the customer database and returns other sessions' in-flight, unmasked SQL.
• SQLService/AdminExecute — same reason. Arbitrary SQL over an admin connection, no access-check callback, no masking pass.
• SavedQueryService/ListSavedQueries — READS_OTHER_USERS_SQL. The successor to ListWorksheets. Its own proto comment states the property: "Bindings are ignored: the permission alone reads every matched saved query's content, private ones included," and it accepts projects/-. SearchSavedQueries is caller-scoped and stays READ.
• QueryHistoryService/ListQueryHistories and the deprecated SQLService alias — same reason: every user's raw statements, workspace-wide under the wildcard.
• SQLService/Export and DatabaseService/DiffSchema are WRITE, not READ. Both share bb.databases.get with plain reads, so a permission-derived classification would have served them to a read-only session. WRITE is a serving mode, not a verb: Export takes a copy of data out of the product, and DiffSchema generates migration DDL from a schema the caller supplied.
• AIService/Chat and ProjectService/TestWebhook — SENDS_DATA_TO_A_THIRD_PARTY.

RETURNS_A_STORED_SECRET — eight rows excluded for a defect, not for what they are

These are ordinary reads that belong in a serving class on their merits. They are out because their response carries a stored secret the product already redacts elsewhere. The reason is meant to go away: fixing each leak moves its row to READ as a reviewed widening, rather than leaving a quiet exposure the moment the ceiling starts serving. TestExcludedOnlyForALeak pins the population.

• ProjectService/GetProject, ListProjects, BatchGetProjects, SearchProjects — convertToProject copies the incoming-webhook URL out verbatim. That URL is a bearer credential, and this repo already treats it as one: redactWebhook masks it in audit rows beside the OIDC client secret and the LDAP bind password. ListProjects pages the whole workspace.
• InstanceService/GetInstance, ListInstances, InstanceRoleService/ListInstanceRoles — InstanceRole.Attribute is the raw SHOW GRANTS text the MySQL driver stores. On MariaDB that carries IDENTIFIED BY PASSWORD '<hash>', verified against mariadb:10.6 and 11.4 containers. The data-source half of the same converter gets this right and says so: "We don't return the password and SSLs on reads."
• UserService/GetCurrentUser — returns temp_otp_secret and temp_recovery_codes when the subject is the caller, so a session reading its own profile during an MFA-setup window captures the TOTP seed. Narrowest of the three: the window is opened by the human in the console, and UpdateUser, which starts it, is FORBIDDEN.

Counterargument: classifying a method by a bug is wrong — the class should describe what the method is for, and the honest move is to fix the leak and keep them READ. Against that: the classification is inert today, so nothing is lost by excluding now, whereas a READ row whose redaction never lands is an exposure the day the gate ships. Fail-closed is this series' stated posture, and the cost is small — the built-in MCP tools do not call any of these; they are reachable only through call_api.

Behavior change: three methods newly denied on merge

ApproveIssue, RejectIssue and RetryIssueApproval, under a new DRIVES_THE_APPROVAL_DECISION reason. Anything annotated FORBIDDEN is enforced by the existing interceptor the moment this merges, so these are the only behavior change in the PR.

ApproveIssue and RejectIssue are two actions of one handler (issue_review.go reviewIssue) and it records the review decision itself: applyReviewAction requires an approver role via canReview, enforces the self-approval guard, and appends an APPROVED or REJECTED approver. An agent composes a change; it does not move its own change through the gate.

RequestIssue is deliberately NOT in this set — it is WRITE. Spec §1b-1 named four approval methods; that line grouped by RPC family and the mechanism does not agree. RequestIssue requires the issue to be already rejected, requires the actor to be the creator, never calls canReview, and records no decision — it strips the REJECTED approvers and returns the issue to PENDING for a fresh human decision. It approves nothing, so refusing it protects nothing the other two do not, and it costs the loop propose_database_change exists for: while a rejection stands both Approve and Reject hard-fail, so this is the only exit from that state. Raised by the Codex review.

RetryIssueApproval is the judgment call. It casts no vote — it re-runs approval-template finding for an issue stuck in CHECKING, and only the issue creator may call it. Counterargument: refusing it costs an agent the documented self-service recovery for its own stuck issue, and the containment is partial anyway — UpdatePlan with a specs mask and UpdateIssue on a label change both reset ApprovalFindingDone and force the same re-derivation, and both stay WRITE. Why it is in regardless: on an auto-approved result the same call activates the grant and enqueues the rollout, so it moves an issue through the gate with no human acting. The WRITE siblings are the deliberate line, not an oversight — editing a proposal is the agent's job and re-review after an edit is the system working.

The reason text claims only what the classification delivers. It does not claim an agent only executes approved work: CreatePlan, CreateRollout and BatchRunTasks are all WRITE, and both approval checks on the execution path are guarded on an issue existing, so a plan created without one reaches execution with no approval at all. Whether an issueless rollout belongs in WRITE is 1b-2's decision to take deliberately rather than inherit.

The lint

Four clauses over the compiled descriptor set, each a function over rows so a deliberately broken copy runs the same code. Nine RED tests; all green:

--- PASS: TestLintClausesFireWhenBroken/an_unclassified_method_fails
--- PASS: TestLintClausesFireWhenBroken/a_reasonless_FORBIDDEN_fails
--- PASS: TestLintClausesFireWhenBroken/a_FORBIDDEN_reason_with_no_wording_fails
--- PASS: TestLintClausesFireWhenBroken/a_reasonless_EXCLUDED_fails
--- PASS: TestLintClausesFireWhenBroken/a_served_method_carrying_a_denial_reason_fails
--- PASS: TestLintClausesFireWhenBroken/a_class_both_served_and_denied_fails_once,_not_once_per_method
--- PASS: TestLintClausesFireWhenBroken/a_class_nobody_decided_fails,_and_takes_its_rows_with_it
--- PASS: TestLintClausesFireWhenBroken/a_ceiling_mode_with_no_serving_row_fails
--- PASS: TestLintClausesFireWhenBroken/a_row_carrying_an_undecided_class_fails

On "no method is both FORBIDDEN and reachable through a serving mode": asserting that row by row proves nothing, because a row carries one class and the contradiction is impossible by construction. What can drift is the vocabulary, so the clause reads MCPMethodClass and MCPCapability out of the compiled descriptors and asserts every class is served by some mode or denied — never both, never neither — that every ceiling mode says what it serves, and that no row carries a class the tables skipped. Add a fifth class or a fourth mode and the build fails until somebody decides who serves it.

Two more pins: the three deprecated SQLService query-history aliases must match their canonical QueryHistoryService method, because the MCP surface resolves them by operation ID and a looser alias is a straight bypass; and TestExcludedOnlyForALeak above.

Debt recorded on the annotation, for 1b-2 and 1b-3

Two rows are classified where they belong and cannot be made safe by a class annotation alone, so the debt sits beside the annotation the gate PR will read:

• SQLService/Query is READ conditionally. On every engine in EngineSupportQueryNewACL the handler classifies each statement and authorizes DML with bb.sql.dml and DDL with bb.sql.ddl — what it does is decided by its argument. 1b-3's statement clamp is what makes READ safe, and the invariant that READ_ONLY is never admitted at /mcp before the clamp is live covers the interval.

• IssueService/CreateIssue is WRITE for the DATABASE_CHANGE issue it exists for. ROLE_GRANT and ACCESS_GRANT issues complete on creation whenever the workspace rule produces no approval template, each reaching past a method that is EXCLUDED for that outcome — ROLE_GRANT writes the project IAM binding (SetIamPolicy), ACCESS_GRANT sets the grant ACTIVE (ActivateAccessGrant). A class annotation cannot split by request field, so the gate PR owes a type carve-out. UpdateIssue carries the same debt through allow_missing.

• RolloutService/CreateRollout and BatchRunTasks stay WRITE, and two ways a change reaches execution without a human approval ride them. They get opposite answers, because only one is fixable at the MCP layer, and both are recorded on the CreateRollout annotation:

  • The gate PR owes a rule. Every approval check on that path is guarded on a linked issue existing — in the handler and in the locked re-check — so an issueless plan executes whatever require_issue_approval says. The rule to implement: on the MCP path, refuse CreateRollout and BatchRunTasks when the project sets require_issue_approval and the plan has no linked approved issue. Not because the behavior is a defect — the shipped GitOps Service Agent role holds no issue permission at all, so issueless deployment is a supported product contract and READ_WRITE preserves supported contracts. Because without it the four FORBIDDEN approval methods are decorative for the change lane: an agent that cannot clear the gate can skip it instead. BOT-71 tracks the product-wide question; if the product closes it, this rule becomes redundant rather than wrong.
  • The gate PR owes nothing on group drift. An approval binds the plan's specs and approval_input_version, not the databases a group target resolves to, and CreateRollout re-resolves the group live. No MCP rule closes it: approving an issue enqueues rollout creation on the auto path with no CreateRollout call, and schema sync enlarges the set with no API call. Only binding approval to a digest of the resolved set works, and live expansion was introduced deliberately (refactor: remove deployment snapshot and simplify plan updates #18589), so that is a product decision. BOT-72.

  CreatePlan stays WRITE with no rule of its own — a plan that is never rolled out changes nothing.

Rows that changed on re-verification of #21141

• WorksheetService (9 methods) is gone; SavedQueryService (11) replaced it. ListSavedQueries inherits ListWorksheets' exclusion; SearchSavedQueries and SearchSavedQueryFolders are caller-scoped and READ; the GetSavedQueryPolicy/SetSavedQueryPolicy pair is EXCLUDED as access control, decided together.
• InstanceService/ListInstanceDatabase moves out of the read-only set. Its inline-instance branch dials a caller-supplied connection config, and even the stored branch opens an admin connection under the same bb.instances.get that GetInstance uses for a pure store read.
• IdentityProviderService/ListIdentityProviders moves out. With an empty parent it lists across workspaces, and each OIDC entry triggers a server-side fetch of the issuer's well-known document.
• ProjectService/CreateProject moves to EXCLUDED. Every other project-lifecycle RPC is EXCLUDED under a reason whose text names "project lifecycle", and create persists the same governance switches update is excluded for — allow_self_approval, require_issue_approval — so a read-write session could have built itself a project with the review gate off.
• SubscriptionService/GetSubscription and ListPurchasePlans move to EXCLUDED, matching the rest of billing. Licence tier stays readable through GetActuatorInfo.
• The 16 methods feat(mcp): AI access capability vocabulary, resolution and CI lint (1b-1) #21141 shipped as ai.op.* operations resolve to plain classes; no identifier is minted.

Also here

backend/api/mcp/tool_change.go told the agent its next action was APPROVE_ISSUE — a call this PR refuses. It says AWAIT_HUMAN_APPROVAL, and the issue link the tool already returns is what a human follows.

Out of scope, found while classifying

Independent of the ceiling, each reachable by a human through the console today. Worth their own issues:

1. Project reads return the webhook URL unredacted while audit redacts it; instance reads return MariaDB account password hashes in InstanceRole.Attribute; GetCurrentUser returns the temp MFA secret. These are the eight RETURNS_A_STORED_SECRET rows.
2. CreateIssue with type=ROLE_GRANT writes a project IAM binding with no approver when no template matches, behind plain bb.issues.create, for a grantee the request names.
3. AccessGrantService/ActivateAccessGrant sets a grant ACTIVE without reading the linked issue's approval state.
4. OrgPolicyService/ListPolicies returns the MASKING_EXEMPTION payload gated on bb.policies.list alone, bypassing the per-type permission GetPolicy enforces.
5. AIService/Chat declares auth_method = CUSTOM but performs no authorization, so every authenticated workspace user can spend the stored provider key.
6. ListInstanceDatabase's inline branch skips checkInstanceDataSources/validateExternalSecretForSaaS, which CreateInstance runs.
7. UpdateDatabaseCatalog ignores update_mask, so a partial request wipes every classification not restated.
8. SettingService/GetSetting substitutes per-name permissions that WorkspaceMemberRole holds, making the sign-in policy and the MCP switch readable by every member.
9. ListSavedQueries, GetSavedQuery and TestWebhook carry audit = false while reading cross-user SQL or posting into customer chat.
10. Stale proto comments: ExportRequest.admin, GetIssueRequest.force and ListInstanceRolesRequest.refresh are dead fields; ListPlansRequest.parent's projects/- wildcard returns zero rows; GetDatabaseMetadataRequest.filter promises a masking level that does not exist; CheckRelease's comment omits its side effects.
11. require_issue_approval is enforced only when a plan has a linked issue, at all four check sites — so an issueless rollout skips it, and creating one first makes approval finding refuse afterwards. The question is whether the setting should mean "every rollout is approved" or keep meaning "if there is an issue, it must be approved" (BOT-71).
12. An approval is not bound to the resolved target set of a database-group plan, so databases joining the group after approval execute under it — and under plan checks that never ran against them (BOT-72).

Gates

buf format / lint / generate; gofmt; golangci-lint ./backend/api/... clean; go test ./backend/api/v1/ ./backend/api/mcp/ ./backend/api/auth/... and ./backend/tests -run '^TestMCP'; server build; frontend type-check.