relicario/docs/superpowers/reviews/2026-05-04-architecture-review.md

# Relicario — Whole-Codebase Architecture Review

**Date:** 2026-05-04
**Reviewers:** DEV-A (Rust core), DEV-B (Rust consumers — CLI, server, WASM), DEV-C (TypeScript — extension, relay)
**Synthesis:** PM
**Goal lens:** "Make this app's architecture logical and readable for a smart developer who doesn't know Rust but wants to learn by tinkering."

## Executive summary

The architecture is fundamentally sound: bytes-in/bytes-out core, no I/O leakage from `relicario-core`, a server that structurally cannot decrypt (no AEAD or KDF crate in its dep graph), a service-worker boundary in the extension that holds (content scripts make zero WASM calls), and CLI/extension parity at 22/23 capabilities. What hurts the goal lens is uneven detail: a few outsized files (`cli/main.rs` 2641 LOC, `extension/src/setup/setup.ts` 1220 LOC, `extension/src/vault/vault.ts` 1027 LOC) absorb concerns that belong in smaller modules, and a couple of cross-cutting boilerplate clusters (16× duplicated git-shell error UX in the CLI; duplicated SW router helpers; hand-maintained `wasm.d.ts`) make a learner re-derive the same pattern repeatedly. The single most important thing to address is a defense-in-depth crypto issue spanning Rust and JS: `SessionHandle` has no `impl Drop`, so the wasm-bindgen-generated `.free()` is a no-op for cleanup — the master key stays in WASM linear memory until JS explicitly calls `lock()`. The strongest aspects worth preserving are the documentation density of the security-critical core files (`crypto.rs`, `imgsecret.rs`, `backup.rs`, `tar_safe.rs` all open with multi-paragraph rationale), the discriminated-union message contract in the extension (`shared/messages.ts` + the popup-only / content-callable capability sets), and the server's structural enforcement of the ciphertext-only invariant via its import surface. These three patterns are the model for what every other surface should look like.

## Top-priority recommendations (P1)

### P1.1 — `SessionHandle` has no `impl Drop`; `.free()` is a cleanup no-op
**Area:** cross-cutting (wasm + extension)
**File(s):** `crates/relicario-wasm/src/lib.rs:15-23`, `crates/relicario-wasm/src/session.rs:1-58`, `extension/src/service-worker/session.ts:26`
**Found by:** DEV-B (Rust side, headline finding); DEV-C (symmetric JS-side concern, originally [P2])
**Observation:** `SessionHandle` is `pub struct SessionHandle(u32)` with no `Drop` impl. wasm-bindgen's auto-generated JS `.free()` drops a `u32` — i.e. nothing. The `SESSIONS` HashMap entry stays alive with the master key and image_secret in WASM linear memory until JS calls the explicit `lock(handle)` (which calls `session::remove`). DEV-C separately observed `service-worker/session.ts:26` swallows `free()` errors with `try { current.free() }` — that swallow is hiding the fact that the call wasn't doing crypto cleanup anyway.
**Why it matters for the user's goal:** This is the only finding in the review where the gap between "what the code looks like it does" and "what it actually does" is dangerous. A tinkerer reading `session.ts` reasonably assumes `free()` cleans up the WASM-side key; it does not. Defense-in-depth here is one Rust block and one JS audit.
**Suggested direction:** Add `impl Drop for SessionHandle { fn drop(&mut self) { session::remove(self.0); } }` to the WASM crate so `.free()` becomes the safety net and `lock()` becomes the explicit "I am done" call. In parallel, remove the `try { ... }` swallow at `service-worker/session.ts:26` (let exceptions propagate or log + counter) and audit every `.free()` callsite under `extension/src/` to ensure `wasm.lock(handle)` happens first regardless. A `wasm-bindgen-test` covering construct → drop → confirm registry empty locks the contract.
**Effort:** S (Rust fix) + S (JS audit) = ~1-2 hours total

### P1.2 — `crates/relicario-cli/src/main.rs` is a 2641-line monolith
**Area:** cli
**File(s):** `crates/relicario-cli/src/main.rs:1-2641`
**Found by:** DEV-B
**Observation:** The clap surface (lines 1-455) is a tour of the product and reads beautifully. Past line 456 the file becomes flat: every `cmd_*`, every `build_*_item`, every `edit_*`, six prompt helpers, three parsing helpers, and 24+ git shell-outs all live as peers. `cmd_add` calls 7 different `build_*_item` functions (each ~50-60 lines) with no module boundaries. Searching "where does add work?" requires scrolling, not navigating.
**Why it matters for the user's goal:** This is the first file a tinkerer opens after `cargo run -p relicario-cli -- --help`. Today they have to scroll through 2200 lines of flat code to follow any one command end-to-end. Splitting this is the precondition for fixing P1.3 and centralizing several other CLI patterns (groups-cache discipline, prompt helpers).
**Suggested direction:** Keep `main.rs` as clap definitions + the dispatching `match` (~470 lines). Split into `commands/{add,get,list,edit,trash,backup,import,attach,settings,sync,status,device,recovery_qr}.rs`, plus `prompt.rs` (the six `prompt_*` helpers + `prompt_secret`) and `parse.rs` (`parse_month_year`, `base32_decode_lenient`, `guess_mime`). Same LOC, different reading experience.
**Effort:** M (mechanical split, no logic changes)

