fix(apicalls): a failed httpcall tool returned "{}" — the model could not know it failed - #684
Merged
Merged
Conversation
… not know it failed
The human approved setupAgent, the call went out and got a 400 - and the
model was handed "{}" as the tool result. HttpCallToolsProvider serializes
ApiCallExecutor.execute()'s returned map verbatim, and that map was only
populated inside "if (isResponseSuccessful && call.getSaveResponse())".
On a non-2xx it stayed EMPTY; on a 2xx with saveResponse=false it stayed
empty too. The model could neither report a failure nor confirm a success:
a human-approved call that 400'd looked exactly like one that worked.
- non-2xx -> {"httpCode", "body": error body truncated to 2000 chars}
(status message when blank). Same keys as the success path, not a new
"error" namespace: ApiCallsTask merges this map into template data,
where that vocabulary is already established.
- 2xx with saveResponse=false -> {"httpCode"}. The body stays out (that is
what the flag means), but a model whose tool returned "{}" cannot tell a
204 from a crash.
- Response OBJECT semantics untouched: an error body still never lands
under responseObjectName; memory sees it only under the *Error key.
Four new tests pin the contract; four existing tests that asserted the
empty map were updated - they were pinning the bug.
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 |
…'s quiet success
Caught in self-review of the tool-result contract: the failure branch
populates body/httpCode on its pass through the retry loop, and a
succeeding retry with saveResponse=false writes only httpCode - so the
model would have been handed {"httpCode": 200, "body": "<the 503's
error>"}, a self-contradictory tool result. The result map is now cleared
at the top of each attempt: the final attempt wins, entirely.
Test stubs two distinct responses switched per attempt via send() -
sequential stubbing on one mock would flip mid-attempt, since the code
reads getHttpCode() several times per iteration. Verified by mutation:
removing the clear() fails it.
There was a problem hiding this comment.
Pull request overview
Ensures HTTP-call tools return actionable status information to the LLM.
Changes:
- Returns HTTP status and truncated error bodies for non-2xx responses.
- Returns status codes for successful calls with
saveResponse=false. - Adds regression coverage for retries and response handling.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
ApiCallExecutor.java |
Implements the revised result contract. |
ApiCallExecutorTest.java |
Tests tool results and retries. |
ApiCallExecutorValidationErrorTest.java |
Verifies truncated errors reach results. |
ApiCallExecutorBranchCoverageTest.java |
Updates branch expectations. |
docs/changelog.md |
Documents the fix. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Copilot's review caught it: the retried-failure-leak regression added a fifth test after the entry was written. It is the most load-bearing of the five, so it is now named rather than merely counted.
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.
The missing piece of "after approval, nothing happened": the human approved
setupAgent, the call went out, got a 400 — and the model was handed{}as its tool result.HttpCallToolsProviderserializesApiCallExecutor.execute()'s returned map verbatim, and that map was only populated insideif (isResponseSuccessful && call.getSaveResponse()). On a non-2xx it stayed empty; on a 2xx withsaveResponse=false, empty too. So the model could neither report a failure nor confirm a success — a human-approved call that 400'd looked exactly like one that worked. (Live evidence:Httpcall tool 'setupAgent' result: keys=[] size=2right after the 400 in this morning's log.)Contract now
{"httpCode": 400, "body": "<error body, truncated to 2000>"}— status message when the body is blanksaveResponse=false{"httpCode": 204}— body stays out, that is what the flag meanssaveResponse=truebody+httpCode)Same keys as the success path rather than a new
errornamespace:ApiCallsTaskmerges this map into template data, where that vocabulary is already established. Response object semantics are untouched — an error body still never lands underresponseObjectName, and memory still sees it only under the*Errorkey.Tests
Five new tests pin the contract (error body + code, blank-body fallback, truncation in the result, code-only on quiet success, and a mutation-verified regression for the retried-failure leak — a 503's error body must not survive into a succeeding retry's quiet-success result; the map is cleared per attempt, final attempt wins). Four existing tests asserted the empty map and were updated — they were pinning the bug. 488 tests across the apicalls/LlmTask suites green.