Skip to content

Commit 537ce1c

Browse files
beyondnetPeruclaude
beyondnetPeru
and
claude
committed
fix(mcp): make the MCP tool surface usable — ABAC/OPA, tool classification, multi-session
Analyzed + fixed three defects that made every MCP tools/call fail. - fs-extra ESM interop: `await import('fs-extra')` exposes members only on `.default` under nodenext, so `fs.stat` was undefined → "OPA Engine Error: fs.stat is not a function" denied nearly every tool. abac-evaluator now uses node:fs/promises; mcp-server.service uses `(mod.default ?? mod)` for fs-extra + yaml. No other unsafe dynamic CJS imports remain. - ABAC classification completeness: the native evaluator used name-substring heuristics, leaving evaluate/drift/topology-recommend/phase-artifacts-evaluate (and others) unclassified → ABAC-03 "Unknown tool" → denied. Added an explicit TOOL_CLASSIFICATION map for all 35 registered tools (read/write/deploy) with a guard test asserting registration↔classification never drift. - Per-session StreamableHTTP transport: a single startup transport rejected any second initialize with 400 "Server already initialized". Now a Map<sessionId,{transport,server}>: mint per initialize, route by mcp-session-id, clean up on close; multiple concurrent clients work. - Also fixed jest.config moduleNameMapper for @modelcontextprotocol/sdk (path one level too shallow) which had silently broken 4 suites. Build clean; 191/193 unit tests (2 pre-existing gate.tools env failures unrelated). Verified live (http, dev): two concurrent initialize sessions; previously-denied tools (topology-list, config-get, drift, topology-recommend) now pass ABAC. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent c06f0d9 commit 537ce1c

5 files changed

Lines changed: 310 additions & 42 deletions

File tree

