fix(web): close P3 chat re-review findings
This commit is contained in:
shaggy (mosaic-dev box)
@@ -1,9 +1,10 @@
|
||||
import { act } from 'react';
|
||||
import { createRoot, type Root } from 'react-dom/client';
|
||||
import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, it, vi } from 'vitest';
|
||||
import { createFakeChatSocket } from './test-support/fake-chat-socket';
|
||||
import { createFakeChatSocket, type EmittedEvent } from './test-support/fake-chat-socket';
|
||||
import {
|
||||
MAX_COMMAND_RESULTS,
|
||||
MAX_EXECUTED_APPROVAL_IDS,
|
||||
MAX_MANIFEST_ITEMS,
|
||||
MAX_MESSAGES,
|
||||
MAX_STREAM_CHARS,
|
||||
@@ -430,6 +431,14 @@ describe('useChatConnection', () => {
|
||||
expect(latest?.state.conversationId).toBe('c1');
|
||||
expect(latest?.state.error).toBe('Failed to start agent session. Please try again.');
|
||||
expect(latest?.state.streaming).toBe(false);
|
||||
expect(latest?.state.sending).toBe(false);
|
||||
|
||||
await act(async () => {
|
||||
latest?.actions.sendMessage({ content: 'retry' });
|
||||
});
|
||||
|
||||
expect(fake.emitted.filter((e) => e.event === 'message')).toHaveLength(2);
|
||||
expect(latest?.state.messages.some((m) => m.text === 'retry')).toBe(true);
|
||||
});
|
||||
|
||||
it('stops streaming when a typed error arrives mid-turn', async () => {
|
||||
@@ -881,6 +890,10 @@ describe('useChatConnection', () => {
|
||||
|
||||
expect(latest?.state.manifest?.commands).toEqual([]);
|
||||
expect(latest?.state.manifest?.skills).toEqual([]);
|
||||
// The raw stored reload must be normalized the same way as the manifest —
|
||||
// never left holding the malformed raw value.
|
||||
expect(latest?.state.systemReload?.commands).toEqual([]);
|
||||
expect(latest?.state.systemReload?.skills).toEqual([]);
|
||||
});
|
||||
|
||||
it('falls back to a safe tool label when agent:tool:start carries a non-string toolName', async () => {
|
||||
@@ -932,19 +945,74 @@ describe('useChatConnection', () => {
|
||||
expect(latest?.state.pendingApproval).toEqual({ command: 'deploy', args: 'prod' });
|
||||
});
|
||||
|
||||
it('caps streamed agent:text at MAX_STREAM_CHARS, keeping only the most recent characters', async () => {
|
||||
it('caps streamed agent:text at MAX_STREAM_CHARS behind a visible marker disclosing the honest dropped-character count across multiple appends', async () => {
|
||||
await act(async () => {
|
||||
fake.serverEmit('message:ack', { conversationId: 'c1', messageId: 'm1' });
|
||||
fake.serverEmit('agent:start', { conversationId: 'c1' });
|
||||
});
|
||||
|
||||
const firstChunk = 'a'.repeat(MAX_STREAM_CHARS);
|
||||
const secondChunk = 'b'.repeat(500);
|
||||
const thirdChunk = 'c'.repeat(300);
|
||||
|
||||
await act(async () => {
|
||||
fake.serverEmit('agent:text', { conversationId: 'c1', text: 'a'.repeat(MAX_STREAM_CHARS) });
|
||||
fake.serverEmit('agent:text', { conversationId: 'c1', text: 'b'.repeat(10) });
|
||||
fake.serverEmit('agent:text', { conversationId: 'c1', text: firstChunk });
|
||||
});
|
||||
await act(async () => {
|
||||
fake.serverEmit('agent:text', { conversationId: 'c1', text: secondChunk });
|
||||
});
|
||||
await act(async () => {
|
||||
fake.serverEmit('agent:text', { conversationId: 'c1', text: thirdChunk });
|
||||
});
|
||||
|
||||
expect(latest?.state.text).toHaveLength(MAX_STREAM_CHARS);
|
||||
expect(latest?.state.text.endsWith('b'.repeat(10))).toBe(true);
|
||||
const text = latest?.state.text ?? '';
|
||||
expect(text.length).toBeLessThanOrEqual(MAX_STREAM_CHARS);
|
||||
|
||||
const match = text.match(/^…\[truncated (\d+) characters\]…/);
|
||||
expect(match).not.toBeNull();
|
||||
const markerLength = match?.[0].length ?? 0;
|
||||
const droppedCount = Number(match?.[1]);
|
||||
const tail = text.slice(markerLength);
|
||||
const totalStreamed = firstChunk.length + secondChunk.length + thirdChunk.length;
|
||||
|
||||
// The marker's N must be the actual count of original characters no
|
||||
// longer visible — not an estimate — across all three appends.
|
||||
expect(droppedCount).toBe(totalStreamed - tail.length);
|
||||
expect(tail.endsWith(thirdChunk)).toBe(true);
|
||||
});
|
||||
|
||||
it('caps streamed agent:thinking at MAX_STREAM_CHARS behind the same honest truncation marker as agent:text', async () => {
|
||||
await act(async () => {
|
||||
fake.serverEmit('message:ack', { conversationId: 'c1', messageId: 'm1' });
|
||||
fake.serverEmit('agent:start', { conversationId: 'c1' });
|
||||
});
|
||||
|
||||
const firstChunk = 'x'.repeat(MAX_STREAM_CHARS);
|
||||
const secondChunk = 'y'.repeat(500);
|
||||
const thirdChunk = 'z'.repeat(300);
|
||||
|
||||
await act(async () => {
|
||||
fake.serverEmit('agent:thinking', { conversationId: 'c1', text: firstChunk });
|
||||
});
|
||||
await act(async () => {
|
||||
fake.serverEmit('agent:thinking', { conversationId: 'c1', text: secondChunk });
|
||||
});
|
||||
await act(async () => {
|
||||
fake.serverEmit('agent:thinking', { conversationId: 'c1', text: thirdChunk });
|
||||
});
|
||||
|
||||
const thinking = latest?.state.thinking ?? '';
|
||||
expect(thinking.length).toBeLessThanOrEqual(MAX_STREAM_CHARS);
|
||||
|
||||
const match = thinking.match(/^…\[truncated (\d+) characters\]…/);
|
||||
expect(match).not.toBeNull();
|
||||
const markerLength = match?.[0].length ?? 0;
|
||||
const droppedCount = Number(match?.[1]);
|
||||
const tail = thinking.slice(markerLength);
|
||||
const totalStreamed = firstChunk.length + secondChunk.length + thirdChunk.length;
|
||||
|
||||
expect(droppedCount).toBe(totalStreamed - tail.length);
|
||||
expect(tail.endsWith(thirdChunk)).toBe(true);
|
||||
});
|
||||
|
||||
it('caps commandResults at MAX_COMMAND_RESULTS when flooded with command:result events', async () => {
|
||||
@@ -983,7 +1051,7 @@ describe('useChatConnection', () => {
|
||||
expect(latest?.state.manifest?.commands).toHaveLength(MAX_MANIFEST_ITEMS);
|
||||
});
|
||||
|
||||
it('caps a system:reload manifest replacement at MAX_MANIFEST_ITEMS', async () => {
|
||||
it('caps a system:reload manifest replacement at MAX_MANIFEST_ITEMS for both the raw stored reload and the manifest', async () => {
|
||||
const commands = Array.from({ length: MAX_MANIFEST_ITEMS + 5 }, (_, i) => ({
|
||||
name: `cmd${i}`,
|
||||
aliases: [],
|
||||
@@ -992,17 +1060,48 @@ describe('useChatConnection', () => {
|
||||
execution: 'socket' as const,
|
||||
available: true,
|
||||
}));
|
||||
const skills = Array.from({ length: MAX_MANIFEST_ITEMS + 5 }, (_, i) => ({
|
||||
name: `skill${i}`,
|
||||
description: '',
|
||||
available: true,
|
||||
}));
|
||||
|
||||
await act(async () => {
|
||||
fake.serverEmit('system:reload', {
|
||||
commands,
|
||||
skills: [],
|
||||
skills,
|
||||
providers: ['anthropic'],
|
||||
message: 'reloaded',
|
||||
});
|
||||
});
|
||||
|
||||
expect(latest?.state.manifest?.commands).toHaveLength(MAX_MANIFEST_ITEMS);
|
||||
expect(latest?.state.manifest?.skills).toHaveLength(MAX_MANIFEST_ITEMS);
|
||||
// The raw stored reload (state.systemReload) must be bounded the same
|
||||
// way as the manifest derived from it — a hostile/oversized broadcast
|
||||
// must never leave the uncapped raw payload sitting in state.
|
||||
expect(latest?.state.systemReload?.commands).toHaveLength(MAX_MANIFEST_ITEMS);
|
||||
expect(latest?.state.systemReload?.skills).toHaveLength(MAX_MANIFEST_ITEMS);
|
||||
});
|
||||
|
||||
it('caps availableThinkingLevels before storing a hostile session:info payload', async () => {
|
||||
await act(async () => {
|
||||
fake.serverEmit('message:ack', { conversationId: 'c1', messageId: 'm1' });
|
||||
});
|
||||
|
||||
const hostileLevels = Array.from({ length: MAX_MANIFEST_ITEMS + 50 }, (_, i) => `level-${i}`);
|
||||
|
||||
await act(async () => {
|
||||
fake.serverEmit('session:info', {
|
||||
conversationId: 'c1',
|
||||
provider: 'anthropic',
|
||||
modelId: 'claude',
|
||||
thinkingLevel: 'level-0',
|
||||
availableThinkingLevels: hostileLevels,
|
||||
});
|
||||
});
|
||||
|
||||
expect(latest?.state.sessionInfo?.availableThinkingLevels).toHaveLength(MAX_MANIFEST_ITEMS);
|
||||
});
|
||||
|
||||
it('ignores a malformed (object) conversationId on the establishing message:ack, leaving the turn recoverable for a later valid ack', async () => {
|
||||
@@ -1245,6 +1344,234 @@ describe('useChatConnection', () => {
|
||||
expect(new Set(ids).size).toBe(ids.length);
|
||||
});
|
||||
|
||||
it('fails closed after the consumed approval cache saturates and rejects replay of the ID the old eviction policy forgot', async () => {
|
||||
await act(async () => {
|
||||
fake.serverEmit('message:ack', { conversationId: 'c1', messageId: 'm1' });
|
||||
});
|
||||
|
||||
// Fill the bounded dedup set to capacity with genuinely distinct,
|
||||
// legitimately approved and executed IDs. Each step of approve/respond/
|
||||
// run gets its own `act()` so a render (and the ref-syncing effects)
|
||||
// lands between them — combining approve+respond in one act would let
|
||||
// `approvalRequestPending` flip true-then-false within a single commit,
|
||||
// skipping the render the lock-sync effect depends on.
|
||||
for (let i = 0; i < MAX_EXECUTED_APPROVAL_IDS; i += 1) {
|
||||
await act(async () => {
|
||||
latest?.actions.approveCommand({ command: 'deploy' });
|
||||
});
|
||||
await act(async () => {
|
||||
fake.serverEmit('command:approval', {
|
||||
conversationId: 'c1',
|
||||
command: 'deploy',
|
||||
success: true,
|
||||
approvalId: `ap-${i}`,
|
||||
});
|
||||
});
|
||||
await act(async () => {
|
||||
latest?.actions.runApprovedCommand();
|
||||
});
|
||||
}
|
||||
|
||||
// One more distinct approval once the set is already full. The old
|
||||
// eviction policy would delete the oldest entry (ap-0) to make room and
|
||||
// let this execute; the fixed behavior must deny it instead.
|
||||
await act(async () => {
|
||||
latest?.actions.approveCommand({ command: 'deploy' });
|
||||
});
|
||||
await act(async () => {
|
||||
fake.serverEmit('command:approval', {
|
||||
conversationId: 'c1',
|
||||
command: 'deploy',
|
||||
success: true,
|
||||
approvalId: 'ap-overflow',
|
||||
});
|
||||
});
|
||||
await act(async () => {
|
||||
latest?.actions.runApprovedCommand();
|
||||
});
|
||||
|
||||
// A fresh local approval request, replaying the very first approvalId —
|
||||
// this is the replay the old eviction policy would have let through a
|
||||
// second time because it had forgotten ap-0 ever ran.
|
||||
await act(async () => {
|
||||
latest?.actions.approveCommand({ command: 'deploy' });
|
||||
});
|
||||
await act(async () => {
|
||||
fake.serverEmit('command:approval', {
|
||||
conversationId: 'c1',
|
||||
command: 'deploy',
|
||||
success: true,
|
||||
approvalId: 'ap-0',
|
||||
});
|
||||
});
|
||||
await act(async () => {
|
||||
latest?.actions.runApprovedCommand();
|
||||
});
|
||||
|
||||
const executes = fake.emitted.filter(
|
||||
(e): e is EmittedEvent<'command:execute'> => e.event === 'command:execute',
|
||||
);
|
||||
// Only the original MAX_EXECUTED_APPROVAL_IDS executions ever happened —
|
||||
// neither the overflow ID nor the replay of ap-0 produced a new one.
|
||||
expect(executes).toHaveLength(MAX_EXECUTED_APPROVAL_IDS);
|
||||
expect(executes.filter((e) => e.payload.approvalId === 'ap-0')).toHaveLength(1);
|
||||
expect(executes.some((e) => e.payload.approvalId === 'ap-overflow')).toBe(false);
|
||||
});
|
||||
|
||||
it('does not unlock turn B when a stale same-conversation agent:end from turn A arrives', async () => {
|
||||
// Turn A completes normally on c1.
|
||||
await act(async () => {
|
||||
fake.serverEmit('message:ack', { conversationId: 'c1', messageId: 'm1' });
|
||||
fake.serverEmit('agent:start', { conversationId: 'c1' });
|
||||
fake.serverEmit('agent:end', { conversationId: 'c1' });
|
||||
});
|
||||
expect(latest?.state.sending).toBe(false);
|
||||
|
||||
// Turn B is sent (same conversation — the wire gives it no distinct
|
||||
// turn id, only the same conversationId as turn A).
|
||||
await act(async () => {
|
||||
latest?.actions.sendMessage({ content: 'turn B' });
|
||||
});
|
||||
expect(latest?.state.sending).toBe(true);
|
||||
|
||||
// Turn A's agent:end arrives late (network reordering) for the SAME
|
||||
// conversation, before turn B's own ack/start ever arrived. A
|
||||
// conversationId-only check cannot tell this apart from turn B's own
|
||||
// terminal event, so this must not unlock — turn B has not yet been
|
||||
// armed by its own accepted ack/start.
|
||||
await act(async () => {
|
||||
fake.serverEmit('agent:end', { conversationId: 'c1' });
|
||||
});
|
||||
expect(latest?.state.sending).toBe(true);
|
||||
// A recognized-stale terminal must be a true no-op — it must not touch
|
||||
// streaming/error, which legitimately belong to the still in-flight
|
||||
// turn B.
|
||||
expect(latest?.state.streaming).toBe(false);
|
||||
expect(latest?.state.error).toBeNull();
|
||||
|
||||
// An attempted turn C must still be rejected — the lock is still held.
|
||||
await act(async () => {
|
||||
latest?.actions.sendMessage({ content: 'turn C attempt' });
|
||||
});
|
||||
expect(fake.emitted.filter((e) => e.event === 'message')).toHaveLength(1);
|
||||
expect(latest?.state.messages.some((m) => m.text === 'turn C attempt')).toBe(false);
|
||||
|
||||
// Turn B's own, current ack/start arms the lock, and its own terminal
|
||||
// event can then legitimately release it, allowing a later send.
|
||||
await act(async () => {
|
||||
fake.serverEmit('message:ack', { conversationId: 'c1', messageId: 'm2' });
|
||||
fake.serverEmit('agent:start', { conversationId: 'c1' });
|
||||
});
|
||||
await act(async () => {
|
||||
fake.serverEmit('agent:end', { conversationId: 'c1' });
|
||||
});
|
||||
expect(latest?.state.sending).toBe(false);
|
||||
|
||||
await act(async () => {
|
||||
latest?.actions.sendMessage({ content: 'turn D' });
|
||||
});
|
||||
expect(fake.emitted.filter((e) => e.event === 'message')).toHaveLength(2);
|
||||
});
|
||||
|
||||
it('does not unlock turn B when a stale same-conversation error from turn A arrives', async () => {
|
||||
await act(async () => {
|
||||
fake.serverEmit('message:ack', { conversationId: 'c1', messageId: 'm1' });
|
||||
fake.serverEmit('agent:start', { conversationId: 'c1' });
|
||||
fake.serverEmit('agent:end', { conversationId: 'c1' });
|
||||
});
|
||||
expect(latest?.state.sending).toBe(false);
|
||||
|
||||
await act(async () => {
|
||||
latest?.actions.sendMessage({ content: 'turn B' });
|
||||
});
|
||||
expect(latest?.state.sending).toBe(true);
|
||||
|
||||
// A stale error for turn A, same conversationId, arrives before turn
|
||||
// B's own ack/start — must not unlock turn B.
|
||||
await act(async () => {
|
||||
fake.serverEmit('error', { conversationId: 'c1', error: 'stale turn A failure' });
|
||||
});
|
||||
expect(latest?.state.sending).toBe(true);
|
||||
// A recognized-stale terminal must be a true no-op — its message must
|
||||
// never be displayed/stored, and it must not touch streaming, which
|
||||
// legitimately belongs to the still in-flight turn B.
|
||||
expect(latest?.state.error).toBeNull();
|
||||
expect(latest?.state.streaming).toBe(false);
|
||||
|
||||
await act(async () => {
|
||||
latest?.actions.sendMessage({ content: 'turn C attempt' });
|
||||
});
|
||||
expect(fake.emitted.filter((e) => e.event === 'message')).toHaveLength(1);
|
||||
expect(latest?.state.messages.some((m) => m.text === 'turn C attempt')).toBe(false);
|
||||
|
||||
// Turn B's own current ack/start arms the lock; its own terminal event
|
||||
// (here, its own error) can then legitimately release it.
|
||||
await act(async () => {
|
||||
fake.serverEmit('message:ack', { conversationId: 'c1', messageId: 'm2' });
|
||||
fake.serverEmit('agent:start', { conversationId: 'c1' });
|
||||
});
|
||||
await act(async () => {
|
||||
fake.serverEmit('error', { conversationId: 'c1', error: 'turn B failed' });
|
||||
});
|
||||
expect(latest?.state.sending).toBe(false);
|
||||
|
||||
await act(async () => {
|
||||
latest?.actions.sendMessage({ content: 'turn D' });
|
||||
});
|
||||
expect(fake.emitted.filter((e) => e.event === 'message')).toHaveLength(2);
|
||||
});
|
||||
|
||||
it('a recognized-stale same-conversation error during an unarmed turn B does not clear approvalRequestPending, does not overwrite the frozen pendingApproval, and does not surface its message', async () => {
|
||||
// Turn A completes normally on c1.
|
||||
await act(async () => {
|
||||
fake.serverEmit('message:ack', { conversationId: 'c1', messageId: 'm1' });
|
||||
fake.serverEmit('agent:start', { conversationId: 'c1' });
|
||||
fake.serverEmit('agent:end', { conversationId: 'c1' });
|
||||
});
|
||||
expect(latest?.state.sending).toBe(false);
|
||||
|
||||
// Turn B is sent but has not yet been armed by its own ack/start.
|
||||
await act(async () => {
|
||||
latest?.actions.sendMessage({ content: 'turn B' });
|
||||
});
|
||||
expect(latest?.state.sending).toBe(true);
|
||||
|
||||
// An approval request is outstanding on c1 — approveCommand has no
|
||||
// dependency on `sending`/`streaming`, so this is legitimate even while
|
||||
// turn B is unarmed.
|
||||
await act(async () => {
|
||||
latest?.actions.approveCommand({ command: 'deploy', args: 'prod' });
|
||||
});
|
||||
expect(latest?.state.approvalRequestPending).toBe(true);
|
||||
expect(latest?.state.pendingApproval).toEqual({ command: 'deploy', args: 'prod' });
|
||||
expect(fake.emitted.filter((e) => e.event === 'command:approve')).toHaveLength(1);
|
||||
|
||||
// A stale error from turn A, same conversationId, arrives before turn
|
||||
// B's own ack/start. This must be a true no-op: it must not release
|
||||
// `sending`, must not display/store its message, must not touch
|
||||
// `streaming`, and — critically — must not clear
|
||||
// `approvalRequestPending`/`pendingApproval`, which would re-arm the
|
||||
// approve UI for a request that is still outstanding.
|
||||
await act(async () => {
|
||||
fake.serverEmit('error', { conversationId: 'c1', error: 'stale turn A failure' });
|
||||
});
|
||||
|
||||
expect(latest?.state.sending).toBe(true);
|
||||
expect(latest?.state.approvalRequestPending).toBe(true);
|
||||
expect(latest?.state.pendingApproval).toEqual({ command: 'deploy', args: 'prod' });
|
||||
expect(latest?.state.error).toBeNull();
|
||||
expect(latest?.state.streaming).toBe(false);
|
||||
|
||||
// A second approval attempt while the first is still outstanding must
|
||||
// still be rejected — exactly one command:approve total, and the
|
||||
// original frozen command+args must be unchanged.
|
||||
await act(async () => {
|
||||
latest?.actions.approveCommand({ command: 'deploy', args: 'staging' });
|
||||
});
|
||||
expect(fake.emitted.filter((e) => e.event === 'command:approve')).toHaveLength(1);
|
||||
expect(latest?.state.pendingApproval).toEqual({ command: 'deploy', args: 'prod' });
|
||||
});
|
||||
|
||||
it('removes every listener and tears down the socket on cleanup, using no network', async () => {
|
||||
const registeredEvents = [...fake.listeners.keys()];
|
||||
expect(registeredEvents.length).toBeGreaterThan(0);
|
||||
|
||||
Reference in New Issue
Block a user