refactor(review): address PR #186 review (#183 — recency sweep, #174 export, tests, cleanups)

15-point review of the persistent-history PR. Architecture decisions: crash
recovery = recency threshold; tool-label duplication = leave as-is.

Must-fix:
1. Boot-sweep bounded by recency. sweepStreaming now also requires
   `updatedAt < now() - SWEEP_STREAMING_STALE_MS` (10 min), so a fresh replica's
   startup sweep can't abort a turn another replica is actively streaming
   (multi-instance deploy). Int-spec: a FRESH 'streaming' row is NOT swept, a
   STALE one IS.
2. Restore export during the FIRST streaming turn of a new chat (#174). The
   server chatId is now adopted EARLY (in-place, on the start-chunk metadata) via
   a new `onServerChatId` callback wired through use-chat-session → chat-thread,
   so `activeChatId` is set at turn start and the Copy button is live mid-first-
   turn (canExport = !!activeChatId). Hook tests for early/in-place/no-op adopt.
3. Cover finalizeAssistant's fallback-insert branch: extracted pure
   `planFinalizeAssistant(assistantId)` (update when id present, insert when the
   upfront insert failed) + a dispatch harness test for both arms.

Tests: onModuleInit lifecycle spec (sweep called; throw → resolves + warns);
int-spec updatedAt assertion → toBeGreaterThan.

Cleanups: cap findAllByChat at 5000 rows; upfront-insert-failure log carries
chatId+workspaceId; removed the now-dead buildPartialAssistantRecord (only the
spec consumed it; shapes still pinned by the flushAssistant suite); controller
passes `lang: dto.lang` (normalizeLang handles undefined); dropped a no-op
`?? undefined` in errorOf; documented the content-column semantics change
(concatenated step text, UI renders from metadata.parts); CHANGELOG [Unreleased]
entry (#183, #174); reworded the stale LABELS parity comment.

Verified: server build + 323 ai-chat unit + 5 integration; client tsc + 160
ai-chat unit; prettier clean.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
claude code agent 227
2026-06-25 11:53:25 +03:00
parent ae6faf3abc
commit ea61c96a7c
13 changed files with 408 additions and 165 deletions

View File

@@ -68,7 +68,8 @@ describe('AiChatMessageRepo.update + sweepStreaming [integration]', () => {
expect(updated!.content).toBe('final answer');
expect(updated!.status).toBe('completed');
expect((updated!.metadata as any).parts).toHaveLength(1);
expect(new Date(updated!.updatedAt).getTime()).toBeGreaterThanOrEqual(
// The 5ms sleep above guarantees a strictly-later timestamp.
expect(new Date(updated!.updatedAt).getTime()).toBeGreaterThan(
new Date(before).getTime(),
);
});
@@ -128,8 +129,23 @@ describe('AiChatMessageRepo.update + sweepStreaming [integration]', () => {
await repo.update(seeded.id, workspaceId, { status: 'completed' });
});
it('sweepStreaming flips dangling streaming rows to aborted and counts them', async () => {
// Two dangling streaming rows in our workspace + one in another workspace.
// Backdate a row's updatedAt so it qualifies as a STALE streaming row (the
// sweep only flips rows untouched for >10 minutes — a live turn bumps
// updatedAt every step, so it would never match).
async function backdateUpdatedAt(
id: string,
minutesAgo: number,
): Promise<void> {
await db
.updateTable('aiChatMessages')
.set({ updatedAt: new Date(Date.now() - minutesAgo * 60 * 1000) })
.where('id', '=', id)
.execute();
}
it('sweepStreaming flips STALE dangling streaming rows to aborted and counts them', async () => {
// Two dangling streaming rows in our workspace + one in another workspace —
// all backdated past the staleness threshold so the sweep picks them up.
const a = await createMessage(db, {
workspaceId,
chatId,
@@ -142,6 +158,16 @@ describe('AiChatMessageRepo.update + sweepStreaming [integration]', () => {
role: 'assistant',
status: 'streaming',
});
const other = await createMessage(db, {
workspaceId: otherWorkspaceId,
chatId: otherChatId,
role: 'assistant',
status: 'streaming',
});
await backdateUpdatedAt(a.id, 20);
await backdateUpdatedAt(b.id, 20);
await backdateUpdatedAt(other.id, 20);
// A settled row must NOT be touched.
const done = await createMessage(db, {
workspaceId,
@@ -156,15 +182,9 @@ describe('AiChatMessageRepo.update + sweepStreaming [integration]', () => {
role: 'assistant',
status: null,
});
await createMessage(db, {
workspaceId: otherWorkspaceId,
chatId: otherChatId,
role: 'assistant',
status: 'streaming',
});
const swept = await repo.sweepStreaming();
// At least the 3 streaming rows we created (2 here + 1 in the other ws).
// At least the 3 stale streaming rows we created (2 here + 1 in the other ws).
expect(swept).toBeGreaterThanOrEqual(3);
const rows = await repo.findAllByChat(chatId, workspaceId);
@@ -181,4 +201,34 @@ describe('AiChatMessageRepo.update + sweepStreaming [integration]', () => {
expect(rows2.find((r) => r.id === a.id)!.status).toBe('aborted');
expect(again).toBeGreaterThanOrEqual(0);
});
it('sweepStreaming does NOT sweep a FRESH streaming row (recency bound, #183 review)', async () => {
// A row that is actively streaming (recent updatedAt) must survive the sweep:
// a fresh replica's boot-sweep must never abort a turn another replica is
// still streaming in a multi-instance deploy.
const fresh = await createMessage(db, {
workspaceId,
chatId,
role: 'assistant',
status: 'streaming',
});
// A STALE streaming row created alongside it IS swept — proving the sweep
// ran and the only difference is recency.
const stale = await createMessage(db, {
workspaceId,
chatId,
role: 'assistant',
status: 'streaming',
});
await backdateUpdatedAt(stale.id, 20);
await repo.sweepStreaming();
const rows = await repo.findAllByChat(chatId, workspaceId);
const byId = new Map(rows.map((r) => [r.id, r]));
// Fresh (recently-updated) streaming row is left untouched...
expect(byId.get(fresh.id)!.status).toBe('streaming');
// ...while the stale one alongside it was swept to 'aborted'.
expect(byId.get(stale.id)!.status).toBe('aborted');
});
});