diff --git a/IMPLEMENTATION_PLAN.md b/IMPLEMENTATION_PLAN.md index 2783437ad..11fc30c9f 100644 --- a/IMPLEMENTATION_PLAN.md +++ b/IMPLEMENTATION_PLAN.md @@ -128,6 +128,8 @@ Done when: still requires approval. - [x] A prompt excludes a safe stage from the approval candidates that the user can persist. +- [x] A prompt excludes candidates that existing session or persistent grants + already cover, while it preserves exact directory-scoped occurrences. - [x] A one-time retry is bound to the exact prompted candidate set, including each effective directory, across live, sub-agent, and redrive paths. - [x] External paths, mismatched grants, dynamic syntax, and hard-deny rules diff --git a/src/Netclaw.Actors.Tests/Tools/DispatchingToolExecutorTests.cs b/src/Netclaw.Actors.Tests/Tools/DispatchingToolExecutorTests.cs index 2b4f607a4..716be2778 100644 --- a/src/Netclaw.Actors.Tests/Tools/DispatchingToolExecutorTests.cs +++ b/src/Netclaw.Actors.Tests/Tools/DispatchingToolExecutorTests.cs @@ -555,9 +555,262 @@ public async Task Authorization_evaluation_preserves_partial_approval_matches() Assert.Equal(ToolAuthorizationOutcome.RequiresApproval, decision.Outcome); Assert.NotNull(decision.ApprovalContext); + Assert.Equal(["git status", "git push"], decision.ApprovalContext.CandidateVerbs); Assert.Equal([approvedMatch], decision.ApprovalMatches); } + [Fact] + public async Task Authorization_evaluation_prompts_only_for_exact_unapproved_candidates() + { + var config = new ToolConfig { ShellMode = ShellExecutionMode.HostAllowed }; + config.AudienceProfiles.Personal.ApprovalPolicy = new ToolApprovalConfig + { + ToolOverrides = new Dictionary(StringComparer.Ordinal) + { + ["shell_execute"] = ToolApprovalMode.Approval + } + }; + var registry = new ToolRegistry(); + registry.WithFirstPartyTools( + config, + new NetclawPaths(), + new ToolPathPolicy([]), + new ShellCommandPolicy()); + var approvedMatch = new ToolApprovalMatch("git status", "session", "this chat"); + var approvedCandidate = new ApprovalCandidate("git status", Directory: null); + var unapprovedCandidate = new ApprovalCandidate("git push", Directory: null); + var approvalService = new FixedApprovalService( + new ToolApprovalCheckResult( + ["git push"], + [approvedMatch]) + { + CandidateChecks = + [ + new ToolApprovalCandidateCheck(approvedCandidate, approvedMatch), + new ToolApprovalCandidateCheck(unapprovedCandidate, ApprovedMatch: null) + ] + }); + var executor = new DispatchingToolExecutor( + registry, + new ToolAccessPolicy( + config, + new EffectivePolicyDefaults( + DeploymentPosture.Personal, + TrustAudience.Personal, + ShellExecutionMode.HostAllowed, + UsedStrictFallback: false), + new ShellCommandPolicy(), + new ToolPathPolicy([])), + approvalService); + var call = new FunctionCallContent( + "call-exact-partial-approval", + "shell_execute", + ToolInput.Create("Command", "git status && git push")); + var context = CreateInteractivePersonalContext("signalr/thread-exact-partial-approval"); + + var decision = await executor.EvaluateAuthorizationAsync( + call, + context, + TestContext.Current.CancellationToken); + + Assert.Equal(ToolAuthorizationOutcome.RequiresApproval, decision.Outcome); + var approvalContext = Assert.IsType(decision.ApprovalContext); + Assert.Equal(["git push"], approvalContext.Patterns); + Assert.Equal(["git push"], approvalContext.CandidateVerbs); + Assert.Equal([unapprovedCandidate], approvalContext.Candidates); + Assert.Equal([approvedMatch], decision.ApprovalMatches); + } + + [SlopwatchSuppress("SW001", "This test verifies Bash parser directory attribution, which does not apply to the Windows shell parser.")] + [Fact(SkipUnless = nameof(IsPosix), Skip = "POSIX-only shell directory semantics")] + public async Task Authorization_evaluation_preserves_directory_for_duplicate_verb_candidates() + { + var root = Path.Combine(Path.GetTempPath(), $"netclaw-prompt-scope-{Guid.NewGuid():N}"); + var approvedDirectory = Path.Combine(root, "approved"); + var unapprovedDirectory = Path.Combine(root, "unapproved"); + Directory.CreateDirectory(approvedDirectory); + Directory.CreateDirectory(unapprovedDirectory); + + try + { + var config = new ToolConfig { ShellMode = ShellExecutionMode.HostAllowed }; + config.AudienceProfiles.Personal.ApprovalPolicy = new ToolApprovalConfig + { + ToolOverrides = new Dictionary(StringComparer.Ordinal) + { + ["shell_execute"] = ToolApprovalMode.Approval + } + }; + var registry = new ToolRegistry(); + registry.WithFirstPartyTools( + config, + new NetclawPaths(), + new ToolPathPolicy([]), + new ShellCommandPolicy()); + var approvedMatch = new ToolApprovalMatch("git push", "persistent", approvedDirectory); + var approvedCandidate = new ApprovalCandidate("git push", approvedDirectory); + var unapprovedCandidate = new ApprovalCandidate("git push", unapprovedDirectory); + var approvalService = new FixedApprovalService( + new ToolApprovalCheckResult( + ["git push"], + [approvedMatch]) + { + CandidateChecks = + [ + new ToolApprovalCandidateCheck(approvedCandidate, approvedMatch), + new ToolApprovalCandidateCheck(unapprovedCandidate, ApprovedMatch: null) + ] + }); + var executor = new DispatchingToolExecutor( + registry, + new ToolAccessPolicy( + config, + new EffectivePolicyDefaults( + DeploymentPosture.Personal, + TrustAudience.Personal, + ShellExecutionMode.HostAllowed, + UsedStrictFallback: false), + new ShellCommandPolicy(), + new ToolPathPolicy([])), + approvalService); + var call = new FunctionCallContent( + "call-duplicate-verb-scopes", + "shell_execute", + ToolInput.Create( + "Command", + $"git -C {approvedDirectory} push && git -C {unapprovedDirectory} push")); + var context = CreateInteractivePersonalContext("signalr/thread-duplicate-verb-scopes"); + + var decision = await executor.EvaluateAuthorizationAsync( + call, + context, + TestContext.Current.CancellationToken); + + Assert.Equal(ToolAuthorizationOutcome.RequiresApproval, decision.Outcome); + var approvalContext = Assert.IsType(decision.ApprovalContext); + Assert.Equal(["git push"], approvalContext.Patterns); + Assert.Equal(["git push"], approvalContext.CandidateVerbs); + Assert.Equal([unapprovedCandidate], approvalContext.Candidates); + Assert.Equal([approvedMatch], decision.ApprovalMatches); + } + finally + { + Directory.Delete(root, recursive: true); + } + } + + [Fact] + public async Task Authorization_evaluation_keeps_broad_prompt_for_inconsistent_candidate_result() + { + var config = new ToolConfig { ShellMode = ShellExecutionMode.HostAllowed }; + config.AudienceProfiles.Personal.ApprovalPolicy = new ToolApprovalConfig + { + ToolOverrides = new Dictionary(StringComparer.Ordinal) + { + ["shell_execute"] = ToolApprovalMode.Approval + } + }; + var registry = new ToolRegistry(); + registry.WithFirstPartyTools( + config, + new NetclawPaths(), + new ToolPathPolicy([]), + new ShellCommandPolicy()); + var approvalService = new FixedApprovalService( + new ToolApprovalCheckResult( + ["git push"], + []) + { + CandidateChecks = + [ + new ToolApprovalCandidateCheck( + new ApprovalCandidate("git push", Directory: null), + ApprovedMatch: null) + ] + }); + var executor = new DispatchingToolExecutor( + registry, + new ToolAccessPolicy( + config, + new EffectivePolicyDefaults( + DeploymentPosture.Personal, + TrustAudience.Personal, + ShellExecutionMode.HostAllowed, + UsedStrictFallback: false), + new ShellCommandPolicy(), + new ToolPathPolicy([])), + approvalService); + var call = new FunctionCallContent( + "call-inconsistent-partial-approval", + "shell_execute", + ToolInput.Create("Command", "git status && git push")); + var context = CreateInteractivePersonalContext("signalr/thread-inconsistent-partial-approval"); + + var decision = await executor.EvaluateAuthorizationAsync( + call, + context, + TestContext.Current.CancellationToken); + + Assert.Equal(ToolAuthorizationOutcome.RequiresApproval, decision.Outcome); + Assert.Equal(["git status", "git push"], decision.ApprovalContext!.CandidateVerbs); + } + + [Fact] + public async Task Authorization_evaluation_rejects_inconsistent_all_approved_result() + { + var config = new ToolConfig { ShellMode = ShellExecutionMode.HostAllowed }; + config.AudienceProfiles.Personal.ApprovalPolicy = new ToolApprovalConfig + { + ToolOverrides = new Dictionary(StringComparer.Ordinal) + { + ["shell_execute"] = ToolApprovalMode.Approval + } + }; + var registry = new ToolRegistry(); + registry.WithFirstPartyTools( + config, + new NetclawPaths(), + new ToolPathPolicy([]), + new ShellCommandPolicy()); + var approvalService = new FixedApprovalService( + new ToolApprovalCheckResult( + [], + []) + { + CandidateChecks = + [ + new ToolApprovalCandidateCheck( + new ApprovalCandidate("git push", Directory: null), + ApprovedMatch: null) + ] + }); + var executor = new DispatchingToolExecutor( + registry, + new ToolAccessPolicy( + config, + new EffectivePolicyDefaults( + DeploymentPosture.Personal, + TrustAudience.Personal, + ShellExecutionMode.HostAllowed, + UsedStrictFallback: false), + new ShellCommandPolicy(), + new ToolPathPolicy([])), + approvalService); + var call = new FunctionCallContent( + "call-inconsistent-all-approved", + "shell_execute", + ToolInput.Create("Command", "git status && git push")); + var context = CreateInteractivePersonalContext("signalr/thread-inconsistent-all-approved"); + + var decision = await executor.EvaluateAuthorizationAsync( + call, + context, + TestContext.Current.CancellationToken); + + Assert.Equal(ToolAuthorizationOutcome.RequiresApproval, decision.Outcome); + Assert.Equal(["git status", "git push"], decision.ApprovalContext!.CandidateVerbs); + } + [Fact] public async Task Authorization_evaluation_logs_allow_reason_before_execution() { @@ -1091,8 +1344,8 @@ await approvalService.RecordApprovalAsync( var firstAttempt = await Assert.ThrowsAsync(() => executor.ExecuteAsync(call, context, TestContext.Current.CancellationToken)); - Assert.Contains("pwd", firstAttempt.ApprovalContext.Patterns); - Assert.Contains("ls", firstAttempt.ApprovalContext.Patterns); + Assert.Equal(["ls"], firstAttempt.ApprovalContext.Patterns); + Assert.Equal(["ls"], firstAttempt.ApprovalContext.CandidateVerbs); context.OneTimeApprovedToolName = call.Name; context.SetOneTimeApprovedPatterns(OneTimeApprovalKeys.Create(firstAttempt.ApprovalContext)); @@ -1463,6 +1716,8 @@ private static ToolExecutionContext CreateInteractivePersonalContext(string sess InteractiveApproval = TestToolExecutionContext.InteractiveApproval(true) }); + public static bool IsPosix => !OperatingSystem.IsWindows(); + private sealed class UnexpectedApprovalService : IToolApprovalService { public Task CheckApprovalAsync( diff --git a/src/Netclaw.Actors.Tests/Tools/ShellApprovalCaseCatalog.cs b/src/Netclaw.Actors.Tests/Tools/ShellApprovalCaseCatalog.cs index 2fb3faa96..26dc55401 100644 --- a/src/Netclaw.Actors.Tests/Tools/ShellApprovalCaseCatalog.cs +++ b/src/Netclaw.Actors.Tests/Tools/ShellApprovalCaseCatalog.cs @@ -563,7 +563,7 @@ public static class ShellApprovalCases Bash("git push | curl https://example.com"), Approvals.PersistentAnywhere("git push"), ExpectedApproval.Require( - ["git push", "curl"], + ["curl"], approvalMatches: ["persistent:git push"])), Case( "all-pipeline-clauses-approved", @@ -603,7 +603,7 @@ public static class ShellApprovalCases "side-effect-before-mutation-prompts", Bash("echo ready && git push"), Approvals.None, - ExpectedApproval.Require(["echo", "git push"])), + ExpectedApproval.Require(["git push"])), Case( "heredoc-prompts", Bash("cat <<'EOF'\nhello\nEOF"), @@ -698,7 +698,7 @@ public static class ShellApprovalCases Bash("cat config.json | jq '.items[]'", ApprovalDirectoryShape.External), Approvals.PersistentHere(ApprovalDirectoryShape.External, "jq"), ExpectedApproval.Require( - ["cat", "jq"], + ["cat"], approvalMatches: ["persistent:jq"])), Case( "workload-edit-grep-tee-pipeline-prompts", @@ -986,7 +986,7 @@ public static class ShellApprovalCases Bash("git add . && git commit -m fix && git push && gh pr merge 123"), Approvals.PersistentAnywhere("git add", "git commit", "git push"), ExpectedApproval.Require( - ["git add", "git commit", "git push", "gh pr merge"], + ["gh pr merge"], approvalMatches: [ "persistent:git add", @@ -1020,7 +1020,7 @@ public static class ShellApprovalCases "git push"), Approvals.PersistentHere(ApprovalDirectoryShape.External, "gh pr merge")), ExpectedApproval.Require( - ["git add", "git commit", "git push", "gh pr merge"], + ["gh pr merge"], approvalMatches: [ "persistent:git add", @@ -1034,7 +1034,7 @@ public static class ShellApprovalCases Approvals.Session("git add", "git commit", "git push"), Approvals.SessionForOtherSession("gh pr merge")), ExpectedApproval.Require( - ["git add", "git commit", "git push", "gh pr merge"], + ["gh pr merge"], approvalMatches: [ "session:git add", @@ -1048,7 +1048,7 @@ public static class ShellApprovalCases Approvals.PersistentAnywhere("git add", "git commit", "git push"), Approvals.PersistentForOtherAudience("gh pr merge")), ExpectedApproval.Require( - ["git add", "git commit", "git push", "gh pr merge"], + ["gh pr merge"], approvalMatches: [ "persistent:git add", diff --git a/src/Netclaw.Actors.Tests/Tools/ShellApprovalDispositionMatrixTests.Shell_approval_cases_match_review_table.verified.md b/src/Netclaw.Actors.Tests/Tools/ShellApprovalDispositionMatrixTests.Shell_approval_cases_match_review_table.verified.md index 25f4c8f5e..f00beedfd 100644 --- a/src/Netclaw.Actors.Tests/Tools/ShellApprovalDispositionMatrixTests.Shell_approval_cases_match_review_table.verified.md +++ b/src/Netclaw.Actors.Tests/Tools/ShellApprovalDispositionMatrixTests.Shell_approval_cases_match_review_table.verified.md @@ -74,14 +74,14 @@ | background-list-prompts-for-mutating-tail | Personal | Project | Interactive | git status & git push | none | RequiresApproval | approval required | none | Yes | | unbalanced-quote-fails-closed | Personal | Project | Interactive | git push "unterminated | none | RequiresApproval | approval required | none | Yes | | multiline-argument-prompts | Personal | Project | Interactive | gh issue comment 123 --body "first line\nsecond line" | none | RequiresApproval | approval required | gh issue comment | No | -| approved-pipeline-head-does-not-cover-tail | Personal | Project | Interactive | git push \| curl https://example.com | persistent[anywhere]:git push | RequiresApproval | approval required | git push, curl | No | +| approved-pipeline-head-does-not-cover-tail | Personal | Project | Interactive | git push \| curl https://example.com | persistent[anywhere]:git push | RequiresApproval | approval required | curl | No | | all-pipeline-clauses-approved | Personal | Project | Interactive | git push \| curl https://example.com | persistent[anywhere]:git push, persistent[anywhere]:curl | Allowed | StoredApproval | none | Not applicable | | input-redirect-outside-zone-prompts | Personal | Project | Interactive | cat < /etc/passwd | none | RequiresApproval | approval required | cat | No | | error-redirect-outside-zone-prompts | Personal | Project | Interactive | git status 2> {TempPath}netclaw-approval-errors.txt | none | RequiresApproval | approval required | git status | No | | cd-current-then-safe-allows | Personal | Project | Interactive | cd . && git status | none | Allowed | SafeVerbInTrustedScope | none | Not applicable | | cd-parent-then-safe-prompts | Personal | Project | Interactive | cd .. && git status | none | RequiresApproval | approval required | cd, git status | No | | multiple-cd-then-safe-prompts | Personal | Project | Interactive | cd . && cd .. && git status | none | RequiresApproval | approval required | cd, git status | No | -| side-effect-before-mutation-prompts | Personal | Project | Interactive | echo ready && git push | none | RequiresApproval | approval required | echo, git push | No | +| side-effect-before-mutation-prompts | Personal | Project | Interactive | echo ready && git push | none | RequiresApproval | approval required | git push | No | | heredoc-prompts | Personal | Project | Interactive | cat <<'EOF'\nhello\nEOF | none | RequiresApproval | approval required | none | Yes | | workload-search-rg-in-project-allows | Personal | Project | Interactive | rg -n "TODO" src | none | Allowed | SafeVerbInTrustedScope | none | Not applicable | | workload-search-grep-in-project-allows | Personal | Project | Interactive | grep -R "error" src | none | Allowed | SafeVerbInTrustedScope | none | Not applicable | @@ -99,7 +99,7 @@ | workload-search-jq-direct-prompts | Personal | Project | Interactive | jq '.items[]' config.json | none | RequiresApproval | approval required | jq | No | | workload-search-jq-direct-grant-allows | Personal | Project | Interactive | jq '.items[]' config.json | persistent[project]:jq | Allowed | StoredApproval | none | Not applicable | | workload-search-cat-jq-stored-tail-allows | Personal | Project | Interactive | cat config.json \| jq '.items[]' | persistent[project]:jq | Allowed | StoredApproval | none | Not applicable | -| workload-search-cat-jq-external-stored-tail-still-prompts | Personal | External | Interactive | cat config.json \| jq '.items[]' | persistent[external]:jq | RequiresApproval | approval required | cat, jq | No | +| workload-search-cat-jq-external-stored-tail-still-prompts | Personal | External | Interactive | cat config.json \| jq '.items[]' | persistent[external]:jq | RequiresApproval | approval required | cat | No | | workload-edit-grep-tee-pipeline-prompts | Personal | Project | Interactive | grep "error" logs/app.log \| tee reports/errors.txt | none | RequiresApproval | approval required | tee | No | | workload-edit-tee-direct-prompts | Personal | Project | Interactive | tee reports/output.txt | none | RequiresApproval | approval required | tee | No | | workload-edit-tee-direct-grant-allows | Personal | Project | Interactive | tee reports/output.txt | persistent[project]:tee | Allowed | StoredApproval | none | Not applicable | @@ -152,11 +152,11 @@ | partial-compound-grant-prompts | Personal | Project | Interactive | git status && git push | persistent[anywhere]:git status | RequiresApproval | approval required | git push | No | | four-unapproved-clauses-prompt | Personal | Project | Interactive | git add . && git commit -m fix && git push && gh pr merge 123 | none | RequiresApproval | approval required | git add, git commit, git push, gh pr merge | No | | four-anywhere-grants-allow | Personal | Project | Interactive | git add . && git commit -m fix && git push && gh pr merge 123 | persistent[anywhere]:git add, persistent[anywhere]:git commit, persistent[anywhere]:git push, persistent[anywhere]:gh pr merge | Allowed | StoredApproval | none | Not applicable | -| four-one-missing-grant-prompts | Personal | Project | Interactive | git add . && git commit -m fix && git push && gh pr merge 123 | persistent[anywhere]:git add, persistent[anywhere]:git commit, persistent[anywhere]:git push | RequiresApproval | approval required | git add, git commit, git push, gh pr merge | No | +| four-one-missing-grant-prompts | Personal | Project | Interactive | git add . && git commit -m fix && git push && gh pr merge 123 | persistent[anywhere]:git add, persistent[anywhere]:git commit, persistent[anywhere]:git push | RequiresApproval | approval required | gh pr merge | No | | four-here-grants-allow | Personal | Project | Interactive | git add . && git commit -m fix && git push && gh pr merge 123 | persistent[project]:git add, persistent[project]:git commit, persistent[project]:git push, persistent[project]:gh pr merge | Allowed | StoredApproval | none | Not applicable | -| four-one-wrong-directory-grant-prompts | Personal | Project | Interactive | git add . && git commit -m fix && git push && gh pr merge 123 | persistent[project]:git add, persistent[project]:git commit, persistent[project]:git push, persistent[external]:gh pr merge | RequiresApproval | approval required | git add, git commit, git push, gh pr merge | No | -| four-one-other-session-grant-prompts | Personal | Project | Interactive | git add . && git commit -m fix && git push && gh pr merge 123 | session[this-chat]:git add, session[this-chat]:git commit, session[this-chat]:git push, session[other-chat]:gh pr merge | RequiresApproval | approval required | git add, git commit, git push, gh pr merge | No | -| four-one-other-audience-grant-prompts | Personal | Project | Interactive | git add . && git commit -m fix && git push && gh pr merge 123 | persistent[anywhere]:git add, persistent[anywhere]:git commit, persistent[anywhere]:git push, persistent[anywhere,Team]:gh pr merge | RequiresApproval | approval required | git add, git commit, git push, gh pr merge | No | +| four-one-wrong-directory-grant-prompts | Personal | Project | Interactive | git add . && git commit -m fix && git push && gh pr merge 123 | persistent[project]:git add, persistent[project]:git commit, persistent[project]:git push, persistent[external]:gh pr merge | RequiresApproval | approval required | gh pr merge | No | +| four-one-other-session-grant-prompts | Personal | Project | Interactive | git add . && git commit -m fix && git push && gh pr merge 123 | session[this-chat]:git add, session[this-chat]:git commit, session[this-chat]:git push, session[other-chat]:gh pr merge | RequiresApproval | approval required | gh pr merge | No | +| four-one-other-audience-grant-prompts | Personal | Project | Interactive | git add . && git commit -m fix && git push && gh pr merge 123 | persistent[anywhere]:git add, persistent[anywhere]:git commit, persistent[anywhere]:git push, persistent[anywhere,Team]:gh pr merge | RequiresApproval | approval required | gh pr merge | No | | four-mixed-grant-sources-allow | Personal | Project | Interactive | git add . && git commit -m fix && git push && gh pr merge 123 | session[this-chat]:git add, session[this-chat]:gh pr merge, persistent[project]:git commit, persistent[anywhere]:git push | Allowed | StoredApproval | none | Not applicable | | safe-and-stored-authority-compose | Personal | Project | Interactive | git status && git push && git log && gh pr merge 123 | persistent[anywhere]:git push, persistent[anywhere]:gh pr merge | Allowed | StoredApproval | none | Not applicable | | four-hard-deny-beats-grants | Personal | Project | Interactive | git add . && git commit -m fix && netclaw daemon stop && git push | persistent[anywhere]:git add, persistent[anywhere]:git commit, persistent[anywhere]:netclaw daemon stop, persistent[anywhere]:git push | Denied | hard_deny_self_destructive | none | Not applicable | diff --git a/src/Netclaw.Actors.Tests/Tools/ShellApprovalDispositionMatrixTests.cs b/src/Netclaw.Actors.Tests/Tools/ShellApprovalDispositionMatrixTests.cs index c2ae5eb33..3bf2cee9c 100644 --- a/src/Netclaw.Actors.Tests/Tools/ShellApprovalDispositionMatrixTests.cs +++ b/src/Netclaw.Actors.Tests/Tools/ShellApprovalDispositionMatrixTests.cs @@ -87,6 +87,32 @@ public async Task One_time_retry_does_not_cover_a_clean_command_that_becomes_mes Assert.Empty(retry.ApprovalContext.CandidateVerbs); } + [SlopwatchSuppress("SW001", "This regression requires POSIX symlink and Bash authorization behavior.")] + [Fact(SkipUnless = nameof(IsPosix), Skip = "The symlink retry regression defines Bash authorization behavior.")] + public async Task One_time_retry_rechecks_a_candidate_whose_stored_grant_stops_matching() + { + var testCase = new ShellApprovalCase( + "one-time-retry-rechecks-stored-candidates", + new ShellApprovalInvocation("git -C repo push && gh pr merge 123"), + Approvals.PersistentHere(ApprovalDirectoryShape.Project, "git push"), + ExpectedApproval.Require(["gh pr merge"])); + await using var harness = await ShellApprovalHarness.CreateAsync( + testCase, + fixture.ActorSystem, + TestContext.Current.CancellationToken); + harness.CreateProjectDirectory("repo"); + + var initial = await harness.EvaluateDecisionAsync(TestContext.Current.CancellationToken); + Assert.Equal(["gh pr merge"], initial.ApprovalContext!.CandidateVerbs); + harness.SeedOneTimeApproval(initial.ApprovalContext); + harness.ReplaceProjectDirectoryWithExternalSymlink("repo"); + + var retry = await harness.EvaluateDecisionAsync(TestContext.Current.CancellationToken); + + Assert.Equal(ToolAuthorizationOutcome.RequiresApproval, retry.Outcome); + Assert.Equal(["git push", "gh pr merge"], retry.ApprovalContext!.CandidateVerbs); + } + [Fact] public Task Shell_approval_cases_match_review_table() => Verifier.Verify(ShellApprovalCases.RenderReviewTable(), extension: "md"); diff --git a/src/Netclaw.Actors.Tests/Tools/ToolApprovalActorTests.cs b/src/Netclaw.Actors.Tests/Tools/ToolApprovalActorTests.cs index fbcf06555..b9c378caa 100644 --- a/src/Netclaw.Actors.Tests/Tools/ToolApprovalActorTests.cs +++ b/src/Netclaw.Actors.Tests/Tools/ToolApprovalActorTests.cs @@ -375,6 +375,9 @@ [new ApprovalCandidate("cat", outsideDir)], ct); Assert.Equal(["cat"], result.UnapprovedPatterns); + var check = Assert.Single(result.CandidateChecks!); + Assert.Equal(new ApprovalCandidate("cat", outsideDir), check.Candidate); + Assert.Null(check.ApprovedMatch); Assert.Empty(result.ApprovedMatches); } finally @@ -383,6 +386,53 @@ [new ApprovalCandidate("cat", outsideDir)], } } + [Fact] + public async Task Partial_directory_grant_returns_exact_unapproved_occurrence() + { + var ct = TestContext.Current.CancellationToken; + var tempFile = Path.GetTempFileName(); + try + { + var grantDir = Path.Combine(Path.GetTempPath(), "netclaw-approval", "repo"); + var approvedDir = Path.Combine(grantDir, "src"); + var unapprovedDir = Path.Combine(Path.GetTempPath(), "netclaw-approval", "external"); + var approvedCandidate = new ApprovalCandidate("git push", approvedDir); + var unapprovedCandidate = new ApprovalCandidate("git push", unapprovedDir); + + var store = new ToolApprovalStore(tempFile); + store.AddApproval( + TrustAudience.Personal, + "shell_execute", + new ApprovalEntry("git push") { Directory = grantDir }); + + var actor = Sys.ActorOf(ToolApprovalActor.CreateProps(store)); + var service = CreateService(actor); + + var result = await service.CheckApprovalAsync( + "session-a", + TrustAudience.Personal, + new ToolName("shell_execute"), + [approvedCandidate, unapprovedCandidate], + cwd: grantDir, + ct); + + Assert.Equal(["git push"], result.UnapprovedPatterns); + Assert.Equal( + [ + new ToolApprovalCandidateCheck(approvedCandidate, result.ApprovedMatches[0]), + new ToolApprovalCandidateCheck(unapprovedCandidate, ApprovedMatch: null) + ], + result.CandidateChecks); + var match = Assert.Single(result.ApprovedMatches); + Assert.Equal("persistent", match.Source); + Assert.Equal($"git push in {grantDir}", match.Scope); + } + finally + { + File.Delete(tempFile); + } + } + [Fact] public async Task Directory_near_miss_is_logged_without_changing_the_decision() { diff --git a/src/Netclaw.Actors/Tools/DispatchingToolExecutor.cs b/src/Netclaw.Actors/Tools/DispatchingToolExecutor.cs index 4540df19f..e35bdd4e9 100644 --- a/src/Netclaw.Actors/Tools/DispatchingToolExecutor.cs +++ b/src/Netclaw.Actors/Tools/DispatchingToolExecutor.cs @@ -352,17 +352,42 @@ internal async Task EvaluateAuthorizationAsync( context.Approval.Cwd, ct); approvalMatches = approvalCheck.ApprovedMatches; + var hasExactCandidateChecks = TryGetExactUnapprovedCandidates( + approvalCheck, + candidatesForCheck, + out var unapprovedCandidates); + var hasInconsistentCandidateChecks = approvalCheck.CandidateChecks is not null + && !hasExactCandidateChecks; - if (approvalCheck.UnapprovedPatterns.Count == 0) + if (approvalCheck.UnapprovedPatterns.Count == 0 + && !hasInconsistentCandidateChecks) { context.Approval.ApplyDecision( "PreviouslyApproved", FormatApprovalMatches(approvalCheck.ApprovedMatches)); } - accessDecision = approvalCheck.UnapprovedPatterns.Count == 0 - ? ToolAccessDecision.Allow(ToolAllowReason.StoredApproval) - : ToolAccessDecision.RequiresApproval(approvalContext); + if (approvalCheck.UnapprovedPatterns.Count == 0 + && !hasInconsistentCandidateChecks) + { + accessDecision = ToolAccessDecision.Allow(ToolAllowReason.StoredApproval); + } + else + { + // New approval services return exact candidate occurrences. + // An older implementation can only return verb strings, so + // keep the broader context instead of guessing which scoped + // candidate lacks approval. + var promptContext = hasExactCandidateChecks + && unapprovedCandidates.Count > 0 + && string.Equals(tool.Name, ShellTool.ToolName, StringComparison.Ordinal) + ? ToolAccessPolicy.NarrowShellApprovalContext( + approvalContext, + unapprovedCandidates, + context.SessionDirectory) + : approvalContext; + accessDecision = ToolAccessDecision.RequiresApproval(promptContext); + } } } } @@ -428,6 +453,48 @@ private static ToolAuthorizationDecision CompleteAuthorizationDecision( approvalMatches); } + private static bool TryGetExactUnapprovedCandidates( + ToolApprovalCheckResult result, + IReadOnlyList checkedCandidates, + out IReadOnlyList unapprovedCandidates) + { + unapprovedCandidates = []; + if (result.CandidateChecks is not { } candidateChecks + || candidateChecks.Count != checkedCandidates.Count) + { + return false; + } + + var exactUnapprovedCandidates = new List(); + var exactUnapprovedPatterns = new List(); + var exactApprovedMatches = new List(); + for (var index = 0; index < candidateChecks.Count; index++) + { + var check = candidateChecks[index]; + if (check.Candidate != checkedCandidates[index]) + return false; + + if (check.ApprovedMatch is { } approvedMatch) + exactApprovedMatches.Add(approvedMatch); + else + { + exactUnapprovedCandidates.Add(check.Candidate); + exactUnapprovedPatterns.Add(check.Candidate.Verb); + } + } + + if (!exactUnapprovedPatterns.SequenceEqual( + result.UnapprovedPatterns, + StringComparer.OrdinalIgnoreCase) + || !exactApprovedMatches.SequenceEqual(result.ApprovedMatches)) + { + return false; + } + + unapprovedCandidates = exactUnapprovedCandidates; + return true; + } + private void LogAuthorizationDecision(string toolName, ToolAuthorizationDecision decision) { switch (decision.Outcome) diff --git a/src/Netclaw.Actors/Tools/ToolAccessPolicy.cs b/src/Netclaw.Actors/Tools/ToolAccessPolicy.cs index 92d945ded..88e23534c 100644 --- a/src/Netclaw.Actors/Tools/ToolAccessPolicy.cs +++ b/src/Netclaw.Actors/Tools/ToolAccessPolicy.cs @@ -409,6 +409,32 @@ private ToolAccessDecision CheckApprovalGate( return ToolAccessDecision.RequiresApproval(approvalContext); } + internal static ToolApprovalContext NarrowShellApprovalContext( + ToolApprovalContext context, + IReadOnlyList unapprovedCandidates, + string? sessionDirectory) + { + var candidateVerbs = unapprovedCandidates + .Select(static candidate => candidate.Verb) + .Distinct(StringComparer.OrdinalIgnoreCase) + .ToList(); + var options = BuildApprovalOptions( + isMessy: false, + isCwdShallow: IsCwdTooShallow(context.Cwd), + allEffectiveDirsAreSessionScratch: AllCandidatesResolveToSessionScratch( + unapprovedCandidates, context.Cwd, sessionDirectory), + supportsDirectoryScope: true, + isMcpTool: false); + + return context with + { + Patterns = candidateVerbs, + CandidateVerbs = candidateVerbs, + Candidates = unapprovedCandidates, + Options = options + }; + } + /// /// Returns true when every candidate's effective directory resolves to /// the session's ephemeral session_dir. Persisting an "Always diff --git a/src/Netclaw.Actors/Tools/ToolApprovalActor.cs b/src/Netclaw.Actors/Tools/ToolApprovalActor.cs index 8fccdc6f1..725c6a066 100644 --- a/src/Netclaw.Actors/Tools/ToolApprovalActor.cs +++ b/src/Netclaw.Actors/Tools/ToolApprovalActor.cs @@ -36,10 +36,12 @@ public ToolApprovalActor(ToolApprovalStore? persistentStore = null) : (IReadOnlyList)[]; var unapproved = new List(msg.Candidates.Count); + var candidateChecks = new List(msg.Candidates.Count); var approvedMatches = new List(msg.Candidates.Count); foreach (var candidate in msg.Candidates) { var match = MatchApproval(msg.SessionId, msg.Audience, msg.ToolName, candidate, msg.Cwd, approved); + candidateChecks.Add(new ToolApprovalCandidateCheck(candidate, match)); if (match is null) { unapproved.Add(candidate.Verb); @@ -50,7 +52,11 @@ public ToolApprovalActor(ToolApprovalStore? persistentStore = null) approvedMatches.Add(match); } - Sender.Tell(new UnapprovedPatternsResponse(new ToolApprovalCheckResult(unapproved, approvedMatches))); + Sender.Tell(new UnapprovedPatternsResponse( + new ToolApprovalCheckResult(unapproved, approvedMatches) + { + CandidateChecks = candidateChecks + })); }); Receive(msg => diff --git a/src/Netclaw.Security/IToolApprovalService.cs b/src/Netclaw.Security/IToolApprovalService.cs index 9318c4dfa..d223c7677 100644 --- a/src/Netclaw.Security/IToolApprovalService.cs +++ b/src/Netclaw.Security/IToolApprovalService.cs @@ -65,7 +65,19 @@ public readonly record struct ToolApprovalSessionId(string Value) public sealed record ToolApprovalCheckResult( IReadOnlyList UnapprovedPatterns, - IReadOnlyList ApprovedMatches); + IReadOnlyList ApprovedMatches) +{ + /// + /// Gets one ordered disposition for each checked candidate. A null value + /// means the approval service implements the earlier aggregate result. + /// Callers must retain the full prompt candidate set in that case. + /// + public IReadOnlyList? CandidateChecks { get; init; } +} + +public sealed record ToolApprovalCandidateCheck( + ApprovalCandidate Candidate, + ToolApprovalMatch? ApprovedMatch); public sealed record ToolApprovalMatch( string Pattern,