fix(oauth): honor MCPORTER_OAUTH_NO_BROWSER outside the auth command - #284
Merged
steipete merged 2 commits intoAug 8, 2026
Merged
Conversation
clawsweeper
Bot
added
rating: 🦪 silver shellfish
Contributor
|
Codex review: needs maintainer review before merge. Reviewed August 8, 2026, 5:34 PM ET / 21:34 UTC. ClawSweeper reviewWhat this changesThe PR applies MCPORTER_OAUTH_NO_BROWSER to daemon OAuth flows, returns a non-retryable authorization failure, and refreshes daemons when that setting changes. Merge readinessKeep this PR open: current main still launches the browser in this daemon path, and the latest maintainer-authored follow-up addresses the prior replay concern with a stable OAuth error code and focused boundary coverage. Priority: P2 Review scores
Verification
How this fits togetherMCPorter’s daemon handles long-lived MCP tool calls and creates OAuth sessions when a remote server requires authorization. This change controls whether that session may launch a browser and how its failure is returned to the caller. flowchart LR
A[MCP tool request] --> B[Daemon client]
B --> C[Daemon host]
C --> D[OAuth session]
D --> E{Browser suppressed?}
E -->|No| F[Launch authorization browser]
E -->|Yes| G[OAuth flow error]
G --> B
B --> H[Fail call without replay]
Before merge
Agent review detailsSecurityNone. Review metrics
Root-cause clusterRelationship: Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge-risk optionsMaintainer options:
Copy recommended automerge instructionTechnical reviewBest possible solution: Merge the existing environment-variable path with its stable daemon error contract so headless callers receive one actionable authorization failure while interactive auth remains unchanged. Do we have a high-confidence way to reproduce the issue? Yes. Current main source shows the browser launcher is used unless an explicit session option is supplied, and the PR provides a real daemon OAuth-401 transcript for the affected path. Is this the best way to solve the issue? Yes. Reusing the established environment variable and preserving a non-retryable error across the existing daemon boundary is narrower than adding a new setting or separate auth mode. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against c2747e2570c4. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (17 earlier review cycles; latest 8 shown)
|
vitalijssilins
force-pushed
the
fix/env-no-browser-serve-path
branch
from
August 7, 2026 08:42
0555305 to
fbc594d
Compare
Contributor
Author
|
@clawsweeper re-review — added real daemon-run proof (real 401 against a live OAuth server from an isolated home; transcript in the PR body) and hardened the headless path to fail fast with a server-named error instead of waiting out the authorization timeout, per the risk flagged in #283. |
Contributor
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
clawsweeper
Bot
added
proof: sufficient
clawsweeper
Bot
added
rating: 🦞 diamond lobster
…penclaw#283) The env var was parsed only in the auth command's argv handling, so the serve/daemon path still launched the system browser on every 401. Read it as a fallback in redirectToAuthorization (an explicit suppressBrowserLaunch option still wins). When no onAuthorizationUrl callback is registered (the headless serve/daemon case), log the authorization URL at warn level and reject the pending authorization with a server-named BrowserLaunchSuppressedError so callers fail fast instead of waiting out the authorization timeout. The rejection path is used deliberately: the SDK invokes redirectToAuthorization from unawaited send paths, where a throw becomes an unhandled rejection that kills the daemon.
Co-authored-by: Vitālijs Silins-Ozers <silins@silins.lv>
steipete
force-pushed
the
fix/env-no-browser-serve-path
branch
from
August 8, 2026 21:30
fbc594d to
b7e29fa
Compare
clawsweeper
Bot
added
rating: 🐚 platinum hermit
clawsweeper
Bot
added
the
status: 👀 ready for maintainer look
Collaborator
|
Exact-head maintainer proof for
Gaps: the proof intentionally used synthetic local OAuth metadata/DCR and no credentials; this in-process browser-suppression, daemon freshness, and no-replay boundary does not require an external provider or real browser. Thanks @vitalijssilins for the high-quality issue, core provider implementation, and real daemon investigation. |
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
2 participants
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.
Fixes #283.
What
MCPORTER_OAUTH_NO_BROWSERwas parsed only inside the auth command's argv handling (consumeBrowserSuppression), so the serve/daemon path still launched the system browser on every 401 — a focus-stealing, context-free tab when a token expires under an MCP bridge, and a tab loop when auth keeps failing (details in the issue).How
redirectToAuthorizationnow falls back to the env var when no explicitsuppressBrowserLaunchoption was passed:this.options.suppressBrowserLaunch ?? suppressBrowserLaunchFromEnv(). An explicit option always wins, and the accepted values (1/true/yes, case-insensitive) mirror the auth command'sTRUE_VALUES.onAuthorizationUrlcallback is registered (the headless serve/daemon case), the authorization URL is logged at warn level and the pending authorization is rejected with a server-namedBrowserLaunchSuppressedError— callers fail fast with an actionable message (run 'mcporter auth <name> --no-browser') instead of silently waiting out the authorization timeout (the "remaining risk" flagged inserve/daemon path launches the system browser on 401 with no way to suppress it (MCPORTER_OAUTH_NO_BROWSERis auth-command-only) #283's review).redirectToAuthorizationfrom unawaited send paths, where a throw becomes an unhandled rejection that kills the daemon (we hit exactly that in a real run while iterating).suppressBrowserLaunch: truewith a callback (the auth command) are unaffected.Real behavior proof (daemon, real 401)
Isolated
$HOMEwith an empty vault (guaranteed 401), one keep-alive OAuth server (Supabase's public MCP endpoint), foreground daemon built from this branch, realcallToolthrough the daemon client:No browser launched at any point (no
openspawn; nothing hit the OS). On current main the same run logsAuthorization required for supabase-room. Opening browser...and launches the system browser from the daemon.(The client_id in the transcript is a throwaway Dynamic Client Registration from the isolated home, since deleted.)
Tests
openExternalnot called, URL surfaced at warn level, late-attaching waiter rejects withBrowserLaunchSuppressedError(server-named).0) → browser opens as before.suppressBrowserLaunch: true+ callback (auth command path) → unchanged, existing test still green.pnpm checkand fullpnpm testgreen (1283 passed).