fix(tools): remove invalid override fallthrough#2428
Conversation
There was a problem hiding this comment.
Hmbown has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
Code Review
This pull request updates the tool registry to remove the original tool when a config override fails to create a replacement, preventing fallback to the original tool. It also adds a unit test to verify this behavior. The review feedback suggests replacing the wildcard pattern (_) in the match block with explicit variant matching to preserve Rust's compile-time exhaustiveness checks.
| @@ -425,11 +425,24 @@ impl ToolRegistry { | |||
| _ => { | |||
There was a problem hiding this comment.
Using a wildcard pattern (_) here bypasses Rust's compile-time exhaustiveness checks. If a new variant is added to ToolOverride in the future, the compiler won't warn you that this match block needs to be updated. It is safer and more idiomatic to explicitly match the expected variants.
| _ => { | |
| crate::config::ToolOverride::Script { .. } | |
| | crate::config::ToolOverride::Command { .. } => { |
c63c9cc to
d2debc9
Compare
There was a problem hiding this comment.
Hmbown has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
Summary
Hardens the tool override path that landed in #2420: if a
ScriptorCommandoverride fails to produce a replacement tool, CodeWhale now removes the original tool instead of leaving the built-in active under a broken override.This addresses the security-shaped review concern from #2420 while keeping the change intentionally narrow. Thanks @aboimpinto for the original tool-registry work that made this path visible.
Validation
git diff --checkCARGO_TARGET_DIR=/Volumes/VIXinSSD/codewhale-target/fix-plugin-override cargo test -p codewhale-tui tools::registry --all-featuresCARGO_TARGET_DIR=/Volumes/VIXinSSD/codewhale-target/fix-plugin-override cargo test -p codewhale-tui tools::plugin --all-features