architecture-audit

name	architecture-audit
description	Systematic architecture audit and refactoring methodology for Rust + TypeScript codebases. Use when performing refactoring, cleanup, unification, code review, dead code removal, module reorganization, or tech debt elimination. Ensures no naming confusion, semantic overloading, hidden defaults, duplicate logic, or architectural inconsistencies are missed.

Architecture Audit & Refactor Methodology

Lessons learned from multiple rounds of agent architecture refactoring where critical issues were repeatedly missed despite "thorough" audits.

Date	Change
2026-04	Added Layers 8–10 (Wire Protocol, Init Parity, Resolver Symmetry) and Systematic Sweep discipline after schemars/proxy token-bloat incident
2026-04	Added anti-patterns 22–34 (learnings-config refactor: config cohesion, background resolver coupling, fallback parity, session-layer decisions)
2026-05	Added anti-patterns 35–46 (control-flow, Agent Org finality, task-runtime, schema discipline, E2E false-path, team-mode, queue-progress)
2026-06	Added anti-patterns 47–54 (queue/turn lifecycle unification: FSM single source of truth, generation counters, cancel semantics, duplicate dispatchers)

When To Use

• Before ANY refactoring plan is finalized
• When user reports confusion about code that you previously audited
• When cleaning up a domain (e.g., "unified agent architecture")
• When doing dead code removal or module reorganization

Core Principle: Acceptance Criteria First

Before writing any plan, define the completion checklist — measurable criteria the codebase must satisfy when done. Every phase must map to at least one checklist item.

