Skip to content

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

Merged
kyle-sexton merged 8 commits into
mainfrom
fix/972-markdown-format-project-dir-fallback
Jul 25, 2026
Merged

fix(markdown-format): skip out-of-tree Markdown when CLAUDE_PROJECT_DIR unset (#972)#1030
kyle-sexton merged 8 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=67 FAIL=0.
    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 telemetry/slow-sink case
    that previously failed on this Windows host is now green: main's 0.6.2 made
    that detector differential rather than a fixed wall-clock bound, which this
    branch picks up in the merge.
  • 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.3 with entry).
  • shellcheck on markdown-format.sh + markdown-format.test.sh → clean;
    markdownlint-cli2 on the CHANGELOG → 0 errors.

Closes #972

Related

Draft hold released. Issue #972 records the git-working-tree fallback as a
defaulted decision with an open veto window ("maintainer-vetoable"), not a
required approval. The window has been open since 2026-07-22; no veto was
entered on the issue or this PR, the work-class was operator-ratified on
2026-07-23, and the implementation matches the defaulted branch and all three
acceptance criteria verbatim. Marked ready on that basis.

The version collisions are resolved: #903 cascade-bumped markdown-format
to 0.6.1, then main shipped 0.6.2 (test-only differential fd1-leak
detector). This branch merged origin/main in and placed the out-of-tree fix
under 0.6.3, keeping both prior entries intact. 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
Resolves the markdown-format CHANGELOG collision: main shipped 0.6.2
(test-only differential fd1-leak detector), so the out-of-tree scoping fix
moves to 0.6.3 and the manifest is re-bumped accordingly. Both entries are
kept.
@claude

claude Bot commented Jul 25, 2026

Copy link
Copy Markdown

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


PR Review

  • Gather PR diff and context
  • Read markdown-format.sh core fix
  • Read markdown-format.test.sh new tests
  • Read CHANGELOG and plugin.json
  • Analyze correctness, security, and CLAUDE.md alignment
  • Post review findings

Scope

4 files changed, all in plugins/markdown-format/. No shared library touched. This review is scoped to those files.


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

Placement is correct. The guard runs after line 54 (hook::read_file_path has already set FILE and enforced membership when CLAUDE_PROJECT_DIR is set), but before line 75 (hook::repo_root). The ${CLAUDE_PROJECT_DIR:-} default-empty expansion is correct under set -u (line 9).

The elif is unnecessary here — unlike the shared hook::read_file_path where the unset check is an elif branch, here it is a standalone if with an && short-circuit. The CLAUDE_PROJECT_DIR-set path already short-circuits on the [[ -z ]] check; git rev-parse only runs on the genuinely-unset path. Zero extra subprocess cost on the common path.

Quoting is clean. dirname "$FILE" double-quotes $FILE; "$(dirname "$FILE")" double-quotes the result into git -C. No word-splitting or glob expansion at any level.

Minor efficiency note (not a bug): When CLAUDE_PROJECT_DIR is unset and the file IS in a git tree (i.e. the guard does not skip), --show-toplevel runs twice — once here (line 69) and again at line 75 via hook::repo_root. Both are local, read-only. Acceptable given the guard is on the rare unset path.

Comment block (lines 60–67) is well-written. The rationale — that this is intentionally local to markdown-format because the shared guard must stay location-agnostic for cli-flag-verify — is stated clearly and will survive future readers.


Test coverage ✅

markdown-format.test.sh lines 242–272

Three things done well:

  1. run_hook already unsets CLAUDE_PROJECT_DIR (line 158: env -u CLAUDE_PROJECT_DIR …) — confirmed. The new test exercises the correct code path with no special setup needed.

  2. Pre-assertion guard (line 251): 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. CI-safe.

  3. Fixable fixture (line 257): printf '# Comment\n\n* bullet' has MD004 (* marker) + 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.

  4. Three assertions (lines 261–270): exit code 0, output empty, file content unchanged. Together these rule out a silent auto-fix (which also exits 0 with no findings output).

  5. Cleanup unconditional (line 272): rm -rf "$OUTOFTREE" is outside the if/else, so it runs in both the "test ran" and "test skipped" branches.

