relicario/docs/superpowers/reviews/2026-05-04-dev-a-notes.md

# DEV-A Architecture Review Notes — Rust Core

Scope: `crates/relicario-core/src/**` (17 source files, ~5.5 kLOC) and `crates/relicario-core/tests/**` (9 integration suites). HEAD reviewed; the only uncommitted change in core is a version bump (`0.2.0 → 0.5.0` in `Cargo.toml`), nothing semantic.

## Summary

The crate has a clear, well-shaped architecture: a thin `lib.rs` re-exporting one concept per module, a unified `RelicarioError` enum, and a strict bytes-in/bytes-out posture (no fs, no net, no git anywhere — the boundary is held). The strongest part is the documentation density of the security-critical files: `crypto.rs`, `imgsecret.rs`, `backup.rs`, and `tar_safe.rs` each open with multi-paragraph rationale that walks a reader through the *why* (XChaCha20 vs AES-GCM, QIM with QUANT_STEP=50, length-prefixed concatenation, tar-bomb defenses) — exactly the "legibility-as-security" philosophy the README aspires to. Tests are excellent: RFC6238 vector for TOTP, an independent reference impl cross-check for Steam, NFC/NFD round-trips, raw-byte tar bombs that bypass the tar crate's sanitiser, and ~30 LastPass importer cases. The weakest part is unevenness — `recovery_qr.rs` is essentially undocumented despite being the user-visible last-resort recovery mechanism, and three modules (`vault.rs`, `manifest.rs`, `attachment.rs`) still carry "during this rewrite" / "added later in Task 22" headers from work that has long since shipped, which will mislead a newcomer about what's load-bearing.

`cargo clippy -p relicario-core --all-targets --no-deps` runs clean (no warnings).

## Findings (prioritized)

### P1 — `recovery_qr.rs` doc gap is conspicuous against the rest of the crate

**File(s):** `crates/relicario-core/src/recovery_qr.rs:1-130` (whole file)
**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 top with no explanation, and the 4+1+32+24+48 layout is hand-counted with no struct, no diagram, and no asserts. The domain-separation prefix `b"relicario-recovery-v1\0"` exists but isn't explained. `production_params()` redeclares the same values as `KdfParams::default()` with no comment on why the recovery format pins them.
**Why it matters:** Every other security-relevant file (`crypto.rs`, `imgsecret.rs`, `backup.rs`, `tar_safe.rs`) has the explanatory framing this codebase rewards readers for. A newcomer 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 a 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()`.

### P2 — Stale "in-progress rewrite" headers in three core files

**File(s):**
- `crates/relicario-core/src/vault.rs:1-7` ("v1 helpers ... intentionally NOT carried forward. The CLI rewrite in Plan 1B switches to the new helpers.")
- `crates/relicario-core/src/manifest.rs:1-2` ("Lives next to the old entry.rs Manifest during this rewrite; entry.rs is deleted in Task 25.")
- `crates/relicario-core/src/attachment.rs:1-4` ("Encryption helpers ... are added later in Task 22 once the crypto module is settled.")

**Observation:** Each comment describes work that has already shipped. Task 22 added the helpers (they're 30 lines below in the same file). `entry.rs` is gone. Plan 1B is merged. The text reads as if there's a sibling file or a future change to wait for.
**Why it matters:** A newcomer trying to understand the manifest will go looking for `entry.rs` to compare. A newcomer reading `attachment.rs` will read past the helpers thinking "those are coming later." These are the cheapest possible cleanup — comment edits — and they each remove a tripwire.
**Suggested direction:** Replace with a one-line description of what the module *is*, not what it was during the rewrite.

### P2 — `extract_with_crop_recovery` is narrower than the spec describes

**File(s):** `crates/relicario-core/src/imgsecret.rs:849-899`
**Observation:** The design spec (`docs/superpowers/specs/2026-04-11-relicario-design.md` §imgsecret extraction step 3) says crop recovery iterates `(dx, dy)` from -15% to +15% stepping by 8 px (~16,800 candidates for 4000×3000). The code only varies assumed original `orig_w`/`orig_h` while pinning `dx = 0, dy = 0`. The successful-crop test at `imgsecret.rs:1108-1137` only crops the *right* edge, where dx=0 happens to be the correct offset.
**Why it matters:** Crops from the *left* or *top* (e.g. an Instagram square crop centered on a portrait, or any social-media platform that re-frames around faces) won't recover. The spec promises "15% from any edge"; the implementation delivers ~15% from the right and bottom only. Either the spec is wrong (which is allowed — the spec is marked historical) or the implementation has a quiet hole in the recovery surface. If the user ever uploads their reference JPEG to a service that left-crops, recovery will fail and the failure mode looks like "your image is wrong" rather than "we don't try that crop direction."
**Suggested direction:** Either (a) extend the recovery loop with a small dx/dy search bounded by the 15% margin, or (b) update the spec language and the user-facing docs to say "right/bottom crop tolerance only." A third option is to add a test that left-crops a watermarked image and currently fails — that captures the gap and lets future work close it.

### P2 — Two dead fields in `EmbedRegion`

**File(s):** `crates/relicario-core/src/imgsecret.rs:225-229`
**Observation:** `region_width` and `region_height` are computed in `compute_region`, stored in the struct, and silenced with `#[allow(dead_code)]`. Nothing reads them — downstream code uses `blocks_x` / `blocks_y` and `BLOCK_SIZE`.
**Why it matters:** The `#[allow(dead_code)]` is an explicit "I know this is unused" — a newcomer reasonably assumes the fields are load-bearing and will hunt for the consumers, finding nothing. Either they're pre-positioned for a future feature (in which case a comment saying so would help) or they should go.
**Suggested direction:** Delete both fields and the allow attributes, or add a one-line comment explaining the future use. (The struct is private, so removal is risk-free.)

