Changed: Propagate pattern expansion errors through Rule::new#93
Changed: Propagate pattern expansion errors through Rule::new#93
Conversation
Rule::new now returns Result<Rule, ExpandError> instead of Rule to properly handle shell pattern expansion failures (e.g., unset /home/sewer in paths). Previously, expand_pattern failures were silently ignored. Now they propagate through the entire call chain: Rule::new -> from_permission_config -> AgentRuntime::from_parts -> AgentRuntimeBuilder::build. Updated all call sites: - Tests use ? operator with TestResult = Result<(), ExpandError> - Benchmarks use .expect() with descriptive messages - Doc examples use hidden fn main() -> Result wrappers Also cleaned up permissions_ext.rs doc example to remove awkward helper fn.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #93 +/- ##
==========================================
- Coverage 81.46% 81.34% -0.13%
==========================================
Files 106 106
Lines 4349 4363 +14
==========================================
+ Hits 3543 3549 +6
- Misses 806 814 +8
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📜 Recent review details⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
WalkthroughRule pattern expansion is made fallible: Possibly related PRs
🚥 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)
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.
🧹 Nitpick comments (1)
src/llm-coding-tools-core/src/permissions.rs (1)
459-461: Preferexpectover bareunwrapin the shared test fixture helper.Line 460 currently loses context if it ever fails; a descriptive
expectmakes triage faster.Suggested tweak
- ruleset.push(Rule::new(*perm, *pat, *action).unwrap()); + ruleset.push( + Rule::new(*perm, *pat, *action) + .expect("test fixture rule pattern should not fail expansion"), + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/llm-coding-tools-core/src/permissions.rs` around lines 459 - 461, The test fixture helper iterates over rules and calls Rule::new(...).unwrap(), which loses context on failure; replace the bare unwrap with expect to provide a descriptive failure message. Update the loop that builds ruleset (the for (perm, pat, action) in rules { ruleset.push(Rule::new(*perm, *pat, *action).unwrap()); }) to call Rule::new(...).expect("failed to create Rule for permission {:?}, pattern {:?}, action {:?}", perm, pat, action) or a similar message that includes perm/pat/action so failures in Rule::new and the ruleset push are easy to triage. Ensure you reference Rule::new and the ruleset construction when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/llm-coding-tools-core/src/permissions.rs`:
- Around line 459-461: The test fixture helper iterates over rules and calls
Rule::new(...).unwrap(), which loses context on failure; replace the bare unwrap
with expect to provide a descriptive failure message. Update the loop that
builds ruleset (the for (perm, pat, action) in rules {
ruleset.push(Rule::new(*perm, *pat, *action).unwrap()); }) to call
Rule::new(...).expect("failed to create Rule for permission {:?}, pattern {:?},
action {:?}", perm, pat, action) or a similar message that includes
perm/pat/action so failures in Rule::new and the ruleset push are easy to
triage. Ensure you reference Rule::new and the ruleset construction when making
the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7ce55b8e-9453-4c1c-99c0-0ab0fcaf44a2
📒 Files selected for processing (23)
src/llm-coding-tools-agents/benches/runtime_task.rssrc/llm-coding-tools-agents/src/extensions.rssrc/llm-coding-tools-agents/src/runtime/builder.rssrc/llm-coding-tools-agents/src/runtime/mod.rssrc/llm-coding-tools-agents/src/runtime/state.rssrc/llm-coding-tools-agents/src/runtime/task.rssrc/llm-coding-tools-core/README.mdsrc/llm-coding-tools-core/benches/permissions.rssrc/llm-coding-tools-core/src/permissions.rssrc/llm-coding-tools-core/src/permissions_ext.rssrc/llm-coding-tools-core/src/tools/bash/blocking_impl.rssrc/llm-coding-tools-core/src/tools/bash/tokio_impl.rssrc/llm-coding-tools-core/src/tools/edit.rssrc/llm-coding-tools-core/src/tools/glob.rssrc/llm-coding-tools-core/src/tools/grep.rssrc/llm-coding-tools-core/src/tools/read.rssrc/llm-coding-tools-core/src/tools/write.rssrc/llm-coding-tools-serdesai/README.mdsrc/llm-coding-tools-serdesai/examples/serdesai-agents.rssrc/llm-coding-tools-serdesai/examples/serdesai-task.rssrc/llm-coding-tools-serdesai/src/agent_runtime/build.rssrc/llm-coding-tools-serdesai/src/agent_runtime/task.rssrc/llm-coding-tools-serdesai/src/task/handle.rs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Semver Checks (Serdesai Full)
- GitHub Check: Blocking macOS
- GitHub Check: Async Linux
- GitHub Check: Async Windows
- GitHub Check: Blocking Windows
- GitHub Check: Async macOS
🔇 Additional comments (53)
src/llm-coding-tools-core/src/permissions.rs (2)
38-55: Doctest update is aligned with the new fallible API.Good update to use
Result<(), ExpandError>and?so the docs match runtime behavior.
136-153:Rule::newnow correctly propagates expansion failures.Line 136 and Line 142-153 correctly shift constructor behavior to fail-fast instead of silently swallowing expansion issues.
src/llm-coding-tools-core/README.md (1)
272-283: README example now correctly reflects fallible rule construction.The snippet is consistent with the new API and compiles cleanly in doctest style.
src/llm-coding-tools-core/benches/permissions.rs (1)
40-53: Benchmark fixture handling is appropriate for the fallible constructor.Using
expect("benchmark fixture patterns should not fail expansion")is a good fit here.src/llm-coding-tools-core/src/permissions_ext.rs (2)
9-24: Doc example is correctly migrated to fallible flow.This keeps
permissions_extdocs aligned with the new constructor semantics.
63-101: Test migration toResult<(), ExpandError>looks good.Line 74, Line 95, and corresponding
?usage correctly propagate expansion errors instead of panicking.src/llm-coding-tools-agents/src/extensions.rs (2)
40-65:from_permission_configerror propagation is correctly implemented.The
Result<Ruleset, ExpandError>signature and?usage at Line 54 and Line 58 are correct.
73-169: Tests are correctly adapted for the new fallible API.The
TestResultconversion and?usage keep test behavior consistent with production call paths.src/llm-coding-tools-agents/src/runtime/state.rs (1)
58-79:AgentRuntime::from_partsnow propagates configuration errors correctly.Line 62-67 and Line 71-79 cleanly thread
ExpandErrorthrough runtime initialization.src/llm-coding-tools-agents/src/runtime/builder.rs (2)
74-76: Builder return type update is correct.
build()now correctly exposesExpandErrorfrom runtime construction.
91-191: Builder tests are properly migrated to the fallible path.Using
TestResultandbuild()?keeps the tests aligned with the new API contract.src/llm-coding-tools-agents/src/runtime/mod.rs (1)
33-41: Runtime module example is updated correctly for fallible build.The doctest-style
main -> Resultandbuild()?are consistent with the current API.src/llm-coding-tools-serdesai/examples/serdesai-task.rs (1)
51-54: LGTM!The fallible
.build()?call correctly propagates any pattern expansion errors to the example'smainfunction, which returnsResult<(), Box<dyn std::error::Error>>. This aligns with the PR's goal of surfacing misconfigurations at startup.src/llm-coding-tools-serdesai/README.md (1)
112-116: LGTM!The README code snippet correctly demonstrates the updated fallible
.build()?pattern, matching the actual API change and the hiddenmain() -> Result<...>wrapper.src/llm-coding-tools-agents/benches/runtime_task.rs (1)
87-91: LGTM!Using
.expect()with a descriptive message is the correct approach for benchmark fixtures with hardcoded patterns that cannot fail expansion. This matches the PR's documented convention for benchmarks.src/llm-coding-tools-serdesai/examples/serdesai-agents.rs (1)
42-45: LGTM!The example correctly propagates the fallible
.build()?result, consistent with the PR's example update pattern.src/llm-coding-tools-serdesai/src/agent_runtime/task.rs (8)
148-153: LGTM!The
TestResulttype alias andExpandErrorimport provide a clean pattern for fallible tests. This convention is consistently applied across all modified test functions.
221-248: LGTM!Test correctly updated to return
TestResult, use.build()?for fallible runtime construction, and terminate withOk(()).
251-285: LGTM!Consistent test pattern applied correctly.
287-318: LGTM!Test correctly propagates build errors.
320-349: LGTM!Consistent with other test updates.
351-374: LGTM!Test correctly handles fallible build.
376-411: LGTM!Test with
max_task_depthcorrectly updated.
413-443: LGTM!Final test in module correctly follows the pattern.
src/llm-coding-tools-serdesai/src/task/handle.rs (2)
166-166: LGTM!Import of
ExpandErrorsupports the updated test helper signature.
239-247: LGTM!The updated
build_test_contexthelper acceptsResult<AgentRuntime, ExpandError>and uses.expect()internally. This centralizes the panic point for test fixtures that use hardcoded patterns and cannot fail expansion, keeping test call sites clean while maintaining proper error handling.src/llm-coding-tools-serdesai/src/agent_runtime/build.rs (5)
323-331: LGTM!The
TestResulttype alias andExpandErrorimport follow the same pattern established in other test modules.
431-464: LGTM!Test
build_filters_tools_by_permissioncorrectly updated to returnTestResultand use.build()?.
466-495: LGTM!Test correctly propagates build errors.
497-540: LGTM!Test
build_handles_catalog_edge_casescorrectly handles multiple runtime builds with?.
545-618: LGTM!Test
build_wires_line_numbers_to_correct_tool_variantcorrectly handles two runtime builds with?for testing different configurations.src/llm-coding-tools-agents/src/runtime/task.rs (15)
11-11: LGTM!Import of
ExpandErrorsupports the updated return types.
34-39: LGTM!
summarize_callable_targetscorrectly wraps the falliblecallable_targetscall and propagates the error.
55-67: LGTM!
callable_targetsnow correctly propagatesExpandErrorfromRuleset::from_permission_config. ReturningOk(Vec::new())for unknown callers is appropriate—it's a graceful fallback that maintains backward compatibility while still surfacing actual expansion failures.
192-197: LGTM!Test module correctly adds
ExpandErrorimport and definesTestResulttype alias.
243-255: LGTM!Test correctly updated to handle fallible
callable_targets.
258-289: LGTM!Test correctly propagates errors.
292-304: LGTM!Consistent test pattern.
307-335: LGTM!Test correctly handles fallible call.
338-369: LGTM!Test correctly propagates errors.
372-403: LGTM!Test correctly handles fallible callable_targets.
406-460: LGTM!OpenCode-style allowlist test correctly updated.
463-503: LGTM!Summary test correctly handles fallible
summarize_callable_targets.
506-516: LGTM!Deny task test correctly updated.
518-544: LGTM!Test correctly uses
.build()?for runtime construction.
546-574: LGTM!Final test correctly handles fallible build.
src/llm-coding-tools-core/src/tools/write.rs (1)
114-189: LGTM!The test correctly adopts the fallible
Rule::newAPI by:
- Importing
ExpandError- Defining
TestResulttype alias- Using
?to propagate errors from rule construction- Returning
Ok(())on successThe pattern is consistent with other test files in this PR.
src/llm-coding-tools-core/src/tools/edit.rs (1)
204-290: LGTM!The test changes follow the same correct pattern as other files:
- Import
ExpandError- Define
TestResulttype alias- Propagate errors with
?onRule::newcalls- Return
Ok(())on successsrc/llm-coding-tools-core/src/tools/read.rs (1)
422-639: LGTM!The test changes are consistent with the PR-wide pattern for handling fallible
Rule::new. The error propagation is correctly implemented.src/llm-coding-tools-core/src/tools/bash/tokio_impl.rs (1)
275-336: LGTM!The async test correctly adopts the fallible
Rule::newAPI. TheTestResultreturn type works correctly with#[tokio::test]async tests.src/llm-coding-tools-core/src/tools/bash/blocking_impl.rs (1)
234-293: LGTM!The blocking test changes mirror the tokio_impl.rs changes correctly. The pattern is consistent across both async and blocking implementations.
src/llm-coding-tools-core/src/tools/grep.rs (1)
475-850: LGTM!The test changes follow the established pattern correctly. Error propagation from
Rule::newis properly handled using the?operator.src/llm-coding-tools-core/src/tools/glob.rs (1)
243-465: LGTM!The test changes are consistent with all other files in this PR. The fallible
Rule::newAPI is correctly handled with proper error propagation.
… error context Replace bare unwrap() with expect() that includes permission, pattern, and action values when Rule::new fails in build_and_eval test helper. This makes test failures easier to triage by showing which rule failed.
1 participant
Previously,
Rule::new()would silently ignore failures when expanding shell patterns (e.g.,$HOME/file.txtwith unset$HOME). This PR changes the signature to returnResult<Rule, ExpandError>, ensuring misconfigurations are discovered at startup rather than silently ignored.Changes
Core API Change
Rule::newnow returnsResult<Rule, ExpandError>instead ofRulefrom_permission_configtrait method updated to returnResult<Ruleset, ExpandError>AgentRuntime::from_partsandAgentRuntimeBuilder::buildnow propagate the errorError Propagation Chain
Call Site Updates
?operator withtype TestResult = Result<(), ExpandError>.expect()with descriptive messages (hardcoded patterns can't fail)fn main() → Result<...>wrappers for clean?usageWhy
Silent failures hurt discoverability. If a user misconfigures a rule like:
and
$HOMEis unset, they should find out immediately at startup, not discover later that their permission rules aren't working as expected. Failing fast makes misconfigurations obvious and fixes easy.