Skip to content

fix: mcp_tool_to_langchain parameters conversion error#191

Merged
NicoleMGomes merged 4 commits into
mainfrom
fix/issue-190
Jul 6, 2026
Merged

fix: mcp_tool_to_langchain parameters conversion error#191
NicoleMGomes merged 4 commits into
mainfrom
fix/issue-190

Conversation

@NicoleMGomes

@NicoleMGomes NicoleMGomes commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Disclaimer: Do not include SAP-internal or customer-specific information in this PR (e.g. internal system URLs, customer names, tenant IDs, or confidential configurations). This is a public repository.

Description

Fix mcp_tool_to_langchain in agentgateway/converters.py to respect the JSON Schema
"type" declared in each MCP tool property instead of hard-coding every field as str.

Previously, all fields in the generated Pydantic args schema were typed as str (or
str | None for optional fields), regardless of the actual type in input_schema. This
caused Pydantic to silently coerce non-string values — e.g. limit=10 became "10",
active=True became "True" — before forwarding them to call_tool. MCP servers that
expected native JSON types then rejected the call.

The fix adds a _JSON_TYPE_MAP lookup that maps JSON Schema type strings to Python types
(integer → int, number → float, boolean → bool, array → list,
object → dict, string → str). Properties with an absent or unrecognised "type"
fall back to Any. The user-guide is updated with a reference table for the mapping.

Related Issue

Closes #190

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update
  • Code refactoring
  • Dependency update

How to Test

  1. Create an MCPTool with integer and boolean properties in input_schema.
  2. Convert it with mcp_tool_to_langchain.
  3. Inspect lc_tool.args_schema.model_fields:
    • limit field annotation must be int (not str).
    • active field annotation must be bool | None (not str | None).
  4. Run the unit tests: pytest tests/agentgateway/unit/test_converters.py -v
    — all 20 tests, including the new TestMcpToolToLangchainTypeMapping class, should pass.

Checklist

  • I have read the Contributing Guidelines
  • I have verified that my changes solve the issue
  • I have added/updated automated tests to cover my changes
  • All tests pass locally
  • I have verified that my code follows the Code Guidelines
  • I have updated documentation (if applicable)
  • I have added type hints for all public APIs
  • My code does not contain sensitive information (credentials, tokens, etc.)
  • I have followed Conventional Commits for commit messages

Additional Notes

Only mcp_tool_to_langchain in agentgateway/converters.py is affected. No other
converters exist in the codebase. The fix is fully backwards-compatible: tools that only
use "string" properties continue to behave identically.

@NicoleMGomes NicoleMGomes marked this pull request as ready for review June 29, 2026 13:17
@NicoleMGomes NicoleMGomes requested a review from a team as a code owner June 29, 2026 13:17
cassiofariasmachado
cassiofariasmachado previously approved these changes Jun 29, 2026
View reviewed changes

@cassiofariasmachado cassiofariasmachado left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@NicoleMGomes NicoleMGomes mentioned this pull request Jun 29, 2026
Closed
@NicoleMGomes NicoleMGomes merged commit 2b64465 into main Jul 6, 2026
10 of 11 checks passed
@NicoleMGomes NicoleMGomes deleted the fix/issue-190 branch July 6, 2026 12:06
tiagoek added a commit that referenced this pull request Jul 6, 2026
@tiagoek
feat(sdk-review): add automated PR review skill (AFSDK-3937)
025d15b 
Adds the SDK Module Review skill to this repository:

## What this PR adds

**20 deterministic checks** (no LLM calls in CI):
- secrets, license-spdx, disclosure, hardcode, telemetry, docs, bdd,
  patterns, versioning + BREAKING family, commits, errors-logging,
  testing-depth, http-hygiene, concurrency, deps-supply,
  deletion-hygiene, constants, binding-shape, quality-gate-parity,
  pr-size

**Rule tiers:** SHADOW | FLAG | BLOCK | BLOCK_LOCKED, configured in
`.claude/config/rules.yaml`

**BLOCK_LOCKED rules cannot be suppressed:**
- SEC-* (all secrets), HC-03 (SAP-internal URL leak),
  DIS-06 (internal artifactory), LIC-01/02 (SPDX headers),
  BND-02 (BTP token URL concat), BND-05 (binding logs credentials),
  BREAKING-* (all breaking-change declarations)

**REUSE.toml aggregate exemption** — this repo uses REUSE.toml at root,
so per-file SPDX rules are baseline-exempted automatically.

**Cross-language BDD parity** — module-aliases.yaml seeded with
`dms ↔ documentmanagement` (Java sibling).

**4 PR signals posted per run:**
1. Inline comments per finding
2. Aggregated summary comment
3. GitHub check-run (Checks tab)
4. Status label (`sdk-review: ✅ passed` / `❌ blocked` / `⚠️ flagged`)

**Idempotent re-runs:** HTML markers let subsequent runs replace prior
artifacts rather than duplicate them.

## Structure

- `.claude/scripts/`: 20 check-*.sh + orchestrate.sh + aggregate.sh
- `.claude/scripts/lib/`: 12 shared libs (2 Python AST + 10 bash)
- `.claude/config/`: rules.yaml, module-aliases.yaml, baseline.json
- `.github/workflows/sdk-module-review.yml`: auto-trigger on every PR
- `.github/workflows/sdk-skill-isolation-check.yml`: validates sub-PRs
- `docs/PR-REVIEW.md`: user-facing docs
- `docs/BRANCH-PROTECTION-SETUP.md`: admin guide for required-check config
- `tests/sdk-review/`: 57 bats tests + 6 fixtures (placeholder secrets,
  not real credentials)
- `CONTRIBUTING.md`: amended — sdk-module-review is required before review

## Validation

- 57/57 bats tests passing
- shellcheck clean on all bash scripts
- E2E on 5 real PRs (#196, #191, #192, #185 in this repo + Java jv#9):
  zero false positives observed
- Self-test on this PR: 0 BLOCK, 0 FLAG (skill correctly skips its own files)
- Flagship BND-02 finding verified on Java new-module PR (token URL concat)

## Rollout

Recommended SHADOW → FLAG → BLOCK progression, 2 weeks each.
See `docs/BRANCH-PROTECTION-SETUP.md`.

## Note on test fixtures

`tests/sdk-review/fixtures/secrets-*.diff` contain intentional placeholder
strings that match secret regexes (e.g. `AKIATESTFIXTUREXXXXX`,
`eyJTESTFIXTURE...`) so the check-secrets rule can be unit-tested.
These are NOT real credentials.

Ref: https://jira.tools.sap/browse/AFSDK-3937
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cassiofariasmachado cassiofariasmachado cassiofariasmachado approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

mcp_tool_to_langchain parameters conversion error

2 participants

@NicoleMGomes @cassiofariasmachado