<!-- review: timestamp=2026-05-18T20-50-00Z  repo=danieljustus/OpenPass  head=a6736af9e82c090cdcb25267c576558750fa5a16 -->

# Code Review — 2026-05-18

Analyzed scope: Full OpenPass repository (Go CLI password manager with MCP server, TUI, and AI agent integration). Evidence drawn from source code, config files, build system, and documentation.

Main evidence base: `cmd/` (107 files, 41 commands), `internal/` (36 packages, 1,900+ files), `go.mod`, `Makefile`, `README.md`, `ARCHITECTURE.md`, `SECURITY.md`, `config.yaml.example`, CI workflows.

---

## Security

- [ ] **[Security] `OPENPASS_MCP_TOKEN` env var never unset after reading**
  - **Status quo:** `cmd/root.go:379-383` unsets `OPENPASS_PASSPHRASE` after reading via `os.Unsetenv()`, but `internal/mcp/token.go:700-707` reads `OPENPASS_MCP_TOKEN` without ever unsetting it. The token remains visible in `/proc/self/environ` and is inherited by all child processes (git, editors, subprocesses) unless filtered by `envfilter`.
  - **Proposed solution:** Mirror the `OPENPASS_PASSPHRASE` pattern: read `OPENPASS_MCP_TOKEN` once at startup, then `os.Unsetenv()` it. Document the behavior in `SECURITY.md` alongside the existing passphrase guidance.
  - **Effort/Impact:** Low effort / high impact

- [ ] **[Security] Audit and grant HMAC keys stored as plaintext in fallback mode**
  - **Status quo:** `internal/audit/keystore_fallback.go` stores the 32-byte audit HMAC key at `auditDir/audit-hmac-key` with file mode `0o600`. `internal/mcp/grant_keys_fallback.go` stores the grant signing key at `vaultDir/grant-signing-key` with the same mode. While `0o600` restricts access to the owner, the keys are not encrypted at rest — a process with the user's UID can read them directly.
  - **Proposed solution:** Encrypt the fallback keys with the vault's age identity (the same key that decrypts entries). Add `LoadEncryptedKey()`/`SaveEncryptedKey()` helpers in `internal/crypto/` and migrate both keystore implementations. This aligns with the vault's threat model where `identity.age` is the root of trust.
  - **Effort/Impact:** Medium effort / high impact

- [ ] **[Security] Manifest update extends write-lock hold time on every entry operation**
  - **Status quo:** `internal/vault/entry.go:320-328` calls `UpdateManifestEntry()` inside the `AcquireWriteLock()` critical section. `internal/vault/manifest.go:105` does a full read-decrypt-unmarshal-modify-marshal-encrypt-write cycle (6+ I/O operations) while holding the write lock, blocking all concurrent writes. For batch operations (import, re-encrypt), this serializes what could be parallel work.
  - **Proposed solution:** Defer manifest updates to a background goroutine with a buffered channel, or batch them with a dirty flag and periodic flush. Release the write lock immediately after the entry file is atomically written; update the manifest outside the lock.
  - **Effort/Impact:** Medium effort / medium impact

## UX/DX

- [ ] **[UX/DX] No `openpass config set` subcommand for CLI-based config editing**
  - **Status quo:** Users must hand-edit `~/.openpass/config.yaml` or re-run the full `openpass setup` wizard to change any configuration value. For a CLI-first tool, this is a notable friction point — there is no equivalent of `git config set key value`.
  - **Proposed solution:** Add `openpass config set <key> <value>` and `openpass config get <key>` commands. Parse the existing YAML, modify the specified dotted key path, and write back with atomic file operations. Add tab-completion for known config keys.
  - **Effort/Impact:** Low effort / high impact

- [ ] **[UX/DX] Passphrase minimum length only enforced after user types it**
  - **Status quo:** `cmd/init.go:64` rejects passphrases shorter than 12 characters with `return fmt.Errorf("passphrase must be at least 12 characters")`, but only after the user has already typed and confirmed the passphrase. The user must re-type from scratch.
  - **Proposed solution:** Add an upfront hint before passphrase entry (e.g., "Choose a passphrase (minimum 12 characters):") and validate on first entry before confirmation, not after confirmation.
  - **Effort/Impact:** Low effort / medium impact

