fix(core): remove three runtime expect()/panic sites#48
Merged
Conversation
Audit flagged three production paths where `.expect(...)` could panic
if a "should be impossible" invariant got violated. Exemplary CLI code
should not panic in runtime paths — replace each with an explicit
fallback or restructure so the impossible case is unrepresentable.
## Changes
`crates/core/src/state.rs:226-229`:
- Removed `impl Default for StateManager` (which called
`Self::new().expect("Failed to create default StateManager")`).
- `StateManager::new()` is fallible — derives a cache directory + does
IO — so a Default impl that hides the Result is wrong-shaped.
- Audit confirmed NO production callers use `StateManager::default()`;
every site uses `StateManager::new()?` or
`StateManager::new_with_cache_dir(path)?` and propagates the Result.
- Replaced the impl with a NOTE comment so a future contributor doesn't
accidentally re-add it.
`crates/core/src/config.rs:1536-1540`:
- The match arm gates `named_configs.len() == 1`, so `next()` is
guaranteed `Some` — but the `.expect(...)` was guarding it anyway.
- Replaced with a pattern match that returns a `ConfigError::Validation`
on the impossible `None` case. Defense-in-depth against a future
contributor changing how `named_configs` is built.
`crates/core/src/retry.rs:206-213`:
- Restructured the retry loop to return the error DIRECTLY on the last
attempt, instead of stashing it in `last_error: Option<E>` and
unwrapping after the loop.
- Removes the `.expect("Should have at least one error")` post-loop.
- Adds a `debug_assert!(false, ...) + unreachable!(...)` as defense if
the control flow ever changes — `u32::MAX + 1` iterations of the
`0..=max_attempts` loop must run at least once.
## Verification
- `cargo build`
- `cargo fmt --all -- --check`
- `cargo clippy --all-targets -- -D warnings`
- `cargo test --lib` → 280 deacon + 1081 deacon-core pass (no regression)
Refs: gap #7 from the post-1.0 audit.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.
Audit flagged three production paths where
.expect(...)could panic if a "should be impossible" invariant got violated. Exemplary CLI code should not panic in runtime paths.Closes gap #7 from the post-1.0 audit.
Changes
crates/core/src/state.rs:226-229— removedimpl Default for StateManager:Self::new().expect("Failed to create default StateManager").StateManager::new()is fallible (derives a cache directory + does IO) — a Default impl that hides the Result is wrong-shaped.StateManager::default(); every site usesnew()?/new_with_cache_dir(path)?and propagates.crates/core/src/config.rs:1536-1540—named_configssingle-match path:named_configs.len() == 1, sonext()is guaranteedSome— but the.expect(...)was guarding it anyway.ConfigError::Validationon the impossibleNonecase. Defense-in-depth.crates/core/src/retry.rs:206-213— restructured retry loop:last_error: Option<E>and unwrapping after the loop..expect("Should have at least one error").debug_assert!(false, ...) + unreachable!(...)as defense —0..=max_attempts(u32) always runs at least once.Verification
cargo buildcargo fmt --all -- --checkcargo clippy --all-targets -- -D warningscargo test --lib→ 280 deacon + 1081 deacon-core pass (no regression)🤖 Generated with Claude Code