query-orchestration/plans/mutable.metadata.plan.combo.v4.review.claude-response.md

Response to Codex Review of v4 Plan

I verified each finding against the plan, tracking doc, and current code before forming a position. Summary of each finding with my stance and reasoning.

Note: the codex review skips from Finding 2 to Finding 4 — there is no Finding 3.

---

1. Per-client isolation on /custom-metadata/*

Agree — with a caveat on severity.

The plan does explicitly accept this limitation (plan 711-726: "a caller with custom-metadata:get/custom-metadata:post ... and knowledge of a document UUID could read or write that document's custom metadata regardless of which client owns the document ... NOT addressed by v3"). Code confirms:

• internal/cognitoauth/mapping.go:7-19 — GetResourceFromRoute() strips to first path segment only.
• cmd/auth_related/permit.setup/permit_policies.yaml:181-184 — "This role requires tenant-level filtering by client_id. Currently, client_user will have access to all clients (no tenant isolation)."

Codex is also right that the plan is internally inconsistent: the reset design promises 404 when caller's client scope doesn't include the document (plan ~643), but no client-scope check is actually wired into the handler layer.

Caveat: the plan leans on per-client deployment topology as the real isolation boundary (line 715: "each client runs in its own isolated stack — separate database, separate services, separate Cognito pool"). If that is truly how production is deployed, the severity drops from "critical security hole" to "latent contract bug when the topology assumption ever changes." The recommendation still stands though — this is a brand-new per-client feature, and baking "topology = security" into a new contract is fragile. Either add document/client ownership enforcement now, or gate the read/write endpoints behind super_admin until object-scoped auth exists.

---

2. Schema version creation can leave multiple active versions in one lineage

Strongly agree. Real bug.

Plan 444-446 says supersede targets only the parent identified by the path {schemaId}, not the currently active version for (client_id, name). Plan 449 locks WHERE client_id=$1 AND name=$2 FOR UPDATE — so the lock could cover all siblings, but the supersede statement as written only touches the parent row.

Concrete failure: v2 is active, caller POSTs .../v1_id/versions → v3 is created and marked active, v1 is marked superseded, v2 remains active. Two active rows in one lineage breaks the "current active version" invariant and will cause the default list (which picks "latest per name") to be non-deterministic depending on how "latest" is resolved.

Same flow also lets a caller create a new version from a retired parent, flipping its lifecycle back to superseded.

Tracking 131-135 and 410 only test "two concurrent posts produce distinct version numbers" — there is no negative test for "create v3 from v1 when v2 is active" or "create from a retired parent."

Fix: require parent to be the currently active version for that lineage, or independently supersede the active version inside the same transaction. Add negative tests.

---

4A. PATCH /super-admin/documents/{id}/schema response fields not backed by service

Agree. Real contract gap.

Plan 558-565 response shows schemaName and schemaVersion. Plan 1081 defines AssignSchema(ctx, documentID, schemaID, actor) error — returns only error. Tracking 280/334 adds no follow-up fetch.

Fix: either shrink the response to {documentId, customSchemaId}, or have AssignSchema return a result struct and add a query task to fetch the schema name/version.

---

4B. Reset response includes fields the ResetMetadataResult struct doesn't have

Agree. Real contract gap.

Plan 625-637 response includes customSchemaId: null and hasCustomMetadata: false. Plan 1091-1099 ResetMetadataResult struct has DocumentID, PreviousSchemaID, PreviousSchemaName, PreviousSchemaVersion, MetadataVersionsDeleted, ResetAt, ResetBy — no customSchemaId or hasCustomMetadata. Tracking 381 repeats the struct fields without adding these two.

Minor observation: after a successful reset, customSchemaId is always null and hasCustomMetadata is always false by definition, so these two fields add nothing to the response. I'd drop them entirely rather than add them to the struct.

---

4C. GET /custom-metadata returns schemaName but nothing fetches it

Agree. Real gap.

Plan 930-941 response includes schemaName. Plan 223 confirms document_custom_metadata has no schema_id column — the schema is resolved via documents.custom_schema_id. Tracking 251-260 query tasks fetch the metadata row and documents.custom_schema_id, but nothing joins to client_metadata_schemas to pick up the name.

Fix: add a join (or a second query) to fetch client_metadata_schemas.name for the currently bound schema, or drop schemaName from the response.

---

5. Actor/createdBy — GetUserSubject returns Cognito sub, not email

Strongly agree. This is a silent API contract change.

• Plan 381, 866 instruct handlers to call cognitoauth.GetUserSubject.
• internal/cognitoauth/auth.go:132-149 — GetUserSubject returns userClaims["sub"].(string), i.e. the opaque Cognito subject UUID.
• Plan response examples at lines 386-395, 636, 893-899 all show "admin@acme.com", "analyst@acme.com", "resetBy": "admin@acme.com".
• serviceAPIs/queryAPI.yaml:3960-3965 and :4719-4724 — existing createdBy fields are declared type: string, format: email, maxLength: 320.

Implemented literally, new endpoints will stuff an opaque UUID into fields that the OpenAPI contract says are email-shaped. That breaks schema validation on consumers, makes logs harder to read, and is a silent regression from how every other endpoint in this service already works.

Fix: decide once. Either the fields are actor IDs (rename them, drop format: email, update the OpenAPI schema) or they are human-readable identities (source them from a claim like email via a new helper, not GetUserSubject). My preference: use the email claim for consistency with existing endpoints, since that's already the contract elsewhere in the service.

---

6. Tracking contradicts itself on triggers

Strongly agree. Obvious documentation bug, trivial fix, easy to miss otherwise.

• Tracking 230-245 (Section 2.2): "Define trg_prevent_legacy_extraction_on_custom_document() function per Section 4.7 Trigger 2" — explicitly requires Trigger 2.
• Tracking 501: "Do NOT add Trigger 2 or Trigger 3. Do NOT add a current_document_custom_metadata view. ... If any of those appear in your diff, something has regressed to v3."

The reminder is referring to v3's old triggers (trg_validate_schema_client_match, trg_enforce_consistent_schema_id) but uses the ordinal labels "Trigger 2/3" which collide with v4's actual Trigger 2. A developer reading the reminder literally will delete the trigger the plan depends on for mutual exclusivity.

Fix: rewrite line 501 to name the forbidden items by identifier: "Do NOT add v3's trg_validate_schema_client_match or trg_enforce_consistent_schema_id."

---

7A. includeAllVersions default vs. the Milestone 1 exit test

Agree. Minor but real — will bite the test writer.

• Plan 456-462: includeAllVersions defaults to false; default status filter is active only.
• Tracking 53-56: "GET the list showing both with correct status" — i.e. expects v1 (superseded) and v2 (active).
• Tracking 200: "GET returns v1 (status superseded) and v2 (status active)."

Default query returns only the latest active version per name, so the test will see only v2 unless it explicitly sets ?includeAllVersions=true&status= (or whatever the "all statuses" sentinel is — the plan doesn't define one).

Fix: make the test description explicit about the query params, and clarify in the plan how to request "all statuses" (empty filter? status=all? a separate flag?). The plan currently describes status as a single-value filter, with no documented way to ask for "everything."

---

7B. Missing reset-vs-SetDocumentMetadata concurrency test

Agree. Important gap.

• Plan 1284: LockDocumentForReset uses "the same parent-row lock shape as LockDocumentForMetadataWrite so the two operations serialize correctly against each other."
• Tracking 392-393: tests reset-vs-reset and reset-vs-AssignSchema only.

The plan's own correctness argument hinges on reset and metadata-write serializing via a shared parent-row lock. That's the pair that actually matters (assign-schema also locks, but a reset-vs-write race is the one that could corrupt metadata if the locks don't match). Not testing it leaves the core claim unverified.

Fix: add a TDD test that races ResetDocumentMetadata against SetDocumentMetadata on the same document and asserts one blocks the other and the final state is consistent.

---

Overall take

I agree with every finding the reviewer raised. Severity ranking as I'd order them:

1. Finding 2 (multi-active-version bug) — blocks implementation. Must fix before Milestone 1 code lands.
2. Finding 6 (trigger reminder contradiction) — 5-minute fix, but high risk of silent regression if missed.
3. Finding 5 (actor field) — API contract change. Decide before any handler is written.
4. Findings 4A/4B/4C (response contracts) — bundle into a single "align response schemas with service return types" pass before implementation.
5. Finding 7B (reset-vs-write concurrency test) — add to tracking, not blocking but important.
6. Finding 1 (per-client isolation) — agreed as a real concern, but the fix depends on a product decision (enforce object scope now vs. gate behind super_admin vs. accept topology-based isolation). Needs Q decision, not a plan tweak.
7. Finding 7A (list test query params) — smallest; fix the test description and clarify the "all statuses" query form.

What I would want clarified from Q before making any plan edits:

• On Finding 1: is per-client-per-stack deployment actually the production model, and are we OK with that being the only isolation boundary for the new endpoints? If yes, narrow the plan's security note to say so clearly. If no, add object-scoped auth to the plan before implementation.
• On Finding 5: should the new endpoints' actor fields match the existing format: email contract (I'd vote yes for consistency), or are we intentionally switching to opaque subject IDs? If the latter, we should also plan to migrate the existing endpoints for consistency.