<!-- review: timestamp=2026-05-26T14-58-51Z  repo=danieljustus/symaira-vault  head=f2ad6b5270de45419f125a7c4be8d726790cf337 -->

# Code Review — 2026-05-26

## Scope
Full codebase analysis of the Symaira Vault Go CLI — a password vault/manager with age encryption, MCP server, Touch ID, and multi-device Git sync. Analysis covered `cmd/`, `internal/` (38 packages), `main.go`, `go.mod`, `.github/workflows/ci.yml`, `.golangci.yml`, and configuration.

**Evidence base**: Entry points (`main.go`, `cmd/root.go`, `internal/cli/cli.go`), crypto paths (`internal/crypto/age.go`, `symmetric.go`, `argon2id.go`, `secstring_unix.go`), session management (`internal/session/session.go`, `oskeyring.go`, `memory_keyring.go`, `biometric.go`), vault core (`internal/vault/vault.go`, `entry.go`, `search.go`, `reencrypt.go`, `manifest.go`), audit (`internal/audit/audit.go`), config (`internal/config/config.go`), env filtering (`internal/envfilter/filter.go`), secrets execution (`internal/secrets/runner.go`), file I/O (`internal/fileutil/safewrite.go`), masking (`internal/masking/sanitizer.go`), CI pipeline (`.github/workflows/ci.yml`), and lint config (`.golangci.yml`).

---

## Security

- [ ] **[Security] Memory keyring fallback is silent — users may not know sessions are ephemeral**
  - **Status quo:** When the OS keyring is unavailable (`internal/session/oskeyring.go:43-45`), the session system silently falls back to `memoryKeyring` — an in-process, encrypted-but-ephemeral store. Sessions are lost on process exit. A log-level warning is emitted via `logging.Default().Warn()` (line 26), but this never reaches the user's terminal in typical usage. Commands that depend on a cached session (e.g., `symvault get`) will prompt for the passphrase again with no explanation of why the previous `symvault unlock` didn't persist.
  - **Proposed solution:** On the first fallback per process lifetime, print a one-line warning to stderr: `"Warning: OS keyring unavailable — session will clear when this process exits. Run 'symvault doctor' for help."` The `memory_init.go` or `oskeyring.go` init function can do this. Additionally, surface the keyring health status in `symvault auth status` so users can proactively check.
  - **Effort/Impact:** Low effort / Medium impact (improves trust and debuggability; reduces support burden for "why do I keep getting asked for my passphrase?")

- [ ] **[Security] Environment variable passphrase ingestion cannot be disabled by config**
  - **Status quo:** `internal/cli/unlock.go:87-96` reads the passphrase from `SYMVAULT_PASSPHRASE` (or legacy `OPENPASS_PASSPHRASE`) as a non-interactive unlock mechanism. While the code wipes the byte slice and unsets the env var after use, the passphrase is still visible to any process on the system during the CLI's lifetime, and shell histories commonly contain `export SYMVAULT_PASSPHRASE=...`. There is no config toggle to disable this path for security-conscious users or headless CI environments that should use Touch ID or keyring exclusively.
  - **Proposed solution:** Add a `security: { disable_env_passphrase: true }` config option in `internal/config/config.go`. When set, `resolveUnlockPassphrase()` in `internal/cli/unlock.go` skips the env var check and falls through to interactive/bio unlock. The `symvault doctor` command can surface this as a recommendation for users in CI environments.
  - **Effort/Impact:** Low effort / Medium impact (reduces attack surface for passphrase-in-env; config flag is trivial, the unlock flow already has the branching structure)

- [ ] **[Security] Legacy environment variable names (`OPENPASS_*`) remain active alongside new names**
  - **Status quo:** The codebase supports both `SYMVAULT_*` and `OPENPASS_*` environment variables via `envutil.Getenv()` (`internal/envutil/`), including `OPENPASS_PASSPHRASE`, `OPENPASS_AGENT`, and `OPENPASS_AUDIT_*`. While documented as backward compatibility, maintaining both names indefinitely increases the attack surface — users who think they've fully migrated may still have old env var names active in shell profiles, systemd units, or launchd plists. The `OPENPASS_PASSPHRASE` smoke test in CI (`.github/workflows/ci.yml:350`) perpetuates the legacy name.
  - **Proposed solution:** Add a deprecation notice printed to stderr when any `OPENPASS_*` env var is consumed (one-shot per process). Plan a removal timeline (e.g., 3 releases) and add a `config.yaml` migration utility that helps users find and replace legacy var names. Update the CI smoke test to use `SYMVAULT_PASSPHRASE`.
  - **Effort/Impact:** Low effort / Medium impact (deprecation warnings are straightforward; eventual removal reduces code paths and cognitive load)

---

## UX/DX

