Skip to content

fix(hooks): guard-git.sh sed patterns work on macOS BSD sed#1146

Merged
carlos-alm merged 4 commits into
mainfrom
fix/1145-guard-git-bsd-sed
May 18, 2026
Merged

fix(hooks): guard-git.sh sed patterns work on macOS BSD sed#1146
carlos-alm merged 4 commits into
mainfrom
fix/1145-guard-git-bsd-sed

Conversation

@carlos-alm
Copy link
Copy Markdown
Contributor

Summary

  • \s and s///;t;s/// flow-control are GNU sed extensions that BSD sed (macOS default) does not support. The pre-fix patterns silently produced empty output, causing the hook to fall back to the calling shell's cwd and emit false "Branch does not match required pattern" denials when pushing to another worktree (e.g. cd /path/to/wt && git push).
  • Replaced \s with [[:space:]] in all sed patterns (lines 36, 37, 124) and updated the matching grep -qE cd/-C guards for consistency.
  • Split the s/quoted/.../p;t;s/unquoted/.../p chain on the -C extraction into two separate sed -nE invocations — BSD sed parses the chained s/// after t as a label and errors with undefined label.

Reproduction (pre-fix, macOS)

echo 'git -C /tmp push' | sed -nE 's/.*-C[[:space:]]+"([^"]+)".*/\1/p;t;s/.*-C[[:space:]]+([^[:space:]]+).*/\1/p'
# → sed: undefined label

echo 'cd /tmp/wt && git push' | sed -nE 's/^\s*cd\s+"?([^"&]+)"?\s*&&.*/\1/p'
# → (empty — silent failure)

Test plan

  • Reproduced both BSD-sed failures on macOS (sed: undefined label; empty cd-extract).
  • End-to-end test with two mock worktrees (good branch + bad branch name) across 7 cases:
    • cd <good-wt> && git push from <bad-wt> → ALLOW
    • cd <bad-wt> && git push from <good-wt> → DENY (was silently ALLOWed pre-fix)
    • git -C <good-wt> push → ALLOW
    • git -C <bad-wt> push → DENY
    • git -C "<bad-wt>" push (quoted path) → DENY
    • git -C <good-wt> -C <bad-wt> push (multi-C bypass) → DENY (last -C wins)
    • cd <good-wt> && git -C <bad-wt> push → DENY (-C overrides cd)
  • Confirmed pre-fix vs post-fix delta: old cd-extract returned [], new returns the actual path.

Closes #1145

`\s` and the `s///;t;s///` flow-control chain are GNU sed extensions
that BSD sed (macOS default) does not support. When a command like
`cd /path/to/wt && git push` was issued from a different cwd, the
cd-path extraction silently returned an empty string, the hook fell
back to the calling shell's branch, and produced false "Branch does
not match required pattern" denials.

Changes:
- Replace `\s` with `[[:space:]]` in all sed patterns (lines 36, 37, 124).
- Split the `s/quoted/.../p;t;s/unquoted/.../p` chain on the `-C`
  extraction into two separate `sed -nE` invocations — BSD sed parses
  the chained `s///` after `t` as a label and errors with
  "undefined label".
- Update the matching `grep -qE` cd/-C guards to `[[:space:]]` for
  consistency.

Verified end-to-end on macOS with all four code paths the hook protects
(cd && push, -C push, -C with quoted path, multi -C, cd + -C precedence).

Closes #1145
@claude
Copy link
Copy Markdown

claude Bot commented May 17, 2026

Claude finished @carlos-alm's task —— View job


Code Review Complete

Review Tasks

  • Read and understand the changes in .claude/hooks/guard-git.sh
  • Phase 1: Root cause analysis
  • Phase 2: Configuration change detection
  • Phase 3: Test integrity assessment
  • Phase 4: Code quality review
  • Phase 5: Backlog compliance rating
  • Provide final assessment and recommendation

Root Cause Analysis

Problem Being Solved: BSD sed (macOS default) doesn't support GNU sed extensions \s (shorthand for whitespace) and s///;t;s/// flow-control branching. This caused the guard-git.sh hook to silently fail when extracting directory paths from commands like cd /path/to/wt && git push, leading to false "Branch does not match required pattern" denials when pushing from worktrees.

Why This Approach: The solution correctly addresses the compatibility issue by:

  1. Replacing \s with POSIX-compliant [[:space:]] character class
  2. Splitting the chained s/quoted/.../p;t;s/unquoted/.../p into two separate sed invocations to avoid BSD sed's label interpretation issue

Risk Assessment: Very low risk - this is a pure compatibility fix that makes the existing logic work correctly on both GNU and BSD sed without changing behavior.

Backlog Compliance

  • Zero-dep: ✓ — No new dependencies, uses existing shell/sed/git tooling
  • Foundation-aligned: ✓ — Infrastructure fix that improves reliability of existing safety hooks
  • Problem-fit: 3 — Useful compatibility fix but doesn't directly address core "AI agents navigate codebases" problem
  • Breaking: No — Pure compatibility fix with identical behavior on both sed variants
  • Tier: 1

Code Quality Assessment

Strengths:

  • Excellent problem identification: Author clearly reproduced both failure modes (sed: undefined label and silent empty extraction)
  • Comprehensive testing: 7 test cases covering all code paths (cd && push, -C variants, quoted paths, multi-C precedence)
  • Proper POSIX compliance: [[:space:]] is the correct portable alternative to \s
  • Well-documented changes: Clear commit message explaining the GNU vs BSD differences
  • Surgical fix: Changes only the problematic patterns without affecting logic

Technical Review:

  • Lines 36-37: ✅ Correctly replaces \s with [[:space:]] in normalization patterns
  • Lines 122-126: ✅ Smart workaround for BSD sed's t label issue - two separate calls preserve the same logic flow
  • Lines 128-129: ✅ Consistent application of [[:space:]] in cd detection

Security: ✅ No security implications - maintains same input validation and escaping

Minor Suggestions

  1. Performance note: The approach of two separate sed calls is correct for BSD compatibility. The alternative (single regex handling both cases) would be more complex and error-prone.

  2. Pattern consistency: All patterns are now consistently using [[:space:]] - good for maintainability.

Root Cause Analysis

Problem Being Solved: Legitimate BSD sed compatibility issue blocking macOS users
Why This Approach: Correct POSIX-compliant solution that preserves existing behavior
Risk Assessment: Very low - pure compatibility fix with no behavioral changes

Backlog Compliance

  • Zero-dep: ✓ — No new dependencies
  • Foundation-aligned: ✓ — Infrastructure reliability improvement
  • Problem-fit: 3 — Useful but doesn't address core AI navigation problem
  • Breaking: No — Identical behavior across platforms
  • Tier: 1

Final Recommendation

  • Rating: ⭐⭐⭐⭐☆ (4/5)
  • Action: APPROVE
  • Reasoning: High-quality compatibility fix with excellent testing, clear problem analysis, and proper POSIX-compliant solution. The author demonstrated thorough understanding by reproducing both failure modes and testing all edge cases. This resolves a real blocker for macOS users without introducing any risk.

Minor deduction: While excellently executed, this is infrastructure maintenance rather than feature development, hence 4/5 rather than 5/5.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 17, 2026

Greptile Summary

This PR fixes BSD sed compatibility issues in .claude/hooks/guard-git.sh so the pre-push/pre-commit hook works correctly on macOS without false branch-pattern denials.

  • Replaces \\s with [[:space:]] in sed -E patterns on lines 36–37 and 129, since BSD sed does not recognize the \\s shorthand in ERE mode and silently returns empty output.
  • Splits the single sed -nE 's/.../p;t;s/.../p' chain in detect_work_dir into two sequential invocations; BSD sed interprets the empty string after t as a label reference rather than a branch-to-end, causing an "undefined label" error.

Confidence Score: 5/5

Safe to merge — the changes are narrowly scoped POSIX compatibility fixes with no logic alterations.

Every modified line is a direct replacement of a BSD-incompatible construct (\s or ;t; flow control in sed) with a well-known POSIX equivalent. The surrounding logic — greedy .*-C anchor, two-pass NCOMMAND normalization, trailing-whitespace trim — is untouched. The unmodified \s occurrences in grep -qE patterns are intentional: macOS grep (TRE engine) supports \s in ERE mode; only sed does not.

No files require special attention.

Important Files Changed

Filename Overview
.claude/hooks/guard-git.sh Targeted BSD sed compatibility fixes: \s[[:space:]] in all sed patterns; s///;t;s/// flow-control split into two separate sed invocations. Logic unchanged; grep patterns that use \s are left as-is since macOS grep (TRE) supports \s in ERE mode.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[git/gh command received] --> B{grep: contains git/gh?}
    B -- No --> Z[exit 0 allow]
    B -- Yes --> C[NCOMMAND: strip -C paths\nsed -E two-pass BSD-safe]
    C --> D{Dangerous command?\nadd/reset/checkout/clean/stash}
    D -- Yes --> DENY[deny with reason]
    D -- No --> E{git push or gh pr create?}
    E -- Yes --> F[detect_work_dir\ntarget=push]
    F --> G{grep: -C flag present?\ngit+space+-C BSD-safe}
    G -- Yes --> H[sed: extract quoted path\nfirst invocation]
    H --> I{work_dir empty?}
    I -- No --> K[use quoted path]
    I -- Yes --> J[sed: extract unquoted path\nsecond invocation]
    J --> K
    G -- No --> L{grep: cd prefix?\nBSD-safe pattern}
    L -- Yes --> M[sed: extract cd path\nBSD-safe pattern]
    M --> K
    K --> N[validate_branch_name\ngit rev-parse HEAD]
    N --> O{branch matches pattern?}
    O -- Yes --> Z
    O -- No --> DENY
    E -- No --> P{git commit?}
    P -- Yes --> Q[detect_work_dir commit\ncheck edit log]
    Q --> R{unexpected staged files?}
    R -- Yes --> DENY
    R -- No --> Z
Loading

Reviews (4): Last reviewed commit: "Merge branch 'main' into fix/1145-guard-..." | Re-trigger Greptile

@carlos-alm carlos-alm merged commit 1ff4b54 into main May 18, 2026
21 checks passed
@carlos-alm carlos-alm deleted the fix/1145-guard-git-bsd-sed branch May 18, 2026 03:35
@github-actions github-actions Bot locked and limited conversation to collaborators May 18, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

follow-up: guard-git.sh \s regex fails on macOS BSD sed

1 participant