timothyandClaude Opus 5
2ff52d4236
Review round 6: one MERGEABLE with non-blocking prose, one NOT-MERGEABLE with a real test defect. Both addressed. THE n PIN DID NOT PIN ANYTHING. `assert n == 10` was checked against a fixture holding exactly ten records, so a mutation returning a constant 10 for EVERY input satisfied it — while changing the live denominator from 183 to 10, which is precisely the production defect the test was added to close. A single hardcoded count cannot tell "counts the input" from "returns this number". Now a dynamic oracle at two distinct cardinalities; verified the constant-n mutation fails it. "MOVED p90 by 21 lines" asserted a history I had not measured. 21 is TODAY's gap (60 -> 81). The actual #672 event was smaller — at that tree p90 was 60 with the next value 83, so the 62-line record moved p90 to 62 and reddened CI with a 2-line move. The capability claim is what matters and is true at both refs; the past tense was not. Changed to "can move" in the two places that asserted it, which also makes all four sites agree with docs/ci-cd.md and the validator docstring, both of which already said "could". A REVIEW FINDING I REJECTED, having measured it. Round 6 called "p90 52" wrong for the #620-era distribution, measuring 57. That measurement is at `fefd11dff`; the record cites `f394d6ce`, and at THAT sha p90 is exactly 52 (n=167, min 2, median 26). The number is correct as written and is unchanged. Recording the disagreement rather than silently keeping it: the reviewer measured a different tree than the one the claim names. Also corrected in this branch's own commit message trail: `b24c51ab5` said origin/main has three 59-line records; it has four 59s and two 60s (HEAD: four and three). The claim that survives, and the only one the code and docs now make, is that NOTHING sits between 61 and 80 at either ref — verified independently at both. Cosmetics from the same round: a dangling modifier in ceiling_calibration's docstring, a test_decisions_lib assertion message that said "field(s) differ" when faults can now also be rejections, and a sentence in corpus-size-signal that named the replacement test without saying what it asserts. Verification: 431 scripts/tests pass; ruff at baseline parity (47); validator exits 0 with no drift notice; corpus at p90=60, 18/183, calibrated. Decisions-Edit: yes Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
91 lines
3.6 KiB
Python
91 lines
3.6 KiB
Python
from pathlib import Path
|
|
|
|
import pytest
|
|
|
|
import scripts.decisions_lib as dl
|
|
|
|
FIX = Path(__file__).parent / "fixtures" / "sample_decisions.md"
|
|
|
|
|
|
def test_parses_migrated_record():
|
|
recs = dl.parse_file(FIX)
|
|
migrated = [r for r in recs if r.key == "ci.runner-placement"]
|
|
assert len(migrated) == 1
|
|
r = migrated[0]
|
|
assert r.status == "active"
|
|
assert r.since == "2026-07-17"
|
|
assert r.supersedes == "none"
|
|
assert r.superseded_by == "none"
|
|
assert r.rule == "Every CI services container gets an explicit cap."
|
|
assert r.signals is not None
|
|
assert "issues: #390 #406" in r.signals
|
|
assert r.mechanics is not None
|
|
assert r.mechanics.startswith("docs/ci-cd.md")
|
|
|
|
|
|
def test_legacy_record_is_unmigrated():
|
|
recs = dl.parse_file(FIX)
|
|
legacy = [r for r in recs if r.heading.endswith("(#231)")]
|
|
assert len(legacy) == 1
|
|
assert legacy[0].key is None
|
|
assert legacy[0].status == "legacy-unmigrated"
|
|
|
|
|
|
def test_index_section_parses_as_heading():
|
|
recs = dl.parse_file(FIX)
|
|
assert any(r.heading == "Index" for r in recs)
|
|
|
|
|
|
def test_parses_optional_stale_after_and_sources():
|
|
recs = dl.parse_file(FIX)
|
|
r = next(r for r in recs if r.key == "ci.peak-anon-measurement")
|
|
assert r.stale_after == "2027-01-15"
|
|
assert r.sources is not None
|
|
assert "gitea run 4471" in r.sources
|
|
|
|
|
|
def test_optional_fields_default_to_none_when_absent():
|
|
recs = dl.parse_file(FIX)
|
|
r = next(r for r in recs if r.key == "ci.runner-placement")
|
|
assert r.stale_after is None
|
|
assert r.sources is None
|
|
|
|
|
|
def test_empty_stale_after_parses_as_empty_string_not_none():
|
|
"""Absent vs present-but-empty must stay distinguishable for the validator."""
|
|
text = (
|
|
"## H\n"
|
|
"`key: a.b` · `status: active` · `since: 2026-01-01` · `stale-after:` "
|
|
"· `supersedes: none` · `superseded-by: none`\n"
|
|
"**Rule:** r\n"
|
|
)
|
|
r = dl.parse_text(text, Path("fake.md"))[0]
|
|
assert r.stale_after == ""
|
|
|
|
|
|
def test_frontmatter_reader_matches_pyyaml_on_every_real_record():
|
|
"""The dependency-free reader must agree with PyYAML on the whole real corpus.
|
|
|
|
The read path cannot import PyYAML — it runs in CI's `decisions lifecycle` job, the Husky
|
|
pre-commit hook, and on every contributor's machine, none of which install it. (Requiring it
|
|
made the validator crash with ModuleNotFoundError once the corpus was migrated.) A hand parser
|
|
is only safe if it provably matches the library that WROTE the files, so this compares the two
|
|
across every record rather than on a sample.
|
|
|
|
Since #674 the comparison itself lives in `decisions_validate.pyyaml_frontmatter_faults`, which
|
|
the VALIDATOR now runs too — before that it existed only here, so `decisions_validate.py`
|
|
happily reported OK on a record PyYAML rejects. This test delegates to that one implementation
|
|
rather than keeping a second copy of the comparison, so the suite and the validator cannot
|
|
drift apart and agree on what "matches PyYAML" means.
|
|
"""
|
|
pytest.importorskip("yaml")
|
|
import scripts.decisions_validate as dv
|
|
|
|
files = [p for p in dl.RECORDS_DIR.rglob("*.md")] + [p for p in dl.ARCHIVE_DIR.rglob("*.md")]
|
|
files = [f for f in files if dl.has_frontmatter(f.read_text(encoding="utf-8"))]
|
|
assert len(files) > 100, f"only {len(files)} frontmatter files found — test would be near-vacuous"
|
|
|
|
faults, ran = dv.pyyaml_frontmatter_faults(files)
|
|
assert ran, "PyYAML is importable here, so the comparison must have actually run"
|
|
assert not faults, f"{len(faults)} frontmatter fault(s) vs PyYAML:\n" + "\n".join(faults[:5])
|