From 0d735946834ae1aeb6885b9951900447d372ed03 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 18 Apr 2026 09:42:24 +0000 Subject: [PATCH 1/2] Don't auto-retry bearer-token sessions stuck in 'unauthorized' Unauthorized sessions were being flipped back to 'connecting' by consolidateSessions() on every `mcpc` invocation, even when the only credentials were a static bearer token supplied via --header. These retries cannot succeed because the token never changes, so the displayed status kept toggling between 'connecting' and 'unauthorized' and the real state was only surfaced once the user actually hit the session. Scope the auto-retry to sessions with an OAuth profile, where another session may have refreshed the shared tokens in the keychain. Also include the bridge log path in the server auth error so the user has a pointer for debugging, matching how the expired-session error already surfaces the log path. https://claude.ai/code/session_011CjxmErafRX4ypgb2CQkea --- CHANGELOG.md | 5 +++++ src/cli/commands/sessions.ts | 4 +++- src/lib/bridge-manager.ts | 5 ++++- src/lib/errors.ts | 9 ++++++--- src/lib/sessions.ts | 11 ++++++++--- 5 files changed, 26 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 82c18e14..ce080beb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `tools-call --task` now prints the task ID and recovery commands when interrupted with Ctrl+C, so you can fetch or cancel the server-side task later - `tasks-get` no longer suggests `tasks-cancel` when the task has already reached a terminal state (completed, failed, or cancelled) +### Fixed + +- Sessions using a static bearer token (via `--header "Authorization: ..."`) no longer flip between `unauthorized` and `connecting` on every `mcpc` invocation — they stay `unauthorized` since retrying the same rejected token cannot succeed without `mcpc login` or reconnecting. OAuth-profile sessions still auto-retry because tokens may have been refreshed by another session +- Server authentication errors now include the path to the bridge log file, so you can inspect it for more detail when investigating why a session was rejected + ### Removed - Removed `tools`, `resources`, and `prompts` shorthand commands — use the full names (`tools-list`, `resources-list`, `prompts-list`) instead diff --git a/src/cli/commands/sessions.ts b/src/cli/commands/sessions.ts index e3e52ad4..142dbecc 100644 --- a/src/cli/commands/sessions.ts +++ b/src/cli/commands/sessions.ts @@ -11,6 +11,7 @@ import { validateProfileName, isProcessAlive, getServerHost, + getLogsDir, redactHeaders, } from '../../lib/index.js'; import { DISCONNECTED_THRESHOLD_MS } from '../../lib/types.js'; @@ -457,7 +458,8 @@ export async function connectSession( // Fallback: check error message for auth patterns (error may have been wrapped // as ClientError/ServerError during bridge IPC serialization) if (detailsError instanceof Error && isAuthenticationError(detailsError.message)) { - throw createServerAuthError(serverConfig.url || target, { sessionName: name }); + const logPath = `${getLogsDir()}/bridge-${name}.log`; + throw createServerAuthError(serverConfig.url || target, { sessionName: name, logPath }); } // Non-auth failure: session was created but server didn't respond properly. diff --git a/src/lib/bridge-manager.ts b/src/lib/bridge-manager.ts index af794438..f54c245d 100644 --- a/src/lib/bridge-manager.ts +++ b/src/lib/bridge-manager.ts @@ -72,8 +72,10 @@ async function classifyAndThrowSessionError( logger.warn(`Failed to mark session ${sessionName} as unauthorized:`, e) ); const target = session.server.url || session.server.command || sessionName; + const logPath = `${getLogsDir()}/bridge-${sessionName}.log`; throw createServerAuthError(target, { sessionName, + logPath, ...(originalError && { originalError }), }); } @@ -598,7 +600,8 @@ export async function ensureBridgeReady(sessionName: string): Promise { if (session.status === 'unauthorized') { const target = session.server.url || session.server.command || sessionName; - throw createServerAuthError(target, { sessionName }); + const logPath = `${getLogsDir()}/bridge-${sessionName}.log`; + throw createServerAuthError(target, { sessionName, logPath }); } if (session.status === 'expired') { diff --git a/src/lib/errors.ts b/src/lib/errors.ts index ff2448bb..38144ea7 100644 --- a/src/lib/errors.ts +++ b/src/lib/errors.ts @@ -97,12 +97,11 @@ export function isAuthenticationError(errorMessage: string): boolean { * Create an AuthError with helpful login guidance for server auth failures * * @param target - Server URL or target for login command - * @param options - Optional session name for session-specific guidance - * @param originalError - Original error for debugging + * @param options - Optional session name for session-specific guidance and log path for debugging */ export function createServerAuthError( target: string, - options?: { sessionName?: string; originalError?: Error } + options?: { sessionName?: string; logPath?: string; originalError?: Error } ): AuthError { let hint: string; if (options?.sessionName) { @@ -117,6 +116,10 @@ export function createServerAuthError( `To authenticate, run:\n` + ` mcpc login ${target}\n\n` + `Then run your command again.`; } + if (options?.logPath) { + hint += `\n\nFor details, check logs at ${options.logPath}`; + } + return new AuthError( `Authentication required by server.\n\n` + hint, options?.originalError ? { originalError: options.originalError } : undefined diff --git a/src/lib/sessions.ts b/src/lib/sessions.ts index 34b9bdb6..23aa48c1 100644 --- a/src/lib/sessions.ts +++ b/src/lib/sessions.ts @@ -381,10 +381,15 @@ export async function consolidateSessions( } // Identify crashed or unauthorized sessions eligible for automatic reconnection. - // Unauthorized sessions are included because another session sharing the same OAuth - // profile may have refreshed the tokens — the bridge reads from keychain on startup. + // Unauthorized sessions are only included when the session has an OAuth profile — + // another session sharing the same profile may have refreshed the tokens, so a + // retry can succeed as the bridge re-reads from keychain on startup. Sessions + // without a profile (e.g. static bearer token via --header) cannot self-heal, so + // retrying would just flip the status back to 'connecting' on every `mcpc` call + // and hide the real state from the user. for (const [name, session] of Object.entries(storage.sessions)) { - if ((session?.status === 'crashed' || session?.status === 'unauthorized') && !session.pid) { + const isRetryableUnauthorized = session?.status === 'unauthorized' && !!session.profileName; + if ((session?.status === 'crashed' || isRetryableUnauthorized) && !session.pid) { // Skip if a connection was already attempted within the cooldown window const lastAttempt = session.lastConnectionAttemptAt ? new Date(session.lastConnectionAttemptAt).getTime() From bd86068aa69f0129b33dca7ee0ab6543fde50692 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 18 Apr 2026 10:00:45 +0000 Subject: [PATCH 2/2] Add e2e test for unauthorized bearer session persistence MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Covers the two guarantees added in the previous commit: - A bearer-only session that fails the initial connect returns exit code 4 and an auth error whose message includes the bridge log path (`check logs at .../bridge-.log`). - The failed session stays `unauthorized` across `mcpc` invocations, even after `lastConnectionAttemptAt` is aged past the 10s auto-retry cooldown. Before the fix, consolidateSessions() would flip the status back to `connecting` whenever the cooldown window had elapsed. The cooldown assertion fakes the post-failure bridge state (old `lastConnectionAttemptAt`, pid cleared) by editing sessions.json directly — otherwise we'd need to wait 10+ seconds for the bridge to exit on its own, which would slow the suite down. Verified both ways: test passes with the fix, reverting only src/lib/sessions.ts causes assertion 3 to fail with exactly the regression message (`expected status to stay 'unauthorized' after cooldown, got: 'connecting'`). https://claude.ai/code/session_011CjxmErafRX4ypgb2CQkea --- .../sessions/unauthorized-persist.test.sh | 98 +++++++++++++++++++ 1 file changed, 98 insertions(+) create mode 100755 test/e2e/suites/sessions/unauthorized-persist.test.sh diff --git a/test/e2e/suites/sessions/unauthorized-persist.test.sh b/test/e2e/suites/sessions/unauthorized-persist.test.sh new file mode 100755 index 00000000..c9eaa8a0 --- /dev/null +++ b/test/e2e/suites/sessions/unauthorized-persist.test.sh @@ -0,0 +1,98 @@ +#!/bin/bash +# Test: Sessions that fail with 401 and have no OAuth profile stay 'unauthorized' +# across `mcpc` invocations, even after the auto-retry cooldown has elapsed. +# Also verifies the auth error message points at the bridge log file. + +source "$(dirname "$0")/../../lib/framework.sh" +test_init "sessions/unauthorized-persist" --isolated + +# Start test server requiring auth. The server's auth check accepts any +# "Bearer " value, so sending an Authorization header that doesn't +# match that shape is enough to trigger 401. +start_test_server REQUIRE_AUTH=true + +SESSION=$(session_name "unauth") +_SESSIONS_CREATED+=("$SESSION") + +# ============================================================================= +# Test: bearer-only session fails with auth error containing log path +# ============================================================================= + +test_case "connect with bad bearer token fails with auth error + log path" +run_mcpc connect "$TEST_SERVER_URL" "$SESSION" \ + --header "X-Test: true" \ + --header "Authorization: InvalidScheme not-a-bearer-token" +assert_failure +assert_exit_code 4 "should fail with auth exit code (4)" +assert_contains "$STDERR" "Authentication required by server" +# The error should point at the bridge log file so the user can investigate +assert_contains "$STDERR" "check logs at" +assert_contains "$STDERR" "bridge-${SESSION}.log" +test_pass + +# ============================================================================= +# Test: session is marked unauthorized right after the failed connect +# ============================================================================= + +test_case "failed bearer session reports 'unauthorized' status" +run_mcpc --json +assert_success +session_status=$(echo "$STDOUT" | jq -r ".sessions[] | select(.name == \"$SESSION\") | .status") +if [[ "$session_status" != "unauthorized" ]]; then + test_fail "expected status 'unauthorized', got: '$session_status'" + exit 1 +fi +test_pass + +# ============================================================================= +# Test: unauthorized bearer session does NOT flip back to 'connecting' +# after the auto-retry cooldown expires. +# +# Before the fix, consolidateSessions() treated all unauthorized sessions as +# retry candidates and reset the status to 'connecting' on every `mcpc` call +# (after the 10s cooldown), which both hid the real state from the user and +# triggered pointless background bridge restarts. Bearer-only sessions cannot +# self-heal because the token never changes. +# ============================================================================= + +test_case "unauthorized bearer session stays unauthorized past the cooldown" +sessions_file="$MCPC_HOME_DIR/sessions.json" +assert_file_exists "$sessions_file" + +# Age the lastConnectionAttemptAt timestamp well past the 10s cooldown and +# clear the bridge pid to simulate the bridge having exited — that's the real +# state consolidateSessions() sees when the user runs `mcpc` well after the +# original failed connect. Leaving pid set would short-circuit the retry path +# via `!session.pid`, hiding the bug we are trying to catch. +old_iso="2000-01-01T00:00:00.000Z" +tmp_file="$TEST_TMP/sessions.json.$$" +jq --arg name "$SESSION" --arg when "$old_iso" \ + '.sessions[$name].lastConnectionAttemptAt = $when + | del(.sessions[$name].pid)' \ + "$sessions_file" > "$tmp_file" +mv "$tmp_file" "$sessions_file" + +# Re-run the list command — consolidateSessions() runs on every list. +run_mcpc --json +assert_success +session_status=$(echo "$STDOUT" | jq -r ".sessions[] | select(.name == \"$SESSION\") | .status") +if [[ "$session_status" != "unauthorized" ]]; then + test_fail "expected status to stay 'unauthorized' after cooldown, got: '$session_status'" + exit 1 +fi +test_pass + +# ============================================================================= +# Test: using an unauthorized session surfaces the auth error + log path +# ============================================================================= + +test_case "using unauthorized session surfaces auth error with log path" +run_mcpc "$SESSION" tools-list +assert_failure +assert_exit_code 4 "should fail with auth exit code (4)" +assert_contains "$STDERR" "Authentication required by server" +assert_contains "$STDERR" "check logs at" +assert_contains "$STDERR" "bridge-${SESSION}.log" +test_pass + +test_done