18 Commits

Include renames

Author	SHA1	Message	Date
timothy	8b73234d78	docs(672): record that the fix closes the route, not the class (#697) ... Probing rather than reasoning turned up a second instance of the same vulnerability class while this fix was in review. `docker-build.yml` also triggers on `pull_request`, so it is head-resolved too, and it carries `ETV_STATUS_AUTH` (`REGISTRY_USER:REGISTRY_PASSWORD`) for the #420 revalidation read. Basic auth is not scoped: an account that can read commit statuses can write them. Confirmed with a scratch PR that POSTed a probe-named context using those credentials and succeeded — so a PR rewriting `docker-build.yml` can still post `review-verdict/h10=success` for its own head. That workflow cannot take the same fix. It builds and tests the PR's code, so it must resolve from the head; `pull_request_target` there would be the real footgun. It needs a read-only status identity instead. Filed as #697. The `rule:` field carries the limitation, not just the body, because the predecessor record's documented failure was exactly a reassuring sentence in the position a catalog reader stops at. Also trims the record to the 60-line prose ceiling. Adding it at 62 lines pushed p90 past the ceiling and reddened the blocking `script-tests` job — which is ersatztv#688 reproducing live, not a defect in this change. Refs: #672 Refs: #697 Decisions-Edit: yes	2026-07-28 21:41:26 +02:00
timothy	cf907f0988	fix(672): trigger the verdict gate on pull_request_target scoped to main ... `review-verdict.yml` writes `review-verdict/h10`, the status branch protection requires on `main`. It triggered on `pull_request`, and Gitea resolves a `pull_request` workflow DEFINITION from the PR's own head commit — so a PR editing this file ran its own rewritten copy and could post `h10=success` for itself. `PROTECTED` did not help (the rewrite defines it too), and neither did branch protection, which requires the context rather than an author and carries `required_approvals: 0`. The #649 base-ref checkout binds the SCRIPTS this job runs to the base. It cannot bind the job definition. Only the trigger can. Measured on this instance (Gitea 1.25.4) with three throwaway scratch PRs rather than inferred from GitHub, since the premise is a claim about someone else's software: A. A PR rewriting the workflow posted its own probe context and the real `review-verdict/h10` was never written — the base definition never ran. B. Under `pull_request_target` the identical rewrite was ignored: the BASE definition ran and posted `h10=pending`, on `opened` and `synchronize` alike, with `secrets` still available. C. With `branches: [main]`, a PR into a non-main base produced no run and no status at all. The probes only ever posted probe-named contexts, never a forged `h10`. `branches: [main]` is half the fix, not a refinement: base resolution means the BASE branch supplies the definition, so without it the rewrite simply moves to an attacker-pushed base — and a status forged there is inherited by any later real PR with the same head sha (#663). `pull_request_target` is safe here only because this job never checks out or executes head-supplied code; the base-ref checkout is what makes the trigger usable, so the two are one decision. Rejected `required_approvals: 1` as the cheaper fix: Gitea forbids approving your own PR and this is effectively a single-maintainer repo, so it would deadlock every PR rather than gate the dangerous ones. Three mutations confirm the new test discriminates rather than merely passing: reverting to `pull_request`, dropping the `branches` filter, and re-adding `pull_request` alongside the safe trigger each go red with a distinct message. It parses the YAML instead of substring-matching because `pull_request` is a prefix of `pull_request_target`. Refs: #672 Decisions-Edit: yes	2026-07-28 21:28:38 +02:00
timothy	c794a48462	docs(649): narrow the base-ref headline to what the checkout actually binds ... The record's bold sentence said the base-ref checkout means the workflow "cannot be rewritten by that same PR to weaken its own judgment". That is true of the SCRIPTS the job executes and false of the workflow itself: Gitea resolves a `pull_request` workflow definition from the PR's own head, so a PR editing `review-verdict.yml` runs its rewritten copy — which can delete the checkout outright, or just post `review-verdict/h10=success` and stop. Branch protection requires the context, not an author, and carries `required_approvals: 0` (#672). The scoping already existed further down, under "What is deliberately NOT claimed". That is not good enough for this particular sentence: it is bold, it is the paragraph a reader resolving this record from the catalog lands on, and someone who stops there leaves with the opposite of the truth. A caveat only works if it is reached. So the headline now says what the checkout binds (the scripts, from the already-reviewed base) and a following paragraph states the head-resolution hole directly, with the superseded claim quoted so a reader who remembers it can see it was retracted rather than wonder whether two records disagree. The later paragraph loses its duplicated opener and points at it instead. Docs only; no behaviour change. Same failure class the rest of #649 kept turning up — a claim stronger than the code — reached this time through prose rather than a test. Refs #649, #672 Decisions-Edit: yes	2026-07-27 18:30:44 +02:00
timothy	3885fd6aea	docs(649): narrow the enumeration's stated guarantees to what it actually proves ... Two limitations the cold review surfaced are now written where the guarantees are described, rather than living only in a review transcript. Both are pre-existing and tracked separately (#663, #664); neither is fixed here. - Head-sha binding detects ONE-WAY movement. An A->B->A force-push round trip restores the expected sha, so the binding holds while the pages came from two states. The record previously read as though the race were closed. - A commit status is repo-GLOBAL, so a success earned on one PR is inherited by any other PR with the same head. Same property that makes the per-sha binding work, read from the other end. Refs #649 Decisions-Edit: yes	2026-07-26 23:29:44 +02:00
timothy	9114a7e8af	fix(649): point the ENFORCED review-verdict gate at the shared PR-file enumeration ... #658 landed the shared implementation, `scripts/pr-changed-files.sh`, and rewired the ADVISORY hook onto it. The ENFORCED copy — the one that writes the branch-protection- required `review-verdict/h10` status — was left byte-identical to main, so its fail-closed behaviour on a malformed or empty response stayed INCIDENTAL: an empty `n` erroring `[ "$n" -lt 50 ]` to false. That is #649's second Done-when box, and the whole point of the issue was that the gate with real authority was weaker than the gate with none. `review-verdict.yml` now: - checks out the PR's BASE ref (`base.sha`, `persist-credentials: false`), never the head, so a PR cannot supply the code that judges it; - runs `scripts/jq-preflight.sh` in FLOOR-ONLY mode — `--expect` here would deadlock every merge on `main` the day the runner's jq changes; - calls `scripts/pr-changed-files.sh` and reads its EXIT STATUS, never its stdout on a failure path. The env trap flagged in review is handled: the script reads GITEA_BASE_URL and takes owner/repo as two separate arguments, so passing BASE_URL and a combined `owner/repo` would have silently fallen back to the hardcoded LAN default. The ~40 lines of inline enumeration are deleted, so the two copies can no longer drift. A base ref predating #658 has no such script; that posts `pending` with the reason rather than dying with no status at all. The drift guard is re-tightened from "the hook uses the shared script" to "BOTH callers do", and the workflow's own preconditions are pinned by parsing the YAML rather than substring-matching it — `head.sha` for `base.sha` is a nine-character diff. Verified by mutation, six mutants, each killed by its intended test: ignore the exit status; check out the head; drop `persist-credentials`; add `--expect`; re-inline a `pulls/N/files?` fetch; delete the PROTECTED clause. That last one initially MISSED, and the miss was the useful finding. The test used a docs-only-plus-protected file list and passed with the clause deleted, because PROTECTED (`.claude/ .gitea/ .husky/ scripts/ docker/ci/`) and DOCS_ONLY (`docs/`, root `*.md`) are disjoint — on the docs-only path that clause can never fire, and DOCS_ONLY was doing all the work. PROTECTED is load-bearing only on the BOT path, so the test now covers a Renovate PR editing the shared script, with a positive control proving the bot exemption fires at all. The caller contract is tested by EXECUTING the workflow's `run:` block against a stubbed enumeration that fails while emitting a perfectly docs-only list — the one combination the "every failure path also happens to print nothing" redundancy cannot absorb, and the exact mutation that survived the whole suite last round. Docs: both "Landing note" blocks removed, and the record's base-ref paragraph converted from a future-tense requirement to present-tense fact with its staging rationale kept as history. Refs #649 Decisions-Edit: yes	2026-07-26 23:05:14 +02:00
timothy	b255b7ffdc	test(648): close the mutation gaps round 5 found — two tests passed for the wrong reason ... Round 5 could not break the predicate itself: 28,930 real runs of the script across 14,465 crafted --version strings on bash 3.2.57 and 5.3.15 produced zero fail-opens, and `{1,9}` is honoured on bash 3.2, so round 4's bound is not void on the authoring Macs. What it did find is that two of round 4's changes were unpinned, and the tests that looked like they covered them did not. Reverting BOTH the first-line slice and `[[:blank:]]`→`[[:space:]]` together left the whole suite green. The four filler cases are all killed by the SEPARATOR restriction alone, so they attributed the fix to the wrong layer. Added three cases carrying the literal word `version` (`jq\nversion\n9.9` and friends), which satisfy the separator rule and can only be stopped by confining the parse to line one with a newline-free blank class. The CR-strip test was worse: vacuous through two independent mechanisms. `str.splitlines()` also splits on `\r`, so a per-line view dropped the stray CR; and `subprocess.run(text=True)` translates `\r` to `\n` outright, so even a raw-string check on stdout was unfalsifiable. The mutant demonstrably emits `... = jq-1.6<CR> (parsed 1.6; ...)` at the byte level while the test reported green. Added `run_bytes()` and a bytes comparison. Both gaps are now mutation-verified: reverting either change reddens exactly its own test. Also records the operational edge this parser acquires in the follow-up: it is strictly fail-closed by design, so once the floor mode gates the required check, a jq wrapper that prints a banner line would deadlock merges. The fix there is to widen the accepted forms, never to relax fail-closed. Decisions-Edit: yes	2026-07-26 22:21:07 +02:00
timothy	4e094637c6	fix(648): the version parser was fail-OPEN on a jq that cannot start ... Round 3, and it found that round 2's fix was a REGRESSION on the case that matters most. `raw=$(jq --version 2>&1 || true)` did two wrong things at once: folded stderr into the parse input and discarded the exit status. Combined with a pattern that matched the first <digits>.<digits> ANYWHERE, a jq broken by a glibc mismatch — which exits 127 and writes "version `GLIBC_2.34' not found" to stderr — parsed as version 2.34 and PASSED the floor. The strip-based parse this replaced failed CLOSED there. So the fix for a fail-open bug introduced a worse fail-open bug, in the one script whose entire purpose is to refuse to certify a version it did not parse. Same mechanism, second symptom: an unanchored match let a prefix outrank the real version. `2026.07.26 jq-1.6` parsed as 2026.07; a leading warning line carrying any number won too. Now: jq's exit status is captured explicitly (`$?` inside `if ! cmd` is the NEGATED status, so that needed care too), stderr is kept out of the parse, and the pattern is anchored to the leading `jq` token. Every legitimate form still parses — `jq-1.6`, `jq version 1.6`, `jq-1.7.1`, `jq-1.6-dirty`, `jq-1.6 (Debian 1.6-2.1)`, `jq-1.10` (numeric compare, so the two-digit minor is not read lexically). The tests could not have caught any of this: the shim always exited 0 and never wrote to stderr, so every case it could express was clean. It now takes stderr and an exit code, and the four new cases turn red under the exact mutation. Also: the drift guard now strips comment lines before matching. A future comment citing `pulls/$pr/files?limit=100` as an example of what not to do would otherwise have reddened script-tests — which, per this branch's own correction, blocks merges. And the record no longer over-corrects: the combined-status read is guarded by `if [ "$mwcs" != "true" ]`, so a red script-tests blocks the hook-mediated merge path, not literally every merge. Decisions-Edit: yes	2026-07-26 22:21:07 +02:00
timothy	5e7623b8d5	fix(648,649): security-review round 2 — close the version-parse hole and the untested caller contract ... Two real defects, and three docs claims that were simply wrong. jq-preflight.sh parsed the version by stripping around the first `-` and `.`, which assumed the format is exactly `jq-X.Y`. A build printing `jq version 1.6` left major empty; the sanity check concatenated major+minor into "6", which is non-empty and all-digits, so it PASSED. The floor comparison then ran `[ "" -lt 1 ]`, which errors — and `set -e` exempts a failing command in an `if` condition, so the conditional read false and the script exited 0 having asserted nothing, after printing a plausible "parsed" line. The silently-untested-axis failure this script exists to eliminate, reproduced inside the script itself. Now parsed by explicit regex, failing closed with a diagnosis when there is no <digits>.<digits> match. Also: `--expect` with no value exited 1 with empty output on both streams. The hook's exit-status check was pinned by nothing: mutating `if files=$(...)` into `files=$(...) || true; files_complete=yes` left the ENTIRE suite green. It survived only by redundancy — the script writes stdout once, right before exit 0, so failures also happen to yield empty stdout and `[ -n "$files" ]` catches it. Safe by accident, which is the exact criticism this branch levels at the old code. Four tests now pin it, with a stub that FAILS while emitting a docs-only list (the one case redundancy cannot absorb) plus a positive control proving the harness can see the difference. Verified: the mutation now turns exactly those tests red. Docs corrections. The record claimed the --expect pin was safe because script-tests is "advisory, not a required check" — false. The merge-consent hook reads the COMBINED status (ci.advisory-red-blocks-the-merge-gate, #598), so firing the tripwire blocks every non-docs-only merge until someone re-pins. Kept anyway, for a stated reason, but no longer described as free. The record also asserted in the present tense that review-verdict.yml checks out the base ref; it has no checkout step at all, so that is now a future-tense requirement on the follow-up. And the documented .status allow-list named GitHub's `removed`, which the code rejects. The drift-guard regex anchored on `?limit=`, so a re-inlined copy written `files?page=1&limit=50` would have walked past it. Decisions-Edit: yes	2026-07-26 22:21:07 +02:00
timothy	2c10f057b8	fix(648,649): stage the enforced-gate wiring behind the scripts it calls ... Splits the review-verdict.yml rewiring out of this PR. That workflow checks out the PR's BASE ref — deliberately, so a PR cannot rewrite the gate that judges it — and the base is main, which does not yet contain scripts/pr-changed-files.sh or scripts/jq-preflight.sh. Wiring it here would make the job exit 127 on its own PR and block the merge gate through the combined status, which reads red jobs as blocking. So this PR lands the scripts, their tests, the hook rewiring and the script-tests jq pin; the follow-up points review-verdict.yml at them once they exist on main. The two tests that asserted on review-verdict.yml are scoped accordingly, each carrying the reason. test_review_verdict_never_pins_a_jq_version is asserted NOW rather than in the follow-up, so the no-pin constraint on the required check is already enforced when the wiring lands. Decisions-Edit: yes	2026-07-26 22:21:07 +02:00
timothy	63fa81fbb5	docs(648,649): the jq contract + the shared PR-file enumeration record ... Adds docs/ci-cd.md "The jq contract" (1.6 floor, the three divergent constructs, and the deliberate pin-vs-floor asymmetry with its merge-deadlock reason), plus two decision records: ci.jq-version-contract and ci.shared-pr-file-enumeration. ci.script-tests-job stops restating the three jq rules and points at the new record. Also corrects the script-tests preflight description: it is now two steps (git presence, then jq VERSION via scripts/jq-preflight.sh --expect 1.6), not one. A literal NUL byte had crept into the ci-cd.md paragraph describing jq 1.6's NUL truncation — which git treats as a binary file. Replaced with the literal text. Decisions-Edit: yes	2026-07-26 22:21:07 +02:00
timothyandClaude Opus 5	bb1809fbf0	fix(647): the jq-1.6 fail-open was ALSO in review-verdict.yml — the enforced gate ... Round-4 review caught the miss that matters most: #643 fixed the `jq -e`-on-empty fail-open in .claude/hooks/pretooluse-merge-consent.sh, but the SAME construct sits in .gitea/workflows/review-verdict.yml — and that is the copy that runs on the CI runner, where jq is 1.6, and that feeds the branch-protection-required review-verdict/h10 status. Reproduced: `printf "" | jq -e '.statuses | type == "array"'` exits 4 on jq 1.8.2 (guard fires, correct) and 0 on jq 1.6 (guard passes). So on a transient API error `statusjson` is empty, the guard lets it through, `existing` reads "", and the job posts `pending` — or for a bot/docs-only PR an exemption `success` — over a possibly-existing human verdict. That is precisely what the comment three lines above it says must never happen. The hook version was harmless in practice because it runs on a dev Mac with jq 1.8. This one is live. Fixed identically, with a comment naming why the sibling fix missed it, and the same hardening applied to the changed-files read in the same workflow. Also from round 4: - LOW, reproduced — an ARRAY-valued `.status` dodged the closed allow-list. `index` is polymorphic: with an array argument it does SUBSEQUENCE matching, so `[...,"renamed",...] | index(["renamed"])` is truthy while `.status == "renamed"` is false — the row passed the allow-list AND skipped the `previous_filename REQUIRED` clause. The same `git mv code -> docs/` dodge the closed set exists to block, one type away. Now requires `.status` to be a string first; mutation-verified. - The record now carries all THREE jq rules rather than the one, and notes that the durable fix is pinning/preflighting the runner's jq version rather than patching constructs one at a time (tracked on #647). 200 tests pass under BOTH jq 1.8.2 and jq 1.6. Refs #647, #643, #631 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-07-26 13:24:20 +02:00
timothyandClaude Opus 5	f4473926d4	fix(643): close the status-string dodge, pin the allow-list anchors, fix two doc claims ... Round-3 review (MERGEABLE, all findings Low/Nit) — applied anyway, because each is the over-claim or unpinned-anchor family this PR keeps hitting. LOW — the `renamed => previous_filename REQUIRED` clause matched `.status` by exact lowercase string, so any other value took the `else true` branch: a row with `"Renamed"`, or with no status at all, validated fine and silently dropped its SOURCE path, letting `git mv ErsatzTV/Program.cs -> docs/a.md` read as docs-only. `.status` is now checked against a closed set and an unknown value fails closed. Two things that fix taught me, both caught by my own positive control rather than by review: 1. The first predicate was WRONG in a way that gated everything: inside `[...] | index(.status)`, jq's `.` is the ARRAY, so `.status` was null and every row failed. `$s` is now bound from the row before the context switches. A security check that rejects everything looks identical to a working one from the failing side — only test_gitea_real_status_values_are_accepted caught it. 2. The set includes BOTH `changed` and `modified`. Live Gitea 1.25.4 emits `changed`, but a closed allow-list built from the wrong vocabulary is worse than the hole it closes: it would gate every genuine docs-only PR. The property wanted is "reject what we don't recognise", not "enumerate one version exactly". LOW — three allow-list anchors had no test at all: dropping `^` from the `docs/` alternative (`ErsatzTV/docs/Program.cs` would exempt), dropping `$` from `.md` (`x.md.cs` would exempt), and dropping the non-empty-list guard. Since the round-3 `..` finding WAS an anchor subversion, they are now pinned; all three mutation-verified. NIT — docs/ci-cd.md called this job "a checkout plus a pure-stdlib pytest run", which the same file contradicts 450 lines later and which this PR's own record names as the bug that turned the job red on its first CI run. Also replaced a frozen "111 tests" with an explicitly indicative figure — the suite is ~190 after rebasing onto main, and a frozen count has rotted four times this session. 198 passed under both jq 1.8.2 and jq 1.6. Refs #643, #631 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-07-26 13:24:20 +02:00
timothy	54c875414c	fix(643): validate every field the extraction consumes; fix a vacuous test ... Re-review of the previous fix commit found it incomplete and its test vacuous. Both reproduced before fixing. MEDIUM — the validation domain did not match the CONSUMPTION domain. `chunk` emits `(.previous_filename // empty)` for EVERY row regardless of status, but the guard validated that field only when `.status == "renamed"`. So a row marked `modified` — or Gitea's distinct `copied` — carrying a newline in previous_filename was still exempted. Verified EXEMPT for both statuses against the previous commit. Now validated whenever present, with the `renamed` => REQUIRED clause kept on top. MEDIUM — test_newline_in_previous_filename_is_also_rejected asserted the right outcome without exercising the mechanism: its payload's second segment was `ErsatzTV/Program.cs`, which the allow-list rejects on its own, so the test passed with the newline guard entirely removed. That is why the hole above went unnoticed — the same filter-hides-the-defect trap the guard itself is about. Payload changed to a segment that PASSES the allow-list, so the test now discriminates, plus parametrized regressions for modified/copied/added. LOW — `..` components rejected. The allow-list anchors `^docs/`, so `docs/../ErsatzTV/Program.cs` matched it (reproduced). Git will not produce such a path, but this guard exists to fail closed on unexpected 2xx shapes. Added a positive control (a legitimate docs->docs rename still exempts) so the tightened row schema cannot be satisfied by never exempting anything. Severity calibrated in the record: the docs-only exemption ends in `decide allow ""`, a passthrough to the normal permission prompt, NOT an auto-grant. Every bypass here downgrades a mechanical deny/ask to a human prompt; none can silently self-merge. Real, worth fixing, but not what an earlier framing of #643 implied. All mutation-verified; 123 passed under BOTH jq 1.8.2 and jq 1.6. Refs #643, #631	2026-07-26 13:24:20 +02:00
timothy	c046add10a	fix(643): close two more fail-opens in the docs-only enumeration, found by cold review ... An independent cross-family review of the jq-1.6 fix found two further ways the docs-only exemption can fire over an incomplete file list — both reachable with NO transport error, so neither had anything to do with the original bug. 1. HIGH — a path containing a newline. `chunk` flattens paths into newline-delimited text before the allow-list grep, so a filename of "safe.md\ndocs/Program.cs" splits into two lines that BOTH match the allow-list, while the real single path ends in .cs. Git permits newlines in filenames and the reviewer reproduced the bypass against this hook. Now rejected outright at the row-schema guard, on both `filename` and `previous_filename`: no docs path contains a control character, so failing closed costs nothing. 2. HIGH — a short page read as the last page. `n < 50` assumed the server's page size is the 50 we requested, but Gitea caps `limit` at the server-wide MAX_RESPONSE_ITEMS (default 50, configurable) and may return fewer. A 30-row docs page followed by a page of code completed the enumeration over a PARTIAL list. Only a validated EMPTY page may now terminate it; the page<=40 cap still fails closed, and the cost is one extra request. 3. MEDIUM — the enumeration was not bound to one head. Paging is several round-trips, so a force-push between them assembles a list belonging to no single commit: page 1 from head A plus a short docs tail from head B, with B's code page never read. The head sha is re-read after enumeration and the exemption refused if it moved. All three mutation-verified: reverting each fix reddens exactly its own test and nothing else. A positive control (short page then empty page) pins that the stricter terminator still exempts a genuinely docs-only PR, so "never terminate early" cannot be satisfied by never exempting anything. 118 passed under BOTH jq 1.8.2 and jq 1.6. The record now states the generalisable lesson: every defect here was an exhaustiveness failure in an enumeration whose completeness is load-bearing. When a security decision depends on having seen ALL of something, the termination condition must be positive and explicit, never inferred from a proxy. Refs #643, #631	2026-07-26 13:24:20 +02:00
timothy	5f068a2488	fix(631): close a fail-open in the merge-consent gate that only CI could see ... Running scripts/tests in CI for the first time turned up a real defect in the gate itself, not just in the harness. `jq -e` over EMPTY input exits 4 on jq >= 1.7 but 0 on jq 1.6 — verified against both binaries, not inferred. The docs-only pagination guard in pretooluse-merge-consent.sh leaned on that exit status to reject a transport failure. On jq 1.6, which the CI runner ships: page 2 errors -> gq returns empty -> jq guard wrongly PASSES -> n is empty so [ "$n" -lt 50 ] errors into false -> the loop walks PAST the failed page -> page 3 legitimately returns [] -> files_complete=yes over a PARTIAL list -> the docs-only exemption fires over unread pages that may be pure code. That is the very defect the guard's own comment describes, reintroduced one layer down by a jq version difference. Fixed by rejecting an empty body explicitly rather than inferring it from jq's exit status. The same hardening is applied to the review-verdict status read, which fell through to `vstate=""` -> deny (fail-CLOSED, so never a hole) but would have surfaced the wrong message. Why it survived: the existing transport-failure test asserts the right thing but can only observe the bug where jq is 1.6, so it passes on a developer Mac with the bug fully present — and the suite had never run anywhere else. The new test removes that dependency by shimming ONLY jq 1.6's empty-input exit status, so it pins the property on any host. Mutation-verified: revert the fix and it goes red on jq 1.8.2. The shim is deliberately narrow (no `-n`): a broader first version swallowed the `jq -n` calls `decide` uses to build its JSON, so the hook emitted nothing and every decision read as a passthrough — the verifier manufacturing the exemption it was meant to disprove. test_jq16_shim_actually_reproduces_the_quirk now pins the shim itself, `-n` case included. Suite: 113 passed under jq 1.8.2 AND under jq 1.6. Refs #631	2026-07-26 13:24:20 +02:00
timothy	73577f484f	ci(631): install pyyaml too — the suite is not pure-stdlib, and CI proved it ... First CI run went red on `ModuleNotFoundError: No module named 'yaml'` at collection: test_migration_equivalence.py imports scripts/migrate_decisions_split.py, which uses PyYAML by design. It passed locally only because this machine already had PyYAML. The original claim came from reading decisions_lib/decisions_validate/build_catalog and the test files themselves — which is precisely the set that does NOT import yaml. Replaced with an AST import scan over all of scripts/, which gives the complete third-party set: pytest and yaml. (etv_client is imported only by scripts/scripted-schedules/entrypoint.py, a container entrypoint pytest never collects.) This does not contradict the dependency-free decisions READ path: decisions_lib._read_frontmatter is hand-written exactly so validation runs where nothing is installed. The one-shot WRITE path is allowed PyYAML, and its test needs it. Verified the way it should have been the first time: a clean venv with pytest alone reproduces CI's collection error; adding pyyaml gives 111 passed. Comments, ci-cd.md and the decision record all corrected to drop the "pure stdlib" claim. Refs #631	2026-07-26 13:24:19 +02:00
timothy	69d8d3ccfe	ci(631): run scripts/tests in CI as its own job, not inside the flake-covered decisions-guard ... Nothing executed scripts/tests/. No workflow and no Husky hook invoked pytest: decisions-guard runs decisions_validate.py and build_decisions_catalog.py directly (the code, never its tests) and the `test` job is dotnet test only. The 111 tests guarding the decision corpus, the #610 migration-equivalence harness, the merge-consent exemption logic and the #622 review-verdict poster were enforced only by whoever remembered to run pytest locally. Adds a `script-tests` job to pr-checks.yml rather than the step inside decisions-guard the issue proposed. decisions-guard is covered by ci.decisions-lifecycle-flake, a standing instruction that a lone `decisions lifecycle` red is a known flake and must not be investigated — so a genuine pytest regression there would surface as exactly the red every session is told to wave through. A gate inherits the credibility of the job it lives in. Runs unconditionally: the suite executes the real post-review-verdict.sh and pretooluse-merge-consent.sh, so a `scripts/**` path filter would miss a .claude/hooks/** edit. ~10s, pure stdlib + pytest. Negative control verified locally (broken test -> exit 1); CI-red measurement follows on a scratch PR per the Done-when. Refs #631	2026-07-26 13:24:19 +02:00
timothy	fba5233caf	feat(610): split the decision corpus into one YAML-frontmatter file per record ... 168 records -> docs/decisions/records/<area>/<topic>.md (163 active, 23 dirs) and docs/decisions/archive/<area>/<topic>.md (5 archived). The filename IS the key, so one-active-record-per-key becomes a filesystem property rather than a validator check, and supersession becomes a `git mv`. WHY: the monolith was a concurrency problem before an aesthetic one. A 3,900-line append target made parallel sessions collide -- PR #605 and PR #614 both hit append-vs-append conflicts during routine rebases, and hand-resolving those inside the corpus is exactly the operation the rationale-rewrite guard exists to police. HOW IT IS VERIFIED: a ~170-file diff cannot be meaningfully read, so correctness does not rest on reading it. The parser was taught BOTH formats first, so the body-diff guard parses the old form at the merge-base and the new form at head -- the migration validates itself, no bypass. The proof is a field-level equivalence harness: 168 records before and after, zero lost, zero gained, zero field mismatches, zero rationale bodies differing. Reviewers should scrutinise the harness; it is the actual evidence. What measuring caught that reading would not have: - ~500 lines sit OUTSIDE any record -- decisions.md's lifecycle schema and each topic file's preamble, mostly the only copy. Source files are kept and stripped, never deleted. They also cannot be filed per-area: topic files hold several areas and 4 of 23 areas span several files. - Archive discovery was a non-recursive glob; after the split it found ZERO archived records, surfacing as four bogus "supersedes points to unknown key" errors rather than an obvious failure. - ~32 live docs point into the corpus BY DATE, which the split dangles. Each stripped file now ends with a generated "Records formerly in this file" index, which also rescues the identical breadcrumbs in old issue comments. - decisions.md's "In this file:" list was 97 same-file anchor bullets that the split makes WRONG, not merely stale. Dropped; the generated index replaces them with links that resolve. The equivalence harness now runs against a checked-in FIXTURE, not the live corpus. The earlier version migrated the real tree, which made it a one-shot: the moment the migration landed there was nothing left to move and the tests failed for reasons unrelated to the code. A fixture keeps them testing the SCRIPT rather than the repo's current state. Keys preserved verbatim, warts included: `sched` (12) and `scheduling` (1) remain two directories for one concept. Renaming a key is not a move -- it changes identity, breaks the equivalence proof, and invalidates MemPalace's per-key drawers. Taxonomy normalisation is separate work. refs #610	2026-07-25 19:45:09 +02:00