### P1.3 — Git invocation boilerplate duplicated 16× with one-line errors
**Area:** cli
**File(s):** `crates/relicario-cli/src/main.rs:601, 602, 610, 986, 988, 1477, 1480, 1767, 1897, 1900, 2432, 2438, 2533, 2540` (and others)
**Found by:** DEV-B
**Observation:** Every `git_command` invocation that bails uses the same shape: `.args([...]).status()? + if !status.success() { bail!("git foo failed") }`. Child stderr is inherited interactively, but in test runs and noninteractive tooling it's lost, and the bail message is just the verb. When this fires in the wild — pre-receive reject, missing remote, dirty tree, signing-key prompt — the user sees one line and nothing actionable.
**Why it matters for the user's goal:** This is the *entire* error UX for the git side of the CLI. Failures are common, the diagnostic is uniformly unhelpful, and a learner will hit "git commit failed" with no context the first time they touch a misconfigured remote.
**Suggested direction:** Add `helpers::git_run(repo, args, context)` that uses `.output()` (capturing stderr), prints captured stderr unmodified on failure, and embeds the human-readable `context` ("commit add: GitHub", "register device", "purge trashed item"). 16 copies become one-liners.
**Effort:** S (single helper + sweep)

### P1.4 — `extension/src/setup/setup.ts` bypasses the SW and orchestrates WASM directly
**Area:** extension
**File(s):** `extension/src/setup/setup.ts:28-37`, `:1118-1120` (and the whole 1220-LOC file)
**Found by:** DEV-C
**Observation:** Popup, vault tab, and content scripts all funnel WASM through the service-worker. `setup.ts` is the only surface that imports `relicario-wasm` directly and orchestrates `unlock` / `embed_image_secret` / `register_device` / `manifest_encrypt` itself — duplicating ~400 LOC of crypto orchestration the SW already knows how to do. Side-effect: the setup tab can't be locked by the same session-timer the rest of the extension uses, and `WizardState` (passphrase + JPEG bytes + WASM handle) persists in module scope if the user abandons mid-wizard.
**Why it matters for the user's goal:** This is the single biggest "code lies about the pattern" surface in the extension. A learner who opens `setup.ts` first will see WASM imported directly and conclude that's how the extension works — it isn't; everywhere else routes through `chrome.runtime.sendMessage`. The file is also a 1220-LOC procedural wizard that could be a step registry.
**Suggested direction:** Add `create_vault` and `attach_vault` messages to the SW; turn `setup.ts` into a UI that posts those messages with the gathered config + image bytes. Convert the 6-step flow into a `{ id, render, attach }[]` step registry. The 1220 LOC drops to ~500. Add a `clearWizardState()` bound to `beforeunload` and to "return to step 0" so abandoned wizards don't persist sensitive material.
**Effort:** L (architectural — touches setup, the SW message router, and `wasm.d.ts`/types)

### P1.5 — `extension/src/vault/vault.ts` is a 1027-LOC do-everything file
**Area:** extension
**File(s):** `extension/src/vault/vault.ts:1-1027`
**Found by:** DEV-C
**Observation:** Single file owns: shell init, hash routing, sidebar render, list render, drawer state, type-picker, form-wrapper, deep-link routing, teardown coupling. Adding a new pane view today is a 5-place edit (`teardownPaneComponents` lines 813-820 is the symptom). Two state-management oddities sit inside: vault tab intercepts `vault_locked` errors via the RPC layer while the popup uses the `session_expired` event (two channels for one outcome), and `state.drawerOpen = true` survives navigation to non-list views.
**Why it matters for the user's goal:** This is the second steepest cliff for a tinkerer (after `setup.ts`). The vault tab is the user's primary fullscreen experience; the file that drives it should read as orchestration, not implementation.
**Suggested direction:** Split into `vault-shell.ts`, `vault-sidebar.ts`, `vault-list.ts`, `vault-drawer.ts`, `vault-form-wrapper.ts`, leaving `vault.ts` to own only routing and state. While doing so, lift the `vault_locked` RPC intercept into `shared/state.ts` (or a wrapper around `sendMessage`) so popup and vault use one path; reset `state.drawerOpen` at the start of `renderPane` for non-list views.
**Effort:** M (mechanical split, plus one channel-unification touch)

