From b545c94041017d000e2c8b2f6272705d21b85dfb Mon Sep 17 00:00:00 2001
From: jif <jif@openai.com>
Date: Thu, 30 Jul 2026 12:19:50 +0000
Subject: [PATCH] Strengthen session file change approval coverage (#36149)

## Why

The notification helper buffers unrelated server requests, so the session-scoped
file change approval test could overlook an unexpected second approval request.

## What changed

- Consume approval resolution and file change completion events directly and
  verify their request, thread, turn, and item identities.
- Fail if the second patch requests approval after the first patch was accepted
  for the session.
- Shut down the app server gracefully and assert that it exits successfully.

GitOrigin-RevId: 53a3f746940677043e097094862c28faf4db1a8d
---
 .../app-server/tests/suite/v2/turn_start.rs   | 72 +++++++++++++++----
 1 file changed, 60 insertions(+), 12 deletions(-)

diff --git a/codex-rs/app-server/tests/suite/v2/turn_start.rs b/codex-rs/app-server/tests/suite/v2/turn_start.rs
index a8a56fbb8b..f977003877 100644
--- a/codex-rs/app-server/tests/suite/v2/turn_start.rs
+++ b/codex-rs/app-server/tests/suite/v2/turn_start.rs
@@ -3779,6 +3779,7 @@ async fn turn_start_file_change_approval_accept_for_session_persists_v2() -> Res
     assert_eq!(params.thread_id, thread.id);
     assert_eq!(params.turn_id, turn_1.id);
 
+    let resolved_request_id = request_id.clone();
     mcp.send_response(
         request_id,
         serde_json::to_value(FileChangeRequestApprovalResponse {
@@ -3787,11 +3788,36 @@ async fn turn_start_file_change_approval_accept_for_session_persists_v2() -> Res
     )
     .await?;
 
-    timeout(
-        DEFAULT_READ_TIMEOUT,
-        mcp.read_stream_until_notification_message("item/completed"),
-    )
-    .await??;
+    let mut approval_resolved = false;
+    let mut patch_completed = false;
+    while !approval_resolved || !patch_completed {
+        let message = timeout(DEFAULT_READ_TIMEOUT, mcp.read_next_message()).await??;
+        let JSONRPCMessage::Notification(notification) = message else {
+            continue;
+        };
+        match notification.method.as_str() {
+            "serverRequest/resolved" => {
+                let resolved: ServerRequestResolvedNotification = serde_json::from_value(
+                    notification.params.expect("serverRequest/resolved params"),
+                )?;
+                if resolved.request_id == resolved_request_id {
+                    assert_eq!(resolved.thread_id, thread.id);
+                    approval_resolved = true;
+                }
+            }
+            "item/completed" => {
+                let completed: ItemCompletedNotification =
+                    serde_json::from_value(notification.params.expect("item/completed params"))?;
+                if matches!(completed.item, ThreadItem::FileChange { ref id, .. } if id == "patch-call-1")
+                {
+                    assert_eq!(completed.thread_id, thread.id);
+                    assert_eq!(completed.turn_id, turn_1.id);
+                    patch_completed = true;
+                }
+            }
+            _ => {}
+        }
+    }
     timeout(
         DEFAULT_READ_TIMEOUT,
         mcp.read_stream_until_notification_message("turn/completed"),
@@ -3802,7 +3828,7 @@ async fn turn_start_file_change_approval_accept_for_session_persists_v2() -> Res
     assert_eq!(std::fs::read_to_string(&readme_path)?, "new line\n");
 
     // Second turn: apply a patch to the same file. Approval should be skipped due to AcceptForSession.
-    let _: TurnStartResponse = mcp
+    let TurnStartResponse { turn: turn_2 } = mcp
         .request(|request_id| ClientRequest::TurnStart {
             request_id,
             params: TurnStartParams {
@@ -3833,13 +3859,30 @@ async fn turn_start_file_change_approval_accept_for_session_persists_v2() -> Res
     assert_eq!(id, "patch-call-2");
     assert_eq!(status, PatchApplyStatus::InProgress);
 
-    // If the server incorrectly emits FileChangeRequestApproval, the helper below will error
-    // (it bails on unexpected JSONRPCMessage::Request), causing the test to fail.
-    timeout(
-        DEFAULT_READ_TIMEOUT,
-        mcp.read_stream_until_notification_message("item/completed"),
-    )
+    let completed_file_change = timeout(DEFAULT_READ_TIMEOUT, async {
+        loop {
+            match mcp.read_next_message().await? {
+                JSONRPCMessage::Request(request) => {
+                    anyhow::bail!("unexpected approval request for session-approved patch: {request:?}");
+                }
+                JSONRPCMessage::Notification(notification)
+                    if notification.method == "item/completed" =>
+                {
+                    let completed: ItemCompletedNotification = serde_json::from_value(
+                        notification.params.expect("item/completed params"),
+                    )?;
+                    if matches!(completed.item, ThreadItem::FileChange { ref id, .. } if id == "patch-call-2")
+                    {
+                        return Ok::<ItemCompletedNotification, anyhow::Error>(completed);
+                    }
+                }
+                _ => {}
+            }
+        }
+    })
     .await??;
+    assert_eq!(completed_file_change.thread_id, thread.id);
+    assert_eq!(completed_file_change.turn_id, turn_2.id);
     timeout(
         DEFAULT_READ_TIMEOUT,
         mcp.read_stream_until_notification_message("turn/completed"),
@@ -3847,6 +3890,11 @@ async fn turn_start_file_change_approval_accept_for_session_persists_v2() -> Res
     .await??;
 
     assert_eq!(std::fs::read_to_string(readme_path)?, "updated line\n");
+    let status = timeout(DEFAULT_READ_TIMEOUT, mcp.shutdown_gracefully()).await??;
+    anyhow::ensure!(
+        status.success(),
+        "app-server exited unsuccessfully: {status}"
+    );
 
     Ok(())
 }