docs: Work identity, versions & Assets design (#60)

[Fooftilly]

This is a design-only architecture proposal for #60, under the #179 roadmap. Nothing is implemented: there are no schema, migration, data, API or UI changes. It has been revised after maintainer review, and the decisions from that review are recorded in §0 (D1–D11).

Contents

• docs/work-identity-model.md is the design document.
• docs/wiki/Domain-Model.md gains a short "Planned" pointer. It states that this is not current behavior.

Model

Work (W-…, existing ID kept)      tags, folder, playlist, status, open state,
  │ 1..*                          Research/Private Notes, Work-scoped roles (Author, Mentioned, Reviewer)
  │ primary_manifestation_id (read/display), citation_manifestation_id (NULL = primary)
  ▼
Manifestation (MF-…)  "Version"   citation metadata, language, identifiers,
  │ 0..*, one primary             edition-scoped roles (Translator, Editor, Foreword…)
  ▼
Asset (AS-…)  "File" / "Source"   one file/source lineage: source slot (pristine, optional) +
                                  working slot (served, materialized); hashes, provenance,
                                  annotations, page state

What changed in the revision

• Same-owner integrity is enforced by the DB (§4.1).
  • New and leaf tables get composite ownership FKs.
  • The two works pointers get narrow triggers instead, because rebuilding works inside the migration transaction would cascade-delete its child rows.
  • Both mechanisms were prototyped in SQLite 3.45.
• argument_sources identity (§8.5).
  • The row key is (argument_id, order_index).
  • A citation's identity is its Work, its Version (or none), and its pinpoint, so one Argument can cite several Versions of one Work.
  • Merges collapse only rows that are exactly identical.
• Merged Works (§10.4, §14.2).
  • Every pending operation naming a merged Work is refused with WORK_MERGED. The client can then re-apply the user's intent to the target.
  • Revisions are never rebased implicitly.
  • The legacy mapping uses a stored, immutable origin_work_id.
• source_mime is preserved by the backfill (§12.2).
• Pristine originals are storage slots inside one Asset (§9.4). PRKS's materialized PDF is not a separate peer Asset.
• No automatic full-library backup (D2). The migration is transactional and makes no filesystem change.
• Slice A acceptance criteria are now spelled out (§17).

Validation

This PR changes only documentation. tests.test_agent_guidance_current and tests.test_e2e_policy pass.

Refs #60, #179

🤖 Generated with Claude Code

https://claude.ai/code/session_016PBTe3CeL5KTUqEj9zJ6Yz

Summary by CodeRabbit

• Documentation
  • Added a section outlining the current Work model and a proposed Work, Version, and File or Source model, which remains under review.
  • Linked to the detailed design document.

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Important

Review skipped

Review was skipped as selected files did not have any reviewable changes.

💤 Files selected but had no reviewable changes (1)

• docs/work-identity-model.md

⚙️ Run configuration

Configuration used: Repository: Fooftilly/PRKS/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: c41e93cc-f371-4505-a113-23fa79579d65

📥 Commits

Reviewing files that changed from the base of the PR and between 7b375ce and 6046c5a.

📒 Files selected for processing (1)

• docs/work-identity-model.md

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository: Fooftilly/PRKS/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 13db59a1-2734-476e-921c-3af77874e3df

📥 Commits

Reviewing files that changed from the base of the PR and between 0e19b86 and 781a36b.

📒 Files selected for processing (2)

• docs/wiki/Domain-Model.md
• docs/work-identity-model.md

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.

---

📝 Walkthrough

Walkthrough

The domain model documentation describes the current relationship between Work, publication, and file. It also outlines a proposed Work → Version → File or Source model and marks it as under review.

Changes

Work identity and file model

Layer / File(s)	Summary
Current model and proposed separation docs/wiki/Domain-Model.md	Documents the current one-Work/one-publication/at-most-one-file model and a proposed Work → Version → File or Source design. The proposal remains under review.

Priority: ⬇️ Low

Estimated code review effort: 1 (Trivial) | ~4 minutes

Change: Other

Merge Risk: 🔵 Low · up to 781a3

Readers previewing this PR may be unable to follow the new design link until the document reaches master, though the proposal is available in the PR and the issue is temporary. This is a bounded documentation concern; merging can proceed.

Architecture Summary

Architecture risk: 🔵 Low · up to 781a3

The change affects 1 system.

Changed systems: docs

Architecture concerns
No architecture-level concerns identified.

Review details

Systems and components

• observed — docs (service) was modified; 1 changed file maps to changed impact.

Before / after behavior

• observed — Modified behavior in docs/wiki/Domain-Model.md: Adds a section documenting the current one-Work/one-publication/at-most-one-file model and a proposed Work → Version → File or Source separation for editions, translations, revisions, duplicate detection, and citation targets; the proposal is under review, not current behavior.

🚥 Pre-merge checks | ✅ 8

✅ Passed checks (8 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0…
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Prks Engineering Invariants	✅ Passed	PASS — The pull request changes only docs/work-identity-model.md and adds a clearly labeled planned-design pointer in docs/wiki/Domain-Model.md. The proposal explicitly states that it adds no sche…
Ui Design Contract	✅ Passed	The pull request changes only two documentation files: docs/wiki/Domain-Model.md and docs/work-identity-model.md. It does not change frontend code or user-visible interactions. The proposal explic…
Offline And Sync Coherence	✅ Passed	PASS. The PR changes only docs/work-identity-model.md and a planned pointer in docs/wiki/Domain-Model.md; no backend, frontend, service-worker, persistence, sync, or test files changed. The pointe…
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly identifies the documentation change and accurately summarizes the proposed Work identity, versions, and Assets design.

✨ Finishing Touches 💡 1

🛠️ Fix failing CI checks 💡

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

---

_{Comment @coderabbitai help to get the list of available commands.}

[qodo-code-review]

PR Summary by Qodo

Design Work, Manifestation, and Asset identity model

📝 Documentation 🕐 40+ Minutes

AI Description

• Audits current Work identity constraints and overloaded ownership across subsystems.
• Proposes Work–Manifestation–Asset boundaries for citations, annotations, roles, files, and
 deduplication.
• Defines additive migration, compatibility, stable IDs, and phased implementation guidance.

Diagram

classDiagram
class Work {
  +W identifier
  +primary version
  +notes and status
}
class Manifestation {
  +MF identifier
  +citation metadata
  +language and edition
}
class Asset {
  +AS identifier
  +storage locator
  +content hashes
}
class Role {
  +person
  +role type
  +scope
}
class Annotation {
  +page coordinates
  +materialization state
}
class Citation {
  +target version
  +optional pinpoint
}
Work "1" --> "1..*" Manifestation : versions
Manifestation "1" --> "0..*" Asset : files
Work "1" --> "0..*" Role : work roles
Manifestation "1" --> "0..*" Role : edition roles
Asset "1" --> "0..*" Annotation : owns
Citation "0..*" --> "1" Manifestation : targets

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Adopt a full FRBR hierarchy

• ➕ Models translations and expressions as independent first-class concepts.
• ➕ Offers greater bibliographic precision for complex edition families.
• ➖ Adds an additional entity, migration layer, and user-facing complexity.
• ➖ Exceeds current product requirements and increases synchronization scope.

2. Reuse Work IDs for Manifestations

• ➕ Simplifies migration of existing bibliographic and annotation references.
• ➕ Keeps legacy operation scopes directly attached to the backfilled version.
• ➖ Requires minting new logical Work IDs and repointing user-facing references.
• ➖ Creates ambiguity after manifestations move between Works during merges.

3. Keep the primary version embedded in Work

• ➕ Reduces initial schema additions for the common single-version case.
• ➕ Allows many existing readers to continue without a projection layer.
• ➖ Creates permanently different representations for primary and additional versions.
• ➖ Forces every version-aware consumer to support two storage paths.

Recommendation: Proceed with the proposed three-level Work → Manifestation → Asset model and additive strangler migration. It preserves existing Work identity, assigns citation and coordinate-sensitive state to the correct entities, and avoids the complexity of a full FRBR hierarchy or dual representation. Implementation should remain split into the proposed independently validated slices, with authority transfers and durable-operation scope migrations receiving the strongest review.

Files changed (2) +1355 / -0

Documentation (2) +1355 / -0

Domain-Model.mdLink the domain model to the planned identity redesign +4/-0

Link the domain model to the planned identity redesign

• Adds a clearly marked planned-design section describing the proposed Work → Version → File separation. It explicitly distinguishes the proposal from current system behavior and links to the full design document.

docs/wiki/Domain-Model.md

work-identity-model.mdDocument the Work, Manifestation, and Asset architecture +1351/-0

Document the Work, Manifestation, and Asset architecture

• Adds the complete design proposal, including the current-state audit, entity ownership, cardinalities, citation and annotation semantics, asset lifecycle, deduplication, stable IDs, migration compatibility, and roadmap implications. It also records rejected alternatives, phased implementation slices, and maintainer decisions required before implementation.

docs/work-identity-model.md

[Fooftilly]

github-advanced-security failure is not caused by this PR. The job (logs) stopped before any analysis ran with CAPIError: 400 The requested model is not supported inside the Copilot code-scanning agent. That is a scanner-side model configuration error. This PR changes only Markdown (docs/work-identity-model.md, docs/wiki/Domain-Model.md), and the scanner's own file-exclusion list already covers every code type. I tried one re-run of the failed job, and GitHub refused it (403 This workflow run cannot be retried). No fix exists in the repository. The check should pass once the scanner's model setting is corrected, or the maintainer can re-run it. I'll keep watching the PR.

---

Generated by Claude Code

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a8fab6b288

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[Fooftilly]

One additional design requirement before approving Slice A: the transitional denormalized owner columns need an enforceable same-owner invariant, not only independent foreign keys.

The proposal deliberately keeps compatibility columns such as work_id beside new manifestation_id / asset_id references. That is useful, but the design currently does not specify how the schema prevents impossible combinations such as:

• roles(work_id=W1, manifestation_id=MF2) where MF2.work_id = W2;
• argument_sources(work_id=W1, manifestation_id=MF2) where the Manifestation belongs to another Work;
• annotations(work_id=W1, asset_id=AS2) where AS2 -> MF2 -> W2;
• works.primary_manifestation_id = MF2 when MF2 belongs to another Work;
• manifestations.primary_asset_id = AS2 when AS2 belongs to another Manifestation.

If these are only separate FKs, all referenced rows can exist while the denormalized ownership is contradictory. That would make the legacy projection, cascades, merges/moves, sync scopes, and cleanup behavior ambiguous.

Please make the ownership invariant explicit in the design and in Slice A acceptance criteria. Prefer DB-enforced consistency where practical (for example composite owner FKs/unique parent keys, or narrowly scoped integrity triggers where SQLite cannot express the relationship cleanly), plus migration/schema validation tests. Canonical application commands should validate too, but application-only enforcement is not enough for a long-lived compatibility layer.

This is separate from the existing review findings about argument_sources row identity, source_mime backfill, and merged-work revision handling.

[Fooftilly]

Design review (PR #201 / #60)

Verified the audit against master @ 0e19b86 / schema v16. Core claims hold: Work-ID holders (incl. research-index tables), I1–I13, SYNCED_FIELDS, one folder / one playlist, annotation bytes overwritten in place, BibTeX urldate from updated_at, sync_tag_lifecycle analogy, ingest vs content hash split. Domain-Model Planned section correctly marks this as not current behavior.

Codex P1/P2 items on citation identity, merged-work writes, and source_mime backfill look addressed in db0a4f4.

Blocking

1. work_lifecycle name collision with existing work_lifecycle_sync (CREATE_WORK/DELETE_WORK). Prefer sync_work_lifecycle (parallel to sync_tag_lifecycle) and update MERGE/redirect references before freezing names.

Non-blocking

2. Domain-Model Planned link points at blob/master/…/work-identity-model.md (404 until merge; relative link better for PR preview).
3. Asset materialized_annotation_revision vs works materialized_pdf_annotation_revision — call out the rename.

CI green aside from known github-advanced-security scanner failure (not caused by this Markdown-only PR).

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: db0a4f44e1

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[Fooftilly]

Grok Bot design review (follow-up on db0a4f4)

Another Fooftilly review already covered the work_lifecycle name collision and two smaller nits. This pass adds findings that review did not cover. Codex items on a8fab6b (citation identity, source_mime backfill, WORK_MERGED refuse) verified fixed in the doc.

Blocking

Same-owner invariant for transitional denormalized columns — still missing (also requested on the issue). Independent FKs allow roles / argument_sources / annotations / primary_* to point at entities whose real owner is a different Work or Manifestation. §6.2 MERGE saying annotations move “unchanged” conflicts with §6.1’s annotations.work_id = Asset’s Work once the Manifestation moves. Please specify the invariant, preferred DB enforcement, mutation rules for MERGE / MOVE_*, and Slice A validation tests before approving A.

Non-blocking

Align §3.4 argument_sources identity with §8.5’s COALESCE unique index and state the rebuilt table’s PRIMARY KEY under SQLite NULL / UNIQUE rules.

Audit claims checked against master @ 0e19b86 look accurate.

[Fooftilly]

Addressed the same-owner invariant and the rest of the review in c196eea. §0 now lists every decision (D1–D11).

Ownership integrity (§4.1). The database enforces it, and Slice A's acceptance criteria now include it (§17).

• Composite ownership FKs cover the new tables and the leaf tables, which the Slice A migration rebuilds:

  • assets(manifestation_id, work_id) → manifestations(id, work_id)
  • annotations(asset_id, work_id) → assets(id, work_id)
  • roles(manifestation_id, work_id) → manifestations(id, work_id)
  • argument_sources(manifestation_id, work_id) → manifestations(id, work_id)
  • manifestations(primary_asset_id, id) → assets(id, manifestation_id), deferred

  These rely on the parent-pair unique keys (id, work_id) and (id, manifestation_id). ON UPDATE CASCADE keeps the denormalized owner columns correct when a Version moves.

• Narrow triggers guard the works pointers. works can't get a composite FK safely. It is the parent of about ten tables, and migrations run inside one transaction with foreign keys on, so rebuilding it would run DROP TABLE works, which cascade-deletes every child row. ALTER TABLE ADD COLUMN also cannot add a table-level FK. The triggers make each pointer (primary and citation) name a Version of the same Work, stop the primary from being cleared (except inside a merge), and stop a pointer's target from being moved away or deleted.

• Prototyped in SQLite 3.45. Every contradictory combination you listed is refused. Legal moves, pointer swaps and whole-Work deletes leave PRAGMA foreign_key_check clean.

• Not enforceable at commit. SQLite has no deferred triggers, so "every Work has a primary" is enforced at creation plus an integrity query that runs in the schema tests, after backfill, and in backup verification.

The other fixes.

• argument_sources (§8.5). The row key is now (argument_id, order_index), meaning the item's position in the list. Citation identity is UNIQUE(argument_id, work_id, COALESCE(manifestation_id, ''), pages). The backfill renumbers legacy order_index = 0 rows without changing what anyone sees. A merge only collapses rows that are exactly identical.
• Merged Works (§14.2). Every pending operation that names a merged-away Work gets WORK_MERGED and is never applied to the target. The legacy-operation mapping now uses a stored, immutable origin_work_id rather than recomputed uuid5 IDs.
• source_mime (§12.2). The backfill keeps it.

No new decision blocks approval. §18 lists three choices from this revision for you to confirm.

---

Generated by Claude Code

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a7d40058f4

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[Fooftilly]

Grok Bot design review (follow-up on a7d4005)

Verified rename to sync_work_lifecycle (no leftover merge-table work_lifecycle), Asset materialized_pdf_annotation_revision aligned with works, MOVE/MERGE cascade re-keying, crash-safe hash protocol, and §13.2 source routing. Prior items from reviews on db0a4f4 look addressed on this tip; not rehashed.

Non-blocking

§8.5 legacy wire match ambiguity. Citation identity allows both a Work-level locator (manifestation_id NULL, non-empty pages) and a Version-pinned row with the same (work_id, pages). The preserve rule "match existing (work_id, pages) → keep that row's manifestation_id" does not define which row wins when both exist, or whether multi-match must refuse. Please specify a deterministic rule (e.g. prefer pinned; prefer Work-level; or refuse ARGUMENT_SOURCE_VERSION_REQUIRED) before freezing the wire adapter.

[Fooftilly]

Grok Bot design review (follow-up on d6b278e)

Delta since a7d4005: quarantine for legacy FK orphans, edition-scoped role tombstone copy, widened Asset-creation predicate, and refuse-on-ambiguous legacy citation match. Checked against master (work_role_sync.get_roles_state tombstones; add_work accepts source_mime / thumb_url / thumb_page without a file). Prior §8.5 wire ambiguity is fixed. Codex items on this tip look addressed; not rehashed.

Non-blocking

Quarantine can change an Argument's observed source list, but §8.5 and Slice A acceptance (6) still say the rebuild renumbers without changing the list or its revision. When an argument_sources row is skipped into migration_quarantine (orphan work_id), survivors are renumbered and the wire list [{work_id, pages}] shrinks. Please say that those Arguments advance argument-sources/<A> (so offline replaces conflict), and narrow acceptance (6) to Arguments with no quarantined rows (or add an explicit quarantine-loss fixture).

[Fooftilly]

There is still one ownership-transition blocker in §4.1: the primary-pointer rules make two explicitly supported moves impossible when the moved entity is the only child.

Primary pointer invariants deadlock legal MOVE operations

For Assets, the design says:

• a Manifestation may have 0..* Assets;
• if it has an active Asset, primary_asset_id must be non-NULL;
• manifestation_primary_asset_kept refuses clearing primary_asset_id while any active Asset remains;
• the composite primary-Asset FK uses ON UPDATE RESTRICT;
• MOVE_ASSET says that if the Asset is primary, "the pointer must move first."

If a Manifestation has exactly one active Asset and that Asset is being moved to another Manifestation, there is no valid statement order:

1. clear the source primary_asset_id → trigger refuses because the Asset is still active;
2. move the Asset first → the primary FK's ON UPDATE RESTRICT refuses because the source Manifestation still points to it;
3. trash/delete it first → the trigger/FK also refuses.

Yet the desired final state (source Manifestation has zero Assets and NULL primary_asset_id) is explicitly valid.

There is an analogous case for MOVE_MANIFESTATION when the moved Manifestation is the source Work's only Manifestation. The design says the old Work may then be merged or deleted, but the primary Manifestation cannot be cleared/moved first. The merge path has a documented lifecycle exception; the plain-delete path does not have a viable transition.

Please revise the integrity/transition design so every legal final state has a legal transactional path. For example, this may mean relaxing the "may not clear while a child is currently active" trigger and enforcing final-state consistency at canonical-command/schema-validation boundaries, using a deferred NO ACTION relationship rather than immediate RESTRICT where appropriate, or defining an explicit lifecycle transition that the triggers recognize. Do not add transient placeholder Versions/Files merely to satisfy the constraint.

Add Slice A/K tests for at least:

• move the only primary Asset out, leaving the source Manifestation with zero Assets;
• move a non-primary Asset;
• move the only Manifestation out and resolve the now-empty source Work through each supported outcome;
• rollback leaves all primary pointers and owner columns unchanged.

The same-owner FKs themselves look sound; this finding is specifically about the pointer/lifecycle transition protocol around them.

[Fooftilly]

One more quarantine/revision edge case is still missing.

Quarantining a role with a missing Person changes a live Work role state without advancing its scope

§12.3 says quarantined annotations and roles "already belong to a Work that no longer exists, so no live scope reports them." That is not true for one of the preflight cases listed immediately above: a roles row can have a valid/live work_id but an invalid/missing person_id.

work_role_sync.get_roles_state() builds present directly from roles WHERE work_id = ?; it does not join persons. So before migration that broken row is still reported as present: true for work-person-role/[W,P,role] (revision 0 if no revision row exists, or its stored revision if one does). Quarantining the row makes that same live Work scope become absent. If the revision is left unchanged, the canonical state changed without a revision advance/tombstone.

Please make quarantine revision handling depend on the live aggregate that actually changes, not only on table type:

• if a quarantined role's Work still exists, preserve/advance a work-person-role/[W,P,role] tombstone so the observed present→absent transition has a newer revision;
• if the Work itself is missing, no live Work role scope needs to be created;
• keep the analogous argument-sources/<A> bump already added when an Argument survives but one of its source rows is quarantined.

Add a Slice A fixture for live Work + missing Person + role row, with and without an existing role revision, and assert get_roles_state() reports absence at a strictly newer revision after migration. This is the same no-silent-state-change invariant that the design already applies to argument-source quarantine and role tombstones.

[Fooftilly]

Duplicate of the immediately preceding quarantine/revision finding; please use that thread for the fix.

[Fooftilly]

Confirmed: the only-child moves deadlocked. Fixed in 37cd9f4 (§4.1 "Transitions", §10.4, §17). Every legal final state now has a legal transactional path, and no placeholder Version or File is created.

Asset moves.

• The primary-Asset FK is now ON UPDATE/DELETE NO ACTION DEFERRABLE INITIALLY DEFERRED. SQLite applies RESTRICT immediately even when the FK is deferred, which caused the deadlock.
• I removed the "may not clear the primary while an active Asset remains" trigger.
• Moving the only primary Asset out is now: move the Asset, then clear primary_asset_id, in either order. The FK is checked at COMMIT.
• "A Manifestation with an active Asset has an active primary" is a final-state rule. Canonical commands check it on the rows they touched before committing, and the integrity query checks it too.

Manifestation moves. A Work with zero Versions is not a valid final state, so moving a Work's only Manifestation needs an explicit empty_source outcome: delete or merge. Both use one transition that the triggers recognize:

1. Insert a marker in work_retirement.
2. Clear the Work's pointers. The trigger allows this only when the marker exists.
3. Move the Manifestation. The owner columns cascade.
4. Delete the Work. An AFTER DELETE trigger removes the marker.

The marker has a deferred FK to an always-empty guard table. If a marker is left behind, COMMIT fails, so a Work can never persist without a primary.

Prototype in SQLite 3.45. All of these behaved as intended, and foreign_key_check stayed clean:

• moving the only primary Asset out left the source with 0 Assets and a NULL primary;
• moving a non-primary Asset needed no pointer change;
• moving the only Manifestation out worked with the emptied Work deleted, and with it merged;
• clearing a primary without the marker was refused;
• a marker left behind failed COMMIT;
• a move that forgot the pointer failed at COMMIT, and the rollback left every pointer and owner column unchanged.

These tests are now in Slice A's acceptance criteria (schema level) and Slice K's (command level, including each empty_source outcome).

---

Generated by Claude Code

[Fooftilly]

One additional schema-level gap in the design: manifestation_relations is defined as same-Work only, but unlike the other ownership-sensitive tables the document does not specify any DB constraint that enforces that invariant.

Slice A creates this table, so if it is just (from_id, to_id, relation) with independent FKs to manifestations(id), a cross-Work translation_of / revision_of row is structurally valid. Later MOVE_MANIFESTATION explicitly depends on relations never crossing Works (it drops relations to the old Work), so a buggy writer/direct repair could leave a relation the domain says cannot exist.

Please either:

• include a denormalized work_id on manifestation_relations and composite ownership FKs for both endpoints to (manifestation_id, work_id), or
• define an equivalent narrow trigger/integrity rule that rejects endpoints whose manifestations.work_id differs.

Also add from_id <> to_id unless self-relations are intentionally meaningful. This can be a Slice A invariant test. It is less urgent than the primary-pointer transition blocker, but it is cheap to settle before the table becomes permanent.

[Fooftilly]

Re the quarantine and role-revision finding: you're right. get_roles_state() builds present from roles without joining persons, so a row with a live Work but a missing Person reports present: true. Fixed in 86b0b23 (§12.3 step 1.1): the revision rule now follows the live aggregate that changes, not the table type. For each quarantined row, PRKS advances every scope whose state endpoint reported that row:

• Argument still exists: advance argument-sources/<A>, as already specified.
• Role row whose Work still exists: insert or advance a work-person-role/[W, P, role] tombstone, so the scope reports absence at a strictly newer revision.
• Owning Work or Argument missing: no live scope reported the row, so no scope is created. Orphaned annotations can only fall in this case.

Slice A acceptance criterion (6) now includes fixtures for a live Work, a missing Person and a role row, with and without an existing role revision, and asserts that get_roles_state() reports absence at a strictly newer revision.

---

Generated by Claude Code

[Fooftilly]

Grok Bot design review (follow-up on 0f5c78e)

Delta since 127746a (including tip fixes after concurrent review on 261b1b3): lossless role-credit merge via manifestation_credit_overrides, projected title/abstract PATCH routing, content_generation CAS fingerprinting, MOVE_ASSET destination-primary, processing_files.imported_work_id re-point, §7.2/Slice K required for credit overrides, rewrite holds managed_pdf_path_lock across steps 1–3.

Verified FIXED: Codex role-credit / MOVE_ASSET destination / processing_files / title routing / fingerprint race intent; concurrent Fooftilly §7.2 optional-overrides and rewrite-lock items (0f5c78e); prior quarantine and work_retirement items. Domain-Model blob/master not re-raised.

Blocking

1. content_generation uninitialized on create/backfill — CAS content_generation = ? cannot succeed if the column is NULL; SQLite also leaves NULL+1 as NULL. §12.2 backfill omits it while leaving content_sha256 NULL for the fingerprint pass.

Non-blocking

2. Empty string vs NULL for title/abstract overrides under the new §13.2 non-NULL routing (§5 NULL=inherit).
3. §13.2 override writes vs §14.2 work-field/[W, title|abstract] — conflict unit unclear when PATCH writes the Manifestation override.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0f5c78ef80

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[Fooftilly]

There is one remaining blocker in the new Asset-field revision migration.

Asset-owned tombstones can exist even when Slice A creates no Asset

§14.2 now correctly says every work-field/[W, thumb_page] scope row, tombstones included, moves to asset-field/[origin_AS(W), thumb_page], and that the same rule applies to other Work fields that become Asset-owned.

But §12.2's Asset-creation predicate still looks only at the Work's current canonical values:

• file/video identity;
• live annotations;
• non-empty source_mime;
• thumb_url / thumb_page;
• non-zero materialization revision.

That misses a valid existing state: a Work can have no current Asset-owned value but still have a non-zero revision/tombstone in sync_entity_revisions.

Concrete example:

1. Work has no file/video/annotations/etc.
2. User sets thumb_page = 3 through SET_WORK_METADATA_FIELD → work-field/[W, thumb_page] revision becomes 1.
3. User clears it → column becomes NULL, revision becomes 2.
4. At Slice A, the current-value predicate sees no Asset-owned value and creates no Asset, so there is no origin_AS(W).
5. At Slice D, the required tombstone copy to asset-field/[origin_AS(W), thumb_page] has nowhere to go. A queued legacy SET_WORK_METADATA_FIELD(W, thumb_page, base=2) likewise cannot be mapped through origin_AS(W).

The same class can affect any legacy scope that later becomes Asset-owned; annotation tombstones are another example if live annotations/file state have since disappeared.

Please make Slice A's Asset-creation predicate include legacy durable state that will later require an Asset owner, not only current values. In practice, if a Work has any sync_entity_revisions row in a scope family/field scheduled to migrate to Asset ownership (for example work-field/[W, thumb_page], pdf-annotation/[W, ...], and any applicable work-source/W state), create the deterministic placeholder origin_AS(W) even when every current Asset-owned column is empty.

That keeps the existing invariant that legacy operations/tombstones can always map to a stable origin_AS(W).

Please add acceptance fixtures for at least:

• set then clear thumb_page, leaving only the revision tombstone and no current Asset data;
• a deleted annotation whose revision tombstone survives after no live annotation remains;
• verify Slice A still creates exactly one deterministic placeholder Asset for those Works;
• verify Slice D can copy the tombstones and a pending legacy operation maps to that same Asset ID.

Alternatively Slice D could mint the missing Asset, but that would make the legacy mapping appear late and complicate deterministic identity; the cleaner design is to reserve the Asset in Slice A whenever existing durable state proves one is semantically required.

[Fooftilly]

Re Asset-owned tombstones without a current Asset: confirmed, and fixed in e404705 (§12.2, §17).

Slice A's predicate now also creates the deterministic placeholder origin_AS(W) whenever the Work has any durable revision state in a scope that will become Asset-owned, even if every current Asset-owned value is empty. That means a sync_entity_revisions row for:

• work-field/[W, thumb_page], or any other Work field scheduled to become Asset-owned;
• pdf-annotation/[W, …], including tombstones of deleted annotations;
• work-source/W.

The placeholder owns those scopes, so Slice D always has a stable target and never mints one late.

After Slice A the mirror triggers create origin_AS(W) the first time a Work without one gains any Asset-owned value or annotation. A tombstone can only follow a value, so no Asset-bound revision can exist without an owner. Clearing values later never deletes the Asset.

Acceptance criterion (6a) covers:

• thumb_page set then cleared, with only the tombstone remaining;
• a deleted annotation with no live annotation left.

Each gets exactly one deterministic placeholder. A Slice D test then copies those tombstones and maps a queued legacy operation to the same Asset ID.

---

Generated by Claude Code

[Fooftilly]

Grok Bot design review (follow-up on e404705)

Delta since 303e1ec: expands the Asset-creation predicate so Works whose only Asset-bound state is durable revision/tombstone state still get a deterministic origin_AS(W) at Slice A, and adds Slice A acceptance (6a).

Prior findings at 303e1ec remain fixed. Domain-Model blob/master pointer still open (accepted).

Non-blocking

1. Slice A (6a) fixtures cover thumb_page clear and annotation delete, but the expanded predicate also mints on a work-source/W revision row. (6a) does not exercise a cleared video-source / work-source tombstone mapping through that same origin_AS(W), so acceptance does not fully prove the predicate (including a queued SET_WORK_SOURCE mapping).

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 781a36bbb9

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[Fooftilly]

Grok Bot design review (follow-up on 747126a)

Delta since 781a36b: freeze inherited title/abstract on MERGE_WORKS before reparenting Manifestations; make POST /api/works create an Asset only when the §12.2 predicate holds.

Earlier findings: the Codex P1 (inherited metadata changing on merge) and P2 (unconditional Asset on create) that this tip answers are FIXED. Domain-Model blob/master link remains open (maintainer-accepted; not re-raised).

New (non-blocking):

1. §14.2 still says title/abstract overrides are created only by the Version-aware API, but merge step 3 now creates them too.
2. Freeze cannot preserve a NULL source abstract when the target has one (NULL means inherit; '' is forbidden by CHECK), so moved Versions can silently gain the target's abstract unless the preview calls that case out.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4b749ea3a7

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[Fooftilly]

Grok Bot design review (follow-up on 0925f65)

Delta since 4b749ea: freeze inherited title/abstract on MOVE_MANIFESTATION (Codex P1); legacy exact-duplicate fallback compares like-for-like post-linearization hashes (Codex P2).

Earlier findings: Codex P1/P2 FIXED. Prior Grok non-blocking items remain FIXED. Domain-Model blob/master STILL OPEN (accepted — not re-raised).

New (non-blocking):

1. §14.2 still lists only Version-aware API + MERGE_WORKS step 3 as writers of manifestation-field/[MF, title|abstract] overrides; MOVE_MANIFESTATION now also freezes/writes them — extend the writer list (same class as the earlier MERGE-only gap).
2. MOVE_MANIFESTATION freezes title/abstract but not Work-scoped credits (credits(M) = destination Work roles after the move). The row claims displayed/cited metadata never changes silently; Authors/credit spellings still can. Either apply a MERGE step 4–style credit freeze (and mention MOVE in §7.2’s override-table requirement), or preview the credit change and drop the absolute claim.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0925f6557e

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[Fooftilly]

One issue remains in the new legacy exact-duplicate fallback at 0925f65.

The post-linearization hash is not reproducible with PRKS's current qpdf invocation

§9.4 now says a legacy content_sha256 can be compared with a new upload's post-linearization hash and that this should match an unannotated legacy file when the same qpdf version/deterministic IDs are used.

Current PRKS does not produce deterministic qpdf output. backend/pdf_linearize.py invokes:

qpdf --linearize <input> <output>

with no --deterministic-id.

qpdf's own documentation explicitly says byte-for-byte idempotency is not expected in the normal case, including because document-ID generation has a random element. --deterministic-id exists specifically to remove timestamp/output-name/random-ID effects, and even qpdf's stronger idempotency recipe requires deterministic processing.

So two uploads containing exactly the same original PDF can be linearized by the same installed qpdf version and still get different working bytes/hashes. The new fallback therefore still misses the exact legacy duplicate it was added to recover; the statement about "same qpdf version and deterministic IDs" does not match the actual PRKS pipeline.

Please change the design before Slice I in one of these directions:

• normalize both sides to an explicitly defined reproducible representation before comparing (and add tests proving repeated runs produce the same comparison digest); or
• add an independent canonical/semantic digest designed for this purpose; or
• explicitly drop the legacy-content exact-match fallback and document that legacy Assets without ingest_sha256 cannot participate reliably in exact-byte duplicate detection.

Simply adding --deterministic-id to future uploads is not enough to make historical legacy content_sha256 values comparable, because those existing files were already linearized under the old non-deterministic invocation.

Please add a Slice I test that takes the same source PDF through the relevant legacy/new normalization paths multiple times and proves that the comparison key is stable before calling this an exact duplicate signal.

[Fooftilly]

Re the post-linearization legacy fallback: you're right. backend/pdf_linearize.py runs qpdf --linearize <in> <out> without --deterministic-id, so identical uploads can yield different working bytes, and existing files can't be re-derived. Fixed in 6046c5a (§9.4, §10.1, Slice I) by taking your third option, dropping the fallback:

• Legacy Assets do not take part reliably in exact-byte duplicate detection. The doc says this plainly and explains why adding --deterministic-id later wouldn't help historical files.
• One check is kept, because it has no false positives. If a legacy content_sha256 equals a new upload's raw ingest_sha256, the stored legacy bytes are the uploaded bytes. This happens when linearization was skipped and the file was never annotated. Equal hashes can't be wrong, so it is reported as an exact duplicate.
• Otherwise, legacy files are found only through identifier and metadata candidates (§10.3), which are suggestions.
• New Assets always carry ingest_sha256, computed on the raw stream, so their comparison key is deterministic.

Slice I stability test: the same source PDF is ingested several times with linearization on. It must get the same ingest_sha256 every time, even though the working bytes and content_sha256 differ. That proves the key doesn't depend on qpdf's nondeterminism. A second test checks that an unlinearized, unannotated legacy file matches opportunistically, and that a linearized one is never claimed as an exact match.

---

Generated by Claude Code