### P1.6 — `extension/src/shared/state.ts` is fully `any`-typed
**Area:** extension (shared)
**File(s):** `extension/src/shared/state.ts:10-35`
**Found by:** DEV-C
**Observation:** `StateHost` is the bridge that lets popup components run inside the vault tab — and it's the bridge most likely to drift between the two render targets — but its entire contract is `any`-typed. TS gives no signal when popup-only state shape diverges from vault-tab expectations. Module-scope `host` singleton additionally has no double-registration guard; tests forget a reset and the leak silently breaks isolation.
**Why it matters for the user's goal:** Two-render-target reuse is exactly the kind of architecture decision that pays off in maintainability — but only if the contract is type-checked. As-is, the bridge is the weakest learning surface in the extension.
**Suggested direction:** Define a concrete `StateHost` interface: `state: PopupState`, `navigate: (view: View) => void`, `popOutToTab(): void`, `isInTab(): boolean`, `openVaultTab(hash?: string): void`. Make `getState`/`setState` generic over `keyof PopupState`. Throw on `registerHost()` re-register; export a `__resetHostForTests()` helper.
**Effort:** S-M (type definitions + sweep through callers)

### P1.7 — `crates/relicario-core/src/recovery_qr.rs` is undocumented
**Area:** core
**File(s):** `crates/relicario-core/src/recovery_qr.rs:1-130`
**Found by:** DEV-A
**Observation:** No module-level `//!` header. No doc comments on `RecoveryQrPayload`, `generate_recovery_qr`, `unwrap_recovery_qr`, or `recovery_qr_to_svg`. Magic constants (`RREC`, `VERSION = 0x01`, `PAYLOAD_LEN = 109`) sit at the top with no explanation; the 4+1+32+24+48 layout is hand-counted with no struct, diagram, or asserts. The domain-separation prefix `b"relicario-recovery-v1\0"` exists but isn't explained. `production_params()` redeclares `KdfParams::default()` values with no comment on why the recovery format pins them.
**Why it matters for the user's goal:** Every other security-relevant file in the core (`crypto.rs`, `imgsecret.rs`, `backup.rs`, `tar_safe.rs`) has explanatory framing. A learner hitting `recovery_qr.rs` sees a different style and assumes either it doesn't matter or they've stumbled out of the documented zone. It does matter — this is the vault-key escape hatch — and a misuse here (e.g., reusing `production_params` as `KdfParams::default()` and then changing the default) silently breaks all extant recovery QRs.
**Suggested direction:** Add a module-level `//!` summarizing the format, the KDF-input domain separation, and the parameter-pinning rationale. Add a short ASCII diagram of the 109-byte layout near the constants. Doc-comment the four public functions. Either replace `production_params()` with a `const` or add a comment explaining the deliberate divergence from `KdfParams::default()`.
**Effort:** S (documentation only)

### P1.8 — `tools/relay/queue.test.ts` fails on uncommitted state
**Area:** tooling
**File(s):** `tools/relay/queue.test.ts:54`
**Found by:** DEV-C (verified via `bun test` → 1 fail / 4 pass)
**Observation:** Uncommitted changes added `dev-c` to the `Role` union and `KNOWN_ROLES` set in `queue.ts`, but the test still asserts the old enum: `assert.ok(!isRole("dev-c"))`. One-line fix.
**Why it matters for the user's goal:** A learner running `bun test` from `tools/relay/` immediately sees a failure and assumes the codebase is broken. The relay is the literal coordination substrate of this review; a green test run is table stakes.
**Suggested direction:** Update the test assertion to `assert.ok(isRole("dev-c"))` and add a negative case (`assert.ok(!isRole("dev-d"))`). Confirm `start.sh` opens a fourth window for dev-c (DEV-C suspected `:80` still hardcodes "Dev-B" in user-facing output).
**Effort:** S (one-line edit + one-launcher-line check)

### P1.9 — Duplicated SW router helpers (storage helpers + `itemToManifestEntry`)
**Area:** extension (service-worker)
**File(s):** `extension/src/service-worker/router/popup-only.ts:687-703` and `:~169`; `extension/src/service-worker/router/content-callable.ts:187-205` and `:~169`
**Found by:** DEV-C
**Observation:** Three identical `chrome.storage.local` helpers (`loadDeviceSettings`, `loadBlacklist`, `saveBlacklist`) and the 17-line `itemToManifestEntry()` projection are duplicated across both router files. Both code paths can mutate the blacklist via different definitions; future drift will silently corrupt one path. Manifest schema refactors will need to be made twice.
**Why it matters for the user's goal:** A learner reading either router file sees private helpers and assumes that's where they live; finding the same name in the sibling router with a near-identical body is a routine "wait, why is this duplicated?" moment that should not exist in a tightly-typed message-router architecture.
**Suggested direction:** Extract the three storage helpers to `service-worker/storage.ts`; move `itemToManifestEntry` into `service-worker/vault.ts`. Import from both router files. Pair this with [P1.6] for `shared/state.ts` typing — both are about giving the extension's "shared utilities" a concrete shape.
**Effort:** S (extract + sweep)

