mirror of
https://github.com/microsoft/vscode.git
synced 2026-08-23 23:46:57 +01:00
sessions: stop picking the harness by which one has models (#332034)
* sessions: stop picking the harness by which one has models The New Session composer replaced the user's harness with the first one usable without GitHub, which in practice meant the first one that had published models. Models arrive asynchronously, so a user who picked Copilot or Codex while Claude was the only harness with a catalog watched the pick snap back to Claude a moment later. Drop that substitution. The stored preference wins, and the first harness in the list is the default when there is no preference. The draft is also recreated on every session-type change while an explicit pick is set, even when the pick and the draft already agree. That was invisible before because the recreate landed on a different harness, and it is the churn that carried the snap-back. Give the pick branch the same match check the no-pick branch already has. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * sessions: clear the upgrade watcher once the pick matches the draft Address PR feedback: - Once a servable pick already matches the draft, the watcher can no longer do anything, so clear it instead of leaving the listener registered holding the created session. This restores the lifetime the old fall-through to _createNewSession gave it. - Condense the inline explanation to one line. - Cover _createSessionNow's openNewSession arguments so a signed-out user's explicit pick cannot be substituted again unnoticed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
TylerLeonhardt
co-authored by
Claude Opus 5
Claude Opus 5
parent
92eacc5309
commit
d181be19f6
@@ -22,7 +22,7 @@ import { IUriIdentityService } from '../../../../platform/uriIdentity/common/uri
|
||||
import { IDefaultAccountService } from '../../../../platform/defaultAccount/common/defaultAccount.js';
|
||||
import { localize } from '../../../../nls.js';
|
||||
import { IActiveSession, ISessionsManagementService } from '../../../services/sessions/common/sessionsManagement.js';
|
||||
import { ISession, SESSION_WORKSPACE_GROUP_GITHUB, SessionTypeAuthRequirement } from '../../../services/sessions/common/session.js';
|
||||
import { ISession, SESSION_WORKSPACE_GROUP_GITHUB } from '../../../services/sessions/common/session.js';
|
||||
import { IOpenNewSessionResult, ISessionsService } from '../../../services/sessions/browser/sessionsService.js';
|
||||
import { isAllowSignedOutWhenUsableEnabled, shouldShowGitHubWorkspaceGroupSignIn } from '../../../browser/sessionsAuthGate.js';
|
||||
import { AGENTIC_SIGN_IN_COMMAND_ID } from '../../../common/sessionCommands.js';
|
||||
@@ -603,19 +603,12 @@ export class NewChatWidget extends Disposable {
|
||||
const preferredPick = userPick && this._isPreferredServable(folderUri, userPick)
|
||||
? userPick
|
||||
: this._newChatInput.sessionTypePicker.getPreferredSessionType(folderUri);
|
||||
// A signed-out user (under the conditional-auth opt-in) can't run a type
|
||||
// that requires GitHub, so default to the first offered type usable
|
||||
// without it. No-op when signed in or the opt-in is off — today's behavior.
|
||||
// TODO: reconsider silently switching away from the remembered selection;
|
||||
// instead keep it and surface an inline "sign in for this type" affordance
|
||||
// for GitHub-only types.
|
||||
const effectivePick = this._preferUsableSessionTypeWhenSignedOut(folderUri, preferredPick);
|
||||
const fallbackProviderId = this._workspacePicker.selectedResolved?.providerId;
|
||||
try {
|
||||
return await this.sessionsService.openNewSession({
|
||||
folderUri,
|
||||
...(effectivePick
|
||||
? { providerId: effectivePick.providerId, sessionTypeId: effectivePick.sessionTypeId }
|
||||
...(preferredPick
|
||||
? { providerId: preferredPick.providerId, sessionTypeId: preferredPick.sessionTypeId }
|
||||
: fallbackProviderId
|
||||
? { providerId: fallbackProviderId }
|
||||
: undefined),
|
||||
@@ -626,29 +619,6 @@ export class NewChatWidget extends Disposable {
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* While the user is signed out and the conditional-auth opt-in is on, replace
|
||||
* a pick that requires GitHub with the first offered session type usable
|
||||
* without it. A no-op when signed in, when the opt-in is off (today's
|
||||
* behavior), or when no offered type is usable — in which case the caller's
|
||||
* existing fallbacks still apply.
|
||||
*/
|
||||
private _preferUsableSessionTypeWhenSignedOut(folderUri: URI, pick: IPreferredSessionType | undefined): IPreferredSessionType | undefined {
|
||||
if (this.defaultAccountService.currentDefaultAccount !== null || !isAllowSignedOutWhenUsableEnabled(this.configurationService)) {
|
||||
return pick;
|
||||
}
|
||||
const usable = this.sessionsManagementService.getSessionTypesForFolder(folderUri)
|
||||
.filter(type => type.sessionType.authRequirement === SessionTypeAuthRequirement.None);
|
||||
// Match on provider too when the pick names one: two providers can offer
|
||||
// the same session type id, and only one of them may be usable.
|
||||
const pickIsUsable = usable.some(type => type.sessionType.id === pick?.sessionTypeId
|
||||
&& (pick?.providerId === undefined || type.providerId === pick.providerId));
|
||||
if (usable.length === 0 || pickIsUsable) {
|
||||
return pick;
|
||||
}
|
||||
return { providerId: usable[0].providerId, sessionTypeId: usable[0].sessionType.id };
|
||||
}
|
||||
|
||||
private _scheduleRecreateOnProviderChange(folderUri: URI, userPick: IPreferredSessionType | undefined, created: ISession | undefined, replayMissedChange: boolean): void {
|
||||
const store = new DisposableStore();
|
||||
store.add(this.sessionsManagementService.onDidChangeSessionTypes(() => this._recreateOnProviderChange(folderUri, userPick, created)));
|
||||
@@ -668,6 +638,12 @@ export class NewChatWidget extends Disposable {
|
||||
if (!this._isPreferredServable(folderUri, userPick)) {
|
||||
return; // the preferred provider still cannot serve the folder
|
||||
}
|
||||
// Already running the pick: nothing left to upgrade to, so stop watching.
|
||||
if (userPick.sessionTypeId === active.sessionType
|
||||
&& (userPick.providerId === undefined || userPick.providerId === active.providerId)) {
|
||||
this._pendingPreferredUpgrade.clear();
|
||||
return;
|
||||
}
|
||||
} else {
|
||||
// No explicit pick: keep the draft on the preferred (first)
|
||||
// type. Recreate only when that preferred actually changed.
|
||||
|
||||
@@ -7,31 +7,59 @@ import assert from 'assert';
|
||||
import { DeferredPromise } from '../../../../../base/common/async.js';
|
||||
import { CancellationToken } from '../../../../../base/common/cancellation.js';
|
||||
import { Emitter, Event } from '../../../../../base/common/event.js';
|
||||
import { IDisposable, MutableDisposable } from '../../../../../base/common/lifecycle.js';
|
||||
import { IObservable, observableValue } from '../../../../../base/common/observable.js';
|
||||
import { IDisposable, MutableDisposable, toDisposable } from '../../../../../base/common/lifecycle.js';
|
||||
import { constObservable, IObservable, observableValue } from '../../../../../base/common/observable.js';
|
||||
import { extUri } from '../../../../../base/common/resources.js';
|
||||
import { URI } from '../../../../../base/common/uri.js';
|
||||
import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../../base/test/common/utils.js';
|
||||
import { IActiveSession } from '../../../../services/sessions/common/sessionsManagement.js';
|
||||
import { ISession } from '../../../../services/sessions/common/session.js';
|
||||
import { IOpenNewSessionResult } from '../../../../services/sessions/browser/sessionsService.js';
|
||||
import { IOpenNewSessionOptions, IOpenNewSessionResult } from '../../../../services/sessions/browser/sessionsService.js';
|
||||
import { IPreferredSessionType } from '../../browser/sessionTypePicker.js';
|
||||
import { NewChatWidget } from '../../browser/newChatWidget.js';
|
||||
|
||||
interface INewChatWidgetHarness {
|
||||
/** The part of the active session `_recreateOnProviderChange` actually reads. */
|
||||
interface IActiveDraft {
|
||||
readonly sessionId: string;
|
||||
readonly isCreated: IObservable<boolean>;
|
||||
readonly providerId: string;
|
||||
readonly sessionType: string;
|
||||
}
|
||||
|
||||
interface IRecreateHarness {
|
||||
readonly _pendingPreferredUpgrade: MutableDisposable<IDisposable>;
|
||||
readonly _session: IObservable<IActiveDraft | undefined>;
|
||||
readonly _newChatInput: {
|
||||
readonly sessionTypePicker: {
|
||||
getPreferredSessionType(folderUri: URI): IPreferredSessionType | undefined;
|
||||
};
|
||||
};
|
||||
_isPreferredServable(folderUri: URI, pick: IPreferredSessionType): boolean;
|
||||
_createNewSession(folderUri: URI): Promise<IOpenNewSessionResult>;
|
||||
}
|
||||
|
||||
/** The collaborators `_createSessionNow` reads while assembling the `openNewSession` options. */
|
||||
interface ICreateSessionNowHarness {
|
||||
readonly _newChatInput: {
|
||||
readonly sessionTypePicker: {
|
||||
getPreferredSessionType(folderUri: URI): IPreferredSessionType | undefined;
|
||||
};
|
||||
};
|
||||
readonly _workspacePicker: { readonly selectedResolved: { readonly providerId: string } | undefined };
|
||||
readonly sessionsService: { openNewSession(options: IOpenNewSessionOptions, token: CancellationToken): Promise<IOpenNewSessionResult> };
|
||||
readonly logService: { error(message: string, ...args: unknown[]): void };
|
||||
_isPreferredServable(folderUri: URI, pick: IPreferredSessionType): boolean;
|
||||
}
|
||||
|
||||
interface INewChatWidgetHarness extends IRecreateHarness {
|
||||
readonly _newSessionCreation: MutableDisposable<IDisposable>;
|
||||
readonly sessionsManagementService: { readonly onDidChangeSessionTypes: Event<void> };
|
||||
readonly _session: IObservable<IActiveSession | undefined>;
|
||||
readonly _newChatInput: {
|
||||
readonly sessionTypePicker: {
|
||||
getUserPickedSessionType(): IPreferredSessionType | undefined;
|
||||
getPreferredSessionType(folderUri: URI): IPreferredSessionType | undefined;
|
||||
};
|
||||
};
|
||||
_isPreferredServable(folderUri: URI, pick: IPreferredSessionType): boolean;
|
||||
_createSessionNow(folderUri: URI, userPick: IPreferredSessionType | undefined, token: CancellationToken): Promise<IOpenNewSessionResult>;
|
||||
_createNewSession(folderUri: URI): Promise<IOpenNewSessionResult>;
|
||||
_scheduleRecreateOnProviderChange(folderUri: URI, userPick: IPreferredSessionType | undefined, created: ISession | undefined, replayMissedChange: boolean): void;
|
||||
_recreateOnProviderChange(folderUri: URI, userPick: IPreferredSessionType | undefined, created: ISession | undefined): void;
|
||||
}
|
||||
@@ -40,8 +68,19 @@ const createNewSession = Reflect.get(NewChatWidget.prototype, '_createNewSession
|
||||
this: INewChatWidgetHarness,
|
||||
folderUri: URI,
|
||||
) => Promise<IOpenNewSessionResult>;
|
||||
const createSessionNow = Reflect.get(NewChatWidget.prototype, '_createSessionNow') as (
|
||||
this: ICreateSessionNowHarness,
|
||||
folderUri: URI,
|
||||
userPick: IPreferredSessionType | undefined,
|
||||
token: CancellationToken,
|
||||
) => Promise<IOpenNewSessionResult>;
|
||||
const scheduleRecreateOnProviderChange = Reflect.get(NewChatWidget.prototype, '_scheduleRecreateOnProviderChange') as INewChatWidgetHarness['_scheduleRecreateOnProviderChange'];
|
||||
const recreateOnProviderChange = Reflect.get(NewChatWidget.prototype, '_recreateOnProviderChange') as INewChatWidgetHarness['_recreateOnProviderChange'];
|
||||
const recreateOnProviderChange = Reflect.get(NewChatWidget.prototype, '_recreateOnProviderChange') as (
|
||||
this: IRecreateHarness,
|
||||
folderUri: URI,
|
||||
userPick: IPreferredSessionType | undefined,
|
||||
created: { readonly sessionId: string } | undefined,
|
||||
) => void;
|
||||
const handlePromptOptionsWorkspaceChange = Reflect.get(NewChatWidget.prototype, '_handlePromptOptionsWorkspaceChange') as (this: IPromptOptionsWorkspaceHarness, previousFolderUri: URI | undefined, folderUri: URI | undefined) => void;
|
||||
const hasEnoughSessionsForFirstRunNotices = Reflect.get(NewChatWidget.prototype, '_hasEnoughSessionsForFirstRunNotices') as (this: ISessionCountHarness) => boolean;
|
||||
|
||||
@@ -59,13 +98,13 @@ function createHarness(
|
||||
pendingPreferredUpgrade: MutableDisposable<IDisposable>,
|
||||
newSessionCreation: MutableDisposable<IDisposable>,
|
||||
onDidChangeSessionTypes: Event<void>,
|
||||
createSessionNow: (token: CancellationToken) => Promise<IOpenNewSessionResult>,
|
||||
stubCreateSessionNow: (token: CancellationToken) => Promise<IOpenNewSessionResult>,
|
||||
): INewChatWidgetHarness {
|
||||
const harness: INewChatWidgetHarness = {
|
||||
_pendingPreferredUpgrade: pendingPreferredUpgrade,
|
||||
_newSessionCreation: newSessionCreation,
|
||||
sessionsManagementService: { onDidChangeSessionTypes },
|
||||
_session: observableValue<IActiveSession | undefined>('session', undefined),
|
||||
_session: observableValue<IActiveDraft | undefined>('session', undefined),
|
||||
_newChatInput: {
|
||||
sessionTypePicker: {
|
||||
getUserPickedSessionType: () => undefined,
|
||||
@@ -73,7 +112,7 @@ function createHarness(
|
||||
},
|
||||
},
|
||||
_isPreferredServable: () => false,
|
||||
_createSessionNow: (_folderUri, _userPick, token) => createSessionNow(token),
|
||||
_createSessionNow: (_folderUri, _userPick, token) => stubCreateSessionNow(token),
|
||||
_createNewSession: folderUri => createNewSession.call(harness, folderUri),
|
||||
_scheduleRecreateOnProviderChange: (folderUri, userPick, created, replayMissedChange) => scheduleRecreateOnProviderChange.call(harness, folderUri, userPick, created, replayMissedChange),
|
||||
_recreateOnProviderChange: (folderUri, userPick, created) => recreateOnProviderChange.call(harness, folderUri, userPick, created),
|
||||
@@ -145,6 +184,79 @@ suite('NewChatWidget', () => {
|
||||
assert.deepStrictEqual({ tokenCount: tokens.length, firstCancelledWhenSecondStarted }, { tokenCount: 2, firstCancelledWhenSecondStarted: true });
|
||||
});
|
||||
|
||||
test('sends the user pick to openNewSession, falling back to the preferred type', async () => {
|
||||
const folder = URI.file('/project');
|
||||
const userPick: IPreferredSessionType = { providerId: 'agent-host', sessionTypeId: 'claude' };
|
||||
const preferredType: IPreferredSessionType = { providerId: 'copilot', sessionTypeId: 'copilot-cli' };
|
||||
const cases: { pick: IPreferredSessionType | undefined; servable: boolean; preferred: IPreferredSessionType | undefined }[] = [
|
||||
{ pick: userPick, servable: true, preferred: preferredType },
|
||||
{ pick: userPick, servable: false, preferred: preferredType },
|
||||
{ pick: undefined, servable: true, preferred: preferredType },
|
||||
{ pick: undefined, servable: true, preferred: undefined },
|
||||
];
|
||||
|
||||
const requested = await Promise.all(cases.map(async ({ pick, servable, preferred }) => {
|
||||
let options: IOpenNewSessionOptions | undefined;
|
||||
await createSessionNow.call({
|
||||
_newChatInput: { sessionTypePicker: { getPreferredSessionType: () => preferred } },
|
||||
_workspacePicker: { selectedResolved: { providerId: 'workspace-provider' } },
|
||||
sessionsService: {
|
||||
openNewSession: async opts => {
|
||||
options = opts;
|
||||
return { session: undefined, trustDeclined: false };
|
||||
},
|
||||
},
|
||||
logService: { error: () => { } },
|
||||
_isPreferredServable: () => servable,
|
||||
}, folder, pick, CancellationToken.None);
|
||||
return { providerId: options?.providerId, sessionTypeId: options?.sessionTypeId };
|
||||
}));
|
||||
|
||||
assert.deepStrictEqual(requested, [
|
||||
{ providerId: 'agent-host', sessionTypeId: 'claude' },
|
||||
{ providerId: 'copilot', sessionTypeId: 'copilot-cli' },
|
||||
{ providerId: 'copilot', sessionTypeId: 'copilot-cli' },
|
||||
{ providerId: 'workspace-provider', sessionTypeId: undefined },
|
||||
]);
|
||||
});
|
||||
|
||||
test('a provider change only recreates the draft when the pick differs from it', () => {
|
||||
const folder = URI.file('/project');
|
||||
const draft: IActiveDraft = { sessionId: 's1', isCreated: constObservable(false), providerId: 'agent-host', sessionType: 'claude' };
|
||||
const cases: { name: string; pick: IPreferredSessionType; servable: boolean }[] = [
|
||||
{ name: 'pick matches the draft', pick: { providerId: 'agent-host', sessionTypeId: 'claude' }, servable: true },
|
||||
{ name: 'pick names no provider, type matches', pick: { sessionTypeId: 'claude' }, servable: true },
|
||||
{ name: 'pick names another provider', pick: { providerId: 'other', sessionTypeId: 'claude' }, servable: true },
|
||||
{ name: 'pick names another type', pick: { providerId: 'agent-host', sessionTypeId: 'codex' }, servable: true },
|
||||
{ name: 'pick cannot be served yet', pick: { providerId: 'other', sessionTypeId: 'codex' }, servable: false },
|
||||
];
|
||||
|
||||
const outcomes = cases.map(({ name, pick, servable }) => {
|
||||
let recreated = false;
|
||||
const watcher = disposables.add(new MutableDisposable<IDisposable>());
|
||||
watcher.value = toDisposable(() => { });
|
||||
recreateOnProviderChange.call({
|
||||
_pendingPreferredUpgrade: watcher,
|
||||
_session: constObservable(draft),
|
||||
_newChatInput: { sessionTypePicker: { getPreferredSessionType: () => undefined } },
|
||||
_isPreferredServable: () => servable,
|
||||
_createNewSession: async () => {
|
||||
recreated = true;
|
||||
return { session: undefined, trustDeclined: false };
|
||||
},
|
||||
}, folder, pick, { sessionId: 's1' });
|
||||
return `${name}: ${recreated ? 'recreated' : watcher.value ? 'still watching' : 'settled'}`;
|
||||
});
|
||||
|
||||
assert.deepStrictEqual(outcomes, [
|
||||
'pick matches the draft: settled',
|
||||
'pick names no provider, type matches: settled',
|
||||
'pick names another provider: recreated',
|
||||
'pick names another type: recreated',
|
||||
'pick cannot be served yet: still watching',
|
||||
]);
|
||||
});
|
||||
|
||||
test('refreshes prompt options when the draft workspace changes', () => {
|
||||
const changes: string[] = [];
|
||||
const harness: IPromptOptionsWorkspaceHarness = {
|
||||
|
||||
Reference in New Issue
Block a user