feat(cli): add /advisor command for second-opinion conversation review - #7567
Conversation
Adds a manual /advisor slash command that asks a reviewer model for an independent second opinion on the current conversation. The review runs as a read-only forked side query sharing the main conversation context (runForkedAgent cache path, NO_TOOLS), so the main session is never mutated. A new advisorModel setting selects a dedicated reviewer model, falling back to the main model when unset. Refs #6542
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
E2E test report (macOS,
|
gwinthis
left a comment
gwinthis
left a comment
There was a problem hiding this comment.
Review: APPROVE (C=0)
Summary
Clean feature addition (+481/-0, all new code) that adds a /advisor command for second-opinion conversation review. Follows the established /btw forked-query pattern exactly — same runForkedAgent cache path, same buildBtwCacheSafeParams, tools stripped at request level. The advisor cannot mutate session state or execute tools.
Findings
None blocking. Well-architected:
- Security model is sound. Cache path defaults to
NO_TOOLS— the advisor sees the conversation but cannot act on it.cacheSafeParamsshares history read-only. No mutation path exists. - Follows
/btwpattern precisely. SamerunForkedAgentprimitive, samebuildBtwCacheSafeParams, same abort/pending/error handling structure. A developer who understands/btwimmediately understands/advisor. - Prompt design is well-structured.
buildAdvisorPrompt()uses<system-reminder>tags with explicit constraints (no tools, review only, fixed markdown sections). The four-section format (Verdict / Risks / Missing evidence / Recommendation) produces actionable output. - Model override is clean.
advisorModelsetting flows throughsettings.merged→ conditional spread intorunForkedAgent({ model }). Empty string = use main model. No special-casing. - Test coverage is thorough. 16 tests covering: metadata, max length, no config, interactive success/failure/abort, focus passing, model override/fallback, no context, busy state, empty response, ACP mode. Uses
vi.hoisted()andcreateMockCommandContext()correctly. - Settings schema synced. Both
settingsSchema.tsandsettings.schema.jsonupdated consistently.
Minor Observations (non-blocking)
- Reuses
BTW_MAX_INPUT_LENGTH(4096) for focus validation — pragmatic, same constraint applies. - The
pendingItemguard prevents concurrent/advisorwith other long operations — correct conservative choice. buildAdvisorPromptis exported and separately testable — good for maintainability.
Verification Report (tmux)
✓ src/ui/commands/advisor-command.test.ts (16 tests) 28ms
Test Files 1 passed (1)
Tests 16 passed (16)
tsc -p packages/cli --noEmit: exit 0 (TS6305 warnings are pre-existing build artifact issues)- Pre-existing
mustTranslateKeys.test.tsfailure (ajv import) is unrelated to this PR
Architecture Insight
Forked side-query pattern for read-only model features: When adding a feature that needs model access without tool access or session mutation, use runForkedAgent with cacheSafeParams. This shares the prompt cache (saving tokens) while enforcing read-only via NO_TOOLS. The pattern is: build prompt → get cacheSafeParams → runForkedAgent → format result. See /btw and /advisor as reference implementations.
中文说明
评审：APPROVE (C=0)
概要
干净的功能添加（+481/-0，全部新代码），添加 /advisor 命令用于对话的第二意见审查。完全遵循已确立的 /btw forked-query 模式——相同的 runForkedAgent 缓存路径、相同的 buildBtwCacheSafeParams、工具在请求级剥离。advisor 不能修改会话状态或执行工具。
发现
无阻断项。 架构良好：
- 安全模型健全 — 缓存路径默认
NO_TOOLS，advisor 只能看不能做 - 精确遵循
/btw模式 — 理解/btw的开发者立即理解/advisor - Prompt 设计结构化 — 四段式输出（Verdict/Risks/Missing evidence/Recommendation）
- 模型覆盖干净 — 空字符串 = 用主模型，无特判
- 测试覆盖全面 — 16 个测试覆盖所有主要路径
- 设置 schema 同步 — TypeScript 和 JSON Schema 一致更新
验证报告（tmux）
- 16/16 测试通过（28ms）
- typecheck 通过（exit 0）
架构洞察
Forked side-query 模式用于只读模型功能： 需要模型访问但不需要工具或会话修改时，用 runForkedAgent + cacheSafeParams。共享 prompt cache（省 token）同时通过 NO_TOOLS 强制只读。
— qwen3.7-max via Qwen Code /review
yiliang114
added
the
autofix/takeover
|
🤝 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. Remove the 中文说明🤝 已接管：autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突，直到移除标签或达到轮次上限。移除 |
|
Thanks for the PR! Template looks good ✓ (minor: "Risk & Scope" and "Linked Issues" headings are missing, but the content is covered in prose — not blocking). Problem: This is a feature addition, not a bug fix — no reproduction needed. The motivation is clear: long agentic sessions converge early without an independent check, and the advisor pattern (Claude Code ships one server-side) is a proven loop. The proposal is tracked in #6542. Direction: Aligned. The building blocks already exist ( Size: 176 production lines (settingsSchema 11, BuiltinCommandLoader 2, advisor-command 163) + 300 test lines + 5 schema lines. Well under any threshold. Core paths touched ( Approach: The scope feels right — reuses the Risk: No elevated risk signals — no high-risk paths matched. Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓（小问题：缺少 "Risk & Scope" 和 "Linked Issues" 标题，但内容已在正文中覆盖——不阻塞）。 问题：这是功能新增，不是 bug 修复——无需复现。动机清晰：长会话容易过早收敛，advisor 模式（Claude Code 已有服务端实现）是验证过的循环。提案跟踪在 #6542。 方向：对齐。基础设施已存在（ 规模：176 行生产代码 + 300 行测试 + 5 行 schema。远低于任何阈值。触及的核心路径（ 方案：范围合理——几乎原样复用 风险：无升级风险信号。 进入代码审查 🔍 — Qwen Code · qwen3.8-max-preview Reviewed at |
Code ReviewIndependent proposal: I'd reuse the Comparison: The PR matches this almost exactly. Clean implementation — 163 lines that follow the established Specific observations:
Nit (non-blocking): The import in Reuse check: The PR correctly reuses CI EvidenceCI is still running on the reviewed commit. The ubuntu unit suite is in progress; macOS and Windows tests were skipped (conditional trigger). No failures observed so far. Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 Sandboxed verification would settle the end-to-end claim: 中文说明代码审查独立方案： 复用 对比： PR 几乎完全匹配。实现干净——163 行忠实遵循已有的 具体观察：
小问题（不阻塞）： 复用检查： PR 正确复用了 core 的 CI 证据CI 仍在运行。ubuntu 单元测试进行中；macOS 和 Windows 测试被跳过（条件触发）。目前无失败。 沙箱验证可以确认端到端行为： — Qwen Code · qwen3.8-max-preview Reviewed at |
|
Confidence: 4/5 — clean, minimal feature that reuses proven infrastructure; only gap is CI still in flight. This is exactly the kind of PR I like to see: 163 lines of production code that solve a real problem by composing existing primitives ( The test suite is thorough for a command of this size — 16 tests covering the happy path, error paths, abort, busy state, model override, and both execution modes. The mock boundary is at the right level ( My independent proposal matched the PR's approach almost exactly, which is a good sign — there isn't a simpler path being missed. The scope is tight (Phase 1 only, Phase 3 explicitly deferred), every edit serves the goal, and the settings addition follows the established The only reservation: CI hasn't finished yet, and the end-to-end TUI behavior (does the four-section review actually render correctly, does the conversation remain unmutated) rests on the author's manual evidence plus the unit mocks. The Approval deferred until CI lands green on 中文说明置信度：4/5 ——干净、最小的功能，复用已验证的基础设施；唯一差距是 CI 仍在运行。 这正是我喜欢看到的 PR：163 行生产代码通过组合现有原语（ 测试套件对此规模的命令很全面——16 个测试覆盖正常路径、错误路径、中止、忙碌状态、模型覆盖和两种执行模式。mock 边界在正确层级（ 我的独立方案与 PR 方案几乎完全匹配——没有更简路径被遗漏。范围紧凑（仅 Phase 1，Phase 3 明确推迟），每个编辑服务于目标，设置添加完全遵循已有的 唯一保留：CI 尚未完成，端到端 TUI 行为（四段审查是否正确渲染、对话是否保持不变）依赖作者的手动证据和单元 mock。Stage 2 中命名的 批准推迟至 CI 在 — Qwen Code · qwen3.8-max-preview Reviewed at |
|
🔀 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 将重新运行。 |
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： No action requiredThis review round contains no actionable feedback:
No code changes were made. 中文说明无需操作本轮审查不包含任何需要处理的反馈：
未做任何代码变更。 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
— qwen3.8-max-preview via Qwen Code /review
Review:
|
Maintainer verification — real build, real TUI, recorded provider wireVerified locally at Setup. Clean worktree → full 1. What the PR claims — all confirmed
Additional probes not in the test plan:
Wire evidence for claims 2–4 (one log, one session):
2. FindingsF1 — The advisor is told a truncated transcript is complete. (worth one line of prompt text)
…and then asks the model for a Measured in the run above: after 27 turns the main turn sent 56 messages, the advisor request sent 42 (1 system + 40 history entries + the advisor prompt), its first history entry was So on a long session the advisor will confidently list as "missing evidence" things that were established, and the user has no signal that it only saw a window. This bites exactly the sessions the PR motivates ("long agentic sessions tend to converge early"). For Cheapest fix: change that prompt line to say the transcript may be truncated to the most recent turns. Better: give F2 — Reviewer Test Plan step 4 does not reproduce.
It does not error. On a fresh session F3 — The While the advisor is pending the composer is hidden and no input is accepted, so a second F4 — No docs.
F5 — The invocation and the full review are persisted to the session 3. VerdictThe implementation does what it says, on real code, including the two properties that matter most and that unit tests with a mocked Recommend merge after F1 (one-line prompt change) and F2 (test-plan correction). F3–F5 are non-blocking. 中文说明维护者验证 —— 真实构建、真实 TUI、记录 provider 请求在 环境：干净 worktree 执行完整 1. PR 的声明全部得到确认
测试计划之外的补充探针：
2. 发现F1 —— advisor 被告知「这是全部证据」，但它拿到的是被截断的记录（建议改一行提示词）
实测：27 轮对话后，主请求发送 56 条消息，advisor 请求只有 42 条（1 system + 40 条历史 + advisor 提示），其第一条历史是 于是长会话中 advisor 会把实际已经确立过的内容当作「缺失证据」列出，用户也得不到任何截断提示。而这恰恰是 PR 动机所指的那类会话。对 最低成本修法：把该行提示词改成「以上记录可能只包含最近若干轮」。更好的做法：给 F2 —— Reviewer Test Plan 第 4 步无法复现 全新会话执行 F3 —— advisor 等待期间输入框被隐藏、不接受任何输入，第二个 F4 —— 没有文档
F5 —— 调用与完整审查结果会以 3. 结论实现与描述一致，并且用 mock 掉 建议在完成 F1（一行提示词）与 F2（测试计划更正）后合并；F3–F5 不阻塞。 |
- Fix prompt to acknowledge transcript may be truncated (F1) - Add empty history guard so fresh sessions get a clean error (F2) - Add getModel() guard consistent with /btw - Trim advisorModel to reject whitespace-only values - Add cross-provider disclosure to advisorModel description - Add ADVISOR_MAX_FOCUS_LENGTH constant - Add i18n entries for advisor-specific strings (en/zh/zh-TW) - Strengthen tests: section headings, abortSignal forwarding, empty history, whitespace model, no-override assertion
|
🤖 Addressed the latest review feedback (round 1/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 1/100 轮）。改动内容与我反驳保留之处如下： Review feedback addressedRequired fixes (maintainer blockers)F1 — Prompt falsely claims transcript is complete → Fixed Changed the prompt line from "The transcript above is the complete evidence available to you" to "The transcript above may be truncated to the most recent turns; treat what is shown as the evidence available to you." Also updated the F2 — Fresh session does not error as documented → Fixed Added an explicit Suggestions addressed
Missing Added the same i18n gaps → Fixed Added zh and zh-TW translations for all advisor-specific user-facing strings:
Updated the setting description to include: "Setting this sends the recent conversation transcript to that model, even when it uses another provider." Regenerated Borrowed constant → Fixed Added Test improvements → Fixed
Suggestions declinedIssue 1 — Markdown rendering via Rendering the review through No The reviewer noted this is "fine as a follow-up." Adding a new flag to
Moving the prompt builder to No change neededF3 — F5 — F4 — Missing documentation — Noted. The maintainer classified this as non-blocking. Docs for Verification
中文说明审查反馈处理必要修复（维护者阻塞项）F1 — 提示词错误声称记录是完整的 → 已修复 将提示词从「The transcript above is the complete evidence available to you」改为「The transcript above may be truncated to the most recent turns; treat what is shown as the evidence available to you」。同时更新了 F2 — 全新会话不会按文档描述报错 → 已修复 在 已处理的建议
缺少 添加了与 i18n 缺失 → 已修复 为所有 advisor 特有的用户可见字符串添加了 zh 和 zh-TW 翻译：
更新设置描述，增加：「Setting this sends the recent conversation transcript to that model, even when it uses another provider.」已重新生成 借用常量 → 已修复 添加 测试改进 → 已修复
已拒绝的建议Issue 1 — 通过 通过 无 审查者指出这「可以作为后续」。在
将提示词构建器移至 无需更改F3 — F5 — F4 — 缺少文档 — 已记录。维护者将其归类为非阻塞。 验证
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 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/模型 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI (merge-queue-only job) and its suite did not run locally (the touched CLI/ACP behavior is fully unit-tested and green).
Not reviewed: reverse audit — hit the 5-round hard cap while round 5 was still reporting (no dry convergence).
Not explored to full depth (tool budget reached): chunk 7: running forkedAgent.cache.test.ts — the review worktree has no node_modules ( Cannot find package 'vitest' ), so the two new assertions were verified only b…; "You are review agent reverse-audit — Reverse audit agent…": none — no check left unfinished at the ceiling (~42 of ~52 calls used).; "You are review agent reverse-audit — Reverse audit agent…": none — all checks above completed within budget.; "You are review agent reverse-audit — Reverse audit agent…": none — all checks above completed within budget.; "You are review agent reverse-audit — Reverse audit agent…": none — all checks above completed within budget., and 7 more.
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI (merge-queue-only job) and its suite did not run locally (the touched CLI/ACP behavior is fully unit-tested and green)。
未审查：reverse audit — hit the 5-round hard cap while round 5 was still reporting (no dry convergence)。
未探索到全部深度（达到工具调用预算）：chunk 7：running forkedAgent.cache.test.ts — the review worktree has no node_modules ( Cannot find package 'vitest' ), so the two new assertions were verified only b…；"You are review agent reverse-audit — Reverse audit agent…"：none — no check left unfinished at the ceiling (~42 of ~52 calls used).；"You are review agent reverse-audit — Reverse audit agent…"：none — all checks above completed within budget.；"You are review agent reverse-audit — Reverse audit agent…"：none — all checks above completed within budget.；"You are review agent reverse-audit — Reverse audit agent…"：none — all checks above completed within budget.，另有 7 条。
— qwen3.8-max via Qwen Code /review (v0.21.11)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI (merge-queue-only job) and its suite did not run locally.
Not explored to full depth (tool budget reached): "Restart-pass reverse audit of PR #7567 (/advisor command)…": none — all checks above completed within budget; no verification left unfinished.; "Restart-pass reverse audit of PR #7567 (/advisor command)…": none — the entire assigned range was read untruncated and walked.; "Restart-pass reverse audit of PR #7567 (/advisor command)…": none — all checks completed within budget.; "Restart-pass reverse audit of PR #7567 (/advisor command)…": none — stayed well within budget (about 10 calls).; "Restart-pass reverse audit of PR #7567 (/advisor command)…": none — all planned checks completed within budget (~10 tool calls)., and 4 more.
Not reviewed: reverse audit — stopped before round 4 by the review time budget.
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI (merge-queue-only job) and its suite did not run locally。
未探索到全部深度（达到工具调用预算）："Restart-pass reverse audit of PR #7567 (/advisor command)…"：none — all checks above completed within budget; no verification left unfinished.；"Restart-pass reverse audit of PR #7567 (/advisor command)…"：none — the entire assigned range was read untruncated and walked.；"Restart-pass reverse audit of PR #7567 (/advisor command)…"：none — all checks completed within budget.；"Restart-pass reverse audit of PR #7567 (/advisor command)…"：none — stayed well within budget (about 10 calls).；"Restart-pass reverse audit of PR #7567 (/advisor command)…"：none — all planned checks completed within budget (~10 tool calls).，另有 4 条。
未审查：反向审计——评审时间预算不足，未能开始第 4 轮。
— qwen3.8-max via Qwen Code /review (v0.21.11)
|
Status update from resolve-pr-comments: Changed: fixed ACP slash-command early-return telemetry so completed Verified: focused OpenAI pipeline tests, focused ACP Session advisor/conversation_finished tests, ESLint, Prettier, and Intentionally not changed: layout/i18n mutation-hardening and command-metadata extraction suggestions were resolved as out of scope for this closeout. Pending: CI and automatic review on head 485726d. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI (merge-queue-only job) and its suite did not run locally (the touched CLI/ACP/core behavior is fully unit-tested and green in this review).
Not explored to full depth (tool budget reached): "You are review agent reverse-audit — Reverse audit agent…": none — all planned checks completed within budget.; "You are review agent reverse-audit — Reverse audit agent…": none — all checks I started were completed within budget.; "You are review agent reverse-audit — Reverse audit agent…": none — all checks above were completed within budget.; "You are review agent reverse-audit — Reverse audit agent…": none — all checks above completed within budget.; "You are review agent reverse-audit — Reverse audit agent…": none — all checks I started were completed within budget (~8 tool calls)., and 7 more.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI (merge-queue-only job) and its suite did not run locally (the touched CLI/ACP/core behavior is fully unit-tested and green in this review)。
未探索到全部深度（达到工具调用预算）："You are review agent reverse-audit — Reverse audit agent…"：none — all planned checks completed within budget.；"You are review agent reverse-audit — Reverse audit agent…"：none — all checks I started were completed within budget.；"You are review agent reverse-audit — Reverse audit agent…"：none — all checks above were completed within budget.；"You are review agent reverse-audit — Reverse audit agent…"：none — all checks above completed within budget.；"You are review agent reverse-audit — Reverse audit agent…"：none — all checks I started were completed within budget (~8 tool calls).，另有 7 条。
— qwen3.8-max via Qwen Code /review (v0.21.11)
|
Status update from resolve-pr-comments: Changed: normalized compatible JSON schemas before sending OpenAI strict Verified: changed-file Prettier, ESLint, and Pending: CI and automatic review on head 78754a0. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI (merge-queue-only job) and the build-test harness ran zero test suites because the PR's own core build failure blocked the test phase; focused unit suites were run green by review agents instead.
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI (merge-queue-only job) and the build-test harness ran zero test suites because the PR's own core build failure blocked the test phase; focused unit suites were run green by review agents instead。
未审查：反向审计——在 5 轮的反审轮数上限内未收敛。
— qwen3.8-max via Qwen Code /review (v0.21.12)
|
Status update from resolve-pr-comments: Changed: fixed the OpenAI strict-schema TypeScript index-signature compile error and made ACP slash-command error exits emit Verified: ESLint on touched files, Prettier, and Intentionally not changed: the Pending: CI/automatic review on the new head; remaining active test-gap/shadowing threads are not all closed. |
|
Closeout from resolve-pr-comments automation: Changed: no product code. The previous exact-head failed CI jobs had empty or non-actionable failed logs, so I requested a failed-job rerun for the Qwen Code CI and SDK Java runs. Verified: rerun requests were accepted by GitHub. Pending: rerun CI and automatic review on the current head. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：反向审计——在 5 轮的反审轮数上限内未收敛。
— qwen3.8-max via Qwen Code /review (v0.21.12)
R18-6: classify the /advisor recording gate by the resolved command's kind instead of the raw input string, so user-defined commands shadowing the name still record their prompt while the built-in /advisor stays out of the transcript. handleSlashCommand now returns the resolved command's name+kind alongside every result. R19-1: gate buildResponseFormat on the official OpenAI endpoint, matching the prompt-caching precedent; third-party OpenAI-compatible endpoints (DeepSeek, older vLLM, validating gateways) reject the unknown response_format field and this pipeline never sent it before.
R18-10: the TUI recording-skip gate now matches the built-in /advisor by kind+name instead of the bare name in SLASH_COMMANDS_SKIP_RECORDING, so a user-defined command shadowing the name is recorded like any other custom command. Regression test covers the FILE-kind shadow. R18-3: pin the normalizeOpenAIStrictSchema -> json_object fallback with the goalJudge-shaped partial-required schema and a typeless property. R18-4: assert logConversationFinishedEvent fires on the fully-handled non-advisor ACP slash-command path in the existing /btw test.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Unresolved, please confirm:
- [Critical] markdownUtilities.ts — normalizeCodeFences math-block tracker toggles on $$ lines unconditionally vs raw render mode (comment 3714938802): file is not in this PR's diff at head 4d2762f; cannot rule on it for this PR
- [Critical] markdownUtilities.ts — phantom fence from a second delimiter run in a fence-line remainder (comment 3714938807): file is not in this PR's diff at head 4d2762f; cannot rule on it for this PR
- [Critical] markdownUtilities.ts — closesGlued remainder check /^ *$/ rejects \r and \t on CRLF text (comment 3714938811): file is not in this PR's diff at head 4d2762f; cannot rule on it for this PR
- [Critical] markdownUtilities.ts — inserted newline can turn the same-line prefix into a fence line the tracker never re-evaluates (comment 3714938826): file is not in this PR's diff at head 4d2762f; cannot rule on it for this PR
- [Critical] markdownUtilities.ts — R5-3: inline
xclosing run promoted to an unterminated fence opener (comment 3717579076): file is not in this PR's diff at head 4d2762f; cannot rule on it for this PR - [Critical] markdownUtilities.ts — R5-6: tilde twin of R5-3 for inline ~~~x~~~ pairs (comment 3717579078): file is not in this PR's diff at head 4d2762f; cannot rule on it for this PR
- [Critical] markdownUtilities.ts — R5-4: closesGlued splits an inline pair ending a code-content line (comment 3717579081): file is not in this PR's diff at head 4d2762f; cannot rule on it for this PR
- [Critical] markdownUtilities.ts — R5-9: closesGlued splits any mid-line run with a whitespace-only remainder (comment 3717579085): file is not in this PR's diff at head 4d2762f; cannot rule on it for this PR
- [Critical] markdownUtilities.ts — R5-8: tab/NBSP prefix makes a line-start run look mid-line and promotes it (comment 3717579090): file is not in this PR's diff at head 4d2762f; cannot rule on it for this PR
- [Critical] markdownUtilities.ts — R7-6: promotes-branch guards miss 1-2 char inline span prefixes (comment 3736619495): file is not in this PR's diff at head 4d2762f; cannot rule on it for this PR
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI (merge-queue-only job) and its suite did not run locally (the touched CLI/ACP/core behavior is fully unit-tested and green in this review).
Not reviewed: reverse audit — hit the 5-round cap without dry convergence; round-5 candidates were ruled already-discussed (R14-5 schema pin deferred twice; AdvisorMessage fence-normalization deferred as follow-up) or pre-existing (/btw ACP recording unchanged at the merge base).
Not explored to full depth (tool budget reached): chunk 6: none — everything completed. I won't write a Budget gap line.I've completed my review of chunk 6. Summary of what I examined and verified against the worktree:.
中文说明
仅完成部分审查，审查缺口已披露。
未决，请确认：共 10 条（原文未翻译，列表见上方英文部分）。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI (merge-queue-only job) and its suite did not run locally (the touched CLI/ACP/core behavior is fully unit-tested and green in this review)。
未审查：reverse audit — hit the 5-round cap without dry convergence; round-5 candidates were ruled already-discussed (R14-5 schema pin deferred twice; AdvisorMessage fence-normalization deferred as follow-up) or pre-existing (/btw ACP recording unchanged at the merge base)。
未探索到全部深度（达到工具调用预算）：chunk 6：none — everything completed. I won't write a Budget gap line.I've completed my review of chunk 6. Summary of what I examined and verified against the worktree:。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| const resolvedCommandInfo = slashCommandResult.resolvedCommand; | ||
| const shouldRecordSlashCommand = !( | ||
| resolvedCommandInfo?.kind === CommandKind.BUILT_IN && | ||
| resolvedCommandInfo.name === 'advisor' | ||
| ); |
There was a problem hiding this comment.
[Critical] R20-9: Deferred slash-input recording relocates /clear's user-turn record into the NEW session's transcript. /clear (aliases /reset, /new — the only session-switching commands available in ACP mode) passes this gate (not the built-in advisor), its action calls config.startNewSession(), which finalizes the old recorder and assigns a fresh ChatRecordingService mid-turn — so the deferred recordUserMessage below fetches the NEW recorder, and both the /clear user turn and its result record land in the new session's JSONL. Before this PR the user message was recorded before command execution, into the OLD session's transcript. — Failure scenario: run /clear in an ACP session, then resume the new session → SessionApiHistoryAccumulator.add includes plain user records, so the model API history starts with a dangling user "/clear" turn with no assistant reply — permanent context pollution of exactly the session /clear exists to make clean; /export of the old session likewise shows the missing final user turn.
Witness (A/B probe, Session.test.ts harness):
PR: OLD recordUserMessage count= 0 | NEW recordUserMessage count= 1 args= [["/clear"]]
fix: OLD recordUserMessage count= 1 args= [["/clear"]] | NEW recordUserMessage count= 0
Suggested fix: capture this.config.getChatRecordingService() before handleSlashCommand runs and use it for the deferred user-message record (probe-verified to restore base placement). Alternatively skip recording session-lifecycle commands in the ACP gate the way the TUI skip-set does — but that set also contains btw, so this variant additionally changes /btw ACP recording and needs its own decision.
中文说明
[Critical] R20-9：延迟的斜杠输入记录把 /clear 的用户轮次记录写进了新会话的转录。/clear（别名 /reset、/new——ACP 模式下仅有的会话切换命令）能通过这个门控（它不是内置 advisor），其 action 调用 config.startNewSession()，在轮次中途终结旧记录器并指定新的 ChatRecordingService——因此下方延迟执行的 recordUserMessage 拿到的是新记录器，/clear 的用户轮次与结果记录都落入新会话的 JSONL。本 PR 之前，用户消息在命令执行前记录，落入旧会话的转录。— 失败场景：在 ACP 会话中运行 /clear，然后恢复（resume）新会话 → SessionApiHistoryAccumulator.add 会包含普通 user 记录，因此模型 API 历史以一条没有助手回复的悬空用户 "/clear" 轮次开头——恰恰污染了 /clear 本应清理干净的会话；对旧会话执行 /export 同样会看到缺失的最后一个用户轮次。
证据（A/B 探针，Session.test.ts 测试框架）：PR 侧旧会话 recordUserMessage 次数为 0、新会话为 1（参数 [["/clear"]]）；采用修复后旧会话为 1、新会话为 0。
建议修复：在 handleSlashCommand 执行前捕获 this.config.getChatRecordingService()，并用它执行延迟的用户消息记录（探针验证可恢复 base 的落位）。也可以像 TUI 的 skip-set 那样在 ACP 门控中跳过会话生命周期命令的记录——但该集合同时需要包含 btw，这个变体会额外改变 /btw 的 ACP 记录行为，需要单独决策。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| } else if (!isSlashInput) { | ||
| // record user message for session management. Slash input is | ||
| // recorded after command resolution below: a user-defined |
There was a problem hiding this comment.
[Suggestion] R20-2: Moving the slash-input user-message recording from before handleSlashCommand to after it drops the user-turn record whenever a command action THROWS — the pre-change unconditional branch had already persisted it before execution. — Failure scenario: in an ACP session, a custom command whose action throws (handleSlashCommand rethrows action errors) → the prompt rejects and the post-resolution recording block is never reached, so the turn is absent from the transcript; after a restart/replay the user's input line is silently gone, where before this PR it survived command failures. Concrete cost: error-path transcript completeness — the user instruction that preceded a failed command disappears from session records and resume.
Witness (A/B probe at this anchor):
BASE (merge base): recordUserMessage calls: 1 [["/mycustom do things"]]
PR: recordUserMessage calls: 0 []
(both reject with Error: command action blew up)
Suggested fix: record on the rejection path too — wrap the handleSlashCommand call in try/catch, perform the same shouldRecordSlashCommand-gated recordUserMessage on the way out, then rethrow.
中文说明
[Suggestion] R20-2：把斜杠输入的用户消息记录从 handleSlashCommand 之前移到之后，导致命令 action 抛错时用户轮次记录丢失——改动前的无条件分支会在执行前完成持久化。— 失败场景：在 ACP 会话中运行一个 action 会抛错的自定义命令（handleSlashCommand 会原样重抛 action 错误）→ prompt 以拒绝结束，解析后的记录块永远不会执行，该轮次从转录中缺失；重启/回放后用户的输入行被静默丢失，而本 PR 之前命令执行失败也不会丢失。具体代价：错误路径下的转录完整性——失败命令之前的用户指令从会话记录和 resume 中消失。
证据（在该锚点处的 A/B 探针）：BASE（merge base）侧 recordUserMessage 调用 1 次（参数 [["/mycustom do things"]]）；PR 侧为 0 次（两者都以 Error: command action blew up 拒绝）。
建议修复：在拒绝路径同样记录——用 try/catch 包住 handleSlashCommand 调用，在退出时执行同样受 shouldRecordSlashCommand 门控的 recordUserMessage，然后重抛。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| try { | ||
| parts = await this.#processSlashCommandResult( |
There was a problem hiding this comment.
[Suggestion] R20-4: The new conversation_finished error-emission catch wraps only #processSlashCommandResult; the adjacent await handleSlashCommand(...) call above sits outside the try/catch, so a command action or loader throw rejects the ACP turn with no ConversationFinishedEvent, while the identical error class thrown one step later (messageType: 'error' result) does emit. — Failure scenario: in ACP mode, a custom command whose action throws (or a CommandService.create loader failure) → the turn silently drops from conversation metrics while its twin (error message result, pinned by this diff's own test 'records completion when /advisor returns an error message') emits; the diff's stated intent is to emit on every terminal slash path.
Witness (probe):
PR: finishedSpy call count: 0
widened try: finishedSpy call count: 1 (all 4 advisor-telemetry tests still pass with the fix)
Suggested fix: widen the try to start before the handleSlashCommand call — the advisor-cancel early return inside stays valid (it returns, so the catch never double-emits): try { const slashCommandResult = await handleSlashCommand(...); ... parts = await this.#processSlashCommandResult(...); } catch (error) { logConversationFinishedEvent(...); throw error; }.
中文说明
[Suggestion] R20-4：新增的 conversation_finished 错误发射 catch 只包住了 #processSlashCommandResult；上方相邻的 await handleSlashCommand(...) 调用在 try/catch 之外，因此命令 action 或加载器抛错时，ACP 轮次以拒绝结束却不发射 ConversationFinishedEvent，而一步之后抛出的同类错误（messageType: 'error' 结果）却会发射。— 失败场景：ACP 模式下某个自定义命令的 action 抛错（或 CommandService.create 加载失败）→ 该轮次从会话指标中静默消失，而其孪生路径（错误消息结果，本 diff 自己的测试 'records completion when /advisor returns an error message' 已固定）却会发射；diff 声明的意图是在每个终结的斜杠路径上都发射。
证据（探针）：PR 侧 finishedSpy 调用次数为 0；扩大 try 范围后为 1（修复下 4 个 advisor 遥测测试仍全部通过）。
建议修复：把 try 提前到 handleSlashCommand 调用之前——内部的 advisor-cancel 提前返回仍然有效（它是 return，catch 不会重复发射）：try { const slashCommandResult = await handleSlashCommand(...); ... parts = await this.#processSlashCommandResult(...); } catch (error) { logConversationFinishedEvent(...); throw error; }。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| const resolvedCommand: ResolvedSlashCommandInfo = { | ||
| name: commandToExecute.name, | ||
| kind: commandToExecute.kind, | ||
| }; |
There was a problem hiding this comment.
[Suggestion] R20-3: No test pins that handleSlashCommand attaches resolvedCommand with kind: BUILT_IN for built-in commands, yet the ACP transcript-skip gate in Session.ts depends on exactly that pairing. Every Session.test.ts case mocks handleSlashCommand wholesale and hand-feeds resolvedCommand, while nonInteractiveCliCommands.test.ts asserts resolvedCommand only on FILE-kind custom-command results; the new built-in-advisor test there asserts only the abortSignal passthrough. — Failure scenario: mutation executed — dropping resolvedCommand from the generic final return keeps nonInteractiveCliCommands.test.ts 50/50 and Session.test.ts 621/621 green, while the Session gate sees undefined, shouldRecordSlashCommand becomes true, and built-in /advisor reviews start being written into the ACP transcript — the regression this PR exists to prevent.
Witness (executed mutation):
mutation (drop resolvedCommand from generic return): Tests 50 passed (50) + Tests 621 passed (621)
+ suggested assertion under mutation: AssertionError: expected undefined to deeply equal { name: 'advisor', kind: 'built-in' }
Suggested fix: assert on the real result of a BUILT_IN command execution in nonInteractiveCliCommands.test.ts, e.g. extend the existing advisor test with await expect(handleSlashCommand('/advisor check this', ...)).resolves.toEqual(expect.objectContaining({ resolvedCommand: { name: 'advisor', kind: CommandKind.BUILT_IN } })).
中文说明
[Suggestion] R20-3：没有任何测试固定 handleSlashCommand 会为内置命令附带 kind: BUILT_IN 的 resolvedCommand，而 Session.ts 中的 ACP 转录跳过门控恰恰依赖这一配对。Session.test.ts 的所有用例都整体 mock 了 handleSlashCommand 并手工喂入 resolvedCommand；nonInteractiveCliCommands.test.ts 只在 FILE 类型的自定义命令结果上断言 resolvedCommand，其中新增的内置 advisor 测试只断言了 abortSignal 透传。— 失败场景：已执行变异——从通用最终返回中删除 resolvedCommand 后，nonInteractiveCliCommands.test.ts 50/50 与 Session.test.ts 621/621 仍然全绿，而 Session 门控看到 undefined，shouldRecordSlashCommand 变为 true，内置 /advisor 评审开始被写入 ACP 转录——正是本 PR 要防止的回归。
证据（已执行的变异）：变异（从通用返回中删除 resolvedCommand）下两个测试套件分别 50/50、621/621 通过；在变异下加入建议断言则报 AssertionError: expected undefined to deeply equal { name: 'advisor', kind: 'built-in' }。
建议修复：在 nonInteractiveCliCommands.test.ts 中对 BUILT_IN 命令的真实执行结果断言，例如在现有 advisor 测试中补充 await expect(handleSlashCommand('/advisor check this', ...)).resolves.toEqual(expect.objectContaining({ resolvedCommand: { name: 'advisor', kind: CommandKind.BUILT_IN } }))。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| OPENAI_STRICT_UNSUPPORTED_SCHEMA_KEYS.has(key) || | ||
| !OPENAI_STRICT_SCHEMA_KEYS.has(key) |
There was a problem hiding this comment.
[Suggestion] R20-8: The allowlist clause !OPENAI_STRICT_SCHEMA_KEYS.has(key) that strips unknown schema keys in normalizeOpenAIStrictSchema is pinned by no test — every new normalizer test feeds only whitelisted keys (plus the minLength family), so deleting the clause keeps the whole suite green while foreign keys leak into the strict OpenAI wire shape. — Failure scenario: runForkedAgent accepts an arbitrary caller jsonSchema, and zod toJsonSchema() output includes $schema by default; under the executed mutation, $schema/title/default leak into response_format.json_schema.schema and the regression ships silently. Concrete cost: the allowlist exists precisely to keep decorated schemas off the strict wire; nothing notices its removal.
Witness (executed mutation + flip probe):
mutation (remove !OPENAI_STRICT_SCHEMA_KEYS.has(key)): pipeline.test.ts 160/160 still pass
probe with {$schema, title, default} keys: leaks + "title": "AdvisorReview", + "default": "" under the mutation; stripped once restored
Suggested fix: add a normalization test supplying a schema with unknown keys ($schema, title, default, $defs) and assert they are stripped from the emitted json_schema (or that the schema falls back to json_object).
中文说明
[Suggestion] R20-8：normalizeOpenAIStrictSchema 中负责剥离未知 schema 键的允许列表子句 !OPENAI_STRICT_SCHEMA_KEYS.has(key) 没有任何测试固定——所有新的 normalizer 测试只喂入白名单内的键（外加 minLength 一族），删除该子句后整个测试套件仍然全绿，而外部键会泄漏进 OpenAI 严格线格式。— 失败场景：runForkedAgent 接受任意调用方的 jsonSchema，zod 的 toJsonSchema() 输出默认包含 $schema；在已执行的变异下，$schema/title/default 会泄漏进 response_format.json_schema.schema，回归会静默上线。具体代价：允许列表正是为了把带装饰的 schema 挡在严格线格式之外；它的删除不会有任何东西察觉。
证据（已执行的变异 + 翻转探针）：变异（删除 !OPENAI_STRICT_SCHEMA_KEYS.has(key)）后 pipeline.test.ts 160/160 仍全部通过；喂入含 {$schema, title, default} 键的探针在变异下泄漏 + "title": "AdvisorReview"、+ "default": ""，恢复子句后被剥离。
建议修复：新增一个 normalizer 测试，提供含未知键（$schema、title、default、$defs）的 schema，断言它们被从发射的 json_schema 中剥离（或 schema 回退为 json_object）。
— qwen3.8-max via Qwen Code /review (v0.21.12)
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Thorough pass over the full diff (39 files, +2082/-58) plus the surrounding code at head 4d2762f — ACP Session flow, nonInteractiveCliCommands, slashCommandProcessor (ESC/abort wiring), forkedAgent, geminiChat fallback gating, the OpenAI pipeline response_format normalization, docs, and i18n.
The design is solid: recording gates key on resolved-command identity, ESC cancellation in both interactive (abortControllerRef + ESC handler clears the pending indicator) and ACP (abort gate) modes is wired end-to-end, the strict-schema normalizer falls back to json_object on anything OpenAI-strict can't express (partial required, typeless properties), and response_format is gated to the official OpenAI endpoint so third-party compatible endpoints are untouched. Test coverage is strong (26 command tests, 147 pipeline tests, plus ACP recording/cancellation cases) and CI is green.
No blockers found. Three minor, non-blocking suggestions inline: one raw-token vs resolved-identity inconsistency in the ACP abort gate, one name-hardcoded abortSignal routing, and one missing markdown-fence normalization in AdvisorMessage. All three could land as-is or in a small follow-up.
| pendingSend.signal, | ||
| onFullTurnModel, | ||
| if ( | ||
| slashCommandName === 'advisor' && |
There was a problem hiding this comment.
This gate classifies by the raw input token (slashCommandName === 'advisor'), while the recording gate a few lines below classifies by the resolved command (resolvedCommand?.kind === CommandKind.BUILT_IN && name === 'advisor', per R18-6).
For a user-defined advisor command (kind FILE) that returns a message result after the ACP prompt is cancelled, this branch: (1) swallows the resolved message and reports stopReason: 'cancelled' instead of emitting it with end_turn — diverging from how other commands behave post-abort (e.g. the /btw path in the new Session tests), and (2) returns before the deferred recordUserMessage block below, so the shadowing command loses the user-turn record R18-6 explicitly preserves.
Consider keying this on the resolved identity too, e.g. slashCommandResult.resolvedCommand?.kind === CommandKind.BUILT_IN && slashCommandResult.resolvedCommand?.name === 'advisor'.
| const context: CommandContext = { | ||
| executionMode, | ||
| abortSignal: | ||
| commandToExecute.name === 'advisor' ? abortController.signal : undefined, |
There was a problem hiding this comment.
Hard-coding commandToExecute.name === 'advisor' here means every future built-in that needs a real AbortSignal in ACP has to be manually added to this condition. Is there a reason not to pass abortController.signal to all commands' CommandContext (or gate on a small built-in allowlist)? If the narrow name-match is deliberate — e.g. because /btw's fire-and-forget contract or another command's behavior depends on context.abortSignal staying undefined — a short comment saying so would help the next reader.
| </Box> | ||
| <Box flexDirection="column" marginTop={1}> | ||
| <MarkdownDisplay | ||
| text={text} |
There was a problem hiding this comment.
BtwMessage normalizes its body through normalizeCodeFences before MarkdownDisplay because models sometimes emit an opening ``` fence directly after prose without a preceding newline, and the line-based parser then renders it as prose. The advisor review fields are model-generated markdown too (the model composes risks/`recommendation` text), so an inline fence would render incorrectly here. Consider extracting that helper and reusing it for the advisor body.
R20-9: /clear (and its session-switching aliases) swaps in a fresh recorder inside its action, so its user-turn record must land before the action runs. Restore pre-resolution recording for every slash command except /advisor, which alone defers to after resolution so a user-defined command shadowing the built-in name keeps its record (R18-6).
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI (merge-queue-only job) and its suite did not run locally (the touched CLI/ACP/core behavior is fully unit-tested and green in this review).
Not explored to full depth (tool budget reached): chunk 5: run advisor-command.test.ts under vitest (worktree lacks node_modules; install cost exceeded remaining budget).
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI (merge-queue-only job) and its suite did not run locally (the touched CLI/ACP/core behavior is fully unit-tested and green in this review)。
未探索到全部深度（达到工具调用预算）：chunk 5：run advisor-command.test.ts under vitest (worktree lacks node_modules; install cost exceeded remaining budget)。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| if ( | ||
| slashCommandName === 'advisor' && | ||
| shouldRecordSlashCommand && | ||
| goalTurn?.origin !== 'runtime' && | ||
| !isRetry | ||
| ) { | ||
| const recorder = this.config.getChatRecordingService(); |
There was a problem hiding this comment.
[Suggestion] R21-1: The three-branch recordUserMessage shape (displayText+hookContext / goalTurn.permit / bare) is now duplicated verbatim in two places of #executePromptInner — the pre-action recording branch (~line 4119) and this deferred /advisor-shadow block. — Failure scenario: the two copies must stay byte-identical for a shadowing custom advisor command's transcript record to match every other slash command's record (the stated R18-6 intent). Any future change to the recording metadata applied to one site but not the other silently gives /advisor-named custom commands a different transcript record shape, which no test pins. Suggested fix: extract the shared body into a small private helper (e.g. #recordUserTurn(promptText, promptDisplayText, goalTurn)) and call it from both branches.
中文说明
[建议] R21-1：三分支的 recordUserMessage 形状（displayText+hookContext / goalTurn.permit / 裸调用）现在在 #executePromptInner 中逐字重复出现于两处——执行前记录分支（约 4119 行）与这个 /advisor 影子命令延迟记录块。— 失败场景：这两份拷贝必须逐字保持一致，命名为 advisor 的自定义影子命令的转录记录才能与其他斜杠命令的记录形状一致（即 R18-6 注释声明的意图）。未来对记录元数据的任何修改若只应用到其中一处，会静默地让名为 advisor 的自定义命令得到不同的转录记录形状，而没有任何测试能捕获这种漂移。建议修复：把共享主体提取为一个小的私有辅助方法（如 #recordUserTurn(promptText, promptDisplayText, goalTurn)），两处分支都调用它。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| OPENAI_STRICT_UNSUPPORTED_SCHEMA_KEYS.has(key) || | ||
| !OPENAI_STRICT_SCHEMA_KEYS.has(key) |
There was a problem hiding this comment.
[Suggestion] R21-2: OPENAI_STRICT_UNSUPPORTED_SCHEMA_KEYS is dead logic — none of its five members (minLength, maxLength, minItems, maxItems, uniqueItems) is in the OPENAI_STRICT_SCHEMA_KEYS allowlist, so !OPENAI_STRICT_SCHEMA_KEYS.has(key) already skips every one of them; the extra .has(key) condition cannot change the outcome for any key (measured: the two sets are disjoint). — Failure scenario: misleading dead code implying a two-tier strip policy that does not exist — adding a new unsupported keyword (e.g. pattern) to the unsupported set yields no behavior change and may misdiagnose the normalizer; a key ever added to both sets is silently won by the OR with nothing documenting or testing it.
| OPENAI_STRICT_UNSUPPORTED_SCHEMA_KEYS.has(key) || | |
| !OPENAI_STRICT_SCHEMA_KEYS.has(key) | |
| !OPENAI_STRICT_SCHEMA_KEYS.has(key) |
中文说明
[建议] R21-2：OPENAI_STRICT_UNSUPPORTED_SCHEMA_KEYS 是死逻辑——它的五个成员（minLength、maxLength、minItems、maxItems、uniqueItems）都不在 OPENAI_STRICT_SCHEMA_KEYS 白名单中，因此 !OPENAI_STRICT_SCHEMA_KEYS.has(key) 已经会跳过它们中的每一个；额外的 .has(key) 条件对任何键都不可能改变结果（已实测：两个集合交集为空）。— 失败场景：这是误导性的死代码，暗示了一个并不存在的两级剥离策略——向 unsupported 集合新增关键字（如 pattern）不会产生任何行为变化，可能导致对 normalizer 的误判；而任何同时加入两个集合的键会被 OR 静默地交由前者处理，既无文档也无测试。建议直接删除该集合及其条件（白名单是唯一权威）。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| prompt: [{ type: 'text', text: '/advisor check my work' }], | ||
| }); | ||
|
|
||
| expect(mockChatRecordingService.recordUserMessage).toHaveBeenCalled(); |
There was a problem hiding this comment.
[Suggestion] R21-4: This R18-6 shadowing test asserts only toHaveBeenCalled() with no argument match or call count, unlike the sibling /btw test which pins toHaveBeenCalledWith('/btw question'). — Failure scenario: a regression of the pre-action gate (!isSlashInput || slashCommandName !== 'advisor', Session.ts:4112) that records advisor-token input pre-action while the deferred block also records for a shadowing custom command lands the user turn in the JSONL transcript twice — replay re-emits a duplicated user turn — yet toHaveBeenCalled() stays satisfied. A wrong recorded payload (display text instead of raw prompt text) also passes. Probe-verified: seeding the double-recording regression keeps this test green; the strengthened assertion below fails it with expected "spy" to be called 1 times, but got 2 times.
| expect(mockChatRecordingService.recordUserMessage).toHaveBeenCalled(); | |
| expect(mockChatRecordingService.recordUserMessage).toHaveBeenCalledTimes(1); | |
| expect(mockChatRecordingService.recordUserMessage).toHaveBeenCalledWith( | |
| '/advisor check my work', | |
| ); |
中文说明
[建议] R21-4：这个 R18-6 影子命令测试只断言了 toHaveBeenCalled()，没有参数匹配也没有调用次数约束，而同级的 /btw 测试钉住了 toHaveBeenCalledWith('/btw question')。— 失败场景：如果执行前门控（!isSlashInput || slashCommandName !== 'advisor'，Session.ts:4112）回归为对 advisor token 输入也在执行前记录，同时延迟块又为影子自定义命令再记录一次，用户轮次会在 JSONL 转录中落两次——重放时会重复发出用户轮次——而 toHaveBeenCalled() 依然满足。错误的记录载荷（用 display text 而非原始 prompt 文本）同样能通过。已用探针验证：植入双记录回归后本测试仍然绿色；采用下面加强后的断言则会以 expected "spy" to be called 1 times, but got 2 times 失败。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| ...this.buildGenerateContentConfig(request), | ||
| ...this.buildResponseFormat(request), |
There was a problem hiding this comment.
[Suggestion] R21-6: buildResponseFormat spreads AFTER buildGenerateContentConfig, silently overriding a user-supplied samplingParams.response_format on official-endpoint JSON-mode requests, while the default provider spreads extra_body last so the same override via extra_body wins — asymmetric precedence between the two documented escape hatches. Pre-PR the pipeline never sent response_format, so samplingParams was the only way to get structured output on the official endpoint. — Failure scenario: a user who configured samplingParams.response_format with a schema OpenAI-strict cannot express (oneOf, partial required, minLength) used to send their exact shape; now it is silently replaced with json_object — constraints lost with no warning — while the identical override in extra_body wins in the opposite direction. No test pins either precedence. Suggested fix: honor an explicit user-supplied response_format (from samplingParams or extra_body) and skip buildResponseFormat when one is present, or document the precedence and add a test pinning whichever order is intended.
中文说明
[建议] R21-6：buildResponseFormat 的展开位于 buildGenerateContentConfig 之后，会在官方端点的 JSON 模式请求上静默覆盖用户通过 samplingParams 提供的 response_format；而 default provider 最后展开 extra_body，同样的覆盖经由 extra_body 却会生效——两个文档化的逃生通道之间优先级不对称。本 PR 之前该管道从不发送 response_format，samplingParams 是官方端点上获得结构化输出的唯一途径。— 失败场景：用户若在 samplingParams.response_format 中配置了 OpenAI strict 无法表达的 schema（oneOf、部分 required、minLength），过去会原样发送；现在会被静默替换为 json_object——约束被丢弃且无任何警告——而同样的覆盖放在 extra_body 里却朝相反方向生效。没有任何测试钉住任一优先级。建议修复：尊重用户显式提供的 response_format（来自 samplingParams 或 extra_body），存在时跳过 buildResponseFormat；或明确文档化优先级并增加测试钉住预期的顺序。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| await expect(prompt).resolves.toEqual({ stopReason: 'cancelled' }); | ||
| expect(finishedSpy).toHaveBeenCalledTimes(1); |
There was a problem hiding this comment.
[Suggestion] R21-7: This cancelled-advisor test pins stopReason and telemetry but not the recording invariant it exists alongside — that a cancelled built-in /advisor leaves NO user-turn transcript record. Today that invariant is guaranteed only by statement order in Session.ts (the cancel gate returns before the deferred record block). — Failure scenario: a future change that adds a user-turn record to the cancel gate itself (e.g. "preserve the cancelled turn for replay", mirroring the /btw cancel behavior this PR's own btw-cancel test pins as desirable) makes a cancelled /advisor write an orphan user turn to the JSONL transcript — a user turn with no advisor response on replay — while the cancelled test, the success-path no-record test, and the btw test all stay green.
| await expect(prompt).resolves.toEqual({ stopReason: 'cancelled' }); | |
| expect(finishedSpy).toHaveBeenCalledTimes(1); | |
| await expect(prompt).resolves.toEqual({ stopReason: 'cancelled' }); | |
| expect(finishedSpy).toHaveBeenCalledTimes(1); | |
| expect(mockChatRecordingService.recordUserMessage).not.toHaveBeenCalled(); | |
| expect(mockChatRecordingService.recordSlashCommand).not.toHaveBeenCalled(); |
中文说明
[建议] R21-7：这个 advisor 取消测试钉住了 stopReason 与遥测，但没有钉住与之并存的记录不变量——被取消的内置 /advisor 不应留下任何用户轮次转录记录。目前该不变量仅由 Session.ts 中的语句顺序保证（取消门控先于延迟记录块返回）。— 失败场景：未来若有改动在取消门控本身加入用户轮次记录（例如"为重放保留被取消的轮次"，效仿本 PR 自己的 btw-cancel 测试所钉住的 /btw 取消行为），被取消的 /advisor 会在 JSONL 转录中写入一条孤立的用户轮次——重放时是一条没有 advisor 回复的用户轮次——而取消测试、成功路径的无记录测试以及 btw 测试全部仍然绿色。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| !isRetry | ||
| ) { | ||
| const recorder = this.config.getChatRecordingService(); |
There was a problem hiding this comment.
[Suggestion] R20-2 (carried from round 20, re-verified at this head): the deferred user-message record for the advisor token runs AFTER handleSlashCommand returns, while every other command name records pre-action — so the user-turn record is dropped whenever the command action THROWS. — Failure scenario: a user-defined command shadowing the name advisor whose action throws (FILE actions re-throw non-confirmation errors) loses its user-turn transcript record: the throw propagates out of await handleSlashCommand(...) before this deferred block runs, while the identical command under any other name keeps its pre-action record through the same throw. Suggested fix: wrap await handleSlashCommand(...) in a catch that records the user message for the advisor token (same runtime/retry gates) before re-throwing — safe against double-recording because the built-in advisor's ACP action never throws (it catches internally and returns a message result).
中文说明
[建议] R20-2（第 20 轮遗留，已在本 head 复核）：advisor token 的延迟用户消息记录在 handleSlashCommand 返回之后才执行，而其他所有命令名都在执行前记录——因此命令 action 抛出异常时用户轮次记录会丢失。— 失败场景：影子命名为 advisor 的自定义命令若其 action 抛错（FILE action 会重新抛出非确认类错误），其用户轮次转录记录丢失：异常在延迟块运行前就从 await handleSlashCommand(...) 传播出去，而同样命令用任何其他名字时都能保留执行前的记录。建议修复：给 await handleSlashCommand(...) 包一层 catch，在重新抛出前为 advisor token 记录用户消息（同样的 runtime/retry 门控）——不会双记录，因为内置 advisor 的 ACP action 从不抛错（内部捕获并返回 message 结果）。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| expect(result).toEqual({ | ||
| type: 'submit_prompt', | ||
| content: 'Expanded prompt', | ||
| resolvedCommand: { name: 'custom', kind: CommandKind.FILE }, |
There was a problem hiding this comment.
[Suggestion] R20-3 (carried from round 20, re-verified by mutation probe at this head): no test anywhere asserts resolvedCommand for a BUILT-IN command execution, yet the ACP recording gate in Session.ts depends on resolvedCommand.kind === CommandKind.BUILT_IN && name === 'advisor'. Session.test.ts mocks this whole module; all 4 resolvedCommand assertions here are CommandKind.FILE. — Failure scenario: the mutation "attach resolvedCommand only for custom/skill commands, drop it from the built-in return path" leaves the entire suite green (probe-run: 50 + 622 tests pass with the mutation), and the built-in /advisor would silently start being recorded into the ACP transcript again — the exact regression the PR's Session tests were written to catch. Suggested fix: assert on a real built-in command execution that the result carries resolvedCommand: { name: 'advisor', kind: CommandKind.BUILT_IN }.
中文说明
[建议] R20-3（第 20 轮遗留，已在本 head 用变异探针复核）：整个测试套件中没有任何测试断言 BUILT-IN 命令执行时的 resolvedCommand，而 Session.ts 的 ACP 记录门控恰恰依赖 resolvedCommand.kind === CommandKind.BUILT_IN && name === 'advisor'。Session.test.ts 整体 mock 了本模块；此文件中全部 4 处 resolvedCommand 断言都是 CommandKind.FILE。— 失败场景：变异"只为 custom/skill 命令附加 resolvedCommand，从 built-in 返回路径中移除"可以让整个套件保持绿色（探针实测：变异后 50 + 622 个测试全部通过），内置 /advisor 会再次被静默记录进 ACP 转录——正是本 PR 的 Session 测试要捕获的回归。建议修复：在真实 built-in 命令执行的测试中断言结果携带 resolvedCommand: { name: 'advisor', kind: CommandKind.BUILT_IN }。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| try { | ||
| parts = await this.#processSlashCommandResult( |
There was a problem hiding this comment.
[Suggestion] R20-4 (carried from round 20, re-verified at this head): the new conversation_finished error-emission catch wraps only #processSlashCommandResult; the adjacent await handleSlashCommand(...) call above sits outside the try/catch in the same slash phase. — Failure scenario: any slash command whose action throws (a FILE command's processing error, a loader/hook failure) makes handleSlashCommand re-throw; the error propagates out of #executePromptInner before the send-loop try/finally that emits the event starts — the turn's conversation_finished telemetry is silently dropped while the sibling throw path added in this diff emits it. Suggested fix: extend the same try/catch (emitting ConversationFinishedEvent before re-throw) to also wrap await handleSlashCommand(...), e.g. one try around both awaits.
中文说明
[建议] R20-4（第 20 轮遗留，已在本 head 复核）：新的 conversation_finished 错误遥测 catch 只包裹了 #processSlashCommandResult；紧邻其上的 await handleSlashCommand(...) 调用位于同一斜杠阶段却在 try/catch 之外。— 失败场景：任何 action 抛错的斜杠命令（FILE 命令的处理错误、loader/hook 失败）都会让 handleSlashCommand 重新抛出；异常在发出该事件的 send-loop try/finally 启动前就传出了 #executePromptInner——该轮次的 conversation_finished 遥测被静默丢弃，而本 diff 新增的兄弟抛出路径却会发出它。建议修复：把同一个 try/catch（重新抛出前先发出 ConversationFinishedEvent）扩展为同时包裹 await handleSlashCommand(...)，例如一个 try 包住两个 await。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| if ( | ||
| key === 'type' || | ||
| OPENAI_STRICT_UNSUPPORTED_SCHEMA_KEYS.has(key) || | ||
| !OPENAI_STRICT_SCHEMA_KEYS.has(key) |
There was a problem hiding this comment.
[Suggestion] R20-8 (carried from round 20, re-verified at this head): the allowlist clause !OPENAI_STRICT_SCHEMA_KEYS.has(key) that strips unknown schema keys is pinned by no test individually — the only non-whitelisted key any normalizer test feeds is minLength, which the sibling OPENAI_STRICT_UNSUPPORTED_SCHEMA_KEYS clause also strips, so removing either clause alone keeps every normalizer test green. — Failure scenario: a regression narrowing or deleting the allowlist clause (e.g. only stripping keys in the unsupported set) changes which schemas normalize to strict json_schema vs fall back to json_object on the official endpoint, and the whole pipeline suite stays green. Suggested fix: after resolving R21-2 (delete the dead unsupported-keys clause), add a normalizer test feeding a key in neither set (e.g. pattern) and asserting it is stripped from the strict output — the minLength test then pins this clause.
中文说明
[建议] R20-8（第 20 轮遗留，已在本 head 复核）：剥离未知 schema 键的白名单子句 !OPENAI_STRICT_SCHEMA_KEYS.has(key) 没有任何测试单独钉住——normalizer 测试唯一喂入的非白名单键是 minLength，而它同时也会被兄弟子句 OPENAI_STRICT_UNSUPPORTED_SCHEMA_KEYS 剥离，因此单独删除任一子句所有 normalizer 测试仍保持绿色。— 失败场景：缩窄或删除白名单子句的回归（例如只剥离 unsupported 集合中的键）会改变官方端点上哪些 schema 规范化为 strict json_schema、哪些回退为 json_object，而整个 pipeline 套件依然绿色。建议修复：在解决 R21-2（删除死逻辑的 unsupported-keys 子句）之后，新增一个喂入两个集合之外键（如 pattern）的 normalizer 测试并断言它被剥离——届时 minLength 测试即钉住本子句。
— qwen3.8-max via Qwen Code /review (v0.21.12)
|
Merged latest main and resolved the one conflict in the ACP session recorder path. Changed: preserved the advisor-specific delayed recording behavior and kept main's daemon media-reference recording in the same branch. Verified: conflict markers absent, git diff --check passed, and a focused source assertion confirmed both sides of the conflict are present. Pending: CI/review on dc4d6d0. |
|
Released in v0.21.14. |
8 participants
What this PR does
Adds a manual
/advisor [focus]slash command that asks a reviewer model for an independent second opinion on the current conversation. The review runs as a read-only forked side query that shares the main conversation's context (same mechanism as/btw: forked agent cache path with tools stripped at the request level), so the main session history is never mutated and the advisor cannot take any actions. The advisor requests a schema-constrained JSON object with Verdict / Risks / Missing evidence / Recommendation fields, validates the parsed result, and formats it as the same four-section Markdown review for display. Invalid, incomplete, empty, or extra-field output is surfaced through the existing advisor failure path instead of being shown as a partial review. A newadvisorModelsetting selects a dedicated reviewer model (a model at least as capable as the main model is recommended); when unset, the main model is used.This is Phase 1 (manual command) of the Advisor proposal in #6542. The structured result remains internal to the command and is only rendered for the user. Exposing the advisor as a model-callable tool with system-prompt guidance (Phase 3) is intentionally out of scope and needs its own design.
Why it's needed
Long agentic sessions tend to converge early on an interpretation and declare completion without an independent check. Claude Code ships an advisor tool for exactly this loop, but theirs is a server-side Anthropic API feature (the API forwards the whole conversation to a stronger reviewer model), which cannot be replicated for multi-provider setups. The client-side forked query is the equivalent shape for this project, and the building blocks (forked agent cache path, model override, request-level tool stripping, and structured-output support) already exist — this PR only adds the command surface and its bounded response contract.
Reviewer Test Plan
How to verify
/advisor— expect a "Consulting advisor..." pending state, then an info message with the four sections (Verdict / Risks / Missing evidence / Recommendation). The main conversation history should be unaffected; the next user turn should behave as if/advisornever happened./advisor is the proposed fix safe?— expect the review to address the focus question./advisorat the start of a fresh session — expect a "No conversation context available" error rather than a crash.advisorModelto another configured model and rerun — expect the forked query to use that model.The focused command suite passes all 26 tests, and the OpenAI pipeline suite passes all 147 tests. Prettier and ESLint pass for the changed files, and the core package build and typecheck pass. The broader CLI package build remains locally blocked because the shared dependency tree mixes artifacts from another worktree.
Evidence (Before & After)
Before: there was no manual command for an independent, read-only second opinion on the current conversation.
After: earlier manual PR-thread evidence showed the four-section review and an unaffected next turn. A later exact-head macOS run exposed startup-only context and a dropped OpenAI JSON-mode request; both root causes are fixed, while post-fix live provider re-verification remains pending.
Tested on
Environment (optional)
Interactive CLI session with a configured reviewer model; focused Vitest, Prettier, and ESLint checks in the CLI and core packages.
Risk & Scope
/advisorinvocation adds one reviewer-model request and its latency/cost; a provider that does not honor the requested schema now produces an explicit failure instead of unvalidated prose.advisorModelsetting are additive, and the structured response remains internal to the command.Linked Issues
Implements Phase 1 of #6542 without auto-closing it.
中文说明
本 PR 做了什么
新增手动斜杠命令
/advisor [focus]，让 reviewer 模型针对当前对话给出独立的第二意见。评审通过只读的 forked side query 运行并共享主对话上下文（与/btw使用同一机制：走 forked agent cache，并在请求层移除工具），因此不会修改主会话历史，advisor 也不能执行任何操作。advisor 现在请求包含 Verdict / Risks / Missing evidence / Recommendation 字段、受 schema 约束的 JSON 对象，校验解析结果后，再格式化成相同的四段 Markdown 评审展示给用户。无效、不完整、空字段或包含额外字段的输出会进入现有 advisor 失败路径，不会作为残缺评审展示。新增advisorModel设置可指定专用 reviewer 模型（建议不弱于主模型）；未设置时复用主模型。这是 #6542 Advisor 提案的 Phase 1（手动命令）。结构化结果仍是命令内部实现，只会渲染给用户。把 advisor 暴露为可由模型调用的工具并加入 system prompt 指引属于 Phase 3，本 PR 明确不包含，需要单独设计。
为什么需要
长时间运行的 agent 会话容易过早收敛到一种解释，并在缺少独立检查时宣布完成。Claude Code 为这个闭环提供了 advisor 工具，但它依赖 Anthropic API 在服务端把完整对话转发给更强的 reviewer 模型，无法直接复用于多 provider 场景。客户端 forked query 是本项目对应的实现形态，而且 forked agent cache、模型覆盖、请求级工具移除和结构化输出等基础能力已经存在；本 PR 只增加命令入口及其有限的响应契约。
Reviewer 测试计划
如何验证
/advisor：应先显示 "Consulting advisor..." pending 状态，再显示包含 Verdict / Risks / Missing evidence / Recommendation 四部分的信息；主对话历史不应受影响，下一轮用户消息应表现得像从未运行过/advisor。/advisor is the proposed fix safe?：评审应针对该 focus 问题作答。/advisor：应返回 "No conversation context available" 错误，而不是崩溃。advisorModel设置为另一个已配置模型后再次运行：forked query 应使用该模型。聚焦的 command 测试 26 项和 OpenAI pipeline 测试 147 项全部通过。改动文件的 Prettier 与 ESLint 通过，core package 的 build 与 typecheck 通过。由于共享依赖树混用了另一 worktree 的构建产物，本地更广泛的 CLI package build 仍被阻断。
证据（前后对比）
之前：没有手动命令能针对当前对话请求独立、只读的第二意见。
之后：早期 PR thread 手工证据显示四部分评审及下一轮主会话不受影响；后续 exact-head macOS 实测暴露了启动上下文误判和 OpenAI JSON mode 丢失，两个根因均已修复，修复后的真实 provider 复验仍待完成。
测试平台
环境（可选）
使用已配置 reviewer 模型的交互式 CLI 会话；在 CLI 和 core package 中执行聚焦 Vitest、Prettier 和 ESLint 检查。
风险与范围
/advisor都会增加一次 reviewer 模型请求及相应延迟和成本；不遵循所请求 schema 的 provider 现在会产生明确失败，而不是返回未经校验的文本。advisorModel设置均为增量能力，结构化响应仍是命令内部实现。关联 Issue
实现 #6542 的 Phase 1，但不会自动关闭该 Issue。