feat(btw): fork at the last completed turn, and never leak the inherited tail
Two related fixes to what a btw session inherits and shows. **Fork point.** `/btw` is typically typed *while* the main thread is working — that is the moment a side question comes up. Forking at HEAD then clones a turn that is still streaming, so the fork inherits a truncated assistant message and the user instruction that provoked it as the newest, most salient thing in its context. Fork at the parent's last completed assistant turn instead, read from the sync store with no extra round-trip. A parent with no completed turn yet keeps the previous fork-at-HEAD behavior. **Boundary.** `filterBtwTailMessages` shows every inherited message when the boundary is `null`, and the boundary is `null` whenever the newest-cloned read comes back empty. That is correct for a fork of an empty parent, but it also turns an empty read into "nothing was inherited" for a fork that demonstrably did inherit history — the panel then opens with the parent's whole transcript in it. Having picked a fork point proves the parent had turns, so fall back to that id instead of `null`. The fork's own messages are created later and still sort after it, so its tail stays complete either way.
This commit is contained in:
@@ -16,6 +16,7 @@ const upsertedSessions: unknown[] = [];
|
||||
const childStoreSessions: Session[] = [];
|
||||
const currentSessionSwitches: string[] = [];
|
||||
const metadataPatches: Array<{ sessionId: string; result: Record<string, unknown> }> = [];
|
||||
const parentSyncMessages: Message[] = [];
|
||||
|
||||
mock.module('@/lib/opencode/client', () => ({
|
||||
opencodeClient: {
|
||||
@@ -48,6 +49,7 @@ mock.module('@/stores/useGlobalSessionsStore', () => ({
|
||||
}));
|
||||
mock.module('@/sync/sync-refs', () => ({
|
||||
registerSessionDirectory: (sessionId: string, directory: string) => { registeredDirectories.push(`${sessionId}:${directory}`); },
|
||||
getSyncMessages: () => parentSyncMessages,
|
||||
getSyncChildStores: () => ({
|
||||
children: new Map([['/project', {
|
||||
getState: () => ({ session: childStoreSessions }),
|
||||
@@ -56,7 +58,7 @@ mock.module('@/sync/sync-refs', () => ({
|
||||
}),
|
||||
}));
|
||||
|
||||
const { btwSessionTitle, startBtwSession, destroyBtwSession, promoteBtwSession, filterBtwTailMessages, BTW_BOUNDARY_INSTRUCTION } =
|
||||
const { btwSessionTitle, startBtwSession, destroyBtwSession, promoteBtwSession, filterBtwTailMessages, findLastCompletedAssistantMessageID, BTW_BOUNDARY_INSTRUCTION } =
|
||||
await import('@/lib/btw');
|
||||
const { useBtwStore } = await import('@/stores/useBtwStore');
|
||||
|
||||
@@ -74,6 +76,15 @@ const record = (id: string): { info: Message; parts: Part[] } => ({
|
||||
parts: [],
|
||||
});
|
||||
|
||||
// SAFETY: `findLastCompletedAssistantMessageID` reads only `id`, `role` and
|
||||
// `time`, which are the fields spelled out here.
|
||||
const assistantMessage = (id: string, completed?: number) =>
|
||||
({ id, sessionID: 'parent-1', role: 'assistant', time: { created: 1, completed } }) as Message;
|
||||
|
||||
// SAFETY: same narrow read as `assistantMessage`.
|
||||
const userMessage = (id: string) =>
|
||||
({ id, sessionID: 'parent-1', role: 'user', time: { created: 1 } }) as Message;
|
||||
|
||||
const startInput = {
|
||||
parentSessionId: 'parent-1',
|
||||
question: 'wtf is kafka',
|
||||
@@ -90,6 +101,7 @@ beforeEach(() => {
|
||||
childStoreSessions.length = 0;
|
||||
currentSessionSwitches.length = 0;
|
||||
metadataPatches.length = 0;
|
||||
parentSyncMessages.length = 0;
|
||||
useBtwStore.setState({ byParent: {} });
|
||||
forkSessionImpl = () => Promise.reject(new Error('no forkSession stub'));
|
||||
getSessionMessagesImpl = () => Promise.resolve([record('msg-boundary')]);
|
||||
@@ -121,6 +133,17 @@ describe('filterBtwTailMessages', () => {
|
||||
});
|
||||
});
|
||||
|
||||
describe('findLastCompletedAssistantMessageID', () => {
|
||||
test('skips an assistant turn that is still streaming', () => {
|
||||
const messages = [assistantMessage('msg-1', 10), userMessage('msg-2'), assistantMessage('msg-3')];
|
||||
expect(findLastCompletedAssistantMessageID(messages)).toBe('msg-1');
|
||||
});
|
||||
|
||||
test('a session with no completed assistant turn has no fork point', () => {
|
||||
expect(findLastCompletedAssistantMessageID([userMessage('msg-1')])).toBe(null);
|
||||
});
|
||||
});
|
||||
|
||||
describe('startBtwSession', () => {
|
||||
test('forks, marks the fork, links the parent, and routes the question to the fork', async () => {
|
||||
forkSessionImpl = (sessionId, messageId, directory) => {
|
||||
@@ -151,6 +174,32 @@ describe('startBtwSession', () => {
|
||||
expect(useBtwStore.getState().byParent).toEqual({});
|
||||
});
|
||||
|
||||
test('forks at the last completed assistant turn, not at the in-flight one', async () => {
|
||||
parentSyncMessages.push(assistantMessage('msg-1', 10), userMessage('msg-2'), assistantMessage('msg-3'));
|
||||
const forkPoints: Array<string | undefined> = [];
|
||||
forkSessionImpl = (_sessionId, messageId) => {
|
||||
forkPoints.push(messageId);
|
||||
return Promise.resolve(makeSession('fork-1', '/project'));
|
||||
};
|
||||
|
||||
await startBtwSession(startInput);
|
||||
|
||||
expect(forkPoints).toEqual(['msg-1']);
|
||||
});
|
||||
|
||||
test('the boundary falls back to the fork point when the cloned tail reads empty', async () => {
|
||||
parentSyncMessages.push(assistantMessage('msg-1', 10));
|
||||
forkSessionImpl = () => Promise.resolve(makeSession('fork-1', '/project'));
|
||||
getSessionMessagesImpl = () => Promise.resolve([]);
|
||||
|
||||
await startBtwSession(startInput);
|
||||
|
||||
// Not `null`: a null boundary would show the whole inherited transcript.
|
||||
expect(metadataPatches[0]?.result).toEqual({
|
||||
openchamber: { kind: 'btw', originalSessionID: 'parent-1', btwBoundaryMessageID: 'msg-1' },
|
||||
});
|
||||
});
|
||||
|
||||
test('the first question carries the boundary instruction as a synthetic part', async () => {
|
||||
forkSessionImpl = () => Promise.resolve(makeSession('fork-1', '/project'));
|
||||
const sentParts: unknown[] = [];
|
||||
|
||||
@@ -4,7 +4,7 @@ import * as sessionActions from '@/sync/session-actions';
|
||||
import { withBtwSessionLink, withBtwSessionMarker, withoutBtwSessionLink, withoutBtwSessionMarker } from '@/lib/sessionBtwMetadata';
|
||||
import { useBtwStore } from '@/stores/useBtwStore';
|
||||
import { useSessionUIStore } from '@/sync/session-ui-store';
|
||||
import { getSyncChildStores, registerSessionDirectory } from '@/sync/sync-refs';
|
||||
import { getSyncChildStores, getSyncMessages, registerSessionDirectory } from '@/sync/sync-refs';
|
||||
import { Binary } from '@/sync/binary';
|
||||
|
||||
/**
|
||||
@@ -56,9 +56,31 @@ export const BTW_BOUNDARY_INSTRUCTION = [
|
||||
].join('\n');
|
||||
|
||||
/** The boundary as an `additionalParts` entry for `sendMessage`. */
|
||||
export const btwBoundaryParts = (): Array<{ text: string; synthetic: true }> =>
|
||||
const btwBoundaryParts = (): Array<{ text: string; synthetic: true }> =>
|
||||
[{ text: BTW_BOUNDARY_INSTRUCTION, synthetic: true }];
|
||||
|
||||
/**
|
||||
* The parent's last assistant turn that actually finished.
|
||||
*
|
||||
* `/btw` is typically typed *while* the main thread is working — that is the
|
||||
* moment a side question comes up. Forking at HEAD then clones a turn that is
|
||||
* still streaming: the fork inherits a truncated assistant message and the
|
||||
* user instruction that provoked it as the newest, most salient thing in its
|
||||
* context. Anchoring the fork to the last completed turn instead means the
|
||||
* inherited transcript is always a settled conversation.
|
||||
*
|
||||
* Returns `null` when the parent has no completed assistant turn yet (a brand
|
||||
* new session); the caller then keeps the previous fork-at-HEAD behavior.
|
||||
*/
|
||||
export const findLastCompletedAssistantMessageID = (messages: readonly Message[]): string | null => {
|
||||
for (let index = messages.length - 1; index >= 0; index -= 1) {
|
||||
const message = messages[index];
|
||||
if (message?.role !== 'assistant') continue;
|
||||
if (message.time.completed !== undefined) return message.id;
|
||||
}
|
||||
return null;
|
||||
};
|
||||
|
||||
export const btwSessionTitle = (question: string): string => `btw: ${question}`;
|
||||
|
||||
/**
|
||||
@@ -82,7 +104,16 @@ export async function startBtwSession(input: StartBtwInput): Promise<Session> {
|
||||
setPanelState(input.parentSessionId, { creating: true });
|
||||
try {
|
||||
await sessionActions.waitForConnectionOrThrow();
|
||||
const forked = await opencodeClient.forkSession(input.parentSessionId, undefined, input.directory);
|
||||
// Fork at the parent's last completed assistant turn rather than at HEAD,
|
||||
// so a `/btw` typed mid-turn does not inherit a half-finished one.
|
||||
const forkPointMessageID = findLastCompletedAssistantMessageID(
|
||||
getSyncMessages(input.parentSessionId, input.directory),
|
||||
);
|
||||
const forked = await opencodeClient.forkSession(
|
||||
input.parentSessionId,
|
||||
forkPointMessageID ?? undefined,
|
||||
input.directory,
|
||||
);
|
||||
|
||||
// The server may canonicalize the worktree path; the prompt must use the
|
||||
// same directory identity as the forked session.
|
||||
@@ -96,7 +127,14 @@ export async function startBtwSession(input: StartBtwInput): Promise<Session> {
|
||||
// id of the newest cloned message. Message ids are server-generated and
|
||||
// ascending, so everything the fork produces sorts after it.
|
||||
const newestCloned = await opencodeClient.getSessionMessages(forked.id, 1, sessionDirectory);
|
||||
const boundaryMessageID = newestCloned[newestCloned.length - 1]?.info.id ?? null;
|
||||
// A `null` boundary makes the panel show every inherited message, so an
|
||||
// empty read must not be taken as "the fork inherited nothing" when we
|
||||
// know it did: having picked a fork point proves the parent had turns.
|
||||
// Fall back to that id — the fork's own messages are created later and
|
||||
// still sort after it, so the tail stays complete either way.
|
||||
const boundaryMessageID = newestCloned[newestCloned.length - 1]?.info.id
|
||||
?? forkPointMessageID
|
||||
?? null;
|
||||
|
||||
// The fork inherits the parent's metadata and title wholesale: replace
|
||||
// the metadata with the btw marker, and rename it (rename is
|
||||
|
||||
Reference in New Issue
Block a user