Thanks for the PR, @doudouOUC!

Template looks good ✓

On direction: aligning docs with the current CLI behavior and improving MCP OAuth error messages is squarely within scope. Documentation drift — stale command names, broken links, wrong config keys — directly hurts users. The Ink internal-import cleanup is a real build fix (Ink no longer exports those subpaths). CHANGELOG has prior entries for MCP OAuth and docs consistency work, confirming this area is actively maintained.

On approach: the scope is large (43 files, +1412/-381), but the code changes are tightly coupled to the docs they support. The /mcp auth deprecation path, plaintext-token warning, and 401 error-message improvements all exist because the docs now point users at the /mcp dialog — without them the docs would lie. The BaseTextInput Ink cleanup is a standalone build fix that arguably belongs in its own PR, but it's small and self-contained. No scope creep beyond the stated goal.

Moving on to code review. 🔍

— Qwen Code · qwen3.7-max

Code Review

Reviewed all 43 changed files. The code changes are well-structured and focused:

• mcpCommand.ts: The legacy auth/noauth subcommand handler is clean — returns a warning message pointing users to the /mcp dialog, with the server name interpolated safely via i18n t(). The argumentHint is correctly narrowed to desc|nodesc|schema.
• oauth-token-storage.ts: The plaintext storage warning uses a module-level didWarnPlaintextTokenStorage flag to fire exactly once per process. The try/catch around process.stderr.write is correct — stderr is best-effort, token persistence has already succeeded.
• mcp-client.ts: The getMcpOAuthDialogInstruction helper centralizes recovery messaging. The connectToMcpServer 401 handler is substantially improved — it snapshots credentials before createTransport (whose internal getValidToken may purge them), then uses a three-state discriminator (accepted-token-rejected, unusable, missing) for accurate post-401 messaging. The duplicated SSE-OAuth-handling blocks from the old code are properly consolidated.
• BaseTextInput.tsx: Replaces ink/dom addLayoutListener and CursorContext (both internal APIs) with public useBoxMetrics/useCursor hooks and useInsertionEffect. The extracted getAbsolutePosition and getPhysicalCursorPosition helpers are well-tested and exported for reuse.

No correctness bugs, security issues, or convention violations found. The reuse-before-new-code check passes — getMcpOAuthDialogInstruction replaces three copy-pasted error strings, and the token-state discriminator replaces duplicated 401-handling blocks.

Build & Typecheck

npm run build         → exit 0 (15 pre-existing vscode companion warnings, 0 errors)
npm run typecheck     → exit 0

Tests

packages/core:  oauth-token-storage (31), oauth-provider (27), mcp-client (89) → 147 passed
packages/cli:   mcpCommand (10), BaseTextInput (12), McpStatus (12)            → 34 passed
packages/web-shell: slashCompletion (14)                                        → 14 passed
Total: 195 tests, all passing

Real-Scenario Test

$ npm run dev -- -p 'say hello'
> @qwen-code/[email protected] dev
> node scripts/dev.js -p say hello

DEV is set to true, but the React DevTools server is not running. Start it with:
$ npx react-devtools

Hello! How can I help you today?

Dev build starts cleanly and responds normally. The /mcp auth deprecation path is interactive-only (slash command), so it's covered by the unit tests above rather than headless -p mode.

— Qwen Code · qwen3.7-max

Stepping back to look at the whole picture: this PR solves a real problem — docs had drifted from the code, pointing users at removed commands (/mcp auth <server-name>) and wrong config keys ("theme" vs "ui.theme"). The code changes exist to make the docs true in practice, not just on paper.

My independent proposal for this problem would have been: (1) fix the docs, (2) add the /mcp auth deprecation warning, (3) improve the 401 error messages, (4) add the plaintext-token warning. That's essentially what this PR does, and it does it well. The getMcpOAuthDialogInstruction helper avoids string duplication across four error paths, and the three-state token discriminator in connectToMcpServer replaces copy-pasted 401 blocks with something maintainable.

The BaseTextInput Ink cleanup is the one piece that's tangentially related — it fixes a build break from Ink no longer exporting internal subpaths, but it's not strictly required by the docs changes. It's small (removes ~60 lines of internal-API gymnastics, replaces with ~15 lines using public hooks) and well-tested, so it's fine to ship together.

Build passes, typecheck passes, 195 tests pass, dev build runs cleanly. The PR already has CHANGES_REQUESTED from a prior round — the author has addressed the feedback and the current state looks solid.

