History

[codex] prevent ACP assistant ID collisions after restarts (#3932)

Maxwell Young committed 3 months ago

Commit bcd640b1 parent b49a2e614 files · +90 −14Browse files

Unified · Split

14 files
  1. apps/server/src/provider/Drivers/CursorDriver.ts +2 −0
  2. apps/server/src/provider/Drivers/GrokDriver.ts +2 −0
  3. apps/server/src/provider/Layers/CursorAdapter.test.ts +5 −2
  4. apps/server/src/provider/Layers/CursorAdapter.ts +1 −0
  5. apps/server/src/provider/Layers/CursorProvider.test.ts +2 −1
  6. apps/server/src/provider/Layers/CursorProvider.ts +2 −1
  7. apps/server/src/provider/Layers/GrokAdapter.ts +1 −0
  8. apps/server/src/provider/Layers/GrokProvider.ts +6 −1
  9. apps/server/src/provider/acp/AcpJsonRpcConnection.test.ts +37 −0
  10. apps/server/src/provider/acp/AcpSessionRuntime.ts +22 −5
  11. apps/server/src/provider/acp/CursorAcpSupport.ts +2 −1
  12. apps/server/src/provider/acp/GrokAcpSupport.ts +2 −1
  13. apps/server/src/textGeneration/CursorTextGeneration.ts +3 −1
  14. apps/server/src/textGeneration/GrokTextGeneration.ts +3 −1
apps/server/src/provider/Drivers/CursorDriver.ts+2−0
@@ -101,6 +101,7 @@ export const CursorDriver: ProviderDriver<CursorSettings, CursorDriverEnv> = { · 100 unmodified lines
101101 defaultConfig: (): CursorSettings => decodeCursorSettings({}),
102102 create: ({ instanceId, displayName, accentColor, environment, enabled, config }) =>
103103 Effect.gen(function* () {
104+ const crypto = yield* Crypto.Crypto;
104105 const spawner = yield* ChildProcessSpawner.ChildProcessSpawner;
105106 const fileSystem = yield* FileSystem.FileSystem;
106107 const path = yield* Path.Path;
@@ -133,6 +134,7 @@ export const CursorDriver: ProviderDriver<CursorSettings, CursorDriverEnv> = { · 26 unmodified lines
133134
134135 const checkProvider = checkCursorProviderStatus(effectiveConfig, processEnv).pipe(
135136 Effect.map(stampIdentity),
137+ Effect.provideService(Crypto.Crypto, crypto),
136138 Effect.provideService(ChildProcessSpawner.ChildProcessSpawner, spawner),
137139 Effect.provideService(FileSystem.FileSystem, fileSystem),
138140 Effect.provideService(Path.Path, path),
apps/server/src/provider/Drivers/GrokDriver.ts+2−0
@@ -84,6 +84,7 @@ export const GrokDriver: ProviderDriver<GrokSettings, GrokDriverEnv> = { · 83 unmodified lines
8484 defaultConfig: (): GrokSettings => decodeGrokSettings({}),
8585 create: ({ instanceId, displayName, accentColor, environment, enabled, config }) =>
8686 Effect.gen(function* () {
87+ const crypto = yield* Crypto.Crypto;
8788 const spawner = yield* ChildProcessSpawner.ChildProcessSpawner;
8889 const httpClient = yield* HttpClient.HttpClient;
8990 const serverSettings = yield* ServerSettingsService;
@@ -114,6 +115,7 @@ export const GrokDriver: ProviderDriver<GrokSettings, GrokDriverEnv> = { · 24 unmodified lines
114115
115116 const checkProvider = checkGrokProviderStatus(effectiveConfig, processEnv).pipe(
116117 Effect.map(stampIdentity),
118+ Effect.provideService(Crypto.Crypto, crypto),
117119 Effect.provideService(ChildProcessSpawner.ChildProcessSpawner, spawner),
118120 );
119121
apps/server/src/provider/Layers/CursorAdapter.test.ts+5−2
@@ -228,7 +228,7 @@ cursorAdapterTestLayer("CursorAdapterLive", (it) => { · 227 unmodified lines
228228 assert.isDefined(delta);
229229 if (delta?.type === "content.delta") {
230230 assert.equal(delta.payload.delta, "hello from mock");
231− assert.match(String(delta.itemId), /^assistant:mock-session-1:segment:0$/);
231+ assert.match(String(delta.itemId), /^assistant:mock-session-1:runtime:[^:]+:segment:0$/);
232232 }
233233
234234 const assistantCompleted = runtimeEvents.find(
@@ -687,7 +687,10 @@ cursorAdapterTestLayer("CursorAdapterLive", (it) => { · 452 unmodified lines
687687 if (contentDelta?.type === "content.delta") {
688688 assert.equal(String(contentDelta.turnId), String(turn.turnId));
689689 assert.equal(contentDelta.payload.delta, "hello from mock");
690− assert.equal(String(contentDelta.itemId), "assistant:mock-session-1:segment:0");
690+ assert.match(
691+ String(contentDelta.itemId),
692+ /^assistant:mock-session-1:runtime:[^:]+:segment:0$/,
693+ );
691694 }
692695 });
693696
apps/server/src/provider/Layers/CursorAdapter.ts+1−0
@@ -558,6 +558,7 @@ export function makeCursorAdapter( · 557 unmodified lines
558558 : {}),
559559 ...acpNativeLoggers,
560560 }).pipe(
561+ Effect.provideService(Crypto.Crypto, crypto),
561562 Effect.provideService(Scope.Scope, sessionScope),
562563 Effect.mapError(
563564 (cause) =>
apps/server/src/provider/Layers/CursorProvider.test.ts+2−1
@@ -1,6 +1,7 @@
11 import * as NodeOS from "node:os";
22
33 import * as NodeServices from "@effect/platform-node/NodeServices";
4+import type * as Crypto from "effect/Crypto";
45 import * as Effect from "effect/Effect";
56 import * as FileSystem from "effect/FileSystem";
67 import * as Path from "effect/Path";
@@ -28,7 +29,7 @@ const runNode = <A, E>( · 21 unmodified lines
2829 effect: Effect.Effect<
2930 A,
3031 E,
31− ChildProcessSpawner.ChildProcessSpawner | FileSystem.FileSystem | Path.Path
32+ ChildProcessSpawner.ChildProcessSpawner | Crypto.Crypto | FileSystem.FileSystem | Path.Path
3233 >,
3334 ): Promise<A> => Effect.runPromise(effect.pipe(Effect.provide(NodeServices.layer)));
3435
apps/server/src/provider/Layers/CursorProvider.ts+2−1
@@ -11,6 +11,7 @@ import type { · 10 unmodified lines
1111 import { ProviderDriverKind } from "@t3tools/contracts";
1212 import type * as EffectAcpSchema from "effect-acp/schema";
1313 import { causeErrorTag } from "@t3tools/shared/observability";
14+import * as Crypto from "effect/Crypto";
1415 import * as DateTime from "effect/DateTime";
1516 import * as Effect from "effect/Effect";
1617 import * as Exit from "effect/Exit";
@@ -992,7 +993,7 @@ export const checkCursorProviderStatus = Effect.fn("checkCursorProviderStatus")( · 975 unmodified lines
992993 ): Effect.fn.Return<
993994 ServerProviderDraft,
994995 never,
995− ChildProcessSpawner.ChildProcessSpawner | FileSystem.FileSystem | Path.Path
996+ ChildProcessSpawner.ChildProcessSpawner | Crypto.Crypto | FileSystem.FileSystem | Path.Path
996997 > {
997998 const checkedAt = DateTime.formatIso(yield* DateTime.now);
998999 const fallbackModels = getCursorFallbackModels(cursorSettings);
apps/server/src/provider/Layers/GrokAdapter.ts+1−0
@@ -596,6 +596,7 @@ export function makeGrokAdapter(grokSettings: GrokSettings, options?: GrokAdapte · 595 unmodified lines
596596 : {}),
597597 ...acpNativeLoggers,
598598 }).pipe(
599+ Effect.provideService(Crypto.Crypto, crypto),
599600 Effect.provideService(Scope.Scope, sessionScope),
600601 Effect.mapError(
601602 (cause) =>
apps/server/src/provider/Layers/GrokProvider.ts+6−1
@@ -7,6 +7,7 @@ import { · 6 unmodified lines
77 } from "@t3tools/contracts";
88 import type * as EffectAcpSchema from "effect-acp/schema";
99 import { causeErrorTag } from "@t3tools/shared/observability";
10+import * as Crypto from "effect/Crypto";
1011 import * as DateTime from "effect/DateTime";
1112 import * as Effect from "effect/Effect";
1213 import * as Exit from "effect/Exit";
@@ -167,7 +168,11 @@ const runGrokVersionCommand = ( · 154 unmodified lines
167168 export const checkGrokProviderStatus = Effect.fn("checkGrokProviderStatus")(function* (
168169 grokSettings: GrokSettings,
169170 environment: NodeJS.ProcessEnv = process.env,
170−): Effect.fn.Return<ServerProviderDraft, never, ChildProcessSpawner.ChildProcessSpawner> {
171+): Effect.fn.Return<
172+ ServerProviderDraft,
173+ never,
174+ ChildProcessSpawner.ChildProcessSpawner | Crypto.Crypto
175+> {
171176 const checkedAt = DateTime.formatIso(yield* DateTime.now);
172177 const fallbackModels = grokModelsFromSettings(grokSettings.customModels);
173178
apps/server/src/provider/acp/AcpJsonRpcConnection.test.ts+37−0
@@ -116,6 +116,43 @@ describe("AcpSessionRuntime", () => { · 115 unmodified lines
116116 ),
117117 );
118118
119+ it.effect("keeps assistant item IDs unique when a provider session restarts", () => {
120+ const collectFirstAssistantItemId = Effect.gen(function* () {
121+ const runtime = yield* AcpSessionRuntime.AcpSessionRuntime;
122+ const started = yield* runtime.start();
123+ expect(started.sessionId).toBe("mock-session-1");
124+
125+ yield* runtime.prompt({
126+ prompt: [{ type: "text", text: "hi" }],
127+ });
128+
129+ const events = Array.from(yield* Stream.runCollect(Stream.take(runtime.getEvents(), 4)));
130+ const assistantStart = events.find((event) => event._tag === "AssistantItemStarted");
131+ expect(assistantStart?._tag).toBe("AssistantItemStarted");
132+ return assistantStart?._tag === "AssistantItemStarted" ? assistantStart.itemId : "";
133+ }).pipe(
134+ Effect.provide(
135+ AcpSessionRuntime.layer({
136+ spawn: {
137+ command: mockAgentCommand,
138+ args: mockAgentArgs,
139+ },
140+ cwd: process.cwd(),
141+ clientInfo: { name: "t3-test", version: "0.0.0" },
142+ authMethodId: "test",
143+ }),
144+ ),
145+ Effect.scoped,
146+ );
147+
148+ return Effect.gen(function* () {
149+ const beforeRestart = yield* collectFirstAssistantItemId;
150+ const afterRestart = yield* collectFirstAssistantItemId;
151+
152+ expect(afterRestart).not.toBe(beforeRestart);
153+ }).pipe(Effect.provide(NodeServices.layer));
154+ });
155+
119156 it.effect("drops session updates emitted for a child ACP session", () =>
120157 Effect.gen(function* () {
121158 const runtime = yield* AcpSessionRuntime.AcpSessionRuntime;
apps/server/src/provider/acp/AcpSessionRuntime.ts+22−5
@@ -1,6 +1,7 @@
11 import * as Cause from "effect/Cause";
22 import * as Clock from "effect/Clock";
33 import * as Context from "effect/Context";
4+import * as Crypto from "effect/Crypto";
45 import * as Deferred from "effect/Deferred";
56 import * as Duration from "effect/Duration";
67 import * as Effect from "effect/Effect";
@@ -270,14 +271,24 @@ export const make = ( · 263 unmodified lines
270271 ): Effect.Effect<
271272 AcpSessionRuntime["Service"],
272273 EffectAcpErrors.AcpError,
273− ChildProcessSpawner.ChildProcessSpawner | Scope.Scope
274+ ChildProcessSpawner.ChildProcessSpawner | Crypto.Crypto | Scope.Scope
274275 > =>
275276 Effect.gen(function* () {
277+ const crypto = yield* Crypto.Crypto;
276278 const spawner = yield* ChildProcessSpawner.ChildProcessSpawner;
277279 const runtimeScope = yield* Scope.Scope;
278280 const eventQueue = yield* Queue.unbounded<AcpSessionRuntimeEvent>();
279281 const modeStateRef = yield* Ref.make<AcpSessionModeState | undefined>(undefined);
280282 const toolCallsRef = yield* Ref.make(new Map<string, AcpToolCallState>());
283+ const assistantItemRuntimeId = yield* crypto.randomUUIDv4.pipe(
284+ Effect.mapError(
285+ (cause) =>
286+ new EffectAcpErrors.AcpTransportError({
287+ detail: "Failed to generate an ACP assistant item runtime identifier.",
288+ cause,
289+ }),
290+ ),
291+ );
281292 const assistantSegmentRef = yield* Ref.make<AcpAssistantSegmentState>({ nextSegmentIndex: 0 });
282293 const configOptionsRef = yield* Ref.make(sessionConfigOptionsFromSetup(undefined));
283294 const startStateRef = yield* Ref.make<AcpStartState>({ _tag: "NotStarted" });
@@ -387,6 +398,7 @@ export const make = ( · 103 unmodified lines
387398 modeStateRef,
388399 toolCallsRef,
389400 assistantSegmentRef,
401+ assistantItemRuntimeId,
390402 params: notification,
391403 });
392404 }),
@@ -802,7 +814,7 @@ export const layer = ( · 409 unmodified lines
802814 ): Layer.Layer<
803815 AcpSessionRuntime,
804816 EffectAcpErrors.AcpError,
805− ChildProcessSpawner.ChildProcessSpawner
817+ ChildProcessSpawner.ChildProcessSpawner | Crypto.Crypto
806818 > => Layer.effect(AcpSessionRuntime, make(options));
807819
808820 function sessionConfigOptionsFromSetup(
@@ -834,12 +846,14 @@ const handleSessionUpdate = ({ · 25 unmodified lines
834846 modeStateRef,
835847 toolCallsRef,
836848 assistantSegmentRef,
849+ assistantItemRuntimeId,
837850 params,
838851 }: {
839852 readonly queue: Queue.Queue<AcpSessionRuntimeEvent>;
840853 readonly modeStateRef: Ref.Ref<AcpSessionModeState | undefined>;
841854 readonly toolCallsRef: Ref.Ref<Map<string, AcpToolCallState>>;
842855 readonly assistantSegmentRef: Ref.Ref<AcpAssistantSegmentState>;
856+ readonly assistantItemRuntimeId: string;
843857 readonly params: EffectAcpSchema.SessionNotification;
844858 }): Effect.Effect<void> =>
845859 Effect.gen(function* () {
@@ -887,6 +901,7 @@ const handleSessionUpdate = ({ · 41 unmodified lines
887901 queue,
888902 assistantSegmentRef,
889903 sessionId: params.sessionId,
904+ assistantItemRuntimeId,
890905 });
891906 yield* Queue.offer(queue, {
892907 ...event,
@@ -924,17 +939,19 @@ function shouldEmitToolCallUpdate( · 31 unmodified lines
924939 return previous === undefined || previous.title !== next.title || previous.detail !== next.detail;
925940 }
926941
927−const assistantItemId = (sessionId: string, segmentIndex: number) =>
928− `assistant:${sessionId}:segment:${segmentIndex}`;
942+const assistantItemId = (sessionId: string, runtimeId: string, segmentIndex: number) =>
943+ `assistant:${sessionId}:runtime:${runtimeId}:segment:${segmentIndex}`;
929944
930945 const ensureActiveAssistantSegment = ({
931946 queue,
932947 assistantSegmentRef,
933948 sessionId,
949+ assistantItemRuntimeId,
934950 }: {
935951 readonly queue: Queue.Queue<AcpSessionRuntimeEvent>;
936952 readonly assistantSegmentRef: Ref.Ref<AcpAssistantSegmentState>;
937953 readonly sessionId: string;
954+ readonly assistantItemRuntimeId: string;
938955 }) =>
939956 Ref.modify<AcpAssistantSegmentState, EnsureActiveAssistantSegmentResult>(
940957 assistantSegmentRef,
@@ -942,7 +959,7 @@ const ensureActiveAssistantSegment = ({ · 1 unmodified line
942959 if (current.activeItemId) {
943960 return [{ itemId: current.activeItemId }, current] as const;
944961 }
945− const itemId = assistantItemId(sessionId, current.nextSegmentIndex);
962+ const itemId = assistantItemId(sessionId, assistantItemRuntimeId, current.nextSegmentIndex);
946963 return [
947964 {
948965 itemId,
apps/server/src/provider/acp/CursorAcpSupport.ts+2−1
@@ -1,4 +1,5 @@
11 import { type CursorSettings, type ProviderOptionSelection } from "@t3tools/contracts";
2+import * as Crypto from "effect/Crypto";
23 import * as Effect from "effect/Effect";
34 import * as Layer from "effect/Layer";
45 import * as Scope from "effect/Scope";
@@ -50,7 +51,7 @@ export const makeCursorAcpRuntime = ( · 45 unmodified lines
5051 ): Effect.Effect<
5152 AcpSessionRuntime.AcpSessionRuntime["Service"],
5253 EffectAcpErrors.AcpError,
53− Scope.Scope
54+ Crypto.Crypto | Scope.Scope
5455 > =>
5556 Effect.gen(function* () {
5657 const acpContext = yield* Layer.build(
apps/server/src/provider/acp/GrokAcpSupport.ts+2−1
@@ -1,4 +1,5 @@
11 import { type GrokSettings, ProviderDriverKind } from "@t3tools/contracts";
2+import * as Crypto from "effect/Crypto";
23 import * as Effect from "effect/Effect";
34 import * as Layer from "effect/Layer";
45 import * as Scope from "effect/Scope";
@@ -55,7 +56,7 @@ export const makeGrokAcpRuntime = ( · 50 unmodified lines
5556 ): Effect.Effect<
5657 AcpSessionRuntime.AcpSessionRuntime["Service"],
5758 EffectAcpErrors.AcpError,
58− Scope.Scope
59+ Crypto.Crypto | Scope.Scope
5960 > =>
6061 Effect.gen(function* () {
6162 const acpContext = yield* Layer.build(
apps/server/src/textGeneration/CursorTextGeneration.ts+3−1
@@ -1,3 +1,4 @@
1+import * as Crypto from "effect/Crypto";
12 import * as Effect from "effect/Effect";
23 import * as Option from "effect/Option";
34 import * as Ref from "effect/Ref";
@@ -38,6 +39,7 @@ export const makeCursorTextGeneration = Effect.fn("makeCursorTextGeneration")(fu · 34 unmodified lines
3839 cursorSettings: CursorSettings,
3940 environment?: NodeJS.ProcessEnv,
4041 ) {
42+ const crypto = yield* Crypto.Crypto;
4143 const commandSpawner = yield* ChildProcessSpawner.ChildProcessSpawner;
4244 const resolvedEnvironment = environment ?? process.env;
4345
@@ -66,7 +68,7 @@ export const makeCursorTextGeneration = Effect.fn("makeCursorTextGeneration")(fu · 22 unmodified lines
6668 childProcessSpawner: commandSpawner,
6769 cwd,
6870 clientInfo: { name: "t3-code-git-text", version: "0.0.0" },
69− });
71+ }).pipe(Effect.provideService(Crypto.Crypto, crypto));
7072
7173 yield* runtime.handleSessionUpdate((notification) => {
7274 const update = notification.update;
apps/server/src/textGeneration/GrokTextGeneration.ts+3−1
@@ -1,3 +1,4 @@
1+import * as Crypto from "effect/Crypto";
12 import * as Effect from "effect/Effect";
23 import * as Option from "effect/Option";
34 import * as Ref from "effect/Ref";
@@ -37,6 +38,7 @@ export const makeGrokTextGeneration = Effect.fn("makeGrokTextGeneration")(functi · 33 unmodified lines
3738 grokSettings: GrokSettings,
3839 environment: NodeJS.ProcessEnv = process.env,
3940 ) {
41+ const crypto = yield* Crypto.Crypto;
4042 const commandSpawner = yield* ChildProcessSpawner.ChildProcessSpawner;
4143
4244 const runGrokJson = <S extends Schema.Top>({
@@ -65,7 +67,7 @@ export const makeGrokTextGeneration = Effect.fn("makeGrokTextGeneration")(functi · 22 unmodified lines
6567 childProcessSpawner: commandSpawner,
6668 cwd,
6769 clientInfo: { name: "t3-code-git-text", version: "0.0.0" },
68− });
70+ }).pipe(Effect.provideService(Crypto.Crypto, crypto));
6971
7072 yield* runtime.handleSessionUpdate((notification) => {
7173 const update = notification.update;