Skip to content

[ENG-373] Persisted review flags on oc_recordings, with retroactive backfill - #134

Open
henokteixeira wants to merge 6 commits into
mainfrom
henok/eng-373-review-flags
Open

[ENG-373] Persisted review flags on oc_recordings, with retroactive backfill#134
henokteixeira wants to merge 6 commits into
mainfrom
henok/eng-373-review-flags

Conversation

@henokteixeira

Copy link
Copy Markdown
Contributor

Summary

Closes ENG-373. tripod-api had no notion of "this recording needs review", so recordings with no classification, a throwaway description or no storyteller sat in the API and nothing could list or count them. ENG-354 (#129) made a sufficient description mandatory but deliberately only on write, grandfathering every existing row — this marks what is already on the floor. The Flutter half is ENG-374, which will render these flags and delete its own local derivation.

  • One sa.JSON column on oc_recordings holding {code, origin} objects. Objects rather than bare strings so a user-raised flag can join the same list later without a schema change; only system exists today. JSON rather than ARRAY because the suite runs on SQLite and production on Postgres. none_as_null=True turns a stray None into a loud NOT NULL violation instead of the JSON string "null", which would insert cleanly into a NOT NULL column and then fail response validation for an entire project listing.
  • recompute_review_flags is called from every write path. The issue named two; a search found four. persist_split_segments builds child recordings outside create_recording. And delete_storyteller is the one no Python hook could have caught: storyteller_id is an FK with ON DELETE SET NULL, so deleting a storyteller nulled the column inside Postgres with no ORM object loaded, leaving orphaned recordings reporting a storyteller they no longer had. The NULL is now applied in Python first. Its documented contract is unchanged.
  • missing_classification deliberately ignores subcategory, matching the exact Dart predicate ENG-374 deletes. A server that disagreed would fight the app over the same recording. The description condition imports ENG-354's rule rather than reimplementing it.
  • Schema change and backfill are one revision, not two. Split, the CI gate's downgrade -1 would only ever reach the backfill — whose downgrade() is necessarily a no-op — and the column drop would stop being exercised. env.py wraps the run in a single transaction, so there was never an observable window between them.
  • The migration does not import the service. Reaching into app.services pulls in ~291 modules and constructs a database engine at import time, which would let an unrelated service's import error block alembic upgrade head against the database six production apps share. Only app/utils/description_rule.py is imported (5 app.* modules, no engine); the three conditions are inlined, which is the correct semantics for a frozen historical artifact.

Not in this PR, and why

No flag filter on list_recordings. sa.JSON renders as Postgres json, not jsonb, so @> and jsonb_path_exists are unavailable, and SQLite's json_each has no portable equivalent — a filter would need dialect branching, reintroducing exactly the divergence that made JSON the right choice over ARRAY. Filtering in Python after the query breaks pagination, since offset/limit are already applied. And it would not deliver what ENG-374 §4 actually needs: list_recordings returns an untotalled list capped at 50, so the client would still be counting a truncated page, which that issue explicitly forbids. This needs a decision (normalised child table, aggregate endpoint, or defer) and is flagged for @henokteixeira rather than guessed at.

update_recording_fields (app/inngest/helpers.py) has no recompute. It is a generic setattr helper shared by three Inngest jobs; no current caller passes a flag-bearing field, and it opens its own session so nothing could test a change there. Deliberate gap, not an oversight.

Test plan

  • uv run pytest tests1022 passed, 2 skipped (baseline on main is 992 passed, 2 skipped).
  • uv run ruff check ., uv run ruff format --check ., uv run mypy app — all clean.
  • Migrations CI gate: single head 20260731_0001, and upgrade headdowngrade -1upgrade head on Postgres 16.
  • Verified locally against a real postgres:16 container with seeded legacy rows, which the CI gate cannot cover because its database is empty: the backfill logged scanned 2 recording(s), updated 2, a second run reported updated 0, and information_schema confirmed the column is genuinely dropped by downgrade -1 and the rows survive the round trip.
  • Tests were mutation-checked, not assumed. Counting description length in code points instead of grapheme clusters fails the just-under-threshold cases while the shared vector alone passes it happily — every row of that vector sits at exactly twenty clusters and more than twenty code points, so a naive count only over-counts and never flags. Computing flags without persisting them fails 19 tests.

Reviewer's attention

  • UNCLASSIFIED_GENRE_ID must stay in step with alembic/versions/20260415_0001_seed_unclassified_oc_taxonomy.py. If the sentinel id ever changes, missing_classification stops firing and the failure looks like "everything is fine" — the worst shape of failure for a review-flag feature. There is no automated tie, because the test database is built from Base.metadata and never runs migrations.
  • The backfill does not bump updated_at (it writes through a lightweight sa.table() with no onupdate), while delete_storyteller now does. If the Flutter client ever delta-syncs on updated_at, backfilled flags would not reach it. No updated_since filter exists today, so this is not live.

A recording with no classification, a throwaway description or no storyteller was
invisible: nothing could list or count it. ENG-354 stopped the bleeding on write and
deliberately grandfathered every existing row, so the rule that marks them has to live
server-side and be readable.

One JSON column holding {code, origin} objects rather than bare strings, so a
user-raised flag can join the same list later without a schema change. JSON rather than
ARRAY because the suite runs on SQLite and production on Postgres. none_as_null makes a
stray None a loud NOT NULL violation instead of the JSON string "null", which would
insert cleanly and then break response validation for the whole listing.

The rule ignores subcategory on purpose: the Dart predicate it replaces does too, and a
server that disagreed would fight the app over the same recording.
A persisted flag that nothing invalidates is worse than no flag. The issue named two
write paths; a search found four.

persist_split_segments builds child recordings directly, outside create_recording, so
segments would otherwise be born with an empty flag list that is simply wrong.

delete_storyteller is the one no hook could have caught: the FK is ON DELETE SET NULL,
so the column changed inside Postgres with no ORM object loaded, leaving orphaned
recordings reporting a storyteller they no longer have. The NULL is now applied in
Python first so the loss is observable. The documented contract is unchanged.

update_recording recomputes after the generic setattr loop, so it sees final state and
covers any field later added to RecordingUpdate.
Both GET endpoints already validate from ORM attributes, so the router needs no change.
The server default is what lets existing rows read back as an empty list rather than
NULL without a table rewrite, but an empty list reads as "reviewed and clean" when in
fact nothing has ever looked at the row. Hence the backfill.

Schema change and backfill are one revision on purpose. Split in two, the CI gate's
downgrade -1 would only reach the backfill, whose downgrade is necessarily a no-op, and
the column drop would stop being exercised at all. env.py wraps the run in a single
transaction, so there was never an observable window between them.

Written against the lesson of 20260518_0002: preconditions return an empty report rather
than raising, and the row counts are logged, so an operator can tell a table with nothing
to do apart from a backfill that never ran.

The rule is inlined rather than imported from the service. Reaching into app.services
pulls in nearly three hundred modules and builds a database engine at import time, which
would let an unrelated service's import error block alembic upgrade head against the
database six production apps share. Only the description rule is imported, as ENG-354
requires — a second copy of that would drift from the Dart client.
Mutation-tested rather than assumed. The shared vector alone cannot detect a count over
code points instead of grapheme clusters, because every row sits at exactly twenty
clusters and more than twenty code points, so a naive count only over-counts and never
flags; the just-under-threshold cases are what make that requirement real. Membership
negation is likewise satisfied by losing the flag list entirely, so the clearing tests
assert exact sets.

The backfill's callable is imported from the revision by path: the suite builds its
schema from Base.metadata and never runs Alembic.
It named eight service packages of twenty-three, and had already drifted long before
this change. Naming them all would rot again within weeks.
@linear-code

linear-code Bot commented Jul 31, 2026

Copy link
Copy Markdown

ENG-373

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant