fix(hook-utils): expand Windows 8.3 short names in the membership guard - #1692
Conversation
GNU realpath under Git Bash resolves symlinks but leaves Windows 8.3 short names (KYLESE~1) unexpanded, so hook::physical_path handed the membership comparison in hook::read_file_path a short-form path that could never prefix-match a long-form CLAUDE_PROJECT_DIR. An IN-project file whose file_path arrived in short form - the shape Claude Code's own scratchpad paths take - was silently skipped: no lint, no notice, no telemetry. 8.3 generation is a per-volume property (fsutil 8dot3name), which is why the defect was invisible to contributors whose checkouts live on a non-generating volume. The lib now expands short names on Windows/MSYS hosts (new hook::expand_8dot3): cygpath -m (form conversion only) is compared against cygpath -l -m (long names via Win32), and the path is replaced only when the two differ, so a legitimate long name containing '~' passes through byte-for-byte untouched. The expansion runs only on the resolver's success path, preserving the documented unchanged-return signature that markdown-format's fail-closed checks rely on. A genuinely out-of-project file is still skipped - the deliberate defense-in-depth scoping is unchanged. Fixing the shared lib rather than per-consumer follows the issue's call: three consumers have met this edge, and actionlint's 0.6.0 local opt-out stays intact. All 14 carrying plugins are synced, version-bumped, and changelogged. A regression test drives a short-form in-project path, a short-form project dir, a short-form OUT-of-project path (still rejected), and a literal-tilde long name through hook::read_file_path, skipping with a visible volume-scoped reason where short names are unavailable. Closes #1636 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Claude encountered an error after 1m 28s —— View job PR Review in Progress
|
|
Claude encountered an error after 1m 34s —— View job Security Review
|
|
This was generated by AI (triage lane — raw-intake brief). VerdictReal and actionable. Genuine defect fix, not noise: linked issue #1636 is OPEN and labeled Root cause confirmed on this machine (Git Bash, Work classC4 — structural. Per GateHuman review + human merge. Not eligible for autonomous merge. This conclusion does not depend on winning the C3-vs-C4 argument: C4 mandates human review and human merge always and never promotes, and C3's verifiability bundle explicitly requires AI review, which is absent here. Under either classification an autonomous lane must not merge this. Review status is UNKNOWN, not cleanBoth automated reviews FAILED as infrastructure failures. Neither returned a verdict. Treat the absence as unknown — never as a pass.
Evidence: the
Re-running both reviews is a prerequisite to any merge decision. This is not box-ticking: the PR's own regression test is structurally blind to the highest risk in the diff (next section), which is precisely what an AI review pass would have been positioned to catch. Security / blast-radius assessmentGuard-bypass direction: no bypass found. 8.3 expansion is canonicalization-correct — it changes only the spelling of components that already resolve to the same filesystem object, so it cannot make an out-of-project path resolve inside the project. However — one real cross-consumer defect, fail-CLOSED direction (functional regression, not a hole).
Note the divergence axis is drive-form only, not spelling: Test coverage gap: Test 12b drives only Consumers (verified in the real tree, nested worktrees excluded):
Next concrete actions
Blockers / open questions for a human
|
|
Merge-lane cycle 2 claim. Working this PR now. |
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
|
Claude encountered an error after 1s —— View job I'll analyze this and get back to you. |
|
Merge-lane cycle 2 claim released. |
…what the writer does (#1734) 🤖 Agent-authored (autonomous worker lane). Closes #1685. ## Summary The plugin told readers that the release adding an account identifier upgrades the tee file for free. The writer cannot guarantee that. `scripts/statusline-tee.sh` selects on the **top-level key name only** — `to_entries` over the root object — matching `account` as a **case-insensitive substring**, and an unmatched key is dropped **with no diagnostic**. So the promise holds only when the new field's own top-level key name contains `account`. A field called `user`, `identity`, `org`, or `seat`, or an `account_uuid` buried inside a non-matching object, vanishes silently — in a contract that fail-closes on every other unresolvable input. The doc now follows the code. `statusline-tee.sh`'s **behavior is unchanged**: the jq filter is byte-identical to `main` and only its header comment moved. Widening the filter is a design question owned by `TODO(#1218)`, which the issue and triage both scoped out of this correction. ## What changed **Four surfaces carried the same overstatement, not the one the issue quotes.** The issue quotes only the reader contract's Invariants bullet; fixing that alone would have shipped a document contradicting its own Tee-file-shape section, and would have left the claim standing on the plugin's front door. | Surface | Was | Now | | --- | --- | --- | | `reference/reader-contract.md` "Tee file shape" | "...so the release that adds an account identifier upgrades this file without a plugin change" | States the writer's actual reach; owns the rule | | `reference/reader-contract.md` "Invariants" (the quoted one) | "...so an identity field costs no plugin change the release one appears" | Scoped to that shape; **points at** the rule rather than restating it | | `README.md` known-gap bullet | "The wrapper automatically adopts any future account-identifying field the schema grows" — fully unqualified, and broader than the sentence the issue quotes | Scoped, with a pointer to the contract | | `scripts/statusline-tee.sh` header comment | qualifiers sat in the parenthetical; the `so ...` inference did not carry them forward | Inference now carries the qualifiers | **One correction the issue did not anticipate.** An earlier draft of this PR implied that *nesting* defeats the forward-pass. It does not: a selected top-level key crosses with its **whole value**, nested objects included (`account_info: {uuid, display_name}` arrives complete). The drop is caused by no top-level key matching, not by depth. Because a passed key may therefore carry an **object of arbitrary user/AI-influenced strings**, the untrusted-value discipline is restated to cover the subtree rather than a scalar — the parse-with-a-JSON-parser, never-interpolate rule applies to all of it. **Terminology unified** on "writer change"; the contract had split between "writer change" and "filter change" for the same fact. **Two phrasings from the issue body were rejected as not surviving contact with the code:** the filter also selects `rate_limits`, so it is not "the whole of" the forward-pass; and `map(select(...))` has no branch to lack an else — the accurate statement is that unmatched keys are dropped with no diagnostic. ## Verification **This is a doc-only prose correction with no executable surface. No test can reach the claim, and I did not invent coverage for it.** Verification is the read of the filter against the new prose, plus the gates below. The one executable file touched changed only a comment; `git diff origin/main -- scripts/statusline-tee.sh` shows two comment hunks and nothing else. | Gate | Result | | --- | --- | | `check-changelog-parity.sh --check` | pass | | `check-changelog-parity.sh --check-bump origin/main` | pass | | `generate-catalog.mjs --check` | catalog in sync | | `validate-plugins.sh` | all manifests + catalog validated | | `validate-plugin-contracts.mjs` | 43 setup skills, 2123 plugin files | | `check-cross-plugin-source-drift.sh --check` | no drift | | `check-changed-skills.sh origin/main` | no changed skills (correctly a no-op) | | `check-shell-portability.sh origin/main` | no unexcused GNU-only constructs, 1 file | | `shellcheck` + `bash -n` on `statusline-tee.sh` | clean | | `markdownlint-cli2` on all 3 changed `.md` | 0 errors | The **operable floor** (the block the three lane skills inline byte-identically per loop-lane convention §6) is untouched, so no lane-skill fan-out is needed and §6 needs nothing. **`CHANGELOG.md:146` deliberately still carries the old wording.** It is a historical `[0.2.0]` release note, not a live surface — changelog entries are immutable history. Not a missed site. ## Reviewer note — version collision with #1692 This PR takes `rate-limit-guard` **0.3.4 → 0.3.5**. **PR #1692 also claims 0.3.5** for this plugin. Whichever merges second must rebase and re-bump, and `check-changelog-parity.sh --check-bump` will fail for the loser until it does. Not pre-emptively jumping to 0.3.6, since that guesses at merge order and would leave a version gap. The bump is warranted by `docs/MIGRATION-PLAYBOOK.md:373-375` — a `version` bump is the only delivery vehicle, so an unbumped plugin never delivers a corrected reference to consumers. Note `--check-bump` only fires *when* a version changes; it does not itself compel one. ## Related - Closes #1685 - `TODO(#1218)` — widening the filter is that issue's call, explicitly not this one's. #1218 specifies **no field name**, so this PR makes no claim about whether its eventual field will satisfy the existing filter. - Reader contract: `plugins/rate-limit-guard/reference/reader-contract.md` - Loop-lane convention §6 (inline-floor rule): `docs/conventions/loop-lane/README.md` 🤖 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 7m 11s —— View job PR Review — Complete
SummaryThe core fix is technically correct for its stated purpose: However, I found one confirmed correctness bug the PR introduces, which the triage comment raised and which the PR's own CHANGELOG misrepresents as resolved. It is fail-closed (a functional regression, not a security hole), but it is a real cross-consumer defect that CI cannot detect and the regression test cannot see. Finding 1 —
|
|
Claude finished @kyle-sexton's task in 3m 11s —— View job Security Review
Scope reviewed: Finding 1 — IMPORTANT | PLAUSIBLEForm-leak from
# markdown-format.sh:231
CONFIG_ROOT="$(cd "$REPO_ROOT" 2>/dev/null && pwd -P)" || CONFIG_ROOT="$REPO_ROOT"
# ...
# markdown-format.sh:489
resolved="$(hook::physical_path "$candidate")" # now may return C:/... via cygpath -l -m
# ...
# markdown-format.sh:512-513
case "$resolved" in
"$CONFIG_ROOT"/*) # /c/Users/… pattern vs C:/Users/… value → never matchesWhen this comparison fails (mixed-form Security direction: This is fail-closed, not fail-open. The security guard is not weakened — no external code gains approval. The error makes the guard more restrictive than correct, producing a false positive that blocks legitimate operations. An attacker cannot use this form-mismatch to smuggle an external module through. Reachability: Requires a Windows host with a short-name-generating volume and a repo accessed via a short-name path — exactly the scenario this PR is designed to fix. The three direct Fix seam (per prior triage — either option is sufficient): (a) normalize both sides before the Permalink: No bypass vector found in
|
…1812) ## Summary - **Root cause.** `hook::read_file_path` (`lib/hook-utils.sh`) decides project membership by prefix-matching `CLAUDE_PROJECT_DIR`. When the project directory is the user's home — the shape a session started outside any checkout takes — the OS temp root sits *under* it, so Claude Code's own per-session scratchpad passes the membership test. Every file-scoped hook that scopes on this guard (12 of them) then treats scratch files as project content. In the reported case `typos-format` autocorrected a shell variable in a throwaway script from typos' built-in dictionary, in a location with no repository and therefore no typos config to allow-list the token with, silently breaking the script. This is the failure the repository's own `hook-precision` rule 5 already names, and `typos-format.sh`'s header wrongly claimed rule 5 was N/A for it. - **Fix (shared lib).** New `hook::under_temp_root` in `lib/hook-utils.sh`; `hook::read_file_path` now rejects a file inside the OS temp tree when the project root is outside it. The exemption is deliberate and load-bearing: when the project root itself lives under temp — a `mktemp -d` fixture checkout, which is how this repository's own hook suites run — its files are still project content and are accepted. Temp roots come from `TMPDIR` / `TMP` / `TEMP` plus the POSIX defaults, resolved through the same `hook::physical_path` + `hook::normalize_path` pipeline the membership comparison already uses (both spellings matter: on Git Bash `TMPDIR=/tmp` while `TEMP` carries the Windows form of the identical directory, and `realpath` resolves one but not the other). Cost: every in-project file now pays one candidate-resolution pass — after dedup, about two `realpath` spawns — before the guard returns; the second call (on the project root) only runs when the file matched, so the common case pays one pass, not two. - **Two scope caveats, stated plainly.** The gate lives inside the existing `if [[ -n "${CLAUDE_PROJECT_DIR:-}" ]]` branch, so a session with `CLAUDE_PROJECT_DIR` **unset** (some headless `-p` sessions, per `bash-format`'s README) still processes temp-tree files — the reported case had it set, so this closes the report, but the temp tree is not universally excluded. And a Git worktree living under the OS temp root, reached from a home-shaped project dir, is now skipped by every file-scoped hook; a git-worktree escape hatch was considered and dropped as a second heuristic layer. Working *inside* such a worktree makes it the project root, which the temp-rooted exemption already covers, so the exposure is narrow — noted because #1774 concerns worktrees in system temp. - **Scope.** Canonical source edited, `scripts/sync-hook-utils.sh` run: all 16 carrying plugins synced byte-identical, version-bumped, and changelogged. `bash-format`'s README and setup SKILL state this guard's contract (they already claimed temp/scratchpad files were skipped — true only when the project dir was a checkout) and are corrected to state the refined rule. - **`typos-format` disclosure.** The applied-rewrite guidance pointed only at `extend-words` / `extend-identifiers`. It now also names `extend-ignore-re` for a region quoted verbatim (a Markdown code fence, a transcript, a signature block), which typos' [reference](https://github.com/crate-ci/typos/blob/master/docs/reference.md) documents for exactly that case and supports across lines via `(?s)`. - **Not fixed here, by design.** The issue's defects 2 (correction applied to a token in identifier position) and 3 (correction applied inside a Markdown code fence) are `typos-cli` behavior, not hook behavior — declining by syntactic position would mean re-implementing typos' tokenizer. Both remain live *inside* a repository, where the consuming repo's typos config is the seam (`extend-identifiers` / `extend-ignore-identifiers-re`, `extend-ignore-re`). Suggested-fix item 4 ("fail loudly rather than rewrite silently") is a plugin posture change, not a defect, and is left for a human decision; `typos_format_write_changes = false` already gives a report-only mode. Analysis is on the issue. ## Test plan Repro-first, per `docs/conventions/hook-precision/README.md`: each new stay-quiet case fails against the unmodified guard and passes after. Run on Windows 11 / Git Bash. - **The real production shape, end to end** — the committed hook driven against a file inside the actual harness session scratchpad (`%LOCALAPPDATA%\Temp\claude\<project>\<session>\scratchpad\`) with `CLAUDE_PROJECT_DIR='C:\Users\<user>'` and **no** temp-root overrides, so nothing about the fixture is synthetic: - pristine hook: `this has the typo` → `this has the typo`, hook reported `REWROTE 1 word(s)` — the reported defect, reproduced in place - fixed hook: file byte-identical afterward, hook stdout empty - This also proves the candidate list matches the backslash spelling the environment actually uses (`TEMP=C:\Users\KYLESE~1\AppData\Local\Temp`, while `TMPDIR=/tmp` resolves to a different spelling that `realpath` leaves alone). Test 12c pins that spelling with its own case. - **Black-box hook contract** — `plugins/typos-format/hooks/typos-format.test.sh`, new Case 7b (a temp-tree file under a home-shaped project dir must be left untouched and silent): - before the sync, against the unmodified plugin copy: `PASS=75 FAIL=2` — `temp-tree file not silent (rc=0 out={"hookSpecificOutput"... "typos-format REWROTE 1 word(s) in inventory.txt"...})` and `temp-tree file was rewritten: this has the typo` - after: `PASS=77 FAIL=0` - **Shared lib** — `lib/hook-utils.test.sh`, new Test 12c (four cases: the regression, the temp-rooted-project exemption, an in-project control, and the Windows backslash spelling of the temp root). Driven against the pristine copy of the lib and then the fixed one: - before: `FAIL: temp-tree file admitted under a non-temp project root` (the exemption and control cases are non-regression guards, green either way) - after: all 4 pass - full suite: 4 runs of the fixed lib gave `PASS=120..121` with 0–2 failures, all in the `buffer_stdin` timing class (`trickle`, `stall overshoot`, `late-EOF`). 5 runs of the pristine baseline on the same machine gave `PASS=116..118` with 0–2 failures **in the same class** — that class is load-sensitive and pre-existing (#1692's test plan documents it too). `under_temp_root` has no call path into `buffer_stdin`. - **Whole plugin corpus** — `scripts/run-plugin-tests.sh`: all plugin contract tests pass. - **Gates** — `scripts/sync-hook-utils.sh --check` (all 16 copies match), `scripts/check-changelog-parity.sh --check`, `scripts/check-silent-skips.sh`, `scripts/check-shell-portability.sh --paths` on the changed shell files, `shellcheck --rcfile .shellcheckrc -x`, `shfmt -d` — all clean. ## Cleanup guidance for anyone affected This change stops future rewrites; it cannot repair a file already corrupted. A scratch file written during a session whose `CLAUDE_PROJECT_DIR` was a home directory may carry silent dictionary corrections. There is no way to enumerate them after the fact — the hook's disclosure was emitted at the time, in the transcript. Re-generate rather than trust any scratch script from such a session. ## Related - Follow-on to #1692, which made short-form (`KYLESE~1`) scratchpad paths pass the membership guard; correct for in-project files, and it is what exposed this guard's over-broad membership predicate. - `docs/conventions/hook-precision/README.md` rule 5 and its repro-first discipline. Fixes #1769 🤖 Generated with [Claude Code](https://claude.com/claude-code) <https://claude.ai/code/session_01RhS3T7ShwJgKTrvk2Mvd3C> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 participant
Summary
KYLESE~1) unexpanded, sohook::physical_pathhanded theCLAUDE_PROJECT_DIRmembership comparison inhook::read_file_patha short-form path that can never prefix-match the long-form project root. An in-project file whosefile_patharrived in short form — the shape Claude Code's own scratchpad paths take — was silently skipped: no lint, no notice, no telemetry. 8.3 generation is per-volume (fsutil 8dot3name), which is why the defect reproduces on generating volumes (this machine'sC:) and is invisible on non-generating ones (D:).hook::expand_8dot3inlib/hook-utils.sh, applied onhook::physical_path's success path. Gated onOSTYPEmsys*|cygwin*|win32(same host gate ashook::normalize_path), so POSIX hosts are bit-identical with zero extra spawns. On Windows it comparescygpath -m(form conversion only) againstcygpath -l -m(long names via Win32) and replaces the path only when the two differ — a legitimate long name containing~passes through byte-for-byte, and a genuinely out-of-project file is still skipped (the deliberate defense-in-depth scoping is preserved). Running only on the resolver's success path preserves the documented unchanged-return signature that markdown-format's fail-closed checks rely on. Fail-open when cygpath is absent (it ships with Git Bash, the documented Windows bash), degrading to the pre-fix comparison.scripts/sync-hook-utils.shrun — all 14 carrying plugins synced byte-identical, version-bumped, and changelogged.bash-formatandmarkdown-formatconsume the fixed lib via the sync;actionlint's 0.6.0 (actionlint: membership guard silently skips lint on 8.3 short paths + cd-failure reads as clean + abs-path leak in telemetry #1133) local opt-out is untouched and its changelog records why. The volume-scoped 8.3 contract is documented at the function, in the regression test's skip reason, and in bash-format's README/setup-SKILL scope sections (the only docs that state the guard's contract).lib/hook-utils.test.shTest 12b): driveshook::read_file_pathwith a short-form in-project file (accepted + original spelling emitted), a short-form project dir, a short-form out-of-project file (still rejected), and a literal-tilde long name (untouched). Skips with a visible, volume-scoped reason on non-Windows hosts and on volumes that do not generate short names — the skip message states it is absence of coverage, not a pass.Test plan
All commands run from the worktree on Windows 11 / Git Bash, where
C:generates short names (fix exercised for real, not just skipped):bash lib/hook-utils.test.sh— all 4 new Test 12b cases pass (short-form in-project file accepted,short-form project dir admits in-project file,short-form out-of-project file still rejected,literal-tilde long name accepted). Full suitePASS=115 FAIL=3; the 3 failures are pre-existing load-sensitivebuffer_stdintiming tests — a pristineorigin/maincopy run side-by-side on the same loaded machine fails 5 of the same class (PASS=109 FAIL=5), and this diff does not touchbuffer_stdin.bash scripts/check-shell-portability.sh origin/main(CI form) —No unexcused GNU-only constructs in 16 shell file(s).(A full--allsweep has one pre-existing finding in untouchedplugins/repo-hygiene/.../batch-common.test.sh.)bash scripts/sync-hook-utils.sh --check—All 14 plugin copies match lib/hook-utils.sh.bash scripts/sync-hook-utils.sh --check-bump origin/main—Lib changed vs origin/main and every carrying plugin bumped its version.bash scripts/check-changelog-parity.sh --check-bump origin/main— pass.bash scripts/check-contract-slice-prune.sh --check-diff origin/main— pass.bash scripts/check-skill-portability.sh origin/main— pass.bash scripts/check-silent-skips.sh— pass.npx markdownlint-cli2 <16 changed .md files>— 0 errors.shellcheck --rcfile=.shellcheckrc lib/hook-utils.sh lib/hook-utils.test.sh— clean (two justifieddisabledirectives for the SC2030/SC2031 false-positive pair created by Test 11's deliberate subshell-local OSTYPE override).realpath "C:\Users\KYLESE~1\...\CLAUDE.md"returns the short form unchanged;cygpath -l -mexpands it;cygpath -l -mon a stale/nonexistent short path passes it through unchanged (fail-open).Related
plugins/actionlint/CHANGELOG.md0.6.0 (actionlint: membership guard silently skips lint on 8.3 short paths + cd-failure reads as clean + abs-path leak in telemetry #1133) — same defect fixed locally; this PR fixes the shared lib that change deliberately left untouched🤖 Generated with Claude Code
https://claude.ai/code/session_013E5dps5keCfRvYCicNYH6r