diff --git a/bin/agent-executor.js b/bin/agent-executor.js index 8d6a448f6..ea69abd4d 100755 --- a/bin/agent-executor.js +++ b/bin/agent-executor.js @@ -10,6 +10,9 @@ import { AgentCLI } from '../dist/agents/core/AgentCLI.js'; import { AgentRegistry } from '../dist/agents/registry.js'; +import { installProcessGuards } from '../dist/utils/process-guards.js'; + +installProcessGuards(); // Load built-in agent (codemie-code) const agent = AgentRegistry.getAgent('codemie-code'); diff --git a/bin/codemie-claude-acp.js b/bin/codemie-claude-acp.js index da6c9211d..2c05fe969 100755 --- a/bin/codemie-claude-acp.js +++ b/bin/codemie-claude-acp.js @@ -10,6 +10,9 @@ import { AgentCLI } from '../dist/agents/core/AgentCLI.js'; import { AgentRegistry } from '../dist/agents/registry.js'; +import { installProcessGuards } from '../dist/utils/process-guards.js'; + +installProcessGuards(); const agent = AgentRegistry.getAgent('claude-acp'); if (!agent) { diff --git a/bin/codemie-claude.js b/bin/codemie-claude.js index d8edfffe7..7d65a8e7f 100755 --- a/bin/codemie-claude.js +++ b/bin/codemie-claude.js @@ -7,6 +7,9 @@ import { AgentCLI } from '../dist/agents/core/AgentCLI.js'; import { AgentRegistry } from '../dist/agents/registry.js'; +import { installProcessGuards } from '../dist/utils/process-guards.js'; + +installProcessGuards(); const agent = AgentRegistry.getAgent('claude'); if (!agent) { diff --git a/bin/codemie-codex.js b/bin/codemie-codex.js index 8ce77c3cf..bc8ab455d 100755 --- a/bin/codemie-codex.js +++ b/bin/codemie-codex.js @@ -7,6 +7,9 @@ import { AgentCLI } from '../dist/agents/core/AgentCLI.js'; import { AgentRegistry } from '../dist/agents/registry.js'; +import { installProcessGuards } from '../dist/utils/process-guards.js'; + +installProcessGuards(); const agent = AgentRegistry.getAgent('codex'); if (!agent) { diff --git a/bin/codemie-copilot.js b/bin/codemie-copilot.js index b7f030b28..95749a808 100755 --- a/bin/codemie-copilot.js +++ b/bin/codemie-copilot.js @@ -14,6 +14,9 @@ import { AgentRegistry } from '../dist/agents/registry.js'; import { resolveCopilotModel } from '../dist/agents/plugins/copilot-cli/index.js'; import { ConfigLoader } from '../dist/utils/config.js'; import { getCodemiePath } from '../dist/utils/paths.js'; +import { installProcessGuards } from '../dist/utils/process-guards.js'; + +installProcessGuards(); const SAVED_MODEL_PATH = getCodemiePath('agents', 'copilot-cli', 'model.json'); diff --git a/bin/codemie-gemini.js b/bin/codemie-gemini.js index b8de0e033..f15e70476 100755 --- a/bin/codemie-gemini.js +++ b/bin/codemie-gemini.js @@ -7,6 +7,9 @@ import { AgentCLI } from '../dist/agents/core/AgentCLI.js'; import { AgentRegistry } from '../dist/agents/registry.js'; +import { installProcessGuards } from '../dist/utils/process-guards.js'; + +installProcessGuards(); const agent = AgentRegistry.getAgent('gemini'); if (!agent) { diff --git a/bin/codemie-kimi-acp.js b/bin/codemie-kimi-acp.js index 94f9b36f6..deafb5e65 100755 --- a/bin/codemie-kimi-acp.js +++ b/bin/codemie-kimi-acp.js @@ -7,6 +7,9 @@ import { AgentCLI } from '../dist/agents/core/AgentCLI.js'; import { AgentRegistry } from '../dist/agents/registry.js'; +import { installProcessGuards } from '../dist/utils/process-guards.js'; + +installProcessGuards(); const agent = AgentRegistry.getAgent('kimi-acp'); if (!agent) { diff --git a/bin/codemie-kimi.js b/bin/codemie-kimi.js index 11795dc4c..706c4700b 100755 --- a/bin/codemie-kimi.js +++ b/bin/codemie-kimi.js @@ -7,6 +7,9 @@ import { AgentCLI } from '../dist/agents/core/AgentCLI.js'; import { AgentRegistry } from '../dist/agents/registry.js'; +import { installProcessGuards } from '../dist/utils/process-guards.js'; + +installProcessGuards(); const agent = AgentRegistry.getAgent('kimi'); if (!agent) { diff --git a/bin/codemie-mcp-proxy.js b/bin/codemie-mcp-proxy.js index dc895b282..ab74c34ee 100755 --- a/bin/codemie-mcp-proxy.js +++ b/bin/codemie-mcp-proxy.js @@ -16,6 +16,9 @@ import { appendFileSync, mkdirSync } from 'fs'; import { join } from 'path'; import { homedir } from 'os'; +// No installProcessGuards(): this entrypoint owns its own uncaughtException/ +// unhandledRejection handlers below and deliberately survives rejections. + // Boot-level file logger (before any imports that might touch stdout) const logDir = join(homedir(), '.codemie', 'logs'); const logFile = join(logDir, 'mcp-proxy.log'); diff --git a/bin/codemie-opencode.js b/bin/codemie-opencode.js index 318094847..7ff1f6c50 100755 --- a/bin/codemie-opencode.js +++ b/bin/codemie-opencode.js @@ -7,6 +7,9 @@ import { AgentCLI } from '../dist/agents/core/AgentCLI.js'; import { AgentRegistry } from '../dist/agents/registry.js'; +import { installProcessGuards } from '../dist/utils/process-guards.js'; + +installProcessGuards(); const agent = AgentRegistry.getAgent('opencode'); if (!agent) { diff --git a/bin/codemie-openwiki.js b/bin/codemie-openwiki.js index 1ef7dbb9f..648c3b712 100755 --- a/bin/codemie-openwiki.js +++ b/bin/codemie-openwiki.js @@ -7,6 +7,9 @@ import { AgentCLI } from '../dist/agents/core/AgentCLI.js'; import { AgentRegistry } from '../dist/agents/registry.js'; +import { installProcessGuards } from '../dist/utils/process-guards.js'; + +installProcessGuards(); const agent = AgentRegistry.getAgent('openwiki'); if (!agent) { diff --git a/bin/codemie-pi.js b/bin/codemie-pi.js index 6ad4a4e19..2c3341546 100755 --- a/bin/codemie-pi.js +++ b/bin/codemie-pi.js @@ -7,6 +7,9 @@ import { AgentCLI } from '../dist/agents/core/AgentCLI.js'; import { AgentRegistry } from '../dist/agents/registry.js'; +import { installProcessGuards } from '../dist/utils/process-guards.js'; + +installProcessGuards(); const agent = AgentRegistry.getAgent('pi'); if (!agent) { diff --git a/bin/codemie.js b/bin/codemie.js index b52d7df8c..918389010 100755 --- a/bin/codemie.js +++ b/bin/codemie.js @@ -7,6 +7,9 @@ import { MigrationRunner } from '../dist/migrations/index.js'; import { checkAndPromptForUpdate } from '../dist/utils/cli-updater.js'; +import { installProcessGuards } from '../dist/utils/process-guards.js'; + +installProcessGuards(); // Auto-run pending migrations (happens at startup) // Migrations are tracked in ~/.codemie/migrations.json and only run once diff --git a/bin/proxy-daemon.js b/bin/proxy-daemon.js index 619914ef3..485bbe422 100755 --- a/bin/proxy-daemon.js +++ b/bin/proxy-daemon.js @@ -3,6 +3,9 @@ /** * CodeMie Proxy Daemon entry point * Imports compiled daemon from dist/ + * + * No installProcessGuards(): long-running daemon — exiting on the first + * unhandled rejection would be wrong here. */ import('../dist/bin/proxy-daemon.js').catch((error) => { process.stderr.write(`[proxy-daemon] Fatal: ${error.message}\n`); diff --git a/docs/AUTHENTICATION.md b/docs/AUTHENTICATION.md index 29f08051a..2300b2721 100644 --- a/docs/AUTHENTICATION.md +++ b/docs/AUTHENTICATION.md @@ -104,17 +104,38 @@ codemie setup # Run wizard again ### Non-Interactive Environments (CI/Automation) When SSO credentials are missing or expired, the CLI normally offers an interactive -re-authentication prompt. In a non-interactive environment — no TTY attached to `stdin`, as in CI +re-authentication prompt. In a non-interactive environment — no TTY attached to `stdin`, as in most CI pipelines, cron jobs, or piped/redirected invocations — that prompt is automatically skipped. The -CLI detects the missing TTY, fails fast with a clear message (e.g. "No valid SSO credentials found. -Please run `codemie setup` interactively before using this command."), and exits non-zero instead of -hanging. +CLI detects the missing TTY, fails fast, and exits non-zero instead of hanging. + +The failure is a single actionable line on **stderr**, with no stack trace: + +```console +$ codemie sdk assistants list < /dev/null +❌ SSO authentication required. Please run "codemie setup" with SSO provider first. +$ echo $? +1 +``` + +Diagnostics go to stderr rather than stdout, so piping stdout or consuming `--json` output stays +clean. Progress spinners are suppressed whenever their output stream is not a terminal — including +`codemie … > run.log 2>&1` from an interactive shell — so captured logs do not fill with +cursor-control escape sequences. Spinner suppression tracks the **output** stream, independently of +whether `stdin` is redirected. There is no separate `--non-interactive` flag to set — detection is automatic, based solely on -whether `stdin` is a TTY. To run unattended in CI, either authenticate ahead of time +whether `stdin` is a TTY. + +> **Known limitation.** Because detection looks only at `stdin`, it does **not** fire in an +> environment that allocates a pseudo-TTY — `docker run -t`, and some Jenkins and GitLab runner +> configurations. There, a missing SSO session can still reach the interactive prompt and block. The +> `CI` environment variable is not consulted. If your runner allocates a TTY, do not rely on +> automatic detection; use one of the two unattended options below. + +To run unattended in CI, either authenticate ahead of time (`codemie profile login`) with credentials persisted before the run, or use [JWT Bearer Authorization](#jwt-bearer-authorization) instead, which requires no interactive -session at all. +session at all. The JWT path never prompts, so it is the safest choice for a pty-allocating runner. ## Enterprise SSO Features diff --git a/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/ac4-investigation.md b/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/ac4-investigation.md new file mode 100644 index 000000000..c38baed5b --- /dev/null +++ b/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/ac4-investigation.md @@ -0,0 +1,76 @@ +# AC4 — `ERR_USE_AFTER_CLOSE` on kill: investigation result + +**Acceptance criterion:** "Killing during prompt does not produce readline lifecycle crash." + +**Verdict: exercised and passing.** 12 of 12 runs reached the interactive re-auth prompt and were then interrupted; none produced `ERR_USE_AFTER_CLOSE` or any readline lifecycle error. + +This supersedes two earlier verdicts in this document ("cannot reproduce", then "precondition not constructible"). Both were wrong, and both were wrong for the same reason: the test environment, not the product. See *Why the earlier attempts failed*. + +## Result + +Harness: `node-pty` (a real pty, so `stdin.isTTY === true`), isolated `CODEMIE_HOME` per run holding only a copy of the config — **no credentials**, which is what makes `validateAuth` fail and the prompt appear. Interrupt delivered once `Re-authenticate now?` is actually on screen. + +| Mode | delay after prompt | `ERR_USE_AFTER_CLOSE` | exit | +|---|---|---|---| +| Ctrl-C (`\x03`) | 0 / 300 / 1500 ms | none | code 0, no signal | +| `SIGINT` | 0 / 300 / 1500 ms | none | code 0, signal 2 | +| `SIGTERM` | 0 / 300 / 1500 ms | none | code 0, signal 15 | +| `SIGHUP` | 0 / 300 / 1500 ms | none | code 0, signal 1 | + +**Prompt reached: 12/12. Readline crashes: 0.** + +The ticket hedges with "*can* crash", so this is evidence of non-reproduction on this build and platform (macOS, Node v24.19.0) — not proof the failure mode is impossible everywhere. But it is now a real negative result, obtained with the criterion's precondition genuinely satisfied, rather than an absence of testing. + +### Incidental finding, not AC4 + +**Ctrl-C at the prompt exits with code 0.** Conventionally an interrupt at a prompt should exit non-zero (130 by convention). A script that runs `codemie sdk …`, has the user hit Ctrl-C at the re-auth prompt, and checks `$?` would conclude the command succeeded. Out of scope here; worth its own ticket. + +## Why the earlier attempts failed + +Every earlier run in this investigation was executed from a shell that **CodeMie itself had launched** (`CODEMIE_AGENT=claude`, `CODEMIE_CLIENT_TYPE=codemie-claude`). That parent session exports provider settings into the environment: + +``` +CODEMIE_PROVIDER=anthropic-subscription +CODEMIE_AUTH_METHOD=manual +CODEMIE_PROFILE_CONFIG={"name":"default","provider":"anthropic-subscription",…} +``` + +`ConfigLoader` gives `process.env` precedence over both the global and the project-local config file (`utils/config.ts`), so **every probe ran as `anthropic-subscription`, never as `ai-run-sso`** — regardless of which config file was planted in the isolated `CODEMIE_HOME`. + +The `anthropic-subscription` provider has no `validateAuth` and no `promptForReauth`. So `promptReauthentication` hit its final `throw` immediately, and the run terminated long before any prompt could appear. Traced directly: + +| | polluted env | env cleaned | +|---|---|---| +| `config.provider` | `anthropic-subscription` | `ai-run-sso` | +| `setupSteps.validateAuth` | `undefined` | `function` | +| `setupSteps.promptForReauth` | `undefined` | `function` | +| `validateAuth(config)` | not called | `{valid: false, error: 'No SSO credentials found for …'}` | +| `isNonInteractiveEnvironment()` | — | `false` | +| Reaches `promptForReauth`? | **no** | **yes** | + +Clearing every `CODEMIE_*` variable except `CODEMIE_HOME` before spawning is what fixed it. + +**Consequence for the superseded recipe.** A previous draft proposed planting credentials whose `apiUrl` points at a closed port so `validateAuth` would fail deterministically. That was never needed: **absent** credentials already produce `{valid: false}` via the `No SSO credentials found` branch, which is enough to reach the prompt. The recipe was solving a problem that did not exist, because the real blocker was environmental. + +## Retracted claims + +Four, kept on record so nobody re-derives them. All four were **environment or harness artifacts misread as product behaviour** — the recurring failure mode of this investigation. + +1. **"73 MB ora escape-sequence flood."** Not real. 1 470 bytes under a real pty. Only appears under `script(1)`, which yields a pty with no usable `stdout.columns`, breaking ora's line-clearing arithmetic. +2. **"Case C proves a TTY hang on the prompt."** Not the prompt. The same `script` invocation still hangs 25 s against the *fixed* build, while `script` around an immediately-exiting child returns in 0 s. The capture stalls at `⠋ Loading configuration...`, before credentials are read, with a stray `^D` — an immediately-EOF stdin forwarded into the pty. +3. **"The spinner suppression in this MR mitigates AC4."** False. Suppression is gated on the environment being non-interactive, so it cannot fire in a scenario that requires a TTY. This was the sole justification offered for shipping without AC4 and did not survive inspection. +4. **"The re-auth prompt is unreachable from `codemie sdk …`."** False, and the most misleading of the four, because it questioned the ticket's premise on the strength of a polluted environment. The prompt is reachable, reliably — 12/12. + +**Methodological notes for the next person:** + +- Use `node-pty` (already a dependency, wrapped by `tests/helpers/pty-session.ts`) for anything TTY-dependent. `script(1)` injects its own stdin and terminal geometry and produced two of the four artifacts above. +- When testing CLI behaviour from inside a CodeMie-launched agent session, **strip `CODEMIE_*` from the child environment**. `CODEMIE_HOME` isolates configuration files but not the environment variables that outrank them. + +## Recommendation + +AC4 can be marked **verified** on this build, citing the table above, rather than deferred. + +Two follow-ups, neither blocking this MR: + +1. **Ctrl-C at a prompt exits 0** (see above) — its own ticket. +2. If the reporter can still reproduce `ERR_USE_AFTER_CLOSE`, the missing variable is platform or Node version, not the scenario — this investigation now covers the scenario. Ask for OS, Node version, and the exact command. diff --git a/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/acceptance-verification.md b/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/acceptance-verification.md new file mode 100644 index 000000000..7e26c65fa --- /dev/null +++ b/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/acceptance-verification.md @@ -0,0 +1,74 @@ +# EPMCDME-14148 — final acceptance verification + +Full re-verification of all four acceptance criteria against the branch head, run end-to-end through `bin/codemie.js` exactly as the ticket's *Steps to Reproduce* describe. + +**Result: 4 / 4 pass.** + +## Why this re-run exists + +The first pass at AC1 and AC2 was executed from a shell that CodeMie itself had launched, which exports `CODEMIE_PROVIDER=anthropic-subscription` and `CODEMIE_PROFILE_CONFIG`. `ConfigLoader` gives `process.env` precedence over both config files, so those runs resolved to the wrong provider — the same pollution that produced the four retracted claims in `ac4-investigation.md`. They happened to pass anyway (the message originates in `sdk-client.ts`, which is provider-agnostic), but a criterion verified under the wrong provider is not verified. + +A second, subtler harness bug had to be fixed first: the shell here is **zsh**, which does not word-split an unquoted `$VAR`, so `env $FLAGS …` passed all 38 `-u` flags as a single argument and stripped nothing. Environment cleaning is therefore done inside Node, not in the shell. + +## Conditions + +Matching the ticket's preconditions: + +- **No valid SSO session** — a throwaway `CODEMIE_HOME` per run, holding a copy of the config and **no credentials**. The real `~/.codemie` is never touched. +- **Non-interactive stdin** — `stdio: ['ignore', …]` for the AC1/AC2 runs. +- **Clean environment** — every `CODEMIE_*` except `CODEMIE_HOME` deleted from the child env, so provider resolution comes from config (`ai-run-sso`), not from the parent agent session. +- Interactive cases use `node-pty` (a real pty), never `script(1)` — see the methodological note in `ac4-investigation.md`. + +## AC1 — "Non-TTY stdin skips interactive prompt" + +| Check | Result | +|---|---| +| `Re-authenticate now?` absent from output | ✅ | +| Completes without hanging | ✅ 964 ms | +| **Control:** same command *with* a TTY does prompt | ✅ prompt at 604 ms | + +The control is the part that matters. Without it, AC1 passes trivially whenever anything else terminates the run early — which is precisely how the earlier investigation fooled itself. With a TTY the prompt appears; without one it does not; the only difference is the TTY, so the `isNonInteractiveEnvironment()` guard is demonstrably what skips it. + +Detection matcher is the exact inquirer question `Re-authenticate now?`. An earlier draft matched `/authentication required/i`, which also matches the legitimate error text `SSO authentication required` and produced a false "prompt appeared". + +## AC2 — "CLI exits non-zero with clear remediation" + +| Check | Result | +|---|---| +| Exit code non-zero | ✅ `1` | +| Message names `codemie setup` | ✅ | +| No stack trace (`^\s+at\s`) | ✅ | +| No leaked `ConfigurationError:` prefix | ✅ | +| No `Node.js v…` crash banner | ✅ | +| stdout free of the diagnostic | ✅ | + +Emitted on stderr: + +``` +❌ SSO authentication required. Please run "codemie setup" with SSO provider first. +``` + +## AC3 — "Optional `--non-interactive` or `--ci` behavior is supported **or** documented" + +| Check | Result | +|---|---| +| No flag registered in `src/cli/index.ts` | ✅ deliberate — EPMCDME-13953 recorded it out of scope | +| Absence documented in `AUTHENTICATION.md` | ✅ | +| Boundary documented (pty-allocating runners) | ✅ added by this MR | + +Satisfied through the "or documented" limb. This MR strengthens it: the page previously claimed stdin-TTY detection was sufficient for CI, which overstated it — a `docker run -t` runner still reaches the prompt. That limitation is now stated explicitly, with JWT auth given as the unattended path. + +## AC4 — "Killing during prompt does not produce readline lifecycle crash" + +| Check | Result | +|---|---| +| Prompt genuinely reached before interrupting | ✅ 3/3 | +| `ERR_USE_AFTER_CLOSE` | ✅ none | + +A broader sweep — 4 signal modes × 3 delays — is recorded in `ac4-investigation.md`: **12/12 reached the prompt, 0 crashes.** + +The ticket hedges with "*can* crash", so this is a negative result on this build and platform (macOS, Node v24.19.0), not proof the failure is impossible everywhere. It is now a real negative result, with the precondition genuinely satisfied. + +## Out of scope, worth its own ticket + +**Ctrl-C at the prompt exits with code 0.** Conventionally an interrupt should exit non-zero (130). A script checking `$?` after the user interrupts would conclude the command succeeded. Unrelated to the four criteria; not fixed here. diff --git a/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/actual-complexity.json b/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/actual-complexity.json new file mode 100644 index 000000000..708c5fa5c --- /dev/null +++ b/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/actual-complexity.json @@ -0,0 +1,118 @@ +{ + "task": "Make non-interactive (non-TTY) SSO authentication failure fail fast and cleanly across the CLI — exit non-zero with an actionable stderr message instead of hanging on a re-auth prompt or printing a raw stack trace — via a new process-level guard module installed in every bin/ entrypoint plus TTY-aware auth, SDK-client and provider-validation paths.", + "generated": "2026-09-03T00:00:00Z", + "dimensions": { + "component_scope": { + "score": 5, + "label": "XL", + "affected": "new process-guard module (src/utils/process-guards.ts), shared auth utility (src/utils/auth.ts), SDK client bootstrap (src/utils/sdk-client.ts), provider-core auth validation (src/providers/core/auth-validation.ts), SDK CLI command utils (src/cli/commands/sdk/utils/cli-utils.ts), all 12 bin/ entrypoints (codemie, agent-executor, claude, claude-acp, codex, copilot, gemini, kimi, kimi-acp, opencode, openwiki, pi), integration test harness (tests/helpers/cli-runner.ts), docs/AUTHENTICATION.md", + "layers": "Process/entrypoint bootstrap (bin/), Service (auth + sdk-client utils), Provider-core (SSO auth validation), CLI command utils, Test infrastructure, Docs" + }, + "requirements_clarity": { + "score": 3, + "label": "M", + "status": "Partially Clear", + "gaps": "The headline criterion was unambiguous and directly testable (non-zero exit, no hang, no raw stack trace, actionable remediation string — asserted verbatim in tests/integration/cli-commands/non-interactive-auth.test.ts). Three design decisions were not specified up front and were resolved during implementation: (1) which stream the diagnostic goes to — settled on stderr so piped stdout and --json consumers stay clean; (2) whether the stack is discarded or relocated — settled on synchronous appendFileSync persistence to the log file while the console shows only the actionable line; (3) how far the guard installation should reach — settled on every bin/ entrypoint rather than AgentCLI, so constructing an AgentCLI never mutates global process state." + }, + "technical_risk": { + "score": 5, + "label": "XL", + "risk_factors": "Authentication/authorization failure paths are directly modified (src/utils/auth.ts, src/providers/core/auth-validation.ts, src/utils/sdk-client.ts). No prior precedent in the codebase for process-level error guards — process-guards.ts installs global unhandledRejection/uncaughtException handlers into all 12 entrypoints, mutating global process state and changing exit semantics for the entire CLI surface. Non-obvious async/IO ordering hazard: logger writes through an fs.WriteStream that process.exit() does not drain, so a fatal record is lost on a cold stream — this required a separate synchronous appendFileSync path (persistFatalSync). Subtle control flow in getAuthenticatedClient, where promptReauthentication's generic 'Authentication expired' throw would shadow the upstream message that names the actual remediation. Test-harness hazard: runSilent wraps execSync, which blocks the Vitest worker synchronously so testTimeout cannot interrupt it — a regression to the original hang would wedge CI instead of failing, forcing a new timedOut flag on CommandResult plus an elapsed-time assertion. Realized-risk evidence: two code-review rounds — the first produced 5 blocking findings, and the check round found 2 further defects introduced by the first fix-up, the signature of an under-scoped risk surface.", + "mitigation": "Guard is additive and trivially reversible (no migration, no schema, no persisted state); installProcessGuards() is idempotent via an `installed` flag; persistFatalSync swallows its own failures so a logging error can never mask the original fatal; process.exitCode is set before any I/O so an early exit still reports non-zero. Coverage was deliberately layered against mock blindness: process-guards.test.ts mocks the logger, while a dedicated companion (process-guards.logfile.test.ts) runs the REAL logger and asserts the stack actually reaches the log file — added specifically because that contract shipped undetected in review round 1 (CR-001). The end-to-end criterion is pinned by an integration test running against an isolated CODEMIE_HOME with stdin: 'ignore'." + }, + "file_change_estimate": { + "score": 5, + "label": "XL", + "modified_files": 20, + "modified_file_list": [ + "bin/agent-executor.js", + "bin/codemie-claude-acp.js", + "bin/codemie-claude.js", + "bin/codemie-codex.js", + "bin/codemie-copilot.js", + "bin/codemie-gemini.js", + "bin/codemie-kimi-acp.js", + "bin/codemie-kimi.js", + "bin/codemie-opencode.js", + "bin/codemie-openwiki.js", + "bin/codemie-pi.js", + "bin/codemie.js", + "docs/AUTHENTICATION.md", + "src/cli/commands/sdk/utils/cli-utils.ts", + "src/providers/core/auth-validation.ts", + "src/utils/auth.ts", + "src/utils/sdk-client.ts", + "src/utils/__tests__/auth.test.ts", + "src/utils/__tests__/sdk-client.test.ts", + "tests/helpers/cli-runner.ts" + ], + "new_files": 6, + "new_file_list": [ + "src/utils/process-guards.ts", + "src/utils/__tests__/process-guards.test.ts", + "src/utils/__tests__/process-guards.logfile.test.ts", + "src/cli/commands/sdk/utils/__tests__/cli-utils.test.ts", + "src/providers/core/__tests__/auth-validation.test.ts", + "tests/integration/cli-commands/non-interactive-auth.test.ts" + ], + "affected_dirs": [ + "bin", + "docs", + "src/utils", + "src/providers/core", + "src/cli/commands/sdk/utils", + "tests/helpers", + "tests/integration/cli-commands" + ] + }, + "dependencies": { + "score": 1, + "label": "XS", + "new_packages": [], + "version_changes": [] + }, + "affected_layers": { + "score": 4, + "label": "L", + "layers_changed": [ + "Process/entrypoint bootstrap (bin/)", + "Service (src/utils/auth.ts, src/utils/sdk-client.ts)", + "Provider-core / External auth (src/providers/core/auth-validation.ts)", + "CLI command utils (src/cli/commands/sdk/utils/cli-utils.ts)", + "Test infrastructure (tests/helpers/cli-runner.ts)" + ], + "schema_migration": false, + "cross_system": false + } + }, + "total": 23, + "size": "L", + "band_range": "21-26", + "files_changed": 34, + "routing": "brainstorming", + "key_reasoning": [ + { + "dimension": "component_scope", + "reason": "Five substantive components plus a new cross-cutting module: process-guards.ts (new, global unhandledRejection/uncaughtException handling with synchronous fatal log persistence), the shared auth utility (error-shadowing fix so the upstream remediation message survives), the SDK client bootstrap (ora spinner suppressed when no TTY, since it emits raw cursor-control escapes into captured output), provider-core auth validation (skip promptForReauth entirely when isNonInteractiveEnvironment(), avoiding ERR_USE_AFTER_CLOSE), and the SDK CLI utils where getSdkClient() became the single gate routing auth acquisition through handleSdkError. The guard is then wired into every one of the 12 bin/ entrypoints, so the change reaches the whole binary surface of the project. Red flag applied: touches core shared utilities (src/utils/auth.ts, src/utils/sdk-client.ts, new src/utils/process-guards.ts) — bumped from L (4) to XL (5)." + }, + { + "dimension": "technical_risk", + "reason": "Highest-signal dimension and the main driver of the delta from the M (19/36) initial estimate. Modifies authentication failure paths (explicit red flag), and installs global process-level handlers with no prior precedent in the codebase — this mutates global process state and changes exit semantics for every entrypoint. Two genuinely non-obvious hazards were only discovered during implementation: the logger's fs.WriteStream is not drained by process.exit() (so the fatal record — the one that matters most — was silently lost, requiring a separate appendFileSync path), and execSync inside the test runner blocks the Vitest worker so testTimeout cannot interrupt a hang, meaning a regression would have wedged CI rather than failing the test. Realized evidence confirms the score: review round 1 produced 5 blocking findings, and the check round found 2 further defects introduced by the round-1 fix-up." + }, + { + "dimension": "file_change_estimate", + "reason": "26 non-artifact files changed (20 modified, 6 new) across 7 directories; the diffstat total of 34 includes 8 SDLC planning artifacts under docs/superpowers/tasks/ which are not implementation. Scored XL rather than XXL deliberately: 12 of the 20 modified files are bin/ entrypoints receiving the identical mechanical two-line addition (import + installProcessGuards()), which is one substantive change repeated, not twelve. Discounting those leaves 14 substantive files with 6 new across 6 directories spanning multiple subsystems — squarely the XL band. Test files outnumber production files (6 test/harness files vs 5 production source files), reflecting the layered anti-mock-blindness coverage strategy." + }, + { + "dimension": "affected_layers", + "reason": "Four production layers plus test infrastructure: process/entrypoint bootstrap (bin/), Service (auth + sdk-client utils), Provider-core external auth (SSO validation), and CLI command utils — with two cross-cutting concerns added across them simultaneously (auth failure handling and observability, via fatal log-file persistence). Held at L rather than XL because there is no persistence layer, no schema migration, and no new external integration: the SSO path is only made to fail cleanly, not re-plumbed." + } + ], + "red_flags_applied": [ + "Technical Risk bumped from L (4) to XL (5): affects authentication/authorization — the change modifies the SSO auth failure path in src/utils/auth.ts, src/providers/core/auth-validation.ts and src/utils/sdk-client.ts.", + "Component Scope bumped from L (4) to XL (5): touches core shared utilities — src/utils/auth.ts, src/utils/sdk-client.ts, and the new cross-cutting src/utils/process-guards.ts.", + "Component Scope 'affects multiple workflows or agents' red flag noted but NOT double-counted: the 12 agent entrypoint edits are the same underlying fact as the shared-utility bump already applied, and a second bump would have pushed the dimension to XXL on a mechanical two-line repetition.", + "No dependency, schema-migration, data-migration, real-time/streaming, performance, or new-external-integration red flags apply." + ], + "split_recommendation": null +} diff --git a/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/code-review-check.json b/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/code-review-check.json new file mode 100644 index 000000000..0c9942f9a --- /dev/null +++ b/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/code-review-check.json @@ -0,0 +1,57 @@ +{ + "decision": "request-changes", + "rationale": "All five original findings are resolved, three of them verified adversarially by an independent checker that re-derived the evidence rather than trusting the author. The check round also found two NEW major defects introduced by the first fix-up: installing the guards from the AgentCLI constructor mutated global process state (listener counts measured going 1->2, with four unit test files constructing AgentCLI), and the added execSync timeout stopped CI wedging without making a hang detectable (ETIMEDOUT yields status: null, which collapsed to exitCode 1 while stderr already held the remediation, so all four cases passed during a genuine 15s hang). Both were fixed and each fix was empirically demonstrated rather than asserted. The decision remains request-changes for one reason only: those two fixes were authored AFTER the independent check and have not themselves been independently verified, and the review contract forbids a third automated round. This is an escalation for a human decision, not an unresolved defect list. AC4 remains unmet by design and is recommended for a separate ticket.", + "confidence": "medium", + "risk_flags": ["auth", "acceptance-criteria-unmet", "unverified-fixup"], + "business_review": [ + {"id": "AC1", "criterion": "Non-TTY stdin skips interactive prompt.", "status": "pass", "notes": "Carried forward. Regression coverage is now genuinely able to fail: CR-005's timedOut assertion closes the hole where a hang passed every assertion."}, + {"id": "AC2", "criterion": "CLI exits non-zero with clear remediation.", "status": "pass", "notes": "Carried forward. Re-confirmed end-to-end after both fix-up rounds: exit 1, single actionable stderr line, no stack trace."}, + {"id": "AC3", "criterion": "Optional --non-interactive or --ci behavior is supported or documented.", "status": "partial", "notes": "Carried forward, improved by CR-004. The documentation now states the pty-CI boundary explicitly instead of implying completeness. Remains 'documented' rather than 'supported' per the EPMCDME-13953 decision and the user's scope call."}, + {"id": "AC4", "criterion": "Killing during prompt does not produce readline lifecycle crash.", "status": "fail", "notes": "Unchanged and unmet. Reclassified honestly as 'precondition not constructed, criterion never exercised' with a concrete reproduction recipe carried forward. Recommended for a separate ticket."} + ], + "standards_review": [ + {"standard": "git-workflow.md - Conventional Commits, scope-enum", "status": "pass", "notes": "Carried forward. All five commits use fix(cli)/docs(cli), both in scope-enum."}, + {"standard": "code-quality.md - ESLint --max-warnings=0", "status": "pass", "notes": "Carried forward; re-run clean after both fix-up rounds."}, + {"standard": "code-quality.md - explicit return types, .js extensions", "status": "pass", "notes": "Carried forward; typecheck clean."}, + {"standard": "testing-patterns.md - unit tests co-located in src/**/__tests__/", "status": "pass", "notes": "Carried forward. The new logfile companion suite is also correctly placed."}, + {"standard": "development-practices.md - do not log stack traces to console; log errors with logger.error", "status": "pass", "notes": "Was fail. Now resolved: the stack reaches the log file (proven by reverting the fix and watching the new test fail with ENOENT) while the console keeps only the actionable line."} + ], + "finding_status": [ + { + "id": "CR-001", + "status": "resolved", + "notes": "Independently verified. The checker reverted reportFatal to the object-literal form and confirmed the new logfile test fails with ENOENT, proving persistFatalSync is load-bearing and the test is non-vacuous. sanitizeLogArgs index confirmed correct; append flag 'a' on both writers confirmed atomic w.r.t. offset. Two residuals raised by the checker were then also closed: the logfile test now asserts logPath is truthy instead of silently early-returning, so it can no longer pass vacuously." + }, + { + "id": "CR-002", + "status": "resolved", + "notes": "Independently verified: the false spinner-mitigation claim is gone, surviving only inside an explicitly labelled retraction that quotes it and states nothing in this MR mitigates AC4. The checker grepped both documents for any surviving sentence implying mitigation and found none. Two internal contradictions it flagged were then also fixed: Case D no longer claims signals were sent 'while parked on the prompt' (an assumption the pty work disproved), and the AC table no longer cites retracted Case C as evidence." + }, + { + "id": "CR-003", + "status": "resolved", + "notes": "Was partially-resolved. The checker confirmed all 11 agent entrypoints reach the AgentCLI constructor, closing the stated coverage gap, but measured that construction now mutated global process state - see CR-006. Re-fixed by installing per entrypoint instead. Note the checker's suggested alternative (move to AgentCLI.run()) was independently checked and rejected: tests drive run() as their seam, so it would not have helped." + }, + { + "id": "CR-004", + "status": "resolved", + "notes": "Independently verified as accurate against src/utils/interactive.ts and judged prominent rather than hedged. One residual overstatement the checker flagged upstream of the callout ('as in CI pipelines' implying CI-implies-no-TTY) has since been softened to 'as in most CI pipelines'." + }, + { + "id": "CR-005", + "status": "resolved", + "notes": "Was partially-resolved. The checker confirmed the timeout survives the options spread (CI no longer wedges) but empirically demonstrated that a hang still passed all four assertions - see CR-007. Re-fixed; independently reproduced before and after." + }, + { + "id": "CR-006", + "status": "resolved", + "notes": "NEW, introduced by the first fix-up. installProcessGuards() in the AgentCLI constructor made construction mutate global process state; the checker measured uncaughtException and unhandledRejection listener counts going 1->2 inside a Vitest worker, with four unit test files constructing AgentCLI directly. A later unhandled rejection would hard-exit the worker instead of surfacing as a test failure. Fixed by installing in each of the 11 agent entrypoints; AgentCLI reverted to zero references. Daemons deliberately excluded - exiting on first unhandled rejection is wrong for a long-running process, and codemie-mcp-proxy owns handlers with a survive-on-rejection policy. NOT independently re-verified." + }, + { + "id": "CR-007", + "status": "resolved", + "notes": "NEW, introduced by the first fix-up. The execSync timeout prevented CI wedging but left a hang undetectable: ETIMEDOUT reports status null, runSilent's `error.status || 1` collapsed it to 1, and stderr already carried the remediation printed before the block, so all four cases passed during a real hang - including the one named for it. Reproduced directly against a child that prints then hangs, confirming the old assertions pass and the new guard catches it. Fixed by adding timedOut to CommandResult and asserting it. NOT independently re-verified." + } + ], + "findings": [] +} diff --git a/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/code-review-final.json b/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/code-review-final.json new file mode 100644 index 000000000..81a60d626 --- /dev/null +++ b/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/code-review-final.json @@ -0,0 +1,116 @@ +{ + "decision": "request-changes", + "rationale": "All three lenses ran. The core two-part fix (getSdkClient wrap + auth.ts message preservation) is sound and independently verified end-to-end by the acceptance lens, and AC2 holds CLI-wide rather than only for the sdk family. Blocking issues are concentrated in the adjacent scope items rather than the core fix: the process-guards module destroys the diagnostic it promises to relocate (stack stringifies to [object Object]; the log entry can be dropped entirely by the exit race), it is wired into 1 of 14 bin entrypoints and not the agent binaries where the auth path actually lives, the AC1 regression test cannot fail on a hang because execSync receives no timeout, and the documentation overstates detection as stdin-TTY-only without naming the pty-allocating-CI blind spot that is the original reported failure. Separately, ac4-investigation.md contains one unearned mitigation claim that inspection refutes, and AC4 was never actually exercised (0 of 8 attempts reached the prompt), so it must be reclassified rather than closed as not-reproducible. 8 hypotheses were refuted with evidence and dropped; 6 minor findings deferred (spinner gates on stdin rather than the output stream, discarded re-auth error has no debug breadcrumb, defensive throw after handleSdkError, ConfigLoader.load sharing the 'authenticate' error label, brittle 'at ' assertion, reproduction.md's stale 'undocumented' wording). Confidence is low because AC4's disposition is a product decision, not an implementation one.", + "confidence": "low", + "risk_flags": ["auth", "observability", "acceptance-criteria-unmet"], + "business_review": [ + { + "id": "AC1", + "criterion": "Non-TTY stdin skips interactive prompt.", + "status": "pass", + "notes": "Verified against git, not just docs: PR #471 (5b2de4b7) introduced the guard and this MR does not touch the condition. Empirically confirmed at 1.4s, exit 1, no prompt. But see CR-005 - the new regression test cannot fail on a hang." + }, + { + "id": "AC2", + "criterion": "CLI exits non-zero with clear remediation.", + "status": "pass", + "notes": "Both halves implemented and both load-bearing. Independently reproduced: stderr carries exactly the actionable line, exit 1, no 'at ' frames, no ConfigurationError prefix, no Node banner. Acceptance lens confirmed the auth.ts fix also improves assistants/chat, assistants/setup and skills/setup, so AC2 holds CLI-wide." + }, + { + "id": "AC3", + "criterion": "Optional --non-interactive or --ci behavior is supported or documented.", + "status": "partial", + "notes": "Citing existing documentation is a legitimate reading of 'supported OR documented', and the pre-existing text was confirmed to predate this MR. Not a clean pass because the documentation asserts detection is 'based solely on whether stdin is a TTY' without naming the blind spot: a pty-allocating CI runner or docker run -t still reaches the prompt. See CR-006." + }, + { + "id": "AC4", + "criterion": "Killing during prompt does not produce readline lifecycle crash.", + "status": "fail", + "notes": "No code addresses it and no test asserts it. Decisively, the investigation's own table records 'Reached the re-auth prompt: 0' across all 8 pty attempts - the criterion was never exercised. 'Cannot reproduce the crash' and 'cannot construct the precondition' are different claims and only the second is supported. See CR-002." + } + ], + "standards_review": [ + { + "standard": "git-workflow.md - Conventional Commits, scope-enum", + "status": "pass", + "notes": "Both commits use fix(cli), which is in scope-enum. fix(auth)/fix(sdk) correctly avoided." + }, + { + "standard": "code-quality.md - ESLint --max-warnings=0", + "status": "pass", + "notes": "Full lint clean. no-useless-catch did not fire; both new catches transform rather than bare-rethrow." + }, + { + "standard": "code-quality.md - explicit return types, .js extensions, no console.log left in code", + "status": "pass", + "notes": "Typecheck clean. New module exports declare return types." + }, + { + "standard": "testing-patterns.md - unit tests co-located in src/**/__tests__/", + "status": "pass", + "notes": "All three new unit suites are under src/**/__tests__/, avoiding the tests/unit/ dead zone where five files match no vitest glob." + }, + { + "standard": "development-practices.md - do not log stack traces to console; log errors with logger.error", + "status": "fail", + "notes": "Console side is correct (no stack printed). The logger side is broken: the stack never reaches the log file. See CR-001." + } + ], + "findings": [ + { + "id": "CR-001", + "severity": "major", + "triage": "patch", + "file": "src/utils/process-guards.ts", + "line": "16-24", + "problem": "reportFatal calls logger.error(msg, { stack: payload.stack }) with a plain object. logger.error only extracts .stack when the second argument is instanceof Error, so it falls through to String(error) and writes '[object Object]'. Independently confirmed by both the edge-case and acceptance lenses against a real build. Compounding it, process.exit(1) fires before the logger's fs.WriteStream flushes its first write, so on a cold stream the entry is dropped entirely and the logs directory stays empty.", + "impact": "The guard removes the stack from stderr (intended) but then fails to put it anywhere else, so for the exact class of unexpected failure this module exists to report, diagnostic information is destroyed rather than relocated. The module's own header comment asserts the opposite. The new unit tests cannot catch this because they mock ../logger.js wholesale and assert only on stderr.", + "recommendation": "Pass the payload straight through: logger.error(`${kind}: ${message}`, payload) - logger already handles both Error and non-Error. For the flush race, use a synchronous write for the fatal entry or set process.exitCode and let the loop drain. Add one test that does not mock the logger and asserts the stack reaches the file.", + "sources": ["edge-case EC-2", "edge-case EC-3", "acceptance"] + }, + { + "id": "CR-002", + "severity": "major", + "triage": "decision_needed", + "file": "docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/ac4-investigation.md", + "line": "47", + "problem": "The document justifies shipping without an AC4 fix by claiming the spinner suppression 'removes one plausible contributor (a spinner writing to a torn-down TTY)'. That is provably false: the suppression is gated on isNonInteractiveEnvironment(), so it only takes effect when NO TTY is attached, while AC4's scenario requires a TTY by definition. The guard can never fire in it. The spinner implicated in the original escape-sequence observation is the one inside promptForReauth (sso.setup-steps.ts:294), which this MR does not touch.", + "impact": "This is the single piece of reasoning offered for why shipping without addressing AC4 is acceptable, and it does not survive inspection - it lends the closure a false appearance of partial mitigation. Separately, AC4 was never exercised at all: 0 of 8 pty attempts reached the prompt, so 'not reproducible' overstates what was established.", + "recommendation": "Strike the sentence from ac4-investigation.md and from the MR description. Reclassify AC4 as 'precondition not constructed, criterion not exercised' and split it to a follow-up ticket carrying the concrete recipe the acceptance lens supplied: plant SSO credentials whose apiUrl points at a closed local port so validateAuth fails deterministically with no network dependency, then drive tests/helpers/pty-session.ts and signal during the prompt. This is a product call on whether 14148 can close with AC4 unmet.", + "sources": ["acceptance"] + }, + { + "id": "CR-003", + "severity": "major", + "triage": "patch", + "file": "bin/codemie.js", + "line": "10-14", + "problem": "installProcessGuards() is wired into 1 of 14 bin/ entrypoints. agent-executor.js, codemie-claude.js, codemie-codex.js, codemie-gemini.js, codemie-kimi*.js, codemie-opencode.js, codemie-pi.js, codemie-openwiki.js, codemie-copilot.js, proxy-daemon.js are all unguarded - and per this branch's own technical-analysis.md, AgentCLI.handleRun is the entry point every agent binary uses and is where the SSO auth-failure path lives.", + "impact": "The commit message and docs imply the clean-failure behavior is universal. It is absent from precisely the binaries that carry the primary auth path, so behavior is now inconsistent across entrypoints.", + "recommendation": "Call installProcessGuards() from the shared AgentCLI constructor/run path so all agent binaries inherit it, or add the import to each bin/* entrypoint. Narrow the docs claim accordingly.", + "sources": ["edge-case EC-4", "blind"] + }, + { + "id": "CR-004", + "severity": "major", + "triage": "patch", + "file": "docs/AUTHENTICATION.md", + "line": "104-124", + "problem": "The page states non-interactive detection is 'based solely on whether stdin is a TTY' and presents that as sufficient for CI. interactive.ts:15 is literally !process.stdin.isTTY with no CI env-var fallback, so CI runners and containers that allocate a pty (docker run -t, several Jenkins/GitLab configurations) still reach the interactive prompt and hang - the precise failure this ticket was filed about. The doc offers no flag, env var, or troubleshooting note.", + "impact": "AC3 is being satisfied by documentation that overstates the guarantee. A CI user following this page can still hit the original hang. The limitation is inherited from PR #471, not introduced here, but this MR is the one asserting completeness.", + "recommendation": "Either honor process.env.CI in isNonInteractiveEnvironment() (a one-line change that would move AC3 from 'documented' to 'supported'), or amend the doc to state the boundary explicitly and point pty-CI users at JWT auth or pre-persisted credentials.", + "sources": ["acceptance", "blind"] + }, + { + "id": "CR-005", + "severity": "major", + "triage": "patch", + "file": "tests/integration/cli-commands/non-interactive-auth.test.ts", + "line": "31-45", + "problem": "The case named 'exits non-zero instead of hanging on a re-authentication prompt' asserts only exitCode !== 0. It routes through CLIRunner.runSilent, which is execSync-based (tests/helpers/cli-runner.ts:44) and receives no timeout option. Because execSync blocks the worker synchronously, Vitest's 30s testTimeout cannot interrupt it.", + "impact": "The AC1 disposition rests on 'already met, covered by regression test', so the safety net is nominal. A change reintroducing the hang would stall CI until the job-level timeout instead of failing with a pointer to the cause - the slowest and most confusing failure mode for exactly this bug.", + "recommendation": "Add timeout: 15_000 to the options passed to runSilent in runWithoutTty and assert on the resulting non-zero/SIGTERM outcome so a hang surfaces as a named test failure. Also tighten the remediation assertion to the exact message and assert stdout is empty, since the stderr-vs-stdout split is a headline doc claim with no test behind it.", + "sources": ["acceptance", "blind"] + } + ] +} diff --git a/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/complexity-assessment.json b/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/complexity-assessment.json new file mode 100644 index 000000000..4a3750c80 --- /dev/null +++ b/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/complexity-assessment.json @@ -0,0 +1,38 @@ +{ + "schema": 1, + "task": "Make the CLI's non-interactive SSO auth failure exit cleanly with actionable remediation by wrapping the shared getSdkClient() choke point and preserving the upstream \"run codemie setup\" message that promptReauthentication currently discards.", + "generated": "2026-09-03T00:00:00Z", + "dimensions": { + "component_scope": { "score": 4, "label": "L" }, + "requirements_clarity": { "score": 4, "label": "L" }, + "technical_risk": { "score": 4, "label": "L" }, + "file_change_estimate": { "score": 3, "label": "M" }, + "dependencies": { "score": 1, "label": "XS" }, + "affected_layers": { "score": 3, "label": "M" } + }, + "total": 19, + "size": "M", + "routing": "brainstorming", + "key_reasoning": [ + { + "dimension": "component_scope", + "reason": "Base M (2-3 components across 2 layers, clear pattern): the fix is two coordinated edits - getSdkClient() at src/cli/commands/sdk/utils/cli-utils.ts:15-18 (CLI) and the message-discarding throw at src/utils/auth.ts:76 (Utils), plus an optional one-line stdout->stderr change in src/providers/core/auth-validation.ts:39 and a mandatory docs/AUTHENTICATION.md:104-117 update. Bumped to L: src/utils/auth.ts is a core shared utility - getSdkClient is the sole bridge for ~50 sdk actions across 8 files, and getAuthenticatedClient/promptReauthentication have four further consumers (assistants/chat, assistants/setup, skills/setup, AgentCLI) that already exit cleanly and must not regress. The 'affects multiple workflows' red flag was considered but NOT applied on top: the shared-gate approach (PR #471 precedent) keeps the edit confined to one function, so the ~50 actions are consumers rather than edit sites." + }, + { + "dimension": "requirements_clarity", + "reason": "Base M (core requirement defined, 1-2 clarifying questions): AC2 - the only live criterion - is unambiguous, and technical-analysis.md has already resolved the implementation path (fix at the choke point + preserve the upstream message; both halves required, neither sufficient alone). Bumped to L: AC3 (--non-interactive/--ci) directly conflicts with a recorded decision - EPMCDME-13953's spec lists it as explicitly out of scope and docs/AUTHENTICATION.md:113 documents the absence as intentional - and its 'supported or documented' wording is arguably already satisfied. AC4 (ERR_USE_AFTER_CLOSE) was not reproduced in 2 attempts and is unproven rather than disproven. Both are scoping/product calls that need resolving before implementation, and the ticket's own premise about the missing try/catch was partly wrong and had to be corrected during research." + }, + { + "dimension": "technical_risk", + "reason": "Base M (some new patterns required, no exact precedent for the message-preservation half). Bumped to L by the 'affects authentication or authorization' red flag - this change sits directly on the SSO auth path. Concrete traps documented in the analysis: ConfigurationError's constructor accepts only `message`, so preserving the upstream error as `cause` means touching a repo-wide base class or a post-hoc assignment; ESLint no-useless-catch is warn under --max-warnings=0, so a naive catch-and-rethrow fails the lint gate; promptReauthentication's declared Promise can only return true or throw, so converting the throw to a `false` return would revive dead code at auth.ts:48/52 and change behaviour at assistants/chat/index.ts:423. Mitigation is strong: every ingredient exists in-repo (isNonInteractiveEnvironment, four never-returning error sinks, and NOT_AUTHENTICATED_MESSAGE in skills/lib/require-auth.ts:15-16 is verbatim the actionable text the ticket asks for), and the change is trivially reversible." + } + ], + "red_flags_applied": [ + "Component Scope bumped from M (3) to L (4): touches core shared utilities - src/utils/auth.ts and the getSdkClient() gate bridging ~50 sdk command actions across 8 files plus 4 adjacent auth call sites.", + "Requirements Clarity bumped from M (3) to L (4): vague/conflicting acceptance criteria - AC3 (--non-interactive/--ci) contradicts EPMCDME-13953's recorded out-of-scope decision and shipped text in docs/AUTHENTICATION.md:113; AC4 is unproven.", + "Technical Risk bumped from M (3) to L (4): affects authentication - the change modifies the SSO auth acquisition and failure-reporting path.", + "NOT applied - 'affects multiple workflows or agents' (second Component Scope bump): the shared-gate fix confines the edit to one function; the ~50 sdk actions inherit the fix as consumers rather than being edited.", + "NOT applied - 'requires data migration' (File Changes bump): no persistence, schema, or migration surface in this change." + ], + "split_recommendation": null +} diff --git a/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/events.jsonl b/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/events.jsonl new file mode 100644 index 000000000..c194a4077 --- /dev/null +++ b/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/events.jsonl @@ -0,0 +1,6 @@ +{"event":"work_item.adapter_warning","intent":"record_complexity_score","phase":2,"adapter":"codemie-jira-assistant","reason":"Adapter declared configured in .ai-run/guides/project.md but the codemie-jira-assistant skill is not resolvable in this session; no external sync performed.","external_sync":"pending"} +{"event":"lifecycle_emission","intent":"record_complexity_score","assessment_mode":"initial","status":"failed"} +{"event":"work_item.adapter_receipt","intent":"record_complexity_score","phase":2,"adapter":"codemie-jira-assistant","attempt":2,"status":"failed","reason":"Skill tool returned 'Unknown skill: codemie-jira-assistant'. Root cause: the skill is project-scoped at codemie-code/.claude/skills/codemie-jira-assistant, but the session project directory is the parent codemie-dev, so project skills from the repo are not loaded. Not a Jira connectivity problem - Jira MCP is connected and healthy. Deterministic MCP fallback (jira_add_comment) is available but is an outward-facing write and was not performed without explicit authorization.","external_sync":"pending"} +{"event":"lifecycle_emission","intent":"record_complexity_score","assessment_mode":"initial","status":"failed"} +{"event":"work_item.adapter_receipt","intent":"record_complexity_score","phase":8,"adapter":"codemie-jira-assistant","status":"failed","reason":"Same root cause as the initial assessment: adapter skill is project-scoped to codemie-code and unresolvable from the codemie-dev session project directory. Authorized MCP fallback was attempted for the initial score and blocked by the harness auto-mode classifier, so it was not retried here.","data":{"complexity_total":23,"complexity_size":"L","assessment_mode":"actual","delta_from_initial":4},"external_sync":"pending"} +{"event":"lifecycle_emission","intent":"record_complexity_score","assessment_mode":"actual","status":"failed"} diff --git a/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/qa-report.md b/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/qa-report.md new file mode 100644 index 000000000..1f053a942 --- /dev/null +++ b/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/qa-report.md @@ -0,0 +1,68 @@ +# QA Gate Report — EPMCDME-14148 + +**Branch**: `EPMCDME-14148` +**Merge base**: `origin/main` @ `d7097a2a` (rebased; originally cut from `1d5cc22b`) +**Runner**: npm (guide-first from `.ai-run/guides/quality-gates.md`) +**Started**: 2026-09-03T18:05:00Z +**Status**: PASSED (with two SKIPPED gates owed to CI — see below) + +> **Re-run after rebase.** `origin/main` advanced by one commit (`d7097a2a`, revert of the LiteLLM/SSO setup enforcement gate) while this work was in progress. The branch was rebased onto it and every gate below was re-run against the rebased tree. That commit also **renamed the test scripts** — `test:unit` and `test:integration` no longer exist in `package.json`; the commands below are the current equivalents. The unit count drops from 3969 to 3939 because the revert removed its own tests, not because anything here regressed. + +## Gates + +| Gate | Source | Status | Duration | Command | Notes | +|---|---|---|---|---|---| +| license | guide | PASS | 8s | `npm run license-check` | Required `npm_config_cache` override; `~/.npm/_cacache` is not writable in this environment | +| lint | guide | PASS | 2s | `npm run lint` | `--max-warnings=0`; `no-useless-catch` did not fire on either new catch | +| typecheck | guide | PASS | 2s | `npm run typecheck` | | +| build | guide | PASS | 2s | `npm run build` | | +| unit | guide | PASS | 5s | `npx vitest run --project unit` | **3939 passed / 3939**, 267 files, 0 failures (post-rebase) | +| integration | guide + ci | **PARTIAL** | — | `npx vitest run --project cli` | See below — pre-existing local hangs, not caused by this branch | +| commitlint | guide + ci | PASS | 0s | `npx commitlint --from origin/main --to HEAD --verbose` | 0 problems, 0 warnings across all 7 commits | +| secrets | hook | **SKIPPED** | 0s | `npm run validate:secrets` | Self-skipped — see below | +| affected | guide | N/A | — | — | No changed-file-aware command configured in this project | +| ui | guide | SKIPPED | — | — | No UI surface changed — green outcome | + +## Integration — partial, and why + +The `cli` vitest project cannot complete locally in this environment. Confirmed hanging files: `tests/integration/cli-commands/doctor.test.ts` and `tests/integration/cli-commands/list.test.ts`; the full-project run produced no output at all across two attempts (600s and ~10 min). + +**This is pre-existing and not caused by this branch.** Verified directly: `codemie doctor` was run against both the branch's `bin/codemie.js` and `origin/main`'s version inside the repo, and **both hang identically at 40s**. An earlier comparison that appeared to implicate this branch was invalid (main's `bin/codemie.js` had been copied outside the repo, so its `../dist/...` imports could not resolve and it died instantly on module-not-found rather than actually running). + +Files that do complete locally, all passing: + +| File | Result | +|---|---| +| `cli-commands/non-interactive-auth.test.ts` | 4 / 4 passed — the AC2 regression suite added by this change | +| `cli-commands/version.test.ts` | 2 / 2 passed | +| `cli-commands/help.test.ts` | 2 / 2 passed | +| `cli-commands/error-handling.test.ts` | 2 / 2 passed | + +The `version` / `help` / `error-handling` files are the ones that exercise `bin/codemie.js`, which this change modifies, so the entrypoint change is covered by the files that do run. The remainder is owed to CI, which runs `npm run test:integration` in a clean container. + +## Secrets — SKIPPED, not PASS + +The gate exited 0 in 0s. Per the self-skipping rule that is not a pass. Exact output: + +``` +No staged changes to scan +``` + +`scripts/validate-secrets.js` scans the **staged** git diff, and the working tree is clean because all work is committed — so it did no work. + +**Coverage is nonetheless real**: `.husky/pre-commit` chains `npm run validate:secrets`, so gitleaks ran against the staged diff of **every one of the 7 commits** on this branch during the session, reporting `no leaks found` each time. To run it here deliberately you would need staged changes present (and podman running, which it is). + +Worth flagging: this gate is **hook-only**. `npm run ci` is `license-check && lint && build && test:unit && test:integration` — it does **not** include `validate:secrets`, so CI will not re-run it. + +## Additional verification beyond the gate list + +- All 12 CLI entrypoints (`bin/codemie.js` + 11 agent binaries) execute `--version` successfully, exit 0 — confirming the new `process-guards` import resolves post-build in every one. +- End-to-end acceptance behaviour re-confirmed after every fix-up round: exit 1, single actionable stderr line, no stack trace. + +## Drift signal + +**no** — the implementation matches `spec.md`. Every symbol the spec names (`getSdkClient`, `handleSdkError`, `promptReauthentication`, `isNonInteractiveEnvironment`, `installProcessGuards`) exists with the described signature. The one deliberate deviation from the spec's first draft — installing the guards per bin entrypoint rather than from `AgentCLI` — was made in response to code-review finding CR-006 and is recorded in `code-review-check.json`. + +## Outcome + +`PASSED`. Nothing local blocks this branch. Two gates are owed to CI: the full integration suite (locally unrunnable, pre-existing) and the secrets scan (hook-only, already run per-commit). diff --git a/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/reproduction.md b/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/reproduction.md new file mode 100644 index 000000000..6336bcfca --- /dev/null +++ b/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/reproduction.md @@ -0,0 +1,115 @@ +# EPMCDME-14148 — Reproduction Report + +**Baseline:** `codemie-code` @ `1d5cc22b` (origin/main, v0.15.0), branch `EPMCDME-14148`, Node v24.19.0, macOS. +**Isolation:** every case ran with `CODEMIE_HOME` pointed at a throwaway directory. The real `~/.codemie` was never modified; its `credentials` store was verified intact afterwards. + +## Verdict + +The ticket describes two symptoms. Only one is still live. + +| Ticket symptom | Status on main | +|---|---| +| "CLI hangs on a re-authentication prompt" (non-TTY) | **Already fixed** by `5b2de4b7` (PR #471) | +| "exits non-zero with an actionable message" | **Still broken** — exits 1 via an *unhandled exception + raw stack trace*, and the message drops the actionable remediation | +| `ERR_USE_AFTER_CLOSE` on kill | **Not reproduced** — 12/12 interrupts at a genuinely-reached prompt, zero crashes (`ac4-investigation.md`). Case D below was inconclusive and is superseded | +| `--non-interactive` / `--ci` flag | **Does not exist** as a flag, but its absence *is* documented (`docs/AUTHENTICATION.md:113`, added by PR #471) — corrected; an earlier draft of this table wrongly called it undocumented | + +The residual defect is not a hang. It is that the non-interactive failure path terminates by letting a `ConfigurationError` escape to Node's default handler. + +## Cases + +### Case A — no config at all, stdin `< /dev/null` + +``` +CODEMIE_HOME= codemie sdk assistants list < /dev/null +``` + +Exit code **1**, duration **0s** (no hang). + +``` +- Loading configuration... +✖ No valid SSO credentials found +file:///.../dist/utils/auth.js:65 + throw new ConfigurationError('Authentication expired. Please re-authenticate.'); + ^ +ConfigurationError: Authentication expired. Please re-authenticate. + at promptReauthentication (.../dist/utils/auth.js:65:11) + at getAuthenticatedClient (.../dist/utils/auth.js:40:36) + at async Command. (.../dist/cli/commands/sdk/assistants.js:24:24) +Node.js v24.19.0 +``` + +### Case B — valid config present, no SSO credentials, stdin `< /dev/null` + +Matches the ticket's stated precondition more closely (config exists, session does not). Exit code **1**, duration **1s**. Output **identical** to Case A, same stack trace, same line. + +### Case C — attempted control: same command **with** a TTY (pty via `script`) + +Exit code **142** after the 25 s alarm fired. + +**This case was originally read as "blocks on the interactive re-auth prompt". That interpretation is wrong** — corrected after the AC4 investigation (see `ac4-investigation.md`): + +- Re-running the identical `script` harness against the **fixed** build still hangs for 25 s, so the hang is not the defect this ticket fixes. +- A control (`script` wrapping a child that exits immediately) returns in **0 s**, so `script` does not hang unconditionally. +- The captured output stalls at `⠋ Loading configuration...` — *before* credentials are ever read — and contains a stray `^D`. The re-auth prompt is never reached. +- The same command under a real pty (node-pty) exits in **under 1 s** with the correct message. + +Conclusion: the Case C hang is an artifact of the `script` harness (its stdin is an immediately-EOF pipe, which something downstream blocks on), not CLI behaviour. Case C is **not** valid evidence for or against the non-TTY guard. + +The guard is still confirmed working — but by `auth-validation.test.ts` and by Cases A/B exiting in 0–1 s without prompting, not by Case C. + +### Case D — AC4: kill during the prompt + +Ran under a pty and sent `SIGINT` then `SIGTERM`. **No `ERR_USE_AFTER_CLOSE` and no readline lifecycle error** appeared across two attempts. + +**Corrected:** an earlier draft described these signals as being sent "while parked on the prompt". That was an assumption, not an observation, and the later node-pty work disproved it — the process exits before any signal is delivered and the prompt is never reached. See `ac4-investigation.md`; the criterion was never exercised. + +Incidental observation, **not** confirmed as a product bug: after the interrupt the `ora` spinner emitted a runaway stream of cursor-control escapes (`ESC[1A ESC[0K`), producing a 73 MB capture. This is most likely an artifact of `script` giving the child a pty with no usable `stdout.columns`, which breaks ora's line-clearing arithmetic. It should be re-checked in a real terminal before anyone treats it as a finding. + +The ticket hedges with "**can** crash", so AC4 is plausibly intermittent or environment-specific. It is unproven here, not disproven. + +## Root cause of the live defect + +`src/utils/auth.ts`: + +``` +getAuthenticatedClient(config) // line 22 + └─ getCodemieClient() // line 44 + └─ throws ConfigurationError + 'SSO authentication required. Please run "codemie setup" with SSO provider first.' + └─ catch (line 45): message matches 'SSO authentication required' + └─ promptReauthentication(config) // line 47 + └─ handleAuthValidationFailure(...) // line 68 + └─ isNonInteractiveEnvironment() === true + → prompt correctly skipped, returns false ← PR #471 working + └─ falls through to line 76 + └─ throw new ConfigurationError('Authentication expired. Please re-authenticate.') + ← UNCAUGHT +``` + +Two distinct problems on that last hop: + +1. **The throw escapes the command's error handler.** *(Corrected after technical analysis — my first reading of this was wrong.)* `assistants.ts` **does** import and use `handleSdkError` in all six actions. The defect is **statement ordering**: `const client = await getSdkClient();` sits at line 65, *outside and above* the `try {` at line 68, so the try/catch only guards the post-auth API call. The auth throw sails past it to Node's default handler, which prints the raw stack and exits 1. `bin/codemie.js` installs no `uncaughtException` / `unhandledRejection` handler as a backstop. AC "exits non-zero with clear remediation" fails on *clear*, not on *non-zero*. + +2. **The actionable text is discarded.** `getCodemieClient` produced the remediation the AC asks for — *"Please run `codemie setup` with SSO provider first"*. `promptReauthentication` throws a **new**, vaguer error at line 76 that overwrites it. The useful string is generated and then thrown away one frame later. + +`handleAuthValidationFailure` and `isNonInteractiveEnvironment` are working exactly as designed; the gap is in what happens *after* they correctly decline to prompt. + +## Blast radius + +**Corrected after technical analysis.** My initial claim that `src/agents/core/AgentCLI.ts` and `src/cli/commands/profile/index.ts` were affected was **wrong** — those call `handleAuthValidationFailure` *directly*, not `getAuthenticatedClient`/`promptReauthentication`, and they already exit cleanly. The uncaught-throw path is narrower than this report first stated. + +The actual blast radius is wider in a different direction: the same outside-the-try ordering repeats in **all 8 `src/cli/commands/sdk/*.ts` files, ~50 command actions**. A grep for `try {` within two lines above any `await getSdkClient()` returns zero matches. + +There is, however, a **single choke point**: `getSdkClient()` at `src/cli/commands/sdk/utils/cli-utils.ts:15-18` is the sole bridge from all ~50 sdk actions into `src/utils/auth.ts`. Fixing there — plus preserving the message at `auth.ts:76` — follows the shared-gate precedent PR #471 set, instead of 50 mechanical edits. + +Note that `handleSdkError`'s `else` branch **already** renders `ConfigurationError` cleanly. So wrapping alone removes the stack trace but still prints the vague message: **both halves must be fixed** to satisfy the AC. + +## Acceptance criteria mapped to evidence + +| AC | Verdict | Evidence | +|---|---|---| +| Non-TTY stdin skips interactive prompt | **Met already** | Cases A/B exit in 0–1 s without prompting, plus the `auth-validation.test.ts` guard tests. **Not** Case C, which this document retracts as a `script(1)` artifact | +| CLI exits non-zero with clear remediation | **Not met** | Case A/B: exit 1 but raw stack trace; message lacks `codemie setup` | +| `--non-interactive` / `--ci` supported or documented | **Partially met** | No such flag in `src/`, but the absence is documented at `docs/AUTHENTICATION.md:113`. The AC reads "supported **or** documented". Not a clean pass: the documented mechanism (`stdin` TTY only) has a blind spot for pty-allocating CI — now stated explicitly in that doc | +| Kill during prompt produces no readline crash | **Met** | Later verified properly: 12/12 runs reached the prompt under a real pty and none crashed. The earlier "never reached" reading was an artifact of `CODEMIE_*` env vars injected by the parent CodeMie session — see `ac4-investigation.md` | diff --git a/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/spec.md b/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/spec.md new file mode 100644 index 000000000..53f54f827 --- /dev/null +++ b/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/spec.md @@ -0,0 +1,76 @@ +# EPMCDME-14148 — Spec + +Approved design, bounded path. Grounding: `reproduction.md` (empirical, 4 cases), `technical-analysis.md` (407 lines), `complexity-assessment.json` (M, 19/36). + +## Problem + +In a non-TTY session with no valid SSO credentials, `codemie sdk ` exits 1 by letting a `ConfigurationError` escape to Node's default handler — printing a raw stack trace — and the message it prints has lost the actionable remediation. + +The non-TTY *hang* named in the ticket title is **already fixed** on main by `5b2de4b7` (PR #471). Case C in `reproduction.md` (same command with a TTY blocks; without a TTY it does not) is the control proving that guard is live. What remains is the *quality of the failure*, not the hang. + +## Root cause + +``` +getCodemieClient() sdk-client.ts:73-75 + throws ConfigurationError('SSO authentication required. Please run "codemie setup"...') ← actionable +getAuthenticatedClient() catch auth.ts:45 + → promptReauthentication() auth.ts:47 + → handleAuthValidationFailure() auth-validation.ts:34 → non-TTY: skips prompt, returns false ✅ PR #471 + → throw ConfigurationError('Authentication expired. Please re-authenticate.') auth.ts:76 ← shadows the actionable message +getSdkClient() cli-utils.ts:15-18 → no try/catch → escapes to Node +``` + +Two independent defects. Fixing either alone is insufficient: wrapping alone still prints the vague message (`handleSdkError`'s `else` branch already renders `ConfigurationError` cleanly); message-fixing alone still prints a stack trace. + +## Scope + +**In scope** + +1. `sdk/utils/cli-utils.ts` — wrap `getSdkClient()` so auth failures reach the existing `handleSdkError` sink. One edit covers ~50 actions across 8 files (shared-gate precedent from PR #471). +2. `utils/auth.ts` — preserve the actionable upstream error rather than letting `promptReauthentication`'s throw shadow it. +3. `providers/core/auth-validation.ts:39` — diagnostic to **stderr**, not stdout (currently pollutes piped output and `--json`). +4. `utils/sdk-client.ts:26` — no `ora` spinner when non-interactive. +5. `bin/codemie.js` — process-level `unhandledRejection` / `uncaughtException` net (template: `bin/codemie-mcp-proxy.js:75-82`). +6. `docs/AUTHENTICATION.md:104-117` — align documented promise with actual emitted message. **Non-optional**: PR #471 set the precedent, and EPMCDME-13953's CR-001 was closed specifically on this doc surface. +7. Timeboxed AC4 investigation in a real terminal. + +**Out of scope** + +- Any `--non-interactive` / `--ci` flag. AC3 is worded "supported **or** documented"; `docs/AUTHENTICATION.md:113` already documents the absence as intentional, and EPMCDME-13953's spec lists the flag as an explicit out-of-scope decision. Cite, do not implement. +- The other 45 unguarded `inquirer.prompt` sites across 19 files — same class of latent bug, their own ticket. +- `profile/index.ts:138-140` exiting 0 on declined re-auth — a real inconsistency, but not this defect. +- Reformatting `cli-utils.ts` double quotes (ESLint does not enforce quote style; would balloon the diff). + +## Design decisions + +**Why wrap at `getSdkClient()` rather than move `await getSdkClient()` inside each action's existing `try`.** The latter is ~50 mechanical edits across 8 files with 50 chances to miss one; a grep confirms all 50 are currently outside their `try`. The gate is one function and matches how PR #471 fixed the sibling defect. + +**Why preserve the original error rather than add `cause` or change `promptReauthentication`'s throw.** Three constraints make this the cheapest correct option: + +- `ConfigurationError`'s constructor takes only `message` — `cause` would mean touching a repo-wide base class. +- `promptReauthentication`'s declared `Promise` is unreachable-`false`; converting its throw to a return would revive dead code and change `assistants/chat/index.ts:423`. +- `auth.test.ts:142-155` asserts that throw's message verbatim. Leaving `promptReauthentication` untouched means **that test keeps passing unmodified** — only the `getAuthenticatedClient` reauth-fails case changes, deliberately. + +`no-useless-catch` is an ESLint *warn* under `--max-warnings=0`. Both new catches transform rather than bare-rethrow, so neither trips it. + +**Layering.** `utils/` throws; the CLI layer formats and exits (`architecture.md:159-171`). The fix keeps user-facing formatting in `cli-utils.ts` and does not deepen `auth.ts`'s existing chalk-output smell. + +## Acceptance + +| AC | Disposition | +|---|---| +| Non-TTY stdin skips interactive prompt | Already met (PR #471); covered by regression test | +| CLI exits non-zero with clear remediation | **The fix.** Exit 1, no stack trace, message names `codemie setup` | +| `--non-interactive` / `--ci` supported or documented | Met by existing `AUTHENTICATION.md:113`; cite in MR | +| Kill during prompt → no readline crash | Timeboxed investigation; report "cannot reproduce" if it stays quiet | + +## Testing + +Unit tests live in `src/**/__tests__/` — **never** `tests/unit/`, where five files match no vitest project glob and silently never execute. + +- **New** `src/cli/commands/sdk/utils/__tests__/cli-utils.test.ts` (greenfield — `sdk/**` has zero tests across 22 files). Pattern from `skills/__tests__/commands.test.ts`: `process.exit` spy that records the code then throws. +- **Updated** `src/utils/__tests__/auth.test.ts` — the `getAuthenticatedClient` reauth-fails case now asserts the preserved actionable message. +- **Non-interactivity is injected at the seam** (`vi.mock` the function), not via `process.stdin.isTTY` — per the `auth-validation.test.ts` template. +- **Integration** via `tests/helpers/cli-runner.ts` `runSilent()` with stdin ignored: assert exit ≠ 0, remediation text present, and **no stack trace** in output. + +Commit scope must be `fix(cli)` or `fix(utils)` — commitlint's `scope-enum` rejects `fix(auth)` and `fix(sdk)`. diff --git a/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/technical-analysis.md b/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/technical-analysis.md new file mode 100644 index 000000000..eabd487af --- /dev/null +++ b/docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/technical-analysis.md @@ -0,0 +1,407 @@ +# Technical Research + +**Task**: cli auth sso non-interactive error-handling (EPMCDME-14148) +**Generated**: 2026-09-03 +**Research path**: filesystem (codegraph MCP not available in this environment) + +--- + +## 1. Original Context + +EPMCDME-14148 (Bug, Major) — "CLI non-interactive SSO failure hangs on prompt and can crash with readline error" + +Repository: `/Users/Evgenii_Kurdakov/Desktop/projects/codemie-dev/codemie-code` (the `codemie` CLI, `@codemieai/code`). Branch EPMCDME-14148, based on origin/main @ 1d5cc22b (v0.15.0). + +Ticket description verbatim: + +``` +## Summary +CLI non-interactive SSO failure hangs on prompt and can crash with readline error. +## Description +When SSO credentials are missing in a non-TTY session, CLI prompts for re-authentication and may crash with ERR_USE_AFTER_CLOSE. Non-interactive automation should fail cleanly with actionable remediation. +## Preconditions +- CLI has no valid SSO session. +- Standard input is non-interactive. +## Steps to Reproduce +1. Ensure there is no valid CLI SSO session. +2. Run any `codemie sdk ...` command with stdin redirected from /dev/null. +3. Observe authentication prompt behavior and process termination. +## Expected Result +CLI exits non-zero with an actionable message such as "run codemie setup". +## Actual Result +CLI hangs on a re-authentication prompt and can crash with ERR_USE_AFTER_CLOSE if killed. +## Acceptance Criteria +- Non-TTY stdin skips interactive prompt. +- CLI exits non-zero with clear remediation. +- Optional --non-interactive or --ci behavior is supported or documented. +- Killing during prompt does not produce readline lifecycle crash. +``` + +Reproduction has already been performed; full evidence at `docs/superpowers/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/reproduction.md`. Confirmed inputs to this research: + +- The non-TTY hang is ALREADY FIXED on main by commit `5b2de4b7` (PR #471), via `src/utils/interactive.ts` `isNonInteractiveEnvironment()` consumed by `src/providers/core/auth-validation.ts` `handleAuthValidationFailure()`. +- The RESIDUAL live defect: `src/utils/auth.ts` `promptReauthentication()` line 76 throws `ConfigurationError('Authentication expired. Please re-authenticate.')` uncaught — Node prints a raw stack trace and exits 1. Two problems: (a) no error boundary between the throw and Node's default handler; (b) the actionable remediation string produced upstream by `getCodemieClient` is discarded. +- Shared blast radius beyond the sdk commands. +- No `--non-interactive` or `--ci` flag exists anywhere in `src/`. +- ERR_USE_AFTER_CLOSE was NOT reproduced in 2 attempts. + +--- + +## 2. Codebase Findings + +### 2.0 Correction to the ticket premise — READ FIRST + +The reproduction note states there is "no try/catch at the command action layer (`src/cli/commands/sdk/assistants.ts`)". **That is not accurate, and the distinction changes the shape of the fix.** + +`src/cli/commands/sdk/assistants.ts` **does** import and use `handleSdkError` in all 6 of its actions. The real defect is **statement ordering**: + +- `src/cli/commands/sdk/assistants.ts:65` — `const client = await getSdkClient();` sits **outside and above** the `try {` at line 68. +- The `try/catch` therefore guards only the *post-auth* API call. Auth acquisition itself is unguarded, so the `ConfigurationError` escapes the action, escapes commander, and reaches Node's default handler. +- **This is systemic.** All 8 SDK command files place `await getSdkClient()` before their `try {`. A grep for `try {` within two lines above any `await getSdkClient()` returns **zero** matches across the tree. + +**Blast radius: ~50 SDK command actions across 8 files, every one unguarded at the auth step.** + +### 2.1 Failure chain (verified end to end) + +1. `src/utils/sdk-client.ts:73-75` throws `ConfigurationError('SSO authentication required. Please run "codemie setup" with SSO provider first.')` — **the actionable message**. +2. `src/utils/auth.ts:46` catches it, matches `error.message.includes('SSO authentication required')` (brittle string coupling), calls `promptReauthentication(config)`. +3. `src/utils/auth.ts:67` — SSO `validateAuth` returns `{ valid: false, ... }`. +4. `src/utils/auth.ts:68` — `handleAuthValidationFailure` → `src/providers/core/auth-validation.ts:34` guard `setupSteps?.promptForReauth && !isNonInteractiveEnvironment()` is **false** in non-TTY → prompt correctly skipped → prints `chalk.red("\n✗ \n")` via `console.log` (**stdout**, line 39) → returns `false`. +5. `src/utils/auth.ts:76` throws `ConfigurationError('Authentication expired. Please re-authenticate.')` — **the step-1 actionable message is discarded**: never re-thrown, never attached as `cause`. +6. Nothing catches it → raw stack trace, exit 1. + +The `validationResult.error` printed at step 4 contains genuinely useful text produced by `src/providers/plugins/sso/sso.setup-steps.ts:236-239` (`No SSO credentials found for . Please run: codemie profile login --url `) — but that string is only *printed*, never propagated into the exception. That is the second half of the "message discarded" complaint. + +### 2.2 Existing Implementations + +**Auth core** +- `src/utils/auth.ts` (77 lines) — two exports, no non-interactive awareness of its own. + - `getAuthenticatedClient(config): Promise` L22-54. JWT branch L23-41 (throws at L26-29, L32-34; no prompting). SSO branch L43-53: `try { return await getCodemieClient(); } catch { ... }`. + - `promptReauthentication(config): Promise` L63-77. **L76 is the uncaught throw.** Control-flow note: this function can only return `true` or throw — the declared `Promise` is misleading and the `false` branch at `auth.ts:48` is dead code. Confirmed by `src/utils/__tests__/auth.test.ts:142-176`. + - JSDoc at L61 already documents `@throws ConfigurationError if re-authentication is not available`. + - L71 does `console.log(chalk.green(...))` — Utils layer doing user output, an existing layering smell. +- `src/utils/sdk-client.ts` `getCodemieClient(quiet = false)` L23-110. Throws at L37-39, **L73-75 (the discarded actionable message)**, L106-108. Starts an `ora` spinner at L26 unless `quiet`; `getAuthenticatedClient` always calls it non-quiet, so a spinner is spawned even in non-TTY. +- `src/providers/core/auth-validation.ts` (41 lines) — sole export `handleAuthValidationFailure`. Guard at L34. Fallback L39: `console.log(chalk.red(...)); return false;`. +- `src/utils/interactive.ts` (16 lines) — sole export `isNonInteractiveEnvironment(): boolean { return !process.stdin.isTTY; }` (L15). Consults **stdin only** — not stdout, not `process.env.CI`, not `TERM`. + +**SDK command layer** +- `src/cli/commands/sdk/utils/cli-utils.ts` (145 lines): + - `getSdkClient(): Promise` L15-18 — `ConfigLoader.load()` → `getAuthenticatedClient(config)`. **No try/catch.** Sole bridge from the SDK CLI to `src/utils/auth.ts`; single choke point for all ~50 actions. + - `handleSdkError(error: unknown, operation: string): never` L88-119 — extracts `error.message` (else `String(error)`); `logger.error("SDK operation failed", ...sanitizeLogArgs({operation, error: msg}))`; branches on `ApiError` status 401/403 (prints `Run "codemie setup" to re-authenticate if your session expired.` at L102), 404, `ZodError`; **else branch L116** prints `chalk.red("❌ " + msg)`; always writes to **stderr**; ends `process.exit(1)` L118. + - Consequence: a `ConfigurationError` already falls into the `else` branch and would exit 1 cleanly with a red message. **Moving `getSdkClient()` inside the existing `try` fixes the stack trace — but the message would still be the useless one.** Both halves must be fixed. + - Siblings in the same file: `parseDataInput` L23-35, `parseJsonFileInput` L40-49, `parseDataOrJsonFile` L55-76, `outputJson` L81-83, `getResponseMessage` L124-129, `parseConfigInput` L134-145. + +**handleSdkError adoption audit** — `path` — uses handleSdkError — try/catch in action — notes: +- `src/cli/commands/sdk/assistants.ts` — yes (import L23; L106,151,183,212,228,280) — **partial** — `getSdkClient()` at L65,115,166,198,220,240 all precede `try {` at L68,118,169,200,222,243. 6/6 actions unguarded for auth. +- `src/cli/commands/sdk/categories.ts` — yes (L22) — partial — 5/5 unguarded. +- `src/cli/commands/sdk/datasources.ts` — yes (L10) — partial — 7/7 unguarded. +- `src/cli/commands/sdk/integrations.ts` — yes (L22) — partial — 6/6 unguarded. +- `src/cli/commands/sdk/llm.ts` — yes (L9) — partial — 1/1 unguarded (`getSdkClient()` L39 vs `try` L43). +- `src/cli/commands/sdk/skills.ts` — yes (L38, 18 call sites) — partial — 18/18 unguarded. +- `src/cli/commands/sdk/users.ts` — yes (L5) — partial — 2/2 unguarded. +- `src/cli/commands/sdk/workflows.ts` — yes (L21) — partial — 5/5 unguarded. +- `src/cli/commands/sdk/index.ts` — no — n/a — pure `Command` composition. +- `src/cli/commands/sdk/utils/{render,datasource-types,file-utils}.ts` — no — n/a. +- `src/cli/commands/sdk/services/*.ts` (9 files) — no — n/a — thin SDK pass-through; deliberately lets errors propagate to the command layer. + +### 2.3 Call-site census + +**`getCodemieClient`** (def `src/utils/sdk-client.ts:23`): +- `src/utils/auth.ts:44` — in `getAuthenticatedClient` — try/catch **yes** (L43-53); catch string-matches then calls `promptReauthentication`, else rethrows original at L52. +- `src/utils/auth.ts:49` — inside that catch, after successful re-auth — try/catch **no**; a second failure propagates raw. +- `src/cli/commands/skills/setup/sync.ts:36` — try/catch **yes**; catch is `logger.debug` only, fully swallowed by design. + +**`getAuthenticatedClient`** (def `src/utils/auth.ts:22`): +- `src/cli/commands/sdk/utils/cli-utils.ts:17` — in `getSdkClient` — try/catch **no**. Hot path for all ~50 SDK actions. **Primary fix location.** +- `src/cli/commands/assistants/chat/index.ts:91` — no try/catch at the line, but the commander action at L47-63 wraps it: catch L58-62 does `createErrorContext` → `logger.error` → `console.error(formatErrorForUser(context))` → `process.exit(1)`. **Clean exit, no stack trace.** +- `src/cli/commands/assistants/setup/index.ts:68` — guarded at the caller; action L47-57 → `handleSetupError(error, 'setup assistants')` L55. +- `src/cli/commands/skills/setup/index.ts:107` — guarded at the caller → `handleSetupError(error, 'setup skills')` L32. + +**`promptReauthentication`** (def `src/utils/auth.ts:63`): +- `src/utils/auth.ts:47` — in the catch of `getAuthenticatedClient` — **no** guard; its L76 throw escapes and shadows the original error. +- `src/cli/commands/assistants/chat/index.ts:423` — in `handleChatError`, reached from L265/L314 inside existing catches, all under the action-level boundary at L56-62. Only invoked when `error.message.includes('401'|'403')` (L422). + +**Correction on blast radius.** `src/agents/core/AgentCLI.ts` and `src/cli/commands/profile/index.ts` are **not** call sites of the three symbols. They call `handleAuthValidationFailure` directly: +- `src/agents/core/AgentCLI.ts:293-294` and `:323-324` (dynamic imports). Both inside try/catch. On `reauthed === false` (L296-299): `console.log(chalk.yellow('\n⚠️ Authentication required\n'))` + `process.exit(1)` — **already a clean non-zero exit**. +- `src/cli/commands/profile/index.ts:130` — inside try L123 / catch L143-145 (`logger.error` only, execution continues). On `reauthed === false` (L138-140): prints a yellow warning and `return`s — **exit code 0**. An inconsistency worth noting, arguably out of scope. + +So the uncaught-throw defect is confined to the `getAuthenticatedClient` → `promptReauthentication` path, whose only unguarded consumer is `getSdkClient`. + +### 2.4 Architecture and Layers Affected + +Project layer taxonomy (`.ai-run/guides/architecture/architecture.md` L65-88): **CLI (`src/cli/`) → Registry → Plugin (`src/*/plugins/`) → Core (`src/*/core/`) → Utils (`src/utils/`)**. Never skip layers, never reverse direction (L147-155). + +Documented error flow (L159-171), directly load-bearing here: + +``` +Plugin Error (throws) → Registry (catches, adds context) → re-throws → CLI (catches, formats for user) +``` + +⇒ **Formatting for the user is the CLI layer's job, not `src/utils/auth.ts`'s.** `auth.ts` already violates this (chalk output at L71) and the fix should not deepen it. + +Layers touched by a fix: +- **CLI** — `src/cli/commands/sdk/utils/cli-utils.ts` (error sink + auth gate); optionally the 8 sdk command files; optionally `bin/codemie.js` (process-level net). +- **Utils** — `src/utils/auth.ts` (message preservation); possibly `src/utils/errors.ts` if `cause` support is added. +- **Core** — `src/providers/core/auth-validation.ts` (stdout→stderr only, cosmetic). + +### 2.5 Top-level error handling — there is none + +- `bin/codemie.js` (41 lines): try/catch around `MigrationRunner` L13-22 (non-fatal warning); try/catch around `checkAndPromptForUpdate()` L28-34 (swallowed); `import('../dist/cli/index.js').catch(err => { console.error('Error:', err.message); process.exit(1); })` L37-40. **That `.catch` only covers module-load/top-level-await rejections.** `program.parse()` is synchronous and returns immediately; commander async action rejections never reach this chain. **No `process.on('uncaughtException')`, no `process.on('unhandledRejection')`.** +- `src/cli/index.ts` (148 lines): `new Command()` L44; `program.parse(process.argv)` L147 — plain **sync** `parse`, not `parseAsync`, not awaited. **No `.exitOverride()`, no `program.error()`, no `.configureOutput()`, no `.showHelpAfterError()`.** +- Whole-tree grep: `uncaughtException` and `unhandledRejection` each have exactly **one** hit, both in `bin/codemie-mcp-proxy.js:75` and `:79` — a ready-made in-repo template if a process-level net is wanted. `exitOverride` appears only in two test files. `process.exit` appears at 162 non-test sites across ~29 command files. + +**Conclusion: no centralised CLI error boundary exists.** Anything escaping a commander action prints a raw stack. + +### 2.6 Established clean-exit patterns (exemplars a fix should imitate) + +Four competing conventions exist. Ranked by fit: + +**(a) `handleSdkError` — the in-domain convention.** `src/cli/commands/sdk/utils/cli-utils.ts:88-119`. Already imported by all 8 SDK files. `logger.error` + `sanitizeLogArgs` → `chalk.red('❌ …')` to stderr → `process.exit(1)`. + +**(b) `requireAuthenticatedSession` / `failAuth` — the closest semantic analogue.** `src/cli/commands/skills/lib/require-auth.ts:24-51`: +```ts +function failAuth(message: string): never { + console.error(chalk.red(`\n${message}\n`)); + process.exit(1); +} +``` +with `NOT_AUTHENTICATED_MESSAGE` (L15-16) = `'CodeMie SSO authentication required. Run "codemie setup" or "codemie profile login" first.'` — **precisely the actionable text EPMCDME-14148 says is being discarded.** Used as the first statement of 5 skills actions: `find.ts:48`, `list.ts:30`, `add.ts:49`, `update.ts:32`, `remove.ts:41`. Note L41-44: it treats a *thrown* auth check as unauthenticated rather than letting it escape — exactly the defensive posture missing from `getSdkClient`. **This path already never prompts and already exits non-zero cleanly.** + +**(c) `printProxyError` — the `ConfigurationError`-aware formatter.** `src/cli/commands/proxy/connect-orchestrator.ts:250-262`: known project error → terse one-line `chalk.red('✗ …')`; unknown error → `formatErrorForUser(context, { showSystem: false })`; then `process.exit(1)`. Used at `connect-orchestrator.ts:673` and `proxy/index.ts:173`. **Best model for the discrimination this fix needs.** + +**(d) `handleSetupError` — the guide-canonical generic boundary.** `src/cli/commands/shared/helpers.ts:64-69`: `createErrorContext` → `logger.error` → `console.error(formatErrorForUser(context))` → `process.exit(1)`. Used at `assistants/setup/index.ts:55` and `skills/setup/index.ts:32`; the same shape is inlined at `assistants/chat/index.ts:58-62`. Caveat: `formatErrorForUser` defaults `showSystem: true`, printing an OS/Node/version block — verbose for a simple "you're not logged in". + +### 2.7 Error class hierarchy — `src/utils/errors.ts` (566 lines) + +Flat, single base, all `constructor(message: string)`. **No `code`, no `exitCode`, no `isOperational` on the base.** + +| Class | Line | Constructor | +|---|---|---| +| `CodeMieError extends Error` | L1-6 | `(message)`, sets `this.name` | +| `ConfigurationError extends CodeMieError` | L8-13 | `(message)` | +| `AgentNotFoundError` | L15-20 | `(agentName)` | +| `AgentInstallationError` | L22-27 | `(agentName, reason)` | +| `ToolExecutionError` | L29-34 | `(toolName, reason)` | +| `PathSecurityError` | L36-41 | `(path, reason)` | +| `AnalyticsSourceError` | L43-48 | `(message)` | +| `NpmError extends CodeMieError` | L64-73 | `(message, code: NpmErrorCode, originalError?)` — **only subclass with a `code`** | + +**No constructor accepts an `options`/`cause` argument.** Preserving the upstream error via `{ cause }` therefore requires a constructor change or a post-hoc `.cause` assignment. + +Exported helpers: `parseNpmError` L81, `getErrorMessage` L133, `createErrorContext` L302, `formatErrorForUser` L358 (plain string, **no chalk**; `❌ ` wrapped at 97 chars + System Information block, `showSystem` defaults **true**), `formatErrorForLog` L424, `getErrorExplanation` L434, `formatErrorWithExplanation` L529. **Caution: `getErrorExplanation`'s generic fallback (L512-519) says "Metrics collection encountered an issue"** — metrics-specific and wrong for auth errors, so avoid `formatErrorWithExplanation` here. + +`instanceof` checks against project error classes exist at only **5 non-test sites**: `src/utils/auth.ts:46`, `proxy/connect-orchestrator.ts:254`, `proxy/connectors/vscode.ts:197`, `proxy/connectors/desktop.ts:155`, `proxy/connectors/vscode-claude-code.ts:68`. + +### 2.8 Integration Points + +- `codemie-sdk` (`CodeMieClient`, `ApiError`) — constructed in `src/utils/sdk-client.ts` and `src/utils/auth.ts:36-40`; consumed by all sdk services. +- `ProviderRegistry` (`src/providers/core/registry.ts`) → `getSetupSteps` → SSO plugin `src/providers/plugins/sso/sso.setup-steps.ts` (`validateAuth` L~230, `promptForReauth` L268 with an `inquirer.prompt` confirm at L274-281). Interface declared at `src/providers/core/types.ts:390`. +- `SecureStorage` (`src/utils/security.ts`) — OS keychain via lazily-imported `keytar`, AES-256-CBC machine-keyed file fallback. `FALLBACK_FILE = getCodemiePath('sso-credentials.enc')` L258, `CREDENTIALS_DIR = getCodemiePath('credentials')` L259; per-URL `credentials/.enc` and `credentials/jwt-.enc`. +- `ConfigLoader` (`src/utils/config.ts`) — global `~/.codemie/codemie-cli.config.json`, project-local `.codemie/codemie-cli.config.json`, precedence at L113-117. +- `logger` + `sanitizeLogArgs` — used by `handleSdkError`. +- `chalk`, `ora`, `inquirer`, `commander` — presentation and prompting. + +### 2.9 Patterns and Conventions + +- **Shared-gate fix pattern** (established by PR #471): fix once at the single shared gate, not per-caller. +- `never`-returning error sinks that own `process.exit(1)` — `handleSdkError`, `failAuth`, `printProxyError`, `handleSetupError`. +- `.js` extensions on all relative imports; `@/` alias preferred over deep `../../..` (AGENTS.md pitfalls table). Note the inconsistency: `src/utils/auth.ts` uses `@/`, `src/providers/core/auth-validation.ts` uses relative `../../utils/`. +- Explicit return types on all exported functions; `interface` over `type`. +- `src/cli/commands/sdk/utils/cli-utils.ts` uses **double quotes** throughout, contradicting the single-quote guidance — ESLint does not enforce quote style, so **do not mass-reformat**. + +--- + +## 3. Documentation Findings + +### Guides and Architecture Docs + +All present under `.ai-run/guides/`. + +**`development/development-practices.md`** — Error Handling L13-76. Exception hierarchy table L17-25. Canonical pattern L29-43 (citing `src/cli/commands/execute.ts:30-38`): `createErrorContext` → `logger.error` → `console.error(formatErrorForUser(context))`. Rules L45-51 verbatim: +- ✅ Use specific error classes (not bare `Error`) +- ✅ Always add context via `createErrorContext()` +- ✅ Log errors with `logger.error()` +- ✅ Format errors for user with `formatErrorForUser()` +- ❌ Expose internal implementation details +- ❌ Log stack traces to console (use `logger.debug()`) + +Defensive-null precedent L53-76 (return instead of throw when the caller can degrade). Logging L80-130: WARN/ERROR/SUCCESS go to console and file; always `sanitizeLogArgs()`; never log tokens. TypeScript rules L194-203. **No prompt/UX section, and no explicit exit-code rule** — exit codes are conventional (`process.exit(1)`) only. + +**`standards/code-quality.md`** — explicit return types on exports (L35); `.js` extensions and `import type` (L49); ESLint: `no-explicit-any` **off**, `no-unused-vars` warn, **`no-useless-catch` warn — "Avoid catch-and-rethrow"** (a bare re-throw in `auth.ts` would trip this and, under `--max-warnings=0`, fail the gate); functions **under 50 lines**, files **under 500** (L95-99); comment the *why*, JSDoc with `@throws` on public APIs (L105-110); pre-commit checklist L145-152 includes **no `console.log()` left in code**. + +**`quality-gates.md`** — literal commands, fastest-to-slowest, stop at first failure: + +| Gate | Command | +|---|---| +| License headers | `npm run license-check` | +| Lint | `npm run lint` (`eslint '{src,tests}/**/*.ts' --max-warnings=0`) | +| Typecheck | `npm run typecheck` (`tsc --noEmit`) | +| Build | `npm run build` | +| Unit | `npm run test:unit` | +| Integration | `npm run test:integration` | +| Secrets (local) | `npm run validate:secrets` | +| Commitlint | `npm run commitlint:last` | +| Pre-commit aggregate | `npm run check:pre-commit` | +| Full CI | `npm run ci` | + +Guide text for the test scripts is **stale** vs `package.json` (actual: `vitest run --project unit` / `--project cli`). Hooks: husky pre-commit = lint-staged → typecheck; `.claude/settings.json` PostToolUse auto-runs `npm run format` after every Edit/Write; Stop runs `npm run check:pre-commit`. + +**`architecture/architecture.md`** — layer taxonomy and error flow, summarised in §2.4. + +**`project.md`** — Jira, prefix `EPMCDME`; GitHub `codemie-ai/codemie-code`, target `main`, PRs via `gh`, squash-merge default. + +**`standards/git-workflow.md`** — branch `EPMCDME-[_kebab-description]`; Conventional Commits `(): `; **allowed scopes** (commitlint `scope-enum`): `cli, agents, providers, assistants, config, proxy, workflows, ci, analytics, utils, deps, tests, skills, kimi`. **`fix(auth)` and `fix(sdk)` would be rejected** — use `fix(cli)` or `fix(utils)`. Never `--no-verify`. + +**`AGENTS.md`** (canonical; `CLAUDE.md` imports it) — "Check Guides First"; precedence: guides win for process, source wins for facts. **"Tests Only On Explicit Request"** and **"Git Operations Only On Explicit Request"**. Task classifier: `error, exception, validation` → P0 development-practices; `cli, command, commander` → P0 architecture. + +**`CONTRIBUTING.md`** — no error-handling or CLI-UX section. Conventional Commits required for commits *and PR titles*; `npm run ci` fails if the last commit is non-conforming. + +### Architectural Decisions + +**PR #471 / commit `5b2de4b7`** — `fix(providers): skip interactive re-auth prompt in non-interactive environments (#471)`, Aug 7 2026. 16 files, +1045/−5. Substantive changes: +- `src/utils/interactive.ts` — **new**, 16 lines, sole export `isNonInteractiveEnvironment()`, JSDoc naming ERR_USE_AFTER_CLOSE as the motivation. +- `src/providers/core/auth-validation.ts` — one behavioural line: guard became `if (setupSteps?.promptForReauth && !isNonInteractiveEnvironment())`. +- `docs/AUTHENTICATION.md` +15 — new section, now at **L104-117**. +- `src/providers/core/__tests__/auth-validation.test.ts` — new, 104 lines, 5 tests. +- `src/utils/__tests__/interactive.test.ts` — new, 33 lines, 3 tests. + +**Approach precedent this fix must extend, not replace**: fix at the single shared gate; TTY-only detection; no new flag; provider setup-steps untouched; co-located unit tests; documentation change is part of the deliverable. + +**Prior task `docs/superpowers/tasks/2026-08-06-non-interactive-sso/` (EPMCDME-13953)** — the decision record that governs this ticket: +- `spec.md` "Out of scope": *"No new `--non-interactive` CLI flag or `CI` env var detection (**explicit decision — TTY check only**)."* +- `spec.md` asserted the callers "already handle a failed/`false` result correctly today (… `utils/auth.ts` throws `ConfigurationError`)" — **this assumption is exactly what reproduction.md falsifies.** +- `code-review-final.json` — decision `request-changes`, CR-001 (major, docs). Deferred/dismissed list explicitly records *"a pre-existing missing try/catch explicitly out of scope"*, plus *"TTY-only detection: no stdout check, pseudo-TTY containers still hang"*. **The missing try/catch was knowingly deferred in Aug 2026 and is the live defect now.** + +**`docs/AUTHENTICATION.md:104-117`** — the documented promise, verbatim in substance: the prompt is "automatically skipped", the CLI "fails fast with a clear message … and exits non-zero instead of hanging", and (L113) *"There is no separate `--non-interactive` flag to set — detection is automatic, based solely on whether `stdin` is a TTY."* **This documented promise is precisely what the CLI does not currently deliver** — the gap is in the message, not the exit code. + +**`docs/superpowers/specs/2026-07-01-EPMCDME-12992-session-origin-validation-design.md:97`** — non-interactive (piped stdin / `--yes` / `CODEMIE_NO_PROMPTS=1`) → behave as if the user declined → exit 1. Three-way precedent worth mirroring. + +**Inline decision markers**: `grep -rnE 'TODO:|HACK:|NOTE:|FIXME:|XXX|@deprecated'` over `src/utils/auth.ts`, `src/utils/interactive.ts`, `src/providers/core/auth-validation.ts`, `src/cli/commands/sdk/`, `sso.setup-steps.ts`, `AgentCLI.ts`, `profile/index.ts`, `errors.ts` → **zero matches.** No inline decision debt; all recorded decisions live in the SDLC task dirs. + +**No CHANGELOG.md exists** anywhere in the repo. `docs/AUTHENTICATION.md` is the only user-facing surface for this behaviour. + +### Derived Conventions + +- Auth failures are surfaced by a `never`-returning helper that logs, prints one chalk-red line, and exits 1. Four such helpers exist; **reuse, do not invent a fifth.** +- Non-interactivity is detected once, in `src/utils/interactive.ts`, and consumed at the gate — never re-derived at call sites. +- Utils-layer modules should throw; the CLI layer formats and exits. + +--- + +## 4. Testing Landscape + +### Existing Coverage + +- **`src/utils/__tests__/auth.test.ts`** (177 lines) — covers `getAuthenticatedClient` (success L52, retry-after-reauth L63, throw when reauth fails L90, non-auth rethrow L108) and `promptReauthentication` (success L126, **throws `'Authentication expired. Please re-authenticate.'` at L142-155**, no setupSteps L157, no validateAuth L166). Mocks `../sdk-client.js`, `../../providers/core/registry.js`, `../../providers/core/auth-validation.js` (L9-21). **L142-155 encodes the current buggy behaviour and will have to change.** Does not cover the JWT branch (`auth.ts:23-41`) at all. +- **`src/utils/__tests__/interactive.test.ts`** (33 lines) — 3 tests; assigns `process.stdin.isTTY` directly, restores in `afterEach`. +- **`src/providers/core/__tests__/auth-validation.test.ts`** (104 lines) — the PR #471 guard test; the template (see below). +- **`src/agents/core/__tests__/`** — 5 AgentCLI test files, none about SSO/auth. +- **`src/cli/commands/sdk/**` — NO TESTS AT ALL.** All 22 files untested; there is no `src/cli/commands/sdk/__tests__/` directory. **`cli-utils.ts` — containing `getSdkClient()` and `handleSdkError()` — is entirely untested.** This is the natural home for the fix's tests and is greenfield. +- **`bin/codemie.js` — no direct tests**; exercised only as a subprocess via `tests/helpers/cli-runner.ts`. `bin/` is excluded from coverage (`vitest.config.ts:43`), so a process-level handler there cannot be unit-tested — it needs a cli-project subprocess test. + +### Testing Framework and Patterns + +- **Vitest 3+ style**, single `vitest.config.ts` (100 lines, no `vitest.workspace.ts`), three `defineProject` entries. All alias `@` → `/src`; all set `FORCE_COLOR=1`, `NODE_ENV=test`, `CODEMIE_HOME=/codemie-test-home-`. + - **unit**: `include: ['src/**/*.test.ts', 'src/**/*.spec.ts']`, `globals: true`, node env, 30 s timeouts, `isolate: true`, **no setupFiles**. + - **cli**: `include: ['tests/integration/**/*.test.ts']`, excludes `agent-*`. + - **agent**: `include: ['tests/integration/agent-*.test.ts']`, `globalSetup: tests/setup/agent-build-setup.ts`. + - Coverage on the unit project only, provider `v8`, **no `thresholds` configured** — the 80/90 % numbers in the guide are advisory, not machine-enforced. +- **Guide mandates** (`.ai-run/guides/testing/testing-patterns.md`): unit tests co-located at `src/[module]/__tests__/*.test.ts` (L9-13); AAA (L21); `vi.mock()` at module level, `vi.spyOn()` in `beforeEach` + `vi.restoreAllMocks()` in `afterEach` (L46-66); **CRITICAL — import the module under test *inside the test body* via `await import('../auth.js')` after mocks are set** (L70-94), because static top-level imports are cached before `beforeEach`; async errors via `await expect(fn()).rejects.toThrow(ErrorClass)` (L115-122); assert **both** error class and code (L128-138); no hardcoded POSIX paths (L145-154); critical paths incl. `src/utils/` 90 %+ (L256-262). **No TDD mandate** — and AGENTS.md sets a stronger repo policy: *"Tests Only On Explicit Request."* +- **Template — `src/providers/core/__tests__/auth-validation.test.ts`**: explicit named imports from `vitest` despite `globals: true` (L1-3); a single top-level `vi.mock('../../../utils/interactive.js', () => ({ isNonInteractiveEnvironment: vi.fn() }))` (L5-7); **it does NOT touch `process.stdin.isTTY` or env** — it mocks the *function* and drives it with `mockReturnValue(true|false)` per test (L26, L41, L63, L78, L94). **Inject non-interactivity at the seam, not at the process level.** Per-test dynamic import inside each `it()` (L24-28, L39-44). Console captured via `vi.spyOn(console, 'log').mockImplementation(() => {})` and asserted with `expect.stringContaining(...)` (L55-57, L88) plus `toHaveBeenCalledTimes(1)` (L72) to prove no extra output path was taken. `afterEach` calls only `vi.restoreAllMocks()`. No `process.exit` or thrown-error assertion in this file — for the exit-code half of the fix, take that from the CLI exemplars below. +- **CLI command test exemplars**: `src/cli/commands/skills/__tests__/commands.test.ts` is the best all-round template — hoisted `vi.fn()`s behind `vi.mock()` factories (L22-46); `process.exit` spy that **records the code then throws** so the action aborts, with `exitCalls[]` capturing the first meaningful code (L67-78); `process.stderr.write` silenced (L79); `vi.resetModules()` in `afterEach` (L87); commander driven via `command.exitOverride(); await command.parseAsync([...])` (L96-101). Also `src/cli/commands/proxy/__tests__/index.test.ts` (canonical `{ from: 'user' }` argv form) and `src/agents/core/__tests__/AgentCLI-resume.test.ts` (`class ExitError extends Error` carrying the exit code, L9-13; `vi.spyOn(console, 'error')` + `expect.stringContaining` for remediation text, L115-122). +- **SDK client mocking**: only one file in the repo mocks it — `src/utils/__tests__/auth.test.ts:9-11` mocks `../sdk-client.js`. There is **no `vi.mock('codemie-sdk')` anywhere**. For `cli-utils.ts` tests, mock `@/utils/auth.js`'s `getAuthenticatedClient`. +- **`@clack/prompts` is not mocked anywhere**; the repo standardises on `inquirer` in tests (10 files). `vi.stubEnv` is used in exactly one file — manual save/restore of `process.env` is the house style. +- **Integration harness**: `tests/helpers/cli-runner.ts` — `CLIRunner` wraps `execSync('node ./bin/codemie.js ')`. **`runSilent()` returns `{ output, exitCode, error }` without throwing** (L43-59) — exactly the tool for asserting a non-zero exit plus remediation text. Exemplar: `tests/integration/cli-commands/error-handling.test.ts:24-35`. stdin redirection from `/dev/null` is **not currently done anywhere but is fully supported** — `runSilent(cmd, options)` spreads options into `execSync`, so `{ stdio: ['ignore','pipe','pipe'] }` or `{ input: '' }` yields a non-TTY stdin. Caveat: the spread lands *after* `encoding: 'utf-8'`. Real-TTY cases use node-pty via `tests/helpers/pty-session.ts`. `bin/codemie.js` imports from `../dist/`, so cli-project tests require a prior `npm run build`; the **cli project has no globalSetup** — `npm run ci` relies on `build` preceding `test:integration`. + +### Coverage Gaps + +- `src/cli/commands/sdk/**` — zero tests across 22 files, including the `getSdkClient` / `handleSdkError` choke point the fix will modify. +- `src/utils/auth.ts` JWT branch (L23-41) — untested. +- `bin/codemie.js` — untested and excluded from coverage. +- No test anywhere manipulates a `CI` env var; a `CI=true`-with-TTY scenario is currently undetectable and untested. +- **Orphaned test files — verified via `npx vitest list`**: the unit project collects 3 956 tests from `src/**` and **zero** from `tests/unit`; the cli project collects 37 files, all under `tests/integration/`. These 5 files match **no** project glob and **never execute**: + - `tests/unit/cli/commands/assistants/chat/index.test.ts` + - `tests/unit/cli/commands/assistants/chat/historyLoader.test.ts` + - `tests/unit/cli/commands/assistants/chat/utils.test.ts` + - `tests/skills/pattern-invocation.test.ts` + - `tests/scripts/test-proxy-endpoint.test.ts` + + This matters directly: `src/cli/commands/assistants/chat/index.ts:423` is the second `promptReauthentication` call site and its only test file is in that dead zone. **Any new unit test placed under `tests/unit/` will silently never run.** + +--- + +## 5. Configuration and Environment + +### Environment Variables + +No central registry or allowlist of `CODEMIE_*` names exists. `src/env/manager.ts` (`EnvManager`) is a key/value store over `~/.codemie/codemie-cli.config.json` with precedence `process.env[key]` > global config (L38-44); it does **not** declare env vars. + +Relevant to this task: +- `CODEMIE_NO_PROMPTS` — opt out of interactive prompts, value `'1'` — `src/agents/core/AgentCLI.ts:756`. **The only non-test consumer, and the only env-var escape hatch precedent in the repo.** +- `CI` — **written, never read as detection** — `src/cli/commands/skills/lib/run-skills-cli.ts:89`. +- `CODEMIE_HOME` — overrides `~/.codemie` — `src/utils/paths.ts:356-361` (99 references, the most-used var; used for test isolation). +- `CODEMIE_JWT_TOKEN`, `CODEMIE_AUTH_METHOD` — JWT auth path — `src/agents/core/AgentCLI.ts:~206-207`. +- `CODEMIE_URL`, `CODEMIE_BASE_URL`, `CODEMIE_API_KEY`, `CODEMIE_PROVIDER`, `CODEMIE_MODEL`, `CODEMIE_TIMEOUT`, `CODEMIE_PROFILE_CONFIG`, `CODEMIE_PROFILE_NAME` — `src/utils/config.ts`, `src/utils/profile.ts`. +- `CODEMIE_INSECURE` — read at `src/utils/auth.ts:39` (`verify_ssl: process.env.CODEMIE_INSECURE !== '1'`). +- `CODEMIE_DEBUG` (30 refs), `DO_NOT_TRACK`, `DISABLE_TELEMETRY`, `NODE_OPTIONS`. + +**Zero hits anywhere in `src/` or `bin/` for**: `CODEMIE_NON_INTERACTIVE`, `TERM=dumb`, `FORCE_COLOR` (as a read), `noninteractive`. + +### Configuration Files + +- Global: `~/.codemie/codemie-cli.config.json` — declared twice (`src/env/manager.ts:11`, `src/utils/config.ts:57`). +- Project-local: `.codemie/codemie-cli.config.json` — `src/utils/config.ts:62`, created at L886-911; precedence documented at L113-117. +- Credentials: `src/utils/security.ts` — keychain (keytar) primary, `~/.codemie/credentials/.enc` AES-256-CBC fallback. +- `vitest.config.ts`, `commitlint.config.cjs` (`scope-enum`), `.claude/settings.json` (format/pre-commit hooks). + +### Feature Flags and Deployment Concerns + +**Definitive: no `--non-interactive`, `--ci`, `--no-input`, `--headless`, or `--batch` option exists anywhere in `src/` or `bin/`.** A targeted grep for those as commander `option(...)` registrations returns zero matches. + +The root program (`src/cli/index.ts`, `new Command()` L44) registers **exactly one** option: `.option('--task ', …)` at L61. Everything else at L80-106 is `program.addCommand(...)`. There is no root `--yes`, `--force`, or `--quiet`. + +Non-interactive detection census: +- **Predicates**: `src/utils/interactive.ts:15` (`!process.stdin.isTTY`, canonical); `src/agents/core/AgentCLI.ts:755-756` `shouldBlockNonInteractiveResume()` = `!process.stdin.isTTY || process.env.CODEMIE_NO_PROMPTS === '1'` (**the only predicate with an env escape hatch**); `src/cli/commands/skills/add.ts:52` `const interactive = !options.yes && process.stdin.isTTY === true` (**flag ∧ TTY — the closest thing to a `--non-interactive` flag**); `src/agents/core/AgentCLI.ts:176` `const isNonInteractiveMode = !!options.task`; `src/cli/commands/analytics/index.ts:154` — the only use of `stdout.isTTY` as a prompt gate. +- **Consumers**: `src/providers/core/auth-validation.ts:34` (sole consumer of the shared helper); `src/agents/core/AgentCLI.ts:707`; `src/cli/commands/skills/lib/agent-detection.ts:76-82`. Raw-mode guards (not prompt skips) at `shared/selection/interactive-prompt.ts:34,66`, `profile/index.ts:259,267`, `shared/agent-targets.ts:177,207`. +- **Forcing sites**: `src/cli/commands/skills/lib/run-skills-cli.ts:89` — `baseEnv.CI = process.env.CI ?? '1'` (note it *respects* an inherited `CI`), gated on `if (!interactive)` where interactive is the default (L64); the L84-90 comment explains that forcing `CI` on interactive runs interferes with Clack/inquirer prompts. Also `speckit.plugin.ts:162` (`--force`), `bmad.plugin.ts:213` (`--yes`), `native-installer.ts:102`, `processes.ts:161`, `claude.plugin.ts:656`. +- **Flag naming precedent**: `-y, --yes` "skip interactive confirmations" on subcommands — `skills/add.ts:46`, `skills/remove.ts:39`, `skills/update.ts:30`, `profile/index.ts:377`, `log/index.ts:144`; `--force` at `setup.ts:129`, `workflow.ts:155`, `proxy/index.ts:297,330,350`. Machine-output precedent `--json` at `proxy/index.ts:196`, `skills/find.ts:42`, `skills/list.ts:28`. + +**Verdict on AC 3**: introducing `--non-interactive` or `--ci` would be **novel and contrary to a recorded decision**. `docs/AUTHENTICATION.md:113` documents the flag's absence as intentional, and EPMCDME-13953's spec lists it as explicitly out of scope. AC 3 is worded "supported **or documented**" and is arguably **already satisfied by documentation**; the honest options are (a) cite the existing docs and close AC 3, or (b) if an opt-out is wanted, follow `CODEMIE_NO_PROMPTS=1` / `-y, --yes` rather than inventing a new global flag. + +Prompt-surface risk: there are **46 `inquirer.prompt` call sites across 19 non-test files**, and **only the `promptForReauth` path is guarded** by `isNonInteractiveEnvironment()`. Unguarded prompt-bearing modules include `providers/core/codemie-auth-helpers.ts`, `providers/plugins/jwt/jwt.setup-steps.ts`, `cli/commands/setup.ts`, `install.ts`, `update.ts`, `utils/cli-updater.ts`, `agents/core/BaseAgentAdapter.ts`, `assistants/chat/index.ts`. These are latent instances of the same class of bug, out of scope here but worth flagging. + +--- + +## 6. Risk Indicators + +- **Ticket premise is partly wrong.** `assistants.ts` *does* use `handleSdkError`; the defect is `await getSdkClient()` sitting outside the `try`. A plan written from the ticket text alone will fix the wrong thing. +- **Blast radius is ~50 actions across 8 files, not one.** `assistants.ts:65,115,166,198,220,240` plus 44 analogous lines in `categories/datasources/integrations/llm/skills/users/workflows`. Fixing only `assistants list` leaves 50 identical paths broken. PR #471's shared-gate precedent argues for fixing inside `getSdkClient()` (`cli-utils.ts:15-18`) rather than at 50 call sites. +- **Two independent defects must both be fixed.** Wrapping `getSdkClient` alone removes the stack trace but still prints the useless `'Authentication expired. Please re-authenticate.'`. Preserving the message alone still leaves the throw uncaught. AC 2 ("clear remediation") fails on *clear*, not on *non-zero* — the CLI already exits 1. +- **`ConfigurationError`'s constructor accepts only `message`** (`src/utils/errors.ts:8-13`). Preserving the upstream error via `{ cause }` requires a constructor change (touching a base class used repo-wide) or a post-hoc `.cause` assignment. Neither is established practice here. +- **`no-useless-catch` is an ESLint warn and the lint gate runs `--max-warnings=0`.** A naive catch-and-rethrow in `auth.ts` will fail CI. +- **`promptReauthentication`'s `Promise` return type is a lie** — it can only return `true` or throw, making `auth.ts:48`'s `if (reauthed)` false branch dead code (`auth.ts:52` unreachable via that route). Changing the throw to a `false` return would revive dead code paths and change `src/cli/commands/assistants/chat/index.ts:423` behaviour. +- **Regression pressure on an existing test.** `src/utils/__tests__/auth.test.ts:142-155` asserts the exact buggy message. Any message change breaks it; the test must be updated deliberately, not incidentally. +- **`src/cli/commands/sdk/**` has zero test coverage** across 22 files, including the choke point being modified. Tests there are greenfield — no local conventions to copy, must borrow from `skills/__tests__/commands.test.ts`. +- **Orphaned test directory.** Five test files under `tests/unit/` and `tests/skills/` match no vitest project glob and never run. A new unit test placed under `tests/unit/` would silently never execute. Unit tests must go in `src/**/__tests__/`. +- **No process-level error boundary exists.** `src/cli/index.ts:147` uses sync `program.parse` with no `.exitOverride()`; `bin/codemie.js`'s `.catch` covers only module-load rejections. Any future uncaught async rejection in any of ~29 command files prints a raw stack. A `process.on('unhandledRejection')` net (template at `bin/codemie-mcp-proxy.js:75-82`) would be defence in depth — but `bin/` is excluded from coverage and cannot be unit-tested. +- **`auth-validation.ts:39` writes the diagnostic to stdout via `console.log`**, polluting piped output and breaking `--json` consumers. Cosmetic but in scope-adjacent. +- **Layering.** `src/utils/auth.ts` (Utils) already reaches into `ProviderRegistry` (Core) and does chalk console output (L71), contradicting `architecture.md` L159-171 ("CLI catches, formats for user"). The fix must not deepen this — user-facing formatting belongs in `cli-utils.ts`. +- **AC 3 conflicts with a recorded decision.** EPMCDME-13953's spec put `--non-interactive`/`--ci` explicitly out of scope, and `docs/AUTHENTICATION.md:113` documents the absence as intentional. Adding the flag now would contradict shipped documentation; resolving AC 3 is a product decision, not an implementation one. +- **AC 4 (ERR_USE_AFTER_CLOSE on kill) is unproven, not disproven.** Two attempts failed to reproduce. The reproduction also observed an ora escape-sequence flood (73 MB capture) under `script`, attributed to a pty with no usable `stdout.columns` — flagged as needing a real-terminal recheck before being treated as a finding. Note `src/utils/sdk-client.ts:26` starts an ora spinner even in non-TTY, which is a plausible contributor. +- **The same class of bug is latent in 45 other prompt sites.** Only `promptForReauth` is TTY-guarded; 46 `inquirer.prompt` call sites exist across 19 non-test files. +- **`docs/AUTHENTICATION.md:104-117` currently makes a promise the CLI does not keep.** If the fix changes the emitted message, that section must be updated in the same change — PR #471's precedent makes the doc update part of the deliverable, and CR-001 on the prior ticket was closed *specifically* on that doc addition. +- **Commit scope constraint**: `fix(auth)` / `fix(sdk)` are **not** in commitlint's `scope-enum` and will be rejected. Use `fix(cli)` or `fix(utils)`. +- **`cli-utils.ts` uses double quotes** against the guide's single-quote rule; ESLint does not enforce quote style. Do not mass-reformat — it would balloon the diff. +- **Repo policy: "Tests Only On Explicit Request"** (AGENTS.md). Coverage thresholds in the testing guide are advisory; `vitest.config.ts` configures **no** thresholds. + +--- + +## 7. Summary for Complexity Assessment + +**Layers and file surface.** The task touches three layers: CLI (`src/cli/commands/sdk/utils/cli-utils.ts`, and potentially 8 sdk command files), Utils (`src/utils/auth.ts`, possibly `src/utils/errors.ts`), and Core (`src/providers/core/auth-validation.ts`, a one-line stdout→stderr change). The minimal correct implementation is small — the entire defect funnels through a single 4-line function, `getSdkClient()` at `cli-utils.ts:15-18`, which is the sole bridge from all ~50 sdk actions into the auth path. A shared-gate fix there plus a message-preservation change in `auth.ts:76` is roughly **2-3 source files, well under 100 changed lines**. The tempting alternative — moving `await getSdkClient()` inside the `try` in each action — is ~50 mechanical edits across 8 files and should be rejected in favour of the gate, consistent with the precedent set by PR #471. Documentation (`docs/AUTHENTICATION.md:104-117`) must be updated in the same change; the prior ticket's code review was closed specifically on that doc surface, so omitting it repeats a known review failure. + +**Technical novelty: low, but the design space has traps.** Every ingredient already exists in-repo. `isNonInteractiveEnvironment()` is the sanctioned detector; `handleSdkError`, `failAuth` (`skills/lib/require-auth.ts:24-51`), `printProxyError` and `handleSetupError` are four existing `never`-returning error sinks, and `require-auth.ts`'s `NOT_AUTHENTICATED_MESSAGE` is verbatim the actionable text the ticket asks for. Nothing needs inventing. The traps are: (1) `ConfigurationError`'s constructor takes only `message`, so preserving the upstream error as `cause` means touching a repo-wide base class; (2) ESLint's `no-useless-catch` warn under `--max-warnings=0` makes a naive catch-and-rethrow a CI failure; (3) `promptReauthentication`'s declared `Promise` is unreachable-`false`, so switching from throw to return revives dead code and changes behaviour at `assistants/chat/index.ts:423`; (4) AC 3 (`--non-interactive`/`--ci`) contradicts an explicit recorded out-of-scope decision from EPMCDME-13953 and shipped text in `docs/AUTHENTICATION.md:113` — it is a product call, not an implementation one, and should be escalated rather than implemented unilaterally. + +**Test posture: mixed, tilting bad at the point of change.** `src/utils/auth.ts` and `src/providers/core/auth-validation.ts` are well covered, and `src/utils/__tests__/auth.test.ts:142-155` currently asserts the *buggy* message — so a fix necessarily edits an existing green test, which reviewers must not mistake for a regression. Conversely `src/cli/commands/sdk/**` has **zero tests across all 22 files**, so tests for the primary fix location are greenfield. The templates are unambiguous: `src/providers/core/__tests__/auth-validation.test.ts` for mocking the non-interactive seam (mock the function, not `process.stdin`), `src/cli/commands/skills/__tests__/commands.test.ts` for the `process.exit` spy-and-throw pattern, and `tests/helpers/cli-runner.ts`'s `runSilent()` for an end-to-end non-zero-exit assertion with stdin redirected. Two landmines: unit tests must live in `src/**/__tests__/` because five existing files under `tests/unit/` match no vitest glob and silently never run; and `bin/` is excluded from coverage, so any process-level `unhandledRejection` net cannot be unit-tested and needs a cli-project subprocess test. + +**Risk factors for scoring.** Complexity is inflated above the raw line count by: a partially incorrect ticket premise that must be corrected before planning; a 50-call-site blast radius that makes "fix the reported symptom" the wrong answer; one acceptance criterion (AC 3) that conflicts with a documented prior decision; another (AC 4) that was never reproduced and may not be a real defect; a required change to an existing passing test; zero test coverage at the primary fix location; and a mandatory documentation update. Nothing here is architecturally hard — the difficulty is entirely in scoping discipline and in not regressing the four adjacent auth paths (`assistants/chat`, `assistants/setup`, `skills/setup`, `AgentCLI`) that already handle this correctly. diff --git a/src/cli/commands/sdk/utils/__tests__/cli-utils.test.ts b/src/cli/commands/sdk/utils/__tests__/cli-utils.test.ts new file mode 100644 index 000000000..7c54e76f3 --- /dev/null +++ b/src/cli/commands/sdk/utils/__tests__/cli-utils.test.ts @@ -0,0 +1,70 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; + +vi.mock('@/utils/config.js', () => ({ + ConfigLoader: { load: vi.fn() }, +})); + +vi.mock('@/utils/auth.js', () => ({ + getAuthenticatedClient: vi.fn(), +})); + +vi.mock('@/utils/logger.js', () => ({ + logger: { error: vi.fn(), debug: vi.fn(), warn: vi.fn(), info: vi.fn() }, +})); + +const SSO_ERROR_MESSAGE = + 'SSO authentication required. Please run "codemie setup" with SSO provider first.'; + +describe('getSdkClient', () => { + let exitCode: number | undefined; + let stderr: string[]; + + beforeEach(() => { + exitCode = undefined; + stderr = []; + vi.spyOn(process, 'exit').mockImplementation(((code?: number) => { + exitCode = code; + throw new Error(`process.exit:${code}`); + }) as never); + vi.spyOn(console, 'error').mockImplementation((...args: unknown[]) => { + stderr.push(args.join(' ')); + }); + }); + + afterEach(() => { + vi.restoreAllMocks(); + vi.resetModules(); + }); + + it('exits non-zero via handleSdkError with the remediation on stderr', async () => { + const { ConfigLoader } = await import('@/utils/config.js'); + const { getAuthenticatedClient } = await import('@/utils/auth.js'); + const { ConfigurationError } = await import('@/utils/errors.js'); + + vi.mocked(ConfigLoader.load).mockResolvedValue({} as never); + vi.mocked(getAuthenticatedClient).mockRejectedValue( + new ConfigurationError(SSO_ERROR_MESSAGE) + ); + + const { getSdkClient } = await import('../cli-utils.js'); + + // handleSdkError terminates the process; the spy converts that into a throw. + await expect(getSdkClient()).rejects.toThrow('process.exit:1'); + expect(exitCode).toBe(1); + expect(stderr.join('\n')).toContain('codemie setup'); + }); + + it('returns the client unchanged when authentication succeeds', async () => { + const { ConfigLoader } = await import('@/utils/config.js'); + const { getAuthenticatedClient } = await import('@/utils/auth.js'); + + const client = { marker: 'authenticated-client' }; + vi.mocked(ConfigLoader.load).mockResolvedValue({} as never); + vi.mocked(getAuthenticatedClient).mockResolvedValue(client as never); + + const { getSdkClient } = await import('../cli-utils.js'); + + await expect(getSdkClient()).resolves.toBe(client); + expect(exitCode).toBeUndefined(); + }); +}); diff --git a/src/cli/commands/sdk/utils/cli-utils.ts b/src/cli/commands/sdk/utils/cli-utils.ts index 3d0887cc6..c2e93f6cd 100644 --- a/src/cli/commands/sdk/utils/cli-utils.ts +++ b/src/cli/commands/sdk/utils/cli-utils.ts @@ -11,10 +11,17 @@ import z, { ZodError } from "zod"; /** * Get an authenticated CodeMie SDK client + * + * Every sdk action calls this outside its own try/catch, so this is the single + * gate keeping a missing session from printing a raw stack (EPMCDME-14148). */ export async function getSdkClient(): Promise { - const config = await ConfigLoader.load(); - return getAuthenticatedClient(config); + try { + const config = await ConfigLoader.load(); + return await getAuthenticatedClient(config); + } catch (error) { + handleSdkError(error, "authenticate"); + } } /** diff --git a/src/providers/core/__tests__/auth-validation.test.ts b/src/providers/core/__tests__/auth-validation.test.ts index f0aac3820..9487c177c 100644 --- a/src/providers/core/__tests__/auth-validation.test.ts +++ b/src/providers/core/__tests__/auth-validation.test.ts @@ -39,7 +39,7 @@ describe('handleAuthValidationFailure', () => { const { isNonInteractiveEnvironment } = await import('../../../utils/interactive.js'); isNonInteractiveEnvironmentMock = isNonInteractiveEnvironment as ReturnType; isNonInteractiveEnvironmentMock.mockReturnValue(true); - const consoleLogSpy = vi.spyOn(console, 'log').mockImplementation(() => {}); + const consoleErrorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); const { handleAuthValidationFailure } = await import('../auth-validation.js'); const setupSteps = { promptForReauth: promptForReauthSpy } as unknown as ProviderSetupSteps; @@ -52,7 +52,7 @@ describe('handleAuthValidationFailure', () => { expect(promptForReauthSpy).not.toHaveBeenCalled(); expect(result).toBe(false); - expect(consoleLogSpy).toHaveBeenCalledWith( + expect(consoleErrorSpy).toHaveBeenCalledWith( expect.stringContaining('No valid SSO credentials found.') ); }); @@ -61,7 +61,7 @@ describe('handleAuthValidationFailure', () => { const { isNonInteractiveEnvironment } = await import('../../../utils/interactive.js'); isNonInteractiveEnvironmentMock = isNonInteractiveEnvironment as ReturnType; isNonInteractiveEnvironmentMock.mockReturnValue(true); - const consoleLogSpy = vi.spyOn(console, 'log').mockImplementation(() => {}); + const consoleErrorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); const { handleAuthValidationFailure } = await import('../auth-validation.js'); const setupSteps = { promptForReauth: promptForReauthSpy } as unknown as ProviderSetupSteps; @@ -69,14 +69,14 @@ describe('handleAuthValidationFailure', () => { await handleAuthValidationFailure(validationResult, setupSteps, testConfig); - expect(consoleLogSpy).toHaveBeenCalledTimes(1); + expect(consoleErrorSpy).toHaveBeenCalledTimes(1); }); it('should keep existing behavior when setupSteps has no promptForReauth (JWT-style), regardless of TTY', async () => { const { isNonInteractiveEnvironment } = await import('../../../utils/interactive.js'); isNonInteractiveEnvironmentMock = isNonInteractiveEnvironment as ReturnType; isNonInteractiveEnvironmentMock.mockReturnValue(false); - const consoleLogSpy = vi.spyOn(console, 'log').mockImplementation(() => {}); + const consoleErrorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); const { handleAuthValidationFailure } = await import('../auth-validation.js'); const setupSteps = {} as ProviderSetupSteps; @@ -85,14 +85,14 @@ describe('handleAuthValidationFailure', () => { const result = await handleAuthValidationFailure(validationResult, setupSteps, testConfig); expect(result).toBe(false); - expect(consoleLogSpy).toHaveBeenCalledWith(expect.stringContaining('JWT token missing')); + expect(consoleErrorSpy).toHaveBeenCalledWith(expect.stringContaining('JWT token missing')); }); it('should return false when setupSteps is null, regardless of TTY', async () => { const { isNonInteractiveEnvironment } = await import('../../../utils/interactive.js'); isNonInteractiveEnvironmentMock = isNonInteractiveEnvironment as ReturnType; isNonInteractiveEnvironmentMock.mockReturnValue(true); - vi.spyOn(console, 'log').mockImplementation(() => {}); + vi.spyOn(console, 'error').mockImplementation(() => {}); const { handleAuthValidationFailure } = await import('../auth-validation.js'); const validationResult: AuthValidationResult = { valid: false, error: 'no provider configured' }; @@ -101,4 +101,21 @@ describe('handleAuthValidationFailure', () => { expect(result).toBe(false); }); + + it('should write the failure diagnostic to stderr so piped stdout stays clean', async () => { + const { isNonInteractiveEnvironment } = await import('../../../utils/interactive.js'); + isNonInteractiveEnvironmentMock = isNonInteractiveEnvironment as ReturnType; + isNonInteractiveEnvironmentMock.mockReturnValue(true); + const consoleLogSpy = vi.spyOn(console, 'log').mockImplementation(() => {}); + const consoleErrorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); + + const { handleAuthValidationFailure } = await import('../auth-validation.js'); + const setupSteps = { promptForReauth: promptForReauthSpy } as unknown as ProviderSetupSteps; + const validationResult: AuthValidationResult = { valid: false, error: 'session expired' }; + + await handleAuthValidationFailure(validationResult, setupSteps, testConfig); + + expect(consoleErrorSpy).toHaveBeenCalledWith(expect.stringContaining('session expired')); + expect(consoleLogSpy).not.toHaveBeenCalled(); + }); }); diff --git a/src/providers/core/auth-validation.ts b/src/providers/core/auth-validation.ts index 0dff91e5b..edab067da 100644 --- a/src/providers/core/auth-validation.ts +++ b/src/providers/core/auth-validation.ts @@ -35,7 +35,7 @@ export async function handleAuthValidationFailure( return await setupSteps.promptForReauth(config); } - // No re-auth available (or no TTY to prompt on), show full error with instructions - console.log(chalk.red(`\n✗ ${validationResult.error}\n`)); + // stderr, so piped stdout and --json consumers stay clean (EPMCDME-14148). + console.error(chalk.red(`\n✗ ${validationResult.error}\n`)); return false; } diff --git a/src/utils/__tests__/auth.test.ts b/src/utils/__tests__/auth.test.ts index 9133c53e4..acad79fec 100644 --- a/src/utils/__tests__/auth.test.ts +++ b/src/utils/__tests__/auth.test.ts @@ -87,7 +87,7 @@ describe('Auth Utilities', () => { ); }); - it('should throw error if re-authentication fails', async () => { + it('should preserve the actionable setup message if re-authentication fails', async () => { const authError = new ConfigurationError('SSO authentication required. Please run "codemie setup" with SSO provider first.'); getCodemieClient.mockRejectedValue(authError); @@ -101,7 +101,12 @@ describe('Auth Utilities', () => { const { getAuthenticatedClient } = await import('../auth.js'); - await expect(getAuthenticatedClient(mockConfig)).rejects.toThrow(ConfigurationError); + // The generic 'Authentication expired' throw inside promptReauthentication + // must not shadow the upstream message that names the remediation. + const thrown = await getAuthenticatedClient(mockConfig).catch((error: unknown) => error); + + expect(thrown).toBeInstanceOf(ConfigurationError); + expect((thrown as ConfigurationError).message).toContain('codemie setup'); expect(getCodemieClient).toHaveBeenCalledTimes(1); }); diff --git a/src/utils/__tests__/interactive.test.ts b/src/utils/__tests__/interactive.test.ts index 841503ebc..57341aebd 100644 --- a/src/utils/__tests__/interactive.test.ts +++ b/src/utils/__tests__/interactive.test.ts @@ -31,3 +31,40 @@ describe('isNonInteractiveEnvironment', () => { expect(isNonInteractiveEnvironment()).toBe(false); }); }); + +describe('isNonInteractiveOutput', () => { + const originalStdin = process.stdin.isTTY; + const originalStderr = process.stderr.isTTY; + + afterEach(() => { + process.stdin.isTTY = originalStdin; + process.stderr.isTTY = originalStderr; + }); + + + + // Input and output redirect independently — the two must be able to disagree. + it('should track stderr, not stdin, when only stdin is redirected', async () => { + process.stdin.isTTY = false as unknown as true; + process.stderr.isTTY = true; + + const { isNonInteractiveOutput, isNonInteractiveEnvironment } = await import( + '../interactive.js' + ); + + expect(isNonInteractiveEnvironment()).toBe(true); + expect(isNonInteractiveOutput()).toBe(false); + }); + + it('should track stderr, not stdin, when only output is redirected', async () => { + process.stdin.isTTY = true; + process.stderr.isTTY = undefined as unknown as true; + + const { isNonInteractiveOutput, isNonInteractiveEnvironment } = await import( + '../interactive.js' + ); + + expect(isNonInteractiveEnvironment()).toBe(false); + expect(isNonInteractiveOutput()).toBe(true); + }); +}); diff --git a/src/utils/__tests__/process-guards.coverage.test.ts b/src/utils/__tests__/process-guards.coverage.test.ts new file mode 100644 index 000000000..455b8a588 --- /dev/null +++ b/src/utils/__tests__/process-guards.coverage.test.ts @@ -0,0 +1,63 @@ +/** + * Every declared bin must either install the process guards or be an explicit, + * justified exclusion — so a new entrypoint fails here rather than silently + * shipping without a fatal-error net (EPMCDME-14148). + */ + +import { describe, it, expect } from 'vitest'; +import { readFileSync } from 'node:fs'; +import { join } from 'node:path'; +import { getDirname } from '../paths.js'; + +const REPO_ROOT = join(getDirname(import.meta.url), '..', '..', '..'); + +/** + * Long-running processes must survive an unhandled rejection; installProcessGuards + * exits on the first one, which would be wrong for them. + */ +const EXCLUDED: Record = { + 'codemie-mcp-proxy': + 'owns uncaughtException/unhandledRejection handlers with a deliberate survive-on-rejection policy', + 'proxy-daemon': 'long-running daemon — must not exit on the first unhandled rejection', +}; + +describe('process guard coverage across bin entrypoints', () => { + const pkg = JSON.parse( + readFileSync(join(REPO_ROOT, 'package.json'), 'utf-8') + ) as { bin: Record }; + + const entrypoints = Object.entries(pkg.bin); + + it('declares at least the known entrypoints', () => { + expect(entrypoints.length).toBeGreaterThanOrEqual(14); + }); + + it.each(entrypoints)('%s installs the guards or is explicitly excluded', (name, relPath) => { + // Strip comments first: the excluded entrypoints name the function in a + // comment explaining why they opt out. + const source = readFileSync(join(REPO_ROOT, relPath), 'utf-8') + .replace(/\/\*[\s\S]*?\*\//g, '') + .replace(/^\s*\/\/.*$/gm, ''); + const installs = /^\s*installProcessGuards\(\);/m.test(source); + + if (name in EXCLUDED) { + expect( + installs, + `${name} is listed as excluded (${EXCLUDED[name]}) but now installs the guards — drop it from EXCLUDED` + ).toBe(false); + return; + } + + expect( + installs, + `${name} (${relPath}) neither installs the guards nor is listed in EXCLUDED with a reason` + ).toBe(true); + }); + + it('lists no stale exclusions', () => { + const declared = new Set(entrypoints.map(([name]) => name)); + for (const name of Object.keys(EXCLUDED)) { + expect(declared.has(name), `EXCLUDED lists ${name}, which package.json no longer declares`).toBe(true); + } + }); +}); diff --git a/src/utils/__tests__/process-guards.logfile.test.ts b/src/utils/__tests__/process-guards.logfile.test.ts new file mode 100644 index 000000000..ca77c119c --- /dev/null +++ b/src/utils/__tests__/process-guards.logfile.test.ts @@ -0,0 +1,59 @@ +/** Uses the REAL logger — process-guards.test.ts mocks it and so cannot catch a + * broken logging contract (CR-001). */ + +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { readFileSync, existsSync } from 'node:fs'; + +type Handler = (payload: unknown) => void; + +describe('installProcessGuards log-file persistence', () => { + let handlers: Record; + + beforeEach(() => { + handlers = {}; + vi.spyOn(process, 'on').mockImplementation((( + event: string, + handler: Handler + ) => { + handlers[event] = handler; + return process; + }) as never); + + vi.spyOn(process, 'exit').mockImplementation(((code?: number) => { + throw new Error(`process.exit:${code}`); + }) as never); + + vi.spyOn(console, 'error').mockImplementation(() => {}); + }); + + afterEach(() => { + vi.restoreAllMocks(); + vi.resetModules(); + }); + + it('writes the stack to the log file even though the console omits it', async () => { + const { logger } = await import('../logger.js'); + const { installProcessGuards } = await import('../process-guards.js'); + + // Asserted, not early-returned: a silent return would pass vacuously. + const logPath = logger.getLogFilePath(); + expect(logPath).toBeTruthy(); + + const before = existsSync(logPath!) ? readFileSync(logPath!, 'utf-8') : ''; + + installProcessGuards(); + + const marker = 'process-guards-logfile-probe'; + const boom = new Error(marker); + + expect(() => handlers.uncaughtException(boom)).toThrow('process.exit:1'); + + const after = readFileSync(logPath!, 'utf-8'); + const appended = after.slice(before.length); + + expect(appended).toContain(marker); + // A stack, not just the message. + expect(appended).toMatch(/\n\s+at\s/); + expect(appended).not.toContain('[object Object]'); + }); +}); diff --git a/src/utils/__tests__/process-guards.test.ts b/src/utils/__tests__/process-guards.test.ts new file mode 100644 index 000000000..a572e05cc --- /dev/null +++ b/src/utils/__tests__/process-guards.test.ts @@ -0,0 +1,121 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; + +vi.mock('../logger.js', () => ({ + logger: { + error: vi.fn(), + debug: vi.fn(), + warn: vi.fn(), + info: vi.fn(), + getLogFilePath: vi.fn(() => null), + }, +})); + +type Handler = (payload: unknown) => void; + +describe('installProcessGuards', () => { + let handlers: Record; + let exitCode: number | undefined; + let stderr: string[]; + + beforeEach(() => { + handlers = {}; + exitCode = undefined; + stderr = []; + + vi.spyOn(process, 'on').mockImplementation((( + event: string, + handler: Handler + ) => { + handlers[event] = handler; + return process; + }) as never); + + vi.spyOn(process, 'exit').mockImplementation(((code?: number) => { + exitCode = code; + throw new Error(`process.exit:${code}`); + }) as never); + + vi.spyOn(console, 'error').mockImplementation((...args: unknown[]) => { + stderr.push(args.join(' ')); + }); + }); + + afterEach(() => { + vi.restoreAllMocks(); + vi.resetModules(); + }); + + it('registers guards for both unhandledRejection and uncaughtException', async () => { + const { installProcessGuards } = await import('../process-guards.js'); + + installProcessGuards(); + + expect(handlers.unhandledRejection).toBeTypeOf('function'); + expect(handlers.uncaughtException).toBeTypeOf('function'); + }); + + it('reports an unhandled rejection without a stack trace and exits non-zero', async () => { + const { installProcessGuards } = await import('../process-guards.js'); + installProcessGuards(); + + const boom = new Error('credentials unavailable'); + + expect(() => handlers.unhandledRejection(boom)).toThrow('process.exit:1'); + expect(exitCode).toBe(1); + + const output = stderr.join('\n'); + expect(output).toContain('credentials unavailable'); + expect(output).not.toContain('at '); + }); + + it('reports an uncaught exception without a stack trace and exits non-zero', async () => { + const { installProcessGuards } = await import('../process-guards.js'); + installProcessGuards(); + + expect(() => handlers.uncaughtException(new Error('boom'))).toThrow( + 'process.exit:1' + ); + expect(exitCode).toBe(1); + expect(stderr.join('\n')).not.toContain('at '); + }); + + it('passes the original error to the logger so the stack is preserved', async () => { + const { logger } = await import('../logger.js'); + const { installProcessGuards } = await import('../process-guards.js'); + installProcessGuards(); + + const boom = new Error('credentials unavailable'); + + expect(() => handlers.uncaughtException(boom)).toThrow('process.exit:1'); + + // logger.error unwraps .stack only from an Error; an object literal + // stringifies to "[object Object]". + expect(logger.error).toHaveBeenCalledWith(expect.any(String), boom); + }); + + it('sets a non-zero exitCode before exiting so a premature natural exit still fails', async () => { + const { installProcessGuards } = await import('../process-guards.js'); + installProcessGuards(); + + const original = process.exitCode; + try { + process.exitCode = 0; + expect(() => handlers.uncaughtException(new Error('boom'))).toThrow( + 'process.exit:1' + ); + expect(process.exitCode).toBe(1); + } finally { + process.exitCode = original; + } + }); + + it('handles a non-Error rejection reason without crashing', async () => { + const { installProcessGuards } = await import('../process-guards.js'); + installProcessGuards(); + + expect(() => handlers.unhandledRejection('plain string reason')).toThrow( + 'process.exit:1' + ); + expect(stderr.join('\n')).toContain('plain string reason'); + }); +}); diff --git a/src/utils/__tests__/sdk-client.test.ts b/src/utils/__tests__/sdk-client.test.ts new file mode 100644 index 000000000..97931f371 --- /dev/null +++ b/src/utils/__tests__/sdk-client.test.ts @@ -0,0 +1,97 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; + +const oraInstance = { + text: '', + start: vi.fn(), + succeed: vi.fn(), + fail: vi.fn(), +}; +const oraFactory = vi.fn(() => { + oraInstance.start.mockReturnValue(oraInstance); + return oraInstance; +}); + +vi.mock('ora', () => ({ default: oraFactory })); + +vi.mock('../interactive.js', () => ({ + isNonInteractiveEnvironment: vi.fn(), + isNonInteractiveOutput: vi.fn(), +})); + +vi.mock('../config.js', () => ({ + ConfigLoader: { load: vi.fn() }, +})); + +vi.mock('../logger.js', () => ({ + logger: { error: vi.fn(), debug: vi.fn(), warn: vi.fn(), info: vi.fn() }, +})); + +const getStoredCredentials = vi.fn(); +vi.mock('../../providers/plugins/sso/sso.auth.js', () => ({ + CodeMieSSO: class { + getStoredCredentials = getStoredCredentials; + }, +})); + +describe('getCodemieClient spinner behaviour', () => { + beforeEach(() => { + oraFactory.mockClear(); + oraInstance.start.mockClear(); + getStoredCredentials.mockReset(); + }); + + afterEach(() => { + vi.resetModules(); + }); + + async function arrange(opts: { stdinTty: boolean; stderrTty: boolean }) { + const { isNonInteractiveEnvironment, isNonInteractiveOutput } = await import( + '../interactive.js' + ); + const { ConfigLoader } = await import('../config.js'); + + vi.mocked(isNonInteractiveEnvironment).mockReturnValue(!opts.stdinTty); + vi.mocked(isNonInteractiveOutput).mockReturnValue(!opts.stderrTty); + vi.mocked(ConfigLoader.load).mockResolvedValue({ + codeMieUrl: 'https://example.test', + } as never); + getStoredCredentials.mockResolvedValue(null); + + return import('../sdk-client.js'); + } + + + + it('suppresses the spinner when stdout/stderr are redirected but stdin is a TTY', async () => { + const { ConfigurationError } = await import('../errors.js'); + const { getCodemieClient } = await arrange({ + stdinTty: true, + stderrTty: false, + }); + + await expect(getCodemieClient()).rejects.toThrow(ConfigurationError); + expect(oraFactory).not.toHaveBeenCalled(); + }); + + it('keeps the spinner when stdin is redirected but stderr is still a TTY', async () => { + const { ConfigurationError } = await import('../errors.js'); + const { getCodemieClient } = await arrange({ + stdinTty: false, + stderrTty: true, + }); + + await expect(getCodemieClient()).rejects.toThrow(ConfigurationError); + expect(oraFactory).toHaveBeenCalled(); + }); + + it('honours an explicit quiet flag even when stderr is a TTY', async () => { + const { ConfigurationError } = await import('../errors.js'); + const { getCodemieClient } = await arrange({ + stdinTty: true, + stderrTty: true, + }); + + await expect(getCodemieClient(true)).rejects.toThrow(ConfigurationError); + expect(oraFactory).not.toHaveBeenCalled(); + }); +}); diff --git a/src/utils/auth.ts b/src/utils/auth.ts index ff542f660..5342a3fe7 100644 --- a/src/utils/auth.ts +++ b/src/utils/auth.ts @@ -44,7 +44,12 @@ export async function getAuthenticatedClient(config: ProviderProfile): Promise { + throw error; + }); if (reauthed) { return await getCodemieClient(); } diff --git a/src/utils/interactive.ts b/src/utils/interactive.ts index e37950e80..610213654 100644 --- a/src/utils/interactive.ts +++ b/src/utils/interactive.ts @@ -14,3 +14,13 @@ export function isNonInteractiveEnvironment(): boolean { return !process.stdin.isTTY; } + +/** + * Returns true when progress output would not reach a terminal. + * + * Separate from isNonInteractiveEnvironment() because input and output redirect + * independently: `cmd > log 2>&1` still has an interactive stdin. + */ +export function isNonInteractiveOutput(): boolean { + return !process.stderr.isTTY; +} diff --git a/src/utils/process-guards.ts b/src/utils/process-guards.ts new file mode 100644 index 000000000..35021748e --- /dev/null +++ b/src/utils/process-guards.ts @@ -0,0 +1,74 @@ +/** + * Process-level error guards. + * + * program.parse() is synchronous, so a rejection escaping an async commander + * action reaches neither the action's try/catch nor the import().catch() in + * bin/codemie.js — Node then prints a raw stack trace (EPMCDME-14148). + */ + +import { appendFileSync } from 'node:fs'; +import chalk from 'chalk'; +import { getErrorMessage } from './errors.js'; +import { logger } from './logger.js'; +import { sanitizeLogArgs } from './security.js'; + +/** logger writes through a WriteStream that process.exit() does not drain. */ +function persistFatalSync(logPath: string, kind: string, payload: unknown): void { + try { + const detail = + payload instanceof Error && payload.stack + ? payload.stack + : getErrorMessage(payload); + const [safeDetail] = sanitizeLogArgs(detail); + + appendFileSync( + logPath, + `[${new Date().toISOString()}] [FATAL] ${kind}: ${String(safeDetail)}\n` + ); + } catch { + // A logging failure must never mask the original fatal. + } +} + +function reportFatal(kind: string, payload: unknown): never { + const message = getErrorMessage(payload); + + // Set before anything that could exit early: console.error can throw EPIPE + // under `codemie … | head`, skipping process.exit() below. + process.exitCode = 1; + + // Its file write never survives process.exit() — measured. Kept for the + // CODEMIE_DEBUG console echo; persistFatalSync is the durable path. + logger.error(`${kind}: ${message}`, payload); + + const logPath = logger.getLogFilePath(); + if (logPath) { + persistFatalSync(logPath, kind, payload); + } + + // Point at the stack rather than printing it, matching logger.notice(). + const suffix = logPath ? ` (see ${logPath})` : ''; + console.error(chalk.red(`\n❌ ${message}${suffix}\n`)); + process.exit(1); +} + +let installed = false; + +/** + * Called from each bin/* entrypoint, never from the AgentCLI constructor — + * constructing an AgentCLI must not mutate global process state (unit tests do). + */ +export function installProcessGuards(): void { + if (installed) { + return; + } + installed = true; + + process.on('unhandledRejection', (reason: unknown) => { + reportFatal('Unhandled rejection', reason); + }); + + process.on('uncaughtException', (error: unknown) => { + reportFatal('Uncaught exception', error); + }); +} diff --git a/src/utils/sdk-client.ts b/src/utils/sdk-client.ts index 1f948d2dd..f2ddf5e64 100644 --- a/src/utils/sdk-client.ts +++ b/src/utils/sdk-client.ts @@ -11,6 +11,7 @@ import type { CodeMieConfigOptions } from '../env/types.js'; import { CodeMieSSO } from '../providers/plugins/sso/sso.auth.js'; import { ConfigLoader } from './config.js'; import { ConfigurationError } from './errors.js'; +import { isNonInteractiveOutput } from './interactive.js'; import { logger } from './logger.js'; /** @@ -21,8 +22,11 @@ import { logger } from './logger.js'; * @throws ConfigurationError if setup is incomplete or credentials are invalid */ export async function getCodemieClient(quiet = false): Promise { + // ora writes to stderr, so gate on stderr, not stdin (EPMCDME-14148). + const showProgress = !quiet && !isNonInteractiveOutput(); + let spinner; - if (!quiet) { + if (showProgress) { spinner = ora('Loading configuration...').start(); } diff --git a/tests/helpers/cli-runner.ts b/tests/helpers/cli-runner.ts index e62b979c4..752280b23 100644 --- a/tests/helpers/cli-runner.ts +++ b/tests/helpers/cli-runner.ts @@ -11,6 +11,12 @@ export interface CommandResult { output: string; exitCode: number; error?: string; + /** + * Killed by the `timeout` option rather than exiting on its own. execSync + * reports ETIMEDOUT with `status: null`, which would otherwise collapse to + * exitCode 1 and let a hang masquerade as a clean failure. + */ + timedOut?: boolean; } export class CLIRunner { @@ -54,6 +60,7 @@ export class CLIRunner { output, exitCode: error.status || 1, error: errorOutput, + timedOut: error.code === 'ETIMEDOUT', }; } } diff --git a/tests/integration/cli-commands/non-interactive-auth.test.ts b/tests/integration/cli-commands/non-interactive-auth.test.ts new file mode 100644 index 000000000..2ae274be4 --- /dev/null +++ b/tests/integration/cli-commands/non-interactive-auth.test.ts @@ -0,0 +1,66 @@ +/** EPMCDME-14148 — non-interactive SSO failure must exit cleanly, end to end. */ + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { mkdtempSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { CLIRunner } from '../../helpers/cli-runner.js'; +import { ssoCleanEnv } from '../../helpers/sso-auth.js'; + +const EXPECTED_MESSAGE = + 'SSO authentication required. Please run "codemie setup" with SSO provider first.'; + +describe('non-interactive SSO auth failure', () => { + const runner = new CLIRunner(); + let isolatedHome: string; + let result: ReturnType & { combined: string }; + + beforeAll(() => { + // Empty home = "no valid SSO session", without touching the real ~/.codemie. + isolatedHome = mkdtempSync(join(tmpdir(), 'codemie-14148-')); + + const startedAt = Date.now(); + const raw = runner.runSilent('sdk assistants list', { + // ssoCleanEnv strips CODEMIE_*: ConfigLoader.loadFromEnv reads + // CODEMIE_PROVIDER/API_KEY/URL directly, so an empty home alone does not + // guarantee "no valid SSO session" when run from a CodeMie agent shell. + env: { ...ssoCleanEnv(), CODEMIE_HOME: isolatedHome }, + // 'ignore' stdin is not a TTY — the condition under test. + stdio: ['ignore', 'pipe', 'pipe'], + // execSync blocks the worker, so Vitest's testTimeout cannot interrupt a hang. + timeout: 15_000, + }); + + // Without these two, a hang satisfies every assertion below — including the + // one named for it: ETIMEDOUT yields status null, which collapses to exit 1, + // and stderr already holds what was printed before the block. + expect(raw.timedOut ?? false).toBe(false); + expect(Date.now() - startedAt).toBeLessThan(5_000); + + result = { ...raw, combined: `${raw.output}\n${raw.error ?? ''}` }; + }); + + afterAll(() => { + rmSync(isolatedHome, { recursive: true, force: true }); + }); + + it('exits non-zero instead of hanging on a re-authentication prompt', () => { + expect(result.exitCode).not.toBe(0); + }); + + it('names the remediation the user should run', () => { + // Verbatim: a loose /codemie setup/ also matches other failure paths. + expect(result.combined).toContain(EXPECTED_MESSAGE); + }); + + it('sends the diagnostic to stderr and keeps it off stdout', () => { + expect(result.error ?? '').toContain(EXPECTED_MESSAGE); + expect(result.output).not.toContain(EXPECTED_MESSAGE); + }); + + it('does not print a raw stack trace', () => { + expect(result.combined).not.toMatch(/^\s+at\s+/m); + expect(result.combined).not.toContain('ConfigurationError:'); + expect(result.combined).not.toMatch(/Node\.js v\d/); + }); +});