-
Notifications
You must be signed in to change notification settings - Fork 0
Migrate to new architecture [bad] #31
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Migrate to new architecture [bad] #31
Conversation
Co-authored-by: jonas <jonas@nustom.com>
|
Cursor Agent can help with this pull request. Just |
| case "FileAttachmentEvent": { | ||
| // File attachments are handled during LLM request, not stored in messages | ||
| return { ...ctx, nextEventNumber } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: File attachments never sent to LLM
The FileAttachmentEvent case in the reducer says "handled during LLM request, not stored in messages" and only increments nextEventNumber. However, llm-turn.ts has no file handling code - the contextToPrompt function simply converts ctx.messages to a prompt. Since file attachments are never added to messages and the LLM turn doesn't process them separately, images and files added via the --image CLI flag are persisted but never actually included in LLM requests. The old deleted llm.ts had explicit code to read files and add them as Prompt.filePart - this functionality is missing from the new architecture.
Additional Locations (1)
This commit introduces explicit CLI modes (tui, script, piped, auto) and refactors event handling to provide more detailed output and better TUI integration. Co-authored-by: jonas <jonas@nustom.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Interrupted turn partial response not added to LLM context
The AgentTurnInterruptedEvent case in the event reducer doesn't add the partialResponse to the messages array. In the old architecture, when an LLM response was interrupted, the partial response was included as an assistant message and a user message explaining the interruption was added. Without this, the LLM loses context about what was said before the interruption. The test comment at line 432 in test/cli.e2e.test.ts explicitly expects this behavior but the reducer only clears turn state without preserving the partial response content.
src/event-reducer.ts#L138-L146
mini-agent/src/event-reducer.ts
Lines 138 to 146 in e032075
| case "AgentTurnInterruptedEvent": | |
| case "AgentTurnFailedEvent": { | |
| // Turn ended (failed or interrupted) - clear in-progress state | |
| return { | |
| ...ctx, | |
| agentTurnStartedAtEventId: Option.none(), | |
| nextEventNumber | |
| } |
| ) | ||
|
|
||
| // Add event to trigger LLM turn | ||
| yield* agent.addEvent(userEvent) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Missing delay causes race condition in event subscription
The code subscribes to agent.events with Effect.fork and immediately calls agent.addEvent without waiting for the subscription to be active. Since broadcastDynamic doesn't replay events, late subscribers miss events. Other locations in the codebase (src/cli/commands.ts line 395 and src/http.ts line 123) correctly add Effect.sleep("10 millis") after forking to ensure the subscription is ready. Without this delay, the UserMessageEvent and subsequent LLM response events may be missed by the subscriber.
Additional Locations (2)
Co-authored-by: jonas <jonas@nustom.com>
|
Cursor Agent can help with this pull request. Just |
jonastemplestein
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cursor fix
|
|
||
| if (options.raw) { | ||
| if (Schema.is(TextDeltaEvent)(event) && !options.showEphemeral) { | ||
| if (event._tag === "TextDeltaEvent" && !options.showEphemeral) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you using _tag directly? That seems wrong! Isn't Schema.is more idiomatic?
| } | ||
|
|
||
| // --script flag is alias for --mode script | ||
| if (options.script) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need this option anymore?
| }) | ||
| const SimpleSystemPrompt = Schema.Struct({ | ||
| _tag: Schema.Literal("SystemPrompt"), | ||
| content: Schema.String |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No this is rubbish - there should be functions on the actual schemas to create them from
Just a message
| ) | ||
|
|
||
| const scriptInteractiveLoop = (contextName: string, options: OutputOptions) => | ||
| const isUserMessage = (e: ScriptInputEvent): e is typeof UserMessageEvent.Type | typeof SimpleUserMessage.Type => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rubbish. Delete
Should be schema.is (though the whole helper function here should be deleted)
| if (Schema.is(UserMessageEvent)(event)) { | ||
| yield* contextService.addEvents(contextName, [event]).pipe( | ||
| Stream.runForEach((outputEvent) => handleEvent(outputEvent, options)) | ||
| if (isUserMessage(event)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Schema.is
| yield* Effect.logDebug("Starting chat session", { mode, explicitMode: options.mode }) | ||
|
|
||
| const contextName = Option.getOrElse(options.name, generateRandomContextName) | ||
| const agentName = contextName as AgentName |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Type assertion? Is there no better way to do this in effect?
| yield* Console.log(" POST /context/:contextName") | ||
| yield* Console.log(" Send JSONL events, receive SSE stream") | ||
| yield* Console.log(" Content-Type: application/x-ndjson") | ||
| yield* Console.log(" POST /agent/:agentName") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add GET on same path to just tap into event stream
| type FeedItem = typeof FeedItem.Type | ||
|
|
||
| type FeedAction = { event: ContextEvent; isHistory: boolean } | ||
| type FeedAction = { event: ChatEvent; isHistory: boolean } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No this is a pointless abstraction
Instead of Chat we want to use AgentTUIFeed
And AgentTUIFeedItem
And there should be no FeedAction
Instead the AgentTUIFeedItem base schema should have isHistory
Everything has isHistory true except for the feed items after the last session started event
Probably needs a finalizer or something that runs after the reducer to determine this
| const FeedItemRenderer = memo<{ item: FeedItem }>(({ item }) => { | ||
| if (!("_tag" in item)) return null | ||
|
|
||
| switch (item._tag) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See if there is a more idiomatic way to do this in effect.schema
| durationMs | ||
| }), | ||
|
|
||
| textDelta: ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this EventBuilder pattern?
Check the effect source code to see if there is a more idiomatic way to e.g. put these factory methods on the Schemas themselves
|
Addressed the interruption gap: Learn more about Cursor Agents |
jonastemplestein
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cursor please fix comments
|
On it. Already fixed the interrupted-turn gap ( Learn more about Cursor Agents |
Integrate the new actor-based architecture into
src/and remove thesrc/new-architecturefolder, establishing it as the main architecture.Linear Issue: ITE-3285
Note
Replaces legacy context services with an event-sourced MiniAgent architecture, revamps CLI/TUI and HTTP interfaces, and updates docs/tests accordingly.
domain.ts(events/types),mini-agent.ts,agent-registry.ts,event-reducer.ts,event-store.ts+event-store-fs.ts, andllm-turn.ts.context.model.ts,context.repository.ts,context.service.ts,llm.ts) and unifies errors.makeContextNamereturnsagentName), file storage under.mini-agent/contexts.chatcommand with explicit--mode(tui|script|piped|auto), improved script input (JSONL), piped and single-turn flows, and optional image attachments.opentui-chat.tsx) andchat-ui.tsto consume newContextEvents viaAgentRegistry.cli/main.ts) to new services and telemetry./context/:namewithPOST /agent/:agentName(JSON body), SSE now streams historical + current turn events.AgentRegistryand stream responses.EventStoretests; removes old new-architecture scaffolding.Written by Cursor Bugbot for commit 73bf894. This will update automatically on new commits. Configure here.