Skip to content

build: no linter or formatter checked the code - #40

Draft
SferaDev wants to merge 6 commits into
chore/pnpm-node26from
chore/biome-lefthook
Draft

SferaDev wants to merge 6 commits into
chore/pnpm-node26from
chore/biome-lefthook

Conversation

@SferaDev

@SferaDev SferaDev commented Oct 3, 2026 •

Copy link
Copy Markdown
Member

Adds Biome (house config), lefthook pre-commit and knip, formats the code, and fixes what Biome's recommended rules flagged. CI runs lint and knip. Stacked on #39.

The approved stack for sferarc repositories uses Biome for lint and format,
lefthook for pre-commit checks and knip for unused code. peeksafe had none of
them, so style drifted file by file and unused imports went unnoticed.

Add all three from the catalog, with biome.json in the house style used by
the other sferarc repositories. noNonNullAssertion is off: with
noUncheckedIndexedAccess the numerical loops assert about 200 in-bounds reads,
and rewriting them would change code for no behavioural gain. CI runs
`pnpm lint` and `pnpm knip` on every leg.
Apply `biome format` with no other change, so the lint fixes that follow can be reviewed on their own. Quotes become double, lines wrap at 100 columns.
Apply the safe fixes Biome offers (sorted imports, `import type` where only
types are used, `**` over Math.pow, unused imports removed) and fix the rest
by hand. The changelog script's `escape` helper shadowed the global of that
name and is now `escapeRegExp`. The unused `sb` and `nb` parameters of
rawLogE keep their position for the call sites and gain an underscore.

test/exports.test.ts read source text with single-quote patterns. After the
format change its import stripping matched nothing, so the "every export is
used in a test" check passed on imports alone, and the error-code check
missed constructor calls that now wrap. Both patterns now match the
formatted source.
@SferaDev
SferaDev added this pull request to stack #41 October 3, 2026 13:07
@SferaDev SferaDev changed the title chore/biome lefthook build: no linter or formatter checked the code Oct 3, 2026
@SferaDev
SferaDev removed this pull request from stack #41 October 3, 2026 15:34
@sferarc-hq

sferarc-hq Bot commented Oct 3, 2026

Copy link
Copy Markdown
Contributor

Review: build: no linter or formatter checked the code

Reviewed at cbadb53. 3847 added and 1610 deleted lines across every source and test file, so I read the configuration in full (biome.json, lefthook.yml, pnpm-workspace.yaml, package.json, .github/workflows/ci.yml, both brain notes) and audited the reformatted code for structural invariants rather than reading 1800 lines of re-indentation line by line. What I checked and how is below, so you can tell what I did not look at.

The blocking problem

This is based on chore/pnpm-node26, not on main. baseRefName is #39's branch, and I marked #39 hq-changes-requested a few minutes ago: its release workflow rests on an unsourced claim that pnpm performs npm's OIDC trusted publishing, in the same change that deletes the pinned npm that existed to make trusted publishing work, and it moves NPM_TOKEN into a credential file the body does not mention.

Two consequences, and the second is why I cannot mark this one sound.

The diff above is not a diff against main, so approving it approves a change whose base is going to move. More concretely, hq-reviewed tells the merge gate this is safe to land, and landing it merges 3847 lines into another open pull request's branch rather than into main. That silently triples #39's diff and, because a push drops both marks, throws away the review #39 just got. Rebase onto main once #39 is settled, or onto a reworked #39, and the labelling then means what it says.

What I verified about the reformat

