LLM-24890 Generate a chat name based on the first message#52
Conversation
| } | ||
| } | ||
|
|
||
| private extractNotificationThreadId(notification: ServerNotification): string | null { |
There was a problem hiding this comment.
Why we need this unsafe method?
App-server listener should already be session-scoped.
Plus we can take threadId directly if we pass specific type after switch-case.
| rateLimits: RateLimitsMap | null; | ||
| account: Account | null; | ||
| cwd: string; | ||
| threadRenameState: "notScheduled" | "running" | "done"; |
There was a problem hiding this comment.
Can we simplify the state here? Ideally we don't save it and on first prompt check if it's a first user prompt and schedule rename if it is.
In worst case we can introduce this flag instead of rename state.
| private scheduleThreadRenameFromContext( | ||
| sessionState: SessionState, | ||
| modelId: ModelId, | ||
| summary: "none" | null, |
There was a problem hiding this comment.
Maybe we can always disable streaming reasoning for thread naming process?
Do they stream this reasoning in CLI?
ModelId is already defined in the sessionState, why we need to duplicate?
| resolve(event); | ||
| }); | ||
| }); | ||
| async awaitTurnCompletedForThread(threadId: string, turnId: string | null = null): Promise<TurnCompletedNotification> { |
There was a problem hiding this comment.
Do you really need to wait for specific turn? You can't have multiple running turns in the same session.
Also to me it's enough to add threadId as parameter, no need to duplicate it in the name.
awaitTurnCompleted(threadId)
| } | ||
| } | ||
|
|
||
| private handleTurnCompleted(event: TurnCompletedNotification): void { |
There was a problem hiding this comment.
Why we need all these methods?
| @@ -265,9 +264,9 @@ describe('ACP server test', { timeout: 40_000 }, () => { | |||
|
|
|||
| // Trigger notifications after both prompts - should produce only 3 events, not 6 | |||
| const serverNotifications: ServerNotification[] = [ | |||
There was a problem hiding this comment.
Why we renamed it and introduced parameter for the function loadNotifications? As far as I can see it only have single usage.
| }); | ||
| forkedThreadId = fork.thread.id; | ||
|
|
||
| const started = await this.codexClient.turnStart({ |
There was a problem hiding this comment.
Is this approach copied from VS Code implementation including prompt text?
I would expect official API from the app-server, we can ask about this codex team.
There was a problem hiding this comment.
It is possible an ACP agent does not support thread name providing. For that case we can fallback to some AIA logic which will be written and tested inside AIA chat and will be common for all agents.
I also don't think we hgave to write such business logic in the adapter with custom promts, models, params, etc
| return await this.codexClient.awaitTurnCompleted(); | ||
| // Wait for completion of the specific started turn. | ||
| // If turnInterrupt() was called, Codex will send turn/completed with status "interrupted". | ||
| return await this.codexClient.awaitTurnCompletedForThread(request.sessionId, started?.turn?.id ?? null); |
There was a problem hiding this comment.
it seems it is a valid fix and it should be merged even withpout the feature itself. but worth an additional test which reproduces possible issue with previous versions without specific thread id
3 participants
No description provided.