MCP exposure & input validation TODO's Completion

[MarawanYakout]

Summary

Tool Re-exposure: src/entrypoints/mcp.ts has TODOs to re-expose any connected MCP tools back through the server, enabling "Inception-style" tool chaining

what changed & why it changed:

1. Import and Initialization (File: src/entrypoints/mcp.ts)

   • Action: Import getMcpToolsCommandsAndResources. Add arrays for mcpClients and mcpTools.
   • Why: We need to store the state of connected MCP servers and their tools.

2. Load Connections (File: src/entrypoints/mcp.ts)

   • Action: Await getMcpToolsCommandsAndResources inside startMCPServer, pushing each client and its tools to the arrays.
   • Why: Establishes connections so tools can be queried and executed.

3. Re-expose Tools (File: src/entrypoints/mcp.ts)

   • Action: In ListToolsRequestSchema, combine getTools(...) with mcpTools.
   • Why: This fulfills the TODO to re-expose connected MCP tools to the caller.

4. Execute Tools (File: src/entrypoints/mcp.ts)

   • Action: In CallToolRequestSchema, combine getTools(...) with mcpTools for findToolByName. Pass mcpClients into toolUseContext.options.mcpClients.
   • Why: Our internal tool abstraction for MCP tools relies on finding the active connection in toolUseContext.options.mcpClients to execute the remote call.

Impact

• user-facing impact:
• developer/maintainer impact:

Testing

Testing Strategy

• Security scan: Run npx ecc-agentshield scan .
• Manual check or unit test of claude mcp output if possible.

Risks & Mitigations

• Risk: Connecting to slow MCP servers delays the claude mcp startup.

  • Mitigation: getMcpToolsCommandsAndResources runs in parallel with timeouts, so it will bound the startup delay.

• Risk: Name collisions between native tools and MCP tools.

  • Mitigation: Our MCP tool wrapper automatically prefixes or handles tool names appropriately if conflicts exist, but findToolByName will return the first match.

• bun run build

• bun run smoke

• [ X ] focused tests:

Success Criteria

• claude mcp loads MCP configurations.
• ListToolsRequestSchema returns native + MCP tools.
• CallToolRequestSchema can successfully route calls to the connected MCP tools.

[MarawanYakout]

solved merge conflicts

[kevincodex1]

looks great

[MarawanYakout]

Here are 2 potential solutions i can start with:

Solution 1: Tool Deduplication (Recommended)

This solution explicitly removes built-in tools that share a name with an overriding MCP tool. This prevents findToolByName from returning the wrong tool and ensures the ListTools endpoint doesn't return duplicate tool definitions.

Implementation:
In src/entrypoints/mcp.ts, replace:

const toolPermissionContext = getEmptyToolPermissionContext()
const tools = [...getTools(toolPermissionContext), ...mcpTools]

With:

const toolPermissionContext = getEmptyToolPermissionContext()
const builtins = getTools(toolPermissionContext)
const mcpToolNames = new Set(mcpTools.map(t => t.name))
const deduplicatedBuiltins = builtins.filter(t => !mcpToolNames.has(t.name))

const tools = [...mcpTools, ...deduplicatedBuiltins]

(This change must be applied inside both the ListToolsRequestSchema and CallToolRequestSchema handlers).

Solution 2: Reordering the Spread (Simpler, but less robust)

If we only care about findToolByName picking the correct tool, we could simply swap the order of the arrays so mcpTools comes first.

Implementation:

const tools = [...mcpTools, ...getTools(toolPermissionContext)]

Pros: One-line fix.
Cons: The ListTools endpoint will still return two tools with the exact same name (e.g., two Bash tools), which violates the intended uniqueness of tool schemas and might break external MCP clients.

Personally, Solution 1 is the correct path forward because it fixes the immediate bug in CallTool (correct resolution) while simultaneously preventing malformed duplicate schemas in ListTools.

[kevincodex1]

lets go with solution 1 then

[anandh8x]

@MarawanYakout I reviewed this branch locally and I think Solution 1 is the correct fix, not Solution 2.

