fix: surface invalid MINIMAL_INDEX_SOURCE as a clean error instead of a panic - #1132
Conversation
download_if_available unwrapped the Result from remote_cache(), so an unrecognised MINIMAL_INDEX_SOURCE value aborted with a Rust panic and backtrace note, not a clean non-zero exit. remote_cache() already builds RemoteError::Config for this value; propagate it via map_err so the CLI prints the message instead of panicking.
norrietaylor
left a comment
There was a problem hiding this comment.
Reviewed: scope, clobber, correctness, CI. Approving.
Scope — one file, one hunk in crates/mctx/src/lib.rs. The single removal is the .unwrap() being replaced.
No clobber — nothing else removed; no competing work in flight on this file.
Correctness — and the match arm is load-bearing, not redundant. On first read the two arms look identical, both producing Error::Other(anyhow!(...)). They are not, because rcache::Error's Display is:
impl<BE: Debug> Display for Error<BE> {
fn fmt(&self, f) -> Result { write!(f, "{:?}", self) }
}It Debug-formats itself. So "{other}" on a Config variant renders Config("MINIMAL_INDEX_SOURCE: unknown index source \"banana\" ...") — the variant name, quotes and escaped inner quotes, i.e. precisely the string the issue is complaining about. The RemoteError::Config(msg) => anyhow!("{msg}") arm is what unwraps the clean inner message so the user sees:
MINIMAL_INDEX_SOURCE: unknown index source "banana" (expected "auto", "pinned", "root" or "closure")
Collapsing this to a single arm would reintroduce the defect. Worth a comment in the code someday, but not worth blocking this on.
The ? replaces a panic with normal error propagation, so the binary exits non-zero with a clean message and no backtrace note — exactly what the issue asked for. The error text itself was already correct and is unchanged.
CI — 20/20 green.
📝 WalkthroughWalkthrough
ChangesRemote-cache error handling
Estimated code review effort: 1 (Trivial) | ~3 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Both arms of the `map_err` added in #1132 produce `Error::Other(anyhow!(...))` and read as interchangeable. They are not. `rcache::Error`'s Display is `write!(f, "{:?}", self)` — it Debug-formats itself — so the fallback arm renders a Config as `Config("MINIMAL_INDEX_SOURCE: unknown index source \"banana\" ...")`, variant name and escaped quotes included. That is exactly the string gominimal/inbox#447 was filed about. Destructuring Config and formatting the inner `msg` is what produces the clean user-facing message. Collapsing this to a single arm would reintroduce the defect while looking like a simplification. Found while reviewing #1132, where I nearly filed that simplification myself before checking the Display impl. Comment only; no behaviour change. `cargo check -p mctx --locked` and `cargo fmt --all --check` both clean. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Routing-Key: inbox-route/I_kwDOSUhdos8AAAABK6GERQ
Setting
MINIMAL_INDEX_SOURCEto an unrecognised value mademinabort with a Rust panic and backtrace note, even though the underlying validation message is correct and already enumerates the valid modes.Context::download_if_availableunwrapped theResultfromremote_cache(), which already buildsRemoteError::Configfor a bad override; this propagates it viamap_errso the CLI prints the clean message and exits non-zero instead of panicking. No regression test is added: reaching that call site needs a full daemon/remote-cache context and the override is a process-global env var, while the parsing that builds the message already returnsResultat its source.Verification
cargo fmt --all --check — clean
cargo clippy --workspace --locked -- -D warnings — no warnings
cargo build --workspace --locked — Finished, ok
cargo test --workspace --locked — all tests passed (0 failed)
Note
Fix panic on invalid
MINIMAL_INDEX_SOURCEinContext.download_if_availableReplaces an
unwrap()on the remote cache result inContext.download_if_availablewith proper error propagation. ARemoteError::Config(or any otherRemoteError) is now converted toError::Otherviamap_errand returned to the caller instead of panicking.Macroscope summarized 88755d5.
Summary by CodeRabbit