Skip to content

Commit 07f849a

Browse files
rojiCopilot
andauthored
Fix flaky permission E2E assertions (#1827)
Assert final permission tool lifecycle from persisted session events instead of racing live event callbacks after turn completion. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 40ae242 commit 07f849a

1 file changed

Lines changed: 60 additions & 20 deletions

File tree

dotnet/test/E2E/PermissionE2ETests.cs

Lines changed: 60 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -377,7 +377,7 @@ void AddLifecycleEvent(string phase, string? toolCallId)
377377
}
378378
});
379379

380-
await session.SendAsync(new MessageOptions
380+
var sendTask = session.SendAndWaitAsync(new MessageOptions
381381
{
382382
Prompt = "Run 'echo slow_handler_test'"
383383
});
@@ -391,7 +391,13 @@ await session.SendAsync(new MessageOptions
391391

392392
releaseHandler.SetResult();
393393

394-
var message = await TestHelper.GetFinalAssistantMessageAsync(session);
394+
var message = await sendTask;
395+
var persistedEvents = await WaitForPersistedEventsAsync(
396+
session,
397+
events =>
398+
events.OfType<ToolExecutionStartEvent>().Any(evt => evt.Data.ToolCallId == targetToolId) &&
399+
events.OfType<ToolExecutionCompleteEvent>().Any(evt => evt.Data.ToolCallId == targetToolId),
400+
$"Timed out waiting for persisted tool lifecycle for tool call '{targetToolId}'.");
395401

396402
List<(string Phase, string? ToolCallId)> orderedLifecycle;
397403
lock (lifecycleLock)
@@ -401,20 +407,23 @@ await session.SendAsync(new MessageOptions
401407

402408
var permissionStartIndex = orderedLifecycle.FindIndex(evt => evt.Phase == "permission-start" && evt.ToolCallId == targetToolId);
403409
var permissionCompleteIndex = orderedLifecycle.FindIndex(evt => evt.Phase == "permission-complete" && evt.ToolCallId == targetToolId);
404-
var toolStartIndex = orderedLifecycle.FindIndex(evt => evt.Phase == "tool-start" && evt.ToolCallId == targetToolId);
405-
var toolCompleteIndex = orderedLifecycle.FindIndex(evt => evt.Phase == "tool-complete" && evt.ToolCallId == targetToolId);
406410
var observedLifecycle = string.Join(", ", orderedLifecycle.Select(evt => $"{evt.Phase}:{evt.ToolCallId}"));
411+
var toolStartIndex = persistedEvents.FindIndex(evt =>
412+
evt is ToolExecutionStartEvent started && started.Data.ToolCallId == targetToolId);
413+
var toolCompleteIndex = persistedEvents.FindIndex(evt =>
414+
evt is ToolExecutionCompleteEvent completed && completed.Data.ToolCallId == targetToolId);
415+
var observedPersistedEvents = string.Join(", ", persistedEvents.Select(DescribeEvent));
407416

408417
Assert.InRange(permissionStartIndex, 0, orderedLifecycle.Count - 1);
409418
Assert.InRange(permissionCompleteIndex, 0, orderedLifecycle.Count - 1);
410-
Assert.InRange(toolStartIndex, 0, orderedLifecycle.Count - 1);
411-
Assert.InRange(toolCompleteIndex, 0, orderedLifecycle.Count - 1);
412419
Assert.True(
413-
permissionCompleteIndex < toolCompleteIndex,
414-
$"Expected permission completion before target tool completion. Observed: {observedLifecycle}");
420+
permissionStartIndex < permissionCompleteIndex,
421+
$"Expected permission handler to complete after it started. Observed: {observedLifecycle}");
422+
Assert.InRange(toolStartIndex, 0, persistedEvents.Count - 1);
423+
Assert.InRange(toolCompleteIndex, 0, persistedEvents.Count - 1);
415424
Assert.True(
416425
toolStartIndex < toolCompleteIndex,
417-
$"Expected target tool start before target tool completion. Observed: {observedLifecycle}");
426+
$"Expected target tool start before target tool completion. Observed: {observedPersistedEvents}");
418427

419428
// The tool should have actually run after permission was granted
420429
Assert.Contains("slow_handler_test", message?.Data.Content ?? string.Empty);
@@ -573,24 +582,21 @@ public async Task Should_Short_Circuit_Permission_Handler_When_Set_Approve_All_E
573582

574583
try
575584
{
576-
var toolCompleted = new TaskCompletionSource<ToolExecutionCompleteEvent>(TaskCreationOptions.RunContinuationsAsynchronously);
577-
using var subscription = session.On<SessionEvent>(evt =>
578-
{
579-
if (evt is ToolExecutionCompleteEvent done && done.Data.Success)
580-
{
581-
toolCompleted.TrySetResult(done);
582-
}
583-
});
584-
585585
await session.SendAndWaitAsync(new MessageOptions
586586
{
587587
Prompt = "Run 'echo test' and tell me what happens",
588588
});
589589

590-
// A real shell tool must have completed successfully under the runtime-level approval.
591-
await toolCompleted.Task.WaitAsync(TimeSpan.FromSeconds(30));
590+
var persistedEvents = await WaitForPersistedEventsAsync(
591+
session,
592+
events => events.OfType<ToolExecutionCompleteEvent>().Any(evt =>
593+
evt.Data.Success && ToolCompleteContains(evt, "test")),
594+
"Timed out waiting for persisted successful shell tool completion.");
592595

593596
Assert.Equal(0, Volatile.Read(ref handlerCallCount));
597+
Assert.Contains(
598+
persistedEvents.OfType<ToolExecutionCompleteEvent>(),
599+
evt => evt.Data.Success && ToolCompleteContains(evt, "test"));
594600
}
595601
finally
596602
{
@@ -758,6 +764,40 @@ private static bool PathsEqual(string expected, string actual)
758764
OperatingSystem.IsWindows() ? StringComparison.OrdinalIgnoreCase : StringComparison.Ordinal);
759765
}
760766

