fix(cli): close wallet-security gaps, bump CLI to 0.5.0

.github/workflows/ci.yml

@@ -117,10 +117,59 @@ jobs:
       - name: Run collar smoke tests
         run: ${{ steps.detect-pm.outputs.run }} test:collar
 
+  cli:
+    name: CLI
+    runs-on: ubuntu-latest
+    defaults:
+      run:
+        working-directory: cli
+    steps:
+      - name: Checkout
+        uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5
+
+      - name: Setup Node.js
+        uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020
+        with:
+          node-version: '22'
+          cache: npm
+          cache-dependency-path: |
+            package-lock.json
+            cli/package-lock.json
+
+      # The CLI depends on the SDK via `file:..`, and the SDK's package entry
+      # points at ./dist, which is gitignored. A fresh checkout therefore has
+      # no SDK build for the CLI to typecheck or run against — build the root
+      # package first or every step below fails on unresolved types.
+      - name: Install root SDK dependencies
+        run: npm ci
+        working-directory: .
+
+      - name: Build root SDK
+        run: npm run build
+        working-directory: .
+
+      - name: Install CLI dependencies
+        run: npm ci
+
+      - name: Run TypeScript check
+        run: npm run typecheck
+
+      # Offline only. Covers the wallet-security regressions (config write
+      # atomicity, approval-target defaults) and the documented exit-code
+      # contract — all of which are silent failures if they regress.
+      - name: Run CLI tests
+        run: npm test
+
+      - name: Build
+        run: npm run build
+
+      - name: Check build output exists
+        run: test -f dist/index.js
+
   all-checks:
     name: All Checks
     runs-on: ubuntu-latest
-    needs: [lint, build, collar-tests]
+    needs: [lint, build, collar-tests, cli]
     steps:
       - name: All checks passed
         run: echo "All checks passed!"

cli/CHANGELOG.md

