Repository navigation
feat(mcp): let an agent call external tools over MCP - #90
Merged
Merged
Conversation
Designs letting an agent call external tools over MCP (FR-2.18, FR-2.19), which no module owned yet. Two source documents contradicted each other on the shape: module-diagram.md described tool-server integrations as configured once for the app and shared across agents, while ERD.md made them per-agent. ADR-0017 settles it as per-agent. The shared store was credible because one URL and token can serve several agents and rotation happens once. It loses on the operator side: it makes them manage a connection *and* which agents use it, needs a screen outside the agent page, and silently grants every attached agent the credential. The accepted cost is re-entering a URL and token per agent, which stays cheap only if operators run few agents. That is an assumption, not a measurement -- the seed creates one agent and one agent serves many embeds, neither of which settles operator scale -- so ADR-0017 marks it as such rather than as a constraint. Later extraction of a shared store is additive but not free: envelopes are bound to their row by associated data, so a migration must decrypt and re-encrypt. Both non-deprecated transports are supported, Streamable HTTP by URL and stdio by command. HTTP+SSE is excluded as deprecated, so a server offering only that endpoint will not connect. Auth is per server: none, static headers, or OAuth 2.1 for HTTP, and the environment map for stdio, which the specification names as the credential mechanism there. Findings that shaped the design and would otherwise have to be rediscovered, each verified against the package's types or the built image: - streamText defaults to stopWhen isStepCount(1), so a tool call would execute, the loop would stop, and the model would never see the result. Every answer would ignore the data while appearing to work. A step bound is mandatory, which also means a daily message limit no longer bounds model calls. - The client's transport config accepts type 'sse' | 'http', where 'http' is Streamable HTTP. There is no separate streamable-http value. - The stdio transport is documented as Node-only and spawns through cross-spawn, but connects and lists tools under Bun. Co-Authored-By: space-bunny-free <noreply@opencode.ai>
MCP servers are documented with npx, which the runtime image lacks. Copy-pasting an upstream install command therefore failed with a command-not-found an operator cannot act on. A symlink does not work. bun dispatches on argv[0], so npx pointing at bunx invokes the runtime instead of the package runner and rejects bunx flags such as -y. Verified that `npx -y <pkg>` falls back to bun's runtime mode and errors on the flag. This wrapper execs bun's x mode instead. Verified in the built image that both `npx <pkg>` and `npx -y <pkg>` resolve and run. Co-Authored-By: space-bunny-free <noreply@opencode.ai>
Gives an agent access to live business data, so it can answer stock levels, orders, and prices instead of only from uploaded documents. Satisfies FR-2.18 and FR-2.19. Design in docs/specs/2026-10-05-mcp-tools-design.md, cardinality in ADR-0017. One new module owns mcp_server, its probe, credential handling, the OAuth provider, and stdio process lifecycle. conversation -> mcp -> agent, and ai-provider learns nothing about MCP: it takes an opaque tools record and a tool-identity map and passes both to streamText. Three findings shaped the design: - streamText defaults to stopWhen isStepCount(1), so a tool call would execute, the loop would stop, and the model would never see the result. Every answer would ignore the data while appearing to work. A step bound is mandatory, and it means a daily message limit no longer bounds model calls. - The client's transport config accepts type 'sse' | 'http', where 'http' IS Streamable HTTP. There is no separate streamable-http value to pass, and stdio is an instance rather than a config object because it is not expressible in that union. - The SDK's parseChatEvent allowlists every event type, so emitting tool events without changing it would throw into session recovery on the first one. Both new variants are handled explicitly. Deliberately not shipped: a shared server store (ADR-0017), the deprecated HTTP+SSE transport, resources and prompts, and persistence of tool arguments or results. Chat never fails because of MCP -- an unreachable, unrunnable, or failing server is dropped for that generation and logged. The vault moved to src/lib/credential-vault.ts and is keyed per module, so an mcp envelope cannot be decrypted by ai-provider and a header value cannot be read as an environment value. Demo servers for the dev stack and tests live in test-fixtures, speaking the wire protocol directly so the repository keeps one MCP stack. The stdio tests prove the child process is reaped on close and that APP_SECRET and DATABASE_URL are invisible to operator-chosen code; the isolation test fails if that regresses. Copy says bunx, which the runtime image already provides, rather than npx, which it does not. The npx wrapper added in the previous commit is reverted here: naming bunx keeps the image untouched, and bunx -y works, so the install form from upstream MCP documentation still applies. Co-Authored-By: space-bunny-free <noreply@opencode.ai>
Rebasing onto main brought f3fb442, which moved authorization out of services and into each contract: a route spreads one access.permission entry that sets both its OpenAPI security and its enforcement, and requireAccessPolicy refuses any matched route without one. mcp still checked permissions inside the service, so every route would have answered 500 rather than doing its job. Each contract now declares agents:read for the two reads and agents:manage for the rest, the service takes no userId, and the PermissionDeniedError mapping is gone. The access test moves with it: a read-only member is now built by granting agents:read and then asserting 403 through the routes, which is where enforcement lives. Co-Authored-By: space-bunny-free <noreply@opencode.ai>
Six reports from using the tab, one of which explains the other two.
The dialog's mutations closed themselves and never invalidated the list, so the
card kept a stale revision. The next edit or tool toggle then sent that stale
revision and was rejected as a conflict, which is why selecting tools appeared
broken and why a newly created connection only appeared after a manual reload.
One missing invalidation, three symptoms.
Beyond that, from using it:
- Multiple environment variables. One pair per connection was wrong for almost
every real server, so it is a list now, like arguments.
- "Test connection" is gone. Whether a connection works is whether it offered
tools, so health is derived from the tool list rather than stored, which
removes a route, two columns, a service method, and a problem code. A
connection with no tools says so on the card instead.
- The transport and health badges were noise. The card shows a green or red
dot, the transport is in the summary line, and arguments moved onto the tool
count row.
- Editing reuses the add form through one optional prop rather than a second
form. A secret is only sent when both halves are filled, so leaving the value
blank keeps what is stored; sending the name alone would have overwritten it
with an empty string.
- Seeds are named stdio and http, and the stdio command is bare `bun`. An
absolute path from one machine is a broken seed on every other one. The http
seed no longer depends on an env var, so both are always present.
Two fixes outside the module, both to shared components:
- Select triggers showed the raw value ("stdio") instead of the option's label
("Local program"). Base UI resolves that label only from the `items` prop, and
items inside a closed popup never register, so the wrapper now derives the
label map from its own children.
- Added a shadcn Checkbox, since there was none and the tool list needed one.
Base UI emits `data-checked`, not `data-state`, so the primary-colour variants
key off `data-[checked]`; the state-based spelling compiles fine and never
applies.
Co-Authored-By: space-bunny-free <noreply@opencode.ai>
The seeded http connection is not expected to work, which is useful: it shows the failed card alongside a working one, with nothing extra to install. So the compose service that made it work is gone, and with it the only caller of the fixture's HTTP mode. That left startDemoHttpServer exported and unused, and a "stdio" argument on every invocation that only fed a mode switch. Both are gone, along with handle's export and a price field no tool ever returned. The fixture is now a stdio server and nothing else: 113 lines, no exports, no arguments. It still speaks the wire protocol directly, so the repository keeps one MCP stack. Co-Authored-By: space-bunny-free <noreply@opencode.ai>
Cancel was wired to a bare `close`. Nothing in that file declares one, so it resolved to the global lib.dom declares, which is `window.close`: the VS Code webview tore itself down and Opera ignored it because it only closes windows scripts opened. Both now call setOpen(false), which is what they always meant. The cause was mine. A local `const close` existed until a later edit replaced the surrounding block and dropped it, leaving both call sites pointing at the global. TypeScript said nothing, because lib.dom declares `function close(): void`. Two other things found while in there: - The transport options read "Web address" and "Local program". Neither name came from the operator; they were mine, and they hid the value the operator has to type elsewhere. Both now read stdio and http, through t() like every other string. - en.json had "unreachable": "mcp.unreachable", so the failed-connection message rendered as its own key. i18n:fix filled cs and zh but left en, because it takes the source locale as given. The edit path also reported nothing when a save was rejected. One error state now covers draft problems and failed saves, so both paths say what went wrong. Co-Authored-By: space-bunny-free <noreply@opencode.ai>
The hand-written checkbox carried Radix-style data-[checked:] classes that Tailwind never generates for Base UI's boolean attribute, so checked boxes rendered hollow. It is now the base-nova registry source verbatim, byte-identical modulo the repo's React import convention. The select label wrapper is deleted with it: select.tsx is back to byte-identical with main, and the one call site needing mapped labels passes explicit items. Verified by rendered trigger text, not reasoning. Also unblocks the shadcn CLI in this package, which refused every command: tsconfig paths now resolve the components.json aliases (no baseUrl, which TypeScript 7 removed). And no-restricted-globals bans bare window globals after a bare `close` resolved to window.close and closed the whole browser. Co-Authored-By: space-bunny-free <noreply@opencode.ai>
Seeds read as real connections: Shop inventory (working) and Warehouse API (deliberately unreachable, so both card states are visible with no setup). The footer is one quiet row: the switch alone, pencil and trash icon buttons. The redundant "Connection enabled" text and the competing pills are gone; edit/remove/edit-enabled strings survive as aria-labels. Removal stays guarded by the typed-name confirmation. Co-Authored-By: space-bunny-free <noreply@opencode.ai>
Removes the OAuth sign-in flow, which could never complete: the SDK only persists state when the provider already returns one, so the state in the authorization URL was never stored and the callback always failed. The UI offered it with no authorize button. Out go the provider, both routes, the columns (single squashed 0009), the enum value, and the problem code, with the OpenAPI spec and both generated clients regenerated in the same change. Stored secrets can now be revoked: updates carry explicit deleteHeaders / deleteEnv name lists, omission still preserves, and switching auth mode away still clears the column. The toggle path always sends the complete tool list, so an explicit empty selection now deselects everything instead of falling through to the stored flags, and a changed argument list invalidates the snapshot like a changed command does. Envelopes sealed before the vault moved keep opening: the old AAD carried version 1, which sorts last, so decrypt retries with it before failing. Without this every stored AI credential would read as unusable. Tool runtime is bounded: results are cut at 8k characters of text with a marker, listings time out at 15s with page and tool caps, and a namespaced collision degrades one server instead of silently winning. A tool-result carrying isError now reports failed, and parallel calls to the same tool are tracked by call id so an abort closes each one. Co-Authored-By: space-bunny-free <noreply@opencode.ai>
The dialog computes removals from the stored names: a cleared header name deletes it, a renamed header forgets the old name, a removed environment row deletes that variable, and untouched masked values are still omitted. A value typed without a name is rejected instead of silently dropped. The auth field is labeled Authentication with None and Secret key as the only options. The widget no longer names raw tools to shoppers while one runs, and the SDK keys active tools by server and tool so two connections offering the same tool no longer collapse into one entry. Docs lose the sign-in paragraph, gain one sentence on what a local command runs as, and the corrupted zh string is repaired. Co-Authored-By: space-bunny-free <noreply@opencode.ai>
The update route dropped the tool list on the floor, so checkbox clicks reached the service without it and nothing ever changed. The route now passes tools through, with a route-level test that would have caught it. Adding a connection enables everything it offers; mergeTools keeps the operator's flags for known tools and defaults gained tools to on. The argument and environment editors were type-then-Add, which silently dropped anything never added. Rows are now added empty and typed in place: blank rows are ignored, repeats collapse, a value without a name and (on create) a name without a value are refused instead of silently dropped. The add-time duplicate, empty, and limit errors are gone with the helpers that produced them; the Add button simply disables at the limit. Net -90 lines across the two editors, their helpers, and the body. Toggles are optimistic: the checkbox flips before the round trip, a failure rolls it back through the cached list, and settling refetches. The delete dialog no longer asks for the typed name; the dialog itself is the confirmation. Co-Authored-By: space-bunny-free <noreply@opencode.ai>
The SDK's default stream onError console.errors the full APICallError, prompt included, so a transient Google 500 dumps user messages into the log on top of the graceful SSE error event. Pass a no-op onError and redact provider errors at the global boundary; finalization failures now emit an error event instead of rejecting the background task. Co-Authored-By: muse-spark <noreply@opencode.ai>
-y never varies and every tutorial spells it out; the package name is the argument operators mistype, and without it bunx has nothing to run. The placeholder now shows that shape instead of another flag. Co-Authored-By: space-bunny-free <noreply@opencode.ai>
The five-step bound is what lets a tool result reach the model, and nothing pinned it: isStepCount returns a closure, so the test captures the stopWhen handed to streamText through a now-injectable seam and asserts it stops at five steps, not before. streamText joins vault, generate, discover, and the repository as an explicit dependency rather than a module mock. A bare JSON-RPC stub over POST covers http discovery against a working server; the client also opens a GET event stream, which the stub answers 405 since it has no server-initiated messages. Co-Authored-By: space-bunny-free <noreply@opencode.ai>
Headers become rows like env vars: one secret section, one diff, one validation path, and the dialog reuses the same row editor. The update body has a single builder with the toggle as a thin wrapper, the card splits into list, card, and dialog, and tool toggles use the switch already on the card, so the registry checkbox goes away. Constants are plain numbers, the SDK drops its no-op dedup copy, and the comments say less while keeping every why that constrains the design. The spec marks OAuth as removed rather than describing it as shipped. Co-Authored-By: space-bunny-free <noreply@opencode.ai>
…ode/add-mcp-support
The spec no longer describes the removed authorization flow; the Auth block keeps the one line stating what shipped. The dialog's tools omission is asserted on the edit body itself instead of a describe block for a trivial assembler. Co-Authored-By: space-bunny-free <noreply@opencode.ai>
Three journeys through the real app and API: an unreachable http connection is added, edited, and removed; a stdio connection discovers tools and the toggle survives reload; a stored header survives a blank edit. Cards carry a test id so assertions scope past the seeded cards. The unit file keeps only what the browser cannot reach cheaply: the validation matrix, the secret diffing combinations, and the exact body shapes. Journeys replace the three behavior tests. Co-Authored-By: space-bunny-free <noreply@opencode.ai>
Knip flags the exported builder: both callers live in the same module, so nothing outside needs it. Co-Authored-By: space-bunny-free <noreply@opencode.ai>
Fidasek009
marked this pull request as ready for review
October 7, 2026 15:18
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧑🍳 Code Review: MCP Support PR
Two findings. Nothing that blocks shipping, but both worth fixing.
1. Select props: pick a lane (connection-dialog.tsx:176)
The auth mode <Select> has both an items Record and explicit <SelectItem> children. The transport <Select> two lines up does it right — just children. Having both means one is dead code (and a future maintainer will waste time figuring out which one wins).
2. Stale closure on draft (connection-dialog.tsx:68)
useState(() => draftFor(server)) snapshots server once on first render. The component is only used in add mode right now, so this is dormant — but the first person who adds edit support and forgets a <Dialog key={server.id}> will have a very confusing bug where the form shows old data.
No security issues found. Permission checks were moved from the service layer to contract-level middleware (access.permission(...)), which is the right place for them.
Rest of the code is clean. The toolCallId fix for parallel MCP calls and the associatedData AAD migration path both show real attention to edge cases.
Contributor
Code Review Roast 🔥Verdict: No Issues Found | Recommendation: Merge Oh wait, this incremental change is actually clean. I had my flamethrower warmed up and everything. The Three locale files updated, both generated clients regenerated, integration test updated to match the new error type. Even the 📊 Overall: Like a bug report that includes the fix, the tests, and a screenshot — you don't see this every day. Files Reviewed (12 files in this increment)
Previous Review Summaries (5 snapshots, latest commit 42a8eb7)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 42a8eb7)Verdict: No Issues Found | Recommendation: Merge Oh wait, this incremental change is actually clean. I had my flamethrower warmed up and everything. The I almost flagged the missing defense-in-depth comment, but the commit message explains the rationale clearly, and a server that loops through infinite cursors can only harm its own agent — same as any broken server. 📊 Overall: Like removing training wheels on a bike you've been riding for years — sensible, and the rider knows what they're doing. Files Reviewed (2 files in this increment)
Previous review (commit 4398283)Verdict: No Issues Found | Recommendation: Merge Oh wait, this incremental change is actually clean. I had my flamethrower warmed up and everything. The rename from What I really appreciate: the comment about renaming to 📊 Overall: Like a refund that actually shows up — rare, but when it happens, you remember why you bother. Files Reviewed (14 files in this increment)
Previous review (commit 8d24d69)Verdict: No Issues Found | Recommendation: Merge Oh wait, this incremental change is actually clean. I had my flamethrower warmed up and everything. ASCII validation on both sides, matching tests, all three locale files updated, update path covered — you even remembered 📊 Overall: Like finding a parking spot right in front — I don't trust it, but I'll take it. Files Reviewed (8 files in this increment)
Previous review (commit 11ee562)Verdict: 1 Carried Forward | Recommendation: Address before merge What changedIncremental review of commits
What's still standing
The author replied that 🏆 Best part: The 💀 Worst part: The 📊 Overall: Like a refactor that left one dead branch on the tree. Prune it. Files Reviewed (10 files in this increment)
Fix these issues in Kilo Cloud Previous review (commit 2c2dd8e)🧑🍳 Code Review: MCP Support PRTwo findings. Nothing that blocks shipping, but both worth fixing. 1.
|
Fidasek009
commented
Oct 7, 2026
Fidasek009
left a comment
Fidasek009
left a comment
Contributor
Author
There was a problem hiding this comment.
I just looked at the backend code and I think it could be cleaner,
The enable/disable endpoint is now the only writer of the flag: isDisabled leaves the update schema, the service input, and the dialog bodies, which preserve it by omission. The vault is a module const since APP_SECRET is required at import, and the edit dialog is keyed on its server so a future prop change resets the draft. Docs describe header rows instead of the old single pair. Co-Authored-By: space-bunny-free <noreply@opencode.ai>
The name becomes part of provider tool names, whose grammars accept printable ASCII only, so Czech or CJK input would silently become underscores. The service rejects it and the dialog explains it without the word ASCII: letters without accents, numbers, spaces, and symbols. Co-Authored-By: space-bunny-free <noreply@opencode.ai>
The UI is a switch, not a checkbox, and enabled is the standard term: selected becomes enabled across the snapshot type, contract, service, client, dialog, card, seeds, tests, and docs. The key lives in jsonb on an unmerged PR, so no migration change is needed; pre-rename rows heal through the default-on merge. Listings no longer stop at 200 tools. A large server is the operator's responsibility, and the cap hid tools the UI could have shown. The page cap against a hostile cursor stays, as do timeouts. Co-Authored-By: space-bunny-free <noreply@opencode.ai>
A page cap is a count cap in disguise, with the page size chosen by the server. The remaining guards are the per-page timeout, user cancel, and the generation timeout; a looping server can only harm its own agent, and the operator disables it the same way as any broken server. Co-Authored-By: space-bunny-free <noreply@opencode.ai>
Fildass1
reviewed
Oct 7, 2026
Name failures (empty, too long, non-ASCII) threw InvalidMcpServerError, so API consumers saw invalid-mcp-server-url and the dialog fell through to the generic save error. They now throw InvalidMcpServerNameError, mapped to invalid-mcp-server-name, with a dialog catalog entry and a client-side length check mirroring the server limit. Co-Authored-By: space-bunny-free <noreply@opencode.ai>
Text parts shared an 8k budget but image/audio blobs passed through untouched into model context. Every part now counts against the budget by serialized size; over-budget non-text parts are dropped and the marker is appended at most once instead of once per trailing part. Co-Authored-By: space-bunny-free <noreply@opencode.ai>
Fildass1
approved these changes
Oct 8, 2026
Fildass1
left a comment
Fildass1
left a comment
Contributor
There was a problem hiding this comment.
I have spend too much time trying to find mistakes...
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Gives an agent access to live business data over MCP, so it can answer stock levels, orders and prices instead of only from uploaded documents. Satisfies FR-2.18 and FR-2.19. Design in
docs/specs/2026-10-05-mcp-tools-design.md, cardinality in ADR-0017.A new
mcpmodule ownsmcp_server, tool discovery, credentials, and stdio process lifecycle.conversation -> mcp -> agent;ai-providerlearns nothing about MCP and takes an opaque tools record plus a step bound.Deliberately not shipped: a shared server store, the deprecated HTTP+SSE transport, resources and prompts, sign-in flows (the OAuth attempt never completed a round trip, so it was removed rather than left half-working), and persistence of tool arguments or results. Chat never fails because of MCP.
Findings worth flagging, each verified against the package rather than assumed:
streamTextdefaults tostopWhen isStepCount(1), so a tool call would execute, the loop would stop, and the model would never see the result. Every answer would ignore the data while appearing to work. The five-step bound is pinned by a test.type 'sse' | 'http', where'http'is Streamable HTTP. There is nostreamable-httpvalue, and stdio is an instance rather than a config object.parseChatEventallowlists every event type, so emitting tool events without changing it throws into session recovery on the first one.Review fixes since: stored secrets are revocable through explicit deletion lists (omission still preserves); envelopes sealed before the vault move still open through a legacy-AAD fallback; tool results are bounded and listings time out; every tool a new connection offers starts enabled.
Shared-component notes: per-tool toggles reuse the Switch already on the card, so the registry checkbox is gone; the select is stock, with explicit items at the one call site needing mapped labels.
How to test
bun run devseeds two connections on the seed agent under the agent's Tools tab.bun test-fixtures/mcp-demo-server.tsand offersget_stock_level,list_orders,report_environment, all enabled. Toggle a tool (it flips instantly and rolls back on failure), disable and re-enable, edit the name, add an argument, a variable, or a header through the add-first rows, and remove it in the plain confirm dialog.Covered by tests: the five-step bound, http discovery against a JSON-RPC stub, tool toggles through the update route, and secret deletion at both levels, plus browser journeys for add/edit/toggle/delete and the secret round-trip. The unit file keeps only the validation matrix, the secret diffing combinations, and exact body shapes.
Not verified visually — the reviewer's browser and my own were both unusable during development, so the dashboard work is covered by types and unit tests only.
Checklist
Prepared with opencode (space-bunny-free).