full mcp implementation - #4408
Conversation
wavehassman
left a comment
wavehassman
left a comment
There was a problem hiding this comment.
Since the caller here is a model rather than our UI, the inputs aren't really controlled. We could get 01.1.0, 1.1, sometimes a WBS that doesn't exist. Right now wbsNum is an unconstrained z.string(). I think adding a regex to the tool schema would help return a proper validation error
z.string().regex(/^\d+\.\d+\.\d+$/, 'WBS must look like 1.1.0')
This would fix the validation issues I flagged in the other pr
| }), | ||
| annotations: { readOnlyHint: true } | ||
| }, | ||
| async ({ startDate, endDate }) => |
There was a problem hiding this comment.
could the model send something like "next Monday" Wouldn't an invalid date slip past both range checks since NaN comparisons are false, and then it would be a 500 that says 'report this to the software team'
|
|
||
| // The MCP endpoint, authenticated with the same per-user API tokens. It must accept POST (JSON-RPC), | ||
| // so it is mounted separately from the read only /agent router; every tool it exposes is a read. | ||
| app.all('/mcp', requireApiToken, attachAuthInfo, (req, res) => void mcpNodeHandler(req, res, req.body)); |
There was a problem hiding this comment.
maybe i'm confused but this doesn't seem to have a readOnly guard, only a requireApiToken guard. If someone later registers a tool that writes, could any API token reach it?
There was a problem hiding this comment.
The guard is not on the auth, its just a byproduct of the fact that api token auth can only access the /mcp and /agent routes, which only have readonly calls. Eventually we will want to add write abilities, and so this scoping control is the best way to separate
|
|
||
| // The MCP endpoint, authenticated with the same per-user API tokens. It must accept POST (JSON-RPC), | ||
| // so it is mounted separately from the read only /agent router; every tool it exposes is a read. | ||
| app.all('/mcp', requireApiToken, attachAuthInfo, (req, res) => void mcpNodeHandler(req, res, req.body)); |
There was a problem hiding this comment.
shouldn't this have an onerror handler?
| * no progress or logging notifications, and it keeps the endpoint easy to test with curl. | ||
| */ | ||
| const mcpHandler = createMcpHandler(({ authInfo }) => buildMcpServer(authInfo?.extra as unknown as AgentContext), { | ||
| responseMode: 'json' |
There was a problem hiding this comment.
shouldn't we check that authInfo is there and give a good error if not
There was a problem hiding this comment.
This is not user input so that wouldn't really be helpful
| * Serializes a tool result, truncating long lists. | ||
| * @param value the value to return to the model | ||
| */ | ||
| export const toolJson = (value: unknown): CallToolResult => { |
There was a problem hiding this comment.
is the MAX_ITEMS cap still doing anything here? looks like it only truncates when the value is an array, but getProjects returns { carNumber, projects } now, so finishline_list_projects might not get capped at all
There was a problem hiding this comment.
changed to pagination
210f21c to
c389b03
Compare
c389b03 to
6ee5c41
Compare
- constrain wbsNum and event dates in the tool schemas so the model gets a validation error instead of a 500 on input like "1.2" or "next Monday" - guard against unparseable dates in getEvents, which NaN comparisons let past both range checks - page finishline_list_projects and finishline_get_tasks, the two lists that can realistically outgrow a response, rather than silently truncating results - register every tool through registerReadOnlyTool so /mcp stays read only by construction now that it cannot use the readOnlyGuard - forward mcp handler rejections to the error handler instead of dropping them - fail with a clear error when the request never resolved to a caller
6ee5c41 to
2e42bba
Compare
2 participants
Changes
The actual mcp implementation necessary for agents to connect
Notes
Screenshots
Closes #4404