feat: thing manifest schema + metadata assembler

[devonjones]

Summary

Ticket openforge_catalog-d7a — the "what goes in a thing and what it says" layer of the Thingiverse epic (4kx). Unblocks the sync engine (ad5) together with #225.

• openforge/thingiverse/manifest.py — per-thing YAML manifests, strictly validated (unknown keys/sections/select-keys all fail fast — typos die at load, not at publish). Model entries:
  • select: tag queries — the killer feature: newly scanned files matching the query flow into the thing on the next sync run, zero manual entry. Semantics match the catalog's tag engine: require = exact-match ALL, accept = hierarchical-prefix ANY, deny = exact exclude.
  • explicit md5: / full_name: refs (ambiguous full_names rejected with a pointer to use md5)
  • images/zips/others as local paths resolved relative to the manifest
• openforge/thingiverse/templates/openforge2.yaml — product-line defaults (license, category, base tags, description boilerplate) ported from the old v1 tooling's new.openforge2.json. Boilerplate lives once in the template, so changing the copy re-syncs every managed thing's description — the old tv_update_description bulk-edit workflow, automated. ⚠️ The boilerplate copy is the original ~2016 text (reddit/twitter links) — needs Devon's review before first production publish.
• openforge/thingiverse/assembler.py — assemble_thing(curs, manifest) → API-neutral payload: merged metadata (template ← manifest overrides), resolved blueprint rows (deduped by md5), verified local files, and metadata_hash (canonical sha256 that feeds thingiverse_things.remote_metadata_hash for drift detection). Fail-fast everywhere: unknown template, selector matching nothing, selector hitting the truncation ceiling, missing refs, missing files.

One subtle bug caught during dev worth flagging: tag_search_blueprints takes {"tag": ...} dicts — bare strings silently no-op the filters ("tag" in "texture|cave" is just a substring check). The assembler wraps correctly and carries a comment.

Test plan

• 42 new tests: 13 manifest validation, 29 assembler (template load, override/merge/compose semantics, DB-backed selector require/accept/deny behavior, md5/full_name resolution, dedupe, local-file resolution, metadata_hash stability)
• ruff check . clean; full suite 402 passed (same 4 pre-existing test_incremental local failures; CI green on test)

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

🤖 Generated with Claude Code

[devonjones]

Review scope

Domain correctness of the manifest → assembled thing payload for the Thingiverse epic (openforge_catalog-d7a, unblocking ad5), selector semantics vs. openforge/db/sql/tags.py, metadata_hash stability, and house style. Complexity/error-handling/test-coverage/dead-code/importlib-resources/credentials are covered by other passes and skipped here. Verified several claims experimentally against a live test DB (pytest + test_db fixture), not just by reading.

Critical

1. select.accept with multiple entries is ANDed, not ORed — contradicts the manifest.py docstring's documented "ANY" semantics.

   The docstring says: "accept: model must carry at least one tag in the listed subtree, matched by hierarchical prefix ANY." That's true for a single accept entry (verified: accept: ['shape|floor'] correctly matches a blueprint tagged shape|floor|corner via prefix match). But with multiple entries it's a different story.

   _query_tags_include (openforge/db/sql/tags.py) adds one JOIN tags AS tags_N per accept entry and just concatenates all the WHERE conditions — there's no OR grouping across entries, only within a single entry's own position checks. That means a blueprint must satisfy every listed accept subtree simultaneously (same AND behavior as require), not any one of them.

   I confirmed this by inserting two blueprints, each carrying exactly one of two sibling texture tags, and running:

   select:
     accept: ['texture|cave', 'texture|dungeon_stone']

   Expected (per docstring): both blueprints match (OR). Actual: AssemblyError: selector matched no models — because no single blueprint carries both texture subtrees at once.

   This means the most natural use of a multi-value accept list — "any of these sibling subtrees" (e.g. several textures, several sizes) — can never match anything unless a blueprint happens to be tagged under every listed subtree at once. Since the assembler fails fast on empty selector results, this surfaces as an immediate hard error rather than silent corruption, but it means an entire class of manifest expression the docstring promises doesn't work. There is no test anywhere (test_thingiverse_assembler.py, test_thingiverse_manifest.py) exercising accept with more than one entry, so this shipped unnoticed.

   This needs a decision before ad5/lpe manifests start relying on it: either _query_tags_include/_query_tags_basics need real OR-across-entries support for accept (touches shared tag-search code used elsewhere by the app, e.g. the search API), or the assembler needs to run one sub-select per accept entry and union the results itself, or (minimal fix) the docstring should be corrected to state accept is ALL-across-entries like require (in which case accept differs from require only by prefix-vs-exact matching, which is a much narrower and less useful primitive than advertised).

