feat(mcp): let OpenAPI-generated tools read response headers - #688
Merged
ginccc merged 4 commits intoAug 15, 2026
Merged
Conversation
POST /agents/{agentId}/start answers 201 with an empty body and the new
conversation id only in Location. ApiCallExecutor fills the result map's
"headers" key only when the call declares a responseHeaderObjectName, and
McpApiToolBuilder never set one — it defaults to null, so no generated tool has
ever seen a response header. A model that started a conversation got
{"httpCode": 201} and no id, which makes an operator test-drive impossible.
buildApiCall now sets <name>_responseHeaders, unconditionally rather than only
where a spec documents a 201, so a badly-described spec still yields a usable
tool. This is the 201 + empty body + Location convention generally, not one
endpoint.
Exposure, stated plainly: response headers now reach the tool result and
conversation memory, Set-Cookie included. Accepted because the response body
already travels that path for these tools and is the larger surface; a
header-name filter would have to live in the executor's shared header path,
where it would also strip values hand-authored configs template against.
Review of the previous commit found the unconditional grant was not worth its exposure, and that two pre-existing ordering bugs made it worse. Scoped. buildApiCall now binds <name>_responseHeaders only where the operation plausibly answers in a header: a declared 201/202/3xx, or a 2xx declaring no content. An operation whose success declares a body is answering in the body; a spec documenting no responses gets nothing, which is the safe reading of missing information. Response headers reach the tool result, the LLM context and persisted conversation memory with nothing on that path redacting them — RequestRedactor is request-only and SecretRedactionFilter runs on the display copy — and Set-Cookie here is a live session credential, since HttpClientModule builds a cookie-aware application-scoped WebClientSession. Result map is a LinkedHashMap with headers inserted last. It is serialized verbatim and truncated from the front, and with a HashMap "headers" hashed ahead of "body" on both paths regardless of insertion order — so a per-tool limit or the tool-context budget spent the allowance on headers and cut away the body the model asked for. convertHeaderToMap returns a case-insensitive TreeMap. HTTP field names are case-insensitive and HTTP/2 mandates lowercase, so the same endpoint answers Location over h1 and location over h2. This already cost us: ApiCallExecutor looks up the literal "Content-Type", so a lowercase-header response took the <not-present> branch and stored every JSON body as a raw String. The changelog now also records that this reaches only agents created after the deploy — AgentSetupService persists the generated config at creation — and docs/httpcalls.md documents the field.
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Enables OpenAPI-generated MCP tools to consume response headers needed by bodyless REST responses.
Changes:
- Generates response-header bindings for qualifying OpenAPI responses.
- Preserves response-body ordering and case-insensitive header lookup.
- Adds documentation, changelog, and regression tests.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
ApiCallExecutor.java |
Orders headers after response bodies. |
McpApiToolBuilder.java |
Enables headers for qualifying operations. |
HttpClientWrapper.java |
Adds case-insensitive header lookup. |
ApiCallExecutorBranchCoverageTest.java |
Tests result ordering. |
McpApiToolBuilderTest.java |
Tests response-shape selection. |
HttpClientWrapperTest.java |
Tests header casing behavior. |
docs/httpcalls.md |
Documents response-header access. |
docs/changelog.md |
Records the feature and rationale. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+260
to
+261
| if (returnsDataInHeaders(operation)) { | ||
| httpCall.setResponseHeaderObjectName(name + "_responseHeaders"); |
| | httpCall.saveResponse | (`Boolean`) whether to save the `JSON` response into `${memory.current.httpCalls}` | | ||
| | httpCall.fireAndForget | (`Boolean`) whether to execute the request without waiting for a response to be returned, (useful for `POST`) | | ||
| | httpCall.responseObjectName | (`String`) name of the `JSON` object so it can be accessed from other `httpCalls` or `outputsets`. | | ||
| | httpCall.responseHeaderObjectName | (`String`) name under which the RESPONSE headers are stored, reachable as `${memory.current.httpCalls.<responseHeaderObjectName>.<Header-Name>}` and, for an LLM tool, returned under the result's `headers` key. Unset by default — set it only when the answer you need is in a header (a `201`'s `Location`, say) rather than the body, since headers reach conversation memory unredacted. Header names are matched case-insensitively. | |
Review finding on #688, and the reviewer is right that the status-based scoping does not close the exposure on its own. Choosing which operations may BIND headers is a different control from choosing which headers may be STORED. An operation that qualifies on its documented 201 still answers other calls — the error path especially — with a Set-Cookie, and that value is a live session credential, since HttpClientModule builds a cookie-aware application-scoped WebClientSession that EDDI actively replays. ApiCallExecutor now drops Set-Cookie, Set-Cookie2, the authorization and the authenticate headers before the map reaches the tool result, the template data or conversation memory, matched case-insensitively because the wire decides the casing. A deny-list rather than an allow-list: which header is useful is not knowable here (Location, ETag, a cursor, a rate-limit budget, a vendor X-*) and an allow-list would silently break hand-authored configs templating one of those — what is knowable is the small closed set that is never data. docs/httpcalls.md also used the legacy ${...} form for the new access path; EDDI's templates are Qute {...}, so copying it would have left a literal $.
…/EDDI into feat/generated-tools-response-headers # Conflicts: # docs/changelog.md
pull Bot
pushed a commit
to Stars1233/EDDI
that referenced
this pull request
Aug 17, 2026
… live guard A four-reviewer audit of labsai#679-labsai#689 plus an end-to-end operator-path trace, then a second adversarial round over this fix itself. Everything confirmed is addressed here; docs/changelog.md carries the full account. - Cross-version placeholder stranding: dropPendingApprovalPlaceholder now recognises the previous builds' default wordings (legacy constant, suffix-less tool-named), so the first post-upgrade resume of an in-flight pause no longer renders [stale placeholder, answer]. Two upgrade-boundary tests simulate a pre-upgrade pause. - Self-conversation guard is now enforced on the LIVE path, including the mixed-batch pause branch the second review round caught (ungated calls execute before the pause is thrown and are never rechecked). Shared core extracted; same NOT_EXECUTED envelope and trace everywhere. - labsai#684 contract narrowed: failed results stay out of ApiCallsTask's cross-call template merge and out of the RAG system prompt; error bodies (and the status-message fallback) are redacted before reaching the model. Memory-side *Error keys unchanged. - Test-drive read-back: a blank returningFields entry means NO filter - [""] no longer nulls steps/outputs/properties out of the snapshot. - Generated tools: body $refs resolve one level (schemas namespace only), so descriptions name real fields - a guessed say-body bound to InputData defaults and silently sent an empty message; enum values and defaults now reach parameter descriptions (the environment typo->production trap). - padDataLines normalises bare CR; RFC 7615 headers join the credential response deny-list; labsai#688's shared-path stripping disclosed in changelog. 264 tests across the affected suites, including mutation-informed pins: same-tool ordinal drop, refused-mid-batch pairing, blank-filter recovery, redaction survival of the failure reason.
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
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.
Why
Found while wiring the Platform Operator to test-drive another agent. That flow starts with
POST /agents/{agentId}/start, which answersResponse.created(conversationUri).build()— 201, an empty body, and the new conversation's id only in theLocationheader. The model received{"httpCode": 201}and had no way to learn the id every following call needs, so the capability could not work at all.The cause is one unset field.
ApiCallExecutorpopulates the result map'sheaderskey only when the call declares aresponseHeaderObjectName, andMcpApiToolBuilder.buildApiCallnever set one — it defaults to null, so no tool generated from an OpenAPI spec has ever seen a response header. That breaks a whole convention, not just this endpoint: 201 + empty body +Locationis how a large share of REST APIs report a create.Scoped, not universal
buildApiCallnow sets<name>_responseHeadersonly for operations that plausibly answer in a header: a declared201/202/3xx, or a2xxthat declares no content. An operation whose success declares a body is answering in the body and gets nothing; a spec documenting no responses gets nothing either — the safe reading of missing information.The first version of this granted headers to every generated call, and that is not worth the exposure. Response headers reach the tool result, the LLM context and conversation memory (persisted, and rendered in the Manager's tool trace), and nothing on that path redacts them —
RequestRedactoris request-only by construction andSecretRedactionFilterruns on the display copy.Set-Cookieis the case that matters:HttpClientModulebuilds a cookie-aware, application-scopedWebClientSession, so that value is a live session credential EDDI is actively replaying, and copying it into prompt-injectable context is whatHttpOnlyexists to prevent.In the Petstore fixture the scoping withholds headers from all five calls. EDDI's own spec documents
201on/agents/{agentId}/start, which is the case this exists for.Two ordering bugs fixed alongside — both pre-existing, both load-bearing here
Truncation ate the body.
ApiCallExecutor's result map is now aLinkedHashMapwithheadersinserted last. It is serialized verbatim as the tool result and truncated from the FRONT, and with a plainHashMapheadershashed ahead ofbodyon both the success and the error path regardless of insertion order — so a per-tool limit, or the always-on tool-context budget, spent the allowance on a header block and cut away the response body the model asked for.Header lookups were case-sensitive.
convertHeaderToMapnow returns aTreeMapwithCASE_INSENSITIVE_ORDER. HTTP field names are case-insensitive and HTTP/2 mandates lowercase, so the same endpoint answersLocationover h1 andlocationover h2. This was already costing us:ApiCallExecutorlooks the content type up as the literal"Content-Type", so against a lowercase-header response it found nothing, took the<not-present>branch, and stored every JSON body as a raw String instead of parsed JSON. The casing the server sent is preserved; only lookup is relaxed.Limitation, stated because it will otherwise read as a bug
AgentSetupServicepersists the generatedApiCallsConfigurationat creation, and the runtime loads the stored document. This therefore reaches agents created after the deploy; an operator provisioned earlier keepsresponseHeaderObjectName: nulluntil it is re-provisioned. There is no migration.Tests
Nine. Five on the builder pinning each response shape it decides on (201, 204, 3xx, a body-returning 200, an undocumented operation) plus a fixture-size assertion so the sweeping ones cannot pass on an empty stream; two on the executor pinning
headersafterbodyon both paths; one on the case-insensitive lookup; and the pre-existing coverage thatheadersis populated once the name is set.docs/httpcalls.mddocuments the field.🤖 Generated with Claude Code