diff --git a/CLAUDE.md b/CLAUDE.md index 40cd024a..3a6ae483 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -66,6 +66,12 @@ how it gets built. Concretely: `downloads/`, `reader/`, `discover/`, `catalog/`, `youtube/`, `issues/` (shared by routes + scheduler + worker) → flat core (`auth`, `apitokens`, `quota`, `jobruns`, `sysconfig`, `state`, `audit`, `notifications`, `userscope`, `ratelimit`, `bodylimit`, `progress`, `collation`, `security`, `logs`, `logsink`, `diagnostics`, `debugsession`) → `models/db/config`. **Dependency direction is one-way: core never imports routes.** +The domain is four **units**, each owning its tables: Capture (`downloads/`), Reader (`reader/`, `discover/`, the playlist +sync), Catalog (`catalog/`, the `sync/` ingests) and Plex (`plex/`). A unit's route does no database work: it reads through +the unit's `queries`, writes through its `service` (which flushes) and commits. A unit's code reads another unit's tables +only along a declared edge and writes them only through that unit's service; core reaches a unit's tables through its +`queries`. **Fence-held, with no exemptions** (`backend/tests/test_unit_fences.py`); conventions in +[`backend/app/README.md`](backend/app/README.md). Frontend: `main.tsx` provider tree → `App.tsx` shell (a shared `/watch` link boots from its own entry, `watch.html` → `watchMain.tsx`, held app-free by a contract test) → `moduleRegistry` route-chunks → module components → the **platform layer `components/ui/`** (domain-free primitives: the popover engine and @@ -87,12 +93,12 @@ CQRS, a repository/DI framework, a router library, a generic CRUD-router factory | ORM | SQLAlchemy 2.0 `select()` | `db.query(...)` | | Request body | a Pydantic model | `payload: dict` | | Request SIZE | nothing — `bodylimit` caps every route (SEC-20); need more? add a ceiling to `ROUTE_MAX_BODY_BYTES` | `capped_body` on a route that does not read raw bytes itself | -| Ownership check | `owned_or_404` / `get_or_404` | ad-hoc `_own_playlist`/`_owned`/`_own_job` | +| Ownership check | `userscope.owned_by` (a filter) / `owned_row` (a row by id): a unit's `queries.own_x` returns None and its route answers `not_found`; a core route `owned_or_404` | an inline `Model.user_id == …` (fence-held, in every module) or a `db.get` and a compare | | Admin gate import | `app.auth` | the `app.routes.admin` re-export | | Upstream failure status | `422` with JSON detail | `502` (reserve 502 for raw byte-proxies) | | User-facing error | structured `detail` (`{code, reason}`) | a bare English string under a bilingual UI | | Mutation response | `{"ok": true, …}` (`{"deleted": id}`), test-held | a bare `{"unsubscribed"}`/`{"cleared": n}`, no `ok` | -| Transaction commit | the handler commits; helpers `flush` (test-held list) | helpers that commit (only `quota` and `jobruns` own their session) | +| Transaction commit | the handler commits; a unit's `service` and every helper `flush` (fence-held) | a helper that commits (only `quota`, `jobruns` and the jobs the fence's `UNIT_COMMITS` names own their session) | | Admin knob storage | `sysconfig` (values) | env / `app_state` / a table of its own (the job intervals had one) | | Job status word | the `downloads/status.py` StrEnum | loose `"canceled"`/`"cancelled"` literals | | Current time | inline `datetime.now(timezone.utc)` | new `now_utc`/`_now` helpers | diff --git a/backend/app/README.md b/backend/app/README.md index 7a9ac54f..0afc71ad 100644 --- a/backend/app/README.md +++ b/backend/app/README.md @@ -1,9 +1,8 @@ # Backend conventions The rules below are already followed across `backend/app` — they lived only in a few module -docstrings (`userscope.py`, `routes/_common.py`) until this file collected them (E3 S3, -B-core-20). When you add code, match these so the next reader doesn't have to guess "which pattern do -I copy". +docstrings (`userscope.py`, `routes/_common.py`) until this file collected them. When you add code, +match these so the next reader doesn't have to guess "which pattern do I copy". ## Module layering @@ -13,12 +12,13 @@ Three layers, by *who* runs the code — not by feature: response models, status-code mapping, serialization. It owns logic that runs *only* on a request. Business logic that a request happens to trigger does **not** live here if anything else runs it too. -2. **`plex/`, `downloads/`, `sync/`, `issues/` — domain logic shared by more than one caller.** Anything the - **scheduler or the worker** runs (not just a request) lives in one of these domain packages, so the - route, the worker, and the scheduler share one definition. Examples from this codebase: - `downloads/service.py` owns enqueue + the job state transitions (routes call it); `plex/sync.py` is - run by both the admin route and the scheduler; `downloads/pipeline.py` is the worker's child pipeline; - `issues/bundle.py` is the one definition of a bug report's files, for the zip export and the tracker. +2. **The units and the other domain packages — logic shared by more than one caller.** A unit's + database work (below), and anything the **scheduler or the worker** runs (not just a request), lives + in a domain package, so the route, the worker, and the scheduler share one definition. Examples from + this codebase: `downloads/service.py` owns enqueue + the job state transitions (routes call it); + `plex/sync.py` is run by both the admin route and the scheduler; `downloads/pipeline.py` is the + worker's child pipeline; `issues/bundle.py` is the one definition of a bug report's files, for the + zip export and the tracker. 3. **Flat `app/*.py` core modules — cross-cutting primitives.** `auth` (with `apitokens`, a machine's folder-scoped way into the Files library), `sysconfig`, `quota`, `audit`, `jobruns` (every @@ -34,19 +34,49 @@ Three layers, by *who* runs the code — not by feature: a layer — into the relevant domain package, or a flat core module if it's cross-cutting. Don't pre-abstract for a single caller; don't leave shared logic duplicated once there are two. +## Units + +The domain is four units, each owning its tables (`models.py` in its package): + +| Unit | Package | Its routes | +| --- | --- | --- | +| Capture | `downloads/`, the worker | `downloads.py`, `files.py`, `public.py` | +| Reader | `reader/`, `discover/`, `feedquery.py`, the playlist sync and tag suggestions in `sync/` | `feed.py`, `playlists.py`, `saved_views.py`, `discover.py`, `tags.py` | +| Catalog | `catalog/`, the ingests in `sync/` | `channels.py`, `search.py`, `sync.py` | +| Plex | `plex/` | `plex/` | + +- **A unit's route does no database work.** It validates, reads through the unit's `queries.py`, + writes through its `service.py`, serializes, and commits. It may name a unit's model as a type + (under `TYPE_CHECKING`), never use one as a value, and it passes a kind (`"movie"`), not a model, to + a query. +- **A unit's code keeps to its edges.** It reads another unit's tables only along a declared edge + (Capture and Reader read Catalog's and Plex's: the feed joins `Video` with `VideoState` in one + query) and writes them only through the owner's service. Core code reaches a unit's tables through + that unit's `queries`. +- **Whose a row is, `userscope` says** — in every module, core included: `owned_by(Model.user_id, + user)` for a filter, `owned_row(db, Model, id, user)` for a row by id (someone else's row is the + same answer as a missing one). A unit's `queries.own_x` returns None and its route answers + `not_found`; a core route calls `owned_or_404`. + +All of it is held by `tests/test_unit_fences.py`, with no exemptions: the unit map, each unit's +packages and edges (`UNIT_PACKAGES`, `READS`) and the jobs that commit (`UNIT_COMMITS`) are listed +there. + ## Transaction ownership -This follows CLAUDE.md's canonical rule verbatim — **"the handler/job-wrapper commits; helpers flush -(only `quota` and `jobruns` own their session)"** — so read it that way, not as an exception to it. The commit belongs -to the **operation owner**; anything called *inside* another transaction only stages its change +This follows CLAUDE.md's canonical rule — **the handler commits; a unit's `service` and every helper +flush** — so read it that way, not as an exception to it. The commit belongs to the **operation +owner**; anything called *inside* another transaction only stages its change (`db.add` / `flush`) and lets the owner commit, so the whole operation is one transaction. `audit.record()` is the reference: it adds the audit row and the caller commits it together with the mutation it records, so an audit entry can never exist without its change (or vice-versa). -- The **operation owner** is usually the route handler. A `service.*` operation the handler delegates a - whole transition to (e.g. `service.cancel`) *is* that owner and commits its one transition — it isn't - a mid-operation helper. A helper that owns its OWN short-lived session (the worker's `jobdb` writes, - `jobruns`' run rows — the same carve-out CLAUDE.md grants `quota`) commits that session. Everything else flushes. +- The **operation owner** is the route handler, or a job. A unit's `service` flushes even when it + makes a whole transition (`service.cancel`): the handler commits it, together with whatever else the + request changed. What owns its OWN session commits it: `quota` and `jobruns`' run rows, and a unit's + jobs (the worker's `jobdb` writes, the syncs and ingests that commit per page or batch so a later + failure loses nothing already stored) — the fence's `UNIT_COMMITS` names each. Everything else + flushes. - `SessionLocal` is `expire_on_commit=False`: after a raw `UPDATE` + commit, an ORM object you already loaded is **stale**. Read the fresh value from the row (RETURNING, or a re-`SELECT`), not the ORM attribute — see `service.decr_ref_count`. diff --git a/backend/app/userscope.py b/backend/app/userscope.py index 78dd5312..a66e0226 100644 --- a/backend/app/userscope.py +++ b/backend/app/userscope.py @@ -1,10 +1,10 @@ """Whose a row is, and who an account is to the others — one place for both. **Ownership.** A user-owned table says whose a row is in one column (`user_id`, or `to_user_id` for -something handed to them). A unit's reads and writes name that column through `owned_by` and fetch a +something handed to them). Every read and write names that column through `owned_by` and fetches a row by id through `owned_row`, never with an inline `Model.user_id == …`: the rule "a row owned by someone else is answered like a missing one" then lives here, and a contract test -(`test_unit_fences.py`) holds a switched-on unit to it. +(`test_unit_fences.py`) holds every module to it, core included. **Recipients.** "An active, non-suspended account" — the download share-by-email and the recipient picker filter on it. Kept here so the two forms (SQL clauses for a query, a Python check for a loaded diff --git a/backend/tests/test_unit_fences.py b/backend/tests/test_unit_fences.py index e51eb292..28efe86c 100644 --- a/backend/tests/test_unit_fences.py +++ b/backend/tests/test_unit_fences.py @@ -5,17 +5,14 @@ What it may not do is build SQL, call the session's data methods, change a model import a unit's tables: when a route did those, ownership, the commit and the conditional `WHERE` lived in a different place in each handler, and a fix in one never reached its siblings. -The rule is switched on unit by unit. `ALLOWED` pins what each HTTP file still does today, and the -pin is exact: a count that rises fails (new database work in a route), and one that falls fails -until its entry is lowered (the fence only tightens, as `baseline.json` does for basedpyright). An -empty `ALLOWED` is the end state — a file listed there is a debt, not an exemption. +The rule has no exemptions: a single `select()` in a unit's route file fails it. A core route (identity, admin, ops) is not a unit's HTTP layer and may use the session; it may still not reach into a unit's tables. A route may NAME a unit's model as a type (under `TYPE_CHECKING`), never use one as a value. -Below the HTTP layer, a fenced unit's own code keeps to its edges (`FENCED`, `READS`): see the -second half of this file. +Below the HTTP layer, a unit's own code keeps to its edges (`READS`): see the second half of this +file. Whose a row is, every module says through `userscope` — a core one too. """ import ast @@ -107,10 +104,6 @@ UNIT_ROUTES = { "plex/": "plex", } -# What each HTTP file still does, by file: the debt a unit's sprint pays off. Exact counts. A core -# route's count is only the unit tables it imports; it is paid by the sprint of the unit it reaches. -ALLOWED: dict[str, int] = {} - # The session methods that read or write data. `commit` and `rollback` stay with the handler. SESSION_DATA = { "add", @@ -257,19 +250,13 @@ def test_every_listed_route_exists(): rels = [rel for rel, _ in _http_files()] for key in UNIT_ROUTES: assert any(rel == key or (key.endswith("/") and rel.startswith(key)) for rel in rels), key - assert set(ALLOWED) <= set(rels), set(ALLOWED) - set(rels) -def test_no_http_file_does_database_work_beyond_its_pin(): - drift = {} - for rel, src in _http_files(): - found = violations(src, _unit_of(rel)) - pinned = ALLOWED.get(rel, 0) - if len(found) != pinned: - drift[rel] = (pinned, len(found), found if len(found) > pinned else "lower its pin") - assert drift == {}, ( - "an HTTP file's database work moved off its pin (pinned, now, what): move new work into" - " the unit's queries/service; when a count fell, lower ALLOWED to it" +def test_no_http_file_does_database_work(): + hits = {rel: found for rel, src in _http_files() if (found := violations(src, _unit_of(rel)))} + assert hits == {}, ( + "a route does database work: reads go through the unit's queries, writes through its" + " service; a core route reaches a unit's tables through that unit's queries too" ) @@ -307,11 +294,11 @@ def test_the_detector_sees_each_kind_of_database_work(): # --- inside a unit: its package -------------------------------------------------------------- -# A unit whose sprint has landed is fenced beyond its HTTP layer too. Its own code reads another -# unit's tables only along a declared edge (the feed joins Video with VideoState in one query; a -# boundary that split it would cost the speed the app is for), and writes another unit's tables -# never — that goes through the owner's service. Whose a row is, it says through `userscope`, and -# it commits only where a job owns its session. +# Every unit is fenced beyond its HTTP layer too. Its own code reads another unit's tables only +# along a declared edge (the feed joins Video with VideoState in one query; a boundary that split +# it would cost the speed the app is for), and writes another unit's tables never — that goes +# through the owner's service. It commits only where a job owns its session. Whose a row is, it +# says through `userscope`, and so does core code: the owner filter is checked in every module. # Each unit's code outside the HTTP layer: its package, and the entry point that runs its jobs. UNIT_PACKAGES = { @@ -341,9 +328,7 @@ UNIT_PACKAGES = { # pages' reads and writes. "plex": ("plex/",), } -# The units fenced so far; the rest join with their sprint. -FENCED = {"capture", "reader", "catalog", "plex"} -# Whose tables a unit's own code may READ. Core code reads a fenced unit's tables through its queries. +# Whose tables a unit's own code may READ. Core code reads a unit's tables through its queries. READS = { "capture": {"catalog", "plex"}, "reader": {"catalog", "plex"}, @@ -352,7 +337,7 @@ READS = { } # The columns that say whose a row is. OWNER_COLUMNS = {"user_id", "to_user_id", "from_user_id", "created_by"} -# Where a fenced unit's code may commit: the jobs that own their session (the worker and what it +# Where a unit's code may commit: the jobs that own their session (the worker and what it # runs, the GC, the backfill — none of them inside a request), and the one write that must commit # where it stands: an upload's rows and the file it placed on disk land together or not at all, so # a failed commit can remove the file (`upload.ingest_upload`). Everything else flushes; the handler @@ -465,14 +450,14 @@ def _enclosing_functions(tree: ast.AST) -> dict[ast.AST, str]: def package_violations(rel: str, source: str) -> list[str]: - """What a module outside the HTTP layer does against a fenced unit's boundary, as `line: what`.""" + """What a module outside the HTTP layer does against a unit's boundary, as `line: what`.""" tree = ast.parse(source) mine = _package_unit(rel) found = [] models = _imported_models(tree) for name in models.values(): theirs = UNIT_MODEL_NAMES[name] - if theirs == mine or (mine not in FENCED and theirs not in FENCED): + if theirs == mine: continue if theirs not in READS.get(mine or "", set()): found.append(f"0: {mine or 'core'} imports {theirs}'s {name}") @@ -487,10 +472,10 @@ def package_violations(rel: str, source: str) -> list[str]: if written is None: continue theirs = UNIT_MODEL_NAMES[written] - if theirs != mine and (mine in FENCED or theirs in FENCED): + if theirs != mine: found.append(f"{node.lineno}: {mine or 'core'} writes {theirs}'s {written}") - if mine in FENCED: - found += _owner_filters(tree) + found += _owner_filters(tree) + if mine is not None: functions = _enclosing_functions(tree) allowed = UNIT_COMMITS.get(mine, set()) for node in ast.walk(tree): @@ -522,7 +507,7 @@ def _owner_filters(tree: ast.AST) -> list[str]: return found -def test_a_fenced_units_code_keeps_to_its_edges(): +def test_a_units_code_keeps_to_its_edges(): hits = {rel: found for rel, src in _app_files() if (found := package_violations(rel, src))} assert hits == {}, ( "read another unit's tables only along READS, write them only through their service, say" @@ -530,12 +515,8 @@ def test_a_fenced_units_code_keeps_to_its_edges(): ) -def test_a_fenced_units_routes_filter_owners_through_userscope(): - hits = { - rel: found - for rel, src in _http_files() - if _unit_of(rel) in FENCED and (found := _owner_filters(ast.parse(src))) - } +def test_every_route_filters_owners_through_userscope(): + hits = {rel: found for rel, src in _http_files() if (found := _owner_filters(ast.parse(src)))} assert hits == {} @@ -562,6 +543,7 @@ PACKAGE_CASES = { ("sync/videos.py", "from app.models import Playlist"): 1, # no catalog→reader edge ("downloads/x.py", "q = DownloadJob.user_id == 1"): 1, ("downloads/x.py", "q = owned_by(DownloadJob.user_id, 1)"): 0, + ("quota.py", "q = QuotaEvent.user_id == 1"): 1, # core too ("downloads/x.py", "def f(db):\n db.commit()"): 1, ("downloads/gc.py", "def f(db):\n db.commit()"): 0, ("downloads/upload.py", "def ingest_upload(db):\n db.commit()"): 0, diff --git a/docs/gates.md b/docs/gates.md index 2810eaf2..75537e98 100644 --- a/docs/gates.md +++ b/docs/gates.md @@ -113,7 +113,6 @@ project checks needs a row. | CVE gate | HIGH/CRITICAL with a fix available only | `--ignore-unfixed` + `--severity HIGH,CRITICAL`: a finding with no fix cannot be acted on by an upgrade; it shows in the scan output. A fixable one fails the publish, and the only way past it is `.trivyignore.yaml`. | 0 open (E32 S1) | | npm audit | `--omit=dev --audit-level=high` | What ships to the browser, at high or above. The dev toolchain's advisories were burned down by hand in E32 S1 (18 → 0). | 0 | | knip | `entry` — one file added | Not a narrowing: knip's Vite plugin reads entries from index.html only, so the shared link's page (`watch.html` → `src/watchMain.tsx`) is named here, or the lane reports that whole entry unused. It adds files to the lane, never takes any out. | 1 entry | -| unit fence | the database work the HTTP layer still does, pinned | `backend/tests/test_unit_fences.py`: a unit's route file may not build SQL, call the session's data methods, set a model's attributes or import a unit's tables; a core route may not import a unit's tables. Switched on unit by unit: `ALLOWED` pins each file's count exactly, so a rise fails and a fall fails until the pin is lowered — the list may only shrink. End state: empty, and the pin mechanism goes. A unit in `FENCED` is held below its HTTP layer too: its code reads another unit's tables only along `READS`, writes them never, filters owners through `userscope`, and commits only where `UNIT_COMMITS` names a job's own session. | 826 in 23 files (E42 S0) → 667 in 19 (S1: Capture 0, fenced) → 525 in 14 (S2: Reader 0, fenced) → 354 in 9 (S3: Catalog 0, fenced; core routes 0) → 178 in 4 (S4a: Plex browse, detail, people, collections 0) → 0 (S4b: Plex 0, fenced — every unit fenced) | | React Compiler | the functions it skips, pinned | `components/compilerCoverage.contract.test.ts` — the list may only shrink. End state: only VirtualGrid (TanStack Virtual, an IncompatibleLibrary) remains. | 93 (E32 S1) → 29 (E32 S3) → 1, VirtualGrid (E32 S4) | ## Accepted findings