diff --git a/backend/__tests__/unit/services/nativeRuntimeService.byoHandoff.test.js b/backend/__tests__/unit/services/nativeRuntimeService.byoHandoff.test.js index c69891ca9..fc3e372db 100644 --- a/backend/__tests__/unit/services/nativeRuntimeService.byoHandoff.test.js +++ b/backend/__tests__/unit/services/nativeRuntimeService.byoHandoff.test.js @@ -204,7 +204,8 @@ describe('commonly_offer_local_agent', () => { const result = await runAgent(install, ask); - expect(result.status).toBe('succeeded'); + // Its last text was held back too, so the turn answered no one. + expect(result).toEqual(expect.objectContaining({ status: 'failed', errorKind: 'guardrail_blocked' })); // The runtime offers every model tool call to the broker; only the granted // tool is a broker dispatch, so count that one. const brokerDispatches = mockDispatchHostedBrokerTool.mock.calls diff --git a/backend/__tests__/unit/services/nativeRuntimeService.guardStop.test.js b/backend/__tests__/unit/services/nativeRuntimeService.guardStop.test.js index 4d5fa272f..42592661a 100644 --- a/backend/__tests__/unit/services/nativeRuntimeService.guardStop.test.js +++ b/backend/__tests__/unit/services/nativeRuntimeService.guardStop.test.js @@ -27,12 +27,19 @@ jest.mock('../../../models/User', () => ({ findById: jest.fn(() => ({ select: () => ({ lean: () => Promise.resolve({ entitlements: { pro: false }, isBot: false }) }) })), })); jest.mock('../../../services/agentTypingService', () => ({ emitAgentTypingStart: jest.fn(), emitAgentTypingStop: jest.fn() })); -jest.mock('../../../services/agentMessageService', () => ({ postMessage: jest.fn() })); +jest.mock('../../../services/agentMessageService', () => ({ + postMessage: jest.fn(), + // The real sentinel rules, so a NO_REPLY reads as silent. + sanitizeAgentContent: (content) => jest.requireActual('../../../services/agentMessageService').sanitizeAgentContent(content), +})); jest.mock('../../../services/messageClaimService', () => ({ claim: jest.fn().mockResolvedValue({ claimed: true, expiresAt: new Date() }), release: jest.fn().mockResolvedValue({ released: true }), })); -jest.mock('../../../services/messageClaimHandoffService', () => ({ handOff: jest.fn().mockResolvedValue(undefined) })); +jest.mock('../../../services/messageClaimHandoffService', () => ({ + handOff: jest.fn().mockResolvedValue(undefined), + release: jest.fn().mockResolvedValue({ released: true }), +})); jest.mock('../../../models/pg/Message', () => ({ findByPodId: jest.fn().mockResolvedValue([]) })); jest.mock('../../../services/nativeOutsiderGrantPolicy', () => ({ podHasOutsiderWritableGrant: jest.fn().mockResolvedValue(false), @@ -58,6 +65,7 @@ jest.mock('../../../models/Message', () => { const axios = require('axios').default; const AgentRun = require('../../../models/AgentRun'); const AgentMessageService = require('../../../services/agentMessageService'); +const ClaimHandoff = require('../../../services/messageClaimHandoffService'); const { runAgent, HOSTED_RUN_FAILED_LINE } = require('../../../services/nativeRuntimeService'); const POD = '507f1f77bcf86cd799439011'; @@ -170,8 +178,9 @@ describe('a hosted turn the guard stops, or that fails', () => { expect(posted()).toEqual(['Here is the changelog. I could not use the blog page.']); }); - // The extra round is one model call, not the rest of the budget. - describe('the extra round is one model call', () => { + // The extra round is one model call, not the rest of the budget. A call that + // asked only for tools it was not offered gets one more, after the refusal. + describe('the extra round is one model call, or two when the first asks only for other tools', () => { const neverAsked = reply('A later round.'); test('a reply posted through the tool ends the run, with one post', async () => { @@ -207,23 +216,158 @@ describe('a hosted turn the guard stops, or that fails', () => { expect(posted()).toEqual(['Recovered once.']); }); - test('a lookup asked for in that call is refused and the run ends with the fixed line', async () => { + // F33 (11-01): a research turn read two releases pages, the guard stopped + // one, and in the extra round the model asked for that page again. The + // refusal ended the run before the model saw it, 4 runs in 3 days. + test('a lookup asked for in that call is refused, and one more call answers', async () => { + chatReplies = [ + toolCalls(fetchCall('c1', CLEAN_URL), fetchCall('c2', STOPPED_URL)), + guardStop, + toolCalls(fetchCall('c3', STOPPED_URL)), + toolCalls(call('c4', 'commonly_post_message', { podId: POD, content: 'The changelog says this. I could not use the blog page.' })), + neverAsked, + ]; + + const result = await runAgent(hostedInstall(), mention); + + expect(result.status).toBe('succeeded'); + expect(chatBodies).toHaveLength(4); + expect(mockGetPage.mock.calls).toEqual([[CLEAN_URL], [STOPPED_URL]]); + // The model reads the refusal before its last call, offered a post only. + const last = chatBodies[3]; + expect(offeredIn(last)).toEqual(['commonly_post_message']); + expect(last.messages[last.messages.length - 1]).toEqual(expect.objectContaining({ role: 'tool', tool_call_id: 'c3' })); + expect(String(last.messages[last.messages.length - 1].content)).toMatch(/No more lookups/); + expect(JSON.stringify(last.messages)).not.toContain(STOPPED_TEXT); + expect(posted()).toEqual(['The changelog says this. I could not use the blog page.']); + expect(savedRun.metered).toBeUndefined(); + }); + + test('a second refused lookup ends the run, naming the step and the tool, with the fixed line', async () => { chatReplies = [ toolCalls(fetchCall('c1', STOPPED_URL)), guardStop, toolCalls(fetchCall('c2', CLEAN_URL)), + toolCalls(fetchCall('c3', CLEAN_URL)), neverAsked, ]; const result = await runAgent(hostedInstall(), mention); expect(result).toEqual(expect.objectContaining({ status: 'failed', errorKind: 'guardrail_blocked' })); - expect(chatBodies).toHaveLength(3); + expect(savedRun.errorMessage).toMatch(/round after .* ended without a reply/); + expect(savedRun.errorMessage).toContain('commonly_fetch_url'); + expect(chatBodies).toHaveLength(4); expect(mockGetPage.mock.calls).toEqual([[STOPPED_URL]]); expect(posted()).toEqual([HOSTED_RUN_FAILED_LINE]); expect(savedRun.metered).toBe(false); }); + // The extra call was given because the model meant to answer, so silence + // from it is a failed reply, not a decline. + test('an empty extra call ends the run with the fixed line', async () => { + chatReplies = [toolCalls(fetchCall('c1', STOPPED_URL)), guardStop, toolCalls(fetchCall('c2', STOPPED_URL)), reply(''), neverAsked]; + + const result = await runAgent(hostedInstall(), mention); + + expect(result).toEqual(expect.objectContaining({ status: 'failed', errorKind: 'guardrail_blocked' })); + expect(savedRun.errorMessage).toMatch(/ended without a reply: its extra call returned nothing\.$/); + expect(chatBodies).toHaveLength(4); + expect(posted()).toEqual([HOSTED_RUN_FAILED_LINE]); + expect(savedRun.metered).toBe(false); + }); + + test('a reply held back because the pod was bridged after a page read fails the run, unmetered', async () => { + mockBridged.mockImplementation(async () => mockGetPage.mock.calls.length > 0); + chatReplies = [toolCalls(fetchCall('c1', STOPPED_URL)), guardStop, reply('From what I have.'), neverAsked]; + + const result = await runAgent(hostedInstall(), mention); + + expect(result).toEqual(expect.objectContaining({ status: 'failed', errorKind: 'guardrail_blocked' })); + expect(savedRun.metered).toBe(false); + // Bridged, so not even the fixed line goes out. + expect(posted()).toEqual([]); + }); + + // CodeRabbit on 67496fac9: a decline is not a held-back reply. In a bridged + // pod it must still end as a decline, so the wake goes on to another agent. + test('a NO_REPLY after a page read in a bridged pod stays a decline, handed on', async () => { + mockBridged.mockImplementation(async () => mockGetPage.mock.calls.length > 0); + AgentMessageService.postMessage.mockResolvedValue({ skipped: true }); + chatReplies = [toolCalls(fetchCall('c1', CLEAN_URL)), reply('NO_REPLY'), neverAsked]; + + // A hosted install never takes a message.posted wake; a native one does. + const result = await runAgent({ + podId: POD, + agentName: 'analyst', + instanceId: 'default', + displayName: 'Analyst', + installedBy: OWNER, + config: { runtime: { runtimeType: 'native' }, tools: ['commonly_fetch_url', 'commonly_post_message'] }, + }, { + type: 'message.posted', + eventId: '507f1f77bcf86cd799439014', + payload: { content: `anyone read ${CLEAN_URL}?`, userId: OWNER, messageId: 'msg-2', senderIsHuman: true }, + }); + + expect(mockGetPage).toHaveBeenCalledWith(CLEAN_URL); + expect(mockBridged).toHaveBeenCalled(); + expect(result.status).toBe('succeeded'); + expect(savedRun.errorKind).toBeUndefined(); + expect(ClaimHandoff.release).toHaveBeenCalledWith(expect.objectContaining({ outcome: 'declined' })); + }); + + test('a post refused in that call ends the run, with no second call', async () => { + // The bridge is added once the page is in the turn, so the post is refused. + mockBridged.mockImplementation(async () => mockGetPage.mock.calls.length > 0); + chatReplies = [ + toolCalls(fetchCall('c1', STOPPED_URL)), + guardStop, + toolCalls(call('c2', 'commonly_post_message', { podId: POD, content: 'Recovered.' })), + neverAsked, + ]; + + const result = await runAgent(hostedInstall(), mention); + + expect(result.status).toBe('failed'); + expect(savedRun.errorMessage).toMatch(/ended without a reply: its post was refused\.$/); + expect(chatBodies).toHaveLength(3); + expect(posted()).toEqual([]); + }); + + // The extra call can itself be stopped. Then every web result left is set + // aside, and the last round still gets no further call. + test('a stop on the extra call sets the rest aside, and a refetch after it ends the run', async () => { + chatReplies = [ + toolCalls(fetchCall('c1', CLEAN_URL), fetchCall('c2', STOPPED_URL)), + guardStop, + toolCalls(fetchCall('c3', STOPPED_URL)), + guardStop, + toolCalls(fetchCall('c4', STOPPED_URL)), + neverAsked, + ]; + + const result = await runAgent(hostedInstall(), mention); + + expect(result).toEqual(expect.objectContaining({ status: 'failed', errorKind: 'guardrail_blocked' })); + expect(chatBodies).toHaveLength(5); + chatBodies.slice(2).forEach((body) => expect(offeredIn(body)).toEqual(['commonly_post_message'])); + const last = JSON.stringify(chatBodies[4].messages); + expect(last).not.toContain(CLEAN_TEXT); + expect(last).not.toContain(STOPPED_TEXT); + expect(mockGetPage.mock.calls).toEqual([[CLEAN_URL], [STOPPED_URL]]); + expect(savedRun.metered).toBe(false); + expect(posted()).toEqual([HOSTED_RUN_FAILED_LINE]); + }); + + test('the set-aside notice says no lookups are left', async () => { + chatReplies = [toolCalls(fetchCall('c1', STOPPED_URL)), guardStop, reply('I could not use the blog page.')]; + + await runAgent(hostedInstall(), mention); + + expect(JSON.stringify(chatBodies[2].messages)).toMatch(/No more lookups are possible in this turn/); + }); + test('a declined reply in that call ends the run without the fixed line', async () => { AgentMessageService.postMessage.mockResolvedValue({ skipped: true }); chatReplies = [toolCalls(fetchCall('c1', STOPPED_URL)), guardStop, reply('NO_REPLY'), neverAsked]; diff --git a/backend/__tests__/unit/services/nativeRuntimeService.webSearch.test.js b/backend/__tests__/unit/services/nativeRuntimeService.webSearch.test.js index d078b0fee..79d85f83c 100644 --- a/backend/__tests__/unit/services/nativeRuntimeService.webSearch.test.js +++ b/backend/__tests__/unit/services/nativeRuntimeService.webSearch.test.js @@ -22,7 +22,10 @@ jest.mock('../../../models/User', () => ({ })), })); jest.mock('../../../services/agentTypingService', () => ({ emitAgentTypingStart: jest.fn(), emitAgentTypingStop: jest.fn() })); -jest.mock('../../../services/agentMessageService', () => ({ postMessage: jest.fn().mockResolvedValue({ success: true }) })); +jest.mock('../../../services/agentMessageService', () => ({ + postMessage: jest.fn().mockResolvedValue({ success: true }), + sanitizeAgentContent: jest.fn((content) => content), +})); const mockProposeAction = jest.fn(); jest.mock('../../../services/approvalActionService', () => ({ proposeAction: (...args) => mockProposeAction(...args), diff --git a/backend/services/nativeRuntimeService.ts b/backend/services/nativeRuntimeService.ts index a7442fbcc..f6b1c29d5 100644 --- a/backend/services/nativeRuntimeService.ts +++ b/backend/services/nativeRuntimeService.ts @@ -141,8 +141,14 @@ const withDeadline = (work: Promise, ms: number): Promise => { // A web result the guard stops on its own is replaced with this before the // turn's one extra round. It names no rule and repeats nothing from the result. const WEB_SET_ASIDE_NOTICE = 'This result was set aside: a content filter stopped it, and its text is no longer ' - + 'in this conversation. Answer with what you have, and tell the person which page or search you could not use.'; + + 'in this conversation. No more lookups are possible in this turn: answer with what you have, and tell the ' + + 'person which page or search you could not use.'; const RECOVERY_REPLIED_REFUSAL = 'This turn has posted its reply.'; +// The refusal for any tool that round does not offer. When the round did not +// try to post, the model reads it before one further call (F33: it asked for +// the set-aside page again). +const RECOVERY_NOT_OFFERED_REFUSAL = 'No more lookups are possible in this turn. Reply with commonly_post_message from ' + + 'what you have, and tell the person which page or search you could not use.'; // What a hosted agent posts when a turn someone was waiting on ends on a guard // stop or an error with nothing posted. Fixed text, so it carries nothing the // turn read. @@ -2216,6 +2222,9 @@ export async function runAgent( // Set when the recovery round was stopped too, and every web result left // was set aside for one last round. let webSetAsideAll = false; + // Set when the recovery round asked only for tools it was not offered and + // was given one more call to read the refusal (F33). + let recoveryRetried = false; // Returns how many results were set aside, and how many web results the // turn lost in all (those plus the ones in the dropped rounds). const setAsideStoppedWebResult = async (): Promise<{ setAside: number; removed: number }> => { @@ -2282,7 +2291,9 @@ export async function runAgent( const webTainted = Boolean(dispatchCtx.webTainted); const podBridged = mayLookUp ? await podBridgedNow() : false; // The round after a set-aside is the recovery round: one model call, - // offered only a post, and the run ends after it whatever it did. + // offered only a post, and the run ends after it whatever it did. The + // one exception is a call that asked only for other tools: it gets a + // second call, after reading the refusal. const recoveryRound = webSetAside; offeredBroker = broker; const offeredTools = [ @@ -2481,7 +2492,9 @@ export async function runAgent( const narrowedAway = (Boolean(dispatchCtx.inPodOnly) && !keptInPod(toolName, Boolean(dispatchCtx.brokerCallSeen))) || withdrawnAfterBroker(toolName, Boolean(dispatchCtx.brokerCallSeen)); - const notOffered = `${toolName || 'That tool'} is not available to you in this turn.`; + const notOffered = recoveryRound + ? RECOVERY_NOT_OFFERED_REFUSAL + : `${toolName || 'That tool'} is not available to you in this turn.`; // The recovery round posts at most one reply. const replied = recoveryRound && postedViaTool ? RECOVERY_REPLIED_REFUSAL : ''; let result: DispatchResult; @@ -2560,12 +2573,24 @@ export async function runAgent( // The recovery round ends the run: answered (or declined through a // post) is a success, anything else could not finish. if (recoveryRound) { + const asked = toolCalls.map((tc) => tc.function?.name || ''); if (postedViaTool) { run.status = run.status === 'running' ? 'succeeded' : run.status; + } else if (!recoveryRetried && !asked.includes('commonly_post_message')) { + // On prod the model asked for the set-aside page again, every time + // (F33), and the run ended before it read the refusal. + recoveryRetried = true; + continue; } else { run.status = 'failed'; run.errorKind = 'guardrail_blocked'; - run.errorMessage = 'Request blocked by a content guardrail (prompt-injection / moderation).'; + const other = [...new Set(asked.filter((name) => name && name !== 'commonly_post_message'))]; + const why = [ + ...(asked.includes('commonly_post_message') ? ['its post was refused'] : []), + ...(other.length ? [`it asked for ${other.join(', ')}, which that round does not offer`] : []), + ]; + run.errorMessage = 'A content filter set web results aside, and the round after it ended without a reply' + + (why.length ? `: ${why.join('; ')}.` : '.'); } break; } @@ -2592,10 +2617,15 @@ export async function runAgent( const textOut = typeof msg.content === 'string' ? msg.content.trim() : ''; // The fallback post is a post like any other: after an external result, - // re-read the bridge before letting it through. + // re-read the bridge before letting it through. A silent reply (NO_REPLY) + // is a decline, not a held-back answer: it would post nothing anyway. const fallbackBlocked = Boolean(textOut && !postedViaTool - && (dispatchCtx.webTainted || dispatchCtx.brokerCallSeen) && await podBridgedNow()); + && (dispatchCtx.webTainted || dispatchCtx.brokerCallSeen) && await podBridgedNow() + && require('./agentMessageService').sanitizeAgentContent(textOut)); + // Held back, the reply never reached the person: the run failed, so it + // is not metered and its digest is not settled as answered. if (fallbackBlocked) { + run.status = 'failed'; run.errorKind = 'guardrail_blocked'; run.errorMessage = dispatchCtx.webTainted ? WEB_TAINTED_BRIDGED_REFUSAL : BROKER_TAINTED_BRIDGED_REFUSAL; } @@ -2632,6 +2662,14 @@ export async function runAgent( run.errorKind = 'tool_error'; } } + // The extra recovery call was given because the model meant to answer, + // so an empty one is a reply that never came, not a decline. + if (recoveryRetried && !textOut && !postedViaTool) { + run.status = 'failed'; + run.errorKind = 'guardrail_blocked'; + run.errorMessage = 'A content filter set web results aside, and the round after it ended without a reply: ' + + 'its extra call returned nothing.'; + } run.status = run.status === 'running' ? 'succeeded' : run.status; break;