Repository navigation
Conversation
The cached client validity check was looking for `_process` directly on the client object, but Claude SDK stores the process at `client._transport._process`. This caused the check to always fail and fall through to the else branch which assumed the client was valid. This fix: - Corrects the path to `client._transport._process` - When process info is unavailable, removes client from cache instead of assuming it's valid (safer behavior) - Adds more detailed logging including exit_code and process_pid Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds persistent cancel tracking and changes Claude client lifecycle and cancellation handling: validates cached Claude SDK client processes, shifts cancellation checks ahead of residual-interrupt handling, replaces client disconnect with process termination, and preserves cancelled task state across cleanup until a task restarts. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant AgentService
participant TaskStateManager
participant ResponseProcessor
participant ClaudeClient
rect rgba(200,200,255,0.5)
Client->>AgentService: send_cancel_callback_async(task_id)
AgentService->>TaskStateManager: mark CANCELLED (persist in _cancelled_tasks)
AgentService-->>Client: acknowledge cancel
end
rect rgba(200,255,200,0.5)
ClaudeClient->>ResponseProcessor: stream result messages
ResponseProcessor->>TaskStateManager: is_cancelled(task_id)?
alt task cancelled
ResponseProcessor-->>ClaudeClient: stop processing -> return CANCELLED
else not cancelled
ResponseProcessor->>ClaudeClient: handle residual-interrupt / normal flow
end
end
rect rgba(255,200,200,0.5)
AgentService->>SessionManager: cleanup_task_clients(session_id)
SessionManager->>ClaudeClient: _terminate_client_process -> kill if necessary
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@executor/agents/claude_code/claude_code_agent.py`:
- Around line 604-611: Align the claude-agent-sdk version pins (resolve the
mismatch between executor/requirements.txt and executor/pyproject.toml) before
touching private internals; after aligning versions, replace or guard direct
access to cached_client._transport._process (used in claude_code_agent.py) by
either using a supported public API from the SDK or wrapping that access with a
stable compatibility helper, add a brief comment referencing the SDK
source/changelog location that documents _transport/_process structure, and add
a type annotation for process (e.g., process: Any = None) to clarify intent.
🧹 Nitpick comments (1)
executor/agents/claude_code/claude_code_agent.py (1)
613-643: Core fix logic looks correct — good improvement over the previous behavior.The three-way branching (terminated → remove, running → reuse, no info → remove) is sound. The previous code's assumption that an uncheckable client was valid was a real bug, and invalidating unknown-state clients is the right call.
One readability nit: in the terminated-process branch (lines 615–622) and the no-process-info branch (lines 637–643), the code relies on
self.clientstill beingNonefrom__init__to trigger new-client creation at line 652. This is correct but implicit. An explicitself.client = Noneafter eachremove_clientcall would make the intent immediately obvious and guard against future refactors that might setself.clientearlier.Optional: make the intent explicit
SessionManager.remove_client(self.session_id) + self.client = None # Proceed to create new clientSessionManager.remove_client(self.session_id) + self.client = None,
| process = None | ||
| if ( | ||
| hasattr(cached_client, "_transport") | ||
| and cached_client._transport | ||
| and hasattr(cached_client._transport, "_process") | ||
| and cached_client._transport._process | ||
| ): | ||
| process = cached_client._transport._process |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check for dependency specifications
find . -type f \( -name "requirements*.txt" -o -name "pyproject.toml" -o -name "setup.py" -o -name "setup.cfg" \) | head -20Repository: wecode-ai/Wegent
Length of output: 369
🏁 Script executed:
# Read the actual file to verify the code segment
cat -n executor/agents/claude_code/claude_code_agent.py | sed -n '600,650p'Repository: wecode-ai/Wegent
Length of output: 3179
🏁 Script executed:
# Check for any claude_agent_sdk version references
rg "claude_agent_sdk|claude-agent-sdk" --type py -B2 -A2Repository: wecode-ai/Wegent
Length of output: 5549
🏁 Script executed:
# Check the executor pyproject.toml for claude_agent_sdk version
cat executor/pyproject.toml | grep -A5 -B5 "claude"Repository: wecode-ai/Wegent
Length of output: 340
🏁 Script executed:
# Also check executor/requirements.txt
cat executor/requirements.txt | grep "claude"Repository: wecode-ai/Wegent
Length of output: 83
🏁 Script executed:
# Check the type hints used in the function to see if type annotations are used
cat -n executor/agents/claude_code/claude_code_agent.py | sed -n '450,470p'Repository: wecode-ai/Wegent
Length of output: 1165
🏁 Script executed:
# Check the function signature and surrounding context for type annotation patterns
cat -n executor/agents/claude_code/claude_code_agent.py | sed -n '440,460p'Repository: wecode-ai/Wegent
Length of output: 899
🏁 Script executed:
# Let me check what's actually in the pyproject.toml more carefully
cat executor/pyproject.tomlRepository: wecode-ai/Wegent
Length of output: 2452
🌐 Web query:
claude-agent-sdk 0.1.31 _transport _process documentation
💡 Result:
In claude-agent-sdk==0.1.31 (Python), modules named _transport / _process (leading underscore) are private, internal implementation details and aren’t part of the supported/public API, so there isn’t “official documentation” for them the way there is for query() / ClaudeSDKClient / ClaudeAgentOptions. The public docs focus on those supported entry points instead. [1], [2]
What you can do:
- Use the supported API:
claude_agent_sdk.query(...)for simple/streaming runs, orClaudeSDKClientfor interactive sessions, tools, and hooks. [1], [2], [3] - Read the source for internals (best-effort, may change between patch versions):
- Download the 0.1.31 sdist from PyPI and inspect the package code locally. [1]
- In the repo / installed package, the transport logic is implemented under internal paths such as
claude_agent_sdk/_internal/transport/...(for examplesubprocess_cli.py, referenced in bug reports). [4]
If you tell me what you’re trying to accomplish (e.g., custom transport, embedding, debugging a subprocess hang, replacing the bundled CLI, etc.), I can point to the exact internal files/classes involved and the least-fragile way to extend/patch it for 0.1.31.
🌐 Web query:
claude-agent-sdk changelog version 0.1.6 to 0.1.31 breaking changes
💡 Result:
First: which “claude-agent-sdk”?
- Python package (
pip install claude-agent-sdk): the latest visible release in the official GitHub releases feed is v0.1.29 (Feb 4, 2026)—there is no 0.1.31 shown there. [3] - TypeScript package (
npm install@anthropic-ai/claude-agent-sdk``) may have different versions; however, your version range 0.1.6 → 0.1.31 does not match the Python release list. [3]
Breaking changes from 0.1.6 upward (Python) — confirmed
1) CLI distribution / install requirements changed (potentially breaking operationally)
- In 0.1.6, the PyPI page lists Node.js as a prerequisite and instructs installing Claude Code via npm (
npm install -g@anthropic-ai/claude-code``). [1] - The current PyPI page states the Claude Code CLI is bundled with the package (no separate install), and you can optionally point to a custom CLI path via
ClaudeAgentOptions(cli_path=...). [2]
Why this can be “breaking” in practice: deployments that relied on a system/global CLI (or on Node being present, or on small wheels) may need changes (e.g., slimmer containers now pulling large wheels; different CLI selection/override behavior). [1][2]
What I can’t confirm from official sources here
I couldn’t access an official, complete per-version changelog that enumerates all breaking changes specifically between 0.1.6 and 0.1.31, and for Python specifically 0.1.31 doesn’t appear to exist in the official Python releases listing. [3]
If you tell me whether you mean Python or TypeScript, and (for TS) share the exact package name + a link to the changelog page you’re using, I can narrow this to a definitive “breaking changes only” list.
🏁 Script executed:
# Check if there are any other version pins or if this is a known configuration
cat executor/requirements.txt | head -20Repository: wecode-ai/Wegent
Length of output: 422
🏁 Script executed:
# Look for any build or CI configuration that might explain the version discrepancy
find . -type f \( -name "*.yml" -o -name "*.yaml" -o -name "*.md" \) | grep -i "build\|ci\|readme" | head -10Repository: wecode-ai/Wegent
Length of output: 324
Resolve the version mismatch in claude-agent-sdk dependencies before relying on private SDK internals.
executor/requirements.txt pins claude_agent_sdk==0.1.6 while executor/pyproject.toml pins claude-agent-sdk==0.1.31. This inconsistency must be resolved first, as accessing private internals like _transport._process is inherently fragile and will break across SDK versions. Once the versions are aligned, consider adding a brief comment linking to the SDK source or changelog that documents the structure being relied upon, since future maintainers will need to re-verify this if the SDK updates.
Minor: process could benefit from a type annotation (process: Any = None) for clarity, though the existing defensive hasattr checks and try-except wrapper provide appropriate mitigations.
🤖 Prompt for AI Agents
In `@executor/agents/claude_code/claude_code_agent.py` around lines 604 - 611,
Align the claude-agent-sdk version pins (resolve the mismatch between
executor/requirements.txt and executor/pyproject.toml) before touching private
internals; after aligning versions, replace or guard direct access to
cached_client._transport._process (used in claude_code_agent.py) by either using
a supported public API from the SDK or wrapping that access with a stable
compatibility helper, add a brief comment referencing the SDK source/changelog
location that documents _transport/_process structure, and add a type annotation
for process (e.g., process: Any = None) to clarify intent.
…ope error 1. Fix cancel scope error when closing client across different asyncio tasks - Use _terminate_client_process instead of disconnect() in SessionManager - This prevents RuntimeError when closing client from different task context 2. Fix process.poll() error for asyncio.subprocess.Process - Use process.returncode instead of poll() which is subprocess.Popen only - asyncio.subprocess.Process uses returncode attribute 3. Fix Docker mode resume issue after cancellation - Use cleanup_task_clients() instead of close_client(task_id) - task_id != session_id format, cleanup_task_clients handles all formats correctly 4. Fix RETRY_RESIDUAL issue after cancellation (方案1+2组合) - Add _cancelled_tasks set in TaskStateManager for persistent cancel tracking - Remove cleanup(task_id) call from send_cancel_callback_async - is_cancelled() now checks both state and _cancelled_tasks set - This prevents race condition where cleanup() runs before SDK returns result Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@executor/agents/claude_code/response_processor.py`:
- Around line 859-863: The logger.info call that logs "Detected residual
interrupt message (usage tokens all 0), this is likely from a previous cancelled
session. Returning None to continue processing." uses unnecessary f-string
prefixes; remove the leading f from each of the three string literals in the
logger.info invocation so they are plain strings (i.e., change f"..." to "..."
for the parts inside the logger.info call in response_processor.py).
- Around line 859-870: Log message and return value are inconsistent: the
logger.info inside the is_residual_interrupt branch states "Returning None"
while the code returns the string "RETRY_RESIDUAL"; update the code so the log
accurately reflects the action taken. Locate the is_residual_interrupt check
(variable is_residual_interrupt) and the logger.info call, and change the logged
text to indicate "Returning RETRY_RESIDUAL to resend query" (or alternatively
change the return to None if the intended behavior is to return None) so the
logger.info and the return "RETRY_RESIDUAL" are consistent.
In `@executor/tasks/task_state_manager.py`:
- Around line 50-52: _cancelled_tasks currently holds cancelled task IDs
indefinitely; change it to store timestamps (e.g., dict[int, float] mapping id
-> cancelled_at) or use a bounded/LRU structure and prune stale entries in
cleanup() by removing IDs older than a configurable grace period; keep removal
on the existing RUNNING transition logic (where IDs are evicted when a task
restarts) but add the timed eviction in cleanup() to prevent unbounded growth,
or implement a max-size eviction policy on _cancelled_tasks if timestamps are
unacceptable.
🧹 Nitpick comments (1)
executor/tasks/task_state_manager.py (1)
96-112:_cancelled_tasksis read outside_state_lock— consider a single lock acquisition.
get_stateacquires and releases_state_lock, thentask_id in self._cancelled_tasksis checked without the lock. While CPython's GIL makes this safe in practice, a single lock acquisition would be more correct:♻️ Suggested refactor
def is_cancelled(self, task_id: int) -> bool: - state = self.get_state(task_id) - return ( - state in [TaskState.CANCELLING, TaskState.CANCELLED] - or task_id in self._cancelled_tasks - ) + with self._state_lock: + state = self._states.get(task_id) + return ( + state in [TaskState.CANCELLING, TaskState.CANCELLED] + or task_id in self._cancelled_tasks + )
| logger.info( | ||
| f"Detected residual interrupt message (usage tokens all 0), " | ||
| f"this is likely from a previous cancelled session. " | ||
| f"Returning None to continue processing." | ||
| ) |
There was a problem hiding this comment.
Remove extraneous f prefixes on lines with no placeholders.
All three continuation strings are plain literals — the f prefix is unnecessary and flagged by Ruff (F541).
Proposed fix
logger.info(
- f"Detected residual interrupt message (usage tokens all 0), "
- f"this is likely from a previous cancelled session. "
- f"Returning None to continue processing."
+ "Detected residual interrupt message (usage tokens all 0), "
+ "this is likely from a previous cancelled session. "
+ "Returning None to continue processing."
)🧰 Tools
🪛 Ruff (0.14.14)
[error] 860-860: f-string without any placeholders
Remove extraneous f prefix
(F541)
[error] 861-861: f-string without any placeholders
Remove extraneous f prefix
(F541)
[error] 862-862: f-string without any placeholders
Remove extraneous f prefix
(F541)
🤖 Prompt for AI Agents
In `@executor/agents/claude_code/response_processor.py` around lines 859 - 863,
The logger.info call that logs "Detected residual interrupt message (usage
tokens all 0), this is likely from a previous cancelled session. Returning None
to continue processing." uses unnecessary f-string prefixes; remove the leading
f from each of the three string literals in the logger.info invocation so they
are plain strings (i.e., change f"..." to "..." for the parts inside the
logger.info call in response_processor.py).
| logger.info( | ||
| f"Detected residual interrupt message (usage tokens all 0), " | ||
| f"this is likely from a previous cancelled session. " | ||
| f"Returning None to continue processing." | ||
| ) | ||
|
|
||
| if is_residual_interrupt: | ||
| # Residual interrupt message detected, need to resend query | ||
| logger.info( | ||
| "Returning RETRY_RESIDUAL to resend query after residual interrupt" | ||
| ) | ||
| return "RETRY_RESIDUAL" |
There was a problem hiding this comment.
Log message says "Returning None" but the code returns "RETRY_RESIDUAL".
Line 862 logs "Returning None to continue processing" but the actual return at Line 870 is "RETRY_RESIDUAL". This is misleading for anyone reading logs during incident triage.
Proposed fix
logger.info(
"Detected residual interrupt message (usage tokens all 0), "
"this is likely from a previous cancelled session. "
- "Returning None to continue processing."
+ "Will return RETRY_RESIDUAL to resend query."
)🧰 Tools
🪛 Ruff (0.14.14)
[error] 860-860: f-string without any placeholders
Remove extraneous f prefix
(F541)
[error] 861-861: f-string without any placeholders
Remove extraneous f prefix
(F541)
[error] 862-862: f-string without any placeholders
Remove extraneous f prefix
(F541)
🤖 Prompt for AI Agents
In `@executor/agents/claude_code/response_processor.py` around lines 859 - 870,
Log message and return value are inconsistent: the logger.info inside the
is_residual_interrupt branch states "Returning None" while the code returns the
string "RETRY_RESIDUAL"; update the code so the log accurately reflects the
action taken. Locate the is_residual_interrupt check (variable
is_residual_interrupt) and the logger.info call, and change the logged text to
indicate "Returning RETRY_RESIDUAL to resend query" (or alternatively change the
return to None if the intended behavior is to return None) so the logger.info
and the return "RETRY_RESIDUAL" are consistent.
| cls._instance._cancelled_tasks: set[int] = ( | ||
| set() | ||
| ) # Track cancelled tasks even after cleanup |
There was a problem hiding this comment.
_cancelled_tasks can grow unboundedly for tasks that are never restarted.
Entries are only evicted when a task transitions back to RUNNING (Line 71). If a task is cancelled and never re-executed, its ID remains in the set indefinitely. For a long-running executor, consider periodically pruning stale entries (e.g., in cleanup() after a grace period) or bounding the set size.
🤖 Prompt for AI Agents
In `@executor/tasks/task_state_manager.py` around lines 50 - 52, _cancelled_tasks
currently holds cancelled task IDs indefinitely; change it to store timestamps
(e.g., dict[int, float] mapping id -> cancelled_at) or use a bounded/LRU
structure and prune stale entries in cleanup() by removing IDs older than a
configurable grace period; keep removal on the existing RUNNING transition logic
(where IDs are evicted when a task restarts) but add the timed eviction in
cleanup() to prevent unbounded growth, or implement a max-size eviction policy
on _cancelled_tasks if timestamps are unacceptable.
The cached client validity check was looking for
_processdirectly on the client object, but Claude SDK stores the process atclient._transport._process. This caused the check to always fail and fall through to the else branch which assumed the client was valid.This fix:
client._transport._processSummary by CodeRabbit
Bug Fixes
Improvements