fix(api): replace JWT query parameter with one-time ticket for WebSocket auth#493
fix(api): replace JWT query parameter with one-time ticket for WebSocket auth#493
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📜 Recent review details⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
🧰 Additional context used📓 Path-based instructions (4)web/src/**/*.{ts,tsx,vue}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
web/**/*.{ts,tsx,vue}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
docs/**/*.md📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/*.md📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (1)📚 Learning: 2026-03-16T20:52:02.789ZApplied to files:
🧬 Code graph analysis (1)web/src/composables/useWebSocketSubscription.ts (1)
🔇 Additional comments (4)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds one‑time WebSocket ticket auth: backend POST /api/v1/auth/ws-ticket, an in‑memory WsTicketStore (single‑use, TTL, per‑user cap), ticket validation/consumption in /ws, periodic cleanup task and middleware exclusion for /ws, plus frontend changes to fetch a ticket and connect to /ws?ticket=... instead of sending JWT in URL. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Frontend as Frontend App
participant AuthAPI as Auth Endpoint
participant TicketStore as Ticket Store
participant WSHandler as WebSocket Handler
User->>Frontend: Open app (has JWT)
Frontend->>AuthAPI: POST /api/v1/auth/ws-ticket\n(Authorization: Bearer JWT)
AuthAPI->>TicketStore: create(user)
TicketStore-->>TicketStore: generate token, set expires_at = now + TTL
TicketStore->>AuthAPI: return ticket
AuthAPI-->>Frontend: { ticket, expires_in }
Frontend->>WSHandler: CONNECT /ws?ticket=...
WSHandler->>TicketStore: validate_and_consume(ticket)
alt ticket valid & not expired
TicketStore-->>TicketStore: remove ticket (single-use)
TicketStore->>WSHandler: return AuthenticatedUser
WSHandler-->>Frontend: 101 Upgrade (attach user to scope)
else invalid/expired/consumed
TicketStore->>WSHandler: return None
WSHandler-->>Frontend: Close (auth failed / forbidden)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
✨ Simplify code
📝 Coding Plan
Comment |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly upgrades the security posture of WebSocket connections by transitioning from direct JWT usage in URLs to a robust, ticket-based authentication system. This change mitigates several security risks associated with exposing long-lived credentials, while also introducing a dedicated API for ticket issuance and a resilient backend mechanism for managing ticket lifecycles. The frontend has been adapted to seamlessly integrate with this enhanced authentication flow, ensuring a secure and reliable real-time experience. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #493 +/- ##
=======================================
Coverage ? 93.06%
=======================================
Files ? 501
Lines ? 24098
Branches ? 2298
=======================================
Hits ? 22426
Misses ? 1327
Partials ? 345 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Code Review
This is an excellent pull request that significantly improves security by replacing JWTs in query parameters with a one-time ticket system for WebSocket authentication. The backend implementation, including the WsTicketStore and the background cleanup task, is robust and well-designed. The frontend changes are also of high quality, with the WebSocket store refactored to handle the new ticket exchange flow gracefully, including race condition prevention and resilient reconnect logic. The tests are comprehensive and cover the new functionality well. I have one suggestion to improve configurability.
src/synthorg/api/app.py
Outdated
| async def _ticket_cleanup_loop() -> None: | ||
| """Periodically prune expired WS tickets.""" | ||
| while True: | ||
| await asyncio.sleep(60) |
There was a problem hiding this comment.
The ticket cleanup loop uses a hardcoded sleep interval of 60 seconds. While this is reasonable for the default 30-second ticket TTL, it would be more robust to make this interval configurable, perhaps relating it to the ticket TTL itself. This would prevent expired tickets from accumulating for too long if the TTL is significantly shortened in the future.
I recommend making both the ticket TTL (currently defaulted in WsTicketStore) and this cleanup interval configurable, for instance via ApiConfig.
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/synthorg/api/app.py (1)
634-645:⚠️ Potential issue | 🟠 MajorAlways merge the WS upgrade path into auth exclusions.
/api/v1/wsis only excluded whenauth.exclude_pathsisNone. Any deployment that already setsexclude_pathswill still run auth middleware on the WebSocket upgrade, and browser upgrades cannot carry theAuthorizationheader that middleware expects. That breaks ticket-authenticated connections as soon as auth exclusions are customized.Suggested fix
- if auth.exclude_paths is None: - prefix = api_config.api_prefix - auth = auth.model_copy( - update={ - "exclude_paths": ( - f"^{prefix}/health$", - "^/docs", - "^/api$", - f"^{prefix}/auth/setup$", - f"^{prefix}/auth/login$", - f"^{prefix}/ws$", - ), - }, - ) + prefix = api_config.api_prefix + exclude_paths = auth.exclude_paths or ( + f"^{prefix}/health$", + "^/docs", + "^/api$", + f"^{prefix}/auth/setup$", + f"^{prefix}/auth/login$", + ) + ws_path = f"^{prefix}/ws$" + if ws_path not in exclude_paths: + auth = auth.model_copy( + update={"exclude_paths": (*exclude_paths, ws_path)}, + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/synthorg/api/app.py` around lines 634 - 645, The auth.exclude_paths logic must always include the WebSocket upgrade path; update the code that handles the case when auth.exclude_paths is already set so it merges/ensures the pattern for the WS upgrade (use the same pattern f"^{api_config.api_prefix}/ws$" or equivalent) into the existing exclude_paths instead of leaving it unchanged. Locate the auth.model_copy usage and adjust both branches: when auth.exclude_paths is None build the tuple as shown, and when it's not None compute a new exclude_paths that appends or inserts the WS pattern (avoiding duplicates) and pass that into auth.model_copy(update={"exclude_paths": new_exclude_paths}) so the WS upgrade is always excluded from auth middleware.src/synthorg/api/controllers/ws.py (1)
105-184: 🛠️ Refactor suggestion | 🟠 MajorSplit
ws_handlerinto smaller helpers to meet function-size limits.This now handles multiple responsibilities (auth result handling, socket lifecycle, channel subscriber wiring, and teardown) in one block.
As per coding guidelines, "Keep functions under 50 lines and files under 800 lines".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/synthorg/api/controllers/ws.py` around lines 105 - 184, ws_handler is too large; split responsibilities into smaller helpers: extract authentication and socket accept logic into a helper (e.g., authenticate_and_accept(socket) that calls _authenticate_ws and sets socket.scope["user"] and accepts the socket), move the event processing callback out of ws_handler into a top-level function (rename _on_event to event_forwarder(event_data, socket, subscribed, filters) and keep its JSON parsing, filtering and send logic), create a helper to create and manage the channels subscription (e.g., create_subscriber(channels_plugin) that returns subscriber and wraps subscribe/unsubscribe), and keep ws_handler as a thin orchestrator that calls authenticate_and_accept, create_subscriber, uses subscriber.run_in_background(event_forwarder) and awaits _receive_loop; finally move the unsubscribe and logging teardown into a small teardown helper to keep ws_handler under 50 lines and focused on orchestration while preserving existing symbols like _receive_loop, channels_plugin.subscribe, subscriber.run_in_background and WebSocket handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/synthorg/api/auth/controller.py`:
- Around line 443-446: The unauthorized branch that checks auth_user (auth_user
= request.scope.get("user")) must log the failed ticket-issuance attempt using
API_AUTH_FAILED with context before raising UnauthorizedError; update the block
where you currently raise UnauthorizedError(msg) to call API_AUTH_FAILED
(passing the request and a reason/context such as "ticket issuance - no
authenticated user" and any identifying fields available) at WARNING or ERROR
level, then raise UnauthorizedError(msg) as before so the failure is recorded in
the API auth log stream.
In `@src/synthorg/api/auth/ticket_store.py`:
- Around line 149-153: The cleanup log currently uses logger.debug for the state
transition; change the call in the cleanup routine that references
API_WS_TICKET_CLEANUP and the removed/remaining kwargs (using expired and
self._tickets) to logger.info so the mutation is recorded at INFO level, keeping
structured kwargs (removed=len(expired), remaining=len(self._tickets)) and no
%-formatting.
- Around line 65-69: The __init__ of the ticket store currently only checks
ttl_seconds <= 0 and thus allows non-finite values; update the validation in
__init__ to also reject non-finite values (NaN or +/-inf) by checking
math.isfinite(ttl_seconds) (or an equivalent) and raising ValueError with a
clear message if not finite, before assigning self._ttl = ttl_seconds.
In `@tests/unit/api/auth/test_ticket_store.py`:
- Around line 83-92: Add a new test that races concurrent consumers against a
single-use ticket to ensure only one caller receives the bound user: create a
WsTicketStore, user via _make_user(), and ticket via store.create(), then spawn
multiple concurrent callers (e.g., threads with threading.Barrier or asyncio
tasks) that simultaneously call store.validate_and_consume(ticket); collect
results and assert exactly one result is non-None (and equals the expected user)
while all others are None. Use the existing test_validate_and_consume_single_use
as a template and reference WsTicketStore, validate_and_consume, create, and
_make_user to locate where to add the concurrent test. Ensure the concurrency
mechanism reliably starts all consumers at the same time to avoid serial
ordering.
In `@tests/unit/api/controllers/test_ws.py`:
- Around line 212-213: The test class TestWsTicketAuth lacks a per-test timeout
marker; add a class-level pytest timeout marker (pytest.mark.timeout(30))
applied to the TestWsTicketAuth class so every test in that class gets a
30-second timeout per the test policy.
In `@web/src/__tests__/composables/useWebSocketSubscription.test.ts`:
- Around line 13-18: The test harness currently swallows Promise rejections by
assigning mountedPromise = result.catch(() => {}) inside the onMounted mock
(vi.fn(...) that invokes cb), which hides setup failures; change the mock so it
does not catch errors—assign mountedPromise = result (or await/return the
promise) so rejections propagate to the test runner, and similarly update the
second onMounted mock block (the one around the later lines) to remove the
.catch(() => {}) swallow; keep the vi.fn wrapper and mountedPromise usage but
allow the promise to reject so tests fail on real errors.
In `@web/src/__tests__/stores/websocket.test.ts`:
- Around line 96-104: The test currently exercises the fast-path because the
first store.connect() resolves before the second call; change it to simulate
concurrent callers by making getWsTicket() return a controllable pending
promise, call store.connect() twice before resolving that promise (so both enter
connect() while ticket is unresolved), then resolve the ticket and advance
timers, and finally assert mockInstances has length 1 and no duplicate WebSocket
was created; reference useWebSocketStore, connect, getWsTicket, mockInstances
and vi.advanceTimersByTimeAsync to locate and modify the test scaffolding.
In `@web/src/stores/websocket.ts`:
- Around line 29-30: The in-flight connect attempt can continue after
disconnect() runs because connect() awaits getWsTicket() and then proceeds; fix
by making doConnect/connect() check a cancellation token or snapshot of
shouldBeConnected immediately after each await (e.g., capture a local
shouldConnectAtStart = shouldBeConnected or increment a connectAttemptId and
store it locally) and abort if it changed so stale attempts don't create a
WebSocket or flip connected; also ensure connectPromise is set to the current
attempt and cleared on abort/finish (and that disconnect() increments the
token/attempt id or flips shouldBeConnected and clears/awaits/negates any
in-flight connectPromise) so abandoned attempts cannot leave connectPromise tied
to an old exchange or resurrect connected.
---
Outside diff comments:
In `@src/synthorg/api/app.py`:
- Around line 634-645: The auth.exclude_paths logic must always include the
WebSocket upgrade path; update the code that handles the case when
auth.exclude_paths is already set so it merges/ensures the pattern for the WS
upgrade (use the same pattern f"^{api_config.api_prefix}/ws$" or equivalent)
into the existing exclude_paths instead of leaving it unchanged. Locate the
auth.model_copy usage and adjust both branches: when auth.exclude_paths is None
build the tuple as shown, and when it's not None compute a new exclude_paths
that appends or inserts the WS pattern (avoiding duplicates) and pass that into
auth.model_copy(update={"exclude_paths": new_exclude_paths}) so the WS upgrade
is always excluded from auth middleware.
In `@src/synthorg/api/controllers/ws.py`:
- Around line 105-184: ws_handler is too large; split responsibilities into
smaller helpers: extract authentication and socket accept logic into a helper
(e.g., authenticate_and_accept(socket) that calls _authenticate_ws and sets
socket.scope["user"] and accepts the socket), move the event processing callback
out of ws_handler into a top-level function (rename _on_event to
event_forwarder(event_data, socket, subscribed, filters) and keep its JSON
parsing, filtering and send logic), create a helper to create and manage the
channels subscription (e.g., create_subscriber(channels_plugin) that returns
subscriber and wraps subscribe/unsubscribe), and keep ws_handler as a thin
orchestrator that calls authenticate_and_accept, create_subscriber, uses
subscriber.run_in_background(event_forwarder) and awaits _receive_loop; finally
move the unsubscribe and logging teardown into a small teardown helper to keep
ws_handler under 50 lines and focused on orchestration while preserving existing
symbols like _receive_loop, channels_plugin.subscribe,
subscriber.run_in_background and WebSocket handling.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 25e971cb-608d-4663-979b-95309ea96101
📒 Files selected for processing (20)
CLAUDE.mddocs/design/operations.mdsrc/synthorg/api/app.pysrc/synthorg/api/auth/__init__.pysrc/synthorg/api/auth/controller.pysrc/synthorg/api/auth/models.pysrc/synthorg/api/auth/ticket_store.pysrc/synthorg/api/controllers/ws.pysrc/synthorg/api/state.pysrc/synthorg/observability/events/api.pytests/unit/api/auth/test_controller.pytests/unit/api/auth/test_ticket_store.pytests/unit/api/controllers/test_ws.pyweb/src/__tests__/composables/useWebSocketSubscription.test.tsweb/src/__tests__/stores/websocket.test.tsweb/src/api/endpoints/auth.tsweb/src/api/types.tsweb/src/composables/useWebSocketSubscription.tsweb/src/stores/websocket.tsweb/src/views/MeetingLogsPage.vue
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Deploy Preview
- GitHub Check: Test (Python 3.14)
- GitHub Check: Build Web
- GitHub Check: Build Sandbox
- GitHub Check: Build Backend
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Do not usefrom __future__ import annotations— Python 3.14 has PEP 649 native lazy annotations
Useexcept A, B:syntax (no parentheses) per PEP 758 — ruff enforces this on Python 3.14
Add type hints to all public functions and classes; enforce mypy strict mode
Use Google-style docstrings on all public classes and functions — required and enforced by ruff D rules
Keep line length to 88 characters (ruff enforced)
Files:
src/synthorg/api/auth/models.pysrc/synthorg/api/app.pysrc/synthorg/api/state.pysrc/synthorg/api/auth/controller.pysrc/synthorg/api/auth/__init__.pysrc/synthorg/observability/events/api.pytests/unit/api/auth/test_ticket_store.pytests/unit/api/auth/test_controller.pysrc/synthorg/api/auth/ticket_store.pytests/unit/api/controllers/test_ws.pysrc/synthorg/api/controllers/ws.py
src/synthorg/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/synthorg/**/*.py: Create new objects for immutability instead of mutating existing ones; for non-Pydantic internal collections (registries,BaseTool), usecopy.deepcopy()at construction andMappingProxyTypewrapping for read-only enforcement
Fordict/listfields in frozen Pydantic models, usecopy.deepcopy()at system boundaries (tool execution, LLM provider serialization, inter-agent delegation, persistence serialization)
Use frozen Pydantic models for config/identity; use separate mutable-via-copy models (usingmodel_copy(update=...)) for runtime state that evolves; never mix static config fields with mutable runtime fields in one model
Use Pydantic v2 (BaseModel,model_validator,computed_field,ConfigDict); use@computed_fieldfor derived values instead of storing redundant fields; useNotBlankStrfor all identifier/name fields
Preferasyncio.TaskGroupfor fan-out/fan-in parallel operations in new code (multiple tool invocations, parallel agent calls); prefer structured concurrency over barecreate_task
Keep functions under 50 lines and files under 800 lines
Handle errors explicitly, never silently swallow exceptions
Validate at system boundaries (user input, external APIs, config files)
Every module with business logic must importfrom synthorg.observability import get_loggerand createlogger = get_logger(__name__); never useimport loggingorlogging.getLogger()orprint()in application code
Always use event name constants from domain-specific modules undersynthorg.observability.events(e.g.,PROVIDER_CALL_STARTfromevents.provider,BUDGET_RECORD_ADDEDfromevents.budget); import directly:from synthorg.observability.events.<domain> import EVENT_CONSTANT
Log all state transitions at INFO level with structured kwargs (never use % formatting); log all error paths at WARNING or ERROR with context before raising; log object creation and key function entry/exit at DEBUG
Pure data models, enums, and re-e...
Files:
src/synthorg/api/auth/models.pysrc/synthorg/api/app.pysrc/synthorg/api/state.pysrc/synthorg/api/auth/controller.pysrc/synthorg/api/auth/__init__.pysrc/synthorg/observability/events/api.pysrc/synthorg/api/auth/ticket_store.pysrc/synthorg/api/controllers/ws.py
web/src/**/*.{vue,ts,tsx,js}
📄 CodeRabbit inference engine (CLAUDE.md)
Lint frontend code with ESLint; type-check with vue-tsc; test with Vitest; audit with npm audit (critical + high severity)
Files:
web/src/__tests__/stores/websocket.test.tsweb/src/__tests__/composables/useWebSocketSubscription.test.tsweb/src/api/types.tsweb/src/api/endpoints/auth.tsweb/src/views/MeetingLogsPage.vueweb/src/stores/websocket.tsweb/src/composables/useWebSocketSubscription.ts
docs/design/**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
Update the relevant
docs/design/page to reflect approved deviations from the spec
Files:
docs/design/operations.md
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Mark unit tests with@pytest.mark.unit, integration tests with@pytest.mark.integration, e2e tests with@pytest.mark.e2e, and slow tests with@pytest.mark.slow
Useasyncio_mode = "auto"in pytest configuration — no manual@pytest.mark.asyncioneeded on async tests
Set 30-second timeout per test
Prefer@pytest.mark.parametrizefor testing similar cases
Use Hypothesis for property-based testing in Python (@given+@settings); useciprofile (200 examples, default) anddevprofile (1000 examples) controlled viaHYPOTHESIS_PROFILEenv var
Never skip, dismiss, or ignore flaky tests — always fix them fully and fundamentally; for timing-sensitive tests, mocktime.monotonic()andasyncio.sleep()to make them deterministic
Files:
tests/unit/api/auth/test_ticket_store.pytests/unit/api/auth/test_controller.pytests/unit/api/controllers/test_ws.py
🧠 Learnings (13)
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to web/** : Web dashboard: Node.js 20+, dependencies in web/package.json (Vue 3, PrimeVue, Tailwind CSS, Pinia, VueFlow, ECharts, Axios, vue-draggable-plus, Vitest, fast-check, ESLint, vue-tsc).
Applied to files:
web/src/views/MeetingLogsPage.vue
📚 Learning: 2026-03-16T07:36:16.026Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T07:36:16.026Z
Learning: Applies to src/synthorg/**/*.py : Use Pydantic v2 (`BaseModel`, `model_validator`, `computed_field`, `ConfigDict`); use `computed_field` for derived values instead of storing redundant fields; use `NotBlankStr` for all identifier/name fields
Applied to files:
src/synthorg/api/auth/controller.py
📚 Learning: 2026-03-16T07:36:16.026Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T07:36:16.026Z
Learning: Applies to src/synthorg/**/*.py : Use frozen Pydantic models for config/identity; use separate mutable-via-copy models (using `model_copy(update=...)`) for runtime state that evolves; never mix static config fields with mutable runtime fields in one model
Applied to files:
src/synthorg/api/auth/controller.py
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to src/synthorg/**/*.py : Every module with business logic MUST have: `from synthorg.observability import get_logger` then `logger = get_logger(__name__)`. Never use `import logging` / `logging.getLogger()` / `print()` in application code. Variable name: always `logger` (not `_logger`, not `log`).
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to src/synthorg/**/*.py : Structured kwargs in logging: always `logger.info(EVENT, key=value)` — never `logger.info('msg %s', val)`.
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-16T07:36:16.026Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T07:36:16.026Z
Learning: Applies to src/synthorg/**/*.py : Every module with business logic must import `from synthorg.observability import get_logger` and create `logger = get_logger(__name__)`; never use `import logging` or `logging.getLogger()` or `print()` in application code
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-16T07:36:16.026Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T07:36:16.026Z
Learning: Applies to src/synthorg/**/*.py : Log all state transitions at INFO level with structured kwargs (never use % formatting); log all error paths at WARNING or ERROR with context before raising; log object creation and key function entry/exit at DEBUG
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to src/synthorg/**/*.py : Event names: always use constants from domain-specific modules under synthorg.observability.events (e.g., PROVIDER_CALL_START from events.provider, BUDGET_RECORD_ADDED from events.budget, etc.). Import directly: `from synthorg.observability.events.<domain> import EVENT_CONSTANT`.
Applied to files:
CLAUDE.mdsrc/synthorg/observability/events/api.py
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to src/synthorg/**/*.py : All error paths must log at WARNING or ERROR with context before raising.
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-16T07:36:16.026Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T07:36:16.026Z
Learning: Applies to src/synthorg/**/*.py : Always use event name constants from domain-specific modules under `synthorg.observability.events` (e.g., `PROVIDER_CALL_START` from `events.provider`, `BUDGET_RECORD_ADDED` from `events.budget`); import directly: `from synthorg.observability.events.<domain> import EVENT_CONSTANT`
Applied to files:
CLAUDE.mdsrc/synthorg/observability/events/api.py
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to src/synthorg/**/*.py : All state transitions must log at INFO.
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to src/synthorg/**/*.py : DEBUG logging for object creation, internal flow, entry/exit of key functions.
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-16T07:36:16.026Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T07:36:16.026Z
Learning: Applies to src/synthorg/**/*.py : Pure data models, enums, and re-exports do NOT need logging
Applied to files:
CLAUDE.md
🧬 Code graph analysis (12)
web/src/__tests__/composables/useWebSocketSubscription.test.ts (2)
web/src/api/types.ts (1)
WsEventHandler(687-687)web/src/composables/useWebSocketSubscription.ts (1)
useWebSocketSubscription(47-105)
web/src/api/types.ts (1)
src/synthorg/api/auth/controller.py (1)
WsTicketResponse(154-165)
web/src/api/endpoints/auth.ts (3)
src/synthorg/api/auth/controller.py (1)
WsTicketResponse(154-165)web/src/api/types.ts (2)
WsTicketResponse(164-167)ApiResponse(125-127)web/src/api/client.ts (2)
apiClient(12-16)unwrap(70-80)
web/src/stores/websocket.ts (2)
web/src/api/endpoints/auth.ts (1)
getWsTicket(32-35)web/src/utils/logging.ts (1)
sanitizeForLog(2-11)
src/synthorg/api/state.py (1)
src/synthorg/api/auth/ticket_store.py (1)
WsTicketStore(51-154)
src/synthorg/api/auth/controller.py (5)
src/synthorg/api/auth/models.py (3)
AuthenticatedUser(69-88)AuthMethod(11-16)User(19-40)web/src/api/types.ts (2)
WsTicketResponse(164-167)ApiResponse(125-127)src/synthorg/api/errors.py (1)
UnauthorizedError(171-179)src/synthorg/api/state.py (1)
ticket_store(229-231)src/synthorg/api/auth/ticket_store.py (2)
create(77-98)ttl_seconds(73-75)
src/synthorg/api/auth/__init__.py (2)
src/synthorg/api/state.py (1)
ticket_store(229-231)src/synthorg/api/auth/ticket_store.py (1)
WsTicketStore(51-154)
tests/unit/api/auth/test_ticket_store.py (4)
src/synthorg/api/auth/models.py (2)
AuthenticatedUser(69-88)AuthMethod(11-16)src/synthorg/api/state.py (1)
ticket_store(229-231)src/synthorg/api/auth/ticket_store.py (5)
WsTicketStore(51-154)create(77-98)ttl_seconds(73-75)validate_and_consume(100-132)cleanup_expired(134-154)web/src/api/types.ts (1)
HumanRole(46-51)
tests/unit/api/auth/test_controller.py (4)
tests/unit/api/conftest.py (2)
test_client(179-203)make_auth_headers(97-123)tests/unit/api/test_guards.py (1)
bare_client(17-20)src/synthorg/api/state.py (1)
ticket_store(229-231)src/synthorg/api/auth/ticket_store.py (1)
validate_and_consume(100-132)
src/synthorg/api/auth/ticket_store.py (1)
src/synthorg/api/auth/models.py (1)
AuthenticatedUser(69-88)
tests/unit/api/controllers/test_ws.py (6)
src/synthorg/api/auth/models.py (1)
AuthMethod(11-16)src/synthorg/api/controllers/ws.py (1)
_handle_message(207-277)web/src/api/types.ts (1)
HumanRole(46-51)tests/unit/api/conftest.py (1)
test_client(179-203)src/synthorg/api/state.py (1)
ticket_store(229-231)src/synthorg/api/auth/ticket_store.py (1)
validate_and_consume(100-132)
src/synthorg/api/controllers/ws.py (3)
src/synthorg/api/auth/models.py (1)
AuthenticatedUser(69-88)src/synthorg/api/state.py (1)
ticket_store(229-231)src/synthorg/api/auth/ticket_store.py (1)
validate_and_consume(100-132)
🔇 Additional comments (13)
src/synthorg/api/auth/__init__.py (1)
6-6: LGTM!The
WsTicketStoreis correctly imported and exported as part of the auth package's public API.Also applies to: 15-15
src/synthorg/observability/events/api.py (1)
50-54: LGTM!The five new event constants follow the established naming convention and cover the complete WS ticket lifecycle (issuance, consumption, expiration, invalidation, cleanup). Based on learnings: "Always use event name constants from domain-specific modules under
synthorg.observability.events" — these constants are correctly placed in theapi.pyevents module.src/synthorg/api/auth/models.py (1)
16-16: LGTM!The
WS_TICKETenum value correctly extendsAuthMethodto support the new ticket-based WebSocket authentication flow.web/src/api/endpoints/auth.ts (1)
31-35: LGTM!The
getWsTicket()endpoint follows the established pattern: POST request (appropriate for ticket creation), uses the standardapiClientandunwrappattern, and correctly types the response. The JWT is passed via the Authorization header (handled byapiClientinterceptors), so no request body is needed.web/src/composables/useWebSocketSubscription.ts (1)
56-61: LGTM!The
onMountedcallback is correctly converted to async to await the connection before subscribing. The try/catch block properly handles connection failures, and the flow ensures subscriptions only happen after a successful connection. The token is now managed internally bywsStore.connect()(which fetches the WS ticket), aligning with the new ticket-based auth flow.web/src/api/types.ts (1)
164-167: LGTM!The
WsTicketResponseinterface correctly mirrors the backend Pydantic model (ticket: NotBlankStr,expires_in: intwithgt=0constraint). Placement afterTokenResponseis logical given the similar structure.docs/design/operations.md (1)
963-963: LGTM!Documentation correctly updated to reflect the new WebSocket ticket authentication flow:
/api/v1/authnow mentionsws-ticket/api/v1/wsdocuments the?ticket=query parameter- New
POST /api/v1/auth/ws-ticketendpoint is documentedAs per coding guidelines: "Update the relevant
docs/design/page to reflect approved deviations from the spec" — this change properly documents the new auth mechanism.Also applies to: 977-978
src/synthorg/api/state.py (1)
10-10: LGTM!The
WsTicketStoreintegration follows the established pattern for always-available services:
- Added to
__slots__for memory efficiency- Instantiated unconditionally in
__init__(unlike optional services)- Property returns directly without
_require_servicecheck, matching the docstring "always available"The default 30-second TTL aligns with PR objectives. If TTL configurability is needed later, it can be passed from
configat construction time.Also applies to: 60-60, 97-97, 228-231
web/src/views/MeetingLogsPage.vue (1)
26-28: Nice simplification with the shared subscription composable.This removes page-local socket lifecycle code and keeps meeting updates on the same reconnect path as the rest of the dashboard.
web/src/__tests__/composables/useWebSocketSubscription.test.ts (1)
72-83: Async post-mount assertions are synchronized correctly.Converting these cases to await
mountedPromiseis a solid fix for timing flakiness after asynconMountedsetup.Also applies to: 113-129, 131-145, 147-165, 167-189, 250-269, 271-286, 288-300
src/synthorg/api/auth/ticket_store.py (1)
100-133: Single-use consume flow looks correct.Using
popbefore expiry checks enforces one-time usage and cleanly handles invalid tickets.tests/unit/api/controllers/test_ws.py (1)
233-273: Ticket-auth behavior coverage is strong here.Auth method mapping, single-use semantics, and identity preservation are all validated.
src/synthorg/api/controllers/ws.py (1)
51-103: Pre-accept ticket auth flow and close-code handling look solid.Authenticating/consuming before
accept()and using dedicated 4001/4003 codes aligns well with the security objective.
| it('does not create duplicate connections', async () => { | ||
| const store = useWebSocketStore() | ||
| store.connect('test-token') | ||
| await store.connect() | ||
| await vi.advanceTimersByTimeAsync(0) | ||
| expect(mockInstances).toHaveLength(1) | ||
|
|
||
| store.connect('test-token') // should be no-op | ||
| await store.connect() // should be no-op | ||
| expect(mockInstances).toHaveLength(1) // no new WebSocket created | ||
| }) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Make this a true concurrent-connect regression.
The second connect() runs after the first one has already resolved, so this only covers the ready-state fast path. The new connectPromise guard matters when two callers enter connect() before getWsTicket() resolves.
Example test shape
it('does not create duplicate connections', async () => {
const store = useWebSocketStore()
- await store.connect()
- await vi.advanceTimersByTimeAsync(0)
- expect(mockInstances).toHaveLength(1)
-
- await store.connect() // should be no-op
- expect(mockInstances).toHaveLength(1) // no new WebSocket created
+ let resolveTicket!: (value: { ticket: string; expires_in: number }) => void
+ vi.mocked(getWsTicket).mockReturnValueOnce(
+ new Promise((resolve) => {
+ resolveTicket = resolve
+ }),
+ )
+
+ const first = store.connect()
+ const second = store.connect()
+
+ expect(getWsTicket).toHaveBeenCalledTimes(1)
+
+ resolveTicket({ ticket: 'test-ticket-abc', expires_in: 30 })
+ await Promise.all([first, second])
+ await vi.advanceTimersByTimeAsync(0)
+
+ expect(mockInstances).toHaveLength(1)
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('does not create duplicate connections', async () => { | |
| const store = useWebSocketStore() | |
| store.connect('test-token') | |
| await store.connect() | |
| await vi.advanceTimersByTimeAsync(0) | |
| expect(mockInstances).toHaveLength(1) | |
| store.connect('test-token') // should be no-op | |
| await store.connect() // should be no-op | |
| expect(mockInstances).toHaveLength(1) // no new WebSocket created | |
| }) | |
| it('does not create duplicate connections', async () => { | |
| const store = useWebSocketStore() | |
| let resolveTicket!: (value: { ticket: string; expires_in: number }) => void | |
| vi.mocked(getWsTicket).mockReturnValueOnce( | |
| new Promise((resolve) => { | |
| resolveTicket = resolve | |
| }), | |
| ) | |
| const first = store.connect() | |
| const second = store.connect() | |
| expect(getWsTicket).toHaveBeenCalledTimes(1) | |
| resolveTicket({ ticket: 'test-ticket-abc', expires_in: 30 }) | |
| await Promise.all([first, second]) | |
| await vi.advanceTimersByTimeAsync(0) | |
| expect(mockInstances).toHaveLength(1) | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/__tests__/stores/websocket.test.ts` around lines 96 - 104, The test
currently exercises the fast-path because the first store.connect() resolves
before the second call; change it to simulate concurrent callers by making
getWsTicket() return a controllable pending promise, call store.connect() twice
before resolving that promise (so both enter connect() while ticket is
unresolved), then resolve the ticket and advance timers, and finally assert
mockInstances has length 1 and no duplicate WebSocket was created; reference
useWebSocketStore, connect, getWsTicket, mockInstances and
vi.advanceTimersByTimeAsync to locate and modify the test scaffolding.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/synthorg/api/controllers/ws.py (1)
239-290: 🛠️ Refactor suggestion | 🟠 MajorSplit
_handle_messageinto smaller helpers.This dispatcher now crosses the 50-line limit and is carrying parse, validate, and action-dispatch concerns together.
As per coding guidelines, "Keep functions under 50 lines and files under 800 lines".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/synthorg/api/controllers/ws.py` around lines 239 - 290, _handle_message is too large and mixes parsing, validation, and dispatch logic; split it into smaller helpers to keep functions <50 lines. Extract the JSON loading/size checks into a helper (e.g., _parse_ws_message) that takes data and _MAX_WS_MESSAGE_BYTES and returns either a dict or an error string, extract type/structure validation (channels list and filters dict and non-dict msg check) into a _validate_ws_message that returns normalized (action, channels, client_filters) or an error, and extract the final action dispatch into a small _dispatch_ws_action that calls _handle_subscribe/_handle_unsubscribe or returns the unknown-action error. Have _handle_message call these helpers in sequence, passing subscribed and filters only to the dispatch helper, and preserve existing logging symbols (API_WS_INVALID_MESSAGE, API_WS_UNKNOWN_ACTION) and return values.
♻️ Duplicate comments (2)
tests/unit/api/auth/test_ticket_store.py (1)
110-120:⚠️ Potential issue | 🟡 MinorAdd a concurrent-consumer test for the single-use guarantee.
This verifies serial reuse, but it still doesn’t pin the race case where multiple consumers hit the same ticket concurrently.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/api/auth/test_ticket_store.py` around lines 110 - 120, The current test (test_validate_and_consume_single_use) only checks serial reuse; add a new concurrent-consumer test that guarantees single-use under racing accesses by creating a WsTicketStore, calling store.create(user) to get a ticket, then concurrently invoking store.validate_and_consume(ticket) from multiple workers (e.g., ThreadPoolExecutor or asyncio.gather) and asserting exactly one call returns a non-None user while all others return None; reference WsTicketStore.create and WsTicketStore.validate_and_consume to locate the implementation to exercise the race.tests/unit/api/controllers/test_ws.py (1)
192-193:⚠️ Potential issue | 🟡 MinorAdd
@pytest.mark.timeout(30)toTestWsTicketAuth.This class-level timeout is still missing for the new WS ticket auth tests.
Suggested fix
+@pytest.mark.timeout(30) `@pytest.mark.unit` class TestWsTicketAuth:As per coding guidelines, "Set 30-second timeout per test".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/api/controllers/test_ws.py` around lines 192 - 193, Add a 30-second pytest timeout decorator to the WebSocket ticket auth test class by annotating the TestWsTicketAuth class with `@pytest.mark.timeout`(30); ensure pytest is imported if not already. This will apply the 30s per-test timeout to all tests in the TestWsTicketAuth class (look for the class name TestWsTicketAuth in the diff) and satisfy the "Set 30-second timeout per test" guideline.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/synthorg/api/app.py`:
- Around line 652-658: The code treats an intentionally empty auth.exclude_paths
as falsy and reintroduces defaults; change the assignment so it only uses the
defaults when auth.exclude_paths is None (not when it's empty). Replace the
current use of "auth.exclude_paths or (...)" with a None-check (e.g., use
auth.exclude_paths if auth.exclude_paths is not None else (...)) so the variable
exclude_paths preserves explicitly empty values; keep references to
auth.exclude_paths and prefix when locating the assignment.
In `@src/synthorg/api/auth/controller.py`:
- Around line 469-470: The expires_in construction currently uses
int(app_state.ticket_store.ttl_seconds) which can floor sub-second TTLs to 0 and
violate expires_in: Field(gt=0); change this to round up and guarantee at least
1 by using math.ceil on app_state.ticket_store.ttl_seconds (or equivalent) and
then cast to int (e.g., int(math.ceil(...)) or max(1, int(math.ceil(...)))), and
add the necessary math import; update the expression where expires_in is built
(referencing expires_in and app_state.ticket_store.ttl_seconds) so it never
becomes 0.
- Around line 461-464: The TicketLimitExceededError exception handler in
src/synthorg/api/auth/controller (the except block catching
TicketLimitExceededError that currently raises ConflictError(msg)) must log the
rejection before raising; update that except block to call process logger (or
the existing logger used in this module) to emit a WARNING or ERROR with the msg
and contextual fields (e.g., user identifier, request id, or session info if
available) and include the exception type, then re-raise ConflictError(msg) as
before—refer to the except TicketLimitExceededError block and the
ConflictError(msg) raise to locate and instrument the logging call.
In `@src/synthorg/api/controllers/ws.py`:
- Around line 153-157: The code assumes event["payload"] is a dict and iterates
over channel_filters which will raise if payload is not a mapping; update the
block that reads channel_filters = filters.get(channel) and payload =
event.get("payload", {}) to first verify payload is a dict (e.g., use
isinstance(payload, dict)) and if not, either coerce to an empty dict or return
early so the subsequent all(payload.get(k) == v for k, v in
channel_filters.items()) check cannot raise; ensure you keep the checks around
channel_filters and use the same symbol names (channel_filters, payload, event)
when adding the guard.
In `@tests/unit/api/auth/test_controller.py`:
- Around line 330-331: Add the missing explicit timeout decorator to the test
class by annotating TestWsTicket with `@pytest.mark.timeout`(30); locate the class
definition for TestWsTicket in tests/unit/api/auth/test_controller.py and add
the pytest.mark.timeout(30) decorator above it (alongside the existing
`@pytest.mark.unit`) so it follows the codebase convention.
In `@tests/unit/api/auth/test_ticket_store.py`:
- Around line 28-29: Add the 30-second timeout pytest decorator to the unit test
classes missing it: place `@pytest.mark.timeout`(30) immediately above the
TestWsTicketStoreCreate class definition (and likewise add the same decorator
above the two other unit test classes later in the file that are flagged by the
review) so each test class uses a 30s timeout; ensure you import pytest at top
if not already present and keep the decorator directly above the class
definitions.
---
Outside diff comments:
In `@src/synthorg/api/controllers/ws.py`:
- Around line 239-290: _handle_message is too large and mixes parsing,
validation, and dispatch logic; split it into smaller helpers to keep functions
<50 lines. Extract the JSON loading/size checks into a helper (e.g.,
_parse_ws_message) that takes data and _MAX_WS_MESSAGE_BYTES and returns either
a dict or an error string, extract type/structure validation (channels list and
filters dict and non-dict msg check) into a _validate_ws_message that returns
normalized (action, channels, client_filters) or an error, and extract the final
action dispatch into a small _dispatch_ws_action that calls
_handle_subscribe/_handle_unsubscribe or returns the unknown-action error. Have
_handle_message call these helpers in sequence, passing subscribed and filters
only to the dispatch helper, and preserve existing logging symbols
(API_WS_INVALID_MESSAGE, API_WS_UNKNOWN_ACTION) and return values.
---
Duplicate comments:
In `@tests/unit/api/auth/test_ticket_store.py`:
- Around line 110-120: The current test (test_validate_and_consume_single_use)
only checks serial reuse; add a new concurrent-consumer test that guarantees
single-use under racing accesses by creating a WsTicketStore, calling
store.create(user) to get a ticket, then concurrently invoking
store.validate_and_consume(ticket) from multiple workers (e.g.,
ThreadPoolExecutor or asyncio.gather) and asserting exactly one call returns a
non-None user while all others return None; reference WsTicketStore.create and
WsTicketStore.validate_and_consume to locate the implementation to exercise the
race.
In `@tests/unit/api/controllers/test_ws.py`:
- Around line 192-193: Add a 30-second pytest timeout decorator to the WebSocket
ticket auth test class by annotating the TestWsTicketAuth class with
`@pytest.mark.timeout`(30); ensure pytest is imported if not already. This will
apply the 30s per-test timeout to all tests in the TestWsTicketAuth class (look
for the class name TestWsTicketAuth in the diff) and satisfy the "Set 30-second
timeout per test" guideline.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 368e2c4a-d1c8-4223-a43f-8ac42b71094e
📒 Files selected for processing (15)
CLAUDE.mddocs/architecture/index.mddocs/roadmap/index.mddocs/security.mdsrc/synthorg/api/app.pysrc/synthorg/api/auth/__init__.pysrc/synthorg/api/auth/controller.pysrc/synthorg/api/auth/ticket_store.pysrc/synthorg/api/controllers/ws.pysrc/synthorg/api/state.pytests/unit/api/auth/test_controller.pytests/unit/api/auth/test_ticket_store.pytests/unit/api/controllers/test_ws.pyweb/src/__tests__/composables/useWebSocketSubscription.test.tsweb/src/stores/websocket.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Test (Python 3.14)
- GitHub Check: Build Backend
- GitHub Check: Build Sandbox
- GitHub Check: Build Web
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (4)
web/src/**/*.{vue,ts,tsx,js}
📄 CodeRabbit inference engine (CLAUDE.md)
Lint frontend code with ESLint; type-check with vue-tsc; test with Vitest; audit with npm audit (critical + high severity)
Files:
web/src/stores/websocket.tsweb/src/__tests__/composables/useWebSocketSubscription.test.ts
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Do not usefrom __future__ import annotations— Python 3.14 has PEP 649 native lazy annotations
Useexcept A, B:syntax (no parentheses) per PEP 758 — ruff enforces this on Python 3.14
Add type hints to all public functions and classes; enforce mypy strict mode
Use Google-style docstrings on all public classes and functions — required and enforced by ruff D rules
Keep line length to 88 characters (ruff enforced)
Files:
src/synthorg/api/auth/__init__.pysrc/synthorg/api/state.pytests/unit/api/auth/test_ticket_store.pysrc/synthorg/api/auth/ticket_store.pysrc/synthorg/api/controllers/ws.pysrc/synthorg/api/app.pysrc/synthorg/api/auth/controller.pytests/unit/api/auth/test_controller.pytests/unit/api/controllers/test_ws.py
src/synthorg/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/synthorg/**/*.py: Create new objects for immutability instead of mutating existing ones; for non-Pydantic internal collections (registries,BaseTool), usecopy.deepcopy()at construction andMappingProxyTypewrapping for read-only enforcement
Fordict/listfields in frozen Pydantic models, usecopy.deepcopy()at system boundaries (tool execution, LLM provider serialization, inter-agent delegation, persistence serialization)
Use frozen Pydantic models for config/identity; use separate mutable-via-copy models (usingmodel_copy(update=...)) for runtime state that evolves; never mix static config fields with mutable runtime fields in one model
Use Pydantic v2 (BaseModel,model_validator,computed_field,ConfigDict); use@computed_fieldfor derived values instead of storing redundant fields; useNotBlankStrfor all identifier/name fields
Preferasyncio.TaskGroupfor fan-out/fan-in parallel operations in new code (multiple tool invocations, parallel agent calls); prefer structured concurrency over barecreate_task
Keep functions under 50 lines and files under 800 lines
Handle errors explicitly, never silently swallow exceptions
Validate at system boundaries (user input, external APIs, config files)
Every module with business logic must importfrom synthorg.observability import get_loggerand createlogger = get_logger(__name__); never useimport loggingorlogging.getLogger()orprint()in application code
Always use event name constants from domain-specific modules undersynthorg.observability.events(e.g.,PROVIDER_CALL_STARTfromevents.provider,BUDGET_RECORD_ADDEDfromevents.budget); import directly:from synthorg.observability.events.<domain> import EVENT_CONSTANT
Log all state transitions at INFO level with structured kwargs (never use % formatting); log all error paths at WARNING or ERROR with context before raising; log object creation and key function entry/exit at DEBUG
Pure data models, enums, and re-e...
Files:
src/synthorg/api/auth/__init__.pysrc/synthorg/api/state.pysrc/synthorg/api/auth/ticket_store.pysrc/synthorg/api/controllers/ws.pysrc/synthorg/api/app.pysrc/synthorg/api/auth/controller.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Mark unit tests with@pytest.mark.unit, integration tests with@pytest.mark.integration, e2e tests with@pytest.mark.e2e, and slow tests with@pytest.mark.slow
Useasyncio_mode = "auto"in pytest configuration — no manual@pytest.mark.asyncioneeded on async tests
Set 30-second timeout per test
Prefer@pytest.mark.parametrizefor testing similar cases
Use Hypothesis for property-based testing in Python (@given+@settings); useciprofile (200 examples, default) anddevprofile (1000 examples) controlled viaHYPOTHESIS_PROFILEenv var
Never skip, dismiss, or ignore flaky tests — always fix them fully and fundamentally; for timing-sensitive tests, mocktime.monotonic()andasyncio.sleep()to make them deterministic
Files:
tests/unit/api/auth/test_ticket_store.pytests/unit/api/auth/test_controller.pytests/unit/api/controllers/test_ws.py
🧠 Learnings (14)
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to src/synthorg/**/*.py : All state transitions must log at INFO.
Applied to files:
src/synthorg/api/auth/ticket_store.pyCLAUDE.md
📚 Learning: 2026-03-16T07:36:16.026Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T07:36:16.026Z
Learning: Applies to src/synthorg/**/*.py : Log all state transitions at INFO level with structured kwargs (never use % formatting); log all error paths at WARNING or ERROR with context before raising; log object creation and key function entry/exit at DEBUG
Applied to files:
src/synthorg/api/auth/ticket_store.pyCLAUDE.md
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to src/synthorg/**/*.py : Every module with business logic MUST have: `from synthorg.observability import get_logger` then `logger = get_logger(__name__)`. Never use `import logging` / `logging.getLogger()` / `print()` in application code. Variable name: always `logger` (not `_logger`, not `log`).
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to src/synthorg/**/*.py : Structured kwargs in logging: always `logger.info(EVENT, key=value)` — never `logger.info('msg %s', val)`.
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-16T07:36:16.026Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T07:36:16.026Z
Learning: Applies to src/synthorg/**/*.py : Every module with business logic must import `from synthorg.observability import get_logger` and create `logger = get_logger(__name__)`; never use `import logging` or `logging.getLogger()` or `print()` in application code
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to src/synthorg/**/*.py : Event names: always use constants from domain-specific modules under synthorg.observability.events (e.g., PROVIDER_CALL_START from events.provider, BUDGET_RECORD_ADDED from events.budget, etc.). Import directly: `from synthorg.observability.events.<domain> import EVENT_CONSTANT`.
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to src/synthorg/**/*.py : All error paths must log at WARNING or ERROR with context before raising.
Applied to files:
CLAUDE.mdsrc/synthorg/api/auth/controller.py
📚 Learning: 2026-03-16T07:36:16.026Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T07:36:16.026Z
Learning: Applies to src/synthorg/**/*.py : Always use event name constants from domain-specific modules under `synthorg.observability.events` (e.g., `PROVIDER_CALL_START` from `events.provider`, `BUDGET_RECORD_ADDED` from `events.budget`); import directly: `from synthorg.observability.events.<domain> import EVENT_CONSTANT`
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to src/synthorg/**/*.py : DEBUG logging for object creation, internal flow, entry/exit of key functions.
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-16T07:36:16.026Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T07:36:16.026Z
Learning: Applies to src/synthorg/**/*.py : Pure data models, enums, and re-exports do NOT need logging
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-16T07:36:16.026Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T07:36:16.026Z
Learning: Applies to src/synthorg/**/*.py : Use Pydantic v2 (`BaseModel`, `model_validator`, `computed_field`, `ConfigDict`); use `computed_field` for derived values instead of storing redundant fields; use `NotBlankStr` for all identifier/name fields
Applied to files:
src/synthorg/api/auth/controller.py
📚 Learning: 2026-03-16T07:36:16.026Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T07:36:16.026Z
Learning: Applies to src/synthorg/**/*.py : Use frozen Pydantic models for config/identity; use separate mutable-via-copy models (using `model_copy(update=...)`) for runtime state that evolves; never mix static config fields with mutable runtime fields in one model
Applied to files:
src/synthorg/api/auth/controller.py
📚 Learning: 2026-03-16T07:36:16.026Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T07:36:16.026Z
Learning: Applies to tests/**/*.py : Set 30-second timeout per test
Applied to files:
tests/unit/api/controllers/test_ws.py
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to tests/**/*.py : Test timeout: 30 seconds per test.
Applied to files:
tests/unit/api/controllers/test_ws.py
🧬 Code graph analysis (11)
web/src/stores/websocket.ts (2)
web/src/api/endpoints/auth.ts (1)
getWsTicket(32-35)web/src/utils/logging.ts (1)
sanitizeForLog(2-11)
src/synthorg/api/auth/__init__.py (2)
src/synthorg/api/state.py (1)
ticket_store(231-233)src/synthorg/api/auth/ticket_store.py (2)
TicketLimitExceededError(35-36)WsTicketStore(58-169)
src/synthorg/api/state.py (1)
src/synthorg/api/auth/ticket_store.py (1)
WsTicketStore(58-169)
tests/unit/api/auth/test_ticket_store.py (3)
src/synthorg/api/auth/models.py (2)
AuthenticatedUser(69-88)AuthMethod(11-16)src/synthorg/api/state.py (1)
ticket_store(231-233)src/synthorg/api/auth/ticket_store.py (6)
TicketLimitExceededError(35-36)WsTicketStore(58-169)create(84-112)ttl_seconds(80-82)validate_and_consume(114-147)cleanup_expired(149-169)
web/src/__tests__/composables/useWebSocketSubscription.test.ts (2)
web/src/api/types.ts (1)
WsEventHandler(687-687)web/src/composables/useWebSocketSubscription.ts (1)
useWebSocketSubscription(47-105)
src/synthorg/api/auth/ticket_store.py (1)
src/synthorg/api/auth/models.py (1)
AuthenticatedUser(69-88)
src/synthorg/api/controllers/ws.py (4)
src/synthorg/api/auth/models.py (1)
AuthenticatedUser(69-88)web/src/api/types.ts (1)
HumanRole(46-51)src/synthorg/api/state.py (1)
ticket_store(231-233)src/synthorg/api/auth/ticket_store.py (1)
validate_and_consume(114-147)
src/synthorg/api/app.py (2)
src/synthorg/api/state.py (2)
AppState(31-250)ticket_store(231-233)src/synthorg/api/auth/ticket_store.py (1)
cleanup_expired(149-169)
src/synthorg/api/auth/controller.py (5)
src/synthorg/api/auth/models.py (3)
AuthenticatedUser(69-88)AuthMethod(11-16)User(19-40)src/synthorg/api/state.py (1)
ticket_store(231-233)src/synthorg/api/auth/ticket_store.py (3)
TicketLimitExceededError(35-36)create(84-112)ttl_seconds(80-82)web/src/api/types.ts (2)
WsTicketResponse(164-167)ApiResponse(125-127)src/synthorg/api/errors.py (2)
UnauthorizedError(171-179)ConflictError(149-157)
tests/unit/api/auth/test_controller.py (4)
tests/unit/api/conftest.py (2)
test_client(179-203)make_auth_headers(97-123)tests/unit/api/test_guards.py (1)
bare_client(17-20)src/synthorg/api/state.py (1)
ticket_store(231-233)src/synthorg/api/auth/ticket_store.py (1)
validate_and_consume(114-147)
tests/unit/api/controllers/test_ws.py (7)
src/synthorg/api/auth/models.py (1)
AuthMethod(11-16)src/synthorg/api/controllers/ws.py (1)
_handle_message(239-290)web/src/api/types.ts (1)
HumanRole(46-51)tests/unit/api/conftest.py (1)
test_client(179-203)src/synthorg/api/state.py (1)
ticket_store(231-233)src/synthorg/api/auth/ticket_store.py (1)
validate_and_consume(114-147)src/synthorg/api/app.py (1)
_build_middleware(642-670)
🔇 Additional comments (19)
docs/roadmap/index.md (1)
15-15: LGTM!Documentation accurately updated to reflect the new WebSocket ticket authentication method alongside JWT and API key auth.
docs/architecture/index.md (1)
40-40: LGTM!Module responsibility description accurately updated to include WS ticket authentication.
src/synthorg/api/auth/__init__.py (1)
6-17: LGTM!Clean re-exports of the new WsTicketStore and TicketLimitExceededError, maintaining alphabetical ordering in
__all__. As per coding guidelines, pure re-exports do not need logging.CLAUDE.md (2)
116-118: LGTM!Package structure documentation accurately updated to reflect the WS ticket store addition to the auth subpackage.
195-195: LGTM!Event constants list properly extended with the new API_WS_TICKET_* events for observability.
web/src/stores/websocket.ts (3)
30-32: Excellent implementation of the connect() deduplication and generation guard.The
connectGenerationcounter effectively prevents stale in-flight connects from opening a socket afterdisconnect()wins the race. TheconnectPromisepattern properly deduplicates concurrentconnect()calls during the async ticket exchange window.Also applies to: 44-52
54-83: Well-structured ticket exchange flow with proper error handling.The implementation correctly:
- Fetches a one-time ticket before opening the WebSocket
- Handles 401 auth errors specially to avoid reconnect storms (the 401 interceptor handles redirect)
- Guards against stale connect attempts with the generation check at line 78
- Uses
encodeURIComponentfor the ticket query parameter
174-178: LGTM!The
disconnect()function properly incrementsconnectGenerationto invalidate any in-flight connects and clearsconnectPromiseto allow fresh connect attempts after reconnection is desired.web/src/__tests__/composables/useWebSocketSubscription.test.ts (2)
5-21: LGTM!The async lifecycle mock implementation is correct. The
mountedPromiseis now stored without swallowing rejections (the previous.catch(() => {})pattern has been removed), allowing test failures to propagate properly.
72-83: LGTM!Test correctly awaits
mountedPromisebefore verifying thatconnect()was called, ensuring post-connect behavior is properly verified.src/synthorg/api/auth/ticket_store.py (4)
72-77: LGTM!TTL validation correctly rejects non-finite values (NaN/inf) with
math.isfinite()check, addressing the previously identified issue.
84-112: LGTM!The
create()method properly:
- Enforces per-user pending ticket cap (5) to prevent abuse
- Generates cryptographically secure 256-bit entropy tokens
- Uses frozen Pydantic model for the ticket entry
- Logs ticket issuance at INFO level with structured kwargs
114-147: LGTM!The
validate_and_consume()implementation correctly:
- Atomically removes the ticket via
dict.popbefore checking expiry (safe in single-threaded asyncio)- Logs invalid tickets and expired tickets at WARNING level
- Logs successful consumption at INFO level
- Returns None for both invalid and expired tickets (correct security behavior)
149-169: LGTM!The
cleanup_expired()method properly logs at INFO level when tickets are removed, addressing the previously flagged issue about state transition logging. Based on learnings: "Log all state transitions at INFO level with structured kwargs."src/synthorg/api/state.py (2)
99-99: LGTM!The
WsTicketStoreis correctly instantiated with default TTL (30s) duringAppState.__init__. This ensures the ticket store is always available from app startup.
230-233: LGTM!The
ticket_storeproperty correctly returns the always-available ticket store without the_require_serviceguard, consistent with the docstring documentation.src/synthorg/api/app.py (1)
208-220: Cleanup-task startup/shutdown wiring looks correct.Task creation, cancellation, and awaited shutdown handling are clean and prevent orphan background work.
docs/security.md (1)
75-75: Security documentation update is clear and accurate.The new bullet correctly captures TTL, single-use semantics, and URL credential leakage mitigation.
src/synthorg/api/controllers/ws.py (1)
184-193: Ticket validation beforeaccept()is implemented correctly.Rejecting unauthenticated/unauthorized connections before accepting the socket is the right flow for this auth model.
5aa49ce to
738718e
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
web/src/__tests__/stores/websocket.test.ts (1)
96-104:⚠️ Potential issue | 🟠 MajorMake the dedup regression test truly concurrent.
This currently covers the “already connected” no-op path, not two callers racing before
getWsTicket()resolves.🧪 Proposed fix
it('does not create duplicate connections', async () => { const store = useWebSocketStore() - await store.connect() - await vi.advanceTimersByTimeAsync(0) - expect(mockInstances).toHaveLength(1) - - await store.connect() // should be no-op - expect(mockInstances).toHaveLength(1) // no new WebSocket created + let resolveTicket!: (value: { ticket: string; expires_in: number }) => void + vi.mocked(getWsTicket).mockReturnValueOnce( + new Promise((resolve) => { + resolveTicket = resolve + }), + ) + + const first = store.connect() + const second = store.connect() + + expect(getWsTicket).toHaveBeenCalledTimes(1) + + resolveTicket({ ticket: 'test-ticket-abc', expires_in: 30 }) + await Promise.all([first, second]) + await vi.advanceTimersByTimeAsync(0) + + expect(mockInstances).toHaveLength(1) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/__tests__/stores/websocket.test.ts` around lines 96 - 104, The test currently only checks the already-connected no-op path; update it to simulate two concurrent callers racing before getWsTicket() resolves by invoking connect() twice without awaiting the first (e.g. const p1 = store.connect(); const p2 = store.connect(); await Promise.all([p1, p2]) or call both and then advance timers), then advance timers/resolve mocked ticket and assert mockInstances has length 1; use the useWebSocketStore, connect, and mockInstances identifiers to locate and change the test so both connects run concurrently before getWsTicket() completes.web/src/stores/websocket.ts (1)
44-50:⚠️ Potential issue | 🟠 MajorKeep stale connect attempts from clearing newer
connectPromises.If attempt A is awaiting
getWsTicket(),disconnect()can nullconnectPromiseand let attempt B start. When A finally settles, this unconditionalfinallyresetsconnectPromiseagain, so a third caller can start another in-flight connect while B is still running. That reintroduces the duplicate ticket-exchange / duplicate socket race this change was meant to close.Suggested fix
async function connect() { if (socket?.readyState === WebSocket.OPEN || socket?.readyState === WebSocket.CONNECTING) return // Deduplicate concurrent connect() calls — the ticket exchange is async, // so two callers could pass the readyState guard before the first resolves. if (connectPromise) return connectPromise const generation = connectGeneration - connectPromise = doConnect(generation).finally(() => { connectPromise = null }) - return connectPromise + let trackedPromise: Promise<void> + trackedPromise = doConnect(generation).finally(() => { + if (connectPromise === trackedPromise) { + connectPromise = null + } + }) + connectPromise = trackedPromise + return trackedPromise }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/stores/websocket.ts` around lines 44 - 50, The finally handler on connectPromise unconditionally clears the shared connectPromise, allowing stale connect attempts to wipe out newer ones; change connect() to capture the newly assigned promise in a local (e.g., const local = connectPromise) and in the doConnect(...).finally() only set connectPromise = null if the connectGeneration still equals the captured generation and connectPromise === local (ensuring you only clear the same promise), referencing the connect(), connectPromise, doConnect, and connectGeneration symbols to locate and update the logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/synthorg/api/auth/controller.py`:
- Around line 446-459: The current code accepts any AuthenticatedUser (including
API-key) and mints a WebSocket ticket; change the check after auth_user =
request.scope.get("user") to ensure only JWT-based identities are allowed before
creating ws_user: verify the authenticated identity is a JWT (e.g., check
auth_user.auth_method == AuthMethod.JWT or isinstance(auth_user,
JWTAuthenticatedUser)) and if not, emit the same API_AUTH_FAILED log
(reason="ws_ticket_auth_required") and raise UnauthorizedError; only then call
auth_user.model_copy(update={"auth_method": AuthMethod.WS_TICKET}) to produce
ws_user.
In `@src/synthorg/api/auth/ticket_store.py`:
- Around line 93-98: The per-user count is including expired tickets still in
self._tickets; either call self.cleanup_expired() before computing user_pending
or change the generator in create() to only count non-expired entries (e.g.,
check an expiry predicate like e.is_expired() or compare e.expires_at against
now) when summing over self._tickets.values(); update the user_pending
computation to ignore expired tickets so TicketLimitExceededError is only raised
for active tickets.
In `@src/synthorg/api/controllers/ws.py`:
- Around line 314-336: Subscription logic in _handle_subscribe currently treats
an empty dict as "no update" and preserves prior filters; change it so callers
can distinguish "no filters provided" from "clear filters": accept None to mean
"leave existing filters", treat {} explicitly as "clear filters" and update the
loop in _handle_subscribe to set filters[c] = dict(client_filters) when
client_filters is a non-empty dict, delete filters[c] when client_filters == {},
and leave filters unchanged when client_filters is None; update any callers to
pass None when filters are omitted.
In `@tests/unit/api/auth/test_ticket_store.py`:
- Around line 53-71: Replace the five nearly-identical tests with a single
parametrized pytest that iterates over the invalid TTL values and their expected
error-match strings; e.g., use `@pytest.mark.parametrize`("ttl,match",
[(0.0,"positive"), (-5.0,"positive"), (math.nan,"finite positive"),
(math.inf,"finite positive"), (-math.inf,"finite positive")]) and assert with
with pytest.raises(ValueError, match=match): WsTicketStore(ttl_seconds=ttl);
keep the test name (e.g., test_invalid_ttl_rejected) and ensure math is imported
if not already.
In `@web/src/__tests__/composables/useWebSocketSubscription.test.ts`:
- Around line 13-17: The inline comment "swallow — tests check via spies" on the
assignment mountedPromise = result inside the onMounted mock is stale and
misleading because the current implementation preserves promise rejections;
update the comment to accurately state that the mountedPromise holds the
returned Promise and its rejections are preserved (or remove the "swallow"
wording entirely) so it reflects the behavior of the onMounted mock and
mountedPromise variable.
---
Duplicate comments:
In `@web/src/__tests__/stores/websocket.test.ts`:
- Around line 96-104: The test currently only checks the already-connected no-op
path; update it to simulate two concurrent callers racing before getWsTicket()
resolves by invoking connect() twice without awaiting the first (e.g. const p1 =
store.connect(); const p2 = store.connect(); await Promise.all([p1, p2]) or call
both and then advance timers), then advance timers/resolve mocked ticket and
assert mockInstances has length 1; use the useWebSocketStore, connect, and
mockInstances identifiers to locate and change the test so both connects run
concurrently before getWsTicket() completes.
In `@web/src/stores/websocket.ts`:
- Around line 44-50: The finally handler on connectPromise unconditionally
clears the shared connectPromise, allowing stale connect attempts to wipe out
newer ones; change connect() to capture the newly assigned promise in a local
(e.g., const local = connectPromise) and in the doConnect(...).finally() only
set connectPromise = null if the connectGeneration still equals the captured
generation and connectPromise === local (ensuring you only clear the same
promise), referencing the connect(), connectPromise, doConnect, and
connectGeneration symbols to locate and update the logic.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 17403dbe-eb8d-4aeb-a2b4-708ea3a87c2c
📒 Files selected for processing (23)
CLAUDE.mddocs/architecture/index.mddocs/design/operations.mddocs/roadmap/index.mddocs/security.mdsrc/synthorg/api/app.pysrc/synthorg/api/auth/__init__.pysrc/synthorg/api/auth/controller.pysrc/synthorg/api/auth/models.pysrc/synthorg/api/auth/ticket_store.pysrc/synthorg/api/controllers/ws.pysrc/synthorg/api/state.pysrc/synthorg/observability/events/api.pytests/unit/api/auth/test_controller.pytests/unit/api/auth/test_ticket_store.pytests/unit/api/controllers/test_ws.pyweb/src/__tests__/composables/useWebSocketSubscription.test.tsweb/src/__tests__/stores/websocket.test.tsweb/src/api/endpoints/auth.tsweb/src/api/types.tsweb/src/composables/useWebSocketSubscription.tsweb/src/stores/websocket.tsweb/src/views/MeetingLogsPage.vue
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build Backend
- GitHub Check: Build Sandbox
- GitHub Check: Build Web
- GitHub Check: Test (Python 3.14)
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (10)
docs/**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
ALWAYS read the relevant
docs/design/page before implementing any feature — design spec is the starting point for architecture, data models, and behavior
Files:
docs/security.mddocs/roadmap/index.mddocs/architecture/index.mddocs/design/operations.md
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Nofrom __future__ import annotations— Python 3.14 has PEP 649 native lazy annotations
Use PEP 758 except syntax:except A, B:(no parentheses) — ruff enforces this on Python 3.14
Files:
src/synthorg/api/auth/models.pytests/unit/api/auth/test_controller.pysrc/synthorg/api/auth/__init__.pysrc/synthorg/observability/events/api.pytests/unit/api/auth/test_ticket_store.pysrc/synthorg/api/auth/ticket_store.pysrc/synthorg/api/auth/controller.pytests/unit/api/controllers/test_ws.pysrc/synthorg/api/controllers/ws.pysrc/synthorg/api/state.pysrc/synthorg/api/app.py
src/synthorg/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/synthorg/**/*.py: All public functions and classes must have type hints with mypy strict mode enabled
Use Google-style docstrings (required on all public classes and functions) — enforced by ruff D rules
Every module with business logic MUST import logger viafrom synthorg.observability import get_loggerand assign to variable namedlogger
Never useimport logging,logging.getLogger(), orprint()in application code — use the logger from observability module instead
Create new objects instead of mutating existing ones; for non-Pydantic internal collections usecopy.deepcopy()at construction andMappingProxyTypefor read-only enforcement
Use frozen Pydantic models for config/identity; use separate mutable-via-copy models for runtime state that evolves (viamodel_copy(update=...)) — never mix static config with mutable runtime fields
UseNotBlankStrfromcore.typesfor all identifier/name fields (including optional and tuple variants) instead of manual whitespace validators
Use@computed_fieldfor derived values instead of storing and validating redundant fields
Preferasyncio.TaskGroupfor fan-out/fan-in parallel operations in new code (e.g., multiple tool invocations, parallel agent calls) — use structured concurrency over barecreate_task
Keep functions under 50 lines and files under 800 lines
Handle errors explicitly, never silently swallow exceptions
Validate input at system boundaries: user input, external APIs, and config files
All provider calls must go throughBaseCompletionProviderwhich applies retry and rate limiting automatically — never implement retry logic in driver subclasses or calling code
Use event constants fromsynthorg.observability.events.<domain>modules for all structured logging instead of string literals
Log all error paths at WARNING or ERROR level with context before raising, and log all state transitions at INFO level
Use DEBUG logging for object creation, internal flow, and entry/exit of key function...
Files:
src/synthorg/api/auth/models.pysrc/synthorg/api/auth/__init__.pysrc/synthorg/observability/events/api.pysrc/synthorg/api/auth/ticket_store.pysrc/synthorg/api/auth/controller.pysrc/synthorg/api/controllers/ws.pysrc/synthorg/api/state.pysrc/synthorg/api/app.py
web/**/*.{ts,tsx,vue}
📄 CodeRabbit inference engine (CLAUDE.md)
Use fast-check (
fc.assert+fc.property) for property-based testing in Vue components and TypeScript code
Files:
web/src/api/types.tsweb/src/__tests__/stores/websocket.test.tsweb/src/__tests__/composables/useWebSocketSubscription.test.tsweb/src/composables/useWebSocketSubscription.tsweb/src/stores/websocket.tsweb/src/api/endpoints/auth.tsweb/src/views/MeetingLogsPage.vue
web/src/api/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Web API client must mirror backend Pydantic models with TypeScript types
Files:
web/src/api/types.tsweb/src/api/endpoints/auth.ts
web/src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use Axios client with TypeScript for API calls with proper error handling
Files:
web/src/api/types.tsweb/src/__tests__/stores/websocket.test.tsweb/src/__tests__/composables/useWebSocketSubscription.test.tsweb/src/composables/useWebSocketSubscription.tsweb/src/stores/websocket.tsweb/src/api/endpoints/auth.ts
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: All tests must be marked with@pytest.mark.unit,@pytest.mark.integration,@pytest.mark.e2e, or@pytest.mark.slow
Maintain 80% minimum code coverage (enforced in CI)
Never use real vendor names (Anthropic, OpenAI, Claude, GPT, etc.) in project-owned test code — use generic names:test-provider,test-small-001,test-medium-001,test-large-001
Use@pytest.mark.parametrizefor testing similar cases instead of duplicating test code
Use Hypothesis (@given+@settings) for property-based testing; run dev profile withHYPOTHESIS_PROFILE=devfor 1000 examples
Never skip, dismiss, or ignore flaky tests — always fix them fundamentally by mocking time functions instead of widening timing margins
Files:
tests/unit/api/auth/test_controller.pytests/unit/api/auth/test_ticket_store.pytests/unit/api/controllers/test_ws.py
web/src/stores/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use Pinia stores for state management
Files:
web/src/stores/websocket.ts
docs/design/**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
docs/design/**/*.md: When a spec topic is referenced (e.g., 'the Agents page' or 'the Engine page's Crash Recovery section'), read the relevant design page before coding
When approved deviations from design spec occur, update the relevantdocs/design/page to reflect the new reality
Files:
docs/design/operations.md
web/src/**/*.vue
📄 CodeRabbit inference engine (CLAUDE.md)
Vue 3 components with PrimeVue + Tailwind CSS styling
Files:
web/src/views/MeetingLogsPage.vue
🧠 Learnings (21)
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to tests/**/*.py : Test timeout: 30 seconds per test.
Applied to files:
tests/unit/api/auth/test_controller.pytests/unit/api/auth/test_ticket_store.pytests/unit/api/controllers/test_ws.py
📚 Learning: 2026-03-16T19:51:10.862Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T19:51:10.862Z
Learning: Use 30-second timeout per test to catch performance regressions early
Applied to files:
tests/unit/api/auth/test_controller.pytests/unit/api/auth/test_ticket_store.pytests/unit/api/controllers/test_ws.py
📚 Learning: 2026-03-16T19:51:10.861Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T19:51:10.861Z
Learning: Applies to tests/**/*.py : Never skip, dismiss, or ignore flaky tests — always fix them fundamentally by mocking time functions instead of widening timing margins
Applied to files:
tests/unit/api/auth/test_controller.pytests/unit/api/auth/test_ticket_store.pytests/unit/api/controllers/test_ws.py
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to tests/**/*.py : Test markers: pytest.mark.unit, pytest.mark.integration, pytest.mark.e2e, pytest.mark.slow. Coverage: 80% minimum (enforced in CI).
Applied to files:
tests/unit/api/auth/test_controller.pytests/unit/api/auth/test_ticket_store.py
📚 Learning: 2026-03-16T19:51:10.861Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T19:51:10.861Z
Learning: Applies to web/src/stores/**/*.ts : Use Pinia stores for state management
Applied to files:
web/src/__tests__/composables/useWebSocketSubscription.test.tsweb/src/stores/websocket.ts
📚 Learning: 2026-03-16T19:51:10.861Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T19:51:10.861Z
Learning: Applies to web/__tests__/**/*.{ts,vue} : Use Vitest for unit testing Vue components and TypeScript utilities
Applied to files:
web/src/__tests__/composables/useWebSocketSubscription.test.ts
📚 Learning: 2026-03-16T19:51:10.861Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T19:51:10.861Z
Learning: Applies to web/src/**/*.ts : Use Axios client with TypeScript for API calls with proper error handling
Applied to files:
web/src/stores/websocket.ts
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to src/synthorg/**/*.py : Every module with business logic MUST have: `from synthorg.observability import get_logger` then `logger = get_logger(__name__)`. Never use `import logging` / `logging.getLogger()` / `print()` in application code. Variable name: always `logger` (not `_logger`, not `log`).
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to src/synthorg/**/*.py : Structured kwargs in logging: always `logger.info(EVENT, key=value)` — never `logger.info('msg %s', val)`.
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-16T19:51:10.861Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T19:51:10.861Z
Learning: Applies to src/synthorg/**/*.py : Every module with business logic MUST import logger via `from synthorg.observability import get_logger` and assign to variable named `logger`
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to src/synthorg/**/*.py : Event names: always use constants from domain-specific modules under synthorg.observability.events (e.g., PROVIDER_CALL_START from events.provider, BUDGET_RECORD_ADDED from events.budget, etc.). Import directly: `from synthorg.observability.events.<domain> import EVENT_CONSTANT`.
Applied to files:
CLAUDE.mdsrc/synthorg/observability/events/api.py
📚 Learning: 2026-03-16T19:51:10.861Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T19:51:10.861Z
Learning: Applies to src/synthorg/**/*.py : Never use `import logging`, `logging.getLogger()`, or `print()` in application code — use the logger from observability module instead
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-16T19:51:10.861Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T19:51:10.861Z
Learning: Applies to src/synthorg/**/*.py : Use event constants from `synthorg.observability.events.<domain>` modules for all structured logging instead of string literals
Applied to files:
CLAUDE.mdsrc/synthorg/observability/events/api.py
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to src/synthorg/**/*.py : All error paths must log at WARNING or ERROR with context before raising.
Applied to files:
CLAUDE.mdsrc/synthorg/api/auth/controller.py
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to src/synthorg/**/*.py : All state transitions must log at INFO.
Applied to files:
CLAUDE.mdsrc/synthorg/api/auth/ticket_store.py
📚 Learning: 2026-03-16T19:51:10.861Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T19:51:10.861Z
Learning: Applies to src/synthorg/**/*.py : Log all error paths at WARNING or ERROR level with context before raising, and log all state transitions at INFO level
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-16T19:51:10.861Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T19:51:10.861Z
Learning: Applies to src/synthorg/**/*.py : Use DEBUG logging for object creation, internal flow, and entry/exit of key functions
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-16T19:51:10.861Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T19:51:10.861Z
Learning: Applies to web/src/**/*.vue : Vue 3 components with PrimeVue + Tailwind CSS styling
Applied to files:
web/src/views/MeetingLogsPage.vue
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to web/** : Web dashboard: Node.js 20+, dependencies in web/package.json (Vue 3, PrimeVue, Tailwind CSS, Pinia, VueFlow, ECharts, Axios, vue-draggable-plus, Vitest, fast-check, ESLint, vue-tsc).
Applied to files:
web/src/views/MeetingLogsPage.vue
📚 Learning: 2026-03-16T19:51:10.862Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T19:51:10.862Z
Learning: Applies to web/package.json : Web dashboard dependencies: Node.js 20+, Vue 3, PrimeVue, Tailwind CSS, Pinia, VueFlow, ECharts, Axios, Vitest, fast-check, ESLint, vue-tsc
Applied to files:
web/src/views/MeetingLogsPage.vue
📚 Learning: 2026-03-16T19:51:10.861Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T19:51:10.861Z
Learning: Applies to src/synthorg/**/*.py : Use frozen Pydantic models for config/identity; use separate mutable-via-copy models for runtime state that evolves (via `model_copy(update=...)`) — never mix static config with mutable runtime fields
Applied to files:
src/synthorg/api/auth/controller.py
🧬 Code graph analysis (12)
web/src/api/types.ts (1)
src/synthorg/api/auth/controller.py (1)
WsTicketResponse(156-167)
tests/unit/api/auth/test_controller.py (4)
tests/unit/api/conftest.py (2)
test_client(190-214)make_auth_headers(108-134)tests/unit/api/test_guards.py (1)
bare_client(17-20)src/synthorg/api/state.py (1)
ticket_store(231-233)src/synthorg/api/auth/ticket_store.py (1)
validate_and_consume(114-147)
src/synthorg/api/auth/__init__.py (2)
src/synthorg/api/state.py (1)
ticket_store(231-233)src/synthorg/api/auth/ticket_store.py (2)
TicketLimitExceededError(35-36)WsTicketStore(58-169)
web/src/__tests__/composables/useWebSocketSubscription.test.ts (2)
web/src/api/types.ts (1)
WsEventHandler(701-701)web/src/composables/useWebSocketSubscription.ts (1)
useWebSocketSubscription(47-105)
web/src/stores/websocket.ts (2)
web/src/api/endpoints/auth.ts (1)
getWsTicket(32-35)web/src/utils/logging.ts (1)
sanitizeForLog(2-11)
web/src/api/endpoints/auth.ts (3)
src/synthorg/api/auth/controller.py (1)
WsTicketResponse(156-167)web/src/api/types.ts (2)
WsTicketResponse(178-181)ApiResponse(139-141)web/src/api/client.ts (2)
apiClient(12-16)unwrap(70-80)
tests/unit/api/auth/test_ticket_store.py (3)
src/synthorg/api/auth/models.py (2)
AuthenticatedUser(69-88)AuthMethod(11-16)src/synthorg/api/auth/ticket_store.py (6)
TicketLimitExceededError(35-36)WsTicketStore(58-169)create(84-112)ttl_seconds(80-82)validate_and_consume(114-147)cleanup_expired(149-169)web/src/api/types.ts (1)
HumanRole(46-51)
src/synthorg/api/auth/ticket_store.py (1)
src/synthorg/api/auth/models.py (1)
AuthenticatedUser(69-88)
src/synthorg/api/auth/controller.py (4)
src/synthorg/api/auth/models.py (3)
AuthenticatedUser(69-88)AuthMethod(11-16)User(19-40)src/synthorg/api/state.py (1)
ticket_store(231-233)src/synthorg/api/auth/ticket_store.py (3)
TicketLimitExceededError(35-36)create(84-112)ttl_seconds(80-82)src/synthorg/api/errors.py (2)
UnauthorizedError(211-219)ConflictError(189-197)
src/synthorg/api/controllers/ws.py (4)
src/synthorg/api/auth/models.py (1)
AuthenticatedUser(69-88)web/src/api/types.ts (1)
HumanRole(46-51)src/synthorg/api/state.py (1)
ticket_store(231-233)src/synthorg/api/auth/ticket_store.py (1)
validate_and_consume(114-147)
src/synthorg/api/state.py (1)
src/synthorg/api/auth/ticket_store.py (1)
WsTicketStore(58-169)
src/synthorg/api/app.py (2)
src/synthorg/api/state.py (2)
AppState(31-250)ticket_store(231-233)src/synthorg/api/auth/ticket_store.py (1)
cleanup_expired(149-169)
🔇 Additional comments (21)
docs/roadmap/index.md (1)
15-15: LGTM!Documentation accurately reflects the new WS ticket authentication capability alongside existing JWT and API key methods.
docs/architecture/index.md (1)
40-40: LGTM!Architecture documentation correctly updated to reflect the WS ticket authentication addition to the API layer.
src/synthorg/api/auth/models.py (1)
16-16: LGTM!Clean addition to
AuthMethodenum following the existing pattern, enabling proper tracking of WS ticket authentication inAuthenticatedUser.web/src/composables/useWebSocketSubscription.ts (1)
56-67: LGTM!Converting
onMountedto an async callback and awaitingwsStore.connect()correctly aligns with the new ticket-based authentication flow. The existing try/catch properly handles connection failures by settingsetupError.src/synthorg/api/auth/__init__.py (1)
6-17: LGTM!Clean re-exports of
TicketLimitExceededErrorandWsTicketStorefollowing the existing module pattern. The__all__list maintains alphabetical ordering.docs/design/operations.md (1)
963-978: LGTM!Design documentation accurately captures the new WS ticket authentication flow:
- Auth endpoint description expanded to include
ws-ticket- WebSocket endpoint notes ticket-based authentication via
?ticket=- New
POST /api/v1/auth/ws-ticketendpoint properly documentedweb/src/views/MeetingLogsPage.vue (1)
26-28: LGTM!Clean migration to
useWebSocketSubscriptioncomposable. This eliminates manual WebSocket lifecycle management and centralizes the ticket-based connection flow. The binding correctly wires themeetingschannel tomeetingStore.handleWsEvent.src/synthorg/api/controllers/ws.py (3)
49-74: LGTM!Ticket validation is well-structured with appropriate close codes and logging for the various failure scenarios (missing ticket, invalid/expired ticket).
115-167: LGTM!The
_on_eventfunction properly validates the payload type before filter matching (Lines 155-157), addressing the previous review feedback. Exception handling is comprehensive with appropriate logging levels.
170-214: LGTM!The ticket-based authentication flow is correctly implemented: validate ticket → check role → accept connection. User context is properly stored in scope for downstream access, and cleanup is handled in the finally block.
web/src/api/endpoints/auth.ts (1)
9-10: WS ticket endpoint client integration looks solid.Type import, Axios usage, and
unwrap()handling are consistent with the existing API client conventions.Also applies to: 32-35
docs/security.md (1)
75-75: Security doc update is clear and aligned with the ticket-based WS auth flow.Nice concise explanation of why JWT query params were replaced.
web/src/api/types.ts (1)
178-181:WsTicketResponsetype matches the backend contract.The added interface shape is consistent with the server response model.
src/synthorg/api/state.py (1)
62-63:AppStateticket store integration is clean.Initialization, slot declaration, and typed accessor are consistent and make ticket storage reliably available.
Also applies to: 99-100, 230-233
src/synthorg/api/app.py (2)
151-165: Background cleanup task lifecycle handling is well done.Startup creation, done-callback wiring, and graceful cancellation/await on shutdown are all in place.
Also applies to: 183-220
650-668: Middleware exclusion handling for/wsis implemented correctly.The
None-only fallback and explicit WS-path exclusion keep ticket-auth routing isolated from JWT middleware.web/src/__tests__/stores/websocket.test.ts (1)
328-345: Good additions for reconnect and ticket-exchange failure behavior.These tests strengthen the new WS ticket auth flow and improve regression coverage.
Also applies to: 407-422
tests/unit/api/auth/test_controller.py (1)
330-380:TestWsTicketcoverage is solid.The class exercises the key endpoint behaviors and single-use ticket consumption semantics.
tests/unit/api/auth/test_ticket_store.py (1)
123-315: Great depth of behavioral coverage for ticket lifecycle semantics.The suite now covers concurrent consumption, TTL boundaries, and cleanup interactions comprehensively.
src/synthorg/observability/events/api.py (1)
53-57: Nice addition of explicit WS ticket lifecycle events.Keeping these as first-class API event constants makes the new ticket flow observable without falling back to string literals at call sites.
Based on learnings: Applies to src/synthorg/**/*.py : Use event constants from
synthorg.observability.events.<domain>modules for all structured logging instead of string literals.tests/unit/api/controllers/test_ws.py (1)
179-308: Good breadth on the new WS-ticket contract.This covers issuance, single-use consumption, identity preservation, middleware exclusion, and bad-ticket rejection in one place, which gives the new auth path solid regression coverage.
…ket auth The WebSocket connection previously passed the JWT as a URL query parameter, exposing the bearer token in proxy/server logs, browser history, and Referer headers. The auth middleware also rejected WS connections because browsers cannot set custom headers on WebSocket upgrade requests (reason=missing_header, 403). Implement a short-lived, single-use ticket exchange: - POST /api/v1/auth/ws-ticket: authenticated endpoint returns a 30-second, single-use ticket (secrets.token_urlsafe, 256-bit) - WS handler validates and consumes the ticket before accept(), closing with 4001/4003 codes on auth failure - /api/v1/ws excluded from auth middleware (ticket auth is inline) - Frontend fetches a fresh ticket on each connect/reconnect - Background cleanup task prunes unconsumed expired tickets Closes #343
Pre-reviewed by 12 agents, 17 findings addressed: - Await cancelled cleanup task to prevent CancelledError leak - Add error handling to periodic cleanup loop - Promote _cleanup_expired to public cleanup_expired method - Validate TTL > 0 at construction - Remove duplicate API_WS_TICKET_ISSUED log emission - Add connect() race guard (connectPromise dedup) in WS store - Migrate MeetingLogsPage to useWebSocketSubscription composable - Improve middleware exclusion test assertion - Add defense-in-depth comment for role check - Remove eager O(n) cleanup from validate_and_consume - Update CLAUDE.md package structure and event constants - Update operations.md API surface table
The async connect() was called fire-and-forget inside setTimeout, causing unhandled promise rejections if the ticket exchange failed during reconnect.
…and Gemini
Security hardening:
- Add per-user pending ticket cap (5) to prevent memory exhaustion
- Reject non-finite TTL values (NaN/inf) in WsTicketStore
- Always merge WS upgrade path into auth exclusions (even when
custom exclude_paths are configured)
- Add connect generation counter to invalidate in-flight connects
when disconnect() wins the race
- Skip reconnect on 401 to prevent redirect storm with Axios interceptor
Backend robustness:
- Re-raise MemoryError/RecursionError in cleanup loop (match project pattern)
- Add done_callback on cleanup background task to detect silent death
- Extract cleanup loop to module-level function
- Use dict.pop instead of del in cleanup_expired (defensive against races)
- Log unauthorized ticket-issuance path before raising
- Promote cleanup state-transition log from DEBUG to INFO
Code structure (CLAUDE.md function size limit):
- Split ws_handler into _validate_ticket, _check_ws_role, _on_event helpers
- Split _handle_message into _handle_subscribe/_handle_unsubscribe
- Import _READ_ROLES from guards.py instead of shadowing locally
- Separate TypeError from JSONDecodeError catches for better diagnostics
- Close socket on non-disconnect send failure (prevent stale connections)
- Add user context to _receive_loop transport error logging
Documentation consistency:
- Add auth/ sub-entry to CLAUDE.md Package Structure
- Update docs/architecture, docs/security, docs/roadmap with WS ticket auth
- Update AppState docstring with ticket_store attribute
- Rephrase GIL atomicity claim for asyncio cooperative model (Python 3.14)
Test improvements:
- Move @pytest.mark.unit to class level (consistent convention)
- Move import re to module top level
- Parametrize non-dict JSON test cases
- Fix test_client.headers mutation (pass per-request headers)
- Add tests for NaN/inf TTL rejection and per-user ticket cap
- Remove .catch(() => {}) from composable test harness onMounted mock
- Use None-check instead of falsy-or for auth.exclude_paths (preserves explicitly empty tuples) - Use math.ceil for expires_in to prevent floor-to-zero on sub-second TTLs - Log ticket limit exceeded before raising ConflictError - Guard against non-dict payload in WS event filter matching - Split _handle_message into _parse_ws_message + _validate_ws_fields + dispatch (all functions now <50 lines) - Add @pytest.mark.timeout(30) to all new test classes - Add concurrent consumer test for single-use ticket guarantee
- Restrict ws-ticket endpoint to JWT-authenticated users only (reject API key auth) - Exclude expired tickets from per-user pending count - Distinguish absent filters (None, leave unchanged) from explicit clear (empty dict) in subscribe handler - Parametrize invalid TTL rejection tests (5 tests → 1 parametrized) - Use generation-based connectPromise cleanup to prevent stale finally callbacks from clearing newer attempts - Fix stale "swallow" comment in composable test harness
738718e to
10c0be5
Compare
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)
web/src/composables/useWebSocketSubscription.ts (1)
56-83:⚠️ Potential issue | 🟠 MajorGuard async mount flow against unmount races.
If the component unmounts while awaiting
wsStore.connect()on line 61, the setup continues after cleanup runs. This re-registers subscriptions and handlers thatonUnmountedjust removed, leaking them.Add a disposed flag set in
onUnmountedand check it after each async operation to prevent setup from continuing after cleanup:🛠️ Suggested fix
export function useWebSocketSubscription( options: WebSocketSubscriptionOptions, ): WebSocketSubscriptionReturn { const wsStore = useWebSocketStore() const authStore = useAuthStore() const setupError = ref<string | null>(null) + let disposed = false const uniqueChannels: WsChannel[] = [...new Set(options.bindings.map((b) => b.channel))] onMounted(async () => { if (!authStore.token) return try { if (!wsStore.connected) { await wsStore.connect() } + if (disposed) return } catch (err) { setupError.value = 'WebSocket connection failed.' console.error('WebSocket connect failed:', sanitizeForLog(err), err) return } try { wsStore.subscribe(uniqueChannels, options.filters) + if (disposed) return } catch (err) { setupError.value = 'WebSocket subscription failed.' console.error('WebSocket subscribe failed:', sanitizeForLog(err), err) } for (const binding of options.bindings) { + if (disposed) return try { wsStore.onChannelEvent(binding.channel, binding.handler) } catch (err) { console.error('WebSocket handler wiring failed:', sanitizeForLog(err), err) } } }) onUnmounted(() => { + disposed = true try { wsStore.unsubscribe(uniqueChannels)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/composables/useWebSocketSubscription.ts` around lines 56 - 83, The onMounted async flow can continue after the component unmounts, so add a local disposed flag (e.g., let disposed = false) set to true in an onUnmounted callback and check it after each await/async step to bail out; specifically, after awaiting wsStore.connect() verify disposed before continuing, and perform the same disposed check before calling wsStore.subscribe(uniqueChannels, options.filters), and before wiring handlers with wsStore.onChannelEvent(binding.channel, binding.handler); ensure you also avoid mutating setupError or calling sanitizeForLog/console if disposed to prevent re-registering or logging after cleanup.
♻️ Duplicate comments (1)
web/src/__tests__/stores/websocket.test.ts (1)
96-104:⚠️ Potential issue | 🟡 MinorDuplicate-connect test still misses the in-flight race path.
This currently verifies the “already connected” path, not two callers entering
connect()before ticket fetch resolves.Suggested test adjustment
it('does not create duplicate connections', async () => { const store = useWebSocketStore() - await store.connect() - await vi.advanceTimersByTimeAsync(0) - expect(mockInstances).toHaveLength(1) - - await store.connect() // should be no-op — already connected - expect(mockInstances).toHaveLength(1) // no new WebSocket created + let resolveTicket!: (value: { ticket: string; expires_in: number }) => void + vi.mocked(getWsTicket).mockReturnValueOnce( + new Promise((resolve) => { + resolveTicket = resolve + }), + ) + + const first = store.connect() + const second = store.connect() + expect(getWsTicket).toHaveBeenCalledTimes(1) + + resolveTicket({ ticket: 'test-ticket-abc', expires_in: 30 }) + await Promise.all([first, second]) + await vi.advanceTimersByTimeAsync(0) + expect(mockInstances).toHaveLength(1) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/__tests__/stores/websocket.test.ts` around lines 96 - 104, Update the test to exercise the in-flight race: stub the ticket-fetch used by useWebSocketStore().connect() so it returns a controllable (pending) promise, then call store.connect() twice without awaiting the first (i.e., invoke connect() twice before resolving the ticket promise), resolve the ticket promise afterwards and advance timers, and finally assert mockInstances has length 1; target the useWebSocketStore() instance and its connect() method and the existing mockInstances array to ensure only one WebSocket is created even when two callers race to connect.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/security.md`:
- Line 75: Update the "One-time WebSocket tickets" sentence to soften the claim:
change wording to say the tickets replace JWTs in WebSocket query parameters
(preventing long-lived JWT leakage in URLs and logs) rather than implying they
eliminate all URL-carried credential artifacts; keep details about short-lived
(30s), single-use, cryptographically random tokens, the POST
/api/v1/auth/ws-ticket endpoint requirement (valid JWT), in-memory store,
monotonic clock expiry, and per-process scope intact while clarifying the
security benefit is specifically about reducing JWT exposure.
---
Outside diff comments:
In `@web/src/composables/useWebSocketSubscription.ts`:
- Around line 56-83: The onMounted async flow can continue after the component
unmounts, so add a local disposed flag (e.g., let disposed = false) set to true
in an onUnmounted callback and check it after each await/async step to bail out;
specifically, after awaiting wsStore.connect() verify disposed before
continuing, and perform the same disposed check before calling
wsStore.subscribe(uniqueChannels, options.filters), and before wiring handlers
with wsStore.onChannelEvent(binding.channel, binding.handler); ensure you also
avoid mutating setupError or calling sanitizeForLog/console if disposed to
prevent re-registering or logging after cleanup.
---
Duplicate comments:
In `@web/src/__tests__/stores/websocket.test.ts`:
- Around line 96-104: Update the test to exercise the in-flight race: stub the
ticket-fetch used by useWebSocketStore().connect() so it returns a controllable
(pending) promise, then call store.connect() twice without awaiting the first
(i.e., invoke connect() twice before resolving the ticket promise), resolve the
ticket promise afterwards and advance timers, and finally assert mockInstances
has length 1; target the useWebSocketStore() instance and its connect() method
and the existing mockInstances array to ensure only one WebSocket is created
even when two callers race to connect.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5905ecbb-6fc7-4cbe-ae31-3d3755b1667c
📒 Files selected for processing (23)
CLAUDE.mddocs/architecture/index.mddocs/design/operations.mddocs/roadmap/index.mddocs/security.mdsrc/synthorg/api/app.pysrc/synthorg/api/auth/__init__.pysrc/synthorg/api/auth/controller.pysrc/synthorg/api/auth/models.pysrc/synthorg/api/auth/ticket_store.pysrc/synthorg/api/controllers/ws.pysrc/synthorg/api/state.pysrc/synthorg/observability/events/api.pytests/unit/api/auth/test_controller.pytests/unit/api/auth/test_ticket_store.pytests/unit/api/controllers/test_ws.pyweb/src/__tests__/composables/useWebSocketSubscription.test.tsweb/src/__tests__/stores/websocket.test.tsweb/src/api/endpoints/auth.tsweb/src/api/types.tsweb/src/composables/useWebSocketSubscription.tsweb/src/stores/websocket.tsweb/src/views/MeetingLogsPage.vue
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Test (Python 3.14)
- GitHub Check: Build Backend
- GitHub Check: Build Web
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (12)
{docs,site}/**/*.{md,astro}
📄 CodeRabbit inference engine (CLAUDE.md)
NEVER use
cdin Bash commands — the working directory is already set to the project root. Use absolute paths or run commands directly
Files:
docs/security.mddocs/design/operations.mddocs/roadmap/index.mddocs/architecture/index.md
{src,tests,web/src,cli}/**/*.{py,ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Line length: 88 characters (ruff)
Files:
web/src/api/endpoints/auth.tsweb/src/api/types.tsweb/src/composables/useWebSocketSubscription.tsweb/src/__tests__/composables/useWebSocketSubscription.test.tssrc/synthorg/api/app.pysrc/synthorg/api/auth/__init__.pyweb/src/stores/websocket.tssrc/synthorg/observability/events/api.pysrc/synthorg/api/auth/ticket_store.pyweb/src/__tests__/stores/websocket.test.tstests/unit/api/auth/test_ticket_store.pysrc/synthorg/api/controllers/ws.pysrc/synthorg/api/auth/models.pysrc/synthorg/api/state.pytests/unit/api/auth/test_controller.pytests/unit/api/controllers/test_ws.pysrc/synthorg/api/auth/controller.py
docs/design/**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
When approved deviations occur, update the relevant
docs/design/page to reflect the new reality
Files:
docs/design/operations.md
docs/design/operations.md
📄 CodeRabbit inference engine (CLAUDE.md)
Vendor names may only appear in: (1) Operations design page (
docs/design/operations.md), (2).claude/skill/agent files, (3) third-party import paths/module names
Files:
docs/design/operations.md
web/src/composables/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Vue composition functions: reusable composition functions (useAuth, useLoginLockout, usePolling, useOptimisticUpdate, useWebSocketSubscription)
Files:
web/src/composables/useWebSocketSubscription.ts
{tests,web/src/__tests__,cli}/**/*.{py,ts,go}
📄 CodeRabbit inference engine (CLAUDE.md)
Property-based testing: Python uses Hypothesis (
@given+@settings), Vue uses fast-check (fc.assert+fc.property), Go uses nativetesting.Ffuzz functions
Files:
web/src/__tests__/composables/useWebSocketSubscription.test.tsweb/src/__tests__/stores/websocket.test.tstests/unit/api/auth/test_ticket_store.pytests/unit/api/auth/test_controller.pytests/unit/api/controllers/test_ws.py
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Nofrom __future__ import annotations— Python 3.14 has PEP 649 native lazy annotations
PEP 758 except syntax: useexcept A, B:(no parentheses) — ruff enforces this on Python 3.14
Files:
src/synthorg/api/app.pysrc/synthorg/api/auth/__init__.pysrc/synthorg/observability/events/api.pysrc/synthorg/api/auth/ticket_store.pytests/unit/api/auth/test_ticket_store.pysrc/synthorg/api/controllers/ws.pysrc/synthorg/api/auth/models.pysrc/synthorg/api/state.pytests/unit/api/auth/test_controller.pytests/unit/api/controllers/test_ws.pysrc/synthorg/api/auth/controller.py
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.py: Type hints: all public functions, mypy strict mode
Docstrings: Google style, required on public classes/functions (enforced by ruff D rules)
Files:
src/synthorg/api/app.pysrc/synthorg/api/auth/__init__.pysrc/synthorg/observability/events/api.pysrc/synthorg/api/auth/ticket_store.pysrc/synthorg/api/controllers/ws.pysrc/synthorg/api/auth/models.pysrc/synthorg/api/state.pysrc/synthorg/api/auth/controller.py
src/synthorg/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/synthorg/**/*.py: Immutability: create new objects, never mutate existing ones. For non-Pydantic internal collections (registries,BaseTool), usecopy.deepcopy()at construction +MappingProxyTypewrapping for read-only enforcement
Fordict/listfields in frozen Pydantic models, rely onfrozen=Truefor field reassignment prevention andcopy.deepcopy()at system boundaries (tool execution, LLM provider serialization, inter-agent delegation, serializing for persistence)
Config vs runtime state: frozen Pydantic models for config/identity; separate mutable-via-copy models (usingmodel_copy(update=...)) for runtime state that evolves
Never mix static config fields with mutable runtime fields in one model
Models: Pydantic v2 (BaseModel,model_validator,computed_field,ConfigDict). Use@computed_fieldfor derived values instead of storing + validating redundant fields
UseNotBlankStr(fromcore.types) for all identifier/name fields — including optional (NotBlankStr | None) and tuple (tuple[NotBlankStr, ...]) variants — instead of manual whitespace validators
Async concurrency: preferasyncio.TaskGroupfor fan-out/fan-in parallel operations in new code
Functions: < 50 lines, files < 800 lines
Errors: handle explicitly, never silently swallow
Validate: at system boundaries (user input, external APIs, config files)
Every module with business logic MUST have:from synthorg.observability import get_loggerthenlogger = get_logger(__name__)
Never useimport logging/logging.getLogger()/print()in application code
Variable name: alwayslogger(not_logger, notlog)
Event names: always use constants from the domain-specific module undersynthorg.observability.events
Structured kwargs: alwayslogger.info(EVENT, key=value)— neverlogger.info("msg %s", val)
All error paths must log at WARNING or ERROR with context before raising
All state transitions must log at INFO
DEBUG for object creation, internal flow, entry/e...
Files:
src/synthorg/api/app.pysrc/synthorg/api/auth/__init__.pysrc/synthorg/observability/events/api.pysrc/synthorg/api/auth/ticket_store.pysrc/synthorg/api/controllers/ws.pysrc/synthorg/api/auth/models.pysrc/synthorg/api/state.pysrc/synthorg/api/auth/controller.py
{src,tests}/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Vendor-agnostic everywhere: NEVER use real vendor names (Anthropic, OpenAI, Claude, GPT, etc.) in project-owned code, docstrings, comments, tests, or config examples. Use generic names:
example-provider,example-large-001,example-medium-001,example-small-001,large/medium/smallas aliases
Files:
src/synthorg/api/app.pysrc/synthorg/api/auth/__init__.pysrc/synthorg/observability/events/api.pysrc/synthorg/api/auth/ticket_store.pytests/unit/api/auth/test_ticket_store.pysrc/synthorg/api/controllers/ws.pysrc/synthorg/api/auth/models.pysrc/synthorg/api/state.pytests/unit/api/auth/test_controller.pytests/unit/api/controllers/test_ws.pysrc/synthorg/api/auth/controller.py
web/src/stores/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Vue stores: Pinia stores (auth, agents, tasks, budget, messages, meetings, approvals, websocket, analytics, company, providers)
Files:
web/src/stores/websocket.ts
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Markers:@pytest.mark.unit,@pytest.mark.integration,@pytest.mark.e2e,@pytest.mark.slow
Parametrize: Prefer@pytest.mark.parametrizefor testing similar cases
Tests must usetest-provider,test-small-001, etc. for vendor-neutral test identifiers
For timing-sensitive tests, mocktime.monotonic()andasyncio.sleep()to make them deterministic instead of widening timing margins
Files:
tests/unit/api/auth/test_ticket_store.pytests/unit/api/auth/test_controller.pytests/unit/api/controllers/test_ws.py
🧠 Learnings (19)
📚 Learning: 2026-03-16T20:29:28.807Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T20:29:28.807Z
Learning: Applies to web/src/composables/**/*.{ts,tsx} : Vue composition functions: reusable composition functions (useAuth, useLoginLockout, usePolling, useOptimisticUpdate, useWebSocketSubscription)
Applied to files:
web/src/composables/useWebSocketSubscription.tsweb/src/__tests__/composables/useWebSocketSubscription.test.tsweb/src/views/MeetingLogsPage.vue
📚 Learning: 2026-03-16T20:29:28.807Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T20:29:28.807Z
Learning: Applies to web/src/stores/**/*.{ts,tsx} : Vue stores: Pinia stores (auth, agents, tasks, budget, messages, meetings, approvals, websocket, analytics, company, providers)
Applied to files:
web/src/__tests__/composables/useWebSocketSubscription.test.tsweb/src/stores/websocket.tsweb/src/views/MeetingLogsPage.vueweb/src/__tests__/stores/websocket.test.ts
📚 Learning: 2026-03-16T20:29:28.807Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T20:29:28.807Z
Learning: Applies to src/synthorg/**/*.py : Functions: < 50 lines, files < 800 lines
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to src/synthorg/**/*.py : Every module with business logic MUST have: `from synthorg.observability import get_logger` then `logger = get_logger(__name__)`. Never use `import logging` / `logging.getLogger()` / `print()` in application code. Variable name: always `logger` (not `_logger`, not `log`).
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to src/synthorg/**/*.py : Structured kwargs in logging: always `logger.info(EVENT, key=value)` — never `logger.info('msg %s', val)`.
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-16T20:29:28.807Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T20:29:28.807Z
Learning: Applies to src/synthorg/**/*.py : Structured kwargs: always `logger.info(EVENT, key=value)` — never `logger.info("msg %s", val)`
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-16T20:29:28.807Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T20:29:28.807Z
Learning: Applies to src/synthorg/**/*.py : Every module with business logic MUST have: `from synthorg.observability import get_logger` then `logger = get_logger(__name__)`
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to src/synthorg/**/*.py : Event names: always use constants from domain-specific modules under synthorg.observability.events (e.g., PROVIDER_CALL_START from events.provider, BUDGET_RECORD_ADDED from events.budget, etc.). Import directly: `from synthorg.observability.events.<domain> import EVENT_CONSTANT`.
Applied to files:
CLAUDE.mdsrc/synthorg/observability/events/api.py
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to src/synthorg/**/*.py : All error paths must log at WARNING or ERROR with context before raising.
Applied to files:
CLAUDE.mdsrc/synthorg/api/auth/controller.py
📚 Learning: 2026-03-16T20:29:28.807Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T20:29:28.807Z
Learning: Applies to src/synthorg/**/*.py : Event names: always use constants from the domain-specific module under `synthorg.observability.events`
Applied to files:
CLAUDE.mdsrc/synthorg/observability/events/api.py
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to src/synthorg/**/*.py : All state transitions must log at INFO.
Applied to files:
CLAUDE.mdsrc/synthorg/api/auth/ticket_store.py
📚 Learning: 2026-03-16T20:29:28.807Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T20:29:28.807Z
Learning: Applies to src/synthorg/**/*.py : All error paths must log at WARNING or ERROR with context before raising
Applied to files:
CLAUDE.mdsrc/synthorg/api/auth/controller.py
📚 Learning: 2026-03-16T20:29:28.807Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T20:29:28.807Z
Learning: Applies to src/synthorg/**/*.py : All state transitions must log at INFO
Applied to files:
CLAUDE.mdsrc/synthorg/api/auth/ticket_store.py
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to web/** : Web dashboard: Node.js 20+, dependencies in web/package.json (Vue 3, PrimeVue, Tailwind CSS, Pinia, VueFlow, ECharts, Axios, vue-draggable-plus, Vitest, fast-check, ESLint, vue-tsc).
Applied to files:
web/src/views/MeetingLogsPage.vue
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to tests/**/*.py : Test timeout: 30 seconds per test.
Applied to files:
tests/unit/api/auth/test_ticket_store.pytests/unit/api/auth/test_controller.pytests/unit/api/controllers/test_ws.py
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to tests/**/*.py : Test markers: pytest.mark.unit, pytest.mark.integration, pytest.mark.e2e, pytest.mark.slow. Coverage: 80% minimum (enforced in CI).
Applied to files:
tests/unit/api/auth/test_ticket_store.pytests/unit/api/auth/test_controller.py
📚 Learning: 2026-03-16T20:29:28.807Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T20:29:28.807Z
Learning: Applies to tests/**/*.py : For timing-sensitive tests, mock `time.monotonic()` and `asyncio.sleep()` to make them deterministic instead of widening timing margins
Applied to files:
tests/unit/api/auth/test_ticket_store.pytests/unit/api/auth/test_controller.py
📚 Learning: 2026-03-16T20:29:28.807Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T20:29:28.807Z
Learning: Timeout: 30 seconds per test
Applied to files:
tests/unit/api/auth/test_ticket_store.pytests/unit/api/controllers/test_ws.py
📚 Learning: 2026-03-16T20:29:28.807Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T20:29:28.807Z
Learning: Applies to src/synthorg/**/*.py : Models: Pydantic v2 (`BaseModel`, `model_validator`, `computed_field`, `ConfigDict`). Use `computed_field` for derived values instead of storing + validating redundant fields
Applied to files:
src/synthorg/api/auth/controller.py
🧬 Code graph analysis (11)
web/src/api/types.ts (1)
src/synthorg/api/auth/controller.py (1)
WsTicketResponse(156-167)
web/src/__tests__/composables/useWebSocketSubscription.test.ts (2)
web/src/api/types.ts (1)
WsEventHandler(701-701)web/src/composables/useWebSocketSubscription.ts (1)
useWebSocketSubscription(47-105)
src/synthorg/api/auth/__init__.py (2)
src/synthorg/api/state.py (1)
ticket_store(245-247)src/synthorg/api/auth/ticket_store.py (2)
TicketLimitExceededError(35-36)WsTicketStore(58-172)
web/src/stores/websocket.ts (2)
web/src/api/endpoints/auth.ts (1)
getWsTicket(32-35)web/src/utils/logging.ts (1)
sanitizeForLog(2-11)
src/synthorg/api/auth/ticket_store.py (1)
src/synthorg/api/auth/models.py (1)
AuthenticatedUser(69-88)
tests/unit/api/auth/test_ticket_store.py (1)
src/synthorg/api/auth/ticket_store.py (6)
TicketLimitExceededError(35-36)WsTicketStore(58-172)create(84-115)ttl_seconds(80-82)validate_and_consume(117-150)cleanup_expired(152-172)
src/synthorg/api/controllers/ws.py (4)
src/synthorg/api/auth/models.py (1)
AuthenticatedUser(69-88)web/src/api/types.ts (1)
HumanRole(46-51)src/synthorg/api/state.py (1)
ticket_store(245-247)src/synthorg/api/auth/ticket_store.py (1)
validate_and_consume(117-150)
src/synthorg/api/state.py (1)
src/synthorg/api/auth/ticket_store.py (1)
WsTicketStore(58-172)
tests/unit/api/auth/test_controller.py (4)
tests/unit/api/conftest.py (2)
test_client(193-224)make_auth_headers(111-137)tests/unit/api/test_guards.py (1)
bare_client(17-20)src/synthorg/api/state.py (1)
ticket_store(245-247)src/synthorg/api/auth/ticket_store.py (1)
validate_and_consume(117-150)
tests/unit/api/controllers/test_ws.py (4)
src/synthorg/api/auth/models.py (1)
AuthMethod(11-16)web/src/api/types.ts (1)
HumanRole(46-51)src/synthorg/api/state.py (1)
ticket_store(245-247)src/synthorg/api/app.py (1)
_build_middleware(648-680)
src/synthorg/api/auth/controller.py (3)
src/synthorg/api/auth/models.py (3)
AuthenticatedUser(69-88)AuthMethod(11-16)User(19-40)src/synthorg/api/state.py (1)
ticket_store(245-247)src/synthorg/api/auth/ticket_store.py (3)
TicketLimitExceededError(35-36)create(84-115)ttl_seconds(80-82)
🔇 Additional comments (33)
src/synthorg/api/auth/models.py (1)
16-16: Auth method extension is consistent.
WS_TICKETis added cleanly and matches the enum’s existing naming/value pattern.docs/roadmap/index.md (1)
15-15: Roadmap update matches the implemented auth flow.docs/architecture/index.md (1)
40-40: Architecture table is aligned with the new WebSocket auth design.web/src/api/endpoints/auth.ts (1)
32-35:getWsTicket()follows the existing endpoint wrapper pattern correctly.web/src/api/types.ts (1)
178-181:WsTicketResponsetype matches the backend contract.docs/design/operations.md (1)
963-963: Operations API surface documentation is correctly updated for WS ticket auth.Also applies to: 977-979
src/synthorg/api/auth/__init__.py (1)
6-16: Public auth exports are clean and consistent.Exposing
WsTicketStoreandTicketLimitExceededErrorhere keeps the auth package API coherent for callers and tests.src/synthorg/api/state.py (1)
103-247: AppState ticket-store integration looks solid.Initializing a dedicated
WsTicketStoreinAppStateand exposing it via a typed property is a good fit for the per-process WS ticket flow.src/synthorg/api/app.py (2)
152-221: Cleanup-task lifecycle handling is robust.Creating the cleanup task at startup, logging unexpected task death, and cancel-awaiting it on shutdown is correctly implemented.
656-674: Middleware exclusion logic is correctly hardened.Preserving explicit
exclude_pathsvalues and forcibly excluding the WS upgrade path closes the auth-boundary gap cleanly.tests/unit/api/auth/test_ticket_store.py (1)
53-310: Strong WS ticket-store test coverage.The suite thoroughly exercises TTL validation, single-use behavior (including concurrent consumers), and cleanup/expiry transitions with deterministic time control.
web/src/views/MeetingLogsPage.vue (1)
18-29: Composable migration is a good cleanup.Using
useWebSocketSubscriptionhere removes page-level WS wiring noise and keeps subscription lifecycle concerns centralized.src/synthorg/observability/events/api.py (1)
53-57: WS ticket observability events are well-scoped.These constants cleanly cover issuance/consumption/expiry/invalid/cleanup lifecycle points.
web/src/__tests__/composables/useWebSocketSubscription.test.ts (1)
13-48: Async mount test orchestration is improved.Capturing
mountedPromiseand awaiting it in async setup scenarios makes these composable tests significantly more reliable.Also applies to: 72-296
src/synthorg/api/auth/controller.py (2)
156-168: LGTM! Well-structured response model.The
WsTicketResponsemodel correctly usesfrozen=True,NotBlankStrfor the ticket field, andField(gt=0)forexpires_invalidation.
431-488: LGTM! Secure ws-ticket endpoint implementation.The endpoint correctly:
- Validates that the user is authenticated (lines 446-454)
- Enforces JWT-only authentication (lines 456-464), rejecting API keys per the PR requirements
- Logs all error paths at WARNING before raising
- Uses
max(1, math.ceil(...))to ensureexpires_inis never zero- Handles
TicketLimitExceededErrorgracefully with a 409 responseThe
noqa: B904on line 479 is appropriate since re-raising would expose internal details.CLAUDE.md (2)
117-119: LGTM! Documentation accurately reflects the new WS ticket authentication.The package structure description correctly documents the new authentication subpackage with JWT + API key + WS ticket auth support.
199-199: LGTM! Event constants properly documented.The new
API_WS_TICKET_ISSUED,API_WS_TICKET_CONSUMED,API_WS_TICKET_EXPIRED,API_WS_TICKET_INVALID, andAPI_WS_TICKET_CLEANUPevents are correctly added to the observability events reference.tests/unit/api/auth/test_controller.py (1)
330-380: LGTM! Comprehensive test coverage for WS ticket endpoint.The test class properly:
- Includes required markers (
@pytest.mark.timeout(30),@pytest.mark.unit)- Tests the happy path with ticket generation and expiration
- Validates authentication requirement
- Verifies role-based access (observer can get tickets)
- Confirms single-use semantics through the ticket store
The tests correctly verify that
auth_methodis set to"ws_ticket"after consumption.web/src/stores/websocket.ts (3)
44-56: LGTM! Well-implemented connect deduplication.The generation-based deduplication correctly:
- Prevents concurrent
connect()calls from racing- Only clears
connectPromisewhen the generation matches, so stalefinallycallbacks are no-ops- Returns the existing promise for callers to await
58-84: LGTM! Robust ticket-based connection with proper race guards.The implementation correctly:
- Fetches a one-time ticket before opening the WebSocket
- Skips reconnect on 401 errors to allow the auth interceptor to handle redirection
- Guards against stale connect attempts after the async ticket fetch (line 82-84)
- Documents the reason for using a query parameter (browsers can't set headers on WS upgrade)
178-196: LGTM! Disconnect properly invalidates in-flight connections.Incrementing
connectGenerationand clearingconnectPromiseensures any in-flightdoConnect()will bail out at the generation check, preventing zombie connections from resurrecting after an intentional disconnect.src/synthorg/api/auth/ticket_store.py (3)
1-56: LGTM! Well-documented module with clear constraints.The module docstring correctly documents:
- Ephemeral nature of tickets (no persistence across restarts)
- Use of
time.monotonic()for clock-drift immunity- Per-process limitation requiring single-worker deployment
The
_TicketEntrymodel is appropriately frozen and internal.
72-115: LGTM! Secure ticket creation with proper validation.The implementation correctly:
- Validates TTL with
math.isfinite()check (line 73)- Filters expired tickets from the per-user cap calculation (lines 94-98)
- Uses 256-bit entropy via
secrets.token_urlsafe(32)- Logs ticket issuance at INFO level with structured kwargs
117-172: LGTM! Atomic single-use consumption and cleanup.The
validate_and_consumemethod correctly usesdict.pop()for atomic removal before expiry checks, which is safe in the single-threaded asyncio event loop as documented. Thecleanup_expiredmethod properly logs state transitions at INFO level.tests/unit/api/controllers/test_ws.py (2)
179-189: LGTM! Good use of parametrize for similar test cases.Refactoring
test_non_dict_json_returns_errorto use@pytest.mark.parametrizeimproves test clarity and follows the coding guidelines preference for parametrize.
192-308: LGTM! Thorough test coverage for WS ticket authentication.The
TestWsTicketAuthclass properly:
- Includes required markers (
@pytest.mark.timeout(30),@pytest.mark.unit)- Tests ticket endpoint functionality and single-use semantics
- Validates identity preservation through the ticket
- Verifies the
/wspath is excluded from auth middleware- Ensures
_READ_ROLESincludes allHumanRolevalues- Confirms close codes are in RFC 6455 application range (4000-4999)
- Tests rejection of missing and invalid tickets
src/synthorg/api/controllers/ws.py (6)
44-46: LGTM! Appropriate RFC 6455 close codes.Using application-layer codes 4001 (auth failed) and 4003 (forbidden) correctly falls within the RFC 6455 §7.4.2 range (4000-4999) for application-specific close codes.
49-74: LGTM! Clean ticket validation with proper logging.The
_validate_tickethelper correctly:
- Logs missing tickets at WARNING before closing
- Delegates to
ticket_store.validate_and_consume()which handles its own logging- Closes with an appropriate application-layer code (4001)
77-112: LGTM! Defense-in-depth role validation.The explicit
HumanRolevalidation provides a safety net against future changes to the role model, even though Pydantic already validates the role field. The warning logs include the actual role value for debugging.
115-167: LGTM! Robust event filtering with proper type validation.The
_on_eventhelper correctly:
- Handles both
JSONDecodeErrorandTypeErrorfrom malformed backend data- Validates the event is a dict before accessing fields
- Validates the payload is a dict before filter matching (addresses the past review comment)
- Handles
WebSocketDisconnectgracefully during send
170-214: LGTM! Clean ws_handler with ticket-based pre-auth.The handler correctly:
- Validates the ticket and role before accepting the connection
- Attaches the user to
socket.scope["user"]for downstream access- Logs connection with
user_idcontext- Properly cleans up the subscriber on disconnect
318-367: LGTM! Well-documented filter semantics.The
_handle_subscribedocstring clearly documents the three-way filter semantics:
None(key absent) → leave existing filters{}(empty object) → clear filters{...}(non-empty) → set new filtersThis addresses the past review comment about the filter behavior.
- Add disposed flag to useWebSocketSubscription to prevent subscribe/ handler wiring after component unmounts during async connect() - Soften security.md WS ticket description — clarify the benefit is preventing long-lived JWT leakage, not eliminating all URL credentials
🤖 I have created a release *beep* *boop* --- ## [0.3.1](v0.3.0...v0.3.1) (2026-03-17) ### Features * **api:** RFC 9457 Phase 2 — ProblemDetail and content negotiation ([#496](#496)) ([30f7c49](30f7c49)) * **cli:** verify container image signatures and SLSA provenance on pull ([#492](#492)) ([bef272d](bef272d)), closes [#491](#491) * **engine:** implement context budget management in execution loops ([#520](#520)) ([181eb8a](181eb8a)), closes [#416](#416) * implement settings persistence layer (DB-backed config) ([#495](#495)) ([4bd99f7](4bd99f7)), closes [#450](#450) * **memory:** implement dual-mode archival in memory consolidation ([#524](#524)) ([4603c9e](4603c9e)), closes [#418](#418) * migrate config consumers to read through SettingsService ([#510](#510)) ([32f553d](32f553d)) * **settings:** implement settings change subscriptions for service hot-reload ([#526](#526)) ([53f908e](53f908e)), closes [#503](#503) * **settings:** register API config in SettingsService with 2-phase init ([#518](#518)) ([29f7481](29f7481)) * **tools:** add SSRF prevention for git clone URLs ([#505](#505)) ([492dd0d](492dd0d)) * **tools:** wire RootConfig.git_clone to GitCloneTool instantiation ([#519](#519)) ([b7d8172](b7d8172)) ### Bug Fixes * **api:** replace JWT query parameter with one-time ticket for WebSocket auth ([#493](#493)) ([22a25f6](22a25f6)), closes [#343](#343) ### Documentation * add uv cache lock contention handling to worktree skill ([#500](#500)) ([bd85a8d](bd85a8d)) * document RFC 9457 dual response formats in OpenAPI schema ([#506](#506)) ([8dd2524](8dd2524)) ### Maintenance * upgrade jsdom from 28 to 29 ([#499](#499)) ([1ea2249](1ea2249)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Summary
POST /api/v1/auth/ws-ticketendpoint exchanges a valid JWT for a 30-second, single-use ticket (256-bit entropy viasecrets.token_urlsafe)connect()callsMeetingLogsPageto useuseWebSocketSubscriptioncomposable (was the last view using raw store calls)Review coverage
Pre-reviewed by 12 agents (security, code-reviewer, conventions-enforcer, logging-audit, api-contract-drift, test-quality, async-concurrency, issue-resolution, docs-consistency, frontend-reviewer, silent-failure-hunter, type-design-analyzer). 17 findings triaged and addressed:
_cleanup_expiredto publiccleanup_expiredmethodAPI_WS_TICKET_ISSUEDlog emissionconnect()race guard (connectPromisededup) in WS storeuseWebSocketSubscriptioncomposablevalidate_and_consume(pop-first design)Test plan
uv run python -m pytest tests/ -m unit -n auto)npm --prefix web run test)uv run ruff check src/ tests/+npm --prefix web run lintuv run mypy src/ tests/+npm --prefix web run type-checkCloses #343