‎src/packages/mcp-server/jest.config.js‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -28,13 +28,13 @@ module.exports = {
2828
"^@evolith/core/(.*)$":
2929
"<rootDir>/../../../node_modules/@evolith/core-domain/dist/$1",
3030
"^@modelcontextprotocol/sdk/types\\.js$":
31-
"<rootDir>/../../../node_modules/@modelcontextprotocol/sdk/dist/cjs/types.js",
31+
"<rootDir>/../../../../node_modules/@modelcontextprotocol/sdk/dist/cjs/types.js",
3232
"^@modelcontextprotocol/sdk/server/index\\.js$":
33-
"<rootDir>/../../../node_modules/@modelcontextprotocol/sdk/dist/cjs/server/index.js",
33+
"<rootDir>/../../../../node_modules/@modelcontextprotocol/sdk/dist/cjs/server/index.js",
3434
"^@modelcontextprotocol/sdk/server/stdio\\.js$":
35-
"<rootDir>/../../../node_modules/@modelcontextprotocol/sdk/dist/cjs/server/stdio.js",
35+
"<rootDir>/../../../../node_modules/@modelcontextprotocol/sdk/dist/cjs/server/stdio.js",
3636
"^@modelcontextprotocol/sdk/server/streamableHttp\\.js$":
37-
"<rootDir>/../../../node_modules/@modelcontextprotocol/sdk/dist/cjs/server/streamableHttp.js",
37+
"<rootDir>/../../../../node_modules/@modelcontextprotocol/sdk/dist/cjs/server/streamableHttp.js",
3838
"^@nestjs/cache-manager$":
3939
"<rootDir>/__mocks__/@nestjs/cache-manager.ts",
4040
"^cache-manager$":

‎src/packages/mcp-server/src/mcp/abac-classification-coverage.spec.ts‎

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
1+
import { Test } from '@nestjs/testing';
2+
import { AppModule } from '../app.module';
3+
import { ToolRegistryService } from './tool-registry.service';
4+
import { AbacEvaluator } from './abac-evaluator';
5+
6+
/**
7+
* Guard test: the ABAC classification and the registered tool surface must never
8+
* drift. Every tool the DI graph registers has to be classifiable by the native
9+
* ABAC evaluator, otherwise it would be denied with ABAC-03 ("Unknown tool ...
10+
* not in any known classification") before OPA ever runs — the exact defect this
11+
* suite exists to prevent from regressing.
12+
*/
13+
describe('ABAC classification coverage (full DI graph)', () => {
14+
let registry: ToolRegistryService;
15+
const abac = new AbacEvaluator();
16+
17+
beforeAll(async () => {
18+
const moduleRef = await Test.createTestingModule({ imports: [AppModule] }).compile();
19+
registry = moduleRef.get(ToolRegistryService);
20+
});
21+
22+
it('classifies EVERY registered tool (no ABAC-03 for a real tool)', () => {
23+
const registeredNames = registry.listSchemas().map((s) => s.name);
24+
expect(registeredNames.length).toBeGreaterThan(0);
25+
26+
const unclassified = registeredNames.filter((name) => abac.classifyTool(name) === undefined);
27+
expect(unclassified).toEqual([]);
28+
});
29+
30+
it('every registered tool has an EXPLICIT classification entry (not just a heuristic match)', () => {
31+
const registeredNames = registry.listSchemas().map((s) => s.name);
32+
const explicitlyClassified = new Set(AbacEvaluator.classifiedToolNames());
33+
34+
const missingExplicit = registeredNames.filter((name) => !explicitlyClassified.has(name));
35+
expect(missingExplicit).toEqual([]);
36+
});
37+
38+
it('native evaluation never returns ABAC-03 for a registered tool', () => {
39+
const registeredNames = registry.listSchemas().map((s) => s.name);
40+
for (const name of registeredNames) {
41+
const decision = abac.evaluateNative({
42+
user: { id: 'u1', roles: ['architect'], tenant: 't1' },
43+
tool_name: name,
44+
resource_domain: 'mcp-server',
45+
environment: 'development',
46+
});
47+
const abac03 = decision.violations.find((v) => v.id === 'ABAC-03');
48+
expect(abac03).toBeUndefined();
49+
}
50+
});
51+
});

‎src/packages/mcp-server/src/mcp/abac-evaluator.cache.spec.ts‎

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,13 @@
11
const loadPolicyMock = jest.fn();
22
const readFileMock = jest.fn();
3-
const pathExistsMock = jest.fn();
43
const statMock = jest.fn();
54

65
jest.mock('@open-policy-agent/opa-wasm', () => ({
76
loadPolicy: (...args: unknown[]) => loadPolicyMock(...args),
87
}));
9-
jest.mock('fs-extra', () => ({
10-
pathExists: (...args: unknown[]) => pathExistsMock(...args),
8+
// AbacEvaluator.evaluateOpa reads the compiled policy via node:fs/promises
9+
// (fs-extra's members are undefined under the runtime's ESM dynamic import).
10+
jest.mock('node:fs/promises', () => ({
1111
readFile: (...args: unknown[]) => readFileMock(...args),
1212
stat: (...args: unknown[]) => statMock(...args),
1313
}));
@@ -25,9 +25,7 @@ describe('AbacEvaluator OPA policy cache (GT-348)', () => {
2525
beforeEach(() => {
2626
loadPolicyMock.mockReset();
2727
readFileMock.mockReset();
28-
pathExistsMock.mockReset();
2928
statMock.mockReset();
30-
pathExistsMock.mockResolvedValue(true);
3129
readFileMock.mockResolvedValue(Buffer.from('wasm-bytes'));
3230
statMock.mockResolvedValue({ mtimeMs: 100 });
3331
loadPolicyMock.mockResolvedValue({ evaluate: () => [{ result: [] }] });

‎src/packages/mcp-server/src/mcp/abac-evaluator.ts‎

Lines changed: 118 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -22,13 +22,91 @@ const DEVELOPER_ROLES = new Set(['developer', 'qa']);
2222
const OPERATOR_ROLES = new Set(['operator', 'sre']);
2323
const ARCHITECT_ROLES = new Set(['architect', 'admin']);
2424

25+
/** Verb class an ABAC decision is scoped against. */
26+
export type ToolClass = 'read' | 'write' | 'deploy';
27+
28+
/**
29+
* Explicit ABAC classification for EVERY registered MCP tool.
30+
*
31+
* This is the source of truth the native evaluator consults BEFORE falling back
32+
* to name heuristics. The previous heuristic-only approach silently denied any
33+
* tool whose name lacked a magic substring (read/list/get/write/…), which left
34+
* governance-critical tools like `evolith-evaluate`, `evolith-drift-detect`,
35+
* `evolith-topology-recommend` and `evolith-phase-artifacts-evaluate`
36+
* unclassified → ABAC-03 → denied before OPA even ran.
37+
*
38+
* Keep this map in lockstep with the registered tool surface
39+
* (`tools.module.ts` / `tools-registration`). A guard test
40+
* (`abac-classification-coverage.spec.ts`) fails the build if a registered tool
41+
* has no entry here, so drift cannot silently reintroduce ABAC-03 denials.
42+
*
43+
* Verb rationale:
44+
* - read → non-mutating: catalogs, status, evaluations, metrics, reports.
45+
* - write → mutates repo/config/agents/satellites or advances phase/handoff.
46+
* - deploy → release/merge/deploy-class operations (none registered today, but
47+
* reserved for the deploy tools the native rules gate in production).
48+
*/
49+
const TOOL_CLASSIFICATION: Record<string, ToolClass> = {
50+
// Core validation / evaluation (read: analysis, no mutation)
51+
'evolith-validate': 'read',
52+
'evolith-evaluate': 'read',
53+
'evolith-composable-validate': 'read',
54+
'evolith-architecture-validate': 'read',
55+
'evolith-drift-detect': 'read',
56+
'evolith-gate-evaluate': 'read',
57+
'evolith-phase-artifacts-evaluate': 'read',
58+
59+
// Topology catalog / advisory (read)
60+
'evolith-topology-list': 'read',
61+
'evolith-topology-get': 'read',
62+
'evolith-topology-recommend': 'read',
63+
64+
// MoSCoW (create/update/remove/load mutate the backing file; the rest read)
65+
'evolith-moscow-create': 'write',
66+
'evolith-moscow-load': 'read',
67+
'evolith-moscow-update': 'write',
68+
'evolith-moscow-remove': 'write',
69+
'evolith-moscow-list': 'read',
70+
'evolith-moscow-validate': 'read',
71+
'evolith-moscow-report': 'read',
72+
73+
// SDLC (status reads; handoff advances/mutates)
74+
'evolith-sdlc-status': 'read',
75+
'evolith-sdlc-handoff': 'write',
76+
77+
// Metrics (read)
78+
'evolith-dora-metrics': 'read',
79+
'evolith-metrics': 'read',
80+
81+
// Config (get reads; set mutates)
82+
'evolith-config-get': 'read',
83+
'evolith-config-set': 'write',
84+
85+
// Auto-fix / phase advance (mutating)
86+
'evolith-auto-fix': 'write',
87+
'evolith-phase-advance': 'write',
88+
89+
// Agents (list/validate read; install/upgrade/remove/run mutate)
90+
'evolith-agent-list': 'read',
91+
'evolith-agent-validate': 'read',
92+
'evolith-agent-install': 'write',
93+
'evolith-agent-upgrade': 'write',
94+
'evolith-agent-remove': 'write',
95+
'evolith-agent-run': 'write',
96+
97+
// Satellites (list/status read; create/adopt mutate)
98+
'evolith-satellite-list': 'read',
99+
'evolith-satellite-status': 'read',
100+
'evolith-satellite-create': 'write',
101+
'evolith-satellite-adopt': 'write',
102+
};
103+
25104
const READ_TOOLS = new Set([
26105
'evolith-ping',
27106
'evolith-echo',
28107
'evolith-read-gap-tracking',
29108
'evolith-read-file',
30109
'evolith-list-dir',
31-
'evolith-gate-evaluate',
32110
'evolith-gate-status',
33111
'read-tool',
34112
]);
@@ -52,6 +130,34 @@ export class AbacEvaluator {
52130
// change. AbacEvaluator is a singleton, so this persists across dispatches.
53131
private readonly policyCache = new Map<string, { mtimeMs: number; policy: any }>();
54132

133+
/** All tool names with an explicit ABAC classification (guard-test surface). */
134+
static classifiedToolNames(): string[] {
135+
return Object.keys(TOOL_CLASSIFICATION);
136+
}
137+
138+
/**
139+
* Resolve a tool's verb class. Consults the explicit {@link TOOL_CLASSIFICATION}
140+
* map first (authoritative for the registered surface), then falls back to the
141+
* legacy static sets and name heuristics for out-of-band names (e.g. test
142+
* fixtures, ping/echo). Returns `undefined` when the tool cannot be classified
143+
* at all → ABAC-03.
144+
*/
145+
classifyTool(tool_name: string): ToolClass | undefined {
146+
const explicit = TOOL_CLASSIFICATION[tool_name];
147+
if (explicit) return explicit;
148+
149+
if (DEPLOY_TOOLS.has(tool_name) || tool_name.includes('deploy') || tool_name.includes('publish') || tool_name.includes('merge')) {
150+
return 'deploy';
151+
}
152+
if (WRITE_TOOLS.has(tool_name) || tool_name.includes('write') || tool_name.includes('replace') || tool_name.includes('run') || tool_name.includes('fix') || tool_name.includes('advance')) {
153+
return 'write';
154+
}
155+
if (READ_TOOLS.has(tool_name) || !tool_name.startsWith('evolith-') || tool_name.includes('read') || tool_name.includes('list') || tool_name.includes('get')) {
156+
return 'read';
157+
}
158+
return undefined;
159+
}
160+
55161
evaluateNative(input: AbacInput): AbacDecision {
56162
const { user, tool_name, environment } = input;
57163
const roles = user?.roles || [];
@@ -65,13 +171,13 @@ export class AbacEvaluator {
65171
};
66172
}
67173

68-
// ABAC-03: Unknown tool
69-
const isRead = READ_TOOLS.has(tool_name) || !tool_name.startsWith('evolith-') || tool_name.includes('read') || tool_name.includes('list') || tool_name.includes('get');
70-
const isWrite = WRITE_TOOLS.has(tool_name) || tool_name.includes('write') || tool_name.includes('replace') || tool_name.includes('run') || tool_name.includes('fix') || tool_name.includes('advance');
71-
const isDeploy = DEPLOY_TOOLS.has(tool_name) || tool_name.includes('deploy') || tool_name.includes('publish') || tool_name.includes('merge');
174+
// ABAC-03: Unknown tool. Explicit classification wins; heuristics are fallback.
175+
const toolClass = this.classifyTool(tool_name);
176+
const isRead = toolClass === 'read';
177+
const isWrite = toolClass === 'write';
178+
const isDeploy = toolClass === 'deploy';
72179

73-
const classified = isRead || isWrite || isDeploy;
74-
if (!classified) {
180+
if (!toolClass) {
75181
return {
76182
allowed: false,
77183
violations: [{ id: 'ABAC-03', message: 'Unknown tool requested; not in any known classification' }],
@@ -128,7 +234,11 @@ export class AbacEvaluator {
128234

129235
async evaluateOpa(input: AbacInput, corePath: string): Promise<AbacDecision> {
130236
try {
131-
const fs = await import('fs-extra');
237+
// Use node:fs/promises (not fs-extra): under the runtime's ESM build
238+
// (module: nodenext) `await import('fs-extra')` returns a namespace whose
239+
// CJS members live on `.default`, so `fs.stat` would be undefined and throw
240+
// "fs.stat is not a function", crashing the OPA policy load for every call.
241+
const fs = await import('node:fs/promises');
132242
const path = await import('node:path');
133243
// @ts-ignore: opa-wasm lacks types
134244
const { loadPolicy } = await import('@open-policy-agent/opa-wasm');

0 commit comments

Comments
 (0)