diff --git a/.gitattributes b/.gitattributes index 3097430..86c799d 100644 --- a/.gitattributes +++ b/.gitattributes @@ -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. diff --git a/.github/workflows/hooks.yml b/.github/workflows/hooks.yml index aac68d7..ff9b9d5 100644 --- a/.github/workflows/hooks.yml +++ b/.github/workflows/hooks.yml @@ -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. @@ -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 @@ -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 diff --git a/.github/workflows/markdown_lint.yml b/.github/workflows/markdown_lint.yml index 73df513..1e00731 100644 --- a/.github/workflows/markdown_lint.yml +++ b/.github/workflows/markdown_lint.yml @@ -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 diff --git a/README.md b/README.md index f306940..a6aee32 100644 --- a/README.md +++ b/README.md @@ -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 diff --git a/docs/rules/DEV-338.md b/docs/rules/DEV-338.md index 9ff1199..4c1d8e8 100644 --- a/docs/rules/DEV-338.md +++ b/docs/rules/DEV-338.md @@ -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 diff --git a/docs/rules/DEV-380.md b/docs/rules/DEV-380.md index 7e6d645..2b41cb6 100644 --- a/docs/rules/DEV-380.md +++ b/docs/rules/DEV-380.md @@ -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" @@ -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 diff --git a/lefthook.yml b/lefthook.yml index 808c388..ee98f84 100644 --- a/lefthook.yml +++ b/lefthook.yml @@ -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 diff --git a/scripts/hook-env.sh b/scripts/hook-env.sh new file mode 100644 index 0000000..31fbb96 --- /dev/null +++ b/scripts/hook-env.sh @@ -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