From 91a47502e723ff20f343592ecba3edff83b2d1ac Mon Sep 17 00:00:00 2001 From: Aaron Stannard Date: Fri, 7 Aug 2026 18:41:53 +0000 Subject: [PATCH 1/3] fix(sessions): seed the immediate-retry approval bypass for every approved scope MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A shell command the user approved could still throw ToolApprovalRequiredException on its immediate retry. When the command is a pipeline, its standalone verbs (base64, head, ...) have no path argument, so "Always here" cannot persist a directory-scoped grant for them (by design). The one-time bypass that lets a just-approved call run this once was seeded ONLY for ApprovedOnce, so ApprovedSession / ApprovedAlways re-hit the gate on retry — the verbs the durable grant could not cover read as unapproved — and the turn failed with "I encountered an error executing a tool" for a command the user had approved. This is the trigger behind the self-hosted/DeepSeek approval failures. In a parallel batch approved call-by-call it presented as one sibling running and the other throwing (session-scoped verb-only match worked; the persistent grant did not cover the standalone verbs). Seed the one-time bypass for the just-approved call regardless of scope, in both the live pipeline (SessionToolExecutionPipeline) and the cold re-drive plan (LlmSessionActor.BuildApprovalRedrivePlan). This matches the sub-agent loop, which already seeds for every approved scope. The bypass is per-call, pattern-scoped, and cleared after the attempt, so it only authorizes the immediate retry the user approved; broader scopes still record their durable grant separately for future calls. Regression test drives ApprovedAlways through the pipeline against an executor that requires the bypass on retry: ToolExecutionFailed before, runs after. --- .../SessionToolExecutionPipelineTests.cs | 83 +++++++++++++++++++ .../Sessions/LlmSessionActor.cs | 14 +++- .../Pipelines/SessionToolExecutionPipeline.cs | 19 +++-- 3 files changed, 106 insertions(+), 10 deletions(-) diff --git a/src/Netclaw.Actors.Tests/Sessions/SessionToolExecutionPipelineTests.cs b/src/Netclaw.Actors.Tests/Sessions/SessionToolExecutionPipelineTests.cs index b29e9adc0..3dc1d574d 100644 --- a/src/Netclaw.Actors.Tests/Sessions/SessionToolExecutionPipelineTests.cs +++ b/src/Netclaw.Actors.Tests/Sessions/SessionToolExecutionPipelineTests.cs @@ -136,6 +136,52 @@ await probe.ExpectNoMsgAsync( Assert.Equal("approved-and-ran", completed.ToolResults[0].Content); } + [Fact] + public async Task Approved_always_seeds_the_immediate_retry_bypass_so_a_partially_covered_command_still_runs() + { + // Regression for the "approved command still throws" trigger. A piped + // command's standalone verbs (e.g. base64, head) have no path argument, so + // "Always here" cannot persist a directory-scoped grant for them — by + // design. The user's click must still run THIS call once, via the one-time + // bypass. The bug: the pipeline seeded that bypass only for ApprovedOnce, so + // ApprovedSession/ApprovedAlways re-hit the gate on retry and failed the + // turn. This fake re-requires approval until the retry carries the bypass. + var executor = new BypassRequiredOnRetryExecutor(); + var approvalChannel = new ApprovalChannel(); + var probe = CreateTestProbe("approved-always-bypass-probe"); + var approvalRequestTcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var sessionId = new SessionId("D1/approved-always-bypass"); + + var toolCalls = new List + { + new("call-1", "shell_execute", new Dictionary + { + ["command"] = "gh api foo/bar 2>/dev/null | base64 -d | head" + }) + }; + + var pipelineTask = new SessionToolPipelineTestFixture(executor, toolCalls, sessionId, probe.Ref) + .WithTurnContext(InteractiveTurnContext(sessionId)) + .WithApprovals( + approvalChannel, + request => approvalRequestTcs.TrySetResult(request.Request), + Timeout.InfiniteTimeSpan) + .ExecuteAsync(TestContext.Current.CancellationToken); + + var approvalRequest = await approvalRequestTcs.Task.WaitAsync( + TimeSpan.FromSeconds(3), cancellationToken: TestContext.Current.CancellationToken); + + approvalChannel.Complete(approvalRequest.CallId, ApprovalDecision.ApprovedAlways); + + var completed = await probe.ExpectMsgAsync( + TimeSpan.FromSeconds(3), cancellationToken: TestContext.Current.CancellationToken); + + await pipelineTask.WaitAsync(TimeSpan.FromSeconds(3), TestContext.Current.CancellationToken); + + var result = Assert.Single(completed.ToolResults); + Assert.Equal("ran-with-bypass", result.Content); + } + [Fact] public async Task Source_less_approval_required_turn_fails_closed_without_prompt() { @@ -798,6 +844,43 @@ public Task ExecuteAsync(FunctionCallContent toolCall, ToolExecutionCont } } + private sealed class BypassRequiredOnRetryExecutor : IToolExecutor + { + private static readonly string[] Patterns = ["gh api", "base64", "head"]; + + public Task AuthorizeAsync(FunctionCallContent toolCall, ToolExecutionContext? context = null, CancellationToken ct = default) + => Task.CompletedTask; + + public Task ExecuteAsync(FunctionCallContent toolCall, ToolExecutionContext? context = null, CancellationToken ct = default) + { + // Stand in for the real gate on a command whose persisted grant does not + // cover every candidate verb: authorization keeps requiring approval + // until the immediate retry carries the one-time bypass for these + // patterns. That bypass is what the pipeline must seed for every + // approved scope, not just ApprovedOnce. + var approval = context?.Approval; + if (approval is not null + && string.Equals(approval.OneTimeApprovedToolName, toolCall.Name, StringComparison.Ordinal) + && Patterns.All(approval.OneTimeApprovedPatterns.Contains)) + { + return Task.FromResult("ran-with-bypass"); + } + + throw new ToolApprovalRequiredException(new ToolApprovalContext( + ToolName: toolCall.Name, + DisplayText: "gh api foo/bar | base64 -d | head", + Patterns: Patterns, + CandidateVerbs: Patterns, + Options: + [ + new ToolApprovalOption(ApprovalOptionKeys.ApproveOnceKey, ApprovalOptionKeys.ApproveOnceLabel), + new ToolApprovalOption(ApprovalOptionKeys.ApproveSessionKey, ApprovalOptionKeys.ApproveSessionLabel), + new ToolApprovalOption(ApprovalOptionKeys.ApproveAlwaysKey, ApprovalOptionKeys.ApproveAlwaysLabel), + new ToolApprovalOption(ApprovalOptionKeys.DenyKey, ApprovalOptionKeys.DenyLabel) + ])); + } + } + private sealed class ContextCapturingExecutor : IToolExecutor { public ToolExecutionContext? Context { get; private set; } diff --git a/src/Netclaw.Actors/Sessions/LlmSessionActor.cs b/src/Netclaw.Actors/Sessions/LlmSessionActor.cs index 8d33fbebb..e40e44e6d 100644 --- a/src/Netclaw.Actors/Sessions/LlmSessionActor.cs +++ b/src/Netclaw.Actors/Sessions/LlmSessionActor.cs @@ -4315,10 +4315,18 @@ private ApprovalRedrivePlan BuildApprovalRedrivePlan(SerializableChatMessage ass if (!_resolvedToolApprovals.TryGetValue(call.CallId.Value, out var resolved)) continue; - if (resolved.Decision == ApprovalDecision.ApprovedOnce) + if (resolved.Decision is ApprovalDecision.ApprovedOnce + or ApprovalDecision.ApprovedSession + or ApprovalDecision.ApprovedAlways + or ApprovalDecision.ApprovedEverywhere) { - // ApprovedOnce has no persisted grant. Pre-seed only this call - // so the re-drive skips the gate once without broadening approval. + // Pre-seed the one-time bypass for the just-approved call so the + // re-drive runs it once even when its durable grant (if any) does + // not cover every candidate verb — e.g. a piped command's standalone + // verbs (base64, head) are never persisted directory-scoped. + // ApprovedOnce has no durable grant at all; broader scopes still + // record their durable grant separately. This only authorizes the + // immediate re-drive, matching the live pipeline and the sub-agent. preSeed[call.CallId.Value] = resolved.Pending.Patterns; } diff --git a/src/Netclaw.Actors/Sessions/Pipelines/SessionToolExecutionPipeline.cs b/src/Netclaw.Actors/Sessions/Pipelines/SessionToolExecutionPipeline.cs index 13c992a52..08440da00 100644 --- a/src/Netclaw.Actors/Sessions/Pipelines/SessionToolExecutionPipeline.cs +++ b/src/Netclaw.Actors/Sessions/Pipelines/SessionToolExecutionPipeline.cs @@ -570,13 +570,18 @@ or ApprovalDecision.ApprovedSession or ApprovalDecision.ApprovedAlways or ApprovalDecision.ApprovedEverywhere) { - // Retry execution now that approval is granted - // (Approve-once is retried through transient context state; broader scopes - // are also recorded by the session actor into the shared approval service.) - if (decision == ApprovalDecision.ApprovedOnce) - { - context.Approval.SeedOneTimeApproval(tc.Name, ctx.Patterns); - } + // Retry execution now that approval is granted. Seed the one-time + // bypass for the just-approved call regardless of scope. Broader + // scopes (session/always) DO get a durable grant recorded by the + // session actor, but that grant can legitimately not cover every + // candidate: a piped command's standalone verbs (base64, head) have + // no path argument and so are never persisted directory-scoped + // (by design). Without the transient bypass, the immediate retry + // re-hits the gate and fails a call the user just approved. This + // matches the sub-agent loop (SubAgentActor), which seeds for every + // approved scope. The bypass is per-call, pattern-scoped, and + // cleared after the attempt, so it cannot leak to any other call. + context.Approval.SeedOneTimeApproval(tc.Name, ctx.Patterns); sw = Stopwatch.StartNew(); if (meta is { Background: true } From b0ae318d15104000dcb687771ae2c67dba1b8bba Mon Sep 17 00:00:00 2001 From: Aaron Stannard Date: Fri, 7 Aug 2026 19:01:12 +0000 Subject: [PATCH 2/3] refactor(sessions): extract IsApprovalGrant for the approve-scope predicate Route the three "approval granted" seed sites through one predicate instead of duplicating the four-scope list: the live pipeline retry (SessionToolExecutionPipeline), the cold re-drive plan (LlmSessionActor.BuildApprovalRedrivePlan), and the sub-agent loop (SubAgentActor). Adds ApprovalDecision.IsApprovalGrant() and the ParentApprovalDecision twin. A future approval scope now updates one predicate, not three. Missing a site had reintroduced exactly the "approved command still fails on retry" bug this branch fixes, for the new scope (addresses the code-review maintainability finding). --- .../Sessions/IApprovalChannel.cs | 20 +++++++++++++++++++ .../Sessions/LlmSessionActor.cs | 5 +---- .../Pipelines/SessionToolExecutionPipeline.cs | 5 +---- src/Netclaw.Actors/SubAgents/SubAgentActor.cs | 5 +---- .../IParentApprovalBridge.cs | 17 ++++++++++++++++ 5 files changed, 40 insertions(+), 12 deletions(-) diff --git a/src/Netclaw.Actors/Sessions/IApprovalChannel.cs b/src/Netclaw.Actors/Sessions/IApprovalChannel.cs index 0e0aadaeb..b839e7de4 100644 --- a/src/Netclaw.Actors/Sessions/IApprovalChannel.cs +++ b/src/Netclaw.Actors/Sessions/IApprovalChannel.cs @@ -43,6 +43,26 @@ public enum ApprovalDecision TimedOut } +/// +/// Extensions over . +/// +public static class ApprovalDecisionExtensions +{ + /// + /// True when the decision grants execution (any approve scope) rather than + /// Denied or TimedOut. Every "the user approved" branch — the live pipeline + /// retry, the cold re-drive plan, and the sub-agent loop — must classify the + /// approve scopes identically, so route them through this one predicate + /// instead of duplicating the scope list (a missed site reintroduces the + /// "approved command still fails" bug for the new scope). + /// + public static bool IsApprovalGrant(this ApprovalDecision decision) + => decision is ApprovalDecision.ApprovedOnce + or ApprovalDecision.ApprovedSession + or ApprovalDecision.ApprovedAlways + or ApprovalDecision.ApprovedEverywhere; +} + /// /// Bridge between the tool execution pipeline (thread pool) and the session actor /// (mailbox). Allows tool tasks to block awaiting user approval while the actor diff --git a/src/Netclaw.Actors/Sessions/LlmSessionActor.cs b/src/Netclaw.Actors/Sessions/LlmSessionActor.cs index e40e44e6d..11cf3de96 100644 --- a/src/Netclaw.Actors/Sessions/LlmSessionActor.cs +++ b/src/Netclaw.Actors/Sessions/LlmSessionActor.cs @@ -4315,10 +4315,7 @@ private ApprovalRedrivePlan BuildApprovalRedrivePlan(SerializableChatMessage ass if (!_resolvedToolApprovals.TryGetValue(call.CallId.Value, out var resolved)) continue; - if (resolved.Decision is ApprovalDecision.ApprovedOnce - or ApprovalDecision.ApprovedSession - or ApprovalDecision.ApprovedAlways - or ApprovalDecision.ApprovedEverywhere) + if (resolved.Decision.IsApprovalGrant()) { // Pre-seed the one-time bypass for the just-approved call so the // re-drive runs it once even when its durable grant (if any) does diff --git a/src/Netclaw.Actors/Sessions/Pipelines/SessionToolExecutionPipeline.cs b/src/Netclaw.Actors/Sessions/Pipelines/SessionToolExecutionPipeline.cs index 08440da00..2e0707998 100644 --- a/src/Netclaw.Actors/Sessions/Pipelines/SessionToolExecutionPipeline.cs +++ b/src/Netclaw.Actors/Sessions/Pipelines/SessionToolExecutionPipeline.cs @@ -565,10 +565,7 @@ private async Task ExecuteSingleToolAsync( sw.Stop(); - if (decision is ApprovalDecision.ApprovedOnce - or ApprovalDecision.ApprovedSession - or ApprovalDecision.ApprovedAlways - or ApprovalDecision.ApprovedEverywhere) + if (decision.IsApprovalGrant()) { // Retry execution now that approval is granted. Seed the one-time // bypass for the just-approved call regardless of scope. Broader diff --git a/src/Netclaw.Actors/SubAgents/SubAgentActor.cs b/src/Netclaw.Actors/SubAgents/SubAgentActor.cs index 09fea7a0f..2731503de 100644 --- a/src/Netclaw.Actors/SubAgents/SubAgentActor.cs +++ b/src/Netclaw.Actors/SubAgents/SubAgentActor.cs @@ -1190,10 +1190,7 @@ private static async Task ExecuteToolsAsync( self.Tell(SubAgentApprovalWaitCompleted.Instance); } - if (decision is ParentApprovalDecision.ApprovedOnce - or ParentApprovalDecision.ApprovedSession - or ParentApprovalDecision.ApprovedAlways - or ParentApprovalDecision.ApprovedEverywhere) + if (decision.IsApprovalGrant()) { // The immediate retry needs a transient grant even for session/always // approvals because the sub-agent's scope ID differs from the parent diff --git a/src/Netclaw.Tools.Abstractions/IParentApprovalBridge.cs b/src/Netclaw.Tools.Abstractions/IParentApprovalBridge.cs index 75d586bad..f7364006e 100644 --- a/src/Netclaw.Tools.Abstractions/IParentApprovalBridge.cs +++ b/src/Netclaw.Tools.Abstractions/IParentApprovalBridge.cs @@ -39,6 +39,23 @@ public enum ParentApprovalDecision TimedOut } +/// +/// Extensions over . +/// +public static class ParentApprovalDecisionExtensions +{ + /// + /// True when the sub-agent's parent decision grants execution (any approve + /// scope). Mirrors ApprovalDecision.IsApprovalGrant so the sub-agent + /// loop classifies approve scopes identically to the parent session paths. + /// + public static bool IsApprovalGrant(this ParentApprovalDecision decision) + => decision is ParentApprovalDecision.ApprovedOnce + or ParentApprovalDecision.ApprovedSession + or ParentApprovalDecision.ApprovedAlways + or ParentApprovalDecision.ApprovedEverywhere; +} + /// /// Thrown when a sub-agent needs parent approval but the parent session cannot /// safely emit an approval prompt with complete authority context. From 3c7251e62633d2f78e311e5cd273cd0fce1b642b Mon Sep 17 00:00:00 2001 From: Aaron Stannard Date: Fri, 7 Aug 2026 19:08:31 +0000 Subject: [PATCH 3/3] docs: link the approval-bypass fix to netclaw-dev/netclaw#1802 Reference the tracking issue from the pipeline/session-actor fix comments and the pipeline regression test, and trim the inline explanation now that the issue carries the full analysis. --- .../Sessions/SessionToolExecutionPipelineTests.cs | 13 ++++++------- src/Netclaw.Actors/Sessions/LlmSessionActor.cs | 1 + .../Pipelines/SessionToolExecutionPipeline.cs | 3 ++- 3 files changed, 9 insertions(+), 8 deletions(-) diff --git a/src/Netclaw.Actors.Tests/Sessions/SessionToolExecutionPipelineTests.cs b/src/Netclaw.Actors.Tests/Sessions/SessionToolExecutionPipelineTests.cs index 3dc1d574d..363a2dbdf 100644 --- a/src/Netclaw.Actors.Tests/Sessions/SessionToolExecutionPipelineTests.cs +++ b/src/Netclaw.Actors.Tests/Sessions/SessionToolExecutionPipelineTests.cs @@ -139,13 +139,12 @@ await probe.ExpectNoMsgAsync( [Fact] public async Task Approved_always_seeds_the_immediate_retry_bypass_so_a_partially_covered_command_still_runs() { - // Regression for the "approved command still throws" trigger. A piped - // command's standalone verbs (e.g. base64, head) have no path argument, so - // "Always here" cannot persist a directory-scoped grant for them — by - // design. The user's click must still run THIS call once, via the one-time - // bypass. The bug: the pipeline seeded that bypass only for ApprovedOnce, so - // ApprovedSession/ApprovedAlways re-hit the gate on retry and failed the - // turn. This fake re-requires approval until the retry carries the bypass. + // Regression for the "approved command still throws" trigger + // (https://github.com/netclaw-dev/netclaw/issues/1802): the pipeline seeded + // the one-time retry bypass only for ApprovedOnce, so a command approved + // with ApprovedSession/ApprovedAlways whose durable grant does not cover + // every verb re-hit the gate on retry and failed the turn. This fake + // re-requires approval until the immediate retry carries the bypass. var executor = new BypassRequiredOnRetryExecutor(); var approvalChannel = new ApprovalChannel(); var probe = CreateTestProbe("approved-always-bypass-probe"); diff --git a/src/Netclaw.Actors/Sessions/LlmSessionActor.cs b/src/Netclaw.Actors/Sessions/LlmSessionActor.cs index 11cf3de96..86b06d687 100644 --- a/src/Netclaw.Actors/Sessions/LlmSessionActor.cs +++ b/src/Netclaw.Actors/Sessions/LlmSessionActor.cs @@ -4324,6 +4324,7 @@ private ApprovalRedrivePlan BuildApprovalRedrivePlan(SerializableChatMessage ass // ApprovedOnce has no durable grant at all; broader scopes still // record their durable grant separately. This only authorizes the // immediate re-drive, matching the live pipeline and the sub-agent. + // See https://github.com/netclaw-dev/netclaw/issues/1802. preSeed[call.CallId.Value] = resolved.Pending.Patterns; } diff --git a/src/Netclaw.Actors/Sessions/Pipelines/SessionToolExecutionPipeline.cs b/src/Netclaw.Actors/Sessions/Pipelines/SessionToolExecutionPipeline.cs index 2e0707998..bb422d495 100644 --- a/src/Netclaw.Actors/Sessions/Pipelines/SessionToolExecutionPipeline.cs +++ b/src/Netclaw.Actors/Sessions/Pipelines/SessionToolExecutionPipeline.cs @@ -568,7 +568,8 @@ private async Task ExecuteSingleToolAsync( if (decision.IsApprovalGrant()) { // Retry execution now that approval is granted. Seed the one-time - // bypass for the just-approved call regardless of scope. Broader + // bypass for the just-approved call regardless of scope + // (https://github.com/netclaw-dev/netclaw/issues/1802). Broader // scopes (session/always) DO get a durable grant recorded by the // session actor, but that grant can legitimately not cover every // candidate: a piped command's standalone verbs (base64, head) have