Skip to content

feat(cli): adopt Goal v3 in non-interactive mode - #8324

Merged
wenshao merged 4 commits into
QwenLM:mainfrom
qqqys:agent/goal-v3-noninteractive
Aug 3, 2026
Merged

feat(cli): adopt Goal v3 in non-interactive mode#8324
wenshao merged 4 commits into
QwenLM:mainfrom
qqqys:agent/goal-v3-noninteractive

Conversation

@qqqys

@qqqys qqqys commented Aug 1, 2026

Copy link
Copy Markdown
Collaborator

What this PR does

This PR moves non-interactive CLI /goal commands onto the canonical Goal v3 runtime. Status, create, replace, edit, pause, resume, and clear now return the same persisted v2 state used by the interactive clients, and stream-json consumers receive an ordered goal_state event while the legacy active_goal projection remains available when partial messages are enabled.

Headless Goal workers now use Core-issued permits across model and tool-result segments, flush transcript evidence before releasing a permit, stream verification and terminal transitions, and terminate cleanly after update_goal without an unnecessary follow-up model call. Runtime-generated Goal continuations no longer consume the generic non-interactive session-turn limit, while explicit wall-clock and tool-call budgets continue to fail closed by pausing active work.

ACP intentionally remains on the legacy Goal command path in this split PR.

Why it's needed

The previous non-interactive path still used the legacy hook-backed Goal behavior and could not expose the canonical persisted lifecycle. It also had no consistent permit ownership or state stream for unattended clients. During E2E verification, a terminal update_goal additionally exposed a real exit-path defect: the worker could finish without emitting the normal success result and exit 0. This change closes that gap and aligns headless execution with the already-landed Goal v3 runtime contract.

Reviewer Test Plan

How to verify

Run /goal in non-interactive text mode with no active Goal and confirm it returns No Goal is set. without contacting the model. Run the same command with stream-json and confirm the canonical idle v2 state appears before the assistant summary and success result.

Create a Goal that requires an exact response followed by get_goal and update_goal. Confirm the worker is admitted with a Core permit, the requested content is delivered, verification and terminal states are streamed, no extra model turn follows the terminal tool call, and the process emits a success result with exit 0.

Pause and resume the same persisted session, then clear it. Confirm the same session state is restored, resume schedules a fresh exact permit, and clear completes without confirmation. Finally, apply a short explicit wall-clock budget and confirm an over-budget Goal pauses, emits the paused state, and exits with the budget-specific code rather than remaining active.

Evidence (Before & After)

Before: non-interactive /goal used the legacy hook projection, did not provide canonical v2 lifecycle events, and a terminal Goal tool call could fall through without a successful result envelope.

After: the observed stream is system/init → canonical goal_state → assistant/result for status, and active → running → verifying → terminal → success for a completed worker. A verifier wait interrupted by the explicit wall-clock budget produces paused/idle state and exit 55.

Tested on

OS Status
🍏 macOS
🪟 Windows ⚠️
🐧 Linux ⚠️

Environment (optional)

Node.js 24.14.1, local production bundle, no sandbox. Focused CLI tests: 280 passed and 1 existing skip. Full repository typecheck, modified-file ESLint, full build, and root bundle completed successfully.

Risk & Scope

  • Main risk or tradeoff: non-interactive clients that parse Goal output can now observe the canonical goal_state event in addition to the compatibility projection.
  • Not validated / out of scope: ACP conversion, Windows/Linux live E2E, verifier-provider reliability, and removal of Core's independent fixed 50-continuation cap.
  • Breaking changes / migration notes: text commands keep human-readable summaries; stream clients should adopt goal_state as authoritative and treat active_goal as a compatibility projection.

Linked Issues

N/A

中文说明

本 PR 做了什么

本 PR 将非交互 CLI 的 /goal 命令迁移到 canonical Goal v3 runtime。状态查询、创建、替换、编辑、暂停、恢复和清除现在都会返回与交互端一致的持久化 v2 状态;stream-json 消费方会收到有序的 goal_state 事件,同时在开启 partial messages 时继续保留旧的 active_goal 兼容投影。

Headless Goal worker 现在会在模型调用和工具结果续轮之间保持 Core 签发的 permit,在释放 permit 前刷新 transcript evidence,输出验证及终态变更,并在 update_goal 结束当前轮后直接完成,不再额外调用一次模型。runtime 自动产生的 Goal 续轮不再消耗非交互模式的通用 session turn 限制,但显式 wall-clock 和 tool-call 预算仍然生效,并会通过暂停活跃 Goal 的方式 fail closed。

作为拆分 PR 的边界,ACP 本次仍然保留旧 Goal 命令路径。

为什么需要

此前非交互模式仍使用基于 hook 的旧 Goal 行为,无法提供 canonical 持久化生命周期,也缺少适用于无人值守客户端的一致 permit 所有权和状态流。E2E 验证还发现了一个真实退出路径缺陷:终态 update_goal 之后,worker 可能结束但没有输出正常 success result 和退出码 0。本次修改修复该问题,并让 headless 执行与已合入的 Goal v3 runtime 契约对齐。

Reviewer 测试计划

如何验证

在没有活跃 Goal 时用非交互文本模式执行 /goal,确认输出 No Goal is set. 且不请求模型。再使用 stream-json 执行相同命令,确认 canonical idle v2 状态出现在 assistant summary 和 success result 之前。

创建一个要求先精确回复、再调用 get_goalupdate_goal 的 Goal。确认 worker 使用 Core permit 准入,指定内容被输出,验证和终态被写入状态流,终态工具调用后没有多余模型轮次,进程最终输出 success result 并以 0 退出。

在同一持久化会话中暂停、恢复并清除 Goal。确认会话状态能够恢复,resume 调度新的精确 permit,clear 不弹确认。最后设置较短的显式 wall-clock 预算,确认超时 Goal 会暂停、输出 paused 状态,并使用预算专用退出码结束,而不是继续保持 active。

前后证据

修改前:非交互 /goal 使用旧 hook 投影,不输出 canonical v2 生命周期事件,且终态 Goal 工具调用可能漏掉成功结果封包。

修改后:状态查询的实测输出顺序为 system/init → canonical goal_state → assistant/result;完成 worker 的链路为 active → running → verifying → terminal → success。显式 wall-clock 预算中断 verifier 等待时会输出 paused/idle 状态并以 55 退出。

测试平台

OS 状态
🍏 macOS
🪟 Windows ⚠️
🐧 Linux ⚠️

环境

Node.js 24.14.1,本地 production bundle,无 sandbox。聚焦 CLI 测试 280 个通过、1 个既有 skip。完整仓库 typecheck、改动文件 ESLint、全量 build 和根 bundle 均通过。

风险与范围

  • 主要风险或取舍:解析 Goal 输出的非交互客户端现在会在兼容投影之外收到 canonical goal_state 事件。
  • 未验证或不在范围内:ACP 迁移、Windows/Linux 实机 E2E、verifier/provider 稳定性,以及移除 Core 中独立的固定 50 续轮上限。
  • 破坏性变更或迁移说明:文本命令继续使用人类可读摘要;stream 客户端应以 goal_state 为权威状态,并将 active_goal 视为兼容投影。

关联 Issue

@qqqys

qqqys commented Aug 1, 2026

Copy link
Copy Markdown
Collaborator Author

Goal v3 non-interactive CLI E2E report

Date: 2026-08-01
Branch: agent/goal-v3-noninteractive
Build: local dist/cli.js

Scope

  • Plain-text /goal status and clear
  • stream-json canonical Goal state contract
  • Goal creation, worker admission, get_goal, update_goal, verification, terminal result
  • Explicit wall-clock budget and fail-closed pause
  • Same-session recovery with --resume
  • Legacy ACP isolation (unit boundary; ACP conversion is a later PR)

Results

1. Empty status, text output

node dist/cli.js -p "/goal" --output-format text

Result: exit 0, No Goal is set. No model request was made.

2. Empty status, stream JSON

node dist/cli.js -p "/goal" --output-format stream-json

Result: exit 0. The stream emitted, in order:

  1. system/init
  2. stream_event with event.type = goal_state and { v: 2, goal: null, activity: "idle" }
  3. assistant summary
  4. success result

The canonical Goal event is emitted even when partial messages are disabled.

3. Create and propose completion

node dist/cli.js -p "/goal Reply exactly GOAL_E2E_OK, then read the current goal and mark it complete." --output-format stream-json --max-wall-time 90s

Observed chain:

  1. create: active/idle
  2. worker admission: active/running
  3. assistant delivered GOAL_E2E_OK
  4. get_goal succeeded
  5. update_goal queued a proposal
  6. active/verifying
  7. terminal Goal state
  8. success result and exit 0

The first run exposed a real defect: after update_goal, the process fell out of the worker loop without the common terminal-result path and exited 1. The implementation now routes the terminated Goal turn through normal finalization. A regression test asserts exit 0 and no extra model call.

