fix: error handling — log instead of silently swallowing errors (11 bugs)#2881
fix: error handling — log instead of silently swallowing errors (11 bugs)#2881HUQIANTAO wants to merge 6 commits into
Conversation
1. Fix swallowed persist_config errors (app-server/lib.rs:882,896)
- Log errors when config persistence fails after set/unset
- Users previously got success response even when disk write failed
2. Fix swallowed job store load error (core/lib.rs:751)
- Add warning log when job store fails to load at startup
- Previously silently started with empty job list on corruption
3. Fix silent config parse failures (config/lib.rs:1590)
- Log warning when project config TOML is malformed
- Previously returned None indistinguishable from 'no config file'
4. Fix MCP connect_all errors swallowed (mcp.rs:2151,2189)
- Log warnings for each server that fails to connect
- Previously returned incomplete resource list with no indication
5. Fix error context stripped in engine status (core/engine.rs:2223)
- Use {err:#} format to include full error chain
- Was inconsistent with line 2234 which already used {err:#}
6. Fix tool audit log failures silently dropped (tool_execution.rs:122-136)
- Log each failure: serialization, directory creation, file open, write
- Previously silently dropped all errors for security audit trail
7. Fix Err(_) arms discarding error info (runtime_log.rs:179, runtime_threads.rs:828)
- Log stderr redirect failures on Windows
- Log poisoned mutex in pending_approvals
8. Fix env var parsing errors silently ignored (config/lib.rs:2519-2530)
- Warn when DEEPSEEK_TELEMETRY, DEEPSEEK_YOLO, DEEPSEEK_HTTP_HEADERS
have invalid values instead of silently treating as unset
9. Fix MCP config reload errors swallowed (mcp.rs:2011)
- Log config reload errors instead of complete silence
10. Fix .expect() on sub-agent runtime (core/engine.rs:1715)
- Gracefully fall back to basic tool set when API client missing
- Previously panicked if subagents enabled but no client configured
11. Fix .expect() on goal objective (core/engine.rs:2543)
- Use safe if-let pattern instead of check+expect
- Prevents panic if refactoring changes control flow
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
HUQIANTAO has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
Thanks @HUQIANTAO for taking the time to contribute. This repository is currently observing a maintainer-managed contribution gate in dry-run mode, so this pull request is staying open. When enforcement is enabled, pull requests from contributors who are not listed in Please read |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request improves error handling and observability across several crates by replacing silent error discards with explicit logging (using tracing::warn! and tracing::error!). However, the changes in crates/tui/src/core/engine.rs introduce two issues: a compilation error in the sub-agent fallback logic due to an invalid return in a let-else block, and a subtle logic change in goal_objective_for_prompt that alters the fallback behavior when the state is inactive. Addressing these issues with the suggested refactorings will ensure the code compiles and maintains its original behavior.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| let Some(subagent_runtime) = runtime else { | ||
| tracing::warn!("Sub-agents enabled but no API client available, falling back to basic tool set"); | ||
| return Some(builder.build(tool_context)); | ||
| }; | ||
| Some( | ||
| builder | ||
| .with_subagent_tools( | ||
| self.subagent_manager.clone(), | ||
| runtime.expect("sub-agent runtime should exist with active client"), | ||
| subagent_runtime, | ||
| ) | ||
| .build(tool_context), | ||
| ) |
There was a problem hiding this comment.
This block introduces a compilation error. handle_send_message is an async fn that returns (), but the return Some(builder.build(tool_context)) statement inside the let-else block attempts to return Some(ToolRegistry). Additionally, using let-else here forces a diverging control flow (like return), which is not what you want since you want to fall back to the basic tool set and continue the function. Using a standard if let Some expression allows you to cleanly assign the fallback registry to tool_registry and continue execution without compilation errors.
| let Some(subagent_runtime) = runtime else { | |
| tracing::warn!("Sub-agents enabled but no API client available, falling back to basic tool set"); | |
| return Some(builder.build(tool_context)); | |
| }; | |
| Some( | |
| builder | |
| .with_subagent_tools( | |
| self.subagent_manager.clone(), | |
| runtime.expect("sub-agent runtime should exist with active client"), | |
| subagent_runtime, | |
| ) | |
| .build(tool_context), | |
| ) | |
| if let Some(subagent_runtime) = runtime { | |
| Some( | |
| builder | |
| .with_subagent_tools( | |
| self.subagent_manager.clone(), | |
| subagent_runtime, | |
| ) | |
| .build(tool_context), | |
| ) | |
| } else { | |
| tracing::warn!("Sub-agents enabled but no API client available, falling back to basic tool set"); | |
| Some(builder.build(tool_context)) | |
| } |
| if let Some(objective) = state.objective() { | ||
| if state.is_active() { | ||
| return Some(objective.to_string()); | ||
| } | ||
| } |
There was a problem hiding this comment.
This refactoring introduces a subtle logic change. In the original code, if state.objective() was Some but state.is_active() was false, the function returned None immediately, bypassing the fallback to normalized_goal_objective(configured_goal). With the new if let Some structure, if state.is_active() is false, the function falls through the match block and returns normalized_goal_objective(configured_goal). To preserve the exact original behavior safely without using expect, you can return state.is_active().then(...) directly when state.objective() is Some.
if let Some(objective) = state.objective() {
return state.is_active().then(|| objective.to_string());
}1. Replace let-else with if-let-Some to avoid compilation error - let-else with return would return from the entire function - if-let-Some correctly assigns to tool_registry and continues 2. Preserve original goal_objective_for_prompt behavior - Return None (not fallback) when objective exists but goal is inactive - Use state.is_active().then() to match original semantics
There was a problem hiding this comment.
HUQIANTAO has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
HUQIANTAO
changed the title
- Add tracing.workspace = true to crates/core/Cargo.toml (required for tracing::warn! in lib.rs:752) - Apply cargo fmt formatting to engine.rs, mcp.rs, tool_execution.rs, config/lib.rs
There was a problem hiding this comment.
HUQIANTAO has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
HUQIANTAO has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
HUQIANTAO has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
Required for tracing::error! in persist_config error handling.
There was a problem hiding this comment.
HUQIANTAO has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
1 participant
Summary
Fixes 11 error handling bugs where errors were silently discarded via
let _ =,Err(_),.ok(), or.to_string(), preventing users from diagnosing failures and masking data loss.Bugs Fixed
Silent Error Discards
persist_configerrors discarded (app-server/lib.rs:882,896)tracing::error!on persistence failure.Job store load error discarded (
core/lib.rs:751)tracing::warn!on load failure.Config parse failure returns
None(config/lib.rs:1590)toml::from_str().ok()makes "malformed config" indistinguishable from "no config file".tracing::warn!with the parse error before returningNone.MCP
connect_allerrors discarded (mcp.rs:2151,2189)tracing::warn!for each failed server.Error chain stripped in engine status (
core/engine.rs:2221)err.to_string()loses the anyhow error chain. Line 2234 already uses{err:#}correctly.format!("{err:#}")for consistency.Tool audit log failures all discarded (
tool_execution.rs:122-136)tracing::error!.Env var invalid values silently ignored (
config/lib.rs:2519-2530)DEEPSEEK_YOLO=TRUE(wrong case) orDEEPSEEK_TELEMETRY=maybetreated as unset.tracing::warn!on parse failure.MCP config reload errors completely silent (
mcp.rs:2011)tracing::warn!instead oflet _ =.Logic Errors
expect()on sub-agent runtime (core/engine.rs:1713)let-elsewithreturnwhich causes compilation errors.if let Somewith fallback to basic tool set.expect()on goal objective (core/engine.rs:2536)state.is_active().then(|| objective.to_string())to preserve original behavior.Windows stderr redirect error discarded (
runtime_log.rs:179)tracing::warn!on redirect failure.