fix(handoffs): preserve message name when nesting handoff history - #4300
fix(handoffs): preserve message name when nesting handoff history#4300hsusul wants to merge 1 commit into
Conversation
The compact "role (name): content" summary line used for nested handoff
history cannot be reversed when the role or name contains the delimiters
the flattening parser splits on. A ":" is consumed by the "role: content"
split and "("/")" confuse the "(name)" extraction, so on the next handoff
a message whose name contained those characters was silently corrupted
(e.g. name "a:b" turned into role "assistant (a" with content "b): ...").
Route such items to the existing JSON encoding, which round-trips
losslessly, leaving the readable compact form for names without those
delimiters.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 060e8b2beb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| name = item.get("name") | ||
| if isinstance(name, str) and any(char in name for char in _LEGACY_SUMMARY_DELIMITERS): | ||
| return False |
There was a problem hiding this comment.
When a named message has leading or trailing whitespace (for example, "name": " bob "), this predicate classifies the compact representation as reversible, but _split_role_and_name() strips that whitespace while parsing, so the next handoff silently changes the name to "bob"; a whitespace-only name corrupts the role to "assistant ( )". Treat names that differ from name.strip() as non-reversible and use the JSON path, with a round-trip test for this supported string edge case.
AGENTS.md reference: AGENTS.md:L70-L70
Useful? React with 👍 / 👎.
|
Thanks for the careful investigation and focused patch. I confirmed the direct helper round-trip failure, but the PR does not establish a supported runtime path for the affected input. TResponseInputItem uses the Responses input-item schema, whose message shapes do not include name, and the Chat Completions converter rejects the same named message before it can reach nested handoff handling. The regression test therefore demonstrates a cast-only helper case, not user-visible SDK impact. I am going to close this PR for now. If you can provide an end-to-end reproduction using a supported Runner input and provider path that accepts a message name containing these delimiters and demonstrates the downstream effect, we can revisit the narrow serialization fix. |
2 participants
Summary
When
nest_handoff_history(nested handoff history) summarizes the previous transcript, each turn is rendered as a compactrole (name): contentline. On a subsequent handoff the summary is flattened back into structured items by_extract_nested_history_transcript, which reverses that format.That compact form is ambiguous when the
roleor the messagenamecontains one of the characters the parser splits on::is consumed by therole: contentsplit.(/)confuse the(name)extraction (which usesrfind("(")).So a message whose
namecontained those characters was silently corrupted on the next handoff. For example, with{"role": "assistant", "name": "a:b", "content": "answer here"}:{"role": "assistant (a", "content": "b): answer here"}— the role, name, and content are all wrongThe SDK already renders items losslessly via the JSON encoding used for content that contains newlines. This change routes items to that JSON encoding whenever the compact line would not be reversible (role/name containing
:,(,),\n, or\r), and keeps the readable compact form for everything else (e.g. a plainnamelike"bob", which is unaffected).The fix is confined to
_format_transcript_item's legacy-vs-JSON choice; parsing is unchanged. The JSON path is already exercised by existing tests, and the change is idempotent across repeated handoffs.Test plan
Added
test_nested_history_preserves_message_name, parametrized over namesbob,O(1),a:b,foo(bar),x)y. It nests a 3-turn history whose middle message carries thename, then re-flattens across several handoffs and asserts the transcript is unchanged each time.mainfor thecolon,parens, andtrailing_callcases (role/name/content corrupted); passes with this change.Commands run (from a clean
upstream/mainworktree):uv run pytest tests/test_handoff_history_duplication.py -q→ 97 passeduv run pytest tests/test_handoff_history_duplication.py tests/test_run_internal_items.py tests/test_handoff_tool.py tests/test_run_step_processing.py -q→ 198 passeduv run ruff check src/agents/handoffs/history.py tests/test_handoff_history_duplication.py→ All checks passeduv run ruff format --check …→ already formatteduv run pyright src/agents/handoffs/history.py→ 0 errorsTYPECHECK_SRC_ONLY=1 uv run mypy src --exclude site→ no new errors inhandoffs/history.py; the 44 reported errors are pre-existing onmainin unrelated optional-dependency modules (extensions/sandbox/*,extensions/models/litellm_model.py,extensions/memory/sqlalchemy_session.py,voice/imports.py, etc.).Issue number
N/A
Checks
.agents/skills/code-change-verification/scripts/run.shrun.sh)/reviewbefore submitting this PR