feat: variation ontology provenance — stop silent promotion of machine-derived annotations (#608) - #615
Merged
Merged
Conversation
Releases the consolidated dependabot bumps and the brace-expansion CVE remediation from #609. Bumps all four version surfaces: - app/package.json - app/package-lock.json (both root version fields) - api/version_spec.json - CHANGELOG.md (promotes [Unreleased] to [0.30.14]) Verified: `npm ci --legacy-peer-deps` and `make code-quality-audit`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W7njywYE4bCjoJ6YedziVQ
…on 047) Two additive tables recording where a variation-ontology annotation came from. An assertion is one (entity_id, vario_id, modifier_id) claim carrying curator-facing state; evidence rows hang off it, one per source batch, so two independent sources corroborating a term is representable. modifier_id is part of the identity because modifier_list defines both present (1) and absent (5) as valid for variation -- those are two different claims with independent state. Deliberately not columns on ndd_review_variation_ontology_connect: that table is DELETEd and re-INSERTed wholesale on every review save (ontology-repository.R:233-307), which would destroy a provenance column each time. The vario_id FK column derives its charset/collation from information_schema at migration time rather than hardcoding utf8mb3. The referenced variation_ontology_list.vario_id is not reliably utf8mb3 -- in this repo's own sysndd_db_test it has drifted to utf8mb4_0900_ai_ci -- and a static charset makes MySQL reject the FK as incompatible. Verified empirically both ways; see the migration header. Refs #608
Normalizes source-specific evidence into a comparable 0-4 strength so sources stay sortable in the curator suggestion queue without unpacking evidence_json. Returns NA rather than guessing: an unrecognized source, a fractional value, a non-digit string, a logical, a non-scalar, or an out-of-range value never becomes a plausible strength. This feature exists to stop fabricated provenance, so a normalizer that invents a strength would be self-defeating. There is no curator source type -- a curator-authored annotation has no assertion row, so a curator evidence row cannot exist. Refs #608
provenance_for_entity() reads one row per evidence record and deliberately excludes evidence_json so the public endpoint stays small. attach_provenance() joins on the full (entity_id, vario_id, modifier_id) identity -- never (entity_id, vario_id), which would conflate 'missense is present' with 'missense is absent'. Terms with no assertion row get NULL, which the API contract defines as curator-authored. Multiple evidence rows collapse into a sources array ordered by strength descending then source_key ascending, so rendering order is deterministic. Join keys coerce entity_id/modifier_id to integer first: paste(100000) renders '1e+05' for a double but '100000' for an integer, so a double entity_id arriving from a JSON round-trip would silently fail to match an integer one from DBI. An assertion with no evidence rows keeps its state but reports an empty sources array rather than a single all-NA phantom source, because sources is rendered directly to clients. Refs #608
bootstrap_load_modules() is the single loader for both the API and the durable worker, so one registration covers both. Files are not autodiscovered. Refs #608
) The curation forms prefill their variation-ontology picker from the entity's existing terms, so a curator editing one sentence of synopsis re-saves every pre-checked term onto a new, curator-attributed review. No user action distinguished 'I read the papers and agree' from 'I did not notice the checkbox' -- which is how machine-imported annotations became indistinguishable from curated content, one review at a time. Reconciliation is server-side and identity-aware. Three independent frontend surfaces prefill and resubmit terms (useEntityInfo, useReviewForm, useReviewApprovalActions) and only two route submission through the shared tag helper, so correctness cannot depend on a client sending the right field: the server compares the previous assertion set against the submitted set instead. The load-bearing rule is that a submitted, machine-derived term with no explicit action STAYS active_unconfirmed. The annotation does not vanish -- it stays live and stays submitted -- it is simply never silently upgraded. Confirmation becomes an act, not a side effect. Reconciliation runs inside review_write_mutate() on the same txn_conn as the connect-table write, so provenance and curated membership commit or roll back together. It is deliberately not exists()-guarded: a missing module must fail loudly rather than silently restore the bug. The risky logic is a pure planner (variation_provenance_plan_reconciliation) covering every transition, so the state machine is fully unit-testable without a database; only a thin applier touches SQL. With zero assertion rows the whole path is a strict no-op, which keeps it provably inert until the companion backfill runs. Refs #608
Review findings on the provenance foundation. attach_provenance() built its join key with paste(), which renders a missing column as an empty string. A terms tibble lacking modifier_id therefore produced keys that matched nothing and marked EVERY term as curator-authored -- silently, through nothing worse than a caller typo or a column rename. That is precisely the fabricated-provenance failure this feature exists to prevent, so it now validates the identity columns up front and stops, naming what is missing. A NULL provenance argument stays permitted (no production caller passes one, and provenance_for_entity always returns a tibble). max_strength is now explicitly integer on the non-NA branch, locked by an expect_identical assertion. normalize_evidence_strength() range-checked its input only after coercing it, so a digit string like "99999999999" or a double like 1e20 emitted "NAs introduced by coercion to integer range" on the way to the correct NA answer. It now range-checks as a double before coercing. A validator running in the request path and the durable worker should reject cleanly rather than leak a warning a tryCatch caller could escalate. Fixed by validating, not by suppressWarnings(). Refs #608
The test resolved its FK-target tables from ambient schema left behind by another test file. Run alone -- or with that file excluded or reordered -- it skipped AFTER the table-existence assertions but BEFORE the identity, CHECK and idempotency ones: green CI with zero constraint verification. It now creates ndd_entity / variation_ontology_list / modifier_list itself, and the skip path is gone. Verified by dropping all three tables first and confirming the full assertion set still runs. It also asserted no foreign key and no charset match, so deleting all four FK fragments from the migration still passed 15/15 -- which would silently un-justify the charset derivation the migration exists to get right. The two gaps masked each other. Now asserted: all five FKs via KEY_COLUMN_USAGE with their referenced table and column, charset/collation equality between variation_ontology_assertion.vario_id and variation_ontology_list.vario_id, FK rejection of a bogus entity_id, and the positive path of a confirmed row with valid attribution. The dynamic DDL embedded its ENUM/CHECK literals as double-quoted strings, which are string literals only while sql_mode excludes ANSI_QUOTES -- 047 was the first migration in the repo to depend on that. Switched to escaped single quotes and verified the migration applies under ANSI_QUOTES. The header attributed the observed utf8mb4 collation to production restore drift; it actually comes from a test fixture. Corrected, keeping the probe results and the do-not-simplify warning. Refs #608
Issue #608's provenance model rests on one invariant: absence of an assertion row means the annotation is curator-authored. That holds only while nothing writes curated variation-ontology rows without also recording provenance -- the issue's own words are that an automated process may write suggestions but may not write ndd_review_variation_ontology_connect. Policy cannot hold that on its own: the historical import scripts inserted into the connect table directly, and any future code repeating the pattern silently manufactures curated-looking data. This freezes the boundary. The sanctioned path is ontology-repository.R, reached through review_write_mutate() where reconciliation runs on the same transaction; anything else fails the guard with a message naming that path. Proven able to fail, not just to pass: an injected INSERT in a scratch file under functions/ made the guard fail, and removing it made it pass again. Matching is whitespace-normalized and case-insensitive because the SQL in this codebase is written as multi-line R string literals. Also asserts the reconciliation stays inside the connect-table write's transaction (conn = txn_conn). A refactor moving it onto its own connection would silently stop provenance and curated membership being atomic, and nothing else caught that. svc_review_add_variation_ontology() and put_post_db_var_ont_con() in review-service.R are confirmed dead (no callers) and delegate to the sanctioned repository by name rather than raw SQL, so they do not trip the guard -- but they are flagged in a comment as a bypass hazard if ever resurrected. Refs #608
…face
GET /api/entity/<id>/variation gains a per-term provenance object joined on
the full (entity_id, vario_id, modifier_id) identity, plus two new DB-only
routes: a Curator-gated per-entity suggestions read, and an evidence detail
read carrying the full evidence_json payloads.
The suggestions route is declared BEFORE the dynamic
/<vario_id>/<modifier_id>/evidence route. Plumber matches in declaration
order, so the reverse would capture the literal 'suggestions' as a vario_id
-- the same shadowing that once made GET /api/status/_list 403.
Two serializer defects found by probing the real decorator rather than
reasoning about it:
- Without null="null", a NULL list-column element serializes as {} rather
than null. The contract that provenance is null for curator-authored terms
would have shipped as "provenance":{}, quietly breaking the release gate
it exists to enforce.
- na="string" renders a nested NA_integer_ as the literal string "NA", so
an unrecorded strength would have arrived as text where a number or null
belongs. NA scalars are normalized to NULL in the service.
Plumber routes a raw colon inside a path segment fine, so the CURIE works as
a path parameter -- but it does not percent-decode path parameters, so a
client using encodeURIComponent would look up a literal 'VariO%3A0017' and
404. The parameter guard URL-decodes so both encodings resolve identically,
and a malformed escape (which only warns) becomes a 400.
Provenance costs one extra query per request, never one per term, and the
approved-review gate is unchanged.
With zero assertion rows every term reports provenance null and the response
is identical to the pre-change shape. That inertness is what makes shipping
the read path safe before the companion backfill runs, and it is locked by
test rather than left to inspection.
Refs #608
Types and typed clients for the three provenance reads, so the public card and the curation form both go through app/src/api/* rather than raw axios. provenance is typed as optional-and-nullable: null IS the contract for a curator-authored term, and the field is absent entirely on a pre-#608 API build. strength is number | null because null means not recorded, which must never render as zero stars. sources is documented as pre-ordered by the API so no consumer re-sorts it and reintroduces nondeterministic rendering. getEntityVariationEvidence deliberately does NOT encodeURIComponent the vario_id CURIE. Plumber routes a raw colon in a path segment fine but does not percent-decode path parameters, so an encoded VariO%3A0017 would arrive verbatim; the API decodes defensively, but sending it raw is what actually matches the route. VariationEvidenceRecord documents that evidence_json carries only what the import manifests contained -- no HGVS or protein labels, which were never recorded and must not be displayed or inferred. Refs #608
entity-rename-service.R copies an entity's variation-ontology terms onto a brand-new entity_id. Provenance assertions are keyed on entity_id, so the carried terms arrived with no assertion row -- and absence of an assertion row is precisely how this feature encodes 'curator-authored'. Renaming an entity therefore converted every machine-derived, unconfirmed annotation into an apparently curated one, silently. A rename is not an act of confirmation. This is a second laundering channel, independent of the review-save one, and it defeats the same invariant, so it is closed rather than deferred. Assertions and their evidence are copied preserving state, confirmed_by, confirmed_at and rejected_reason exactly: a confirmed assertion keeps its ORIGINAL curator's attribution rather than being re-attributed to whoever performed the rename. Evidence reattaches to the new assertion_id. The copy is idempotent, leaves the source entity's rows in place (the old entity may still be referenced, and its history must survive), and is a no-op returning zero when the source has no assertions -- which is every entity today, since the companion backfill has not run. The call is deliberately NOT gated on the connect-table snapshot being non-empty: assertions are keyed independently of it and can exist when it is empty, so gating would drop rows. Extracted into its own module rather than growing the reconcile file past the 600-line ceiling. An earlier pass had trimmed documentation across three attempts to squeeze under it, which trades away the readability the ceiling exists to protect. Refs #608
Two review findings, both of which let a machine-derived term end up reading
as curator-authored -- the failure this feature exists to prevent.
Reconciliation was entity-scoped, so a Reviewer's draft save that omitted
variation_ontology (or omitted the field entirely, which prepare() normalizes
to zero rows) rejected EVERY assertion on the entity while the approved
review went on serving those terms. The read path filters state IN
('active_unconfirmed','confirmed'), so the still-served terms then rendered
as curated. A draft edit laundered the entity.
The design's §5.3 row 3 mandates rejection on omission, but its primary goal
forbids a machine-derived annotation becoming curator-authored without an
explicit act. On this input the two contradict each other and the goal
governs. Confirmation transitions now always apply; REJECTION applies only
when the save actually determines the entity's served term set. Assertions
are entity-scoped, but what the public sees comes from the primary approved
review, so a draft's omission is not a statement about the served set --
deferring rejection to approval is strictly more faithful to row 3's stated
purpose. The predicate is computed in review_write_mutate() and passed in, so
the planner stays pure and DB-free.
Identity was compared case-sensitively in R while vario_id's collation is
utf8mb4_0900_ai_ci. The originally-reported path turns out unreachable (lookup
validation rejects a case variant with a 400 first, via a case-sensitive
setdiff), but the reachable direction is worse: if the backfill stores
non-canonical casing, every backfilled assertion looks omitted and gets
rejected. Both directions are now tested.
An unparseable submitted set previously degraded to empty, which reads as
'everything omitted' and pushes toward rejection. It now raises. A module
whose purpose is to stop fabricated provenance must not fail toward
destroying it.
Refs #608
A reader could not tell a curator's literature-based claim from a machine batch import, or from a machine import that had been silently promoted to curator-authored. This makes origin visible: a machine-derived term carries a quiet marker and an evidence dialog listing the actual supporting records. Quiet by design. An unconfirmed annotation is un-reviewed, not broken, so it uses the neutral chip tone plus a dotted border -- never a warning or danger tone. Saturated colour across thousands of entity pages would read as alarm and train people to ignore it. State is carried in text on the trigger's accessible name, never by glyph or colour alone, and the dialog is a labelled dialog with focus moved in, focus returned on close, and Escape to dismiss. The trigger is a separate button rather than the chip itself, so the existing external ontology outlink keeps working unchanged. Evidence is fetched on first open only and cached per entity, so the card costs nothing extra on page load. The inertness gate is the reason this is safe to ship before the companion backfill runs: with no provenance the card renders exactly as before. That is enforced structurally -- the body branches with v-if/v-else on <template> rather than per-affordance, because a falsy per-affordance v-if leaves <!--v-if--> placeholder comments in the DOM -- then proved by diffing the rendered HTML against git show HEAD: of the old component, and locked by a golden-HTML assertion plus explicit absence of every testid, button and provenance term, plus a fetch mock that must never be called. Nested scalars arrive as length-1 arrays because plumber does not auto-unbox, so each field is unwrapped explicitly and the fixtures use the real wire shape. A null strength renders as not recorded, never as zero stars, and no protein or cDNA label is ever displayed: the importer never recorded them, so showing one would be the fabrication this feature exists to prevent. Refs #608
The curation form pre-checked every existing term, so a curator editing one sentence of synopsis re-saved them all onto a new, curator-attributed review. Nothing distinguished reading the papers and agreeing from not noticing a checkbox. The picker now has three zones. Confirmed holds curator-authored and explicitly confirmed terms. Needs confirmation holds machine-derived ones: they stay selected and are still submitted, so the annotation does not vanish, but only Confirm changes their state and saving without touching them is explicitly allowed. Suggested holds candidates that are not in the entity at all, unchecked by default. Each card shows the modifier alongside the term, because present and absent are different assertions with independent state, and evidence is inline rather than behind a popover -- in the curation flow the evidence is the decision. Zone headers carry an honest count. Two terms need confirmation is actionable in a way a pre-checked box never was. Remove and Dismiss need no wire protocol: dropping a term from the submitted set is what the server records, so no client-sent rejected-terms array is invented. Only an explicit Confirm adds provenance_action, and confirming marks the form dirty; it is stripped from the draft save/restore so a localStorage restore can never re-assert a confirmation the curator did not make. Unconfirmed is styled quietly, not as an error -- saturated colour is reserved for the action buttons. Every action button's accessible name names the term it acts on rather than four identical Confirm labels. With no provenance and no suggestions the form renders the plain picker, which is production today until the companion backfill runs. Refs #608
AGENTS.md gains an Architecture Invariants section, plus a Stack-Specific
Gotchas bullet for the serializer traps this work uncovered: without
null="null" a NULL list-column serializes as {} rather than null, na="string"
renders a nested NA as the literal string "NA", plumber does not auto-unbox
nested scalars, and it does not percent-decode path parameters.
The section documents what the code does, which in two places is not what the
design document says. Rejection scope departs from the literal reading of
spec §5.3 row 3, and that departure is written down with its reasoning so
nobody restores the unconditional form and reintroduces the draft-save
laundering. The charset derivation in migration 047 carries the same kind of
warning against being simplified back.
Residuals are recorded rather than omitted: the feature is inert until the
companion backfill runs and a partial backfill is worse than none; two
curation surfaces still lack the deliberate-act UI and are protected
server-side instead; edit-then-approve-separately never rejects; the
evidence_json key names are a contract the other repo must honour; and the
dead connect-table writers must not be re-wired.
08-development covers how to get provenance rows locally and what each test
file guards. 09-deployment covers the backfill gate, what an operator should
see once it lands, and that no new env vars, secrets or egress are involved.
Refs #608
partitionVariationZones() decided the zone with `provenance?.state === 'active_unconfirmed'`, but plumber does not auto-unbox, so the real payload is `"state":["active_unconfirmed"]`. Strict equality against a length-1 array is false, so every machine-derived unconfirmed term was misfiled into Confirmed and the Needs-confirmation zone rendered EMPTY -- the deliberate review step this whole feature exists to force was invisible. Observed live before the fix: confirmed=3 (including the unconfirmed term), needs-confirmation=0, suggested=2. After: 2 / 1 / 2. All 44 existing unit tests passed both before and after, because every fixture used plain scalars rather than the wire shape. The four new tests use the verbatim real payload so this cannot regress. maxStrength, strength, summary and source_key are hardened the same way -- they were array-typed too and only worked by JS coercing a length-1 array in template and Number() contexts, where strict === does not. The public card was never affected; it already unwrapped field by field. Refs #608
Radius and surface literals replaced with the existing tokens, and the legend prose left-aligned rather than centred, per documentation/10-visual-design-guide.md. Refs #608
…wser The provenance tables are empty in every environment until the companion backfill runs, so without seeded rows there is nothing to exercise and both the specs and the design review would be vacuous. The seed is shaped to hit every branch at once: a confirmed term with two evidence sources (the second with a NULL strength, which must read "Not recorded" and sort last), the weak-evidence active_unconfirmed case from the issue, and a suggested term with no connect row. VariO:0001 deliberately has NO assertion row -- that absence is the curator-authored control, and a SQL comment says so, so nobody "completes" the fixture later. confirmed_by is resolved by user name rather than hardcoded, because user_id is AUTO_INCREMENT. (VariO:0017, modifier 5) is seeded in a different state from modifier 1 on purpose: it makes the identity invariant visible in a real browser, with the same CURIE in two zones simultaneously. Keyed on vario_id alone the two collapse and confirming one would confirm the other, so it also stops the monkey pass's "no term in two zones" invariant from passing vacuously. The public spec asserts the wire contract, the affordance appearing on only machine-derived terms, the unchanged ontology outlink, lazy fetch with no request on page load and no refetch on reopen, the absence of any HGVS or protein label, full keyboard operation, no overflow at 390 and 1440, and axe. The curate spec asserts zone membership and counts, the browser-visible identity invariant, per-term accessible names, and a seeded monkey pass that re-checks its invariants after every interaction. Three curate tests are gated on a runtime probe and skip while #613 is open -- they re-activate automatically once a review save succeeds. Refs #608
Pre-existing P0, live since v0.29.3 and unrelated to #608 -- fixed here because #608's provenance reconciliation runs inside this same transaction, so Confirm, Accept and reject-by-omission were all dead in production behind it. The endpoint takes the request body verbatim, and review_json always carries literature, phenotypes and variation_ontology (the handler reads them off the same object to pass separately). review_write_mutate() forwarded the whole object to review_update(), whose mass-assignment allowlist then aborted with "Disallowed review field(s)" and rolled the transaction back. A curator lost their work to an opaque 500. The allowlist's own comment claims the update path "passes a fixed-column tibble (synopsis/comment)" -- that assumption had gone stale, and its being stale is the bug. The save path now projects the body to synopsis and comment before it reaches review_update(). That is deliberately narrower than review_update()'s own allowlist, and the narrowness is the second half of the fix: the allowlist legitimately permits is_primary, review_approved and approving_user_id because svc_review_update() and the approval path need them, so forwarding a client-supplied review_json into it let a caller smuggle review_approved past the Curator gate that /api/review/approve enforces. Latent rather than exploited, since any such payload also carried the ontology keys and 400'd first. review_update()'s allowlist and review_create() are untouched -- POST does not share the allowlist, which is why entity creation still worked and this stayed hidden. The existing integration tests missed it because they built review_data from allowlisted columns only; the new tests use the endpoint's real payload shape and assert the escalation case against the stored row rather than against the absence of an error. Closes #613 Refs #608
variation_provenance_plan_reconciliation() built its result with
tibble(from_state = from_state[changed],
to_state = to_state[changed],
needs_attribution = to_state[changed] == 'confirmed' & ...)
tibble() evaluates its arguments sequentially with data masking, so by the
needs_attribution line to_state and from_state no longer referred to the outer
vectors -- they referred to the columns just created, already subsetted to
sum(changed) elements. Re-indexing those with the full-length changed mask read
out of bounds and produced NA.
The applier coerced that NA to FALSE, so it wrote state='confirmed' while
leaving confirmed_by NULL, and migration 047's chk_confirmed_attribution
rejected the row -- an opaque 500 that rolled the whole review save back. Every
Confirm and every Accept was broken, verified in a browser against the real
stack.
It only misbehaves when at least one assertion is UNCHANGED, which is the
normal case. When every row changes, the column length equals length(changed)
and the mask is all-TRUE, so the expression is accidentally correct -- which is
why a 902-line unit suite passed. The new tests exercise mixed changed and
unchanged sets and assert needs_attribution identically TRUE, plus anyNA(plan)
is false as a general invariant, since that is the property that was violated.
Refs #608
The sibling fix projected the PUT path's body down to synopsis and comment, but POST still forwarded the raw review_json into review_create(), so a Reviewer could submit is_primary, review_approved and approving_user_id and publish an approved primary review without passing the Curator gate that /api/review/approve enforces. Post-#608 that is worse than it looks: review_write_save_determines_served_set() consults exactly those columns, so a forged approved primary review would also flip the provenance rejection gate and let a submission reject an entity's assertions. The create path is now restricted to the columns a submission may set -- entity_id, review_user_id, synopsis, comment. direct_approval still approves, via review_approve() afterwards, where the role check lives. The new test asserts a POST attempting to set the approval columns stores the review unapproved and non-primary, reading the row back rather than trusting the absence of an error. Refs #608
…tion attach_provenance() compared vario_id case-sensitively while variation_ontology_list.vario_id collates utf8mb4_0900_ai_ci, so the FK accepts an assertion stored as 'vario:0017' against a term served as 'VariO:0017'. The write path was already normalized for exactly this reachable direction -- the companion backfill lives in another repository and may store non-canonical casing -- and the read path was not, so the two halves of the feature disagreed. The consequence is the fabrication this feature exists to prevent: a mis-cased assertion yields provenance: null, which the contract defines as curator-authored, so the public card showed it as curated and the picker filed it under Confirmed -- while the evidence route, which resolves in SQL and is therefore case-insensitive, still returned its machine-derived evidence. Normalized locally rather than by calling the write-path helper, deliberately: the endpoint tests source only this module, and the write-path form as.integer(trimws(as.character(x))) would have reintroduced a real bug, since as.character(100000) is '1e+05' and would silently split the entity_id join that an existing large-id test guards. Both key builds here now share one helper, with roxygen cross-referencing the write path. Also adds the static assertion the rename call site never had: entity-rename- service.R must call variation_provenance_carry_forward_entity with the rename transaction's connection. That call site was only parse()-verified, and a dropped conn argument would silently lose provenance on a renamed entity. Proven able to fail in both directions before being restored. Refs #608
Plumber does not auto-unbox, so every scalar the R services build with list() arrives as a length-1 array. The client declared them unboxed and returned the raw payload, so TypeScript lied to every consumer -- and that is exactly how the Needs-confirmation zone came to render empty: a strict-equality predicate was silently false against ['active_unconfirmed']. The three provenance reads now normalize the wire shape in the client, so the declared types are accurate at the one documented boundary rather than being patched up per consumer. The normalization is idempotent, so the two consumers' existing defensive unwrapping keeps working and becomes a harmless no-op. null is preserved exactly: provenance stays null (the curator-authored contract) and an unrecorded strength stays null rather than becoming 0. evidence_json is deliberately untouched -- it is typed unknown because its inner shape is a cross-repo contract the dialog probes by alias, and normalizing it could silently change what the dialog finds. Refs #608
A review save resets review_approved = 0, so a term vanishes from the public read until it is approved again. The write-path tests now re-approve through PUT /api/review/approve and read back through the public endpoint, so each assertion travels the same path a reader does instead of peeking at the database. Also records, in a comment rather than an assertion, that apply_rejections is computed after review_update() has already reset the approval flag, so on the plain PUT path reject-by-omission never fires and is reachable only via direct_approval. That is consistent with the module's own "removal becomes real when it becomes public" rationale, but whether it is the intended end state is a product decision rather than a test one. Refs #608
Two layout defects found by looking at the rendered dialog rather than the code. The dialog is rendered from inside the entity evidence card, which centres its text, so every free-flowing paragraph inherited that centring while the label/value rows only LOOKED aligned because they are grids. The panel ended up with four competing alignments: right-aligned labels, left-aligned values, centred summary/heading/matched-via prose, and record rows at a fourth x-position. Centred prose inside a left-aligned data panel reads as accidental, so the dialog now establishes its own alignment instead of inheriting the card's. Record rows used the default stretch alignment. A row mixes a monospace id with smaller prose, and at 390px the prose wraps and the row grows -- measured, all three children became 44px tall and the consequence text rendered ABOVE its own id. They now share a baseline, which is what a reader expects of a text row. The 44px row height itself is deliberate and left alone: it comes from the global mobile tap-target rule, so each ClinVar link is a real touch target and the spec's axe target-rule assertion depends on it. Also fixes a fixture stutter -- source_version '2026-01 release' rendered as 'release 2026-01 release' under the dialog's own Imported label -- and makes the public spec reseed once before it runs. The sibling curation spec accepts a suggested term and saves reviews, which adds curated rows and re-inserts the rest with fresh ids, so run after it this read-only spec saw a suggested term in the curated set. That is order-coupling, not a product defect. Refs #608
Three test files pinned EXPECTED_LATEST_MIGRATION and EXPECTED_MIGRATION_COUNT to 046 / 44L, so adding migration 047 failed seven assertions across them. The manifest is a single source of truth and every migration legitimately moves it; these are the assertions that keep it honest. Two of the test names hardcoded '046' as well, which would have gone stale again on the next migration, so they now describe the invariant (the manifest tracks the LATEST migration) rather than a specific number. The separate test that reads migration 046's own file to assert its generator_json column still points at 046, because that one is about that migration's contents rather than the manifest. Caught by CI rather than locally because I had been running the provenance test files directly instead of the gate. make test-api-fast now reports FAIL 0 | PASS 9071. Refs #608
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements issue #608 (design revision 2, externally reviewed) for the application repo, plus two
pre-existing defects that blocked it.
The problem
SysNDD presents itself as an expert-curated resource, but the curation forms prefill their
variation-ontology picker from the entity's existing terms. A curator opening an entity to change one
sentence of synopsis got every existing term pre-checked, and saving rewrote all of them onto a
new, curator-attributed review. No user action distinguished "I read the papers and agree" from
"I did not notice the pre-checked box."
The 8,111 annotations written straight into the curated table by the February 2026 imports were
therefore one re-review away from being indistinguishable from curated content — and 1,981 of the
ClinVar batch rest on 1-star evidence alone.
What ships
047) — one assertion per(entity_id, vario_id, modifier_id), evidence rows per source batch. Deliberately not a column onthe review-linked join table, which is deleted and re-inserted wholesale on every review save.
no explicit confirmation stays unconfirmed. Confirmation is now an act, not a side effect.
provenance, an on-demand evidence route, and a Curator-gatedsuggestions route.
keyboard-operable evidence dialog.
card and evidence inline, because in the curation flow the evidence is the decision.
sanctioned path.
Release gate — read before deploying
The feature is inert until the companion backfill runs. That backfill lives in
sysndd-administrationand covers all three February batches. Until then there are zero assertionrows, every term reports
provenance: null, and the public card renders exactly as before. Apartial backfill is worse than none: because absence of an assertion row means curator-authored,
it would positively present the un-backfilled batches as curated (design §7.1).
That inertness is enforced structurally and locked by a golden-HTML assertion, not left to
inspection. The write-side fix is the opposite — ungated and valuable immediately, since every review
saved without it makes one more entity permanently ambiguous.
Deviations from the design, each deliberate
vario_id's charset frominformation_schemainstead of the specifiedstatic
utf8mb3. The design's SQL provably cannot apply: the referenced column has drifted toutf8mb4and MySQL refuses the FK as incompatible. Verified both ways.rejected every assertion on the entity while the approved review kept serving those terms — making
them read as curator-authored. On that input the spec contradicts its own primary goal, and the
goal governs: confirmation always applies, rejection only when the save determines the served set.
Beyond the design: three more fabrication paths closed
The design describes one laundering channel. Auditing found three more, each of which would have let
a machine-derived annotation read as curator-authored:
attach_provenance()mark every term curator-authored.vario_id— R compared case-sensitively, the DB collation does not.entity_idand stripped provenance wholesale.Two pre-existing defects fixed here
PUT /api/review/updatereturned an opaque 500 for every payload the frontendsends, since v0.29.3; curators lost their work. Provenance reconciliation shares that transaction,
so Variation ontology provenance: mark machine-derived annotations and stop silent promotion to curator-authored #608's entire curation write path was dead behind it. Narrowing the save to
synopsis/commentalso closes a latent path for smuggling
review_approvedpast the Curator gate — and the samenarrowing is applied to the create path.
publication.Lastname VARCHAR(50)overflows on PubMed consortiumauthors and rolls back the whole save.
Caught only by running it in a browser
Two defects that unit tests with mocks could not reach, both in this branch's own code:
provenance?.state === 'active_unconfirmed'was false against["active_unconfirmed"]and everyunconfirmed term was misfiled as Confirmed — the deliberate review step the feature exists to force
was invisible. All 44 unit tests passed either way, because their fixtures used plain scalars. The
typed client now normalizes the wire shape so its declared types are actually true.
tibble()evaluates arguments sequentiallywith data masking, so
needs_attributionre-indexed already-subsetted columns and yieldedNA;the applier then wrote
confirmedwith a NULLconfirmed_byand the CHECK constraint rejected it.It only misbehaves when some assertion is unchanged — the normal case — which is exactly why a
902-line unit suite passed.
Verification
single failure is the pre-existing
/DataReleasescase (this stack has no snapshots and no worker)and the 3 skips are the documented env-gated baseline
make code-quality-audit,make lint-api,make lint-app(0 errors),npm run type-check,npm run type-check:strict— all cleanThe end-to-end proof that matters: saving without touching an unconfirmed term leaves it live and
unpromoted, verified in a real browser, reading back through the public endpoint.
Design
Unconfirmed is styled as information, not an error — neutral tokens and a dotted treatment, zero
--status-danger/--status-warninguse, state always carried in text, axe clean, no overflow at 390or 1440. Two layout defects were found by looking at the rendered dialog: four competing alignments
(inherited centring from the card) and record rows on mismatched baselines at mobile widths. Both
fixed; the 44px row height was left alone after verifying it is the deliberate tap-target rule.
Closes #608
Closes #613
🤖 Generated with Claude Code
https://claude.ai/code/session_01W7njywYE4bCjoJ6YedziVQ