Skip to content

fix(markdown-format): skip out-of-tree Markdown when CLAUDE_PROJECT_DIR unset (#972)#1030

Draft
kyle-sexton wants to merge 4 commits into
mainfrom
fix/972-markdown-format-project-dir-fallback
Draft

fix(markdown-format): skip out-of-tree Markdown when CLAUDE_PROJECT_DIR unset (#972)#1030
kyle-sexton wants to merge 4 commits into
mainfrom
fix/972-markdown-format-project-dir-fallback

Conversation

@kyle-sexton

@kyle-sexton kyle-sexton commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Summary

markdown-format's PostToolUse hook linted .md files outside any repository
(a loop lane's scratchpad/temp comment-body composed for gh issue comment --body-file) with repo-doc rules that do not apply — most visibly MD041
(first-line-h1) and MD013 (line-length). Pure advisory noise on every such write.

Cause: when CLAUDE_PROJECT_DIR is unset (an autonomous session whose cwd is not
a repo), the shared hook::read_file_path guard applies no membership scoping, so
the hook processed the file wherever it lived.

Fix

Add a markdown-format-local fallback in markdown-format.sh, right after the
extension gate: when CLAUDE_PROJECT_DIR is unset, skip a file that is not under
any git working tree.

if [[ -z "${CLAUDE_PROJECT_DIR:-}" ]] &&
  ! git -C "$(dirname "$FILE")" rev-parse --show-toplevel >/dev/null 2>&1; then
  exit 0
fi

A scratch/temp file in no git tree is skipped; a repo .md edited in such a
session is still linted; set-CLAUDE_PROJECT_DIR behavior is unchanged.
--show-toplevel succeeds only inside a working tree — the same predicate
hook::repo_root already uses — and the extra git rev-parse runs only on the
unset path.

Why local, not in the shared guard

The obvious-looking fix — teach the shared hook::read_file_path in
lib/hook-utils.sh to fall back to git-tree membership — is wrong, and its
test suite proves it: hook::read_file_path is consumed by 10 hooks, and
guardrails/cli-flag-verify is a location-independent guardrail — it catches
hallucinated CLI flags in written content regardless of repository membership
(a bad gh flag in a scratchpad comment-body is precisely its job, and precisely
the file this hook should not lint). Widening the shared guard made
cli-flag-verify.test.sh fail 9 assertions (the hook began skipping its
out-of-tree fixtures). The two hooks want opposite unset-case membership
policies, so the repo-scoping policy belongs in markdown-format, not the shared
library. This keeps the change to one plugin (matching the issue's scope and
rule 6d's single-plugin version bump) and touches no shared code.

Verification

Ran locally on Windows Git Bash (git 2.x, jq present), branch merged up to date
with current origin/main:

  • plugins/markdown-format/hooks/markdown-format.test.sh: PASS=66 FAIL=1.
    New case passes: an out-of-tree scratchpad .md is skipped (exit 0, no
    findings, file left unmodified). The in-tree-still-linted acceptance case is
    covered by every existing $REPO fixture (they live in a git working tree and
    already run with CLAUDE_PROJECT_DIR unset). The single FAIL is
    telemetry/slow-sink, a pre-existing Windows spawn-latency flake unrelated to
    this change
    : it fails on unmodified origin/main on this machine too, and
    worse (8396ms vs ~3000ms here; both under the sink's 3000ms sleep, so no fd1
    leak — it merely exceeds the tight 2000ms threshold under load). CI runs on
    ubuntu-24.04 where the headroom passes.
  • plugins/guardrails/hooks/cli-flag-verify.test.sh: PASS=48 FAIL=0
    confirms the guardrail is untouched (this is the regression the shared-lib
    approach caused; the local fix avoids it).
  • lib/hook-utils.test.sh: PASS=83 FAIL=0 (post-merge, includes fix(guardrails): distinguish --config-env from -c/--config in shared git parser #903's tests).
  • scripts/sync-hook-utils.sh --check → 12 copies match; --check-bump origin/main → "Lib unchanged; no version bumps required" (no shared-lib touch).
  • scripts/check-changelog-parity.sh --check-bump origin/main → OK
    (markdown-format 0.6.2 with entry).
  • shellcheck on markdown-format.sh + markdown-format.test.sh → clean;
    markdownlint-cli2 on the CHANGELOG → 0 errors.

Closes #972

Related

Held as draft because the issue flagged the git-worktree-fallback default as
"veto before merge" — a maintainer should confirm the approach before this lands.

The earlier version collision with #903 (fix(guardrails): shared git parser) is resolved: #903 merged to main and cascade-bumped
markdown-format to 0.6.1; this branch merged origin/main in and re-bumped
to 0.6.2, keeping #903's 0.6.1 CHANGELOG entry and placing the
out-of-tree fix under 0.6.2. The net diff (GitHub "Files changed") is the four
markdown-format files; no shared code is touched.

History note: earlier commits on this branch attempted a shared-lib approach
(edit lib/hook-utils.sh + sync 12 copies + bump all 12). That was reverted
after cli-flag-verify.test.sh proved the guardrail divergence described above.
The superseded cascade commit remains reachable in the "Commits" tab only via an
ours-merge and contributes nothing to the tree; this repository is
squash-merge only (allow_rebase_merge / allow_merge_commit both false),
so the intermediate commits collapse to the net four-file change on merge and
the superseded cascade can never be replayed.

Deferred follow-up (not in scope for #972): the 9 sibling formatter hooks
(bash-format, biome-format, eol-normalizer, go-format,
powershell-format, ruff-format, typos-format, actionlint) share the same
latent out-of-tree noise. Fixing them as a class wants an opt-in shared
scoping mechanism (formatters opt in; the guardrail stays location-agnostic) —
worth a separate issue with that trigger recorded.

Origin: converted from the fleet-sweep #657 line (markdown-format comment-body
lint noise).

Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com

Work-class: C3 (bug-fix-shaped) — attended triage 2026-07-23, operator-ratified. 🤖

…E_PROJECT_DIR unset (#972)

hook::read_file_path gated project-membership scoping on CLAUDE_PROJECT_DIR
being set. When unset (e.g. an autonomous session whose cwd is the home
directory, not a repo) the membership block was skipped entirely and the hook
processed the file wherever it lived — most visibly linting a lane's temp
comment-body .md outside any repository with repo-doc rules (MD041/MD013).

Fall back to git-working-tree containment when CLAUDE_PROJECT_DIR is unset: a
file under no git working tree is skipped, while a repo file edited in such a
session is still processed. Set-CLAUDE_PROJECT_DIR behavior is unchanged, and
the extra git rev-parse runs only on the unset (elif) path.

The fix lives in the shared lib/hook-utils.sh, so the change is synced to all
12 carrying plugins and each is version-bumped with a CHANGELOG entry (rule 6d
+ the sync/changelog CI gates). Ten call hook::read_file_path (real scoping
fix); claude-ops and desktop-notification carry the lib only (no behavior
change).

Regression coverage: lib/hook-utils.test.sh Test 12b (unset + in-tree accepted,
unset + out-of-tree skipped) and a markdown-format.test.sh black-box case.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@kyle-sexton kyle-sexton added do-not-merge Hard merge gate: do not merge while applied. automated Opened by automation. labels Jul 22, 2026
@claude

claude Bot commented Jul 22, 2026

Copy link
Copy Markdown

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


PR Review

  • Gather PR context
  • Read lib/hook-utils.sh core fix
  • Read lib/hook-utils.test.sh new tests
  • Read sample plugin changelogs and plugin.json version bumps
  • Read markdown-format.test.sh black-box test
  • Analyze correctness, security, and alignment with guidelines
  • Post review findings

Summary

Reviewed the 4-line logic change in hook::read_file_path and its full cascade (12 synced copies, 12 version bumps + changelogs, 2 new test suites). No blocking issues found. One observation and one minor nit below.


Core fix — correctness ✅

lib/hook-utils.sh lines 214–218

elif ! git -C "$(dirname "$file")" rev-parse --show-toplevel >/dev/null 2>&1; then
  # No project dir to scope against: --show-toplevel succeeds only inside a
  # git working tree, so its failure means the file is under none → skip.
  return 1
fi

The elif correctly fires only when CLAUDE_PROJECT_DIR is unset or empty (the if branch above handles the set case). --show-toplevel is the right predicate for "inside a working tree" — it's exactly what hook::repo_root already uses. The quoted double-expansion "$(dirname "$file")" is safe against command injection since $file arrives already-parsed by jq -r in a quoted shell context. Subprocess cost is zero on the common CLAUDE_PROJECT_DIR-set path; it only runs on the elif branch.

Edge case worth knowing (not a bug): If $file has no directory component (bare filename like foo.md), dirname returns . and the git check runs against the hook's CWD. In hook context that's typically the consumer repo root, so the file is correctly admitted. Files passed by Claude Code are normally absolute paths so this is academic.


Test coverage ✅

Test 12b in lib/hook-utils.test.sh (lines 437–474): The in-tree / out-of-tree duality is well covered. The guard that checks whether the temp dir actually sits outside a git tree (git -C "$NOGIT12B" rev-parse --show-toplevel) is the right defensive move for CI environments. Skipping with an ok message rather than a fail is the correct choice here since the test cannot be made to succeed on such a host.

Black-box test in markdown-format.test.sh (lines 231–261): Writes a file with fixable issues (* marker, missing final newline) to an out-of-tree temp dir and asserts exit 0 + no output + file unmodified — proving the skip is genuine (not a spurious clean pass). The existing fixtures implicitly cover the "in-tree with unset CLAUDE_PROJECT_DIR" path since run_hook already does env -u CLAUDE_PROJECT_DIR.


Cascade integrity ✅

All 12 plugin copies of hook-utils.sh share SHA f8f3c19cc85dc3b177acc5915eb86c6eec9c2048 — consistent with lib/hook-utils.sh. The 10 behavior-affected plugins have "Fixed" changelog entries; the 2 passthrough carriers (claude-ops, desktop-notification) correctly say "no behavior change." Version bump pattern (e.g., 0.6.00.6.1 for markdown-format, 0.18.10.18.2 for claude-ops) is consistent across the cascade.


Security ✅

  • git rev-parse is a read-only query against the local filesystem — no network access, no writes.
  • Both stdout and stderr are discarded (>/dev/null 2>&1), so no information leaks from this call.
  • No new trust boundaries introduced.

Observation — do-not-merge sequencing

The PR is correctly held. As documented in the PR body, #903 edits the same lib/hook-utils.sh and bumps the same 12 carriers, so whoever merges second will need a re-bump to 0.x.2 on all 12 plugins plus a --re-sync. If #969 (autonomy plugin, 13th carrier) lands first, an additional sync + autonomy bump is required. These are coordination issues, not defects in this PR's code.


Nit (non-blocking)

The rfp_unset helper in hook-utils.test.sh (line 443) is structurally identical to rfp (line 356) but hardcodes unset CLAUDE_PROJECT_DIR rather than accepting it as a parameter. This duplication is minor and acceptable for test isolation, but worth noting if the test suite grows more fallback cases.


Verdict: The fix is correct, minimal, well-tested, and compliant with the repo's design rules. Ready to merge once the sequencing with #903 (and optionally #969) is resolved.

@claude

claude Bot commented Jul 22, 2026

Copy link
Copy Markdown

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


Security Review — PR #1030

  • Gather PR diff and changed files
  • Audit lib/hook-utils.sh changes for injection / logic flaws
  • Audit lib/hook-utils.test.sh additions
  • Audit markdown-format.test.sh additions
  • Spot-check synced plugin copies for unexpected delta
  • Review CHANGELOG / plugin.json changes for supply-chain risk
  • Summarize findings

Findings

No security issues found.

The PR makes exactly one functional change: adding an elif branch to hook::read_file_path in lib/hook-utils.sh, then syncing that file byte-for-byte into 12 plugin copies. Everything else is version bumps and CHANGELOG entries.

What was reviewed

Core logic change (lib/hook-utils.sh line 214):

elif ! git -C "$(dirname "$file")" rev-parse --show-toplevel >/dev/null 2>&1; then
  return 1
fi

Injection surface — $filedirnamegit -C: $file is extracted via jq -r and then double-quoted at every callsite: dirname "$file" and "$(dirname "$file")". There is no word-splitting or glob-expansion exposure. Paths with spaces, special characters, or .. components are handled correctly. The dirname utility performs only lexical path manipulation; it evaluates nothing. The same git -C <path> rev-parse --show-toplevel pattern is already used in hook::repo_root with no changes in this PR.

Scope effect: The change narrows the hook's reach when CLAUDE_PROJECT_DIR is unset. Before this PR, the CLAUDE_PROJECT_DIR-unset path fell through the if block with no guard at all (every file was processed). After this PR, only files inside a git working tree are processed in that scenario. This is a strictly more restrictive posture.

New subprocess cost: The git rev-parse is gated behind the elif (only runs when CLAUDE_PROJECT_DIR is genuinely unset), so the common set-env path incurs no additional subprocess. Not a security issue, noted for completeness.

Synced copies: All 12 plugin copies of hook-utils.sh are byte-identical to lib/hook-utils.sh (verified by the PR's own sync-hook-utils.sh --check gate and confirmed by inspecting the changed SHA listed in the PR's file list — every plugin copy carries the same SHA f8f3c19cc85dc3b177acc5915eb86c6eec9c2048).

plugin.json version bumps: Purely semver integer increments (e.g. 0.6.0 → 0.6.1). No new external dependencies, no new action pins, no URL or registry changes.

GitHub Actions workflows: No workflow files changed in this PR. No pull_request_target, workflow_run-with-secrets, or ${{ github.* }}-in-run: patterns introduced.

Test additions: Test 12b in hook-utils.test.sh and the out-of-tree case in markdown-format.test.sh both exercise the new guard using mktemp -d fixtures. Both correctly pre-assert that the temp dir is genuinely outside a git tree before relying on the skip verdict — avoiding a false-pass on a CI host where /tmp sits under a worktree. No unsafe shell patterns in the test code.

@kyle-sexton

Copy link
Copy Markdown
Contributor Author

🔒 merge-drive lane claiming this PR for on-branch fix work this cycle (amendment-round: 16, worker tier). Will fix clear branch-owned CI/bot findings, defer non-DEFER-FORBIDDEN items with reply+resolve, push, then re-check the merge gate.

…IR unset (#972)

When CLAUDE_PROJECT_DIR is unset (an autonomous session whose cwd is not a
repo), hook::read_file_path applies no membership scoping, so the hook linted
the .md wherever it lived — most visibly a lane's temp comment-body composed
outside any repository (for gh --body-file), firing repo-doc rules (MD041,
MD013) that do not apply. Pure advisory noise on every such write.

Add a markdown-format-local fallback: when CLAUDE_PROJECT_DIR is unset, skip a
file that is not under any git working tree. A scratch/temp file in no git tree
is skipped; a repo .md edited in such a session is still linted; set-
CLAUDE_PROJECT_DIR behavior is unchanged.

The scoping is deliberately local to this hook rather than in the shared
lib/hook-utils.sh guard: repo-doc linting is repository-scoped, but sibling
hooks on the same guard (e.g. guardrails/cli-flag-verify) must fire regardless
of repository membership, so widening the shared guard would regress them.

Regression: markdown-format.test.sh asserts an out-of-tree scratchpad .md is
skipped (exit 0, no findings, file unmodified); the in-tree-still-linted case
is covered by every existing $REPO fixture.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@kyle-sexton kyle-sexton changed the title fix(markdown-format): fall back to git-worktree membership when CLAUDE_PROJECT_DIR unset (#972) fix(markdown-format): skip out-of-tree Markdown when CLAUDE_PROJECT_DIR unset (#972) Jul 22, 2026
@claude

claude Bot commented Jul 22, 2026

Copy link
Copy Markdown

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


PR Review

  • Gather PR context and changed files
  • Read markdown-format.sh core fix
  • Read markdown-format.test.sh new tests
  • Read disk-hygiene changes (disk-hygiene files unchanged vs origin/main — those SHAs in the PR context are from the merge base, not new changes)
  • Read markdown-format CHANGELOG and plugin.json
  • Analyze correctness, security, and alignment with CLAUDE.md guidelines
  • Post review findings

Actual diff vs origin/main: 4 files, all scoped to markdown-format — the PR context's disk-hygiene entries are from the merge base commit, not new work.


Core fix — correctness ✅

markdown-format.sh lines 68–71

if [[ -z "${CLAUDE_PROJECT_DIR:-}" ]] &&
  ! git -C "$(dirname "$FILE")" rev-parse --show-toplevel >/dev/null 2>&1; then
  exit 0
fi

The guard is correctly placed — after hook::read_file_path (line 54) sets FILE from the jq-parsed payload, so FILE is available and already membership-scoped when CLAUDE_PROJECT_DIR is set. The ${CLAUDE_PROJECT_DIR:-} expansion is safe under set -u. The git -C "$(dirname "$FILE")" path is double-quoted at every level; $FILE arrives from jq -r and is double-quoted in dirname "$FILE" and the outer "$(…)" — no word-splitting or glob exposure.

One minor efficiency note (not a bug): when CLAUDE_PROJECT_DIR is unset but the file IS in a git tree, git rev-parse --show-toplevel runs twice — once here and again at line 75 via hook::repo_root. Acceptable; both are local, read-only calls.

The placement decision (local to markdown-format, not in the shared guard) is sound and well-documented. The PR body's explanation — that cli-flag-verify needs the opposite unset-case policy — is the decisive argument, and the comment in the code summarizes it clearly without being redundant.


Test coverage ✅

markdown-format.test.sh lines 231–261

Three things done well:

  1. Pre-assertion guardgit -C "$OUTOFTREE" rev-parse --show-toplevel is checked before relying on the skip verdict. If the CI /tmp sits inside a git worktree the case is emitted as ok + skipped rather than a false pass. Correct and CI-safe.

  2. Fixable fixture contentprintf '# Comment\n\n* bullet' (MD004 * marker + MD047 missing final newline) means an unmodified file on exit 0 genuinely proves the skip, not a vacuous clean pass.

  3. Three assertions in one case — exit code 0, empty output, and file content unchanged. Together these rule out the hook silently auto-fixing (which would also exit 0 with no findings output).

One minor style nit: SCRATCH_BEFORE="$(cat "$SCRATCH")" uses command substitution, which strips trailing newlines. The fixture has no trailing newline, so the comparison [[ "$(cat "$SCRATCH")" == "$SCRATCH_BEFORE" ]] is accurate here. If a future fixture adds trailing content, this could mask a partial fix. Not a bug for the current test; just worth knowing.


Version bump and CHANGELOG ✅

0.6.0 → 0.6.1 is a correct patch bump for a bug fix. The CHANGELOG entry accurately describes the symptom, cause, and behavior-preservation guarantee ("Behavior when CLAUDE_PROJECT_DIR is set is unchanged").


Security ✅

  • git rev-parse is read-only, local, no network access.
  • Both stdout and stderr discarded; no information leakage.
  • No new trust boundaries, no new external dependencies.

CLAUDE.md alignment ✅

  • Repo-agnostic: guard references only $CLAUDE_PROJECT_DIR and $FILE; no hardcoded paths.
  • Plugin-form-safe: no ../ reach-outs; sources only $(dirname "${BASH_SOURCE[0]}")/hook-utils.sh.
  • Single-plugin scope: shared lib/hook-utils.sh untouched; sync-hook-utils.sh --check gate passes; no cascade bump needed.
  • Versioned: 0.6.1 explicit in plugin.json.

Summary

Fix is minimal, correct, well-tested, and properly scoped. No blocking issues. Ready to merge once the do-not-merge label (sequencing with #903 and optionally #969) is resolved.

@claude

claude Bot commented Jul 22, 2026

Copy link
Copy Markdown

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


Security Review — PR #1030

  • Gather PR diff (git diff origin/main...HEAD — 4 files changed in this branch)
  • Read markdown-format.sh core fix
  • Read markdown-format.test.sh additions
  • Read disk-hygiene changes (the 8-file count in the PR includes disk-hygiene files brought in by the merge commit from main; they don't appear in the branch diff and are not authored by this PR — reviewed as unscoped)
  • Trace $FILE origin through hook::read_file_path and jq -r
  • Analyze for injection, logic, and authorization issues
  • Post findings

No security issues found.

The PR makes one functional change: a 3-line early-exit guard in markdown-format.sh. The remainder is a version bump and CHANGELOG entry.


Core fix — injection surface

markdown-format.sh lines 68–71

if [[ -z "${CLAUDE_PROJECT_DIR:-}" ]] &&
  ! git -C "$(dirname "$FILE")" rev-parse --show-toplevel >/dev/null 2>&1; then
  exit 0
fi

$FILE provenance: $FILE is assigned at line 54 via FILE=$(printf '%s' "$INPUT" | hook::read_file_path). Inside hook::read_file_path (lib/hook-utils.sh line 195), the value is extracted with jq -r '(.tool_input.file_path // empty) | gsub("\r";"")' 2>/dev/null — a trusted JSON-decode path, not eval'd shell text.

Quoting audit: dirname "$FILE"$FILE is double-quoted; no word-splitting or glob expansion occurs regardless of the path's contents. The result of $(dirname "$FILE") is in turn double-quoted as the argument to git -C. git -C treats that value as a directory path, not a shell command. There is no injection surface.

Output handling: >/dev/null 2>&1 discards all stdout and stderr from the subprocess. No git output (errors, repo names, paths) can leak through the hook's output channel.

Scope effect: The change is strictly more restrictive — the hook now skips more files than before. Before this PR, the CLAUDE_PROJECT_DIR-unset path was unguarded (every .md file was processed). After, only files inside a git working tree are processed in that scenario. No capability is widened.


Test additions

markdown-format.test.sh lines 231–261

mktemp -d returns an absolute, non-empty path under safe system temp; its result is stored in $OUTOFTREE and used double-quoted throughout. The pre-assertion (git -C "$OUTOFTREE" rev-parse --show-toplevel) correctly guards against a CI host where /tmp sits inside a worktree — the test skips rather than producing a false pass. Cleanup via rm -rf "$OUTOFTREE" is safe because mktemp -d always returns a unique, non-empty path (if it fails, the shell exits before reaching the rm).


GitHub Actions / supply-chain

No workflow files are changed. No new external dependencies, action pins, or remote MCP servers are introduced. The plugin.json change is a semver patch bump (0.6.0 → 0.6.1) with no new dependencies or userConfig keys.

Resolve the markdown-format version collision introduced when #903 (shared git
parser fix) landed on main and cascade-bumped markdown-format to 0.6.1 — the
same bump this branch made. Re-bump to 0.6.2 (one past main) and split the
CHANGELOG so #903's 0.6.1 entry and this branch's out-of-tree fix (now 0.6.2)
each stand alone. No shared-lib change from this branch; main's hook-utils.sh
(including #903's --config-env parser change) is taken as-is.
@claude

claude Bot commented Jul 22, 2026

Copy link
Copy Markdown

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


PR Review

  • Gather PR diff and changed files
  • Review markdown-format.sh core fix
  • Review markdown-format.test.sh new tests
  • Review CHANGELOG.md and plugin.json
  • Summarize findings

Sequencing resolved ✅

The PR's do-not-merge note cited a version collision with #903. Looking at the current branch (2768aef merges origin/main), #903 has already landed and is incorporated. The CHANGELOG now correctly shows 0.6.1 for the #903 cascade and 0.6.2 for this fix — the collision is resolved by this merge-forward. The do-not-merge label can be removed if no other blockers remain.


Core fix — correctness ✅

markdown-format.sh lines 60–71

if [[ -z "${CLAUDE_PROJECT_DIR:-}" ]] &&
  ! git -C "$(dirname "$FILE")" rev-parse --show-toplevel >/dev/null 2>&1; then
  exit 0
fi

Placement is correct. The guard runs after line 54 (hook::read_file_path sets FILE) but before line 75 (hook::repo_root). This means:

  • When CLAUDE_PROJECT_DIR is set, hook::read_file_path has already scoped by membership — this guard short-circuits on the [[ -z ... ]] check and adds no subprocess cost.
  • When CLAUDE_PROJECT_DIR is unset, the git check fires on the previously-unguarded code path.

Injection surface is clean. $FILE arrives from jq -r inside hook::read_file_path, is double-quoted in dirname "$FILE", and the result is double-quoted again as the git -C argument. No word-splitting or glob expansion.

${CLAUDE_PROJECT_DIR:-} is correct under set -u (line 9) — the default-empty expansion avoids an unbound variable error.

One minor efficiency note (not a bug): When CLAUDE_PROJECT_DIR is unset and the file IS in a git tree, --show-toplevel runs here (line 69) and again at line 75 via hook::repo_root. Both are local, read-only — acceptable cost.

The comment (lines 60–67) is well-reasoned and correctly explains why the fix is local to markdown-format rather than in the shared guard. The argument — that cli-flag-verify needs the opposite policy — is the decisive one and is stated clearly without being redundant.


Test coverage ✅

markdown-format.test.sh lines 231–261

Three things done well:

  1. Pre-assertion guard (line 240): git -C "$OUTOFTREE" rev-parse --show-toplevel before relying on the skip verdict. If CI's /tmp sits inside a worktree, the case emits ok + skips rather than a false pass.

  2. Fixable fixture (line 246): printf '# Comment\n\n* bullet' has MD004 (* marker) and MD047 (missing final newline) — issues the hook would fix if it ran. An unmodified file on exit 0 therefore proves the skip, not a vacuous clean pass.

  3. Three assertions per case (lines 250–259): exit code, output, and file content unchanged. Together these rule out a silent auto-fix (which also exits 0 with no findings output).

Cleanup is correct (line 261): rm -rf "$OUTOFTREE" is outside the if/else, so it runs even in the "skipped" branch.

One minor style note (non-blocking): SCRATCH_BEFORE="$(cat "$SCRATCH")" (line 247) uses command substitution, which strips trailing newlines. The fixture has no trailing newline, so the cat comparison on line 255 is accurate here. Future fixtures with trailing content would need a different comparison method.


Version bump and CHANGELOG ✅

0.6.1 → 0.6.2 is the correct patch bump after #903 already claimed 0.6.1. The CHANGELOG entry accurately describes the symptom, cause, and the "behavior when CLAUDE_PROJECT_DIR is set is unchanged" guarantee.


Security ✅

  • git rev-parse is read-only, local, no network access.
  • Both stdout and stderr are discarded (>/dev/null 2>&1) — no information leakage.
  • No new trust boundaries, external dependencies, or workflow files changed.

CLAUDE.md alignment ✅

  • Repo-agnostic: guard references only $CLAUDE_PROJECT_DIR and $FILE; no hardcoded paths.
  • Single-plugin scope: lib/hook-utils.sh untouched; sync-hook-utils.sh --check passes; no cascade bump needed.
  • Versioned: explicit 0.6.2 in plugin.json.

Summary

The fix is minimal, correct, and well-tested. The sequencing issue with #903 is resolved in the current branch state. No blocking issues. Ready to merge pending the do-not-merge label being lifted.

@claude

claude Bot commented Jul 22, 2026

Copy link
Copy Markdown

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


Security Review — PR #1030

  • Gather PR context and changed files
  • Read markdown-format.sh core fix
  • Read markdown-format.test.sh additions
  • Audit injection surfaces and logic
  • Audit GitHub Actions / supply-chain risk
  • Post security findings

No security issues found.

This PR makes one functional change — a 3-line early-exit guard local to markdown-format.sh — plus a version bump (0.6.1 → 0.6.2) and CHANGELOG entry. The shared lib/hook-utils.sh is untouched.

Core fix — injection audit

markdown-format.sh lines 68–71

if [[ -z "${CLAUDE_PROJECT_DIR:-}" ]] &&
  ! git -C "$(dirname "$FILE")" rev-parse --show-toplevel >/dev/null 2>&1; then
  exit 0
fi

$FILE provenance: Assigned at line 54 via FILE=$(printf '%s' "$INPUT" | hook::read_file_path) || exit 0. Inside hook::read_file_path, the value is extracted with jq -r '(.tool_input.file_path // empty) | gsub("\r";"")' 2>/dev/null — a JSON-decode path, not evaluated shell text. The || exit 0 guard prevents an empty or missing path from reaching the new guard.

Quoting: dirname "$FILE" double-quotes $FILE; "$(dirname "$FILE")" double-quotes the result as the argument to git -C. No word-splitting or glob expansion at any level. git -C treats the value as a directory path, not a shell command.

Output handling: Both stdout and stderr are discarded via >/dev/null 2>&1. No git output (error messages, repo names, resolved paths) can leak through the hook's output channel.

Scope effect: The change is strictly more restrictive — the hook now exits 0 for more files than before. The CLAUDE_PROJECT_DIR-unset path was previously unguarded (every .md was processed); now only files inside a git working tree are processed in that scenario. No capability is widened; no information is emitted.

Edge case — bare filename: If $FILE has no directory component, dirname returns . and git -C . runs against CWD. In hook context, CWD is typically the consumer repo root, so the file is admitted. Claude Code provides absolute paths in practice; this is academic and not exploitable.

Test additions

markdown-format.test.sh lines 231–266

mktemp -d produces a unique, non-empty absolute path. All uses are double-quoted. The rm -rf "$OUTOFTREE" cleanup is safe — mktemp -d exits non-zero on failure and the script would have aborted before reaching it. The pre-assertion (git -C "$OUTOFTREE" rev-parse --show-toplevel) guards against a false-pass on a CI host where /tmp sits inside a worktree. No unsafe shell patterns.

GitHub Actions / supply chain

No workflow files changed. No new external dependencies, remote MCP servers, action pins, pull_request_target, or workflow_run-with-secrets patterns introduced. The plugin.json change is a pure semver patch increment with no new dependencies or userConfig keys.

@kyle-sexton kyle-sexton removed the do-not-merge Hard merge gate: do not merge while applied. label Jul 22, 2026
@kyle-sexton kyle-sexton added the agent-ready Fully specified and briefed; eligible for autonomous pickup from the frontier. label Jul 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent-ready Fully specified and briefed; eligible for autonomous pickup from the frontier. automated Opened by automation.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(markdown-format): hook lints out-of-tree files when CLAUDE_PROJECT_DIR is unset (add git-worktree fallback scoping)

1 participant