Skip to content

style: format the tree with bun fmt, and keep it formatted - #1379

Open
oddharsh wants to merge 2 commits into
mainfrom
fmt-everything
Open

oddharsh wants to merge 2 commits into
mainfrom
fmt-everything

Conversation

@oddharsh

@oddharsh oddharsh commented Oct 16, 2026 •

Copy link
Copy Markdown
Owner

What changed

Two commits. Review them separately:

  • 2a8e182 (mechanical): bun fmt under the [fmt] section from docs(gotchas): the bun fmt hazards, held by a safe [fmt] config #1376 on the pinned canary (1.4.3-canary.20261013.1). It touches 503 files, +31.2K / -14.7K lines. There are no hand edits in this commit.
  • fc4ad87 (everything else):
    • package.json: format (bun fmt) and format:check, which check:fast now runs.
    • ci.yml: a Format step in validate that runs bun run format:check, in the background like Lint. It takes 0.1 s.
    • tools/format-hook.ts: registered in .claude/settings.json on PostToolUse for Edit|Write|MultiEdit. It runs bun fmt on the one file an agent just edited, from the project root, and never fails the tool call.
    • 8 contract-test regexes in 5 files spelled out one-line source. They now also accept the wrapped form, using \s* where the formatter breaks a line.
    • shell/desktop and llms/endpoints are re-locked. Their input digests moved with the formatted sources, and both outputs regenerated byte-identical.
    • Docs: CLAUDE.md gets a command and a working-in-this-repo rule, validate's list gains format, and bunfig.toml, the fmt test header and gotcha 74 now say the tree is formatted.

Rebuilt once already. #1372, #1374 and #1377 landed minutes after this opened and left it conflicted. Rather than merge main in, I regenerated the mechanical commit on b8eb4a8 and reapplied the fix commit. The reapply stopped only on the two derivation digest files: #1377 and the formatted sources had each re-locked llms/endpoints. Both outputs were regenerated (byte-identical) and re-locked, never hand-merged.

Why

You asked for it after the trial and impact review. One style everywhere, with no taste calls left in review:

  • Trailing commas: 1,917 added in 401 files.
  • Quotes: 580 single-quoted strings become double, in 66 files.
  • Semicolons: 171 added, in 28 files.
  • Lines over 160 columns: 1,511 down to 814 (what remains is strings and comments).

The hook means agents never have to remember the formatter, and the validate step catches whatever a human or an edit outside Claude Code misses.

Understanding check

  • Intended model: formatting is a pure function of [fmt] and the pinned bun, so the mechanical commit can be regenerated rather than merged. Everything the formatter must not touch is excluded in [fmt] (gotchas 71 to 74), and the hook inherits those exclusions because bun fmt honours ignorePatterns for explicit paths.
  • Prediction:
    • No /a/ URL re-mints and no dictionary delta changes.
    • The served diff is the 46 readable .src twins and the sw.js unregister stub.
    • The Worker bundle keeps its size. esbuild picks minified names by character frequency, so its bytes move.
  • Likely misconception or failure mode:
  • Evidence: every gate was run on main (b8eb4a8) and on this head in the same sandbox:
    • Build: passes. No /a/ or pd/ file changed; 147 staged files differ: 100 Worker sources staged for wrangler, 46 .src twins and sw.js.
    • Suite: 1485 pass, against 1483 on main plus the 2 new tests. The same sandbox failures, 0 new.
    • Lint: identical findings.
    • Typecheck: identical output.
    • derive:check, pages:check and cal's suite: same as main.
    • fmt --check: passes on 616 files.
    • Formatter builds: three (9bcbf1ecb, 408c9374b and 5f5799ed1, 10-13 to 10-16) agree on every file.
    • Prettier: 3.9.10 with the same options accepts the output, so an editor running Prettier won't fight it.
    • Hook control: a hook that spawns fmtx fails the new hook test.
  • Remaining uncertainty:
    • The sandbox has no wrangler (pkg.pr.new is blocked). routes:check fails the same way on unformatted main there, and wrangler's real bundle was never built. This PR's validate covers both.
    • The hook was tested by piping hook input into it. Its first real run will be the next Claude Code session on this repo.

Validation

  • Every gate above, on main and on this head.
  • bun test tools/contract-bun-fmt-keeps-out-of-what-it-would-break.test.mjs: 6 pass. The 2 new cases cover the hook and the wiring.
  • Label: release-path, because this changes validate, the one required check.

🤖 Generated with Claude Code

https://claude.ai/code/session_01JrwabjbnqxMv6U4YmV57aF

