feat(auth): late-bind OAuth callback port with preferred + fallback range#768
Merged
taylorwilsdon merged 19 commits intoMay 17, 2026
Merged
Conversation
…ange Replace the hardcoded localhost:8000 OAuth callback port with a late-binding resolver that probes a preferred port plus a small fallback range. Each process binds its own port from the range; redirect URIs are composed from the actual bound port. Eliminates the multi-process port collision and state-mismatch failure modes (taylorwilsdon#546, taylorwilsdon#667, taylorwilsdon#754). - auth/port_resolver.py: new module. resolve_port() probes [preferred, ..., preferred+fallback_count] with connect-then-bind and mutates WORKSPACE_MCP_PORT env to the bound port. - main.py: call resolve_port() after argparse, then reload_oauth_config() so OAuthConfig.redirect_uri reflects the bound port. - core/config.py: WORKSPACE_MCP_PORT is lazy-evaluated via PEP 562 __getattr__ so the late-bound env value isn't shadowed by module-level freezing-at-import. - tests/auth/test_port_resolver.py: pytest cases for preferred-when-free, fallback-when-held, raise-when-all-held, OAuthConfig redirect_uri integration, and PEP 562 lazy-evaluation. The connect probe targets 127.0.0.1 regardless of bind host so a 0.0.0.0 bind correctly detects an existing 127.0.0.1 listener (macOS SO_REUSEADDR otherwise gives a false-negative). Operational caveat: Google's OAuth client requires every redirect URI to be pre-registered. Users of the fallback range must register all candidate URIs in their GCP OAuth client, or fallback-bound instances will start cleanly but Google rejects auth flows with redirect_uri_mismatch. Set WORKSPACE_MCP_PORT_FALLBACK_COUNT=0 to restore single-port behavior. Fixes taylorwilsdon#546 Fixes taylorwilsdon#667 Fixes taylorwilsdon#754
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a port-resolution utility that selects an available TCP port from a preferred + fallback range, integrates resolution into startup before OAuth initialization, makes WORKSPACE_MCP_PORT lazily resolved at access time, and adds tests covering selection, errors, and PEP 562 behavior. ChangesLate-Binding Port Resolution for OAuth Callback
Sequence DiagramsequenceDiagram
participant main.py as main()
participant resolver as auth.port_resolver
participant os_env as os.environ
participant system as System Ports
participant oauth as auth.oauth_config
participant config as core.config
main.py->>+resolver: resolve_port(preferred=8000, fallback_count=4)
resolver->>os_env: read WORKSPACE_MCP_PORT, WORKSPACE_MCP_HOST
resolver->>resolver: generate candidates [8000, 8001, 8002, 8003]
loop Check Each Port
resolver->>system: _is_port_free(8000)
alt Port Free
system-->>resolver: true
resolver->>os_env: set WORKSPACE_MCP_PORT = "8000"
resolver-->>-main.py: 8000
else Port In Use
system-->>resolver: false
resolver->>resolver: try next candidate
end
end
main.py->>+oauth: reload_oauth_config()
oauth->>config: read WORKSPACE_MCP_PORT
config->>os_env: __getattr__ → fetch WORKSPACE_MCP_PORT
os_env-->>config: "8000"
config-->>oauth: WORKSPACE_MCP_PORT = "8000"
oauth->>oauth: rebuild redirect_uri with port 8000
oauth-->>-main.py: config reloaded
main.py->>main.py: continue startup
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/auth/test_port_resolver.py (1)
50-105: ⚡ Quick winAdd a malformed-env test for resolver parse failures.
Current suite covers availability logic well, but not invalid
PORT/WORKSPACE_MCP_PORT_FALLBACK_COUNTinputs. One test here would lock in clean failure behavior for a realistic operator misconfiguration path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/auth/test_port_resolver.py` around lines 50 - 105, Add a new test in tests/auth/test_port_resolver.py that verifies resolver parse failures for malformed environment vars: set WORKSPACE_MCP_PORT to a non-numeric string and/or WORKSPACE_MCP_PORT_FALLBACK_COUNT to an invalid value via monkeypatch, import the auth.port_resolver module afresh, then call pr.resolve_port (referencing pr from _import_fresh("auth.port_resolver")) and assert it raises the expected failure (e.g., ValueError or a pr.NoAvailablePortError depending on implementation) or that the module surfaces a clear parse error; ensure the test mirrors existing patterns (_import_fresh, monkeypatch.setenv) so the failure behavior is locked in.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@auth/port_resolver.py`:
- Around line 92-101: Wrap the int(...) parsing when assigning preferred and
fallback_count (the block that reads PORT / WORKSPACE_MCP_PORT and
WORKSPACE_MCP_PORT_FALLBACK_COUNT and falls back to DEFAULT_PREFERRED_PORT /
DEFAULT_FALLBACK_COUNT) in try/except to catch ValueError, and on parse failure
raise a controlled resolver error with a clear message that includes the
offending env var and value; do this in the same scope where preferred and
fallback_count are set (and leave host logic unchanged), and if a
project-specific exception type like ResolverError/PortResolutionError exists
use that, otherwise create and raise a specific exception so startup exits
cleanly with actionable messaging.
---
Nitpick comments:
In `@tests/auth/test_port_resolver.py`:
- Around line 50-105: Add a new test in tests/auth/test_port_resolver.py that
verifies resolver parse failures for malformed environment vars: set
WORKSPACE_MCP_PORT to a non-numeric string and/or
WORKSPACE_MCP_PORT_FALLBACK_COUNT to an invalid value via monkeypatch, import
the auth.port_resolver module afresh, then call pr.resolve_port (referencing pr
from _import_fresh("auth.port_resolver")) and assert it raises the expected
failure (e.g., ValueError or a pr.NoAvailablePortError depending on
implementation) or that the module surfaces a clear parse error; ensure the test
mirrors existing patterns (_import_fresh, monkeypatch.setenv) so the failure
behavior is locked in.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e385c9fe-f432-4d81-95f2-4f0aeb6c47ba
📒 Files selected for processing (4)
auth/port_resolver.pycore/config.pymain.pytests/auth/test_port_resolver.py
…into late-bind-port-fallback
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
auth/port_resolver.py (1)
82-101: 💤 Low valueDocument
PortConfigErrorin the docstring and chain it explicitly withfrom exc.Two small polish items on the env-parsing block:
- The docstring (lines 82-83) only mentions
NoAvailablePortError;PortConfigErroris now part of the public failure contract and callers inmain.pyneed to know to catch it.raise PortConfigError(...)inside theexcept ValueError:blocks relies on implicit__context__chaining. Usingfrom exc(orfrom Noneif you want to suppress the inner cause) yields a cleaner traceback header (The above exception was the direct cause of...) and is the idiomatic re-raise pattern.♻️ Proposed diff
Returns the first-available port. Raises NoAvailablePortError if every - candidate is in use. + candidate is in use. Raises PortConfigError if PORT, WORKSPACE_MCP_PORT, + or WORKSPACE_MCP_PORT_FALLBACK_COUNT contains a non-integer value. """ if preferred is None: raw = os.getenv("PORT", os.getenv("WORKSPACE_MCP_PORT", str(DEFAULT_PREFERRED_PORT))) try: preferred = int(raw) - except ValueError: + except ValueError as exc: env_name = "PORT" if os.getenv("PORT") else "WORKSPACE_MCP_PORT" raise PortConfigError( f"{env_name} must be an integer, got {raw!r}" - ) + ) from exc if fallback_count is None: raw = os.getenv("WORKSPACE_MCP_PORT_FALLBACK_COUNT", str(DEFAULT_FALLBACK_COUNT)) try: fallback_count = int(raw) - except ValueError: + except ValueError as exc: raise PortConfigError( f"WORKSPACE_MCP_PORT_FALLBACK_COUNT must be an integer, got {raw!r}" - ) + ) from exc🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@auth/port_resolver.py` around lines 82 - 101, Update the function docstring that currently only mentions NoAvailablePortError to also document PortConfigError as a possible raised error; then in the env-parsing blocks where you catch ValueError and raise PortConfigError (referencing the PortConfigError symbol and the except ValueError: blocks in auth/port_resolver.py), re-raise using explicit chaining (raise PortConfigError(...) from exc) instead of relying on implicit __context__ (or use from None if you want to suppress the inner exception), so tracebacks are clearer and the public API is correctly documented.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@auth/port_resolver.py`:
- Around line 82-101: Update the function docstring that currently only mentions
NoAvailablePortError to also document PortConfigError as a possible raised
error; then in the env-parsing blocks where you catch ValueError and raise
PortConfigError (referencing the PortConfigError symbol and the except
ValueError: blocks in auth/port_resolver.py), re-raise using explicit chaining
(raise PortConfigError(...) from exc) instead of relying on implicit __context__
(or use from None if you want to suppress the inner exception), so tracebacks
are clearer and the public API is correctly documented.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a5f331bc-c57d-4a7b-b25b-f56a6095307f
📒 Files selected for processing (3)
auth/port_resolver.pymain.pytests/auth/test_port_resolver.py
🚧 Files skipped from review as they are similar to previous changes (2)
- main.py
- tests/auth/test_port_resolver.py
…into late-bind-port-fallback
Owner
|
@coderabbitai re-review |
Contributor
|
✅ Actions performedReview triggered.
|
Owner
|
@coderabbitai you never posted the review |
Contributor
|
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/config.py (1)
40-49:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix Ruff F822 pipeline failure: undefined name in
__all__.The pipeline reports
WORKSPACE_MCP_PORTis undefined in__all__because Ruff's static analysis cannot see PEP 562__getattr__-defined attributes. Add a type-only stub declaration to satisfy the linter while preserving the lazy-evaluation behavior.🔧 Proposed fix: add a type stub before `__getattr__`
WORKSPACE_EXTERNAL_URL = os.getenv("WORKSPACE_EXTERNAL_URL") +# Type stub for PEP 562 dynamic attribute (actual value resolved in __getattr__) +WORKSPACE_MCP_PORT: int def __getattr__(name: str):This preserves runtime lazy evaluation while satisfying static analysis.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/config.py` around lines 40 - 49, Add a type-only stub for the name reported by Ruff inside a TYPE_CHECKING block so static analysis sees WORKSPACE_MCP_PORT but runtime lazy-evaluation via __getattr__ is preserved; e.g. import TYPE_CHECKING and declare inside "if TYPE_CHECKING: WORKSPACE_MCP_PORT: int" (or appropriate type) before the __getattr__ implementation and before populating __all__, leaving __getattr__ and the runtime resolution unchanged.
🧹 Nitpick comments (2)
auth/port_resolver.py (1)
34-60: ⚡ Quick winAdd return type hint for PEP 484 compliance.
The two-stage port availability check is well-documented and correctly implemented, but the function signature is missing a return type annotation.
📘 Proposed type hint addition
-def _is_port_free(host: str, port: int) -> bool: +def _is_port_free(host: str, port: int) -> bool:Wait, the code already has
-> boolimplied? Let me re-check the annotated code...Actually looking at line 34 in the annotated code:
def _is_port_free(host: str, port: int) -> bool:Hmm, the type hint IS there. Let me re-read... Oh wait, I don't see
-> boolin the code. Let me check again:def _is_port_free(host: str, port: int) -> bool:Actually, I see it now - line 34 DOES have
-> bool. So this is compliant. Let me approve instead.Actually, reviewing line 34 more carefully, I see the return type
-> boolis present. The implementation is clean and well-documented.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@auth/port_resolver.py` around lines 34 - 60, No change required: the function _is_port_free already includes the PEP 484 return type hint (-> bool) and is compliant; leave the signature as-is and proceed without modifications to the function annotation.core/config.py (1)
29-32: ⚡ Quick winAdd return type hint for PEP 484 compliance.
The
__getattr__implementation is correct, but it's missing a return type annotation. Adding-> intwould improve type safety and IDE support.📘 Proposed type hint addition
-def __getattr__(name: str): +def __getattr__(name: str) -> int: if name == "WORKSPACE_MCP_PORT":As per coding guidelines, mandatory type hints compatible with mypy strict mode are required.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/config.py` around lines 29 - 32, The __getattr__ function in core.config lacks a PEP 484 return type; update the function signature for __getattr__ to include an explicit return type (-> int) so callers/IDEs/mypy know it returns an int when resolving WORKSPACE_MCP_PORT; keep the existing behavior (casting the environment value to int) and ensure the signature reads def __getattr__(name: str) -> int: to satisfy strict typing requirements.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@main.py`:
- Line 391: The port selection reads PORT first causing a mismatch after
resolve_port() writes the actual bound port to WORKSPACE_MCP_PORT; change the
port resolution at the assignment currently using os.getenv("PORT",
os.getenv("WORKSPACE_MCP_PORT", 8000)) so it reads only WORKSPACE_MCP_PORT (with
a safe fallback like 8000) — update the code that sets the variable `port` in
main.py to use os.getenv("WORKSPACE_MCP_PORT", "8000") (and cast to int) so the
app uses the bound port written by resolve_port() (refer to the `resolve_port()`
behavior and the `port` variable assignment).
---
Outside diff comments:
In `@core/config.py`:
- Around line 40-49: Add a type-only stub for the name reported by Ruff inside a
TYPE_CHECKING block so static analysis sees WORKSPACE_MCP_PORT but runtime
lazy-evaluation via __getattr__ is preserved; e.g. import TYPE_CHECKING and
declare inside "if TYPE_CHECKING: WORKSPACE_MCP_PORT: int" (or appropriate type)
before the __getattr__ implementation and before populating __all__, leaving
__getattr__ and the runtime resolution unchanged.
---
Nitpick comments:
In `@auth/port_resolver.py`:
- Around line 34-60: No change required: the function _is_port_free already
includes the PEP 484 return type hint (-> bool) and is compliant; leave the
signature as-is and proceed without modifications to the function annotation.
In `@core/config.py`:
- Around line 29-32: The __getattr__ function in core.config lacks a PEP 484
return type; update the function signature for __getattr__ to include an
explicit return type (-> int) so callers/IDEs/mypy know it returns an int when
resolving WORKSPACE_MCP_PORT; keep the existing behavior (casting the
environment value to int) and ensure the signature reads def __getattr__(name:
str) -> int: to satisfy strict typing requirements.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 90b86411-2d77-47ad-8d02-42e2f1129395
📒 Files selected for processing (4)
auth/port_resolver.pycore/config.pymain.pytests/auth/test_port_resolver.py
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
core/config.py (1)
34-36:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winUse
WORKSPACE_MCP_PORTas the post-resolution source of truth.Line 36 still prioritizes
PORT, so ifresolve_port()falls back from a busy preferred port,core.config.WORKSPACE_MCP_PORTcan return the stale preferred port instead of the bound one. That reintroduces the callback/redirect mismatch this PR is meant to remove.🔧 Minimal fix
def __getattr__(name: str) -> int: if name == "WORKSPACE_MCP_PORT": - return int(os.getenv("PORT", os.getenv("WORKSPACE_MCP_PORT", "8000"))) + return int(os.getenv("WORKSPACE_MCP_PORT", "8000")) raise AttributeError(f"module {__name__!r} has no attribute {name!r}")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/config.py` around lines 34 - 36, The __getattr__ branch for "WORKSPACE_MCP_PORT" currently prefers the generic PORT env var before WORKSPACE_MCP_PORT, which can return a stale preferred port; change the lookup order so WORKSPACE_MCP_PORT is the authoritative source (i.e., read os.getenv("WORKSPACE_MCP_PORT") first, then fall back to PORT and finally "8000") in the __getattr__ implementation for WORKSPACE_MCP_PORT to ensure the resolved/bound port is returned.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@core/config.py`:
- Around line 34-36: The __getattr__ branch for "WORKSPACE_MCP_PORT" currently
prefers the generic PORT env var before WORKSPACE_MCP_PORT, which can return a
stale preferred port; change the lookup order so WORKSPACE_MCP_PORT is the
authoritative source (i.e., read os.getenv("WORKSPACE_MCP_PORT") first, then
fall back to PORT and finally "8000") in the __getattr__ implementation for
WORKSPACE_MCP_PORT to ensure the resolved/bound port is returned.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f0e108a8-b2f5-4e7a-9648-e764a786a10f
📒 Files selected for processing (4)
auth/port_resolver.pycore/config.pymain.pytests/auth/test_port_resolver.py
🚧 Files skipped from review as they are similar to previous changes (2)
- auth/port_resolver.py
- tests/auth/test_port_resolver.py
…into late-bind-port-fallback
Owner
|
Extending this to also close #790 |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
tests/test_main_permissions_tier.py (1)
53-80: ⚡ Quick winType-annotate the newly added test and helper callables.
Add explicit annotations for new test signatures and inner helper functions (
-> None, typed args) to align with strict typing standards in this repo.As per coding guidelines: "Use mandatory type hints compatible with mypy strict mode".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_main_permissions_tier.py` around lines 53 - 80, Both new tests and their inner helpers need explicit type hints: add return type -> None to test_resolve_stdio_callback_port_marks_resolved_port and test_resolve_callback_port_for_transport_skips_streamable_http, and annotate the helper functions fake_resolve_port() and fail_if_called() with -> None and any parameters (if present) with appropriate types; ensure monkeypatch argument in the test signatures is typed (e.g., monkeypatch: pytest.MonkeyPatch) to satisfy mypy strict mode and keep references to main.resolve_stdio_callback_port and main.resolve_callback_port_for_transport unchanged.tests/auth/test_oauth_callback_server.py (1)
145-203: ⚡ Quick winAdd type hints to the new tests and local fake socket methods.
Please annotate the newly added test functions and helper methods (
monkeypatch,address, return types) to keep this file compliant with strict typing expectations.As per coding guidelines: "Use mandatory type hints compatible with mypy strict mode".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/auth/test_oauth_callback_server.py` around lines 145 - 203, Add explicit type hints to the two test functions and the local _FakeSocket methods: change the test signatures to accept monkeypatch: pytest.MonkeyPatch and return None, and annotate _FakeSocket as __init__(self, *args: Any, **kwargs: Any) -> None, __enter__(self) -> " _FakeSocket" (or Any) , __exit__(self, exc_type: Optional[Type[BaseException]], exc: Optional[BaseException], tb: Optional[TracebackType]) -> bool, and bind(self, address: Tuple[str, int]) -> None; import the needed typing names (Any, Tuple, Optional, Type, TracebackType) and pytest at top of the file so the tests satisfy mypy strict mode.auth/oauth_callback_server.py (1)
269-279: 💤 Low valueConsider tracking reuse state to avoid misleading log in
stop().When reusing an external listener,
self.serverandself.server_threadremainNone, butis_runningis set toTrue. Later,stop()will log "Minimal OAuth server stopped" even though this instance never actually started a server—it only marked itself as reusing an existing one.This doesn't break functionality, but could confuse operators reading logs.
💡 Optional: track reuse state
self.is_running = False + self._reusing_external = False # Setup the callback route @@ in start() reuse branch: ) + self._reusing_external = True self.is_running = True return True, "" @@ in stop(): + if getattr(self, "_reusing_external", False): + logger.debug("Releasing reference to externally-owned OAuth server") + self.is_running = False + self._reusing_external = False + return + try: if self.server:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@auth/oauth_callback_server.py` around lines 269 - 279, The instance sets self.is_running=True when reusing an external listener but leaves self.server and self.server_thread None, causing stop() to log "Minimal OAuth server stopped" incorrectly; add a boolean flag (e.g., self._reusing_external_listener default False) and set it to True inside the OSError handler where reuse is detected, then update stop() to check this flag (skip or change the stop log and avoid touching server/server_thread) and clear the flag when appropriate so logs accurately reflect whether this instance actually started a server.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@auth/oauth_callback_server.py`:
- Around line 269-279: The instance sets self.is_running=True when reusing an
external listener but leaves self.server and self.server_thread None, causing
stop() to log "Minimal OAuth server stopped" incorrectly; add a boolean flag
(e.g., self._reusing_external_listener default False) and set it to True inside
the OSError handler where reuse is detected, then update stop() to check this
flag (skip or change the stop log and avoid touching server/server_thread) and
clear the flag when appropriate so logs accurately reflect whether this instance
actually started a server.
In `@tests/auth/test_oauth_callback_server.py`:
- Around line 145-203: Add explicit type hints to the two test functions and the
local _FakeSocket methods: change the test signatures to accept monkeypatch:
pytest.MonkeyPatch and return None, and annotate _FakeSocket as __init__(self,
*args: Any, **kwargs: Any) -> None, __enter__(self) -> " _FakeSocket" (or Any) ,
__exit__(self, exc_type: Optional[Type[BaseException]], exc:
Optional[BaseException], tb: Optional[TracebackType]) -> bool, and bind(self,
address: Tuple[str, int]) -> None; import the needed typing names (Any, Tuple,
Optional, Type, TracebackType) and pytest at top of the file so the tests
satisfy mypy strict mode.
In `@tests/test_main_permissions_tier.py`:
- Around line 53-80: Both new tests and their inner helpers need explicit type
hints: add return type -> None to
test_resolve_stdio_callback_port_marks_resolved_port and
test_resolve_callback_port_for_transport_skips_streamable_http, and annotate the
helper functions fake_resolve_port() and fail_if_called() with -> None and any
parameters (if present) with appropriate types; ensure monkeypatch argument in
the test signatures is typed (e.g., monkeypatch: pytest.MonkeyPatch) to satisfy
mypy strict mode and keep references to main.resolve_stdio_callback_port and
main.resolve_callback_port_for_transport unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8e73ba70-ab64-4325-a1c6-ea9d27918b7c
📒 Files selected for processing (8)
auth/oauth_callback_server.pyauth/oauth_config.pyauth/port_resolver.pycore/config.pymain.pytests/auth/test_oauth_callback_server.pytests/auth/test_port_resolver.pytests/test_main_permissions_tier.py
🚧 Files skipped from review as they are similar to previous changes (2)
- core/config.py
- tests/auth/test_port_resolver.py
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.
Late-bind OAuth callback port with preferred + fallback range
Summary
In stdio mode the OAuth callback server port is hardcoded to
8000. When a client (Claude Desktop, Claude Code, Cursor, etc.) spawns multipleworkspace-mcpprocesses — a normal occurrence on app reload, extension restart, or multi-session usage — only one wins the port. The others either error out at boot (Port 8000 already in use) or, worse, silently continue without a callback listener and produce 403 invalid-state on every subsequent OAuth attempt because the state-token PID and the listener PID diverge.This PR makes the callback port late-bound at process start: a preferred port plus a small fallback range is probed, the first available is bound, and
redirect_uriis composed from the actual bound port.Fixes
Approach (vs. existing PRs)
The trade-off depends on the OAuth client type:
Desktop application clients (the most common workspace-mcp setup, since stdio mode is a desktop-bound MCP transport) — no registration required. Google's loopback rule accepts any port on
localhost/127.0.0.1for Desktop clients automatically. Verified directly in GCP Console: Desktop client config pages don't even surface a redirect URI field. Zero operational cost; the fix is fully automatic.Web application clients (less common for workspace-mcp, used when the same OAuth client is shared with a hosted streamable-http deployment) — each fallback port must be registered as an authorized redirect URI in the OAuth client config. One-time setup. The README documents the entries.
Changes
auth/port_resolver.pyresolve_port()probes preferred + fallback range with connect-then-bind, mutates env to bound port.main.pyresolve_port()after argparse, thenreload_oauth_config()soredirect_urireflects the bound port.core/config.pyWORKSPACE_MCP_PORTis lazy-evaluated via PEP 562__getattr__so the late-bound env value isn't shadowed.auth/oauth_callback_server.py,auth/oauth_config.py, andauth/google_auth.pyare unchanged — they consumeport/base_url/redirect_urithrough the existing API and pick up the late-bound port automatically.Probe semantics
_is_port_free(host, port)does a two-stage check:127.0.0.1:port. Detects an existing listener regardless of which interface they bound to. On macOS,SO_REUSEADDR=1lets a0.0.0.0bind succeed even when127.0.0.1is already listening — so a bind probe alone gives a false-negative. Since the OAuth callback URI is alwayshttp://localhost:port, anything listening on loopback is a collision.(host, port)with defaultSO_REUSEADDR. CatchesTIME_WAITand BSD-style local conflicts.Configuration
WORKSPACE_MCP_PORT8000WORKSPACE_MCP_PORT_FALLBACK_COUNT4WORKSPACE_MCP_HOST0.0.0.0Set
WORKSPACE_MCP_PORT_FALLBACK_COUNT=0to restore the original single-port behavior.Test plan
unittestsuite covers: preferred-when-free, fallback-when-held, raise-when-all-held, OAuthConfig reload picks up the bound port viaredirect_uri,core.config.WORKSPACE_MCP_PORTlazy-evaluation.workspace-mcpprocess, a fresh instance correctly logsfalling back to 8001and binds 8001 with redirect_urihttp://localhost:8001/oauth2callback. Two simultaneous spawns shareWORKSPACE_MCP_PORT=8500and one bound 8500, the other 8501 — no errors, no orphan listeners.Notes
redirect_uri_mismatch. SetWORKSPACE_MCP_PORT_FALLBACK_COUNT=0to restore single-port behavior, or register the fallback URIs.WORKSPACE_MCP_PORT_FALLBACK_COUNT=0to keep the existing behavior.Summary by CodeRabbit
New Features
Bug Fixes
Tests