Medium

2. Model row shape differs depending on resolution path. _resolve_select gets rows via tag_search_blueprints's SELECT * (all blueprints columns, including search_text, added in schema v4). _resolve_md5/_resolve_full_name get rows via get_blueprints_by_md5/get_blueprints_by_full_name's explicit column list (no search_text). Both paths run rows through _convert_config, so the fields the sync engine actually needs (file_md5, file_name, full_name, storage_address) are present and consistent either way — but a manifest mixing select: entries with md5:/full_name: entries for the same thing produces a files.models list whose dicts don't all have the same keys. Low risk today since nothing downstream inspects search_text, but worth normalizing to one explicit projection so files.models is shape-stable regardless of how each entry was resolved.

Minor

3. _resolve_md5/_resolve_full_name don't filter by blueprint_type='model' the way _resolve_select explicitly does (models=True, blueprints=False, with a comment explaining why). I traced the fixtures/scanner pipeline (openforge/db/fixtures/utils.py:munge_blueprint, openforge/db/fixtures/incremental.py) and confirmed composite blueprint_type='blueprint' rows never populate file_md5/full_name today (they're config-only compositions, keyed by blueprint_name instead), so this is currently unreachable — a real md5/full_name literal can never equal a NULL column. But nothing enforces that invariant at the schema level (no CHECK constraint), and the admin blueprint API (openforge/app/routes/blueprints.py, openforge/openapi/schemas/blueprint.yaml) would accept a row with blueprint_type='blueprint' and full_name/file_md5 both set if anyone ever created one by hand. Adding the same type filter to _resolve_md5/_resolve_full_name is a one-line-each defense-in-depth fix.

