feat: add the MCP approval management API - #5072
Open
daviddanialy wants to merge 6 commits into
Open
Conversation
Contributor
🦋 Changeset detectedLatest commit: be5fde6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
cubic analysis
All reported issues were addressed across 53 files
Linked issue analysis
Linked issue: AIS-470: feat: MCP approval management API
| Status | Acceptance criteria | Notes |
|---|---|---|
| ✅ | Expose a Goa service mcpApproval with endpoints mcpApproval.listRequests, mcpApproval.getRequest, and mcpApproval.recordDecision and generate OpenAPI/SDK/React Query hooks. | Design/service added and goa-generated HTTP server + client + CLI + SDK/react-query artifacts were added. Paths and OpenAPI entries for /rpc/mcpApproval.* are present. |
| ✅ | All SQL queries and handlers are tenant-scoped (every query bounded by project_id) and handlers resolve the request under the project to prevent IDOR; cross-project access 404s. | Queries file and generated repo code bind queries to project_id, and handler resolves request under the project. Tests cover cross-project access returning 404 and no writes. |
| ✅ | recordDecision snapshots the evidence (evidence_snapshot) and copies evidence_version from the request; a denial grants no principals regardless of caller payload. | DB model and sqlc-generated create approval decision persist evidence_snapshot and evidence_version; implementation and tests assert snapshot behaviour and that denials do not grant principals. |
| ✅ | Introduce RBAC scopes mcp_approval:read and mcp_approval:decide (and paired blocked_ scopes) and wire them into the grant surface/admin scopes, with tests exercising scope behaviour. | New scopes and grant-surface entries were added and the PR notes tests exercising read/decide relationships in both directions. |
| ✅ | Provide automated tests and generated SDK/tooling; CI/lint/type-check steps run successfully per PR description. | Multiple test files added (one per handler + harness), sqlc-generated repo code present, and SDK + OpenAPI generation artifacts added. PR description states lint/test/gen:sdk and pnpm type-check passed. |
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 49 files (changes from recent commits).
Tip: instead of fixing issues one by one fix them all with cubic
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 26 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adds the service test harness and one file per handler, covering the happy paths, the RBAC boundary between reviewing the queue and deciding on a server, and the cross-project cases: a request id read or decided from another project must not resolve, since the id appears in dashboard URLs and is not a secret. Seeding goes through three new SQLc queries rather than raw SQL, per the notestingrawsql analyzer. All three are intake and evidence-gather writes the feature needs regardless, and all are bounded by project id. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Completes what the first cut of the management API left half-wired, from adversarial review and cubic findings: - The four mcp_approval scopes now exist everywhere a scope must be registered, not just in the authz engine: the role-grant slug enum, the listScopes catalogue, the full-access grant set, the access request form, the selector resource-kind enum, and the dashboard role editor's resource types. Before this, the generated validator rejected the scopes before any handler ran, so no role could carry them. - ResourceKindForScope maps mcp_approval to its own resource kind instead of falling through to the wildcard, which had made project-narrowed grants unrepresentable: selector validation demanded resource_kind="*", so the scopes could only ever be granted org-wide. - The service is gated by the mcp_approval product feature, per the ticket. The gate is independent of RBAC: a grant says who may use the surface, the feature says whether the organization has it at all. - recordDecision writes an audit entry in the same transaction as the decision, with approve and deny as distinct filterable actions, on a new mcp_approval_request audit subject with its own URN type and outbox event. - Each decision in the API now carries the evidence frozen at its own decision time plus its version, so after a re-gather the surface can still show what an older decision actually rested on. - Evidence numerics decode with UseNumber so a large integer re-encodes exactly as stored. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- Concurrent decisions on one request now serialise: the decision transaction resolves the request through a locking read, so its status always matches the newest decision rather than whichever transaction happened to commit last. - A re-request reopens a denied review — the denial stays in the decision history and the request returns to the queue — while an approved or still-pending request keeps its status. The upsert's conflict branch previously only bumped updated_at, contradicting the documented re-request behaviour. - A rationale is now required at the payload boundary and blank ones are rejected: it is the artifact cited when explaining the decision to the requester, and a decision without one cannot be explained. - getRequest returns the request's research-agent runs, newest first, completing the ticket's read surface. Reports are project-bounded like every other child read. - Test honesty from review: the page-size test now seeds past the cap so the clamp is what bounds the result; the cross-project child test exercises the repo predicates directly instead of asserting on a fresh request the parent 404 already covered; the grant-surface success case covers the four new scopes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- The role editor actually shows the new permissions now: the dashboard group list is hard-coded and anything unlisted is silently dropped, so mcp_approval gets a group, joins the project-selectable resource types, and is registered in the client-side scope mirrors (resourceKindForScope, exclusion sets, sub-scope expansion) that previously fell through to the wildcard. - A decision can cite the research report that informed it. The report is resolved against the request being decided and the caller's project before anything is written, so a decision can never attribute research about one server to another; the citation rides on the decision in the API. - Research reports expose prompt_version, and failed runs can record their failure reason. - The URN type validates its current ID on every call instead of caching the constructor's verdict, and a zero UUID is rejected at parse rather than deferred to a later marshal. - RBAC runs before the feature lookup, so an unauthorized caller costs no feature-store work and a lookup failure never masks a denial. - rawEvidence rejects trailing content after the decoded value, restoring the strict no-partial-document contract UseNumber's decoder had relaxed. - The cross-project child test now seeds a decision in the other project, so its empty result proves the predicate filters rather than the data being absent. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- rawEvidence verifies end-of-input with a Token read rather than
More(): More reports false for a trailing `]` or `}`, so `{"a":1}]`
slipped past the guard — confirmed empirically before fixing.
- A malformed research_report_id is rejected before any database work,
so it costs no transaction and never locks the request row.
- The mcp_approval mappings in useRBAC get unit coverage matching the
other scope families, and the orphaned ptrString test helper is gone.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
daviddanialy
force-pushed
the
daviddanialy/ais-470-feat-mcp-approval-management-api
branch
from
August 7, 2026 21:15
f0a7ad2 to
be5fde6
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds the management API behind the MCP approval page (AIS-470). Stacked on the evidence-package PR; review that one first.
What it does
Three endpoints, all project-scoped:
mcpApproval.listRequests— the review queue, filterable by status, with a requester count per row so demand is visible without a second call.mcpApproval.getRequest— one request with its evidence, everyone who asked, and every decision made on it. A repeat request starts from the last rationale rather than from zero.mcpApproval.recordDecision— approve or deny, with a rationale and the principals the approval covers.Notes for review
Evidence is frozen onto the decision. The request's
current_evidenceis a cache that gets overwritten on re-gather; the copy a reviewer actually saw is snapshotted onto the decision, andevidence_versionis copied from the request rather than defaulted, so a v2 payload cannot later be read as v1. Covered by a test that re-gathers and asserts the decision is unchanged.A denial grants nobody anything, whatever the caller sent in
granted_principal_urns.Nothing here adjudicates. Every field is a declaration by the server or its registry, and the API descriptions say so — the surface has to present them as claims, not findings.
Tenancy
A request id appears in dashboard URLs and is not a secret, so id-alone lookups would be an IDOR. Every one of the nine queries in this service is bounded by
project_id, no tenant id is ever read from the payload, andrecordDecisionresolves the request under the project before writing rather than relying on the insert's predicate alone.organization_idon a decision is derived from the resolved request, not from the auth context, because the composite FK pins project but not organisation.Tests cover the cross-project path for all three handlers: another project's request must not list, must 404 on read, and must 404 on decide with nothing written.
RBAC
Two new scopes.
mcp_approval:readto review the queue,mcp_approval:decideto commit the organisation to a server;decideexpands to satisfyread, matching howskill:writecarriesskill:read. Tested in both directions.Not in this PR
Request intake (
createRequest, and promotion from a shadow-MCP block) lands with AIS-469. Pagination is capped at 200 rather than paged —next_cursoris always null.Testing
23 tests, one file per handler plus a shared harness.
mise run lint:server,mise run test:server ./internal/mcpapproval/,mise run gen:sdk, andpnpm -F dashboard type-checkall pass.AIS-470
Summary by cubic
Adds the
mcpApprovaldashboard API to manage MCP server approvals: list requests, view evidence, research reports, and history, and record approve/deny decisions with a required rationale (and optional research report citation). Delivers AIS-470 and generates the TypeScript SDK, React Query hooks, and CLI bindings for the new endpoints.New Features
mcpApproval.listRequests(filter by status; includes requester count),mcpApproval.getRequest(evidence, requesters, prior decisions, and research-agent runs),mcpApproval.recordDecision(approve/deny with rationale, covered principals, and an optional citedresearch_report_idvalidated to the same request/project).evidence_version; JSON is parsed strictly (rejects trailing content) and numerics preserve precision; concurrent decisions serialize so status reflects the latest; re-requests reopen denied reviews.project_idand cross-project reads/decides 404; gated by themcp_approvalproduct feature; decisions emit audit log events on themcp_approval_requestsubject (new URN and outbox event); scopes (mcp_approval:read,mcp_approval:decideand theirblocked_pairs) are registered across the role editor (project-selectable “MCP Approvals”), access request form, and selector (resource_kind: mcp_approval); OpenAPI/CLI and dashboardsdk/React Query hooks added for/rpc/mcpApproval.*.Migration
mcp_approval:readto reviewers; grantmcp_approval:decideonly to approvers (decideimpliesread; both haveblocked_pairs).mcp_approvalproduct feature.Written for commit be5fde6. Summary will update on new commits.