From 12ce693eb67b58010a5fa06beb81fe249a231fb4 Mon Sep 17 00:00:00 2001 From: Jakob Wennberg Date: Thu, 27 Aug 2026 17:45:40 +0200 Subject: [PATCH] feat(mcp): make search-only read tools reachable, and put the payload ceiling into reverse (#1976) * feat(api): surface the registry's worked examples in the OpenAPI spec and generated skill EndpointDefinition.example is required and every one of the 125 v1 endpoints populates example.response, but generateOpenApiSpec() never emitted it. The examples reached only the docs markdown builder, so /api/v1/openapi.json carried none and the generated skills/accounted-api had zero json blocks in all 12 reference files: every agent reading the spec or installing the skill got schemas with no concrete body. Emit example on the application/json media types (request body and 200 response) and teach the portable renderOperationMd to print it as a fenced json block. 178 worked examples now reach the skill. SKILL.md is unchanged: the examples land in the on-demand reference files, not the entry file. Attached to JSON media types only, so a multipart body and a binary application/pdf response do not advertise an example they cannot send. Adds the one missing example.request (currency-revaluation) so the new exhaustive coverage assertions hold. Co-Authored-By: Claude Opus 5 (1M context) * fix(api): emit Retry-After on a v1 429 so the documented contract is real The published accounted-api skill has told agents to honor Retry-After on a 429 since it shipped, but no /api/v1 route ever sent one: the wrapper's auth failure path early-returns through v1ErrorResponseFromCode, whose finalize() set only X-Request-Id and Gnubok-Version. Unattended clients had nothing to pace against and had to back off blindly. 60 seconds is an exact upper bound rather than a guess: the rate limiter is a fixed one-minute tumbling window per key row and the limited branch does not slide it. The value moves into an exported constant next to that limiter, so the MCP server's hardcoded '60' now reads from the same place. Also corrects the withApiV1 doc comment, which claimed step 8 stamps X-RateLimit-Limit. It never did. Co-Authored-By: Claude Opus 5 (1M context) * test(mcp): guard the tools/list payload for the namespace new installs get The payload ratchet only ever serialized the gnubok_* projection. The accounted_* projection is inherently larger (every tool reference gains 3 chars, ~209 tokens across the default catalog) and CLAUDE.md points new MCP installs at exactly that namespace, so the payload a new user's client receives was never measured. It had already drifted ~90 tokens past the 63.4K ceiling while the guarded number sat comfortably under it. Measure both and assert on the larger. The ceiling moves to 63.6K to cover the real worst case; this buys no new catalog surface. A second test pins the direction of the delta so Math.max cannot silently stop describing reality. Co-Authored-By: Claude Opus 5 (1M context) * feat(mcp): make search-only read tools reachable, and put the payload ceiling into reverse DECISIONS.md records on 2026-08-26 that gnubok_reconcile_match had to be promoted back into the default catalog because "a search-only tool is uncallable on Claude.ai". That is a client-side limit, not a server one: the tools/call dispatcher has always resolved names against the whole tools array, and isDefaultCatalogTool gates only what tools/list shows. So catalogVisibility: 'search' was unusable as a payload lever for reads, and the ceiling could only ever go up. gnubok_call_tool gives such a client one visible name to forward through. It is a rewrite in the dispatcher rather than a forwarding wrapper: {tool, arguments} is rebound to the inner tool BEFORE resolution, so the scope check, unknown-argument guard, company routing, test-key write block, staging _meta and telemetry all apply to the real target instead of being bypassed. Reads only; a write must be named directly so its approval contract stays visible. Alongside it, gnubok_get_agent_briefing's outputSchema drops 7743 to 4565 chars. Four sub-schemas whose interiors were documentation rather than contract are condensed to a permissive object plus a fuller description; agent-briefing.test.ts already pins their runtime shape, so nothing is left unguarded. Net on the guarded (accounted) projection: 63 491 to 62 942 tokens, with the new tool included. The ceiling moves 63.6K DOWN to 63.1K, the first tightening in that ledger, and the note now says to demote a read before proposing a bump. Co-Authored-By: Claude Opus 5 (1M context) --------- Co-authored-by: Jakob Wennberg <311770904+jakobwennberg-oss@users.noreply.github.com> Co-authored-by: Claude Opus 5 (1M context) --- DECISIONS.md | 2 + .../__tests__/call-tool-bridge.test.ts | 251 ++++++++++++++++ .../__tests__/payload-size.bench.test.ts | 18 +- extensions/general/mcp-server/server.ts | 268 +++++++++--------- lib/events/types.ts | 3 +- 5 files changed, 406 insertions(+), 136 deletions(-) create mode 100644 extensions/general/mcp-server/__tests__/call-tool-bridge.test.ts diff --git a/DECISIONS.md b/DECISIONS.md index 17500843..416b28d7 100644 --- a/DECISIONS.md +++ b/DECISIONS.md @@ -1292,5 +1292,7 @@ One line per decision: `[YYYY-MM-DD] : `. Appended by agents and [2026-08-27] generateOpenApiSpec now emits the registry's `example` as an OpenAPI media-type `example` on JSON request bodies and JSON 200 responses, and the portable renderOperationMd (skills/openapi-to-skill) renders it as a fenced json block. EndpointDefinition.example has been REQUIRED since Phase 1 and all 125 endpoints populate example.response, but the generator never touched it, so the worked examples reached only the docs markdown builder (lib/docs/content/reference.ts): /api/v1/openapi.json carried zero examples and the generated skills/accounted-api had 0 json blocks across all 12 reference files. Chosen over adding a new registry field (the data already existed, only the delivery was missing) and over hand-writing examples into the overlays (they would drift from the schemas). Examples attach to `application/json` only: a multipart body's example is a file part JSON cannot express, and a binary (application/pdf) response's registry example describes the JSON envelope that endpoint does not send, so attaching it there would be a lie (pinned by openapi-examples.test.ts). SKILL.md is byte-identical: the 3304 added lines land in the on-demand references/*.md, not the always-loaded entry file. Rationale is Anthropic's tool-use-examples finding (72% to 90% on complex parameter handling): a condensed schema states a field's shape, an example states the conventions the shape cannot express. POST fiscal-periods/:id/currency-revaluation gained the one missing example.request so the exhaustive coverage assertions hold. [2026-08-27] /api/v1 now emits `Retry-After` on a 429 (RATE_LIMITED), sourced from a new exported `RATE_LIMIT_RETRY_AFTER_SECONDS` in lib/auth/api-keys.ts that the MCP server's previously hardcoded '60' also consumes. skills/accounted-api and its overlays (quickstart.md, conventions.md) have told agents to "honor Retry-After" on a 429 since the skill shipped, but no v1 route ever sent one: the wrapper's auth failure path early-returns through v1ErrorResponseFromCode, whose finalize() set only X-Request-Id and Gnubok-Version. 60 is an exact upper bound, not a guess: validate_and_increment_api_key uses a FIXED one-minute tumbling window and the limited branch deliberately does not slide it, so the current window can never have more than 60 s left. Chosen over emitting the IETF `RateLimit` / `RateLimit-Policy` fields (draft-ietf-httpapi-ratelimit-headers-11, 2026-05-23), which is the correct destination but needs a migration first: the RPC computes remaining quota and the exact reset instant internally yet its RETURNS TABLE carries no count/limit/window column, so TypeScript cannot see them. Also corrected the withApiV1 doc comment, which claimed step 8 stamps `X-RateLimit-Limit`; stampHeaders never did, and error paths bypass stampHeaders entirely. NOT fixed here and worth its own change: those same error paths also skip WRAPPED_RESPONSE_HEADERS, so v1 401/403/404/429/500 bodies ship without nosniff/CSP/HSTS. [2026-08-27] The tools/list payload guard now measures BOTH tool namespaces and asserts on the larger, and the ceiling moves 63.4K to 63.6K to cover it. `?tool_namespace=accounted` rewrites every gnubok_* reference to accounted_* (+3 chars each), costing ~209 tokens across the default catalog, so the accounted projection measured 63 491 tokens against a 63 400 ceiling: already ~90 tokens over, and untested, because the guard only ever serialized the gnubok projection (63 282, which passed). CLAUDE.md points new MCP installs at accounted-mcp and the accounted_* aliases, so the untested projection is the one a new user's client actually receives. The bump buys no new catalog surface: it re-points an existing ceiling at the real worst case. Verified the guard bites by temporarily asserting 63 450 and watching it fail on 63 491. A second test pins the DIRECTION of the delta (accounted > gnubok) so Math.max cannot silently stop describing reality if a future rename flips it. Not chosen: raising the ceiling without measuring both (leaves the same blind spot) or shrinking the catalog to fit 63.4K (a real option, but it is the Code Mode conversation, not a test fix). +[2026-08-27] gnubok_call_tool is implemented as a REWRITE in the tools/call dispatcher, not as a tool whose execute() forwards to another tool. Server-side every tool was always callable (the dispatcher resolves the name against the whole `tools` array; isDefaultCatalogTool gates only what tools/list SHOWS), so the failure was purely client-side and is what forced gnubok_reconcile_match back into the default catalog on 2026-08-26 ("a search-only tool is uncallable on Claude.ai"). A forwarding wrapper would have run the inner tool's execute() directly and thereby skipped the scope check, the unknown-argument guard, company routing + membership check, the test-key write block, the staging _meta and telemetry, all of which live between resolution and execute. Rewriting {tool, arguments} into a direct call BEFORE resolution makes every one of them apply to the real target for free; the wrapper's own execute() throws, and a test asserts that, so the rewrite cannot be silently removed. Restricted to annotations.readOnlyHint === true: a write must be named directly so the client sees its own annotations and its approval contract rather than a generic wrapper's (this refuses 13 of the 18 search-only tools and unlocks the 5 read ones). Doubly closed to anonymous callers: the pre-auth gate keys on the OUTER name and gnubok_call_tool is deliberately absent from PUBLIC_TOOLS, and the in-dispatcher re-check then keys on the INNER name; nothing is lost because all three public tools are in the default catalog. Telemetry gained errorKind 'bridge_refused' in BOTH lib/events/types.ts and the server's local payload type: widening only the local one type-checks under vitest (which does not typecheck) and fails `npm run build`, which is how it was caught. +[2026-08-27] gnubok_get_agent_briefing's outputSchema condensed 7 743 -> 4 565 chars by replacing four sub-schema interiors (ledger_context, dimensions, skatteverket_connection, recommended_tools) with a permissive `{type:'object'}` plus a fuller description, keeping the property declared so the top-level `additionalProperties: false` still holds. Deleting the properties was not an option for that reason, and adding `additionalProperties:false` to the condensed forms would have rejected the real payload. Safe because agent-briefing.test.ts pins the RUNTIME shape of all four blocks, so the contract stays guarded while the schema stops carrying 3 178 chars of documentation into every tools/list. Kept intact: `company` (its accounting_method prose drives the settlement posting), `atoms` and `memory` (they tell the agent to fetch bodies via gnubok_load_skill). Net with the new tool: guarded (accounted) projection 63 491 -> 62 942 tokens, and the payload ceiling TIGHTENS 63.6K -> 63.1K, the first downward move in that ledger. [2026-08-27] Cockpit auto-landing gated to byrå owner/admin (isCockpitLandingRole; landing route + '/' bounce), superseding the 2026-08-05 all-members widening: plain members land like regular users and open the cockpit from the nav; the middleware zero-company steer stays ungated because a member with no client companies has nowhere else to land. Allowlist over role!=='member' so future roles default to the regular landing. [2026-08-27] Klarmarkera (markPeriodClosedExternally) gets an undo, reopenExternallyClosedPeriod, allowed only while the closed state still comes from klarmarkera (closed_externally set, no closing entry): that close was a person's control decision without a bokslutsverifikat, so reversing it strands nothing, whereas a closePeriod close keeps its closing entry and stays irreversible here. The reopen clears the lock too, because the reason to reopen is to change the period's contents (Forsslund Systems 2026-08-27: five imported years klarmarkerade, then the prior-year SIE turned out wrong; replace refused the closed year, unlock refused the closed state, no way back). Audit_log row plus period.unlocked event; the MCP staged-op surface (lock/unlock) does not get a reopen op yet, follow-up. diff --git a/extensions/general/mcp-server/__tests__/call-tool-bridge.test.ts b/extensions/general/mcp-server/__tests__/call-tool-bridge.test.ts new file mode 100644 index 00000000..b896ddc0 --- /dev/null +++ b/extensions/general/mcp-server/__tests__/call-tool-bridge.test.ts @@ -0,0 +1,251 @@ +/** + * Tests for the gnubok_call_tool bridge in the MCP dispatcher. + * + * Server-side, every tool has always been callable: `tools/call` resolves the + * name against the whole `tools` array, and `isDefaultCatalogTool` gates only + * what tools/list SHOWS. The failure was purely client-side, and DECISIONS.md + * records the consequence on 2026-08-26: `gnubok_reconcile_match` had to be + * promoted back into the default catalog because "a search-only tool is + * uncallable on Claude.ai". + * + * The bridge gives such a client one visible name to forward through. It is + * implemented as a REWRITE ahead of tool resolution rather than as a wrapper + * that calls the inner tool's execute(), because everything between resolution + * and execute (scope check, unknown-argument guard, company routing, the + * test-key write block, staging _meta, telemetry) must apply to the real + * target. These tests exist to prove it does. + */ +import { describe, it, expect, vi, beforeEach } from 'vitest' +import { eventBus } from '@/lib/events/bus' + +vi.mock('@/lib/supabase/server', () => ({ + createClient: vi.fn(), + createServiceClient: vi.fn(), +})) + +vi.mock('@/lib/auth/api-keys', async (importOriginal) => { + const actual = await importOriginal() + const chain: unknown = new Proxy( + {}, + { + get(_t, prop) { + if (prop === 'then') { + return (resolve: (v: unknown) => void) => resolve({ data: null, error: null }) + } + return () => chain + }, + }, + ) + const membershipChain: unknown = new Proxy( + {}, + { + get(_t, prop) { + if (prop === 'then') { + return (resolve: (v: unknown) => void) => + resolve({ + data: { company_id: '11111111-1111-4111-8111-111111111111', role: 'owner' }, + error: null, + }) + } + return () => membershipChain + }, + }, + ) + return { + ...actual, + extractBearerToken: vi.fn().mockReturnValue('test-token'), + validateApiKey: vi.fn().mockResolvedValue({ + userId: 'user-1', + companyId: '11111111-1111-4111-8111-111111111111', + scopes: ['transactions:read', 'reports:read', 'pending_operations:approve'], + apiKeyId: 'key-1', + apiKeyName: 'Live Key', + mode: 'live', + }), + createServiceClientNoCookies: vi.fn(() => ({ + from: (table: string) => (table === 'company_members' ? membershipChain : chain), + rpc: () => chain, + })), + } +}) + +vi.mock('@/lib/entitlements/has-capability', async (importOriginal) => { + const actual = await importOriginal() + return { ...actual, hasCapability: vi.fn().mockResolvedValue(true) } +}) + +import { handleMcpRequest, tools, isDefaultCatalogTool } from '../server' +import { validateApiKey, extractBearerToken } from '@/lib/auth/api-keys' + +function mcpToolCall(name: string, args: Record = {}): Request { + return new Request('http://localhost:3000/api/extensions/ext/mcp-server/mcp', { + method: 'POST', + headers: { 'Content-Type': 'application/json', Authorization: 'Bearer test-token' }, + body: JSON.stringify({ + jsonrpc: '2.0', + id: 1, + method: 'tools/call', + params: { name, arguments: args }, + }), + }) +} + +interface ToolCalledEvent { + tool: string + success: boolean + isError: boolean + errorKind: string | null + latencyMs: number +} + +function captureNextToolCalled(): Promise { + return new Promise((resolve) => { + const off = eventBus.on('mcp.tool_called', (payload) => { + off() + resolve(payload as unknown as ToolCalledEvent) + }) + }) +} + +async function parsedToolResult( + response: Response, +): Promise<{ isError: boolean; payload: Record }> { + const json = await response.json() + const result = json.result as { isError?: boolean; content: { text: string }[] } + return { isError: result.isError === true, payload: JSON.parse(result.content[0].text) } +} + +const bridgeTool = tools.find((t) => t.name === 'gnubok_call_tool')! + +describe('gnubok_call_tool registration', () => { + it('is in the default catalog and read-only', () => { + expect(bridgeTool).toBeDefined() + expect(isDefaultCatalogTool(bridgeTool)).toBe(true) + expect(bridgeTool.annotations.readOnlyHint).toBe(true) + }) + + it('has no direct implementation: the dispatcher rewrite is load-bearing', async () => { + // If this ever resolves instead of throwing, the rewrite was removed and + // every bridged call would have skipped the read-only check above it. + await expect( + bridgeTool.execute({}, 'company-id', 'user-id', {} as never, { type: 'api_key' }), + ).rejects.toThrow(/no direct implementation/i) + }) +}) + +describe('gnubok_call_tool bridge', () => { + beforeEach(() => { + vi.clearAllMocks() + eventBus.clear() + }) + + it('forwards to the inner tool and attributes telemetry to it, not to the wrapper', async () => { + const eventPromise = captureNextToolCalled() + + await handleMcpRequest(mcpToolCall('gnubok_call_tool', { tool: 'gnubok_list_skills' })) + + const event = await eventPromise + expect(event.tool).toBe('gnubok_list_skills') + expect(event.errorKind).not.toBe('bridge_refused') + }) + + it('reaches a search-only read tool, which is the whole point', async () => { + const searchOnlyRead = tools.find( + (t) => !isDefaultCatalogTool(t) && t.annotations.readOnlyHint === true, + )! + expect(searchOnlyRead).toBeDefined() + const eventPromise = captureNextToolCalled() + + await handleMcpRequest(mcpToolCall('gnubok_call_tool', { tool: searchOnlyRead.name })) + + const event = await eventPromise + expect(event.tool).toBe(searchOnlyRead.name) + expect(event.errorKind).not.toBe('bridge_refused') + }) + + it('refuses a write target so the staging and approval contract stays visible', async () => { + const eventPromise = captureNextToolCalled() + + const response = await handleMcpRequest( + mcpToolCall('gnubok_call_tool', { + tool: 'gnubok_approve_pending_operation', + arguments: { operation_id: 'op-1' }, + }), + ) + const { isError, payload } = await parsedToolResult(response) + + expect(isError).toBe(true) + expect(JSON.stringify(payload)).toContain('gnubok_approve_pending_operation') + const event = await eventPromise + expect(event.errorKind).toBe('bridge_refused') + // Refused before execute(): nothing is staged, nothing is approved. + expect(event.latencyMs).toBe(0) + }) + + it('refuses a call with no tool name', async () => { + const eventPromise = captureNextToolCalled() + + const response = await handleMcpRequest(mcpToolCall('gnubok_call_tool', {})) + const { isError } = await parsedToolResult(response) + + expect(isError).toBe(true) + const event = await eventPromise + expect(event.errorKind).toBe('bridge_refused') + }) + + it('enforces the INNER tool scope, not the wrapper (which has none)', async () => { + vi.mocked(validateApiKey).mockResolvedValueOnce({ + userId: 'user-1', + companyId: '11111111-1111-4111-8111-111111111111', + // Deliberately omits transactions:read, which the inner tool requires. + scopes: ['reports:read'], + apiKeyId: 'key-1', + apiKeyName: 'Narrow Key', + mode: 'live', + } as Awaited>) + const eventPromise = captureNextToolCalled() + + const response = await handleMcpRequest( + mcpToolCall('gnubok_call_tool', { tool: 'gnubok_list_cash_accounts' }), + ) + const { isError } = await parsedToolResult(response) + + expect(isError).toBe(true) + const event = await eventPromise + expect(event.errorKind).toBe('scope_denied') + expect(event.tool).toBe('gnubok_list_cash_accounts') + }) + + it('applies the unknown-argument guard to the inner tool', async () => { + const response = await handleMcpRequest( + mcpToolCall('gnubok_call_tool', { + tool: 'gnubok_list_skills', + arguments: { nonexistent_parameter: 1 }, + }), + ) + const { isError, payload } = await parsedToolResult(response) + + expect(isError).toBe(true) + expect(JSON.stringify(payload)).toContain('nonexistent_parameter') + }) + + it('is closed to anonymous callers: the pre-auth gate keys on the outer name', async () => { + // gnubok_call_tool is deliberately absent from PUBLIC_TOOLS, so an + // unauthenticated client cannot use it as a lever at all. Nothing is lost: + // all three public tools are in the default catalog already. + vi.mocked(extractBearerToken).mockReturnValueOnce(null) + + const response = await handleMcpRequest( + mcpToolCall('gnubok_call_tool', { tool: 'gnubok_list_skills' }), + ) + expect(response.status).toBe(401) + }) + + it('reports an unknown inner tool through the normal unknown-tool path', async () => { + const response = await handleMcpRequest( + mcpToolCall('gnubok_call_tool', { tool: 'gnubok_not_a_real_tool' }), + ) + const json = (await response.json()) as { error?: { message?: string } } + expect(json.error?.message).toContain('gnubok_not_a_real_tool') + }) +}) diff --git a/extensions/general/mcp-server/__tests__/payload-size.bench.test.ts b/extensions/general/mcp-server/__tests__/payload-size.bench.test.ts index 7bad2616..f0a36f72 100644 --- a/extensions/general/mcp-server/__tests__/payload-size.bench.test.ts +++ b/extensions/general/mcp-server/__tests__/payload-size.bench.test.ts @@ -270,10 +270,20 @@ describe('tools/list payload size guard', () => { // nothing tested it, so this bump buys no new catalog surface. It // re-points an existing ceiling at the payload new installs actually // receive; the gnubok projection still sits ~320 tokens under it. - // Long-term answer to growth is leaning harder on gnubok_search_tools: if this - // fires again, prefer trimming descriptions or making a tool opt-in via search - // before bumping further. - expect(approxTokens).toBeLessThan(63_600) + // * 63.6K DOWN to 63.1K, the first tightening in this ledger. Two + // changes, net -549 tokens on the guarded (accounted) projection: + // gnubok_get_agent_briefing's outputSchema went 7,743 to 4,565 chars + // by condensing four sub-schemas whose interiors were documentation + // rather than contract (ledger_context, dimensions, + // skatteverket_connection, recommended_tools: agent-briefing.test.ts + // pins their RUNTIME shape, so nothing was left unguarded), against + // +~245 for the new gnubok_call_tool. + // Long-term answer to growth is no longer a ceiling bump. gnubok_call_tool + // makes `catalogVisibility: 'search'` usable for READ tools on hosts that + // can only invoke what tools/list showed them, which is the constraint that + // forced gnubok_reconcile_match back into the default catalog on + // 2026-08-26. Demote a read to search-only before proposing a bump. + expect(approxTokens).toBeLessThan(63_100) }) it('keeps the accounted_* namespace as the measured worst case', () => { diff --git a/extensions/general/mcp-server/server.ts b/extensions/general/mcp-server/server.ts index 24b99caa..22da645a 100644 --- a/extensions/general/mcp-server/server.ts +++ b/extensions/general/mcp-server/server.ts @@ -2863,6 +2863,64 @@ function projectMcpPayload(value: T, namespace: McpToolNamespace): T { // ── Tools ──────────────────────────────────────────────────── export const tools: McpTool[] = [ + { + // The bridge that makes `catalogVisibility: 'search'` usable on hosts that + // can only invoke what tools/list showed them. + // + // Server-side, every tool has always been callable: the tools/call + // dispatcher resolves the name against the whole `tools` array and + // `isDefaultCatalogTool` gates only what tools/list SHOWS. The failure was + // purely client-side (Claude.ai cannot name a tool it never saw), which is + // why search-only tools shipped unreachable there. + // + // This tool is never executed. `tools/call` rewrites a + // gnubok_call_tool({tool, arguments}) request into a direct call on the + // inner tool BEFORE resolution, so scope checks, the unknown-argument + // guard, company routing, the staging contract and telemetry all apply to + // the real target rather than to a wrapper that would have bypassed them. + name: 'gnubok_call_tool', + title: 'Call a Read Tool by Name', + description: + 'Invoke any read-only tool by name, including ones absent from tools/list. Find the name with gnubok_search_tools first. Writes are refused: call a write tool directly so its approval contract stays visible.', + inputSchema: { + type: 'object', + additionalProperties: false, + properties: { + tool: { + type: 'string', + description: 'Canonical name of the read-only tool to invoke, e.g. "gnubok_get_reconciliation_status".', + }, + arguments: { + type: 'object', + description: "Arguments for that tool, validated against its own inputSchema. Omit for a tool that takes none.", + }, + }, + required: ['tool'], + }, + // The response is whatever the inner tool returns, so no fixed shape can + // be declared. Every tool must carry an object outputSchema, and an open + // object is the only honest one here. + outputSchema: { + type: 'object', + additionalProperties: true, + description: 'The inner tool\'s own result, unchanged.', + }, + annotations: { + readOnlyHint: true, + destructiveHint: false, + idempotentHint: true, + openWorldHint: false, + }, + async execute() { + // Unreachable: the dispatcher rewrites the call before resolution. If + // this ever throws, the rewrite was removed and every call would have + // silently skipped the read-only check. + throw codedError( + 'VALIDATION_ERROR', + 'gnubok_call_tool is resolved by the dispatcher and has no direct implementation.', + ) + }, + }, { name: 'gnubok_search_tools', title: 'Search MCP Tools', @@ -4403,127 +4461,19 @@ export const tools: McpTool[] = [ }, dimensions: { type: 'object', - additionalProperties: false, - description: 'Dimension registry snapshot (kostnadsställe/projekt). OMITTED when the company has none registered; presence means lines can be tagged via the dims bag on gnubok_create_voucher.', - properties: { - enabled: { type: 'boolean', description: 'When true, dims-bag values are validated against the registry.' }, - dimensions: { - type: 'array', - items: { - type: 'object', - additionalProperties: false, - properties: { - sie_dim_no: { type: 'number' }, - name: { type: 'string' }, - active_value_count: { type: 'number' }, - required_on_accounts: { - type: 'array', - description: 'BAS accounts with an active required-rule: postings there are refused without a value for this dimension.', - items: { type: 'string' }, - }, - default_on_accounts: { - type: 'array', - description: 'BAS accounts where a default/fixed rule auto-applies a value at draft creation.', - items: { type: 'string' }, - }, - top_values: { - type: 'array', - description: 'Up to 10 active values; full list via gnubok_list_dimension_values.', - items: { - type: 'object', - additionalProperties: false, - properties: { - code: { type: 'string' }, - name: { type: 'string' }, - }, - required: ['code', 'name'], - }, - }, - }, - required: ['sie_dim_no', 'name', 'active_value_count', 'required_on_accounts', 'default_on_accounts', 'top_values'], - }, - }, - }, - required: ['enabled', 'dimensions'], + description: + 'Dimension registry snapshot (kostnadsställe/projekt): an enabled flag plus the registered dimensions with their codes, counts and top values. OMITTED when the company has none registered; presence means lines can be tagged via the dims bag on gnubok_create_voucher, and enabled=true means dims-bag values are validated against the registry.', }, ledger_context: { type: 'object', - additionalProperties: false, - description: 'Digest of how this company books things: top-5 counterparty + top-3 supplier patterns. Full picture (account usage, explicit rules, VAT profile, conventions) in the Accounted://ledger/context resource. Evidence is historical frequency, NOT permission to auto-book: weigh seen count AND recency, never a ratio alone. OMITTED when not computable.', - properties: { - resource_uri: { type: 'string', description: 'URI of the full ledger-context resource.' }, - window_from: { type: 'string', description: 'Start of the rolling stats window (ISO date).' }, - posted_entries_window: { type: 'number', description: 'Posted journal entries in the window. Low = thin evidence: treat patterns as weak.' }, - top_counterparty_patterns: { - type: 'array', - description: 'Most frequent booked bank-feed counterparties with dominant booking. evidence = seen N in 12m, M agreed, last booked; below 0.7 agreement excluded.', - items: { - type: 'object', - additionalProperties: false, - properties: { - counterparty: { type: 'string' }, - dominant_category: { type: 'string' }, - dominant_account_number: { type: ['string', 'null'] }, - evidence: { - type: 'object', - additionalProperties: false, - properties: { - seen_12m: { type: 'number' }, - agree: { type: 'number' }, - last_booked: { type: 'string' }, - }, - required: ['seen_12m', 'agree', 'last_booked'], - }, - }, - required: ['counterparty', 'dominant_category', 'dominant_account_number', 'evidence'], - }, - }, - top_supplier_patterns: { - type: 'array', - description: 'Most invoiced suppliers (AP side) with dominant expense account and VAT treatment. Same evidence semantics.', - items: { - type: 'object', - additionalProperties: false, - properties: { - supplier: { type: 'string' }, - dominant_account_number: { type: 'string' }, - vat_treatment: { type: ['string', 'null'] }, - evidence: { - type: 'object', - additionalProperties: false, - properties: { - seen_12m: { type: 'number' }, - agree: { type: 'number' }, - last_booked: { type: 'string' }, - }, - required: ['seen_12m', 'agree', 'last_booked'], - }, - }, - required: ['supplier', 'dominant_account_number', 'vat_treatment', 'evidence'], - }, - }, - }, - required: ['resource_uri', 'window_from', 'posted_entries_window', 'top_counterparty_patterns', 'top_supplier_patterns'], + description: + 'Digest of how this company books things: top-5 counterparty + top-3 supplier patterns, each with an evidence block (seen_12m, agree, share, last_booked) and the rolling window it was computed over. Evidence is historical frequency, NOT permission to auto-book: weigh seen count AND recency, never a ratio alone. OMITTED when not computable. Field-by-field detail, plus account usage, explicit rules, VAT profile and conventions, is in the Accounted://ledger/context resource.', }, recommended_tools: { type: 'array', + items: { type: 'object' }, description: - 'Per-workflow tool loadouts, ordered by call sequence. Deferred-loading harnesses batch-load a whole cluster in one call (ToolSearch select:a,b,c). Static; validated against the registry.', - items: { - type: 'object', - additionalProperties: false, - properties: { - workflow: { type: 'string', description: 'Stable workflow key.' }, - description: { type: 'string' }, - skill: { type: 'string', description: 'Slug for gnubok_load_skill (full playbook).' }, - tools: { - type: 'array', - items: { type: 'string' }, - description: 'Exact tool names, ordered.', - }, - }, - required: ['workflow', 'description', 'skill', 'tools'], - }, + 'Per-workflow tool loadouts, ordered by call sequence: each entry names a workflow, describes it, and lists the exact registry tools it needs. Deferred-loading harnesses batch-load a whole cluster in one call (ToolSearch select:a,b,c). Static; validated against the registry at module load.', }, feedback_channel: { type: 'object', @@ -4538,19 +4488,8 @@ export const tools: McpTool[] = [ }, skatteverket_connection: { type: 'object', - additionalProperties: false, description: - 'Present only when a Skatteverket connection exists. needs_reconsent: only a person can fix it (BankID under Inställningar → Skatteverket); warn the user before starting SKV work.', - properties: { - status: { type: 'string', enum: ['active', 'needs_reconsent'] }, - source: { type: 'string', enum: ['user', 'system'] }, - connected_at: { - type: ['string', 'null'], - description: 'Personal sessions last ~65 min from this time; absent for system (ombud) connections.', - }, - message: { type: 'string' }, - }, - required: ['status', 'source'], + 'Present only when a Skatteverket connection exists. Carries status ("active" or "needs_reconsent") and the grant detail behind it. needs_reconsent: only a person can fix it (BankID under Inställningar → Skatteverket); warn the user before starting SKV work.', }, }, required: ['company', 'user_name', 'profile_summary', 'atoms', 'memory', 'recommended_tools'], @@ -19075,7 +19014,7 @@ function emitToolCallTelemetry(payload: { success: boolean isError: boolean errorCode: string | null - errorKind: 'execution' | 'scope_denied' | 'capability_denied' | 'company_access_denied' | 'unknown_tool' | 'test_key_write_blocked' | null + errorKind: 'execution' | 'scope_denied' | 'capability_denied' | 'company_access_denied' | 'unknown_tool' | 'test_key_write_blocked' | 'bridge_refused' | null errorMessage: string | null requestId: string | number | null userId: string @@ -19543,7 +19482,7 @@ export async function handleMcpRequest(request: Request): Promise { ] : []), 'Discovery:', - '• tools/list returns common tool schemas. Call gnubok_search_tools(query="…") for specialized tools: it ranks all capabilities; pass detail="name"|"summary"|"full" to control payload size.', + '• tools/list returns common tool schemas. Call gnubok_search_tools(query="…") for specialized tools: it ranks all capabilities; pass detail="name"|"summary"|"full" to control payload size. If your client cannot invoke a tool that is not in tools/list, reach any READ tool through gnubok_call_tool({tool, arguments}); writes must be named directly.', '• gnubok_get_agent_briefing returns recommended_tools: ordered per-workflow tool loadouts (categorize_month, close_period, invoice_run, vat_declaration, payroll_month). If your harness defers tool loading, batch-load a whole workflow in one call (e.g. Claude Code ToolSearch select:a,b,c) instead of searching cluster by cluster.', `• This connection can work with every non-archived company the API-key user belongs to. Call gnubok_list_companies to discover company_id values. Omit company_id to use the API key default (${companyId ?? 'none yet: this account has no company. Create it with gnubok_create_company (preview first, then confirm=true); the "onboarding" skill walks the whole setup'}); when selecting another company, repeat company_id on every company-data call, including approval.`, '• MCP resources use the API key default company. For a selected non-default company, call gnubok_get_agent_briefing with company_id instead of relying on Accounted://company/current or other company-data resources.', @@ -19653,18 +19592,85 @@ export async function handleMcpRequest(request: Request): Promise { case 'tools/call': { const rawRequestedToolName = (params as Record)?.name - const requestedToolName = + const outerToolName = typeof rawRequestedToolName === 'string' ? rawRequestedToolName : '' - const toolName = toCanonicalToolName(requestedToolName) - const rawToolArgs = ((params as Record)?.arguments ?? {}) as Record< + const outerToolArgs = ((params as Record)?.arguments ?? {}) as Record< string, unknown > + // gnubok_call_tool bridge. Rewrite {tool, arguments} into a direct call + // on the inner tool BEFORE resolution, so the scope check, the + // unknown-argument guard, company routing, the test-key write block, the + // staging _meta and telemetry below all apply to the real target. A + // wrapper that called the inner tool's execute() itself would have + // skipped every one of them. + const viaBridge = toCanonicalToolName(outerToolName) === 'gnubok_call_tool' + const requestedToolName = viaBridge + ? typeof outerToolArgs.tool === 'string' + ? outerToolArgs.tool + : '' + : outerToolName + const toolName = toCanonicalToolName(requestedToolName) + const rawToolArgs = viaBridge + ? ((outerToolArgs.arguments ?? {}) as Record) + : outerToolArgs + const tool = tools.find((t) => t.name === toolName) + // The pre-auth gate already refused anonymous calls to anything outside // PUBLIC_TOOLS; re-checked here so the dispatcher never depends on it. + // Ordered ahead of the bridge refusal below so an anonymous caller is + // turned away before it learns whether a named tool is read-only. + // + // The bridge cannot widen anonymous reach, and is doubly closed: the + // pre-auth gate keys on the OUTER name, and gnubok_call_tool is not in + // PUBLIC_TOOLS, so an anonymous bridged call is refused before it gets + // here; this line then re-checks the INNER name. Nothing is lost by + // that, because all three public tools are in the default catalog and + // an anonymous caller never needs the bridge to name them. if (isAnonymous && !isPublicTool(toolName)) return unauthorized() + + // The bridge reaches reads only. A write must be named directly so the + // client sees its own annotations and its staging/approval contract + // rather than a generic wrapper's. An unknown-but-named target falls + // through to the unknown-tool handler below, which lists what exists. + if (viaBridge && (!requestedToolName || (tool && tool.annotations.readOnlyHint !== true))) { + const bridgeError = toToolError( + codedError( + 'VALIDATION_ERROR', + requestedToolName + ? `${requestedToolName} is not a read-only tool, so gnubok_call_tool will not invoke it. Call ${requestedToolName} directly by name.` + : 'gnubok_call_tool requires a "tool" argument naming the read-only tool to invoke.', + ), + { toolName: 'gnubok_call_tool' }, + ) + emitToolCallTelemetry({ + tool: 'gnubok_call_tool', + requiredScope: null, + actor, + latencyMs: 0, + success: false, + isError: true, + errorCode: bridgeError.error.code, + errorKind: 'bridge_refused', + errorMessage: bridgeError.error.message_sv, + requestId: id ?? null, + userId, + companyId, + }) + return NextResponse.json( + jsonRpc(id ?? null, decorate({ + content: [ + { + type: 'text', + text: JSON.stringify(projectMcpPayload(bridgeError, toolNamespace), null, 2), + }, + ], + isError: true, + })) + ) + } if (!tool) { emitToolCallTelemetry({ tool: toolName ?? '', diff --git a/lib/events/types.ts b/lib/events/types.ts index 5d5b3efc..4945a5a5 100644 --- a/lib/events/types.ts +++ b/lib/events/types.ts @@ -242,7 +242,8 @@ export type CoreEvent = success: boolean // true iff the tool returned without throwing AND was invoked (not denied) isError: boolean // matches the JSON-RPC tool-result isError flag returned to the client errorCode: string | null // structured error code from tool-result.toToolError when applicable - errorKind: 'execution' | 'scope_denied' | 'capability_denied' | 'company_access_denied' | 'unknown_tool' | 'test_key_write_blocked' | null + errorKind: 'execution' | 'scope_denied' | 'capability_denied' | 'company_access_denied' | 'unknown_tool' | 'test_key_write_blocked' | 'bridge_refused' | null + // bridge_refused: gnubok_call_tool was pointed at a write tool, or at nothing. errorMessage: string | null // human-readable error message (truncated to 500 chars), null on success. // Raw material for clustering real agent failures into curated gotchas: // errorCode alone can't distinguish "period locked" from "unbalanced".