fix(client): address conversation assembly review findings
This commit is contained in:
@@ -238,7 +238,7 @@ export class ConversationNodeAssembler {
|
|||||||
}
|
}
|
||||||
this.applyPendingMatches(pending, affected)
|
this.applyPendingMatches(pending, affected)
|
||||||
this.replayContexts(affected)
|
this.replayContexts(affected)
|
||||||
if ((fresh.length > 0 || previousHasMore !== hasMore) && this.replayDependencies()) {
|
if ((this.revised.size > 0 || previousHasMore !== hasMore) && this.replayDependencies()) {
|
||||||
publication = 'immediate'
|
publication = 'immediate'
|
||||||
}
|
}
|
||||||
if (changedLocations.size > 0) publication = 'immediate'
|
if (changedLocations.size > 0) publication = 'immediate'
|
||||||
@@ -563,18 +563,18 @@ export class ConversationNodeAssembler {
|
|||||||
|
|
||||||
private replayRevisedDependents(): boolean {
|
private replayRevisedDependents(): boolean {
|
||||||
const pending = [...this.revised]
|
const pending = [...this.revised]
|
||||||
const replayed = new Set<InternalContext>()
|
const affected = new Set<InternalContext>()
|
||||||
for (let index = 0; index < pending.length; index++) {
|
for (let index = 0; index < pending.length; index++) {
|
||||||
const dependency = pending[index]
|
const dependency = pending[index]
|
||||||
if (dependency === undefined) continue
|
if (dependency === undefined) continue
|
||||||
for (const dependent of this.dependents.get(dependency.key) ?? []) {
|
for (const dependent of this.dependents.get(dependency.key) ?? []) {
|
||||||
if (replayed.has(dependent)) continue
|
if (affected.has(dependent)) continue
|
||||||
replayed.add(dependent)
|
affected.add(dependent)
|
||||||
this.replayContext(dependent)
|
|
||||||
pending.push(dependent)
|
pending.push(dependent)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
return replayed.size > 0
|
this.replayContexts(affected)
|
||||||
|
return affected.size > 0
|
||||||
}
|
}
|
||||||
|
|
||||||
private readerFor(
|
private readerFor(
|
||||||
|
|||||||
@@ -344,6 +344,7 @@ export class Session implements SessionFace {
|
|||||||
// §D.2 continuity assertion: on violation drop the page fail-soft rather than render an out-of-order stream.
|
// §D.2 continuity assertion: on violation drop the page fail-soft rather than render an out-of-order stream.
|
||||||
console.error(`[web-runtime] history page discontinuous: tail seq ${tail?.event.seq} vs baseSeq ${this.baseSeq}`)
|
console.error(`[web-runtime] history page discontinuous: tail seq ${tail?.event.seq} vs baseSeq ${this.baseSeq}`)
|
||||||
this.hasMore = false
|
this.hasMore = false
|
||||||
|
this.conversation.prepend([], false)
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
this.events = [...older.map(e => e.event), ...this.events]
|
this.events = [...older.map(e => e.event), ...this.events]
|
||||||
|
|||||||
@@ -458,6 +458,73 @@ describe('ConversationNodeAssembler', () => {
|
|||||||
expect([...chatSnapshot(assembler)?.nodes.values() ?? []][0]?.data).toBe(2)
|
expect([...chatSnapshot(assembler)?.nodes.values() ?? []][0]?.data).toBe(2)
|
||||||
})
|
})
|
||||||
|
|
||||||
|
it('replays a transitive dependency closure in start order', () => {
|
||||||
|
const sourceA: ConversationNodeDefinition<number> = {
|
||||||
|
kind: 'diamond-a',
|
||||||
|
match: (event) => {
|
||||||
|
if (event.type === 'user/message') return { id: 'one', role: 'start' }
|
||||||
|
if ((event.type as string) === 'diamond/a') return { id: 'one', role: 'update' }
|
||||||
|
return null
|
||||||
|
},
|
||||||
|
start: () => 1,
|
||||||
|
update: (_context, match) => (match.event.data as unknown as { value: number }).value,
|
||||||
|
buildViewNode: () => null,
|
||||||
|
}
|
||||||
|
const sourceX: ConversationNodeDefinition<number> = {
|
||||||
|
kind: 'diamond-x',
|
||||||
|
match: (event) => {
|
||||||
|
if (event.type === 'turn/start') return { id: 'one', role: 'start' }
|
||||||
|
if ((event.type as string) === 'diamond/x') return { id: 'one', role: 'update' }
|
||||||
|
return null
|
||||||
|
},
|
||||||
|
start: () => 10,
|
||||||
|
update: (_context, match) => (match.event.data as unknown as { value: number }).value,
|
||||||
|
buildViewNode: () => null,
|
||||||
|
}
|
||||||
|
const middle: ConversationNodeDefinition<number> = {
|
||||||
|
kind: 'diamond-b',
|
||||||
|
match: event => event.type === 'assistant/message'
|
||||||
|
? { id: 'one', role: 'start' }
|
||||||
|
: null,
|
||||||
|
start: (_context, _match, reader) => (
|
||||||
|
(reader.previous<number>('diamond-a')?.state ?? 0)
|
||||||
|
+ (reader.previous<number>('diamond-x')?.state ?? 0)
|
||||||
|
),
|
||||||
|
update: context => context.state,
|
||||||
|
buildViewNode: context => node(context, context.state),
|
||||||
|
}
|
||||||
|
const consumer: ConversationNodeDefinition<number> = {
|
||||||
|
kind: 'diamond-c',
|
||||||
|
match: event => event.type === 'tool/call'
|
||||||
|
? { id: 'one', role: 'start' }
|
||||||
|
: null,
|
||||||
|
start: (_context, _match, reader) => (
|
||||||
|
(reader.previous<number>('diamond-a')?.state ?? 0) * 100
|
||||||
|
+ (reader.previous<number>('diamond-b')?.state ?? 0)
|
||||||
|
),
|
||||||
|
update: context => context.state,
|
||||||
|
buildViewNode: context => node(context, context.state),
|
||||||
|
}
|
||||||
|
const assembler = new ConversationNodeAssembler(
|
||||||
|
new TestEventDefinitions([sourceA, sourceX, middle, consumer]),
|
||||||
|
new TestViewDefinitions([testView()]),
|
||||||
|
)
|
||||||
|
assembler.replaceWindow([
|
||||||
|
input(at(1, 'user/message', { id: 'source', content: [], source: { kind: 'user' } })),
|
||||||
|
input(at(2, 'turn/start', { turn: 1 })),
|
||||||
|
input(at(3, 'assistant/message', { turn: 1, step: 1, message: { role: 'assistant', content: [] } })),
|
||||||
|
input(at(4, 'tool/call', { turn: 1, step: 1, callId: 'call', name: 'x', arguments: '{}' })),
|
||||||
|
], false)
|
||||||
|
|
||||||
|
assembler.append(input(at(5, 'diamond/x', { value: 20 })))
|
||||||
|
assembler.append(input(at(6, 'diamond/a', { value: 2 })))
|
||||||
|
assembler.flush()
|
||||||
|
|
||||||
|
const value = [...chatSnapshot(assembler)?.nodes.values() ?? []]
|
||||||
|
.find(candidate => candidate.kind === 'diamond-c')
|
||||||
|
expect(value?.data).toBe(222)
|
||||||
|
})
|
||||||
|
|
||||||
it('replays Location-derived State and rebuilds only owned Nodes when a step closes', () => {
|
it('replays Location-derived State and rebuilds only owned Nodes when a step closes', () => {
|
||||||
const apply = vi.fn()
|
const apply = vi.fn()
|
||||||
const starts = vi.fn((
|
const starts = vi.fn((
|
||||||
|
|||||||
@@ -122,14 +122,26 @@ function tailData(context: ConversationNodeContext<TurnTailState>): TurnTailChat
|
|||||||
.filter((candidate): candidate is Readonly<FinalAssistantChatData> => candidate.finalNode !== undefined)
|
.filter((candidate): candidate is Readonly<FinalAssistantChatData> => candidate.finalNode !== undefined)
|
||||||
.sort((left, right) => left.finalNode.seq - right.finalNode.seq)
|
.sort((left, right) => left.finalNode.seq - right.finalNode.seq)
|
||||||
const closing = finalized.findLast(hasText) ?? null
|
const closing = finalized.findLast(hasText) ?? null
|
||||||
const latest = finalized.at(-1)
|
let latestTranscriptSeq = finalized.at(-1)?.finalNode.seq
|
||||||
|
for (const match of context.matches) {
|
||||||
|
const event = match.event
|
||||||
|
const candidate = event.type === 'tool/call'
|
||||||
|
|| (event.type === 'tool/result' && isAppendSurfaceEvent(event))
|
||||||
|
|| (event.type === 'turn/end' && event.data.reason.kind === 'error')
|
||||||
|
|| (event.type as string) === 'llm/retry'
|
||||||
|
? event.seq
|
||||||
|
: undefined
|
||||||
|
if (candidate !== undefined && (latestTranscriptSeq === undefined || candidate > latestTranscriptSeq)) {
|
||||||
|
latestTranscriptSeq = candidate
|
||||||
|
}
|
||||||
|
}
|
||||||
const metrics = deriveTurnMetrics(finalized.map(candidate => candidate.finalNode)).get(end.event.data.turn)
|
const metrics = deriveTurnMetrics(finalized.map(candidate => candidate.finalNode)).get(end.event.data.turn)
|
||||||
return {
|
return {
|
||||||
turn: end.event.data.turn,
|
turn: end.event.data.turn,
|
||||||
seq: end.event.seq,
|
seq: end.event.seq,
|
||||||
time: end.event.time,
|
time: end.event.time,
|
||||||
closing,
|
closing,
|
||||||
branchUnavailable: closing === null || latest?.finalNode.seq !== closing.finalNode.seq,
|
branchUnavailable: closing === null || latestTranscriptSeq !== closing.finalNode.seq,
|
||||||
...metrics?.ttftMs === undefined ? {} : { ttftMs: metrics.ttftMs },
|
...metrics?.ttftMs === undefined ? {} : { ttftMs: metrics.ttftMs },
|
||||||
...metrics?.tokensPerSecond === undefined ? {} : { tokensPerSecond: metrics.tokensPerSecond },
|
...metrics?.tokensPerSecond === undefined ? {} : { tokensPerSecond: metrics.tokensPerSecond },
|
||||||
}
|
}
|
||||||
@@ -141,6 +153,9 @@ export const turnTailDefinition: ConversationNodeDefinition<TurnTailState> = {
|
|||||||
match: (event) => {
|
match: (event) => {
|
||||||
if (event.type === 'turn/start') return { id: String(event.data.turn), role: 'start' }
|
if (event.type === 'turn/start') return { id: String(event.data.turn), role: 'start' }
|
||||||
if (event.type === 'turn/end') return { id: String(event.data.turn), role: 'update' }
|
if (event.type === 'turn/end') return { id: String(event.data.turn), role: 'update' }
|
||||||
|
if (event.type === 'tool/call' || event.type === 'tool/result') {
|
||||||
|
return { id: String(event.data.turn), role: 'update' }
|
||||||
|
}
|
||||||
const coordinates = turnCoordinates(event)
|
const coordinates = turnCoordinates(event)
|
||||||
if (coordinates !== undefined) return { id: String(coordinates.turn), role: 'update' }
|
if (coordinates !== undefined) return { id: String(coordinates.turn), role: 'update' }
|
||||||
return null
|
return null
|
||||||
|
|||||||
@@ -3,19 +3,18 @@
|
|||||||
* between the independently implemented skeleton and chat domains; `apply.ts`
|
* between the independently implemented skeleton and chat domains; `apply.ts`
|
||||||
* owns their slot assembly.
|
* owns their slot assembly.
|
||||||
*/
|
*/
|
||||||
|
export type {} from './conversation-nodes/assistant.ts'
|
||||||
|
export type {} from './conversation-nodes/command.ts'
|
||||||
|
export type {} from './conversation-nodes/compaction.ts'
|
||||||
|
export type {} from './conversation-nodes/fallback.ts'
|
||||||
|
export type {} from './conversation-nodes/message.ts'
|
||||||
|
export type {} from './conversation-nodes/retry.ts'
|
||||||
|
export type {} from './conversation-nodes/tool.ts'
|
||||||
|
export type {} from './conversation-nodes/turn-error.ts'
|
||||||
|
export type {} from './conversation-nodes/turn-tail.ts'
|
||||||
|
|
||||||
export { apply, inject } from './apply.ts'
|
export { apply, inject } from './apply.ts'
|
||||||
export { ConversationService } from './service.ts'
|
export { ConversationService } from './service.ts'
|
||||||
export { registerAssistantConversationNode } from './conversation-nodes/assistant.ts'
|
|
||||||
export { registerChatConversationView } from './conversation-nodes/chat-snapshot-builder.ts'
|
|
||||||
export { registerCommandConversationNode } from './conversation-nodes/command.ts'
|
|
||||||
export { registerCompactionConversationNode } from './conversation-nodes/compaction.ts'
|
|
||||||
export { registerUnknownConversationFallback } from './conversation-nodes/fallback.ts'
|
|
||||||
export { registerInboxConversationNodes } from './conversation-nodes/inbox.ts'
|
|
||||||
export { registerMessageConversationNode } from './conversation-nodes/message.ts'
|
|
||||||
export { registerRetryConversationNode } from './conversation-nodes/retry.ts'
|
|
||||||
export { registerToolConversationNode } from './conversation-nodes/tool.ts'
|
|
||||||
export { registerTurnErrorConversationNode } from './conversation-nodes/turn-error.ts'
|
|
||||||
export { registerTurnTailConversationNode } from './conversation-nodes/turn-tail.ts'
|
|
||||||
export type { IConversation } from './service.ts'
|
export type { IConversation } from './service.ts'
|
||||||
|
|
||||||
export type {
|
export type {
|
||||||
|
|||||||
@@ -16,7 +16,7 @@ import { toolDefinition } from '../src/client/conversation-nodes/tool.ts'
|
|||||||
import { turnErrorDefinition } from '../src/client/conversation-nodes/turn-error.ts'
|
import { turnErrorDefinition } from '../src/client/conversation-nodes/turn-error.ts'
|
||||||
import { turnTailDefinition } from '../src/client/conversation-nodes/turn-tail.ts'
|
import { turnTailDefinition } from '../src/client/conversation-nodes/turn-tail.ts'
|
||||||
import type {
|
import type {
|
||||||
AssistantChatData, ManualCompactionChatData, RetryChatData, ToolChatData,
|
AssistantChatData, ManualCompactionChatData, RetryChatData, ToolChatData, TurnTailChatData,
|
||||||
} from '../src/client/contract/chat-nodes.ts'
|
} from '../src/client/contract/chat-nodes.ts'
|
||||||
|
|
||||||
const DEFINITIONS: readonly ConversationNodeDefinition[] = [
|
const DEFINITIONS: readonly ConversationNodeDefinition[] = [
|
||||||
@@ -413,6 +413,30 @@ describe('built-in conversation node Definitions', () => {
|
|||||||
])
|
])
|
||||||
})
|
})
|
||||||
|
|
||||||
|
it('keeps branching unavailable when a tool result follows the closing Assistant', () => {
|
||||||
|
const value = assembler([
|
||||||
|
at(1, 'turn/start', { turn: 1 }),
|
||||||
|
at(2, 'step/start', { turn: 1, step: 1 }),
|
||||||
|
at(3, 'assistant/message', {
|
||||||
|
turn: 1,
|
||||||
|
step: 1,
|
||||||
|
message: assistantMessage('assistant-before-tool', 'running a tool'),
|
||||||
|
}, { surfaceOp: 'append' }),
|
||||||
|
at(4, 'tool/call', { turn: 1, step: 1, callId: 'late-tool', name: 'read', arguments: '{}' }),
|
||||||
|
at(5, 'tool/result', {
|
||||||
|
turn: 1,
|
||||||
|
step: 1,
|
||||||
|
message: toolResult('late-tool', 'done'),
|
||||||
|
}, { surfaceOp: 'append' }),
|
||||||
|
at(6, 'step/end', { turn: 1, step: 1 }),
|
||||||
|
at(7, 'turn/end', { turn: 1, reason: { kind: 'completed' } }),
|
||||||
|
])
|
||||||
|
|
||||||
|
const tail = node(snapshot(value), 'turn-tail')?.data as TurnTailChatData
|
||||||
|
expect(tail.closing?.finalNode.seq).toBe(3)
|
||||||
|
expect(tail.branchUnavailable).toBe(true)
|
||||||
|
})
|
||||||
|
|
||||||
it('replays inbox predecessors after prepend and reclassifies the dependent message as steering', () => {
|
it('replays inbox predecessors after prepend and reclassifies the dependent message as steering', () => {
|
||||||
const value = assembler([
|
const value = assembler([
|
||||||
at(3, 'user/message', textMessage('steer-1', 'change direction'), { surfaceOp: 'append' }),
|
at(3, 'user/message', textMessage('steer-1', 'change direction'), { surfaceOp: 'append' }),
|
||||||
|
|||||||
@@ -121,6 +121,8 @@ export const SERVICE_WALK_EXEMPTIONS: Record<string, string> = {
|
|||||||
chatFileMentions: 'client-side slot-contract accessor (ChatFileMentions) — packages/client/ui-conversation/README.md owns the surface',
|
chatFileMentions: 'client-side slot-contract accessor (ChatFileMentions) — packages/client/ui-conversation/README.md owns the surface',
|
||||||
command: 'client-side interface-typed browser service — packages/client/ui-command/README.md owns the surface',
|
command: 'client-side interface-typed browser service — packages/client/ui-command/README.md owns the surface',
|
||||||
conversation: 'client-side interface-typed browser service — packages/client/ui-conversation/README.md owns the surface',
|
conversation: 'client-side interface-typed browser service — packages/client/ui-conversation/README.md owns the surface',
|
||||||
|
conversationEvents: 'client-side interface-typed registry — packages/client/runtime/README.md owns the surface',
|
||||||
|
conversationViews: 'client-side interface-typed registry — packages/client/runtime/README.md owns the surface',
|
||||||
layout: 'client-side interface-typed browser service — packages/client/ui-layout/README.md owns the surface',
|
layout: 'client-side interface-typed browser service — packages/client/ui-layout/README.md owns the surface',
|
||||||
locale: 'client-side interface-typed browser service — packages/client/locale/README.md owns the surface',
|
locale: 'client-side interface-typed browser service — packages/client/locale/README.md owns the surface',
|
||||||
models: 'client-side interface-typed browser service — packages/client/ui-model/README.md owns the surface',
|
models: 'client-side interface-typed browser service — packages/client/ui-model/README.md owns the surface',
|
||||||
|
|||||||
Reference in New Issue
Block a user