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
68 changes: 29 additions & 39 deletions openspec/changes/simplify-shell-policy-evaluator/design.md
Original file line number Diff line number Diff line change
Expand Up @@ -39,10 +39,10 @@ The exact D-case fixtures, 12 adversarial cases, 11 live cases, and the full pol
**Goals:**

- Replace the context analysis cache with one explicit preflight result.
- Give each ordered policy stage one clear owner.
- Keep one call-local state for candidates, coverage, evidence, trace facts, and terminal status.
- Keep the ordered policy phases in one direct evaluator.
- Keep one call-local state for candidates, coverage, evidence, and trace facts.
- Validate actor output once before any grant coverage applies.
- Compute parser-derived path facts once and reuse them across policy stages.
- Compute parser-derived path facts once and reuse them across policy phases.
- Reduce total production lines and control-flow lines below the measured baseline.
- Preserve all public, wire, persistence, prompt, trace, and operator contracts.
- Keep every unknown or invalid internal state fail-closed.
Expand Down Expand Up @@ -115,42 +115,32 @@ Add one internal `ShellPolicyEvaluation` class. It exists for one authorization

It owns:

- the preflight result and `ShellPolicyProjection`;
- the `ShellPolicyProjection`;
- candidate IDs and immutable candidate facts;
- one coverage slot per candidate;
- validated actor evidence;
- approval matches;
- persistent-store status;
- the trace builder;
- the terminal decision, when present.
- the trace builder.

Only methods on this type may change coverage. A stage cannot replace candidate facts or IDs.
Only methods on this type may change coverage. The evaluator cannot replace candidate facts or IDs.

Why:

- Mutation stays local and auditable.
- Stages no longer pass parallel lists and maps.
- Policy phases no longer pass parallel lists and maps.
- Coverage and trace updates can occur through one atomic method.
- The class avoids a new immutable array allocation after each stage.
- The class avoids a new immutable array allocation after each phase.

Alternative: return a new immutable state after every stage. Rejected because it adds allocation and code without stronger call-local safety.
Alternative: return a new immutable state after every phase. Rejected because it adds allocation and code without stronger call-local safety.

### 3. Ordered stages return one closed outcome
### 3. One direct evaluator owns the fixed order

Each stage changes state only through `ShellPolicyEvaluation`, then returns a closed outcome:
The coordinator executes each policy phase in one method. A terminal decision returns immediately. Coverage changes still pass through `ShellPolicyEvaluation`, while the decision remains a local return value.

```csharp
internal enum ShellPolicyStageOutcome
{
Invalid = 0,
Continue = 1,
Complete = 2,
}
```

Coverage, terminal decisions, and typed faults remain owned by the evaluation state. The coordinator verifies that `Continue` has no terminal decision and `Complete` has one. An invalid enum or mismatched outcome fails closed.
Unexpected exceptions enter one catch boundary. That boundary preserves accumulated trace rows, appends the terminal internal-failure row, and denies. Caller cancellation still propagates.

The early syntax and causal helpers are the sole callers that may complete an uncovered exact one-time allow.
The early syntax and causal phases are the sole paths that may complete an uncovered exact one-time allow.

That marker requires all of these facts:

Expand All @@ -161,7 +151,7 @@ That marker requires all of these facts:

No session, persistent, reviewed-safe, or other allow reason may bypass candidate coverage.

The pipeline invokes stages in this order:
The evaluator invokes phases in this order:

1. syntax and candidate validation;
2. protected real and fallback paths;
Expand All @@ -176,7 +166,7 @@ The pipeline invokes stages in this order:

Actor evidence stays before approval-exempt trace rows to preserve the frozen trace order. Pure side effects never enter the actor request.

Synchronous preflight keeps its current order before these stages:
Synchronous preflight keeps its current order before these phases:

1. parse validation;
2. hard deny;
Expand All @@ -185,16 +175,16 @@ Synchronous preflight keeps its current order before these stages:
5. candidate construction;
6. noninteractive trust zones.

A terminal outcome stops the pipeline. A later stage cannot revise an earlier deny or prompt.
A terminal outcome returns from the evaluator. A later phase cannot revise an earlier deny or prompt.

Why:

- Order becomes data, not incidental control flow.
- Each stage has a narrow test surface.
- Order is visible in one compact method.
- Behavior tests exercise the real coordinator instead of isolated stage machinery.
- Terminal precedence is visible.
- Internal faults map through one fail-closed path.

Alternative: retain one large method with regions. Rejected because regions do not enforce ownership or terminal precedence.
The earlier stage-result hierarchy was removed. It added terminal state, fault state, transition validation, factories, and 1,664 lines of implementation-shaped tests without changing observable policy.

