Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,51 @@ 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
// (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");
var approvalRequestTcs = new TaskCompletionSource<ToolInteractionRequest>(TaskCreationOptions.RunContinuationsAsynchronously);
var sessionId = new SessionId("D1/approved-always-bypass");

var toolCalls = new List<FunctionCallContent>
{
new("call-1", "shell_execute", new Dictionary<string, object?>
{
["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<ToolExecutionCompleted>(
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()
{
Expand Down Expand Up @@ -798,6 +843,43 @@ public Task<string> 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<string> 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; }
Expand Down
20 changes: 20 additions & 0 deletions src/Netclaw.Actors/Sessions/IApprovalChannel.cs
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,26 @@ public enum ApprovalDecision
TimedOut
}

/// <summary>
/// Extensions over <see cref="ApprovalDecision"/>.
/// </summary>
public static class ApprovalDecisionExtensions
{
/// <summary>
/// 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).
/// </summary>
public static bool IsApprovalGrant(this ApprovalDecision decision)
=> decision is ApprovalDecision.ApprovedOnce
or ApprovalDecision.ApprovedSession
or ApprovalDecision.ApprovedAlways
or ApprovalDecision.ApprovedEverywhere;
}

/// <summary>
/// Bridge between the tool execution pipeline (thread pool) and the session actor
/// (mailbox). Allows tool tasks to block awaiting user approval while the actor
Expand Down
12 changes: 9 additions & 3 deletions src/Netclaw.Actors/Sessions/LlmSessionActor.cs

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Original file line number Diff line number Diff line change
Expand Up @@ -4315,10 +4315,16 @@ private ApprovalRedrivePlan BuildApprovalRedrivePlan(SerializableChatMessage ass
if (!_resolvedToolApprovals.TryGetValue(call.CallId.Value, out var resolved))
continue;

if (resolved.Decision == ApprovalDecision.ApprovedOnce)
if (resolved.Decision.IsApprovalGrant())
{
// 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.
// See https://github.com/netclaw-dev/netclaw/issues/1802.
preSeed[call.CallId.Value] = resolved.Pending.Patterns;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -565,18 +565,21 @@ private async Task<ToolCallResult> 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
// (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)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

{
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
// (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
// 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 }
Expand Down
5 changes: 1 addition & 4 deletions src/Netclaw.Actors/SubAgents/SubAgentActor.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
17 changes: 17 additions & 0 deletions src/Netclaw.Tools.Abstractions/IParentApprovalBridge.cs
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,23 @@ public enum ParentApprovalDecision
TimedOut
}

/// <summary>
/// Extensions over <see cref="ParentApprovalDecision"/>.
/// </summary>
public static class ParentApprovalDecisionExtensions

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

{
/// <summary>
/// True when the sub-agent's parent decision grants execution (any approve
/// scope). Mirrors <c>ApprovalDecision.IsApprovalGrant</c> so the sub-agent
/// loop classifies approve scopes identically to the parent session paths.
/// </summary>
public static bool IsApprovalGrant(this ParentApprovalDecision decision)
=> decision is ParentApprovalDecision.ApprovedOnce
or ParentApprovalDecision.ApprovedSession
or ParentApprovalDecision.ApprovedAlways
or ParentApprovalDecision.ApprovedEverywhere;
}

/// <summary>
/// Thrown when a sub-agent needs parent approval but the parent session cannot
/// safely emit an approval prompt with complete authority context.
Expand Down
Loading