feat(publisher-services): implement BE-03 protected service configuration - #809
Conversation
…tion Implement the approved bounded BE-03 specification as an inactive additive foundation, from the authorized base 3b6b3a3. Database (migration 20260813_v1.7.0, with schema.rs edited manually and atomically under ADR-0003 Architecture A): - publisher.service_configuration_updated_at, the canonical optimistic concurrency token, strictly increasing per publisher; - the closed publisher_service_configuration_source type (SUPERUSER_API, MIGRATION_BACKFILL) with no OTHER, UNKNOWN or Default; - the append-only publisher_service_configuration_history audit table with a named non-blank actor check, an ON DELETE CASCADE foreign key and a deterministic composite index. Exactly one authoritative production write path owns every committed write of package, enabled-platform desired state, configuration token and audit history in one transaction on one connection, under an explicit caller-supplied source/actor context: publisher row lock first, stale expectedUpdatedAt rejected before any validation or lifecycle call, whole normalized desired set validated before the first write, subscription_package written directly so no publisher_history row is produced, platform state applied only through BE-02's lifecycle primitives, and exactly one audit row per committed change carrying bounded three-key canonical before/after state. BE-02's lifecycle is refactored additively into connection-scoped enable_on and disable_on primitives that return a must_use Changed/Unchanged outcome and check is_assignable() themselves before any write. The pool-level enable/disable keep their signatures, semantics, early non-assignable check and merged tests, and there is still exactly one linked-platform algorithm, so a membership-equal but split OAPEN/DOAB pair is repaired rather than skipped. GraphQL adds the protected PublisherServiceConfiguration type with effective capabilities derived on read from the canonical package and persisted nowhere, the owner-and-superuser publisherServiceConfiguration query under a least-privilege per-publisher check with no role inheritance, the superuser-only publisherServiceConfigurations report and publisherServiceConfigurationCount, and the superuser-only replacePublisherServiceConfiguration mutation. Exactly one new error variant and one new into_field_error arm are added. ThothPackage and PublisherCapability become SDL-reachable only through the protected type. Accepted and measured consequence: because the token is a publisher column, every committed change also moves publisher.updated_at and refreshes work.updated_at_with_relations across that publisher's catalogue through the existing trigger's single set-based statement; a no-op, a stale request and a rollback move none of the three, and other publishers are never touched. Inactive foundation only: the migration creates zero audit rows and changes no package or assignment; no job, dissemination or activation exists; no environment or production migration was executed.
Normal pull_request-triggered CI on the draft implementation PR completed with all ten checks passing, including run_migrations against CI's own PostgreSQL service. Record the per-check results in the implementation report's CI section in place of the PENDING placeholder. No workflow file was changed and no workflow was dispatched.
The recorded per-check durations are from the run on the implementation commit; every push to the branch re-runs the same ten checks and the live per-head result is the pull request's own check record under ADR-0005.
The independent HIGH-risk implementation review of exact head 1315057 returned BLOCKED on five findings. The CTO resolved the single blocking authority conflict and authorized bounded corrections to all five plus the generated-schema wording. This commit implements exactly that. P1 SDL guard coverage. The shared `sdl_block()` used `.split_once('}')`, which stopped at the first closing brace after the declaration. In the real generated SDL that is the brace closing `imprints`' nested `order: ImprintOrderBy = {...}` default, so every field after `imprints` was never inspected and a protected field added there passed both guards. A new test-only `graphql::sdl_support` provides one brace-balanced, string-aware extractor: string awareness is required because the real SDL contains braces inside descriptions, escaped quotes and `"""` block strings inside a type body. Measured on the real SDL, the old extraction covered 1203 characters and missed `contacts` and `distributionPlatforms`; the new one covers 1733 and catches a `subscriptionPackage` field inserted after `imprints`. Both guards now use it and assert those two post-`imprints` sentinels as a coverage precondition. Guards are strengthened, never weakened; BE-02's four verbatim read surfaces are unchanged. P1 actor-check contradiction. The specification's exact DDL `CHECK (btrim(actor) <> '')` contradicted its own acceptance wording, since one-argument `btrim` trims spaces only and so accepted a tab-only or newline-only actor. Per the CTO decision the authoritative invariant is that an audit actor must contain at least one non-whitespace character. The specification and the unmerged migration now express that one invariant as `CHECK (actor ~ '[^[:space:]]')`, verified against PostgreSQL 17.10 to cover space, tab, newline, carriage return, vertical tab and form feed. Same table, same column, same constraint name, same audit architecture; only the predicate is strengthened, and no second constraint is added. P2 doubled publisher UPDATE. Step 8 wrote `subscription_package` and step 10 wrote the token, so a combined change updated the publisher row twice and ran the shared AFTER UPDATE work-freshness cascade twice over the same rows. Step 8 now compares only, and step 10 issues one conditional UPDATE carrying the package when it changed and always the token. Every committed change shape now costs exactly one publisher UPDATE and one cascade; a true no-op or stale request costs zero. Re-measured at 2000 works: 23 statements (was 24), one publisher UPDATE, 2000 target rows refreshed, 0 unrelated, 39.55 ms (was 86.35 ms). Stop condition 19 reassessed from the new evidence: NOT TRIGGERED. P2 DataLoader statement count. The report claimed the count does not grow with N. The merged ADR-0007 loader has MAX_BATCH_SIZE = 200, so the accurate shape is two set-based report statements plus ceil(N / MAX_BATCH_SIZE) set-based assignment dispatches, with no per-publisher loop. A 201-publisher regression asserts chunks [200, 1] and two dispatches. No second loader. P2 call-site arithmetic. Re-run at this head: the pool-level enable/disable search returns 61 hits, not 66, with per-file counts derived mechanically (45/14/2) rather than by hand. All are in test files; zero production callers bypass the coordinator, which is the invariant that matters and is unchanged. Generated-schema wording. `thoth-client/assets/schema.graphql` is build-generated and ignored under existing repository authority, so the specification's "commit the regenerated schema" was unsatisfiable. It now requires regeneration through the normal build path, the exact SDL diff, a reproducible artifact identity, workspace-path client verification and the backend head for APP-01 pinning. `.gitignore` is untouched and the artifact is not force-added. Regenerated after the full remediation: 72 added / 0 removed against the authorized base and byte-identical to the previous head, so no production GraphQL contract changed. The amended specification is a candidate in this PR and is not yet reachable from develop. The new exact head has not been independently reviewed and requires a fresh HIGH-risk review.
…er UPDATE Two records still described the superseded two-UPDATE sequence after the remediation commit changed it: - implementation report section 5 item 5 still presented "two publisher UPDATE statements on a package change" as an accepted measured consequence, and cross-referenced step 8 as the package writer; - specification section 18 still attributed the no-publisher_history-row test to step 8. Both now describe the corrected sequence: step 8 compares only, step 10 issues the single conditional UPDATE. Section 18 additionally requires the per-shape write-footprint tests the remediation added, so the acceptance criteria state the one-UPDATE property rather than leaving it implied by the transaction sequence alone. Documentation only. No code, migration, schema, GraphQL contract or test change.
|
Control record — BE-03 HIGH-risk merge authorization (2026-08-13) Exact implementation head: Fresh independent HIGH-risk implementation review of this exact head returned APPROVED with no findings. The reviewer declared independence from both implementation/remediation and the prior reviewer that returned BLOCKED on CTO authorization supplied in the control conversation explicitly approves the BE-03 implementation and the bounded CTO-authorized BE-03 specification corrections on this exact head and authorizes merge of PR #809 into Immediately before this control record, GitHub was re-checked: PR #809 is OPEN/DRAFT/UNMERGED and mergeable at the exact head above; Authorization boundary: repository integration only. This does not authorize deployment, environment or production migration execution, production access, MIG-01, backfill, assignment creation, distribution activation, dissemination, BE-04, APP-01, APP-02, OBSERVE/ENFORCE changes, workflow changes or manual workflow dispatch, or action on PR #799. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Control record — BE-03 HIGH-risk merge authorization and independent review Exact implementation head: Fresh independent exact-head implementation review: APPROVED — findings NONE. Reviewer declared independence from both the implementing/remediation agent and the prior reviewer that returned BLOCKED on CTO authorization supplied in the control conversation on 2026-08-13 explicitly approved the BE-03 implementation and bounded CTO-authorized BE-03 specification corrections at that exact head and authorized merge into Pre-merge controls were satisfied: PR head remained Authorization boundary remains unchanged: repository integration only. No deployment, environment or production migration execution, production access, MIG-01, backfill, assignment creation, distribution activation, dissemination, BE-04, APP-01, APP-02, OBSERVE/ENFORCE changes, workflow changes/manual workflow dispatch, or action on PR #799 is authorized by this merge. |
Bounded review remediation for BE-03-CLOSEOUT-01. Additive only; the existing commit is untouched. Finding 1 - correct the ADR-0005 evidence claim. The CHANGELOG entry and implementation report sections 5 and 15 asserted absolutely that no merge commit SHA is transcribed into repository files, which the same change contradicts by legitimately recording b51bcc0 as the exact CTO-authorized implementation base, its identity as PR #812's merge commit, and PR #809's merge commit as ancestry evidence. The wording now distinguishes prohibited terminal-lifecycle transcription - copying GitHub lifecycle metadata merely to restate terminal review, authorization or merge state - from exact SHAs recorded as authorized base or preflight/ancestry evidence, which repository controls require as execution evidence. No exact-base or preflight evidence is removed and BE-03-CLOSEOUT-01.md section 18 is unchanged. Finding 2 - record the actual link-check command. Report section 9 carried a prose description rather than an executable command. It now records the exact reproducible command and its fresh result: 97 relative links checked, 0 broken, re-run after every repository-file edit including these. Also clarifies report section 5.1: the two items are the additional BE-03 source-state hits found by the fresh classified search, distinct from the closeout task record's own Status and section 18 updates that the implementation authorization separately requires. No classification, runtime, schema, migration, workflow or downstream semantics changed.
1 participant
Task identity
docs/engineering/ai-delivery/tasks/BE-03.md, merged through specification PR #8083b6b3a31f9358011f0c998015dfd0c2508380e83c678bdcec33c2aa01be1f887a85ff851dfe35891(implementation3facc3f9+ two docs-only CI-evidence commits + remediationc3206085+ docs-only alignmentc678bdce)1315057983d389d1ef6b85bc4e69b81eda53aa79— independently reviewed, decision BLOCKEDdevelopdevelopwas at exactly the authorized base at preflight and at push time; thebranch was created by the control agent at that SHA and was not recreated,
rebased, amended or force-pushed.
Review history
An independent HIGH-risk implementation review was performed on the previous
exact head
1315057983d389d1ef6b85bc4e69b81eda53aa79and returned BLOCKED onfive findings:
UPDATE/ doubled trigger cascadeFinding 2 was an authority conflict inside the approved specification itself —
its exact DDL
CHECK (btrim(actor) <> '')contradicted its own acceptance-testrequirement to reject a whitespace-only actor — and only the CTO could resolve
it. The CTO resolved that conflict and authorized bounded corrections to all
five findings plus the generated-schema wording. Remediation commit
c3206085implements exactly that, appended as an ordinary commit with norebase, amend, squash or force-push. A second, documentation-only commit
c678bdcefollows it, correcting two records that still described thesuperseded two-
UPDATEsequence; it changes no code, migration, schema,GraphQL contract or test.
The amended specification text is a candidate in this pull request; it is
not yet reachable from
develop, which remains at3b6b3a31.Full evidence:
BE-03 implementation report.Migration
thoth-api/migrations/20260813_v1.7.0/{up,down}.sql, created with therepository-supported command under the explicitly authorized DATE override
make migration DATE=20260813, because20260812is already occupied by mergedBE-02. It produced exactly that one directory and nothing else; no existing
migration was appended to, renamed or rewritten.
It adds
publisher.service_configuration_updated_at, the closedpublisher_service_configuration_sourcetype and the append-onlypublisher_service_configuration_historytable with its primary key,ON DELETE CASCADEforeign key, named non-whitespace actor checkCHECK (actor ~ '[^[:space:]]')and deterministic composite index.thoth-api/src/schema.rsis edited manually and atomically inthis same PR under ADR-0003 Architecture A.
Evidence: empty-database apply/revert/re-apply clean; populated 500-publisher /
2 000-work database migrated forward by the real runner with
relfilenodeunchanged (no rewrite), packages unchanged, BE-02 assignments
byte-identical by md5 digest, zero audit rows and zero job tables;
observed locks and per-statement DDL timings recorded.
Actor constraint (corrected). The authoritative invariant is that an audit
actor must contain at least one non-whitespace character. The constraint is
CHECK (actor ~ '[^[:space:]]')under the unchanged namepublisher_service_configuration_history_actor_check, with the table, theactor text NOT NULLcolumn and the whole audit architecture unchanged and nosecond constraint added. Verified against PostgreSQL 17.10 (CI uses
postgres:17) that POSIX[[:space:]]covers space, tab, newline, carriagereturn, vertical tab and form feed. Re-applied on a fresh disposable database:
apply → revert → re-apply clean, and the catalog reports
CHECK ((actor ~ '[^[:space:]]'::text)). RealINSERTs against the liveconstraint reject
'',' ',' ','\t','\n','\r','\v','\f'and a mixed whitespace string, and accept' real-actor-42 'and'\t\nreal-actor-42\r\n'. The supersededbtrimpredicate acceptedeverything from the tab case downwards, which is the defect this closes.
Canonical write coordinator
One authoritative production write path,
replace_publisher_service_configuration, owns every committed write acrosssubscription_package, desired enabled platform state, the configuration tokenand the audit table, in one transaction on one connection, under an explicit
caller-supplied
source/actorwrite context. It makes no authorizationdecision of its own. The GraphQL mutation is its only production caller.
Sequence: publisher row
FOR UPDATEfirst → read under lock → staleexpectedUpdatedAtrejected before validation and every lifecycle call →normalize (dedupe + linked closure) → validate the whole desired set →
package written directly (so no
publisher_historyrow) → lifecycle appliedonly through BE-02's connection-scoped primitives,
enable_oncalledunconditionally per desired group → token bumped and exactly one audit
row written iff something changed → commit.
BE-02 lifecycle composition
Additive, behaviour-preserving refactor:
enable_on/disable_ontake&mut PgConnection, assume a caller-owned transaction, return a#[must_use]Changed/Unchangedoutcome, and checkis_assignable()themselves beforeany write. The pool-level
enable/disablekeep their signatures, semantics,ordering, error behaviour and early pre-connection non-assignable check, and now
delegate. There is still exactly one linked-platform algorithm in the
repository. BE-02's 40 existing lifecycle tests pass unchanged.
Protected GraphQL
PublisherServiceConfiguration(publisher,subscriptionPackage,effectiveCapabilities,enabledDistributionPlatforms,updatedAt), theowner-and-superuser
publisherServiceConfigurationquery, the superuser-onlypublisherServiceConfigurationsreport andpublisherServiceConfigurationCount,and the superuser-only
replacePublisherServiceConfigurationmutation. Exactlyone new error variant (
StalePublisherServiceConfiguration) and one newinto_field_errorarm (STALE_SERVICE_CONFIGURATION).Authorization is least-privilege with no role inheritance: superuser, or
exactly
PUBLISHER_USERfor the target publisher.PUBLISHER_ADMIN,WORK_LIFECYCLEandCDN_WRITEwithoutPUBLISHER_USERare denied — alone andcombined. Anonymous is rejected before any publisher load; a
NULLzitadel_idfails closed for every non-superuser; a multi-publisher account reads its two and
not a third. Every matrix row is tested, each requesting
effectiveCapabilities,so capability exposure follows the same single decision.
Effective capabilities are exactly
ThothPackage::capabilities()in thatslice's order, derived on read from the same locked row that reports the package,
persisted nowhere — no capability column, table, override, cache or second
mapping, and no capability input.
OASISreturns[]. Upgrade and downgradechange them automatically; a platform-only change does not.
Audit
Exactly one row per committed change, with
actor = PolicyContext::user_id()and
source = SUPERUSER_APIfor every row BE-03 writes; no BE-03 path emitsMIGRATION_BACKFILL.before_state/after_statecarry exactly three keys —subscriptionPackage,enabledDistributionPlatforms(canonical order),configurationVersion— asserted by a test that fails if any key is ever added.No activation identifier, per-row timestamp, capability list, credential or
publisher metadata. No audit JSON is exposed through GraphQL.
Concurrency
Two clients on one token → one winner, one
STALE_SERVICE_CONFIGURATIONthatwrites nothing; the same with linked OAPEN/DOAB leaves no one-sided pair; a stale
true-no-op still fails; a stale repair still fails and the split pair
survives byte-identically; two concurrent repairs → one repair plus one stale;
different publishers do not contend; a replacement concurrent with a direct BE-02
transition serializes on the publisher row lock with no deadlock; the token
is strictly monotonic per publisher across a sequence containing a repair.
OAPEN/DOAB repair
Membership-equal split states are repaired, not skipped: split activation,
split
enabled_at, and one-sided pairs named by either member — each bumps thetoken and writes exactly one audit row whose states differ only in
configurationVersion. A fully normalized pair is a true no-op writingnothing.
JISC_NBKfails before any write, both when the request would otherwisechange nothing and when it would otherwise change the package.
OCLC_KBandEX_LIBRIS_KBstay independent. A directenable_on(JISC_NBK)regression provesthe primitive fails before any write whether the caller's transaction is then
committed or rolled back.
Staff report and query efficiency
Superuser only, checked before any database access. Filters render in the merged
repository shape —
publishers: [Uuid!] = [],packages: [ThothPackage!] = [],enabledPlatforms: [DistributionPlatform!] = []— withANDsemantics forenabledPlatformsand a mandatorypublisher_id ASCtie-breaker.Measured with the existing observed-loader harness. The accurate statement shape
is bounded, not constant: the merged ADR-0007 assignment loader has
MAX_BATCH_SIZE = 200, so a report page of N publishers issues[1][25][200][200, 1]The earlier claim that "the count does not grow with N" was wrong without the
batch-size qualification — it holds only while N ≤
MAX_BATCH_SIZE— so 201 isasserted as a regression point. There is still no second loader: the
protected field reuses BE-02's existing request-local ADR-0007 loader,
loader-first,
try_loadonly.Trigger and write-amplification evidence
Because the token is a
publishercolumn, every committed change also movespublisher.updated_atand refresheswork.updated_at_with_relationsacross thepublisher's catalogue. All six cases were reproduced against a real disposable
database with two imprints, two target works and a control work of another
publisher:
publisher.updated_atOne publisher
UPDATEper committed change (corrected). The reviewed headwrote
subscription_packageat step 8 and the token at step 10, so a combinedchange updated the publisher row twice and ran the shared
AFTER UPDATEwork-freshness cascade twice over the same rows. Step 8 now compares only, and
step 10 issues a single conditional
UPDATEcarrying the package when itchanged and always the token:
UPDATEsThe committed write footprint is therefore one publisher row + bounded
configuration/audit rows + N related work rows for every committed shape — no
longer
2Nfor a combined change. Optimistic concurrency, thestaleness-before-validation ordering, whole-set prevalidation, the monotonic
token expression and the one-audit-row-per-committed-change rule are unchanged,
and no shared trigger, package location or token architecture was altered.
Catalogue-scale measurement re-run after the fix (disposable environment
only; not a production SLA and deliberately not extrapolated): 2 000 target
works, 23 SQL statements for the whole request (was 24), 1 publisher
UPDATE, 2 000 work rows changed by the trigger, 0 unrelated works changed,39.55 ms (was 86.35 ms). No per-work application loop — the request
issues no statement against
workat all. Stop-condition 19 reassessed fromthe new evidence as not triggered.
SDL and client
Exactly +72 / −0 lines: the three new object types, one input, one new enum,
three queries, one mutation, and
ThothPackage/PublisherCapabilitybecomingreachable only through the protected type. No existing field, nullability,
default or description changed.
thoth-client/assets/queries.graphqlisunchanged (reviewed conclusion: the internal export client consumes none of the
new surface).
thoth-client/assets/schema.graphqlis build-generated andgitignored under existing repository authority, so it is regenerated, never
committed or hand-edited;
thoth-client/.gitignorewas not modified and theartifact was not force-added. The specification wording was corrected to match
those mechanics: regenerate through the normal build path, record the exact SDL
diff against the authorized base, record a reproducible artifact identity,
verify the client through the repository-supported workspace path, and record
the backend head for APP-01 pinning.
Regenerated after the complete remediation: base artifact SHA-256
0ba96aa1aa15006e8bf8b9f4a711f9e493eec4ce51911eebc32fb99d1ba53a67, headartifact SHA-256
25329c1687d8b4222638c2f673bd2751a13adeda8c6f181d4ac83e869abac479, diffunchanged at +72 / −0, and the head artifact is byte-identical to the
previous head — independent confirmation that this remediation changed no
production GraphQL contract. The APP-01 pin is the head SHA
c678bdcec33c2aa01be1f887a85ff851dfe35891plus that artifact SHA-256; thec678bdcecommit is documentation-only, so the artifact is unchanged fromc3206085.thoth-appwas not modified.Authorized merged-test changes
Two merged SDL guards contained global exclusions that BE-03 intentionally
falsifies. Both were amended only to preserve their security intent, and both
are now stricter than the whole-document string searches they replace:
graphql::tests— the publicPublishertype,NewPublisherandPatchPublisherexpose no package, capability or configuration field; exactlyone field in the schema returns
PublisherCapabilityand it isPublisherServiceConfiguration.effectiveCapabilities; everyThothPackagereference is the enum declaration, the protected field, the superuser input or
a superuser report argument;
distribution_platform_tests— no BE-02 type nor the publicPublisherexposes package, capability or configuration state, and BE-02's four public read
surfaces are asserted verbatim as unchanged.
Guard coverage defect, repaired. As reviewed, both guards extracted a type
body with
.split_once('}'), which stops at the first closing brace afterthe declaration. In the real generated SDL that is the brace closing the
imprintsfield's nested defaultorder: ImprintOrderBy = {direction: "ASC", field: "IMPRINT_NAME"}, soeverything declared after
imprints—contactsanddistributionPlatforms—was never inspected, and a protected field added there would have passed both
guards silently.
Extraction now lives in one test-only module,
thoth-api/src/graphql/sdl_support.rs, and is brace-balanced andstring-aware. String awareness is required rather than defensive: the real SDL
contains braces inside descriptions (
\d{4,9}indoi), escaped quotes(
Timestamp) and"""block strings inside a type body(
Imprint.crossmarkDoi).Publisherblockcontacts(distributionPlatforms:split_once('}')The tampered case inserts
subscriptionPackage: ThothPackage!afterimprintsand asserts the guard fails, which is the regression the oldextraction could not catch. Both guards additionally assert the two
post-
imprintssentinels as a coverage precondition, and every.split_once('}')extraction indistribution_platform_testswas replaced. Thereal regenerated
Publishertype contains none ofsubscriptionPackage,effectiveCapabilities, any capability field,serviceConfiguration*orserviceConfigurationUpdatedAt. The guards were strengthened, never weakenedor deleted, and BE-02's four verbatim read surfaces are untouched.
No other merged test's behavioural expectation was changed.
Checks
git diff --check,cargo fmt --all -- --check,cargo check --workspaceandcargo clippy --all --all-targets --all-features -- -D warningsall clean at theremediation head.
cargo test --workspace: 1252 passed, 0 failed (BE-03 adds80 tests; the remediation adds 8 and strengthens 2 existing ones without
weakening any).
cargo run migrate/--revertexercised against disposabledatabases only — no environment or production database was contacted.
Write-path containment, re-run at this exact head. The pool-level
PublisherDistributionPlatform::(enable|disable)(search returns 61 hits —model/publisher_distribution_platform/tests.rs45,graphql/distribution_platform_tests.rs14,model/publisher_service_configuration/tests.rs2 — every one in a test file,with per-file counts derived mechanically rather than by hand. The reviewed
head's report said 66 (and 50 for the first file); both numbers were wrong and
are corrected. Zero production callers bypass the canonical coordinator,
which is the invariant that matters and is unchanged.
GitHub CI results for this exact head are recorded in the PR checks below.
Boundaries
Repository history only. No merge, deployment, environment migration,
production migration, production access, MIG-01, backfill, assignment creation,
distribution activation, dissemination, BE-04, APP-01, APP-02,
thoth-appchange, shared publisher-trigger change,
OBSERVE/ENFORCEchange, workflowchange or workflow dispatch. PR #799 untouched. Merge would authorize repository
integration only.
Do not merge: fresh independent exact-head review and explicit CTO merge
authorization are outstanding.