fix(mau): surface identity and connection failures instead of hiding them - #462

Review: PR 462 — fix(mau): surface identity and connection failures instead of hiding them

Overall: LGTM with one minor note. The architecture is solid and the tests are thorough.

Summary

Findings

src/base-command.ts — waitAndTrackCleanup timeout path (minor)

The old status: "complete" (no trailing 'd') that was emitted by the --duration timeout path is now replaced with status: "completed" + exitCode. This aligns it with the finally() path and fixes an inconsistency. Worth calling out explicitly for anyone with external JSON consumers watching that specific key, though no tests were checking the old shape so it wasn't load-bearing.

src/utils/connection-health.ts — stable-connection reset logic (correct, noting it for visibility)

When a disconnected/suspended event follows a stable connection (≥10s), unstableDrops resets to 1, not 0. That's intentional — the current drop counts as the first of a new run — but it means a connection that repeatedly stays up for exactly 10s and then drops needs 5 additional drops after each stable period before the monitor fires. The comment in STABLE_CONNECTION_MS explains the intent. The test "restarts the count after a stable connection" validates this correctly.

src/base-command.ts:2168 — tokenAuth detection scope

tokenAuth: Boolean(process.env.ABLY_TOKEN),

This is consistent with every other ABLY_TOKEN check in the file (e.g., lines 508, 802, 1234) and correctly targets the main token auth use case. ably login stores API keys, not tokens, so the env-var check covers the primary scenario. No issue here.

waitAndTrackCleanup — already-aborted signal edge case (correct)

If connectionLost fires between setupConnectionStateLogging() and waitAndTrackCleanup() being called, waitUntilInterruptedOrTimeout correctly handles the pre-aborted signal:

if (signal?.aborted) {
  handleExit("aborted");
} else {
  signal?.addEventListener("abort", abortHandler);
}

No race possible in single-threaded JS. ✓

No issues blocking merge

The PR does exactly what it says: a connection failed event or 5 consecutive drops without staying connected now produces a non-zero exit instead of a silent success. The JSON contract change (status: "complete" → status: "completed" with exitCode) is a fix, not a regression. Tests cover all the important boundary conditions.