- [ ] Zero compiler warnings (cargo check / tsc --noEmit)
- [ ] Zero clippy warnings (cargo clippy --all-targets)
- [ ] Zero hardcoded domain strings (grep for known patterns)
- [ ] Zero duplicate type definitions across modules
- [ ] Zero layer violations (lower layers do not import upper layers)
- [ ] All files under size limit (per workspace rules)
- [ ] No backward-compat shims remaining (grep "compat", "legacy", "backward")
- [ ] Pre-user schema changes modify canonical DDL directly; no `ALTER TABLE`, legacy rebuilds, or migration tests unless explicitly requested
- [ ] No duplicate logic patterns (manual audit of init/setup/registration flows)
- [ ] No unused pub items (compiler warnings or manual grep)
- [ ] Term overloading table complete (Layer 4)
- [ ] Default branch analysis complete (Layer 5)
- [ ] Core modules free of variant-specific leakage (Layer 6)
- [ ] Wire payloads inspected for bloat/unwanted fields (Layer 8)
- [ ] All entry points perform identical init steps — comparison matrix complete (Layer 9)
- [ ] Multi-field resolvers use symmetric fallback chains — fallback matrix complete (Layer 10)
- [ ] Every found issue class has been swept globally, not just fixed at the reported site
- [ ] No types alive only in definition + re-export + test chains (Layer 2 call-chain trace)
- [ ] No cross-module naming collisions (same type name, different fields)
- [ ] No config structs spanning multiple unrelated domains (embedding + learnings + model selection in one struct)
- [ ] No background subsystems calling full session resolvers that enforce model-presence invariants
- [ ] No `expect()` on fallback paths that share the same failure mode as the primary path
- [ ] Session-layer decisions (LLM model, account) stay in session records, not agent config layer
- [ ] User-visible control actions have one dispatcher/source of truth, not UI-side duplicate send/cancel paths
- [ ] Runtime-completed assistant output is written to the authoritative EventStore, not only broadcast over transient UI channels
- [ ] Cancel APIs distinguish user Stop from programmatic Force Send so one path cannot poison the next turn
- [ ] Long-running orchestration surfaces reconcile finality from durable state, not from optimistic UI/session assumptions
- [ ] Run status, session status, task status, and member activity are asserted as separate dimensions
- [ ] No ownerless `in_progress`/claimed work can be persisted; if open work remains after all workers are terminal, the run is explicitly abandoned/failed/cancelled, not running
- [ ] Multi-agent task tools are role-aware: member self-claim is distinct from coordinator assignment, and recoverable misuse returns structured guidance rather than trajectory-visible execution errors
- [ ] Live orchestration context (task board, inbox, member activity) is marked volatile or revision-keyed; it is never hidden inside a stale stable prompt cache
- [ ] Rendered E2E for orchestration proves final outcome, durable invariants, prompt/context evidence, readable UI evidence, and absence of hidden tool-error trajectory leaks
- [ ] Rendered E2E does not use debug/helper endpoints as the side-effect path for the user-visible behavior under assertion; helpers may seed or inspect only
- [ ] Control/sentinel records (`redo:*`, batch envelopes, internal markers) are excluded from user-actionable UI registries and transcript input surfaces unless explicitly rendered as diagnostic metadata
- [ ] Team-mode / Agent Org member identity is sourced from runtime `member_id`/member name, not inferred from `agent_definition_id` or `agent_id` (one definition can back coordinator + multiple members)
- [ ] Drained inbox/mailbox messages are persisted as visible turn input before agent execution; LLM-only ephemeral attachments are not enough, and raw XML/internal payloads must not leak into the UI transcript
- [ ] Member turn completion maps to idle/available semantics, not terminal session completion; run finality must remain separate from per-turn member availability
- [ ] Task queue progress is event-driven: blocked assigned tasks are not notified early, dependency completion redispatches newly ready assigned tasks, and coordinator/cross-member tool calls cannot persist another member's work as `in_progress`
- [ ] Agent Org E2E asserts production inbox drain: unread member inbox rows must become visible turn input through the real member session path, and ready assigned open work must have either an active owner turn or unread wake row
- [ ] Adding a new E2E helper (`setTextarea`, custom drain endpoint, seeded snapshot helper, etc.) includes a sweep of all semantically matching call sites so old helpers do not keep driving the wrong DOM/runtime shape
- [ ] Turn finality has exactly one authoritative source (an FSM or equivalent monotonic state machine); `runtimeStatus` atoms, rendered events, heuristic timestamps, and streaming deltas are UI mirrors only and MUST NOT drive queue-flush decisions
- [ ] Every turn-ending signal (provider terminal, stream end, error, user Stop) carries a monotonically increasing generation counter; signals whose generation does not match the current turn are silently discarded
- [ ] The queue dispatcher reads a single gate (`turnPhase === "idle"`) — it does not read multiple atoms, boolean flags, or heuristic conditions to decide whether to send or queue
- [ ] User Stop and programmatic interrupt (Force Send cancel) travel separate code paths with explicit intent encoding; no shared cancel atom, flag, or default branch handles both simultaneously
- [ ] No separate "hold" atom or boolean flag shadows FSM state (e.g. "don't flush even if idle"); the FSM phase is the only source of truth for whether the queue may flush
- [ ] Provider events (stream end, tool call complete, error) are FSM *inputs*, not direct setters of `runtimeStatus`; the FSM transitions on them, the UI mirrors the FSM
- [ ] For every user-visible send/submit control, there is exactly one code path from button click to message dispatch; UI shortcut paths and background dispatcher paths that perform the same mutation are eliminated
- [ ] Atoms or flags that serve more than one concern (e.g. "signal user Stop" AND "gate draft restoration") are split; each concern has its own named atom with a single documented purpose

---

The 10-Layer Audit

Every audit MUST cover all 10 layers. Previous failures came from only covering layers 1-3, then layers 1-7 (missing wire protocol and init parity), then layers 1-9 (missing resolver symmetry).

Layer 1: Compilation Correctness

• Does it compile? (cargo check, tsc --noEmit)
• Zero warnings? (cargo clippy --all-targets)

Layer 2: Dead Code & Structural Deduplication

• Duplicate functions/structs across modules?
• Parallel code paths doing the same work?
• Abstractions created but never wired into execution path?
• Types that only appear in definition + re-export chains + tests?

Method: Call-Chain Tracing (not static grep)

For each major entry point:

1. Identify entry point (e.g., "user sends message" -> Tauri command -> handler)
2. Trace forward: what functions does it call? What structs does it construct?
3. Mark every touched function/struct as "alive"
4. Everything NOT marked is a deletion candidate
5. For "alive" items: is the same work done in >1 place? -> duplication candidate

Static grep for TODO, legacy, dead only finds self-documented problems. It misses structs never instantiated, functions never called, and duplicate logic in parallel paths.

CRITICAL: Reference counting is NOT a dead code audit. A type with 15+ grep hits can still be dead if all hits are: (a) its own definition, (b) re-export chains (types/mod.rs → session/mod.rs), (c) internal conversion methods, and (d) tests that only exercise those conversions. Trace from business entry points (Tauri commands, API handlers, gateway dispatchers) forward — if no production code path constructs or consumes the type, it's dead. See anti-pattern #26.

