Repository navigation
feat: add native advisor tool - #9636
Conversation
E2E Verification Report — Native Advisor ToolBranch: Verified Scenarios1.
|
| Test Suite | Tests | Status |
|---|---|---|
packages/core/src/tools/advisor.test.ts |
15/15 | ✅ |
packages/cli/src/ui/commands/advisor-command.test.ts |
18/18 | ✅ |
packages/cli/src/ui/components/messages/ToolMessage.test.tsx |
74/74 | ✅ |
packages/cli/src/ui/hooks/useGeminiStream.test.tsx |
229/229 | ✅ |
Environment
- macOS, Node.js 26, bundled CLI
- tmux session (200×50), approval-mode yolo
- Advisor model:
openai:qwen3.7-maxvia IdeaLab provider
Verdict: ✅ PASS
All 6 scenarios verified. Native advisor tool integrates correctly with executor workflow, feedback card renders properly, ordering is preserved, and enable/disable lifecycle works as expected.
d986128 to
a23d43f
Compare
|
Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. 中文请勿对活跃的 PR 执行 rebase 或 force-push，因为这会使已有的评审评论失效。另外，供日后参考：作为集成流程的一部分，机器人始终会自动将所有改动压缩（squash）为单个提交。 |
|
@qwen-code /review |
|
Qwen Code review request accepted. Review is queued in workflow run. |
948df71 to
8c8153e
Compare
…tool # Conflicts: # packages/cli/src/ui/daemon/daemon-tui-adapter.test.ts # packages/cli/src/ui/daemon/daemon-tui-adapter.ts
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
Round 2 — Follow-up at updated head
Head changed from round-1 (8824c5c7) to current (f10c2d94). This pass re-verifies the approval blocker from round 1 and checks whether the three Minors were addressed.
R13-1 / approval blocker from round 1 — RESOLVED
syncAdvisorToolRegistration now opens with if (this.getDisabledTools().has(ToolNames.ADVISOR)) return; at line 11247, consulted before the factory loop. setAdvisorModel additionally vetoes registration when disabledTools contains ADVISOR (line 6124), so the "unregister-then-re-register while disabled" path no longer exists. Independently confirmed by doudouOUC at an earlier head; verified still present at f10c2d94.
No approval blockers remain from round 1.
Round-1 Minors — status at current head
| Finding | Status |
|---|---|
F1 — parseJsonObjectText duplicated with divergent behaviour |
Open — still two implementations; Minor, no runtime impact |
F2 — inherit guard is case-sensitive (=== 'inherit') while off uses .toLowerCase() |
Open — L90 of advisor-model.ts unchanged; Minor |
F3 — /advisor review matched case-sensitively; /advisor off is lower-cased |
Open — L269 of advisor-command.ts unchanged; Minor |
All three are Minor. None blocks merge.
Coverage carried from round 1
ModelDialog.tsxproduction file (378 lines changed): not read. Test diff shows all advisor-mode scenarios (off-only, endpoint selection, fast-only, disabled-tool veto, stale persisted model) are covered; no coverage gap identified.integration-tests/cli/advisor-tool.test.ts: not executed — outside all npm workspaces and CI has not produced a green run on this file on any head of this PR.- R13-2 (unresolvable selector): boot-time
validateCliAdvisorModelthrowsFatalConfigErrorfor unresolvable selectors; runtime/advisor <model>callscheckAdvisorModelAvailabilityand surfaces an error beforeconfig.setAdvisorModel. Not a silent fallback. Concern resolved.
Verdict: APPROVE
No confirmed blocker. All three open findings are Minor. All 128 prior threads are resolved; round-15 bot reported zero findings at 3d276bd2; PR is MERGEABLE at f10c2d94.
Reviewed with AI assistance.
Retiring `setting:advisor-model` from the Web Shell presentation map dropped
its key mapping, so `isSettingVisible('advisorModel', …)` fell through to the
"no alias" branch: an exclusion no longer hid the item and an inclusion no
longer showed it. The App-level close-on-exclusion effect for a
settings-launched Advisor picker therefore stopped closing the dialog, which
is what App.test.tsx (from QwenLM#11975) covers.
`advisorModel` is hidden from the settings rows by `showInDialog: false`, not
by dropping the published host id. Keep the alias (and its acceptance in
`WEB_SHELL_SETTING_ITEM_IDS`) exactly as on main.
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Follow-up at 13ffa6e5 (continuing my review from f10c2d94):
The new commit is the right fix — keeping setting:advisor-model in the published alias map while hiding the row via showInDialog: false restores the exclusion/inclusion semantics that retiring the alias had broken, and the test update pins the parity contract again.
My two pre-merge asks are now closed:
- Text-fallback coverage:
advisor.test.tshas bothaccepts structured Advisor JSON returned as textandrecovers an array-wrapped review from the text fallback— this matters because the text fallback is the production path on the default provider (DashScope dropstool_choicewhen thinking is on, andresponse_formatonly exists on api.openai.com). - Egress disclosure: the
advisorModelsettings description now states that enabling it sends the active conversation to that model even across providers.
Fixed since my last pass: jsonResult ??= asJsonObject(...) (monotonic per-chunk assignment) and the dead Array.isArray(result.jsonResult) branch is gone.
Still open, non-blocking: the sanitize() guard shape — unknown keys like fileData/executableCode still pass through verbatim (denylist, not allowlist). Not a live leak today (no producer puts those into executor history), but worth a follow-up.
Remaining gate is CI: Test (ubuntu) + Lint + web-shell visuals are pending on this head. The CHANGES_REQUESTED on the PR is the stale bot review from d536a687 (August), several heads behind — needs a maintainer dismissal, not more code.
There was a problem hiding this comment.
Approving at 13ffa6e5.
I scoped this to the delta nobody has reviewed yet: everything after the round-13 pass at d536a687 (2026-08-29). Of the seven commits since then, cda083bf / 1910bd49 / 9ce81f92 are the contributor's and 14d7c5ba is a main sync, so the unreviewed production surface is 08fc82e2, f10c2d94 and 13ffa6e5. No blocking findings.
The deferred Critical R13-36 is fixed, and I verified it in the code
checkAdvisorModelAvailability now computes uncapturedRuntimeModelId, which is exactly this finding's shape: the selector resolves to context.currentModel and context.currentAuthType, and is absent from configuredModels. In that case the id is added to availableModelIds and available becomes true, so an advisor identical to the CLI runtime model passed as an auth-qualified selector is accepted rather than rejected at startup. resolvesToSameModel compares authType as well as modelId, so the fast-model equivalence check is auth-aware too. Pinned by advisor-model.test.ts:80 (allows the active runtime model).
R13-3 is fixed as a side effect, and the ensureTool call does not undo it
registerAdvisorTool dropped its own binary permissionManager.isToolEnabled check in favour of registerLazyTool, which resolves getToolRegistrationStatus — three-state, covering the coreTools allowlist, deny rules and tools.eager, and warning-and-skipping on error exactly as the deleted block did. So an eager allowlist that omits Advisor now demotes it to deferred instead of registering it eagerly.
The line worth checking was the await registry.ensureTool(ToolNames.ADVISOR) immediately after, since registerPermissionDeferredFactory writes into the same factories map and isToolAvailable only special-cases IMAGE_GEN — so ensureTool does consume the factory and materialize the instance. It still does not defeat the deferral: isEffectivelyDeferred (tool-registry.ts:422) and the declaration-hiding skip (:1059) both read the permissionDeferred set, which ensureTool never clears. The instance loads early; the schema stays out of the eager model request.
The removed Array.isArray guard was dead code
advisor.ts now passes result.jsonResult ?? parseJsonObjectText(result.text) straight to parseReview, dropping the old Array.isArray(result.jsonResult) ? undefined : .... That guard could not fire: asJsonObject (forkedAgent.ts:373) returns undefined for arrays, and jsonResult's only two assignment sites (:618, :649) both go through it. parseReview throws on any missing or non-string field, so malformed structured output still fails loudly rather than degrading silently.
One non-blocking residue
08fc82e2 retired the web-shell presentation alias and 13ffa6e5 restored it, so packages/web-shell/client/settings.ts nets to zero. But 08fc82e2 also moved the SettingsDialog "String Settings Editing" case off advisorModel onto general.outputLanguage, and the revert did not touch that file. The assertion is indexOf(...) >= 0, so nothing fails — the CLI dialog test just no longer exercises an advisor setting. Cosmetic; not worth a round.
Gate state, stated plainly
100 review threads, none unresolved. At the moment of this approval: 18 checks passed, 26 skipped, none failing, and Test (ubuntu-latest, Node 22.x) was still in progress — so I am approving on the review evidence, not on a completed run of the lane that executes these suites. That check still gates the merge independently, so a red result there blocks landing regardless of this vote. The previous head's lanes were cancelled by the 13ffa6e5 push, which is why this head has no earlier signal.
I have not dismissed the standing CHANGES_REQUESTED from the round-13 pass at d536a687; that is a separate action and not mine to take here. Note that its own body records every listed item as deferred and non-blocking under the convergence posture.
Correction, added after this approval was submitted
The Test (ubuntu-latest, Node 22.x) lane that was in progress when I approved has since failed, and the failure is caused by this PR, not by runner infrastructure. I am recording it here so this approval is not read as a clean bill of health.
- Job: https://github.com/QwenLM/qwen-code/actions/runs/36112125194/job/107997921347
- Suite result:
Test Files 1 failed | 1112 passed (1113),Tests 1 failed | 34046 passed | 92 skipped - The single failure:
src/serve/routes/workspace-settings.test.ts > web-shell settings alias drift > aliases every rendered row and serves every published alias, atworkspace-settings.test.ts:1082—AssertionError: expected [ 'advisorModel' ] to deeply equal [] - No corepack/runner signature anywhere in the log (the
Cannot find module .../corepack/v1/pnpm/...breakage seen on other PRs today appears 0 times here), so this is not the infra failure mode.
Line :1082 is expect(aliased.filter((key) => !rendered.includes(key))).toEqual([]); the reverse check on :1081 passes. So advisorModel is aliased but not rendered. Per the test's own derivation, aliased is parsed from SETTING_KEYS in packages/web-shell/client/settings.ts, and rendered is GET /workspace/settings' served keys minus HIDDEN_SETTING_KEYS and LIVE_SETTING_KEYS from SettingsMessage.tsx. advisorModel is in neither exclusion set, so the gap is on the served side: the daemon does not publish an advisorModel key while the web-shell alias table claims one.
This is a half-revert between the last two commits. 08fc82e2 ("retire hidden presentation alias") removed 'setting:advisor-model': 'advisorModel' from SETTING_KEYS and introduced RETIRED_IDS — which is what made this drift guard pass. 13ffa6e5 ("keep the Advisor presentation alias") restored the alias and dropped RETIRED_IDS, but nothing on the served side was restored with it.
I also want to correct my own earlier grading of this: I reviewed that flip-flop, concluded settings.ts nets to zero, and filed the leftover as a cosmetic P3 because the SettingsDialog.test.tsx assertion is only indexOf(...) >= 0. That was too narrow — I checked the test the commit touched and missed that a separate drift guard in another package cross-references the same alias against the served schema. The residue was real and it is the one red test.
Two ways to close it, author's call: revert 13ffa6e5 and keep the alias retired as 08fc82e2 intended, or keep the alias and make /workspace/settings serve advisorModel.
The rest of this review stands — R13-36 and R13-3 verified fixed, the removed Array.isArray guard verified dead, no other findings. Withdrawing the recommendation to merge until this lane is green.
There was a problem hiding this comment.
Duplicate — please ignore. The substantive approval is the first one at this head (review id 5315518532, 08:37:47Z).
Posted by an automated retry loop whose verification read used per_page=100 without --paginate; this PR has 123 reviews, so the read-back could not see that each write had already landed and kept retrying. No content difference between the copies.
There was a problem hiding this comment.
Duplicate — please ignore. The substantive approval is the first one at this head (review id 5315518532, 08:37:47Z).
Posted by an automated retry loop whose verification read used per_page=100 without --paginate; this PR has 123 reviews, so the read-back could not see that each write had already landed and kept retrying. No content difference between the copies.
There was a problem hiding this comment.
Duplicate — please ignore. The substantive approval is the first one at this head (review id 5315518532, 08:37:47Z).
Posted by an automated retry loop whose verification read used per_page=100 without --paginate; this PR has 123 reviews, so the read-back could not see that each write had already landed and kept retrying. No content difference between the copies.
There was a problem hiding this comment.
Duplicate — please ignore. The substantive approval is the first one at this head (review id 5315518532, 08:37:47Z).
Posted by an automated retry loop whose verification read used per_page=100 without --paginate; this PR has 123 reviews, so the read-back could not see that each write had already landed and kept retrying. No content difference between the copies.
There was a problem hiding this comment.
Duplicate — please ignore. The substantive approval is the first one at this head (review id 5315518532, 08:37:47Z).
Posted by an automated retry loop whose verification read used per_page=100 without --paginate; this PR has 123 reviews, so the read-back could not see that each write had already landed and kept retrying. No content difference between the copies.
Hiding `advisorModel` from the TUI dialog by flipping `showInDialog` to false also removed it from the daemon `/workspace/settings` response, so the Web Shell settings panel lost the only surface that configures the Advisor model and `workspace-settings.test.ts` reported the now-unmatched `setting:advisor-model` alias as drift. `voiceModel` and `imageModel` are `showInDialog: false` and are re-served through `WEB_SHELL_SETTINGS` for exactly this reason; do the same for `advisorModel`. The TUI dialog still hides the free-text row, and the alias stays mapped so a host exclusion closes the settings-launched picker.
Re-serving `advisorModel` to the Web Shell makes a workspace-scope write reachable again: the settings panel's model sub-dialogs persist to the active tab's scope, which defaults to Workspace. The merge now strips Workspace `advisorModel`, so that write would persist a committable dead entry, answer 200 + requiresRestart, and leave the picker showing a model that never applies. `rejectWorkspaceRestrictedWrite` already refuses this for the sectioned restriction list; `advisorModel` is the root-level half of that list, so the guard now tests both. The list itself moves next to its sectioned sibling so the strip, the warning, and this refusal read one source.
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
Re-review at 5c96f29f — approving
Head moved from 08fc82e2 (my previous pass, a COMMENT) to 5c96f29f; base 64c04538. All three gates I named last time are now closed, and I found no Critical on the current head.
The delta nobody had reviewed yet
Two commits landed after the review bot's approval at 13ffa6e5, both by yiliang114, touching four files. I read all of them at head.
packages/cli/src/serve/routes/workspace-settings.ts adds advisorModel to WEB_SHELL_SETTINGS so the Web Shell model panel can read and persist it, and widens the write guard's membership test from WORKSPACE_RESTRICTED_SETTING_KEYS.includes(key) to a Set combining that list with the new WORKSPACE_RESTRICTED_ROOT_SETTINGS. The direction is fail-closed and the placement is right: rejectWorkspaceRestrictedWrite returns early for any scope other than workspace, and at the call site (line 546) it runs after scope validation (514) and the workspace-trust check (522) and before persistence (585). A workspace-scope write therefore 400s with workspace_restricted_setting instead of silently committing a key that stripWorkspaceRestrictedSettings would strip at merge — which was the actual defect. User scope is untouched, so the enablement path still works. Both directions are pinned by the new workspace-settings.test.ts cases (400 + persistSetting not called; 200 + called).
packages/cli/src/config/settings.ts is a pure relocation: the module-local WORKSPACE_RESTRICTED_ROOT_SETTINGS const is deleted and the shared export from settingsUtils.ts is imported instead. The strip loop and the "ignored" warning that consume it are unchanged in this delta, so there is one list now rather than two that could drift. No behavior change.
One bot claim I verified myself rather than accepting
The round-15 approval argues the Array.isArray guard dropped from advisor.ts was unreachable. Confirmed at head: asJsonObject (packages/core/src/agents/forkedAgent.ts:373-377) rejects arrays through !Array.isArray(value), jsonResult is declared Record<string, unknown> | undefined (line 607), and both of its assignment sites (618, 649) route through asJsonObject. An array can never reach parseReview, and parseReview still throws on a missing or non-string field, so malformed structured output fails loudly.
My three prior gates
- CI. Now green on this exact head, zero failures:
Test (ubuntu-latest, Node 22.x),Lint & Static,Integration Tests (no-AK, No Sandbox),Serve A/B,Desktop Shell (ubuntu-22.04)and(windows-2022),TUI parity snapshots,OpenTUI no-flicker gate,Real daemon E2E / Java 11, and the Java matrix — 22 passing. - The unreviewed
packages/web-shell/client/settings.tsedit. No longer in the PR's file list at all. - The nine conflict resolutions. Covered since: chiga0's round-1 pass at
8824c5c7and round-2 atf10c2d94(which re-verifies R13-1 in code atconfig.ts:11247/:6124), plus the bot's round-15 scope, which explicitly took the post-round-13 surface including08fc82e2.
Unresolved threads
134 threads total, 3 unresolved. All three are labelled Minor by their author — the duplicated parseJsonObjectText, the case-sensitivity of the inherit guard, and the case-sensitivity of the review subcommand — and each has a written disposition from yiliang114 declining it as out of scope for a bounded bug-fix pass. None is merge-blocking, and I am not re-litigating an adjudicated Minor here.
Non-blocking residue
web-shell E2E Smoke is still in progress on this head (it was cancelled at 13ffa6e5 when the head moved), so the one lane that would exercise the daemon settings route end-to-end has not completed; Serve A/B and the unit lane that runs the new guard tests both pass, which is why this does not block. review-pr is also pending. Separately, reviewDecision: CHANGES_REQUESTED is stale — every review in that state belongs to qwen-code-ci-bot, the newest at d536a687 from 2026-08-29. No human change request stands against this head.
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
All 3 round-13 blockers addressed at HEAD 5c96f29f (commit cda083bf fix(advisor): enforce independent model availability):
| Bot-ledger ID | Finding summary | Status at HEAD |
|---|---|---|
| R13-1 | Advisor unregister/re-register not safe when disabledTools contains advisor — disabled tool re-registers after every setAdvisorModel call |
Fixed — syncAdvisorToolRegistration in packages/core/src/config/config.ts now checks getDisabledTools().has(ToolNames.ADVISOR) before the unregister/re-register cycle; setAdvisorModel also returns false early when the tool is disabled |
| R13-2 | Unresolvable advisor selector silently fell back to executor model | Fixed — packages/core/src/tools/advisor.ts execute() now returns advisorErrorResult(new Error('Advisor model is no longer available.')) when resolvedModel is undefined |
| R13-3 | checkAdvisorModelAvailability accepted inherit as a valid selector |
Fixed — explicit if (modelSelector.trim() === 'inherit') throw new Error('Advisor cannot inherit the executor model.') added in packages/cli/src/config/advisor-model.ts |
Earlier CI-bot Criticals from rounds 1–2 also resolved:
- hot-reload.ts TypeError on non-string
advisorModel: advisor sync block removed fromhot-reload.ts; no advisor references remain in that file at HEAD. - settingsSchema.ts
advisorMaxUsesvalidation gap: field removed from schema entirely (+3/-3in the diff replaces the oldadvisorModelfield metadata —requiresRestart: false→true, description updated,showInDialog: true→false;advisorMaxUsesis absent at HEAD). resolveAdvisorModeltrim without type guard (R2-5): fixed —packages/cli/src/config/config.ts:1256now usestypeof raw === 'string' ? raw.trim() : undefined, correctly handling non-string settings values from hand-edited config files.- scope-shadowing in
/advisor off/ Settings Dialog (advisor-command.ts,DialogManager.tsx): persists toSettingScope.Userin the current code; carries forward as a non-blocker per round-13 posture — the in-memorysetAdvisorModel(undefined)still disables the advisor for the running session.
Deferred non-blockers from round 13 (carried forward as follow-up work, not requested in this round):
R13-36 (Critical [fails-closed, new-surface]: auth-qualified startup rejection of a model identical to the runtime executor) · R13-1–R13-25 + 16 additional per the bot's ledger. None of these items have changed character at the current head.
No new blockers found in the 60-file diff at HEAD 5c96f29f.
Reviewed with AI assistance.
…s-p0-p8 Brings in main through 9534a52. The Runtime Broker README and QWEN.md keep the branch's text and add QwenLM#12681's Workspace binding sections. cli/src/config/config.ts keeps QwenLM#9636's advisor helpers next to the branch's isDebugMode(argv, environment), and environment.test.ts imports both sides' helpers.
The base refresh brought in the native advisor tool (#9636), and `places every built-in tool name` failed with `expected [ 'advisor' ] to deeply equal []`. AdvisorTool is constructed with shouldDefer=true and alwaysLoad=false (advisor.ts:210-224), so it belongs in DEFERRED: it stays out of the first request until tool_search loads it, and it gets no size ceiling because a deferred declaration is not resident cost. Listed first to keep DEFERRED alphabetical. Verified: 868 passed / 0 failed across the eight touched packages/core/src/tools suites (the count is the previous 867 plus the new `advisor stays deferred` case), core typecheck clean, prettier clean. Co-authored-by: Qwen-Coder <[email protected]>
…surface (QwenLM#12532) * perf(core): trim restated tool guidance and budget the resident tool surface Every resident tool's description and schema is sent on every request. Remove text that restates the tool's own schema, repeats the system prompt, or contradicts itself, and pin a per-turn size budget on every built-in tool that had none (QwenLM#12054). Trimmed, in each case without losing a rule the model can act on: - run_shell_command: "The command argument is required" restated `required`; "5-10 words" contradicted the `description` parameter's "up to 3 sentences"; the git status/git diff and mkdir/cp examples restated the sentence before them; the foreground list recommended `ls`, `cat`, `grep`, which the same description forbids, and is collapsed to one line. - read_file: the PDF page cap and the notebook offset/limit/pages rule are already stated on the `pages`/`offset`/`limit` parameters. - edit: the closing replace_all line repeated the opening paragraph and the parameter; a stray six-space indent is removed. - glob, grep_search: drop the parallel-call reminder (the system prompt owns it) and the unconditional "use the Agent tool" pointer, which names a tool that may not be registered. The call-boundary clauses shell.test.ts pins stay byte-identical. tool-surface-budget.test.ts adds a ceiling for 25 resident tools (measurement plus ~2%) and pins which 20 tools are deferred, since a tool flipping to resident is the largest per-turn change a one-line diff can make. agent, run_shell_command and workflow keep their existing budgets. Not run locally: vitest, typecheck and eslint are left to CI. Sizes were measured by instantiating each tool with the test's own minimal config in a single tsx process; the shell snapshot was compared byte-for-byte against the rendered non-Windows description. * test(core): make the tool budget cover the whole built-in surface Review of QwenLM#12532: - exit_plan_mode is shouldDefer but alwaysLoad (QwenLM#5210), so it is sent with every request. It moves to the resident list with a 2,450 ceiling, and the deferred list now also asserts alwaysLoad is false. - exec's class schema is never what a request carries: code mode declares it with every bound tool folded in. The row is removed and the reason recorded. - skill (2,313), team_create (7,619) and artifact (1,754) get ceilings; omni_recall_media_memory joins the deferred list. - A completeness check fails when a ToolNames entry is not placed in the resident list, the deferred list, or the documented not-budgeted set. - The trimmed edit, glob, grep and read_file descriptions get toContain assertions for the clauses a future trim must keep. - shell.test.ts budgets were still the pre-trim ones; each shape is re-measured (bash 4,315, Git Bash 4,140, powershell.exe 4,040, pwsh.exe 3,934, cmd.exe 3,791) and budgeted at measurement plus ~350 as before. Measured with the test's own minimal config in a tsx process; the new file was also executed there against a vitest shim (82 cases, all pass). * fix(core): keep the Agent-tool steering for open-ended searches The trim removed the "use the Agent tool for open-ended searches" clause from glob, grep and ripGrep. Unlike every other line this PR removes, that clause is not restated anywhere else: git grep -l across main for "open ended search" and "open-ended searches" in packages/core/src and packages/cli/src returns exactly those three files. Removing it from all three deletes the steering from the whole model-facing surface rather than de-duplicating it, which is outside this PR stated scope of trimming *restated* guidance. Restored byte-identical to main in all three descriptions, and added to the load-bearing-clause oracle so a future trim cannot drop it silently. Re-measured the three declarations (budgets temporarily set to 1 to read the actual sizes off the assertion) and set each ceiling to the next multiple of 50 above the measurement: ripgrep 1204 to 1250, grep fallback 1078 to 1100, glob 886 to 900. The ripgrep row moves up so RESIDENT stays in descending order. The other removals are unchanged; each was confirmed to be a genuine restatement: edit replace_all line (edit.ts:824), read_file page cap and notebook argument rule (read-file.ts:613, enforced at :669/:695), glob parallel-call line (prompts.ts:471). Verified: 877 passed / 0 failed across the ten touched packages/core/src/tools suites, core typecheck clean, prettier clean. Co-authored-by: Qwen-Coder <[email protected]> * test(core): place the new advisor tool in the surface budget census The base refresh brought in the native advisor tool (QwenLM#9636), and `places every built-in tool name` failed with `expected [ 'advisor' ] to deeply equal []`. AdvisorTool is constructed with shouldDefer=true and alwaysLoad=false (advisor.ts:210-224), so it belongs in DEFERRED: it stays out of the first request until tool_search loads it, and it gets no size ceiling because a deferred declaration is not resident cost. Listed first to keep DEFERRED alphabetical. Verified: 868 passed / 0 failed across the eight touched packages/core/src/tools suites (the count is the previous 867 plus the new `advisor stays deferred` case), core typecheck clean, prettier clean. Co-authored-by: Qwen-Coder <[email protected]> * Revert "fix(core): keep the Agent-tool steering for open-ended searches" This reverts f2456fa. The restoration was my error, not a review-driven fix. I ordered it on the reasoning that the clause is not restated anywhere on main, so removing it deleted steering rather than de-duplicating it. That missed the reason the trim was made, which the PR description already states: the clause names a tool that may not be declared in this session, which is the same dangling-reference defect QwenLM#12145 fixed for the system prompt. QwenLM#12145 (merged 2026-09-18) made the base prompt describe only the tools a session actually declares and left no "Agent tool" mention in prompts.ts, so glob, grep and ripGrep were the last static references to it. Tool descriptions cannot be line-gated the way the prompt now is, so dropping the reference is the consistent fix and restoring it re-opened the defect QwenLM#12145 closed. Reverting returns grep.ts and ripGrep.ts to the trimmed shape, glob.ts to trimming both clauses, and the three budgets to their measured values (ripgrep 1200, grep fallback 1050, glob 800). The base refresh and the advisor census placement are kept. Verified: 878 passed / 0 failed across the ten touched packages/core/src/tools suites, core typecheck clean, prettier clean. Co-authored-by: Qwen-Coder <[email protected]> --------- Co-authored-by: Qwen-Coder <[email protected]>
7 participants
What this PR does
This PR adds an opt-in native Advisor tool. When a separate Advisor model is configured, the executor can call the no-argument
advisor {}tool for an independent second opinion and continue the same task with the review as a tool result.Users can open the configured-model picker with
/advisor, select directly with/advisor <model>, disable it with/advisor off, or apply a session startup override with--advisor <model|off>. Invalid model selectors are rejected instead of being persisted or sent to a provider. When Advisor is off, the tool is not registered and is absent from subsequent executor requests.Persisted Advisor selection is restricted to user and system settings. A repository's Workspace setting cannot enable or override Advisor; it is ignored with a warning. The explicit
--advisorsession override still takes precedence.The Advisor receives the executor system instruction, tool declarations, and conversation up to the call point. Hidden reasoning and binary payloads are omitted. It uses the selected independent model, has no executable workspace tools, does not use model fallbacks, and returns a validated review with verdict, risks, missing evidence, and recommendation. Failures are non-fatal so the executor can continue; cancellation still cancels the active turn.
The structured-review path also changes the shared forked-agent
jsonSchemamechanism used by existing follow-up suggestion, speculation,/btw, and manual Advisor review callers. These requests now use the syntheticrespond_in_schematool, with text JSON parsing retained as a fallback when a provider does not force the tool.Native results reuse the existing Advisor presentation: the same four-title Markdown is rendered inside the existing cyan
/advisor · modelbordered view. The executor continuation remains below it throughout streaming and final history. Web Shell receives the same structured review through the normal tool event path.Why it's needed
Issue #9036 requests an executor-visible Advisor capability comparable to Claude Code: the main model should be able to consult a separately configured model without granting that model workspace execution permissions. The existing implementation only offered a user-triggered review, so its output could not be reinjected into an in-flight executor task.
Reviewer Test Plan
How to verify
Run
/advisorin the interactive TUI and confirm the picker containsOffand only configured eligible models. Select a model, ask the executor to handle a non-trivial task, and confirm it can calladvisor {}. The completed result should use the existing cyan/advisor · modelborder and showVerdict,Risks,Missing evidence, andRecommendation; the executor's continuation should remain below it.Run
/advisor off, then send another prompt. Confirm the next executor request does not expose Advisor and no Advisor model request occurs. Separately test--advisor <model>and--advisor off. Unknown, ineligible,inherit, or unresolved selectors should fail configuration instead of silently falling back to the executor.Place a different
advisorModelin Workspace settings and restart. Confirm it is ignored with a warning and cannot override the user selection or enable Advisor by itself.With an OpenAI-compatible mock provider, confirm the Advisor request uses the configured independent model, contains the conversation through the call point, and has only
respond_in_schema, with no executable workspace tools. A malformed response should produce a non-fatal tool error. A response with the four required fields plus extra fields should retain the four-field review.Evidence (Before & After)
Before: the executor had no native Advisor tool. Advisor was a manual review command whose output was not returned to the executor as part of the active task.
After: the executor can call
advisor {}, the independent review is reinjected as a tool result, and the existing presentation is preserved:A real tmux run against the bundled CLI verified the picker, independent model call, four-title cyan border, stable continuation ordering, and
/advisor offremoval. Maintainer verification additionally built and drove the merge result on Linux against a recording mock provider.Automated verification includes focused Core and CLI Advisor/configuration/display tests, full typecheck, build and bundle, and
integration-tests/cli/advisor-tool.test.ts(3/3) against the bundled CLI.Tested on
Environment (optional)
macOS and Linux x86_64, Node.js 22+, bundled CLI, isolated runtime/config directories, tmux, and a local OpenAI-compatible fake provider.
Risk & Scope
/advisornow configures the native Advisor model; manual review moves to/advisor review [focus]. LeavingadvisorModelempty disables Advisor rather than falling back to the executor model. Workspace-leveladvisorModelvalues are ignored because repository settings must not opt a user into sending the active conversation to another model/provider.Linked Issues
References #9036.
中文说明
这个 PR 做了什么
这个 PR 增加可选的原生 Advisor 工具。配置独立 Advisor 模型后，executor 可以调用无参数的
advisor {}获取第二意见，并将评审作为工具结果回注，在同一个任务中继续执行。用户可以通过
/advisor打开模型选择器，通过/advisor <model>直接选择，通过/advisor off禁用，或用--advisor <model|off>覆盖当前 session。无效模型不会被持久化或发送给 provider；关闭后工具不会注册。持久化选择只接受 User 和 System 设置。Workspace 设置不能启用或覆盖 Advisor；该值会被忽略并给出警告。
Advisor 会收到 executor 的 system instruction、工具声明和调用点之前的会话。隐藏推理和二进制载荷会被移除。Advisor 使用独立模型，不具备 workspace 可执行工具，不使用模型 fallback，并返回 verdict、risks、missing evidence 和 recommendation。失败是非致命的，取消仍会取消当前 turn。
结构化评审路径还会改变既有 follow-up suggestion、speculation、
/btw和手动 Advisor review 共同使用的 forked-agentjsonSchema机制：这些请求现在使用合成的respond_in_schema工具；当 provider 未强制该工具时，保留文本 JSON 解析兜底。原生结果复用既有四标题青色
/advisor · model边框，executor 后续输出稳定保持在其下方。Web Shell 通过普通工具事件接收同一份结构化评审。如何验证
在 TUI 中运行
/advisor，确认 picker 包含Off且只列出符合要求的模型。选择模型后让 executor 处理非简单任务，确认它可以调用advisor {}，并显示四标题边框。运行/advisor off后，后续请求不应再暴露 Advisor。在 Workspace 设置中写入
advisorModel，确认它被忽略且不能覆盖 User 选择。使用 mock provider 确认 Advisor 请求使用独立模型、只携带respond_in_schema，且格式错误时 executor 可以继续。macOS 与 Linux 已验证；focused Core/CLI 测试、全仓 typecheck、build、bundle，以及 bundled CLI 的 Advisor E2E 3/3 均通过。
风险与范围
/advisor现在配置原生 Advisor；手动评审入口为/advisor review [focus]。空advisorModel表示关闭，不再回退到 executor。WorkspaceadvisorModel会被忽略。关联 Issue
References #9036.