docs(02): add code review report

.planning/phases/02-env-audit-and-cli-polish/02-REVIEW.md

@@ -0,0 +1,127 @@
+---
+phase: 02-env-audit-and-cli-polish
+reviewed: 2026-04-09T12:00:00Z
+depth: standard
+files_reviewed: 2
+files_reviewed_list:
+  - claudebox.sh
+  - flake.nix
+findings:
+  critical: 1
+  warning: 3
+  info: 2
+  total: 6
+status: issues_found
+---
+
+# Phase 2: Code Review Report
+
+**Reviewed:** 2026-04-09
+**Depth:** standard
+**Files Reviewed:** 2
+**Status:** issues_found
+
+## Summary
+
+Reviewed `claudebox.sh` (298 lines, shell script) and `flake.nix` (49 lines, Nix expression). The shell script is well-structured with good separation of concerns (flag parsing, audit display, sandbox exec). The Nix flake is clean and idiomatic.
+
+Key concerns: the `CLAUDEBOX_EXTRA_ENV` escape hatch allows passing arbitrary environment variables into the sandbox without any blocklist, which undermines the core security model. There are also several robustness issues around variable handling and the dry-run output not matching actual execution.
+
+## Critical Issues
+
+### CR-01: CLAUDEBOX_EXTRA_ENV allows smuggling secrets into sandbox
+
+**File:** `claudebox.sh:162-172`
+**Issue:** The `CLAUDEBOX_EXTRA_ENV` escape hatch accepts any variable name without validation. A user (or a parent process) could set `CLAUDEBOX_EXTRA_ENV=SSH_AUTH_SOCK,GPG_AGENT_INFO,AWS_SECRET_ACCESS_KEY` and those values would be injected into the sandbox. This directly violates the project's core constraint: "Secrets never enter the Claude Code environment." While `mask_value` hides sensitive values in the audit display, the actual values are still passed via `--setenv`. An attacker or misconfigured environment could bypass the entire allowlist model.
+**Fix:** Add a blocklist check before accepting extra vars:
+```bash
+# Blocklist of vars that must never enter the sandbox
+BLOCKED_VARS="SSH_AUTH_SOCK|SSH_AGENT_PID|GPG_AGENT_INFO|GPG_TTY|AWS_SECRET_ACCESS_KEY|AWS_SESSION_TOKEN|GITHUB_TOKEN|GH_TOKEN|TAILSCALE_.*"
+
+for var in "${EXTRAS[@]}"; do
+  var="${var// /}"
+  if [[ -n "$var" ]] && [[ -v "$var" ]]; then
+    if [[ "$var" =~ ^($BLOCKED_VARS)$ ]]; then
+      echo "${RED}Blocked:${RESET} $var is not allowed in sandbox" >&2
+      continue
+    fi
+    ENV_ARGS+=(--setenv "$var" "${!var}")
+    AUDIT_EXTRA_KEYS+=("$var")
+    AUDIT_EXTRA_VALS[$var]="${!var}"
+  fi
+done
+```
+
+## Warnings
+
+### WR-01: Dry-run output diverges from actual bwrap invocation
+
+**File:** `claudebox.sh:240-272`
+**Issue:** The dry-run block manually reconstructs the bwrap command with `echo` statements (lines 242-270) rather than deriving it from the same data used by the actual `exec bwrap` call (lines 275-298). If a mount is added or changed in the real invocation, the dry-run output will silently become stale. The `--bind /nix/var/nix` mount and the `--ro-bind /etc/nix` mount are both present in both places today, but this is a maintenance hazard -- the two blocks must be kept in sync manually.
+**Fix:** Build the bwrap args array once, then use it for both dry-run printing and actual execution:
+```bash
+BWRAP_ARGS=(
+  --clearenv
+  "${ENV_ARGS[@]}"
+  --tmpfs /
+  --proc /proc
+  # ... all mounts ...
+  -- "${SANDBOX_CMD[@]}"
+)
+
+if [[ "$DRY_RUN" == true ]]; then
+  printf 'bwrap'
+  printf ' %q' "${BWRAP_ARGS[@]}"
+  printf '\n'
+  exit 0
+fi
+
+exec bwrap "${BWRAP_ARGS[@]}"
+```
+
+### WR-02: Temp file cleanup race with exec
+
+**File:** `claudebox.sh:108-109`
+**Issue:** Line 109 sets `trap 'rm -f "$GITCONFIG_TMP"' EXIT` to clean up the temp gitconfig. However, line 275 uses `exec bwrap ...` which replaces the current process -- the EXIT trap never fires. The temp file leaks on every successful run. This is not a security issue (the file contains only name/email), but it accumulates stale files in `/tmp`.
+**Fix:** Either accept the leak (temp files are cleaned by the OS on reboot) and remove the misleading trap, or delete the file after bwrap starts by restructuring to not use `exec`:
+```bash
+# Option A: Remove the misleading trap, add a comment
+GITCONFIG_TMP=$(mktemp)
+# Note: leaked intentionally -- exec replaces process, trap won't fire.
+# /tmp is cleaned on reboot. File contains only git name/email.
+```
+
+### WR-03: Unquoted variables in dry-run echo statements
+
+**File:** `claudebox.sh:264-268`
+**Issue:** Several dry-run echo statements use unquoted shell variables: `$HOME`, `$CWD`. If these contain spaces (unlikely for HOME but possible for CWD), the dry-run output would be misleading -- it would show split words rather than the actual path. The real `exec bwrap` call properly quotes these on lines 293-297.
+**Fix:**
+```bash
+echo "  --tmpfs \"$HOME\" \\"
+echo "  --bind \"$HOME/.claudebox\" \"$HOME/.claude\" \\"
+echo "  --bind \"$CWD\" \"$CWD\" \\"
+echo "  --chdir \"$CWD\" \\"
+```
+
+## Info
+
+### IN-01: Hardcoded system architecture in flake.nix
+
+**File:** `flake.nix:18`
+**Issue:** `system = "x86_64-linux";` hardcodes the architecture. This prevents the flake from being used on aarch64-linux or other systems. This is fine if claudebox is strictly for the developer's NixOS desktop, but limits portability.
+**Fix:** Use `flake-utils` or `nixpkgs.lib.genAttrs` to support multiple systems:
+```nix
+nixpkgs.lib.genAttrs ["x86_64-linux" "aarch64-linux"] (system: ...)
+```
+
+### IN-02: `pass` variable in --check uses string comparison instead of boolean
+
+**File:** `claudebox.sh:23,31,56`
+**Issue:** The `pass` variable is set to the string `true`/`false` and compared with `[[ "$pass" == true ]]`. This works in bash but is a code smell -- it looks like a boolean but is a string. Minor readability concern, no functional impact given `writeShellApplication` ensures bash.
+**Fix:** No change needed -- this is idiomatic bash. Noting for awareness only.
+
+---
+
+_Reviewed: 2026-04-09_
+_Reviewer: Claude (gsd-code-reviewer)_
+_Depth: standard_