Feat/rfc 0027 acp subagent zed - #45
Conversation
- 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
- 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
![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 implements RFC-0027 to introduce an experimental 'zed' subagent display mode, enabling compatibility with the Zed editor by tracking subagent session boundaries, message indices, and routing child subsessions. Feedback on the changes highlights a bug in the test fixtures where a missing child_session_id parameter causes snapshot tests to fall back to legacy mode, an indentation violation of PEP 8 in session.py, and an unidiomatic use of locals() for variable binding checks in session_manager.py.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| def _get_subagent_event( | ||
| source_name: str, | ||
| inner_event: Any, | ||
| source_type: str = "agent", | ||
| depth: int = 1, | ||
| ) -> SubAgentEvent: | ||
| """Wrap an event in a SubAgentEvent.""" | ||
| return SubAgentEvent( | ||
| source_name=source_name, | ||
| source_type=source_type, # type: ignore[arg-type] | ||
| event=inner_event, | ||
| depth=depth, | ||
| ) |
There was a problem hiding this comment.
The _get_subagent_event helper does not accept or forward a child_session_id parameter. Consequently, all SubAgentEvents generated by the Zed test fixtures have child_session_id set to None.
In event_converter.py, the "zed" mode match arm requires child_session_id to be truthy:
match self._display_mode:
case "zed" if child_session_id:Because child_session_id is missing/falsy in these test events, the converter falls back to legacy mode (_convert_subagent_legacy) for all SubAgentEvents in the snapshot tests. This means the snapshot tests are not actually testing the "zed" mode routing or parent-only fallback behavior for subagent progress/completion events, but are instead snapshotting the legacy fallback behavior.
To fix this, _get_subagent_event should accept and default child_session_id (e.g., to "child_sess_001" to match the spawn event), and pass it to SubAgentEvent.
| def _get_subagent_event( | |
| source_name: str, | |
| inner_event: Any, | |
| source_type: str = "agent", | |
| depth: int = 1, | |
| ) -> SubAgentEvent: | |
| """Wrap an event in a SubAgentEvent.""" | |
| return SubAgentEvent( | |
| source_name=source_name, | |
| source_type=source_type, # type: ignore[arg-type] | |
| event=inner_event, | |
| depth=depth, | |
| ) | |
| def _get_subagent_event( | |
| source_name: str, | |
| inner_event: Any, | |
| source_type: str = "agent", | |
| depth: int = 1, | |
| child_session_id: str = "child_sess_001", | |
| ) -> SubAgentEvent: | |
| """Wrap an event in a SubAgentEvent.""" | |
| return SubAgentEvent( | |
| source_name=source_name, | |
| source_type=source_type, | |
| event=inner_event, | |
| depth=depth, | |
| child_session_id=child_session_id, | |
| ) |
References
- Treat the use of '# type: ignore' in protocol implementation code as a 'code smell' that may indicate a semantic mismatch, such as an incorrect message direction or incompatible schema types.
| if isinstance(self.agent, Agent) and self.get_cwd_context in self.agent.sys_prompts.prompts: | ||
| self.agent.sys_prompts.prompts.remove( | ||
| self.get_cwd_context # pyright: ignore[reportArgumentType] | ||
| ) # ty: ignore[invalid-argument-type] |
There was a problem hiding this comment.
The indentation of the body of this combined if statement is incorrect (8 spaces instead of 4 spaces), violating PEP 8 guidelines.
| if isinstance(self.agent, Agent) and self.get_cwd_context in self.agent.sys_prompts.prompts: | |
| self.agent.sys_prompts.prompts.remove( | |
| self.get_cwd_context # pyright: ignore[reportArgumentType] | |
| ) # ty: ignore[invalid-argument-type] | |
| if isinstance(self.agent, Agent) and self.get_cwd_context in self.agent.sys_prompts.prompts: | |
| self.agent.sys_prompts.prompts.remove( | |
| self.get_cwd_context | |
| ) |
References
- PEP 8: Use 4 spaces per indentation level. (link)
| if "session" in locals() and session is not None: | ||
| with contextlib.suppress(Exception): | ||
| await session.close() |
There was a problem hiding this comment.
Using "session" in locals() to check if a variable is bound is unidiomatic in Python and relies on dynamic namespace inspection. A cleaner and more robust approach is to initialize session = None before the try block (above line 164), and then simply check if session is not None: in the except block.
| if "session" in locals() and session is not None: | |
| with contextlib.suppress(Exception): | |
| await session.close() | |
| if session is not None: | |
| with contextlib.suppress(Exception): | |
| await session.close() |
状态分析此 PR 是 #42 的旧版重复（同一功能的不同分支版本），功能已全部在主分支上实现。 具体情况
详见 #42 的分析评论。 结论功能已被主分支完全覆盖，关闭此 PR。 |
2 participants
No description provided.