Close MCP workflow, pagination and reliability gaps

apps/web/src/features/mcp/handler.ts

@@ -178,21 +178,38 @@ export async function handleMcpRequest(request: Request): Promise<Response> {
     });
   const secret = bearerSecret(request);
   if (!secret) return unauthorized();
-  if (!getOptionalServiceDatabase() || !origin)
-    return problem(503, "MCP_UNAVAILABLE", "MCP is not available here");
-  const repository = accessTokenRepository();
-  const token = await repository.authenticate(secret);
-  if (!token) return unauthorized();
-  if (token.windowCount > accessTokenRequestsPerMinute)
+  let repository: ReturnType<typeof accessTokenRepository>;
+  let token: Awaited<ReturnType<typeof repository.authenticate>>;
+  let session: Awaited<ReturnType<typeof accessTokenCommerceSession>>;
+  try {
+    if (!getOptionalServiceDatabase() || !origin)
+      return problem(503, "MCP_UNAVAILABLE", "MCP is not available here");
+    repository = accessTokenRepository();
+    token = await repository.authenticate(secret);
+    if (!token) return unauthorized();
+    if (token.windowCount > accessTokenRequestsPerMinute)
+      return problem(
+        429,
+        "MCP_RATE_LIMITED",
+        "Too many requests for this token",
+        {
+          "retry-after": String(token.windowResetSeconds),
+        },
+      );
+    session = await accessTokenCommerceSession(token);
+  } catch {
+    // Only authentication setup is retryable here: no tool has run. Keep
+    // provider/database details private, and do not mislabel an outage as an
+    // invalid token or encourage a retry after a tool may have changed data.
     return problem(
-      429,
-      "MCP_RATE_LIMITED",
-      "Too many requests for this token",
+      503,
+      "MCP_UNAVAILABLE",
+      "MCP is temporarily unavailable. Try again shortly.",
       {
-        "retry-after": String(token.windowResetSeconds),
+        "retry-after": "5",
       },
     );
-  const session = await accessTokenCommerceSession(token);
+  }
   if (!session?.accessToken) return unauthorized();
   const body = await readMessage(request);
   if (!body.ok) return body.response;

apps/web/src/features/mcp/mcp-owner-rule.integration.test.ts

@@ -144,7 +144,13 @@ const person = {
   email: `mcp.rule.${tag}@clockwork.test`,
   name: `MCP Rule ${tag}`,
 };
-const roleTarget = { id: randomUUID(), membershipId: randomUUID() };
+// Order acceptance falls back to the first internal user by UUID. Keep this
+// disposable staff fixture after the persistent seed operator so concurrent
+// suites cannot assign their exception cases to a user this suite deletes.
+const roleTarget = {
+  id: `ffff${randomUUID().slice(4)}`,
+  membershipId: randomUUID(),
+};
 let secret = "";
 let next = 1;
 const partners: string[] = [];
