Skip to content

fix: replace bare unwrap() with expect() in production code - #23

Closed
ztsalexey wants to merge 1 commit into
nearai:mainfrom
ztsalexey:fix/unwrap-expect
Closed

ztsalexey wants to merge 1 commit into
nearai:mainfrom
ztsalexey:fix/unwrap-expect

Conversation

@ztsalexey

@ztsalexey ztsalexey commented Feb 10, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • Replace all 31 bare .unwrap() calls in production code with .expect("reason") or idiomatic alternatives across 9 files
  • After this change, zero .unwrap() calls remain in non-test code
  • All replacements are in cases where the unwrap is logically safe but lacked documentation:
    • safety/leak_detector.rs: 16 hardcoded Regex::new() patterns — always valid at compile time
    • safety/sanitizer.rs: 4 hardcoded Regex::new() patterns — same
    • sandbox/proxy/http.rs: 3 Response::builder() chains — status/headers/body always valid
    • setup/wizard.rs: 2 state unwraps — guarded by control flow (set in prior steps)
    • workspace/chunker.rs: 2 chunks.pop() calls — both guarded by !chunks.is_empty() check
    • workspace/repository.rs: 1 embedding.unwrap() — guarded by embedding.is_some() check
    • settings.rs: 1 parts.last().unwrap() — guarded by parts.is_empty() early return
    • tools/builder/core.rs: 1 serde_json::to_string().unwrap() — simple struct serialization
    • cli/tool.rs: 1 check.unwrap() — replaced with map_or pattern (eliminates unwrap entirely)

Motivation

CLAUDE.md states: "Never use .unwrap() in production code (tests are fine)". A full audit found 31 bare .unwrap() calls in non-test code. While all are logically safe, replacing them with .expect() adds documentation for future readers and gives clear panic messages if assumptions are ever violated.

Test plan

  • cargo test --lib — all 524 unit tests pass (no test changes in this PR)
  • cargo check — compiles cleanly
  • Verified zero .unwrap() in production code after this change
  • Review each replacement to confirm the expect() reason is accurate

The project standard (CLAUDE.md) prohibits .unwrap() in production code,
but 31 calls existed across the codebase. A full audit confirmed all
remaining unwrap calls are in #[cfg(test)] blocks.

All 31 are technically safe (hardcoded regex patterns, guarded pops,
response builders with valid inputs, or state set in prior steps), but
bare unwrap() hides the safety reasoning. Replacing with expect()
documents the invariant and gives clear panic messages if assumptions
are ever violated.

Changes:
- leak_detector.rs: 16 Regex::new().unwrap() -> .expect("valid <name> regex")
- sanitizer.rs: 4 Regex::new().unwrap() -> .expect("valid <name> regex")
- sandbox/proxy/http.rs: 3 Response::builder().unwrap() -> .expect("valid response")
- setup/wizard.rs: 2 state.unwrap() -> .expect("set in previous step")
- chunker.rs: 2 chunks.pop().unwrap() -> .expect("checked non-empty above")
- repository.rs: 1 embedding.unwrap() -> .expect("checked is_some above")
- settings.rs: 1 parts.last().unwrap() -> .expect("checked non-empty above")
- tools/builder/core.rs: 1 to_string().unwrap() -> .expect("serializable output")
- cli/tool.rs: 1 check.unwrap() -> eliminated via map_or
@ztsalexey

Copy link
Copy Markdown
Contributor Author

nope

@ztsalexey ztsalexey closed this Feb 10, 2026
nickpismenkov added a commit that referenced this pull request May 13, 2026
This commit addresses every code review comment from PR #3590 and lands the
P1 follow-ups for the Telegram v2 tracer (durable storage backends fully
tested, migration smoke tests, boundary entry, real bot identity, ledger
settle verification, docs).

## Review comments addressed

