Follow-up: deferred review findings from PR #593

[philcunliffe]

Deferred findings from the review rounds on PR #593 (login-outcome-return, triaged at head b0b701d4fef97670fdc29bb17caa1a3c4763ea96). Triage classified every one as a preference, not a blocker; none affects behaviour, exit codes, or output. They are recorded here so they are not lost.

N1 - src/core/cli/remote_commands.js:522: unreadable --token-file reports reason: 'login_failed' rather than 'usage'.
Deferred because the two review rounds disagreed and round 2's reasoning holds: the catch also covers stdin read failure (not a usage error), and the branch exits 1 while every 'usage' return in the file exits 2, so relabelling would trade one inconsistency for a sharper one. Vocabulary only; classifyLoginFailure maps both to 'abandoned' either way. If someone wants 'usage' here, split the tokenFile arm from the stdin arm and keep exit codes consistent.

N2 - src/core/remote/types.d.ts:159-161: awkward "except for daemon_incomplete" clause on LoginOutcome.
The clause reads like an exception to the exitCode === 0 iff reason === 'ok' invariant but actually confirms it (remote_commands.js only reaches daemon_incomplete under daemonCode !== 0). Docs wording only; suggested phrasing from round 1: "every non-ok reason is non-zero; daemon_incomplete carries the installer's code rather than 1".

N3 - empty glosses on the two new [tests] refs: test/core/remote-login-command.test.js:127 and test/core/cli/wizard/join.test.js:108.
Deferred because the empty-gloss pattern is repo-wide (59 occurrences) and is being cleaned up corpus-wide (see #624); not this PR's debt.

N4 - stale prose mentions of runRemoteLogin at src/core/cli/wizard/steps.js:33 and test/core/cli/wizard/join.test.js:13.
Deferred because both sentences still read true at the level they are written (runRemoteLogin still exists as a one-line adapter over remoteLogin). Fix opportunistically when touching those files.

N5 - no test pins reason: 'connected_elsewhere' on the unreadable arm of the D4 gate (src/core/cli/remote_commands.js:628).
test/core/remote-login-command.test.js:206 covers the origins arm only. The unreadable-arm reason was a merge-resolution judgement call (recorded in the round-2 review); a test would pin the answer either way. No caller branches on the reason today, so this is a test nicety, not a defect.

Governance note (also non-blocking, but flagged for the human at the merge button): the round-2 fix commit b0b701d4 rewrote a rationale paragraph under #no-prose-control-flow in llp/0179-login-lane-returns-its-outcome.decision.md, whose header says Status: Accepted. Triage judged this acceptable rather than blocking: LLP 0179 is introduced by PR #593 itself and does not exist on master, so no settled corpus record was rewritten; the doc only becomes a settled record when it lands. The decision (classify on reason codes, keep the tee) is untouched; only a factually false sentence about WizardJoinResult.detail was corrected, and merging a known-false rationale into an Accepted record would be the worse outcome for record integrity. The orchestrator's <!-- neutral-note: accepted-llp-edit --> comment on the PR leaves the final call visible to the merging human; if they prefer the strict reading, the alternative is reverting the llp/0179 hunk and carrying the correction in a small extending LLP.

Related open design question recorded in the round-2 review, no action needed now: if a future caller ever branches on connected_elsewhere to give switch-server advice, the unreadable-layer arm should get its own reason (via a new LLP extending 0179 #outcome).

Backlink: #593 (review rounds at 7cf0a434 and 925ca2aa, fixes pushed in b0b701d4).

[philcunliffe]

Blocked: this issue is not actionable against master.

What neutral was doing. The issue-fix reconciler picked up #691 (needs-fix, no fix attempt yet) and dispatched a worker to address the five deferred findings from PR #593's review.

Why it cannot proceed. Every finding targets code that PR #593 introduces and that does not exist on master. #593 is still open (state=OPEN, not merged). Verified:

LoginOutcome in src/core/remote/types.d.ts @ master   -> absent
connected_elsewhere in src/ @ master                  -> absent
connected_elsewhere in src/ @ login-outcome-return    -> present (remote_commands.js, types.d.ts)

Per finding:

#	Status	Why
N1	blocked	reason: 'login_failed' is added by #593; on master runRemoteLogin/runStaticLogin return bare exit codes with no reason: at all
N2	blocked	LoginOutcome / LoginOutcomeReason are added by #593; master's types.d.ts ends at RemoteCredentialRecord
N3	won't-fix	the two cited refs are added by #593; master's pre-existing empty-gloss [tests] refs are part of a 59-occurrence repo-wide pattern that #624 is working through file-by-file. Fixing two arbitrary instances here is piecemeal churn
N4	won't-fix	the prose is accurate on master, not stale. runRemoteLogin is the real implementation at remote_commands.js:366 on master; it only becomes a thin adapter once #593 lands. Editing it on master would reference a symbol that does not exist here
N5	blocked	the D4 gate on master returns 2 with no reason code. A test pinning connected_elsewhere cannot be written without inventing the vocabulary it is meant to pin. This is the most worthwhile item of the five

What it needs from you. A routing decision:

1. Push the actionable subset onto login-outcome-return before Login lane returns its outcome; the wizard stops reading its prose (LLP 0179) #593 merges (N5's test, N2's rewording, optionally N1's arm split), and close Follow-up: deferred review findings from PR #593 #691 as absorbed. Cleanest, but it reopens an already-approved-and-held PR.
2. Leave Follow-up: deferred review findings from PR #593 #691 to fire after Login lane returns its outcome; the wizard stops reading its prose (LLP 0179) #593 lands. Everything becomes actionable the moment the branch is on master. Simplest, and costs nothing but time.
3. Close N3 and N4 as won't-fix regardless of which you pick, per the reasoning above.

Neutral's own read is option 2: #593 is already reviewed-clean and held, and none of the deferred items are severe enough to justify reopening it.

How to unstick. Reply with a comment on this issue (or merge #593). Neutral monitors this thread and will re-engage with your guidance on its next tick. It has labelled this neutral:stuck so it stops re-dispatching a worker that would reach this same conclusion every tick.