fix: consortium bylines, hyphenated CURIEs, and the evidence import date - #616
Merged
berntpopp merged 6 commits intoJul 30, 2026
Merged
Conversation
…/modify
The curation form encodes a selection as "<modifier_id>-<ontology_CURIE>", and
two of the four surfaces that resubmit those tags split them with
`item.split('-')[1]` (or the equivalent array destructuring), which returns only
the segment between the FIRST and SECOND hyphen. So
'5-CURIE:part-with-hyphen' was submitted as 'CURIE:part' -- either a different
existing term, or a non-existent one that fails the connect step. Either way the
curator's annotation is silently not the one they picked.
No identifier in either vocabulary contains a hyphen today (HPO ids are HP: plus
seven digits, VariO ids VariO: plus four), so this is a latent trap rather than
observed corruption. It goes live the moment a hyphen-bearing id enters either
list, and it fails quietly when it does, which is why it is worth closing now
rather than after the first wrong annotation.
splitOntologyTag() splits on the first separator only and exists precisely for
this -- it is the helper #600 introduced, and useReviewForm/
useReviewApprovalActions already route through it. These were the two call sites
that were missed.
modifier_id now arrives as a number rather than the raw string half of the tag,
which is what the review form has always submitted; the API coerces either.
The regression tests assert through the actual request body, because a test
using a simple '1-VariO:0015' passes against the broken code and proves nothing.
The pre-existing fixtures used 'present-HP:1', a modifier shape the form never
produces, and are corrected to the real wire shape.
Closes #611
…cannot roll back a review save PubMed records a corporate author -- Deciphering Developmental Disorders Study, Autism Sequencing Consortium, Epi4K -- in <CollectiveName> rather than as a surname, and consortium-authored papers are ordinary in NDD genetics. Those names routinely exceed the 50 characters publication.Lastname allowed. Because publication ingestion runs inside the review-save transaction, the resulting "Data too long for column 'Lastname' at row 1 [1406]" did not fail in isolation: it rolled back the entire save and the curator lost their work behind an opaque 500. Both author columns are exposed, not just the one named in the error. With no surname to fall back on the parser assigns the collective name to Lastname AND Firstname, so widening only Lastname would have left the identical failure one column over. Migration 048 widens both to VARCHAR(255). Widened rather than truncated: the byline is citation metadata, and silently shortening an author name to protect a column width is the wrong trade for a curation database. pubmed_clamp_author_name() stays as a backstop for an upstream string longer than even 255 -- losing the tail of a byline beats losing the save -- and it warns when it fires, so a systematically over-long source shows up in the logs instead of quietly corrupting names. It clamps at the width the migration sets; a test pins the two together. The migration re-states each column's own charset, read back from information_schema, rather than naming one. ALTER TABLE ... MODIFY COLUMN does NOT preserve a column's character set when the statement omits it -- it falls back to the TABLE default -- so the obvious one-liner would silently downconvert a column that had drifted to utf8mb4. Same family as the disease_ontology_mapping collation trap. Each ALTER is guarded on the live width, so a rerun no-ops. Four test files pin the migration manifest and move with it. The assertions that read migration 047's own file still point at 047; only the manifest pair advances. One test name that hardcoded '047' now describes the invariant instead, so it does not go stale on the next migration. Closes #614
…hen a term was imported The provenance dialog was designed to read "Imported 15 Feb 2026 · batch clinvar-2026-02" but rendered the batch alone, because the evidence route never selected variation_ontology_evidence.created_at. Both files carried a comment justifying the missing date as an honesty rule; those comments now describe what is served instead of what is absent. Both evidence-bearing queries select the column, aliased as evidence_created_at. Aliased because the suggestions query also selects the ASSERTION's own a.created_at and two same-named columns collide in one result set. Both, because the two routes share .svc_vp_evidence_records() -- a caller that forgets the column does not degrade to a missing field, it errors on NULL[[i]]. That is the right direction for this surface and is now pinned by a test that asserts the column onto both SQL statements. A date, never a time. The column is a MySQL DATETIME and carries no timezone, so the API sends no zone designator, and formatImportedDate() reads the calendar fields off the leading YYYY-MM-DD literally rather than handing the string to new Date() -- which would have the engine assume a timezone and could shift the displayed day for a viewer elsewhere. A value it cannot parse, or one that would roll over (2026-02-31), returns null and the date is omitted rather than displayed as something the payload never recorded. The Imported line is assembled by importedLineParts() as date · batch · release, each part dropped independently when null and the row omitted entirely when all three are, rather than by nested v-ifs hand-managing separators. Refs #612
svc_review_add_phenotypes, svc_review_add_variation_ontology, put_post_db_phen_con and put_post_db_var_ont_con have no callers -- a repo-wide search, including dynamic dispatch by name, finds none outside their own definitions and archived planning docs. They are worth removing rather than leaving inert. Each delegates to the sanctioned repository functions BY NAME instead of embedding SQL, so they pass the static write-guard while bypassing review_write_mutate() -- the one place provenance reconciliation runs. Wiring any of them to an endpoint would write curated ontology terms with no assertion-state transition, so a machine-derived annotation would read as curator-authored again: #608's laundering bug through a door the text scan cannot see. The guard file already said as much and called deletion the right outcome. The phenotype pair goes too. It is the same dead code in the same file, and removing only the variation pair would leave two identical functions behind with no explanation for why one went and the other stayed. A third guard in test-unit-variation-connect-write-guard.R keeps them deleted, and records that a resurrection is legitimate only if it routes through review_write_mutate(). Refs #612
… the deletion Three repository invariants a future change could plausibly undo: The publication author columns are VARCHAR(255), not the base schema's 50, and BOTH are exposed because the parser writes a collective name into each. The migration re-states each column's own charset for the same reason 047 does, and the parser's clamp constant must stay in step with the column. The evidence import date is selected on BOTH provenance queries and aliased, because the suggestions query already selects the assertion's own created_at and the two routes share one record builder. It is rendered as a date, not an instant, because the column carries no timezone. The four dead connect helpers are gone, and the note now says what a legitimate resurrection would look like rather than describing them as live hazards. Also drops the stale #611 residue note: all four surfaces now use splitOntologyTag(). Refs #611, #612, #614
berntpopp
deleted the
fix/ontology-tag-truncation-611-publication-author-614
branch
July 30, 2026 18:05
10 tasks
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.
Three follow-up defects from the #608 audit, each with a verified reproduction.
#614 — a consortium byline destroyed the curator's entire review save
PubMed records a corporate author (Deciphering Developmental Disorders Study, Autism Sequencing Consortium, Epi4K) in
<CollectiveName>rather than as a surname, and consortium-authored papers are ordinary in NDD genetics. Those names routinely exceed the 50 characterspublication.Lastnameallowed. Because publication ingestion runs inside the review-save transaction,Data too long for column 'Lastname'did not fail in isolation — it rolled back the whole save and the curator lost their work behind an opaque 500.Both author columns were exposed, not just the one in the error: with no surname to fall back on, the parser assigns the collective name to
LastnameandFirstname. Migration048widens both toVARCHAR(255).Widened rather than truncated — the byline is citation metadata, and silently shortening an author name to protect a column width is the wrong trade for a curation database.
pubmed_clamp_author_name()remains as a backstop for an upstream string longer than even that, and warns when it fires.The migration re-states each column's own charset read from
information_schemarather than naming one:ALTER TABLE … MODIFY COLUMNdoes not preserve a column's charset when the statement omits it — it falls back to the table default — so the obvious one-liner would silently downconvert a column that had drifted toutf8mb4. Same family as thedisease_ontology_mappingcollation trap.#611 — hyphenated ontology CURIEs were truncated on entity create/modify
'5-CURIE:part-with-hyphen'.split('-')[1]is'CURIE:part'. Two of the four surfaces that resubmit curation tags used that form, so the submittedphenotype_id/vario_idwas either a different existing term or a non-existent one that fails the connect step.Honest scoping: no identifier in either vocabulary contains a hyphen today (HPO ids are
HP:+ 7 digits, VariO idsVariO:+ 4), so this was a latent trap rather than observed corruption. It goes live the moment a hyphen-bearing id enters either list, and it fails silently when it does.All four surfaces now use
splitOntologyTag()— the helper #600 introduced for exactly this. The regression tests assert through the actual submitted request body, because a test using a simple1-VariO:0015passes against the broken code and proves nothing. Proven red first:#612 — the evidence import date (one item only)
The dialog was designed to read
Imported 15 Feb 2026 · batch clinvar-2026-02but showed the batch alone, because the route never selectedvariation_ontology_evidence.created_at.Both evidence-bearing queries now select it, aliased — the suggestions query already selects the assertion's own
a.created_at, and two same-named columns collide in one result set. Both, because the two routes share.svc_vp_evidence_records(); a caller that forgets the column errors rather than silently omitting provenance, which is the right direction here and is now pinned by a test.A date, never a time: the column is a MySQL
DATETIMEwith no timezone, so the API sends no zone designator and the frontend reads the calendar fields literally instead of parsing an instant, which would let a viewer's timezone shift the day.Also removes four dead
review-service.Rhelpers (the two named in #612 plus their equally-dead phenotype siblings). They delegate to the sanctioned repository functions by name rather than embedding SQL, so they passed the static write guard while bypassingreview_write_mutate()— the one place provenance reconciliation runs. A new guard keeps them deleted.#612 stays open — the rest of it is genuinely blocked on the administration-repo backfill.
Verification
make code-quality-auditmake lint-apimake lint-appnpm run type-check+:strictnpx vitest runmake test-api-fastnpx playwright test --workers=1The single Playwright failure is the known baseline exception (
analyses.data-releases— that stack has no snapshots and no worker); the 3 skips are the documented env-gated ones.Migration
048verified applied against the live Playwright stack: both columns255, charset preserved asutf8mb3.Closes #611
Closes #614
🤖 Generated with Claude Code
https://claude.ai/code/session_01W7njywYE4bCjoJ6YedziVQ