Repository navigation
fix(hooks): drop biome --unsafe from the pre-commit format hook - #166
Merged
Merged
Conversation
The pre-commit `format` hook ran `biome check --write --unsafe` with `stage_fixed: true`. Unsafe fixes may change runtime behavior, so that combination could rewrite semantics and auto-stage the rewrite into the commit with no prompt, no diff, and exit 0 — making the committed revision differ from the reviewed one while every check reported green. Verified against this repository's own biome config: the old command rewrote `db.transaction!(...)` to `db.transaction?.(...)` and reported only "Checked 1 file in 39ms. Fixed 1 file.". In a branch already gated on `transaction` being present, that turns an assertion into a silent skip. Without `--unsafe` the same input is left untouched and the proposed change is printed as a warning instead, so formatting and safe fixes still auto-stage while a semantics-changing fix becomes visible and opt-in. Unsafe fixes remain available as `pnpm format:unsafe`, scoped with `--staged` (biome vcs integration is already enabled) so the remedy touches only the files being committed; a trailing path narrows it further. Verified with one staged file and an identical unstaged sibling: "Checked 1 file", staged file rewritten, sibling and every other repository file untouched. One consequence is intended rather than incidental: for an error-severity rule whose only fix is unsafe, the commit now fails instead of succeeding silently. The fail_text therefore names the scoped command and warns that unsafe fixes can change runtime behavior, rather than only saying "fix manually". Lint enforcement does not regress: pre-push already runs `biome ci --diagnostic-level=error .`, green across 35 files. This is the same defect and the same resolution as happyvertical/smrt#2869 (PR happyvertical/smrt#2870), found by sweeping sibling repositories. Refs #165
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The pre-commit
formathook rannpx biome check --write --unsafewithstage_fixed: true. Biome documents unsafe fixes as ones that may changeruntime behavior; combined with auto-staging, such a rewrite landed in the
commit with no prompt, no diff, and exit 0 — so the committed revision could
differ from the reviewed one while every check still reported green.
Closes #165.
Found by sweeping sibling repositories after the identical defect was fixed in
happyvertical/smrt(happyvertical/smrt#2869, PR happyvertical/smrt#2870, nowmerged). This applies the same resolution, adapted to this repository's naming.
The defect, reproduced against this repo's own biome config
db.transaction!(() => {})in a branch already gated ontransactionChecked 1 file in 39ms. Fixed 1 file.exit 0db.transaction?.(…)— an assertion becomes a silent skipNo diagnostic named the rule.
fail_textonly fires when Biome cannot fixsomething, so a successful semantic rewrite was completely silent.
What changed
lefthook.yml—--unsaferemoved from the pre-commitformatcommand.Safe fixes and formatting still apply and still auto-stage, so the hook's
convenience is unchanged. The same input is now preserved and the proposed
change is printed as a warning instead.
fail_text— now names the scoped remedy and warns that unsafe fixes canchange runtime behavior, instead of only saying "fix manually".
package.json— newformat:unsafe, scoped to staged files (--staged;biome.jsonalready setsvcs.enabled+clientKind: git), so the remedytouches only what the author is committing.
Naming note
smrt called its equivalent script
lint:fix-unsafe. This repo useslint*=biome lintandformat=biome check --write ., so the unsafecounterpart of
formatisformat:unsafe. The name follows this repository'sconvention rather than smrt's; the
fail_textnames the correct script, sothere is nothing to memorize either way.
Intended behavior change, stated plainly
For an error-severity rule whose only fix is unsafe, the commit now fails
where it previously succeeded after silently rewriting the file. That is the
point of the change, and it is documented in the hook comment. Warn-level
diagnostics (e.g.
noNonNullAssertion) do not block commits.Lint enforcement does not regress: pre-push already runs
biome ci --diagnostic-level=error ., green across 35 files.Validation
Evidence was regenerated after
pnpm install --frozen-lockfile; a first run wasdiscarded because the worktree had no
node_modulesand the commands exitedELIFECYCLErather than actually checking anything.noNonNullAssertionformat:unsafescopingChecked 1 file; sibling and every other repo file untouchedpnpm format-checkChecked 35 files. No fixes applied.npx lefthook dump--unsafeNo source file is touched, so
svelte-check/tsccannot observe this change;the probes above are the executable evidence for the behavior that did change.
Review
fastest, gpt)advanced)Both findings accepted during smrt#2870's review (scope the opt-in script;
make
fail_textname it) are already incorporated here rather than rediscovered.{ "schema": "hv-agent-run:v1", "runtime": "claude", "session": "a740c64d-ca3b-48fd-bb27-f43b473caaa5", "issue": 165, "policy_revision": "1.0.0", "status": "complete", "validation": [ "probe: biome check --write --unsafe vs --write on noNonNullAssertion (base rewrote silently at exit 0; head preserves + warns)", "probe: pnpm format:unsafe with one staged file + identical unstaged sibling (Checked 1 file; sibling and all other files untouched)", "pnpm format-check (biome ci --diagnostic-level=error .) — 35 files, 0 errors", "npx lefthook dump — effective run line carries no --unsafe", "pnpm install --frozen-lockfile before evidence generation (first run discarded: no node_modules)", "commit made with hooks ENABLED (commitlint passed); no --no-verify" ] }