feat(cli): bgagent change-password + force-rotate on first login (#238) - #680
feat(cli): bgagent change-password + force-rotate on first login (#238)#680scottschreckengaust wants to merge 2 commits into
Conversation
✅ Acceptance summary (for the reviewer)#238 —
🔀 Merge guidance (for the reviewer)Cluster 🤖 orchestrator note (agent) — promotion is orchestrator-driven; merge remains a human action. |
isadeks
left a comment
isadeks
left a comment
There was a problem hiding this comment.
Principal review — PR #680 (feat(cli): bgagent change-password + force-rotate on first login)
1. Verdict
Request changes — one blocking correctness defect. The security intent is right and the Cognito primitives are the correct ones, but B1: the new first-login path can hang the CLI indefinitely, and it hangs precisely where the code comment claims it "surfaces a clear error rather than hanging." I reproduced the hang. Everything else is nits.
Credit where due: the ChangePassword-needs-an-access-token problem is solved the right way (re-auth to mint an in-memory token rather than persisting an access token), the Permanent: true removal is the actual security win the issue asked for, and the invite half-failure branch that this diff deletes really did become dead code — removing it is correct, not a regression.
2. Vision alignment — pass
Squarely on-tenet for VISION.md bounded blast radius: an admin-generated credential sitting in Slack scrollback forever is unbounded exposure; making it one-shot bounds it. No tenet is traded, no ADR needed. Correctly scoped to cli/ per the AGENTS.md routing table, and it does not touch the async fire-and-forget submit path. Consistent with ADR-016 — it stays inside the single shipped Cognito verification path and adds no second auth code path, which is the drift hazard ADR-016 §68 explicitly warns about.
Governance gate satisfied: #238 carries approved. Branch feat/issue-238-change-password matches the convention.
3. Blocking
B1 — bgagent login hangs forever on a first-login account when stdin is a pipe (cli/src/commands/login.ts:38)
login.ts:38 passes the prompt callback unconditionally, so the non-interactive guard at cli/src/auth.ts:111 is statically unreachable from the only caller. The PR body and the docstring at auth.ts:63 both claim --password non-interactive login "surfaces a clear error instead of hanging." It does not — nothing ever passes undefined, so the guard never fires and control flows straight into promptSecret.
When stdin is an open pipe with no further data (CI job, wrapper script, bgagent login --username u --password "$TEMP" inside automation), promptSecret's non-TTY reader parks a waiter on a line that never arrives. I reproduced this against the real prompt-secret.ts non-TTY logic and the real promptNewPasswordWithConfirmation body:
isTTY=false (bgagent login --username u --password TEMP, first-login user)
This account requires a new password on first login.
New password:
>>> RESULT: HANG — no error, no prompt answered, still waiting after 4s
The two adjacent stdin states fail differently but are also wrong, and neither is tested:
| stdin state | actual behavior |
|---|---|
| open pipe, no data (CI) | hangs indefinitely |
< /dev/null (EOF) |
New password cannot be empty. — blames the user for an automation-shape problem |
| one line piped | Passwords do not match. — the confirm read returns '' |
There is a second, subtler defect in the same flow. promptPassword in login.ts calls rl.close() before resolve(line), and rl.once('close') rejects with No password provided.. Because close is emitted synchronously inside rl.close(), the rejection wins the race and settles the promise first. Demonstrated with the exact source ordering:
--- real source order (rl.close() then resolve) ---
Password: → REJECTED "No password provided."
--- swapped (resolve then rl.close()) ---
→ RESOLVED "TempPass1!"
That is pre-existing (promptPassword is untouched by this diff) and it is masked today because piped login was single-line and never reached a second read. This PR is what makes the interactive-vs-piped distinction load-bearing, so it is the right PR to stop relying on it. Note also that login now mixes two different hand-rolled stdin readers in one command — readline in login.ts, then the raw-mode/piped reader in prompt-secret.ts. Two readers contending for one fd is inherently fragile; the confirm-read returning '' above is that fragility showing.
Suggested fix — make the guard reachable by gating the callback on actual interactivity, so automation gets the clear error the docstring already promises:
// login.ts
const interactive = process.stdin.isTTY && !opts.password;
await login(opts.username, password, interactive ? () => {
console.log('This account requires a new password on first login.');
return promptNewPasswordWithConfirmation();
} : undefined);Then add a test that pins it: challenge fires + non-interactive ⇒ throws the guard message, never reads stdin. The existing auth.test.ts case passes login(...) with no third argument, which no real caller does — it tests a shape the CLI cannot produce, which is why the reachability gap slipped through (AI005: asserting what the code does, not what it should).
One knock-on worth a line of docs: since invites no longer promote to permanent, invited users now sit in FORCE_CHANGE_PASSWORD under Cognito's default 7-day TemporaryPasswordValidityDays (never set anywhere in cdk/). A teammate who waits 8 days gets a bare NotAuthorizedException with no hint that the temp password merely expired and the admin must re-issue. Previously permanent passwords never expired, so this failure mode is new. Mapping that to "your temporary password expired — ask your admin to re-run bgagent admin invite-user" would close it.
4. Non-blocking nits
N1 — cli/README.md:355 still documents the old permanent-password behavior. It reads "invite-user creates the user, sets a permanent password…" — now false. cli/README.md also carries a per-command reference (### bgagent login at :64, ### bgagent admin invite-user … at :337) and change-password appears zero times in it. docs/guides/ was updated thoroughly; the CLI's own README was missed. Not a mirror-sync issue — cli/README.md is hand-maintained and outside the Starlight tree.
N2 — the invite file's column alignment is now visibly broken, and the docs show output the code does not produce. admin.ts:272 widens the label to temp password: (14 chars) while email: keeps its hardcoded 4-space pad. Rendering the real line-builder:
email: your-email@example.com ← actual
temp password: K9$mPq2nL!vXf3Hb
bundle: eyJhcGlfdXJsIjoi
The USER_GUIDE / Authentication.md sample (docs/guides/USER_GUIDE.md:99-101) claims a re-aligned block (email: …, bundle: …) that the code never emits. Either pad all three labels to width 14 or revert the docs sample. Per the repo's own "render the copy, don't read it" lesson, this is exactly the class of defect invisible in a source diff. No test asserts the invite-file content, so nothing catches it — cli/test/commands/admin.test.ts has no assertion on the password label or the new trailing line.
N3 — promptSecret trims, so passwords with leading/trailing whitespace are silently mangled. prompt-secret.ts applies .trim() on both the TTY and piped paths. change-password sends the trimmed value as ProposedPassword, so a user who sets " Str0ng!Pass " gets a different password stored than typed and cannot reproduce it. Pre-existing in promptSecret, newly load-bearing here because it now sets a credential rather than just reading one.
N4 — adminInviteUser is now a one-line passthrough (cli/src/cognito-admin.ts:167-172) whose 11-line docstring is longer than its body, and whose try/catch diagnostics were the only thing distinguishing it from adminCreateUser. Worth collapsing into the call site, or keeping it with a one-line comment. Related: adminSetPermanentPassword (cognito-admin.ts:140) is now reached only via adminResetPassword — the exported-symbol surface could shrink.
N5 — no command-layer test for the challenge path. cli/test/commands/login.test.ts:33 adds the ChallengeNameType mock but no test exercises it; the challenge is only covered at the auth.ts seam. The reachability defect in B1 lives precisely in the login.ts → auth.ts wiring that no test crosses — the repo's own "test the real seam, not a mock" lesson.
N6 — InvalidPasswordException → CliError mapping is duplicated verbatim at cli/src/auth.ts:135-137 and auth.ts:166-169. Two lines, but it is the user-visible policy message; worth one shared helper so the wording cannot drift.
5. Documentation
Updated well: docs/guides/USER_GUIDE.md and LINEAR_SETUP_GUIDE.md teammate-onboarding sections are genuinely rewritten, not patched — including the raw-AWS-CLI fallback, the email_verified / FORCE_CHANGE_PASSWORD interaction, and the narrowed IAM requirement (AdminCreateUser only, no AdminSetUserPassword). The service-account escape hatch is preserved as a blockquote rather than deleted. That is careful work.
Mirror sync — verified clean. I re-ran node docs/scripts/sync-starlight.mjs in the worktree; git status docs/ came back empty, so the generated Starlight mirror matches its sources and CI's mutation guard will not trip.
Gaps: N1 (cli/README.md stale + missing command) and N2 (docs sample ≠ rendered output). docs/design/IDENTITY_AND_AUTH.md documents no invite/login/password flow at all today, so nothing is stale there — no action needed.
6. Tests & CI
CI green. Coverage is genuinely good on the happy and error paths at the auth.ts seam — wrong current password (asserting ChangePassword is never reached), weak new password, no session, mismatch, empty. The cognito-admin test asserting expect(input).not.toHaveProperty('Permanent') is a well-chosen regression guard, and __command-discriminated mock dispatch is a real improvement over call-order brittleness.
Gaps: the three untested stdin shapes in B1, the missing command-layer challenge test (N5), and the unasserted invite-file rendering (N2).
Bootstrap synth-coverage: not applicable — verified zero cdk/ or bootstrap/ files in the diff, and no new CFN resource types. AdminCreateUser/AdminSetUserPassword are operator-shell credentials, not a stack execution role, so there is no bootstrap policy to narrow. Per ADR-002 this PR correctly needs no BOOTSTRAP_VERSION bump.
7. Review agents run
- code-reviewer — ran. Surfaced B1 (unreachable guard / dead code contradicting its own docstring) and N6 (duplicated mapping).
- silent-failure-hunter — ran. The catch blocks are correctly narrow (
err.namechecks, rethrow otherwise) with zero empty catches and no mock fallbacks; genuinely good. Its finding is the inverse of a swallowed error — a failure mode with no error at all (the B1 hang), plus the unmapped expired-temp-password path. - pr-test-analyzer — ran. N5 (uncrossed
login.ts→auth.tsseam, criticality 8), N2 (invite rendering unasserted, 5), and the observation thatauth.test.ts's no-callback case tests an unreachable shape. - comment-analyzer — ran. Highest-value hit of the pass:
auth.ts:63"we surface a clear error rather than hanging" is factually false given the only caller — the comment documents intent, not behavior. Otherwise the docstrings are accurate and explain why (the access-token rationale is exemplary). - type-design-analyzer — ran.
NewPasswordPrompt(auth.ts:29) is a clean minimal seam that keeps TTY concerns out of the auth layer: encapsulation 8, expression 7. TheAuthResultalias (auth.ts:114-118) is a reasonable structural narrowing of the two SDK shapes, though hand-mirroring SDK fields will drift;Pick<AuthenticationResultType, …>would stay pinned to the SDK. Its optionality is what makes the B1 guard look reachable — the type permitsundefinedthat no caller supplies. - security-review — ran (auth/secrets change). No secrets reach
console.*ordebug()— audited every call site in the two changed files;auth.ts:71logs only region/client_id/user_pool_id. No hand-rolled crypto, SRP, or token storage. Access token stays in memory and the test assertscreds.access_tokenisundefinedon disk. Invite file keeps mode 0600 +chmod. Net posture improves. Residual: N3 (trim mangles whitespace passwords) and the unmapped temp-password expiry. - code-simplifier — ran. N4 (one-line passthrough with an 11-line docstring; now-narrow
adminSetPermanentPasswordsurface).
None omitted — every agent's scope is touched by this diff.
8. Human heuristics
- Proportionality — pass. ~200 lines of source for two user-visible flows, no speculative abstraction.
NewPasswordPromptis the one new indirection and it earns itself by keeping readline out ofauth.ts. Deleting the invite half-failure branch is a net simplification. - Coherence — pass with one nit. Lands in
cli/per routing; "temporary password" / "first-login rotation" used consistently across code, docs, and commit message. Nit:cli/README.mdstill says "permanent" (N1), so the repo now contradicts itself. - Clarity — concern.
cli/src/auth.ts:63documents behavior the code does not have (B1), the single highest-value thing to fix here. Names are otherwise strong —respondToNewPasswordChallenge,accessTokenFor,usernameFromSessionall say what they do. - Appropriateness — concern. Cognito integration is verified against real SDK semantics (challenge/session echo,
FORCE_CHANGE_PASSWORDstate machine, correct exception names) rather than invented — no AI001. But B1's failure mode is exactly AI001/AI005 one layer down: the stdin contract was validated only against self-written mocks, never against a real pipe, andauth.test.tsasserts a caller shape the CLI cannot produce.
Fix B1 (make the guard reachable + a test that crosses the login.ts→auth.ts seam) and I am happy to approve. N1/N2 are quick and I would take them in the same push since they are user-facing copy; N3–N6 are fine as follow-ups.
| // Cognito challenges with NEW_PASSWORD_REQUIRED. Pass an interactive | ||
| // prompt so the user can set a permanent password inline; `login` | ||
| // answers the challenge and persists the resulting tokens. | ||
| await login(opts.username, password, () => { |
There was a problem hiding this comment.
Blocking (B1) — the non-interactive guard is unreachable, and this path can hang forever.
The prompt callback is passed unconditionally, so auth.ts:111's if (!promptNewPassword) guard can never fire from the only caller in the codebase. The docstring at auth.ts:63 promises --password non-interactive login "surfaces a clear error rather than hanging" — it cannot, because nothing ever passes undefined.
I reproduced this against the real prompt-secret.ts non-TTY reader and the real promptNewPasswordWithConfirmation body. With stdin as an open pipe carrying no further data (CI, wrapper script, bgagent login --username u --password "$TEMP"):
isTTY=false
This account requires a new password on first login.
New password:
>>> HANG — no error, no prompt answered, still waiting after 4s
The neighbouring stdin states fail differently, and none is tested:
| stdin | behavior |
|---|---|
| open pipe, no data | hangs indefinitely |
< /dev/null |
New password cannot be empty. — blames the user |
| one line piped | Passwords do not match. — confirm read returns '' |
Suggested fix — gate the callback on real interactivity so automation gets the error the docstring already promises:
const interactive = process.stdin.isTTY && !opts.password;
await login(opts.username, password, interactive ? () => {
console.log('This account requires a new password on first login.');
return promptNewPasswordWithConfirmation();
} : undefined);Separately, promptPassword below calls rl.close() before resolve(line), and rl.once('close') rejects — since close is emitted synchronously the rejection wins the race. Verified with the exact source ordering: real order → REJECTED "No password provided.", swapped → RESOLVED "TempPass1!". Pre-existing and masked today (piped login was single-line), but this PR makes the interactive/piped distinction load-bearing, so it is worth fixing here. Note this command now chains two hand-rolled stdin readers (readline here, raw-mode/piped in prompt-secret.ts) contending for one fd — the '' confirm read above is that fragility surfacing.
There was a problem hiding this comment.
Fixed. login.ts now gates the callback on real interactivity — const interactive = Boolean(process.stdin.isTTY) && !opts.password; — and passes undefined otherwise, so the if (!promptNewPassword) guard in auth.ts:111 is now reachable and a first-login --password/non-TTY invocation gets the clear "log in interactively" error instead of hanging. Added a command-layer seam test (login.test.ts) that crosses login.ts→auth.ts: challenge fires + non-interactive ⇒ throws the guard message and asserts no stdin data listener is attached / resume is never called, so it can never read stdin. Also fixed the promptPassword race per your finding: it now resolve(line) before rl.close() and guards the close-reject with a resolved flag, so the synchronous close no longer wins and a piped password is not lost — covered by two new tests (piped line resolves; empty close rejects "No password provided.").
| * instead of tokens. When ``promptNewPassword`` is supplied, we prompt for a | ||
| * replacement, answer the challenge via ``RespondToAuthChallenge``, and persist | ||
| * the resulting tokens. Without a prompt (e.g. ``--password`` passed | ||
| * non-interactively) we surface a clear error rather than hanging. |
There was a problem hiding this comment.
Blocking (B1, same defect) — this sentence is factually false given the only caller.
"Without a prompt (e.g. --password passed non-interactively) we surface a clear error rather than hanging" describes intent, not behavior: login.ts:38 always supplies the callback, so the guard at line 111 is dead code and the non-interactive path hangs (reproduced — see the login.ts:38 comment).
This is the most valuable comment in the diff to get right, because a future maintainer reading it will reasonably assume the non-interactive case is handled and not re-test it. Once the callback is gated on process.stdin.isTTY && !opts.password, the sentence becomes true as written and no edit is needed.
Worth adding while you are here: invited users now sit in FORCE_CHANGE_PASSWORD under Cognito's default 7-day TemporaryPasswordValidityDays — I checked, it is never set anywhere in cdk/. A teammate who logs in on day 8 gets a bare NotAuthorizedException with no hint that the temp password merely expired and the admin must re-issue. Permanent passwords never expired, so this failure mode is new to this PR; mapping it to "your temporary password expired — ask your admin to re-run bgagent admin invite-user" would close the loop.
There was a problem hiding this comment.
Addressed. With the callback now gated on process.stdin.isTTY && !opts.password (see the login.ts thread), the docstring sentence "Without a prompt … we surface a clear error rather than hanging" is true as written, so no edit was needed there. I also added the expired-temp-password mapping you suggested: login() now wraps the initial InitiateAuth and maps NotAuthorizedException to "Login failed: incorrect password, or your temporary password expired. Temporary passwords lapse after a few days — ask your admin to re-run bgagent admin invite-user for a fresh one." No secret is included in the message (the attempted password is never echoed), and non-auth errors are rethrown unchanged. Two new auth.test.ts cases cover the mapping (asserting the message and that the password is not leaked) and the rethrow path.
| const lines = [ | ||
| `email: ${email}`, | ||
| `password: ${password}`, | ||
| `${passwordLabel} ${password}`, |
There was a problem hiding this comment.
Nit (N2) — column alignment is now broken, and the docs show output this code cannot produce.
The label widens to temp password: (14 chars) while email: and bundle: keep their hardcoded 4-space and 3-space pads. Rendering the real line-builder:
email: your-email@example.com ← actual output
temp password: K9$mPq2nL!vXf3Hb
bundle: eyJhcGlfdXJsIjoi
But docs/guides/USER_GUIDE.md:99-101 (and the Authentication.md mirror) advertise a re-aligned block:
email: your-email@example.com
temp password: K9$mPq2nL!vXf3Hb
bundle: eyJhcGlfdXJsIjoi…
So the docs promise alignment the code never emits. Either pad all three labels to width 14, or revert the docs sample to match. This is the class of defect a source diff hides and only rendering reveals.
No test covers it — cli/test/commands/admin.test.ts has no assertion on the password label or on the new trailing "On first login…" line, so nothing guards either the alignment or the wording.
There was a problem hiding this comment.
Fixed by padding, matching the docs. writeCredentialsFile now pads every label to the widest one (temp password: = 14) via padEnd(LABEL_WIDTH), so email:/temp password:/bundle: all put their value at the same column — exactly the aligned block docs/guides/USER_GUIDE.md:99-101 advertises (I kept the docs sample and made the code match it, per your first option). Added a test in admin.test.ts that drives invite-user (Cognito layer mocked), reads the written ~/.bgagent/invites/<email>.txt, and asserts (a) the label is temp password: and never password: for an invite, (b) both the email and password values start at the same offset (label width + 1), and (c) the trailing "On first login you will be prompted to set a permanent password." line is present — so both the alignment and the wording are now guarded.
| * they log in. An admin who needs a login-ready permanent password (no | ||
| * first-login prompt) can still use ``bgagent admin reset-password``. | ||
| */ | ||
| export async function adminInviteUser( |
There was a problem hiding this comment.
Removing the adminSetPermanentPassword call is correct, and I want to be explicit that deleting the half-failure try/catch is not a lost safety net: that branch existed only because a second mutating call could fail after adminCreateUser succeeded. With one call there is no half-state to report, so the diagnostic is genuinely dead code. Good call, and the expect(input).not.toHaveProperty('Permanent') regression guard in the test is exactly the right pin.
Nit (N4): the function is now a one-line passthrough to adminCreateUser with an 11-line docstring above it — the docstring is longer than the body, and the error diagnostics were the only thing that distinguished the two. Consider collapsing it into the call site at commands/admin.ts:141 and keeping the why as a one-line comment, or keep the wrapper as the stable seam and trim the prose. Either is fine; the current shape just reads like a wrapper that outlived its reason.
Related: adminSetPermanentPassword (line 140) is now reached only through adminResetPassword, so the exported surface here could shrink — worth a look if knip flags it.
There was a problem hiding this comment.
Done — kept the wrapper as a stable seam (smaller diff than inlining, and admin.test.ts now drives it directly) and trimmed the 11-line docstring to ~2 lines: "Invite a user with a temporary password (#238): the FORCE_CHANGE_PASSWORD state it leaves them in forces a first-login rotation, so the admin-generated string stops being a valid credential once the teammate sets their own. Kept as a named seam over adminCreateUser for the command layer and tests." Thanks for confirming the half-failure try/catch deletion was correct. On the exported-surface note: adminSetPermanentPassword is still reached via adminResetPassword (and the Permanent-flag regression test pins it), and the dead-code ratchet is advisory/non-blocking here, so I have left the export as-is rather than reshaping the surface in this PR.
Part 1 — `bgagent change-password`: - New `cli/src/commands/change-password.ts`, registered in bin/bgagent.ts. - Interactive: prompts current password + new password (twice, confirmed). - `auth.ts changePassword()` re-authenticates with the current password to mint an in-memory access token (ChangePassword requires an AccessToken, which the CLI never persists) then calls `ChangePasswordCommand`. Wrong current password → clear error before any change; weak new password → Cognito's `InvalidPasswordException` surfaced verbatim. Part 2 — force-rotate on first login: - `login()` now handles the `NEW_PASSWORD_REQUIRED` challenge: prompts for a new password, answers via `RespondToAuthChallengeCommand`, persists tokens. A non-interactive login (no prompt) surfaces a clear error instead of hanging. - Invites now issue a TEMPORARY password: `adminInviteUser` drops the `AdminSetUserPassword` (Permanent: true) call, leaving the user in FORCE_CHANGE_PASSWORD so the admin-shared credential stops being valid once the teammate logs in once. `admin reset-password` keeps its permanent-password path unchanged. Docs: USER_GUIDE + LINEAR_SETUP_GUIDE teammate-onboarding sections updated for the temp-password first-login flow; Starlight mirrors regenerated. Uses the AWS SDK's Cognito commands throughout — no hand-rolled crypto, token storage, or SRP. No passwords or tokens are logged. Closes #238 Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
… align invite output + docs (#238) Remediate @isadeks review of PR #680. B1 (blocking): the NEW_PASSWORD_REQUIRED prompt callback was passed to `login` unconditionally, so the non-interactive guard in auth.ts was statically unreachable — a first-login account driven with `--password` on a non-TTY pipe hung forever on the readline prompt. Gate the callback on real interactivity (`process.stdin.isTTY && !opts.password`) so automation gets the clear "log in interactively" error the docstring already promised. Also fix a pre-existing promptPassword race: it called `rl.close()` (which emits 'close' synchronously → reject) before `resolve(line)`, so a successfully piped password was lost. Resolve before close and guard the close-reject with a `resolved` flag. Expired-temp-password mapping: invited users sit in FORCE_CHANGE_PASSWORD under Cognito's default 7-day TemporaryPasswordValidityDays; a day-8 login got a bare NotAuthorizedException. Map it (no secret leaked) to a message pointing at a fresh `bgagent admin invite-user`. N2: invite credentials file mis-aligned (`temp password:` = 14 chars vs 4/3-space pads on email/bundle). Pad all labels to width 14 so the value column lines up with the block the USER_GUIDE advertises. N4: trim adminInviteUser's 11-line docstring to ~2 lines, kept as a named seam over adminCreateUser (smaller diff than inlining). Docs (N1): cli/README.md no longer says invite-user "sets a permanent password" (now temporary/first-login rotation) and documents `bgagent change-password`. Tests: command-layer seam test crossing login.ts→auth.ts (challenge + non-interactive ⇒ guard error, never reads stdin), interactive first-login path, piped-password race + no-line reject, expired-temp mapping, and the invite-file label/alignment. Closes #238 Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
f4737a6 to
4474cb7
Compare
isadeks
left a comment
isadeks
left a comment
There was a problem hiding this comment.
Re-review — PR #680 at 4474cb74 (was f4737a6f)
1. Verdict
Approve. B1 is genuinely fixed — I re-ran my original repro against the real code at both heads and the hang is gone, with the fix proven causal. All four remediated threads hold up, and the new tests actually pin the fixes (I mutation-tested each one). Two nits below, neither blocking; the expired-temp-password one (N7) is new and worth a follow-up because the remedy the message prescribes is refused by the next command.
2. Adjudication of my prior findings
| Prior finding | Verdict | Evidence at the new head |
|---|---|---|
| B1 (blocking) — callback passed unconditionally ⇒ guard unreachable ⇒ hang on piped stdin | FIXED | cli/src/commands/login.ts:45-49 gates on Boolean(process.stdin.isTTY) && !opts.password and passes undefined otherwise. Repro below. |
B1 (cont.) — promptPassword closed readline before resolving, so the synchronous close-reject won the race |
FIXED | cli/src/commands/login.ts:106-116 — resolved = true; resolve(line); rl.close(); plus a if (!resolved) guard on the close handler. |
B1 (knock-on) — day-8 expired temp password gave a bare NotAuthorizedException with no hint |
FIXED (with a new nit, N7) | cli/src/auth.ts:84-100 maps it and names the fix. Mapping is right; the prescribed action is the problem — see N7. |
N1 — cli/README.md said invite-user "sets a permanent password"; change-password absent |
FIXED | cli/README.md:76 (first-login section incl. the non-interactive behavior), :78-83 (change-password reference), :367 (temporary, and the reset-password contrast). |
| N2 — invite-file columns misaligned; docs advertised output the code could not emit | FIXED | cli/src/commands/admin.ts:273-279 pads all labels to width 14. I rendered the real line-builder: all three values now land at column 15, byte-identical to docs/guides/USER_GUIDE.md:99-101. |
| N4 — 11-line docstring on a one-line passthrough | FIXED | cli/src/cognito-admin.ts:155-159, trimmed to 4 lines; wrapper kept as a named seam, which admin.test.ts now drives directly. Reasonable call. |
N5 — no command-layer test crossing login.ts→auth.ts |
FIXED | cli/test/commands/login.test.ts adds exactly that seam test, and it is the one that catches the regression (mutation-proven below). |
N3 (promptSecret trims) / N6 (duplicated InvalidPasswordException mapping) |
STILL OPEN, correctly deferred | cli/src/prompt-secret.ts:80,105,118; cli/src/auth.ts:155 + :305. Both pre-existing, both explicitly filed as follow-ups in my prior review. Not raising again. |
My note that removing adminSetPermanentPassword from the invite path is correct |
STANDS | cli/src/cognito-admin.ts:161-168 — one call, no half-state. Permanent-flag regression guard still pinned at cli/test/commands/cognito-admin.test.ts:305. |
Nothing withdrawn — but see N7, which is a defect in the fix for the knock-on I asked for, not in my original finding.
3. Proof that B1 is actually fixed (not just moved)
I compiled the real login.ts + auth.ts + prompt-secret.ts + change-password.ts at both heads and ran them as real processes with a real OS pipe on stdin. Only the Cognito transport is stubbed, returning the NEW_PASSWORD_REQUIRED challenge a FORCE_CHANGE_PASSWORD account actually produces. Same harness, both heads:
OLD HEAD f4737a6f NEW HEAD 4474cb74
─ open pipe, no data, --password ────────────────────────────────────────
This account requires a new password… ERROR: This account requires a new
New password: __TIMEOUT__ ← THE HANG password on first login. Run `bgagent
login …` interactively (omit
--password) so the CLI can prompt you.
─ </dev/null (EOF), --password ──────────────────────────────────────────
ERROR: New password cannot be empty. ERROR: This account requires a new
← blamed the user password on first login. …
← now the honest, actionable error
That is the exact input from my original finding, and it now returns the error the auth.ts:63 docstring promises — so that docstring is true as written and needed no edit. Correct call. Case C (one line piped, no --password) also improved: the password is now read successfully and the challenge is reported, where the old race lost the line entirely.
4. Do the new tests pin the fixes? (mutation-tested)
I reverted each fix in the worktree and confirmed the new test fails — a test that stays green with the fix reverted is not a fix. Full suite is 664/664 green unmutated.
| Mutation | Result |
|---|---|
| Pass the callback unconditionally again (the original B1) | FAILS — first-login challenge + non-interactive (--password) throws the guard error and never reads stdin. This is the seam test from N5 doing exactly its job. |
Restore rl.close() before resolve(line) |
FAILS — piped password (non-TTY…): resolves before close so the race does not reject |
Remove all three padEnd(LABEL_WIDTH) calls |
FAILS — Expected: 15, Received: 10 |
Delete the NotAuthorizedException mapping |
FAILS — maps NotAuthorizedException to an expired-temp-password hint |
I also verified the readline mock in login.test.ts:48-59 is faithful rather than convenient: real Node readline emits 'close' synchronously from inside rl.close() (["line:TempPass1!","close","after-close-call"]), which is precisely what the mock models. That is the right way to build that seam.
Two mutations survived — coverage gaps, not defects, listed as N8/N9.
5. Regression hunt on the fix commit — clean
- The
try/catcharoundInitiateAuthdid not weaken typing.let result;is inferred, so I probed it: adding a bogus property yieldsTS2339: Property … does not exist on type 'InitiateAuthCommandOutput'. Control-flow analysis keeps the full SDK type, and theChallengeName/AuthenticationResultreads downstream stay type-checked. - The
resolvedflag does not strand the process.rl.close()still runs on the success path (login.ts:110), so the interface is torn down; the flag only suppresses the reject. The no-line path still rejects — pinned bypiped stdin closes with no line, and confirmed live (Case E:ERROR: No password provided.). - The gate does not reject a valid case. Interactive TTY without
--passwordstill gets the prompt and completes the rotation — verified by the new interactive test (RespondToAuthChallengereceives the rightUSERNAME/NEW_PASSWORD, tokens persist). NotAuthorizedExceptionis not double-mapped. The new catch is scoped to the initialInitiateAuthinlogin;refreshToken(auth.ts:263) andaccessTokenFor(auth.ts:366) keep their own distinct messages. Non-auth errors rethrow unchanged (pinned by the new rethrow test).- No secret leaks through the new path. The message is a constant — the attempted password is not interpolated, and the test asserts its absence.
- The
adminInviteUserdocstring trim deleted no behavior, and no orphans: the wrapper is still the only caller ofadminCreateUseron the invite path,adminSetPermanentPasswordremains reachable viaadminResetPassword(cognito-admin.ts:197), and thereset-passwordinvite-file branch still emits the plainpassword:label — pinned by the new negative assertion. - Docs mirror clean. I re-ran
node docs/scripts/sync-starlight.mjs;git status docs/came back empty. CI's mutation guard will not trip.
6. Nits (non-blocking)
N7 (new, in the fix) — the expired-temp message prescribes a remedy the next command refuses. cli/src/auth.ts:95-96 tells the teammate to ask their admin to "re-run bgagent admin invite-user". But adminInviteUser → adminCreateUser issues AdminCreateUserCommand on a user that already exists, so Cognito answers UsernameExistsException and the CLI (cognito-admin.ts:130-133) replies "User … already exists. Use a different email, or run bgagent admin delete-user …". The admin's only paths are delete-then-recreate (destroys the Cognito sub, which is the platform user_id per cdk/src/handlers/shared/gateway.ts:38 — so all their prior tasks orphan) or admin reset-password, which sets a permanent password and silently opts them out of the rotation this PR exists to enforce. The mapping itself is right and worth keeping; it is the prescribed action that dead-ends. Cheapest honest fix is to point at bgagent admin reset-password (or teach invite-user to re-issue a temp password for an existing FORCE_CHANGE_PASSWORD user via AdminSetUserPassword with Permanent: false, which is the real fix). Also worth noting: an established user who simply typos their long-standing permanent password now reads two sentences about temporary passwords that do not apply to them. Fine as a follow-up issue.
N8 — the isTTY half of the gate is unpinned. cli/src/commands/login.ts:45. Mutating it to const interactive = !opts.password; leaves all 5 login tests green, because the non-interactive test supplies --password and so exercises only the second conjunct. The uncovered case is the one that motivated the fix: first-login on a non-TTY pipe with no --password (a CI wrapper piping just the temp password). My Case D shows the real behavior there is still an indefinite block — this time inside promptPassword's readline waiting for a line that never comes, before login is ever called. That is a pre-existing property of promptPassword on an open pipe rather than something this PR introduced, and the EOF variant (Case E) errors correctly, so I am not blocking on it. But a test that sets isTTY = false without --password and asserts no new-password prompt is reached would pin the conjunct you deliberately added.
N9 — the alignment test only fails when all labels lose padding. cli/test/commands/admin.test.ts. Un-padding only the bundle: label keeps the suite green, because the assertions cover emailLine and pwdLine but not the bundle line — and the bundle label is the one that renders in the docs sample. One more expect(bundleLine.indexOf(bundle)).toBe(VALUE_COLUMN) closes it. (The invite-user invocation in that test produces no bundle, so it would need resolveCognitoAdminContext to return a configureBundle.)
Still-open from last time, correctly deferred: N3 (promptSecret trims, so a password with edge whitespace is silently mangled), N6 (New password rejected: duplicated at auth.ts:155 and :305). Neither is a regression; both are fine as follow-ups. Related: cli/src/commands/linear.ts:1598-1604 carries the same close-before-resolve shape this PR just fixed in login.ts, and its own docstring describes the hazard — worth the same treatment in a separate PR, out of scope here.
7. Documentation
Good, and the gaps I flagged are closed. cli/README.md:76 now documents the first-login rotation including the non-interactive failure mode and the expiry — a nice touch, since it means the docs no longer over-promise. :78-83 adds the change-password reference. :367 corrects "permanent" → "temporary" and, usefully, contrasts reset-password. docs/guides/USER_GUIDE.md:99-101 and its Authentication.md mirror now match rendered output exactly (verified by rendering, not reading). Mirror sync verified clean. Governance gate satisfied: #238 carries approved, and both Part 1 and Part 2 acceptance criteria are met.
8. Tests & CI
CI green on 4474cb74 (CodeQL ×3, build, dead-code, security-pr, PR-title lint). Locally 664/664 pass, up from 657 — the 7 new cases are all on the previously-uncovered paths (the login.ts→auth.ts seam, both piped-stdin shapes, the interactive rotation, the expired-temp mapping + its rethrow, and the invite-file rendering). That directly closes the N5 and N2 test gaps and, per the mutation results, four of them are load-bearing. Bootstrap synth-coverage: not applicable — no cdk/ or bootstrap/ files in the diff and no new CFN resource types; AdminCreateUser/AdminSetUserPassword are operator-shell credentials, not the stack execution role, so no BOOTSTRAP_VERSION bump is required (ADR-002).
9. Review agents run
- code-reviewer — ran over the delta. No new style/correctness issues; the type probe on
let resultcame back clean. - silent-failure-hunter — ran (the delta is almost entirely error-handling). The new catch is narrow, rethrows non-matching errors, and the
resolvedflag suppresses only a spurious reject while leaving the genuine no-input reject intact. Its one hit is N7: not a swallowed error but an error whose remedy dead-ends. - pr-test-analyzer — ran, and drove the mutation matrix. 4 of 6 mutations caught; the 2 survivors are N8/N9.
- comment-analyzer — ran. The previously-false
auth.ts:63sentence is now true as written, which is the outcome I wanted rather than a reworded comment. The new comments atlogin.ts:39-44andauth.ts:85-91explain why (including the synchronous-closemechanism) and I verified both claims against real Node behavior. Theadmin.ts:271-273comment accurately describes what the padding does. - type-design-analyzer — ran. No new types;
NewPasswordPrompt's optionality is now genuinely exercised by a real caller shape, which is what made the guard reachable. - security-review — ran (auth path). Net posture still improves. The new message is a constant with no secret interpolation (asserted). No token or password reaches
console.*/debug(). ThePermanent: trueremoval — the actual security win — is intact and pinned. - code-simplifier — ran. N4 addressed; nothing new.
None omitted; every agent's scope is touched by the delta.
10. Human heuristics
- Proportionality — pass. The fix is ~10 lines of source for the blocker plus a tight docs/alignment change. No new abstraction introduced to solve it.
- Coherence — pass. "temporary password" / "first-login rotation" now consistent across code,
cli/README.md,docs/guides/, and the invite file. The repo no longer contradicts itself. - Clarity — pass. The comment that was factually false is now factually true, and the new comments name the mechanism (synchronous
'close') rather than gesturing at it. Sole residue is N7, where the message is clear but points somewhere that does not work. - Appropriateness — pass, materially improved. This is the dimension I marked a concern last time, and it is the one that moved most. The stdin contract is no longer validated only against self-written mocks: there is a real command-layer seam test, and the
readlinemock is checked against actual Node semantics. The tests assert what the code should do — the mutation matrix is the evidence.
Nice remediation. Every thread was addressed on the merits rather than papered over, the docstring was made true instead of softened, and the tests were built at the seam that had let the bug through. Approving. N7 is the one I would file as a follow-up issue before too many teammates hit day 8; N8/N9 are cheap whenever you are next in these files.
| // the readline prompt with no terminal to answer it. When stdin is not a | ||
| // TTY or `--password` was supplied, hand `login` no prompt so it raises | ||
| // the clear "log in interactively" error instead of hanging. | ||
| const interactive = Boolean(process.stdin.isTTY) && !opts.password; |
There was a problem hiding this comment.
B1 — FIXED, and verified causal. The gate is exactly the shape I suggested, and undefined now reaches login, so the if (!promptNewPassword) guard at auth.ts:130 is reachable from the only caller.
I did not take this on the diff — I compiled the real login.ts + auth.ts + prompt-secret.ts at both heads and ran them as real processes with a real OS pipe on stdin (only the Cognito transport stubbed, returning the NEW_PASSWORD_REQUIRED challenge a FORCE_CHANGE_PASSWORD account produces). Same harness, my original failing input:
OLD f4737a6f: This account requires a new password on first login.
New password: __TIMEOUT__ ← the hang
NEW 4474cb74: ERROR: This account requires a new password on first login.
Run `bgagent login …` interactively (omit --password) …
The </dev/null case also stopped blaming the user (New password cannot be empty. → the honest guard error). And because the guard is now reachable, the auth.ts:63 docstring is true as written — leaving it alone was the right call.
Mutation-tested: reverting to the unconditional callback makes first-login challenge + non-interactive (--password) throws the guard error and never reads stdin fail. The N5 seam test earns its keep.
Nit (N8) — the isTTY conjunct itself is unpinned. Mutating this line to const interactive = !opts.password; leaves all 5 login tests green: the non-interactive test passes --password, so it only ever exercises the second conjunct. The uncovered case is the motivating one — first-login on a non-TTY pipe with no --password, e.g. a CI wrapper piping just the temp password. I checked that shape live and it still blocks indefinitely, though one layer earlier (inside promptPassword's readline, before login is called) — a pre-existing promptPassword property rather than something you introduced, and the EOF variant errors correctly, so not blocking. A test that sets isTTY = false with no --password and asserts no new-password prompt is reached would pin the conjunct you deliberately added.
| // include the attempted password or any secret in the message. | ||
| if (err instanceof Error && err.name === 'NotAuthorizedException') { | ||
| throw new CliError( | ||
| 'Login failed: incorrect password, or your temporary password expired. ' |
There was a problem hiding this comment.
Mapping FIXED — but the remedy it prescribes dead-ends (N7, non-blocking).
The mapping is right and I want it kept: a day-8 teammate previously got a bare NotAuthorizedException with nothing to distinguish "expired" from "wrong", and no secret is echoed here (the message is a constant — the test asserting the password's absence is a good pin). Mutation-tested: deleting the mapping fails maps NotAuthorizedException to an expired-temp-password hint.
The problem is the action. Tracing the journey this message sends the user on:
teammate: bgagent login → "…ask your admin to re-run `bgagent admin invite-user`"
admin: bgagent admin invite-user → adminInviteUser (cognito-admin.ts:161)
→ adminCreateUser (:111) → AdminCreateUserCommand
→ user exists → UsernameExistsException
→ "User … already exists. Use a different email, or
run `bgagent admin delete-user …` and try again."
So the fix this message names is refused by the very next command. The admin's actual options are both bad: delete-then-recreate destroys the Cognito sub, which is the platform user_id (cdk/src/handlers/shared/gateway.ts:38), orphaning every task the teammate ever submitted; or admin reset-password, which sets a permanent password and so silently opts them out of the first-login rotation this PR exists to enforce.
The new test asserts the message wording but not that the prescribed remedy works — which is why this slipped through. Cheapest honest fix: point at bgagent admin reset-password. The real fix: let invite-user re-issue a temp password for an existing FORCE_CHANGE_PASSWORD user (AdminSetUserPassword with Permanent: false), which preserves the sub and the rotation.
Secondary: an established user who typos their long-standing permanent password now reads two sentences about temporary passwords that do not apply to them. Worth a follow-up issue, not a blocker — the mapping is still a net improvement on the bare SDK error.
| const lines = [ | ||
| `email: ${email}`, | ||
| `password: ${password}`, | ||
| `${'email:'.padEnd(LABEL_WIDTH)} ${email}`, |
There was a problem hiding this comment.
N2 — FIXED. You took the option I preferred (keep the docs, make the code match). I rendered the real line-builder rather than reading it:
email: your-email@example.com
temp password: K9$mPq2nL!vXf3Hb
bundle: eyJhcGlfdXJsIjoiaHR0cHM6Ly9hYmMxMjM
All three values at column 15 — byte-identical to docs/guides/USER_GUIDE.md:99-101 and its Authentication.md mirror. Deriving LABEL_WIDTH from 'temp password:'.length rather than hardcoding 14 is the right move, and the password-reset branch still emits the plain password: label (pinned by the new negative assertion). Mutation-tested: removing the padding fails the new test with Expected: 15, Received: 10.
Nit (N9) — the test only catches an all-or-nothing regression. Un-padding only the bundle: label on line 279 keeps the suite green: the assertions cover emailLine and pwdLine but not the bundle line — and the bundle label is the one rendered in the docs sample. One more expect(bundleLine.indexOf(bundle)).toBe(VALUE_COLUMN) closes it, though it needs resolveCognitoAdminContext mocked to return a configureBundle since the current invocation produces none.
2 participants
Summary
Two related additions to the
bgagentCLI auth flow (see theClosesline below):bgagent change-password— a new interactive command that lets any logged-in user rotate their own Cognito password.bgagent loginhandles Cognito'sNEW_PASSWORD_REQUIREDchallenge so the teammate sets a permanent password only they know on first login.Closes #238
Root cause / context
loginhad no challenge handling.cli/src/auth.tsloginonly issued anInitiateAuthCommandand expectedAuthenticationResultdirectly — any Cognito challenge (likeNEW_PASSWORD_REQUIRED) fell through to the "Unexpected authentication response" error.adminInviteUser(cli/src/cognito-admin.ts) calledadminSetPermanentPassword→AdminSetUserPasswordCommand({ Permanent: true })(thePermanent: truelived atcli/src/cognito-admin.ts:151). So the admin-generated string the admin shared over Slack/email stayed a valid credential forever, and there was nochange-passwordcommand to rotate it.The fix
Part 1 —
change-password(cli/src/commands/change-password.ts, registered incli/src/bin/bgagent.ts):prompt-secrethelper).auth.ts::changePassword()re-authenticates with the current password to obtain a short-lived, in-memory access token.ChangePasswordCommandrequires anAccessToken, which the CLI deliberately does not persist (only the ID + refresh tokens the REST authorizer needs) — re-auth avoids widening the on-disk credential surface and doubles as verification of the current password (wrong one →NotAuthorizedException→ "Current password is incorrect." before any change is attempted). Cognito enforces the password policy server-side; a weak new password surfaces asInvalidPasswordException.Part 2 — force-rotate on first login:
auth.ts::login()now branches onresult.ChallengeName === NEW_PASSWORD_REQUIRED: it prompts for a new password (via a caller-supplied callback, keeping TTY concerns in the command layer), answers withRespondToAuthChallengeCommand, and persists the resulting tokens. A non-interactive login (no prompt callback, e.g.--passwordpassed) surfaces a clear error instead of hanging.adminInviteUserdrops the permanent-password step —adminCreateUseralready setsTemporaryPassword, which lands the user inFORCE_CHANGE_PASSWORD. That's exactly the state that triggers the first-login challenge.admin reset-passwordkeeps its permanent-password path unchanged (admin account recovery is out of scope).Why the SDK commands are the right call:
ChangePasswordCommandandRespondToAuthChallengeCommandare the AWS SDK's first-class primitives for exactly these flows — no hand-rolled crypto, password hashing, SRP, or token storage.@aws-sdk/client-cognito-identity-provideris already a CLI dependency (no new dependency introduced).Testing
MISE_EXPERIMENTAL=1 mise //cli:build→ 657 tests pass, compile clean, eslint clean;change-password.tsat 100% coverage.New/updated cases:
changePassword, reports success; access token never written to disk.ChangePassword.InvalidPasswordExceptionmessage.NEW_PASSWORD_REQUIRED— prompts, responds with the challengedUSERNAME/NEW_PASSWORD+ echoedSession, persists tokens; non-interactive path errors clearly; policy rejection surfaced.adminInviteUsernow issues exactly oneAdminCreateUserwithTemporaryPasswordand noPermanentflag (regression guard);UsernameExistsExceptionstill maps to aCliError.Security note
console.*ordebug()— only status strings and non-secret config fields (region, client_id, user_pool_id).ChangePasswordaccess token is minted in memory and discarded — never persisted to~/.bgagent/credentials.json.gitleaks git origin/main..HEAD→ no leaks). The 3 gitleaks findings onmise run security:secretsare a pre-existing full-history sweep of the shared store, not in this diff.Dependencies / related
cli/src/bin/bgagent.ts(UA command registration). This PR keeps itsbin/bgagent.tschange to a single import + a singleaddCommandline to minimize the rebase conflict; if feat(observability): solution attribution via native AWS_SDK_UA_APP_ID (#319, alt to #338) #345 merges first, this should rebase with a trivial 2-line conflict.Self-review notes
/review_prself-review complete (security/auth, silent-failures, test coverage, scope): approve-with-nits, zero code blocking. Fixed one blocking finding — theCloses #238line was previously backticked and did not auto-close; it is now unformatted (closingIssuesReferencesresolves to[238]).git diff origin/main..HEADare a stale-branch artifact — this branch is 1 commit behindorigin/main, which bumped several pinned action SHAs after the branch was cut. HEAD (f4737a6f) touches no.github/workflows/files; a merge/rebase ofmainat integration time resolves them automatically.Review remediation (@isadeks, rebased on
main)Rebased onto
origin/main, then remediated all four threads:login.tsnow gates the new-password callback on real interactivity (Boolean(process.stdin.isTTY) && !opts.password) and passesundefinedotherwise, so theif (!promptNewPassword)guard inauth.tsis reachable: a first-login--password/non-TTY login gets the clear "log in interactively" error instead of hanging. Also fixed the pre-existingpromptPasswordrace —resolve(line)now runs beforerl.close(), guarded by aresolvedflag, so the synchronous'close'reject no longer clobbers a piped password.login()now maps aNotAuthorizedExceptionon the initialInitiateAuthto a hint that the temporary password may have expired (Cognito's default 7-dayTemporaryPasswordValidityDays) and to re-runbgagent admin invite-user. No secret is echoed; non-auth errors rethrow unchanged.email:/temp password:/bundle:) now pad to width 14, so the value column lines up with the blockUSER_GUIDE.mdadvertises (kept the docs, made the code match).adminInviteUserdocstring. Trimmed the 11-line docstring to ~2 lines; kept the wrapper as a named seam overadminCreateUser.cli/README.mdno longer says invite-user "sets a permanent password" (now temporary / first-login rotation) and documentsbgagent change-password.New tests: command-layer seam test crossing
login.ts→auth.ts(challenge + non-interactive ⇒ guard error, never reads stdin), interactive first-login path, piped-password race + empty-close reject, expired-temp mapping (+ no-leak assertion), and the invite-file label/alignment/first-login-note.Gates:
MISE_EXPERIMENTAL=1 mise //cli:build→ 664 tests pass, eslint clean, coverage threshold met. SAST (semgrep auto + silent-success-masking) clean on the changed files.mise //docs:syncclean (no mirror drift).gitleaks git origin/main..HEAD→ no leaks (the 3 full-history findings are pre-existing, not in this diff).🤖 Generated with Claude Code