test(markdown-format): write the override-inert negative with a direct REPO_ROOT read

[kyle-sexton]

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

• fix(markdown-format): stop reading a missing git as "file is out of tree" #2121 — where the override landed and where the incorrect comment shipped
• Triage the remaining stranded P2 review findings from the post-merge sweep #1938 — the stranded post-merge review-findings sweep

[cursor]

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.

[claude]

Claude finished @kyle-sexton's task in 53s —— View job

---

Security Review — PR #2128

• Fetch PR diff (git diff origin/main...HEAD)
• Read changed file in full for context
• Review for injection, secrets handling, trust-boundary issues
• Post findings

Scope: single file, test-only — plugins/markdown-format/hooks/markdown-format.test.sh (+65/-20). No workflow, manifest, or permission changes; nothing outside this test harness is touched.

Findings

No security issues found.

The diff adds a negative-case test for the REPO_ROOT override in markdown-format.sh, plus rewrites the surrounding comment explaining why. The new logic:

• Builds a restricted PATH string (MD_SHIM_PATH) by filtering the test runner's own $PATH, and plants shim scripts at fixed, non-attacker-controlled locations under $WORK (a private mktemp -d, cleaned up by the pre-existing trap cleanup EXIT). Nothing here consumes untrusted external input.
• Interpolates only two fixed literals ("OUTER" / "INNER") and a mktemp-derived path into the generated shim script via printf %s, so there's no command-injection surface — the values are constants controlled by the test itself, not attacker-influenced data.
• Invokes the real hook (bash "$HOOK") under a deliberately narrowed PATH/CLAUDE_PROJECT_DIR, consistent with the sandboxing pattern already used throughout the rest of this file (e.g. the existing BASH_ENV-based git/jq/markdownlint shims a few hundred lines up).
• Doesn't touch secrets, tokens, credentials, network access, or any GitHub Actions trigger/permission surface — it's local, hermetic test tooling exercising a shell script's REPO_ROOT resolution against fixture directories it creates and destroys itself.

No logic flaws, authorization gaps, or injection surfaces identified in this diff.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: fb0b3e094b

ℹ️ 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".

[claude]

Claude finished @kyle-sexton's task in 5m 30s —— View job

---

Claude's review

• Gather context (diff, CLAUDE.md, related files)
• Understand the request
• Review the changed test file for correctness, coverage, maintainability
• Post findings

This PR replaces the "negative twin is unwritable" comment in markdown-format.test.sh with an actual negative test for the REPO_ROOT override, using a repo-local markdownlint-cli2 shim planted at both candidate roots to directly observe which root the hook resolved.

I traced the logic against markdown-format.sh's hook::repo_root (line 229), the override guard (lines 230-237), and resolve_repo_markdownlint (lines 386-417), and the test's control flow is sound:

• Initializing git at $NEG_OUTER/inner means hook::repo_root resolves the toplevel directly, so the override guard's REPO_ROOT == dirname(FILE) condition never becomes true — this correctly exercises the "override stays inert because git already answered" path, not a no-op.
• resolve_repo_markdownlint's symlink/physical-path checks pass cleanly since the shim is a plain file, not a symlink.
• The .markdownlint-cli2.jsonc written at both NEG_OUTER and NEG_OUTER/inner is deliberate, not dead weight — it keeps markdownlint_config_discoverable from short-circuiting even in the hypothetical case where REPO_ROOT resolves to OUTER instead, so that branch would still be exercised and caught as a fail rather than silently skipped.
• The SKIPPED branch follows the same visible-skip convention already used twice elsewhere in this file (lines 295, 412), consistent with scripts/check-silent-skips.sh's doctrine even though *.test.sh files are outside that script's scope.

I left two related 🟡 nits inline (lines 578, 586-591) on the PATH-stripping mechanism used to hide the PATH copy of markdownlint-cli2: it duplicates, with a more fragile approach, the command()-override shim (NO_MDLINT_ENV) the same file already uses for identical purpose, and the code checks only that jq survives the strip even though the neighboring comment says "jq and git must stay reachable." Both are non-blocking — the failure mode is a loud, misleading fail rather than a false pass.

