feat(tui): make settings MCP recovery first-class and clickable

[Hmbown]

Why

Settings and MCP recovery were a path/command wall. Plugin problems (invalid manifests, duplicates, stale OAuth) were not diagnosable from the session product surface. Recovery copy on main still names /mcp auth, which does not exist.

This is not the session-boot hang fix. First-turn invisible MCP/plugin connecting remains a separate lane. This PR is the Settings / Extensions / /mcp recovery surface.

What

• Clickable Settings tabs (General / Models / Permissions / Display / Advanced)
• Advanced → MCP action rows: /mcp, /mcp reload, /mcp validate, /plugin
• Extensions → MCP: Connect / Reconnect / Re-auth / Diagnose / Enable using real commands (/mcp login, never /mcp auth)
• Plugin Problems group for invalid manifests and duplicates
• /mcp pager names a per-server next action
• OpenRouter Hunyuan aliases (hy3 / hunyuan); no invented hy4

Tests

Focused codewhale-tui --lib settings/MCP/plugin tests + cargo fmt --check + coauthor trailer check.

No-Issue: settings/MCP recovery surface; no GitHub issue to close.

Do not merge while required GitHub checks are red or still running.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

[claude]

Claude finished @Hmbown's task in 3m 51s —— View job

---

Review: feat(tui): make settings MCP recovery first-class and clickable

