Skip to content

fix: remove Unix bash dependency in hook executor - #30

Merged
errfld merged 2 commits into
mainfrom
codex/git-smee-5tx/portable-shell-executor
Feb 6, 2026
Merged

fix: remove Unix bash dependency in hook executor#30
errfld merged 2 commits into
mainfrom
codex/git-smee-5tx/portable-shell-executor

Conversation

@errfld

@errfld errfld commented Feb 6, 2026

Copy link
Copy Markdown
Owner

Summary

  • switch Unix command execution from bash to portable sh -c
  • include shell details in spawn failure errors to improve diagnostics
  • add regression tests for shell command construction and spawn error reporting
  • document the Unix portability behavior in README

Testing

  • cargo test --workspace

Summary by CodeRabbit

  • Improvements

    • Use POSIX sh for hook execution on Unix-like systems for better portability.
    • Error messages now include the shell used when command execution fails.
  • Documentation

    • Clarified that hooks on Unix-like systems run via sh -c (not Bash-specific).
  • Chores

    • Added new issue tracker entries and closed/annotated an existing issue with verification notes.

@coderabbitai

coderabbitai Bot commented Feb 6, 2026

Copy link
Copy Markdown
📝 Walkthrough

Walkthrough

Switches Unix hook execution from Bash to POSIX sh, adds shell-identifying information to command spawn errors and the CommandRunner trait, updates platform helpers to expose a human-readable shell string, and records related metadata and README documentation changes. Tests updated accordingly.

Changes

Cohort / File(s) Summary
Metadata & Docs
\.beads/issues.jsonl, README.md
Added two new issue entries and updated an issue's status/notes; clarified in README that hooks run via POSIX sh -c on Unix-like systems.
Executor: error & trait
crates/git-smee-core/src/executor.rs
Error::CommandSpawnFailed now includes a shell: String; CommandRunner gained shell_display(); error construction captures the runner's shell; tests and FakeRunner adjusted to supply/verify shell display.
Platform: shell change & exposure
crates/git-smee-core/src/platform.rs
Unix command creation switched from bash to sh; added Platform::shell_display() returning platform-specific strings ("cmd.exe /C" or "sh -c"); tests updated to assert command args and shell display.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Poem

🐰 Hops and thumps for a smaller bash,
Now scripts run under POSIX sh splash.
Errors will whisper which shell took the blame,
Tests nod politely and note the new name.
A carrot of clarity, snug in the frame. 🥕

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title accurately summarizes the main change: switching Unix hook execution from bash to portable sh, which is the primary code change across platform.rs and executor.rs.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch codex/git-smee-5tx/portable-shell-executor

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai 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.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In @.beads/issues.jsonl:
- Line 3: The issue record with id "git-smee-5tx" has conflicting metadata:
status is "closed" but the notes say the fix still needs verification; update
the JSONL entry so it is consistent by either setting "status" to "open" (or
"reopened") and adding a clear action item in "notes" to re-verify
platform.rs/spawn usage, or keep "status":"closed" and edit the "notes" and
"close_reason" to confirm the fix landed and the remaining comment removed;
ensure you update the "updated_at" timestamp and, if reopening, add a short
"owner" assignment or comment describing the verification task.

Comment thread .beads/issues.jsonl Outdated

@coderabbitai coderabbitai 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.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In @.beads/issues.jsonl:
- Line 9: The JSONL issue entries embed "Acceptance criteria:" inside the
description instead of using a dedicated acceptance_criteria field—update the
.beads/issues.jsonl entries (e.g., the entry with id "git-smee-h4v") to move the
acceptance criteria out of the description into a top-level
"acceptance_criteria" field, keep the original description text intact minus the
acceptance block, and ensure the parser/loader that consumes these files is
adjusted to read the new acceptance_criteria key (and unit tests or schema
validation updated accordingly) so parsing remains backward compatible for
entries that still embed criteria in description.

Comment thread .beads/issues.jsonl
@errfld
errfld merged commit 72eabed into main Feb 6, 2026
16 checks passed
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.

1 participant