Layer 3: Naming Consistency

• Are renamed items updated everywhere?
• Old names still referenced in comments/strings?

Layer 4: Semantic Overloading (CRITICAL — Often Missed)

Search for the same word used with different meanings across the codebase.

Method: Pick every domain term and search ALL usages. Build a table:

Term: "gateway"
Usage 1: ProviderSpec.is_gateway -> means API aggregator
Usage 2: AgentVariant::Gateway -> means message routing agent
Usage 3: GATEWAY_AGENT_TYPES -> means Azure cross-provider proxy
VERDICT: Rename usages 1 and 3 to avoid confusion

Common overloaded terms: gateway, session, channel, provider, context, runtime, config, state, manager, handler, bridge, proxy, client.

Layer 5: Default Branch Analysis (CRITICAL — Often Missed)

Find every match with _ => or else catch-all and ask: "Is the default correct for ALL current and future variants?"

Dangerous pattern:

match variant {
    Sde => SdePromptBuilder,
    _ => OsPromptBuilder, // Custom agents silently get OS identity!
}

Audit every:

• match x { ..., _ => default } — is the default truly universal?
• if is_os { ... } else { ... } — does the else work for Custom/Gateway/future variants?
• unwrap_or(some_default) — is the default always correct?

Layer 6: Cross-Domain Concept Leakage (Often Missed)

Check if domain-specific concepts leak into shared/core modules.

Examples: sde_config field on shared SessionRuntime, hardcoded AgentVariant::Os.agent_id() in shared work item code, display labels "SDE Agent" hardcoded in shared aggregation code.

Method: For every file in core/ or shared modules, grep for variant-specific terms. Each hit needs justification.

Layer 7: "New Developer Confusion" Test (Often Missed)

Read the code as if you've never seen the codebase. For each function/struct:

1. Does the name accurately describe what it does?
2. Would a new developer understand this without tribal knowledge?
3. Are there misleading names that suggest a relationship that doesn't exist?

Layer 8: Wire Protocol & Serialization Audit (CRITICAL — Added 2026-04)

Check what the code ACTUALLY SENDS over the wire, not just what the source looks like.

This layer was added after schemars::openapi3() silently injected $schema, title, nullable, and default fields into tool schemas. The Rust source looked perfectly reasonable — the problem was only visible in the serialized JSON output, and only triggered by a specific proxy resolving the $schema URL.

Method:

1. Dump real payloads: For every external API call (LLM, HTTP, WebSocket), add a temporary debug dump of the serialized body to a file. Inspect the actual bytes, not the source structs.
2. Check schema generation libraries: If using schemars, serde_json::to_value, or any schema generator, inspect the output for fields the target API does not expect ($schema, title, nullable, default, examples, $ref).
3. Test against actual endpoints: A payload that "should work" per the source code may fail at a proxy or gateway. Always verify with a real call, not just cargo test.
4. Measure token impact: For LLM APIs, check prompt_tokens in the response. If it's 10x higher than expected, the payload has hidden bloat.

Dangerous patterns:

// Looks fine in source, but openapi3() adds $schema URL, title, nullable
schemars::generate::SchemaSettings::openapi3()

// Fix: use draft07 with no meta_schema
schemars::generate::SchemaSettings::draft07()
    .with(|s| { s.meta_schema = None; })

Checklist:

• Every to_value() / to_string() that crosses a network boundary: inspect the output
• Every schema generator: verify no unwanted fields in output
• Every proxy/gateway in the call chain: test with real payloads

Layer 9: Init Parity Across Entry Points (Added 2026-04)

Every entry point (production, test, E2E, API endpoint) must perform the SAME initialization steps.

This layer was added after the E2E test endpoint (/agent/test/sde) skipped AgentSession registration, causing init.rs to miss definition-level disabled tools — but production code via Tauri commands did register it.

Method:

