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
12 changes: 9 additions & 3 deletions IMPLEMENTATION_PLAN.md
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,12 @@ Done when:
- [ ] The policy pipeline replaces the shell branches in `ToolAccessPolicy`
and `ShellApprovalMatcher`; any retained legacy scan is deny-only and cannot
authorize, create candidates, or widen scope.
- [x] Shell calls pass through one coordinator. It snapshots immutable parser
and run-scope facts, requests one typed actor batch, and composes grant and
reviewed-safe coverage per candidate before one final result.
- [x] The actor response preserves stable candidate IDs and typed persistent
store status. Duplicate IDs, mismatched facts, impossible grant states, and
internal stage faults deny without a prompt.
- [ ] The bundled safe catalog removes every executable whose accepted
arguments can write, delete, execute code, or mutate a remote service through
executable argv interpretation. Redirect, parser-owned path/provider, and
Expand Down Expand Up @@ -245,12 +251,12 @@ Done when:
and path-shape facts introduced in 0.3.1. This store-v3 slice preserves those parser token facts
without executable-private command rules; later parent tasks consume the new
value-domain facts in the coordinator.
- [ ] Netclaw consumes public ShellSyntaxTree `0.3.3` for the parser-owned
- [x] Netclaw consumes public ShellSyntaxTree `0.3.3` for the parser-owned
authored filesystem domain. Local code accepts only `Exact` and `FiniteSet`.
It checks each value through path policy and keeps unsafe transforms strict.
The Release build and all 7,138 runnable tests pass. The suite reports 15
expected platform or opt-in skips. Adversarial review and CI remain before
completion.
expected platform or opt-in skips. Adversarial review and all required CI
checks passed before merge.
- [x] The expanded 247-test matrix covers command-substitution and PowerShell
execution-region behavior. Known command-owned regions reuse independently
matched host and body grants after Netclaw accounts for the parsed body.
Expand Down
14 changes: 7 additions & 7 deletions openspec/changes/structure-shell-approval-policy/tasks.md
Original file line number Diff line number Diff line change
Expand Up @@ -16,17 +16,17 @@

## 2. Typed coordinator and actor protocol

