feat(kilo-mcp): publish all non-admin/debug mutations

REVIEW.md

@@ -9,6 +9,7 @@
 - Logic bugs (wrong conditions, off-by-one)
 - Typos that cause runtime errors
 - Breaking API changes
+- MCP write exposure: a mutation newly published to the Kilo MCP catalog (`services/kilo-mcp/catalog.json`, generated from `apps/web/src/scripts/mcp-catalog/`) that issues or rotates credentials or tokens, reads or writes secrets, changes organization membership or authorization, deploys, charges or purchases, or deletes data - flag the path unless the PR explains why an MCP agent may call it
 
 **Skip these:**
 

apps/web/src/scripts/mcp-catalog/catalog.test.ts

@@ -14,43 +14,38 @@ import { rootRouter } from '@/routers/root-router';
 import {
   CATALOG_JSON_DISPLAY_PATH,
   CATALOG_JSON_PATH,
-  DENYLISTED_TOP_LEVEL_SEGMENTS,
   SUMMARY_INSTRUCTION,
   buildCatalogJson,
   buildCatalogRows,
   collectCatalogLeaves,
   generateMissingSummaries,
+  isDenylistedPath,
   parseKiloCompletion,
   readCommittedSummaries,
   runKiloCompletion,
   type CatalogLeaf,
 } from './catalog';
-import { MCP_MUTATION_ALLOWLIST } from './mutations';
 
