Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .gitattributes
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
# Without this, a Windows checkout with `core.autocrlf=true` (the Windows
# default) writes CRLF into the working tree while the committed content is LF.
# The scripts that parse these files are written against LF, so the pre-push
# audit fails on files that are perfectly valid, and the push is refused.
# The scripts that parse these files are written against LF, so the commit
# hook audit fails on files that are perfectly valid, and the commit is refused.
# Pinning the checkout to LF keeps the working tree byte-identical to what is
# committed, on every platform.

Expand Down
22 changes: 11 additions & 11 deletions .github/workflows/hooks.yml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
name: Run Git Hooks

# Runs the pre-push hooks from lefthook.yml on the files this PR changes.
# Runs the pre-commit hooks from lefthook.yml on the files this PR changes.
# A local hook can be skipped with --no-verify or never installed, so this is
# the check that cannot be bypassed. Same config as local, nothing to sync.

Expand Down Expand Up @@ -31,7 +31,7 @@ jobs:
steps:
- uses: actions/checkout@v4
with:
# lefthook diffs against the base branch, so it needs its commits.
# The diff below needs the base branch's commits.
fetch-depth: 0

- uses: actions/setup-node@v4
Expand All @@ -41,20 +41,20 @@ jobs:

- run: npm ci

- name: Run the pre-push hooks
- name: Run the hooks on the files this PR changes
env:
BASE_REF: ${{ github.event.pull_request.base.ref }}
run: |
# lefthook reads the base branch from origin/HEAD, which checkout
# does not set, then diffs against the LOCAL branch of that name,
# which checkout does not create. The PR checkout is the merge of
# this PR into the base, so that diff is exactly this PR's changes.
git remote set-head origin "$BASE_REF"
git branch "$BASE_REF" "origin/$BASE_REF"
node_modules/.bin/lefthook run pre-push
# Three dots: changes since the merge base, so files that moved on
# the base branch are never linted. Deleted files are left out.
# -z: lefthook reads stdin as NUL-separated paths, not lines.
# pipefail: a failed diff must fail the job, not pass an empty list.
set -o pipefail
git diff -z --name-only --diff-filter=ACMR "origin/$BASE_REF...HEAD" |
node_modules/.bin/lefthook run pre-commit --files-from-stdin

- name: Audit the rules system
# lefthook skips every command when a PR only deletes files, and a
# The step above sees no file list when a PR only deletes docs, and a
# deleted rule is what breaks dependencies and the index. This runs the
# cross-file checks on every PR, with no file list to get wrong.
run: node scripts/check-rules.mjs --rules
2 changes: 1 addition & 1 deletion .github/workflows/markdown_lint.yml
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ jobs:
with:
run-prettier: false
# Markdown lint runs from lefthook.yml in hooks.yml, with the same
# pinned rumdl as the local pre-push hook.
# pinned rumdl as the local pre-commit hook.
run-markdown: false
run-commits: true
package-manager: bun
4 changes: 2 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,9 @@ improvements.
### Local

After cloning, run `npm install` once to install the pinned `rumdl` version and
enable the markdown lint and rules-audit checks on push (`postinstall` runs
enable the markdown lint and rules-audit checks on commit (`postinstall` runs
`lefthook install`). The hooks live in `lefthook.yml`, and CI runs the same
file.
file. To skip them once, run `LEFTHOOK=0 git commit`; CI still checks.

### Stage / Preview

Expand Down
6 changes: 3 additions & 3 deletions docs/rules/DEV-338.md
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,9 @@ links `docs/README.md`); this rule covers the landing page's own content.
[DEV-330](./DEV-330.md). A README that lies about how to run the project is
worse than one that says nothing.
1. Where the repository provides an automated README check, run it before
pushing and enforce it in the pre-push hook or CI. In this repository that is
`npm run check:rules`, run by the pre-push hook; a repo without such a script
verifies these requirements by review instead.
committing and enforce it in a git hook and CI. In this repository that is
`npm run check:rules`, run by the pre-commit hook and CI; a repo without such
a script verifies these requirements by review instead.

### Acceptance Criteria

Expand Down
35 changes: 21 additions & 14 deletions docs/rules/DEV-380.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
---
id: DEV-380
title: "Enforce Markdown Lint on Push With a Pinned rumdl Hook"
title: "Enforce Markdown Lint on Commit With a Pinned rumdl Hook"
status: "active"
enforcement: "automated"
severity: "error"
Expand All @@ -9,31 +9,38 @@ severity: "error"
## Problem

If lint runs from whatever `rumdl` a contributor has installed, versions format
the same file differently and pushes get blocked by unrelated reformatting. If
it is not enforced at all, malformed markdown lands unchecked.
the same file differently and contributors get blocked by unrelated
reformatting. If it is not enforced at all, malformed markdown lands unchecked.

## Solution

Make lint automatic and version-stable, from the npm-pinned `rumdl` only.