- [ ] **[UX/DX] `config.yaml.example` is not surfaced during vault initialization**
  - **Status quo:** `config.yaml.example` is a well-commented 136-line reference, but `symvault init` creates only a minimal `config.yaml` with defaults. New users aren't told about the example, so they may never discover agent profiles, clipboard timeout tuning, audit config, or custom scan patterns. The `symvault setup` wizard (if it exists as an interactive TUI path) may cover some of this, but the non-interactive path misses it entirely.
  - **Proposed solution:** After `symvault init` completes, print a hint: `"Tip: See config.yaml.example for agent profiles, clipboard timeout, and other settings — copy it to ~/.symvault/config.yaml as a starting point."` Alternatively, embed a commented config template in the binary (via `embed`) and offer `symvault config init --full` to write a fully commented config.
  - **Effort/Impact:** Low effort / Medium impact (one-time hint or small command addition; dramatically improves discoverability of advanced features)

- [ ] **[UX/DX] Error output uses mixed formatting approaches**
  - **Status quo:** `internal/cli/cli.go:103` uses `cliout.Errorf("Error: %v", err)` for top-level error formatting. But individual subcommands spread across `cmd/` and `internal/` use `fmt.Fprintf(os.Stderr, ...)` directly (e.g., `internal/cli/unlock.go:68`, `internal/audit/audit.go:423`). This leads to inconsistent error formatting — some errors get the `cliout` color/structure treatment, others are raw stderr writes. Users see different error styles depending on which code path triggered the failure.
  - **Proposed solution:** Standardize all user-facing output through `cliout` or a dedicated output interface. For packages in `internal/` that need to emit warnings (like the `audit` package), pass an `io.Writer` or use structured logging instead of direct `fmt.Fprintf(os.Stderr)`. The `symvault doctor` command already demonstrates this pattern — extend it.
  - **Effort/Impact:** Medium effort / Low impact (many call sites to update; payoff is aesthetic consistency rather than functional improvement)

- [ ] **[UX/DX] Vault directory default path is duplicated and inconsistent**
  - **Status quo:** The default vault path `~/.symvault` appears in at least three places: `internal/cli/cli.go:131` (flag default), `internal/config/config.go:7` (`defaultConfigDir`), and `internal/audit/audit.go:29` (`defaultDirName`). The `config.yaml.example:6` also references `~/.symvault`. If `config.go` is changed, the others may drift. The audit package constructs `~/.symvault` from `$HOME` + `.symvault` independently rather than deferring to the config package's default.
  - **Proposed solution:** Use a single exported constant in `internal/config/` (e.g., `DefaultVaultSubdir = ".symvault"`) and reference it everywhere. The `VaultPath()` function in `internal/cli/vaultpath.go` already has path resolution logic — extend it to be the single source of truth for vault directory resolution.
  - **Effort/Impact:** Low effort / Medium impact (consolidation prevents drift; easy to implement but requires touching multiple files)

---

## Architecture

- [ ] **[Architecture] `WithVault` and `WithVaultRaw` have identical implementations**
  - **Status quo:** `internal/cli/unlock.go:114-129` (`WithVault`) and `lines 131-146` (`WithVaultRaw`) contain identical code: both resolve vault path, check initialization, call `UnlockVault(vaultDir, true)`, and invoke the callback. One is used by MCP-token-authenticated code paths and the other by standard interactive commands, but at this abstraction level they are interchangeable.
  - **Proposed solution:** Have `WithVaultRaw` delegate to `WithVault` (or vice versa), or merge into a single function. If the semantic distinction ("raw" = non-interactive MCP context) is important, add a parameter or use the agent context to choose the unlock mode rather than duplicating the function.
  - **Effort/Impact:** Trivial effort / Low impact (one-line change; minor but clears a maintenance landmine)

- [ ] **[Architecture] 38 internal subpackages — some with very low cohesion**
  - **Status quo:** The `internal/` directory contains 38 packages. Several have only 2–4 files with limited functionality: `internal/secrets/` (run command + resolver, 4 files), `internal/envfilter/` (env whitelist, 2 files), `internal/masking/` (PII sanitization, 3 files), `internal/errors/` (error types), `internal/pathutil/` (path validation). Go encourages small packages, but the sheer number creates navigational overhead for contributors and makes it harder to see which packages are "core" vs. utility. The `internal/` prefix also means there's no public API boundary — everything is equally internal.
  - **Proposed solution:** Consolidate closely related utility packages. For example: merge `pathutil` + `fileutil` into a single `fsutil` package; fold `envfilter` into `secrets` (since it serves the run command); move `masking` into `mcp` (its only consumer). This reduces package count by ~4–5 without changing functionality. Consider adding a `doc.go` in each remaining package describing its role.
  - **Effort/Impact:** Medium effort / Medium impact (mechanical refactor with import path updates; improves code discoverability and reduces decision fatigue on "where does X live?")

