Login lane returns its outcome; the wizard stops reading its prose (LLP 0179)

[platypii]

The wizard's join phase classified a failed login by substring-matching three exported English sentences (LOGIN_NO_MEMBERSHIP_MESSAGE and friends) out of the login lane's tee'd stderr, and printJoinFailure matched a fourth. That made user-facing text load-bearing: rewording a message silently changed which fork the wizard offered, and nothing in the types said so.

The information was never lost, just discarded at the command boundary: the loopback receiver attaches the server's refusal code as callbackError, explainLoginError switches on it one frame below, and runRemoteLogin collapsed everything to a number.

What changed

• remoteLogin(argv, ctx, deps) returns { exitCode, reason }. runRemoteLogin is now a one-line adapter over it, so the CLI dispatch table still sees Promise<number>.
• reason is a LoginOutcomeReason: ok, the three D7 refusals, denied, login_failed, usage, connected_elsewhere, and the post-auth steps (store_failed, seed_failed, enroll_failed, daemon_incomplete). Every return in the browser and static paths names one.
• classifyLoginFailure and printJoinFailure switch on the code. The message constants are module-local again.
• LoginLaneResult carries reason; stderr stays, but only as the narration echoed in detail.

Same exit codes, same output, byte for byte. LLP 0179 records the decision, including why the fuller "extract a pure core, reduce the command to printing" refactor was rejected for now: the lane's printing is ordered by its work (consent notice before the browser, hold message after the marker write), and hoisting it risks a silent regression in messages other LLPs pin. LLP 0135 gets an Extended-by forward-ref.

Full suite: 3348 pass, 3 failures all pre-existing on master (blob-store, usage-policy-fold, attach-enable-resume). Typecheck clean apart from the pre-existing ai-gateway error.

[philcunliffe]

Review: findings (no blockers)

The refactor is faithful. Every prose branch has a code, exit codes and terminal output are preserved return for return, and no failure path can return undefined or fall out without an outcome. One doc claim is not true and is worth fixing before an Accepted doc lands.

Old-behavior enumeration

Two places read the login lane's prose on master. Both are covered.

old prose branch	site	replacement outcome	preserved
stderr includes LOGIN_NO_MEMBERSHIP_MESSAGE	classifyLoginFailure	reason: 'no_membership'	yes
stderr includes LOGIN_ORG_NOT_PERMITTED_MESSAGE	classifyLoginFailure	'org_not_permitted'	yes
stderr includes LOGIN_ORG_SELECTION_MESSAGE	classifyLoginFailure	'org_selection_required'	yes
no phrase matched	classifyLoginFailure	'login_failed', 'denied', 'store_failed', 'seed_failed', 'enroll_failed', 'usage', 'connected_elsewhere', 'daemon_incomplete' all hit default	yes
empty or absent stderr	classifyLoginFailure	absent reason hits default	yes
join.detail?.includes(LOGIN_ORG_SELECTION_MESSAGE)	printJoinFailure	join.reason === 'org_selection_required'	yes
status === 'failed', no org-selection phrase	printJoinFailure	'no_membership' / 'org_not_permitted' fall through	yes

Nothing lost. The three message constants had no consumer outside the wizard, so de-exporting them breaks nothing.

