Skip to content

fix(rules): resolve unmatched sub-commands to concrete actions in compound evaluation #178

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
fohte merged 4 commits into from
Mar 10, 2026
Merged

fix(rules): resolve unmatched sub-commands to concrete actions in compound evaluation #178

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3a99554
rules: fix command substitution bypass in compound evaluation
fohte Mar 9, 2026
dfcc36a
rules: improve deny message for default policy denials
fohte Mar 9, 2026
b4295a0
rules: simplify wrapper candidate selection with tuple comparison
fohte Mar 9, 2026
1e06a9a
docs: update rule evaluation docs after Action::Default removal
fohte Mar 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion src/adapter/check_adapter.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,6 @@ fn build_check_output(result: &ActionResult) -> CheckOutput {
deny.fix_suggestion.clone(),
),
Action::Ask(message) => ("ask".to_string(), message.clone(), None),
Action::Default => ("ask".to_string(), None, None),
};

let sandbox = build_sandbox_info(&result.sandbox);
Expand Down
32 changes: 5 additions & 27 deletions src/adapter/exec_adapter.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,11 @@ impl Endpoint for ExecAdapter {
Ok(exit_code)
}
Action::Deny(deny_response) => {
eprintln!("runok: denied: {}", deny_response.matched_rule);
if deny_response.matched_rule.is_empty() {
eprintln!("runok: command denied by default policy");
} else {
eprintln!("runok: denied: {}", deny_response.matched_rule);
}
if let Some(ref message) = deny_response.message {
eprintln!(" reason: {}", message);
}
Expand All @@ -127,10 +131,6 @@ impl Endpoint for ExecAdapter {
eprintln!("runok: {}", msg);
Ok(3)
}
Action::Default => {
// Should not reach here; run() handles Default before calling handle_action
Ok(0)
}
}
}

Expand Down Expand Up @@ -182,9 +182,6 @@ impl Endpoint for ExecAdapter {
.unwrap_or("command would require confirmation");
eprintln!("runok: dry-run: {}", msg);
}
Action::Default => {
eprintln!("runok: dry-run: no matching rule (default behavior)");
}
}
Ok(0)
}
Expand Down Expand Up @@ -435,24 +432,6 @@ mod tests {
assert_eq!(result, 3);
}

// --- handle_action: Default ---

#[rstest]
fn handle_action_default_returns_exit_0() {
let adapter = ExecAdapter::new(
vec!["git".into(), "status".into()],
None,
Box::new(MockExecutor::new(0)),
);
let result = adapter
.handle_action(ActionResult {
action: Action::Default,
sandbox: SandboxInfo::Preset(None),
})
.unwrap();
assert_eq!(result, 0);
}

// --- handle_no_match ---

