fix(cloud): #92 review, round three — sign-outs, BYOK in Settings, malformed renewals

1. A sign-out could be reported as an expiry. Chat and the agent both called a
   gateway 401 "the session ended" whenever the window was no longer signed in.
   Signing out while that 401's refresh is still out leaves the window signed out
   as well — the superseded refresh finds no token — so someone who had just
   pressed Sign out was told "Your session has expired". Both paths now ask one
   function, isEndedSessionError, and it also requires an expiry to be waiting.
   One function, because the rule was written twice and had the same hole twice.

2. Choosing BYOK in Settings left the card up. The webview dropped the card only
   on a signed-in account message, so with the mode switched — or the cloud host
   cleared — requests ran on the user's own key under a card still asking them to
   sign in. The account message now carries `expired`, the host's own answer to
   "is an expiry still waiting?", and the card goes when that is false; the
   webview does not keep its own list of reasons. The way back is covered too:
   returning to gateway mode puts the expiry back in force, so the settings
   listener replays the card. And a session that ends while in BYOK mode no
   longer shows a card at all — nothing the user was doing has stopped working.

3. A malformed 2xx counted as a renewal. classifyRefresh accepted any truthy
   `access`, so `{ access: {} }` went to SecretStorage, which takes strings. I had
   moved those writes out of the refresh's try in 5182a1f, so the rejection came
   out of the check `ready` awaits and took the rest of initialization with it.
   `{ access: 'a', refresh: {} }` replaced the access token first and threw
   second. Both fields are validated before a reply is 'ok'. Separately,
   refreshCloudToken can no longer reject: a store that will not write (a locked
   keychain) is one more way for the attempt to fail, which is the containment
   that commit lost.

Verified: 43 suites, 757 cases, 0 failing. sessionExpiredHost.test.js goes from 44
to 57 cases and session.test.js from 12 to 15. The agent cases run the REAL loop
from agent.js — loaded whole, with the provider call replaced — through a 401
whose refresh is refused, and through a sign-out while that refresh is out. The
suite takes the real fetch away, so nothing in it can reach a gateway even if a
refactor routes around the stand-ins. Its SecretStorage stand-in now rejects
non-strings, as the real one does. Each defect put back fails its own case: no
"expiry waiting" clause -> the chat and agent sign-outs; the hook with its own
copy of the rule -> the wiring guard; either token unvalidated -> session.test.js
and "nothing was written"; the refresh allowed to reject -> "the keychain is
locked" escapes; no `expired` on the account message, no replay on returning to
gateway, the webview dismissing on sign-in alone -> each its own case. In headless
Chrome against the real chat.html, the BYOK and cleared-host checks fail on the
previous commit and pass on this one. tsc --checkJs reports nothing new.

Author: ndemianc

extensions/levelcode-ai/extension.js

