fix(markdown-format): resolve the repo root from the filesystem so nested files reach the root config without git - #2130
Conversation
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Claude finished @kyle-sexton's task in 5m 50s —— View job Code review completeReviewed the four changed files ( Note on verification: I could not run Findings
Other things checked, no issues found
|
|
Claude finished @kyle-sexton's task in 4m 50s —— View job Security review
Scope: the 4 files changed in this PR (diff against IMPORTANT (confidence: plausible, narrow preconditions) — the new no-git root walk trusts any
if [[ -e "$dir/.git" ]]; then
printf '%s' "$dir"
return 0
fiThis walks upward from the edited file's directory and stops at the first directory containing anything named Before this PR, the no-git fallback never walked the filesystem at all — it just returned the file's own directory ( Impact chain, given the surrounding gates:
This is a real trust-boundary widening, but the practical blast radius is narrow: it requires simultaneously (a) Worth deciding explicitly whether that precondition is acceptable, or whether the walk should validate the No other issues found. Specifically checked and clear:
zizmor-covered categories (unpinned actions, dangerous triggers, excessive permissions, template injection) are out of scope for this lane per instructions, and this PR touches no workflow files anyway. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 66a100dfcd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Claude has reviewed this PR 5 times. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
P1
|
| hook | canary (does the nested fix work?) | out-of-tree symlink target |
|---|---|---|
main 5f92d946 |
unchanged — this is the bug this PR fixes | UNCHANGED (vacuously: nothing was linted) |
this branch 66a100df |
REWRITTEN | REWRITTEN 3c62fb6e → 60fdfc85 |
with df8cf3d9 |
REWRITTEN | UNCHANGED 3c62fb6e → 3c62fb6e |
Main's row is UNCHANGED only because it never lints a nested file at all — so it is not evidence of containment, and I would not have caught that without the canary.
Measured while I was there: the root-level form of the same escape already rewrites the external target on main. So this branch did not invent the hole; it widened it from root-level files to every nested one. df8cf3d9 closes both.
Root cause
Making discovery succeed is what puts a file in front of --fix. Without git, the membership scope could not ask in_git_working_tree anything, so containment was not checked at all — and a symlink is the one shape whose lexical parent (inside the repository, where the config lives) and physical parent (outside it) disagree.
So containment is now decided from the filesystem instead of skipped. It runs only where the physical path differs from the lexical one, which for an ordinary file it never does — a git-less repository lints exactly as before. An undecidable git verdict still lints; an escape the filesystem can prove does not.
The two remedies that don't work, and why this one does
Resolving from FILE_PHYSICAL at the REPO_ROOT line does not help. markdownlint_config_discoverable is called with "$FILE", and it anchors on dirname "$1" — the symlink's own parent, a real directory inside the repository — independently of REPO_ROOT. Moving the root does not move discovery. The guard therefore sits in the membership scope, before discovery, where an early exit governs whether any of it runs.
A containment check comparing hook::physical_path output against REPO_ROOT is a spelling mismatch, not a containment answer. hook::physical_path resolves via realpath, which leaves /tmp as /tmp, while pwd -P resolves it to the underlying directory — hook-utils.sh documents exactly this divergence on hook::under_temp_root ("realpath resolves the Windows form to a drive path while leaving /tmp as /tmp"). Compare the two currencies and a file plainly inside the tree is rejected.
physically_inside therefore canonicalizes both operands through cd … && pwd -P at the point of comparison. That is the deliberate answer to "how are the two spellings reconciled":
pwd -Pis already this hook's local currency —markdownlint_config_discoverable(both operands),CONFIG_ROOT,CONFIG_TARGET_DIR, andresolve_repo_root's walk all use it. Adding a third regime would be the drift, not the fix.hook::normalize_pathis a spelling normalizer (backslashes, drive-letter case) — it does not canonicalize/tmp, so pairing it withphysical_pathreproduces the divergence rather than resolving it. It is right forhook::read_file_path's guard, where both operands arephysical_pathoutput, and wrong for a comparison against apwd -Proot.hook::physical_pathis documented to degrade to the unchanged lexical path when no canonicalizer exists. A containment guard whose currency can silently become un-normalized input is the wrong basis for deciding whether a write may leave the repository.
Tests
Two cases, both nestings, asserting on link survival rather than the target's bytes. The suite's stub linter rewrites with sed -i, which renames a temp over the path and so replaces a symlink while leaving the target untouched — verified on this host. Under that stub an unchanged target proves nothing; a surviving symlink proves --fix never ran on it. They also use MSYS=winsymlinks:nativestrict, which does create real symlinks here — the file's existing escape cases probe with bare ln -s and skip on Windows, which is why this shape had no Windows coverage before.
suite before this commit PASS=138 FAIL=2 (the two new cases, red)
suite after PASS=140 FAIL=0
shellcheck -x -S warning, check-shell-portability.sh --paths, check-silent-skips.sh --paths, and markdownlint-cli2 on the CHANGELOG all clean.
Nit PRRT_kwDOTCGFQM6XwDTK — fixed in the same commit
Correct as stated. hook::repo_root strips /.claude and \.claude; the signature check tested only the first, so a backslash-spelled hint would not have been recognised as a fallback. Both forms are now checked and the comment no longer says "one rewrite".
Not verified
The /tmp-versus-pwd -P divergence is documented in hook-utils.sh and I reproduced its effect on this Windows host; I have not observed it on a POSIX host, where realpath and pwd -P may well agree and the mismatch would not arise. The reconciliation is correct either way — it only ever normalizes — but the necessity argument above is Windows-grounded.
I have not resolved either thread.
|
Claude finished @kyle-sexton's task in 5m 11s —— View job Code review completeReviewed the current state of all 4 changed files (
Note on verification: I could not run Findings1 inline finding posted on
Traced through and correct, no issues found
Both inline threads from the previous round (the |
|
Claude finished @kyle-sexton's task in 4m 1s —— View job Security review completeScope: the 4 files changed in this PR at HEAD IMPORTANT (confidence: plausible) — the no-git
if dir="$(cd "$hint" 2>/dev/null && pwd -P)"; then
while :; do
if [[ -e "$dir/.git" ]]; then
printf '%s' "$dir"
return 0
fi
parent="$(dirname "$dir")"
[[ "$parent" != "$dir" ]] || break
dir="$parent"
done
fi
if [[ -n "${CLAUDE_PROJECT_DIR:-}" ]]; then
printf '%s' "$CLAUDE_PROJECT_DIR"
return 0
fiTwo issues compound here, both already latent in this PR's design and neither addressed by
Impact chain: Practical blast radius stays narrow — it requires (a) git absent, or the edited file sitting outside any git working tree git itself can discover (the documented target scenario for this whole fallback), and (b) some other writer controlling a directory above the project root ( This was flagged by an earlier automated review on this same PR (as "narrow preconditions: git absent AND Other things checked, no issues found
|
…ll open
The nested no-git case landed here is anchored by CLAUDE_PROJECT_DIR. Two
neighbours of it are not covered and do not pass.
CLAUDE_PROJECT_DIR UNSET. Not an exotic variant: the membership scope this PR
fixes is itself gated on `[[ -z "${CLAUDE_PROJECT_DIR:-}" ]]`, and the primary
no-git fixture runs unset — so unset is the configuration the fix is about, and
a root taken from CLAUDE_PROJECT_DIR cannot serve it. Nothing but the
filesystem can anchor the root there.
The opt-in PRE-CHECK's own root resolution, on the path that runs before jq
exists. No case in this file can reach it: every jq-absence case runs with git
present, every git-absence case runs with jq present. With both absent and the
file nested, a root that collapses to the file's own directory reads a
repository that DID opt in as one that never did, and swallows the jq notice it
is owed. The existing config-less pair pins the opposite direction, so this
cannot pass by the hook merely having stopped warning.
Both fail on this commit by design; the fix follows.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…from a variable Replaces the CLAUDE_PROJECT_DIR override with resolve_repo_root, which recognizes hook::repo_root's fallback by its signature (an answer equal to the hint) and then performs the walk git's own discovery performs: upward for a `.git` entry, accepted as a directory for an ordinary clone or as a FILE for a linked worktree or submodule. git's answer is returned untouched whenever git produced one. CLAUDE_PROJECT_DIR stays as a last resort, for a project that is no working tree at all, so the previous behaviour is subsumed rather than dropped; when nothing resolves, the hint stands, which keeps the documented out-of-tree bound true. Three things follow from resolving the root rather than reading it off a variable. The case this PR is about is covered. The membership scope is gated on CLAUDE_PROJECT_DIR being UNSET and the no-git fixture runs unset, so a root taken from that variable cannot serve the configuration the gate exists for. The opt-in pre-check gets the same resolution. It runs before jq exists and had the identical defect; with git and jq both absent a nested file made it read an opted-in repository as one that never opted in and swallowed the jq notice. The `[[ "$REPO_ROOT" == "$(dirname "$FILE")" ]]` guard goes away, and with it two problems that are one guard seen from two sides: it is true only for a file at the repository root, so it spawns a redundant `git rev-parse` on every root-level Markdown edit wherever the payload's path spelling matches git's, and it is false for every nested file, so a nested fixture cannot exercise the branch behind it at all. The shared hook::repo_root is deliberately NOT changed: it is a synced library whose edit fans out to sixteen plugin copies and their manifest versions under CI's --check-bump, and it is under concurrent edit for ccp#2122. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…esort resolve_repo_root keeps CLAUDE_PROJECT_DIR below the filesystem walk, for a project that is no working tree at all — an unpacked archive, a vendored copy — where the walk finds no `.git` to stop at. Every other fixture in this file lives in a real git tree, so nothing else can reach that branch; the fixture is deliberately outside $REPO for that reason. Not vacuous: with the clause cut from resolve_repo_root the same fixture is skipped rather than linted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… rewrite of 0.11.1 The earlier form of this change was cut against the #2121 branch before it merged, so it edited the then-unreleased entry in place. That version has since shipped, and rewriting a released entry would misstate what it did. Restores 0.11.1 verbatim and gives this change its own entry, with the manifest bumped to match — the plugin version is the update cache key, so an unbumped plugin never delivers the change. The number is whatever sits above the highest released entry at rebase time; main has taken several while this branch was in review, so it is deliberately not restated here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ave the repo Review found that resolving the root from the filesystem introduced a new failure mode, and it reproduces: an in-repository symlink whose target lives outside the tree. Making discovery succeed is what puts a file in front of --fix, so the repository's own config opened the gate and the linter followed the link and rewrote a file outside the repository. Measured with the real markdownlint-cli2 v0.23.2 and md5 of the target, and the same measurement shows the root-level form of the escape was already reachable before this branch. The cause is that without git this scope could not ask in_git_working_tree anything, so containment was not checked at all — and a symlink is the one shape whose lexical parent (inside the repository, where the config lives) and physical parent (outside it) disagree. Containment is now decided from the filesystem instead of skipped. It runs only where the physical path differs from the lexical one, which for an ordinary file it never does, so a git-less repository lints exactly as before: an undecidable GIT verdict still lints, an escape the filesystem can prove does not. physically_inside canonicalizes BOTH operands through `cd … && pwd -P`. That is load-bearing rather than incidental. hook::physical_path resolves via realpath, which leaves /tmp as /tmp where pwd -P resolves it to the underlying directory — hook-utils.sh documents that divergence on hook::under_temp_root — so comparing a physical_path result against a pwd -P root compares spellings, not containment, and rejects a file that is plainly inside the tree. pwd -P is also the currency markdownlint_config_discoverable, CONFIG_ROOT and CONFIG_TARGET_DIR already use, and unlike hook::physical_path it cannot silently degrade to the lexical path when no canonicalizer exists. The guard sits in the membership scope, before config discovery rather than at the REPO_ROOT line: discovery is anchored on the file's lexical parent independently of REPO_ROOT, so moving the root alone cannot contain the escape. Also fixes the signature check in resolve_repo_root to recognize both suffix forms hook::repo_root strips, `/.claude` and `\.claude`, not only the first. Tests cover both nestings and assert on LINK SURVIVAL rather than target bytes: the suite's stub rewrites with `sed -i`, which replaces a symlink instead of writing through it, so an unchanged target proves nothing there while a surviving link proves --fix never ran. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 2m 37s —— View job Security review complete
Scope: the 4 files changed in this PR at HEAD No security-relevant code changed since the last review round. I diffed IMPORTANT (confidence: plausible) — still open: the no-git This is the same finding posted on this thread twice already (05:12:59 and 13:29:13) and it is unchanged at current HEAD — if dir="$(cd "$hint" 2>/dev/null && pwd -P)"; then
while :; do
if [[ -e "$dir/.git" ]]; then
printf '%s' "$dir"
return 0
fi
parent="$(dirname "$dir")"
[[ "$parent" != "$dir" ]] || break
dir="$parent"
done
fi
if [[ -n "${CLAUDE_PROJECT_DIR:-}" ]]; then
printf '%s' "$CLAUDE_PROJECT_DIR"
return 0
fiTwo compounding issues, neither addressed by the
Impact chain is unchanged from the prior report: Worth an explicit decision: content-validate a file-form Other things checked, no issues found
|
|
Claude finished @kyle-sexton's task in 2m 0s —— View job Code review completeThis lane's job on
Verified, not just diffed: What changed since the last roundThe PR body's predicted version collision with #2135 happened as anticipated:
No new findings. The merge-conflict resolution is mechanically correct and consistent with what the PR body described as the expected coordination outcome with #2135. |
62765c6 to
193c9d2
Compare
|
Claude finished @kyle-sexton's task in 3m 3s —— View job Code review complete
No content changes since the last full review round. I compared the blob SHAs of all 4 changed files at current HEAD All four are byte-identical — Since the diff against the last-reviewed state is empty, I have nothing new to add on correctness, error handling, or maintainability — both P1s from earlier rounds (the git-absent symlink escape, and the Two test-coverage gaps flagged by this lane in earlier rounds are still open at this content (not re-flagging as new inline comments since nothing changed to re-review, just noting they remain unaddressed):
The unbounded/unauthenticated |
|
Claude finished @kyle-sexton's task in 2m 46s —— View job Security review complete
Scope: the 4 files changed in this PR at HEAD IMPORTANT (confidence: plausible, narrow preconditions) — the unauthenticated This is a real escalation of the finding posted twice already on this thread (05:12:59, 13:29:13, 14:26:36), not a duplicate of it. What's new at this head:
Concrete chain, under the same preconditions the prior finding already required (git absent from
So the Blast radius is unchanged from the prior rounds' assessment — still requires git absent, Worth deciding explicitly, same remedy shape suggested in the last two rounds: content-validate a file-form Other things checked, no issues found
Note on verification: I could not execute |
Rebased onto current
|
| arm | canary (nested regular) | nested symlink | root symlink |
|---|---|---|---|
main 28768645 |
unchanged — the defect this PR fixes | UNCHANGED (vacuous) | REWRITTEN 3c62fb6e→60fdfc85 |
branch b62c8148 (pre-containment) |
REWRITTEN | REWRITTEN 3c62fb6e→60fdfc85 |
REWRITTEN 3c62fb6e→60fdfc85 |
branch 193c9d2e (HEAD) |
REWRITTEN | UNCHANGED 3c62fb6e |
UNCHANGED 3c62fb6e |
One row needs an honest caveat rather than a blanket dismissal. On the main arm the canary correctly does not fire — that is precisely the bug this PR fixes, so main's nested row is vacuous. Its root row is still genuine evidence: a REWRITE can only happen if the linter ran, so it is self-evidencing regardless of the canary. That row is the #2134 finding re-confirmed on today's main.
Suite
PASS=141 FAIL=0
ok: git absent: a nested escaping symlink is skipped, not handed to --fix
ok: git absent: a root escaping symlink is skipped, not handed to --fix
ok: git present: the override stays inert — the hook resolved REPO_ROOT to the git toplevel
That third line is #2128's negative test, which landed on main after this branch was cut and which this PR deletes the override behind. It passes unchanged: resolve_repo_root returns git's own answer whenever git produced one, so REPO_ROOT is still the git toplevel. I also ran that case in isolation against both hooks — main reports INNER, this branch reports INNER.
Also clean: shellcheck -x -S warning, check-shell-portability.sh --paths, check-silent-skips.sh --paths, sync-hook-utils.sh --check (all 16 copies match), check-changelog-parity.sh --check-bump, and markdownlint-cli2 on the CHANGELOG.
The intermittent from earlier is now identified
I previously reported an unnamed PASS=133 FAIL=1 I could not reproduce. It is telemetry/slow-sink, a wall-clock assertion that the hook does not block on a slow sink. It fired once during this round at delta 12870ms. It is not caused by this branch — measured on main's own hook, untouched by me, 8 samples:
delta 7764ms 1472ms 888ms 16007ms 174ms 26605ms -2202ms 13723ms
Four of eight exceed the threshold on unmodified main, with baseline invocation times swinging 1572ms→13343ms. A genuine fd1 leak would pin the delta at ≈8000ms on every sample; deltas of 26.6s against an 8s sleep, and occasional negative deltas, are host contention. The final 141/0 run above did not trip it. Flagging rather than filing, since it is a pre-existing test-robustness issue in someone else's lane.
Unverified, unchanged
The POSIX-host git rev-parse spawn count is still simulated via git's path spelling on Windows, never observed on a POSIX host. The /tmp-versus-pwd -P divergence behind physically_inside is likewise Windows-grounded — the reconciliation is correct on any host because it only ever normalizes, but the argument for its necessity is not verified on POSIX.
I have not resolved any thread.
…hellCheck passes The hygiene lane runs ShellCheck with no severity floor, so SC2249 (info, "Consider adding a default *) case") failed the whole lane on markdown-format.test.sh:584 — the only failing check on this PR. Confirmed from the job log rather than inferred: run 31399762274, job hygiene, `shellcheck=failure` with SC2249 as the sole finding. `shellcheck -x` on the file is rc 0 after this change. The branch exits rather than falling through. A silently unhandled case there would leave _link unset from the previous iteration and report a PASS for a symlink case that never ran, which is the failure shape these two cases exist to catch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 1m 6s —— View job Code review complete
Only one new commit since the last review round, and it's a mechanical ShellCheck fix. Diffing The new commitAdds a for _case in nested root; do
case "$_case" in
nested) _link="$REPO/docs/escapeNoGitNested.md" ;;
root) _link="$REPO/escapeNoGitRoot.md" ;;
*)
echo "unhandled escape case: $_case" >&2
exit 1
;;
esacThis closes SC2249 ("consider adding a default case"), the sole ShellCheck finding blocking the hygiene lane per the commit message (job No new findings. This commit doesn't touch The two test-coverage gaps this lane flagged in earlier rounds are still open and unaffected by this commit:
Verification note: I could not run ShellCheck or the test suite myself in this environment (no Bash execution beyond read-only |
|
Claude finished @kyle-sexton's task in 1m 27s —— View job Security review complete
Scope: the 4 files changed in this PR at HEAD Nothing security-relevant changed since the last full round on this thread. claude-code-plugins/plugins/markdown-format/hooks/markdown-format.test.sh Lines 584 to 599 in fe5d6a7 That's test-fixture control flow, not production logic reachable from the hook, and doesn't change any trust boundary. IMPORTANT (confidence: plausible, narrow preconditions) — still open: the unauthenticated Raised on this thread four times already (05:12:59, 13:29:13, 14:26:36, and escalated at 14:44:41) and unchanged at this HEAD. if dir="$(cd "$hint" 2>/dev/null && pwd -P)"; then
while :; do
if [[ -e "$dir/.git" ]]; then
printf '%s' "$dir"
return 0
fi
parent="$(dirname "$dir")"
[[ "$parent" != "$dir" ]] || break
dir="$parent"
done
fi
if [[ -n "${CLAUDE_PROJECT_DIR:-}" ]]; then
printf '%s' "$CLAUDE_PROJECT_DIR"
return 0
fi
The escalation from the 14:44:41 round still holds at this HEAD: Blast radius is unchanged from prior rounds: requires git absent from Other things checked, no issues found
Note on verification: as in prior rounds on this thread, I could not execute |
main advanced again (#2140, #2149 and later). No conflicts this pass — the markdown-format changelog and manifest both auto-merged, and 0.11.4 is still one patch above main's 0.11.3. check-changelog-parity.sh --check-bump origin/main is clean on the merged tree. Merging rather than rebasing, and pushed fast-forward on top of the branch's current head. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 1m 9s —— View job Security review complete
Scope: the 4 files changed in this PR at current HEAD Nothing changed since the last review round.
IMPORTANT (confidence: plausible, narrow preconditions) — still open: the unauthenticated Raised on this thread five times already and unchanged at this HEAD. if dir="$(cd "$hint" 2>/dev/null && pwd -P)"; then
while :; do
if [[ -e "$dir/.git" ]]; then
printf '%s' "$dir"
return 0
fi
parent="$(dirname "$dir")"
[[ "$parent" != "$dir" ]] || break
dir="$parent"
done
fi
if [[ -n "${CLAUDE_PROJECT_DIR:-}" ]]; then
printf '%s' "$CLAUDE_PROJECT_DIR"
return 0
fi
This still matters beyond config discovery: This has now been open across six review rounds on this thread without a code change addressing it. Not re-filing a new inline comment since nothing changed to re-review — restating so it isn't lost in the merge noise. Worth an explicit decision (accept the documented narrow precondition, or close it with a Other things checked, no issues found
Note on verification: as in prior rounds, I could not execute |
…y gate The --check-bump heading probe exited on first match while rendered_lines was still writing. Under the script's pipefail, the writer's SIGPIPE death (exit 141) became the pipeline's status, so a correctly documented bump in any changelog larger than one stdio buffer — and the newest heading is always near the top — was reported as UNDOCUMENTED BUMP. gawk, the CI runner's awk, loses that race deterministically; the suite's small fixtures fit in one buffer and never tripped it, which is how the gate shipped green at PASS=55 and then failed the first real bump PR (#2130). Scan the whole input instead of exiting on first match, and add a large-changelog fixture (~260 KB, entry near the top) that fails against the early-exit reader. Closes #2158 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…y gate The --check-bump heading probe exited on first match while rendered_lines was still writing. Under the script's pipefail, the writer's SIGPIPE death (exit 141) became the pipeline's status, so a correctly documented bump in any changelog larger than one stdio buffer — and the newest heading is always near the top — was reported as UNDOCUMENTED BUMP. gawk, the CI runner's awk, loses that race deterministically; the suite's small fixtures fit in one buffer and never tripped it, which is how the gate shipped green at PASS=55 and then failed the first real bump PR (#2130). Scan the whole input instead of exiting on first match, and add a large-changelog fixture (~260 KB, entry near the top) that fails against the early-exit reader. Closes #2158 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…y gate The --check-bump heading probe exited on first match while rendered_lines was still writing. Under the script's pipefail, the writer's SIGPIPE death (exit 141) became the pipeline's status, so a correctly documented bump in any changelog larger than one stdio buffer — and the newest heading is always near the top — was reported as UNDOCUMENTED BUMP. gawk, the CI runner's awk, loses that race deterministically; the suite's small fixtures fit in one buffer and never tripped it, which is how the gate shipped green at PASS=55 and then failed the first real bump PR (#2130). Scan the whole input instead of exiting on first match, and add a large-changelog fixture (~260 KB, entry near the top) that fails against the early-exit reader. Closes #2158 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…y gate (#2159) Closes #2158 ## Problem `changelog-parity-gate` failed PR #2130 twice with `UNDOCUMENTED BUMP: markdown-format went 0.11.3 -> 0.11.4 ...` even though `plugins/markdown-format/CHANGELOG.md` carries `## [0.11.4]` at line 6, column one — a required merge gate confidently asserting the opposite of the truth, while the same command passed locally. PR #2135 then failed the same gate on **every one of its sixteen bumped plugins** (smallest flagged changelog: 15 KB). The regression landed on `main` at 15:19:19Z in #2154 and blocks **every PR that bumps a plugin whose changelog exceeds roughly one stdio buffer (~4 KB)** — which newest-first ordering makes essentially all of them. ## Blast radius — precisely Confined to the `--check-bump` path: `has_heading` is defined inside that branch and called in exactly two places (the head-side check and the base-side `git show "$base:$changelog" | has_heading`). `--check` and `--check-order` read changelogs through `changelog_versions`, whose `grep -oE` stages drain stdin with no early exit and cannot take SIGPIPE. So the failure class is exactly "PRs that bump a manifest version"; both failing call sites go through the one function this PR fixes. ## Root cause `has_heading` runs a pipeline under `set -o pipefail` whose reader `exit`s on first match: ```bash rendered_lines - | awk -v h="$heading" 'index($0, h) == 1 { found = 1; exit } END { exit !found }' ``` The newest heading sits near the top, so the reader exits while `rendered_lines` is still writing; the writer dies of SIGPIPE (141) and pipefail reports the pipeline — the FOUND heading — as a failure. Reproduced deterministically in an `ubuntu:24.04` container at the exact CI merge commit `ba4b72fb`: `PIPESTATUS=141 0` and the byte-identical CI error under **gawk** (what the `ubuntu-24.04` runner resolves `/usr/bin/awk` to — gawk outranks mawk in the alternatives system, and only the gawk mechanism explains CI failing 15 KB files). mawk survives the closed pipe and passes at every size tested, and Windows/MSYS process timing lets the writer finish first — which is why the failure existed only in CI. The suite's 55 fixtures all fit in one buffer — hence `PASS=55` in the very job that then failed on the real file. ## Fix The reader consumes to EOF; `END { exit !found }` decides. Correct **by construction**: no reader exits early, so no writer can ever take SIGPIPE, under any awk — the failure is impossible, not rarer. Chosen over restoring the pre-#2154 single-awk form because it preserves the one-tracker-three-modes property (`rendered_lines` shared by all modes, so they cannot drift). Both `has_heading` call sites are covered (same function); the script has no other early-exiting reader downstream of a pipe (`changelog_versions` greps drain stdin; the `grep -m1` format probe reads a file directly, not a pipe). ## Regression fixture — with its fails-against-unfixed proof, per engine New `--check-bump` case: ~260 KB changelog, new `## [1.1.0]` entry near the top, expected pass. 260 KB deliberately exceeds the 64 KB pipe **capacity**, so against the unfixed script the writer blocks mid-write and the SIGPIPE is deterministic, not a winnable race. | script | gawk | mawk | |---|---|---| | unfixed | **FAIL=1** (exact #2130 error text) | PASS (mawk survives the closed pipe) | | fixed | 56/56 | 56/56 | The fixture's guard is therefore **engine-conditional — it discriminates only where `awk` resolves to gawk**, which is what the runner resolves; this is recorded in the fixture comment. The fix itself is engine-independent. Windows local: 56/56 fixed. ## Related - #2158 — the defect issue this closes - #2154 — shipped the early-exit reader; this is its first contact with a production-size changelog - #2130, #2135, #2155 — blocked by this regression; once this lands, their recomputed merge refs carry the fixed gate and need no branch-side changes - Known adjacent gap, tracked separately and deliberately NOT fixed here: the gate cannot see a **deleted** predecessor heading — a change set that renames the top heading in place (rather than adding a new one above it) destroys a shipped release section while `--check` and `--check-order` both pass 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 1m 0s —— View job Security review complete
Scope: the 4 files changed in this PR at current HEAD Nothing changed since the last review round.
IMPORTANT (confidence: plausible, narrow preconditions) — still open: the unauthenticated Raised on this thread seven times now, unchanged at this HEAD. if dir="$(cd "$hint" 2>/dev/null && pwd -P)"; then
while :; do
if [[ -e "$dir/.git" ]]; then
printf '%s' "$dir"
return 0
fi
parent="$(dirname "$dir")"
[[ "$parent" != "$dir" ]] || break
dir="$parent"
done
fi
if [[ -n "${CLAUDE_PROJECT_DIR:-}" ]]; then
printf '%s' "$CLAUDE_PROJECT_DIR"
return 0
fi
This matters beyond config discovery: This has now been open across seven review rounds on this thread without a code change addressing it. Not re-filing as a new inline comment since nothing changed to re-review — restating so it isn't lost in the merge noise. Worth an explicit decision (accept the documented narrow precondition, or close it with a Other things checked, no issues found
Note on verification: as in prior rounds, I could not execute |
…ing guards (#2135) Closes #2122 ## Update — `main` moved under this PR, and the disposition changed with it **#2120 merged (`fd075c27`), and it fixed the same function with the opposite value disposition: it STRIPS every NUL out of a value where this branch TRUNCATED at the first one.** The PR went `DIRTY`. Resolved by merging `origin/main` into the branch — never a rebase, since force-push is blocked here twice over. **The resolution keeps `main`'s strip and this branch's flag plus fail-closed guards.** That is additive over `main` rather than a choice between the two sides, and it is what this body already argued for in its own words: the disposition is *immaterial for this PR's own two callers*, which refuse on the flag before reading a value, while `main` now carries the ten scanner-class callers #2120 converted, none of which consults the flag. Truncating would have hidden a credential placed after a NUL from `secret-pattern-detection` and `hardcoded-path-check`. **Everything below that says "truncate" describes the pre-merge branch; the shipped behaviour is strip + flag.** ### The textual merge git produced was silently fatal, and was not taken git auto-merged the function body into a hybrid carrying BOTH `main`'s per-filter `split("\u0000") | join("")` **and** this branch's array-level `explode | .[0:(index(0) // length)] | implode`. Strip runs first, so `index(0)` looked at a value with no NUL left in it and **the flag read `0` on every payload** — the guards would never have fired, with no conflict marker and no test of the pre-merge branch able to see it. The flag is now computed from the untouched values with the strip applied after, and both the library and the guard comments say the ordering is load-bearing, because it is exactly what the next textual merge will get wrong again. ### Why the flag and the guards are still needed after #2120 #2120 closed the fail-open for the CONTENT guards. It did not close the COMMAND guards: stripping SPLICES the bytes either side of the NUL into a token the payload never carried contiguously, and the guards then match against that token. Re-measured at the hook boundary, `origin/main` at `fd075c27` versus this tree, same script, same host, on fixtures whose NUL is a real byte — verified by decoding each fixture and counting the byte (`jq -j .tool_input.command | tr -dc '\u0000' | wc -c` = 1) rather than trusting that the escape survived construction: | payload | `main` | this change | | --- | --- | --- | | `git commit --no-verify<NUL>x` | **0 ALLOWED** | **2 blocked** | | `git push --force<NUL>x` | **0 ALLOWED** | **2 blocked** | | a lone NUL | **0 ALLOWED** | **2 blocked** | | a trailing NUL | **0 ALLOWED** | **2 blocked** | | `git commit --no-veri<NUL>fy` | 2 blocked | 2 blocked | | clean `--no-verify` | 2 | 2 | | clean `--force` | 2 | 2 | | harmless (`git status`) | 0 | 0 | Identical on both guards. **The fifth row is stated, not counted:** the splice happens to reassemble a real `--no-verify` there, so `main` already blocks it and it evidences nothing about this change. The live rows are the first four, and the first two are the ones that matter — a real `--no-verify` and a real `--force` that `main` waves through. No clean command changed verdict in either direction. ### Tests re-pointed rather than deleted Every assertion this branch wrote against truncation was measuring a value the helper no longer produces, so each was rewritten for strip and two new cases were added: the splice (`--no-verify<NUL>x` -> the single token `--no-verifyx`), and an ALL-NUL value, which strips to empty — that case, and not a leading NUL, is the real reason both guards consult the flag ahead of their empty-command skip. The guard suites keep every NUL row at exit 2; the verdict never depended on the disposition, only its justification did, and one mislabelled row was corrected accordingly. ### Conflicts and versions - `lib/hook-utils.sh` — header comment and jq program, resolved by hand. - The 16 vendored copies were **regenerated with `scripts/sync-hook-utils.sh`**, not hand-resolved; `--check` reports 16/16 byte-identical. - 16 CHANGELOGs where both sides claimed the same version: this branch's entry moves up one patch above `main`'s and is rewritten for the resolved design. - **All 16 `plugin.json` files had auto-merged to `main`'s number, leaving no bump at all** — no conflict, only `--check-bump` catches it, exactly the trap flagged below. Re-bumped: `guardrails 0.23.1 -> 0.23.2`, `markdown-format 0.11.2 -> 0.11.3`, `source-control 0.51.2 -> 0.51.3`, patch bumps for the other 13. - **Coordination with #2130:** it also bumps `markdown-format` to `0.11.3`. Whichever merges second must re-bump. ### `main` moved twice more: three merges, and one of them was silently lossy `main` landed #2147, then #2140 and #2149, while this PR sat. Three merge passes, no rebase at any point. Second pass: #2147 took `guardrails` to `0.24.0` and edited `block-dangerous-git.sh`, which this branch also edits — resolved by keeping main's three-field `hook::jq_fields "$INPUT" '.tool_input.command' '.cwd' '.tool_name'` call verbatim and appending this branch's NUL block after it. Third pass: one changelog conflict on `source-control`. Every plugin manifest had auto-merged to main's number with no bump on **both** passes. **The second pass exposed a defect this branch had introduced, and it is worth reading even if you skip the rest.** An earlier commit here accidentally wrote a **real NUL byte** into `plugins/guardrails/CHANGELOG.md` — a `\u0000` that was meant to be literal text in a prose description of the fixtures. git classifies any file containing a NUL as **binary**, so the textual three-way merge never ran on that changelog: it kept ours wholesale and **silently discarded main's entire `0.24.0` section**, with no conflict marker and nothing in `git status` to distinguish it from a file that merged cleanly. It was caught by counting NUL bytes across the touched files, not by reading the diff. The byte is gone, the section is restored, and the changelog's `0.24.1` entry now sits above main's `0.24.0`. That is a mistake this PR made, not a pre-existing one, and it is reported rather than quietly fixed because the failure mode generalises: **a NUL in a tracked text file turns every future merge of that file into a silent take-ours.** In a repository whose CHANGELOGs are the merge-conflict surface for every shared-library change, that is worth knowing independently of this fix. ### Incidental, and relevant to the "what I could NOT verify" list below While posting a review reply, the **harness itself refused a tool call** whose `command` field carried a stray control character, with `command contains control characters that would be hidden in the approval dialog`. That is a live observation of the validation the list below names as unverified — it fires, and it fires on the `command` field. It is **not** the discriminating probe: it says nothing about whether that validation runs before or after PreToolUse hooks, and nothing about whether the rejected class includes NUL specifically rather than the control characters it does cover. Recorded as an observation, not as evidence that the guards are unreachable. Nothing in this change leans on it in either direction. ### Gates re-run after the merge `sync-hook-utils.sh --check` (16/16) - `sync-hook-utils.sh --check-bump origin/main` - `check-changelog-parity.sh --check` / `--check-bump origin/main` / `--check-order` - `shellcheck -x` with **no severity floor** on `lib/hook-utils.sh`, the `bash-format` vendored copy, both guards and all three test files (rc 0 — this is what the two open review threads reported failing; the jq-variable spelling they flagged is gone from the current program text) - `shfmt -d -i 2` (rc 0). Suite results after the merge are in the thread below. ## The defect `hook::jq_fields` frames its fields with a NUL delimiter drawn from the same byte space as the values it separates. A JSON NUL escape inside a value splits that value in two, the cardinality check `((${#values[@]} == $#)) || return 1` fires, and both real callers spell that `|| exit 0` — a PreToolUse **ALLOW**, emitted with no diagnostic of any kind. One correction to the issue's mechanism, because it moves where the fix belongs. The collision is **reliably detected**, not intermittently: every NUL adds exactly one record, so the count is always `N + k` for `k >= 1` and the check never misses. The defect therefore never lived in the library's return value. It lives in **one exit path serving two conditions with opposite correct responses** — "jq is absent or cannot parse this" (where allowing is the documented, deliberate behaviour) and "this payload carries a NUL" (where allowing is wrong). Separating those two is the fix. ## Design **jq truncates each value at its first NUL and reports the fact; the caller owns the verdict.** - `lib/hook-utils.sh` — each filter becomes `... | explode | .[0:(index(0) // length)] | implode`. The separator then cannot occur inside a value, so the record count no longer depends on what a parseable payload holds. - A leading record carries the NUL flag, computed from the untruncated values and emitted by the **same** jq program, so reporting it costs no second spawn. It surfaces as `HOOK_JQ_FIELDS_NUL`, assigned in the same unconditional block that resets `HOOK_JQ_FIELDS` — above all three return paths, so no early return can leak a stale `1`, which in a guard would mean blocking a clean payload on the strength of an earlier one. - `block-no-verify.sh` and `block-dangerous-git.sh` fail **CLOSED** on that flag, **before** their empty-command skip, because the helper truncates at the first NUL and a leading one therefore leaves an empty value that would otherwise be waved through as "no command". ### Why fail CLOSED, and why that argument does not depend on the executor **No executor-fidelity claim is made here, in either direction.** Two behaviours were measured and they disagree, and which of them a hook payload actually reaches has **not been traced by anyone**: | measured | result | | --- | --- | | bash parsing a command it reads (stdin, script file) | **discards** the NUL — `echo ha<NUL>rd` prints `hard`, and `--no-verify<NUL>x` becomes `--no-verifyx` | | a NUL inside an argv word handed to `execve` | the string simply ends there | | Node v24.18.0 `child_process` — argv, `shell: true`, and `execSync` | **refuses** outright, `ERR_INVALID_ARG_VALUE: must be a string without null bytes`, while the same calls with a clean string run normally | An earlier draft of this PR argued that truncation was right *because the executor truncates*. That was wrong — it generalised the argv case to a path that is not known to be the one in use. **The correct argument is that the design does not need it:** failing closed on the flag is correct under deletion, under truncation, and under refusal alike, so it cannot be invalidated by tracing the path later. That is the whole case for it. Matching the value would need the trace; refusing does not. ### Truncate rather than delete, on grounds that appeal to no shell Truncation never fabricates a token the payload did not carry contiguously, and when a caller forgets the flag it is the *content* class that degrades rather than the command class — a matcher sees a prefix rather than a joined token that matches nothing. **For this PR's own two callers the choice is immaterial: they refuse on the flag before reading a value at all.** It is the conservative default, not the accurate one, and the flag is the load-bearing part. ### Why the library does not block on its own It is sourced by 15 other plugins, formatters among them, for which exiting 2 would be wrong; and a sourced library calling `exit` on its caller's behalf is hidden control flow. Policy stays with the caller and the library only reports the fact. ### Rejected alternatives | Alternative | Why not | | --- | --- | | Delete the NUL (`map(select(. != 0))`) | Fabricates contiguity the payload did not have, and inverts which caller class degrades unsafely when a hook forgets the flag; see above. Not rejected on executor grounds. | | `gsub` / `split`+`join` on a NUL | Both work on jq 1.8.2 here, but each puts a NUL inside the jq **program** text — a regex pattern and a string literal. A construct whose behaviour varied across jq builds would fail EVERY payload: a universal fail-open, strictly worse than the payload-dependent one. `explode`/`implode` use integer comparison only, with no NUL anywhere in the program. This is a reason, not a measurement — see the unverified list. | | Length-prefixed framing | Needs `read -N` (bash 4.1+); this lib supports 3.2+. | | An explicit emitted count | Redundant once the separator is absent from the value space. | | Per-field `@base64` | Needs a `base64` binary; only `jq` is a documented prerequisite. | | `@sh` + `eval` | Puts payload-derived text through `eval`. | | Fail closed inside the library | Impossible without the library exiting on its caller's behalf, which is wrong for the 15 other plugins. | ## Scope **This is a shared-library change, and the repo's own gate makes it 55 files.** `plugins/guardrails/hooks/hook-utils.sh` is a **vendored copy**; `lib/hook-utils.sh` is the source of truth. CI enforces `scripts/sync-hook-utils.sh --check` (all 16 copies byte-identical) and `--check-bump` (every carrying plugin bumped when the lib changes), so editing only the guardrails copy would fail CI. Precedent: 9b90e35, 50 files. Hence 16 vendored copies, 16 `plugin.json` bumps and 16 changelog entries, plus the lib, its test, the two guards, their two test files and the guardrails README. **`hook::jq_field` — SINGULAR — is untouched.** It is a separate two-line function; there is no shared internal the two route through. `grep -rn "hook::jq_field " --include=*.sh plugins/`, with the vendored copies excluded, finds **22 call sites across 12 files** in `claude-ops`, `context-guard` and `source-control`. None of them are touched. `git diff origin/main -- lib/hook-utils.sh` mentions `hook::jq_field` on exactly two lines, both of them the same doc-comment cross-reference inside the *plural* function's header ("Values are CR-stripped, as in `hook::jq_field`"); the singular function's own body appears nowhere in the diff. **Blast radius is exactly the two guards.** **No other plugin is affected by the truncation.** `grep -rn "hook::jq_fields" --include=*.sh .`, excluding the 16 vendored copies and `lib/hook-utils.*`, returns exactly two call sites — both in this PR. Every other hit across the 16 plugins is the doc comment in the vendored library. Nothing round-trips a value into a file, and nothing compares a length or hash against one. **Versions**, taken against `origin/main` at the time of the last rebase: `guardrails 0.23.0 -> 0.23.1`, `markdown-format 0.11.1 -> 0.11.2`, `source-control 0.51.1 -> 0.51.2`, and plain patch bumps for the other 13. Worth flagging for anyone rebasing a sibling branch: when a plugin's version moved on `main` mid-flight, `git` **auto-merged the manifest to main's number**, silently leaving no bump at all — no conflict, and only `sync-hook-utils.sh --check-bump` catches it. That happened three times here. #2120 is still open against the same guardrails files and owes a re-bump. ## Two caller classes want opposite dispositions — which is why there is a flag This is the strongest argument for the design, and it is demonstrated rather than theoretical. #2120 has independently fixed the same function with the **opposite** disposition: at its head `9fb8383d`, `hook::jq_fields` does `... | tostring | split("<NUL>") | join("")` — it **strips**. Neither disposition is simply right, because the two caller classes disagree: | payload | under strip | under truncate | | --- | --- | --- | | `content: harmless<NUL>aws_secret=AKIA…` (a scanner) | secret is joined and **scanned** | secret is cut off and **invisible** | | `command: --no-verify<NUL>x` (a guard) | joins to `--no-verifyx`, matches nothing, **allowed** | leaves `--no-verify`, **blocked** | (Which of those two readings the executor would agree with is untraced, and is not the argument — see above. The point is only that a caller ignoring the flag degrades unsafely in one class or the other, depending which disposition the helper picks.) Both halves measured. The command half is the boundary table below. The content half I measured by driving the helper directly, since no shipped hook reads `.tool_input.content` through it on `main`: ``` payload: .tool_input.content = "harmless preamble<NUL>aws_secret=AKIA…" this branch (truncate) rc=0 flag=1 value=[harmless preamble] credential NOT visible 468bb2d (base) rc=1 flag=- value=[<none>] credential NOT visible ``` **So yes — truncation loses post-NUL content for a scanning caller.** Stated plainly because it is a real consequence of this design. It is not a regression (the base loses it too, and additionally allows), and truncation is still the chosen default: it keeps the *command* class safe when a caller ignores the flag, where strip keeps the *content* class safe instead. Strip inverts which class fails unsafely; it does not remove the failure. Neither is chosen on executor grounds. **A single disposition cannot serve both callers. The flag is what resolves it** — the helper reports, and each caller decides: a command guard refuses outright, a content scanner refuses the write rather than scanning a value it knows is incomplete. Either way the credential never lands. ### The count, measured on `9fb8383d` **Every one of the ten hooks #2120 converts calls `hook::jq_fields`. Zero of them consult any NUL signal. Six own an `exit 2` verdict:** | hook | `jq_fields` calls | flag checks | `exit 2` paths | | --- | --- | --- | --- | | `secret-pattern-detection` | 2 | **0** | 2 | | `hardcoded-path-check` | 2 | **0** | 2 | | `block-convention-violation` | 2 | **0** | 3 | | `block-hook-bypass` | 2 | **0** | 2 | | `block-noncanonical-commit` | 2 | **0** | 5 | | `cli-flag-verify` | 2 | **0** | 1 | | `skill-reference-verify` | 3 | **0** | 0 | | `stale-path-verify` | 3 | **0** | 0 | | `flag-commit-pr-skill-bypass` | 2 | **0** | 0 | | `workflow-resilience-check` | 2 | **0** | 0 | Zero flag checks is expected — the flag does not exist on their branch. The point is what it implies for whichever of us merges second: **merge order does not rescue it.** This PR first, then their rebase, and the scanning hooks receive truncated values with no flag check. Theirs first, then this one, and the same is true the moment strip becomes truncate. **A reader must not conclude that this PR makes that conversion safe. It does not.** Adding the flag checks to those ten hooks is a prerequisite for the conversion, not a follow-up — and it is theirs to do, since those hooks exist in converted form only on their branch. This PR deliberately does not touch them. `hardcoded-path-check.sh` is a **third** caller class worth calling out: it reads `.tool_input.content`, `.new_string` and `.new_source` **and** owns two `exit 2` paths, so it is both scanner and guard. Per-field reachability was checked separately and holds: at their head, both `secret-pattern-detection.sh` and `hardcoded-path-check.sh` reach `exit 2` through `.content` and through `.new_string`. (`hardcoded-path-check.sh` returns early unless `CLAUDE_PROJECT_DIR` is set, so a probe without it exits 0 on every payload and looks exactly like "not reachable".) #2123 needs nothing — its diff introduces zero `hook::jq_fields` call sites. **Merge coordination:** #2120 now also edits `lib/hook-utils.sh`, so this is a direct conflict on the same function rather than only on the manifest and changelog. Whoever merges second must **keep both correctness properties** — the flag and the fail-closed guards from here, and the scanning-caller requirement from there — rather than resolving by taking one side of the hunk. ## Evidence ### Hook boundary, before and after Real hooks, payload piped on stdin, exit code read. BEFORE is a `git archive` of `origin/main` at `468bb2d9` — re-measured after #2123 merged, because #2123 changed `plugins/guardrails/lib/powershell/ps-command.sh`, which both guards source. AFTER is this branch. Same script, same host. | case | before | after | | --- | --- | --- | | clean `git push --no-verify` / `git reset --hard` | 2 | 2 | | clean harmless (`echo hi` / `git status`) | 0 | 0 | | trailing NUL | **0** | **2** | | NUL splitting the flag (`--no-veri<NUL>fy`) | **0** | **2** | | NUL then junk (`--no-verify<NUL>x`) | **0** | **2** | | leading NUL | **0** | **2** | | NUL in an otherwise harmless command | **0** | **2** | Identical for both guards. No row where a clean command changed verdict. The `<NUL>x` row is the one that matters most: it is the payload that executes as the dangerous command. ### The leading-NUL row blocks for the right reason Identical truncated content, opposite verdicts, so the flag decides rather than incidental matching: | payload | exit | | --- | --- | | `"command": ""` (empty, no NUL) | 0 | | `command` field absent entirely | 0 | | leading NUL, truncates to empty | **2** | | a lone NUL and nothing else | **2** | Same on both guards. ### Test suites, same host, baseline vs branch **Both arms ran in full**, serially, on an uncontended host: every `*.test.sh` under `plugins/guardrails/hooks/` plus `lib/hook-utils.test.sh` — 14 suites, every one of them listed below. BASELINE is the same `468bb2d9` tree used for the boundary table; BRANCH is this tip. | suite | baseline | branch | delta | | --- | --- | --- | --- | | `lib/hook-utils.test.sh` | 156 / 0 | **162 / 0** | +6 new cases | | `block-dangerous-git.test.sh` | 341 / 0 | **346 / 0** | +5 new cases | | `block-no-verify.test.sh` | 120 / 0 | **127 / 0** | +7 new cases | | `block-convention-violation.test.sh` | 31 / 0 | 31 / 0 | — | | `block-hook-bypass.test.sh` | 260 / 0 | 260 / 0 | — | | `block-noncanonical-commit.test.sh` | 202 / 0 | 202 / 0 | — | | `cli-flag-verify.test.sh` | 52 / 0 | 52 / 0 | — | | `flag-commit-pr-skill-bypass.test.sh` | 29 / 0 | 29 / 0 | — | | `hardcoded-path-check.test.sh` | 94 / 0 | 94 / 0 | — | | `require-jq-notice-isolation.test.sh` | 2 / 0 | 2 / 0 | — | | `secret-pattern-detection.test.sh` | 52 / 0 | 52 / 0 | — | | `skill-reference-verify.test.sh` | 96 / 0 | 96 / 0 | — | | `stale-path-verify.test.sh` | 87 / 0 | 87 / 0 | — | | `workflow-resilience-check.test.sh` | 16 / 0 | 16 / 0 | — | | **total** | **1538 / 0** | **1556 / 0** | **+18, 0 failures either side** | Every suite that does not exercise the new path is byte-identical across the two arms, so the +18 is entirely the new cases. No pre-existing failure to disambiguate. Two of the new library tests look redundant and are not: `HOOK_JQ_FIELDS_NUL` is checked both after a clean payload and after an **early return**, each running a NUL payload first, because a single-call test cannot observe a stale flag however it is written, and two of the three return paths fire before any NUL could be seen. ### Other gates, all re-run after the rebase `sync-hook-utils.sh --check` (16/16) - `sync-hook-utils.sh --check-bump origin/main` - `check-changelog-parity.sh --check` / `--check-bump origin/main` / `--check-order` - `check-silent-skips.sh` - `check-contract-clause-coverage.py` - `check-cross-plugin-source-drift.sh --check` - `check-hook-userconfig-argv.sh` - `check-plugin-manifest-presence.sh` - `sync-parse-concern-value.sh --check` - `sync-resolve-convention-pattern.sh --check` - `sync-standards-contract.sh --check` - `check-skill-leaf-names.sh --check` - `check-shell-portability.sh --paths` - `shellcheck -x -S warning` (rc 0) - `shfmt -d -i 2` (rc 0) - `markdownlint-cli2` (0 issues) - `check-manifest-duplicate-keys.py`. ## What this PR does NOT fix, stated rather than implied **A payload jq cannot parse still returns 1 and is still allowed.** Malformed JSON, a wrongly typed field or an empty buffer all reach the same `|| exit 0`, exactly as before this change. Process substitution also means jq's own exit status is never observed. That path is untouched here and out of scope, and the header comment now says so instead of claiming — as an earlier draft of this very fix did — that nothing a payload contains can reach it. That claim is the same reasoning shape that produced #2122, and it should not ship inside its fix. ## What I could NOT verify - **How a command actually travels from hook payload to execution.** Nobody traced it. Two shell behaviours were measured and they disagree, and Node refuses NUL-bearing strings on every shape tried, so the command may never reach a shell parser at all. The design is built so this does not matter: fail-closed is right under deletion, truncation, and refusal alike. An earlier draft of this PR did lean on it, in one direction and then the other; both are gone, from the body and from the code comments, the README and the changelog. - **Whether the harness's control-character validation runs before or after PreToolUse hooks**, and **whether the class it rejects includes NUL specifically.** The discriminating probe is bypass-shaped and was deliberately not run. The guard that exists is worded *"contains control characters that would be hidden in the approval dialog"* — approval-surface anti-spoofing, covering `command` / `script` / `url` only, with no equivalent on `content` / `new_string` / `file_text`. It is an implementation detail, not a documented guarantee, and nothing here leans on it in either direction. - **Behaviour on jq builds other than 1.8.2, and on bash other than 5.3.9 (Cygwin).** The chosen construct uses only `explode`, `implode`, `index`, array slicing and `any` — core since jq 1.5 — precisely to keep that risk low, but it was not executed against an older jq. The repo's `hook-utils-windows` job exercises Git Bash on windows-2025 in CI. - **Any performance claim.** The spawn count is unchanged at one, which is structural. Measured per-field cost of the sanitiser was below spawn noise on this host — the no-op control benchmarked *slower* than all three candidates — so no number is claimed. - **Whether a NUL payload can reach a PreToolUse hook through the harness's own serialization.** Being settled separately. This fix does not depend on the answer: failing open on a parse failure is wrong regardless of how the parse came to fail. ## Related - Closes #2122 — the reproduction and the shipped-hook measurements this PR fixes. - Refs #2120 (`perf/guardrails-jq-fields`) — converts ten further guardrails hooks to `hook::jq_fields` and, at `9fb8383d`, independently fixes the same function by **stripping** NULs. Direct conflict on `lib/hook-utils.sh`; whoever merges second must keep both correctness properties rather than taking one side of the hunk, and those ten hooks need `HOOK_JQ_FIELDS_NUL` checks as a prerequisite. Not closed by this PR. - Refs #2123 — merged mid-work; changed `plugins/guardrails/lib/powershell/ps-command.sh`, which both guards source, so the boundary table was re-measured against it. Introduces no `hook::jq_fields` call site, so it needs nothing from this change. - Refs 9b90e35 (#1979) — the precedent for a shared-lib change costing a version bump and a changelog entry in every carrying plugin. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Resolves the markdown-format version collision: #2135 landed the vendored hook-utils NUL-reporting bump as 0.11.4 on main while this branch also claimed 0.11.4. main's entry keeps 0.11.4; this branch's two no-git fixes move to 0.11.5. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…t control Every git-absent symlink case asserted a SKIP, so a physically_inside that regressed to always-false would satisfy all of them while silently costing every legitimately in-repo symlinked .md its --fix on a git-less host. The assertion is on the link being GONE rather than the target's bytes: the stub linter rewrites with sed -i, which renames a temp over the path it was handed, so a --fix that ran replaces the symlink with a regular file and leaves the target untouched. Asserting target bytes would fail against correct code. Negative control run before committing: with physically_inside forced to refuse, this case fails with link-intact=yes and no other case changes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 1m 53s —— View job Security review complete
Scope: the 4 files changed in this PR at current HEAD No production code changed since the last review round. IMPORTANT (confidence: plausible, narrow preconditions) — still open: the unauthenticated Raised on this thread eight times now across prior rounds, unchanged at this HEAD. if dir="$(cd "$hint" 2>/dev/null && pwd -P)"; then
while :; do
if [[ -e "$dir/.git" ]]; then
printf '%s' "$dir"
return 0
fi
parent="$(dirname "$dir")"
[[ "$parent" != "$dir" ]] || break
dir="$parent"
done
fi
if [[ -n "${CLAUDE_PROJECT_DIR:-}" ]]; then
printf '%s' "$CLAUDE_PROJECT_DIR"
return 0
fi
This matters beyond config discovery: This has now been open across eight review rounds on this thread without a code change addressing it. Not re-filing as a new inline comment since nothing changed to re-review — restating so it isn't lost in the merge noise. Worth an explicit decision (accept the documented narrow precondition, or close it with a Other things checked, no issues found
Note on verification: as in prior rounds, I could not execute |
The merge resolution removed the <<<<<<<, ======= and >>>>>>> markers but not the ||||||| base marker, which markdownlint then read as body text abutting the 0.11.4 heading. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 1m 22s —— View job Security review complete
Scope: the 4 files changed in this PR at current HEAD Nothing security-relevant changed since the last review round. -||||||| 38bb041d
+
## [0.11.4]That's a whitespace/marker cleanup in prose, not code. IMPORTANT (confidence: plausible, narrow preconditions) — still open: the unauthenticated Raised on this thread nine times now across prior rounds, unchanged at this HEAD. if dir="$(cd "$hint" 2>/dev/null && pwd -P)"; then
while :; do
if [[ -e "$dir/.git" ]]; then
printf '%s' "$dir"
return 0
fi
parent="$(dirname "$dir")"
[[ "$parent" != "$dir" ]] || break
dir="$parent"
done
fi
if [[ -n "${CLAUDE_PROJECT_DIR:-}" ]]; then
printf '%s' "$CLAUDE_PROJECT_DIR"
return 0
fi
This matters beyond config discovery: This has now been open across nine review rounds on this thread without a code change addressing it. Not re-filing as a new inline comment since nothing changed to re-review — restating so it isn't lost in the merge noise. Worth an explicit decision (accept the documented narrow precondition, or close it with a Other things checked, no issues found
Note on verification: as in prior rounds, I could not execute |
main advanced markdown-format to 0.11.6 while this branch held 0.11.5, so the branch's two no-git fixes move to 0.11.7. main's 0.11.6 and 0.11.5 sections are kept under their own headings with their own content. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 1m 58s —— View job Security review complete
Scope: the 4 files changed in this PR at HEAD No code change since the last reviewed HEAD ( IMPORTANT (confidence: plausible, narrow preconditions) — still open: the unauthenticated Raised on this thread ten times now across prior rounds (05:12:59, 13:29:13, 14:26:36, 14:44:41, 15:28:43, 15:59:42, 20:08:36, 20:24:16, 20:31:19, and now this round), unchanged at this HEAD. if dir="$(cd "$hint" 2>/dev/null && pwd -P)"; then
while :; do
if [[ -e "$dir/.git" ]]; then
printf '%s' "$dir"
return 0
fi
parent="$(dirname "$dir")"
[[ "$parent" != "$dir" ]] || break
dir="$parent"
done
fi
if [[ -n "${CLAUDE_PROJECT_DIR:-}" ]]; then
printf '%s' "$CLAUDE_PROJECT_DIR"
return 0
fi
This matters beyond config discovery: Preconditions are unchanged and narrow: git absent from This has now been open across ten review rounds on this thread without a code change addressing it. Not re-filing as a new inline comment since nothing changed to re-review — restating so it isn't lost in the merge noise. Worth an explicit decision before merge: accept the documented narrow precondition, or close it by requiring a file-form Other things checked, no issues found
Note on verification: as in all prior rounds on this thread, I could not execute |
… last (#2171) No linked issue ## Summary `babysit_merge.branch_rules` reads the right endpoint — `repos/{repo}/rules/branches/{branch}` — but folds it as if each rule type appeared at most once. That endpoint returns one rule of a given type **per ruleset** governing the branch, and the fold is a plain assignment inside the loop, so each ruleset overwrote the previous one and only the last survived. Measured live on this repository. `main` is governed by two rulesets carrying required contexts, both org-sourced: | ruleset id | contexts | | --- | --- | | 17989001 | `pr-title / pr-title`, `do-not-merge / do-not-merge`, `ci-status` | | 19388547 | `security-review / security-review` | 19388547 is returned last, so the helper reported `effectiveRules.requiredContexts` as **only** `["security-review / security-review"]` — three of four required contexts silently dropped. The single-rule assumption held under classic branch protection, which has exactly one such rule. It does not hold under rulesets. **Impact: a reporting and defence-in-depth defect, not a merge-safety hole.** The gate refuses independently on `mergeStateStatus not in READY_MERGE_STATES` (`{CLEAN, HAS_HOOKS}`), and GitHub integrates required checks into that field — live `MergeStateStatus` introspection gives `CLEAN: "Mergeable and passing commit status"`, `UNSTABLE: "Mergeable with non-passing commit status"`, `BLOCKED: "The merge is blocked"` — so a **failing** required context cannot present as `CLEAN`/`HAS_HOOKS`. The **absent**-context case is derived from required-status-check semantics, not observed: every required context runs on every PR here, so there was no live PR to reproduce it against. Unconditional `if failing:` / `if pending:` blockers built from the whole rollup cover the rest. What the bug cost is the **explanation**: `effectiveRules` and the `required checks not satisfied` blocker both under-reported, so an operator could not see which contexts actually govern. One safety-adjacent consequence, in the **over-holding** direction. `base_is_unprotected = not required_reviews and not required_context_list`, and this repo's `pull_request` rule sets `required_approving_review_count: 0`, so the flag hangs entirely on `requiredContexts` being empty. Under the bug that meant "the **last** status-checks rule is empty"; fixed, it means "**all** of them are". "All empty" is a subset of "last empty", and both consumers of the flag only ever *add* blockers — so the bug produced a **false hold** on a superset of cases and never retired one. Latent here, since neither ruleset carries an empty context list. It is not a fail-open. ## Fix **Commit 1 — `required_status_checks`.** - Accumulate contexts into a set across **all** rules, reported `sorted()`. Deduped because two rulesets may legitimately require the same context; sorted so the reported set is stable regardless of the order the API returns rulesets in. - Entries carrying no `context` are dropped rather than carried. Previously a missing key produced a `None` that reached the reconciliation loop and surfaced as a literal `"None"` required context; it would also crash the new sort. This is a visible change in the helper's output. - `base_is_unprotected` needs **no code change** and is confirm-safe once the union is correct: the union is empty only when no ruleset requires anything, which is exactly what the flag means. **Commit 2 — `pull_request`.** The same assign-in-loop shape sat three lines below, in the same function. Not observed misreporting — exactly one `pull_request` rule (ruleset 17988999) governs the branch today — but nothing prevents a second, and a ruleset requiring 2 approvals returned before one requiring 0 would have reported 0. `requiredApprovingReviews` now takes the `max`, `requireThreadResolution` the `OR`. That fold direction is deliberately argued from safety, not from GitHub's internal composition rule, which this change does not claim to know: **max/OR can only ever over-report**, which holds a PR for a human, where last-wins can under-report and release one. This one could lose a blocker outright, not merely under-report: a trailing `pull_request` rule with `required_approving_review_count: 0` erased an earlier ruleset's requirement and **dropped the `needs N approving review(s)` blocker**. Keep that distinct from the `base_is_unprotected` consequence above, which runs the other way (over-hold). A malformed-but-present count reads as **one** review, never zero — reading it as zero would be the single fail-open step in a fold whose whole argument is that it can only over-report. Severity split, kept separate on purpose: - `requiredApprovingReviews` — a **fail-closed behaviour change**, not currently firing. It feeds both `base_is_unprotected` and the `needs N approving review(s)` blocker. - `requireThreadResolution`, `requireSignatures`, `requireLinearHistory` — **report-only**. Set into the summary, never consumed as a blocker; the gate holds on unresolved threads unconditionally via `if threads:`. They do not borrow the first item's severity. Version bumped `0.51.5` → `0.51.6` with a matching CHANGELOG entry, following the plugin's convention — every comparable `fix(source-control)` commit in recent history (`cf743d61`, `ac27ea5a`, `30be2a0b`, `e6ee72ef`) bumped the manifest version. ## Verification New module `tests/test_babysit_merge_branch_rules.py` (7 tests), each run against the fixed code and against the unfixed file: | test | fixed | unfixed | | --- | --- | --- | | `test_contexts_from_every_ruleset_survive` | ok | **FAIL** — `['security-review / security-review'] != ['ci-status', 'do-not-merge / do-not-merge', 'pr-title / pr-title', 'security-review / security-review']` | | `test_a_context_required_by_two_rulesets_is_reported_once` | ok | **FAIL** — `['ci-status'] != ['ci-status', 'pr-title / pr-title']` | | `test_a_context_less_entry_is_dropped` | ok | **FAIL** — `[None] != []` | | `test_empty_trailing_rule_leaves_the_base_protected` | ok | **FAIL** — `True is not false` (`baseUnprotected` flipped) | | `test_the_strictest_approval_count_wins` | ok | **FAIL** — `0 != 2` | | `test_thread_resolution_required_by_any_ruleset_survives` | ok | **FAIL** — `False is not true` | | `test_no_context_anywhere_still_reports_an_unprotected_base` | ok | ok | Six regress. The seventh passes both ways **by design** — it is the over-correction guard, pinning that a genuinely context-less base still reports unprotected. It is labelled as such in its class docstring so nobody counts it among the regression tests. `test_empty_trailing_rule_leaves_the_base_protected` asserts on `evaluate()`'s `baseUnprotected` and blocker list, not on `branch_rules` alone, and its fixture sets `required_approving_review_count: 0` — with a non-zero count the flag would be `False` against the unfixed code too and the test would prove nothing. **End-to-end against a live CLEAN PR.** The fix feeds four contexts into the reconciliation matcher where one went before, so a context that failed to match its rollup entry would convert a silent under-report into a spurious blocker. Ran `evaluate()` against #2150 (CLEAN, all four contexts green): ``` requiredContexts: ["ci-status", "do-not-merge / do-not-merge", "pr-title / pr-title", "security-review / security-review"] requiredChecks: all four found: true, satisfied: true, category: "success" baseUnprotected: false blockers: [] ready: true mergeStateStatus: CLEAN ``` **Suite.** `bash plugins/source-control/skills/babysit-prs/scripts/engine.test.sh` exits 0 — 612 tests OK, `ruff` (CI pin) clean, guarded-wrapper behaviour all PASS. No shell files changed, so no shellcheck surface. **Changelog parity.** `--check` and `--check-order` pass. `--check-bump origin/main` passed 8/8 consecutive local runs on GNU Awk 5.4.0. Recording that as an observation, not a health claim: the gate is reported to have a SIGPIPE race after #2154, and local green does not establish CI green. **Not verified — recorded, not claimed.** A ruleset carrying **bypass actors** is the one shape where GitHub could plausibly report `CLEAN` to a bypassing identity while a required context is unmet; there the unmet-required blocker would be the only defence, which raises the severity of the under-report. Untestable here — every ruleset carries `bypass_actors: []`. Likewise the absent-required-context case above. Neither refutes the characterisation; both are open. **Two things worth knowing about the union.** Adding `security-review / security-review` does not mint a false blocker when that check skips: `babysit_checks.py` treats `NEUTRAL`/`SKIPPED`/`SUCCESS` as success states, so a name-stable skipped check still satisfies. And the deliberately loose context matcher now processes four contexts where it processed one — this **amplifies** pre-existing false-match exposure rather than introducing it, which is exactly what the live `evaluate()` check above is there to catch. **Sibling scripts.** `babysit_resolve_thread.py` reads no branch rules, and a repo-wide search for `rules/branches` / `required_status_checks` / `effectiveRules` finds no other fold and no other consumer — `babysit_merge.py` is the only one. `plugins/source-control/skills/setup/SKILL.md:121` documents the same endpoint to operators but instructs them to read the whole payload and flag zero-reviews-and-zero-contexts repos, so it carries no one-rule-wins assumption and needs no change. ## Related Refs #2130, #2135 — reported as observed there. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
Follow-up to #2121. Both gaps are live on
mainright now — not stale review findings. Reproduced independently: the two new tests, run againstmainown unmodified hook, give PASS=136 FAIL=2. With the change, 138/0.The two defects
1.
markdown-format.sh:119callshook::repo_rootraw. Withgitandjqboth absent, a nested file makes the opt-in pre-check read an opted-in repo as opted-out, and thejqnotice is swallowed. A repository that did opt in is treated as if it had not, silently.2. The
REPO_ROOTguard at229-237covers only theCLAUDE_PROJECT_DIR-set case. The membership scope it exists to fix is gated on that variable being unset, and the no-git fixture runs unset — so the configuration the fix was written for is still broken for nested files.hook::repo_rootfalls back to the file own directory, the root markdownlint config is never discovered, and the edit is skipped with no diagnostic.The second is the one #2121 review comment described as "leaving the normal nested-docs case unfixed". That reading was correct and remains correct at
main.The change
Resolve the repository root from the filesystem rather than from a variable: walk up for a
.gitentry, accepting a directory or a file so linked worktrees and submodules resolve. Git own answer is returned untouched whenever git produced one, andCLAUDE_PROJECT_DIRis kept as a further fallback, so the casemainalready handles is subsumed rather than replaced.Four commits, ordered so the defect is demonstrated before it is fixed:
Verification
main135/0; with the change 138/0; the two new tests red againstmainown hook (independently reproduced ate47964ca).mainnewest positive override test passes unchanged under the replacement — verified rather than assumed, after confirming no.gitsits on the temp-dir ancestor chain that would have made the walk answer differently on this host.shellcheck -x -S warning, shell-portability, silent-skips, markdownlint, and changelog-parity all clean.Stated rather than glossed — three things not confirmed
rev-parse --show-toplevelanddirnamenever produce the same path spelling there. The extra probe fires only where the spellings agree — 2 to 3 spawns, root-level files only.PASS=133 FAIL=1intermittent was seen at an abandoned intermediate commit. It was unnamed, did not reproduce in five runs at the successor commit, and never recurred in any run backing these numbers. Unconfirmed rather than dismissed.Provenance
Prepared as a cherry-pickable offer while #2121 was open; #2121 merged at
5f92d946without taking it, leaving no branch to cherry-pick onto, so this is cut frommaininstead. The offer comment on #2121 remains accurate for what it offered at the time.Fixes #2134
Conflict resolution against a moving
mainmainmoved under this branch twice and the PR wentDIRTY. The version collision was resolvedtwice, and the branch now carries the second resolution's numbers.
plugins/markdown-format/CHANGELOG.md.maintook0.11.2(perf(guardrails): parse each hook payload in one jq process, not two or three #2120'sshared
hook-utils.shNUL fix), then0.11.3(fix(guardrails): read the payload cwd, and stop env -S hiding commands from every git guard #2147). This branch's entry moved up each time andnow sits at
0.11.4, withmain's0.11.3and0.11.2kept below it, order strictlydescending.
plugin.jsonauto-merged tomain's number on both passes, silently leaving no bump at all —no conflict marker, and only
check-changelog-parity.sh --check-bumpcatches it. Bumped to0.11.4to match the changelog. This is the trap worth carrying forward: a manifest versioncollision does not conflict, it resolves to whichever side git saw last.
check-changelog-parity.sh --check-bump origin/mainclean at the resolved tree.History note, stated rather than glossed. This resolution was first delivered as two merge
commits (
git merge origin/main, never a rebase, since force-push is blocked here). The branch wassubsequently force-pushed to a rebased, linear history carrying the same resolved content and the
same
0.11.4numbers, which discarded those merge commits. The shipped branch is therefore a rebase,not the merge described above; the resolution it carries is the same one.
Version coordination with #2135: that PR also bumps
markdown-format, and after its own mergesof
mainit currently takes0.11.4as well. Whichever of the two merges second must re-bump — themanifests will auto-merge to the same number without conflicting, exactly as described above.
Related
5f92d946without taking the offered follow-up, which is why this is cut frommain.mainmid-flight; its sharedhook-utils.shchange took the0.11.2slot this branch's changelog entry originally occupied.
markdown-formatversion bump; see the coordination note above.