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.