- [ ] 2.1 Snapshot immutable preflight facts from existing
- [x] 2.1 Snapshot immutable preflight facts from existing
`ToolExecutionContext`, `ToolRunScope`, `ToolApprovalAttempt`, and
`ShellExecutionEnvironment`; preserve `OneTimeApprovalKeys` exact-set
semantics and do not add a parallel context or scalar retry key.
- [ ] 2.2 Add one coordinator that runs synchronous preflight, sends one actor
- [x] 2.2 Add one coordinator that runs synchronous preflight, sends one actor
batch request, and completes policy without a second grant scan.
- [ ] 2.3 Add `ShellApprovalMatchRequest` and `ShellApprovalMatchResult` to
- [x] 2.3 Add `ShellApprovalMatchRequest` and `ShellApprovalMatchResult` to
`ToolApprovalActor`; match inherited session and persistent snapshots
atomically, return typed persistent-store status, and leave one-time state in
`ToolApprovalAttempt`.
- [ ] 2.4 Route `DispatchingToolExecutor` through the coordinator without
- [x] 2.4 Route `DispatchingToolExecutor` through the coordinator without
changing the original source, argument object, or tool history.
- [ ] 2.5 Preserve session-pipeline pending-request persistence,
stale/duplicate response rejection and recovery; preserve exact-set one-time
Expand All @@ -38,11 +38,11 @@
candidate construction, noninteractive trust-zone enforcement, actor match,
safe policy, exact-set one-time matching, and prompt completion in the
specified order.
- [ ] 3.2 Track coverage per candidate; allow only when all candidates are
- [x] 3.2 Track coverage per candidate; allow only when all candidates are
covered and call-level invariants pass.
- [ ] 3.3 Make internal exceptions, invalid enums, duplicate candidate IDs,
- [x] 3.3 Make internal exceptions, invalid enums, duplicate candidate IDs,
mismatched actor results, and impossible transitions terminal deny.
- [ ] 3.4 Allow fully one-time/session/safe-covered calls when persistent state
- [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.
- [ ] 3.5 Let expected unresolved shell input offer only one-time approval and
Expand Down
311 changes: 300 additions & 11 deletions src/Netclaw.Actors.Tests/Tools/DispatchingToolExecutorTests.cs

Large diffs are not rendered by default.

17 changes: 11 additions & 6 deletions src/Netclaw.Actors.Tests/Tools/ShellApprovalCaseCatalog.cs
Original file line number Diff line number Diff line change
Expand Up @@ -178,15 +178,15 @@ internal sealed record ExpectedApproval(
{
public static ExpectedApproval Allow(
ToolAllowReason reason,
int approvalChecks = 0,
int? approvalChecks = null,
params string[] approvalMatches)
=> new(
ToolAuthorizationOutcome.Allowed,
reason,
null,
[],
null,
approvalChecks,
approvalChecks ?? (reason == ToolAllowReason.SafeVerbInTrustedScope ? 1 : 0),
approvalMatches);

public static ExpectedApproval Require(
Expand Down Expand Up @@ -362,13 +362,13 @@ public static class ShellApprovalCases
"live-read-chain-with-separator-allows",
Bash("rg -rn \"operation failed\" src/ tests/ | head -20; echo \"---\"; rg -rln \"upload\" src/ | head -20"),
Approvals.None,
ExpectedApproval.Allow(ToolAllowReason.ApprovalExemptShellCandidates)),
ExpectedApproval.Allow(ToolAllowReason.SafeVerbInTrustedScope)),

Case(
"live-git-diagnostic-chain-with-separators-allows",
Bash("git status --short 2>&1 | head; echo \"---branch---\"; git branch --show-current 2>&1; echo \"---remotes---\"; git remote -v 2>&1 | head -4; echo \"---recent---\"; git log --oneline -3 2>&1"),
Approvals.None,
ExpectedApproval.Allow(ToolAllowReason.ApprovalExemptShellCandidates)),
ExpectedApproval.Allow(ToolAllowReason.SafeVerbInTrustedScope)),

Case(
"live-finite-url-loop-prompts-with-reusable-phrase",
Expand All @@ -382,7 +382,7 @@ public static class ShellApprovalCases
"gh run view 123456 --repo example/project --log-failed --verbose 2>&1 "
+ "| head -200; echo \"---EXIT $?---\""),
Approvals.None,
ExpectedApproval.Allow(ToolAllowReason.ApprovalExemptShellCandidates)),
ExpectedApproval.Allow(ToolAllowReason.SafeVerbInTrustedScope)),

