query-orchestration/plans/mutable.metadata.plan.combo.v4.review.coded.old.md

Critical Review: mutable.metadata.plan.combo.v4

Findings

High: The milestone rollout is not deployable as written because it schedules migration 129 before 128

Evidence: the tracking doc explicitly places Migration 129 in Milestone 1 while deferring Migration 128 to Milestone 2 (plans/mutable.metadata.plan.combo.v4.tracking.md:85, plans/mutable.metadata.plan.combo.v4.tracking.md:87, plans/mutable.metadata.plan.combo.v4.tracking.md:221). The plan repeats the same sequencing in Milestones 1 and 2 (plans/mutable.metadata.plan.combo.v4.md:1384, plans/mutable.metadata.plan.combo.v4.md:1404). This repo applies embedded migrations with golang-migrate Up() in numeric order (internal/database/migrations.go:64, internal/database/migrations.go:80) and the local generation path does the same with migrate ... up (scripts/database.yml:18, scripts/database.yml:24).

Impact: if Milestone 1 ships with 00000000000129_* but 00000000000128_* does not exist yet, later Milestone 2 cannot safely introduce migration 128. The "vertical slice" claim becomes false in production because the DB version will already be past that number.

Recommendation: renumber now so milestone order is monotonic, or merge the Milestone 1 and Milestone 2 DDL into a single deployable migration set. Do not leave renumbering as an implementation-time choice.

High: The mutual exclusivity invariant is not concurrency-safe

Evidence: the plan claims documents use either legacy extractions or custom metadata, never both, enforced by service checks plus trg_prevent_schema_reassignment (plans/mutable.metadata.plan.combo.v4.md:1341, plans/mutable.metadata.plan.combo.v4.md:1343). The tracking doc only adds an application pre-check in FieldExtractionService that reads documents.custom_schema_id and returns a 409 if it is non-null (plans/mutable.metadata.plan.combo.v4.tracking.md:286, plans/mutable.metadata.plan.combo.v4.tracking.md:288). The current legacy write path begins a transaction, inserts the extraction row, and only later locks version rows; it never locks the parent documents row (internal/fieldextraction/service.go:50, internal/fieldextraction/service.go:65, internal/fieldextraction/service.go:72). The trigger on documents only checks documentFieldExtractionVersions, not in-flight extraction intent (plans/mutable.metadata.plan.combo.v4.md:257, plans/mutable.metadata.plan.combo.v4.md:267).

Impact: AssignSchema can commit custom_schema_id while a concurrent CreateFieldExtraction has already passed its pre-check but has not yet inserted the version row the trigger looks for. Both commits can succeed, leaving the document in the exact forbidden "both systems active" state.

Recommendation: serialize both schema assignment and legacy extraction creation on the same documents row with SELECT ... FOR UPDATE, or add a database-side guard on legacy extraction writes. The current one-sided defense-in-depth is not sufficient.

High: The authorization model no longer matches the original requirement

Evidence: the requirements say each client should curate its own schemas and the client admin should be able to list, delete, and update them (plans/mutable.metadata.requirments.md:10, plans/mutable.metadata.requirments.md:16). The v4 plan instead makes schema CRUD, schema assignment, and reset all super_admin-only (plans/mutable.metadata.plan.combo.v4.md:35, plans/mutable.metadata.plan.combo.v4.md:691, plans/mutable.metadata.plan.combo.v4.md:696). The tracking doc then locks this in with explicit user_admin 403 expectations on every /super-admin/* route (plans/mutable.metadata.plan.combo.v4.tracking.md:170, plans/mutable.metadata.plan.combo.v4.tracking.md:204, plans/mutable.metadata.plan.combo.v4.tracking.md:335).

Impact: this is not an implementation detail. It is a functional change that removes self-service schema administration from client admins and turns schema assignment into an internal platform operation.

Recommendation: either update the requirements explicitly before implementation, or restore client-scoped administration to the appropriate role with object-level client ownership checks. Right now the plan is solving a different problem than the one requested.

High: reset-metadata breaks the stated lifetime immutability contract

Evidence: the requirements state that once metadata has been assigned, a document's schema ID must remain the same for the lifetime of the document (plans/mutable.metadata.requirments.md:14). The v4 plan introduces POST /super-admin/documents/{id}/reset-metadata specifically so the same document can be rebound to a different schema after deleting its metadata history (plans/mutable.metadata.plan.combo.v4.md:585, plans/mutable.metadata.plan.combo.v4.md:587, plans/mutable.metadata.plan.combo.v4.md:627). Milestone 3 then treats that destructive rebind as the supported upgrade path (plans/mutable.metadata.plan.combo.v4.tracking.md:347, plans/mutable.metadata.plan.combo.v4.tracking.md:393).

Impact: the contract becomes "schema is immutable unless a super_admin deletes history." That is materially different from the requirement and changes what downstream systems can assume about a document ID's schema lineage.

Recommendation: either remove this escape hatch from v4 or amend the requirements and downstream contract explicitly to allow destructive schema rebinding. This should not slip in as an undocumented exception.

Medium-High: The plan explicitly accepts cross-client custom metadata access

Evidence: the plan says any caller with custom-metadata:* can read or write any document's custom metadata if they know the UUID, and instructs implementers not to add application-level tenant checks in v4 (plans/mutable.metadata.plan.combo.v4.md:681, plans/mutable.metadata.plan.combo.v4.md:685). The tracking doc repeats that as an accepted limitation (plans/mutable.metadata.plan.combo.v4.tracking.md:165).

Impact: the new feature inherits a known isolation hole and bakes it into the design. That is unsafe in shared environments today and fragile if deployment topology changes later. It also undercuts the requirement that each client owns and curates its own schema-bound metadata.

Recommendation: add document-to-client ownership checks in the new custom metadata and schema-assignment paths now, even if the broader API still needs a larger tenancy retrofit.

Medium: The v4 documents still contain stale v3 instructions that can cause implementation regression

Evidence: the v4 simplification says only Trigger 1 remains and document_custom_metadata.schema_id is gone (plans/mutable.metadata.plan.combo.v4.md:13, plans/mutable.metadata.plan.combo.v4.md:14, plans/mutable.metadata.plan.combo.v4.md:1636). But the "Resolved Design Decisions" section still says three triggers exist and explicitly names trg_enforce_consistent_schema_id as a current decision (plans/mutable.metadata.plan.combo.v4.md:1523, plans/mutable.metadata.plan.combo.v4.md:1529). There are other stale refs too: schema validation still says "POST or PUT" (plans/mutable.metadata.plan.combo.v4.md:961), testing still says "Concurrent PUT requests" (plans/mutable.metadata.plan.combo.v4.md:1493), and the reset section still talks about future updates to document_custom_metadata.schema_id (plans/mutable.metadata.plan.combo.v4.md:651).

Impact: the tracking doc has good anti-regression reminders, but the plan itself is not internally consistent. That makes code review harder and raises the odds that someone reintroduces the dropped column, trigger, or route shape while "following the plan."

Recommendation: scrub stale v3 references before implementation starts and make the v4 plan internally self-consistent. The plan and tracking doc should not disagree about core invariants.

Bottom Line

I would not start implementation from this v4 package yet. The migration ordering issue and the mutual exclusivity race are blocking design problems, and the authorization plus reset behavior need an explicit requirements decision before coding.