Split hook executor boundaries - #184
Conversation
|
Warning Review limit reached
Next review available in: 46 minutes Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable usage-based reviews in Billing to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information, and refer to the rate limits docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough
ChangesExecutor submodule extraction
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/git-smee-core/src/executor.rs`:
- Around line 150-155: The extracted-module tests in executor are still reaching
into redaction, runner, scheduler, and summary internals from the parent module.
Move each related test block into the owning module files, using the existing
symbols redact_command, apply_hook_arg_env, is_hook_arg_env_key,
windows_cmd_quote_hook_arg, windows_command_script, execute_command,
CommandOutcome, and CommandRun to place them beside the implementation they
verify. Keep the behavior coverage the same, but relocate the tests into
redaction.rs, runner.rs, scheduler.rs, and summary.rs so each module owns its
own tests.
- Line 12: The executor facade currently re-exports HookRunSummary and
CommandPhase, but CommandRun is still hidden while
HookRunSummary::first_failure() exposes it in its return type. Update the public
surface in executor::executor.rs by re-exporting CommandRun from the summary
module, or change HookRunSummary::first_failure() to return a public facade type
so the method signature no longer leaks a private type.
In `@crates/git-smee-core/src/executor/runner.rs`:
- Around line 107-130: The Windows hook-arg quoting in
windows_cmd_quote_hook_arg only wraps arguments in quotes, so cmd metacharacters
like % and ! can still expand before the .cmd script receives them. Update the
Windows path in runner.rs to escape cmd metacharacters in addition to quoting,
and ensure append_windows_hook_args still passes each hook argument through the
revised helper. Add test coverage for literal %VAR% and !VAR! arguments to
verify they remain unchanged when executed through the Windows command path.
In `@crates/git-smee-core/src/executor/summary.rs`:
- Around line 65-70: `HookRunSummary::first_failure` currently exposes
`&CommandRun` even though `CommandRun` is not publicly reachable from
`git_smee_core::executor`, so downstream users cannot use the return type
cleanly. Fix this by either re-exporting `CommandRun` from the public executor
API or making `first_failure` non-public/removing it if it should remain
internal; update the `HookRunSummary` and `CommandRun` visibility boundaries in
`executor::summary` accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1dd438c7-cd9a-4395-8f9f-1b6b605e9155
📒 Files selected for processing (5)
crates/git-smee-core/src/executor.rscrates/git-smee-core/src/executor/redaction.rscrates/git-smee-core/src/executor/runner.rscrates/git-smee-core/src/executor/scheduler.rscrates/git-smee-core/src/executor/summary.rs
| use super::redaction::redact_command; | ||
| use super::runner::{ | ||
| apply_hook_arg_env, is_hook_arg_env_key, windows_cmd_quote_hook_arg, windows_command_script, | ||
| }; | ||
| use super::scheduler::execute_command; | ||
| use super::summary::{CommandOutcome, CommandRun}; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Move extracted-module tests next to the modules they exercise.
These imports pull redaction, runner, scheduler, and summary internals back into the parent test module. Move the relevant test blocks into redaction.rs, runner.rs, scheduler.rs, and summary.rs so the new boundaries own their behavior tests. As per coding guidelines, “Add tests in git-smee-core close to the implemented modules.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/git-smee-core/src/executor.rs` around lines 150 - 155, The
extracted-module tests in executor are still reaching into redaction, runner,
scheduler, and summary internals from the parent module. Move each related test
block into the owning module files, using the existing symbols redact_command,
apply_hook_arg_env, is_hook_arg_env_key, windows_cmd_quote_hook_arg,
windows_command_script, execute_command, CommandOutcome, and CommandRun to place
them beside the implementation they verify. Keep the behavior coverage the same,
but relocate the tests into redaction.rs, runner.rs, scheduler.rs, and
summary.rs so each module owns its own tests.
Source: Coding guidelines
|
Addressed review feedback in follow-up commit
Local validation after the follow-up:
@coderabbitai review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== PR head and changed files =="
gh pr view 184 --repo errfld/git-smee --json headRefOid,baseRefName,headRefName,files \
--jq '{headRefOid, baseRefName, headRefName, files: [.files[].path]}'
echo
echo "== Diffstat vs base =="
git fetch --quiet origin "$(gh pr view 184 --repo errfld/git-smee --json baseRefName --jq .baseRefName)"
BASE_REF="$(gh pr view 184 --repo errfld/git-smee --json baseRefName --jq .baseRefName)"
git diff --stat "origin/$BASE_REF...HEAD"
echo
echo "== Outline: crates/git-smee-core/src/executor.rs =="
ast-grep outline crates/git-smee-core/src/executor.rs --view expanded || true
echo
echo "== Outline: crates/git-smee-core/src/executor/runner.rs =="
ast-grep outline crates/git-smee-core/src/executor/runner.rs --view expanded || true
echo
echo "== Relevant diff hunks for executor facade and runner =="
git diff --unified=80 "origin/$BASE_REF...HEAD" -- \
crates/git-smee-core/src/executor.rs \
crates/git-smee-core/src/executor/runner.rs \
crates/git-smee-core/src/executor/summary.rs \
crates/git-smee-core/src/executor/scheduler.rs \
crates/git-smee-core/src/executor/redaction.rs
echo
echo "== Search for CommandRun re-export and first_failure signature =="
rg -n -C3 'pub use .*CommandRun|first_failure\(' crates/git-smee-core/src/executor.rs crates/git-smee-core/src/executor/summary.rs
echo
echo "== Search for Windows hook arg quoting tests and implementation =="
rg -n -C4 'windows_cmd_quote_hook_arg|windows_hook_arg|%VAR%|!VAR!|\^' crates/git-smee-core/src/executor/runner.rs crates/git-smee-core/src/executor.rsLength of output: 73757
I didn’t spot any new concerns in the touched executor modules from this follow-up. I’ll kick off a fresh review on the latest commit. If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! ✅ Action performedReview finished.
|
Dismissed as stale: review comments fixed in a87aeb6; CodeRabbit status and CI now pass.
2 participants
Summary
execute_hook_with_summary,HookRunSummary, andCommandPhase.executor/.Fixes #183
Validation
cargo test -p git-smee-core executor::tests::given_summary_success_when_rendering_then_counts_phases_and_durations -- --nocapturecargo test -p git-smee-corecargo fmt --all -- --checkcargo clippy --workspace --all-targets --all-features -- -D warningscargo test -p git-smee-cli --test cli_integrationcargo test --workspace --all-targets --all-featuresSummary by CodeRabbit
Bug Fixes
New Features