### P1.10 — Pure parsers in CLI that the extension will eventually need
**Area:** cli → core
**File(s):** `crates/relicario-cli/src/main.rs:942-980` (`parse_month_year`, `base32_decode_lenient`, `guess_mime`)
**Found by:** DEV-B
**Observation:** Three pure parsers producing typed core values currently live only in the CLI. Per the project's CLI/extension parity philosophy (CLAUDE.md memory rule), anything the CLI does in pure logic the extension will eventually need too — QR-import, month-year smart input, attachment MIME detection. There is also a `base32_encode` in `core/item.rs` and a `decode_base32_totp` in `core/import_lastpass.rs` that are inverse pairs in different modules — DEV-A flagged the same shape from the core side ([P2] base32 has three implementations).
**Why it matters for the user's goal:** Parity is a stated design philosophy, and parser drift between CLI and extension is the most likely place for it to fail silently.
**Suggested direction:** Migrate to core: `MonthYear::parse` (already partial in `time.rs`), `Totp::parse_secret` (or `ItemCore::parse_totp_seed`), `mime::guess_for_extension`. Pair with extracting a `pub(crate) mod base32` in core with `encode_rfc4648` / `decode_rfc4648`, leaving Steam's bespoke alphabet where it is. The CLI keeps thin wrappers; the WASM crate exposes the new functions; the extension calls them via SW message handlers.
**Effort:** M (move + adjust CLI callers + add WASM exports)

## P2 recommendations

### Core (Rust)
- **`extract_with_crop_recovery` is narrower than the spec describes.** `crates/relicario-core/src/imgsecret.rs:849-899`. Spec promises 15% from any edge; impl pins `dx=0, dy=0` and varies only `orig_w`/`orig_h`. Left/top crops won't recover. Either extend the recovery loop or update the spec language to "right/bottom crop tolerance only." (DEV-A)
- **Stale "in-progress rewrite" headers** in `vault.rs:1-7`, `manifest.rs:1-2`, `attachment.rs:1-4`. Each comment describes work that already shipped. Replace with one-line module summaries. (DEV-A)
- **Two dead fields in `EmbedRegion`** (`imgsecret.rs:225-229`). `region_width`/`region_height` are computed, stored, and silenced with `#[allow(dead_code)]`. Either delete or comment the future use. (DEV-A)
- **Three base32 implementations.** `item.rs:255-275`, `import_lastpass.rs:202-220`, `item_types/totp.rs:13` (Steam, intentionally different). Extract a `pub(crate) mod base32`; leave Steam alone with a neighbour comment. (DEV-A; folds into P1.10.)
- **Backup format embeds the reference JPEG as base64-in-JSON** (`backup.rs:148-152, 274-280, 343-345`). Round-trip works; bloats by ~33% over the binary baseline post-zstd. Defer until backup-size pressure shows up. (DEV-A)

### CLI (Rust)
- **`build_*_item` functions mix prompting, parsing, and core construction.** `main.rs:664-921`. Compress with a `prompt_or_flag<T>` helper. (DEV-B)
- **`refresh_groups_cache` invocation discipline is manual at 7 sites.** `main.rs:641, 998, 1123, 1197, 1414, 1432, 1474`. Rule "every mutating handler must call this" is unenforced. Wrap in `Vault::after_manifest_change(&self, manifest: &Manifest)`. (DEV-B)
- **`ParamsFile` defined twice with mismatching shapes.** Write side `main.rs:2287` has `aead`, `salt_path`, `format_version`; read side `session.rs:114` takes only `kdf`. Single struct in core or shared session module. (DEV-B)
- **`cmd_purge` and `cmd_trash_empty` duplicate the manifest-add-and-commit dance** (`main.rs:1476-1480, 1896-1900`). 50-item purge does 150 git invocations; batch the staging. (DEV-B)

### Server (Rust)
- **`generate-hook` assumes `$PATH`.** `relicario-server/src/main.rs:170`. Most Gitea hook environments don't have `/usr/local/bin`; `command not found` is the failure mode. Embed `current_exe()` or emit an explicit `PATH=` line. (DEV-B)
- **Bootstrap branch is permissive.** `:38-44, 54-57`. Missing `.relicario/devices.json` at `newrev` is treated as bootstrap. An attacker pushing a brand-new branch that strips `.relicario/` could push unsigned commits. Either document the rule or enforce: `oldrev != 0` ⇒ `devices.json` must exist in `newrev`. (DEV-B)
- **stdin-parsing lives in the shell hook only.** `:130-189`. Wiring up the binary by hand isn't possible without re-implementing the per-line `<old> <new> <ref>` parse. Add `verify-commit --from-stdin` or doc-comment the constraint. (DEV-B)
- **Test coverage gaps** (`tests/verify_commit.rs`): unsigned-commit, malformed `devices.json`, bootstrap-empty, stripped-`.relicario/`. Each is one extra `#[test]`. (DEV-B)

