# Staff Engineer Critique — sopsy (working tree as of 2026-07-02)

This is a better-than-average CLI codebase: the module boundaries are clean, the external-tool wrappers are consistently injectable for tests, the integration suite drives real `sops`/`age`/`git` end to end, and the documentation is unusually honest about the crypto model ("reading == re-granting"). Clippy is clean, `cargo test` passes 240+ tests, and the rollback snapshot in `recipient.rs` shows someone thought about partial failure. That is exactly why the remaining problems matter: a secrets tool is judged by its failure paths, and the failure paths here are where it leaks plaintext, grants access it claims to have rolled back, silently skips files during re-keying, and — with the new `.sopsy.sha` sidecar — breaks its own flagship self-service workflow the first time two join PRs merge. Every finding below was verified against the working tree; several were reproduced with live experiments. Fix the top five before this ships.

## Issues Found

1. **[CRITICAL] Failed `init` leaves plaintext secrets in a committable `.env.encrypted`** — `src/commands/init.rs:92-96`, `init.rs:105-124`.
   Step 7 writes the seed plaintext (the user's real `.env` if present, per `read_seed`, `init.rs:362-372`) directly into `.env.encrypted`, then encrypts in place. If `sops` fails — realistic: `age-plugin-se` not installed while the supplied `--public-key` is an `age1se1…` key (init only preflights `sops`, `init.rs:54`), a pre-existing broken `.sops.yaml` kept without `--force`, or a typo'd key — the error propagates and the file is left containing the full plaintext under a name that screams "encrypted". The `.gitignore` step (step 8) runs *after* encryption, so on a first-run failure no ignore rules exist at all; and when they do exist, `!*.encrypted` explicitly un-ignores the file. Verified live: after a failed init, `.env.encrypted` contained `PASSWORD=supersecret`, `git check-ignore` said "NOT ignored", and no `.gitignore` had been written. Worse, re-running init after fixing the tool prints ".env.encrypted already present; leaving it untouched" (`init.rs:88-90`) — the recovery path *preserves* the leak. One `git add -A && git commit` later, plaintext secrets are in history.

2. **[HIGH] Rollback restores the configs but not the already-re-wrapped encrypted files** — `src/commands/recipient.rs:726-728` (per-file `?` abort), `recipient.rs:177-181`/`246-250`/`659-665`, `src/commands/approve.rs:123-127`, `ConfigSnapshot` at `recipient.rs:262-294`.
   `run_updatekeys` re-wraps files one at a time and aborts on the first failure. Files re-keyed before the failure are not restored — `ConfigSnapshot` only captures `.sopsy.yml`, `.sopsy.sha`, and `.sops.yaml`. Verified live with a fake `sops` that succeeds on `a.encrypted` and fails on `b.encrypted`: after `recipient add` printed "rolled back configuration changes — no recipient was added", `a.encrypted` was still re-wrapped for the new key. For `add`/`approve` this means the "not-added" recipient *can decrypt a subset of the secrets* while no config file records them — the exact inconsistency the snapshot comment (`recipient.rs:256-261`) promises can't happen. For `remove` it's inverted: the "still-a-member" recipient has already been stripped from some files' key stanzas.

3. **[HIGH] Re-keying silently skips nested encrypted artifacts: `.sops.yaml` regexes and `.sopsy.yml` globs disagree** — `src/commands/recipient.rs:792-819` (`expand_glob`, no `**`, `*` never crosses `/`), `src/config.rs:176-182` (default globs), `src/commands/init.rs:349-358` (`path_regex: '\.encrypted$'` matches *any* depth), `src/commands/secrets.rs:45-56` (output path unconstrained).
   `sops` encrypts by regex, which matches `secrets/prod.env.encrypted` at any depth; sopsy re-keys by single-level globs, which never do. So the tool's own `sopsy secrets encrypt -o secrets/prod.env.encrypted` produces a committed artifact that `approve`/`recipient add`/`recipient remove` never re-wrap. Verified live: with `.env.encrypted` and `secrets/prod.env.encrypted` both present, `recipient add` re-encrypted "1 file(s)" — only the root one. Consequences: a newly approved member cannot decrypt nested files (confusing, but visible), and a *removed* member keeps a live key stanza in every nested file (invisible, and a revocation failure). `check` uses the same glob semantics (`check.rs:183-231`), so CI passes vacuously — invariants 4 and 6 never even see the nested file.

4. **[HIGH] `recipient remove` re-wraps but never rotates the data key — revocation is weaker than its output claims** — `src/commands/recipient.rs:188-254`, module doc `recipient.rs:10-13`.
   `sops updatekeys` re-encrypts the *existing* AES data key for the new recipient set; it does not generate a new one (that is `sops rotate`). Any removed member who ever decrypted a file (or who scripted `sops -d --extract` once) can retain the data key and decrypt every *future* revision of that file until someone runs a full rotation — which sopsy never does and never mentions. The success line "recipient `X` removed" (`recipient.rs:252`) and the test assertion "key B must no longer decrypt" (`tests/recipient.rs:1374-1380`) are true only for an adversary polite enough to keep nothing but their age key. The project docs acknowledge "reading == re-granting", but the one command whose entire purpose is revocation neither rotates nor warns.

5. **[HIGH] `.sopsy.sha` breaks the self-service join workflow on routine merges, while providing ~zero tamper evidence** — `src/config.rs:218-223` (checksum = SHA-256 of raw bytes + first-recipient key), `config.rs:239-254` (hard failure on mismatch), `src/commands/doctor.rs:258-280` (unconditional auto-repair), `src/commands/join.rs:141-143` (tells every joiner to commit both files).
   Two concurrent join PRs each rewrite the single-line `.sopsy.sha`; merging the second one is a *guaranteed* conflict in both files (verified live: `UU .sopsy.sha`, `UU .sopsy.yml`). After any legitimate hand-resolution of `.sopsy.yml`, the sidecar is stale and **every** sopsy command fails with "failed its integrity check … it was edited outside sopsy" (verified live) until someone runs `sopsy doctor` — which repairs it unconditionally, no questions asked. So the mechanism's steady state is: honest users hit a scary tamper error on every merge and learn to reflexively run the command that blesses *any* edit, while an actual attacker just recomputes the hash themselves (the "binding" key is the first recipient *inside the very file being hashed*, so swapping the admin key and recomputing is self-consistent). Maximum friction, negligible evidence.

6. **[MEDIUM] Mid-mutation failures bypass the rollback entirely** — `src/commands/recipient.rs:150-184` (`add`), `229-250` (`remove`), `640-665` (ceremony), `src/commands/approve.rs:84-127`.
   The `ConfigSnapshot` is only restored when `run_updatekeys` fails. Every earlier fallible step after the snapshot — `config.save_to_dir(&repo)?`, `add_key_to_sops_yaml(...)?` — returns via `?` without restoring. Concrete: `.sops.yaml` exists (passes the existence check at `recipient.rs:91-100`) but has a YAML syntax error (hand-maintained files do); `add` writes the new recipient into `.sopsy.yml`+`.sopsy.sha`, then `mutate_sops_yaml` fails to parse (`recipient.rs:907-911`) and the command exits leaving `.sopsy.yml` listing a recipient that `.sops.yaml` doesn't have and no file was re-keyed for.

7. **[MEDIUM] `Config::save` is a non-atomic two-file write; a crash window manufactures a false tamper error** — `src/config.rs:283-292`.
   `save` writes `.sopsy.yml`, then `.sopsy.sha`, both via plain `std::fs::write` (truncate-then-write, no temp+rename). Kill the process between the two writes (or mid-write on a full disk) and the next `Config::load` fails the integrity check with the "edited outside sopsy" accusation — for a file sopsy itself half-wrote. The crate already depends on `tempfile` (Cargo.toml:32) and doesn't use it here.

8. **[MEDIUM] `press_enter` treats EOF as confirmation, and interactivity is detected from stdout only — the break-glass key can be deleted before anyone stored it** — `src/ui.rs:219-232`, `src/ui.rs:59-72`, `src/cli.rs:56-58`, `src/commands/recipient.rs:629-636`.
   Interactivity is `stdout.is_terminal()`; stdin is never checked. Run `sopsy recipient break-glass -o bg < /dev/null` (stdout a TTY), or hit Ctrl-D instead of ENTER at the prompt: `read_line` returns `Ok(0)` on EOF, `press_enter` returns `Ok(())`, and the ceremony proceeds to `remove_file` both halves (`recipient.rs:634-635`). The private key — the *disaster-recovery* key — is now irrecoverably gone, yet its public half gets registered as a recipient, and `sopsy check` invariant 7 happily reports "a break-glass recipient is configured" forever after. The team discovers the truth on the day they actually need to break the glass.

9. **[MEDIUM] Nothing checks that the approver is an active member — pending members can self-approve with `--no-updatekeys`** — `src/commands/approve.rs:37-131` (no approver gate), `approve.rs:140-150` (`resolve_approver` is label-only).
   The doc comment says "any **active** member can approve", but the code enforces nothing: normally the crypto gate (updatekeys needs decryption) provides de-facto enforcement, yet `sopsy approve <me> --no-updatekeys` skips exactly that gate. A pending member can flip themselves to `active`, insert their key into `.sops.yaml`, vouch for themselves (`SOPSY_ASSUME_YES` or their own terminal), and record `approved_by: <themselves>` — then the next legitimate member who runs any re-keying command grants them the data key. Yes, roles are documented as soft guardrails; but the tool should enforce the invariants it *can* check locally, and "the approver must currently be an active recipient" is one of them.

10. **[MEDIUM] The `just warnings` release gate is a no-op: `-D warmings`** — `justfile:31-32`, `justfile:22`, `justfile:46`.
    `cargo clippy -- -D warmings` denies a lint that doesn't exist; rustc emits "unknown lint" *as a warning* and exits 0 (verified: exit code 0). So `just build` and the entire `just publish` chain (fmt → warnings → test → package → publish) never actually deny warnings — the gate the recipe name promises. CLAUDE.md documents the typo instead of fixing it, which is the wrong resolution for a one-character bug in a release gate. Secondary: `publish` depends on `fmt`, which *mutates* the tree (`cargo fmt`, not `--check`) moments before `cargo publish`, so the published crate can silently differ from the committed code.

11. **[MEDIUM] `check`'s hand-rolled regex engine silently mis-evaluates common `path_regex` syntax** — `src/commands/check.rs:359-398`.
    The Kernighan–Pike matcher supports `^ $ . * \`, nothing else. Real `.sops.yaml` files routinely use `\.ya?ml$`, `(dev|prod)\.enc$`, `[^/]+\.encrypted$` — here `? ( | ) [ ]` all match as *literal characters*, so `regex_is_match(r"\.ya?ml$", "config.yaml")` is false. Invariant 4 then reports "encrypted file matches no .sops.yaml creation rule" and `sopsy check` fails CI on a perfectly valid repo — or worse, a rule with unsupported syntax that *should* flag a gap doesn't. Silent divergence from the engine `sops` itself uses (Go regexp) is the failure mode; a matcher that cannot say "I don't support this pattern" is wrong for a CI gate.

12. **[MEDIUM] Every recipient mutation destroys comments and reformats `.sops.yaml`, contradicting its own documentation** — `src/commands/recipient.rs:877-879` ("preserving all other YAML"), `recipient.rs:903-936`.
    `mutate_sops_yaml` round-trips the file through `serde_yaml_ng::Value`, which drops all comments (including the `# Managed by sopsy.` header init itself writes, `init.rs:351`), resolves anchors, and rewrites quoting/indentation. Verified live: after one `recipient add`, the header comment was gone and the style changed. Teams annotate `.sops.yaml` rules ("# prod keys — ask @alice"); sopsy silently shreds that on every `add`/`remove`/`approve`, and the doc comment claims the opposite.

13. **[MEDIUM] The secret-scan workflow's push trigger targets `master`, but the repo's branch is `main`** — `.github/workflows/secret-scan.yml:24-26` (also the branch-protection instructions at lines 17-18).
    The file's own header calls this job "the authoritative, non-bypassable secret gate", yet the `push:` trigger names a branch that doesn't exist here, so direct pushes to `main` are never scanned (the PR trigger still works). Anyone with push access bypasses the "real wall" without trying.

14. **[LOW] `recipient list` truncates public keys to 24 characters — the audit table cannot be used to verify keys** — `src/commands/recipient.rs:398-400` (`KEY_COL_MAX: usize = 24`), `recipient.rs:344`.
    An age key is ~62 chars starting with a fixed `age1`/`age1se1` prefix; 24 chars shows almost none of the distinguishing material, and two different keys can render identically. This is the table an approver looks at around vouching time; the one column that must be exact is the one that's cut. (Also, `truncate` yields max+1 chars — cap plus ellipsis — so the "cap" isn't one; the test at `recipient.rs:996` codifies it.)

