Skip to content

test(core): mcpxDaemonAutostart fail-open branches (settings exec error / missing binary / timeout) have no test #178

Closed

Description

@dezren39

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.ts covers the pure parser (parseMcpxDaemonAutostart, line 245), the env opt-outs, and the explicit true/false settings answers via fakeMcpxWithAutostart (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, no OPENCODE_MCPX_BIN pointing 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.autostart exits non-zero asserts ensureDaemon() still spawns the daemon (record file written with the usual spawn args).
  • A test with OPENCODE_MCPX_BIN pointing at a nonexistent binary asserts ensureDaemon() 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.ts still pass; new tests follow the existing fakeMcpx* helper pattern and keep the Windows skipIf conventions 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

  1. added 2 commits that reference this issue on Oct 20, 2026
    b8c91cb
    77f9fea
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions