refactor(repositories): let a repository serve a declarative model - #1451
Merged
Merged
Conversation
A repository over `BudgetResetLog`, a declarative table, with a plain Pydantic schema. `make typecheck` fails at this commit: the type arguments of `BaseRepository` must be SQLModel classes. The test passes at runtime, because nothing enforces that bound there.
The budget tables are declarative, so a repository over them could not type-check while `BaseRepository` required SQLModel classes. The model type is now any mapped class, and the two schema types are any Pydantic model. SQLModel tables and schemas still satisfy both.
|
Understand this PR’s impact Explore downstream dependencies and potential security impact with Blast Radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: mozilla-ai/otari/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 2 included reviews per hour; 0 remain after this review. Walkthrough
ChangesDeclarative repository typing
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Refactor 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Title checkExplanation The title clearly describes the repository refactor, uses imperative wording, and is 66 characters long. It does not start with an exact required prefix followed by a colon because it uses
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
tbille
approved these changes
Sep 21, 2026
3 tasks
peteski22
added a commit
that referenced
this pull request
Sep 23, 2026
Step 2 of the files domain move, for the code already on a Unit of Work. `repositories/files/` held four module-level functions that each took the Unit of Work and reached the session through `session_for`, so the queries were in the repository layer but nothing bound them to one table or gave a service something to receive. They become methods of `FileRepository` over `BaseRepository`, which has served a declarative table since #1451, and `files_repositories.py` carries the bundle. The sweeper and the sandbox bridge receive the repository instead of building queries from the Unit of Work, and the bridge gets it from its builder in `api/deps.py`. The sandbox bridge's tests stubbed the ID lookup by patching a module function. They now subclass the repository, so the write path under test is the real one.
peteski22
added a commit
that referenced
this pull request
Sep 24, 2026
Step 2 of the files domain move, for the code already on a Unit of Work. `repositories/files/` held four module-level functions that each took the Unit of Work and reached the session through `session_for`, so the queries were in the repository layer but nothing bound them to one table or gave a service something to receive. They become methods of `FileRepository` over `BaseRepository`, which has served a declarative table since #1451, and `files_repositories.py` carries the bundle. The sweeper and the sandbox bridge receive the repository instead of building queries from the Unit of Work, and the bridge gets it from its builder in `api/deps.py`. The sandbox bridge's tests stubbed the ID lookup by patching a module function. They now subclass the repository, so the write path under test is the real one.
peteski22
added a commit
that referenced
this pull request
Sep 24, 2026
Step 2 of the files domain move, for the code already on a Unit of Work. `repositories/files/` held four module-level functions that each took the Unit of Work and reached the session through `session_for`, so the queries were in the repository layer but nothing bound them to one table or gave a service something to receive. They become methods of `FileRepository` over `BaseRepository`, which has served a declarative table since #1451, and `files_repositories.py` carries the bundle. The sweeper and the sandbox bridge receive the repository instead of building queries from the Unit of Work, and the bridge gets it from its builder in `api/deps.py`. The sandbox bridge's tests stubbed the ID lookup by patching a module function. They now subclass the repository, so the write path under test is the real one.
peteski22
added a commit
that referenced
this pull request
Sep 24, 2026
* refactor(files): give the Files API named response shapes Step 1 of the files domain move in docs/domains.md. The five routes answered with `dict[str, Any]`, built by `to_dict()` and `to_anthropic_dict()` on the `FileObject` row, so the ORM model carried the wire contract and the spec published a bare object for every answer. The six shapes now live in `schemas/files.py`, one pair per answer, with the row-to-shape mapping beside them. Each route declares the pair its caller's headers pick between, so the spec names them and the generated clients see them. The JSON is unchanged: same keys, same values, same order of decision. Regenerated `docs/public/openapi.json` and `web/src/client/schema.ts`. * refactor(files): move FileObject into models/files.py Step 1's companion in the files domain move: the table sat in `models/tools.py`, which the tools domain holds, so the two domains shared a model module and the files table looked like a tool's. The class moves unchanged into `models/files.py`, which joins the import list in `models/__init__.py` so Alembic still sees the table. No column, name or constraint changes, so there is no migration. * refactor(files): make the file queries a repository class Step 2 of the files domain move, for the code already on a Unit of Work. `repositories/files/` held four module-level functions that each took the Unit of Work and reached the session through `session_for`, so the queries were in the repository layer but nothing bound them to one table or gave a service something to receive. They become methods of `FileRepository` over `BaseRepository`, which has served a declarative table since #1451, and `files_repositories.py` carries the bundle. The sweeper and the sandbox bridge receive the repository instead of building queries from the Unit of Work, and the bridge gets it from its builder in `api/deps.py`. The sandbox bridge's tests stubbed the ID lookup by patching a module function. They now subclass the repository, so the write path under test is the real one. * refactor(files): serve the Files API from one files service Steps 3, 4 and 5 of the files domain move, which are one edit here: the five routes built their own queries, committed the session themselves and raised `HTTPException` for every refusal, so moving the rules out of the route is also what moves the commits into Unit of Work blocks and the refusals into the domain. `FileService` in `services/files/` now holds the use cases, built on the request's Unit of Work by a builder in `api/deps.py`. Each verb is one public method over the file repository and the storage port, and the listing's cursor, in both spellings, is the service's. The routes read the request, call one method and map the row to a schema. The refusals become the classes in `exceptions/files_exceptions.py`, each carrying its status, which the registered handler renders, so no route needs a try/except. The deployment's default workspace stays with the organizations domain: the service receives a resolver rather than reaching for it, until that domain moves. Two things change on the wire, both consequences of the error family. A storage or metadata failure answers 500 `{"detail": "Internal server error"}` rather than naming the operation, which is what every other 5xx in this app answers and what the handler does for the whole family; the specific message, with the file's ID, goes to the log where an operator can act on it. Nothing else moves: same statuses, same 4xx messages, same JSON. `api/routes/files.py` comes off the database-import and transaction-control baselines, and the streaming test moves to the service helper it now covers. * refactor(files): fold the flat file module into the files package `services/file_service.py` was the last flat piece of the domain: a top-level module that held a query, so it sat on both the service database-import baseline and the flat-module baseline. Its parts land where they belong. The query becomes `FileService`'s `staged_upload`, which answers with a `StagedFile` rather than a row. The media type and the retention window go to `_metadata.py`, and the sandbox staging types to `_staging.py`. The package's other modules take private names, so the package root is the domain's whole surface. The content normalizer is the one caller outside files. It took a session and a file store so that it could run that query itself; it now takes the files service, which is how one domain reaches another, and stops importing SQLAlchemy and the storage port. Its callers build the service from the same builder the rest of the request path uses. `file_service.py`, `content_normalizer.py` and `_normalize.py` come off the database-import baselines. One thing to watch: resolving a reference now runs inside a Unit of Work block, as every read through a repository must, so it commits the request's session on the way out. The reservation before it is already committed by its own block, so there is nothing else pending, and the sandbox bridge has opened blocks on this same Unit of Work since #1478. * refactor(files): tighten the new files service Four points a review of the previous two commits raised, all inside the code they moved. An upload that has to be undone now removes its bytes best effort, so a store that will not drop them leaves an orphan for an operator to reclaim instead of replacing the refusal that caused the cleanup. The database failure is logged before that cleanup runs, so the cause is on the record whatever the cleanup then does. The page bounds move to the service, which clamps to them, so the ceiling holds for a caller that does not bound its own request; the route bounds its query against the same two constants rather than a second copy. `FileRepository.live` and `any_owned` take a `str` user, not `str | None`. A None was fail-closed, because the column is NOT NULL, but a tenant predicate that admits it reads as unscoped. And the refusal for a cursor that names no position is spelled as two statements rather than a conditional that mixed an exception class with an instance. * test(files): cover a deployment that does not serve files `files_enabled` off answers 404 on every verb, and nothing asserted it. * refactor(files): act on the three-round review of the files service One commit because the changes share files and cannot be staged apart without interactive hunk selection. Each is small and listed here. **`discard` is one transaction.** It read the row in one Unit of Work block and stamped it in a second, so another request could discard the file in between and both callers were told they had done it. The row also crossed a commit boundary, which only worked because the session factory sets `expire_on_commit=False`. One block now covers the read and the write, and the blob removal stays after it, best effort. **A file ID that could name nothing answers 404.** `could_name_a_file` guarded the page token and the `ids[]` filter but not the path parameter or the OpenAI cursor, so `GET /api/v1/files/file-%00x` reached a bind parameter and PostgreSQL refused it with a driver error. The guard moves to the repository, where an ID enters a query, so every query goes through it and the token codec borrows the one answer. **`created_at` is published as required.** The column is NOT NULL and every write sets it, but the new shapes typed it optional so the two conversions could be reused for `expires_at`. This is the commit that first publishes a shape into the spec, the collection, the dashboard client and the SDKs, and narrowing an optional afterwards breaks all of them. **A listing serves a file up to the same instant a read does.** `live` treated it as expired below `expires_at`, the listing above it, so for one instant a file was readable and unlisted. **Smaller.** `FileStorageError` declares the 500 its behavior rests on rather than inheriting it. `FileRepository.add` no longer refreshes: the database generates nothing on this table, so it read back what it had just written, once per upload. The listing builds one scope instead of two. The service's docstring says where the store and the row can come apart, rather than claiming an invariant that a cancellation breaks. Tests: the generic 5xx body and the 404 for an unusable ID, both stated behavior, plus `discard` when the row will not change and when the blob will not delete, and `content` for a row with no bytes and bytes that will not read back. * refactor(files): build the files service as a dependency, not in the route `build_file_service` was a plain function the three completion routes called in their bodies. It copied the shape of `build_sandbox_file_bridge` beside it without the reason: that one takes the billed user, the workspace and the staged inputs, none of which exist until the request is part-way through, so it cannot be a dependency. The files service takes only the config, the session, the Unit of Work and the blob store, and all four are resolved before the handler runs. It becomes `get_file_service_if_needed`, the counterpart of `get_file_service` in the same pairing the session and the Unit of Work already use, with `get_file_store_if_needed` completing it so the store is a dependency rather than an attribute read inside a builder. The routes declare `OptionalFileServiceDep` and lose the call. `run_chat_completion` receives the service rather than building one, which is what makes the Playground's own files support explicit: it runs completions through that same function and had been getting a service by accident. It is standalone only, so it takes the non-optional dependency. One shape for one service, and the hybrid variant says so in its name. * fix(files): refuse a disabled deployment before reading the request `FilesDisabledError` says the paths behave as if they were never mounted, and they did not. The check had moved into the service, which runs only once a route awaits it, so everything the route does first answered ahead of it: a master-key upload carrying no `user` got 400 for the missing field, an `ids[]` listing combined with `limit` got its own 400, and an unauthenticated read got 401. Each of those tells a caller the paths are there. The switch guards one surface, so it moves onto that surface as a router dependency, beside the one that refuses the files beta. It now runs before the request is parsed and before the caller is authenticated, so every verb answers 404 to everyone, which is what an unmounted path does. That also leaves the rule in one place rather than two that must stay in step. `staged_upload` keeps answering while the API is off, because it resolves a reference a request already holds rather than serving the API, and its caller has a switch of its own. Two comments named code this branch moved: the pipeline's workspace note pointed at `fetch_file`, and the storage adapter's cleanup attributed the size cap to the route. * fix(files): resolve an upload's workspace inside its transaction A master-key upload resolved its default workspace before the bytes were stored. That read leaves the session's transaction open, so the pooled connection was held for as long as the upload took, and it ran outside any Unit of Work block, which this layer's rules do not allow. It now happens in the same block that records the row, so it holds nothing across the upload and the workspace it may create is stored with the file rather than separately. The resolver flushes and never commits, so it cannot end the block early. That moves a failure to after the bytes exist, so the handler had to cover it. It caught only `DATABASE_ERRORS`, and anything else the resolver raised would have left bytes no row points at and no sweep can reach, because the sweep walks rows. It now catches `Exception` as well, drops the blob and re-raises unchanged. A cancellation is still not caught. It derives from `BaseException`, and one arriving while the block commits leaves the outcome unknown, so removing the bytes could strand a row that did land. * refactor(files): reuse the keyset helpers in the sweep query `reclaimable` wrote out the `(created_at, id)` comparison and the ordering that `_past` and `_ordering` already express, so one module held the same keyset rule twice and the copies could drift apart. * fix(files): keep a cancelled output file's bytes rather than stranding its row The sandbox output path caught `BaseException` around its Unit of Work block and removed the blob, so a cancellation arriving while the block committed could delete the bytes of a row that had landed. The caller then sees a file in its listing whose content cannot be read. It now catches `Exception`, which leaves cancellation alone, and matches the policy the upload path states: an orphan a reclaim pass can find is a smaller failure than a live row pointing at nothing. The broad catch in the provider copy stays, because no row has been attempted there yet. * fix(files): keep one unusable ID from failing a whole batch lookup `existing_ids` was the one query in the repository that did not check whether its input could name a file, so a provider announcing an ID PostgreSQL will not accept made the lookup fail. The sandbox bridge treats that failure as a reason to copy nothing, so a single bad ID cost the batch every valid file in it. The unusable IDs are left out of the query instead. They have no row by definition, so the answer is unchanged, and the file that carried one fails on its own when its row is written rather than taking the others with it. * fix(files): drop the bytes when an upload is cancelled before its commit Both paths that write bytes then record a row cleaned up for a database failure and left a cancellation alone, on the grounds that its commit outcome is unknown. That is right for a cancellation arriving while the commit runs, and wrong for one arriving before it: the Unit of Work never reaches its commit when the block body raises, so no row can have landed and the bytes are certainly unreferenced. The two cases are told apart by where they are caught. A handler inside the block sees only what was raised before the commit, so it drops the blob for anything including cancellation. The handler outside sees the commit's own failure and still leaves cancellation alone. The reclaim pass in #1597 is still needed for what no caller can clean, such as the process dying between the write and the commit. This narrows what it has to find. Tests pin both sides: cancelled inside the block, the blob goes; cancelled as the block commits, the blob stays. * docs(files): state what an adapter owes a cleanup that runs under cancellation An adapter's delete is called while a cancellation unwinds, and AnyIO raises at any of its checkpoints inside a cancelled scope. Every shipped adapter reaches its backend through `asyncio.to_thread`, which AnyIO does not interrupt, so the cleanup completes; nothing said that it had to. The port now does, so an adapter that reached for an AnyIO primitive would be breaking a stated contract rather than a silent assumption. The sandbox bridge's comment claimed its `asyncio.shield` kept a cancellation from cutting the cleanup short. The shield detaches the delete rather than holding the caller, so it says that instead.
This branch was successfully deployed
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.
Description
Nothing changes for someone using Otari. This is a typing change that the budgets domain work in #1202 needs.
The gateway's tables come in two styles: SQLModel tables and plain SQLAlchemy declarative tables. The generic repository base accepted only SQLModel tables, so a repository over a declarative table, such as the budget tables or the API key table, failed
make typecheck. The base now accepts any mapped table, and any Pydantic model as its create and update schema. SQLModel tables and schemas still fit both, so every existing repository type-checks unchanged. Runtime behavior does not change, because Python does not enforce these type bounds.How to test it locally
All of these pass on this branch. The one exception is
tests/integration/test_mcp_dependency_ceiling.py, which failed here on creating its throwaway venv;AGENTS.mdlists it as environment noise.The new test,
tests/integration/test_base_repository_declarative.py, builds a repository over a declarative table on a Unit of Work and counts its rows. Its real check ismake typecheck: at the first commit, which adds the test alone, mypy rejects the repository's type arguments, and at the second it accepts them.PR Type
Relevant issues
Part of #1202: Task 5.1 of its plan. Task 5.2, a lookup service for API keys, is stacked on this PR.
Checklist
tests/unit,tests/integration).make lint,make typecheck,make test).uv run python scripts/generate_openapi.py).ARCHITECTURE.mdorscripts/check_architecture.py, the description names the rule and says why.AI Usage
AI Model/Tool used:
Claude, via Claude Code.
Any additional AI details you'd like to share:
This was shared work. The change comes from the plan for #1202, and I set the scope and the rules it ran under: what it must not touch, a failing test as its own first commit, one small commit per step, and the project's comment standard for every text it adds or touches. Claude wrote the code and the test under that direction, ran the checks, and drafted this description. I reviewed both commits, and every added or touched docstring, before the push.
Summary
BaseRepositoryto support mapped SQLAlchemy classes and Pydantic create/update schemas.BudgetResetLogtable through aUnitOfWork.Benefits
This enables repositories for plain SQLAlchemy tables while preserving existing CRUD behavior and SQLModel compatibility.