Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
99 changes: 99 additions & 0 deletions src/Capacitor.Cli.Core/Auth/AuthRejectionNotice.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
namespace Capacitor.Cli.Core.Auth;

/// <summary>How the stored credential relates to a 401 the server just returned.</summary>
public enum StoredCredentialState {
/// <summary>No usable stored token — the caller genuinely is not logged in.</summary>
Missing,

/// <summary>A token is stored but was minted by a different server than the target.</summary>
WrongServer,

/// <summary>A token is stored but expired (and the recovery's refresh evidently failed).</summary>
Expired,

/// <summary>The stored token is unexpired and bound to the target — the server rejected a
/// credential that looks perfectly valid locally.</summary>
LooksValid,
}

/// <summary>
/// Builds the user-facing line for a request the server answered 401 AFTER the one-shot
/// re-read-and-refresh recovery (<c>TokenStore.RecoverForServerAsync</c>) already ran.
///
/// <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
Comment on lines +23 to +26

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remediation recommended

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

/// two surfaces as contradicting each other, concluded <c>kcap login</c> was ineffective,
/// and reached for a daemon restart — which never helps, because every process re-reads the
/// same token store. The message now says what is actually true for each store state; the
/// legacy wording survives byte-identical for the genuinely-logged-out case.</para>
/// </summary>
public static class AuthRejectionNotice {
public const string NotLoggedIn = "Not logged in. Run 'kcap login' on the host shell.";

/// <summary>Pure classification of a raw store snapshot against the request's target server.</summary>
public static StoredCredentialState Classify(StoredTokens? stored, string targetBaseUrl) {
if (stored is null) return StoredCredentialState.Missing;

// An unbound (pre-upgrade) token is treated as bound — same rule as TokenStore's
// BoundToTarget: there is nothing to contradict.
if (stored.ServerUrl is not null && !ServerIdentity.SameServer(stored.ServerUrl, targetBaseUrl)) {
return StoredCredentialState.WrongServer;
}

return stored.IsExpired ? StoredCredentialState.Expired : StoredCredentialState.LooksValid;
}

public static string Render(StoredCredentialState state, StoredTokens? stored, string targetBaseUrl) =>
state switch {
StoredCredentialState.Missing => NotLoggedIn,

StoredCredentialState.Expired =>
"Your kcap login has expired and an automatic refresh did not succeed. " +
"Run 'kcap login' on the host shell.",

StoredCredentialState.WrongServer =>
$"Stored login was issued by {stored?.ServerUrl} but this request targets {targetBaseUrl}. " +
$"Run 'kcap login' (or switch profiles with 'kcap use') to authenticate against {targetBaseUrl}.",

// The stored token is unexpired and bound to this server, was re-read from disk and
// re-sent, and the server still said 401. Locally there is nothing left to fix —
// this is the server's auth state having moved (a restart that rotated the signing
// key, or an auth incident). Say so, and steer AWAY from the daemon-restart
// superstition the incident produced.
_ =>
$"The server rejected kcap's credentials (HTTP 401) even after re-reading the token store — " +
$"yet the stored login for {stored?.GitHubUsername} looks valid locally ({DescribeExpiry(stored)}). " +
"This usually means the server's auth state changed (a restart or auth incident). " +
"Run 'kcap login' on the host shell to mint a fresh credential — restarting the daemon will not help. " +
"If a fresh login still hits this, the server is mid-incident; retry later.",
};

/// <summary>
/// Store-inspecting convenience for the MCP servers' 401 sites. Raw, non-mutating read —
/// deliberately never the refresh-aware accessor, which could rotate a credential just to
/// build an error string. Any store fault degrades to the legacy message rather than
/// replacing an auth diagnosis with an IO stack trace.
/// </summary>
public static async Task<string> ForPersistentUnauthorizedAsync(string targetBaseUrl, CancellationToken ct = default) {
try {
var profile = await TokenStore.ResolveProfileNameAsync(ct);
var stored = await TokenStore.LoadForProfileAsync(profile, ct);

return Render(Classify(stored, targetBaseUrl), stored, targetBaseUrl);
} catch {
return NotLoggedIn;
}
Comment on lines +85 to +87

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remediation recommended

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

}

static string DescribeExpiry(StoredTokens? stored) {
if (stored is null) return "expiry unknown";

var remaining = stored.ExpiresAt - DateTimeOffset.UtcNow;

return remaining.TotalHours >= 1
? $"expires in {remaining.TotalHours:F0}h"
: $"expires in {Math.Max(1, remaining.TotalMinutes):F0}m";
}
}
10 changes: 9 additions & 1 deletion src/Capacitor.Cli/Commands/McpAnalyticsServer.cs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ namespace Capacitor.Cli.Commands;
/// rejection reasons. Structure cloned from McpMemoryServer.
/// </summary>
static class McpAnalyticsServer {
internal const string NotLoggedInMessage = "Not logged in. Run 'kcap login' on the host shell.";
internal const string NotLoggedInMessage = AuthRejectionNotice.NotLoggedIn;

internal const string NotSupportedMessage =
"This server does not support analytics queries — upgrade kcap-server.";
Expand Down Expand Up @@ -182,6 +182,14 @@ internal static async Task<string> HandleToolCallAsync(

var body = await httpResponse.Content.ReadAsStringAsync();

// The 401 wording is store-aware (a locally-valid stored credential the
// server rejects must not read as "not logged in"), which needs an async store
// read — resolved here so MapResponse stays a pure, unit-testable mapper (its own
// 401 arm remains as the fallback wording).
if (httpResponse.StatusCode == HttpStatusCode.Unauthorized) {
return BuildToolResult(id, await AuthRejectionNotice.ForPersistentUnauthorizedAsync(baseUrl), isError: true);
}

return BuildToolResult(id, MapResponse(toolName, httpResponse.StatusCode, body, out var isError), isError);
} catch (ArgumentException ex) {
return BuildToolResult(id, $"Error: {ex.Message}", isError: true);
Expand Down
4 changes: 2 additions & 2 deletions src/Capacitor.Cli/Commands/McpFlowResultServer.cs
Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,7 @@ async Task<string> TimedDispatchToolCallAsync(JsonNode callId, JsonObject callRe
return ("Result recorded. You may end your reply now.", false);

if (response.StatusCode == HttpStatusCode.Unauthorized)
return ("Not logged in. Run 'kcap login' on the host shell.", true);
return (await AuthRejectionNotice.ForPersistentUnauthorizedAsync(apiRoot), true);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

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


var errorNode = TryParse(responseBody);
var code = errorNode?["error"]?.GetValue<string>();
Expand Down Expand Up @@ -322,7 +322,7 @@ async Task<string> TimedDispatchToolCallAsync(JsonNode callId, JsonObject callRe
return ("Message sent to the flow driver. It will be delivered with the driver's next flow call — you may continue.", false);

if (response.StatusCode == HttpStatusCode.Unauthorized)
return ("Not logged in. Run 'kcap login' on the host shell.", true);
return (await AuthRejectionNotice.ForPersistentUnauthorizedAsync(apiRoot), true);

var responseBody = await response.Content.ReadAsStringAsync();
var errorNode = TryParse(responseBody);
Expand Down
13 changes: 7 additions & 6 deletions src/Capacitor.Cli/Commands/McpFlowsServer.cs
Original file line number Diff line number Diff line change
Expand Up @@ -242,7 +242,7 @@ internal static async Task<string> HandleToolCallAsync(
var postBody = await postResponse.Content.ReadAsStringAsync();

if (postResponse.StatusCode == HttpStatusCode.Unauthorized)
return BuildToolResult(id, "Not logged in. Run 'kcap login' on the host shell.", isError: true);
return BuildToolResult(id, await AuthRejectionNotice.ForPersistentUnauthorizedAsync(apiRoot), isError: true);

// Catalog-start protocol-v2 skew seam (404 means an old server, before any run
// started) plus an explicit-vendor echo check once the route matched.
Expand Down Expand Up @@ -334,7 +334,7 @@ internal static async Task<string> HandleToolCallAsync(
// send already had its go) is an auth problem, not a vendor one — say so, rather
// than printing a raw HTTP 401 the caller would read as a flow rejection.
if (retryResponse.StatusCode == HttpStatusCode.Unauthorized)
return BuildToolResult(id, "Not logged in. Run 'kcap login' on the host shell.", isError: true);
return BuildToolResult(id, await AuthRejectionNotice.ForPersistentUnauthorizedAsync(apiRoot), isError: true);

if (CheckVendorOverrideResult(toolName, preference, retryResponse.StatusCode, retryResponse.IsSuccessStatusCode, retryBody, out var retryRunIdToClose) is { } retryVendorCheck) {
if (retryRunIdToClose is not null)
Expand Down Expand Up @@ -389,7 +389,7 @@ internal static async Task<string> HandleToolCallAsync(
var body = await httpResponse.Content.ReadAsStringAsync();

if (httpResponse.StatusCode == HttpStatusCode.Unauthorized) {
return BuildToolResult(id, "Not logged in. Run 'kcap login' on the host shell.", isError: true);
return BuildToolResult(id, await AuthRejectionNotice.ForPersistentUnauthorizedAsync(apiRoot), isError: true);
}

if (!httpResponse.IsSuccessStatusCode) {
Expand Down Expand Up @@ -432,7 +432,8 @@ internal static async Task<string> HandleToolCallAsync(
/// the refresh flow for WorkOS / GitHubApp), update the client's <c>Authorization</c>
/// header, and retry the same request once. If refresh fails (genuinely not logged in
/// or refresh-token expired), the original 401 is returned and the caller surfaces the
/// friendly "Not logged in" message.
/// store-aware <see cref="AuthRejectionNotice"/> line (which keeps the legacy
/// "Not logged in" wording only for a genuinely missing login).
/// </summary>
static async Task<HttpResponseMessage> SendWithRefreshRetryAsync(
HttpClient client, string baseUrl, Func<HttpClient, CancellationToken, Task<HttpResponseMessage>> send,
Expand Down Expand Up @@ -1392,7 +1393,7 @@ static async Task<PollResult> PollUntilTerminalAsync(HttpClient client, string a
}

if (resp.StatusCode == HttpStatusCode.Unauthorized)
return new("Not logged in. Run 'kcap login' on the host shell.", true);
return new(await AuthRejectionNotice.ForPersistentUnauthorizedAsync(apiRoot), true);

// Fix #4: non-transient 4xx (e.g. 400, 403, 409 budget_unverifiable) fail
// immediately — coded bodies surface via FormatFlowStartError like the POST path.
Expand Down Expand Up @@ -1502,7 +1503,7 @@ static async Task<PollResult> PollStatusUntilTerminalAsync(

using (resp) {
if (resp.StatusCode == HttpStatusCode.Unauthorized)
return new("Not logged in. Run 'kcap login' on the host shell.", true);
return new(await AuthRejectionNotice.ForPersistentUnauthorizedAsync(apiRoot), true);

var statusCode = (int)resp.StatusCode;
if (statusCode is >= 400 and < 500) {
Expand Down
7 changes: 4 additions & 3 deletions src/Capacitor.Cli/Commands/McpMemoryServer.cs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
namespace Capacitor.Cli.Commands;

static class McpMemoryServer {
internal const string NotLoggedInMessage = "Not logged in. Run 'kcap login' on the host shell.";
internal const string NotLoggedInMessage = AuthRejectionNotice.NotLoggedIn;

public static async Task<int> RunAsync(string baseUrl) {
var cwdRepoHash = await ResolveCwdRepoHashAsync();
Expand Down Expand Up @@ -187,7 +187,7 @@ internal static async Task<string> HandleToolCallAsync(
var body = await httpResponse.Content.ReadAsStringAsync();

if (httpResponse.StatusCode == HttpStatusCode.Unauthorized) {
return BuildToolResult(id, NotLoggedInMessage, isError: true);
return BuildToolResult(id, await AuthRejectionNotice.ForPersistentUnauthorizedAsync(baseUrl), isError: true);
}

if (!httpResponse.IsSuccessStatusCode) {
Expand All @@ -210,7 +210,8 @@ internal static async Task<string> HandleToolCallAsync(
/// the refresh flow for WorkOS / GitHubApp), update the client's <c>Authorization</c>
/// header, and retry the same request once. If refresh fails (genuinely not logged in
/// or refresh-token expired), the original 401 is returned and the caller surfaces the
/// friendly "Not logged in" message.
/// store-aware <see cref="AuthRejectionNotice"/> line (which keeps the legacy
/// "Not logged in" wording only for a genuinely missing login).
/// </summary>
static async Task<HttpResponseMessage> SendWithRefreshRetryAsync(HttpClient client, string baseUrl, Func<HttpClient, Task<HttpResponseMessage>> send) {
var response = await send(client);
Expand Down
9 changes: 5 additions & 4 deletions src/Capacitor.Cli/Commands/McpSessionsServer.cs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
namespace Capacitor.Cli.Commands;

static class McpSessionsServer {
internal const string NotLoggedInMessage = "Not logged in. Run 'kcap login' on the host shell.";
internal const string NotLoggedInMessage = AuthRejectionNotice.NotLoggedIn;

public static async Task<int> RunAsync(string baseUrl) {
var cwdRepoHash = await ResolveCwdRepoHashAsync();
Expand Down Expand Up @@ -183,7 +183,7 @@ internal static async Task<string> HandleToolCallAsync(
var body = await httpResponse.Content.ReadAsStringAsync();

if (httpResponse.StatusCode == HttpStatusCode.Unauthorized) {
return BuildToolResult(id, NotLoggedInMessage, isError: true);
return BuildToolResult(id, await AuthRejectionNotice.ForPersistentUnauthorizedAsync(baseUrl), isError: true);
}

if (!httpResponse.IsSuccessStatusCode) {
Expand Down Expand Up @@ -219,7 +219,7 @@ static async Task<string> HandleSearchSessionsAsync(
var body = await first.Content.ReadAsStringAsync();

if (first.StatusCode == HttpStatusCode.Unauthorized) {
return BuildToolResult(id, NotLoggedInMessage, isError: true);
return BuildToolResult(id, await AuthRejectionNotice.ForPersistentUnauthorizedAsync(baseUrl), isError: true);
}

if (!first.IsSuccessStatusCode) {
Expand Down Expand Up @@ -269,7 +269,8 @@ static async Task<string> HandleSearchSessionsAsync(
/// the refresh flow for WorkOS / GitHubApp), update the client's <c>Authorization</c>
/// header, and retry the same request once. If refresh fails (genuinely not logged in
/// or refresh-token expired), the original 401 is returned and the caller surfaces the
/// friendly "Not logged in" message.
/// store-aware <see cref="AuthRejectionNotice"/> line (which keeps the legacy
/// "Not logged in" wording only for a genuinely missing login).
/// </summary>
static async Task<HttpResponseMessage> SendWithRefreshRetryAsync(HttpClient client, string baseUrl, Func<HttpClient, Task<HttpResponseMessage>> send) {
var response = await send(client);
Expand Down
7 changes: 4 additions & 3 deletions src/Capacitor.Cli/Commands/McpWorkItemsServer.cs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ namespace Capacitor.Cli.Commands;
/// memory this server has no repo/machine context to resolve — the only per-call input is the
/// session id and the declare selector, both carried in the tool arguments.</summary>
static class McpWorkItemsServer {
internal const string NotLoggedInMessage = "Not logged in. Run 'kcap login' on the host shell.";
internal const string NotLoggedInMessage = AuthRejectionNotice.NotLoggedIn;

internal const string NoSessionIdMessage =
"No session id: pass session_id explicitly or run inside a kcap-hooked session (KCAP_SESSION_ID or CODEX_THREAD_ID).";
Expand Down Expand Up @@ -176,7 +176,7 @@ string baseUrl
var body = await httpResponse.Content.ReadAsStringAsync();

if (httpResponse.StatusCode == HttpStatusCode.Unauthorized) {
return BuildToolResult(id, NotLoggedInMessage, isError: true);
return BuildToolResult(id, await AuthRejectionNotice.ForPersistentUnauthorizedAsync(baseUrl), isError: true);
}

if (!httpResponse.IsSuccessStatusCode) {
Expand All @@ -199,7 +199,8 @@ string baseUrl
/// the refresh flow for WorkOS / GitHubApp), update the client's <c>Authorization</c>
/// header, and retry the same request once. If refresh fails (genuinely not logged in
/// or refresh-token expired), the original 401 is returned and the caller surfaces the
/// friendly "Not logged in" message.
/// store-aware <see cref="AuthRejectionNotice"/> line (which keeps the legacy
/// "Not logged in" wording only for a genuinely missing login).
/// </summary>
static async Task<HttpResponseMessage> SendWithRefreshRetryAsync(HttpClient client, string baseUrl, Func<HttpClient, Task<HttpResponseMessage>> send) {
var response = await send(client);
Expand Down
Loading
Loading