Repository navigation
feat(goal): start approved Web Shell proposals after their owning turn - #11360
Conversation
E2E report — ce6688cVerified on macOS / Chromium using the final local build, a real isolated daemon and ACP child, and a controlled local OpenAI model fixture. Separate home, runtime and workspace directories were used; no production daemon or real provider was modified.
Persistence audit: the approval session had exactly one Coverage limits: cancellation was triggered through the real HTTP endpoint, not a successful Escape-key interaction. The existing generic panel did not react to Escape. Browser reconnect was not exercised. Stale targets, protective exits, mid-turn input and cancellation/pause-write races are covered by the focused unit tests. This controlled fixture verifies lifecycle and transport, not natural-language drafting quality. All test services and the browser were closed after verification. Build, bundle, repository typecheck, changed-file ESLint/Prettier and 3,232 targeted tests passed locally. Hosted CI is separate and was still running when this report was posted. 中文：真实 |
qqqys
added
the
autofix/takeover
|
@qwen-code /takeover |
|
🔄 Takeover re-armed: the round counter starts a fresh window (previous rounds no longer count toward the cap); management continues. 中文说明🔄 已重新武装：轮次计数开启新窗口（此前轮次不再计入上限），托管继续。 |
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Review of PR #11360 — feat(goal): start approved Web Shell proposals after their owning turn
Head: ce6688c32ed · Effort: high · Mode: COMMENT
Summary
This is the second slice after #11284, enabling Web Shell (ACP) sessions to use the propose_goal tool with an Allow/Reject approval panel, and automatically starting the approved Goal after the proposing turn ends normally. The change is well-structured with strong test coverage (715 test lines, 258 production logic, 8 docs).
I reviewed the diff, the base code, and the existing triage analysis. The triage — posted by qwen-code-ci-bot — already provided a thorough analysis. I verified the triage's key claims against the code at HEAD ce6688c32ed:
Verified sound:
- Consent cannot be bypassed under YOLO/allow rules:
hideAlwaysAllow: trueis set on theinfoconfirmation,supportsHideAlwaysAllow()excludes onlyask_user_question, andfilterAlwaysAllowOptionsstripsallow_alwaysoptions. The ACP permission flow (permissionUtils.ts:30-49) correctly handles this. - Meta-key spoofing is prevented:
bridge.tsstripsqwen.goalProposalApprovalunconditionally and re-arms it only for an attached originator on a non-channel prompt.acpAgent.tsre-arms it only whenprivateParentState === 'trusted'. Both paths are tested. - The
isGoalProposalAvailable()gate correctly scopes schema visibility: returns true for interactive mode OR when both host support and turn key are set. The turn key is set in#executePromptInnerand cleared in thefinallyblock. matchesReviewedGoalis correctly implemented: records the goal at review time, validates at execution time, and rejects if the goal changed. Covered by thecreate/edit/replace/clear×before approval/before settlementparameterized matrix.- No import cycles:
tool-names.tsis a leaf import.
Triage findings confirmed (no new Criticals):
-
(Suggestion)
getFunctionDeclarationsFiltered()attool-registry.ts:896lacks thepropose_goalfilter present ingetFunctionDeclarations(). I confirmed neither subagent (builtin-agents.ts) nor background-agent-resume (background-agent-resume.ts:1610) currently declarespropose_goalin their tool lists, andEXCLUDED_TOOLS_FOR_SUBAGENTS(agent-core.ts:182) does not include it either. The gap is theoretical — execution is guarded atrunToolCallsinSession.ts:11935— but the asymmetry remains. -
(Suggestion) The
info-confirmation branch inbuildPermissionRequestContent(permissionUtils.ts:180-185) is generic and changes behavior for workflow inline-script and hook confirmations that already setrenderPromptAsPlainText: true. Base returned[]forinfo, so there is no duplication; the two tests (forward-the-complete-plain-textandpreserve-existing-contract) pin the split. The widening is unmentioned in the PR description.
CI Status
| Check | State |
|---|---|
| Test (ubuntu-latest, Node 22.x) | ❌ FAILURE (infra: disk full) |
| Lint & Static (ubuntu-latest) | ✅ SUCCESS |
| web-shell E2E Smoke | ✅ SUCCESS |
| Integration Tests (no-AK, No Sandbox) | ✅ SUCCESS |
| Desktop Shell (ubuntu-22.04) | ✅ SUCCESS |
| Desktop Shell (windows-2022) | ✅ SUCCESS |
The Test (ubuntu-latest, Node 22.x) check failure is infrastructure-related (disk full / ENOSPC on the runner), not code-related. All test suites that completed passed.
Verdict
No Criticals. The triage's findings remain accurate at HEAD. The code is architecturally sound, consent is non-bypassable, and the test coverage is comprehensive.
— Qwen Code · review 11360
|
🔀 Base updated: red check(s) [Test (ubuntu-latest, Node 22.x)] pass on current main — merged current main via update-branch; CI will re-run. 中文说明🔀 已更新 base：红色检查 [Test (ubuntu-latest, Node 22.x)] 在当前 main 上通过 —— 已通过 update-branch 合入当前 main，CI 将重新运行。 |
|
@qwen-code /takeover stop |
qwen-code-dev-bot
removed
the
autofix/takeover
|
👋 Takeover released: the autofix loop will no longer engage this PR (an in-flight round, if any, completes its bounded work). Re-apply 中文说明👋 已释放：autofix 循环不再介入此 PR（在飞的一轮如有，将完成其有界工作）。重新打上 |
…0260909 # Conflicts: # packages/cli/src/acp-integration/session/Session.ts # packages/core/src/core/client-goal.test.ts # packages/core/src/core/client.ts
|
@qwen-code /takeover from 4 |
|
🤝 Takeover engaged: the autofix loop now manages this PR — it will address new review feedback and resolve base conflicts until the label is removed or the round cap is reached. This is a fork PR, so the first round comes from the next scheduled scan (usually within minutes). This window's round counter starts at 4 (the rounds this PR spent in review before takeover), so the Critical-only brake engages after 1 more change-producing round(s) instead of a full fresh 5. Remove the 中文说明🤝 已接管：autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突，直到移除标签或达到轮次上限。本 PR 来自 fork，首轮处理将由下一次定时扫描执行（通常几分钟内）。本窗口轮次计数从 4 起算（即本 PR 托管前已进行的评审轮数），因此再经过 1 个产生改动的轮次即进入 Critical-only，而非重新计满 5 轮。移除 |
qwen-code-dev-bot
added
the
autofix/takeover
|
🤖 AutoFix ran out of time before finishing (timeout (7200000ms)) (attempt 5/100) — it will retry on the next scan.
See the Qwen Autofix agent step logs for model/tool output. 中文说明🤖 AutoFix 在完成前耗尽了时间（timeout (7200000ms)）（第 5/100 次尝试）—— 将在下次扫描时重试。 Run log: https://github.com/QwenLM/qwen-code/actions/runs/34414796812 🧠 Handled by Qwen Code · model/模型 |
… to a queued prompt (QwenLM#11360) #runStopContinuation's skipped-send funnel returned a plain end_turn for the queued-prompt and unreliable-drain stops without marking the turn's settlementBlocked flag, so a parked proposal approved earlier in the turn was applied even though the turn ceded to the queued user prompt. Set the flag at those three stops, mirroring the loop-level exits, and pin each with a regression that fails without it. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
🤖 Addressed the latest review feedback (round 6/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 6/100 轮）。改动内容与我反驳保留之处如下： Address summary — PR #11360 (round: R5 findings)One commit: Mode constraints applied
Fixed this roundR5-1 [Critical] (rc:3972497757) — Verified against the code before editing: the third Fix ( Tests ( Reproduction (before the fix): all three new tests failed on the pre-round code — Mutation probes (each guard's own witness): removing the block at the unreliable-drain stop fails only Resolved in earlier commits, re-verified at this HEADListed in Deferred to the next round (threads left open, replies posted)
Reason for deferral, identical for each: critical-only mode plus an exhausted budget last round caps this batch at the smallest blocking subset; these are Suggestion-level and each needs its own witness tests. None is declined on the merits. Verification
中文说明处理摘要 — PR #11360（本轮：R5 发现）一个提交： 本轮应用的模式约束
本轮修复R5-1 [Critical]（rc:3972497757）—— 修改前已对代码核实： 修复（ 测试（ **复现（修复前）：**三个新测试在改动前的代码上全部失败——每条路径上 **变异探针（每个守卫各自的见证）：**移除不可靠 drain 停止点的阻断后，只有 已在更早提交中解决、并在当前 HEAD 重新核实已列入 延后到下一轮（线程保持开放，已逐条回复）
延后理由对每条相同：仅 Critical 模式加上上一轮预算耗尽，本批次上限就是最小阻断子集；这些都是 Suggestion 级，且各自需要独立的见证测试。没有任何一条是因价值判断而被拒绝。 验证
Deferred non-Critical feedbackCritical-only mode is active: the round counter reached 5 (this window was seeded at round 4 by 中文说明已进入仅处理 Critical 的模式：轮次计数已达 5（本窗口由 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧵 Resolved all 40 selected review thread(s). · 已关闭全部选中的 40 条评审线程。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
|
@qwen-code /takeover stop |
qwen-code-dev-bot
removed
the
autofix/takeover
|
👋 Takeover released: the autofix loop will no longer engage this PR (an in-flight round, if any, completes its bounded work). Re-apply 中文说明👋 已释放：autofix 循环不再介入此 PR（在飞的一轮如有，将完成其有界工作）。重新打上 |
…temp dirs (QwenLM#11360) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
🤖 Addressed the latest review feedback (round 7/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 7/100 轮）。改动内容与我反驳保留之处如下： Autofix round summary — PR #11360What this round didFixed the deterministic verification rejection from the previous round with Root cause of the rejection (diagnosed from evidence)The gate failed on The fixThe two tests that count run dirs in the global temp dir A new witness test ( Mutation probe (required before committing): with the redirect temporarily Feedback dispositions
Footprint noteThis commit touches Verification
中文说明Autofix 本轮总结 — PR #11360本轮完成的工作用一个后续提交（ 拒绝原因（基于证据的诊断）门禁在 修复方式两个在全局临时目录中统计运行目录的测试（ 新增一个见证测试（ 变异探针（提交前必做）：临时移除见证测试中的重定向后，该测试以 反馈处置
改动范围说明本提交触及 验证
Deferred non-Critical feedbackCritical-only mode is active: the round counter reached 5 (this window was seeded at round 4 by
中文说明已进入仅处理 Critical 的模式：轮次计数已达 5（本窗口由 Base-conflict check · 基分支冲突检查: conflicted with main — resolved in this push. · 与 main 有冲突——已在本次推送中解决。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
Convergence scopeTakeover remains stopped. This PR is being brought to a bounded acceptance decision for the original Web Shell Goal proposal flow: explicit approval, exactly-once application after the proposing turn ends normally, and no stale execution after cancellation, protective exits, queued user input, Goal changes, or entry into Plan mode. The Plan-mode transition finding remains in this PR for reproduction and a correctness fix if confirmed: #11360 (comment). The five other open review threads describe three non-blocking recovery-message problems. They are consolidated in #11569, assigned to the PR author. Their disposition is Completion requires the final commit's relevant regressions, build/typecheck, key isolated Web Shell behavior checks, and one bounded correctness review. Hosted CI and review completion are reported separately. Remaining non-blocking polish or review-harness improvements do not expand this patch. |
|
Convergence verification for head The Plan-switch defect is fixed and its thread now has reproduction and verification evidence. The five nonblocking threads were explicitly deferred to #11569 (three recovery-message topics). A full paginated readback found 47 total review threads and zero unresolved threads. The scope remains the existing explicit-approval panel, settlement by the owning normal turn, exactly-once application, and safe automatic start. Takeover remains stopped. Before and afterBefore the repair, a real terminal session could approve a Goal, enter Plan while acknowledgement was pending, then create an active Goal and run three automatic turns after acknowledgement was released. The three corresponding deterministic negative tests failed on the pre-fix head, while the ordinary exactly-once control passed. The final bundle passed these real Web Shell flows through the browser, daemon, ACP child, permission panel, Goal persistence, and automatic-turn execution:
The same Plan-switch terminal flow passed on the repair before the final upstream-only merge ( Local verification
Validation boundary and hosted gatesThe UI evidence covers lifecycle behavior with a controlled model response fixture, not live external-provider behavior or production deployment. No new anonymous/clientless E2E, browser-reconnect check, or Escape-key check is claimed. Local verification and thread resolution do not constitute formal GitHub approval or a merge. Current-head Qwen Code CI completed successfully, including Ubuntu unit tests, lint/static checks, integration tests, and the downstream Web Shell browser gate and smoke tests. SDK Java, including real-daemon E2E, and TUI parity also passed. The complete current-head readback contains no failed or pending checks; conditional platform and comment-routing jobs were explicitly skipped. GitHub reports 中文说明本轮已完成范围收敛：Plan 切换缺陷已修复并附证据关闭；五条非阻断线程按三类提示问题移至 #11569，完整分页回读为 47 条、未解决 0 条。接管保持停止，验收范围固定为已有审批面板、原回合正常结算、仅应用一次和安全自动启动。 最终 Web Shell 真实页面及持久化验证：批准后进入 Plan、拒绝、批准后取消均为 0 次 Goal 创建，正常批准为 1 次并开始自动运行。基线曾在进入 Plan 后错误创建 Goal 并自动运行三轮；修复后真实终端显示批准已撤销， 本地 17 个相关文件共 4,270 项测试通过，构建、类型、lint、格式、diff 检查通过；上游最后一次队列清理改动在最终 head 重跑了 914 项受影响会话测试及相关构建检查。一轮限定范围正确性审查未发现可证实阻断项。模型回复为本地 fixture，不代表真实模型或生产部署；线程关闭也不等于正式批准或合并。 当前 head 的远端 CI 已通过，包含单测、静态检查、集成测试及后续 Web Shell 浏览器门禁和 Smoke；Java SDK/真实 daemon E2E、TUI 检查也通过。完整回读没有失败或等待中的检查，条件矩阵及评论路由任务按配置跳过。正式评审状态为 |
qwen-code-dev-bot
left a comment
qwen-code-dev-bot
left a comment
There was a problem hiding this comment.
Approved at head 56f115f0.
Required CI is green at this commit — Test (ubuntu-latest, Node 22.x), Lint & Static, Integration Tests (no-AK, No Sandbox) and web-shell E2E Smoke completed successfully; only review-pr (the reviewer's own job) is still running. All 47 review threads are resolved, and the one Critical the last deep round left standing is closed — verified against this head, not against the resolution flag.
R5-1 (the skipped-send funnel settling an approved Goal anyway). The three exits that finding named now block settlement before returning, each on the same line that hands back { kind: 'stop', stopReason: 'end_turn' }: the unreliable-drain arm at Session.ts:7059, the empty-drain probe === 'queued' arm at :7082, and the final claim === 'queued' arm at :7159. blockGoalProposalSettlement is now defined twice — once in #handleStopHookLoop (:6560, its original nine exits) and once inside #runStopContinuation (:6920) — which is the right shape, because the hole existed precisely because the funnel was outside the first one's reach. The rest of the earlier Criticals are gone from this diff: the originator-client grant, the provisional-workspace clause and the mid-turn notice were each rewritten in rounds that later reviews re-measured and did not re-list.
I also read what landed after that review rather than assuming a merge is inert. aeadac2afb (Plan mode revoking an approved proposal) hangs off a single new optional field: the pending proposal carries an approvalSignal from a per-Config AbortController, entering PLAN aborts it and leaving the proposal resets the controller, and every consumer treats an aborted signal as applied: false with a message that tells the user to ask for a new draft rather than silently starting the Goal. Two details are right rather than incidental: the abort is guarded by !isDerivedConfig(this), so a derived config cannot revoke the host's controller, and the settlement paths check proposal.approvalSignal?.aborted beside the existing ownsTurn() term instead of replacing it, so the "another turn owns it" case still wins. It brings its own tests (client-goal-plan-switch.test.ts, plus 101 lines in Session.test.ts).
No new Critical found. One question worth an answer, not a gate: the sibling probe === 'unavailable' (:7088-7094) and claim === 'unavailable' (:7166-7172) arms return the identical { kind: 'stop', stopReason: 'end_turn' } without blocking settlement. If "unavailable" can never leave a queued prompt in pendingPromptList, that asymmetry is correct and deserves the one-line comment it currently lacks; if it can, it is the same race R5-1 described by another arm.
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Reviewed at head 56f115f0 against merge-base 2488d12d. One merge blocker, plus two suggestions and one nice-to-have. Much of this PR checks out — I list what I verified at the end so the objection's scope is clear.
Blocker: the parked approval has two uncoordinated consumers of one single-take latch
Config.takePendingGoalProposal (config.ts:8186-8198) clears pendingGoalProposal unconditionally and returns it to whoever calls first:
this.pendingGoalProposal = undefined;
return proposal;Two paths call it for the same turn, with different preconditions and different failure reporting.
Consumer A — core, in-stream (client.ts:897-911). Its entire guard set is if (!turnEnded && !signal.aborted) return; … if (!proposal) return; if (signal.aborted) return;. grep -c settlementBlocked packages/core/src/core/client.ts returns 0 — it never consults the block flag the Session sets. It then calls loadGoalRuntime(false) (non-forced), and when that returns undefined it drops an approved proposal with nothing but debugLogger.debug('Dropping an approved Goal proposal: the Goal runtime is unavailable') — no reportFailure, so no GoalSettlementFailed event is emitted on this path.
Consumer B — Session, post-turn (Session.ts:5008 → #settleGoalProposal). This one force-loads via getGoalRuntimeReady() and emits formatProposeGoalRecoveryNotStarted when it does not own the turn.
They key on the identical string, which is what makes this a race rather than two independent mechanisms: Session.ts:5418 builds this.config.getSessionId() + '########' + this.turn, :5425 stores it as the captured turn's turnKey (the same object :5040 later takes with), and :5456 does promptIdContext.enterWith(promptId) so the stream — and therefore core's settler, which takes with prompt_id (client.ts:3346, :3397 goalTurnKey ??= prompt_id) — sees that exact value.
Two wrong outcomes follow, and they are different failures of the same defect:
- Start despite block. Core settles at the in-stream model-stop boundary, so a turn the Session marked
settlementBlocked— set atSession.ts:6560from roughly a dozen call sites covering queued mid-turn user input,todoStopGuardQueuedPromptPriority, guard exhaustion and the stop-hook cap — can still create an active Goal and begin autonomous turns. That is precisely the flow this PR's own test plan says must not start. - Silent total loss. On a first draft, where no Goal exists yet and the runtime is not loaded, core takes the proposal, fails the non-forced load, and drops it with a debug line. Session's settler then receives
undefinedfrom the latch and returns without emitting the recovery line. The user clicks Allow, sees the model acknowledge, and no Goal ever exists and no/goal set …recovery is shown — even though the Session path deliberately force-loads to avoid exactly that.
Deterministic coverage cannot catch this today: Session.test.ts mocks the stream, so core's take never runs, and the four browser flows in the test plan (Allow / Reject / cancel-after-Allow / Plan-after-Allow) are all decided by signal.aborted or approvalSignal.aborted, which both consumers honour. The queued-input flow is listed in the test plan but absent from the evidence.
Fix — give the parked proposal one owner. Either skip core's settle when the host settles (if (this.config.getGoalProposalHostSupported?.()) return; before the take at client.ts:900), or record a settlementOwner/blocked flag on PendingGoalProposal that core consults. Independently, make the runtime-unavailable branch at client.ts:903-908 call reportFailure(formatProposeGoalRecoveryFailed(proposal.objective)) (or force-load, as Session does) instead of dropping silently. Then add one test that runs core's settle and Session's settle in sequence against the same real turnKey, for both the blocked-turn and runtime-not-loaded cases.
Suggestion: the turn-error path discards an explicit approval in silence
Session.ts:5038-5044 — the finally calls this.config.takePendingGoalProposal(proposalTurn.turnKey) and ignores the return value, and #settleGoalProposal only runs on the success path (:5008). So when the proposing turn throws (provider error, or the loop-detected turn error at :5035), the user's consent is dropped with no formatProposeGoalRecoveryNotStarted and no GoalSettlementFailed (that channel is wired only inside client.ts). Contrast :5327-5333, where the identical "did not own the turn" outcome does emit the recovery line. Dropping is the correct safety decision; the silence is the defect. Capture the taken proposal in the finally and emit the recovery line when it is non-undefined and the session is not disposed or closing.
Suggestion (security): the capability is trusted on the peer's assertion
acpAgent.ts:14328-14330 sets config.setGoalProposalHostSupported(true) from this.clientCapabilities?._meta?.['qwen.goalProposals'] === true — a value the connecting ACP client supplies in initialize, with no host-side corroboration. That flag is what unlocks the surface: base gated on resolveInteractionMode(this) === 'interactive' alone, and config.ts:9753 is now (resolveInteractionMode(this) === 'interactive' || this.goalProposalHostSupported), so registerLazy(ToolNames.PROPOSE_GOAL, …) runs for a non-interactive session and isGoalProposalAvailable() (:8160-8165) returns true on any turn carrying the approval meta. A non-bridge client — another editor integration, a scripted qwen --acp driver — that asserts the flag can then answer its own confirmation dialog and start autonomous multi-turn Goal work in a session nobody is watching.
To be precise about what is not wrong here: the per-prompt approval meta is correctly gated (privateParentState === 'trusted' at :6314-6320, and the bridge re-injects only for a live attached originator at bridge.ts:10541-10550). So this is the one remaining peer-controlled input in the chain — it is the capability, not the approval, that is trusted on assertion. Corroborate it host-side before enabling the surface: honour qwen.goalProposals only when the connection is a trusted private parent / daemon bridge channel (the same privateParentState === 'trusted' predicate already used at :6316), or require an attached, identified prompt originator at initialize time.
Nice-to-have: two pieces of new public surface
Config.trackSessionRegistration changed from Promise<boolean> to Promise<SessionRegistration> — a breaking signature change on an exported class. In-repo callers were updated, but an out-of-tree JS embedder that still resolves a boolean gets outcome.slot === undefined stored into sessionRegistrySlot and passed to patchSessionRecord(patch, undefined) / unregisterSession(undefined), i.e. a wrong-record or no-record write rather than a compile error. Accepting Promise<boolean | SessionRegistration> and normalising internally would keep it additive. Separately, LlmEventType.GoalSettlementFailed is a new member of the exported ServerLlmStreamEvent union; both in-repo mappers were updated (use-llm-stream.ts:3010-3025, event-adapter.ts:714-718) but an older consumer hits its default branch and drops it silently, losing the only notice that an approved Goal failed to start.
What I verified as sound
The start is authorized by a host-side dialog outcome, not by a wire field or a plan document: ProposeGoalInvocation.shouldConfirmExecute snapshots reviewedGoal = {goalId, revision} | null at confirm time (goal-tools.ts:884-887), execute() re-verifies matchesReviewedGoal before parking (:911-919, and it returns false for undefined, so an older producer fails closed), and applyPendingGoalProposal re-verifies again at settle time (:679-691) while refusing an active Goal. hideAlwaysAllow: true (:892) stops always-allow from converting one consent into standing consent. Idempotence is a real latch, not a boolean: setPendingGoalProposal returns false when one is already parked (config.ts:8173), plus the single take, plus Session's activeGoalProposalTurn === proposalTurn identity guard (:5041, :5324) and the per-turn tool gate (:12239-12249); a second approval in a later turn cannot reuse the old key because the key is the turn's promptId.
I enumerated the release paths and found none that starts twice: normal end (:5001-5008, requiring stopReason === 'end_turn' && !pendingSend.signal.aborted && settlementBlocked !== true), abort/cancel (settlementBlocked set at :8828-8833; core also returns on signal.aborted after taking), Plan-mode entry mid-acknowledgement (setApprovalMode aborts the controller at config.ts:7335-7337, applyPendingGoalProposal refuses on approvalSignal.aborted, and leaving Plan does not revive it because a fresh setPendingGoalProposal mints a new controller), and session close/dispose (ownsTurn() re-checked before and after getGoalRuntimeReady() at :5325-5326/:5337, with a Goal created before the switch paused as GOAL_PAUSE_REASON_SESSION_DISPOSED at :5350-5356).
No new listeners to leak: use-llm-stream.ts +16 is one added case inside the existing per-event switch, event-adapter.ts +5 is one added case in a pure mapper. Turn-start/turn-end races are guarded by identity comparison rather than a boolean, so a new prompt demotes the old turn instead of double-firing it. Approval state is in-memory only, so a restart or channel-worker recycle drops a parked approval — fail-closed, though silent (see the second finding).
Two lifecycle items I could not substantiate to a producer, flagged rather than filed: setTools() is called on turn start (:5428-5436) and in every prompt's finally (:5045-5055) on a LlmClient shared with concurrently dispatched automatic Goal/cron turns, so a non-atomic mutation of the declared-tool list could let an in-flight Goal turn see propose_goal appear or disappear mid-turn (confirming needs reading setTools, outside this pass); and the held-proposal recovery at :4747-4756 does goalQueue.splice(0) plus releaseTurn(key, { requeue: false }) for every queued turn, permanently discarding queued automatic work on each user prompt while a failed pause is held — whether /goal resume re-enqueues those depends on runtime behaviour I did not read.
CI at head: green apart from review-pr. 47 review threads, 0 unresolved. I did not run the suite locally — no node_modules in this checkout.
中文说明（阻塞项摘要）
阻塞项：一个「单次领取」闩锁有两个互不协调的消费者。 takePendingGoalProposal（config.ts:8186-8198）无条件清空并返回给第一个调用者。core 侧在流内结算（client.ts:897-911），其全部前置条件里没有 settlementBlocked（该文件里 grep 命中数为 0），且用非强制的 loadGoalRuntime(false)，失败时只写一行 debugLogger.debug 就丢弃、不发 GoalSettlementFailed；Session 侧在回合后结算（Session.ts:5008），用强制的 getGoalRuntimeReady() 并会发恢复提示。两者的 key 是同一个字符串（Session.ts:5418 构造 → :5425 存入被捕获回合的 turnKey → :5456 promptIdContext.enterWith，core 侧以 prompt_id 领取，见 client.ts:3346、:3397），所以是竞态而非两套独立机制。
两个后果：① 被阻止的回合仍会启动——Session 标了 settlementBlocked（:6560，约十二处调用：回合中排队输入、todoStopGuardQueuedPromptPriority、守卫耗尽、stop-hook 上限）的回合，core 仍可能建出 active Goal 并开始自主回合，而这正是本 PR 测试计划声明不该发生的；② 静默全丢——首个草案（Goal runtime 尚未加载）时 core 领取后丢弃、只留一行 debug，Session 侧随后从闩锁拿到 undefined 直接返回、不发恢复提示：用户点了 Allow、看到模型确认，却既没有 Goal 也没有 /goal set … 恢复行。
现有确定性测试抓不到：Session.test.ts mock 了流，core 的领取根本不执行；测试计划里的四个浏览器流程都由 signal.aborted / approvalSignal.aborted 决定，两条路径都遵守。修法：给这个待处理提案唯一的所有者（core 在宿主会结算时直接跳过领取，或在 PendingGoalProposal 上记一个 owner/blocked 标志供 core 读取），并让 runtime 不可用那一支改为 reportFailure(...) 而不是静默丢弃；再补一个用同一个真实 turnKey 依次跑两条结算路径的测试，覆盖「被阻止的回合」与「runtime 未加载」两种情形。
建议一：回合出错路径（Session.ts:5038-5044）在 finally 里领取后丢弃返回值、不发任何提示，而 :5327-5333 对同样的「未拥有该回合」结果会发恢复行——丢弃是对的，沉默是缺陷。
建议二（安全）：「宿主能回答 Goal 提案」这个能力取自对端在 initialize 里自报的 _meta['qwen.goalProposals'] === true（acpAgent.ts:14328-14330），没有任何宿主侧佐证，而它正是解锁开关：base 只看 resolveInteractionMode(this) === 'interactive'，现在 config.ts:9753 变成「interactive 或 goalProposalHostSupported」。非 bridge 的 ACP 客户端只要断言该标志就能启用该面并自问自答，从而在无人观看的会话里启动自主多回合 Goal 工作。需要说清楚的是：每回合的审批 meta 是正确受控的（:6314-6320 的 privateParentState === 'trusted'；bridge 只为在线且已挂接的发起者重注入，bridge.ts:10541-10550），所以这是授权链上唯一剩下的对端可控输入——被采信的是能力，不是审批。建议用同一个 privateParentState === 'trusted' 谓词在宿主侧佐证后再启用。
可选：Config.trackSessionRegistration 从 Promise<boolean> 改成 Promise<SessionRegistration>，是导出类上的破坏性签名变更（树外 JS 嵌入者会得到 outcome.slot === undefined 并流进 patchSessionRecord/unregisterSession，是运行时错记录而不是编译错误）；LlmEventType.GoalSettlementFailed 是导出联合类型的新成员，旧消费者会走 default 分支静默丢掉「已批准的 Goal 启动失败」这一唯一通知。
我也核实了大量正确的部分（授权来自宿主侧对话框结果而非线上字段、reviewedGoal 三处复验、hideAlwaysAllow、幂等闩锁、四条释放路径均不会二次启动、无新增监听器），详见英文部分——反对的范围只限于上面这一条阻塞项。
|
Addressed the current-head single-take settlement blocker from review 5168066176 in
Local validation passed on Node 22: 54 core Goal tests, 28 ordinary-turn/blocked Session tests, build, repository typecheck, lint, Prettier check, and diff check. A high-concurrency full core run hit unrelated process/Git integration timeouts; all five affected files then passed with one worker and coverage disabled (259 passed, 10 skipped). Current-head hosted checks are running. |
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Re-reviewed at head 60e2c85e, which adds one commit (fix(goal): give ACP proposal settlement one owner, +87 across 3 files) on top of what I requested changes on. The blocker is fixed. Withdrawing my CHANGES_REQUESTED and approving.
The blocker, verified fixed
Both halves of the fix I asked for are in, in the right places:
client.ts:899—if (this.config.getGoalProposalHostSupported?.()) return;now sits at the top ofsettlePendingGoalProposal, beforeconst take = this.config.takePendingGoalProposal. So on a host-settled session core never touches the single-take latch at all. That closes both outcomes I filed, and it closes the worse one structurally rather than by patching a condition: core can no longer settle at the in-stream boundary for these sessions, so thesettlementBlockedflag is honoured by the only consumer that reads it (Session), and the queued-mid-turn-input / guard-exhausted / stop-hook-cap cases can no longer start an autonomous Goal.client.ts:911— the runtime-unavailable branch now callsreportFailure(formatProposeGoalRecoveryFailed(proposal.objective))before returning, so a dropped approval emitsGoalSettlementFailedinstead of onlydebugLogger.debug. That was the silent-loss half.- The doc comment at
:889-890records the ownership rule ("Host-supported sessions settle after classifying their own protective exits, so core must leave their single-take proposal latch untouched"), which is what stops a future call site from re-introducing the second consumer.
The witnesses are real, not decorative. leaves a host-owned approved proposal for Session settlement flips getGoalProposalHostSupported to true and asserts store.take was not called, loadGoalRuntime was not called, runtime.dispatch was not called, and that the proposal is still retrievable afterwards — so reverting the guard reddens it on all four. reports recovery when the core-owned proposal runtime is unavailable pins the new reportFailure. The shared mock gained getGoalProposalHostSupported: vi.fn(() => false) so the pre-existing core-owned cases keep exercising the old path rather than silently switching sides. This is exactly the coverage gap I named (deterministic tests could not reach core's take because Session.test.ts mocks the stream) and it is now closed from the core side, which is where the take actually lives.
One new coupling worth naming, not blocking: the flag now means "somebody else will settle this", and exactly one site sets it (acpAgent.ts:14328-14330) while exactly one site settles (Session.ts:5008 success path, :5040 error path — between them they always either settle or drop). A future host that set the flag without settling would leave an approval parked forever. The ?. optional call fails toward core settling, which is the safe direction for a Config that predates the method.
Still open from my last pass — all Suggestions, and I am not escalating any of them
- The turn-error path (
Session.ts:5038-5044) still discards an explicit approval with no user-visible notice, unlike:5327-5333where the identical "did not own the turn" outcome emitsformatProposeGoalRecoveryNotStarted. Dropping is the right safety call; the silence is the gap. - The host capability is still taken from the peer-supplied
initialize_meta['qwen.goalProposals'] === truewith no host-side corroboration. I am comfortable approving with this open, and want to be explicit about why rather than let it look overlooked: an ACP client that can connect already drives the agent and answers tool confirmations, so enabling the proposal surface is not a meaningful escalation for an already-connected peer. Corroborating it with theprivateParentState === 'trusted'predicate already used at:6314-6320would still be the right hardening, and it is a small change. - The two public-surface notes stand:
Config.trackSessionRegistration'sPromise<boolean>→Promise<SessionRegistration>change is breaking for out-of-tree JS embedders (they getoutcome.slot === undefinedflowing intopatchSessionRecord/unregisterSessionrather than a compile error), andLlmEventType.GoalSettlementFailedis a new union member an older consumer drops silently.
None of the three is a merge blocker, and the first two are each a few lines if you want them in this PR rather than a follow-up.
Everything I verified as sound last round is untouched by this commit — the diff is client.ts plus two test files — so the authorization chain (reviewedGoal snapshot and its three re-verifications, hideAlwaysAllow, the idempotence latch, the four release paths, no double-start) still holds as recorded.
CI at 60e2c85e: green apart from review-pr. I did not run the suite locally (no node_modules in this checkout); the two new core cases are the evidence for the fix.
中文说明
在 head 60e2c85e 上复审——它在我提 CHANGES_REQUESTED 的版本之上加了一个提交（fix(goal): give ACP proposal settlement one owner，3 个文件 +87 行）。阻塞项已修好，撤回我的 CHANGES_REQUESTED 并批准。
我要的两半修复都到位、且位置正确：client.ts:899 的 if (this.config.getGoalProposalHostSupported?.()) return; 位于 settlePendingGoalProposal 顶部、在 takePendingGoalProposal 之前，所以宿主结算的会话里 core 根本不碰那个单次领取闩锁——这从结构上（而不是靠补一个条件）关掉了两个后果，尤其是较严重的那个：core 不再能在流内边界结算，于是 settlementBlocked 由唯一会读它的消费者（Session）生效，排队输入/守卫耗尽/stop-hook 上限这些情形不再可能启动自主 Goal。client.ts:911 在 runtime 不可用分支补上了 reportFailure(formatProposeGoalRecoveryFailed(...))，静默丢失那一半也关掉了。:889-890 的注释把所有权规则写了下来，这正是防止将来某个调用点重新引入第二个消费者的东西。
见证测试是实的：leaves a host-owned approved proposal for Session settlement 把开关置 true 后断言 store.take 未被调用、loadGoalRuntime 未被调用、runtime.dispatch 未被调用、且提案事后仍可领取——回退守卫会让四条全红；reports recovery when the core-owned proposal runtime is unavailable 钉住新的 reportFailure；共享 mock 补了 getGoalProposalHostSupported: vi.fn(() => false)，让既有的 core-owned 用例继续走原路径而不是静默换边。这正好补上了我点出的覆盖缺口（确定性测试到不了 core 的领取，因为 Session.test.ts mock 了流），而且是从 core 侧补的——领取动作就发生在那里。
一处新耦合，仅作记录、不阻塞：这个标志现在意味着「由别人来结算」，而只有一处设置它（acpAgent.ts:14328-14330）、只有一处结算（Session.ts:5008 成功路径与 :5040 错误路径，二者合起来必定要么结算要么丢弃）。将来若有宿主设了标志却不结算，已批准的提案会永远停在那里。?. 的可选调用在方法缺失时倒向 core 结算，对旧 Config 是安全方向。
上一轮其余三条仍开着，都是建议级，我不升级任何一条：① 回合出错路径（Session.ts:5038-5044）仍静默丢弃用户的明确批准，而 :5327-5333 对同样的「未拥有该回合」结果会发恢复提示；② 宿主能力仍取自对端 initialize 自报的 _meta['qwen.goalProposals']，无宿主侧佐证——我明确说明为什么可以带着它批准：能连上的 ACP 客户端本来就能驱动 agent、回答工具确认，所以启用提案面对一个已连接的对端不构成实质提权；用 :6314-6320 已有的 privateParentState === 'trusted' 谓词佐证仍是正确的加固，且改动很小；③ 两处公开面（trackSessionRegistration 的破坏性签名变更、GoalSettlementFailed 新联合成员会被旧消费者静默丢弃）。
上一轮我核实为正确的部分未被此提交触碰（diff 只有 client.ts 加两个测试文件），所以授权链（reviewedGoal 快照与三处复验、hideAlwaysAllow、幂等闩锁、四条释放路径、无二次启动）仍如记录成立。
60e2c85e 上的 CI：除 review-pr 外全绿。我没有在本地跑套件（此 checkout 无 node_modules）；修复的证据是那两个新的 core 用例。
qwen-code-dev-bot
left a comment
qwen-code-dev-bot
left a comment
There was a problem hiding this comment.
Approved at head 60e2c85e.
Required CI is green — Test (ubuntu-latest, Node 22.x), Lint & Static, Integration Tests (no-AK, No Sandbox) and web-shell E2E Smoke completed successfully; only review-pr (the reviewer's own job) is still running. All 47 review threads are resolved.
The merge blocker from the maintainer review is fixed at this head, in the form he specified, and I checked the shape rather than the claim:
- One owner for the single-take latch.
LlmClient.settlePendingGoalProposalnow returns before touching anything whenconfig.getGoalProposalHostSupported?.()is true (client.ts:899), so in an ACP-hosted session core cannot consume the parked proposal ahead of the Session. That closes both wrong outcomes from one place: a turn the Session markedsettlementBlockedcan no longer create an active Goal through core's in-stream boundary, and the non-forcedloadGoalRuntime(false)in core can no longer be the path that eats the proposal. The early return sits above thetake, not below the other guards, which is what makes it total, and it is host-supported-gated, so the interactive CLI/TUI sessions core still owns keep settling exactly as before. - The silent-drop branch no longer drops silently.
client.ts:908-911now callsreportFailure(formatProposeGoalRecoveryFailed(proposal.objective))before returning when the runtime is unavailable, in the path core still owns, with the doc block updated to say why core must leave a host-owned latch untouched. - Coverage went where the race actually lives.
client-goal.test.tsgains 59 lines andSession.test.ts24, covering the host-supported stand-down and the failure report — the review's point that a mocked stream hid core's take from the Session suite is answered rather than argued around.
No new Critical found. Two items from that review remain open and I am listing them so they are not read as settled — the reviewing maintainer classified both as Suggestions, and they are his call to triage:
acpAgent.ts:14328still enables the surface fromclientCapabilities._meta['qwen.goalProposals'] === truewith no host-side corroboration, so the capability — not the per-prompt approval, which is correctly gated at:6314-6320andbridge.ts:10541-10550— is trusted on the peer's assertion. Corroborating it with the sameprivateParentState === 'trusted'predicate would close it cheaply.Session.ts:5038-5044still discards the proposal taken in thefinallywithout emittingformatProposeGoalRecoveryNotStarted, so a proposing turn that throws loses the user's consent quietly. With core now standing down in these sessions, nothing else reports it either, which makes the one-line recovery emit on that path more worth having than it was before.
Both look like small, well-scoped follow-ups rather than reasons to hold this PR, and the second one is the same family as my own earlier note that the probe === 'unavailable' / claim === 'unavailable' arms in the stop-hook funnel behave differently from the three that now block settlement — worth an explicit comment or a test either way.
|
Post-merge review — PR was merged while this review ran; findings below are for follow-up tracking, not a merge gate. Verdict: No blocking findingsTier: Deep (auth/trust boundary, concurrency, lifecycle — yes to all five risk questions) Scope: all 34 source files in the diff reviewed; docs and test files read as supporting evidence. No local execution (working tree unavailable); runtime-tier findings are therefore unreviewed dimensions. Cross-checkThe blocking finding filed by yiliang114 — two uncoordinated consumers of the single-take proposal latch — was verified fixed at the current head by yiliang114's own re-review (approved at FindingsNo blocking findings. Non-blocking items below. Minor — Turn-error path discards an explicit approval silently Minor — Host capability trusted on peer assertion Minor — Minor — What was checkedAuthorization chain:
Race conditions:
Zombie state:
Channel exclusion:
Settlement ownership:
Not covered: No local execution ran (no working tree). Runtime-tier findings (test validity, mutation probes) are an unreviewed dimension. Reviewed with AI assistance. |
6 participants
What this PR does
Web Shell offers a drafted Goal in its existing Allow/Reject panel. Approval is applied exactly once after the proposing user turn finishes normally, and the Goal runtime starts its first working turn automatically. This is the second slice after #11284.
Why it's needed
Web Shell previously required users to copy a manual
/goal setcommand. The new path requires a capable host and an attached prompt originator. Consent remains explicit under YOLO and permission rules, belongs to the proposing turn and the exact reviewed Goal, and is revoked when the session enters Plan mode. Leaving Plan mode does not revive that approval; a fresh approval is required.Reviewer Test Plan
How to verify
Evidence (Before & After)
Baseline reproduction confirmed that approving a Goal and entering Plan before the acknowledgement finished still started an active Goal and three automatic turns. Deterministic regressions reproduced creation after Plan entry, after entering and leaving Plan, and during runtime loading; the ordinary exactly-once control passed.
At head
56f115f03f720c02805259bb1ff6c5af61265655, the final Web Shell bundle passed real-browser flows for normal Allow, Reject, cancellation after Allow, and entering Plan after Allow. The three negative flows left no Goal even after a delayed acknowledgement was released. Normal Allow created one Goal and began automatic work; a controlled no-progress response fixture then exercised the expected pause after three turns. The terminal Plan-switch flow also passed on the same repair before the final upstream-only merge: no Goal was set and the user saw the revoked-approval notice.Local verification passed: 4,270 tests across 17 affected suites, build and bundle, typecheck, focused lint, formatting, and diff checks. The final upstream merge changed session queue cleanup, so its 914 session tests plus the affected build, bundle, and typecheck were repeated on the pushed head. One bounded correctness review found no demonstrable blocker in the consent, turn ownership, settlement, and automatic-start paths. This is separate from the hosted CI result and formal GitHub review. The UI fixture validates deterministic lifecycle behavior, not a live external model's drafting quality.
Tested on
Environment
An isolated worktree with Node.js 24.14.1. Real UI checks use a local controlled model fixture and separate configuration, runtime, and workspace directories. Hosted CI is reported independently of local checks.
Risk & Scope
Linked Issues
Follow-up to #11284. Deferred recovery-message improvements: #11569.
中文说明
本 PR 的改动
Web Shell 使用现有 Allow/Reject 面板展示目标草案。批准只在提出草案的用户回合正常结束后生效一次，随后 Goal runtime 自动启动首轮工作。这是 #11284 之后的第二阶段。
为什么需要
此前用户需要手动复制
/goal set命令。新流程要求宿主支持且存在已连接的输入发起客户端；YOLO 和许可规则不能绕过明确同意。批准绑定原回合及审阅时的 Goal，进入 Plan 模式会撤销旧批准，退出 Plan 不会恢复它，需要重新批准。审阅者验证计划
前后证据
基线真实终端复现：批准后进入 Plan，再结束确认回答，仍创建活动 Goal 并自动运行三轮。确定性测试也复现了进入 Plan、进入后退出 Plan、加载 runtime 时进入 Plan 的错误创建；正常批准仅创建一次的对照测试通过。
最终 head
56f115f03f720c02805259bb1ff6c5af61265655的 Web Shell 构建已通过真实浏览器的正常批准、拒绝、批准后取消、批准后进入 Plan 四条路径。三条负向路径在释放迟到确认回答后仍没有 Goal；正常批准只创建一个 Goal 并开始自动工作，本地固定无进度响应随后触发三轮后的预期暂停。相同修复在最后一次仅合入上游改动前也通过真实终端 Plan 切换验证：没有设置 Goal，并向用户显示批准已撤销。本地验证通过：17 个相关测试文件共 4,270 项测试，以及构建、bundle、类型检查、针对性 lint、格式和 diff 检查。最后一次上游合并涉及会话队列清理，因此在已推送 head 上重跑了 914 项会话测试及受影响的构建、bundle 和类型检查。一轮限定范围的正确性审查未发现同意、回合归属、结算及自动启动路径中的可证实阻断项；这与远端 CI 及 GitHub 正式评审分开报告。真实界面的本地模型 fixture 验证确定性生命周期，不代表真实外部模型的起草质量。
测试平台与环境
macOS 本地验证；Windows、Linux 未本地验证。使用独立 worktree 和 Node.js 24.14.1；真实界面验证使用本地受控模型及独立配置、运行目录和工作区。远端 CI 与本地验证分别报告。
风险与范围
关联
后续阶段：#11284。延期的恢复提示改进：#11569。