<!-- review: timestamp=2026-05-25T20-41-16Z  repo=danieljustus/symaira-vault  head=6ba52495cdccd0361f734b33fc9c23f563463616 -->

# Code Review — 2026-05-25

## Scope
Full-stack review of the Symaira Vault Go CLI project (`internal/` and `cmd/`). Analyzed: security-sensitive paths, architecture/simplification opportunities, performance hot paths, and UX/DX surface. Evidence drawn from source code, README, CONTRIBUTING.md, config files, and CI workflows.

---

## Security

- [ ] **[Security] Memory fallback keyring returns encrypted payload as decrypted passphrase**
  - **Status quo:** `internal/session/memory_keyring.go:140-146` — the `Get()` method decrypts the passphrase, then immediately calls `zeroBytes(plain)` before returning. It then returns `string(newPayload)` (the still-encrypted payload) as if it were the decrypted passphrase. Any caller expecting the raw passphrase from the memory fallback receives encrypted garbage. This silently breaks the in-memory session cache when OS keyring is unavailable.
  - **Proposed solution:** Remove the premature `zeroBytes(plain)` call on line 145. Wipe the plaintext only after it has been returned to the caller (or use a `copy` to a caller-provided buffer). Add a unit test asserting that `memoryKeyring.Get()` returns the original plaintext, not encrypted data.
  - **Effort/Impact:** Low effort / Critical impact

