refactor(chat): split routers/chat.py into thin handlers + bounded services (#1483)#1695
Merged
Conversation
Pin the observable behavior of routers/chat.py's two monsters and the Invariant #4 route-order landmine BEFORE any code moves (issue-mandated TDD spine), so every extraction step can be held byte-identical. - test_1483_run_chat_and_finalize_characterization.py: the sync-chat execute+finalize path — SUCCESS (UUID-validated claude_session_id, activity completion, idempotency snapshot, slot release), the full SUB-003 429/auth switch matrix (switch-authoritative / switch-raised / switch-real-errored / no-switch — the top-risk branch), budget exhausted, and #678 partial-metadata salvage. Anchored on sys.modules[fn.__module__] so patch targets follow the move. - test_1483_execute_parallel_task_characterization.py: the /task monster end-to-end — sync immediate/backlog(drain + row-reconstruction), async queued-202 (#914 shape) / accepted-202, #1672 resume 400/404, SELF-EXEC-001 403 spoof, idempotency replay 409/snapshot, #1444 chat_persist_failed marker, the upload-502-keeps-idem quirk (RD11), and the #1578 reserved-event triggered_by="event" sinks. - test_1483_route_order.py: proves GET /executions/running resolves to chat's get_agent_running_executions (not schedules' get_execution) via the app's real match order — OpenAPI is blind to route order (#1483 §5). 35 new tests green against unmodified code. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…#1483 Move `_persist_chat_session` + `_persist_and_broadcast_chat_session` out of routers/chat.py into services/chat_persistence_service.py (Invariant #1 — the router holds no DB/persistence logic). Byte-for-byte preserved: the #1444 SUCCESS-guard, the IDOR owner-check that falls through to the caller's own session, and the fail-loud non-fatal ERROR log carrying only agent name + execution_id + exception type (no user content, no re-raise). The chat_response_ready WebSocket broadcast moves with it and gets its own set_websocket_manager wired in main.py (per-service setter pattern). The router imports the service module and delegates at both call sites (the sync /task branch and the async wrapper). No route/model/status change; OpenAPI byte-identical. Tests repointed in-commit (module-identity): test_1444 direct calls + the fail-loud caplog logger name (now services.chat_persistence_service, RD13); test_async_task_persistence patches persist_chat_session + mirrors the service _websocket_manager for the chat_response_ready broadcast. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…1483 Move the request-admission orchestration out of routers/chat.py (Invariant #1). New leaf services: - services/chat_signals.py — dependency-free domain signals the chat services return/raise instead of touching FastAPI: ChatAdmission / ChatExecutionContext (relocated NamedTuples), ChatAdmissionReplay (idempotent-replay outcome), and ChatDispatchError (HTTP-free error carrying the status/detail/headers the router maps 1:1). - services/dispatch_admission_service.py — the /chat admission gate (admit_chat_request: idempotency begin/replay + audit, the #526 F1 pure-state breaker read, CapacityManager.acquire) and the shared /task idempotency begin/replay (begin_task_idempotency + audit_idempotent_replay). HTTP-free: it returns ChatAdmission/ChatAdmissionReplay and raises the already-domain CircuitOpen/CapacityFull/EphemeralBudgetExhausted; the thin router maps them to 503/429/410 (the FAILED-row-write + raise stay in the router's _raise_* helpers, RD-E12). Named "dispatch" not "chat" because it serves both endpoints (RD2). Preserved byte-for-byte: idempotency release-vs-keep (upfront deny → fail), the audit rows, the breaker fast-fail, and every deny status/detail. OpenAPI byte-identical (372 paths). Route-order guard green. Tests repointed in-commit (module-identity): admission collaborators now patched at dispatch_admission_service in test_chat_admission / test_946 / test_1578 / the #1483 /task characterization suite; the direct admit-helper test imports from the new service + chat_signals. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
#1483) Move the /chat business logic out of routers/chat.py into services/chat_execution_service.py (Invariant #1), decomposing the CC-57 _run_chat_and_finalize monster (AC #2): - prepare_chat_execution (was _prepare_chat_execution): exec record + subscription + collaboration broadcast/activity + session + chat-start activity + user-msg log. - broadcast_collaboration_event (moved, own set_websocket_manager wired in main.py). - run_chat_turn (was _run_chat_and_finalize, declared TRANSITIONAL — RD15) split into build_chat_payload / _finalize_chat_success / _parse_agent_http_error / _finalize_budget_exhausted / _finalize_http_failure / _apply_sub003_autoswitch. Each ≤ CC 20. HTTP-free: failure paths raise ChatDispatchError, mapped 1:1 by the thin chat_with_agent handler; the lone fastapi touch is the defensive `except HTTPException: raise` preserving SUB-003 propagate-unchanged semantics. Byte-for-byte preserved: MEM-001 runtime-aware prompt, #686 mark-dispatched-before-POST, UUID-validated claude_session_id, #1332 read-before-close mirroring, #678 partial- metadata salvage, the full SUB-003 429/auth switch matrix, and the finally slot+idem release. routers/chat.py: 2756 → 1889 lines; unused imports trimmed. OpenAPI byte-identical (372 paths). Tests repointed in-commit: the run_chat_turn char suite + test_chat_admission's direct finalize/prepare tests (now raise ChatDispatchError, patch _CE collaborators); the test_chat_dispatched_marker AST/mirror guards now parse chat_execution_service.py. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Decompose the CC-82 execute_parallel_task monster (AC #2). The router keeps only the request-boundary guards (container 404/503, #1672 resume IDOR gate, #1068 timeout normalization — the redis-via-router helper stays here per RD10) and a thin call that maps ChatDispatchError → HTTPException / ChatAdmissionReplay → 409/200. Everything else — derive/spoof (403), idempotency begin/replay, file upload (502), row+activity creation, and the async/sync fork — moves to chat_execution_service, each function ≤ CC 20 / ≤ 150 lines: dispatch_parallel_task, derive_source_and_trigger, process_task_file_uploads, create_task_execution_and_activities, _acquire_task_capacity, _map_task_failure, _dispatch_async, _dispatch_sync{,_immediate,_backlog}, run_async_task, complete_collaboration_activity, finalize_self_task (kept WHOLE, RD9). execute_parallel_task: CC 82 → 13; routers/chat.py: 2756 → 1132 lines. The sync paths still delegate to task_execution_service.execute_task (never a 2nd applier, RD1). Circuit/ephemeral 503/410 + FAILED-write mirrored in the service (single place per path — no double-write, RD-E12). #1578 reserved-event tag flows through every sink; #914 queued-202 shape, #1444 chat_persist_failed, and the upload-502 no-idempotency-fail quirk (RD11) preserved. OpenAPI byte-identical. - Repoint the production importer backlog_service.py:265 → services.chat_execution_service.run_async_task (kept LAZY — breaks a real cycle). - The chat router no longer holds a WebSocket manager (all broadcasts moved to the two services, each with its own setter — §7 minimize-WS-globals); main.py wiring updated. Unused router imports trimmed. - Tests repointed in-commit to the new collaborator modules (module-identity): the /task char suite, test_946, test_1578, test_backlog (fake-module + AST guard), test_1332, test_1457, test_async_task_persistence, test_1444, test_1672. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…#1483 Decompose terminate_agent_execution (CC 22, 177 lines — the last function over the AC #2 gate) into chat_execution_service, HTTP-free: terminate_execution (orchestrator), _cancel_queued_if_queued (BACKLOG-001), _proxy_terminate_and_finalize (agent-proxy + force-release + #679 CANCELLED CAS + final activity), _close_dispatch_activity_cancelled (#1332). The router handler is now a thin mapper (ChatDispatchError → HTTPException). The agent-proxy stays inline rather than reusing task_execution_service.terminate_execution_on_agent — that helper returns a bool and swallows connect/timeout, so reusing it would drop the 502/504/404 the router surfaces (a behavior change). #679 already-finished-vs-terminated and the #1332 CAS-gated dispatch-activity close preserved byte-for-byte. Result: EVERY function in routers/chat.py + the chat services is now ≤ CC 20 and ≤ 150 lines (AC #2 met). routers/chat.py: 2756 → 988 lines. OpenAPI byte-identical. Unused router imports trimmed. test_679_terminate / test_1332 terminate patches repointed to chat_execution_service (httpx.AsyncClient stays a global patch). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…1483 §7) Static AST check: any `set_*_ws_manager` alias imported into main.py must also be called at startup wiring. Mitigates the silent-no-op hazard the split introduced by fanning the chat broadcasts across chat_execution_service / chat_persistence_service — a missed setter leaves the module global None and the broadcast vanishes (invisible to the OpenAPI diff and manager-patching tests). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- architecture.md: add chat_execution_service / dispatch_admission_service / chat_persistence_service / chat_signals to the Backend Services catalog; note task_execution_service stays the single terminal applier; repoint the chat_sessions/chat_messages persistence citation to chat_persistence_service. - feature-flows.md: Recent Updates row for #1483. - authenticated-chat-tab.md: persistence now in chat_persistence_service (logger name change); async wrapper now chat_execution_service.run_async_task. - task-execution-service.md: sync-chat sibling note — run_chat_turn is the transitional divergent applier; /task delegates here; single-applier seams untouched; convergence is a follow-up. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…services The async /task wrapper (run_async_task) and #1444 persistence now live in chat_execution_service / chat_persistence_service; the router keeps only thin handlers. Historical Recent-Updates rows are left as dated records. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…tened route table test_chat_router_precedes_schedules_router_in_include_order failed on the pinned fastapi==0.115.6 (and 0.124.2): its _include_index helper only handled an _IncludedRouter-wrapped app.routes, but include_router flattens routes into plain APIRoute entries in both versions — so the endpoint lookup always fell through to AssertionError. Reuse the same both-shapes handling already in _flatten_in_match_order (flattened APIRoute in app.routes, or legacy _IncludedRouter wrapper). The three load-bearing Match.FULL resolution tests were unaffected. 4/4 now green on fastapi 0.115.6 and 0.124.2. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…1483) The two fail-loud persistence tests monkeypatched `chat_mod.db`, but after the #1483 split `persist_chat_session` lives in `chat_persistence_service` and binds `db` at its own import. The `chat_mod` fixture pops `database` from `sys.modules` and reimports `routers.chat`, so `chat_mod.db` is a different DatabaseManager instance than the one the function calls — the injected boom only landed when a full-sweep import order made the two coincide, so the tests passed in-batch but failed standalone. Repoint both patches to `chat_persistence_service.db` (the instance the code under test actually calls). No product-code change; the tests now pass standalone and in-batch, keeping the #1444 fail-loud + PII-safety guard order-independent. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ep (#1483) The full `tests/unit` collection binds `sys.modules['utils']` to the repo's `tests/utils` package (pythonpath lists `tests` before `src/backend`), which has no `password_validation` submodule. This module imports the assembled `main` app at module scope, and `main` → `routers/setup.py` does `from utils.password_validation import ...`, so under the whole-directory collection the import dies with ModuleNotFoundError and the module ERRORS collection — a permanently-red entry in CI's regression-diff (which runs the whole matrix). Standalone, `utils` resolves to `src/backend/utils` and the import is clean. Wrap the module-scope `import main` so a polluted collection produces a loud module-level `pytest.skip(...)` (with a run-standalone reason) instead of a collection error. Keying on the actual `import main` outcome is bulletproof for the success path: standalone still imports cleanly and runs 4/4; the full collection goes from "4437 collected, 1 error" (exit 1) to "4437 collected" (exit 0), with this file skipped in-collection. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
AndriiPasternak31
marked this pull request as ready for review
July 18, 2026 22:08
|
Resolve by running |
vybe
approved these changes
Jul 19, 2026
vybe
left a comment
vybe
left a comment
Contributor
There was a problem hiding this comment.
Validated via /validate-pr: OpenAPI byte-identical proof (same 372 paths/477 ops SHA), task_execution_service untouched (single-applier seam preserved), all #1444/#1578/#914/#1457 guards byte-for-byte, route-order guard standalone-green. New modules all under services/ (no Dockerfile COPY gap). Security greps clean. Approving.
…feature-flows rows)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1483
Summary
Enforces Invariant #1 (Router → Service → DB) for the chat surface by splitting the monolithic
routers/chat.py(2,756 → 988 lines) into a thin, HTTP-only router over four new leaf services:chat_execution_service—/chatsetup + the sync-chatrun_chat_turnapplier (decomposed per plan §3a), the/taskdispatch orchestration (derive/spoof, file upload, row+activity creation, async/sync fork), andterminate_execution. HTTP-free — raisesChatDispatchError, mapped 1:1 at the router.dispatch_admission_service— request admission for/chatand/task: idempotency begin/replay + audit, the/chatpure-state breaker read (feat: per-agent dispatch circuit breaker to prevent backlog poisoning (RELIABILITY-007) #526 F1), andCapacityManager.acquirefor/chat.chat_persistence_service— authenticated/taskchat-session persistence (bug: async chat with save_to_session completes but never creates a chat session #1444):persist_chat_session(SUCCESS-guard + IDOR owner-check + fail-loud no-user-content ERROR log) +persist_and_broadcast_chat_session.chat_signals— dependency-free leaf holding the chat domain signals (ChatAdmission/ChatExecutionContext,ChatAdmissionReplay,ChatDispatchError).task_execution_serviceis byte-untouched — the split delegates/tasksync/async to the existingexecute_task/apply_resultand never introduces a second/taskapplier, keeping the pull-migration single-applier seams intact.run_chat_turnis the ONE remaining divergent terminal writer (sync-chat does its ownchat_sessionspersistence +mode="chat"prompt thatexecute_taskdoesn't) and is declared transitional — converging it ontoexecute_taskis a tracked follow-up (a genuine behaviour change, out of #1483's scope).OpenAPI proof (no behaviour change)
The API surface is byte-identical before and after the split, reproduced independently in this stage against
origin/dev:origin/dev(pre-split, chat.py 2,756 ln)05f0de8d6a7018a57a6436808322009351205437257212b0b3d33907ccc9f1c505f0de8d6a7018a57a6436808322009351205437257212b0b3d33907ccc9f1c5Same 372 paths / 477 operations, identical SHA256 — first proven at review, independently re-derived here from a fresh
origin/devworktree. (OpenAPI is order-independent, so it is blind to route order — see the dedicated route-order guard below.)Preserved guards (byte-for-byte)
chat_session_id.agent.task.*recursion-break (X-Event-Triggertag honored only with a valid internal secret).begin/replay/fail) on/chatand/task.queued_timeout202 recovery.chat_routerbeforeschedules_router, so/executions/runningis a static hit, notexecution_id="running"), plus a new standalone guard test.Verification
test_1483_execute_parallel_task_characterization,test_1483_run_chat_and_finalize_characterization,test_1483_ws_setters_wired,test_1483_route_order). The route-order gate was hardened at review to survive a flattened route table under the pinnedfastapi 0.115.6; re-confirmed here (also green under a newer fastapi).test_1332,test_1444,test_1457,test_1578,test_1672,test_679,test_946,test_async_task_persistence,test_backlog,test_chat_admission,test_chat_dispatched_marker), 0 failures in-sweep.--skip-agent: PASS (prior stage — full prod-image build + booted sibling stack + unit/integration suites).origin/dev.chat_mod.db, but after the splitpersist_chat_sessionbindsdbinsidechat_persistence_service; thechat_modfixture popsdatabasefromsys.modules, sochat_mod.dbis a differentDatabaseManagerinstance than the one the code calls. The tests passed in-sweep (import order made the two coincide) but failed standalone. Repointed both patches tochat_persistence_service.db(the object the function actually calls) — now green standalone and in-batch, no product-code change.test_1483_route_ordererrored the whole-directorytests/unitcollection (ModuleNotFoundError: utils.password_validation) because its module-scopeimport mainhits thetests/utilsshadow — a permanently-red[E]in CI's regression-diff, which collects the whole tree. Wrapped that import so a polluted-sweep import failure becomes a loud module-levelpytest.skipinstead of a collection error. Verified both modes: standalone4 passed; whole-tree collect-only4437 collected, exit 0, 0 errors (was exit 1, 1 error). Test-only, no product-code change.Reviewer note
Run the route-order guard standalone to exercise the real assertions:
It imports the assembled
mainapp at module scope (cross-router order needs the whole app). In the whole-directorytests/unitcollection an earlier module bindssys.modules['utils']to the repo'stests/utilspackage (pythonpathliststestsbeforesrc/backend), which has nopassword_validationsubmodule — soimport main(viarouters/setup.py'sfrom utils.password_validation import ...) would ModuleNotFound and ERROR collection. The module now self-guards: it wraps thatimport mainand, on the polluted-sweep import failure, issues a loud module-levelpytest.skip(...)(reason: "requires pristine sys.modules — run standalone"). So in the full collection this file is SKIPPED, not errored (whole-tree collect-only:4437 collected, exit 0), and standalone it runs 4/4.Follow-ups (for Andrii to file)
run_chat_turn→execute_taskconvergence — retire the last divergent sync-chat terminal applier onto the sharedexecute_task/apply_resultseam. A real behaviour change (sync-chat persistence +mode="chat"prompt), so it is out of refactor(routers): split routers/chat.py (2,095 lines) and push chat logic into services #1483's no-behaviour-change scope. Belongs under the pull-coordination epic (Epic: Agent Infrastructure #1045/feat: pull / work-stealing coordination (push→pull dispatch) #1081).add_chat_messagesignature) and refactor(auth): INV-8 imperative auth-guard family — consolidate inline auth wiring (#1310) #1683 (its 2 kept session endpoints rebase cleanly onto this split — both endpoints were deliberately KEPT inrouters/chat.py).🤖 Generated with Claude Code