15. **[LOW] `approve`'s next-steps guidance references a command that doesn't exist** — `src/commands/approve.rs:289`.
    "…can `sopsy edit`/`sopsy decrypt`" — there is no `sopsy decrypt`; it's `sopsy secrets decrypt`. The newcomer's first post-merge action, straight from the tool's own instructions, is an argument-parse error.

16. **[LOW] Unused dependency: `toml`** — `Cargo.toml:33`.
    `cargo tree -i toml` shows only sopsy depending on it; `grep` finds zero uses in `src/` or `tests/`. Dead compile time and supply-chain surface in a security-sensitive tool.

17. **[LOW] No format validation of supplied age public keys** — `src/commands/join.rs:108-114`, `src/commands/recipient.rs:120-129`, `src/commands/init.rs:237-243`.
    Only emptiness is checked. `sopsy join alice --public-key "not-a-key"` records garbage as a pending entry; `recipient add --public-key garbage --no-updatekeys` persists it into `.sops.yaml`, after which *every* subsequent re-keying operation for *everyone* fails (and rolls back) until the file is hand-edited — which then trips the `.sopsy.sha` tamper error (issue 5). A typo'd key discovered only at approve time wastes the whole request/vouch round-trip.

## Rectifying These Problems

1. **Never place plaintext at the artifact path.** Encrypt to a string first and write only ciphertext: reuse `sops::encrypt_to_string(&seed_file, FileType::Dotenv, Path::new(".env.encrypted"))` with the seed in a temp file (or `--filename-override` on the real `.env`), then `std::fs::write(env_encrypted, ciphertext)` on success. If you keep in-place encryption, wrap it: on `Err`, `remove_file(&env_encrypted)` before propagating, and print what was cleaned up. Independently, move the `.gitignore` step (current step 8) *before* step 7 so even a crash mid-encrypt lands in an ignored-by-default state, and make the idempotency check refuse to "keep" an existing `.env.encrypted` that lacks `sops`/`ENC[` markers (you already have `is_encrypted_file`). Add an integration test: fake failing `sops` → assert `.env.encrypted` absent.