### WASM (Rust)
- **Redundant double-lookup pattern.** `lib.rs:73, 84, 92, 103, 111, 122, 160, 172` all do `need_key(handle)?` then `session::with(handle.0, |k| ...).unwrap()` — two HashMap lookups per call. Single-`with` helper that returns `JsError` on miss. (DEV-B)
- **`Vec<u8>` getters clone on every read.** `EncryptedAttachment::aid` and `bytes` (`lib.rs:141-150`). Document "call once, cache locally" or consume by value. (DEV-B)
- **`wasm_*_recovery_qr` prefix is inconsistent** with everything else. `lib.rs:497, 510`. Rename to `generate_recovery_qr_svg` and `unwrap_recovery_qr` (and update `extension/src/wasm.d.ts`). Trivial breaking rename — do it before any new caller appears. (DEV-B)
- **`device.rs` and `session.rs` use different concurrency primitives** (`Lazy<Mutex<...>>` vs `thread_local! { RefCell<...> }`). Pick one. (DEV-B)
- **`extension/src/wasm.d.ts` is hand-written and explicitly requires manual sync** with `crates/relicario-wasm/src/lib.rs`. Every change to `#[wasm_bindgen]` must be mirrored manually; today they're aligned. Add a CI check comparing `wasm-pack`–generated `.d.ts` against this file, or import the generated file directly. (DEV-C; partner finding to DEV-B's WASM section.)

### Extension — service-worker
- **Inactivity timer reset skips content-callable messages** (`service-worker/index.ts:76-78`). A user actively autofilling but not opening the popup will be force-locked despite continuous use. Reset on all messages except known read-only content calls. (DEV-C)
- **Session expiry clears `state.manifest` but leaves `state.gitHost`** (`service-worker/index.ts:51-58`). Cached git-host client survives expiry; rotation could mix with stale connection state. Null `state.gitHost` alongside. (DEV-C)
- **`try { current.free() }` swallows free errors** (`service-worker/session.ts:26`). See P1.1 — this becomes important once `impl Drop` lands. (DEV-C)

### Extension — popup + components
- **Duplicated teardown helpers** (`settings.ts:56-65` and `settings-vault.ts:15-22`). After the recent stub-restore commits (`8baef5b`, `ddfb95d`), teardown leaks are a known regression class — duplicated cleanup is exactly the pattern that re-introduces them. Extract a single `teardownSettingsCommon()`. (DEV-C; originally P1, demoted by PM because the leak vector is small but the duplication is real.)
- **Settings module-scope singletons** (`settings.ts:33-34`). `pendingVaultSettings` and `sessionHandle` survive section navigation. Reset on `renderSection` entry, or scope into a closure per render. (DEV-C)
- **Item-list popover wires listeners on every render** (`item-list.ts:152-353`) without a reuse path. Cache the DOM and reuse, or use AbortController. (DEV-C)
- **`Promise.all` without per-promise error handling** (`devices.ts:47-50`, `trash.ts:39-46`). Single rejected RPC fails the whole render. Use `Promise.allSettled`. (DEV-C)
- **Generator-panel cleanup not idempotent-guarded** (`generator-panel.ts:89-261`). Currently safe by accident. Add `if (!activePanel) return`. (DEV-C)
- **Popup teardown calls every type module unconditionally** (`popup.ts:178-181`). Track last-mounted type. (DEV-C)

### Extension — vault tab
- **Vault tab intercepts `vault_locked` via RPC; popup uses `session_expired` event.** `vault/vault.ts:47-74`. See P1.5 — collapse into one mechanism in `shared/state.ts`. (DEV-C)
- **Drawer doesn't auto-close on non-list view changes** (`vault.ts:495-536`). Reset `state.drawerOpen = false` in `renderPane`. (DEV-C)
- **Sidebar re-renders on every search keystroke without debounce** (`vault.ts:648-695`). 50-100ms debounce. (DEV-C)

### Extension — content scripts
- **`fillFields()` returns silently when no password field is found** (`content/fill.ts:50-64`). Dynamic forms can race the listener; user clicks autofill, nothing happens. Send a `fill_failed` ack. (DEV-C)
- **MutationObserver scan is not debounced** (`content/detector.ts:96-103`). SPA churn re-runs the full scan many times per second. Wrap in `requestIdleCallback` or 200ms timer. (DEV-C)
- **Outside-click listener leaks on alternate close paths** (`content/icon.ts:169-175`). AbortController scoped to picker open, or remove in every close path. (DEV-C)

### Extension — setup
- **`setup.ts` 1220-LOC procedural wizard.** Step registry pattern. See P1.4. (DEV-C)
- **`WizardState` is module-scope; sensitive material persists if user abandons mid-wizard** (`setup.ts:69-94`). Add `clearWizardState()` on `beforeunload` and on returning to step 0. Folds into P1.4. (DEV-C)
- **Manifest path constants duplicated** between `setup/probe.ts:11-23` and `service-worker/vault.ts`. Define `VAULT_PATHS` in `shared/types.ts` (or `shared/paths.ts`). (DEV-C)

### Extension — shared utilities
- **Base `Response` is `{ data?: unknown }`; every consumer does hand-written `as ListItemsResponse` casts** (`shared/messages.ts:85-87`). Generic `Response<TKind extends Request['type']>` mapped from a single `MessageMap` table. (DEV-C)
- **`group-autocomplete.ts:26` builds list HTML via `innerHTML` with only `&quot;` escaping.** Group names are user-entered. `<` and `>` aren't escaped — markup-injection risk. Use `document.createElement('option')`. (DEV-C)
- **`restore_backup` flattens `newRemote` inline** (`shared/messages.ts:56-66`). Inconsistent with sibling messages. Extract `RestoreBackupPayload`. (DEV-C)