The live verifier was non-deterministic across provider calls: one verification accepted; later calls produced usage_limited for request abort or invalid JSON. These are Core verifier/provider outcomes, not CLI transport failures. In every case the final Goal state and result envelope were emitted and the process closed deterministically.

4. Same-session recovery

node dist/cli.js --resume d3a065bf-92e9-4516-82ff-d8cfacb75a89 -p "/goal" --output-format stream-json
node dist/cli.js --resume d3a065bf-92e9-4516-82ff-d8cfacb75a89 -p "/goal resume" --output-format stream-json --max-wall-time 90s

Result: the original session ID was preserved; the persisted usage_limited snapshot was restored, /goal resume changed it to active, admitted an exact runtime permit, ran the worker, verified, emitted the terminal state, emitted a success result, and exited 0.

5. Explicit wall-clock budget

node dist/cli.js -m qwen3.6-flash -p "/goal Reply exactly OK, then read the current goal and mark it complete." --output-format stream-json --max-wall-time 75s

Result: repeated verifier rejection kept the Goal active until the explicit wall-clock budget fired. The active turn was paused, a paused/idle Goal state was emitted, the run reported Run aborted: wall-clock budget of 75s exceeded (--max-wall-time)., and exited 55.

The code now also interrupts a Goal while finishTurn is awaiting verification, so a verifier wait cannot bypass the explicit run budget. This has a focused regression test using a blocked settlement.

6. Clear without confirmation

node dist/cli.js --resume d3a065bf-92e9-4516-82ff-d8cfacb75a89 -p "/goal clear" --output-format text

Result: exit 0, Goal cleared. No confirmation prompt.

Automated verification

  • Focused CLI tests: 4 files, 280 passed (1 existing skip)
  • Full repository typecheck
  • Modified-file ESLint
  • Full repository build
  • Root bundle build

The final rebuilt bundle repeated the empty stream-json status check with the same canonical ordering and exit 0.

Follow-ups outside this PR

  • ACP still intentionally uses the legacy Goal path; canonical ACP handling is the next split PR.
  • Core still contains MAX_GOAL_CONTINUATION_TURNS = 50. This PR removes the generic non-interactive session-turn charge from runtime Goal continuations, but does not remove the independent Core cap. The explicit product decision to have no fixed 50-turn limit requires a separate narrow Core PR.
  • Provider/verifier failures (Request was aborted, invalid JSON, or model-specific evidence-policy drift) correctly become terminal/retryable Goal states, but verifier reliability should be hardened separately from the CLI transport conversion.

@qwen-code-ci-bot

qwen-code-ci-bot commented Aug 1, 2026

Copy link
Copy Markdown
Collaborator

Qwen Triage finishedview run. See the stage comments in this thread for the result.

Qwen Triage 已完成 —— 查看运行。结果见本线程中的各阶段评论。

@qwen-code-ci-bot

qwen-code-ci-bot commented Aug 1, 2026

Copy link
Copy Markdown
Collaborator

Thanks for the PR!

Template looks good ✓ — every section is present, with a real reviewer test plan and a before/after.

Problem: this is a feature adoption, and the gap is a real one. Non-interactive /goal was still on the legacy hook-backed projection while interactive clients already moved to the landed Goal v3 runtime, so headless consumers had no canonical persisted lifecycle or permit ownership. The description also calls out an observed exit-path defect found during E2E — a terminal update_goal could finish without emitting the success result / exit 0 — which is a concrete reproduction, not a theoretical concern.

Direction: aligned. Goal v3 is already in core (goals/goal-runtime.ts, goals/goal-protocol.ts; I confirmed GoalRuntime, GoalTurnHost, GoalTurnPermit, GoalSnapshotV2, config.getGoalRuntimeReady/bindGoalTurnHost are all landed on main), and this PR adopts that infrastructure on the headless surface instead of reinventing it. Direction signal: Claude Code's CHANGELOG lists /goal as working "in interactive, -p, and Remote Control", so non-interactive is a first-class /goal surface in the reference agent. One thing for the maintainer's awareness: this is an additive public-contract change — stream-json consumers gain a goal_state event while active_goal is kept as a compatibility projection.

Size: not a core-module PR — every file lives in packages/cli (single package, no core/auth/providers/models/config/tools/services paths). Roughly ~543 production lines (the bulk in nonInteractiveCli.ts, ~485) against ~1014 test lines, so tests outweigh the change. Below the large-PR advisory; no core escalation.

Approach: the scope feels right for a runtime adoption — the command wiring (goalCommand.ts, nonInteractiveCliCommands.ts), the deduped goal_state stream event, and the permit/turn lifecycle in nonInteractiveCli.ts. The lifecycle plumbing is the feature, not scope creep, and the enforceSessionTurnLimit / routeAbort consolidations are tied to it (runtime continuations are excluded from the generic session-turn limit; abort fails closed). Deferring ACP to a follow-up split PR is good discipline. I don't see a materially simpler path given the permit lifecycle is inherent.

Risk: Stage 1e flags packages/cli/src/acp-integration/session/Session.ts as a high-risk path. The actual change there is a 5-line defensive throw (ACP stays on the legacy path), so the risk is low — but per policy this PR gets full CI evidence before approval and no skipped review depth. The added goal_state stream event is the other surface to watch.

Moving on to code review. 🔍

中文说明

感谢贡献!

模板完整 ✓——各节齐全,有真实的 reviewer 测试计划和 before/after。

**问题:**这是功能迁移,缺口真实存在。非交互 /goal 仍停留在旧的 hook 投影上,而交互端已经迁移到已合入的 Goal v3 runtime,因此 headless 消费方拿不到 canonical 持久化生命周期和 permit 所有权。描述中还提到 E2E 期间观测到的一个真实退出路径缺陷——终态 update_goal 可能结束但不输出 success result / exit 0——这是具体复现,不是理论性问题。

**方向:**对齐。Goal v3 已经在 core 中(goals/goal-runtime.tsgoals/goal-protocol.ts;我已确认 GoalRuntimeGoalTurnHostGoalTurnPermitGoalSnapshotV2config.getGoalRuntimeReady/bindGoalTurnHost 都已在 main 上合入),本 PR 是在 headless 侧采用这套基础设施,而不是重新造轮子。方向信号:Claude Code 的 CHANGELOG 明确写到 /goal "Works in interactive, -p, and Remote Control",即非交互是参考 agent 中 /goal 的一等场景。需要维护者留意的一点:这是一个附加式的公共契约变更——stream-json 消费方会新增 goal_state 事件,同时保留 active_goal 作为兼容投影。

**规模:**不是核心模块 PR——所有文件都在 packages/cli(单一 package,未触及 core/auth/providers/models/config/tools/services 路径)。约 543 行生产代码(主要在 nonInteractiveCli.ts,约 485 行),对应约 1014 行测试代码,测试多于改动。低于大 PR 提示线;无需核心升级。

**方案:**对于一次 runtime 迁移来说范围合理——命令接线(goalCommand.tsnonInteractiveCliCommands.ts)、去重的 goal_state 流事件,以及 nonInteractiveCli.ts 中的 permit/turn 生命周期。生命周期管线本身就是这个功能,不算 scope creep;enforceSessionTurnLimit / routeAbort 的整合也与之相关(runtime 续轮被排除在通用 session-turn 限制之外;abort 时 fail closed)。把 ACP 延后到单独的拆分 PR 是很好的纪律。考虑到 permit 生命周期本身不可避免,我没有看到明显更简的路径。

**风险:**Stage 1e 命中 packages/cli/src/acp-integration/session/Session.ts 这一高风险路径。该处实际改动只是 5 行防御性 throw(ACP 仍走旧路径),所以风险很低——但按政策,本 PR 在批准前需要完整的 CI 证据,且不跳过任何 review 深度。新增的 goal_state 流事件是另一个需要关注的面。

进入代码审查 🔍

Qwen Code · qwen3.8-max-preview

Reviewed at 7abb7f8d8636ed4754b67e38d5eaa7056fc6bc05 · re-run with @qwen-code /triage

@qwen-code-ci-bot

qwen-code-ci-bot commented Aug 1, 2026

Copy link
Copy Markdown
Collaborator

Code review

I went in looking for the usual fork-PR over-reach and came away impressed. The change does exactly what an adoption of the landed Goal v3 runtime should do, and nothing extra. My independent take before reading the diff was: flip the goalCommand mode guard, wire the goal_control result through the headless command handler, surface a goal_state stream event (keeping active_goal for compat), and implement the permit/turn lifecycle in runNonInteractive with fail-closed release on every exit path. That is precisely this PR — I didn't find a simpler path it missed, because the permit lifecycle is inherent to adopting the runtime.

