Repository navigation
Description
Problem
mcpxDaemonAutostart() in packages/core/src/tool/plugin/mcpx.ts (lines 102–119 on main 8dae425bda) is deliberately fail-open: only an explicit daemon.autostart = false from mcpx settings get daemon.autostart blocks ensureDaemon() from spawning. Every failure of the settings call itself — non-zero exit, missing binary (ENOENT), the 5s execFile timeout — lands in the catch and returns true, and unparseable output falls through ?? true. That is the documented contract (comment at lines 94–101): a broken/old mcpx must keep today's spawn behaviour, because the setting defaults to true.
That fail-open contract has no test at the ensureDaemon level:
packages/core/test/mcpx-daemon-autostart.test.tscovers the pure parser (parseMcpxDaemonAutostart, line 245), the env opt-outs, and the explicittrue/falsesettings answers viafakeMcpxWithAutostart(which always exits 0).- The plain
fakeMcpx(lines 61–77) answers every invocation — including the settings call — with exit 0 and empty stdout, so the spawn test only implicitly exercises the unparseable-output branch. - No fake ever makes the settings call fail: there is no variant that exits non-zero for
settings, noOPENCODE_MCPX_BINpointing at a nonexistent path, and no hanging settings call hitting the 5s timeout.
Why it matters
A regression that treats a settings error as false (e.g. resolving the exec callback's err branch to the parsed stderr, or dropping the catch) would silently disable daemon autostart for every user whose mcpx is old, broken, or slow to answer — the same silent-disable failure shape as #55, and the inverse of the #134 bug this code was written to fix. Nothing in CI would catch it: all current tests would still pass.
Acceptance criteria
- A test where the fake mcpx's
settings get daemon.autostartexits non-zero assertsensureDaemon()still spawns the daemon (record file written with the usual spawn args). - A test with
OPENCODE_MCPX_BINpointing at a nonexistent binary assertsensureDaemon()still attempts the spawn path / does not treat the missing binary as autostart=false (assert at whatever level is observable without a real binary). - (Stretch) A settings call that never answers is covered without waiting the full 5s in CI — e.g. by making the timeout injectable for tests — or the timeout case is explicitly documented in the test file as untested-by-design.
- Existing tests in
mcpx-daemon-autostart.test.tsstill pass; new tests follow the existingfakeMcpx*helper pattern and keep the WindowsskipIfconventions used by the neighbouring spawn tests.
Found by pr-diff-review of PR #140 and recorded as an unfiled REVIEW-FINDING in the sweep backlog; re-verified against current main this run (function, fakes, and test list all as described above). Single-repo: the daemon.autostart spawn policy exists only in this repo's mcpx plugin (no equivalent in openchamber or code).
Activity
test(core): cover mcpx daemon autostart fail-open branches (#186)
docs: record the #178 and #175 completions in ROADMAP Done (#203)