@oddharsh oddharsh added the release-path Touches what decides which commit reaches production label Oct 16, 2026 — with Claude
`bun fmt` under bunfig.toml's [fmt] section (#1376) on the pinned
1.4.3-canary.20261013.1. No hand edits in this commit; regenerate it
with `bun run format` on any later main rather than resolving conflicts.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JrwabjbnqxMv6U4YmV57aF
…s it needed

On top of the mechanical reformat in the previous commit:

- package.json: `format` (bun fmt) and `format:check`, which check:fast
  now runs. Named format so the script never shadows the built-in the
  way `lint` shadows `bun lint` (gotcha 67).
- ci.yml: a Format step in validate, background like Lint, 0.1s.
- tools/format-hook.ts, registered in .claude/settings.json on
  PostToolUse for Edit|Write|MultiEdit: formats the one edited file from
  the project root. bun fmt honours [fmt].ignorePatterns for explicit
  paths, so protected files stay byte for byte. It never fails the call.
- 8 contract-test regexes in 5 files that spelled out one-line source
  now accept the wrapped form too (`\s*` where the formatter breaks).
- shell/desktop and llms/endpoints re-locked: their input digests moved
  with the formatted sources; both outputs regenerated byte-identical.
- CLAUDE.md, bunfig.toml and the fmt contract test now say the tree is
  formatted; gotcha 74 says what a new source-reading test must do.

The contract test gains two cases: the hook formats an in-scope file,
leaves an ignored one and anything outside the project alone, and exits
0 on bad input (control: a hook that spawns `fmtx` fails it); and the
scripts, the validate step and the hook registration are all wired.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JrwabjbnqxMv6U4YmV57aF
@github-actions

Copy link
Copy Markdown
Contributor

Wire-size diff vs merge base

b8eb4a889068b71983c66cfccbc3b98944e4cbd3 → 1ca52eb2033a3b4f484b6652f74f5db6e6333908

Brotli q11, the quality the edge serves. Every number here is deterministic:
an unchanged file produces no row. Advisory — this check fails on nothing.

Client assets

Total Brotli: 205.67 KiB → 205.67 KiB (+0.00 KiB, +0.0%) across 39 files, 1 changed.

file base head Δ
sw.js 0.50 KiB 0.50 KiB +0.00 KiB (+0.6%)

Pages

No change. 74 files, 667.11 KiB Brotli total.

Worker bundle

No change. 285.80 KiB gzip.

Dictionary transport

Family dictionary acquisition: no change, 14.50 KiB q11.

pd/: no change. 294 deltas, 445.24 KiB total.

Comment thread tools/lib/html-to-md.ts
Comment on lines +958 to +959
renderInline(d.children, ctx)
.replace(/\|/g, "\\|")

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This existed before this PR. The formatter only wrapped the chain across lines; main has the same tokens on one line (.replace(/\|/g, "\\|")). CodeQL's own summary notes that a diff this large can surface alerts the PR didn't introduce.

The pattern is also deliberate. The cell text comes from renderInline, which has already run escapeMd on it, and the comment right above this line records the measurement: double-backslash-then-pipe, lone backslashes and trailing backslashes, in both text and code-span cells, all 10 of 10 round-tripped. contract-md-table-escapes.test.mjs pins those bytes.

Escaping backslashes again here would double-escape them and break that test, so I'm not changing it in a formatting PR. Whether to dismiss the alert is the owner's call, so I'm leaving this thread open.


Generated by Claude Code

Copy link
Copy Markdown
Owner Author

CodeQL is red: 2 high "Incomplete string escaping" alerts. I don't think this PR introduced either one. The reformat only wraps lines, and CodeQL's summary itself warns that a diff this large can surface alerts a PR didn't introduce.

  • tools/lib/html-to-md.ts:959 (alert 130). The same tokens sit on one line on main. The escape is deliberate: cell text has already been through escapeMd. I checked the obvious "fix", adding .replace(/\\/g, "\\\\") before the pipe escape, and it fails contract-md-table-escapes.test.mjs (1 pass, 1 fail) because it double-escapes. Details are in the review thread.
  • The second alert didn't post inline, and this session can't read code-scanning alerts. The only other backslash-producing replace in a reformatted file that I'd expect CodeQL to look at is escICS in cal/src/email.ts. It escapes [\\;,] (backslash included) before newlines, and it's unchanged from main apart from line breaks.

CodeQL isn't a required check (validate is). Dismissing both alerts as false positives, or marking them "won't fix", is your call from the security tab. I haven't dismissed anything.


Generated by Claude Code

@cloudflare-workers-and-pages

Copy link
Copy Markdown

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Preview URL Updated (UTC)
✅ Deployment successful!
View logs
aadhar-sh fc4ad87 Commit Preview URL

Branch Preview URL
Oct 16 2026, 06:07 PM

This branch has not been deployed

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

Labels

release-path Touches what decides which commit reaches production

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants