Complete Plexus management MCP support#567
Conversation
|
| Filename | Overview |
|---|---|
| packages/backend/src/routes/mcp/plexus.ts | Major expansion: all previously stub-only tools now shim to real management routes via fastify.inject. MCP usage logging added. Logic appears correct; type safety sacrificed with Promise returns but practical for this pattern. |
| packages/backend/src/routes/management/system-logs.ts | Adds GET /v0/system/logs/recent endpoint and serializeRecentLog helper. WeakSet circular-reference detection will incorrectly label shared (non-circular) objects as [Circular], producing misleading log output. |
| packages/backend/src/utils/logger.ts | Adds 1,000-entry in-memory ring buffer captured in StreamTransport.log. Buffer eviction uses Array.shift() which is O(n); functional but inefficient at scale. |
| packages/backend/src/routes/management/config.ts | Adds GET/PATCH endpoints for aliases, keys, and mcp-servers that were previously write-only. Also adds GET+PATCH /v0/management/config/mcp-enabled. All new routes follow existing patterns with Zod validation and consistent error handling. |
| packages/backend/src/routes/management/tests/system-logs.test.ts | New test for /v0/system/logs/recent. Re-mocks utils/logger with a pass-through to bypass the global mock, violating AGENTS.md. Also uses a fragile 20ms setTimeout to wait for setImmediate. |
| packages/backend/src/routes/mcp/tests/plexus-mcp-routes.test.ts | Comprehensive expansion: mocks fastify.inject in beforeEach to simulate all management routes, covering all newly implemented tools. Uses registerSpy correctly and tests all major tool operations. |
| packages/frontend/src/pages/Mcp.tsx | Adds Plexus Management MCP enable/disable toggle in two places. Optimistic UI with revert on failure. Clean implementation. |
| packages/frontend/src/lib/api.ts | Adds getMcpEnabled and patchMcpEnabled API methods, following existing patterns exactly. |
| packages/backend/src/routes/mcp/index.ts | Threads mcpUsageStorage through to registerPlexusMcpRoutes so admin MCP requests are now recorded in usage logs. |
Reviews (1): Last reviewed commit: "chore(mcp): limit toggle and simplify MC..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| describe('system log routes', () => { | ||
| let fastify: FastifyInstance; | ||
|
|
There was a problem hiding this comment.
utils/logger
AGENTS.md explicitly states "utils/logger and @earendil-works/pi-ai are globally mocked; do not re-mock them in test files." This file calls vi.mock on utils/logger (with a pass-through to the real module) precisely to bypass the global mock and use the real transport. The stated workaround may be necessary to make the real StreamTransport fire, but it still violates the project rule. Either the global mock configuration should be updated to expose clearRecentLogsForTesting, or the test approach should be aligned with how other logger-touching tests handle this.
Context Used: AGENTS.md (source)
| return { | ||
| name: value.name, | ||
| message: value.message, | ||
| stack: value.stack, | ||
| }; | ||
| } | ||
|
|
||
| if (typeof value === 'object' && value !== null) { | ||
| if (seen.has(value)) { | ||
| return '[Circular]'; | ||
| } | ||
| seen.add(value); | ||
| } | ||
|
|
||
| return value; | ||
| }) | ||
| ); | ||
| } catch { | ||
| const fallback = entry as Record<string, unknown> | null; | ||
| return { | ||
| level: fallback && typeof fallback.level === 'string' ? fallback.level : 'unknown', | ||
| message: | ||
| fallback && typeof fallback.message === 'string' | ||
| ? fallback.message | ||
| : '[unserializable log entry]', |
There was a problem hiding this comment.
WeakSet will flag multiply-referenced objects as [Circular]
The seen set is created once per serializeRecentLog call and never reset between tree nodes. If the same object appears at two different paths in a log entry (a DAG, not a cycle), the second occurrence is replaced with '[Circular]' even though there is no circular reference. This can make log entries misleading or incomplete. A safe alternative is to use structuredClone with a try/catch before falling back to the manual replacer.
| function appendRecentLog(info: any): void { | ||
| recentLogs.push(info); | ||
| if (recentLogs.length > RECENT_LOG_BUFFER_SIZE) { | ||
| recentLogs.shift(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Array.shift() is O(n) for a 1,000-entry buffer
Every log entry written after the buffer is full calls shift(), which moves all 1,000 remaining elements. In a high-throughput environment this will run on every write. A ring-buffer approach eliminates the linear scan. At 1,000 entries the impact is small today, but the buffer size is a named constant that could grow.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
1 participant
Summary