fix(ci): wire the existing oxfmt gate into check.mjs - #3
Open
catomean wants to merge 5 commits into
Open
Conversation
The repo already had oxfmt wired as format/format:check, but nothing in scripts/check.mjs ever called it — verify passing was never evidence the tree was formatted. These 11 files had drifted; fixed mechanically with 'oxfmt --write'.
format/format:check have existed since oxfmt was adopted, but scripts/check.mjs never called format:check — a passing 'pnpm check' was never evidence the tree was formatted, only that it built and typechecked. Prettier was never the right formatter here for a 17.6k-file repo; this repo already made the right call with a faster Rust-based one, it just never got wired in. Added as the last entry in the existing 'preflight guards' stage, alongside the other cheap npm-script guards it already runs in parallel with — one line, same shape as every other entry there. Proved by mutation: appended deliberately mis-formatted code to a tracked file and ran 'node scripts/check.mjs' end to end. It failed at the format step, naming exactly that file, and nothing else changed. Reverted before committing. Unrelated pre-existing finding while running the full check, noted for whoever owns it: 'npm shrinkwrap guard' also failed — 'npm-shrinkwrap.json is stale. Run pnpm deps:shrinkwrap:generate.' — with a clean git status on both npm-shrinkwrap.json and pnpm-lock.yaml beforehand, so it is not something this change caused.
The scripts/check.mjs wiring in the previous commit is what 'pnpm check' and any local dev running the aggregate check picks up. It turns out CI itself does not call check.mjs at all for this task: .github/workflows/ci.yml's check-shard job hand-lists the same commands a second time, independently, in a 'case "$TASK" in guards) ... esac' block — and that list had drifted from check.mjs's own array even before this PR (it's missing several entries check.mjs has, e.g. media-download-helpers, runtime-sidecar-loaders, opengrep-rule-metadata). Confirmed by reading check-guards' actual CI log: the workflow runs each 'pnpm check:*'/'lint:*' line directly, never invoking node scripts/check.mjs. So the previous commit alone, however correct, would never have been enforced by CI — only by a human or agent remembering to run 'pnpm check' locally, which is the exact kind of unenforced-by-default gap this PR exists to close. Added as one more line in the 'guards)' case, matching every other entry's shape. The two lists (check.mjs's array, and this case block) are still two independently-maintained copies of the same guard list after this change — that duplication is pre-existing and out of scope here, but is worth someone eventually collapsing to one source of truth.
…52.0 oxfmt@0.52.0's native binding (@oxfmt/binding-linux-x64-gnu) cannot be resolved by npm/pnpm's optional-dependency install on GitHub's hosted runner (confirmed via a diagnostic workflow_dispatch run, cache-cleared to rule out stale-cache: same failure with zero cache present). This makes the format:check gate wired in the previous commits unable to run at all on this repo's actual CI, not just locally. 0.65.0 resolves the binding cleanly and was confirmed green via a diagnostic PR/dispatch before being folded in here. The 13-version gap also changed a few internal formatting rules (mainly how ternaries inside lit-html template expressions wrap), so this commit includes the resulting --write diff across 138 files alongside the version bump and lockfile update. No source-level renames, directive relocations, or logic changes -- verified the diff is whitespace/wrap only and that no eslint-disable/@ts-expect-error comment lost its target line.
Dependency GuardThis PR changes dependency-related files. Maintainers should confirm these changes are intentional. Changed files:
Maintainer follow-up:
|
Dependency graph changes notedThis PR includes dependency graph changes. The dependency guard is informational because the PR author is a repository admin or a member of
Security review is still recommended before merge when the dependency graph change is intentional. |
…-check # Conflicts: # pnpm-lock.yaml
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
1 participant
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.
format/format:check have existed via oxfmt for a while, but nothing in scripts/check.mjs ever called them — a green
pnpm checkwas never evidence the tree was formatted.Two commits: mechanical
oxfmt --writeon the 11 files that had drifted, then the one-line wiring into the existingpreflight guardsstage.Proved by mutation (details in the second commit message): a deliberately mis-formatted file made
node scripts/check.mjsfail specifically at theformatstep.Unrelated, noted for whoever owns it: the same full run also failed
npm shrinkwrap guard—npm-shrinkwrap.json is stale. Confirmed not caused by this branch (clean git status on both lockfiles beforehand).