Repository navigation
feat(acp): connect durable team reports and session approvals [13902 B/3] - #15658
yiliang114 wants to merge 19 commits into
Conversation
…e budget The web-shell E2E smoke job failed only on web-shell.history-viewport.spec.ts:190 > "global turn navigation preserves the reading row across bounded 200-record pages" with "Test timeout of 120000ms exceeded" on all three attempts (job 121686839091), 367/368 otherwise green. Main now carries the 300s per-case budget for that throttled fixture (067090d, #15656), which this branch predates.
… repair/pr15658-mainmerge-20261027 # Conflicts: # packages/cli/src/acp-integration/session/Session.ts
|
Merged this layer's base branch and resolved the one conflict; the stacked diff stays its own 29 files / +3,110. What was merged (ordinary merge commits; no rebase, no force):
Conflict (1 file): Why the main merge was reverted: while the base still sits at 45e7289, main's 16 newer commits land inside this PR's three-dot diff — 199 files / +23,978, past GitHub's 20,000-line render cap ( Also dropped one stray blank line prettier rejects in Verification (local, on the merged tree): full build pass · Head: 435e4f9 → 05300de. Draft state and base branch unchanged. |
Two replay findings from the #13902 review, both about trusting the persisted record too much: - `executionMode` alone was treated as proof the subagent started, but two rejection paths persist it for a session that never registered: worktree-provisioning failure (`status: 'failed'`) and the foreground model-slot cancel (`status: 'cancelled'`). Require `subagentSessionReady === true` on that leg, which is the field current writers flip once the subagent session exists; fixtures modelling a genuine run-then-fail now carry it. - the rejection branch was a negative allowlist gated on `typeof status === 'string'`, so a truncated or older record with an absent/non-string status minted an identity the live wire never stamped. Replace it with a positive allowlist over completed/running/background/ cancelled. New cases: both rejection-shaped displays stay unattributed, and absent / non-string / null status stays unattributed. Dropping the session-ready requirement reddens the first two; restoring the negative gate reddens the status cases.
… split/13902-team-coordination
… split/13902-team-coordination # Conflicts: # packages/cli/src/acp-integration/acpAgent.test.ts # packages/cli/src/acp-integration/session/Session.test.ts # packages/cli/src/acp-integration/session/Session.ts
|
Main sync deliberately not carried by this layer, and reverted with an explicit commit ( Why: merging the current What that means: adapting those two tests to main's rewind contract (promptId/cut-point instead of a user-turn count) is real work that should not be smuggled into the split PR. This layer therefore stays one Next step for this layer: re-stack on A once A lands, then do the rewind-contract adaptation in one focused commit with the two tests as the acceptance. |
All six are review feedback on the split B layer, not new scope: - R8-3: the report body is `TeamManager.formatLeaderEnvelope`'s markup, so blanket XML-escaping it entity-encoded the framing and every comparison while the TUI, stream-json and task-list paths deliver the same string verbatim. Defang only the envelope's own delimiters (`peer-envelope.ts`); the roster notice composes bare prose, so it keeps the blanket escape. - R9-2: one `cleanup()` publishes up to ten exits and each queued peer item drained as its own automatic leader turn. Core now flags teardown exits and the CLI coalesces the synchronous burst into one notification; the per-teammate events and approval retirements are unchanged. - R9-5: `PendingTeammatePermission` sat between the class JSDoc and the class, so the trust-model doc attached to the interface. Moved above it. - R9-7: the suite modelled `requestPermission` as pending on abort, while the real collaborator rejects. New case drives the rejection and pins the single Cancel with no fabricated leader notice. - R9-8: `TEAMMATE_EXITED` was the only retraction without a post-detach assertion. The detach test now pins listener count 0 and unchanged retirement/roster counts. - R9-9: `resolvePermissionOutcome` ran outside the answered-guard, so an answer naming a stripped option produced a Cancel reported as a host failure. It now cancels without the false leader notice. Mutation-checked: dropping the abort guard, the exit retraction, the coalescing, the answered-guard placement, or restoring the blanket escape each reddens exactly its own case. R9-5 is pinned with the TypeScript parser (interface and class each carry their own doc).
|
All six inherited findings are fixed at
Verification at Stacked on A, so the diff is still this layer only. The PR remains a draft; three findings remain on C (R9-1, R9-6) and one new |
|
One current reviewability regression on the replacement stack, observed from GitHub's native PR API (not a local build or a claim about runtime behavior): At #15658 HEAD This may be three-dot merge-base/stack drift rather than 460 intended changes; I have not rebuilt the branches or reproduced the underlying code failure. Before treating B as independently reviewable, could the stack's merge base / ancestry relative to the current A HEAD be reconciled and the GitHub Files changed view checked for the intended B-only scope? The existing main-rewind and generated-import test issues still need their separate source-specific repair; a broad main merge or an unverified CI/test claim would not close them. Keeping the A → B → C review scope and head-bound verification distinct should make the maintainer's acceptance decision easier. |
Managed shutdown cancelled a delivery turn with `void` and flushed the queue right after. A cancel re-queues the in-flight report on a later microtask, so the synchronous flush iterated a queue the report had already left, and the re-queued landing then hit a writer that the following loop had closed — `appendRecord` is a silent no-op there, yet the item still latched `persisted = true`, while the mailbox row was already marked read. The per-session cancel now settles first and the flush rides that promise, so the ordering is cancel → flush → close writers. The new test holds the cancel open and asserts neither the flush nor the writer close runs until it settles.
…dination Rebuilds the merged tree from main plus this layer's own commits rather than accepting the auto-merge result: earlier merges on this branch had resolved by dropping upstream files and hunks wholesale, leaving ~21k lines of base content deleted (40 files entirely). The tree here is main + the layer's 32 files, so the PR diff shows the layer only. A/3 (#15657) landed on main as 4dd7ac7, so retargeting to main keeps this layer's own surface. The rewind regression test now uses main's per-prompt cut points, which replaced the rewindable-turn count (#15441).
|
Conflict/base sync — and a repair of pre-existing branch damage Merged The branch was already corrupted before this sync. At So the tree was rebuilt as main + this layer's own commits instead of accepting the auto-merge result. Result: CONFLICTING → MERGEABLE, 32 files, +3,458 / −146. (While retargeted but unpushed it transiently read 468 files / +8,307 / −51,745.) Base-merge conflicts (2, both Main-merge conflicts (5): Adapted to main: Concurrent work: Verification (local, merged tree): Two cli failures are pre-existing, not from this sync: Left to CI: Lint & Static, Test (ubuntu/windows/macos), Integration (no-AK), TUI parity, OpenTUI no-flicker, web-shell visuals and E2E smoke. Kept as a draft. |
The writer-close loop invoked closeSessionWriter synchronously while beginManagedShutdown was still collecting the per-session cancel/flush promises, so the writer could close before the flush riding the cancel promise landed. Defer the loop into the allSettled(peerFlushes) continuation so the ordering is cancel -> flush -> close.
|
Repair: the writer close raced the peer flush — draft head Both failures reproduced on the author's own head Root cause — confirmed, not refuted. In Fix — move the writer-close loop into the Verification at
Still CI-owned / residual risk
Draft kept; no review requests, thread resolution, or dismissals. |
|
Recording the one red on this head, which is not this diff's:
That is the export-renderer budget, and it is now breached on main-derived trees: the sibling layer #15659 hits the same wall at 2,050,452, and this PR at 2,050,117. The fix is #15778, which emits the renderer bundle as literal UTF-8 instead of Everything else is green on For the record on the fix itself: the shutdown ordering bug was real and pre-dated the merge that landed this branch. |
|
The two red lanes here are main-side, not this PR.
Attribution （判 ② / main, not ①本 PR):
Flagged to infra-CI in OPT with the two choices （瘦身 transcript 条目真正用到的 Generated Ui 项 / 测量后 deliberate 抬预算，参照 #12298），main 侧处理，不塞进本 PR. |
|
Follow-up to my earlier 460-file reviewability finding on B/3: I re-read the current native PR metadata and complete This is a scoped native-source verification, not a code/runtime approval or a claim all CI is green. I also verified #15657 is merged ( |
|
One remaining shutdown intake/quiescence acceptance case on B/3 HEAD
Suggested red/green fixture: Hold This is a current-head edge to verify, not a claim your earlier writer-order fix failed, not CI approval, and not a reason to reopen the superseded #13902. |
|
Correction / consolidation: my earlier, more specific B/3 source review already contains this same late TeamManager inbox-callback acceptance case: #15658 (comment) . Please treat that earlier comment as the sole actionable test proposal. This later message is a duplicate and requests no additional investigation or response. I am consolidating here to avoid review noise. |
🖼️ web-shell visual previewRendered against a mock daemon (no real backend): the PR base vs this PR head Screenshots · before / afterℹ️ No screenshot changed against the PR base — but this PR edits 3 render-shaping files:
Either the change has no visual effect (logic, plumbing, a state the scenarios never reach), or no scenario renders this UI — in which case the preview cannot see it, and an empty result is a coverage gap rather than a clean bill of health. To make it visible, add a scenario to Full-resolution recordings (.webm) are attached to the workflow run. — Qwen Code · web-shell visuals |
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
Draft PR. Review is informational; approval withheld pending the major finding and execution gap below.
Tier: Deep — crosses queue, recording, bridge and shutdown boundaries; persists new fields (backgroundTask.agent); contains permission/trust handling and async lifecycle.
Scope: source files only (15 files reviewed). NOT reviewed: test-file correctness as a subject; Windows/Linux runtime behavior; TypeScript standalone typecheck; PermissionMediator internals (not in diff).
Findings
R1-1 · Major · supported — Shutdown quiescence: late TeamManager inbox poll can land to a closed writer
Mechanism: acpAgent.ts managed shutdown flushes peer notifications once (flushPendingPeerNotifications()) and then awaits writer-terminal promises. During the awaited writer-terminal chain, the TeamManager 500 ms poll timer (pollLeaderInbox, not touched by this diff) can fire, consume the mailbox (marking rows read), and invoke the leader-message callback. Session.#enqueueTeammateMessage rejects only this.disposed, which is still false at this point — so the late report is enqueued as a new peer item with persisted: false. The writer has since closed; dispose() runs later and calls #landUndrainedPeerNotifications(), which calls recording.recordNotification() — a no-op after writer close. The report is permanently lost while already marked read in the on-disk mailbox.
Chain:
acpAgent.ts~4240–4270: single peer flush sequenced before but not after writer terminalsTeamManager.ts~1194–1199 (not in diff): 500 ms poll; ~1226–1257: async inbox consume → callbackSession.ts#enqueueTeammateMessage(~12704):if (this.disposed) returnis the only guardSession.tsdispose()(~5273):detachTeamSink()→#landUndrainedPeerNotifications()on a closed writer
Nakagawa-master filed this same race on the prior head; the writer-close ordering fix since then did not add an intake quiescence barrier. A second flushPendingPeerNotifications() after all writers close (or an awaited in-flight poll drain before writer start) would close the window.
R1-2 · Question for author — mediator.vote(CANCEL_VOTE_SENTINEL) on an in-flight mediator.request()
In bridgeClient.ts (~line 1219), when a binding retires while mediator.request() is already awaited:
const resolution = this.mediator.request(record, this.permissionTimeoutMs);
// ... check: bindingId <= retiredThrough ...
this.mediator.vote({ ..., optionId: CANCEL_VOTE_SENTINEL, ... });
return resolutionToAcpResponse(await resolution);Does mediator.vote(CANCEL_VOTE_SENTINEL) on a request started on the same tick cause await resolution to resolve as cancelled? The same pattern in the teammate-binding-ended handler votes on already-pending entries. If the mediator resolves its promise only when a vote arrives via vote(), this is correct. If not, await resolution hangs until the configured timeout. Could you point to the resolution path in PermissionMediator?
R1-3 · Minor — Batched teardown exits in #pendingTeardownExits may be lost on fast dispose
#enqueueTeamRosterChange batches teardown exits in #pendingTeardownExits and schedules queueMicrotask(() => #flushTeardownRosterExits()). If dispose() fires before the microtask, #flushTeardownRosterExits guards if (this.disposed) return and drops the batch — exits never reach notificationQueue and skip #landUndrainedPeerNotifications. Practical impact is bounded (session is tearing down simultaneously; no recording is needed post-dispose), but a client reconstructing roster from events would hold a stale roster after a session snapshot taken during teardown.
Coverage
Checked:
- Contract between
persistedNotificationAgent()writer andisTeammateAgentRecord()reader — kind='teammate' projection only on both sides; subagent identity uses a separate replay path. Consistent. defangPeerNotificationTagsregex: lookahead\s*\/?\s*peer-notification(?![-\w])correctly matches both opening and closing tags;iflag covers casing. Distinction betweenescapeXml(roster strings) anddefangPeerNotificationTags(formatted envelope) is correct.attachTeamSinksame-instance re-notification guard (if (boundManager === manager) return): prevents duplicate listener registration.bindingController.abort()→AbortSignal.any(...): abort propagates correctly to in-flight approval; catch guardsif (signal.aborted) return. No double-respond path visible.- Binding epoch monotonicity: module-level
nextBindingIdgives increasing IDs per process; per-sessionWeakMapkeeps epoch tracking session-local. BackgroundNotificationQueueItem.kindunion extension topeer: closing-guard logic at ~4914 updated to includepeer.#withBackgroundPermissionearly return onpermissionScope: 'session': prevents session-scoped teammate approvals from being gated on a background-turn context that doesn't apply.- Idempotency of
#landUndrainedPeerNotificationsviaitem.persisted = true: multiple calls safe.
Not run: execution rungs 1–3 (working tree unavailable).
Unreviewed dimensions: PermissionMediator.vote internals · Windows/Linux runtime · TypeScript standalone typecheck · execution tier (working tree unavailable).
Approval blockers
- R1-1: late-poll quiescence race — report marked read but lost on managed shutdown (supported static chain)
- Execution gap: Deep-tier working-tree verification not run (working tree unavailable)
Reviewed with AI assistance.
| }) | ||
| .then(() => { | ||
| try { | ||
| // Queued teammate reports are already marked read in the on-disk |
There was a problem hiding this comment.
R1-1 Major: Shutdown quiescence race. This flush runs once, before writer terminals start. During the async writer-terminal chain, the TeamManager 500 ms poll can still fire, consume the inbox (marking rows read), and call the leader callback — which enqueues a new peer item (disposed is still false). When dispose() eventually runs, #landUndrainedPeerNotifications() calls recording.recordNotification() on an already-closed writer: silent no-op, permanent loss.
Nakagawa-master filed this same race on the prior head. A second flush after writers close, or an awaited in-flight poll drain before writers start, would close the window.
| issuedAtMs: Date.now(), | ||
| }; | ||
| const resolution = await this.mediator.request( | ||
| const resolution = this.mediator.request( |
There was a problem hiding this comment.
R1-2 Question: mediator.request() is started here (not awaited yet), and mediator.vote(CANCEL_VOTE_SENTINEL) may be called on it moments later if the binding was synchronously retired. Does that vote cause await resolution below to resolve as cancelled, or does it hang until timeout? I cannot trace this without the PermissionMediator.vote implementation — could you point to the resolution path?
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Code Review — PR #15658 (B/3)
评审结论：需修改后通过
🔴 BLOCKER
1. Managed shutdown 中 inbox poll 与 peer flush 之间存在竞态窗口，可能导致 teammate report 丢失
文件: packages/cli/src/acp-integration/acpAgent.ts:4246-4260
beginManagedShutdown() 在 cancelPendingPrompt() settle 后调用一次 flushPendingPeerNotifications()，然后进入 writer close 流程。但 TeamManager.pollLeaderInbox() 以 500ms 间隔持续轮询 leader mailbox，且在消费 inbox 行（标记 read: true）之后才异步调用 sink callback。
如果一次 poll 在 flush 完成后、writer 关闭前完成了消费并触发了 callback：
#enqueueTeammateMessage仅检查this.disposed，不检查this.closing，新 peer item 可入队- drain 被
closing门控不会自动排空 dispose()中的#landUndrainedPeerNotifications是最后机会，但此时 writer 可能已关闭，appendRecord变为静默 no-op- 结果：已被 mailbox 标记为 read 的报告既未持久化也未投递，数据丢失
修复方向：在执行最终 flush 之前先停止 inbox polling（或 await 当前 in-flight poll），确保没有新的 consume-and-callback 能在 flush 和 writer close 之间插入。PR comment 中 Nakagawa-master 已独立指出此问题并提供了 red/green fixture 建议。
对应测试缺口：需要一个 hold 住 consumeLeaderInbox() 直到 cancel+flush 完成后才释放的测试，assert 报告在 restart 后可恢复。
🟡 SUGGESTION
2. nextBindingId 模块级全局变量在多 Session/测试场景下缺乏隔离
文件: packages/cli/src/acp-integration/session/team-sink.ts:35
let nextBindingId = 0;同一进程内多个 Session 共享同一个递增序列。虽然 bindingId 的唯一性不依赖连续性，但测试间存在隐式状态耦合。如果未来有 hot-reload 或模块重新评估的场景，counter 可能重置导致旧 binding 的 fence 失效。
建议：将 counter 放入 TeamSinkDeps 或 per-session scope，或在测试 setup 中显式 reset。
3. defangPeerNotificationTags 正则在极端输入上有 ReDoS 风险
文件: packages/cli/src/acp-integration/session/peer-envelope.ts:22
return text.replace(/<(?=\s*\/?\s*peer-notification(?![-\w]))/gi, '<');正则包含 \s* 量词嵌套在 lookahead 中。对于恶意构造的超长空白字符串（如 < 后跟数万个空格再跟 peer-notification），lookahead 内的回溯可能导致 ReDoS。虽然实际队友报告不太可能包含这种输入，但作为安全边界应考虑添加长度上限或使用非回溯匹配。
建议：对 text 长度加一个合理上限（如 64KB），或在正则中使用 possessive/atomic 等价写法。
✅ 已确认修复的继承反馈
以下六个继承反馈项均已修复且有 falsifier 测试支撑：R8-3（targeted XML defang）、R9-2（teardown coalescing）、R9-5（JSDoc attachment）、R9-7（abort fake）、R9-8（detach coverage）、R9-9（unoffered option cancel）。
安全审查
无硬编码密钥/token；XML defang 已从 blanket escape 改为 targeted defang；身份字段校验完整；teammate name 注入风险低（下游渲染做了转义）。安全层面无阻塞。
CI 状态
Lint & Static ✅ pass | Test (ubuntu) ⏳ pending | web-shell E2E ⏳ pending | Integration Tests ⏳ pending
Review by QwenCode Reviewer (automated)
Stack: #15657 → #15658 → #15659. This is B, based on A (
split/13902-agent-identity,3294001f39040a668fcbad20045b64e03dc1b6e6); base branch:split/13902-agent-identity. C (split/13902-teammate-activity) depends on B. Split from #13902 at source commit6ed7884c381dfe1c59bd975d6cf61dbb41ef441c; candidate B:435e4f949eca7232c2b48d3a259f5083cb788b8a. Review B against A to isolate the coordination change.What this PR does
Connects an ACP-hosted leader to Team reports, roster membership and teammate approvals. Reports and join/exit notices enter the existing peer notification path with canonical teammate identity, survive recording and replay, and receive teammate attribution in the Web Shell. Pending reports participate in work holds, reversible close, queue overflow and shutdown flushing.
Teammate approvals reach the host independently of the leader's permission queue. Session scope, request UUIDs, binding epochs, ownership checks and retirement on manager replacement or teammate exit keep approval lifetime tied to the owning session. B includes producer, persistence, bridge, SDK/UI consumers and their tests. It consumes A's shared identity contract without depending on subagent runtime behavior. Tool activity and inter-agent message display arrive in C.
Why it's needed
Without a leader sink, ACP-hosted teammate reports remain unread and teammate approval events have no consumer. The leader and its client need a delivery path that carries attribution, preserves undrained reports and exposes teammate approval requests.
Reviewer Test Plan
How to verify
435e4f949eca7232c2b48d3a259f5083cb788b8a, runnpm run buildfrom the repository root, then run the package-local commands below serially. Each Vitest command runs from the stated package directory.Evidence (Before & After)
On candidate
435e4f949eca7232c2b48d3a259f5083cb788b8a,npm run buildexited 0. The focused package-local Vitest runs passed 3058 tests in 11 test files: ACP bridge 356, CLI 2246, core 130, SDK 13 and Web Shell 313. These counts come from this split candidate, not the original PR.Runtime, UI E2E and standalone typecheck were not run. Matched before/after protocol, persistence and UI evidence is not available. The build and focused tests do not establish that all verification obligations are complete; this PR remains a draft and the unresolved feedback below remains open.
Tested on
Environment (optional)
Local isolated worktree; repository-root build followed by serial package-local Vitest runs.
Risk & Scope
Inherited unresolved feedback is not fixed by splitting:
A's unresolved replay feedback is inherited through the stack; its ownership remains with A.
Design: English · 中文. Both documents expand A with coordination decisions, constraints, acceptance criteria and the C follow-up boundary.
Linked Issues
Related: #13785. Split source: #13902. This PR remains a draft. No automatic issue or PR closure is requested.
Stack links:
中文说明
堆叠顺序：#15657 → #15658 → #15659。本 PR 是 B，基于 A（
split/13902-agent-identity，3294001f39040a668fcbad20045b64e03dc1b6e6）；以split/13902-agent-identity为 base。C（split/13902-teammate-activity）依赖 B。拆分来源 #13902，固定源提交6ed7884c381dfe1c59bd975d6cf61dbb41ef441c；B 候选为435e4f949eca7232c2b48d3a259f5083cb788b8a。请相对 A 评审 B，以隔离协调层改动。本 PR 做什么
将 ACP-hosted leader 接入 Team 报告、成员变更和队友审批。报告及加入/退出通知携带规范队友身份，进入现有 peer 通知路径，通过录制和重放保留，并在 Web Shell 中标注队友。待处理报告参与 work hold、可逆关闭、队列溢出和关机 flush。
队友审批独立于 leader 权限队列到达 host。会话范围、请求 UUID、binding epoch、所有权检查及 manager 替换或队友退出时的撤回，将审批生命周期绑定到所属会话。B 包含生产者、持久化、bridge、SDK/UI 消费方及配套测试。它消费 A 的共享身份契约，不依赖 subagent 运行时行为。工具活动与 agent 间消息展示由 C 交付。
为什么需要
缺少 leader sink 时，ACP-hosted 队友报告保持未读，队友审批事件没有消费方。leader 和客户端需要能传递归因、保存未排空报告并展示队友审批请求的投递路径。
评审测试计划
435e4f949eca7232c2b48d3a259f5083cb788b8a上从仓库根目录运行npm run build，再串行执行英文部分列出的各包命令。每条 Vitest 命令均从标明的包目录执行。**前后证据：**候选
435e4f949eca7232c2b48d3a259f5083cb788b8a的npm run build退出码为 0。各包目录内的聚焦 Vitest 共通过 11 个测试文件、3058 项测试：ACP bridge 356、CLI 2246、core 130、SDK 13、Web Shell 313。这些数字来自本次拆分候选，不是原 PR 的历史结果。未运行 runtime、UI E2E 或独立 typecheck，也没有匹配的协议、持久化和 UI 前后证据。构建与聚焦测试不代表所有验证义务均已完成；本 PR 保持 draft，下述未解决反馈仍然开放。
**测试平台：**macOS 已执行构建及聚焦测试，未运行 runtime/UI E2E 或独立 typecheck；Windows、Linux 未测试。
**环境：**本地隔离 worktree；从仓库根目录构建，再串行运行各包目录内的 Vitest。
风险与范围
以下继承的未解决反馈未因拆分而修复：
A 的未解决 replay 反馈也通过堆叠继承，责任归属仍在 A。
设计文档：English · 中文。两份文档在 A 基础上扩展协调层决策、约束、验收标准及 C 的后续边界。
关联：#13785；拆分来源 #13902。本 PR 保持 draft，不请求自动关闭任何 issue 或 PR。
堆叠链接：