tui: the MCP boot chip counts expired logins separately from failures - #5938
Conversation
…#5926) The founder's footer read 'MCP · 12 connected · 8 failed' when seven of the eight only needed 'codewhale mcp login <server>'. The boot surface already classifies those servers as NeedsLogin; the chip now says 'MCP · 12 connected · 7 ◆ auth required · 1 failed' (narrow form 'MCP · 7 login · 1 failed') and sits at attention level when nothing actually failed. The /mcp ordering and the refresh-parse detail stay open on #5926. Tests: a needs-login server renders under the auth-required label at attention level without the word failed; mixed states keep both counts in the full and the narrow form. cargo test -p codewhale-tui --lib -- tui::session_boot: 14 passed, 0 failed. Signed-off-by: CodeWhale Bot <hmbown@gmail.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_08f16cc4-b03f-4786-8ee3-779d153c988e) |
|
Claude finished @Hmbown's task in 1m 47s —— View job Review: MCP boot chip separates expired logins from failures
Scope: SummaryThe change is correct and well-scoped.
No provider/model routing code is touched; not applicable to the EPIC #2608 invariant. Correctness check
Minor, non-blocking observations
Verification noteI was not able to execute |
There was a problem hiding this comment.
🟢 Approval recommended
The change is localized to boot-chip rendering logic, matches the stated behavior in #5926 for the “chip half,” and is covered by updated unit tests for both single- and mixed-state cases.
Pull request overview
Updates the TUI session-boot footer “MCP” activity chip to distinguish auth-required (expired/missing login) from true failures, aligning the boot surface with the engine’s existing NeedsLogin classification and the shared ◆ auth required label used elsewhere in the TUI.
Changes:
- Count
McpServerBootState::NeedsLoginseparately fromFailed, and render it undermcp_auth_required_state_label()in the chip text. - Map chip severity to Attention when only logins are required, and Failure when any server actually failed.
- Expand tests to cover needs-login-only and mixed-state chips (including full vs compact rendering under tight budgets).
File summaries
| File | Description |
|---|---|
| crates/tui/src/tui/session_boot.rs | Separates NeedsLogin vs Failed in the boot footer MCP chip, adjusts severity mapping, and adds tests for the new text/level behavior. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0aad71a9e6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| full.push_str(&format!("{ITEM_SEPARATOR}{need_login} {auth_label}")); | ||
| compact.push_str(&format!("{ITEM_SEPARATOR}{need_login} login")); |
There was a problem hiding this comment.
When app.ui_locale is non-English, this new branch emits English in both candidates: mcp_auth_required_state_label() returns ◆ auth required, while the compact form hardcodes login; only the surrounding connected/failed labels are translated. This produces mixed-language wide and narrow footers even though localized LaunchMcpNeedsSignInOne/LaunchMcpNeedsSignInMany messages already exist. Compose the authentication prose through tr(locale, MessageId::...), leaving only the glyph in code.
AGENTS.md reference: crates/tui/AGENTS.md:L25-L26
Useful? React with 👍 / 👎.
| let mut compact = String::from("MCP"); | ||
| if need_login > 0 { | ||
| full.push_str(&format!("{ITEM_SEPARATOR}{need_login} {auth_label}")); | ||
| compact.push_str(&format!("{ITEM_SEPARATOR}{need_login} login")); |
There was a problem hiding this comment.
When a needs-auth server uses a manual bearer/environment header or is plugin-contributed, the compact candidate incorrectly labels it login. These servers still set McpServerSnapshot::auth_required after a 401, but auth_required_recovery_hint() explicitly routes them to correcting the credential and /mcp reload, while OAuth login is disabled. At narrow widths this therefore advertises the wrong recovery class; use a neutral compact label such as auth/auth required, or derive the wording from the server's supported recovery.
Useful? React with 👍 / 👎.
| } else { | ||
| SessionBootActivityLevel::Attention | ||
| }; | ||
| return activity_notice_from_candidates(level, vec![full, compact], budget); |
There was a problem hiding this comment.
At the supported 40-column layout, notice_budget is 20, but with both counts nonzero the shortest candidate, MCP · 1 login · 1 failed, is 24 columns, so activity_notice_from_candidates() returns None and the footer hides the actual failure. Before this change the same state fell back to MCP · 2 failed, which fit in 14 columns. Add another fallback that sheds the auth clause and retains the higher-severity failed count instead of dropping the entire notice.
AGENTS.md reference: crates/tui/AGENTS.md:L22-L22
Useful? React with 👍 / 👎.
![codewhale-agent[bot]](https://avatars.githubusercontent.com/u/101357273?s=60&v=4){kind=small}
There was a problem hiding this comment.
Codewhale review
The PR splits NeedsLogin MCP servers from Failed in the boot footer chip and maps login-only cases to Attention severity. The logic is straightforward and the existing tests are updated to cover the full and compact mixed-state paths.
Findings
- [WARNING] Compact auth-required label is not localized (
crates/tui/src/tui/session_boot.rs:256)
The new compact fallback hard-codes the string "login" whenneed_login > 0. The full candidate localizes connected/failed viatr(locale, ...), so non-English TUI users can see an English word in the narrow footer. If an existing MessageId for "login" exists, use it; otherwise a new localized message is needed for the compact form. - [INFO] Compact login-only path is not directly tested (
crates/tui/src/tui/session_boot.rs:256)
The updated single-state test uses a 100-column budget and asserts the full candidate. The new compact candidate forneed_login > 0 && failed == 0(MCP · N login) is not exercised by a test; the mixed-state test does cover the compact failed+login combination. A narrow-budget assertion for the login-only case would lock the intended fallback.
Assessment
The change is low-risk and functionally correct. The main gap is localization of the compact login label; adding a narrow login-only test would further strengthen coverage.
Advisory review by Codewhale (codewhale review --pr 5938 --post, head 0aad71a9e663e6239164723e11cf4506efe7f2ba). Line-specific findings are also posted as inline review comments; mechanical fixes arrive as committable suggestions you can apply from the Files tab. CODEOWNERS approval still governs merge.
| let mut compact = String::from("MCP"); | ||
| if need_login > 0 { | ||
| full.push_str(&format!("{ITEM_SEPARATOR}{need_login} {auth_label}")); | ||
| compact.push_str(&format!("{ITEM_SEPARATOR}{need_login} login")); |
There was a problem hiding this comment.
[WARNING] Compact auth-required label is not localized
The new compact fallback hard-codes the string "login" when need_login > 0. The full candidate localizes connected/failed via tr(locale, ...), so non-English TUI users can see an English word in the narrow footer. If an existing MessageId for "login" exists, use it; otherwise a new localized message is needed for the compact form.
| let mut compact = String::from("MCP"); | ||
| if need_login > 0 { | ||
| full.push_str(&format!("{ITEM_SEPARATOR}{need_login} {auth_label}")); | ||
| compact.push_str(&format!("{ITEM_SEPARATOR}{need_login} login")); |
There was a problem hiding this comment.
[INFO] Compact login-only path is not directly tested
The updated single-state test uses a 100-column budget and asserts the full candidate. The new compact candidate for need_login > 0 && failed == 0 (MCP · N login) is not exercised by a test; the mixed-state test does cover the compact failed+login combination. A narrow-budget assertion for the login-only case would lock the intended fallback.
…en on this head) Signed-off-by: CodeWhale Bot <hmbown@gmail.com>
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_02cc73d7-3de5-4aed-b91a-301c954a5276) |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| if need_login > 0 { | ||
| full.push_str(&format!("{ITEM_SEPARATOR}{need_login} {auth_label}")); | ||
| compact.push_str(&format!("{ITEM_SEPARATOR}{need_login} login")); |
There was a problem hiding this comment.
🔍 Login status bypasses localization
The new login labels remain English while adjacent status labels use tr. Non-English footers therefore mix languages.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
Claude finished @Hmbown's task in 3m 39s —— View job Review: MCP boot chip separates expired logins from failures
Scope: Correctness
Bug: at narrow footer widths, a real failure can be hidden entirely, not just abbreviated. When The same file already has the right pattern for this exact problem — Provider/model routing (EPIC #2608)Not applicable — no provider/model selection code is touched. ReuseGood rung-2 discipline: Localization gap (new in this PR)
Related wording issue: the compact Tests
SecurityNo secret handling, shell/exec, or input-validation surface touched. |
There was a problem hiding this comment.
Codewhale review
PR cleanly separates NeedsLogin MCP servers from Failed servers in the session boot chip, uses the shared auth-required label, downgrades login-only chips to Attention, and adds tests for the two-state and mixed-state paths. The logic matches the PR description and is low risk.
Findings
- [INFO] Compact chip hardcodes English 'login' (
crates/tui/src/tui/session_boot.rs:256)
The narrow candidate usescompact.push_str(&format!("{ITEM_SEPARATOR}{need_login} login"));, which hardcodes English 'login'. The full candidate uses the sharedmcp_auth_required_state_label(), andconnected/failedare translated in the full form. If the TUI is localized, the compact footer will show an English word; consider using an existing localized message or abbreviation if one is available. - [INFO] Test comment says 'compact' but exercises full-width candidate (
crates/tui/src/tui/session_boot.rs:664)
The updated test comment says 'The compact activity chip names a needs-login server...' but the assertion callsactivity_notice(Locale::En, 100)and verifies the full candidate including the connected count. The wording is misleading. - [INFO] Compact needs-login-only branch lacks explicit coverage (
crates/tui/src/tui/session_boot.rs:679)
The mixed-state test covers the compact branch when both login and failure counts are present, and the first needs-login test covers the full-width branch. The compact path forneed_login > 0 && failed == 0(MCP · N login) is not explicitly asserted, despite the PR description implying both widths are covered for the single-state case.
Suggestions
-
crates/tui/src/tui/session_boot.rs:664— The test callsactivity_noticewith width 100 and asserts the full candidate, so the comment should say full-width rather than compact.// The full-width activity chip names a needs-login server under the
Assessment
No blocking correctness issues found in the diff. The separation of NeedsLogin from Failed is implemented correctly and the severity change is appropriate. Minor follow-ups are test clarity and a possible compact-localization inconsistency.
Advisory review by Codewhale (codewhale review --pr 5938 --post, head 236cda03fb2484d5129314887dcfedb9b3f5b8b9). Line-specific findings are also posted as inline review comments; mechanical fixes arrive as committable suggestions you can apply from the Files tab. CODEOWNERS approval still governs merge.
| let mut compact = String::from("MCP"); | ||
| if need_login > 0 { | ||
| full.push_str(&format!("{ITEM_SEPARATOR}{need_login} {auth_label}")); | ||
| compact.push_str(&format!("{ITEM_SEPARATOR}{need_login} login")); |
There was a problem hiding this comment.
[INFO] Compact chip hardcodes English 'login'
The narrow candidate uses compact.push_str(&format!("{ITEM_SEPARATOR}{need_login} login"));, which hardcodes English 'login'. The full candidate uses the shared mcp_auth_required_state_label(), and connected/failed are translated in the full form. If the TUI is localized, the compact footer will show an English word; consider using an existing localized message or abbreviation if one is available.
| // The compact activity chip counts a needs-login server with the | ||
| // failure band (the receipt-line renderer this pinned moved to the | ||
| // chip in the Tideline boot-surface refactor). | ||
| // The compact activity chip names a needs-login server under the |
There was a problem hiding this comment.
[INFO] Test comment says 'compact' but exercises full-width candidate
The updated test comment says 'The compact activity chip names a needs-login server...' but the assertion calls activity_notice(Locale::En, 100) and verifies the full candidate including the connected count. The wording is misleading.
| mcp_auth_required_state_label() | ||
| ) | ||
| ); | ||
| assert!(!chip.text.contains("failed"), "{}", chip.text); |
There was a problem hiding this comment.
[INFO] Compact needs-login-only branch lacks explicit coverage
The mixed-state test covers the compact branch when both login and failure counts are present, and the first needs-login test covers the full-width branch. The compact path for need_login > 0 && failed == 0 (MCP · N login) is not explicitly asserted, despite the PR description implying both widths are covered for the single-state case.
| // The compact activity chip counts a needs-login server with the | ||
| // failure band (the receipt-line renderer this pinned moved to the | ||
| // chip in the Tideline boot-surface refactor). | ||
| // The compact activity chip names a needs-login server under the |
There was a problem hiding this comment.
The test calls activity_notice with width 100 and asserts the full candidate, so the comment should say full-width rather than compact.
| // The compact activity chip names a needs-login server under the | |
| // The full-width activity chip names a needs-login server under the |
No-Issue: chip half of #5926; the issue stays open for the /mcp ordering and the refresh-parse detail.
Refs #5926 (chip half; the /mcp ordering with a one-key login and the refresh-parse detail stay open)
Receipt: the footer read
MCP · 12 connected · 8 failedwhilecodewhale mcp connectshowed seven of those servers answering 401 with the engine already printing the remedy. The boot surface classifies them asNeedsLogin; the chip lumped them into failed.Now:
MCP · 12 connected · 7 ◆ auth required · 1 failed(narrow:MCP · 7 login · 1 failed), at attention level when nothing actually failed and failure level otherwise. The label is the sharedmcp_auth_required_state_label()every other MCP surface prints.Verified:
RUST_MIN_STACK=16777216 cargo test -p codewhale-tui --lib -- tui::session_boot→ 14 passed, 0 failed (two tests cover the single-state and mixed-state chips in both widths).Note
Low Risk
Footer copy and severity for MCP boot only; no auth, connect, or data-path changes.
Overview
The session boot footer chip no longer treats
NeedsLoginMCP servers as failures. Expired or missing logins are counted with the sharedmcp_auth_required_state_label()(e.g.MCP · 12 connected · 7 ◆ auth required · 1 failed), while only trueFailedstates use the failed label.When only auth is missing, the chip uses
SessionBootActivityLevel::Attentioninstead of failure; mixed auth + failure still uses failure level. A compact line (MCP · N login · M failed) keeps both counts on narrow footer budgets. Tests cover needs-login-only and mixed login/failure chips at full and tight widths (#5926).Reviewed by Cursor Bugbot for commit 236cda0. Bugbot is set up for automated code reviews on this repo. Configure here.