Skip to content

refactor(core): model executor spawn failures explicitly - #24

Merged
errfld merged 2 commits into
mainfrom
git-smee-7ud/executor-error-modeling
Feb 4, 2026
Merged

refactor(core): model executor spawn failures explicitly#24
errfld merged 2 commits into
mainfrom
git-smee-7ud/executor-error-modeling

Conversation

@errfld

@errfld errfld commented Feb 4, 2026

Copy link
Copy Markdown
Owner

What changed

  • replaced the ambiguous NonSuccessfulExitStatus error with CommandSpawnFailed { command, source }
  • added command redaction for spawn failures (<args redacted>) to improve diagnostics without leaking sensitive args
  • refactored executor internals to use an internal command runner abstraction so spawn and exit failure paths are explicitly modeled
  • kept non-zero exit and signal-termination failures distinct (ExecutionFailed, ExecutionTerminatedBySignal)
  • replaced flaky shell/timing-based executor tests with deterministic fake-runner tests that cover:
    • spawn failures
    • non-zero exits
    • signal termination
    • sequential-before-parallel behavior
    • failure propagation that skips parallel hooks after sequential failure

Validation

  • cargo test --workspace

Closes git-smee-7ud.

Summary by CodeRabbit

  • Refactor

    • Reworked hook/command execution into a modular, pluggable runner approach to improve extensibility and testability.
  • Bug Fixes

    • Improved error reporting for command spawn failures, including redaction of sensitive tokens and clearer underlying error propagation.
    • Preserved existing exit-code semantics while hardening edge-case handling.
  • Tests

    • Expanded coverage for execution ordering, spawn errors, redaction, and mixed parallel/sequential hooks.

@coderabbitai

coderabbitai Bot commented Feb 4, 2026

Copy link
Copy Markdown
📝 Walkthrough

Walkthrough

Introduced a pluggable CommandRunner abstraction and PlatformCommandRunner adapter, refactored command execution to use runners, added command redaction and a CommandSpawnFailed error variant, and updated tests to exercise runner-based execution and redaction behavior.

Changes

Cohort / File(s) Summary
Executor core
crates/git-smee-core/src/executor.rs
Added CommandRunner trait and PlatformCommandRunner implementation; refactored execution flows to execute_hook_with_runner, run_hooks_with_runner, and generic execute_command using a runner; implemented redact_command and is_inline_env_assignment; replaced previous error variants with CommandSpawnFailed { command, source }.
Tests / Fake runner
crates/git-smee-core/src/executor.rs (tests section)
Added FakeRunner for deterministic testing; new tests for command redaction, spawn errors, empty commands, and parallel vs sequential hook execution ordering.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐰🥕 I hopped through runners, light and fleet,
Commands now masked so secrets sleep sweet.
Platform or fake, I bounce on the cue,
Hooks run in order — then hop — and whoo! 🎩

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 47.62% 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 title 'refactor(core): model executor spawn failures explicitly' directly and clearly summarizes the main change: replacing ambiguous error handling with explicit spawn failure modeling through the CommandSpawnFailed variant.

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

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch git-smee-7ud/executor-error-modeling

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 `@crates/git-smee-core/src/executor.rs`:
- Around line 114-125: The redact_command function currently treats the first
whitespace token as the executable and can leak inline env assignments like
KEY=VALUE; update redact_command to skip any leading tokens that match an
ENV_ASSIGN pattern (tokens containing '=' before any whitespace and not starting
with '-' or containing a path separator) and consider the first non-ENV_ASSIGN
token as the executable (or use "<redacted>" if none remain) before applying the
existing truncation and "<args redacted>" logic; reference the redact_command
function and the local variables tokens, executable, and redacted when making
the change.

Comment thread crates/git-smee-core/src/executor.rs
@errfld

errfld commented Feb 4, 2026

Copy link
Copy Markdown
Owner Author

Addressed the CodeRabbit comment about potential secret leakage in redaction.

  • Updated redact_command to skip leading inline env-assignment tokens (KEY=VALUE) when selecting the executable token.
  • Falls back to <redacted> if no executable remains.
  • Added regression test: given_spawn_error_with_env_prefix_when_executing_then_redaction_hides_env_assignments.

This is now in commit 828a6cd.

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