test(cli): restore coverage deleted under a false justification - #107
Merged
Conversation
V3RON
force-pushed
the
fix/0003-14-cli-coverage
branch
from
September 3, 2026 17:23
cb1b976 to
8fa31fa
Compare
V3RON
force-pushed
the
fix/0003-14-cli-coverage
branch
from
September 4, 2026 01:39
8fa31fa to
6eed8cf
Compare
ADR 0003 §12's per-frontend smoke-test policy is correct, but
src/cli/index.test.ts:93-95 claimed lease-lost/device-health push
parsing and --bind-pid/parent-watch termination were "covered once at
daemon/dispatcher.test.ts" -- the dispatcher has no notion of CLI
process lifecycle or stderr rendering, so that behaviour had no
coverage anywhere after the ADR 0003 deletions (adversarial review
S12).
Adds tests that pin the CLI's own logic:
- a lost lease exits 14 and does not attempt to re-release it
- the own-lease-id filter on lease-lost pushes (a push for a lease
this connection does not hold must not be treated as fatal)
- {push:...} stderr line shapes for progress/device-unhealthy/
device-recovered
- --full/--no-wait -> full/noWait input mapping, including the
omit-when-false case
- `mcp` command's lazy module load (not loaded for other commands)
and its startup-failure reporting as a structured error
- `daemon logs` works without any connection attempt, per ADR §11
Also corrects the false justification comment to describe what is
actually covered where, and notes that --bind-pid/parent-watch
termination races remain untested outside the e2e lanes.
…ence daemon.stop is accepted during startup, and an auxiliary frontend that fails to bind asks for a stop of its own, so stop() can run to completion while convergence is still in flight. start() then resumed and armed live machinery anyway: it subscribed to a disposed fact bus, scheduled a heartbeat tick on a dead daemon, started the health monitor, and emitted daemon.started after daemon.stopping -- a fact that is not true when emitted, which agent-rules/events.md rule 3 forbids. Two earlier fixes in this stack worked around the symptom from the outside (main.ts defers its stop until start() settles, and stopAuxiliary now awaits the gateway). This closes it at the root; those guards stay correct and become redundant rather than wrong.
V3RON
force-pushed
the
fix/0003-14-cli-coverage
branch
from
September 4, 2026 06:25
6eed8cf to
9eaec2c
Compare
V3RON
added a commit
that referenced
this pull request
Sep 4, 2026
…e race (#108) **Stacked on #107.** A regression this stack introduced, caught by a verification pass over the earlier fixes. `gatewayStarted` is settled only from inside `onSocketClaimed`'s handler, and `DaemonServer.start()` fires that callback only once the socket claim **and** the admin-secret write have succeeded. So a daemon that loses the start race rejects with `DaemonAlreadyRunningError` without the callback ever running — nothing settles `gatewayStarted`, and the `Promise.allSettled` join added by the bind-failure fix waits on it **forever**. `startDaemon()` therefore hung: no rejection, no "Daemon failed to start", no non-zero exit code. Both `main` and the pre-fix stack awaited `daemon.start()` alone and rejected immediately, so this is a regression introduced by that fix, not a pre-existing gap. It is reachable by racing `simlock daemon start`, and by the CLI's own auto-launch — a common path, not an exotic one, whenever HTTP is enabled. The fix releases `gatewayStarted` on the `daemon.start()` failure path. A `socketClaimed` guard keeps it from resolving early while a *claimed* daemon's gateway is still binding, so a genuine bind failure is still reported — the behaviour the previous PR added stays intact. The regression test races two `startDaemon()` calls on one data directory with distinct HTTP ports, so the only thing that can fail is the socket claim, and asserts the second **rejects** rather than hanging. Verified by stashing the fix: without it the test reports `hung` instead of `rejected`.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #106. The last finding from the adversarial review.
ADR §12 legitimately says frontends are now serialization and one smoke test each suffices, and most of the CLI suite's deletion was right. But the test file justified specific deletions by claiming
lease-lost/device-health push parsing and process-lifecycle behaviour were "covered once atdaemon/dispatcher.test.ts". That is false — the dispatcher knows nothing about CLI process lifecycle or stderr rendering; its tests cover parsing, roles, ownership, error codes, the download clamp and startup parking.So these behaviours had no coverage anywhere, and each is genuinely the CLI's own logic — exactly what §12 says to keep:
UNKNOWN_LEASE).lease-lostpushes. This matters more than it looks: since pushes became owner-routed, a connection can receive alease-lostfor a lease it does not hold, and the CLI must not treat that as fatal. The test fires a push for a different lease id and asserts it is ignored, then drives the normal release path to exit 0.{push: …}stderr line shapes for progress and device health.--full/--no-wait→full/noWaitmapping, including that absent flags omitfullrather than sendingfalse.mcplazy-load and startup-failure reporting — a load rejection surfaces as the CLI's structured stderr error rather than an uncaught throw.daemon logswith the daemon dead — an explicit ADR §11 requirement, previously unproven. The test asserts no connection is ever attempted.No new test exposed a bug; all passed against current code. The false justification comment is rewritten to say what is actually covered where, including that
--bind-pid/parent-watch races survive only in the real-hardware e2e lanes.One thing worth knowing, found while writing the push test:
DeviceUnhealthyPush's public type deliberately drops the wire schema'sreason: z.literal("crashed")as a constant carrying no information, so that stderr line never containsreason. Not a bug, but surprising if you expect it.