Returning gate keeps managed truthful when the merged config is invalid (#605)

[philcunliffe]

Root cause

evaluateReturningGate (src/core/cli/wizard/fork.js) returned
{ action: 'first-run', managed: false } whenever !report.configExists || !report.configValid, before the hasCentral check ran. A machine with a
central layer whose merged config fails validation (a server-side config
change, client/server schema drift) was classified as an unmanaged first run:

• gate.managed was false, so computeCentralLockedSources never ran and the
  locked set stayed empty.
• The picker rendered the org's rows editable instead of checked-and-disabled.
  A user who picked one composed it into the local layer: exactly the
  central/local collision LLP 0129 #join-before-picker exists to avoid.

The fix (the minimum from the issue)

Two lines of behavior, no UX change:

1. src/core/cli/wizard/fork.js: derive managed from
   report.layered.hasCentralbefore the first-run early return, so the
   gate reports it truthfully on that path. A central layer on disk is a
   property of the machine (LLP 0031), not of whether the merged config
   currently validates.
2. src/core/cli/wizard/index.js: on action === 'first-run' with
   gate.managed, compute the locked set (the same guarded
   computeLockedSafe the scoped re-entry uses) and thread managed into the
   pick phase. The pathway stays unset, so the fork still runs exactly as
   before.

Which gate or screen is shown is unchanged, and no "config invalid" diagnostic
was added. Both changed sites carry an
@ref LLP 0129#join-before-picker [implements] annotation.

The design question remains open for the maintainer

The issue asks what the gate should do with an invalid config on a managed
machine, and this PR deliberately does not answer it. Still open: whether an
invalid config on a managed machine should show the returning gate with a
"config invalid" diagnostic instead of falling to the first-run path. That is
a UX call for the maintainer. This change is a pure correctness fix that stops
the central/local collision (the actual harm) without deciding it, and it does
not preclude the diagnostic option later.

Ground truth

Regression tests added: two in test/core/cli/wizard/fork.test.js (a managed
machine with an invalid config, and with no config at all, still reports
managed: true) and one in test/core/cli/wizard/index.test.js that seeds a
real central layer on disk and asserts the orchestrator locks claude from it
on the first-run path.

Pre-fix FAIL (tests on unmodified source)

not ok 11 - evaluateReturningGate: a managed machine with an invalid config is still managed on the first-run path
---
duration_ms: 0.654686
type: 'test'
location: '/tmp/tmp.47UdSyzNSh/test/core/cli/wizard/fork.test.js:135:1'
failureType: 'testCodeFailure'
error: |-
Expected values to be strictly equal:
false !== true
code: 'ERR_ASSERTION'
name: 'AssertionError'
expected: true
actual: false
operator: 'strictEqual'
...
not ok 12 - evaluateReturningGate: a managed machine with no config at all is still managed
---
duration_ms: 0.24328
type: 'test'
location: '/tmp/tmp.47UdSyzNSh/test/core/cli/wizard/fork.test.js:143:1'
failureType: 'testCodeFailure'
error: |-
Expected values to be strictly equal:
false !== true
code: 'ERR_ASSERTION'
name: 'AssertionError'
expected: true
actual: false
operator: 'strictEqual'
...
not ok 22 - runInitWizard: a managed first run locks the org rows from the on-disk central layer
---
duration_ms: 4.204262
type: 'test'
location: '/tmp/tmp.47UdSyzNSh/test/core/cli/wizard/index.test.js:167:1'
failureType: 'testCodeFailure'
error: |-
Expected values to be strictly deep-equal:
+ actual - expected
+ undefined
- [
- 'claude'
- ]
code: 'ERR_ASSERTION'
name: 'AssertionError'
expected:
0: 'claude'
operator: 'deepStrictEqual'
...
1..37
# tests 37
# suites 0
# pass 34
# fail 3

Post-fix PASS (same two files)

1..37
# tests 37
# suites 0
# pass 37
# fail 0
# cancelled 0
# skipped 0
# todo 0
# duration_ms 204.476103

Full suite + typecheck

$ npm test
1..3378
# tests 3380
# suites 0
# pass 3379
# fail 0
# cancelled 0
# skipped 1
# todo 0
# duration_ms 15773.593144
$ npm run typecheck
> tsc -p tsconfig.json --noEmit
(exit 0)

Scope

Touches only src/core/cli/wizard/fork.js and the locked-set call site in
src/core/cli/wizard/index.js (plus their two test files). No overlap with the
parallel work on src/core/cli/wizard/pick.js / composePickerConfig (#603)
or the finale/attach lane in src/core/cli/walkthrough.js (#604).

Fixes#605

[philcunliffe]

Verdict: the fix is correct and well-targeted. One doc-honesty finding fixed, two informational notes for the maintainer.

Reviewed at head a8594ef in a detached worktree. The change does exactly what
issue #605's "at minimum" bullet asks, the reachability of the bug is real, and
the regression tests are genuine.

Ground truth I re-derived (not taken from the PR body)

Pre-fix FAIL confirmed. With src/core/cli/wizard/fork.js and
src/core/cli/wizard/index.js reverted to origin/master and the PR's test
files left in place:

not ok 11 - evaluateReturningGate: a managed machine with an invalid config is still managed on the first-run path
not ok 12 - evaluateReturningGate: a managed machine with no config at all is still managed
not ok 22 - runInitWizard: a managed first run locks the org rows from the on-disk central layer
# tests 37
# pass 34
# fail 3

Post-fix, full suite + typecheck at my head (fd15e15):

$ npm test
1..3378
# tests 3380
# pass 3379
# fail 0
# skipped 1
# duration_ms 15872.254059
$ npm run typecheck
> tsc -p tsconfig.json --noEmit
(exit 0)

The bug is reachable and the fix is real.collectHypAwareStatus builds
layered from hasCentral (src/core/daemon/status.js:287-296) before and
independently of the validation pass at src/core/daemon/status.js:305-325, so
report.layered.hasCentral is populated even when configValid is false. Since
resolveLayeredConfig prunes local additions that break the merge
(src/core/config/merge.js:120, LLP 0031 central-is-sacrosanct), a
configValid: false on a machine with a central layer means the central
document itself is invalid: exactly the server-side-change / schema-drift case
the issue names. Reading managed above the early return therefore changes
behavior on a state that actually occurs.

The lock actually lands.index.test.js's new test seeds a real central
layer on disk and asserts opts._pickOpts.locked deep-equals ['claude'].
That is a strong assertion here, because index.js:157 threads with
...(locked ? { locked } : {}) and [] is truthy in JS: a wrong seed path
would surface as [] (the computeLockedSafe swallow at index.js:406-412),
not as undefined, so the deep-equal genuinely proves resolution from disk.
Downstream, pick.js:91-92,149,173 renders the locked ids checked+disabled and
filters them out of the local-layer composition, which is the harm the issue
describes.

Conventions: no semicolons, no em dashes, no @typedef, no inline
import('...') types anywhere in the diff. Both @ref LLP 0129#join-before-picker
annotations point at a real anchor (llp/0129-init-wizard-fork.decision.md:22)
and are honest about what the code does. The bare [tests]: gloss in the two
test files matches the convention those files already use
(test/core/cli/wizard/index.test.js:18-19).

---

Findings

1. Low (fixed): computeCentralLockedSources's docstring enumerates its callers and went stale

src/core/cli/wizard/join.js:120-122 read:

Shared by the join phase (after convergence) and the wizard's scoped
re-entry, where no join runs but a managed machine's org rows must still
render locked.

This PR adds a third caller (the managed first-run path at
src/core/cli/wizard/index.js:86-94) and left the enumeration at two. A
docstring that lists its callers is a claim, and it is now false; CLAUDE.md's
living-docs rule wants the doc edit in the same change as the code.

Fixed in fd15e15: the doc now covers "every wizard entry that reaches the
picker on an already-managed machine without a join: the scoped re-entry, and
the first-run path a managed machine falls to when its merged config no longer
validates." Verified in the committed tree with
git show HEAD:src/core/cli/wizard/join.js. Doc-only, so npm test (3379 pass
/ 0 fail) and npm run typecheck (exit 0) were re-run after it and are the
numbers reported above.

2. Medium (not fixed, maintainer's call): the now-truthfully-managed machine still sees the pathway fork, which LLP 0129 #fork settled it should not

llp/0129-init-wizard-fork.decision.md:18-20:

The wizard's first question is the pathway fork: "Join a team" or "Local
install and configuration". [...] An enrolled machine never sees the fork.

After this PR, src/core/cli/wizard/index.js:86-94 establishes, at that exact
point, that the machine is enrolled (gate.managed is true because a central
layer is on disk), and then deliberately leaves pathway unset so the
while (!pathway) loop at index.js:96 runs the fork anyway. The PR's own
comment says so ("The pathway stays unset, so the fork still runs"). So an
already-enrolled machine is offered "Join a team".

This is pre-existing on master and the PR discloses it, but it is worth stating
plainly as a review finding rather than leaving it implicit: the new @ref LLP 0129#join-before-picker [implements] annotation now sits directly above the
branch that knowingly contradicts LLP 0129 #fork three lines later. The two
anchors are in the same Accepted decision.

I deliberately did not fix this. Resolving it means choosing between the
issue's options (preset pathway = 'scoped', or show the returning gate with a
"config invalid" diagnostic), which changes which screen the user sees and is
the design question #605 explicitly reserves for the maintainer. Per CLAUDE.md,
an Accepted LLP is settled: whichever way this goes, it wants a new LLP that
@refs 0129 rather than a silent code change. Flagging it so the follow-up is
not lost when #605 closes.

3. Informational: the second added fork test asserts a machine state the status collector cannot produce

test/core/cli/wizard/fork.test.js:143 ("a managed machine with no config at
all is still managed") constructs { configExists: false, hasCentral: true }.
In production that pairing is unreachable: src/core/daemon/status.js:295-302
sets configExists = config !== null where
config = (centralConfig || localConfig) ? merged.effective : null, and
mergeConfigLayers always returns a non-null effective object when a central
layer loaded (src/core/config/merge.js:38-88). A central layer on disk
therefore always implies configExists: true.

No change requested. The test is harmless defensive coverage of the || in the
early-return guard, and it does fail pre-fix, so it earns its place. Recording
it only so nobody later reads it as evidence that this state occurs.

---

What I changed

Commit	File	Finding
fd15e15	src/core/cli/wizard/join.js	#1, docstring now names the third caller

Pushed to fix/issue-605. No change to the PR body, no merge.

[philcunliffe]

Round 2 verdict: the fix is correct and ready. Round 1's medium is not a blocker (evidence below). Three doc-honesty findings fixed in aebb3f2. No residual code findings.

Reviewed at head fd15e15 in a detached worktree with node_modules symlinked
from the main checkout (full 3380-test run, not the truncated no-modules subset).

---

1. Re-assessment of round 1's medium finding: pre-existing design gap, not a blocker

Round 1 flagged that llp/0129-init-wizard-fork.decision.md:20 says "An
enrolled machine never sees the fork," while src/core/cli/wizard/index.js:86-96
establishes the machine is enrolled and then leaves pathway unset so the
fork runs. I re-derived this and conclude it is a pre-existing design gap in
LLP 0129, correctly deferred, not a defect this PR ships or worsens. Evidence:

a. The PR does not change which screen any user sees. The diff to
fork.js touches only the value ofmanaged in the returned object and the
log line; action is byte-identical on every branch. The diff to index.js is
a pure else if appended after the existing scoped-reconfigure branch, and
that branch deliberately does not set pathway. So the while (!pathway)
loop at index.js:99 ran the fork for this exact machine state on master too.
The fork exposure is identical pre- and post-PR; the PR strictly improves
this state by locking the org's rows that master left editable.

b. LLP 0129 never decided this state. "An enrolled machine never sees the
fork" (#fork) is realized through #returning-gate
(llp/0129-init-wizard-fork.decision.md:45-57), whose managed bullet describes
what the gate offers. The gate only reaches that code after
renderConfigSummary (fork.js:168), i.e. only when a config exists and
validates. "Enrolled, but the merged config no longer validates" falls to the
first-run path before the gate has anything to summarise, and 0129 does not
cover it. This is a gap, not a contradiction the code introduces.

c. Closing it is a design decision reserved to the maintainer, and CLAUDE.md
forbids closing it silently. Both candidate resolutions from issue #605
(preset pathway = 'scoped', or show the returning gate with a "config
invalid" diagnostic) change which screen the user sees. LLP 0129 is Accepted,
so per CLAUDE.md it is settled: the change wants a new LLP that @refs 0129,
not an edit to 0129 and not a silent code change under an existing anchor.

d. The annotation is honest. The new @ref LLP 0129#join-before-picker [implements] at index.js:93 claims only the locking behavior, which it does
implement, and the comment two lines above it discloses "The pathway stays
unset, so the fork still runs." It does not claim to implement #fork.

Recommendation for triage: classify as a preference / follow-up, not a
blocker on #607. It warrants a new LLP (or issue) titled roughly "the returning
gate on an enrolled machine whose merged config no longer validates," carrying
#605's open design question forward so it is not lost when #605 closes.

---

2. Findings new in round 2 (all fixed in aebb3f2)

Round 1 fixed one stale caller-enumeration docstring (computeCentralLockedSources
in join.js). The same class of finding was live in two more places round 1 did
not reach, both in files this PR modifies.

Low (fixed): computeLockedSafe's docstring named only the scoped re-entry

src/core/cli/wizard/index.js:398 read "The scoped re-entry's locked-set
computation, guarded". This PR adds a second call site at index.js:95 (the
managed first-run path), so the doc's naming of its one caller was false the
moment the diff landed. Same living-docs rule round 1 applied one call frame
deeper, in the function computeLockedSafe itself wraps.

Fixed: now "for every entry that reaches the picker on an already-managed
machine without a join (the scoped re-entry, and the first-run path a managed
machine falls to when its merged config no longer validates)". Verified with
git show HEAD:src/core/cli/wizard/index.js (lines 397-408).

Low (fixed): the ReturningGateResult.managed contract still tied managed to the merged config

src/core/cli/wizard/types.d.ts:55 read:

/** True when the merged config carries a central layer (LLP 0031). */

Decoupling managed from the merged config's existence and validity is the
entire substance of this PR, so the published type contract now contradicts
the implementation: after fork.js:161, managed is true when a central layer
is on disk even when there is no merged config to carry anything. This one
matters more than a comment because types.d.ts is what npm run build:types
emits to package consumers.

Fixed: the contract now reads "True when a central layer is on disk (LLP
0031), independently of whether the merged config currently exists or
validates". Verified with git show HEAD:src/core/cli/wizard/types.d.ts
(lines 53-63).

Informational (round 1's #3, now resolved in place rather than left open)

Round 1 correctly noted that test/core/cli/wizard/fork.test.js:143 asserts
{configExists: false, hasCentral: true}, a pairing collectHypAwareStatus
cannot emit: src/core/daemon/status.js:284,302 sets configExists from a
non-null effective config, and mergeConfigLayers always returns a non-null
effective once a central layer loaded (src/core/config/merge.js:42-88).
Round 1 requested no change, which left a test whose name asserts a production
state that does not occur.

Rather than leave that for triage, I recorded the fact in the test itself:
a comment now states the pairing is defensive coverage of the || guard's
other branch, not a collector-emitted state, pinned so a future rewrite cannot
make managed depend on configExists again. The test is unchanged. Verified
with git show HEAD:test/core/cli/wizard/fork.test.js (lines 143-156).

---

3. Everything else I checked at this head

• Delta since a8594ef is fd15e15, doc-only in join.js. Its new prose
  is accurate: computeCentralLockedSources has exactly the callers it names
  (join.js:112, index.js:85, index.js:95).
• The else if cannot mis-order. It is chained off scoped-reconfigure and
  guarded on gate.action === 'first-run', so scoped runs never double-compute
  and reconfigure (solo) is untouched.
• The precomputed values survive the fork correctly. A local choice keeps
  them; a successful team join overwrites both from join.lockedSources /
  join.managed (index.js:135-136); a failed join continues with them
  intact, which is the right fallback for an already-enrolled machine.
• [] vs undefined.index.js:156 threads with ...(locked ? {locked} : {})
  and [] is truthy, so the computeLockedSafe catch (index.js:406-412) yields
  an empty-but-present locked set: an unlocked picker, matching the guard's
  documented LLP 0132 intent.
• Conventions: no semicolons and no U+2014 anywhere in the changed files,
  no @typedef, no inline import('...') types, type imports root-anchored
  .js. Both @ref LLP 0129#join-before-picker targets resolve to a real
  anchor (llp/0129-init-wizard-fork.decision.md:22).

4. Ground truth

$ npm test (worktree, node_modules symlinked from the main checkout)
1..3378
# tests 3380
# pass 3379
# fail 0
# skipped 1
# duration_ms 15808.088751
$ npm run typecheck
> tsc -p tsconfig.json --noEmit
(exit 0)

Run twice: once before my commit and once on the committed tree at aebb3f2.
Identical numbers both times.

5. What I changed

Commit	File	Finding
aebb3f2	src/core/cli/wizard/index.js	computeLockedSafe doc names its second caller
aebb3f2	src/core/cli/wizard/types.d.ts	ReturningGateResult.managed contract decoupled from the merged config
aebb3f2	test/core/cli/wizard/fork.test.js	the unreachable-pairing test says so in place

Doc and comment only, no behavior change. Pushed to fix/issue-605
(fd15e15..aebb3f2). No change to the PR body, no merge.

Residual findings

No residual findings against the code in this PR. Both round-2 findings are
fixed and verified in the committed tree, and round 1's informational item is
resolved in place.

The one open item is not a finding against #607 and should not block it:
LLP 0129 does not decide what the returning gate does on an enrolled machine
whose merged config no longer validates, so such a machine is still offered the
"Join a team" fork. This is byte-identical on master (see section 1a), the PR
improves rather than worsens the state, and #605 explicitly reserves the
decision for the maintainer. It wants a new LLP @refing 0129, tracked
separately, not a silent code change here.

[philcunliffe]

Triage complete at head aebb3f2. Independently re-verified round 2's central claim by diffing fork.js/index.js against origin/master directly: action is byte-identical on every branch of the fork.js diff, and index.js's change is a purely additive else if that leaves pathway unset, so the fork ran for this machine state on master too. The claim holds, so the LLP 0129 #fork gap (an enrolled machine with an invalid merged config still sees the fork) is pre-existing on master, not introduced or worsened by this PR. Full npm test (3379 pass / 0 fail / 1 skipped) and npm run typecheck (exit 0) reproduced independently at this head. No residual code findings; both rounds' doc/type findings are fixed and verified in the committed tree. Opened #612 to carry the LLP 0129 design gap forward as a design question (not a code bug) so it is not lost when #605 closes.