767+
private static async Task<List<SessionEvent>> WaitForPersistedEventsAsync(
768+
CopilotSession session,
769+
Func<List<SessionEvent>, bool> condition,
770+
string timeoutMessage)
771+
{
772+
List<SessionEvent> events = [];
773+
await TestHelper.WaitForConditionAsync(
774+
async () =>
775+
{
776+
events = (await session.GetEventsAsync()).ToList();
777+
return condition(events);
778+
},
779+
timeoutMessage: timeoutMessage);
780+
return events;
781+
}
782+
783+
private static string DescribeEvent(SessionEvent evt)
784+
=> evt switch
785+
{
786+
ToolExecutionStartEvent started => $"{evt.Type}:{started.Data.ToolCallId}",
787+
ToolExecutionCompleteEvent completed => $"{evt.Type}:{completed.Data.ToolCallId}:{completed.Data.Success}",
788+
_ => evt.Type,
789+
};
790+
791+
private static bool ToolCompleteContains(ToolExecutionCompleteEvent evt, string expected)
792+
=> evt.Data.Result?.Content.Contains(expected, StringComparison.OrdinalIgnoreCase) == true ||
793+
evt.Data.Result?.DetailedContent?.Contains(expected, StringComparison.OrdinalIgnoreCase) == true ||
794+
evt.Data.Result?.Contents?.Any(content => content switch
795+
{
796+
ToolExecutionCompleteContentText text => text.Text.Contains(expected, StringComparison.OrdinalIgnoreCase),
797+
ToolExecutionCompleteContentTerminal terminal => terminal.Text.Contains(expected, StringComparison.OrdinalIgnoreCase),
798+
_ => false,
799+
}) == true;
800+
761801
private static string NormalizePath(string path)
762802
{
763803
var fullPath = Path.GetFullPath(path);

0 commit comments

Comments
 (0)