Skip to content

Commit 1821651

Browse files
richardsolomourichardsolomou
committed
fix(agent): guard full access permission handling
Generated-By: PostHog Code Task-Id: 1f1fe07c-6384-4d1e-8ab9-bad3700caa2e
1 parent ff404a2 commit 1821651

8 files changed

Lines changed: 224 additions & 94 deletions

File tree

‎packages/agent/src/adapters/codex-app-server/session-config.ts‎

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,10 @@
11
import type { SessionConfigOption } from "@agentclientprotocol/sdk";
2-
import { CODEX_MODE_PRESETS, type CodexModePreset } from "@posthog/shared";
2+
import {
3+
CODEX_MODE_PRESETS,
4+
type CodexModePreset,
5+
type ExecutionMode,
6+
resolveCloudInitialPermissionMode,
7+
} from "@posthog/shared";
38
import { type GatewayModel, isOpenAIModel } from "../../gateway-models";
49
import { getReasoningEffortOptions } from "./models";
510

@@ -126,10 +131,11 @@ export function collaborationModeFor(
126131
* modes fall back to default.
127132
*/
128133
export function resolveInitialMode(permissionMode: string | undefined): string {
129-
if (permissionMode === "bypassPermissions") return "full-access";
130-
return permissionMode && CODEX_MODES.some((m) => m.id === permissionMode)
131-
? permissionMode
132-
: DEFAULT_MODE;
134+
if (!permissionMode) return DEFAULT_MODE;
135+
return resolveCloudInitialPermissionMode(
136+
"codex",
137+
permissionMode as ExecutionMode,
138+
);
133139
}
134140

135141
/** Codex's standard reasoning efforts; used when model/list doesn't expose them. */

‎packages/core/src/sessions/sessionService.ts‎

Lines changed: 18 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -324,7 +324,6 @@ export interface SessionServiceDeps {
324324
options: SessionConfigOption[],
325325
) => void;
326326
removePersistedConfigOptions: (taskRunId: string) => void;
327-
updatePersistedConfigOptionValue: (...args: any[]) => any;
328327
adapterStore: {
329328
getAdapter(taskRunId: string): Adapter | undefined;
330329
setAdapter(taskRunId: string, adapter: Adapter): void;
@@ -4054,16 +4053,25 @@ export class SessionService {
40544053
});
40554054
}
40564055
} catch (error) {
4057-
// Rollback on error
4058-
const rolledBackOptions = configOptions.map((opt) =>
4059-
opt.id === configId
4060-
? ({ ...opt, currentValue: previousValue } as SessionConfigOption)
4061-
: opt,
4056+
const latestConfigOptions =
4057+
this.d.store.getSessionByTaskId(taskId)?.configOptions ?? [];
4058+
const latestOption = latestConfigOptions.find(
4059+
(option) => option.id === configId,
40624060
);
4063-
this.d.store.updateSession(session.taskRunId, {
4064-
configOptions: rolledBackOptions,
4065-
});
4066-
this.d.setPersistedConfigOptions(session.taskRunId, rolledBackOptions);
4061+
if (latestOption?.currentValue === value) {
4062+
const rolledBackOptions = latestConfigOptions.map((option) =>
4063+
option.id === configId
4064+
? ({
4065+
...option,
4066+
currentValue: previousValue,
4067+
} as SessionConfigOption)
4068+
: option,
4069+
);
4070+
this.d.store.updateSession(session.taskRunId, {
4071+
configOptions: rolledBackOptions,
4072+
});
4073+
this.d.setPersistedConfigOptions(session.taskRunId, rolledBackOptions);
4074+
}
40674075
this.d.log.error("Failed to set session config option", {
40684076
taskId,
40694077
configId,

‎packages/ui/src/features/sessions/sessionConfigStore.ts‎

Lines changed: 0 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -15,12 +15,6 @@ interface SessionConfigActions {
1515
getConfigOptions: (taskRunId: string) => SessionConfigOption[] | undefined;
1616
/** Remove config options for a task run */
1717
removeConfigOptions: (taskRunId: string) => void;
18-
/** Update a single config option value */
19-
updateConfigOptionValue: (
20-
taskRunId: string,
21-
configId: string,
22-
value: string,
23-
) => void;
2418
}
2519

2620
type SessionConfigStore = SessionConfigState & SessionConfigActions;
@@ -42,22 +36,6 @@ export const useSessionConfigStore = create<SessionConfigStore>()(
4236
const { [taskRunId]: _removed, ...rest } = state.configsByRunId;
4337
return { configsByRunId: rest };
4438
}),
45-
46-
updateConfigOptionValue: (taskRunId, configId, value) =>
47-
set((state) => {
48-
const existing = state.configsByRunId[taskRunId];
49-
if (!existing) return state;
50-
51-
const updated = existing.map((opt) =>
52-
opt.id === configId
53-
? ({ ...opt, currentValue: value } as SessionConfigOption)
54-
: opt,
55-
);
56-
57-
return {
58-
configsByRunId: { ...state.configsByRunId, [taskRunId]: updated },
59-
};
60-
}),
6139
}),
6240
{
6341
name: "session-config-storage",
@@ -86,14 +64,3 @@ export function setPersistedConfigOptions(
8664
export function removePersistedConfigOptions(taskRunId: string): void {
8765
useSessionConfigStore.getState().removeConfigOptions(taskRunId);
8866
}
89-
90-
/** Non-hook accessor for updating a single config option value */
91-
export function updatePersistedConfigOptionValue(
92-
taskRunId: string,
93-
configId: string,
94-
value: string,
95-
): void {
96-
useSessionConfigStore
97-
.getState()
98-
.updateConfigOptionValue(taskRunId, configId, value);
99-
}

‎packages/ui/src/features/sessions/sessionServiceHost.recovery.integration.test.ts‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -127,7 +127,6 @@ const mockSessionConfigStore = vi.hoisted(() => ({
127127
getPersistedConfigOptions: vi.fn(() => undefined),
128128
setPersistedConfigOptions: vi.fn(),
129129
removePersistedConfigOptions: vi.fn(),
130-
updatePersistedConfigOptionValue: vi.fn(),
131130
}));
132131

133132
vi.mock(

‎packages/ui/src/features/sessions/sessionServiceHost.test.ts‎

Lines changed: 121 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -194,7 +194,6 @@ const mockSessionConfigStore = vi.hoisted(() => ({
194194
>(() => undefined),
195195
setPersistedConfigOptions: vi.fn(),
196196
removePersistedConfigOptions: vi.fn(),
197-
updatePersistedConfigOptionValue: vi.fn(),
198197
}));
199198

200199
vi.mock(
@@ -1006,6 +1005,14 @@ describe("SessionService", () => {
10061005
currentValue: "auto",
10071006
options: [],
10081007
},
1008+
{
1009+
id: "model",
1010+
name: "Model",
1011+
type: "select",
1012+
category: "model",
1013+
currentValue: "gpt-5.5",
1014+
options: [],
1015+
},
10091016
],
10101017
}),
10111018
);
@@ -1022,6 +1029,10 @@ describe("SessionService", () => {
10221029
id: "mode",
10231030
currentValue: "full-access",
10241031
}),
1032+
expect.objectContaining({
1033+
id: "model",
1034+
currentValue: "gpt-5.5",
1035+
}),
10251036
]);
10261037

