-
Notifications
You must be signed in to change notification settings - Fork 91
fix(cli): exit cleanly on non-interactive SSO auth failure (EPMCDME-14148) #532
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
SleepySML
wants to merge
16
commits into
codemie-ai:main
Choose a base branch
from
SleepySML:EPMCDME-14148
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
16 commits
Select commit
Hold shift + click to select a range
d1872cb
fix(cli): exit cleanly on non-interactive SSO auth failure
SleepySML d77128a
fix(cli): add process-level error guards and document non-interactive…
SleepySML 7ebcfb2
fix(cli): address code review findings CR-001 through CR-005
SleepySML db9d403
docs(cli): add EPMCDME-14148 planning artifacts and correct AC4 record
SleepySML d90576c
fix(cli): install guards per entrypoint and make the hang test able t…
SleepySML 9c243a7
docs(cli): record check-round verdict and close review residuals
SleepySML 36dd759
test(cli): tighten hang detection and drop the false-positive disjunct
SleepySML 09bcc53
docs(cli): add QA gate report for EPMCDME-14148
SleepySML 2fc88c9
docs(cli): record actual complexity for EPMCDME-14148
SleepySML bb306fc
docs(cli): re-run QA gates after rebase onto d7097a2a
SleepySML 22ae3fb
fix(utils): gate the spinner on the output stream, not stdin
SleepySML 8b1a9fe
docs(cli): supersede the AC4 reproduction recipe with measured evidence
SleepySML 7a892df
docs(cli): verify AC4 - prompt is reachable, no readline crash
SleepySML 85fd48a
docs(cli): add final acceptance verification - all four criteria pass
SleepySML 95aeb87
refactor(cli): cut comment volume in the EPMCDME-14148 diff
SleepySML cc6939a
refactor(cli): apply reuse, simplification and efficiency findings
SleepySML File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
76 changes: 76 additions & 0 deletions
76
...rs/tasks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/ac4-investigation.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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. |
74 changes: 74 additions & 0 deletions
74
...ks/2026-09-03-epmcdme-14148-non-interactive-sso-hang/acceptance-verification.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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. |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's not the best option to integrate ot each js. consider different approach for implementaiton. it hsouldn't be just methon in each JS