Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
196 changes: 196 additions & 0 deletions packages/core/src/sessions/clearSessionError.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,196 @@
import type { AgentSession } from "@posthog/shared";
import { describe, expect, it, vi } from "vitest";
import { SessionService, type SessionServiceDeps } from "./sessionService";

const TASK_ID = "task-1";
const RUN_ID = "run-1";
const REPO = "/repo";

const noopLog = {
info: vi.fn(),
warn: vi.fn(),
error: vi.fn(),
debug: vi.fn(),
};

function makeSession(overrides: Partial<AgentSession> = {}): AgentSession {
return {
taskRunId: RUN_ID,
taskId: TASK_ID,
taskTitle: "Task",
channel: "agent-event:run-1",
events: [],
startedAt: Date.now(),
status: "error",
isPromptPending: false,
isCompacting: false,
pendingPermissions: new Map(),
promptStartedAt: null,
pausedDurationMs: 0,
messageQueue: [],
optimisticItems: [],
initialPrompt: [{ type: "text", text: "do the thing" }],
...overrides,
};
}

function createService(session?: AgentSession) {
const sessions: Record<string, AgentSession> = {};
if (session) sessions[session.taskRunId] = session;

const store = {
getSessions: () => sessions,
getSessionByTaskId: (taskId: string) =>
Object.values(sessions).find((s) => s.taskId === taskId),
setSession: (s: AgentSession) => {
sessions[s.taskRunId] = s;
},
updateSession: (taskRunId: string, updates: Partial<AgentSession>) => {
const s = sessions[taskRunId];
if (s) Object.assign(s, updates);
},
clearTailOptimisticItems: vi.fn(),
appendOptimisticItem: vi.fn(),
replaceOptimisticWithEvent: vi.fn(),
clearMessageQueue: vi.fn(),
};

const deps = {
store,
log: noopLog,
notifyPromptComplete: vi.fn(),
notifyPermissionRequest: vi.fn(),
getPersistedConfigOptions: () => undefined,
setPersistedConfigOptions: vi.fn(),
trpc: {
agent: {
onSessionIdleKilled: {
subscribe: () => ({ unsubscribe: vi.fn() }),
},
cancel: {
mutate: vi.fn().mockResolvedValue(undefined),
},
getPreviewConfigOptions: {
query: vi.fn().mockResolvedValue([]),
},
},
},
} as unknown as SessionServiceDeps;

const service = new SessionService(deps);

// Replace the private helpers clearSessionError delegates to so the test can
// assert the session-intent args in isolation, without standing up the full
// agent.start/createTaskRun/subscribe chain inside createNewLocalSession.
const createSpy = vi.fn().mockResolvedValue(undefined);
const teardownSpy = vi.fn().mockResolvedValue(undefined);
const authSpy = vi.fn().mockResolvedValue({
apiHost: "https://us.posthog.com",
projectId: 1,
client: {},
});
// biome-ignore lint/suspicious/noExplicitAny: spy on private methods
const anyService = service as any;
anyService.createNewLocalSession = createSpy;
anyService.teardownSession = teardownSpy;
anyService.getAuthCredentials = authSpy;

return { service, createSpy, teardownSpy, sessions };
}

