Skip to content

fix(hook-utils): distinguish unresolved helpers and honor # comments - #2549

Merged
kyle-sexton merged 2 commits into
mainfrom
cursor/fix-1487-hook-utils-fail-closed-2ae1
Aug 13, 2026
Merged

fix(hook-utils): distinguish unresolved helpers and honor # comments#2549
kyle-sexton merged 2 commits into
mainfrom
cursor/fix-1487-hook-utils-fail-closed-2ae1

Conversation

@kyle-sexton

@kyle-sexton kyle-sexton commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Fixes #1487

Summary

Shared hook-utils.sh helpers no longer answer "I could not determine this" as an indistinguishable success:

  • hook::physical_path — returns 1 and sets HOOK_PHYSICAL_PATH_UNRESOLVED=1 when neither realpath nor readlink -f resolves, while still printing the lexical path for advisory callers.
  • hook::repo_root — returns 1 and sets HOOK_REPO_ROOT_UNRESOLVED=1 when git rev-parse --show-toplevel cannot answer, while still printing the hint fallback.
  • hook::bash_parse_segments — unquoted # starts a shell comment to EOL; quoted # is preserved.

Contract is documented at each helper: return 0 means resolved; advisory callers that ignore the status keep today's behavior.

Tests

lib/hook-utils.test.sh covers path-helper failure signals and the # comment bypass shape (git b # -C other-repo).

Propagation

Synced via scripts/sync-hook-utils.sh; all 16 carrying plugins bumped with changelog entries.

Related

N/A

@cursor

cursor Bot commented Aug 13, 2026

Copy link
Copy Markdown

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.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6d8bd49d44

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread lib/hook-utils.sh
@cursor
cursor Bot force-pushed the cursor/fix-1487-hook-utils-fail-closed-2ae1 branch from 6d8bd49 to d3cbc22 Compare August 13, 2026 03:00
@claude

claude Bot commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Claude finished @cursor[bot]'s task in 3m 41s —— View job


Claude finished the security review

  • Gather context (PR diff, files, custom instructions)
  • Invoke /review:security-review and follow its instructions
  • Post findings

Security review of PR #2549

Scope: hook::bash_parse_segments's new #-comment handling in lib/hook-utils.sh (propagated to 16 plugin copies). The hook::physical_path / hook::repo_root fail-closed-signal changes are a net-positive hardening with no new callers relying on the flags yet, so no issue there.

CRITICAL — mid-word # lets a dangerous git command bypass all guardrails hooks

lib/hook-utils.sh#L1825-L1836

Bash only treats # as a comment introducer when it is the first character of a word (nothing accumulated yet since the last separator/operator). This new case triggers comment-skip-to-EOL on any unquoted #, including one that lands mid-word (have == 1) — it flushes the in-progress word first and then discards everything up to the newline, rather than treating # as a literal character the way real bash does.

Concretely, for the command string:

true#; git push --force
  • Real bash tokenizes true# as one word (terminated by ;, a control operator), then runs git push --force as a second command — the dangerous git invocation executes.
  • hook::bash_parse_segments hits # with have=1 (mid-word), flushes true as a complete segment, then treats ; git push --force as a comment and never emits it as a segment to the callback at all.

This parser is the shared tokenizing spine for plugins/guardrails/hooks/block-dangerous-git.sh, block-no-verify.sh, block-noncanonical-commit.sh, and block-convention-violation.sh (each calls hook::bash_parse_segments "$COMMAND" check_segment directly on the full tool-call command string). Because the trailing segment is silently swallowed as a "comment," check_segment never sees the git invocation, so the guard returns "allow" for a command it exists specifically to block — git reset --hard, git push --force, --no-verify, etc. can all be smuggled past these hooks with an innocuous word immediately followed by # earlier in the same shell string. This is a straightforward fail-open bypass of a security control, introduced by this PR's own new code (previously a mid-word # had no special handling and stayed literal, so the guard saw the real segments).

The PR's own new tests (lib/hook-utils.test.sh) only cover word-start # (git b # -C other-repo) and quoted #; they don't exercise the mid-word case, so this regression isn't caught by the added suite.

Suggested fix — only enter comment mode when no word is currently being assembled; otherwise treat # as an ordinary word character (mirrors the *) default case):

    '#')
      # Unquoted `#` starts a shell comment to EOL only when it begins a
      # word (bash only treats a word-initial `#` as a comment introducer);
      # a `#` continuing a word already in progress is a literal character.
      if ((have)); then
        word+="$c"
      else
        while ((i + 1 < n)) && [[ "${chars[i + 1]}" != $'\n' ]]; do
          ((i++))
        done
      fi
      ;;

This is the same issue flagged by the automated Codex review comment on this PR (lib/hook-utils.sh:1828, P1) — independently reproduced above by tracing it through to the live guardrails call sites, confirming it's a genuine, exploitable, security-relevant bug rather than a false positive.

Fix this →

No other security-relevant issues found in this diff's scope (the physical_path/repo_root unresolved-signal work is additive and correctly documented as advisory-by-default).

@github-actions

github-actions Bot commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Last security-reviewed head: 11a410d1bd21e2cf9522789ef3510bb1988c357d. On the next push, the relevance gate compares only the commits since this SHA; delete this comment to force a full re-review.

@cursor
cursor Bot force-pushed the cursor/fix-1487-hook-utils-fail-closed-2ae1 branch 2 times, most recently from 1107116 to 56865ba Compare August 13, 2026 03:11
@claude

claude Bot commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Claude finished @cursor[bot]'s task in 2m 12s —— View job


Claude finished the security review

  • Gather context (PR diff, files, custom instructions)
  • Invoke /review:security-review and follow its instructions
  • Post findings

Security review of PR #2549 (head 56865ba)

Scope: lib/hook-utils.sh and its 16 synced plugin copies (git diff origin/main...HEAD). I checked whether this push (d3cbc22456865ba) changed anything security-relevant — it did not: git diff d3cbc224...56865ba -- lib/hook-utils.sh lib/hook-utils.test.sh is empty. The SHA change is from a rebase that picked up an unrelated, already-merged commit (fix(source-control): refuse bulk --include-human..., #2547); the hook-utils.sh content is byte-identical to what was already reviewed.

CRITICAL (still open) — mid-word # lets a dangerous git command bypass all guardrails hooks

lib/hook-utils.sh#L1825-L1836

Real bash only treats # as a comment introducer when it's the first character of a word (nothing accumulated since the last separator/operator). This case treats any unquoted # as starting a comment, including one appearing mid-word (have == 1): it flushes the in-progress word and then discards everything to EOL, rather than treating # as a literal character the way bash does.

For true#; git push --force:

  • Real bash tokenizes true# as one word (terminated by ;), then runs git push --force as a second command.
  • hook::bash_parse_segments hits # at have=1, flushes true, then swallows ; git push --force as a "comment" — that segment is never passed to the callback.

This function is the shared tokenizing spine for block-dangerous-git.sh, block-no-verify.sh, block-noncanonical-commit.sh, and block-convention-violation.sh in the guardrails plugin (each calls hook::bash_parse_segments "$COMMAND" check_segment directly on the full tool-call command, confirmed via the current call sites at block-dangerous-git.sh:1389, block-no-verify.sh:285, block-noncanonical-commit.sh:927, block-convention-violation.sh:465). Because the trailing segment is silently dropped, check_segment never sees the git invocation and the guard fails open on exactly the commands it exists to block (git reset --hard, git push --force, --no-verify, etc.) — smuggled past by an innocuous word immediately followed by # earlier in the same shell string. This regressed with this PR's new code (previously a mid-word # had no special handling and stayed literal).

lib/hook-utils.test.sh's new cases only cover word-start # (git b # -C other-repo) and quoted #; the mid-word case isn't exercised, so the suite doesn't catch this.

Suggested fix — only enter comment mode when no word is currently being assembled; otherwise treat # as an ordinary word character (mirrors the *) default case):

    '#')
      # Unquoted `#` starts a shell comment to EOL only when it begins a
      # word (bash only treats a word-initial `#` as a comment introducer);
      # a `#` continuing a word already in progress is a literal character.
      if ((have)); then
        word+="$c"
      else
        while ((i + 1 < n)) && [[ "${chars[i + 1]}" != $'\n' ]]; do
          ((i++))
        done
      fi
      ;;