One minor style note (non-blocking, pre-existing): SCRATCH_BEFORE="$(cat "$SCRATCH")" uses command substitution, which strips trailing newlines. The fixture has none, so the comparison is accurate here. Worth knowing if a future fixture adds trailing content.


Version and CHANGELOG ✅

plugin.json shows 0.6.3; CHANGELOG has three distinct entries:

Version Change
0.6.3 This fix — out-of-tree scratchpad skip
0.6.2 Test-only: C1 fd1-leak detector differential (dcd9825, already on main)
0.6.1 hook-utils.sh sync for #903 git-option parser

One clarification note (no action needed): The PR description says "re-bumped to 0.6.2" — that text predates dcd9825 landing on main and being merged into this branch. The code is correct: 0.6.2 was already claimed, so 0.6.3 is the right bump. The description is stale but the implementation is right.

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 discarded (>/dev/null 2>&1) — no information leakage from the subprocess.
  • No new trust boundaries, external dependencies, or workflow files changed.
  • Scope effect is strictly more restrictive: the previously-unguarded CLAUDE_PROJECT_DIR-unset path now exits early for out-of-tree files.

CLAUDE.md alignment ✅

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

Summary

Fix is minimal, correct, well-tested, and properly scoped. No blocking issues. The only item that may look surprising is the version showing 0.6.3 while the PR description says 0.6.2 — but that is a stale description artifact, not a defect; the version progression in the code is correct. Ready to merge.

@claude

claude Bot commented Jul 25, 2026

Copy link
Copy Markdown

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


Security Review

  • Gather PR diff and identify changed files
  • Read markdown-format.sh core fix
  • Read markdown-format.test.sh additions
  • Read plugin.json and CHANGELOG.md
  • Analyze injection surfaces, logic flaws, and authorization issues
  • Post security findings

No security issues found.

This PR makes one functional change — a 3-line early-exit guard in markdown-format.sh (lines 68–71) — plus a version bump (0.6.2 → 0.6.3) 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 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: Strictly more restrictive — the CLAUDE_PROJECT_DIR-unset path was previously unguarded (every .md was processed). After this PR, only files inside a git working tree are processed in that scenario. No capability is widened.

Path traversal consideration: If $FILE contained ../ sequences, git -C "$(dirname "$FILE")" would run against a parent directory. The only consequence would be that the file is admitted for linting (if that parent is a git tree) or skipped. Either direction is safe — the worst case is a file gets skipped that shouldn't be, which is not exploitable.


Test additions — injection/safety audit

markdown-format.test.sh lines 242–272

mktemp -d produces a unique, non-empty absolute path. All uses of $OUTOFTREE and $SCRATCH are double-quoted. rm -rf "$OUTOFTREE" at line 272 is outside the if/else block, so it runs even in the skipped branch. The $OUTOFTREE value comes from mktemp -d which exits non-zero on failure — the script would have aborted before reaching rm -rf. No unsafe shell patterns.

The pre-assertion guard (line 251: git -C "$OUTOFTREE" rev-parse --show-toplevel) correctly defends against a false-pass on a CI host where /tmp sits inside a worktree — the test skips with ok rather than producing a spurious green result.


GitHub Actions / supply chain

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


@kyle-sexton
kyle-sexton marked this pull request as ready for review July 25, 2026 16:49
@claude

claude Bot commented Jul 25, 2026

Copy link
Copy Markdown

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


PR Review

  • Gather PR context and changed files
  • Read markdown-format.sh core fix
  • Read markdown-format.test.sh new tests
  • Read CHANGELOG and plugin.json
  • Check REVIEW.md always-check criteria
  • Analyze correctness, security, and CLAUDE.md alignment
  • Post review findings

Scope

4 files changed, all in plugins/markdown-format/. No shared lib/hook-utils.sh touched. Review is scoped to those files.


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

Placement is correct. The guard runs after line 54 (hook::read_file_path has set FILE and enforced membership when CLAUDE_PROJECT_DIR is set) and after the extension gate (lines 55–58), but before hook::repo_root (line 75). This order is right: the new guard has no meaning until FILE is available and confirmed to be a .md/.mdc file.

Short-circuit is correct. [[ -z "${CLAUDE_PROJECT_DIR:-}" ]] is evaluated first; the git rev-parse subprocess only runs on the genuinely-unset path. Zero added cost on the common CLAUDE_PROJECT_DIR-set path.

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