@@ -7,6 +7,113 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+## [0.5.0] — 2026-08-25
+
+Wallet-security release. Three changes are **breaking** — they turn silent,
+irreversible defaults into explicit choices. See *Migration* below.
+
+### Security
+
+- **Config writes are now atomic.** `saveConfig` writes to a temp file in the
+  same directory and `rename(2)`s it into place, matching the pattern already
+  used by the RFQ key storage. Previously an in-place `writeFileSync` could be
+  interrupted by a crash, `SIGINT`, or `ENOSPC` and leave a truncated
+  `config.json`. Because `wallet create` discards the BIP-39 mnemonic and
+  declares the config file the user's only copy of the key, a torn write there
+  permanently destroyed funds. A reader now always sees either the complete old
+  file or the complete new one.
+  - Side effect, intentional: a symlink at the config path is now **replaced**
+    rather than written through. This closes the write-side gap left by
+    `loadConfig`'s `O_NOFOLLOW` read. Users who symlink their config from a
+    dotfiles repo will find the link severed after the next write — the config
+    becomes a regular file and the dotfile copy goes stale.
+  - Not covered: there is no `fsync` before the rename. Process-level crashes
+    are fully handled; a kernel panic or power loss during the write is not.
+
+- **`wallet approve` no longer defaults to an unlimited allowance.**
+  `--amount` is now required. It previously defaulted to `max`, so
+  `thetanuts wallet approve --token USDC --for optionBook --yes` granted
+  `MaxUint256` to the spender with no interactive gate — the warning went to
+  stderr and `--yes` auto-passed the confirm. `--amount max` still works and
+  still warns.
+
+- **`rfq request --ensure-allowance` approves the exact escrow on BUY.**
+  Omitting `--approve-amount` on a BUY now approves exactly `reservePrice`,
+  the amount the OptionFactory escrows at request time, matching `book fill`
+  and `position close`. SHORT deliberately still defaults to `MaxUint256`: the
+  settle-time collateral draw is a structure-dependent max-loss figure that is
+  not carried on the request (`params.collateralAmount` is hardcoded to 0 by
+  every SDK builder, and the send path rejects any nonzero value), so there is
+  no exact amount to approve. Under-approving a SHORT reverts at settlement
+  *after* a maker has committed, which is worse than a broad allowance.
+  `--approve-amount <n>` remains available to cap it.
+
+- **`--yes` is no longer consent to destroy an existing private key.**
+  `wallet create` and `wallet import` now exit 2 when the config already holds
+  a key and `--yes` is passed, instead of overwriting it. The old key has no
+  other copy on disk and the replacement wallet's mnemonic is not persisted, so
+  a habitual `--yes` in a script silently burned funds with only a stderr line
+  to show for it. `wallet create --force` overwrites deliberately;
+  `wallet import` is interactive by nature and has no `--force`. This matches
+  the existing rule that `--reveal-key` refuses `--yes`.
+
+### Fixed
+
+- **Usage errors exit `2` again.** Commander exits `1` for every parse failure,
+  but the documented contract reserves `1` for generic runtime errors (network,
+  RPC, contract revert) and `2` for usage errors. Making `wallet approve
+  --amount` required therefore produced a `1` where the README promised a `2`,
+  leaving automation unable to tell a mistyped flag from a reverted transaction.
+  Commander's usage-error codes are now mapped to `2` centrally across the whole
+  command tree; `--help` and `--version` continue to exit `0`, and any code the
+  map does not recognise keeps Commander's own exit code rather than being
+  silently coerced.
+  - This also covers a group command invoked with no subcommand — `thetanuts
+    wallet`, `thetanuts book`, or bare `thetanuts` — which Commander reports as
+    `commander.help` with exit `1`. That is an incomplete invocation, not a
+    runtime failure. Successful `--help` uses a separate code and still
+    exits `0`.
+
+### Added
+
+- `cli/tests/approvalDefaults.test.ts` — regression coverage for the
+  `--ensure-allowance` target defaults and for `saveConfig` atomicity,
+  permissions, and symlink replacement. No existing test touched these paths.
+- `cli/tests/exitCodes.test.ts` — subprocess coverage for the documented exit
+  codes. Exit status is a process-level contract, so an in-process assertion
+  cannot observe it.
+- **CI now runs the CLI.** The workflow previously installed, built, and tested
+  only the root SDK, so every CLI test could fail without blocking a merge. A
+  `cli` job runs typecheck, tests, and build, and `all-checks` depends on it.
+  The job builds the root SDK first: the CLI resolves the SDK through `file:..`
+  and the SDK's entry points at a gitignored `dist/`, so a fresh checkout has
+  nothing for the CLI to compile against.
+
+### Migration
+
+- `thetanuts wallet approve --token X --for Y` → add an explicit
+  `--amount <n>`, or `--amount max` to keep the previous behavior.
+- `thetanuts wallet create --yes` / `wallet import --yes` over an existing key
+  → use `wallet create --force`, or drop `--yes` and confirm interactively.
+- Scripts that relied on the leftover unlimited allowance from a BUY
+  `rfq request --ensure-allowance` for a **subsequent** transaction must now
+  pass `--approve-amount max` explicitly.
+
+### Known gaps
+
+- The private key is still stored **unencrypted** in `config.json`, protected
+  only by `0600` permissions. That stops other users on the machine; it does
+  not stop another process running as you (for example a malicious postinstall
+  script in an unrelated project). An encrypted keystore or OS-keychain option
+  is not yet implemented.
+- `config set privateKey` and `config unset privateKey` still overwrite or
+  delete the stored key with no confirmation.
+- Excess positional arguments are still accepted and ignored rather than
+  rejected — Commander's `allowExcessArguments` defaults to true and is not
+  disabled, so `wallet approve extraarg --token USDC --amount 1 --for
+  optionBook` runs the approve flow instead of erroring. Pre-existing; the
+  exit-code mapping above does not change it.
+
 ## [0.4.0] — 2026-08-21
 
 ### Added

cli/README.md

@@ -794,7 +794,7 @@ thetanuts wallet transfer --token USDC --to 0xRecipient --amount 5.50
 | `--token <sym>` | Token symbol (USDC for trading) |
 | `--spender <addr>` | Explicit spender address |
 | `--for <name>` | Alternative: `optionBook` or `optionFactory` |
