Skip to content

Make the local rustfmt gate runnable on Windows worktrees - #1485

Merged
justinchuby merged 1 commit into
mainfrom
squad/fmt-gate-windows
Aug 19, 2026
Merged

justinchuby merged 1 commit into
mainfrom
squad/fmt-gate-windows

Conversation

@justinchuby

@justinchuby justinchuby commented Aug 19, 2026 •

Copy link
Copy Markdown
Owner

Why

CI's Rust quality lane (cargo fmt --all -- --check) runs on Linux and is fine.
The local gate that is supposed to catch rustfmt drift before it lands is
non-functional on Windows — which is where this repo's agent workflow runs, from
git worktrees. The quality lane on main has been repaired for rustfmt drift
four times (#1260, #1320, #1393, #1400). That the broken local gate is the cause
of those four repairs is a plausible inference, not something I measured — I only
verified that the local gate does not work.
This PR makes the local gate
runnable on Windows. It does not claim to prevent future drift.

Defects (each verified on this Windows box)

Environment: rustfmt 1.9.0-stable, cargo 1.97.1, Git-for-Windows bash 5.3,
git config core.autocrlf = true, workspace = 54 members / 972 tracked .rs
files
, mixed-edition (52 on edition 2024, 2 on 2021).

  1. Shell scripts check out as CRLF. .gitattributes only pinned
    schema/inference_metadata.schema.json; *.sh and the extension-less
    scripts/hooks/* were unprotected, so all 14 tracked .sh files + the hook
    showed w/crlf. Running one under bash printed
    scripts/install-hooks.sh: line 10: $'\r': command not found and
    set: pipefail: invalid option name — unrunnable.

  2. install-hooks.sh cannot work in a worktree. It used
    HOOKS_DST="$REPO_ROOT/.git/hooks" and bailed if that dir was missing. In a
    worktree .git is a file, so it always errored "are you in a git repo?".

  3. cargo fmt --all fails on Windows regardless. cargo fmt --all -- --check
    exits 1 with The filename or extension is too long. (os error 206):
    cargo-fmt passes every path to one rustfmt, overflowing the Windows ~32 KB
    command-line limit. Linux CI is unaffected (ARG_MAX ~2 MB). The old
    pre-commit ran this with 2>/dev/null and then told the user
    Fix: cargo fmt --all — a command that also fails with os error 206.
    (This fails loudly with exit 1 — there is no false-green here.)

  4. No hook was installed in this checkout — a consequence of 1+2.

Mixed-edition trap (why the fix uses cargo fmt -p, not raw rustfmt): with
the wrong edition, rustfmt mis-parses 2024-only syntax (e.g. let chains:
error: let chains are only allowed in Rust 2024 or later) and fails. Only
cargo knows each package's declared edition, so driving the check per package is
the only correct approach. (An earlier claim of a silent exit-0 false green was
traced to a measurement artifact — rustfmt … | Select-Object -First N truncates
the pipeline and drops the native exit code — and has been withdrawn; the failure
is exit 1.)

Changes

  • .gitattributes — pin *.sh and scripts/hooks/* to eol=lf (comment
    explains a CRLF bash script is unexecutable) and renormalize. All 15 files now
    report i/lf w/lf attr/text eol=lf.
  • scripts/install-hooks.sh — resolve the hooks dir via
    git rev-parse --git-common-dir (the shared gitdir used by the main checkout
    and every linked worktree), resolving a relative result to absolute. Keeps
    --dry and the "does not clobber foreign hooks" property.
  • scripts/hooks/pre-commit — map the staged .rs files to their owning
    workspace packages and run cargo fmt -p <pkg> -- --check only for those.
    Now mirrors CI's scope exactly:
    • Files whose crate is not a workspace member (e.g. the root-level
      bench-* crates) are skipped with a warning, because cargo fmt --all
      does not cover them either. Blocking on a non-member would recreate the
      os-error-206 failure shape (cargo fmt -p <non-member> → "not a member of
      the workspace") and wall people off behind drift they never introduced.
      Membership is taken from cargo metadata --no-deps (matched on
      manifest_path, which is unambiguous — bare "name" keys also appear on
      every dependency).
    • If cargo metadata itself fails, the hook fails open (warns, lets the
      commit through) — a format gate must not lock you out of the repo.
    • Stops suppressing stderr; the printed fix is cargo fmt -p <pkg> (works on
      Windows).
  • wiki/development/Testing and Verification.md — state plainly that
    cargo fmt --all does not work on Windows here; give the per-package
    alternative and bash scripts/install-hooks.sh.

Verification (measured on this box)

  • install-hooks.sh --dry succeeds from the worktree and (relative-.git
    branch) from a normal checkout, both resolving to the same shared
    …/onnx-genai/.git/hooks. Run under Git-for-Windows bash, which actually
    executes hooks. Note: WSL bash cannot run git in a Windows-created worktree at
    all — the .git pointer holds a C:/… path WSL's git can't resolve; that is a
    WSL/Windows limitation affecting every git command there, not this script.
  • End-to-end, against the committed hook:
    • staged a mis-formatted member .rs → commit blocked (exit 1), diff
      shown, fix cargo fmt -p onnx-runtime-cpuinfo printed; ran it → commit
      passed.
    • staged only a non-member (bench-seqmajor) .rs → commit passed with
      the "not a workspace member … CI's cargo fmt --all does not cover them
      either" skip warning.
    • staged a mis-formatted member and a non-member together → commit
      blocked, and the block came only from the member; the non-member was
      skipped and the printed fix command works.
    • simulated cargo metadata failure (stub returning 101) → hook exited 0
      with the fail-open warning.
    • All test artifacts discarded; nothing committed.
  • main is clean by the new check: looping cargo fmt -p <name> -- --check
    over all 54 members → checked 54, failed 0, ignored 0.
  • Hook wall time on a realistic single-package staged change: ~1–2 s (the
    hook only checks the staged packages, not all 54).
  • Full-member confirmation timing (this is the main-clean sweep, not the
    per-commit hook cost): two consecutive runs 26.1 s then 25.2 s,
    consistent with an independent 23.1 s measurement. An earlier one-off 87.5 s
    reading was a non-reproducible first-run outlier and is not representative.

Not touched

  • .github/workflows/ci.yml — CI is not broken; this is a local-gate fix.
  • Anything under .squad/.

Rebase (onto latest main)

Rebased from base 4b1cabb8 onto origin/main at 1557a355 (which had
advanced through #1482, #1173, #1420, #1487). The only conflict was in
wiki/development/Testing and Verification.md: #1482 translated the whole wiki
to Chinese (lang: zh-CN), so my originally-English Windows-formatting section
collided with the now-Chinese baseline. Resolved by following the new Chinese
baseline
— the added formatting/pre-commit documentation is written in Chinese
to match the surrounding prose, and none of #1482's translation was reverted.
Per project rules, code, commit messages and this PR title/body stay in English;
only that wiki body follows its file's language.

Checked that #1487's docs/benchmarks/windows-cuda-runbook.md neither
overlaps nor conflicts with the wiki formatting note (the runbook covers CUDA
benchmarking and contains no formatting/hook content), so no cross-link was
needed.

After the rebase, re-ran the three end-to-end scenarios (member-block→fix→pass,
non-member-only→pass+skip-warning, mixed→blocked-only-by-member) and the
main-clean sweep (checked 54, failed 0) — all still correct. Test
artifacts cleaned; working tree clean.

@justinchuby
justinchuby force-pushed the squad/fmt-gate-windows branch from a550d8e to 504295c Compare August 19, 2026 17:10
CI runs `cargo fmt --all -- --check` on Linux and is fine. The local gate meant
to catch rustfmt drift before it lands was non-functional on Windows, where this
repo's agent workflow runs from git worktrees. Four independent defects, each
verified on this box:

1. Shell scripts checked out as CRLF (core.autocrlf=true, no .gitattributes
   coverage), so bash refused to run them ("$'\r': command not found",
   "set: pipefail: invalid option name").
2. install-hooks.sh assumed `$REPO_ROOT/.git/hooks`, but in a worktree .git is
   a FILE, so it always bailed with "are you in a git repo?".
3. `cargo fmt --all` overflows the Windows ~32 KB command-line limit
   (54 members / ~970 .rs files) and exits 1 with os error 206 — and the hook
   suppressed stderr and suggested the same broken command as the fix.
4. Following from 1+2, no pre-commit hook was installed.

Changes:
- .gitattributes: pin `*.sh` and `scripts/hooks/*` to `eol=lf` and renormalize,
  so bash scripts stay executable on Windows checkouts.
- scripts/install-hooks.sh: resolve the hooks dir via
  `git rev-parse --git-common-dir` (works from the main checkout and any linked
  worktree; hooks are shared across worktrees), handling a relative result.
- scripts/hooks/pre-commit: replace `cargo fmt --all -- --check` with a
  Windows-safe check that maps staged .rs files to their owning workspace
  packages and runs `cargo fmt -p <pkg> -- --check` (each package's own
  edition, important for this mixed 2024/2021 workspace: rustfmt run with the
  wrong edition mis-parses 2024-only syntax such as let chains and fails, so
  only cargo's per-package edition is correct). The hook mirrors CI's scope:
  staged files whose crate is NOT a workspace member (e.g. the root bench-*
  crates, which `cargo fmt --all` does not cover either) are skipped with a
  warning instead of blocked; if `cargo metadata` itself fails the hook warns
  and lets the commit through rather than locking the author out. Stop
  suppressing stderr; print a fix command that works on Windows.
- wiki/development/Testing and Verification.md: document that `cargo fmt --all`
  does not work on Windows here and give the per-package alternative and the
  hook installer.

This makes the local gate runnable on Windows; it does not claim to prevent
future drift.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@justinchuby
justinchuby force-pushed the squad/fmt-gate-windows branch from 504295c to 817fb59 Compare August 19, 2026 17:24
@justinchuby
justinchuby merged commit b4b87ea into main Aug 19, 2026
5 checks passed
@justinchuby
justinchuby deleted the squad/fmt-gate-windows branch August 19, 2026 17:27
@github-actions

Copy link
Copy Markdown

🔴 Benchmark Regression Detected

Comparison of criterion micro-benchmarks: PR head vs merge-base, measured on the same runner in the same job (base first → PR second).

ℹ️ Absolute times are informational only — they vary with runner load. The % change column is the reliable signal because both sides ran under identical conditions.

Status Scenario Base PR Change
🔴 gather/medium_f32_threads=1-internal/32768 3.67 µs 6.59 µs +79.7%
🔴 block_quantized_matmul_cached_dense/mxfp4_preexpanded_dense_oncelock_like_proxy/1x1024x1024 83.42 µs 138.36 µs +65.9%
🔴 add/large_f16_threads=1-internal/4194304 1.74 ms 2.45 ms +40.8%
🔴 matmul/large_generic_f32_threads=8/32x1024x1024 4.07 ms 5.58 ms +37.1%
🔴 sampling_latency/greedy_per_token 3.39 µs 4.60 µs +35.8%
⚠️ tokenization/encode_tokens_per_second 396.44 µs 504.11 µs +27.2%
⚠️ gather/large_bf16_threads=1-internal/131072 12.00 µs 14.70 µs +22.5%
⚠️ block_quantized_matmul_cached_dense/mxfp4_uncached_dequant_each_call/1x1024x1024 1.09 ms 1.30 ms +19.4%
⚠️ tokenization/decode_tokens_per_second 6.56 ms 7.63 ms +16.2%
✅ matmul/large_generic_f16_threads=8/32x1024x1024 95.24 µs 108.79 µs +14.2%
✅ matmul/medium_generic_bf16_threads=8/32x512x512 441.25 µs 499.84 µs +13.3%
✅ gather/large_f16_threads=1-internal/131072 11.62 µs 13.02 µs +12.0%
✅ sampling_latency/top_k_per_token 53.13 µs 59.40 µs +11.8%
✅ matmul/large_generic_f32_threads=1/32x1024x1024 10.08 ms 11.20 ms +11.1%
✅ gather/small_bf16_threads=1-internal/4096 511.1 ns 564.8 ns +10.5%
✅ block_quantized_moe_cached_dense/mxfp4_cached_dense_expert_repeated_call/rows=1,H=256,I=256,E=4,top_k=1 89.39 µs 97.52 µs +9.1%
✅ gather/small_f32_threads=1-internal/4096 706.8 ns 768.1 ns +8.7%
✅ qwen3_sampling_processors/top_k_top_p_full_sort_baseline 5.60 ms 6.04 ms +7.9%
✅ add/small_bf16_threads=1-internal/1024 544.8 ns 580.2 ns +6.5%
✅ sampling_latency/top_p_per_token 376.85 µs 400.95 µs +6.4%
✅ gather/medium_f16_threads=1-internal/32768 2.77 µs 2.95 µs +6.4%
✅ matmul/large_generic_f16_threads=1/32x1024x1024 85.89 µs 90.48 µs +5.3%
✅ qwen3_sampling_processors/top_k_top_p_fast 640.35 µs 669.54 µs +4.6%
✅ grammar_masking/llguidance_compute_mask/32 78.42 µs 81.87 µs +4.4%
✅ reduce_mean/large_f32_threads=1-internal/262144 1.00 ms 1.04 ms +3.9%
✅ sampling_latency/min_p_per_token 209.02 µs 216.72 µs +3.7%
✅ logit_processing/seven_processor_chain_per_step 322.95 µs 331.96 µs +2.8%
✅ qwen3_sampling_processors/top_p_fast_after_top_k 522.30 µs 536.79 µs +2.8%
✅ qwen3_sampling_processors/top_p_full_sort_after_top_k_baseline 3.42 ms 3.50 ms +2.4%
✅ gather/medium_bf16_threads=1-internal/32768 2.45 µs 2.51 µs +2.4%
✅ matmul/small_generic_bf16_threads=8/1x256x256 36.11 µs 36.62 µs +1.4%
✅ qwen3_sampling_processors/top_k_partial_selection 144.72 µs 146.11 µs +1.0%
✅ reduce_mean/small_f32_threads=1-internal/4096 15.76 µs 15.89 µs +0.8%
✅ reduce_mean/medium_f32_threads=1-internal/65536 272.13 µs 270.72 µs -0.5%
✅ block_quantized_matmul_cached_dense/mxfp4_cached_dense_repeated_call/1x1024x1024 98.07 µs 97.51 µs -0.6%
✅ qwen3_sampling_processors/top_k_full_sort_baseline 2.15 ms 2.13 ms -0.9%
✅ matmul/small_generic_bf16_threads=1/1x256x256 34.46 µs 33.83 µs -1.8%
✅ kv_cache/alloc_dealloc_pages 41.96 µs 41.08 µs -2.1%
✅ block_quantized_moe_cached_dense/mxfp4_uncached_expert_dequant_each_call/rows=1,H=256,I=256,E=4,top_k=1 452.03 µs 441.25 µs -2.4%
✅ gather/large_f32_threads=1-internal/131072 41.72 µs 40.16 µs -3.8%
✅ matmul/large_generic_bf16_threads=1/32x1024x1024 2.17 ms 2.07 ms -4.3%
✅ add/medium_f32_threads=1-internal/262144 30.16 µs 28.43 µs -5.8%
✅ add/large_bf16_threads=1-internal/4194304 1.89 ms 1.77 ms -6.7%
✅ gather/small_f16_threads=1-internal/4096 544.2 ns 507.4 ns -6.7%
✅ matmul/medium_generic_f32_threads=1/32x512x512 2.46 ms 2.28 ms -7.3%
✅ matmul/large_generic_bf16_threads=8/32x1024x1024 2.03 ms 1.86 ms -8.5%
✅ matmul/medium_generic_f16_threads=8/32x512x512 30.54 µs 27.82 µs -8.9%
✅ matmul/medium_generic_bf16_threads=1/32x512x512 545.65 µs 494.81 µs -9.3%
✅ add/large_f32_threads=1-internal/4194304 955.82 µs 856.81 µs -10.4%
🟢 add/medium_bf16_threads=1-internal/262144 134.34 µs 113.46 µs -15.5%
🟢 matmul/medium_generic_f16_threads=1/32x512x512 33.42 µs 27.59 µs -17.5%
🟢 matmul/small_generic_f32_threads=8/1x256x256 63.98 µs 50.38 µs -21.3%
🟢 matmul/medium_generic_f32_threads=8/32x512x512 1.31 ms 977.26 µs -25.2%
🟢 add/medium_f16_threads=1-internal/262144 138.15 µs 103.25 µs -25.3%
🟢 matmul/small_generic_f32_threads=1/1x256x256 47.91 µs 35.24 µs -26.4%
🟢 add/small_f32_threads=1-internal/1024 282.8 ns 200.6 ns -29.1%
🟢 matmul/small_generic_f16_threads=8/1x256x256 48.17 µs 33.32 µs -30.8%
🟢 add/small_f16_threads=1-internal/1024 731.0 ns 485.7 ns -33.6%
🟢 matmul/small_generic_f16_threads=1/1x256x256 49.86 µs 33.01 µs -33.8%

Visual flags: ⚠️ ≥ 15% slower, 🔴 ≥ 30% slower — calibrated against measured runner noise (~27% worst-case on multi-threaded matmul)

Host info
CPU: Apple M1 (Virtual)
Cores: 3
OS: Darwin 25.5.0 arm64
Rust: rustc 1.97.1 (8bab26f4f 2026-07-14)
Load avg: { 2.86 3.24 5.48 }
What this cannot catch
  • Regressions in code paths not covered by these benchmarks (e.g., end-to-end decode with a real model)
  • Sub-threshold regressions that compound over multiple PRs
  • Performance changes that only manifest under GPU execution
  • Latency changes in the ORT integration path (these benchmarks exercise the native Rust kernels)

@codecov

codecov Bot commented Aug 20, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.12%. Comparing base (4a9f4ec) to head (817fb59).
⚠️ Report is 66 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##             main    #1485       +/-   ##
===========================================
- Coverage   82.10%   80.12%    -1.99%     
===========================================
  Files          12      376      +364     
  Lines        5471   164127   +158656     
  Branches     5471   164127   +158656     
===========================================
+ Hits         4492   131504   +127012     
- Misses        780    27795    +27015     
- Partials      199     4828     +4629     
Flag Coverage Δ
cli-ort-linux 82.60% <ø> (?)
cli-ort-windows 82.10% <ø> (ø)
offline 80.03% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.
see 365 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants