Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,7 @@ export class HistoryReplayer {
await this.messageEmitter.emitUserMessage(
displayText,
record.timestamp,
record.subtype === 'cron' ? { source: 'cron' } : undefined,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] Two concerns on this line:

1. Hardcoded 'cron' loses 'loop' on replay. The live path (Session.ts:2812) sends source: 'loop' for loop prompts (where cronExpr === '@wakeup'), but both cron and loop prompts are recorded with subtype: 'cron' by chatRecordingService.ts. On replay, loop-originated messages will incorrectly carry source: 'cron' instead of source: 'loop'. No current UI consumer distinguishes the two, but this contradicts the PR's goal of preserving source metadata end-to-end.

Consider persisting the original source value on the chat record (e.g., in systemPayload) and reading it back during replay:

Suggested change
record.subtype === 'cron' ? { source: 'cron' } : undefined,
record.subtype === 'cron'
? { source: (record.systemPayload as { source?: string } | undefined)?.source ?? 'cron' }
: undefined,

2. No test coverage. The conditional that tags cron-subtype user messages with source metadata during replay is not tested in HistoryReplayer.test.ts. The downstream layers are individually tested, but this integration point is not — a future refactor that drops or inverts the ternary would go undetected. Consider adding test cases for (a) cron-subtype records emitting _meta.source: 'cron' and (b) notification-subtype records emitting no source.

— qwen3.7-max via Qwen Code /review

);
}
break;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,21 @@ describe('MessageEmitter', () => {
content: { type: 'text', text: multilineText },
});
});

it('should include source metadata when provided', async () => {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] This test covers source + timestamp together, but the implementation has two independent conditional spreads inside _meta. The source-only branch (no timestamp) is never exercised.

A future caller passing source without timestamp — a valid parameter combination per the signature — has unverified behavior.

Suggested change
it('should include source metadata when provided', async () => {
it('should include source metadata when provided', async () => {
await emitter.emitUserMessage('scheduled prompt', 1_700_000_000_000, {
source: 'cron',
});
expect(sendUpdateSpy).toHaveBeenCalledWith({
sessionUpdate: 'user_message_chunk',
content: { type: 'text', text: 'scheduled prompt' },
_meta: {
timestamp: 1_700_000_000_000,
source: 'cron',
},
});
});
it('should include source metadata without timestamp', async () => {
await emitter.emitUserMessage('scheduled prompt', undefined, {
source: 'cron',
});
expect(sendUpdateSpy).toHaveBeenCalledWith({
sessionUpdate: 'user_message_chunk',
content: { type: 'text', text: 'scheduled prompt' },
_meta: { source: 'cron' },
});
});

— qwen3.7-max via Qwen Code /review

await emitter.emitUserMessage('scheduled prompt', 1_700_000_000_000, {
source: 'cron',
});

expect(sendUpdateSpy).toHaveBeenCalledWith({
sessionUpdate: 'user_message_chunk',
content: { type: 'text', text: 'scheduled prompt' },
_meta: {
timestamp: 1_700_000_000_000,
source: 'cron',
},
});
});
});

