From abd755012e233804101e2d2cc41079587ddc2449 Mon Sep 17 00:00:00 2001 From: Claude Code Bot Date: Thu, 10 Sep 2026 18:16:13 -0700 Subject: [PATCH] feat(standards): scope linting to changed files, with a hygiene opt-in MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Whole-repo linting failed PRs for debt in files their author never opened. Measured across the fleet, that was the largest single source of standards-check failures: 24 of 24 failures were markdownlint and/or shellcheck, and the markdownlint ones were dominated by three whitespace rules (MD032 blanks-around-lists, MD031 blanks-around-fences, MD049 emphasis-style) in untouched files. The local pre-commit hook already runs `markdownlint --fix` and works correctly — verified against the exact file that failed CI run 34548182880. It cannot help here, because a staged-file hook cannot fix a file the commit never staged. The gap is scope, not tooling. --changed-since REF narrows the four linters that enumerate through _tracked (shellcheck, yamllint, zizmor, markdownlint) to the files a branch changed, plus new untracked files. actionlint and the Node-floor check find their own inputs and stay whole-repo; neither has ever failed on this fleet. Whole-repo remains available and is still the default without the flag. In CI a PR narrows by default; a deliberate hygiene sweep is requested with the `standards:hygiene` label, and any non-pull_request trigger sweeps because there is no base to diff against. Two false-OK paths are closed explicitly. An unresolvable --changed-since ref exits 2 rather than computing an empty changed set, which would let every linter pass over zero files. A base-SHA fetch failure in CI falls back to a whole-repo sweep rather than to an empty diff — noisy but honest, instead of a green check that linted nothing. Tests pin both directions: a branch must not inherit debt from files it did not touch, and must still fail on violations in files it did. The dirty-file assertion also checks that the untouched file is absent from the output, so a flag that suppressed everything would fail. Hooks are disabled in the fixture via core.hooksPath, because the author's global branch protection refuses the commit and the global markdownlint --fix hook silently repairs the deliberately-bad fixture. 15/15 tests pass; shellcheck -S info clean. Advances smartwatermelon/dev-env#113. Claude-Session: https://claude.ai/code/session_01TkReZXv8XkcWNbcaiWfcvg --- .github/workflows/standards-check.yml | 66 +++++++++++++++++++++++++- standards/run-standards.sh | 66 +++++++++++++++++++++++++- tests/test-run-standards.sh | 68 +++++++++++++++++++++++++++ 3 files changed, 198 insertions(+), 2 deletions(-) diff --git a/.github/workflows/standards-check.yml b/.github/workflows/standards-check.yml index ed71281..cb88342 100644 --- a/.github/workflows/standards-check.yml +++ b/.github/workflows/standards-check.yml @@ -21,6 +21,14 @@ name: Standards Check # uses: smartwatermelon/github-workflows/.github/workflows/standards-check.yml@standards-check-v1 # # Tool versions are pinned here and verified by checksum. Bump them by PR. +# +# SCOPE: by default a pull_request run lints only the files the PR changed. +# Whole-repo linting fails a PR for debt in files its author never opened, +# which was the largest single source of standards-check failures on this +# fleet. To sweep the whole repo deliberately — a hygiene round — add the +# `standards:hygiene` label to the PR and push (the label list is read from +# the event payload, which is captured when the run is triggered, so adding a +# label does not by itself start a new run). Non-PR triggers always sweep. on: workflow_call: @@ -53,6 +61,12 @@ on: description: Lowest supported Node.js major type: string default: "22" + hygiene_label: + description: >- + PR label that forces a whole-repo sweep instead of the default + changed-files-only scope. + type: string + default: "standards:hygiene" permissions: {} @@ -85,6 +99,48 @@ jobs: # An empty ref is the dangerous case: actions/checkout would silently # take the default branch, so the check would go green having linted a # different commit than the one under review. Fail loudly instead. + # Decide the scope of this run, and fetch the base commit if narrowing. + # + # Default is changed-files-only: a PR is judged on what it changed, not + # on debt that predates it. A whole-repo sweep is still available, and is + # what runs on any non-pull_request trigger (there is no base to diff + # against) or when the PR carries the hygiene label. + # + # `fetch-depth: 1` leaves only one commit locally, so the base commit has + # to be fetched explicitly; GitHub serves any SHA reachable in the repo. + # The checked-out HEAD for a pull_request event is refs/pull/N/merge, + # whose first parent is the base — so diffing against BASE_SHA yields the + # PR's own changes and nothing else. + # + # If that fetch fails, this falls back to a whole-repo sweep rather than + # to an empty diff. A failed narrow must never be the quiet path: linting + # everything is noisy but honest, linting nothing is a green check over + # zero files. + - name: Resolve scope + id: scope + env: + BASE_SHA: ${{ github.event.pull_request.base.sha }} + LABELS: ${{ join(github.event.pull_request.labels.*.name, ',') }} + HYGIENE_LABEL: ${{ inputs.hygiene_label }} + run: | + set -euo pipefail + if [ -z "${BASE_SHA}" ]; then + echo "no pull_request base sha — whole-repo sweep" + exit 0 + fi + case ",${LABELS}," in + *",${HYGIENE_LABEL},"*) + echo "hygiene label '${HYGIENE_LABEL}' present — whole-repo sweep" + exit 0 + ;; + esac + if git -C repo fetch --depth=1 origin "${BASE_SHA}" 2>/dev/null; then + echo "changed_since=${BASE_SHA}" >> "${GITHUB_OUTPUT}" + echo "scoping to files changed since ${BASE_SHA}" + else + echo "::warning::could not fetch base ${BASE_SHA}; falling back to whole-repo sweep" + fi + - name: Resolve the standards SHA id: standards_sha env: @@ -161,6 +217,7 @@ jobs: SKIP_MARKDOWNLINT: ${{ inputs.markdownlint == false && 'markdownlint' || '' }} SKIP_NODE_FLOOR: ${{ inputs.node_floor_check == false && 'node-floor' || '' }} NODE_FLOOR: ${{ inputs.node_floor }} + CHANGED_SINCE: ${{ steps.scope.outputs.changed_since }} run: | set -euo pipefail [[ "${NODE_FLOOR}" =~ ^[0-9]+$ ]] || { echo "::error::node_floor must be an integer major"; exit 2; } @@ -171,8 +228,15 @@ jobs: done skip_list="$(IFS=,; echo "${skips[*]-}")" echo "skip list: '${skip_list}'" + # An empty CHANGED_SINCE means whole-repo, so the flag is omitted + # entirely rather than passed with an empty value — the script + # treats an unresolvable ref as a hard error, which is correct for a + # typo'd ref but wrong for "no narrowing requested". + scope_args=() + [ -n "${CHANGED_SINCE}" ] && scope_args+=(--changed-since "${CHANGED_SINCE}") bash standards-src/standards/run-standards.sh \ --repo repo \ --config-dir standards-src/standards \ --skip "${skip_list}" \ - --node-floor "${NODE_FLOOR}" + --node-floor "${NODE_FLOOR}" \ + "${scope_args[@]-}" diff --git a/standards/run-standards.sh b/standards/run-standards.sh index 72b5c56..d64daf5 100755 --- a/standards/run-standards.sh +++ b/standards/run-standards.sh @@ -2,12 +2,25 @@ # run-standards.sh — the deterministic standards check. # # run-standards.sh [--repo DIR] [--config-dir DIR] [--skip a,b,c] [--node-floor N] +# [--changed-since REF] # # Runs shellcheck, yamllint, actionlint, zizmor, markdownlint, and the # Node-floor check over the tracked files of DIR (default: cwd). Exit 0 only # when every enabled linter is clean. A linter with nothing to lint passes # with a notice — absence of files is not a failure. # +# --changed-since REF narrows the file-based linters to files this branch +# actually changed, plus new untracked files. Without it the linters see every +# tracked file, which is the right behaviour for a deliberate hygiene sweep but +# the wrong one for a feature PR: pre-existing debt in a file the author never +# opened fails their unrelated change. Measured across the fleet, that was the +# single largest source of standards-check failures. +# +# Scope: the flag narrows the four linters that enumerate through _tracked +# (shellcheck, yamllint, zizmor, markdownlint). actionlint and the Node-floor +# check find their own inputs and stay whole-repo — both are cheap, and +# neither has ever produced a failure on this fleet. +# # Config precedence, per linter: a config at the repo root wins; otherwise # the canonical file under --config-dir (github-workflows/standards/) is # used. zizmor's canonical config is ../zizmor.yml relative to --config-dir. @@ -19,12 +32,14 @@ repo="$(pwd)" config_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" skip="" node_floor="22" +changed_since="" while (($# > 0)); do case "$1" in --repo) repo="$2"; shift 2 ;; --config-dir) config_dir="$2"; shift 2 ;; --skip) skip="$2"; shift 2 ;; --node-floor) node_floor="$2"; shift 2 ;; + --changed-since) changed_since="$2"; shift 2 ;; *) echo "::error::unknown argument: $1"; exit 2 ;; esac done @@ -42,9 +57,58 @@ fi failures=0 _skipped() { [[ ",${skip}," == *",$1,"* ]]; } +# Say which mode is in effect. A run that lints 3 files and one that lints 300 +# both print "all enabled linters clean", so without this line the scope of a +# green check is not recoverable from its log. +if [[ -n "${changed_since}" ]]; then + echo "scope: files changed since ${changed_since} (+ new untracked files)" +else + echo "scope: all tracked files (whole-repo hygiene mode)" +fi _header() { echo; echo "== $1"; } _fail() { echo "::error::$1 found problems"; failures=$((failures + 1)); } -_tracked() { git -C "${repo}" ls-files -z --cached --others --exclude-standard; } + +# Fail loudly on a --changed-since ref git cannot resolve. Left to fall +# through, an unresolvable ref yields an empty changed set, every file-based +# linter reports a clean pass over nothing, and the check goes green having +# linted zero files — the same false-OK shape the non-repo guard above exists +# to prevent. +if [[ -n "${changed_since}" ]] && + ! git -C "${repo}" rev-parse --verify --quiet "${changed_since}^{commit}" >/dev/null; then + echo "::error::--changed-since ${changed_since} is not a resolvable commit in ${repo}" + exit 2 +fi + +# The file set every file-based linter enumerates. +# +# Default: all tracked and untracked-but-not-ignored files. +# +# With --changed-since REF: the files this branch changed, plus new untracked +# files. Both halves matter — the diff covers edits to existing files, and +# ls-files --others covers a file added but not yet committed, which has no +# diff entry against REF at all. +# +# The changed set is intersected with the default set rather than used +# directly, so .gitignore handling, deleted files (ACMR excludes D), and the +# "is this file even ours" question all keep exactly one answer. +_tracked_all() { git -C "${repo}" ls-files -z --cached --others --exclude-standard; } +_tracked() { + if [[ -z "${changed_since}" ]]; then + _tracked_all + return + fi + local -A changed=() + local f + while IFS= read -r -d '' f; do changed["${f}"]=1; done < <( + git -C "${repo}" diff --name-only -z --diff-filter=ACMR "${changed_since}" || true + ) + while IFS= read -r -d '' f; do changed["${f}"]=1; done < <( + git -C "${repo}" ls-files -z --others --exclude-standard || true + ) + while IFS= read -r -d '' f; do + [[ -n "${changed[${f}]:-}" ]] && printf '%s\0' "${f}" + done < <(_tracked_all || true) +} # Shell lint pass over *.sh, *.bash, and files whose shebang is a # bourne-family shell. (Do not start this comment with the linter's name diff --git a/tests/test-run-standards.sh b/tests/test-run-standards.sh index a6a8a3e..3ecf969 100755 --- a/tests/test-run-standards.sh +++ b/tests/test-run-standards.sh @@ -91,5 +91,73 @@ rc=$? set -e if [[ "${rc}" -eq 2 ]]; then _ok "non-repo --repo exits 2"; else _bad "non-repo --repo exited ${rc}, expected 2 (see ${tmp}/not-a-repo.log)"; fi +# --changed-since: a feature branch must not inherit pre-existing debt from +# files it never touched, while still being held to its own changes. +# +# The fixture pins both halves, because a flag that suppressed everything +# would pass the first assertion and be worthless. OLD.md carries real MD032 +# debt and is never touched by the branch; NEW.md is the branch's own file. +# +# Hooks are disabled via core.hooksPath: the author's global hooks include +# branch protection (which refuses the commit on main) and a markdownlint +# --fix hook (which silently repairs the deliberately-bad fixture). Either +# one makes this test measure the environment instead of the flag. +_mk changed-since +cs="${tmp}/changed-since" +git -C "${cs}" config core.hooksPath "${tmpl}" +git -C "${cs}" config user.email test@example.invalid +git -C "${cs}" config user.name "standards test" +printf '# Title\n\nText\n- a\n- b\n' >"${cs}/OLD.md" +git -C "${cs}" add -A +git -C "${cs}" commit -qm base +cs_base="$(git -C "${cs}" rev-parse HEAD)" +printf '# New\n\nSome text.\n' >"${cs}/NEW.md" +git -C "${cs}" add -A +git -C "${cs}" commit -qm feat + +# Control: without the flag, the old debt must still fail. If this passes, +# the fixture is not actually dirty and the assertion below proves nothing. +if bash "${runner}" --repo "${cs}" --config-dir "${cfg}" --skip node-floor >"${tmp}/cs-whole.log" 2>&1; then + _bad "--changed-since control: whole-repo accepted known MD032 debt (see ${tmp}/cs-whole.log)" +else + _ok "--changed-since control: whole-repo still fails on pre-existing debt" +fi + +if bash "${runner}" --repo "${cs}" --config-dir "${cfg}" --skip node-floor \ + --changed-since "${cs_base}" >"${tmp}/cs-narrow.log" 2>&1; then + _ok "--changed-since ignores debt in files the branch did not touch" +else + _bad "--changed-since rejected a clean branch (see ${tmp}/cs-narrow.log)" +fi + +# The other half: the branch is still responsible for its own changes. +printf '# New\n\nText\n- a\n- b\n' >"${cs}/NEW.md" +git -C "${cs}" add -A +git -C "${cs}" commit -qm dirty +if bash "${runner}" --repo "${cs}" --config-dir "${cfg}" --skip node-floor \ + --changed-since "${cs_base}" >"${tmp}/cs-dirty.log" 2>&1; then + _bad "--changed-since accepted a violation in a file the branch changed" +else + if grep -q 'OLD.md' "${tmp}/cs-dirty.log"; then + _bad "--changed-since reported OLD.md, which the branch never touched" + else + _ok "--changed-since still fails on violations in changed files, and only those" + fi +fi + +# An unresolvable ref must exit 2 rather than compute an empty changed set: +# every file-based linter would then pass over nothing and the check would go +# green having linted zero files. +set +e +bash "${runner}" --repo "${cs}" --config-dir "${cfg}" \ + --changed-since deadbeef99 >"${tmp}/cs-badref.log" 2>&1 +rc=$? +set -e +if [[ "${rc}" -eq 2 ]]; then + _ok "--changed-since with an unresolvable ref exits 2" +else + _bad "--changed-since bad ref exited ${rc}, expected 2 (see ${tmp}/cs-badref.log)" +fi + echo "${pass} passed, ${fail} failed" [[ "${fail}" -eq 0 ]]