feat(setup): default into GitHub login when setup finishes logged-out on a TTY

[tonychang04]

Tony's ask: the one command should carry the whole journey, curl-installer style, and "default login if not logged in makes sense".

After the ✓ checkmarks, when setup ran on an interactive human terminal with no stored session, it now flows straight into insta login --oauth github — Enter continues into the browser, n skips. shouldOfferLogin (pure, tested) gates it:

• -y runs, piped stdin/stdout, agents, CI: never prompted (a browser OAuth flow can't work there; they keep the printed next: hint, and prompt.md walks agents through login as its own step, relaying the sign-in link to the human).
• Already logged in: straight to the next: insta project create hint.
• Declined prompt or failed browser flow: best-effort — setup already succeeded, prints the manual hint, never an error, exit code untouched (tested).

The flow is injected (LoginFlow) so tests never touch a real TTY or browser. Gate: typecheck clean, 410/410 tests (4 new).

Ships in 0.0.39 (0.0.38 = #116 alone, releasing now via #118 — kept separate so the staging fix isn't held by this review).

🤖 Generated with Claude Code

---

Summary by cubic

Defaults setup into GitHub OAuth login when it finishes on an interactive TTY without a session, instead of only printing a manual next step. This reduces first-run friction while keeping non-interactive and already-logged-in behavior unchanged.

• Adds pure gate shouldOfferLogin(yes, loggedIn, stdinTty, stdoutTty); when true, prompts “log in now with GitHub? (Y/n)” and runs insta login --oauth github on Enter.
• Non-TTY and -y runs never prompt; logged-in users go straight to the project-create hint. EOF (Ctrl-D) counts as decline. Declined or failed browser login is best-effort: setup still succeeds, prints the manual hint, and points SSH users to insta login --device.
• With --mcp-token, re-runs MCP token registration after a successful interactive login so the token mints with the new session; tests pin that registration happens exactly once and only after login.
• Injects LoginFlow (ask/login/stdinTty/stdoutTty) with a readline-based default; adds tests for gating, TTY flow, decline/failure, non-TTY cases, and the --mcp-token contract.

^{Written for commit c4ee1f0. Summary will update on new commits.}

[jwfing]

Review: default into GitHub login when setup finishes logged-out on a TTY

Summary: Clean, tightly-scoped enhancement that flows an interactive, logged-out setup agent straight into insta login --oauth github behind a pure, well-tested gate; behavior for -y/non-TTY/already-logged-in paths is preserved and the login step is genuinely best-effort.

Requirements context: No /docs/superpowers/ (or docs/specs/) directory exists in this repo — assessing against the PR description and Tony's stated ask ("the one command should carry the whole journey… default login if not logged in makes sense") alone.

---

Findings

Critical

(none)

Suggestion

• Software engineering — defaultAsk is untested (src/commands/setup.ts:387-393). All four new tests inject a fake ask, so the real readline-based prompt never executes under test. It's thin, so this isn't blocking, but it's the one piece of new production code with zero coverage. Since shouldOfferLogin already guarantees both stdin and stdout are TTYs before defaultAsk runs, the risk is low — noting it for completeness.
• Functionality — defaultAsk can hang on EOF (src/commands/setup.ts:387-393). rl.question(...) resolves only on a line of input; if the user sends Ctrl-D (EOF) at the prompt, the wrapped promise never resolves and setup appears to hang after the ✓ checkmarks. Consider resolving to a decline on the interface 'close' event (rl.on('close', () => resolve(true /* treat as decline */))). Interactive-only, low blast radius.

Information

• Test conflates two gates (test/setup-agent.test.ts:139-145). The 'non-TTY (agents/CI) never prompts' case sets both yes: true and stdinTty:false/stdoutTty:false, so it doesn't isolate the TTY gate through setupAgent. The pure gate is fully covered by the shouldOfferLogin unit test above it, so this is fine as-is; a yes:false + non-TTY variant would exercise the non--y TTY path end-to-end.
• Env correctness (verified, no change needed) — src/commands/setup.ts:410. The default login: () => loginOauth('github', {}) passes empty opts, but this is correct: envUse has already persisted (and dropped any foreign session for) the target env at line 423, so loginOauth → ApiClient.load() resolves the right host, including under setup agent --env staging. loggedIn is also (correctly) recomputed from readStored() after the switch, so a session dropped by the env switch re-triggers the prompt as intended. The adjacent comment already documents the prod-hosted prompt.md rationale; a one-liner noting the "env already persisted" invariant here would help future readers.

---

Dimension coverage

• Software engineering: shouldOfferLogin is pure and exhaustively unit-tested; the injected LoginFlow seam keeps tests off real TTY/browser. Import style (node:readline, ./auth.js) and error handling match the file. Only gap is the untested defaultAsk (Suggestion).
• Functionality: Solves the stated ask. -y/piped/CI never prompt (a loopback OAuth flow can't work there anyway), already-logged-in short-circuits, decline (^n, default-Enter = yes) and browser-flow failure are both swallowed with the manual hint and no exit-code change — matching the "setup already succeeded" contract. Only edge is EOF-at-prompt (Suggestion).
• Security: No new user input reaching SQL/shell/HTTP. The catch surfaces e.message only — loginOauth's errors are generic strings (oauth failed: …, state mismatch, timed out), no token/PII leakage. No auth checks weakened; no new dependencies.
• Performance: N/A — one conditional readline prompt on an interactive terminal, no hot path, no I/O regressions.

---

Verdict: approved (informational)

No Critical findings — the two Suggestions and Information notes are non-blocking. Posting as a COMMENT; a human still gives the explicit GitHub approval via the approve flow.

[cubic-dev-ai]

All reported issues were addressed across 2 files

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[jwfing]

Summary
The setup login flow mostly matches the intended human-terminal path, but it does not enforce the PR’s explicit “agents/CI never prompted” requirement outside of non-TTY detection.

Requirements context
I used the PR title/description as the primary behavior spec: after successful setup agent, an interactive logged-out human terminal should default into insta login --oauth github, while -y, piped/non-TTY, agent, and CI runs should keep the manual next: hint and never prompt. Existing README/docs confirm setup agent is the one-command agent onboarding path and login --oauth github is the browser login flow; I did not find a separate linked issue/spec in the checked-out repo.

Findings

Critical

• src/commands/setup.ts:375-380, src/commands/setup.ts:463-466 — Critical: shouldOfferLogin only gates on yes, stored-login state, and stdin/stdout TTY. That means CI=true insta setup agent or an agent-run command with a pseudo-TTY can still reach loginFlow.ask(...) and then launch browser OAuth, despite the PR description explicitly saying agents and CI are never prompted. The repo already treats AI_AGENT as an agent-mode signal for subprocess behavior (src/ensure-skills.ts:22-27), so this gate needs an injected CI/agent/non-human signal, plus tests for CI=true and/or AI_AGENT, before prompting.

Suggestion

• test/setup-agent.test.ts:121-133 — The test name says declined and failed login print the manual hint, but the assertions only check that login is skipped/swallowed and process.exitCode is unchanged. Capturing output or factoring the hint string would make that promised fallback behavior regression-proof.

Information

• No security-relevant changes found: this prompts locally and calls the existing OAuth path; I did not see new token logging, authorization weakening, new dependencies, or unvalidated SQL/shell flows.
• No performance-relevant changes found: this is a post-setup interactive path with no hot-loop, N+1, or large allocation concerns.

Verdict
REQUEST_CHANGES. The critical issue is the missing agent/CI suppression in the new login prompt gate. Local verification note: npm run typecheck could not run in this checkout because tsc is not installed.

[jwfing]

Review — feat(setup): default into GitHub login on a logged-out TTY

Summary: After the setup checkmarks, an interactive human terminal with no stored session now flows straight into insta login --oauth github behind a pure, tested shouldOfferLogin gate; non-TTY / -y / already-logged-in paths are unchanged. Clean, in-scope, well-tested — no blocking issues.

Requirements context: No /docs/superpowers/ (or any docs/specs/) directory exists in this repo — assessed against the PR description and Tony's stated ask alone. Intent ("the one command carries the whole journey; default login if not logged in") is faithfully implemented.

Findings

Critical — (none)

Suggestion

• Functionality / UX — src/commands/setup.ts:468: the gate treats "stdin & stdout are TTYs" as "can run a browser OAuth flow", but that isn't true over SSH. On a remote-but-interactive shell the loginOauth('github') loopback flow (callback on 127.0.0.1 of that box) can't complete, so the user eats a failed browser attempt before the best-effort catch prints the manual hint. It's handled gracefully (setup already succeeded, exit code untouched — good), but the failure message at src/commands/setup.ts:477 doesn't point at the actual escape hatch. Consider mentioning insta login --device in that catch message (the manual next: hint already does), so a headless-over-SSH user is nudged toward the flow that works. Non-blocking.

Information

• Testing — src/commands/setup.ts:386-396: defaultAsk (the only un-injected real-IO piece, including the EOF/Ctrl-D → decline branch) isn't exercised by a unit test since loginFlow.ask is injected everywhere. The four new tests cover the gate, the TTY flow, decline/failure best-effort, non-TTY, and the --mcp-token-after-login ordering — solid coverage of the branching. The readline EOF branch is simple enough that this is fine to leave; just noting the one gap.
• Correctness (verified, no action) — the --mcp-token ordering is right: the pre-login registerMcp (setup.ts:455) is a safe no-op while logged out because defaultMinter returns null (checks accessToken, swallows errors) rather than throwing, so it prints the login hint and returns; the post-login re-run at setup.ts:474 then mints and registers. No crash risk on the logged-out mint path.
• Style (pre-existing, out of scope) — setup.ts:420-422 prints "non-interactive shell — assuming -y" without actually setting opts.yes = true; harmless here since the new login gate keys off stdoutTty independently.

Dimension notes

• Software engineering: TDD honored — 4 new tests added alongside the behavior; injection (LoginFlow) keeps tests off real TTY/browser. Matches repo conventions (.js import specifiers, info() for output, pure-function + injected-effect split).
• Functionality: Solves the stated ask; edge cases (EOF, decline, browser failure, --mcp-token requiring a session first) are all handled. See the SSH suggestion above for the one soft edge.
• Security: No new user input reaches SQL/shell/HTTP unsanitized (answer is only regex-tested); no secrets logged — the catch prints e.message from OAuth/network errors, which don't carry the token. No auth checks weakened. No new dependencies (node:readline is stdlib).
• Performance: N/A — a single one-shot readline prompt on an interactive path; no loops, queries, or hot-path work.

Verdict

approved (informational; posted as a COMMENT — the green-check approval remains a separate human action). Zero Critical findings; the one Suggestion and the Information notes are non-blocking.

[jwfing]

Review: feat(setup) default into GitHub login on a logged-out TTY

Summary: Clean, well-scoped change that makes insta setup agent flow straight into insta login --oauth github when it finishes on an interactive terminal with no session, gated by a pure/tested predicate and with a best-effort failure posture — no correctness, security, or performance problems found.

Requirements context: No /docs/superpowers/ (or docs/specs/) directory exists in this repo — assessed against the PR description, the cubic summary, and the surrounding code alone. Intent ("Tony's ask: the one command should carry the whole journey… default login if not logged in makes sense") is taken from the PR body.

Findings

Critical

(none)

Suggestion

• Software engineering — test doesn't model the logged-out pre-login mint (test/setup-agent.test.ts:151-166): In the --mcp-token test the injected mint returns insta_tok unconditionally, so the pre-login registerMcp call (src/commands/setup.ts:455) also mints and would add the MCP server. In production defaultMinter returns null while logged-out (setup.ts:304), so the pre-login add is correctly skipped and only the post-login add runs. The test still passes because it only asserts a mint+add occur after login, but it therefore wouldn't catch a regression that double-registers. Consider a mint mock that returns null until login has run, and asserting no mcp/mint events occur before loginAt — that pins the actual production contract (register exactly once, after the session exists).

Information

• Software engineering — defaultAsk is not unit-tested (src/commands/setup.ts:386-396): The readline/EOF branch is the one piece exercised only via the injected default (all setupAgent tests supply their own ask). It's thin and its EOF-as-decline behavior is well-commented, so this is acceptable; noting it only for completeness. Behavior looks correct: rl.on('close', …→'n') resolves EOF as a decline, and empty Enter → !/^n/i → proceed, matching the (Y/n) default.
• Functionality — post-login re-registration is correctly ordered (setup.ts:468-479): Verified the sequence for --mcp-token, logged-out, accept-prompt: pre-login registerMcp(…, true) short-circuits on a null mint (setup.ts:327-329) before any mcp add, then post-login registerMcp(run, mint, true) re-loads the freshly-persisted session via defaultMinter→ApiClient.load() and adds once. loginOauth('github', {}) persists into the same env already pinned earlier by planSetupEnv/envUse, so the session lands in the right config. No double-registration in production.

Dimension coverage

• Software engineering: Good — 4 new behavioral tests plus a pure-predicate test; DI pattern (LoginFlow) is consistent with the file's existing seams (run, mint, readStored, switchEnv); import style (./auth.js) and info(...) messaging match conventions. New optional loginFlow param is last, so existing callers are unaffected.
• Functionality: Solves the stated ask. Handles logged-in (early return), non-TTY/-y/piped (never prompts), decline, EOF/Ctrl-D, browser-flow failure (swallowed, exit code untouched), and remote/SSH guidance toward --device. No gaps found.
• Security: No security-relevant changes — no new user input reaches SQL/shell; the bearer token comes from mint() (not user input) and is not logged; the failure message surfaces only the error text, not the token. Auth checks unchanged.
• Performance: No concerns — readline is created only on the interactive path; no loops, queries, or blocking I/O added to any hot path.

Verdict

approved (informational — posted as a comment; explicit GitHub approval is a separate human action). Zero Critical findings; the Suggestion/Information items are optional polish.

[jwfing]

Summary
The PR cleanly adds a best-effort interactive GitHub OAuth prompt after successful insta setup agent when no stored session is present, without changing the non-interactive or already-logged-in paths.

Requirements context
I used the PR title/description as the primary intent: after setup succeeds on an interactive human TTY with no stored session, offer insta login --oauth github; skip prompting for -y, non-TTY/agent/CI paths, and already-logged-in users; preserve success exit behavior on decline or login failure; rerun MCP token registration after successful interactive login when --mcp-token was requested. Local README/setup/auth docs confirm setup agent installs agent skills and MCP, while login is normally insta login --oauth github; CONTRIBUTING and .claude/skills/developing-insta-cli/SKILL.md confirm the Node/TS/commander style, injected side-effect test pattern, and npm run typecheck && npm test gate. I did not find a local linked issue or local skills/insta/cli-reference.md; this is a behavior change rather than a command/flag surface change.

Findings
Critical

(none)

Suggestion

(none)

Information

(none)

Verdict
Approved per the rubric: no Critical findings. Software engineering: the LoginFlow injection follows existing side-effect isolation conventions, and tests cover gating, TTY login, decline/failure, non-TTY behavior, and post-login --mcp-token registration. Functionality: the implementation matches the described best-effort flow; security: no new dependency, shell, SQL, or secret exposure issues found; performance: no hot-path or unbounded work introduced. Verification note: I inspected code and tests only, and did not run npm run typecheck && npm test because the request specified read-only/non-mutating review.

[jwfing]

Summary
No critical issues found; the change is narrow, test-covered, and follows the repo's setup-command dependency-injection style.

Requirements Context
I used the PR title/description and public PR page (#119), plus repo docs. README.md:28-45 documents setup agent as the one-command agent setup path, README.md:77-89 documents browser OAuth, and .claude/skills/developing-insta-cli/SKILL.md:17-18 calls out injected side effects as the local testing convention. I did not find a linked issue or more specific design doc; this PR does not add or rename commands/flags, so the cli-reference mirror requirement in .claude/skills/developing-insta-cli/SKILL.md:36-38 does not appear to apply.

Findings

Critical
(none)

Suggestion

• Functionality: src/commands/setup.ts:455-474 - In a logged-out interactive --mcp-token run, setup first prints the existing “MCP not registered (--mcp-token needs a login)” hint, then prompts for login, then may successfully register MCP after login. That is functionally best-effort, but noisy in the happy path. Consider deferring token-backed MCP registration until after the interactive login attempt when shouldOfferLogin is true, or suppressing the pre-login hint for that path.
• Functionality: src/commands/setup.ts:379-380 - The “human terminal” gate is purely stdinTty && stdoutTty. If “agents/CI never prompt” is intended literally for PTY-backed automation, a CI or agent run with allocated TTYs and no -y could still prompt and start browser OAuth. Non-blocking because the installer agent path passes -y and typical CI is non-TTY, but an env-level guard or doc wording could make the contract sharper.

Information

• Software engineering: test/setup-agent.test.ts:99-160 - The added tests cover the pure gate, TTY accept path, decline/failure behavior, non-TTY behavior, and post-login MCP token retry. The injected LoginFlow matches the repo convention for side-effectful code.
• Security: src/commands/setup.ts:469-478 - No new SQL/shell execution path, dependency, or auth weakening. Prompt input is only used as a yes/no decision, and OAuth remains delegated to the existing loginOauth path.
• Performance: src/commands/setup.ts:468-475 - No performance concern; the added work is a single prompt and optional OAuth/MCP retry after setup completes, not a hot path.

Verdict
Approved per rubric: zero Critical findings. I did not run npm run typecheck or npm test because the review instructions were read-only, so this verdict is based on source and test inspection.

[jwfing]

Review: feat(setup): default into GitHub login when setup finishes logged-out on a TTY

Summary: A tightly-scoped, well-tested UX improvement — after the ✓ checkmarks, an interactive logged-out session now flows straight into insta login --oauth github, gated by a pure shouldOfferLogin, with all failure/decline paths kept best-effort. I found no blocking issues.

Requirements context: No /docs/superpowers/ (or docs/specs/, or any docs/) directory exists in this repo — assessing against the PR description alone. Intent as stated: carry the one-command journey through the single genuinely-human step (browser auth) on interactive terminals, while leaving agents/CI/-y/already-logged-in behavior unchanged. The implementation matches that intent.

---

Critical

(none)

---

Suggestion

(none blocking; see Information.)

---

Information

Software engineering

• Good test coverage for the changed behavior: test/setup-agent.test.ts:99-171 adds 5 assertions across the pure gate, the TTY happy path, decline + failed-login (exit code preserved), the non-TTY gate, and the --mcp-token post-login re-registration ordering. All four shouldOfferLogin false-branches are exercised. The injected LoginFlow keeps tests off a real TTY/browser — consistent with the existing Runner/TokenMinter injection style in this file.
• The defaultAsk readline path (src/commands/setup.ts:386-396) is the one piece exercised only by inspection (it's injected away in tests). It reads correctly: EOF (close) resolves to 'n' before the question is answered, and the post-answer rl.close() re-fires close but the Promise is already settled, so the answer wins. Enter (empty) → !/^n/i.test('') → proceed; n/no/N → decline. No action needed — just noting where the coverage boundary sits.

Functionality

• --mcp-token ordering is correct: the pre-login registerMcp(run, mint, true) at src/commands/setup.ts:455 mints null while logged out and adds nothing (printing the "needs a login" hint), then the post-login registerMcp(run, mint, true) at :474 mints and adds exactly once. Verified against registerMcp (:317-340) and the test at :145-171.
• loginOauth('github', {}) matches its signature (src/commands/auth.ts:40) and throws on any failure, so the catch at :476-479 reliably degrades to the manual hint with the exit code untouched.
• Remote/SSH note: on an SSH box both stdio are TTYs, so shouldOfferLogin returns true and the browser flow is attempted. It degrades correctly — browserOauth rejects after a 2-minute timeout (src/commands/auth.ts:192), which is caught and surfaces the insta login --device hint. Worth being aware that a remote user can wait up to 2 min after a successful setup before that hint appears; this is pre-existing insta login behavior, not introduced here, so no change requested.

Security

• No security-relevant regressions. Provider is hard-coded ('github'), no new user input reaches shell/SQL/HTTP, and no secrets are logged — the catch prints e.message (a timeout/network string), and loginOauth only echoes the account email/id on success, not the token.

Performance

• Negligible. One extra readline prompt plus the existing OAuth flow on the interactive path only. The readStored() double-read (:427 and :462) is pre-existing local-file I/O, not introduced by this PR.

---

Verdict

approved (informational — the human still gives the explicit GitHub approval). Zero Critical findings; the change is correct, in-scope, and well-tested. Posting as a COMMENT per the review policy.

[jwfing]

LGTM - approved.

[jwfing]

Summary
No blocking issues found; the change is focused, tested around the new setup/login gate, and does not introduce security or performance concerns.

Requirements context
I used the PR title/description at #119 as the primary intent: after successful interactive setup agent, offer GitHub OAuth login only when there is no stored session, while preserving -y, non-TTY, already-logged-in, decline, and failed-login behavior. Local context checked: README.md:28-45 for setup expectations, README.md:77-89 for OAuth behavior, and .claude/skills/developing-insta-cli/SKILL.md:17-18 for the repo's dependency-injection testing convention. I did not find a separate design/spec document for this exact default-login behavior.

Findings

Critical

(none)

Suggestion

(none)

Information

(none)

Verdict
Approved per the requested verdict rule: no Critical findings. Software engineering: the pure gate and injected LoginFlow fit existing testability conventions, with tests covering TTY, -y, non-TTY, decline/failure, and --mcp-token retry paths. Functionality: the implementation matches the PR description in the reviewed paths. Security: no new dependency, shell construction, token logging, SQL, or auth weakening. Performance: this is a one-shot setup prompt plus an optional second MCP registration, not a hot path.

Verification: git diff --check main...HEAD passed. npm run typecheck could not run because tsc is not installed in this checkout; I did not install dependencies or run npm test because this review was explicitly read-only/no-mutating-commands.