fix(#1095): lint tracked markdown in the pre-push gate, not the whole disk - #1096

Code Review: PR #1096

Commit: f742d1230ada45fa3189fc7afd9c012a1c59c13a

Summary

Replaces the pre-push markdownlint glob ('**/*.md' minus three excluded dirs) with git ls-files '*.md', so the check lints tracked markdown instead of the whole working tree. Adds an empty-set guard. One file, +18/-1, of which 17 lines are comment.

I verified the change end-to-end rather than reading it: the clean-tree run, the discriminating "does a tracked error still fail" test, the markdownlint-cli2-with-no-args claim, and the xargs semantics. The change is correct and the coverage argument holds. Findings below are advisory, plus one pre-merge action.

Pre-merge action — this PR's own commit and body disarm the gate

Running the gate at HEAD does not do what the Testing section says:

$ bash bin/run-pre-push-checks.sh
WARN: pre-push checks bypassed by skip marker in HEAD commit message.
      All skipped checks will still run in CI - fix before merging.
$ echo $?
0

Zero checks ran. The commit body mentions the escape hatch in prose — the paragraph beginning "The gate ships an escape hatch (<!-- pre-push: skip --> in the commit message)" — and detection at line 79 is a plain grep -qF over the whole commit body. So merely describing the marker disables the entire gate. The PR body carries the same literal string in its "Why this is worth fixing" section, which matters because a squash-merge derives the commit message from the PR body: the squash commit landing on dev would carry a live bypass, and the next contributor to publish a branch while it is HEAD gets a silent full skip of markdownlint, shellcheck, and the subpack test.

This is not a regression from this diff — the marker mechanism is untouched — so it is not blocking. But it is worth fixing before merge, and it is a sharp illustration of the PR's own thesis about keeping the hatch meaningful:

1. Before merging, break the literal string in the PR body and the commit body (e.g. write it as pre-push: skip without the HTML-comment delimiters, or split it across a line break). One edit each.
2. Consider a follow-up ticket for the detector: matching the raw string anywhere in the body means the gate cannot be discussed in a commit message without being switched off. Anchoring to a line that is only the marker, or requiring it in the subject, would close it.

Testing item 1 in the PR body should also be corrected — as written it is not reproducible; what it produces at HEAD is a bypass, not a pass.

Checklist Results

• Architecture & Design: Pass
• Code Quality: Pass (bash -n clean, shellcheck --severity=warning clean — both claims verified)
• Testing: Pass — see the verification section; the discriminating property is preserved
• Security: Pass — no secrets, no injection surface; $md_files is quoted throughout
• Performance: Pass — this is the point of the change (~6,900 to 404 files)
• PR Description & Glossary: Pass — Glossary present, Refs #1095 present, single concern
• Summary Bullet Narrative: Pass — all four bullets are what + why, well clear of the label-only heuristic
• Technical Decisions (AgDR): N/A — a bug fix to a local latency check, reversible in one PR; no new dependency, schema, security control, or cross-cutting pattern. Not trust-chain: this is not under .claude/hooks/** or .claude/settings.json, and the script's own header documents CI as the real backstop
• Adopter Handbooks: Pass (one advisory finding below)
• Fallow Static Analysis (JS/TS): N/A — shell diff

Verification performed

The property that mattered most — a genuine markdown error in a tracked file still fails the check — is preserved. The PR body does not claim this test; it is the one it most needed, and it passes.

Is the coverage argument sound?

Yes. I probed for the four holes worth probing:

• Published but untracked — not reachable. A push transmits commits; every file in a commit is in the tree. There is no path by which an untracked file reaches the remote.
• Submodules — none in this repo. In general git ls-files lists the gitlink, not files inside it, so submodule markdown would be missed — but that content is not part of this repo's transfer either, so nothing being published loses coverage.
• Odd filenames — no tracked .md contains a space, newline, or quote (verified). On case: git pathspecs are case-sensitive, so README.MD would be missed — but markdownlint-cli2's **/*.md uses fast-glob, also case-sensitive by default, so it missed the same file. No regression, and no such file exists.
• In the published commits but not in the index — this is the one real gap, and it is narrow. git ls-files reads the index of the current checkout, not the commits being transferred. Publish a branch you are not currently on (an explicit feature:feature refspec while sitting on main) and you lint main's markdown, not that branch's. But the old glob had the identical hole and worse — those files are not on disk either — so this is not a regression, and CI lints the merged tree.

The precise claim, which is what the comment should say, is: no file this transfer publishes loses coverage relative to the old glob. That holds.

Handbook Findings

Commit Message Quality — handbooks/general/commit-message-quality.md (advisory)

• Commit f742d12 subject is fix: lint tracked markdown in the pre-push gate, not the whole disk — no ticket reference. The handbook's required-elements table specifies type(#TICKET): description for the subject line. The PR title has fix(#1095):; the commit subject does not. Minor, and the body is otherwise a model example of the handbook's WHY-not-WHAT recipe (problem, trigger, trade-offs, Refs #1095).

Nothing to flag from handbooks/architecture/clean-architecture-layers.md (shell script) or handbooks/architecture/migration-safety.md (blocking, but no migration in this diff).

Suggestions

1. xargs is not NUL-safe here — and the mirror it must stay in sync with already is. (bin/run-pre-push-checks.sh:144)

The script header at lines 12-14 states: "If you add a check, add it here AND mirror it in .claude/project-config.example.json -> pre_push.commands so both paths stay in sync." That example — tracked since 8822d05 (#1065) — already uses the safe form:

echo "$md_files" | tr '\n' '\0' | xargs -0 npx --yes markdownlint-cli2

This PR uses the plain form: printf '%s\n' "$md_files" | xargs npx --yes markdownlint-cli2. Plain xargs word-splits on whitespace and interprets quotes — I confirmed a path containing an apostrophe produces xargs: unterminated quote, a hard failure with an error that gives no hint about the real cause, and a path with a space silently becomes two arguments. No tracked .md has such a character today, so there is no live bug — but the two paths that are documented as mirrors now differ in robustness, and the safer construction already exists one file over. Adopting tr '\n' '\0' | xargs -0 makes them match and costs nothing.

(The INFO: skip text also differs slightly between the two — 'no tracked markdown' here vs 'no tracked markdown files found' in the example. Cosmetic, but same sync argument.)

2. The empty-set guard comment is factually wrong; the guard itself is right. (bin/run-pre-push-checks.sh:141-143)

The comment says invoking with no arguments "would lint the default glob and reintroduce the bug." Tested against this repo: npx --yes markdownlint-cli2 with no arguments prints its usage text and exits 2. It lints nothing. The repo's config is .markdownlint.json — a rules-only format that cannot carry a globs key (and a grep for a globs key finds none), so there is no default-glob source to fall back to.

So the guard is still correct and still necessary, but for the opposite reason to the one stated: without it, a repo with no tracked markdown would hard-fail the gate with a usage dump, not silently over-lint. Worth correcting, because the current comment would mislead the next person deciding whether the guard can be dropped.

Related: the first line of that block, `xargs -r` (GNU) / no-op on empty input, references a flag the code does not use. Reads as a leftover from an earlier draft.

3. Consider a one-line note that git ls-files reads the index of the current checkout, so the check covers the checked-out branch rather than an arbitrary published ref. It is not a regression (the old glob was strictly worse), but it is the kind of thing the next reader will wonder about given how carefully the rest of the comment reasons about coverage.

Issues Found

None blocking. The diff does what it claims, the coverage argument survives scrutiny, and the check still catches real errors in tracked files.

Verdict

APPROVED

The change is correct, the reasoning behind it holds up to probing, and the property it must not lose is intact — I confirmed a tracked-file error still fails the check, which the PR body did not claim. Please action the pre-merge item at the top (strip the literal marker string from the PR body and commit body) before squash-merging, so the squash commit does not land a live bypass on dev; the three suggestions are advisory and fine as follow-ups.

Reviewed by Rex (Code Reviewer Agent)
Reviewed commit: f742d1230ada45fa3189fc7afd9c012a1c59c13a

Code Review: PR #1096 (re-review)

Commit: e56dc0992ff535c67c71c087cef590fb7f6ae599
Previous review: f742d123 — APPROVED with one significant finding + three advisory suggestions. All four acted on.

Summary

Scopes the pre-push markdownlint check to tracked files via git ls-files instead of a working-tree glob, restoring the form the mirror in .claude/project-config.example.json already uses. Single file, +23/-7, one concern.

I re-verified the three claims rather than reading them. All three hold.

Checklist Results

• Architecture & Design: Pass
• Code Quality: Pass
• Testing: Pass
• Security: Pass
• Performance: Pass (6,900 → 404 files linted)
• PR Description & Glossary: Pass
• Summary Bullet Narrative: Pass (all four bullets carry what + why)
• Technical Decisions (AgDR): N/A — a sync to an existing documented pattern, reversible in one PR; no new dependency, no schema, no trust-chain path
• Adopter Handbooks: Pass (general/commit-message-quality.md loaded; commit body is well above the bar)
• Fallow Static Analysis: N/A (not JS/TS)

Verification of the three claims

1. The gate no longer disables itself — confirmed.

skip marker in HEAD commit message : 0 occurrences
skip marker in PR body             : 0 occurrences

Ran the gate at PR head in a detached worktree:

pre-push checks:
  running: markdownlint
  running: shellcheck
  running: subpacks
  all checks passed.
GATE_RC=0    bypass warnings: 0    checks run: 3

Three checks actually executed and the WARN: pre-push checks bypassed line is absent. This is the real fix — my headline finding at f742d123 was that the gate reported itself bypassed, and it now genuinely runs. Filing the unanchored grep -qF as #1097 with grep -qxF is the right split; that detector bug is not this diff's regression.

2. Mirror parity — confirmed, byte-identical.

Evaluated MARKDOWNLINT_CMD from the script at e56dc09 and compared with cmp against the mirror's .run value:

script : 399 bytes / 395 chars
mirror : 399 bytes / 395 chars
cmp    : byte-identical

The stated "395 chars" is the character count; the byte count is 399 because the string carries two em-dashes (3 bytes each in UTF-8). Both figures are right — no discrepancy. The "copied verbatim" comment is now load-bearing and is backed by the artifact.

Provenance also checks out: 8822d05 is fix(#1031): untrack project-config.json and ship a tracked example template (#1065), and the mirror it created carries git ls-files from inception. The header's sync mandate is real (lines 12–14).

3. The corrected no-arg comment — confirmed accurate.

$ npx --yes markdownlint-cli2      # no arguments
markdownlint-cli2 v0.23.2 (markdownlint v0.41.1)
Syntax: markdownlint-cli2 glob0 [glob1] ...
rc=2

Usage banner, nothing linted, non-zero exit. And .markdownlint.json contains no globs key — correct, since globs is a markdownlint-cli2 config option that the plain rules-only format cannot carry. The replacement comment ("prints usage and lints nothing", guard justified by "an empty repo would otherwise produce a confusing non-zero") matches observed behaviour exactly, and the stale xargs -r reference is gone.

Discriminating test re-confirmed (cheap in an isolated worktree, so I re-ran it rather than take it on report): a deliberate MD018 appended to tracked README.md yields rc=1 with FAILED: markdownlint. The property the change must not lose is intact.

Issues Found

None blocking.

Handbook Findings

None. handbooks/general/commit-message-quality.md is advisory and this commit clears it comfortably — descriptive subject, sectioned body carrying the WHY, an explicit corrections-from-review section, and a verified section. No violation.

Suggestions

1. nit: — one sentence in the new comment over-claims, and it is the history, not the behaviour.

# this one had drifted back to a filesystem glob.

The script never had the tracked-files form to drift back from. Verified:

$ git log --oneline --all -S'git ls-files' -- bin/run-pre-push-checks.sh
e56dc09   # this PR, and nothing before it

The tracked-files form entered the config path at bcd06f0 (#596) and survived into the example mirror at 8822d05; the script simply never followed. So this is an unclosed divergence, not a regression — "drifted back" implies a state it once held and lost. The same wording appears in the PR body's third Summary bullet.

Everything the sentence is doing still stands (the two are out of sync, the header mandates sync, the mirror's form is the correct one), so this is wording only. Suggested: "the script never followed" or "this one had never been synced". Worth correcting precisely because of the failure mode you named — a plausible historical narrative that no one re-checks is how six false claims accumulated over nine rounds on that AgDR.

2. nit: — "no longer quoted anywhere in this branch" is broader than what is true.

The PR body says the marker "is no longer quoted anywhere in this branch." The literal string still exists at bin/run-pre-push-checks.sh:77 — it is the SKIP_MARKER definition, and necessarily so. The claim that matters (and that I verified) is about the commit message and PR body, where the detector actually looks. Suggested: "anywhere in this branch's commit message or PR body."

On comment length — I checked, and it mostly earns its place.

You asked whether the block over-claims given it is longer than the code. Reading it sentence by sentence: the sync mandate, the coverage argument, the git ls-files-reads-current-index limit, and the guard rationale are each a direct answer to a review finding, and each states something a future editor would otherwise have to re-derive or get wrong. The 404-tracked figure is exact (I counted). The ~6,900 is appropriately hedged. The NUL-safety sentence is precise — plain xargs does fail on both a space and an apostrophe.

The only sentence reaching past the code is the "drifted back" one above. The one candidate for cutting on redundancy rather than accuracy is "Copy the mirror's command verbatim rather than inventing a variant — two implementations of the same command is how they diverge", which appears in the comment, the commit body, and the PR body. It is true in all three; it only needs to be in the code. Optional.

Verdict

APPROVED

The bypass is genuinely gone — verified by running the gate, not by reading the commit. Parity is byte-identical against the artifact. The corrected comment matches observed tool behaviour. The two nits are wording-only and non-blocking; fix them in this branch if convenient, or let them ride.

Reviewed by Rex (Code Reviewer Agent)
Reviewed commit: e56dc0992ff535c67c71c087cef590fb7f6ae599

Code Review: PR #1096 (re-review)

Commit: ad2268063748222db374da90066c4c128c0ed8a8
Previously approved at: e56dc099 — scope of this pass limited to the delta.

Summary

Follow-up to my two wording nits. Both are fixed, and the delta is genuinely
comment-only. But the correction to my correction introduced a new, smaller
false claim — one word, and the commit the sentence cites is what disproves it.

Delta verification

git diff e56dc099 ad226806 --stat → bin/run-pre-push-checks.sh | 10 +++++-----,
a single contiguous comment block (lines 130–134). No executable line changed.
MARKDOWNLINT_CMD, the empty guard, the NUL-delimiting, and SKIP_MARKER are
byte-for-byte unchanged. Per the brief I did not re-run the hook tests, the MD018
discriminating test, or the no-arg probe.

Nit 1 (my headline) — fixed, and the replacement is true

I re-ran the check myself rather than taking the correction on trust:

$ git log --all -S'git ls-files' --oneline -- bin/run-pre-push-checks.sh
ad22680 fix(#1095): lint tracked markdown in the pre-push gate, not the whole disk

Only this branch. The script never held the tracked-files form, so it could not
have "drifted back" from it. The replacement — "while this script never adopted
it" — is accurate. The false-history claim is gone.

Nit 2 — fixed

SKIP_MARKER is correctly scoped to "the commit body or this PR body — the only
places the detector reads," and the definition site is named. Verified it still
exists at bin/run-pre-push-checks.sh:77, read at :79, emitted at :184.

Nit 3 (redundancy) — fixed

The "copy verbatim" line now appears only in the code comment.

Mirror parity — still holds after the adjacent edit

Evaluated the shell assignment and the JSON pre_push.commands[0].run and
compared:

PARITY: byte-identical
--- len script=395 mirror=395

The edit did not disturb the assignment. The header's sync requirement and its
pre_push.commands key path both check out against the actual file.

⚠ New finding — "long-standing" is false, disproved by its own citation

The replacement sentence opens:

Closes a long-standing divergence from the mirror in
.claude/project-config.example.json, which has used the tracked-files form
since #1065 (8822d05) …

The cited commit dates the claim, and it does not survive it:

$ git log -1 --format='%h %ad' --date=short 8822d05
8822d05 2026-07-30
$ git log -1 --format='%h %ad' --date=short ad226806
ad22680 2026-07-31
$ git rev-list --count 8822d05..origin/main
2

The divergence is one day and two commits old. Stronger still — 8822d05 is
the commit that created the mirror file:

$ git log --diff-filter=A --format='%h %ad %s' --date=short -- .claude/project-config.example.json
8822d05 2026-07-30 fix(#1031): untrack project-config.json and ship a tracked example template (#1065)

A divergence cannot predate the file it diverges from. "Long-standing" is not a
soft overstatement here; it is contradicted by the same SHA the sentence uses as
evidence.

This is the same defect class as the round-one finding — a narrative that reaches
past the facts — which is exactly the accumulation pattern AgDR-0113 documents.

Fix: delete one word. Closes a divergence from the mirror … is accurate,
and the "since #1065 (8822d05)" clause already gives the reader the real
timeline without editorialising it.

On length (you invited the cut)

The replacement paragraph is 5 lines carrying about 2 lines of information. With
the word removed it compresses cleanly:

# Closes a divergence from the mirror in .claude/project-config.example.json,
# which has used the tracked-files form since #1065 (8822d05) while this script
# never adopted it. The header above requires the two to stay in sync; the
# command here is copied from the mirror verbatim rather than reimplemented.

Same facts, 4 lines, no adjective doing unearned work.

Checklist Results

• Architecture & Design: N/A (comment-only delta)
• Code Quality: Pass — no executable change
• Testing: N/A — verified at e56dc099, nothing executable changed
• Security: Pass — trust-chain-adjacent script, but the gate logic is untouched
• Performance: N/A
• PR Description & Glossary: Pass
• Summary Bullet Narrative: Pass — bullets carry the git log -S evidence inline
• Technical Decisions (AgDR): N/A
• Adopter Handbooks: N/A — no handbook dirs present
• Fallow Static Analysis: N/A — not a JS/TS diff

Verdict

APPROVED

The executable change was verified correct at e56dc099 and is untouched here;
the two nits are genuinely resolved. I am not blocking on a single adjective in
a code comment — that would be disproportionate to a Lean-tier, zero-behaviour
delta.

But I would cut long-standing before merging. It costs one word, and the
alternative is carrying a claim that the cited SHA refutes into a file whose
whole purpose is to be the canonical reference. Your call: amend (which moves
the SHA and needs a trivial re-mark) or merge as-is knowingly.

🤖 Reviewed by Rex (Code Reviewer Agent)
📌 Reviewed commit: ad2268063748222db374da90066c4c128c0ed8a8

Code Review: PR #1096 (re-mark)

Commit: 819153e120d3652241c3ac65affd6dde47eeee1f
Prior approval: ad226806 — this pass covers only the delta.

Delta

git diff ad226806 819153e → bin/run-pre-push-checks.sh | 8 +++-----, one
comment block, 5 lines to 3. No executable line changed — the
MARKDOWNLINT_CMD assignment is not in the hunk, so mirror parity is unchanged
by construction rather than by re-verification. Per the brief I re-ran nothing.

The finding is closed

long-standing is gone, and the replacement states the timeline instead of
characterising it. Every remaining sentence is a bare fact already verified in
earlier rounds:

• "has used this form since 8822d05 (#1065)" — confirmed via
  git log --diff-filter=A -- .claude/project-config.example.json
• "this script never adopted it" — confirmed via
  git log --all -S'git ls-files' -- bin/run-pre-push-checks.sh (returns this
  branch only)
• "the header above requires the two to stay in sync" — confirmed against the
  header's own mirror instruction and its pre_push.commands key path
• "Copied from the mirror verbatim" — the assignment is untouched

No claim reaches past the code. Nothing further to cut.

Verdict

APPROVED

One note, not a request for work

Three rounds on a one-line change, all three spent on comment prose rather than
behaviour. Your read is the right one: the fix is to write less, not to correct
more. Worth recording that the shortest version — 3 lines, no adjectives — is
also the only one that has been accurate on the first pass. That is the reusable
lesson here, and it matches AgDR-0113.

🤖 Reviewed by Rex (Code Reviewer Agent)
📌 Reviewed commit: 819153e120d3652241c3ac65affd6dde47eeee1f