4. _dedupe_models keys on row.get("file_md5"), which trusts every resolved row has a non-null, unique md5. If that were ever violated (only reachable via #3), two distinct rows with file_md5 = None would silently collapse into one instead of erroring. Moot once #3 is closed, but the function has no explicit assertion of the invariant it relies on.

5. manifest["license"] or template.get("license") (and same pattern for category) treats an explicit-but-falsy manifest override (e.g. license: "") the same as "not set," silently falling back to the template value. Edge case, probably never hit since these fields are either meaningfully set or omitted (None) in practice, but it's a subtle "" vs None trap worth a one-line comment for future maintainers.

Positive

• storage_address, file_md5, and file_name are present on every resolved model row (verified for both selector and explicit-ref paths), so ad5 has everything it needs to know where to fetch a model's bytes from (R2 via storage_address) and what to name it on upload (file_name) without extra lookups.
• The {"tag": t} wrap for tag_search_blueprints is correct and well-commented. Verified against tags.py: bare strings really would silently no-op, since _query_tags_include/_query_tags_deny gate on if "tag" in req/if "tag" in d, and a bare string like "texture|cave" makes "tag" in "texture|cave" a truthy substring check that then crashes/misbehaves rather than doing a real filter. Good catch, correctly guarded against.
• require (exact ALL across entries) and deny (exact match, excluded if any listed tag present) semantics both match the docstring exactly — verified experimentally and by reading _query_tag_require/_query_tags_deny.
• metadata_hash is properly decoupled from file selection — only the metadata sub-dict is hashed, not files, so selector/file drift isn't conflated with "should this thing's metadata re-push," matching ad5's separate file-vs-metadata diffing design. sort_keys=True handles dict-key ordering, and tag-list order is deterministic (template-then-manifest, stable case-insensitive dedupe), so a template boilerplate or tag change reliably changes the hash as intended.
• Fail-fast validation in manifest.py (unknown top-level keys, unknown file sections, unknown select keys, ambiguous full_name) is thorough and in keeping with the project's "no silent failures" philosophy.
• SELECT_LIMIT truncation-ceiling check (len(rows) >= SELECT_LIMIT) is correctly reasoned given tag_search_blueprints's LIMIT-based pagination and the catalog's known size (~1,400 designs).
• ruff check/ruff format --check both clean; docstrings, private-helper decomposition, and function sizes match openforge/CLAUDE.md's style guide.

Recommendations

1. Get a ruling from Devon on accept's intended algebra (ANY vs. ALL across list entries) and fix either the SQL or the docstring to match — this is the one item that actually blocks safe consumption of manifests by anyone writing selectors for ad5/lpe.
2. Add a regression test for multi-entry accept (none exists today) once semantics are settled — this gap is squarely why the mismatch shipped.
3. Normalize model row shape between _resolve_select and _resolve_md5/_resolve_full_name to one explicit column projection.
4. Add blueprint_type='model' filtering to _resolve_md5/_resolve_full_name for defense-in-depth.

Overall

The plumbing that ad5 actually depends on — storage_address/file_md5/file_name flow-through, metadata_hash design, fail-fast validation — is solid and gives the sync engine what it needs. The one real correctness issue is the accept selector's actual AND semantics contradicting its documented ANY semantics; that's worth resolving before other manifests start writing multi-value accept lists expecting OR behavior. Everything else here is minor hardening, not a blocker.

[devonjones]

Response to Claude Review

Critical (multi-accept ANDed, contradicting docs): Fixed in code — the documented ANY semantics are correct for manifests (and consistent with multiple select entries unioning), so _resolve_select now runs one engine query per accept entry and unions: (require AND accept₁) OR (require AND accept₂) ≡ require AND (any accept). Pinned by test_multiple_accepts_are_any_not_all using your exact repro shape (two models each carrying only one of the two accept subtrees). Excellent catch — this was the one finding in the round that would have shipped wrong behavior.

Medium (row shape differs by resolution path): Fixed — all resolved models are projected to an explicit MODEL_FIELDS shape (id, names, md5, size, storage_address, timestamps); test_model_rows_have_stable_shape_across_paths asserts key-set equality across select and explicit paths.

Minor (no model-type filter on explicit refs): Fixed — _require_model guards both md5 and full_name refs; test_explicit_ref_to_non_model_raises covers it. You're right that it's currently unreachable via the scanner pipeline, but nothing enforced it.

Minor (dedupe trusts file_md5): Fixed — _project_model fails fast on a model row without file_md5 (the sync diff engine couldn't track it anyway), so dedupe now operates on guaranteed-present keys.

Minor (or treats empty-string override as unset): Won't fix — an empty-string license/category is never a valid value; falling back to the template is the safer interpretation of that input, and None-sensitivity would turn license: "" into a broken thing.

[devonjones]

Verification of round-1 fixes (commit df77937)

1. Accept-ANY fix — VERIFIED CORRECT. Traced _query_tags_include in openforge/db/sql/tags.py:459-509: every require/accept entry appends to a single flat wheres list joined with implicit AND (accept entries are individually parenthesized OR-of-positions for prefix match, but multiple accept entries are still ANDed with each other and with require). Confirms the original diagnosis exactly.

The fix in _resolve_select/_run_tag_search (assembler.py:162-197) runs one tag_search_blueprints call per accept entry, passing the same select["require"]/select["deny"] unchanged on every call — so each batch is require AND deny-exclusion AND accept_i, and the assembler-level union gives require AND (accept_1 OR accept_2 OR ...), matching the docstring's ANY-of-subtrees semantics. Algebraically sound.

Edge cases:

• accept + require together: preserved correctly per above (require passed identically into every batch).
• Empty accepts (select["accept"] == []): accept_batches = [[]], single batch, behaves exactly as pre-fix (require/deny only, no accept join) — no regression for the common single-accept/no-accept case.
• Duplicate rows across batches (e.g. a model tagged under both accept subtrees): correctly deduped later by _dedupe_models on file_md5, and that happens after the "matched no models" empty check in _resolve_select, which only cares whether any batch produced rows — order is fine, no false failures.
• Ceiling check now per-batch (_run_tag_search, not aggregated in _resolve_select): still sound — SELECT_LIMIT is the same cap tag_search_blueprints applies per call via its own LIMIT, so checking truncation at the point where that LIMIT is actually applied is the correct unit of measurement. It does not check the summed union size, but that's not what this guard is for (guarding against tag_search_blueprints's own pagination truncation), and the catalog's real scale (~1,400 designs) makes a multi-thousand-row union implausible.

Ran the repro directly: pytest tests/test_thingiverse_assembler.py tests/test_thingiverse_manifest.py -q → 35 passed, matching the stated 29→35. test_multiple_accepts_are_any_not_all reproduces my exact prior repro shape (two models, each carrying only one of two sibling texture subtrees) and now passes.

Verdict: LGTM, critical resolved.

2. Row shape (medium) — VERIFIED FIXED. MODEL_FIELDS (assembler.py:136-146) is now the single explicit projection point (_project_model), applied uniformly to rows from _resolve_select, _resolve_md5, and _resolve_full_name alike in _resolve_models (line 159). test_model_rows_have_stable_shape_across_paths asserts key-set equality between a selector-resolved row and an explicit-md5-resolved row — passes. _require_model correctly gates both md5 and full_name explicit refs on blueprint_type == 'model' before projection, and _project_model fail-fasts with a clear AssemblyError if file_md5 is falsy, closing the old "trusts every row has md5" gap (minor #4) as a side effect. test_explicit_ref_to_non_model_raises covers the type gate.

Verdict: LGTM.

3. Won't-fix rationale (license/category or-fallback on empty string) — reasonable. An explicit "" override is not a meaningful distinct value from "unset" in this domain (nobody wants a blank Thingiverse license/category), and treating it as "fall back to template" is the safer default over shipping a blank field to the API. Low-traffic manifest-authoring path, not worth the None-sensitivity complexity. Agreed, no objection.

4. Scan of df77937 for new issues:

• N-queries-per-accept: _resolve_select now issues one DB round trip per accept entry instead of one. This is a local CLI/admin tool operating on a ~1,400-row catalog with manifests that realistically list a handful of accept subtrees — non-issue at this scale, correctly judged not worth optimizing (would just add complexity for no real benefit per this project's philosophy).
• logger.info("selector %s matched %d models", select, len(rows)) (assembler.py:177) now logs the pre-dedupe, cross-batch union count rather than a single query's row count. If a model matches more than one accept subtree, or overlaps with a later manifest entry, the logged count can be higher than what actually ends up in files.models after _dedupe_models. Cosmetic only (it's an info log, not used for control flow), but worth a follow-up nit: move/clarify this log to report post-dedupe count if precision matters for ops visibility.
• Manifest.py's (YAMLError, UnicodeDecodeError) narrowing is a good, unflagged bonus fix — no longer mislabels PermissionError/IsADirectoryError etc. as "not valid YAML". No regressions; not covered by a new test but low-risk and self-evidently correct from the diff.

No other correctness regressions found in the diff. ruff check/ruff format --check clean on all three touched files.

Summary

Item	Verdict
Critical: accept ANY-not-ALL	Fixed, verified correct (traced SQL + reran exact repro test, 35/35 pass)
Medium: row shape	Fixed, verified (shape-stability test passes)
Minor: no model-type filter	Fixed, verified
Minor: dedupe trusts md5	Fixed as side effect of fail-fast _project_model
Minor: or empty-string fallback	Won't-fix accepted, reasonable
New: logger.info counts pre-dedupe union	Minor nit, non-blocking

LGTM — no remaining concerns block merge.