Aller au contenu
login
arrow_backRetour aux issues
MakazhanAlpamys/Soup #483

MCP refuses execution tools with 'Execution tools are not implemented in this version' — they shipped in v0.73.3 (#297)

ecoDébutant bug help wanted good first issue

descriptionDescription

`soup mcp serve` without `--allow-execute` refuses `train_execute` / `export_execute` with a message that ends: ``` 'train_execute' can execute commands and is disabled; restart with 'soup mcp serve --allow-execute' to enable. Execution tools are not implemented in this version. ``` `src/soup_cli/mcp_server/registry.py:1026-1029` (on `main`, today). The last sentence is false. Execution **is** implemented — #297 shipped it in **v0.73.3** (`src/soup_cli/mcp_server/execution.py`, the `train_execute` / `export_execute` handlers, the server-generated confirmation token). The string predates that release and was never updated when the feature landed. ## Why this is worth fixing rather than shrugging at The first half of the message tells an operator to restart with `--allow-execute`. The second half tells them it would not help. An operator who reads to the end concludes the flag is a stub and stops — so the message actively discourages using a feature that works. It is also the *only* thing many users will ever read about execution being available. I found it while reviewing #479 (network transports), where the **same** stale assumption had produced a second, worse instance: the startup banner printed `execution disabled` while `--allow-execute` was being handed to a network listener. That one is fixed in #479. This one is on `main` and is separate, so I am filing it rather than widening that PR. ## What needs to change 1. Drop the false sentence in `_refuse_execute` (`registry.py:1021-1031`). The first half is correct and should stay: name the tool, say it is disabled, name the flag that enables it. 2. Check for siblings. `_mutating_specs`' docstring and `build_registry`'s docstring (`registry.py:1117-1125`) still say `allow_execute` "is retained separately for **future** execution tools" — same vintage, same wrong tense. ## Acceptance criteria - [ ] No user-facing string or docstring in `mcp_server/` claims execution is unimplemented or future - [ ] A test asserts the refusal message names the tool and `--allow-execute`, and does **not** contain "not implemented" — behavioural, i.e. it calls the handler and inspects `McpToolError` - [ ] The test fails if the sentence is restored (please verify by putting it back and running, not by inspection) - [ ] `grep -rn "not implemented" src/soup_cli/mcp_server/` returns nothing stale ## A note on how to test this `build_registry` **always lists** `train_execute` / `export_execute` and swaps only the handler, so any test that compares tool *names* passes in both gate states and guards nothing. That is exactly how the #479 defect stayed invisible. Call the handler and assert on the raised `McpToolError`. Small, self-contained, no hardware needed. Good first issue.
codeOuvre sur GitHub