No reservations. This is a thorough, well-motivated docs-and-consistency PR that ships the code changes needed to keep the docs honest.

— Qwen Code · qwen3.7-max

@qwen-code /triage

@qwen-code /triage

✅ Maintainer local verification — real build, typecheck, 190 unit tests, mutation test

Verified PR #5589 at head 8732cd720 in an isolated worktree with a fresh npm ci (Node 22.22.2). This is a real build of the whole workspace, not a diff read. My earlier Ink-import blocker is now fixed. Summary first, evidence below.

Verdict: safe to merge. Build + typecheck exit 0, all touched suites pass (190 tests), and a mutation test proves the substantive code changes are well-guarded. The open CHANGES_REQUESTED items are AI-reviewer [Suggestion]s, not correctness blockers (assessed individually below). CI is already green on macOS/Ubuntu/Windows; this is an independent local cross-check.

1. My prior blocker (the ink/dom import) — RESOLVED ✅

Earlier I requested changes because BaseTextInput.tsx imported Ink internal subpaths not exported by the current Ink package (a build/runtime break). Current head migrates to the public Ink 7 API:

- import { addLayoutListener, type DOMElement } from 'ink/dom';
- import CursorContext from 'ink/components/CursorContext';
+ import { Box, Text, type DOMElement, useBoxMetrics, useCursor } from 'ink';

Confirmed [email protected] exports all three from its main entry (build/index.d.ts: useCursor, useBoxMetrics, export type { DOMElement }). No ink/ subpath imports remain anywhere in the file. The whole-workspace typecheck passes, and — as a bonus — the mutation test below shows the base BaseTextInput.tsx no longer even loads against the installed Ink 7, confirming the migration was necessary, not cosmetic.

2. Build + typecheck — exit 0 (the PR's central claim)

3. Unit tests — 190 passed across the 7 touched suites

4. Mutation test — substantive changes are non-vacuous

Reverted the 6 production files to base (31dcb6e32), kept the PR's test files, re-ran the suites:

Restoring the PR source → all green again. 29 guard tests flip with the fix removed.

Honest note on web-shell: reverting localCommands.ts did not fail slashCompletion.test.ts, because that test builds its own mock command list and asserts the completion logic, not the argumentHint string in localCommands.ts. So the one-line 'desc|nodesc|schema|auth|noauth' → 'desc|nodesc|schema' change is cosmetic and not independently asserted. Low risk; just flagging it accurately.

5. Code-change correctness highlights

• OAuth 401 recovery (mcp-client.ts) snapshots hadStoredSseOAuthCredentials before createTransport, because getValidToken inside transport creation may refresh/purge tokens — so the later 401 handler can correctly distinguish "had credentials, now unusable" (unusable) from "never configured" (missing). All getCredentials calls are now try/caught. Solid handling of a real race.
• Plaintext-token warning (oauth-token-storage.ts) fires in both setCredentials (L126) and deleteCredentials (L207, the rewrite path) — this addresses the earlier review note that deleteCredentials rewrote the plaintext file silently. One-shot per process, best-effort stderr.
• Legacy /mcp auth is gracefully deprecated, not silently dropped: mcpCommand.ts returns a warning message redirecting to the /mcp dialog.
• The new guidance is factually accurate — it points headless/SDK users to qwen mcp add --oauth-*, and those flags genuinely exist (--oauth-client-id/-secret/-redirect-uri/-authorization-url/-token-url/-scopes in commands/mcp/add.ts).

6. Documentation consistency

• All 200 relative links across the 29 changed docs resolve (with .md/index/README resolution). No broken local links introduced.
• /mcp auth is removed from every user-facing doc, status tip, completion hint, and error message. The only 2 remaining references live in docs/design/f2-mcp-transport-pool.md — an internal design doc not touched by this PR and out of scope.

7. Open CHANGES_REQUESTED threads — both non-blocking

• BaseTextInput.tsx:129 PhysicalCursorState not exported — the bot labels it [Suggestion]; the function is an internal (test-only) helper and typecheck is clean, so this is a stylistic nit, not a TS error or blocker.
• mcp-client.test.ts:412 coverage gaps — [Suggestion] to add the OAuth discovery happy-path and the handleAutomaticOAuth fallback-branch tests. The PR already covers the error paths (its actual subject) with 12+ new tests; these are nice-to-have additions, not correctness blockers.

The active CHANGES_REQUESTED is from the AI qwen-code-ci-bot with [Suggestion]-level items; the one substantive blocker (Ink) is fixed and CI is green on all three platforms.

Risk notes (low)

• Broad doc sweep + OAuth error-message rewrites; risk is wording, not behavior. Error paths are now actionable and test-covered.
• Cosmetic web-shell hint string is untested (see §4 note).

- import { addLayoutListener, type DOMElement } from 'ink/dom';
- import CursorContext from 'ink/components/CursorContext';
+ import { Box, Text, type DOMElement, useBoxMetrics, useCursor } from 'ink';

@qwen-code /triage

Maintainer local verification — real build + tests + docs link/consistency checks

I verified this PR locally on the latest head (31b649bae): a clean build/typecheck, the focused test suites, a real doc-link resolver over the changed docs, and doc↔code consistency checks. Verdict: mergeable — the docs sweep is accurate, the MCP OAuth error refactor is sound, and tests pass. One item is worth addressing before merge: the BaseTextInput.tsx change is justified as a build fix, but that justification does not hold at the pinned Ink version (details below).

Environment

• Clean worktree at PR head 31b649bae, fresh npm ci + full build; tsc -p packages/cli/tsconfig.json and prettier/eslint on changed files; a doc-link resolver; a virtual git merge-tree against current origin/main.

Results

⚠️ The BaseTextInput.tsx change is a refactor, not a build fix — the stated rationale doesn't hold

The PR description says the Ink change is required because "the previous internal subpath imports are no longer exported by the current Ink package, which breaks workspace builds." Against the repo's pinned [email protected] (unchanged on both the merge-base and current main), that is not the case:

• [email protected] still exports both old symbols at runtime and at the type level: ink/dom exports addLayoutListener (declared in build/dom.d.ts:60), and ink/components/CursorContext exports the default CursorContext context (build/components/CursorContext.d.ts). Both subpaths are in Ink's exports map.
• origin/main's BaseTextInput.tsx still uses exactly these old imports and ships green (released as v0.19.1).
• I restored origin/main's BaseTextInput.tsx into this worktree and typechecked it against the installed [email protected]: zero ink/dom / addLayoutListener / CursorContext errors (the only errors were the PR's new test referencing the new exports, which is expected when mixing old source with the new test).