-| `--amount <max\|n>` | `max` approves MaxUint256 (WARNING printed). Otherwise a decimal. |
+| `--amount <max\|n>` | **Required.** A decimal amount, or `max` for MaxUint256 (WARNING printed). No default — as of 0.5.0 omitting it is an error rather than a silent unlimited approval. |
 | `--yes` | Skip confirmation prompt |
 | `--dry-run` | Emit calldata, do not broadcast |
 
@@ -1162,7 +1162,9 @@ fi
 - **Fill identity.** A successful book fill also renders the created option address, ticker, buyer/seller, and an on-chain `position info` command; `position list` may lag briefly while the indexer catches up.
 - **`--yes` skips prompts.** Use it in CI / automation only.
 - **Approvals are never bundled silently with fills** — they require their own confirmation.
-- **`max` approvals require explicit opt-in.** `--approve-amount max` (or `wallet approve --amount max`) prints a stderr WARNING and cannot be combined with key-disclosure flags.
+- **`max` approvals require explicit opt-in.** `--approve-amount max` (or `wallet approve --amount max`) prints a stderr WARNING and cannot be combined with key-disclosure flags. `wallet approve --amount` has no default, and `rfq request --ensure-allowance` approves exactly the escrowed `reservePrice` on a BUY. A SHORT RFQ still defaults to `max`: the settle-time collateral draw is not knowable at request time, and under-approving it reverts at settlement after a maker has committed.
+- **`--yes` is not consent to destroy a key.** `wallet create` and `wallet import` refuse `--yes` when the config already holds a private key (exit 2) — the old key is unrecoverable and the new wallet's mnemonic is not persisted. Use `wallet create --force` to overwrite deliberately, or confirm interactively. Same rule as `--reveal-key`, which also refuses `--yes`.
+- **The private key is stored unencrypted**, protected only by `0600` file permissions. That stops other users on the machine; it does not stop another process running as you. Config writes are atomic (temp file + rename), so an interrupted write cannot truncate the file that holds it.
 - **`keys export`/`import` refuse stdin/stdout** to prevent private-key material from landing in shell history or pipe targets.
 - **HTTPS-only RPC.** The CLI rejects non-HTTPS RPC URLs unless they point at localhost.
 

cli/package.json