| # | Reviewer | Change |
|---|---|---|
| 1 | gemini-code-assist | Centralize `derive_user_id` — new `identifiers.rs` module; libSQL and Postgres bindings share one implementation. Removes the documented/implementation drift the reviewer flagged. |
| 2 | gemini-code-assist | `parse_phase` exhaustive `match` in both ledger impls (replaces the fragile serde-quoted-string trick); paired comment on `phase_to_str` so any future `ActionPhase` variant fails closed. |
| 3 | gemini-code-assist | Postgres binding upsert is now a single-roundtrip `INSERT ... ON CONFLICT ... DO UPDATE SET adapter_id = EXCLUDED.adapter_id RETURNING thread_id` (was: separate `INSERT ... ON CONFLICT DO NOTHING` + `SELECT thread_id`). |
| 4 | gemini-code-assist | V28 `product_inbound_actions.action_id` is `UUID` (was `TEXT`); Postgres ledger binds `uuid::Uuid` directly via tokio-postgres `with-uuid-1` feature. libSQL keeps `TEXT` (no UUID type). |
| 5 | gemini-code-assist | Runner map keyed by the validated `installation_id.as_str()` (was: raw `installation_id_str` from env). Webhook lookup now matches what the adapter sees. |
| 6 | serrrfirat | Extracted Reborn boot orchestration out of `src/main.rs` into `src/channels/reborn/registry.rs::register_reborn_channels`. Main binary now has one function call + a `RebornChannelWiringInputs` struct; no direct references to Reborn crate types. This is interpretation (A) of the comment — orchestration-only extraction. If the reviewer's intent is interpretation (B), full crate-level separation of `src/channels/reborn/` is a follow-up. |

## P1 batch (per pre-agreed in-scope follow-ups)

* **#15** — Postgres contract tests: 4 tests for ledger (begin/settle/replay/release) and binding (idempotent + per-actor distinct) under `--features postgres`, gated on `IRONCLAW_PRODUCT_STORAGE_POSTGRES_URL` (or `DATABASE_URL`). Skip-clean when no Postgres available.
* **#16** — Migration smoke tests in `src/db/libsql_migrations.rs`: applies V26 against fresh in-memory libSQL, asserts both tables + all required columns + recording in `_migrations`.
* **#17** — `ironclaw_product_workflow_storage` added to architecture boundary rules forbidding `dispatcher` / `extensions` / `host_runtime` / `mcp` / `wasm` / `scripts` / `network` / `engine` / `gateway` / `secrets` / `authorization` / `capabilities` / `reborn` / `reborn_cli`.
* **#18** — `getMe` at boot resolves real `bot_user_id` + `bot_username` from `api.telegram.org` (was: hardcoded placeholders). Fail-soft on network/token errors: warn + use safe placeholders, so the binary still starts.
* **#19** — `accepted_inbound_settles_the_ledger_row` e2e test SELECTs the DB row after a webhook completes, asserts `phase='settled'` + `outcome_json` is non-null + `settled_at` is set. Proves `DefaultProductWorkflow::accept_inbound` actually invokes `ledger.settle` end-to-end.
* **#20** — Documentation: new `src/channels/reborn/CLAUDE.md` (file map, call paths, bridge seams, migration path post-#3583, operator setup, known gaps) + paragraph in top-level `CLAUDE.md` mentioning `REBORN_TELEGRAM_V2_ENABLED` and pointing at the module guide.

## Verification

  cargo fmt --all -- --check                                                                       # clean
  cargo clippy --bin ironclaw --features libsql --tests -- -D warnings                              # clean
  cargo clippy -p ironclaw_product_workflow_storage --features libsql,postgres --tests -- -D warnings  # clean
  cargo test -p ironclaw_product_workflow_storage --features libsql                                 # 13/13
  IRONCLAW_SKIP_POSTGRES_TESTS=1 cargo test --test postgres_contract -p ironclaw_product_workflow_storage --features libsql,postgres  # 4/4
  cargo test --test reborn_telegram_v2_e2e --features libsql --no-default-features                  # 7/7
  cargo test --lib --no-default-features --features libsql migration_smoke                          # 2/2
  cargo test -p ironclaw_architecture                                                               # 13/13

## P2 deferred to follow-up PRs

Per the in-scope vs follow-up split agreed during triage:

* #21 multi-installation support — own PR (env vs registry design)
* #22 setWebhook helper — own PR (CLI subcommand)
* #23 slash command routing — own PR (couples to Epic Child 9)

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.

1 participant