This is the same issue independently flagged by Codex (lib/hook-utils.sh:1828, P1) and by the previous automated Claude review on commit d3cbc224 — restated here because it is still unresolved and this PR cannot safely merge with it in place, given the change hasn't been pushed to any of the 16 synced copies either.

Fix this →

No other security-relevant issues in this diff's scope — the hook::physical_path / hook::repo_root unresolved-signal work remains additive and advisory-by-default, with no callers yet relying on the new flags.

@claude

claude Bot commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Claude finished @cursor[bot]'s task in 3m 15s —— View job


Code review complete

  • Gather context (PR diff, CLAUDE.md, REVIEW.md)
  • Review changed files for correctness/maintainability (code-review lane; security scoped to the existing security lane)
  • Post findings

Scope note: This repo runs both claude-review and claude-security-review (.github/workflows/claude-security-review.yml exists), so per REVIEW.md this lane owns correctness/design/conventions/error-handling/observability/tests/docs and explicitly omits security findings even when a hunk plainly contains one — those belong exclusively to the security lane.

Findings

None in this lane's scope.

The one substantive defect in this diff — hook::bash_parse_segments's new # case in lib/hook-utils.sh treating any unquoted # (including mid-word) as a comment starter, when real bash only does this word-initially — is a guardrails-bypass security bug (block-dangerous-git.sh/block-no-verify.sh/etc. can be tricked into silently dropping a dangerous trailing git command as a "comment"). It's already been reported twice: by the Codex review (lib/hook-utils.sh:1828, P1) and independently by this PR's own Claude security-review pass. Per the scope split above it belongs exclusively to the security lane, so I'm not duplicating it here — just flagging that it's the reason this otherwise-clean diff isn't fully done yet.

