fix(agents): close gaps from /gstack-review

P1 — thread reads now enforce ownership
========================================
chat.service.ts / routes/agent-chat.ts
  GET /api/v1/threads/:id/messages was previously RBAC-mapped to
  view:agents (no resourceName scope) with the route comment promising
  "service-level owner check enforces fine-grained access" — but the
  service didn't actually check. Any caller with view:agents could read
  another user's thread by guessing/learning the threadId. CUIDs are
  hard to brute-force but they leak: SSE `final` chunks, agents-plugin
  `_meta.threadId`, and several response bodies surface them. Now
  ChatService.listMessages(threadId, ownerId) loads the thread, returns
  404 (not 403, to avoid id-enumeration via differential status codes)
  if ownerId doesn't match. Regression test in chat-service.test.ts
  covers Alice/Bob isolation + nonexistent-thread same-shape 404.

P2 — AgentChatRequestSchema strict mode
========================================
validation/agent.schema.ts
  `.merge()` does NOT inherit `.strict()` from AgentChatParamsSchema.
  Typo'd fields (e.g. `temprature`) silently fell through and the agent
  silently used the default — debuggable only by reading the LLM call
  payload. Re-applied `.strict()` on the merged schema.

P2 — per-agent maxIterations override + clamp
==============================================
chat.service.ts
  Loop cap was a hard-coded module constant (12), wrong for both
  research-style agents (need higher) and cheap-probe agents (could opt
  lower). Now reads `agent.extras.maxIterations`, clamps 1..50, falls
  back to 12 default. The clamp is the soft-DoS guard: a hostile agent
  definition with `maxIterations:1000000` can't burn unbounded LLM calls
  per request. Both chat() and chatStream() use ctx.maxIterations now.
  Regression test covers low-cap override (rejects with `exceeded 2`)
  and hostile-value clamp (rejects with `exceeded 50`).

P3 — SSE write to closed socket
================================
routes/agent-chat.ts
  When the upstream adapter throws after some chunks were already
  written AND the client disconnected, the catch block tried to flush
  more chunks to a closed socket. Without an `on('error')` handler
  Node emits unhandled error events; once Pino is wired to alerts
  this'd page on every disconnect-mid-stream. writeSseChunk now
  checks `reply.raw.destroyed || writableEnded` before write.

P3 — BACKEND_TOKEN_DEAD preserves original stack
=================================================
services/secret-backend-rotator.service.ts
  When wrapping mintRoleToken/lookupSelf failures as
  BACKEND_TOKEN_DEAD, the new Error() discarded the original throw —
  hard to tell whether the inner failure was a network blip vs an
  OpenBao API mismatch vs DNS. Now uses `new Error(msg, { cause: err })`
  so the inner stack survives.

P3 — .gitignore .claude/scheduled_tasks.lock
=============================================
This persisted state file was leaking into every `git status`.

Tests
=====
mcpd 761/761 (+2 regression tests). mcplocal 715/715. cli 430/430.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Michal
2026-04-25 23:53:19 +01:00
parent 2e266e318a
commit 1f0be8a5c1
6 changed files with 143 additions and 8 deletions

View File

@@ -410,4 +410,91 @@ describe('ChatService', () => {
expect(ctx.body.tools).toHaveLength(1);
expect(ctx.body.tools?.[0]?.function.name).toBe(`s1${TOOL_NAME_SEPARATOR}a`);
});
// Regression: per-agent maxIterations override + clamp.
// Found by /gstack-review on 2026-04-25.
// Without the clamp, a hostile agent definition with `extras.maxIterations:1000000`
// could spin the loop into a near-infinite tool-call burn.
it('per-agent extras.maxIterations clamps below default and refuses absurd values', async () => {
const chatRepo = mockChatRepo();
const tools = mockTools({
listTools: vi.fn(async () => [{
name: `g${TOOL_NAME_SEPARATOR}t`, description: '', parameters: {},
}]),
callTool: vi.fn(async () => ({})),
});
// Agent with maxIterations=2 — only 2 tool-call rounds allowed before bail.
const agentsLowCap = {
getByName: vi.fn(async () => ({
id: 'agent-low', name: 'low', description: '', systemPrompt: '',
llm: { id: 'llm-1', name: 'qwen3-thinking' },
project: { id: 'proj-1', name: 'mcpctl-dev' },
proxyModelName: null, defaultParams: {},
extras: { maxIterations: 2 },
ownerId: 'owner-1', version: 1, createdAt: NOW, updatedAt: NOW,
})),
} as unknown as AgentService;
const adapter = scriptedAdapter([toolCall(`g${TOOL_NAME_SEPARATOR}t`, {})]);
const svc = new ChatService(
agentsLowCap, mockLlms(), adapterRegistry(adapter),
chatRepo, mockPromptRepo(), tools,
);
await expect(svc.chat({
agentName: 'low', userMessage: 'spin', ownerId: 'owner-1',
})).rejects.toThrow(/exceeded 2 iterations/);
// Hostile agent with maxIterations=1000000 — must clamp to 50, not iterate forever.
const agentsHostile = {
getByName: vi.fn(async () => ({
id: 'agent-bad', name: 'bad', description: '', systemPrompt: '',
llm: { id: 'llm-1', name: 'qwen3-thinking' },
project: { id: 'proj-1', name: 'mcpctl-dev' },
proxyModelName: null, defaultParams: {},
extras: { maxIterations: 1_000_000 },
ownerId: 'owner-1', version: 1, createdAt: NOW, updatedAt: NOW,
})),
} as unknown as AgentService;
const adapter2 = scriptedAdapter([toolCall(`g${TOOL_NAME_SEPARATOR}t`, {})]);
const chatRepo2 = mockChatRepo();
const svc2 = new ChatService(
agentsHostile, mockLlms(), adapterRegistry(adapter2),
chatRepo2, mockPromptRepo(), tools,
);
await expect(svc2.chat({
agentName: 'bad', userMessage: 'spin', ownerId: 'owner-1',
})).rejects.toThrow(/exceeded 50 iterations/);
});
// Regression: thread message reads must enforce ownership.
// Found by /gstack-review on 2026-04-25.
// Without this, any caller with `view:agents` could read another user's thread
// by guessing/learning the threadId (CUIDs leak through SSE chunks + tool _meta).
it('listMessages refuses a thread owned by another user (404, not 403, to avoid id-enumeration)', async () => {
const chatRepo = mockChatRepo();
// Pre-seed a thread owned by 'alice'
await chatRepo.createThread({ agentId: 'agent-x', ownerId: 'alice' });
const aliceThread = chatRepo._threads[0]!;
await chatRepo.appendMessage({
threadId: aliceThread.id,
role: 'user',
content: 'private to alice',
});
const svc = new ChatService(
mockAgents(), mockLlms(), adapterRegistry(scriptedAdapter([chatCompletion('ok')])),
chatRepo, mockPromptRepo(), mockTools(),
);
// Bob requests Alice's thread by id — must 404.
await expect(svc.listMessages(aliceThread.id, 'bob'))
.rejects.toThrow(/not found/i);
// Alice gets her own messages.
const aliceMessages = await svc.listMessages(aliceThread.id, 'alice');
expect(aliceMessages.map((m) => m.content)).toEqual(['private to alice']);
// Genuinely missing thread — same 404 shape (no oracle leak).
await expect(svc.listMessages('cnonexistent000000000000000', 'alice'))
.rejects.toThrow(/not found/i);
});
});