Add basedpyright check to MCP deploy workflow

[RafaelPo]

Summary

• Add basedpyright as a CI step in the checks job of deploy-mcp.yaml, matching the convention used in other deploy workflows (square, coster)
• Fix all 16 type errors in src/ so the check passes cleanly (0 errors, 0 warnings)
• Fix pyproject.toml basedpyright config: correct venvPath, add extraPaths, scope include to src/

Test plan

• CI checks pass (ruff + basedpyright + pytest all green)
• basedpyright reports 0 errors locally with uv run --directory everyrow-mcp basedpyright --project .
• All 227 tests pass, no regressions

🤖 Generated with Claude Code

[RafaelPo]

@claude code review

[github-actions]

Claude finished @RafaelPo's task in 1m 37s —— View job

---

Code Review: Add basedpyright check to MCP deploy workflow

• Gather context (read CLAUDE.md, diff, changed files)
• Review workflow changes
• Review pyproject.toml changes
• Review type-fix changes in src/
• Post review summary

---

Overall this is a clean PR — it adds basedpyright to CI and fixes all 16 type errors with minimal, correct changes. No functional regressions expected. A few observations below.

---

Workflow & Config

.github/workflows/deploy-mcp.yaml — The new basedpyright step slots in between ruff and pytest, matching the convention in other deploy workflows. Looks good.

everyrow-mcp/pyproject.toml — venvPath = ".." is correct since uv sync --directory everyrow-mcp resolves the venv from the repo root. extraPaths = ["src", "../src"] correctly exposes both the MCP package and the root SDK for import resolution. Dropping tests from include is fine — tests typically have looser typing.

---

Type Fixes

app.py / http_config.py — redis.ping() suppression (lines 53, 76, 107)
The # pyright: ignore[reportGeneralTypeIssues] on await redis_client.ping() is the standard workaround for redis-py's async stubs. Acceptable.

auth.py — assert client.client_id is not None (lines 349, 477, 525)
These narrow client_id: str | None → str before it's passed to methods that require str. The asserts are safe here since all three call sites are downstream of flows that already validated the client. One thing to keep in mind: assert is stripped under python -O. If you ever run with optimizations enabled, these would silently pass None. In this codebase that's unlikely to matter, but an alternative would be:

if client.client_id is None:
    raise ValueError("client_id is required")

Not blocking — just flagging for awareness.

auth.py — state default value (line 368)

state = request.path_params.get("state", "")

This is the right fix to satisfy the type checker (set_cookie value needs str, not str | None). The empty-string default is safe because the line above already validates state via _validate_auth_request, which raises HTTPException if state is missing. So state will always be a real value by the time set_cookie runs.

result_store.py — dict → dict[str, Any] (lines 30, 58, 60, 90, 246)
Correct generics for all the record/metadata dicts. Clean.

routes.py — set(_UI_EXCLUDE) (line 92)
_UI_EXCLUDE is defined as a frozenset (tool_helpers.py:135), but pydantic's model_dump(exclude=...) signature expects set. Wrapping in set() allocates a new set on every call to api_progress. Since this is a polling endpoint, consider changing the constant definition to a plain set instead:

_UI_EXCLUDE: set[str] = {"is_terminal", "is_screen", "task_type", "error", "started_at"}

This avoids the per-call allocation. Minor optimization — not blocking.

Fix this →

tools.py — UUID(session_id) (line 682)
get_session_url expects UUID but _fetch_task_result returns str. The explicit UUID() conversion is the correct fix.

---

Summary

Looks good to merge. The type fixes are all correct and minimal — no functional changes to runtime behavior. The only suggestion worth considering before merge is the _UI_EXCLUDE frozenset → set cleanup to avoid the per-call allocation in routes.py.
| Branch

[RafaelPo]

@claude code review

[github-actions]

Claude finished @RafaelPo's task in 1m 23s —— View job

---