### WASM boundary (JS side)
- **`__stubs__/relicario_wasm.stub.ts` only stubs 7 of ~25 exports.** Adding a vitest test that touches a new WASM call needs an ad-hoc per-test mock. Round out the stub or provide a `mockWasm({...})` test helper. (DEV-C)

### Relay tooling
- **`tools/relay/start.sh:80` may still reference "Dev-B"** in user-facing output despite the dev-c expansion. Confirm a fourth window opens for dev-c. (DEV-C)
- **`tools/relay/call.py` and `call.ts` are untracked but load-bearing** for the multi-agent fallback path (kickoff prompts reference `call.py` by path). Either track them with a one-line header explaining "MCP-fallback shim" or add to `.gitignore`. (DEV-C)
- **In-memory queue has no TTL, persistence, or cap** (`queue.ts:21-27`). Document the dev-only ephemeral contract or add a per-role cap. (DEV-C)

### Cross-cutting
- **`#[allow(dead_code)]` without explanation** appears in `cli/device.rs`, `cli/gitea.rs`, and `wasm/device.rs`. Each is either "API completeness" or "scar tissue." Annotate with `// TODO(<plan>):` or delete. (DEV-B)
- **Direct `chrome.storage.local` reads from popup components** (`settings-security.ts:112-113`, `setup.ts:1056-1062`). Every other module routes via `sendMessage`. Pick one paradigm and document the exception. (DEV-C)
- **`bun test` is not the project's intended runner** (`extension/package.json:13` is `vitest run`; `tools/relay/` uses `node:test` via `bun test`). README clarification. (DEV-C)
- **Error formatting is inconsistent** across all three Rust crates: CLI mixes sentence-case and lowercase fragments; server is `eprintln!("REJECT: ...")` *except* the malformed-devices.json path; WASM ranges from explicit messages to `RelicarioError::Display` passthrough. Short style note + audit pass. (DEV-B)

## P3 / nice-to-have

A long tail of style sweeps and small ergonomic wins. Pulling the most representative; full lists in the per-reviewer notes.

- Inconsistent error types and constant styles in core (`MonthYear::new -> Result<_, &'static str>`; `pub const MAGIC: [u8;4]` vs `const MAGIC: &[u8;4]`; empty `[dev-dependencies]`). (DEV-A)
- Mid-file `use` blocks in `item.rs:117-122` and `attachment.rs:43-46`. (DEV-A)
- `r#type` field on `Item` and `ManifestEntry` — rename to `item_type` with `#[serde(rename = "type")]`. (DEV-A)
- BIP39 minimum word count of 3 is misleading vs. the 128-bit-security framing — restrict to canonical sets or rename to "BIP39-wordlist passphrase generator." (DEV-A)
- `STEAM_ALPHABET` source comment contradicts its own test about the '5' character. (DEV-A)
- `let _ = entry;` newcomer-hostile pattern repeated 6× in CLI. (DEV-B)
- `cmd_recovery_qr_unwrap` doesn't check empty input before base64 decode; QR ASCII and "NOT been saved" mixed on stdout (pipe-unfriendly). (DEV-B)
- `Lock` CLI subcommand is a visible no-op; either `#[command(hide = true)]` or document the parity rationale. (DEV-B)
- Three test-only env vars duplicated under `#[cfg(debug_assertions)]/#[cfg(not(debug_assertions))]` — one macro flattens ~30 lines. (DEV-B)
- WASM exports use snake_case in JS (`manifest_encrypt`); JS idiom is camelCase. Decide once via `#[wasm_bindgen(js_name = ...)]`. (DEV-B/DEV-C)
- Glyph-rule partial adoption — six popup files use raw glyph literals (`⧉`, `↻`, `▸`, `▾`, `≡`, `⤓`) inline despite the `glyphs.ts` abstraction. Add the missing constants and migrate. (DEV-C)
- `TotpKind = 'totp' | 'steam' | { hotp: { counter: number } }` mixed string/object union — flat discriminated union reads cheaper. (DEV-C)
- `void tick()` in `totp-tools.ts:39-46` swallows promise rejections. (DEV-C)
- `setup.ts:1-7` header says "5-step flow"; code is 6 steps (0..5). (DEV-C)
- `helpers.rs:37 #[allow(dead_code)] pub fn relicario_dir()` has no callers but several `vault_dir.join(".relicario")` sites should use it. (DEV-B)
- `gitea.rs` constructs a fresh `reqwest::blocking::Client::new()` per method call — make it a struct field. (DEV-B)

## Cross-cutting themes

**1. The "where the work happens" boundary holds, but two surfaces lie about it.** `relicario-core` is genuinely platform-agnostic (no fs, no net, no git) and the server provably cannot decrypt (no AEAD/KDF crate present). The extension's content scripts make zero direct WASM calls. These are excellent structural invariants. The two surfaces that break the pattern are `extension/src/setup/setup.ts` (loads WASM and orchestrates crypto directly, bypassing the SW) and the `SessionHandle` `.free()` non-cleanup contract on the WASM/JS boundary. Both are P1; both are also single-surface fixes. The architecture is one P1.1 + one P1.4 away from being uniform.

