fix(review): prohibit isolation param in roster output and SKILL.md

[wenshao]

What this PR does

Adds a parameter note to the review roster output that states the exact working_dir value and explicitly prohibits the isolation parameter on review agent calls. Also adds the same prohibition to the review skill prompt (SKILL.md) as a second line of defense.

Why it's needed

A real review run (session 6b154e50) passed both working_dir and isolation: "worktree" to all 11 review agents. These parameters are mutually exclusive in the Agent tool, so every agent call failed immediately — 3 with a parameter error, 8 killed by loop detection — and the review produced nothing despite ~4 minutes and ~126k tokens of setup work.

The root cause is that the skill prompt tells the orchestrator to set working_dir but never explicitly prohibits isolation. The word "isolation" appears descriptively in the prompt ("Worktree isolation: all subsequent steps..."), which the model conflated with the Agent tool's isolation: "worktree" parameter. The roster output — the last text the orchestrator reads before constructing agent calls — contained no parameter guidance at all.

Reviewer Test Plan

How to verify

Run the roster command against a plan with worktreePath and confirm the header includes the parameter note:

cd packages/cli && npx vitest run src/commands/review/agent-prompt.test.ts

Two new tests verify: (1) the note appears when worktreePath is present, (2) it is absent when worktreePath is missing. All 142 tests pass.

Evidence (Before & After)

N/A — non-UI change (CLI output + skill prompt text).

Tested on

OS	Status
🍏 macOS	✅
🪟 Windows	N/A
🐧 Linux	N/A

Risk & Scope

• Main risk or tradeoff: The roster header is slightly longer in worktree mode (~200 chars). This is well within the output budget and directly prevents a total-failure mode.
• Not validated / out of scope: Platform-level improvement to the Agent tool's error message for mutually exclusive parameters (would help retry if the error recurs despite the prohibition).
• Breaking changes / migration notes: None. The change is additive — existing roster consumers that copy blocks verbatim are unaffected.

Linked Issues

