Skip to content

fix(auth): quiet interactive signup output - #57

Merged
justinhelmer merged 2 commits into
mainfrom
fix/quiet-signup-output
Aug 20, 2026
Merged

fix(auth): quiet interactive signup output#57
justinhelmer merged 2 commits into
mainfrom
fix/quiet-signup-output

Conversation

@justinhelmer

@justinhelmer justinhelmer commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

From live install-flow testing (screenshots in the onboarding thread): interactive email sign-in dumped the raw API envelope — full user object, the session token in plaintext, and the landing table — plus a stray dim undefined under the terms gate, and the CLI's own Next-steps box mid-install.

  • Raw envelopes print only in JSON mode (piped or --output json), where they are the data contract for scripts — the token still reaches pipes. A terminal now gets Signed in as <email>.
  • promptEnter uses a Continue/Cancel confirm instead of p.text: clack's text prompt renders a dim undefined as its final value on an empty submit (verified against @clack/prompts in a PTY; defaultValue fixes the return value but not the render). Enter still continues; Ctrl-C or Cancel still aborts.
  • Next-steps box suppressed under the installer (installer sets POLYLANE_HINTS=0` (the hints capability from feat(config): hints — a first-class switch for next-step guidance #58)) — the installer owns the post-sign-in journey.
  • Behavior fix: the existing-verified-account path never persisted a default workspace, which is why the installer died at credential_mint for a re-authenticated account (no config.json at all). It now runs the same persistDefaultWorkspace as the fresh-verification path.

288/288 tests, typecheck and lint clean.

🤖 Generated with Claude Code

Raw envelopes (user object, session token, landing) now print only in
JSON mode where they are the scripting contract; a terminal gets a
clean 'Signed in as <email>'. promptEnter switches from p.text (which
renders a dim 'undefined' on empty submit) to a Continue/Cancel
confirm. The next-steps box is suppressed under the installer, which
owns that journey. The existing-verified-account path now persists a
default workspace like the fresh path, so the installer's key mint no
longer dies on re-authenticated accounts.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified this against the PR head (7109dd8) by running it, not just reading the diff — repro'd all four claimed fixes live:

  • Text mode no longer leaks token/user object/undefined — confirmed emitResult() (src/commands/auth/signup.ts:201-206) gates all 7 former formatOutput call sites behind config.output === 'json'. Ran a live repro against the existing-verified-account (re-auth) path with output: 'text': no token, no raw user object, no undefined in output.
  • "Signed in as <email>." confirmed printed (signup.ts:225, :311).
  • Next-steps box suppressed under installerunderInstaller() (signup.ts:208-213) checks POLYLANE_ONBOARDING_RUN; confirmed live: box appears without the env var, disappears with it set, while "Signed in as ..." still prints either way.
  • Workspace persistence fix for re-authenticated accounts — confirmed via git show 36c6b0f:src/commands/auth/signup.ts that the pre-PR existing-verified-account branch never called persistDefaultWorkspace; the PR head adds it (signup.ts:301). Confirmed live: config.json now gets workspace_id written on that branch, which it didn't before. persistDefaultWorkspace is wrapped in try/catch (non-fatal) and its inner selectWorkspace short-circuits under non-interactive mode, so no regression risk to scripted/agent invocations.
  • CI green (3/3), and I independently reproduced clean npm ci / codegen / typecheck / lint / full test suite (283/283, 0 fail) locally against this head.
  • No leftover debug prints found (console.log/console.error grep across src/ is empty); the verbose HTTP logger already redacts Authorization/x-api-key and never logs response bodies, unaffected by this change.

One real gap I'd like addressed before merge: none of this PR's own behavioral claims are covered by its test diff. The only test change is an assertion-string update (test/signup.test.ts:145). Specifically missing:

  1. A output: 'text' test asserting the token/user object/undefined do NOT appear in output for the existing-verified-account path (the exact regression this PR fixes).
  2. A test for "Signed in as <email>." appearing.
  3. A test for underInstaller() / POLYLANE_ONBOARDING_RUN suppressing the Next-steps box.
  4. A test asserting persistDefaultWorkspace/config.json.workspace_id is now written on the existing-verified-account re-auth branch — the current persistence test (test/signup.test.ts:226-254) only covers the --code (first-time) path, which already worked before this PR.

Given the whole point of this PR is fixing a silent session-token leak, I'd rather see it locked down with tests than rely on manual re-verification next time something touches this file. Happy to re-review once those land — the fix itself is correct and I found no functional regressions.

Non-blocking follow-up (pre-existing, not introduced by this PR): JSON-mode/scripted output is still polluted by clack's intro/note/outro writing directly to stdout regardless of config.output (confirmed in node_modules/@clack/prompts, and confirmed this predates this PR via the base commit). Worth a separate follow-up given the PR's own framing of JSON as "the data contract for scripts."


Generated by Claude Code

Covers the review's four asks: no token/user-object/undefined in text
mode, the JSON envelope still emitted for scripts, workspace_id
persisted on re-auth, and the next-steps box suppressed under the
installer.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@justinhelmer

Copy link
Copy Markdown
Contributor Author

Added the requested test coverage — new auth signup existing-account re-auth suite covering all four asks:

  1. text mode: no tok_signup, no raw user object, no undefined, and Signed in as <email>. present
  2. JSON mode still emits the full envelope (token + user) for scripts
  3. config.json.workspace_id persisted on the re-auth branch (the exact gap behind the installer's credential_mint failure)
  4. next-steps box printed standalone, suppressed with POLYLANE_ONBOARDING_RUN set (env cleaned up so it can't leak into other suites)

292/292 passing, typecheck + lint clean. Agreed on the clack intro/note/outro-in-JSON-mode follow-up being separate — happy to pick it up next.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM: round-2 adds real test coverage for all four round-1 asks, and everything is green.

Verified against head ded76ab90764ecb82e5ee8f869f1879b7ec3a08f (fetched fresh via git fetch origin pull/57/head, confirmed git rev-parse HEAD matches the PR API's head SHA).

Diff since the round-1 fix (7109dd8): purely additive — +76/-0 in test/signup.test.ts only. No production code changed this round, so the previously-verified fixes in src/commands/auth/signup.ts and src/utils/prompt.ts are untouched.

New test suite (auth signup existing-account re-auth, test/signup.test.ts:210-282) — checked each assertion is real, not a smoke test:

  • Text mode: asserts !output.includes('tok_signup'), !output.includes('emailVerified'), !output.includes('undefined'), and output.includes('Signed in as dev@acme.com.') — exercises the exact user.emailVerified branch in emailSignup (signup.ts:299-312).
  • JSON mode: asserts the envelope (tok_signup + emailVerified) IS present, confirming emitResult only gates in text mode, not silently dropping data everywhere.
  • persistDefaultWorkspace: asserts config.json's workspace_id actually gets written on the re-auth branch specifically (signup.ts:308, the new await persistDefaultWorkspace(config) call) — this is the exact line that fixes the installer's credential_mint failure from round 1.
  • Next-steps suppression: asserts 'Onboarding (in order)' present standalone and absent with POLYLANE_ONBOARDING_RUN set, with the outro message still shown either way, and the env var is cleaned up in beforeEach/finally so it can't leak into other suites.

Grepped all callers of the changed helpers (emitResult, underInstaller, persistDefaultWorkspace, promptEnter, finishEmailSignIn) — emitResult/underInstaller are private to signup.ts with no other call sites to break; persistDefaultWorkspace's other call site (the --code verification path, signup.ts:222) is already covered by the pre-existing auth signup --code describe block. promptEnter's p.confirm({ active, inactive }) shape matches @clack/prompts' actual ConfirmOptions type (checked node_modules/@clack/prompts/dist/index.d.ts) and returns Promise<boolean | symbol>, consistent with the result === false check.

Local checks (fresh run, not trusting CI or the PR body's stated numbers):

  • npm ci — clean, 0 vulnerabilities
  • npm run codegen — succeeded (277 paths, 168 types, 350 operations)
  • npm run typecheck — clean, no errors
  • npm run lint — clean, no errors (src/ + codegen/)
  • npm run test292 tests, 292 pass, 0 fail, 0 cancelled (matches the author's comment claim exactly; the PR description's stale "288/288" is from before this round's test-only commit)

CI: all 3 check runs green (Test & typecheck on Node 20.x/22.x/24.x), which also cover the npm run build + dist smoke-test leg that I didn't need to run locally since this diff doesn't touch build.ts/codegen//skill/SKILL.md/packaging.

No regressions found. The pre-existing clack intro/note/outro-in-JSON-mode noise remains a known, separate follow-up (author acknowledged it in their comment) and is not part of this PR's scope.


Generated by Claude Code

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Auto-approved: Claude reviewed this PR and posted an LGTM verdict (see its review). A repo admin enabled this via the auto-approve workflow.


Generated by Claude Code

@justinhelmer
justinhelmer merged commit 0299079 into main Aug 20, 2026
4 checks passed
@justinhelmer
justinhelmer deleted the fix/quiet-signup-output branch August 20, 2026 17:13
@justinhelmer

Copy link
Copy Markdown
Contributor Author

Updated to build on #58: the next-steps suppression now reads config.hints instead of piggybacking on POLYLANE_ONBOARDING_RUN (an attribution channel shouldn't double as a UX switch). underInstaller() is gone; the run id is attribution-only again. Merge #58 first — this branch contains its commits. The installer-side POLYLANE_HINTS=0 lands in a polylanedotcom follow-up. 295/295 tests.

@justinhelmer

Copy link
Copy Markdown
Contributor Author

Correction to my earlier comment: this PR merged before the refactor landed, so the underInstallerhints switch moved into #58 (which is rebased on this merge). The stale fix/quiet-signup-output branch has been deleted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant