security-guidance: 5 precise if entries fixing #2089 regression + gt support

URGENT REGRESSION FIX. PR #2076 (Graphite gt workflow) gated the
PostToolUse commit/push hooks with:

    "if": "Bash(git commit:*)|Bash(gt create:*)|Bash(gt modify:*)"
    "if": "Bash(git push:*)|Bash(gt submit:*)"

mirroring the regex-OR idiom that `matcher` uses
("Edit|Write|MultiEdit|NotebookEdit"). But `if` is NOT a regex —
it's a SINGLE permission-rule string. The CC harness's dispatch
filter parses the entire `if` value as one rule of shape
`ToolName(rule_content)` via:

    let firstParen = H.indexOf("(");
    let lastParen  = H.lastIndexOf(")");      // searches from END
    if (lastParen !== H.length - 1) return { toolName: H };
    let toolName    = H.slice(0, firstParen);
    let ruleContent = H.slice(firstParen + 1, lastParen);

Applied to the broken commit clause:
    toolName    = "Bash"
    ruleContent = "git commit:*)|Bash(gt create:*)|Bash(gt modify:*"

The garbled `ruleContent` never matches any real command, so the
hook never fires — for ANY workflow, not just gt. The plugin's
deepest review layer was dead in production for all users on builds
shipping PR #2076.

Fix shape: split into separate hook entries, each with its own
well-formed single-rule `if` clause. The Python hook self-routes
commit vs push via the bash-command regexes and dedups concurrent
spawns via `_claim_bash_hook_once`, so multiple entries firing the
same script is safe.

This commit:

