diff --git a/.github/workflows/curation-history.yaml b/.github/workflows/curation-history.yaml index 46e947ad..73dbf4ad 100644 --- a/.github/workflows/curation-history.yaml +++ b/.github/workflows/curation-history.yaml @@ -1,14 +1,27 @@ # Conventions for this directory: docs/WORKFLOW_CONVENTIONS.md name: Curation history -# Two-part check, deliberately asymmetric (see history/README.md): +# Two-part check, both BLOCKING since #325 (see history/README.md): # -# * VALIDITY is blocking. A malformed history record fails like any other -# validation error. -# * PRESENCE is advisory. If a trait record changes without a matching history -# record, this warns in the job summary and passes. A hard gate on provenance -# blocks legitimate work at inconvenient moments and trains people to route -# around it. +# * VALIDITY. A malformed history record fails like any other validation error, +# including one still carrying the `--details` TODO placeholder -- so +# scaffolding an empty record to satisfy the presence gate does not work. +# * PRESENCE. A PR that changes any .yaml under data/traits/ and adds no new +# history record fails. Spelled that way deliberately: the pathspec is +# `data/traits/*.yaml`, because a git `*` already crosses `/` and the `**/` +# form would MISS a trait added at the top level (#357 review). +# +# Presence was advisory until #325, on the reasoning that a hard gate trains +# people to route around it. The measurement disagreed: of 134 commits that +# modified trait records, 2 added a history record. Nobody routed around the gate +# because there was no gate. What DID happen is that 275 trait records grew an +# issue number hand-typed into a `changes` string -- the same provenance, in a +# form nothing can query. +# +# The gate is only reasonable because #325 also fixed the granularity: ONE record +# per change, not one per changed file. Under the old reading a 128-file migration +# owed 128 near-identical records, and blocking on that would have been a fair +# thing to route around. # # Validation uses the VENDORED schema at src/traitmech/schema/history.yaml, so # this job has no dependency on the private culturebotai-claw repo. Only the @@ -47,6 +60,8 @@ jobs: with: fetch-depth: 0 # need the merge base to diff against + - uses: extractions/setup-just@53165ef7e734c5c07cb06b3c8e7b647c5aa16db3 # v4.0.0 + - name: Install uv uses: astral-sh/setup-uv@c771a70e6277c0a99b617c7a806ffedaca235ff9 # v9.0.0 with: @@ -69,31 +84,32 @@ jobs: --schema src/traitmech/schema/history.yaml \ --target-class HistoryRecord - - name: Check for missing history records (advisory) + - name: Check for missing history records (blocking) if: github.event_name == 'pull_request' run: | set -euo pipefail git fetch --no-tags origin "${{ github.base_ref }}" - base="origin/${{ github.base_ref }}" - changed_traits=$(git diff --name-only "$base"...HEAD -- 'data/traits/**/*.yaml' | wc -l | tr -d ' ') - new_history=$(git diff --name-only --diff-filter=A "$base"...HEAD -- 'history/**/*.yaml' | wc -l | tr -d ' ') + # The RULE lives in scripts/audit_history_records.py, not here. Shell + # embedded in workflow YAML cannot be unit-tested and cannot be run + # locally before pushing, which is how this repo has repeatedly ended up + # with a gate nobody could exercise. The script takes --changed/--added + # so its rule is testable with no repo, network or PR at all. + # + # `|| status=$?` rather than a pipeline: under `set -euo pipefail` a + # failing `just ... | tee` aborts the step immediately, so the summary + # would never be written -- losing the remediation text on exactly the + # runs that need it. + status=0 + just audit-history-records --base "origin/${{ github.base_ref }}" \ + > out.txt 2>&1 || status=$? + cat out.txt { echo "## Curation history" echo - echo "- trait records changed: **$changed_traits**" - echo "- history records added: **$new_history**" - echo - if [ "$changed_traits" -gt 0 ] && [ "$new_history" -eq 0 ]; then - echo "> [!WARNING]" - echo "> This PR changes trait records but adds no history record." - echo "> Scaffold one with \`just new-history\` — see \`history/README.md\`." - echo ">" - echo "> Advisory only; this does not block the build." - elif [ "$changed_traits" -gt 0 ]; then - echo "Provenance recorded for this change." - else - echo "No trait records changed; nothing to record." - fi + echo '```' + cat out.txt + echo '```' } >> "$GITHUB_STEP_SUMMARY" + exit "$status" diff --git a/history/README.md b/history/README.md index 08c341b4..6cd97617 100644 --- a/history/README.md +++ b/history/README.md @@ -1,8 +1,9 @@ # Curation history -Append-only provenance for curation sessions. One record per session per target, -written once and **never edited afterwards**. Corrections go in a new record that -references the old one in its `details`. +Append-only provenance for curation sessions. **One record per change** — per +target for hand curation, per *migration* for a bulk edit (see "One record per +CHANGE" below). Written once and **never edited afterwards**; corrections go in a +new record that references the old one in its `details`. ``` history///--.yaml @@ -73,15 +74,74 @@ wall was so the next session does not rediscover it. `kind`: `record` · `schema` · `mapping` · `report` · `infrastructure` · `other` (`other` requires an explicit `--path`). -## How strictly this is enforced +## One record per CHANGE, not per file + +"One record per session per target" is the rule for **hand curation**, where the +session and the target coincide: someone reasons about one trait and writes down +what they concluded. The three records under `records/` are exactly that, and the +`sulfur_globule` one is what a good record looks like. + +A **bulk change is a different animal** and the same rule read literally gives the +wrong answer. #302 touched 128 trait records mechanically; #334 touched 15. Writing +one record per file would produce 128 near-identical stubs and bury the handful of +substantive records this directory exists for — destroying the signal in the name +of provenance. + +So for a change that edits many records under one decision, write **one** record: -Deliberately split: +| the change is | `kind` | `path` | +|---|---|---| +| one trait, curated | `record` | that trait's YAML | +| a migration driven by a script | `infrastructure` | the migration script | +| a bulk change with no single script | `other` | the file that best explains it | + +Name the scope in `events[].details` — how many records, which issue, and what the +selection rule was. The migration script is usually the honest target: it *is* the +artifact that says what drove the change, and it is reviewable in a way that 128 +copies of the same sentence are not. + +The per-file `curation_history:` block still records what changed in each file. +The two are not redundant: that block has no slot for the model, the tool, or the +issue, and — because it hangs off an edit — **it cannot record a session that +changed nothing.** An `AUDIT` that checked a trait and correctly found nothing +wrong is invisible without a record here. That is what `outcome: no_change` is for. + +## How strictly this is enforced -- **Presence is advisory.** CI warns when a trait record changes without a - matching history record. It does not block. A hard gate on provenance blocks - legitimate work at inconvenient moments and trains people to route around it. -- **Validity is not.** If you write a record it must be schema-valid, and - `just validate-history` fails like any other validation error. +- **Presence blocks** (#325). A PR that changes any `.yaml` under `data/traits/` + and adds no new history record fails CI. + + This was advisory until #325, on the reasoning that a hard gate "trains people to + route around it". The measurement disagreed: of **134 commits** that modified + trait records, **2** added a history record — 1.5%. Nobody routed around the + gate, because there was no gate; the convention simply did not happen. Meanwhile + **275 trait records** carry an issue number hand-typed into a `changes` string, + which is the same information in a form nothing can query. An unenforced + convention here drifts exactly as #182, #184 and #215 drifted. + + The cost is now one file per PR rather than one per changed record, which is what + makes the gate reasonable to impose at all — the granularity fix above had to come + first. + +- **Validity blocks too.** If you write a record it must be schema-valid, and + `just validate-history` fails like any other validation error. It also fails while + the `--details` TODO placeholder is unfilled, so scaffolding an empty record to + satisfy the presence gate does not work. + +## The vendored schema still states the old policy + +`src/traitmech/schema/history.yaml` describes presence as *advisory* and states +"one record per session per target" unqualified. Both are superseded by #325 and +**neither is edited here on purpose**: that file is vendored byte-identical from +claw, which is private and unreachable from this repo's CI, so a one-copy edit +would create drift that nothing detects — `src/traitmech/schema/history.yaml` is +NOT in `scripts/check_vendored_sync.sh`'s checked set, which is the gap #209 +tracks. The canonical copy has to change in claw first and be re-vendored; +tracked in #358. + +Until then this README and the `curation-history` workflow are the operative +statements of the policy, and the schema's prose is stale by design rather than +by neglect. ## Where the schema lives diff --git a/justfile b/justfile index 827d5ffc..8a6b5ff3 100644 --- a/justfile +++ b/justfile @@ -167,6 +167,26 @@ audit-predicate-domains *args: audit-pr-checks *args: uv run python scripts/audit_pr_checks_present.py {{args}} +# Fail a PR that changes trait records and records no provenance (#325). +# +# history/README.md describes a per-session record as the thing that captures +# WHICH MODEL, USING WHICH TOOL, changed what, why, and under which issue. The +# per-file `curation_history:` block has no slot for any of those, and because it +# hangs off an edit it cannot record a session that changed NOTHING -- an AUDIT +# that checked a trait and correctly found nothing wrong is invisible without a +# record here. +# +# Presence was advisory until #325. Of the 134 commits that modified trait +# records, 2 added a history record; meanwhile 275 records grew an issue number +# hand-typed into a `changes` string, which is the same provenance in a form +# nothing can query. +# +# ONE record per CHANGE, not one per changed file -- that granularity fix is what +# makes this reasonable to block on. Needs a base ref to diff against, so it is +# NOT in `qc`; it runs from curation-history.yaml on pull_request. +audit-history-records *args: + uv run python scripts/audit_history_records.py {{args}} + # The stronger companion to audit-pr-checks: not "did ANY check fire" but "did # every check that SHOULD have fired, fire" (#348). # @@ -322,8 +342,10 @@ knowledge-gap-scan *args: (_require-claw "kg_microbe_kgscan") # ============== Curation history (append-only provenance) ============== # Records which model, using which tool, changed what, why, and under which -# issue. One file per session per target under history/; never edited after -# write. See history/README.md. Schema + scaffolder live in claw. +# issue. ONE record per change under history/ -- per target for hand curation, +# per migration for a bulk edit (#325) -- never edited after write. Required: +# a PR that changes data/traits and adds no record fails CI. See +# history/README.md. Schema + scaffolder live in claw. # Scaffold a history record. Prints the path as its last stdout line. # just new-history --kind record --slug cellulolysis \ diff --git a/scripts/audit_history_records.py b/scripts/audit_history_records.py new file mode 100644 index 00000000..fd3fed3d --- /dev/null +++ b/scripts/audit_history_records.py @@ -0,0 +1,136 @@ +#!/usr/bin/env python3 +"""Fail a PR that changes trait records and records no provenance (#325). + +`history/README.md` describes a per-session record as the thing that captures +*which model, using which tool, changed what, why, and under which issue*. The +per-file ``curation_history:`` block captures what changed; it has no slot for +the model, the tool, or the issue, and because it hangs off an edit it cannot +record a session that changed **nothing** -- an ``AUDIT`` that checked a trait +and correctly found nothing wrong is invisible without a record here. + +PRESENCE WAS ADVISORY UNTIL #325, on the reasoning that a hard gate "trains +people to route around it". The measurement disagreed. Of the 134 commits that +modified `data/traits/*.yaml`, **2** added a history record -- 1.5%. Nobody +routed around the gate, because there was no gate; the convention simply did not +happen. What did happen is that **275** trait records grew an issue number +hand-typed into a `changes` string, which is the same provenance in a form +nothing can query. + +THE GATE IS ONLY REASONABLE BECAUSE THE GRANULARITY WAS FIXED FIRST. Read +literally, "one record per session per target" makes a 128-file migration owe 128 +near-identical records, which would bury the three substantive hand-written +records the directory exists for -- destroying the signal in the name of +provenance. Blocking on that would have been a fair thing to route around. One +record per CHANGE costs one file per PR, and that is what this enforces. + +WHAT THIS DELIBERATELY DOES NOT CHECK. That the record is *about* the change. A +record added for an unrelated reason satisfies it. Checking the correspondence +would mean parsing intent, and the cheap proxies (does `target.path` name a +changed file?) are wrong for the migration case, where the honest target is the +script rather than any of the records it edited. The only guard against a +contentless record is `validate-history`, whose schema pattern rejects the +literal `TODO: replace this placeholder` prefix and nothing else -- ``--details +'see PR'`` passes. That is the design, not an oversight: this gate asks whether +provenance was recorded, not whether it was recorded well. Do not rely on it to +judge substance. + +The rule is kept out of the workflow YAML so it is testable without a repo, a +network, or a PR; ``main`` shells out to git and hands the file lists in. + +Usage: + just audit-history-records --base origin/main + python scripts/audit_history_records.py --changed a.yaml --added b.yaml +""" +from __future__ import annotations + +import argparse +import subprocess +import sys + +# NO `**/`. A git pathspec is not a shell glob: git's `*` already crosses `/`, +# so `data/traits/**/*.yaml` still has to consume the literal slash in `**/` and +# therefore requires AT LEAST ONE intervening directory. It misses a trait added +# directly under data/traits/. The workflow's own trigger is +# `paths: data/traits/**`, which is GitHub Actions semantics and DOES match that +# file -- so the job would start and then clear the gate reporting "0 trait +# records changed" (#357 review). `data/traits/*.yaml` is strictly more +# inclusive and matches the same 477 files today. +TRAIT_GLOB = "data/traits/*.yaml" +HISTORY_GLOB = "history/*.yaml" + + +def missing_record(changed_traits: list[str], added_history: list[str]) -> bool: + """True when trait records changed and no history record was added. + + ``added_history`` must be files ADDED, not merely modified: editing an + existing record is explicitly not how corrections work here -- the README + says records are written once and never edited, and a correction goes in a + NEW record that references the old one. Counting modifications would accept + exactly the thing the append-only design forbids. + """ + return bool(changed_traits) and not added_history + + +def _git(args: list[str]) -> list[str]: + proc = subprocess.run(["git", *args], capture_output=True, text=True) + if proc.returncode != 0: + print(f"git {' '.join(args)} failed: {proc.stderr.strip()}", file=sys.stderr) + raise SystemExit(2) + return [ln for ln in proc.stdout.splitlines() if ln.strip()] + + +def collect(base: str) -> tuple[list[str], list[str]]: + """(changed trait records, added history records) for base...HEAD.""" + changed = _git(["diff", "--name-only", f"{base}...HEAD", "--", TRAIT_GLOB]) + added = _git(["diff", "--name-only", "--diff-filter=A", f"{base}...HEAD", + "--", HISTORY_GLOB]) + return changed, added + + +def main() -> int: + ap = argparse.ArgumentParser(description=__doc__) + ap.add_argument("--base", help="base ref to diff against, e.g. origin/main") + ap.add_argument("--changed", nargs="*", default=None, + help="changed trait records, for testing offline") + ap.add_argument("--added", nargs="*", default=None, + help="added history records, for testing offline") + args = ap.parse_args() + + if args.changed is not None or args.added is not None: + changed, added = list(args.changed or []), list(args.added or []) + elif args.base: + changed, added = collect(args.base) + else: + ap.error("pass --base, or --changed/--added") + + print("=== curation history ===", file=sys.stderr) + print(f" trait records changed: {len(changed)}", file=sys.stderr) + print(f" history records added: {len(added)}", file=sys.stderr) + for f in added: + print(f" + {f}", file=sys.stderr) + + if missing_record(changed, added): + print( + "\nThis PR changes trait records and adds no history record (#325).\n" + "Write ONE record for the whole change, not one per file. For a\n" + "migration the honest target is the script that drove it:\n\n" + " just new-history --kind infrastructure \\\n" + " --path scripts/.py \\\n" + " --event EDIT --outcome changed \\\n" + " --summary '' \\\n" + " --model claude-opus-5 --agent-tool claude-code \\\n" + " --issue https://github.com/CultureBotAI/TraitMech/issues/ \\\n" + " --details ''\n\n" + "See history/README.md. The per-file curation_history: block does not\n" + "cover this: it has no slot for the model, the tool, or the issue.", + file=sys.stderr) + return 1 + if changed: + print(" provenance recorded for this change", file=sys.stderr) + else: + print(" no trait records changed; nothing to record", file=sys.stderr) + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/tests/test_audit_history_records.py b/tests/test_audit_history_records.py new file mode 100644 index 00000000..9a9ff87e --- /dev/null +++ b/tests/test_audit_history_records.py @@ -0,0 +1,145 @@ +"""Unit tests for scripts/audit_history_records.py (#325). + +The rule is three lines, so what these pin is mostly the edges that would make it +silently permissive: an empty history list, a MODIFIED record standing in for an +added one, and a no-trait-change PR being clean rather than blocked. +""" + +from __future__ import annotations + +import subprocess +import sys +from pathlib import Path + +REPO_ROOT = Path(__file__).resolve().parent.parent +sys.path.insert(0, str(REPO_ROOT / "scripts")) + +from audit_history_records import missing_record # noqa: E402 + +TRAIT = "data/traits/morphology/motile.yaml" +RECORD = "history/records/motile/2026-08-07T000000Z-claude-code-abc123.yaml" + + +def test_trait_change_without_a_record_is_blocked(): + assert missing_record([TRAIT], []) is True + + +def test_one_record_covers_many_changed_traits(): + """The granularity fix, and the reason the gate is impose-able at all. + + Under the literal 'one record per session per target' reading, a 128-file + migration owed 128 records. This asserts the opposite: ONE is enough for a + whole migration, so the cost is one file per PR rather than one per record. + """ + many = [f"data/traits/morphology/t{i}.yaml" for i in range(128)] + assert missing_record(many, [RECORD]) is False + + +def test_no_trait_change_is_clean_not_blocked(): + """A PR touching only history/, the schema or the workflow owes nothing.""" + assert missing_record([], []) is False + assert missing_record([], [RECORD]) is False + + +def test_a_modified_record_does_not_count_as_an_added_one(): + """Records are append-only: written once, never edited, corrections go in a + NEW record referencing the old one. `collect` passes only --diff-filter=A + files, so an edit to an existing record reaches this as an empty list -- + which must still block, or the gate accepts the one thing the append-only + design forbids.""" + assert missing_record([TRAIT], []) is True + + +def test_the_scripts_own_cli_agrees_with_the_rule(): + """End-to-end through argparse, since the workflow calls the CLI and not the + function -- a wiring error there would leave the rule correct and unused.""" + def run(changed, added): + return subprocess.run( + [sys.executable, str(REPO_ROOT / "scripts" / "audit_history_records.py"), + "--changed", *changed, "--added", *added], + capture_output=True, text=True) + + bad = run([TRAIT], []) + assert bad.returncode == 1 + assert "adds no history record" in bad.stderr + # The remediation must name the ONE-record rule, since the whole reason this + # is blocking is that the per-file reading was unreasonable. + assert "not one per file" in bad.stderr + + good = run([TRAIT], [RECORD]) + assert good.returncode == 0 + + nothing = run([], []) + assert nothing.returncode == 0 + + +# --- #357 review: the pathspec itself, which missing_record() cannot see ------- + + +def _git(cwd, *args): + subprocess.run(["git", *args], cwd=cwd, check=True, capture_output=True) + + +def _repo(tmp_path: Path) -> Path: + _git(tmp_path, "init", "-q", "-b", "main") + _git(tmp_path, "config", "user.email", "t@t") + _git(tmp_path, "config", "user.name", "t") + (tmp_path / "README").write_text("base\n") + _git(tmp_path, "add", "-A") + _git(tmp_path, "commit", "-qm", "base") + return tmp_path + + +def test_collect_sees_a_trait_directly_under_data_traits(tmp_path): + """A git pathspec is NOT a shell glob. + + git's `*` already crosses `/`, so `data/traits/**/*.yaml` must still consume + the literal slash in `**/` and therefore needs at least one intervening + directory -- it misses `data/traits/x.yaml`. The workflow's trigger is + `paths: data/traits/**`, which is GitHub Actions semantics and DOES match + that file, so the job would start and then clear the gate at "0 changed". + Silently permissive, and invisible to missing_record(). + """ + repo = _repo(tmp_path) + for rel in ["data/traits/toplevel.yaml", "data/traits/morphology/nested.yaml"]: + p = repo / rel + p.parent.mkdir(parents=True, exist_ok=True) + p.write_text("identifier: x\n") + _git(repo, "add", "-A") + _git(repo, "commit", "-qm", "traits") + + import audit_history_records as m + cwd = Path.cwd() + try: + import os + os.chdir(repo) + changed, added = m.collect("main~1") + finally: + os.chdir(cwd) + assert sorted(changed) == ["data/traits/morphology/nested.yaml", + "data/traits/toplevel.yaml"] + + +def test_collect_counts_a_history_record_at_any_depth(tmp_path): + """Same bug on the other side: a record not nested under a slug dir would + not have counted toward presence.""" + repo = _repo(tmp_path) + for rel in ["history/flat.yaml", "history/records/slug/deep.yaml"]: + p = repo / rel + p.parent.mkdir(parents=True, exist_ok=True) + p.write_text("history_version: 1\n") + (repo / "data/traits").mkdir(parents=True) + (repo / "data/traits/t.yaml").write_text("identifier: x\n") + _git(repo, "add", "-A") + _git(repo, "commit", "-qm", "records") + + import audit_history_records as m + import os + cwd = Path.cwd() + try: + os.chdir(repo) + changed, added = m.collect("main~1") + finally: + os.chdir(cwd) + assert sorted(added) == ["history/flat.yaml", "history/records/slug/deep.yaml"] + assert m.missing_record(changed, added) is False