@@ -380,6 +380,16 @@ async function refreshCloudToken() {
 	if (!ctx) { return false; }
 	const endpoint = cloudApiUrl();
 	if (!/^https:\/\//i.test(endpoint) && !/^http:\/\/(localhost|127\.0\.0\.1)([:/]|$)/i.test(endpoint)) { return false; }
+	// Never rejects. The stores can throw as well as the network (a locked keychain, say), and the
+	// callers are a webview's `ready`, a timer on window focus, and the 401 path of a request that is
+	// already in trouble — none of which can do anything with that except fail worse. It is one more
+	// way for this attempt to fail.
+	try { return await renewSession(endpoint); }
+	catch (e) { dbg('cloud.refresh', { error: String((e && e.message) || e) }); return false; }
+}
+
+/** refreshCloudToken's work, free to throw. */
+async function renewSession(endpoint) {
 	// Which session this request is about. Read under the lock, so never halfway through a sign-in.
 	const was = await withSessionLock(async () => ({ generation: sessionGeneration, refresh: await ctx.secrets.get(ACCOUNT_REFRESH_KEY) }));
 	if (!was.refresh) { return false; }
@@ -496,7 +506,10 @@ async function sessionExpired(was) {
 	if (!ended) { return false; }
 	const p = ctx.globalState.get(ACCOUNT_PROFILE_KEY) || {};
 	dbg('cloud.sessionExpired', { name: p.name || p.email || '' });
-	postSessionExpired();
+	// In BYOK mode nothing the user is doing has stopped working, so there is no card to show — the
+	// popover going back to "signed out" is the whole of it. (The account message that follows would
+	// take the card straight down again anyway.) The marker is kept for if they return to gateway mode.
+	if (sessionExpiredPending()) { postSessionExpired(); }
 	await postAccount(false);
 	sendConfigToWebview();
 	return true;
@@ -512,20 +525,43 @@ function postSessionExpired() {
  * True while an ended session is still waiting on the user: sessionExpired() left its marker, and
  * nothing has answered it — a sign-in, a sign-out, or "use my own key" (clearSessionExpired).
  *
- * Two things read it, and both need more than "there is no token":
+ * Everything about the sign-in card hangs off it, and each use needs more than "there is no token":
  *
- *   1. The replay on a webview's `ready`. The session can end with no chat open to hear about it
+ *   1. Showing it — replaySessionExpired. The session can end with no chat open to hear about it
  *      (the focus check does not need one), and post() has nowhere to send the card.
- *   2. prepProviderRequest. Gateway is the DEFAULT mode, so "gateway mode, no token" describes
- *      everyone who uses LevelCode on their own key with no account. Only a session that ended may
- *      stop a request and ask for a sign-in; the rest fall back to BYOK, as the setting promises.
+ *   2. Stopping a request to ask for a sign-in — prepProviderRequest. Gateway is the DEFAULT mode,
+ *      so "gateway mode, no token" describes everyone who uses LevelCode on their own key with no
+ *      account. Only a session that ended may stop a request; the rest fall back to BYOK, as the
+ *      setting promises.
+ *   3. Calling a failed request an expiry — isEndedSessionError. A 401 that lands after a
+ *      deliberate sign-out finds no session either, and is not one.
+ *   4. Taking it away — currentAccount reports it as `expired`, and the webview drops the card as
+ *      soon as that is false.
  *
- * Scoped to where the card can be acted on: gateway mode, with a host to sign in to.
+ * Scoped to where the card can be acted on: gateway mode, with a host to sign in to. Leaving either
+ * (BYOK chosen in Settings, a cleared host) puts the expiry out of play WITHOUT answering it: the
+ * marker stays, and coming back brings the card back.
  */
 function sessionExpiredPending() {
 	return !!(ctx && ctx.globalState.get(ACCOUNT_EXPIRED_KEY)) && providerMode() === 'gateway' && !!cloudEndpoint();
 }
 
+/**
+ * Is this failure the session having ENDED — the one failure that gets the sign-in card instead of
+ * its own message? Chat and the agent both ask it about a gateway 401 their refresh could not
+ * recover, and every clause is there for a case that looks the same without it:
+ *
+ *   - a gateway request: a BYOK provider's 401 is that provider's business;
+ *   - this window no longer signed in: a refresh that merely failed leaves the session alive;
+ *   - an expiry still waiting on the user: a 401 that lands after they signed OUT, mid-request,
+ *     finds the session gone as well — and "your session has expired" is the wrong thing to tell
+ *     someone who has just pressed Sign out;
+ *   - and the error being that 401 at all.
+ */
+function isEndedSessionError(req, e) {
+	return !!(req && req.gateway) && !cloudSignedIn && sessionExpiredPending() && session.isSessionExpiredError(e);
+}
+
 /** The user has answered the expiry, so stop asking: the marker goes. Unconditional — another window
  *  may have set it, and this window's view of globalState can lag behind the write. */
 async function clearSessionExpired() {
@@ -1918,8 +1954,9 @@ async function agentFlow(text, imageBlocks) {
 			},
 			// After refreshAuth has failed on a gateway 401, agent.js asks whether that was the session
 			// ending (→ it posts a sign-in card) or just an error. The refresh itself already ran
-			// sessionExpired() when the server said the refresh token was dead.
-			isSessionExpired: (e) => !!req.gateway && !cloudSignedIn && session.isSessionExpiredError(e),
+			// sessionExpired() when the server said the refresh token was dead. The same question, asked
+			// the same way, as the chat path's — see isEndedSessionError.
+			isSessionExpired: (e) => isEndedSessionError(req, e),
 			sessionExpiredMessage: session.SESSION_EXPIRED_MESSAGE,
 			skills: skillsObj,                  // M6.5: implicit skills (name+desc menu in SYSTEM + use_skill resolver)
 			projectMemory: projectMemoryMarkdown(), // cross-session memory: a verify-first digest of past sessions, injected like project rules
@@ -2242,9 +2279,9 @@ async function handleSend(text, images) {
 			// is what clears cloudSignedIn). A 401 whose refresh merely failed — offline, a 5xx, a
 			// malformed reply — is this request failing, not the session: the tokens stay, the error is
 			// shown as the error it is, and the next message tries the refresh again. Ending the session
-			// from here would delete credentials classifyRefresh deliberately kept. Same rule as the
-			// agent's isSessionExpired hook.
-			if (req.gateway && !cloudSignedIn && session.isSessionExpiredError(e)) {
+			// from here would delete credentials classifyRefresh deliberately kept. The agent's
+			// isSessionExpired hook asks the same question: isEndedSessionError.
+			if (isEndedSessionError(req, e)) {
 				post({ type: 'assistantError', message: session.SESSION_EXPIRED_MESSAGE, code: 'session_expired' });
 			} else {
 				post({ type: 'assistantError', message: String((e && e.message) || e), code: e && e.code });
@@ -3029,9 +3066,12 @@ async function currentAccount() {
 	cloudSignedIn = !!token;   // keep the footer/model gate in sync with the real session
 	if (token) {
 		const p = (ctx && ctx.globalState.get(ACCOUNT_PROFILE_KEY)) || {};
-		return { signedIn: true, mode, name: p.name || p.email || 'LevelCode user', email: p.email || '', plan: p.plan || '' };
+		return { signedIn: true, mode, expired: false, name: p.name || p.email || 'LevelCode user', email: p.email || '', plan: p.plan || '' };
 	}
-	return { signedIn: false, mode, status: cloudEndpoint() ? 'signedout' : 'unconfigured' };
+	// `expired`: an ended session is still waiting on the user. The webview keeps its sign-in card only
+	// while this says so — which is how the card leaves when the expiry is answered somewhere other
+	// than the card itself: switching to BYOK in Settings, or clearing the cloud host.
+	return { signedIn: false, mode, expired: sessionExpiredPending(), status: cloudEndpoint() ? 'signedout' : 'unconfigured' };
 }
 /**
  * Best-effort refresh of the cached profile (esp. `plan`) from the backend, so the free-tier lock /
@@ -3241,6 +3281,22 @@ async function openWorkspaceFile(rel) {
 	} catch (e) { dbg('openFile.error', { msg: String((e && e.message) || e) }); }
 }
 
+/**
+ * Settings changed. The routing mode and the cloud host decide whether a session is in play at all,
+ * so they resync the account popover — and with it the sign-in card, in BOTH directions. Leaving
+ * gateway mode (or clearing the host) answers an unanswered expiry as far as the screen goes: the
+ * account message says nothing is pending and the webview drops the card. Coming back puts the
+ * expiry back in force, and the card has to come back with it — otherwise the next message is
+ * stopped by something nothing on screen mentions.
+ */
+function onConfigChanged(e) {
+	if (e.affectsConfiguration('levelcode.ai')) { sendConfigToWebview(); }
+	if (e.affectsConfiguration('levelcode.ai.providerMode') || e.affectsConfiguration('levelcode.cloud')) {
+		postAccount();
+		replaySessionExpired().catch(() => { });
+	}
+}
+
 function activate(context) {
 	// A window that comes back after a while away may have outlived its access token (8 h). Re-check
 	// on focus, throttled, so the expiry is found before the next message rather than by it.
@@ -3316,10 +3372,7 @@ function activate(context) {
 		vscode.workspace.onDidDeleteFiles(scheduleFileIndex),
 		vscode.workspace.onDidRenameFiles(scheduleFileIndex),
 		vscode.workspace.onDidChangeWorkspaceFolders(scheduleFileIndex),
-		vscode.workspace.onDidChangeConfiguration((e) => {
-			if (e.affectsConfiguration('levelcode.ai')) { sendConfigToWebview(); }
-			if (e.affectsConfiguration('levelcode.ai.providerMode') || e.affectsConfiguration('levelcode.cloud')) { postAccount(); }
-		})
+		vscode.workspace.onDidChangeConfiguration(onConfigChanged)
 	);
 
 	// AI edit-with-diff (select code → instruct → review diff → apply) — provider-agnostic.

extensions/levelcode-ai/media/chat.html

@@ -4198,7 +4198,11 @@
     else if (m.type === 'compactResult'){ onCompactResult(m); }
     else if (m.type === 'debug'){ addDebug(m); }
     else if (m.type === 'account'){
-      if (m.signedIn && sessionCard && sessionCard.isConnected){ sessionCard.remove(); sessionCard = null; } renderAccount(m); if (m.open) openAccount(); }
+      // The sign-in card stays only while the host says an expiry is still waiting (m.expired).
+      // Signing in answers it; so does anything that takes the session out of play — BYOK mode
+      // chosen in Settings, or no cloud host to sign in to. Requests run on the user's own key by
+      // then, so a card still asking them to sign in would be wrong.
+      if (!m.expired && sessionCard && sessionCard.isConnected){ sessionCard.remove(); sessionCard = null; } renderAccount(m); if (m.open) openAccount(); }
     else if (m.type === 'fileIndex'){ setFileIndex(m.files || []); }
     else if (m.type === 'agentError'){ clearStatus(); finishAgentBubble(); closeGroup(); const cap = capReachedInfo(m.message); if (isSessionExpired(m)){ addSignInCard(m); } else if (cap){ addUpgradeCard(cap); } else if (isServiceIssue(m)){ addServiceCard(m); } else { add('assistant', '<span class="err">' + esc(m.message) + '</span>'); } }
     else if (m.type === 'agentDone'){ clearStatus(); finishAgentBubble(); addAgentDone(m.reason, m.edits, m.credits, m.maxSteps, m.costMicros); setStreaming(false); }

extensions/levelcode-ai/providers/session.js

@@ -65,14 +65,24 @@ function accessNeedsRefresh(token, nowMs = Date.now()) {
  *
  * Only an explicit 401 ends the session. Clearing credentials on a network blip would log a user
  * out for closing their laptop on the train.
+ *
+ * And 'ok' means the reply can be STORED as it stands, not merely that it was a 2xx with something
+ * in the right field. The tokens go into SecretStorage, which takes strings: `{ access: {} }` would
+ * throw on the way in, and `{ access: 'a', refresh: {} }` would throw after the access token had
+ * already been replaced. Either is a malformed reply — "nothing is known yet" — and the credentials
+ * in hand are still the best ones available. The refresh token is optional (a server that does not
+ * rotate sends none); when one is present it has to be usable too.
  * @param {{status?:number, body?:any}|null|undefined} res
  * @returns {'ok'|'expired'|'retry'}
  */
 function classifyRefresh(res) {
 	if (!res) { return 'retry'; }
 	if (res.status === 401) { return 'expired'; }
-	if (res.status >= 200 && res.status < 300 && res.body && (res.body.access || res.body.token)) { return 'ok'; }
-	return 'retry';
+	if (!(res.status >= 200 && res.status < 300) || !res.body) { return 'retry'; }
+	const isToken = (v) => typeof v === 'string' && v.length > 0;
+	if (!isToken(res.body.access || res.body.token)) { return 'retry'; }   // the one the host will store
+	if (res.body.refresh && !isToken(res.body.refresh)) { return 'retry'; }
+	return 'ok';
 }
 
 /**

extensions/levelcode-ai/test/session.test.js

@@ -2,7 +2,8 @@
  *  Unit tests for providers/session.js (pure) — run: node test/session.test.js
  *    - jwtExpiresAt: reads `exp` off a JWT payload without verifying; never throws
  *    - accessNeedsRefresh: the 5-minute margin, expired, unreadable
- *    - classifyRefresh: ONLY a 401 ends the session; offline/5xx keep the tokens
+ *    - classifyRefresh: ONLY a 401 ends the session; offline/5xx keep the tokens; a 2xx is a
+ *      renewal only when what it carries can be stored
  *    - isSessionExpiredError: the shapes a dead session arrives in
  *--------------------------------------------------------------------------------------------*/
 // @ts-check
@@ -70,6 +71,27 @@ test('classifyRefresh: offline, 5xx, 403, a 200 with no token, nothing at all 
 	assert.strictEqual(S.classifyRefresh({ status: 200, body: null }), 'retry');
 });
 
+test('classifyRefresh: a 2xx whose access token is not a usable string is NOT a renewal', () => {
+	for (const access of [{}, [], ['a'], 123, true]) {
+		assert.strictEqual(S.classifyRefresh({ status: 200, body: { access } }), 'retry', 'access=' + JSON.stringify(access));
+	}
+	assert.strictEqual(S.classifyRefresh({ status: 200, body: { token: {} } }), 'retry', 'the legacy field too');
+	// The host stores `access || token`, so that is the one judged: a broken `access` is not rescued by a
+	// good `token` beside it, and an EMPTY `access` falls through to `token` exactly as the store would.
+	assert.strictEqual(S.classifyRefresh({ status: 200, body: { access: {}, token: 'legacy' } }), 'retry');
+	assert.strictEqual(S.classifyRefresh({ status: 200, body: { access: '', token: 'legacy' } }), 'ok');
+});
+test('classifyRefresh: a refresh token, when one is sent, has to be a usable string too', () => {
+	for (const refresh of [{}, [], ['r'], 123, true]) {
+		assert.strictEqual(S.classifyRefresh({ status: 200, body: { access: 'a', refresh } }), 'retry', 'refresh=' + JSON.stringify(refresh));
+	}
+});
+test('classifyRefresh: no refresh token is still ok — a server that does not rotate sends none', () => {
+	for (const body of [{ access: 'a' }, { access: 'a', refresh: null }, { access: 'a', refresh: '' }, { access: 'a', refresh: 'r' }, { token: 'a', refresh: 'r' }]) {
+		assert.strictEqual(S.classifyRefresh({ status: 200, body }), 'ok', JSON.stringify(body));
+	}
+});
+
 test('isSessionExpiredError: the adapter\'s "<label> API 401: …" shape, with and without e.status', () => {
 	const e = new Error('LevelCode Cloud API 401: Your LevelCode Cloud session has expired. Sign in again to continue.');
 	assert.strictEqual(S.isSessionExpiredError(e), true);