diff --git a/openspec/changes/consolidate-binding-actor-engines/specs/channel-binding-parity/spec.md b/openspec/changes/consolidate-binding-actor-engines/specs/channel-binding-parity/spec.md index c3736c75f..a81c56efa 100644 --- a/openspec/changes/consolidate-binding-actor-engines/specs/channel-binding-parity/spec.md +++ b/openspec/changes/consolidate-binding-actor-engines/specs/channel-binding-parity/spec.md @@ -32,7 +32,7 @@ The shared engine SHALL store cursors as strings, which matches the persisted `C ### Requirement: Shared approval-response flow -The system SHALL implement text-approval parsing, cold-spawn approval forwarding, and pending-prompt resolution in a single shared flow. The requester identity check SHALL execute inside the shared flow. Per-channel hooks SHALL be limited to prompt rendering, the pending-approval match order, and, for Mattermost only, the synchronous webhook reply. +The system SHALL implement text-approval parsing, cold-spawn approval forwarding, and pending-prompt resolution in a single shared flow. The requester identity check SHALL execute inside the shared flow. The pending-approval match order SHALL be the same on every channel. Per-channel hooks SHALL be limited to prompt rendering and, for Mattermost only, the synchronous webhook reply. #### Scenario: Wrong requester is rejected on every channel @@ -48,20 +48,19 @@ The system SHALL implement text-approval parsing, cold-spawn approval forwarding - **THEN** the Mattermost hook sends the synchronous HTTP reply - **AND** Discord and Slack register no such hook -#### Scenario: Channel match order picks the same candidate as before +#### Scenario: Text approval resolves the earliest pending approval - **GIVEN** two pending approvals that the same sender may approve - **WHEN** that sender sends a text approval reply -- **THEN** Slack resolves the earliest pending approval -- **AND** Discord and Mattermost resolve the most recent pending approval +- **THEN** Slack, Discord, and Mattermost each resolve the earliest pending approval +- **AND** the next text approval reply resolves the second pending approval -> Note: this match order is the one real difference the step-3 stop rule found +> Note: the step-3 stop rule found this match order as the one real difference > between the three copies. Slack selected its candidate with `FindIndex` > (earliest match); Discord and Mattermost selected it with `LastOrDefault` -> (most recent match). The shared lookup keeps one requester check and takes the -> order as a required `ApprovalMatchOrder` input, so each channel keeps the -> selection it had. Which order is correct is a separate product question, -> tracked outside this change. +> (most recent match). The maintainer resolved the difference: the earliest +> pending approval wins on every channel. The shared lookup keeps one requester +> check and hard-codes the earliest match, so no channel supplies an order. ### Requirement: Shared output-completion bookkeeping diff --git a/openspec/specs/channel-binding-parity/spec.md b/openspec/specs/channel-binding-parity/spec.md index a8e889c4f..01279cbad 100644 --- a/openspec/specs/channel-binding-parity/spec.md +++ b/openspec/specs/channel-binding-parity/spec.md @@ -36,7 +36,7 @@ The shared engine SHALL store cursors as strings, which matches the persisted `C ### Requirement: Shared approval-response flow -The system SHALL implement text-approval parsing, cold-spawn approval forwarding, and pending-prompt resolution in a single shared flow. The requester identity check SHALL execute inside the shared flow. Per-channel hooks SHALL be limited to prompt rendering, the pending-approval match order, and, for Mattermost only, the synchronous webhook reply. +The system SHALL implement text-approval parsing, cold-spawn approval forwarding, and pending-prompt resolution in a single shared flow. The requester identity check SHALL execute inside the shared flow. The pending-approval match order SHALL be the same on every channel. Per-channel hooks SHALL be limited to prompt rendering and, for Mattermost only, the synchronous webhook reply. #### Scenario: Wrong requester is rejected on every channel @@ -52,20 +52,19 @@ The system SHALL implement text-approval parsing, cold-spawn approval forwarding - **THEN** the Mattermost hook sends the synchronous HTTP reply - **AND** Discord and Slack register no such hook -#### Scenario: Channel match order picks the same candidate as before +#### Scenario: Text approval resolves the earliest pending approval - **GIVEN** two pending approvals that the same sender may approve - **WHEN** that sender sends a text approval reply -- **THEN** Slack resolves the earliest pending approval -- **AND** Discord and Mattermost resolve the most recent pending approval +- **THEN** Slack, Discord, and Mattermost each resolve the earliest pending approval +- **AND** the next text approval reply resolves the second pending approval -> Note: this match order is a real difference between the pre-consolidation +> Note: the consolidation found a real difference between the pre-consolidation > copies. Slack selected its candidate with `FindIndex` (earliest match); > Discord and Mattermost selected it with `LastOrDefault` (most recent match). -> The shared lookup keeps one requester check and takes the order as a -> required `ApprovalMatchOrder` input, so each channel keeps the selection it -> had. Which order is correct is a separate product question, tracked outside -> the introducing change. +> The maintainer resolved the difference: the earliest pending approval wins on +> every channel. The shared lookup keeps one requester check and hard-codes the +> earliest match, so no channel supplies an order. ### Requirement: Shared output-completion bookkeeping diff --git a/src/Netclaw.Actors.Tests/Channels/Contracts/SessionBindingContractTests.cs b/src/Netclaw.Actors.Tests/Channels/Contracts/SessionBindingContractTests.cs index 5be2b3424..3ee462703 100644 --- a/src/Netclaw.Actors.Tests/Channels/Contracts/SessionBindingContractTests.cs +++ b/src/Netclaw.Actors.Tests/Channels/Contracts/SessionBindingContractTests.cs @@ -746,6 +746,70 @@ await AwaitAssertAsync(() => }, cancellationToken: ct); } + // Cross-channel match-order contract. Slack resolved the earliest pending + // approval; Discord and Mattermost resolved the most recent one. The + // consolidation found the divergence and the maintainer chose one rule for + // every channel: a text reply answers the earliest pending approval, which + // is the first prompt the channel shows. + [Fact] + public async Task Text_approval_response_resolves_earliest_pending_approval() + { + var ct = TestContext.Current.CancellationToken; + var detector = new ConfigurablePromptInjectionDetector(PromptInjectionResult.Safe()); + var sid = new SessionId("session-text-approve-order"); + var pipeline = new RecordingSessionPipeline(_ => + [ + ApprovalRequest(sid, "call-order-1", "write_file"), + ApprovalRequest(sid, "call-order-2", "execute_shell") + ]); + + var actor = CreateBindingActor(sid, pipeline, detector); + + await AwaitAssertAsync(() => + { + var texts = GetPostedTexts(); + Assert.Contains(texts, t => t.Contains("write_file")); + Assert.Contains(texts, t => t.Contains("execute_shell")); + }, cancellationToken: ct); + + // The same sender can approve both prompts, so only the order decides. + actor.Tell(CreateInboundMessage("A", "user-1"), TestActor); + + await AwaitAssertAsync(() => + { + var feedback = pipeline.RecordedFeedback.OfType().ToList(); + Assert.Single(feedback); + Assert.Equal("call-order-1", feedback[0].CallId.Value); + Assert.Equal(ApprovalOptionKeys.ApproveOnce, feedback[0].SelectedKey.Value); + }, cancellationToken: ct); + + // The second prompt stays pending and the next reply resolves it. + actor.Tell(CreateInboundMessage("A", "user-1"), TestActor); + + await AwaitAssertAsync(() => + { + var feedback = pipeline.RecordedFeedback.OfType().ToList(); + Assert.Equal(2, feedback.Count); + Assert.Equal("call-order-2", feedback[1].CallId.Value); + }, cancellationToken: ct); + } + + private static ToolInteractionRequest ApprovalRequest(SessionId sessionId, string callId, string toolName) + => new() + { + SessionId = sessionId, + Kind = "approval", + CallId = new Netclaw.Tools.ToolCallId(callId), + ToolName = new Netclaw.Tools.ToolName(toolName), + DisplayText = $"run {toolName}", + RequesterSenderId = new SenderId("user-1"), + Options = + [ + new ToolInteractionOption(ApprovalOptionKeys.ApproveOnceKey, ApprovalOptionKeys.ApproveOnceLabel), + new ToolInteractionOption(ApprovalOptionKeys.DenyKey, ApprovalOptionKeys.DenyLabel) + ] + }; + [Fact] public async Task Approval_response_after_turn_completed_forwards_to_session() { diff --git a/src/Netclaw.Actors/Reminders/ReminderManagerActor.cs b/src/Netclaw.Actors/Reminders/ReminderManagerActor.cs index 9d5a27edb..66100a169 100644 --- a/src/Netclaw.Actors/Reminders/ReminderManagerActor.cs +++ b/src/Netclaw.Actors/Reminders/ReminderManagerActor.cs @@ -52,6 +52,12 @@ public sealed partial class ReminderManagerActor : ReceiveActor private readonly ActiveExecutionTracker _activeExecutions = new(); private readonly Dictionary _skipCounts = []; + // Uniqueness source for execution child actor names. A wall-clock + // millisecond suffix collided when two fires for one reminder landed in + // the same millisecond and threw InvalidActorNameException. The actor is + // single-threaded, so a plain counter is collision-free for its lifetime. + private long _executionSequence; + public ReminderManagerActor( ISessionPipeline pipeline, EffectivePolicyDefaults defaults, @@ -1251,7 +1257,7 @@ private void StartExecution( var startedAt = _timeProvider.GetUtcNow(); _activeExecutions.Add(definition.Id, executionId, envelope, startedAt); - var actorName = $"exec-{SanitizeActorName(definition.Id.Value)}-{startedAt.ToUnixTimeMilliseconds()}"; + var actorName = $"exec-{SanitizeActorName(definition.Id.Value)}-{++_executionSequence}"; var executionActor = Context.ActorOf( ReminderExecutionActor.CreateProps( executionId, diff --git a/src/Netclaw.Channels.Discord/DiscordSessionBindingActor.cs b/src/Netclaw.Channels.Discord/DiscordSessionBindingActor.cs index 4595bf3f9..3a611c635 100644 --- a/src/Netclaw.Channels.Discord/DiscordSessionBindingActor.cs +++ b/src/Netclaw.Channels.Discord/DiscordSessionBindingActor.cs @@ -165,7 +165,6 @@ public DiscordSessionBindingActor( pipeline: _dependencies.Pipeline, operationTimeout: OperationTimeout, pendingRequests: _pendingApprovalRequests, - matchOrder: ApprovalMatchOrder.Newest, hasObservedApprovalRequest: () => _outputEngine.HasObservedApprovalRequest, postWrongRequesterWarningAsync: () => SafeReplyAsync(WrongRequesterWarning), persistPromptCleared: callId => Persist( diff --git a/src/Netclaw.Channels.Mattermost/MattermostSessionBindingActor.cs b/src/Netclaw.Channels.Mattermost/MattermostSessionBindingActor.cs index 12aee877a..9331ec392 100644 --- a/src/Netclaw.Channels.Mattermost/MattermostSessionBindingActor.cs +++ b/src/Netclaw.Channels.Mattermost/MattermostSessionBindingActor.cs @@ -157,7 +157,6 @@ public MattermostSessionBindingActor( pipeline: _dependencies.Pipeline, operationTimeout: OperationTimeout, pendingRequests: _pendingApprovalRequests, - matchOrder: ApprovalMatchOrder.Newest, hasObservedApprovalRequest: () => _outputEngine.HasObservedApprovalRequest, postWrongRequesterWarningAsync: () => SafeReplyAsync(WrongRequesterWarning), persistPromptCleared: callId => Persist( diff --git a/src/Netclaw.Channels.Slack/SlackThreadBindingActor.cs b/src/Netclaw.Channels.Slack/SlackThreadBindingActor.cs index b1a51b1fa..57a35c7a6 100644 --- a/src/Netclaw.Channels.Slack/SlackThreadBindingActor.cs +++ b/src/Netclaw.Channels.Slack/SlackThreadBindingActor.cs @@ -156,10 +156,6 @@ public SlackThreadBindingActor( pipeline: _dependencies.Pipeline, operationTimeout: OperationTimeout, pendingRequests: _pendingApprovalRequests, - // Slack resolves the earliest matching prompt, which is the order its - // own lookup used before the extraction. Discord and Mattermost - // resolve the most recent one. - matchOrder: ApprovalMatchOrder.Oldest, hasObservedApprovalRequest: () => _outputEngine.HasObservedApprovalRequest, postWrongRequesterWarningAsync: () => SafePostAsync(WrongRequesterWarning), persistPromptCleared: callId => Persist( diff --git a/src/Netclaw.Channels/ApprovalResponseFlow.cs b/src/Netclaw.Channels/ApprovalResponseFlow.cs index 9333b0041..0bf59ab2e 100644 --- a/src/Netclaw.Channels/ApprovalResponseFlow.cs +++ b/src/Netclaw.Channels/ApprovalResponseFlow.cs @@ -56,7 +56,6 @@ public sealed class ApprovalResponseFlow private readonly ISessionPipeline _pipeline; private readonly TimeSpan _operationTimeout; private readonly List _pendingRequests; - private readonly ApprovalMatchOrder _matchOrder; private readonly Func _hasObservedApprovalRequest; private readonly Func _postWrongRequesterWarningAsync; private readonly Action _persistPromptCleared; @@ -77,7 +76,6 @@ public sealed class ApprovalResponseFlow /// The actor's own pending-approval list. The flow reads it and removes the /// entry it resolves; the actor keeps adding to it and replaying it. /// - /// Which candidate wins when more than one matches. /// Reads the cold-path gate. /// Posts the channel's wrong-requester warning. /// @@ -93,7 +91,6 @@ public ApprovalResponseFlow( ISessionPipeline pipeline, TimeSpan operationTimeout, List pendingRequests, - ApprovalMatchOrder matchOrder, Func hasObservedApprovalRequest, Func postWrongRequesterWarningAsync, Action persistPromptCleared, @@ -106,7 +103,6 @@ public ApprovalResponseFlow( _pipeline = pipeline; _operationTimeout = operationTimeout; _pendingRequests = pendingRequests; - _matchOrder = matchOrder; _hasObservedApprovalRequest = hasObservedApprovalRequest; _postWrongRequesterWarningAsync = postWrongRequesterWarningAsync; _persistPromptCleared = persistPromptCleared; @@ -125,7 +121,7 @@ public ApprovalResponseFlow( public async Task TryHandleTextApprovalResponseAsync(string? text, string senderId) { var (result, pending) = PendingApprovalLookup.Resolve( - _pendingRequests, senderId, callId: null, _matchOrder); + _pendingRequests, senderId, callId: null); if (result is ApprovalLookupResult.NotFound) { @@ -240,7 +236,7 @@ public async Task HandleApprovalResponseAsync( Action? respondSynchronously = null) { var (result, pending) = PendingApprovalLookup.Resolve( - _pendingRequests, senderId, callId, _matchOrder); + _pendingRequests, senderId, callId); // CanApprove fast-path: if the binding still holds the original request we can // post the wrong-requester warning locally without round-tripping through the diff --git a/src/Netclaw.Channels/PendingApprovalLookup.cs b/src/Netclaw.Channels/PendingApprovalLookup.cs index f6db58d1d..4e8a01710 100644 --- a/src/Netclaw.Channels/PendingApprovalLookup.cs +++ b/src/Netclaw.Channels/PendingApprovalLookup.cs @@ -10,37 +10,30 @@ namespace Netclaw.Channels; public enum ApprovalLookupResult { Matched, WrongRequester, NotFound } -/// -/// Selects which candidate wins when more than one pending request matches. -/// The requester check is the same for both orders; only the tie-break differs. -/// -public enum ApprovalMatchOrder -{ - /// The most recent match wins. Discord and Mattermost use this. - Newest, - - /// The earliest match wins. Slack uses this. - Oldest -} - /// /// Finds the pending approval a channel binding actor should act on. A -/// given call ID takes priority; otherwise the pending request the sender -/// is allowed to approve wins, picked by the channel's match order. +/// given call ID takes priority; otherwise the earliest pending request the +/// sender is allowed to approve wins. /// +/// +/// Slack, Discord, and Mattermost all resolve the earliest match. Discord and +/// Mattermost resolved the most recent match before the binding-actor +/// consolidation. The maintainer decided that one order applies to every +/// channel: the earliest pending approval wins. A user who answers a queue of +/// prompts answers them in the order that the channel shows them. +/// public static class PendingApprovalLookup { public static (ApprovalLookupResult Result, TRequest? Pending) Resolve( IReadOnlyList pendingRequests, string approvingSenderId, - ToolCallId? callId, - ApprovalMatchOrder matchOrder) + ToolCallId? callId) where TRequest : PendingApprovalRequest where TPromptId : struct { if (callId is { } resolvedCallId) { - var byCallId = Select(pendingRequests, p => p.CallId == resolvedCallId, matchOrder); + var byCallId = pendingRequests.FirstOrDefault(p => p.CallId == resolvedCallId); if (byCallId is null) return (ApprovalLookupResult.NotFound, null); if (!ApprovalButtonValueCodec.CanApprove(byCallId.RequesterPrincipal, byCallId.RequesterSenderId, approvingSenderId)) @@ -51,21 +44,10 @@ public static (ApprovalLookupResult Result, TRequest? Pending) Resolve ApprovalButtonValueCodec.CanApprove(p.RequesterPrincipal, p.RequesterSenderId, approvingSenderId), - matchOrder); + var bySender = pendingRequests.FirstOrDefault( + p => ApprovalButtonValueCodec.CanApprove(p.RequesterPrincipal, p.RequesterSenderId, approvingSenderId)); return bySender is not null ? (ApprovalLookupResult.Matched, bySender) : (ApprovalLookupResult.WrongRequester, null); } - - private static TRequest? Select( - IReadOnlyList pendingRequests, - Func predicate, - ApprovalMatchOrder matchOrder) - where TRequest : class - => matchOrder is ApprovalMatchOrder.Newest - ? pendingRequests.LastOrDefault(predicate) - : pendingRequests.FirstOrDefault(predicate); }