Observed in session 6b154e50-c280-480c-b31a-42cab1d73a87 (review of PR #7221).

中文说明

这个 PR 做了什么

在 review roster 输出中添加参数提示，明确给出 working_dir 的确切值并禁止在 review agent 调用中使用 isolation 参数。同时在 review skill prompt（SKILL.md）中添加相同的禁止指令作为二级防线。

为什么需要

一次真实的 review 运行（会话 6b154e50）在所有 11 个 review agent 调用中同时传了 working_dir 和 isolation: "worktree"。这两个参数在 Agent 工具中互斥，导致所有 agent 调用立即失败——3 个报参数错误，8 个被循环检测杀掉——尽管花了约 4 分钟和约 126k tokens 做准备，review 没有任何产出。

根因是 skill prompt 告诉编排器设置 working_dir 但从未显式禁止 isolation。"isolation" 一词在 prompt 中作为描述性词汇出现（"Worktree isolation: all subsequent steps..."），模型将其与 Agent 工具的 isolation: "worktree" 参数混淆。而 roster 输出——编排器构造 agent 调用前读到的最后一段文本——完全不包含参数指导。

风险与范围

• 主要风险：roster header 在 worktree 模式下略长（约 200 字符），远在输出预算内，直接防止全量失败。
• 未验证/不在范围内：平台层改善 Agent 工具互斥参数的错误信息。
• 破坏性变更：无。改动是纯追加的。

[qwen-code-ci-bot]

Thanks for the PR!

Template looks good ✓

Problem: this is an observed failure, not a theoretical one — the description names a concrete review run (session 6b154e50…, review of PR #7221) where every one of the 11 agents was handed both working_dir and isolation: "worktree", which are mutually exclusive, so the whole review produced nothing after ~4 min of setup. That's a real, documented failure mode worth addressing.

Direction: aligned. This is internal review-tooling robustness — clarifying the Agent-tool parameters at the point where the orchestrator constructs its calls. No sensitive area (auth/sandbox/model/telemetry/public contract) is involved. The root-cause read is plausible: the word "isolation" appears descriptively in the skill prompt, which the model can conflate with the Agent tool's isolation parameter.

Size: the PR touches a core path (packages/core/src/skills/bundled/review/SKILL.md, +1/-1 of prompt text). Production logic ≈ 21 lines (agent-prompt.ts 19 + SKILL.md 2), test lines 44, generated/schema 0 — well under any threshold. It's a fix, so no size gate applies; the core-path touch just means we hold the "name every downstream consumer" bar. The SKILL.md edit is purely additive prose consumed by the review orchestrator, so that's satisfiable.

Approach: the scope feels right. Two lines of defense — a reminder in the roster output (the last text read before agent calls) plus an explicit prohibition in SKILL.md — is reasonable defense-in-depth rather than redundancy. One mild observation, not a blocker: the roster note also restates subagent_type / run_in_background, which is slightly beyond the isolation bug, but it's consistent with "remind the exact params at the point of action" and already required by SKILL.md. No drive-by changes spotted.

Moving on to code review. 🔍

中文说明

感谢贡献！

模板完整 ✓

问题：这是一个已观测到的失败，而非理论性问题——描述中给出了一次具体的 review 运行（会话 6b154e50…，review PR #7221），其中 11 个 agent 全部被同时传了 working_dir 和 isolation: "worktree"，二者互斥，导致约 4 分钟准备后整个 review 没有任何产出。这是一个真实、有据可查的失败模式，值得处理。

方向：对齐。这是内部 review 工具的健壮性改进——在编排器构造 agent 调用的位置澄清 Agent 工具参数。不涉及敏感区域（auth/sandbox/model/telemetry/公共契约）。根因分析合理："isolation" 一词在 skill prompt 中作为描述性词汇出现，模型可能将其与 Agent 工具的 isolation 参数混淆。

规模：PR 触及核心路径（packages/core/src/skills/bundled/review/SKILL.md，+1/-1 的 prompt 文本）。生产逻辑约 21 行（agent-prompt.ts 19 + SKILL.md 2），测试 44 行，生成/schema 0 行——远低于任何阈值。类型为 fix，不触发规模门槛；触及核心路径只是意味着需要满足"能点名每个下游消费者"的标准。SKILL.md 改动是纯追加文本，由 review 编排器消费，因此可满足。

方案：范围合理。两道防线——roster 输出中的提示（agent 调用前读到的最后一段文本）加上 SKILL.md 中的显式禁止——是合理的纵深防御，而非冗余。一个非阻塞的小观察：roster 提示还重申了 subagent_type / run_in_background，略超出 isolation 问题本身，但与"在动作点提醒确切参数"一致，且 SKILL.md 本就要求这些。未发现顺手改动。

进入代码审查 🔍

— Qwen Code · qwen3.7-max

_{Reviewed at 1f8cfdabbe8b130a4a5aedb057541e85d36ddd30 · re-run with @qwen-code /triage}

[wenshao]

Review

Overview

Adds a parameter note to the --roster header (worktree mode only) stating the exact working_dir value and prohibiting isolation on review agent calls, plus the same prohibition in SKILL.md's worktree paragraph as a second line of defense. Targets a real total-failure mode: session 6b154e50 passed both parameters to all 11 agents and every call failed.

What I verified

• The mutual-exclusivity claim is accurate. The Agent tool rejects working_dir + isolation at packages/core/src/tools/agent/agent.ts:1067 ("Parameters "working_dir" and "isolation" are mutually exclusive."). The note's other advice is also consistent with the tool's actual constraints: working_dir requires run_in_background: false (incompatible-params check just above) and an explicit non-fork subagent_type — so the three parameters the note prescribes are exactly the valid combination.
• The typeof wt === 'string' && wt guard matches how the rest of the file handles worktreePath (typed unknown on PlanReport; same pattern as the role-7 handler).
• The new test's plan (worktreePath + prNumber: '9999', no ownerRepo) doesn't accidentally change the roster. requiredAgents only adds Agent 0 when both prNumber and ownerRepo are present, so no role-0 brief is built that would throw on the missing repo.
• The note can't leak into agent prompts. It lives in the header element, outside every ───── agent k ───── block; recordPrompt runs before the header is composed, so verbatim-delivery checks and the printedBlocks() test helper are unaffected.
• Tests and lint: ran npx vitest run src/commands/review/agent-prompt.test.ts on the PR head in a clean worktree — 142/142 pass. eslint --max-warnings 0 on both changed TS files is clean (the CI Test job gates on this).
• Both new tests exercise the right branches (note present with worktreePath, absent without), and the describe block's beforeEach clears the writeStdoutLine mock, so calls[0][0] is safe.

Minor observations (non-blocking)

1. The note only covers the Step 3 roster. The Step 4 --findings and Step 5 auditor outputs don't carry it. SKILL.md's Step 4/5 sections say "same rule as Step 3," so the prohibition arguably inherits — and an orchestrator that got Step 3 right will likely keep the pattern — but if the same confusion recurs at a later step, those outputs are the natural place for the same one-liner. Fine as a follow-up if it's ever observed.
2. The note sits at the top of a potentially very long output. A 17-agent roster puts the header tens of KB before the last agent call is constructed. If the failure mode ever recurs on late agents only, repeating a one-line reminder next to the end of roster line would be the cheap fix.
3. The mutual-exclusivity explanation now lives in two places (roster note + SKILL.md). Acceptable as deliberate defense in depth, but they'll need to move together if the Agent tool's constraint ever changes.

Verdict

LGTM. The fix is small, additive, placed at the point of action, and both the failure mode and the prescribed parameters check out against the actual Agent tool validation code.

---

🤖 Generated with Claude Code — Claude Fable 5

[qwen-code-ci-bot]

Code review

I wrote my independent proposal first: for "the orchestrator passes both working_dir and isolation and every agent dies," the deterministic fix lives in the Agent tool itself (reconcile the two params, or fail with an actionable error instead of a generic one). But that's core-infrastructure territory and the PR explicitly scopes it out as future work. The pragmatic alternative — prohibit isolation in the skill and restate the exact params at the point of action — is what this PR does, and it matches what I'd ship for a quick, safe mitigation.

The implementation is clean. The paramNote is gated on typeof wt === 'string' && wt, the same guard already used a few hundred lines up for the worktree brief, so it only fires for real PR-worktree reviews and stays silent for local-diff / cross-repo lightweight runs. It's appended to the roster header — genuinely the last text the orchestrator reads before building agent calls. The wt value is CLI-generated (.qwen/tmp/review-pr-<n> shape), interpolated into prompt text the model reads, never into a shell, so there's no injection surface. Comments explain the why (the failed run), tests are collocated, no any, ESM — house style throughout.

The two new tests are meaningful, not tautological: one asserts the note (with the exact working_dir value and the isolation prohibition) appears when worktreePath is set, the other asserts it's absent when it isn't. All 142 tests in the file pass.

No blockers. Two non-blocking observations: the note also restates subagent_type / run_in_background, which is a touch beyond the isolation bug but consistent with "remind the exact params at the point of action"; and this is a prompt-level mitigation, not a guarantee — a model could still pass isolation despite the note. The PR is honest about that and points at the platform-level fix as follow-up. Fine for a fix.

Real-scenario testing

Drove the actual qwen review agent-prompt --roster command in tmux against a fabricated PR-worktree plan (worktree mode, PR number set), through npm run dev. "Before" is the base commit's agent-prompt.ts checked out into the same tree; "After" is this PR. Same invocation, only the code differs. (The installed global qwen 0.20.0 refuses to emit plain stdout under a tmux pane — it tries to launch its interactive UI — so I ran both sides through the dev pipeline for an apples-to-apples capture.)

===== BEFORE (base code) — roster header =====
11 agents required. Launch one agent per block below, passing its block VERBATIM — copy, do not retype. The ───── lines are separators, not part of any prompt. This is the same roster `check-coverage`
 reads out of the plan: a block you skip or reword is a dimension nobody reviewed. Blocks are numbered `agent k of 11` and the output ends with an end-of-roster line — if either is missi
parameter note present?
0
===== AFTER (this PR) — roster header + note =====
11 agents required. Launch one agent per block below, passing its block VERBATIM — copy, do not retype. The ───── lines are separators, not part of any prompt. This is
...
**Agent tool parameters (worktree mode):** Set `working_dir: "/tmp/triage-roster/wt"` and `subagent_type: "general-purpose"`, `run_in_background: false` on EVERY agent call below. Do NOT set `isolatio
n` — the worktree already exists; `isolation` creates a new copy and is mutually exclusive with `working_dir`.

Counts confirm it: base code → 210-line roster, isolation-note=0; this PR → 212-line roster, isolation-note=1. I also verified the negative path through the real CLI: with worktreePath removed from the plan, the note count is 0, so the conditional gating holds in the product, not just in the unit test.

Unit tests: npx vitest run src/commands/review/agent-prompt.test.ts → 142 passed (142), including the two new ones.

中文说明

代码审查

我先独立写下自己的方案：对于"编排器同时传 working_dir 和 isolation 导致所有 agent 失败"这个问题，确定性的修复应在 Agent 工具本身（协调这两个参数，或用可操作的报错代替通用报错）。但那属于核心基础设施，PR 已明确将其列为后续工作而不在本次范围内。务实的替代方案——在 skill 中禁止 isolation 并在动作点重申确切参数——正是本 PR 所做的，也与我会快速、安全上线的方案一致。

实现干净。paramNote 以 typeof wt === 'string' && wt 门控，与上方几百行处 worktree brief 使用的门控一致，因此只在真实 PR-worktree review 时触发，对 local-diff / 跨仓轻量 review 保持沉默。它被追加到 roster header——确实是编排器构造 agent 调用前读到的最后一段文本。wt 值由 CLI 生成（形如 .qwen/tmp/review-pr-<n>），插入到模型读取的 prompt 文本中，从不进入 shell，因此没有注入面。注释解释了为什么（失败的那次运行），测试就近放置，无 any，ESM——符合项目风格。

两个新测试有意义、非同义反复：一个断言当 worktreePath 设置时出现提示（含确切 working_dir 值和 isolation 禁止），另一个断言未设置时不出现。文件中全部 142 个测试通过。

无阻塞项。两个非阻塞观察：提示还重申了 subagent_type / run_in_background，略超出 isolation 问题本身，但与"在动作点提醒确切参数"一致；并且这是 prompt 层缓解，不是保证——模型仍可能不顾提示传 isolation。PR 对此坦诚，并指出平台层修复作为后续。对于一个 fix 来说可以接受。

真实场景测试

在 tmux 中驱动真实的 qwen review agent-prompt --roster 命令，使用一个构造的 PR-worktree plan（worktree 模式、设置了 PR 号），通过 npm run dev 运行。"Before" 是在同一棵树中签出基线提交的 agent-prompt.ts；"After" 是本 PR。调用相同，仅代码不同。（已安装的全局 qwen 0.20.0 在 tmux pane 下拒绝输出纯 stdout——它试图启动交互式 UI——因此两侧都通过 dev 管线运行，以保证可比对。）

（终端输出见上方英文代码块。）

计数证实：基线代码 → 210 行 roster，isolation-note=0；本 PR → 212 行 roster，isolation-note=1。我还通过真实 CLI 验证了反向路径：从 plan 中移除 worktreePath 后，note 计数为 0，因此条件门控在产品中成立，而不仅在单测中成立。

单元测试：npx vitest run src/commands/review/agent-prompt.test.ts → 142 通过 (142)，含两个新测试。

— Qwen Code · qwen3.7-max

_{Reviewed at 1f8cfdabbe8b130a4a5aedb057541e85d36ddd30 · re-run with @qwen-code /triage}

[qwen-code-ci-bot]

Confidence: 4/5 — a small, well-tested fix for a real total-failure mode; the only reservations are non-blocking (a prompt-level mitigation is probabilistic by nature, and the note restates a couple of params beyond the isolation bug).

Stepping back: this is exactly the kind of PR that's easy to under-rate because it's small, but it closes a genuine hole — a review run that passed both working_dir and isolation failed all eleven agents and produced nothing after minutes of setup, and the skill prompt's descriptive use of the word "isolation" actively nudged the model toward the bad combination. Putting an explicit prohibition at the point of action (the roster header) plus a line in the skill is the right pragmatic fix; the deterministic version belongs in the Agent tool itself, and the PR is honest that it's scoping that out.

My independent proposal landed in the same place, so there's no simpler path I think it missed. The code is straightforward — one conditional note reusing the existing worktreePath guard, one prose sentence, two tests that assert presence and absence. I ran it for real: the note shows up with the exact working_dir value when the plan has a worktree, and disappears when it doesn't, in the product and not just the unit test. Every line in the diff earns its place.

The 1/5 reservation I'm holding back from: a model can still pass isolation despite the note — prompt engineering mitigates, it doesn't guarantee. That's inherent to the approach, not a defect of this PR, and it's why the platform-level error message is worth a follow-up issue. Approving.

— Qwen Code · qwen3.7-max

_{Reviewed at 1f8cfdabbe8b130a4a5aedb057541e85d36ddd30 · re-run with @qwen-code /triage}

[qwen-code-ci-bot]

LGTM, looks ready to ship. ✅

[doudouOUC]

Review

Independently reviewed at 1f8cfdab. LGTM — approving.

What I verified against the code (not just the description)

• The mutual-exclusivity claim is real. packages/core/src/tools/agent/agent.ts returns Parameters "working_dir" and "isolation" are mutually exclusive. inside the working_dir validation block. That same block also rejects working_dir + run_in_background: true and requires an explicit non-fork subagent_type — so the exact trio the note prescribes (working_dir + subagent_type: "general-purpose" + run_in_background: false, and no isolation) is the one valid combination, not merely "don't pass isolation."
• The note can't leak into an agent prompt. It's appended to the header element (index 0), \n\n-joined ahead of the ───── agent k ───── blocks, and recordPrompt runs before the header is composed — so recorded prompts, verbatim-delivery checks, and the printedBlocks() helper are all untouched.
• The guard matches the house pattern. typeof wt === 'string' && wt is the same shape the role-7 handler uses for worktreePath (typed unknown on PlanReport).
• The new tests hit the right branches. The enclosing beforeEach clears the writeStdoutLine mock and runRoster calls it exactly once, so calls[0][0] is safe. The "emits" plan carries worktreePath + prNumber but no ownerRepo, so requiredAgents doesn't add Agent 0 (gated on both) — the roster is unchanged and nothing throws.
• The SKILL.md prose change breaks no test — nothing snapshots the review SKILL.md body; the references that exist are path fixtures or the Agent tool's isolation behaviour tests (which corroborate the constraint).
• Ran it: agent-prompt.test.ts → 142/142 pass; eslint --max-warnings 0 on both changed TS files → clean.

Non-blocking (fine as follow-ups)

1. The note only rides the Step 3 --roster output; the Step 4 (--findings) and Step 5 (auditor) outputs don't carry it. SKILL.md's "every agent call" prohibition is the backstop there — worth the same one-liner only if the confusion ever recurs at a later step.
2. On a 17+ agent roster the note sits tens of KB above the last block. Low risk given the orchestrator reads the whole output before constructing calls; repeating it next to end of roster would be the cheap fix if late-agent-only failures ever appear.
3. The mutual-exclusivity rationale now lives in two places (roster note + SKILL.md) — deliberate defense-in-depth, but the two move together if the Agent tool constraint ever changes.

Small, additive, placed at the point of action, and every claim checks out against the actual validation code. 👍

[wenshao]

⚠️ Downgraded from Approve to Comment: self-PR; CI still running. Reviewed.

— qwen3.8-max-preview via Qwen Code /review

[qwen-code-ci-bot]

Released in v0.20.1.