#[rstest]
Expand Down Expand Up @@ -600,7 +579,6 @@ mod tests {
Action::Ask(Some("please confirm".to_string())),
0
)]
#[case::default_action(Action::Default, 0)]
fn handle_dry_run_always_returns_exit_0(
#[case] action: Action,
#[case] expected_exit_code: i32,
Expand Down
10 changes: 5 additions & 5 deletions src/adapter/hook_adapter.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,11 @@ pub struct ClaudeCodeHookAdapter {
/// Build a combined reason string from a `DenyResponse`, including
/// the matched rule, optional message, and optional fix suggestion.
fn build_deny_reason(deny: &DenyResponse) -> String {
let mut reason = format!("denied: {}", deny.matched_rule);
let mut reason = if deny.matched_rule.is_empty() {
"command denied by default policy".to_string()
} else {
format!("denied: {}", deny.matched_rule)
};
if let Some(ref message) = deny.message {
reason.push_str(&format!(" ({})", message));
}
Expand Down Expand Up @@ -100,10 +104,6 @@ impl ClaudeCodeHookAdapter {
let updated = Self::sandbox_updated_input(&result.sandbox, &bash_input.command)?;
("ask", message.clone(), updated)
}
Action::Default => {
// run() dispatches Default to handle_no_match, but handle safely.
("allow", None, None)
}
};

Ok(Self::build_output(decision, reason, updated_input))
Expand Down
139 changes: 47 additions & 92 deletions src/adapter/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ pub mod hook_adapter;
use crate::config::{Config, Defaults, MergedSandboxPolicy};
use crate::rules::command_parser::extract_commands;
use crate::rules::rule_engine::{
Action, EvalContext, RuleMatchInfo, evaluate_command, evaluate_compound,
Action, EvalContext, RuleMatchInfo, default_action, evaluate_command, evaluate_compound,
};

/// Unified evaluation result for the adapter layer.
Expand Down Expand Up @@ -36,19 +36,6 @@ pub struct RunOptions {
pub verbose: bool,
}

/// Describes what should happen after command evaluation.
/// Separates the "what happened" decision from "how to handle it",
/// ensuring dry-run is checked at exactly one point.
enum Dispatch {
/// A rule matched and produced an action (Allow/Deny/Ask).
Matched(ActionResult),
/// No rule matched; fall back to defaults.
NoMatch {
action_result: ActionResult,
defaults: Defaults,
},
}

/// Abstracts protocol-specific input/output differences across
/// exec, check, and Claude Code hook endpoints.
pub trait Endpoint {
Expand Down Expand Up @@ -99,25 +86,6 @@ fn log_matched_rules(matched_rules: &[RuleMatchInfo]) {
}
}

/// Resolve an `Action::Default` against the user's configured defaults,
/// producing a concrete action (Allow/Ask/Deny) for dry-run reporting.
/// When no default is configured (`None`), leaves the action as `Default`
/// so each adapter's `handle_dry_run` can apply its own interpretation.
fn resolve_no_match(mut action_result: ActionResult, defaults: &Defaults) -> ActionResult {
use crate::config::ActionKind;
action_result.action = match defaults.action {
Some(ActionKind::Allow) => Action::Allow,
Some(ActionKind::Deny) => Action::Deny(crate::rules::rule_engine::DenyResponse {
message: None,
fix_suggestion: None,
matched_rule: String::new(),
}),
Some(ActionKind::Ask) => Action::Ask(None),
None => return action_result, // keep Action::Default
};
action_result
}

/// Apply `defaults.sandbox` as a fallback when the rule evaluation produced
/// no sandbox information. This ensures that `defaults.sandbox` acts as a
/// true default: it is used whenever the matched rule does not specify its
Expand Down Expand Up @@ -199,6 +167,13 @@ pub fn run_with_options(endpoint: &dyn Endpoint, config: &Config, options: &RunO
match evaluate_compound(config, &command, &context) {
Ok(compound_result) => {
if options.verbose {
for detail in &compound_result.sub_command_details {
log_matched_rules(&detail.matched_rules);
eprintln!(
"[verbose] sub-command {:?}: {:?}",
detail.command, detail.action
);
}
eprintln!(
"[verbose] Compound evaluation result: {:?}",
compound_result.action
Expand All @@ -214,7 +189,7 @@ pub fn run_with_options(endpoint: &dyn Endpoint, config: &Config, options: &RunO
} else if commands.is_empty() {
// No executable commands (e.g. comment-only input) — use default action
ActionResult {
action: Action::Default,
action: default_action(config),
sandbox: SandboxInfo::Preset(None),
}
} else {
Expand All @@ -239,45 +214,18 @@ pub fn run_with_options(endpoint: &dyn Endpoint, config: &Config, options: &RunO
// Apply defaults.sandbox fallback when the matched rule has no sandbox
let action_result = apply_sandbox_fallback(action_result, &defaults);

// Determine dispatch target
let dispatch = if matches!(action_result.action, Action::Default) {
if options.verbose {
eprintln!("[verbose] No matching rule, using default behavior");
}
Dispatch::NoMatch {
action_result,
defaults,
}
} else {
Dispatch::Matched(action_result)
};

// Single dry-run checkpoint: all dispatch paths are covered here
if options.dry_run {
if options.verbose {
eprintln!("[verbose] Dry-run mode: skipping execution");
}
let action_result = match dispatch {
Dispatch::Matched(ar) => ar,
Dispatch::NoMatch {
action_result,
defaults,
} => resolve_no_match(action_result, &defaults),
};
return endpoint
.handle_dry_run(action_result)
.unwrap_or_else(|e| endpoint.handle_error(e));
}

// Normal execution
match dispatch {
Dispatch::Matched(action_result) => endpoint
.handle_action(action_result)
.unwrap_or_else(|e| endpoint.handle_error(e)),
Dispatch::NoMatch { defaults, .. } => endpoint
.handle_no_match(&defaults)
.unwrap_or_else(|e| endpoint.handle_error(e)),
}
endpoint
.handle_action(action_result)
.unwrap_or_else(|e| endpoint.handle_error(e))
}

#[cfg(test)]
Expand Down Expand Up @@ -520,26 +468,31 @@ mod tests {
);
}

// --- no matching rule -> handle_no_match ---
// --- no matching rule -> handle_action with default action ---

#[rstest]
#[case::no_matching_rule("unknown-command", make_config(vec![allow_rule("git status")]))]
#[case::empty_config("git status", Config::default())]
fn no_match_calls_handle_no_match(#[case] command: &str, #[case] config: Config) {
fn no_match_calls_handle_action_with_ask(#[case] command: &str, #[case] config: Config) {
// With no defaults.action configured, unmatched commands resolve
// to Ask (the safe fallback).
let endpoint = MockEndpoint::new(Ok(Some(command.to_string())));
let exit_code = run(&endpoint, &config);

assert!(*endpoint.called_handle_no_match.borrow());
assert!(!*endpoint.called_handle_action.borrow());
assert!(*endpoint.called_handle_action.borrow());
assert!(!*endpoint.called_handle_no_match.borrow());
assert!(matches!(
*endpoint.last_action.borrow(),
Some(Action::Ask(None))
));
assert_eq!(exit_code, 0);
}

// --- defaults are passed through when no rules match ---
// --- defaults.action = allow resolves to Allow ---

#[rstest]
fn no_match_uses_config_defaults() {
let endpoint =
MockEndpoint::new(Ok(Some("unknown-command".to_string()))).with_no_match_exit_code(5);
fn no_match_with_defaults_allow_calls_handle_action_with_allow() {
let endpoint = MockEndpoint::new(Ok(Some("unknown-command".to_string())));
let config = Config {
defaults: Some(Defaults {
action: Some(ActionKind::Allow),
Expand All @@ -550,12 +503,13 @@ mod tests {
};
let exit_code = run(&endpoint, &config);

assert!(*endpoint.called_handle_no_match.borrow());
assert_eq!(exit_code, 5);
assert_eq!(
*endpoint.last_defaults_action.borrow(),
Some(Some(ActionKind::Allow))
);
assert!(*endpoint.called_handle_action.borrow());
assert!(!*endpoint.called_handle_no_match.borrow());
assert!(matches!(
*endpoint.last_action.borrow(),
Some(Action::Allow)
));
assert_eq!(exit_code, 0);
}

// --- compound command: deny wins over allow ---
Expand All @@ -580,16 +534,20 @@ mod tests {
));
}

// --- compound command: all default -> handle_no_match ---
// --- compound command: all unmatched -> handle_action with Ask ---

#[rstest]
fn compound_all_default_calls_handle_no_match() {
fn compound_all_unmatched_calls_handle_action_with_ask() {
let endpoint = MockEndpoint::new(Ok(Some("unknown-cmd1 && unknown-cmd2".to_string())));
let config = make_config(vec![allow_rule("git status")]);
let exit_code = run(&endpoint, &config);

assert!(*endpoint.called_handle_no_match.borrow());
assert!(!*endpoint.called_handle_action.borrow());
assert!(*endpoint.called_handle_action.borrow());
assert!(!*endpoint.called_handle_no_match.borrow());
assert!(matches!(
*endpoint.last_action.borrow(),
Some(Action::Ask(None))
));
assert_eq!(exit_code, 0);
}

Expand Down Expand Up @@ -676,12 +634,9 @@ mod tests {
// --- handle_no_match error falls back to handle_error ---

#[rstest]
#[case::extract_none(Ok(None))]
#[case::no_matching_rule(Ok(Some("unknown-command".to_string())))]
fn handle_no_match_error_delegates_to_handle_error(
#[case] command: Result<Option<String>, String>,
) {
let endpoint = MockEndpoint::new(command)
fn handle_no_match_error_delegates_to_handle_error() {
// handle_no_match is only called when extract_command returns None
let endpoint = MockEndpoint::new(Ok(None))
.with_no_match_failure()
.with_error_exit_code(2);
let config = Config::default();
Expand Down Expand Up @@ -772,11 +727,11 @@ mod tests {
assert!(!*endpoint.called_handle_no_match.borrow());
assert_eq!(exit_code, 0);

// With no configured defaults, Action::Default is preserved
// With no configured defaults, defaults fall back to Ask
let last = endpoint.last_action.borrow();
assert!(
matches!(last.as_ref(), Some(Action::Default)),
"expected Action::Default, got {last:?}"
matches!(last.as_ref(), Some(Action::Ask(None))),
"expected Action::Ask(None), got {last:?}"
);
}

Expand Down Expand Up @@ -917,8 +872,8 @@ mod tests {
#[case::for_loop_no_matching_rule(
"for f in *.yaml; do echo $f; done",
allow_rule("git status"),
true, // defaults to Ask -> handle_action
false,
true
)]
fn single_extracted_subcommand_evaluates_simplified_form(
#[case] command: &str,
Expand Down
Loading
Loading