diff --git a/docs/superpowers/specs/2026-05-31-m2-ai-planning-coach-design.md b/docs/superpowers/specs/2026-05-31-m2-ai-planning-coach-design.md index 79c59b7..f2f2ceb 100644 --- a/docs/superpowers/specs/2026-05-31-m2-ai-planning-coach-design.md +++ b/docs/superpowers/specs/2026-05-31-m2-ai-planning-coach-design.md @@ -72,15 +72,42 @@ type Backend interface { } ``` -Two real adapters: +Two real adapters. The exact invocations below were verified empirically on +this machine (claude 2.1.154, codex-cli 0.135.0); both authenticate via the +existing CLI login — **no API keys**. -- `claudeBackend` runs `claude --print` with the prompt delivered on **stdin**. -- `codexBackend` runs `codex exec --` with the prompt delivered on **stdin**. +- **`claudeBackend`** runs: + ``` + claude --print --tools "" --no-session-persistence --output-format text + ``` + The prompt is delivered on **stdin** (avoids argv limits and shell-escaping; + also dodges a quirk where an empty `--tools ""` positional can be mistaken for + the prompt). The model's answer is exactly **stdout** (trailing newline + trimmed). `--tools ""` disables all tools so it just answers; + `--no-session-persistence` avoids writing resumable session files. Do **not** + use `--bare` (it forces `ANTHROPIC_API_KEY` and ignores the machine's login). + +- **`codexBackend`** runs: + ``` + codex exec --skip-git-repo-check --ignore-user-config --ignore-rules \ + -s read-only -a never --ephemeral -o - + ``` + The prompt is delivered on **stdin** (the trailing `-` tells codex to read it + from stdin). codex's **stdout is not clean** (it includes session preamble), + so the adapter writes the final answer to a per-call **temp file** via `-o`, + then reads and returns that file's contents. The adapter creates the temp file + (`os.CreateTemp`) and removes it on return. The flags matter: + `--ignore-user-config --ignore-rules -s read-only` stop codex from executing + shell commands driven by local config (observed: it otherwise runs tool calls + even for a trivial prompt, adding latency); `-a never` disables approval + prompts for headless use; `--ephemeral` skips persisting session files; + `--skip-git-repo-check` lets it run anywhere. Both use `os/exec` with the `ctx` passed to `exec.CommandContext` so a timeout cancels the child process. Each adapter stores its command name and base args in struct fields so argument construction is unit-testable without spawning a -process. +process. The codex adapter's temp-file handling lives inside its `Run` so the +`Backend` interface stays uniform (`Run(ctx, prompt) (string, error)`). A selector constructs the configured backend: @@ -150,6 +177,14 @@ All parse/validation failures return a non-nil error; the caller degrades gracefully (see below). Sentinel errors: `ErrEmptyResponse`, `ErrNoJSON`, `ErrInvalidProposal`. +Both CLIs also offer native structured-output flags (claude +`--output-format json --json-schema`; codex `--output-schema `) that would +guarantee shape. We deliberately do **not** use them in M2: they diverge the two +adapters (different flags, envelope vs file) and would push schema concerns into +the `Backend` layer. Prompt-instructed JSON + tolerant `extractJSON` keeps the +`Backend` interface uniform and the parsing in one place. Native schemas remain a +clean future robustness upgrade behind the same `Coach` boundary. + ### `session.Controller` — async coach orchestration A new method drives the coach using the **exact concurrency pattern** already in @@ -179,7 +214,9 @@ Behavior of `RequestCoach`: unlock, `notify()` (broadcasts the pending state). 4. Launch a goroutine: - `ctx, cancel := context.WithTimeout(context.Background(), coachTimeout)` - (`coachTimeout = 30 * time.Second`); `defer cancel()`. + (`coachTimeout = 60 * time.Second` — codex in particular runs tens of + seconds even for trivial prompts; 60s gives a real coaching prompt + headroom); `defer cancel()`. - Call `coach.Coach(ctx, intent)`. - Lock. **If `gen != c.coachGen` or `runtimeState != RuntimePlanning`, unlock and return** (stale result — a newer request superseded this one, or @@ -332,7 +369,7 @@ as a broken Planning view: | ------- | ------ | | No backend wired (`SetCoach` never called / nil) | `RequestCoach` sets `status=error`, "coach unavailable"; returns nil | | CLI binary missing | `backend.Run` errors → goroutine sets `status=error` | -| CLI timeout (>30s) | `context` cancels child → error → `status=error` | +| CLI timeout (>60s) | `context` cancels child → error → `status=error` | | Empty / non-JSON output | `extractJSON`/`parseProposal` error → `status=error` | | Missing/empty fields, non-positive timebox | `parseProposal` error → `status=error` | | Request issued outside planning | `RequestCoach` returns `ErrNotPlanning` → HTTP 400 | @@ -364,8 +401,10 @@ constraint. - `Service.Coach` against a `fakeBackend` returning canned strings: success, chatty-wrapped success, malformed → error. - `claudeBackend`/`codexBackend`: argument construction is correct and the prompt - is routed to stdin (assert on the built `*exec.Cmd` fields; do not spawn the - real CLI). + is routed to stdin (assert on the built `*exec.Cmd` `Args`/`Stdin` fields; do + not spawn the real CLI). For codex, assert the `-o ` flag is present + and that `Run` would read that path (factor the temp-file path out so it is + injectable/observable in the test). - `NewBackend`: returns claude by default, codex by name, error on unknown. **`session` package** (with a fake `ai.Coach`):