feat(tui): /mcp leads with need-login servers and a failed refresh keeps the endpoint's receipt - #5971
Conversation
…eps the endpoint's receipt (#5926) The founder's receipt: of the eight servers the footer called failed, seven only needed a login. Three surfaces now keep the states apart. /mcp lists the servers that need a login first, as their own Needs login group above Needs attention, and opens with the cursor already on the first such row, so the Enter the screen advertises runs `/mcp login <server>` straight away. New ExtensionsGroupNeedsLogin key in all 15 locale packs. A token refresh that fails to parse the provider's answer keeps the endpoint's receipt - status line, content type, and a 200-byte excerpt with every credential-shaped value (access_token, refresh_token, client_secret, id_token, bearer schemes) masked before the cut - instead of rmcp's bare 'Failed to parse server response', so a provider outage answering an HTML 502 reads differently from a parser defect. The receipt rides a RecordingOAuthHttpClient that executes what rmcp's stock client would and records the latest token-endpoint answer; only the current refresh's answer may explain that refresh's failure. The remedy wording itself landed earlier as #5959. A footer snapshot test pins 'MCP . 1 connected . 1 auth required . 1 failed' so an expired login never regresses into the failed count (the chip landed with the boot-surface half of #5926). Gates on this head (rebased onto main f974685): cargo fmt --all clean; cargo clippy --workspace --all-targets --all-features --locked -D warnings clean; full suite cargo test -p codewhale-tui --lib --locked 11877 passed / 0 failed / 13 ignored (targeted module filters 71 passed, the eight new tests verified by name, localization 50 passed). Earlier runs under the shared CI runner showed isolated load flakes (deepseek translate, tmux clipboard, fleet concurrent manager loops) that each pass in isolation and in the final clean run; one zai compatibility-stream failure at the previous base was root-caused to the tools-registry probe defect fixed on main by #5944's follow-up. Signed-off-by: CodeWhale Bot <bot@codewhale.net>
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_e07b4bdc-81fc-4307-a6a0-0cb4a6a87bd9) |
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. |
|
Claude finished @Hmbown's task in 3m 0s —— View job Review:
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| let receipt = self.inner.http_client.take_token_endpoint_receipt(); | ||
| Err(err) | ||
| .with_context(|| refresh_failure_context(&server_name, names_remedy, receipt.as_ref())) |
There was a problem hiding this comment.
🟡 Retry failures show stale receipts
When a refresh retries after adopting rotated credentials, take_token_endpoint_receipt can return the first attempt's response. The retry does not clear that response before a transport failure produces no replacement. Diagnostics then attribute an unrelated endpoint response to the failed retry.
Prompt for agents
The receipt slot is cleared before the initial refresh, but not before the second refresh at McpOAuthRuntime::refresh_and_persist. If the first request returns invalid_grant, rotated on-disk credentials are adopted, and the retry fails before receiving a response, lines 731-733 consume the first request's receipt. Clear or otherwise correlate the receipt immediately before each try_refresh_and_persist attempt so only the attempt producing the returned error can contribute a receipt. Add a regression test covering an invalid_grant response followed by a transport failure on the rotated-token retry.
Was this helpful? React with 👍 or 👎 to provide feedback.
| tool, a sub-agent or the operator; the 0.9.12 shell had dropped the overall | ||
| working-time indicator from the place a glancing user checks (#5914). | ||
|
|
||
| - An MCP token refresh that fails to parse the provider's answer keeps the | ||
| endpoint's receipt — status line, content type, and a 200-byte excerpt | ||
| with every credential-shaped value (`access_token`, `refresh_token`, | ||
| `client_secret`, `id_token`, bearer schemes) masked before the cut — | ||
| instead of rmcp's bare `Failed to parse server response`, so a provider | ||
| outage answering an HTML 502 reads differently from a parser defect, and | ||
| the login remedy stays named (#5926; remedy wording landed in #5959). |
There was a problem hiding this comment.
🔍 Remove branch changelog edits
Contribution rules reserve both changelogs for batched updates on main. This PR edits both copies, increasing conflict risk during landing.
(Refers to this code)
Was this helpful? React with 👍 or 👎 to provide feedback.
| fn token_response_excerpt(body: &[u8]) -> String { | ||
| let masked = mask_oauth_secrets(&String::from_utf8_lossy(body)); | ||
| let collapsed = masked.split_whitespace().collect::<Vec<_>>().join(" "); | ||
| if collapsed.len() <= TOKEN_RECEIPT_EXCERPT_BYTES { | ||
| return collapsed; | ||
| } | ||
| let mut end = TOKEN_RECEIPT_EXCERPT_BYTES; | ||
| while !collapsed.is_char_boundary(end) { | ||
| end -= 1; | ||
| } | ||
| format!("{}…", &collapsed[..end]) |
There was a problem hiding this comment.
🟥 OAuth diagnostics can expose credentials
A token endpoint can echo submitted credentials under unrecognized fields or prose. mask_oauth_secrets leaves those values in the new diagnostic excerpt, exposing them through errors and logs.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
🟡 Changes recommended
There are confirmed issues affecting correctness (MCP initial selection when a problem banner is present) and security (newline-separated secrets may bypass masking in recorded receipts).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR improves MCP operational UX in the TUI by (1) re-ordering /mcp so “needs login” servers are surfaced first with a one-key remediation flow, and (2) enhancing OAuth refresh failures by retaining a masked, byte-capped receipt of the token endpoint response to distinguish provider outages from parsing/client defects.
Changes:
- Split
/mcprows intoNeeds login→Needs attention→Servers, and initialize the cursor to the first login row for immediate/mcp login <server>. - Add
RecordingOAuthHttpClient+TokenEndpointReceiptso refresh parse failures include HTTP status/content-type and a masked excerpt of the response body. - Wire a new i18n key (
ExtensionsGroupNeedsLogin) across locale packs and pin footer chip wording via snapshot test.
File summaries
| File | Description |
|---|---|
| crates/tui/src/tui/views/extensions.rs | Groups /mcp servers into login/attention/healthy buckets and adjusts initial selection to favor login remediation. |
| crates/tui/src/tui/ui/tests.rs | Adds a snapshot-style assertion that footer summary separates connected / auth-required / failed MCP states. |
| crates/tui/src/mcp/oauth.rs | Introduces token-endpoint receipt recording and masking logic to enrich refresh parse failure context safely. |
| crates/tui/src/localization.rs | Adds ExtensionsGroupNeedsLogin MessageId and updates extensions-locale parity test count. |
| crates/tui/locales/en.json | Adds ExtensionsGroupNeedsLogin translation. |
| crates/tui/locales/de.json | Adds ExtensionsGroupNeedsLogin translation. |
| crates/tui/locales/es-419.json | Adds ExtensionsGroupNeedsLogin translation. |
| crates/tui/locales/fr.json | Adds ExtensionsGroupNeedsLogin translation. |
| crates/tui/locales/hi.json | Adds ExtensionsGroupNeedsLogin translation. |
| crates/tui/locales/id.json | Adds ExtensionsGroupNeedsLogin translation. |
| crates/tui/locales/ja.json | Adds ExtensionsGroupNeedsLogin translation. |
| crates/tui/locales/ko.json | Adds ExtensionsGroupNeedsLogin translation. |
| crates/tui/locales/pt-BR.json | Adds ExtensionsGroupNeedsLogin translation. |
| crates/tui/locales/ru.json | Adds ExtensionsGroupNeedsLogin translation. |
| crates/tui/locales/uk.json | Adds ExtensionsGroupNeedsLogin translation. |
| crates/tui/locales/vi.json | Adds ExtensionsGroupNeedsLogin translation. |
| crates/tui/locales/zh-Hans.json | Adds ExtensionsGroupNeedsLogin translation. |
| crates/tui/locales/zh-Hant.json | Adds ExtensionsGroupNeedsLogin translation. |
| crates/tui/locales/ca.json | Adds ExtensionsGroupNeedsLogin translation. |
| crates/tui/CHANGELOG.md | Documents the /mcp grouping/cursor behavior and token refresh receipt behavior. |
| CHANGELOG.md | Mirrors the changelog entries at the workspace root. |
Review details
Suppressed comments (1)
crates/tui/src/mcp/oauth.rs:248
- This inline comment is also garbled (the example after
Authorization:is missing/obscured), so it’s hard to understand why the code treats scheme+token as a single secret span.
// An unquoted `Authorization: Bearer <token>` carries its scheme in
// front of the credential; the whole value is the secret.
- Files reviewed: 21/21 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| let skip_spaces = |mut cursor: usize| { | ||
| while bytes | ||
| .get(cursor) | ||
| .is_some_and(|byte| *byte == b' ' || *byte == b'\t') | ||
| { | ||
| cursor += 1; | ||
| } | ||
| cursor | ||
| }; |
| .first() | ||
| .is_some_and(|group| group.id == MCP_LOGIN_GROUP_ID) | ||
| { | ||
| view.selected[tab.index()] = 1; |
| /// Replace every credential-shaped value in `text` with `***`: JSON | ||
| /// members (`"access_token": "…"`), form/query pairs (`refresh_token=…`), | ||
| /// and bearer schemes (`Bearer …`). Field names, separators and everything | ||
| /// else survive so the shape of the answer stays readable. |
![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: 31d912ff8c
ℹ️ 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".
| let skip_spaces = |mut cursor: usize| { | ||
| while bytes | ||
| .get(cursor) | ||
| .is_some_and(|byte| *byte == b' ' || *byte == b'\t') | ||
| { |
There was a problem hiding this comment.
When an unparseable token response places a newline around the separator, such as {"access_token":\n"secret"} trailing, skip_spaces stops before the newline, so the function masks an empty span and leaves "secret" intact. token_response_excerpt then includes that credential in the user-visible refresh error. Accept all JSON whitespace before retaining the receipt so malformed or pretty-printed responses cannot leak OAuth tokens.
AGENTS.md reference: AGENTS.md:L28-L29
Useful? React with 👍 / 👎.
| fn mcp_item_needs_login(item: &ExtensionItem) -> bool { | ||
| item.action | ||
| .as_ref() | ||
| .and_then(ExtensionAction::command) | ||
| .is_some_and(|command| command.starts_with("/mcp login ")) |
There was a problem hiding this comment.
When the centralized login command formatting changes—for example to add quoting, spacing, or an alias—this string-prefix test silently stops recognizing McpRecoveryKind::Reauth; those servers then move back under “Needs attention” and the initial cursor no longer selects the login row. Preserve the typed recovery classification while constructing the row instead of reconstructing lifecycle state from its rendered command.
AGENTS.md reference: crates/tui/AGENTS.md:L16-L17
Useful? React with 👍 / 👎.
| pub(crate) struct RecordingOAuthHttpClient { | ||
| client: reqwest::Client, | ||
| last_token_response: std::sync::Mutex<Option<TokenEndpointReceipt>>, |
There was a problem hiding this comment.
When cloned runtimes refresh concurrently, they clear and consume this shared Option outside the manager lock, so one call can erase or report the other call's response. The same stale association occurs when an invalid_grant attempt records a receipt, adopts rotated credentials, and the retry fails before receiving a response: the final error reports the first attempt's receipt as the retry's answer. Associate the receipt with the individual request or serialize the clear/refresh/take sequence.
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 improves /mcp ordering and OAuth refresh receipts with locale additions, new tests, and a recording OAuth HTTP client. The main change is generally sound, but the credential-masking scanner has whitespace-handling gaps that can leak tokens for pretty-printed JSON or tab-separated bearer values.
Findings
- [ERROR] Credential masking can miss secrets when token-endpoint bodies use newlines or tabs (
crates/tui/src/mcp/oauth.rs:198)
secret_value_span's skip_spaces only skips ASCII space and tab. JSON permits \r and \n as whitespace between the key, colon, and value, so pretty-printed responses such as {"access_token":\n"secret"} are not masked and the secret ends up in the receipt. Similarly, the Authorization: Bearer continuation check only accepts a literal space, so Authorization: Bearer\t leaves the token unmasked. The receipt can be emitted in error text, so this defeats the intended credential-masking guarantee. Current tests only exercise space-separated forms.
Suggestions
-
crates/tui/src/mcp/oauth.rs:198— Treat all ASCII whitespace as skippable between a secret field and its separator/value so multiline JSON still masks credentials..is_some_and(|byte| byte.is_ascii_whitespace()) -
crates/tui/src/mcp/oauth.rs:251— Accept any ASCII whitespace after Bearer/Basic so tab-separated Authorization values are masked as well.&& bytes.get(value_end).is_some_and(|byte| byte.is_ascii_whitespace())
Assessment
The UX grouping, locale wiring, and failure-receipt plumbing are well covered by tests and should land once the masking whitespace edge case is fixed. The masking leak is high-impact even if uncommon, so it should be addressed before merge.
Advisory review by Codewhale (codewhale review --pr 5971 --post, head 31d912ff8cf17688e56c83d0862155391dd6db1f). 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 bytes = text.as_bytes(); | ||
| let skip_spaces = |mut cursor: usize| { | ||
| while bytes | ||
| .get(cursor) |
There was a problem hiding this comment.
[ERROR] Credential masking can miss secrets when token-endpoint bodies use newlines or tabs
secret_value_span's skip_spaces only skips ASCII space and tab. JSON permits \r and \n as whitespace between the key, colon, and value, so pretty-printed responses such as {"access_token":\n"secret"} are not masked and the secret ends up in the receipt. Similarly, the Authorization: Bearer continuation check only accepts a literal space, so Authorization: Bearer\t leaves the token unmasked. The receipt can be emitted in error text, so this defeats the intended credential-masking guarantee. Current tests only exercise space-separated forms.
| let bytes = text.as_bytes(); | ||
| let skip_spaces = |mut cursor: usize| { | ||
| while bytes | ||
| .get(cursor) |
There was a problem hiding this comment.
Treat all ASCII whitespace as skippable between a secret field and its separator/value so multiline JSON still masks credentials.
| .get(cursor) | |
| .is_some_and(|byte| byte.is_ascii_whitespace()) |
| // front of the credential; the whole value is the secret. | ||
| let value_start = cursor; | ||
| let mut value_end = unquoted_end(cursor); | ||
| if matches!(lower[value_start..value_end].as_ref(), "bearer" | "basic") |
There was a problem hiding this comment.
Accept any ASCII whitespace after Bearer/Basic so tab-separated Authorization values are masked as well.
| if matches!(lower[value_start..value_end].as_ref(), "bearer" | "basic") | |
| && bytes.get(value_end).is_some_and(|byte| byte.is_ascii_whitespace()) |
Closes #5926: the footer/
/mcpordering and raw-response receipt halves; the refresh-remedy wording half landed earlier as #5959, and the boot-chip counting half as the◆ auth requiredchip.Receipt: the founder's
/mcpread8 failedwhen seven of the eight only neededcodewhale mcp login <server>, and a refresh that failed to parse the provider's answer said onlyFailed to parse server response— no evidence, no next step.What changed:
/mcplists the servers that need a login first, as their ownNeeds logingroup aboveNeeds attention, and opens with the cursor already on the first such row, so theEnterthe screen advertises runs/mcp login <server>straight away. Real failures follow with their reason; healthy servers sort last. NewExtensionsGroupNeedsLoginkey in all 15 locale packs.access_token,refresh_token,client_secret,id_token, bearer schemes) masked before the cut: the token endpoint did not answer the way the client expects (it answered HTTP 502 Bad Gateway (text/html; charset=utf-8): 502 Bad Gateway ); if this persists, runcodewhale mcp login supabase(or/mcp login supabase) to re-authorize. A provider outage answering HTML reads differently from a parser defect. The receipt rides aRecordingOAuthHttpClientthat executes what rmcp's stock client would and records the latest token-endpoint answer; a stale answer can never explain a later failure.MCP · 1 connected · 1 ◆ auth required · 1 failedso an expired login never regresses into the failed count (the chip itself landed with the boot-surface half).Verified:
RUST_MIN_STACK=16777216 cargo fmt --allclean;cargo clippy --workspace --all-targets --all-features --locked -- -D warningsclean; targeted filters (mcp::oauth extensions session_boot footer) 71 passed / 0 failed with the eight new tests verified by name;localization50 passed / 0 failed; full suitecargo test -p codewhale-tui --lib --lockedon the pushed head (rebased onto main f974685): 11877 passed / 0 failed / 13 ignored. Earlier full-suite runs under the shared CI runner showed isolated load flakes (deepseek translate, tmux clipboard, fleet concurrent-manager loops) that each pass in isolation and in the final clean run; one zai compatibility-stream failure at the previous base was root-caused to the tools-registry probe defect fixed on main by #5944's follow-up.Note
Medium Risk
Changes MCP OAuth HTTP plumbing and refresh error handling where mishandled receipts or masking could leak secrets or mis-route users; UI grouping is lower risk but affects operator recovery flows.
Overview
MCP extensions UI now splits servers that only need re-auth into a Needs login group above Needs attention, with the list opening on the first login row so Enter runs
/mcp login <server>immediately. Expired OAuth sessions no longer sit in the same bucket as hard failures; a snapshot test locks the footer toconnected · ◆ auth required · failedso auth-required servers do not inflate the failed count.OAuth token refresh errors attach a token-endpoint receipt (HTTP status, content-type, 200-byte body excerpt) when the provider answered but the response could not be parsed. Credentials are masked in excerpts via
RecordingOAuthHttpClient, which wraps rmcp’s HTTP path and records the latest POST to the token URL so stale bodies cannot explain a later failure. Non-auth parse failures still name the login remedy, now with(it answered …)context (e.g. HTML 502 vs JSON).i18n: new
ExtensionsGroupNeedsLoginacross 15 locale packs andMessageIdwiring.Reviewed by Cursor Bugbot for commit 31d912f. Bugbot is set up for automated code reviews on this repo. Configure here.