feat: thingiverse sync-state tables (schema v17)

[devonjones]

Summary

Ticket openforge_catalog-750 — the Postgres side of the Thingiverse publish/sync epic (4kx). Unblocks the sync engine (ad5) and the prod-propagation fixture path (8yb).

openforge/db/schema/version_17.py:

• thingiverse_things — one row per managed thing: nullable-unique thing_id (drafts exist locally before Thingiverse assigns an id), status enum (draft/published/deleted), manifest_path, publish/sync timestamps
• thingiverse_files — the per-file diff ledger. Each row ties a remote file to its local source: blueprint_id (catalog models), image_id (gallery photos), or local_path (ad-hoc zips/artifacts), with a file_type enum discriminator. Carries local_md5 + local_sha256 (defensive, pending hash-algo confirmation from the HAR ticket hnr) and remote_file_id/remote_file_name/remote_hash
• Constraint design: CHECK num_nonnulls(blueprint_id, image_id, local_path) >= 1; blueprint/image FKs are RESTRICT (SET NULL would trip the CHECK and make parent deletes fail confusingly — RESTRICT makes unlinking deliberate, so the sync engine can delete the remote file first); thing deletion CASCADEs its ledger rows
• FK indexes for the sync engine's join patterns

These tables are local-only operational state — prod gets just the derived (blueprint_id, thing_id, public_url) mapping via the fixture path (see 8yb), per the two-DB split.

Test plan

• bin/db_update up on local DB — applies cleanly
• bin/db_update down 16 → up — full down/up round-trip verified
• tests/test_thingiverse_schema.py — 8 tests covering round-trip, nullable-unique thing_id, the CHECK, each local-source kind, CASCADE, RESTRICT, and both enums
• Full suite: 381 passed (4 pre-existing test_incremental failures reproduce on clean test; CI green there)

Beads: openforge_catalog-750 (closed via bd close on merge).

🤖 Generated with Claude Code

[devonjones]

Review scope

This is a domain/schema-soundness review of PR #225 (feat: thingiverse sync-state tables (schema v17), openforge_catalog-750). Migration-discipline, serverless-architecture, and test-coverage concerns are being handled by separate reviews and are intentionally out of scope here; this focuses on whether the data model actually serves the sync engine (ad5) that depends on it, constraint semantics, and house-style consistency with version_02/12/16.

---

Critical

None. The schema is sound enough to build on; nothing here corrupts data or blocks bin/db_update up/down.

---

Medium

1. No uniqueness on (thingiverse_thing_id, blueprint_id) / (..., image_id) / (..., local_path). Nothing stops two thingiverse_files rows in the same thing from pointing at the same blueprint (or image, or local path). Given the ledger model appears to be "one row per remote file, updated in place as remote_file_id/remote_hash change," a duplicate row for the same logical source is an ambiguous state the sync engine (ad5) will have to defend against at query time instead of the database preventing it outright. Partial unique indexes would close this without conflicting with the tri-source CHECK, e.g.:

   CREATE UNIQUE INDEX idx_thingiverse_files_thing_blueprint
     ON thingiverse_files(thingiverse_thing_id, blueprint_id)
     WHERE blueprint_id IS NOT NULL;
   -- same pattern for image_id, local_path

   (A blueprint appearing in two different things is fine and should stay allowed — this is only about dupes within one thing.)

2. No persisted signal for thing-level metadata drift. The CHECK/hash machinery here only covers file content drift (local_md5/local_sha256 vs remote_hash). ad5's own notes call out the "shared-boilerplate description" case from the v1 tool as a lesson learned — i.e., the sync engine needs to detect when a thing's name/description has drifted, not just its files, and needs to do so without hammering the API for every managed thing on every run (the same notes flag throttling as a requirement). There's no column here to cache a last-known description/name hash on thingiverse_things to short-circuit that check cheaply. This may be a deliberate deferral to d7a/ad5 (diff against a live fetch each time, no persisted state needed) — but if so it's worth confirming now, because it's exactly the kind of thing that forces a v18 migration next sprint if it turns out caching is wanted. Recommend at least a comment in ad5/d7a's design about which approach is intended.

3. thingiverse_files.blueprint_id/image_id model implies a blueprint can belong to multiple things, but 8yb's prod fixture is framed as a single (blueprint_id, thing_id, public_url) mapping. Nothing here forces 1:1, and moving the blueprint linkage down to the file-ledger (rather than putting blueprint_id on thingiverse_things as the original ticket description sketched) is the right domain call — a Thing legitimately bundles multiple files/blueprints, and a blueprint could legitimately be bundled into more than one Thing. But that means 8yb's "derive the mapping" step has an open question baked in: what happens when a blueprint resolves to more than one thing_id? Worth flagging now so 8yb doesn't quietly assume 1:1.