@@ -1,6 +1,6 @@
 {
   "name": "@thetanuts-finance/cli",
-  "version": "0.4.0",
+  "version": "0.5.0",
   "description": "Thetanuts Finance V4 command-line interface",
   "type": "module",
   "main": "dist/index.js",

cli/src/commands/rfq.ts

@@ -1523,8 +1523,38 @@ function checkOfferDeadlineFuture(req: RFQRequest): void {
  *          path stays a soft stderr advisory and proceeds with the request.
  *
  * With `--ensure-allowance`, confirm prompt for the approval, then
- * `ensureAllowance(MaxUint256)` (or the user's `--approve-amount`).
+ * `ensureAllowance()`. BUY defaults to the exact `reservePrice` the contract
+ * escrows at request time; SHORT defaults to MaxUint256 because the settle-time
+ * collateral draw is not knowable from the request. `--approve-amount`
+ * overrides either with a decimal amount or an explicit `max`.
  */
+/**
+ * Decide what allowance amount `--ensure-allowance` should target.
+ *
+ * Pure and exported so the direction-dependent default is unit-testable —
+ * a regression here is silent (an under-approval just reverts later, at
+ * settlement), so it must not depend on running a live RFQ to observe.
+ *
+ * @param approveAmount raw `--approve-amount` flag ('max' | decimal | undefined)
+ * @param isBuy         `params.isRequestingLongPosition`
+ * @param reservePrice  `req.reservePrice` — what the factory escrows for a BUY
+ * @param parseCustom   converts a decimal flag value to token units
+ */
+export function resolveApproveTarget(
+  approveAmount: string | undefined,
+  isBuy: boolean,
+  reservePrice: bigint,
+  parseCustom: (value: string) => bigint
+): { target: bigint; isMax: boolean } {
+  if (approveAmount === 'max') return { target: MaxUint256, isMax: true };
+  if (approveAmount !== undefined) return { target: parseCustom(approveAmount), isMax: false };
+  // Defaults. BUY escrows exactly reservePrice at request time, so approve
+  // exactly that. SHORT's settle-time draw is not derivable from the request
+  // (see maybeEnsureCollateralAllowance), so it stays unlimited rather than
+  // under-approving and reverting after a maker has already committed.
+  return isBuy ? { target: reservePrice, isMax: false } : { target: MaxUint256, isMax: true };
+}
+
 async function maybeEnsureCollateralAllowance(
   result: GetClientResult,
   req: RFQRequest,
@@ -1540,20 +1570,36 @@ async function maybeEnsureCollateralAllowance(
   const minRequired = isBuy ? req.reservePrice : 0n;
   const directionLabel = isBuy ? 'BUY' : 'SHORT';
 
-  // Compute target allowance amount (what we approve TO, if --ensure-allowance is set)
-  let target: bigint;
-  let isMax = false;
-  if (flags.approveAmount === undefined || flags.approveAmount === 'max') {
-    target = MaxUint256;
-    isMax = true;
-  } else {
-    const decimals = Number(await client.erc20.getDecimals(req.params.collateral));
-    target = client.utils.toBigInt(flags.approveAmount, decimals);
-  }
+  // Compute target allowance amount (what we approve TO, if --ensure-allowance is set).
+  //
+  // Omitting --approve-amount used to mean MaxUint256 on BOTH directions, so a
+  // scripted `rfq request --ensure-allowance --yes` granted the factory an
+  // unlimited allowance non-interactively. BUY now defaults to the exact
+  // reservePrice the contract escrows at request time, matching `book fill`
+  // and `position close`.
+  //
+  // SHORT deliberately still defaults to MaxUint256. The collateral pulled at
+  // settle is a structure-dependent max-loss figure that is NOT carried on the
+  // request — `params.collateralAmount` is hardcoded to 0 by every SDK builder
+  // and the send path rejects any nonzero value ('collateral is pulled by the
+  // contract'), so there is no exact bigint to approve here. Under-approving a
+  // SHORT reverts at settlement AFTER a maker has committed, which is a worse
+  // outcome than a broad allowance; the MaxUint256 warning below still fires,
+  // and `--approve-amount <n>` remains available for users who want to cap it.
+  const decimals =
+    flags.approveAmount !== undefined && flags.approveAmount !== 'max'
+      ? Number(await client.erc20.getDecimals(req.params.collateral))
+      : 0;
+  const { target, isMax } = resolveApproveTarget(
+    flags.approveAmount,
+    isBuy,
+    req.reservePrice,
+    (value) => client.utils.toBigInt(value, decimals)
+  );
   if (target < minRequired) {
     throw new Error(
       `--approve-amount (${target.toString()}) is less than the contract's required ` +
-        `escrow at request time (${minRequired.toString()}). Increase --approve-amount or omit it (defaults to MaxUint256).`
+        `escrow at request time (${minRequired.toString()}). Increase --approve-amount, omit it to approve exactly what is required, or pass --approve-amount max.`
     );
   }
 
@@ -1578,7 +1624,7 @@ async function maybeEnsureCollateralAllowance(
       `BUY RFQ: current allowance on ${req.params.collateral} → OptionFactory (${spender}) ` +
         `is ${current.toString()}, but the contract will escrow ${minRequired.toString()} ` +
         `(your reserve-price total) at request time. Either:\n` +
-        `  • Pass --ensure-allowance to approve in-flow (uses MaxUint256 by default), or\n` +
+        `  • Pass --ensure-allowance to approve in-flow (approves exactly this reserve-price amount by default), or\n` +
         `  • Pre-approve manually: thetanuts wallet approve --token <SYM> --for optionFactory --amount 5`
     );
     (err as Error & { exitCode?: number }).exitCode = 4;
@@ -1632,7 +1678,7 @@ function registerRequest(grp: Command): void {
     )
     .option(
       '--approve-amount <max|n>',
-      'amount to ensure-allowance to (default: max = MaxUint256; or a decimal token amount). Only used with --ensure-allowance.'
+      'allowance amount when --ensure-allowance fires. Default: exact reservePrice for BUY, "max" for SHORT (settle-time collateral is not known at request time). Pass "max" for unlimited (MaxUint256), or a decimal token amount.'
     )
     .option(
       '--pay-with <eth|symbol|addr>',