feat(rfc-0027): ACP subagent Zed compatibility - #42
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new 'zed' subagent display mode for Zed editor compatibility, implementing child ACP subsession creation, event routing, and message index tracking. Feedback on the changes highlights several critical issues: a missing return statement in LocalResourceProvider.get_skill, potential premature garbage collection of asyncio tasks stored in local variables, incomplete initialization of ACPEventConverter display modes, database connection leaks in zed_provider when exceptions occur, potential session ID mismatches during child session creation, and semantically incorrect usage of TypeError instead of ValueError for syntax validation.
| try: | ||
| skill = self._registry.get(name) | ||
| self._cache[name] = skill |
There was a problem hiding this comment.
The return skill statement was accidentally removed from the try block of get_skill. This causes the method to implicitly return None on success, violating the -> Skill return type annotation and causing runtime failures for any callers expecting a Skill object.
| try: | |
| skill = self._registry.get(name) | |
| self._cache[name] = skill | |
| try: | |
| skill = self._registry.get(name) | |
| self._cache[name] = skill | |
| return skill |
| child_converter = ACPEventConverter( | ||
| subagent_display_mode=self._display_mode | ||
| ) |
There was a problem hiding this comment.
Initializing child_converter with subagent_display_mode=self._display_mode is insufficient because _display_mode is a separate field that defaults to the environment variable value and is not automatically synchronized with subagent_display_mode during dataclass initialization. If the environment variable is not set to 'zed', the child converter will fall back to 'legacy' mode, breaking the Zed-compatible event routing. Pass _display_mode=self._display_mode explicitly to ensure the child converter runs in the correct mode.
child_converter = ACPEventConverter(
_display_mode=self._display_mode,
subagent_display_mode=self._display_mode,
)| converter = ACPEventConverter( | ||
| subagent_display_mode=self.subagent_display_mode, | ||
| _session_manager=self.manager, | ||
| _parent_session_id=self.session_id, | ||
| _client=self.client, | ||
| ) |
There was a problem hiding this comment.
When creating the main event converter, passing subagent_display_mode=self.subagent_display_mode does not automatically update the internal _display_mode field of ACPEventConverter if the environment variable ACP_SUBAGENT_DISPLAY_MODE is not set. To ensure the converter runs in the expected mode (especially 'zed'), pass _display_mode=self.subagent_display_mode explicitly.
| converter = ACPEventConverter( | |
| subagent_display_mode=self.subagent_display_mode, | |
| _session_manager=self.manager, | |
| _parent_session_id=self.session_id, | |
| _client=self.client, | |
| ) | |
| converter = ACPEventConverter( | |
| _display_mode=self.subagent_display_mode, | |
| subagent_display_mode=self.subagent_display_mode, | |
| _session_manager=self.manager, | |
| _parent_session_id=self.session_id, | |
| _client=self.client, | |
| ) |
| async def create_child_session( | ||
| self, | ||
| parent_session_id: str, | ||
| agent_name: str, | ||
| agent_type: str = "acp", | ||
| ) -> str: |
There was a problem hiding this comment.
If self._pool.sessions.create_child_session generates a new random session ID, it will mismatch with the child_session_id generated by the client/subagent and used in memory for event routing. Consider updating create_child_session to accept an optional child_session_id parameter so that the persistent session is created with the correct, matching ID.
async def create_child_session(
self,
parent_session_id: str,
agent_name: str,
agent_type: str = "acp",
child_session_id: str | None = None,
) -> str:
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces an experimental 'zed' display mode for subagents, allowing compatibility with the Zed editor by tracking child ACP subsessions and routing subagent events accordingly. Feedback on the changes highlights a potential mismatch issue where the returned child_session_id is ignored, and a garbage collection risk in local.py where background tasks are assigned to local variables that immediately go out of scope. Additionally, suggestions were provided to catch broader exceptions for safer graceful degradation during child session creation and to clean up completed child session entries from tracking maps to prevent memory accumulation.
| status="completed", | ||
| field_meta=meta, | ||
| ) | ||
| self._child_sessions.pop(child_session_id, None) |
There was a problem hiding this comment.
When a child session completes, we should also pop its entries from self._subagent_message_counts and self._subagent_tool_map to prevent memory accumulation of completed subagent states during a single long-running stream.
| self._child_sessions.pop(child_session_id, None) | |
| self._child_sessions.pop(child_session_id, None) | |
| self._subagent_message_counts.pop(child_session_id, None) | |
| self._subagent_tool_map.pop(child_session_id, None) |
Leoyzen
left a comment
Leoyzen
left a comment
There was a problem hiding this comment.
Review Comment Fixes
All review comments from the Gemini Code Assist review have been addressed in commit b79718e.
High Priority Fixes
-
local.py task_ref garbage collection ✅
- Added self._background_tasks set to LocalResourceProvider
- Both on_added and on_removed callbacks now store tasks and use add_done_callback
-
zed_provider.py database connection leak ✅
- Replaced manual conn.close() with try/finally blocks
- Connection guaranteed to close even if exceptions occur
-
event_converter.py _display_mode propagation ✅
- child_converter and main converter now receive _display_mode explicitly
-
event_converter.py child_session_id mismatch ✅
- create_child_session() return value captured as actual_child_session_id
- Actual ID used in ACPNotifications and _ChildSessionState
Medium Priority Fixes
-
event_converter.py exception handling ✅
- Changed to catch Exception broadly for safer graceful degradation
-
event_converter.py cleanup mappings ✅
- Child session completion now cleans up _subagent_message_counts and _subagent_tool_map
Additional Changes
- RFC Status Update: Moved RFC-0027 to implemented/ and updated status to IMPLEMENTED
Not Addressed
- base.py TypeError: Already reverted to ValueError in previous commit
- session_manager.py child_session_id parameter: Evaluated but not implemented - current design intentionally uses subagent-generated ID as mapping key while actual persisted ID is used for notifications
All 29 event converter tests pass.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements RFC-0027 Wave 3, introducing compatibility with the Zed editor by adding a "zed" subagent display mode. This mode supports creating independent ACP child subsessions, routing subagent events to these child sessions, and tracking message indices to communicate session boundaries to the client. Additionally, the PR cleans up SQLite connection handling in provider.py by ensuring connections are closed in finally blocks, and prevents garbage collection of background tasks in local.py.
Feedback on the PR highlights two main areas for improvement:
- In
session_manager.py, ifinitialize_mcp_servers()fails during session creation, the session's async environment remains entered, which can cause resource leaks. It is recommended to callsession.close()in theexceptblock. - In
event_converter.py, the fallback path forStreamCompleteEvent(when no child session exists) does not clean upchild_session_idfrom_subagent_tool_mapand_subagent_message_counts, potentially leading to memory accumulation. These should be popped upon completion.
| await session.initialize_mcp_servers() | ||
| self._active[session_id] = session | ||
| logger.info("Created ACP session", session_id=session_id, agent=session_agent.name) | ||
| return session_id | ||
| except Exception: | ||
| # Session creation failed - clean up the orphaned agent | ||
| try: | ||
| with contextlib.suppress(TypeError, AttributeError): | ||
| await acp_agent.remove_session_agent(session_id) | ||
| except (TypeError, AttributeError): | ||
| pass # Mock agent, no cleanup needed | ||
| raise | ||
| else: | ||
| return session_id |
There was a problem hiding this comment.
If session.initialize_mcp_servers() fails after session.initialize() has succeeded, the session's async environment (self.acp_env) is left entered, which can cause resource leaks. Ensure that session.close() is called on failure if the session object was successfully instantiated.
| await session.initialize_mcp_servers() | |
| self._active[session_id] = session | |
| logger.info("Created ACP session", session_id=session_id, agent=session_agent.name) | |
| return session_id | |
| except Exception: | |
| # Session creation failed - clean up the orphaned agent | |
| try: | |
| with contextlib.suppress(TypeError, AttributeError): | |
| await acp_agent.remove_session_agent(session_id) | |
| except (TypeError, AttributeError): | |
| pass # Mock agent, no cleanup needed | |
| raise | |
| else: | |
| return session_id | |
| await session.initialize_mcp_servers() | |
| self._active[session_id] = session | |
| logger.info("Created ACP session", session_id=session_id, agent=session_agent.name) | |
| except Exception: | |
| # Session creation failed - clean up the orphaned agent | |
| if "session" in locals() and session is not None: | |
| with contextlib.suppress(Exception): | |
| await session.close() | |
| with contextlib.suppress(TypeError, AttributeError): | |
| await acp_agent.remove_session_agent(session_id) | |
| raise | |
| else: | |
| return session_id |
| case StreamCompleteEvent(): | ||
| count = self._subagent_message_counts.get( | ||
| child_session_id, 0 | ||
| ) | ||
| message_end_index = ( | ||
| count - 1 if count > 0 else None | ||
| ) | ||
| meta = self._build_subagent_field_meta( | ||
| child_session_id, | ||
| tool_call_id, | ||
| message_end_index=message_end_index, | ||
| ) | ||
| yield ToolCallProgress( | ||
| tool_call_id=tool_call_id, | ||
| status="completed", | ||
| field_meta=meta, | ||
| ) |
There was a problem hiding this comment.
In the fallback path (when no child session exists), once StreamCompleteEvent is processed, the subagent run is complete. However, the child_session_id is not popped from self._subagent_tool_map and self._subagent_message_counts. This can lead to memory accumulation and potential state desync. Pop them from both dictionaries to ensure consistency with the child session path.
case StreamCompleteEvent():
count = self._subagent_message_counts.get(
child_session_id, 0
)
message_end_index = (
count - 1 if count > 0 else None
)
meta = self._build_subagent_field_meta(
child_session_id,
tool_call_id,
message_end_index=message_end_index,
)
self._subagent_message_counts.pop(child_session_id, None)
self._subagent_tool_map.pop(child_session_id, None)
yield ToolCallProgress(
tool_call_id=tool_call_id,
status="completed",
field_meta=meta,
)
- SpawnSessionStart in zed mode now generates a NEW tool_call_id instead of reusing the PydanticAI-native tool_call_id - This creates a distinct tool call bearing _meta.subagent_session_info - Zed can recognize this as a subagent and load the child session - Original PydanticAI tool call remains untouched Related Zed-side change: dynamic subagent loading via EntryUpdated
…e providers - Reverted accidental deletion of get_skills() from ResourceProvider base class which broke 18 callers across agent_routes, command_registry, aggregating, etc. - Restored missing return skill in LocalResourceProvider.get_skill() Fixes: serve-acp config loading error "Failed to import class"
…fications) Reverted 50 files that contained: - ruff auto-fix (import sorting, line length, TYPE_CHECKING moves) - Exception type changes in tools/base.py (ValueError -> TypeError) - Import removals in provider_router.py - Various formatting and style changes Kept only RFC-0027 core files: - acp_server/ event_converter, session, session_manager, server, acp_agent - pool_server config, serve_acp CLI - Tests and snapshots - resource_providers base.py and local.py (get_skills fix)
- Fix local.py task_ref garbage collection with _background_tasks set - Fix zed_provider.py database connection leaks with finally blocks - Fix event_converter.py child_session_id mismatch using returned ID - Fix event_converter.py exception handling to catch Exception broadly - Fix event_converter.py cleanup _subagent_message_counts and _subagent_tool_map on completion - Fix session.py and event_converter.py _display_mode propagation - Move RFC-0027 to implemented status
b79718e to
e612edf
Compare
- fix(session_manager): add session.close() on initialization failure to prevent resource leaks - feat(session_manager): allow passing explicit child_session_id to create_child_session for ID consistency - fix(event_converter): clean up subagent state on StreamCompleteEvent fallback path to prevent memory accumulation - fix(sessions/manager): accept optional child_session_id in create_child_session
Review Comments 修复总结已针对未解决的 review comments 提交修复（ 1.
|
状态分析此 PR 的功能已全部在主分支上实现，PR 已过时。 具体情况PR #42 实现 RFC-0027 ACP subagent Zed 兼容性，共 8 项核心变更。当前主分支已包含所有功能：
原因2026-06-30 合并的 PR #65（"refactor: eliminate pool-level agents, introduce Run/Turn separation, and overhaul EventBus + ACP session lifecycle"）在大规模重构中直接将 Zed 兼容性功能整合进了新架构，并扩展了 结论功能已被主分支完全覆盖，关闭此 PR。 |
2 participants
Summary
RFC-0027 implementation: ACP subagent Zed compatibility support.
Changes
Bug fixes in event_converter.py
_current_message_idfield declarationreset()bodyreset()call on StreamCompleteEventZed mode type propagation
"zed"tosubagent_display_modeacross 7 files (server, CLI, config, session, agent)SubagentSessionInfo model and helpers
SubagentSessionInfoPydantic model withsession_id,message_start_index,message_end_index_build_subagent_field_meta()helper for constructing_metapayload_metafilling in zed modeSpawnSessionStarthandler emitsToolCallStartwith_meta.subagent_session_infoandtool_name="task"SubAgentEventhandler routes inner events and emitsToolCallProgresswith_meta_metanever leaks in non-zed modes (legacy/inline/tool_box)Child session creation and routing
SpawnSessionStartcreates independent ACP subsession viasession_manager.create_child_session()SubAgentEventinner events routed to child session's event loopStreamCompleteEventcloses child session and emits parent completionMessage index tracking
message_start_index=0on SpawnSessionStartmessage_end_index=count-1on StreamCompleteEventIndependent tool_call_id for zed subagent
_meta.subagent_session_infoTests and snapshots
Verification
pytest tests/test_event_converter.py- 29/29 passpytest tests/test_acp_event_converter_snapshots.py::TestZedModeSnapshots- 4/4 passRelated
Zed-side change required for full functionality: dynamic subagent loading via
EntryUpdatedevent handling (separate PR).