---

Minor

• file_type enum isn't tied to which source column is populated. The CHECK only enforces "at least one of blueprint_id/image_id/local_path is set" — nothing enforces that file_type = 'image' implies image_id IS NOT NULL, etc. The code comment describes the intended mapping ("catalog models point at blueprints, gallery photos at images...") but the DB doesn't enforce it. Might be intentional looseness (e.g. an ad-hoc STL not yet cataloged could still be file_type = 'model' with only local_path set), but if that's not intended, a stricter CHECK would catch a sync-engine bug that mislabels a row instead of silently persisting bad state.
• No deleted_at on thingiverse_things. published_at/last_synced_at are tracked but the status = 'deleted' transition has no matching timestamp, unlike its siblings. Minor asymmetry, cheap to add now, more annoying to backfill later if audit/history ever matters.
• No size/mtime fields for the local_path (ad-hoc artifact) case. blueprints already established a size+mtime+hash pattern (file_size, file_modified_at, file_md5) for cheap dirty-checking before a full hash; thingiverse_files only carries hashes for the zip/ad-hoc case, so any staleness check there is full-hash-only. Probably fine if these artifacts are always regenerated rather than checked for staleness, but worth a conscious call rather than an oversight.
• No index on thingiverse_things(status) or (thing_id) for lookups by status. Table will be tiny (managed-things count, not blueprint count) so this is very unlikely to matter, but noting for completeness since the PR was otherwise careful about FK-join indexes.
• PR body says test_file_accepts_each_local_source_kind covers "each local-source kind" but the test only exercises blueprint_id and local_path, not image_id. Small enough that it's arguably test-coverage-agent territory, but it's also a schema-semantics gap (the third leg of the tri-source CHECK is untested), so flagging here too.

---

Positive

