refactor(models): centralize router Pydantic models in models.py (INV-14, #654)#1308
Open
AndriiPasternak31 wants to merge 3 commits into
Open
refactor(models): centralize router Pydantic models in models.py (INV-14, #654)#1308AndriiPasternak31 wants to merge 3 commits into
AndriiPasternak31 wants to merge 3 commits into
Conversation
This was referenced Jun 22, 2026
Open
|
Resolve by running |
…-14, #654) Move 97 request/response BaseModel classes from 32 routers into src/backend/models.py (Architectural Invariant #14), plus the fan_out / loops / webhooks module-level constants their validators reference. Each class moved verbatim — validators, default_factory, and Config preserved. Behaviour preserved, proven two ways the move could silently break it: - Normalized app.openapi() snapshot diff: 330 paths / 198 schemas, byte-identical origin/dev vs branch (schema-level proof). - Validator-preservation unit tests for the 5 behaviour-bearing files (fan_out, loops, canary, audit_log, schedules) — the field_validator logic, default_factory dicts, and Config.from_attributes an OpenAPI diff can't see. One documented exception, allowlisted in the new static guard: canary.py::RunCycleRequest evaluates INVARIANTS (the canary library) in a Field(description=...) at class-definition time, and the canary library imports TaskExecutionStatus back from models — relocating it would force models.py to `from canary import ...`, inverting the dependency direction of a module meant to be a low-level leaf everything imports from. Flat models.py (not a package): a models/ dir would break tests/conftest.py's by-file model preload and the Dockerfile's by-file COPY *.py (the #1033 crash-loop class). Scope is router models only — db_models.py and adapters/base.py are intentional separate homes. New / changed: - tests/unit/test_models_centralized.py: static guard (no BaseModel under routers/ except the documented allowlist) + planted-violation tests - tests/unit/test_router_model_validators.py: validator / default_factory / Config preservation negatives - tests/unit/test_telegram_webhook_backfill.py: load the real models module in the standalone settings.py stub (was already red on dev) and stub the #1197 services.agent_service.capabilities import -> green - docs/memory/architecture.md: Invariant #14 scope note + re-measured router/service/agents counts Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
38d28e3 to
a6da83f
Compare
- feature-flows.md: Recent Updates row for the router-model centralization (Invariant #14), noting behavior-preserved / OpenAPI-byte-identical and the documented canary.RunCycleRequest allowlist exception. - security-reports: CSO --diff report for PR #1308 (37 files); zero findings across all categories. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
INV-14 centralized EmailStr-bearing models (SendMessageRequest, etc.) into models.py, which the db layer imports non-tolerantly (db/schedules.py -> `from models import TaskExecutionStatus`). Building those schemas at import time now requires email-validator, so the unit-CI job failed at conftest collection: "ImportError: email-validator is not installed". The backend Docker image already installs pydantic[email]>=2.10.4; tests/requirements-test.txt had bare pydantic. Align the test env with production. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
vybe
requested changes
Jun 24, 2026
vybe
left a comment
Contributor
There was a problem hiding this comment.
Validated via /validate-pr — clean refactor (CSO verdict CLEAR, behavior-preserving, dedicated centralization tests, Closes #654). Blocking only on a rebase: this is now CONFLICTING with dev after #1333 and #1317 merged, both of which added new models:
- #1333 — cancelled-status models in
routers/agents.py/chat.py/paid.py/public.py+models.py - #1317 — access models in
routers/sharing.py
Please rebase onto current dev and relocate those newly-added models into models.py so INV-14 centralization stays complete. Otherwise good to go.
— posted via /validate-pr
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
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.
What
Clears Architectural Invariant #14 (Pydantic models centralized in
models.py) for issue #654: moves 97 request/responseBaseModelclasses out of 32 routers intosrc/backend/models.py, plus thefan_out/loops/webhooksmodule-level constants their validators reference. Each class moved verbatim — validators,default_factory, andConfigpreserved.#654was a stalevalidate-architecturedrift report flagging INV-7 / INV-8 / INV-14. Scope (confirmed in plan review): this PR clears INV-14 only (backend, mechanical) + applies the D1/D2 doc fixes. INV-7 (frontendapi.js) and INV-8 (auth sprawl) are split into separate child issues — they're owned by other areas and need behavioral/security work.Why flat
models.py(not amodels/package)A package dir would break two by-file mechanisms invisible to source-only CI (the #1033 class):
tests/conftest.py's by-file model preload, and the Dockerfile's by-fileCOPY *.py. Flat keeps both working. Scope is router models only —db_models.py(95) andadapters/base.py(4) are intentional separate homes.Behaviour preserved — two proofs
The move is mechanically simple but not purely build-time: a careless cut-paste can change runtime validation while imports + schema still pass. Both gaps are covered:
app.openapi()snapshot diff —origin/devvs branch: 330 paths / 198 schemas, byte-for-byte identical. Schema-level proof.tests/unit/test_router_model_validators.py) — the@field_validatorlogic,default_factorydicts, andConfig.from_attributesthat an OpenAPI diff cannot see, for the 5 behaviour-bearing files (fan_out, loops, canary, audit_log, schedules).One documented exception (allowlisted in the guard)
canary.py::RunCycleRequeststays in its router: it evaluatesINVARIANTS(from thecanarylibrary) in aField(description=…)at class-definition time, and thecanarylibrary importsTaskExecutionStatusback frommodels. Relocating it would forcemodels.pytofrom canary import …, inverting the dependency direction of a module meant to be a low-level leaf everything imports from. The new static guard allowlists exactly this one model (and fails if the allowlist goes stale).Tests / docs
tests/unit/test_models_centralized.py— static guard: noBaseModelunderrouters/except the documented allowlist; + planted-violation tests.tests/unit/test_router_model_validators.py— 19 validator / default_factory / Config-preservation negatives.test_telegram_webhook_backfill.py— its standalonesettings.pyloader now uses the realmodelsmodule (a safe leaf) instead of a partial fake, and stubs theservices.agent_service.capabilitiesimport (bug: agent creation crashes with opaque ValueError on non-integer CPU in template resources #1197). This test was already red onorigin/dev(bug: agent creation crashes with opaque ValueError on non-integer CPU in template resources #1197 standalone-load gap); it is now green.docs/memory/architecture.md— Invariant Add internal health route, without which main didn't start #14 scope note + re-measured router/service/agents.pycounts.Verification
python -c "import main"— clean.tests/unitsuite: only the two pre-existing failure clusters remain (test_1115_schedules_summary,test_login_rate_limit_split— both confirmed failing identically on pristineorigin/dev). Zero new failures; +23 passing tests (19 new + 4 telegram fixed).ruff: error breakdown byte-identical toorigin/dev(zero new lint).verify-localis optional here — flat move, no new module dir, no dep/Docker change (models.pyis already in theCOPY *.py).🤖 Generated with Claude Code
Closes #654 (narrowed to INV-14 + docs). INV-7 → #1309, INV-8 → #1310.