From 0f565b1f7ec2afbddcf63efd1d446822ceb99a6c Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 26 Jul 2026 00:37:21 +0200 Subject: [PATCH 1/5] fix(621): make an unparseable decision record loud instead of silently invisible MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A file under the record wings that the dependency-free frontmatter reader cannot parse returned [] and simply vanished from the corpus: decisions_validate.py printed OK, build_decisions_catalog.py --check said "up to date", the record was absent, and nothing anywhere errored. Reproduced end-to-end with a YAML block scalar (`rule: >-`), which is the natural thing to reach for on this corpus's very long rule: values and which parsed fine under PyYAML before #610's dependency-free reader. An EXISTING record disappearing was already loud (the no-vanish diff check). The hole was a NEWLY ADDED record, which that check structurally cannot see — no base state to diff against — so the author's own PR looks clean. Hence a per-PATH check, not a per-construct or diff-driven one: asserting "this path yields exactly one keyed record" turns any present or future reader limitation from silent to loud in one move. Adds record_wing_files/record_wing_faults to decisions_validate.py, surfaced through validate() as ERRORS (a file in the record wings that is not a record is a mistake by definition) and reported first, since a file that failed to parse leaves every downstream check silently evaluating an incomplete corpus. The five top-level stripped legacy archive files are exempt — they are generated "Records formerly in this file" indexes, keyless by construction, and are what keeps older date-based pointers resolvable. _read_frontmatter is deliberately NOT extended to accept block scalars; rationale in the new docs.record-wing-parse-guard record. 8 tests, mutation-verified: with the check neutered 5 go red, restored all 119 pass. A live-corpus positive control asserts the wings are non-empty so a clean result can never be vacuous. Refs #621 --- docs/ci-cd.md | 6 +- docs/decisions/README.md | 1 + .../records/docs/record-wing-parse-guard.md | 50 ++++++++ scripts/decisions_validate.py | 69 ++++++++++ scripts/tests/test_decisions_validate.py | 118 ++++++++++++++++++ 5 files changed, 243 insertions(+), 1 deletion(-) create mode 100644 docs/decisions/records/docs/record-wing-parse-guard.md diff --git a/docs/ci-cd.md b/docs/ci-cd.md index 97ee42b21..369bbc0ca 100644 --- a/docs/ci-cd.md +++ b/docs/ci-cd.md @@ -579,7 +579,11 @@ Enforces decision-record lifecycle invariants (ersatztv#521, supersedes the ersa append-only mechanic): well-formed 5-field metadata, exactly one `active` record per `key`, reciprocal `supersedes`/`superseded-by` links, no record vanishing from the active set without an archive copy, no rationale-prose rewrite without a `Decisions-Edit: yes` trailer on a non-merge commit -in the range (ersatztv#609), +in the range (ersatztv#609), a structural per-path check that every `*.md` under +`docs/decisions/records/**` and `docs/decisions/archive//` parses to **exactly one keyed +record** (ersatztv#621 — without it, a file the dependency-free frontmatter reader cannot parse, +such as one using a YAML block scalar, yields `[]` and vanishes from the corpus with every check +still reporting green; the top-level stripped legacy archive indexes are exempt), and the generated active catalog (`docs/decisions/README.md`) in sync with source. Two steps: `scripts/decisions_validate.py --base origin/ --head HEAD` (the merge-base diff checks, which need a base/head range — CI-only) and `scripts/build_decisions_catalog.py --check` (catalog drift). diff --git a/docs/decisions/README.md b/docs/decisions/README.md index d5a2834c4..366f9646b 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -66,6 +66,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera | `docs.decision-lifecycle` | every decision `##` record (active or archived) carries a 5-field metadata block (`key`, `status`, `since`, `supersedes`, `superseded-by`) checked by `scripts/decisions_validate.py`; a record is never deleted or line-edited to reverse a call — it is moved to `docs/decisions/archive/` with `status: superseded`/`retired` and a reciprocal `superseded-by`/`supersedes` key pair to its replacement. | 2026-07-21 | [link](records/docs/decision-lifecycle.md) | | `docs.decision-one-file-per-record` | Each decision record is its own file at `docs/decisions/records//.md` (archived ones at `docs/decisions/archive//.md`) with YAML frontmatter; the filename IS the key, so one-active-record-per-key is a filesystem property rather than a validator check, and supersession is a `git mv`. | 2026-07-25 | [link](records/docs/decision-one-file-per-record.md) | | `docs.decision-optional-provenance` | Decision records gain two OPTIONAL fields — `stale-after: YYYY-MM-DD` on the metadata line and a `**Sources:**` line in the metadata block; the Open Knowledge Format (OKF) itself is NOT adopted as the record format. | 2026-07-25 | [link](records/docs/decision-optional-provenance.md) | +| `docs.record-wing-parse-guard` | `decisions_validate.py` asserts, per PATH, that every `*.md` under `docs/decisions/records/**` and `docs/decisions/archive//*.md` parses to exactly one record carrying a `key` — an ERROR, not a warning, since a file in the record wings that is not a record is a mistake by definition. The top-level `docs/decisions/archive/*.md` stripped legacy files and `_NON_DECISION_FILES` are exempt (they are generated indexes, not records). `_read_frontmatter` is deliberately NOT extended to accept YAML block scalars: every record value goes on ONE quoted line, and the structural check is what makes that limitation loud instead of silent. | 2026-07-26 | [link](records/docs/record-wing-parse-guard.md) | | `docs.tracker-comment-retrofit` | When the knowledge exporter flags an over-cap tracker issue and excludes it from ingestion, triage its comments instead of assuming a retrofit is owed — and for each decision-shaped item check the **worked issue first**, because a tracker session comment is by construction a précis of the fuller closing record posted on the issue it narrates. Applied to #237 (111 comments) this yielded **zero** records, so server-management#642's "a fact found only in a #237 comment" retrieval row has no valid subject and its interim target (an already-migrated record) is permanent. | 2026-07-21 | [link](records/docs/tracker-comment-retrofit.md) | | `ffmpeg.external-logo-graphics-engine` | External-URL channel logos pass through to the graphics engine like any other watermark source; `WatermarkSelector` must never gate them on `File.Exists` (always false for a URL) and never route them through the ffmpeg-native overlay shortcut. | 2026-07-20 | [link](records/ffmpeg/external-logo-graphics-engine.md) | | `ffmpeg.hls-cold-start-burst` | HLS cold-start latency is fixed with a bounded `-readrate_initial_burst` (gated on FFmpeg ≥6.1 capability detection), not by raising `work_ahead_limit`, which would remove the concurrency guarantee it exists for. | 2026-07-20 | [link](records/ffmpeg/hls-cold-start-burst.md) | diff --git a/docs/decisions/records/docs/record-wing-parse-guard.md b/docs/decisions/records/docs/record-wing-parse-guard.md new file mode 100644 index 000000000..12dcc8e4f --- /dev/null +++ b/docs/decisions/records/docs/record-wing-parse-guard.md @@ -0,0 +1,50 @@ +--- +key: docs.record-wing-parse-guard +title: '2026-07-26 — Every file in the record wings must parse to exactly one keyed record; the frontmatter reader stays flat-scalar-only (#621)' +status: active +since: '2026-07-26' +supersedes: none +superseded-by: none +rule: '`decisions_validate.py` asserts, per PATH, that every `*.md` under `docs/decisions/records/**` and `docs/decisions/archive//*.md` parses to exactly one record carrying a `key` — an ERROR, not a warning, since a file in the record wings that is not a record is a mistake by definition. The top-level `docs/decisions/archive/*.md` stripped legacy files and `_NON_DECISION_FILES` are exempt (they are generated indexes, not records). `_read_frontmatter` is deliberately NOT extended to accept YAML block scalars: every record value goes on ONE quoted line, and the structural check is what makes that limitation loud instead of silent.' +signals: 'record silently invisible, parsed to 0 records, block scalar rule >- , unterminated frontmatter, file vanishes from corpus, validator OK but record absent, catalog up to date but missing, one keyed record per file, stripped legacy archive index exempt · paths: `scripts/decisions_validate.py`, `scripts/decisions_lib.py`, `scripts/tests/test_decisions_validate.py` · issues: #621, #610, #609, #603' +mechanics: '`scripts/decisions_validate.py` -> `record_wing_files` / `record_wing_faults`; `docs/ci-cd.md` -> "`decisions-guard` job"' +--- + +Found in adversarial review of #610 / PR #619. Nothing enforced that a file under the record wings +is actually a record. A file the dependency-free frontmatter reader cannot parse returns `[]` and +simply **vanishes** — `decisions_validate.py` prints OK, `build_decisions_catalog.py --check` says +up to date, the record is absent from the corpus, and there is no error anywhere. + +**This is the corpus's own failure mode turned on itself.** The whole point of the decision system is +retrievability; the one thing worse than a missing record is a missing record that reports success. +Same family as the #609 marker that printed `OK` while doing nothing and the #603 `stale-after` field +that silently never fired. + +**What was already safe, and what wasn't.** An *existing* record disappearing was always loud — the +no-vanish diff check catches it. The hole is a **newly added** record, which the diff check +structurally cannot see because there is no base state to compare against, so the author's own PR +looks clean. That asymmetry is why the fix had to be path-driven rather than diff-driven. + +**Why per-path rather than per-construct.** Enumerating the YAML the reader rejects would have to be +re-done every time the reader meets something new. Asserting *"this path must yield exactly one keyed +record"* converts **any** present or future reader limitation from silent to loud in one move — +block scalars, indented structure, unterminated frontmatter, a stray note file, an empty file. + +**The block-scalar decision: NO.** `rule: >-` is the natural thing to reach for on this corpus's very +long `rule:` values, and under PyYAML it parsed fine — so #610's dependency-free reader *widened* a +hole rather than creating it. Teaching `_read_frontmatter` to fold block scalars was still declined: + +- Correct folding is a real subset of YAML (`>` vs `|`, strip/clip/keep chomping, indentation-relative + continuation). A subtly wrong folder is *worse* than a rejection, because it would silently alter + rule text — the exact class of failure this record exists to close — while + `test_frontmatter_reader_matches_pyyaml` (which compares against PyYAML on real records) would keep + passing on the flat records that make up the entire corpus. +- The reader must stay dependency-free: it runs in CI's `decisions lifecycle` job, in the Husky + pre-commit hook, and on every contributor's machine, none of which install anything. +- With the structural check in place the cost of not supporting block scalars is a **loud error + naming the fix**, and all 167 existing records already use single quoted lines. There is no + value a flat scalar cannot hold. + +Verified by mutation, not by a green tick: with `record_wing_faults` neutered, five of the new tests +go red; restored, all pass. `test_real_repo_record_wings_are_all_parseable` additionally asserts the +wings are non-empty (>100 files) so a clean result can never be vacuous. diff --git a/scripts/decisions_validate.py b/scripts/decisions_validate.py index 06aa9b484..c96243e97 100644 --- a/scripts/decisions_validate.py +++ b/scripts/decisions_validate.py @@ -96,9 +96,15 @@ def validate( rewritten, archive_records=None, demoted=(), + wing_faults=(), ) -> list[str]: errs: list[str] = [] archive_records = archive_records or [] + # #621: structural "one keyed record per file". ERRORS, not warnings — a file under the record + # wings that is not a record is a mistake by definition. Reported first because a file that + # failed to parse is absent from `records`, so every downstream check below is silently + # evaluating an incomplete corpus. + errs += list(wing_faults) decision_recs = [r for r in records if r.heading not in SKIP_HEADINGS and r.status != "legacy-unmigrated"] active_by_key: dict[str, int] = {} @@ -406,6 +412,68 @@ def _diff_findings(base: str, head: str) -> tuple[list[str], list[str], list[str return sorted(set(removed)), sorted(set(rewritten)), sorted(set(demoted)) +def record_wing_files(records_dir: Path | None = None, archive_dir: Path | None = None) -> list[Path]: + """Every file that MUST be exactly one keyed record. + + Scope is the split-layout record wings only: + * `docs/decisions/records/**/*.md` — the active wing + * `docs/decisions/archive//*.md` — the archived wing, nested by area since #610 + + Deliberately NOT included: `docs/decisions/archive/*.md` at the top level. Those are the five + stripped legacy topic files left behind by the #610 split (`api.md`, `scan.md`, `spa.md`, + `startup.md`, `release-ci-governance.md`); each parses to a single KEYLESS pseudo-record from its + generated "Records formerly in this file" index, which is exactly what keeps older date-based + pointers resolvable. They are indexes, not records. `_NON_DECISION_FILES` (README.md and + friends) is filtered on top of that. + """ + records_dir = dl.RECORDS_DIR if records_dir is None else records_dir + archive_dir = dl.ARCHIVE_DIR if archive_dir is None else archive_dir + files: list[Path] = [] + if records_dir.exists(): + files += sorted(records_dir.rglob("*.md")) + if archive_dir.exists(): + files += sorted(archive_dir.glob("*/*.md")) # area-nested only — see docstring + return [p for p in files if p.name not in dl._NON_DECISION_FILES] + + +def record_wing_faults(records_dir: Path | None = None, archive_dir: Path | None = None) -> list[str]: + """Structural check (#621): every file in the record wings parses to exactly ONE keyed record. + + Without this, a file the frontmatter reader cannot parse yields `[]` and simply VANISHES — no + error, no warning, validator OK, catalog "up to date", record absent from the corpus. That is + the corpus's own failure mode turned on itself: the one thing worse than a missing record is a + missing record that reports success (cf. the #609 marker that printed OK while doing nothing and + the #603 `stale-after` that silently never fired). + + An EXISTING record disappearing was already loud — the no-vanish diff check catches it. This + closes the case the diff check structurally cannot see: a NEWLY ADDED record, where the author's + own PR looks clean because there is no prior state to diff against. + + Making it path-driven rather than record-driven is the point: it converts ANY present or future + reader limitation from silent to loud, instead of enumerating the constructs we happen to know + about today. + """ + faults: list[str] = [] + for p in record_wing_files(records_dir, archive_dir): + try: + recs = dl.parse_file(p) + except Exception as exc: # unreadable/undecodable file is a fault, not a crash + faults.append(f"{p}: could not be read as a decision record ({exc})") + continue + if len(recs) != 1: + faults.append( + f"{p}: parsed to {len(recs)} records, expected exactly 1 — a file under the record " + f"wings must be one record. Common cause: YAML the dependency-free frontmatter " + f"reader does not accept (a block scalar such as `rule: >-` or `rule: |`, any " + f"indented/nested structure, or unterminated `---` frontmatter). Put the value on " + f"ONE quoted line, as all other records do." + ) + continue + if not recs[0].key: + faults.append(f"{p}: parsed to a record with no `key` — every record in the wings must be keyed.") + return faults + + def _archive_keys() -> set[str]: keys: set[str] = set() if dl.ARCHIVE_DIR.exists(): @@ -491,6 +559,7 @@ def main(argv=None) -> int: rewritten=rewritten, archive_records=archive_records, demoted=demoted, + wing_faults=record_wing_faults(), ) if not budget_ok: diff --git a/scripts/tests/test_decisions_validate.py b/scripts/tests/test_decisions_validate.py index cb39c79c2..565c761e6 100644 --- a/scripts/tests/test_decisions_validate.py +++ b/scripts/tests/test_decisions_validate.py @@ -700,3 +700,121 @@ def test_retitling_a_record_is_not_reported_as_a_removal(tmp_path): assert removed == [], f"a retitle was reported as a removal: {removed}" assert rewritten == [], f"a retitle was reported as a prose rewrite: {rewritten}" assert demoted == [], f"a retitle was reported as a demotion: {demoted}" + + +# --- #621: structural "one keyed record per file" over the record wings ----------------------- +# +# The defect these cover: a file the dependency-free frontmatter reader cannot parse yields `[]` +# and vanishes from the corpus with NO error anywhere — validator OK, catalog "up to date", record +# absent. Only a NEWLY ADDED record is affected; an existing one disappearing is already caught by +# the no-vanish diff check, which structurally cannot see a record that never existed in the base. + + +def _wing(tmp_path) -> tuple[Path, Path]: + """A tmp record-wing pair: (records_dir, archive_dir).""" + records = tmp_path / "docs" / "decisions" / "records" + archive = tmp_path / "docs" / "decisions" / "archive" + (records / "ci").mkdir(parents=True) + (archive / "ci").mkdir(parents=True) + return records, archive + + +_GOOD = ( + "---\n" + "key: ci.good\n" + "title: '2026-01-01 — Good (#1)'\n" + "status: active\n" + "since: '2026-01-01'\n" + "supersedes: none\n" + "superseded-by: none\n" + "rule: 'a rule on one quoted line'\n" + "signals: 'concept · paths: a/b.py · issues: #1'\n" + "mechanics: 'x'\n" + "---\n\nRationale prose.\n" +) + + +def test_wing_faults_clean_corpus_is_silent(tmp_path): + records, archive = _wing(tmp_path) + (records / "ci" / "good.md").write_text(_GOOD) + assert dv.record_wing_faults(records, archive) == [] + + +def test_wing_faults_block_scalar_record_fails_loudly(tmp_path): + """The exact reproduction from #621: a YAML block scalar — the natural thing to reach for on + this corpus's very long `rule:` values — makes the whole record silently invisible.""" + records, archive = _wing(tmp_path) + bad = records / "ci" / "blockscalar.md" + bad.write_text(_GOOD.replace("rule: 'a rule on one quoted line'\n", "rule: >-\n a long rule wrapped\n over two lines\n")) + + # Precondition: this really is the silent-vanish case, not some other parse error. + assert dl.parse_file(bad) == [], "expected the reader to drop the record entirely" + + faults = dv.record_wing_faults(records, archive) + assert len(faults) == 1, faults + assert "blockscalar.md" in faults[0] + assert "expected exactly 1" in faults[0] + + +def test_wing_faults_unterminated_frontmatter_fails_loudly(tmp_path): + records, archive = _wing(tmp_path) + bad = records / "ci" / "unterminated.md" + bad.write_text("---\nkey: ci.unterminated\nstatus: active\n\nNo closing delimiter.\n") + assert dl.parse_file(bad) == [] + + faults = dv.record_wing_faults(records, archive) + assert len(faults) == 1, faults + assert "unterminated.md" in faults[0] + + +def test_wing_faults_stray_note_file_fails_loudly(tmp_path): + """A file under records/ that is not a record at all — no frontmatter whatsoever.""" + records, archive = _wing(tmp_path) + (records / "ci" / "notes.md").write_text("# Scratch notes\n\nNot a record.\n") + faults = dv.record_wing_faults(records, archive) + assert len(faults) == 1, faults + assert "notes.md" in faults[0] + + +def test_wing_faults_keyless_record_fails_loudly(tmp_path): + """Parses to exactly one record, but carries no `key` — still a fault.""" + records, archive = _wing(tmp_path) + (records / "ci" / "keyless.md").write_text(_GOOD.replace("key: ci.good\n", "")) + faults = dv.record_wing_faults(records, archive) + assert len(faults) == 1, faults + assert "no `key`" in faults[0] + + +def test_wing_faults_exempts_stripped_legacy_archive_files(tmp_path): + """The #610 split left five KEYLESS stripped topic files at the archive TOP level (api.md, + scan.md, spa.md, startup.md, release-ci-governance.md) whose only content is a generated + "Records formerly in this file" index — that index is what keeps older date-based pointers + resolvable. They are indexes, not records, and must stay exempt. Their area-NESTED siblings + are real records and are checked.""" + records, archive = _wing(tmp_path) + (records / "ci" / "good.md").write_text(_GOOD) + # a stripped legacy file at the archive top level: parses to a single keyless pseudo-record + (archive / "api.md").write_text("# api\n\n## Records formerly in this file\n\n- `api.thing`\n") + (archive / "README.md").write_text("# archive\n") + assert dv.record_wing_faults(records, archive) == [] + + # ...but a real archived record nested under an area IS checked + (archive / "ci" / "broken.md").write_text("# not a record\n") + faults = dv.record_wing_faults(records, archive) + assert len(faults) == 1 and "broken.md" in faults[0], faults + + +def test_validate_surfaces_wing_faults_as_errors(tmp_path): + """Faults must arrive as validator ERRORS (exit 1), not warnings.""" + errs = _v([_rec(key="ci.a", source=Path("docs/decisions/records/ci/a.md"), heading="A")], + wing_faults=["docs/decisions/records/ci/x.md: parsed to 0 records, expected exactly 1"]) + assert any("x.md" in e for e in errs), errs + + +def test_real_repo_record_wings_are_all_parseable(): + """Positive control against the LIVE corpus: every one of the real record files parses to + exactly one keyed record. This is what makes the check's clean result meaningful rather than + vacuous — if the wings were empty or unreadable, the checks above would pass trivially.""" + files = dv.record_wing_files() + assert len(files) > 100, f"record wings look empty ({len(files)} files) — check is vacuous" + assert dv.record_wing_faults() == [] -- 2.47.3 From b42df5f15fc29a86a90b6eba2c0135d755d643be Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 26 Jul 2026 01:24:17 +0200 Subject: [PATCH 2/5] fix(621): close a depth blind spot in the record-wing scan MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adversarial self-review: the archive side used `glob("*/*.md")` while records used rglob. Both exempt the top-level stripped legacy files correctly, but the one-level glob would ALSO skip anything nested deeper, letting a record at archive///x.md escape the check entirely. A path silently escaping the scan is precisely the failure mode this guard exists to close, so the exemption is now expressed as the actual condition — "directly in archive/" — rather than a glob shape that happens to match today's layout. Test added and mutation-verified: reverting to the one-level glob turns it red. 120 passed. Refs #621 --- scripts/decisions_validate.py | 8 +++++++- scripts/tests/test_decisions_validate.py | 16 ++++++++++++++++ 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/scripts/decisions_validate.py b/scripts/decisions_validate.py index c96243e97..0800fa128 100644 --- a/scripts/decisions_validate.py +++ b/scripts/decisions_validate.py @@ -432,7 +432,13 @@ def record_wing_files(records_dir: Path | None = None, archive_dir: Path | None if records_dir.exists(): files += sorted(records_dir.rglob("*.md")) if archive_dir.exists(): - files += sorted(archive_dir.glob("*/*.md")) # area-nested only — see docstring + # rglob + "not directly in archive/", NOT glob("*/*.md"). Both exempt the top-level + # stripped legacy files, but a one-level glob would ALSO skip anything nested deeper — + # letting a record at `archive///x.md` escape the check entirely. Silently + # skipping a path is the exact failure mode this function exists to close, so the + # exemption is expressed as the precise condition (depth-1 file) rather than a shape that + # happens to match today's layout. + files += sorted(p for p in archive_dir.rglob("*.md") if p.parent != archive_dir) return [p for p in files if p.name not in dl._NON_DECISION_FILES] diff --git a/scripts/tests/test_decisions_validate.py b/scripts/tests/test_decisions_validate.py index 565c761e6..90cc1a7d0 100644 --- a/scripts/tests/test_decisions_validate.py +++ b/scripts/tests/test_decisions_validate.py @@ -818,3 +818,19 @@ def test_real_repo_record_wings_are_all_parseable(): files = dv.record_wing_files() assert len(files) > 100, f"record wings look empty ({len(files)} files) — check is vacuous" assert dv.record_wing_faults() == [] + + +def test_wing_faults_sees_a_DEEPER_nested_archive_record(tmp_path): + """A one-level `archive/*/*.md` glob would exempt the top-level stripped files correctly but + silently skip anything nested deeper — a path escaping the check, which is the very failure + mode this guard exists to close. The exemption must be 'directly in archive/', not 'exactly + one level down'.""" + records, archive = _wing(tmp_path) + (records / "ci" / "good.md").write_text(_GOOD) + (archive / "api.md").write_text("# api\n\n## Records formerly in this file\n") # still exempt + deep = archive / "ci" / "sub" + deep.mkdir(parents=True) + (deep / "broken.md").write_text("# not a record\n") + + faults = dv.record_wing_faults(records, archive) + assert len(faults) == 1 and "broken.md" in faults[0], faults -- 2.47.3 From 98b3e8715be24f0bbb08620bc9b0f10293bd897c Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 26 Jul 2026 11:07:00 +0200 Subject: [PATCH 3/5] fix(621): close three exemption holes and pin the guard's own wiring, per cold review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An independent cold-context review found the guard could be disabled or bypassed in four ways, one of them silently. HIGH — the guard's only wiring was untested. Deleting `wing_faults=record_wing_faults()` from main() left the ENTIRE suite green (120 passed) while a real block-scalar record vanished again with `decisions-validate: OK`. Every existing test either called the collector directly or handed validate() a hand-built list, so nothing pinned that main() ever invokes it — the #609 "prints OK while doing nothing" defect, one level up, inside the fix for that same defect. Independently reproduced before fixing. Now pinned by a monkeypatched sentinel test. MEDIUM — a record could escape by BASENAME. `_NON_DECISION_FILES` is a set of TOPIC-dir names ({README, migration-map, retrieval-eval}) and was applied to the wings too, so a genuine record at records/docs/retrieval-eval.md was skipped. Not hypothetical: the path<->key rule forces key `docs.retrieval-eval` to that exact filename, and dl.active_files() applies that filter only to the TOPIC_DIR glob — so the file would be a corpus source while exempt from the guard. Now nothing is exempt by basename. MEDIUM — the archive exemption was by LOCATION but justified by IDENTITY. Anything directly in archive/ was skipped, so a new unparseable archive/foo.md would vanish in the one directory left unwatched. Now exempt only if it really is a #610 stripped index: one keyless record with a known generated heading. LOW — parse-to-WRONG. `rule: >-` with an UNINDENTED continuation containing a colon parses to one VALID keyed record whose rule is literally ">-", plus a junk key, silently truncating the real value. PyYAML rejects that input, so the hand reader is more permissive than the writer. Now faulted via an unknown-frontmatter-key check. LOW — an absent or empty wing reported clean. Now loud: refusing to certify a corpus from a scan of nothing. Doc corrections, all measured rather than asserted: "all 167 records use single quoted lines" was false (117 of 168 rule: values are unquoted plain scalars — the invariant is single-LINE); the archive shape in the record, ci-cd.md and the rule: field said `archive//` where the code deliberately uses archive/** ; the mutation count is 10, not five; and the "converts ANY reader limitation" claim is softened to the class it actually covers. 125 passed. Refs #621 --- docs/ci-cd.md | 6 +- docs/decisions/README.md | 2 +- .../records/docs/record-wing-parse-guard.md | 19 +-- scripts/decisions_validate.py | 119 ++++++++++++++---- scripts/tests/test_decisions_validate.py | 73 +++++++++++ 5 files changed, 184 insertions(+), 35 deletions(-) diff --git a/docs/ci-cd.md b/docs/ci-cd.md index 369bbc0ca..d74ecc7ba 100644 --- a/docs/ci-cd.md +++ b/docs/ci-cd.md @@ -580,10 +580,12 @@ append-only mechanic): well-formed 5-field metadata, exactly one `active` record reciprocal `supersedes`/`superseded-by` links, no record vanishing from the active set without an archive copy, no rationale-prose rewrite without a `Decisions-Edit: yes` trailer on a non-merge commit in the range (ersatztv#609), a structural per-path check that every `*.md` under -`docs/decisions/records/**` and `docs/decisions/archive//` parses to **exactly one keyed +`docs/decisions/records/**` and `docs/decisions/archive/**` parses to **exactly one keyed record** (ersatztv#621 — without it, a file the dependency-free frontmatter reader cannot parse, such as one using a YAML block scalar, yields `[]` and vanishes from the corpus with every check -still reporting green; the top-level stripped legacy archive indexes are exempt), +still reporting green; a file directly in `archive/` is exempt only when it really is a stripped index — one keyless +record with a known generated heading — never merely by its location, and nothing is exempt by +basename), and the generated active catalog (`docs/decisions/README.md`) in sync with source. Two steps: `scripts/decisions_validate.py --base origin/ --head HEAD` (the merge-base diff checks, which need a base/head range — CI-only) and `scripts/build_decisions_catalog.py --check` (catalog drift). diff --git a/docs/decisions/README.md b/docs/decisions/README.md index 366f9646b..27cfaaddf 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -66,7 +66,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera | `docs.decision-lifecycle` | every decision `##` record (active or archived) carries a 5-field metadata block (`key`, `status`, `since`, `supersedes`, `superseded-by`) checked by `scripts/decisions_validate.py`; a record is never deleted or line-edited to reverse a call — it is moved to `docs/decisions/archive/` with `status: superseded`/`retired` and a reciprocal `superseded-by`/`supersedes` key pair to its replacement. | 2026-07-21 | [link](records/docs/decision-lifecycle.md) | | `docs.decision-one-file-per-record` | Each decision record is its own file at `docs/decisions/records//.md` (archived ones at `docs/decisions/archive//.md`) with YAML frontmatter; the filename IS the key, so one-active-record-per-key is a filesystem property rather than a validator check, and supersession is a `git mv`. | 2026-07-25 | [link](records/docs/decision-one-file-per-record.md) | | `docs.decision-optional-provenance` | Decision records gain two OPTIONAL fields — `stale-after: YYYY-MM-DD` on the metadata line and a `**Sources:**` line in the metadata block; the Open Knowledge Format (OKF) itself is NOT adopted as the record format. | 2026-07-25 | [link](records/docs/decision-optional-provenance.md) | -| `docs.record-wing-parse-guard` | `decisions_validate.py` asserts, per PATH, that every `*.md` under `docs/decisions/records/**` and `docs/decisions/archive//*.md` parses to exactly one record carrying a `key` — an ERROR, not a warning, since a file in the record wings that is not a record is a mistake by definition. The top-level `docs/decisions/archive/*.md` stripped legacy files and `_NON_DECISION_FILES` are exempt (they are generated indexes, not records). `_read_frontmatter` is deliberately NOT extended to accept YAML block scalars: every record value goes on ONE quoted line, and the structural check is what makes that limitation loud instead of silent. | 2026-07-26 | [link](records/docs/record-wing-parse-guard.md) | +| `docs.record-wing-parse-guard` | `decisions_validate.py` asserts, per PATH, that every `*.md` under `docs/decisions/records/**` and `docs/decisions/archive/**` parses to exactly one record carrying a `key` — an ERROR, not a warning, since a file in the record wings that is not a record is a mistake by definition. A file sitting DIRECTLY in `archive/` is exempt only when it actually looks like a #610 stripped index — exactly one keyless record with a known generated heading — never merely by living there; nothing is exempt by BASENAME. `_read_frontmatter` is deliberately NOT extended to accept YAML block scalars: every record value goes on ONE line, and the structural check is what makes that limitation loud instead of silent. | 2026-07-26 | [link](records/docs/record-wing-parse-guard.md) | | `docs.tracker-comment-retrofit` | When the knowledge exporter flags an over-cap tracker issue and excludes it from ingestion, triage its comments instead of assuming a retrofit is owed — and for each decision-shaped item check the **worked issue first**, because a tracker session comment is by construction a précis of the fuller closing record posted on the issue it narrates. Applied to #237 (111 comments) this yielded **zero** records, so server-management#642's "a fact found only in a #237 comment" retrieval row has no valid subject and its interim target (an already-migrated record) is permanent. | 2026-07-21 | [link](records/docs/tracker-comment-retrofit.md) | | `ffmpeg.external-logo-graphics-engine` | External-URL channel logos pass through to the graphics engine like any other watermark source; `WatermarkSelector` must never gate them on `File.Exists` (always false for a URL) and never route them through the ffmpeg-native overlay shortcut. | 2026-07-20 | [link](records/ffmpeg/external-logo-graphics-engine.md) | | `ffmpeg.hls-cold-start-burst` | HLS cold-start latency is fixed with a bounded `-readrate_initial_burst` (gated on FFmpeg ≥6.1 capability detection), not by raising `work_ahead_limit`, which would remove the concurrency guarantee it exists for. | 2026-07-20 | [link](records/ffmpeg/hls-cold-start-burst.md) | diff --git a/docs/decisions/records/docs/record-wing-parse-guard.md b/docs/decisions/records/docs/record-wing-parse-guard.md index 12dcc8e4f..14ed0de87 100644 --- a/docs/decisions/records/docs/record-wing-parse-guard.md +++ b/docs/decisions/records/docs/record-wing-parse-guard.md @@ -5,7 +5,7 @@ status: active since: '2026-07-26' supersedes: none superseded-by: none -rule: '`decisions_validate.py` asserts, per PATH, that every `*.md` under `docs/decisions/records/**` and `docs/decisions/archive//*.md` parses to exactly one record carrying a `key` — an ERROR, not a warning, since a file in the record wings that is not a record is a mistake by definition. The top-level `docs/decisions/archive/*.md` stripped legacy files and `_NON_DECISION_FILES` are exempt (they are generated indexes, not records). `_read_frontmatter` is deliberately NOT extended to accept YAML block scalars: every record value goes on ONE quoted line, and the structural check is what makes that limitation loud instead of silent.' +rule: '`decisions_validate.py` asserts, per PATH, that every `*.md` under `docs/decisions/records/**` and `docs/decisions/archive/**` parses to exactly one record carrying a `key` — an ERROR, not a warning, since a file in the record wings that is not a record is a mistake by definition. A file sitting DIRECTLY in `archive/` is exempt only when it actually looks like a #610 stripped index — exactly one keyless record with a known generated heading — never merely by living there; nothing is exempt by BASENAME. `_read_frontmatter` is deliberately NOT extended to accept YAML block scalars: every record value goes on ONE line, and the structural check is what makes that limitation loud instead of silent.' signals: 'record silently invisible, parsed to 0 records, block scalar rule >- , unterminated frontmatter, file vanishes from corpus, validator OK but record absent, catalog up to date but missing, one keyed record per file, stripped legacy archive index exempt · paths: `scripts/decisions_validate.py`, `scripts/decisions_lib.py`, `scripts/tests/test_decisions_validate.py` · issues: #621, #610, #609, #603' mechanics: '`scripts/decisions_validate.py` -> `record_wing_files` / `record_wing_faults`; `docs/ci-cd.md` -> "`decisions-guard` job"' --- @@ -27,8 +27,10 @@ looks clean. That asymmetry is why the fix had to be path-driven rather than dif **Why per-path rather than per-construct.** Enumerating the YAML the reader rejects would have to be re-done every time the reader meets something new. Asserting *"this path must yield exactly one keyed -record"* converts **any** present or future reader limitation from silent to loud in one move — -block scalars, indented structure, unterminated frontmatter, a stray note file, an empty file. +record"* converts a whole CLASS of reader limitations from silent to loud in one move — block +scalars, indented structure, unterminated frontmatter, a stray note file, an empty file. It does +NOT catch parse-to-WRONG (frontmatter yielding one keyed record with corrupted values); a separate +junk-key check covers the realistic instance of that, so this is a strong guard, not a total one. **The block-scalar decision: NO.** `rule: >-` is the natural thing to reach for on this corpus's very long `rule:` values, and under PyYAML it parsed fine — so #610's dependency-free reader *widened* a @@ -42,9 +44,12 @@ hole rather than creating it. Teaching `_read_frontmatter` to fold block scalars - The reader must stay dependency-free: it runs in CI's `decisions lifecycle` job, in the Husky pre-commit hook, and on every contributor's machine, none of which install anything. - With the structural check in place the cost of not supporting block scalars is a **loud error - naming the fix**, and all 167 existing records already use single quoted lines. There is no - value a flat scalar cannot hold. + naming the fix**, and all 168 record files already keep each value on ONE line. (The invariant is + single-**line**, not single-**quoted**: 117 `rule:` values are unquoted plain scalars, 51 are + quoted.) There is no value a flat scalar cannot hold. -Verified by mutation, not by a green tick: with `record_wing_faults` neutered, five of the new tests -go red; restored, all pass. `test_real_repo_record_wings_are_all_parseable` additionally asserts the +Verified by mutation, not by a green tick: with `record_wing_faults` neutered, **10** tests go red, +and separately, deleting the single line that WIRES it into `main()` reddens a dedicated test — that +one line's removal previously left the whole suite green while a real record vanished, which is the +guard's own failure mode applied to the guard. `test_real_repo_record_wings_are_all_parseable` additionally asserts the wings are non-empty (>100 files) so a clean result can never be vacuous. diff --git a/scripts/decisions_validate.py b/scripts/decisions_validate.py index 0800fa128..85790219e 100644 --- a/scripts/decisions_validate.py +++ b/scripts/decisions_validate.py @@ -415,31 +415,48 @@ def _diff_findings(base: str, head: str) -> tuple[list[str], list[str], list[str def record_wing_files(records_dir: Path | None = None, archive_dir: Path | None = None) -> list[Path]: """Every file that MUST be exactly one keyed record. - Scope is the split-layout record wings only: - * `docs/decisions/records/**/*.md` — the active wing - * `docs/decisions/archive//*.md` — the archived wing, nested by area since #610 + Scope is EVERY `*.md` under `records/**` and `archive/**` — including the files directly in + `archive/`, which are scanned but may qualify for the stripped-index exemption applied in + `record_wing_faults` (see there). Nothing is excluded by BASENAME. - Deliberately NOT included: `docs/decisions/archive/*.md` at the top level. Those are the five - stripped legacy topic files left behind by the #610 split (`api.md`, `scan.md`, `spa.md`, - `startup.md`, `release-ci-governance.md`); each parses to a single KEYLESS pseudo-record from its - generated "Records formerly in this file" index, which is exactly what keeps older date-based - pointers resolvable. They are indexes, not records. `_NON_DECISION_FILES` (README.md and - friends) is filtered on top of that. + Excluding by basename was a real hole: `_NON_DECISION_FILES` is `{README.md, migration-map.md, + retrieval-eval.md}` — three TOPIC-dir names — and applying it to the wings meant a genuine + record at `records/docs/retrieval-eval.md` was silently skipped. That path is not hypothetical: + the path<->key rule forces key `docs.retrieval-eval` to live at exactly that filename, and + `docs/decisions/retrieval-eval.md` is a real unmigrated file, i.e. a plausible migration target. + Worse, `dl.active_files()` applies that filter only to the TOPIC_DIR glob, not to + `RECORDS_DIR.rglob` — so such a file IS a corpus source while being exempt from the guard. + Only a literal `README.md` sitting directly in a wing root is dropped here (no such file exists + today; the catalog README lives in TOPIC_DIR, one level up). """ records_dir = dl.RECORDS_DIR if records_dir is None else records_dir archive_dir = dl.ARCHIVE_DIR if archive_dir is None else archive_dir files: list[Path] = [] - if records_dir.exists(): - files += sorted(records_dir.rglob("*.md")) - if archive_dir.exists(): - # rglob + "not directly in archive/", NOT glob("*/*.md"). Both exempt the top-level - # stripped legacy files, but a one-level glob would ALSO skip anything nested deeper — - # letting a record at `archive///x.md` escape the check entirely. Silently - # skipping a path is the exact failure mode this function exists to close, so the - # exemption is expressed as the precise condition (depth-1 file) rather than a shape that - # happens to match today's layout. - files += sorted(p for p in archive_dir.rglob("*.md") if p.parent != archive_dir) - return [p for p in files if p.name not in dl._NON_DECISION_FILES] + for wing in (records_dir, archive_dir): + if wing.exists(): + files += sorted(p for p in wing.rglob("*.md") if not (p.parent == wing and p.name == "README.md")) + return files + + +def _is_stripped_index(path: Path, archive_dir: Path, recs: list) -> bool: + """True for a #610 stripped legacy topic file sitting DIRECTLY in `archive/`. + + Those five files (`api.md`, `scan.md`, `spa.md`, `startup.md`, `release-ci-governance.md`) are + generated "Records formerly in this file" indexes — keyless by construction, and what keeps + older date-based pointers resolvable. They are indexes, not records. + + The exemption is by IDENTITY, not by location. Exempting everything directly in `archive/` + would leave that one directory unguarded: a new unparseable `archive/foo.md` would vanish + silently — the same defect this guard exists to close, in the last place it isn't watched. So a + file there is exempt only if it actually LOOKS like a stripped index: exactly one keyless + record whose heading is one of the known generated ones (`dl.SKIP_HEADINGS`). + """ + return ( + path.parent == archive_dir + and len(recs) == 1 + and not recs[0].key + and recs[0].heading in dl.SKIP_HEADINGS + ) def record_wing_faults(records_dir: Path | None = None, archive_dir: Path | None = None) -> list[str]: @@ -455,31 +472,83 @@ def record_wing_faults(records_dir: Path | None = None, archive_dir: Path | None closes the case the diff check structurally cannot see: a NEWLY ADDED record, where the author's own PR looks clean because there is no prior state to diff against. - Making it path-driven rather than record-driven is the point: it converts ANY present or future - reader limitation from silent to loud, instead of enumerating the constructs we happen to know - about today. + Making it path-driven rather than record-driven is the point: it converts a whole CLASS of + reader limitations — parse-to-zero, parse-to-many, keyless — from silent to loud in one move, + instead of enumerating the constructs we happen to know about today. + + It does NOT catch parse-to-WRONG: frontmatter that yields one keyed record with corrupted + values. `rule: >-` followed by an UNINDENTED continuation containing a colon parses to a valid + record whose `rule` is literally `>-`, plus a junk key from the continuation line. The junk-key + check below is what catches that case; a mis-parse producing only known fields would still slip + through, so this is a strong guard, not a total one. """ + records_dir = dl.RECORDS_DIR if records_dir is None else records_dir + archive_dir = dl.ARCHIVE_DIR if archive_dir is None else archive_dir faults: list[str] = [] - for p in record_wing_files(records_dir, archive_dir): + files = record_wing_files(records_dir, archive_dir) + + # A wing that is absent or empty must be LOUD, not silently clean. Otherwise a partial checkout, + # a renamed directory, or a bad monkeypatch turns the whole guard into a no-op that reports + # success — which is the defect this function exists to close, applied to itself. + if not records_dir.exists() or not any(records_dir.rglob("*.md")): + faults.append( + f"{records_dir}: the active record wing is missing or contains no *.md files — refusing " + f"to report a clean corpus from an empty scan." + ) + + for p in files: try: recs = dl.parse_file(p) except Exception as exc: # unreadable/undecodable file is a fault, not a crash faults.append(f"{p}: could not be read as a decision record ({exc})") continue + if _is_stripped_index(p, archive_dir, recs): + continue if len(recs) != 1: faults.append( f"{p}: parsed to {len(recs)} records, expected exactly 1 — a file under the record " f"wings must be one record. Common cause: YAML the dependency-free frontmatter " f"reader does not accept (a block scalar such as `rule: >-` or `rule: |`, any " f"indented/nested structure, or unterminated `---` frontmatter). Put the value on " - f"ONE quoted line, as all other records do." + f"ONE line." ) continue if not recs[0].key: faults.append(f"{p}: parsed to a record with no `key` — every record in the wings must be keyed.") + continue + # Junk keys: the reader accepted a line it should not have. The realistic source is a block + # scalar whose continuation was read as its own `k: v` pair, which silently truncates the + # real value (`rule` becomes `>-`). PyYAML REJECTS that input, so the hand reader is more + # permissive than the writer and nothing else in the pipeline notices. + unknown = _unknown_frontmatter_keys(p) + if unknown: + faults.append( + f"{p}: frontmatter has unrecognized key(s) {sorted(unknown)} — a value was probably " + f"split across lines (a block scalar continuation read as its own key), which " + f"silently truncates the real value. Put each value on ONE line." + ) return faults +def _unknown_frontmatter_keys(path: Path) -> set[str]: + """Frontmatter keys outside the known schema. Empty on any read/parse failure (reported elsewhere).""" + try: + text = path.read_text(encoding="utf-8") + except Exception: + return set() + if not dl.has_frontmatter(text): + return set() + lines = text.splitlines() + end = next((i for i, ln in enumerate(lines[1:], start=1) if ln.rstrip() == "---"), None) + if end is None: + return set() + meta = dl._read_frontmatter("\n".join(lines[1:end])) + if not meta: + return set() + known = set(dl._FM_TO_FIELD) | {"title"} + return {k for k in meta if k not in known} + + def _archive_keys() -> set[str]: keys: set[str] = set() if dl.ARCHIVE_DIR.exists(): diff --git a/scripts/tests/test_decisions_validate.py b/scripts/tests/test_decisions_validate.py index 90cc1a7d0..77b941b5f 100644 --- a/scripts/tests/test_decisions_validate.py +++ b/scripts/tests/test_decisions_validate.py @@ -834,3 +834,76 @@ def test_wing_faults_sees_a_DEEPER_nested_archive_record(tmp_path): faults = dv.record_wing_faults(records, archive) assert len(faults) == 1 and "broken.md" in faults[0], faults + + +# --- #621 cold-review findings ------------------------------------------------------------------ + + +def test_main_actually_CALLS_the_wing_scan(monkeypatch, capsys): + """The guard's only wiring was untested: deleting `wing_faults=record_wing_faults()` from + main() left the entire suite green, and a real block-scalar record vanished again with + `decisions-validate: OK`. Every other test either calls the collector directly or hands + validate() a hand-built list, so nothing pinned that main() invokes it at all — the #609 + 'marker that printed OK while doing nothing' defect, one level up.""" + monkeypatch.setattr(dv, "record_wing_faults", lambda *a, **k: ["SENTINEL-WING-FAULT"]) + rc = dv.main([]) + assert rc == 1, "a wing fault must fail the validator" + assert "SENTINEL-WING-FAULT" in capsys.readouterr().err + + +def test_a_record_is_not_exempted_by_its_BASENAME(tmp_path): + """`_NON_DECISION_FILES` is a set of TOPIC-dir names ({README, migration-map, retrieval-eval}). + Applying it to the wings meant a genuine record at `records/docs/retrieval-eval.md` was + silently skipped — and that path is forced, not hypothetical: the path<->key rule puts key + `docs.retrieval-eval` at exactly that filename. Worse, dl.active_files() applies the filter + only to the TOPIC_DIR glob, so such a file IS a corpus source while being exempt from the + guard.""" + records, archive = _wing(tmp_path) + (records / "docs").mkdir(parents=True, exist_ok=True) + (records / "ci" / "good.md").write_text(_GOOD) + (records / "docs" / "retrieval-eval.md").write_text("# just a note, not a record\n") + faults = dv.record_wing_faults(records, archive) + assert len(faults) == 1 and "retrieval-eval.md" in faults[0], faults + + +def test_archive_toplevel_is_exempt_by_IDENTITY_not_by_location(tmp_path): + """Exempting everything directly in `archive/` left that one directory unguarded: a new + unparseable `archive/foo.md` would vanish silently. The exemption is for the five #610 stripped + INDEX files, so it must test for that shape — one keyless record with a known generated + heading — not merely for sitting in that directory.""" + records, archive = _wing(tmp_path) + (records / "ci" / "good.md").write_text(_GOOD) + (archive / "api.md").write_text("# api\n\n## Records formerly in this file\n\n- `api.thing`\n") + assert dv.record_wing_faults(records, archive) == [], "a real stripped index must stay exempt" + + (archive / "foo.md").write_text("rule: >-\n wrapped\n value\n") + faults = dv.record_wing_faults(records, archive) + assert len(faults) == 1 and "foo.md" in faults[0], faults + + +def test_junk_frontmatter_key_from_a_split_value_is_faulted(tmp_path): + """parse-to-WRONG, the case the structural check alone cannot see. A block scalar with an + UNINDENTED continuation containing a colon parses to ONE valid keyed record whose `rule` is + literally `>-`, plus a junk key from the continuation — silently truncating the real value. + PyYAML rejects this input, so the hand reader is more permissive than the writer.""" + records, archive = _wing(tmp_path) + bad = records / "ci" / "corrupt.md" + bad.write_text(_GOOD.replace("rule: 'a rule on one quoted line'\n", + "rule: >-\nthe real rule: with a colon\n")) + recs = dl.parse_file(bad) + assert len(recs) == 1 and recs[0].key, "precondition: this parses to one KEYED record" + assert recs[0].rule == ">-", f"precondition: the real value was truncated, got {recs[0].rule!r}" + + faults = dv.record_wing_faults(records, archive) + assert len(faults) == 1 and "unrecognized key" in faults[0], faults + + +def test_an_empty_or_missing_record_wing_is_LOUD(tmp_path): + """A guard that reports a clean corpus from a scan of nothing is the defect it exists to close, + applied to itself. Reachable via a partial checkout, a renamed directory, or a bad monkeypatch.""" + missing = dv.record_wing_faults(tmp_path / "nope" / "records", tmp_path / "nope" / "archive") + assert missing and "missing or contains no" in missing[0], missing + + records, archive = _wing(tmp_path) # exists but holds no *.md + empty = dv.record_wing_faults(records, archive) + assert empty and "missing or contains no" in empty[0], empty -- 2.47.3 From 3d720a6bc1d971d78a6b88831d97767ac09cdd98 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 26 Jul 2026 11:28:43 +0200 Subject: [PATCH 4/5] =?UTF-8?q?fix(621):=20exempt=20by=20exact=20path,=20n?= =?UTF-8?q?ot=20basename=20=E2=80=94=20my=20"no=20basename=20exemption"=20?= =?UTF-8?q?claim=20was=20false?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Re-review found the basename exemption was not actually removed: the code still dropped any wing-root README.md, and the docstring defended that with "no such file exists today; the catalog README lives in TOPIC_DIR". That is false — docs/decisions/archive/ README.md exists and parses to []. So the rule text in the record, the catalog row and ci-cd.md all asserted something the shipped code contradicted. It was also a live hole, not just wrong prose: dl.active_files() globs RECORDS_DIR.rglob with NO filter, so a future docs/decisions/records/README.md would be a corpus source while exempt from the guard — verbatim the hole this PR cites as the reason to stop using _NON_DECISION_FILES, reintroduced one directory over. Now exempt by exact RELATIVE PATH ({archive/README.md}), with a test pinning that the same basename in the ACTIVE wing is still faulted. Mutation-verified. DISAGREEING with one review finding, deliberately. It asked for the empty-wing anti-vacuity check to be symmetric across both wings. The semantics differ: zero ACTIVE records means the scan measured nothing and any clean result is vacuous, while zero ARCHIVED records is a normal state — nothing superseded yet, true of any young repo and every fresh clone before the first supersession. Making it symmetric fails a correct corpus (it reddened 7 tests). The asymmetry stays, with the reason in the code and a test pinning it so nobody "fixes" it back. 127 passed. Refs #621 --- docs/ci-cd.md | 5 +-- docs/decisions/README.md | 2 +- .../records/docs/record-wing-parse-guard.md | 2 +- scripts/decisions_validate.py | 19 +++++++++--- scripts/tests/test_decisions_validate.py | 31 +++++++++++++++++++ 5 files changed, 51 insertions(+), 8 deletions(-) diff --git a/docs/ci-cd.md b/docs/ci-cd.md index d74ecc7ba..5ad5f92c2 100644 --- a/docs/ci-cd.md +++ b/docs/ci-cd.md @@ -584,8 +584,9 @@ in the range (ersatztv#609), a structural per-path check that every `*.md` under record** (ersatztv#621 — without it, a file the dependency-free frontmatter reader cannot parse, such as one using a YAML block scalar, yields `[]` and vanishes from the corpus with every check still reporting green; a file directly in `archive/` is exempt only when it really is a stripped index — one keyless -record with a known generated heading — never merely by its location, and nothing is exempt by -basename), +record with a known generated heading — never merely by its location; the single further exemption, +`archive/README.md`, is by exact relative path, never by basename, which would otherwise exempt the +same filename in the active wing), and the generated active catalog (`docs/decisions/README.md`) in sync with source. Two steps: `scripts/decisions_validate.py --base origin/ --head HEAD` (the merge-base diff checks, which need a base/head range — CI-only) and `scripts/build_decisions_catalog.py --check` (catalog drift). diff --git a/docs/decisions/README.md b/docs/decisions/README.md index 27cfaaddf..f24dbcbd2 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -66,7 +66,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera | `docs.decision-lifecycle` | every decision `##` record (active or archived) carries a 5-field metadata block (`key`, `status`, `since`, `supersedes`, `superseded-by`) checked by `scripts/decisions_validate.py`; a record is never deleted or line-edited to reverse a call — it is moved to `docs/decisions/archive/` with `status: superseded`/`retired` and a reciprocal `superseded-by`/`supersedes` key pair to its replacement. | 2026-07-21 | [link](records/docs/decision-lifecycle.md) | | `docs.decision-one-file-per-record` | Each decision record is its own file at `docs/decisions/records//.md` (archived ones at `docs/decisions/archive//.md`) with YAML frontmatter; the filename IS the key, so one-active-record-per-key is a filesystem property rather than a validator check, and supersession is a `git mv`. | 2026-07-25 | [link](records/docs/decision-one-file-per-record.md) | | `docs.decision-optional-provenance` | Decision records gain two OPTIONAL fields — `stale-after: YYYY-MM-DD` on the metadata line and a `**Sources:**` line in the metadata block; the Open Knowledge Format (OKF) itself is NOT adopted as the record format. | 2026-07-25 | [link](records/docs/decision-optional-provenance.md) | -| `docs.record-wing-parse-guard` | `decisions_validate.py` asserts, per PATH, that every `*.md` under `docs/decisions/records/**` and `docs/decisions/archive/**` parses to exactly one record carrying a `key` — an ERROR, not a warning, since a file in the record wings that is not a record is a mistake by definition. A file sitting DIRECTLY in `archive/` is exempt only when it actually looks like a #610 stripped index — exactly one keyless record with a known generated heading — never merely by living there; nothing is exempt by BASENAME. `_read_frontmatter` is deliberately NOT extended to accept YAML block scalars: every record value goes on ONE line, and the structural check is what makes that limitation loud instead of silent. | 2026-07-26 | [link](records/docs/record-wing-parse-guard.md) | +| `docs.record-wing-parse-guard` | `decisions_validate.py` asserts, per PATH, that every `*.md` under `docs/decisions/records/**` and `docs/decisions/archive/**` parses to exactly one record carrying a `key` — an ERROR, not a warning, since a file in the record wings that is not a record is a mistake by definition. A file sitting DIRECTLY in `archive/` is exempt only when it actually looks like a #610 stripped index — exactly one keyless record with a known generated heading — never merely by living there. The one other exemption, `archive/README.md`, is by exact RELATIVE PATH; nothing is ever exempt by BASENAME, since that would exempt the same filename in the active wing too. `_read_frontmatter` is deliberately NOT extended to accept YAML block scalars: every record value goes on ONE line, and the structural check is what makes that limitation loud instead of silent. | 2026-07-26 | [link](records/docs/record-wing-parse-guard.md) | | `docs.tracker-comment-retrofit` | When the knowledge exporter flags an over-cap tracker issue and excludes it from ingestion, triage its comments instead of assuming a retrofit is owed — and for each decision-shaped item check the **worked issue first**, because a tracker session comment is by construction a précis of the fuller closing record posted on the issue it narrates. Applied to #237 (111 comments) this yielded **zero** records, so server-management#642's "a fact found only in a #237 comment" retrieval row has no valid subject and its interim target (an already-migrated record) is permanent. | 2026-07-21 | [link](records/docs/tracker-comment-retrofit.md) | | `ffmpeg.external-logo-graphics-engine` | External-URL channel logos pass through to the graphics engine like any other watermark source; `WatermarkSelector` must never gate them on `File.Exists` (always false for a URL) and never route them through the ffmpeg-native overlay shortcut. | 2026-07-20 | [link](records/ffmpeg/external-logo-graphics-engine.md) | | `ffmpeg.hls-cold-start-burst` | HLS cold-start latency is fixed with a bounded `-readrate_initial_burst` (gated on FFmpeg ≥6.1 capability detection), not by raising `work_ahead_limit`, which would remove the concurrency guarantee it exists for. | 2026-07-20 | [link](records/ffmpeg/hls-cold-start-burst.md) | diff --git a/docs/decisions/records/docs/record-wing-parse-guard.md b/docs/decisions/records/docs/record-wing-parse-guard.md index 14ed0de87..9fca79f5b 100644 --- a/docs/decisions/records/docs/record-wing-parse-guard.md +++ b/docs/decisions/records/docs/record-wing-parse-guard.md @@ -5,7 +5,7 @@ status: active since: '2026-07-26' supersedes: none superseded-by: none -rule: '`decisions_validate.py` asserts, per PATH, that every `*.md` under `docs/decisions/records/**` and `docs/decisions/archive/**` parses to exactly one record carrying a `key` — an ERROR, not a warning, since a file in the record wings that is not a record is a mistake by definition. A file sitting DIRECTLY in `archive/` is exempt only when it actually looks like a #610 stripped index — exactly one keyless record with a known generated heading — never merely by living there; nothing is exempt by BASENAME. `_read_frontmatter` is deliberately NOT extended to accept YAML block scalars: every record value goes on ONE line, and the structural check is what makes that limitation loud instead of silent.' +rule: '`decisions_validate.py` asserts, per PATH, that every `*.md` under `docs/decisions/records/**` and `docs/decisions/archive/**` parses to exactly one record carrying a `key` — an ERROR, not a warning, since a file in the record wings that is not a record is a mistake by definition. A file sitting DIRECTLY in `archive/` is exempt only when it actually looks like a #610 stripped index — exactly one keyless record with a known generated heading — never merely by living there. The one other exemption, `archive/README.md`, is by exact RELATIVE PATH; nothing is ever exempt by BASENAME, since that would exempt the same filename in the active wing too. `_read_frontmatter` is deliberately NOT extended to accept YAML block scalars: every record value goes on ONE line, and the structural check is what makes that limitation loud instead of silent.' signals: 'record silently invisible, parsed to 0 records, block scalar rule >- , unterminated frontmatter, file vanishes from corpus, validator OK but record absent, catalog up to date but missing, one keyed record per file, stripped legacy archive index exempt · paths: `scripts/decisions_validate.py`, `scripts/decisions_lib.py`, `scripts/tests/test_decisions_validate.py` · issues: #621, #610, #609, #603' mechanics: '`scripts/decisions_validate.py` -> `record_wing_files` / `record_wing_faults`; `docs/ci-cd.md` -> "`decisions-guard` job"' --- diff --git a/scripts/decisions_validate.py b/scripts/decisions_validate.py index 85790219e..5151a0928 100644 --- a/scripts/decisions_validate.py +++ b/scripts/decisions_validate.py @@ -419,22 +419,27 @@ def record_wing_files(records_dir: Path | None = None, archive_dir: Path | None `archive/`, which are scanned but may qualify for the stripped-index exemption applied in `record_wing_faults` (see there). Nothing is excluded by BASENAME. - Excluding by basename was a real hole: `_NON_DECISION_FILES` is `{README.md, migration-map.md, + Excluding by BASENAME was a real hole: `_NON_DECISION_FILES` is `{README.md, migration-map.md, retrieval-eval.md}` — three TOPIC-dir names — and applying it to the wings meant a genuine record at `records/docs/retrieval-eval.md` was silently skipped. That path is not hypothetical: the path<->key rule forces key `docs.retrieval-eval` to live at exactly that filename, and `docs/decisions/retrieval-eval.md` is a real unmigrated file, i.e. a plausible migration target. Worse, `dl.active_files()` applies that filter only to the TOPIC_DIR glob, not to `RECORDS_DIR.rglob` — so such a file IS a corpus source while being exempt from the guard. - Only a literal `README.md` sitting directly in a wing root is dropped here (no such file exists - today; the catalog README lives in TOPIC_DIR, one level up). + + So the exemption is by exact RELATIVE PATH, never by basename. The only entry is + `archive/README.md`, a hand-written directory README that really does exist — an earlier + version of this function excluded any wing-root `README.md` "since no such file exists today", + which was simply false and would additionally have exempted a future `records/README.md`, i.e. + reintroduced the very hole one directory over. """ records_dir = dl.RECORDS_DIR if records_dir is None else records_dir archive_dir = dl.ARCHIVE_DIR if archive_dir is None else archive_dir + exempt_paths = {archive_dir / "README.md"} files: list[Path] = [] for wing in (records_dir, archive_dir): if wing.exists(): - files += sorted(p for p in wing.rglob("*.md") if not (p.parent == wing and p.name == "README.md")) + files += sorted(p for p in wing.rglob("*.md") if p not in exempt_paths) return files @@ -490,6 +495,12 @@ def record_wing_faults(records_dir: Path | None = None, archive_dir: Path | None # A wing that is absent or empty must be LOUD, not silently clean. Otherwise a partial checkout, # a renamed directory, or a bad monkeypatch turns the whole guard into a no-op that reports # success — which is the defect this function exists to close, applied to itself. + # DELIBERATELY asymmetric: only the ACTIVE wing must be non-empty. A corpus with zero active + # records is definitionally broken (and means the scan is measuring nothing, so a clean result + # would be vacuous). An empty ARCHIVE is a perfectly normal state — it just means nothing has + # been superseded or retired yet, which is true of any young repo and of a fresh clone before + # the first supersession. Faulting on it would fail a correct corpus. Review asked for symmetry + # here; the semantics genuinely differ, so this stays asymmetric with the reason stated. if not records_dir.exists() or not any(records_dir.rglob("*.md")): faults.append( f"{records_dir}: the active record wing is missing or contains no *.md files — refusing " diff --git a/scripts/tests/test_decisions_validate.py b/scripts/tests/test_decisions_validate.py index 77b941b5f..663a818ce 100644 --- a/scripts/tests/test_decisions_validate.py +++ b/scripts/tests/test_decisions_validate.py @@ -907,3 +907,34 @@ def test_an_empty_or_missing_record_wing_is_LOUD(tmp_path): records, archive = _wing(tmp_path) # exists but holds no *.md empty = dv.record_wing_faults(records, archive) assert empty and "missing or contains no" in empty[0], empty + + +def test_a_wing_root_README_is_exempt_by_PATH_not_by_basename(tmp_path): + """`docs/decisions/archive/README.md` really exists (a hand-written directory README), so it + must be exempt — but by exact relative path, not by basename. + + An earlier version excluded ANY wing-root `README.md` on the stated grounds that no such file + existed. That was false, and it would additionally have exempted a future + `records/README.md` — reintroducing the basename hole one directory over, in the wing that + matters most.""" + records, archive = _wing(tmp_path) + (records / "ci" / "good.md").write_text(_GOOD) + (archive / "README.md").write_text("# archive\n\nHand-written directory README.\n") + assert dv.record_wing_faults(records, archive) == [], "archive/README.md must stay exempt" + + (records / "README.md").write_text("# not a record\n") + faults = dv.record_wing_faults(records, archive) + assert len(faults) == 1 and "README.md" in faults[0], ( + f"a README in the ACTIVE wing must NOT inherit the archive exemption: {faults}" + ) + + +def test_an_empty_ARCHIVE_wing_is_legitimate(tmp_path): + """Deliberate asymmetry with the active-wing check, pinned so nobody 'fixes' it into symmetry. + + Zero active records means the scan measured nothing and any clean result is vacuous. Zero + ARCHIVED records just means nothing has been superseded yet — normal for a young repo and for + every fresh clone before the first supersession.""" + records, archive = _wing(tmp_path) + (records / "ci" / "good.md").write_text(_GOOD) + assert dv.record_wing_faults(records, archive) == [], "an empty archive must not fault" -- 2.47.3 From 99497035851124ec26b783953f583588c4e3a3c9 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 26 Jul 2026 12:05:09 +0200 Subject: [PATCH 5/5] =?UTF-8?q?docs(621):=20state=20the=20mutation=20count?= =?UTF-8?q?=20precisely=20=E2=80=94=20it=20depends=20on=20which=20mutation?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The record said "10 tests go red"; on the rebased base a full short-circuit reddens 11. Both numbers are real and measure different mutations: neutering only the scan loop leaves the empty-wing check live (10), while returning [] from the whole function disables that too (11). The reviewer flagged the phrasing as ambiguous before the count also drifted. Now states the mutation alongside the number, since a bare count is exactly the kind of frozen figure this session has watched rot four times. Refs #621 Co-Authored-By: Claude Opus 5 (1M context) --- docs/decisions/records/docs/record-wing-parse-guard.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/docs/decisions/records/docs/record-wing-parse-guard.md b/docs/decisions/records/docs/record-wing-parse-guard.md index 9fca79f5b..288e38c22 100644 --- a/docs/decisions/records/docs/record-wing-parse-guard.md +++ b/docs/decisions/records/docs/record-wing-parse-guard.md @@ -48,7 +48,8 @@ hole rather than creating it. Teaching `_read_frontmatter` to fold block scalars single-**line**, not single-**quoted**: 117 `rule:` values are unquoted plain scalars, 51 are quoted.) There is no value a flat scalar cannot hold. -Verified by mutation, not by a green tick: with `record_wing_faults` neutered, **10** tests go red, +Verified by mutation, not by a green tick: with `record_wing_faults` short-circuited to return `[]`, +**11** tests go red (10 if only the scan loop is neutered, leaving the empty-wing check live), and separately, deleting the single line that WIRES it into `main()` reddens a dedicated test — that one line's removal previously left the whole suite green while a real record vanished, which is the guard's own failure mode applied to the guard. `test_real_repo_record_wings_are_all_parseable` additionally asserts the -- 2.47.3