cli tests: the resolveInstance active/savedAt guards both resolve on a millisecond tie — the discriminating test reds 1 run in 8 #1337

Filed as an issue rather than a PR comment because #1335 is merged (16916aff) and shipped as cli 0.1.24 — no lifecycle race here.

Summary

cli/__tests__/lib.test.mjs gained two tests in #1335 for resolveInstance's new URL-match preference (active first, then newest savedAt). Both of them resolve on a savedAt millisecond tie, so which one guards which branch is decided by clock timing rather than by what the test was written to assert.

Net effect: the active branch is guarded today, but by the test that was not written to guard it, and by accident. Nothing goes red if that accident stops happening.

Measurement

Mutation const active = matches.find(...) → const active = null, run 8× at d4c08cca7, same head, no other change:

run 1: 1 failed, 25 passed      run 5: 1 failed, 25 passed
run 2: 2 failed, 24 passed      run 6: 1 failed, 25 passed
run 3: 1 failed, 25 passed      run 7: 1 failed, 25 passed
run 4: 1 failed, 25 passed      run 8: 1 failed, 25 passed

  8x  ✕ getToken("https://...") prefers the ACTIVE instance when several keys share the URL
  1x  ✕ prefers an explicitly-active match over a NEWER one      <-- the discriminating one

Cause

saveInstance writes savedAt: new Date().toISOString() (cli/src/lib/config.js:117). Two back-to-back calls produce the same string 199 times out of 200 (measured directly, 200 trials). localeCompare on equal strings returns 0, and Array.prototype.sort is stable, so a tie resolves to object key order — the oldest entry, which is precisely the behaviour #1335 set out to fix.

That inverts what each test proves:

• "prefers the ACTIVE instance" expects the newer token. A tie yields the older one, so it reds — but via the sort, not via active.
• "prefers an explicitly-active match over a NEWER one" expects the older token. A tie yields exactly that, so it passes for the wrong reason, ~7 runs in 8.

Removing the tie (explicit distinct savedAt on both new tests) and re-running the identical mutation 5×:

baseline:            26 passed, 26 total
M1 active = null:    1 failed x5  —  ✕ prefers an explicitly-active match over a NEWER one
                                      ("prefers the ACTIVE instance" now passes)

The reds swap sides completely, which is the proof that neither test is asserting what its name says.

Why it is worth fixing rather than leaving

Anything that breaks the millisecond collision — an added await, --runInBand, a faster or coarser clock, or savedAt gaining sub-ms precision — flips "prefers the ACTIVE instance" to green and leaves "prefers an explicitly-active match" green as well. The active branch then has no reliable guard anywhere, and no test goes red to announce the loss.

There is also a stale comment on main now. cli/__tests__/lib.test.mjs:178-179 reads:

// (The test above is satisfied by savedAt alone — sprint-review mutated
// active to null and it stayed green.)

That records my own earlier, weaker single-run measurement as settled fact, and the 8-run result contradicts it: "prefers the ACTIVE instance" reds under that mutation 8 times in 8. The comment should go or be corrected along with the test.

Fix (verified)

Stamp savedAt in the discriminating test only — the same thing the third new test already does deliberately:

// savedAt must DIFFER, or the two saves tie on the same millisecond and
// the stable sort returns 'old' anyway — passing for the wrong reason.
const cfg2 = JSON.parse(fs.readFileSync(configFile, 'utf8'));
cfg2.instances.old.savedAt = '2026-07-10T05:10:00.000Z';
cfg2.instances.new.savedAt = '2026-08-26T22:40:12.000Z';
fs.writeFileSync(configFile, JSON.stringify(cfg2));
setActive('old');
expect(getToken('https://api.commonly.me')).toBe('cm_old');

Ran exactly this: baseline 26/26 green, and active = null reds 2 tests, 5/5 runs, deterministically. Both guards fire, and the intended one fires by design.

Not verified

• Whether a savedAt tie can occur in production. Two saves for one URL land days apart in normal use, so I treat this as test determinism only. I did not enumerate saveInstance callers looking for a back-to-back pair, so "cannot happen in production" is not something I established — if such a caller exists, the newest-wins fallback silently degrades to oldest-wins for it.
• Ran on Node 26 locally; CI's Node version may differ, and that is itself one of the timing variables above — so the 8/8 and 1/8 splits are this machine's, not necessarily CI's.
• The shipped behaviour is correct and was field-verified by Sam on 0.1.24 (two-key config that 401'd on 0.1.23 now succeeds). This issue is about the guard, not the fix.