docs(hooks): align pre-commit guide with the active gate - #462
Conversation
vladimirrott
left a comment
There was a problem hiding this comment.
Approving at eb7bd568.
The guard is the part I want to single out. #430 asked for a check that bites when the hook and the prose disagree, and you built one that fails closed in every direction I pushed it. Deriving the command list from .githooks/pre-commit by filtering out #, set , cd and echo lines, rather than hard-coding five strings in Python, is what makes it survive the next hook edit.
What I ran
All of it in rootless podman against a detached worktree at eb7bd568, never on the host: maintainer screen 462 returns DO NOT EXECUTE for any PR carrying .py or .sh.
First, the suite you could not run on macOS Bash 3.2:
$ out="$(podman run --rm --network=none -v /tmp/r21p462:/repo:z -w /repo docker.io/library/python:3.12 bash tests/release/public-claims.test.sh 2>&1)"; rc=$?; echo "PR462 public-claims rc=$rc"; printf '%s\n' "$out" | grep -v level=warning | tail -25
PR462 public-claims rc=0
Published figures match the evidence artifacts.
Public claims are internally consistent.
Public claims contract passed.
Then four mutations, each running the checker directly so the exit code is the checker's own rather than a pipeline's.
A sixth step appended to the real .githooks/pre-commit:
$ printf 'node --version\n' >> .githooks/pre-commit
rc=1
Claims that do not derive from evidence:
- docs/developer-guide.md: pre-commit steps differ from .githooks/pre-commit; expected ['scripts/check_no_secrets.sh --staged', 'cargo fmt --all --check', 'cargo clippy --workspace --all-features --all-targets --locked -- -D warnings', 'RUSTDOCFLAGS="-D warnings" cargo doc --no-deps --workspace --locked', 'scripts/test_baseline.sh', 'node --version']; documented ['scripts/check_no_secrets.sh --staged', 'cargo fmt --all --check', 'cargo clippy --workspace --all-features --all-targets --locked -- -D warnings', 'RUSTDOCFLAGS="-D warnings" cargo doc --no-deps --workspace --locked', 'scripts/test_baseline.sh']
A documented step deleted from the hook:
$ sed -i '/^scripts\/test_baseline.sh$/d' .githooks/pre-commit
rc=1
Claims that do not derive from evidence:
- docs/developer-guide.md: pre-commit steps differ from .githooks/pre-commit; expected ['scripts/check_no_secrets.sh --staged', 'cargo fmt --all --check', 'cargo clippy --workspace --all-features --all-targets --locked -- -D warnings', 'RUSTDOCFLAGS="-D warnings" cargo doc --no-deps --workspace --locked']; documented ['scripts/check_no_secrets.sh --staged', 'cargo fmt --all --check', 'cargo clippy --workspace --all-features --all-targets --locked -- -D warnings', 'RUSTDOCFLAGS="-D warnings" cargo doc --no-deps --workspace --locked', 'scripts/test_baseline.sh']
The hook file deleted, which is where a guard goes quiet and reports having looked:
$ mv .githooks/pre-commit /tmp/hook462.moved
rc=1
Evidence check could not run: .githooks/pre-commit is missing
The heading renamed from ## Pre-commit Hooks to ## Pre-commit hooks, so the anchor your parser splits on stops existing:
$ sed -i 's/^## Pre-commit Hooks$/## Pre-commit hooks/' docs/developer-guide.md
rc=1
Claims that do not derive from evidence:
- docs/developer-guide.md: pre-commit steps differ from .githooks/pre-commit; expected ['scripts/check_no_secrets.sh --staged', 'cargo fmt --all --check', 'cargo clippy --workspace --all-features --all-targets --locked -- -D warnings', 'RUSTDOCFLAGS="-D warnings" cargo doc --no-deps --workspace --locked', 'scripts/test_baseline.sh']; documented []
documented [] and a red exit, rather than a pass, is the answer I was hoping for there.
Last, the one this repo has been burned by twice. Revert the production hunk, re-run the PR's own test:
$ sed -i '/problems += check_pre_commit_commands(root, texts\["docs\/developer-guide.md"\])/d' scripts/check_evidence_claims.py
$ out="$(podman run --rm --network=none -v /tmp/r21p462:/repo:z -w /repo docker.io/library/python:3.12 bash tests/release/public-claims.test.sh 2>&1)"; rc=$?; echo "REVERT-CALLSITE rc=$rc"; printf '%s\n' "$out" | grep -v level=warning | tail -6
REVERT-CALLSITE rc=1
Published figures match the evidence artifacts.
Public claims are internally consistent.
FAIL: checker accepted stale claim: undocumented sixth hook step
Red with the fix removed. That is the bar here, and you cleared it.
shellcheck --severity=warning over tests/e2e tests/release scripts assets/demo exits 0 at eb7bd568, and your board is green on all eleven checks, markdownlint inside docs-and-hygiene included.
One thing outside your scope
docs/contributing/CONTRIBUTING.md still carries the instruction you removed from the developer guide:
$ sed -n '128,129p' docs/contributing/CONTRIBUTING.md
# All pre-commit hooks
pre-commit run --all-files
That page ships in the book (docs/SUMMARY.md:47), so a reader who lands there installs the framework, writes into .git/hooks, and reaches the no-gate outcome #430 describes. Your checker reads docs/developer-guide.md only, so it will not catch it.
I do not want that change in this PR. #430 named one file and you fixed that file. I will open it separately and hold it for you for a week, since you have already done the reading. Say the word either way and I will not hold it past then.
Merging
Taking this through the merge gate now. Nothing outstanding on your side.
Thanks for naming the command you could not run instead of estimating its result. That habit is what makes the rest of a validation section worth reading.
You run Bash 3.2 on macOS and care about hook mechanics in a shared clone, which tells me you administer machines you do not want surprising you. SysKnife's daemon is Linux-only, but the CLI's read-only half works over a forwarded socket, so if you keep a Linux box or a VM, sysknife plan and sysknife audit verify are the harmless end to try first. Optional, and no bearing on this merge.
|
Thanks for the thorough review and for offering me the follow-up. I appreciate it, but I’ll pass on the additional documentation issue this time. Please feel free to make it available to other contributors rather than holding it for me. Thanks again for your help getting #462 merged! |
|
Understood, and thank you for saying so plainly instead of leaving it open. The hold is off and I have not assigned it to anyone. It is filed as #464, and writing it up turned it into a slightly bigger thing than the one line I quoted you. The book ships Your checker reads That is not a gap you introduced. #430 named one file and you covered it, and whoever takes #464 will copy the assertion shape you wrote at Nothing outstanding on your side. #462 is merged and the pre-commit section of the developer guide now matches the hook that runs. |
Correct the developer guide to describe the active core.hooksPath-based hook and its commands in order. Remove the obsolete framework instructions and exclusion claims. Extend the existing evidence checker and public-claims fixture to reject hook/prose drift, including an added or removed step.
Validation: focused checker verification and added/removed-step mutations passed; bash -n and git diff --check passed. The full claim screen could not run on this machine because its existing story metadata runner requires associative arrays unavailable in macOS Bash 3.2. No Rust source, test baseline, hook behavior, or CI workflow changed.
AI assistance: OpenAI Codex assisted with these changes and focused verification.
Closes #430