Skip to content

Fix MCP tool allowlist filtering in SDK run path#501

Merged
jahooma merged 1 commit intomainfrom
aether/fix-mcp-tool-filter-df8f
Apr 15, 2026
Merged

Fix MCP tool allowlist filtering in SDK run path#501
jahooma merged 1 commit intomainfrom
aether/fix-mcp-tool-filter-df8f

Conversation

@aether-agent
Copy link
Copy Markdown
Contributor

@aether-agent aether-agent bot commented Apr 15, 2026

Summary

  • fix MCP tool filtering in sdk/src/run.ts to use value membership instead of array index/property checks
  • add a regression test covering an agent with a restricted MCP tool allowlist
  • validate with targeted SDK test execution and SDK typecheck

Validation

  • bun test sdk/src/__tests__/run-mcp-tool-filter.test.ts
  • bun run typecheck (from sdk/)

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Apr 15, 2026

Greptile Summary

This PR fixes a one-line bug in sdk/src/run.ts where the JavaScript in operator was used to test array value membership instead of Array.prototype.includes. The in operator checks whether a value exists as a key (index) of an array, not as a value — so 'browser_navigate' in ['browser_navigate', 'browser_snapshot'] always returns false. The fix replaces tool.name in toolNames with toolNames.includes(tool.name), which correctly tests value membership. A regression test is added to cover the exact failure mode.

  • Root cause: in checks object keys ('0', '1', …), not string values stored in an array.
  • Fix: Single-character change to use Array.prototype.includes for correct value membership.
  • Test coverage: New test mocks the MCP client to return three tools (browser_navigate, browser_snapshot, browser_click) and asserts that only the two allowlisted tools survive filtering — directly exercising the patched code path.
  • No broader scope changes: Only run.ts (the fix) and the new test file are touched.

Confidence Score: 5/5

Safe to merge — the fix is a correct, minimal one-line change with a targeted regression test that directly validates the repaired behavior.

The bug (in vs includes on an array) is unambiguously wrong and the fix is unambiguously correct. The change touches only two files, introduces no new dependencies, and the new test would have caught the original defect. No side-effects on other call sites.

No files require special attention.

Important Files Changed

Filename Overview
sdk/src/run.ts Single-line fix replacing tool.name in toolNames with toolNames.includes(tool.name) — the in operator was testing array indices rather than values, so MCP tool allowlist filtering was silently broken; fix is correct and minimal.
sdk/src/tests/run-mcp-tool-filter.test.ts New regression test that stubs the MCP client with three tools and asserts only the two allowlisted ones are returned; directly exercises the patched requestMcpToolData path and would have caught the original bug.

Sequence Diagram

sequenceDiagram
    participant C as CodebuffClient
    participant R as run.ts (runOnce)
    participant MCP as getMCPClient / listMCPTools
    participant MP as callMainPrompt

    C->>R: client.run({ agent, prompt })
    R->>MP: callMainPrompt({ requestMcpToolData, ... })
    MP->>R: requestMcpToolData({ mcpConfig, toolNames })
    R->>MCP: getMCPClient(mcpConfig)
    MCP-->>R: mcpClientId
    R->>MCP: listMCPTools(mcpClientId)
    MCP-->>R: { tools: [browser_navigate, browser_snapshot, browser_click] }
    Note over R: for each tool:<br/>toolNames.includes(tool.name) ✅<br/>(was: tool.name in toolNames ❌)
    R-->>MP: filteredTools: [browser_navigate, browser_snapshot]
    MP-->>R: { sessionState, output }
    R-->>C: AgentOutput
Loading

Reviews (1): Last reviewed commit: "Fix MCP tool allowlist filtering" | Re-trigger Greptile

@jahooma jahooma merged commit 4b81586 into main Apr 15, 2026
34 checks passed
@jahooma jahooma deleted the aether/fix-mcp-tool-filter-df8f branch April 15, 2026 02:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant