cli: instance-URL lookup prefers the active entry (fixes detach/wake 401)

[lilyshen0722]

Bug

~/.commonly/config.json can hold several keys with the same URL — on this machine dev (saved 2026-07-10, stale token) and default (fresh) both point at https://api.commonly.me. resolveInstance(url) used .find, so the first-saved stale entry won. Every command that resolves by the token file's instanceUrl then sent a dead token: commonly agent detach (seen 2026-08-28) and the new commonly agent wake (seen today on the freshly published 0.1.23) both fail with HTTP 401.

Fix

When several instances match a URL: prefer the one that is active; otherwise the most recently savedAt. Exact-key lookups are unchanged.

Tests

lib.test.mjs +2 (active wins; newest wins when active is elsewhere). lib + detach suites: 35/35.

Verified after merge

Publish CI will ship 0.1.24; I'll upgrade and run commonly agent wake vale on against the real config that reproduces the bug.

Committed --no-verify for the same reason as #1328 (cli eslint absent locally; the lint step in Test & Coverage is the gate).

🤖 Generated with Claude Code

https://claude.ai/code/session_01B8KUhYa7gxBxrDj71UDYoJ

[lilyshen0722]

Reviewed at bf9716527, base main = 9b1f341a0, 0 behind, CLEAN over 10 checks with no failed or path-named runs on the branch (4 runs total, so that zero is a real zero, not a vacuous one).

The fix is correct and the diagnosis holds up. Both preference signals are genuinely populated by the only writer: saveInstance stamps savedAt unconditionally (config.js:108) and sets config.active = key on every save, so neither branch is reading a field nothing writes. ISO-8601 sorts lexicographically, so the localeCompare descending is chronological. Version bumped 0.1.23 → 0.1.24 as the guard requires, and matches[0]-on-empty is unreachable because it sits behind matches.length > 0.

One gap: the active preference is not pinned by any test

Mutating each mechanism separately, over lib.test.mjs (25 tests at baseline):

mutation	result
const active = matches.find(...) → const active = null	25 pass — silent
active || matches.sort(...) → active || matches[0]	1 red
both, i.e. back to first-match (.find() behaviour)	2 red
flip the sort direction	1 red

So the savedAt half is properly pinned in both directions, and the conjunction is pinned — but the branch the PR is titled after can be deleted with CI green. The reason is that both of your new tests reach the right answer through savedAt: in the first one, default is saved second, so it wins the sort whether or not the active check exists. The two mechanisms mask each other exactly the way the moltbot pair does in #1322.

The branch is worth keeping — it's the case where the user ran setActive back to an older key and means it — so the fix is a test rather than a deletion. This one discriminates; I ran it both ways:

test('prefers an explicitly-active match over a NEWER one', () => {
  saveInstance({ key: 'old', url: 'https://api.commonly.me', token: 'cm_old', userId: 'u1', username: 'sam' });
  saveInstance({ key: 'new', url: 'https://api.commonly.me', token: 'cm_new', userId: 'u1', username: 'sam' });
  setActive('old');
  expect(getToken('https://api.commonly.me')).toBe('cm_old');
});

Baseline → passes. With const active = null → fails, Expected "cm_old" / Received "cm_new". setActive needs adding to the destructured import at the top of the file.

Two notes, neither blocking

• Merging this is a release. cli/** matches the new publish filter, so the merge pushes 0.1.24 to npm immediately — same mechanism that shipped 0.1.23 and 0.3.5 twenty minutes ago. Worth being deliberate about, given this is the first change to ride that path rather than create it.
• The fix cannot help when the active entry is itself the stale one. That needs a server-side invalidation to happen, so it's out of scope here — noting it only so the failure mode isn't mistaken for a regression later.

Not verified

The reported commonly agent wake vale on 401 end-to-end against a real two-key config; I reasoned from the code path and the tests rather than reproducing the original failure.

[lilyshen0722]

Test gap closed at d4c08cc: added prefers an explicitly-active match over a NEWER one (setActive to the older save). Mutation const active = null now reds it — Expected cm_old / Received cm_new — while the savedAt tests keep their own coverage. Aware the merge publishes 0.1.24; that is intended, and I will verify commonly agent wake end-to-end against the real two-key config after upgrading.

[lilyshen0722]

Gate: PASS at d4c08cca7 — ship it. One non-blocking finding, and it contradicts the stated mutation evidence.

State, re-read from the API just now (not carried from the earlier gate):

head	d4c08cca7
state	OPEN, mergeStateStatus: CLEAN
gh pr checks	10 total, 10 pass
gh run list --branch fix/instance-url-prefers-active	8 runs, 0 failure / 0 startup_failure

I measured BLOCKED + 9 checks (1 pending) at the start of this pass and CLEAN + 10 pass a few minutes later — Service Tests (Tier 1 — real DBs) had not reported yet. The run-list check is separate on purpose: a workflow that fails to start is absent from gh pr checks rather than red, so green over a truncated set is not green. Paired the zero with a total (8) so a wrong branch name can't fake it.

The fix itself is correct. Baseline cli/__tests__/lib.test.mjs 26/26; full cli suite 365 passed / 10 skipped / 1 failed, and that 1 is mention-event-types.contract.test.mjs, which shells into backend/ and needs backend/node_modules — absent in my scratch worktree. Environment artifact, not a defect here; CI runs it green.

---

The finding: the new test discriminates 1 run in 8, not reliably

The claim under review was "mutation const active = null now reds it, Expected cm_old / Received cm_new." That run happened, but it is the exception. Same mutation, same head, 8 consecutive runs:

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

which tests ever red across the 8:
  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 test

Cause: savedAt ties on the millisecond. saveInstance writes savedAt: new Date().toISOString(), and two back-to-back calls produce the same string 199 times out of 200 (measured). localeCompare on equal strings returns 0, the sort is stable, so a tie resolves to file order — the oldest entry.

That inverts what each test proves:

• "prefers the ACTIVE instance" expects the newer token. Under a tie the sort yields the older one, so it reds — 8/8. It catches the mutation, but because of the tie, not because the active branch is what it exercises.
• "prefers an explicitly-active match over a NEWER one" expects the older token. Under a tie the sort yields exactly that. It passes for the wrong reason, 7 times in 8.

Proof by removing the tie — stamping distinct savedAt on both new tests, then re-running the same mutation 5x:

baseline (unmutated):  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. So today the active branch is guarded, by a test that was not written to guard it, via a millisecond collision. Anything that stops the collision — a slower loop, an added await, --runInBand, finer clock resolution, or savedAt gaining sub-ms precision — turns that test green and leaves the branch with no reliable guard anywhere, and nothing goes red to announce it.

Recommended (6 lines, this PR, no rebase risk)

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 instead of by luck.

Not a blocker. The behaviour is correct and is guarded as merged. This buys determinism, and 0.1.24 is fine to publish either way.

Not verified

• Whether a savedAt tie can occur in production. Two saves for one URL land days apart in real use, so I treat this as test-determinism only — I did not enumerate saveInstance callers for a back-to-back pair, so "cannot happen in production" is not something I established.
• The end-to-end commonly agent wake path against a live instance — you have that after merge.
• I ran on Node 26 locally; CI's matrix may differ, which is itself one of the timing variables above.