authorgravatar for 114993336+walid-baharwal@users.noreply.github.comMuhammad Waleed <114993336+walid-baharwal@users.noreply.github.com> 2026-09-05 07:16:04+05:00
committergravatar for noreply@github.comGitHub <noreply@github.com> 2026-09-04 19:16:04-07:00
log57a6b70e24221f00c9a161f1e5b6b2ddd753bc8f
tree2557b5dc1e5c2f5d41bbf1fb6a1debc349b2fab3
parentce4712d5b04fb998f79fe132245289191147e5d5
signature Signed by PGP key B5690EEEBB952194

fix(codex): show what a file-change approval will change (#8669)

Co-authored-by: Julius Marminge <51714798+juliusmarminge@users.noreply.github.com>

2 files changed, 245 insertions(+), 2 deletions(-)

apps/server/src/provider/Layers/CodexAdapter.test.ts+207
...@@ -1877,6 +1877,213 @@ lifecycleLayer("CodexAdapterLive lifecycle", (it) => {...@@ -1877,6 +1877,213 @@ lifecycleLayer("CodexAdapterLive lifecycle", (it) => {
1877 }),1877 }),
1878 );1878 );
18791879
1880 it.effect("names the edited files in an apply-patch approval without a reason", () =>
1881 Effect.gen(function* () {
1882 const { adapter, runtime } = yield* startLifecycleRuntime();
1883 const firstEventFiber = yield* Stream.runHead(adapter.streamEvents).pipe(Effect.forkChild);
1884
1885 yield* runtime.emit({
1886 id: asEventId("evt-apply-patch"),
1887 kind: "request",
1888 provider: ProviderDriverKind.make("codex"),
1889 threadId: asThreadId("thread-1"),
1890 createdAt: "2026-08-24T00:00:00.000Z",
1891 method: "applyPatchApproval",
1892 requestKind: "file-change",
1893 requestId: ApprovalRequestId.make("req-patch"),
1894 turnId: asTurnId("turn-1"),
1895 payload: {
1896 callId: "call-1",
1897 conversationId: "provider-thread-1",
1898 fileChanges: {
1899 "/tmp/removed.md": { type: "delete", content: "gone" },
1900 "/tmp/added.ts": { type: "add", content: "export {};" },
1901 },
1902 },
1903 } satisfies ProviderEvent);
1904
1905 const firstEvent = yield* Fiber.join(firstEventFiber);
1906
1907 NodeAssert.equal(firstEvent._tag, "Some");
1908 if (firstEvent._tag !== "Some" || firstEvent.value.type !== "request.opened") {
1909 return;
1910 }
1911 NodeAssert.equal(firstEvent.value.payload.requestType, "apply_patch_approval");
1912 NodeAssert.equal(
1913 firstEvent.value.payload.detail,
1914 "add /tmp/added.ts\ndelete /tmp/removed.md",
1915 );
1916 }),
1917 );
1918
1919 it.effect("keeps the reason when an apply-patch approval carries one", () =>
1920 Effect.gen(function* () {
1921 const { adapter, runtime } = yield* startLifecycleRuntime();
1922 const firstEventFiber = yield* Stream.runHead(adapter.streamEvents).pipe(Effect.forkChild);
1923
1924 yield* runtime.emit({
1925 id: asEventId("evt-apply-patch-reason"),
1926 kind: "request",
1927 provider: ProviderDriverKind.make("codex"),
1928 threadId: asThreadId("thread-1"),
1929 createdAt: "2026-08-24T00:00:00.000Z",
1930 method: "applyPatchApproval",
1931 requestKind: "file-change",
1932 requestId: ApprovalRequestId.make("req-patch-reason"),
1933 turnId: asTurnId("turn-1"),
1934 payload: {
1935 callId: "call-2",
1936 conversationId: "provider-thread-1",
1937 reason: "Needs to rewrite the changelog",
1938 fileChanges: { "/tmp/CHANGELOG.md": { type: "add", content: "x" } },
1939 },
1940 } satisfies ProviderEvent);
1941
1942 const firstEvent = yield* Fiber.join(firstEventFiber);
1943
1944 NodeAssert.equal(firstEvent._tag, "Some");
1945 if (firstEvent._tag !== "Some" || firstEvent.value.type !== "request.opened") {
1946 return;
1947 }
1948 NodeAssert.equal(firstEvent.value.payload.detail, "Needs to rewrite the changelog");
1949 }),
1950 );
1951
1952 it.effect("falls back to the grant root for a file-change approval without a reason", () =>
1953 Effect.gen(function* () {
1954 const { adapter, runtime } = yield* startLifecycleRuntime();
1955 const firstEventFiber = yield* Stream.runHead(adapter.streamEvents).pipe(Effect.forkChild);
1956
1957 yield* runtime.emit({
1958 id: asEventId("evt-file-change"),
1959 kind: "request",
1960 provider: ProviderDriverKind.make("codex"),
1961 threadId: asThreadId("thread-1"),
1962 createdAt: "2026-08-24T00:00:00.000Z",
1963 method: "item/fileChange/requestApproval",
1964 requestKind: "file-change",
1965 requestId: ApprovalRequestId.make("req-file-change"),
1966 turnId: asTurnId("turn-1"),
1967 payload: {
1968 itemId: "item-1",
1969 grantRoot: "/tmp/workspace",
1970 startedAtMs: 0,
1971 threadId: "provider-thread-1",
1972 turnId: "turn-1",
1973 },
1974 } satisfies ProviderEvent);
1975
1976 const firstEvent = yield* Fiber.join(firstEventFiber);
1977
1978 NodeAssert.equal(firstEvent._tag, "Some");
1979 if (firstEvent._tag !== "Some" || firstEvent.value.type !== "request.opened") {
1980 return;
1981 }
1982 NodeAssert.equal(firstEvent.value.payload.requestType, "file_change_approval");
1983 NodeAssert.equal(firstEvent.value.payload.detail, "/tmp/workspace");
1984 }),
1985 );
1986
1987 it.effect("prefers the edited files over a blank apply-patch reason", () =>
1988 Effect.gen(function* () {
1989 const { adapter, runtime } = yield* startLifecycleRuntime();
1990 const firstEventFiber = yield* Stream.runHead(adapter.streamEvents).pipe(Effect.forkChild);
1991
1992 yield* runtime.emit({
1993 id: asEventId("evt-apply-patch-blank"),
1994 kind: "request",
1995 provider: ProviderDriverKind.make("codex"),
1996 threadId: asThreadId("thread-1"),
1997 createdAt: "2026-08-24T00:00:00.000Z",
1998 method: "applyPatchApproval",
1999 requestKind: "file-change",
2000 requestId: ApprovalRequestId.make("req-patch-blank"),
2001 turnId: asTurnId("turn-1"),
2002 payload: {
2003 callId: "call-3",
2004 conversationId: "provider-thread-1",
2005 reason: " ",
2006 fileChanges: {
2007 "/tmp/moved.ts": { type: "update", unified_diff: "@@", move_path: "/tmp/renamed.ts" },
2008 },
2009 },
2010 } satisfies ProviderEvent);
2011
2012 const firstEvent = yield* Fiber.join(firstEventFiber);
2013
2014 NodeAssert.equal(firstEvent._tag, "Some");
2015 if (firstEvent._tag !== "Some" || firstEvent.value.type !== "request.opened") {
2016 return;
2017 }
2018 NodeAssert.equal(firstEvent.value.payload.detail, "update /tmp/moved.ts -> /tmp/renamed.ts");
2019 }),
2020 );
2021
2022 it.effect("caps the described files in an oversized apply-patch approval", () =>
2023 Effect.gen(function* () {
2024 const { adapter, runtime } = yield* startLifecycleRuntime();
2025 const firstEventFiber = yield* Stream.runHead(adapter.streamEvents).pipe(Effect.forkChild);
2026
2027 const fileChanges = Object.fromEntries(
2028 Array.from({ length: 25 }, (_unused, index) => [
2029 `/tmp/file-${String(index).padStart(2, "0")}.ts`,
2030 { type: "add", content: "x" },
2031 ]),
2032 );
2033
2034 yield* runtime.emit({
2035 id: asEventId("evt-apply-patch-many"),
2036 kind: "request",
2037 provider: ProviderDriverKind.make("codex"),
2038 threadId: asThreadId("thread-1"),
2039 createdAt: "2026-08-24T00:00:00.000Z",
2040 method: "applyPatchApproval",
2041 requestKind: "file-change",
2042 requestId: ApprovalRequestId.make("req-patch-many"),
2043 turnId: asTurnId("turn-1"),
2044 payload: { callId: "call-4", conversationId: "provider-thread-1", fileChanges },
2045 } satisfies ProviderEvent);
2046
2047 const firstEvent = yield* Fiber.join(firstEventFiber);
2048
2049 NodeAssert.equal(firstEvent._tag, "Some");
2050 if (firstEvent._tag !== "Some" || firstEvent.value.type !== "request.opened") {
2051 return;
2052 }
2053 const detail = firstEvent.value.payload.detail ?? "";
2054 NodeAssert.equal(detail.split("\n").length, 21);
2055 NodeAssert.ok(detail.endsWith("+5 more"));
2056 }),
2057 );
2058
2059 it.effect("leaves an apply-patch approval without changes or a reason undetailed", () =>
2060 Effect.gen(function* () {
2061 const { adapter, runtime } = yield* startLifecycleRuntime();
2062 const firstEventFiber = yield* Stream.runHead(adapter.streamEvents).pipe(Effect.forkChild);
2063
2064 yield* runtime.emit({
2065 id: asEventId("evt-apply-patch-empty"),
2066 kind: "request",
2067 provider: ProviderDriverKind.make("codex"),
2068 threadId: asThreadId("thread-1"),
2069 createdAt: "2026-08-24T00:00:00.000Z",
2070 method: "applyPatchApproval",
2071 requestKind: "file-change",
2072 requestId: ApprovalRequestId.make("req-patch-empty"),
2073 turnId: asTurnId("turn-1"),
2074 payload: { callId: "call-5", conversationId: "provider-thread-1", fileChanges: {} },
2075 } satisfies ProviderEvent);
2076
2077 const firstEvent = yield* Fiber.join(firstEventFiber);
2078
2079 NodeAssert.equal(firstEvent._tag, "Some");
2080 if (firstEvent._tag !== "Some" || firstEvent.value.type !== "request.opened") {
2081 return;
2082 }
2083 NodeAssert.equal(firstEvent.value.payload.detail, undefined);
2084 }),
2085 );
2086
1880 it.effect("maps MCP elicitation requests into app access approvals", () =>2087 it.effect("maps MCP elicitation requests into app access approvals", () =>
1881 Effect.gen(function* () {2088 Effect.gen(function* () {
1882 const { adapter, runtime } = yield* startLifecycleRuntime();2089 const { adapter, runtime } = yield* startLifecycleRuntime();
apps/server/src/provider/Layers/CodexAdapter.ts+38-2
...@@ -787,6 +787,36 @@ function itemDetail(itemType: CanonicalItemType, item: CodexLifecycleItem): stri...@@ -787,6 +787,36 @@ function itemDetail(itemType: CanonicalItemType, item: CodexLifecycleItem): stri
787 return undefined;787 return undefined;
788}788}
789789
790// Codex sends `reason` only sometimes, and sends it blank rather than absent
791// often enough to matter, so an empty one must not outrank the paths below.
792function nonEmptyDetail(value: string | null | undefined): string | undefined {
793 const trimmed = value?.trim();
794 return trimmed === undefined || trimmed.length === 0 ? undefined : trimmed;
795}
796
797// Keeps one oversized patch from pushing a wall of paths through every consumer
798// of the approval, while still saying how much it covers.
799const MAX_DESCRIBED_FILE_CHANGES = 20;
800
801// An apply-patch approval carries the edited paths as the keys of `fileChanges`.
802// Without them the approval card has nothing to show but its own title — the
803// command-execution branch already falls back to the command for the same reason.
804function describeFileChanges(
805 fileChanges: EffectCodexSchema.ServerRequest__ApplyPatchApprovalParams["fileChanges"] | undefined,
806): string | undefined {
807 if (fileChanges === undefined) return undefined;
808 const entries = Object.entries(fileChanges).toSorted(([left], [right]) =>
809 left.localeCompare(right),
810 );
811 if (entries.length === 0) return undefined;
812 const described = entries.slice(0, MAX_DESCRIBED_FILE_CHANGES).map(([path, change]) => {
813 const movePath = change.type === "update" ? change.move_path : undefined;
814 return movePath ? `${change.type} ${path} -> ${movePath}` : `${change.type} ${path}`;
815 });
816 const remaining = entries.length - described.length;
817 return remaining > 0 ? `${described.join("\n")}\n+${remaining} more` : described.join("\n");
818}
819
790function toRequestTypeFromMethod(method: string): CanonicalRequestType {820function toRequestTypeFromMethod(method: string): CanonicalRequestType {
791 switch (method) {821 switch (method) {
792 case "item/commandExecution/requestApproval":822 case "item/commandExecution/requestApproval":
...@@ -1325,7 +1355,9 @@ function mapToRuntimeEvents(...@@ -1325,7 +1355,9 @@ function mapToRuntimeEvents(
1325 EffectCodexSchema.ServerRequest__FileChangeRequestApprovalParams,1355 EffectCodexSchema.ServerRequest__FileChangeRequestApprovalParams,
1326 event.payload,1356 event.payload,
1327 );1357 );
1328 return payload?.reason ?? undefined;1358 // These params carry no path of their own, only the root the agent
1359 // wants to write under.
1360 return nonEmptyDetail(payload?.reason) ?? nonEmptyDetail(payload?.grantRoot);
1329 }1361 }
1330 case "mcpServer/elicitation/request":1362 case "mcpServer/elicitation/request":
1331 return elicitation?.message;1363 return elicitation?.message;
...@@ -1334,7 +1366,11 @@ function mapToRuntimeEvents(...@@ -1334,7 +1366,11 @@ function mapToRuntimeEvents(
1334 EffectCodexSchema.ServerRequest__ApplyPatchApprovalParams,1366 EffectCodexSchema.ServerRequest__ApplyPatchApprovalParams,
1335 event.payload,1367 event.payload,
1336 );1368 );
1337 return payload?.reason ?? undefined;1369 return (
1370 nonEmptyDetail(payload?.reason) ??
1371 describeFileChanges(payload?.fileChanges) ??
1372 nonEmptyDetail(payload?.grantRoot)
1373 );
1338 }1374 }
1339 case "execCommandApproval": {1375 case "execCommandApproval": {
1340 const payload = readPayload(1376 const payload = readPayload(