Case(
"native-project-path-operand-allows-safe-verb",
Expand Down Expand Up @@ -1443,12 +1443,17 @@ public static class ShellApprovalCases
ExpectedApproval.Allow(
ToolAllowReason.StoredApproval,
1,
"session:git status",
"persistent:git push")),
Case(
"partial-compound-grant-prompts",
Bash("git status && git push"),
Approvals.PersistentAnywhere("git status"),
ExpectedApproval.Require(["git push"])),
ExpectedApproval.Require(
["git push"],
false,
1,
"persistent:git status")),
Case(
"four-unapproved-clauses-prompt",
Bash("git add . && git commit -m fix && git push && gh pr merge 123"),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,10 @@
| mixed-safe-unsafe-compound-prompts | Bash | Personal | Project | Interactive | git status && git push | none | RequiresApproval | approval required | git push | No |
| safe-pipe-unsafe-tail-prompts | Bash | Personal | Project | Interactive | git status \| git push | none | RequiresApproval | approval required | git push | No |
| safe-pipeline-allows | Bash | Personal | Project | Interactive | git log \| head -20 | none | Allowed | SafeVerbInTrustedScope | none | Not applicable |
| live-read-chain-with-separator-allows | Bash | Personal | Project | Interactive | rg -rn "operation failed" src/ tests/ \| head -20; echo "---"; rg -rln "upload" src/ \| head -20 | none | Allowed | ApprovalExemptShellCandidates | none | Not applicable |
| live-git-diagnostic-chain-with-separators-allows | Bash | Personal | Project | Interactive | git status --short 2>&1 \| head; echo "---branch---"; git branch --show-current 2>&1; echo "---remotes---"; git remote -v 2>&1 \| head -4; echo "---recent---"; git log --oneline -3 2>&1 | none | Allowed | ApprovalExemptShellCandidates | none | Not applicable |
| live-read-chain-with-separator-allows | Bash | Personal | Project | Interactive | rg -rn "operation failed" src/ tests/ \| head -20; echo "---"; rg -rln "upload" src/ \| head -20 | none | Allowed | SafeVerbInTrustedScope | none | Not applicable |
| live-git-diagnostic-chain-with-separators-allows | Bash | Personal | Project | Interactive | git status --short 2>&1 \| head; echo "---branch---"; git branch --show-current 2>&1; echo "---remotes---"; git remote -v 2>&1 \| head -4; echo "---recent---"; git log --oneline -3 2>&1 | none | Allowed | SafeVerbInTrustedScope | none | Not applicable |
| live-finite-url-loop-prompts-with-reusable-phrase | Bash | Personal | Project | Interactive | for url in /api/first /api/second; do echo "=== $url ==="; curl -sS -m 10 "$url" \| head -c 1500; echo; done | none | RequiresApproval | approval required | curl | No |
| safe-gh-run-diagnostic-exit-status-allows | Bash | Personal | Project | Interactive | gh run view 123456 --repo example/project --log-failed --verbose 2>&1 \| head -200; echo "---EXIT $?---" | none | Allowed | ApprovalExemptShellCandidates | none | Not applicable |
| safe-gh-run-diagnostic-exit-status-allows | Bash | Personal | Project | Interactive | gh run view 123456 --repo example/project --log-failed --verbose 2>&1 \| head -200; echo "---EXIT $?---" | none | Allowed | SafeVerbInTrustedScope | none | Not applicable |
| native-project-path-operand-allows-safe-verb | Bash | Personal | Project | Interactive | git diff install-skills.sh | none | Allowed | SafeVerbInTrustedScope | none | Not applicable |
| native-external-path-operand-prompts | Bash | Personal | Project | Interactive | git diff /etc/passwd | none | RequiresApproval | approval required | git diff | No |
| native-project-path-operand-reuses-grant | Bash | Personal | Project | Interactive | kubectl apply deployment.yaml | persistent[project]:kubectl apply | Allowed | StoredApproval | none | Not applicable |
Expand Down
13 changes: 12 additions & 1 deletion src/Netclaw.Actors.Tests/Tools/ShellApprovalHarness.cs
Original file line number Diff line number Diff line change
Expand Up @@ -328,7 +328,8 @@ public Task<IActorRef> GetAsync(CancellationToken cancellationToken = default)

internal sealed class CountingApprovalService(IToolApprovalService inner) :
IToolApprovalService,
IStructuredToolApprovalService
IStructuredToolApprovalService,
IShellApprovalMatchService
{
private int _checkCount;

Expand All @@ -346,6 +347,16 @@ public async Task<ToolApprovalCheckResult> CheckApprovalAsync(
return await inner.CheckApprovalAsync(sessionId, audience, toolName, candidates, cwd, ct);
}

public async Task<ShellApprovalMatchResult> MatchShellCandidatesAsync(
ShellApprovalMatchRequest request,
CancellationToken cancellationToken)
{
Interlocked.Increment(ref _checkCount);
return await ((IShellApprovalMatchService)inner).MatchShellCandidatesAsync(
request,
cancellationToken);
}

public Task<IReadOnlyList<string>> GetUnapprovedPatternsAsync(
ToolApprovalSessionId? sessionId,
TrustAudience audience,
Expand Down
57 changes: 57 additions & 0 deletions src/Netclaw.Actors.Tests/Tools/ToolApprovalActorTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -728,6 +728,63 @@ [new ToolApprovalGrant(candidate, Directory: null)],
}
}

[Fact]
public async Task Typed_shell_batch_preserves_ids_and_store_status()
{
var ct = TestContext.Current.CancellationToken;
var tempFile = Path.GetTempFileName();
try
{
File.WriteAllText(tempFile, "{\"version\":3,\"audiences\":{\"personal\":null}}");
var store = new ToolApprovalStore(
tempFile,
timeProvider: null,
migrationContext: new ApprovalStoreMigrationContext(NativeShell),
lockTimeout: TimeSpan.Zero);
var actor = Sys.ActorOf(ToolApprovalActor.CreateProps(store));
var service = CreateService(actor);
await service.RecordApprovalCandidatesAsync(
(ToolApprovalSessionId)"session-a",
TrustAudience.Personal,
new ToolName("shell_execute"),
[new ToolApprovalGrant(NativeCandidate("git status"), Directory: null)],
persistent: false,
ct);
var candidates = Array.AsReadOnly(
[
new ShellGrantCandidate(
new ShellPolicyCandidateId(7),
NativeCandidate("git status"),
RealDirectory: null),
new ShellGrantCandidate(
new ShellPolicyCandidateId(11),
NativeCandidate("dotnet test"),
RealDirectory: null)
]);

var result = await ((IShellApprovalMatchService)service).MatchShellCandidatesAsync(
new ShellApprovalMatchRequest(
(ToolApprovalSessionId)"session-a",
TrustAudience.Personal,
new ToolName("shell_execute"),
TestShellEnvironment.Current,
candidates),
ct);

var unavailable = Assert.IsType<PersistentGrantStoreStatus.Unavailable>(result.PersistentStore);
Assert.Equal(ApprovalStoreFailure.InvalidData, unavailable.Failure);
Assert.Equal([7, 11], result.CandidateMatches.Select(match => match.CandidateId.Value));
Assert.Equal(ShellCoverageKind.Session, result.CandidateMatches[0].GrantCoverage);
Assert.NotNull(result.CandidateMatches[0].Match);
Assert.Null(result.CandidateMatches[1].GrantCoverage);
Assert.Null(result.CandidateMatches[1].Match);
}
finally
{
File.Delete(tempFile);
}
}

private static AkkaToolApprovalService CreateService(IActorRef actor)
=> new(new StubRequiredActor(actor), TestShellEnvironment.Current);

Expand Down
26 changes: 25 additions & 1 deletion src/Netclaw.Actors/Tools/AkkaToolApprovalService.cs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,10 @@

namespace Netclaw.Actors.Tools;

public sealed class AkkaToolApprovalService : IToolApprovalService, IStructuredToolApprovalService
public sealed class AkkaToolApprovalService :
IToolApprovalService,
IStructuredToolApprovalService,
IShellApprovalMatchService
{
private readonly IRequiredActor<ToolApprovalActorKey> _actorProvider;
private readonly ShellExecutionEnvironment? _compatibilityEnvironment;
Expand Down Expand Up @@ -81,6 +84,27 @@ public async Task<ToolApprovalCheckResult> CheckApprovalAsync(
return response.Result;
}

async Task<ShellApprovalMatchResult> IShellApprovalMatchService.MatchShellCandidatesAsync(
ShellApprovalMatchRequest request,
CancellationToken cancellationToken)
{
var actor = await _actorProvider.GetAsync(cancellationToken);
var protocolSessionId = request.SessionId.HasValue
? (SessionId)request.SessionId.Value.Value
: (SessionId?)null;
var response = await actor.Ask<ShellApprovalMatchResponse>(
new MatchShellCandidates(
protocolSessionId,
request.Audience,
request.ToolName,
request.Environment,
request.Candidates),
TimeSpan.FromSeconds(5),
cancellationToken);

return response.Result;
}

public async Task RecordApprovalAsync(
string sessionId,
TrustAudience audience,
Expand Down
Loading
Loading