From bc07f7a1fd10614ed5aa7563cad47f35b4a69377 Mon Sep 17 00:00:00 2001 From: Mohamed Hegazy Date: Fri, 29 May 2026 13:08:46 -0700 Subject: [PATCH] security-guidance: 5 precise if entries fixing #2089 regression + gt support MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 ` 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: ":*"}` 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) --- plugins/security-guidance/hooks/hooks.json | 28 +++++++++++++++++-- .../hooks/security_reminder_hook.py | 10 ++++++- 2 files changed, 35 insertions(+), 3 deletions(-) diff --git a/plugins/security-guidance/hooks/hooks.json b/plugins/security-guidance/hooks/hooks.json index 38390d1a..39dbac57 100644 --- a/plugins/security-guidance/hooks/hooks.json +++ b/plugins/security-guidance/hooks/hooks.json @@ -37,7 +37,7 @@ { "type": "command", "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, "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" @@ -45,7 +45,31 @@ { "type": "command", "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, "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" diff --git a/plugins/security-guidance/hooks/security_reminder_hook.py b/plugins/security-guidance/hooks/security_reminder_hook.py index 6fab7a66..7406a871 100755 --- a/plugins/security-guidance/hooks/security_reminder_hook.py +++ b/plugins/security-guidance/hooks/security_reminder_hook.py @@ -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 # same change set — without that widening this regex change is dead code # 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 ` 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 # preserved from the original) OR `gt modify` which is semantically an # amend. The handler treats matches as "find the pre-amend SHA via reflog