fix: address PR review — stale mcp-oauth refs, Playwright TimeoutError catch
- Replace 4 stale mcp-oauth references in CLAUDE.md with mcp-login-flow - Import and catch playwright.async_api.TimeoutError in consent retry loop (Playwright's TimeoutError doesn't inherit from Python's built-in) - Replace unreachable `return True` with explicit RuntimeError raise - Add clarifying comment for hardcoded login-flow port default Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.6
parent
86f350ab49
commit
b4c3b48e61
@@ -198,7 +198,7 @@ uv run pytest tests/client/notes/test_notes_api.py -v
|
||||
|
||||
**Important**: After code changes, rebuild the correct container:
|
||||
- Single-user tests: `docker compose up --build -d mcp`
|
||||
- OAuth tests: `docker compose up --build -d mcp-oauth`
|
||||
- Login Flow tests: `docker compose up --build -d mcp-login-flow`
|
||||
- Keycloak tests: `docker compose up --build -d mcp-keycloak`
|
||||
|
||||
### Running the Server
|
||||
@@ -209,7 +209,7 @@ uv run mcp run --transport sse nextcloud_mcp_server.app:mcp
|
||||
|
||||
# Docker development (rebuilds after code changes)
|
||||
docker compose up --build -d mcp # Single-user (port 8000)
|
||||
docker compose up --build -d mcp-oauth # Nextcloud OAuth (port 8001)
|
||||
docker compose up --build -d mcp-login-flow # Login Flow v2 (port 8004)
|
||||
docker compose up --build -d mcp-keycloak # Keycloak OAuth (port 8002)
|
||||
```
|
||||
|
||||
@@ -443,7 +443,7 @@ async def nc_notes_semantic_search_answer(
|
||||
### Use Existing Fixtures
|
||||
See `tests/conftest.py` for 2888 lines of test infrastructure:
|
||||
- `nc_mcp_client` - MCP client for tool/resource testing (uses `mcp` container)
|
||||
- `nc_mcp_oauth_client` - MCP client for OAuth testing (uses `mcp-oauth` container)
|
||||
- `nc_mcp_oauth_client` - MCP client for OAuth testing (uses `mcp-login-flow` container)
|
||||
- `nc_client` - Direct NextcloudClient for setup/cleanup
|
||||
- `temporary_note`, `temporary_addressbook`, `temporary_contact` - Auto-cleanup
|
||||
|
||||
@@ -478,7 +478,7 @@ async def test_notes_api_get_note(mocker):
|
||||
OAuth tests use **Playwright browser automation** to complete flows programmatically.
|
||||
|
||||
**Test Environment**:
|
||||
- Three MCP containers: `mcp` (single-user), `mcp-oauth` (Nextcloud OIDC), `mcp-keycloak` (external IdP)
|
||||
- Three MCP containers: `mcp` (single-user), `mcp-login-flow` (Login Flow v2), `mcp-keycloak` (external IdP)
|
||||
- OAuth tests require `NEXTCLOUD_HOST`, `NEXTCLOUD_USERNAME`, `NEXTCLOUD_PASSWORD` environment variables
|
||||
- Playwright configuration: `--browser firefox --headed` for debugging
|
||||
- Install browsers: `uv run playwright install firefox`
|
||||
|
||||
+4
-3
@@ -21,6 +21,7 @@ from mcp import ClientSession
|
||||
from mcp.client.session import RequestContext
|
||||
from mcp.client.streamable_http import streamablehttp_client
|
||||
from mcp.types import ElicitRequestParams, ElicitResult, ErrorData
|
||||
from playwright.async_api import TimeoutError as PlaywrightTimeoutError
|
||||
|
||||
from nextcloud_mcp_server.client import NextcloudClient
|
||||
|
||||
@@ -2022,7 +2023,7 @@ async def _handle_oauth_consent_screen(page, username: str = "user"):
|
||||
)
|
||||
logger.info(f" Consent granted for {username}")
|
||||
return True
|
||||
except TimeoutError:
|
||||
except (TimeoutError, PlaywrightTimeoutError):
|
||||
if attempt == 2:
|
||||
screenshot_path = f"/tmp/consent_click_failed_{username}.png"
|
||||
await page.screenshot(path=screenshot_path)
|
||||
@@ -2035,7 +2036,7 @@ async def _handle_oauth_consent_screen(page, username: str = "user"):
|
||||
f" Consent click attempt {attempt + 1} didn't navigate, retrying..."
|
||||
)
|
||||
|
||||
return True # unreachable but satisfies type checker
|
||||
raise RuntimeError("consent click retry loop exited unexpectedly")
|
||||
else:
|
||||
logger.error(f" Allow button not found for {username}")
|
||||
return False
|
||||
@@ -2051,7 +2052,7 @@ async def _get_oauth_token_with_scopes(
|
||||
oauth_callback_server,
|
||||
scopes: str,
|
||||
resource: str | None = None,
|
||||
mcp_server_base_url: str = "http://localhost:8004",
|
||||
mcp_server_base_url: str = "http://localhost:8004", # login-flow container port
|
||||
) -> str:
|
||||
"""
|
||||
Helper function to obtain OAuth token with specific scopes.
|
||||
|
||||
Reference in New Issue
Block a user