--show-toplevel is the right predicate. It succeeds only inside a working tree — the same test hook::repo_root already uses — so the skip criterion is consistent with the rest of the hook's tree-membership logic.

Minor efficiency note (non-blocking): When CLAUDE_PROJECT_DIR is unset and the file IS inside a git tree (the guard allows it through), --show-toplevel runs twice: once here (line 69) and again at line 75 via hook::repo_root. Both are local read-only calls; acceptable given this is the rare unset-directory path.

Comment block (lines 60–67) is well-written. The rationale — that the fix is intentionally local to markdown-format because cli-flag-verify needs the opposite unset-case policy — is stated clearly and is the decisive argument for the design choice.


REVIEW.md — always-check criteria ✅

Trust boundaries and injection (conventions/review/security.md#trust-boundaries-and-injection): $FILE is derived via jq -r '(.tool_input.file_path // empty)' inside hook::read_file_path — a JSON-decode, not shell eval. It is double-quoted in dirname "$FILE" and the outer "$(dirname "$FILE")" passed to git -C. The path reaches git as a pre-split argument, never as a concatenated shell string. No injection surface.

The remaining always-check items (object-level authorization, tenant-scoped access, secrets in source, atomicity, audit logging) do not apply to this change.


Test coverage ✅

markdown-format.test.sh lines 242–272

Four things done well:

  1. run_hook already unsets CLAUDE_PROJECT_DIR (line 158: env -u CLAUDE_PROJECT_DIR …) — the new test exercises the correct code path without any extra scaffolding.
  2. Pre-assertion guard (line 251): verifies the temp dir is genuinely outside a git tree before relying on the skip verdict. If CI's /tmp sits inside a worktree the case emits ok + skips rather than a false pass.
  3. Fixable fixture (line 257): printf '# Comment\n\n* bullet' carries MD004 + MD047 — issues the hook would fix if it ran. An unmodified file on exit 0 proves the skip is genuine, not a vacuous clean pass.
  4. Three assertions + unconditional cleanup (lines 261–272): exit code 0, output empty, file unmodified — together ruling out a silent auto-fix. rm -rf "$OUTOFTREE" runs outside the if/else, so cleanup happens in both branches.

The in-tree counterpart (unset CLAUDE_PROJECT_DIR, file inside a git tree, still linted) is covered by every existing fixture: Fixture A, B, T, etc. all live in $REPO (a git working tree) and already run via run_hook which unsets CLAUDE_PROJECT_DIR.


Version and CHANGELOG ✅

plugin.json shows 0.6.3; CHANGELOG has a clean three-entry progression:

Version Change
0.6.3 This fix — out-of-tree scratchpad skip
0.6.2 Test-only: C1 fd1-leak detector differential (already on main)
0.6.1 hook-utils.sh sync for #903 git-option parser

0.6.3 is the correct next patch bump given 0.6.2 was already claimed by the test-only change that landed on main. The CHANGELOG entry accurately describes the symptom, cause, and the "behavior when CLAUDE_PROJECT_DIR is set is unchanged" guarantee.

Note on PR description: The body still refers to "re-bumped to 0.6.2" — that text predates 0.6.2 landing on main and being merged into this branch. The code is correct; the description is a stale artifact. No action needed.


CLAUDE.md alignment ✅

  • Repo-agnostic: guard references only $CLAUDE_PROJECT_DIR and $FILE; no hardcoded paths.
  • Configurable without editing: no new config surface introduced.
  • Plugin-form-safe: sources only $(dirname "${BASH_SOURCE[0]}")/hook-utils.sh; no ../ reach-outs.
  • Single-plugin scope: lib/hook-utils.sh untouched; no cascade bump needed.
  • Versioned: explicit 0.6.3 in plugin.json.
  • Security: change is strictly more restrictive (more files skipped than before); no new trust boundaries, no new external dependencies.

🟡 Nit — $(cat) comparison strips trailing newlines

markdown-format.test.sh line 258

SCRATCH_BEFORE="$(cat "$SCRATCH")"

Command substitution strips trailing newlines. The fixture (printf '# Comment\n\n* bullet') has none, so the comparison on line 266 is accurate here. If a future test author adds a trailing-newline fixture, the before/after comparison would mask a partial modification. Not a bug for this PR; worth noting for future maintainers.


Summary

The fix is minimal, correct, well-tested, and properly scoped to a single plugin. All REVIEW.md always-check criteria pass. The version progression and CHANGELOG are accurate. One 🟡 Nit (command substitution strips trailing newlines in the before-content capture) — non-blocking. Ready to merge.

@claude

claude Bot commented Jul 25, 2026

Copy link
Copy Markdown

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


Security Review — PR #1030

  • Gather PR diff
  • Read changed files
  • 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 in markdown-format.sh (lines 68–71) — plus a test case, a version bump (0.6.2 → 0.6.3), and a 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.

Bash variable expansion in $FILE value: Bash does not perform command substitution or variable expansion inside a double-quoted argument that happens to contain $(...) or ${...} as literal characters — those sequences are only expanded when they appear in the shell source text, not inside an already-expanded variable value. A crafted file_path of $(evil) in the JSON input would be passed literally as a directory name to git -C, not executed.

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

${CLAUDE_PROJECT_DIR:-} under set -u: The default-empty expansion is correct and avoids an unbound variable abort under set -u (line 9).

Scope effect: Strictly more restrictive — the CLAUDE_PROJECT_DIR-unset path was previously unguarded (every .md was processed). After this PR, only files inside a git working tree are processed in that scenario. No capability is widened.

Path traversal: If $FILE contained ../ sequences, git -C "$(dirname "$FILE")" would run against a traversed ancestor. The consequence is binary — the file is either skipped (if the resolved directory is not in a git tree) or admitted for linting (if it is). Neither direction is exploitable — the hook's only action on admitted files is markdownlint-cli2 --fix "$FILE" (a read/write linter invoked with the quoted $FILE value), which is not sensitive to the git-tree check path.


Test additions — injection/safety audit

markdown-format.test.sh lines 250–272

mktemp -d produces a unique, non-empty absolute path. All uses of $OUTOFTREE and $SCRATCH are double-quoted. The rm -rf "$OUTOFTREE" cleanup at line 272 is unconditional (outside the if/else), so it executes even in the "skipped" branch. mktemp -d exits non-zero on failure; the script would abort before reaching rm -rf. No unsafe shell patterns in the new test code.

The pre-assertion guard (line 251: git -C "$OUTOFTREE" rev-parse --show-toplevel) correctly defends against a false-pass on a CI host where /tmp sits inside a worktree — the test skips with ok rather than producing a spurious result.


GitHub Actions / supply chain

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

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d1882518ba

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

Comment thread plugins/markdown-format/hooks/markdown-format.sh Outdated
…hip test

The CLAUDE_PROJECT_DIR-unset fallback tested the lexical parent of the edited
path, so an in-repository symlink whose target lives outside any git working
tree passed the guard and markdownlint-cli2 --fix rewrote that external target
under the repository's rules. Derive the directory from hook::physical_path
first, matching how hook::read_file_path decides project membership.

Adds a regression case: an in-repo symlink to an out-of-tree .md is skipped and
its target left unmodified, guarded to skip on hosts that cannot create real
symlinks.
@claude

claude Bot commented Jul 25, 2026

Copy link
Copy Markdown

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


PR Review

  • Gather PR diff and changed files
  • Read markdown-format.sh (symlink resolution commit)
  • Read markdown-format.test.sh additions
  • Read CHANGELOG and plugin.json
  • Analyze correctness, security, and CLAUDE.md alignment
  • Post review findings

Scope of this review pass

The latest commit (3cbdffe) directly addresses the Codex P2 finding (symlink escape in the out-of-tree membership test). Three files changed: markdown-format.sh (one-line fix), markdown-format.test.sh (symlink regression case), and CHANGELOG.md (updated description). plugin.json is already at 0.6.3 from the prior push; this commit does not re-bump it.


Symlink fix — correctness ✅

markdown-format.sh line 73

! git -C "$(dirname "$(hook::physical_path "$FILE")")" rev-parse --show-toplevel >/dev/null 2>&1; then

The guard now resolves the physical path before deriving the directory for the git -C test. This is the right fix and matches exactly how hook::read_file_path resolves membership (hook::normalize_path "$(hook::physical_path "$file")"). Consistency between the two membership checks is the key correctness property here.

hook::physical_path implementation (lib/hook-utils.sh line 176):

hook::physical_path() {
  local resolved
  if resolved=$(realpath -- "$1" 2>/dev/null) || resolved=$(readlink -f -- "$1" 2>/dev/null); then
    if [[ -n "$resolved" ]]; then
      printf '%s' "$resolved"
      return
    fi
  fi
  printf '%s' "$1"
}

realpath / readlink -f fallback chain is correct. The -- "$1" stops option parsing so paths starting with - are handled safely. The fallback to the raw path on resolution failure is consistent with the existing comment ("degrading to the historical comparison beats silently disabling the hook").

Validity guarantee at call site: $FILE reaches the new guard only after line 54's hook::read_file_path validates [[ -f "$file" ]]. A -f test on a symlink resolves through to the target, so when hook::physical_path is called here the target is guaranteed to exist — realpath will resolve it, not fail.

Quoting chain: "$(dirname "$(hook::physical_path "$FILE")")"$FILE is double-quoted into hook::physical_path, the function's output is captured in $() (no word-splitting), and dirname's output is double-quoted into git -C. No injection surface.


Symlink regression test — correctness ✅

markdown-format.test.sh lines 272–296

The test:

  1. Reuses the existing $OUTOFTREE scratchpad fixture (same $SCRATCH target).
  2. Creates $REPO/escaping-link.md$SCRATCH — a symlink whose lexical parent is a git tree but whose physical target is out-of-tree.
  3. Passes the symlink path ($LINK) to run_hook — exactly what Claude Code would provide — and asserts exit 0, no output, and the external target unmodified.
  4. Uses [[ -L "$LINK" ]] to confirm a real symlink was created (not a copy, as Git Bash without winsymlinks mode would produce), then gracefully skips with ok if symlinks aren't supported.
  5. Cleans up with rm -f "$LINK" before the unconditional rm -rf "$OUTOFTREE".

This is precisely the right regression test for the reported vector. The four assertions (two exit-code/output checks, two content-unchanged checks) leave no path for a silent partial fix.

One minor style note (non-blocking, pre-existing): LINK_BEFORE="$(cat "$SCRATCH")" captures the target content via command substitution, which strips trailing newlines. The fixture has no trailing newline, so the comparison on line 288 is accurate. Same nit as flagged in prior review rounds; still non-blocking.


CHANGELOG ✅

The 0.6.3 entry now explicitly documents the symlink-resolution behavior:

Membership is decided on the physical path (symlinks resolved), matching the set-CLAUDE_PROJECT_DIR guard, so an in-repository symlink to an out-of-tree file cannot pull the external target into --fix.

This makes the security guarantee visible to consumers upgrading past 0.6.2.


Security ✅

The Codex-reported P2 finding is fully addressed. Before this commit, a Claude Code session with CLAUDE_PROJECT_DIR unset could Write a symlink inside the working repo pointing to a sensitive out-of-tree file, and the hook would run markdownlint-cli2 --fix on the external target. After this commit, hook::physical_path resolves the symlink to its target first; the git membership test then evaluates the target's directory, and a target outside any working tree causes the hook to exit 0 without touching it.

No new trust surfaces, no new subprocesses beyond the existing realpath / readlink -f calls already used by hook::read_file_path in the shared library.


CLAUDE.md alignment ✅

  • Repo-agnostic: references only $FILE and $CLAUDE_PROJECT_DIR.
  • Plugin-form-safe: sources only the co-located hook-utils.sh; no ../ reach-outs.
  • Single-plugin scope: lib/hook-utils.sh untouched; no cascade bump triggered.
  • Versioned: 0.6.3 already in place in plugin.json.

Summary

The symlink fix is minimal (one changed expression in the guard), uses the pre-existing hook::physical_path function for consistency, and is directly regression-tested. No blocking issues. Ready to merge.

@claude

claude Bot commented Jul 25, 2026

Copy link
Copy Markdown

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


Security Review — PR #1030

  • Gather PR diff and identify changed files
  • Read markdown-format.sh core fix (current HEAD 3cbdffe)
  • Read markdown-format.test.sh additions
  • Read plugin.json and CHANGELOG.md
  • Analyze injection surfaces, symlink handling, and logic flaws
  • Analyze GitHub Actions / supply-chain risk
  • Post security findings

No exploitable security issues found.

This review covers the 4 files changed in this PR at HEAD 3cbdffea548503218e14912cfd4c8e904c6604dc. The functional delta is one early-exit guard in markdown-format.sh, a test suite addition, a version bump (0.6.2 → 0.6.3), and a CHANGELOG entry.


Codex P2 (symlink escape) — addressed ✅

The Codex review comment flagged that an in-repo symlink to an out-of-tree target would bypass the membership guard because the check ran against the lexical parent (the in-repo directory). Commit 3cbdffe directly addresses this by resolving the file's physical path before deriving the directory:

markdown-format.sh line 73

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

For a symlink /repo/link.md → /tmp/out-of-tree/comment-body.md:

  • hook::physical_path "/repo/link.md"/tmp/out-of-tree/comment-body.md
  • dirname/tmp/out-of-tree
  • git -C "/tmp/out-of-tree" rev-parse --show-toplevel fails → guard exits 0

The external file is not rewritten. The fix is correct.


Injection audit ✅

$FILE provenance. $FILE is assigned via hook::read_file_path, which extracts the value with jq -r '(.tool_input.file_path // empty) | gsub("\r";"")' — a JSON decode path, not evaluated shell text. The || exit 0 on line 54 means an empty or missing path never reaches the new guard.

Quoting. dirname "$FILE" double-quotes $FILE; "$(dirname ...)" double-quotes the dirname result before passing it to git -C. hook::physical_path calls realpath -- "$1" and readlink -f -- "$1" with -- terminators, preventing a path starting with - from being interpreted as a flag. No word-splitting or glob expansion at any level.

Output discarding. Both stdout and stderr of git rev-parse are discarded via >/dev/null 2>&1. No git output (error messages, repo names, resolved paths) can reach the hook's output channel.

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


hook::physical_path fallback — SUGGESTION (very low confidence)

lib/hook-utils.sh lines 176–185

On platforms where neither realpath nor readlink -f is available, hook::physical_path falls back to returning the argument unchanged (the lexical path). On such a host the symlink escape detection degrades to the pre-fix behavior — an in-repo symlink pointing out-of-tree would not be caught, and markdownlint-cli2 --fix would be called on the symlink path, which the kernel follows to rewrite the external target under repo formatting rules.

This is acknowledged as a known degradation in the function's comment ("degrading to the historical comparison beats silently disabling the hook on those hosts"). In practice, realpath ships with GNU coreutils on Linux, readlink -f is available on macOS, and Git Bash on Windows ships realpath via its bundled coreutils. The fallback is unreachable on any common deployment platform. The consequence if reached (wrong formatting rules applied to a file the agent already wrote) is not exploitable.

Not a blocking finding; noted for completeness.


Test coverage of the symlink scenario ✅

markdown-format.test.sh lines 272–296

The test creates a real symlink ($REPO/escaping-link.md → $OUTOFTREE/comment-body.md), passes the in-repo symlink path to the hook, and asserts both that the hook exits 0 and that the out-of-tree target file is unmodified. The [[ -L "$LINK" ]] guard skips the case on Git Bash without winsymlinks (where ln -s copies rather than links). No unsafe shell patterns.


GitHub Actions / supply chain ✅

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

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3cbdffea54

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

Comment thread plugins/markdown-format/hooks/markdown-format.sh Outdated
…available

hook::physical_path degrades to the unchanged lexical path when neither
realpath nor readlink -f resolves. The out-of-tree membership test then
asked git about the in-repository symlink's own parent, admitted it, and
let markdownlint-cli2 --fix rewrite the out-of-tree target under repo
rules.

An unresolved symlink is now skipped. The check is on the outcome -- the
path came back unchanged for a path that IS a symlink -- rather than on
resolver availability, so it also covers a resolver that exists but
fails.
@claude

claude Bot commented Jul 25, 2026

Copy link
Copy Markdown

Claude encountered an error after 2s —— View job


I'll analyze this and get back to you.

@claude

claude Bot commented Jul 25, 2026

Copy link
Copy Markdown

Claude encountered an error after 2s —— View job


I'll analyze this and get back to you.

@github-actions

github-actions Bot commented Jul 25, 2026

Copy link
Copy Markdown

Warning

Automated review did not complete — this is an infrastructure failure, not a review verdict.

Treat any Claude comment on this PR (including a placeholder like "I'll analyze this and get back to you") as incomplete, not "no findings."

Re-running the job, or pushing a new commit, will retry the review.

@github-actions

github-actions Bot commented Jul 25, 2026

Copy link
Copy Markdown

Warning

Automated security review did not complete — this is an infrastructure failure, not a review verdict.

Treat any Claude comment on this PR (including a placeholder like "I'll analyze this and get back to you") as incomplete, not "no findings."

Re-running the job, or pushing a new commit, will retry the review.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 660b52cc94

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

Comment thread plugins/markdown-format/hooks/markdown-format.sh Outdated
…ambient git env

GIT_DIR and GIT_WORK_TREE override Git's repository discovery outright
(https://git-scm.com/docs/git), so `git -C <out-of-tree dir> rev-parse
--show-toplevel` answered with the inherited repository and admitted an
external file. The membership probe now clears the repository-selection
and discovery variables so the verdict comes from the file's physical
directory alone.
@claude

claude Bot commented Jul 25, 2026

Copy link
Copy Markdown

Claude encountered an error after 2s —— View job


I'll analyze this and get back to you.

@claude

claude Bot commented Jul 25, 2026

Copy link
Copy Markdown

Claude encountered an error after 2s —— View job


I'll analyze this and get back to you.

@kyle-sexton
kyle-sexton merged commit 7d9a829 into main Jul 25, 2026
26 checks passed

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a1491f966e

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

(
unset GIT_DIR GIT_WORK_TREE GIT_COMMON_DIR GIT_CEILING_DIRECTORIES \
GIT_DISCOVERY_ACROSS_FILESYSTEM
git -C "$1" rev-parse --show-toplevel

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Handle missing Git before treating the file as out-of-tree

When CLAUDE_PROJECT_DIR is unset on a POSIX host without git on PATH, this command fails exactly like a negative membership result, so every Markdown edit is silently skipped even when the file is in a repository and jq and markdownlint-cli2 are available. Previously hook::repo_root tolerated Git being unavailable by falling back to the file directory, and neither the README requirements nor the setup check requires Git; either preserve that behavior or make Git an explicit, visibly checked prerequisite rather than interpreting command-not-found as out-of-tree.

Useful? React with 👍 / 👎.

@kyle-sexton
kyle-sexton deleted the fix/972-markdown-format-project-dir-fallback branch July 25, 2026 23:12
kyle-sexton added a commit that referenced this pull request Aug 10, 2026
…ree" (#2121)

No linked issue

## Summary

Discharges stranded review thread `PRRT_kwDOTCGFQM6TzHVg` on merged PR
#1030, filed against
`plugins/markdown-format/hooks/markdown-format.sh:73` and never
resolved.

The working-tree membership scope added in 0.6.3 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
`PATH`. 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: the README
"Requirements" section lists Bash,
`jq` and `markdownlint-cli2`, the setup skill checks those three, and
`hook::repo_root` has always
tolerated 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_gitignored` already documents for the same
input:

```bash
-  if ! in_git_working_tree "$(dirname "$FILE_PHYSICAL")"; then
+  if command -v git >/dev/null 2>&1 &&
+    ! in_git_working_tree "$(dirname "$FILE_PHYSICAL")"; then
     exit 0
   fi
```

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_root` all 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_root`
cannot resolve a working-tree top and falls back to the edited file's
own directory, so
`markdownlint_config_discoverable` searches that single directory — a
scratch `/tmp/comment-body.md`
still does not lint unless `/tmp` itself carries a markdownlint config.
The noise class this scope
exists 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 `PATH` via `BASH_ENV`, the same technique
the existing
markdownlint-cli2 PATH-hiding case uses, and it is exact for this
predicate — `command -v git`
fails and a direct `git` call exits 127.

They carry a control: **"git absent with `CLAUDE_PROJECT_DIR` set still
lints"**, which proves the
git-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 git` guard,
`in_git_working_tree` returns non-zero when git is missing, `!` inverts
it, and the `exit 0` skip
fires. I did not execute the suite against the pre-fix tree; stating
that rather than implying a
run I did not do.

## Related

- #1030 — where the finding was filed
- #1938 — the stranded post-merge review-findings sweep this came out of

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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