10271038
resetSessionService();
@@ -1046,9 +1057,19 @@ describe("SessionService", () => {
10461057
id: "mode",
10471058
currentValue: "full-access",
10481059
}),
1060+
expect.objectContaining({
1061+
id: "model",
1062+
currentValue: "gpt-5.5",
1063+
}),
10491064
]),
10501065
}),
10511066
);
1067+
1068+
mockSessionConfigStore.getPersistedConfigOptions.mockReset();
1069+
mockSessionConfigStore.getPersistedConfigOptions.mockReturnValue(
1070+
undefined,
1071+
);
1072+
mockSessionConfigStore.setPersistedConfigOptions.mockReset();
10521073
});
10531074

10541075
it("shows the selected cloud model and reasoning before preview config loads", () => {
@@ -6637,49 +6658,121 @@ describe("SessionService", () => {
66376658

66386659
it("rolls back on API failure", async () => {
66396660
const service = getSessionService();
6640-
mockSessionStoreSetters.getSessionByTaskId.mockReturnValue(
6641-
createMockSession({
6642-
configOptions: [
6643-
{
6644-
id: "mode",
6645-
name: "Mode",
6646-
type: "select",
6647-
category: "mode",
6648-
currentValue: "default",
6649-
options: [],
6650-
},
6651-
],
6652-
}),
6661+
let currentSession = createMockSession({
6662+
configOptions: [
6663+
{
6664+
id: "mode",
6665+
name: "Mode",
6666+
type: "select",
6667+
category: "mode",
6668+
currentValue: "default",
6669+
options: [],
6670+
},
6671+
],
6672+
});
6673+
mockSessionStoreSetters.getSessionByTaskId.mockImplementation(
6674+
() => currentSession,
6675+
);
6676+
mockSessionStoreSetters.updateSession.mockImplementation(
6677+
(_taskRunId, updates) => {
6678+
currentSession = { ...currentSession, ...updates };
6679+
},
66536680
);
66546681
mockTrpcAgent.setConfigOption.mutate.mockRejectedValue(
66556682
new Error("Failed"),
66566683
);
66576684

66586685
await service.setSessionConfigOption("task-123", "mode", "acceptEdits");
66596686

6660-
// Should rollback
6661-
expect(mockSessionStoreSetters.updateSession).toHaveBeenLastCalledWith(
6662-
"run-123",
6663-
{
6664-
configOptions: [
6665-
{
6666-
id: "mode",
6667-
name: "Mode",
6668-
type: "select",
6669-
category: "mode",
6670-
currentValue: "default",
6671-
options: [],
6672-
},
6673-
],
6687+
expect(currentSession.configOptions).toEqual([
6688+
expect.objectContaining({
6689+
id: "mode",
6690+
currentValue: "default",
6691+
}),
6692+
]);
6693+
expect(
6694+
mockSessionConfigStore.setPersistedConfigOptions,
6695+
).toHaveBeenLastCalledWith("run-123", [
6696+
expect.objectContaining({
6697+
id: "mode",
6698+
currentValue: "default",
6699+
}),
6700+
]);
6701+
});
6702+
6703+
it("preserves a newer successful config change during rollback", async () => {
6704+
const service = getSessionService();
6705+
let currentSession = createMockSession({
6706+
configOptions: [
6707+
{
6708+
id: "mode",
6709+
name: "Mode",
6710+
type: "select",
6711+
category: "mode",
6712+
currentValue: "default",
6713+
options: [],
6714+
},
6715+
{
6716+
id: "model",
6717+
name: "Model",
6718+
type: "select",
6719+
category: "model",
6720+
currentValue: "claude-3-opus",
6721+
options: [],
6722+
},
6723+
],
6724+
});
6725+
mockSessionStoreSetters.getSessionByTaskId.mockImplementation(
6726+
() => currentSession,
6727+
);
6728+
mockSessionStoreSetters.updateSession.mockImplementation(
6729+
(_taskRunId, updates) => {
6730+
currentSession = { ...currentSession, ...updates };
66746731
},
66756732
);
6733+
6734+
let rejectModeChange: (error: Error) => void = () => undefined;
6735+
const pendingModeChange = new Promise<never>((_resolve, reject) => {
6736+
rejectModeChange = reject;
6737+
});
6738+
mockTrpcAgent.setConfigOption.mutate.mockImplementation(({ configId }) =>
6739+
configId === "mode" ? pendingModeChange : Promise.resolve({}),
6740+
);
6741+
6742+
const modeChange = service.setSessionConfigOption(
6743+
"task-123",
6744+
"mode",
6745+
"acceptEdits",
6746+
);
6747+
await service.setSessionConfigOption(
6748+
"task-123",
6749+
"model",
6750+
"claude-3-sonnet",
6751+
);
6752+
rejectModeChange(new Error("Mode change failed"));
6753+
await modeChange;
6754+
6755+
expect(currentSession.configOptions).toEqual([
6756+
expect.objectContaining({
6757+
id: "mode",
6758+
currentValue: "default",
6759+
}),
6760+
expect.objectContaining({
6761+
id: "model",
6762+
currentValue: "claude-3-sonnet",
6763+
}),
6764+
]);
66766765
expect(
66776766
mockSessionConfigStore.setPersistedConfigOptions,
66786767
).toHaveBeenLastCalledWith("run-123", [
66796768
expect.objectContaining({
66806769
id: "mode",
66816770
currentValue: "default",
66826771
}),
6772+
expect.objectContaining({
6773+
id: "model",
6774+
currentValue: "claude-3-sonnet",
6775+
}),
66836776
]);
66846777
});
66856778

‎packages/ui/src/features/sessions/sessionServiceHost.ts‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,6 @@ import {
2929
getPersistedConfigOptions,
3030
removePersistedConfigOptions,
3131
setPersistedConfigOptions,
32-
updatePersistedConfigOptionValue,
3332
} from "@posthog/ui/features/sessions/sessionConfigStore";
3433
import { sessionStoreSetters } from "@posthog/ui/features/sessions/sessionStore";
3534
import {
@@ -105,7 +104,6 @@ function buildSessionServiceDeps(): SessionServiceDeps {
105104
getPersistedConfigOptions(taskRunId) ?? undefined,
106105
setPersistedConfigOptions,
107106
removePersistedConfigOptions,
108-
updatePersistedConfigOptionValue,
109107
adapterStore: {
110108
getAdapter: (taskRunId) =>
111109
useSessionAdapterStore.getState().getAdapter(taskRunId),

0 commit comments

Comments
 (0)