feat: address critical gaps — model routing, CI mode, per-tool permissions#27
Merged
kienbui1995 merged 1 commit intomainfrom Apr 11, 2026
Merged
feat: address critical gaps — model routing, CI mode, per-tool permissions#27kienbui1995 merged 1 commit intomainfrom
kienbui1995 merged 1 commit intomainfrom
Conversation
…sions
Tier 1:
- Subagent model routing: 'model' field in subagent tool input
allows using cheaper models (haiku) for simple subtasks
Tier 2:
- --yes/-y flag: bypass all permission prompts for CI/CD
- Per-tool permissions in config.toml:
[default]
tool_permissions = { bash = 'deny', write_file = 'prompt' }
- Compaction threshold already configurable (verified)
📝 WalkthroughWalkthroughThe PR introduces a Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mc/crates/mc-core/src/subagent.rs (1)
32-39: 🛠️ Refactor suggestion | 🟠 MajorAdd
#[must_use]onrun_task.
run_taskis a public function returning a value and should be explicitly marked#[must_use]to satisfy the project Rust rules.Proposed patch
- pub async fn run_task( + #[must_use] + pub async fn run_task(As per coding guidelines, "Use
#[must_use]on all public functions returning values in Rust".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mc/crates/mc-core/src/subagent.rs` around lines 32 - 39, Add the #[must_use] attribute to the public async function run_task so callers are warned when its Result<String, ProviderError> is ignored; place #[must_use] immediately above the pub async fn run_task(...) signature (the function that takes &mut self, provider: &dyn LlmProvider, task_prompt: &str, system_prompt: &str, tool_registry: &ToolRegistry, model_override: Option<&str> and returns Result<String, ProviderError>).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mc/crates/mc-cli/src/main.rs`:
- Around line 164-176: The current code sets policy =
mc_tools::PermissionPolicy::new(Allow) when cli.yes is true but then applies
per-tool overrides from config.tool_permissions, which can reintroduce
Prompt/Deny; change the logic so that when cli.yes is true the config overrides
are skipped (or ignored) to preserve unconditional Allow: i.e., guard the for
(tool, mode) in &config.tool_permissions loop with a check for !cli.yes (or
early continue) so with_tool_mode is only applied when cli.yes is false.
- Around line 169-174: The match over mode.as_str() ignores unknown values and
omits support for the existing PermissionMode::Auto; update the match in main.rs
to include an "auto" => mc_tools::PermissionMode::Auto arm and replace the
current default `_ => continue` with a warning log that reports the invalid
per-tool mode string (use the project's logger, e.g., warn! or eprintln!, and
include the offending `mode` and tool name/context), then skip/continue only
after logging so misconfigurations are visible to users.
In `@mc/crates/mc-core/src/subagent.rs`:
- Around line 45-50: The code currently uses
model_override.unwrap_or(&self.model) which treats Some("") as a valid model;
change the selection of effective_model so that empty or whitespace-only
overrides are ignored: evaluate model_override and use it only when it is
Some(m) and m.trim().is_empty() is false, otherwise fall back to &self.model;
update the binding named effective_model (used when calling run_simple_agent) to
use this check so only non-empty, non-whitespace overrides are forwarded.
---
Outside diff comments:
In `@mc/crates/mc-core/src/subagent.rs`:
- Around line 32-39: Add the #[must_use] attribute to the public async function
run_task so callers are warned when its Result<String, ProviderError> is
ignored; place #[must_use] immediately above the pub async fn run_task(...)
signature (the function that takes &mut self, provider: &dyn LlmProvider,
task_prompt: &str, system_prompt: &str, tool_registry: &ToolRegistry,
model_override: Option<&str> and returns Result<String, ProviderError>).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: ce9b8369-f895-4793-ada5-131e95de5cea
📒 Files selected for processing (4)
mc/crates/mc-cli/src/main.rsmc/crates/mc-config/src/types.rsmc/crates/mc-core/src/runtime.rsmc/crates/mc-core/src/subagent.rs
Comment on lines
+164
to
+176
| if cli.yes { | ||
| policy = mc_tools::PermissionPolicy::new(mc_tools::PermissionMode::Allow); | ||
| } | ||
| // Per-tool permission overrides from config | ||
| for (tool, mode) in &config.tool_permissions { | ||
| let m = match mode.as_str() { | ||
| "allow" => mc_tools::PermissionMode::Allow, | ||
| "deny" => mc_tools::PermissionMode::Deny, | ||
| "prompt" => mc_tools::PermissionMode::Prompt, | ||
| _ => continue, | ||
| }; | ||
| policy = policy.with_tool_mode(tool, m); | ||
| } |
There was a problem hiding this comment.
--yes is currently overridden by config tool modes (breaks CI mode intent).
With --yes, policy is set to Allow, but the following loop can reintroduce prompt/deny. In non-interactive runs, prompt resolves to deny, so this does not “auto-approve all tool executions.”
Proposed patch
- let mut policy = build_permission_policy(&config);
- if cli.yes {
- policy = mc_tools::PermissionPolicy::new(mc_tools::PermissionMode::Allow);
- }
- // Per-tool permission overrides from config
- for (tool, mode) in &config.tool_permissions {
+ let mut policy = if cli.yes {
+ mc_tools::PermissionPolicy::new(mc_tools::PermissionMode::Allow)
+ } else {
+ build_permission_policy(&config)
+ };
+ // Per-tool permission overrides from config (skip in --yes mode)
+ if !cli.yes {
+ for (tool, mode) in &config.tool_permissions {
let m = match mode.as_str() {
"allow" => mc_tools::PermissionMode::Allow,
"deny" => mc_tools::PermissionMode::Deny,
"prompt" => mc_tools::PermissionMode::Prompt,
_ => continue,
};
policy = policy.with_tool_mode(tool, m);
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mc/crates/mc-cli/src/main.rs` around lines 164 - 176, The current code sets
policy = mc_tools::PermissionPolicy::new(Allow) when cli.yes is true but then
applies per-tool overrides from config.tool_permissions, which can reintroduce
Prompt/Deny; change the logic so that when cli.yes is true the config overrides
are skipped (or ignored) to preserve unconditional Allow: i.e., guard the for
(tool, mode) in &config.tool_permissions loop with a check for !cli.yes (or
early continue) so with_tool_mode is only applied when cli.yes is false.
Comment on lines
+169
to
+174
| let m = match mode.as_str() { | ||
| "allow" => mc_tools::PermissionMode::Allow, | ||
| "deny" => mc_tools::PermissionMode::Deny, | ||
| "prompt" => mc_tools::PermissionMode::Prompt, | ||
| _ => continue, | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify available PermissionMode variants and current CLI mapping coverage.
rg -n -C3 'enum PermissionMode|Allow|Deny|Prompt|Auto' mc/crates/mc-tools/src/permissions.rs
rg -n -C6 'for \(tool, mode\) in &config.tool_permissions|match mode\.as_str\(\)' mc/crates/mc-cli/src/main.rsRepository: kienbui1995/mc-code
Length of output: 5836
🏁 Script executed:
head -50 mc/crates/mc-cli/src/main.rs | grep -E "use|tracing"
rg -n "tracing::" mc/crates/mc-cli/src/main.rs | head -20Repository: kienbui1995/mc-code
Length of output: 964
Support auto mode and warn on invalid per-tool mode strings.
The CLI config parser ignores unknown permission mode values silently and does not handle the auto mode, which is available in the PermissionMode enum. This prevents users from configuring tool-specific auto mode and makes misconfigurations difficult to detect.
Add support for "auto" and replace the silent-discard catch-all with a warning that logs invalid values:
Patch
let m = match mode.as_str() {
"allow" => mc_tools::PermissionMode::Allow,
"deny" => mc_tools::PermissionMode::Deny,
"prompt" => mc_tools::PermissionMode::Prompt,
- _ => continue,
+ "auto" => mc_tools::PermissionMode::Auto,
+ other => {
+ tracing::warn!(tool = %tool, mode = %other, "unknown tool permission mode; ignoring");
+ continue;
+ }
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let m = match mode.as_str() { | |
| "allow" => mc_tools::PermissionMode::Allow, | |
| "deny" => mc_tools::PermissionMode::Deny, | |
| "prompt" => mc_tools::PermissionMode::Prompt, | |
| _ => continue, | |
| }; | |
| let m = match mode.as_str() { | |
| "allow" => mc_tools::PermissionMode::Allow, | |
| "deny" => mc_tools::PermissionMode::Deny, | |
| "prompt" => mc_tools::PermissionMode::Prompt, | |
| "auto" => mc_tools::PermissionMode::Auto, | |
| other => { | |
| tracing::warn!(tool = %tool, mode = %other, "unknown tool permission mode; ignoring"); | |
| continue; | |
| } | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mc/crates/mc-cli/src/main.rs` around lines 169 - 174, The match over
mode.as_str() ignores unknown values and omits support for the existing
PermissionMode::Auto; update the match in main.rs to include an "auto" =>
mc_tools::PermissionMode::Auto arm and replace the current default `_ =>
continue` with a warning log that reports the invalid per-tool mode string (use
the project's logger, e.g., warn! or eprintln!, and include the offending `mode`
and tool name/context), then skip/continue only after logging so
misconfigurations are visible to users.
Comment on lines
+45
to
+50
| let effective_model = model_override.unwrap_or(&self.model); | ||
| tracing::debug!(task = task_prompt, model = effective_model, "spawning subagent"); | ||
|
|
||
| let result = run_simple_agent( | ||
| provider, | ||
| &self.model, | ||
| effective_model, |
There was a problem hiding this comment.
Ignore empty model overrides before dispatching.
model_override.unwrap_or(&self.model) treats Some("") as a valid model and forwards an invalid model string. Fallback should apply for empty/whitespace values.
Proposed patch
- let effective_model = model_override.unwrap_or(&self.model);
+ let effective_model = model_override
+ .map(str::trim)
+ .filter(|m| !m.is_empty())
+ .unwrap_or(&self.model);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mc/crates/mc-core/src/subagent.rs` around lines 45 - 50, The code currently
uses model_override.unwrap_or(&self.model) which treats Some("") as a valid
model; change the selection of effective_model so that empty or whitespace-only
overrides are ignored: evaluate model_override and use it only when it is
Some(m) and m.trim().is_empty() is false, otherwise fall back to &self.model;
update the binding named effective_model (used when calling run_simple_agent) to
use this check so only non-empty, non-whitespace overrides are forwarded.
This was referenced Apr 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses feedback on critical gaps:
Tier 1: Agent model routing
Subagent tool now accepts optional
modelfield:{"task": "review this code", "model": "claude-haiku-4-5"}Enables cost optimization — use haiku for simple tasks, sonnet for complex.
Tier 2: CI/CD mode
--yes/-yflag bypasses all permission prompts:magic-code --yes --json 'fix the failing test'Tier 2: Per-tool permissions
Config-driven tool-level permission overrides:
Already existed (verified)
compaction_threshold = 0.8in config/save,/load,--resume[retry]section180 tests pass.
Summary by CodeRabbit
--yes/-yCLI flag for non-interactive execution mode