feat(mcp): annotate tools with MCP capability hints - #251
Open
BeamNawapat wants to merge 2 commits into
Open
Conversation
The ten tools in `tools/list` shipped with no `annotations`, so every host had to fall back to its own default. Per the MCP spec that default is `readOnlyHint: false`, which means wigolo was advertising all ten tools as potentially state-changing. Claude Code's plan mode refuses any MCP tool that is not annotated read-only, and it resolves that before consulting permission rules — so an `mcp__wigolo__*` allow rule could not lift it and every call prompted. Cursor, VS Code, Zed, Windsurf and opencode read the same hints for auto-approval, and wigolo ships installers for all of them. Eight tools are read-only. `cache` and `watch` are not, and are annotated accordingly: `cache` accepts `clear`, `watch` creates and deletes jobs. Prompting on those two is the correct outcome, not a regression. Two values are less obvious than they look, both derived from the handlers rather than the tool names: - `cache` is open-world. `check_changes` re-fetches every matching cached URL over the network (src/tools/cache.ts). - `diff` is closed-world. It resolves its `url` sides from the local cache and returns `cache_miss` rather than fetching (src/tools/diff.ts). `idempotentHint` is inert wherever `readOnlyHint` is true, per the spec, so those tools carry `true` for consistency rather than as a claim about output stability. No schema, handler, or API-surface change. The new test pins the full matrix so an eleventh tool added without annotations fails the suite instead of silently regressing permissions in every host.
📝 WalkthroughWalkthroughThe MCP ChangesMCP tool annotations
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/server.ts`:
- Around line 334-344: Update the fetch tool annotations in src/server.ts (lines
334-344) to set readOnlyHint false, destructiveHint true, and idempotentHint
false. Update both corresponding fetch entries in
tests/unit/server/tool-annotations.test.ts (lines 99-109 and 204-212) so the
pinned annotation matrix and notReadOnly assertion expect the
interaction-capable semantics.
In `@tests/unit/server/tool-annotations.test.ts`:
- Around line 112-115: Update the test setup around connectClient to isolate
plugin loading by creating an empty temporary directory and assigning its path
to WIGOLO_PLUGINS_DIR before initSubsystems runs. Ensure this setup applies
before loadPlugins is reached, preventing user-installed plugins from affecting
tools/list or executing during the test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d7878363-d6b3-43c6-b445-7f7d107e1a16
📒 Files selected for processing (5)
README.mddocs/installation.mddocs/troubleshooting.mdsrc/server.tstests/unit/server/tool-annotations.test.ts
6 tasks
Caught in review: `fetch` accepts an `actions` array whose type enum includes `click` and `type` (src/server/tool-schemas.ts), and passing it forces browser rendering. Those run as live Playwright interactions, so a caller can submit a login field or trigger navigation on the target page. That is not read-only. Annotations are static per tool and cannot vary on whether `actions` was passed, so `fetch` has to declare the widest behaviour it can reach — the same rule already applied to `cache` and `watch`. A click on an arbitrary page can destroy remote state, so `destructiveHint` is true. The practical cost is real and worth stating: `fetch` now prompts in Claude Code's plan mode, and it is the most-used tool. The alternatives were to claim read-only semantics a host would act on, or to split `fetch` into read and interactive tools, which is an API break. Documented in troubleshooting.md, with the allow rule for anyone who never passes `actions`. Also pins WIGOLO_PLUGINS_DIR in the test. It already resolved under the redirected WIGOLO_DATA_DIR, but an exported WIGOLO_PLUGINS_DIR in a developer's shell would have let initSubsystems() import real plugin code during the run.
BeamNawapat
force-pushed
the
feat/mcp-tool-annotations
branch
from
July 29, 2026 09:19
6d50117 to
b622b7d
Compare
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/unit/server/tool-annotations.test.ts`:
- Around line 140-156: Update the beforeEach/afterEach setup around
WIGOLO_DATA_DIR and WIGOLO_PLUGINS_DIR to capture their inherited values before
overriding them, then restore those values in afterEach instead of always
deleting the variables. Preserve deletion only when a variable was originally
unset, while keeping the existing resetConfig, cleanup, and mock-reset behavior
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: cfada988-b734-4a80-97cc-733c3c9d9e4a
📒 Files selected for processing (5)
README.mddocs/installation.mddocs/troubleshooting.mdsrc/server.tstests/unit/server/tool-annotations.test.ts
Comment on lines
+140
to
+156
| beforeEach(() => { | ||
| tmpDataDir = mkdtempSync(join(tmpdir(), 'wigolo-tool-annotations-')); | ||
| process.env.WIGOLO_DATA_DIR = tmpDataDir; | ||
| // `pluginsDir` defaults to `<dataDir>/plugins`, so the line above already | ||
| // isolates it — but pin it anyway so an exported WIGOLO_PLUGINS_DIR in the | ||
| // developer's shell can't make `initSubsystems()` import real plugin code. | ||
| process.env.WIGOLO_PLUGINS_DIR = join(tmpDataDir, 'plugins'); | ||
| resetConfig(); | ||
| _resetMigrationGuard(); | ||
| vi.clearAllMocks(); | ||
| }); | ||
| afterEach(() => { | ||
| delete process.env.WIGOLO_DATA_DIR; | ||
| delete process.env.WIGOLO_PLUGINS_DIR; | ||
| resetConfig(); | ||
| try { rmSync(tmpDataDir, { recursive: true, force: true }); } catch { /* ignore */ } | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Restore inherited environment values in teardown.
Lines 152-153 permanently discard pre-existing WIGOLO_DATA_DIR / WIGOLO_PLUGINS_DIR values. Save and restore them so later tests retain their runner configuration.
Proposed fix
describe('tools/list capability annotations', () => {
let tmpDataDir: string;
+ let previousDataDir: string | undefined;
+ let previousPluginsDir: string | undefined;
beforeEach(() => {
+ previousDataDir = process.env.WIGOLO_DATA_DIR;
+ previousPluginsDir = process.env.WIGOLO_PLUGINS_DIR;
tmpDataDir = mkdtempSync(join(tmpdir(), 'wigolo-tool-annotations-'));
@@
afterEach(() => {
- delete process.env.WIGOLO_DATA_DIR;
- delete process.env.WIGOLO_PLUGINS_DIR;
+ if (previousDataDir === undefined) delete process.env.WIGOLO_DATA_DIR;
+ else process.env.WIGOLO_DATA_DIR = previousDataDir;
+ if (previousPluginsDir === undefined) delete process.env.WIGOLO_PLUGINS_DIR;
+ else process.env.WIGOLO_PLUGINS_DIR = previousPluginsDir;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| beforeEach(() => { | |
| tmpDataDir = mkdtempSync(join(tmpdir(), 'wigolo-tool-annotations-')); | |
| process.env.WIGOLO_DATA_DIR = tmpDataDir; | |
| // `pluginsDir` defaults to `<dataDir>/plugins`, so the line above already | |
| // isolates it — but pin it anyway so an exported WIGOLO_PLUGINS_DIR in the | |
| // developer's shell can't make `initSubsystems()` import real plugin code. | |
| process.env.WIGOLO_PLUGINS_DIR = join(tmpDataDir, 'plugins'); | |
| resetConfig(); | |
| _resetMigrationGuard(); | |
| vi.clearAllMocks(); | |
| }); | |
| afterEach(() => { | |
| delete process.env.WIGOLO_DATA_DIR; | |
| delete process.env.WIGOLO_PLUGINS_DIR; | |
| resetConfig(); | |
| try { rmSync(tmpDataDir, { recursive: true, force: true }); } catch { /* ignore */ } | |
| }); | |
| describe('tools/list capability annotations', () => { | |
| let tmpDataDir: string; | |
| let previousDataDir: string | undefined; | |
| let previousPluginsDir: string | undefined; | |
| beforeEach(() => { | |
| previousDataDir = process.env.WIGOLO_DATA_DIR; | |
| previousPluginsDir = process.env.WIGOLO_PLUGINS_DIR; | |
| tmpDataDir = mkdtempSync(join(tmpdir(), 'wigolo-tool-annotations-')); | |
| process.env.WIGOLO_DATA_DIR = tmpDataDir; | |
| // `pluginsDir` defaults to `<dataDir>/plugins`, so the line above already | |
| // isolates it — but pin it anyway so an exported WIGOLO_PLUGINS_DIR in the | |
| // developer's shell can't make `initSubsystems()` import real plugin code. | |
| process.env.WIGOLO_PLUGINS_DIR = join(tmpDataDir, 'plugins'); | |
| resetConfig(); | |
| _resetMigrationGuard(); | |
| vi.clearAllMocks(); | |
| }); | |
| afterEach(() => { | |
| if (previousDataDir === undefined) delete process.env.WIGOLO_DATA_DIR; | |
| else process.env.WIGOLO_DATA_DIR = previousDataDir; | |
| if (previousPluginsDir === undefined) delete process.env.WIGOLO_PLUGINS_DIR; | |
| else process.env.WIGOLO_PLUGINS_DIR = previousPluginsDir; | |
| resetConfig(); | |
| try { rmSync(tmpDataDir, { recursive: true, force: true }); } catch { /* ignore */ } | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unit/server/tool-annotations.test.ts` around lines 140 - 156, Update
the beforeEach/afterEach setup around WIGOLO_DATA_DIR and WIGOLO_PLUGINS_DIR to
capture their inherited values before overriding them, then restore those values
in afterEach instead of always deleting the variables. Preserve deletion only
when a variable was originally unset, while keeping the existing resetConfig,
cleanup, and mock-reset behavior unchanged.
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
1 participant
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.
What & why
The ten tools in
tools/listship with noannotations, so every host falls back to its own default. Per the MCP spec that default isreadOnlyHint: false— wigolo is currently advertising all ten tools as potentially state-changing.The sharpest consequence is Claude Code's plan mode: it refuses any MCP tool that is not annotated read-only, and it resolves that before consulting permission rules. So an
mcp__wigolo__*allow rule cannot lift it, and every wigolo call prompts in plan mode no matter what the user puts insettings.json. Observed on Claude Code 2.1.220.Cursor, VS Code, Zed, Windsurf and opencode read the same hints for auto-approval, and wigolo ships installers for all of them, so this is not Claude-Code-specific.
Changes
src/server.ts—annotationson all tentools/listentries. Eight are read-only.cacheandwatchare not, and say so:cacheacceptsclear,watchcreates and deletes jobs. Prompting on those two is the correct outcome.cacheis open-world —check_changesre-fetches every matching cached URL over the network (src/tools/cache.ts:55).diffis closed-world — it resolves itsurlsides from the local cache and returnscache_missrather than fetching (src/tools/diff.ts:49-56).idempotentHintis inert whereverreadOnlyHintis true (spec: "meaningful only whenreadOnlyHint == false"), so those tools carrytruefor consistency rather than as a claim about output stability.readOnlyHint: truewhilecache— the tool that exposes the store directly — does not. Noted in a comment so the next reader does not have to re-derive it.tests/unit/server/tool-annotations.test.ts— pins the full matrix, asserts every hint is an explicit boolean (an absent hint falls back to a host default, which is the failure this fixes), and asserts exactlycache+watchare non-read-only.docs/troubleshooting.mdcovering the plan-mode behaviour, the~/.claude/settings.jsonallow rule, and the restart requirement — permission rules are read once at session start, which is the step people miss. Pointers fromdocs/installation.mdand the README troubleshooting list.No schema, handler, or API-surface change. Purely additive.
Testing
npm testpassesnpm run lintpassesFull suite locally: 4 pre-existing failures in
llm-fallback-e2e,extraction/llm-fallback, andintegrations/llm-runner. All three assert "no LLM provider configured" and fail on any machine with a provider key in the OS keychain. Verified identical before and after this change by stashing it and re-running the same files, so they are unrelated to this PR.tests/unit/server/schema-registration.test.tsasserts tool names and count only, never whole tool objects, so adding anannotationskey does not break it — confirmed by running it.Checklist
CONTRIBUTING.mdand agree to its contribution termsSummary by CodeRabbit
New Features
Documentation
Tests