Skip to content

feat(registry): report every validation finding in one run - #20

Open
jdjioe5-cpu wants to merge 2 commits into
Kinlock-Org:mainfrom
jdjioe5-cpu:feat/aggregate-validation-findings
Open

jdjioe5-cpu wants to merge 2 commits into
Kinlock-Org:mainfrom
jdjioe5-cpu:feat/aggregate-validation-findings

Conversation

@jdjioe5-cpu

Copy link
Copy Markdown

What and why

Closes #16.
Implements multi-violation finding aggregation for the registry validator per roadmap row M1-41. Instead of stopping at the first failure, validateRegistry checks all rules on every file, producing structured findings with stable rule IDs, messages, and actionable fix hints sorted deterministically by file then rule ID. Unparseable JSON is reported distinctly and stops further per-file checks.

Hard-rule and doc review:

  • docs/ARCHITECTURE_ESSENTIALS.md §8 (Registry rules)
  • AGENTS.md §3 (Hard rules 10: Minimum viable; 11: Country-agnostic core)
  • AGENTS.md §8.4 (kinlock-registry: public fields only, ISO codes, attester authority scope, canonical formatting, global slug uniqueness)

Changes

  • scripts/registry.ts:
    • Added Finding (file, ruleId, field, message, fixHint) and ValidationResult interfaces.
    • Refactored validateRegistry to aggregate findings without exiting early on schema errors, continuing to check slug, directory, ISO country/currency, attester authorization, and duplicate slug.
    • Sorted findings deterministically (file ascending, then ruleId ascending).
    • Preserved backward compatibility by providing errors string array with existing pattern format alongside findings.
    • Exported formatFinding helper.
  • scripts/validate.ts:
    • Uses formatFinding to output structured findings with rule ID and actionable fix hints.
    • Exits with code 1 if findings exist, 0 otherwise.
  • scripts/registry.test.ts:
    • All 16 existing tests pass unedited.
    • Added new tests for multi-violation aggregation (3 simultaneous violations in 1 record), distinct unparseable JSON reporting, and deterministic sorting.
  • scripts/validate.test.ts:
    • Added unit tests for formatFinding formatting and fix hints.
  • README.md:
    • Documented the full table of stable rule IDs, what each enforces, and fix hints.
  • ROADMAP.md:
    • Marked M1-41 as DONE, added Changelog entry, updated Last updated.

Tests

Commands executed locally:

pnpm lint
> biome check .
Checked 13 files in 19ms. No fixes applied.

pnpm typecheck
> tsc -p tsconfig.json

pnpm test
> vitest run
Test Files  2 passed (2)
Tests  23 passed (23)

pnpm validate
> tsx scripts/validate.ts
3 payee file(s) checked, 0 problem(s)

pnpm hash fixtures/payees/ke/ke-kinlock-test-school.json
payee_id  4485665bae55d2562f20ad82cec50c8aaa99ddf613b982509711a6b474ded0ba
meta_hash 4c62af9d86c73d0a0f3733c76741ae1d1206e439daf7d1ee7fa0d4f167b8ed24

Docs / ADR

  • Updated README.md with rule IDs and descriptions.

Roadmap

  • M1-41: TODO -> DONE
  • Changelog entry added: yes

Assumptions and open questions

None. Output formatting is deterministic and diffable across platforms.

Resolves the ROADMAP.md conflict structurally: rebuilt on main, kept every
main row, kept both sides' Changelog lines, and let only this repo's own rows
advance from the branch (AGENTS.md 12). Section 3 counts recomputed.
@abrcrmb

abrcrmb commented Oct 29, 2026

Copy link
Copy Markdown
Collaborator

Maintainer push to this branch — exactly what changed.

This PR conflicted with main (the conflict was in ROADMAP.md only). Instead of asking you to rebase, I merged main into feat/aggregate-validation-findings so the PR is mergeable as-is.

My commit: 65e5cce — merges e0da124 (main at the time) into your branch.

Measured scope: I compared my merge's tree against what a plain automatic merge of the same two commits produces. Files where my version differs from both inputs:

  • ROADMAP.md

In ROADMAP.md that means: your Changelog row and your **Last updated** line were kept, main's rows and Changelog lines were kept, and the conflict markers were removed (AGENTS.md §12: keep both sides' row changes, never delete a row). Where scripts/roadmap-progress re-derived the phase-count table after the merge, the count lines moved too — that is the tool's output, not a status decision by me.

Current CI: red on validate — measured, and stated below.

No code, test, or config file of yours was modified unless it is listed above. This branch is yours; if you would rather resolve conflicts yourself, say so and I will stop pushing here.

Why validate is red. pnpm test fails at scripts/registry.errors.test.ts:72 — a test that exists on main and passes there (main's validate run at e0da124 is green). The test "stops before scanning payees when the country list is unusable" feeds a supported-countries.json whose countries is the string "KE" and asserts that no per-payee error is emitted; with the new all-findings aggregation, the run also reports payees/ke/ke-test-school.json: KE is not in …/supported-countries.json. So your correct-looking behavior change collides with a deliberate fail-fast on dependency-level config files. That is an invariant decision, not a formatting problem — I am not going to edit the test to make it pass, and neither should it be weakened. Details and options in a separate comment on this PR.

@abrcrmb

abrcrmb commented Oct 29, 2026

Copy link
Copy Markdown
Collaborator

validate is red because your behavior change collides with an invariant that main deliberately guards — so this is a decision, not a formatting problem. I am not going to edit the assertion to make the run pass.

The facts I checked

  • main is green: the validate run at e0da124 (this PR's merge base) passes.
  • The failing test predates your PR: scripts/registry.errors.test.ts, and it is not in your diff.
  • The assertion that fails is line 72, inside "stops before scanning payees when the country list is unusable":
json("supported-countries.json", { countries: "KE" });   // wrong type: string, not array
const report = errors().join("\n");
expect(report).toMatch(/supported-countries\.json: /);   // still passes
expect(report).not.toMatch(/ke-test-school\.json/);      // now fails
  • What CI actually got, from the job log:
<root>/payees/ke/ke-test-school.json: KE is not in /tmp/kinlock-registry-errors-QNXQa6/supported-countries.json
<root>/supported-countries.json: data/countries must be array

So with per-file aggregation, one unusable country list produces one message per payee file that references it. The test exists precisely to stop that cascade.

Two coherent ways out

A — aggregate findings, but keep fail-fast on dependency-level config. supported-countries.json and attesters/*.json are inputs every payee is validated against, not peer files. Rule becomes: aggregate across payee records, stop before the payee scan when a file the payees depend on is itself invalid. The existing test stays as written, M1-41 still lands for the case that matters day to day (many independent problems in different payee files reported in one run), and CI output on a broken country list stays one line instead of N.

B — aggregate unconditionally and change the invariant. Then the test has to change deliberately, with a rewritten assertion describing the new expected output plus an ADR in docs/adr/ recording why cascading config errors into every payee is acceptable. Per AGENTS.md §11 that is a reviewed doc change, and per §2 a reviewer/human signs off — it should not ride in as a test edit inside this PR.

My recommendation is A: it delivers the row's stated value (no more one-failure-at-a-time whack-a-mole across payee files) without degrading the single most confusing failure mode, and it needs no invariant discussion.

Which one the org wants is Allison's call as maintainer. Until she answers, leave the assertion alone rather than skipping or relaxing it — a red validate here is the check doing its job.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Report every validation failure in one run instead of stopping at the first

4 participants