describe('emitAgentMessage', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -90,12 +90,17 @@ export class MessageEmitter extends BaseEmitter {
async emitUserMessage(
text: string,
timestamp?: string | number,
options: { source?: string } = {},
): Promise<void> {
const epochMs = BaseEmitter.toEpochMs(timestamp);
const _meta = {
...(epochMs != null ? { timestamp: epochMs } : {}),
...(options.source ? { source: options.source } : {}),
};
await this.sendUpdate({
sessionUpdate: 'user_message_chunk',
content: { type: 'text', text },
...(epochMs != null && { _meta: { timestamp: epochMs } }),
...(Object.keys(_meta).length > 0 ? { _meta } : {}),
});
}

Expand Down
21 changes: 19 additions & 2 deletions packages/sdk-typescript/src/daemon/ui/normalizer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -531,6 +531,7 @@ function normalizeSessionUpdate(
) {
return [];
}
const meta = extractUpdateMeta(update);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] meta is extracted here before the content-type branch, but only propagated on user.text.delta events. The user.image.delta return (in the image branch above) omits meta entirely. This creates an asymmetric contract — the PR aims for end-to-end metadata preservation, but if a scheduled user message ever carries an image attachment, its source metadata is silently dropped.

No live bug today (cron/loop prompts are text-only), but future image-bearing scheduled messages would lose provenance without warning.

Consider propagating meta on the image path too, or adding a comment noting the intentional omission.

— qwen3.7-max via Qwen Code /review

const content = update['content'];
const part = extractContentPart(content);
if (part) {
Expand All @@ -555,13 +556,29 @@ function normalizeSessionUpdate(
}
if (part.kind === 'text') {
return part.text
? [{ ...base, type: 'user.text.delta', text: part.text }]
? [
{
...base,
type: 'user.text.delta',
text: part.text,
...(meta ? { meta } : {}),
},
]
: [];
}
return [];
}
const text = getTextContent(content);
return text ? [{ ...base, type: 'user.text.delta', text }] : [];
return text
? [
{
...base,
type: 'user.text.delta',
text,
...(meta ? { meta } : {}),
},
]
: [];
}
case 'agent_message_chunk': {
const text = getTextContent(update['content']);
Expand Down
33 changes: 33 additions & 0 deletions packages/sdk-typescript/test/unit/daemonUi.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -522,6 +522,39 @@ describe('daemon UI normalizer and transcript reducer', () => {
).toMatchObject([{ type: 'user.text.delta', text: 'hello' }]);
});

it('preserves user message metadata on transcript blocks', () => {
const events = normalizeDaemonEvent({
id: 23,
v: 1,
type: 'session_update',
data: {
update: {
sessionUpdate: 'user_message_chunk',
content: { type: 'text', text: 'scheduled prompt' },
_meta: { source: 'cron' },
},
},
} as const);

expect(events).toMatchObject([
{
type: 'user.text.delta',
text: 'scheduled prompt',
meta: { source: 'cron' },
},
]);

const state = reduceDaemonTranscriptEvents(
createDaemonTranscriptState({ now: 1 }),
events,
);
expect(state.blocks[0]).toMatchObject({
kind: 'user',
text: 'scheduled prompt',
meta: { source: 'cron' },
});
});

it('carries user shell command metadata into user shell transcript blocks', () => {
let state = createDaemonTranscriptState({ now: 1 });
const commandEvents = normalizeDaemonEvent({
Expand Down
1 change: 1 addition & 0 deletions packages/web-shell/client/adapters/messageTypes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ export interface DaemonUserMessage extends DaemonMessageMeta {
role: 'user';
content: string;
images?: Array<{ data: string; mimeType: string }>;
source?: string;
}

export interface DaemonAssistantMessage extends DaemonMessageMeta {
Expand Down
15 changes: 15 additions & 0 deletions packages/web-shell/client/adapters/transcriptToMessages.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,21 @@ function toolBlock(
}

describe('transcriptBlocksToDaemonMessages', () => {
it('preserves user source metadata', () => {
const messages = transcriptBlocksToDaemonMessages([
textBlock('user-1', 'user', 'scheduled prompt', 1, false, {
meta: { source: 'cron' },
}),
]);

expect(messages[0]).toMatchObject({
id: 'user-1',
role: 'user',
content: 'scheduled prompt',
source: 'cron',
});
});

it('hides background task notifications by metadata', () => {
const messages = transcriptBlocksToDaemonMessages([
textBlock(
Expand Down
5 changes: 5 additions & 0 deletions packages/web-shell/client/adapters/transcriptToMessages.ts
Original file line number Diff line number Diff line change
Expand Up @@ -195,11 +195,16 @@ export function transcriptBlocksToDaemonMessages(
currentThinkingIdx = null;
needsNewContentMessage = false;
const textBlock = block as DaemonTextTranscriptBlock;
const meta = getRecord(
(textBlock as ExtendedDaemonTextTranscriptBlock).meta,
);
const source = getString(meta, 'source');
const msg: DaemonUserMessage = {
id: block.id,
role: 'user',
content: textBlock.text,
timestamp: blockTime,
...(source ? { source } : {}),
};
// Attach images if present
if (textBlock.images && textBlock.images.length > 0) {
Expand Down
Loading