@@ -153,6 +159,11 @@ const requestedMndaId = randomUUID();
 let countersignerId = "";
 
 beforeAll(async () => {
+  const persistentOperators = await sql`
+    select id from commerce_users
+    where id = '20000000-0000-4000-8000-000000000001'
+      and is_internal_staff and id < ${roleTarget.id}::uuid`;
+  expect(persistentOperators).toHaveLength(1);
   await sql`insert into commerce_users (id, workos_user_id, email, name, is_internal_staff, mfa_enrolled)
     values (${person.id}, ${`mcp_rule_${tag}`}, ${person.email}, ${person.name}, true, true)`;
   await sql`insert into memberships (id, organization_id, user_id, role)
@@ -318,6 +329,10 @@ describe("the owner rule", () => {
           nfr_capacity: "5 TiB for 60 days",
         },
       }),
+      check_contract_status: () => ({
+        contractId: state.templateContractId,
+        requestNumber: state.signingRequestNumber,
+      }),
       create_partner: () => ({ name: `Rule Northwind ${tag}` }),
       update_partner: () => ({
         id: state.partnerId,
@@ -368,6 +383,57 @@ describe("the owner rule", () => {
 
     const outcomes: Record<string, boolean> = {};
     for (const [name, args] of Object.entries(cases)) {
+      if (name === "check_contract_status") {
+        // This fixture template needs no approval. Simulate an existing sent
+        // provider request, then prove refresh only reads its current state.
+        const { contractSigningRepository } =
+          await import("@/src/features/internal-ops/contracts/server");
+        const repository = contractSigningRepository();
+        const id = String(state.templateContractId);
+        const { record, token } = await repository.claim(id);
+        const providerId = randomUUID();
+        await repository.update(
+          id,
+          token,
+          { state: "sent", providerId },
+          { kind: "user", id: person.id, display: person.name },
+        );
+        await repository.release(id, token);
+        contact.readResponses.set(
+          `signwell-contracts.getContract:${providerId}`,
+          {
+            id: providerId,
+            status: "Viewed",
+            test_mode: record.testMode,
+            metadata: {
+              commerce_contract_id: record.contractId,
+              template_sha256: record.templateHash,
+            },
+            apply_signing_order: true,
+            recipients: [
+              {
+                id: "counterparty",
+                name: record.counterpartySigner.name,
+                email: record.counterpartySigner.email,
+              },
+              {
+                id: "fil-one",
+                name: record.countersigner.name,
+                email: record.countersigner.email,
+              },
+            ],
+            fields: [
+              ["counterparty", "fil-one"].flatMap((recipient_id) =>
+                ["signature", "autofill_date_signed"].map((type) => ({
+                  recipient_id,
+                  type,
+                  required: true,
+                })),
+              ),
+            ],
+          },
+        );
+      }
       if (name === "check_mnda_status") {
         // Simulate an already sent request locally, then let the real sync
         // workflow observe a provider read and advance it to viewed.
@@ -442,6 +508,23 @@ describe("the owner rule", () => {
           contact.reads.some((read) => read.startsWith("signwell-mnda.get:")),
         ).toBe(true);
       }
+      if (name === "prepare_from_template") {
+        state.templateContractId = (value.contract as { id: string }).id;
+        state.signingRequestNumber = (
+          value.signing as { requestNumber: number }
+        ).requestNumber;
+      }
+      if (name === "check_contract_status") {
+        expect(value.signing).toMatchObject({
+          state: "viewed",
+          requestNumber: state.signingRequestNumber,
+        });
+        expect(
+          contact.reads.some((read) =>
+            read.startsWith("signwell-contracts.getContract:"),
+          ),
+        ).toBe(true);
+      }
       if (name === "create_mnda_draft" || name === "prepare_mnda_preview")
         state.mndaId = (value.mnda as { id: string } | undefined)?.id ?? "";
       if (name === "record_contract")
@@ -463,6 +546,7 @@ describe("the owner rule", () => {
       create_mnda_draft: true,
       prepare_mnda_preview: true,
       check_mnda_status: true,
+      check_contract_status: true,
       record_contract: true,
       prepare_contract_for_signature: true,
       prepare_from_template: true,

apps/web/src/features/mcp/mcp-pagination.test.ts

@@ -0,0 +1,127 @@
+import { beforeEach, expect, it, vi } from "vitest";
+import type { McpToolContext } from "./tool";
+
+const mocks = vi.hoisted(() => ({
+  staff: vi.fn(),
+  pricing: vi.fn(),
+  handoffs: vi.fn(),
+  partners: vi.fn(),
+  owners: vi.fn(),
+}));
+vi.mock("@/src/auth/session", () => ({
+  explicitDemoIdentityEnabled: () => false,
+  getRequestCommerceSession: mocks.staff,
+}));
+vi.mock("@/src/features/internal-ops/contracts/server", () => ({
+  contractStaff: mocks.staff,
+  sessionHas: vi.fn(),
+}));
+vi.mock(
+  "@/src/features/internal-ops/sales-pricing/scenario-server",
+  async (original) => ({
+    ...(await original<object>()),
+    scenarioRepository: () => ({ listPage: mocks.pricing }),
+  }),
+);
+vi.mock("@/src/features/internal-ops/sales-pricing/scenario-actions", () => ({
+  saveScenario: vi.fn(),
+}));
+vi.mock("@/src/features/internal-ops/handoff/server", () => ({
+  handoffRepository: () => ({ listPage: mocks.handoffs }),
+}));
+vi.mock("@/src/features/internal-ops/handoff/actions", () => ({
+  requestHandoff: vi.fn(),
+}));
+vi.mock("@/src/features/internal-ops/partners/server", () => ({
+  partnerRepository: () => ({ listPage: mocks.partners, owners: mocks.owners }),
+  partnerToday: () => "2026-10-11",
+  loadPartner: vi.fn(),
+}));
+vi.mock("@/src/features/internal-ops/partners/actions", () => ({
+  savePartner: vi.fn(),
+  savePartnerDeal: vi.fn(),
+}));
+
+const { listPricingScenarios } = await import("./tools/pricing");
+const { listHandoffs } = await import("./tools/handoffs");
+const { listPartners } = await import("./tools/partners");
+const userId = "20000000-0000-4000-8000-000000000001";
+const context = () =>
+  ({ origin: "https://commerce.fil.one", targets: {} }) as McpToolContext;
+beforeEach(() => {
+  vi.clearAllMocks();
+  mocks.staff.mockResolvedValue({ userId, roles: ["revenue"] });
+  for (const method of [mocks.pricing, mocks.handoffs, mocks.partners])
+    method.mockResolvedValue({ rows: [], nextCursor: "next-page" });
+  mocks.owners.mockResolvedValue([]);
+});
+
+it.each([false, true])(
+  "forwards pricing pagination while respecting administrator=%s scope",
+  async (admin) => {
+    mocks.staff.mockResolvedValue({
+      userId,
+      roles: [admin ? "commerce_admin" : "revenue"],
+    });
+    expect(
+      await listPricingScenarios.run(
+        { limit: 7, cursor: "previous-page" },
+        context(),
+      ),
+    ).toMatchObject({ scenarios: [], nextCursor: "next-page" });
+    expect(mocks.staff).toHaveBeenCalledWith("sales:read");
+    expect(mocks.pricing).toHaveBeenCalledWith(
+      admin ? { kind: "all" } : { kind: "own", ownerId: userId },
+      { limit: 7, cursor: "previous-page" },
+    );
+  },
+);
+it.each(["mine", "queue"])(
+  "filters %s handoffs in the repository before limiting",
+  async (scope) => {
+    expect(
+      await listHandoffs.run(
+        { scope, status: "open", limit: 3, cursor: "previous-page" },
+        context(),
+      ),
+    ).toMatchObject({ handoffs: [], nextCursor: "next-page" });
+    expect(mocks.staff).toHaveBeenCalledWith(
+      scope === "mine" ? "contract:write" : "operations:read",
+    );
+    expect(mocks.handoffs).toHaveBeenCalledWith(
+      scope === "mine" ? { kind: "own", userId } : { kind: "all" },
+      "open",
+      { limit: 3, cursor: "previous-page" },
+    );
+  },
+);
+it("forwards partner filters, viewer scope and cursor without a capped UI read", async () => {
+  expect(
+    await listPartners.run(
+      {
+        search: "Northwind",
+        status: "active",
+        mine: true,
+        limit: 11,
+        cursor: "previous-page",
+      },
+      context(),
+    ),
+  ).toMatchObject({ partners: [], nextCursor: "next-page", owners: [] });
+  expect(mocks.staff).toHaveBeenCalledWith("sales:read");
+  expect(mocks.partners).toHaveBeenCalledWith(
+    expect.objectContaining({ q: "Northwind", status: "active", mine: true }),
+    { viewerId: userId, today: "2026-10-11" },
+    { limit: 11, cursor: "previous-page" },
+  );
+});
+it.each([listPricingScenarios, listHandoffs, listPartners])(
+  "refuses $name before repository access when permission is missing",
+  async (tool) => {
+    mocks.staff.mockRejectedValue(new Error("CONTRACT_FORBIDDEN"));
+    await expect(tool.run({}, context())).rejects.toThrow("CONTRACT_FORBIDDEN");
+    expect(mocks.pricing).not.toHaveBeenCalled();
+    expect(mocks.handoffs).not.toHaveBeenCalled();
+    expect(mocks.partners).not.toHaveBeenCalled();
+  },
+);

apps/web/src/features/mcp/mcp-transport.test.ts

@@ -3,6 +3,8 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
 const mocks = vi.hoisted(() => ({
   authenticate: vi.fn(),
   recordToolCall: vi.fn(),
+  accessTokenSession: vi.fn(),
+  runWithSession: vi.fn((_session: unknown, run: () => unknown) => run()),
   session: {
     userId: "20000000-0000-4000-8000-0000000000aa",
     profile: { name: "Transport tester", email: "tester@clockwork.test" },
@@ -20,11 +22,11 @@ const mocks = vi.hoisted(() => ({
 
 vi.mock("@/src/auth/access-token-session", () => ({
   accessTokenRepository: () => mocks,
-  accessTokenCommerceSession: () => Promise.resolve(mocks.session),
+  accessTokenCommerceSession: mocks.accessTokenSession,
 }));
 vi.mock("@/src/auth/session", () => ({
   explicitDemoIdentityEnabled: () => false,
-  runWithAccessTokenSession: (_session: unknown, run: () => unknown) => run(),
+  runWithAccessTokenSession: mocks.runWithSession,
 }));
 vi.mock("@/src/auth/demo-deploy", () => ({
   demoDeployIdentityEnabled: () => false,
@@ -71,10 +73,61 @@ beforeEach(() => {
     windowResetSeconds: 60,
   });
   mocks.recordToolCall.mockReset().mockResolvedValue(undefined);
+  mocks.accessTokenSession.mockReset().mockResolvedValue(mocks.session);
+  mocks.runWithSession.mockClear();
 });
 
 afterEach(() => vi.unstubAllEnvs());
 
+describe("MCP authentication availability", () => {
+  it.each(["authenticate", "accessTokenSession"] as const)(
+    "returns a sanitized retryable outage when %s fails before a tool can run",
+    async (stage) => {
+      mocks[stage].mockRejectedValueOnce(
+        new Error("private database URL and identity details"),
+      );
+      const incoming = request();
+      const response = await handleMcpRequest(
+        new Request(incoming, {
+          body: JSON.stringify({
+            jsonrpc: "2.0",
+            id: 9,
+            method: "tools/call",
+            params: { name: "whoami", arguments: {} },
+          }),
+        }),
+      );
+      expect(response.status).toBe(503);
+      expect(response.headers.get("content-type")).toContain(
+        "application/problem+json",
+      );
+      expect(response.headers.get("retry-after")).toBe("5");
+      expect(response.headers.get("www-authenticate")).toBeNull();
+      const body = await response.text();
+      expect(JSON.parse(body)).toMatchObject({
+        code: "MCP_UNAVAILABLE",
+        status: 503,
+      });
+      expect(body).not.toContain("private");
+      expect(mocks.runWithSession).not.toHaveBeenCalled();
+      expect(mocks.recordToolCall).not.toHaveBeenCalled();
+    },
+  );
+
+  it("keeps invalid tokens and missing memberships as authentication refusals", async () => {
+    mocks.authenticate.mockResolvedValueOnce(null);
+    const invalidToken = await handleMcpRequest(request());
+    expect(invalidToken.status).toBe(401);
+    expect(invalidToken.headers.get("www-authenticate")).toContain(
+      "invalid_token",
+    );
+    mocks.accessTokenSession.mockResolvedValueOnce(null);
+    const missingMembership = await handleMcpRequest(request());
+    expect(missingMembership.status).toBe(401);
+    expect(mocks.runWithSession).not.toHaveBeenCalled();
+  });
+});
+
 describe("MCP transport origin validation", () => {
   it.each([
     "https://unrelated.invalid",