fix(templates): read long stream-json lines and bound Claude Code shutdown - #542
Open
michaelxu2288 wants to merge 1 commit into
Open
michaelxu2288 wants to merge 1 commit into
michaelxu2288 wants to merge 1 commit into
Conversation
…tdown The Claude Code scaffolds (sync, async and Temporal) and their tutorial copies read `claude -p --output-format stream-json` stdout through asyncio's StreamReader with the default 64 KiB line limit. Claude Code writes one JSON line per event, so a tool_result that echoes a large file read is a single line well past 64 KiB; readline() then raises "Separator is found, but chunk is longer than limit" and the turn aborts. Read with an 8 MiB limit, the same value `agentex agents run` uses for its child processes. Their cleanup also sent SIGTERM and then awaited proc.wait() with no bound, so a CLI that ignores or delays SIGTERM hangs request or activity cancellation. Wait at most 5 s, then SIGKILL. Verified with a fake `claude` on PATH, against the rendered sync and Temporal templates: - a 100,000-character line: main raises the ValueError, this branch reads it whole; - a CLI that ignores SIGTERM: main's aclose() is still hanging after 20 s, this branch returns after 5 s. tests/lib/cli/test_init_templates.py passes, and the three tutorials' test_agent_offline.py suites pass (7, 5 and 5 tests).
Comment on lines
131
to
+137
| proc.terminate() | ||
| except ProcessLookupError: | ||
| pass | ||
| await proc.wait() | ||
| try: | ||
| await asyncio.wait_for(proc.wait(), timeout=TERMINATE_TIMEOUT_SECONDS) | ||
| except TimeoutError: | ||
| try: |
There was a problem hiding this comment.
The new five-second shutdown path and the larger line limit have no test that runs a subprocess. The template test only checks Python syntax, and the tutorial tests replace _spawn_claude with fake lines. Please add a subprocess test with a long JSON line and a child that ignores SIGTERM, so a later change cannot silently bring back either bug.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agentex/lib/cli/templates/sync-claude-code/project/acp.py.j2
Line: 131-137
Comment:
**Subprocess fixes lack tests**
The new five-second shutdown path and the larger line limit have no test that runs a subprocess. The template test only checks Python syntax, and the tutorial tests replace `_spawn_claude` with fake lines. Please add a subprocess test with a long JSON line and a child that ignores SIGTERM, so a later change cannot silently bring back either bug.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
This branch has not been deployed
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.
Problem
The Claude Code scaffolds (
sync-claude-code,default-claude-code,temporal-claude-code) and their tutorial copies (00_sync/060,10_async/00_base/130,10_async/10_temporal/140) share two bugs in how they read the CLI.1. Lines over 64 KiB abort the turn. stdout is read through asyncio's
StreamReaderwith its default 64 KiB limit (async for chunk in proc.stdoutisreadline()underneath). Claude Code writes one JSON line per event, so atool_resultthat echoes a large file read is a single line far past 64 KiB.readline()then raisesValueError: Separator is found, but chunk is longer than limit, and the turn fails before itsresultevent. The same class of bug was already fixed foragentex agents run(SUBPROCESS_STREAM_LIMITincli_utils.py).2. Cancellation can hang. Cleanup sends SIGTERM and then awaits
proc.wait()with no bound. A CLI that ignores or delays SIGTERM blocks request cancellation (sync/async ACP) or Temporal activity cancellation indefinitely.Fix
In each of the six files:
limit=STDOUT_LINE_LIMIT(8 MiB);TERMINATE_TIMEOUT_SECONDS(5 s), then SIGKILL;_spawn_claude's docstring.Nothing else changes. The Codex templates read with
read(4096)and usekill(), so neither bug applies to them. The Gemini CLI templates in #516 got the same treatment.Verification
I put a fake
claudeonPATHand drove_spawn_claude()from the rendered sync and Temporal templates.mainValueError: Separator is found, but chunk is longer than limitaclose()tests/lib/cli/test_init_templates.py: 26 passed.tests/test_agent_offline.py: 7, 5 and 5 passed.ruff checkis clean on the tutorial files, and all three compile.The PR appears safe to merge, though a subprocess test would help keep both fixes working.
Fix with agent prompt
Summary
This PR updates the Claude Code scaffolds and tutorial copies to read stream-json lines up to 8 MiB and to bound subprocess shutdown. It addresses large tool-result lines and cleanup when a caller stops reading.
Reviews (1) · Last reviewed commit: "fix(templates): read long stream-json li..."