Make a history record required, once it is one record per change (#325)

[realmarcin]

Closes #325. Decision: enforce it — after fixing the granularity that made enforcing it unreasonable.

The issue asked two things: whether a bulk migration warrants a record at all, and whether it should be enforced. Answering the second first was the mistake available here — as written, the convention could not be enforced without doing damage.

Why not drop it

I had leaned toward folding everything into the per-file curation_history: block as duplication. That was wrong, and the README's own justification (merge-conflict avoidance) isn't what distinguishes the two — curation_history is per-file, so it doesn't conflict either.

What distinguishes it: HistoryOutcomeEnum has no_change. A session that audited a trait and correctly concluded nothing needed changing produces no file edit, so curation_history has nothing to attach to and the work is invisible. Same for blocked and needs_followup. Add model, agent_tool, agent_version and structured issue/PR links — none of which CurationEvent has a slot for — and it's a real capability, not a second copy.

Why the granularity had to change first

"One record per session per target" is right for hand curation, where session and target coincide; the sulfur_globule record is a paragraph of real reasoning about one graph. Read literally it also makes a 128-file migration owe 128 near-identical stubs, burying the three substantive records the directory exists for. That's destroying the signal in the name of provenance, and blocking on it would have been a fair thing to route around.

So: one record per change, targeting the migration script — which is the artifact that says what drove it, and is reviewable in a way that 128 copies of one sentence are not. No schema change needed; infrastructure and other kinds already exist.

Why blocking

The old comment said a hard gate "trains people to route around it." The measurement disagrees:

commits that modified data/traits/*.yaml	134
commits that added a history record	2
trait records with an issue number hand-typed into a changes string	275

Nobody routed around the gate, because there was no gate. And the 275 are the tell: that's the same provenance in a form nothing can query. My own #334 commits did it, which is how I know it's substitution rather than forgetting.

The rule is a script, not workflow YAML

scripts/audit_history_records.py, with 5 tests including one through the CLI — the workflow calls the CLI, so a wiring error would leave the rule correct and unused. Shell embedded in a workflow can't be unit-tested or run locally before pushing, which is how this repo keeps ending up with gates nobody can exercise.

Two bugs found while wiring it, both mine:

• set -euo pipefail plus a just … | tee pipeline aborts the step the moment the audit fails, so the job summary would never be written — losing the remediation text on exactly the runs that need it. Now || status=$?.
• the job never installed just. Added setup-just, the same pinned action the sibling pr-checks-present.yaml uses.

🐤 Canary — against real diffs, not fixtures

	diff	result
A	fix/334-enables-range vs main — 15 traits, 0 records	exit 1 ✅
B	this branch vs main — 0 traits, 0 records	exit 0 ✅
C	A plus one scaffolded record — 15 traits, 1 record	exit 0 ✅

C used just new-history for real (exercising the no-claw fallback) and the record passed just validate-history. That record is now on PR #355, so main doesn't get a grandfathered gap on the day the gate lands.

Deliberately not checked

That the record is about the change. The cheap proxy — does target.path name a changed file? — is wrong for exactly the migration case, where the honest target is the script rather than any record it edited. The guard against an empty record is validate-history, which fails while the --details TODO placeholder is unfilled.

Merge order

#355 first. It carries the retrospective record for #334; if this lands first, #355's already-completed checks won't re-run and main would briefly contain a 15-file trait change with no record.

One self-inflicted mess worth flagging: I switched branches with this PR's working-tree changes present and git add history/ swept history/README.md into a commit on #355's branch. Fixed forward-only with a follow-up commit rather than by rewriting the pushed branch — the two-commit trail shows the mistake, which is better than hiding it.

just qc green · 517 tests pass · ruff clean.

[culturebot-reviewer]

Claude finished @realmarcin's task in 4m 14s —— View job

---

Review

Two things need changing. The design call — blocking, one record per change, rule in a testable script rather than in workflow YAML — is right, and the || status=$? and setup-just fixes are real bugs correctly found.

---

🟡 The rule this PR establishes is still contradicted in three canonical places, one of which states the opposite policy outright.

• src/traitmech/schema/history.yaml:20-23 — "ENFORCEMENT IS DELIBERATELY SPLIT. Presence of a record is *advisory* — CI warns, it does not block, because a hard gate on provenance blocks legitimate work at inconvenient moments and trains people to route around it." This is false the moment the PR merges, and it is verbatim the argument the PR body refutes with the 134/2 measurement.
• src/traitmech/schema/history.yaml:9 and history/README.md:3 and justfile:345 — all still state "One record per session per target" / "One file per session per target" as the unqualified rule. history/README.md:78 re-scopes that phrase to hand curation, but the README's own lede, 73 lines above, still presents it as general.
• justfile:345 sits directly above new-history — the command the failure message tells people to run.

The PR argues that an unenforced convention drifts. Three surviving statements of the superseded rule are that drift, on day one, in the artifacts a reader hits first. The schema is vendored from claw, so if editing it here is off the table say so — but history/README.md:3 and justfile:345 are TraitMech's own. Fix this →

🟡 TRAIT_GLOB / HISTORY_GLOB under-match: git pathspec ** is not shell **.

scripts/audit_history_records.py:47-48. Without :(glob) magic, git's * already crosses /, and the literal / in **/ still has to be consumed — so data/traits/**/*.yaml requires at least one intervening directory. Verified in this checkout:

git ls-files -- 'scripts/**/*.py'      → 0
git ls-files -- 'scripts/*.py'         → 41
git ls-files -- 'data/traits/*.yaml'   → 477   (same 477 as the '**' form)

Nothing bypasses it today — all 477 trait files are data/traits/<category>/<file>.yaml. But the workflow's own trigger is paths: data/traits/**, and that glob is GitHub Actions semantics, which does match data/traits/foo.yaml. So a trait added at the top level starts the job and then clears the presence gate with "0 trait records changed" — the silently-permissive failure the test file's docstring says it is pinning against. Same shape for history/**/*.yaml: a record at history/foo.yaml wouldn't count toward presence.

Fix is to delete the **/ from both; data/traits/*.yaml is strictly more inclusive and matches all 477 today. Worth a test, too — missing_record is well covered but collect(), where the glob actually lives, has none, which is why the unit tests can't see this. Fix this →

🔵 Over-claim worth softening. The docstring, the workflow comment and history/README.md all say scaffolding an empty record "does not work". The guard is pattern: '^(?!TODO: replace this placeholder)' (history.yaml:265), and new_history_record.py:53-55 already notes it only catches that exact prefix. --details 'see PR' passes both gates. That's fine as a design — the gate isn't meant to judge substance — but stating it as closed invites someone to rely on it.

---

Not blocking, for the record: the schema-level anti-gaming pattern is real, audit_required_workflows.py derives its required set rather than declaring it so the new blocking step needs no registration, the setup-just pin matches the four sibling call sites exactly, and keeping audit-history-records out of qc is correct since it needs a base ref.

I could not execute the test suite here — uv run pytest is outside this job's allowed tools — so the tests are reviewed statically only.
· fix/325-enforce-history

[culturebot-reviewer]

Two 🟡 items — see the detailed review comment.

1. The rule this PR establishes is still contradicted in three canonical places: src/traitmech/schema/history.yaml:20-23 still states presence is advisory and CI does not block; history/README.md:3, justfile:345 and history.yaml:9 still state 'one record per session per target' unqualified.

2. scripts/audit_history_records.py:47-48 — git pathspec ** is not shell **. data/traits/**/*.yaml requires an intervening directory, so a trait at data/traits/foo.yaml starts the job (the workflow's paths: filter uses Actions glob semantics, which match it) and then clears the presence gate. Drop the **/; data/traits/*.yaml matches all 477 today. collect() has no test coverage, which is why the unit tests can't see it.

The design call — blocking, one record per change, rule in a testable script — is right, and the || status=$? and missing-just fixes are real.

[realmarcin]

Both correct, and the pathspec one is a live bug rather than prose.

🟡 ** in a git pathspec

Verified exactly as you described:

git ls-files -- 'scripts/**/*.py'   →   0
git ls-files -- 'scripts/*.py'      →  41

and with a probe file at data/traits/_probe.yaml: the ** form saw 0, the * form saw 1.

Your second-order point is the one that mattered: the workflow's trigger is paths: data/traits/**, which is GitHub Actions semantics and does match a top-level file — so the job would start and then clear the gate reporting "0 trait records changed". Silently permissive, and it's precisely the failure my own test docstring claimed to be pinning against.

Sharper still: why the unit tests couldn't see it. The glob lives in collect(), which had no tests at all, while missing_record() was thoroughly covered — I tested the easy half. Two tests now build a real git repo with a top-level and a nested trait and assert collect() returns both. Confirmed they fail against the old globs and pass against these, so they pin the fix rather than restate it.

Both globs lose the **/.

🟡 Three surviving statements of the superseded rule

This is my own argument turned on itself — the PR says an unenforced convention drifts, and the drift was on day one in the artifacts a reader hits first.

• history/README.md:3 — fixed. The lede now states the one-record-per-change rule up front, rather than being corrected 73 lines later.
• justfile:345 — fixed, and you're right that it's the worst of the three: it sits directly above new-history, the command the failure message tells people to run.
• src/traitmech/schema/history.yaml — editing it here is off the table, and thanks for offering that out explicitly. It's vendored byte-identical from claw, which is private and unreachable from this repo's CI, and src/traitmech/schema/history.yaml is not in check_vendored_sync.sh's checked set — so a one-copy edit would create drift that nothing detects, which is exactly the gap vendored-sync.yaml is now a fourth de-facto shared file with no drift protection #209 tracks. Filed as Vendored history.yaml still states the pre-#325 enforcement policy #358 with the re-vendoring path, and history/README.md now says the schema's prose is stale by design rather than neglect so the next reader isn't left to guess.

🔵 The over-claim

Taken, and it was in three places. The guard is a schema pattern rejecting the literal TODO: replace this placeholder prefix and nothing else — --details 'see PR' passes both gates. All three now say what the gate actually does: it asks whether provenance was recorded, not whether it was recorded well. Stating it as closed invites exactly the reliance you describe.

On your last note — you're right that the test claims were unverified from your side. just qc green · 519 tests pass · ruff clean.

[culturebot-reviewer]

Claude finished @realmarcin's task in 3m 26s —— View job

---

Review

Both 🟡 items are fixed, and the fixes are right rather than merely responsive.

---

Pathspec — scripts/audit_history_records.py:58-59 now uses data/traits/*.yaml / history/*.yaml. Re-verified in this checkout that the semantics are what the fix assumes:

git ls-files -- 'scripts/**/*.py'  →  0     # the old form needed an intervening dir
git ls-files -- 'scripts/*.py'     → 41
git ls-files -- 'history/*.yaml'   →  4     # incl. history/records/<slug>/<file>.yaml

That last one is the load-bearing check: the depth-3 records still match, so dropping **/ widened coverage without losing any. The two new collect() tests build a real repo with a top-level and a nested file on each side and assert both come back — they exercise the pathspec itself, which is where the bug was and where there was no coverage. The gap you named (missing_record well covered, collect not at all) is closed rather than papered over.

Prose — history/README.md:3-7 now leads with one-record-per-change, and justfile:344-348 no longer contradicts the command sitting directly under it. Declining to edit src/traitmech/schema/history.yaml is the correct call and the stated reason checks out: scripts/check_vendored_sync.sh has no reference to that path (its FILES/MAPPED arrays don't cover it), so a one-copy edit really would be undetected drift. The new "The vendored schema still states the old policy" section pointing at #358 is the right disposition — a reader who hits the stale schema prose now knows it's stale by decision.

The workflow's || status=$? and the setup-just pin were already correct; nothing in the follow-up commit regressed them.

---

🔵 One factual correction to the response, not a request. The over-claim is fixed in the docstring (audit_history_records.py:30-35 now says the pattern rejects the literal prefix "and nothing else — --details 'see PR' passes"), but history/README.md and .github/workflows/curation-history.yaml:6-8 both still end on "so scaffolding an empty record to satisfy the presence gate does not work." Conditioning it on the placeholder being unfilled makes that sentence true as written, so it's no longer misleading — but two of the three still carry the conclusion clause, not just the docstring's narrower statement. Fine to leave.

---

Approving. I could not execute pytest here — uv run is outside this job's allowed tools — so the 519-test claim is unverified from my side; the two new tests are reviewed statically and their assertions match the pathspec behaviour I confirmed with git ls-files above. Merge order stands: #355 first.
· fix/325-enforce-history

[culturebot-reviewer]

Both 🟡 items fixed and verified.

Pathspec: data/traits/*.yaml / history/*.yaml are correct — re-confirmed history/*.yaml still matches the depth-3 records, so dropping **/ widened coverage without losing any. The two new collect() tests exercise the pathspec itself, closing the coverage gap that hid the bug.

Prose: README lede and justfile:344 now state one-record-per-change. Leaving src/traitmech/schema/history.yaml alone is the right call — check_vendored_sync.sh genuinely does not cover that path, so a one-copy edit would be undetected drift; #358 plus the new README section is the correct disposition.

🔵 Non-blocking: the 'scaffolding an empty record does not work' conclusion still stands in history/README.md and the workflow comment; conditioning it on the unfilled placeholder makes it true as written, so fine to leave.

Could not run pytest in this job, so the 519-test claim is unverified from my side. Merge #355 first.