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 " 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 .gitignored if it isn't already. User: confirm.

Appendix: pointers to per-reviewer notes

• DEV-A notes — Rust core
• DEV-B notes — Rust consumers (CLI, server, WASM)
• DEV-C notes — TypeScript (extension + relay)