- [ ] **[UX/DX] Single `cmd` package with 107 files — approaching unmaintainable**
  - **Status quo:** All 41 cobra commands live in a single `cmd` package with 107 `.go` files. Cross-references between files are dense, and new contributors must navigate a flat directory with no domain grouping.
  - **Proposed solution:** Split into domain sub-packages: `cmd/crud/` (add/get/set/list/find/edit/delete), `cmd/mcp/` (serve, mcp-config, token), `cmd/admin/` (backup, import, doctor, migrate), `cmd/auth/` (unlock, lock, auth). Keep `cmd/root.go` as the shared entry point. This is a pure move refactor with no behavior change.
  - **Effort/Impact:** Medium effort / medium impact

## Architecture

- [ ] **[Architecture] `doctor.go` monolith — 1,434 lines with 30+ health checks**
  - **Status quo:** `internal/health/doctor.go` contains all health check functions (vault, git, crypto, MCP, session, permissions, config), the orchestration loop, filtering logic, and result formatting in a single file. New checks require modifying this file, increasing merge conflict risk.
  - **Proposed solution:** Split into domain-grouped files: `doctor_vault.go`, `doctor_git.go`, `doctor_crypto.go`, `doctor_mcp.go`, `doctor_session.go`. Keep `doctor.go` for shared types and the orchestration loop only.
  - **Effort/Impact:** Low effort / medium impact

- [ ] **[Architecture] Config mirror struct pattern creates dual-maintenance burden**
  - **Status quo:** `internal/config/config.go` defines `AgentProfile` (with bare types) and `fileAgentProfile` (with pointer types) — 18 identically-named fields duplicated. Adding a new config field requires changes in 3 places: the main struct, the file struct, and the `Load()` mapping logic. Several fields already show drift between the two structs.
  - **Proposed solution:** Eliminate the mirror struct. Unmarshal YAML directly into `Config` using `omitempty` tags and a `SetDefaults()` post-processing step. This is the standard Go pattern and removes the pointer-to-bool/string workaround for "not set" detection.
  - **Effort/Impact:** Medium effort / high impact

## Performance

- [ ] **[Performance] Manifest read-modify-write amplifies I/O on every entry operation**
  - **Status quo:** Every call to `WriteEntry()` triggers `UpdateManifestEntry()`, which performs a full read-decrypt-unmarshal-modify-marshal-encrypt-write cycle. For a batch import of 100 entries, this is 600+ I/O operations on the manifest file alone, all while holding the write lock.
  - **Proposed solution:** Batch manifest updates with a dirty flag and a 100ms-500ms debounced flush goroutine, or append-only manifest entries with periodic compaction. Release the write lock before manifest writes.
  - **Effort/Impact:** Medium effort / high impact

- [ ] **[Performance] `ReencryptAll` is fully sequential despite having a parallel worker pattern**
  - **Status quo:** `internal/vault/reencrypt.go:55` re-encrypts entries one at a time in a single goroutine. `internal/vault/search.go` already implements a bounded worker pool (default 4 workers) for parallel decryption during search — the same pattern could be reused here.
  - **Proposed solution:** Extract the bounded worker pool from `search.go` into a shared `internal/vault/workerpool.go` utility, then use it in `ReencryptAll` to decrypt and re-encrypt entries in parallel. Each entry is independently encrypted, so there are no cross-entry dependencies.
  - **Effort/Impact:** Low effort / medium impact

---

## Conclusion

**Best first quick win:** Add `openpass config set <key> <value>` — low effort, high user impact, and it leverages the existing config infrastructure without touching crypto or security-critical paths.

**Second quick win:** Unset `OPENPASS_MCP_TOKEN` after reading — a one-line fix (`os.Unsetenv()`) that closes a real secret-leakage vector to child processes.

**Suggested next step:** Run `/02-gh-plan --from-review .github/review/2026-05-18T20-50-00Z.md` to convert these TODOs directly into GitHub issues.

**Note:** `.github/review/` is not versioned. Consider adding it to `.gitignore`:
```gitignore
.github/review/
```
