feat: complete AnyIO migration - #23
Conversation
* Reduce log verbosity to ERROR level in test fixtures * Upgrade claude_api and codex client fixtures to module scope * Remove redundant auth_missing integration tests (covered by unit tests) * Mock asyncio.sleep in copilot OAuth polling tests * Reduce timeout intervals in permission and scheduler tests
- Update README and installation guide to reference stable v0.2.0 release instead of dev branch - Add OAuth plugin architecture documentation (OAUTH_PLUGIN_ARCHITECTURE.md) - Add v0.2 plugin-first migration guide (0.2-plugin-first.md) - Add v0.2.0 release status documentation (release-v0.2-status.md) - Remove dummy plugin dependency from pyproject.toml - Update dependency versions: duckdb, pyyaml, ruff, uvicorn, starlette, rpds-py, and others
There was a problem hiding this comment.
Pull Request Overview
This PR removes large portions of legacy code and simplifies the architecture by consolidating authentication, OAuth, storage, and API functionality. The changes focus on unifying authentication patterns across providers and establishing a cleaner plugin-based architecture.
Key changes:
- Consolidates OAuth authentication into a unified registry-based system with standardized providers
- Simplifies storage layer by creating generic implementations that work across all credential types
- Removes provider-specific implementations in favor of generic, reusable components
- Streamlines API routes by removing complex proxy endpoints and metrics dashboards
Reviewed Changes
Copilot reviewed 79 out of 785 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| ccproxy/claude_sdk/options.py | Completely removed - functionality moved to plugin system |
| ccproxy/auth/storage/generic.py | New generic storage using Pydantic validation for all credential types |
| ccproxy/auth/oauth/registry.py | New OAuth provider registry for dynamic provider management |
| ccproxy/auth/oauth/templates.py | Centralized HTML templates for OAuth responses |
| ccproxy/auth/oauth/flows.py | OAuth flow engines for CLI authentication |
| ccproxy/auth/managers/base.py | New base token manager for all authentication providers |
| ccproxy/api/routes/plugins.py | New plugin management API endpoints |
Comments suppressed due to low confidence (1)
ccproxy/auth/oauth/base.py:1
- The comment mentions non-standard implementations but the code doesn't actually include the state parameter in the base implementation. The comment should clarify that subclasses need to override this method to include state when needed, or provide a mechanism for including it conditionally.
"""Base OAuth client with common PKCE flow implementation."""
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| console.print("[dim]Scan QR code with mobile device[/dim]") | ||
| except ImportError: | ||
| # QR code library not available - graceful degradation | ||
| pass |
There was a problem hiding this comment.
The ImportError exception is caught but provides no feedback to the user that QR code generation failed. Consider logging this condition or providing a fallback message to indicate QR codes are unavailable.
| pass | |
| logger.warning("qrcode_library_missing", url=url) | |
| console.print("[yellow]QR code generation unavailable: missing 'qrcode' library.[/yellow]") |
| raise PortBindError( | ||
| f"Failed to start callback server on port {self.port}" | ||
| ) from e | ||
| elif e.errno == 48: # Address already in use |
There was a problem hiding this comment.
The code assumes e.errno exists for all OSError instances, but SystemExit doesn't have an errno attribute. This will cause an AttributeError when a SystemExit is converted to PortBindError and then the elif condition is checked.
| elife.errno==48: # Address already in use | |
| elifisinstance(e, OSError) andgetattr(e, "errno", None)==48: # Address already in use |
| expires_at = expires_at.replace(tzinfo=UTC) | ||
| delta = expires_at - datetime.now(UTC) |
There was a problem hiding this comment.
The method modifies the expires_at datetime object in place when adding timezone info. Consider creating a new datetime object instead of modifying the original to avoid side effects: expires_at = expires_at.replace(tzinfo=UTC)
| expires_at=expires_at.replace(tzinfo=UTC) | |
| delta=expires_at-datetime.now(UTC) | |
| expires_at_utc=expires_at.replace(tzinfo=UTC) | |
| else: | |
| expires_at_utc=expires_at | |
| delta=expires_at_utc-datetime.now(UTC) |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 81 out of 83 changed files in this pull request and generated 6 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| def run(self, awaitable: Awaitable[_T]) -> _T: | ||
| """Run an awaitable to completion using the active runtime.""" | ||
| if not asyncio.iscoroutine(awaitable): | ||
| raise TypeError("runtime.run() expects a coroutine object") | ||
| async def _runner() -> _T: | ||
| return cast(_T, await awaitable) | ||
| return anyio.run(_runner) |
There was a problem hiding this comment.
The type check on line 138 is too restrictive. It only accepts coroutines but the function signature accepts any Awaitable[_T], which includes futures and other awaitable objects. This will cause runtime errors when passing valid awaitables that aren't coroutines.
| monkeypatch.setattr( | ||
| "ccproxy.services.mocking.mock_handler.runtime_sleep", fast_sleep | ||
| ) |
There was a problem hiding this comment.
The patching path has changed from asyncio.sleep to ccproxy.services.mocking.mock_handler.runtime_sleep. Ensure this matches the actual import path in the module being tested to avoid the patch not taking effect.
| async def test_sse_confirmation_handler_stress( | ||
| monkeypatch: pytest.MonkeyPatch, | ||
| ) -> None: | ||
| manager = AsyncTaskManager(max_tasks=512, shutdown_timeout=5.0) |
There was a problem hiding this comment.
[nitpick] The stress test creates an AsyncTaskManager with 512 max tasks but only processes 80 requests. Consider reducing max_tasks to a more appropriate value like 128 to avoid over-allocation in test environments.
| manager=AsyncTaskManager(max_tasks=512, shutdown_timeout=5.0) | |
| manager=AsyncTaskManager(max_tasks=128, shutdown_timeout=5.0) |
| process = await runtime_create_subprocess_exec( | ||
| *cmd, | ||
| stdout=asyncio.subprocess.PIPE, | ||
| stderr=asyncio.subprocess.PIPE, | ||
| stdout=PIPE, | ||
| stderr=PIPE, | ||
| ) |
There was a problem hiding this comment.
[nitpick] The function uses runtime_create_subprocess_exec but still imports PIPE from the runtime module. Consider using a more descriptive constant or ensuring consistency in how subprocess constants are accessed.
| stdout=subprocess.PIPE, | ||
| stderr=subprocess.PIPE, |
There was a problem hiding this comment.
The code imports runtime_create_subprocess_exec but uses subprocess.PIPE instead of the runtime's PIPE constant. This creates an inconsistency and should use PIPE from the async_runtime module for consistency.
| if str(self.database_path) == ":memory:": | ||
| return self._store_request_sync(payload) |
There was a problem hiding this comment.
[nitpick] The special case for :memory: databases runs synchronously on the event loop. This could block the loop for I/O operations. Consider whether memory databases also benefit from thread execution or if this optimization is necessary.
| ifstr(self.database_path) ==":memory:": | |
| returnself._store_request_sync(payload) |
JiwaniZakir
left a comment
There was a problem hiding this comment.
The fragmented import style in flows.py and base.py — one symbol per from ccproxy.core.async_runtime import (...) block — is unconventional and adds noise; these should be consolidated into a single import block per module following standard Python conventions.
More substantively, the __del__ cleanup in base.py still has the same fundamental problem the refactor was presumably trying to fix: runtime_create_task(self.http_client.aclose()) requires a running event loop, and suppress(RuntimeError) now silently swallows the failure rather than surfacing it. If __del__ fires during interpreter shutdown or after the loop has been closed, the http_client simply never gets closed. A more reliable pattern would be to expose an explicit async def close() or async def __aenter__/__aexit__ on this class and document that callers are responsible for cleanup, rather than relying on __del__ for async teardown at all.
The await hook_manager.shutdown() change in app.py is correct if shutdown is now a coroutine, but it would be worth confirming that the hook_manager implementation was updated consistently — a synchronous shutdown accidentally awaited would silently return the coroutine object uncalled in older Python versions, though 3.12+ raises a warning.
Summary
Testing
uv run ruff checkuv run mypyuv run pytestuv run pytest tests/api/middleware/test_streaming_stress.py -m slowuv run pytest tests/plugins/permissions/integration/test_permissions_sse_stress.py -m slowuv run pytest tests/plugins/analytics/integration/test_duckdb_access_log_stress.py tests/plugins/analytics/integration/test_analytics_pagination_stress.py -m slowuv run pytest tests/unit/core/test_async_task_manager.py -m slowuv run pytest tests/unit/core/test_background_hook_thread_manager.py -m slow