fix(markdown-format): stop reading a missing git as "file is out of tree" - #2121
Conversation
…ree" The working-tree membership scope added in 0.6.3 (#1030) probes with `git rev-parse --show-toplevel`, which fails identically when the file is outside every working tree and when git is not installed. On a POSIX host without git that read every Markdown edit as out-of-tree, so the hook skipped all of them — silently, repo-wide, with jq and markdownlint-cli2 present and a markdownlint config in place. Git is not a documented prerequisite of this hook: README "Requirements" lists Bash, jq and markdownlint-cli2, the setup skill checks those, and hook::repo_root has always tolerated git being absent. Gate the skip on git being available, so an undecidable verdict lints instead of skipping — the direction file_is_gitignored already documents for the same input. The scope is unchanged wherever git can answer, and the fail-closed symlink-escape check ahead of it is untouched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ff22dcbc86
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… is absent
Merges origin/main (version collision 0.10.2 vs main's 0.11.0, resolved to
0.11.1) and closes the review finding on this PR, which was correct.
Gating the membership skip was not enough on its own. Config discovery walks UP
from the edited file and stops at hook::repo_root, which without git returns
the hint it was given — the file's own directory. Root and start were the same
directory, the walk terminated immediately, and a repository whose markdownlint
config sits at its root stopped linting everything below it. Reproduced:
git PRESENT: root-level.md LINTED docs/nested.md LINTED
git ABSENT: root-level.md LINTED docs/nested.md SKIPPED
CLAUDE_PROJECT_DIR answers the same question without git and is now preferred
as the walk's terminator when the git probe cannot resolve a working-tree top.
Terminator only, never a scope widening — discovery still starts at the file
and still stops at a root — so markdownlint_config_discoverable's fail-closed
reasoning is unchanged.
The capability is probed by running `git rev-parse --show-toplevel`, not by
testing `command -v git`. A first attempt used `command -v` and silently did
nothing: the suite's git-absence shim is an exported bash FUNCTION, which
`command -v git` reports as present. `command -v` tests for a NAME, not a
capability, and answers yes for a function, a PATH stub, and a real binary
standing in a directory that is no repository alike — every case where this
fallback still applies.
The reviewer's point about the existing control was exact: it passes only
because its fixture sits beside the root config. The new case nests the
fixture, which is the whole assertion; a root-level twin passes either way.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 4m 54s —— View job Code review — PR #2121
Scope reviewed: the 5 changed files ( SummaryBoth fixes in this PR do what they claim:
One consistency finding posted inline: the new fallback probe at Nothing else stood out:
Note: the PR description covers only the first commit's fix; it predates the second commit that addresses the Codex review comment, so it doesn't mention the nested-config-discovery fix or its test. Not a blocker, just worth folding in if the description gets updated. |
|
Claude finished @kyle-sexton's task in 2m 19s —— View job Security review complete ✅
Scope: the 5 files changed in this PR ( No security issues found. Summary of what I checked in the two behavior-changing hunks:
Other notes:
The one review comment already on this PR (Codex, on |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Claude has reviewed this PR 4 times. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
… probe
The two sibling probes in this file — in_git_working_tree and
file_is_gitignored — both unset GIT_DIR, GIT_WORK_TREE, GIT_COMMON_DIR,
GIT_CEILING_DIRECTORIES and GIT_DISCOVERY_ACROSS_FILESYSTEM before asking git
anything, because an inherited GIT_DIR or GIT_WORK_TREE from whatever launched
the session lets some OTHER repository answer the question. The new toplevel
probe asks the structurally identical question and did not.
Here a spurious SUCCESS is the harmful direction: it withholds the
CLAUDE_PROJECT_DIR fallback and leaves config discovery terminating at the
edited file's own directory — precisely the bug the surrounding block exists to
fix. Cleared in a subshell so the surrounding process keeps its environment.
The nested reproduction still passes on both sides:
git PRESENT: root-level.md LINTED docs/nested.md LINTED
git ABSENT: root-level.md LINTED docs/nested.md LINTED
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 4m 18s —— View job Code review — PR #2121
Scope reviewed: the 5 files changed in this PR, at HEAD The previously-flagged env-clearing gap is fixedCommit One test-coverage finding (posted inline)
Everything else
|
|
Claude finished @kyle-sexton's task in 2m 22s —— View job Security review complete
Scope: the 5 files changed in this PR ( No security issues found. I re-verified the two behavior-changing hunks independently of the two automated reviews already on this PR:
Other checks, no findings:
One residual behavior worth noting for completeness, not filed as a finding: the membership-skip gate at L196 still uses |
…and fix a typo Two things, both from review of this PR. The override added here had NO coverage for the case it executes on nearly every real Markdown edit — CLAUDE_PROJECT_DIR set AND git present. Every other case in this file runs either `-u CLAUDE_PROJECT_DIR` or under the git-absence shim, so the ordinary path was invisible to the whole suite. Both halves are needed, because "it still lints" cannot distinguish a correctly-inert override from one that fired and happened not to change the outcome: - positive: git present, CLAUDE_PROJECT_DIR set, a nested .md still lints. - negative, and the load-bearing one: CLAUDE_PROJECT_DIR pointed at a directory ABOVE the repository root carrying its own markdownlint config. With git present the walk must stop at the git toplevel, so that config must NOT be reached. A wrongly-firing override widens discovery past the repo root and lints the file, which this case makes observable. Also fixes the typos gate: `coreutil` -> `coreutils` in a comment. That was the real cause of the red `ci-status` — every hygiene sub-check the aggregate printed said success, and `typos=failure` sat further up the log. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 5m 10s —— View job Code review — PR #2121 (new commit
|
|
Claude finished @kyle-sexton's task in 1m 12s —— View job Security review complete
Scope: the 5 files changed in this PR, at HEAD No security issues found. This run was triggered by the sync to commit
The new test fixtures (temp git repos under
|
|
Offering a branch rather than pushing to yours — Branch:
I started this before 1. A defect report, which stands whether or not you take any of the aboveThe guard Measured with the PATH-shim technique your own telemetry spawn-budget tests use (a
I have to correct my own first framing of this, because I measured it and was wrong: it is not unconditional, and it is not the ~140 ms Git Bash cost I expected. The comparison is between a git-produced path and a shell-produced one, and on Windows Git Bash those never match — This is the same guard as the two open P2 coverage threads ( 2. A design proposal, which is optional and larger than the two remaining gaps strictly requireTwo gaps are still open at (a) if [[ -z "${CLAUDE_PROJECT_DIR:-}" ]]; thenand your primary no-git fixture runs unset via (b) The opt-in pre-check at My branch replaces the guard with Deleting the guard also dissolves both P2 threads: there is no path-form equality left to be true-at-the-root and false-when-nested, and the The option I would not choose, since you should have it too: both gaps can be closed far more narrowly — extend your existing condition to fall back to 3. EvidenceSame host, same shell, No pre-existing failure at any point; the only skip is Also clean on every file I touched: 4. What I did not do, and one thing I could not verify
Unverified: I could not observe the POSIX-host spawn count on a real POSIX host. The I also saw one unexplained |
…ch, drop the inert twin Both cases added in f87ac6a were inert, for two independent reasons, and both passed anyway — the exact defect they were added to close. The guard is `"$REPO_ROOT" == "$(dirname "$FILE")"`. Both fixtures were NESTED, so with git resolving a toplevel dirname is `<repo>/docs` while REPO_ROOT is `<repo>`; they never match, the whole `if` short-circuits, and the override branch never executed. Separately, the outer config was `{}`, leaving MD004 on "consistent", which has nothing to flag on a single-item list — so even a wrongly-widened walk produced byte-identical output and the negative could not have failed either. The positive is rewritten to reach the branch and to be load-bearing: git present and working, but the file sits at the root of a directory that is NOT a repository, so hook::repo_root falls back to the hint, the guard matches, the probe fails, and CLAUDE_PROJECT_DIR terminates the walk. Proven discriminating against the pre-override hook: POSITIVE (dir is not a repo): PRE=SKIPPED POST=LINTED NEGATIVE (dir is a repo): PRE=SKIPPED POST=SKIPPED The outer config now pins MD004 to "dash" so a wrong walk is observable. The negative twin is dropped rather than shipped, because three instruments were tried and none can fail — and the reason is a fact about the code, not the fixtures. Forcing the override to fire (probe replaced by `false`) still rewrites nothing, because markdownlint-cli2 does its own config discovery and does not cross the repository boundary. Telemetry's data.file is derived from REPO_ROOT but emits empty under the forced override, which is indistinguishable from a sink that never populated. Exit status is 0 either way. The reasoning is recorded in the test file so it is not re-derived. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 3m 47s —— View job Code review — PR #2121 (new commit
|
|
Claude finished @kyle-sexton's task in 46s —— View job Security review complete
Scope: the 5 files changed in this PR, at HEAD No security issues found. The only file touched since the last security pass ( Commit
All of this is test-only: paths are built from Restating from the prior passes on this PR, since the reviewed logic itself is unchanged: the |
…of 0.11.1 The earlier form of this change was cut against the #2121 branch before it merged, so it edited the then-unreleased 0.11.1 entry in place. 0.11.1 has since shipped with the CLAUDE_PROJECT_DIR mechanism, and rewriting its entry would misstate what that version did. Restores 0.11.1 verbatim and describes this change as 0.11.2, with the manifest bumped to match — the plugin version is the update cache key, so an unbumped plugin never delivers the change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…t REPO_ROOT read (#2128) No linked issue ## Summary Test-only follow-up to #2121. That PR shipped a comment asserting the `REPO_ROOT` override's *inertness* was **not behaviourally observable**, and left the negative case unwritten on that basis. The assertion was wrong. A fourth instrument exists, it works, and the negative is now written. The claim mattered beyond the missing case: a comment saying a thing cannot be observed tells the next maintainer to stop looking. ## Why three instruments failed All three try to observe an **effect** of `REPO_ROOT`: 1. **Lint output.** Force the override to fire on a file at a real repository root — replace the probe with `false` — and the file is still not rewritten. `markdownlint-cli2` performs its own config discovery and does not cross the repository boundary, so widening the hook's gate changes no observable byte. 2. **Telemetry `data.file`**, which is derived from `REPO_ROOT` and looked like the obvious answer. The forced-override run emits an **empty** value rather than a relative-to-outer path — and empty is also what a sink that never populated looks like, so the assertion could not separate a regression from a flaky sink. 3. **Exit status** is 0 either way. ## The fourth instrument reads `REPO_ROOT` directly The hook resolves a repo-local linter at `"$REPO_ROOT/node_modules/.bin/markdownlint-cli2"`. Plant a distinguishable shim at **both** candidate roots and whichever one runs names the root the hook actually computed. That is a read of the variable from outside the process, not an inference. ``` POST: negative (file dir IS a repo) -> INNER positive (file dir is NOT a repo) -> OUTER ``` **Control for the negative** — a hook whose probe is forced to `false`, so the override always fires: ``` correct hook -> INNER forced-override hook -> no marker ``` The assertion is **positive**: the marker must read `INNER`. A wrongly-firing override produces `OUTER` or no marker at all, and both fail it. ### Two mechanics that silently defeat this Recorded in the test file, because each one makes the instrument look like a dead end: - **The `PATH` copy of `markdownlint-cli2` wins** over the repo-local one, so the shim never runs while the real binary is reachable. The case strips only the directories carrying it, leaving `jq` and `git` on `PATH` — remove those and the hook exits early for unrelated reasons. - **The shim cannot announce itself on stdout or stderr.** The hook captures both into a variable, so anything printed is swallowed. It must write a **marker file**. ## Tests ``` ok: git present, dir is no repo: the override fires and CLAUDE_PROJECT_DIR terminates the walk ok: git present: the override stays inert — the hook resolved REPO_ROOT to the git toplevel ``` The unusable-environment branch emits a **visible** `ok` rather than passing over in silence — this suite has no skip helper and sources none, and a silent omission is exactly what `scripts/check-silent-skips.sh` exists to catch. ## Credit and provenance The instrument was found by the session that wrote the guardrails work on #2100, after I concluded the negative was unwritable. I reproduced it independently before building on it, including the forced-override control above. Worth recording alongside it: while testing this, that session hit the same defect the reviewer found in #2121's first attempt — fixtures built under a Windows 8.3 shortname (`C:/Users/KYLESE~1/…`) while `hook::repo_root` returns the long form, so the guard `"$REPO_ROOT" == "$(dirname "$FILE")"` compared two spellings of one directory and was always false. The branch never executed and the output looked plausible throughout. Same lesson as the rest of this sequence: **prove the fixture reached the path under test.** ## Related - #2121 — where the override landed and where the incorrect comment shipped - #1938 — the stranded post-merge review-findings sweep --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
… rewrite of 0.11.1 The earlier form of this change was cut against the #2121 branch before it merged, so it edited the then-unreleased entry in place. That version has since shipped, and rewriting a released entry would misstate what it did. Restores 0.11.1 verbatim and gives this change its own entry, with the manifest bumped to match — the plugin version is the update cache key, so an unbumped plugin never delivers the change. The number is whatever sits above the highest released entry at rebase time; main has taken several while this branch was in review, so it is deliberately not restated here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…sted files reach the root config without git (#2130) Follow-up to #2121. **Both gaps are live on `main` right now** — not stale review findings. Reproduced independently: the two new tests, run against `main` own unmodified hook, give **PASS=136 FAIL=2**. With the change, **138/0**. ## The two defects **1. `markdown-format.sh:119` calls `hook::repo_root` raw.** With `git` and `jq` both absent, a nested file makes the opt-in pre-check read an opted-in repo as opted-out, and the `jq` notice is swallowed. A repository that did opt in is treated as if it had not, silently. **2. The `REPO_ROOT` guard at `229-237` covers only the `CLAUDE_PROJECT_DIR`-set case.** The membership scope it exists to fix is gated on that variable being **unset**, and the no-git fixture runs unset — so the configuration the fix was written for is still broken for nested files. `hook::repo_root` falls back to the file own directory, the root markdownlint config is never discovered, and the edit is skipped with no diagnostic. The second is the one #2121 review comment described as "leaving the normal nested-docs case unfixed". That reading was correct and remains correct at `main`. ## The change Resolve the repository root from the **filesystem** rather than from a variable: walk up for a `.git` entry, accepting a directory **or** a file so linked worktrees and submodules resolve. Git own answer is returned untouched whenever git produced one, and `CLAUDE_PROJECT_DIR` is kept as a further fallback, so the case `main` already handles is subsumed rather than replaced. Four commits, ordered so the defect is demonstrated before it is fixed: ``` 9cbb3c2 tests (red against main) 4d2cd84 fix b42a935 coverage 66a100d changelog + version ``` ## Verification - Baseline `main` **135/0**; with the change **138/0**; the two new tests **red** against `main` own hook (independently reproduced at `e47964ca`). - `main` newest positive override test passes unchanged under the replacement — verified rather than assumed, after confirming no `.git` sits on the temp-dir ancestor chain that would have made the walk answer differently on this host. - `shellcheck -x -S warning`, shell-portability, silent-skips, markdownlint, and changelog-parity all clean. ## Stated rather than glossed — three things not confirmed - **The POSIX-host spawn count was simulated**, by addressing the repo in git own path spelling on a Windows host. It was never observed on a real POSIX host. - **A perf claim was wrong on first pass and is corrected here.** An unconditional ~140ms Git Bash cost was expected; measurement showed **zero** extra spawns on Git Bash, because `rev-parse --show-toplevel` and `dirname` never produce the same path spelling there. The extra probe fires only where the spellings agree — 2 to 3 spawns, root-level files only. - **One `PASS=133 FAIL=1` intermittent** was seen at an abandoned intermediate commit. It was unnamed, did not reproduce in five runs at the successor commit, and never recurred in any run backing these numbers. Unconfirmed rather than dismissed. ## Provenance Prepared as a cherry-pickable offer while #2121 was open; #2121 merged at `5f92d946` without taking it, leaving no branch to cherry-pick onto, so this is cut from `main` instead. The offer comment on #2121 remains accurate for what it offered at the time. Fixes #2134 ## Conflict resolution against a moving `main` `main` moved under this branch twice and the PR went `DIRTY`. The version collision was resolved twice, and the branch now carries the second resolution's numbers. - **Conflict, both times: `plugins/markdown-format/CHANGELOG.md`.** `main` took `0.11.2` (#2120's shared `hook-utils.sh` NUL fix), then `0.11.3` (#2147). This branch's entry moved up each time and now sits at **`0.11.4`**, with `main`'s `0.11.3` and `0.11.2` kept below it, order strictly descending. - **`plugin.json` auto-merged to `main`'s number on both passes, silently leaving no bump at all** — no conflict marker, and only `check-changelog-parity.sh --check-bump` catches it. Bumped to `0.11.4` to match the changelog. This is the trap worth carrying forward: a manifest version collision does not conflict, it resolves to whichever side git saw last. - `check-changelog-parity.sh --check-bump origin/main` clean at the resolved tree. **History note, stated rather than glossed.** This resolution was first delivered as two merge commits (`git merge origin/main`, never a rebase, since force-push is blocked here). The branch was subsequently **force-pushed** to a rebased, linear history carrying the same resolved content and the same `0.11.4` numbers, which discarded those merge commits. The shipped branch is therefore a rebase, not the merge described above; the resolution it carries is the same one. **Version coordination with #2135:** that PR also bumps `markdown-format`, and after its own merges of `main` it currently takes `0.11.4` as well. Whichever of the two merges second must re-bump — the manifests will auto-merge to the same number without conflicting, exactly as described above. ## Related - Fixes #2134 — the two no-git root-resolution defects this PR closes. - Refs #2121 — the predecessor whose review comment identified the nested-docs case; merged at `5f92d946` without taking the offered follow-up, which is why this is cut from `main`. - Refs #2120 — merged into `main` mid-flight; its shared `hook-utils.sh` change took the `0.11.2` slot this branch's changelog entry originally occupied. - Refs #2135 — concurrent `markdown-format` version bump; see the coordination note above. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
No linked issue
Summary
Discharges stranded review thread
PRRT_kwDOTCGFQM6TzHVgon merged PR #1030, filed againstplugins/markdown-format/hooks/markdown-format.sh:73and never resolved.The working-tree membership scope added in 0.6.3 probes with
git rev-parse --show-toplevel, whichfails identically when the file is outside every working tree and when git is not installed on
PATH. On a POSIX host without git, that read every Markdown edit as out-of-tree, so the hookskipped all of them — silently, repo-wide, with
jqandmarkdownlint-cli2present and amarkdownlint config in place.
Git is not a documented prerequisite of this hook: the README "Requirements" section lists Bash,
jqandmarkdownlint-cli2, the setup skill checks those three, andhook::repo_roothas alwaystolerated git being absent by falling back to the file's directory.
Fix
Gate the skip on git being available, so an undecidable verdict lints rather than skips — the
same direction
file_is_gitignoredalready documents for the same input:The scope is unchanged wherever git can answer, and the fail-closed symlink-escape check ahead of it
is untouched.
The reviewer offered two routes — preserve the git-optional behaviour, or make git an explicit,
visibly checked prerequisite. This takes the first, because the second would add a hard dependency
the README, the setup skill and
hook::repo_rootall currently deny.Why the resulting fail-open is bounded
Exposure is bounded by the consumer opt-in gate, not by this scope. Without git,
hook::repo_rootcannot resolve a working-tree top and falls back to the edited file's own directory, so
markdownlint_config_discoverablesearches that single directory — a scratch/tmp/comment-body.mdstill does not lint unless
/tmpitself carries a markdownlint config. The noise class this scopeexists to stop stays stopped wherever git can actually answer.
Tests
markdown-format.test.sh: 133 pass, 0 fail on this branch.The new cases hide git from
PATHviaBASH_ENV, the same technique the existingmarkdownlint-cli2 PATH-hiding case uses, and it is exact for this predicate —
command -v gitfails and a direct
gitcall exits 127.They carry a control: "git absent with
CLAUDE_PROJECT_DIRset still lints", which proves thegit-absence shim does not disable the hook independently of the membership scope. Without it, a
green assertion would be consistent with the shim having broken the hook outright.
Assertions are made on the file's bytes, not on stdout: without git the finding digest and the
hook's own reporting differ, so stdout is not a stable oracle for "did this file get linted".
The pre-fix behaviour is not in doubt from the diff — with no
command -v gitguard,in_git_working_treereturns non-zero when git is missing,!inverts it, and theexit 0skipfires. I did not execute the suite against the pre-fix tree; stating that rather than implying a
run I did not do.
Related