- [ ] **[Security] Audit HMAC key falls back to plaintext storage on unsupported platforms**
  - **Status quo:** `internal/audit/keystore_fallback.go:58` — when the OS keyring is unavailable and `identity` is nil (e.g., FreeBSD without cgo, or when the vault identity hasn't been loaded), the audit HMAC key is written to disk as raw bytes via `os.WriteFile(keyPath, key, 0o600)`. File permissions are 0600 but the key is unencrypted at rest. This breaks the tamper-evidence guarantee for audit logs on affected platforms.
  - **Proposed solution:** Derive an encryption key from machine-local entropy (e.g., `crypto/rand` + `scrypt` with a machine-unique salt) when the age identity is unavailable. Store the encrypted key. Or: require identity to be available before audit logging is enabled, and fail closed (disable audit logging with a warning) when identity is nil.
  - **Effort/Impact:** Medium effort / High impact

- [ ] **[Security] Error messages leak vault directory paths in session operations**
  - **Status quo:** `internal/session/session.go:281,291,324,417,427` — error messages like `"session expired for vault %s"`, `"no wrap key available for vault %s"`, and `"identity cache expired for vault %s"` embed the raw vault directory path. When these errors are logged or returned via MCP, they reveal the filesystem location of the vault.
  - **Proposed solution:** Replace raw `vaultDir` in user-visible error messages with a sanitized identifier (e.g., `filepath.Base(vaultDir)` or a short hash). Keep the full path in structured logs at `Debug` level only.
  - **Effort/Impact:** Low effort / Medium impact

---

## UX/DX

- [ ] **[UX/DX] Exit code 6 is undefined in shared errors package, breaking scriptability**
  - **Status quo:** `cmd/admin/config.go:22` defines `exitConfigError ExitCode = 6` locally. `internal/errors/errors.go:13-30` documents exit codes 0-5 and 7-8, but omits 6 entirely. Scripts relying on exit code documentation cannot discover that `config validate` returns 6 on validation failure.
  - **Proposed solution:** Add `ExitConfigError ExitCode = 6` to `internal/errors/errors.go`, document it in the package-level comment, and update `cmd/admin/config.go` to reference the shared constant.
  - **Effort/Impact:** Low effort / Medium impact

- [ ] **[UX/DX] Update-available conflated with general error (exit code 1)**
  - **Status quo:** `cmd/admin/update.go:59` returns `errUpdateAvailable = errors.NewCLIError(1, ...)` — exit code 1 is also `ExitGeneralError`. Scripts cannot distinguish "a new version is available" from "something went wrong".
  - **Proposed solution:** Define a dedicated exit code `ExitUpdateAvailable = 10` (or similar, outside the 0-8 range) in `internal/errors/errors.go`. Update the update command to use it. Document the new code.
  - **Effort/Impact:** Low effort / Medium impact

- [ ] **[UX/DX] Inconsistent default vault path across code, README, and config example**
  - **Status quo:** Three different default paths are documented: `~/.symvault` (`internal/config/config.go:7`), `~/.openpass` (README, `internal/cli/vaultpath.go`), and `~/.symaira` (`config.yaml.example:6`). New users don't know which location is canonical.
  - **Proposed solution:** Pick one canonical default (`~/.symvault` to match the binary name). Update `config.yaml.example`, README, CONTRIBUTING.md, and `internal/cli/vaultpath.go` to reference it consistently.
  - **Effort/Impact:** Low effort / Medium impact

---

## Architecture

- [ ] **[Architecture] Unnecessary vaultsvc.Service interface with single implementation**
  - **Status quo:** `internal/vaultsvc/service.go` defines a `Service` interface with 11 methods. There is exactly one production implementation (`vaultService`) and one manually-maintained `MockService` (`mock.go:115` lines). The package is a thin wrapper around `vaultpkg` that adds auto-commit and list cache invalidation. All 12 cmd/ callers use it via `cli.WithVault`.
  - **Proposed solution:** Remove `internal/vaultsvc/` entirely. Move auto-commit logic into `vaultpkg.WriteEntry`/`DeleteEntry` (or into `cli.WithVault`). Have cmd/ commands work with `*vaultpkg.Vault` directly. Delete the mock — tests can use a real vault in a temp directory.
  - **Effort/Impact:** Medium effort / High impact (eliminates 367 lines + 1 package)

- [ ] **[Architecture] Duplicate output printer in cmd/ and internal/cli/**
  - **Status quo:** `cmd/output.go` and `internal/cli/output.go` both define an identical `Printer` interface plus `TextPrinter`, `JSONPrinter`, `YAMLPrinter`, `NewPrinter()`, and `PrintResult()`. The `cmd` version wraps the `cli` version but also carries a deprecated `PrintJSON` function. This is pure dead code duplication.
  - **Proposed solution:** Delete `cmd/output.go`. Redirect any remaining callers to `cli.PrintResult()`. Remove the deprecated `PrintJSON` wrapper.
  - **Effort/Impact:** Low effort / Low impact

- [ ] **[Architecture] Two independent error systems**
  - **Status quo:** `internal/errors/errors.go` defines `CLIError` with `ErrorKind` and exit codes for the CLI layer. `internal/mcp/errors/codes.go` defines `MCPError` with string error codes for the MCP layer. Both define semantically overlapping errors (`ErrEntryNotFound`, `ErrFieldNotFound`, etc.) independently. Adding a new error concept requires updating both packages.
  - **Proposed solution:** Make `MCPError` wrap `CLIError` (or a shared base error) where the semantics overlap. Define a single source of truth for "entry not found", "field not found", etc. Keep MCP-specific codes (tool errors, auth errors) in the MCP package.
  - **Effort/Impact:** Medium effort / Medium impact

---

## Performance

- [ ] **[Performance] Search index rebuild decrypts all entries on every write**
  - **Status quo:** `internal/vault/search_index.go:67-78` — `Build()` iterates over every entry, decrypts it, unmarshals JSON, collects string values, re-serializes to JSON, then encrypts the result. For a 50k-entry vault this is 50k decrypt + 50k unmarshal + 1 full marshal + 1 encrypt. `InvalidateSearchIndex()` (`search_index.go:187-194`) is called on every write, so the first search after any write pays this full cost.
  - **Proposed solution:** Replace the full-nuke invalidation with incremental index updates. On write, only re-decrypt and re-index the affected entry paths. This reduces the penalty from O(N) to O(1) per write.
  - **Effort/Impact:** Medium effort / High impact

- [ ] **[Performance] Short 30s cache TTL causes repeated decryption in long-running MCP sessions**
  - **Status quo:** `internal/vault/search.go:144-152,222-242` — both `listCache` and `pseudonymizedCache` use a 30-second TTL. For MCP server sessions that run for hours, entries expire frequently, forcing repeated directory walks and full decryption on every search.
  - **Proposed solution:** Increase the TTL to 300s (5 minutes). Explicit invalidation on writes already handles correctness — the short TTL adds no safety, only overhead.
  - **Effort/Impact:** Low effort / Medium impact

- [ ] **[Performance] Legacy migration runs on every vault open, adding 100-500ms startup latency**
  - **Status quo:** `internal/vault/vault.go:97-98` — `migrateLegacyEntries()` is called inside `Open()` on every vault unlock. It walks the entire vault directory via `filepath.Walk`. For legacy users with 10k+ entries this adds 100-500ms to every unlock.
  - **Proposed solution:** Move migration out of the hot path. Run it lazily on first write operation, or gate it behind a `needs_migration` flag in the vault config. Once migration completes, set the flag to false and skip on subsequent opens.
  - **Effort/Impact:** Low effort / Medium impact

---

## Best First Quick Win

Fix the **memory fallback keyring bug** (`internal/session/memory_keyring.go:140-146`). It is a one-line change (remove premature `zeroBytes`) plus a unit test, but it fixes a broken fallback path that affects all users on platforms without OS keyring support.

Next: run `/02-gh-plan --from-review .github/review/2026-05-25T20-41-16Z.md` to turn these TODOs into GitHub issues.
