mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-09-11 13:28:57 +00:00
xarmian
47d2b15e10
* feat(store): make one-collection-per-trait a database invariant (TASK-2710) Partial unique indexes on both drivers over the artifact_kind and invocation_field declarations, with the de-duplication pass that has to precede them. The de-dup is Go, not SQL, and runs BEFORE migrate(). The ruling requires every resolution to be REPORTED, because it silently changes which collection owns a kernel behaviour, and a SQL migration cannot log — Postgres RAISE NOTICE goes nowhere here and SQLite has no equivalent. Splitting decide-in-SQL from report-in-Go would give one rule two spellings to keep in step, which is the defect class IDEA-2883 closed. It runs before migrate() because the CREATE statements fail on exactly the databases needing repair. The rule, after three proposals each retired by a measurement: most user-written items wins; ties break on lowest (created_at, id), reported as ARBITRARY rather than as age, because created_at is second-resolution and newID() is a random uuid v4, so same-second rows carry no age at all. The loser keeps every item and loses only the declaration. Routing MAY change on affected deployments; there is no current behaviour to preserve, since with two declarations live the winner was measured flipping between runs on Postgres. SeedCollectionsFromTemplate now skips a definition whose artifact kind is already declared. Renaming re-slugs, so a workspace whose conventions became house-rules looked slug-empty while its kind was still claimed; seeding used to mint the duplicate and would now fail the whole seed instead. Claude-Session: https://claude.ai/code/session_01HeChkgZVYb3NTgTcckF5KR * test(server): keep the shadowing tests under the new trait invariant (TASK-2710) TestResolvePlaybookIgnoresInvisibleCollections and TestCollectionIDForKindIgnoresInvisibleCollections build two collections declaring one trait, which the partial unique indexes now forbid. They are not obsolete and I did not weaken them. They guard the round-2 shadowing fix: when two collections declare one kind and the first-sorting one is invisible to the caller, resolution must return the visible one instead of failing. TASK-2710 makes that state unrepresentable going forward and repairs it at startup on databases holding it, but the resolver is what stands between a legacy database and a wrong answer in the window before that repair, and on any deployment where an operator dropped the index. So the fixture now constructs the state the way it exists in the wild — with the constraint suspended — and the assertions are untouched. Same reasoning as IDEA-2883's disagreeing-reminder fixture. Claude-Session: https://claude.ai/code/session_01HeChkgZVYb3NTgTcckF5KR * test(store): SuspendTraitUniquenessForTesting beside its neighbour, with a restore (TASK-2710) Lead ruling: shape it like SetBcryptCostForTesting — same file, ForTesting suffix, returns a restore the caller defers, so a suspended constraint cannot outlive the test that suspended it. The restore recreates the indexes from the SHIPPED migration text rather than a hand-copied approximation, which would drift and then attest to an index the product does not have. Its failure is information, not noise: recreating a unique index while a duplicate is live is exactly what the migration would hit. The de-dup tests therefore assert the restore SUCCEEDS, which is the migration's precondition checked rather than assumed; the server shadowing tests leave the duplicate live for their whole duration and ignore the error explicitly rather than by omission. Claude-Session: https://claude.ai/code/session_01HeChkgZVYb3NTgTcckF5KR * fix(store): de-dup uses the index's own extraction, strips every declaration once, counts NULL source as user-written (TASK-2710) Three P1s from codex round 1, each verified before accepting. A collection can lose BOTH declarations — the playbooks definition declares artifact_kind and invocation_field — and resolving them in two passes, each re-parsing the row's ORIGINAL traits, made the second write restore what the first stripped. The duplicate survived and the migration would still have failed on it, surfacing as a broken upgrade rather than a test. Now one write per collection accumulating every strip, with a regression test. Detection asked Go what a declaration is while the indexes ask json_extract / ->>, so the two could disagree: a row the Go parser rejects still carries a value the index sees, and the de-dup would leave a pair the CREATE then refuses. It now asks the database the same question the index asks, which is the same one-rule-one-spelling reasoning that put the report in Go. source IS NULL now counts as user-written. The column is nullable and legacy rows predate it; 'source <> template' alone is NULL for those, which SQL treats as not-true, so a workspace whose only user content is old would have had it ignored when picking the winner. Claude-Session: https://claude.ai/code/session_01HeChkgZVYb3NTgTcckF5KR * fix(server,store): seed skips either held declaration; unique violations map to 409 on both drivers (TASK-2710) Two P2s from codex round 1. The seeder checked only artifact_kind, but the playbooks definition also declares invocation_field and TASK-2710 adds an index for each — so a workspace whose invocation-routing collection had been renamed would still have failed its seed. It now skips when EITHER declaration is held, and says which. Collection create recognised only SQLite's "UNIQUE constraint" text, so the identical race answered 409 on SQLite and 500 on Postgres; the update path recognised neither. Both now use one named isUniqueViolation covering both drivers, matching what every item handler already did. The conflict message also distinguishes the indexes: "a collection with this name already exists" is actively misleading for a trait conflict, where the name is fine and the declaration is taken — a user told to rename would rename forever. Claude-Session: https://claude.ai/code/session_01HeChkgZVYb3NTgTcckF5KR * chore(store): drop the unused order slice from the de-dup pass (TASK-2710) Leftover scaffolding from the one-write-per-collection rewrite; staticcheck caught it (SA4010). Mine to catch earlier — I ran lint at the tip BEFORE that rewrite and not after it, so the gate found what a re-run would have. Claude-Session: https://claude.ai/code/session_01HeChkgZVYb3NTgTcckF5KR * fix(store): de-duplicate a conflicting archive on import; guard json_extract against malformed traits (TASK-2710) Both from codex round 2, both real. Import warned about a duplicate declaration and inserted both, which was right while nothing forbade the pair. With the unique indexes the second INSERT is refused, the whole transaction rolls back, and the workspace minted beforehand survives as a husk (BUG-2892) — so an archive carrying a duplicate would become unimportable, and those archives are exactly the ones this release repairs. This is the task's item 4, which I had not done. The first declaring collection in bundle order keeps it and later ones are stripped and reported; the rule cannot use user-item counts here because items are inserted after collections, so bundle order IS the terminator and the log says so. SQLite's json_extract RAISES on malformed JSON rather than returning NULL, so the unguarded expressions in the index predicates and the de-dup scan would have failed STARTUP on any database holding one bad blob. json_valid now guards both, matching what every other reader does with malformed traits — treat the row as declaring nothing. Postgres needs no equivalent: traits is JSONB, so the column type makes malformed content unrepresentable at rest. Claude-Session: https://claude.ai/code/session_01HeChkgZVYb3NTgTcckF5KR * test(store): assert the malformed-traits asymmetry per driver (TASK-2710) My own Postgres gate caught this: the test planted a malformed traits blob and Postgres refused it — invalid input syntax for type json — because traits is JSONB there. That refusal IS the reason migration 064 carries no json_valid guard while 087 does, and it was prose in the migration until the gate turned it into an observation. The test now asserts it per driver: on Postgres the plant must be REFUSED, on SQLite it must succeed and the guard must keep both the de-dup pass and index creation working. It therefore also catches someone 'fixing' the asymmetry later — adding a guard Postgres does not need, or dropping the one SQLite does. Claude-Session: https://claude.ai/code/session_01HeChkgZVYb3NTgTcckF5KR * fix(store,server): de-duplicate on the bytes being written; stop claiming a name conflict for item-index violations (TASK-2710) Round 3, two findings. P1, a regression I introduced: the import de-duplication pre-computed its strips from the traits the BUNDLE carries, which is not what gets written — coercion, validation-discard and canonical inference all run afterwards. A pre-traits archive carrying conventions with an empty blob has its declaration INFERRED from the slug (BUG-2702), so a bundle pairing that with an explicit declarer showed the pre-pass one declaration and the database two, and the index aborted the whole import. The check now runs immediately before the INSERT, on the final bytes, which turns the question from 'what did the file say' into 'what am I about to write'. Reproduced first, then fixed. P2: a collection UPDATE can migrate item field values, and an item-level unique index can fail there — invocation_slug is the live example. My catch-all reported that as 'a collection with this name already exists', sending the caller to rename something that is not the problem. The name message now requires the error to name the collections table; otherwise it says what it knows. Claude-Session: https://claude.ai/code/session_01HeChkgZVYb3NTgTcckF5KR * test(store): fail when an archived collection takes the live one's declaration (TASK-2710) The test for the rebase onto BUG-2884, written before the fix and failing against the naive resolution: live collections declaring convention = [], want exactly [conventions] BUG-2884 made the bundle carry soft-deleted collections. This branch moved import's duplicate-declaration check out of a pre-pass and into the insert loop, so it operates on the bytes actually being written (round 3's P1) — but `dropDuplicateImportDeclarations` has no notion of liveness, which the pre-pass had gained on main. An archived collection travelling ahead of the live one that replaced it therefore CLAIMS the kind, and the live collection is stripped of it. Every resolver filters deleted_at IS NULL, so the workspace imports with no live convention routing at all. The second assertion is the other direction: the archived collection must KEEP its declaration. Nothing routes to it, both partial unique indexes exclude it, and stripping it would edit data the operator archived rather than deleted. Claude-Session: https://claude.ai/code/session_01HeChkgZVYb3NTgTcckF5KR * fix(store): an archived collection neither takes nor loses a trait declaration on import (TASK-2710) The rebase fix for the test in the previous commit. dropDuplicateImportDeclarations now returns an archived collection's traits untouched. A soft-deleted row sits outside both partial unique indexes (each carries `AND deleted_at IS NULL`) and outside every trait resolver, so it can neither create the conflict this function prevents nor be harmed by holding a stale declaration. Letting it take a claim was the real damage: the live collection later in the bundle lost the declaration and the workspace imported with no routing for that kind at all. BUG-2884's pre-pass had grown the same condition; this branch replaced that pre-pass with an in-loop check on the final bytes (round 3's P1) and the condition did not come with it. Keeping both is what the rebase owes. TestImportRoutingIgnoresSoftDeletedCollections builds its fixture in a new order — declare, archive, then seed — because the unique index refuses two LIVE collections declaring one kind. The order is not a workaround: it is the production path that mints this state (delete the conventions collection, seed again), every step legal under the invariant, and it needs no test-only suspension of the constraint. Its assertions are unchanged. Claude-Session: https://claude.ai/code/session_01HeChkgZVYb3NTgTcckF5KR * docs(server): checkTraitConflicts no longer claims the index it now has (TASK-2710) CONVE-23 sweep. The doc comment describing that gate was written when the invariant did not exist and this branch falsified three of its sentences: "workspace IMPORT bypasses it entirely by design" (import de-duplicates on the way in now), "the database-level version is deliberately NOT added in phase 0" (migration 087 adds it), and the closing paragraph handing duplicates back to the resolvers' order-dependent behaviour. Rewritten to say what the division of labour actually is — the pre-check survives for the MESSAGE, because a unique violation is a 409 about a name unless something tells the handler otherwise and "rename your collection" is useless when the name is fine and the declaration is taken; the index is what holds. It also states the two things the invariant genuinely does not cover: import (which de-duplicates rather than refusing a restore) and archived collections (outside both indexes and every resolver, so a soft-deleted row may hold a declaration a live one also holds). Claude-Session: https://claude.ai/code/session_01HeChkgZVYb3NTgTcckF5KR * fix(store): take the pre-migration snapshot before the trait repair writes (TASK-2710) Codex round 5, P2, verified. dedupeTraitDeclarations ran from the two constructors, ahead of migrate() — and snapshotBeforeMigrate() runs INSIDE migrate(). The repair changes data: it strips a declaration, moving which collection owns a kernel behavior. So the altered ownership was already committed when the snapshot was copied, and `<db>.pre-<version>` — the operator's rollback for a bad upgrade — contained it. Restoring after a failed migration handed back the old schema with the repair applied and unrecorded: the one thing the rollback could not undo was the only thing that had silently changed routing. The call moves into migrate(), immediately after the snapshot and before the migration loop, which satisfies both constraints at once — 087 / 064 still cannot run against a database holding duplicates, and the snapshot now precedes the write. The Postgres path takes the same position for symmetry; there is no snapshot there to sit after, so the ordering argument is one-sided on that dialect and the comment says so. TestTheSnapshotIsTakenBeforeTheRepairWrites pins it end to end: plant a duplicate, un-apply 087 so a migration is genuinely pending, reopen, then read the snapshot with the RAW driver — New() would migrate and repair the snapshot too, destroying the thing being measured — and assert it still holds both declarations while the live database holds one. Claude-Session: https://claude.ai/code/session_01HeChkgZVYb3NTgTcckF5KR