Skip to content

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

Merged
Mikola Lysenko (mikolalysenko) merged 1 commit into
mainfrom
chore/unwrap-to-expect
Aug 18, 2026
Merged

chore: convert production unwrap() to expect() with invariant messages#198
Mikola Lysenko (mikolalysenko) merged 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 115 production-scope sites (13 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 69 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 4 parse_checked object-root guarantee, guard-inserted scripts key
core/src/api/client.rs 3 org_slug.is_some() guards, single-org match arm
core/src/vendor/go_sum_edit.rs 1 peek()-guarded iterator next()

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 13 files.

Rebase 2026-08-18

Rebased onto current main (was based pre-#187). Conflicts came from #196's rewrite of the Cargo.toml pinning logic (which replaced add_cargo_toml_registry — and 14 already-converted sites — with plan_cargo_toml), the yarn-classic CRLF work, the bundler CHECKSUMS CRLF tolerance, and composer's serialize_like_input (which removed 2 converted to_string_pretty sites). Resolution kept main's code and re-applied the conversion. The same scope-aware sweep then found 19 new production unwrap()s introduced on main (18 in redirect/mod.rs from #196 et al., 1 in the new vendor/go_sum_edit.rs from #173) — all converted in the same style, restoring the zero-production-unwraps invariant. Count moved 111 → 115 (−15 sites deleted by main's rewrites, +19 new). clippy --all-features -D warnings and default cargo test --workspace green after rebase.

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.

@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

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>
@mikolalysenko
Mikola Lysenko (mikolalysenko) merged commit 9b9af3f into main Aug 18, 2026
42 checks passed
@mikolalysenko
Mikola Lysenko (mikolalysenko) deleted the chore/unwrap-to-expect branch August 18, 2026 15:16
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