Restore curation the editor's stale saves reverted, and fail CI on deletions

.github/scripts/validate_mappings.py

@@ -47,6 +47,7 @@
     "mapping_justification",
     "author_id",
     "mapping_date",
+    "comment",
 ]
 EQUIV_COLUMNS = [
     "source_prefix",
@@ -59,6 +60,13 @@
     "source",
 ]
 
+# A reversed judgment keeps BOTH rows: the editor app annotates the withdrawn one
+# in `comment` instead of deleting it, so a consumer can see which of two
+# contradictory rows was withdrawn without reimplementing the ordering. Kept in
+# step with `SUPERSEDED_PREFIX` in the app's `app/sssom_service.py`. Only rows
+# without this marker count as live judgments.
+SUPERSEDED_MARKER = "Superseded by the "
+
 ALLOWED_PREDICATES = {"skos:exactMatch"}
 ALLOWED_JUSTIFICATIONS = {"semapv:ManualMappingCuration", "semapv:LexicalMatching"}
 ALLOWED_MODIFIERS = {"", "Not"}
@@ -133,7 +141,10 @@
 
 ARI_SUBJECT_RE = re.compile(r"ARI:\d{4,7}")
 AUTHOR_RE = re.compile(r"github:[A-Za-z0-9](?:[A-Za-z0-9-]{0,37}[A-Za-z0-9])?")
-DATE_RE = re.compile(r"\d{4}-\d{2}-\d{2}")
+# SSSOM types `mapping_date` as a date, but the editor app publishes a full ISO
+# 8601 timestamp because two judgments on one pair in one day need an order.
+# Both are accepted; the date part is what the check is really about.
+DATE_RE = re.compile(r"\d{4}-\d{2}-\d{2}(?:T\d{2}:\d{2}:\d{2}(?:Z|[+-]\d{2}:\d{2})?)?")
 ICD9_RE = re.compile(r"\d{2,3}(\.\d{1,2})?")
 
 ENTITY_OPEN_RE = re.compile(r"<owl:(?:NamedIndividual|Class)\b")
@@ -289,7 +300,11 @@ def load_ontology(report: Report) -> dict[str, Disease] | None:
     except ElementTree.ParseError as exc:
         report.error("ontology-not-well-formed", ONTOLOGY_PATH, 0, f"OWL/XML does not parse: {exc}.")
         return None
+    return parse_ontology(text, report)
+
 
+def parse_ontology(text: str, report: Report | None = None) -> dict[str, Disease]:
+    """Diseases keyed by ARI id, from already-read OWL text."""
     diseases: dict[str, Disease] = {}
     current: dict | None = None
     for index, line in enumerate(text.replace("\r\n", "\n").split("\n"), start=1):
@@ -307,12 +322,14 @@ def load_ontology(report: Report) -> dict[str, Disease] | None:
             ari_id = ids[0][0] if ids else None
             if ari_id and ari_id.startswith("ARI:"):
                 if ari_id in diseases:
-                    report.error(
-                        "duplicate-ari-id",
-                        ONTOLOGY_PATH,
-                        ids[0][1],
-                        f"{ari_id} is used by more than one entity, so mappings for it are ambiguous.",
-                    )
+                    if report is not None:
+                        report.error(
+                            "duplicate-ari-id",
+                            ONTOLOGY_PATH,
+                            ids[0][1],
+                            f"{ari_id} is used by more than one entity, so mappings for it "
+                            "are ambiguous.",
+                        )
                 else:
                     diseases[ari_id] = Disease(ari_id, current["label"], dict(current["annotations"]))
             current = None
@@ -408,9 +425,11 @@ def check_sssom_rows(rows: list[Row], report: Report) -> None:
         mapping_date = fields["mapping_date"]
         if not DATE_RE.fullmatch(mapping_date):
             report.error(
-                "date-format", SSSOM_PATH, line, f"`mapping_date` {mapping_date!r} is not ISO YYYY-MM-DD."
+                "date-format", SSSOM_PATH, line, f"`mapping_date` {mapping_date!r} is not an ISO 8601 date or timestamp."
             )
