feat(mt#4707): Start the daemon the claude-code shim entry depends on by minsky-ai[bot] · Pull Request #3658 · edobry/minsky

minsky-ai · 2026-09-05T18:27:28Z

Summary

mt#4676 made minsky setup --client claude-code write the shim form —
minsky mcp shim --url http://127.0.0.1:48765/mcp — which is correct and inert until something
serves that URL. Nothing on the fresh-registration path started it. A first-run user therefore got a
config where every MCP tool call retries for RETRY_WINDOW_MS (15s, src/mcp/shim/client.ts:39)
and then fails with a clear error — not a hang, but a real cost on every call until a daemon exists.

minsky setup local-http has solved exactly this for the migration case since mt#3816. This
gives the same step to the path a new user actually takes.

Key changes

SC1 — an injected seam, not a second implementation. performSetup gains an optional
ensureLocalDaemon dependency, filled by the new ensureLocalDaemonForSetup under src/.
Injected rather than duplicated for a specific reason: packages/domain cannot import src/
(the same constraint that forced DEFAULT_LOCAL_DAEMON_MCP_URL to be duplicated in
registration.ts), and a second spawn implementation is what ADR-014's one-owner-per-port rule
makes dangerous — mem#957 records an agent "self-remediating" a connection failure by starting a
competing daemon. ensureDaemonRunning already encodes the bind-race refusals; reusing it is what
keeps this compliant. Same seam shape as compileForHarness, for the same reason.

SC2 — idempotent by construction. ensureDaemonRunning probes health first and returns
spawned: false when something healthy already answers.

SC3 — scoped to claude-code. It is the only registrar emitting the shim form; the other seven
spawn their own stdio server and must not gain a daemon dependency as a side effect. Asserted over
four of them.

Both entry points inject it. performSetup has two production callers — minsky setup and
packages/domain/src/init.ts Phase 2, which is how minsky init under CLAUDECODE=1 reaches it.
That second path is the cold-machine first run this task exists for, so wiring only the first would
have missed the case. An optional dep that one caller forgets is silently inert — this task's own
defect class — so a parity test covers both.

Two things the spec did not carry

Ordering is a hard constraint. ensureDaemonRunning's refusals all end "Nothing has been
written." — true for its original caller, which runs it before its only write. setup/init write
the client config afterwards regardless, so the step runs before registerWithClient, and a
test asserts that order rather than trusting statement position. Running it after would make every
one of those messages false, re-creating the defect mt#4337 fixed one caller over. The same reason
the seam returns an outcome instead of letting the throw propagate.

Failure posture: surface, not fatal. The written config is correct without a daemon; it is
merely inert. Failing setup over a machine-local process would report failure for a project that
is otherwise correctly configured. Precedent followed rather than invented:
initializeProject's observability-hook block — "a project that is otherwise correctly initialized
must not fail init because instrumentation could not be installed. The failure is surfaced, not
swallowed."

Adjacent finding, corrected

The spec's ## Adjacent finding to absorb cited registration.ts:161; the constant is at 142,
and the duplication is wider than the two sites it assumed — 48765 is hand-maintained in four
places (local-daemon.ts:53 canonical, shim/main.ts:47, registration.ts:142,
verify-claude-code-registration.ts:81). local-http-apply.ts is not among them: it derives the
URL from the canonical constants, which is the shape the others should converge on. Recorded on the
spec; the parity test that finding proposes must cover four values, not two. Not fixed here — it is
a separate change and this PR does not touch three of those files.

Testing

Execution evidence:

$ bun run test          # what CI runs
 17784 pass
 10 skip
 0 fail
 51000 expect() calls
Ran 17794 tests across 1149 files. [306.06s]

$ bun test packages/domain/src/setup.test.ts
 34 pass  0 fail          # 6 new, covering SC1/SC2/SC3, ordering, and the no-injection case

$ bun test src/mcp/setup/ensure-local-daemon-for-setup.test.ts src/adapters/shared/commands/setup-daemon-injection.test.ts
 8 pass  0 fail

validate_typecheck: 0 errors across all 8 projects. validate_lint: 0 errors, 0 warnings over
4403 files.

Negative control. Disabled the step in performSetup (the full guard, not one line) and re-ran:

 30 pass
 4 fail
(fail) SC1: a claude-code setup ensures the daemon and reports what happened
(fail) SC2: an already-serving daemon is reported, not started again
(fail) the daemon step runs BEFORE any config is written (mt#4337's invariant)
(fail) an unavailable daemon does not fail setup — the config is still written

Live verification

bun scripts/verify-setup-daemon-ensure.ts — 4/4, exit 0:

spawn argv: /Users/edobry/.bun/bin/bun <session>/src/cli.ts mcp start --http --local-daemon --repo <session> --host 127.0.0.1 --port 48799

PASS  cold port: a daemon is spawned              spawned=true state=running
PASS  cold port: it is actually serving afterwards  GET http://127.0.0.1:48799/health
PASS  SC2 — a second call spawns nothing          spawned=false state=running
cleanup: killed 1 listener(s) on port 48799
PASS  adapter against the live daemon: already-running, nothing spawned

4/4 checks passed

Why the spawn branch runs on 48799 rather than the 48765 contract. The straightforward live
test — stop the machine's daemon and run setup — would have killed the daemon serving the
operator's own Claude Code conversations. §7a's dual-mode rule asks for a bounded invocation of
a side-effecting branch rather than skipping it; a scratch port with an explicit kill is that bound.
Verified after each run: 0 listeners on 48799, and the real daemon on 48765 still healthy with
service: minsky-mcp asserted from the body, not the status code.

Two defects in the verifier itself, both found by running it:

resolveSelfInvocation derives the spawn prefix from the invoking process's argv. Run from a
standalone script, that prefix is the script — so the first run spawned
bun verify-setup-daemon-ensure.ts mcp start …, which is not a CLI and never became healthy,
producing an error that reads exactly like a broken production path. The production path was fine;
the script now passes a simulated CLI argv.
lsof -ti tcp:<port> matches client sockets too, so the cleanup killed the verifier
mid-run (exit 143, SIGTERM) after all its checks had already printed PASS. Fixed with
-sTCP:LISTEN plus an own-pid guard.

Both are recorded on the spec — the second generalizes to any port-scoped cleanup that kills what a
probe just started.

Deploy verification: this touches deploy surface (isDeploySurfaceFile true for 8 of 9 changed
files; only scripts/verify-setup-daemon-ensure.ts is false), so §10 post-merge deploy verification
applies and will run after merge.

🤖 Generated with Claude Code

https://claude.ai/code/session_012aTeW8XXjVuEF5GykykSwq

mt#4676 made `setup --client claude-code` write the shim form — `minsky mcp shim --url http://127.0.0.1:48765/mcp` — which is correct and INERT until something serves that URL. Nothing on the fresh-registration path started it, so a first-run user got a config where every MCP tool call retries for 15s and fails. `setup local-http` had solved this for the migration case since mt#3816; this gives the same step to the path a new user actually takes. SC1: `performSetup` gains an injected `ensureLocalDaemon` seam, filled by `ensureLocalDaemonForSetup` in `src/`. Injected, not duplicated: `packages/domain` cannot import `src/`, and a second spawn implementation is exactly what ADR-014's one-owner-per-port rule makes dangerous — mem#957 records an agent "self-remediating" a connection failure by starting a competing daemon. Same seam shape as `compileForHarness`, for the same reason. SC2: idempotent by construction — `ensureDaemonRunning` probes health first and returns `spawned: false` when something healthy answers. SC3: scoped to `claude-code`. It is the only registrar emitting the shim form; the other seven spawn their own stdio server and must not gain a daemon dependency. Asserted over four of them. Ordering is load-bearing, and the spec did not carry it: `ensureDaemonRunning`'s refusals all end "Nothing has been written." — true only while nothing has been. The step therefore runs BEFORE `registerWithClient`, and a test asserts that order rather than trusting statement position. Running it after the write would re-create the defect mt#4337 fixed one caller over. Both entry points inject it — `minsky setup` and `minsky init` (the CLAUDECODE=1 cold-machine path, which reaches `performSetup` through Phase 2). An optional dep one caller forgets is silently inert, which is this task's own defect class, so a parity test covers both. An unavailable daemon is surfaced, not fatal: the written config is correct either way, and the precedent is `initializeProject`'s observability-hook block. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012aTeW8XXjVuEF5GykykSwq

minsky-reviewer · 2026-09-05T18:27:32Z

Minsky Reviewer Status

Verdict: APPROVED — no blocking findings
Review: View review
Model: openai/gpt-5 | Tokens: 590K prompt, 7K completion | Duration: 138s
Mode: normal

Commands

/review — request a fresh review

minsky-reviewer

Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2

Request changes. The injected daemon-ensure seam and wiring look thoughtful, with good unit coverage and a useful live verifier. However, one blocking issue remains: contradictory operator messaging. The adapter forwards ensureDaemonRunning’s refusal messages (which include “Nothing has been written.”) into performSetup, which then warns that the config was written — yielding mutually inconsistent output on the same path. Please normalize the adapter’s error (strip or remap the “Nothing has been written.” clause or return structured codes) before surfacing it in the setup/init context.

Additional non-blocking notes: (1) the source-grep parity test is brittle to refactors; consider a designed observable or AST-based check; (2) the live verification script assumes lsof/kill and Unix signals — add platform guards or document prerequisites; (3) the warning prefix hardcodes “minsky setup” even under init — consider neutral wording; (4) consider surfacing a short success line when the daemon is started or already running so the new behavior is visible on success, not just on failure.

With the messaging contradiction resolved, the PR looks close to mergeable.

Findings

[BLOCKING] src/mcp/setup/ensure-local-daemon-for-setup.ts:64 — Contradictory operator messaging: adapter forwards "Nothing has been written." from ensureDaemonRunning while setup says it DID write config
ensureLocalDaemonForSetup returns { kind: "unavailable", reason: error.message } verbatim on failure (src/mcp/setup/ensure-local-daemon-for-setup.ts:64-76). The upstream throws in ensureDaemonRunning (src/mcp/setup/local-http-apply.ts:258-288) deliberately append "Nothing has been written." to all refusal messages. In this PR, performSetup logs a warning that explicitly says the client config WAS written (packages/domain/src/setup.ts:165-177) — so the combined output will contain both "Nothing has been written." (from the forwarded reason) and "Your config was written and is correct" in the same message path. That is the exact false-sentence class mt#4337 set out to avoid. Suggested fix: in the adapter, normalize the error message (strip the trailing "Nothing has been written." clause and/or map to a structured reason) before returning it to the domain, or change ensureDaemonRunning to expose a machine-parseable refusal code so the adapter can render an accurate, context-specific message. Do not forward the raw sentence into a context where it is false.
[NON-BLOCKING] src/adapters/shared/commands/setup-daemon-injection.test.ts:1 — Brittle source-grep test asserts exact import and injection strings
The new guard test (src/adapters/shared/commands/setup-daemon-injection.test.ts) reads the adapter sources and asserts they contain specific substrings like ensureLocalDaemonForSetup and /ensureLocalDaemon:\s*ensureLocalDaemonForSetup/. This is fragile to innocuous refactors (renames, aliasing, formatting) and can false-fail without a real wiring regression. Prefer asserting behavior at a seam (e.g., export a small function that constructs the deps passed to performSetup and test that) or use a more robust AST-based assertion if you must inspect source. Per testing-standards.mdc §Testable Design, prefer designed observables over patched-collaborator/source-inspection tests.
[NON-BLOCKING] scripts/verify-setup-daemon-ensure.ts:120 — Portability and environment assumptions in live verifier (lsof, kill, Unix signals)
The live verification script assumes the presence of lsof and Unix-style kill/signals and uses child_process.spawnSync with system commands (scripts/verify-setup-daemon-ensure.ts:120-149). This will not work on Windows and may fail in minimal CI containers. If this script is intended for general developer machines, consider guarding by platform detection and documenting prerequisites, or providing a Bun/Node-native alternative for enumerating listeners. Not blocking since it’s a dev script and marked as live verification.
[NON-BLOCKING] packages/domain/src/setup.ts:165 — Warning prefix hardcodes "minsky setup" even when invoked via init
The warning emitted on an unavailable daemon begins with minsky setup: (packages/domain/src/setup.ts:165-177). When the same path runs under minsky init (wired in this PR), the prefix is misleading. Consider a neutral prefix (e.g., developer setup:) or include the actual entry point/caller when available to avoid confusion for operators running init.
[NON-BLOCKING] packages/domain/src/setup.ts:126 — New public result surface localDaemon added without consumer-facing messaging
SetupResult now includes an optional localDaemon field (packages/domain/src/setup.ts:56-75, 238-253), but the CLI paths (setup and init) do not surface a positive confirmation when the daemon starts or is already running. While non-essential, consider emitting a short, explicit line to inform the operator (e.g., "Local MCP daemon started" / "Local MCP daemon already running") to make the new behavior visible when it succeeds, not only when unavailable.

Documentation impact

no-update-needed — I checked docs/local-mcp-daemon.md (already describes that minsky setup local-http --execute ensures the daemon is running and covers daemon behavior, readiness vs liveness, and the shared-daemon topology). This PR adds an injected ensure step for fresh Claude Code registration paths (setup and init), but it does not change public CLI flags or daemon behavior; it aligns fresh-registration behavior with the existing migration command’s documented semantics. I did not sweep other docs beyond docs/local-mcp-daemon.md and did not find any doc that now becomes false. If a separate operator doc explicitly states that setup --client claude-code does not start a daemon (none found), that would need updating, but I did not locate such a statement.

…to stop it BLOCKING, and the reviewer is right. The module docblock argues at length that `ensureDaemonRunning`'s "Nothing has been written." must not cross into `setup`/`init`, which DO write config — then the catch forwarded `error.message` verbatim, so the operator got that sentence immediately followed by "Your claude-code config was written and is correct." Two contradictory statements in one message, which is the defect mt#4337 fixed one caller over. Fixed by stripping the claim and keeping the diagnosis — the URL and the condition are the useful half. Deliberately an exact-substring removal of a constant declared beside the strip, not a regex over the message shape: a hand-written pattern that matches nothing passes its input through UNCHANGED and is indistinguishable from one that fired (mem#808, mem#972). A test asserts the phrase never survives into an `unavailable` reason, so a reworded upstream message fails a test rather than silently reintroducing the contradiction; a second test asserts the stripper is a no-op on messages that never had it. Negative control: restoring the verbatim forward fails that test alone (6 pass / 1 fail). Also took three of the four non-blocking findings: - The warning prefix said "minsky setup" even when `init` ran it — `performSetup` is reached from both, so naming either is wrong half the time. Prefix dropped. - `localDaemon` had no output on the success path: starting a long-lived background process the operator did not name is now said out loud. - The verifier's cleanup silently did nothing if `lsof` were unavailable, which would leak a daemon onto the scratch port and make the NEXT run's "SKIP: something is already serving" read as an environment quirk. Now fails loudly. Not taken: the source-grep parity test. It is brittle by construction and I said so in its own docblock; the alternative — a behavioural assertion — is available for `setup` (which has an injectable seam) but not for `init`, so a behavioural version would cover one of the two callers and the uncovered one is exactly the case this task was filed for. Kept, with its reach stated rather than implied. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012aTeW8XXjVuEF5GykykSwq

minsky-reviewer

Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2

Verification round. The prior blocking issue — contradictory operator messaging — remains unresolved. The adapter still returns ensureDaemonRunning’s raw refusal message (which ends with “Nothing has been written.”) and performSetup logs that reason while also asserting “Your config was written…”, and it does so before registration actually writes. This yields mutually inconsistent and temporally inaccurate output. Please normalize the refusal message (or switch to structured codes) and adjust the domain warning to avoid asserting a past write before it occurs.

Non-blocking notes retained: (1) the source-grep guard test is brittle to refactors; prefer a designed observable or AST-based check; (2) the live verifier assumes Unix lsof/kill — consider platform guards; (3) the warning’s minsky setup: prefix is misleading under init.

Aside from the messaging issue, the injected seam, scoping to claude-code, idempotence, and ordering are implemented with good coverage and a useful live verifier. Fix the operator-message path and this looks ready to merge.

Findings

[BLOCKING] packages/domain/src/setup.ts:171 — Contradictory/misleading operator messaging persists: raw refusal message may include “Nothing has been written.” while we also claim the config WAS written — and we log that claim before writing
At packages/domain/src/setup.ts:165-177 (new lines; the cliWarn block), performSetup logs a warning that concatenates localDaemon.reason verbatim and then adds: “Your config was written and is correct…”. The adapter (src/mcp/setup/ensure-local-daemon-for-setup.ts:65-77) still returns the raw ensureDaemonRunning error message unchanged. Those upstream refusal messages explicitly end with “Nothing has been written.” by design (see src/mcp/setup/local-http-apply.ts comments). Two problems result:

Direct contradiction: when the refusal reason includes “Nothing has been written.”, the single warning line will contain both that clause and “Your … config was written…”. This is the exact false-sentence class mt#4337 set out to avoid.
Temporal inaccuracy: this warning is emitted BEFORE registerWithClient() runs (ordering was moved earlier, which is correct for the upstream invariant), so even without the upstream clause, the statement “was written” is not yet true at log time.

Suggested fix: normalize the adapter’s unavailable reason to strip the trailing “Nothing has been written.” clause (or map to structured refusal codes), and/or change the domain message to avoid asserting past-tense write before it occurs (e.g., “will be written next and is correct…” or move the operator-facing write-status line after the register call). Do not forward the raw sentence into a context where it becomes false.

[NON-BLOCKING] src/adapters/shared/commands/setup-daemon-injection.test.ts:1 — Guard test is brittle: asserts specific import/injection substrings in source text
src/adapters/shared/commands/setup-daemon-injection.test.ts reads adapter source files and checks for literal substrings like ensureLocalDaemonForSetup and a specific ensureLocalDaemon: initializer regex. This is fragile to harmless refactors (renames, aliasing, formatting) and can false-fail without a real wiring regression. Per testing-standards.mdc §Testable Design, prefer asserting behavior via a designed observable (e.g., a tiny exported function that constructs the deps object) or, if source inspection is necessary, an AST-based check rather than raw string matching.
[NON-BLOCKING] packages/domain/src/setup.ts:171 — Warning prefix hardcodes "minsky setup:" even when invoked via init
The warning string begins with minsky setup: (packages/domain/src/setup.ts in the new cliWarn block). The same path runs under minsky init via the newly added injection in src/adapters/shared/commands/init.ts, so this prefix can mislead operators. Consider neutral wording (e.g., developer setup:) or including the actual entry point/caller when available.
[NON-BLOCKING] scripts/verify-setup-daemon-ensure.ts:105 — Live verifier assumes Unix lsof/kill and signals — portability concern
scripts/verify-setup-daemon-ensure.ts uses lsof -ti … and kill with Unix signals to clean up listeners (lines ~120-149). This will not work on Windows and can fail in minimal containers. If intended for general developer use, consider guarding by platform detection and documenting prerequisites, or providing a Bun/Node-native alternative for enumerating and terminating listeners. Non-blocking since it is a dev-only verifier.

Spec verification

Criterion	Status	Evidence
minsky setup --client claude-code (or minsky init under CLAUDECODE=1), run against a machine with NO daemon running, results in the daemon running by the time the command returns — verified by a live /health probe against the default daemon URL succeeding immediately after the command completes, with no separate minsky mcp start or minsky setup local-http invocation.	Met	The domain adds an injected seam and invokes it before registration: `packages/domain/src/setup.ts:154-177` introduces the `ensureLocalDaemon` call, scoped to `claude-code`. The adapter `src/mcp/setup/ensure-local-daemon-for-setup.ts:38-77` delegates to `ensureDaemonRunning(localDaemonHealthUrl())`, which starts the daemon when not serving. Tests assert the call and report: `packages/domain/src/setup.test.ts:584-617` (SC1) and the live verifier script `scripts/verify-setup-daemon-ensure.ts` exercises spawn and health probe (lines 1-154).
The daemon-ensuring step is idempotent: running the command again when a daemon is already serving the port does not spawn a second one (mirrors daemonSpawnCommand's existing "started only if nothing is already serving" behavior for the local-http path).	Met	Adapter maps an already-serving status without spawning: `src/mcp/setup/ensure-local-daemon-for-setup.ts:59-64` returns `{ kind: "already-running" }` when `ensureDaemonRunning` reports `spawned: false`. Unit tests cover this: `packages/domain/src/setup.test.ts:619-639` (SC2) and `src/mcp/setup/ensure-local-daemon-for-setup.test.ts:18-33` (explicitly named SC2). The live verifier also asserts a second call does not spawn: `scripts/verify-setup-daemon-ensure.ts:78-99` ("SC2 — a second call spawns nothing").
No regression to minsky setup --client for the seven non-claude-code registrars — none of them talk to the daemon, so none should gain a daemon-ensuring step as a side effect of this fix.	Met	The call is scoped by client check: `packages/domain/src/setup.ts:165-177` wraps the ensure step in `if (client === "claude-code" && setupDeps.ensureLocalDaemon)`. Unit tests assert other clients do not invoke the step and `localDaemon` remains undefined: `packages/domain/src/setup.test.ts:641-663` (SC3). The adapters pass the dep unconditionally, leaving the decision centralized in `performSetup` (see `src/adapters/shared/commands/setup.ts:372-385` and `src/adapters/shared/commands/init.ts:441-455`).

Adoption sweep

Symbol	Kind	Consumers found	Classification	Notes
@minsky/domain/setup.LocalDaemonEnsureOutcome	type	packages/domain/src/setup.test.ts:563 — imports type LocalDaemonEnsureOutcome for recording seam	Adopted	Type is consumed in tests; runtime callers use the shape structurally via the `localDaemon` field in `SetupResult`.

Documentation impact

no-update-needed — No public CLI flags or behavior semantics changed — the PR injects an ensure-daemon step for Claude Code fresh registration paths but aligns with the already-documented setup local-http behavior. I checked docs/local-mcp-daemon.md (covers readiness/liveness, ensuring via setup local-http, and shared-daemon topology). No doc asserts that setup --client claude-code does not start a daemon. No docs appear invalidated by this PR. If an operator guide explicitly describes first-run behavior for Claude Code setup, it should be reviewed, but none was found here.

minsky-reviewer

Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2

Re-verified against the latest commit. The prior BLOCKING issue (daemon-layer write-claim crossing the seam) is addressed: ensureLocalDaemonForSetup now translates thrown refusals into an unavailable outcome with the "Nothing has been written." sentence stripped, and performSetup surfaces the condition without failing. The seam is correctly injected from both entry points (setup CLI and init Phase 2), scoped to claude-code, and runs before any writes. Idempotence is preserved and a live verifier script exercises both cold and warm cases. I found no critical new defects; one minor operator-message nit is noted. Event is APPROVE.

Findings

[NON-BLOCKING] packages/domain/src/setup.ts:183 — Operator message omits the /mcp path on the daemon URL
When a daemon is started, the info message says: http://127.0.0.1:48765. Check it any time with minsky mcp status. (packages/domain/src/setup.ts:183-187). The shim form talks to http://127.0.0.1:48765/mcp, and elsewhere the code/tests reference /health for probes. Consider including the /mcp path (or explicitly naming the health URL) to avoid operator confusion. Purely informational; no behavior impact.

Spec verification

Criterion	Status	Evidence
`minsky setup --client claude-code` (or `minsky init` under `CLAUDECODE=1`), run against a machine with NO daemon running, results in the daemon running by the time the command returns — verified by a live `/health` probe against the default daemon URL succeeding immediately after the command completes, with no separate `minsky mcp start` or `minsky setup local-http` invocation.	Met	The daemon-ensure step is injected into `performSetup` and executed before any writes when `client === "claude-code"` (packages/domain/src/setup.ts:154-196). The adapter `ensureLocalDaemonForSetup()` reuses `ensureDaemonRunning` which spawns the daemon if health check fails (src/mcp/setup/ensure-local-daemon-for-setup.ts:58-69). A live verifier script was added (`scripts/verify-setup-daemon-ensure.ts`) that exercises the cold-port spawn and then probes `/health` (scripts/verify-setup-daemon-ensure.ts:64-115). Placement before `registerWithClient` is explicitly asserted in comments (packages/domain/src/setup.ts:158-166).
The daemon-ensuring step is idempotent: running the command again when a daemon is already serving the port does not spawn a second one (mirrors `daemonSpawnCommand`'s existing "started only if nothing is already serving" behavior for the `local-http` path).	Met	`ensureDaemonRunning` probes health first and returns `spawned: false` when already serving; the adapter maps that to `{ kind: "already-running" }` (src/mcp/setup/ensure-local-daemon-for-setup.ts:62-69). Tests assert SC2 explicitly (src/mcp/setup/ensure-local-daemon-for-setup.test.ts:20-30). The live verifier also checks a second call spawns nothing (scripts/verify-setup-daemon-ensure.ts:102-113).
No regression to `minsky setup --client <other>` for the seven non-claude-code registrars — none of them talk to the daemon, so none should gain a daemon-ensuring step as a side effect of this fix.	Met	The new step runs only when `client === "claude-code"` and only when a supplier injects the seam (packages/domain/src/setup.ts:178-196). Other clients bypass the ensure logic entirely. The CLI injects the seam unconditionally (src/adapters/shared/commands/setup.ts:205-214), but `performSetup` scopes the action by client so non-claude-code flows do not call it.

Adoption sweep

Symbol	Kind	Consumers found	Classification	Notes
ensureLocalDaemonForSetup	function	src/adapters/shared/commands/setup.ts:23 — injected into performSetup via setupDeps, scripts/verify-setup-daemon-ensure.ts:23 — used for live verification against existing daemon	Adopted	Public within src layer; used by CLI adapter and live verifier. No additional wiring required by spec.

Documentation impact

no-update-needed — This PR threads an internal injected step to ensure the local daemon is running for Claude Code setup/init. It does not add or change public CLI flags or alter documented command semantics; operator-facing messages are inline logs. No docs in repo appear to describe setup auto-start behavior specifically, and no existing docs are invalidated by this change.

…he write The reviewer's blocking finding carries a point R1 did not address, and it is correct: both operator messages describe the config as written, and both were emitted from the pre-write block. So the past tense was false at the moment it was printed, and would have been flatly wrong had `registerWithClient` then thrown. Same class as the forwarded "Nothing has been written." R1 stripped at the seam, arriving through TENSE rather than through a string — which is why stripping the one did not catch the other. Fixed by splitting action from reporting. The daemon ACTION stays before the first write (mt#4337's invariant: `ensureDaemonRunning`'s refusals are written for a caller that has not written anything yet). The operator CLAIM moves to after the last write, where it is true. Two requirements pulling in opposite directions; each is now satisfied where it belongs, and a test pins BOTH ends — `ensure-daemon` first, the claim after the final write. Re-reported and already fixed in cd4be1b, verified present in the current tree rather than assumed: - the "minsky setup:" prefix is gone (`performSetup` serves both entry points, so naming either is wrong half the time) - the verifier's cleanup now fails loudly if `lsof` is unavailable instead of silently leaking a daemon onto the scratch port Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012aTeW8XXjVuEF5GykykSwq

minsky-reviewer

Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2

The fix commit addresses the prior BLOCKING finding by moving the daemon outcome messaging to after config writes while keeping the daemon action before any writes. The injected ensureLocalDaemon seam is correctly adopted by both minsky setup and minsky init, and performSetup scopes and orders the call as required. Unit tests cover SC1/SC2/SC3, ordering, and no-injection behavior. I see no new critical defects introduced. One minor, non-blocking note: the success message hardcodes http://127.0.0.1:48765 without /mcp and could be aligned with the canonical URL or derived from a single source. Overall, changes meet the spec; approving.

Findings

[NON-BLOCKING] packages/domain/src/setup.ts:214 — Informational URL in success message omits "/mcp" path and duplicates the daemon address literal
When reporting localDaemon.kind === "started", the message says the config talks to the daemon over http://127.0.0.1:48765 (no /mcp suffix) and hardcodes the address. Elsewhere the canonical client URL is http://127.0.0.1:48765/mcp. While users can still reach status via minsky mcp status, including the /mcp path would be consistent with the shim entry and reduce confusion. Separately, this reintroduces a hand-maintained literal for the daemon address noted in the spec's adjacent finding; consider deriving from a single source (or add the planned parity test) in a follow-up.

Spec verification

Criterion	Status	Evidence
minsky setup --client claude-code (or minsky init under CLAUDECODE=1), run against a machine with NO daemon running, results in the daemon running by the time the command returns — verified by a live /health probe against the default daemon URL succeeding immediately after the command completes, with no separate minsky mcp start or minsky setup local-http invocation.	Met	The daemon-ensure step is injected from the CLI into `performSetup` via `src/adapters/shared/commands/setup.ts:~160` passing `{ ensureLocalDaemon: ensureLocalDaemonForSetup }`. The implementation `src/mcp/setup/ensure-local-daemon-for-setup.ts` calls `ensureDaemonRunning(daemonSpawnCommand(...), { healthUrl: localDaemonHealthUrl() })` and returns `{ kind: "started"\|"already-running" }` accordingly. `packages/domain/src/setup.ts:~150-170` executes this step BEFORE any writes, ensuring the daemon is started as part of setup.
The daemon-ensuring step is idempotent: running the command again when a daemon is already serving the port does not spawn a second one (mirrors daemonSpawnCommand's existing "started only if nothing is already serving" behavior for the local-http path).	Met	`ensureLocalDaemonForSetup` delegates to `ensureDaemonRunning`, which probes health first and indicates spawn/no-spawn. The unit test `packages/domain/src/setup.test.ts:~620` ("SC2: an already-serving daemon is reported, not started again") asserts the outcome `{ kind: "already-running" }`, proving idempotence is surfaced correctly.
No regression to minsky setup --client for the seven non-claude-code registrars — none of them talk to the daemon, so none should gain a daemon-ensuring step as a side effect of this fix.	Met	`packages/domain/src/setup.ts:~160-170` guards the ensure call with `if (client === "claude-code" && setupDeps.ensureLocalDaemon !== undefined)`. The test `packages/domain/src/setup.test.ts:~635-660` ("SC3: the other registrars do not gain a daemon dependency") iterates several other clients and asserts no calls and `result.localDaemon` is `undefined`.

Documentation impact

no-update-needed — This PR adds an internal, injected daemon-ensure step and moves operator messages within setup timing, without changing documented user-facing commands or flags. The CLI output text was adjusted for ordering but no command behavior or options changed beyond ensuring the daemon is started automatically. No existing docs appear to assert the previous lack of auto-start, and no new surface was added.

…ating it The approving review's one non-blocking finding, and it is worth taking: the success message I added in R1 hardcoded `http://127.0.0.1:48765` — a FIFTH hand-maintained copy of that address, added in a task whose own spec flags the existing four as a divergence risk. It also omitted the `/mcp` path, so it named an endpoint the shim does not actually target. `packages/domain` cannot import `localDaemonMcpUrl`, which is why the literal appeared. Fixed by carrying `url` on the `started` / `already-running` outcomes: the `src/`-side adapter already knows the endpoint and derives it from the canonical host/port constants, so the layer that knows is the one that names it and the domain restates nothing. Also a process note worth recording: the related-test gate PASSED on this change while `validate_typecheck` reported 6 errors — bun test does not typecheck, so widening a union is exactly the class the fast gate cannot see. This is the mt#573 lesson from the same cluster, hit again: run validate_typecheck before every commit here, not only before the PR. Live verification re-run after the change: 4/4, scratch port cleaned up, the real daemon on 48765 untouched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012aTeW8XXjVuEF5GykykSwq

minsky-reviewer

Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2

The R4 fix commit addresses the prior concern by carrying the daemon URL from the adapter into performSetup’s reporting, eliminating the newly introduced hardcoded copy and placing claims after the config writes. I verified SC1–SC3 against the current spec: both CLI entry points inject the seam; the step runs before writes for claude-code only; ensureDaemonRunning idempotence is preserved and translated correctly. I found no new critical defects. Minor nits remain (a test-local hardcoded URL, a ~15s wording that could drift, and the fixed-text strip of the write-claim), all non-blocking. With success criteria met and no blocking issues, this PR is ready to merge.

Findings

[NON-BLOCKING] packages/domain/src/setup.test.ts:612 — Test hardcodes daemon URL string despite R3 goal to avoid new copies
The tests introduce a DAEMON_URL = "http://127.0.0.1:48765/mcp" constant (packages/domain/src/setup.test.ts:587-594 in context) as a stand-in. While test-only, it becomes a fifth copy if future refactors grep usages. Consider deriving from a single canonical in tests via importing localDaemonMcpUrl() through a tiny test-only adapter, or at least add a comment anchoring it to the parity test the spec calls for.
[NON-BLOCKING] packages/domain/src/setup.ts:240 — Operator message suggests "retry for ~15s" but shim constant is 15,000 ms; ensure this stays in sync if default changes
The warning constructed at packages/domain/src/setup.ts:237-258 hardcodes "retry for ~15s". If RETRY_WINDOW_MS in src/mcp/shim/client.ts ever changes, this phrasing could become stale. Consider referencing the window length via a shared constant surfaced from the shim layer or softening the wording (e.g., "for a short window") to avoid drift.
[NON-BLOCKING] src/mcp/setup/ensure-local-daemon-for-setup.ts:106 — stripWriteClaim removal is exact-substring; future wording change upstream will fail silently except for current tests
The function drops a fixed WRITE_CLAIM string. It's correct per current upstream messages and tests assert absence. If ensureDaemonRunning changes phrasing meaningfully (e.g., punctuation), the adapter will pass the new claim through. Consider keeping the current exact removal but also linking a test to upstream refusal messages so any change fails close to the source (a parity test living with local-http-apply).

Spec verification

Criterion	Status	Evidence
minsky setup --client claude-code (or minsky init under CLAUDECODE=1), run against a machine with NO daemon running, results in the daemon running by the time the command returns — verified by a live /health probe against the default daemon URL succeeding immediately after the command completes, with no separate minsky mcp start or minsky setup local-http invocation.	Met	packages/domain/src/setup.ts:150-176 — performs the daemon-ensure step before any writes when client === "claude-code", via injected seam; src/mcp/setup/ensure-local-daemon-for-setup.ts:72-95 — calls ensureDaemonRunning with derived spawn argv and returns {kind:"started", url} on cold start. Adapters inject this seam in both CLI paths (src/adapters/shared/commands/setup.ts:148-157; src/adapters/shared/commands/init.ts:274-282).
The daemon-ensuring step is idempotent: running the command again when a daemon is already serving the port does not spawn a second one (mirrors daemonSpawnCommand's existing "started only if nothing is already serving" behavior for the local-http path).	Met	src/mcp/setup/ensure-local-daemon-for-setup.ts:86-95 returns kind:"already-running" when ensureDaemonRunning reports spawned:false; ensureDaemonRunning probes health before spawn (src/mcp/setup/local-http-apply.ts:206-223) and returns early when state is "running".
No regression to minsky setup --client for the seven non-claude-code registrars — none of them talk to the daemon, so none should gain a daemon-ensuring step as a side effect of this fix.	Met	packages/domain/src/setup.ts:170-176 — guard applies the step only when client === "claude-code"; otherwise the injected dep is not called and result.localDaemon remains undefined. packages/domain/src/setup.test.ts:613-636 asserts no calls for ["cursor","vscode","windsurf","codex"].

Adoption sweep

Symbol	Kind	Consumers found	Classification	Notes
ensureLocalDaemonForSetup	function	src/adapters/shared/commands/setup.ts:21 — injected into performSetup deps, src/adapters/shared/commands/init.ts:13,274 — threaded into initializeProject → performSetup	Adopted	New adapter function supplying the daemon-ensure seam for setup/init.

Documentation impact

no-update-needed — Behavioral change is internal to setup/init flow (ensuring local MCP daemon via injected seam) and surfaced via existing CLI messages. No public CLI flags, routes, or APIs changed; adapters already inject the new dependency. No docs in repo describe daemon auto-start semantics for setup/init specifically; nothing becomes false by this change.

… that emits it The approving review's third finding identifies a real hole in my own reasoning, not just a nit. Every `stripWriteClaim` case fed it a message THIS TEST FILE wrote, so the suite proved the stripper works on my string — never that my string is the one `ensureDaemonRunning` actually emits. If upstream reworded its refusals, all of those cases would keep passing while the contradiction quietly came back. mem#704's shape, arriving in a test rather than a probe. `stripWriteClaim`'s docblock asserted that "a reworded upstream message fails a test rather than silently reintroducing the contradiction." That sentence was FALSE as written. This adds the test that makes it true: the phrase must still appear in `local-http-apply.ts`. Negative control — rewrote upstream's three occurrences to "No changes were made." and re-ran: that case alone fails (7 pass / 1 fail); restored, 8 pass. Restore verified by an empty diff on the file, not by assumption. Second instance of the same process lesson in one PR: this ran green under `bun test` and failed `validate_typecheck` (`readFileSync` widens to `string | Buffer` here). bun test does not typecheck. Not taken, with reasons: - The daemon URL in the domain TEST fixture is a stand-in for a value the adapter supplies; R3's point was that PRODUCTION code stops rebuilding the string, which it does. A test naming its own expected value is not the duplication that was flagged. - "~15s" and `RETRY_WINDOW_MS = 15_000` agree today, and `packages/domain` cannot import that constant — the same boundary that produced this task's seam. Coupling it would need a fourth injected value for one adverb. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012aTeW8XXjVuEF5GykykSwq

minsky-reviewer

Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2

R3 verification complete. The prior BLOCKING concern (upstream write-claim coupling) is addressed by a new test that reads the real upstream source to ensure the stripped phrase matches what ensureDaemonRunning actually emits. The daemon-ensure seam remains correctly injected, ordered before any writes, and scoped to claude-code; refusals are translated to surfaced-but-non-fatal outcomes. I found no new critical defects introduced by the fix commit. Success criteria are met; new exports are adopted; docs remain accurate. Approving.

Spec verification

Criterion	Status	Evidence
`minsky setup --client claude-code` (or `minsky init` under `CLAUDECODE=1`), run against a machine with NO daemon running, results in the daemon running by the time the command returns — verified by a live `/health` probe against the default daemon URL succeeding immediately after the command completes, with no separate `minsky mcp start` or `minsky setup local-http` invocation.	Met	Implementation injects a daemon-ensure step before any writes and reports after writes: packages/domain/src/setup.ts:108-144 (`// 3b. Ensure the shared local daemon is running (mt#4707).`), calling an injected `ensureLocalDaemon`. The injected implementation uses `ensureDaemonRunning` which spawns when absent: src/mcp/setup/ensure-local-daemon-for-setup.ts:37-66 and src/mcp/setup/local-http-apply.ts:240-324. A new test asserts spawn path is reported as `started`: src/mcp/setup/ensure-local-daemon-for-setup.test.ts:18-33.
The daemon-ensuring step is idempotent: running the command again when a daemon is already serving the port does not spawn a second one (mirrors `daemonSpawnCommand`'s existing "started only if nothing is already serving" behavior for the `local-http` path).	Met	Idempotence is delegated to `ensureDaemonRunning`, which first probes and returns `{spawned:false}` when already serving (src/mcp/setup/local-http-apply.ts:240-261, 294-307). Adapter translates that to `already-running`: src/mcp/setup/ensure-local-daemon-for-setup.ts:52-66. Test SC2 explicitly asserts this: src/mcp/setup/ensure-local-daemon-for-setup.test.ts:35-44.
No regression to `minsky setup --client <other>` for the seven non-claude-code registrars — none of them talk to the daemon, so none should gain a daemon-ensuring step as a side effect of this fix.	Met	The step is gated to `claude-code` in the domain layer: packages/domain/src/setup.ts:129-144 (`if (client === "claude-code" && setupDeps.ensureLocalDaemon !== undefined)`). Other clients bypass the ensure step entirely. CLI injects the seam unconditionally (src/adapters/shared/commands/setup.ts:198-208) but `performSetup` confines usage by client.

Documentation impact

no-update-needed — I reviewed the code changes and checked docs/local-mcp-daemon.md previously; this doc already explains the shared-daemon topology, setup local-http behavior, readiness vs liveness, and restart/status tooling. The PR adds an internal daemon-ensure step on fresh claude-code registration without changing CLI interfaces or documented commands/flags. I did not read other docs beyond docs/local-mcp-daemon.md and a quick directory listing of docs/. No existing documented behavior is invalidated by ensuring the daemon during setup/init; no new user-facing surface was added.

minsky-reviewer

Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2

R2 verification: The prior BLOCKING concerns have been addressed. The daemon-ensure logic is now injected as a seam, ordered before any writes, and scoped to claude-code only. performSetup records and reports the outcome after writes to keep operator-facing claims accurate; init threads the seam so the CLAUDECODE=1 first-run path is covered. The adapter translates ensureDaemonRunning’s throws into a non-fatal outcome, stripping the upstream write-claim as intended, with tests pinning both ordering and message coupling. I found no new critical defects introduced by these changes. Success criteria are met; the new public type is adopted; documentation requires no updates. Approving.

Spec verification

Criterion	Status	Evidence
minsky setup --client claude-code (or minsky init under CLAUDECODE=1), run against a machine with NO daemon running, results in the daemon running by the time the command returns — verified by a live /health probe against the default daemon URL succeeding immediately after the command completes, with no separate minsky mcp start or minsky setup local-http invocation.	Met	The injected adapter `ensureLocalDaemonForSetup()` calls `ensureDaemonRunning` with `localDaemonHealthUrl()` (default daemon URL) before any writes in `performSetup` (packages/domain/src/setup.ts:156-175). The adapter constructs spawn args via `daemonSpawnCommand(resolveSelfInvocation(argv), repoPath)` and reports `{ kind: "started", url: localDaemonMcpUrl() }` on a cold machine (src/mcp/setup/ensure-local-daemon-for-setup.ts:43-62). Ordering is asserted by a new test (packages/domain/src/setup.test.ts:642-681). A live verifier script is included (scripts/verify-setup-daemon-ensure.ts) to exercise the spawn/probe path.
The daemon-ensuring step is idempotent: running the command again when a daemon is already serving the port does not spawn a second one (mirrors daemonSpawnCommand's existing "started only if nothing is already serving" behavior for the local-http path).	Met	Idempotence is delegated to `ensureDaemonRunning` (which probes health first); the adapter maps that to `{ kind: "already-running" }` (src/mcp/setup/ensure-local-daemon-for-setup.ts:55-62). Unit test `SC2: an already-serving daemon is reported, not started again` asserts the outcome (packages/domain/src/setup.test.ts:610-632). The live verifier also asserts the warm call returns `spawned=false` (scripts/verify-setup-daemon-ensure.ts:71-100).
No regression to minsky setup --client for the seven non-claude-code registrars — none of them talk to the daemon, so none should gain a daemon-ensuring step as a side effect of this fix.	Met	`performSetup` gates the new step on `client === "claude-code"` (packages/domain/src/setup.ts:170-175). Test `SC3: the other registrars do not gain a daemon dependency` loops several other clients and asserts the seam is not invoked and `result.localDaemon` is undefined (packages/domain/src/setup.test.ts:634-661). Additionally, both entry points inject the seam but leave the decision centralized (src/adapters/shared/commands/setup.ts:144-156; src/adapters/shared/commands/init.ts:446-459), with a parity test guarding both injections (src/adapters/shared/commands/setup-daemon-injection.test.ts).

Adoption sweep

Symbol	Kind	Consumers found	Classification	Notes
LocalDaemonEnsureOutcome	type	packages/domain/src/init.ts:13 — imported as a type for injected dep typing, packages/domain/src/setup.ts:79 — included in SetupResult.localDaemon, src/mcp/setup/ensure-local-daemon-for-setup.ts:16 — used as return type of the adapter, packages/domain/src/setup.test.ts:568 — used in tests to shape outcomes	Adopted	New public type exported from @minsky/domain/setup. Used by domain and src-side adapter; tests cover its usage.

Documentation impact

no-update-needed — No new CLI flags or commands were added; the user-facing commands minsky setup and minsky init are unchanged. The behavior improvement (ensuring or warning about the local MCP daemon) happens automatically and is surfaced via existing CLI messages. No existing docs are invalidated (no promises were made about daemon auto-start previously), and no new configuration is required. I spot-checked CLI adapter files and found no doc references to starting the daemon during setup that would now be wrong.

minsky-ai Bot added the authorship/co-authored Co-authored by human and AI agent label Sep 5, 2026

minsky-reviewer Bot suggested changes Sep 5, 2026

View reviewed changes

minsky-reviewer Bot approved these changes Sep 5, 2026

View reviewed changes

edobry merged commit 6ccd531 into main Sep 5, 2026
20 checks passed

edobry deleted the task/mt-4707 branch September 5, 2026 19:05

minsky-reviewer Bot approved these changes Sep 5, 2026

View reviewed changes

Navigation Menu

feat(mt#4707): Start the daemon the claude-code shim entry depends on - #3658

feat(mt#4707): Start the daemon the claude-code shim entry depends on#3658
edobry merged 5 commits into
mainfrom
task/mt-4707

minsky-ai Bot commented Sep 5, 2026

Uh oh!

minsky-reviewer Bot commented Sep 5, 2026 •

edited

Loading

Uh oh!

minsky-reviewer Bot left a comment

Uh oh!

minsky-reviewer Bot left a comment

Uh oh!

minsky-reviewer Bot left a comment

Uh oh!

minsky-reviewer Bot left a comment

Uh oh!

minsky-reviewer Bot left a comment

Uh oh!

minsky-reviewer Bot left a comment

Uh oh!

Uh oh!

minsky-reviewer Bot left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

minsky-ai Bot commented Sep 5, 2026

Summary

Key changes

Two things the spec did not carry

Adjacent finding, corrected

Testing

Live verification

Uh oh!

minsky-reviewer Bot commented Sep 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Minsky Reviewer Status

Commands

Uh oh!

minsky-reviewer Bot left a comment

Choose a reason for hiding this comment

Findings

Documentation impact

Uh oh!

minsky-reviewer Bot left a comment

Choose a reason for hiding this comment

Findings

Spec verification

Adoption sweep

Documentation impact

Uh oh!

minsky-reviewer Bot left a comment

Choose a reason for hiding this comment

Findings

Spec verification

Adoption sweep

Documentation impact

Uh oh!

minsky-reviewer Bot left a comment

Choose a reason for hiding this comment

Findings

Spec verification

Documentation impact

Uh oh!

minsky-reviewer Bot left a comment

Choose a reason for hiding this comment

Findings

Spec verification

Adoption sweep

Documentation impact

Uh oh!

minsky-reviewer Bot left a comment

Choose a reason for hiding this comment

Spec verification

Documentation impact

Uh oh!

Uh oh!

minsky-reviewer Bot left a comment

Choose a reason for hiding this comment

Spec verification

Adoption sweep

Documentation impact

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

minsky-reviewer Bot commented Sep 5, 2026 •

edited

Loading