Join wait converges on the applied org config, not client attaches

[platypii]

The wizard's "Applying your org's configuration..." phase polled the daemon's client attach markers, so an org config that attaches no clients on this machine (and the no-org-config 404 steady state) always burned the full 60-second budget - on top of the login lane's own 30-second wait over the same markers.

The wait now polls for the applied config slot under config-control/ instead. That is the on-disk fact the locked-row computation reads right after the wait, and it lands within one pull-and-apply cycle, so the common cases converge in seconds. The join seed deliberately does not count as convergence, and the 404 steady state still runs to the timeout (nothing lands on disk to observe) and falls through to the unlocked picker as before.

LLP 0223 records the decision and extends LLP 0129.

• npm test green (4030 pass), npm run typecheck clean
• core_boot_noop and client_attach_idempotent smokes green
• walkthrough_picker_to_first_query smoke fails, but identically on unmodified master (pre-existing)

[philcunliffe]

Verdict: findings (3 minor, all fixed on the branch). The core change is correct and well grounded; nothing blocking.

Reviewed at 0a83bbf. Fixes pushed as 9651405, new head 9651405e6106ffbf21064bb39539a93d33232dd8.

What I checked and what holds

The central claim of this PR is that the wait should converge on the fact its own consumer reads. I verified that end to end and it holds:

• computeCentralLockedSources (src/core/cli/wizard/join.js:132) reads only the layered config via classifyClientProvenance, which is a pure membership check over centralConfig.plugins / effective.plugins (src/core/cli/wizard/provenance.js:44). It never touches an attach marker. So the attach marker really was a proxy signal, and the applied slot is the fact.
• The pointer only appears after the slot content is durable: each applied config is written to its own slot file with the etag sidecar written before the flip, and the pointer is replaced atomically via tmp+rename (src/core/config/apply.js:214-227). So hasAppliedCentralConfig cannot observe a half-written config. That is the property the whole change rests on and it is real.
• The predicate cannot be satisfied by a non-central apply. Only the daemon constructs the engine (src/core/daemon/runtime.js:190), and it derives every path from stateRoot and never touches the user-owned local layer, so the active slot is by construction a pulled config.
• Stale-slot false convergence does not happen. This was my main correctness worry: a machine with an already-applied slot from a previous enrollment would converge on the first probe and lock rows from the old org's config. It does not, because enrollCentralSink calls resetCentralLayerToSeed(obsEnv.stateDir) (src/core/commands/central.js:160) before the wait starts, clearing active, both slots, both etag sidecars, and state.json. A re-join therefore genuinely waits for a fresh apply, and a login that does not re-enroll converges on a slot that is in fact the current applied org config. Worth stating explicitly somewhere, since LLP 0223's "a re-run converges on the first probe" bullet reads as if it were unconditional.
• Termination is sound at every exit. waitForCentralConverge always probes at least once (even at timeoutMs: 0), returns on truthy probe, returns on remaining <= 0, and otherwise sleeps at least 1ms capped at the remaining budget. There is no unbounded path and no path that returns ok: true without a probe having said so. A throwing probe cannot escape as a join failure.
• Rollback/probation are handled correctly by omission: a rollback flips to the other slot (still an applied central config), and a first pulled config that fails to apply leaves no pointer, so the wait times out to the unlocked picker.
• LLP process is followed correctly for the doc of record: LLP 0223 is a new Decision, and LLP 0129's Extended-by: line was updated rather than 0129's prose being edited underneath the code. The @ref on waitForCentralConverge was updated honestly (0129 for the budget/fallback, 0223 for the signal) rather than left stale.
• Dropping attached from the return is safe: nothing consumes it. src/core/cli/wizard/index.js reads only status, lockedSources, managed.

Findings

1. Minor - LLP 0135 (Status: Active) was contradicted without a forward-ref.
llp/0135-install-experience-overhaul.design.md:322 states that waitForCentralConverge "reuses the bounded reconcile-wait runRemoteLogin already performs internally ... rather than adding a second poll loop". This branch adds exactly that second poll loop. LLP 0129 correctly received its Extended-by: note; 0135 did not, and it is Active, not Implemented. CLAUDE.md requires appending the forward-ref to "the applicable parts of the old doc", and 0135 already uses the inline Extended-by: idiom four lines below the affected paragraph (for LLP 0179), so the shape was already established.
Fixed in 9651405: an inline Extended-by: [LLP 0223] note now follows that paragraph, recording that the reuse sketch is withdrawn while the budget, the fallback, and the helper's location are unchanged. Verified: git show 9651405:llp/0135-install-experience-overhaul.design.md:339 contains the note; the same file at 0a83bbf contained no reference to 0223.
Deliberately left: LLP 0128 (:153) and LLP 0136 (:70) also describe the attach reuse. 0128 is the RFC that spawned 0129 and 0136 is a plan with Status: Implemented; both are historical records of the build, and 0129 is the doc of record that carries the forward-ref. Annotating every mention would be noise.

