alee/relicario
1
0
Fork 0
Code Issues Pull Requests 4 Actions Packages Projects Releases Wiki Activity

docs(specs): three architecture-review followup plans (security/CLI/extension)

Browse Source 
Plan A (security & docs polish, S): SessionHandle impl Drop + JS .free()
audit + recovery_qr.rs documentation + relay launcher dev-c expansion.
Independent of B/C; ships first.

Plan B (CLI restructure, M-L): split cli/main.rs (2641 LOC) into commands/
folder + prompt.rs + parse.rs; helpers::git_run captures stderr; Vault::
after_manifest_change centralizes the groups-cache discipline; canonical
ParamsFile; batched purge; migrate parse_month_year/base32_decode_lenient/
guess_mime to relicario-core with WASM re-exports.

Plan C (extension restructure, L): typed StateHost (precondition); extract
service-worker/storage.ts; setup.ts SW migration via create_vault/
attach_vault messages + step-registry pattern; vault.ts split into
shell/sidebar/list/drawer/form-wrapper with vault_locked channel
unified through shared/state.ts; P2 cluster (timer reset, gitHost clear,
teardown helper, allSettled, MutationObserver debounce); get_vault_status
closes the relicario status parity gap.

Cross-boundary cites verified: Plan B Phase 8 WASM exports are the seam
Plan C consumes (deferred to a future plan); Plan A owns the .free() swallow
removal that Plan C respects without redoing.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This commit is contained in:
adlee-was-taken
2026-05-05 20:02:40 -04:00
parent 4a726c2631
commit 4f7ab91f14
3 changed files with 718 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files

215
docs/superpowers/specs/2026-05-04-cli-restructure-design.md Normal file
View File