2. **Make the rollback unit match the mutation unit.** Extend `ConfigSnapshot::capture` to also snapshot the bytes of every file `collect_encrypted_files` returns (you compute that set anyway in `run_updatekeys`) and restore them in `restore()`. Secrets-sized files make the memory cost trivial; if you're worried, copy to a temp dir instead. Alternative (weaker): on updatekeys failure, restore configs and then best-effort re-run `updatekeys` on the files already touched to re-wrap them back — but that can fail for the same reason the forward pass did, so byte snapshots are the honest fix. Have `run_updatekeys` continue past failures and report all of them, so one bad file doesn't strand nine good ones mid-state.

3. **One matching engine, one source of truth.** The cleanest fix: derive the re-key/check file set from `.sops.yaml` itself — walk the tree (you already do in `check`'s `gather_encrypted_files`) and match repo-relative paths against the creation-rule regexes, exactly as `sops` will. If you keep `encrypted_globs`, add `**` support and change the defaults to `**/*.encrypted` etc., and make `sopsy secrets encrypt -o <path>` warn (or refuse without `--force`) when the output path matches no configured glob/rule — that closes the "tool creates a file it then can't manage" loop. Add a test with a nested artifact asserting it *is* re-keyed.

4. **Rotate on remove.** After a successful `remove`, run `sops rotate -i` (with the same `--input-type` handling as `updatekeys_file`) on every managed file so a fresh data key is generated — that is the operation the user believes they ran. Trade-off: `rotate` rewrites the whole encrypted body (bigger diffs, requires decrypt ability — which remove already requires). If you decide rotation is too heavy for a default, then at minimum print an unmissable warning: "the data key was re-wrapped, not rotated; a removed member who previously decrypted may retain access to future revisions — run `sops rotate -i <file>` to fully rotate", and add a `--rotate` flag. Silence is the only unacceptable option.

5. **Delete `.sopsy.sha`, or demote it to a warning.** It cannot provide integrity (anyone recomputes it; the "binding" key lives inside the hashed file), and it demonstrably breaks merges of the very file your join workflow tells everyone to edit concurrently. Your own docs say git history is the real audit trail — believe them. If you must keep it: (a) make `Config::load` *warn* on mismatch instead of failing, reserving hard failure for `check` where a human opted in; (b) stop hashing raw bytes — hash a canonicalized serialization so a clean YAML merge doesn't invalidate it; (c) ship a `.gitattributes` entry so at least the conflict message points at `sopsy doctor`. None of that makes it evidence; (a) merely stops it taking hostages.

6. **Restore on every error path after the snapshot.** Wrap the mutation phase in a closure and translate any `Err` into restore-then-return:
   ```rust
   let snapshot = ConfigSnapshot::capture(&repo, &sops_config);
   let result = (|| -> Result<()> {
       config.save_to_dir(&repo)?;
       add_key_to_sops_yaml(&sops_config, &public_key)?;
       run_updatekeys(ui, &repo, args.no_updatekeys)
   })();
   if let Err(err) = result { snapshot.restore()?; return Err(rewrap_error(err)); }
   ```
   Do the same in `remove`, `approve`, and `portable_key_ceremony`. Also parse-validate `.sops.yaml` *before* writing `.sopsy.yml` (cheap pre-flight: `serde_yaml_ng::from_str` on it up front), so the common failure never mutates anything.

7. **Atomic writes via `tempfile` (already a dependency).** In `Config::save`: write the YAML to a `NamedTempFile` in the same directory, `persist()` over `.sopsy.yml`, then the same for `.sopsy.sha`. Two renames still aren't jointly atomic — which is one more argument for issue 5's warn-not-fail posture (a sha that's one save behind then costs a warning, not an outage). Apply the same temp+rename pattern to `mutate_sops_yaml` and `secrets --output` writes while you're there.

8. **Confirmations must require affirmative input.** In `press_enter`, treat EOF as refusal:
   ```rust
   let n = std::io::stdin().read_line(&mut line)?;
   if n == 0 { return Err(Error::NonInteractive { prompt: …, flag: … }); }
   ```
   And fold stdin into interactivity detection: `interactive && stdout_tty && std::io::stdin().is_terminal()` in `Ui::resolve` (plus `cli.rs::resolve_interactive`). Separately, reorder the ceremony so deletion happens *after* the recipient is registered and re-keyed — if registration fails today, the operator's only copy is already off-box; deleting last costs nothing and keeps a local fallback until the repo state is consistent.

9. **Enforce the invariant you can check.** In `approve::run`, resolve the approver first and require it to map (via `username` or an explicit `--as <name>`) to an *active, non-pending* recipient; refuse when the approver set intersects the approved set ("you cannot approve yourself"); and when `--no-updatekeys` is passed, print a warning that the crypto gate was skipped. Keep `--force` as the documented escape hatch for repos that don't record usernames. It's still a soft guardrail against a malicious git author — but it stops the tool itself from executing the bypass in one obvious command.

10. **Fix the gate.** `warnings: cargo clippy --all-targets --all-features -- -D warnings` (matching CI so local and CI agree), and change the `publish`/`build` dependency from `fmt` to a non-mutating `fmt-check: cargo fmt --all -- --check`. Then delete the CLAUDE.md sentence that documents the typo. Consider a CI step that runs `just warnings` itself so the recipe can't silently rot again.

11. **Use a real regex engine or fail loudly.** `regex-lite` is ~small, no-unsafe, and matches Go-ish syntax closely enough for `path_regex`; `check` is not a hot path. If you insist on zero dependencies, scan the pattern for unsupported metacharacters (`( ) [ ] | + ? { }`) and report "unsupported path_regex `…` — sopsy check cannot evaluate this rule" as an explicit failure instead of silently treating them as literals. A CI gate may be strict or ignorant, but never confidently wrong.

12. **Stop shredding user YAML — or stop claiming you don't.** Minimal honest fix: correct the doc comment to "comments and formatting are not preserved" and re-emit the managed-by header after serialization. Better: do a targeted textual edit for the common case — locate the `age:` scalar/sequence for each rule and splice the key list in place (you already normalize to the comma-separated string form, which makes the edit a single-line replacement) — falling back to the serde round-trip only when the structure is unrecognizable, with a warning.

13. **`branches: [main]`** in `secret-scan.yml`'s push trigger, and update the branch-protection comment to say `main`. Grep the repo for other `master` remnants while you're in there (the lefthook fail_text's "Rails encrypted credentials" advice is copy-pasted from another project too — wrong remedy for a Rust repo).

14. **Show the whole key in `recipient list`.** Drop `KEY_COL_MAX` (or raise it to full key width) — this table is the audit surface, and terminal width is better spent here than on padding. If you must truncate, keep prefix *and* suffix (`age1se1qg8v…kqleh258`), which preserves most of the distinguishing entropy; and fix `truncate` to include the ellipsis *within* the cap.

15. One-line fix: `"{joined} then pull main and can `sopsy edit`/`sopsy secrets decrypt` — Touch ID unlocks it."` Add a cheap test that greps help text/next-steps strings for `sopsy <word>` tokens and asserts each is a real subcommand, so guidance can't drift again.

16. Remove `toml = "1.1.2"` from `Cargo.toml`. Consider adding `cargo-udeps` (or `cargo machete`) to CI so unused dependencies can't accumulate silently.

17. **Validate keys at the door.** A small shared validator — `key.starts_with("age1")`, all chars in the bech32 set `[02-9ac-hj-np-z]` after the prefix, length in a sane band (~59–70 chars; Secure Enclave `age1se1…` keys are longer than X25519 `age1…` ones) — applied in `join`, `recipient add`, and `init --public-key`. Reject with "that does not look like an age public key (age1…)". This converts a whole family of late, confusing `sops` failures (and the issue-5/6 fallout chains) into an instant, obvious error at input time.