2. Minor - the default budget was a literal that can now drift.
src/core/cli/remote_commands.js:216 hardcoded timeoutMs = 30000 with a comment claiming it "matches the attach wait's", in a file that exports ATTACH_WAIT_DEFAULT_MS = 30000 at line 40 specifically so the progress line and the poll loop "quote the same number". Before this PR the value was inherited by delegation and could not drift; a literal can.
Fixed in 9651405: { timeoutMs = ATTACH_WAIT_DEFAULT_MS, intervalMs = 500 }. Verified: line 217 of the committed file uses the constant; line 216 at 0a83bbf was { timeoutMs = 30000, intervalMs = 500 } = {}. intervalMs is deliberately left at 500 (faster than the attach wait's 1000, which is the point of the change).

3. Minor - a stuck probe was invisible at every level (log-driven development).
src/core/cli/remote_commands.js:225-229 swallowed the probe error with no signal at all. A durable EACCES/EIO on config-control/ is then indistinguishable from the no-org-config 404 steady state: same { ok: false }, same converged=false span attribute, same "Didn't hear back from your org's config in time" narration. CLAUDE.md's log-driven-development rule asks that a failure identify the broken step, and the previous implementation at least had waitForClientAttach's own context; the new loop has none.
Fixed in 9651405: one warn per wait (not per poll, so a stuck fs does not emit 120 lines over the 60s budget) with hyp_component / hyp_operation / error_kind: converge_probe_unreadable / error_message, matching the existing markFirstSyncHoldBestEffort call in the same file. Verified: join.converge_probe_failed and the loggedProbeError latch are present at lines 222-236 of the committed file. Behavior is unchanged; the verdict is still ok: false and the wait still runs to the timeout, so the existing throwing-probe test passes untouched.

Also added: the default-probe test now covers a host with no config-control/ directory at all (never joined), which is a clean not-converged rather than a swallowed probe error. test/core/remote-login-command.test.js:480-484.

Not findings, but worth recording

• The 404 steady state still burns the full 60s, and the login lane's own 30s attach wait still runs before it, so the worst case is ~90s of waiting for a machine with no org config. LLP 0223 acknowledges both and scopes them out (ending the 404 wait early would need the pull loop to persist its outcome cross-process). No action asked for here; noting it so nobody reads the PR title as having fixed the 404 case.
• A dangling active symlink (pointer present, slot file gone) reports converged, then resolves to an unloadable central layer and yields managed: true with an empty lock set. This is exact parity with resolveCentralLayerPath, which is the same read boot uses, so the wait is no more optimistic than the loader it fronts. Not worth special-casing.
• Test coverage of the new predicate is adequate: converge-after-N-polls, timeout, throwing probe, seed-is-not-convergence, applied-slot-is-convergence, and now never-joined. The termination condition that changed is covered from both sides.

Verification

• npm test in a clean worktree at the new head: 3977 pass / 15 fail, byte-identical to the failure set at 0a83bbf before my commit. All 15 are environmental in this sandbox (marked absent from the borrowed node_modules, and a hyparquet version skew in the parquet filter tests); none touch this PR's files. The six directly relevant files (remote-login-command, cli/wizard/join, cli/wizard/progress, config-apply, join-command, leave-command) are 145/145 green.
• npm run typecheck: clean apart from the same pre-existing Cannot find module 'marked' from the missing dependency. No new type errors, and the { ok: boolean } narrowing in types.d.ts typechecks against every call site.
• CI on the PR is the authoritative gate for the new head.

Nothing here needs a human decision. Nice change: grounding the wait in the fact the next line of code actually reads is the right instinct, and the reset-on-enroll interaction that makes it safe is the non-obvious part that made it work.

[philcunliffe]

Verdict: findings (1 minor, fixed). Nothing blocking. The change itself is correct; the one finding is that round 1's own fix did not actually do what it claimed.

Reviewed at 9651405. Fix pushed as df8039b, new head df8039b8a071f67a0b7b5f1638f53982cadf982a. This is round 2 of 2.

Round 1's three fixes, re-checked

Fix 2 (quote ATTACH_WAIT_DEFAULT_MS) is correct and behavior-preserving. ATTACH_WAIT_DEFAULT_MS = 30000 (src/core/cli/remote_commands.js:41) is the same number the pre-PR delegation inherited: the old code forwarded an unset timeoutMs into waitForClientAttach, whose own default is that constant. So the 30s budget is preserved exactly, and it is now impossible for the two to drift. The constant is module-level and the reference sits in a default-parameter expression evaluated at call time, so there is no TDZ hazard from the declaration order. intervalMs staying at 500 rather than the attach wait's 1000 is deliberate and is the point of the change.

Fix 1 (LLP 0135 forward-ref) is correctly placed. llp/0135-install-experience-overhaul.design.md:339-346 now carries the inline Extended-by: LLP 0223 note, in the same idiom as the LLP 0179 note four lines above, and it names precisely what is withdrawn (the reuse sketch) and what stands (budget, fallback, helper location). LLP 0129's Extended-by: line and the two @refs on waitForCentralConverge all resolve: #join-before-picker exists at llp/0129-init-wizard-fork.decision.md:30, and LLP 0223 exists with the anchor-less @ref LLP NNNN [relation]: gloss form, which is valid syntax.

Fix 3 (the latched probe warn) is where the finding is. The latch itself is right: loggedProbeError is declared once before the loop (src/core/cli/remote_commands.js:222) and is never re-armed, so a 60s wait over a durable fault logs once, not 120 times. The problem is upstream of the latch.

Finding

1. Minor (observability) - the probe-error branch was unreachable in production, so the diagnostic round 1 added never fired for the error class it was written for.

src/core/cli/remote_commands.js:227-243 catches a throwing probe and warns. But the default probe is hasAppliedCentralConfig (src/core/config/apply.js:112), which went through readActiveSlot (src/core/config/apply.js:778-790) - and readActiveSlot wraps fs.readlinkSync in a bare catch { return null }. Every fs error is folded into null, so the probe returned a plain false and could not throw.

That is exactly the scenario the round-1 fix names. A config-control/ the CLI cannot read (EACCES, EIO, an active that is not a symlink) produced { ok: false }, converged=false, "Didn't hear back from your org's config in time", and no warn at all - byte-identical to the healthy no-org-config 404 steady state. The code comment claiming "a durable EACCES would otherwise log for the whole budget" described a branch that could not be entered.

Verified before the fix, against 9651405: with config-control/ at mode 000, hasAppliedCentralConfig returned false and did not throw.

Fixed in df8039b. hasAppliedCentralConfig now re-reads the pointer on the not-converged answer only and rethrows anything that is not ENOENT/ENOTDIR. That is the same "no directory is the answer, an unreadable one is not" discrimination centralLayerResolutionFailure already makes twenty lines above (src/core/config/apply.js:157-165), so it is the file's own established idiom rather than a new one. The happy path is still a single readlink (the extra read happens only when the answer is already "not converged"), and the wait's verdict is unchanged in every case: still { ok: false }, still the unlocked-picker fallback, still never an error. The only difference is that a stuck host now leaves a join.converge_probe_failed warn behind.

Verified after the fix: same harness now reports EACCES -> threw EACCES, while the happy path is still true, a never-joined host still false, and a seed-only host still false. git show df8039b:src/core/config/apply.js carries the new body; git show 9651405:src/core/config/apply.js is the one-line return readActiveSlot(...) !== null.

Test added: test/core/remote-login-command.test.js walks all three states - an empty control directory is the silent steady state, an active that is not a symlink throws, and the wait still returns { ok: false } over it. The last assertion is the one that matters: the fix must make the branch reachable without making the wait fail. Using a non-symlink active (readlink EINVAL) rather than a chmod 000 keeps the test deterministic under any uid, including root in CI.

Nothing left unfixed. No blockers, no preferences parked for triage.

Re-examined this round, and holding

• Termination. waitForCentralConverge probes before checking the clock, so timeoutMs: 0 still gets exactly one probe; it returns on a truthy probe, returns on remaining <= 0, and sleeps max(1, min(intervalMs, remaining)), so a non-positive interval cannot busy-spin and no sleep can overshoot the deadline. There is no path that returns ok: true without a probe having said so, and no unbounded path short of a probe that never settles (which is the attach wait's pre-existing shape, not new here).
• Success on a config that was not applied. The pointer flips only after the slot file and its etag sidecar are durable (src/core/config/apply.js:220-227), so a converged probe cannot observe a half-written config. resetCentralLayerToSeed (src/core/config/apply.js:198-212) removes active first, before the slot files, so the reset opens no dangling-pointer window of its own. Only external tampering leaves a pointer with no target, and that resolves exactly as resolveCentralLayerPath would, which is the read boot itself uses.
• Stale-slot false convergence. Still holds for the reason round 1 gave: enrollCentralSink calls resetCentralLayerToSeed before the wait starts (src/core/commands/central.js:160), and that call predates this PR - it exists for hyp join token silently ignored when a stale active config slot shadows seed.json #139. The converge wait inherits its guarantee rather than needing one.
• The empty/initial case. A never-joined host (no config-control/ at all), a seed-only host, and a host mid-enrollment all read as not-converged and fall to the narrated unlocked picker. Covered from all three sides in the tests now.
• Type seam. RunWizardJoinOptions.waitForConverge (src/core/cli/wizard/types.d.ts:399) narrowed to { ok: boolean } and every call site was updated; nothing consumed the dropped attached.

Worth recording, no action asked for

• The apply-then-reset race remains theoretically open: between resetCentralLayerToSeed and the daemon restart, a still-live old daemon could complete an in-flight apply and flip a pointer for the previous enrollment, which the wait would then read as converged. This predates the PR (the attach markers the wait used to poll had the identical window) and is what the hyp join token silently ignored when a stale active config slot shadows seed.json #139 reset is already aimed at, so it is not a regression and not something this branch should take on.
• The 404 no-org-config steady state still burns the full 60s on top of the login lane's own 30s attach wait. LLP 0223 scopes that out explicitly. Noted so nobody reads the PR title as having fixed it.

Verification

• npm test at df8039b in a clean worktree: 3978 pass / 15 fail. The 15 are byte-identical to the set at 9651405 before my commit (missing marked, hyparquet version skew in the parquet filter tests) and are environmental to this sandbox; the only diff between the two runs is the test-number shift from my added test. The seven directly relevant files (remote-login-command, config-apply, config-apply-recovery, join-command, leave-command, cli/wizard/join, cli/wizard/progress) are 150/150 green.
• npm run typecheck: unchanged, the same single pre-existing Cannot find module 'marked'.
• CI on the PR is the authoritative gate for the new head.

Good change, and the underlying instinct is the right one: ground the wait in the fact its own next line reads. The round-1 fixes were the right calls too; fix 3 just needed its probe to cooperate.

[philcunliffe]

Triage verdict: safe to ship at df8039b8a071f67a0b7b5f1638f53982cadf982a. No unresolved findings, nothing deferred, no follow-up issue needed.

The review fix-loop hit its 2-round cap because round 2's own fix moved the head, not because anything is outstanding. I re-verified every finding from both rounds against the committed tree at df8039b:

• Round 1, finding 1 (LLP 0135 contradicted without a forward-ref): resolved. llp/0135-install-experience-overhaul.design.md:339-346 carries the inline Extended-by: LLP 0223 note withdrawing the reuse sketch, in the file's established idiom.
• Round 1, finding 2 (hardcoded 30000 could drift): resolved. src/core/cli/remote_commands.js:217 defaults timeoutMs from ATTACH_WAIT_DEFAULT_MS.
• Round 1, finding 3 (silent probe errors): resolved. The latched once-per-wait join.converge_probe_failed warn with structured attributes is at src/core/cli/remote_commands.js:227-247.
• Round 2, finding 1 (that warn branch was unreachable because readActiveSlot folded all fs errors to null): resolved. hasAppliedCentralConfig (src/core/config/apply.js:121-135) now re-reads the pointer on the not-converged answer and rethrows anything that is not ENOENT/ENOTDIR, mirroring centralLayerResolutionFailure's existing discrimination. The wait still returns { ok: false } over it, and the test at test/core/remote-login-command.test.js:502-519 covers all three states (empty control dir silent, non-symlink active throws, wait survives it).

I also re-confirmed the LLP wiring: LLP 0223 exists as an Accepted Decision, LLP 0129's Extended-by: line names it against #join-before-picker, and both @refs on waitForCentralConverge resolve.

The two round-2 notes are not deferrals from this PR, so no follow-up issue was opened: the apply-then-reset race predates the PR (the attach markers the wait used to poll had the identical window, and the #139 reset already targets it), and the no-org-config 404 steady state burning the full budget is unchanged behavior that LLP 0223 explicitly scopes out.

All 9 CI checks are green at this head and the PR is mergeable. Ready for the maintainer.