Skip to content

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

Open
platypii wants to merge 2 commits into
masterfrom
login-outcome-return
Open

Login lane returns its outcome; the wizard stops reading its prose (LLP 0179)#593
platypii wants to merge 2 commits into
masterfrom
login-outcome-return

Conversation

@platypii

@platypii platypii commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

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.

…LP 0179)

The wizard classified a failed join by substring-matching three exported
English sentences out of the login lane's captured stderr, which made
user-facing text load-bearing. remoteLogin() now returns { exitCode, reason },
runRemoteLogin is the exit-code adapter over it, and classifyLoginFailure and
printJoinFailure switch on the reason code. The three message constants are
module-local again. Output and exit codes are unchanged.
@platypii platypii added the neutral:adopt Foreign PR adopted into neutral's reconcile scope label Aug 4, 2026
@philcunliffe philcunliffe added the neutral:adopted Adoption completion record: merged while carrying neutral:adopt (LLP 0031) label Aug 4, 2026
…unLogin doc

Assert the reason on the three post-auth failure paths the outcome return
names but nothing tested (seed, enroll, daemon install); each one's reason
could be swapped for another and the suite stayed green.

RunWizardJoinOptions.runLogin still documented a captured stderr as what
classifyLoginFailure reads, which this branch is exactly what changes.

Co-Authored-By: Claude <noreply@anthropic.com>
philcunliffe pushed a commit that referenced this pull request Aug 4, 2026
PR #593 (created earlier) also adds an llp/0179-*, and the
duplicate-numbers CI check validates each PR against master alone, so
both pass and whichever merges second lands a duplicate number. This PR
yields: 0180 is free repo-wide (master's highest is 0178).

Mechanical renumber only, per CLAUDE.md ("renumbering that does not
change meaning" is a permitted edit to an Accepted doc). Every updated
reference means this PR's decision; none refer to #593's separate
0179 (the login lane returning its outcome).

Also repairs the @ref in test/core/walkthrough-backfill.test.js, which
had an empty gloss and a blank line between it and the test it
annotates (a blank line breaks attachment, per CLAUDE.md).

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

Copy link
Copy Markdown
Contributor

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 philcunliffe added the neutral:changes-requested neutral reviewed an adopted PR and requests changes (non-binding; maintainer decides) label Aug 4, 2026

@philcunliffe philcunliffe 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.

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.

bgmcmullen added a commit that referenced this pull request Aug 4, 2026
…t contributions (LLP 0177/0180) (#594)

* The init picker enables OpenClaw but never attaches it: issue LLP 0177 filed

The finale's attach lane is generic; the clientsPicked list feeding it is
a hardcoded claude/codex pair in two call sites, so a picked OpenClaw
lands enabled-but-unattached with the sweep already recording - the limbo
LLP 0175's investigation started from. Fix direction: derive picked
clients from manifest client contributions; sibling of LLP 0174's
manual-path consent design.

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

* The wizard finale attaches OpenClaw: clientsPicked derives from client contributions (LLP 0177/0179)

Picking OpenClaw in hyp init enabled the adapter and then dropped it from
the finale: no attach, no import, a client_attach_missing limbo. Both
clientsPicked sites now derive the client list from the picked rows'
manifest client contributions instead of a hardcoded claude/codex pair,
so the finale attaches OpenClaw like any other client and a future
adapter joins by declaring contributes.client.

The backfill consent question stays honest for a sweep-backed provider:
its history imports on the daemon sweep schedule regardless of any
answer (LLP 0170), so the finale discloses that and runs the first
import instead of asking, and the question names only the providers the
answer can control. The openclaw-gateway-restart instruction rides the
adapter's attach output (LLP 0169), no wizard copy needed.

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

* Renumber this PR's decision LLP 0179 -> 0180; repair an orphaned @ref

PR #593 (created earlier) also adds an llp/0179-*, and the
duplicate-numbers CI check validates each PR against master alone, so
both pass and whichever merges second lands a duplicate number. This PR
yields: 0180 is free repo-wide (master's highest is 0178).

Mechanical renumber only, per CLAUDE.md ("renumbering that does not
change meaning" is a permitted edit to an Accepted doc). Every updated
reference means this PR's decision; none refer to #593's separate
0179 (the login lane returning its outcome).

Also repairs the @ref in test/core/walkthrough-backfill.test.js, which
had an empty gloss and a blank line between it and the test it
annotates (a blank line breaks attachment, per CLAUDE.md).

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

* An adapterless client contribution is not applicable to the attach lane, not failed (review of #594)

Addresses the requested change and both test gaps from the review:

- Picking Claude Desktop no longer prints a false attach failure. Its
  plugin contributes a client for skill/agent ownership but deliberately
  registers no runtime adapter (LLP 0115#no-attach-on-join), so the
  finale records it as noAdapter with ok: true and the run summary
  prints nothing for it; a registered adapter that throws still reports
  a real failure. LLP 0180#decision now states that not every client
  contribution is attachable, and names the row-to-plugin-to-clients
  fan-out invariant.
- buildPickerBackfillRunner is exported and pinned against the real
  provider contributions, so sweeping: [] (or a sweep rename) now fails
  a test instead of riding through green CI.
- The full derived clientsPicked set is pinned against the bundled
  catalog: claude, claude-desktop, codex, openclaw.

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

* LLP 0180 cites LLP 0115 in its decision; list it in Related and References

---------

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Co-authored-by: test <test@example.com>
Co-authored-by: neutral <neutral@example.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

neutral:adopt Foreign PR adopted into neutral's reconcile scope neutral:adopted Adoption completion record: merged while carrying neutral:adopt (LLP 0031) neutral:changes-requested neutral reviewed an adopted PR and requests changes (non-binding; maintainer decides)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants