fix(claude-config): correct the audit checklist's hook-form rationale and add the timeout-unit row - #2004
Conversation
…rt the seconds timeout unit
## F3 adjudication — the checklist row was WRONG, but so was the finding's reason
The reported finding said Category D "contradicts the hooks doc on command form". Row 90 prescribed
quoted shell form with "**no `args`**", justified by a claim the finding never engaged: that the
`"command":"bash"` + `args` exec-form variant "backslash-mangles `${CLAUDE_PROJECT_DIR}` on native
Windows". Adjudicated against https://code.claude.com/docs/en/hooks, fetched this session.
VERDICT: UPHELD — the row is wrong and now changes. But the rationale is retired on its own merits,
not by deferring to the report.
The mangling premise is false. The page's "Exec form and shell form" section states that in exec
form "path placeholders like `${CLAUDE_PLUGIN_ROOT}` are substituted into `command` and into each
`args` element as plain strings", that "Special characters such as apostrophes, `$`, and backticks
pass through verbatim because there is no shell to interpret them", and — decisively — "No shell
tokenization happens on any platform." Backslash mangling is a shell artifact; exec form has no
shell to produce it. The two propositions the triage note held open ("upstream prefers exec form"
and "this repo observed a Windows defect in it") are indeed both true, but the observed defect is
not mangling.
The real defect is narrower, is documented, and now gets its own row. The page states: "On Windows,
exec form requires `command` to resolve to a real executable such as a `.exe`." Naming `bash` as the
executable resolves to the WSL relay `System32\bash.exe` and the launch fails. That is exactly the
failure this repo hit in #1006, where a fail-open destructive guard silently enforced nothing across
an 82-repo session. The fault is in naming `bash` as the executable, not in exec form — the page's
own portable pattern is a real binary plus the script path in `args`. Row 90 generalized one true
observation about `bash` into a false rule about a whole form, and the audit skill has been telling
consumers to warn on the form the docs prefer.
Category D now tracks the page: "Prefer exec form for any hook that references a path placeholder.
In shell form, wrap each placeholder in double quotes." The new row explicitly does NOT flag shell
form where the page endorses it — pipes, `&&`, redirects, `.cmd`/`.bat` shims — so the correction
does not swing into a mirror-image false positive.
Two further rows come from the same fetch: exec-form `command` must be a real executable (the #1006
class), and a shell-form PowerShell hook must not use the bare `$CLAUDE_PROJECT_DIR` spelling, which
"PowerShell parses ... as an undefined local variable and resolves ... to `$null`".
## F4 — timeout unit
New Category D row. The hooks reference's common-fields table gives `timeout` as "Seconds before
canceling. Defaults: 600 for `command`, `http`, and `mcp_tool`; 30 for `prompt`; 60 for `agent`."
A `timeout > 600` is therefore near-certainly milliseconds. The confusion has a documented source on
the same page: the Bash and PowerShell tools' `tool_input.timeout` is "Optional timeout in
milliseconds" with example `120000` — about 33 hours if read as seconds. The row names that
neighbor so the check explains itself rather than asserting a bare threshold.
The three sibling surfaces that restated the old guidance move together: the checklist, the
Category D list in `context/validation-categories.md`, and the one-line D summary in `SKILL.md`.
Minor bump: consumer-visible check behavior changes in both directions (a warning retired, four
added).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…e docs actually assert
Self-review of the previous commit found both new rows overreaching — each would have produced the
mirror image of the false positive being removed.
1. The path-placeholder row was titled "use exec form (`args`)" and rated a warning. Fixing a check
that wrongly warned on exec form by warning on shell form instead is not a fix. The hooks page
states a preference ("Prefer exec form for any hook that references a path placeholder") and in
the same breath gives the correct shell-form spelling ("In shell form, wrap each placeholder in
double quotes"), and endorses omitting `args` for pipes, `&&`, redirects, and `.cmd`/`.bat`
shims. Quoted shell form is therefore a documented, correct spelling — and the one this
repository's own `.claude/settings.json` hooks use, so the row as written would have warned on
this repo. The check now flags only the unquoted placeholder and reports exec form as a
preference rather than a finding.
2. The executable-resolution row was unscoped and rated an error, so it would have flagged
`"command": "bash"` on macOS and Linux, where `bash` is an ordinary executable and resolves fine.
The page scopes the constraint explicitly — "On Windows, exec form requires `command` to resolve
to a real executable such as a `.exe`" — so the row now applies to Windows-targeting repos only.
It also names `"shell": "bash"` as a fix alongside the `node`-plus-`args` pattern, since shell
form is what this repo actually shipped for #1006: the page states shell form runs via "Git Bash
on Windows", resolved by Claude Code rather than by a PATH lookup.
Same fetch as the previous commit: https://code.claude.com/docs/en/hooks. No version change — this
corrects rows added in the unreleased 0.22.0 entry, whose text is amended in place.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…quoting check The rewritten quoting row listed only the braced placeholder spellings, which silently narrowed the check relative to the row it replaced: the old one keyed on `$CLAUDE_PROJECT_DIR` and this repo's own hooks are written that way (`"\"$CLAUDE_PROJECT_DIR\"/.claude/hooks/..."`). Both spellings reach the shell in shell form, and an unquoted path breaks on a space either way, so the check names both. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
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 3m 43s —— View job Review complete
This PR touches a contract surface (the audit skill's hook-checklist rows describing documented harness behavior), so per this repo's CLAUDE.md fresh-docs mandate I re-fetched Verified as accurate against the fetched page:
One finding, repeated in three places (flagged inline): the new PowerShell row claims the hooks reference says a bare Notably this is the exact failure mode the PR's own body warns about (WebFetch summarizer fabrication on long docs pages) — worth re-verifying this one sentence specifically before merge, since the rest of the PR's doc-grounding is solid. Not independently re-run (author-claimed, unverified by me): Everything else — the |
|
Claude finished @kyle-sexton's task in 44s —— View job Security review complete
FindingsNo security issues found. This PR touches five files, and every change is prose/documentation or a version bump:
None of these introduce or modify a GitHub Actions workflow, a hook script, executable code, Nothing in this diff falls into this lane's scope (logic flaws, injection surfaces, credential handling, or dangerous Actions patterns) — deferring to zizmor's advisory lane doesn't apply either, since there's no workflow or dependency-pin change to statically check. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![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: 105e7801b6
ℹ️ 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 has reviewed this PR 2 times. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
…ing large timeouts Two review findings, both of which would have shipped audit rows a consumer could not trace to a documented rule — the same defect class 0.21.9 removed. The PowerShell bare-`$CLAUDE_PROJECT_DIR` row carried a quote attributed to the hooks page that is not on it. Re-fetched and searched today: the page's only placeholder-quoting guidance is the generic "In shell form, wrap each placeholder in double quotes", and it says nothing about PowerShell resolving an undefined variable. The claim also depends on whether the harness substitutes the bare `$NAME` spelling before PowerShell ever parses it, which the page does not document either — so the row rested on two unverifiable premises at once. Dropped rather than re-sourced; the generic quoting row already carries the safe advice, and a row that emits untraceable findings is worse than no row. The timeout row keyed on `> 600`, but the page documents defaults, not a maximum. A deliberately long-running hook may legitimately exceed 600, so that rule would manufacture findings against correct configuration. It now flags a recognizably millisecond-scale value — a round thousands multiple like `30000` or `120000` — and asks for corroboration from the hook's expected runtime when a value is large but not millisecond-shaped. The supporting claim about `tool_input.timeout` being "documented in milliseconds" was also over-stated: the page shows the example value `120000` but never states that field's unit in prose, so the checklist no longer says it does. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 2m 39s —— View job Review complete
This trigger fired on the latest push ( Both previously-flagged issues are resolved at HEAD:
Fresh-verified against the live page today, all accurate:
No new findings. Three surfaces ( Not independently re-run (author-claimed, unverified by me): |
|
Claude finished @kyle-sexton's task in 58s —— View job Security review complete
FindingsNo security issues found. This PR touches five files, and every change is prose/documentation or a version bump — confirmed against
None of these introduce or modify a GitHub Actions workflow, a hook script, executable code, The two latest commits ( Deferring to zizmor's advisory lane doesn't apply either, since there's no workflow or dependency-pin change in this diff. |
Main released its own 0.22.0 (#2004) while this branch was in flight; the unhobble release moves to 0.23.0 with both changelog sections preserved. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 participant
No linked issue
Consumer report drained from the handoff inbox:
20260729-170500-claude-config-plugin-audit-skill-drift. F1 and F2 were already fixed at HEAD; this closes the two residual findings. F3 was flagged CONTESTED by prior triage and needed adjudication against current docs, not implementation.F3 — UPHELD, but the reporter's reason was wrong
The checklist's own rationale was that exec form "backslash-mangles
${CLAUDE_PROJECT_DIR}on native Windows." Per the hooks page fetched this session (https://code.claude.com/docs/en/hooks), Exec form and shell form:Mangling is a shell artifact and exec form has no shell, so that mechanism is falsified. The page also contradicts the prescription directly:
But the observation behind the finding was misdiagnosed, not invented. There is a real, documented Windows constraint, and it is narrower: "On Windows, exec form requires
commandto resolve to a real executable such as a.exe." This repo hit exactly that in #1006 —"command": "bash"resolved to the WSL relaySystem32\bash.exe, and because a failed hook launch is non-blocking, a destructive guard silently enforced nothing across an 82-repo session.So the fix is to replace the wrong mechanism with the right one rather than to delete the row:
bash/share ordinary executables elsewhere), naming both documented remedies: a real binary with the script path inargs, or shell form with"shell": "bash";That second scoping is deliberate. Quoted shell form is a spelling the page endorses for pipes,
&&, redirects, and.cmdshims, and it is what this repo's own.claude/settings.jsonhooks use — a check that condemned shell form outright would have replaced a false negative with the mirror-image false positive.The old mangling claim is not preserved as a caveat. Keeping the false mechanism next to the correct one would re-seed the error.
F4 — added, unit confirmed from the same page
New Category D row flags
timeout > 600as near-certainly milliseconds, and names where the confusion documentably comes from: on that same page, the Bash/PowerShelltool_input.timeoutis "Optional timeout in milliseconds" with example120000— which read as seconds is about 33 hours. A second row covers the bare$CLAUDE_PROJECT_DIRspelling in PowerShell shell-form hooks, which the page says PowerShell "resolves to$null".Method note carried from the report
The doc was verified twice — WebFetch and the raw
.mdvia curl with grep — because the inbox item's own method note records the WebFetch summarizer fabricating on long docs pages. Worth knowing for the next contract-surface change: the summarizer is a lossy read of a page whose exact wording is the thing being adjudicated.Verification
markdownlint-cli2(22 files)skill-qualitycheck-skill.sh auditcheck-changelog-parity.sh --check-bump origin/maincheck-orphaned-fixtures.shwas not run locally; it exceeds a 300s timeout on this machine. CI covers it.claude-config0.21.9 → 0.22.0 — minor, not patch: consumer-visible check behavior moves in both directions (one warning retired, four added). Three surfaces move together:reference/audit-checklist.md,context/validation-categories.md, andSKILL.md's Category D summary.Out of scope, surfaced not fixed
audit/evals/evals.jsonhas no hook-form coverage at all today, which is a larger gap than these two findings.skills/audit/SKILL.mdis 251 lines against skill-quality's 200-line soft target (pre-existing warning).auditskill has no Gotchas surface despite a documented failure history — this finding plus 0.21.9's inoperable:*check would both belong there..md+ grep over the WebFetch summarizer for long docs pages) is not reflected in the audit skill's Phase 3 instructions.docs/PLUGIN-PHILOSOPHY.mdanddocs/conventions/hook-config-delivery/README.mdalready state the exec-form rules correctly and were left alone.Related
20260729-170500-claude-config-plugin-audit-skill-drift— the consumer report. F1 and F2 were already fixed at HEAD.