diff --git a/IMPLEMENTATION_PLAN.md b/IMPLEMENTATION_PLAN.md index f07ab8ff7..d30e4038e 100644 --- a/IMPLEMENTATION_PLAN.md +++ b/IMPLEMENTATION_PLAN.md @@ -207,6 +207,9 @@ Done when: can persist. - [x] A prompt excludes candidates that existing session or persistent grants already cover, while it preserves exact directory-scoped occurrences. +- [x] Reviewed-safe phrases grant no implicit authority to a headless, reminder, + or webhook run. Unattended candidates need explicit one-time or stored-grant + authority; approval-exempt shell side effects retain their bounded exception. - [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/openspec/changes/structure-shell-approval-policy/specs/tool-approval-gates/spec.md b/openspec/changes/structure-shell-approval-policy/specs/tool-approval-gates/spec.md index 914ff56cf..d55b94514 100644 --- a/openspec/changes/structure-shell-approval-policy/specs/tool-approval-gates/spec.md +++ b/openspec/changes/structure-shell-approval-policy/specs/tool-approval-gates/spec.md @@ -21,12 +21,18 @@ own or inspect one-time approval state. The coordinator SHALL import actor coverage, apply safe policy to still-uncovered candidates, validate the invocation-owned one-time set exactly, and SHALL NOT rescan grants. +Reviewed-safe phrase coverage SHALL cover a candidate only when the run has +interactive approval capability. A run without that capability SHALL require +explicit one-time, session, or persistent authority for every candidate that +is not an approval-exempt side effect. + The actor result SHALL include typed persistent-store status. An absent store file SHALL be ready with an empty snapshot. Expected corruption or migration failure SHALL be unavailable. Completion SHALL allow a call fully covered by -one-time, session, or reviewed-safe authority without persistent state. If any -candidate remains uncovered and persistent state was unavailable, completion -SHALL return terminal `ApprovalStoreUnavailable` instead of a prompt. +one-time, session, approval-exempt side effects, or, for an interactive run, +reviewed-safe phrase coverage without persistent state. If any candidate +remains uncovered and persistent state was unavailable, completion SHALL +return terminal `ApprovalStoreUnavailable` instead of a prompt. `ToolApprovalAttempt` SHALL remain owner of one-time invocation state. `ToolApprovalActor` SHALL remain owner of session and persistent grants. The @@ -49,11 +55,28 @@ a fact that no current type represents. #### Scenario: Independent coverage survives unavailable persistence - **GIVEN** the persistent store is unavailable +- **AND** interactive approval capability is available - **AND** session and reviewed-safe coverage jointly cover every candidate - **WHEN** completion evaluates the actor result - **THEN** the call is allowed - **AND** no persisted grant is assumed +#### Scenario: Reviewed-safe policy does not grant headless authority + +- **GIVEN** interactive approval capability is unavailable +- **AND** a complete candidate is in the reviewed-safe catalog +- **WHEN** no one-time, session, or persistent grant covers that candidate +- **THEN** the candidate remains uncovered +- **AND** the caller follows the current unsupported-channel denial path + +#### Scenario: Explicit grant covers a headless candidate + +- **GIVEN** interactive approval capability is unavailable +- **AND** a session or persistent grant covers a complete candidate +- **WHEN** completion evaluates the call +- **THEN** the explicit grant covers that candidate +- **AND** reviewed-safe policy adds no authority + #### Scenario: Uncovered candidate fails closed when persistence is unavailable - **GIVEN** the persistent store is unavailable diff --git a/openspec/changes/structure-shell-approval-policy/tasks.md b/openspec/changes/structure-shell-approval-policy/tasks.md index 965afdd3f..c1ca3edf9 100644 --- a/openspec/changes/structure-shell-approval-policy/tasks.md +++ b/openspec/changes/structure-shell-approval-policy/tasks.md @@ -42,13 +42,18 @@ covered and call-level invariants pass. - [x] 3.3 Make internal exceptions, invalid enums, duplicate candidate IDs, mismatched actor results, and impossible transitions terminal deny. -- [x] 3.4 Allow fully one-time/session/safe-covered calls when persistent state - is unavailable; deny with `ApprovalStoreUnavailable` instead of prompting - when any candidate still depends on that state. +- [x] 3.4 Allow calls covered by one-time or session authority and + approval-exempt side effects when persistent state is unavailable. Also allow + reviewed-safe phrase coverage for an interactive run. Deny with + `ApprovalStoreUnavailable` rather than open a prompt when any candidate still + depends on persistent state. - [ ] 3.5 Let expected unresolved shell input offer only one-time approval and deny; never create a reusable candidate. - [ ] 3.6 Keep legacy token scans deny-only and prove they cannot authorize, create persistence choices, or widen scope. +- [x] 3.7 Apply reviewed-safe phrase coverage only when interactive approval is + available. Prove that unattended calls need explicit one-time or stored-grant + authority while approval-exempt side effects keep their current behavior. ## 4. Typed grant phrases and persistence diff --git a/src/Netclaw.Actors.Tests/Tools/ShellApprovalDispositionMatrixTests.cs b/src/Netclaw.Actors.Tests/Tools/ShellApprovalDispositionMatrixTests.cs index 6ad335642..947846d42 100644 --- a/src/Netclaw.Actors.Tests/Tools/ShellApprovalDispositionMatrixTests.cs +++ b/src/Netclaw.Actors.Tests/Tools/ShellApprovalDispositionMatrixTests.cs @@ -26,7 +26,11 @@ public Task Power_shell_approval_contract(string caseId) private async Task AssertApprovalContract(string caseId) { - var testCase = ShellApprovalCases.Get(caseId); + await AssertApprovalContract(ShellApprovalCases.Get(caseId)); + } + + private async Task AssertApprovalContract(ShellApprovalCase testCase) + { await using var harness = await ShellApprovalHarness.CreateAsync( testCase, fixture.ActorSystem, @@ -43,6 +47,43 @@ private async Task AssertApprovalContract(string caseId) Assert.Equal(testCase.Expected.ApprovalMatches, observed.ApprovalMatches); } + [Fact] + public Task Interactive_reviewed_safe_candidate_uses_reviewed_policy() + => AssertApprovalContract(new ShellApprovalCase( + "interactive-reviewed-safe-allows", + new ShellApprovalInvocation("git status"), + Approvals.None, + ExpectedApproval.Allow(ToolAllowReason.SafeVerbInTrustedScope))); + + [Fact] + public Task Noninteractive_reviewed_safe_candidate_stays_uncovered() + => AssertApprovalContract(new ShellApprovalCase( + "noninteractive-reviewed-safe-requires-approval", + new ShellApprovalInvocation("git status", Interactive: false), + Approvals.None, + ExpectedApproval.Require(["git status"]))); + + [Fact] + public Task Noninteractive_candidate_can_use_an_explicit_persistent_grant() + => AssertApprovalContract(new ShellApprovalCase( + "noninteractive-reviewed-safe-with-grant-allows", + new ShellApprovalInvocation("git status", Interactive: false), + Approvals.PersistentAnywhere("git status"), + ExpectedApproval.Allow( + ToolAllowReason.StoredApproval, + 1, + "persistent:git status"))); + + [Fact] + public Task Noninteractive_safe_candidate_does_not_fill_a_partial_grant_gap() + => AssertApprovalContract(new ShellApprovalCase( + "noninteractive-partial-grant-keeps-safe-candidate-uncovered", + new ShellApprovalInvocation("git push && git status", Interactive: false), + Approvals.PersistentAnywhere("git push"), + ExpectedApproval.Require( + ["git status"], + approvalMatches: ["persistent:git push"]))); + [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_candidates_that_become_unsafe() diff --git a/src/Netclaw.Actors/Tools/ShellPolicyCoordinator.cs b/src/Netclaw.Actors/Tools/ShellPolicyCoordinator.cs index 345477fce..f32cf4c11 100644 --- a/src/Netclaw.Actors/Tools/ShellPolicyCoordinator.cs +++ b/src/Netclaw.Actors/Tools/ShellPolicyCoordinator.cs @@ -158,8 +158,11 @@ approvalService is not null ShellScopeRelation.None); } + var canUseReviewedSafePolicy = + projection.RunScope.InteractiveApproval is InteractiveApprovalCapability.Available; foreach (var candidate in grantCandidates.Where(candidate => - coverage.UncoveredIds.Contains(candidate.Id))) + canUseReviewedSafePolicy + && coverage.UncoveredIds.Contains(candidate.Id))) { if (policy.IsReviewedSafeCandidate( candidate.Candidate,