diff --git a/openspec/changes/simplify-shell-policy-evaluator/evidence/refactor-reduction-revision.md b/openspec/changes/simplify-shell-policy-evaluator/evidence/refactor-reduction-revision.md index b00d097de..18667465f 100644 --- a/openspec/changes/simplify-shell-policy-evaluator/evidence/refactor-reduction-revision.md +++ b/openspec/changes/simplify-shell-policy-evaluator/evidence/refactor-reduction-revision.md @@ -60,6 +60,16 @@ The slice removes another 45 production lines and eight test lines. It does not The cumulative footprint uses 8,520 lines and 521 control-flow lines. The complete reduction gate remains open. +## Unified reviewed-safe path slice + +This slice removes the unused aggregate reviewed-safe route. The compatibility entry now projects candidate-scoped path facts and calls the same reviewed-safe method as the coordinator. + +The evaluation also reuses the projection's immutable candidate snapshot, and the path-fact projection returns its immutable candidate list directly instead of wrapping a second indexed container. + +The slice removes another 57 production lines and four control-flow lines. It adds 19 test lines while moving the existing direct safe-policy cases onto the shared typed route. + +The cumulative footprint uses 8,463 lines and 517 control-flow lines. The complete reduction gate remains open. + ## Preliminary coverage and risk The audit used `dotnet-coverage` 18.10.0 and `crap4dotnet` 0.1.1. diff --git a/src/Netclaw.Actors.Tests/Tools/ScopedShellSafeVerbPolicyTests.cs b/src/Netclaw.Actors.Tests/Tools/ScopedShellSafeVerbPolicyTests.cs index 70358eae4..956094c8a 100644 --- a/src/Netclaw.Actors.Tests/Tools/ScopedShellSafeVerbPolicyTests.cs +++ b/src/Netclaw.Actors.Tests/Tools/ScopedShellSafeVerbPolicyTests.cs @@ -95,7 +95,19 @@ private static bool ShortCircuits( string verb, string? cwd, ToolInvocationContext context) => - policy.AllShortCircuit([Candidate(verb)], cwd, context); + AllShortCircuit(policy, [Candidate(verb)], cwd, context); + + private static bool AllShortCircuit( + ScopedShellSafeVerbPolicy policy, + IReadOnlyList candidates, + string? cwd, + ToolInvocationContext context) + { + if (candidates.Count == 0) + return false; + + return candidates.All(candidate => policy.ShortCircuits(candidate, cwd, context)); + } private ToolInvocationContext PersonalContext(string? projectDir = null, string? sessionDir = null) => TestToolExecutionContext.CreateBound("session-1", sessionDir ?? _sessionDir, new TestToolExecutionContextOptions @@ -191,7 +203,7 @@ public void All_short_circuit_returns_false_when_any_verb_is_unsafe() var policy = new ScopedShellSafeVerbPolicy(VerbList("grep", "cat")); var ctx = PersonalContext(projectDir: _projectDir); - Assert.False(policy.AllShortCircuit(Candidates("grep", "git push"), _projectDir, ctx)); + Assert.False(AllShortCircuit(policy, Candidates("grep", "git push"), _projectDir, ctx)); } [Fact] @@ -200,7 +212,7 @@ public void All_short_circuit_returns_true_when_every_verb_is_safe_and_in_space( var policy = new ScopedShellSafeVerbPolicy(VerbList("grep", "cat", "wc")); var ctx = PersonalContext(projectDir: _projectDir); - Assert.True(policy.AllShortCircuit(Candidates("grep", "cat", "wc"), _projectDir, ctx)); + Assert.True(AllShortCircuit(policy, Candidates("grep", "cat", "wc"), _projectDir, ctx)); } [Fact] @@ -209,7 +221,7 @@ public void Empty_candidate_list_does_not_short_circuit() var policy = new ScopedShellSafeVerbPolicy(VerbList("grep")); var ctx = PersonalContext(projectDir: _projectDir); - Assert.False(policy.AllShortCircuit([], _projectDir, ctx)); + Assert.False(AllShortCircuit(policy, [], _projectDir, ctx)); } [Fact] @@ -244,7 +256,11 @@ public void New_safe_verb_chained_with_mutating_verb_still_prompts() var policy = new ScopedShellSafeVerbPolicy(VerbList("whoami")); var ctx = PersonalContext(projectDir: _projectDir); - Assert.False(policy.AllShortCircuit(Candidates("whoami", "git push origin main"), _projectDir, ctx)); + Assert.False(AllShortCircuit( + policy, + Candidates("whoami", "git push origin main"), + _projectDir, + ctx)); } [Fact] @@ -253,7 +269,8 @@ public void Reviewed_phrase_matches_a_longer_canonical_token_chain() var policy = new ScopedShellSafeVerbPolicy(VerbList("git ls-tree")); var ctx = PersonalContext(projectDir: _projectDir); - Assert.True(policy.AllShortCircuit( + Assert.True(AllShortCircuit( + policy, [Candidate("git ls-tree feature", _projectDir)], _projectDir, ctx)); @@ -279,7 +296,7 @@ public void Argument_before_reviewed_phrase_stays_strict(string commandTemplate) ["WorkingDirectory"] = _projectDir }); - Assert.False(policy.AllShortCircuit(candidates, _projectDir, ctx)); + Assert.False(AllShortCircuit(policy, candidates, _projectDir, ctx)); } [Theory] @@ -302,7 +319,7 @@ public void Path_shaped_option_operand_outside_safe_root_stays_strict( ["WorkingDirectory"] = _projectDir }); - Assert.False(policy.AllShortCircuit(candidates, _projectDir, ctx)); + Assert.False(AllShortCircuit(policy, candidates, _projectDir, ctx)); } [Fact] @@ -320,7 +337,7 @@ public void Path_shaped_option_operand_under_safe_root_remains_eligible() ["WorkingDirectory"] = _projectDir }); - Assert.True(policy.AllShortCircuit(candidates, _projectDir, ctx)); + Assert.True(AllShortCircuit(policy, candidates, _projectDir, ctx)); } [Fact] @@ -337,7 +354,7 @@ public void Path_shaped_data_under_safe_root_does_not_create_new_authority() ["WorkingDirectory"] = _projectDir }); - Assert.True(policy.AllShortCircuit(candidates, _projectDir, ctx)); + Assert.True(AllShortCircuit(policy, candidates, _projectDir, ctx)); } [Fact] @@ -361,7 +378,7 @@ public void PowerShell_compatibility_paths_use_posix_host_roots() ["WorkingDirectory"] = _projectDir }); - Assert.True(policy.AllShortCircuit(candidates, _projectDir, ctx)); + Assert.True(AllShortCircuit(policy, candidates, _projectDir, ctx)); } [Fact] @@ -370,7 +387,8 @@ public void Prefix_collision_does_not_match_reviewed_phrase() var policy = new ScopedShellSafeVerbPolicy(VerbList("git ls-tree")); var ctx = PersonalContext(projectDir: _projectDir); - Assert.False(policy.AllShortCircuit( + Assert.False(AllShortCircuit( + policy, [Candidate("git ls-treex feature", _projectDir)], _projectDir, ctx)); @@ -383,7 +401,7 @@ public void Candidate_without_canonical_tokens_stays_strict() var ctx = PersonalContext(projectDir: _projectDir); var candidate = new ApprovalCandidate("head", _projectDir); - Assert.False(policy.AllShortCircuit([candidate], _projectDir, ctx)); + Assert.False(AllShortCircuit(policy, [candidate], _projectDir, ctx)); } [Fact] @@ -392,7 +410,8 @@ public void Candidate_from_another_shell_stays_strict() var policy = new ScopedShellSafeVerbPolicy(VerbList("Get-Content")); var ctx = PersonalContext(projectDir: _projectDir); - Assert.False(policy.AllShortCircuit( + Assert.False(AllShortCircuit( + policy, [Candidate("Get-Content", _projectDir, ApprovalShell.PowerShell)], _projectDir, ctx)); @@ -413,7 +432,7 @@ public void Dotted_symlink_directory_does_not_short_circuit() var ctx = PersonalContext(projectDir: _projectDir); var candidate = Candidate("find", link); - Assert.False(policy.AllShortCircuit([candidate], _projectDir, ctx)); + Assert.False(AllShortCircuit(policy, [candidate], _projectDir, ctx)); } finally { @@ -428,7 +447,7 @@ public void Candidate_path_outside_safe_spaces_falls_through_to_prompt() var ctx = PersonalContext(projectDir: _projectDir); var candidates = new[] { Candidate("cat", _outsideDir) }; - Assert.False(policy.AllShortCircuit(candidates, _projectDir, ctx)); + Assert.False(AllShortCircuit(policy, candidates, _projectDir, ctx)); } [Fact] diff --git a/src/Netclaw.Actors.Tests/Tools/ShellPolicyEvaluationTests.cs b/src/Netclaw.Actors.Tests/Tools/ShellPolicyEvaluationTests.cs index 69d92ac16..a39f98155 100644 --- a/src/Netclaw.Actors.Tests/Tools/ShellPolicyEvaluationTests.cs +++ b/src/Netclaw.Actors.Tests/Tools/ShellPolicyEvaluationTests.cs @@ -141,7 +141,7 @@ public void Causal_protected_path_check_denies_an_invalid_known_value() var consumer = Assert.Single( evaluation.Candidates, static candidate => candidate.Role == ShellPolicyCandidateRole.CausalIntentConsumer); - var facts = evaluation.Projection.PathFacts.For(consumer.Id); + var facts = evaluation.Projection.PathFacts[consumer.Id.Value]; var source = Assert.Single( Assert.IsType(facts.Intent).Facts, static fact => fact.Source.Origin == ShellPolicyPathOrigin.EffectiveArgument).Source; @@ -167,7 +167,7 @@ public void Causal_protected_path_check_does_not_treat_unknown_as_a_denied_path( var consumer = Assert.Single( evaluation.Candidates, static candidate => candidate.Role == ShellPolicyCandidateRole.CausalIntentConsumer); - var facts = evaluation.Projection.PathFacts.For(consumer.Id); + var facts = evaluation.Projection.PathFacts[consumer.Id.Value]; var environment = ShellExecutionEnvironment.CreatePowerShell( @"C:\Program Files\PowerShell\7\pwsh.exe", PwshDialect.PowerShell7); @@ -502,7 +502,7 @@ public async Task Reviewed_safe_real_scope_stage_uses_projected_path_facts( "; ", evaluation.Candidates.Select(candidate => { - var facts = evaluation.Projection.PathFacts.For(candidate.Id); + var facts = evaluation.Projection.PathFacts[candidate.Id.Value]; return $"{candidate.Candidate.Verb}: " + $"directory={candidate.Candidate.Directory}; " + $"sourceCwd={candidate.SourceOccurrence?.WorkingDirectory}; " @@ -519,7 +519,7 @@ public void Unproved_non_file_semantics_keep_reviewed_safe_policy_strict() interactive: true, "tr"); var candidate = Assert.Single(evaluation.Candidates); - var facts = evaluation.Projection.PathFacts.For(candidate.Id); + var facts = evaluation.Projection.PathFacts[candidate.Id.Value]; var real = Assert.IsType(facts.Real); var invalid = facts with { @@ -733,7 +733,7 @@ public void Path_facts_preserve_candidate_and_real_scope_identity() "head"); var candidate = Assert.Single(evaluation.Candidates); - var facts = evaluation.Projection.PathFacts.For(candidate.Id); + var facts = evaluation.Projection.PathFacts[candidate.Id.Value]; Assert.Same(candidate.SourceOccurrence, facts.SourceOccurrence); Assert.Equal(ShellPolicyPathResolutionState.Known, facts.RealScope.State); @@ -805,7 +805,7 @@ public void Path_facts_keep_candidate_scope_separate_from_the_command_base() evaluation.Candidates, static candidate => candidate.Candidate.Directory == "/work/sub"); - var facts = evaluation.Projection.PathFacts.For(candidate.Id); + var facts = evaluation.Projection.PathFacts[candidate.Id.Value]; Assert.Equal("/work/sub", facts.RealScope.Path?.Value); Assert.Equal("/work", facts.Real?.ResolutionBase.Path?.Value); @@ -825,7 +825,7 @@ public void Causal_path_facts_keep_intent_and_fallback_resolutions_distinct() evaluation.Candidates, static candidate => candidate.Role == ShellPolicyCandidateRole.CausalIntentConsumer); - var facts = evaluation.Projection.PathFacts.For(candidate.Id); + var facts = evaluation.Projection.PathFacts[candidate.Id.Value]; Assert.Equal("/tmp", facts.IntentScope?.Path?.Value); Assert.Contains(facts.FallbackScopes, scope => scope.Path?.Value == "/work"); diff --git a/src/Netclaw.Actors/Tools/ScopedShellSafeVerbPolicy.cs b/src/Netclaw.Actors/Tools/ScopedShellSafeVerbPolicy.cs index 78895c8ab..049c931ed 100644 --- a/src/Netclaw.Actors/Tools/ScopedShellSafeVerbPolicy.cs +++ b/src/Netclaw.Actors/Tools/ScopedShellSafeVerbPolicy.cs @@ -35,25 +35,6 @@ public ScopedShellSafeVerbPolicy(SafeVerbList safeVerbs) _safeVerbs = safeVerbs; } - /// - /// Returns true when each candidate has a safe verb and a safe effective - /// directory. The candidate directory takes precedence over the cwd. - /// - public bool AllShortCircuit( - IReadOnlyList candidates, - string? cwd, - ToolInvocationContext context) - { - if (candidates.Count == 0) - return false; - - return candidates.All(candidate => ShortCircuits( - candidate, - candidate.SourceOccurrence, - cwd, - context)); - } - /// /// Returns true when declaring as the project root /// would make every candidate eligible for the reviewed-safe short circuit. @@ -204,33 +185,18 @@ internal bool ShortCircuitsCausalIntent( internal bool ShortCircuits( ApprovalCandidate candidate, - CommandOccurrence? sourceOccurrence, string? cwd, ToolInvocationContext context) { - var safeRoots = ResolveSafeSpaceRoots(context); - var resolvedPaths = ResolveCompatibilityPaths(candidate, sourceOccurrence); - if (safeRoots.Count == 0 - || !IsReviewedDiagnostic(candidate, sourceOccurrence, safeRoots, resolvedPaths)) - { - return false; - } - - var effectiveDirectory = candidate.Directory ?? cwd; - if (string.IsNullOrWhiteSpace(effectiveDirectory)) - return false; - - try - { - var fullDirectory = Path.GetFullPath(effectiveDirectory); - return safeRoots.Any(root => IsSafePath(fullDirectory, root)); - } - catch (Exception ex) when (ex is ArgumentException - or NotSupportedException - or PathTooLongException) - { - return false; - } + var projected = new ShellPolicyCandidate( + new ShellPolicyCandidateId(0), + candidate.Directory is null ? candidate with { Directory = cwd } : candidate, + candidate.SourceOccurrence); + var pathStyle = OperatingSystem.IsWindows() + ? ShellPathStyle.Windows + : ShellPathStyle.Posix; + var facts = ShellPolicyPathFacts.Create([projected], pathStyle); + return ShortCircuits(projected.Candidate, facts[projected.Id.Value], context); } internal bool ShortCircuits( diff --git a/src/Netclaw.Actors/Tools/ShellPolicyCoordinator.cs b/src/Netclaw.Actors/Tools/ShellPolicyCoordinator.cs index 608c477e7..3d6256a66 100644 --- a/src/Netclaw.Actors/Tools/ShellPolicyCoordinator.cs +++ b/src/Netclaw.Actors/Tools/ShellPolicyCoordinator.cs @@ -287,7 +287,7 @@ internal static ShellPolicyStageResult ProtectedCausalPaths( return evaluation.Candidates.Any(candidate => candidate.Role == ShellPolicyCandidateRole.CausalIntentConsumer && policy.CausalIntentReferencesProtectedPath( - evaluation.Projection.PathFacts.For(candidate.Id))) + evaluation.Projection.PathFacts[candidate.Id.Value])) ? new ShellPolicyStageResult.Complete( ToolAuthorizationDecision.Deny("shell_references_protected_path")) : new ShellPolicyStageResult.Continue(); @@ -448,7 +448,7 @@ internal static ShellPolicyStageResult RealScope( { if (!policy.IsReviewedSafeCandidate( candidate.Candidate, - evaluation.Projection.PathFacts.For(candidate.Id), + evaluation.Projection.PathFacts[candidate.Id.Value], invocation)) { continue; @@ -485,7 +485,7 @@ internal static ShellPolicyStageResult IntentScope( !evaluation.IsCovered(prerequisite)) || !policy.IsReviewedSafeIntentCandidate( candidate.Candidate, - evaluation.Projection.PathFacts.For(candidate.Id), + evaluation.Projection.PathFacts[candidate.Id.Value], invocation)) { continue; diff --git a/src/Netclaw.Actors/Tools/ShellPolicyEvaluation.cs b/src/Netclaw.Actors/Tools/ShellPolicyEvaluation.cs index c64d3f7d5..d228d0a22 100644 --- a/src/Netclaw.Actors/Tools/ShellPolicyEvaluation.cs +++ b/src/Netclaw.Actors/Tools/ShellPolicyEvaluation.cs @@ -147,8 +147,6 @@ internal ShellPolicyAuthorization( internal sealed class ShellPolicyEvaluation { - private readonly ShellPolicyCandidate[] _candidates; - private readonly IReadOnlyList _candidateView; private readonly ShellPolicyCoverageSource[] _coverage; private readonly ShellPolicyDecisionTraceBuilder _trace = new(); private ToolAuthorizationDecision? _terminalDecision; @@ -162,19 +160,17 @@ internal ShellPolicyEvaluation(ShellPolicyProjection projection) ArgumentNullException.ThrowIfNull(projection); Projection = projection; - _candidates = projection.Candidates.ToArray(); - _candidateView = Array.AsReadOnly(_candidates); - _coverage = new ShellPolicyCoverageSource[_candidates.Length]; + _coverage = new ShellPolicyCoverageSource[projection.Candidates.Count]; } internal ShellPolicyProjection Projection { get; } - internal IReadOnlyList Candidates => _candidateView; + internal IReadOnlyList Candidates => Projection.Candidates; internal bool AllCovered => !_coverage.Contains(ShellPolicyCoverageSource.Uncovered); internal IReadOnlyList UncoveredCandidates => - Array.AsReadOnly(_candidates + Array.AsReadOnly(Projection.Candidates .Where((_, index) => _coverage[index] == ShellPolicyCoverageSource.Uncovered) .ToArray()); @@ -271,10 +267,10 @@ internal ShellPolicyStageResult Cover( return new ShellPolicyStageResult.Complete(_terminalDecision); var index = candidate.Id.Value; - if ((uint)index >= (uint)_candidates.Length) + if ((uint)index >= (uint)Candidates.Count) return Fail(ShellPolicyFault.InvalidCandidateId); - if (!ReferenceEquals(candidate, _candidates[index])) + if (!ReferenceEquals(candidate, Projection.Candidates[index])) return Fail(ShellPolicyFault.CandidateFactsChanged); if (_coverage[index] != ShellPolicyCoverageSource.Uncovered) @@ -310,7 +306,7 @@ internal ShellPolicyStageResult Complete( ToolAuthorizationOutcome.Allowed => AllCovered || allowsUncoveredOneTime && decision.AllowReason == ToolAllowReason.OneTimeApproval, - ToolAuthorizationOutcome.RequiresApproval => _candidates.Length == 0 || !AllCovered, + ToolAuthorizationOutcome.RequiresApproval => Candidates.Count == 0 || !AllCovered, ToolAuthorizationOutcome.Denied => true, _ => false, }; diff --git a/src/Netclaw.Actors/Tools/ShellPolicyPathFacts.cs b/src/Netclaw.Actors/Tools/ShellPolicyPathFacts.cs index 44594baba..dc4aa18cf 100644 --- a/src/Netclaw.Actors/Tools/ShellPolicyPathFacts.cs +++ b/src/Netclaw.Actors/Tools/ShellPolicyPathFacts.cs @@ -61,28 +61,9 @@ internal sealed record ShellPolicyCandidatePathFacts( ShellPolicyResolvedPathView? Intent, IReadOnlyList Fallbacks); -internal sealed class ShellPolicyPathFacts +internal static class ShellPolicyPathFacts { - private readonly ShellPolicyCandidatePathFacts[] _candidates; - - private ShellPolicyPathFacts(ShellPolicyCandidatePathFacts[] candidates) - { - _candidates = candidates; - Candidates = Array.AsReadOnly(candidates); - } - - internal IReadOnlyList Candidates { get; } - - internal ShellPolicyCandidatePathFacts For(ShellPolicyCandidateId candidateId) - { - var index = candidateId.Value; - if ((uint)index >= (uint)_candidates.Length) - throw new ArgumentOutOfRangeException(nameof(candidateId)); - - return _candidates[index]; - } - - internal static ShellPolicyPathFacts Create( + internal static IReadOnlyList Create( IReadOnlyList candidates, ShellPathStyle pathStyle) { @@ -149,7 +130,7 @@ is ShellValueDomain.Exact exact Array.AsReadOnly(fallbacks)); } - return new ShellPolicyPathFacts(projected); + return Array.AsReadOnly(projected); } internal static ShellPolicyScopePathFact ResolveScope( diff --git a/src/Netclaw.Actors/Tools/ShellPolicyProjection.cs b/src/Netclaw.Actors/Tools/ShellPolicyProjection.cs index c02d996f7..5d1cad204 100644 --- a/src/Netclaw.Actors/Tools/ShellPolicyProjection.cs +++ b/src/Netclaw.Actors/Tools/ShellPolicyProjection.cs @@ -80,7 +80,7 @@ private ShellPolicyProjection( ToolRunScope runScope, ToolApprovalContext approvalContext, IReadOnlyList candidates, - ShellPolicyPathFacts pathFacts, + IReadOnlyList pathFacts, IReadOnlySet approvedOneTimeKeys, string? approvedOneTimeToolName) { @@ -109,7 +109,7 @@ private ShellPolicyProjection( internal IReadOnlyList Candidates { get; } - internal ShellPolicyPathFacts PathFacts { get; } + internal IReadOnlyList PathFacts { get; } internal IReadOnlySet ApprovedOneTimeKeys { get; } diff --git a/src/Netclaw.Actors/Tools/ToolAccessPolicy.cs b/src/Netclaw.Actors/Tools/ToolAccessPolicy.cs index 3adb4984a..775d46bf0 100644 --- a/src/Netclaw.Actors/Tools/ToolAccessPolicy.cs +++ b/src/Netclaw.Actors/Tools/ToolAccessPolicy.cs @@ -629,8 +629,8 @@ private ToolAccessDecision CheckApprovalGate( if (!deferReviewedSafeCoverage) { approvalCandidates = approvalCandidates - .Where(candidate => !_safeVerbPolicy.AllShortCircuit( - [candidate], + .Where(candidate => !_safeVerbPolicy.ShortCircuits( + candidate, context.Approval.Cwd, context.Invocation)) .ToList();