Reordering alone fixes the immediate findToolByName() precedence issue, but it still leaves duplicate tool names in ListTools.

One additional change is still needed though: in ListTools, MCP tools should prefer tool.inputJSONSchema ?? zodToJsonSchema(tool.inputSchema). Right now the MCP wrapper's inputSchema is just the generic passthrough object, so otherwise the re-exposed MCP tools lose their real remote argument schema even after the collision fix.

So I'd recommend:

1. apply Solution 1 in both handlers
2. in ListTools, prefer tool.inputJSONSchema when present
3. add a focused regression test for the no-prefix collision + duplicate-name + schema-preservation path

After that, the logic looks good to me.

[MarawanYakout]

1. Added getCombinedTools: Created and exported the getCombinedTools helper function in src/entrypoints/mcp.ts to implement the
   deduplication logic properly:

   1    export function getCombinedTools(
   2      builtins: InternalTool[],
   3      mcpTools: InternalTool[],
   4    ): InternalTool[] {
   5      const mcpToolNames = new Set(mcpTools.map(t => t.name))
   6      const deduplicatedBuiltins = builtins.filter(t => !mcpToolNames.has(t.name))
   7
   8      return [...mcpTools, ...deduplicatedBuiltins]
   9    }

2. Applied the Fix: Updated the ListToolsRequestSchema and CallToolRequestSchema handlers in src/entrypoints/mcp.ts to construct
   the tools array using this new function:

   1    const toolPermissionContext = getEmptyToolPermissionContext()
   2    const tools = getCombinedTools(getTools(toolPermissionContext), mcpTools)

3. Verified via TDD: I wrote a test in src/entrypoints/mcp.test.ts to ensure that when a collision occurs, the MCP tool takes precedence over the built-in tool.
4. Validation: I ran the full test suite (bun test) and the build script (bun run build). All tests, including the newly added
   one, passed successfully.

[gnanam1990]

Thanks for pushing this forward. I can see the MCP collision follow-up work on the branch, and I agree with the direction of re-exposing connected MCP tools. That said, after taking a deeper look at the current head, I’m still not comfortable approving this PR in its current form for three reasons:

1. PR scope is much broader than the stated change
   The PR is described as “MCP tool re-exposure & strict input validation,” but the current branch also includes a large number of unrelated changes across gRPC, onboarding, UI, docs, workflow, privacy/tooling, and other areas. That makes it very difficult to review or merge safely as a single unit.

Examples include changes in:

• src/grpc/server.ts
• src/components/ProviderManager.tsx
• src/components/ThemePicker.tsx
• docs/litellm-setup.md
• scripts/pr-intent-scan.ts
• .github/workflows/pr-checks.yml

I think this needs to be narrowed down substantially before it is mergeable.

2. “Strict input validation” for re-exposed MCP tools still does not appear to be truly enforced
   The current MCP server path still seems to rely on the MCP tool wrapper schema rather than the real upstream MCP schema when validating tool inputs.

Relevant files:

• src/entrypoints/mcp.ts
• src/services/mcp/client.ts
• src/tools/MCPTool/MCPTool.ts

As a result, the branch appears to re-expose MCP tools, but not actually validate arguments strictly against the real remote tool schema in the way the PR description claims.

3. Re-exposed MCP tool outputs are still flattened into plain text
   In the current mcp.ts path, tool results are converted into a single text response using either the string result or jsonStringify(finalResult.data). That loses the native structure of upstream MCP tool responses and will break or degrade any non-text or structured MCP result.

Relevant files:

• src/entrypoints/mcp.ts
• src/services/mcp/client.ts

So even if tool lookup precedence is fixed, the proxy path still looks lossy for richer MCP tools.

I think the MCP re-exposure idea is worth pursuing, but I’d want this PR either:

• split down to just the MCP-specific change, or
• updated so the current branch actually enforces strict upstream MCP schemas and preserves structured MCP outputs

Once that is tightened up, I’d be happy to take another look.

[Vasanthdev2004]

Rechecked the current head ad6ae12c3089a317d6fb30d7d31b90d12e7461bf against current origin/main.