I could not execute the test suite in this sandbox (bash execution beyond git required approval that wasn't available), so the above is static analysis against the hook's source, not a confirmed run. The author's own "Tests" section in the PR body (ok: ... output) is author-claimed and I did not independently reproduce it.

No security-relevant surface in this diff (test-only shell script, no new trust boundary).

Branch: test/markdown-format-override-negative

[github-actions]

Claude has reviewed this PR 3 times. The lane skips further automatic reviews after 5; deleting this comment resets the count.

[claude]

Claude finished @kyle-sexton's task in 1m 5s —— View job

---

Security Review — PR #2128 (HEAD 5660176)

• Fetch PR diff (git diff origin/main...HEAD)
• Read changed file in full for context
• Review for injection, secrets handling, trust-boundary issues
• Post findings

Scope: single file, test-only — plugins/markdown-format/hooks/markdown-format.test.sh (+72/-20). No workflow, manifest, or permission changes.

Findings

No security issues found.

This commit replaces the earlier PATH-directory-stripping approach with a BASH_ENV command() override (NEG_ENV) that hides markdownlint-cli2 from command -v without mutating PATH — this also resolves the non-security correctness concern raised by Codex and my prior review (that stripping a whole PATH directory could remove git alongside the linter and misreport a REPO_ROOT regression). From a security standpoint the new mechanism has the same properties as before:

• All interpolated values ($NEG_MARK, $_md_label ∈ {OUTER, INNER}, $NEG_OUTER) are test-local — derived from $WORK (a private mktemp -d, cleaned up by the file's pre-existing trap cleanup EXIT) or fixed literals, never from untrusted external input.
• The NEG_ENV bash-env file is written via a quoted heredoc (<<'NEGEOF'), so no shell expansion occurs when the file is created — it can't be used as an injection vector.
• The generated shim scripts (node_modules/.bin/markdownlint-cli2) are built with printf using only the two fixed labels and a mktemp-derived marker path — no command-injection surface.
• The hook under test is invoked via env BASH_ENV="$NEG_ENV" ... bash "$HOOK" inside a subshell with cd "$UNRELATED", consistent with the sandboxing pattern already used elsewhere in this file.
• No secrets, tokens, credentials, or network access are touched; nothing here interacts with GitHub Actions triggers or permissions.

This confirms the conclusion from the prior two review passes on this PR (before and after the PATH-stripping → BASH_ENV rework): the change is hermetic, local test tooling with no new trust boundary.

[claude]

Claude finished @kyle-sexton's task in 2m 23s —— View job

---

Code Review — PR #2128

• Gather context (diff, CLAUDE.md, changed file)
• Review test correctness, coverage, maintainability
• Cross-check existing review comments (Codex, prior Claude passes)
• Post inline findings and summary

Summary

This PR is two commits: fb0b3e0 introduced the negative test using a hand-rolled PATH-directory-stripping mechanism, and 5660176 replaced that mechanism with the BASH_ENV command() override (matching the pattern NO_MDLINT_ENV already uses further down the file). That second commit directly addresses the concerns raised by Codex's and the earlier Claude review's inline comments on this PR (the "git could share a PATH directory with markdownlint-cli2" failure mode, and "reuse the existing command()-override technique instead of a fragile PATH strip") — I traced the final diff against markdown-format.sh (command -v markdownlint-cli2 gate at line 420, resolve_repo_markdownlint at lines 386-417, hook::repo_root at hook-utils.sh:315) and confirmed the command() override correctly forces the fallthrough to the repo-local shim without touching PATH, so git and everything else on it stays reachable. The removal of the old PATH-dependent skip branch is also a genuine improvement — the case is no longer conditionally skipped based on the runner's PATH layout.

One issue survives the second commit: a stale comment paragraph (lines 576-580) was left over from the superseded PATH-stripping draft and now contradicts the paragraph immediately below it (lines 586-596), which correctly explains why PATH-stripping was abandoned. Left inline on the relevant lines.

I could not execute the test suite in this sandbox (no shell access beyond the read-only checks used for review). The logic was verified by reading the hook's source and tracing the control flow by hand, not by running the tests — the author's own "Tests" section in the PR body is author-claimed and unverified by me.

No other correctness, error-handling, or maintainability issues found in this diff. Security review is out of scope for this lane (see #2128's dedicated security-review pass).

[claude]

Claude finished @kyle-sexton's task in 1m 7s —— View job

---

Security Review — PR #2128 (HEAD 72e4598)

• Fetch PR diff (git diff origin/main...HEAD)
• Read changed file in full for context
• Review for injection, secrets handling, trust-boundary issues
• Post findings

Scope: single file, test-only — plugins/markdown-format/hooks/markdown-format.test.sh (+73/-20). No workflow, manifest, or permission changes.

Findings

No security issues found.

This trigger is on the final commit (72e4598), which only edits the comment above the negative-case test — it fixes the stale "PATH must be stripped" prose flagged by the prior Claude review to now describe the shipped BASH_ENV command()-override mechanism. No executable logic changed since the previously reviewed commit (5660176); see diff between the two.

Re-confirming the properties already established for the executable diff (fb0b3e0 → 5660176) that this commit doesn't touch:

• All interpolated values ($NEG_MARK, $_md_label ∈ {OUTER, INNER}, $NEG_OUTER) are test-local — derived from $WORK (a private mktemp -d, cleaned up by the pre-existing trap cleanup EXIT) or fixed literals, never from untrusted external input. See markdown-format.test.sh#L598-L620.
• NEG_ENV is written via a quoted heredoc (<<'NEGEOF'), so no shell expansion occurs at write time — not an injection vector.
• The generated shim scripts under node_modules/.bin/markdownlint-cli2 are built with printf using only the two fixed labels and a mktemp-derived marker path — no command-injection surface.
• The hook under test runs via env BASH_ENV="$NEG_ENV" ... bash "$HOOK" from $UNRELATED, consistent with the sandboxing pattern already used elsewhere in this file (e.g. NO_MDLINT_ENV).
• No secrets, tokens, credentials, or network access are touched; nothing here interacts with GitHub Actions triggers or permissions.

This is hermetic, local test tooling with no new trust boundary — consistent with all three prior security-review passes on this PR.
· Branch: test/markdown-format-override-negative

[claude]

Claude finished @kyle-sexton's task in 2m 33s —— View job

---

Code Review — PR #2128 (HEAD 72e4598)

• Gather context (diff, CLAUDE.md, changed file, prior review threads)
• Review test correctness, coverage, maintainability
• Check whether prior findings (Codex PATH/git gap, stale-comment nit) were resolved by the latest commit
• Post inline findings and summary

Summary

Reviewed the final diff (git diff origin/main HEAD -- plugins/markdown-format/hooks/markdown-format.test.sh, +73/-20, single file) against the hook's actual resolution logic in markdown-format.sh — hook::repo_root and the override guard (lines 229–237), the command -v markdownlint-cli2 gate (line 420) that the new NEG_ENV shim targets, and resolve_repo_markdownlint (lines 386–417).

Traced the control flow for correctness: NEG_OUTER/inner is a real git repo whose toplevel is itself, so hook::repo_root returns NEG_OUTER/inner — which happens to equal dirname("$FILE"), satisfying the guard's string-equality half. What keeps the override from firing anyway is the guard's second, live half: git -C "$(dirname "$FILE")" rev-parse --show-toplevel succeeds because it's a genuine repo, so the negated condition is false and CLAUDE_PROJECT_DIR never substitutes in. REPO_ROOT stays NEG_OUTER/inner, resolve_repo_markdownlint reads the shim planted there, and the marker correctly comes back INNER. That's precisely the "the guard's equality half is not sufficient by itself" case this PR's body describes reproducing independently from #2100 — the test exercises it correctly, not by coincidence.

No new correctness issues found. Variable names (NEG_ENV, NEG_OUTER, NEG_MARK, _md_root_spec/_md_root/_md_label) are unique in the file (checked via grep) — no collision with existing fixtures. The .markdownlint-cli2.jsonc is deliberately duplicated at both NEG_OUTER and NEG_OUTER/inner so config-discoverability can't mask which root the override actually picked, matching the earlier review's read of that same pattern in the sibling positive case.

All prior review threads on this PR read as resolved by the final commit:

• Codex's P2 finding (PATH-stripping could remove git alongside markdownlint-cli2, false-failing a correct hook) and the matching nit from my earlier pass are moot — 5660176 replaced the PATH-directory strip with the BASH_ENV command() override (NEG_ENV, L598–606), which never touches PATH.
• The stale-comment nit from my second pass (the "jq and git must stay reachable" paragraph contradicting the paragraph right after it) is gone — 72e4598 rewrote the block (L557–597) to describe only the shipped command()-override mechanism, with the abandoned PATH-strip draft mentioned once, past tense, as the reason it was rejected. No remaining internal contradiction.

Maintainability: the choice not to reuse the existing NO_MDLINT_ENV shim is explained inline (that shim also stubs npx, which could mask "no shim ran" as an npx marker) and is a reasonable, narrow tradeoff — the self-contained NEG_ENV is 8 lines and easy to audit next to the thing it's guarding.

Scope check against CLAUDE.md: this is a test body / prose change to an existing hook's test harness, not a change to a plugin manifest, marketplace schema, hook contract, or documented harness behavior — the fresh-docs mandate's contract-surface scope doesn't apply here.

Not independently verified: I could not execute the test suite in this sandbox (shell execution beyond git requires approval that wasn't available here), so the above is static tracing against the hook's source, not a confirmed run. The PR body's ok: ... output is author-claimed and unverified by me.

No inline comments filed — no findings that survive the fixes already made in 5660176/72e4598. Security review is out of scope for this lane (see #2128's dedicated security-review pass, already posted).
· Branch: test/markdown-format-override-negative