feat(tools): report pre-execution authorization decisions - #1745
Conversation
Aaronontheweb
added
security
There was a problem hiding this comment.
Pull request overview
This PR adds a typed allow reason for each successful tool authorization.
It records the final allow reason in the per-attempt approval context.
It provides one exhaustive helper that maps each reason to a short operator explanation.
It adds the reason and the explanation to existing tool completion and failure logs.
Changes:
- Add
ToolAllowReasonplusGetDescription()for stable operator-facing explanations. - Replace applied-decision strings with
ToolApprovalAttempt.AllowReasonand optionalApprovalEvidence. - Thread allow reasons through
ToolAccessDecisionand log them inDispatchingToolExecutor.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/Netclaw.Tools.Abstractions/ToolExecutionContext.cs | Add ToolAllowReason, store allow reason and approval evidence on ToolApprovalAttempt, and add a description helper. |
| src/Netclaw.Actors/Tools/ToolAccessPolicy.cs | Add AllowReason to ToolAccessDecision and set reasons for policy auto-allow and safe-verb short-circuit. |
| src/Netclaw.Actors/Tools/DispatchingToolExecutor.cs | Capture allow reason from authorization and include it in success and failure logs. |
| src/Netclaw.Actors.Tests/Tools/ToolExecutionValueObjectTests.cs | Add a test that enforces a unique, non-empty description per allow reason. |
| src/Netclaw.Actors.Tests/Tools/ToolApprovalGateTests.cs | Assert allow reasons for auto-allow and safe-verb short-circuit paths. |
| src/Netclaw.Actors.Tests/Tools/DispatchingToolExecutorTests.cs | Assert allow reasons are recorded on context and emitted in executor logs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| var projectDirectory = Path.Combine( | ||
| Path.GetTempPath(), | ||
| $"netclaw-safe-reason-{Guid.NewGuid():N}"); |
Aaronontheweb
left a comment
Aaronontheweb
left a comment
There was a problem hiding this comment.
Working through it
749deac to
9afc4c2
Compare
Aaronontheweb
changed the title
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
src/Netclaw.Actors/Tools/ToolAccessPolicy.cs:612
- The AllowReason property comment says it applies only to a policy result, but the executor also sets it for StoredApproval and OneTimeApproval. Update the summary so it matches actual usage.
/// <summary>
/// Gets the allow rule for an allowed policy result.
/// </summary>
internal ToolAllowReason? AllowReason { get; private init; }
| var candidatesForCheck = approvalContext.Candidates is { Count: > 0 } candidates | ||
| ? candidates | ||
| .Where(c => !ApprovalPatternMatching.IsPureSideEffect(c)) | ||
| .ToList() | ||
| : approvalContext.CandidateVerbs | ||
| .Select(verb => new ApprovalCandidate(verb, Directory: null)) | ||
| .ToList(); | ||
|
|
||
| if (candidatesForCheck.Count == 0) | ||
| { | ||
| // Every candidate is side-effect-only — auto-allow. | ||
| accessDecision = ToolAccessDecision.Allow(); | ||
| accessDecision = ToolAccessDecision.Allow(ToolAllowReason.ApprovalExemptShellCandidates); | ||
| } | ||
| else | ||
| { |
There was a problem hiding this comment.
Good catch. This fail-open branch already exists on dev; this pull request only exposes its reason.
We will fix it in #1746 immediately after this pull request merges. The fix will require at least one extracted candidate before the exemption applies. A zero-candidate result will remain RequiresApproval.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/Netclaw.Actors/Tools/DispatchingToolExecutor.cs:469
- The ArgumentOutOfRangeException uses nameof(decision) as paramName, but the out-of-range value is decision.Outcome. This makes the exception message less precise for diagnostics.
default:
throw new ArgumentOutOfRangeException(nameof(decision), decision.Outcome, "Unknown authorization outcome.");
There was a problem hiding this comment.
Moved the decision-making telemetry from the final stage of the tool approval process to the middle part of it, so we can better see what the internal plumbing is deciding. It's not interesting to test "tools approved by user was approved" - which is what would happen if we tested the TOP of the approval pipeline. Better to see what the software decided to do automatically based off a combination of ShellSyntaxTree, netclaw approvals, and some other auto-rules like denying known dangerous commands.
| /// This method returns expected authorization outcomes instead of exceptions. | ||
| /// Execution adapters translate the result into the existing pipeline exceptions. | ||
| /// </remarks> | ||
| internal async Task<ToolAuthorizationDecision> EvaluateAuthorizationAsync( |
There was a problem hiding this comment.
LGTM - this should help make #1733 more testable when the time comes
| var candidatesForCheck = approvalContext.Candidates is { Count: > 0 } candidates | ||
| ? candidates | ||
| .Where(c => !ApprovalPatternMatching.IsPureSideEffect(c)) | ||
| .ToList() | ||
| : approvalContext.CandidateVerbs | ||
| .Select(verb => new ApprovalCandidate(verb, Directory: null)) | ||
| .ToList(); | ||
|
|
||
| if (candidatesForCheck.Count == 0) | ||
| { | ||
| // Every candidate is side-effect-only — auto-allow. | ||
| accessDecision = ToolAccessDecision.Allow(); | ||
| accessDecision = ToolAccessDecision.Allow(ToolAllowReason.ApprovalExemptShellCandidates); | ||
| } | ||
| else | ||
| { |
There was a problem hiding this comment.
Good catch. This fail-open branch already exists on dev; this pull request only exposes its reason.
We will fix it in #1746 immediately after this pull request merges. The fix will require at least one extracted candidate before the exemption applies. A zero-candidate result will remain RequiresApproval.
Summary
Pipeline
The dispatcher now evaluates one complete decision for each attempt.
Execution methods consume that decision through the existing exception adapter.
The session pipeline still owns the user prompt and its response.
A retry creates a new authorization decision through the same evaluator.
Telemetry
The evaluator emits telemetry when it makes the decision.
The telemetry does not include commands, paths, patterns, or grant details.
Scope
This PR does not change authorization behavior.
It does not test a user interaction.
It does not change model-facing tool results.
It does not change the IToolExecutor contract.
It keeps the existing public ToolAccessDecision API.
This contract gives the later approval matrix a direct result to assert.
The matrix does not need to execute a tool or drive an approval prompt.
Validation