1. List ALL entry points that create or initialize a session:
   • Tauri commands (production)
   • HTTP API endpoints (gateway/test)
   • Test helpers (#[cfg(test)])
   • CLI entry points
2. For each entry point, list the initialization steps it performs (in order)
3. Build a comparison matrix: rows = entry points, columns = init steps
4. Every cell must be filled — if an entry point skips a step, it needs explicit justification
5. Missing steps are bugs, not "simplifications for testing"

Dangerous pattern:

// Production path: registers definition, then inits session
state.register_session(agent_session).await;
ensure_session_initialized(&state, &session_id, &model).await;

// Test endpoint: skips registration, so init can't read definition
// This means disabled_tools from definition are never applied!
ensure_session_initialized(&state, &session_id, &model).await;

Layer 10: Resolver Symmetry (Added 2026-04)

When a single function resolves multiple fields using a priority chain (overrides → cache → DB → fallback), every field MUST follow the same chain unless there is an explicit, documented reason to diverge.

This was found in identity.rs where model only checked overrides + runtime (2 layers), while account_id and workspace_root checked overrides + runtime + DB (3 layers). The DB always had a valid model (required at creation time), but the resolver skipped it — causing an error on app restart when the frontend lost its lastModelSelectionAtom and the in-memory runtime hadn't been initialised yet.

Method:

1. Find every multi-field resolver — functions that resolve N related fields from the same set of sources
2. Build a fallback matrix: rows = fields, columns = data sources. Mark which sources each field checks.
3. Every cell should be filled — if a field skips a source, ask "why doesn't field X check source Y?"
4. Check the DB query trigger condition — if the DB query is conditional (lazy), verify the condition accounts for ALL fields, not just a subset

Dangerous pattern:

// model checks 2 layers, account_id and workspace check 3 — asymmetric!
let model = overrides.model
    .or_else(|| runtime.model.clone());  // stops here — no DB fallback
let model = model.ok_or("model is required")?;  // errors on app restart

let account_id = overrides.account_id
    .or_else(|| runtime.account_id.clone())
    .or_else(|| db_record.account_id.clone());  // has DB fallback

// Fix: all fields follow the same chain
let model = overrides.model
    .or_else(|| runtime.model.clone())
    .or_else(|| db_record.model.clone())  // now symmetric
    .ok_or("model is required")?;

Also watch for the DB query gate:

// BAD: gate only checks 2 of 3 fields — model miss won't trigger DB
let db_record = if account_id.is_none() || workspace.is_none() { query_db() }

// GOOD: gate checks all fields that may need DB fallback
let needs_db = model.is_none() || account_id.is_none() || workspace.is_none();
let db_record = if needs_db { query_db() }

Also audit for dimension mismatch: when a boolean flag (like is_channel) is used to branch behavior, check whether the flag's semantic dimension matches the actual requirement. Example: is_channel_session (dimension: "message source") was used to decide workspace path (dimension: "agent type"). OS Agent from the UI had no workspace — but is_channel_session was false for UI-launched sessions, so it hit the wrong branch.

---

Plan Structure

Phase ordering rules

1. Delete dead code first (Phase 1 always) — reduces noise for all subsequent phases
2. Unify duplicated logic next — establishes shared foundations
3. Structural/naming cleanup last — cosmetic changes on a clean codebase

Phase granularity

Each phase must be:

• Independently verifiable: cargo check passes after each phase
• Scope-bounded: affects at most ~20 files
• Both-sides: if a Rust change affects frontend types, the frontend change is in the SAME phase

Plan anti-patterns

• "Create abstraction" without "Wire it in" — creates dead code. Every "create" must have "integrate" + "delete old" in same phase.
• Phase marked "complete" without verification — each phase ends with cargo check --all-targets + zero warnings.
• Auditing one layer (Rust) but not the other (TypeScript) — audit both together for shared concepts.
• "Future" or "deferred" items — if worth noting, worth doing now or explicitly descoping with user.
• "It compiles, ship it" — compilation says nothing about semantic correctness.
• "Not in my task scope" — always expand audit scope to adjacent systems that share terminology.

---

Execution Discipline

Before each phase

1. Verify starting state: cargo check passes, note warning count
2. Read the files you're about to change (never edit blind)

After each phase

1. cargo check — zero errors
2. Warning count must be <= previous (ideally decreasing)
3. For frontend: tsc --noEmit or equivalent

Global verification (after all phases)

Run every checklist item. If any fails, the refactor is not complete.

---

Common Refactoring Patterns

Unifying duplicate initialization

When two code paths do overlapping work:

1. List every step each path performs (side by side)
2. Mark shared steps vs variant-specific steps
3. Create factory function for shared steps, returns "base" result
4. Each variant calls factory, adds variant-specific work
5. Delete duplicated code from each variant

Eliminating dead abstractions

1. Confirm zero callers (grep + compiler warnings)
2. If abstraction SHOULD be used: integrate it properly
3. If not: delete entirely
4. Never leave "aspirational" code

Replacing hardcoded strings with typed constants

1. Define enum/const in ONE canonical location
2. Add as_str() for serialization boundaries
3. Replace ALL occurrences (including tests and comments)
4. Verify zero remaining with grep

Introducing an FSM to replace scattered boolean/atom state

When "is the system in state X?" is answered by reading multiple atoms:

1. List every atom/boolean that contributes to the answer
2. Define the complete set of mutually-exclusive states (phases) as an enum/union type
3. Write transition functions for each edge (e.g. beginTurn, markRunning, markTerminal, forceIdle)
4. Add a monotonically increasing generation field; bump it synchronously in every begin* transition
5. All signal handlers check signal.generation === current.generation before acting
6. Delete the old atoms; derive any needed UI booleans from the FSM phase
7. Verify: grep the codebase for the old atom names — zero remaining reads outside the FSM module

---

Systematic Sweep Discipline (Added 2026-04)

When you find one instance of a problem category, you MUST sweep the entire codebase for all instances before moving on.

This was the single biggest failure mode in the 2026-04 audit cycle: fixing one blocking I/O site but not scanning for all others, fixing one error swallowing pattern but only in JSON/serde contexts.

The Rule

For every issue found:

1. Classify it — what is the general pattern? (e.g., "sync I/O in async fn", "unwrap_or_default hiding errors", "hardcoded string instead of const")
2. Write a grep pattern that catches ALL instances of this class, not just the one you found
3. Run the grep across the entire target scope (e.g., all of agent_core/)
4. Record the full hit list before fixing any
5. Fix ALL instances or explicitly defer with user agreement

Common sweep patterns

# Blocking I/O in async context
rg "std::fs::" --type rust -l  # then check if callers are async

# Error-swallowing unwrap_or_default
rg "unwrap_or_default\(\)" --type rust

# HTTP client construction hiding errors
rg "\.build\(\)\.unwrap_or" --type rust

# Hardcoded finish_reason strings
rg '"stop"|"tool_calls"|"end_turn"' --type rust

# Schema generators that may add unwanted fields
rg "SchemaSettings|into_root_schema" --type rust

# Repeated state lookups in one function (consolidation candidate)
rg "get_session\(&session_id\)" --type rust -c  # >1 per file = suspect

# Guaranteed-Some Option wrappers (ok_or followed by Some())
rg "ok_or.*\?\s*;" --type rust  # then check if result is wrapped in Some()

# Non-atomic multi-step DB writes (split-brain window)
rg "update_status|upsert_session" --type rust  # multiple calls in sequence = candidate for merge

# DEPRECATED fields still being assigned or read — remove or migrate first
rg -i "deprecated" --type rust -C 3  # then check: is the deprecated item still assigned/read?

# Types alive only in definition + re-export chains (zombie types)
# For each pub struct: count callers outside its own file + mod.rs re-exports + tests
# If all hits are definition/re-export/test → dead

# Cross-module naming collisions
# Export every pub struct name, sort, find duplicates across modules
rg "^pub struct " -- rust -l  

TypeScript/JavaScript sweep patterns

# TypeScript: atoms serving multiple concerns
rg "Atom\b" --type ts -l  # list files, then check each atom name for conjunctions

# TypeScript: event handlers directly setting runtime status
rg "setRuntimeStatus|setIsRunning|isRunning\s*=" --type ts

# TypeScript: duplicate send paths (direct transport calls outside dispatcher)
rg "dispatchMessage|sendMessage" --type ts -l  # >1 file calling transport = suspect

# TypeScript: UI components importing transport/dispatch directly
rg "from.*dispatcher|from.*transport" --type ts  # should only appear in the dispatcher file

# TypeScript: atoms reset in multiple places for different concerns
rg "set\(.*Atom.*false\)" --type ts  # find atoms cleared in multiple locations

Anti-pattern: "Fix the one, forget the class"

Round 1: Found blocking I/O in memory/commands.rs. Fixed it. Declared "blocking I/O: done."
Round 2: Found blocking I/O in init_helpers.rs, channel.rs, prompt_sections.rs, prompt_helpers.rs.

Why? Because round 1 only fixed the reported instance, never swept for the pattern.

---

Anti-Patterns That Caused Missed Issues

1. "It compiles, ship it" — _ => OsPromptBuilder compiles perfectly but gives Custom agents the wrong identity. Compilation correctness != semantic correctness.

2. "Not in my task scope" — Provider naming was missed because task was "unify agents". Always expand audit to adjacent systems sharing terminology.

3. "Grep-and-skim" — Searching AgentVariant::Os finds explicit uses but misses _ => branches. Read the logic, not just pattern matches.

4. "Fix what's reported, not what's wrong" — Fixing variant branches is shallow. The deeper issue (prompts fundamentally different, init 80% duplicated) requires reading full code paths.

5. "One more pass will catch everything" — Same mental model finds same category of issues. Use different audit lenses (the 7 layers) to find different categories.

6. "Fix the one, forget the class" (Added 2026-04) — Finding one blocking I/O site and fixing only that site. The correct response is: classify the pattern, grep the entire codebase, fix ALL instances. See "Systematic Sweep Discipline" above.

7. "Source looks fine, must be fine" (Added 2026-04) — schemars::openapi3() looks like a perfectly reasonable API call. The bug is in the OUTPUT, not the source. For anything that crosses a network boundary, inspect the serialized output, not just the source code. See Layer 8.

8. "Tests are simpler, they don't need full init" (Added 2026-04) — E2E test endpoints skipping AgentSession registration because "it's just a test." Every entry point must do the same init steps as production. See Layer 9.

9. "Infrastructure code doesn't need auditing" (Added 2026-04) — HTTP client construction, schema generation, serialization format — these are "boring plumbing" that gets skipped during audits. But they're exactly where silent failures hide (wrong TLS config via unwrap_or_default(), bloated schemas, missing headers).

10. "Some fields need fewer fallback layers" (Added 2026-04) — A resolver function resolves model, account_id, and workspace_root from the same source chain. Model skips the DB layer because "it's always provided by the frontend." But on app restart the frontend may not have it cached. All fields in the same resolver should follow the same priority chain. See Layer 10.

11. "Boolean flag matches the branching need" (Added 2026-04) — (semantic: message source) was used to branch workspace resolution (semantic: agent type). OS Agent sessions launched from the UI were , so they took the wrong path. When a flag drives branching, verify the flag's dimension matches the decision's dimension.

---

Refactoring Planning Rules

1. Never declare "final" in a plan name — there's always more. Use descriptive names.

2. Build term overloading table FIRST — before any plan, map every domain term to all usages.

3. Trace full call path — from frontend -> Tauri command -> core -> variant code. Issues hide at boundaries.

4. Check default branches — for every enum match, verify _ is intentional and correct.

5. Question "shared" modules — if a "shared" module references specific variants, it's not truly shared.

6. Read adjacent systems — auditing agent definitions? Also audit providers, sessions, tools.

7. Ask "what happens when someone adds a new variant?" — if adding AgentVariant::Research breaks things silently, fix now.

8. Sweep the class, not the instance — when you find a bug, define its category, grep the entire scope, fix all hits. Never fix one and move on.

9. Dump and inspect wire payloads — for any code that sends data to an external service, serialize and inspect the actual output. Source code is not enough.

10. Compare all entry points — build a matrix of (entry point) x (init steps). Missing cells are bugs.

11. Check resolver symmetry — when a function resolves N fields from the same source chain, build a (field) x (source) matrix. Every field should check every source. Asymmetry is a latent bug.

12. Match flag dimension to decision dimension — when a boolean flag drives an if/else, ask: "does this flag's semantic axis match the decision being made?" is_channel (message source) branching on workspace path (agent type) is a dimension mismatch.

13. Consolidate repeated lookups — when state.get_session(&id).await (or any map/lock lookup) appears N times in one function, consolidate into one lookup and extract all needed fields. Each extra lookup is a wasted lock acquisition and a readability tax.

14. Eliminate guaranteed-Some Option wrappers — when a value is produced by an ok_or / ok_or_else (guaranteed non-None), do NOT wrap it in Option just to match a legacy if let Some(ref x) pattern downstream. The wrapper erases the guarantee and forces defensive code throughout.