**2. Duplication concentrates at boundaries, not inside modules.** The two router files duplicate storage helpers and `itemToManifestEntry` (P1.9). The CLI duplicates git-shell error UX 16× (P1.3). Three base32 implementations live in core (P2). Two `ParamsFile` definitions disagree (P2). `parse_month_year` / `base32_decode_lenient` / `guess_mime` live only in the CLI but the extension will need them (P1.10). The pattern: every time a concept crosses a module/crate boundary, the second crossing copies the first instead of importing it. The remedy is small extractions (one helper module per cluster), not refactors. None of these are individually expensive; together they account for a meaningful fraction of total findings.

**3. Three files account for most of the readability cost.** `cli/main.rs` (2641 LOC), `setup.ts` (1220 LOC), `vault.ts` (1027 LOC). Each carries multiple concerns that belong in sibling modules. Splitting all three is cumulative ~M-L effort and unlocks several smaller cleanups (centralizing groups-cache discipline depends on splitting `main.rs`; the SW message-routing fix depends on extracting `setup.ts`'s WASM orchestration; lifting the `vault_locked` channel depends on splitting `vault.ts`). For the user's stated learning-by-tinkering goal, these three splits are the highest-leverage architectural moves available.

**4. Documentation density is uneven in exactly one place.** Core's security-critical files (`crypto.rs`, `imgsecret.rs`, `backup.rs`, `tar_safe.rs`) open with multi-paragraph rationale walking through the *why*. `recovery_qr.rs` does not — and it's the user-visible last-resort recovery mechanism. Bringing one file up to the existing standard is a P1.7 effort of S. Beyond that one file, the core doc story is genuinely strong; preserve it.

## What's strong (preserve)

- **`crypto.rs`, `imgsecret.rs`, `backup.rs`, `tar_safe.rs`: documentation density.** Each opens with multi-paragraph rationale (XChaCha20 vs AES-GCM, QIM with QUANT_STEP=50, length-prefixed concatenation, tar-bomb defenses). This is the model the rest of the crate should be measured against.
- **`relicario-server` trust-model enforcement via the import surface.** The server cannot decrypt the vault even in principle: no AEAD or KDF crate in its dep graph; the entire `relicario-core` import surface is `DeviceEntry`, `RevokedEntry`, `fingerprint`, and (in tests) `generate_keypair` — all plaintext device metadata. This is structural, not by convention.
- **Discriminated-union message contract in the extension.** `shared/messages.ts` + the `POPUP_ONLY_TYPES` / `CONTENT_CALLABLE_TYPES` capability sets give every router handler typed dispatch with origin-aware gating. Content scripts make zero WASM calls and re-validate origin in `fill.ts:32-38`. The boundary discipline holds.
- **Test design across the codebase.** Synthetic JPEGs via `make_test_jpeg()` (no binary fixtures), raw-byte tar bombs that bypass the tar crate's sanitizer, RFC6238 vector for TOTP, an independent reference impl cross-check for Steam, ~30 LastPass importer cases, NFC/NFD round-trips, day-boundary tests for retention. The tests are themselves a documentation artifact.
- **Helpers.rs in the CLI.** Small, focused, tested, and the doc-comment at `:90-93` explaining the plaintext `groups.cache` trade-off is admirably explicit. The git-command hardening (`core.hooksPath=/dev/null`, `commit.gpgsign=false`, `core.editor=true`) is exactly the right kind of comment to leave for a learner.

## CLI/extension parity status

Per DEV-C's full table: **22 of 23 CLI subcommands have a clean extension equivalent**.

- **Clean parity (✓)**: `init`, `add` (all 7 item kinds), `get`, `list` (with all filters), `edit`, `history`, `rm` (soft delete), `restore`, `purge`, `trash list`/`empty`, `backup export`/`restore`, `import lastpass`, `attach`, `attachments`, `extract`, `generate`, `settings *`, `sync`, `lock`, `rate`, `device add`/`revoke`/list, `recovery-qr generate`/`unwrap`. The settings unification under v0.5.1's left-nav (commit `bd6a301`) is the right shape.
- **Partial (✗ish)**: `detach <item> <aid>`. Extension uses a roundtrip through `update_item` with the `attachments[]` mutated client-side. Functional but racy if two devices edit at once. **Suggested fix**: add a `delete_attachment` SW message that does the surgical remove server-side. (P3.)
- **True gap (✗)**: `relicario status`. CLI shows pending sync state, ahead/behind, dirty-tree summary; extension surfaces nothing comparable. **Suggested fix**: `get_vault_status` message returning `{ ahead, behind, lastSyncAt, pendingItems }` plus a small status indicator in the vault sidebar. (P2.)
- **Browser-only (by design)**: `get_autofill_candidates`, `get_credentials`, `check_credential`, `blacklist_site`, `capture_save_login`, `fill_credentials`, `ack_autofill_origin`, `get_blacklist`, `remove_blacklist`, `get_active_tab_url`, `update_settings` for `DeviceSettings`. No CLI counterpart needed.
- **CLI-only (by design)**: `completions <shell>`. n/a.

**No "CLI first, extension follow-up" violations found** under this lens. The parity philosophy is intact; the one gap and the one partial are surgical fixes, not architectural debt.

## Beginner-friendliness assessment

Three reviewers converge on a consistent story for a smart developer who's never written Rust but wants to learn by tinkering.

**Where the floor is high already:**
- `crates/relicario-core/` reads beautifully: bytes-in/bytes-out boundary holds, public API is enumerated in one `lib.rs`, module names map directly to vault concepts (`item`, `manifest`, `attachment`, `settings`, `generators`, `vault`, `backup`, `device`, `recovery_qr`), and the algorithmically dense `imgsecret.rs` is the most heavily commented. A reader can land in `crypto.rs`, follow `derive_master_key` → `encrypt` → `vault::encrypt_item`, and reach `tests/integration.rs::full_workflow_login_and_note` in one sitting.
- `relicario-server` is 189 lines, one trust-model question, every dependency is plaintext metadata. Readable end-to-end in under ten minutes.
- The extension's discriminated-union message contract (`shared/messages.ts` + `types.ts`) gives the entire vocabulary in 500 lines. The per-type item form modules (`popup/components/types/login.ts` and siblings) are small parallel surfaces — once one clicks, the others read as variations.

**Where the cliff is:**
- **`cli/main.rs`** at 2641 LOC is the first real wall. A reader following `cmd_add` finds 7 peer `build_*_item` functions plus 6 prompt helpers plus 3 parsers, all in a flat file. (P1.2.)
- **`extension/src/setup/setup.ts`** at 1220 LOC is the second wall, and it's worse because it lies about the architecture: a reader who opens this file first sees WASM imported directly and concludes that's the pattern — it isn't. (P1.4.)
- **`extension/src/vault/vault.ts`** at 1027 LOC is the third. The vault tab is the user's primary surface; the orchestrator file shouldn't also be the implementation file. (P1.5.)
- **`shared/state.ts`** is the weakest learning surface in the extension — `any`-typed bridge between popup and vault tab, no double-registration guard. (P1.6.)
- **`recovery_qr.rs`** in core is the one file where a reader will hit a wall in an otherwise-well-documented crate. (P1.7.)

**The single most valuable change** for the user's stated goal: split `cli/main.rs` into a `commands/` folder (P1.2). It's the first file a tinkerer opens, the change is mechanical, and it unlocks several smaller cleanups (centralized git error UX, unified groups-cache discipline). After that, in order: bring `recovery_qr.rs` to the existing core doc standard (P1.7), extract `setup.ts`'s WASM orchestration into SW messages (P1.4), and split `vault.ts` (P1.5). The four together turn "this is mostly readable" into "this is uniformly readable."

## Open architectural decisions (escalated by reviewers; user judgement)

Pulled from DEV-B's question list and DEV-C's flags. None are blockers for the synthesis; each is a one-paragraph user decision.

1. **Was `impl Drop for SessionHandle` deliberately omitted?** (e.g. to avoid double-free if JS holds two refs.) PM verdict at synthesis: not deliberate; it's the headline fix (P1.1). User: confirm.
2. **Should CLI `parse_month_year`, `base32_decode_lenient`, `guess_mime` migrate to core?** PM verdict: yes, for parity (P1.10). User: confirm timing.
3. **Is "missing `.relicario/devices.json` = bootstrap = accept" intended in perpetuity?** Or should it tighten once a repo has any non-empty devices.json in history? (DEV-B P2.) User: pick a rule.
4. **Is the `Lock` no-op CLI subcommand worth keeping visible in `--help` for parity?** Or hide behind `#[command(hide = true)]`? (DEV-B P3.) User: pick.
5. **Has Task 12 shipped?** `cmd_backup_export` still reads `devices.json` per a "Task 12 will remove" TODO at `main.rs:1535-1537`. (DEV-B P3.) User: confirm and clean up.
6. **Track `tools/relay/call.py` and `call.ts`, or `.gitignore` them?** They're load-bearing for the multi-agent fallback path. (DEV-C P2.) PM verdict: track them — they're documented in coordination prompts.
7. **WASM JS naming: snake_case (current) or camelCase?** Trivial breaking rename via `#[wasm_bindgen(js_name = ...)]`, but only if done before the surface grows. (DEV-B/DEV-C P3.) User: pick once.
8. **`.gitea_env_vars` is untracked.** Name suggests local credentials. PM verdict: should be `.gitignore`d if it isn't already. User: confirm.

## Appendix: pointers to per-reviewer notes

- [DEV-A notes — Rust core](./2026-05-04-dev-a-notes.md)
- [DEV-B notes — Rust consumers (CLI, server, WASM)](./2026-05-04-dev-b-notes.md)
- [DEV-C notes — TypeScript (extension + relay)](./2026-05-04-dev-c-notes.md)