So migrating off the internal subpaths to the public useBoxMetrics / useCursor API is a reasonable maintainability improvement in its own right (internal subpaths can break on a future Ink bump), but it is not needed to keep the build working today, and it's somewhat out of scope for a docs-alignment PR. It also rewrites the IME physical-cursor-positioning path (layout-listener → render-time compute + useInsertionEffect); the new pure helpers are unit-tested, but the render-cycle timing isn't exercised by the tests. The PR's own code typechecks and the merge stays coherent, so this isn't a correctness blocker — but I'd suggest either splitting it into its own change (with an accurate "move off internal Ink subpaths" rationale and a quick real-terminal IME cursor check) or correcting the justification here.

Minor / non-blocking

• Two /mcp auth <name> references remain in docs/design/f2-mcp-transport-pool.md (a historical design doc, not user-facing). The user-doc sweep didn't touch it; optional follow-up for full consistency.
• oauth-token-storage.ts writes the new plaintext-storage warning straight to process.stderr (once per process). Fine functionally, but raw stderr writes can interleave with the Ink TUI — routing it through the app's notice channel would be cleaner. Also slightly beyond the PR's stated scope.
• The branch is 19 commits behind origin/main; the merge is verified clean and coherent, so this is hygiene only — a rebase keeps CI reflecting current main.

What's solid

The MCP OAuth error refactor is well done: a single getMcpOAuthDialogInstruction() helper replaces every stale /mcp auth string with /mcp-dialog + qwen mcp add --oauth-* guidance (both verified to exist); a 3-state token machine (accepted-token-rejected / unusable / missing) gives accurate failure messaging; a pre-createTransport credential snapshot correctly distinguishes "had credentials, now unusable" from "never configured"; and credential/discovery reads are hardened with try/catch. Docs and SDK wording now match the code, links resolve, and all focused tests pass.

Verdict

Mergeable. The substance (docs accuracy, MCP OAuth recovery guidance, completions) is correct and verified. Recommend addressing the one item — split or re-justify the BaseTextInput.tsx Ink change (it's a refactor, not the build fix the description claims) — and rebasing onto current main.