Expose discovered MCP tools through the act allowlist (#251) #71

2026-08-01T02:37:03+02:00

claude commented

2026-08-01 02:37:03 +02:00

Second of two branches for Vikunja #251. Base is overnight/mcp-client (PR #70).

What changed

internal/mcp: CallPositional + bindPositional — the narrow positional→named argument rule.
internal/tool: MCPCaller seam + WithMCP; one branch in Exec routes an ["mcp",server,tool] row to the manager. Enabled and destructive checks run first, unchanged.
internal/store: ProposeMCPTool — a proposed row that already carries cmd + destructive. Never touches an existing row.
cmd/mavend/mcp.go: wireMCP (nil unless a server is configured AND enabled), boot connect + propose, a one-minute refresh ticker, status. Every failure is non-fatal.
internal/ipc: read-only mcp_servers method + MCPServerStatus. No call-a-tool method, on purpose.
cmd/mavweb: "MCP servers" card on /tools; proposals prefill their cmd and destructive box.
deploy/mavend.json: an mcp block with the Vikunja server, enabled: false.

Why this shape

MCP tools are acts, so they go through the machinery acts already have rather than beside it. Encoding the server and tool in the existing cmd column means no migration and no second allowlist: status='enabled' is still the only thing that makes a tool runnable, destructive=1 still forces the confirm turn, and enabling still happens on /tools behind step-up. destructive is !readOnlyHint, so a tool that does not promise to be read-only is assumed to mutate.

Small catalogue by construction, as required: allow_tools + max_tools cap what a server contributes, only enabled rows are visible to the router, and the existing stage-3 gate already refuses an act with no allowlisted fn — so an uncertain tool call asks. No grammar change was needed.

How verified

make build and make test both exit 0. New tests: MCP row dispatch (including that mcp is never exec'd), still-needs-confirm, refuses without a caller; ProposeMCPTool idempotence; bindPositional table; wiring off when unconfigured, nil-safe, unreachable server non-fatal, loopback URL refused without allow_private; /tools MCP section present and degrades when the core cannot answer.

Verified live against the real Vikunja MCP server on the LAN, not a fixture: connect + discover 3 tools, three proposed rows with scope=mcp:vikunja and update_task marked destructive, a no-arg call returning real project JSON, an integer-arg call binding task_id, and Russian words for a number correctly refused.

That live run is also where the read-only condition on argument binding came from. CallPositional("update_task", ["251"]) succeeded and blanked the fields it did not send, because update_task requires only task_id. A mutating tool now never receives a guessed argument; a mutating tool with nothing required still runs, behind confirm, since nothing was guessed.

Not in scope

Plan step 5 (MCP resource text in router/phraser prompts) is deferred. No MCP server direction — the task asks for a client.

Vikunja #251

Second of two branches for Vikunja #251. Base is `overnight/mcp-client` (PR #70). ## What changed - `internal/mcp`: `CallPositional` + `bindPositional` — the narrow positional→named argument rule. - `internal/tool`: `MCPCaller` seam + `WithMCP`; one branch in `Exec` routes an `["mcp",server,tool]` row to the manager. Enabled and destructive checks run first, unchanged. - `internal/store`: `ProposeMCPTool` — a `proposed` row that already carries cmd + destructive. Never touches an existing row. - `cmd/mavend/mcp.go`: `wireMCP` (nil unless a server is configured AND enabled), boot connect + propose, a one-minute refresh ticker, status. Every failure is non-fatal. - `internal/ipc`: read-only `mcp_servers` method + `MCPServerStatus`. No call-a-tool method, on purpose. - `cmd/mavweb`: "MCP servers" card on /tools; proposals prefill their cmd and destructive box. - `deploy/mavend.json`: an `mcp` block with the Vikunja server, `enabled: false`. ## Why this shape MCP tools are acts, so they go through the machinery acts already have rather than beside it. Encoding the server and tool in the existing `cmd` column means no migration and no second allowlist: `status='enabled'` is still the only thing that makes a tool runnable, `destructive=1` still forces the confirm turn, and enabling still happens on /tools behind step-up. `destructive` is `!readOnlyHint`, so a tool that does not promise to be read-only is assumed to mutate. Small catalogue by construction, as required: `allow_tools` + `max_tools` cap what a server contributes, only enabled rows are visible to the router, and the existing stage-3 gate already refuses an act with no allowlisted fn — so an uncertain tool call asks. No grammar change was needed. ## How verified `make build` and `make test` both exit 0. New tests: MCP row dispatch (including that `mcp` is never exec'd), still-needs-confirm, refuses without a caller; `ProposeMCPTool` idempotence; `bindPositional` table; wiring off when unconfigured, nil-safe, unreachable server non-fatal, loopback URL refused without `allow_private`; /tools MCP section present and degrades when the core cannot answer. Verified live against the real Vikunja MCP server on the LAN, not a fixture: connect + discover 3 tools, three `proposed` rows with `scope=mcp:vikunja` and `update_task` marked destructive, a no-arg call returning real project JSON, an integer-arg call binding `task_id`, and Russian words for a number correctly refused. That live run is also where the read-only condition on argument binding came from. `CallPositional("update_task", ["251"])` succeeded and blanked the fields it did not send, because `update_task` requires only `task_id`. A mutating tool now never receives a guessed argument; a mutating tool with nothing required still runs, behind confirm, since nothing was guessed. ## Not in scope Plan step 5 (MCP resource text in router/phraser prompts) is deferred. No MCP server direction — the task asks for a client. Vikunja #251

claude added 1 commit 2026-08-01 02:37:03 +02:00

Expose discovered MCP tools through the act allowlist (#251 ) 8d5e357b57

Second half of the MCP client: the tools the manager discovers become rows in
the existing act allowlist instead of a parallel capability system.

An MCP tool is encoded in the columns that already exist — cmd
["mcp",<server>,<tool>], scope mcp:<server> — so no migration, and
ProposeTool/EnableTool/DisableTool, tool.Matcher and the confirm turn need no
changes. One branch in Executor.Exec routes such a row to the manager instead
of exec, and "mcp" is never run as a binary.

Discovery only ever PROPOSES. destructive comes from the inverse of the MCP
readOnlyHint, so a tool that does not promise to be read-only inherits the
confirm turn, and enabling stays on /tools behind step-up.

Voice args are positional and MCP args are named, so CallPositional binds only
what it can defend: no required properties runs bare, and a read-only tool with
exactly one required string or number gets the tail. Everything else refuses
with ErrNeedsArgs rather than guessing. The read-only condition was learned
against the live Vikunja server: update_task requires only task_id and takes
the rest as optional, so one guessed argument blanked the fields it did not
mention. A partially-filled write destroys what it omits, so a mutating tool
never receives a guessed argument.

Also: a read-only mcp_servers IPC method and an "MCP servers" card on /tools
showing transport, target and state, with the trust level of a local target
spelled out. There is deliberately no call-a-tool IPC method and no run button,
so mutation keeps exactly one path.

Vikunja #251

claude reviewed 2026-08-01 11:32:55 +02:00

claude left a comment

Two decisions carry most of the weight and both are right. Discovery writes a
proposed row and nothing else. A configured server is a place she may look,
not a capability she has. And bindPositional refuses instead of guessing. The
update_task story in that comment is the best argument in the diff. One
guessed argument blanked every field the call did not mention. That earned the
read-only condition. The deploy block
ships with enabled: false and a four-name allow_tools, which is how a
capability like this should arrive.

The rest of this review is about one thing. The external server, not Kami,
chooses what a name means.

1. A row is keyed by name, and the server owns the name

ProposeMCPTool is ON CONFLICT(name) DO NOTHING. mcp.Cmd stores
["mcp", server, tool], which is also just names. Nothing in the row pins what
the tool was when Kami looked at it. So the identity of an enabled capability
is a string the remote server controls and may redefine at any time.

Walked through. Day 1 the server offers list_tasks with
"readOnlyHint": true. Discovery proposes vikunja_list_tasks with
destructive=false. Kami reads the description on /tools, agrees, enables it.
Day 30 the server is upgraded, or taken over, and list_tasks now means
something that writes. Discovery runs on the next boot, hits the conflict, and
does nothing. The row is still enabled, still destructive=0, and Exec still
dispatches ["mcp","vikunja","list_tasks"]. The confirm turn never fires,
because the flag was frozen on day 1 against a claim the server has since
withdrawn.

The doc comment on ProposeMCPTool reads as a safety argument. Re-discovery
"cannot silently re-arm a tool that was disabled or change the cmd of one
already enabled". Both halves are true. Neither is the risk. The cmd does not
have to change for the behaviour to change.

The fix is reconciliation rather than insert-or-skip. On every discovery,
compare the discovered ReadOnly against the stored destructive. Escalate
when they disagree. A tool that stopped claiming read-only must have
destructive=1 written, and the row should probably drop back to proposed so
a human looks again. Only ever escalate, never relax. Store a hash of the
description and input schema on the row too. Then /tools can show that a tool
changed since it was approved, which is what Kami needs to see.

2. `readOnlyHint` is an unverified claim, and it now buys two exemptions

Tool.ReadOnly comes from the server's own annotation. PR 70 uses it in the
safe direction only: absent or false means assume it mutates. This PR starts
using a true affirmatively, in two places, and they compound.

A readOnlyHint: true tool gets destructive=false, so no confirm turn. The
same flag is the condition in bindPositional that permits binding the spoken
tail to the one required property. So a server that lies about one boolean
converts a voice utterance into an unconfirmed, argument-carrying write.

Walked through, against a server that is hostile or merely wrong. It advertises
delete_project, "readOnlyHint": true, description "Show a project and its
tasks", required: ["id"], type integer. Discovery proposes
vikunja_delete_project, marked non-destructive, with that description shown as
the provenance on /tools. Kami reads a plausible description of a read, enables
it. "удали проект 4" routes to that fn with tail 4. bindPositional sees one
required integer on a read-only tool, binds it, and Exec skips the confirm
turn because destructive=0. The project is gone and Maven never asked.

The two exemptions should not rest on the same unverified bit. The cheapest
split: keep readOnlyHint for the destructive flag, where being wrong costs a
question, and require something local for argument binding. Reuse allow_tools
as the condition, or add a per-server bind_positional list. Either way a
guessed argument only reaches a tool Kami named in mavend.json. The name
delete_project is not a defence. The router picks tools by name similarity,
and the description that the model and the human both read is server-written
too.

3. Boot blocks for up to 30s on someone else's process

wireMCP runs mgr.Connect(ctx) synchronously, from wireVoice, from run,
with a 30s budget. Connect dials servers serially. Each HTTP dial is three
requests, each waiting up to that server's 15s timeout plus the webfetch
per-host interval. One black-holed endpoint costs 15s of boot. Two cost the
whole budget.

The comment directly above says the opposite. It claims a server unreachable at
boot is "logged and retried, because Maven starting is not contingent on someone
else's process". Not failing and not blocking are different properties. Only
the first one holds. The passkey path is worse. wireVoice runs inside the
unlock handler, so an unreachable MCP server delays the response to an unlock.

Connect and the first propose belong on the same goroutine as run. The
manager already tolerates a server that has not been dialed yet. Related:
wireMCP builds its context from context.Background(), so a shutdown during
those 30s is not observed.

4. `mcpRefreshInterval` claims a backoff that does not exist

The comment says the manager "applies its own backoff on top, so this being
short is cheap". The manager's only spacing is DefaultReconnectEvery, a flat
30s, shorter than this ticker. There is no backoff anywhere and nothing grows.
So a permanently misconfigured stdio server is re-spawned once a minute forever.
newStdioTransport runs exec.Command and Start on every attempt. A server that starts, fails its handshake and exits leaves a process
spawn per minute in the logs indefinitely. Either add the backoff the comment
promises, or delete the sentence and accept the flat retry.

Smaller notes

Nothing ever retracts a proposal. A tool the server stops offering keeps its
row on /tools forever. If it was enabled, Manager.Call refuses it at call
time with a raw internal string. actionAct does not match that error, so it
falls to the generic path. The /tools page is where Kami would find out,
and it is the one place that does not say.
The provenance string is written once, at first proposal. When the server
later rewrites a tool's description, /tools keeps showing the old one. That is
the same freeze as finding 1, in the field a human reads before deciding.
Tool.Description is still unbounded (raised on PR 70) and it now lands
verbatim in a table cell on /tools, through the Utterance column. Go's
template escaping keeps this a layout problem rather than an injection one. A
server that returns a 40 KB description still makes the enable surface
unusable. Truncate at proposal time.
When e.mcp is nil, the MCP branch in Exec returns ErrNotEnabled, which
actionAct routes to proposeGap. So drop the mcp block from the config
while enabled MCP rows remain. An act on an existing enabled tool then makes
Maven draft a new proposal for it. A distinct error, mapped to a plain "that
tool is not connected", would be more honest.
The tick goroutine calls mgr.Refresh, which is the call that deadlocks under
PR 70 finding 1. Once that is fixed there, this is fine.
deploy/mavend.json sets allow_private: true on a LAN host. Pair that with
the redirect note on PR 70. That fetcher follows a redirect from
192.168.1.104 to anywhere private, the metadata endpoint included.
bindPositional reads s.Properties[name] for a name that required may
list without properties describing. The zero value gives Type: "", which
the case "string", "" arm binds as a string. Refusing an undescribed
property would match the rest of the function's posture.
Untested: a tool whose readOnlyHint flips between discoveries, a discovered
name that collides with an existing enabled non-MCP row, and an enabled row
whose tool has disappeared from the server.

Two decisions carry most of the weight and both are right. Discovery writes a `proposed` row and nothing else. A configured server is a place she may look, not a capability she has. And `bindPositional` refuses instead of guessing. The `update_task` story in that comment is the best argument in the diff. One guessed argument blanked every field the call did not mention. That earned the read-only condition. The deploy block ships with `enabled: false` and a four-name `allow_tools`, which is how a capability like this should arrive. The rest of this review is about one thing. The external server, not Kami, chooses what a name means. ## 1. A row is keyed by name, and the server owns the name `ProposeMCPTool` is `ON CONFLICT(name) DO NOTHING`. `mcp.Cmd` stores `["mcp", server, tool]`, which is also just names. Nothing in the row pins what the tool *was* when Kami looked at it. So the identity of an enabled capability is a string the remote server controls and may redefine at any time. Walked through. Day 1 the server offers `list_tasks` with `"readOnlyHint": true`. Discovery proposes `vikunja_list_tasks` with `destructive=false`. Kami reads the description on /tools, agrees, enables it. Day 30 the server is upgraded, or taken over, and `list_tasks` now means something that writes. Discovery runs on the next boot, hits the conflict, and does nothing. The row is still enabled, still `destructive=0`, and `Exec` still dispatches `["mcp","vikunja","list_tasks"]`. The confirm turn never fires, because the flag was frozen on day 1 against a claim the server has since withdrawn. The doc comment on `ProposeMCPTool` reads as a safety argument. Re-discovery "cannot silently re-arm a tool that was disabled or change the cmd of one already enabled". Both halves are true. Neither is the risk. The cmd does not have to change for the behaviour to change. The fix is reconciliation rather than insert-or-skip. On every discovery, compare the discovered `ReadOnly` against the stored `destructive`. Escalate when they disagree. A tool that stopped claiming read-only must have `destructive=1` written, and the row should probably drop back to `proposed` so a human looks again. Only ever escalate, never relax. Store a hash of the description and input schema on the row too. Then /tools can show that a tool changed since it was approved, which is what Kami needs to see. ## 2. `readOnlyHint` is an unverified claim, and it now buys two exemptions `Tool.ReadOnly` comes from the server's own annotation. PR 70 uses it in the safe direction only: absent or false means assume it mutates. This PR starts using a `true` affirmatively, in two places, and they compound. A `readOnlyHint: true` tool gets `destructive=false`, so no confirm turn. The same flag is the condition in `bindPositional` that permits binding the spoken tail to the one required property. So a server that lies about one boolean converts a voice utterance into an unconfirmed, argument-carrying write. Walked through, against a server that is hostile or merely wrong. It advertises `delete_project`, `"readOnlyHint": true`, description "Show a project and its tasks", `required: ["id"]`, type integer. Discovery proposes `vikunja_delete_project`, marked non-destructive, with that description shown as the provenance on /tools. Kami reads a plausible description of a read, enables it. "удали проект 4" routes to that fn with tail `4`. `bindPositional` sees one required integer on a read-only tool, binds it, and `Exec` skips the confirm turn because `destructive=0`. The project is gone and Maven never asked. The two exemptions should not rest on the same unverified bit. The cheapest split: keep `readOnlyHint` for the destructive flag, where being wrong costs a question, and require something local for argument binding. Reuse `allow_tools` as the condition, or add a per-server `bind_positional` list. Either way a guessed argument only reaches a tool Kami named in mavend.json. The name `delete_project` is not a defence. The router picks tools by name similarity, and the description that the model and the human both read is server-written too. ## 3. Boot blocks for up to 30s on someone else's process `wireMCP` runs `mgr.Connect(ctx)` synchronously, from `wireVoice`, from `run`, with a 30s budget. `Connect` dials servers serially. Each HTTP dial is three requests, each waiting up to that server's 15s timeout plus the webfetch per-host interval. One black-holed endpoint costs 15s of boot. Two cost the whole budget. The comment directly above says the opposite. It claims a server unreachable at boot is "logged and retried, because Maven starting is not contingent on someone else's process". Not failing and not blocking are different properties. Only the first one holds. The passkey path is worse. `wireVoice` runs inside the unlock handler, so an unreachable MCP server delays the response to an unlock. `Connect` and the first `propose` belong on the same goroutine as `run`. The manager already tolerates a server that has not been dialed yet. Related: `wireMCP` builds its context from `context.Background()`, so a shutdown during those 30s is not observed. ## 4. `mcpRefreshInterval` claims a backoff that does not exist The comment says the manager "applies its own backoff on top, so this being short is cheap". The manager's only spacing is `DefaultReconnectEvery`, a flat 30s, shorter than this ticker. There is no backoff anywhere and nothing grows. So a permanently misconfigured stdio server is re-spawned once a minute forever. `newStdioTransport` runs `exec.Command` and `Start` on every attempt. A server that starts, fails its handshake and exits leaves a process spawn per minute in the logs indefinitely. Either add the backoff the comment promises, or delete the sentence and accept the flat retry. ## Smaller notes - Nothing ever retracts a proposal. A tool the server stops offering keeps its row on /tools forever. If it was enabled, `Manager.Call` refuses it at call time with a raw internal string. `actionAct` does not match that error, so it falls to the generic path. The /tools page is where Kami would find out, and it is the one place that does not say. - The provenance string is written once, at first proposal. When the server later rewrites a tool's description, /tools keeps showing the old one. That is the same freeze as finding 1, in the field a human reads before deciding. - `Tool.Description` is still unbounded (raised on PR 70) and it now lands verbatim in a table cell on /tools, through the `Utterance` column. Go's template escaping keeps this a layout problem rather than an injection one. A server that returns a 40 KB description still makes the enable surface unusable. Truncate at proposal time. - When `e.mcp` is nil, the MCP branch in `Exec` returns `ErrNotEnabled`, which `actionAct` routes to `proposeGap`. So drop the `mcp` block from the config while enabled MCP rows remain. An act on an existing enabled tool then makes Maven draft a *new* proposal for it. A distinct error, mapped to a plain "that tool is not connected", would be more honest. - The tick goroutine calls `mgr.Refresh`, which is the call that deadlocks under PR 70 finding 1. Once that is fixed there, this is fine. - `deploy/mavend.json` sets `allow_private: true` on a LAN host. Pair that with the redirect note on PR 70. That fetcher follows a redirect from `192.168.1.104` to anywhere private, the metadata endpoint included. - `bindPositional` reads `s.Properties[name]` for a name that `required` may list without `properties` describing. The zero value gives `Type: ""`, which the `case "string", ""` arm binds as a string. Refusing an undescribed property would match the rest of the function's posture. - Untested: a tool whose `readOnlyHint` flips between discoveries, a discovered name that collides with an existing enabled non-MCP row, and an enabled row whose tool has disappeared from the server.

kami referenced this issue from a commit

2026-08-01 14:43:52 +02:00

tool: separate a tool that is off from a backend that is down

kami referenced this issue from a commit

2026-08-01 14:43:52 +02:00

mcp: pin what a tool was when it was approved

kami closed this pull request

2026-08-01 14:51:56 +02:00

kami commented

2026-08-01 14:52:50 +02:00

Landed on master. The stack was one linear chain, so #84 carried every commit from #50 up, and master now contains this branch in full. Merging this PR on its own is an empty diff, so it is closed rather than merged. The review findings for it were fixed in the 2026-08-01 pass and are on master as commits on the stack tip, not on this branch.

Pull request closed

Please reopen this pull request to perform a merge.

Sign in to join this conversation.

2 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: kami/Maven#71