-
Notifications
You must be signed in to change notification settings - Fork 20
fix(channels): prevent cold approval path from swallowing normal chat (#1164) #1165
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -480,6 +480,51 @@ await AwaitAssertAsync(() => | |
| }, cancellationToken: ct); | ||
| } | ||
|
|
||
| // Regression for #1164: when no approval has ever been requested in the session, | ||
| // a short message like "yes", "a", or "1" should NOT be consumed by the cold | ||
| // approval path. The message must fall through to normal LLM ingress. | ||
| [Fact] | ||
| public async Task Normal_chat_text_that_looks_like_approval_is_not_consumed_when_no_approval_history() | ||
| { | ||
| var ct = TestContext.Current.CancellationToken; | ||
| var detector = new ConfigurablePromptInjectionDetector(PromptInjectionResult.Safe()); | ||
| var sid = new SessionId("session-cold-text-false-positive"); | ||
|
|
||
| // Empty output stream: the binding never observed an approval prompt, | ||
| // so _hasObservedApprovalRequest stays false and the cold path is active. | ||
| // The ResponseFactory simulates the session rejecting the cold-path | ||
| // response with approval_no_history (meaning: no approval ever existed). | ||
| var pipeline = new RecordingSessionPipeline(_ => []) | ||
| { | ||
| ResponseFactory = (feedback, _) => | ||
| { | ||
| return feedback is ToolInteractionTextResponse | ||
| ? Task.FromResult<ICommandReply>(CommandNack.For(sid, "approval_no_history")) | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. send a |
||
| : Task.FromResult<ICommandReply>(CommandAck.For(feedback.SessionId)); | ||
| } | ||
| }; | ||
|
|
||
| var actor = CreateBindingActorWithPipeline(sid, pipeline, detector); | ||
|
|
||
| // Send a message that LooksLikeApprovalResponse matches ("yes" -> ApproveOnce) | ||
| // but is ordinary conversation. With the fix, the message falls through | ||
| // to normal ChannelInput ingestion. | ||
| actor.Tell(CreateInboundMessage("yes", "user-1"), TestActor); | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
|
||
| await AwaitAssertAsync(() => | ||
| { | ||
| // The cold path should have forwarded the message to the session | ||
| Assert.Single(pipeline.RecordedFeedback.OfType<ToolInteractionTextResponse>()); | ||
|
|
||
| // The message should NOT be consumed — it must fall through to normal input | ||
| Assert.NotEmpty(pipeline.CapturedInputs); | ||
| Assert.True( | ||
| pipeline.CapturedInputs.Any(ci => | ||
| ci.Contents.Any(c => c is TextContent tc && tc.Text == "yes")), | ||
| "The original message text should appear in ChannelInput"); | ||
| }, cancellationToken: ct); | ||
| } | ||
|
|
||
| // Regression for the silent-drop class of bugs: the binding observes a | ||
| // ToolInteractionRequest then a TurnCompleted (which clears its local | ||
| // _pendingApprovalRequests). A button click arriving afterwards must still | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3262,6 +3262,10 @@ private void EmitUsageOutput(UsageDetails usage) | |
| _ => ApprovalDecision.Denied | ||
| }; | ||
|
|
||
| private bool HasApprovalHistory | ||
| => _resolvedToolApprovals.Count > 0 | ||
| || ParkedToolBatchHistory.FindRedrivableAssistantMessage(_state.History, null) is not null; | ||
|
|
||
| /// <summary> | ||
| /// Emits the channel-visible "approval prompt expired" notice. Used when a | ||
| /// tool interaction response cannot be honored — fail loud instead of | ||
|
|
@@ -3378,9 +3382,21 @@ private bool TryResolveTextApprovalResponse( | |
| { | ||
| if (_pendingToolInteractions.Count == 0) | ||
| { | ||
| _log.Warning("Ignoring text tool interaction response with no pending approvals for sender {SenderId}", msg.SenderId); | ||
| EmitExpiredPromptNotice(); | ||
| nackReason = "approval_prompt_expired"; | ||
| if (HasApprovalHistory) | ||
| { | ||
| _log.Warning("Ignoring text tool interaction response with no pending approvals for sender {SenderId}", msg.SenderId); | ||
| EmitExpiredPromptNotice(); | ||
| nackReason = "approval_prompt_expired"; | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. needs to be a |
||
| } | ||
| else | ||
| { | ||
| // Session has never had an approval request. The channel cold path | ||
| // matched the text as approval-like, but this is almost certainly | ||
| // ordinary conversation (e.g., "yes", "a", "1"). Don't emit a | ||
| // user-visible notice and don't consume — the channel should | ||
| // fall through to normal LLM ingress. See #1164. | ||
| nackReason = "approval_no_history"; | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as above. |
||
| } | ||
| } | ||
| else | ||
| { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -785,6 +785,12 @@ private async Task<bool> TryHandleColdTextApprovalResponseAsync(DiscordThreadInb | |
| return true; | ||
| } | ||
|
|
||
| // approval_no_history means the session has never had an approval request. | ||
| // The message was a false-positive from LooksLikeApprovalResponse. | ||
| // Don't consume — let it fall through to normal LLM ingress. See #1164. | ||
| if (reply is CommandNack { Reason: "approval_no_history" }) | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These reasons either need to be an enum or a constant. |
||
| return false; | ||
|
|
||
| return reply is CommandNack; | ||
| } | ||
| catch (Exception ex) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1304,6 +1304,12 @@ private async Task<bool> TryHandleColdTextApprovalResponseAsync(SlackThreadInbou | |
| return true; | ||
| } | ||
|
|
||
| // approval_no_history means the session has never had an approval request. | ||
| // The message was a false-positive from LooksLikeApprovalResponse. | ||
| // Don't consume — let it fall through to normal LLM ingress. See #1164. | ||
| if (reply is CommandNack { Reason: "approval_no_history" }) | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as the other comments about |
||
| return false; | ||
|
|
||
| return reply is CommandNack; | ||
| } | ||
| catch (Exception ex) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let all content through the prompt injection detector