Verification performed (code-review lane checks)

  • Propagation correctness: all 16 plugin copies of hook-utils.sh are byte-identical to lib/hook-utils.sh (sha256sum match) — the scripts/sync-hook-utils.sh propagation did its job correctly.
  • Version/changelog consistency: every plugin's CHANGELOG.md top entry version matches its .claude-plugin/plugin.json version field across all 16 changed plugins — no drift.
  • Behavioral-compatibility of the new fail-closed signals: hook::physical_path/hook::repo_root now return 1 on an unresolved answer instead of always 0. Checked every existing call site (grep across plugins/*/hooks/*.sh) — all are bare VAR=$(hook::repo_root ...) / VAR=$(hook::normalize_path "$(hook::physical_path ...)") assignments, none gate on the new return code, and none of the consuming hook scripts use set -e. So existing "advisory" callers are unaffected, matching the PR's documented contract ("advisory callers that ignore the status keep today's behavior"). No consumer currently reads HOOK_PHYSICAL_PATH_UNRESOLVED/HOOK_REPO_ROOT_UNRESOLVED yet, which is consistent with this PR's stated scope of adding the distinguishable signal without changing any guard's behavior.
  • Test additions in lib/hook-utils.test.sh for the resolved/unresolved path helpers and the comment-parsing cases are structurally sound and exercise both the success and the documented-fallback paths.

@cursor
cursor Bot force-pushed the cursor/fix-1487-hook-utils-fail-closed-2ae1 branch 2 times, most recently from c8c3f8c to 11a410d Compare August 13, 2026 03:17
@claude

claude Bot commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Claude finished @cursor[bot]'s task in 1m 25s —— View job


Claude finished the security review

  • Gather context (PR diff, files, custom instructions)
  • Invoke /review:security-review and follow its instructions
  • Post findings

Security review of PR #2549 (head 11a410d)

Scope: lib/hook-utils.sh and its 16 synced plugin copies. I re-verified git diff 56865ba...11a410d -- lib/hook-utils.sh — the helper's logic is byte-identical to what was already reviewed on this PR; only lib/hook-utils.test.sh (removed 4 duplicate bps_collect() { bps_last=("$@"); } lines) and trailing blank lines in a few CHANGELOG.md files changed.

CRITICAL (still open, unaddressed) — mid-word # lets a dangerous git command bypass all guardrails hooks

lib/hook-utils.sh#L1825-L1836

Real bash only treats # as a comment introducer when it's the first character of a word (nothing accumulated since the last separator/operator). This case treats any unquoted # as starting a comment — including one appearing mid-word (have == 1) — flushing the in-progress word and then discarding everything to EOL, rather than treating # as a literal character the way bash does.

I traced the tokenizer end-to-end to confirm exploitability, not just eyeballed the diff. For true#; git push --force (no trailing newline):

  • Real bash tokenizes true# as one word (terminated by ;, a control operator), then runs git push --force as a second command.
  • hook::bash_parse_segments hits # at have=1 (L1828), flushes true into seg, then the while at L1833-1835 advances i straight to end-of-string — skipping over the ; that would normally trigger cb emission at L1924-1933. The loop ends with only seg=(true), which is emitted once at L1968. git push --force is never passed to the callback.

This function is the shared tokenizing spine for the guardrails plugin's block-dangerous-git.sh (L1389), block-no-verify.sh (L285), block-noncanonical-commit.sh (L927), and block-convention-violation.sh (L465) — each calls hook::bash_parse_segments "$COMMAND" check_segment directly on the full tool-call command string. Because the trailing segment is silently dropped, check_segment never sees the dangerous invocation and the guard fails open on exactly the commands it exists to block (git reset --hard, git push --force, --no-verify, non-canonical commits, etc.), smuggled past by any innocuous word immediately followed by # earlier in the same shell string. This regressed with this PR's new code — previously a mid-word # had no special handling and stayed literal, so the real segments were still seen.

lib/hook-utils.test.sh's new cases only cover word-start # (git b # -C other-repo) and quoted #; the mid-word case is not exercised, so the added suite doesn't catch this.

This is the same issue flagged by the automated Codex review comment on this PR (lib/hook-utils.sh:1828, P1) and by both prior automated security-review passes on this PR (commits d3cbc224 and 56865ba7) — restated here because the code is unchanged and it remains unresolved and unpropagated to any of the 16 synced plugin copies.

Suggested fix — only enter comment mode when no word is currently being assembled; otherwise treat # as an ordinary word character (mirrors the *) default case at L1961-1964):

    '#')
      # Unquoted `#` starts a shell comment to EOL only when it begins a
      # word (bash only treats a word-initial `#` as a comment introducer);
      # a `#` continuing a word already in progress is a literal character.
      if ((have)); then
        word+="$c"
      else
        while ((i + 1 < n)) && [[ "${chars[i + 1]}" != $'\n' ]]; do
          ((i++))
        done
      fi
      ;;

Fix this →

No other security-relevant issues in this diff's scope — the hook::physical_path / hook::repo_root unresolved-signal work remains additive and advisory-by-default, with no callers yet relying on the new flags.

@github-actions

github-actions Bot commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Claude has reviewed this PR 5 times. The lane skips further automatic reviews after 5; deleting this comment resets the count.

@claude

claude Bot commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Claude finished @cursor[bot]'s task in 3m 7s —— View job


Code review complete

  • Gather context (PR diff, files, custom instructions)
  • Invoke /review:code-review and follow its instructions
  • Post findings

Code review of PR #2549 (/review:code-review lane)

Scope: lib/hook-utils.sh (source of truth; the 16 plugin copies are byte-identical syncs) and lib/hook-utils.test.sh. The CHANGELOG/plugin.json changes across all 16 plugins are mechanical version bumps (each a correct patch increment) with no issues.

This repo carries a dedicated claude-security-review lane, which has already reviewed this PR twice and flagged a CRITICAL, confirmed bug: the new '#' case in hook::bash_parse_segments (lib/hook-utils.sh#L1825-L1836) starts a comment on any unquoted #, including mid-word, where real bash only treats a word-initial # as a comment introducer — letting a trailing dangerous git command be silently swallowed and bypass the guardrails plugin's blocking hooks. Per this lane's scope-out rule ("Scope security findings out of this lane wherever the consumer carries a claude-security-review workflow file"), I did not re-report it here — see the security lane's comments on this PR for the full analysis and suggested fix. It should still block merge regardless of which lane's comment is authoritative.

No additional correctness or maintainability issues found in this diff's scope:

  • hook::physical_path / hook::repo_root: the new HOOK_*_UNRESOLVED flag plumbing and return 0/return 1 contract are internally consistent, correctly documented at each helper, and preserve the pre-existing fallback output for advisory callers that ignore the status.
  • None of the direct callers of hook::repo_root/hook::physical_path in this diff's blast radius run under set -e, so the new non-zero returns can't trip an unexpected errexit in existing callers (checked all call sites across the changed plugins).
  • The #-comment skip-to-EOL loop mechanics mirror the existing heredoc-body skip pattern in the same function (lib/hook-utils.sh:1943-1945) — structurally consistent with the rest of the parser.
  • lib/hook-utils.test.sh's new coverage (word-start #, quoted #, segment-scoped #, and the resolved/unresolved probes for both path helpers) follows the file's existing test idioms (mktemp -d for a guaranteed non-repo hint, BASH_ENV shims to neuter realpath/readlink) and asserts both the return code and the new flag.

One minor documentation note, not blocking: the shellcheck disable=SC2034 comments on both helpers say HOOK_PHYSICAL_PATH_UNRESOLVED/HOOK_REPO_ROOT_UNRESOLVED "is read by sourcing hooks," but no hook in this diff's scope currently reads either flag — only the new test probes do. Worth softening once real fail-closed callers adopt the signal, so future readers don't assume an enforcement path exists today that doesn't.

Test suite execution wasn't independently verified in this pass (Bash execution beyond git/gh read-only commands wasn't available in this lane's tool grant) — the PR's own description of lib/hook-utils.test.sh coverage is author-claimed and unverified by me.
· branch cursor/fix-1487-hook-utils-fail-closed-2ae1

@cursor
cursor Bot force-pushed the cursor/fix-1487-hook-utils-fail-closed-2ae1 branch from 11a410d to d6026b4 Compare August 13, 2026 03:33
@claude

claude Bot commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Claude finished @cursor[bot]'s task in 2m 37s —— View job


Code review complete

  • Gather context (PR diff, CLAUDE.md, REVIEW.md)
  • Invoke /review:code-review and follow its instructions
  • Post findings

Scope note: Per REVIEW.md, this repo carries a claude-security-review.yml workflow, so this lane owns correctness/design/conventions/error-handling/observability/tests/documentation and explicitly omits security findings — those belong exclusively to the security lane.

Good news: the critical bug is fixed

Every prior automated review on this PR (Codex, and three passes of the security lane) flagged the same CRITICAL issue: the new '#' case in hook::bash_parse_segments was starting a comment on any unquoted #, including mid-word, which let a trailing dangerous git command be silently swallowed as a "comment" and bypass the guardrails plugin's blocking hooks (block-dangerous-git.sh, block-no-verify.sh, etc.).

I verified against the current head (d6026b4) and this is now fixed: lib/hook-utils.sh#L1825-L1834 correctly checks if ((have)) and appends # literally when a word is already being assembled, matching real bash semantics (comment introducer only at word-start). A regression test now covers exactly this shape at lib/hook-utils.test.sh#L2738-L2742 (echo x#y; git reset --hard → asserts the full segment list, including the git reset --hard segment, survives).

Findings

One non-blocking documentation nit posted inline on lib/hook-utils.sh:352 (applies equally to line 483): the shellcheck disable=SC2034 comments claim HOOK_PHYSICAL_PATH_UNRESOLVED/HOOK_REPO_ROOT_UNRESOLVED are "read by sourcing hooks," but no hook in plugins/*/hooks/*.sh currently reads either flag — only this PR's own test probes do. Consistent with the PR's stated scope (add the signal, don't wire up a consumer yet), just worth softening the wording so it doesn't imply an enforcement path that isn't there yet.

Verification performed

  • Propagation correctness: all 16 plugin copies of hook-utils.sh are byte-identical to lib/hook-utils.sh (sha256sum match across all copies, including guardrails, source-control, typos-format, ruff-format, markdown-format).
  • Version/changelog consistency: spot-checked several plugins (e.g. guardrails0.28.20) — CHANGELOG.md top entry matches .claude-plugin/plugin.json version.
  • Behavioral compatibility of the new fail-closed signals: hook::physical_path/hook::repo_root now return 1 on an unresolved answer instead of always 0, while still emitting the same fallback output. No existing caller of either helper gates on the return code or reads the new HOOK_*_UNRESOLVED flags yet, so advisory callers are unaffected — matches the PR's documented contract.
  • Test coverage: the new physical_path_resolved/physical_path_unresolved/repo_root_resolved/repo_root_unresolved probes (lib/hook-utils.test.sh:2595-2700) correctly assert both return code and flag, using BASH_ENV shims to neuter realpath/readlink and mktemp -d for a guaranteed non-repo hint. The #-comment tests cover word-start, mid-word, and quoted shapes.

I did not have shellcheck available in this environment's tool grant, so lint cleanliness is unverified by me (author-claimed only).
· branch cursor/fix-1487-hook-utils-fail-closed-2ae1

Comment thread lib/hook-utils.sh Outdated
@cursor
cursor Bot force-pushed the cursor/fix-1487-hook-utils-fail-closed-2ae1 branch from d6026b4 to 14b7d43 Compare August 13, 2026 03:40
@claude

claude Bot commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Claude finished @cursor[bot]'s task in 2m 38s —— View job


Code review complete

  • Gather context (PR diff, CLAUDE.md, REVIEW.md)
  • Invoke /review:code-review and follow its instructions
  • Post findings

Scope note: Per REVIEW.md, this repo carries a claude-security-review.yml workflow, so this lane owns correctness/design/conventions/error-handling/observability/tests/documentation and explicitly omits security findings — those belong exclusively to the security lane.

Good news: the critical bug remains fixed

Every earlier automated review on this PR (Codex, and multiple passes of the security lane) flagged a CRITICAL issue: the new '#' case in hook::bash_parse_segments was starting a comment on any unquoted #, including mid-word, which let a trailing dangerous git command be silently swallowed as a "comment" and bypass the guardrails plugin's blocking hooks (block-dangerous-git.sh, block-no-verify.sh, etc.).

Re-verified against the current head (14b7d43): still fixed. lib/hook-utils.sh#L1825-L1834 correctly checks if ((have)) and appends # literally when a word is already being assembled, matching real bash semantics (comment introducer only at word-start). The regression test at lib/hook-utils.test.sh#L2738-L2743 still covers exactly this shape (echo x#y; git reset --hard → asserts both segments, including git reset --hard, survive).

Findings

One non-blocking documentation nit, posted inline on lib/hook-utils.sh:483: the shellcheck disable=SC2034 comment for HOOK_REPO_ROOT_UNRESOLVED still claims it "is read by sourcing hooks," but no hook currently reads either new flag. The sibling comment for HOOK_PHYSICAL_PATH_UNRESOLVED (line 352) was already softened to "advisory callers may read..." on this revision, so line 483 is now the sole holdout and reads inconsistently with its neighbor. Suggested a matching one-line rewording inline.

Verification performed

  • Propagation correctness: all 16 plugin copies of hook-utils.sh carry the identical SHA (b26079e8daa1...) as lib/hook-utils.sh in this PR's file list — byte-identical, confirming scripts/sync-hook-utils.sh propagation worked correctly.
  • Version/changelog consistency: spot-checked guardrails and others — CHANGELOG.md top entry matches .claude-plugin/plugin.json version.
  • No premature consumers: grep -rn 'HOOK_PHYSICAL_PATH_UNRESOLVED\|HOOK_REPO_ROOT_UNRESOLVED' plugins/**/hooks/*.sh only matches the hook-utils.sh copies' own definitions — no guard hook reads the new flags yet, consistent with this PR's stated scope of adding a distinguishable signal without wiring up a fail-closed consumer.
  • Behavioral compatibility: hook::physical_path/hook::repo_root now return 1 on an unresolved answer instead of always 0, while still emitting the same fallback output for advisory callers — matches the documented contract.
  • Parser diff (lib/hook-utils.sh L1710-1850) reviewed line-by-line against bash tokenization semantics for the new #-comment case interacting with the existing quote/backslash/redirection branches — no other bypass shape found (e.g. escaped \# is consumed by the pre-existing backslash branch before reaching the # case, so it correctly stays literal without special-casing).

I did not have permission to execute bash -n/run the test suite directly in this environment's tool grant, so I did not independently execute lib/hook-utils.test.sh — the PR's test coverage was verified by reading, not running.
· branch cursor/fix-1487-hook-utils-fail-closed-2ae1

Comment thread lib/hook-utils.sh Outdated
…1487)

Give hook::physical_path and hook::repo_root a non-zero return and
HOOK_*_UNRESOLVED flag when they fall back to a lexical answer, while
still emitting that answer for advisory callers. Teach
hook::bash_parse_segments to drop unquoted # comments to EOL.

Sync lib copies and bump every carrying plugin version.

Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
@cursor
cursor Bot force-pushed the cursor/fix-1487-hook-utils-fail-closed-2ae1 branch from 14b7d43 to 004f587 Compare August 13, 2026 03:46
@claude

claude Bot commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Claude finished @cursor[bot]'s task in 1m 36s —— View job


Code review complete

  • Gather context (PR diff, CLAUDE.md, REVIEW.md)
  • Invoke /review:code-review and follow its instructions
  • Post findings

Scope note: Per REVIEW.md, this repo carries a claude-security-review.yml workflow, so this lane owns correctness/design/conventions/error-handling/observability/tests/documentation and explicitly omits security findings — those belong exclusively to the security lane.

The critical bug remains fixed

Every earlier automated review on this PR (Codex, and multiple passes of the security lane) flagged a CRITICAL issue: the new '#' case in hook::bash_parse_segments was starting a comment on any unquoted #, including mid-word, which let a trailing dangerous git command be silently swallowed as a "comment" and bypass the guardrails plugin's blocking hooks (block-dangerous-git.sh, block-no-verify.sh, etc.).

Re-verified against the current head (004f587): still fixed. lib/hook-utils.sh#L1825-L1835 correctly checks if ((have)) and appends # literally when a word is already being assembled, matching real bash semantics (comment introducer only at word-start). The regression test at lib/hook-utils.test.sh#L2738-L2743 covers exactly this shape (echo x#y; git reset --hard → asserts both segments, including git reset --hard, survive).

Findings

One non-blocking documentation nit posted inline on lib/hook-utils.sh:483: the shellcheck disable=SC2034 comment still claims HOOK_REPO_ROOT_UNRESOLVED "is read by sourcing hooks," but no hook currently reads either new flag — only this PR's own test probes do. The sibling comment for HOOK_PHYSICAL_PATH_UNRESOLVED (line 352) was already softened to "advisory callers may read..." on an earlier revision, so line 483 is the sole remaining holdout and now reads inconsistently with its neighbor. Same nit raised twice on earlier revisions of this PR — restated because it's still present on this head.

Verification performed

  • Propagation correctness: all 16 plugin copies of hook-utils.sh are byte-identical to lib/hook-utils.sh (sha256sum match, spot-checked across guardrails, source-control, typos-format, actionlint) — scripts/sync-hook-utils.sh propagation worked correctly.
  • No premature consumers: no hook under plugins/*/hooks/*.sh reads HOOK_PHYSICAL_PATH_UNRESOLVED or HOOK_REPO_ROOT_UNRESOLVED yet, consistent with this PR's stated scope of adding a distinguishable signal without wiring up a fail-closed consumer.
  • Behavioral compatibility: hook::physical_path/hook::repo_root now return 1 on an unresolved answer instead of always 0, while still emitting the same fallback output for advisory callers — matches the documented contract, and no existing caller runs under set -e or gates on the return code.
  • Test coverage: the mid-word, word-start, and quoted # cases, plus the resolved/unresolved probes for both path helpers, are present and structurally sound in lib/hook-utils.test.sh.

I did not have permission to execute bash lib/hook-utils.test.sh directly in this environment's tool grant, so test-suite execution wasn't independently verified in this pass — coverage was confirmed by reading the test file, not running it.
· branch cursor/fix-1487-hook-utils-fail-closed-2ae1

Comment thread lib/hook-utils.sh Outdated
…rsor[bot]

- Align HOOK_REPO_ROOT_UNRESOLVED shellcheck comment with physical_path
- Add cursor[bot] to security-review skip-actors so cloud-agent PRs pass the
  evidence guard when the lane skips non-member bot actors (#2549)

Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
@kyle-sexton
kyle-sexton merged commit 2b4d8ab into main Aug 13, 2026
38 checks passed
@kyle-sexton
kyle-sexton deleted the cursor/fix-1487-hook-utils-fail-closed-2ae1 branch August 13, 2026 04:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

hook-utils: shared helpers degrade silently to permissive answers (fail-open defaults in the SSOT lib)

2 participants