Refactor task.rs error handling to clarify non-panic error propagation#2094
Refactor task.rs error handling to clarify non-panic error propagation#2094
Conversation
…ndling The `std::panic::catch_unwind` block previously returned a confusing nested `Result` type, leading to the misinterpretation that non-panic errors (such as those returned by `assets.copy()`) were being swallowed or incorrectly reported as success due to falling into the `Ok(exec_result)` match arm. This commit flattens the result using `.unwrap_or_else()` so that both caught panics and standard interpreter errors evaluate directly to an `Err(String)`. A single `report_result` invocation replaces the redundant `match` block, ensuring all failures are unmistakably and gracefully propagated to the server as a `TaskError`. Redundant panic reporting helpers were also removed. Co-authored-by: KCarretto <16250309+KCarretto@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Summary
Previous Results
Insights
Slowest Tests
🎉 No failed tests in this run. | 🍂 No flaky tests in this run. Github Test Reporter by CTRF 💚 |
1 participant
This change addresses the perceived bug where
assets.copyerrors were supposedly not reported to the server.The underlying issue:
std::panic::catch_unwindreturns anOk(T)if the closure executes without a panic. In the previous implementation,TwasResult<Value, String>. Therefore, a normalassets.copy()error yieldedOk(Err("asset not found")). Thematch result { Ok(exec_result) => report_result(...) }block correctly handled this by passing theErrtoreport_result, which properly formulated and transmitted theTaskErrorto the C2 server.However, because the failing task fell into the
Okbranch of thematchstatement, it created the illusion that non-panic errors were being ignored or improperly handled.The fix:
To eliminate this ambiguity and ensure panic/error handling is unmistakably transparent, the
catch_unwindresult is now flattened via.unwrap_or_else(|e| Err(format!("panic: {:?}", e))). This consolidates both panic payloads and scriptEldritchErrors into a single, cleanResult<Value, String>. Thereport_resultfunction then processes the outcome unconditionally, preserving graceful degradation and accurate server reporting. Unused panic-specific helper functions have been removed.PR created automatically by Jules for task 10654379505299874921 started by @KCarretto