-const queryLeaf = (path: string, firstInput?: CatalogLeaf['firstInput']): CatalogLeaf => ({
+const queryLeaf = (path: string, inputs: CatalogLeaf['inputs'] = []): CatalogLeaf => ({
   path,
   type: 'query',
-  firstInput,
+  inputs,
 });
 
-const mutationLeaf = (path: string, firstInput?: CatalogLeaf['firstInput']): CatalogLeaf => ({
+const mutationLeaf = (path: string, inputs: CatalogLeaf['inputs'] = []): CatalogLeaf => ({
   path,
   type: 'mutation',
-  firstInput,
+  inputs,
 });
 
-/** The allowlist as the real rootRouter exposes it: every listed path exists. */
-const allowlistedMutationLeaves = (): CatalogLeaf[] =>
-  MCP_MUTATION_ALLOWLIST.map(path => mutationLeaf(path));
-
 /**
- * A synthetic leaf set that satisfies the rot guard: the fixture's own leaves
- * plus every allowlisted mutation. The guard now compares the allowlist even
- * when no mutation leaf is enumerated, so a query-only fixture would throw.
+ * A fixture leaf set that satisfies the mutation drift guard: the fixture's own
+ * leaves plus one non-denylisted mutation. The extra leaf carries no summary,
+ * so it lands in `missing` and never disturbs a `rows` assertion.
  */
-const withAllowlistedMutations = (leaves: CatalogLeaf[]): CatalogLeaf[] => [
+const withMutation = (leaves: CatalogLeaf[]): CatalogLeaf[] => [
   ...leaves,
-  ...allowlistedMutationLeaves(),
+  mutationLeaf('user.updateProfile'),
 ];
 
 describe('mcp-catalog catalog', () => {
@@ -72,60 +67,83 @@ describe('mcp-catalog catalog', () => {
   });
 
   describe('buildCatalogRows', () => {
-    it('keeps every query and only allowlisted mutations, dropping the denylist and subscriptions', () => {
+    it('keeps every non-denylisted query and mutation, dropping internal paths and subscriptions', () => {
       const { rows, missing } = buildCatalogRows(
         [
           queryLeaf('admin.users.list'),
           queryLeaf('debug.ping'),
           queryLeaf('test.echo'),
+          queryLeaf('organizations.admin.list'),
           mutationLeaf('admin.users.delete'),
+          mutationLeaf('organizations.admin.grantCredit'),
+          mutationLeaf('codingPlans.adminMarkRevocationFailed'),
+          mutationLeaf('slack.devRemoveDbRowOnly'),
           mutationLeaf('organizations.subscription.cancel'),
           mutationLeaf('user.deleteAccount'),
-          { path: 'user.onEvent', type: 'subscription', firstInput: undefined },
+          { path: 'user.onEvent', type: 'subscription', inputs: [] },
           queryLeaf('user.getProfile'),
-          ...allowlistedMutationLeaves(),
+          mutationLeaf('user.updateProfile'),
+          queryLeaf('slack.testConnection'),
         ],
         new Map([
           ['user.getProfile', 'Returns the profile of a user.'],
-          ['agentProfiles.create', 'Create an agent profile.'],
           ['organizations.subscription.cancel', 'Cancel the subscription.'],
           ['user.deleteAccount', 'Delete the account.'],
+          ['user.updateProfile', 'Update the profile.'],
+          ['slack.testConnection', 'Test a Slack connection.'],
+          ['admin.users.delete', 'Delete a user (internal).'],
+          ['organizations.admin.grantCredit', 'Grant credit (internal).'],
+          ['codingPlans.adminMarkRevocationFailed', 'Mark revocation failed (internal).'],
+          ['slack.devRemoveDbRowOnly', 'Remove a DB row (dev only).'],
         ])
       );
-      // Only the query and the allowlisted mutation with a summary are rows.
-      expect(rows.map(row => row.path)).toEqual(['user.getProfile', 'agentProfiles.create']);
-      expect(rows[0]?.kind).toBe('query');
-      expect(rows[1]?.kind).toBe('mutation');
-      // Non-allowlisted mutations are dropped even with a committed summary;
-      // allowlisted mutations without a summary wait for the LLM pass.
-      expect(missing.map(leaf => leaf.path)).toEqual(
-        MCP_MUTATION_ALLOWLIST.filter(path => path !== 'agentProfiles.create')
-      );
+      // Every non-internal query and mutation with a summary is a row; a
+      // denylisted segment anywhere in the path and subscriptions are dropped,
+      // summary or not. `testConnection` stays because `test` is exact-only.
+      expect(rows.map(row => row.path)).toEqual([
+        'organizations.subscription.cancel',
+        'user.deleteAccount',
+        'user.getProfile',
+        'user.updateProfile',
+        'slack.testConnection',
+      ]);
+      expect(rows.map(row => row.kind)).toEqual([
+        'mutation',
+        'mutation',
+        'query',
+        'mutation',
+        'query',
+      ]);
+      expect(missing).toEqual([]);
     });
 
-    it('locks the denylist constant to the internal-only segments', () => {
-      expect([...DENYLISTED_TOP_LEVEL_SEGMENTS].sort()).toEqual(['admin', 'debug', 'test']);
+    it('treats an internal segment anywhere in the path as internal', () => {
+      expect(isDenylistedPath('admin.users.list')).toBe(true);
+      expect(isDenylistedPath('organizations.admin.grantCredit')).toBe(true);
+      expect(isDenylistedPath('codingPlans.adminInsights')).toBe(true);
+      expect(isDenylistedPath('debug.ping')).toBe(true);
+      expect(isDenylistedPath('test.echo')).toBe(true);
+      expect(isDenylistedPath('slack.devRemoveDbRowOnly')).toBe(true);
+      expect(isDenylistedPath('user.getProfile')).toBe(false);
+      expect(isDenylistedPath('slack.testConnection')).toBe(false);
     });
 
     it('fails on a zero-row catalog instead of emitting an empty one', () => {
       expect(() => buildCatalogRows([queryLeaf('admin.nothing')])).toThrow(/zero catalog rows/);
       expect(() => buildCatalogRows([])).toThrow(/zero catalog rows/);
     });
 
-    it('throws naming the allowlisted paths that no longer match a mutation leaf', () => {
-      // The router still exposes mutations, so a missing allowlisted path is a
-      // rename or deletion: the dump must fail loudly instead of dropping it.
-      expect(() =>
-        buildCatalogRows([
-          queryLeaf('user.getProfile'),
-          mutationLeaf('agentProfiles.createRenamed'),
-        ])
-      ).toThrow(/agentProfiles\.create\b/);
+    it('throws when the enumeration exposes no mutation', () => {
+      // A query-only enumeration (a wholesale router drift) must not publish a
+      // mutation-free catalog in silence: write support would vanish.
+      expect(() => buildCatalogRows([queryLeaf('user.getProfile')])).toThrow(
+        /zero mutation procedures/
+      );
     });
 
     it('shapes rows with derived tags, input schema and search blob', () => {
       const { rows } = buildCatalogRows(
-        withAllowlistedMutations([queryLeaf('user.getProfile', z.object({ userId: z.string() }))]),
+        withMutation([queryLeaf('user.getProfile', [z.object({ userId: z.string() })])]),
         new Map([['user.getProfile', 'Returns the profile of a user.']])
       );
       const row = rows[0];
@@ -145,16 +163,64 @@ describe('mcp-catalog catalog', () => {
 
     it('emits an empty input schema for procedures without input', () => {
       const { rows } = buildCatalogRows(
-        withAllowlistedMutations([queryLeaf('user.listSessions')]),
+        withMutation([queryLeaf('user.listSessions')]),
         new Map([['user.listSessions', 'Lists active sessions.']])
       );
       expect(rows[0]?.inputSchema).toEqual({});
     });
 
+    it('composes every chained .input() schema into one object schema', () => {
+      const { rows } = buildCatalogRows(
+        withMutation([
+          mutationLeaf('workspaceFolders.create', [
+            z.object({ organizationId: z.uuid().nullable() }),
+            z.object({ name: z.string(), color: z.string() }),
+          ]),
+        ]),
+        new Map([['workspaceFolders.create', 'Creates a workspace folder.']])
+      );
+      const schema = rows[0]?.inputSchema;
+      expect(schema).toMatchObject({
+        type: 'object',
+        properties: {
+          organizationId: expect.any(Object),
+          name: { type: 'string' },
+          color: { type: 'string' },
+        },
+      });
+      expect(schema?.['required']).toEqual(
+        expect.arrayContaining(['organizationId', 'name', 'color'])
+      );
+    });
+
+    it('keeps every constraint when two chained schemas declare the same key', () => {
+      const { rows } = buildCatalogRows(
+        withMutation([
+          mutationLeaf('user.rename', [z.object({ id: z.string() }), z.object({ id: z.uuid() })]),
+        ]),
+        new Map([['user.rename', 'Renames a user.']])
+      );
+      const properties = rows[0]?.inputSchema['properties'] as Record<string, unknown>;
+      expect(properties['id']).toEqual({ allOf: [{ type: 'string' }, expect.any(Object)] });
+    });
+
+    it('keeps the tags of every chained input schema field', () => {
+      const { rows } = buildCatalogRows(
+        withMutation([
+          mutationLeaf('workspaceFolders.create', [
+            z.object({ organizationId: z.uuid().nullable() }),
+            z.object({ name: z.string(), color: z.string() }),
+          ]),
+        ]),
+        new Map([['workspaceFolders.create', 'Creates a workspace folder.']])
+      );
+      expect(rows[0]?.tags).toEqual(expect.arrayContaining(['organizationid', 'name', 'color']));
+    });
+
     it('keeps committed summaries byte-for-byte, including whitespace', () => {
       const summary = '  Returns the user profile.  ';
       const { rows } = buildCatalogRows(
-        withAllowlistedMutations([queryLeaf('user.getProfile')]),
+        withMutation([queryLeaf('user.getProfile')]),
         new Map([['user.getProfile', summary]])
       );
       expect(rows[0]?.summary).toBe(summary);
@@ -171,7 +237,7 @@ describe('mcp-catalog catalog', () => {
   describe('buildCatalogJson', () => {
     it('sorts rows by path and ends with a trailing newline', () => {
       const { rows } = buildCatalogRows(
-        withAllowlistedMutations([queryLeaf('b.b'), queryLeaf('a.a')]),
+        withMutation([queryLeaf('b.b'), queryLeaf('a.a')]),
         new Map([
           ['a.a', 'A'],
           ['b.b', 'B'],
@@ -260,16 +326,35 @@ describe('mcp-catalog catalog', () => {
       expect(buildCatalogJson(rows)).toBe(onDisk);
     });
 
-    it('publishes every allowlisted mutation with kind mutation and a searchable blob', () => {
+    it('publishes every non-denylisted mutation with kind mutation and a searchable blob', () => {
       const catalog = JSON.parse(readFileSync(CATALOG_JSON_PATH, 'utf8')) as Record<
         string,
         { kind?: string; searchBlob?: string } | undefined
       >;
-      for (const path of MCP_MUTATION_ALLOWLIST) {
+      const expected = collectCatalogLeaves(rootRouter)
+        .filter(leaf => leaf.type === 'mutation' && !isDenylistedPath(leaf.path))
+        .map(leaf => leaf.path);
+      expect(expected.length).toBeGreaterThan(0);
+      for (const path of expected) {
         expect(catalog[path]?.kind).toBe('mutation');
         expect(catalog[path]?.searchBlob).toContain(path);
       }
     });
+
+    it('publishes every required field of a chained-input mutation', () => {
+      // workspaceFolders.create chains a second .input() requiring name/color;
+      // a first-input-only catalog advertises organizationId alone and lets an
+      // MCP request pass local validation before failing upstream.
+      const catalog = JSON.parse(readFileSync(CATALOG_JSON_PATH, 'utf8')) as Record<
+        string,
+        { inputSchema?: Record<string, unknown> } | undefined
+      >;
+      const schema = catalog['workspaceFolders.create']?.inputSchema;
+      const required = Array.isArray(schema?.['required']) ? (schema['required'] as string[]) : [];
+      const properties = Object.keys((schema?.['properties'] as object) ?? {});
+      expect(required).toEqual(expect.arrayContaining(['organizationId', 'name', 'color']));
+      expect(properties).toEqual(expect.arrayContaining(['organizationId', 'name', 'color']));
+    });
   });
 
   describe('generateMissingSummaries', () => {