1. hooks.json: 5 precise entries (one per command shape) instead of
   the broken |-joined 2-entry form. Restores the original commit/
   push behavior bit-for-bit (`Bash(git commit:*)` + `Bash(git push:*)`
   are unchanged from pre-#2076), and adds 3 separate entries for
   the Graphite commands (`Bash(gt create:*)`, `Bash(gt modify:*)`,
   `Bash(gt submit:*)`). No git behavior change.

   The earlier draft used the broader `Bash(git *)` + `Bash(gt *)`
   per the reporter's suggestion, but that has a real cost: every
   `git status` / `git log` / `git diff` would spawn the Python
   hook only to early-exit via the regex matcher. Precise per-command
   entries avoid the spawn overhead and match the pre-#2076 cost
   profile exactly.

2. security_reminder_hook.py: widen `_GIT_COMMIT_RE` to tolerate
   `git -C <path>` and `git -c k=v` global options between `git`
   and `commit` (mirrors `_GIT_PUSH_RE`'s long-standing tolerance).
   Without this, `git -C /repo commit` is silently dropped by the
   handler — reporter flagged this as the secondary finding.

Verified locally on macOS Python 3.13:

  - hooks.json valid JSON, 5 `if` clauses each parses to a single
    `{toolName: "Bash", ruleContent: "<command>:*"}` pair.
  - py_compile security_reminder_hook.py clean.
  - 9-case regex sanity: all 4 commit forms match (bare, -C path,
    -c k=v, gt create/modify); 3 non-commit forms reject (status,
    gt submit, gt log). Pre-fix would reject -C path form.
  - 7 new tests in test_2089_if_clause_validity.py + 2 updated tests
    in test_gt_graphite_workflow.py:
      * 12 sanity tests for a Python parser mirroring harness's BA(H)
        — pinned so a future refactor can't silently start accepting
        the broken form.
      * 2 hooks.json validity: every `if` clause parses as a single
        valid rule; at least one if-gated hook exists.
      * 1 post-fix structure: separate entries cover git AND gt.
      * 2 updated gt-coverage: SOME clause covers git, SOME clause
        covers gt (no longer requires both in the same |-joined
        clause, which was the broken shape).

    TDD-verified the test catches the bug: temporarily restored
    main's broken |-joined hooks.json, ran the new test, saw
    `test_every_if_clause_is_single_valid_rule` fail with a clear
    error explaining #2089's cause. Restored fix, test passes.

  - Full suite: 336/353 pass (17 unrelated failures from open PRs
    #2078 / #2086 not in this branch).

NOT verified end-to-end with a real CC instance triggering the hooks
on a git or gt commit. The static-shape tests catch the regression
class and the regex sanity tests pin the `git -C` tolerance, but
the asyncRewake feedback loop needs runtime verification.

Closes #2089. Restores the closes for #2048 that PR #2076 attempted.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Mohamed Hegazy 2026-05-29 13:08:46 -07:00
parent 8435428dfc
commit bc07f7a1fd
No known key found for this signature in database
2 changed files with 35 additions and 3 deletions

View File

@ -37,7 +37,7 @@
{ {
"type": "command", "type": "command",
"command": "bash \"${CLAUDE_PLUGIN_ROOT}/hooks/sg-python.sh\" \"${CLAUDE_PLUGIN_ROOT}/hooks/security_reminder_hook.py\"", "command": "bash \"${CLAUDE_PLUGIN_ROOT}/hooks/sg-python.sh\" \"${CLAUDE_PLUGIN_ROOT}/hooks/security_reminder_hook.py\"",
"if": "Bash(git commit:*)|Bash(gt create:*)|Bash(gt modify:*)", "if": "Bash(git commit:*)",
"asyncRewake": true, "asyncRewake": true,
"rewakeMessage": "Background security review of commit — address or acknowledge the findings below, then continue with the user's original request or continue waiting for their reply:", "rewakeMessage": "Background security review of commit — address or acknowledge the findings below, then continue with the user's original request or continue waiting for their reply:",
"rewakeSummary": "Commit security review found issues" "rewakeSummary": "Commit security review found issues"
@ -45,7 +45,31 @@
{ {
"type": "command", "type": "command",
"command": "bash \"${CLAUDE_PLUGIN_ROOT}/hooks/sg-python.sh\" \"${CLAUDE_PLUGIN_ROOT}/hooks/security_reminder_hook.py\"", "command": "bash \"${CLAUDE_PLUGIN_ROOT}/hooks/sg-python.sh\" \"${CLAUDE_PLUGIN_ROOT}/hooks/security_reminder_hook.py\"",
"if": "Bash(git push:*)|Bash(gt submit:*)", "if": "Bash(git push:*)",
"asyncRewake": true,
"rewakeMessage": "Background security review of pushed commits not yet reviewed — address or acknowledge the findings below, then continue with the user's original request or continue waiting for their reply:",
"rewakeSummary": "Push security review found issues"
},
{
"type": "command",
"command": "bash \"${CLAUDE_PLUGIN_ROOT}/hooks/sg-python.sh\" \"${CLAUDE_PLUGIN_ROOT}/hooks/security_reminder_hook.py\"",
"if": "Bash(gt create:*)",
"asyncRewake": true,
"rewakeMessage": "Background security review of commit — address or acknowledge the findings below, then continue with the user's original request or continue waiting for their reply:",
"rewakeSummary": "Commit security review found issues"
},
{
"type": "command",
"command": "bash \"${CLAUDE_PLUGIN_ROOT}/hooks/sg-python.sh\" \"${CLAUDE_PLUGIN_ROOT}/hooks/security_reminder_hook.py\"",
"if": "Bash(gt modify:*)",
"asyncRewake": true,
"rewakeMessage": "Background security review of commit — address or acknowledge the findings below, then continue with the user's original request or continue waiting for their reply:",
"rewakeSummary": "Commit security review found issues"
},
{
"type": "command",
"command": "bash \"${CLAUDE_PLUGIN_ROOT}/hooks/sg-python.sh\" \"${CLAUDE_PLUGIN_ROOT}/hooks/security_reminder_hook.py\"",
"if": "Bash(gt submit:*)",
"asyncRewake": true, "asyncRewake": true,
"rewakeMessage": "Background security review of pushed commits not yet reviewed — address or acknowledge the findings below, then continue with the user's original request or continue waiting for their reply:", "rewakeMessage": "Background security review of pushed commits not yet reviewed — address or acknowledge the findings below, then continue with the user's original request or continue waiting for their reply:",
"rewakeSummary": "Push security review found issues" "rewakeSummary": "Push security review found issues"

View File

@ -640,7 +640,15 @@ _COMMIT_SHA_RE = re.compile(r'^\[[^\]]*?\b([0-9a-f]{7,40})\]', re.MULTILINE)
# widening for `gt create:*` / `gt modify:*` / `gt submit:*` ships in the # widening for `gt create:*` / `gt modify:*` / `gt submit:*` ships in the
# same change set — without that widening this regex change is dead code # same change set — without that widening this regex change is dead code
# because the hook subprocess never spawns for gt invocations. See #2048. # because the hook subprocess never spawns for gt invocations. See #2048.
_GIT_COMMIT_RE = re.compile(r'\b(?:git\s+commit|gt\s+(?:create|modify))(?:\s|$)') _GIT_COMMIT_RE = re.compile(
# `git -C <path>` and `git -c key=val` global options are allowed between
# `git` and `commit` (mirrors the long-standing tolerance in
# _GIT_PUSH_RE). Without this, `git -C /repo commit` is silently dropped
# by the handler — see #2089's secondary finding. The gt branch has no
# global-option layer to worry about.
r'\bgit(?:\s+-[Cc]\s+\S+|\s+--\S+=\S+)*\s+commit\b'
r'|\bgt\s+(?:create|modify)\b'
)
# Match either the `--amend` flag (with the leading whitespace boundary # Match either the `--amend` flag (with the leading whitespace boundary
# preserved from the original) OR `gt modify` which is semantically an # preserved from the original) OR `gt modify` which is semantically an
# amend. The handler treats matches as "find the pre-amend SHA via reflog # amend. The handler treats matches as "find the pre-amend SHA via reflog