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
24 changes: 23 additions & 1 deletion app/src/components/approvals/inline-approval.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,24 @@ export function InlineApproval({
) : null}
</AnimatePresence>
{pending ? null : fallback}
{/*
The conversation could not be carried on after this was decided, so the step above never got
its result and would say "Send message…" for good. What came of it is said here, from the
request itself, which is the one place that still knows.
*/}
{request?.left ? (
<p
className={
request.left.done
? "mt-1 text-muted-foreground text-sm"
: "mt-1 text-destructive text-sm"
}
data-testid="approval-left"
role={request.left.done ? "status" : "alert"}
>
{request.left.message}
</p>
) : null}
</>
);
}
Expand Down Expand Up @@ -189,9 +207,13 @@ export function PendingApprovals({
toolCallIds: readonly string[];
}) {
const inbox = useQuery(approvalInboxOptions());
/*
* And one whose conversation could not be carried on after it was decided (`left`): what came of
* it is said beside the group too, since the folded line ("Used Slack") says nothing of it.
*/
const waiting = new Set(
(inbox.data?.requests ?? [])
.filter((request) => request.status === "pending")
.filter((request) => request.status === "pending" || request.left)
.flatMap((request) =>
request.action.toolCallId ? [request.action.toolCallId] : [],
),
Expand Down
7 changes: 6 additions & 1 deletion app/src/components/channels/channel-chat.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ import {
answerPersonQuestionMutationOptions,
approvalInboxOptions,
carryingOnAfterApproval,
leftAnswers,
} from "@/lib/approvals";
import { attachmentUrl } from "@/lib/channels/attachments";
import {
Expand Down Expand Up @@ -1267,7 +1268,11 @@ export function ChannelChat({
report(trimmed || describeAttachments(attachments), null);

// Providers reject later turns if prior tool calls have no result; repair before sending.
const repaired = repairUnansweredToolCalls(target.messages);
const repaired = repairUnansweredToolCalls(
target.messages,
undefined,
leftAnswers(queryClient.getQueryData(approvalInboxOptions().queryKey)),
);
if (repaired !== target.messages) {
target.setMessages(repaired as typeof target.messages);
}
Expand Down
20 changes: 20 additions & 0 deletions app/src/lib/approvals.ts
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,11 @@ export type ApprovalInboxData = {
decidedAt?: string | null;
/** What the person answered; null while it waits. */
decision?: ApprovalDecision | null;
/**
* Set when the conversation could not be carried on after this was decided: whether the action
* was done, the sentence that says so, and the answer the Bot never received.
*/
left?: { done: boolean; message: string; answer: string };
action: {
botId: string;
toolRef: string;
Expand Down Expand Up @@ -121,6 +126,21 @@ export type ApprovalInboxData = {
createdAt: string;
}[];
};
/**
* The answers the Bot never received, by the call each belongs to: actions decided whose
* conversation could not be carried on (`left`). Sent with the next message in place of "no result
* came back", so the Bot knows a post it asked for went out and does not make it again.
*/
export function leftAnswers(
inbox: ApprovalInboxData | undefined,
): ReadonlyMap<string, string> {
const answers = new Map<string, string>();
for (const request of inbox?.requests ?? [])
if (request.left && request.action.toolCallId)
answers.set(request.action.toolCallId, request.left.answer);
return answers;
}

export const approvalInboxOptions = () =>
queryOptions({
queryKey: ["approvals"],
Expand Down
7 changes: 6 additions & 1 deletion app/src/lib/copilot/repair-history.ts
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,11 @@ export function repairUnansweredToolCalls(
* repair branch, which every test avoided by passing its own. Biome said so, as an unused import.
*/
mintId: () => string = newId,
/**
* Answers known from somewhere other than the thread, by call id: an approved action whose
* conversation could not be carried on keeps its result on the request (`leftAnswers`).
*/
known: ReadonlyMap<string, string> = new Map(),
): ReadonlyArray<Message> {
/*
* A held step's stand-in is not its answer. Counted as one, it sat right after the call, so the
Expand Down Expand Up @@ -188,7 +193,7 @@ export function repairUnansweredToolCalls(
id: mintId(),
role: "tool",
toolCallId: call.id,
content: UNANSWERED,
content: known.get(call.id) || UNANSWERED,
} as Message),
);
// A duplicated call id may only receive one repair result.
Expand Down
164 changes: 164 additions & 0 deletions app/tests/approval-left-not-carried-on.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,164 @@
import { afterAll, afterEach, beforeAll, expect, test } from "bun:test";
import type { Message } from "@ag-ui/core";
import { GlobalRegistrator } from "@happy-dom/global-registrator";
import { QueryClient, QueryClientProvider } from "@tanstack/react-query";
import { cleanup, render, waitFor } from "@testing-library/react";
import {
InlineApproval,
PendingApprovals,
} from "@/components/approvals/inline-approval";
import { type ApprovalInboxData, leftAnswers } from "@/lib/approvals";
import {
repairUnansweredToolCalls,
UNANSWERED,
} from "@/lib/copilot/repair-history";
import { settleReactWork } from "./settle-react-work";

/*
* FOUND IN TESTING. A person allowed a Slack post, the post went out, and the conversation could
* not be carried on after it. The step sat on "Send message…" with nothing said, so the person
* never learned the post was made, and the next message sent the Bot "no result came back" for a
* call that had in fact run, which invites it to post again.
*
* The request now says what came of it (`left`). The step draws that sentence, and the next message
* sends the Bot the answer the conversation never received.
*/

const originalFetch = global.fetch;
beforeAll(() => {
GlobalRegistrator.register();
});
afterEach(() => {
cleanup();
global.fetch = originalFetch;
});
afterAll(async () => {
await settleReactWork();
GlobalRegistrator.unregister();
});

const DONE =
"Done: send message on Slack. OpenBot couldn't carry on the conversation after that, so nothing else was done. Send a message to continue.";
const ANSWER =
'Ran: the app made this change. It answered: {"ok":true,"channel":"C0LAUNCH01"}';

/** `GET /api/approvals`, with one request whose conversation was never carried on. */
function inbox(left: { done: boolean; message: string; answer: string }) {
return {
enabled: true,
requests: [
{
id: "approval-1",
status: "completed",
createdAt: "2026-11-14T04:06:40Z",
decidedAt: "2026-11-14T04:06:47Z",
decision: "allow_once",
left,
action: {
botId: "personal-assistant",
toolRef: "composio-slack/SLACK_SEND_MESSAGE",
effect: "write",
scope: "composio-slack",
threadId: "thread-1",
toolCallId: "call-send",
args: { channel: "C0LAUNCH01", markdown_text: "the deck is ready" },
target: null,
},
},
],
rules: [],
teamRules: [],
questions: [],
} as unknown as ApprovalInboxData;
}

function drawn(data: ApprovalInboxData) {
global.fetch = (async () => Response.json(data)) as unknown as typeof fetch;
const client = new QueryClient({
defaultOptions: { queries: { retry: false } },
});
return render(
<QueryClientProvider client={client}>
<InlineApproval fallback={<p>Send message…</p>} toolCallId="call-send" />
</QueryClientProvider>,
);
}

test("a step whose conversation could not be carried on says the action was done and what to do next", async () => {
const view = drawn(inbox({ done: true, message: DONE, answer: ANSWER }));
const told = await waitFor(() => view.getByTestId("approval-left"));
expect(told.textContent).toBe(DONE);
// Said as a status under the step, which is still drawn: nothing replaces the step itself.
expect(told.getAttribute("role")).toBe("status");
expect(view.getByText("Send message…")).toBeTruthy();
});

test("a step folded into its group says so beside the group, where a waiting card would be", async () => {
global.fetch = (async () =>
Response.json(
inbox({ done: true, message: DONE, answer: ANSWER }),
)) as unknown as typeof fetch;
const client = new QueryClient({
defaultOptions: { queries: { retry: false } },
});
const view = render(
<QueryClientProvider client={client}>
<PendingApprovals toolCallIds={["call-find", "call-send"]} />
</QueryClientProvider>,
);
const told = await waitFor(() => view.getByTestId("approval-left"));
expect(told.textContent).toBe(DONE);
});

test("one that was not done says so as an alert", async () => {
const message =
"Not done: send message on Slack. OpenBot couldn't carry on the conversation. Send a message to try again.";
const view = drawn(inbox({ done: false, message, answer: "Not done." }));
const told = await waitFor(() => view.getByTestId("approval-left"));
expect(told.textContent).toBe(message);
expect(told.getAttribute("role")).toBe("alert");
});

test("the next message sends the Bot the answer it never received, not that nothing came back", () => {
const history = [
{ id: "u1", role: "user", content: "Post 'the deck is ready' in #launch." },
{
id: "a1",
role: "assistant",
content: "",
toolCalls: [
{
id: "call-send",
type: "function",
function: { name: "SLACK_SEND_MESSAGE", arguments: "{}" },
},
{
id: "call-other",
type: "function",
function: { name: "SLACK_FIND_USERS", arguments: "{}" },
},
],
},
] as unknown as Message[];
const known = leftAnswers(
inbox({ done: true, message: DONE, answer: ANSWER }),
);
const repaired = repairUnansweredToolCalls(history, () => "minted", known);
const results = repaired.filter((message) => message.role === "tool") as {
toolCallId: string;
content: string;
}[];
expect(results.map((result) => [result.toolCallId, result.content])).toEqual([
["call-send", ANSWER],
// A call nothing is known about keeps the words it always had.
["call-other", UNANSWERED],
]);
// With nothing known, the same history is repaired as before.
expect(
(
repairUnansweredToolCalls(history, () => "minted").filter(
(message) => message.role === "tool",
) as { content: string }[]
).map((result) => result.content),
).toEqual([UNANSWERED, UNANSWERED]);
});
Loading