docs(rfc): RFC-0027 ACP Subagent Zed Compatibility - #26
Closed
Leoyzen wants to merge 1 commit into
Closed
Conversation
12-round Oracle+Metis reviewed RFC for AgentPool ACP subagent compatibility with Zed editor. Key design: display_mode=zed as 4th enum value, _meta.subagent_session_info extension, Phase 1-3 implementation plan.
![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 RFC outlines a strategy to achieve compatibility between the AgentPool ACP Server and the Zed editor's subagent UI. It identifies key gaps in the current implementation—specifically the lack of _meta field population—and proposes a phased approach to introduce a zed display mode with child session management and message index tracking. Reviewers suggested adding Pydantic validation for message indices, optimizing the cleanup of subagent mappings, and adhering to documentation standards for rejected design options.
Comment on lines
+500
to
+510
| message_start_index: int | ||
| """子会话中该 turn 的起始条目索引（0-based）。 | ||
|
|
||
| 对应 Zed SubagentSessionInfo.message_start_index (usize, required)。 | ||
| """ | ||
|
|
||
| message_end_index: int | None = None | ||
| """子会话中该 turn 的结束条目索引（0-based）。 | ||
|
|
||
| 对应 Zed SubagentSessionInfo.message_end_index (Option<usize>)。 | ||
| """ |
There was a problem hiding this comment.
建议为 message_start_index 和 message_end_index 字段增加 ge=0 的约束。由于这些字段在 Zed (Rust) 中对应 usize 类型，负值在逻辑上是不合理的。通过 Pydantic 校验可以防止上游逻辑错误导致的异常数据。
Comment on lines
+530
to
+536
| if isinstance(raw, str): | ||
| # Fallback: 尝试解析 JSON 字符串（兼容错误格式） | ||
| try: | ||
| data = json.loads(raw) | ||
| return cls.model_validate(data) | ||
| except (json.JSONDecodeError, ValueError): | ||
| return None |
There was a problem hiding this comment.
建议将 from_meta 中对 str 类型的 fallback 解析标记为 'Rejected' 并附带决策引用，而不是直接移除。RFC 明确要求 _meta 中的 subagent_session_info 必须是 JSON 对象。根据仓库规则，在 RFC 中讨论并拒绝的设计选项应显式标记为 'Rejected' 以保留历史背景。
References
- When a design option discussed in an RFC is rejected, explicitly mark it as 'Rejected' with a reference to the decision, rather than removing it, to preserve historical context for future readers.
Comment on lines
+657
to
+666
| self._subagent_tool_map[child_id] = tc_id or f"spawn:{child_id}" | ||
|
|
||
| icon = "⚡" if mechanism == "spawn" else "🚀" | ||
| yield ToolCallStart( | ||
| tool_call_id=tc_id or f"spawn:{child_id}", | ||
| title=f"{icon} `{source_name}`: {description}", | ||
| kind="other", | ||
| status="in_progress", | ||
| field_meta=meta, | ||
| ) |
There was a problem hiding this comment.
建议将 tc_id or f"spawn:{child_id}" 的结果提取为局部变量。目前该逻辑在第 657 行和第 661 行重复出现，提取变量可以提高代码的可读性并确保 ID 的一致性。
Suggested change
| self._subagent_tool_map[child_id] = tc_id or f"spawn:{child_id}" | |
| icon = "⚡" if mechanism == "spawn" else "🚀" | |
| yield ToolCallStart( | |
| tool_call_id=tc_id or f"spawn:{child_id}", | |
| title=f"{icon} `{source_name}`: {description}", | |
| kind="other", | |
| status="in_progress", | |
| field_meta=meta, | |
| ) | |
| # 记录映射 | |
| tc_id = tc_id or f"spawn:{child_id}" | |
| self._subagent_tool_map[child_id] = tc_id | |
| icon = "⚡" if mechanism == "spawn" else "🚀" | |
| yield ToolCallStart( | |
| tool_call_id=tc_id, | |
| title=f"{icon} {source_name}: {description}", | |
| kind="other", | |
| status="in_progress", | |
| field_meta=meta, | |
| ) |
| # 在 convert() 方法的 SubAgentEvent case 分支内： | ||
| case SubAgentEvent(child_session_id=child_id, tool_call_id=_, event=inner_event) | ||
| if self._display_mode == "zed" and child_id in self._subagent_tool_map | ||
| and isinstance(inner_event, StreamCompleteEvent): |
There was a problem hiding this comment.
此处的 and 关键字缩进建议调整为 4 空格或与首个条件对齐，以符合 PEP 8 的代码风格。
References
- PEP 8 suggests using 4 spaces per indentation level or aligning with the opening delimiter for multi-line conditional statements. (link)
Comment on lines
+848
to
+850
| for child_id in list(self._subagent_tool_map.keys()): | ||
| # Phase 2: await self._session_manager.close_session(child_id) | ||
| del self._subagent_tool_map[child_id] |
There was a problem hiding this comment.
在 cleanup 方法中，目前的实现是遍历 key 并逐个 del。如果目的是清空字典，在循环结束后调用 self._subagent_tool_map.clear() 会更高效且代码更简洁。
5 tasks
Collaborator
状态分析此 PR 的内容已存在于主分支中，且 RFC-0027 描述的功能已完全实现。 具体情况
结论文档和实现均已完成，关闭此 PR。 |
Collaborator
Synthesized Review:
|
Collaborator
PR Review:
|
| Area | Verdict | Notes |
|---|---|---|
| Type safety | ✅ N/A | No source files changed |
| Testing | ✅ N/A | No test files changed |
| Telemetry | ✅ N/A | No instrumented code paths affected |
| Architecture | ✅ | No architecture concerns |
Finding: Zero Python files touched. No code concerns. ✓
Docs Review (review-docs)
| Area | Verdict | Notes |
|---|---|---|
| AGENTS.md context table | ✅ | Unaffected |
| Subsystem AGENTS.md | ✅ | Unaffected |
docs/explanation/ coverage / link integrity |
✅ | Unaffected |
| Heading hierarchy | ❌ Broken | README.md:209-210 — # test and # test review add two orphan H1s. The document's only legitimate H1 is # AgentPool (line 1); all sections use ##/###. |
| README cleanliness | ❌ | Test artifacts pollute user-facing documentation; second artifact compounds the pre-existing one |
Key Findings
- Trivial test change — aligns with commit message. All three trigger modes (pull_request, issue_comment, workflow_dispatch) verified.
- Heading hierarchy violation — Two H1 headings at the end of
README.mdbreak Markdown structure. Both# test(pre-existing from a prior test) and# test review(this PR) are structural errors. - README.md is not a test fixture — Using the project's primary landing page for trigger tests leaves garbage in a user-facing file.
Recommendations
- Close without merging once trigger verification is complete.
- Clean up both
README.md:209-210(# testand# test review) before any production merge tomain. - For future trigger tests, use a dedicated test branch that is never merged to
main, or a temp fixture file.
Collaborator
PR Review:
|
| Area | Verdict | Notes |
|---|---|---|
| Type safety | ✅ N/A | No .py files touched |
| Testing | ✅ N/A | No test infrastructure affected |
| Telemetry | ✅ N/A | No instrumented code paths touched |
| Architecture | ✅ N/A | No modules, protocols, or handlers changed |
No code risk. Trivial README-only change. No OpenSpec needed per AGENTS.md section 101–107 (not a significant change).
Docs Review (review-docs)
| Area | Verdict | Notes |
|---|---|---|
| AGENTS.md context table | ✅ Unaffected | README.md is correctly absent (it is the GitHub landing page, not contributor docs) |
| Subsystem AGENTS.md files | ✅ Unaffected | All 6 subsystem files untouched |
docs/explanation/ / link integrity |
✅ Unaffected | No docs/ files changed; no links added/removed |
| README heading hierarchy | ❌ Broken | Lines 209–210 add two H1s (# test, # test review) after the documents natural end (line 208). Only # AgentPool (line 1) is the legitimate H1 |
| Doc quality / artifact pollution | ❌ | Compounds pre-existing # test artifact (line 209) with another orphaned heading |
Key Findings
- Trivial CI trigger test — The change fulfills its stated purpose of verifying
pull_request,issue_comment, andworkflow_dispatchtrigger modes. All three modes have been exercised. - Heading hierarchy violated —
README.md:209-210:# test(pre-existing) and# test review(this PR) are orphaned H1s with no body content, violating the one-H1-per-document Markdown convention. - Compounded artifact problem — The pre-existing
# testonmainwas never cleaned up; this PR adds a second alongside it. - README.md is not a test fixture — Using the projects primary landing page for trigger tests leaves permanent garbage in a user-facing file.
Recommendations
- Close without merging once trigger verification is complete — the goal has been achieved and all three trigger modes work.
- Clean up both
README.md:209(# test) andREADME.md:210(# test review) in a follow-up PR before any production merge tomain. - For future trigger tests, use a dedicated branch that is never merged to
main, or write to a temporary fixture file instead ofREADME.md.
Collaborator
PR Review:
|
| Area | Verdict | Notes |
|---|---|---|
| Type safety | ✅ N/A | No .py files touched |
| Testing | ✅ N/A | No test infrastructure changed |
| Telemetry | ✅ N/A | No instrumented codepaths affected |
| Architecture | ✅ N/A | Trivial README-only change; no OpenSpec needed |
Findings: None. The diff is minimal, scoped, and the commit message matches the intent.
Docs Review (review-docs) — 🔴 2 findings
| Area | Verdict | Notes |
|---|---|---|
| AGENTS.md context table | ✅ Unaffected | README.md is correctly absent (user-facing landing page, not contributor docs) |
| Subsystem AGENTS.md files | ✅ Unaffected | All 6 subsystem files untouched |
docs/explanation/ / link integrity |
✅ Unaffected | No docs/ files touched; no links added/removed |
| README heading hierarchy | 🔴 Broken | Lines 209–210 add two orphaned H1s (# test, # test review) to a document whose only legitimate H1 is # AgentPool (line 1). The rest of the doc uses ##/###. H1 after H2 is a Markdown convention violation. |
| README cleanliness | 🔴 Artifact pollution | README.md is the public project homepage. Stray # test headings are debug remnants with zero informational value, and compound the pre-existing # test artifact on line 209. |
Key Findings
- Trivial CI trigger test — The change is well-scoped for its stated purpose.
- Heading hierarchy violated —
README.md:209-210are orphaned H1s appended after the document's natural conclusion under## Documentation. - Compounded artifact problem — The pre-existing
# test(line 209, from a prior trigger test) was never cleaned up; this PR adds# test reviewalongside it. - Pre-existing doc integrity issue (not caused by this PR):
AGENTS.md:79referencesdocs/explanation/team-mode.md, but that file does not exist on disk. Worth a follow-up fix.
Recommendations
- Close without merging once trigger verification is complete — the goal has been achieved.
- Remove both
README.md:209(# test) andREADME.md:210(# test review) before any production merge tomain. - For future trigger tests, use a dedicated branch never merged to
main, or write to a temp fixture file instead ofREADME.md. - Follow-up: Fix the stale
docs/explanation/team-mode.mdlink inAGENTS.md:79.
Collaborator
Synthesized Review:
|
| Area | Verdict | Notes |
|---|---|---|
| Type safety | ✅ PASS | No .py files changed |
| Testing | ✅ PASS | No test infrastructure affected |
| Telemetry | ✅ PASS | No instrumented code paths touched |
| Architecture | ✅ PASS | Trivial README-only change; OpenSpec bypass acceptable for CI test |
Docs Review — 🔴 2 findings
| Area | Verdict | Notes |
|---|---|---|
| AGENTS.md context table | ✅ Unaffected | |
| Subsystem AGENTS.md | ✅ Unaffected | |
docs/explanation/ / links |
✅ Unaffected | |
| Heading hierarchy | 🔴 Broken | Lines 209–210 add two orphaned H1s (# test pre-existing, # test review this PR) to a document whose only legitimate H1 is # AgentPool (line 1). The document consistently uses ##/### for sections. |
| README cleanliness | 🔴 Artifact pollution | Test headings have zero user value and compound pre-existing # test artifact |
Pre-existing: team-mode.md link |
AGENTS.md:78 references docs/explanation/team-mode.md which does not exist on disk (not introduced by this PR) |
Summary
- Trivial CI trigger test — commit message matches the change. Three trigger modes (pull_request, issue_comment, workflow_dispatch) have been exercised.
- Heading hierarchy violated — both
# testand# test revieware orphaned H1s at end of file after the document's natural conclusion. - Pre-existing broken link —
AGENTS.md:78→docs/explanation/team-mode.mdis a 404 on disk (follow-up fix needed).
Recommendations
- Close without merging once trigger verification is complete.
- Remove both
README.md:209(# test) andREADME.md:210(# test review) before any production merge tomain. - Fix stale link in
AGENTS.md:78(team-mode.mddoes not exist). - For future trigger tests, use a dedicated branch never merged to
main, or write to a temporary fixture file instead ofREADME.md.
Collaborator
Synthesized PR Review:
|
| Area | Verdict | Notes |
|---|---|---|
| Type safety | ✅ N/A | No .py files touched |
| Testing | ✅ N/A | No test infrastructure changed |
| Telemetry | ✅ N/A | No instrumented code paths affected |
| Architecture | ✅ N/A | Trivial README-only change; OpenSpec bypass is appropriate for this scope |
No code concerns. The commit message accurately describes intent — this is a CI trigger test.
Docs Review — 🔴 3 findings
| Area | Verdict | Notes |
|---|---|---|
| AGENTS.md context table | ✅ Unaffected | README.md not in the context table (correct — it is a user-facing landing page) |
| Subsystem AGENTS.md files | ✅ Unaffected | All 6 subsystem */AGENTS.md files untouched |
docs/explanation/ links |
✅ Unaffected | No docs/ files changed |
| README heading hierarchy | 🔴 HIGH | README.md:209-210: # test (pre-existing) and # test review (this PR) are orphaned H1s appended after the document's natural conclusion. Only # AgentPool (line 1) is the legitimate H1 — all sections use ##/###. |
| README cleanliness | 🔴 HIGH | User-facing landing page contaminated with debug headings that have zero informational value. Compounds pre-existing # test artifact. |
| Dead link (pre-existing) | 🔴 MEDIUM | AGENTS.md:78 references docs/explanation/team-mode.md — this file does not exist on disk. |
Key Findings
- Trivial CI trigger test — The change fulfills its stated purpose: exercising
pull_request,issue_comment, andworkflow_dispatchtrigger modes. - Heading hierarchy violated —
README.md:209-210: two extra H1s break Markdown convention (one H1 per document). - Compounded artifact problem — The pre-existing
# test(line 209, from a prior test onmain) was never cleaned up; this PR adds# test reviewalongside it. - Dead link in AGENTS.md (pre-existing) —
docs/explanation/team-mode.mdreferenced atAGENTS.md:78is missing from disk.
Recommendations
- Close without merging once trigger verification is complete — all three modes have been exercised.
- Remove both
README.md:209(# test) andREADME.md:210(# test review) before any production merge tomain. - For future trigger tests, use a dedicated branch never merged to
main, or write to a temporary fixture file instead ofREADME.md. - Follow-up: Fix the stale
team-mode.mdlink atAGENTS.md:78.
This was referenced Aug 22, 2026
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
2 participants
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.
Summary
Add RFC-0027: ACP Subagent Zed Compatibility — a 12-round Oracle+Metis reviewed RFC for AgentPool ACP subagent compatibility with Zed editor.
Key Design Decisions
display_mode=zedas 4th enum value (alongside legacy/inline/tool_box) — explicit config, no auto-detection_meta.subagent_session_infoextension on SpawnSessionStart's ToolCallStart — JSON Object, not stringGAP Analysis (5 gaps addressed)
_metanever populated on ToolCallStart/ToolCallProgressReview History
display_mode=zedTest Plan
Phase 1 includes 8+ test items covering: _meta presence/absence guards, SubagentSessionInfo model, ACP snapshot tests, StreamCompleteEvent field_meta, explicit drop behavior.