Conversation
Co-authored-by: openhands <openhands@all-hands.dev>
|
📁 PR Artifacts Notice This PR contains a |
|
Thank you for the contribution @BORAN002 ! My agent seems to have found a risk of regression, do you think you could look into it? AGENT: P2 regression: skipping blank text blocks can discard the Anthropic prompt-cache breakpoint. The SDK automatically marks the last user/tool content block for caching. If a tool result ends with a blank block, this new continue skips the code that preserves its cache marker. I reproduced this through SDK serialization and LiteLLM’s Anthropic request conversion: base retains cache_control; PR loses it. That can reduce cache reuse and increase cost/latency. Preserve the tool-level marker before filtering, and cover this case. Otherwise, the intended fix checks out:
This changes LLM message serialization, so benchmark runs can encounter it regardless of which tools are registered. The repository’s review guide calls for lightweight evals and human review for this scope; I’d address the cache regression first. |
Co-authored-by: openhands <openhands@all-hands.dev>
Retain tool-level cache markers before filtering and carry trailing blank block markers onto preceding content for other roles. Cover automatic SDK caching through Anthropic request conversion and loopback HTTP. Co-authored-by: openhands <openhands@all-hands.dev>
|
Hi enyst, thanks for catching this — you're right.
Codex reproduced the regression and implemented the fix in 86199bd<86199bd>. Tool-level cache markers are now preserved before filtering blank blocks, including when the tool result is entirely blank. Regression tests cover both mixed and entirely blank tool results.
Validation passed all 1,037 LLM tests, full pre-commit, the four original HTTP cases, and six Anthropic-compatible loopback HTTP cases. The reproduction scripts and logs are linked in the updated PR description.
These checks validate request serialization; no live-provider or benchmark validation has been performed. Could you take another look and approve CI for the updated commit? Could you also advise which lightweight eval would be appropriate, or trigger it if maintainer access is required?
Thanks for the review!
…________________________________
发件人: Engel Nyst ***@***.***>
发送时间: 2026年9月13日 10:38
收件人: OpenHands/software-agent-sdk ***@***.***>
抄送: #ZHANG BORAN# ***@***.***>; Mention ***@***.***>
主题: Re: [OpenHands/software-agent-sdk] fix(sdk): normalize blank Chat Completions content across roles (PR #4981)
[Alert: Non-NTU Email] Be cautious before clicking any link or attachment.
[https://avatars.githubusercontent.com/u/6080905?s=20&v=4]enyst left a comment (OpenHands/software-agent-sdk#4981)<#4981 (comment)>
Thank you for the contribution @BORAN002<https://github.com/BORAN002> ! My agent seems to have found a risk of regression, do you think you could look into it?
AGENT:
P2 regression: skipping blank text blocks<https://github.com/OpenHands/software-agent-sdk/blob/e6c6e43f73fb328b8904c1496ac1a58cdf089dec/openhands-sdk/openhands/sdk/llm/message.py#L357-L359> can discard the Anthropic prompt-cache breakpoint.
The SDK automatically marks the last user/tool content block for caching. If a tool result ends with a blank block, this new continue skips the code that preserves its cache marker. I reproduced this through SDK serialization and LiteLLM’s Anthropic request conversion: base retains cache_control; PR loses it. That can reduce cache reuse and increase cost/latency. Preserve the tool-level marker before filtering, and cover this case.
Otherwise, the intended fix checks out:
* 120 focused tests pass.
* Four local HTTP cases fail on base and pass on the PR.
* No live-provider or benchmark validation performed.
This changes LLM message serialization, so benchmark runs can encounter it regardless of which tools are registered. The repository’s review guide calls for lightweight evals and human review for this scope; I’d address the cache regression first.
—
Reply to this email directly, view it on GitHub<#4981?email_source=notifications&email_token=B4HAN6VM2BO7FPV47GHBM3D5OYCD5A5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKNRVGAZTAMBXGUZ2M4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLDGN5XXIZLSL5RWY2LDNM#issuecomment-5650300753>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/B4HAN6VQRNBFGCPE3FTXEYL5OYCD5AVCNFSNUABGKJSXA33TNF2G64TZHMYTANBTGQZDMMBYGI5US43TOVSTWNJUGMYTKMJQGI2TFILWAI>.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS<https://github.com/notifications/mobile/ios/B4HAN6VSJYHENK66GV5KBQD5OYCD5A5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKNRVGAZTAMBXGUZ2M4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJKTGN5XXIZLSL5UW64Y> and Android<https://github.com/notifications/mobile/android/B4HAN6UNUEU2PP2ZNUQWN2L5OYCD5A5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKNRVGAZTAMBXGUZ2M4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLTGN5XXIZLSL5QW4ZDSN5UWI>. Download it today!
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
HUMAN:
I asked Codex to investigate contribution opportunities and prepare this fix for blank Chat Completions content. Codex implemented and tested the change and drafted this note with my authorization. The validation below was performed by the agent; I am submitting the change for maintainer review.
AGENT:
Why
Blank text blocks and empty content arrays can cause strict Chat Completions endpoints to reject a conversation with HTTP 400.
Summary
Issue Number
Fixes #4965.
How to Test
From the repository root after
make build:.venv/bin/python .pr/repro_chat_content.py: 4/4 SDK → LiteLLM → loopback HTTP cases pass, preserving the original HTTP 400 → 200 correction..venv/bin/python .pr/repro_prompt_cache.py: 6/6 SDK completion calls retain cache markers in the actual Anthropic-compatible HTTP request. Covers empty and whitespace-only trailing blocks, mixed/wholly blank tool results, and mixed user content; input messages remain unchanged.uv run pytest tests/sdk/llm -q: 1037 passed, 17 warnings. The focused serialization and caching subset passes 136 tests. All ten added cache-regression cases failed before the correction.uv run pre-commit run --all-files --show-diff-on-failure: passed. Merged upstream76e9e250(fix(sdk): generate titles with Responses and subscription streaming #4968), resolving the previously documented baseline dynamic-attribute violations.Reproduction and validation · original before · original after · cache validation log
Video/Screenshots
CLI evidence is linked above; this change has no UI.
Design Doc
Small bug fix; reproduction and compatibility notes are linked above.
Type
Notes
The cache regression identified in review is fixed in
86199bdf. These local HTTP checks validate serialization, not live cache hits, cost, latency, or task performance. No real provider or benchmark was run; maintainer-triggered lightweight evals and human review remain outstanding. New upstream CI requires maintainer approval. No public API or persisted-schema change.