Failure and edge paths. All four D7 codes come out of loginFailureReason. An unmodelled code and a callbackError-less throw (timeout, network, exchangeCode failure, the loopback's no_code path, which deliberately throws a plain Error) both land on 'login_failed', matching the old "no phrase matched" answer exactly. --no-browser and the non-interactive static path are covered: static read failure to 'login_failed', empty token to 'usage', store failure to 'store_failed', success to 'ok'. Every return in remoteLogin / runStaticLogin / persistStaticToken / runBrowserLogin yields a well-formed LoginOutcome.

Faithfulness. 'ok' is produced only on the five exit-0 paths that already existed. daemon_incomplete is reachable only when result.daemonCode !== 0, so a partial enroll cannot report 'ok'.

Exit codes and output. All 18 converted returns checked against master: 2 to 2, 1 to 1, 0 to 0, result.daemonCode to result.daemonCode. No message added, removed or reordered.

Findings

1. minor, and the one worth acting on - llp/0179-...decision.md (§no-prose-control-flow) records a justification that is false.

"The wizard keeps teeing stderr, because WizardJoinResult.detail echoes the lane's own explanation to the user"

detail echoes nothing. It is set at src/core/cli/wizard/join.js:93 and read by nobody: printJoinFailure was its only consumer, and this PR removes that read. What actually shows the user the lane's output is the tee's write-through to ctx.stderr, which would happen without capturing at all. So after this PR, teeWriter and detail are dead weight, and the LLP records a reason for keeping them that does not hold. The same overstatement is in src/core/cli/wizard/types.d.ts:186-190 ("for narration").

Not pushed, because deleting detail is a design call and editing an LLP's decision text is yours. Either drop detail and the tee, or reword to "kept as an unread diagnostic field" and say so in the type doc. Worth doing before merge: the doc becomes immutable on acceptance, and this would freeze an incorrect rationale.

2. minor - src/core/cli/wizard/join.js:151: @ref LLP 0058#d7 [constrained-by] was dropped from classifyLoginFailure. The function is still constrained by the D7 taxonomy (the docstring says so in prose); only the machine-readable ref went, replaced by the 0179 one. Both can coexist. Suggest restoring it alongside.

3. minor - src/core/cli/remote_commands.js:520-522: a missing or unreadable --token-file reports reason: 'login_failed'. That is a usage error, and the sibling empty-token case two branches down correctly reports 'usage'. No behavioral consequence today (both classify 'abandoned', exit code unchanged at 1), but the vocabulary is the point of this PR and this one is mislabeled.

4. nit - src/core/remote/types.d.ts:691-694 reads as if daemon_incomplete could be zero. It cannot; the invariant holds unconditionally. Suggest "every non-ok reason is non-zero; daemon_incomplete carries the installer's code rather than 1".

5. nit - stale runRemoteLogin mentions at src/core/cli/wizard/steps.js:31 and test/core/cli/wizard/join.test.js:13. Both still read true at the level they are written, so left alone; fix if you are touching those files anyway.

6. nit - llp/0135-...design.md:327: the inline Extended-by: is unbolded, where the corpus writes **Extended-by: [LLP NNNN](...)** mid-doc (0033, 0049, 0062, 0067).

LLP docs

0179 accurately describes the code apart from finding 1. Status: Accepted, header fields, inline anchors and the author convention all match the corpus. All three anchors exist, every @ref this PR adds resolves, and the outbound refs to 0058#d7, 0134#login-lane, 0135#join, 0129#failed-join-returns-to-fork all resolve.

The 0135 edit is permitted: it appends a forward-ref plus a gloss to the applicable section, exactly the mechanism CLAUDE.md prescribes. It does not rewrite what 0135 settled; the classifyLoginFailure taxonomy paragraph above it is untouched and still accurate.

Revert tests

reverted	test that caught it
drop case 'org_selection_required'	join.test.js "org_selection_required (multi-org account) -> failed"
drop reason propagation in runJoinFlow	join.test.js "a non-zero login exit returns the classified failure"
printJoinFailure branches on the wrong reason	index.test.js "a multi-org join failure points at hyp remote login --org"
remove access_denied from loginFailureReason	remote-login-command.test.js "a server refusal is reported as its own reason"

Three did not fail: swapping seed_failed, enroll_failed or daemon_incomplete for another reason left the suite green. The test named "post-auth failures name their step" only asserted store_failed, so it did not hold what its name implies.

Pushed (7cf0a43)

1. test/core/remote-login-command.test.js: extended "post-auth failures name their step" to assert the outcome on the seed, enroll and daemon-install paths, reusing the harnesses already in the file. Each new assertion was revert-tested: swapping the reason now fails. Also pins exitCode: 3 on daemon_incomplete, so the installer's code cannot be flattened to 1 unnoticed.
2. src/core/cli/wizard/types.d.ts: RunWizardJoinOptions.runLogin still documented "Defaults to runRemoteLogin over ctx with its stderr captured for classifyLoginFailure", which is precisely what this branch stops being true. Retargeted to remoteLogin and the returned reason.

Conventions

Clean. No em dashes, no semicolons, no @typedef, no inline import('...'). remote_commands.js uses a root-anchored .js @import specifier as required.

Checks

npm test: 3351 pass, 0 fail, 1 skipped. npm run typecheck: clean. Both at the pushed head.

[philcunliffe]

Requesting changes on one small point; the full review is in the thread. No blockers, and the refactor itself is faithful: I enumerated every prose branch the wizard used to take and each has a corresponding outcome, with exit codes and terminal output preserved return for return.

The ask: llp/0179-...decision.md (§no-prose-control-flow) justifies keeping the stderr tee by saying WizardJoinResult.detail echoes the lane's explanation to the user. It does not. detail is set at join.js:93 and read by nobody once this PR removes printJoinFailure's read of it. What actually surfaces the lane's output is the tee's write-through to ctx.stderr, which would happen without capturing at all. So teeWriter and detail are now dead weight, and the doc records a reason for keeping them that does not hold. types.d.ts:186-190 says the same thing.

Worth fixing before merge specifically because the doc becomes immutable on acceptance, and this would freeze an incorrect rationale. Either drop detail and the tee, or reword to "kept as an unread diagnostic field".

Everything else is optional: a dropped @ref LLP 0058#d7 on classifyLoginFailure, a --token-file read failure reporting login_failed where the sibling empty-token case correctly says usage, and three nits.

Also pushed 7cf0a43 under neutral:adopt: your "post-auth failures name their step" test only asserted store_failed, so swapping seed_failed/enroll_failed/daemon_incomplete for any other reason left the suite green. It now asserts all four and pins exitCode: 3 on daemon_incomplete.

[philcunliffe]

Review: 2 actionable findings, both fixed and pushed (b0b701d)

Reviewed the merge head 925ca2aa. npm test 3863 pass / 0 fail / 6 skipped; npm run typecheck clean (the ai-gateway error the PR body mentions is gone - master fixed it). Both numbers re-run after the fix commit, same result.

Verdict on the three merge resolutions

1. src/core/cli/wizard/index.js - dropping import { LOGIN_ORG_SELECTION_MESSAGE }: CORRECT.

Grepped the whole tree. All three constants still exist at src/core/cli/remote_commands.js:858-860, now module-local, and all three are still read by explainLoginError (:906, :908, :910), which still writes them to stderr on the D7 paths. Keeping master's import would not have been "harmless" - it would be a broken import, because this branch un-exported them. So the resolver had no choice, and the choice it made is the right one.

Nothing user-facing was lost:

• the login lane still prints the sentence itself (remote_commands.js:668, explainLoginError);
• the wizard's own multi-org line is still printed, now selected by code rather than by substring: src/core/cli/wizard/index.js:779 if (join.reason === 'org_selection_required');
• the reason reaches it - src/core/cli/wizard/join.js:93 spreads reason onto the failure result, and src/core/cli/wizard/join.js:93 is the only site in src/ that constructs a WizardJoinResult failure, so there is no second path that would arrive without one.

Covered by test/core/cli/wizard/index.test.js:509 ("a multi-org join failure points at hyp remote login --org").

2. src/core/cli/wizard/types.d.ts - union of type imports: CORRECT. I recomputed the merge (git merge-tree --write-tree 7cf0a434 d3b3ea3) and diffed the raw conflicted tree against HEAD: the only change in this file is the deletion of the three conflict-marker lines. Both sides' imports are present, LoginOutcomeReason and ClientDescriptor / FolderAskMode / SelectSpec alike. Typecheck confirms nothing dangles.

3. src/core/cli/remote_commands.js:628 - return { exitCode: 2, reason: 'connected_elsewhere' } on the unreadable-central-layer branch: ACCEPTABLE, not a finding. I went in expecting this to be the one to change, and came out the other way:

• LLP 0179 #outcome assigns the code at gate granularity, verbatim: "'connected_elsewhere' for the LLP 0063 D4 exclusivity gate". The unreadable branch (added on master by LLP 0063 D4 exclusivity gate fails open on an unparseable central layer: login to a second org is permitted while enrolled #623 / 5688e06, four days after 0179 was written) is that same gate, its first arm. The resolver's inline comment says exactly this and is honest.
• It is also the semantically fail-closed reading, which is the whole point of LLP 0063 D4 exclusivity gate fails open on an unparseable central layer: login to a second org is permitted while enrolled #623: that commit's reasoning is that an unreadable layer is "an enrollment by every other definition the codebase uses". The gate refuses because it must assume the machine is connected. Reporting "not connected elsewhere" would be the mis-report; reporting the assumption the gate acts on is not.
• No caller behaves differently. classifyLoginFailure (join.js:158) maps everything outside the three D7 refusals to 'abandoned', and printJoinFailure (index.js:779) branches only on org_selection_required. A distinct reason would change no behaviour today.
• The cost of the alternative is real: LoginOutcomeReason is an enumeration that LLP 0179 §outcome spells out member by member. Adding one is extending an Accepted decision, which per CLAUDE.md wants a new LLP plus an Extended-by:, not a merge-round edit.

The honest caveat, recorded rather than acted on: the branch also refuses a same-origin re-login, and there connected_elsewhere is a name for a state we specifically cannot establish. If a future caller ever branches on connected_elsewhere to give switch-server advice, split it then - the prose already distinguishes the two cases correctly, so nothing is stuck.

Nothing else was dropped by the merge. git diff <auto-merge-tree> HEAD is exactly three hunks: the two conflict-marker deletions above and the six-line comment + return at remote_commands.js:628. Every other file in the merge is byte-identical to git's own auto-merge, so no behaviour from either side was hand-edited away. git diff origin/master..HEAD is the PR's own 10 files and nothing more.

Findings

F1 (actionable, minor - doc) llp/0179-login-lane-returns-its-outcome.decision.md §no-prose-control-flow recorded a rationale that is false. It read:

The wizard keeps teeing stderr, because WizardJoinResult.detail echoes the lane's own explanation to the user

detail echoes nothing. It is written at src/core/cli/wizard/join.js:93 and, once this PR removes printJoinFailure's substring match, read by no production code - I grepped src/ for every .detail access; the only other hit is an unrelated field in report_commands.js:538. What actually puts the lane's explanation on the user's terminal is teeWriter's write-through (join.js:216), which would happen if the wizard passed ctx.stderr unchanged. This is the one thing worth catching before merge, because the doc is Status: Accepted and the rationale would freeze on landing.

Fixed by making the sentence true rather than by changing the decision: the tee stays (that is the decision), and the doc now says the write-through is what the user sees and the copy is a diagnostic nothing branches on. Carried over unchanged from the previous round's finding 1, which was left for a human; this PR carries neutral:adopt, so I took it.

F2 (actionable, minor - refs) src/core/cli/wizard/join.js:154: the @ref LLP 0058#d7 [constrained-by] on classifyLoginFailure was dropped. The rewrite replaced it with the 0179 ref, but the function is still constrained by the D7 taxonomy - its three definitive cases are the D7 refusal codes verbatim. CLAUDE.md's "keep refs honest" is about updating a ref whose section no longer applies; this one still applies. Restored alongside the 0179 ref with a gloss.

Nits (not fixed, recorded)

• N1 src/core/cli/remote_commands.js:522: a --token-file that cannot be read reports reason: 'login_failed', while the empty-token case below reports 'usage'. Flagged last round as worth changing; I disagree and left it. The catch covers stdin read failure too, which is not a usage error under any reading, and the branch exits 1 - every 'usage' return in the file exits 2, so relabelling it would trade one inconsistency for a sharper one. 'login_failed' ("a transient or local failure") covers both arms honestly.
• N2 src/core/remote/types.d.ts, LoginOutcome: "exitCode === 0 iff reason === 'ok', except for daemon_incomplete, which carries the ... non-zero code" - the "except" is doing nothing, since the clause it introduces confirms the invariant rather than excepting it (remote_commands.js:764 only reaches it under daemonCode !== 0). Awkward, not wrong.
• N3 the two new [tests] refs carry an empty gloss: test/core/remote-login-command.test.js:127 and test/core/cli/wizard/join.test.js:108. Master landed Every [tests] @ref in the wizard pick suite carries a gloss (#613) #624 specifically to fill these in for one file. Left alone because the pattern is repo-wide (59 occurrences) and join.test.js:17-18 already has two, so this is a corpus-wide cleanup, not this PR's debt.
• N4 stale runRemoteLogin mentions at src/core/cli/wizard/steps.js:33 and test/core/cli/wizard/join.test.js:13. Both still read true (runRemoteLogin still exists and still delegates); carried over from last round.
• N5 no test pins the reason on the unreadable arm of the D4 gate. test/core/remote-login-command.test.js:206 covers the origins arm only. Given resolution 3 above is a judgement call, a test would pin the answer either way.

Conventions

Clean. No em dashes anywhere in the diff (checked the full origin/master..HEAD diff and my own), no statement semicolons, no @typedef, no inline import('...') types. remote_commands.js uses the root-anchored .js @import specifier. All @refs added by this PR resolve: 0179#outcome, 0179#no-prose-control-flow and the restored 0058#d7 all exist as anchors in their targets. The llp/0135 edit is the permitted mechanism - an appended Extended-by: forward-ref that leaves what 0135 settled untouched.

What I pushed

b0b701d on login-outcome-return, doc + annotation only, no behaviour change:

1. llp/0179-...decision.md §no-prose-control-flow - rewrote the tee rationale (F1). Verified in the pushed tree: the string "echoes the lane's own explanation to the user" now has 0 occurrences.
2. src/core/cli/wizard/join.js:154 - restored @ref LLP 0058#d7 [constrained-by] (F2). Verified present in the pushed tree.

Post-fix: npm test 3863 pass / 0 fail; npm run typecheck clean.

[philcunliffe]

Note for the human merging this: an Accepted LLP was edited.

The review round just pushed b0b701d which, alongside an uncontroversial @ref restore in src/core/cli/wizard/join.js, rewrote a rationale paragraph in llp/0179-login-lane-returns-its-outcome.decision.md (section #no-prose-control-flow). That document is Status: Accepted.

The reviewer's justification is sound on the merits: the old sentence claimed WizardJoinResult.detail "echoes the lane's own explanation to the user", and that is factually wrong once this PR removes printJoinFailure's substring match. What the user actually sees is teeWriter's write-through. Merging the false rationale would freeze it into an Accepted record.

But CLAUDE.md routes this differently: Accepted docs are settled, and only mechanical edits (typos, broken links, status changes, renumbering) are allowed in place. A corrected rationale is arguably more than that, even when the decision itself is untouched (the decision here, classify on reason codes and keep the tee, is unchanged).

So this is a judgement call that belongs to you, not to neutral. Two clean options:

1. Accept the in-place correction as an editorial fix, on the grounds that a factually false rationale is a defect in the record rather than part of what was settled.
2. Revert the llp/0179 hunk from this PR and carry the correction in a small follow-up LLP that extends 0179, per the "change is a new request" convention.

Neutral has not reverted anything and is not blocking on this. Flagging it here so the choice is visible at the merge button rather than discovered later.

[philcunliffe]

Triage: clear to merge, residuals deferred to #691

Triage rung at head b0b701d4fef97670fdc29bb17caa1a3c4763ea96. Both review rounds' actionable findings (F1, F2) were fixed and pushed; every remaining residual was re-examined against the code and classified non-blocking:

• N1 (remote_commands.js:522 'login_failed' vs 'usage'): vocabulary preference, no behavioural difference; round 2's reasoning for leaving it stands.
• N2 (remote/types.d.ts LoginOutcome wording): docs phrasing only, the invariant it describes holds.
• N3 (empty [tests] glosses): corpus-wide pattern, not this PR's debt.
• N4 (stale runRemoteLogin prose mentions): still literally true.
• N5 (no test on the unreadable D4 arm's reason): test nicety; no caller branches on the reason.
• Accepted-LLP edit (llp/0179 §no-prose-control-flow rewritten in b0b701d4): judged an acceptable pre-landing correction, not a blocker. LLP 0179 is introduced by this PR and does not exist on master, so no settled corpus record was rewritten; the decision itself is untouched, and the only change makes a factually false rationale true before it freezes. The <!-- neutral-note: accepted-llp-edit --> comment above keeps the strict-reading alternative (revert the hunk, extend via a new LLP) visible for the merging human.

All deferred findings are enumerated with file:line and rationale in #691.

[philcunliffe]

Correction to my earlier note above: I was wrong, and you can disregard it.

I flagged that commit b0b701d edits llp/0179-login-lane-returns-its-outcome.decision.md while that document is Status: Accepted, and asked you to choose between accepting the edit and reverting it in favour of a follow-up LLP.

That framing was based on an incomplete check. I read the Status: header on the PR branch but never checked whether the document was already part of the settled corpus. It is not:

$ git cat-file -e origin/master:llp/0179-login-lane-returns-its-outcome.decision.md
ABSENT
$ git diff --name-status origin/master...origin/login-outcome-return -- llp/0179-...decision.md
A	llp/0179-login-lane-returns-its-outcome.decision.md

LLP 0179 is introduced by this PR. It has never existed on master. The immutability convention protects landed records that other work builds on; a document arriving in the same change that edits it is not yet such a record, and correcting a factually wrong rationale sentence before it lands is strictly better for record integrity than merging the false version and minting a correcting LLP afterwards.

So there is no governance question here and nothing for you to decide. The triage rung reached the same conclusion independently and classified it as non-blocking; the deferred items from this PR are enumerated in #691.

Apologies for the noise.