docs(publisher-services): specify the BE-02 distribution platform model - #788
Conversation
Author the bounded BE-02 implementation specification required by Gate 1 before any BE-02 runtime work may be authorized. BE-02 is the HIGH-risk, schema-bearing task that adds the inactive additive backend foundation for publisher distribution-platform configuration. The specification settles every low-level representation choice from live repository conventions inspected at develop 5a8c27b, rather than from the provisional values in the approved design, which ADR-0004 supersedes: - the PostgreSQL public.distribution_platform enum with exactly the 17 ADR-0004 values in binding declaration order, no OTHER and no fallback; - the closed Rust and GraphQL DistributionPlatform enum following the merged BE-01 ThothPackage pattern, with Default deliberately not implemented because a default would act as a fallback; - the publisher_distribution_platform schema: composite primary key, ON DELETE CASCADE foreign key, a single-row lifecycle check constraint, the partial enabled reverse-lookup index and the set_updated_at trigger; - the six activation-lifecycle transitions with their write, activation_id, timestamp and idempotency semantics, with disabled rows retained and a genuine re-enable generating a new activation; - group-level linked OAPEN/DOAB normalization: one transaction, one shared activation identity, identical transaction timestamps, atomic rollback, and no supported path to a one-sided enabled state, enforced transactionally rather than through a new database trigger subsystem; - OCLC_KB and EX_LIBRIS_KB as independent assignments sharing a feed profile without duplicate feed state or shared activations; - code-owned exhaustive descriptors proved by a wildcard-free match, with the BackCatalogueBehaviour vocabulary and an internal adapter profile that represents shared mechanism identity without collapsing destinations; - JISC_NBK visible but non-assignable, enforced fail-closed in the domain layer because assignability is code-owned descriptor metadata; - the four public GraphQL surfaces using the repository's existing offset/limit pagination, with a mandatory publisher_id tie-breaker that the existing Publisher::all lacks, and count/lookup agreement by construction. The specification also fixes the ADR-0003 Architecture A atomicity requirement, the empty and representative populated disposable-database migration evidence, the DDL lock assessment with no unevidenced production duration claim, the retained-foundation operational rollback, the concrete required tests, the acceptance criteria and the stop conditions. Reconcile the Publisher Services tracker, whose BE-02 row pointed at a non-existent specification and carried TBD. Documentation only. No BE-02 runtime code, migration, schema, model, GraphQL surface, descriptor, assignment table or test is added; no implementation branch is created; issue #765 is not modified; no ADR, inventory, evidence ledger or evidence count changes; and nothing is deployed, released, migrated or activated. BE-02 remains BLOCKED and unauthorized: implementation requires this approved specification plus separate explicit CTO authorization bound to a freshly verified exact develop head.
Add the required CHANGELOG entry under Unreleased for the BE-02 specification, now that the specification pull request number exists, and record that number in the specification implementation report. Root AGENTS.md section 13 requires every pull request to update CHANGELOG.md and to reference the pull request number where available, which is only knowable after the pull request is opened. Documentation only. No specification content changes, no runtime change, and no authorization is granted.
Independent review of the BE-02 specification returned CHANGES REQUIRED with six findings. Five are resolved; the sixth is an architecture decision that belongs to the CTO and is escalated rather than papered over. 1. Lifecycle and approval wording was internally contradictory: Status: DRAFT sat alongside "Approved by: CTO" and "Approved for implementation by: CTO", a dependency row asserted the specification was approved, and other prose said CTO approval was still to come. Under ADR-0005 any value those fields could hold is false before the event and stale after it, and the commit correcting them would invalidate the exact-head review that justified it. Remove the transient fields entirely and record only durable authority: approval authority CTO, approval evidence the GitHub pull-request record, implementation authorization separate and absent. Section 2.1's last rows become named gates rather than statuses; section 23 becomes a ten-event table naming where each event's evidence lives and separating repository authority from CTO approval and from implementation authorization; section 24 records approval authority and effect, never whether approval occurred. 2. The lock assessment claimed the migration takes no lock on any populated table. That is wrong: the foreign key references publisher, and per the PostgreSQL 17 documentation ADD FOREIGN KEY acquires SHARE ROW EXCLUSIVE on the referenced table as well as on the constrained one. Because SHARE ROW EXCLUSIVE conflicts with ROW EXCLUSIVE but not with ACCESS SHARE or ROW SHARE, publisher reads continue and publisher writes are blocked while it is held. Rewrite section 13.3 with per-operation locks, a blocked/not-blocked table, a conservative assessment naming lock acquisition and queueing rather than duration as the dominant risk, an explicit rejection of NOT VALID as useless against an empty child table, mandatory pg_locks verification, and prohibitions on the old claim and on any production-duration claim. 3. The row check constraint permitted states the lifecycle forbids: disabled rows with a null activation_id or enabled_at, enabled rows still carrying a disabled_at, and an enabled DEFAULT false that implied a never-activated row was valid. Since a row is created only by ABSENT -> ENABLED, every persisted row has an activation. Make activation_id and enabled_at NOT NULL, drop the default on enabled, leave disabled_at as the only nullable lifecycle column, and reduce the constraint to enabled = (disabled_at IS NULL). Add section 6.1.1 with the row-existence rule, the two legal states, the rejected states, per-operation satisfaction and the Diesel mapping; correct the model fields from Option<Uuid>/Option<Timestamp> to Uuid/Timestamp. 4. N+1 access for Publisher.distributionPlatforms on publisher lists is not settled and cannot be settled by this task. thoth-api/AGENTS.md section 6 requires new lists to avoid N+1 and use set-based SQL or batched loaders, and the repository provides nothing to reuse: no DataLoader, no look_ahead, no request-scoped state on the GraphQL Context, and all 56 existing child resolvers query once per parent. The exposure also arises through the pre-existing publishers root query, so no change confined to BE-02's own root fields removes it. Every remedy either adds cross-cutting GraphQL architecture, adds a dependency, waives a standing control or removes approved public API. Record the binding prohibition, keep section 6 intact, and escalate: sections 9.2.1 and 19.1 state the four options and the exact CTO decision required, section 18.7b requires measured query-count evidence, and section 22 gains the decision as a step before authorization. 5. Rollout conflated repository merge with environment state, claiming the table would be empty "in every environment" after merge. A merge deploys nothing and runs no migration. Split section 14 into what a merge guarantees in the repository and, conditionally, how an environment behaves once deployment and migration execution have been separately authorized and performed. Correct the same conflation in rollback and performance. 6. Compatibility claimed four new enums while naming three and two new root query fields while specifying three. Add section 12.1 as the binding inventory - 3 root fields, 1 new Publisher field, 2 object types, 3 GraphQL enums, 0 inputs, mutations or scalars - with an explicit table of the three internal Rust enums that must not reach the generated SDL, and assert it in section 18.7a. Acceptance criteria, required tests, the implementation-report expectations, the tracker and the CHANGELOG entry for PR #788 are updated to match. A classified search was used for affected claims; no global replacement was made and historical records outside this pull request are untouched. Documentation only. No runtime file changes, no migration, schema, model, GraphQL or error implementation, no implementation branch, no new issue or pull request, no ADR change and no modification of issue #765. BE-02 runtime implementation remains unauthorized.
Independent architecture review returned CHANGES REQUIRED with three P1 findings and one P2. Option A / A2 - look-ahead-driven set-based prefetch into request-scoped state - is unchanged; B, C and D were not reconsidered. P1 cache identity: the store was keyed by (loader, parent key), which collides for the argument-bearing child fields Thoth already has. Identity is now (loader identity, normalized load shape, parent key), with typed loader-specific shapes, one constructor shared by prefetch and lookup, and explicit default normalization - Juniper look-ahead reads only literal AST arguments and never applies schema defaults, while the child resolver receives the default-applied value. BE-02's argument-free field takes a Unit shape; no production field gains an argument. P1 failure state: the draft required a failed prefetch both to leave keys absent and to suppress the fallback that absence triggers. Replaced with a three-state store - NotLoaded falls back, Loaded (including empty) never queries, LoadFailed returns the error with no retry - the failure recorded once per dispatch, the parent list field still resolving, and a GraphQL-visible equivalence contract over errors[].path, null propagation and extensions.type rather than error text. P1 path coverage: correctness and N+1 compliance are now distinct. Publisher fans out through Imprint.publisher and Contact.publisher as well as the publishers root query, so a loader-backed field with one prefetch site can still issue a query per parent. Adopting tasks owe an exact-base path inventory, coverage or escalation, and per-path measurement; the BE-02 inventory belongs to BE-02. P2 measurement: statement counts must use a pool constructed after the instrumentation hook, not the process-wide OnceLock test pool. Documentation only. ADR-0006 remains PROPOSED, THOTH-GQL-BATCH-01 remains DRAFT and unauthorized, PR #788 and issue #765 are unmodified, and the changed head requires a fresh independent exact-head review.
CTO specification approval - exact headI approve the BE-02 specification on PR #788 at exact head This is specification approval only. It does not authorize BE-02 implementation, creation of the implementation branch, merge, deployment, migration execution, assignment backfill, distribution activation, Control boundary: any subsequent commit invalidates this exact-head specification approval and requires fresh review/approval as applicable. BE-02 implementation remains separately unauthorized pending repository-authoritative merge of the approved specification, fresh exact- |
ja573
left a comment
ja573
left a comment
There was a problem hiding this comment.
Independent exact-head specification review record
Decision from independent reviewer: APPROVED
This GitHub review record transcribes the result of a fresh independent review conducted in a separate Claude session that did not author, reconcile, or remediate the BE-02 specification. It is recorded here for repository evidence and is not represented as a review independently performed by the GitHub account submitting this transcription.
Repository: thoth-pub/thoth
Programme: Publisher Services and Distribution Configuration
Task: BE-02 specification
PR: #788
Risk: HIGH
Exact reviewed head: 50f9d36bfe4882ffb28addaafef26dd4a1bed738
Base: develop at 8dcf031d76beaf0ad1ac2b6abd3673e37f9a9d55
The independent reviewer verified the live PR state, complete four-file docs/control diff, exact-head docs-only CI, current repository implementation conventions, ADR-0004 inventory, ADR-0007/DataLoader foundation, persistence and migration design, lifecycle semantics, OAPEN/DOAB linked normalization remediation, OCLC/Ex Libris separation, JISC_NBK fail-closed behavior, public GraphQL contract, deterministic pagination, DataLoader/N+1 evidence requirements, error semantics, authorization boundary, rollout and rollback.
Findings: NONE.
Independent reviewer conclusion:
APPROVED
The BE-02 specification at exact head
50f9d36bfe4882ffb28addaafef26dd4a1bed738is suitable for CTO specification approval.Any subsequent commit invalidates this approval and requires a fresh exact-head review.
Authorization boundary: this independent review does not authorize BE-02 implementation, creation of feature/publisher-services/be-02, merge, deployment, production migration, assignment backfill, distribution activation, OBSERVE/ENFORCE, or action on PR #799.
CTO implementation authorization - exact baseI authorize implementation of BE-02 against freshly verified This authorization permits creation of the bounded implementation branch It does not authorize implementation merge, deployment, release, production migration execution, production assignment creation/backfill, distribution activation, The implementation must remain within the approved BE-02 specification, including ADR-0003 schema atomicity, ADR-0004 inventory, ADR-0007 first-production DataLoader requirements, required migration/GraphQL/client/query-count/error/concurrency evidence, and independent exact-head review before any future merge authorization. |
Add the CHANGELOG entry for implementation PR #805, reconcile the Publisher Services tracker to the actual live state, and add the BE-02 implementation report. The tracker's previous "BLOCKED - IMPLEMENTATION NOT AUTHORIZED" wording is now stale: the reconciled specification was independently reviewed, CTO-approved and merged through PR #788, and implementation was then separately authorized against that exact `develop` SHA. BE-02 moves to "IMPLEMENTED - AWAITING INDEPENDENT REVIEW / MERGE AUTHORIZATION" — not complete, because fresh independent exact-head review and separate CTO merge authorization are both still outstanding. The report records the preflight, the exact commands and their concise results, empty and representative-populated migration evidence, empirical `pg_locks` observation on a disposable PostgreSQL 17.10 database, the base-versus-head SDL diff, the DataLoader contract, the real-SQL query-count evidence for both parent shapes and every required boundary, and the known limitations — including the two places where the repository's own facts shaped how a specification requirement could be evidenced. Implementation remains an inactive foundation. No deployment, production migration, backfill, distribution activation or guard-mode change is performed or authorized, and PR #799 is untouched.
1 participant
BE-02 - Distribution platform model specification
STATUS: DRAFT - REVIEW FINDINGS REMEDIATED; DOCS-ONLY CI GREEN; AWAITING FRESH INDEPENDENT REVIEW
Task identity
thoth-pub/thothBE-02- Distribution platform modelfeature/publisher-services/be-02-specdevelopd411d4935a507804f28d8798419d405e32880d02develop:8dcf031d76beaf0ad1ac2b6abd3673e37f9a9d55ee2511d160b3963f690f058182278a08ab9bebee(review decision: CHANGES REQUIRED)50f9d36bfe4882ffb28addaafef26dd4a1bed738This is the existing bounded BE-02 specification PR, reconciled rather than replaced. The old branch was 89 commits behind current
develop; the reconciliation commit preserves the old #788 head as one parent and currentdevelopas the other, so no intervening repository work was discarded or force-pushed.Why this reconciliation exists
The previous exact-head review of #788 correctly left one architecture blocker open:
BLOCKED - N+1 CONTROL REQUIRES ARCHITECTURE DECISIONThat blocker is now resolved by repository authority:
THOTH-GQL-DATALOADER-01was independently reviewed, CTO merge-authorized and merged through PR THOTH-GQL-DATALOADER-01: add request-scoped GraphQL DataLoader foundation #802 as8dcf031d76beaf0ad1ac2b6abd3673e37f9a9d55;Contextnow owns request-local non-cachedRequestLoaders, with explicit 200 max-batch-size / 10 yield-count construction,try_load, total batches, set-based Diesel behindspawn_blocking, and safe non-serde error projection.The reconciled BE-02 specification therefore retires the old local architecture options/escalation and binds
Publisher.distributionPlatformsto the approved ADR-0007 foundation as its first production field consumer.Preserved BE-02 decisions
The reconciliation preserves the mature platform/model contract already developed on #788:
DistributionPlatformvalues; noOTHER, fallback or default;publisher_distribution_platformrelation;(publisher_id, platform), ON DELETE CASCADE FK, row lifecycle CHECK, partial enabled index and updated-at trigger;publisher_idtie-breaker;New binding first-production-DataLoader contract
Publisher.distributionPlatformsmust:RequestLoaders;200/10;publisher_id(Uuid);try_load, never DataLoader.load();[];tokio::task::spawn_blocking;Field-specific SQL evidence is mandatory for both parent-list shapes:
Reference acceptance case:
The spec also requires 1/100/200/201/500 boundary evidence, field-specific membership/order correctness, request-local/non-cached evidence and direct-vs-loader backend-failure equivalence.
Current diff
Against current
develop, the reconciled head changes exactly four documentation/control files:CHANGELOG.mddocs/engineering/ai-delivery/tasks/BE-02.mddocs/engineering/ai-delivery/implementation-reports/BE-02-SPEC-implementation-report.mddocs/publisher-services/task-status.mdNo runtime, Cargo, GraphQL implementation, schema, migration, workflow or infrastructure file changes.
Independent review remediation
The independent exact-head review at
ee2511d160b3963f690f058182278a08ab9bebeereturned CHANGES REQUIRED with two bounded findings. Both are remediated at the current head50f9d36bfe4882ffb28addaafef26dd4a1bed738, which changes onlydocs/engineering/ai-delivery/tasks/BE-02.mdanddocs/engineering/ai-delivery/implementation-reports/BE-02-SPEC-implementation-report.md.Finding 1 - OAPEN/DOAB linked normalization contradiction. The prior section 7.2 said both "if both are already enabled, linked enable is a complete no-op" and "linked enable repairs one-sided/split-activation state", which conflict when both rows are enabled but carry different
activation_id/enabled_at. Section 7.2 now defines normalized fully enabled (both rows present, enabled, sameactivation_id, same sharedenabled_at) and makes a linked enable a no-op only in that state; any other enabled-but-non-normalized pair (split activation, differing timestamp, one-sided, absent, disabled) is atomically normalized to one newactivation_idand one sharedenabled_at. Acceptance criteria (section 17) and required tests (section 18.4) are updated to add the explicitA != Bsplit-activation repair test and the analogous inconsistent-timestamp case. No linked-group architecture was broadened.Finding 2 - docs validation not recorded in repository evidence. The implementation report section 10 now records the actual
git diff --checkresult (PASS - no whitespace/conflict-marker errors) and separates locally executed checks from GitHub exact-head CI, recording runtime jobs as SKIPPED by the docs-only classifier rather than PASS.Repository-hygiene / CI state
All required docs-only CI is green at the remediated head, with heavy runtime jobs skipped by the repository's docs-only classifier as designed:
check-changelog: PASSclassify: PASSbuild/test/run_migrations/format_check/lint/build_and_push_staging_docker_image: SKIPPED by docs-only classifierThe
CHANGELOG.mdUnreleased / Added#788 entry added at the previous head remains present and accurate; this review remediation required no changelog change.This PR remains DRAFT. The remediated exact head requires fresh independent exact-head specification review; the remediation agent does not approve its own work.
BE-02 implementation remains NOT AUTHORIZED.
Authorization boundary
This specification reconciliation does not authorize:
feature/publisher-services/be-02;With the changelog gate now green, the next gate is fresh independent exact-head specification review. Only after an APPROVED independent review may the CTO approve that exact specification; only after the approved specification is merged and repository-authoritative may BE-02 implementation receive a separate authorization against a freshly verified exact
develophead.The authoring agent does not approve its own work.