@@ -0,0 +1,215 @@
# CLI Restructure — Design
**Date:** 2026-05-04
**Status:** Proposed
**Source:** `docs/superpowers/reviews/2026-05-04-architecture-review.md` (P1.2, P1.3, P1.10) + folded P2s from DEV-B's CLI section + DEV-A's P2 base32 dedup
**Effort estimate:** M-L
## Summary
Plan B is the single biggest readability lift in the whole-codebase architecture review. `crates/relicario-cli/src/main.rs` is a 2641-line flat file: every `cmd_*`, every `build_*_item`, every `edit_*`, six prompt helpers, three pure parsers, the `ParamsFile` writer, and 24+ git shell-outs all live as peers. This plan splits that file into a `commands/` folder plus `prompt.rs` and `parse.rs`, then builds on top of that split to centralize the duplicated git-error UX, unify the manifest-after-mutation cache discipline, deduplicate the on-disk `params.json` shape, batch the `cmd_purge` git invocations, and finally migrate the pure parsers (and a third copy of base32) into `relicario-core` so the extension can consume them through new WASM exports. After the file is navigable rather than scrollable, a tinkerer who opens `cmd_add` can follow it end-to-end in one screenful — which is the user's stated learning-by-tinkering goal.
## Findings addressed
- **P1.2** — `crates/relicario-cli/src/main.rs:1-2641` is a 2641-line monolith with no submodule boundaries (DEV-B). The clap surface (lines 1-455) is excellent; lines 456-2641 are flat code.
- **P1.3** — Git invocation boilerplate duplicated ~16× with one-line `bail!("git X failed")` errors at `crates/relicario-cli/src/main.rs:601, 602, 610, 986, 988, 1477, 1480, 1767, 1897, 1900, 2432, 2438, 2533, 2540` (and others) (DEV-B).
- **P1.10** — Pure parsers (`parse_month_year`, `base32_decode_lenient`, `guess_mime`) live only in the CLI at `crates/relicario-cli/src/main.rs:942-980` despite the extension needing them (DEV-B). Pairs with DEV-A's P2 finding that three base32 implementations live in core (`crates/relicario-core/src/item.rs:255-275`, `crates/relicario-core/src/import_lastpass.rs:202-220`, and intentionally separate Steam at `crates/relicario-core/src/item_types/totp.rs:13`).
- **CLI P2 — `build_*_item` complexity** at `crates/relicario-cli/src/main.rs:664-921` (DEV-B). Each builder mixes prompting, parsing, and core construction.
- **CLI P2 — `refresh_groups_cache` discipline** at `crates/relicario-cli/src/main.rs:641, 998, 1123, 1197, 1414, 1432, 1474` (DEV-B). The "every mutating handler must call this" rule is unenforced.
- **CLI P2 — `ParamsFile` defined twice with mismatching shapes** at `crates/relicario-cli/src/main.rs:2287` (writer with `aead`, `salt_path`, `format_version`) versus `crates/relicario-cli/src/session.rs:114` (reader with only `kdf`) (DEV-B).
- **CLI P2 — Batched purge needed** at `crates/relicario-cli/src/main.rs:1476-1480` and `:1896-1900` (DEV-B). A 50-item `trash empty` currently fires 150 git invocations.
## Approach
The architectural shape is "one file per top-level subcommand, plus two cross-cutting helpers, plus a thin dispatcher." The clap definitions stay at the top of `main.rs` because they read like a tour of the product and that's exactly what a learner wants to land on first; everything past the dispatcher moves out. Each new module becomes self-contained enough that a tinkerer following a single command does not need to scroll through neighbours.
Post-split layout under `crates/relicario-cli/src/`:
```
src/
├── main.rs # clap surface (Cli, Commands, AddKind, …) + dispatch match (~470 lines)
├── helpers.rs # vault_dir, git_command, git_run (NEW), iso8601, humanize_age,
│ # groups_cache_path, sanitize_for_commit, decode_totp_qr
├── session.rs # UnlockedVault, ParamsFile (single canonical), after_manifest_change (NEW)
├── device.rs # ed25519 keypair storage, configure_git_signing
├── gitea.rs # GiteaClient (deploy-key API)
├── prompt.rs # NEW — prompt, prompt_optional, prompt_secret, prompt_or_flag<T>,
│ # and the four other small prompt_* helpers
├── parse.rs # NEW — thin wrappers around the migrated core parsers
│ # (until phase 7, the parsers themselves live here)
└── commands/
├── mod.rs # NEW — pub use re-exports for the dispatcher
├── add.rs # cmd_add + the seven build_*_item helpers
├── get.rs # cmd_get
├── list.rs # cmd_list, cmd_history, resolve_query
├── edit.rs # cmd_edit + per-type edit_* helpers
├── trash.rs # cmd_rm, cmd_restore, cmd_purge, cmd_trash, cmd_trash_empty,
│ # purge_item
├── backup.rs # cmd_backup, cmd_backup_export, cmd_backup_restore
├── import.rs # cmd_import (LastPass)
├── attach.rs # cmd_attach, cmd_attachments, cmd_extract, cmd_detach
├── settings.rs # cmd_settings (and its subaction handlers)
├── sync.rs # cmd_sync (push/pull/fetch)
├── status.rs # cmd_status (vault state summary)
├── device.rs # cmd_device + register/revoke/list, load_gitea_client
├── recovery_qr.rs # cmd_recovery_qr, cmd_recovery_qr_generate, cmd_recovery_qr_unwrap
├── init.rs # cmd_init (also writes ParamsFile via session module)
├── generate.rs # cmd_generate
└── rate.rs # cmd_rate
```
Rationale for the boundaries:
- **One file per top-level subcommand** is the navigation invariant: `relicario add` ⇒ `commands/add.rs`. `cmd_add` and the seven `build_*_item` helpers it calls are the ~260-line vertical slice a tinkerer wants in front of them at once. Same shape for every other subcommand.
- **`prompt.rs` is the input layer**: every command that reads from the user funnels through one module, so swapping in a different prompt strategy (richer TTY widgets, scripted-test mode) is a one-file change. The `prompt_or_flag<T>` helper introduced in phase 3 lives here.
- **`parse.rs` is the validation layer** during phases 1–6 — it owns `parse_month_year`, `base32_decode_lenient`, `guess_mime`. In phase 7 the bodies move to `relicario-core` and `parse.rs` becomes a thin re-export so callers don't have to change imports twice.
- **`helpers.rs` keeps `git_command` (the hardened `Command` builder) and gains `git_run`** (the bail-on-failure wrapper). Both are flat utilities; they belong with `vault_dir` and `iso8601`.
- **`session.rs` keeps `UnlockedVault` and absorbs the canonical `ParamsFile`** so the on-disk shape has one definition, not two. The `after_manifest_change` wrapper introduced in phase 4 also lives here, because it's logically a session method.
- **`commands/mod.rs`** re-exports each `cmd_*` so the dispatch match in `main.rs` reads as `Commands::Add { kind } => commands::add::cmd_add(kind)` — the dispatch surface still fits on one screen.
Boundary discipline: every `cmd_*` becomes `pub fn` in its module file; every `build_*_item` and `edit_*` stays `pub(super)` (only its sibling dispatcher needs it). Helpers shared across `commands/` (e.g. `commit_paths`, `resolve_query`) live in `commands/mod.rs` as `pub(crate)`. The compile error if a sibling moves and a caller forgets the visibility bump is exactly the safety net we want.
**WASM/extension parity seam (P1.10).** The three CLI parsers move into `relicario-core` in phase 7, get re-exported as `#[wasm_bindgen]` functions in phase 8, and the `extension/src/wasm.d.ts` file is updated in the same commit (per DEV-C's boundary note, that file is hand-maintained — every export change must be mirrored manually). Plan C (extension restructure) then wires SW message handlers that call those WASM exports; **the SW handlers themselves are not designed in this plan** — phase 8 only delivers the seam Plan C consumes.
## Implementation phases
### Phase 1 — Mechanical split of `main.rs`
- **Goal:** turn 2641 LOC of flat code into a navigable directory tree without changing any behaviour.
- **Changes:**
- New: `crates/relicario-cli/src/commands/{mod,add,get,list,edit,trash,backup,import,attach,settings,sync,status,device,recovery_qr,init,generate,rate}.rs`.
- New: `crates/relicario-cli/src/prompt.rs` (lifts `prompt`, `prompt_optional`, `prompt_secret` from `main.rs:508-940`).
- New: `crates/relicario-cli/src/parse.rs` (lifts `parse_month_year`, `base32_decode_lenient`, `guess_mime` from `main.rs:942-980`).
- Modified: `crates/relicario-cli/src/main.rs` — keeps `mod` declarations, the clap `Cli`/`Commands`/`AddKind` enums (lines 1-455), the dispatch `match` (lines 405-454), the three `test_*_override()` shims (lines 475-503), and the `refresh_groups_cache` shim (lines 457-473) until phase 4 collapses it.
- Modified: every other file that referenced `crate::session::UnlockedVault::unlock_interactive`, `crate::helpers::*`, `crate::test_passphrase_override` keeps working unchanged (visibility bumps only).
- **Tests:** the existing CLI integration tests at `crates/relicario-cli/tests/{basic_flows,attachments,backup,edit_and_history,settings,smart_inputs,vault_detection,import_lastpass}.rs` are the regression budget. They test from the binary surface (via `assert_cmd`), so a pure relocation must not change a single assertion. Run between every sub-step. No new tests in this phase; logic is unchanged.
- **Effort:** M (largest single phase by LOC moved; mechanical).
- **Depends on:** none.
### Phase 2 — `helpers::git_run` and the 16-site sweep
- **Goal:** replace 16 `bail!("git X failed")` one-liners with `git_run(repo, args, context)?`, so a failing git subprocess prints actionable stderr instead of just the verb.
- **Changes:**
- Modified: `crates/relicario-cli/src/helpers.rs` — adds `pub fn git_run(repo: &Path, args: &[&str], context: &str) -> Result<()>` that uses `.output()` (capturing stderr), prints the captured stderr unmodified to the user's stderr on failure, and bails with `"<context>: git failed (status N)"`. Keeps `git_command` for the rare interactive callsite that needs a `Command` (phase decides per call).
- Modified: `crates/relicario-cli/src/commands/init.rs` — three sites collapse (`main.rs:601, 602, 610` post-split). Contexts: `"init: git init"`, `"init: git add"`, `"init: git commit"`.
- Modified: `crates/relicario-cli/src/commands/add.rs` — two sites in `commit_paths` (`main.rs:986, 988` post-split moves to `commands/mod.rs`). Context: `"add: git add <id>"`, `"add: git commit"`.
- Modified: `crates/relicario-cli/src/commands/trash.rs` — `cmd_purge` and `cmd_trash_empty` sites (`main.rs:1477, 1480, 1897, 1900` post-split). Contexts include the item title for trace clarity. (Phase 6 batches them further.)
- Modified: `crates/relicario-cli/src/commands/edit.rs` — `main.rs:1767` site post-split. Context: `"edit: git commit"`.
- Modified: `crates/relicario-cli/src/commands/sync.rs` — `main.rs:2432, 2438, 2533, 2540` sites post-split. Contexts: `"sync: git fetch"`, `"sync: git pull"`, `"sync: git push"`.
- Modified: any other `git_command(...).status()? + bail!` site identified by grep during phase 1 (DEV-B's "and others" list).
- **Tests:** add a unit test in `crates/relicario-cli/src/helpers.rs`'s `mod tests` that invokes `git_run` against a deliberately-failing git subcommand in a `tempfile::TempDir` and asserts both that the bail message contains the context string and that the captured stderr is reproduced. Existing integration tests must still pass.
- **Effort:** S.
- **Depends on:** Phase 1.
### Phase 3 — `prompt_or_flag<T>` and `build_*_item` compression
- **Goal:** collapse the seven `build_*_item` builders so each one takes already-validated values, with a single `prompt_or_flag<T>` helper handling the "use this flag if Some, otherwise prompt" pattern.
- **Changes:**
- Modified: `crates/relicario-cli/src/prompt.rs` — adds `pub fn prompt_or_flag<T>(flag: Option<T>, label: &str, parser: impl FnOnce(&str) -> Result<T>) -> Result<T>` and a `prompt_or_flag_optional<T>` sibling for fields where the empty-string case maps to `None`.
- Modified: `crates/relicario-cli/src/commands/add.rs` — every `build_*_item` (post-split locations of `main.rs:664-921`) drops its `Option<T>::map(Ok).unwrap_or_else(|| prompt(...))?` chains in favour of `prompt_or_flag(title, "Title", |s| Ok(s.into()))?` etc. Per-type bodies shrink by ~30%.
- **Tests:** existing `tests/basic_flows.rs::add_login_then_list_shows_it`, `tests/smart_inputs.rs` cover the prompt path; `tests/attachments.rs` covers the document builder. Add one focused test: `prompt_or_flag_uses_flag_value_when_some` and `prompt_or_flag_prompts_when_none` (synthetic stdin via `Cursor`).
- **Effort:** S-M.
- **Depends on:** Phase 1.
### Phase 4 — `Vault::after_manifest_change` and the seven-site sweep
- **Goal:** make "every mutating handler must call `refresh_groups_cache`" a compile-time invariant rather than a discipline rule.
- **Changes:**
- Modified: `crates/relicario-cli/src/session.rs` — adds `pub fn after_manifest_change(&self, manifest: &Manifest) -> Result<()>` that calls `self.save_manifest(manifest)?` and then writes the groups cache. Marks the existing `save_manifest` as `pub(crate)` (or renames to `save_manifest_raw` and makes it `pub(crate)`) so callers funnel through the wrapper. Cache writing logic moves out of `main.rs:457-473` into this method.
- Modified: `crates/relicario-cli/src/commands/{add,edit,trash,attach,settings,import}.rs` — the seven sites (`main.rs:641, 998, 1123, 1197, 1414, 1432, 1474` post-split) collapse from `vault.save_manifest(&manifest)?; refresh_groups_cache(vault.root(), &manifest);` to `vault.after_manifest_change(&manifest)?;`.
- Removed: the standalone `refresh_groups_cache` function in `main.rs:463`. The cache-write closure inside `after_manifest_change` keeps the existing "failures are silently swallowed" semantics (preserve the `let _ =` and the doc comment from `main.rs:457-462`).
- **Tests:** `tests/smart_inputs.rs::groups_cache_*` already exercises the write/suppress paths from the binary surface; rerun. Add a unit test in `session.rs` confirming `after_manifest_change` writes both the manifest and the cache file.
- **Effort:** S.
- **Depends on:** Phase 1.
### Phase 5 — Single canonical `ParamsFile`
- **Goal:** one definition of the on-disk `params.json` shape, used by both the `init` writer and the `unlock_interactive` reader.
- **Changes:**
- Modified: `crates/relicario-cli/src/session.rs` — promotes the inner `ParamsFile` struct (`session.rs:114`) to a module-level `pub(crate) struct ParamsFile { pub format_version: u32, pub kdf: KdfParams, pub aead: String, pub salt_path: String }`. Adds `Serialize` + `Deserialize` derives. Provides constructors: `ParamsFile::for_new_vault(params: &KdfParams) -> Self` and inversion `pub fn into_kdf_params(self) -> KdfParams`. Verifies field-rename compatibility against the existing on-disk JSON (read a temp `params.json` written by current `main.rs` to confirm round-trip).
- Modified: `crates/relicario-cli/src/commands/init.rs` — replaces the inline `ParamsFile`/`ParamsKdf` structs at the post-split equivalent of `main.rs:2287-2301` with `session::ParamsFile::for_new_vault(&params)`. Removes the duplicate writer-side definition.
- Decision note in the module doc-comment: keep `ParamsFile` in `session.rs` rather than `relicario-core`. Rationale: the struct describes a *file on disk*, and `relicario-core` is bytes-in/bytes-out (no filesystem). The `KdfParams` value inside is core's already; the envelope is the CLI's I/O concern.
- **Tests:** add `tests/common/mod.rs` (or extend an existing test) round-trip: write via `ParamsFile::for_new_vault`, parse via `read_params`, confirm the resulting `KdfParams` matches the input. The on-disk schema must not change (`tests/basic_flows.rs::init_creates_expected_layout` indirectly covers this).
- **Effort:** S.
- **Depends on:** Phase 1.
### Phase 6 — Batched purge in `cmd_purge` and `cmd_trash_empty`
- **Goal:** a 50-item `trash empty` should fire 3 git invocations total, not 150.
- **Changes:**
- Modified: `crates/relicario-cli/src/commands/trash.rs` — `purge_item` (post-split of `main.rs:1450-1462`) is renamed `purge_item_filesystem` and **only** mutates the working tree + manifest (filesystem `remove_file`/`remove_dir_all`, manifest `remove`). It does **not** invoke `git rm`. Returns `Vec<String>` of the paths it removed (so the caller can stage them).
- `cmd_purge` (post-split `main.rs:1464-1482`): collects the paths from `purge_item_filesystem`, calls `vault.after_manifest_change(&manifest)?`, then a single `git_run(vault.root(), &["rm", "-rf", "--ignore-unmatch", … paths …, "manifest.enc"], "purge: git rm")?` followed by `git_run(... ["add", "manifest.enc"], "purge: git add")?` (manifest is staged separately because it was rewritten not removed) and `git_run(... ["commit", "-m", &msg], "purge: git commit")?`. Three invocations, fixed.
- `cmd_trash_empty` (post-split `main.rs:1885-1903`): same pattern over the loop of purgeables — accumulate all paths, then one `git rm`, one `git add manifest.enc`, one `git commit`. The commit message keeps `"trash empty: purged N item(s)"`.
- **Tests:** existing `tests/basic_flows.rs::rm_restore_purge_cycle` covers the single-item purge contract. Add `tests/basic_flows.rs::trash_empty_batches_git_invocations` (or extend `tests/settings.rs`) — purge ≥3 items via `trash empty`, then assert via `git log --oneline --since=…` that exactly **one** new commit appeared (a strict invariant that catches accidental re-introduction of per-item commits). Synthetic fixtures only; no binary blobs.
- **Effort:** S-M.
- **Depends on:** Phases 1, 2, 4 (uses `git_run` and `after_manifest_change`).
### Phase 7 — Migrate parsers to `relicario-core` + `pub(crate) mod base32`
- **Goal:** close DEV-A's three-base32-implementations finding and DEV-B's parsers-in-CLI-only finding in one stroke. The CLI keeps thin wrappers (no caller-side import changes); core becomes the single source of truth.
- **Changes:**
- Modified: `crates/relicario-core/src/time.rs` — adds `impl MonthYear { pub fn parse(s: &str) -> Result<Self, RelicarioError> { … } }` accepting `MM/YYYY`, `MM-YYYY`, and `MM/YY` (lifts the body of `parse_month_year` from `crates/relicario-cli/src/parse.rs`). Note: `MonthYear::new` currently returns `Result<_, &'static str>` (DEV-A P3); this phase has the option to either bring `new` to `RelicarioError` for consistency, or leave it and have `parse` call `new` and re-wrap. Recommendation: re-wrap only — the `new` migration is DEV-A's P3 to keep this plan focused.
- New: `crates/relicario-core/src/base32.rs` — `pub(crate) mod base32 { pub fn encode_rfc4648(bytes: &[u8]) -> String; pub fn decode_rfc4648_lenient(s: &str) -> Result<Vec<u8>, RelicarioError>; }`. Body lifted/unified from `crates/relicario-core/src/item.rs:255-275` (the encoder) and `crates/relicario-core/src/import_lastpass.rs:202-220` (the decoder), plus the lenient input handling (case-insensitive, padding-optional, whitespace-stripped) from `crates/relicario-cli/src/parse.rs`. Steam's `STEAM_ALPHABET` at `crates/relicario-core/src/item_types/totp.rs:13` stays put with a neighbour comment: `// not RFC 4648 — Steam Guard's de-ambiguated alphabet; see crate::base32 for the standard impl.`
- Modified: `crates/relicario-core/src/item.rs` — removes the inline `base32_encode` (lines 255-275); call sites switch to `crate::base32::encode_rfc4648`.
- Modified: `crates/relicario-core/src/import_lastpass.rs` — removes the inline `decode_base32_totp` (lines 202-220); call sites switch to `crate::base32::decode_rfc4648_lenient`.
- New: `crates/relicario-core/src/item_types/totp.rs` — adds `impl TotpConfig { pub fn parse_secret(s: &str) -> Result<Zeroizing<Vec<u8>>, RelicarioError> { Ok(Zeroizing::new(crate::base32::decode_rfc4648_lenient(s)?)) } }`. The `Zeroizing` wrap is the value-add over a raw base32 call.
- New: `crates/relicario-core/src/mime.rs` — `pub fn guess_for_extension(filename: &str) -> &'static str` (lifts `guess_mime` from CLI). Default `application/octet-stream`. The match is small enough that exhaustive enum tagging (PDF/PNG/JPEG/TXT/JSON) is overkill; keep the string-match shape.
- Modified: `crates/relicario-core/src/lib.rs` — re-exports `MonthYear` (already exported), `mime`, `base32` (`pub(crate)` only, not in the public API surface). `Totp::parse_secret` is reachable via `ItemCore::Totp(_).parse_secret(...)` already.
- Modified: `crates/relicario-cli/src/parse.rs` — becomes a thin shim re-exporting the new core API. `pub fn parse_month_year(s: &str) -> Result<MonthYear, RelicarioError> { MonthYear::parse(s) }` (so the existing CLI callsites need zero edits in this phase). Same for `base32_decode_lenient` and `guess_mime`. Once Plan C lands and consumers have all migrated, a follow-up plan can delete `parse.rs` entirely.
- Verification step: before phase 7, grep `grep -RIn "parse_month_year\|base32_decode_lenient\|guess_mime\|base32_encode\|decode_base32_totp" crates/ extension/` to confirm the only consumers are the CLI files about to be touched. The Cargo workspace currently has no other consumer.
- **Tests:** move CLI's existing parser unit tests (if any) into `crates/relicario-core/src/`. Add: `MonthYear::parse` round-trip for `01/2026`, `12/30`, `12-2026`, plus error cases (`13/2026`, `01/1999`, `garbage`); `base32::encode_rfc4648`/`decode_rfc4648_lenient` round-trip on a known TOTP vector (the RFC 6238 test vector already lives in `item_types/totp.rs`); `mime::guess_for_extension` table — `pdf.PDF`, `image.jpg`, `image.jpeg`, `unknown.xyz`. Keep `tests/import_lastpass.rs` green (it'll go through the new shared decoder). Existing CLI integration tests at `crates/relicario-cli/tests/*` must still pass; the public CLI surface is unchanged.
- **Effort:** M.
- **Depends on:** Phase 1.
### Phase 8 — WASM exports for the migrated parsers + wasm.d.ts mirror
- **Goal:** make the new core parsers reachable from the extension via the SW. Deliver only the seam; Plan C wires the SW handlers.
- **Changes:**
- Modified: `crates/relicario-wasm/src/lib.rs` — adds three `#[wasm_bindgen]` exports:
- `pub fn parse_month_year(s: &str) -> Result<JsValue, JsError>` returning the serialized `MonthYear` (`{ month, year }` plain object via the existing `Serializer::new().serialize_maps_as_objects(true)` pattern; reuse `js_value_for`).
- `pub fn base32_decode_lenient(s: &str) -> Result<Vec<u8>, JsError>` returning the decoded bytes as a `Uint8Array` on the JS side (wasm-bindgen converts `Vec<u8>` into a `Uint8Array` copy — same convention as `attachment_decrypt`).
- `pub fn guess_mime(filename: &str) -> String`.
- Modified: `extension/src/wasm.d.ts` — adds the three matching declarations under the existing `declare module 'relicario-wasm'` block. Per DEV-C's boundary note, this file is hand-maintained; this commit must update both files together. Suggested placement: directly after `extract_image_secret`/`embed_image_secret` (line ~60), grouped under a `// Pure parsers (no session needed)` comment so the extension-side reader sees they require no `SessionHandle`.
- Naming convention call-out: phase 8 keeps **snake_case** JS names (consistent with every existing export, e.g. `manifest_encrypt`, `extract_image_secret`). The snake_case → camelCase decision (DEV-B/DEV-C P3) is **explicitly deferred to a separate plan**; introducing camelCase only for the three new exports would create a worst-of-both-worlds inconsistency.
- **Tests:** add `wasm-bindgen-test` cases (or extend the existing `session_tests` mod at `crates/relicario-wasm/src/lib.rs:522-591`) covering the three new exports — at minimum, a round-trip for `base32_decode_lenient` against a known input, an error case for `parse_month_year("not-a-date")`, and a `guess_mime("doc.pdf") == "application/pdf"` smoke. Confirm `cargo build -p relicario-wasm --target wasm32-unknown-unknown` is clean.
- **Cross-boundary cite:** **after Phase 8 ships, Plan C can wire SW message handlers** for `parse_month_year`, `base32_decode_lenient`, and `guess_mime` (e.g. messages `parse_month_year`, `decode_totp_secret`, `guess_attachment_mime` in `extension/src/shared/messages.ts`, dispatched by `extension/src/service-worker/router/popup-only.ts`). This plan does **not** design those handlers; phase 8 only delivers the WASM seam Plan C consumes. Coordinate the wasm.d.ts update commit with Plan C so both crates' callers see the new surface in the same merge.
- **Effort:** S.
- **Depends on:** Phase 7.
## Risks and mitigations
- **Mechanical splits drift if a function calls a sibling that moved.** Mitigation: `cargo check -p relicario-cli` between every sub-step of phase 1 (i.e. between every file-extraction); explicit `pub(crate)` discipline so the compiler enforces visibility. The integration tests at `crates/relicario-cli/tests/*` are the regression budget — they exercise the binary surface, not internal modules, so a pure relocation cannot break them silently.
- **`helpers::git_run` semantic change: switching from `.status()` (inherited stderr to TTY) to `.output()` (captured) means interactive flows lose live stderr streaming.** A user who runs `relicario sync` in a terminal sees git's progress today; with `git_run` they see only the captured chunk on failure. Mitigation options: (a) `git_run` could keep `inherit` semantics by detecting `stderr.is_terminal()` and switching strategy; (b) accept the trade — captured-and-printed-on-failure is uniformly better for the CI/test/scripted-tooling case which is the failure-mode that matters. Recommendation: option (b) for phase 2, with a follow-up TODO if the live-streaming loss is reported. Keep `git_command` available for the rare site that needs a manual `Command` (pipe/stdin/etc.) so neither contract has to be perfect.
- **`Vault::after_manifest_change` adds a method to the session module; risk that callers forget to use the wrapper.** Mitigation: in phase 4, mark `save_manifest` as `pub(crate)` (or rename to `save_manifest_raw`) so all `commands/*` files are forced through `after_manifest_change`. The clap dispatcher calls only `cmd_*` functions, none of which need the raw method.
- **`ParamsFile` migration is on-disk-format-sensitive.** A change in the `Serialize` derive's field order or a missed `Deserialize` field tolerance would break existing vaults. Mitigation: phase 5's test round-trip reads a `params.json` written by the current code (committed as a fixture *string literal* in the test, not a binary blob) and confirms the new code parses it identically. Field names match exactly (`format_version`, `kdf`, `aead`, `salt_path`); the existing `read_params` only reads `kdf`, so adding tolerance for the other fields is the natural step.
- **Parser migration to core changes the public API surface of `relicario-core`.** Any in-flight work consuming the old CLI helpers must adapt. Mitigation: the grep step at the top of phase 7 confirms zero non-CLI consumers exist today; the CLI's `parse.rs` shim keeps callsites unchanged through phases 1–6, so this is a one-pass migration with no follow-up required from other plans. The Steam alphabet stays untouched (intentional non-RFC-4648 alphabet) with a neighbour comment.
- **WASM export rename / addition could break the extension at the `wasm.d.ts` boundary.** Per DEV-C's boundary notes, `extension/src/wasm.d.ts` is hand-maintained; every change to `crates/relicario-wasm/src/lib.rs`'s `#[wasm_bindgen]` surface must be mirrored manually. Mitigation: phase 8 updates both files in the same commit; a future CI guard (DEV-C's WASM P2 — comparing wasm-pack–generated `.d.ts` against the hand-maintained one) is out of scope here but would close this hole permanently. The boundary notes also flag the BigInt typing care that `attachment_encrypt` already requires (DEV-B item 3 in DEV-C's boundary notes); the three new exports take only `&str` and return primitives, so they avoid that class of footgun.
- **Cross-plan coupling.** Phase 8 lands a wasm.d.ts change; Plan C will land SW handlers consuming it. If the merge order interleaves — e.g. Plan C ships its handler stub before phase 8 lands the WASM export — the extension build breaks. Mitigation: phase 8 ships first; Plan C's SW-handler phase explicitly depends on phase 8's WASM exports (cite this seam in Plan C). The user's release-train coordination is the enforcement mechanism.
## Out of scope
- **Plan A** (security/docs polish) and **Plan C** (extension restructure) entirely.
- **All CLI P3 nits** that DEV-B enumerates: `let _ = entry;` pattern (`main.rs:1170, 1407, 1426, 1469, 1913, 2030`); `Lock` subcommand visibility; `Display` for `ItemType` (the `format!("{:?}", e.r#type)` site at `main.rs:1158`); `helpers::relicario_dir` adoption sweep; `gitea::GiteaClient` per-call construction; scripted-prompt test layer for `tests/edit_and_history.rs`; dead `blob_path` variable in `tests/attachments.rs:69-76`; the three-test-env-var macro consolidation; `cmd_recovery_qr_unwrap` empty-input check; `cmd_recovery_qr_generate` stdout/stderr split; the Task-12 `cmd_backup_export` cleanup. Each is a one-line edit appropriate for a follow-up sweep, not for this M-L plan.
- **Server findings** (P2/P3 in DEV-B's relicario-server section): `generate-hook` `$PATH`, bootstrap-branch tightening, `verify-commit --from-stdin`, server test coverage gaps. These belong in a server-focused plan.
- **WASM findings beyond the parser exports needed for P1.10**: DEV-B's WASM P2 list (`need_key`/`with(...).unwrap()` double-lookup, `Vec<u8>` getter clones, `wasm_*_recovery_qr` rename, `device.rs`/`session.rs` concurrency-primitive split). Not in this plan.
- **The 8 "Open architectural decisions"** in the synthesis appendix.
- **WASM JS-naming** (snake_case → camelCase) — defer to a separate plan, as called out in phase 8.
- **In-flight uncommitted v0.5.x work** (`extension/src/vault/vault.ts`, `vault.css`, `glyphs.ts`, manifest version bumps, relay `queue.ts`/`server.ts`/`start.sh`). Treat as in-flight; this plan touches none of them.
## Done criteria
A reviewer can confirm Plan B has shipped by checking, in order:
- [ ] `crates/relicario-cli/src/main.rs` is ≤ 500 LOC and contains only the clap surface, the dispatch `match`, and the `mod` declarations + `test_*_override` shims.
- [ ] The `crates/relicario-cli/src/commands/` directory contains the 17 files listed in the post-split tree, each owning exactly one top-level subcommand or a tightly-scoped helper.
- [ ] `crates/relicario-cli/src/prompt.rs` and `crates/relicario-cli/src/parse.rs` exist, with the 6 prompt helpers and the 3 parser shims respectively.
- [ ] `cargo test --workspace` passes (CLI integration tests, core unit tests, server tests, wasm `wasm-bindgen-test`s).
- [ ] `cargo clippy --workspace` is silent.
- [ ] `cargo build -p relicario-wasm --target wasm32-unknown-unknown` is clean.
- [ ] The 16 `bail!("git X failed")` sites listed in P1.3 (and any others surfaced by grep) are now `git_run(repo, args, "<context>")?` one-liners. `grep -n 'bail!("git ' crates/relicario-cli/src/` returns zero matches.
- [ ] The 7 `refresh_groups_cache` callsites all collapse to `vault.after_manifest_change(&manifest)?`. `grep -n 'refresh_groups_cache' crates/relicario-cli/src/` returns zero matches outside `session.rs`.
- [ ] One canonical `ParamsFile` definition in `crates/relicario-cli/src/session.rs`. `grep -n 'struct ParamsFile' crates/relicario-cli/src/` returns one match.
- [ ] A 50-item `relicario trash empty` produces exactly **one** new git commit (verified by `git log --oneline` in the test); a single-item `relicario purge` produces exactly one commit. Per-purge git invocations: 3 (rm + add manifest + commit), down from 3-per-item.
- [ ] `MonthYear::parse`, `Totp::parse_secret`, `mime::guess_for_extension` exist in `relicario-core`. `crates/relicario-core/src/base32.rs` is `pub(crate)` and is the only RFC-4648 implementation in the crate (Steam alphabet at `item_types/totp.rs:13` stays, with a neighbour comment).
- [ ] `crates/relicario-wasm/src/lib.rs` exports `parse_month_year`, `base32_decode_lenient`, `guess_mime`. `extension/src/wasm.d.ts` mirrors the three declarations.
- [ ] All existing CLI integration tests at `crates/relicario-cli/tests/*` still pass without modification (regression budget held).

368
docs/superpowers/specs/2026-05-04-extension-restructure-design.md Normal file
View File

@@ -0,0 +1,368 @@
# Extension Restructure — Design
**Date:** 2026-05-04
**Status:** Proposed
**Source:** docs/superpowers/reviews/2026-05-04-architecture-review.md (P1.4, P1.5, P1.6, P1.9) + folded P2s from DEV-C's extension sections + the `relicario status` parity gap
**Effort estimate:** L
## Summary
This is the largest of the three architecture-review followups — multi-day to multi-week — and it eliminates the two steepest learning cliffs in the extension. After this plan ships, a tinkerer who opens `extension/src/setup/setup.ts` no longer sees WASM imported directly (it isn't the pattern; it was the exception); a tinkerer who opens `extension/src/vault/vault.ts` sees ~200 LOC of routing and state instead of 1027 LOC of shell + sidebar + list + drawer + form-wrapper inlined together; the `shared/state.ts` bridge between popup and vault tab becomes type-checked end to end; and the duplicated service-worker router helpers consolidate into one home each. As a side effect, the extension closes its last CLI-parity gap (`relicario status` → vault-sidebar status indicator).
## Findings addressed
- **P1.4** (DEV-C; `extension/src/setup/setup.ts:28-37`, `:1118-1120`, whole 1220-LOC file) — setup wizard imports `relicario-wasm` directly and orchestrates `unlock` / `embed_image_secret` / `register_device` / `manifest_encrypt` itself, bypassing the SW abstraction every other surface uses; ~400 LOC of crypto orchestration duplicated.
- **P1.5** (DEV-C; `extension/src/vault/vault.ts:1-1027`) — single 1027-LOC file owns shell + hash routing + sidebar + list + drawer + type-picker + form-wrapper + deep-link routing + teardown. Contains the `vault_locked` RPC intercept (lines 47-74) and the drawer-state leak (lines 495-536, 648-695).
- **P1.6** (DEV-C; `extension/src/shared/state.ts:10-35`) — `StateHost` contract is fully `any`-typed; `host` singleton has no double-registration guard.
- **P1.9** (DEV-C; `extension/src/service-worker/router/popup-only.ts:687-703`, `:~169`; `extension/src/service-worker/router/content-callable.ts:187-205`, `:~169`) — `loadDeviceSettings`, `loadBlacklist`, `saveBlacklist`, and `itemToManifestEntry` duplicated across both router files.
- **P2 — inactivity-timer reset on content-callable messages** (DEV-C; `extension/src/service-worker/index.ts:76-78`) — active autofiller never opens popup, gets force-locked despite continuous use.
- **P2 — `state.gitHost` clear on session expiry** (DEV-C; `extension/src/service-worker/index.ts:51-58`) — expiry callback clears manifest but leaves the cached git-host client.
- **P2 — duplicated teardown helpers** (DEV-C; `extension/src/popup/components/settings.ts:56-65` and `settings-vault.ts:15-22`) — two near-identical cleanup paths in a regression class with known prior leaks.
- **P2 — `Promise.all` without per-promise error handling** (DEV-C; `extension/src/popup/components/devices.ts:47-50`, `trash.ts:39-46`) — single rejected RPC fails the whole render.
- **P2 — MutationObserver `scan()` not debounced** (DEV-C; `extension/src/content/detector.ts:96-103`) — SPA churn re-runs the full scan many times per second.
- **CLI/extension parity gap — no equivalent to `relicario status`** (DEV-C parity table; PM kickoff) — extension surfaces nothing comparable to ahead/behind/lastSyncAt/pendingItems.
## Approach
Architectural shape: **types first, then extract, then split**. The extension already has a clean message-router/SW boundary; this plan finishes the job for the three surfaces that don't yet honor it (state.ts, setup.ts, vault.ts) and pulls duplicated SW helpers into one home.
### Post-split `extension/src/vault/`
```
extension/src/vault/
├── vault.html (unchanged)
├── vault.css (unchanged)
├── vault.ts (~200 LOC; routing + state only — owns
│ hash parsing, RouterState, render() entry,
│ imports the modules below)
├── vault-shell.ts (DOM scaffolding, color-scheme apply,
│ onMessage wiring, applyShellViewClass)
├── vault-sidebar.ts (renderSidebarCategories, search input
│ wiring with 50-100ms debounce, nav buttons,
│ global keydown shortcuts)
├── vault-list.ts (renderListPane, row rendering, type icons)
├── vault-drawer.ts (openDrawer/closeDrawer, renderDrawer,
│ drawer field grid, drawer event wiring;
│ state.drawerOpen reset is owned here)
├── vault-form-wrapper.ts (renderFormWrapped, sticky bar, header;
│ __test__ export migrates with it)
├── vault-status.ts (NEW — renders the get_vault_status indicator
│ in the sidebar; phase 6)
└── components/
├── backup-panel.ts (unchanged)
└── import-panel.ts (unchanged)
```
Rationale: each module owns one concern that has its own teardown, its own DOM rectangle, and its own subset of `RouterState`. Adding a new pane view today is a 5-place edit (`teardownPaneComponents` lines 813-820 is the symptom DEV-C flagged); after the split, it is one new module + one entry in `vault.ts`'s render switch. The `vault_locked` RPC intercept at lines 47-74 lifts out entirely (see `shared/state.ts` below).
### Post-split `extension/src/service-worker/`
```
extension/src/service-worker/
├── index.ts (entry point — onMessage, initWasm,
│ inactivity-timer wiring; phase 5 touches
│ the content-callable timer-reset rule
│ and the gitHost clear-on-expiry)
├── session.ts (unchanged in scope; Plan A removes the
│ try{free()} swallow at :26)
├── session-timer.ts (unchanged; phase 5 documents the
│ exclusion list inline)
├── storage.ts (NEW — phase 2: loadDeviceSettings,
│ loadBlacklist, saveBlacklist, plus the
│ config + image-base64 + setup-state loaders
│ if natural; both router files import here)
├── vault.ts (gains: itemToManifestEntry (moved from
│ both routers; phase 2), create_vault and
│ attach_vault handlers (phase 3), and the
│ get_vault_status handler (phase 6))
├── git-host.ts (unchanged)
├── github.ts / gitea.ts (unchanged)
├── devices.ts (unchanged)
└── router/
├── index.ts (unchanged)
├── popup-only.ts (imports from ../storage and ../vault;
duplicated definitions deleted)
└── content-callable.ts (imports from ../storage and ../vault;
duplicated definitions deleted)
```
Rationale: `service-worker/storage.ts` becomes the single source for `chrome.storage.local` reads/writes the SW does. `service-worker/vault.ts` already owns vault-tier WASM orchestration, so `itemToManifestEntry`, `create_vault`, `attach_vault`, and `get_vault_status` all belong there.
### `StateHost` interface contract (P1.6)
```ts
// extension/src/shared/state.ts (post-rewrite)
import type { Request, Response } from './messages';
import type { PopupState, View } from '../popup/popup';
export interface StateHost {
getState(): PopupState;
setState(partial: Partial<PopupState>): void;
navigate(view: View, extras?: Partial<PopupState>): void;
sendMessage(request: Request): Promise<Response>;
escapeHtml(s: string): string;
popOutToTab(): void;
isInTab(): boolean;
openVaultTab(hash?: string): void;
}
let host: StateHost | null = null;
export function registerHost(h: StateHost): void {
if (host) throw new Error('state host already registered');
host = h;
}
/** Test-only — vitest beforeEach() calls this to break inter-test leakage. */
export function __resetHostForTests(): void { host = null; }
export function getState(): PopupState {
if (!host) throw new Error('No state host registered');
return host.getState();
}
export function setState<K extends keyof PopupState>(
partial: Pick<PopupState, K> | Partial<PopupState>,
): void {
if (!host) throw new Error('No state host registered');
host.setState(partial);
}
// navigate / sendMessage / escapeHtml / popOutToTab / isInTab / openVaultTab
// keep their generic-friendly signatures (View, Request, Response).
//
// vault_locked unification: shared/state.ts wraps sendMessage so a
// `{ ok: false, error: 'vault_locked' }` response (for any request other
// than is_unlocked / unlock) flips host state to locked + dispatches a
// `session_expired`-equivalent event the popup also listens for.
// Both popup.ts and vault.ts consume from this single channel — the
// vault.ts:47-74 RPC intercept is removed in phase 4.
```
Note `View` and `PopupState` are currently defined in `extension/src/popup/popup.ts`. To avoid a popup→shared circular import, they migrate to `extension/src/shared/types.ts` (or `extension/src/shared/popup-state.ts`) before `state.ts` re-imports them. This migration is part of phase 1.
### Setup wizard step-registry shape (P1.4)
```ts
// extension/src/setup/setup.ts (post-rewrite)
interface StepContext {
state: WizardState;
rerender: () => void;
goto: (id: StepId) => void;
}
interface SetupStep {
id: StepId;
/** Pure render — returns innerHTML for #setup-step-host. */
render: (ctx: StepContext) => string;
/** Wire DOM events; return a teardown the wizard runs on step change. */
attach: (root: HTMLElement, ctx: StepContext) => () => void;
}
type StepId = 'mode' | 'host' | 'connection' | 'vault' | 'device' | 'done';
const STEPS: ReadonlyArray<SetupStep> = [
modeStep, hostStep, connectionStep, vaultStep, deviceStep, doneStep,
];
/** Cleared on `beforeunload` and on `goto('mode')`. */
function clearWizardState(): void {
// Best-effort wipe: zero-fill Uint8Arrays before drop where reachable;
// null out passphrase + token strings (JS strings are GC-only — see Risks).
if (state.carrierImageBytes) state.carrierImageBytes.fill(0);
if (state.referenceImageBytes) state.referenceImageBytes.fill(0);
if (state.referenceImageBytesAttach) state.referenceImageBytesAttach.fill(0);
// Reset every field of `state` to its initial value.
// NOTE: setup no longer holds a SessionHandle (the SW does); the
// phase 1 sweep deletes `verifiedHandle` from WizardState entirely.
}
```
`setup.ts` no longer imports `relicario-wasm` and no longer touches `wasm.lock` / `.free()`. The two new SW handlers do that work (see Plan A coordination below).
The 1220 LOC drops to ~500: the ~400 LOC of crypto orchestration (image-secret extract / embed, KDF gating, manifest_encrypt, attachment_encrypt) moves to the SW; the remaining UI logic compresses by ~300 LOC because the step-registry pattern collapses the six `renderStepN` / `attachStepN` pairs into six `SetupStep` objects.
### New SW message handlers
Added to `extension/src/shared/messages.ts`:
```ts
| { type: 'create_vault'; config: VaultConfig; passphrase: string;
carrierImageBytes: ArrayBuffer; deviceName: string }
| { type: 'attach_vault'; config: VaultConfig; passphrase: string;
referenceImageBytes: ArrayBuffer; deviceName: string }
| { type: 'get_vault_status' }
```
Both `create_vault` and `attach_vault` are added to `POPUP_ONLY_TYPES`. `get_vault_status` joins the same set. Response shapes:
```ts
export interface CreateVaultResponse extends Extract<Response, { ok: true }> {
data: { referenceImageBytes: Uint8Array; deviceName: string;
recoveryQrAvailable: true };
}
export interface AttachVaultResponse extends Extract<Response, { ok: true }> {
data: { deviceName: string };
}
export interface GetVaultStatusResponse extends Extract<Response, { ok: true }> {
data: { ahead: number; behind: number; lastSyncAt: number | null;
pendingItems: number };
}
```
The SW handlers live in `service-worker/vault.ts`. `create_vault` and `attach_vault` hold their own internal session reference for the duration of the operation — they do not depend on the user-facing inactivity timer (see Risks).
## Implementation phases
Each phase keeps the existing vitest test suite green throughout. Regression budget per phase: **zero** test failures introduced; new tests added per phase (synthetic fixtures only, no binary blobs — `make_test_jpeg` style equivalents on the JS side).
### Phase 1 — `StateHost` typing + `__resetHostForTests` (P1.6)
- **Goal:** Make `extension/src/shared/state.ts` type-checked end to end so phases 3 and 4 can refactor against a real contract.
- **Changes:**
- Move `View` and `PopupState` from `extension/src/popup/popup.ts` to `extension/src/shared/types.ts` (or new `extension/src/shared/popup-state.ts`).
- Rewrite `extension/src/shared/state.ts` to the snippet in **Approach**: typed `StateHost`, generic `getState`/`setState`, double-registration throw, `__resetHostForTests` export. No `any` in the public surface.
- Sweep all callers of `getState()` / `setState()` / `navigate()`. Existing `as any` casts surface as TS errors and get fixed (typed access where the field is known; an explicit narrowing where it isn't).
- The `vault_locked` channel collapse is **not** in this phase — the wrapper around `sendMessage` lands here as a no-op signature change; its body (the RPC intercept) lifts in phase 4.
- **Tests:**
- New `extension/src/shared/__tests__/state.test.ts` covering: register-then-getState round-trip; double-register throws; `__resetHostForTests` clears the singleton; `getState()` without a registered host throws.
- Adjust any existing test that accidentally relied on a leaked host (none expected; the existing suites already register-then-tear-down per-test).
- **Effort:** S-M
- **Depends on:** none
### Phase 2 — Extract `service-worker/storage.ts` + move `itemToManifestEntry` (P1.9)
- **Goal:** Eliminate the duplicated SW helpers so blacklist mutations and manifest-projection refactors happen in one place.
- **Changes:**
- Create `extension/src/service-worker/storage.ts` exporting `loadDeviceSettings`, `saveDeviceSettings`, `loadBlacklist`, `saveBlacklist`. Migrate the bodies from `extension/src/service-worker/router/popup-only.ts:687-703` (and `saveDeviceSettings` from the same file) and delete the `loadDeviceSettings` / `loadBlacklist` / `saveBlacklist` definitions in `extension/src/service-worker/router/content-callable.ts:187-205`.
- Move `itemToManifestEntry` from both router files (`popup-only.ts:707` and `content-callable.ts:169`) into `extension/src/service-worker/vault.ts` as a named export. Both routers import it from there.
- Optionally also fold `loadConfig` / `loadImageBase64` / `loadSetupState` into `storage.ts` since they're chrome.storage.local readers; keep the boundary clean.
- **Tests:**
- New `extension/src/service-worker/__tests__/storage.test.ts` covering load/save round-trips for each helper, default-value fallback when the key is absent.
- The existing `extension/src/service-worker/router/__tests__/router.test.ts` keeps passing; the dispatch behavior is unchanged.
- **Effort:** S
- **Depends on:** none (independent of phase 1; can ship parallel)
### Phase 3 — Setup wizard SW migration + step registry (P1.4)
- **Goal:** Setup wizard becomes UI-only. SW owns `create_vault` and `attach_vault` end to end. Wizard restructures to a step-registry pattern. Sensitive material clears on abandon.
- **Changes:**
- Add `create_vault` and `attach_vault` types to `extension/src/shared/messages.ts` (request union, response interfaces, capability set).
- Implement handlers in `extension/src/service-worker/vault.ts`. Each handler:
1. Computes salt + params, calls `unlock` (create) or verifies (attach), calls `embed_image_secret` / `extract_image_secret`, calls `register_device`, calls `manifest_encrypt` for the empty manifest, writes the resulting bytes to the configured remote via the existing `git-host` abstraction.
2. Holds its own `SessionHandle` for the duration. On success, transitions the SW into the unlocked state (replaces the popup-driven `unlock` path's outcome); on failure, calls `wasm.lock(handle)` then `.free()` (see Plan A coordination).
3. Returns the reference image bytes (`create_vault`) or just the device name (`attach_vault`) so the wizard's "Done" step can offer a download.
- Rewrite `extension/src/setup/setup.ts`:
- Delete the WASM dynamic-import block at lines 28-37; delete the `loadWasm()` helper; delete the `wasm` module variable; delete `verifiedHandle` from `WizardState`.
- Convert each of the six `renderStepN` / `attachStepN` pairs into a `SetupStep` object (`modeStep`, `hostStep`, `connectionStep`, `vaultStep`, `deviceStep`, `doneStep`) per the step-registry snippet in **Approach**. The wizard's render loop becomes `STEPS[state.step].render(ctx)` + `STEPS[state.step].attach(root, ctx)`.
- Replace direct WASM orchestration in the vault step with `sendMessage({ type: 'create_vault', ... })` / `sendMessage({ type: 'attach_vault', ... })`.
- Add `clearWizardState()` per the snippet; bind to `window.addEventListener('beforeunload', clearWizardState)` and call from the `goto('mode')` path.
- Update `extension/src/wasm.d.ts` only if the new SW handlers need a WASM entry point that isn't already declared (verify by reading `extension/src/service-worker/vault.ts` — they likely don't, since `unlock`/`embed_image_secret`/`register_device`/`manifest_encrypt` are already declared; the SW just orchestrates them). If no new WASM entry, this file is **not touched** by Plan C.
- The `recovery_qr_generated_at` direct chrome.storage.local write at `setup.ts:1056-1062` is **out of scope** per the kickoff (defer to a P3 cleanup) — it stays as-is in this phase.
- **Tests:**
- Update `extension/src/setup/__tests__/setup.test.ts` to assert on the step registry shape (each `SetupStep.id`, `render` returns non-empty HTML, `attach` returns a callable teardown).
- New SW-side test in `extension/src/service-worker/__tests__/vault.test.ts` (or extend an existing one) covering `create_vault` happy path with a stubbed `git-host` and the WASM stub from `__stubs__/relicario_wasm.stub.ts` (round out the stub for `embed_image_secret`, `register_device`, `manifest_encrypt` if they aren't there yet — DEV-C P2 noted only 7 of ~25 are stubbed).
- New test for `clearWizardState`: simulate `beforeunload`, assert `Uint8Array` contents are zero-filled.
- **Effort:** L
- **Depends on:** Phase 1 (the wizard's UI uses `getState`/`setState` against a typed `StateHost`)
### Phase 4 — Split `vault.ts` + lift `vault_locked` channel (P1.5)
- **Goal:** `extension/src/vault/vault.ts` shrinks to ~200 LOC of routing + state. Each pane concern lives in its own module. The `vault_locked` RPC intercept disappears from vault.ts and runs in `shared/state.ts` instead.
- **Changes:**
- Create the five new files (`vault-shell.ts`, `vault-sidebar.ts`, `vault-list.ts`, `vault-drawer.ts`, `vault-form-wrapper.ts`) per the directory tree in **Approach**. Migrate the corresponding code blocks from the existing `vault.ts` into them. Each module exports a `render*` function plus, where stateful, an explicit `teardown()`.
- In `vault.ts`, retain only: `RouterState` declaration, hash parsing (`parseHash`, `setHash`), `loadManifest`, the `render()` entry point, the `renderPane()` switch, and the imports that wire the modules together.
- In `vault-drawer.ts`: include an `ensureDrawerClosedForRoute(route)` helper that the `renderPane` switch calls before any non-list view. This implements the P2 fix for `vault.ts:495-536` (drawer state no longer leaks across navigation).
- In `vault-sidebar.ts`: wrap the search input handler in a 50-100ms debounce (DEV-C P2 — `vault.ts:648-695`).
- Lift the `vault_locked` RPC intercept (`vault.ts:47-74`) into the `sendMessage` wrapper in `extension/src/shared/state.ts` (the wrapper signature landed in phase 1; phase 4 fills the body). Both popup and vault now consume the same `session_expired`-equivalent flow. **Migration discipline:** keep both signals firing for one merge cycle (the SW continues to dispatch `session_expired` and the wrapper still fires the intercept) until both surfaces have been verified as consuming from `shared/state.ts`; then collapse.
- **Tests:**
- Existing `extension/src/vault/__tests__/form-wrapper.test.ts` and `sidebar-glyphs.test.ts` continue to pass (the symbols they import move from `vault.ts` to `vault-form-wrapper.ts` / `vault-sidebar.ts`; update import paths).
- New `extension/src/vault/__tests__/drawer-state.test.ts` covering: drawer auto-closes when navigating from list to trash/devices/settings.
- New `extension/src/shared/__tests__/state-vault-locked.test.ts` covering: a `{ ok: false, error: 'vault_locked' }` response (for a request other than `is_unlocked`/`unlock`) flips host state to locked.
- **Effort:** M
- **Depends on:** Phase 1 (uses the typed `StateHost` surface and the `sendMessage` wrapper)
### Phase 5 — Extension P2 cluster
- **Goal:** Sweep five small extension P2s that share the same "small fix, real correctness win" shape.
- **Changes:**
- **Inactivity timer reset on content-callable messages** (`extension/src/service-worker/index.ts:76-78`): invert the current condition. Reset on **all** messages except a small documented exclusion set (`get_autofill_candidates` is the only known read-only content call). Define `READ_ONLY_CONTENT_CALLABLE` in `service-worker/session-timer.ts` with a doc comment listing each excluded type and the rationale; `index.ts` consults that set.
- **`state.gitHost` clear on session expiry** (`extension/src/service-worker/index.ts:51-58`): in the `sessionTimer.onExpired` callback, also set `state.gitHost = null`. The initializer rebuilds it on demand.
- **Teardown helper extraction** (`extension/src/popup/components/settings.ts:56-65` and `settings-vault.ts:15-22`): extract `teardownSettingsCommon()` exported from `settings.ts` (or a new `settings-shared.ts`); both `settings.ts:teardownSettings` and `settings-vault.ts:teardown` call it. Single source for the closeGeneratorPanel + activeKeyHandler removal pattern.
- **`Promise.allSettled` in devices/trash** (`extension/src/popup/components/devices.ts:47-50`, `trash.ts:39-46`): swap `Promise.all` for `Promise.allSettled`; render each settled response defensively (`.status === 'fulfilled' && r.value.ok`); fall back to "couldn't load" copy per failed slot.
- **MutationObserver debounce** (`extension/src/content/detector.ts:96-103`): wrap the existing `() => scan()` callback in a 200ms trailing-edge debounce (or `requestIdleCallback` if available; `setTimeout(..., 200)` fallback). Reset on every observer fire.
- **Tests:**
- Existing `extension/src/service-worker/__tests__/session-timer.test.ts` extended with a "popup-only message resets, content-callable message does not reset (except listed exclusions)" case.
- Existing `extension/src/popup/components/__tests__/devices.test.ts` and `trash.test.ts` extended with a "one RPC fails, the other still renders" case.
- Existing `extension/src/popup/components/__tests__/settings.test.ts` and `settings-vault.test.ts` extended to confirm both call paths invoke `teardownSettingsCommon`.
- **Effort:** M
- **Depends on:** none (all five are independent of phases 1-4)
### Phase 6 — `get_vault_status` SW message + vault-sidebar status indicator
- **Goal:** Close the last CLI/extension parity gap (`relicario status` ↔ extension status indicator).
- **Changes:**
- Add `{ type: 'get_vault_status' }` to `extension/src/shared/messages.ts` and to `POPUP_ONLY_TYPES`. Add `GetVaultStatusResponse` per the **Approach** snippet.
- Implement the handler in `extension/src/service-worker/vault.ts`. It returns `{ ahead, behind, lastSyncAt, pendingItems }` from cached state on `state.gitHost` (no actual sync). A "last sync" timestamp is recorded in `service-worker/index.ts` (or `state.gitHost`) on each successful `sync` handler return.
- Create `extension/src/vault/vault-status.ts` rendering a small indicator in the sidebar footer: glyph + "in sync" / "N ahead" / "N pending" / "last sync 2m ago". Polls on sidebar mount and on a manual refresh button (NOT every render — see Risks).
- **Tests:**
- New `extension/src/service-worker/__tests__/vault-status.test.ts` covering the four state combinations and the no-sync invariant (handler does not touch the network).
- New `extension/src/vault/__tests__/status-indicator.test.ts` for the renderer.
- **Effort:** S-M
- **Depends on:** Phase 4 (the indicator lives inside the new `vault-sidebar.ts` boundary)
### Future / deferred (Plan B coordination)
Plan B (CLI restructure) migrates `parse_month_year`, `base32_decode_lenient`, and `guess_mime` from the CLI into `relicario-core`, then re-exports them through `relicario-wasm`. Once those WASM exports land, the extension can consume them via new SW message handlers (e.g. `parse_month_year`, `decode_totp_secret`, `guess_mime_for_filename`) — this is a natural follow-up and explicitly **deferred to a future plan**. The seam to consume them is `extension/src/service-worker/vault.ts` (or a new `service-worker/parse.ts`) plus a new entry in `extension/src/wasm.d.ts`. Plan C does **not** design those handlers in detail.
## Risks and mitigations
- **State.ts typing changes ripple.** Every consumer of `getState`/`setState` becomes type-checked; existing `as any` casts will surface as TS errors. *Mitigation:* phase 1 includes a sweep + targeted TS error fix as part of the phase, not as a follow-up. Run `tsc --noEmit` per file class to triage; expect ~15-30 errors clustered in `popup/components/*.ts` and `vault/*.ts`.
- **Setup-via-SW migration changes the crypto state machine.** Today setup orchestrates WASM directly; after phase 3, the SW owns vault creation. If the SW's user-facing inactivity timer fires mid-creation, the user could lose progress. *Mitigation:* design `create_vault` / `attach_vault` to be transactional from the SW's perspective — they hold their own internal session reference for the duration of the operation and do not consult or reset the user-facing inactivity timer until they return successfully. Document this contract in the handler header comments.
- **`vault.ts` split + `vault_locked` channel unification.** The popup currently uses `session_expired` event; vault tab uses RPC intercept. Unifying onto one channel means popup behavior must continue to work after phase 4. *Mitigation:* keep both signals firing during phase 4 (the SW continues to dispatch `session_expired`; the new wrapper in `shared/state.ts` also fires the intercept); collapse only after both surfaces are verified to consume from `shared/state.ts` in the same merge cycle. Add a regression test asserting the popup's lock screen renders on `session_expired` and the vault tab's lock screen renders on the SW response intercept.
- **`.free()` callsite policy.** Plan A (security/docs polish) handles the Rust-side `impl Drop for SessionHandle` and removes the `try { current.free() }` swallow at `extension/src/service-worker/session.ts:26`. Plan C does **not** redo that work, but wherever this refactor *moves* a `.free()` callsite — most notably during the phase 3 setup-to-SW migration where `setup.ts`'s `verifiedHandle` retires and the new `create_vault` / `attach_vault` handlers acquire their own handles — the new location must call `wasm.lock(handle)` first regardless of whether Plan A's Rust-side `impl Drop` lands. Cite Plan A as the source of the policy.
- **WASM boundary coordination.** Plan B (CLI restructure) will touch `extension/src/wasm.d.ts` for the new parser exports (`parse_month_year`, `base32_decode_lenient`, `guess_mime`) once they land in `relicario-wasm`. Plan C should **not** touch `extension/src/wasm.d.ts` unless `create_vault` / `attach_vault` need WASM entry points that aren't already declared (verify by reading `service-worker/vault.ts`; the SW already orchestrates `unlock`/`embed_image_secret`/`register_device`/`manifest_encrypt`, so likely no new entries needed). If both plans must touch `wasm.d.ts`, sequence Plan B's edits first and rebase Plan C on top.
- **`clearWizardState()` semantics.** Clearing on `beforeunload` is best-effort in browsers (the event can be skipped if the tab crashes or is killed). JS strings (passphrase, API token) are also GC-only — there is no `Zeroize` for them. *Mitigation:* explicit zero-fill of `Uint8Array` fields where possible (carrier image bytes, reference image bytes); document the best-effort contract in the function header comment; do not over-promise in user docs. The same caveat already applies to existing strings in `WizardState` today, so this is a maintenance of the existing contract, not a regression.
- **`get_vault_status` design.** Needs to call into the git-host abstraction without triggering an actual sync. *Mitigation:* cache the last-sync state in `state.gitHost` (add `lastSyncAt: number | null`, `ahead: number`, `behind: number` fields populated by the existing `sync` handler) and have `get_vault_status` read those cached values; the sidebar indicator polls on mount + on a manual refresh button rather than on every render. Any UI element that wants live status calls `sync` explicitly.
- **Phase ordering risk.** Phase 1 is the blocker — phases 3 and 4 both depend on the typed `StateHost`. If phase 1 takes longer than estimated, run phase 2 (independent) and phase 5 (independent) in parallel to avoid total stall.
## Out of scope
Plan C does NOT touch:
- Anything in **Plan A** (security/docs polish): Rust `impl Drop for SessionHandle`, the `service-worker/session.ts:26` swallow removal, `.free()` callsite audit, `recovery_qr.rs` documentation, server hardening, env-var audit.
- Anything in **Plan B** (CLI restructure): `cli/main.rs` split, git-shell error UX helper, `parse_month_year`/`base32_decode_lenient`/`guess_mime` migration to core (Plan C only consumes them, deferred to a future phase).
- Extension P3s: form-header `isInTab()` redundancy, popup.ts `isInTab()` heuristic, item-form.ts `renderComingSoon` dead code, `types/login.ts` size, `vault.ts:18-26` backup-panel comment, capture/detector/fill username-finder dedup, capture submit-button hook scope, setup.ts passphrase-score `-1` sentinel, `setup.ts:1056-1062` chrome.storage bypass, `setup.ts:1-7` "5-step" header comment, glyphs.ts partial adoption, `types.ts` `TotpKind` flat-union refactor, `totp-tools.ts:39-46` swallowed rejections, generator-panel cleanup idempotence guard, `item-list.ts` popover listener reuse path, popup `popup.ts:178-181` unconditional teardowns.
- Other CLI/extension parity items: per-attachment `delete_attachment` SW message (P3), `list --tag` filter doc note (P3) — only `get_vault_status` is in scope.
- Cross-cutting items not explicitly listed: `chrome.storage.local` direct reads outside the setup migration (e.g. `settings-security.ts:112-113`), `bun test` runner doc note, manifest version sync.
- The full P2/P3 tail outside the items folded above.
- The 8 "Open architectural decisions" from the synthesis appendix.
- WASM JS-naming snake_case → camelCase decision (defer to a separate plan).
- Anything touching the in-flight uncommitted v0.5.x work (vault.ts +151/-99, vault.css +238/-99, manifest version, glyphs additions). Plan C executes against committed `main` post-061facd.
## Done criteria
A reviewer confirms the plan shipped by checking each item:
- [ ] `extension/src/shared/state.ts` `StateHost` interface defines every field; no `any` in the public surface.
- [ ] `getState` returns `PopupState`; `setState` is generic over `keyof PopupState`.
- [ ] `registerHost` throws on second registration; `__resetHostForTests` is exported and used in vitest setup.
- [ ] `extension/src/service-worker/router/popup-only.ts` and `content-callable.ts` no longer contain `loadDeviceSettings` / `loadBlacklist` / `saveBlacklist` definitions (only imports from `service-worker/storage.ts`).
- [ ] `itemToManifestEntry` defined once in `extension/src/service-worker/vault.ts`, imported by both router files.
- [ ] `extension/src/setup/setup.ts` LOC ≤ ~500.
- [ ] `extension/src/setup/setup.ts` does not import `relicario-wasm` (no dynamic import, no `loadWasm`, no module-level `wasm` binding).
- [ ] SW handles `create_vault` and `attach_vault` messages (entries in `messages.ts`, `POPUP_ONLY_TYPES`, and `service-worker/vault.ts`).
- [ ] `clearWizardState()` exists, is bound to `beforeunload`, and is called from the `goto('mode')` path; sensitive `Uint8Array` fields are zero-filled.
- [ ] `extension/src/vault/vault.ts` is split into `vault.ts` + `vault-shell.ts` + `vault-sidebar.ts` + `vault-list.ts` + `vault-drawer.ts` + `vault-form-wrapper.ts`; `vault.ts` is ~200 LOC of routing + state only.
- [ ] Single `vault_locked` channel: the RPC intercept is in `extension/src/shared/state.ts`'s `sendMessage` wrapper; `vault.ts` no longer contains the intercept block at the old lines 47-74.
- [ ] Drawer auto-closes on navigation to non-list views (`state.drawerOpen = false` reset in `renderPane` / `ensureDrawerClosedForRoute`).
- [ ] Sidebar search input is debounced (50-100ms).
- [ ] Inactivity timer resets on **all** messages except a documented exclusion set (currently `get_autofill_candidates`); the exclusion set is defined and commented in `service-worker/session-timer.ts`.
- [ ] `state.gitHost = null` runs alongside `state.manifest = null` in the `sessionTimer.onExpired` callback.
- [ ] One `teardownSettingsCommon` helper exists; both `settings.ts` and `settings-vault.ts` call it.
- [ ] `devices.ts` and `trash.ts` use `Promise.allSettled` and render each settled response defensively.
- [ ] `extension/src/content/detector.ts` MutationObserver `scan()` is debounced (200ms or `requestIdleCallback`).
- [ ] `get_vault_status` SW message and response interface exist; vault sidebar renders an indicator on mount and on manual refresh.
- [ ] All existing vitest tests under `extension/src/**/__tests__/` pass; new tests added per phase pass.
- [ ] No `extension/src/wasm.d.ts` change introduced by Plan C unless coordinated with Plan B (verify the file's diff is empty post-merge, or coordinated explicitly with Plan B's parser exports).
- [ ] Every `.free()` callsite moved by this plan is preceded by `wasm.lock(handle)` (per Plan A's policy, regardless of whether Plan A's Rust `impl Drop` has landed).

135
docs/superpowers/specs/2026-05-04-security-polish-design.md Normal file
View File

@@ -0,0 +1,135 @@
# Security & Docs Polish — Design
**Date:** 2026-05-04
**Status:** Proposed
**Source:** `docs/superpowers/reviews/2026-05-04-architecture-review.md` (P1.1, P1.7, P1.8)
**Effort estimate:** S (whole-PR; phase totals below)
## Summary
This is the one-day security and documentation PR that ships first in the v0.5.x architecture-followup train, independent of Plan B (CLI restructure) and Plan C (extension restructure). It closes the single defense-in-depth crypto gap the review flagged (`SessionHandle` has no `impl Drop`, so wasm-bindgen's `.free()` is a cleanup no-op while the master key sits in WASM linear memory), removes the JS-side error swallow that was masking that fact, brings the one undocumented security-critical core file (`recovery_qr.rs`) up to the documentation density of `crypto.rs` / `imgsecret.rs` / `backup.rs` / `tar_safe.rs`, and confirms the relay test/launcher polish items. Together these are mechanical, low-risk changes that fix the only finding in the review where "what the code looks like it does" diverges dangerously from "what it actually does" — which is exactly the wall a tinkerer learning Rust would hit hardest.
## Findings addressed
- **P1.1 — `SessionHandle` has no `impl Drop`; `.free()` is a cleanup no-op** — DEV-B (Rust headline) + DEV-C (JS partner finding). Files: `crates/relicario-wasm/src/lib.rs:15-23`, `crates/relicario-wasm/src/session.rs:1-58`, `extension/src/service-worker/session.ts:26`.
- **Partner JS swallow** — DEV-C P2 in service-worker. File: `extension/src/service-worker/session.ts:26` (the `try { current.free(); } catch { /* already freed */ }` block).
- **P1.7 — `recovery_qr.rs` is undocumented relative to the rest of core** — DEV-A. File: `crates/relicario-core/src/recovery_qr.rs:1-130`.
- **P1.8 — `tools/relay/queue.test.ts` assertion update** — DEV-C. File: `tools/relay/queue.test.ts:54`. Confirmed during planning: **already fixed in `061facd`** — line 54 now reads `assert.ok(isRole("dev-c"));` and the negative case `assert.ok(!isRole("dev-d"));` is on line 55. No code work remains; the plan tracks this as a verification step.
- **Launcher follow-up — `tools/relay/start.sh` still references three roles** — DEV-C P2. File: `tools/relay/start.sh:30-86`. Confirmed during planning: **not yet fixed.** Both `launch_tmux` and `launch_kitty` open only PM, Dev-A, Dev-B (no Dev-C); the manual-mode banner still says "Open 3 new terminals"; the post-launch summary lines (`:61, :81`) print "3 windows (PM, Dev-A, Dev-B)"; no `*-dev-c-prompt.md` is discovered or used. This phase is real.
## Approach
The PR is mechanical and uniform across four phases:
- **Adds** one `impl Drop for SessionHandle` block in `crates/relicario-wasm/src/lib.rs`, one Rust test that exercises construct → drop → registry-empty, one module-level `//!` doc-block in `recovery_qr.rs`, one ASCII layout diagram next to the magic constants there, four per-function doc-comments on the public API there, and one `DEV_C_PROMPT` discovery line + a fourth window in each launcher mode of `start.sh`.
- **Removes** the `try { current.free(); } catch { /* already freed */ }` swallow at `extension/src/service-worker/session.ts:26`.
- **Verifies** every `.free()` callsite under `extension/src/` (the audit deliverable for P1.1's JS side; today the audit itself is small — only one match — but the deliverable is the recorded grep + a check that every code path that holds a `SessionHandle` calls `wasm.lock(handle)` before `free()`).
- **Confirms in passing** that `tools/relay/queue.test.ts:54` already matches the new role union (it does — committed in `061facd`).
No public API changes on the Rust side beyond the `Drop` impl (which is observably equivalent to "calling `lock(handle)` was previously the only way to clean up"). No JS public API changes. No file moves. No new dependencies (`wasm-bindgen-test` is already in `[dev-dependencies]` for the wasm crate).
## Implementation phases
### Phase 1 — Rust-side `impl Drop for SessionHandle` + test
- **Goal:** make wasm-bindgen's auto-generated `.free()` actually clear the master key from WASM linear memory, regardless of whether JS also called `lock(handle)`.
- **Changes:**
- `crates/relicario-wasm/src/lib.rs` — add `impl Drop for SessionHandle { fn drop(&mut self) { let _ = session::remove(self.0); } }` immediately after the existing `#[wasm_bindgen] impl SessionHandle { ... value() ... }` block (around line 23). Update the doc-comment on `SessionHandle` (line 15) to document the contract: "Dropping (or `.free()` from JS) removes the entry from the session registry and zeroizes the wrapped key/image_secret. `lock(handle)` remains available as the explicit early-cleanup path; `Drop` is the safety net."
- `crates/relicario-wasm/src/session.rs` — no functional change required, but add a one-line module-level note above `pub fn remove(...)` explicitly stating that `Drop for SessionHandle` is the second caller of this function alongside `lock()`, so that a future reader removing `lock()` doesn't accidentally orphan the cleanup path.
- **Tests:**
- Add a `#[wasm_bindgen_test]` (the `wasm-bindgen-test` crate is already in `[dev-dependencies]`, line 25 of `Cargo.toml`) under `crates/relicario-wasm/tests/session_drop.rs`. The test constructs a `SessionHandle` (via the public `unlock` path with synthetic JPEG + tiny `KdfParams` for fast key derivation, mirroring the existing native `session_tests::manifest_round_trip_via_handle` shape), records the underlying handle id via `handle.value()`, drops the handle, then asserts `session::with(id, |_| ()).is_none()`. The `session::with` symbol must be re-exported from `tests/` scope; if it isn't, expose `pub fn with_for_tests(handle: u32) -> bool` on the `session` module guarded by `#[cfg(test)]` so the test does not need to reach into thread-locals.
- As a fallback / belt-and-suspenders, add a parallel native `#[test]` under the existing `mod session_tests` in `lib.rs` that constructs a `SessionHandle(session::insert(...))`, drops it explicitly with `drop(handle)`, then asserts `session::with(h, |_| ()).is_none()`. Native tests run on every `cargo test`; the wasm-bindgen-test runs only when someone invokes `wasm-pack test`, so the native variant is the one that catches regressions in CI.
- **Effort:** S
- **Depends on:** none
### Phase 2 — JS-side `.free()` audit + remove the swallow
- **Goal:** make the JS side stop hiding crypto-state-transition errors, and prove (via a recorded audit) that there are no other `.free()` callsites on `SessionHandle` (or on `EncryptedAttachment` / `TotpCode`) that bypass `wasm.lock(handle)` first.
- **Changes:**
- `extension/src/service-worker/session.ts:24-28` — replace the body of `clearCurrent()` so that `current.free()` is called unconditionally (no `try`/`catch`) and `current` is set to `null` afterwards. The doc-block at the top of the file (lines 1-9) gets one extra paragraph noting that with `impl Drop` on the Rust side (Phase 1), `free()` now removes the registry entry — `lock(handle)` becomes a redundant early-cleanup option, but the SW currently does not call it. Document whether to keep `clearCurrent()` minimal (just `free()`) or add an explicit `wasm.lock(current.value)` immediately before `free()` for symmetry with the CLI's session lifecycle. Recommended: just `free()` post-Phase-1 — calling `lock` first is now belt-and-suspenders that adds a redundant HashMap remove. Document the rationale in the comment.
- **Audit deliverable:** run `grep -rn "\.free\b" extension/src/` and record the result in the PR description. Today this returns exactly one match (`extension/src/service-worker/session.ts:26`), confirming the SW is the only surface that calls `.free()` on a wasm-bindgen handle. If a future surface (e.g. an offscreen document, a future setup-flow refactor) adds a second callsite, this PR's grep becomes the baseline that flags the new one.
- **Lifecycle audit (no code change required, but record findings in PR description):** confirm every code path that constructs a `SessionHandle` via `unlock(...)` ultimately reaches `clearCurrent()`. The two paths are (a) explicit user lock via the popup `lock` action → `service-worker/router/popup-only.ts` `lock` handler → `clearCurrent()`, and (b) inactivity timer expiry → `service-worker/index.ts:51-58` session-expired callback → `clearCurrent()`. Both are already wired; this audit just records that the wiring is complete.
- **Tests:**
- Vitest in `extension/`: add a small unit test in `extension/src/service-worker/__tests__/session.test.ts` (create if absent) that asserts `clearCurrent()` is a no-op when `current` is `null`, calls `setCurrent({ free: spy, value: 1 } as unknown as SessionHandle)` then `clearCurrent()`, and asserts `spy` was called exactly once and `getCurrent()` returns `null`. Use the existing `__stubs__/relicario_wasm.stub.ts` pattern for the handle shape if convenient.
- Optionally extend the lifecycle test: a second case where `free()` throws — assert the exception now propagates out of `clearCurrent()` (regression coverage for the swallow removal).
- **Effort:** S
- **Depends on:** Phase 1 (the swallow removal is only safe once the Rust side actually does the cleanup work that `.free()` is now supposed to do)
### Phase 3 — `recovery_qr.rs` documentation pass
- **Goal:** raise `crates/relicario-core/src/recovery_qr.rs` to the documentation density of `crypto.rs` / `imgsecret.rs` / `backup.rs` / `tar_safe.rs` so a reader landing in the file does not assume it is out-of-the-documented-zone scratch code.
- **Changes:** all in `crates/relicario-core/src/recovery_qr.rs`:
- **Module-level `//!` header** at the top of the file (above the `use` block on line 1). Three short paragraphs: (1) *what* the module produces (a 109-byte payload encoded as a QR v40 EcLevel::M SVG that, given the same passphrase, recovers the 32-byte image_secret); (2) *why the format is structured this way* — XChaCha20-Poly1305 wrapping over an Argon2id-derived "wrap key" derived from the recovery passphrase (`b"relicario-recovery-v1\0"` domain-separation prefix prevents the wrap key from ever colliding with a vault master key, even if the user reuses their vault passphrase here); (3) *parameter-pinning rationale* — `production_params()` deliberately re-states `KdfParams::default()`'s values rather than referencing them, because changing the default at the type level must not silently invalidate every recovery QR a user has printed.
- **ASCII layout diagram** above the magic-constant block (lines 7-9):
```
// byte field length
// ------ -------------- ------
// 0..4 MAGIC = "RREC" 4
// 4..5 VERSION = 0x01 1
// 5..37 kdf_salt 32 (random per QR)
// 37..61 wrap_nonce 24 (random per QR)
// 61..109 ciphertext 48 (32 image_secret + 16 AEAD tag)
// ------------------------------
// total 109
```
Pair the diagram with a `const _: () = assert!(PAYLOAD_LEN == 4 + 1 + 32 + 24 + 48);` static assertion so a future edit that changes the layout cannot drift from the documented total without a compile error.
- **Doc-comments on the four public items:**
- `RecoveryQrPayload` (line 11): one-paragraph summary; note that `as_bytes()` is the only accessor and that the bytes are an opaque sealed package — they should travel together (printing them as a QR is the supported transport, but a hex string also works for paranoid sneakernet).
- `generate_recovery_qr` (line 45): documents inputs (passphrase, image_secret), output (the 109-byte payload), and the security properties (random kdf_salt + random wrap_nonce per call ⇒ two QRs from the same passphrase + image_secret are different bytes). Cross-reference `recovery_qr_to_svg` for rendering.
- `unwrap_recovery_qr` (line 81): documents inputs/output, return-type rationale (`Zeroizing<[u8; 32]>` so the recovered image_secret zeroes when dropped), and the error vocabulary (`RecoveryQr` for format errors, `Decrypt` for AEAD failure / wrong passphrase — the deliberate non-distinguishing rejection from audit M4).
- `recovery_qr_to_svg` (line 122): documents the rendering choice (QR v40, EcLevel::M, 140×140 minimum dimension) and the rationale (109 bytes fits comfortably in v40 at M, leaving recovery-error-correction headroom for a smudged print).
- **`production_params` (line 32):** add a doc-comment explaining the deliberate divergence-tolerance from `KdfParams::default()` — wording: "Pinned at QR-format authoring time. `KdfParams::default()` may evolve as we re-tune Argon2 cost; this function MUST NOT change values that have ever shipped, because doing so silently invalidates every recovery QR printed under a previous parameter set." Either keep the function-with-comment shape, or per the synthesis suggestion, replace it with a `const RECOVERY_PRODUCTION_PARAMS: KdfParams = KdfParams { argon2_m: 65536, argon2_t: 3, argon2_p: 4 };` if `KdfParams` is `const`-constructible (it is — checked via `crypto.rs`). Either is acceptable; the `const` form is preferred because it makes the "this is not derived from defaults" property visible at the use site.
- **Optional micro-cleanups while in the file** (cite explicitly so they aren't surprising in code review):
- Replace the hand-counted `payload_bytes[5..37]`, `[37..61]`, `[61..109]` slice indices with named constants (`KDF_SALT_RANGE`, `WRAP_NONCE_RANGE`, `CIPHERTEXT_RANGE`) derived from the layout offsets. Improves diff-readability if the layout ever extends.
- Add a one-line comment to `recovery_kdf_input` documenting that the length-prefix on `nfc_bytes` mirrors the same convention as `crypto::derive_master_key` (audit H1 in the design spec) — this is the connective tissue that explains "why is there a `(len as u64).to_be_bytes()` here too?"
- **Tests:**
- No new test logic needed — the existing `tests/recovery_qr.rs` and the `RecoveryQr`/`Decrypt` round-trips in `tests/integration.rs` already pin behaviour.
- The `const _: () = assert!(...)` static assertion is itself the contract test for the layout diagram.
- **Effort:** S
- **Depends on:** none
### Phase 4 — Relay launcher (queue.test.ts already done in `061facd`)
- **Goal:** confirm the test fix that already shipped, and bring the launcher script up to the four-role world.
- **Changes:**
- `tools/relay/queue.test.ts` — **no change.** Confirmed during planning: line 54 is already `assert.ok(isRole("dev-c"));` and line 55 is already `assert.ok(!isRole("dev-d"));` (committed in `061facd`). This phase records the verification step in the PR description so a reviewer can see the file was checked.
- `tools/relay/start.sh` — real changes:
- Add a `DEV_C_PROMPT` discovery line at lines 30-33 alongside the existing `PM_PROMPT` / `DEV_A_PROMPT` / `DEV_B_PROMPT` definitions: `DEV_C_PROMPT="$(ls -t "$COORD_DIR"/*-dev-c-prompt.md 2>/dev/null | head -1 || echo "(none found)")"`.
- Manual mode (`print_manual_instructions`, lines 35-51) — change "Open 3 new terminals" to "Open 4 new terminals" and add the `Terminal 4 (Dev C): cat '$DEV_C_PROMPT'` line.
- tmux mode (`launch_tmux`, lines 53-69) — add a fourth `tmux new-window -t "$SESSION:" -n "dev-c" "cd '$REPO_ROOT' && claude"` line; update the summary print on line 61 from "4 windows: relay, pm, dev-a, dev-b" to "5 windows: relay, pm, dev-a, dev-b, dev-c"; add `Dev C: $DEV_C_PROMPT` to the prompts list (currently lines 64-66).
- kitty mode (`launch_kitty`, lines 71-86) — add a fourth `kitty @ launch --type=tab --tab-title "Dev-C" --hold -- bash -l -i -c "cd '$REPO_ROOT' && claude"` line; update the summary print on line 81 from "3 windows (PM, Dev-A, Dev-B)" to "4 windows (PM, Dev-A, Dev-B, Dev-C)"; add `Dev C: $DEV_C_PROMPT` to the prompts list.
- **Tests:** none directly. The relay launcher has no automated tests; verification is "run `bash tools/relay/start.sh --manual` and confirm the banner mentions four terminals + the Dev-C prompt path; run `bash tools/relay/start.sh --kitty` on a kitty terminal and confirm a fourth tab opens." Both checks belong in the PR's manual-test plan.
- **Effort:** S
- **Depends on:** none
## Risks and mitigations
- **Adding `impl Drop` could change observable behaviour for any JS code that today holds two references to the same `SessionHandle` and frees one of them.** wasm-bindgen does not in fact let JS hold two refs to the same Rust struct without explicit cloning — the `SessionHandle` is moved into JS at construction and `.free()` consumes the handle pointer. The audit in Phase 2 confirmed only one `.free()` callsite under `extension/src/`. The CLI does not touch the WASM crate at all (Rust-side; the CLI uses `relicario-core` directly). So the realistic blast radius is exactly the one SW callsite this plan also touches. Mitigation: Phase 1's native test explicitly covers "drop the handle, registry is empty," and Phase 2's vitest covers "free is called exactly once."
- **Removing the `try { current.free() } catch { /* already freed */ }` swallow could expose latent bugs that were silently tolerated.** Specifically, if any code path today double-frees the handle (calls `clearCurrent()` twice without a `setCurrent()` in between), the catch is currently hiding it. Reading the SW lifecycle: `clearCurrent()` is called from (a) the `lock` message handler, (b) the inactivity-timer session-expired callback, and (c) potentially the unlock-failure cleanup path. The current `if (!current) return;` early-out at line 25 already guards the simplest double-free case (calling `clearCurrent()` twice in a row is safe because the second call no-ops). The only way the catch fires today is if something in WASM raises during `free()` — which post-Phase-1 should be impossible (Drop is infallible; `session::remove` returns a bool, not a Result). Mitigation: the swallow removal happens *after* Phase 1's Drop lands, so the catch should now have nothing to catch. If a regression test surfaces a real bug, the right fix is to find and fix the bug, not re-add the swallow.
- **The `recovery_qr.rs` static assertion (`const _: () = assert!(PAYLOAD_LEN == ...);`) requires Rust 1.79+** for `const` panic in test position. The workspace's `rust-toolchain.toml` (if present) or the `Cargo.toml` `rust-version` field needs to be checked; if the repo is on an older toolchain, fall back to a runtime `#[test] fn payload_len_matches_layout() { assert_eq!(PAYLOAD_LEN, 4 + 1 + 32 + 24 + 48); }` — same effect, slightly less elegant. Verify during implementation; not a blocker.
- **Phase 4's launcher change is untested by automation.** A typo in the kitty/tmux command lines won't surface until someone runs `start.sh --kitty` or `start.sh --tmux`. Mitigation: the PR description includes an explicit manual-test plan ("run all three modes and confirm 4 windows + Dev-C prompt resolution") and the change pattern is symmetric to the existing Dev-A/Dev-B lines, minimizing typo surface.
- **The `wasm-bindgen-test` variant in Phase 1 only runs under `wasm-pack test --node`, which is not part of the project's default `cargo test` cycle.** The native `#[test]` in `mod session_tests` is the one that protects against regressions in CI. The wasm-bindgen-test is included for symmetry and to exercise the actual WASM target's Drop behaviour — it should be invoked at least once during PR review and added to a follow-up CI workflow if the project's test infrastructure ever expands to wasm-pack.
## Out of scope
This plan deliberately does not address, and explicitly leaves to other plans or the P2/P3 tail:
- Plan B's CLI restructure work in its entirety (P1.2 main.rs split, P1.3 git-shell error UX, P1.10 parser migration to core, plus the CLI P2/P3 entries). These touch `crates/relicario-cli/` and are independent of this PR.
- Plan C's extension restructure work in its entirety (P1.4 setup.ts WASM extraction, P1.5 vault.ts split, P1.6 shared/state.ts typing, P1.9 SW router helper extraction, plus the extension P2/P3 entries). These touch `extension/src/` and are independent of this PR — except for the single SW file that Phase 2 of this plan touches, which is small and disjoint from Plan C's surfaces.
- The P2 WASM cleanups DEV-B flagged: redundant `need_key + with(...).unwrap()` double-lookup, `Vec<u8>` getter clones, the `wasm_*_recovery_qr` naming inconsistency, and the `device.rs` vs `session.rs` concurrency-primitive split. Each is independently correct and worth doing, but each is its own one-paragraph change and the PR is already covering its theme.
- DEV-C's other relay P2s: queue TTL/persistence/cap, the `tools/relay/call.py` and `call.ts` tracking decision, `server.ts` per-connection factory comment. None block this PR.
- The eight "Open architectural decisions" at the bottom of the synthesis (deliberate-Drop-omission, parser migration timing, bootstrap rule, `Lock` subcommand visibility, Task 12 status, call.py tracking, snake_case-vs-camelCase WASM naming, `.gitea_env_vars` gitignore). Most are user-judgement calls; this PR confirms #1 (the omission was not deliberate; the fix lands here) but takes no position on #2-#8.
- Anything touching the in-flight uncommitted v0.5.x work — manifests, glyphs, vault.css, relay tooling beyond `start.sh`. Those changes are on parallel branches and are explicitly hands-off for this PR.
## Done criteria
A reviewer can confirm this plan shipped by checking:
- [ ] `crates/relicario-wasm/src/lib.rs` contains `impl Drop for SessionHandle` whose body removes the registry entry.
- [ ] `crates/relicario-wasm/tests/session_drop.rs` (or equivalent native `#[test]` inside `mod session_tests`) asserts that dropping a `SessionHandle` removes its handle id from the session registry. `cargo test -p relicario-wasm` passes the new test.
- [ ] `extension/src/service-worker/session.ts` no longer contains `try { current.free(); } catch`. The body of `clearCurrent()` calls `current.free()` unconditionally, then sets `current = null`.
- [ ] `grep -rn "\.free\b" extension/src/` returns exactly one match (the SW callsite). PR description records this audit.
- [ ] `extension/src/service-worker/__tests__/session.test.ts` covers `clearCurrent()` with both the no-op (no current handle) and the propagating-throw cases. `npm test` from `extension/` passes.
- [ ] `crates/relicario-core/src/recovery_qr.rs` has a module-level `//!` header explaining format + domain-separation + parameter-pinning, an ASCII layout diagram next to the magic constants, doc-comments on `RecoveryQrPayload`, `generate_recovery_qr`, `unwrap_recovery_qr`, and `recovery_qr_to_svg`, plus either a `const RECOVERY_PRODUCTION_PARAMS` or a doc-commented `production_params()` explaining why it deliberately diverges from `KdfParams::default()`. `cargo test -p relicario-core` still passes.
- [ ] `tools/relay/queue.test.ts:54-55` verified to already match the new role union (no change required, recorded in PR description).
- [ ] `tools/relay/start.sh` discovers a `*-dev-c-prompt.md`, opens a fourth window/terminal in tmux and kitty modes, and the manual-mode banner mentions all four roles. `bash tools/relay/start.sh --manual` shows "Open 4 new terminals" and lists the Dev-C prompt path.