feat(043): MCP write tools — users, licence assignments and tier pricing

[studert]

Summary

Gives an admin connected over OAuth the ability to act, not just read: nine write tools alongside the existing 14 read-only ones. Every mutation lands a change_history row attributed to the real human behind the token.

Spec, plan and running notes: specs/043-mcp-write-tools/.

Tool surface (9)

Tool	Guard	Notes
create_user	org-domain allow-list	role forced viewer; mints the invite but never returns the URL
update_user	expectedEmail	no email/role/status — see below
deactivate_user	expectedEmail + plan token	cascades licence revocation; no reactivation
assign_license	expectedUserEmail	refuses if the user already holds the tool, and if requiresApiKey
update_assignment	expectedUserEmail	the retier tool; re-snapshots cost
revoke_license	expectedUserEmail + plan token
create_access_tier	expectedToolName	two-sided cents guard
update_access_tier	tool + tier name	price structurally absent
set_tier_price	tool + tier name + current price + plan token	the org-wide write

Architecture

Business logic moves out of the Server Actions into actor-parameterised cores (src/lib/core/*), so there is one implementation of each mutation shared by the UI and MCP. It could not become a parameter on the actions themselves: every export of a "use server" file is a client-callable RPC endpoint whose arguments are attacker-controlled, so assignLicense(input, ctx) would let any client forge ctx.caps.

The audit writers move out of src/actions/history.ts for the same reason — they took changedBy as a parameter with no auth check, which made them an audit-forgery endpoint. Only the read side stays behind "use server".

Guardrails, each from a specific failure mode

• Conjunctive write gate: MCP_WRITE_ENABLED -> live admin role -> mcp:write scope -> token-bound user id -> non-automation actor. No fallback branch.
• New mcp:write scope, because the consent screen promised "No write access." Existing grants stay read-only until the human re-authorises.
• The shared MCP secret cannot write — it is unbound, so a write with it would be unattributable.
• Plan tokens (HMAC over the resolved plan, HKDF-derived key, 10-min TTL, caller-bound) on the three destructive tools. A boolean confirm is satisfied by the model in the same turn it invents the call.
• Echo guards: every tool taking a numeric id also takes a human-readable echo. Tier tools need tool name + tier name + current price, because tier names are unique only per tool and the real ones are generic (copilot-sync creates "Business"/"Enterprise").
• Secrets refused in the handler, before actor resolution and any DB read, so the refusal text provably cannot contain the submitted value.
• update_user cannot change email: every credential-recovery path mails the current address, so that field is an account-takeover primitive the echo cannot defend against.
• No reactivation: deactivateUser never clears passwordHash nor revokes OAuth tokens, so a status flip back to active silently restores an offboarded person's password and re-arms their live grants.
• Sync-owned fields refused: a Copilot tier reprice would be silently reverted at 06:00 UTC by the cron.
• No-op returns success, not isError — a retry after a lost response must not read as failure.

Schema (0030 + 0031, applied together)

• change_history.source distinguishes an agent write from the same human's UI edit.
• Partial unique index enforcing one ACTIVE assignment per (user, tool). Nothing enforced this before; MCP refusing rather than replacing widens the race into duplicate rows that double-bill a seat in every aggregation.
• 0031 supplies a DEFAULT purely for deploy safety. 0030 alone breaks every audited write in both deploy directions — proven with 23502. Provenance is enforced by the required TS field, not the default.

Verification

• 703 unit / 69 integration / 5 Playwright, plus a live MCP session over Streamable HTTP. typecheck and lint clean.
• Integration + e2e ran against a branch of production, which caught things unit tests structurally cannot (they all mock @/lib/db): the 23505 constraint-name match, SELECT … FOR UPDATE through neon-serverless (no precedent in this repo), and that the UI retier path does not trip the new unique index.
• Real data caught two bugs in the migration: the survivor rule was inverted (a higher id carried an earlier assigned_at), and revoked_at = now() would have left the double-count in every point-in-time report permanently.
• Migrations already applied to production: org active spend 1,349,200c -> 1,347,300c (-$19.00), which is one double-billed seat being corrected. Rollback record in specs/043-mcp-write-tools/production-rollback-record.json.

Rollout state

Production DB is migrated and backward-compatible, so this can merge at any time. MCP_WRITE_ENABLED=1 is being set on Vercel; it grants nothing on its own — each admin must reconnect their connector once to obtain the mcp:write scope.

🤖 Generated with Claude Code

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
ai-developer-hub	Ready	Preview	Aug 7, 2026 9:47am

[Copilot]

🟡 Human review recommended

It introduces new privileged write surfaces plus DB migrations and broad auth/audit refactors, which warrants final human review despite strong automated test coverage.

Pull request overview

Adds first-class MCP write capabilities (9 mutation tools) gated by a multi-step authorization flow, while refactoring existing UI Server Actions to share a single set of actor-parameterized write cores and ensuring every mutation is attributed in change_history with a provenance source.

Changes:

• Introduces MCP write tool handlers (src/lib/mcp/write.ts) with guardrails (kill switch, mcp:write OAuth scope, bound admin actor, secret refusal, plan-token preview/commit for destructive operations).
• Refactors user/license/tier mutations into shared cores (src/lib/core/*) and moves audit writers into a non–Server Action module (src/lib/history.ts) to prevent audit forgery.
• Adds DB enforcement + provenance: partial unique index for one active assignment per (user, tool), plus change_history.source (with deploy-safe default migration), and broad test coverage (unit/integration/e2e).

File summaries

File	Description
tests/unit/oauth/metadata.test.ts	Updates scope metadata expectations to include mcp:write.
tests/unit/mcp/write.test.ts	New unit tests for MCP write tool registration, gating, secret refusal, echoes, plan tokens.
tests/unit/mcp/tools.test.ts	Updates list_ai_tools call expectations to include includeInactive behavior.
tests/unit/actions/tier-cost-propagation.test.ts	Updates tier repricing tests for new transactional + auditing behavior and new history module.
tests/integration/mcp-write-cores.test.ts	New integration tests validating write cores against a real DB (constraints, FOR UPDATE, audit rows).
tests/e2e/043-write-paths.spec.ts	New Playwright coverage ensuring UI write flows still function after core refactor.
src/lib/sync/sources/invoice-matching.ts	Switches audit creation to @/lib/history with source: "sync".
src/lib/oauth/validate.ts	Adds mcp:write scope, request validation, grant resolution, and helper checks.
src/lib/oauth/metadata.ts	Advertises mcp:write in AS/Protected Resource metadata.
src/lib/oauth/authorize.ts	Preserves requested scope for later server-side grant resolution.
src/lib/mcp/write.ts	New MCP write tool surface and adapter: gating, secret refusal, plan-token protocol, echo requirements.
src/lib/mcp/tools.ts	Extends list_ai_tools tool with optional includeInactive (admin-only) parameter.
src/lib/mcp/plan-token.ts	New HMAC/HKDF-signed plan tokens enabling preview→commit for destructive write tools.
src/lib/mcp/data.ts	Adds includeInactive support for AI tools + tiers catalog reads.
src/lib/mcp/access.ts	Adds centralized write authorization (authorizeWrite) and related refusal messages.
src/lib/history.ts	New non–Server Action audit writers requiring source to prevent audit forgery.
src/lib/env.ts	Adds env validation entries for MCP write enablement + allowed email domains.
src/lib/db/schema.ts	Adds partial unique index on active assignments and adds change_history.source with default + index.
src/lib/db/migrations/meta/_journal.json	Records migrations 0030 and 0031 in the Drizzle journal.
src/lib/db/migrations/0031_change_history_source_default.sql	Adds deploy-safety default for change_history.source.
src/lib/db/migrations/0030_mcp_write_audit_source.sql	Adds change_history.source, remediates duplicate active assignments, creates partial unique index.
src/lib/core/users.ts	New user mutation cores (create/update/deactivate) with caps-based refusals and auditing.
src/lib/core/context.ts	New actor-parameterized write context, capabilities model, and core result shape.
src/lib/core/assignments.ts	New assignment mutation cores (assign/update/revoke) with caps and audit semantics.
src/app/oauth/authorize/page.tsx	Updates consent UI to accurately reflect granted write scope when applicable.
src/app/api/mcp/[transport]/route.ts	Registers write tools alongside existing read tools (refuse at call time if gated).
src/actions/users.ts	Replaces action bodies with calls into shared user cores; updates history calls to require source.
src/actions/tools.ts	Replaces action bodies with calls into shared tool/tier cores; routes tier price edits through setTierPriceCore.
src/actions/oauth.ts	Resolves granted scopes server-side from requested scope + session role (for write scope).
src/actions/invoices.ts	Updates history calls to use @/lib/history with source: "ingest".
src/actions/invoice-sync.ts	Updates sync history calls to use @/lib/history with source: "sync".
src/actions/invite.ts	Updates history calls to use @/lib/history with explicit source.
src/actions/history.ts	Removes write helpers; retains read-side history API and documents why.
src/actions/github.ts	Updates history calls to use @/lib/history with explicit source.
src/actions/github-sync.ts	Updates sync-side history calls to use @/lib/history with source: "sync".
src/actions/forecast-scenarios.ts	Updates history calls to use @/lib/history with source: "ui".
src/actions/copilot.ts	Updates history calls to use @/lib/history with source: "ui".
src/actions/budget.ts	Updates history calls to use @/lib/history with explicit source.
src/actions/budget-extensions.ts	Updates history calls to use @/lib/history with explicit source.
src/actions/assignments.ts	Refactors actions into thin wrappers over assignment cores; updates history usage to new module/signature.
scripts/preflight-duplicate-assignments.ts	New read-only preflight script to report duplicates before applying migration 0030.
.env.local.example	Documents MCP write env flags, allow-list domains, and shared-secret read-only behavior.

Review details

• Files reviewed: 45/47 changed files
• Comments generated: 2
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[studert]

Fixed in 0b44b01 — you were right, and the comment was describing a design that got reversed late.

change_history.source was originally specified as NOT NULL with no DB default, for exactly the reason that comment gives. That turned out to be undeployable in either direction: migrate-first and the live pre-043 code inserts without a source, so every audited write fails with 23502 (reproduced against a real database, not theorised); deploy-first and the new code writes a column that does not exist yet. Migration 0031 therefore adds DEFAULT 'ui', and this header no longer claims otherwise.

The enforcement argument survives, just relocated: source is a required field on HistoryOptions, so the compiler still forces all ~44 call sites going through these helpers to state it explicitly. The DB default only backstops a raw db.insert(changeHistory) that bypasses them — there are exactly two, both of which pass it explicitly.

[studert]

Fixed in 0b44b01. Same root cause as the history.ts comment: this header described the original design, which 0031 reversed for deploy safety. Item 1 now says 0030 deliberately leaves the column without a default and points at 0031_change_history_source_default.sql for why the default is added immediately afterwards.

Worth recording why editing an already-applied migration is safe here, since that is normally a red flag — this migration is already applied to production. Drizzle decides which migrations are pending from the journal's folderMillis timestamp, not from the recorded hash (pg-core/dialect.js:62: Number(lastDbMigration.created_at) < migration.folderMillis). The hash is written to drizzle.__drizzle_migrations but never compared. I verified against production before committing: its newest created_at is 1785849754855 (0031), and 0030's when is 1785835084489, so 0030 is skipped and cannot re-run.

If drizzle had compared hashes instead, this comment edit would have made 0030 look unapplied and the next db:migrate would have tried to re-add an existing column.

[Copilot]

Pull request overview

Copilot reviewed 52 out of 54 changed files in this pull request and generated 3 comments.

Suppressed comments (4)

src/lib/core/tools.ts:605

• This limit is documented as an MCP guard that should direct large reprices to the Hub UI, but it is unconditional and updateTier calls this same core with a UI context. Once a tier exceeds 250 seats, the UI is also refused with instructions to use the UI. Gate both this check and the repeated in-transaction check on the MCP capability/source, while handling the UI audit batch without an unbounded sequence of inserts.

  if (seats.length > MAX_REPRICE_ROWS) {
    return coreErr(
      `${seats.length} active assignments would be repriced (limit ` +
        `${MAX_REPRICE_ROWS}). Make a change this large in the Hub UI at ` +
        `/tools/${existing.tool.id}.`,
    );

src/lib/core/tools.ts:663

• SELECT ... FOR UPDATE only locks rows that already exist; it does not prevent a concurrent assignLicenseCore insert for this tier. Such an insert can either be caught by the later bulk update without appearing in locking (so it is unaudited), or land after the update with the old cost snapshot. Serialize assignment creation on the same tier row (or use equivalent predicate/transaction isolation) so the locked set really is the set rewritten and audited.

    // THIS is the row set the update below rewrites, so it is also the row set
    // the audit rows describe.
    const locking = await tx
      .select({
        id: licenseAssignments.id,
        beforeCents: licenseAssignments.costAtAssignmentCents,
      })
      .from(licenseAssignments)
      .where(
        and(
          eq(licenseAssignments.tierId, tierId),
          eq(licenseAssignments.status, "active"),
        ),
      )
      .for("update");

src/lib/core/users.ts:345

• The active-assignment list used for the preview, result, and audit is read before the transaction, but this bulk update can match a newly inserted assignment that is absent from that list. The new row is then revoked without an audit entry, and revokedCount/released cost can be wrong. Re-read and lock the affected assignments inside the transaction and derive both the update and audit/result from that locked set (while preventing assignment creation for the deactivated user).

    if (activeAssignments.length > 0) {
      await tx
        .update(licenseAssignments)
        .set({ status: "inactive", revokedAt: now, updatedAt: now })
        .where(
          and(
            eq(licenseAssignments.userId, input.id),
            eq(licenseAssignments.status, "active"),
          ),
        );

src/lib/mcp/write.ts:178

• gated authorizes the actor and calls isAgentUser (a DB read) before invoking the tool callback, so the secret checks inside assign_license and update_assignment are not actually the first operation. This contradicts the documented guardrail that submitted secrets are refused before actor resolution/DB access. Add a pre-authorization input guard to this wrapper (or wrap these two handlers outside gated) so secret-bearing requests return the controlled refusal immediately.

[studert]

Fixed in a94e37a. Real bug, introduced when I split the propagating price write out of updateTier — the original did both in one transaction and I did not preserve that.

Both metadata failure modes you can actually hit are pure validation: a duplicate tier name, and deactivating a tier that still has active assignments. Neither needs a write to detect. So updateTier now runs updateTierCore with commit: false first and returns before touching the price, which makes the reported sequence unreachable.

If the commit pass still fails afterwards — a concurrent rename between validation and commit, or a DB fault — the price is already committed, so that path now replays the price's revalidation instead of dropping it. That was the second half of your comment and it was the worse half: an error return that leaves /, /budget and /reports serving the old number is harder to notice than the error itself.

Being explicit about what this does not do: it is not true cross-core atomicity. setTierPriceCore and updateTierCore each open their own transaction, so genuine all-or-nothing needs a shared-transaction parameter threaded through WriteContext. I have not added that here — it changes every core signature and this PR is already carrying a merge with #125. What is fixed is the reachable failure, not the theoretical one, and the residual is now a concurrent-rename race rather than ordinary validation.

[studert]

Fixed in a94e37a. Correct, and this is the most serious of the three — thank you.

createInviteTokenForUser lived in src/actions/invite.ts with a comment on it literally reading "internal helper — no auth check, no user lookup", which is fine for an internal helper and disqualifying for a "use server" export. It took a bare userId and returned a live 72-hour /setup-password URL, so any client could POST an arbitrary id and receive a working password-set link for that account.

You are right that it is the same class of issue this PR fixes for the history writers, which makes it worse rather than better: I identified that exact pattern, moved four functions out for it, and then imported this one into the MCP layer without noticing it had the same shape.

Moved to src/lib/invite.ts — a plain module, and the natural home since the token helpers it already calls (generateToken, buildInviteUrl) live there. src/actions/invite.ts keeps only the authenticated wrappers (generateInviteToken, resetUserPassword, sendInviteEmail, sendBatchInviteEmails), each of which calls it after its own requireAdmin(), and now carries a comment saying why the raw minter is not there. All three importers repointed.

[studert]

Fixed in a94e37a. Partly a correction to the framing, and one genuine bug underneath that is worse than the race.

On the framing: the token is not verified against the original preview. previewOrCommit re-resolves the plan by running the core with commit: false at commit time, then verifies the token against that freshly-resolved plan. So a change between the human's first preview and the commit is already caught — there is an integration test for exactly that ("rejects a token whose plan no longer matches the resolved state"). set_tier_price narrows it further by re-reading the price under FOR UPDATE inside the commit transaction and hard-failing if it moved.

But the window you point at between the re-resolve and the write is real, and chasing it surfaced something concrete in deactivateUserCore. The cascade UPDATE is WHERE user_id = $1 AND status = 'active', so it revokes any seat assigned after the pre-transaction read — while the audit loop iterated that earlier snapshot. A seat appearing in that window was therefore revoked with no change_history row at all. Not a plan mismatch: a silent audit hole in the one operation whose whole justification is that it records what the person held, because nothing restores it.

The seats are now re-read inside the transaction under FOR UPDATE (locking only license_assignments, not the joined ai_tools/access_tiers, which would contend with unrelated tier edits), and the audit is driven from that locked set. Same pattern setTierPriceCore already used. New integration test covers it directly — two seats on different tools, since the partial unique index permits only one active seat per tool.

revokeLicenseCore got your CAS suggestion: its UPDATE is now guarded on status = 'active' and reports a no-op when zero rows match, rather than overwriting an existing revokedAt with a later timestamp and writing a second "active → inactive" row for a transition that already happened.