### P2 — Three base32 implementations live in one crate

**File(s):**
- `crates/relicario-core/src/item.rs:255-275` (`base32_encode`, RFC 4648 alphabet)
- `crates/relicario-core/src/import_lastpass.rs:202-220` (`decode_base32_totp`, same alphabet)
- `crates/relicario-core/src/item_types/totp.rs:13` (`STEAM_ALPHABET`, *different* alphabet — by design)

**Observation:** The first two are inverses of each other but live in different modules with no shared helper. The third is intentionally different (Steam Guard's de-ambiguated alphabet). They all hand-roll the bit-packing loop.
**Why it matters:** A reader who finds one of the three has to grep to discover whether there are others, and whether they agree. A future change to the RFC 4648 path (e.g., padding behavior) needs to be applied in two places.
**Suggested direction:** Extract a small `pub(crate) mod base32` with `encode_rfc4648`, `decode_rfc4648`, leaving Steam's bespoke alphabet where it is (with a `// not RFC 4648 — Steam Guard's de-ambiguated alphabet` neighbour comment).

### P2 — Backup format embeds the reference JPEG as base64-in-JSON

**File(s):** `crates/relicario-core/src/backup.rs:148-152, 274-280, 343-345`
**Observation:** The `Envelope.vault.reference_jpg: Option<String>` carries the JPEG as base64-encoded JSON string. After zstd (which can't compress JPEG), a 4 MB reference photo bloats by ~33% from base64 plus JSON overhead.
**Why it matters:** Backup files for users who bundle the reference image will be substantially larger than necessary. Round-trip works (covered by `tests/backup.rs:96-106`), so this is a footprint concern, not a correctness one. Worth flagging if backup size ever shows up in a complaint.
**Suggested direction:** Bump `FORMAT_VERSION` and put `reference_jpg` and `git_archive` in a binary tail outside the JSON envelope, base64 only the small bytes. Defer until there's actual user pressure on backup size.

### P3 — Inconsistent error types and constant styles

**Observations (all small, batched here):**
- `crates/relicario-core/src/time.rs:18` — `MonthYear::new` returns `Result<Self, &'static str>` instead of `RelicarioError`. Every other constructor in the crate uses the unified error type.
- `crates/relicario-core/src/backup.rs:30` declares `pub const MAGIC: [u8; 4] = *b"RBAK";`; `crates/relicario-core/src/recovery_qr.rs:7` declares `const MAGIC: &[u8; 4] = b"RREC";`. Two different idioms for the same concept in adjacent files.
- `crates/relicario-core/Cargo.toml:36` has an empty `[dev-dependencies]` table (delete the header).
- `crates/relicario-core/src/crypto.rs:248-261` `derive_master_key_raw` is `pub` but only consumed by `recovery_qr.rs` inside this crate (verified via grep). `pub(crate)` would prevent accidental external misuse.

**Why it matters:** Each is trivial in isolation, but for a Rust newcomer reading the crate front-to-back, every inconsistency is a moment of "wait, why is this one different?" — and the answer is almost always "no reason, just historical."
**Suggested direction:** Pick one form for each pattern, sweep.

### P3 — Mid-file `use` blocks in `item.rs` and `attachment.rs`

**File(s):** `crates/relicario-core/src/item.rs:117-122`, `crates/relicario-core/src/attachment.rs:43-46`
**Observation:** Both files have `use` statements partway down the file (in `item.rs`, after `Section`; in `attachment.rs`, after `AttachmentSummary`). Idiomatic Rust hoists all `use` to the top.
**Why it matters:** Newcomer expectation; trivial to fix.

### P3 — `derive_icon_hint` only handles `Login` with no comment about other types

**File(s):** `crates/relicario-core/src/manifest.rs:84, 92-99`
**Observation:** Only `ItemCore::Login` produces a hostname hint; `_ => None` for the other six. No comment explaining why card-brand / favicon-from-URL / etc aren't derived for other types.
**Suggested direction:** One-line `// only Login items have a URL today; other types don't have an obvious icon source.` (Or implement card-brand / identity-favicon if the popup UI wants them.)

### P3 — `attachment.rs` has WHAT-comments on a deref pattern

**File(s):** `crates/relicario-core/src/attachment.rs:60-63, 83-85`
**Observation:** Two "Call-site adaptation" sections explain `&**master_key` in prose. The pattern is idiomatic Rust deref-coercion; the comment explains *what* not *why*.
**Suggested direction:** Delete both comment blocks. The code is self-documenting; if anything, the cognitive load is in the comment, not the deref.

### P3 — TOTP dynamic-truncation extraction is reproduced four times

**File(s):** `crates/relicario-core/src/item_types/totp.rs:75-99` (3 algorithm arms each duplicate the DT slice math), `217-228` (test reference impl reproduces it again).
**Suggested direction:** Extract a `fn dt(hmac_out: &[u8]) -> u32` helper. The test would still need its own copy as the cross-check.

### P3 — `STEAM_ALPHABET` source comment contradicts its own test

**File(s):** `crates/relicario-core/src/item_types/totp.rs:13` and `:287-291`
**Observation:** Source comment says "excludes ambiguous glyphs (0/O, 1/I/L, S/5, A/Z)". Test docstring at line 287 says "Note: '5' IS in the alphabet — S is excluded, so 5 is unambiguous." The test is right; the source comment is wrong about '5'.
**Suggested direction:** Fix the source comment to match the test (and the actual alphabet).

### P3 — `device.rs::sign` and `verify` share an extraction pattern that begs for a helper

**File(s):** `crates/relicario-core/src/device.rs:64-72, 86-94`
**Observation:** Both functions do `key_data.ed25519().ok_or(...)?.try_into::<[u8; 32]>().map_err(...)?`. Five-line copy.
**Suggested direction:** A `fn ed25519_bytes_from_private(...) -> Result<[u8; 32]>` and a `fn ed25519_bytes_from_public(...)` would each fold the extraction. Minor; not worth a refactor on its own but a free win if the file is being touched.

### P3 — `imgsecret.rs::read_block`'s `.unwrap()` deserves a one-liner

**File(s):** `crates/relicario-core/src/imgsecret.rs:315-319`
**Observation:** `read_block_abs(...).unwrap()` is safe because `compute_region` guarantees the block lies inside the embed region, but the invariant isn't stated.
**Suggested direction:** `// safe: compute_region ensures (start_x, start_y) + 8 fits within the image`. Same idea as the existing `expect("ascii-only charset")` in `generators.rs:64`.

### P3 — `r#type` field on `Item` and `ManifestEntry`

**File(s):** `crates/relicario-core/src/item.rs:134`, `crates/relicario-core/src/manifest.rs:23`
**Observation:** Using `r#type` (raw identifier) because `type` is a reserved keyword. Functional but jarring; a Rust newcomer doesn't know what `r#` means and won't immediately realize it's a field name not a type prefix.
**Suggested direction:** Rename to `item_type` with `#[serde(rename = "type")]` to keep wire format. Minor ergonomic win.

### P3 — BIP39 minimum word count of 3 is misleading

**File(s):** `crates/relicario-core/src/generators.rs:79-86`
**Observation:** `word_count` accepted range is `3..=12`. BIP39 spec proper starts at 12 words. 3-word output has only ~33 bits of entropy and would never pass `validate_passphrase_strength` for security uses, but the API permits it. The comment "This gives full-entropy sourcing even for short passphrases" elides that effective entropy is `word_count * log2(2048) = 11 * word_count`, not 128 bits.
**Suggested direction:** Either restrict to BIP39's actual word counts (12, 15, 18, 21, 24) or document that this is a *passphrase generator inspired by BIP39* (using its wordlist) rather than a BIP39 mnemonic generator. The latter is honest about what the code actually does.

### P3 — `StrengthEstimate::guesses_log10` uses base-10 while the spec talks bits

**File(s):** `crates/relicario-core/src/generators.rs:111-113`
**Observation:** `guesses_log10: f64` is base-10 log of guess count; the design spec discusses entropy in bits. Mild domain-translation friction for callers.
**Suggested direction:** Add a one-line comment showing the bits conversion (`bits ≈ guesses_log10 * log2(10) ≈ guesses_log10 * 3.32`), or expose a `bits_estimate()` accessor.

## File-by-file walk

**`lib.rs` (99 lines).** Crate-level docs are tight and accurate; the crypto pipeline diagram in the header is the right thing to greet a newcomer with. Public re-exports surface every meaningful type from one location. Clear.

**`error.rs` (195 lines).** Single error enum with thiserror. Every variant carries helpful context (item id, byte counts, found/expected version) except `Decrypt` which is opaque on purpose (audit M4). Tests exercise the public message format. Reads well.

**`crypto.rs` (437 lines).** The cornerstone. Module-level doc explains why XChaCha over AES-GCM and the binary layout. `derive_master_key` does NFC normalization + length-prefixed concatenation, both with explicit "audit H1" provenance comments. `decrypt_v1` rejection is tested. `derive_master_key_raw` is the seam used by the recovery QR. Solid.

**`ids.rs` (161 lines).** `ItemId`/`FieldId` are 64-bit random hex; `AttachmentId` is content-addressed (SHA-256 truncated to 128 bits). `is_valid()` provides a path-traversal guard (tested for `../../etc`). Header comment cites the audit IDs (M8, I2/B4) that motivated the entropy bumps from v1 — that traceability is great.

**`time.rs` (63 lines).** `now_unix()` and `MonthYear`. Only blemish is `MonthYear::new -> Result<_, &'static str>`; everything else returns `RelicarioError`.

**`vault.rs` (90 lines).** Six typed encrypt/decrypt wrappers (item / manifest / settings) that JSON-roundtrip through the crypto layer. Mechanical and correct. Stale module header (P2).

**`item.rs` (497 lines).** Defines the Item envelope, Field/FieldKind/FieldValue, Section, FieldHistoryEntry. Kind/value invariant enforced at construction and `validate()` post-deserialization. `set_field_value` captures history for password/concealed/totp kinds with kind-change rejection. `prune_history` honors LastN/Days/Forever. Mid-file `use` block (P3). Inline `base32_encode` (P2 above). Otherwise solid; the test at `set_field_value_captures_history_for_password` is exactly the right shape.

**`manifest.rs` (159 lines).** Browse-without-decrypt index v2. `upsert`/`remove`/`get`/`search`. `derive_icon_hint` only handles Login (P3). `r#type` field (P3). Stale header (P2).

**`attachment.rs` (166 lines).** `AttachmentRef` (carried on Item) and `AttachmentSummary` (carried in Manifest) plus `encrypt_attachment`/`decrypt_attachment`. The cap check fires before any crypto work — good order. Stale header + WHAT-comments (P2 + P3).

**`settings.rs` (184 lines).** `VaultSettings` with `TrashRetention`, `HistoryRetention`, `GeneratorRequest`, `AttachmentCaps`, plus the autofill TOFU ack map. Defaults match the design spec. `should_purge` is tested at the day-boundary. Clear.

**`generators.rs` (269 lines).** CSPRNG passwords (rejection-sampled via `Uniform::from`) and BIP39 passphrases (with capitalization variants) plus a zxcvbn-backed strength gate. Solid except the BIP39 lower-bound and `guesses_log10` ergonomics (both P3).

**`imgsecret.rs` (1138 lines).** The novel component. Documentation is excellent — DCT, QIM, EMBED_POSITIONS, MAX_DIMENSION rejection, JPEG header peek (audit M3), even an inline ITU-R BT.601 derivation. Tests cover round-trip, recompression to Q85, 10% crop, oversized-header rejection, and synthetic JPEG generation. Three real concerns: dead `region_width`/`region_height` (P2), narrower-than-spec crop recovery (P2), unannotated `unwrap()` in `read_block` (P3). The 1100-line size is fine — the algorithm warrants it.

**`backup.rs` (348 lines).** `.relbak` container: magic + version + salt + nonce + zstd(JSON envelope). Pinned Argon2id params. Schema/format versioning is paranoid in the right places. Reference-JPEG embedding is base64-in-JSON (P2). Otherwise solid.

**`device.rs` (168 lines).** ed25519 keypair generation in OpenSSH format, sign/verify, and SHA-256 fingerprint. The ssh-key + ed25519-dalek choreography is awkward but unavoidable. Sign/verify share an extraction pattern (P3). Tests cover sign, verify, wrong-data, wrong-key, and fingerprint format/determinism — comprehensive.

**`recovery_qr.rs` (129 lines).** The doc-gap finding (P1). Mechanically correct: domain-separated KDF input, AEAD wrap of the 32-byte image_secret, 109-byte payload that fits a QR v40 at EcLevel::M, SVG render via `qrcode`. But the documentation density doesn't match the rest of the crate.

**`import_lastpass.rs` (220 lines).** Header validation is strict (column count + order). Per-row failures degrade gracefully into `ImportWarning` rather than aborting. SecureNote vs Login dispatch via the `http://sn` sentinel matches the spec (D10). Custom base32 decoder (P2). Otherwise clean.

**`tar_safe.rs` (138 lines).** Replaces `tar::Archive::unpack` with a validated extractor. Rejects `..`, absolute paths, Windows prefixes, symlinks, hardlinks, oversized claimed sizes, and cumulative-size bombs. Returns `(PathBuf, Vec<u8>)` to the caller. Surgical and well-scoped.

**`item_types/mod.rs` (127 lines).** `ItemType` and `ItemCore` enums plus a `pub use` of every per-type core. The `// INVARIANT: no *Core struct may have a field serialized as "type"` comment at line 38 is exactly the kind of cross-cutting note that earns its keep — preserving that convention is what `ItemCore`'s tag-based serde works against. Comprehensive round-trip test at `item_core_round_trips_for_all_seven_types`.

**`item_types/login.rs` (63 lines).** Username/password/url/totp, all Optional, password Zeroizing. `omitted_fields_dont_appear_in_json` is the right shape for a serde test.

**`item_types/secure_note.rs` (30 lines).** Just a Zeroizing body. Right-sized.

**`item_types/identity.rs` (45 lines).** Five Optional fields. `empty_identity_omits_all_fields` round-trips to `{}` — clean.

**`item_types/card.rs` (68 lines).** Number/holder/expiry/cvv/pin/kind. `CardKind` defaults to `Credit`. `MonthYear` reused from `time.rs`.

**`item_types/key.rs` (42 lines).** Key material as Zeroizing string + label/public_key/algorithm. Loose schema (algorithm is a free string), which is appropriate for "any kind of key material."

**`item_types/document.rs` (40 lines).** Filename + mime + AttachmentId pointer to the primary blob. The actual bytes live in the attachment store, not the item.

**`item_types/totp.rs` (293 lines).** TotpCore + the shared TotpConfig (also reused as a field on Login). RFC6238 SHA1 vector check, an independent reference impl for Steam, an alphabet exhaustiveness sweep. HOTP is rejected with a typed error rather than silently mis-counted — the right choice for a stateless library. DT extraction duplicated four times (P3). Source vs test comment disagreement on the '5' character (P3).

**Tests folder (9 files, ~1.1 kLOC).** `integration.rs` covers the full encrypt/decrypt pipeline plus two-factor independence. `format_v2.rs` pins the version byte, the v1-rejection error type, and the length-prefix domain separation. `field_history.rs` covers capture, prune, and survival across encrypt/decrypt. `attachments.rs` covers round-trip + AID determinism + cap. `backup.rs` is thorough — round-trip with and without reference image / git archive, bad magic, unsupported version, wrong passphrase, truncation, tag tamper, NFC/NFD passphrase round-trip. `generators.rs` does class-balance statistics across 10k chars (well-documented why aggregating, since per-call cap is 128). `import_lastpass.rs` is the single largest suite (~30 cases) and exercises every column-mapping edge. `recovery_qr.rs` is minimal but covers the essentials. `safe_unpack.rs` builds raw-byte tars by hand to bypass `tar::Builder`'s sanitizer — exactly the right way to test a security boundary. The `fast_params()` helper is repeated across most files; a `tests/common/mod.rs` could DRY it but it's not a real cost.

## Beginner-friendliness assessment

For a competent dev who has never written Rust, this crate is unusually approachable. The boundary discipline is consistent (no I/O anywhere), the public surface is enumerated in one place (`lib.rs`), the module names map directly to vault concepts a reader already understands (item, manifest, attachment, settings, generators, vault, backup, device, recovery_qr), and the most algorithmically dense file (`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` to see the whole pipeline run, all in one sitting. The Rust idioms that would trip up a newcomer (deref-coercion, `r#type` raw identifiers, `Zeroizing` wrappers, `&**master_key`) are all present, but they cluster in patterns a reader will see often enough to absorb.

The single change that would help most: write `recovery_qr.rs` to the same documentation standard as `crypto.rs` and `backup.rs`. It's the only file where a learning reader will hit a wall. Closing that gap brings the floor up to the ceiling and makes the "read it like a security proof" pitch true everywhere, not just in 16 of 17 files.