explain: gate the value census, don't just make it re-derivable (#260) - #267
Conversation
The census figures live in three places — the census itself, the `corpus` block of `test/fixtures/explain/values.json`, and prose in `commands/explain/README.md`. Committing the census script (#242) made them re-derivable; nothing checked that they still matched. #256 changed the value emission, regenerated the fixture and the unit assertions, and shipped with the README prose still quoting the pre-change numbers — three review comments found that, not CI. Each link in corpus -> fixture -> README now has its own gate, separated by what data it needs: - `explain:value-census:check` re-runs the census and asserts it figure by figure against the committed fixture. It needs `corpus.sqlite`, so it runs in ci.yaml's `corpus` job, which #186 gave `corpus:fetch` reachability for precisely so this could exist. It runs after the summary step, so a drifted run still publishes its figures — those are what say whether the drift is an intended emission change or a regression. - `explain:value-census:readme:check` asserts the README paragraph against that same fixture block. The paragraph is now a generated projection of it (`explain:value-census:readme`), so the drift that actually happened is caught with no corpus at all: in `lint:ci`, and in `bun test`. The fixture is the hinge — the one copy a human writes — and both gates point at it rather than at each other. The generated prose reports the invariants as counters rather than asserting they hold ("read 0, 0 and 0, and each must stay 0"), so every sentence stays true at any value and regenerating after a regression states it instead of publishing a reassuring lie. A unit test proves that by rendering a synthetic regressed census. Verified end to end against the real corpus: `explain:value-census --json` still reproduces the committed fixture byte for byte, and both gates were confirmed to FAIL on an injected one-figure drift before being wired in. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe value-census script now validates corpus output against a committed fixture and synchronizes generated README content. Package scripts, CI, documentation, and offline unit tests enforce both drift checks. ChangesValue census drift gates
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Corpus as corpus.sqlite
participant Census as explain-value-census.ts
participant Fixture as values.json
participant README as README.md
participant CI as CI workflow
Corpus->>Census: Generate fresh value census
Census->>Fixture: Compare census values
Census->>README: Render or check generated block
CI->>Census: Run fixture validation
CI->>Census: Run README validation
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/unit/explain-value-census.test.ts`:
- Around line 61-63: Update the outside-content extraction in the explain-value
census test to locate the generated block using its BEGIN and END marker
indices, then concatenate only the lines before BEGIN and after END. Replace the
content-based filter around README and block so duplicated lines matching
generated content cannot be incorrectly removed.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b84bab06-39f9-4ec4-9cc2-6ce6a32391e7
📒 Files selected for processing (6)
.github/workflows/ci.yamlCHANGELOG.mdcommands/explain/README.mdpackage.jsonscripts/explain-value-census.tstest/unit/explain-value-census.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (7)
- GitHub Check: CHR smoke (stable)
- GitHub Check: Corpus census
- GitHub Check: Build
- GitHub Check: copilot-pull-request-reviewer
- GitHub Check: Analyze (actions)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (javascript-typescript)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Transport or RouterOS-touching code is not done untilbun run test:integrationpasses.
Do not silently fall back to another protocol when the caller pinned--via.
Do not make generated output the hand-edited source of truth.
Do not disable validation to make a test pass; validation is part of the product.
Files:
test/unit/explain-value-census.test.tsscripts/explain-value-census.ts
{docs/**,.github/instructions/**,.github/**/*.yml,.github/**/*.yaml,**/*.{md,txt,dict}}
📄 CodeRabbit inference engine (AGENTS.md)
Run
bun run lint:ciwhen changing documentation, instructions, security configuration, spelling dictionaries, or workflow files.
Files:
CHANGELOG.md.github/workflows/ci.yamlcommands/explain/README.md
commands/*/README.md
📄 CodeRabbit inference engine (commands/AGENTS.md)
commands/*/README.md: Eachcommands/<name>/README.mdis the executable specification's designed tier: document intent, flags, and behavior; include aDesigned, not implementedtable for spec-only flags; do not duplicate implemented-flag tables generated indocs/CLI.md; link todocs/CONSTITUTION.mdrather than restating constitution-wide rules; document only command-specific behavior; and keep theStatusline consistent withdocs/MATRIX.md.
Implemented flags must be generated fromCliCommandMetadataintodocs/CLI.mdviabun run docs:cli; command READMEs must not maintain duplicate tables of implemented flags.
Files:
commands/explain/README.md
commands/*/{README.md,examples.md}
📄 CodeRabbit inference engine (AGENTS.md)
Read the target command's
README.mdandexamples.mdas the executable specification before writing code or tests.
Files:
commands/explain/README.md
🧠 Learnings (1)
📚 Learning: 2026-08-11T03:49:52.355Z
Learnt from: mobileskyfi
Repo: tikoci/centrs PR: 265
File: .github/workflows/ci.yaml:171-172
Timestamp: 2026-08-11T03:49:52.355Z
Learning: In this repository's GitHub Actions workflows, SHA-pin `oven-sh/setup-bun` and `actions/setup-node` references and include version comments. Other first-party `actions/*` references may continue using floating major-version tags according to the repository's current convention. Do not request SHA pinning for those first-party actions unless a repository-wide maintainer policy and corresponding Dependabot update policy have been established.
Applied to files:
.github/workflows/ci.yaml
🔇 Additional comments (3)
scripts/explain-value-census.ts (1)
407-451: 🎯 Functional CorrectnessProvide the RouterOS integration result.
This TypeScript census reads RouterOS corpus data. Run
bun run test:integrationand attach a passing result before merge.As per coding guidelines, “Transport or RouterOS-touching code is not done until
bun run test:integrationpasses.”Source: Coding guidelines
package.json (1)
67-67: LGTM!Also applies to: 101-103
commands/explain/README.md (1)
782-801: 📐 Maintainability & Code QualityRun documentation and workflow validation.
Run
bun run lint:ciand attach the passing result before merge.
commands/explain/README.md#L782-L801: validate generated README content.CHANGELOG.md#L30-L41: validate changelog documentation..github/workflows/ci.yaml#L161-L169: validate workflow changes..github/workflows/ci.yaml#L217-L221: validate workflow changes.As per coding guidelines, run
bun run lint:ciwhen changing documentation or workflow files.Source: Coding guidelines
There was a problem hiding this comment.
Pull request overview
This PR closes #260 by turning the explain value-census numbers into drift-gated artifacts, using the committed test/fixtures/explain/values.json corpus block as the single source of truth and adding two checks: (1) fresh corpus census vs fixture (in CI’s corpus job) and (2) README generated block vs fixture (in lint:ci and unit tests).
Changes:
- Add
explain:value-census:check(fixture-vs-fresh-census) andexplain:value-census:readme(:check)(README-vs-fixture) flows inscripts/explain-value-census.ts. - Add a unit test suite to validate README rendering invariants and the fixture-diff comparator behavior.
- Wire the new gates into
lint:ciand thecorpusCI job; updatecommands/explain/README.mdto use a generated value-census block.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
test/unit/explain-value-census.test.ts |
Adds unit tests to enforce README block/fixture consistency and validate drift detection. |
scripts/explain-value-census.ts |
Adds README block render/rewrite/check functionality and a fixture drift comparator plus --check behavior. |
package.json |
Wires README drift check into lint:ci and adds new scripts for value-census gates. |
commands/explain/README.md |
Replaces hand-written census prose with a generated block bounded by markers. |
CHANGELOG.md |
Documents the new drift-gating behavior and the two-gate model. |
.github/workflows/ci.yaml |
Adds a corpus-job step to gate fresh value census output against the committed fixture. |
Suppressed comments (2)
test/unit/explain-value-census.test.ts:63
- The
outsidecalculation filters out any line whose text appears anywhere inside the generated block (block.includes(line)), which can hide duplicated census figures if the same line appears both inside and outside the block. Compute “outside” by slicing the README around the marker indices instead.
const block = readmeBlock().join("\n");
const outside = README.split("\n")
.filter((line) => !block.includes(line))
.join("\n");
scripts/explain-value-census.ts:376
runReadme()splits the README withsplit("\n")and then looks for exact marker lines viaindexOf. If the README has CRLF line endings, lines will include trailing "\r" and the markers won’t match. Splitting on/\r?\n/makes this gate robust on Windows checkouts.
const lines = readme.split("\n");
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…fixes CodeRabbit's finding on the "no census figure outside the block" test was right, and worse than reported. Filtering out README lines whose text appears in the generated block used a SUBSTRING test, so it dropped every blank line (the empty string is "in" any block) plus any short line duplicating generated text: measured, 112 of 1343 lines vanished from the region the test claimed to scan. Now sliced by marker index, like the gate itself does. The test still passes over the full region, so it was not masking real drift. Copilot's CRLF finding is real here, not hypothetical: the repo ships no .gitattributes, so a Windows clone with the Git-for-Windows default core.autocrlf=true gets CRLF, every line keeps a trailing \r, and neither marker is found — the gate would fail for a line ending rather than for drift, in the unit tier #142 wants running on Windows. Both the gate and the test now split on /\r?\n/. The write path additionally rejoins with the ending the file already used: splitLines discards the \r, so writing back a bare "\n" would have silently converted a CRLF checkout to LF and put the whole README in the diff. Verified by CRLF-ifying the real README — check passes, and a forced rewrite preserves all 1342 CRLFs with zero bare LFs introduced. The remaining two were accurate reads of imprecise prose, not bugs: - wrap()'s "counts as one character" described the TOKENIZATION regex, where a backtick span is one atom in the alternation, but read as a claim about length accounting. Reworded to say which it is, and that a span longer than the wrap column overflows rather than splitting. No behavior change. - diffAgainstFixture excluded `censusCommand` only in its second loop. It cannot fire today, since a fresh ValueCensus never carries that key, but the contract was true by accident rather than by rule. Now a PROVENANCE_KEYS set skipped in both directions. Adds a CRLF regression test that asserts the fix is load-bearing (a bare "\n" split finds neither marker). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
#260) Codex's exact-head review of 7612f1e found diffAgainstFixture treating JSON object insertion order as census data. Reproduced: reversing only the KEYS of `shapeCounts`, with every key/value pair preserved, reported drift. It is a live hazard, not a theoretical one. `shapeCounts` is built by `bump()` in first-seen order across the corpus walk, so a corpus whose scripts are visited in a different order — or a hand-tidied fixture — reorders the keys without moving a single count. And the failure mode was the worst kind: the message dumped both tallies, so a reader would have compared two 200-character blobs containing identical data and seen no difference. Tallies are now compared entry by entry over the union of their keys, which fixes the false positive and sharpens the diagnostic at the same time: before: shapeCounts: fixture {"time":128,"str":14845,…}, measured {…} after: shapeCounts.array: fixture 813, measured 814 Sensitivity is unchanged in the direction that matters — added, removed and changed entries all still report, each naming the shape that moved, and an absent side prints "absent" rather than a misleading 0. Regression test covers both halves as asked: reordered-equal maps produce no drift, while add/remove/change still do. Verified end to end that the CI gate still fails on a real injected `shapeCounts.array` change, now with the precise message. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codex Rereview (original review in #260)Yes—PR #267 is good to merge at exact head 1e70c7d.
No remaining findings from my rereview. |
Closes #260.
The gap
The value-census figures live in three places: the census script, the
corpusblock oftest/fixtures/explain/values.json, and prose incommands/explain/README.md. Committing the census script (#242) made themre-derivable; it did not make them checked. #256 changed the value
emission, regenerated the fixture and the unit assertions, and shipped with
the README still quoting the pre-change numbers — three review comments found
that, not CI.
Two gates, split by what data they need
The issue's option (3) was "the one to do first" because it needs no corpus.
But
ci.yaml:164already reserved the other half — "Asserting them againstthe committed fixture is #260's job, and it needs the reachability this job
provides" — so both legs ship here.
bun run explain:value-census:checkcorpusblockcorpusjobbun run explain:value-census:readme:checkcorpusblocklint:ci+bun testThe fixture is the hinge: it is the one copy a human writes, and both gates
point at it rather than at each other. The README paragraph is now a
generated projection (
bun run explain:value-census:readme) between<!-- BEGIN/END GENERATED value-census -->markers.The corpus gate runs after the summary step, so a drifted run still
publishes its figures — those are what say whether the drift is an intended
emission change or a regression.
The generated prose states regressions
Every sentence stays true at any value. The invariants are reported as
counters, not asserted as held:
So regenerating after a real regression states it rather than publishing a
reassuring lie.
test/unit/explain-value-census.test.tsproves that byrendering a synthetic regressed census and asserting the numbers surface.
Verification
explain:value-census --jsonagainst the real 948-script corpus stillreproduces the committed fixture byte for byte — no figure moved.
(
948->999in the README;keyedElements/shapeCounts.arrayin thefixture) before being wired in, and to pass again on restore.
bun run lint:ci && bun run test && bun run buildgreen (2595 pass, 0 fail).Not done here
explain:corpus-censusstill only reports — it has no committed fixture to beasserted against, so there is nothing to gate yet.
🤖 Generated with Claude Code
Summary by CodeRabbit
Documentation
Quality Improvements