Repository navigation
test: move check-conformance's branch floor to where the file now stands - #854
Conversation
The floors were measured before #838 and #840 merged. #838 added the release-wiring check to check-conformance.mjs, whose defensive branches (an unreadable workflow file, and the CLI dispatch in main) no test reaches, so the file's branch coverage moved from the measured 94% to 93.07% and main's scripts suite went red. Per the floors rule, the row moves to where the file stands now: 645 of 693 branches covered, less the two-branch allowance, is 92.
allxsmith
added
deep-review
needs-human-review
📸 Story screenshots at handoff —
|
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: allxsmith/bestax/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. WalkthroughThe branch coverage floor for ChangesCoverage floor adjustment
Priority: ⬇️ Low Estimated code review effort: 1 (Trivial) | ~3 minutes Change: Other Merge Risk: ⚪ Minimal · up to The PR reports branch coverage above the new 92% floor, with line and function floors unchanged. No actionable merge-blocking risk is evident; the change is ready for normal checks. Architecture SummaryArchitecture risk: 🔵 Low · up to The change affects 1 system. Changed systems: Architecture concerns Review detailsSystems and components
Before / after behavior
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description explains the coverage change, its cause, affected file, test status, and rationale. However, it omits the required template sections for affected packages, related issues, type of change, checklist, screenshots or demos, and additional context. Resolution Update the description to use the repository template. Identify the affected package, select the type of change, complete the checklist, provide related issue references or state that none apply, and complete or explicitly mark the optional sections as not applicable.
✨ Finishing Touches🧪 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 |
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The updated floor matches the repository’s documented calculation and introduces no unresolved issues.
Review effort: Balanced
Findings: None
What changed in this PR
Updates the per-file coverage baseline after new conformance branches lowered measured coverage.
Changes:
- Lowers
check-conformance.mjsbranch floor from 94% to 92%.
| File | Description |
|---|---|
scripts/coverage-floors.json |
Aligns the branch floor with current coverage and the two-branch allowance. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Preview DeploymentPreview URL: https://615b273c.bestax.pages.dev |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| { | ||
| "scripts/auto-close-duplicates.mjs": { "lines": 50, "branches": 83, "functions": 66 }, | ||
| "scripts/check-conformance.mjs": { "lines": 70, "branches": 94, "functions": 79 }, | ||
| "scripts/check-conformance.mjs": { "lines": 70, "branches": 92, "functions": 79 }, |
There was a problem hiding this comment.
Row lowered past a branch a real assertion can reach — 🟡 Minor · Coverage
What: The floors rule allows lowering a row only "where only a test asserting nothing could reach it". Of the branches #838 left uncovered, one is checkReleaseWiring's declared-dir-without-manifest path at scripts/check-conformance.mjs:4574-4584 — and it is drivable through the function's own root parameter, which no test passes. release-wiring.test.mjs:487 calls await checkReleaseWiring() with the default root only, which is wired, so the violations.push arm never runs. The same fixture root also reaches the catch { return null } at 4559-4563, the other branch the PR body names.
A test for these asserts the exact documented diagnostic, so it is not the hollow test the rule excludes. The genuinely unreachable part is main()'s dispatch (4620-4675), which is where the row legitimately has to give.
Why it matters: The gate exists so a drop gets looked at, and the two arms involved are the fail-closed ones: a declared package whose manifest went unreadable, and a workflow file that stopped being readable. Those are the #706 shape the check was written for. Moving the row to the measured value retires the only pressure to cover them, and it gives up more of the floor than the untestable branches account for — covering them lets the row land higher than 92.
Fix: add one fixture-root case to scripts/release-wiring.test.mjs, then re-measure the row with pnpm test:scripts rather than assuming a value:
test('a declared directory with no readable manifest is reported, not skipped', async () => {
const root = await mkdtemp(join(tmpdir(), 'release-wiring-'));
// The dirs are declared in the workspace file but have no package.json, and
// neither workflow exists, so both the read() catch and the roster arm run.
await writeFile(
join(root, 'pnpm-workspace.yaml'),
`packages:\n${PNPM_PUBLISHED.map(d => ` - ${d}`).join('\n')}\n`
);
const v = await checkReleaseWiring(root);
for (const dir of PNPM_PUBLISHED) {
assert.ok(
v.some(m => m.startsWith(`${dir} is declared in PNPM_PUBLISHED`)),
`${dir} was dropped from the roster in silence:\n${v.join('\n')}`
);
}
});Evidence
pnpm test:scripts on Node 24 after a full turbo run build, 1122 tests green, check-conformance.mjs at 72.47 | 93.07 | 82.70. Its uncovered-line ranges include 4562-4563, 4578-4584 and 4620-4664 / 4671-4675 — the read catch, the roster arm, and main() respectively. grep -rn checkReleaseWiring scripts/ returns exactly two call sites: the CHECKS table and release-wiring.test.mjs:487, neither passing a root.
Note publishablePackages(root) reads pnpm-workspace.yaml without a catch, so an empty temp dir throws before reaching the arm — the fixture needs that file, as above.
There was a problem hiding this comment.
Deep review — 1 blocking · 2 advisory
| # | Severity | Area | Finding | Location |
|---|---|---|---|---|
| 1 | 🟡 Minor | Coverage | Row lowered past checkReleaseWiring's declared-dir-without-manifest arm, which the function's own root parameter makes testable with a real assertion; only main()'s dispatch is genuinely unreachable |
scripts/coverage-floors.json:3 |
| 2 | 🔵 Advisory | Correctness | PR body attributes "the CLI dispatch in main" to #838. main() was already uncovered before either PR, and the new dispatch branch there is #840's --allow-untagged ternary (check-conformance.mjs:4640-4645) — #838 added no branch to main. For the maintainer; neither this review nor a fix agent can edit the body |
PR body |
| 3 | 🔵 Advisory | Robustness | The stated rationale omits the roster arm at check-conformance.mjs:4578-4584, so it covers less than the move does — the substance is finding 1 |
PR body |
Overall: The change is mechanically right and the diagnosis is real — I reproduced main's failure from its own job log (branches 93.07% is below its floor of 94%) and reproduced 93.07% locally on Node 24, CI's major, to the hundredth. 92 is exactly what floorFor(645, 693, 2) returns, so the row matches measuredRow and nothing was fudged. The riskiest part is not the arithmetic but the precondition: the floors rule permits lowering only where a hollow test is the alternative, and one of the branches given up here is the fail-closed roster arm that a short fixture-root test asserts for real. Look at finding 1 first; everything else about this PR is sound, and it does unblock main.
Residual risk:
- Another row stale right now — refuted. A full
pnpm test:scriptsafterturbo run buildis green across all 36 measured files with this change applied, and main's CI log names exactly one failing row. - Node/platform variance between my measurement and CI's — refuted. CI runs Node 24 and so does this checkout (
v24.21.0); both reportcheck-conformance.mjsbranches at93.07%identically, so the new row has the same margin on CI as it does locally. - The new floor hiding a future drop — refuted as in-scope. With 693 branches a whole-percent floor plus
SLACK.branches = 2tolerates roughly eight lost covered branches, and lines (70 vs 72.47) and functions (79 vs 82.70) now sit stale-low. Both are the documented scheme rather than anything this change introduced:coverage-floors.mjsstates floors round down to a whole percent and never ratchet up. - The collision class recurring — open. Two PRs each green alone turned main red together because the ruleset sets
strict_required_status_checks_policy: false, so neither branch had to be up to date before merging. Nothing in this diff changes that, and the row will go stale again the next time a merge adds uncovered branches to a measured file. Out of this PR's scope, but it is why the row moved rather than anything being wrong with the gate. - The gate itself passing while doing nothing — refuted.
coverage-floors.mjsfails on a missing row, on a row no test loaded, and on a non-finite summary field, and its sibling drives a realnode --testin both directions; my green run printed the affirmativeEvery measured file (36) is at or above its floorline rather than staying quiet.
🏄 One number, one point lower, and main's surf is rideable again — clean little paddle-out. Only thing is you traded away a bit more beach than the tide actually took: grab that one fixture-root test and the floor comes back up a notch.
|
🎉 This PR is included in version 5.19.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 1.14.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 2.25.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 4.2.13 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Main's scripts suite is red:
scripts/check-conformance.mjsmeasures 93.07% branch coverage against a floor of 94%.The floors in
scripts/coverage-floors.json(#841) were measured before #838 and #840 merged. #838 added the release-wiring check tocheck-conformance.mjs, and its defensive branches (an unreadable workflow file, and the CLI dispatch inmain) are reached by no test, so the file's branch coverage moved below the row. Each PR passed on its own; together they turn theTeststep red on main and on every open PR.The floors rule for this case is to move the row to where the file stands now and say why. 645 of 693 branches are covered; less the two-branch allowance that's 92. Lines and functions are unchanged and still pass.
pnpm allpasses locally.Summary by CodeRabbit