jacob-mathison/query-orchestration
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Activity

Merged in feature/service-accounts-1 (pull request #227)

Browse Source 
Support for service accounts with static credentials

* feature complete

* tests and docs

* lint fix
This commit is contained in:
Jay Brown
2026-05-21 19:40:04 +00:00
parent e3d9047143
commit 451da3d26d
63 changed files with 4347 additions and 11179 deletions
Download Patch File Download Diff File Expand all files Collapse all files
plans/mutable.metadata.plan.claude.md
-1100
View File
File diff suppressed because it is too large Load Diff
plans/mutable.metadata.plan.codex.comparison.claude.md
-242
View File
@@ -1,242 +0,0 @@
# Codex Plan vs Claude Plan - Comparison Analysis
**Date**: 2026-03-23
**Purpose**: Identify issues or strengths in the Codex plan that our Claude plan may have missed or should consider adopting.
---
## 1. Agreement / Shared Ground
Both plans converge on the same high-level design, which is reassuring:
- Additive hybrid approach: keep legacy fixed-column field extractions, add new JSONB-backed custom metadata
- Schema-as-data stored as JSON Schema documents in JSONB
- Immutable schema versions (new row per revision, new ID)
- Nullable schema reference on the `documents` table
- Schema binding locks once metadata is written
- Application-layer JSON Schema validation (not DB-side `json_matches_schema()`)
- `DISTINCT ON` view for current metadata version
- No GIN index initially
- JSON Schema draft 2020-12
- Client-scoped schemas with cross-client isolation
- Version history for metadata records
These are the foundational decisions and both plans agree on all of them. No action needed.
---
## 2. Issues Codex Raises That We Should Evaluate
### 2.1 DB Triggers for Critical Invariants (STRONG - Consider Adopting)
**What Codex says**: Two database triggers should enforce invariants:
1. Prevent `documents.metadataSchemaId` changes after any extraction exists (legacy or custom)
2. Validate `documents.clientId = documentMetadataSchemas.clientId` on insert/update
**Our plan**: Relies entirely on application-layer enforcement.
**Assessment**: This is the strongest point in the Codex plan that we missed. Application-layer enforcement is correct but fragile -- a future developer could bypass the service layer (e.g., direct SQLC query, migration script, admin fix-up query) and silently violate the invariant. DB triggers make the invariant durable regardless of code path.
**Risk of NOT adopting**: A data integrity bug that is invisible until documents have mismatched schemas. Hard to detect, hard to fix retroactively.
**Recommendation**: **Adopt.** Add both triggers. The cost is two small trigger functions in the migration. The benefit is defense-in-depth for the two most critical invariants (schema lock and client isolation). This aligns with our existing pattern of using DB constraints as safety nets alongside app-layer checks.
---
### 2.2 Three-State Schema Status Enum (MODERATE - Consider Adopting)
**What Codex says**: Uses a `metadataSchemaStatus` enum with three values: `active`, `superseded`, `retired`.
**Our plan**: Uses a boolean `is_active` (true/false).
**Assessment**: The three-state model captures a meaningful distinction:
- `superseded`: automatically set when a newer version is created -- the schema is "replaced" but not manually disabled
- `retired`: explicitly disabled by an admin -- different intent, possibly different business rules (e.g., retired schemas might warrant a warning in the UI)
With a boolean, both cases collapse to `is_active = false` and the system cannot distinguish "replaced by v2" from "admin decided this schema is bad."
**Risk of NOT adopting**: Minor. We can always add the distinction later. But it's cleaner to model it upfront since it costs nothing extra at the DB level and avoids a later migration.
**Recommendation**: **Consider adopting.** A three-state enum is a small change that gives us more precise semantics. If we keep the boolean, we should at minimum document that `is_active = false` covers both cases and note it as a known simplification.
---
### 2.3 `schemaHash` for Deduplication Detection (WEAK - Note But Skip)
**What Codex says**: Includes a `schemaHash text NOT NULL` column on the schema table.
**Our plan**: No hash column.
**Assessment**: Useful for detecting accidental duplicate submissions (same schema content submitted as a "new version"). Could also speed up equality checks. However, it adds complexity (what hash algorithm? is it normalized JSON before hashing?) and the use case is marginal for v1. An admin accidentally creating a duplicate version is a minor annoyance, not a data integrity issue.
**Recommendation**: **Skip for now.** Note it as a potential future addition. If duplicate detection becomes a real problem, we can add it without schema changes (just a new column + backfill).
---
### 2.4 `supersedesSchemaId` / Explicit Version Chain (WEAK - Skip)
**What Codex says**: Each schema row has a `supersedesSchemaId uuid REFERENCES documentMetadataSchemas(id)` creating an explicit linked-list chain of versions.
**Our plan**: Relies on `(client_id, name, version)` ordering to determine lineage. The `PUT` response includes `previousVersionId` but it's computed, not stored.
**Assessment**: Both approaches work. The explicit FK chain makes traversal trivial (`SELECT * FROM ... WHERE supersedesSchemaId = ?`) but `(name, version)` ordering is equally functional and simpler. An explicit chain also creates complexity around edge cases: what if a superseded schema is itself deleted? What if someone creates a revision of a superseded (not latest) version?
**Recommendation**: **Skip.** Our `(name, version)` ordering is simpler and sufficient. The lineage is already navigable via `ORDER BY version`.
---
### 2.5 `documentClass` Column (WEAK - Skip)
**What Codex says**: Includes a `documentClass text` column on the schema table for categorization.
**Our plan**: No document class column. The schema `name` and `description` serve the categorization role.
**Assessment**: This is a thin metadata field. It could be useful for filtering schemas by document type (e.g., "show me all schemas for aircraft engineering documents"). But `name` already serves this purpose, and adding a separate classification dimension without clear UI or API requirements is speculative.
**Recommendation**: **Skip.** Can be added later if a classification use case emerges. The schema `name` and `description` fields handle this for now.
---
### 2.6 Metadata Table Design: Two Tables vs One (MODERATE - Evaluate)
**What Codex says**: Uses two tables mirroring the existing legacy pattern:
- `documentMetadataExtractions` (the actual JSONB data)
- `documentMetadataExtractionVersions` (version tracking)
**Our plan**: Uses a single `document_custom_metadata` table with a `version` column combining both concerns.
**Assessment**: This is a tradeoff:
| Aspect | Two-table (Codex) | One-table (Ours) |
|--------|-------------------|------------------|
| Consistency with existing code | Matches legacy `documentFieldExtractions` + `documentFieldExtractionVersions` pattern | New pattern, different from legacy |
| Simplicity | More tables, more joins | Fewer tables, simpler queries |
| Row-level locking for version increment | Version rows can be locked independently of data rows | Must lock the last data row for version increment |
| Query complexity | Need JOIN for full data | Direct SELECT |
The existing codebase uses the two-table pattern for field extractions. Matching it means developers familiar with the legacy code can reason about the new code by analogy. Breaking from it means two different versioning patterns in the same codebase.
**Risk of NOT adopting**: Cognitive overhead for developers who expect the two-table pattern. Potential row-locking issues if we need to increment versions under concurrency (though our traffic patterns likely don't warrant this concern).
**Recommendation**: **Evaluate.** The consistency argument is real. However, our single-table approach is simpler and YAGNI applies -- we don't need the version table separation unless we have a concrete reason. If Q prefers consistency with the legacy pattern, we should switch. Otherwise, keep our simpler design and document why we deviated.
---
### 2.7 Mutual Exclusivity: Legacy OR Custom (MODERATE - Discuss with Q)
**What Codex says**: Strict mutual exclusivity:
- If `metadataSchemaId IS NULL`, only legacy `/field-extractions` endpoints work
- If `metadataSchemaId IS NOT NULL`, only custom `/field-extractions/metadata` endpoints work
- Assigning a schema to a document with legacy extractions returns `409 Conflict`
**Our plan**: A document can have BOTH static field extractions AND custom metadata. They are "independent and complementary."
**Assessment**: This is a fundamental design philosophy difference:
- **Codex (exclusive)**: Cleaner model, prevents confusion about which system is "authoritative" for a document. Simpler to reason about. But constrains flexibility -- what if a document needs both legacy fields AND custom fields during a transition period?
- **Ours (both allowed)**: More flexible. Allows gradual migration. But introduces ambiguity: if a document has both, which is the "real" metadata? Could lead to inconsistencies if different systems write to different stores for the same document.
**Risk of our approach**: A document could end up with conflicting data in legacy fields and custom metadata, with no clear source of truth. This is a data governance concern, not a technical one.
**Recommendation**: **Discuss with Q.** This is a business decision. If the expectation is that documents will gradually migrate from legacy to custom, allowing both may be needed during transition. If the expectation is a clean cut-over, mutual exclusivity is cleaner. Our plan should explicitly document whichever choice is made and why.
---
### 2.8 Route Structure: `/field-extractions/...` vs Split (MINOR)
**What Codex says**: All new endpoints under `/field-extractions/...` to stay aligned with existing Permit resource naming.
**Our plan**: Admin schemas under `/admin/custom-schemas`, metadata under `/custom-metadata`.
**Assessment**: Our split is intentional and defensible -- it separates admin-scoped operations from user-scoped operations, which maps cleanly to role-based access. The Codex approach keeps everything grouped under one resource family, which simplifies Permit configuration but mixes admin and user operations under the same prefix.
**Recommendation**: **Keep our approach.** The admin/user split is cleaner for RBAC. We already account for the Permit.io changes needed.
---
### 2.9 Schema Size Limit: 256KB vs 64KB (MINOR)
**What Codex says**: 256KB limit.
**Our plan**: 64KB limit with a tunable constant.
**Assessment**: Both are reasonable. 64KB is generous for a JSON Schema document (most schemas will be 1-5KB). 256KB is very generous. Since we made it a tunable constant (`MaxSchemaDefinitionBytes`), the exact value is easy to change.
**Recommendation**: **Keep 64KB.** It's more than enough, and a lower limit is a better default. If a client hits it, we bump the constant.
---
### 2.10 Schema Validation: `additionalProperties` Required (MINOR - Consider)
**What Codex says**: The server should reject schema creation unless `additionalProperties` is explicitly present at root level.
**Our plan**: `additionalProperties: false` is "recommended but not required."
**Assessment**: Requiring it prevents a common mistake -- submitting a schema without `additionalProperties`, which defaults to `true` in JSON Schema, meaning the schema accepts any extra fields. This is almost never what the client admin intends. Requiring explicit declaration forces a conscious choice.
**Recommendation**: **Consider adopting.** Requiring `additionalProperties` to be explicitly set (not necessarily `false`, just present) is a small validation rule that prevents a common gotcha. Low cost, moderate value.
---
### 2.11 Request Body Should Not Carry `schemaId` (ALREADY HANDLED)
**What Codex says**: The metadata write request body should not include `schemaId` -- the server derives it from the document's assigned schema.
**Our plan**: Same approach. The `POST /custom-metadata` request contains `documentId` and `metadata` but not `schemaId`. The response includes `schemaId` (server-derived).
**Assessment**: Both plans agree. No action needed.
---
### 2.12 Usage Stats in Schema Responses (MINOR - Already Handled Differently)
**What Codex says**: Schema detail response includes a `usage` block: `assignedDocumentCount`, `metadataRecordCount`, `canDelete`.
**Our plan**: Schema list/detail responses include `documentCount`.
**Assessment**: The Codex response is slightly richer (`canDelete` is convenient for UIs, `metadataRecordCount` distinguishes "assigned but no data yet" from "actively in use"). Our `documentCount` covers the main use case.
**Recommendation**: **Consider adding `canDelete`.** It's a simple boolean derived from the count and saves the client a mental step. `metadataRecordCount` is nice-to-have but not essential for v1.
---
## 3. Things Our Plan Has That Codex Missed
### 3.1 Bulk Folder Assignment
Our plan includes `POST /admin/folders/{folderId}/assign-schema` with recursive subfolder traversal, skip reporting, and transactional safety. Codex has no bulk operations at all. This is a significant operational feature for large clients.
### 3.2 `client_admin` Role
Our plan defines a new `client_admin` role with clear permission boundaries. Codex mentions the need for Permit.io updates but doesn't define a new role or permission model.
### 3.3 Detailed Service Layer Design
Our plan provides Go code outlines for the service, validator, and controller layers. Codex stays at the SQL/API level and doesn't describe the Go implementation structure.
### 3.4 Sample Schemas / Documentation
Our plan includes comprehensive sample schemas covering all supported JSON Schema types. Codex doesn't address documentation.
### 3.5 More Granular Implementation Phases
Our plan has 11 phases with explicit dependency tracking. Codex has 4 broad phases.
---
## 4. Summary: Recommended Actions
| # | Codex Issue | Strength | Action |
|---|------------|----------|--------|
| 2.1 | DB triggers for invariants | **STRONG** | **Adopt** -- add triggers for schema lock + client match |
| 2.2 | Three-state schema status | MODERATE | **Consider** -- enum is cleaner than boolean |
| 2.3 | `schemaHash` | Weak | Skip |
| 2.4 | `supersedesSchemaId` chain | Weak | Skip |
| 2.5 | `documentClass` column | Weak | Skip |
| 2.6 | Two-table metadata pattern | MODERATE | **Discuss with Q** -- consistency vs simplicity |
| 2.7 | Mutual exclusivity (legacy OR custom) | MODERATE | **Discuss with Q** -- business decision |
| 2.10 | Require `additionalProperties` present | Minor | **Consider** -- prevents common mistake |
| 2.12 | `canDelete` in responses | Minor | **Consider** -- small UX improvement |
**Bottom line**: The Codex plan is a competent alternative design that arrives at the same destination via slightly different choices. The one genuinely strong point we should adopt is **DB triggers for critical invariants** (2.1). The three moderate items (2.2, 2.6, 2.7) deserve a decision from Q. Everything else is either already covered in our plan or is a minor preference difference that doesn't materially affect the design.
plans/mutable.metadata.plan.codex.md
-539
View File
@@ -1,539 +0,0 @@
# Mutable Metadata Plan
## What I Studied
The current field extraction design is rigid end to end:
- `serviceAPIs/queryAPI.yaml` hard-codes `FieldExtractionRequest`, `SingleFields`, and `ArrayFieldItem`.
- `internal/database/migrations_collapsed/00000000000008_field_extractions.up.sql` hard-codes 19 single-value columns and 112 array-value columns.
- `internal/database/queries/fieldextractions.sql` and `internal/fieldextraction/service.go` implement document-scoped versioning by inserting immutable rows and a separate versions table.
- `internal/database/migrations_collapsed/00000000000006_documents.up.sql` shows that `documents` currently has no schema pointer.
- `internal/document/get.go`, `internal/folder/service.go`, and `serviceAPIs/queryAPI.yaml` expose field extraction data as `textRecord` / `hasTextRecord`, even though it is really structured metadata.
- `internal/server/api/listener.go` uses the OpenAPI middleware for request-shape validation, but dynamic per-client schema validation does not exist today.
- `internal/database/README.md` mentions `json_matches_schema()`, but I did not find an active migration or function definition for it, so this plan assumes application-side JSON Schema validation.
- `cmd/auth_related/permit.setup/permit_policies.yaml` only maps the existing field extraction endpoints, so any new routes here will require Permit mapping updates.
## Recommendation
Use an additive hybrid design.
- Keep the current fixed-column field extraction model for legacy documents where no custom schema is assigned.
- Add a new client-scoped JSON Schema registry.
- Add a new JSONB-backed metadata record store for documents that opt into a custom schema.
- Pin each document to exactly one schema revision through a nullable document column.
- Allow schema assignment changes only until the first metadata record exists for that document.
- Do not try to generate physical columns per client schema. The current codebase is too strongly coupled to generated OpenAPI types, SQLC queries, and static SQL for that to scale cleanly.
This gives the team a safe path:
- Existing `POST/GET /field-extractions` flows keep working for legacy data.
- New clients can use fully custom schemas without changing the database schema every time they add fields.
- The system can later deprecate the legacy fixed-field model without blocking this first implementation.
## Core Design
### 1. Document-level schema pinning
Add a nullable schema reference to `documents`.
Recommended column name:
- `metadataSchemaId uuid NULL`
Meaning:
- `NULL`: this document uses the legacy fixed `SingleFields` + `ArrayFieldItem` model.
- non-`NULL`: this document uses the custom JSON Schema-backed metadata model.
Rule:
- A document may change `metadataSchemaId` only while it has no field extraction record of any kind.
- Once the first legacy extraction or custom metadata record exists, schema assignment is locked forever.
### 2. Client-managed schema registry
Store each schema revision as its own immutable row.
Recommended table:
```sql
CREATE TYPE metadataSchemaStatus AS ENUM ('active', 'superseded', 'retired');
CREATE TABLE documentMetadataSchemas (
id uuid PRIMARY KEY DEFAULT uuid_generate_v7(),
clientId varchar(255) NOT NULL REFERENCES clients(clientId),
schemaKey varchar(255) NOT NULL,
version int NOT NULL,
title text NOT NULL,
description text,
documentClass text,
status metadataSchemaStatus NOT NULL DEFAULT 'active',
supersedesSchemaId uuid REFERENCES documentMetadataSchemas(id),
schema jsonb NOT NULL,
schemaHash text NOT NULL,
createdAt timestamp NOT NULL DEFAULT NOW(),
createdBy varchar(255) NOT NULL,
CHECK (jsonb_typeof(schema) = 'object'),
UNIQUE (clientId, schemaKey, version)
);
CREATE INDEX idx_documentmetadataschemas_client
ON documentMetadataSchemas(clientId);
CREATE INDEX idx_documentmetadataschemas_client_key_status
ON documentMetadataSchemas(clientId, schemaKey, status);
```
Why a single table instead of family + version tables:
- every revision already needs a new system-assigned ID
- `schemaKey + version` is enough to group revisions
- it matches the current repo preference for straightforward tables and version rows
Suggested semantics:
- `schemaKey`: stable logical name inside a client, for example `aircraft-engineering`
- `version`: starts at `1`, increments on each revision
- `id`: exact revision identifier that documents reference
- `status`:
- `active`: assignable
- `superseded`: replaced by a newer revision
- `retired`: manually disabled for future assignment
### 3. Custom metadata record storage
Use JSONB for the extracted metadata itself.
Recommended tables:
```sql
CREATE TABLE documentMetadataExtractions (
id uuid PRIMARY KEY DEFAULT uuid_generate_v7(),
documentId uuid NOT NULL REFERENCES documents(id),
metadataSchemaId uuid NOT NULL REFERENCES documentMetadataSchemas(id),
metadata jsonb NOT NULL,
createdAt timestamp NOT NULL DEFAULT NOW(),
createdBy varchar(255) NOT NULL,
CHECK (jsonb_typeof(metadata) = 'object')
);
CREATE INDEX idx_documentmetadataextractions_document
ON documentMetadataExtractions(documentId);
CREATE INDEX idx_documentmetadataextractions_schema
ON documentMetadataExtractions(metadataSchemaId);
CREATE TABLE documentMetadataExtractionVersions (
id uuid PRIMARY KEY DEFAULT uuid_generate_v7(),
metadataExtractionId uuid NOT NULL REFERENCES documentMetadataExtractions(id),
documentId uuid NOT NULL REFERENCES documents(id),
metadataSchemaId uuid NOT NULL REFERENCES documentMetadataSchemas(id),
version bigint NOT NULL,
createdAt timestamp NOT NULL DEFAULT NOW(),
createdBy varchar(255) NOT NULL,
UNIQUE (documentId, version)
);
CREATE INDEX idx_documentmetadataextractionversions_document
ON documentMetadataExtractionVersions(documentId);
```
Recommended view:
```sql
CREATE VIEW currentDocumentMetadataExtractions AS
SELECT DISTINCT ON (dme.documentId)
dme.id,
dme.documentId,
dme.metadataSchemaId,
dme.metadata,
dmev.version,
dmev.createdAt,
dmev.createdBy
FROM documentMetadataExtractions dme
JOIN documentMetadataExtractionVersions dmev
ON dmev.metadataExtractionId = dme.id
ORDER BY dme.documentId, dmev.version DESC, dmev.id DESC;
```
Important choice:
- do not add a GIN index on `metadata` initially
- index only `documentId`, `metadataSchemaId`, and version paths for now
- add GIN or projected search columns later only if querying custom fields becomes a real requirement
## API Design
### Routing recommendation
To stay aligned with the existing `FieldExtractionService` tag and current Permit resource naming, use the `/field-extractions/...` prefix for the new endpoints.
Recommended routes:
| Method | Path | Purpose |
| --- | --- | --- |
| `POST` | `/field-extractions/schemas` | Create schema revision `v1` |
| `GET` | `/field-extractions/schemas` | List client schemas |
| `GET` | `/field-extractions/schemas/{schemaId}` | Get one schema revision |
| `POST` | `/field-extractions/schemas/{schemaId}/revisions` | Create new revision with new ID |
| `DELETE` | `/field-extractions/schemas/{schemaId}` | Delete unused schema revision |
| `PUT` | `/field-extractions/documents/{documentId}/schema` | Assign or clear a document schema before first extraction |
| `POST` | `/field-extractions/metadata` | Create custom metadata record version |
| `GET` | `/field-extractions/metadata` | Get current custom metadata record |
| `GET` | `/field-extractions/metadata/version` | Get custom metadata record by version |
| `GET` | `/field-extractions/metadata/history` | Get custom metadata version history |
Cleaner but slightly broader routing alternative:
- `/document/{id}/metadata-schema` instead of `/field-extractions/documents/{documentId}/schema`
That route is more document-centric, but it will require matching Permit and middleware updates outside the current `field-extractions` path family.
### Schema registry payloads
Create request:
```json
{
"clientId": "acme",
"schemaKey": "aircraft-engineering",
"title": "Aircraft Engineering Document",
"description": "ACME aircraft engineering extraction schema",
"documentClass": "aircraft engineering",
"jsonSchema": {
"$schema": "https://json-schema.org/draft/2020-12/schema",
"type": "object",
"additionalProperties": false,
"properties": {
"manufacturer": { "type": "string", "maxLength": 200 },
"engine_hours": { "type": "number", "minimum": 0 }
}
},
"createdBy": "admin@acme.com"
}
```
Schema detail response:
```json
{
"id": "019....",
"clientId": "acme",
"schemaKey": "aircraft-engineering",
"version": 3,
"title": "Aircraft Engineering Document",
"description": "ACME aircraft engineering extraction schema",
"documentClass": "aircraft engineering",
"status": "active",
"supersedesSchemaId": "019....",
"createdAt": "2026-03-23T12:00:00Z",
"createdBy": "admin@acme.com",
"usage": {
"assignedDocumentCount": 42,
"metadataRecordCount": 37,
"canDelete": false
},
"jsonSchema": { "...": "..." }
}
```
Revision create request:
```json
{
"title": "Aircraft Engineering Document",
"description": "Added section for turbine notes",
"jsonSchema": { "...": "..." },
"createdBy": "admin@acme.com",
"changeSummary": "Added turbine_notes and tightened serial number validation"
}
```
Revision semantics:
- creates a brand new row with a new `id`
- keeps the same `schemaKey`
- increments `version`
- marks the old schema row as `superseded`
- does not mutate any document already pinned to the older schema ID
### Document schema assignment payload
Request:
```json
{
"schemaId": "019....",
"assignedBy": "admin@acme.com"
}
```
To clear before first extraction:
```json
{
"schemaId": null,
"assignedBy": "admin@acme.com"
}
```
Response:
```json
{
"documentId": "019....",
"metadataSchemaId": "019....",
"locked": false
}
```
### Custom metadata record payloads
Create request:
```json
{
"documentId": "019....",
"createdBy": "user@acme.com",
"metadata": {
"manufacturer": "GE",
"engine_hours": 481.25,
"inspection_notes": [
{ "section": "fuel", "status": "pass" }
]
}
}
```
Current record response:
```json
{
"id": "019....",
"documentId": "019....",
"metadataSchemaId": "019....",
"schemaKey": "aircraft-engineering",
"schemaVersion": 3,
"version": 2,
"metadata": {
"manufacturer": "GE",
"engine_hours": 481.25
},
"createdAt": "2026-03-23T12:34:56Z",
"createdBy": "user@acme.com"
}
```
History response:
```json
{
"versions": [
{
"id": "019....",
"documentId": "019....",
"metadataSchemaId": "019....",
"version": 2,
"createdAt": "2026-03-23T12:34:56Z",
"createdBy": "user@acme.com"
}
]
}
```
## Document API Changes
Extend the document responses so clients can see schema assignment without fetching the schema registry separately.
Add to `DocumentEnriched` and `DocumentInFolder`:
- `metadataSchemaId: uuid | null`
- `hasMetadataRecord: boolean`
- `metadataRecord` optional, only when requested
Recommended query parameter:
- `metadataRecord=true`
Compatibility note:
- keep the existing `hasTextRecord` / `textRecord` fields for legacy fixed-field records
- do not overload `textRecord` with custom JSON metadata
- treat `metadataRecord` as the new preferred field for schema-bound documents
This avoids breaking current consumers that deserialize `FieldExtractionResponse`.
## Validation and Invariants
### JSON Schema rules
The server should reject schema creation unless all of the following pass:
- valid JSON object
- compiles successfully as JSON Schema
- root `type` is `object`
- root `additionalProperties` is explicitly present
- size stays under a practical guardrail, for example 256 KB
Recommended draft:
- Draft 2020-12
Recommended validation location:
- application layer in the new schema service
- do not rely on database-side JSON Schema validation for the first implementation
Reason:
- the repo already uses OpenAPI middleware for static contracts, but dynamic schema compilation is not available there
- I did not find a live DB implementation of `json_matches_schema()`
### Custom metadata rules
When `POST /field-extractions/metadata` is called:
1. load the document
2. confirm `metadataSchemaId` is non-null
3. load the exact schema row referenced by the document
4. confirm schema and document belong to the same client
5. validate `metadata` against that schema
6. lock the version rows for that document
7. insert immutable extraction row plus new version row
Important: the request body should not carry `schemaId`. The server should derive it from the document assignment to avoid client/server drift.
### Legacy/custom exclusivity rules
- if `documents.metadataSchemaId IS NULL`, only the existing legacy `/field-extractions` endpoints are allowed
- if `documents.metadataSchemaId IS NOT NULL`, only the new custom `/field-extractions/metadata` endpoints are allowed
- assigning a schema to a document that already has a legacy extraction is rejected with `409 Conflict`
- changing or clearing a schema after the first custom metadata record exists is rejected with `409 Conflict`
### Delete rules
`DELETE /field-extractions/schemas/{schemaId}` should succeed only when all are true:
- no document points to that schema in `documents.metadataSchemaId`
- no metadata extraction row references it
- no metadata extraction version row references it
Otherwise return `409 Conflict`.
## DB-enforced protections
I recommend two database triggers in addition to service checks.
### 1. Prevent schema reassignment after first extraction
On `documents`, before update of `metadataSchemaId`:
- if the value changes
- and the document has any row in `documentFieldExtractionVersions`
- or any row in `documentMetadataExtractionVersions`
- then raise an error
This enforces the "locked after first extraction" rule even if another code path updates the row later.
### 2. Validate client/schema match
On `documents`, before insert/update of `metadataSchemaId`:
- if non-null, ensure `documents.clientId = documentMetadataSchemas.clientId`
This can also be enforced in the assignment service, but a trigger makes the invariant durable.
## Why JSONB is the right tradeoff here
This codebase currently pays a large change cost whenever the extraction shape changes:
- OpenAPI changes
- generated Go type changes
- conversion helper changes
- SQLC query changes
- migration changes
- view changes
That is acceptable for one fixed 19/112-field model. It is not acceptable for per-client schemas that can change frequently.
JSONB plus JSON Schema avoids that churn:
- per-client field definitions become data, not code
- extraction validation becomes runtime validation against the stored schema
- version history stays consistent with the current service style
Tradeoff:
- JSONB is not ideal for ad hoc SQL reporting across arbitrary custom fields
Mitigation:
- keep the initial implementation retrieval-focused
- if analytics/search later require it, add projected indexes or a derived flattened table keyed by JSON path
## Auth and middleware impact
The current Permit mapping only includes:
- `/field-extractions`
- `/field-extractions/version`
- `/field-extractions/history`
This design requires new entries for:
- `/field-extractions/schemas`
- `/field-extractions/schemas/{schemaId}`
- `/field-extractions/schemas/{schemaId}/revisions`
- `/field-extractions/documents/{documentId}/schema`
- `/field-extractions/metadata`
- `/field-extractions/metadata/version`
- `/field-extractions/metadata/history`
If the team chooses the cleaner `/document/{id}/metadata-schema` route instead, the `document` Permit resource mapping also needs to be extended.
## Rollout Plan
### Phase 1
- add `documents.metadataSchemaId`
- add `documentMetadataSchemas`
- add `documentMetadataExtractions`
- add `documentMetadataExtractionVersions`
- add current-view and usage queries
### Phase 2
- add schema registry endpoints
- add assignment endpoint
- add custom metadata endpoints
- add JSON Schema validation service
### Phase 3
- extend document responses with `metadataSchemaId`, `hasMetadataRecord`, and optional `metadataRecord`
- keep legacy `textRecord` behavior unchanged
### Phase 4
- optionally add deprecation notices around the legacy fixed-field model
- evaluate whether new clients should be prohibited from using the legacy model at all
## Recommended non-goals for this change
Do not include these in the first pass:
- migrating existing legacy field extraction rows into JSONB
- replacing the current legacy `/field-extractions` response shape
- building SQL search/filtering across arbitrary custom metadata fields
- building a flattened field catalog table unless a UI requirement forces it immediately
## Final Recommendation
Implement custom schemas as a new schema registry plus JSONB-backed metadata version store, while preserving the current fixed-column field extraction path for legacy documents.
That approach fits the current repo architecture best:
- it respects the existing version-per-document pattern
- it avoids forcing a breaking rewrite of the static `FieldExtractionResponse`
- it gives each client independent schema control
- it keeps future migration toward a fully generic metadata model open
plans/mutable.metadata.plan.combo.md
-1364
View File
File diff suppressed because it is too large Load Diff
plans/mutable.metadata.plan.combo.v2.md
-1566
View File
File diff suppressed because it is too large Load Diff
plans/mutable.metadata.plan.combo.v3.md
-1729
View File
File diff suppressed because it is too large Load Diff
plans/mutable.metadata.plan.combo.v3.tracking.md
-652
View File
@@ -1,652 +0,0 @@
# Mutable Metadata v3 - Implementation Tracking
**Source plan**: `plans/mutable.metadata.plan.combo.v3.md`
**Journal (details / decisions / blockers)**: `./journals/implement_mutableMetadata.md`
**Purpose**: One-glance status of every task needed to ship v3. Each checkbox is a discrete, verifiable unit of work. TDD rule: write the test first, watch it fail, make it pass, check the box. If a task requires Q's input or produces a decision, log it to the journal and keep the checkbox state accurate here.
## How to use this document
- `[ ]` = not started
- `[~]` = in progress
- `[x]` = done AND tests passing (`task fullsuite:ci` clean for the affected code paths)
- `[!]` = blocked -- add a short `(blocked: <why>)` suffix and open a journal entry
- `[-]` = consciously skipped -- add a short `(skipped: <why>)` suffix
- Every task line ends with a `test:` marker indicating where the passing test(s) live or what test type proves it.
- When a whole phase is green, flip the phase header to `COMPLETE` and date-stamp it.
- Never mark a phase complete until `task fullsuite:ci` ends with `All coverage checks passed!` with no `FAIL` lines.
---
## Phase roll-up (high-level)
| Phase | Title | Status | Started | Completed |
|-------|-------|--------|---------|-----------|
| 1 | Database migrations | NOT STARTED | | |
| 2 | SQLC queries + generate | NOT STARTED | | |
| 3 | Schema validator component | NOT STARTED | | |
| 4 | Service layer (schema CRUD, metadata, assign, bulk, reset) | NOT STARTED | | |
| 5a | Legacy field extraction guard | NOT STARTED | | |
| 5b | Delete cascade updates | NOT STARTED | | |
| 6 | OpenAPI spec updates | NOT STARTED | | |
| 7a | Permit.io policy file updates | NOT STARTED | | |
| 7b | Run permit setup tool (dev/uat/prod) | NOT STARTED | | |
| 8 | API controllers + codegen | NOT STARTED | | |
| 9 | Document endpoint modifications | NOT STARTED | | |
| 10 | Bulk folder schema assignment endpoint | NOT STARTED | | |
| 10b | Reset-metadata endpoint (v3) | NOT STARTED | | |
| 11 | Integration & authorization tests | NOT STARTED | | |
| 12 | Documentation updates | NOT STARTED | | |
---
## Phase 1 - Database migrations
**Goal**: Land all DDL needed for the feature. No Go code touched in this phase.
**Dependencies**: None.
**Exit criteria**: Migrations up/down cleanly on a fresh database AND on a database with existing data; `task db:generate` runs clean; triggers verifiable via direct SQL.
> **Migration numbering note**: The repo already contains migrations through `00000000000126_batch_document_outcomes_delete_actions`. v3 therefore starts at `127` to avoid filename collisions. If another branch lands new migrations before this feature merges, renumber this block to the next free sequence and update the references below.
### 1.1 Migration 127 - `schema_status_type` enum + `client_metadata_schemas`
- [ ] Create `internal/database/migrations/00000000000127_create_client_metadata_schemas.up.sql` with `CREATE TYPE schema_status_type AS ENUM ('active', 'superseded', 'retired')` -- test: migration applies cleanly on empty DB
- [ ] Same migration creates `client_metadata_schemas` table per Section 4.2 (columns, PK, FK to `clients`, `schema_def jsonb`, `version`, `status`, timestamps, `created_by`) -- test: `\d client_metadata_schemas` shows all columns
- [ ] Add `CONSTRAINT uq_client_schema_name_version UNIQUE (client_id, name, version)` -- test: direct SQL INSERT of two rows with same `(client_id, name, version)` fails
- [ ] Add `CONSTRAINT chk_schema_def_size CHECK (pg_column_size(schema_def) <= 70000)` -- test: insert of a schema_def larger than 70000 bytes fails with CHECK violation
- [ ] Add indexes `idx_cms_client_id`, `idx_cms_client_name`, `idx_cms_client_status` -- test: `\di client_metadata_schemas` shows the three indexes
- [ ] Create `00000000000127_create_client_metadata_schemas.down.sql` dropping the table and enum in correct order -- test: up then down leaves DB state byte-identical to pre-migration
- [ ] Run `task db:generate` after migration lands -- test: no errors
### 1.2 Migration 128 - `document_custom_metadata` table + view
- [ ] Create `00000000000128_create_document_custom_metadata.up.sql` with `document_custom_metadata` table per Section 4.3 (columns, FKs, `metadata jsonb`, version column) -- test: migration applies cleanly
- [ ] Add `CONSTRAINT uq_doc_custom_metadata_version UNIQUE (document_id, version)` -- test: direct SQL duplicate `(document_id, version)` INSERT fails
- [ ] Add indexes `idx_dcm_document_id`, `idx_dcm_schema_id`, `idx_dcm_doc_version_desc` -- test: indexes present
- [ ] Create `current_document_custom_metadata` view per Section 4.5 (`DISTINCT ON (document_id) ... ORDER BY document_id, version DESC`) -- test: insert 3 versions for same doc, `SELECT * FROM current_document_custom_metadata WHERE document_id = X` returns only version 3
- [ ] Create corresponding `down.sql` (drop view then table) -- test: up/down round trip clean
### 1.3 Migration 129 - Add `custom_schema_id` to `documents`
- [ ] Create `00000000000129_add_custom_schema_id_to_documents.up.sql` -- `ALTER TABLE documents ADD COLUMN custom_schema_id uuid NULL REFERENCES client_metadata_schemas(id)` -- test: column present, nullable, FK enforced
- [ ] Create `idx_documents_custom_schema_id` -- test: index present
- [ ] Write `down.sql` that drops index then column -- test: round trip clean
- [ ] Verify existing documents rows have `custom_schema_id = NULL` after migration -- test: `SELECT COUNT(*) FROM documents WHERE custom_schema_id IS NOT NULL` returns 0
### 1.4 Migration 130 - Invariant triggers
- [ ] Create `00000000000130_add_schema_invariant_triggers.up.sql` -- test: migration applies cleanly
- [ ] Define `trg_prevent_schema_reassignment()` function per Section 4.7 Trigger 1 -- test: function exists in `pg_proc`
- [ ] Attach trigger to `documents` BEFORE UPDATE OF `custom_schema_id` -- test: trigger listed in `\d documents`
- [ ] Trigger 1 unit test: insert document, assign schema, write custom metadata, attempt UPDATE `custom_schema_id` -- expect `RAISE EXCEPTION` with "custom metadata already exists" message
- [ ] Trigger 1 unit test: insert document, add legacy extraction row, attempt UPDATE `custom_schema_id` -- expect `RAISE EXCEPTION` with "legacy field extractions already exist" message
- [ ] Trigger 1 unit test: insert document with no metadata and no extractions, UPDATE `custom_schema_id` from schema_a to schema_b -- expect success (pre-binding change allowed)
- [ ] Define `trg_validate_schema_client_match()` function per Section 4.7 Trigger 2 -- test: function exists
- [ ] Attach trigger to `documents` BEFORE INSERT OR UPDATE OF `custom_schema_id` -- test: trigger listed
- [ ] Trigger 2 unit test: attempt to assign a schema owned by client A to a document owned by client B -- expect exception
- [ ] Trigger 2 unit test: attempt to assign a non-existent schema ID -- expect "Schema % does not exist"
- [ ] Trigger 2 unit test: same-client assignment -- expect success
- [ ] Define `trg_enforce_consistent_schema_id()` function per Section 4.7 Trigger 3 -- test: function exists
- [ ] Attach trigger to `document_custom_metadata` BEFORE INSERT -- test: trigger listed
- [ ] Trigger 3 unit test: insert metadata v1 under schema_a, attempt direct SQL INSERT of metadata v2 under schema_b for same document -- expect exception
- [ ] Trigger 3 unit test: insert metadata v1 and v2 under same schema -- expect success
- [ ] Write `down.sql` dropping triggers and functions in correct order -- test: up/down round trip clean
### 1.5 Phase 1 integration
- [ ] Run `task generate` -- test: clean
- [ ] Run `task test:unit:short` -- test: all existing tests still pass (no regression)
- [ ] Commit Phase 1 on a dedicated branch or commit cluster; journal entry recording migration numbers used
---
## Phase 2 - SQLC queries + code generation
**Goal**: All DB access the feature needs is expressed as SQLC-generated Go.
**Dependencies**: Phase 1.
**Exit criteria**: `task generate` clean; generated `*.sql.go` files compile; every query listed below exists and has a typed wrapper.
### 2.1 New file `internal/database/queries/customschemas.sql`
**Schema CRUD queries:**
- [ ] `CreateClientMetadataSchema` -- INSERT returning full row -- test: generated code compiles, query name appears in `*.sql.go`
- [ ] `GetClientMetadataSchema` (by id) -- test: generated
- [ ] `ListClientMetadataSchemas` (filters: client_id required, name optional, status optional with default `active`, pagination limit/offset) -- test: generated
- [ ] `GetLatestSchemaByName` -- test: generated
- [ ] `GetSchemaDocumentCount` (count of `documents.custom_schema_id = $1`) -- test: generated
- [ ] `GetSchemaMetadataRecordCount` (count of `document_custom_metadata.schema_id = $1`) -- test: generated
- [ ] `SetSchemaStatus` (update status, used for supersede and retire) -- test: generated
- [ ] `GetMaxSchemaVersion` (for auto-increment on PUT) -- test: generated
- [ ] `LockSchemaVersionsForName` (`SELECT ... FOR UPDATE` on all versions of a `(client_id, name)` pair) -- test: generated
**Metadata CRUD queries:**
- [ ] `CreateDocumentCustomMetadata` -- test: generated
- [ ] `GetCurrentDocumentCustomMetadata` (via view) -- test: generated
- [ ] `GetDocumentCustomMetadataByVersion` -- test: generated
- [ ] `GetDocumentCustomMetadataHistory` (with limit/offset) -- test: generated
- [ ] `LockDocumentCustomMetadataForVersion` (`SELECT ... FOR UPDATE` on max version) -- test: generated
**Document-binding queries:**
- [ ] `SetDocumentCustomSchemaId` -- test: generated
- [ ] `GetDocumentCustomSchemaId` -- test: generated
- [ ] `BulkSetDocumentCustomSchemaIdInFolderTree` (`WITH RECURSIVE`) -- test: generated; recursive CTE compiles
- [ ] `GetDocumentsWithMetadataInFolderTree` -- test: generated
- [ ] `GetDocumentsWithLegacyExtractionsInFolderTree` -- test: generated
- [ ] `DocumentHasLegacyExtractions` (boolean, single doc) -- test: generated
- [ ] `CountDocumentsInFolderTree` (for `MaxBulkAssignDocuments` check) -- test: generated
**Reset-metadata support queries (v3):**
- [ ] `LockDocumentForReset` (`SELECT id, "clientId", custom_schema_id FROM documents WHERE id = $1 FOR UPDATE`) -- test: generated
- [ ] `GetDocumentSchemaBindingForReset` (join `documents` -> `client_metadata_schemas`, returns `(custom_schema_id, schema_name, schema_version)` or all-null) -- test: generated
- [ ] `CountDocumentCustomMetadataVersions` -- test: generated
### 2.2 Query additions for delete cascade (Phase 5b prep)
- [ ] Add `DeleteDocumentCustomMetadata` to `document.sql` -- test: generated
- [ ] Add `NullifyDocumentCustomSchemaId` to `document.sql` -- test: generated
- [ ] Add `DeleteClientMetadataSchemas` to `client.sql` -- test: generated
### 2.3 Phase 2 integration
- [ ] `task generate` clean -- test: no diff after re-run (idempotent)
- [ ] Generated files compile: `go build ./...` -- test: clean
- [ ] Commit Phase 2 with journal entry
---
## Phase 3 - Schema validator component
**Goal**: A standalone `internal/customschema/validator.go` that can meta-validate JSON Schema definitions and validate metadata payloads against a compiled schema.
**Dependencies**: JSON Schema library added to `go.mod`.
**Exit criteria**: Validator unit tests cover all supported types in Section 6.3 and the explicit-`additionalProperties` rule.
### 3.1 Dependency
- [ ] Add `github.com/santhosh-tekuri/jsonschema/v6` to `go.mod` -- test: `go mod tidy` clean, build clean
### 3.2 Constants file
- [ ] Create `internal/customschema/constants.go` with `MaxSchemaDefinitionBytes = 65536`, `MaxMetadataPayloadBytes = 1048576`, `MaxBulkAssignDocuments = 10000` -- test: constants referenced by tests
### 3.3 Validator implementation
- [ ] Create `internal/customschema/validator.go` with `SchemaValidator` struct -- test: compiles
- [ ] Implement `ValidateSchemaDefinition(schemaDef json.RawMessage) error` -- test: valid JSON Schema draft 2020-12 accepted
- [ ] Reject schema definitions that are not valid JSON -- test: TDD test with malformed JSON
- [ ] Reject schema definitions that fail JSON Schema meta-validation -- test: TDD test with `{"type": "nonsense"}`
- [ ] Reject schema definitions where root `type` is not `"object"` -- test: TDD test with `{"type": "array"}`
- [ ] Reject schema definitions where root `additionalProperties` is not explicitly present -- test: TDD test with schema missing the key
- [ ] Accept schema definitions where root `additionalProperties` is explicitly `true` -- test: TDD test
- [ ] Accept schema definitions where root `additionalProperties` is explicitly `false` -- test: TDD test
- [ ] Reject schema definitions larger than `MaxSchemaDefinitionBytes` -- test: TDD test with 64KB+1 payload
- [ ] Implement `ValidateMetadata(schemaDef, metadata json.RawMessage) ([]ValidationError, error)` -- test: compiles
- [ ] Conforming metadata returns empty error slice -- test: Appendix A.1 sample schema + conforming payload
- [ ] Missing required field produces `ValidationError` with field pointer and message -- test: TDD
- [ ] `maxLength` violation produces structured error -- test: TDD
- [ ] `minimum`/`maximum` violation produces structured error -- test: TDD
- [ ] `pattern` violation produces structured error -- test: TDD
- [ ] `enum` violation produces structured error -- test: TDD
- [ ] `format: date` / `date-time` / `email` / `uri` / `uuid` enforced -- test: one TDD test per format
- [ ] `additionalProperties: false` rejects extra fields -- test: TDD
- [ ] Nested object validation works (e.g., `contact_info` from A.1) -- test: TDD
- [ ] Array `items` / `minItems` / `maxItems` enforced -- test: TDD
- [ ] Metadata larger than `MaxMetadataPayloadBytes` rejected -- test: TDD with 1MB+1 payload
### 3.4 Phase 3 integration
- [ ] `go test ./internal/customschema/...` green -- test: full package green
- [ ] `task test:race` green for the package -- test: no races
- [ ] Journal entry
---
## Phase 4 - Service layer
**Goal**: `internal/customschema/service.go` exposes every operation the HTTP layer will call. Role-agnostic. Real DB in tests (testcontainers, no mocks per project rules).
**Dependencies**: Phase 2 (SQLC), Phase 3 (validator).
**Exit criteria**: Service integration tests cover every method and every documented error path.
### 4.1 Skeleton and models
- [ ] Create `internal/customschema/service.go` with `Service` struct holding `cfg serviceconfig.ConfigProvider` and `validator *SchemaValidator` -- test: compiles
- [ ] Create `internal/customschema/models.go` with `Schema`, `SchemaSummary`, `CreateSchemaInput`, `UpdateSchemaInput`, `ListFilters`, `CustomMetadata`, `MetadataVersion`, `SetMetadataInput`, `BulkAssignResult`, `SkippedDocument`, `ResetMetadataResult` -- test: compiles
- [ ] Every `*Input` struct that historically carried `CreatedBy string` is renamed or documented so the service expects the actor to arrive as a **separate function argument** (e.g. `Service.CreateSchema(ctx, input, actor string)`). The service must never read `CreatedBy` off a request body model. -- test: compiles + unit test that asserts passing an input with a non-empty `CreatedBy` is ignored in favor of the `actor` argument (or, cleaner, the struct field is removed entirely)
### 4.1a Audit sink definition (new, required before 4.2 and 4.5 can log)
- [ ] Decide sink location: either `internal/customschema/audit.go` (feature-local) or `internal/audit/` (shared) -- test: decision recorded in journal
- [ ] Define `AuditRecord` struct: `Actor string`, `Action string` (enum: `schema.create|schema.update|schema.delete|schema.assign|metadata.write|metadata.reset`), `ResourceID uuid.UUID`, `ClientID uuid.UUID`, `Timestamp time.Time`, `Details map[string]any` -- test: compiles
- [ ] Define `AuditSink` interface with `Record(ctx, AuditRecord) error` -- test: compiles
- [ ] Provide a default implementation (structured `slog` with `audit=true` attribute, or a DB-backed appender if Q decides) -- test: unit test verifies a recorded event lands in the sink
- [ ] Wire the sink into `customschema.Service` via `serviceconfig.ConfigProvider` or a constructor arg -- test: compiles
### 4.2 Schema CRUD methods
- [ ] `CreateSchema(ctx, input)` -- meta-validates, enforces 64KB, enforces explicit `additionalProperties`, enforces `name` unique within client for active schemas at version 1 -- test: happy path
- [ ] `CreateSchema` rejects invalid JSON Schema -- test: TDD
- [ ] `CreateSchema` rejects oversize definition -- test: TDD
- [ ] `CreateSchema` rejects duplicate name for same client -- test: TDD
- [ ] `UpdateSchema(ctx, schemaID, input)` -- uses `LockSchemaVersionsForName` -> `GetMaxSchemaVersion` -> INSERT new version -> SetSchemaStatus(old, 'superseded') inside a single tx -- test: happy path produces v2
- [ ] `UpdateSchema` concurrency: two concurrent calls against same `(client_id, name)` produce distinct version numbers -- test: TDD with goroutines against testcontainer DB
- [ ] `UpdateSchema` rejects when old version does not exist -- test: TDD
- [ ] `UpdateSchema` rejects invalid schema definition (same validator rules as create) -- test: TDD
- [ ] `GetSchema(ctx, schemaID)` -- test: returns full row including `schema_def`
- [ ] `GetSchema` 404 when not found -- test: TDD
- [ ] `ListSchemas(ctx, clientID, filters)` -- default returns only `active` -- test: TDD
- [ ] `ListSchemas` filter by `name` -- test: TDD
- [ ] `ListSchemas` filter by `status` -- test: TDD
- [ ] `ListSchemas` `includeAllVersions=true` returns all versions -- test: TDD
- [ ] `ListSchemas` pagination (`limit`, `offset`) -- test: TDD
- [ ] `ListSchemas` populates `documentCount` and `canDelete` fields -- test: TDD
- [ ] `DeleteSchema(ctx, schemaID)` -- sets status to `retired` -- test: happy path
- [ ] `DeleteSchema` returns 409 if any document references the schema via `custom_schema_id` -- test: TDD
- [ ] `DeleteSchema` returns 409 if any `document_custom_metadata.schema_id` row references it -- test: TDD
### 4.3 Metadata methods
- [ ] `SetDocumentMetadata(ctx, input)` -- derives `schemaId` from `documents.custom_schema_id`, never trusts client -- test: happy path
- [ ] `SetDocumentMetadata` rejects when document has no `custom_schema_id` -- test: TDD (400)
- [ ] `SetDocumentMetadata` rejects when document has legacy extractions -- test: TDD (409)
- [ ] `SetDocumentMetadata` validates metadata against schema via `validator.ValidateMetadata` -- test: TDD with conforming and non-conforming payloads
- [ ] `SetDocumentMetadata` enforces `MaxMetadataPayloadBytes` -- test: TDD
- [ ] `SetDocumentMetadata` serializes version assignment by first taking `SELECT id FROM documents WHERE id = $1 FOR UPDATE` on the parent row, then reading `GetMaxDocumentMetadataVersion`, then INSERTing `version = max+1` -- test: TDD confirms the parent-row lock is acquired before the version read
- [ ] `SetDocumentMetadata` concurrency: two concurrent writers against the **same** document that already has >=1 metadata row -- both succeed, produce distinct versions, no unique-violation -- test: TDD with goroutines against testcontainer DB
- [ ] `SetDocumentMetadata` concurrency: two concurrent **first** writers against a document with zero metadata rows -- both succeed, produce `version=1` and `version=2` in some order, no unique-violation surfaces to the caller -- test: TDD with goroutines, proves the parent-row lock (not just `FOR UPDATE` on the non-existent max-version row) is what serializes the first write
- [ ] `SetDocumentMetadata` retries once on `UNIQUE (document_id, version)` violation as a belt-and-suspenders fallback (should be unreachable once the parent lock is in place) -- test: TDD with an injected unique-violation on the first INSERT attempt
- [ ] `SetDocumentMetadata` subsequent writes must use same schema (consistency with Trigger 3) -- test: TDD
- [ ] `GetCurrentMetadata(ctx, documentID)` -- test: uses view, returns latest
- [ ] `GetMetadataByVersion(ctx, documentID, version)` -- test: TDD
- [ ] `GetMetadataHistory(ctx, documentID)` with pagination -- test: TDD
### 4.4 Document schema binding methods
- [ ] `AssignSchema(ctx, documentID, schemaID)` -- test: happy path single-document assignment
- [ ] `AssignSchema` rejects if schema and document belong to different clients (app-layer + trigger 2) -- test: TDD
- [ ] `AssignSchema` rejects if schema status is not `active` -- test: TDD
- [ ] `AssignSchema` rejects if document already has custom metadata (matches Trigger 1 behavior; expect 409) -- test: TDD
- [ ] `AssignSchema` rejects if document has legacy extractions -- test: TDD
- [ ] `AssignSchema` allows setting `customSchemaId=nil` to clear binding when no metadata exists -- test: TDD
- [ ] `AssignSchemaToFolder(ctx, folderID, schemaID, createdBy)` -- test: walks subtree with `WITH RECURSIVE` and returns `BulkAssignResult`
- [ ] Bulk assignment skips documents with different schema + existing metadata with `SkippedDocument` reason -- test: TDD
- [ ] Bulk assignment skips documents with legacy extractions with reason -- test: TDD
- [ ] Bulk assignment no-ops documents already on target schema with reason -- test: TDD
- [ ] Bulk assignment reassigns documents with different schema but no metadata -- test: TDD
- [ ] Bulk assignment returns 422 when subtree exceeds `MaxBulkAssignDocuments` -- test: TDD with 10001 docs (or a smaller constant override for tests)
- [ ] Bulk assignment runs in single transaction -- test: verify partial-failure rolls back everything
### 4.5 Reset-metadata method (v3)
- [ ] `ResetDocumentMetadata(ctx, documentID, resetBy)` -- single transaction, `LockDocumentForReset`, `GetDocumentSchemaBindingForReset`, `CountDocumentCustomMetadataVersions`, `DeleteDocumentCustomMetadata`, `NullifyDocumentCustomSchemaId`, audit log -- test: happy path wipe with schema bound + N metadata versions
- [ ] Reset returns populated `previous*` fields when document had a schema -- test: TDD
- [ ] Reset returns `metadataVersionsDeleted` matching actual deleted count -- test: TDD
- [ ] Reset is idempotent: already-clean document returns `metadataVersionsDeleted: 0`, `previousSchemaId: nil` -- test: TDD
- [ ] Reset rejects documents with legacy field extractions (409) -- test: TDD
- [ ] Reset returns 404 on non-existent document -- test: TDD
- [ ] Reset atomicity: inject failure between DELETE and UPDATE -- test: transaction rolls back, metadata rows still present
- [ ] Reset + reassign + write: after reset, Trigger 1 still fires on second schema change under new metadata -- test: TDD regression
- [ ] Reset + reassign + write: after reset, Trigger 3 still fires on mismatched-schema direct INSERT -- test: TDD regression
- [ ] Reset writes audit log entry containing actor, documentID, previousSchemaId, metadataVersionsDeleted -- test: TDD reads the audit sink
- [ ] Reset concurrency: two concurrent resets on same document serialize via `FOR UPDATE` -- test: TDD
- [ ] Reset concurrency: reset vs `AssignSchema` on same document serialize -- test: TDD
### 4.6 Phase 4 integration
- [ ] `go test ./internal/customschema/...` green -- test: full package green
- [ ] `task test:race` green for package -- test: no races
- [ ] `task test:unit:short` still green globally -- test: no cross-package regressions
- [ ] Journal entry
---
## Phase 5a - Legacy field extraction guard
**Goal**: The existing `FieldExtractionService` refuses to write to any document that has `custom_schema_id IS NOT NULL`, and the HTTP controller surfaces that refusal as `409 Conflict`.
**Dependencies**: Phase 2.
**Exit criteria**: Existing field extraction tests still pass; new service-level test proves the guard rejects; new controller-level test proves the HTTP response is `409` (not `500`).
### 5a.1 Service-layer guard
- [ ] Locate the existing `FieldExtractionService.CreateFieldExtraction` entry point (currently `internal/fieldextraction/service.go`) -- test: `grep` and read code
- [ ] Add a pre-check: read `documents.custom_schema_id` for target document -- test: happy path unchanged (null binding)
- [ ] If `custom_schema_id IS NOT NULL`, return a typed sentinel error (e.g. `ErrMutualExclusivityViolation`) with message referencing mutual exclusivity -- test: TDD service test asserts `errors.Is(err, ErrMutualExclusivityViolation)`
- [ ] Existing field extraction happy-path tests still green -- test: `go test ./internal/fieldextraction/...`
### 5a.2 Controller-layer 409 mapping (REQUIRED to deliver the documented response)
**Why this sub-phase exists**: today `api/queryAPI/fieldextractions.go:CreateFieldExtraction` maps every service error to `http.StatusInternalServerError`. Adding only the service pre-check is not enough -- without controller error mapping the new guard still surfaces as a `500`. Phase 5a is not complete until the HTTP layer returns `409`.
- [ ] Update `CreateFieldExtraction` handler in `api/queryAPI/fieldextractions.go` to branch on the service sentinel error and return `echo.NewHTTPError(http.StatusConflict, ...)` with the mutual-exclusivity message -- test: handler unit test asserts status 409 and body message
- [ ] Audit the other `FieldExtractionService` handler entry points in `api/queryAPI/fieldextractions.go` (Update, AppendValueTo*, etc.) and apply the same 409 mapping where those methods also run the guard -- test: TDD handler test per entry point
- [ ] Preserve the existing mapping of non-sentinel service errors to `500` -- test: TDD handler test with a generic error path
- [ ] End-to-end HTTP test: create document, assign custom schema, call `POST /field-extractions`, assert response status is `409` and JSON body carries the documented message -- test: integration against testcontainer DB
- [ ] Journal entry
---
## Phase 5b - Delete cascade updates
**Goal**: Hard-deleting a document or client works even when custom metadata and schemas are present.
**Dependencies**: Phase 2.
**Exit criteria**: Document and client delete cascades succeed with custom metadata present; no orphaned rows.
### 5b.1 Document delete cascade
- [ ] Locate `DeleteDocumentCascade` implementation -- test: found
- [ ] Insert step: `DeleteDocumentCustomMetadata` between legacy extractions delete and document row delete -- test: TDD
- [ ] Insert step: `NullifyDocumentCustomSchemaId` before document row delete -- test: TDD
- [ ] Document delete succeeds for a document with N metadata versions and a schema bound -- test: TDD integration
- [ ] Document delete succeeds for a document with no custom metadata (regression) -- test: TDD
- [ ] Post-delete: `SELECT COUNT(*) FROM document_custom_metadata WHERE document_id = deleted_id` returns 0 -- test: TDD
- [ ] Post-delete: `client_metadata_schemas` rows referenced by the document are NOT deleted (only the binding is) -- test: TDD
### 5b.2 Client delete cascade
- [ ] Locate `client.HardDelete` / `deleteClientDependencies` -- test: found
- [ ] Insert step: `DeleteClientMetadataSchemas` after all documents are deleted, before client row delete -- test: TDD
- [ ] Client hard-delete succeeds when client has schemas and documents with custom metadata -- test: TDD integration
- [ ] Post-delete: `SELECT COUNT(*) FROM client_metadata_schemas WHERE client_id = deleted_id` returns 0 -- test: TDD
- [ ] Post-delete: no orphaned `document_custom_metadata` rows -- test: TDD
- [ ] Regression: client delete still works for clients with no custom metadata -- test: TDD
- [ ] Journal entry
---
## Phase 6 - OpenAPI spec updates
**Goal**: `serviceAPIs/queryAPI.yaml` reflects all new endpoints and schemas; code generation produces clean client/server stubs.
**Dependencies**: None for spec authoring; must land before Phase 8 codegen.
**Exit criteria**: `task generate` produces updated `api/queryAPI/*` and `pkg/queryAPI/*` without errors.
### 6.1 New tags
- [ ] Add `SuperAdminSchemaService` tag with description explicitly stating the `super_admin`-only boundary -- test: spec lints
- [ ] Add `CustomMetadataService` tag with description stating `client_user` + `user_admin` + `super_admin` access -- test: spec lints
### 6.2 Schemas
- [ ] `CustomSchemaRequest` -- test: codegen produces Go type
- [ ] `CustomSchemaResponse` -- test: codegen
- [ ] `CustomSchemaListResponse` -- test: codegen
- [ ] `CustomMetadataRequest` -- test: codegen
- [ ] `CustomMetadataResponse` -- test: codegen
- [ ] `CustomMetadataHistoryResponse` -- test: codegen
- [ ] `ValidationErrorResponse` -- test: codegen
- [ ] `BulkSchemaAssignRequest` -- test: codegen
- [ ] `BulkSchemaAssignResponse` -- test: codegen
- [ ] `DocumentSchemaAssignRequest` -- test: codegen
- [ ] `DocumentSchemaAssignResponse` -- test: codegen
- [ ] `ResetDocumentMetadataResponse` (v3) -- test: codegen
- [ ] `SchemaStatus` enum (`active`/`superseded`/`retired`) -- test: codegen
- [ ] Modify `DocumentEnriched` to add `customSchemaId`, `customSchemaName`, `hasCustomMetadata`, `customMetadata` -- test: codegen
### 6.3 Paths
- [ ] `POST /super-admin/custom-schemas` -- test: codegen produces handler interface
- [ ] `GET /super-admin/custom-schemas` (with query params) -- test: codegen
- [ ] `GET /super-admin/custom-schemas/{schemaId}` -- test: codegen
- [ ] `PUT /super-admin/custom-schemas/{schemaId}` -- test: codegen
- [ ] `DELETE /super-admin/custom-schemas/{schemaId}` -- test: codegen
- [ ] `PATCH /super-admin/documents/{id}/schema` -- test: codegen
- [ ] `POST /super-admin/folders/{folderId}/assign-schema` -- test: codegen
- [ ] `POST /super-admin/documents/{id}/reset-metadata` (v3) -- test: codegen
- [ ] `GET /custom-metadata` -- test: codegen
- [ ] `POST /custom-metadata` -- test: codegen
- [ ] `GET /custom-metadata/version` -- test: codegen
- [ ] `GET /custom-metadata/history` -- test: codegen
- [ ] Modify `GET /document/{id}` to document new fields and the `customMetadata` query parameter -- test: codegen
### 6.4 Phase 6 integration
- [ ] `task generate` clean -- test: no errors
- [ ] Generated controllers compile -- test: `go build ./...`
- [ ] Journal entry
---
## Phase 7a - Permit.io policy file updates
**Goal**: `permit_policies.yaml` declares the new `super-admin` and `custom-metadata` resources and grants the correct role permissions.
**Dependencies**: None.
**Exit criteria**: YAML validates; setup tool dry-run shows the expected resource/role diff.
> **Accepted cross-tenant limitation**: the authorization check performed by the middleware is role/action/resource-type only -- it does not pass a document or client object to Permit.io. Any caller with `custom-metadata:*` grants can therefore read or write *any* document's custom metadata if they know the UUID. This is unchanged from the rest of the API and is **explicitly accepted for v3** because production is deployed per-client (one isolated stack per client, separate DB). The limitation only matters in shared non-prod environments. See Section 5.3 of the plan for the full rationale. **Do not add application-level tenant checks to the custom-metadata service in v3** -- that is a separate, API-wide initiative.
- [ ] Add `super-admin` resource with `get`, `post`, `patch`, `delete` actions -- test: YAML valid
- [ ] Add `custom-metadata` resource with `get`, `post` actions -- test: YAML valid
- [ ] Grant `super_admin` role: `super-admin: [get, post, patch, delete]`, `custom-metadata: [get, post]` -- test: YAML valid
- [ ] Confirm `user_admin` role NOT granted `super-admin` -- test: visual diff against v1 intent; test-assertion in Phase 11
- [ ] Grant `user_admin` role: `custom-metadata: [get, post]` -- test: YAML valid
- [ ] Grant `auditor` role: `super-admin: [get]`, `custom-metadata: [get]` -- test: YAML valid
- [ ] Grant `client_user` role: `custom-metadata: [get, post]` -- test: YAML valid
- [ ] Add documentation-only `policy_mappings` entries for every `/super-admin/*` path including `POST /super-admin/documents/{id}/reset-metadata` -- test: YAML valid
- [ ] Add documentation-only `policy_mappings` entries for every `/custom-metadata/*` path -- test: YAML valid
- [ ] Resolve PUT-to-patch question per Section 5.3 footnote: either confirm middleware maps PUT -> patch OR add `put` action to `super-admin` resource and grant it. Record decision in the journal. -- test: decision logged
- [ ] Journal entry
---
## Phase 7b - Run Permit.io setup tool (dev / uat / prod)
**Goal**: New resources and grants provisioned in every environment BEFORE Phase 8 code is deployed to that environment.
**Dependencies**: Phase 7a.
**Exit criteria**: Setup tool run succeeds in each env; spot-check via permit.io admin UI or `list_roles.sh` matches YAML.
- [ ] Run `cmd/auth_related/permit.setup/run.tool.dev.sh` -- test: exit code 0
- [ ] Verify `super-admin` resource exists in dev Permit.io tenant -- test: list
- [ ] Verify `custom-metadata` resource exists in dev -- test: list
- [ ] Verify `super_admin` role has expected grants in dev -- test: role inspect
- [ ] Verify `user_admin` role does NOT have `super-admin` grants in dev -- test: role inspect
- [ ] Run `run.tool.uat.sh` after dev verified -- test: exit 0
- [ ] Same verification pass on uat -- test: list + role inspect
- [ ] Run `run.tool.prod.sh` after uat verified AND only with Q's explicit go-ahead -- test: exit 0, Q confirmation logged in journal
- [ ] Same verification pass on prod -- test: list + role inspect
- [ ] Journal entry with timestamps of each env run
---
## Phase 8 - API controllers + codegen
**Goal**: HTTP handlers thin-wrap the service layer; every generated interface method is implemented.
**Dependencies**: Phase 4, Phase 6.
**Exit criteria**: Every new endpoint compiles and routes resolve; handler unit tests green.
### 8.1 Schema CRUD controller (`api/queryAPI/customschemas.go`)
- [ ] File skeleton created with handler receivers wired to `customschema.Service` -- test: compiles
- [ ] **Actor extraction helper**: every handler in this file derives actor identity via `cognitoauth.GetUserSubject(c)` (or the existing middleware helper) and passes it to the service as a separate argument; any `createdBy` on the request body is ignored -- test: handler unit test sets a JWT subject via middleware fixture, posts a body with a different `createdBy`, asserts the service receives the JWT subject
- [ ] `POST /super-admin/custom-schemas` handler -- parse, validate, derive actor from JWT, call `CreateSchema`, map errors to 400/409/500 -- test: TDD handler test
- [ ] `GET /super-admin/custom-schemas` handler with query params -- test: TDD
- [ ] `GET /super-admin/custom-schemas/{schemaId}` handler -- test: TDD
- [ ] `PUT /super-admin/custom-schemas/{schemaId}` handler -- derives actor from JWT -- test: TDD
- [ ] `DELETE /super-admin/custom-schemas/{schemaId}` handler -- derives actor from JWT -- test: TDD including 409 path
- [ ] `PATCH /super-admin/documents/{id}/schema` handler -- derives actor from JWT -- test: TDD
- [ ] `POST /super-admin/folders/{folderId}/assign-schema` handler -- derives actor from JWT -- test: TDD (covered more deeply in Phase 10)
- [ ] `POST /super-admin/documents/{id}/reset-metadata` handler (v3) -- derives actor from JWT -- test: TDD (covered more deeply in Phase 10b)
### 8.2 User-facing custom metadata controller (`api/queryAPI/custommetadata.go`)
- [ ] File skeleton -- test: compiles
- [ ] **Actor extraction**: `POST /custom-metadata` derives actor identity from the JWT and ignores any body-supplied `createdBy` -- test: TDD handler test asserts body `createdBy` does not override
- [ ] `POST /custom-metadata` handler -- test: TDD
- [ ] `GET /custom-metadata` handler -- test: TDD
- [ ] `GET /custom-metadata/version` handler -- test: TDD
- [ ] `GET /custom-metadata/history` handler -- test: TDD
### 8.3 Phase 8 integration
- [ ] All new handler files compile -- test: `go build ./...`
- [ ] Handler unit tests green -- test: `go test ./api/queryAPI/...`
- [ ] Journal entry
---
## Phase 9 - Document endpoint modifications
**Goal**: `GET /document/{id}` surfaces custom schema and metadata when requested.
**Dependencies**: Phase 4, Phase 8.
**Exit criteria**: Existing tests unchanged; new fields populated correctly for documents with and without custom schemas.
- [ ] Extend `DocumentEnriched` builder to include `customSchemaId`, `customSchemaName`, `hasCustomMetadata` unconditionally -- test: TDD
- [ ] Extend builder to include `customMetadata` when `?customMetadata=true` -- test: TDD
- [ ] Authorization: confirm `GET /document/{id}` still uses the existing `document` resource, not `super-admin` -- test: role matrix test in Phase 11
- [ ] Document without any custom schema returns `customSchemaId: null`, `hasCustomMetadata: false` -- test: TDD
- [ ] Document with schema bound but no metadata returns `customSchemaId: <id>`, `hasCustomMetadata: false` -- test: TDD
- [ ] Document with schema + metadata returns populated fields -- test: TDD
- [ ] `PATCH /super-admin/documents/{id}/schema` handler delegates to `AssignSchema` -- test: TDD
- [ ] Assign-schema 409 paths surface correct error messages -- test: TDD
- [ ] Journal entry
---
## Phase 10 - Bulk folder schema assignment endpoint
**Goal**: `POST /super-admin/folders/{folderId}/assign-schema` behaves per Section 5.2 including all skip reasons and the 10K limit.
**Dependencies**: Phase 4 (`AssignSchemaToFolder`), Phase 8.
**Exit criteria**: Recursive walk correct, skip reasons complete, document count limit enforced.
- [ ] Handler wired to `AssignSchemaToFolder` -- test: compiles
- [ ] Recursive walk on a 3-deep folder tree produces correct `documentsUpdated` count -- test: TDD
- [ ] Skip reason: "different schema + existing metadata" -- test: TDD
- [ ] Skip reason: "already assigned to same schema (no-op)" -- test: TDD
- [ ] Skip reason: "has legacy field extractions" -- test: TDD
- [ ] Reassignment on document with different schema but no metadata -- test: TDD
- [ ] Subtree > 10K documents returns 422 with the documented message -- test: TDD (constant override for test)
- [ ] Empty folder returns 200 with 0 counts -- test: TDD
- [ ] Deeply nested (10+ levels) tree walks correctly -- test: TDD
- [ ] Single transaction: simulated mid-walk failure rolls back -- test: TDD
- [ ] Journal entry
---
## Phase 10b - Reset-metadata endpoint (v3)
**Goal**: `POST /super-admin/documents/{id}/reset-metadata` exposes `Service.ResetDocumentMetadata` via HTTP with correct auth, response shape, and error mapping. (Deep service-level tests already live in Phase 4.5; this phase validates the HTTP wiring.)
**Dependencies**: Phase 4.5 (service method), Phase 5b (queries reused), Phase 8 (controller skeleton).
**Exit criteria**: Handler + integration tests green; authorization matrix for the endpoint verified in Phase 11.
- [ ] Handler parses document ID path parameter -- test: TDD
- [ ] Handler extracts caller identity for `resetBy` field -- test: TDD
- [ ] Handler calls `Service.ResetDocumentMetadata` -- test: TDD
- [ ] Handler maps success to 200 with `ResetDocumentMetadataResponse` -- test: TDD
- [ ] Handler maps "document not found" to 404 -- test: TDD
- [ ] Handler maps "legacy extractions exist" to 409 with mutual-exclusivity message -- test: TDD
- [ ] Handler maps unexpected DB errors to 500 -- test: TDD
- [ ] End-to-end HTTP test: bind schema v1, POST metadata, PUT schema -> v2 created, reset-metadata, PATCH schema to v2, POST new metadata with added optional field -- test: integration test exercises the full upgrade flow
- [ ] End-to-end HTTP test: reset on already-clean document returns 200 with `metadataVersionsDeleted: 0` -- test: integration
- [ ] Journal entry
---
## Phase 11 - Integration and authorization tests
**Goal**: Every role's access to every new endpoint is verified. Every cross-cutting invariant has a test. `task fullsuite:ci` ends with `All coverage checks passed!`.
**Dependencies**: Phase 10b and Phase 7b in each environment used for testing.
**Exit criteria**: 80% minimum coverage across new packages; authz matrix 100% covered; no `FAIL` in `fullsuite:ci` output.
### 11.1 Role authorization matrix (every row = one test)
- [ ] `super_admin` CAN `POST /super-admin/custom-schemas` -- test: integration
- [ ] `super_admin` CAN `GET /super-admin/custom-schemas` -- test: integration
- [ ] `super_admin` CAN `GET /super-admin/custom-schemas/{id}` -- test: integration
- [ ] `super_admin` CAN `PUT /super-admin/custom-schemas/{id}` -- test: integration
- [ ] `super_admin` CAN `DELETE /super-admin/custom-schemas/{id}` -- test: integration
- [ ] `super_admin` CAN `PATCH /super-admin/documents/{id}/schema` -- test: integration
- [ ] `super_admin` CAN `POST /super-admin/folders/{folderId}/assign-schema` -- test: integration
- [ ] `super_admin` CAN `POST /super-admin/documents/{id}/reset-metadata` -- test: integration
- [ ] `user_admin` gets 403 on every `/super-admin/*` path+method combination (8 tests) -- test: integration, one per route
- [ ] `user_admin` CAN read + write `/custom-metadata` (no regression) -- test: integration
- [ ] `client_user` gets 403 on every `/super-admin/*` path+method combination (8 tests) -- test: integration
- [ ] `client_user` CAN read + write `/custom-metadata` -- test: integration
- [ ] `auditor` CAN `GET` on `/super-admin/custom-schemas` and `/super-admin/custom-schemas/{id}` -- test: integration
- [ ] `auditor` gets 403 on every `/super-admin/*` write path (POST/PUT/PATCH/DELETE) -- test: integration
- [ ] `auditor` CAN `GET /custom-metadata`, CANNOT `POST /custom-metadata` -- test: integration
- [ ] Every 403 test asserts DB state is unchanged on denial -- test: embedded in each
### 11.2 Mutual exclusivity
- [ ] Legacy write blocked when document has custom schema -- test: integration
- [ ] Schema assignment blocked when document has legacy extractions -- test: integration
- [ ] Bulk folder assign skips legacy-extracting documents -- test: integration
- [ ] Reset-metadata rejected on legacy-extracting document -- test: integration
### 11.3 Schema lifecycle
- [ ] `active` -> `superseded` on PUT create new version -- test: integration
- [ ] `active` -> `retired` on DELETE -- test: integration
- [ ] Assignment rejected for non-`active` status -- test: integration
- [ ] Schema version concurrency (concurrent PUTs) produces distinct versions -- test: integration
### 11.4 Delete cascade
- [ ] Document hard-delete with custom metadata -- test: integration
- [ ] Client hard-delete with schemas and metadata -- test: integration
- [ ] No orphaned rows post-delete -- test: integration
### 11.5 Size and count limits
- [ ] Metadata payload at 1MB boundary: accept 1048576, reject 1048577 -- test: integration
- [ ] Schema definition at 64KB boundary: accept 65536, reject 65537 -- test: integration
- [ ] Bulk folder > 10000 docs: 422 -- test: integration
- [ ] Bulk folder exactly 10000 docs: succeeds -- test: integration
### 11.6 Trigger regression (defense-in-depth)
- [ ] Direct SQL bypass of service layer: attempt UPDATE `custom_schema_id` on a document with metadata -- trigger fires
- [ ] Direct SQL bypass: INSERT `document_custom_metadata` with mismatched `schema_id` -- trigger fires
- [ ] Direct SQL bypass: assign cross-client schema -- trigger fires
- [ ] After reset + reassign + new metadata, all three triggers still fire on repeat attempts -- test: integration
### 11.7 Backward compatibility
- [ ] Existing `/field-extractions` endpoints unchanged for documents with no custom schema -- test: integration
- [ ] Existing `/admin/*` endpoints unaffected by `super-admin` resource -- test: integration
- [ ] `DocumentEnriched` for documents without custom schemas has null new fields, no regression on existing fields -- test: integration
### 11.8 Permit.io setup tool verification
- [ ] Run setup tool against a disposable dev tenant and assert both resources exist with expected actions -- test: tool output + API inspection
- [ ] Assert each role's grants match Section 5.3 exactly (including the negative case for `user_admin`) -- test: tool output
### 11.9 Coverage & full suite
- [ ] `internal/customschema/` >= 80% coverage -- test: `go test -cover`
- [ ] `api/queryAPI/customschemas.go` + `custommetadata.go` >= 80% coverage -- test: `go test -cover`
- [ ] `task test:race` green -- test: no race reports
- [ ] `task test:perf` checked for regressions in existing hot paths -- test: review output
- [ ] `task fullsuite:ci` ends with `All coverage checks passed!` and NO `FAIL` lines -- test: full run
- [ ] Journal entry with test counts, coverage numbers, and any flaky test notes
---
## Phase 12 - Documentation updates
**Goal**: Every doc that references endpoints, env vars, authorization, or the delete cascade is updated.
**Dependencies**: Phase 11.
**Exit criteria**: `docs/ai.generated/` regenerated; manual review of key runbooks complete.
- [ ] Update `docs/ai.generated/README.md` index if new doc files added -- test: review
- [ ] Authorization matrix table published in a dedicated doc page -- test: review
- [ ] Sample schemas from Appendix A published as copyable examples -- test: review
- [ ] `CustomMetadataService` usage guide (end-user perspective) -- test: review
- [ ] `SuperAdminSchemaService` usage guide (platform-operator perspective) -- test: review
- [ ] Reset-metadata runbook: "How to upgrade a document to a newer schema version" with the full wipe -> reassign -> re-POST flow and a warning about metadata history loss -- test: review
- [ ] Mutual exclusivity explanation (legacy vs custom) in the document processing guide -- test: review
- [ ] Delete cascade documentation updated -- test: review
- [ ] Permit.io deployment ordering note (Phase 7b before Phase 8) added to the deployment runbook -- test: review
- [ ] Any new env vars? (none expected for v3; confirm.) -- test: review
- [ ] Generate new API status report / architecture diagrams if the pipeline regenerates them -- test: review
- [ ] Journal entry marking the feature shipped
---
## Cross-phase reminders
- Before starting any phase, re-read the relevant section of `plans/mutable.metadata.plan.combo.v3.md`.
- TDD is mandatory per project rules: write the test, watch it fail, make it pass. Never write multiple tests before running any.
- No mocks without Q's explicit permission. Use testcontainers for real Postgres.
- Keep the journal current. When a checkbox flips to `[x]`, add a timestamped line to `./journals/implement_mutableMetadata.md` with the commit SHA and test command that proved it.
- If any trigger, migration, or service invariant seems to conflict with the plan, STOP and ask Q before modifying it. The defense-in-depth design depends on the triggers being exactly as written.
- `task fullsuite:ci` must be green at the end of every phase, not just the end of the project. Do not stack broken phases.
plans/mutable.metadata.plan.combo.v4.md
-2008
View File
File diff suppressed because it is too large Load Diff
plans/mutable.metadata.plan.combo.v4.review.claude-response.md
-140
View File
@@ -1,140 +0,0 @@
# 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.
plans/mutable.metadata.plan.combo.v4.review.coded.old.md
-55
View File
@@ -1,55 +0,0 @@
# 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](/Users/jaybrown/dev/clients/aarete/query-orchestration.2/plans/mutable.metadata.plan.combo.v4.tracking.md:85), [plans/mutable.metadata.plan.combo.v4.tracking.md:87](/Users/jaybrown/dev/clients/aarete/query-orchestration.2/plans/mutable.metadata.plan.combo.v4.tracking.md:87), [plans/mutable.metadata.plan.combo.v4.tracking.md:221](/Users/jaybrown/dev/clients/aarete/query-orchestration.2/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](/Users/jaybrown/dev/clients/aarete/query-orchestration.2/plans/mutable.metadata.plan.combo.v4.md:1384), [plans/mutable.metadata.plan.combo.v4.md:1404](/Users/jaybrown/dev/clients/aarete/query-orchestration.2/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](/Users/jaybrown/dev/clients/aarete/query-orchestration.2/internal/database/migrations.go:64), [internal/database/migrations.go:80](/Users/jaybrown/dev/clients/aarete/query-orchestration.2/internal/database/migrations.go:80)) and the local generation path does the same with `migrate ... up` ([scripts/database.yml:18](/Users/jaybrown/dev/clients/aarete/query-orchestration.2/scripts/database.yml:18), [scripts/database.yml:24](/Users/jaybrown/dev/clients/aarete/query-orchestration.2/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](/Users/jaybrown/dev/clients/aarete/query-orchestration.2/plans/mutable.metadata.plan.combo.v4.md:1341), [plans/mutable.metadata.plan.combo.v4.md:1343](/Users/jaybrown/dev/clients/aarete/query-orchestration.2/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](/Users/jaybrown/dev/clients/aarete/query-orchestration.2/plans/mutable.metadata.plan.combo.v4.tracking.md:286), [plans/mutable.metadata.plan.combo.v4.tracking.md:288](/Users/jaybrown/dev/clients/aarete/query-orchestration.2/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](/Users/jaybrown/dev/clients/aarete/query-orchestration.2/internal/fieldextraction/service.go:50), [internal/fieldextraction/service.go:65](/Users/jaybrown/dev/clients/aarete/query-orchestration.2/internal/fieldextraction/service.go:65), [internal/fieldextraction/service.go:72](/Users/jaybrown/dev/clients/aarete/query-orchestration.2/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](/Users/jaybrown/dev/clients/aarete/query-orchestration.2/plans/mutable.metadata.plan.combo.v4.md:257), [plans/mutable.metadata.plan.combo.v4.md:267](/Users/jaybrown/dev/clients/aarete/query-orchestration.2/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](/Users/jaybrown/dev/clients/aarete/query-orchestration.2/plans/mutable.metadata.requirments.md:10), [plans/mutable.metadata.requirments.md:16](/Users/jaybrown/dev/clients/aarete/query-orchestration.2/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](/Users/jaybrown/dev/clients/aarete/query-orchestration.2/plans/mutable.metadata.plan.combo.v4.md:35), [plans/mutable.metadata.plan.combo.v4.md:691](/Users/jaybrown/dev/clients/aarete/query-orchestration.2/plans/mutable.metadata.plan.combo.v4.md:691), [plans/mutable.metadata.plan.combo.v4.md:696](/Users/jaybrown/dev/clients/aarete/query-orchestration.2/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](/Users/jaybrown/dev/clients/aarete/query-orchestration.2/plans/mutable.metadata.plan.combo.v4.tracking.md:170), [plans/mutable.metadata.plan.combo.v4.tracking.md:204](/Users/jaybrown/dev/clients/aarete/query-orchestration.2/plans/mutable.metadata.plan.combo.v4.tracking.md:204), [plans/mutable.metadata.plan.combo.v4.tracking.md:335](/Users/jaybrown/dev/clients/aarete/query-orchestration.2/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](/Users/jaybrown/dev/clients/aarete/query-orchestration.2/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](/Users/jaybrown/dev/clients/aarete/query-orchestration.2/plans/mutable.metadata.plan.combo.v4.md:585), [plans/mutable.metadata.plan.combo.v4.md:587](/Users/jaybrown/dev/clients/aarete/query-orchestration.2/plans/mutable.metadata.plan.combo.v4.md:587), [plans/mutable.metadata.plan.combo.v4.md:627](/Users/jaybrown/dev/clients/aarete/query-orchestration.2/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](/Users/jaybrown/dev/clients/aarete/query-orchestration.2/plans/mutable.metadata.plan.combo.v4.tracking.md:347), [plans/mutable.metadata.plan.combo.v4.tracking.md:393](/Users/jaybrown/dev/clients/aarete/query-orchestration.2/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](/Users/jaybrown/dev/clients/aarete/query-orchestration.2/plans/mutable.metadata.plan.combo.v4.md:681), [plans/mutable.metadata.plan.combo.v4.md:685](/Users/jaybrown/dev/clients/aarete/query-orchestration.2/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](/Users/jaybrown/dev/clients/aarete/query-orchestration.2/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](/Users/jaybrown/dev/clients/aarete/query-orchestration.2/plans/mutable.metadata.plan.combo.v4.md:13), [plans/mutable.metadata.plan.combo.v4.md:14](/Users/jaybrown/dev/clients/aarete/query-orchestration.2/plans/mutable.metadata.plan.combo.v4.md:14), [plans/mutable.metadata.plan.combo.v4.md:1636](/Users/jaybrown/dev/clients/aarete/query-orchestration.2/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](/Users/jaybrown/dev/clients/aarete/query-orchestration.2/plans/mutable.metadata.plan.combo.v4.md:1523), [plans/mutable.metadata.plan.combo.v4.md:1529](/Users/jaybrown/dev/clients/aarete/query-orchestration.2/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](/Users/jaybrown/dev/clients/aarete/query-orchestration.2/plans/mutable.metadata.plan.combo.v4.md:961)), testing still says "Concurrent PUT requests" ([plans/mutable.metadata.plan.combo.v4.md:1493](/Users/jaybrown/dev/clients/aarete/query-orchestration.2/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](/Users/jaybrown/dev/clients/aarete/query-orchestration.2/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.
plans/mutable.metadata.plan.combo.v4.review.codex.md
-62
View File
@@ -1,62 +0,0 @@
# Mutable Metadata v4 Review (Codex)
Overall assessment: v4 is meaningfully simpler than the earlier versions, but it is not ready to implement as written. The biggest problems are one real security hole that the plan explicitly accepts, one schema-versioning bug that can produce invalid lineage state, and several plan/tracking inconsistencies that will cause implementation drift.
## Findings
### 1. Critical: per-client isolation is still not actually enforced for `/custom-metadata/*`
Refs: requirements [5-19](./mutable.metadata.requirments.md), plan 35, 711-718, 721-726, tracking 165-166.
Why this matters: the plan explicitly accepts that anyone with `custom-metadata:get` or `custom-metadata:post` can read or write any document's custom metadata if they know the UUID. That is a direct mismatch with the feature's per-client framing in the requirements, and it hard-codes "we deploy one stack per client" as the only security boundary. It also makes the plan internally inconsistent: the reset section says requests should 404 when the caller's client scope does not include the document, but no such client-scope check is actually designed.
Current-code cross-check: this is consistent with the current first-segment-only Permit mapping in [internal/cognitoauth/mapping.go](/Users/jaybrown/dev/clients/aarete/query-orchestration.2/internal/cognitoauth/mapping.go:3) and with the existing "no tenant isolation" note for `client_user` in [cmd/auth_related/permit.setup/permit_policies.yaml](/Users/jaybrown/dev/clients/aarete/query-orchestration.2/cmd/auth_related/permit.setup/permit_policies.yaml:181).
Recommendation: either add document/client ownership enforcement now, or keep the new metadata read/write endpoints behind `super_admin` until object-scoped auth exists. Do not ship a new per-client feature while explicitly depending on deployment topology for isolation.
### 2. High: schema version creation can leave multiple active versions in one lineage
Refs: plan 407-449, 1086, 1399, tracking 131-135, 200, 410.
Why this matters: `POST /super-admin/custom-schemas/{schemaId}/versions` is keyed by an arbitrary parent schema ID, and the plan only supersedes that parent. If v2 is active and someone creates v3 from v1, the flow inserts v3 and marks v1 `superseded` again, but v2 remains `active`. The same bug lets a caller create a new version from a `retired` schema and silently rewrite its lifecycle to `superseded`.
That breaks the plan's own "current active version" model and makes default list behavior, assignment behavior, and admin UX ambiguous.
Recommendation: require the parent to be the latest active version, or independently supersede the currently active version for the `(client_id, name)` lineage inside the same transaction. Add explicit negative tests for creating from `superseded` and `retired` parents.
### 4. Medium: several API response contracts are not backed by the planned service/query layer
Refs: plan 558-566 vs. 1081 and tracking 280/334; plan 625-640 vs. 1091-1100 and tracking 381; plan 930-942 vs. tracking 249-260.
Why this matters: the plan promises response fields that the service/query design never schedules retrieval for.
Examples:
- `PATCH /super-admin/documents/{id}/schema` returns `schemaName` and `schemaVersion`, but `AssignSchema(...)` returns only `error`, and the tracking doc does not add a follow-up lookup.
- The reset response example includes `customSchemaId` and `hasCustomMetadata`, but `ResetMetadataResult` does not.
- `GET /custom-metadata` returns `schemaName`, but the metadata-query work only fetches metadata rows plus `documents.custom_schema_id`; nothing in tracking fetches the schema name.
Recommendation: either shrink the responses to fields the service already owns, or make the method return types and query tasks match the API contract before implementation starts.
### 5. Medium: the actor/`createdBy` design is likely to regress the API contract and audit readability
Refs: plan 381, 866, 1422, tracking 189 and 329.
Why this matters: the plan repeatedly says handlers should call `cognitoauth.GetUserSubject`, which returns the Cognito `sub`, not an email or display identity; see [internal/cognitoauth/auth.go](/Users/jaybrown/dev/clients/aarete/query-orchestration.2/internal/cognitoauth/auth.go:133). But the examples still show email-like `createdBy` / `resetBy` values, and the current API commonly models `createdBy` as an email-shaped field; see [serviceAPIs/queryAPI.yaml](/Users/jaybrown/dev/clients/aarete/query-orchestration.2/serviceAPIs/queryAPI.yaml:3960) and [serviceAPIs/queryAPI.yaml](/Users/jaybrown/dev/clients/aarete/query-orchestration.2/serviceAPIs/queryAPI.yaml:4719).
If implemented literally, new endpoints will expose opaque subject IDs in fields named like human-readable actor fields. That is a contract change and makes logs/UI less useful.
Recommendation: decide explicitly whether these fields are generic actor IDs or human-readable identities. If they are IDs, rename/document them that way. If they are meant to stay user-facing, source them from a claim/helper that returns the intended display value.
### 6. Medium: the tracking doc contradicts itself on the trigger set
Refs: tracking 230-245 vs. 501.
Why this matters: section 2.2 correctly adds the new v4 Trigger 2 (`trg_prevent_legacy_extraction_on_custom_document`), but the cross-milestone reminder says "Do NOT add Trigger 2 or Trigger 3." A developer following the reminder literally can remove the very trigger the plan depends on to close the mutual-exclusivity race.
Recommendation: rewrite the reminder to name the forbidden legacy triggers explicitly, e.g. "Do NOT add v3's old client-match trigger or schema-consistency trigger."
### 7. Medium: the tracking doc has at least one behavior mismatch and one missing concurrency test
Refs: plan 459-460, 1284, tracking 55, 200, 392-393.
Why this matters:
- Milestone 1 expects the schema list call to show both v1 and v2 statuses, but the plan says `includeAllVersions` defaults to `false`. That test description is wrong unless it explicitly calls the list endpoint with `includeAllVersions=true`.
- The plan says reset and metadata writes share the same parent-row lock shape and therefore serialize correctly, but tracking only covers reset-vs-reset and reset-vs-assign. It does not cover reset-vs-`SetDocumentMetadata`.
Recommendation: make the list-test query parameters explicit and add a reset-vs-metadata-write concurrency test.
plans/mutable.metadata.plan.combo.v4.review.findings.md
-270
View File
@@ -1,270 +0,0 @@
# Review of the v4 Plan Review
Cross-check of the five findings against the current code and the v4 plan/tracking
docs. Every claim in the original review was verified by reading the cited files.
Verdict key: **Agree** / **Partial** / **Disagree** with rationale and code evidence.
---
## 1. Milestone 2 missing backend work for `GET /document/{id}` enrichment
**Verdict: Agree (clear implementation gap).**
The plan at `plans/mutable.metadata.plan.combo.v4.md:706` says `customSchemaId`
"is already a column on the `documents` row (no extra query)" and
`hasCustomMetadata` "is a single `EXISTS` subquery that piggybacks on the
existing document read." That wording implies the query is extended on the
`GetDocumentEnriched` read path. Verified state of that read path:
- `internal/database/queries/document.sql:63` — `GetDocumentEnriched` selects
`d.id, d.clientId, d.hash, d.folderId, d.filename, d.originalPath,
d.file_size_bytes` only. No `custom_schema_id`, no `EXISTS` over
`document_custom_metadata`.
- `internal/document/service.go:29` — `DocumentEnriched` struct has no
`CustomSchemaID` or `HasCustomMetadata` fields.
- `internal/document/get.go:56` — `GetEnriched` calls `GetDocumentEnriched` and
builds the result struct with no place to carry the new fields.
Tracking doc coverage:
- `plans/mutable.metadata.plan.combo.v4.tracking.md:325` — OpenAPI schema change
(`DocumentEnriched` gains two fields) only.
- `plans/mutable.metadata.plan.combo.v4.tracking.md:342` — controller-side
"DocumentEnriched builder" change only.
- `grep GetDocumentEnriched|document.sql|internal/document/` in the tracking
doc → **zero matches**.
So the checklist has nothing for (a) extending the SQL query to return
`custom_schema_id` and an `EXISTS` boolean, (b) extending the internal
`DocumentEnriched` struct, or (c) populating those fields in `GetEnriched`.
Without those, the controller "builder" at 342 has nothing to read from.
**Recommended fix**: add a Milestone 2 block under 2.x for
`internal/database/queries/document.sql` (extend `GetDocumentEnriched`),
`internal/document/service.go` (struct fields), and `internal/document/get.go`
(populate from the extended query). These are preconditions for the controller
work already on the list.
---
## 2. `GET /custom-metadata/history` unresolved API behavior
**Verdict: Agree — but the tracking doc already flags it; the fix is to
resolve it in the plan rather than at test time.**
- Plan `plans/mutable.metadata.plan.combo.v4.md:958` defines only the happy-path
query params (`documentId` required, `limit` default 50 max 200, `offset`
default 0) and a response shape. No mention of `limit=0`, `limit>200`,
non-existent `documentId`, or zero-history document behavior.
- Tracking `plans/mutable.metadata.plan.combo.v4.tracking.md:360` explicitly
calls it out: *"Decision needed + test ... the plan documents default=50,
max=200 but does not say whether an over-max value is clamped or rejected, or
whether an empty-history document returns 200 + empty array or 404. Record
the decisions in the journal and add one integration test per case matching
whatever was decided."*
The review's concern is that leaving this to "whatever was decided" during
implementation means the milestone's exit status depends on an ad-hoc
judgement call. That is a real risk: different implementers (or re-runs) can
converge on different answers, and other consumers of `/custom-metadata/history`
(e.g. the history pagination test at tracking 359) will need to match whatever
was picked.
**Recommended fix**: add one small subsection to plan Section 5.4 (or wherever
`/history` is defined) that declares:
- `limit=0` → 400
- `limit > 200` → clamp to 200 (or reject with 400 — pick one and commit)
- non-existent `documentId` → 404
- document with zero metadata rows → 200 with empty array
Then remove the "Decision needed" language from tracking 360 and just keep the
per-case tests.
Q : Recommended fix is ok.
---
## 3. Schema lifecycle: retirement permanently reserves the name
**Verdict: Agree as a documentation / escape-hatch gap.**
The review's reading is correct. Chasing the relevant lines:
- `plans/mutable.metadata.plan.combo.v4.md:125` — `UNIQUE (client_id, name,
version)` at the DB level.
- `plans/mutable.metadata.plan.combo.v4.md:403` — "`name` must be unique within
the client (for active schemas at the same version)" for `POST
/super-admin/custom-schemas`.
- `plans/mutable.metadata.plan.combo.v4.tracking.md:126` — `CreateSchema` "at
version 1" always (no path to version N for a fresh insert).
- `plans/mutable.metadata.plan.combo.v4.md:446` — `POST .../{schemaId}/versions`
"rejects with 409 Conflict" if parent is `superseded` or `retired`.
Put together, once a lineage is retired (its only active row flipped to
`retired`):
- A new `POST /super-admin/custom-schemas` with the same `name` will try to
insert `version=1`, which collides with the retired v1 row on
`uq_client_schema_name_version` → DB rejects with unique violation.
- `POST /super-admin/custom-schemas/{retiredId}/versions` is rejected by the
active-parent check at line 446.
So there is **no path** to bring the name back into use after full retirement.
The name is permanently reserved for the lifetime of the client.
That may be intentional (prevents confusion with reused names across audit
history), or it may be an unnoticed consequence of three independent rules
interacting. Either way, the plan does not state it.
**Recommended fix**: pick one of these and document it in Section 5.1 / 4.2 of
the plan:
- **Intentional**: add one sentence to Section 4.2 and to the
`DELETE /super-admin/custom-schemas/{schemaId}` doc stating that retiring a
lineage permanently reserves `(client_id, name)` and the name cannot be
reused; reference that callers should use a new name (`...-v2`,
`...-replacement`) if they need a similar schema.
- **Not intentional**: relax `POST /super-admin/custom-schemas` to pick
`version = max(version) + 1` when all prior rows for `(client_id, name)` are
non-active, and add an integration test that retirement + fresh create
produces a new lineage.
I would lean toward option 1 (simpler, keeps the invariants clean) unless
product actually expects to reuse names after retirement.
Q: go with the Intentional fix. (name is permanently reserved)
---
## 4. Phantom / duplicate work items in the tracking doc
**Verdict: Agree on two of three, half-agree on the third.**
### 4a. `CountDocumentsInFolderTree` — Agree, duplicate.
- Tracking `plans/mutable.metadata.plan.combo.v4.tracking.md:448` lists it as
new work in Milestone 4.
- `internal/database/queries/folders.sql:114` — `CountDocumentsInFolderTree`
already exists with exactly the shape the bulk cap check needs (recursive
CTE, count of documents in the tree). No new query is needed; the Milestone 4
service method can just call the existing one.
### 4b. Single-document legacy extraction existence — Agree, functional duplicate.
- Tracking `plans/mutable.metadata.plan.combo.v4.tracking.md:264` lists
`DocumentHasLegacyExtractions` as a new boolean query.
- `internal/database/queries/fieldextractions.sql:254` — `HasFieldExtraction`
already returns `EXISTS(SELECT 1 FROM documentFieldExtractionVersions WHERE
documentId = $1)`, which is semantically "does this document have any legacy
field-extraction version row?" — exactly the check the mutual-exclusivity
guard needs.
Caveat: a new name (`DocumentHasLegacyExtractions`) may still be worth adding
as a readability alias at the call site of the guard, but the tracking doc
should say "reuse/rename `HasFieldExtraction`" rather than "add a new query,"
because no new SQL is needed.
### 4c. "Audit every other `FieldExtractionService` mutation entry point" — Agree, points at nonexistent methods.
- Tracking `plans/mutable.metadata.plan.combo.v4.tracking.md:308` — "Audit
every other `FieldExtractionService` mutation entry point (e.g.
`UpdateFieldExtraction`, any `AppendValueTo*` methods) and apply the same
parent-row-lock + sentinel-error pattern..."
- `internal/fieldextraction/service.go:27` — the **only** mutator in the
current service is `CreateFieldExtraction`. The other methods on the service
(`GetCurrentFieldExtraction`, `GetFieldExtractionHistory`,
`GetFieldExtractionArrayFields`, `GetFieldExtractionByVersion`) are all
reads.
`UpdateFieldExtraction` does not exist. No `AppendValueTo*` method exists.
The v4 service is single-mutator.
**Recommended fix**: delete tracking line 308 outright (there is nothing to
audit), or rewrite it as a defensive regression test: "if a future change adds
a new mutation entry point to `FieldExtractionService`, that entry point must
take `LockDocumentForLegacyExtractionWrite` before any DML." That keeps the
intent alive without carrying a phantom task.
---
## 5. Milestone 4 is over-serialized behind Milestone 3
**Verdict: Agree.**
- Plan `plans/mutable.metadata.plan.combo.v4.md:1534` (Milestone 2 exit
criteria): *"The feature now satisfies the central requirement
('client-scoped schemas validated at write time, frozen once metadata
exists'). Everything after this milestone is additive."*
- Plan `plans/mutable.metadata.plan.combo.v4.md:1552` (Milestone 4 intro):
Bulk folder assignment builds on the single-document assign semantics, which
are delivered in Milestone 2 (`AssignSchema`, `PATCH
/super-admin/documents/{id}/schema`).
- Milestone 3 (reset-metadata) is orthogonal — it is about upgrading an
already-assigned document to a new schema version. Nothing in the bulk
folder path depends on `ResetDocumentMetadata`.
- Tracking `plans/mutable.metadata.plan.combo.v4.tracking.md:437` nonetheless
says *"Dependencies: Milestone 3 complete (the per-document assign semantics
need to be solid before adding the recursive variant)."* — but "per-document
assign semantics" are a Milestone 2 deliverable, not Milestone 3.
**Recommended fix**: change Milestone 4's dependency in the tracking doc to
"Milestone 2 complete" and add a note that Milestones 3 and 4 can be executed
in parallel by separate workstreams, with `task fullsuite:ci` as the merge
gate. This unblocks ~1 milestone of wall-clock time on the schedule.
---
## Minor / residual items from the review
- **Stale v3 wording at `plans/mutable.metadata.plan.combo.v4.md:105`** —
**Agree**. The bullet currently says *"Set automatically when a new version
is created via `PUT`"* but v4 deliberately moved this operation to `POST
/versions` (see the v4 rationale at line 409). The "via `PUT`" clause should
be changed to "via `POST /super-admin/custom-schemas/{schemaId}/versions`"
or simply "when a new version is created."
- **Reset rationale at `plans/mutable.metadata.plan.combo.v4.md:682`** —
**Partial**. The bullet references `DeleteDocumentCustomMetadata` and
`NullifyDocumentCustomSchemaId` as "already added for the delete cascade"
and points at Section 8.2. But the v4 design ripped those out in favor of
FK cascades (see tracking note at 267: *"No delete-cascade queries are
added in this milestone"*). The queries that actually exist in the reset
path are `DeleteDocumentCustomMetadataForReset` and
`NullifyDocumentCustomSchemaIdForReset` (tracking 387-388). The bullet's
names are leftover from v3 and should be updated so a reader doesn't go
looking for non-existent SQLC queries.
- **"One client per stack" authorization assumption** — **Agree it is
acceptable for this feature, but worth re-checking with Q before
implementation starts.** Plan line 714 and tracking line 170 both depend on
the production deployment model being one client per stack. If dev/uat are
ever used for anything other than test fixtures, this falls over.
Q: confirmed. One client per stack.
---
## Overall
Four of the five main findings (1, 3, 4, 5) are structural and block using
the tracking doc as the execution source of truth. Finding 2 is real but
already partially acknowledged in the tracking doc — the fix is to promote
the resolution into the plan instead of deferring to the implementer.
The two residual items (stale v3 wording at lines 105 and 682) are cleanup,
not blocking, but are cheap to fix alongside the substantive changes.
Suggested order of operations before Milestone 1 work begins:
1. Rewrite Milestone 2 to include the `GetDocumentEnriched` / `DocumentEnriched`
/ `GetEnriched` backend changes (Finding 1).
2. Resolve the `/custom-metadata/history` edge cases in plan Section 5.4 and
delete the "decision needed" language from tracking 360 (Finding 2).
3. Decide on retirement name-reuse semantics and document them in Section 4.2
/ 5.1 (Finding 3).
4. Delete the three phantom/duplicate items from the tracking doc (Finding 4):
- Rewrite tracking 448 to reference the existing
`CountDocumentsInFolderTree`.
- Rewrite tracking 264 to reuse/rename `HasFieldExtraction`.
- Delete tracking 308 (or rewrite as a regression guard).
5. Change Milestone 4's dependency from "Milestone 3 complete" to "Milestone 2
complete" in tracking 437 (Finding 5).
6. Fix the two stale v3 references at plan lines 105 and 682.
plans/mutable.metadata.plan.combo.v4.tracking.md
-575
View File
@@ -1,575 +0,0 @@
# Mutable Metadata v4 - Implementation Tracking
**Source plan**: `plans/mutable.metadata.plan.combo.v4.md`
**Journal (details / decisions / blockers)**: `./journals/implement_mutableMetadata.md`
**Purpose**: One-glance status of every task needed to ship v4. Each checkbox is a discrete, verifiable unit of work. TDD rule: write the test first, watch it fail, make it pass, check the box. If a task requires Q's input or produces a decision, log it to the journal and keep the checkbox state accurate here.
## How v4 differs from v3 tracking
v3 organized work as 12 horizontal phases (migrations first, then queries, then service, then controllers, then tests). v4 reorganizes into **four vertical slice milestones**. Each milestone lands migrations, queries, service methods, controllers, OpenAPI, Permit.io updates, and integration tests for exactly one slice of feature behavior. At the end of every milestone the feature is end-to-end usable in its current scope -- not a half-built stack.
The simplification pass (see Section 16 of the plan) also removed:
- v3's `trg_validate_schema_client_match` (v3 labelled "Trigger 2") -- replaced by the composite FK `fk_documents_custom_schema_same_client`. Note: v4 introduces a **different** Trigger 2 (`trg_prevent_legacy_extraction_on_custom_document`) — see Section 2.2; do not confuse the two.
- v3's `trg_enforce_consistent_schema_id` (v3 labelled "Trigger 3") -- made structurally impossible by removing `document_custom_metadata.schema_id`. v4 has no Trigger 3.
- `current_document_custom_metadata` view -- replaced by a `LIMIT 1` query
- The entire delete-cascade workstream (v3 Phase 5b) -- replaced by `ON DELETE CASCADE` on the new FKs
- `PUT /super-admin/custom-schemas/{schemaId}` -- replaced by `POST .../versions`
- `createdBy` request-body fields -- actor comes from the JWT
- `customSchemaName` and inlined `customMetadata` on `GET /document/{id}` -- only `customSchemaId` and `hasCustomMetadata` remain
Every task below reflects those decisions.
## How to use this document
- `[ ]` = not started
- `[~]` = in progress
- `[x]` = done AND tests passing (`task fullsuite:ci` clean for the affected code paths)
- `[!]` = blocked -- add a short `(blocked: <why>)` suffix and open a journal entry
- `[-]` = consciously skipped -- add a short `(skipped: <why>)` suffix
- Every task line ends with a `test:` marker indicating where the passing test(s) live or what test type proves it.
- When a whole milestone is green, flip the milestone header to `COMPLETE` and date-stamp it.
- Never mark a milestone complete until `task fullsuite:ci` ends with `All coverage checks passed!` with no `FAIL` lines.
---
## Milestone roll-up (high-level)
| Milestone | Title | Status | Started | Completed |
|-----------|-------|--------|---------|-----------|
| 0-INT | **BLOCKER**: Fix `task fullsuite:ci` regression (container race in `api/queryAPI`) | COMPLETE | 2026-04-15 | 2026-04-15 |
| 1 | Schema foundation (create/list/version/delete schemas; super_admin gating) | COMPLETE | 2026-04-14 | 2026-04-14 |
| 2 | Core document flow (assign schema; write/read metadata; legacy guard; `GET /document/{id}` enrichment) | COMPLETE | 2026-04-14 | 2026-04-15 |
| 3 | Administrative completion (reset-metadata; schema version concurrency; full authz matrix) | COMPLETE | 2026-04-15 | 2026-04-15 |
| 4 | Bulk operations (`POST /super-admin/folders/{folderId}/assign-schema`; 10K cap; skip reasons) | NOT STARTED | | |
| Docs | Documentation updates (runs in parallel with Milestones 3 + 4) | NOT STARTED | | |
---
## Milestone 0-INT — Fix fullsuite:ci regression — COMPLETE (2026-04-15)
**Goal**: `task fullsuite:ci` passes clean with `All coverage checks passed!` and no `FAIL` lines. This is a prerequisite for completing any other milestone.
**Background**: The mutable metadata branch caused widespread duplicate-key failures in test runs because the reused postgres container retained data between runs. A truncation fix was applied to `internal/test/database.go` (see journal entry for 2026-04-15), resolving the duplicate-key issues but introducing a new regression: `api/queryAPI` tests intermittently fail with `"unable to ping database"` under parallel execution due to a container concurrency race in testcontainers `Reuse: true`.
**What is already done**:
- `[x]` Truncation fix in `internal/test/database.go` — resolves duplicate-key violations across all packages
**What remains**:
- `[x]` Diagnose actual failure: Q's run showed the container race was resolved by Docker state cleanup; the remaining failure was function coverage below 60% on `ResetDocumentMetadata` and `mapResetError` in `api/queryAPI/customschemas_reset.go` — both at 0% because no tests existed.
- `[-]` Mutex fix — not needed; container race was accumulated Docker state, not a code bug.
- `[x]` Write tests: added `TestMapResetError` (white-box, `package queryapi`) to `custommetadata_errors_test.go`; created `api/queryAPI/customschemas_reset_test.go` (`package queryapi_test`) with 5 integration tests covering 401/200/200-idempotent/404/409. All pass.
- `[x]` Run `task fullsuite:ci` — passes with `All coverage checks passed!`, Total Coverage 85.4%, 2026-04-15.
- `[x]` Flip this milestone to COMPLETE and date-stamp.
---
## Milestone 1 - Schema foundation — COMPLETE (2026-04-14)
**Goal**: A super_admin can create, list, get, create-new-version, and delete client-scoped custom schemas. Invalid schemas are rejected. Cross-client schema bindings are structurally impossible at the database level. The `super-admin` Permit.io resource exists and gates every new schema endpoint.
**Dependencies**: None.
**Exit criteria**:
- `task fullsuite:ci` green
- End-to-end integration test proves super_admin can POST a schema, POST a new version, `GET /super-admin/custom-schemas?clientId=...&name=aircraft-engineering&includeAllVersions=true&status=any` returns both rows with the correct statuses (v1 `superseded`, v2 `active`), `GET` with the default query (no `status`, no `includeAllVersions`) returns only v2 `active`, DELETE removes a retired schema, and `user_admin` / `client_user` / `auditor` are correctly gated on every route.
- Direct-SQL test proves the composite FK refuses a cross-client schema binding.
### 1.1 Migration 127 - `schema_status_type` enum + `client_metadata_schemas`
> **Numbering note**: The repo contains migrations through `00000000000126`. If another branch lands new migrations before this feature merges, renumber this block and all references in the plan and tracking docs.
- [x] Create `internal/database/migrations/00000000000127_create_client_metadata_schemas.up.sql` with `CREATE TYPE schema_status_type AS ENUM ('active', 'superseded', 'retired')` -- test: migration applies cleanly on empty DB
- [x] Create `client_metadata_schemas` table per Section 4.2 of the plan -- columns, PK, FK to `clients(clientId) ON DELETE CASCADE`, `schema_def jsonb`, `version`, `status`, timestamps, `created_by` -- test: `\d client_metadata_schemas` shows all columns and the cascade FK
- [x] Add `CONSTRAINT uq_client_schema_name_version UNIQUE (client_id, name, version)` -- test: direct SQL INSERT of two rows with same `(client_id, name, version)` fails
- [x] **v4**: Add `CONSTRAINT uq_cms_id_client UNIQUE (id, client_id)` (required as the target of the composite FK in Milestone 1.3) -- test: constraint visible in `\d client_metadata_schemas`
- [x] Add `CONSTRAINT chk_schema_def_size CHECK (pg_column_size(schema_def) <= 70000)` -- test: insert of a schema_def larger than 70000 bytes fails with CHECK violation
- [x] Add indexes `idx_cms_client_id`, `idx_cms_client_name`, `idx_cms_client_status` -- test: `\di client_metadata_schemas` shows the three indexes
- [x] Create `00000000000127_create_client_metadata_schemas.down.sql` dropping the table and enum in correct order -- test: up then down leaves DB state byte-identical to pre-migration
- [x] Run `task db:generate` after migration lands -- test: no errors
### 1.2 Schema validator component + dependency
- [x] Add `github.com/santhosh-tekuri/jsonschema/v6` to `go.mod` -- test: `go mod tidy` clean, build clean
- [x] Create `internal/customschema/constants.go` with `MaxSchemaDefinitionBytes = 65536`, `MaxMetadataPayloadBytes = 1048576`, `MaxBulkAssignDocuments = 10000` -- test: constants referenced by tests
- [x] Create `internal/customschema/validator.go` with `SchemaValidator` struct -- test: compiles
- [x] Implement `ValidateSchemaDefinition(schemaDef json.RawMessage) error` -- test: valid JSON Schema draft 2020-12 accepted
- [x] Reject schema definitions that are not valid JSON -- test: TDD test with malformed JSON
- [x] Reject schema definitions that fail JSON Schema meta-validation -- test: TDD test with `{"type": "nonsense"}`
- [x] Reject schema definitions where root `type` is not `"object"` -- test: TDD test with `{"type": "array"}`
- [x] Reject schema definitions where root `additionalProperties` is not explicitly present -- test: TDD test with schema missing the key
- [x] Accept schema definitions where root `additionalProperties` is explicitly `true` or `false` -- test: TDD tests for both
- [x] Reject schema definitions larger than `MaxSchemaDefinitionBytes` -- test: TDD test with 64KB+1 payload
- [x] `go test ./internal/customschema/...` green (validator only) -- test: package level
### 1.3 Migration 128 - `documents.custom_schema_id` column + composite FK
> **v4 monotonic ordering**: Milestone 1 ships migrations 127 and 128. Milestone 2 ships 129 and 130. The filename numbering must match rollout order because `golang-migrate`'s `Up()` only applies versions strictly greater than the current DB version -- if Milestone 1 lands a 129 file, a later Milestone 2 file numbered 128 will be silently skipped or flagged as out of order. Earlier drafts of v4 had 128 and 129 swapped; do not revert that swap.
- [x] Create `00000000000128_add_custom_schema_id_to_documents.up.sql` -- `ALTER TABLE documents ADD COLUMN custom_schema_id uuid NULL` (**no inline `REFERENCES` clause**; the composite FK below handles it) -- test: column present and nullable
- [x] Add composite FK: `ALTER TABLE documents ADD CONSTRAINT fk_documents_custom_schema_same_client FOREIGN KEY (custom_schema_id, clientId) REFERENCES client_metadata_schemas(id, client_id)` -- test: constraint visible; direct SQL INSERT of a document with `custom_schema_id` pointing at a schema owned by a different client fails with FK violation. **v4 implementation note**: the plan Section 4.4 SQL writes `"clientId"` quoted, but the `documents.clientId` column was declared unquoted in migration 5, so Postgres stored it as lowercase `clientid`. A quoted `"clientId"` in the FK declaration is a case-sensitive literal lookup that fails with `column "clientId" referenced in foreign key constraint does not exist`. The migration uses **unquoted** `clientId` which folds to `clientid` at parse time and matches. This same gotcha applies to any future DDL or trigger that references `documents."clientId"`.
- [x] Create `idx_documents_custom_schema_id` -- test: index present
- [x] Write `down.sql` that drops the index, the constraint, and the column in correct order -- test: round trip clean
- [x] Verify existing documents rows have `custom_schema_id = NULL` after migration -- test: `SELECT COUNT(*) FROM documents WHERE custom_schema_id IS NOT NULL` returns 0
### 1.4 SQLC queries for schema CRUD
New query file: `internal/database/queries/customschemas.sql`
- [x] `CreateClientMetadataSchema` -- INSERT returning full row -- test: generated code compiles, query name appears in `*.sql.go`
- [x] `GetClientMetadataSchema` (by id) -- test: generated
- [x] `ListClientMetadataSchemas` (filters: client_id required, name optional, status optional with default `active`, pagination limit/offset; includes `documentCount` subquery against `documents.custom_schema_id`) -- test: generated; unit test confirms `documentCount` populated correctly
- [x] `GetLatestSchemaByName` -- test: generated
- [x] `GetSchemaDocumentCount` (count of `documents.custom_schema_id = $1`) -- test: generated
- [x] `SetSchemaStatus` (update status, used for supersede and retire) -- test: generated
- [x] `GetMaxSchemaVersion` (for auto-increment on version creation) -- test: generated
- [x] `LockSchemaVersionsForName` (`SELECT ... FOR UPDATE` on all versions of a `(client_id, name)` pair) -- test: generated
- [x] `task generate` clean; no diff after re-run -- test: idempotent
> **Note**: `GetSchemaMetadataRecordCount` is deliberately **not** in this list. v3 used it for `canDelete`, but v4's `canDelete` is derived from `GetSchemaDocumentCount` alone (the metadata table has no `schema_id` column).
### 1.5 Audit sink
> **Why in Milestone 1**: Schema CRUD needs to log schema.create, schema.update, schema.delete. Defining the sink now lets Milestone 1's service methods land with audit wired in, and Milestones 2/3/4 reuse the same sink without back-fill.
- [x] Decide sink location: either `internal/customschema/audit.go` (feature-local) or `internal/audit/` (shared) -- test: decision recorded in journal (feature-local chosen 2026-04-14)
- [x] Define `AuditRecord` struct: `Actor string`, `Action string` (enum: `schema.create|schema.update|schema.delete|schema.assign|metadata.write|metadata.reset|folder.assign_schema`), `ResourceID uuid.UUID`, `ClientID uuid.UUID`, `Timestamp time.Time`, `Details map[string]any` -- test: compiles
- [x] Define `AuditSink` interface with `Record(ctx, AuditRecord) error` -- test: compiles
- [x] Provide a default implementation (structured `slog` with `audit=true` attribute, or a DB-backed appender if Q decides) -- test: unit test verifies a recorded event lands in the sink (`TestSlogAuditSink_Record` — 4 subtests green)
- [x] Wire the sink into `customschema.Service` via constructor arg -- test: compiles (Service.New(cfg, validator, audit) landed in §1.6)
### 1.6 Service layer - schema CRUD
- [x] Create `internal/customschema/service.go` with `Service` struct holding `cfg`, `validator`, `audit` -- test: compiles
- [x] Create `internal/customschema/models.go` with `Schema`, `SchemaSummary`, `CreateSchemaInput`, `CreateSchemaVersionInput`, `ListFilters` -- test: compiles
- [x] **v4**: `CreateSchemaInput` and `CreateSchemaVersionInput` do **not** carry a `CreatedBy` field. Every write method accepts `actor string` as a separate argument. -- test: unit test proves the service method signature has `actor string` and not a body field
- [x] `CreateSchema(ctx, input, actor)` -- meta-validates, enforces 64KB, enforces explicit `additionalProperties`, enforces `name` unique within client for active schemas at version 1 -- test: happy path
- [x] `CreateSchema` rejects invalid JSON Schema -- test: TDD
- [x] `CreateSchema` rejects oversize definition -- test: TDD
- [x] `CreateSchema` rejects duplicate name for same client at version 1 -- test: TDD (service returns `ErrSchemaNameConflict`, wraps pg `23505` / `uq_client_schema_name_version`)
- [x] `CreateSchema` writes `schema.create` audit entry -- test: TDD
- [x] `CreateSchemaVersion(ctx, parentSchemaID, input, actor)` -- uses `LockSchemaVersionsForName` -> verifies the path `parentSchemaID` matches the single row with `status='active'` for `(client_id, name)` -> `GetMaxSchemaVersion` -> INSERT new version -> `SetSchemaStatus(parent, 'superseded')` inside a single tx -- test: happy path produces v2
- [x] `CreateSchemaVersion` concurrency: two concurrent calls against same `(client_id, name)` produce distinct version numbers -- test: TDD with goroutines against testcontainer DB (v4 resolution: winner writes v2, loser returns `ErrSchemaParentNotActive` — no retry loop; the `FOR UPDATE` serializes the two tx and the loser observes the parent as superseded)
- [x] `CreateSchemaVersion` rejects when parent does not exist -- test: TDD (returns `ErrSchemaNotFound`)
- [x] **v4 non-active parent rejection**: `CreateSchemaVersion` rejects with 409 when the path `parentSchemaID` points at a `superseded` row while a different row is the active version for the lineage (e.g. v2 is active, caller passes v1's ID) -- test: TDD asserts 409, asserts no new row inserted, asserts v2 still has `status='active'` and v1 still has `status='superseded'` (service returns `ErrSchemaParentNotActive`)
- [x] **v4 retired parent rejection**: `CreateSchemaVersion` rejects with 409 when the path `parentSchemaID` points at a `retired` row -- test: TDD asserts 409, asserts no new row inserted, asserts the retired row's status is unchanged (service returns `ErrSchemaParentRetired`)
- [x] **v4 active-parent invariant**: After `CreateSchemaVersion` succeeds there is exactly one row with `status='active'` for the `(client_id, name)` lineage -- test: TDD asserts the invariant after happy path, after two sequential calls (v1 -> v2 -> v3), and after the two rejection cases above
- [x] `CreateSchemaVersion` rejects invalid schema definition (same validator rules as create) -- test: TDD
- [x] `CreateSchemaVersion` writes `schema.update` audit entry -- test: TDD (`ResourceID` is the new version's ID; `Details` carries `name`, `version`, `parent_schema_id`)
- [x] `GetSchema(ctx, schemaID)` -- test: returns full row including `schema_def`
- [x] `GetSchema` 404 when not found -- test: TDD (returns `ErrSchemaNotFound` via `errors.Is(err, pgx.ErrNoRows)`)
- [x] `ListSchemas(ctx, clientID, filters)` -- default returns only `active` and only the latest version per `(client_id, name)` -- test: TDD
- [x] `ListSchemas` filter by `name`, by `status` (including the `any` sentinel for "all statuses"), by `includeAllVersions=true`, with pagination -- test: TDD per case, including one case that sets both `status=any` and `includeAllVersions=true` against a lineage that has one active + one superseded row and asserts both rows are returned
- [x] `ListSchemas` populates `documentCount` and `canDelete` -- test: TDD
- [x] `ListFilters` accepts an `Status` field with zero-value meaning "active" (default) and a dedicated `StatusAny` sentinel (or `*schemaStatusType` with `nil` meaning "any") so handlers can distinguish "filter omitted" from "caller asked for everything" — test: unit test on the mapping from query-string `status=any` to the service-layer value
- [x] `DeleteSchema(ctx, schemaID, actor)` -- sets status to `retired` -- test: happy path
- [x] `DeleteSchema` returns 409 if any document references the schema via `custom_schema_id` -- test: TDD (returns `ErrSchemaInUse`)
- [x] `DeleteSchema` writes `schema.delete` audit entry -- test: TDD
- [x] `go test ./internal/customschema/...` green -- test: package level
- [x] `task test:race` green for package -- test: no races (`go test -race ./internal/customschema/... → ok 8.303s`)
### 1.7 OpenAPI spec - schema CRUD
- [x] Add `SuperAdminSchemaService` tag with description explicitly stating the `super_admin`-only boundary -- test: spec lints
- [x] Add `SchemaStatus` enum (`active`/`superseded`/`retired`) -- test: codegen produces Go type
- [x] `CustomSchemaRequest` schema (no `createdBy` field) -- test: codegen
- [x] `CustomSchemaVersionRequest` schema (no `createdBy`; v4 rename from v3's `CustomSchemaUpdateRequest`) -- test: codegen
- [x] `CustomSchemaResponse` schema with `createdBy` declared as plain string (**not** `format: email`) per plan Section 8.3 "Actor fields" -- test: codegen; spec lint
- [x] Spec check: assert that none of the new response schemas declare `format: email` on a `createdBy` / `resetBy` field -- test: grep or programmatic spec check in `task openapi:lint` (verified via `grep -nE 'format: ?email' serviceAPIs/queryAPI.yaml | grep -iE 'createdBy|resetBy'` → 0 matches)
- [x] `CustomSchemaListResponse` schema with `documentCount` and `canDelete` fields -- test: codegen
- [x] `POST /super-admin/custom-schemas` path -- test: codegen produces handler interface
- [x] `GET /super-admin/custom-schemas` (with query params) -- test: codegen
- [x] `GET /super-admin/custom-schemas/{schemaId}` -- test: codegen
- [x] **v4**: `POST /super-admin/custom-schemas/{schemaId}/versions` path (replaces v3's `PUT /super-admin/custom-schemas/{schemaId}`) -- test: codegen
- [x] `DELETE /super-admin/custom-schemas/{schemaId}` -- test: codegen
- [x] `task generate` clean -- test: no errors
- [x] Generated controllers compile -- test: `go build ./...` (stub handlers land in `api/queryAPI/customschemas_stub.go` returning `http.StatusNotImplemented`; real handlers arrive in §1.9)
### 1.8 Permit.io policy file + setup tool run
> **Isolation model (v4)**: Production is deployed **one client per stack** (separate DB, services, Cognito pool). The whole stack is the client scope, so the Permit.io role/action/resource-type check is the complete authorization story on production — there is no other client to cross into. Shared dev/uat stacks are test data only (see plan Section 5.3). Do **not** add application-level document->client ownership checks to the custom-metadata or super-admin routes in v4; there is no cross-client data to protect from inside a production stack, and retrofitting such a check would be an API-wide initiative out of scope for this feature.
- [x] Add `super-admin` resource to `cmd/auth_related/permit.setup/permit_policies.yaml` with `get`, `post`, `patch`, `delete` actions -- test: YAML valid (verified via `python3 -c 'yaml.safe_load(...)'` 2026-04-14)
- [x] Add `custom-metadata` resource with `get`, `post` actions (provisioning it now lets Milestone 2 ship without another setup-tool run) -- test: YAML valid
- [x] Grant `super_admin` role: `super-admin: [get, post, patch, delete]`, `custom-metadata: [get, post]` -- test: YAML valid
- [x] Confirm `user_admin` role NOT granted `super-admin` -- test: visual diff (YAML parser confirmed `user_admin super-admin: None`)
- [x] Grant `user_admin` role: `custom-metadata: [get, post]` -- test: YAML valid
- [x] Grant `auditor` role: `super-admin: [get]`, `custom-metadata: [get]` -- test: YAML valid
- [x] Grant `client_user` role: `custom-metadata: [get, post]` -- test: YAML valid
- [x] Add documentation-only `policy_mappings` entries for every `/super-admin/*` path including `/versions` and `/reset-metadata` -- test: YAML valid
- [x] Add documentation-only `policy_mappings` entries for every `/custom-metadata/*` path -- test: YAML valid
- [x] **v4**: No `put` action needed on `super-admin` -- v4 replaced the v3 `PUT` route with `POST .../versions`. Record this decision in the journal so no one re-adds it. -- test: journal entry (recorded)
- [x] Run `cmd/auth_related/permit.setup/run.tool.dev.sh` -- test: exit code 0 (Q ran 2026-04-14; confirmed dev environment updated)
- [x] Verify `super-admin` and `custom-metadata` resources exist in dev with expected actions -- test: confirmed by Q running the setup tool successfully 2026-04-14
- [x] Verify `super_admin` role has expected grants and `user_admin` has NO `super-admin` grants in dev -- test: confirmed by Q 2026-04-14
- [!] Run `run.tool.uat.sh` after dev verified -- test: exit 0 (deferred until feature complete per Q directive 2026-04-14: "the others do not need to be run until this feature is completed")
- [!] Same verification pass on uat -- test: list + role inspect (deferred until feature complete)
- [!] Run `run.tool.prod.sh` after uat verified AND only with Q's explicit go-ahead -- test: exit 0, Q confirmation logged in journal (deferred until feature complete)
- [!] Same verification pass on prod -- test: list + role inspect (deferred until feature complete)
- [x] Journal entry with timestamps of each env run (dev: 2026-04-14; uat/prod: deferred)
### 1.9 Schema CRUD controllers
- [x] Create `api/queryAPI/customschemas.go` skeleton with handler receivers wired to `customschema.Service` -- test: compiles (replaced the §1.7 stub file)
- [x] **v4 actor extraction helper**: every handler in this file derives actor identity via `cognitoauth.GetUserSubject(c)` (returns the JWT `sub` claim — an opaque Cognito subject UUID, **not** an email) and passes it to the service as a separate argument; any `createdBy` on the request body is ignored (actually absent from v4 schemas, but the test codifies the intent) -- test: handler unit test sets a JWT subject via middleware fixture to a fixed UUID, posts a body that includes an extra `createdBy` key with a different email-shaped value, asserts the service receives the JWT subject UUID and the body key has no effect, and asserts the response `createdBy` is the UUID (not the body value, not an email) (handler tests set claims via `ctx.Set("user_claims", ...)` mirroring `eulaHandlers_test.go`)
- [x] `POST /super-admin/custom-schemas` handler -- parse, validate, derive actor, call `CreateSchema`, map errors to 400/409/500 -- test: TDD (`TestCreateCustomSchema_Handler` 5 subtests)
- [x] `GET /super-admin/custom-schemas` handler with query params -- test: TDD (`TestListCustomSchemas_Handler` 4 subtests)
- [x] `GET /super-admin/custom-schemas/{schemaId}` handler -- test: TDD (`TestGetCustomSchema_Handler` 2 subtests)
- [x] **v4**: `POST /super-admin/custom-schemas/{schemaId}/versions` handler -- derives actor, calls `CreateSchemaVersion` -- test: TDD (`TestCreateCustomSchemaVersion_Handler` 3 subtests)
- [x] `DELETE /super-admin/custom-schemas/{schemaId}` handler -- test: TDD including 409 path (`TestDeleteCustomSchema_Handler` 3 subtests)
- [x] `go test ./api/queryAPI/customschemas_test.go` green -- test: package (17 subtests PASS under scoped -run invocations)
### 1.10 Milestone 1 integration tests
- [x] End-to-end test: super_admin POSTs schema v1 -> GET returns it with version 1, active -- test: integration against testcontainer DB (`TestMilestone1_CreateV1AndGet`)
- [x] End-to-end test: super_admin POSTs schema v2 via `POST .../versions` -> `GET /super-admin/custom-schemas?clientId=...&name=...&includeAllVersions=true&status=any` returns v1 (status `superseded`) and v2 (status `active`). Also assert the default `GET` (no `includeAllVersions`, no `status`) returns **only** v2 (the default filter is `includeAllVersions=false` + `status=active`, which intentionally hides superseded predecessors from the common admin UX). -- test: integration (`TestMilestone1_CreateV2ListSemantics` with 2 subtests `default_list_returns_active_latest_only` and `include_all_versions_status_any_returns_both`)
- [x] End-to-end test: DELETE on a schema with documents bound returns 409 -- test: integration (`TestMilestone1_DeleteBoundReturns409`; asserts row stays `active` in DB on denial)
- [x] End-to-end test: DELETE on an unreferenced schema succeeds and sets status `retired` -- test: integration (`TestMilestone1_DeleteUnreferencedRetires`)
- [x] Direct-SQL cross-client rejection test: attempt an `UPDATE documents SET custom_schema_id = 'schema-owned-by-client-B' WHERE id = 'doc-owned-by-client-A'` and assert the composite FK fires -- test: integration (`TestMilestone1_CrossClientFKRejection`; asserts the error text mentions `fk_documents_custom_schema_same_client` and docA.custom_schema_id is still NULL)
- [x] Authorization matrix for Milestone 1 endpoints: `super_admin` positive on all methods; `user_admin` 403 on every route; `client_user` 403; `auditor` GET-only on the two read routes, 403 on write routes -- test: one integration test per (role, route, method) cell, each asserting status + DB unchanged on denial (`TestMilestone1_AuthorizationMatrix` — 4 roles × 5 routes = 20 subtests; `runPermitGate` reproduces the `performPermitIOAuthorization` choke-point logic in-test using only the exported `cognitoauth` symbols `GetUserSubject`, `GetResourceFromRoute`, `GetActionFromMethod`, `PermitChecker`)
- [x] `task fullsuite:ci` clean with `All coverage checks passed!` -- test: full run (55 packages OK, 0 FAIL, Total Coverage 86.0%, 2026-04-14 evening run captured in `/tmp/fullsuite-ci.log`)
- [x] Journal entry: Milestone 1 exit snapshot (test counts, coverage numbers, timestamp, commit SHA) (see 2026-04-14 Milestone 1 COMPLETE entry in `journals/implement_mutableMetadata.md`)
---
## Milestone 2 - Core document flow
**Goal**: A super_admin can bind a schema to a document. A client_user (or higher) can write and read validated custom metadata on that document. The legacy field extraction path rejects writes to documents that have been opted into the custom schema system. `GET /document/{id}` surfaces two new fields for any reader. The mutual-exclusivity invariant is proven end-to-end.
**Dependencies**: Milestone 1 complete.
**Exit criteria**:
- Full round-trip test: create schema -> assign to document -> POST valid metadata -> GET returns it -> attempt invalid metadata (400) -> attempt legacy field extraction on same document (409).
- FK-cascade delete test: delete a document that has custom metadata, verify rows are cleaned up automatically.
- `task fullsuite:ci` green.
### 2.1 Migration 129 - `document_custom_metadata` table
- [x] Create `00000000000129_create_document_custom_metadata.up.sql` with `document_custom_metadata` table per Section 4.3 of the plan -- columns: `id`, `document_id`, `metadata`, `version`, `created_at`, `created_by`. **No `schema_id` column.** -- test: `\d document_custom_metadata` confirms column set
- [x] FK on `document_id` uses `ON DELETE CASCADE` -- test: direct SQL delete of a document row removes its metadata rows
- [x] Add `CONSTRAINT uq_doc_custom_metadata_version UNIQUE (document_id, version)` -- test: direct SQL duplicate `(document_id, version)` INSERT fails
- [x] Add `idx_dcm_doc_version_desc ON document_custom_metadata(document_id, version DESC)` (the only index v4 needs on this table) -- test: index present
- [x] **v4**: No `idx_dcm_schema_id` (column does not exist). **No view.** Record this explicitly in the migration file comment so a future reader comparing to v3 sees why. -- test: visual
- [x] Create corresponding `down.sql` dropping the table -- test: up/down round trip clean
### 2.2 Migration 130 - schema invariant triggers (v4: Trigger 1 + Trigger 2)
- [x] Create `00000000000130_add_schema_invariant_triggers.up.sql` -- test: migration applies cleanly
- [x] Define `trg_prevent_schema_reassignment()` function per Section 4.7 Trigger 1 -- test: function exists in `pg_proc`
- [x] Attach trigger to `documents` BEFORE UPDATE OF `custom_schema_id` -- test: trigger listed in `\d documents`
- [x] Trigger 1 unit test: insert document, assign schema, write custom metadata, attempt UPDATE `custom_schema_id` -- expect `RAISE EXCEPTION` with "custom metadata already exists" message
- [x] Trigger 1 unit test: insert document, add legacy extraction row, attempt UPDATE `custom_schema_id` -- expect `RAISE EXCEPTION` with "legacy field extractions already exist" message
- [x] Trigger 1 unit test: insert document with no metadata and no extractions, UPDATE `custom_schema_id` from schema_a to schema_b -- expect success (pre-binding change allowed)
- [x] Define `trg_prevent_legacy_extraction_on_custom_document()` function per Section 4.7 Trigger 2 -- reads `documents.custom_schema_id` with `FOR SHARE` and raises `check_violation` if non-null -- test: function exists in `pg_proc`
- [x] Attach trigger to `documentFieldExtractions` BEFORE INSERT -- test: trigger listed in `\d "documentFieldExtractions"`
- [x] Trigger 2 unit test: insert document with `custom_schema_id` set, attempt direct-SQL INSERT into `documentFieldExtractions` -- expect `RAISE EXCEPTION` with "bound to custom schema" message and SQLSTATE `23514` (`check_violation`)
- [x] Trigger 2 unit test: insert document with `custom_schema_id = NULL`, INSERT into `documentFieldExtractions` -- expect success (no-op guard path)
- [x] Trigger 2 concurrency test: start tx A running `AssignSchema`-style `SELECT documents FOR UPDATE` + `UPDATE documents SET custom_schema_id = S`, then in tx B run `INSERT documentFieldExtractions` targeting the same document -- tx B's trigger's `FOR SHARE` must block until tx A commits; after tx A commits tx B's INSERT must fail with the check_violation -- test: TDD with goroutines against testcontainer DB
- [x] **v4**: Migration intentionally does NOT define v3's `trg_validate_schema_client_match` (replaced by the composite FK in Milestone 1.3) -- record this in a migration-file comment for future readers
- [x] **v4**: Migration intentionally does NOT define v3's `trg_enforce_consistent_schema_id` (not needed -- `document_custom_metadata` has no `schema_id` column) -- record this in a migration-file comment
- [x] Write `down.sql` dropping both triggers and both functions in correct order -- test: up/down round trip clean
- [x] Run `task generate` after migrations land -- test: clean
- [x] Run `task test:unit:short` -- test: all existing tests still pass (no regression)
### 2.3 SQLC queries for metadata + document binding
- [x] `CreateDocumentCustomMetadata` (no `schema_id` parameter) -- test: generated code compiles
- [x] `GetCurrentDocumentCustomMetadata` -- `ORDER BY version DESC LIMIT 1` using `idx_dcm_doc_version_desc` (**v4: no view**). **Joins `documents` and `client_metadata_schemas`** to also return `schema_id`, `schema_name`, `schema_version` so the `GET /custom-metadata` handler can populate its response in a single round-trip -- test: generated; unit test asserts the returned row carries the schema name/version for a metadata row whose parent document has a bound schema
- [x] `GetDocumentCustomMetadataByVersion` -- same join as above so `GET /custom-metadata/version` returns matching schema decoration -- test: generated
- [x] `GetDocumentCustomMetadataHistory` (with limit/offset) -- no schema join; the history response returns version summaries only -- test: generated
- [x] **v4**: `LockDocumentForMetadataWrite` (`SELECT id FROM documents WHERE id = @document_id FOR UPDATE`; parent-row lock that serializes first and subsequent writes) -- test: generated. **Replaces** v3's `LockDocumentCustomMetadataForVersion`.
- [x] `GetMaxDocumentMetadataVersion` -- `SELECT COALESCE(MAX(version), 0) FROM document_custom_metadata WHERE document_id = @document_id` -- test: generated
- [x] `SetDocumentCustomSchemaId` -- test: generated
- [x] `GetDocumentCustomSchemaId` -- test: generated
- [x] **Reuse existing query**: `HasFieldExtraction` already exists in `internal/database/queries/fieldextractions.sql` (`EXISTS(SELECT 1 FROM documentFieldExtractionVersions WHERE documentId = $1)`) and is semantically "does this document have any legacy field-extraction version row?" — exactly the check the mutual-exclusivity guard and `AssignSchema` legacy-extraction rejection need. Do **not** add a new `DocumentHasLegacyExtractions` query. Call `HasFieldExtraction` from the service layer. If readability at the call site is a concern, wrap the call in a private service-layer helper named `documentHasLegacyExtractions(ctx, id)` that just forwards to `HasFieldExtraction` — no new SQL, no new generated code -- test: service unit test confirms `AssignSchema` and the metadata write path both reject documents that have an existing `documentFieldExtractionVersions` row
- [x] `task generate` clean -- test: idempotent
> **v4**: No delete-cascade queries are added in this milestone. `DeleteDocumentCustomMetadata`, `NullifyDocumentCustomSchemaId`, and `DeleteClientMetadataSchemas` from v3 are **not** created -- the FK cascades replace them.
### 2.3a Extend `internal/document` backend for `GET /document/{id}` enrichment
> **Why this subsection exists**: Plan Section 5.2 declares that `GET /document/{id}` gains two new response fields (`customSchemaId`, `hasCustomMetadata`). The controller-side builder in §2.7 reads those values out of the `document.DocumentEnriched` struct, but as of the start of Milestone 2 the struct has neither field and the `GetDocumentEnriched` SQL query (`internal/database/queries/document.sql:63`) does not return either column. Without the three changes below, the §2.7 builder has nothing to read and the §2.8 integration tests will fail on the enrichment assertions. Do this subsection **before** §2.7.
- [x] Extend the `GetDocumentEnriched` query in `internal/database/queries/document.sql` to add `d.custom_schema_id` to the SELECT list and a correlated `EXISTS(SELECT 1 FROM document_custom_metadata WHERE document_id = d.id) AS has_custom_metadata` subquery — no join to `document_custom_metadata` (the `EXISTS` keeps the plan flat and index-friendly). -- test: unit test against testcontainer DB asserts the query returns the two new columns with the right values for (a) a document with no schema bound, (b) a document with a schema bound but zero metadata rows, (c) a document with a schema bound and >=1 metadata rows
- [x] `task db:generate` clean — confirm the generated `GetDocumentEnriched` row type in `internal/database/repository/` now has `CustomSchemaID` and `HasCustomMetadata` fields -- test: `go build ./internal/database/repository/...` clean; `grep -n CustomSchemaID internal/database/repository/document.sql.go` finds the new field
- [x] Extend the `DocumentEnriched` struct in `internal/document/service.go` to add `CustomSchemaID *uuid.UUID` (nullable because most documents are not schema-bound) and `HasCustomMetadata bool`. Place them adjacent to `HasTextRecord` and `FileSizeBytes` so the field layout of the struct stays topically grouped -- test: `go build ./internal/document/...` clean; unit test instantiates the struct and asserts the two new fields zero-value to `nil` and `false`
- [x] Update `GetEnriched` in `internal/document/get.go` to copy `docEnriched.CustomSchemaID` and `docEnriched.HasCustomMetadata` from the extended generated row into the returned `DocumentEnriched` struct — **no** separate follow-up query, no extra round-trip to the DB -- test: service-level unit test against testcontainer DB asserts the returned struct carries the right values for all three fixture cases (no schema, schema no metadata, schema with metadata). Assert in a second test that the function issues **exactly** the same number of DB queries before and after the change (i.e., no regression from "one enriched read" to "enriched read + follow-up exists check")
- [x] Run existing tests for `internal/document/` and `api/queryAPI/document*` — every pre-existing test that reads `DocumentEnriched` must still pass without modification; the two new fields are additive. -- test: `go test ./internal/document/... ./api/queryAPI/... -run Document` green
### 2.4 Service layer - metadata + document binding
- [x] Add `CustomMetadata`, `MetadataVersion`, `SetMetadataInput` types to `models.go` -- test: compiles. **`SetMetadataInput` has no `CreatedBy` field.** **`CustomMetadata` carries `SchemaID`, `SchemaName`, `SchemaVersion`** populated from the joined query in 2.3 (Section 7.1 of the plan has the struct shape).
- [x] `SetDocumentMetadata(ctx, input, actor)` -- derives schema from `documents.custom_schema_id`, never trusts client -- test: happy path
- [x] `SetDocumentMetadata` rejects when document has no `custom_schema_id` -- test: TDD (400)
- [x] `SetDocumentMetadata` rejects when document has legacy extractions -- test: TDD (409)
- [x] `SetDocumentMetadata` validates metadata against schema via `validator.ValidateMetadata` -- test: TDD with conforming and non-conforming payloads
- [x] `SetDocumentMetadata` enforces `MaxMetadataPayloadBytes` -- test: TDD
- [x] `SetDocumentMetadata` serializes version assignment by taking `LockDocumentForMetadataWrite` on the parent row, then `GetMaxDocumentMetadataVersion`, then INSERT `version = max+1` -- test: TDD confirms the parent-row lock is acquired before the version read
- [x] `SetDocumentMetadata` concurrency: two concurrent writers against the **same** document that already has >=1 metadata row -- both succeed, produce distinct versions, no unique-violation -- test: TDD with goroutines against testcontainer DB
- [x] `SetDocumentMetadata` concurrency: two concurrent **first** writers against a document with zero metadata rows -- both succeed, produce `version=1` and `version=2` in some order -- test: TDD with goroutines, proves the parent-row lock is what serializes the first write
- [x] `SetDocumentMetadata` retries once on `UNIQUE (document_id, version)` violation as a belt-and-suspenders fallback -- test: TDD with an injected unique-violation on the first INSERT attempt
- [x] `SetDocumentMetadata` writes `metadata.write` audit entry -- test: TDD
- [x] `GetCurrentMetadata(ctx, documentID)` -- test: uses the LIMIT 1 query, returns latest
- [x] `GetMetadataByVersion(ctx, documentID, version)` -- test: TDD
- [x] `GetMetadataHistory(ctx, documentID)` with pagination -- test: TDD
- [x] Add `AssignSchemaResult` type to `models.go` per plan Section 7.1 (`DocumentID uuid.UUID`, `CustomSchemaID *uuid.UUID`, `SchemaName *string`, `SchemaVersion *int`) -- test: compiles
- [x] `AssignSchema(ctx, documentID uuid.UUID, schemaID *uuid.UUID, actor string) (*AssignSchemaResult, error)` — signature takes `schemaID` as a pointer so `nil` clears the binding and returns an `AssignSchemaResult` with all three schema fields `nil` -- test: happy path single-document assignment returns a populated result
- [x] `AssignSchema` populates `SchemaName` and `SchemaVersion` on the result from the same schema row read it already does for the `active` / same-client validation — no new query is added for this decoration -- test: TDD asserts result fields match the target schema row
- [x] `AssignSchema` takes `SELECT id, "clientId", custom_schema_id FROM documents WHERE id = $1 FOR UPDATE` as the **first** statement inside the transaction, before any legacy-extraction existence check or the `UPDATE documents` -- this is the symmetric parent-row lock for the mutual-exclusivity invariant described in plan Section 7.3 -- test: TDD; code review confirms the lock precedes all other DML
- [x] `AssignSchema` rejects if schema and document belong to different clients -- the composite FK does this at the DB layer; the app-layer check converts the FK violation to a clean 409 -- test: TDD
- [x] `AssignSchema` rejects if schema status is not `active` -- test: TDD
- [x] `AssignSchema` rejects if document already has custom metadata (matches Trigger 1 behavior; expect 409) -- test: TDD
- [x] `AssignSchema` rejects if document has legacy extractions (service-layer check under the parent-row lock plus Trigger 1 as backstop; expect 409) -- test: TDD
- [x] `AssignSchema` allows setting `schemaID == nil` to clear binding when no metadata exists; result has `CustomSchemaID`, `SchemaName`, `SchemaVersion` all `nil` -- test: TDD
- [x] `AssignSchema` writes `schema.assign` audit entry -- test: TDD
- [x] `go test ./internal/customschema/...` green -- test: full package green for Milestones 1+2 service methods
### 2.5 Legacy field extraction guard + controller 409 mapping
> **v4 concurrency requirement**: A non-locking `SELECT documents.custom_schema_id` is NOT sufficient. Plan Section 7.3 walks through the race: two transactions each read a stale snapshot under `READ COMMITTED`, both pass the pre-check, and both commit opposing writes. The fix is a `SELECT ... FOR UPDATE` on the parent `documents` row as the **first** statement inside the tx, mirrored by the same lock in `AssignSchema`. Trigger 2 (Milestone 2.2) is the DB-layer backstop.
- [x] Locate the existing `FieldExtractionService.CreateFieldExtraction` entry point (`internal/fieldextraction/service.go:27`) -- test: grep and read code
- [x] Add a new SQLC query `LockDocumentForLegacyExtractionWrite` (`SELECT id, custom_schema_id FROM documents WHERE id = @document_id FOR UPDATE`) in `internal/database/queries/document.sql` -- test: generated code compiles
- [x] Modify `CreateFieldExtraction`: after `Begin(ctx)` and **before** any other DML, call `LockDocumentForLegacyExtractionWrite`. The existing `AddFieldExtraction` call must move to **after** the lock acquisition. -- test: code review confirms no DML runs before the lock
- [x] If the lock query returns `pgx.ErrNoRows`, return a typed sentinel error `ErrDocumentNotFound` (controller maps to 404) -- test: TDD service test
- [x] If the locked row has `custom_schema_id IS NOT NULL`, return a typed sentinel error `ErrMutualExclusivityViolation` -- test: TDD service test asserts `errors.Is(err, ErrMutualExclusivityViolation)`
- [x] Service-layer concurrency test: two goroutines, one calls `AssignSchema` (which will itself take `FOR UPDATE` -- see 2.4) and the other calls `CreateFieldExtraction`. Run both orderings. Whichever commits second must fail -- `AssignSchema`-then-`CreateFieldExtraction` fails with `ErrMutualExclusivityViolation`; `CreateFieldExtraction`-then-`AssignSchema` fails with the Trigger 1 EXCEPTION converted to 409. Neither ordering may leave the document in the "both systems active" state. -- test: TDD with goroutines against testcontainer DB; assertion checks final DB state in addition to returned errors
- [x] Service-layer concurrency test (DB trigger backstop): deliberately skip the `FOR UPDATE` call path in a test-only harness, run the same race, and confirm Trigger 2 still catches it by surfacing a PostgreSQL `check_violation` error with SQLSTATE `23514`. This proves the defense-in-depth works even if the service-layer lock is ever removed by a regression. -- test: TDD
- [x] **Regression guard (v4 — not a present-day audit)**: at the time of this plan, `FieldExtractionService` has exactly one mutator — `CreateFieldExtraction` at `internal/fieldextraction/service.go:27`. The other four methods (`GetCurrentFieldExtraction`, `GetFieldExtractionHistory`, `GetFieldExtractionArrayFields`, `GetFieldExtractionByVersion`) are reads. v3 carried a generic "audit every other mutation entry point" task that pointed at methods (`UpdateFieldExtraction`, `AppendValueTo*`) that do not exist and have never existed. Instead of re-running a phantom audit, add a **regression test** that enforces the rule going forward: list the exported methods of `FieldExtractionService` via reflection (or, simpler, a hand-maintained allow-list), and fail the test if any new exported method with a mutating name prefix (`Create`, `Update`, `Append`, `Delete`, `Insert`, `Set`, `Reset`) is added without being added to a second allow-list that says "this method has been reviewed and it takes `LockDocumentForLegacyExtractionWrite` before any DML." -- test: unit test in `internal/fieldextraction/` that fails loudly with a message pointing at this tracking line if a new mutator is added without the review flag
- [x] Existing field extraction happy-path tests still green -- test: `go test ./internal/fieldextraction/...`
- [x] Update `CreateFieldExtraction` handler in `api/queryAPI/fieldextractions.go` to branch on `ErrMutualExclusivityViolation` -> `echo.NewHTTPError(http.StatusConflict, ...)` and `ErrDocumentNotFound` -> 404 -- test: handler unit tests assert both status codes and body messages
- [x] Handler also converts a PostgreSQL `check_violation` (SQLSTATE `23514`) surfaced from Trigger 2 into the same 409 response so a future bypass of the service-layer lock still returns the documented error shape -- test: TDD handler test with a fabricated `*pgconn.PgError`
- [x] Preserve the existing mapping of non-sentinel service errors to `500` -- test: TDD handler test with a generic error path
- [x] End-to-end HTTP test: create document, assign custom schema, call `POST /field-extractions`, assert response status is `409` and JSON body carries the documented message -- test: integration against testcontainer DB
- [x] End-to-end HTTP race test: fire `POST /super-admin/documents/{id}/schema` and `POST /field-extractions` concurrently against the same document, assert the final DB state has exactly one of `custom_schema_id IS NOT NULL` XOR a `documentFieldExtractionVersions` row, never both -- test: integration
### 2.6 OpenAPI spec - metadata + document binding
- [x] Add `CustomMetadataService` tag with description stating `client_user` + `user_admin` + `super_admin` access -- test: spec lints
- [x] `CustomMetadataRequest` (no `createdBy`) -- test: codegen
- [x] `CustomMetadataResponse` -- test: codegen
- [x] `CustomMetadataHistoryResponse` -- test: codegen
- [x] `ValidationErrorResponse` -- test: codegen
- [x] `DocumentSchemaAssignRequest` (no `createdBy`) -- test: codegen
- [x] `DocumentSchemaAssignResponse` -- test: codegen
- [x] **v4**: Modify `DocumentEnriched` to add **only** `customSchemaId` and `hasCustomMetadata`. **Do NOT** add `customSchemaName` or `customMetadata`. **Do NOT** add a new `?customMetadata=true` query parameter. -- test: codegen produces exactly two new fields
- [x] `PATCH /super-admin/documents/{id}/schema` path -- test: codegen
- [x] `GET /custom-metadata` path -- test: codegen
- [x] `POST /custom-metadata` path -- test: codegen
- [x] `GET /custom-metadata/version` path -- test: codegen
- [x] `GET /custom-metadata/history` path -- test: codegen
- [x] `task generate` clean -- test: no errors
### 2.7 Controllers - metadata + assign schema + document enrichment
- [x] Create `api/queryAPI/custommetadata.go` skeleton -- test: compiles
- [x] **v4 actor extraction**: `POST /custom-metadata` derives actor identity from the JWT and passes it to the service as an argument -- test: TDD handler test asserts body `createdBy` (if provided) does not override
- [x] `POST /custom-metadata` handler -- test: TDD
- [x] `GET /custom-metadata` handler -- test: TDD
- [x] `GET /custom-metadata/version` handler -- test: TDD
- [x] `GET /custom-metadata/history` handler -- test: TDD
- [x] Add `PATCH /super-admin/documents/{id}/schema` handler to `api/queryAPI/customschemas.go` -- derives actor -- test: TDD
- [x] Extend `DocumentEnriched` builder to include `customSchemaId` and `hasCustomMetadata` unconditionally — reads the values directly from the `document.DocumentEnriched` struct (populated in §2.3a); **no** new DB query or service call at this layer. -- test: TDD
- [x] Document without any custom schema returns `customSchemaId: null`, `hasCustomMetadata: false` -- test: TDD
- [x] Document with schema bound but no metadata returns `customSchemaId: <id>`, `hasCustomMetadata: false` -- test: TDD
- [x] Document with schema + metadata returns populated fields -- test: TDD
- [x] Authorization: confirm `GET /document/{id}` still uses the existing `document` resource, not `super-admin` -- test: role matrix test in 2.8
- [x] `go build ./...` clean -- test: compile
### 2.8 Milestone 2 integration tests
- [x] End-to-end: create schema -> assign to document (PATCH) -> POST valid metadata -> `GET /custom-metadata?documentId=...` returns the stored row with populated `schemaId`, `schemaName`, `schemaVersion` (from the §2.3 joined query) and the `createdBy` field equal to the JWT subject UUID -- test: integration
- [x] End-to-end: POST invalid metadata -> 400 with validation errors -- test: integration
- [x] End-to-end: POST metadata to document without assigned schema -> 400 -- test: integration
- [x] End-to-end: assign schema to document that has legacy extractions -> 409 -- test: integration
- [x] End-to-end: write legacy field extraction to document that has custom schema -> 409 -- test: integration
- [x] End-to-end `GET /custom-metadata/version`: POST metadata three times on the same document (version 1, 2, 3), then `GET /custom-metadata/version?documentId=...&version=2` returns exactly the v2 payload (not v1, not v3) with populated `schemaId`/`schemaName`/`schemaVersion` from the §2.3 joined query -- test: integration
- [x] End-to-end `GET /custom-metadata/version` not-found: `version=99` on a document with only 3 versions returns 404; `documentId` for a non-existent document returns 404; `version < 1` (e.g. `0`) returns 400 (plan Section 5.4 declares the parameter `>= 1`) -- test: integration per case
- [x] End-to-end `GET /custom-metadata/history` happy path: after three POSTs, `GET /custom-metadata/history?documentId=...` returns all three version summaries in descending `version` order and each summary carries `version`, `createdAt`, `createdBy` (subject UUID) -- test: integration
- [x] End-to-end `GET /custom-metadata/history` pagination: POST 5 metadata versions, call with `limit=2&offset=0` (returns v5, v4), `limit=2&offset=2` (returns v3, v2), `limit=2&offset=4` (returns v1) — assert no duplicates and correct ordering across the page boundary -- test: integration
- [x] `GET /custom-metadata/history` edge cases (v4 — behavior committed in plan Section 5.4 edge-case table, not up to the implementer): one integration test per case, each asserting the exact status code and response body declared in the plan —
- `limit=0` → 400 with message `"limit must be between 1 and 200"`
- `limit=-1` → 400 with the same message
- `limit=201` → 200, clamped to 200 rows (seed 205 versions, assert exactly 200 returned, newest first)
- `offset=-1` → 400 with message `"offset must be >= 0"`
- `documentId` = random non-existent UUID → 404 with message `"document not found"`
- `documentId` exists but has zero metadata rows → 200 with `{"versions": []}` (and `Content-Type: application/json`)
-- test: integration per case
- [x] Authorization matrix for Milestone 2 endpoints: one integration test per `(role, route, method)` cell across **all four** `/custom-metadata` routes (`GET`, `POST`, `GET /version`, `GET /history`) plus `PATCH /super-admin/documents/{id}/schema`. Expected matrix: `super_admin` positive on every cell; `user_admin` 403 on `PATCH /super-admin/...` and positive on all four `/custom-metadata` routes; `client_user` 403 on `PATCH /super-admin/...` and positive on all four `/custom-metadata` routes; `auditor` 403 on `PATCH /super-admin/...` and on `POST /custom-metadata`, positive on the three `/custom-metadata` GET routes (`GET`, `GET /version`, `GET /history`) -- test: one integration test per (role, route, method) cell, each asserting status + DB unchanged on denial
- [x] **v4 FK-cascade delete test**: create document with custom metadata -> call existing `DeleteDocumentCascade` -> verify `document_custom_metadata` rows for that document are gone (via the ON DELETE CASCADE, without touching the delete code path) -- test: integration
- [x] **v4 FK-cascade client delete test**: create client with schemas and a document with custom metadata -> call `client.HardDelete` -> verify `document_custom_metadata` and `client_metadata_schemas` rows are gone -- test: integration
- [x] Metadata payload at 1MB boundary: accept 1048576, reject 1048577 -- test: integration
- [x] Concurrency: two concurrent first-writers against same document succeed -- test: integration (service test in 2.4 covers it; re-verify at HTTP level)
- [x] `task fullsuite:ci` clean -- test: full run
- [x] Journal entry: Milestone 2 exit snapshot
### 2.9 Pre-partitioning carve-out for M3/M4 parallel execution
> **Purpose**: Milestones 3 and 4 both add handler code and service-layer types that were originally drafted into `api/queryAPI/customschemas.go` and `internal/customschema/models.go`. If both milestones run in parallel on separate branches against those two shared files, the merge at the end produces conflicts at the seam. This section creates four empty stub files so each milestone owns disjoint files from the start. **Do this once, at the end of Milestone 2, before branching M3 and M4 onto separate workstreams.** If M3 and M4 will be run serially, this section can be skipped (the handlers can land in `customschemas.go` directly); only commit the stubs if parallel execution is actually planned.
- [x] Create `api/queryAPI/customschemas_reset.go` as an empty stub containing only `package queryAPI` -- test: `go build ./...` clean
- [x] Create `api/queryAPI/customschemas_bulk.go` as an empty stub containing only `package queryAPI` -- test: `go build ./...` clean
- [x] Create `internal/customschema/models_reset.go` as an empty stub containing only `package customschema` -- test: `go build ./...` clean
- [x] Create `internal/customschema/models_bulk.go` as an empty stub containing only `package customschema` -- test: `go build ./...` clean
- [x] Commit the four stubs on the shared base branch (the branch M3 and M4 will both fork from), so neither workstream has to create them -- test: `git log` shows the four files as committed before either milestone branch exists
- [ ] Update Milestone 3 owner and Milestone 4 owner: M3 only edits `customschemas_reset.go` and `models_reset.go`; M4 only edits `customschemas_bulk.go` and `models_bulk.go`; neither touches `customschemas.go` or `models.go` for new types, only for imports and registration wiring if required -- test: code review of each milestone's final diff
---
## Milestone 3 - Administrative completion
**Goal**: Reset-metadata is exposed so super_admin can upgrade a document to a newer schema version. Schema version-creation concurrency is tested end-to-end. The full authorization matrix (including reset-metadata and version-creation) is locked in.
**Dependencies**: Milestone 2 complete.
**Exit criteria**:
- Full upgrade flow works: assign v1 -> write metadata -> create v2 -> reset -> assign v2 -> write new metadata.
- Reset is atomic, idempotent, rejects legacy documents, returns 404 on missing document.
- `task fullsuite:ci` green.
### 3.1 SQLC queries for reset-metadata
- [x] `LockDocumentForReset` (`SELECT id, "clientId", custom_schema_id FROM documents WHERE id = @document_id FOR UPDATE`) -- test: generated
- [x] `GetDocumentSchemaBindingForReset` (join `documents` -> `client_metadata_schemas`, returns `(custom_schema_id, schema_name, schema_version)` or all-null) -- test: generated
- [x] `CountDocumentCustomMetadataVersions` -- test: generated
- [x] **v4**: `DeleteDocumentCustomMetadataForReset` (`DELETE FROM document_custom_metadata WHERE document_id = @document_id`) -- owned by the reset path, name disambiguates from any future cascade query -- test: generated
- [x] **v4**: `NullifyDocumentCustomSchemaIdForReset` (`UPDATE documents SET custom_schema_id = NULL WHERE id = @document_id`) -- owned by the reset path -- test: generated
- [x] `task generate` clean -- test: idempotent
### 3.2 Service method - ResetDocumentMetadata
- [x] Add `ResetMetadataResult` type to `internal/customschema/models_reset.go` (the M3-owned stub from §2.9; fall back to `internal/customschema/models.go` only if M3/M4 will run serially and §2.9 was skipped) with `DocumentID`, `PreviousSchemaID *uuid.UUID`, `PreviousSchemaName *string`, `PreviousSchemaVersion *int`, `MetadataVersionsDeleted int`, `ResetAt time.Time`, `ResetBy string` -- test: compiles
- [x] `ResetDocumentMetadata(ctx, documentID, actor)` -- single transaction: `LockDocumentForReset`, `GetDocumentSchemaBindingForReset`, `CountDocumentCustomMetadataVersions`, `DeleteDocumentCustomMetadataForReset`, `NullifyDocumentCustomSchemaIdForReset`, audit log -- test: happy path wipe with schema bound + N metadata versions
- [x] Reset returns populated `previous*` fields when document had a schema -- test: TDD
- [x] Reset returns `metadataVersionsDeleted` matching actual deleted count -- test: TDD
- [x] Reset is idempotent: already-clean document returns `metadataVersionsDeleted: 0`, `previousSchemaId: nil` -- test: TDD
- [x] Reset rejects documents with legacy field extractions (409) -- test: TDD
- [x] Reset returns 404 on non-existent document -- test: TDD
- [x] Reset atomicity: inject failure between DELETE and UPDATE -- test: transaction rolls back, metadata rows still present
- [x] Reset + reassign + write: after reset, Trigger 1 still fires on second schema change under new metadata -- test: TDD regression
- [x] (v3's "Trigger 3 regression" test is removed in v4: the table has no `schema_id` column. Replace with a structural test that confirms `document_custom_metadata` has no `schema_id` column after migrations.) -- test: TDD schema-structure test
- [x] Reset writes `metadata.reset` audit log entry containing actor, documentID, previousSchemaId, metadataVersionsDeleted -- test: TDD reads the audit sink
- [x] Reset concurrency: two concurrent resets on same document serialize via `FOR UPDATE` -- test: TDD
- [x] Reset concurrency: reset vs `AssignSchema` on same document serialize -- test: TDD
- [x] **v4 Reset concurrency: reset vs `SetDocumentMetadata` on same document serialize.** This is the pair that matters most in practice: plan Section 7.1 / 8.2 say `LockDocumentForReset` and `LockDocumentForMetadataWrite` use the same parent-row `SELECT ... FOR UPDATE` shape specifically so these two operations cannot interleave. Test both orderings with goroutines against testcontainer DB:
1. **reset-then-write**: start reset (holds `FOR UPDATE`), race `SetDocumentMetadata` for the same document. The writer must block until reset commits; once it unblocks it must observe `custom_schema_id = NULL` and return the "no schema assigned" error (400/409), not a successful write into a stale schema. Assert final DB state has zero metadata rows for the document.
2. **write-then-reset**: start `SetDocumentMetadata` (holds `FOR UPDATE`, inserts a new metadata row), race reset for the same document. The reset must block until the write commits; once it unblocks it must delete the just-written row and return `metadataVersionsDeleted >= 1`. Assert final DB state has zero metadata rows and `custom_schema_id = NULL`.
Neither ordering may leave an orphaned metadata row or a partially-reset document. -- test: TDD with goroutines against testcontainer DB; assertions cover both ordering outcomes
### 3.3 OpenAPI + handler - reset-metadata
- [x] `DocumentMetadataResetResponse` schema (no request body schema needed; empty body) -- test: codegen (NOTE: named `DocumentMetadataResetResponse` not `ResetDocumentMetadataResponse` to avoid codegen name conflict; see journal 2026-04-15)
- [x] `POST /super-admin/documents/{id}/reset-metadata` path -- test: codegen
- [x] `task generate` clean -- test: no errors
- [x] Add handler to `api/queryAPI/customschemas_reset.go` (the M3-owned stub from §2.9; fall back to `api/queryAPI/customschemas.go` only if M3/M4 will run serially and §2.9 was skipped) -- parses document ID, derives actor from JWT, calls service, maps errors -- test: TDD
- [x] Handler maps success to 200 with `ResetDocumentMetadataResponse` -- test: `TestM3_ResetDocumentMetadata_HappyPath` + `TestM3_ResetDocumentMetadata_IdempotentOnCleanDoc`
- [x] Handler maps "document not found" to 404 -- test: `TestM3_ResetDocumentMetadata_DocumentNotFound` + `TestMapResetError/document_not_found`
- [x] Handler maps "legacy extractions exist" to 409 -- test: `TestM3_ResetDocumentMetadata_LegacyExtractionsConflict` + `TestMapResetError/mutual_exclusivity_violation`
- [x] Handler maps unexpected DB errors to 500 -- test: `TestMapResetError/unknown_error_returns_500`
### 3.4 Milestone 3 integration tests
- [x] End-to-end upgrade flow: bind schema v1 -> POST metadata -> `POST .../versions` to create v2 -> reset-metadata -> PATCH schema to v2 -> POST new metadata with added optional field -- test: `TestM3_FullUpgradeFlow` in `api/queryAPI/customschemas_reset_test.go`
- [x] End-to-end: reset on already-clean document returns 200 with `metadataVersionsDeleted: 0` -- test: `TestM3_ResetDocumentMetadata_IdempotentOnCleanDoc`
- [x] Schema version concurrency: two concurrent `POST .../versions` calls for same `(client_id, name)` produce distinct versions -- test: `TestM3_SchemaVersionConcurrency` in `api/queryAPI/customschemas_integration_test.go`
- [x] Authorization matrix for reset-metadata and version creation: `super_admin` positive; `user_admin` 403; `client_user` 403; `auditor` 403 -- test: `TestM3_AuthMatrix_Reset` in `api/queryAPI/customschemas_reset_test.go` (4 subtests: super_admin/user_admin/client_user/auditor)
- [x] `task fullsuite:ci` clean -- test: full run (85.4%, All coverage checks passed!, no FAIL lines, 2026-04-15)
- [x] Journal entry: Milestone 3 exit snapshot (2026-04-15)
---
## Milestone 4 - Bulk operations
**Goal**: Bulk folder schema assignment is available for administrative convenience.
**Dependencies**: Milestone 2 complete. Milestone 4 only needs the single-document `AssignSchema` semantics (committed in Milestone 2), not the `ResetDocumentMetadata` work in Milestone 3 — nothing in the recursive folder walk reads or writes the reset path. Milestones 3 and 4 **can run in parallel on separate workstreams only after the shared files are pre-partitioned**: without that step, both milestones would edit `api/queryAPI/customschemas.go` and `internal/customschema/models.go` concurrently and produce merge conflicts at the seam. The pre-partition carve-out (see section 2.9 below) creates four new stub files so each milestone owns disjoint files: M3 owns `api/queryAPI/customschemas_reset.go` and `internal/customschema/models_reset.go`; M4 owns `api/queryAPI/customschemas_bulk.go` and `internal/customschema/models_bulk.go`. With the carve-out in place the merge gate is `task fullsuite:ci` green after both land, and serializing M4 behind M3 adds roughly one milestone of wall-clock time for no technical reason. Without the carve-out, serialize M4 behind M3 as the safe fallback.
**Exit criteria**:
- Recursive walk covers a 3-deep folder tree correctly, handles all skip reasons, enforces the 10K document cap, runs in a single transaction.
- `task fullsuite:ci` green.
### 4.1 SQLC queries for bulk folder assignment
- [ ] `BulkSetDocumentCustomSchemaIdInFolderTree` (`WITH RECURSIVE`) -- test: generated; recursive CTE compiles
- [ ] `GetDocumentsWithMetadataInFolderTree` -- test: generated
- [ ] `GetDocumentsWithLegacyExtractionsInFolderTree` -- test: generated
- [ ] **Reuse existing query**: `CountDocumentsInFolderTree` already exists in `internal/database/queries/folders.sql` (a recursive CTE over `folders` + `documents.folderId`) and is exactly the shape the `MaxBulkAssignDocuments` pre-check needs. Do **not** add a new query; call the existing one from the Milestone 4 service method. Task: unit test that confirms the bulk service's cap check is wired to the existing `CountDocumentsInFolderTree` generated method -- test: unit test asserts the right method is called and returns a count that matches a seeded fixture
- [ ] `task generate` clean -- test: idempotent
### 4.2 Service method - AssignSchemaToFolder
- [ ] Add `BulkAssignResult`, `SkippedDocument` types to `internal/customschema/models_bulk.go` (the M4-owned stub from §2.9; fall back to `internal/customschema/models.go` only if M3/M4 will run serially and §2.9 was skipped) -- test: compiles. **No `createdBy` on the input.**
- [ ] `AssignSchemaToFolder(ctx, folderID, schemaID, actor)` -- walks subtree with `WITH RECURSIVE` and returns `BulkAssignResult` -- test: happy path on a 3-deep tree
- [ ] Bulk assignment skips documents with different schema + existing metadata with `SkippedDocument` reason -- test: TDD
- [ ] Bulk assignment skips documents with legacy extractions with reason -- test: TDD
- [ ] Bulk assignment no-ops documents already on target schema with reason -- test: TDD
- [ ] Bulk assignment reassigns documents with different schema but no metadata -- test: TDD
- [ ] Bulk assignment returns 422 when subtree exceeds `MaxBulkAssignDocuments` -- test: TDD (constant override for test)
- [ ] Bulk assignment runs in single transaction; simulated mid-walk failure rolls back everything -- test: TDD
- [ ] Bulk assignment writes `folder.assign_schema` audit entry with counts -- test: TDD
### 4.3 OpenAPI + handler - bulk folder assignment
- [ ] `BulkSchemaAssignRequest` schema (no `createdBy`) -- test: codegen
- [ ] `BulkSchemaAssignResponse` schema -- test: codegen
- [ ] `POST /super-admin/folders/{folderId}/assign-schema` path -- test: codegen
- [ ] `task generate` clean -- test: no errors
- [ ] Handler in `api/queryAPI/customschemas_bulk.go` (the M4-owned stub from §2.9; fall back to `api/queryAPI/customschemas.go` only if M3/M4 will run serially and §2.9 was skipped) wired to `AssignSchemaToFolder` -- derives actor from JWT -- test: compiles
### 4.4 Milestone 4 integration tests
- [ ] Recursive walk on a 3-deep folder tree produces correct `documentsUpdated` count -- test: integration
- [ ] Skip reason: "different schema + existing metadata" -- test: integration
- [ ] Skip reason: "already assigned to same schema (no-op)" -- test: integration
- [ ] Skip reason: "has legacy field extractions" -- test: integration
- [ ] Reassignment on document with different schema but no metadata -- test: integration
- [ ] Subtree > 10K documents returns 422 with documented message -- test: integration (constant override for test)
- [ ] Bulk folder exactly 10000 docs: succeeds -- test: integration
- [ ] Empty folder returns 200 with 0 counts -- test: integration
- [ ] Deeply nested (10+ levels) tree walks correctly -- test: integration
- [ ] Authorization matrix for bulk folder endpoint: `super_admin` positive; `user_admin` 403; `client_user` 403; `auditor` 403 -- test: integration per cell
- [ ] `task fullsuite:ci` clean -- test: full run
- [ ] Journal entry: Milestone 4 exit snapshot
---
## Documentation (parallel with Milestones 3 + 4)
**Goal**: Every doc that references endpoints, env vars, authorization, or the feature is updated.
**Dependencies**: Milestone 2 complete (feature is shippable in narrow form); documentation can track Milestones 3 and 4 in real time.
- [x] Update `docs/ai.generated/README.md` index if new doc files added -- test: review (entry 11 added pointing to `11-custom-metadata-guide.md`)
- [x] Authorization matrix table published in a dedicated doc page (include the v4 change that `user_admin` has no schema CRUD) -- test: review (`11-custom-metadata-guide.md` Authorization Matrix section)
- [x] Sample schemas from Appendix A of the plan published as copyable examples -- test: review (`11-custom-metadata-guide.md` Schema Definition Requirements section, two examples)
- [x] `CustomMetadataService` usage guide (end-user perspective) -- test: review (`11-custom-metadata-guide.md` End-User Workflow section + `03-api-documentation.md` CustomMetadataService section)
- [x] `SuperAdminSchemaService` usage guide (platform-operator perspective), including the `POST /versions` route -- test: review (`11-custom-metadata-guide.md` Platform Operator Workflow + `03-api-documentation.md` SuperAdminSchemaService section)
- [x] Reset-metadata runbook: "How to upgrade a document to a newer schema version" with the full wipe -> reassign -> re-POST flow and a warning about metadata history loss -- test: review (`11-custom-metadata-guide.md` Schema Upgrade Runbook section)
- [x] Mutual exclusivity explanation (legacy vs custom) in the document processing guide -- test: review (`11-custom-metadata-guide.md` Mutual Exclusivity section)
- [x] **v4**: Note that `DeleteDocumentCascade` and `client.HardDelete` do NOT need custom-metadata-specific steps because of the FK cascades -- test: review (`11-custom-metadata-guide.md` Cascade Deletion Behavior section)
- [x] Permit.io deployment ordering note (run setup tool before deploying controllers) added to the deployment runbook -- test: review (`11-custom-metadata-guide.md` Permit.io Deployment Note section)
- [x] **v4**: Document the HTTP method choice: `POST /super-admin/custom-schemas/{schemaId}/versions`, not `PUT` -- test: review (`03-api-documentation.md` and `11-custom-metadata-guide.md` Schema Versioning section)
- [x] **v4**: Document that `createdBy` is not accepted from request bodies; actor comes from the JWT -- test: review (`03-api-documentation.md` Notes on POST /super-admin/custom-schemas; `11-custom-metadata-guide.md` Actor Fields section)
- [x] Any new env vars? (none expected for v4; confirm.) -- test: review (confirmed: no new env vars in v4)
- [ ] Generate new API status report / architecture diagrams if the pipeline regenerates them -- test: review
- [ ] Journal entry marking the feature shipped
---
## Cross-milestone reminders
- Before starting any milestone, re-read the relevant section of `plans/mutable.metadata.plan.combo.v4.md`.
- TDD is mandatory per project rules: write the test, watch it fail, make it pass. Never write multiple tests before running any.
- No mocks without Q's explicit permission. Use testcontainers for real Postgres.
- Keep the journal current. When a checkbox flips to `[x]`, add a timestamped line to `./journals/implement_mutableMetadata.md` with the commit SHA and test command that proved it.
- If any migration, trigger, FK, or service invariant seems to conflict with the plan, STOP and ask Q before modifying it. The defense-in-depth design depends on the composite FK and Trigger 1 being exactly as written.
- **v4 reminder — things NOT to add (they are v3 leftovers and mean you are regressing):**
- Do NOT add a `schema_id` column to `document_custom_metadata`.
- Do NOT add v3's `trg_validate_schema_client_match` trigger — it is replaced by the composite FK `fk_documents_custom_schema_same_client` (plan Section 4.7, Milestone 1.3).
- Do NOT add v3's `trg_enforce_consistent_schema_id` trigger — it is structurally unnecessary because `document_custom_metadata` has no `schema_id` column in v4.
- **Reminder**: v4 DOES add two triggers — `trg_prevent_schema_reassignment` (v3's Trigger 1, renamed for clarity) and `trg_prevent_legacy_extraction_on_custom_document` (new in v4, see Section 2.2). Those are **correct** and must be present.
- Do NOT add a `current_document_custom_metadata` view.
- Do NOT add `DeleteDocumentCustomMetadata` / `NullifyDocumentCustomSchemaId` / `DeleteClientMetadataSchemas` to the existing delete code paths.
- Do NOT add a `PUT /super-admin/custom-schemas/{schemaId}` route.
- Do NOT accept `createdBy` in new request bodies.
- Do NOT add `customSchemaName` or `customMetadata` to `GET /document/{id}`.
- Do NOT declare `format: email` on `createdBy` / `resetBy` in the new OpenAPI response schemas — those are plain-string Cognito subject IDs (plan Section 8.3 "Actor fields").
- If any of those appear in your diff, something has regressed to v3.
plans/mutable.metadata.requirments.md
-30
View File
@@ -1,30 +0,0 @@
# Persona
You are an expert db architect and software engineer.
# Context
We are going to extend the schema for the text extraction records in this application that we currently expose so that each client can have their own collections of document schemas instead of everyone being tied into the same static 120 or so field extraction fields.
For example. Say client A (Acme corp) has 200 custom fields that they want to associate with documents that are in the `aircraft engineering` class of document. Each field has a name and data type plus constraints like max length or precision for floating point values. Then acme might also have 100 custom fields that it needs to track for documents that are for `auto engineering` documents.
# Requirements
A SUPER ADMIN should be able to curate ALL collectionions of schemas for their documents. (for field extraction records on their documents) (FieldExtractionService)
So the client should be able to submit a new schema (which will get assigned an id by the system) The system will automatically reject the schema if it does not conform to jsonschema or whatever we are using for the schema. (json schema is assumed unless we come up with something better)
Then once a schema is created they should be able to assign a document to use that schema id. Then when they set the text extraction record for that document the metadata submitted must conform to the schema that the document is currently using. A document's schema id must remain the same for the lifetime of the document onse text extraction data (metadata) has been assigned.
There can be a way to reset and clear the metadata on a document if we need to assigned an new schema and metadata.
Only the super admin should be able to list the schemas they have registered and curate them. (delete ones that are not used) or update ones that need changes. Note that when a schema is updated it will be assigned a new schema id.
document metadata needs to be extended so that each document has an optional (can be nill) custom schema id.
# Task
You will thoroughly study the current API design (code and docs) expecially the FieldExtractionService
to come up with a suggested design for how the new API should look and how the new DB scheam that holds all of this metadata (and now custom schemas) will be structured. It is acceptable for a document to store custom metadata in json blobs in the database if that simplifies the organization.
You will write the new design plan to (plan path here)
You will not change any code in this repository or on this server as part of your research.
This is analysis and design only. and the only file you will write is the plan specified above.
When you are done with the design and the plan is written to disk I will review.
plans/mutable.metadata.review.combo.v3.codex.md
-86
View File
@@ -1,86 +0,0 @@
# Mutable Metadata Plan v3 Review (Codex)
A review of plans/mutable.metadata.plan.combo.v3.md and plans/mutable.metadata.plan.combo.v3.tracking.md since they are to be used as a pair to implement the plans/mutable.metadata.plan.combo.v3.md plan.
I reviewed the current legacy field-extraction flow first so the plan review is anchored to real behavior:
- `api/queryAPI/fieldextractions.go:20-69` binds the request, calls `FieldExtraction.CreateFieldExtraction`, and currently maps any create error to HTTP 500.
- `internal/fieldextraction/service.go:27-100` creates the extraction row, then locks existing version rows, computes `max(version)+1`, and inserts the version row.
- Authorization today is checked as `(user, action, first-path-segment resource)` with no document/client object passed into Permit.io: `internal/cognitoauth/middleware.go:84-110`, `internal/cognitoauth/permitio.go:79-89`.
## Findings
1. **Blocking: the tracking doc’s migration numbers collide with live migrations already in the repo.**
References:
- `plans/mutable.metadata.plan.combo.v3.tracking.md:48-75`
- `internal/database/migrations`: existing `00000000000120_*` through `00000000000123_*` are already present (`00000000000120_remove_text_extraction`, `00000000000121_create_eula_tables`, `00000000000122_make_eula_effective_date_nullable`, `00000000000123_create_ui_settings`)
Impact:
- Phase 1 cannot start as written.
- Any implementation that follows the tracking doc literally will create filename collisions and broken migration ordering.
Recommendation:
- Renumber all new mutable-metadata migrations to the next free sequence and update every tracking reference before coding.
2. **Blocking: the plan grants `/custom-metadata` read/write to `client_user`, but the current auth model is not tenant-scoped.**
References:
- Plan grants: `plans/mutable.metadata.plan.combo.v3.md:730-780`
- Current auth check: `internal/cognitoauth/middleware.go:105-110`, `internal/cognitoauth/permitio.go:79-89`
- Current documented limitation: `cmd/auth_related/permit.setup/permit_policies.yaml:155-184`
Impact:
- `custom-metadata:get/post` would be authorized purely by resource/action on the first path segment.
- Without new application-level document/client ownership checks, a `client_user` or `user_admin` who knows another document UUID can read/write that document’s custom metadata across clients.
Recommendation:
- Resolve scoping before implementation. Either:
- Add explicit document/client ownership enforcement in the custom-metadata service/controller and track it as first-class work, or
- Do not grant `client_user`/`user_admin` write access to `/custom-metadata` until tenant scoping exists.
3. **High: Phase 5a in the tracking doc misses the HTTP-layer work needed to return the documented 409.**
References:
- Tracking: `plans/mutable.metadata.plan.combo.v3.tracking.md:290-299`
- Current controller behavior: `api/queryAPI/fieldextractions.go:45-50`
Impact:
- Adding only a service pre-check will not produce the promised HTTP 409.
- As written, `POST /field-extractions` will still return 500 for the new mutual-exclusivity error.
Recommendation:
- Add explicit controller error mapping and HTTP tests for the legacy write guard, not just service tests.
4. **High: the reset-metadata section is internally inconsistent about documents that already have legacy field extractions.**
References:
- `plans/mutable.metadata.plan.combo.v3.md:643-653`
Impact:
- The behavior bullets say a legacy-extraction document “succeeds trivially.”
- The validation section says the same condition returns `409 Conflict`.
- The tracking doc appears to assume `409`, so the plan and tracker are not actually aligned.
Recommendation:
- Pick one behavior and update both docs. I recommend `409`, because it matches the mutual-exclusivity rule and avoids false-success responses on legacy documents.
5. **High: the custom-metadata versioning plan overstates its concurrency safety, especially on first write.**
References:
- Plan claim: `plans/mutable.metadata.plan.combo.v3.md:152-153`
- Tracking task: `plans/mutable.metadata.plan.combo.v3.tracking.md:239-245`
- Current legacy pattern: `internal/fieldextraction/service.go:70-100`
Impact:
- `SELECT ... FOR UPDATE` on the max-version row does not serialize the first write for a document, because there is no row to lock yet.
- Two concurrent first writes can still race and one will fail on `UNIQUE (document_id, version)`.
Recommendation:
- Decide on a real serialization strategy now:
- Lock the document row before version assignment, or
- Retry on unique-violation explicitly.
- Add a “concurrent first write” test to the tracker, not just a generic concurrency test.
6. **Medium: actor/audit provenance is underspecified and currently inconsistent with the plan’s audit goals.**
References:
- Request bodies still carry caller-supplied `createdBy`: `plans/mutable.metadata.plan.combo.v3.md:566-571`, `847-858`
- Tracking only explicitly extracts caller identity for reset: `plans/mutable.metadata.plan.combo.v3.tracking.md:431-447`, `502`
- Reset refers to a not-yet-defined “same log sink used by `AssignSchema` / `DeleteSchema`”: `plans/mutable.metadata.plan.combo.v3.md:638`, `1069`
Impact:
- If `created_by` is meant to be audit-authoritative, client-supplied JSON is not trustworthy.
- The tracker does not currently include the work to derive actor identity from auth context for schema create/update/delete, folder assign, or metadata writes.
- The audit sink itself is assumed rather than designed.
Recommendation:
- Decide now whether actor identity is authoritative or merely display metadata.
- If authoritative, derive it from auth context everywhere and add explicit tracking tasks.
- Define the concrete audit sink/API instead of leaving it implied.
## Bottom Line
I would not start implementation until Findings 1 and 2 are resolved. Findings 3 through 6 should be folded into the tracking doc before work starts, otherwise the implementation will either drift from the plan or discover these gaps mid-phase.
plans/mutable.metadata.simplify. plan.codex.v3.md
-544
View File
@@ -1,544 +0,0 @@
# Mutable Metadata v3 Simplification Findings (Codex)
## Purpose
This document reviews `plans/mutable.metadata.plan.combo.v3.md` and `plans/mutable.metadata.plan.combo.v3.tracking.md` with one goal: preserve the real requirements while reducing implementation risk, regression surface, and rollout complexity.
The current v3 plan is trying to land too many things at once:
- a new schema system
- a new metadata storage path
- new admin routes
- new user routes
- three database triggers
- document read-path changes
- delete-path changes
- bulk folder assignment
- destructive schema-reset workflow
- a large auth and test matrix
The core requirement set is smaller than that. The simplest safe plan is to reduce redundant state, avoid touching stable paths until necessary, and ship the feature in vertical slices instead of infrastructure layers.
---
## Executive Summary
The biggest simplification is this:
1. Keep `documents.custom_schema_id`.
2. Store versioned metadata by `document_id` only.
3. Derive the schema from the document binding at write/read time.
4. Keep only the invariants that are truly needed for v3.
5. Defer optional surface area until the core path is stable.
That change alone removes one trigger, one FK, one index, one view, several queries, several tests, and a large amount of reasoning complexity.
The second major simplification is rollout shape: stop treating this as twelve horizontal phases. Implement it as a few end-to-end vertical milestones so the system is usable and testable much earlier.
---
## What Should Stay
These are the actual requirements and should remain intact:
- client-scoped custom schemas stored as data, not DDL
- schema definitions validated as JSON Schema
- documents gain an optional `custom_schema_id`
- metadata writes must validate against the document's assigned schema
- once custom metadata exists, the document's schema binding becomes immutable
- legacy field extractions and custom metadata remain mutually exclusive per document
- schema updates create a new schema version/new schema ID
- schema administration remains `super_admin`-only
Everything else should be judged by whether it is necessary to satisfy those requirements.
---
## Recommended Simplifications
### 1. Remove `schema_id` from `document_custom_metadata`
This is the highest-value simplification.
The current plan stores both:
- `documents.custom_schema_id`
- `document_custom_metadata.schema_id`
That is redundant in v3.
Why it is redundant:
- `POST /custom-metadata` already derives schema from `documents.custom_schema_id`.
- the plan already forbids schema reassignment after metadata exists
- reset-metadata deletes all metadata rows before rebinding to a different schema
Because of those three facts, every surviving metadata row for a document necessarily belongs to the document's current schema binding. A second schema pointer on the metadata row adds no new capability for v3.
### Simpler table
```sql
CREATE TABLE document_custom_metadata (
id uuid PRIMARY KEY DEFAULT uuid_generate_v7(),
document_id uuid NOT NULL REFERENCES documents(id) ON DELETE CASCADE,
metadata jsonb NOT NULL,
version int NOT NULL DEFAULT 1,
created_at timestamptz NOT NULL DEFAULT NOW(),
created_by varchar(255) NOT NULL,
CONSTRAINT uq_doc_custom_metadata_version UNIQUE (document_id, version)
);
CREATE INDEX idx_dcm_doc_version_desc
ON document_custom_metadata(document_id, version DESC);
```
### What this removes
- `document_custom_metadata.schema_id`
- `idx_dcm_schema_id`
- Trigger 3 (`trg_enforce_consistent_schema_id`)
- the `GetSchemaMetadataRecordCount` query
- the extra "schema consistency across metadata versions" app-layer checks
- the need to reason about metadata rows carrying a schema separate from the document row
### Why this does not violate requirements
It still preserves:
- schema validation on write
- immutable schema binding after first metadata write
- destructive reset before rebinding
- schema version history at the schema table level
- metadata version history at the document level
Do not pre-pay v4 complexity for a non-requirement. If a future version needs non-destructive in-place schema migration, then add row-level `schema_id` later with a deliberate migration.
---
### 2. Replace Trigger 2 with a composite foreign key
The current plan uses a trigger to ensure the schema belongs to the same client as the document.
That is better expressed as a database constraint.
### Simpler constraint
Keep:
- `client_metadata_schemas(id, client_id)`
- `documents(id, clientId, custom_schema_id)`
Add:
```sql
ALTER TABLE client_metadata_schemas
ADD CONSTRAINT uq_cms_id_client UNIQUE (id, client_id);
ALTER TABLE documents
ADD CONSTRAINT fk_documents_custom_schema_same_client
FOREIGN KEY (custom_schema_id, "clientId")
REFERENCES client_metadata_schemas(id, client_id);
```
### Why this is better
- declarative integrity instead of PL/pgSQL
- no trigger code
- no trigger-specific unit tests
- no divergence risk between service logic and DB logic
With recommendation 1 applied, the only remaining trigger worth keeping is Trigger 1: "once metadata or legacy extraction rows exist, `custom_schema_id` cannot change."
That shrinks the trigger surface from three triggers to one.
---
### 3. Drop the `current_document_custom_metadata` view
The new metadata table is simple enough that a view is unnecessary.
Use:
```sql
SELECT ...
FROM document_custom_metadata
WHERE document_id = $1
ORDER BY version DESC
LIMIT 1;
```
With `idx_dcm_doc_version_desc`, this is straightforward and efficient.
### Why remove the view
- one less schema object
- one less migration artifact
- one less thing to test
- less drift between query logic and view logic
The legacy field-extraction system benefits from a view because its state is spread across multiple tables. The new custom-metadata design does not need the same indirection.
---
### 4. Use FK cascades so delete code does not need feature-specific edits
The current plan adds delete-path work in `internal/document/delete.go` and `internal/client/delete.go`.
That is avoidable.
Because these are new tables, design their FKs so existing delete flows continue to work without feature-specific code.
### Recommended FK behavior
- `document_custom_metadata.document_id -> documents.id ON DELETE CASCADE`
- `client_metadata_schemas.client_id -> clients.clientId ON DELETE CASCADE`
### Result
- document hard-delete automatically removes custom metadata rows
- client hard-delete automatically removes client schema rows after client deletion
- `DeleteDocumentCascade` does not need custom-metadata-specific steps
- `client.HardDelete` does not need `DeleteClientMetadataSchemas`
### Important correction
`documents.custom_schema_id` does **not** need to be nulled before deleting the document row.
The current plan says that delete must null `custom_schema_id` first because the FK to `client_metadata_schemas` would otherwise block the delete. That is incorrect. Deleting the document removes the referencing row; it does not require clearing the reference first.
Keep `NullifyDocumentCustomSchemaId` only for the reset-metadata flow, where the document row survives.
This is a meaningful regression-risk reduction because it avoids touching existing explicit delete code paths at all.
---
### 5. Remove convenience-only fields from the first release
The current plan adds several useful but non-essential response decorations:
- `documentCount`
- `canDelete`
- `previousVersionId`
- `customSchemaName` on unrelated responses
- embedded `customMetadata` inside `GET /document/{id}`
These fields are not required to satisfy the business rules.
### Simpler rule
For the first release:
- schema list returns basic schema summary only
- delete attempts return `409` if the schema is still in use
- full metadata stays on `/custom-metadata`, not `GET /document/{id}`
If usage hints are still wanted later, add them as a dedicated opt-in field or `includeUsage=true` expansion after the core system is stable.
---
### 6. Do not modify `GET /document/{id}` in the core milestone
The current code in `internal/document/get.go` and `api/queryAPI/documents.go` is intentionally narrow: it composes document basics, labels, and optionally legacy text-record data.
Do not expand that path in the first pass.
Reasons:
- it is an existing stable read path
- the feature already has dedicated metadata endpoints
- adding full metadata enrichment multiplies controller, service, OpenAPI, and test work
- it increases regression risk in a widely used endpoint
### Recommendation
For the core release:
- leave `GET /document/{id}` unchanged
Optional later milestone:
- add `customSchemaId`
- add `hasCustomMetadata`
- only add full `customMetadata` if there is a concrete consumer that needs it
This keeps the first delivery focused on the new feature itself instead of modifying a stable cross-cutting read path.
---
### 7. Defer bulk folder assignment until after the core feature is stable
`POST /super-admin/folders/{folderId}/assign-schema` adds a large amount of complexity:
- recursive tree traversal
- large-transaction behavior
- 10K-document guardrail
- skip-reason reporting
- mixed-state subtree handling
- many more integration tests
It is not required to prove the mutable-metadata model.
### Recommendation
Move bulk folder assignment out of the core implementation and into a later milestone.
Core assignment should be:
- single-document only
Then, if bulk assignment is still wanted after the single-document path is stable, add it separately with real operational evidence about folder sizes and expected admin workflows.
This is one of the easiest ways to cut failure risk without weakening the main feature.
---
### 8. Defer reset-metadata to a later additive milestone
The reset endpoint is useful, but it is not part of the minimum path needed to deliver custom schemas and validated custom metadata.
It is also currently coupled to unrelated work in the tracker through delete-cascade query reuse.
### Recommendation
Treat reset-metadata as an additive milestone after the core path is working:
- schema create/list/get
- single-document assign
- metadata write/read current
- legacy guard
- schema update/version
Only then add:
- `POST /super-admin/documents/{id}/reset-metadata`
If Q wants reset kept within the same overall initiative, that is still fine. The key simplification is sequencing: do not let reset block the first stable end-to-end slice.
Also, once recommendation 4 is applied, reset no longer needs to depend on delete-path work. It stands on its own.
---
### 9. Remove `createdBy` from all new request bodies
The plan already says actor identity must come from auth context, not the request body.
Because these are new endpoints, there is no backward-compatibility reason to keep misleading `createdBy` request fields in the contract.
### Recommendation
For all new mutable-metadata endpoints:
- do not accept `createdBy` in the request schema
- derive actor from JWT / auth middleware
- return audit actor in responses if needed
This simplifies:
- OpenAPI
- handler code
- audit rules
- tests
- client expectations
The only place `createdBy` should remain is existing legacy endpoints that already use it today.
---
### 10. Replace schema-version `PUT` with an explicit version-creation `POST`
The current plan uses:
- `PUT /super-admin/custom-schemas/{schemaId}`
But that endpoint does not replace the resource in place. It creates a new schema row with a new ID and supersedes the old one.
That is creation semantics, not update-in-place semantics.
### Simpler route
Use:
- `POST /super-admin/custom-schemas/{schemaId}/versions`
### Why this is better
- matches actual behavior
- avoids the Permit `PUT` action-mapping question entirely
- reduces auth configuration ambiguity
- makes code review and API docs clearer
This preserves the requirement that schema updates create a new ID, while making the API easier to reason about.
---
## Revised Minimal Design
If I were simplifying the v3 design directly, I would target this shape:
### Database
- `client_metadata_schemas`
- keep immutable schema rows
- keep version numbers
- keep lifecycle state if desired
- `documents.custom_schema_id`
- add optional binding
- enforce same-client binding with composite FK
- `document_custom_metadata`
- `document_id`, `metadata`, `version`, audit fields
- no `schema_id`
- `ON DELETE CASCADE`
- DB trigger surface:
- keep only the "schema cannot change after metadata or legacy extraction exists" trigger
### API in the first stable slice
- `POST /super-admin/custom-schemas`
- `GET /super-admin/custom-schemas`
- `GET /super-admin/custom-schemas/{schemaId}`
- `PATCH /super-admin/documents/{id}/schema`
- `POST /custom-metadata`
- `GET /custom-metadata`
### API deferred to later slices
- schema version creation
- schema retirement
- metadata history/version endpoints
- reset-metadata
- bulk folder assignment
- `GET /document/{id}` enrichment
This still reaches a real, usable feature early.
---
## Revised Implementation Milestones
The current tracking doc is organized as horizontal layers. That is the wrong shape for a risky feature. It encourages long periods where nothing is fully integrated.
Use vertical slices instead.
### Milestone 1: Schema CRUD foundation
Goal: prove schema storage and validation end to end.
Includes:
- migrations for `client_metadata_schemas`
- `documents.custom_schema_id`
- composite FK for same-client binding
- schema validator
- `POST/GET/LIST /super-admin/custom-schemas`
- Permit `super-admin` resource update
Exit proof:
- super_admin can create and list schemas
- invalid schema rejected
- cross-client document binding cannot be represented in DB
### Milestone 2: Core document flow
Goal: prove the actual business workflow end to end.
Includes:
- `document_custom_metadata` table without `schema_id`
- single-document schema assignment
- `POST /custom-metadata`
- `GET /custom-metadata`
- legacy field-extraction guard plus controller `409` mapping
Exit proof:
1. create schema
2. assign schema to document
3. write valid metadata
4. reject invalid metadata
5. reject legacy field extraction write afterward
At this point the feature already satisfies the central requirement.
### Milestone 3: Administrative completion
Goal: complete the admin lifecycle without expanding into optional convenience features.
Includes:
- schema version creation endpoint
- schema retirement/delete-if-unused behavior
- optional metadata history endpoints if they are required immediately
Exit proof:
- create v1
- create v2
- v1 no longer assignable
- v2 assignable
- in-use schema cannot be retired/deleted incorrectly
### Milestone 4: Additive extras
Only add these after Milestones 1-3 are green:
- reset-metadata
- bulk folder assignment
- `GET /document/{id}` enrichment
- usage/count decorations in schema list responses
These should not block the core mutable-metadata rollout.
---
## How the Tracking Doc Should Change
The tracking doc should be simplified in these specific ways:
### Remove whole workstreams from the core path
- remove Phase 5b if FK cascades are used correctly
- move Phase 9 behind the core milestones
- move Phase 10 behind the core milestones
- move Phase 10b behind the core milestones
### Collapse redundant DB work
- remove view-related tasks
- remove Trigger 3 tasks
- remove `schema_id`-related metadata tasks
- remove `GetSchemaMetadataRecordCount` tasks
- remove document-delete nullification tasks
### Change phase ordering
Do not wait until Phase 8 for the first real end-to-end flow.
Instead, each milestone should contain:
- migration/query work
- service work
- controller work
- at least one end-to-end integration test
That is the right unit of verification for a change this invasive.
---
## Bottom Line
The current v3 plan is trying to solve the core problem plus several convenience and escape-hatch problems at the same time. That is where most of the risk is coming from.
The safest simplification is:
- remove redundant metadata-side schema state
- replace trigger logic with declarative constraints where possible
- let new FKs handle delete cleanup
- keep stable document read/delete paths unchanged in the first pass
- ship the feature in vertical slices
- defer bulk assignment, reset, and read-path enrichment until after the core path is proven
If Q wants the shortest version of this advice:
**Make the document row the single source of truth for schema binding, make the metadata table just versioned JSON for that document, and do not ship bulk/reset/document-enrichment in the first milestone.**
plans/mutable.metadata.testing.review.codex.md
-79
View File
@@ -1,79 +0,0 @@
# Mutable Metadata v4 Testing Review
Reviewed:
- `plans/mutable.metadata.plan.combo.v4.md`
- `plans/mutable.metadata.plan.combo.v4.tracking.md`
## Findings
1. High: `GET /custom-metadata/version` and `GET /custom-metadata/history` are externally facing endpoints in the plan, but the tracking doc does not explicitly carry them into Milestone 2 integration coverage.
Plan evidence:
- `plans/mutable.metadata.plan.combo.v4.md:950-976` defines both endpoints.
- `plans/mutable.metadata.plan.combo.v4.md:1584` requires HTTP-level integration tests for all new `/custom-metadata/*` endpoints.
Tracking evidence:
- `plans/mutable.metadata.plan.combo.v4.tracking.md:339-340` adds handler-level TDD coverage only.
- `plans/mutable.metadata.plan.combo.v4.tracking.md:351-360` does not call out either endpoint in Milestone 2 integration tests.
- `plans/mutable.metadata.plan.combo.v4.tracking.md:356` references `/custom-metadata` generically, but does not explicitly track `/version` or `/history` route/method cells.
Why this matters:
- Milestone 2 can be marked complete without HTTP-level proof that version lookup, history pagination, and role gating work on those two routes.
Recommended tracking fix:
- Add explicit Milestone 2 integration tasks for:
- `GET /custom-metadata/version` happy path
- `GET /custom-metadata/history` happy path with pagination assertions
- authorization cells for both routes
2. High: The `GET /document/{id}` enrichment is documented as a user-visible feature and explicitly called out for integration verification in the plan, but the tracking doc only carries it at unit/controller level.
Plan evidence:
- `plans/mutable.metadata.plan.combo.v4.md:686-708` defines the externally visible `customSchemaId` and `hasCustomMetadata` additions.
- `plans/mutable.metadata.plan.combo.v4.md:1530` says Milestone 2 integration tests should verify that `GET /document/{id}` reflects the new fields.
- `plans/mutable.metadata.plan.combo.v4.md:722` includes `GET /document/{id}` enrichment in the metadata-read authorization surface.
Tracking evidence:
- `plans/mutable.metadata.plan.combo.v4.tracking.md:342-346` covers builder/controller TDD only.
- `plans/mutable.metadata.plan.combo.v4.tracking.md:351-360` has no explicit Milestone 2 integration item for `GET /document/{id}`.
- `plans/mutable.metadata.plan.combo.v4.tracking.md:356` does not explicitly cover the existing `document` resource behavior for this modified endpoint.
Why this matters:
- The modified read path could regress, or be wired to the wrong resource behavior, without blocking milestone signoff.
Recommended tracking fix:
- Add explicit Milestone 2 integration tasks for:
- document with no schema -> `customSchemaId: null`, `hasCustomMetadata: false`
- document with schema but no metadata -> bound schema ID, `hasCustomMetadata: false`
- document with schema and metadata -> bound schema ID, `hasCustomMetadata: true`
- role coverage confirming `GET /document/{id}` remains on the existing `document` resource behavior
3. High: The reset-metadata endpoint's documented HTTP contract is only partially tracked at integration level.
Plan evidence:
- `plans/mutable.metadata.plan.combo.v4.md:620-673` documents the endpoint, including `200`, `404`, and `409` behaviors.
- `plans/mutable.metadata.plan.combo.v4.md:1612-1619` explicitly requires integration coverage for the happy-path response assertions, legacy-extraction guard (`409`), and document-not-found (`404`).
Tracking evidence:
- `plans/mutable.metadata.plan.combo.v4.tracking.md:389-415` covers these branches at service/handler TDD level.
- `plans/mutable.metadata.plan.combo.v4.tracking.md:419-423` only tracks the upgrade flow, already-clean idempotency case, schema-version concurrency, auth matrix, and fullsuite gate.
Why this matters:
- Milestone 3 can be declared complete without HTTP-level verification of two documented error responses.
- The current happy-path integration item also under-specifies the response assertions the plan requires (`previousSchemaId`, `previousSchemaName`, `previousSchemaVersion`, `metadataVersionsDeleted`, and the post-upgrade `GET /document/{id}` check).
Recommended tracking fix:
- Add explicit Milestone 3 integration tasks for:
- reset on legacy-extraction document -> `409`, DB unchanged
- reset on non-existent document -> `404`, DB unchanged
- reset happy path asserts the documented response fields and verifies the follow-up `GET /document/{id}` state after rebind
4. Medium: Two documented HTTP behaviors are tracked only below the integration layer: clearing a document schema binding with `customSchemaId: null`, and the server-derived actor identity contract on write endpoints.
Plan evidence:
- `plans/mutable.metadata.plan.combo.v4.md:571-576` documents `PATCH /super-admin/documents/{id}/schema` with `customSchemaId: null` removing the binding when no metadata exists.
- `plans/mutable.metadata.plan.combo.v4.md:867-869` and `plans/mutable.metadata.plan.combo.v4.md:1604` make actor identity a user-visible contract: request-body `createdBy` is ignored, stored/returned actor fields come from the JWT subject.
Tracking evidence:
- `plans/mutable.metadata.plan.combo.v4.tracking.md:293` tracks the null-unbind path at service TDD level, but `plans/mutable.metadata.plan.combo.v4.tracking.md:351-360` has no HTTP integration item for it.
- `plans/mutable.metadata.plan.combo.v4.tracking.md:194`, `plans/mutable.metadata.plan.combo.v4.tracking.md:336`, and `plans/mutable.metadata.plan.combo.v4.tracking.md:411` track actor handling in controller/handler tests, but no milestone integration section explicitly carries the contract.
Why this matters:
- The API could pass service/controller tests while still missing the actual HTTP request/response behavior promised to callers.
Recommended tracking fix:
- Add a Milestone 2 integration case for `PATCH /super-admin/documents/{id}/schema` with `{"customSchemaId": null}` on a pre-bound document with no metadata.
- Add integration cases for the write endpoints that expose actor fields in persisted data or responses, at minimum `POST /super-admin/custom-schemas`, `POST /super-admin/custom-schemas/{schemaId}/versions`, `POST /custom-metadata`, and `POST /super-admin/documents/{id}/reset-metadata`.
## Verified
- The tracking doc already encodes the fail-first TDD rule and the "do not mark done until tests pass" rule:
- `plans/mutable.metadata.plan.combo.v4.tracking.md:5`
- `plans/mutable.metadata.plan.combo.v4.tracking.md:26`
- `plans/mutable.metadata.plan.combo.v4.tracking.md:31`
- `plans/mutable.metadata.plan.combo.v4.tracking.md:508-510`
- I did not find conflicting language that would allow a checkbox or milestone to be marked complete before the associated tests are passing.