mosaicstack/stack
1
0
Fork 1
Code Issues 289 Pull Requests 19 Actions Packages Projects Releases 15 Wiki Activity

fix(web,gateway): close P3 re-review#3 findings — sanitize command errors, harden turn-lock & caps

Browse Source
This commit is contained in:
shaggy (mosaic-dev box)
2026-08-10 14:21:52 -05:00
parent 48bb19310d
commit d46a2d675a
7 changed files with 350 additions and 166 deletions
Download Patch File Download Diff File Expand all files Collapse all files
apps/web/src/spa/chat/use-chat-connection.spec.tsx
+151 -79
View File
@@ -287,6 +287,26 @@ describe('useChatConnection', () => {
]);
});
it('bounds an oversized command:result message to a fixed cap before it ever reaches state', async () => {
await act(async () => {
fake.serverEmit('message:ack', { conversationId: 'c1', messageId: 'm1' });
});
const hostileMessage = 'x'.repeat(50_000);
await act(async () => {
fake.serverEmit('command:result', {
conversationId: 'c1',
command: 'model',
success: false,
message: hostileMessage,
});
});
const stored = latest?.state.commandResults.at(-1)?.message ?? '';
expect(stored.length).toBeLessThan(hostileMessage.length);
expect(stored.length).toBeLessThanOrEqual(1000);
});
it('pairs a successful command:approval with the pending request so it can be run with its approvalId', async () => {
await act(async () => {
fake.serverEmit('message:ack', { conversationId: 'c1', messageId: 'm1' });
@@ -1051,7 +1071,7 @@ describe('useChatConnection', () => {
expect(latest?.state.manifest?.commands).toHaveLength(MAX_MANIFEST_ITEMS);
});
it('caps a system:reload manifest replacement at MAX_MANIFEST_ITEMS for both the raw stored reload and the manifest', async () => {
it('caps a system:reload manifest replacement at MAX_MANIFEST_ITEMS for commands, skills, and providers, and drops hostile extra fields instead of spreading the raw payload into state', async () => {
const commands = Array.from({ length: MAX_MANIFEST_ITEMS + 5 }, (_, i) => ({
name: `cmd${i}`,
aliases: [],
@@ -1065,15 +1085,21 @@ describe('useChatConnection', () => {
description: '',
available: true,
}));
const providers = Array.from({ length: 550 }, (_, i) => `provider-${i}`);
await act(async () => {
fake.serverEmit('system:reload', {
commands,
skills,
providers: ['anthropic'],
message: 'reloaded',
});
});
await expect(
act(async () => {
fake.serverEmitRaw('system:reload', {
commands,
skills,
providers,
message: 'reloaded',
// Hostile field not part of the SystemReloadPayload contract —
// must never survive into state.systemReload.
maliciousExtra: 'should-not-survive',
});
}),
).resolves.not.toThrow();
expect(latest?.state.manifest?.commands).toHaveLength(MAX_MANIFEST_ITEMS);
expect(latest?.state.manifest?.skills).toHaveLength(MAX_MANIFEST_ITEMS);
@@ -1082,6 +1108,8 @@ describe('useChatConnection', () => {
// 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);
expect(latest?.state.systemReload?.providers).toHaveLength(500);
expect(latest?.state.systemReload).not.toHaveProperty('maliciousExtra');
});
it('caps availableThinkingLevels before storing a hostile session:info payload', async () => {
@@ -1390,6 +1418,16 @@ describe('useChatConnection', () => {
latest?.actions.runApprovedCommand();
});
// The rejected 201st approval is not silently dropped — it must be
// consumed but also surface a stable, visible notice so the user knows
// why the command did not run. ChatPage renders state.error as
// role="alert".
expect(latest?.state.error).toBe(
'Approval limit reached for this session. This command was not run.',
);
expect(latest?.state.approval).toBeNull();
expect(latest?.state.pendingApproval).toBeNull();
// 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.
@@ -1473,55 +1511,7 @@ describe('useChatConnection', () => {
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 () => {
it('settles turn B and releases its lock when a pre-ack error arrives on an already-established conversation, allowing a later send', async () => {
// Turn A completes normally on c1.
await act(async () => {
fake.serverEmit('message:ack', { conversationId: 'c1', messageId: 'm1' });
@@ -1530,7 +1520,41 @@ describe('useChatConnection', () => {
});
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);
// A same-conversation error arrives before turn B's own ack/start. Once
// turn A has already fully settled via its own terminal event, ordered
// Socket.IO delivery means this cannot be a leftover of A — the Gateway
// has nothing left in flight to emit for a turn it already finished. It
// can only be a genuine error for the newly sent turn B (e.g. a
// session-creation failure emitted before ack), so it must settle B.
await act(async () => {
fake.serverEmit('error', { conversationId: 'c1', error: 'turn B session failure' });
});
expect(latest?.state.sending).toBe(false);
expect(latest?.state.streaming).toBe(false);
expect(latest?.state.error).toBe('turn B session failure');
await act(async () => {
latest?.actions.sendMessage({ content: 'turn C' });
});
expect(fake.emitted.filter((e) => e.event === 'message')).toHaveLength(2);
expect(latest?.state.messages.some((m) => m.text === 'turn C')).toBe(true);
});
it('invalidates an outstanding approval request when a pre-ack error settles turn B on an already-established conversation', 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 received its own ack/start.
await act(async () => {
latest?.actions.sendMessage({ content: 'turn B' });
});
@@ -1538,38 +1562,86 @@ describe('useChatConnection', () => {
// An approval request is outstanding on c1 — approveCommand has no
// dependency on `sending`/`streaming`, so this is legitimate even while
// turn B is unarmed.
// turn B has not yet started.
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.
// A genuine pre-ack error for turn B is terminal — same as an
// active-turn error, it invalidates any approval request still awaiting
// a response, since the Gateway that just errored is unlikely to still
// answer it.
await act(async () => {
fake.serverEmit('error', { conversationId: 'c1', error: 'stale turn A failure' });
fake.serverEmit('error', { conversationId: 'c1', error: 'turn B session 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);
expect(latest?.state.sending).toBe(false);
expect(latest?.state.approvalRequestPending).toBe(false);
expect(latest?.state.error).toBe('turn B session failure');
// 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.
// A fresh approval request can be issued again after the invalidation.
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' });
expect(fake.emitted.filter((e) => e.event === 'command:approve')).toHaveLength(2);
});
it("does not settle turn B when a duplicate agent:end from already-settled turn A is redelivered after B's own ack but before B's own start", 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 and its own ack arrives. Under the old
// ack-arms-the-lock design this alone made the lock releasable by any
// same-conversation terminal — the precise bug: it could not yet
// distinguish B's own eventual agent:end from a late duplicate delivery
// of A's already-consumed one.
await act(async () => {
latest?.actions.sendMessage({ content: 'turn B' });
});
await act(async () => {
fake.serverEmit('message:ack', { conversationId: 'c1', messageId: 'm2' });
});
expect(latest?.state.sending).toBe(true);
// A duplicate/straggler agent:end for turn A (already fully settled
// above) is redelivered for the same conversation before B's own
// agent:start ever arrived. Only B's own accepted agent:start may move
// it into the active phase that a real agent:end may settle — this
// duplicate must be a true no-op.
await act(async () => {
fake.serverEmit('agent:end', { conversationId: 'c1' });
});
expect(latest?.state.sending).toBe(true);
expect(latest?.state.streaming).toBe(false);
// Turn C must still be blocked — the lock is still genuinely held by B.
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);
// B's own start, then its own end, legitimately unlocks it.
await act(async () => {
fake.serverEmit('agent:start', { conversationId: 'c1' });
});
expect(latest?.state.streaming).toBe(true);
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('removes every listener and tears down the socket on cleanup, using no network', async () => {