Truthful 401 notice: distinguish a server-rejected valid credential from a missing login - #516
Merged
Merged
Conversation
The MCP servers answered every post-recovery 401 with "Not logged in.
Run 'kcap login' on the host shell." — factually wrong in the incident
that motivated this (AI-1843): the tenant server's ephemeral signing key
rotated across pod replacements and 401'd tokens that `kcap status`
concurrently (and correctly) reported as valid. Users read the two
surfaces as contradicting each other, concluded `kcap login` was
ineffective, and reached for a daemon restart — which never helps,
because every kcap process re-reads the same token store on 401 (the
issue's suspected "caches auth at process start" bug does not exist; the
recovery path was working as designed on every surface).
The new AuthRejectionNotice classifies the raw store snapshot (never the
refresh-aware accessor — building an error string must not rotate a
credential) and renders one of four truthful lines:
- Missing -> the legacy wording, byte-identical;
- Expired -> expired + refresh failed, run kcap login;
- WrongServer -> names both servers, offers kcap login / kcap use;
- LooksValid -> the server rejected a locally-valid credential: name
the signed-in user and expiry, point at a server-side
auth change, prescribe kcap login, and say explicitly
that restarting the daemon will not help.
All six MCP servers' 401 sites route through it (analytics keeps its
pure MapResponse; the 401 is intercepted at the async call site), and
the per-server NotLoggedInMessage consts now alias the single source.
Any store fault while building the line degrades to the legacy message.
Part of AI-1843 (the root cause — persisting the signing key — is
kcap-server #1392).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
PR Summary by QodoTruthful MCP 401 notices via token-store-aware AuthRejectionNotice
AI Description
Diagram
High-Level Assessment
Files changed (8)
|
Code Review by Qodo
1. Capability 401 misdiagnosed
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+23
to
+26
| /// <para>The MCP servers used to answer every such 401 with a flat | ||
| /// "Not logged in. Run 'kcap login' on the host shell." — factually wrong in the incident | ||
| /// that motivated this: a server-side signing-key rotation 401'd a token that | ||
| /// <c>kcap status</c> concurrently (and correctly) reported as valid, so the user read the |
There was a problem hiding this comment.
1. Verbose authrejectionnotice xml comment 📘 Rule violation ⚙ Maintainability
Verbose incident narratives were added to XML documentation in both the production code and the related unit test, going beyond what’s needed to convey non-obvious intent or constraints. This reduces readability and conflicts with the guideline to keep comments minimal and high-signal.
Agent Prompt
## Issue description
Verbose XML documentation in `AuthRejectionNotice` and `AuthRejectionNoticeTests` narrates historical incident/background context rather than briefly stating non-obvious intent, constraints, and what the tests cover; comments should be reduced to minimal, high-signal documentation.
## Issue Context
PR Compliance ID 5 prefers brief, focused comments that add value beyond what the code already expresses, avoiding lengthy narratives and historical storytelling; replace the incident-style prose with concise statements of intent/constraints for production code and concise descriptions of test assertions/coverage for the unit test.
## Fix Focus Areas
- src/Capacitor.Cli.Core/Auth/AuthRejectionNotice.cs[19-31]
- test/Capacitor.Cli.Tests.Unit/AuthRejectionNoticeTests.cs[5-14]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
|
||
| if (response.StatusCode == HttpStatusCode.Unauthorized) | ||
| return ("Not logged in. Run 'kcap login' on the host shell.", true); | ||
| return (await AuthRejectionNotice.ForPersistentUnauthorizedAsync(apiRoot), true); |
There was a problem hiding this comment.
2. Capability 401 misdiagnosed 🐞 Bug ≡ Correctness
In McpFlowResultServer borrowed-reviewer (daemon capability) mode, a 401 now calls AuthRejectionNotice.ForPersistentUnauthorizedAsync(apiRoot), which reads the local token store and can render “WrongServer/LooksValid” diagnoses even though this process intentionally does not authenticate and may not even be talking to the server. This can mislead users and add unnecessary IO on the only result-delivery channel.
Agent Prompt
### Issue description
`McpFlowResultServer` supports a borrowed-reviewer mode that delivers through a daemon capability URL and **explicitly does not use the token store**. The PR changed both 401 handlers to call `AuthRejectionNotice.ForPersistentUnauthorizedAsync(apiRoot)`, which reads the token store and classifies credentials against `apiRoot`, producing misleading “server rejected credentials / wrong server” messaging for capability-based requests.
### Issue Context
- Borrowed mode is chosen when `KCAP_FLOW_CAPABILITY_URL` is set; in this mode the client is a plain `HttpClient` and must not depend on token-store access.
- On a 401 in borrowed mode, the message should not attempt token-store classification.
### Fix Focus Areas
- src/Capacitor.Cli/Commands/McpFlowResultServer.cs[248-250]
- src/Capacitor.Cli/Commands/McpFlowResultServer.cs[324-326]
### Suggested change
- In both 401 branches, gate the call:
- If `submitUrlOverride`/`messageUrlOverride` is non-null (borrowed mode), return `AuthRejectionNotice.NotLoggedIn` or a capability-specific 401 line (no token-store read).
- Otherwise (normal mode), keep `await AuthRejectionNotice.ForPersistentUnauthorizedAsync(apiRoot)`.
- Keep the tool result marked as error (`true`) as today.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+85
to
+87
| } catch { | ||
| return NotLoggedIn; | ||
| } |
There was a problem hiding this comment.
3. Token-store io masked 🐞 Bug ☼ Reliability
AuthRejectionNotice.ForPersistentUnauthorizedAsync catches all exceptions and returns the legacy NotLoggedIn text, masking real IO/permission/cancellation failures during profile/token reads as “not logged in”. This contradicts TokenStore’s documented rule that real IO/permission faults must not be masked as unauthenticated, and it removes critical debugging signal.
Agent Prompt
### Issue description
`AuthRejectionNotice.ForPersistentUnauthorizedAsync` currently uses a bare `catch` and returns `NotLoggedIn` for *any* failure while resolving the profile or reading tokens. This hides real token-store/config IO and permission issues (and may also swallow cancellation), producing a misleading authentication diagnosis.
### Issue Context
`TokenStore` explicitly distinguishes missing/corrupt files from true IO/permission faults and documents that the latter must propagate and must not be treated as unauthenticated.
### Fix Focus Areas
- src/Capacitor.Cli.Core/Auth/AuthRejectionNotice.cs[79-87]
- src/Capacitor.Cli.Core/Auth/TokenStore.cs[86-93]
### Suggested change
- Replace `catch { return NotLoggedIn; }` with targeted handling:
- Re-throw `OperationCanceledException` so cancellations behave normally.
- For `IOException` / `UnauthorizedAccessException` (and possibly config-load exceptions), return a **generic, non-path-leaking** message like: "Could not read kcap token store (check file permissions). Run 'kcap login' after fixing access." or similar.
- Optionally keep `NotLoggedIn` only for truly expected, non-actionable cases (though most expected cases already return `null` from TokenStore without throwing).
- Avoid including exception messages in the tool output to prevent leaking local paths.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
alexeyzimarev
added a commit
that referenced
this pull request
Aug 10, 2026
#516 landed AuthRejectionNotice on main while this branch was in review, carrying a StoredCredentialState vocabulary whose LooksValid case is exactly this branch's "the server rejected a locally-valid credential". Two near-identically-named auth-notice types is the drift the original type existed to prevent, so there is now one. The surfaces keep different renderings of the shared states, because their length budgets genuinely differ: Render() stays the MCP form (several sentences in a tool result), RecordingNotice() is the one-line form a Claude systemMessage and a vendor stderr line can carry. FromAuthStatus() maps the AuthStatus the hook already holds onto the states, so the per-turn hook path pays none of the disk reads ForPersistentUnauthorizedAsync makes to classify. No wording changes: every rendered string is byte-identical, including WrongServer still rendering as the not-authenticated line in the short form. Naming both servers there would be more truthful and is now a one-line follow-up, but it is a behaviour change and not this PR's. AuthLapseNoticeTests folded into AuthRejectionNoticeTests, plus coverage for the AuthStatus mapping and the WrongServer short-form choice. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
alexeyzimarev
added a commit
that referenced
this pull request
Aug 10, 2026
…#513) * Design: actionable re-login nudge on a hook HTTP 401 A credential the local token store believes is usable but the server rejects surfaces today as an opaque Claude hook-error banner (bare "HTTP 401", exit 1) or, on session-start/session-end, as complete silence. Neither says recording has stopped, and neither says `kcap login`. Spec rides the implementation PR per the repo spec convention. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Plan: hook HTTP 401 re-login nudge Five tasks: collect the notice wording into a Core type (verbatim move of the two existing strings), nudge from the shared stop path, nudge from the session-start arm that drops in silence today, make the vendor stderr line actionable, then README plus full-suite and AOT verification. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Collect the auth-lapse notice wording into one Core type * Nudge the user to re-login when a stop hook is rejected with 401 * Say why session-start was dropped when the server rejects the credential * Name the fix on the vendor hooks' 401 stderr line * Document the in-agent notice for a server-rejected credential * Fix wave from the final whole-branch review of the hook-401 login nudge Tightens the AI-1835 branch before PR: replaces a NotInParallel key that protected nothing with a bare serialization, corrects the README and an AgentHookPoster doc comment now made false by the change, folds the duplicated 401-vs-other stderr ternary into AuthLapseNotice.VendorStderrLine so the two call sites can't diverge, and adds coverage for the pre-flight auth-lapse arm of ClaudeHookCommand.HandleCore that had none. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Emit the re-login nudge from Cursor's own POST path too Cursor was the one vendor the nudge missed. It does not route its recording POST through AgentHookPoster — it POSTs directly and uses the poster only for the IsAuthLapsed predicate — so a 401 returned false/DrainOutcome.Drop in silence, leaving Cursor users with exactly the unexplained failure this change exists to remove. TryPostHookAsync (the live path) now writes the same stderr line. The spool-drain lambda stays silent on purpose: it replays many entries per pass and would repeat the line for each one. Found by the Codex PR reviewer. The design doc had asserted Cursor shared the poster; corrected there with a note rather than a silent rewrite. Also trims two comment blocks flagged as over-narrated, keeping the load-bearing rationale (why exit 0, why stop-only, why this is the arm's only stdout write). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Fold AuthLapseNotice into AuthRejectionNotice #516 landed AuthRejectionNotice on main while this branch was in review, carrying a StoredCredentialState vocabulary whose LooksValid case is exactly this branch's "the server rejected a locally-valid credential". Two near-identically-named auth-notice types is the drift the original type existed to prevent, so there is now one. The surfaces keep different renderings of the shared states, because their length budgets genuinely differ: Render() stays the MCP form (several sentences in a tool result), RecordingNotice() is the one-line form a Claude systemMessage and a vendor stderr line can carry. FromAuthStatus() maps the AuthStatus the hook already holds onto the states, so the per-turn hook path pays none of the disk reads ForPersistentUnauthorizedAsync makes to classify. No wording changes: every rendered string is byte-identical, including WrongServer still rendering as the not-authenticated line in the short form. Naming both servers there would be more truthful and is now a one-line follow-up, but it is a behaviour change and not this PR's. AuthLapseNoticeTests folded into AuthRejectionNoticeTests, plus coverage for the AuthStatus mapping and the WrongServer short-form choice. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
The six MCP servers' 401 sites now render a store-aware, truthful message instead of the flat
Not logged in. Run 'kcap login' on the host shell.A newAuthRejectionNotice(Core) classifies the raw token-store snapshot against the request's target server and renders one of four lines:kcap loginkcap login/kcap usekcap login… restarting the daemon will not help"Why — AI-1843
During the 2026-08-10 kurrent-tenant incident, the server's ephemeral signing key rotated across pod replacements and 401'd tokens that
kcap statusconcurrently (and correctly) reported as valid. The flows MCP kept printing "Not logged in", the user read the two surfaces as contradicting each other, concludedkcap loginwas ineffective, and reached for a daemon restart — which never helps, because every kcap process re-reads the token store on 401.The issue's suspected root cause ("the MCP child caches auth at process start and never re-reads") does not exist —
SendWithRefreshRetryAsync, the daemon'sAccessTokenProvider, and the watchers all re-read~/.config/kcap/tokens/per 401/negotiate, and the incident logs show each surface healing within seconds of eachkcap login. What was genuinely wrong client-side is the message: it told a logged-in user they weren't, and its literal reading sent them away from the one remedy that works. The root cause (the ephemeral signing key) is fixed server-side in kurrent-io/kcap-server#1392.Notes
Classify/Render) with the IO wrapper doing a raw, non-mutating store read — building an error string must never rotate a WorkOS refresh token.MapResponse(unit-tested); the 401 is intercepted at the async call site.NotLoggedInMessageconsts now aliasAuthRejectionNotice.NotLoggedIn(single source).AuthRejectionNoticeTests(new, 7);ReviewerVendorFallbackTests44/44,McpAnalyticsServerTests15/15,McpFlowsServerTests52/52,McpSessionsServerTests(integration) 10/10 unchanged;scripts/check-linear-ids.shclean.Part of AI-1843.
🤖 Generated with Claude Code