The risk in reformatting statistical code is a semantic change hiding in 1800 lines of whitespace. Several things make that unlikely here, and I checked each rather than assuming:

  • Every figure is pinned and the suite is green. Node 22, Node 24 and Node 26 all pass on this head, and test/paper.test.ts and test/readme.test.ts recompute the README's numbers from seeded draws. A formatting change that moved a number would fail those, not pass them.
  • No assertion was dropped. 186 lines containing expect( removed, 186 added. A balanced count is what a pure reformat looks like; a weakened test suite is not.
  • No test was quietly switched off. No .only, .skip or .todo appears on an added line.
  • Only two rewrites are not whitespace or quotes. Math.pow(0.5, 1 / 3) becomes 0.5 ** (1 / 3) in test/stats.test.ts, which is the same IEEE 754 operation and is compared at a tolerance of 1e-8 anyway, and escape becomes escapeRegExp in scripts/release-changelog.mjs, which is Biome objecting to shadowing a global and is covered by test/release-changelog.test.ts. I scanned the whole diff for the rewrites that do change behaviour, isNaN to Number.isNaN, == to ===, parseInt to Number.parseInt, and there are none.
  • The refusals survive. 84 throw/PeeksafeError lines out, 85 in, the difference being a line split. gate.test.ts pins refusals rather than results and it is green.

noNonNullAssertion: off is justified in brain/development/toolchain.md by noUncheckedIndexedAccess making every indexed read optional, with about 200 asserted in-bounds reads in the numerical loops. That is the right trade and the right place to write it down. It does mean a ! outside a bounds-checked loop is now unflagged too, which is worth knowing and not worth a rule exception list.

allowBuilds: lefthook: true is the part I am glad is there: pnpm does not run a dependency's install script unless it is allowed, and lefthook's postinstall is what fetches its binary.

Things to fix in the same pass

NOTICE is now incomplete, and it ships in the tarball. It says "Development dependencies (TypeScript, vitest, @types/node) are not redistributed and are not part of the published package." This pull request adds @biomejs/biome, knip and lefthook, updates brain/development/toolchain.md to list them, and leaves NOTICE naming three of six. NOTICE is published, so a file that enumerates the dev dependencies should enumerate them or stop enumerating them.

Nothing checks that the pre-commit hook exists. CONTRIBUTING.md promises that pnpm install sets up a hook that formats staged files. The hook arrives through lefthook's postinstall plus the allowBuilds entry, and no check anywhere confirms it landed: CI never commits, so a wrong key or a blocked build fails silently and the only symptom is a contributor whose commits are not formatted. Since pnpm lint runs in CI the damage is bounded to a failed check instead of unformatted main, which is why this is a fix-it rather than a blocker, but the promise in CONTRIBUTING.md is stronger than what is verified.

pnpm knip runs with no configuration, and before pnpm build. It passes today, so knip's zero-config inference is finding src/index.ts and treating test/** as entry points. That inference is implicit and version-dependent, in a tool pinned to an exact version in the catalog, and it runs at step 4 while dist/ does not exist until step 7. A small knip.json naming the entry points would make the check mean the same thing after the next knip bump. Also worth saying in brain/development/ci.md why knip sits before the build rather than after it.

What I could not check here

No Docker daemon, no pnpm and no network verb on this runner, so I could not run pnpm lint, pnpm knip or the hook install, and I did not read every reformatted line of src/stats.ts, src/frontier.ts or the eleven test files. The structural audit above is what I substituted for that, and green CI on three Node versions with every README figure recomputed is the strongest evidence available either way.

VERDICT: NO-GO This is based on #39's branch rather than main, and #39 must not land as written, so the gate would merge 3847 lines into another open pull request and discard the review it just received.

# Conflicts:
#	brain/development/toolchain.md
Review of #40 found three gaps. knip ran with no configuration, so what it
checked depended on its zero-config inference in whatever version the
catalog names; knip.json now names the entry and project files and config
hints fail the run. Nothing checked lefthook.yml, so a bad key would only
show as unformatted commits; CI now runs `lefthook validate`, and
CONTRIBUTING says how to install the hook by hand. NOTICE listed three of
the six dev dependencies and ships in the tarball, so it now refers to
package.json instead of enumerating them.
# Conflicts:
#	brain/development/ci.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

hq-changes-requested The reviewer read this and it must not land as written. Dropped when the author pushes.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant