fix(hooks): use portable sed for guard-git -C work_dir detection#1152
fix(hooks): use portable sed for guard-git -C work_dir detection#1152carlos-alm wants to merge 3 commits into
Conversation
The `;t;` chained sed expression in `detect_work_dir` is a GNU-ism: BSD sed (macOS default) parses it as `t` branching to a label named `;s/...` and errors out, leaving `work_dir` empty. The hook then falls back to the ambient cwd's branch and denies legitimate `git -C /other-worktree push` calls from agents running in worktrees with non-conforming branch names. Split the one-liner into two sed invocations (quoted form first, then unquoted as a fallback). Behavior is identical on GNU sed and now works on BSD sed. Closes #1135
|
Claude finished @carlos-alm's task —— View job PR Review CompleteTodo List:
Root Cause AnalysisProblem Being Solved: The Why This Approach: The solution correctly identifies that the issue is a sed dialect incompatibility, not a logical error. Splitting the complex one-liner into two separate sed invocations (quoted path pattern first, then unquoted fallback) maintains identical behavior on GNU sed while making it work on BSD sed. This preserves the exact same logic flow without changing the hook's security model. Risk Assessment: ✅ Low Risk — This is a pure compatibility fix that doesn't change the security logic, just makes it work cross-platform. Backlog Compliance
Critical ConcernsNone. This is a well-executed compatibility fix. Code Quality AssessmentStrengths:
Technical Analysis: work_dir=$(echo "$search_str" | sed -nE 's/.*-C[[:space:]]+"([^"]+)".*/\1/p;t;s/.*-C[[:space:]]+([^[:space:]]+).*/\1/p')Was correctly split into: work_dir=$(echo "$search_str" | sed -nE 's/.*-C[[:space:]]+"([^"]+)".*/\1/p')
if [ -z "$work_dir" ]; then
work_dir=$(echo "$search_str" | sed -nE 's/.*-C[[:space:]]+([^[:space:]]+).*/\1/p')
fiThis preserves the original semantics: try the quoted pattern first, if no match, try the unquoted pattern. Security Analysis:
Performance: Negligible impact — splits one sed call into potentially two, but only in the Test CoverageThe PR description mentions manual testing on macOS:
Manual testing approach is appropriate for a hook compatibility fix. Final Recommendation
Recommendation: Merge this PR. It's exactly the kind of fix needed to ensure the tooling works consistently across development environments. |
Greptile SummaryThis PR fixes a BSD sed (macOS) incompatibility in
Confidence Score: 5/5Safe to merge — the change is a targeted portability fix with no logic changes on GNU sed and correct behaviour on BSD sed. The two-invocation approach is functionally identical to the original on GNU sed (quoted pattern succeeds → skip unquoted, same as before), eliminates the BSD sed parse error that broke macOS, and doesn't touch any other logic paths in the hook. The test plan covers the common cases including multi- No files require special attention. Important Files Changed
Reviews (3): Last reviewed commit: "Merge branch 'main' into fix/1135-guard-..." | Re-trigger Greptile |
|
Superseded by #1146 (merged), which fixed both the Closing this PR — merging it now would actually regress main by replacing the already-portable Will also close #1135 since it shares the root cause already fixed in #1146. |
|
Superseded by #1146. |
Summary
detect_work_dir()in.claude/hooks/guard-git.shused a;t;chained sed expression that GNU sed accepts but BSD sed (macOS default) rejects withundefined label.work_dirstayed empty and the hook fell back to the ambient cwd's branch — sogit -C /other-worktree pushfrom an agent in a worktree likeworktree-agent-XXXXwas always denied even when the targeted worktree had a valid branch.Test plan
printf '{"tool_input":{"command":"git -C /tmp push"}}' | bash .claude/hooks/guard-git.sh— sed error gone.git -C /real-worktree pushagainst a worktree on a valid branch — no deny.-C(last wins):git -C /a -C /b pushresolves to/b.-C "/path with spaces"— quoted regex still matches.-C, ambient invalid branch — still denied (unchanged behavior).Closes #1135