1. Pin `rumdl` as a devDependency, so everyone runs the same version and avoids
the version drift that blocks pushes with unrelated reformatting.
the version drift that blocks work with unrelated reformatting.
1. Install the hooks with [lefthook](https://lefthook.dev), pinned as a
devDependency, and run `lefthook install` in `postinstall`, so the hooks
install on `npm install`. A hand-written hook script re-implements which
files a push changes, and that logic drifts and breaks on new branches.
1. In `lefthook.yml`, add a `pre-push` command that runs the pinned binary
files changed, and that logic drifts and breaks on new branches.
1. In `lefthook.yml`, add a `pre-commit` command that runs the pinned binary
(`node_modules/.bin/rumdl check --fix`), never a global one, over
`{push_files}`, and set `fail_on_changes: always` so the push is blocked if
it reformats anything.
1. Run the same `lefthook.yml` in CI, so a push that skipped the hook is still
checked, and the check lives in one place.
`{staged_files}`, with `stage_fixed: true` so the fixes go into the same
commit. Check at commit, not at push: the staged files are an exact list,
while the files a push changes depend on the local base branch and on which
branch is checked out.
1. Load an `rc` script that finds `node` and the repository's own lefthook, and
skips with a one-line note when they are missing, so a checkout without
`npm install` is never blocked.
1. Run the same `lefthook.yml` in CI on the files the pull request changes
(`lefthook run pre-commit --files-from-stdin`, with `fail_on_changes: ci`),
so a commit that skipped the hook still cannot merge.

### Acceptance Criteria

- [ ] `rumdl` is pinned in `devDependencies`
- [ ] `postinstall` runs `lefthook install`
- [ ] `lefthook.yml` runs `node_modules/.bin/rumdl` on `{push_files}`, not a
global binary
- [ ] CI runs the same `lefthook.yml`
- [ ] A push containing a lint violation is blocked until it is fixed
- [ ] `lefthook.yml` runs `node_modules/.bin/rumdl` on `{staged_files}` in
`pre-commit`, not a global binary
- [ ] A commit's own lint fixes land in that commit
- [ ] CI runs the same `lefthook.yml` and fails a pull request with a lint
violation
40 changes: 24 additions & 16 deletions lefthook.yml
Original file line number Diff line number Diff line change
@@ -1,32 +1,40 @@
# Git hooks for this repo, managed by lefthook (https://lefthook.dev).
# `npm install` installs them. This file is the one place to edit hook logic:
# git runs it on every push, and CI runs the same commands on every PR.
# git runs it on every commit, and CI runs the same commands on every PR.
#
# Checks run at commit time on the staged files: that list is exact, so a
# commit is never blocked by a file it does not touch. rumdl's fixes go into
# the same commit. Skip once with `LEFTHOOK=0 git commit`; CI still checks.

pre-push:
# rumdl --fix rewrites files in place. Fail the push when it did, so the
# fixes get committed instead of pushed around.
fail_on_changes: always
fail_on_changes_diff: true
# Finds node and this checkout's lefthook, or skips with a note when the
# checks cannot run here (no `npm install` yet). CI still checks the PR.
rc: scripts/hook-env.sh

pre-commit:
# Locally rumdl's fixes are staged into the commit. In CI nothing may change.
fail_on_changes: ci
commands:
markdown:
# {push_files} is what this push adds. On a branch with no upstream yet
# lefthook diffs against the default branch, not the whole repo.
glob: "*.md"
# The npm-pinned rumdl only, never a global one: versions format
# differently and would block pushes with unrelated reformatting.
run: node_modules/.bin/rumdl check --fix {push_files}
# differently and would reformat files inconsistently.
run: node_modules/.bin/rumdl check --fix {staged_files}
stage_fixed: true
priority: 1
rules:
# Authoring standard for the rules this push touches, so a push is
# Authoring standard for the rules this commit touches, so a commit is
# blocked by its own rules, never by pre-existing ones.
glob: "docs/rules/*.md"
run: node scripts/check-rules.mjs --rules {push_files}
run: node scripts/check-rules.mjs --rules {staged_files}
priority: 2
rules-system:
# Cross-file invariants (dependencies, index, reachability) over the whole
# tree, on any push. `--rules` with no names keeps the authoring standard
# out of scope. lefthook skips every pre-push command when a push only
# deletes files, so hooks.yml also runs this outside lefthook: deleting a
# rule is exactly what breaks these invariants.
# tree, whenever a doc changes. No file list on purpose: lefthook then
# also runs this for a commit that only deletes a doc, which is exactly
# what breaks these invariants. `--rules` with no names keeps the
# authoring standard out of scope.
glob:
- "docs/**"
- "README.md"
run: node scripts/check-rules.mjs --rules
priority: 3
24 changes: 24 additions & 0 deletions scripts/hook-env.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
# Sourced by every git hook before lefthook runs (see `rc` in lefthook.yml).

# GUI git clients such as GitHub Desktop start without the shell's PATH, so
# node from nvm or Homebrew is missing. Add the usual locations.
if ! command -v node >/dev/null 2>&1; then
for dir in /opt/homebrew/bin /usr/local/bin; do
[ -x "$dir/node" ] && PATH="$dir:$PATH"
done
fi
if ! command -v node >/dev/null 2>&1 && [ -d "$HOME/.nvm/versions/node" ]; then
PATH="$(ls -d "$HOME/.nvm/versions/node"/*/bin | tail -1):$PATH"
fi
export PATH

# Point the hook at this checkout's lefthook. Without this, lefthook's own
# lookup breaks on paths with spaces and falls through to unrelated tools
# (such as Mintlify's `mint`). When the checks cannot run here, say so and let
# the commit through: CI runs the same checks on the PR.
if ! command -v node >/dev/null 2>&1 || [ ! -x node_modules/.bin/lefthook ]; then
echo "git hooks skipped: run \`npm install\` in this checkout (CI still checks)." >&2
exit 0
fi
LEFTHOOK_BIN=node_modules/.bin/lefthook
export LEFTHOOK_BIN
Loading