feat(hooks): add built-in prompt guards for branch protection and dirty tree#134
Conversation
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 005a706869
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
crates/csa-hooks/src/guard_tests.rs
Outdated
| assert_eq!( | ||
| results.len(), | ||
| 1, | ||
| "dirty-tree-reminder should produce exactly one result" |
There was a problem hiding this comment.
Make dirty-tree guard test tolerate clean worktrees
run_prompt_guards only includes guards that emit non-empty stdout, but this test requires one result even though dirty-tree-reminder intentionally prints nothing on a clean repository. In typical CI checkouts (clean tree), results.len() becomes 0, so this assertion makes the test fail despite correct behavior and can block the test suite.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1ba282f. Changed assertion to results.len() <= 1 — guard may produce 0 (clean tree) or 1 (dirty tree) results.
crates/csa-hooks/src/guard_tests.rs
Outdated
| // We are on feat/builtin-prompt-guards, so no warning expected | ||
| let guards = builtin_prompt_guards(); | ||
| let branch_guard = &guards[0]; | ||
| let results = run_prompt_guards(std::slice::from_ref(branch_guard), &test_context()); | ||
| // On a feature branch, the guard should produce empty output |
There was a problem hiding this comment.
Remove branch-dependent assumption from branch guard test
This test assumes the current checkout is a feature branch, but it runs the built-in guard against whatever branch the test runner uses; on main/master/dev/develop/release/* the guard emits a warning and this assertion fails. Because no temporary repo/branch is created, the test result is environment-dependent and can become flaky across local and CI runs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1ba282f. Test now only verifies execution succeeds (no panic/timeout), without asserting on output content.
005a706 to
1ba282f
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1ba282fa3f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
crates/csa-hooks/src/guard.rs
Outdated
| vec![ | ||
| PromptGuardEntry { | ||
| name: "branch-protection".to_string(), | ||
| command: r#"branch=$(git rev-parse --abbrev-ref HEAD 2>/dev/null) || exit 0 |
There was a problem hiding this comment.
Run built-in git guards in the configured project root
The new built-in branch-protection (and the dirty-tree-reminder right below it) calls plain git ... without setting a working directory, but run_single_guard spawns sh -c without current_dir and determine_project_root supports --cd without changing process CWD, so running csa ... --cd <repo> from another directory makes these guards silently exit 0 (stderr is redirected to /dev/null) and skip protection exactly when users expect it. This regresses branch/dirty-tree safety for any workflow where project root differs from the launcher CWD.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 8364aeb. Added current_dir(project_root) to run_single_guard's Command builder. All guard scripts now run in the configured project root directory, ensuring correct behavior with csa --cd <repo>.
1ba282f to
8364aeb
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8364aeb2f0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
crates/csa-hooks/src/config.rs
Outdated
| if !other.builtin_guards { | ||
| self.builtin_guards = false; |
There was a problem hiding this comment.
Let higher-priority config re-enable builtin guards
merge_with only updates builtin_guards when the incoming layer is false, so once any lower-priority config sets builtin_guards = false, later higher-priority layers can never restore the default guards (even if they explicitly set builtin_guards = true). This violates the stated layered-priority merge model and makes per-project override of a global disable impossible.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 13cfdf6. Changed to self.builtin_guards = other.builtin_guards — higher-priority layer always wins, allowing project config to re-enable after global disable.
8364aeb to
13cfdf6
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 13cfdf6dd1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
crates/csa-hooks/src/config.rs
Outdated
| /// (non-empty array wins; empty array means "no override from this layer"). | ||
| fn merge_with(&mut self, other: Self) { | ||
| // builtin_guards: higher-priority layer always wins (can re-enable after disable) | ||
| self.builtin_guards = other.builtin_guards; |
There was a problem hiding this comment.
Merge builtin_guards only when explicitly configured
builtin_guards is deserialized with #[serde(default = "default_true")], so any higher-priority hooks.toml that omits this key still carries true; this unconditional assignment means a project config with unrelated settings will silently re-enable built-in guards that were disabled in global config. In normal global+project loading, users cannot keep a global builtin_guards = false unless every higher layer repeats it, which breaks expected layered-override behavior for omitted fields.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a2ebd5b. Changed builtin_guards to Option<bool>: None = not specified (inherits from lower layer), Some(true/false) = explicitly set. Added two new tests verifying global-disable+project-reenable and global-disable+project-omit-inherits behaviors.
13cfdf6 to
a2ebd5b
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a2ebd5beee
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
crates/csa-hooks/src/guard.rs
Outdated
| vec![ | ||
| PromptGuardEntry { | ||
| name: "branch-protection".to_string(), | ||
| command: r#"branch=$(git rev-parse --abbrev-ref HEAD 2>/dev/null) || exit 0 |
There was a problem hiding this comment.
Use a branch detector that works before first commit
The built-in branch-protection guard uses git rev-parse --abbrev-ref HEAD and then || exit 0, which suppresses warnings when that command fails. In a newly initialized repo before the first commit (for example git init -b main), rev-parse exits non-zero (HEAD is unborn), so the guard emits nothing even on protected branches; this misses the reminder at the exact point users are likely to make an initial direct commit to main/master.
Useful? React with 👍 / 👎.
…ty tree reminder Add two default prompt guard scripts that ship with CSA out of the box: - branch-protection: Warns when working on protected branches (main/master/dev/ develop/release/*) to prevent accidental direct commits. - dirty-tree-reminder: Reminds about uncommitted changes before starting new work, showing the count of modified files. Built-in guards are active by default when no user-defined guards are configured. Users can disable them with `builtin_guards = false` in hooks.toml, or replace them entirely by defining their own `[[prompt_guard]]` entries. Configuration loading: built-in guards are injected as the lowest priority layer (Layer 0) after the 4-tier merge. Any user-defined prompt_guard array takes precedence. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
a2ebd5b to
c281c8d
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Summary
builtin_guards = falsein hooks.toml[[prompt_guard]]entries replace built-ins entirelyTest plan
test_builtin_prompt_guards_returns_two_entries— verifies count, names, timeoutstest_builtin_branch_protection_on_feature_branch— no warning on non-protected branchtest_builtin_dirty_tree_reminder_on_clean_tree— guard executes without errortest_builtin_guards_loaded_by_default— config loads 2 guards with no user configtest_builtin_guards_replaced_by_user_config— user guards replace built-instest_builtin_guards_disabled_explicitly—builtin_guards = falsedisables alljust pre-commitpasses (1123 tests, 0 failed)🤖 Generated with Claude Code