-        elif mapping_date > today:
+        elif mapping_date[:10] > today:
+            # Compare the date part only: a timestamp sorts after the bare date it
+            # falls on, so the whole string would read as tomorrow.
             report.error(
                 "date-future",
                 SSSOM_PATH,
@@ -431,17 +450,20 @@ def check_sssom_rows(rows: list[Row], report: Report) -> None:
             )
         else:
             seen[key] = line
-        modifiers_by_pair[(subject, object_id)][modifier] = line
+        superseded = fields["comment"].startswith(SUPERSEDED_MARKER)
+        modifiers_by_pair[(subject, object_id)][modifier] = (line, superseded)
 
     for (subject, object_id), by_modifier in modifiers_by_pair.items():
-        if len(by_modifier) > 1:
-            lines = ", ".join(str(by_modifier[m]) for m in sorted(by_modifier))
+        live = sorted(line for line, superseded in by_modifier.values() if not superseded)
+        if len(live) > 1:
+            lines = ", ".join(str(line) for line in live)
             report.error(
                 "contradiction",
                 SSSOM_PATH,
-                min(by_modifier.values()),
+                live[0],
                 f"{subject} -> {object_id} is recorded as both confirmed and flagged-wrong "
-                f"(lines {lines}). One of the two judgments has to go.",
+                f"(lines {lines}) with neither row marked superseded. A reversal must annotate "
+                f"the withdrawn row in `comment`; otherwise one of the two judgments has to go.",
             )
 
     for subject, by_label in labels.items():
@@ -878,11 +900,116 @@ def baseline_lines(ref: str, path: str) -> set[str] | None:
     return set(result.stdout.decode("utf-8", "replace").replace("\r\n", "\n").split("\n"))
 
 
-def current_lines(path: str) -> list[str]:
+def current_text(path: str) -> str:
     full = os.path.join(REPO_ROOT, path)
     if not os.path.exists(full):
-        return []
-    return open(full, encoding="utf-8", errors="replace").read().replace("\r\n", "\n").split("\n")
+        return ""
+    return open(full, encoding="utf-8", errors="replace").read().replace("\r\n", "\n")
+
+
+def current_lines(path: str) -> list[str]:
+    return current_text(path).split("\n")
+
+
+# Curation records that only ever accumulate. Nothing a curator decides removes
+# one, so a branch that drops one is reverting somebody rather than reviewing.
+APPEND_ONLY_PROPERTIES = {
+    "ARI_Synonym": "synonym",
+    "ARI_ClinicalSubtype": "clinical subtype",
+    "ARI_ChangeLog": "changelog entry",
+}
+# How many deleted values to name before the message just gives the count.
+DELETION_SAMPLE = 3
+
+
+def _values(disease: Disease, prop: str) -> set[str]:
+    out = set()
+    for value, _ in disease.annotations.get(prop, []):
+        out.update(part.strip() for part in value.split(",") if part.strip())
+    return out
+
+
+def summarise(values: set[str]) -> str:
+    shown = sorted(values)[:DELETION_SAMPLE]
+    rendered = ", ".join(repr(v if len(v) <= 60 else v[:57] + "...") for v in shown)
+    extra = len(values) - len(shown)
+    return rendered + (f" and {extra} more" if extra else "")
+
+
+def check_deletions(ref: str, sssom_rows: list[Row], report: Report) -> None:
+    """Report curation this branch removes from the ontology without reviewing it.
+
+    The row checks only see rows that exist, so a save that reverts somebody
+    else's work passes them all. This is the check that fails on absence.
+
+    A cross-reference may legitimately go: flagging one wrong on the review page
+    is exactly how a bad code is retired, and that judgment is in the mapping set.
+    Anything else -- a synonym, a subtype, a changelog entry, or an id no curator
+    ruled against -- has no decision behind its removal.
+    """
+    result = subprocess.run(
+        ["git", "show", f"{ref}:{ONTOLOGY_PATH}"],
+        cwd=REPO_ROOT,
+        capture_output=True,
+    )
+    if result.returncode != 0:
+        return  # the ontology is new on this branch; nothing to have deleted
+    before = parse_ontology(result.stdout.decode("utf-8", "replace"))
+    after = parse_ontology(current_text(ONTOLOGY_PATH))
+    if not before or not after:
+        return
+
+    flagged = collections.defaultdict(set)
+    for row in sssom_rows:
+        if row.fields["predicate_modifier"].strip() != "Not":
+            continue
+        object_id = row.fields["object_id"].strip()
+        if ":" in object_id:
+            prefix, local = object_id.split(":", 1)
+            flagged[(row.fields["subject_id"].strip(), prefix)].add(local)
+
+    for ari_id, was in sorted(before.items()):
+        now = after.get(ari_id)
+        if now is None:
+            report.error(
+                "disease-deleted",
+                ONTOLOGY_PATH,
+                0,
+                f"{ari_id} ({was.label!r}) is in {ref} but not in this branch. A disease is "
+                "retired by setting ARI_Obsolete, never by deleting the individual.",
+            )
+            continue
+
+        for prop, noun in APPEND_ONLY_PROPERTIES.items():
+            lost = _values(was, prop) - _values(now, prop)
+            if lost:
+                report.error(
+                    "record-deleted",
+                    ONTOLOGY_PATH,
+                    0,
+                    f"{ari_id} loses {len(lost)} {noun}(s) this branch did not add: "
+                    f"{summarise(lost)}. {prop} is an append-only record — restore the "
+                    f"value, or say in review why it is being withdrawn.",
+                )
+
+        for prefix, properties in ONTOLOGY_PROPERTIES.items():
+            was_ids = set().union(*(_values(was, p) for p in properties))
+            now_ids = set().union(*(_values(now, p) for p in properties))
+            lost = was_ids - now_ids - flagged[(ari_id, prefix)]
+            # A value that is not a well-formed identifier for its vocabulary was
+            # never a usable cross-reference: an ICD-9 code under ICD-10, a range,
+            # a doubly-prefixed CURIE. Dropping or re-spelling one is a repair, and
+            # the shape checks already report it if it is still there.
+            lost = {value for value in lost if ID_PATTERNS[prefix].fullmatch(value)}
+            if lost:
+                report.error(
+                    "xref-deleted",
+                    ONTOLOGY_PATH,
+                    0,
+                    f"{ari_id} loses {prefix} {summarise(lost)} with no matching "
+                    f"`predicate_modifier: Not` row in {SSSOM_PATH}. Flag the id wrong on the "
+                    "review page so the judgment is recorded, or restore it.",
+                )
 
 
 def filter_to_changes(findings: list[Finding], ref: str) -> list[Finding]:
