fix(pre-commit): keep key order in pretty-format-json so apm-owned JSON survives
pretty-format-json sorts object keys by default, but apm emits insertion order and `apm audit --ci` diffs its output byte-for-byte. A file in the formatter's scope therefore drifts on every commit with an empty git diff. Pass --no-sort-keys so `.claude/settings.json` and `.claude/apm-hooks.json` round-trip untouched and drop them from the exclude. marketplace.json stays excluded: it carries literal em dashes the formatter re-escapes to —. tests/test-pretty-json-tool-owned.sh runs the repo's real autofix hooks over copies of the four tracked apm-owned files and fails if any is rewritten, so losing the flag fails a test instead of surfacing as drift. Refs: #102 Co-Authored-By: Claude Code <[email protected]> Claude-Session: https://claude.ai/code/session_01KkT7RSDwDbmrM9T34b6sTi
This commit is contained in:
Defame1297andClaude Code committed
1 parent
3ff0741857
commit
c2c56ff948
4 files changed
+156
-50
No files matched your search
+18
-34
@@ -27,41 +27,25 @@ repos:
|
||||
stages: ['pre-commit']
|
||||
- id: pretty-format-json
|
||||
stages: ['pre-commit']
|
||||
args: [--autofix]
|
||||
# Every generated manifest lives at a KNOWN path, so every alternative is
|
||||
# root-anchored and spells that path out. This was five `(^|/)`
|
||||
# any-depth alternatives plus one `^` root-only one -- a mixture with no
|
||||
# rationale, under which a fixture or vendored tree containing
|
||||
# `.../.claude-plugin/marketplace.json` would have been silently excluded
|
||||
# from formatting while an equivalent
|
||||
# `.../.agents/plugins/marketplace.json` would not. Only the one root
|
||||
# marketplace manifest matches now; anything else is hand-authored and
|
||||
# gets formatted. The twelve per-plugin `plugin.json` alternatives were
|
||||
# dropped with the plugin manifests themselves when native
|
||||
# `claude plugin install` support was removed (ADR-0024) -- apm probes
|
||||
# `apm.yml` and never reached them. The `.agents/plugins/` and
|
||||
# `.github/plugin/` marketplace mirrors went the same way, and their
|
||||
# alternations went with them: `check-useless-excludes` fails on a
|
||||
# pattern that matches no file.
|
||||
args: [--autofix, --no-sort-keys]
|
||||
# `--no-sort-keys` is load-bearing. apm OWNS `.claude/settings.json` and its
|
||||
# `.claude/apm-hooks.json` sidecar (ADR-0018, ADR-0019), and
|
||||
# `apm audit --ci` replays the install and diffs the result byte-for-byte.
|
||||
# apm emits insertion order (`matcher` before `hooks`); the formatter's
|
||||
# default sorts keys, rewrites that into a form apm would never produce,
|
||||
# and the `apm-audit-ci` pre-push hook then reports drift on a file with
|
||||
# no git diff (#102, first hit at 2e395a4). Keeping insertion order means
|
||||
# those two files need no exclude:
|
||||
# tests/test-pretty-json-tool-owned.sh pins that they round-trip
|
||||
# untouched.
|
||||
#
|
||||
# `.claude/settings.json` and its `.claude/apm-hooks.json` ownership
|
||||
# sidecar are the last two alternations, and they are the only ones
|
||||
# here for a reason other than "generated manifest":
|
||||
# apm OWNS that file (ADR-0018, ADR-0019), and
|
||||
# `apm audit --ci` replays the install into a scratch tree and diffs
|
||||
# the result byte-for-byte. `pretty-format-json` sorts object keys
|
||||
# unless `--no-sort-keys` is passed, while apm's hook integrator emits
|
||||
# insertion order (`matcher` before `hooks`, `type` before `command`).
|
||||
# Formatting the file therefore rewrites apm's output into a form apm
|
||||
# would never produce, and the `apm-audit-ci` pre-push hook reports it
|
||||
# as permanent drift on a file with no git diff -- exactly what
|
||||
# happened when the SessionStart hook first landed in 2e395a4.
|
||||
# Re-running `apm install` fixes the file; leaving it in scope here
|
||||
# would re-break it on the very commit that carries the fix. The
|
||||
# sidecar is committed so a fresh clone's install can claim the
|
||||
# settings entry instead of duplicating it (ADR-0019, 2026-09-16
|
||||
# correction), and it is apm output under the same byte-for-byte replay.
|
||||
exclude: '^(\.claude-plugin/marketplace\.json|\.claude/(settings|apm-hooks)\.json)$'
|
||||
# `.claude-plugin/marketplace.json` is the one remaining exclude. It
|
||||
# round-trips except for non-ASCII: it carries literal em dashes and the
|
||||
# formatter re-escapes them to `\u2014` (`--no-ensure-ascii` would fix that,
|
||||
# but it changes the output for every JSON file). Root-anchored because it
|
||||
# is one known path; `check-useless-excludes` fails on a pattern that
|
||||
# matches no file.
|
||||
exclude: '^\.claude-plugin/marketplace\.json$'
|
||||
- id: check-yaml
|
||||
stages: ['pre-commit']
|
||||
- id: trailing-whitespace
|
||||
|
||||
+1
-1
@@ -128,7 +128,7 @@ Widening a description-opener rule to also catch mid-sentence text looked like a
|
||||
|
||||
## 2026-08-14 — A formatter in the commit path manufactures drift on a file with a clean git diff
|
||||
|
||||
`apm audit --ci` failed on `.claude/settings.json` with an empty `git diff` — `pretty-format-json --autofix` silently re-sorts JSON keys, and this generated file was missing from its exclude list, so every commit re-sorted apm's insertion-ordered output before apm compared against it. Separately, a defect introduced 3 hours earlier on the same branch was first mis-described as "pre-existing," an unverified claim about history. Fix: add tool-owned paths to every autofixing hook's exclude the moment ownership is declared, and verify "pre-existing" claims with `git log -S` or `git branch --contains` before writing them down.
|
||||
`apm audit --ci` failed on `.claude/settings.json` with an empty `git diff` — `pretty-format-json --autofix` silently re-sorts JSON keys, and this generated file was missing from its exclude list, so every commit re-sorted apm's insertion-ordered output before apm compared against it. Separately, a defect introduced 3 hours earlier on the same branch was first mis-described as "pre-existing," an unverified claim about history. Fix: add tool-owned paths to every autofixing hook's exclude the moment ownership is declared (for JSON, superseded by #102: `--no-sort-keys` makes the exclude unnecessary, and `tests/test-pretty-json-tool-owned.sh` pins it), and verify "pre-existing" claims with `git log -S` or `git branch --contains` before writing them down.
|
||||
|
||||
## 2026-08-16 — A rule reversed inside a retrofit leaves no trace unless someone writes it down (historical)
|
||||
|
||||
|
||||
+19
-15
@@ -1279,24 +1279,28 @@ point only). Machine-specific settings go in the gitignored `.claude/settings.lo
|
||||
does not deploy and the replay does not compare; shared enforcement belongs in
|
||||
`.pre-commit-config.yaml`.
|
||||
|
||||
### Why it is excluded from `pretty-format-json`
|
||||
|
||||
It is in the **second and last alternation** in that hook's `exclude:` pattern, and that alternation
|
||||
is the only one there for a reason other than "generated manifest". Mind which number you are
|
||||
quoting: the pattern is `^(\.claude-plugin/marketplace\.json|\.claude/(settings|apm-hooks)\.json)$`
|
||||
— **two top-level alternations, expanding to three real tracked files**:
|
||||
`.claude-plugin/marketplace.json`, this one, and its committed `.claude/apm-hooks.json` sidecar,
|
||||
which is apm output under the same byte-for-byte replay and is excluded for the same reason.
|
||||
### Why `pretty-format-json` runs with `--no-sort-keys`
|
||||
|
||||
`pretty-format-json --autofix` sorts object keys unless `--no-sort-keys` is passed, while apm's hook
|
||||
integrator emits insertion order (`matcher` before `hooks`, `type` before `command`). Leaving the
|
||||
file in that hook's scope therefore rewrites apm's output into a form apm would never produce on the
|
||||
way into **every** commit, and `apm-audit-ci` then reports permanent drift on a file with an empty
|
||||
`git diff` — exactly what happened when the `SessionStart` hook first landed in `2e395a4`. Re-running
|
||||
`apm install` fixes the file; leaving it in scope would re-break it on the very commit carrying the
|
||||
fix.
|
||||
integrator emits insertion order (`matcher` before `hooks`, `type` before `command`). In scope with
|
||||
the default, the formatter rewrites apm's output into a form apm would never produce on the way into
|
||||
**every** commit, and `apm-audit-ci` then reports permanent drift on a file with an empty `git diff`
|
||||
— exactly what happened when the `SessionStart` hook first landed in `2e395a4` (#102).
|
||||
|
||||
**Load-bearing. Do not tidy it out of that list** (see `LESSONS.md`, 2026-08-14).
|
||||
The hook now passes `--no-sort-keys`, so this file and its committed `.claude/apm-hooks.json` sidecar
|
||||
(apm output under the same byte-for-byte replay) need **no exclude**: the formatter's default 2-space
|
||||
indent already matches apm's, and with insertion order kept they round-trip untouched.
|
||||
`tests/test-pretty-json-tool-owned.sh` runs the repo's real `pretty-format-json`, `end-of-file-fixer`
|
||||
and `trailing-whitespace` over copies of the four tracked apm-owned files (these two,
|
||||
`.claude-plugin/marketplace.json` and `apm.lock.yaml`) and fails if any is rewritten, so dropping
|
||||
`--no-sort-keys` — or an apm change that makes another autofixer touch its output — fails a test
|
||||
instead of surfacing as drift on an unchanged file.
|
||||
|
||||
**Load-bearing. Do not remove `--no-sort-keys`.** `.claude-plugin/marketplace.json` is the one path
|
||||
still in that hook's `exclude:`: it carries literal em dashes that the formatter re-escapes to
|
||||
`\u2014`, which `--no-ensure-ascii` would stop but for every JSON file. This closes the JSON case
|
||||
only; a new tool-owned file in the scope of another autofixer is still caught only by that test,
|
||||
not by a derived gate.
|
||||
|
||||
## Pushing without a network
|
||||
|
||||
|
||||
Executable
+118
@@ -0,0 +1,118 @@
|
||||
#!/usr/bin/env bash
|
||||
# The autofixing pre-commit hooks must leave apm's own output byte-identical.
|
||||
#
|
||||
# apm owns four tracked files, and `apm audit --ci` diffs them byte-for-byte
|
||||
# against a replayed install. If an autofixing hook rewrites any of them, the
|
||||
# pre-push gate reports drift on a file that has no git diff. This suite runs
|
||||
# the repo's real autofix hooks, with the real args and the real `exclude:` from
|
||||
# .pre-commit-config.yaml, over copies of those four files and asserts nothing
|
||||
# changes. A file the config excludes passes trivially; a file it leaves in scope
|
||||
# must survive the formatter untouched, which is what fails if `--no-sort-keys`
|
||||
# is ever dropped.
|
||||
set -euo pipefail
|
||||
|
||||
REPO_ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)"
|
||||
PASS=0
|
||||
FAIL=0
|
||||
|
||||
pass() { echo " PASS: $1"; PASS=$((PASS + 1)); }
|
||||
fail() { echo " FAIL: $1"; FAIL=$((FAIL + 1)); }
|
||||
|
||||
# exit 77 (automake convention; run-tests.sh renders it as SKIPPED, and
|
||||
# --strict turns that into a setup error at the pre-push gate).
|
||||
command -v git > /dev/null 2>&1 || { echo "SKIP: git is required"; exit 77; }
|
||||
command -v pre-commit > /dev/null 2>&1 || { echo "SKIP: pre-commit is required to run pretty-format-json"; exit 77; }
|
||||
command -v python3 > /dev/null 2>&1 || { echo "SKIP: python3 is required to read .pre-commit-config.yaml"; exit 77; }
|
||||
python3 -c 'import yaml' > /dev/null 2>&1 || { echo "SKIP: PyYAML is required to read .pre-commit-config.yaml"; exit 77; }
|
||||
|
||||
TOOL_OWNED=(
|
||||
.claude/settings.json
|
||||
.claude/apm-hooks.json
|
||||
.claude-plugin/marketplace.json
|
||||
apm.lock.yaml
|
||||
)
|
||||
HOOK_IDS="pretty-format-json,end-of-file-fixer,trailing-whitespace"
|
||||
|
||||
WORK="$(mktemp -d)"
|
||||
trap 'rm -rf "$WORK"' EXIT
|
||||
|
||||
# Derive the fixture config from the real one so the args under test are the
|
||||
# repo's, not a copy that can drift. `stages:` is dropped so the hooks run under
|
||||
# a plain `pre-commit run`.
|
||||
if ! python3 - "$REPO_ROOT/.pre-commit-config.yaml" "$HOOK_IDS" "$WORK/pre-commit-config.yaml" << 'PY'
|
||||
import sys
|
||||
import yaml
|
||||
|
||||
src, ids, dest = sys.argv[1], set(sys.argv[2].split(",")), sys.argv[3]
|
||||
cfg = yaml.safe_load(open(src))
|
||||
repos = []
|
||||
for repo in cfg["repos"]:
|
||||
hooks = [dict(h) for h in repo.get("hooks", []) if h["id"] in ids]
|
||||
if not hooks:
|
||||
continue
|
||||
for h in hooks:
|
||||
h.pop("stages", None)
|
||||
repos.append({"repo": repo["repo"], "rev": repo["rev"], "hooks": hooks})
|
||||
found = {h["id"] for r in repos for h in r["hooks"]}
|
||||
missing = ids - found
|
||||
if missing:
|
||||
sys.exit("hooks not found in .pre-commit-config.yaml: " + ", ".join(sorted(missing)))
|
||||
yaml.safe_dump({"repos": repos}, open(dest, "w"))
|
||||
PY
|
||||
then
|
||||
echo "ERROR: could not derive the fixture config"
|
||||
exit 1
|
||||
fi
|
||||
|
||||
# pre-commit clones a hook repo once and caches it; without the cache this
|
||||
# would need the network, which is a setup problem, not a regression.
|
||||
HOOK_REPO="$(python3 -c 'import yaml,sys; print(yaml.safe_load(open(sys.argv[1]))["repos"][0]["repo"])' "$WORK/pre-commit-config.yaml")"
|
||||
HOOK_REV="$(python3 -c 'import yaml,sys; print(yaml.safe_load(open(sys.argv[1]))["repos"][0]["rev"])' "$WORK/pre-commit-config.yaml")"
|
||||
CACHE_DB="${PRE_COMMIT_HOME:-${XDG_CACHE_HOME:-$HOME/.cache}/pre-commit}/db.db"
|
||||
if ! python3 - "$CACHE_DB" "$HOOK_REPO" "$HOOK_REV" << 'PY' > /dev/null 2>&1
|
||||
import sqlite3, sys
|
||||
db, repo, rev = sys.argv[1:4]
|
||||
row = sqlite3.connect(db).execute("select 1 from repos where repo=? and ref=?", (repo, rev)).fetchone()
|
||||
sys.exit(0 if row else 1)
|
||||
PY
|
||||
then
|
||||
echo "SKIP: $HOOK_REPO@$HOOK_REV is not in the pre-commit cache; run the hooks once with network access"
|
||||
exit 77
|
||||
fi
|
||||
|
||||
mkdir -p "$WORK/repo"
|
||||
cd "$WORK/repo"
|
||||
git init -q .
|
||||
git config user.email [email protected]
|
||||
git config user.name test
|
||||
cp "$WORK/pre-commit-config.yaml" .pre-commit-config.yaml
|
||||
for f in "${TOOL_OWNED[@]}"; do
|
||||
mkdir -p "$(dirname "$f")"
|
||||
cp "$REPO_ROOT/$f" "$f"
|
||||
done
|
||||
git add -A
|
||||
|
||||
echo ""
|
||||
echo "--- autofix hooks leave apm-owned files byte-identical ---"
|
||||
RC=0
|
||||
pre-commit run --all-files > "$WORK/hooks.out" 2>&1 || RC=$?
|
||||
|
||||
for f in "${TOOL_OWNED[@]}"; do
|
||||
if cmp -s "$REPO_ROOT/$f" "$f"; then
|
||||
pass "$f is unchanged by $HOOK_IDS"
|
||||
else
|
||||
fail "$f was rewritten by an autofixing hook -- apm audit --ci would report drift"
|
||||
diff "$REPO_ROOT/$f" "$f" | sed 's/^/ /' | head -20 || true
|
||||
fi
|
||||
done
|
||||
|
||||
if [[ "$RC" -eq 0 ]]; then
|
||||
pass "pre-commit exits 0 over the tool-owned files"
|
||||
else
|
||||
fail "pre-commit exited $RC over the tool-owned files"
|
||||
sed 's/^/ /' "$WORK/hooks.out"
|
||||
fi
|
||||
|
||||
echo ""
|
||||
echo "Results: $PASS passed, $FAIL failed"
|
||||
[[ "$FAIL" -eq 0 ]]
|
||||
Reference in new issue
Block a user