The earlier no-prefix collision blocker is fixed on this head: MCP tools now win name collisions over builtins via getCombinedTools(...), and the focused tests/build/smoke pass.

I still can't approve because there is another real behavior bug in the new MCP exposure path.

1. src/entrypoints/mcp.ts:79-88
getMcpToolsCommandsAndResources(...) can return synthetic tools for non-connected states, especially the OAuth recovery tool for needs-auth servers (createMcpAuthTool(...)). But this entrypoint only records tools when client.type === 'connected':

   • if (client.type === 'connected') { mcpClients.push(client); mcpTools.push(...clientTools) }

   That means OAuth-gated MCP servers silently disappear from the re-exposed MCP server instead of surfacing their ...authenticate tool.

   This is a real regression relative to the intended behavior of getMcpToolsCommandsAndResources(...) itself, which explicitly emits:

   • client: { type: 'needs-auth', ... }
   • tools: [createMcpAuthTool(name, config)]

   So on this branch, a caller can no longer see or invoke the auth/recovery path for a needs-auth MCP server through this MCP entrypoint, even though the lower-level MCP client layer provides it.

Non-blocking note:

• the new src/entrypoints/mcp.test.ts only covers builtin-vs-MCP name deduplication. It does not cover the needs-auth path above, which is why this slipped through.

What I verified on this head:

• bun test src/entrypoints/mcp.test.ts src/utils/secureStorage/platformStorage.test.ts -> 12 pass
• direct runtime repro of MCP precedence: getCombinedTools([builtin Bash, builtin Read], [MCP Bash]) returns MCP Bash first
• bun run build -> success
• bun run smoke -> success

I did not find another high-confidence blocker beyond the dropped needs-auth tool path, but I wouldn't merge this until OAuth-gated MCP servers remain visible/authenticatable through the re-exposed MCP surface.

[gnanam1990]

@MarawanYakout good job.....! just one step more to reach target

[kevincodex1]

I'm looking into this and theres a lot of touched files?

[MarawanYakout]

After going about few things , MCP is very good on my end.

1. OAuth-gated MCP Server Visibility Restored

Issue: src/entrypoints/mcp.ts was silently dropping MCP servers that required authentication (needs-auth type), preventing users from invoking the ...authenticate tool through the re-exposed MCP entrypoint.
Resolution:
Modified src/entrypoints/mcp.ts to push tools and client connection contexts regardless of connection status, aligning with the expected behavior of getMcpToolsCommandsAndResources().

2. Strict Input Validation Enforced via Ajv

Issue: The proxy path previously relied on the local wrapper schema (z.object({}).passthrough()) for validation, allowing arbitrary inputs to bypass local inspection. The advertised schema was also inaccurately exposed.
Resolution:

• Schema Exposure: Updated src/entrypoints/mcp.ts to advertise the actual upstream JSON schema (tool.inputJSONSchema) instead of the empty wrapper schema.
• Strict Validation: Integrated ajv inside src/tools/MCPTool/MCPTool.ts. We now override validateInput to compile and enforce the real remote tool JSON schema (this.inputJSONSchema) before proceeding. If validation fails, it correctly surfaces an explicit validation error, preventing invalid parameters from reaching downstream layers.

3. Preserved Structured Output for MCP Tool Results

Issue: Re-exposed MCP tool outputs were being flattened into single plain-text blocks, losing native structures (like distinct image or text blocks) that are essential for richer tool responses.
Resolution:
Updated the final response mapping in src/entrypoints/mcp.ts. If the finalResult.data comprises an array of content blocks (matching the ContentBlockParam[] format), the blocks are now properly translated back into the MCP-compliant CallToolResult.content structures, accurately maintaining text and image block integrity.

Validations Performed

1. Unit Testing:
   Ran bun test src/entrypoints/mcp.test.ts src/utils/secureStorage/platformStorage.test.ts. All 12 test cases successfully passed.
2. Build Success:
   Executed bun run build cleanly without side effects or typing failures.