- [ ] **[Architecture] Config cache in `entry.go` has no eviction policy**
  - **Status quo:** `internal/vault/entry.go:168-214` defines a `configCache` (map + RWMutex) that memoizes parsed `Config` structs keyed by vault directory + mtime. Entries are never evicted — if a process opens multiple vault directories over time (e.g., via `--vault` flag changes), the cache grows without bound. For a long-running MCP server handling multiple vaults, this leaks config structs and their backing AgentProfile maps.
  - **Proposed solution:** Add a simple size cap (e.g., 16 entries) with LRU eviction, or a TTL-based sweep goroutine that cleans entries older than 1 hour. The existing `InvalidateConfigCache()` function already provides manual eviction — adding automatic eviction complements it.
  - **Effort/Impact:** Low effort / Low impact (minor memory leak; affects only long-running multi-vault use cases)

---

## Performance

- [ ] **[Performance] Field search requires decrypting all entries in large vaults**
  - **Status quo:** `internal/vault/search.go` describes the two-pass search: path matching (fast, no decryption) and field content search (slow, decrypts every entry not matched by path). The design document acknowledges this as the primary bottleneck and discusses an encrypted search index as future work. Even with 4 concurrent decrypt workers, a 50k-entry vault takes 2–4 seconds for field search — acceptable for interactive CLI use but painful for MCP agents that issue search tool calls frequently.
  - **Proposed solution:** Implement the encrypted search index proposed in the design doc comments (`search.go:48-80`). Store term→entry-path mappings encrypted with a key derived from the vault identity. Build the index lazily on first search and invalidate on writes. Measure impact: target <500ms field search on 50k entries. The design doc already covers the security tradeoffs (search pattern leakage, encrypted-at-rest requirement) — this is a matter of prioritization.
  - **Effort/Impact:** High effort / High impact (largest single improvement for vault scalability; requires careful design of index encryption scheme and invalidation strategy)

- [ ] **[Performance] Manifest rebuild on first open walks entire entries directory serially**
  - **Status quo:** `internal/vault/vault.go:104-105` rebuilds the manifest on `Open()` if `manifest.age` is missing. `internal/vault/manifest_updater.go` walks all `.age` files in `entries/` serially, computing SHA-256 hashes of each file. For a vault with 10k+ entries (or on slow filesystems like networked Git repos), this single-threaded walk adds latency to startup.
  - **Proposed solution:** Parallelize the manifest rebuild using the same bounded worker pool pattern already established in `search.go` and `reencrypt.go` (4 workers). Compute SHA-256 hashes in parallel, collect results into the manifest map under a mutex. Also add a `--skip-manifest` flag for `symvault open` to skip this step when speed matters more than integrity verification.
  - **Effort/Impact:** Medium effort / Medium impact (reuses existing concurrency patterns; improves cold-start time for large vaults)

- [ ] **[Performance] `ReencryptAll` reads all files into memory before processing**
  - **Status quo:** `internal/vault/reencrypt.go:45-58` calls `filepath.Walk` to collect all `.age` file paths into a slice before dispatching worker tasks. For a vault with 100k entries, this allocates a 100k-entry slice plus the concurrent decrypt/encrypt buffers. The walk itself is fine (it's lazy), but materializing the full path list before any work starts adds latency and memory pressure.
  - **Proposed solution:** Stream file paths into the worker pool using a channel rather than collecting all paths first. The walk goroutine sends paths into a buffered channel; workers receive and process. This overlaps I/O (walk) with CPU (decrypt), starts producing results sooner, and reduces peak memory for large vaults.
  - **Effort/Impact:** Medium effort / Medium impact (channels-based pipeline is idiomatic Go; improves responsiveness for large re-encryption operations)

---

## Conclusion

**Best first quick win: [UX/DX] Show a hint about `config.yaml.example` after `symvault init`.**

This is a one-line change that dramatically improves feature discoverability — users who would otherwise never find agent profiles, clipboard timeout, or audit tuning immediately see the path. Combined with the memory keyring fallback warning (Security #1), these two low-effort changes address the highest-impact papercuts for new users.

**Runner-up: Deprecate `OPENPASS_*` env vars with stderr warnings and a removal timeline.** The dual-namespace environment variables create subtle failure modes and increase attack surface. A deprecation cycle (warn → remove in 3 releases) is low-risk and simplifies the security model.

**For the next iteration:** Prioritize the encrypted search index (Performance #1). This unlocks the MCP agent use case at scale and has been deferred in the design doc since the initial implementation. The groundwork (identity-derived keys, atomic writes, worker pool patterns) is already in place.