Code Review: Add basedpyright check to MCP deploy workflow

• Gather context (read CLAUDE.md, diff, changed files)
• Review workflow and config changes
• Review type-fix changes in src/
• Review test changes
• Post review summary

---

Clean PR — adds basedpyright to CI and fixes all type errors with minimal, correct changes. No functional regressions expected. Details below.

---

Workflow & Config

.github/workflows/deploy-mcp.yaml — The basedpyright step slots between ruff and pytest, matching convention in other deploy workflows. Good.

everyrow-mcp/pyproject.toml — venvPath = ".." is correct for uv sync --directory everyrow-mcp. extraPaths = ["src", "../src"] correctly exposes both the MCP package and root SDK. include still lists both ["src", "tests"] — this is fine; tests are now typed too.

---

Source Fixes

app.py / http_config.py — redis.ping() suppression
# pyright: ignore[reportGeneralTypeIssues] is the standard workaround for redis-py's async stubs. Acceptable.

auth.py:349,477,525 — assert client.client_id is not None
These narrow client_id: str | None → str before it's passed onward. The asserts are safe since all call sites are downstream of validated auth flows. Note that assert is stripped under python -O — unlikely to matter here, but if you ever want belt-and-suspenders:

if client.client_id is None:
    raise ValueError("client_id is required")

Not blocking.

auth.py:368 — state default value

state = request.path_params.get("state", "")

Correct. _validate_auth_request on line 364 already raises HTTPException if state is missing, so the empty-string default is unreachable in practice but satisfies the type checker for set_cookie.

http_config.py:52,69 — auth_provider initialization
Initializing auth_provider: EveryRowAuthProvider | None = None and passing it directly to _register_routes is cleaner than the old auth_provider if not no_auth else None ternary. The previous code had auth_provider potentially unbound in the no_auth branch. Good fix.

result_store.py — dict → dict[str, Any]
Correct generic annotations across all 5 occurrences. Clean.

tool_helpers.py:135 — frozenset → set
This was frozenset before and now set. Since routes.py:92 previously called set(_UI_EXCLUDE) on every api_progress invocation, this avoids the per-call conversion. The previous review noted this and it's been addressed. Good.

tools.py:682 — UUID(session_id)
get_session_url expects UUID but _fetch_task_result returns str. Explicit conversion is the correct fix.

---

Test Fixes

conftest.py — AsyncGenerator return types for fixtures
Correct. Async generator fixtures returning AsyncGenerator[T, None] is the proper type annotation for yield-based pytest fixtures.

test_auth.py — AnyUrl() wrappers
OAuthClientInformationFull.redirect_uris and AuthorizationParams.redirect_uri expect AnyUrl, not bare strings. These changes align with the pydantic model definitions.

test_http_transport.py / test_mcp_e2e.py / test_stdio_content.py — isinstance(block, TextContent) guards
Replacing hasattr(b, "text") with isinstance(b, TextContent) is strictly better — it provides proper type narrowing and is more precise than duck-typing against an attribute. Consistent pattern across all test files.

test_routes.py — # pyright: ignore[reportArgumentType] and bytes(resp.body)
The FakeRequest class isn't a real Starlette Request, so pyright rightly flags the type mismatch. The ignore comments are appropriate for test fakes. The bytes(resp.body).decode() change handles the case where response.body might return a memoryview or similar — correct approach.

test_result_store.py / test_state_redis.py — assert x is not None before .text / json.loads
These are good — they serve as both type narrowing and runtime assertions that the test data is present.

test_server.py:1157 — # pyright: ignore[reportCallIssue]
This test intentionally passes an incomplete model (missing output_path) to verify validation errors. The suppress is correct — the whole point is to trigger the validation error.

---

Summary

Looks good to merge. All type fixes are correct and minimal with no functional changes to runtime behavior. The _UI_EXCLUDE suggestion from the previous review has been incorporated.

View job | Branch