Skip to content

fix: validate required SDK tool arguments before execution - #116

Merged
HarshMN2345 merged 2 commits into
mainfrom
codex/mcp-required-arguments
Sep 10, 2026
Merged

HarshMN2345 merged 2 commits into
mainfrom
codex/mcp-required-arguments

Conversation

@HarshMN2345

@HarshMN2345 HarshMN2345 commented Sep 10, 2026 •

Copy link
Copy Markdown
Member

Missing required arguments to hidden SDK tools now produce a client-input error naming the missing fields before SDK execution. For example, calling functions_create_variable without variable_id previously raised an internal TypeError, even though the generated tool schema already marked that field required.

Validate the existing schema's required keys after argument alias normalization. Supplied IDs and empty values remain unchanged, and optional SDK defaults still apply. No resource IDs are generated implicitly.

Fixes MCP-A.

Validation:

  • Reproduced the reported missing-argument TypeError against the locked SDK. The public-handler regression fails without validation and passes with the fix.
  • Tests exercise the public MCP call handler with real catalog metadata and a fake SDK service boundary. They assert returned input errors and successful results preserving supplied IDs, empty values, camelCase aliases, and omitted optional fields.
  • Python 3.12: 257 unit tests passed; Ruff, Black, Pyright, and Docker build passed.
  • Integration discovery: 10 passed, 14 skipped because Appwrite credentials were not configured.

@greptile-apps

greptile-apps Bot commented Sep 10, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 5/5

The PR appears safe to merge, with no outstanding correctness, security, or repository-rule violations.

Summary

  • Reports all missing required fields deterministically.
  • Preserves supplied IDs, camelCase aliases, empty values, and omitted optional arguments.
  • Reworks regression coverage through the public MCP tool-call handler.

Reviews (2) · Last reviewed commit: "test: cover required arguments through p..."

Comment thread tests/unit/test_server.py Outdated
@HarshMN2345
HarshMN2345 merged commit 0bb9607 into main Sep 10, 2026
5 checks passed
HarshMN2345 added a commit that referenced this pull request Sep 11, 2026
Preserve both the required-argument regressions from #116 and the query validation regressions from #117.
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.

2 participants