# keyman remediation plan Turns [`AUDIT.md`](./AUDIT.md) into sequenced work. Each phase is one commit, independently landable, gate-green on its own. Section references (§) are to `AUDIT.md`. Ordering is by *blast radius per unit of risk*, not by severity: the error boundary comes first because it makes every later phase's failure mode legible, and the config threading comes late because it is the only phase that rewrites existing test assertions. ## Verified before planning Four things the fixes depend on, checked by running them rather than assumed — two of them changed the prescription: | Check | Result | Consequence | | --- | --- | --- | | `ssh-keygen` with `-N` omitted | Prompts `Enter passphrase … (empty for no passphrase)` **and** confirms | §2.2 fix works: omit `-N`, inherit stdio, keyman never holds the passphrase | | `ssh-keygen -y -f ` | **Prompts for the passphrase** | §1.6 fix cannot be a silent spawn — needs `stdio: 'inherit'` and a skip path | | `@inquirer/core` from keyman | `ERR_MODULE_NOT_FOUND` — transitive via `inquirer`, not a direct dep | Detect `ExitPromptError` by `error.name`, never by import | | `z.strictObject` in zod 4.4.3 | Available; reports `unrecognized_keys` with a `keys` array | §3.5 has a hard-failure option, though the plan prefers a warning | ## What is not a breaking change Per §4.3, `src/index.ts` exports only `keyman`, `loadConfig`, `resolveConfigPaths` and the update module. `encryptKeys`, `decryptKeys`, `generateKey`, `listKeys`, `copyKey` and `extractAgePublicKey` are **not** on the public surface, so every signature change below is internal. Phases 2–6 are not semver-breaking. The one user-visible behaviour change is Phase 5 — see [Migration](#migration). ## Gate discipline `lint:ci` → `typecheck` → `test:coverage` runs on `pre-push` and in CI. Two standing constraints: - **Every phase lands its tests with its fix.** No phase may leave a red gate, so there is no "write the failing tests first" commit. - **`keyman.cli.ts` is excluded from coverage** (`vitest.config.ts:18`). Per `CLAUDE.md`, *adding logic to those files means moving it somewhere covered* — which is why Phase 1 extracts argument parsing into a new module rather than growing `cli.ts`. Per-phase verification is `pnpm --filter @bitsquare/keyman run test`; the full gate (`pnpm run lint:ci && pnpm run typecheck && pnpm run test:coverage`) before each push. --- ## Phase 1 — Error boundary, `--help`, argument validation Closes §3.1, §3.2, and the second half of §1.2 (the crash dump). First because it is pure addition, touches no operation module, and converts every latent throw in phases 2–6 from a stack dump into a line of text. The `ExitPromptError` half is independently worth shipping: today **Ctrl-C at any prompt** produces a crash dump. **New file `src/keyman.args.ts`** (covered by the gate, unlike `cli.ts`): - `parseArgs(argv: string[]): ParsedArgs` — supports `--flag value` *and* `--flag=value`, rejects a flag consumed as another flag's value, rejects unknown flags, and validates `--channel` against `'latest' | 'next' | 'main'` so §3.2's false "Could not reach " cannot happen. - `helpText(): string` — flags, both subcommands, and the four `KEYMAN_*` variables. This is the text Phase 9 keeps in step with the README. **`src/keyman.cli.ts`** stays wiring: dispatch on the parse result, and ```ts try { await keyman(); } catch (error) { if ((error as { name?: string }).name === 'ExitPromptError') { console.log('\n👋 Goodbye!\n'); process.exit(0); } console.error(`❌ ${error instanceof Error ? error.message : error}`); process.exit(1); } ``` `error.name`, not `instanceof` — `@inquirer/core` is not a direct dependency and does not resolve from this package. **Tests** — new `tests/args.test.ts`: each rejection, both flag forms, the channel whitelist, and that `helpText()` names every flag `parseArgs` accepts (so the two cannot drift). **Done when** `keyman --help` prints usage and exits 0 without loading config or prompting; `keyman --bogus` errors; Ctrl-C prints Goodbye and exits 0. --- ## Phase 2 — Guards and error handling in encrypt/decrypt Closes §1.2 (first half), §1.5, and §1.10's `statSync`. - `encrypt.ts:13,16` and `decrypt.ts:8` — `existsSync` guard, falling through to the "⚠️ No …" message each function already has but cannot currently reach. - `main.ts:40-41` — create `keysDir` alongside `vaultRoot` and `tmpDir`. Use `{recursive: true, mode: 0o700}` now, so Phase 4 does not have to revisit it. - Wrap the `age` spawns in both functions. An `ENOENT` on the binary gets its own message ("`age` was not found on PATH") — it is the one hard external requirement and currently the least legible failure. - `list.ts:80` — `readdirSync(dir, {withFileTypes: true})` instead of `statSync` per entry, which also drops N stat calls and fixes the broken-symlink throw. - Delete the debug logging while in these files: `encrypt.ts:18-19` and `decrypt.ts:10` (§1.10). That also closes `DOCS-AUDIT.md` §6.4's open bullet. **Tests** — the cases the current suite structurally cannot have, because every `beforeEach` pre-creates the directories: encrypt with no `~/.ssh`, encrypt with no tmp, decrypt with no `/keys`, each asserting the warning and no throw. Plus `list` with a dangling symlink in the keys directory. **Note on coverage.** `encrypt.ts` and `decrypt.ts` are at 100 % lines and branches *today*. The number will not move; the tests are the point. **Done when** a first run against an empty vault can reach every menu entry and return to the menu. --- ## Phase 3 — Resolve the age recipient once, and derive it properly Closes §1.3 and §2.3, and makes `CLAUDE.md`'s `age-keygen` claim true (§5.5). Two changes that belong together because both are about the recipient: 1. **`utils.ts` — derive, don't scrape.** `extractAgePublicKey` currently regexes `# public key:` out of a comment (`utils.ts:16`) and trusts it. Replace with `age-keygen -y `, which derives the public key *from the private key* and cannot disagree with it. Keep the comment parse as a fallback for when `age-keygen` is absent, behind a warning that the recipient is unverified. The function becomes `async`. 2. **`main.ts:73,80` — delete both `!`.** Resolve the recipient once before the `switch`, and treat `null` as recoverable: print the remedy (`age-keygen -o `) and `break` back to the menu. This is the whole of §1.3 — the type already said null was possible. Sequencing matters inside the phase: fix the call site first. Without it, a missing key file still reaches `age -r null`, and the generate path still leaves a **plaintext private key in `tmpDir`** after telling the user the operation failed. **Tests** — `utils.test.ts` gains the `age-keygen -y` path with `execa` mocked, the fallback-with-warning path, and the both-unavailable path. `main.test.ts` gains: missing recipient → neither `generateKey` nor `encryptKeys` is called, a remedy is printed, and the menu loop continues. **Done when** `keyman` against a vault with no `age.key` reaches the menu, refuses generate and encrypt with a remedy, and still offers list and decrypt. --- ## Phase 4 — Decrypt: stop overwriting, stop the 0644 window Closes §1.4 and §2.1. The highest-value phase — §1.4 is the only finding that destroys data the user did not ask to touch. - **Collision check before any decryption.** Both output paths, both modes. Prompt per collision, defaulting to skip; `~/.ssh` deserves the friction more than `vault/tmp` does, but the check is the same code. - **Replace the shell-outs** (`decrypt.ts:49-50`) with `fs.copyFileSync` and `fs.chmodSync`. Three spawns per key become one, it works on Windows, and it removes a `cp` that overwrites unconditionally. - **Close the permission window.** Verified: `age -o` creates the file `0644` and `mkdirSync` creates `vault/tmp` as `0755`, so a plaintext key is world-readable for the duration of two process spawns — and stays `0644` if the `chmod` fails. `fs.chmodSync` immediately after `age` resolves; `mode: 0o700` on the directory (already done in Phase 2); create `~/.ssh` `0700` if absent. **Test rework — the fiddliest in the plan.** `decrypt.test.ts` asserts on the mocked spawns: `argsOf('cp')` (`:98,111`) and `argsOf('chmod')` (`:99,112`) both disappear, and `execa` is mocked with a bare `mockResolvedValue` (`:57`) that writes no output file. Once `copyFileSync` is real it needs a real file, so the mock must write to its `-o` argument the way `encrypt.test.ts:51-54` already does. Assert the on-disk result and mode instead of the argv — a better test than the one it replaces, since it checks the outcome rather than the mechanism. `execa` call counts also change (`:123`: six spawns → two). **Done when** decrypting onto an existing key requires a confirmation, and the decrypted key is never observable at anything but `0600`. --- ## Phase 5 — Thread `keysDir` and `tmpDir` through encrypt and decrypt Closes §1.1 and the `DOCS-AUDIT.md` entry in §5.1. Mechanical, but it is the one phase that rewrites assertions that pass today, so it stays its own commit with nothing else in it. - `encryptKeys(sshDir, keysDir, tmpDir, pubkey)` — drop `vaultDir`, delete the hardcoded `path.join(vaultDir, 'keys')` (`encrypt.ts:38`). - `decryptKeys(sshDir, keysDir, tmpDir, ageKey)` — drop `vaultDir`, delete the hardcoded joins at `decrypt.ts:7,39,43`. - `main.ts:76,84` — pass `paths.keysDir` and `paths.tmpDir`. Neither function has any remaining use for `vaultRoot`, so the parameter goes rather than becoming a second source of truth. **Assertions to change** — all four, named so the diff is reviewable: | Location | Today | After | | --- | --- | --- | | `main.test.ts:164-175` | "encrypts keys into the vault root", asserts `paths.vaultRoot` | asserts `paths.keysDir`, `paths.tmpDir` | | `main.test.ts:177-187` | asserts `paths.vaultRoot` | asserts `paths.keysDir`, `paths.tmpDir` | | `encrypt.test.ts:95,115,128-129` | `path.join(vaultDir, 'keys', …)` | `path.join(keysDir, …)` | | `decrypt.test.ts:53,89` | `keyDir = path.join(vaultDir, 'keys')` | `keysDir` passed in directly | **New test, the one that would have caught this:** a config with `keysDir: 'encrypted'` and `tmpDir: 'plain'`, encrypt a key, then list it, and assert the listing shows it in `[Vault]`. That round trip fails today and is the regression worth owning. **Also in this commit:** move the `DOCS-AUDIT.md:826-827` bullet out of *checked and accurate* and point it at this finding. It was verified against `keyman.encrypt.ts` — the file that ignores the config — which is precisely how §1.1 stayed invisible. --- ## Phase 6 — Generate: passphrase off argv, and `.pub` recovery Closes §2.2, §1.6, and §1.10's leftover-directory bullet. **Passphrase (§2.2).** Verified: omitting `-N` makes `ssh-keygen` prompt *and* confirm. So delete keyman's own password prompt (`generate.ts:26-33`), omit `-N`, and spawn with `stdio: 'inherit'`. The passphrase never enters keyman's memory and never reaches argv — strictly better than routing it more carefully, and it deletes code. `generate.test.ts:78-87` loses `-N`/`'pw'` from the expected argv and the password-prompt case goes away. **Missing `.pub` (§1.6).** The selection list is built from private keys only, so an orphan private key is offered and `copyFileSync` throws *after* `age` has written the `.age` file — leaving a vault entry with no public key and killing the rest of the batch. Fix in two parts: - Derive it with `ssh-keygen -y -f ` when the sibling is absent. **Verified that this prompts for a passphrase on an encrypted key**, so it needs `stdio: 'inherit'` and a clean skip when the user cannot or will not supply it — not a silent spawn whose stdout is captured. - Wrap the loop body in `encrypt.ts:36-48` per key, so one bad key costs one key. Report the failures at the end rather than dying at the first. **Leftover directory.** `generate.ts:65` creates `//` before `age` runs at `:68`. Move the `mkdirSync` after `age` succeeds. --- ## Phase 7 — Config: warn on typos, decide on the dead machinery Closes §3.5, §3.4, and asks for a decision on §3.3. **Typos (§3.5).** Verified: `{"vaultroot": "…"}` is silently stripped by `z.object`. Warn per file rather than failing — diff `Object.keys(rawConfig)` against the schema keys plus `resolution` inside the existing per-file loop (`config.ts:210-227`), where the filename is in hand. That names the offending file, which `z.strictObject` cannot do from the merged result, and it preserves the module's documented posture of degrading to defaults rather than throwing. (`z.strictObject` is available in zod 4.4.3 and reports `unrecognized_keys` with a `keys` array, if a hard failure is preferred later.) **`getConfigPaths` (§3.4).** Add it to the `--print-config` JSON as `configFiles`. The function is currently exercised only by its own test, and `--print-config` currently cannot answer *which files were read* — that exists only as unstructured stderr from `loadConfig`. One change fixes both. **Decision needed — the `resolution` machinery (§3.3).** Roughly 45 lines (`config.ts:23-30,115-157`) that cannot affect a valid config, because every schema property is a `string` and both strategies return `childValue` for primitives. `config.test.ts:212` "honours an explicit override strategy" passes either way. - **Recommended: delete it**, along with the doc comment at `:186-194` that advertises it. `keyman.config.ts` goes from 267 lines to roughly 220, and the config file stops documenting a knob that does nothing. - **Alternative: keep it** as the shape a future array- or object-valued option would need — but then say so in a comment, because today it reads as functional, and make `config.test.ts:212` assert something that distinguishes the two strategies (which requires a non-string property to exist first). Deleting is the smaller lie. It also diverges from nopy, where the machinery *is* load-bearing — worth a line in `CLAUDE.md` so the divergence reads as deliberate. --- ## Phase 8 — Portability and the gaps that make keys invisible Closes §1.7, §1.8, §1.9, §2.4. Independent of each other; split if any grows. - **Clipboard (§1.9).** `pbcopy` / `wl-copy` / `xclip` / `clip.exe` by platform, falling back to printing the key to stdout so the operation is never a dead end. Delete the comment at `copy.ts:46-48` that admits the shortcut. - **Home directory (§1.7).** `os.userInfo()` for the current user; for a named user, look the home directory up rather than assuming `/home/` — wrong on the one platform the tool currently supports. Check `existsSync` and say so, instead of feeding a nonexistent path into a `readdir`. `main.test.ts:198-204` changes. - **Non-`id_*` keys (§1.8).** Relax the filters (`copy.ts:9`, `encrypt.ts:14,17`, `list.ts:23,51`) to *any* private key with a recognisable header, or at minimum print a count of the keys that were skipped and why. Today a key named `deploy_ed25519` is simply absent from the menu — and pre-existing keys are the population a key manager is adopted to take over. `decrypt.ts:9` reconstructs `id_${dir}` from the folder name, so the vault layout has the assumption baked in; relaxing discovery means storing the real filename per key, which is the largest single item in this plan. **Size it before committing to it** — a skipped-key count is a tenth of the work and closes most of the surprise. - **Plaintext hygiene (§2.4).** A "🧹 Clear decrypted keys" menu entry, and write a `.gitignore` next to the vault on first run covering `age.key` and `tmp/` — which `README.md:25-26` currently tells the user to do by hand. This is the cheapest guard against the exact failure the tool exists to prevent. --- ## Phase 9 — Documentation Closes §5.2, §5.3, §5.4, §5.5. Last, so it documents what the code now does. - **`README.md` — the only shipped document** (`package.json:37-41` ships `dist`, `README.md`, `LICENSE`). Currently describes four of six menu entries, invents key rotation, and mentions none of `self-update`, `--print-config`, `--version`, `--help`, or the four `KEYMAN_*` variables. `README.PUBLISH.md` has all of it and never reaches a reader on the registry. Reuse Phase 1's `helpText()` as the source for the CLI section so the two cannot drift. - **`README.md:46-86`** — the configuration and vault-layout sections, now that Phase 5 makes `keysDir`/`tmpDir` real, plus the migration note below. - **`README.md:11`** — drop "Support for key rotation" unless Phase 10 lands first. - **`README.md:33`** — stop telling the user to shell out to `ssh-keygen`; the Generate operation exists. - **`CLAUDE.md`** — `age-keygen` becomes true in Phase 3; note that `cp`/`chmod` are gone (Phase 4) and record the `resolution` divergence from nopy (Phase 7). - **`AUDIT.md`** — mark findings closed, keeping their text as the record, the way `DOCS-AUDIT.md` does. --- ## Phase 10 — Key rotation (decision required) §3.6. `README.md:11` has advertised it since before this audit; `grep -rn "rotat" packages/keyman/src/` returns nothing. Unlike the rest of this plan it is a feature, not a repair, and it is the one item that could reasonably be dropped instead. **Recommendation: build it** — rotation is the operation that makes a key vault worth having, and the pieces all exist by Phase 6 (generate under a new name, encrypt, keep the old key until the replacement is deployed, then shred). Sketch: 1. Pick an existing vault key. 2. Generate a replacement into `tmpDir` under a versioned name. 3. Encrypt it alongside the current one — never replacing it. 4. Report both public keys, so the new one can be deployed before the old one goes. 5. A separate "retire" step that removes the superseded key once the user confirms. Steps 3 and 4 are the whole value: a rotation that atomically replaces the key is a rotation that locks you out of the host you were rotating for. If this is deferred, delete the README claim in Phase 9 instead. --- ## Migration Phase 5 is the only user-visible change. Anyone with a custom `keysDir` or `tmpDir` currently has a **split vault** — `generate` and `list` on the configured directory, `encrypt` and `decrypt` on `/keys` and `/tmp`. After Phase 5 all five agree on the configured directory, so anything written by `encrypt` before the upgrade needs moving: ```sh mv /keys/* // ``` Nobody on the defaults is affected, since the two halves coincide there. The README gets this as a note, and it is worth a line in the release notes for whichever version carries Phase 5. ## Rollout Per `CLAUDE.md`: bump `packages/keyman/package.json`, land on `main`, then tag `keyman-v`. - **Snapshots come free.** Every push to `main` publishes `-main..g` to Gitea under the `main` dist-tag, so each phase is installable for testing without a release. `pnpm run try:snapshot` installs one into a throwaway project. - **Suggested cut points.** After Phase 4 as `0.6.0` — error boundary, guards, recipient handling and the data-loss fix, which is the set worth getting to users first. After Phase 9 as `0.7.0`, carrying the Phase 5 migration note. - **keyman can reach npmjs.** It has no `workspace:*` dependencies (`execa`, `inquirer`, `semver`, `zod` only), so `scripts/linked-deps.mjs` has nothing to block on — unlike `nopy`, which `CLAUDE.md` records as gated behind `nopy-cubes` shipping. keyman has never been published to npmjs; `0.6.0` could be the first, and versions being `0.x.y` rather than `1.0.0-alphaN` means the `latest` dist-tag will now actually move. - **`pnpm publish`, never `npm publish`** — no `workspace:` ranges here, but the rule is repo-wide and `scripts/verify-pack.mjs` enforces it in both workflows. ## Sequencing at a glance ``` 1 cli boundary + --help + args §3.1 §3.2 §1.2(half) isolated, pure addition 2 guards in encrypt/decrypt/list §1.2 §1.5 §1.10 new tests only 3 age recipient, once and derived §1.3 §2.3 §5.5 signature → async 4 decrypt: no clobber, no 0644 §1.4 §2.1 reworks decrypt.test.ts ── cut 0.6.0 ── 5 thread keysDir/tmpDir §1.1 §5.1 rewrites 4 assertions 6 generate: -N gone, .pub recovery §2.2 §1.6 §1.10 reworks generate.test.ts 7 config: warn, prune, print §3.5 §3.4 §3.3* *decision 8 portability + hygiene §1.7 §1.8 §1.9 §2.4 §1.8 needs sizing 9 documentation §5.2 §5.3 §5.4 §5.5 README is the shipped one ── cut 0.7.0 ── 10 rotation §3.6* *decision: build or delete ``` Phases 1–6 are repairs and want to land in order. 7 and 8 are independent of each other and of 5–6. 9 depends on everything before it. 10 is optional and gates one line of Phase 9. ## Open decisions Neither blocks Phase 1. Both change scope where they land: 1. **§3.3, at Phase 7** — delete the inert `resolution` machinery (recommended, −45 lines) or keep it as future shape with a comment saying so. 2. **§3.6, at Phase 10** — build rotation (recommended) or delete the README claim in Phase 9.