fix(channels): prevent cold approval path from swallowing normal chat (#1164)#1165
Merged
Aaronontheweb merged 2 commits intoMay 25, 2026
Merged
Conversation
The cold-text-approval path matches short replies like 'yes', 'a', '1' as potential approval responses regardless of context. When no pending approval exists and the session has never had an approval request, the message was consumed and a spurious 'approval prompt expired' notice was emitted. Check _resolvedToolApprovals and history for redrivable tool batches before emitting the expired notice. If no approval history exists, return approval_no_history so the channel falls through to normal LLM ingress. Fixes netclaw-dev#1164
Aaronontheweb
added
channels
Aaronontheweb
commented
May 25, 2026
| 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()); |
Collaborator
Author
There was a problem hiding this comment.
let all content through the prompt injection detector
Aaronontheweb
commented
May 25, 2026
| ResponseFactory = (feedback, _) => | ||
| { | ||
| return feedback is ToolInteractionTextResponse | ||
| ? Task.FromResult<ICommandReply>(CommandNack.For(sid, "approval_no_history")) |
Collaborator
Author
There was a problem hiding this comment.
send a CommandNack back if the feedback is part of a tool text interaction, i.e. sending A/B/C/D instead of clicking a button for approval signaling.
Aaronontheweb
commented
May 25, 2026
| // 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.
"yes" is a keyword in the text matching pipeline apparently? We should double check that - that was not my understanding.
Aaronontheweb
commented
May 25, 2026
| { | ||
| _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.
needs to be a const string if it has semantic meaning inside the application.
Aaronontheweb
commented
May 25, 2026
| // 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.
Same as above.
Aaronontheweb
commented
May 25, 2026
| // 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.
These reasons either need to be an enum or a constant.
Aaronontheweb
commented
May 25, 2026
| // 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.
Same as the other comments about CommandNack reasons.
- Add ApprovalNackReasons static class with const string values to eliminate magic strings across session actor, channel bindings, and HTTP endpoints - Replace all inline approval_* strings with ApprovalNackReasons constants - Update MattermostActionEndpointExtensions to use constants in switch expression - Test now sends CommandNack for ToolInteractionTextResponse to simulate real session behavior when no approval history exists - Confirmed 'yes' is a keyword matched by LooksLikeApprovalResponse via TryParseNamedSelection Fixes netclaw-dev#1164
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
HasApprovalHistorycheck inLlmSessionActor.TryResolveTextApprovalResponse— when no approval has ever been requested, returnapproval_no_historynack instead of emitting an expired noticeapproval_no_historyas "not consumed" so the message falls through to normal LLM ingressProblem
LooksLikeApprovalResponsematches single lettersa-eand numbers1-5by design. When the session has never had an approval request, this produced a false positive: normal chat like "yes" or "a" was consumed by the cold approval path and a phantom "approval prompt expired" notice appeared to the user.Changes
LlmSessionActor.csHasApprovalHistoryproperty +approval_no_historynack pathSlackThreadBindingActor.csapproval_no_historyas not consumedDiscordSessionBindingActor.csMattermostSessionBindingActor.csSessionBindingContractTests.csNormal_chat_text_that_looks_like_approval_is_not_consumed_when_no_approval_historyRecordingSessionPipeline.csResponseFactoryfor scripted nack responses in testsTesting
Fixes #1164