• UNIQUE(thing_id) correctly relies on Postgres's NULL-are-distinct semantics to let multiple local drafts coexist before Thingiverse assigns an id, and the round-trip is directly tested (test_thing_id_unique_but_nullable). This is exactly right for the domain (drafts exist locally before publish).
• RESTRICT-over-SET NULL rationale is sound and well-documented — both in the PR body and in-file comments. SET NULL would have created a confusing interaction with the tri-source CHECK (an UPDATE ... SET NULL that trips a CHECK is a much worse failure mode than a clean FK violation), and RESTRICT correctly forces the sync engine to unlink/delete-remote-first. This is a case where being more explicit than the codebase's existing implicit-NO-ACTION convention (version_02's bare REFERENCES blueprints(id)) is a genuine improvement, not just a style deviation.
• Plural table names (thingiverse_things/thingiverse_files) over the ticket's literal singular wording, singular enum type names (thingiverse_thing_status) — both match the established blueprints/blueprint_type convention exactly.
• Moving the blueprint linkage into the file ledger rather than the thing row (departing from the ticket description's thingiverse_thing (blueprint_id <-> thing_id) sketch) is the correct domain call: a Thingiverse Thing is fundamentally a bundle of files, and modeling the blueprint relationship at the file level is what actually lets the sync engine detect per-file changes.
• Migration structure, docstring format, helper-method breakdown, and down_impl ordering (indexes → files → things → types) all match version_12/version_16's conventions precisely. No sql.Identifier/sql.Literal needed here since it's static DDL, consistent with existing schema files.
• conftest.py change is correct and minimal — new tables added to the existing explicit TRUNCATE ... CASCADE list, consistent with how every other table is listed there (even though CASCADE alone would technically cover thingiverse_files via the FK from thingiverse_things).
• Test suite directly exercises the two trickiest semantics call-outs (nullable-unique thing_id, CASCADE vs RESTRICT split) rather than just round-tripping the happy path.

---

Recommendations

1. Add partial unique indexes on (thingiverse_thing_id, blueprint_id), (..., image_id), (..., local_path) (each WHERE ... IS NOT NULL) before ad5 starts writing sync logic that assumes one ledger row per (thing, source).
2. Before or during ad5, explicitly decide (and document) whether thing-level metadata drift detection (name/description) will be a live-fetch-and-diff or needs a cached hash column here — if the latter, that's a v18 migration to plan for now rather than discover mid-ad5.
3. Clarify in 8yb whether a blueprint resolving to multiple thing_ids is expected/handled, since this schema permits it.
4. Consider adding the missing image_id case to test_file_accepts_each_local_source_kind while touching this file.

---

Overall

Solid, appropriately-scoped migration that follows house style closely and gets the trickier constraint calls (nullable-unique, RESTRICT-vs-CASCADE, tri-source CHECK) right, with good rationale captured in both the PR body and inline comments. The main gap is forward-looking: the schema currently only models file-level diffing, and the notes on ad5 (shared-boilerplate description drift, throttling) suggest thing-level metadata drift detection is also in scope for the very next ticket this one unblocks — worth a deliberate decision now rather than a v18 migration next week. The lack of per-(thing, source) uniqueness is the one concrete gap I'd want closed before ad5 lands.

[devonjones]

Response to Claude Review

Medium 1 (duplicate ledger rows): Fixed — three partial unique indexes ((thing, blueprint_id), (thing, image_id), (thing, local_path), each WHERE ... IS NOT NULL). Two new tests: duplicate source in the same thing rejected; same source across different things allowed (that's a legitimate domain case — sets share pieces).

Medium 2 (no thing-level metadata drift signal): Fixed — added remote_metadata_hash TEXT to thingiverse_things: hash of the last-pushed assembled metadata payload, so the sync engine detects name/description/tag/boilerplate drift without a v18. Exactly the right catch given ad5's boilerplate-drift requirement.

Medium 3 (8yb mapping is 1:N): Fixed in process — design note added to openforge_catalog-8yb: the mapping is per-file-derived and 1:N (a blueprint may ship in several things); the fixture shape and the app's links UI must handle that. The new cross-thing test pins the schema behavior.

Minor 1 (file_type ↔ source column not DB-enforced): Won't fix — file_type describes the remote artifact kind, not which source column feeds it. An ad-hoc model arrives via local_path; a cross-CHECK would forbid legitimate combinations.

Minor 2 (no deleted_at): Won't fix — the status transition time is captured by updated_at; avoiding speculative columns.

Minor 3 (no size/mtime for ad-hoc files): Fixed — local_size BIGINT + local_mtime TIMESTAMP added; this is the fallback change signal ad5 explicitly anticipates for file types the remote API may not hash.

Minor 4 (no status index): Won't fix — the table will hold hundreds of rows; a status index is noise at that scale.

Minor 5 (PR body claimed image_id tested): Fixed — the image_id source-kind test (and image RESTRICT test) landed this round via test-coverage-reviewer's P1; the claim is now true.

[devonjones]

Verification of round-1 fixes (commit ede9be7)

M1 (partial unique indexes): Verified. All three partial unique indexes present in create_thingiverse_indexes (thing+blueprint, thing+image, thing+path, each WHERE ... IS NOT NULL). test_duplicate_source_in_same_thing_rejected and test_same_source_allowed_in_different_things both pass and correctly pin the semantics.

M2 (remote_metadata_hash): Verified. Column added to thingiverse_things, docstring updated to explain it's a hash of the last-pushed metadata payload for drift detection.

M3 (8yb 1:N design note): Verified. bd show openforge_catalog-8yb has the note recorded, and it accurately reflects the schema (partial unique index is per-thing only, so a blueprint can appear in multiple things — test_same_source_allowed_in_different_things pins it).

m3 (local_size/local_mtime): Verified. Both columns added to thingiverse_files, nullable, positioned sensibly next to the other local-file metadata.

m5 (image test): Verified. test_file_accepts_each_local_source_kind now covers the image case, plus a new test_deleting_referenced_image_is_restricted. Both pass.

Won't-fix rationales:

• m1 (file_type ↔ source cross-CHECK): reasonable. file_type='model' legitimately can arrive via local_path for an ad-hoc model not tied to a blueprint_id, so a strict cross-check would forbid a real case. Agreed.
• m2 (no deleted_at): reasonable. status already has a 'deleted' enum value and updated_at captures the transition timestamp; a dedicated column would be speculative for a low-cardinality lifecycle field.
• m4 (no status index): reasonable given stated scale (hundreds of rows) and low cardinality of the 3-value enum — a B-tree index here wouldn't earn its write-time cost.

down_impl order: Verified. Drops in exact reverse of creation order: uniq_thing_path → uniq_thing_image → uniq_thing_blueprint → idx_files_image → idx_files_blueprint → idx_files_thing.

Tests: pytest tests/test_thingiverse_schema.py -q → 13 passed.

New-issue scan of ede9be7: No new issues found. ruff check and ruff format --check both clean on the two touched files.

Overall: LGTM. All "Fixed" items confirmed in code/tests, won't-fix rationales hold up under scrutiny, and the diff introduces nothing new to flag.