fix(binding-mcp-kafka-connect): accept arbitrary connector config fields - #2538
Merged
Conversation
…body/query McpHttpArguments now accepts the route's path-bound argument names and excludes them from the forwarded tools/call arguments stream while still capturing their scalar values for path interpolation. Without this, a route whose body/query has no explicit template (so the whole arguments object flows through unfiltered) would leak its own path parameter into the outbound request body/query as an extra, unintended field -- a prerequisite for letting such routes accept a fully open-ended body shape. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CLFwU8RqaAWL8xDBXSENvr
…nt's value The KEY_NAME branch only checked forwardDepth==1 to decide whether to forward a key, so a key nested inside an already-excluded argument's own object value (forwardDepth > 1) fell through to the default forwarding path instead of being suppressed. Check the suppressing flag first so every event nested under an excluded argument is withheld, not just its own top-level key. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CLFwU8RqaAWL8xDBXSENvr
…values Two related bugs surfaced by a full clean verify against the existing k3po suite (baseline 126/126 green, this branch had 34 failures/timeouts before this fix): 1. Deciding whether a top-level key is excluded requires the whole key, not a fragment of it. KEY_NAME now declines an incomplete key (consumed(0), STARVED) the same way JsonSchemaImpl.Validator and JsonProjectorImpl already do, instead of matching against a partial view and forwarding the fragment before the exclusion decision was even known. 2. Once a key's value is withheld from the sink, nothing downstream consumes its bytes and advances the source's per-fragment cursor, so getStringView() re-presents everything seen so far on every call instead of just the newest delta -- accumulating those into `text` via append duplicated content. Replace rather than accumulate while suppressing. McpHttpArgumentsTest now drives every case through every input window size from 1 byte up to the full document, which is what caught both bugs. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CLFwU8RqaAWL8xDBXSENvr
update_connector_config and validate_connector_config both declared a requestBody schema with only connector.class/tasks.max as named properties, so binding-mcp-openapi's schema-driven body projector dropped any other connector-type-specific field before it ever reached the outbound Kafka Connect REST call -- e.g. a FileStreamSourceConnector's required file/topic fields never arrived, making PUT .../config fail with "missing required configuration" despite the caller supplying them. Both operations' PUT bodies are already a flat, arbitrary key/value config map per the Kafka Connect REST contract (unlike POST /connectors, which wraps config under a "config" key) -- widen the schema to a bare open object, the same idiom create_connector's own generic "config" property already uses, so the projector retains the whole body instead of pruning it to a fixed enumerated set. Relies on the mcp-http fix in the preceding commits to keep the route's own path parameter (connector/pluginName) from leaking into that now-open body. k3po specs updated to cover a FileStreamSourceConnector's extra required fields (file, topic) alongside connector.class/tasks.max, both at the MCP tool-call layer and the outbound HTTP layer. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CLFwU8RqaAWL8xDBXSENvr
Reverts the excludedKeys mechanism added for kafka-connect (McpHttpArguments/McpHttpProxyFactory changes and their test). A full binding-mcp-http clean verify against this branch showed 34 test failures (mostly hangs) across scenarios entirely unrelated to kafka-connect (plain GitHub-PR-creation fixtures, query-parameter routes, etc.), and two rounds of fixes driven by a synthetic unit test did not move that count at all -- a sign the real bug was never isolated. More fundamentally, the mechanism was solving the wrong layer: once an operation's body schema stops being a closed, fully-enumerated set (via either this branch's earlier "bare object" schema or a proper additionalProperties: true), a path parameter sharing the same tools/call arguments object needs excluding from the body regardless of which approach opens the schema up -- and JSON Schema already has the tool for that (a false sub-schema), without any new imperative forwarding state machine in a hand-rolled streaming JSON transform. Replacing this with a schema-level fix in common-json/common-openapi/binding-mcp-openapi next. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CLFwU8RqaAWL8xDBXSENvr
…ned paths JsonSchema#retainedPaths() (and thus JsonTransforms.projector(JsonSchema), which shapes output from a schema by pruning to the paths it retains) only ever walked a schema's named properties/items/combinators -- additionalProperties was parsed and enforced for validation, but never consulted when computing what to keep. A schema combining named properties with a permissive additionalProperties therefore had no way to express "these declared fields, plus anything else of this shape": the pruning step silently dropped whatever wasn't explicitly named, the same as if additionalProperties were absent or false. JsonSchemaImpl now adds a wildcard retained path (the object-key counterpart to the existing array "-" wildcard, using a distinct "*" segment since an object key literally named "-" already matches as a plain key) alongside a structured schema's named properties whenever additionalProperties is explicitly present and not false -- recursing into its own sub-schema when typed, or treating it as an open leaf otherwise. Absent additionalProperties keeps every existing schema's closed, pruned- to-named-properties behavior unchanged. JsonProjectorImpl's object-key lookup gains the matching wildcard fallback, and a trie node's fragment-decline bound no longer short-circuits to SKIP once a wildcard sibling means a key longer than every named candidate can still match. common-openapi's OpenapiSchema/OpenapiSchemaView gain the additionalProperties field itself (previously absent from the model entirely, so it was silently dropped by JSON-B during parsing regardless of what an OpenAPI document wrote) -- captured as a raw JsonValue and carried through to the schema text a consumer like binding-mcp-openapi hands to JsonSchema.of(), rather than resolved/recursively bound like items/properties/schema, since a boolean literal is a valid alternative to a nested schema object here. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CLFwU8RqaAWL8xDBXSENvr
…nector config update_connector_config and validate_connector_config now declare connector.class/tasks.max as real named properties plus additionalProperties: true, matching standard OpenAPI/JSON Schema idiom and relying on the additionalProperties support just added to common-json/common-openapi. This alone is not yet sufficient: the operation's own path parameter (connector/pluginName) shares the same tools/call arguments object as the body, so it would currently also pass through the new wildcard into the outbound body -- excluding it needs either proper deny-path support in common-json's JsonSchema/JsonProjectorImpl (a keep-only pointer list can't express "deny this even though a sibling wildcard would keep it") or a separate mechanism, still to be decided. tools.list's advertised schema is updated to match the target shape. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CLFwU8RqaAWL8xDBXSENvr
… wildcard retainedPaths() is a pure keep-list: a named property whose schema is false correctly gets no pointer (it's neither kept), but "no pointer" and "unlisted key covered only by a sibling additionalProperties wildcard" were indistinguishable to JsonProjectorImpl -- so a structured schema with both a denied named property and a permissive additionalProperties (e.g. an operation's own path parameter declared false alongside a wildcard for its otherwise-open body) had no way to keep the wildcard from swallowing the one key it was supposed to exclude. JsonSchema#rejectedPaths() collects the RFC 6901 pointers to explicitly deny (JsonSchemaImpl already tracked this per-node as `deny`, just never surfaced it), mirroring retainedPaths()'s own collection pass. JsonTransforms#projector(JsonSchema) now feeds both lists to JsonProjectorImpl, whose trie nodes gain a `rejected` flag alongside `keepAll` -- checked first in decide(), so an explicit reject always wins over this same node's own keepAll, even when reached only via a wildcard sibling. The trie's fragment-decline bound already treats a wildcard sibling as "no early SKIP" (from the additionalProperties support just added); this needed no further change since a rejected node is reached by the same exact-match path as any other named child. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CLFwU8RqaAWL8xDBXSENvr
… body schema An operation's path/query/header/cookie parameters share the same tools/call arguments object as its request body. Once a structured body schema is widened by additionalProperties (now honored by common-json's JsonSchema#rejectedPaths()/retainedPaths()), a parameter name not already a declared body property would otherwise pass straight through that wildcard into the outbound body alongside the fields it actually describes. bodySchema() now denies every such parameter name explicitly (a false sub-schema merged into the schema's own properties) unless it's already a real declared body property -- e.g. pulls/create's requestBody deliberately declares its own "owner" field alongside the path parameter of the same name, and that stays untouched. JsonSchema#rejectedPaths() picks up the denied names and the body projector excludes them even via the wildcard, per the mechanism just added to common-json. Verifying against the full binding-mcp-openapi k3po suite before this lands in the kafka-connect module that motivated it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CLFwU8RqaAWL8xDBXSENvr
Contributor
Author
|
The Generated by Claude Code |
Merged
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
update_connector_configandvalidate_connector_configdeclaredrequestBodyschemas with onlyconnector.class/tasks.maxas named properties, sobinding-mcp-openapi's schema-driven body projector silently dropped any other connector-type-specific field (e.g. aFileStreamSourceConnector's requiredfile/topic) before it ever reached the outbound Kafka Connect REST call — making a realupdate/validatecall fail with a "missing required configuration" error despite the caller supplying the field.Root cause traced to two independent gaps in the shared pipeline these tools sit on top of:
common-json'sJsonSchema#retainedPaths()(used to shape the outbound HTTP body from a schema) only ever walked a schema's declaredproperties/items/combinators —additionalPropertieswas parsed and enforced for validation, but never consulted when computing what to keep. A schema combining named properties with a permissiveadditionalPropertieshad no way to express "these fields, plus anything else of this shape."additionalProperties, the operation's own path parameter (connector/pluginName) — which shares the sametools/callarguments object as the body — would otherwise pass straight through that new wildcard into the outbound body too. A pure keep-list has no way to express "deny this one path even though a sibling wildcard would otherwise retain it."Changes
common-json:JsonSchemaImplnow adds a wildcard retained path (/*, the object-key counterpart to the existing array/-wildcard) alongside a structured schema's named properties wheneveradditionalPropertiesis explicitly present and notfalse; absentadditionalPropertieskeeps every existing schema's closed, pruned-to-named-properties behavior unchanged.JsonSchemagains a newrejectedPaths()method (mirroringretainedPaths()) for paths explicitly denied (false, or an equivalent always-fails sub-schema) — a rejected path always wins over the same path being retained elsewhere, e.g. via a wildcard.JsonProjectorImpl's trie gains arejectednode flag checked ahead ofkeepAll, and its fragment-decline bound no longer short-circuits toSKIPonce a wildcard sibling means a longer key can still match.common-openapi:OpenapiSchema/OpenapiSchemaViewgain theadditionalPropertiesfield itself (previously entirely absent from the model, so any OpenAPI document'sadditionalPropertieswas silently dropped during parsing regardless of what was written) — captured as a rawJsonValueand carried through to the schema text a consumer hands toJsonSchema.of().binding-mcp-openapi:McpOpenapiCompositeGenerator#bodySchema()now denies every operation parameter not already a declared body property (afalsesub-schema merged into the generated body schema's ownproperties), so an operation's path/query/header/cookie parameters can never leak through an open body schema's wildcard — verified against the existingpulls/createfixture, which deliberately declares a body property with the same name as one of its path parameters, to confirm that legitimate case is untouched.binding-mcp-kafka-connect:update_connector_config/validate_connector_config'srequestBodyschema now keepsconnector.class/tasks.maxas real, documented named properties and adds"additionalProperties": true, matching standard OpenAPI/JSON Schema idiom (rather than the reverted first-pass workaround of dropping all named properties down to a bare open object).All new/changed behavior is covered by unit tests in
common-json(including explicit fragmentation tests across every input window size, and an end-to-end test reproducing the exact reject-over-wildcard scenario this issue needed) andbinding-mcp-openapi, plus updated k3po specs inspecs/binding-mcp-kafka-connect.specexercising aFileStreamSourceConnector's extrafile/topicfields at both the MCP tool-call layer and the outbound HTTP layer.Fixes #2530
Test plan
runtime/common-json— full unit suite green, including newadditionalProperties/wildcard/rejectedPathstests (single-shot and windowed/fragmented across every input window size)runtime/common-openapi— full unit suite greenruntime/binding-mcp-openapi— full unit + k3po suite green (56 unit + 26 IT), including a new generator test asserting the denied-parameter-name injectionruntime/binding-mcp-kafka-connect+specs/binding-mcp-kafka-connect.spec— full unit + k3po suite green (87 tests), including updatedupdate_connector_config/validate_connector_configscenarios passing aFileStreamSourceConnector's extrafile/topicfields through while excluding theconnector/pluginNamepath parameter from the outbound bodyGenerated by Claude Code