mAi
df65e4b586
Phase 5c slice A. Pulls the structural rules out of the Postgres triggers into a Go-side validator. The trigger stays as defence in depth; the validator is the human-facing error path. - docs/plans/itemwrite-validation.md enumerates every rule the triggers in 0001 + 0010 enforce, with the ValidationError.Kind callers will see for each. Eleven rules total (two SQL-only safety rails kept untranslated). - internal/itemwrite/itemwrite.go: ValidationError + Input + Reader interface + ValidateFormat (pure: missing fields, slug format, status whitelist, self-parent) + ValidateAgainstStore (DB-aware: unknown-parent, slug-collision under any common parent, cycle via ancestor-closure DFS capped at 64 hops to mirror the trigger). - Eight kind constants exported: missing-required, invalid-slug-format, invalid-status, slug-collision, cycle, self-parent, unknown-parent, unresolvable-path. Tests cover every kind on both happy and reject paths: missing / whitespace fields, slug containing dot / upper / whitespace, invalid status enum, self-parent guard, unknown parent id, root slug collision, sibling slug collision under common parent, cycle on ancestor closure, and the "Reader returns ListAll error → validator returns nil" path (callers see the infra error later, validator doesn't mask it). No caller migrates yet. Same Go-linker DCE caveat as 5a/5b slice A: `strings <binary> | grep internal/itemwrite` returns 0 until slice B imports. Task: t-projax-5c-itemwrite
132 lines
7.7 KiB
Markdown
132 lines
7.7 KiB
Markdown
# itemwrite-validation — Phase 5c
|
||
|
||
**Task:** `t-projax-5c-itemwrite`
|
||
**Status:** in progress (Slice A)
|
||
**Date:** 2026-05-22
|
||
|
||
## Why
|
||
|
||
Six Go-side write paths (`web/server.go:handleDetailWrite/handleReparent/handleNewSubmit`, `web/bulk.go:handleBulkApply/handleBulkChip`, `mcp/tools.go:createItemTool/updateItemTool`) currently hand inputs straight to `store.Create/Update/Reparent`. Validation lives only in Postgres triggers. When the trigger raises, callers get an opaque pgErr string and have to substring-match to render anything useful. The bulk-apply path wants to *pre-flight* every row outside the txn — impossible without a Go-side validator.
|
||
|
||
Phase 5c lifts the structural rules out of the trigger into `internal/itemwrite/` so:
|
||
|
||
1. Callers fail fast with typed `ValidationError{Kind, Path, Detail}` instead of raw pgErr.
|
||
2. The bulk-apply path validates every row outside the txn; the txn only opens for inputs that already passed validation.
|
||
3. Handlers render human-readable banners keyed on `Kind`, no substring matching.
|
||
|
||
The Postgres trigger stays as **defence in depth**. If the trigger rejects something the Go validator allowed, that's a validator bug and the raw pgErr surfaces unchanged so the gap is visible.
|
||
|
||
## Rule enumeration (read from db/migrations/0001 + 0010)
|
||
|
||
Every structural rule the live triggers enforce against `projax.items` writes, with the `ValidationError.Kind` the Go validator will report for the same case.
|
||
|
||
| # | Where (SQL) | Rule | `Kind` | Trigger raises errcode |
|
||
|---|----------------------------------------------------------|-------------------------------------------------------------------|-----------------------|------------------------|
|
||
| 1 | `0001_init.sql` `items_slug_no_dots` | slug must not contain `.` | `invalid-slug-format` | check_violation 23514 |
|
||
| 2 | `0001_init.sql` `items_status_valid` | status ∈ {active, done, archived} | `invalid-status` | check_violation 23514 |
|
||
| 3 | `0001_init.sql` `title not null` | title required (we additionally reject empty) | `missing-required` | not_null_violation 23502 |
|
||
| 4 | `0001_init.sql` `slug not null` | slug required (we additionally reject empty) | `missing-required` | not_null_violation 23502 |
|
||
| 5 | `0010_multi_parent.sql` `items_before_write` self-parent | `new.parent_ids` must not contain `new.id` | `self-parent` | check_violation 23514 |
|
||
| 6 | `0010_multi_parent.sql` `compute_item_paths` direct cycle| `p_self_id = ANY(p_parent_ids)` (transitive variant: closure walk)| `cycle` | check_violation 23514 |
|
||
| 7 | `0010_multi_parent.sql` `compute_item_paths` transitive | A parent's ancestor closure contains self | `cycle` | check_violation 23514 |
|
||
| 8 | `0010_multi_parent.sql` `items_check_slug_collision` | No two items share a slug under any common parent | `slug-collision` | unique_violation 23505 |
|
||
| 9 | `0010_multi_parent.sql` `items_root_slug_uniq` (partial idx) | No two root items share a slug | `slug-collision` | unique_violation 23505 |
|
||
| 10| Implicit: any `parent_ids[i]` must resolve to a live item | `compute_item_paths` swallows pathless parents (returns `[slug]`); the SQL FK is gone with the array model. We add this Go-side. | `unknown-parent` | (n/a — added in Go) |
|
||
| 11| Implicit: Reparent target path resolution | A path arg in handleReparent / MCP reparent must resolve | `unresolvable-path` | (n/a — added in Go) |
|
||
|
||
Two rules in the SQL trigger that we DO NOT validate in Go:
|
||
|
||
- **Path computation cap** (`hops > 64` in `compute_item_paths`). It's a safety rail against pathological inputs; the cycle check fires earlier in practice. If the cap ever fires, we let the trigger surface the raw pgErr so we can investigate.
|
||
- **`items_after_delete` cascade** scrubs deleted ids from descendants' `parent_ids`. Not a validation rule — runs unconditionally on delete.
|
||
|
||
## Design
|
||
|
||
### Package
|
||
|
||
`internal/itemwrite/` — new top-level package alongside `internal/aggregate/`, `internal/cache/`, `internal/graph/`. Nothing outside the projax binary imports it.
|
||
|
||
### Types
|
||
|
||
```go
|
||
type ValidationError struct {
|
||
Kind string // discriminator — see table above
|
||
Path string // dot-path of the offending item ("dev.paliad", or "" if not yet a path)
|
||
Detail string // human-facing message (used as banner copy)
|
||
}
|
||
|
||
func (e *ValidationError) Error() string
|
||
|
||
type Input struct {
|
||
ID string // empty for Create; populated for Update / Reparent
|
||
Title string
|
||
Slug string
|
||
Status string
|
||
ParentIDs []string // resolved IDs (not paths)
|
||
}
|
||
|
||
type Reader interface {
|
||
GetByID(ctx context.Context, id string) (*store.Item, error)
|
||
ListAll(ctx context.Context) ([]*store.Item, error)
|
||
}
|
||
```
|
||
|
||
`Input` is the validator's input shape; callers populate it from form values / MCP args / bulk rows. `*store.Store` satisfies `Reader` by method-set; tests stub with a small fake.
|
||
|
||
### Methods
|
||
|
||
```go
|
||
func ValidateFormat(in Input) *ValidationError
|
||
func ValidateAgainstStore(ctx context.Context, r Reader, in Input) *ValidationError
|
||
```
|
||
|
||
`ValidateFormat` is pure (no DB): rules 1–4 + 5 (self-parent if Input.ID present).
|
||
`ValidateAgainstStore` adds rules 6–11.
|
||
|
||
Callers compose:
|
||
|
||
```go
|
||
if err := itemwrite.ValidateFormat(in); err != nil {
|
||
// render banner via err.Kind
|
||
return
|
||
}
|
||
if err := itemwrite.ValidateAgainstStore(ctx, s.Store, in); err != nil {
|
||
// render banner via err.Kind
|
||
return
|
||
}
|
||
// safe to call store.Create / Update / Reparent
|
||
```
|
||
|
||
### Cycle detection
|
||
|
||
DFS up the ancestor closure starting from each parent id, looking for `in.ID`. Cap at 64 hops to mirror the SQL trigger. Pure-Go; uses the `Reader.ListAll` snapshot to avoid N+1 round-trips.
|
||
|
||
## Slicing
|
||
|
||
| Slice | What lands |
|
||
|-------|-----------|
|
||
| A | Plan doc + `internal/itemwrite/` + table-driven tests covering every Kind. No callers migrate. |
|
||
| B | `web/server.go` + `web/bulk.go` call the validator before the store. pgErr-string-matching deleted from web/. Bulk-apply pre-flights outside the txn. |
|
||
| C | `mcp/tools.go createItemTool` + `updateItemTool` call the validator. MCP errors carry `{kind, path, detail}` structured content. |
|
||
|
||
Standard deploy-verification triple per slice (commit SHA + container task-ID delta + artifact probe). Per-slice `mai report completed`.
|
||
|
||
## Test-modification rule (per task brief)
|
||
|
||
- **Behaviour preservation** is the contract: what HTTP status, response body shape, MCP result, accepted/rejected input set — all stay byte-identical for valid AND invalid inputs.
|
||
- Test SOURCE may change where it asserts on an implementation detail being refactored away (pgErr substring → ValidationError.Kind). Each such change is called out in the commit message.
|
||
- Test SOURCE must NOT change where it asserts on observable behaviour. If such a test breaks, it's a real behaviour drift — investigate, don't loosen.
|
||
|
||
## Out of scope
|
||
|
||
- Type-system enforcement (`ValidatedInput` newtype). Discipline for v1.
|
||
- Removing the Postgres triggers (defence in depth stays).
|
||
- Adding new rules beyond what the trigger enforces today.
|
||
- Slug rename / alias semantics.
|
||
- Auditing `if err != nil` paths in web/ beyond the write paths.
|
||
|
||
## References
|
||
|
||
- Task `t-projax-5c-itemwrite`
|
||
- Trigger source: `db/migrations/0001_init.sql`, `db/migrations/0002_path_trigger.sql`, `db/migrations/0010_multi_parent.sql`
|
||
- Sibling packages: `internal/aggregate/` (5a), `internal/cache/` (5b), `internal/graph/`
|