No correctness blockers, security holes, or convention violations. Things I checked specifically and was satisfied with:

  • Fail-closed permit release. failClosedActiveGoalTurn is invoked on every terminal path — routeAbort, structured-success, loop-detected, the catch, and the finally — and is idempotent via the settlingGoalTurn/goalTurnSettlement guard. Before pausing or finishing it re-checks permit identity with sameGoalPermit, so it can't act on a stale permit. A permit cannot leak if the worker dies mid-turn.
  • The exit-path fix is real. The terminateTurn flag (set by core goal-tools when a Goal is readyForVerification) now propagates out of processToolCallBatch; on a terminal update_goal the worker adds the tool result to history, flushes the transcript, releases the permit via finishGoalTurn, and finalizes into the normal success path instead of falling through. The ends a Goal turn without another model call after update_goal test pins exactly this.
  • Turn-limit semantics. enforceSessionTurnLimit returns early for origin === 'runtime' continuations (so they don't burn the generic session cap) while still counting real user turns, and the explicit wall-clock / tool-call budgets still fail closed through routeAbort — each behavior has a dedicated test.
  • Reuse, not reinvention. I verified every core symbol it imports (GoalRuntime, GoalTurnHost, GoalTurnPermit, GoalSnapshotV2, GoalPersistenceUnavailableError, GoalStateResponse, getGoalRuntimeReady, bindGoalTurnHost, permitForTurn, getVerifierFeedback) is already landed in packages/core/src/goals/ on main. The CLI is a consumer here, not a parallel implementation.

Two non-blocking notes for the author, neither worth holding the PR over:

  • The goal_state dedup keys on JSON.stringify(event.value), which is key-order sensitive. Fine today because snapshots come from a single runtime producer, but worth knowing if a second producer ever appears.
  • projectLegacyActiveGoal hardcodes tokensAtStart: 0, so legacy consumers reading token counts off the compatibility projection will see zero. Acceptable for a deprecated projection, just flagging it.

Test coverage is the strong point: ~1014 test lines against ~543 production, using the real createGoalRuntime rather than mocking the runtime away — permit continuity, flush-before-release, fail-closed budget interruption, stream ordering, and dedup are all asserted.

sequenceDiagram
    participant P1 as User
    participant P2 as CLI runNonInteractive
    participant P3 as GoalRuntime in Core
    participant P4 as Model
    participant P5 as StreamJsonOutputAdapter
    P1->>P2: goal set, or a user query
    P2->>P3: claim permit and beginTurn
    P3-->>P2: GoalTurnPermit
    P2->>P4: send segment carrying goalPermit
    P4-->>P2: update_goal result with terminateTurn
    P2->>P3: flush transcript then finishTurn
    P2->>P5: emit goal_state then active_goal
    P5-->>P1: stream_event goal_state
    P2-->>P1: success result, exit 0
Loading

On any budget expiry, abort, or error, the CLI instead pauses the active Goal and releases the permit before emitting the terminal envelope (the fail-closed path).

Files changed (10)
File What changed
packages/cli/src/ui/commands/goalCommand.ts flips the mode guard so non-interactive uses canonical Goal control; only ACP stays restricted
packages/cli/src/nonInteractiveCliCommands.ts wires the goal_control result through the headless handler instead of returning unsupported
packages/cli/src/nonInteractive/types.ts adds GoalStateStreamEvent to the StreamEvent union
packages/cli/src/nonInteractive/io/StreamJsonOutputAdapter.ts emits a goal_state stream event, deduped by snapshot signature
packages/cli/src/nonInteractiveCli.ts the bulk: permit-based goal turn lifecycle, user and runtime admission, fail-closed release, terminateTurn exit handling, turn-limit exclusion
packages/cli/src/acp-integration/session/Session.ts defensive throw for goal_control since ACP stays on the legacy path
packages/cli/src/nonInteractiveCli.test.ts permit lifecycle, fail-closed, turn-limit, and exit-path tests against the real runtime
packages/cli/src/nonInteractive/io/StreamJsonOutputAdapter.test.ts goal_state ordering and dedup tests
packages/cli/src/nonInteractiveCliCommands.test.ts canonical goal_control result tests for create, status, clear
packages/cli/src/ui/commands/goalCommand.test.ts ACP legacy-path handling tests

Testing

This is an unattended CI run, so I did not drive the product locally — the signal below is the PR's own CI on the reviewed commit, read through the API. No local real-scenario (tmux) testing was run on this path.

CI is settled and green on the reviewed commit: across all 75 check-runs there are 0 failures and 0 still pending (57 skipped, 18 success). The main Linux unit suite Test (ubuntu-latest, Node 22.x) is green, as are the web-shell E2E smoke, the real daemon E2E, and the SDK Java matrix. The macOS/Windows Node unit tests and the CLI integration tests are skipped in this run by design (not failing). The finalize workflow keeps the table below current.

Check Conclusion
Test (ubuntu-latest, Node 22.x) ✅ success
web-shell E2E Smoke (ubuntu-latest, Node 22.x) ✅ success
Real daemon E2E / Java 11 ✅ success
Classify PR ✅ success
precheck-pr / precheck ✅ success
ubuntu-latest / Java 11 ✅ success
ubuntu-latest / Java 17 ✅ success
ubuntu-latest / Java 21 ✅ success
macos-latest / Java 21 ✅ success
windows-latest / Java 21 ✅ success

Sandboxed verification would still settle one thing the unit suite cannot: @qwen-code /verify — that a real qwen -p process actually exits 0 after a terminal update_goal with the system/init → goal_state → assistant/result ordering. The in-process tests pin the lifecycle against the real runtime, but the process-level exit code and stream ordering across platforms is the author's macOS-only E2E (their claim, not independently re-run here), and the macOS/Windows Node suites are skipped in this run. As a fork PR this would be a sponsored run — a maintainer's @qwen-code /verify approves the head it's written against and carries a pre-execution risk screen plus a workspace wipe, so read the resulting report with the same skepticism as the fork's own CI logs.

中文说明

代码审查

我本是带着挑 fork PR 毛病的心态进来的,结果印象很好。这次改动恰好做了一次 Goal v3 runtime 迁移该做的事,没有任何多余。我在读 diff 之前的独立方案是:放开 goalCommand 的模式限制、把 goal_control 结果接进 headless 命令处理器、暴露 goal_state 流事件(同时保留 active_goal 兼容)、并在 runNonInteractive 中实现每条退出路径都 fail-closed 释放的 permit/turn 生命周期。这正是本 PR 所做的——我没有找到它漏掉的更简路径,因为 permit 生命周期是采用 runtime 所固有的。

没有正确性阻塞、安全漏洞或规范违反。我专门核对并放心的几点:

  • Fail-closed 的 permit 释放。failClosedActiveGoalTurn 在每条终态路径上都被调用——routeAbort、structured-success、loop-detected、catchfinally——并通过 settlingGoalTurn/goalTurnSettlement 守卫保证幂等。在 pause 或 finish 之前会用 sameGoalPermit 重新核对 permit 身份,因此不会对过期 permit 操作。worker 在 turn 中途死掉也不会泄漏 permit。
  • 退出路径修复是真实的。terminateTurn 标志(由 core goal-tools 在 Goal readyForVerification 时设置)现在会从 processToolCallBatch 传出;终态 update_goal 时,worker 会把工具结果写入历史、刷新 transcript、通过 finishGoalTurn 释放 permit,并落入正常 success 路径,而不是漏掉。ends a Goal turn without another model call after update_goal 测试正是钉住了这一点。
  • Turn 限制语义。enforceSessionTurnLimitorigin === 'runtime' 的续轮提前返回(不消耗通用 session 上限),同时仍统计真实用户轮次;显式 wall-clock / tool-call 预算仍通过 routeAbort fail closed——每种行为都有专门测试。
  • **复用而非重造。**我核对了它导入的每个 core 符号(GoalRuntimeGoalTurnHostGoalTurnPermitGoalSnapshotV2GoalPersistenceUnavailableErrorGoalStateResponsegetGoalRuntimeReadybindGoalTurnHostpermitForTurngetVerifierFeedback)都已在 mainpackages/core/src/goals/ 中合入。这里 CLI 是消费方,而不是平行实现。

给作者两个非阻塞提示,都不足以卡住 PR:

  • goal_state 去重以 JSON.stringify(event.value) 为键,对键顺序敏感。目前因为快照来自单一 runtime 生产者所以没问题,但如果将来出现第二个生产者就值得注意。
  • projectLegacyActiveGoaltokensAtStart 硬编码为 0,因此从兼容投影读取 token 数的旧消费方会看到 0。对一个已弃用的投影来说可以接受,仅提示。

测试覆盖是亮点:约 1014 行测试对约 543 行生产代码,使用真实createGoalRuntime 而非把 runtime mock 掉——permit 连续性、释放前刷新、fail-closed 预算中断、流顺序和去重都有断言。

(流程图:用户 → CLI runNonInteractive 申领 permit 并 beginTurn → Core GoalRuntime 返回 GoalTurnPermit → 携带 goalPermit 发给模型 → 模型返回带 terminateTurn 的 update_goal 结果 → 刷新 transcript 后 finishTurn → 先发 goal_state 再发 active_goal → 用户收到 goal_state 流事件与 success result,exit 0。任何预算耗尽、abort 或错误时,CLI 改为暂停活跃 Goal 并在输出终态封包前释放 permit,即 fail-closed 路径。)

测试

这是无人值守的 CI 运行,因此我没有在本地驱动产品——下面的信号是通过 API 读取的、本 PR 自身在被审 commit 上的 CI。本路径未运行本地真实场景(tmux)测试。

CI 在被审 commit 上已落定且全绿:全部 75 个 check-run 中 0 失败、0 仍在运行(57 跳过、18 成功)。主 Linux 单测套件 Test (ubuntu-latest, Node 22.x) 为绿,web-shell E2E smoke、real daemon E2E 以及 SDK Java 矩阵 likewise 全绿。macOS/Windows 的 Node 单测和 CLI 集成测试在本次运行中按设计被跳过(并非失败)。finalize workflow 会持续更新下表。

(CI 表格见上方区域标记内,由 finalize 任务在 CI 完成后自动更新。)

沙箱验证仍能补上单测无法覆盖的一点:@qwen-code /verify——验证真实 qwen -p 进程在终态 update_goal 后确实以 0 退出,并具备 system/init → goal_state → assistant/result 的顺序。进程内测试已针对真实 runtime 钉住了生命周期,但跨平台的进程级退出码与流顺序是作者仅在 macOS 上的 E2E(是其自述,未在此独立复跑),且本次 macOS/Windows Node 套件被跳过。作为 fork PR,这将是一次赞助运行——维护者的 @qwen-code /verify 会批准其写入时所对的 head,并带有执行前风险筛查与工作区清空,因此请以看待 fork 自身 CI 日志同样的审慎态度阅读其报告。

Qwen Code · qwen3.8-max-preview

Reviewed at 7abb7f8d8636ed4754b67e38d5eaa7056fc6bc05 · re-run with @qwen-code /triage

@qwen-code-ci-bot

qwen-code-ci-bot commented Aug 1, 2026

Copy link
Copy Markdown
Collaborator

Confidence: 3/5 — a clean, well-tested adoption that I'm comfortable with on the merits, and CI is now green on this commit; the 3 is not correctness doubt, it's that a fork PR adding a public stream event deserves a maintainer's sign-off rather than a bot auto-approve.

Stepping back: my independent proposal before reading the diff was essentially this PR — flip the mode guard, wire goal_control through, surface goal_state while keeping the active_goal projection, and build the permit/turn lifecycle with fail-closed release. I didn't find a simpler path it missed; the permit machinery is inherent to adopting the runtime, not accidental complexity. It solves a real gap (headless /goal was stuck on the legacy hook path while interactive moved to v3, and the reference agent ships /goal in -p mode), every edit in the diff earns its place, and deferring ACP to a split PR is the right discipline. If I were maintaining this in six months I'd thank the author — the fail-closed net on every terminal path and the real-runtime test coverage are exactly the habits that keep a subsystem like this trustworthy.

What changed since my last pass: the head moved to 7abb7f8 and the Linux unit suite that was still running before is now green, with 0 failures and 0 pending across all 75 check-runs. So one of my earlier three reasons (unsettled CI) is resolved. Two remain, and together they say "a human should own this merge," not "this is wrong":

  1. Public contract. stream-json consumers gain a goal_state event. It's additive and active_goal is retained, but it's still a contract change to a headless interface that external tooling parses, and our direction guidance is to put a maintainer's eyes on public-contract touches rather than auto-landing them.
  2. Behaviour rests partly on the author's word. The unit suite is strong and runs the real runtime, but the process-level claim — a real qwen -p exits 0 after a terminal update_goal, with the right stream ordering, cross-platform — is the author's macOS-only E2E, and the macOS/Windows Node suites are skipped in this run. A sponsored @qwen-code /verify (named in my review above) would close that.

Neither is the author's fault, and neither is blocking in the "this needs rework" sense — they're the "this is good enough that the only thing left is a human decision" kind. So I'm parking it here rather than approving or requesting changes.

⏸️ Deferring to @wenshao (core / Goal owner) — the code is in good shape and CI is green; what needs a human call is signing off on the additive goal_state stream-contract change and merging a fork PR that touches the acp-integration path. A sponsored @qwen-code /verify would, in my view, make this ready. Needs a maintainer's eye on this one.

中文说明

置信度:3/5——一次干净、测试充分的迁移,就代码本身我是放心的,而且本 commit 上 CI 现已全绿;这个 3 分不是对正确性有疑虑,而是一个 fork PR 新增了公共流事件,应该由维护者签字,而不是由机器人自动批准。

退一步看:我在读 diff 之前的独立方案基本就是这个 PR——放开模式限制、把 goal_control 接上、暴露 goal_state 同时保留 active_goal 投影、并构建带 fail-closed 释放的 permit/turn 生命周期。我没有找到它漏掉的更简路径;permit 机制是采用 runtime 所固有的,不是多余的复杂度。它解决了一个真实缺口(headless /goal 还停在旧 hook 路径,而交互端已经迁移到 v3,且参考 agent 在 -p 模式下也提供 /goal),diff 中每一处改动都物有所值,把 ACP 延后到拆分 PR 也是正确的纪律。如果半年后由我来维护,我会感谢作者——每条终态路径上的 fail-closed 兜底和针对真实 runtime 的测试覆盖,正是让这类子系统保持可信的习惯。

自上次审查以来的变化:head 移动到了 7abb7f8,之前仍在运行的 Linux 单测套件现在已全绿,全部 75 个 check-run 中 0 失败、0 待运行。因此我早先三个理由中的一个(CI 未落定)已解决。还剩两个,它们叠加起来指向"应由人来决定这次合并",而不是"这有错":

  1. 公共契约。stream-json 消费方会新增 goal_state 事件。它是附加式的、active_goal 也保留了,但这仍然是一个被外部工具解析的 headless 接口的契约变更,而我们的方向指引是:公共契约变更应交给维护者过目,而不是自动合入。
  2. **行为部分依赖作者自述。**单测很扎实且运行真实 runtime,但进程级结论——真实 qwen -p 在终态 update_goal 后以 0 退出、并有正确的流顺序、且跨平台——是作者仅在 macOS 上的 E2E,且本次 macOS/Windows Node 套件被跳过。一次赞助的 @qwen-code /verify(见上方 review)可以补上这一点。

这两点都不是作者的问题,也不是"需要返工"意义上的阻塞——它们属于"代码已经足够好,剩下的只是一个人为决定"那一类。所以我把它停在这里,而不是批准或请求修改。

⏸️ 转交 @wenshao(core / Goal 负责人)——代码状态良好且 CI 已绿;需要人来拍板的是为附加式的 goal_state 流契约变更签字,以及合并一个触及 acp-integration 路径的 fork PR。在我看来,一次赞助的 @qwen-code /verify 就能让它就绪。这个 PR 需要维护者过目。

Qwen Code · qwen3.8-max-preview

Reviewed at 7abb7f8d8636ed4754b67e38d5eaa7056fc6bc05 · re-run with @qwen-code /triage

@wenshao

wenshao commented Aug 1, 2026

Copy link
Copy Markdown
Collaborator

Review

Overview

This PR moves non-interactive /goal onto the canonical Goal v3 runtime: goalCommand now takes the canonical dispatch path for non_interactive (only ACP stays on the legacy hook path), nonInteractiveCliCommands forwards the new goal_control result, and runNonInteractive gains a headless Goal turn host — Core-issued permits held across model/tool-result segments, evidence flush before permit release, fail-closed pause on abort/error/exit, a terminateTurn fast path after terminal update_goal, and a goal_state stream event ordered before the legacy active_goal projection. Runtime-origin continuations are exempted from the generic --max-session-turns cap while explicit wall-clock/tool budgets still fail closed.

I verified the key runtime contracts against Core: bindHost() synchronously flushes the queued continuation into startGoalTurn (the host's push into queuedGoalTurns happens before any await), so the queuedGoalTurns.shift() right after bindGoalHost() in the goal_control worker path is sound today. permitForTurn/getVerifierFeedback/finishTurn usage matches the runtime API, and the base JSON/text adapters ignore the new GoalState event via their default branch, so non-stream outputs stay clean.

Strengths

  • The terminal update_goal fix is correct and well-shaped: tool responses are recorded via addHistory, the transcript is flushed, finishTurn releases the permit, and the loop falls through to the normal success envelope with no extra model call (covered by a dedicated test).
  • Permit hygiene is careful throughout: sameGoalPermit guards before every finishTurn, failClosedActiveGoalTurn is idempotent and wired into routeAbort, the catch block, emitStructuredSuccess, loop detection, and the outer finally, and flush-before-release ordering is asserted by test.
  • Test coverage is strong: table-driven control cases run against the real createGoalRuntime (not mocks), plus permit-threading across ToolResult continuations, budget fail-closed behavior, event ordering, and user-turn admission ordering.
  • goal_state dedup by snapshot signature prevents double emission from the explicit emit + subscription overlap.

Findings

  1. Unbounded-by-default headless Goal loops (nonInteractiveCli.ts, enforceSessionTurnLimit): runtime-origin segments skip --max-session-turns, so a headless /goal <objective> with default flags is bounded only by Core's fixed 50-continuation cap. That is acknowledged as out of scope, but it changes the cost profile of CI/unattended runs; worth calling out in the headless/stream-json docs (neither goal_state nor active_goal is documented anywhere in docs/ today, even though the PR declares goal_state authoritative for stream consumers).
  2. claimUserGoalTurn has no timeout (nonInteractiveCli.ts): if beginTurn denies the permit while the goal stays active, the user turn waits on runtime state changes with only the abort signal as an exit. Today a fresh headless runtime is always idle so denial appears unreachable, but the hang mode is latent — nothing else in the process will grant the permit if that assumption ever breaks. A defensive timeout (or at least a debug log when entering the wait) would make failures diagnosable.
  3. Sync-scheduling coupling (goal_control case): queuedGoalTurns.shift() immediately after bindGoalHost() relies on Core's flushContinuation invoking startGoalTurn synchronously. That holds today, but if Core ever defers scheduling to a microtask this becomes FatalInputError: The Goal runtime did not schedule a continuation. A short comment stating the synchronous contract (here or on GoalTurnHost) would protect the invariant.
  4. Dedup asymmetry in the stream (StreamJsonOutputAdapter.ts): goal_state is deduped by signature but the paired legacy active_goal emission is not, so with partial messages enabled a suppressed duplicate goal_state can still produce a repeated active_goal. Harmless for the stated contract (legacy is a compatibility projection), just an observable inconsistency.
  5. startGoalTurn dedup checks only the queue, not the currently active turn. Unreachable today because flushContinuation mints a fresh turnId per start — a note-level observation.
  6. Test hygiene: the two budget tests end with void run; — the caught run promise can settle after the test finishes. Awaiting it (post-assertion) would avoid cross-test bleed. Also, GoalPersistenceUnavailableError is silently swallowed during user-turn admission; intentional, but a debugLogger line would help field diagnosis, and there is no headless test for the untrusted-folder rejection on set/edit/resume.

Security / compatibility

  • The trusted-workspace boundary is preserved: set/edit/resume still require isTrustedFolder() on the canonical path, matching the legacy hook gate.
  • ACP is explicitly fenced off (goal_control throws in ACP Session.ts, and goalCommand keeps the legacy branch for executionMode === 'acp'), so the split-PR boundary is enforced rather than implied.
  • Stream consumers gain a new goal_state event unconditionally in stream-json; the PR body flags this correctly as the main compatibility surface.

Verdict

Solid, carefully-guarded change with unusually good test discipline for this area. Findings above are non-blocking (docs for goal_state, a comment on the sync-scheduling invariant, and the claimUserGoalTurn wait being the ones I'd most like to see addressed). LGTM once those are considered.

@doudouOUC doudouOUC left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed. Suggestions are inline.

— qwen3.7-max via Qwen Code /review

Comment on lines +261 to 262
if (context.executionMode === 'acp') {
const operation = parseGoalCommand(args);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] No test covers the remaining rejection path — /goal pause, /goal resume, or /goal edit in ACP mode. The guard changed from !== 'interactive' to === 'acp', but the old tests that verified rejection in non-interactive mode were converted to acceptance tests for the new canonical path, leaving the ACP rejection untested.

— Failure scenario: a regression that changes or removes the === 'acp' condition (e.g. back to !== 'interactive', or a typo like === 'ACP') would silently allow /goal pause in ACP mode through the legacy path, or block it in non-interactive mode. The existing test suite would not catch this.

Suggested change
if (context.executionMode === 'acp') {
const operation = parseGoalCommand(args);
// Add parameterized test in goalCommand.test.ts:
it.each(['pause', 'resume', 'edit revised'] as const)(
'rejects /goal %s in ACP mode',
async (subCmd) => {
const ctx = makeContext({ executionMode: 'acp' });
const result = await goalCommand.action(ctx, subCmd);
expect(result).toEqual(
expect.objectContaining({
type: 'message',
messageType: 'error',
content: expect.stringMatching(/not available in ACP/i),
}),
);
},
);

— qwen3.7-max via Qwen Code /review

Comment on lines +588 to +593
preemptGoalTurn: (reason) => {
for (const turn of queuedGoalTurns.splice(0)) {
turn.controller.abort(reason);
}
activeGoalTurn?.controller.abort(reason);
},

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] The headless preemptGoalTurn implementation is not exercised by any headless test. The runtime calls preemptGoalTurn when a goal is edited or cleared while a turn is in flight, but no test in nonInteractiveCli.test.ts triggers this scenario for the headless host.

— Failure scenario: a bug in the splice logic (off-by-one) or a failure to abort the active turn's controller would not be caught. The headless run could continue executing a superseded goal turn after the user issued /goal edit or /goal clear.

Consider adding a test where a goal turn is in flight and a dispatch({ action: 'edit', ... }) triggers preemption, asserting the active turn's controller signal is aborted.

— qwen3.7-max via Qwen Code /review

Comment on lines +1332 to +1334
} catch (error) {
if (!(error instanceof GoalPersistenceUnavailableError)) {
throw error;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] The GoalPersistenceUnavailableError silent degradation path is not tested. When the goal-runtime setup throws this error, the catch block swallows it and the run continues without a goal turn — but no test configures getGoalRuntimeReady() to throw it.

— Failure scenario: if the error class name is renamed or the import path changes, the instanceof check would silently stop matching, and the error would propagate as an unhandled exception instead of being gracefully degraded.

Consider adding two tests: one that makes getGoalRuntimeReady throw GoalPersistenceUnavailableError and asserts the run completes normally, and one that throws a different error and asserts it propagates.

— qwen3.7-max via Qwen Code /review

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed. Suggestions are inline. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally. Test Plan (not a blocker): tests: 280 passed — this review observed 16565 passed.

中文说明

已审查。 建议见行内评论。 未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。 Test Plan(非阻断):tests: 280 passed — this review observed 16565 passed

— qwen3.8-max-preview via Qwen Code /review (v0.21.3)

Comment on lines +2303 to +2305
await finishGoalTurn(activeGoalTurn);
activeGoalTurn = undefined;
const nextGoalTurn = queuedGoalTurns.shift();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] The goal-turn dequeue-and-continue sequence (queuedGoalTurns.shift() → assign activeGoalTurn → reset isFirstGoalSegment → rebuild currentMessages via buildGoalContinuationParts → clear hasUnsentToolResponsecontinue) is duplicated verbatim here (the terminateTurn fast path) and again in the shouldFinalizeTurn block below (~line 2333). — Concrete cost: a future change to the dequeue protocol (a new HeadlessGoalTurn field, an altered continuation-prompt format, or an extra per-segment flag reset) has to be applied in both sites; missing one makes the terminateTurn route set up the next goal segment differently from the normal no-tool-calls finalization route, producing inconsistent model context for the same logical operation.

// Extract once and call from both sites:
const activateNextGoalTurn = (): boolean => {
  const next = queuedGoalTurns.shift();
  if (!next) return false;
  activeGoalTurn = next;
  isFirstGoalSegment = true;
  currentMessages = [{ role: 'user', parts: buildGoalContinuationParts(next) }];
  hasUnsentToolResponse = false;
  return true;
};
中文说明

[建议] Goal turn 的「出队并继续」序列(queuedGoalTurns.shift() → 赋值 activeGoalTurn → 重置 isFirstGoalSegment → 用 buildGoalContinuationParts 重建 currentMessages → 清除 hasUnsentToolResponsecontinue)在此处(terminateTurn 快速路径)和下方 shouldFinalizeTurn 块(约 2333 行)逐字重复。— 具体代价:未来对出队协议的改动(新增 HeadlessGoalTurn 字段、修改续轮 prompt 格式、或重置额外的每段标志)必须同时应用到两处;漏掉一处会使 terminateTurn 路径与普通的「无工具调用」收尾路径对下一个 goal segment 的初始化不一致,从而对同一逻辑操作产生不一致的模型上下文。建议抽取一个本地辅助函数(如 activateNextGoalTurn(): boolean)并在两处调用。

— qwen3.8-max-preview via Qwen Code /review (v0.21.3)

Comment on lines +276 to +278
return new Promise((resolve, reject) => {
let settled = false;
let unsubscribe = () => {};

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] The async waiting path in claimUserGoalTurn (runtime busy → subscribe → wait for a permit) has no test coverage; only the immediate beginTurn-succeeds path is exercised by the existing "claims an active Goal for real user input before binding the host" test. — Concrete cost: if the subscribe/abort/settled plumbing has a race (e.g. the abort fires between subscribe and the first inspect), a headless run that sends real user input while a runtime goal turn is in flight could hang or resolve a stale permit, and no test would catch it.

// e.g. pre-occupy the runtime turn slot, then assert the wait resolves:
goalRuntime.beginTurn('occupying-turn');
const run = runNonInteractive(/* real user input */);
await goalRuntime.finishTurn(/* occupying permit */); // promotes the queued user turn
// also: abort mid-wait and assert a clean exit without a permit
中文说明

[建议] claimUserGoalTurn 中的异步等待路径(runtime 忙 → subscribe → 等待 permit)没有测试覆盖;现有的 "claims an active Goal for real user input before binding the host" 测试只覆盖了 beginTurn 立即成功的路径。— 具体代价:如果 subscribe/abort/settled 机制存在竞态(例如 abort 在 subscribe 和首次 inspect 之间触发),那么在 runtime goal turn 进行中发送真实用户输入的 headless 运行可能挂起或解析到过期的 permit,而没有任何测试能捕获。建议补一个测试:先占用 runtime 的 turn 槽,验证用户请求会等待并在占用 turn 结束后解析;再测试等待期间 abort 的路径。

— qwen3.8-max-preview via Qwen Code /review (v0.21.3)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

已修复。验证:cd packages/cli && npx vitest run src/nonInteractiveCli.test.ts(120 passed,1 skipped)与 git diff --check 均通过。

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed. Suggestions are inline. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally. Test Plan (not a blocker): tests: 280 passed — this review observed 16566 passed.

中文说明

已审查。 建议见行内评论。 未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。 Test Plan(非阻断):tests: 280 passed — this review observed 16566 passed

— qwen3.8-max-preview via Qwen Code /review (v0.21.3)

Comment on lines +563 to +564
await settleBeforeTerminalOutput();
await handleMaxTurnsExceededError(config);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] enforceSessionTurnLimit reaches process.exit (via handleMaxTurnsExceededErrorexitAfterCleanup) without first calling failClosedActiveGoalTurn, unlike every other terminal path in this diff (routeAbort, emitStructuredSuccess, emitLoopDetectedResult, the catch block, and the finally block all call it). Because process.exit does not unwind the stack, the finally-block safety net (~line 2928) is unreachable on this path. — Concrete cost: when the session turn limit is exceeded with an active goal turn, the goal is persisted as active rather than the paused the budget-exceeded and SIGINT paths produce, so the goal's post-exit state depends on why the process stopped instead of being uniformly fail-closed. The existing test masks this by mocking process.exit to throw, which unwinds the stack and lets the finally net run.

Suggested change
await settleBeforeTerminalOutput();
await handleMaxTurnsExceededError(config);
await failClosedActiveGoalTurn(
'Headless Goal stopped after the session turn limit',
);
await settleBeforeTerminalOutput();
await handleMaxTurnsExceededError(config);
中文说明

enforceSessionTurnLimit 会经由 handleMaxTurnsExceededErrorexitAfterCleanup 走到 process.exit,但没有先调用 failClosedActiveGoalTurn,而本 diff 中其它所有终止路径(routeAbortemitStructuredSuccessemitLoopDetectedResultcatch 块和 finally 块)都会调用它。由于 process.exit 不会展开调用栈,这条路径上 finally 块(约第 2928 行)的兜底逻辑不可达。 — 具体代价:当持有活跃 Goal turn 时超出 session turn 上限,Goal 会被持久化为 active,而不是 budget-exceeded 和 SIGINT 路径所产生的 paused,导致进程退出后的状态取决于退出原因,而非统一 fail-closed。现有测试通过把 process.exit mock 成抛异常掩盖了这一点——抛异常会展开调用栈,使 finally 兜底得以执行。

— qwen3.8-max-preview via Qwen Code /review (v0.21.3)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

已修复。验证:cd packages/cli && npx vitest run src/nonInteractiveCli.test.ts(120 passed,1 skipped)、npx eslint src/nonInteractiveCli.ts src/nonInteractiveCli.test.tsnpm run build && npm run typecheckgit diff --check HEAD^ HEAD 均通过。

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally. Test Plan (not a blocker): tests: 280 passed — this review observed 16566 passed.

中文说明

已审查。 未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。 Test Plan(非阻断):tests: 280 passed — this review observed 16566 passed

— qwen3.8-max-preview via Qwen Code /review (v0.21.3)

@wenshao

wenshao commented Aug 2, 2026

Copy link
Copy Markdown
Collaborator

Local E2E verification (macOS, real model)

Verified PR head 7abb7f8d8 in an isolated worktree with a production bundle (npm run bundle, Node v24.18.1) against a real OpenAI-compatible provider (qwen3.8-max-preview), no sandbox. All scenarios from the Reviewer Test Plan were exercised end-to-end with the actual CLI binary; stream-json output and the persisted session JSONL were used as the source of truth.

1. /goal status — no model contact, canonical event order ✅

  • Text mode with no active Goal prints No Goal is set. and exits 0.
  • Re-ran the same command with a deliberately unreachable endpoint (OPENAI_BASE_URL=http://127.0.0.1:9/none) — still succeeds, proving status never contacts the model.
  • stream-json emits system/init → canonical goal_state ({"v": 2, "goal": null, "activity": "idle"}) → legacy active_goal projection → assistant summary → result success, exactly as described.

status

2. Headless worker runs to verified completion ✅

Created a Goal requiring an exact response followed by get_goal and update_goal. Observed stream (session c5796f33): active/idleactive/running → exact content GOAL-V3-E2E-COMPLETE delivered → get_goalupdate_goal ("Proposal queued for independent verification at the turn boundary") → verifyingcompleteresult success, num_turns=2, exit 0 — no extra model turn after the terminal tool call (this is the exit-path defect the PR fixes). The session JSONL shows the canonical provenance chain: system prov=goal_control sub=goal_state records plus the runtime continuation recorded as user prov=goal_runtime (Core-permit-admitted turn).

lifecycle

3. Wall-clock budget fails closed; pause / resume / clear on the same persisted session ✅

  • --max-wall-time 30s with a long-running Goal: goal_state paused is streamed, stderr reports the budget, process exits 55 (budget-specific code), Goal is not left active.
  • -c "/goal" on the same project restores the same session (6eed223d…) and prints Goal paused: ….
  • -c "/goal resume" re-activates the Goal in the same session, schedules a fresh permit (worker re-admitted, active/running observed), and the budget fails closed again with exit 55.
  • -c "/goal clear" completes without any confirmation prompt; a final status check prints No Goal is set.

budget

4. Focused unit tests ✅

All four changed test files pass locally: 284 passed, 1 skipped (goalCommand 63, StreamJsonOutputAdapter 54, nonInteractiveCliCommands 47, nonInteractiveCli 121 | 1 skipped).

tests

Additional observations (fail-closed paths exercised incidentally)

  • 50-continuation cap: in one run the model refused to cooperate and looped; after 50 runtime continuations the Goal transitioned to usage_limited with reason Goal exceeded the 50-turn continuation budget, the stream stayed ordered, and the process still emitted a success envelope with exit 0. Confirms Core's independent cap still applies (the PR correctly lists its removal as out of scope). Note runtime continuations did not consume the generic session turn limit, as intended.
  • SIGTERM: killing the CLI mid-worker left the Goal paused (not active) in the persisted session — the fail-closed permit release works.
  • Verifier-provider reliability (explicitly out of scope in this PR): the Goal verifier runs as a side query on fastModel. With a fast model that was unreachable in my environment, verification failed and the Goal degraded to usage_limited (Goal verifier returned invalid JSON / Request was aborted.) — importantly, even then the PR's mechanics held: the terminal state was streamed, the permit was released, and the process exited 0 with a success envelope. After pointing fastModel at a reachable model, the verifier worked and even rejected an under-evidenced proposal with substantive proofKind reasoning before eventually accepting a delivered-output objective. Reviewers should be aware that unattended headless Goals degrade to usage_limited when the verifier provider is flaky — pre-existing core behavior, not introduced by this PR.

Conclusion: behavior matches the PR description on all four test-plan scenarios; the failure paths I could reach (budget, cap, SIGTERM, verifier failure) all fail closed. LGTM from the E2E perspective on macOS.

中文版本

本地 E2E 验证(macOS,真实模型)

在隔离 worktree 中验证 PR head 7abb7f8d8:production bundle(npm run bundle,Node v24.18.1)+ 真实 OpenAI 兼容 provider(qwen3.8-max-preview),无 sandbox。按 Reviewer Test Plan 用真实 CLI 二进制逐项跑通,以 stream-json 输出和持久化会话 JSONL 作为判据。

1. /goal 状态查询——不请求模型、事件顺序正确 ✅

  • 无活跃 Goal 时文本模式输出 No Goal is set.,退出码 0。
  • 用故意打不通的端点(OPENAI_BASE_URL=http://127.0.0.1:9/none)重跑仍然成功,证明状态查询不请求模型。
  • stream-json 顺序为 system/init → canonical goal_state{"v": 2, "goal": null, "activity": "idle"})→ 旧版 active_goal 兼容投影 → assistant 摘要 → result success,与 PR 描述完全一致。

2. Headless worker 完整跑到验证通过的终态 ✅

创建要求精确输出 + get_goal + update_goal 的 Goal。实测事件流(会话 c5796f33):active/idleactive/running → 精确输出 GOAL-V3-E2E-COMPLETEget_goalupdate_goal("Proposal queued for independent verification at the turn boundary")→ verifyingcompleteresult successnum_turns=2,退出码 0——终态工具调用后没有多余模型轮次(即本 PR 修复的退出路径缺陷)。会话 JSONL 中有 canonical 证据链:system prov=goal_control sub=goal_state 记录,以及以 user prov=goal_runtime 记录的 runtime 续轮(Core permit 准入)。

3. Wall-clock 预算 fail-closed;同一持久化会话上 pause / resume / clear ✅

  • --max-wall-time 30s + 长任务 Goal:流中输出 goal_state paused,stderr 报预算超限,进程以 55(预算专用退出码)结束,Goal 没有残留 active。
  • 同目录 -c "/goal" 恢复同一会话(6eed223d…),输出 Goal paused: …
  • -c "/goal resume" 在同一会话中重新激活 Goal 并调度新 permit(观察到 worker 重新准入、active/running),预算再次 fail-closed,退出码 55。
  • -c "/goal clear" 无确认弹窗直接完成;再查状态输出 No Goal is set.

4. 聚焦单测 ✅

4 个改动测试文件本地全部通过:284 通过、1 跳过goalCommand 63、StreamJsonOutputAdapter 54、nonInteractiveCliCommands 47、nonInteractiveCli 121 | 1 跳过)。

附加观察(顺带覆盖到的 fail-closed 路径)

  • 50 续轮上限:某次运行中模型不配合并循环拒绝,50 次 runtime 续轮后 Goal 转为 usage_limited,原因为 Goal exceeded the 50-turn continuation budget,事件流保持有序,进程仍输出 success 封包并以 0 退出。确认 Core 的独立上限仍然生效(PR 正确地将其移除列为范围外)。同时 runtime 续轮确实不消耗通用 session turn 限制。
  • SIGTERM:worker 运行中杀掉 CLI 后,持久化会话中的 Goal 为 paused(而非残留 active)——permit 的 fail-closed 释放有效。
  • Verifier provider 可靠性(PR 明确列为范围外):Goal verifier 以 side query 跑在 fastModel 上。当我环境中的 fast model 不可达时,验证失败、Goal 降级为 usage_limitedGoal verifier returned invalid JSON / Request was aborted.)——但即便如此 PR 的机制仍然成立:终态被写入事件流、permit 被释放、进程以 0 退出并输出 success 封包。把 fastModel 指向可达模型后,verifier 正常工作,甚至能以实质性的 proofKind 理由驳回证据不足的提案,最终接受 delivered-output 型目标。提醒 reviewer:verifier provider 不稳定时,无人值守的 headless Goal 会降级为 usage_limited——这是既有 core 行为,非本 PR 引入。

结论:四个测试计划场景的行为与 PR 描述一致;我能触达的失败路径(预算、上限、SIGTERM、verifier 失败)全部 fail-closed。macOS 上从 E2E 角度 LGTM。

@wenshao

wenshao commented Aug 2, 2026

Copy link
Copy Markdown
Collaborator

@qwen-code /triage

@qwen-code-ci-bot

qwen-code-ci-bot commented Aug 2, 2026

Copy link
Copy Markdown
Collaborator

Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run

Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check.

Scripted assertions: 9 passed · 0 failed · 9 total

中文 — 判定:✅ 通过 · 可合入(agent 判定)

沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码(与 base 构建 A/B 对照、无 mock harness 断言、定向门禁)。仅作为评审证据,不构成评审、批准或 CI 检查

脚本断言:9 通过 · 0 失败 · 9 总计

Verification report

PR #8324 — Deep Verification Report

Verdict: merge-ready — 9/9 scripted assertions passed, 0 unexpected failures.
Verified head: 7abb7f8d8636ed4754b67e38d5eaa7056fc6bc05 (HEAD^2).
Base tip: 184365390320639be94cc13b66748fd8328cb2f1 (HEAD^1).

中文摘要
  • 结论: merge-ready。9 项脚本化断言全部通过,无意外失败。
  • A/B 结论: 非交互 /goal 状态查询在 HEAD 上输出 canonical goal_state v2 事件({v:2, goal:null, activity:"idle"}),排序在 assistant 摘要之前,返回 "No Goal is set.",且不请求模型(num_turns:0, duration_api_ms:0)。BASE 侧无 goal_state 事件,使用旧 hook 投影文本。canonical 事件是 PR 变更的 load-bearing 产物。
  • 退出路径修复: mutation A/B 证明 terminateTurn && activeGoalTurn 分支是 load-bearing 的——禁用后 exit-path 测试从 exit 0 变为 exit 1(expected 1 to be +0),worker 不再干净终止。adapter 的 goal_state 发射被禁用后,两个排序测试变红(118 个其他测试仍绿)。无 survivor。
  • 未覆盖范围: ACP 迁移(PR 明确保留旧路径)、Windows/Linux 实机 E2E、verifier/provider 稳定性、Core 独立 50 续轮上限移除、per-commit 归因(shallow checkout 仅 1/4 commit 可达)。

Central claim + A/B

Central claim: Non-interactive /goal commands run on the canonical Goal v3 runtime — status with no active goal returns "No Goal is set." without contacting the model, and stream-json consumers receive an ordered goal_state event before the assistant summary.

A/B table (witness: 01-ab-goal-status-base-vs-head.png):

Oracle BASE (HEAD^1) HEAD (PR)
text /goal output No goal set. Usage: /goal <condition>... (legacy hook projection) No Goal is set. (canonical)
goal_state v2 event in stream-json absent present {v:2,goal:null,activity:"idle"}
event order init → assistant → result init → goal_state → assistant → result
model contacted no (num_turns:0, api_ms:0) no (num_turns:0, api_ms:0)
exit code 0 0

The canonical goal_state event is load-bearing: it exists only on HEAD, ordered before the assistant summary, and the base control lacks it entirely. Both sides exit 0 with no model contact — the difference is the canonical lifecycle event the PR introduces.

Harness: 01-ab-goal-status.mjs — runs both bundles with HOME isolated to empty dirs, parses stream-json lines, asserts 9 conditions (6 HEAD-side, 3 control-side). All 9 pass.

Exit-path fix — mutation A/B

The PR's embedded bug fix ("fail closed before session turn exit") addresses a real defect: a terminal update_goal tool call could fall through without emitting a success result envelope, causing exit 1 instead of exit 0.

Mutation matrix (witness: 02-mutation-matrix.png):

Guard mutated Mutation Suite outcome Pinned
terminateTurn && activeGoalTurn branch (nonInteractiveCli.ts:2300) if (false && ...) REDexpected 1 to be +0 on exitCode.toBe(0) CAUGHT
goal_state emission in StreamJsonOutputAdapter.processEvent if (false && ...) RED — 2 tests fail (streams Goal state changes... + emits direct Goal v2 state...); 118 others green CAUGHT
(unmutated control) none GREEN — 120 passed, 1 skipped CONTROL

Both mutations produce real behavioural mismatches (not compile/import breaks). The exit-path mutation fails the intended assertion (exitCode 0 vs 1), proving the branch owns the clean-termination property. The adapter mutation fails the exact ordering assertion it was added for. No survivor.

Targeted gates

Gate Result Liveness
nonInteractiveCli.test.ts (HEAD) 120 passed, 1 skipped
nonInteractiveCliCommands.test.ts + goalCommand.test.ts + StreamJsonOutputAdapter.test.ts (HEAD) 164 passed
tsc --noEmit (cli workspace) clean (0 errors) probe: planted __verify_probe: number = 'not-a-number'error TS2322, exit 2, restored clean

Findings

None blocking. The central claim is proven load-bearing, the exit-path fix is proven load-bearing, all targeted gates are green, and both central mutations are caught.

Not covered

  • ACP migration: PR explicitly keeps ACP on the legacy Goal command path; the goal_control throw in Session.ts is covered by goalCommand.test.ts (green in the 164-suite) but not exercised end-to-end in this round.
  • Windows/Linux live E2E: PR marks these ⚠️; this round runs in the Linux CI container but does not drive a real model+goal worker end-to-end (no auth available).
  • Verifier/provider reliability: out of scope per PR description.
  • Core's independent 50-continuation cap removal: out of scope per PR description.
  • Per-commit attribution: shallow checkout (depth 2) makes only 1 of 4 PR commits reachable (git rev-list HEAD^1..HEAD^2 returns 7abb7f8d8 only). The aggregate HEAD^1..HEAD diff was verified; per-commit claims (e.g. "test(cli): cover ACP goal control rejection") were not individually exercised.
  • Runtime worker end-to-end (create → running → verifying → terminal → success with a real model): the unit tests drive this path with a real createGoalRuntime and mocked model/tool streams, which is the correct oracle for the mechanism; a live-model E2E was not run (no credentials in this container).

Methodology

Environment: node:22-bookworm CI container, refs/pull/8324/merge at depth 2. HEAD bundle (dist/cli.js + 394 chunks) was pre-built; base bundle was built in a scratch worktree (tmp/base-tree at HEAD^1) with root + nested node_modules and generated dirs symlinked/copied from the head tree (core is unchanged by this PR, so the control is clean). The A/B harness (01-ab-goal-status.mjs) runs both bundles with isolated HOME dirs, parses stream-json output, and asserts 9 conditions. Mutation runs used interface-preserving if (false && ...) guards on the two central production hunks, ran the pinned vitest tests, confirmed RED with named behavioural mismatches, then restored via git checkout. Typecheck liveness was proven by planting a type error and confirming tsc reports it. All raw logs are in logs/; evidence PNGs in evidence/.

Evidence images

01-ab-goal-status-base-vs-head

02-mutation-matrix

Harness scripts and raw logs are in the workflow run artifacts (7-day retention).

Qwen Code · sandboxed verification

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Triage re-run completed without a new review.

⚠️ The bot's only review on 7abb7f8d8636ed4754b67e38d5eaa7056fc6bc05 is a COMMENTED one, which carries no vote — so it has no verdict of its own on this commit, and main needs two approving reviews: an approval left by another account is a separate vote and does not count as the bot's own. Two different things look like this, and the stage-3 comment above says which: the triage skill deferring on purpose at 3/5 — a fork refactor hitting the approval guardrail, or a core change escalated for maintainer awareness, both normal outcomes — or an earlier approval that a push dismissed, leaving only the comment behind, which needs a fresh review.

⚠️ 机器人在 7abb7f8d8636ed4754b67e38d5eaa7056fc6bc05 上唯一的评审是 COMMENTED不带票 —— 因此它在该 commit 上没有自己的裁决,而 main 需要两个批准(其他账号的批准是另一张票)。有两种情况长这样,上方的 stage-3 评论会说明是哪一种:triage skill 在 3/5 时有意 defer(fork refactor 命中审批护栏,或核心改动被升级交由维护者把关,两者都是正常结果);或者更早的批准被一次推送作废、只剩下这条评论,此时需要重新评审。

The stage comments above were updated with the latest result. View workflow run.

上方各阶段评论已更新为最新结果。查看工作流运行

@doudouOUC doudouOUC left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed the exact head 7abb7f8d8636ed4754b67e38d5eaa7056fc6bc05. I found one blocking ordering defect in the canonical goal_state stream, noted inline, so I am not approving this revision.

The earlier ACP rejection, queued user-permit wait, and session-turn fail-closed findings are fixed in this head. Focused verification: 284 tests passed / 1 skipped; modified-file Prettier and ESLint checks passed; git diff --check is clean; both reported CI workflows are green. A standalone package typecheck could not be used as independent evidence because the isolated review archive did not contain built workspace dependency declarations; the PR's full CI typecheck remains green.

The remaining defect is missed by the focused tests because sendMessageStream is mocked, so they never combine the CLI's direct runtime subscription with Core's queued GoalState events.

中文说明

已基于精确 head 7abb7f8d8636ed4754b67e38d5eaa7056fc6bc05 重新审查。发现 1 个 canonical goal_state 状态流的阻塞性顺序问题,已在行内说明,因此本轮不 approve。

此前 ACP 拒绝、排队 user permit 等待、session-turn fail-closed 等问题在当前 head 已修复。聚焦验证结果:284 个测试通过、1 个跳过;改动文件 Prettier 和 ESLint 通过;git diff --check 干净;已报告的两个 CI workflow 均为绿色。独立 review archive 未构建完整 workspace 依赖声明,因此无法把单 package typecheck 作为额外证据;PR 的完整 CI typecheck 仍为绿色。

现有聚焦测试 mock 了 sendMessageStream,没有把 CLI 的 runtime 直订阅和 Core 排队的 GoalState 事件合流,所以未覆盖剩余缺陷。

override processEvent(event: ServerGeminiStreamEvent): void {
if (event.type === GeminiEventType.GoalState) {
const signature = JSON.stringify(event.value);
if (signature === this.lastGoalStateSignature) return;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Critical] Prevent the second source from replaying stale canonical Goal states. runNonInteractive subscribes directly to the runtime through observeGoalRuntime, while Core's sendMessageStream also subscribes and later drains the same queued GoalState sequence. This comparison removes only consecutive duplicates. On a no-tool active Goal segment, finishTurn broadcasts idle and synchronously schedules the next permit/broadcasts running; the direct listener emits both, then Core drains its queued copy. A probe against this exact head produced idle → running → idle → running, so a consumer treating goal_state as authoritative regresses from running to stale idle and processes both transitions twice. Please make one source authoritative (while preserving explicit status/no-model emission), or dedupe using source/transition sequencing rather than only the last payload, and add a regression where source A emits [idle, running] before source B replays [idle, running].

中文说明

[严重] 防止第二个事件源重放陈旧的 canonical Goal 状态。 runNonInteractive 通过 observeGoalRuntime 直接订阅 runtime,而 Core 的 sendMessageStream 也会订阅并在稍后排空同一组 GoalState。这里仅能去掉相邻重复。在没有工具调用的 active Goal segment 结束时,finishTurn 会先广播 idle,随后同步调度下一个 permit 并广播 running;直订阅先输出两者,然后 Core 再排空其缓存副本。在当前精确 head 上的最小复现实际得到 idle → running → idle → running,因此把 goal_state 当作权威状态的消费者会从 running 倒退到陈旧 idle,并重复处理两次状态转换。请让一个来源成为权威来源(同时保留显式 status/no-model 输出),或按来源/转换序列去重,而不是只比较最后一个 payload;并补一个回归测试:来源 A 先输出 [idle, running],来源 B 随后重放 [idle, running]

@yiliang114 yiliang114 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The adoption itself looks right to me. I read the diff against packages/core/src/goals/goal-runtime.ts on main and spot-checked the contracts the headless path now depends on: dispatch sets continuationQueued even when no host is bound yet and bindHost flushes it, so the dispatch-then-bind ordering in the goal_control case is sound; restore() forces activity: 'idle' and permits are process-local, so a resumed session can't inherit a stale occupied permit; and the goal_state dedupe holds because revision increments on every dispatch. The permit lifecycle (held across model and tool-result segments, transcript flushed before finishTurn) and the fail-closed coverage on every exit path were the parts I was most suspicious of going in, and they held up under tracing.

A few non-blocking observations, none overlapping with the existing inline notes:

  1. Non-blocking: with runtime continuations exempt from the generic session turn cap, the only turn-count backstop left for headless Goals is Core's MAX_GOAL_CONTINUATION_TURNS. This PR itself lists removing that fixed cap as follow-up work, and nothing on the CLI side anchors the dependency — no comment at enforceSessionTurnLimit, no test pinning it. When the cap goes away, headless Goals silently lose their only turn bound. Might be worth a comment plus one contract test before that follow-up lands.
  2. Non-blocking: finishTurn awaits the verifier, so with no --max-wall-time set, a hung verifier provider leaves the headless run hanging with no escape. I know verifier reliability is scoped out here — this is more a note that headless+Goal without a wall-clock budget now inherits that exposure; a docs line or a dedicated verification timeout in a follow-up would close it.
  3. Non-blocking: the text-mode summaries changed wording (Goal cleared: <condition>Goal cleared., No goal set. Usage: ...No Goal is set.). The Risk section covers stream-json consumers but not text grep-ers; a one-liner in the migration notes would save someone's script.
  4. Non-blocking, mostly theoretical: finishGoalTurn doesn't catch STALE_GOAL_TURN_MESSAGE. In the narrow interleaving where a pause dispatch lands between the permit re-check and finishTurn, the throw would surface as an error envelope instead of a clean exit. The abort gate plus serialized Core dispatch make this hard to hit in practice, so a catch-and-settle would just be belt-and-suspenders.

Nothing here blocks merge — the open test gaps from earlier inline notes are reasonable follow-ups for a split PR of this size. Approving.

@wenshao
wenshao added this pull request to the merge queue Aug 3, 2026
Merged via the queue into QwenLM:main with commit 4fc8568 Aug 3, 2026
113 checks passed
@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Released in v0.21.4.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants