Fix: Dark Lab tool override silently drops parameters/inputSchema

[Deez-Automations]

Summary

The Dark Lab supply-chain tool override endpoint (PUT /darklab/api/v1/supply-chain/servers/{server_type}/tools) accepts and persists a full override object per tool (any JSON keys, no restriction) into MCPServerConfig.tool_overrides_json. But _apply_tool_overrides only ever read the description key from each override — parameters (or inputSchema) was accepted, stored, and reported back as applied, but silently discarded at apply time. The DB says the override is complete; the agent runtime never actually saw the modified parameter schema, so parameter-schema poisoning (a real, intended attack class — Dark Lab's whole purpose is being this platform's tool-poisoning sandbox, confirmed by its own docstring and by the two already-shipped Tool Poisoning Deletion/Exfil challenges using the same underlying mechanism for descriptions) was silently non-functional.

A correction to the linked issue's own suggested fix

The issue proposes:

if new_description:
    tool.description = new_description
    if new_parameters:
        tool.inputSchema = new_parameters

I verified this against the actual installed fastmcp package before writing anything, rather than trusting it: Tool is a pydantic model with model_config = {"extra": "forbid"}, and inputSchema is not a declared field on it — that name only exists on the wire-protocol MCPTool object built by Tool.to_mcp_tool(), which reads self.parameters internally. Confirmed empirically:

>>> tool.inputSchema = {...}
ValueError: "FunctionTool" object has no field "inputSchema"

Applying the issue's diff verbatim would have crashed tool-override application with a misleading log message, after the description half had already silently applied. The correct attribute is tool.parameters, confirmed end-to-end through tool.to_mcp_tool().inputSchema — what the LLM actually receives, not just an internal field.

Comparison against another open fix for the same issue

Another contributor has an independent, already-open PR (#549) for this same issue. Worth flagging directly since both are unmerged: #549's diff makes the identical tool.inputSchema = new_parameters assignment as the linked issue's own suggested fix above — I re-ran the exact empirical check against that PR's diff and it hits the same ValueError: "FunctionTool" object has no field "inputSchema". Not a judgment call, just re-confirming the same runtime fact against a second piece of code that makes the same assumption. This PR uses tool.parameters instead, which is the field that actually exists on the model.

Fix

_apply_tool_overrides now applies description and/or parameters independently (accepts parameters or inputSchema as the input key, writes to tool.parameters), and ToolOverridesUpdate (the API request model) gained a field_validator on tool_overrides.

Two review rounds, each catching a real gap in the fix itself

Round 1:

• override.get("parameters") or override.get("inputSchema") treats a deliberate "parameters": {} override (stripping every param off a tool) as falsy and silently drops it — the exact bug class this issue was filed for, reproduced in miniature by the fix meant to close it. Switched to explicit key-presence checks.
• A non-dict parameters value doesn't fail at assignment time (fastmcp doesn't validate on plain attribute assignment) — it fails later, in unrelated code that lists a server's tools (to_mcp_tool(), called from fastmcp's own tool-listing path), breaking tool discovery for that namespace's server until the override is reset. Added an isinstance guard before the assignment ever happens, plus the same check in the API validator for immediate write-time feedback (422) instead of a silent no-op followed by a later, unrelated crash.

Round 2 found the same failure shape had two more instances:

• A non-string description — identical crash-later behavior via the exact same mechanism, just an unguarded sibling field. Verified directly: tool.description = 12345 succeeds silently, tool.to_mcp_tool() then raises pydantic.ValidationError.
• A tool's entire override entry being a non-object (e.g. {"send_email": "not even a dict"}) — this one is worse: override.get("description") sits outside the function's own try/except, so a non-dict override raises AttributeError uncaught, crashing _apply_tool_overrides entirely — which means create_mcp_server fails, not just tool listing. Fixed by rejecting this at the API boundary (422, so it never reaches the DB from this write path) and defensively skipping (not crashing) at apply time, since nothing guarantees every row in the DB went through the validated path (a future direct write, a seed script, a migration).

All three failure shapes — non-object override entry, non-string description, non-dict parameters/inputSchema — are now rejected both at write time (clear 422) and, defensively, at apply time (skip with a warning log, never crash).

Test plan

• tests/unit/mcp/test_factory.py (new, 15 tests): description-only override still works; parameters-only override actually lands on tool.parameters; the override reaches the wire-protocol schema via tool.to_mcp_tool().inputSchema (end-to-end proof); inputSchema accepted as an alias; both together; parameters-only doesn't touch description; a deliberate {} override isn't dropped; a non-dict parameters is rejected without crashing (and to_mcp_tool() still succeeds afterward, proving nothing bad landed); parameters wins over inputSchema when both present; a non-string description is rejected the same way; a non-object override entry is skipped without crashing _apply_tool_overrides, and doesn't block other tools in the same batch; unknown tool name, empty overrides, and no-providers cases all handled cleanly
• tests/unit/apps/test_darklab_tool_overrides.py (new, 9 tests): ToolOverridesUpdate's validator accepts all valid shapes (including deliberate empty-dict parameters) and rejects non-dict parameters/inputSchema, non-string description, and non-object override entries, including when the bad entry is one of several tools in a batch
• pytest tests/unit/mcp/ tests/unit/ctf/ tests/unit/apps/ -q — 64 passed, 1 pre-existing unrelated skip
• pytest tests/unit/ -q — full suite, 330 passed, 4 pre-existing failures unrelated to this change (present before it too)
• Locally merge-tested against fresh origin/main: clean on its own, and clean paired individually with each of Fix: orchestrator confirms payment to vendor even when it fails #574, Fix: DNS-based SSRF bypass in guardrail webhook URL validation #575, and Fix: guardrail payload corruption before signing + missing after_tool for complete_task #576 (no new conflicts introduced by this branch; the only conflict in the batch remains the one already disclosed between Fix: DNS-based SSRF bypass in guardrail webhook URL validation #575 and Fix: guardrail payload corruption before signing + missing after_tool for complete_task #576, unrelated to this PR)

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.