Skip to content

ci: run format check on every PR, including markdown-only ones - #917

Merged
kriszyp merged 5 commits into
mainfrom
fix/pr-format-check-always-run
Oct 13, 2026
Merged

kriszyp merged 5 commits into
mainfrom
fix/pr-format-check-always-run

Conversation

@kriszyp

@kriszyp kriszyp commented Oct 9, 2026 •

Copy link
Copy Markdown
Member

⊙ Problem

.github/workflows/pr.yml's Test workflow is paths-ignored for **.md/docs/**/LICENSE/.gitignore, so a markdown-only PR never runs its Check job -- which runs pnpm check, including oxfmt --check (oxfmt formats Markdown too, per AGENTS.md's Code Quality section). A markdown-only PR with a formatting problem therefore merges clean, and the drift only surfaces later via formatting.yml's nightly auto-fix PR.

❓ Your call: as specified, yes -- a markdown-only PR should run the format check without paying for the full build/test matrix.

💡 Solution

Added .github/workflows/format-check.yml: a small workflow with no paths-ignore, triggered on every PR and on push to main. It needs no native build (oxfmt --check doesn't touch src/binding/**, and formatting runs identically with or without pnpm build), so its install step skips .github/actions/setup-deps's deps/rocksdb cache and GH_TOKEN entirely.

fmt:check is moved out of pr.yml's Check job rather than duplicated there, so package.json splits check into check:code (type-check + lint) and check (check:code + fmt:check) -- pr.yml's Check step now runs pnpm check:code, format-check.yml runs pnpm fmt:check, and local pnpm check (and lefthook) still runs all three.

Checked before choosing this shape: gh api repos/HarperFast/rocksdb-js/rules/branches/main (the effective-rules endpoint, covering both classic branch protection and any ruleset) returns [] -- no status check is required on main today. So adding an unfiltered job can't leave a doc-only PR with a permanently pending required check; it would only matter once/if a required check is configured. format-check.yml's header comment says which job to require if that ever happens (Format Check, not Test/Check -- paths-ignore means Check never runs on a doc-only PR), mirroring the existing note in validate-caller-workflows.yml for the same class of problem.

⚖️ Alternatives

  • Drop '**.md'/'docs/**' from pr.yml's paths-ignore instead of adding a new workflow -- rejected: paths-ignore gates the whole Test workflow, so this would run the full 3-OS x 3-Node build/test matrix plus native/Bun/Deno jobs on every doc-only PR, which is exactly the cost paths-ignore exists to avoid.
  • Keep fmt:check in pr.yml's Check job and duplicate it in the new workflow -- rejected in favor of the check/check:code split: duplicating it costs a few redundant seconds on every code PR and leaves check's command list defined in two places that can drift; the split keeps one definition and CI stays in sync with pnpm check/lefthook by construction.

🔧 Changes

  • .github/workflows/format-check.yml (new) -- unfiltered format-check workflow.
  • .github/workflows/pr.yml -- Check step now runs pnpm check:code instead of pnpm check.
  • package.json -- check split into check:code + check (check:code && fmt:check).

✅ Verification

  • ./node_modules/.bin/oxfmt --check (direct binary, bypassing a stale local pnpm/npm install-lifecycle quirk in this worktree -- see Findings) -- clean on all touched files at every commit in this PR.
  • YAML/JSON parse check on both workflow files and package.json -- valid.
  • gh api repos/HarperFast/rocksdb-js/rules/branches/main -- [], confirming no required status checks exist today (see Solution).
  • Independent cross-model review: 6 rounds (1 full, 4 delta, 1 closing-full), codex + gemini + harper-domain (cursor-composer on round 1). All actionable findings fixed across the rounds (script-list drift risk, a concurrency bug where quick successive pushes to main could drop each other's status, misleading/narrating comments). Converged on 3 recurring items, resolved below.
  • Not run: the actual GitHub Actions workflows (no hosted execution available to this review). This PR's own CI run exercises both jobs since it touches .github/ and package.json, neither of which is path-ignored; the doc-only path this change targets will first be exercised by a future doc-only PR.

❓ Your call: package.json's check:code/check split -- a gate appended directly to check (e.g. a future C++ format check) would run locally via lefthook but not in either CI workflow. No code change for this; it's an inherent tradeoff of the split, not a bug -- flagging it as the thing to remember when adding a fourth gate.

❓ Your call: declined two review findings as pre-existing, repo-wide conventions rather than fixing them only in the new file: pnpm install --no-frozen-lockfile (same as setup-deps and 7 other workflow sites; exposure is low given oxfmt's exact pin and minimumReleaseAgeStrict) and pnpm/action-setup@... version: latest (same as every other workflow in the repo; no packageManager field exists to pin against). Fixing either only here would make this file diverge from the rest -- better as a repo-wide follow-up issue if wanted.

No numbered GitHub issue was filed for this; it was worked from a dispatch task (rocksdb-js-md-format-check).

Open PR #915 also touches pr.yml (adds push/schedule triggers and a run-id-scoped concurrency group) but in the on:/concurrency: top-level blocks, not the jobs.checks.steps region this PR touches -- no line overlap, should rebase cleanly either direction.

Related PRs: #915 overlaps, #382 independent

🤖 Generated with Claude Code

https://claude.ai/code/session_017oenufV4sz7QuQjeAXyuYP

Origin — the dispatch brief this PR was written from

.github/workflows/pr.yml has paths-ignore including **.md, but its Check job runs pnpm check -> oxfmt --check, which formats markdown (.oxfmtrc.json ignorePatterns is empty). A markdown-only PR therefore skips the format check and the error surfaces later (nightly formatting.yml). Move the format check into a small always-run workflow/job without paths-ignore, keeping the heavy test matrix path-filtered. Before choosing, check which checks are required on main so a skipped workflow does not leave doc-only PRs with a pending required check.

Acceptance

A markdown-only PR runs oxfmt --check; code PRs unchanged.

Dispatch: task rocksdb-js-md-format-check · queued by finding-triage/wave-30 · ran by claude/sonnet/high · worker kzyp-xps-1

Review-Coverage: authored=claude; ran=gemini,codex; adjudicated=domain; declined=cursor-grok,cursor-composer,cursor-kimi,cursor-muse; rounds=6; full=3 @ 4a1e139

Review-Attention: study ~13m (sensitive: format-check.yml; decisions: do-less-alternative, format-out-of-check-job, require-format-check-now) @ 4a1e139

kriszyp and others added 5 commits October 9, 2026 08:55
pr.yml's Test workflow skips **.md/docs/** via paths-ignore, but its Check job
runs pnpm check -> oxfmt --check, which also formats Markdown. A markdown-only
PR therefore never ran the format check; the drift only surfaced later via
formatting.yml's nightly auto-fix PR.

Add format-check.yml: a small, always-run workflow (no paths-ignore, no native
build) that runs oxfmt --check on every PR and push to main. Drop fmt:check
from pr.yml's Check step so it isn't verified twice for code PRs; type-check
and lint stay there, path-filtered with the rest of the heavy jobs.

No required status checks are currently configured on main (classic branch
protection's required_status_checks contexts/checks are both empty), so this
can't leave a doc-only PR with a permanently pending required check.

Dispatch-Task: rocksdb-js-md-format-check
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017oenufV4sz7QuQjeAXyuYP
Trim narrative comments (they restated mechanics instead of informing the
next reader), fix a factual error (setup-deps already passes
--ignore-scripts; the real difference is skipping its deps/rocksdb cache
and GH_TOKEN), add contents:read permissions, and point at format-check.yml
as the job to require instead of Test/Check if main ever gains required
checks -- mirroring validate-caller-workflows.yml's existing note.

Dispatch-Task: rocksdb-js-md-format-check
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017oenufV4sz7QuQjeAXyuYP
Split package.json's check script into check:code (type-check + lint) and
check (check:code + fmt:check), and point pr.yml's Check job at check:code.
Removes the hand-maintained sync comment entirely -- a future gate added to
check:code is automatically picked up by CI, instead of silently only
running locally/in lefthook.

Scope cancel-in-progress in format-check.yml's concurrency group to
pull_request only, so two merges to main in quick succession don't leave
the first commit's push run cancelled with no reported status.

Reword the "require THIS job" comment so it reads as what to require, not
as advice against requiring the test jobs.

Dispatch-Task: rocksdb-js-md-format-check
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017oenufV4sz7QuQjeAXyuYP
…roup

Mirror the same group-splitting fix PR #915 applies to pr.yml: GitHub drops
an older QUEUED run sharing a concurrency group when a newer one joins,
regardless of cancel-in-progress. Three or more merges to main in quick
succession could otherwise drop a push run's status. Push now gets a group
keyed to github.run_id instead of the shared ref, so it never collides.

Also trims the install-step comment per repeated review nits.

Dispatch-Task: rocksdb-js-md-format-check
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017oenufV4sz7QuQjeAXyuYP
Dispatch-Task: rocksdb-js-md-format-check
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017oenufV4sz7QuQjeAXyuYP

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request refactors the scripts in package.json by splitting the check script into check and a new check:code script, which groups type-check and lint together. There are no review comments, and I have no feedback to provide.

@github-actions

github-actions Bot commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

📊 Benchmark Results

get-sync.bench.ts

getSync() > random keys - small key size (100 records)

Implementation Rank Operations/sec Mean (ms) Min (ms) Max (ms) RME (%) Samples
🥇 lmdb 1 23.99K ops/sec 41.68 39.58 2,243.885 0.169 119,953
🥈 rocksdb 2 10.34K ops/sec 96.76 90.07 25,221.22 1.03 51,677

getSync() > sequential keys - small key size (100 records)

Implementation Rank Operations/sec Mean (ms) Min (ms) Max (ms) RME (%) Samples
🥇 lmdb 1 27.29K ops/sec 36.64 34.42 748.554 0.143 136,462
🥈 rocksdb 2 10.28K ops/sec 97.32 91.68 712.322 0.123 51,380

ranges.bench.ts

getRange() > small range (100 records, 50 range)

Implementation Rank Operations/sec Mean (ms) Min (ms) Max (ms) RME (%) Samples
🥇 lmdb 1 24.62K ops/sec 40.61 37.03 3,855.23 0.343 123,123
🥈 rocksdb 2 15.44K ops/sec 64.77 54.71 1,278.645 0.172 77,197

realistic-load.bench.ts

Realistic write load with workers > write variable records with transaction log

Implementation Rank Operations/sec Mean (ms) Min (ms) Max (ms) RME (%) Samples
🥇 rocksdb 1 402.64 ops/sec 2,483.581 43.43 84,657.335 19.40 809
🥈 lmdb 2 26.40 ops/sec 37,885.126 454.631 1,197,931.109 136.289 64.00

transaction-log.bench.ts

Transaction log > read 100 iterators while write log with 100 byte records

Implementation Rank Operations/sec Mean (ms) Min (ms) Max (ms) RME (%) Samples
🥇 rocksdb 1 33.26K ops/sec 30.06 13.73 20,937.528 0.851 166,321
🥈 lmdb 2 439.57 ops/sec 2,274.964 167.089 13,728.747 1.37 2,198

Transaction log > read one entry from random position from log with 1000 100 byte records

Implementation Rank Operations/sec Mean (ms) Min (ms) Max (ms) RME (%) Samples
🥇 rocksdb 1 585.40K ops/sec 1.71 1.43 471.055 0.118 2,927,015
🥈 lmdb 2 438.65K ops/sec 2.28 1.10 6,388.515 0.508 2,193,273

worker-put-sync.bench.ts

putSync() > random keys - small key size (100 records, 10 workers)

Implementation Rank Operations/sec Mean (ms) Min (ms) Max (ms) RME (%) Samples
🥇 rocksdb 1 810.20 ops/sec 1,234.269 1,049.223 3,004.578 0.372 1,621
🥈 lmdb 2 1.16 ops/sec 861,546.74 800,391.934 973,899.24 4.81 10.00

worker-transaction-log.bench.ts

Transaction log with workers > write log with 100 byte records

Implementation Rank Operations/sec Mean (ms) Min (ms) Max (ms) RME (%) Samples
🥇 rocksdb 1 19.94K ops/sec 50.15 31.31 20,614.8 2.09 39,882
🥈 lmdb 2 812.35 ops/sec 1,230.995 53.91 13,334.652 5.63 1,626

Results from commit a3abdc9

@kriszyp
kriszyp marked this pull request as ready for review October 12, 2026 17:39
@kriszyp
kriszyp requested a review from cb1kenobi as a code owner October 12, 2026 17:39
@kriszyp
kriszyp merged commit 4c51421 into main Oct 13, 2026
29 checks passed
@kriszyp
kriszyp deleted the fix/pr-format-check-always-run branch October 13, 2026 17:13
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