### 4. Actor evidence has one validation boundary

Expand All @@ -211,7 +201,7 @@ The factory validates:
- near-miss count and enum values;
- the unavailable-store restrictions.

No grant enters coverage before this factory succeeds. The coordinator receives only validated evidence or a typed fault.
No grant enters coverage before this factory succeeds. The coordinator receives validated evidence or fails closed.

Why:

Expand Down Expand Up @@ -246,7 +236,7 @@ Known paths use an internal canonical absolute value with an explicit `ShellPath

The authorization preflight keeps its single raw `CommandReferencesDeniedPath(analysis)` defense scan. `ShellTool` also retains both execution-time checks.

Policy stages consume these facts through `ToolPathPolicy` and reviewed-safe policy. They do not rescan command text.
Policy phases consume these facts through `ToolPathPolicy` and reviewed-safe policy. They do not rescan command text.

Temporary-scope correction remains unchanged before projection. Projection may reuse only its existing captured temporary-alias predicate for canonical path resolution.

Expand Down Expand Up @@ -274,23 +264,23 @@ This method owns:
- causal full-context retention;
- exact one-time key input.

The one-time stage and prompt stage call the same method. They cannot derive different candidate sets.
The one-time and prompt phases call the same method. They cannot derive different candidate sets.

Alternative: keep two calls to `NarrowShellApprovalContext`. Rejected because future edits can create one-time and prompt drift.

### 7. Trace output observes state transitions

Coverage changes will add their trace row through the same state method. Terminal completion will append exactly one completion row.

The trace builder remains bounded and redacted. Stages cannot write raw commands, arguments, paths, session values, or secrets.
The trace builder remains bounded and redacted. Policy phases cannot write raw commands, arguments, paths, session values, or secrets.

Why:

- Coverage and trace cannot disagree.
- Tests can compare state transitions with trace rows.
- Redaction remains centralized.

Alternative: let each stage write trace rows directly. Rejected because coverage and trace can diverge.
Alternative: let each phase write trace rows directly. Rejected because coverage and trace can diverge.

### 8. Compatibility code remains an isolated adapter

Expand Down Expand Up @@ -349,7 +339,7 @@ The compatibility adapter is process-local. It creates no durable state.
| Failure | Required result |
| --- | --- |
| Parser or projection fault | `internal_policy_failure` deny |
| Invalid stage transition | `internal_policy_failure` deny |
| Invalid call-local invariant | `internal_policy_failure` deny |
| Invalid actor result | `internal_policy_failure` deny |
| Required persistent state unavailable | `approval_store_unavailable` deny |
| Expected unresolved shell input | one-time or deny prompt only |
Expand All @@ -361,20 +351,20 @@ No failure path creates session or persistent authority. No rollback needs data

## Risks / Trade-offs

- **Risk: stage extraction changes precedence** → Lock each terminal overlap with exact matrix cases before code moves.
- **Risk: direct evaluation changes precedence** → Lock each terminal overlap with exact matrix cases before code moves.
- **Risk: a new state class hides mutation** → Keep mutation methods narrow and expose immutable candidate views.
- **Risk: metric goals reward compressed code** → Require readable stages, corpus parity, and adversarial review.
- **Risk: metric goals reward compressed code** → Require readable phases, corpus parity, and adversarial review.
- **Risk: the compatibility adapter survives indefinitely** → Add a removal inventory and make new callers depend on typed evidence.
- **Risk: path facts lose source provenance** → Carry parser occurrence identity only inside call-local projection types.
- **Risk: a broad slice becomes hard to review** → Deliver dependency-ordered slices with full parity after each slice.

## Migration Plan

1. Merge the live corpus before production refactor code.
2. Add stage-state types and parity tests with no route changes.
2. Add call-local state and parity tests with no route changes.
3. Replace the context analysis cache with explicit preflight data.
4. Move actor-result validation behind the typed evidence boundary.
5. Route reviewed-safe, one-time, store, and completion logic through stages.
5. Route reviewed-safe, one-time, store, and completion logic through one ordered evaluator.
6. Consolidate path and prompt-context helpers.
7. Remove dead compatibility branches and duplicate helpers.
8. Run full local, Linux, macOS, and native Windows gates after each production slice.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,23 @@ The current footprint uses 9,947 lines and 650 control-flow lines. The gap is 97

The complete reduction gate remains open.

## Direct evaluation slice

This slice removes the stage-outcome hierarchy, terminal-state machine,
typed stage faults, and separate stage-owner classes. One direct coordinator
method keeps the same ten phases in the same order. Coverage and trace rows
still change atomically through the call-local evaluation state.

The slice removes 254 production lines and 16 control-flow lines. It replaces
the 1,664-line isolated-stage test file with 295 lines of path-fact and atomic
state tests. It also adds one end-to-end cancellation regression. The actor,
disposition, fixture, evidence, recovery, and executor suites exercise the real
coordinator path.

The complete changed footprint now uses 9,693 lines and 634 control-flow
lines. The frozen baseline uses 8,972 lines and 635 control-flow lines. The
control-flow gate now passes. The line gate remains open by 721 lines.

## Preliminary coverage and risk

The audit used `dotnet-coverage` 18.10.0 and `crap4dotnet` 0.1.1.
Expand Down
6 changes: 3 additions & 3 deletions openspec/changes/simplify-shell-policy-evaluator/proposal.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ The live corpus now supplies a stable contract for a behavior-compatible refacto
## What Changes

- Introduce one typed evaluation state for candidates, coverage, actor evidence, trace facts, and the persistent-store result.
- Give each ordered policy stage one input and one typed result.
- Make one direct evaluator own the documented policy order.
- Move actor-result validation behind one protocol boundary.
- Consolidate repeated prompt-context, path-fact, coverage, and terminal-decision logic.
- Shrink shell-specific branches inside `ToolAccessPolicy` and `ShellApprovalMatcher`.
Expand Down Expand Up @@ -35,7 +35,7 @@ Out of scope:

### New Capabilities

- `shell-policy-evaluator-architecture`: Defines the typed stage model, ownership boundaries, equivalence contract, and fail-closed internal protocol.
- `shell-policy-evaluator-architecture`: Defines the direct evaluator, ownership boundaries, equivalence contract, and fail-closed internal protocol.

### Modified Capabilities

Expand All @@ -45,6 +45,6 @@ None. The refactor preserves current `tool-approval-gates` behavior.

The change affects internal code in `Netclaw.Actors` and `Netclaw.Security`. Public tool APIs, persisted events, configuration, approval entries, prompts, and traces retain their current contracts.

Security impact is neutral by design. Unknown facts, invalid stage results, internal faults, protected paths, and unavailable required authority remain terminal deny or prompt under the current contract.
Security impact is neutral by design. Unknown facts, invalid call-local invariants, internal faults, protected paths, and unavailable required authority remain terminal deny or prompt under the current contract.

Operational impact is limited to ordinary binary rollout. No configuration edit, approval-store reset, database migration, or session migration is required.
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

### Requirement: Shell policy uses one explicit evaluation state

The system SHALL use one call-local shell policy state for projected candidates, coverage, grant evidence, trace facts, and terminal status.
The system SHALL use one call-local shell policy state for projected candidates, coverage, grant evidence, and trace facts.

The state SHALL preserve candidate identity and order for the complete authorization call. No state instance SHALL cross an actor, persistence, or session boundary.

Expand All @@ -14,17 +14,17 @@ The state SHALL preserve candidate identity and order for the complete authoriza

#### Scenario: Candidate identity cannot change

- **WHEN** any stage returns a candidate ID or fact that differs from projection
- **WHEN** any policy phase observes a candidate ID or fact that differs from projection
- **THEN** policy SHALL deny with `internal_policy_failure`
- **AND** no later stage SHALL apply authority
- **AND** no later phase SHALL apply authority

#### Scenario: Allowed analysis reaches execution

- **WHEN** shell policy allows a stream or non-stream execution
- **THEN** the executor SHALL receive the exact analysis that policy authorized
- **AND** no analysis SHALL pass through a context cache

#### Scenario: Preflight allows without completion stages
#### Scenario: Preflight allows without asynchronous completion

- **WHEN** preflight allows a parsed shell call through Auto mode or another terminal rule
- **THEN** its terminal result SHALL carry the exact authorized analysis
Expand All @@ -36,9 +36,9 @@ The state SHALL preserve candidate identity and order for the complete authoriza
- **THEN** policy SHALL return the current decision
- **AND** policy SHALL retain no analysis for a later call

### Requirement: Shell policy stages have one fixed order
### Requirement: Shell policy phases have one fixed order

The system SHALL execute synchronous preflight and asynchronous completion stages in the documented order. A terminal stage outcome SHALL stop all later policy stages.
The system SHALL execute synchronous preflight and asynchronous completion phases in the documented order. A terminal decision SHALL stop all later policy phases.

#### Scenario: Protected path precedes grant and safe policy

Expand All @@ -51,9 +51,9 @@ The system SHALL execute synchronous preflight and asynchronous completion stage
- **THEN** policy SHALL preserve the current allow result
- **AND** policy SHALL deny when an uncovered candidate still depends on persistent state

#### Scenario: Invalid stage result fails closed
#### Scenario: Invalid call-local invariant fails closed

- **WHEN** a stage returns an unknown result, invalid enum, or impossible transition
- **WHEN** evaluation observes an invalid enum, changed candidate, duplicate coverage, or impossible terminal decision
- **THEN** policy SHALL deny with `internal_policy_failure`
- **AND** policy SHALL not open an approval prompt

Expand Down Expand Up @@ -85,7 +85,7 @@ Netclaw policy SHALL consume those facts without an executable-private command p

- **WHEN** projection contains exact or finite filesystem facts
- **THEN** policy SHALL evaluate each fact through the current path rules
- **AND** later stages SHALL reuse the projected result without command-text scans
- **AND** later phases SHALL reuse the projected result without command-text scans

#### Scenario: Path facts retain their policy meaning

Expand All @@ -100,7 +100,7 @@ Netclaw policy SHALL consume those facts without an executable-private command p
- **THEN** reviewed-safe coverage SHALL remain unavailable
- **AND** the unknown domain SHALL NOT become a protected-path match
- **WHEN** an exact or finite causal value cannot resolve against an intent or fallback scope
- **THEN** the causal protected-path stage SHALL retain its current deny outcome
- **THEN** the causal protected-path phase SHALL retain its current deny outcome

#### Scenario: Redirect facts preserve their exact boundary

Expand Down Expand Up @@ -154,7 +154,7 @@ Each coverage change SHALL add its bounded trace fact through the same state ope

#### Scenario: Trace data remains redacted

- **WHEN** any stage emits trace evidence
- **WHEN** any policy phase emits trace evidence
- **THEN** trace data SHALL exclude raw commands, arguments, paths, prompts, session values, and secrets

### Requirement: Refactor preserves observable policy behavior
Expand Down Expand Up @@ -206,9 +206,9 @@ It SHALL not add a public API, durable schema, command parser, or duplicate poli

The system SHALL map unexpected internal faults to `internal_policy_failure`. Caller cancellation SHALL still propagate without conversion to a policy result.

#### Scenario: Stage throws an internal exception
#### Scenario: Evaluation throws an internal exception

- **WHEN** a policy stage throws outside caller cancellation
- **WHEN** a policy phase throws outside caller cancellation
- **THEN** the call SHALL deny with `internal_policy_failure`
- **AND** the failure SHALL create no approval authority

Expand Down
2 changes: 2 additions & 0 deletions openspec/changes/simplify-shell-policy-evaluator/tasks.md
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,8 @@
- [x] 7.7 Audit the complete changed production footprint and revise the reduction gate when file moves hide growth.
- [ ] 7.8 Remove the displaced production lines and control flow until the complete footprint is below baseline.
- [ ] 7.9 Run exact parity and adversarial review after each additional reduction slice.
- [x] 7.10 Remove the stage-result hierarchy and terminal-state machine after the real coordinator matrices cover the fixed order.
- [x] 7.11 Remove isolated stage tests that production cannot construct; retain path-fact, state, disposition, fixture, recovery, and cancellation coverage.

## 8. Prove equivalence and reduction

Expand Down
31 changes: 31 additions & 0 deletions src/Netclaw.Actors.Tests/Tools/DispatchingToolExecutorTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1729,6 +1729,37 @@ await Assert.ThrowsAnyAsync<OperationCanceledException>(() =>
Assert.Equal(1, approvalService.RequestCount);
}

[Fact]
public async Task Authorization_evaluation_propagates_cancellation_after_actor_result()
{
using var cancellation = new CancellationTokenSource();
var approvalService = new FixedShellApprovalService(request =>
{
cancellation.Cancel();
return new ShellApprovalMatchResult(
new PersistentGrantStoreStatus.Ready(),
Array.AsReadOnly(request.Candidates.Select(candidate =>
new ShellGrantCandidateMatch(
candidate.CandidateId,
Match: null,
GrantCoverage: null,
NearMisses: [])).ToArray()));
});
var executor = CreateApprovalGatedShellExecutor(approvalService);
var call = new FunctionCallContent(
"call-post-actor-cancellation",
ShellTool.ToolName,
ToolInput.Create("Command", "git status"));

await Assert.ThrowsAnyAsync<OperationCanceledException>(() =>
executor.EvaluateAuthorizationAsync(
call,
CreateInteractivePersonalContext("signalr/post-actor-cancellation"),
cancellation.Token));

Assert.Equal(1, approvalService.RequestCount);
}

[Fact]
public async Task Authorization_evaluation_denies_mismatched_actor_match()
{
Expand Down
Loading
Loading