@@ -983,7 +1110,13 @@ def main() -> int:
     scope = "whole repository"
     if args.since:
         findings = filter_to_changes(findings, args.since)
-        scope = f"lines changed since `{args.since}`"
+        # Deletions are reported after the diff filter, not through it: the filter
+        # keeps findings that sit on a changed line, and a deleted record has no
+        # line left to sit on.
+        deletions = Report()
+        check_deletions(args.since, sssom_rows, deletions)
+        findings = sorted(findings + deletions.findings, key=Finding.sort_key)
+        scope = f"changes since `{args.since}`"
 
     for finding in findings:
         if args.annotate:

changelog.md

@@ -1,5 +1,72 @@
 # Changelog
 
+## restore-overwritten-curation
+
+- Restores curation that the editor app's saves reverted, and re-applies the cleanups they
+  undid. The audit goes from **66 errors** back to **0 errors, 6 warnings**, and every
+  confirmed mapping is now stored on its disease — `confirmed-not-stored` is at zero for the
+  first time since the check was written.
+- **The cause is not curation.** An editor save writes the whole ontology from a copy loaded
+  when the session started, so it reverts anything merged into the branch since. `0f03b91` is
+  labelled a review of one disease, ARI:0001143, and changed 453 lines. `295fb23` deleted a
+  confirmation three seconds after `17e616c` merged it in cleanly. Two curators' saves on
+  17 August landed on byte-identical stale content, which points at a shared server-side copy
+  rather than per-user browser state. **The fix for that belongs in
+  [`KrishnaTO/ARI-metadata-manager`](https://github.com/KrishnaTO/ARI-metadata-manager) and is
+  not in this change** — until a save applies a diff instead of a snapshot, the next publish
+  can revert this one.
+- **Restored 95 changelog entries, 10 synonyms and 17 clinical subtypes** from the merged
+  history, plus **208 synonyms and 57 clinical subtypes** from PR #69, the last commit before
+  the 17 August cliff. Only commits reachable from `main` were read, so nothing arrives from a
+  branch that was never accepted. Synonyms 490 → 708, clinical subtypes 355 → 429, recorded
+  cross-reference reviews 194 → 305. Verified afterwards: every distinct (disease, author,
+  review) record that ever reached `main` is present, and none was invented — 603 of 603.
+- **Stored 30 confirmed cross-references that had never been written to a disease**, across
+  16 diseases — 12 MONDO, 10 Orphanet, 3 UMLS and one each of DOID, NCIt, MeSH, ICD-10 and
+  OMIM. Confirming a term only ever affirmed an id the disease already held, so confirming one
+  the registry lacked recorded a judgment with no data behind it. That skews to MONDO and
+  Orphanet because the original ARI import carried almost nothing from either. Hemophilia B
+  Leyden (ARI:0001098) now holds MONDO:0850054 and ORPHA:617930, confirmed by linikujp on
+  21 August and absent ever since. **Writing the id at confirmation time is also an app-side
+  fix and is not in this change.**
+- **Re-applied the reverted cleanups**: 61 ICD-9 codes filed under `ARI_ICD10`, the two
+  `MONDO:`-prefixed values on ARI:0001080 and ARI:0002, and the ranges `I00-I02` and
+  `390-392.99` on Rheumatic fever. All three had landed on 16 August and were overwritten the
+  next day. Cross-references are now derived from the mapping set rather than from either
+  snapshot: a value flagged `predicate_modifier: Not` is dropped, a confirmed one is stored.
+  That reproduces aaronabend's own correction in `1f18f16` — Multiple sclerosis keeps the
+  single OMOP concept 4027727 and SNOMED 24700007 — without special-casing it.
+- **Fixed three mapping-file errors that had been invisible.** `ari.equivalencies.tsv` and
+  `ari.sssom.tsv` disagreed about which OMOP concept is Multiple sclerosis; the equivalencies
+  rows for 374919 and 4027727 were the inverted pair and now match the SSSOM side and the
+  ontology. Three judgments were recorded twice — a curator reviewed a pair, the record was
+  wiped, the pair resurfaced as unreviewed, and a second curator confirmed the same terms
+  again. The first judgment is kept in each case, so linikujp keeps the credit for
+  ARI:0001019 that was taken once already.
+- **`Validate mappings` can now fail on absence.** It ran with `--since BASE_SHA` and reported
+  only rows a branch added or rewrote, so a save that deleted 385 lines passed clean. The new
+  `check_deletions` compares the ontology against the pull request's base and reports
+  `record-deleted`, `xref-deleted` and `disease-deleted`. A cross-reference may still go — that
+  is what flagging one wrong on the review page does, and the judgment is in the mapping set —
+  but a synonym, a subtype, a changelog entry or an id no curator ruled against may not.
+  Removing a malformed value is exempt, so repairing an ICD-9 code or a doubled prefix is not
+  mistaken for a reversal. Replayed against `0f03b91`, the commit that started this: **24
+  errors**, where CI previously reported none.
+- **Restored the weekly audit's sight.** The editor began writing a tenth `comment` column into
+  `ari.sssom.tsv`; the header check rejected it, `split_rows` returned nothing, and every SSSOM
+  row check was skipped in silence — including the two that exist to catch precisely this. Ports
+  the header fix from `fix/sssom-comment-column` (validator only, none of that branch's data),
+  and widens `mapping_date` to accept the ISO 8601 timestamp the app writes: all 548 rows carry
+  one, and a timestamp sorts after the bare date it falls on, which made every row today read as
+  tomorrow.
+- Two things are deliberately left alone. Some diseases now carry the same review recorded more
+  than once under different timestamps, an artifact of the app re-recording a judgment whose
+  record had been wiped; collapsing them is a curator's call and does not belong in a
+  restoration, particularly one that adds a rule saying `ARI_ChangeLog` is append-only. And
+  Chronic Lyme disease (ARI:0001065) now holds four OMOP concepts — two curated on 16 August,
+  two the stale save put back — with no judgment on any of them; **that pair needs a curator.**
+- The six remaining warnings are the standing `dxcode-without-snomed` debt, unchanged.
+
 ## disease-target-mapping-sheet
 
 - Added `data/4-reports/8_Disease_Target_Mappings.xlsx`: one row per (disease, target

mappings/ari.equivalencies.tsv

@@ -507,8 +507,6 @@ ARI	0001028	Autoimmune encephalitis	skos:exactMatch	MONDO	0020640	manual	github:
 ARI	0001019	Antisynthetase syndrome	skos:exactMatch	SNOMEDCT	445187004	manual	github:aaronabend
 ARI	0001019	Antisynthetase syndrome	skos:exactMatch	omop	40482477	manual	github:aaronabend
 ARI	0001019	Antisynthetase syndrome	skos:exactMatch	DOID	0080744	manual	github:aaronabend
-ARI	0001019	Antisynthetase syndrome	skos:exactMatch	MONDO	0019344	manual	github:aaronabend
-ARI	0001019	Antisynthetase syndrome	skos:exactMatch	ORPHA	81	manual	github:aaronabend
 ARI	0001135	Multiple sclerosis	skos:exactMatch	DOID	2377	manual	github:aaronabend
 ARI	0001135	Multiple sclerosis	skos:exactMatch	MONDO	0005301	manual	github:aaronabend
 ARI	0001135	Multiple sclerosis	skos:exactMatch	ncit	C3243	manual	github:aaronabend
@@ -517,19 +515,18 @@ ARI	0001135	Multiple sclerosis	skos:exactMatch	SNOMEDCT	426373005	manual-negativ
 ARI	0001135	Multiple sclerosis	skos:exactMatch	SNOMEDCT	428700003	manual-negative	github:aaronabend
 ARI	0001135	Multiple sclerosis	skos:exactMatch	SNOMEDCT	49692006	manual-negative	github:aaronabend
 ARI	0001135	Multiple sclerosis	skos:exactMatch	SNOMEDCT	24700007	manual	github:aaronabend
-ARI	0001135	Multiple sclerosis	skos:exactMatch	omop	374919	manual	github:aaronabend
+ARI	0001135	Multiple sclerosis	skos:exactMatch	omop	374919	manual-negative	github:aaronabend
 ARI	0001135	Multiple sclerosis	skos:exactMatch	omop	4178929	manual-negative	github:aaronabend
 ARI	0001135	Multiple sclerosis	skos:exactMatch	omop	4145049	manual-negative	github:aaronabend
 ARI	0001135	Multiple sclerosis	skos:exactMatch	omop	376970	manual-negative	github:aaronabend
-ARI	0001135	Multiple sclerosis	skos:exactMatch	omop	4027727	manual-negative	github:aaronabend
+ARI	0001135	Multiple sclerosis	skos:exactMatch	omop	4027727	manual	github:aaronabend
 ARI	0001028	Autoimmune encephalitis	skos:exactMatch	icd10cm	NoTermFound	manual-absent	github:aaronabend
 ARI	0001098	Hemophilia B Leyden	skos:exactMatch	MONDO	0850054	manual	github:linikujp
 ARI	0001098	Hemophilia B Leyden	skos:exactMatch	ORPHA	617930	manual	github:linikujp
 ARI	0001090	Essential mixed cryoglobulinemia	skos:exactMatch	MONDO	0007407	manual	github:linikujp
 ARI	0001090	Essential mixed cryoglobulinemia	skos:exactMatch	ORPHA	91138	manual	github:linikujp
 ARI	0001019	Antisynthetase syndrome	skos:exactMatch	MONDO	0019344	manual	github:linikujp
 ARI	0001019	Antisynthetase syndrome	skos:exactMatch	ORPHA	81	manual	github:linikujp
-ARI	0001056	Birdshot chorioretinopathy	skos:exactMatch	ORPHA	179	manual	github:KrishnaTO
 ARI	0001056	Birdshot chorioretinopathy	skos:exactMatch	omop	4334133	manual	github:KrishnaTO
 ARI	0001002	Acquired hemophilia	skos:exactMatch	MONDO	0019139	manual	github:KrishnaTO
 ARI	0001002	Acquired hemophilia	skos:exactMatch	ncit	C197822	manual	github:KrishnaTO