mirror of
https://github.com/getpaseo/paseo.git
synced 2026-11-03 07:03:18 +00:00
fix(codex): show the file and diff before patch approval (#8585)
* fix(codex): include patch details in file change approvals * fix(codex): name the file in patch approvals The approval detail renderer shows the diff without its filePath. Include the file in the approval title so the pending decision identifies the change being reviewed. Co-authored-by: Okan YILDIRIM <63511519+ragokan@users.noreply.github.com> * fix(codex): await patch notifications before approval Approval requests bypass the session's notification conversion queue. Drain earlier notifications before reading the patch preview so an approval in the same stdout batch receives its matching file and diff. Co-authored-by: Okan YILDIRIM <63511519+ragokan@users.noreply.github.com> * test(codex): express patch approval through provider actions Keep protocol framing in the fake app-server and precompute the approval title. Streamed completion already requires an item ID; expose that narrowing so completion cleanup stays inline without repeating its guard. --------- Co-authored-by: Okan YILDIRIM <63511519+ragokan@users.noreply.github.com> Co-authored-by: paseo-bot[bot] <266920839+paseo-bot[bot]@users.noreply.github.com>
This commit is contained in:
paseo-bot[bot]
co-authored by
Okan YILDIRIM
Okan YILDIRIM
parent
e06adf7a3e
commit
1f64e68987
@@ -227,6 +227,57 @@ function deferred<T>() {
|
||||
return { promise, resolve };
|
||||
}
|
||||
|
||||
describe("Codex file-change approvals", () => {
|
||||
test("file-change approval includes the matching patch when notifications are still queued", async () => {
|
||||
const appServer = createFakeCodexAppServer();
|
||||
const { session } = await startPublicSteeringSession(appServer);
|
||||
try {
|
||||
for (const [id, filePath, diff] of [
|
||||
["file-alpha", "/workspace/project/alpha.txt", "@@ -1 +1 @@\n-blue\n+green\n"],
|
||||
["file-beta", "/workspace/project/beta.txt", "@@ -1 +1 @@\n-small\n+large\n"],
|
||||
]) {
|
||||
appServer.startsFileChange({
|
||||
threadId: "thread-1",
|
||||
turnId: "native-A",
|
||||
callId: id,
|
||||
path: filePath,
|
||||
diff,
|
||||
});
|
||||
}
|
||||
const permission = waitForNextPermission(session);
|
||||
appServer.requestFileChangeApproval({
|
||||
itemId: "file-alpha",
|
||||
threadId: "thread-1",
|
||||
turnId: "native-A",
|
||||
reason: "Apply the patch",
|
||||
});
|
||||
|
||||
expect((await permission).request).toEqual({
|
||||
id: "permission-file-alpha",
|
||||
provider: "codex",
|
||||
name: "CodexFileChange",
|
||||
kind: "tool",
|
||||
title: "Apply file changes: alpha.txt",
|
||||
description: "Apply the patch",
|
||||
detail: {
|
||||
type: "edit",
|
||||
filePath: "alpha.txt",
|
||||
unifiedDiff: "@@ -1 +1 @@\n-blue\n+green\n",
|
||||
},
|
||||
metadata: { itemId: "file-alpha", threadId: "thread-1", turnId: "native-A" },
|
||||
});
|
||||
|
||||
await session.respondToPermission("permission-file-alpha", { behavior: "allow" });
|
||||
await expect(appServer.waitForApprovalDecision("file-alpha")).resolves.toEqual({
|
||||
decision: "accept",
|
||||
});
|
||||
appServer.assertNoErrors();
|
||||
} finally {
|
||||
await session.close();
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
describe("Codex active-turn steering admission", () => {
|
||||
test("steers an accepted turn whose native id arrives after startTurn returns", async () => {
|
||||
const appServer = createFakeCodexAppServer({
|
||||
|
||||
@@ -3483,6 +3483,7 @@ export class CodexAppServerAgentSession implements AgentSession {
|
||||
private pendingReasoning = new Map<string, string[]>();
|
||||
private pendingCommandOutputDeltas = new Map<string, string[]>();
|
||||
private pendingFileChangeOutputDeltas = new Map<string, string[]>();
|
||||
private pendingToolCallDetails = new Map<string, ToolCallTimelineItem["detail"]>();
|
||||
private pendingAssistantMessageBoundary = false;
|
||||
private terminalCommandByProcessId = new Map<string, string>();
|
||||
// The root thread's still-running command process ids, keyed by the native turn that
|
||||
@@ -5251,6 +5252,7 @@ export class CodexAppServerAgentSession implements AgentSession {
|
||||
async close(): Promise<void> {
|
||||
this.closed = true;
|
||||
this.clearPendingPermissions();
|
||||
this.pendingToolCallDetails.clear();
|
||||
this.pendingSubAgentNotificationsByThreadId.clear();
|
||||
this.subscribers.clear();
|
||||
this.startupEvents = null;
|
||||
@@ -6416,6 +6418,7 @@ export class CodexAppServerAgentSession implements AgentSession {
|
||||
this.pendingReasoning.clear();
|
||||
this.pendingCommandOutputDeltas.clear();
|
||||
this.pendingFileChangeOutputDeltas.clear();
|
||||
this.pendingToolCallDetails.clear();
|
||||
this.pendingAssistantMessageBoundary = false;
|
||||
this.warnedIncompleteEditToolCallIds.clear();
|
||||
this.pendingRootCompactionItemIds.clear();
|
||||
@@ -6789,6 +6792,9 @@ export class CodexAppServerAgentSession implements AgentSession {
|
||||
if (shouldIgnoreMirroredLifecycleItem(parsed.source, parsed.item)) {
|
||||
return;
|
||||
}
|
||||
if (parsed.item.id) {
|
||||
this.pendingToolCallDetails.delete(parsed.item.id);
|
||||
}
|
||||
this.receiveAsyncQuestion(parsed.threadId, parsed.item);
|
||||
if (this.isUserMessageItem(parsed.item)) {
|
||||
await this.handleUserMessageItem(parsed);
|
||||
@@ -6836,10 +6842,8 @@ export class CodexAppServerAgentSession implements AgentSession {
|
||||
if (timelineItem.type === "assistant_message") {
|
||||
this.pendingAssistantMessageBoundary = true;
|
||||
}
|
||||
if (itemId) {
|
||||
this.emittedItemCompletedIds.add(itemId);
|
||||
this.emittedItemStartedIds.delete(itemId);
|
||||
}
|
||||
this.emittedItemCompletedIds.add(itemId);
|
||||
this.emittedItemStartedIds.delete(itemId);
|
||||
await this.replayPendingSubAgentNotifications(registeredChildThreadIds);
|
||||
return;
|
||||
}
|
||||
@@ -6875,7 +6879,7 @@ export class CodexAppServerAgentSession implements AgentSession {
|
||||
private consumeStreamedTextCompletion(
|
||||
timelineItem: AgentTimelineItem,
|
||||
itemId: string | null | undefined,
|
||||
): boolean {
|
||||
): itemId is string {
|
||||
if (!itemId) {
|
||||
return false;
|
||||
}
|
||||
@@ -6984,6 +6988,7 @@ export class CodexAppServerAgentSession implements AgentSession {
|
||||
if (!timelineItem || timelineItem.type !== "tool_call") {
|
||||
return;
|
||||
}
|
||||
this.pendingToolCallDetails.set(timelineItem.callId, timelineItem.detail);
|
||||
const registeredChildThreadIds = this.registerSubAgentToolCall({
|
||||
timelineItem,
|
||||
rawItem: parsed.item,
|
||||
@@ -7253,7 +7258,8 @@ export class CodexAppServerAgentSession implements AgentSession {
|
||||
});
|
||||
}
|
||||
|
||||
private handleFileChangeApprovalRequest(params: unknown): Promise<unknown> {
|
||||
private async handleFileChangeApprovalRequest(params: unknown): Promise<unknown> {
|
||||
await this.pendingNotifications;
|
||||
const parsed = z
|
||||
.object({
|
||||
itemId: z.string(),
|
||||
@@ -7263,20 +7269,25 @@ export class CodexAppServerAgentSession implements AgentSession {
|
||||
})
|
||||
.parse(params);
|
||||
const requestId = `permission-${parsed.itemId}`;
|
||||
const detail: ToolCallTimelineItem["detail"] = this.pendingToolCallDetails.get(
|
||||
parsed.itemId,
|
||||
) ?? {
|
||||
type: "unknown",
|
||||
input: { reason: parsed.reason ?? null },
|
||||
output: null,
|
||||
};
|
||||
const title =
|
||||
detail.type === "edit" && detail.filePath
|
||||
? `Apply file changes: ${detail.filePath}`
|
||||
: "Apply file changes";
|
||||
const request: AgentPermissionRequest = {
|
||||
id: requestId,
|
||||
provider: CODEX_PROVIDER,
|
||||
name: "CodexFileChange",
|
||||
kind: "tool",
|
||||
title: "Apply file changes",
|
||||
title,
|
||||
description: parsed.reason ?? undefined,
|
||||
detail: {
|
||||
type: "unknown",
|
||||
input: {
|
||||
reason: parsed.reason ?? null,
|
||||
},
|
||||
output: null,
|
||||
},
|
||||
detail,
|
||||
metadata: {
|
||||
itemId: parsed.itemId,
|
||||
threadId: parsed.threadId,
|
||||
|
||||
@@ -43,6 +43,13 @@ interface FakeTerminalInput {
|
||||
processId: string;
|
||||
text: string;
|
||||
}
|
||||
interface FakeFileChange {
|
||||
threadId: string;
|
||||
turnId: string;
|
||||
callId: string;
|
||||
path: string;
|
||||
diff: string;
|
||||
}
|
||||
interface FakeLegacyPatch {
|
||||
threadId: string;
|
||||
callId: string;
|
||||
@@ -88,6 +95,7 @@ export interface FakeCodexAppServer {
|
||||
completesSubAgentActivity(params: FakeSubAgentActivity): void;
|
||||
completesCompaction(params: { threadId: string; itemId: string }): void;
|
||||
runsLegacyCommand(params: FakeLegacyCommand): void;
|
||||
startsFileChange(params: FakeFileChange): void;
|
||||
appliesLegacyPatch(params: FakeLegacyPatch): void;
|
||||
completesCommand(params: FakeLegacyCommand): void;
|
||||
completesSilentCommand(params: FakeSilentCommand): void;
|
||||
@@ -467,6 +475,20 @@ export function createFakeCodexAppServer(
|
||||
success: true,
|
||||
});
|
||||
},
|
||||
startsFileChange(params) {
|
||||
writeNotification("item/started", {
|
||||
threadId: params.threadId,
|
||||
turnId: params.turnId,
|
||||
item: {
|
||||
type: "fileChange",
|
||||
id: params.callId,
|
||||
status: "inProgress",
|
||||
changes: [
|
||||
{ path: params.path, kind: { type: "update", move_path: null }, diff: params.diff },
|
||||
],
|
||||
},
|
||||
});
|
||||
},
|
||||
appliesLegacyPatch(params) {
|
||||
const changes = [
|
||||
{
|
||||
|
||||
Reference in New Issue
Block a user