Review Low: the module comment and the _edit_trailer_armed summary still said
"a commit message ... does arm" / "some commit", while the matcher excludes merges.
The decision Rule and catalog already carried the qualification; these two did not.
Comment-only.
fixes#609
Decisions-Edit: yes
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review Low x2, both correct and both the stale-comment class:
The claim that prepush-rebase-check.sh forbids merging main into a PR branch is
false. That hook refuses a branch that is BEHIND origin/main; a merge makes
origin/main an ancestor, so the push is allowed. Merging main in is discouraged by
convention only. So --no-merges does cost a real false negative: an author who marks
ONLY a conflict-resolving merge commit gets a legitimate rewrite rejected. Keeping
--no-merges and stating the trade explicitly -- that failure is loud and costs one
extra commit, whereas honoring forge-composed merge bodies disables the guard
silently, which is #609 itself.
The module comment also still claimed a quoted example cannot arm the guard, which
contradicts the residual the decision record now states accurately. Aligned both, and
narrowed the record's Rule line from "some commit" to "some NON-MERGE commit" so the
stated contract matches the implementation.
No logic change -- comments, docstring, record prose and regenerated catalog only.
fixes#609
Decisions-Edit: yes
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Cross-family review (High + Medium + Low), all three reproduced before fixing:
High -- on a pull_request event actions/checkout lands on a synthetic merge commit whose
body the forge composes from the PR description, so a description ending in an example
marker armed a guard no author armed. Excluded merge commits from the range; merging main
into a PR branch is separately forbidden, so no author-written commit is skipped.
Medium -- a folded value (`no` + continuation ` yes`) was split into independent lines and
the continuation armed on its own, inverting the value the author wrote. Read with `unfold`
so the value is judged whole.
Low -- the module docstring promised fail-open while marker resolution deliberately fails
closed. The posture is right; the docstring was wrong. Documented as the one exception.
Both new negative controls verified red against the unfixed matcher. Decision record
amended to state the residual honestly rather than overclaim: a quoted example that is the
FINAL paragraph of an ordinary commit is a trailer by git's own grammar and does arm. What
the change buys is that discussing the marker can no longer disable anything.
fixes#609
Decisions-Edit: yes
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The rationale-edit exemption was a substring test over the whole commit range, so any
message containing the literal marker armed it -- including prose explaining why no
marker was needed, which is how it fired live in PR #605: a green --base/--head run
that was vacuous on the body-diff dimension, in the one PR that hand-resolved a merge
conflict inside the corpus the guard exists to police.
Now read as an affirmative `Decisions-Edit:` git trailer. Git parses trailers only in
the final paragraph, so a quoted example message cannot arm it -- which matters because
this commit and its decision record both quote one. A non-affirmative value (`no`) does
not arm it either; the retired substring arms nothing and gets a ::warning:: nudge.
Tests: negative controls (prose mention, quoted mid-body trailer, `no` value, retired
substring) plus positive controls (trailer, uppercase, alongside Co-Authored-By). All
four negative controls verified red against the old matcher.
fixes#609
Decisions-Edit: yes
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1. `date.fromisoformat` is not a YYYY-MM-DD validator. On Python >= 3.11 it also
accepts ISO basic format ("20270101") and week dates ("2027-W01-1"), so a
malformed-looking value passed the blocking check — and which forms parse
depends on the interpreter, meaning the same corpus could validate differently
on a dev machine and on the runner (pr-checks.yml pins only python-version
'3.x'). Knock-on: the catalog's Review-due section sorts on the raw STRING, so
an accepted "20270101" sorted AFTER "2027-01-15" ('-' < '0'), contradicting the
section's own "sorted soonest-first" text. Gate on ^\d{4}-\d{2}-\d{2}$ first,
which fixes both — a fixed-width zero-padded form makes string sort == date sort.
2. A present-but-empty `stale-after:` was collapsed to None by `or None` in the
parser and then skipped by a truthiness guard in the validator, so it passed as
"absent" — a field that silently never fires, which is the exact failure mode
the blocking check exists to prevent. Keep "" distinct from None and test with
`is not None`.
3. `test_catalog_is_date_independent` was partly vacuous: with no date in either
render, both sides were trivially equal after the .replace(). It did still catch
an injected clock-derived marker, but it passed with the feature deleted. Assert
the dates are present.
4. The malformed-date check ran only over the active set, exempting archive
records. Staleness is moot there, but a typo is still a typo — check both wings.
Adds regression tests for each, plus a Review-due row for a topic-file record
(pinning the `../decisions.md` vs bare-filename link forms).
Evaluated the Open Knowledge Format (GoogleCloudPlatform/knowledge-catalog okf
v0.2, scaccogatto/okf-skills) as a replacement for our decision-record system and
rejected it: its conformance rules are deliberately permissive exactly where ours
are strict (broken links, unknown types and missing fields must all be tolerated;
`deprecated` points at no successor), and its stable identity is the file path,
which the breadcrumb rule tells agents not to trust.
Adopted two of its optional families instead, additively:
- `stale-after: YYYY-MM-DD` on the metadata line — marks a record asserting an
outside-world fact as due for re-confirmation. Absolute date, no TTL.
- `**Sources:**` in the metadata block — the evidence a record rests on, as
distinct from `Signals:` (recall keywords).
Neither is required; absence is never an error. A malformed `stale-after` is
blocking (it would silently never fire), but a past-due record is only a
non-blocking `::notice::` — going stale is the passage of time, not a defect in
whatever commit is under test. The catalog's new "Review due" section renders the
date only and never a clock-derived verdict, so it cannot drift `--check` red on a
calendar boundary with no commit touching the corpus.
No backfill: no existing record adopts either field here.
fixes#603
The decisions-guard validator enforced lifecycle metadata (key/status/since/
supersedes/superseded-by + reciprocal links) but not the **Signals:** line —
which is exactly what MemPalace's keyword recall matches on. A record without
it ingests with weak recall metadata and produces confident false-negatives
for the "MemPalace to find, file to confirm" retrieval workflow.
Add "signals" to REQUIRED_META so a migrated record with a missing or empty
**Signals:** line fails the same way a missing key does. All 114 active + 2
archive records already carry a Signals line, so this is non-breaking on the
current corpus. Archive records are intentionally out of scope (recall targets
the active corpus).
- scripts/decisions_validate.py: signals in REQUIRED_META (+ rationale comment)
- scripts/tests/test_decisions_validate.py: _rec() default + missing/empty/present cases
- docs/decisions.md: header + Enforcement note the requirement and why
fixes#545
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Timothy asked why the kickoff handoff doc stores historical narrative when
it should be instructions. It shouldn't — its own lore section is chartered
as "STANDING workflow/orchestration rules only" with the why belonging in
docs/decisions.md. But an inventory of every bullet against the decision
corpus inverted the premise: only ~8 of ~38 were actually covered. 19 had
no record anywhere and 11 more were half-covered, so that single file was
the ONLY copy of the mandatory review rubric, the whole CI-triage
vocabulary, the build-concurrency policy, the H12 session-end audit, and
the plumbing-merge recipe. Pruning first would have destroyed them.
So the records come first. New topic file docs/decisions/workflow-process.md
carries 32 records (ci.*, process.*, testing.*) covering every NONE and
PARTIAL the inventory found, including the Gitea `?milestones=` no-op bug
whose only copy was the archived selector section this prune deletes.
Only then the prune: HARD CONSTRAINTS and the lore section become one- or
two-line rules, each citing the decision key that holds its evidence, and
the 40-line "Archived — do not follow" section is gone. 636 -> 353 lines,
with every cited key verified to resolve against the corpus.
The aggregate corpus budget is re-baselined 4800 -> 5600 with the reason in
the code: the corpus grew because knowledge MOVED into it, which is the
system working, not drift.
refs #542
- Exclude docs/decisions/retrieval-eval.md from active decision parsing
(_NON_DECISION_FILES); its `## N.` eval-question headings were being
miscounted as 7 legacy-unmigrated records.
- Add decisions_lib.metadata_line_count() + a decisions_validate guard
that fails a record with more than one `key:` metadata line, so a
stacked-metadata-block migration bug (which the parser silently
tolerated by reading only the first block) can't recur unnoticed.
TDD: test_duplicate_metadata_block_fails / test_single_metadata_block_passes.
- De-duplicate the 6 docs/decisions.md records left with two stacked
metadata blocks (scan.getoraddfolder-db-lookup #488,
scan.musicvideo-reconciliation #494, scan.jellyfin-mixed-content-library
#489, iptv.logo-drives-bug-preset #67, ffmpeg.qsv-decode-encode-split
#498, ci.small-lane-git-only server-management#639), merging the union
of Signals/paths/issues/Mechanics from both blocks and keeping the
richer Rule wording; rationale prose untouched.
- Fill in the deferred Q6b row in docs/decisions/retrieval-eval.md now
that startup.parallel-orientation is active in docs/decisions.md,
scoring it as a real active-vs-superseded question against the
archived docs.queue-state-gitea-tracker.
- Regenerate docs/decisions/README.md via build_decisions_catalog.py.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adds scripts/decisions_validate.py: lifecycle invariant checks (unique
active key, key format, reciprocal supersession, removed-without-archive,
rationale-rewrite-without-token, catalog staleness, corpus budget) plus
git-diff helpers for merge-base-based CI checks. Deviates from the task
brief in one spot: REQUIRED_META narrowed to (key, status) — the brief's
(key, status, since, supersedes, superseded_by) makes its own
test_clean_corpus_passes fail, since since/supersedes/superseded_by
default to None on bare Record() instances built without going through
the markdown parser.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
timothyandClaude Opus 5