3. Security Standards Alignment:
   Followed best practices out of the Sec_skill guidelines ensuring:
   • Input Validation: Enforced explicit whitelist/schema verification (ajv.compile()) to reject invalid inputs early.
   • Error Handling: Enhanced error handling during schema compilation explicitly catches formatting errors while shielding sensitive inner stack traces from escaping directly to users.
   • Structured Responses: Correctly isolating text/images mitigates unintended parsing exploits when passing strings between sub-systems.

Technically system now upholds strict schema enforcement and full-spectrum MCP transparency while resolving the specific blockers noted in the latest PR review. lets see if there is any other adjustments needed, i am more than happy to adhere.

[MarawanYakout]

@kevincodex1 is there something in specific the project needs help in developing ? i can aim my focus towards specific areas of need or importance, tried to contacting on X but couldn't

[Vasanthdev2004]

Rechecked the current head b4d424b4f3d393889e588a3c9b1b5390bd704f5c against current origin/main.

Scope

This is a targeted re-review of the current head and the previously-blocked MCP areas, not a full fresh re-review of every historical branch state.

Verdict

Approve-ready

The confusing part here was mostly earlier branch churn and stale review context. On the current head, the actual merge surface is now narrow and coherent again:

• src/entrypoints/mcp.ts
• src/entrypoints/mcp.test.ts
• src/tools/MCPTool/MCPTool.ts
• src/utils/secureStorage/platformStorage.test.ts

What I specifically rechecked on this head:

1. No-prefix MCP collisions are fixed

   • getCombinedTools(...) deduplicates builtins when an MCP tool reuses the same name
   • MCP tools are ordered ahead of remaining builtins, so findToolByName(...) resolves the MCP override path correctly

2. Needs-auth / OAuth recovery tools are no longer dropped

   • loadReexposedMcpTools() now records tools and client contexts regardless of connection state, so the needs-auth recovery/authenticate path stays visible through the re-exposed MCP surface
   • there is focused coverage for that behavior in src/entrypoints/mcp.test.ts

3. The strict-validation / schema-exposure story is now materially better

   • ListTools now prefers tool.inputJSONSchema ?? zodToJsonSchema(tool.inputSchema)
   • MCPTool.validateInput() compiles and enforces inputJSONSchema with Ajv when present instead of only trusting the passthrough wrapper schema

4. Structured MCP outputs are preserved better than before

   • the CallTool result mapping now preserves text/image block structure instead of flattening everything into a single text blob by default

5. Checks

   • smoke-and-tests is green on this head

I’m approving this current shape. The earlier confusion was real, but the branch now looks focused and the previously-blocking MCP behavior issues appear addressed on the current head.

The merge-base changed after approval.

[Vasanthdev2004]

GitHub auto-dismissed the earlier approval after the merge base changed, so I rechecked the current state.

This is still a targeted re-review of the same head commit b4d424b4f3d393889e588a3c9b1b5390bd704f5c against the updated base, not a new full review from scratch.

I do not see a new blocker from the merge-base movement alone. The approval still stands on the current head:

• MCP/builtin collision handling still looks correct
• needs-auth / OAuth recovery tools are still preserved
• schema exposure + Ajv-backed validation still look correct for this scope
• structured MCP output preservation still looks intact
• checks are still green

So my verdict remains Approve-ready on the current head.

[Vasanthdev2004]

@gnanam1990 one more eyes

[Vasanthdev2004]

good work @MarawanYakout

The merge-base changed after approval.

[MarawanYakout]

@Vasanthdev2004 @gnanam1990 Thanks guys !

[MarawanYakout]

i'm here if there is any extra adjustments

[MarawanYakout]

@kevincodex1 @gnanam1990 @Vasanthdev2004 if guys need help with important issues / pull requests i can help with that too if you guys are working on something challenging or needs collap.

[Vasanthdev2004]

sure @MarawanYakout we will let you know

[kevincodex1]

theres a lot of change files here @Vasanthdev2004 @gnanam1990

[Vasanthdev2004]

@MarawanYakout can you make a new pr for this fix seems this pr messed up with lot of commits shows 5k line of code change

[MarawanYakout]

made a new one @Vasanthdev2004 #675