describe("clearSessionError preserves session intent on retry", () => {
it("forwards the caller overrides (adapter/model/executionMode/reasoningLevel)", async () => {
const { service, createSpy } = createService(makeSession());

await service.clearSessionError(TASK_ID, REPO, {
adapter: "codex",
model: "gpt-5.5",
executionMode: "auto",
reasoningLevel: "high",
});

expect(createSpy).toHaveBeenCalledTimes(1);
// createNewLocalSession(taskId, taskTitle, repoPath, auth, initialPrompt,
// executionMode, adapter, model, reasoningLevel)
const [, , , , , executionMode, adapter, model, reasoningLevel] =
createSpy.mock.calls[0];
expect(adapter).toBe("codex");
expect(model).toBe("gpt-5.5");
expect(executionMode).toBe("auto");
expect(reasoningLevel).toBe("high");
});

it("recovers adapter/model/executionMode from a previously-live session when no overrides are given", async () => {
const session = makeSession({
status: "error",
idleKilled: true,
adapter: "codex",
configOptions: [
{
id: "model",
name: "Model",
type: "select",
currentValue: "gpt-5.5",
options: [],
category: "model",
},
{
id: "mode",
name: "Approval Preset",
type: "select",
currentValue: "auto",
options: [],
category: "mode",
},
{
id: "reasoning_effort",
name: "Reasoning Level",
type: "select",
currentValue: "high",
options: [],
category: "thought_level",
},
],
});
const { service, createSpy } = createService(session);

await service.clearSessionError(TASK_ID, REPO);

expect(createSpy).toHaveBeenCalledTimes(1);
const [, , , , , executionMode, adapter, model, reasoningLevel] =
createSpy.mock.calls[0];
expect(adapter).toBe("codex");
expect(model).toBe("gpt-5.5");
expect(executionMode).toBe("auto");
expect(reasoningLevel).toBe("high");
});

it("keeps the retry placeholder when fresh session creation fails", async () => {
const session = makeSession();
const { service, createSpy, teardownSpy, sessions } =
createService(session);
createSpy.mockRejectedValueOnce(new Error("still broken"));

await expect(service.clearSessionError(TASK_ID, REPO)).rejects.toThrow(
"still broken",
);

expect(teardownSpy).not.toHaveBeenCalled();
expect(sessions[RUN_ID]).toBe(session);
});

it("recovers a Codex placeholder with no explicit model as Codex default", async () => {
const session = makeSession({
adapter: "codex",
configOptions: [],
});
const { service, createSpy } = createService(session);

await service.clearSessionError(TASK_ID, REPO);

expect(createSpy).toHaveBeenCalledTimes(1);
const [, , , , , , adapter, model] = createSpy.mock.calls[0];
expect(adapter).toBe("codex");
expect(model).toBeUndefined();
});
});
85 changes: 76 additions & 9 deletions packages/core/src/sessions/sessionService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import type {
SessionConfigOption,
SessionUpdate,
} from "@agentclientprotocol/sdk";
import { inferAdapterFromModelId } from "@posthog/core/task-detail/modelAdapter";
import {
type AcpMessage,
type Adapter,
Expand All @@ -17,6 +18,7 @@ import {
getBackoffDelay,
getCloudUrlFromRegion,
getConfigOptionByCategory,
getCurrentModeFromConfigOptions,
isFatalSessionError,
isJsonRpcNotification,
isJsonRpcRequest,
Expand Down Expand Up @@ -831,9 +833,18 @@ export class SessionService {
const events = convertStoredEntriesToEvents(rawEntries);

const storedAdapter = this.d.adapterStore.getAdapter(taskRunId);
const resolvedAdapter = adapter ?? storedAdapter;
const persistedConfigOptions = this.d.getPersistedConfigOptions(taskRunId);

// Infer the adapter from the persisted model as a safety net. A missing
// adapter on reconnect would cause the local agent to default to Claude
// even for Codex/OpenAI sessions.
const persistedModelForAdapter =
getConfigOptionByCategory(persistedConfigOptions, "model")?.currentValue;
const resolvedAdapter =
adapter ??
storedAdapter ??
inferAdapterFromModelId(persistedModelForAdapter);

const previous = this.d.store.getSessions()[taskRunId];

const session = createBaseSession(taskRunId, taskId, taskTitle);
Expand Down Expand Up @@ -1207,15 +1218,28 @@ export class SessionService {
}

const { customInstructions: startCustomInstructions } = this.d.settings;
const preferredModel = model ?? this.d.DEFAULT_GATEWAY_MODEL;

// Infer the adapter from the model id when the caller did not supply one.
// Without this, a GPT/Codex model paired with an undefined adapter causes
// the local agent process to default to Claude and silently ignore the
// selected model, falling back to the Claude default (Opus).
const resolvedAdapter = adapter ?? inferAdapterFromModelId(model);

// Use the gateway default only when the caller did not pick a model AND the
// adapter is Claude (or still unknown). Codex resolves its own default
// server-side via Agent.run, so passing no model is correct there.
const preferredModel =
model ??
(resolvedAdapter === "codex" ? undefined : this.d.DEFAULT_GATEWAY_MODEL);

const result = await this.d.trpc.agent.start.mutate({
taskId,
taskRunId: taskRun.id,
repoPath,
apiHost: auth.apiHost,
projectId: auth.projectId,
permissionMode: executionMode,
adapter,
adapter: resolvedAdapter,
customInstructions: startCustomInstructions || undefined,
effort: effortLevelSchema.safeParse(reasoningLevel).success
? (reasoningLevel as EffortLevel)
Expand All @@ -1226,7 +1250,7 @@ export class SessionService {
const session = createBaseSession(taskRun.id, taskId, taskTitle);
session.channel = result.channel;
session.status = "connected";
session.adapter = adapter;
session.adapter = resolvedAdapter;
const configOptions = result.configOptions as
| SessionConfigOption[]
| undefined;
Expand All @@ -1238,8 +1262,8 @@ export class SessionService {
}

// Persist the adapter
if (adapter) {
this.d.adapterStore.setAdapter(taskRun.id, adapter);
if (resolvedAdapter) {
this.d.adapterStore.setAdapter(taskRun.id, resolvedAdapter);
}

// Store the initial prompt on the session so retry/reset flows can
Expand All @@ -1256,7 +1280,7 @@ export class SessionService {
task_id: taskId,
execution_type: "local",
initial_mode: executionMode,
adapter,
adapter: resolvedAdapter,
});

if (initialPrompt?.length) {
Expand Down Expand Up @@ -3025,13 +3049,27 @@ export class SessionService {
* initialPrompt saved from the original creation attempt), creates
* a fresh session and re-sends the prompt instead of reconnecting
* to an empty session.
*
* Optional overrides let callers (e.g. the "Retry" button) explicitly pin
* the adapter/model/mode for the fresh session. When no overrides are given
* the values are recovered from the errored session's adapter +
* configOptions so a retry does not silently revert to Claude/Opus.
*/
async clearSessionError(taskId: string, repoPath: string): Promise<void> {
async clearSessionError(
taskId: string,
repoPath: string,
overrides?: {
adapter?: Adapter;
model?: string;
executionMode?: ExecutionMode;
reasoningLevel?: string;
},
): Promise<void> {
this.localRepoPaths.set(taskId, repoPath);
const session = this.d.store.getSessionByTaskId(taskId);
if (session?.initialPrompt?.length) {
const { taskTitle, initialPrompt } = session;
await this.teardownSession(session.taskRunId);
const oldTaskRunId = session.taskRunId;
const authStatus = await this.getAuthCredentialsStatus();
if (authStatus.kind === "restoring") {
throw new Error("Authentication is still restoring. Please wait.");
Expand All @@ -3041,13 +3079,42 @@ export class SessionService {
"Unable to reach server. Please check your connection.",
);
}

// Recover session intent from the errored session when the caller did
// not supply overrides, so retry preserves the user's original picks
// instead of resetting to defaults.
const recoveredModel =
overrides?.model ??
getConfigOptionByCategory(session.configOptions, "model")?.currentValue;
const recoveredAdapter =
overrides?.adapter ??
session.adapter ??
inferAdapterFromModelId(recoveredModel);
const recoveredMode =
overrides?.executionMode ??
getCurrentModeFromConfigOptions(session.configOptions);
const recoveredReasoning =
overrides?.reasoningLevel ??
getConfigOptionByCategory(
session.configOptions,
"thought_level",
)?.currentValue;

// Create the new session before tearing down the old one. If creation
// fails the caller can retry from the preserved placeholder instead of
// being left with no session at all.
await this.createNewLocalSession(
taskId,
taskTitle,
repoPath,
authStatus.auth,
initialPrompt,
recoveredMode,
recoveredAdapter,
recoveredModel,
recoveredReasoning,
);
Comment on lines 3106 to 3116

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 The currentValue from getConfigOptionByCategory is typed as string, so casting it directly to ExecutionMode is unsafe — any persisted value that doesn't match the union (e.g. a stale or renamed mode) would pass the cast silently and reach agent.start.mutate as an invalid permissionMode. The same pattern used for reasoningLevel — accept it as string and let the downstream call validate or ignore it — is safer here too.

Suggested change
await this.createNewLocalSession(
taskId,
taskTitle,
repoPath,
authStatus.auth,
initialPrompt,
recoveredMode as ExecutionMode | undefined,
recoveredAdapter,
recoveredModel,
recoveredReasoning,
);
await this.createNewLocalSession(
taskId,
taskTitle,
repoPath,
authStatus.auth,
initialPrompt,
(recoveredMode as ExecutionMode | undefined) ?? undefined,
recoveredAdapter,
recoveredModel,
recoveredReasoning,
);

await this.teardownSession(oldTaskRunId);
return;
}
await this.reconnectInPlace(taskId, repoPath);
Expand Down
25 changes: 25 additions & 0 deletions packages/core/src/task-detail/modelAdapter.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
import { describe, expect, it } from "vitest";
import { inferAdapterFromModelId } from "./modelAdapter";

describe("inferAdapterFromModelId", () => {
it.each(["gpt-5.5", "gpt-5.4", "o3", "o4-mini", "codex-mini"])(
"maps OpenAI model %s to Codex",
(model) => {
expect(inferAdapterFromModelId(model)).toBe("codex");
},
);

it.each(["claude-opus-4-8", "claude-sonnet-4-5"])(
"maps Anthropic model %s to Claude",
(model) => {
expect(inferAdapterFromModelId(model)).toBe("claude");
},
);

it.each([undefined, null, "", "fable", "custom-model"])(
"leaves ambiguous model %s unchanged",
(model) => {
expect(inferAdapterFromModelId(model)).toBeUndefined();
},
);
});
Loading