• Gather context (diff, changed files, custom instructions)
• Review correctness (logic errors, panics, unwraps, race conditions, non-exhaustive matches)
• Review provider/model/route safety (EPIC v0.8.65 EPIC: Separate provider facts, model facts, offerings, and route resolution #2608 invariant)
• Review reuse/simplification/dead code
• Review test coverage
• Review security (secrets, exec policy, input validation)
• Post final review summary

This is a focused, well-scoped slice that does what the PR description says: it kills the last /mcp auth reference, gives Settings/Extensions//mcp a real recovery path (McpRecoveryKind + mcp_recovery_kind in crates/tui/src/mcp.rs), and adds Hunyuan aliases without inventing an hy4 id. Logic is well covered by new unit tests (mcp_recovery_kind_names_real_login_and_reload_commands, mcp_item_action_for_stale_oauth_is_login, config_view_tabs_are_clickable, etc.), and all 15 locale packs got real translations for the new message IDs, not just English copy-paste.

1. Correctness — mismatched action label/command in the /mcp pager (minor)

crates/tui/src/tui/mcp_routing.rs:72-90 (push_server) computes command and verb from recovery independently:

let command = if crate::mcp::mcp_name_is_command_safe(&server.name)
    || matches!(recovery, Connect | Reconnect | Diagnose) {
    recovery.slash_command(&server.name)
} else {
    "/mcp validate".to_string()   // fallback for unsafe names
};
let verb = match recovery { Enable => "Enable", ..., Reauth => "Re-auth", ... };
lines.push(format!("  next: {verb} {command}"));

When a configured server name fails mcp_name_is_command_safe (any char outside [A-Za-z0-9._-] — server names are unconstrained JSON object keys in mcp.json, so this is reachable) and the recovery kind is Enable or Reauth, command falls back to /mcp validate, but verb still prints "Enable" or "Re-auth". The pager then shows e.g. next: Enable /mcp validate or next: Re-auth /mcp validate — a verb that doesn't match the command actually named, which is exactly the "recovery copy must name commands that exist and be accurate" problem this PR sets out to fix.

Compare with the equivalent fallback in crates/tui/src/tui/views/extensions.rs:958-974, which gets this right — when the name is unsafe, it explicitly overrides both the label and the command to Diagnose / /mcp validate together, so they stay consistent. push_server should do the same: when falling back to /mcp validate, also force verb = "Diagnose" (or otherwise derive verb from whatever recovery/command pair was actually chosen, not from the original recovery value).

Fix this →

2. Reuse — the safe-name fallback logic is duplicated and has now diverged

The "is this server name safe to interpolate into a slash command, and what do we show if not" logic is implemented twice (mcp_routing.rs::push_server and extensions.rs::mcp_model), and finding #1 shows the two copies have already drifted. Consider a small helper (e.g. McpRecoveryKind::command_and_label(name) -> (String, MessageId/&str)) that both call sites use, so the safety fallback can't diverge again.

3. Provider/model/route safety (EPIC #2608)

The Hunyuan alias additions (crates/agent/src/lib.rs, crates/config/src/lib.rs, crates/tui/src/config.rs) add literal aliases (hy3, hunyuan, tencent-hunyuan, hunyuan-hy3) to an explicit match-arm allowlist that resolves to the single already-provider-scoped OpenRouter id tencent/hy3-preview. This is alias resolution by exact literal match, not inference of provider from a prefix or freeform text — no invariant violation. Good that hy4 was deliberately not added, and that's covered by a test (openrouter_hunyuan_aliases_resolve_to_hy3_preview).

Minor note (pre-existing pattern, not introduced by this PR): canonical_openrouter_recent_model_id is duplicated verbatim between crates/config/src/lib.rs and crates/tui/src/config.rs. This PR correctly kept both copies in sync, but it's a standing risk that a future alias addition updates only one.

4. error_text_looks_auth_required broadening (crates/tui/src/mcp/oauth.rs)

Adding "re-authorize", "/mcp login", "mcp login" as auth-required substrings is a reasonable way to make the classifier recognize the tool's own generated recovery hints. Checked all call sites (crates/tui/src/mcp.rs, crates/tui/src/lib.rs) — they're all used for display/hint purposes, not retry loops, so there's no feedback-loop risk from the broader match.

5. Tests

Good targeted coverage: mcp_recovery_kind truth table (including the important "401 but not oauth-capable → Diagnose, not Reauth" case), tab-click hitboxes, MCP action rows emitting the right command, locale-key presence, and the Hunyuan alias table. Nothing missing that I'd block on. One suggestion: a test for the mismatched-verb scenario in finding #1 (unsafe server name + disabled/reauth state) would have caught it, and would be a cheap regression guard once fixed.

6. Security

Server-name-to-slash-command interpolation is properly guarded by mcp_name_is_command_safe (ASCII alphanumeric + -_. only) before being spliced into /mcp login <name> / /mcp enable <name> in the two view models — no obvious injection path found. No secret handling changes; the OAuth hint helpers only emit static strings.

---

Not run: I could not execute cargo build/cargo test/cargo fmt --check in this sandbox (commands require approval I don't have here), so this review is based on manual reading of the diff plus cross-referencing struct/enum definitions and call sites. The PR body states the author's own focused codewhale-tui --lib test run (16 passed) plus cargo fmt --check and the coauthor trailer check — I have no reason to doubt that, but couldn't independently reproduce it.
· branch grok/v0912-tui-settings-mcp-20260827

[claude]

Claude finished @Hmbown's task in 2m 7s —— View job

---

Review: feat(tui): make settings MCP recovery first-class and clickable

• Gather context (diff, changed files, custom instructions)
• Check whether new commits address prior review findings
• Review correctness (logic errors, panics, unwraps, race conditions, non-exhaustive matches)
• Review provider/model/route safety (EPIC v0.8.65 EPIC: Separate provider facts, model facts, offerings, and route resolution #2608 invariant)
• Review reuse/simplification/dead code
• Review test coverage
• Review security (secrets, exec policy, input validation)
• Post final review summary

No new commits landed since the earlier automated review on this PR (still 12af2fe + the 6456117 handoff-doc commit) — this is a fresh read of the same diff against origin/main.

1. Correctness — mismatched action label/command in the /mcp pager (unresolved)

crates/tui/src/tui/mcp_routing.rs:72-90 (push_server) still computes command and verb independently:

let command = if crate::mcp::mcp_name_is_command_safe(&server.name)
    || matches!(recovery, Connect | Reconnect | Diagnose) {
    recovery.slash_command(&server.name)
} else {
    "/mcp validate".to_string()
};
let verb = match recovery { Enable => "Enable", ..., Reauth => "Re-auth", ... };
lines.push(format!("  next: {verb} {command}"));

Reachable case: a configured server name in mcp.json (server names are unconstrained JSON object keys) that fails mcp_name_is_command_safe (any char outside [A-Za-z0-9._-]), combined with recovery kind Enable or Reauth. command falls back to /mcp validate, but verb is still taken from the original recovery ("Enable" / "Re-auth"), so the pager prints e.g. next: Enable /mcp validate or next: Re-auth /mcp validate — a verb that doesn't name the command actually shown. That's exactly the "recovery copy must name commands that exist and be accurate" defect this PR is meant to fix.

crates/tui/src/tui/views/extensions.rs:958-974 (mcp_model) gets this right: when the name is unsafe, it overrides both the label and the command together to Diagnose / /mcp validate. push_server should do the same — derive verb from whichever (command, recovery) pair was actually chosen, not from the untouched recovery value.

No test currently exercises "unsafe server name + Enable/Reauth recovery," which is why this slipped through; manager_text_shows_failed_disabled_and_runtime_names and manager_text_names_login_for_stale_oauth both use safe names.

Fix this →

2. Reuse — safe-name fallback duplicated across two call sites, and it's already drifted

The "is this name safe to interpolate into a slash command, and what do we show if not" logic is implemented separately in mcp_routing.rs::push_server and extensions.rs::mcp_model; finding #1 shows the two copies disagree. A shared helper (e.g. McpRecoveryKind::command_and_label(name) -> (String, MessageId) on the enum in crates/tui/src/mcp.rs) would let both call sites consume the same fallback and prevent re-drift.

3. Provider/model/route safety (EPIC #2608) — clean

Hunyuan alias additions (crates/agent/src/lib.rs:494-503, crates/config/src/lib.rs:4442-4451, crates/tui/src/config.rs:1086-1095) add hy3, hunyuan, tencent-hunyuan, hunyuan-hy3 as exact literal match arms resolving to the single already-provider-scoped id tencent/hy3-preview (OpenRouter). This is literal-alias resolution, not prefix/freeform-text inference — no invariant violation. hy4 was correctly not invented, and both config and tui copies of canonical_openrouter_recent_model_id were kept in sync (pre-existing duplication between those two crates, not introduced here).

4. error_text_looks_auth_required broadening — verified, no feedback-loop risk

crates/tui/src/mcp/oauth.rs:52 gained "re-authorize", "/mcp login", "mcp login" as auth-required substrings. Traced every call site:

• crates/tui/src/mcp.rs:2921,2970 — display-only (annotates a catalog listing item).
• crates/tui/src/lib.rs:8721-8776 — display/hint generation.
• crates/tui/src/mcp/oauth.rs:264 (refresh_and_persist) — this one is stateful: a positive match clears stored OAuth tokens. However the matched text is the underlying OAuth-manager refresh error (HTTP/protocol-level), not a TUI-generated hint string, so "re-authorize"/"/mcp login" are very unlikely to appear there. No realistic feedback loop.

Also fixes the actual /mcp auth invalid-command references in crates/tui/src/mcp/streamable_http.rs (both the refresh-failure error message and the OAuth hint) by routing through the new tui_reauth_hint() / tui_reauth_refresh_failed_hint() helpers — good dedup, and it's the literal bug the PR description calls out fixing.

5. Settings tabs click handling — spot-checked, correct

crates/tui/src/tui/views/mod.rs: tab_y = inner.y.saturating_add(u16::from(!compact)) lines up correctly with the actual rendered row in both compact (tab_line at lines[0]) and non-compact (tab_line at lines[1], after the title line) layouts — verified against the lines construction just below. Hitboxes are cleared/rebuilt on the edit path too. Covered by config_view_tabs_are_clickable.

6. Tests

Good targeted coverage overall (recovery-kind truth table incl. the important "401 but not oauth-capable → Diagnose, not Reauth" case, tab-click hitboxes, MCP action rows, Hunyuan alias table). Confirmed all 15 locale files (ca, de, en, es-419, fr, hi, id, ja, ko, pt-BR, ru, uk, vi, zh-Hans, zh-Hant) contain the new ConfigActionOpenMcp/ConfigLabelPluginsOpen etc. keys, not just English. Missing: a regression test for the unsafe-name + Enable/Reauth mismatch from finding #1.

7. Security — clean

Server-name→slash-command interpolation is gated by mcp_name_is_command_safe (ASCII alphanumeric + -_. only) before splicing into /mcp login <name> / /mcp enable <name>; the fallback path (finding #1) is a copy/label bug, not an injection path. No secret handling changes; OAuth hint helpers only emit static strings.

---

Not run: cargo build/cargo test/cargo fmt --check require approval not available in this sandbox, so this is based on manual reading of the diff, cross-referencing struct/enum definitions and every call site of the touched functions, and a scripted check of locale-key presence across all 15 locale files. The PR body states the author's own focused codewhale-tui --lib run (16 passed) plus cargo fmt --check and the coauthor trailer check — no reason to doubt that, but it wasn't independently reproduced here.
· branch grok/v0912-tui-settings-mcp-20260827

[claude]

Claude finished @Hmbown's task in 54s —— View job

---

Review: feat(tui): make settings MCP recovery first-class and clickable

• Gather context (diff, changed files, custom instructions)
• Check whether new commit (fbc2e2c) addresses prior review findings
• Review correctness (logic errors, panics, unwraps, race conditions, non-exhaustive matches)
• Review provider/model/route safety (EPIC v0.8.65 EPIC: Separate provider facts, model facts, offerings, and route resolution #2608 invariant)
• Review reuse/simplification/dead code
• Review test coverage
• Review security (secrets, exec policy, input validation)
• Post final review summary

The only commit added since the last automated review is fbc2e2c, which corrects the Extensions* locale-key census assertion in crates/tui/src/localization.rs (82 → 86, to account for the four new Connect/Reconnect/Re-auth/Diagnose ExtensionsAction keys). It's a test-only fix and doesn't touch any of the recovery-routing logic, so it does not address the open finding below.

1. Correctness — mismatched action label/command in the /mcp pager (still unresolved)

crates/tui/src/tui/mcp_routing.rs:72-90 (push_server) still computes command and verb independently:

let command = if crate::mcp::mcp_name_is_command_safe(&server.name)
    || matches!(recovery, Connect | Reconnect | Diagnose) {
    recovery.slash_command(&server.name)
} else {
    "/mcp validate".to_string()
};
let verb = match recovery { Enable => "Enable", ..., Reauth => "Re-auth", ... };
lines.push(format!("  next: {verb} {command}"));

Reachable case: a configured server name in mcp.json (server names are unconstrained JSON object keys) that fails mcp_name_is_command_safe (any char outside [A-Za-z0-9._-]), combined with recovery kind Enable or Reauth. command falls back to /mcp validate, but verb is still taken from the original recovery ("Enable" / "Re-auth"), so the pager prints e.g. next: Enable /mcp validate or next: Re-auth /mcp validate — a verb naming an action that doesn't match the command shown. That's exactly the "recovery copy must name commands that exist and be accurate" defect this PR sets out to fix.

crates/tui/src/tui/views/extensions.rs:958-974 (mcp_model) gets this right: when the name is unsafe it overrides both the label and the command together to Diagnose / /mcp validate. push_server should mirror that — derive verb from whichever (command, recovery) pair was actually chosen, not from the untouched recovery value.

No test currently exercises "unsafe server name + Enable/Reauth recovery" in crates/tui/src/mcp/tests.rs, which is why it's slipped through two review passes.

Fix this →

2. Reuse — safe-name fallback still duplicated across two call sites

Unchanged from the prior pass: the "is this name safe to interpolate into a slash command, and what do we show if not" logic lives separately in mcp_routing.rs::push_server and extensions.rs::mcp_model, and finding #1 shows the two copies disagree. A shared helper (e.g. McpRecoveryKind::command_and_label(name) -> (String, MessageId) on the enum in crates/tui/src/mcp.rs) would let both call sites consume the same fallback and prevent re-drift.

3. Provider/model/route safety (EPIC #2608) — clean, unchanged

Hunyuan alias additions (crates/agent/src/lib.rs, crates/config/src/lib.rs, crates/tui/src/config.rs) remain exact literal match arms (hy3, hunyuan, tencent-hunyuan, hunyuan-hy3) resolving to the single already-provider-scoped id tencent/hy3-preview (OpenRouter) — literal-alias resolution, not prefix/freeform-text inference. No invariant violation; hy4 correctly not invented.

4. error_text_looks_auth_required broadening — unchanged, no feedback-loop risk

Same as previously verified: the new substrings ("re-authorize", "/mcp login", "mcp login") in crates/tui/src/mcp/oauth.rs are only reachable from display/hint call sites and the OAuth-manager's own upstream refresh error text, not from TUI-generated strings, so there's no realistic feedback loop.

5. Tests

The new fbc2e2c commit fixes a real gap (the census assertion was stale after the four new ExtensionsAction keys were added), so it's a legitimate correction, not a workaround. The regression-test gap from finding #1 is still open — recommend adding it alongside the fix.

6. Security — clean, unchanged

Server-name→slash-command interpolation is still gated by mcp_name_is_command_safe before splicing into /mcp login <name> / /mcp enable <name>; the fallback path (finding #1) is a copy/label bug, not an injection path.

---

Not run: cargo build/cargo test/cargo fmt --check require approval not available in this sandbox, so this is based on manual reading of the diff and cross-referencing the touched functions and their call sites (including confirming fbc2e2c only touches the locale test assertion).
· branch grok/v0912-tui-settings-mcp-20260827

[Hmbown]

Rebased onto current main after #5641, #5651, #5643, and #5662 landed. The only conflict was crates/tui/src/mcp/streamable_http.rs; kept the #5643 /mcp login recovery helpers already on main. Waiting for required CI (Lint, Test ubuntu/macos/windows, Version drift, npm wrapper) to go green and mergeStateStatus=CLEAN before merge.

[claude]

Claude finished @Hmbown's task in 2m 35s —— View job

---

Review: feat(tui): make settings MCP recovery first-class and clickable

• Gather context (diff vs origin/main, prior review history, CI status)
• Confirm current build status
• Review correctness (logic errors, panics, unwraps, race conditions, non-exhaustive matches)
• Review provider/model/route safety (EPIC v0.8.65 EPIC: Separate provider facts, model facts, offerings, and route resolution #2608 invariant)
• Review reuse/simplification/dead code
• Review test coverage
• Review security (secrets, exec policy, input validation)
• Post final review summary

No new commits since the last automated pass (top commit is still 40f1eb5, the locale-census-only fix), so this re-reads the same diff against origin/main — but this time cross-checked against live CI, which surfaces something the last three passes missed.

1. Correctness — the crate does not compile (blocking, unresolved)

crates/tui/src/mcp/streamable_http.rs:112 has a stray () that breaks the macro call and the surrounding delimiter balance:

let hint = unauthorized_session_hint(self.auth.oauth.is_some());
return Err(StreamableSendError::Other(anyhow::anyhow!()
    "MCP server {} rejected the request with {status}; the session is no longer accepted. {hint}",
    mask_url_secrets(&self.url),
)));

anyhow::anyhow!() closes the macro invocation immediately, so the format string and the trailing mask_url_secrets(&self.url) argument are left dangling outside of any call, and the delimiter count no longer balances. This isn't a lint nit — it's a hard parse error. Verified directly against this PR's own CI run rather than just reading the diff:

CI job cargo check (aarch64-unknown-linux-ohos) (run 33104341865) is already failing with:

error: unexpected closing delimiter: `}`
  --> crates/tui/src/mcp/streamable_http.rs:194:1
   |
95 |             if status == StatusCode::UNAUTHORIZED || status == StatusCode::FORBIDDEN {
   |                                                                                      - the nearest open delimiter
...
115 |                 )));
   |                   - missing open `(` for this delimiter

This is the standard cargo check invocation for the codewhale-tui lib target, so it fails identically on every target, not just OpenHarmony.

The diff for this hunk (introduced in 7e29bd5, present in every reviewed revision) is a single-token change:

-                return Err(StreamableSendError::Other(anyhow::anyhow!(
+                return Err(StreamableSendError::Other(anyhow::anyhow!()

Fix is to drop the stray () and restore the original open-paren macro call:

return Err(StreamableSendError::Other(anyhow::anyhow!(
    "MCP server {} rejected the request with {status}; the session is no longer accepted. {hint}",
    mask_url_secrets(&self.url),
)));

Since the crate fails to parse, the codewhale-tui --lib test run the PR description cites (16 passed) could not have exercised this file's compiled code as it stands now — worth double-checking locally after the fix that the full suite still passes.

Fix this →

2. Correctness — mismatched action label/command in the /mcp pager (still unresolved, flagged 3x prior)

crates/tui/src/tui/mcp_routing.rs:72-90 (push_server) still computes command and verb independently:

let command = if crate::mcp::mcp_name_is_command_safe(&server.name)
    || matches!(recovery, Connect | Reconnect | Diagnose) {
    recovery.slash_command(&server.name)
} else {
    "/mcp validate".to_string()
};
let verb = match recovery { Enable => "Enable", ..., Reauth => "Re-auth", ... };
lines.push(format!("  next: {verb} {command}"));

Reachable case: a configured server name in mcp.json (unconstrained JSON object key) that fails mcp_name_is_command_safe (any char outside [A-Za-z0-9._-]) combined with recovery kind Enable or Reauth. command falls back to /mcp validate but verb still comes from the untouched recovery value, printing e.g. next: Enable /mcp validate — a verb that doesn't match the shown command. crates/tui/src/tui/views/extensions.rs:958-974 (mcp_model) gets this right by overriding both label and command together. push_server should do the same. No regression test exists for "unsafe name + Enable/Reauth" in crates/tui/src/mcp/tests.rs.

3. Reuse — safe-name fallback still duplicated across two call sites

Unchanged: the "is this name command-safe, and what's the fallback if not" logic lives separately in mcp_routing.rs::push_server and extensions.rs::mcp_model, and finding #2 shows the copies have drifted. A shared helper (e.g. McpRecoveryKind::command_and_label(name) -> (String, MessageId)) would prevent re-drift.

4. Provider/model/route safety (EPIC #2608) — clean

Hunyuan alias additions (crates/agent/src/lib.rs, crates/config/src/lib.rs, crates/tui/src/config.rs) add hy3, hunyuan, tencent-hunyuan, hunyuan-hy3 as exact literal match arms resolving to the single already-provider-scoped id tencent/hy3-preview (OpenRouter). This is literal-alias resolution, not prefix/freeform-text inference — no invariant violation. hy4 correctly not invented, and both config/tui copies of canonical_openrouter_recent_model_id stayed in sync.

5. error_text_looks_auth_required broadening — clean, no feedback-loop risk

New substrings ("re-authorize", "/mcp login", "mcp login") in crates/tui/src/mcp/oauth.rs are only reachable from display/hint call sites and the OAuth manager's own upstream refresh-error text, not from TUI-generated strings that could re-trigger themselves.

6. Tests

Good targeted coverage of the recovery-kind truth table, tab-click hitboxes, MCP action rows, Hunyuan aliases, and all 15 locale files carrying the new keys. Gaps: no compile-time/CI guard would have caught #1 faster than "CI goes red" (that's expected — it's a syntax error, not a logic gap), and finding #2's unsafe-name + Enable/Reauth case still has no regression test.

7. Security — clean

Server-name→slash-command interpolation is still gated by mcp_name_is_command_safe before splicing into /mcp login <name> / /mcp enable <name>. No secret-handling changes; OAuth hint helpers emit static strings only.

---

Bottom line: this PR cannot merge as-is — the branch fails cargo check today (confirmed via live CI, run 33104341865), independent of the still-open cosmetic mismatch in finding #2. Fix #1 first; it's a one-token change.
· branch grok/v0912-tui-settings-mcp-20260827

[claude]

Claude finished @Hmbown's task in 1m 40s —— View job

---

Review: feat(tui): make settings MCP recovery first-class and clickable

• Gather context (diff vs origin/main, prior review history, CI status)
• Check whether the reported compile error (streamable_http.rs) is fixed
• Review correctness (logic errors, panics, unwraps, race conditions, non-exhaustive matches)
• Review provider/model/route safety (EPIC v0.8.65 EPIC: Separate provider facts, model facts, offerings, and route resolution #2608 invariant)
• Review reuse/simplification/dead code
• Review test coverage
• Review security (secrets, exec policy, input validation)
• Post final review summary

One new commit landed since the last automated pass: bc81d3e ("restore streamable HTTP auth-error syntax after rebase").

1. Compile error — fixed ✅

crates/tui/src/mcp/streamable_http.rs:112 previously had a stray anyhow::anyhow!() that broke delimiter balance and failed cargo check in CI (confirmed failing in run 33104341865, flagged in the prior review pass). bc81d3e is a single-line fix that restores the correct open-paren macro call:

let hint = unauthorized_session_hint(self.auth.oauth.is_some());
return Err(StreamableSendError::Other(anyhow::anyhow!(
    "MCP server {} rejected the request with {status}; the session is no longer accepted. {hint}",
    mask_url_secrets(&self.url),
)));

I couldn't run cargo check/cargo build directly in this sandbox (no approval for compiler invocation here, same limitation as prior passes), but current CI on this exact push (run 33104715580 group) shows zero failures so far — OpenHarmony (the job that previously failed on this exact error) and the main CI job are still in_progress/queued rather than red, which is consistent with the fix. Worth confirming they finish green before merge.

2. Correctness — mismatched action label/command in the /mcp pager (still unresolved — flagged 4x now)

crates/tui/src/tui/mcp_routing.rs:72-90 (push_server) still computes command and verb independently:

let command = if crate::mcp::mcp_name_is_command_safe(&server.name)
    || matches!(recovery, Connect | Reconnect | Diagnose) {
    recovery.slash_command(&server.name)
} else {
    "/mcp validate".to_string()
};
let verb = match recovery { Enable => "Enable", ..., Reauth => "Re-auth", ... };
lines.push(format!("  next: {verb} {command}"));

Reachable case: a configured server name in mcp.json (unconstrained JSON object key) that fails mcp_name_is_command_safe (any char outside [A-Za-z0-9._-]) combined with recovery kind Enable or Reauth. command falls back to /mcp validate but verb still comes from the untouched recovery value, so the pager prints e.g. next: Enable /mcp validate — a verb that doesn't name the command actually shown. This is exactly the "recovery copy must name commands that exist and be accurate" defect this PR sets out to fix, and it has slipped through every revision so far.

crates/tui/src/tui/views/extensions.rs:958-974 (mcp_model) gets this right — when the name is unsafe it overrides both the label and the command together to Diagnose / /mcp validate in one else branch. push_server should mirror that (derive verb from whichever (command, recovery) pair was actually chosen, not from the untouched recovery). No regression test exists yet for "unsafe server name + Enable/Reauth recovery" in crates/tui/src/mcp/tests.rs.

Fix this →

3. Reuse — safe-name fallback still duplicated across two call sites

Unchanged: the "is this name command-safe, and what's the fallback if not" logic lives separately in mcp_routing.rs::push_server and extensions.rs::mcp_model, and finding #2 shows the copies have drifted. A shared helper (e.g. McpRecoveryKind::command_and_label(name) -> (String, MessageId) on the enum in crates/tui/src/mcp.rs) would let both call sites consume the same fallback and prevent re-drift.

4. Provider/model/route safety (EPIC #2608) — clean, unchanged

Hunyuan alias additions (crates/agent/src/lib.rs, crates/config/src/lib.rs, crates/tui/src/config.rs) remain exact literal match arms (hy3, hunyuan, tencent-hunyuan, hunyuan-hy3) resolving to the single already-provider-scoped id tencent/hy3-preview (OpenRouter) — literal-alias resolution, not prefix/freeform-text inference. No invariant violation; hy4 correctly not invented, and both config/tui copies of canonical_openrouter_recent_model_id stayed in sync.

5. error_text_looks_auth_required broadening — clean, no feedback-loop risk

Unchanged from prior passes: new substrings ("re-authorize", "/mcp login", "mcp login") in crates/tui/src/mcp/oauth.rs are only reachable from display/hint call sites and the OAuth manager's own upstream refresh-error text, not from TUI-generated strings that could re-trigger themselves.

6. Tests

Good targeted coverage of the recovery-kind truth table, tab-click hitboxes, MCP action rows, Hunyuan aliases, and all 15 locale files carrying the new keys (the 40f1eb5 census-count fix was a legitimate correction after the four new ExtensionsAction keys were added). Still missing: a regression test for the unsafe-name + Enable/Reauth mismatch from finding #2.

7. Security — clean, unchanged

Server-name→slash-command interpolation is still gated by mcp_name_is_command_safe before splicing into /mcp login <name> / /mcp enable <name>; the fallback path (finding #2) is a copy/label bug, not an injection path. No secret-handling changes; OAuth hint helpers emit static strings only.

---

Bottom line: the blocking compile error is fixed. The one remaining open item across all four review passes is the cosmetic next: <verb> <command> mismatch in mcp_routing.rs::push_server (finding #2) — not a build blocker, but worth fixing before merge since it's the exact class of bug ("recovery copy must be accurate") this PR exists to close out.

Not run: cargo build/cargo test/cargo fmt --check require approval not available in this sandbox, so verification of the compile fix relies on manual delimiter-balance reading plus live CI status on this push (no failures reported so far; OpenHarmony/CI still in progress).
· branch grok/v0912-tui-settings-mcp-20260827

[Hmbown]

Follow-up bc81d3e8f restores the #5643 anyhow!( invocation in streamable_http.rs after the rebase accidentally closed it (unexpected closing delimiter). Required Lint/Safety were red on 40f1eb59e; not merging while those are red. Waiting for the new head to go CLEAN with Lint + Test ubuntu/macos/windows + Version drift + npm wrapper green.

[claude]

Claude finished @Hmbown's task in 3m 1s —— View job

---

Review: feat(tui): make settings MCP recovery first-class and clickable

• Gather context (diff, prior review history)
• Confirm current PR head via gh pr view (matches local checkout: d5b8e97, 3 commits — ddd63db, 3d8cff2, d5b8e97)
• Re-check prior open findings
• Post final review summary

Note on scope: at this HEAD, crates/tui/src/mcp/streamable_http.rs does not have the stray-() compile break a prior automated pass flagged — I read the file directly and it's the correct anyhow::anyhow!(...) call, and CI's cargo check (aarch64-unknown-linux-ohos) job (run 33104874845) just came back green for this exact commit. A rebase and a follow-up fix commit (bc81d3e8f) were described in this thread, but gh pr view 5655 reports the PR's commit list as still exactly ddd63db → 3d8cff2 → d5b8e97 — so that history isn't reflected on this branch as currently pushed. Reviewing what's actually here.

1. Correctness — mismatched action label/command in the /mcp pager (still open, raised on prior passes)

crates/tui/src/tui/mcp_routing.rs:72-90 (push_server) still derives command and verb independently:

let command = if crate::mcp::mcp_name_is_command_safe(&server.name)
    || matches!(recovery, Connect | Reconnect | Diagnose) {
    recovery.slash_command(&server.name)
} else {
    "/mcp validate".to_string()
};
let verb = match recovery { Enable => "Enable", ..., Reauth => "Re-auth", ... };
lines.push(format!("  next: {verb} {command}"));

Reachable case: a server name in mcp.json (unconstrained JSON object key) that fails mcp_name_is_command_safe (any char outside [A-Za-z0-9._-]) combined with recovery kind Enable or Reauth. command falls back to /mcp validate, but verb still comes from the untouched recovery value, so the pager prints e.g. next: Enable /mcp validate — a verb that doesn't name the command shown. That's exactly the "recovery copy must name commands that exist and be accurate" defect this PR sets out to fix.

crates/tui/src/tui/views/extensions.rs:958-974 (mcp_model) gets this right today — when the name is unsafe it overrides both the label and the command together to Diagnose / /mcp validate. push_server should mirror that instead of computing verb from the original recovery.

No regression test exists in crates/tui/src/mcp/tests.rs for "unsafe server name + Enable/Reauth recovery."

Fix this →

2. Reuse — safe-name fallback still duplicated across two call sites

Unchanged: the "is this name command-safe, and what's the fallback if not" logic lives separately in mcp_routing.rs::push_server and extensions.rs::mcp_model, and finding #1 shows the copies have drifted. A shared helper (e.g. McpRecoveryKind::command_and_label(name) -> (String, MessageId) on the enum in crates/tui/src/mcp.rs) would let both call sites consume the same fallback and prevent re-drift.

3. Provider/model/route safety (EPIC #2608) — clean

Hunyuan alias additions (crates/agent/src/lib.rs, crates/config/src/lib.rs, crates/tui/src/config.rs) add hy3, hunyuan, tencent-hunyuan, hunyuan-hy3 as exact literal match arms resolving to the single already-provider-scoped id tencent/hy3-preview (OpenRouter) — literal-alias resolution, not prefix/freeform-text inference. No invariant violation; hy4 correctly not invented.

4. error_text_looks_auth_required broadening — clean

New substrings ("re-authorize", "/mcp login", "mcp login") in crates/tui/src/mcp/oauth.rs are only reachable from display/hint call sites and the OAuth manager's own upstream refresh-error text, not from TUI-generated strings that could re-trigger themselves — no feedback-loop risk.

5. Tests

Good targeted coverage of the recovery-kind truth table, tab-click hitboxes, MCP action rows, Hunyuan aliases, and all 15 locale files carrying the new keys. Gap: no regression test for the unsafe-name + Enable/Reauth mismatch in finding #1.

6. Security — clean

Server-name→slash-command interpolation is gated by mcp_name_is_command_safe before splicing into /mcp login <name> / /mcp enable <name>; the fallback path (finding #1) is a copy/label bug, not an injection path. No secret-handling changes.

---

CI status (as of this review): cargo check (aarch64-unknown-linux-ohos), DCO, Web Frontend, and PR closes an issue are green for d5b8e97; CI, Lint, and the required OS test matrix are still queued/in progress. Only finding #1 is a real blocker on merge quality; nothing else here should hold up CI.
· branch grok/v0912-tui-settings-mcp-20260827