Skip to content

chore: convert production unwrap() to expect() with invariant messages - #198

Open
Mikola Lysenko (mikolalysenko) wants to merge 1 commit into
mainfrom
chore/unwrap-to-expect
Open

chore: convert production unwrap() to expect() with invariant messages#198
Mikola Lysenko (mikolalysenko) wants to merge 1 commit into
mainfrom
chore/unwrap-to-expect

Conversation

@mikolalysenko

@mikolalysenko Mikola Lysenko (mikolalysenko) commented Aug 15, 2026

Copy link
Copy Markdown
Collaborator

cc Jon (@darakian)

What

Audited every .unwrap() in the repo (8,651 sites in tracked Rust files) and converted the 111 production-scope sites (12 files) to .expect() with a message stating the invariant that justifies the expectation — so if an invariant is ever violated, the panic says why the value was supposed to exist instead of called Option::unwrap() on a None value.

Area Sites Typical messages
core/src/patch/redirect/mod.rs 64 static/escaped regex validity, capture-group existence, guarded Options
cli/src/commands/* (9 files) 38 --vex/identifier guard invariants, serde_json::Value serialization infallibility, manifest-path parents
core/src/setup/composer/mod.rs 6 parse_checked object-root guarantee, guard-inserted scripts key
core/src/api/client.rs 3 org_slug.is_some() guards, single-org match arm

What was deliberately left alone

  • Test code (~8.5k sites): cfg(test) modules, tests/, and the cfg(test)-gated vex/conformance_tests.rs / vendor/yarn_layering_tests.rs. Bare unwrap() is idiomatic there — a test panic already identifies the failing test — and converting them would drown the review in churn.
  • Zero behavior change: no unwrap? rewrites, no control-flow edits. Sites that are genuinely fallible (below) were converted with an honest message rather than silently "fixed".

How it was verified

  • Fan-out of conversion agents over disjoint file sets, then an independent lexical sweep (comment/string-blanked, brace-balanced #[cfg(test)] extent analysis) confirming no production .unwrap() was missed — including files where an early cfg(test) item precedes more production code. No production .unwrap_err()/.unwrap_unchecked() exist anywhere.
  • Adversarial review of every hunk: purity (token-identical modulo unwrap→expect + rustfmt rewrap), and message truth — each claimed guard/capture group/infallibility was checked against the actual surrounding code and regex patterns. One inaccurate message was caught and corrected (setup.rs stdout flush — see below).
  • cargo check --workspace --all-targets, cargo clippy (clean), full default cargo test --workspace (green), rustfmt --check on all 12 files.

Follow-up findings (reported, not fixed here)

  1. --manifest-path / panics — 4 sites (scan/mod.rs, rollback.rs, apply.rs, repair.rs) do manifest_path.parent().expect(…); resolved_manifest_path() passes an absolute flag value through verbatim, so a pathological --manifest-path / reaches parent() == None. Low severity, user-inflicted; the expect message now documents the intended invariant.
  2. setup.rs confirm_proceed stdout flush is genuinely fallible (EPIPE on closed stdout at prompt time). Same failure class as println! panicking on closed stdout elsewhere in the CLI, so behavior is uniform — but it's a real condition, and the message describes the failure rather than claiming a false invariant.
  3. maven_tag_inner_range interpolates tag into a regex without regex::escape (redirect/mod.rs). All current callers pass literal tag names, so the invariant holds today; a defensive regex::escape(tag) would future-proof it.

🤖 Generated with Claude Code


Note

Low Risk
No logic changes; only panic message text on paths that were already expected to be unreachable or infallible. Residual edge cases (e.g. pathological --manifest-path /) remain the same as before.

Overview
Replaces 111 production .unwrap() sites with .expect(...) so a violated assumption panics with a stated reason instead of a generic Option::unwrap message. Scope is 12 files in the CLI (apply, get, repair, rollback, scan, setup, output), core API client, patch redirect rewriters, and composer setup.

Messages tie to local guards: --vex / identifier Options, manifest file parents, infallible in-memory serde_json serialization, static or escaped regex compilation, and regex capture groups that the pattern always defines. Test code is unchanged (~8.5k unwrap() left in tests).

This is a mechanical refactor only—no unwrap? rewrites and no new error paths.

Reviewed by Cursor Bugbot for commit a44ce82. Configure here.

Sweep of every .unwrap() in production code paths (both crates):
111 sites across 12 files converted to .expect() whose message states
the invariant that justifies the expectation, so a violated invariant
panics with its reason instead of a bare unwrap message.

Test-scope unwraps (cfg(test) modules, tests/, and the cfg(test)-gated
conformance_tests.rs / yarn_layering_tests.rs) are deliberately left
alone: a test panic already identifies the failing test, and the churn
would drown review.

Zero behavior change: no control-flow edits, no unwrap-to-? rewrites.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

@darakian Jon (darakian) left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. 🤞this produces more usable logs over time

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants