From 5a70b290e033ff2127edb22b51489f5700f61519 Mon Sep 17 00:00:00 2001
From: "marcin p. joachimiak" <4625870+realmarcin@users.noreply.github.com>
Date: Mon, 10 Aug 2026 19:37:35 -0700
Subject: [PATCH] Lift the text-append curation helper out of gtdb_ground
 (#526)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

`_append_curation_event` was the only code here that adds a CurationEvent
without a YAML round-trip, and it lived inside one script. The nine writers #325
lists as owing a trace are line editors — splitlines, regex, write_text — so
`record_curation_event`, which appends to a parsed dict, does not reach them.
Nine private copies would be nine chances to re-hit what this one already knows:
that `- timestamp:` is column-0 so a naive scan inserts the event above the
existing history, that `curation_history: []` is the same key and matching the
bare string appends a second one PyYAML silently drops, and that a trailing
comment block belongs to the next key.

Now `communitymech.curate.curation_event.append_curation_event_text`, with
curator and width as parameters rather than hardcoded. gtdb_ground keeps a thin
wrapper so its curator string stays in one place and the existing tests keep
exercising the shared code through its original caller.

One behaviour change on purpose: the library raises ValueError where the script
raised SystemExit. SystemExit does not inherit from Exception, so a caller with
`except Exception` would not catch it — it would take the process down
mid-sweep, which is exactly what a shared helper must not do. gtdb_ground
converts at its own boundary, so its CLI message is unchanged.

drop_obsolete_go_bp.py is wired as the first user, and comes off the owed list.
Deletions are where a trace matters most: the removed lines are simply not there
afterwards.

The canary needed building rather than running. The corpus has no droppable
annotations left, so a dry run reports zero and proves nothing — the shape of
check that passes while persisting nothing. The test constructs a record that
does have one, runs the script in a temp tree the way a batch would, and reads
the file back off disk. Reverting the wiring reddens it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
---
 scripts/drop_obsolete_go_bp.py             |  25 ++-
 scripts/gtdb_ground.py                     | 111 ++++---------
 src/communitymech/curate/curation_event.py |  97 ++++++++++-
 tests/test_append_curation_event_text.py   | 184 +++++++++++++++++++++
 tests/test_writers_leave_a_trace.py        |  11 +-
 5 files changed, 342 insertions(+), 86 deletions(-)
 create mode 100644 tests/test_append_curation_event_text.py

diff --git a/scripts/drop_obsolete_go_bp.py b/scripts/drop_obsolete_go_bp.py
index 11b953302..1b252fbce 100644
--- a/scripts/drop_obsolete_go_bp.py
+++ b/scripts/drop_obsolete_go_bp.py
@@ -22,6 +22,13 @@
 import sys
 from pathlib import Path
 
+# `python scripts/foo.py` does not put `src/` on the path.
+sys.path.insert(0, str(Path(__file__).resolve().parents[1] / "src"))
+
+from communitymech.curate.curation_event import (  # noqa: E402
+    append_curation_event_text,
+)
+
 DRY = "--dry-run" in sys.argv
 COMMUNITIES = Path("kb/communities")
 
@@ -69,7 +76,23 @@ def drop_file(path: Path) -> int:
         j += 1
 
     if not DRY:
-        path.write_text("\n".join(cleaned) + "\n")
+        # Leave a trace. These edits delete curated annotations, and a deletion
+        # is the case where "what did this and why" is least recoverable from
+        # the record itself — the removed lines are simply not there any more
+        # (#325). Text append rather than a YAML round-trip because this whole
+        # module is a line editor; the shared helper owns the insertion rules
+        # (#526).
+        text = append_curation_event_text(
+            "\n".join(cleaned) + "\n",
+            curator="drop_obsolete_go_bp.py",
+            action="DROP_OBSOLETE_GO_BP",
+            changes=(
+                f"Dropped {removed} generic obsolete-GO biological_process "
+                f"annotation(s) that the id-label cleanup had remapped to "
+                f"high-level parents carrying no mechanistic information (#182)."
+            ),
+        )
+        path.write_text(text)
     return removed
 
 
diff --git a/scripts/gtdb_ground.py b/scripts/gtdb_ground.py
index 92ae3f8a9..fa2f646a6 100644
--- a/scripts/gtdb_ground.py
+++ b/scripts/gtdb_ground.py
@@ -70,7 +70,12 @@
 # `scripts/` is on sys.path when this runs as `python scripts/gtdb_ground.py`,
 # so reach the package the same way the other scripts do.
 sys.path.insert(0, str(Path(__file__).resolve().parents[1] / "src"))
-from communitymech.curate.curation_event import record_curation_event  # noqa: E402
+# Two single-line imports rather than a parenthesised one: `# noqa: E402` binds
+# to the first physical line, so the multi-line form left the rule unsuppressed.
+from communitymech.curate.curation_event import (  # noqa: E402
+    append_curation_event_text,
+    record_curation_event,
+)
 from communitymech.validators.ncbi_domain import outside_gtdb_scope  # noqa: E402
 
 # NCBI2GTDB.tsv column indices (0-based); see header row.
@@ -1550,85 +1555,35 @@ def _record_edit(original: str, new_text: str, *, action: str, changes: str) ->
 
 
 def _append_curation_event(text: str, *, action: str, changes: str) -> str:
-    """Append one CurationEvent to a record's `curation_history`, as text.
-
-    Text rather than a YAML round-trip because every write path here is a
-    line-level editor: re-dumping the document would reformat all of it, and
-    `_assert_only_grounding_changed` exists precisely because four hand-rolled
-    attempts at whole-file edits corrupted records (#378).
-
-    Two cases. When `curation_history` is absent — 310 of the 312 records, since
-    #325 is still open — the key is appended at the end of the document. When it
-    is present, the event goes at the end of its block, found by scanning to the
-    next top-level key. Appending a second `curation_history:` instead would be
-    silently lossy, since PyYAML keeps only the last of two identical keys; the
-    duplicate-key detector in the write guard would catch it, but producing it
-    and relying on the guard is the wrong order.
+    """Thin wrapper over the shared appender (#526).
+
+    The implementation moved to `communitymech.curate.curation_event` so the
+    nine other line-editing writers in #325 can use it instead of each growing
+    their own. Everything it knows how to get wrong was learned here: that
+    `- timestamp:` is itself column-0 so a naive scan inserts the event ABOVE
+    the existing history, that `curation_history: []` is the same key as
+    `curation_history:` and matching only the bare string appends a second one
+    PyYAML then silently drops, and that a trailing comment block belongs to
+    the *next* key rather than to the history.
+
+    Kept as a wrapper rather than replaced at the call site so this module's
+    curator string stays in one place, and so `tests/test_gtdb_curation_history.py`
+    keeps exercising the shared code through its original caller.
     """
-    # Built by the shared helper rather than by hand: it owns the field names
-    # and the timestamp format, and `audit_writers.py` cannot tell a hand-rolled
-    # dict from the real thing — its check is a regex for the literal
-    # `'curator':`, so a drifting copy would keep reporting `yes` (review of
-    # #483). Only the *insertion* has to be bespoke here, because every write
-    # path in this module is a line-level editor.
-    holder: dict = {}
-    record_curation_event(holder, curator="gtdb_ground.py", action=action, changes=changes)
-    event = holder["curation_history"][0]
-    dumped = yaml.dump(
-        [event], sort_keys=False, allow_unicode=True, width=DUMP_WIDTH, default_flow_style=False
-    ).rstrip("\n")
-
-    lines = text.rstrip("\n").split("\n")
-    # A document end marker would put the appended key outside the document.
-    if any(ln.rstrip() in ("...", "---") for ln in lines[1:]):
-        raise SystemExit(
-            "record uses explicit YAML document markers; curation_history cannot "
-            "be appended safely by line edit (#395)."
+    try:
+        return append_curation_event_text(
+            text,
+            curator="gtdb_ground.py",
+            action=action,
+            changes=changes,
+            width=DUMP_WIDTH,
         )
-    for i, line in enumerate(lines):
-        # `curation_history:`, `curation_history: []`, `curation_history:  # note`
-        # are the same key. Matching the bare string alone appended a SECOND
-        # `curation_history:` for the other two, which PyYAML resolves by
-        # keeping only the last — silently dropping the existing history. The
-        # write guard caught it, but producing corruption and relying on the
-        # guard is the wrong order (review of #483).
-        head = re.match(r"^curation_history:\s*(.*)$", line)
-        if not head:
-            continue
-        rest = head.group(1).strip()
-        if rest and not rest.startswith("#"):
-            # An inline value: `curation_history: []`. Replace it with a block,
-            # since an event cannot be appended to a flow sequence by line edit.
-            if rest not in ("[]", "~", "null"):
-                raise SystemExit(
-                    f"record has an inline curation_history value ({rest!r}) that "
-                    f"is not an empty list; refusing to edit it (#395)."
-                )
-            lines[i] = "curation_history:"
-        end = len(lines)
-        for j in range(i + 1, len(lines)):
-            # Only a new top-level KEY ends the block. `- timestamp: ...` also
-            # starts at column 0 — testing `not line[0].isspace()` treated the
-            # list's own first item as the next section and inserted the event
-            # ABOVE the existing history, which the append-only write guard then
-            # correctly refused. Match the same `^[A-Za-z_]` rule the rest of
-            # this module uses to find section ends.
-            if re.match(r"^[A-Za-z_]", lines[j]):
-                end = j
-                break
-        # Back off over a comment block introducing that next key, so the event
-        # is not inserted below it — which would silently re-attach the comment
-        # to curation_history.
-        while end > i + 1 and lines[end - 1].lstrip().startswith("#"):
-            end -= 1
-        while end > i + 1 and not lines[end - 1].strip():
-            end -= 1
-        # An indented sequence (`  - action: ...`) cannot take a column-0 item.
-        items = [ln for ln in lines[i + 1 : end] if ln.strip().startswith("- ")]
-        indent = " " * (len(items[0]) - len(items[0].lstrip())) if items else ""
-        body = [f"{indent}{ln}" if ln.strip() else ln for ln in dumped.split("\n")]
-        return "\n".join(lines[:end] + body + lines[end:]) + "\n"
-    return "\n".join(lines + ["curation_history:"] + dumped.split("\n")) + "\n"
+    except ValueError as refusal:
+        # The library raises; this script exits. Deliberate: a helper that
+        # `SystemExit`s cannot be used by anything with its own error handling,
+        # which is the whole point of lifting it (#526). The refusal-to-corrupt
+        # behaviour and its message are unchanged for this caller.
+        raise SystemExit(str(refusal)) from refusal
 
 
 def _assert_only_grounding_changed(
diff --git a/src/communitymech/curate/curation_event.py b/src/communitymech/curate/curation_event.py
index de4ee5882..864e9d7b0 100644
--- a/src/communitymech/curate/curation_event.py
+++ b/src/communitymech/curate/curation_event.py
@@ -32,9 +32,12 @@
 from __future__ import annotations
 
 import datetime
+import re
 from typing import Any
 
-__all__ = ["record_curation_event", "now_iso"]
+import yaml
+
+__all__ = ["record_curation_event", "append_curation_event_text", "now_iso"]
 
 
 def now_iso() -> str:
@@ -112,3 +115,95 @@ def record_curation_event(
 
     history.append(event)
     return event
+
+
+def append_curation_event_text(
+    text: str,
+    *,
+    curator: str,
+    action: str,
+    changes: str,
+    width: int = 100,
+    timestamp: str | None = None,
+) -> str:
+    """Append one CurationEvent to a record's `curation_history`, as text.
+
+    Text rather than a YAML round-trip because every write path here is a
+    line-level editor: re-dumping the document would reformat all of it, and
+    `_assert_only_grounding_changed` exists precisely because four hand-rolled
+    attempts at whole-file edits corrupted records (#378).
+
+    Two cases. When `curation_history` is absent — 310 of the 312 records, since
+    #325 is still open — the key is appended at the end of the document. When it
+    is present, the event goes at the end of its block, found by scanning to the
+    next top-level key. Appending a second `curation_history:` instead would be
+    silently lossy, since PyYAML keeps only the last of two identical keys; the
+    duplicate-key detector in the write guard would catch it, but producing it
+    and relying on the guard is the wrong order.
+    """
+    # Built by the shared helper rather than by hand: it owns the field names
+    # and the timestamp format, and `audit_writers.py` cannot tell a hand-rolled
+    # dict from the real thing — its check is a regex for the literal
+    # `'curator':`, so a drifting copy would keep reporting `yes` (review of
+    # #483). Only the *insertion* has to be bespoke here, because every write
+    # path in this module is a line-level editor.
+    holder: dict = {}
+    record_curation_event(
+        holder, curator=curator, action=action, changes=changes, timestamp=timestamp
+    )
+    event = holder["curation_history"][0]
+    dumped = yaml.dump(
+        [event], sort_keys=False, allow_unicode=True, width=width, default_flow_style=False
+    ).rstrip("\n")
+
+    lines = text.rstrip("\n").split("\n")
+    # A document end marker would put the appended key outside the document.
+    if any(ln.rstrip() in ("...", "---") for ln in lines[1:]):
+        raise ValueError(
+            "record uses explicit YAML document markers; curation_history cannot "
+            "be appended safely by line edit (#395)."
+        )
+    for i, line in enumerate(lines):
+        # `curation_history:`, `curation_history: []`, `curation_history:  # note`
+        # are the same key. Matching the bare string alone appended a SECOND
+        # `curation_history:` for the other two, which PyYAML resolves by
+        # keeping only the last — silently dropping the existing history. The
+        # write guard caught it, but producing corruption and relying on the
+        # guard is the wrong order (review of #483).
+        head = re.match(r"^curation_history:\s*(.*)$", line)
+        if not head:
+            continue
+        rest = head.group(1).strip()
+        if rest and not rest.startswith("#"):
+            # An inline value: `curation_history: []`. Replace it with a block,
+            # since an event cannot be appended to a flow sequence by line edit.
+            if rest not in ("[]", "~", "null"):
+                raise ValueError(
+                    f"record has an inline curation_history value ({rest!r}) that "
+                    f"is not an empty list; refusing to edit it (#395)."
+                )
+            lines[i] = "curation_history:"
+        end = len(lines)
+        for j in range(i + 1, len(lines)):
+            # Only a new top-level KEY ends the block. `- timestamp: ...` also
+            # starts at column 0 — testing `not line[0].isspace()` treated the
+            # list's own first item as the next section and inserted the event
+            # ABOVE the existing history, which the append-only write guard then
+            # correctly refused. Match the same `^[A-Za-z_]` rule the rest of
+            # this module uses to find section ends.
+            if re.match(r"^[A-Za-z_]", lines[j]):
+                end = j
+                break
+        # Back off over a comment block introducing that next key, so the event
+        # is not inserted below it — which would silently re-attach the comment
+        # to curation_history.
+        while end > i + 1 and lines[end - 1].lstrip().startswith("#"):
+            end -= 1
+        while end > i + 1 and not lines[end - 1].strip():
+            end -= 1
+        # An indented sequence (`  - action: ...`) cannot take a column-0 item.
+        items = [ln for ln in lines[i + 1 : end] if ln.strip().startswith("- ")]
+        indent = " " * (len(items[0]) - len(items[0].lstrip())) if items else ""
+        body = [f"{indent}{ln}" if ln.strip() else ln for ln in dumped.split("\n")]
+        return "\n".join(lines[:end] + body + lines[end:]) + "\n"
+    return "\n".join(lines + ["curation_history:"] + dumped.split("\n")) + "\n"
diff --git a/tests/test_append_curation_event_text.py b/tests/test_append_curation_event_text.py
new file mode 100644
index 000000000..794707353
--- /dev/null
+++ b/tests/test_append_curation_event_text.py
@@ -0,0 +1,184 @@
+"""The text-append curation helper, outside its original caller (#526).
+
+`_append_curation_event` was written inside `scripts/gtdb_ground.py` for #395 and
+is the only code in the repo that can add a `CurationEvent` to a record **without
+a YAML round-trip**. That matters because the nine writers #325 lists as owing a
+trace are line editors — `splitlines()`, regex, `write_text` — deliberately, since
+re-dumping would reflow every record they touch.
+
+Nine copies of it would be nine chances to re-hit what it already knows:
+
+* `- timestamp:` starts at column 0, so a naive "next top-level key" scan treats
+  the history's own first item as the next section and inserts the event **above**
+  the existing history;
+* `curation_history: []` is the same key as `curation_history:`, and matching only
+  the bare string appends a *second* one, which PyYAML resolves by keeping the
+  last — silently dropping the existing history;
+* a comment block before the next key belongs to that key, not to the history.
+
+So it moved to `communitymech.curate.curation_event`. These tests exercise it
+**as a library**, with a curator that is not `gtdb_ground.py` — the case the
+existing `tests/test_gtdb_curation_history.py` cannot reach, since it only ever
+calls through the original caller.
+
+One deliberate behaviour change: the library raises `ValueError` where the script
+raised `SystemExit`. A helper that exits the process cannot be used by anything
+with its own error handling, which is the point of lifting it. `gtdb_ground.py`
+converts at its own boundary, so its CLI behaviour and message are unchanged.
+"""
+
+from __future__ import annotations
+
+import pathlib
+import subprocess
+import sys
+
+import pytest
+import yaml
+
+from communitymech.curate.curation_event import append_curation_event_text
+
+REPO = pathlib.Path(__file__).parent.parent
+
+_MINIMAL = """id: CommunityMech:000999
+name: helper test record
+description: a record with no curation history
+"""
+
+_WITH_HISTORY = """id: CommunityMech:000999
+name: helper test record
+curation_history:
+- timestamp: '2026-01-01T00:00:00Z'
+  curator: someone
+  action: FIRST
+  changes: the pre-existing event
+description: a key after the history
+"""
+
+
+def _appended(text: str, **kwargs) -> list[dict]:
+    result = append_curation_event_text(
+        text, curator="a_test", action="TEST_ACTION", changes="what changed", **kwargs
+    )
+    return yaml.safe_load(result)["curation_history"]
+
+
+def test_it_creates_the_block_when_there_is_none():
+    events = _appended(_MINIMAL)
+    assert len(events) == 1
+    assert events[0]["curator"] == "a_test"
+    assert events[0]["action"] == "TEST_ACTION"
+
+
+def test_the_rest_of_the_document_is_untouched():
+    """A line editor's whole reason for existing."""
+    result = append_curation_event_text(
+        _MINIMAL, curator="a_test", action="TEST_ACTION", changes="what changed"
+    )
+    assert result.startswith(_MINIMAL.rstrip("\n"))
+
+
+def test_it_appends_after_an_existing_event_not_before_it():
+    """The column-0 trap: `- timestamp:` is not the next top-level key."""
+    events = _appended(_WITH_HISTORY)
+    assert [event["action"] for event in events] == ["FIRST", "TEST_ACTION"], (
+        "the new event was inserted above the existing history, which the "
+        "append-only write guard in gtdb_ground correctly refuses (#395)"
+    )
+
+
+def test_a_key_after_the_history_survives():
+    result = append_curation_event_text(
+        _WITH_HISTORY, curator="a_test", action="TEST_ACTION", changes="what changed"
+    )
+    document = yaml.safe_load(result)
+    assert document["description"] == "a key after the history"
+
+
+def test_an_empty_inline_list_becomes_a_block_rather_than_a_second_key():
+    """`curation_history: []` is the same key; appending a second one is lossy."""
+    text = "id: CommunityMech:000999\ncuration_history: []\nname: x\n"
+    result = append_curation_event_text(
+        text, curator="a_test", action="TEST_ACTION", changes="what changed"
+    )
+    assert result.count("curation_history:") == 1
+    assert len(yaml.safe_load(result)["curation_history"]) == 1
+
+
+def test_a_non_empty_inline_value_is_refused_rather_than_mangled():
+    text = "id: CommunityMech:000999\ncuration_history: [{a: 1}]\nname: x\n"
+    with pytest.raises(ValueError, match="inline curation_history"):
+        append_curation_event_text(
+            text, curator="a_test", action="TEST_ACTION", changes="what changed"
+        )
+
+
+def test_document_markers_are_refused():
+    """Appending past a `...` would put the key outside the document."""
+    text = _MINIMAL + "...\n"
+    with pytest.raises(ValueError, match="document markers"):
+        append_curation_event_text(
+            text, curator="a_test", action="TEST_ACTION", changes="what changed"
+        )
+
+
+def test_it_raises_rather_than_exiting():
+    """The reason for the lift, asserted directly.
+
+    `SystemExit` does not inherit from `Exception`, so a caller with
+    `except Exception` would not catch the old behaviour — it would take the
+    process down mid-sweep.
+    """
+    text = "id: x\ncuration_history: [{a: 1}]\n"
+    with pytest.raises(ValueError):
+        append_curation_event_text(text, curator="c", action="A", changes="c")
+
+
+def test_the_curator_is_not_hardcoded_to_gtdb_ground():
+    """It was, in the original. Everything above would pass if it still were."""
+    events = _appended(_MINIMAL)
+    assert events[0]["curator"] == "a_test"
+
+
+def test_a_wired_line_editor_actually_writes_the_event_to_disk(tmp_path):
+    """End-to-end through `drop_obsolete_go_bp.py`, the first writer wired (#325).
+
+    The corpus has no droppable annotations left, so a dry run against it
+    reports zero and proves nothing — exactly the shape of canary that passes
+    while persisting nothing. This builds a record that *does* have one, runs
+    the script in a temporary tree the way the batch would, and reads the file
+    back off disk.
+    """
+    communities = tmp_path / "kb/communities"
+    communities.mkdir(parents=True)
+    (communities / "r.yaml").write_text(
+        "id: CommunityMech:000999\n"
+        "name: droppable\n"
+        "taxonomy:\n"
+        "- taxon_term:\n"
+        "    preferred_term: Escherichia coli\n"
+        "  biological_processes:\n"
+        "  - preferred_term: oxidation-reduction process\n"
+        "    term:\n"
+        "      id: GO:0016491\n"
+        "      label: oxidoreductase activity\n",
+        encoding="utf-8",
+    )
+    result = subprocess.run(
+        [sys.executable, str(REPO / "scripts/drop_obsolete_go_bp.py")],
+        capture_output=True,
+        text=True,
+        cwd=tmp_path,
+        timeout=600,
+    )
+    assert result.returncode == 0, result.stderr[-500:]
+
+    document = yaml.safe_load((communities / "r.yaml").read_text(encoding="utf-8"))
+    history = document.get("curation_history") or []
+    assert len(history) == 1, (
+        f"the script edited the record but left no trace on disk: {result.stdout!r}. "
+        f"A write that reports success and persists nothing is the failure this "
+        f"repo keeps finding (#325)."
+    )
+    assert history[0]["curator"] == "drop_obsolete_go_bp.py"
+    assert history[0]["action"] == "DROP_OBSOLETE_GO_BP"
diff --git a/tests/test_writers_leave_a_trace.py b/tests/test_writers_leave_a_trace.py
index 8943385e4..a899a22b9 100644
--- a/tests/test_writers_leave_a_trace.py
+++ b/tests/test_writers_leave_a_trace.py
@@ -71,7 +71,6 @@
     "scripts/apply_taxonomy_corrections.py",
     "scripts/backfill_metals.py",
     "scripts/chebi_fix_apply.py",
-    "scripts/drop_obsolete_go_bp.py",
     "scripts/fix_reference_formats.py",
     "scripts/suggest_related_media.py",
     "scripts/term_fix_apply.py",
@@ -144,12 +143,12 @@ def test_the_exemptions_have_reasons():
 def test_the_owed_backlog_has_not_grown(audit):
     """#325's real number, bounded.
 
-    16 of 26 writers appended nothing when this was measured — 9 owed, 5 exempt,
-    and 2 that the audit counts but which are covered elsewhere. The point of a
-    bound is that adding a tenth owed writer is a decision someone makes on
-    purpose, not a drift.
+    16 of 26 writers appended nothing when this was measured: 9 owed and 7
+    exempt. `drop_obsolete_go_bp.py` was wired in #526, leaving 8. The point of
+    a bound is that adding a ninth back is a decision someone makes on purpose,
+    not a drift.
     """
-    assert len(_OWED) <= 9, (
+    assert len(_OWED) <= 8, (
         f"{len(_OWED)} writers now owe a curation trace, up from 9. Adding one "
         f"is a choice worth defending — the alternative is calling "
         f"`record_curation_event` in the new tool (#325)."