docs(openspec): propose approval-policy-v2#940
Closed
Aaronontheweb wants to merge 69 commits into
Closed
Conversation
2 tasks
Aaronontheweb
force-pushed
the
openspec/approval-policy-v2
branch
2 times, most recently
from
81a306f to
574774a
Compare
Collaborator
Author
Eval Suite Results — Approval Policy v2Targeted N=5 runs against the local inference endpoint ( Baseline → Variant B (prompt rewrites only)
Infrastructure observationThe local inference endpoint is intermittently producing stalled streams — This affects eval reliability but is outside this PR's scope — it points at adding a streaming idle timeout on the daemon's OpenAI-compatible HTTP client so stalled streams surface as real errors instead of producing partial responses. Will file separately. Eval-infra fixes shipped in this PRBoth were latent bugs, surfaced when investigating why the v2 cases scored 0% initially:
What we know works
Acceptance gate statusPer |
Breaking redesign of the persistent approval store and prompt UX: - typed (verb, directory) ApprovalEntry replaces the v1 flat string list; v1 file quarantines to .v1.bak on first read (no migration) - safe-verb ∩ safe-space short-circuit (per-OS verb list, audience-aware roots from ToolAudienceProfileResolver) auto-runs read-only inspection inside session_dir / project_dir - ShellTool cwd defaults to project_dir → session_dir (today inherits daemon-process cwd) - ShellTokenizer refuses pattern extraction on bash control-flow / unbalanced input so junk fragments never persist - 5-button prompt (Once / This chat / Always here / Always anywhere / Deny) with danger styling on the destructive options; one-line resolution message - netclaw approvals trust-verb <verb> CLI for unattended/scheduled grants - AGENTS.md + tool description + failure-path hint coordinate to push the agent toward set_working_directory; eval cases (positive/negative/recovery/ schedule pre-approval) lock the behavior in
Foundation for the approval-policy-v2 storage refactor. Adds: - ApprovalEntry record (Verb required, Directory nullable for global wildcard) - ToolApprovalEntryComparer.Equals(ApprovalEntry, ApprovalEntry) overload that delegates to the existing platform-correct string comparison No behavior change: ToolApprovalStore still operates on the v1 string-based API and the existing test suite (274 tests) passes unchanged. The actual storage cutover, matcher refactor, and caller updates land in subsequent commits per openspec/changes/approval-policy-v2/tasks.md sections 1-6.
Section 1 of the approval-policy-v2 OpenSpec change. Refactors
ToolApprovalStore to a typed (verb, directory) ApprovalEntry model with
a versioned on-disk schema, replacing the v1 flat string list.
What changed:
- ToolApprovalStore now serializes/deserializes ToolApprovalData with
"version": 2 and List<ApprovalEntry> per (audience, tool).
- Two-step Load(): peek schema version via JsonDocument; quarantine
legacy v1 files to tool-approvals.json.v1.bak; quarantine unparseable
files to .invalid; in either case, return an empty store.
- AddApproval/RemoveApproval/RemoveAllForTool/Snapshot operate on
ApprovalEntry. New GetApprovedEntries replaces GetApprovedPatterns.
- AddApproval normalizes the directory portion (trims trailing
separators while preserving "/" and "C:\") so the on-disk file does
not accumulate trailing-slash variants of the same logical entry.
- ToolApprovalEntryComparer gains NormalizeDirectory + Normalize(entry)
helpers; Equals(ApprovalEntry, ApprovalEntry) normalizes both sides.
Caller updates required to compile:
- ToolApprovalActor: persistent writes wrap incoming verb strings as
ApprovalEntry { Verb=pattern, Directory=null } (interim semantic
preserved until section 2 lands the directory-aware matcher).
- ApprovalsListView/ApprovalsCommand: list output renders entries as
"<verb> in <dir>" or "<verb> anywhere"; --json emits the typed
ApprovalEntry shape; --json uses IndentedOmitNull so the CLI shape
matches the file shape (nulls omitted).
- ApprovalsCommand.WarnIfQuarantined surfaces both .v1.bak and
.invalid quarantine paths with distinct remediation guidance.
- ApprovalsManagerViewModel/Page: rendering uses entry.DisplayText.
- ToolAudienceProfilesDoctorCheck: drops the v1 stale-path-aware
pattern detection (irrelevant under v2; v1 contents quarantine on
first read).
Tests:
- ToolApprovalStoreTests rewritten for the v2 API and gain coverage
for v1 quarantine, malformed quarantine, fresh-write-after-quarantine,
trailing-slash normalization, and idempotent add.
- ApprovalsCommand/ApprovalsManagerPage tests rewritten to use
ApprovalEntry and the new "<verb> in <dir>" / "<verb> anywhere"
rendering.
- Stale-pattern doctor test removed.
All 3348 tests pass; dotnet slopwatch analyze reports no new
violations; file-header verification passes.
Section 2 of the approval-policy-v2 OpenSpec change. Refactors the approval matcher and gate to consume v2 typed ApprovalEntry records, plumbs the candidate cwd through the execution context, and deletes the v1 string-shape inspection logic. Matcher contract changes: - IToolApprovalMatcher.ExtractDirectoryRoots is removed; the v2 matcher has no concept of "directory roots extracted from arguments." The directory half of every (verb, directory) pair is the candidate's cwd from ToolExecutionContext. - ExtractApprovalEntries renamed to ExtractCandidateVerbs and now returns pure verb chains. The v1 fallback to normalized commands or bare directory roots is gone. - IsApproved signature: now takes (toolName, args, IReadOnlyList<ApprovalEntry>, cwd) and dispatches to ApprovalPatternMatching.MatchesShellApproval which enforces verb equality + (directory null || cwd under directory) + no-symlink-segment. Cwd plumbing: - ToolExecutionContext gains a Cwd property the session pipeline sets from candidate args / WorkingContext.ProjectDirectory / session_dir (sections 4 + 5 cover the resolution side). - IToolApprovalService.GetUnapprovedPatternsAsync and RecordApprovalAsync take a cwd parameter; AkkaToolApprovalService threads it through GetUnapprovedPatterns and RecordToolApproval actor messages. - ToolApprovalContext: ApprovalEntries field renamed to CandidateVerbs; DirectoryRoots stays but is always populated empty by the gate (section 7's prompt redesign removes the field). SessionOutput, SessionOutputDto, ParentSessionApprovalBridge, PendingToolInteraction, and the protocol mapper rename consistently. Shared symlink-segment guard: - PathUtility.ContainsSymlinkSegment hoisted from ScopedFileAccessPolicy so the matcher and the file-access policy share one implementation. Tests: - Configuration.Tests, Cli.Tests, Daemon.Tests, MemoryRetrievalPoC.Tests, Search.Tests, Security.Tests (397 incl new matcher cases), and Actors.Tests (1483) all pass. - ShellApprovalMatcherTests rewritten to assert the v2 (verb, cwd, entries) semantics: global-wildcard matches anywhere, folder-scoped matches when cwd under directory, requires concrete cwd, recurses into bash -c. - ToolApprovalGateTests' v1 directory-roots assertions replaced with v2 candidate-verb assertions; DirectoryRoots is asserted empty. - ToolApprovalActor's session HashSet now uses ToolApprovalEntryComparer.Comparer so session approvals follow the same platform-correct case rules as the persistent store. - Test plumbing across the codebase passes cwd: null where the invocation isn't directory-anchored. Slopwatch clean; file headers verified.
Section 3 of the approval-policy-v2 OpenSpec change. Adds a cheap
structural scan to ShellTokenizer that detects bash control-flow
keywords and unbalanced quotes/brackets, refuses verb-chain
extraction in those cases, and plumbs an IsMessy flag through the
gate and protocol so the section 7 prompt builder can show "complex
command" hints and omit persistent-grant buttons.
Detection (ShellTokenizer.IsMessyCompoundCommand):
- Single-pass scan that tracks quote state and (), [], {} balance.
- Flags any unquoted standalone token equal to one of:
for, while, do, done, then, fi, case, esac.
- Flags unbalanced quotes (open without close) and unbalanced brackets
(close without open OR open without close).
- Cheap structural only — no semantic bash parsing. Heredocs, command
substitution, and process substitution are not analyzed beyond
bracket balance.
SplitCompoundCommand:
- Returns an empty list when IsMessyCompoundCommand returns true. The
matcher's ExtractCandidateVerbs and ExtractPatterns therefore both
return empty for messy commands, and ShellApprovalMatcher.IsApproved
short-circuits to false (cannot auto-approve what we cannot extract).
Gate / protocol plumbing:
- IToolApprovalMatcher gains IsMessy(toolName, args). Default-false
for DefaultApprovalMatcher and FilePathApprovalMatcher; ShellApprovalMatcher
delegates to ShellTokenizer.IsMessyCompoundCommand.
- ToolApprovalContext gains an IsMessy bool field.
- ToolInteractionRequest, SessionOutputDto (InteractionIsMessy),
PendingToolInteraction, IParentApprovalBridge.RequestApprovalAsync,
and ParentSessionApprovalBridge all carry the flag through.
- DispatchingToolExecutor short-circuits messy invocations to
RequiresApproval regardless of empty CandidateVerbs, so the user
always sees the prompt for messy input.
Trade-off accepted: a bare standalone `done`/`fi`/`esac` token at the
end of a command (e.g. `git fetch && echo done`) is a false positive
for the cheap heuristic — the user gets the "complex command" prompt
(Once/Deny only) instead of the full 4-button row. The mitigation if
this bites real usage is a smarter detector that requires the keyword
to appear in a syntactically meaningful position; for now the trade
favors a clean approval store over coverage of edge bash idioms. One
existing test (SplitCompound_preserves_quoted_operators) updated
accordingly to use a different sentinel word.
Tests:
- ShellTokenizerTests: positive cases (for/while/if/case/unbalanced
quote/unbalanced bracket), negative cases (well-formed compounds,
command substitution, brace expansion, trailing commands), and
guards against keyword-substring false positives ("format",
"fido"). SplitCompoundCommand returns empty for messy input;
still splits well-formed compounds.
- ShellApprovalMatcherTests: IsMessy true for control-flow,
IsMessy false for well-formed; IsApproved returns false for
messy commands even when every conceivable verb is approved.
- All 3367 tests pass; slopwatch clean; file headers verified.
Section 4 of the approval-policy-v2 OpenSpec change. Establishes a deterministic cwd resolution chain for shell invocations so the approval policy can reason about safe-space membership and the spawned process never inherits the daemon's cwd. Resolution order (ToolExecutionContext.ResolveShellCwd): 1. Explicit args.WorkingDirectory when the agent provided one. 2. WorkingContext.ProjectDirectory when set via set_working_directory. 3. SessionDirectory (the per-session ~/.netclaw/sessions/<id>/ scratch). 4. null only when none is available. Plumbing: - ToolExecutionContext gains ProjectDirectory and ResolveShellCwd. The session pipeline populates ProjectDirectory at context-build time from _state.WorkingContext.ProjectDirectory. - SessionToolExecutionPipeline.ExecuteToolsAsync / ExecuteSingleToolAsync / BuildToolExecutionContext gain a projectDirectory parameter; LlmSessionActor passes _state.WorkingContext.ProjectDirectory at every dispatch. - ShellTool.ExecuteAsync uses context.ResolveShellCwd(args.WorkingDirectory) to set psi.WorkingDirectory; never falls through to ProcessStartInfo's default-of-inheriting-the-daemon's-cwd, which is a footgun the approval policy cannot reason about. - DispatchingToolExecutor.AuthorizeCoreAsync calls the same resolver and writes context.Cwd before GetUnapprovedPatternsAsync, so the approval gate evaluates folder-scoped ApprovalEntry records against the same cwd the spawned process will run in. Tests: - Cwd_falls_back_to_project_directory_when_no_explicit_arg - Cwd_falls_back_to_session_directory_when_project_directory_null - Cwd_explicit_arg_overrides_project_and_session_directories - Cwd_does_not_inherit_daemon_process_directory (asserts the spawned pwd output is the resolved session_dir, not Environment.CurrentDirectory) All 3371 tests pass; slopwatch clean; file headers verified.
Section 5 of the approval-policy-v2 OpenSpec change. Adds the
load-bearing friction-reduction layer: read-only verbs invoked inside
declared safe spaces auto-allow without prompting, while every other
combination still routes through the interactive approval gate.
Three-position policy:
layer 1 ToolPathPolicy hard-deny (unchanged)
layer 1.5 NEW: safe-verb ∩ safe-space short-circuit (this commit)
layer 2 interactive approval gate (unchanged)
A candidate (verb, cwd) short-circuits to Allow when ALL hold:
- verb is on the curated SafeVerbList for the current OS
- cwd resolves under one of the audience-aware safe-space roots
(Personal/Team: session_dir + project_dir; Public: session_dir)
- no segment of the cwd path is a filesystem symlink (reparse point)
Bundled lists (Netclaw.Configuration/SafeVerbs/safe-verbs.*.json
embedded as resources, additive user override at
~/.netclaw/config/safe-verbs.<os>.json):
Linux/macOS: ls, find, grep, egrep, fgrep, rg, cat, head, tail, wc,
sort, uniq, cut, tr, awk, sed -n, file, pwd, which, stat, tree,
du, df, git status, git log, git diff, git show, git branch,
git remote, git rev-parse, git ls-files, git blame.
Windows: dir, type, more, where, findstr, Get-ChildItem,
Get-Content, Select-String, Get-Item, Test-Path, Get-Location,
Resolve-Path, plus the same git read subcommands.
Mutating verbs (git push, sed -i, awk -i inplace, rm, mv, etc.) are
intentionally absent from both lists. sed is pinned to "sed -n" so
the matcher refuses to short-circuit "sed -i". The verb-chain
matcher means "awk" auto-allows but "awk -i inplace" hits the gate
because ExtractVerbChain stops at the first flag.
Plumbing:
- New SafeVerbList (Configuration) with platform-correct comparer.
- New SafeVerbLoader that reads the bundled JSON resource and merges
the user override file additively. Malformed override → silently
fall back to bundled defaults (the doctor will surface the problem
out of band; we do not refuse to start the daemon).
- New ScopedShellSafeVerbPolicy (Netclaw.Actors.Tools) mirroring
ScopedFileAccessPolicy: takes (verb, cwd, context), returns a
short-circuit decision; reuses PathUtility.ContainsSymlinkSegment
and the audience model.
- ToolAccessPolicy gains a SafeVerbList ctor parameter and runs the
safe-verb check inline in CheckApprovalGate after the messy/Auto
filters but before producing the approval-prompt context. The cwd
it evaluates is resolved by ToolExecutionContext.ResolveShellCwd
and written back to context.Cwd so the downstream gate and the
spawned process agree on "where this runs."
- DispatchingToolExecutor's duplicate cwd resolution removed —
CheckApprovalGate now owns the write to context.Cwd.
- Program.cs constructs a SafeVerbList at startup and registers it
alongside ToolAccessPolicy.
- NetclawPaths.SafeVerbsOverridePath returns the per-OS user file.
Tests (3388 → 3398 across the suite):
- SafeVerbLoaderTests: bundled defaults present per OS, user override
extends additively, malformed override falls back, missing override
ignored, platform-correct case rules.
- ScopedShellSafeVerbPolicyTests: all seven scenarios from the spec —
safe verb + project_dir → allow; safe verb + session_dir → allow;
safe verb + outside → prompt; mutating verb in safe space →
prompt; Public audience cannot use project_dir as safe space;
symlink segment in cwd breaks short-circuit; AllShortCircuit
fails-loud when any candidate is unsafe.
Slopwatch clean; file headers verified.
Section 6 of the approval-policy-v2 OpenSpec change. Replaces the
section 1 interim revoke parser with a strict parser for the user-
visible scope labels emitted by 'list', and adds the 'trust-verb'
subcommand for pre-approving global wildcards from the CLI.
Revoke parser:
- Accepts only the two forms 'list' emits:
'<verb> in <directory>' -> (verb, directory) entry
'<verb> anywhere' -> (verb, null) global wildcard
- Anything else exits 1 with a clear message — bare verb input
no longer silently treated as a global wildcard, so an operator
typo cannot widen the intended scope. The TryParseRevokePattern
helper is internal so tests can exercise the parser surface
directly without the CLI shell.
trust-verb subcommand:
- 'netclaw approvals trust-verb <verb> [--audience <a>] [--tool <t>]'
- Default audience = personal, default tool = shell_execute.
- Writes a (verb, null) entry to tool-approvals.json — the
global-wildcard form. Idempotent: existing entry exits zero with
a "No changes" message; otherwise prints "Trusted '<verb> anywhere'
for <audience> / <tool>".
- This is the deliberate scriptable path the spec calls out for
unattended/scheduled task pre-approval. Combined with section 5's
safe-verb short-circuit it covers two distinct user goals:
short-circuit (read-only verbs in safe spaces, no persistence)
versus trust-verb (any verb, anywhere, persisted).
Help text updated to document both new forms; quarantine note from
section 1 already covers the .v1.bak case.
Tests (Cli.Tests 620 -> 629):
- Revoke folder-scoped form removes entry with matching directory;
folder-scoped form does not match a global-wildcard entry;
unrecognized pattern exits 1 with clear message.
- trust-verb adds global wildcard with default audience/tool;
idempotent on repeated invocation; honors --audience/--tool;
missing verb argument exits 1 with usage; unknown audience flag
exits 1.
- Help output mentions trust-verb subcommand.
TUI display already shows verb + directory via DisplayText (landed
in section 1). The trust-verb-from-TUI affordance is deferred — the
agent path is CLI-only and the CLI works for human operators too;
revisit if friction surfaces.
All 3397 tests pass; slopwatch clean; file headers verified.
Section 7 of the approval-policy-v2 OpenSpec change. Replaces the
v1 Slack approval prompt (4 buttons + Patterns/Directory Roots
sections) with the v2 design: 5 buttons, danger styling on the
elevated decisions, cwd in the header, verbs as bullets, and a
single-line resolution message.
Five-button row (ApprovalOptionKeys):
Once (primary) - no persist
This chat (default) - session-scoped only
Always here (default) - persist (verb, cwd)
Always anywhere (danger) - persist (verb, null)
Deny (danger) - refuse this call
ApprovalOptionKeys gains ApproveEverywhere/ApproveEverywhereLabel
("Always anywhere") and renames the existing labels to the spec
spelling: "Once" / "This chat" / "Always here" / "Deny". The wire
keys are unchanged so persisted resolutions still decode.
ApprovalDecision and ParentApprovalDecision gain ApprovedEverywhere
so the runtime can distinguish folder-scoped persistence from global
wildcard. LlmSessionActor maps the new button key, picks
cwd-or-null based on which decision was chosen, and threads through
RecordApprovalAsync. ToolApprovalActor's persistent-write path now
uses msg.Cwd directly (replacing the section 1 interim that always
wrote null), so:
Always here -> AddApproval(audience, tool, (verb, msg.Cwd))
Always anywhere -> AddApproval(audience, tool, (verb, null))
Button-row gating by IsMessy / cwd-shallow:
IsMessy -> only Once + Deny (no persistence possible)
cwd shallow -> Always here omitted (This chat / Always
anywhere still available; matches the
tool-approval-gates "Shallow directory prevents
Always here" scenario)
otherwise -> all five buttons
Cwd-shallow check in ToolAccessPolicy: a path with fewer than two
non-empty path segments under its root (e.g. /, /etc/, C:\) cannot
host a folder-scoped grant; fail-closed on Always here so an
operator cannot accidentally persist a too-shallow root.
Slack prompt body changes:
Header (single verb): "Approve git status in /home/user/repos/foo?"
Header (multi-verb): "Approve in /home/user/repos/foo?"
+ "• git fetch / • git rebase / • git status"
Messy: "_complex command — only one-shot approval
available_"
The Patterns and Directory Roots sections are gone; verb display
flows from CandidateVerbs (the v2 matcher's pure verb-chain
extraction) with a Patterns fallback for legacy callers.
Resolution message single-line format:
Always here -> "Saved: <verbs> in <cwd>"
Always anywhere -> "Saved: <verbs> anywhere"
This chat -> "Saved for this chat: <verbs> in <cwd>"
Once -> "Approved (no save)"
Deny -> "Denied"
Tests (Actors.Tests 1497 -> 1507):
- New SlackApprovalBlockBuilderTests covers all the spec scenarios:
single-verb header, multi-verb bulleted header, messy hint,
five-button row with danger styling on Always anywhere + Deny,
legacy Directory Roots / Patterns sections gone, and all five
resolution-message branches (Always here / Always anywhere /
This chat / Once / Deny).
- Existing DiscordApprovalPromptBuilderTests label expectations
bumped to the new spelling ("Once" / "Always here").
All 3407 tests pass; slopwatch clean; file headers verified.
Discord rendering still on v1 — section 8 mirrors this design over.
Section 8 of the approval-policy-v2 OpenSpec change. Brings the
Discord approval prompt to parity with the Slack v2 layout from
section 7: same 5-button row, same danger styling rules, same
header format, same single-line resolution message.
DiscordApprovalPromptBuilder changes:
- BuildButtonPrompt now renders the v2 header
("Approve git status in /home/user/repos/foo?" for single-verb,
"Approve in /home/user/repos/foo?" + bulleted verbs for multi-verb)
and surfaces the "complex command — only one-shot approval
available" hint when IsMessy is true.
- BuildResolvedPromptText emits the single-line resolution form
identical to Slack:
Always here -> "Saved: <verbs> in <cwd>"
Always anywhere -> "Saved: <verbs> anywhere"
This chat -> "Saved for this chat: <verbs> in <cwd>"
Once -> "Approved (no save)"
Deny -> "Denied"
- GetButtonStyle applies DiscordButtonStyle.Danger to both
ApproveEverywhere and Deny, mirroring Slack's danger pair.
- Verb display sources from CandidateVerbs (v2) with a Patterns
fallback for legacy callers.
- GetDecisionLabel handles ApproveEverywhere alongside the existing
keys.
No Discord-side response-handler changes required: the transport
decodes button values and forwards selectedKey to the session actor,
and LlmSessionActor's switch (updated in section 7) already routes
ApproveEverywhere for both channels.
Tests (Actors.Tests 1507 -> 1514):
- Existing two BuildResolvedPromptText cases bumped to assert the
v2 single-line form ("Approved (no save)" / "Denied") instead of
the v1 "Decision: <label>" string.
- Seven new V2_ tests parallel to SlackApprovalBlockBuilderTests:
single-verb header collapse, multi-verb generic header with
bullets, messy-command hint with two-button row, five-button row
with danger styling on Always anywhere and Deny, and the three
persistent-resolution branches (Always here / Always anywhere /
This chat).
All 3414 tests pass; slopwatch clean; file headers verified.
Both Slack and Discord approval flows now end-to-end on v2.
…hint
Section 9 of the approval-policy-v2 OpenSpec change. Steers the
agent toward declaring its project root early and gives it a
self-correction path when a shell call is denied for cwd-outside-
safe-spaces.
netclaw-operations SKILL.md (bumped to v2.0.0):
- Rewrote Approval Prompts around the v2 (verb, directory) model:
three-layer gate (hard-deny / safe-verb short-circuit / interactive
prompt), the five-button row and its scope semantics, when fewer
buttons appear (messy / shallow cwd), and how set_working_directory
affects prompt cadence.
- Added "Pre-approving for unattended tasks (load-bearing)" section
documenting the schedule-creation pre-approval flow. Replaces the
v1 "run interactively first" pattern with the new
'netclaw approvals trust-verb <verb>' path; agent dialogue example
shows how to ask the user before pre-approving.
- Updated the Approval Requirements for Reminders/Webhooks section
to point at trust-verb instead of interactive-first.
- Updated the inspecting/revoking section: list emits typed entries
('<verb> in <dir>' / '<verb> anywhere'); revoke accepts those forms
verbatim; trust-verb is the deliberate scriptable path.
- Last-resort recovery now mentions both .v1.bak and .invalid
quarantine paths.
Resources/AGENTS.md (Personal+Team identity file):
- New top-level "Declare Your Project Root Early (load-bearing)"
section. Tells the agent its FIRST shell-related action MUST be
set_working_directory when the task is project-scoped, with the
consequence framing ("burns the user's attention and your token
budget" if skipped). Includes a recovery rubric: when shell denial
surfaces a set_working_directory hint, read it and self-correct
rather than re-prompting the user.
- AGENTS.public.md unchanged because set_working_directory is
profile-managed away from Public.
set_working_directory tool description:
- Reframed from "set the project directory for this session" to
"Declare your project root and expand your trusted scope." Spells
out the safe-verb short-circuit consequence so the model sees
*why* this tool matters for friction reduction. Removed the cd-
style framing.
- Added public ToolName constant so the failure-path hint logic can
reference it without string duplication.
Failure-path hint (SessionToolExecutionPipeline.BuildSetWorkingDirectoryHint):
- Emits a one-line hint pointing at 'set_working_directory <cwd>' when:
* tool is shell_execute
* decision is Denied (not TimedOut, not hard-deny)
* cwd is non-null
* cwd is NOT inside SessionDirectory or ProjectDirectory
* set_working_directory is exposed to the current audience
- LlmSessionActor pre-computes setWorkingDirectoryAvailable from the
ToolAccessPolicy's IsToolExposed check and threads the bool into
ExecuteToolsAsync; the pipeline appends the hint to the deny-result
text the model sees on its next turn.
- Suppresses for non-shell tools, timeouts, hard-deny refusals, cwd
already inside a safe space, and audiences without the tool — so
Public sessions don't see misleading "use set_working_directory"
guidance.
Tests (Actors.Tests 1514 -> 1521):
- Seven hint-helper unit tests cover all the spec scenarios:
emitted on cwd-outside denial; suppressed when tool unavailable;
suppressed for TimedOut; suppressed for non-shell tools; suppressed
when cwd is inside session_dir; suppressed when cwd is inside
project_dir; suppressed when cwd is null.
All 3421 tests pass; slopwatch clean; file headers verified.
Cleanup pass on the approval-policy-v2 PR. Two related dead-code
removals that were marked "to delete in section 7" but never trimmed.
Dead v1 directory-extraction helpers:
- IShellApprovalSemantics.ExtractDirectoryRoots (interface + impl).
- ShellApprovalSemanticsBase.TryCreateDirectoryApprovalRoot,
ExtractDisplayDirectory, NormalizeDisplayDirectory,
IsRelativeDisplayPath, EnsureTrailingSeparator, CountPathSegments,
GetLastShellSeparatorIndex.
- PosixShellApprovalSemantics.ExtractDisplayDirectory and
EnsureTrailingSeparator overrides.
- ShellTokenizer.ExtractDirectoryRoots and MinDirectoryScopeDepth.
- DirectoryApprovalRoot record (file deleted).
- ShellTokenizerTests.ExtractDirectoryRoots_* test methods plus the
AbsoluteRootCases / RelativeRootCases / WindowsAbsoluteDirectoryRootCases
TheoryData properties that fed them.
These were the v1 "extract directory roots from path arguments" path.
v2 derives directory exclusively from ToolExecutionContext.Cwd so
nothing in production calls these anymore.
DirectoryRoots field plumbing:
- ToolApprovalContext, ToolInteractionRequest (SessionOutput),
SessionOutputDto.InteractionDirectoryRoots, the mapper round-trip,
PendingToolInteraction, IParentApprovalBridge.RequestApprovalAsync,
ParentSessionApprovalBridge, SubAgentActor caller, the pipeline emit
site, and the TUI rendering in ChatViewModel/ChatPage.
- All carriers always passed [], per the spec's "REMOVED Requirement:
Directory root extraction via IToolApprovalMatcher" and the section 7
prompt redesign which moved cwd into the prompt header.
Tests updated:
- DaemonClientMappingTests no longer round-trips DirectoryRoots.
- ParentSessionApprovalBridgeTests passes a real verb chain instead of
the synthetic "/tmp/work/logs/" placeholder it was carrying.
- ToolApprovalGateTests drops Assert.Empty(DirectoryRoots) calls that
only existed to document the empty-after-cutover state.
- ChatPage approval prompt rendering updated to the v2 button labels
("Once / This chat / Always here / Always anywhere / Deny").
3411 tests pass (10 fewer than before because the
ExtractDirectoryRoots_* test methods were removed; nothing else
changed). Slopwatch clean; file headers verified.
Followups from the simplification review pass. ApprovalEntry now owns its display + parse round-trip: - Format: ApprovalEntry.FormatScope() emits "<verb> in <dir>" or "<verb> anywhere". - Parse: ApprovalEntry.TryParseScope(input, out entry, out error) is the inverse, accepting only the two user-visible forms. - Both helpers replace duplicated implementations in ApprovalsCommand.FormatEntryForList, ApprovalsCommand.TryParseRevokePattern, and ApprovalDisplayItem.DisplayText. One round-trip source of truth. Hot-path: the actor's per-message file read. - ToolApprovalActor.GetUnapprovedPatterns now snapshots the persisted approvals once per message rather than re-reading + re-parsing tool-approvals.json per candidate verb. For a compound shell with N verbs that's N file reads → 1. Hot-path: per-verb cwd / safe-roots / symlink work. - ScopedShellSafeVerbPolicy.AllShortCircuit hoists Path.GetFullPath, ResolveSafeSpaceRoots, and ContainsSymlinkSegment out of the per-verb loop. The cwd doesn't change between verbs in the same invocation, so a 4-verb compound now does 1 path-normalize + 1 symlink scan instead of 4. ShortCircuitsApproval becomes a thin wrapper that forwards to AllShortCircuit. Wire ApprovalOptionKeys.IsDangerStyled in both channel builders instead of inlining the same `Deny or ApproveEverywhere` switch arm in two files. Consolidate WorkingDirectory/Command extraction in ShellApprovalMatcher to call ToolArgumentHelper.GetString — the helper already handles the PascalCase ↔ camelCase round-trip via key normalization, so the inline two-key TryGetValue duplication was needless and slightly inconsistent with the rest of the codebase. 3411 tests pass; slopwatch clean; file headers verified.
…approval
Section 10 of the approval-policy-v2 OpenSpec change. Adds a new
"Approval Policy v2" eval category covering the four behavioral
guardrails introduced in sections 5 + 9:
- approval_set_working_directory_positive
Project-scoped prompt mentions a repo path. Asserts the agent
calls set_working_directory before any shell tool call into that
tree (order check: SWD line < first shell_execute line).
- approval_set_working_directory_negative
Unrelated prompts ("what's 2+2?", "explain a hash table"). Asserts
the agent does NOT preemptively call set_working_directory just
because AGENTS.md mentions it.
- approval_recovery_hint (multi-turn)
T1 plants the cwd-outside-safe-spaces denial hint into the
conversation; T2 asserts the agent self-corrects by calling
set_working_directory rather than re-prompting the user. Scripting
an actual denial inside the eval container would require a
preconfigured project_dir mismatch we don't have plumbing for; the
hint-shape feed exercises the same self-correction code path.
- approval_schedule_pre_approval
User asks to schedule a daily reminder using the freshdesk verb.
Asserts the agent calls `netclaw approvals trust-verb freshdesk`
via shell_execute as part of schedule setup.
Task 10.1 cross-checked: "Pre-approving for unattended tasks
(load-bearing)" section in netclaw-operations SKILL.md (added in
section 9) covers the agent-driven trust-verb flow with example
dialogue. No additional skill text needed.
Task 10.6 (run the suite, document baseline pass rate) is deferred
to local execution — the suite needs NETCLAW_EVAL_PROVIDER_* env +
Docker daemon container which only Aaron has set up. Listed in
acceptance gates.
`bash -n evals/run-evals.sh` parses cleanly.
Folds the change's delta specs into main specs and archives the change to openspec/changes/archive/2026-05-08-approval-policy-v2/. - tool-approval-gates: rewrites shell pattern matching, persistent approval storage, and directory-root approvals around the v2 ApprovalEntry model; adds requirements for safe-verb short-circuit, five-button prompt, single-line resolution, and bash control-flow refusal. - session-cwd: adds shell tool cwd defaults, failure-path hint, and the safe-space expansion contract that set_working_directory now carries; modifies set_working_directory tool to reflect the new framing. Also fixes a pre-existing structural defect (spec was authored with the delta '## ADDED Requirements' heading instead of '## Purpose' + '## Requirements'). - netclaw-cli: replaces the Operator CLI for persistent tool approvals requirement with the v2 version (scope-labeled list, strict revoke parser, trust-verb subcommand, .v1.bak quarantine note).
Two bugs together meant evals never tested in-repo skill changes:
1. The skill scanner expects '<skills>/.system/<skill-name>/SKILL.md'
but the eval script copied to '.system/files/<skill-name>/SKILL.md'
(matching the repo's feeds/ layout, not the runtime layout). The
local copies were silently invisible.
2. The daemon then synced from the live R2 feed, which ships the last
released set of skills. So evals always exercised whatever was
published, not the source tree.
Result: a v2 'netclaw-operations' SKILL.md bumped in this PR was a
no-op for evals — the model in the container saw the older 1.x copy
from R2 and missed the new approval/trust-verb guidance entirely.
Fix:
- Copy '.../files/<skill>/' → '$EVAL_HOME/skills/.system/<skill>/'.
- Set 'NETCLAW_SkillSync__DisableSystemSkillSync=true' in the eval
container so the daemon doesn't fetch+overwrite from the live feed.
Confirmed via re-run: skill_load("netclaw-operations") now succeeds
inside the eval container (previously: "Skill not found"). The new
v2 approval cases ('approval_set_working_directory_positive',
'approval_schedule_pre_approval') visibly improve once the model can
see the bumped skill content.
Two cases had genuine eval-design problems independent of the v2
implementation, surfaced once N=5 baselines stabilized.
approval_set_working_directory_positive
Old prompt: 'I'm working on the Netclaw repository at /tmp. List the
files in that directory and tell me what's there.' This is ambiguous
between sustained project work (which the v2 spec says SHOULD pre-
declare) and a one-shot directory listing (which the spec explicitly
says should NOT pre-declare). The model going straight to shell was
arguably a correct read of the prompt, not a guidance failure.
New prompt makes the sustained-work signal explicit ('debugging
session... multiple shell commands across the tree').
approval_recovery_hint
Old structure was multi-turn: T1 fed a denial message and instructed
'do not call any tools yet', T2 said 'now call the tool.' Several
failure runs showed the model getting stuck in T1's no-tools
conditioning and refusing T2 ('I will not call any tools.'). That
tests prompt-flip resilience, not recovery-hint comprehension.
Rewrote as a single conversational prompt that delivers the denial
hint and asks 'how should I unblock this?' which is what a real
recovery turn looks like.
Side note for the PR: full N=5 baselines on local provider show the
inference endpoint is intermittently flaky (Dutchman-style stream
stalls — 'out=3 tok_s=8' instead of normal 'out=80 tok_s=27'),
which produces eval variance unrelated to either the v2
implementation or these prompts. Aaron will validate via binary-
swap before merging.
Aaronontheweb
force-pushed
the
openspec/approval-policy-v2
branch
from
c38eb08 to
1c96848
Compare
…ually scopes
ToolApprovalContext was missing a Cwd field, so SessionToolExecutionPipeline
emitted ToolInteractionRequest with Cwd=null even though ToolExecutionContext
already had it resolved. PendingToolInteraction.Cwd was therefore always null
on the session-actor side, and the persistence path
var persistCwd = decision == ApprovedEverywhere ? null : pending.Cwd;
silently turned every "Always here" click (ApprovedAlways) into a global
wildcard ("Always anywhere"). Confirmed in a live session: tool-approvals.json
contained nine entries, all with directory=null, despite most coming from
"Always here" button clicks.
The fix is small but the bug was load-bearing — folder-scoped trust is the
whole point of the v2 button row. Without cwd flowing through, the "here"
button was UX theater.
- Add 'Cwd' to ToolApprovalContext (string?, defaults null).
- Resolve cwd up-front in ToolAccessPolicy.CheckApprovalGate so it's
populated for every shell approval path (not only the safe-verb
short-circuit branch).
- Pass ctx.Cwd into ToolInteractionRequest.
- Regression test (SessionToolExecutionPipelineTests.Approval_request_
propagates_cwd_from_approval_context) asserts the emitted request
carries the cwd through.
Three bug-fix pillars on top of v2 (which has not deployed beyond a single dogfood operator): verb extraction emits the command head only, the first path-like argument becomes the candidate's effective directory, and pure side-effect clauses (echo / printf / true / false) authorize once but do not pollute persistence. The dogfood evidence (D0AC6CKBK5K/1778303523.861279) showed nine 'tool-approvals.json' entries that never matched future calls because each call's path got baked into the persisted verb. The fix reframes the (verb, directory) pair so verbs are reusable and paths declare scope implicitly. Persistence shape is unchanged. Includes proposal.md, design.md, and a delta spec for the 'tool-approval-gates' capability with all five MODIFIED/ADDED requirements covering verb classification, effective-directory matching, file-parent inference, multi-path tiebreak, and the side-effect skip list. Tasks are next.
Aaronontheweb
added
shell
…nontheweb/netclaw into openspec/approval-policy-v2
Seven sections covering path classification, matcher updates, persistence, side-effect skip list, agent guidance, tests, and the sync/archive flow. Acceptance gates include a manual binary-swap check that explicitly validates the dogfood failure mode (find /repo → Always here → find /repo/sub auto-runs).
Wires the trust-zones-rewrite components (HardDenyOverridesLoader,
AudienceTrustStore, GateEvaluator, TrustStateComposer, IShellParser)
into the daemon's DI container at Program.cs:648-722. v2 ToolApprovalStore
remains the authoritative approval-decision path for shell tools until
ToolAccessPolicy gets the gate-evaluator integration in a follow-up
commit; the new services exist as singletons ready to consume.
Files:
- NetclawPaths: adds TrustZonesPath (~/.netclaw/config/trust-zones.json,
sibling to the v2 tool-approvals.json) and HardDenyOverridesPath
(~/.netclaw/config/hard-deny-overrides.json). The new store is at a
sibling path during transition so the v2 store can keep handling
existing entries without conflict; the two file shapes are
incompatible (AudienceTrustStore.Load would archive a v2-shape file
on first read).
- Program.cs:
- HardDenyOverridesLoader.Load() invoked at startup; loaded rules
flow into ShellCommandPolicy's new (additionalDenyPatterns,
overrideRules) constructor. Malformed override file throws
InvalidDataException with operator context and refuses startup
rather than silently dropping rules.
- AudienceTrustStore registered with TrustZonesPath as singleton.
- IShellParser registered via services.AddShellParser() (BashParser
impl from the ShellSyntaxTree package).
- GateEvaluator registered as singleton wrapping ShellCommandPolicy
+ IShellParser.
- TrustStateComposer registered as singleton wrapping
toolConfig.AudienceProfiles + AudienceTrustStore + SafeVerbList.
Per-call session_dir and session-scope grants flow through Compose().
- using ShellSyntaxTree; added to the Program.cs import list.
Behavior on swap: nothing changes for existing sessions. v2
ToolApprovalStore still gates shell tools. The new services are
constructed at startup but unused until ToolAccessPolicy integration
lands. HardDenyOverrides will activate immediately if an operator
drops a hard-deny-overrides.json file into ~/.netclaw/config/ — the
shipped defaults remain in force regardless.
Build: green. Full test suite passes (Cli 640, Daemon 504, Actors
1522, plus Configuration/Security/Channels). Slopwatch clean.
File headers present.
Next: ToolAccessPolicy.AuthorizeInvocation gets the gate-evaluator
integration path (feature-flagged so v2 remains the default while
operators opt in for testing).
…essPolicy
Activates the trust-zones approval pipeline as an auto-allow fast path
ahead of the v2 (verb, directory) matcher. The integration is
deliberately minimal: GateEvaluator only short-circuits to silent
execution when it returns Approved (read-only verb in trusted zone, or
clause matches a persisted/session verb pattern). If the new
evaluator hits HardDenied, the call denies with the gate's reason.
For NeedsPrompt, control falls through to the existing v2 5-button
prompt path so user-facing approval UX stays unchanged until the
prompt builder rewrite lands.
This is the load-bearing change for reminder reliability. Operators
configure broad audience baseline zones in netclaw.json
(ToolAudienceProfile.ReadFiles.Roots) and pre-approve verb patterns via
the CLI; reminders firing read-only verbs inside trusted zones now
auto-allow at the GateEvaluator without ever entering the prompt-
required v2 code path that previously blocked them.
ToolAccessPolicy.cs changes:
- Constructor accepts optional GateEvaluator + TrustStateComposer
(back-compat: existing call sites without these parameters get
identical v2-only behavior).
- CheckApprovalGate's shell branch invokes GateEvaluator ahead of the
v2 safe-verb short-circuit when:
isShell && !isMessy && both new services are present &&
context.SessionDirectory is set && shellCommand is non-empty.
Builds TrustState via composer (with empty session-scope lists for
now — those plumb through LlmSessionActor when the prompt UI moves
to the new 4-button shape). Evaluates command; on Approved returns
Allow; on HardDenied returns Deny with the GateEvaluator's category
prefix; on NeedsPrompt falls through to v2.
- Existing safe-verb short-circuit retained as fallback for sessions
that don't exercise the GateEvaluator fast path.
Program.cs changes:
- Constructs BashParser, GateEvaluator, TrustStateComposer locally;
registers each as singleton.
- Passes GateEvaluator + TrustStateComposer into ToolAccessPolicy
constructor.
What this delivers for binary-swap testing:
- Reminders that hit read-only verbs (grep, cat, ls, find, etc.) inside
the audience's baseline trusted zones (configured in netclaw.json's
ToolAudienceProfile.ReadFiles.Roots) now run silently without any
approval prompt.
- Verb patterns pre-approved in the new AudienceTrustStore at
~/.netclaw/config/trust-zones.json auto-pass.
- Existing v2 approvals (~/.netclaw/config/tool-approvals.json) are
still consulted for any command that falls through to v2.
- Hard-deny rules in the new structured DSL (compiled defaults +
hard-deny-overrides.json) fire here.
What's NOT yet wired (deferred to subsequent commits):
- v2 button clicks (Once / This chat / Always here / Always anywhere)
still write to the v2 ToolApprovalStore, not the new AudienceTrustStore.
Operators populate trust-zones.json via direct CLI commands or
config edit; the daemon never writes to it.
- Session-scope grants are passed as null to the composer; LlmSessionActor
plumbing lands when prompts switch to the new 4-button shape.
- 4-button prompt UI not built yet; existing v2 5-button row renders.
Build: green. Full test suite passes (Cli 640, Daemon 504, Actors
1522, plus all others). Slopwatch clean. File headers present.
Approvals are user decisions, not clock-bounded operations. The 5-minute auto-deny manufactured a race condition: when a user took longer than 5 minutes to click an approval button, the workflow's TaskCompletionSource silently transitioned to TimedOut. The late click then arrived at an already-terminated workflow, routing through a buggy retry path that re-authorized the tool, didn't find a matching grant, and threw ToolApprovalRequiredException — surfacing to the user as "I encountered an error executing a tool" with a correlation ID. No security benefit: the user is the authority. If they need 20 minutes (or 2 hours) to evaluate a prompt, that's their call. The system should wait, not pre-empt the decision on a timer. Changes: - SessionToolExecutionPipeline.cs:77 — default for null approvalTimeout flipped from TimeSpan.FromMinutes(5) to Timeout.InfiniteTimeSpan, matching the existing default at line 274 and LlmSessionActor's explicit InfiniteTimeSpan pass at line 1678. Net: every caller that passes null now waits forever for user response. - openspec/specs/tool-approval-gates/spec.md — Mid-turn approval pause requirement rewritten to mandate indefinite wait; the "Approval timeout auto-denies" scenario replaced with "Approval pause waits indefinitely for user response." - openspec/changes/approval-policy-trust-zones/specs/tool-approval-gates/spec.md — same edits in the trust-zones delta spec. Session passivation and daemon-restart approval recovery are tracked separately in netclaw-dev#939 and remain out of scope for this fix. This commit only removes the clock-driven auto-deny that was generating false errors on legitimate late clicks. Build: green. Full test suite passes (Cli 640, Daemon 504, Actors 1522, plus all others). Slopwatch clean. Specs re-validate.
…licy-v2 # Conflicts: # src/Netclaw.Actors/Sessions/LlmSessionActor.cs # src/Netclaw.Cli.Tests/Cli/DaemonClientMappingTests.cs
Messy commands (bash control-flow `for/while/case`, unbalanced quotes/brackets) cannot have verb-chain patterns extracted, so the approval prompt only offers Once + Deny and ApprovalContext.Patterns is empty. The IsOneTimeApprovalSatisfied retry-bypass guard required both sides' patterns to match — empty == empty technically passes the All() check, but the guards on lines 200/203 short-circuited to return false on empty inputs. Result: clicking Once on a complex bash command landed the retry into AuthorizeCoreAsync, hit the guards, threw ToolApprovalRequiredException — surfacing to the user as "I encountered an error executing a tool" with a correlation ID. Repro on production: session D0AC6CKBK5K/1778542266.328629 on 2026-05-11 ran a `for repo in ...; do ... worktree list ...; done` loop. Prompt fired with "complex command — only Once available." User clicked Once 28 minutes later (no longer auto-denied at 5 min since af645f7). Click landed on the now-living workflow, retry hit the bypass guard, failed with the same error message that previously came from late-click-on-timed-out-workflow. Fix: reorder IsOneTimeApprovalSatisfied so messy commands take a tool-name-match-only fast path before the empty-patterns guards. The per-retry cleanup at SessionToolExecutionPipeline.cs:467-475 still clears OneTimeApprovedToolName afterward, so the messy bypass cannot be reused for subsequent calls — bypass scope stays per-call as designed. Test added (DispatchingToolExecutorTests.One_time_approval_bypasses_for_messy_command_via_tool_name_match): - Sets up shell_execute with `for i in 1 2 3; do echo $i; done`. - First attempt throws ToolApprovalRequiredException with ApprovalContext.IsMessy=true and Patterns=[]. - Sets OneTimeApprovedToolName, leaves OneTimeApprovedPatterns empty (because Patterns is empty per messy contract). - Second attempt succeeds (no exception). - After bypass cleanup, third attempt re-throws (proves bypass is per-retry only). Build: green. Full test suite passes (Cli 640, Daemon 504, Actors 1528, plus Configuration/Security/Channels). Slopwatch clean. File headers present.
Refactors the messy-command ApprovedOnce test to follow the akka-testing-patterns skill — inheriting from Akka.Hosting.TestKit.TestKit instead of manual ActorSystem.Create + try/finally + Terminate. Test moves to its own file (MessyCommandOneTimeApprovalTests.cs) with proper ConfigureServices / ConfigureAkka overrides and ActorRegistry-based actor retrieval. The original DispatchingToolExecutorTests file's other one-time- approval tests still use the manual ActorSystem.Create pattern; those are pre-existing test smell that could be similarly refactored in a follow-up but are out of scope for this fix. The messy-command test gets a fresh file as the seed for proper-pattern adoption — future test additions in this area should follow the same shape. Per the akka-testing-patterns skill: - Inherits from Akka.Hosting.TestKit.TestKit (framework class) - ConfigureServices wires ToolConfig + EffectivePolicyDefaults + ToolAccessPolicy + ToolRegistry into the host's DI container - ConfigureAkka registers ToolApprovalActor in the ActorRegistry - Test method retrieves dependencies via Host.Services and ActorRegistry — no manual lifecycle code - Automatic actor system shutdown via TestKit fixture teardown (no try/finally for system.Terminate()) Functional behavior unchanged: same scenario, same assertions (IsMessy=true, empty Patterns, ApprovedOnce satisfies bypass on tool-name match alone, per-retry cleanup re-prompts). 1528/1528 Actors tests pass. Build: green. Slopwatch clean. File headers present.
Picks up the bash-comment lexing fix per Aaronontheweb/ShellSyntaxTree#25 — `#` starting a token outside quotes now begins a comment that runs to end-of-line and produces no AST tokens. Closes the cascade where comment text was extracted as the leading verb of the clause it sat in, leading to: 1. Misleading approval prompts: "Approve `# Get` in <dir>?" instead of "Approve `git pull` in <dir>?" 2. Persistence-versus-recheck verb-set mismatches that broke ApprovedSession on commented commands. The persistence path saw one verb set ([# Get, echo]); the retry-authorization path saw a different finer-grained set ([# Get, curl, jq, echo]) when decomposed per-pipeline-segment. The unmatched verbs (curl, jq) threw ToolApprovalRequiredException — surfacing as "I encountered an error executing a tool" in Slack. Both symptoms collapse once `BashLexer` strips comments at lex time: both extraction passes see the same clauses with the same leading verbs, so they agree. Adds two regression smoke tests in ShellSyntaxTreeIntegrationTests: - Leading_line_comment_is_stripped_from_clause_extraction: Confirms `# fetch the latest\ngit pull origin main` produces one clause with verb chain `git pull` (not `# fetch`). - Hash_inside_double_quotes_is_not_a_comment: confirms `# inside double quotes is preserved as a literal arg character (POSIX rule that # is only a comment when starting a word AND outside quotes). Build: green. Full test suite passes (Cli 640, Daemon 504, Actors 1528, plus Configuration/Security 9 new ShellSyntaxTree integration tests). Slopwatch clean. File headers present.
…oser When an audience's ReadFiles profile is configured with `Mode: All`, the composer was still only reading the (now-meaningless) Roots list, handing TrustState an empty zone set. The zone gate then prompted on every path operand even though the operator had explicitly declared the audience as filesystem-unrestricted at the profile layer. The composer now detects Mode=All and threads `trustsAllPaths: true` into TrustState, which short-circuits IsPathInTrustedZone. Mode=Roots and Mode=None still rely on the explicit Roots list (None typically empty). The verb-pattern gate is unaffected — mutating verbs without a pattern match still prompt regardless. Geography is the only thing waived here.
0.1.4-alpha lands the issue netclaw-dev#27 fix: the parser extends the verb chain greedily through every "verb-like" token until it hits a flag (-x) or a path (anything containing / or .). Production hit on `git worktree list` (extracted as `git worktree`, mismatching at retry time) is fixed — multi-token CLI subcommands now extract cleanly without per-CLI tables. Side effects: - Auto-proposed verb patterns are narrower. `git push origin main` now proposes `git push origin main *` instead of `git push *`. This is intentionally tighter — approving the specific argument set is safer than approving the whole verb family. - Test expectations updated for the new shape. Three new integration cases cover stop-at-flag, stop-at-path, and the multi-token CLI subcommand regression directly. - TrustState xmldoc updated: verb-pattern matching is exact verb-chain equality + arg-glob suffix, so a stale `git push *` no longer matches a `git push origin main` invocation. Operators with persisted `git push *` from older runs will be re-prompted on the new shape. 557 Security tests + 314 Configuration tests + 1528 Actors tests pass.
The v2 approval matcher's `ExtractVerbChain` was capping at depth 2,
truncating multi-token CLI subcommands like `freshdesk ticket list` and
`git worktree list` to two tokens (`freshdesk ticket`, `git worktree`).
The truncation surfaced two ways in production:
- Approval prompts displayed misleading verb names ("Approve `freshdesk
ticket` in this session?" for what is really `freshdesk ticket list`).
- Verb-chain mismatch between approval-prompt time and retry time
threw `ToolApprovalRequiredException` mid-flight, surfacing as
"I encountered an error executing a tool" with a correlation ID.
The 0.1.4-alpha ShellSyntaxTree bump shipped a greedy verb-chain
extractor (issue netclaw-dev#27), but it was only wired into the new
GateEvaluator/TrustStateComposer code path — which isn't on the live
runtime approval flow yet (trust-zones milestones B-N pending). This
puts it on the v2 path so the live prompt benefits immediately.
`ShellApprovalSemanticsBase.ExtractVerbChain` now delegates to
`BashParser.Parse(...).Clauses[0].Verb.Joined` for greedy extraction.
The path-aware/side-effect short-circuit (cap at depth 1 for cat, grep,
find, ls, echo, printf, etc.) is preserved as a post-check so positional
search patterns and target paths don't bake into persisted approval
keys (`grep secret /var/log/syslog` still extracts as `grep` alone).
`maxDepth` is now an upper bound rather than a default cap; the
default is `int.MaxValue` so callers get greedy extraction unless
they explicitly request a tighter chain.
Tests:
- ShellTokenizerTests: existing depth-2 expectations updated to greedy
shape (`git push origin main`, `kubectl delete pod my-pod`,
`docker compose up`). Path-aware verbs unchanged (cat, grep, ls
still cap at 1).
- New regression theory `ExtractVerbChain_extracts_multi_token_cli_subcommands`
pins the production hits: freshdesk ticket list, git worktree list,
gh pr view, kubectl get pods.
- ToolApprovalGateTests: gate test renamed and re-asserted to expect
the greedy chain on `git push origin main`.
561 Security + 1530 Actors + 314 Configuration tests pass.
…ates Compound commands like `cd /repo && git checkout -b feature/foo` were failing ApprovedSession retry with ToolApprovalRequiredException. Root cause: the v2 matcher extracted each clause's path argument independently. The `git checkout` clause has no anchored path arg of its own (feature/foo's slash doesn't make it a path token), so the candidate landed as (git checkout, null). At persistence time the candidate inherited cwd → session_dir → the session-scratch guard at PersistApprovalCandidatesAsync dropped the grant on the floor → retry saw the verb as unapproved → throw → "I encountered an error executing a tool". ShellSyntaxTree 0.1.4-alpha already tracks cd-in-compound cwd propagation: every clause that follows a `cd X` in the same compound (including across `bash -c "..."` boundaries) carries a synthetic IsCwdAttribution arg whose Resolved is the absolute tilde-expanded cd target. The matcher now consumes that signal: - POSIX ExtractCandidates iterates BashParser.Parse(command).Clauses directly. For each clause: own anchored path arg wins over cd attribution, then falls back to cwd attribution. Consecutive Pipe clauses fold into one approval unit so `cat /etc/hosts | wc -l` stays one decision. - Side-effect verbs (echo, printf, :, true, false) opt out of cd inheritance — they ignore cwd anyway and the IsPureSideEffect skip in ApprovalPatternMatching requires a null Directory. - Windows keeps the legacy ShellTokenizer-based path; ShellSyntaxTree is bash-only. Tests added in ShellApprovalMatcherTests: - cd target propagates to verbs without anchored path args - Multiple cd hops — latest wins - bash -c "cd X && verb" — verb inherits X - Explicit own path beats cd attribution (`cd /tmp && dotnet test /home/foo` → /home/foo) - Side-effect verbs do not inherit cd - Pipe chain collapses into a single candidate Pre-existing cd-target test updated to assert the new contract (subsequent verb's Directory inherits cd target instead of staying null) and to use generic path placeholders rather than developer- specific home directories. 567 Security + 1537 Actors + 314 Configuration tests pass.
The approval prompt for `cd ~/x && git checkout -b feature/foo` was displaying "Saved for this chat: cd, git checkout in 2 directories" even though both clauses operate on the same folder. The header's distinct-directory counter saw two strings — `~/x` from cd's own path arg and `/home/<user>/x` from git checkout's inherited cwd attribution — and reported them as separate locations. The mismatch was self-inflicted: ExtractCandidates' path branch read `arg.Raw` (user-facing form) while the cwd attribution branch reads `arg.Resolved` (parser-resolved absolute form, which is always pre-expanded for ~/, $HOME, and relative segments). For the same logical directory, the two branches produced different strings. Switch the path branch to `arg.Resolved` when available (falling back to `arg.Raw` for arg kinds where the parser doesn't resolve). Path classification still runs on `arg.Raw` so branch names whose Resolved happens to look path-like don't get misclassified. Tests: - New: ExtractCandidates_normalizes_tilde_cd_to_absolute_path_so_clauses_share_one_directory pins the production header behavior. - Updated: ExtractCandidates_applies_file_parent_rule now expects the absolute home directory (Environment.GetFolderPath) rather than the raw `~`, matching the new canonicalization contract. - Sanitized: ExtractCandidates_strips_path_from_verb no longer uses /home/petabridge in its test fixture. 568 Security + 1537 Actors tests pass.
ApprovedSession on standalone shell verbs with no anchored path argument (curl https://..., gh pr list, git status) failed retry with ToolApprovalRequiredException. Repro: D0AC6CKBK5K/1778588682.018849, 13:37:31-39 — four parallel curl calls to api.github.com, each ApprovedSession sequentially. 1ms after the final approval, the executor threw and the whole turn failed. Root cause: in PersistApprovalCandidatesAsync, every candidate's effective directory fell back to pending.Cwd when candidate.Directory was null. For a curl call with a URL operand (URLs are explicitly excluded from IsPathToken) and no preceding `cd` to attribute, the candidate's Directory was null → effectiveDirectory resolved to the session directory → the session-scratch dead-on-arrival guard fired and skipped the verb entirely. The verb never landed in ToolApprovalActor._sessionApprovals; retry's IsApproved check returned false; throw. Session-scope entries are matched verb-only at lookup time — ToolApprovalActor._sessionApprovals is keyed by (sessionId, audience, tool) and IsSessionApproved consults only the verb, never the directory. Threading session_dir through here just fed the dead-on-arrival filter that drops folder-scoped entries with no viable scope. Fix: only apply the cwd fallback + session-scratch guard to persistent scope (Always / Everywhere); for session scope use candidate.Directory directly so verbs with null Directory persist under the null-directory bucket and remain queryable. Refactored the bucketing branch into the internal-static `LlmSessionActor.BuildApprovalBuckets(...)` helper so the scope-vs- directory logic is unit-testable without spinning up an actor system. Added BuildApprovalBucketsTests pinning: - Session scope + no path arg → verb persists in null-directory bucket - Session scope + concrete directory → verb persists in that bucket - Persistent scope + cwd resolving to session_dir → dropped (existing) - Persistent scope + concrete directory → kept (existing) - Global wildcard → directory=null regardless of inputs - Pure side-effect verbs → never persisted at either scope This bug class evaporates structurally under the trust-zones architecture, where session-scope grants are verb-pattern globs on `LlmSessionActor.SessionVerbPatterns` with no directory dimension at all. The fix here is necessary for v2's remaining lifespan and goes away on the trust-zones cutover. 568 Security + 1543 Actors tests pass.
Trust-zones tasks 4.1-4.4. The user's Session click on either gate (zone or verb) needs a place to live for the actor's lifetime that doesn't survive recovery. Adds: - `SessionScopeGrants` helper class with `AddTrustedZone(string)` / `AddVerbPattern(string)` mutators backed by HashSets. Verb-pattern dedupe is case-insensitive to mirror `ApprovalPatternMatching`'s contract; trusted-zone dedupe uses `ToolApprovalEntryComparer`'s platform-correct comparer so a planted directory under a case-sensitive filesystem can't be redeemed by a casing variant. - `LlmSessionActor._sessionScopeGrants` field placed under the existing "Transient state (not persisted)" comment block alongside `_buffer` and `_inFlightReminderIds`. The actor wires it into the gate evaluator when §6 (`ToolApprovalWorkflow`) lands. The in-memory-only contract is pinned structurally: `SessionSnapshot_does_not_expose_session_scope_grant_storage` asserts the snapshot type has no field/property name hinting at trustedZones, sessionVerbPatterns, or sessionScopeGrants. A future refactor that tries to silently move session-scope into the persistence path fails the test rather than violating the trust-zones design intent. This bug class — session-scope grants leaking through persistence — also doesn't exist in v2 because v2 has no session-scope storage at all (it uses `ToolApprovalActor._sessionApprovals` which is per-actor in-memory). The new SessionScopeGrants makes the v2 behavior explicit and testable, and gives §6 a clear handle to write into. 10 new unit tests (add/dedupe/whitespace/case/structural pin) + full Actors suite (1553) pass.
Tasks 6.1-6.3 of approval-policy-trust-zones. Introduces the pure- functional core of the per-call approval workflow: given a GateEvaluation and a sequence of user ApprovalDecisions, emits the side-effect list the actor's dispatcher applies to channels, session-scope grants, the persistent store, and the approval channel. Three new types: - `ToolApprovalWorkflow` (record): per-call snapshot with CallId, ToolName, Audience, Gate, Stage. Immutable; transitions produce a new instance via `with`. - `WorkflowStage` (enum): AwaitingZoneResponse, AwaitingVerbResponse, Complete. Either awaiting stage may short-circuit on Deny/TimedOut; the prompt corresponding to a stage is skipped when not needed. - `WorkflowEffect` (record hierarchy): EmitZonePrompt, EmitVerbPrompt, AddSessionZone, AddSessionVerbPattern, PersistZoneGrant, PersistVerbPatternGrant, CompleteCall. Effects are ordered so the dispatcher can iterate without reordering. Engine API (`Netclaw.Actors.Sessions.WorkflowEngine`, internal static): - `Start(callId, toolName, audience, gate) -> (state, effects)` - `OnResponse(workflow, decision) -> (state, effects)` Sequencing per the trust-zones spec: 1. Approved or HardDenied gates terminate at Start (defensive — the executor should handle these before reaching the workflow). 2. NeedsPrompt with zone prompt issues the zone prompt; verb prompt queued. 3. After zone response with non-Once scope, AddSessionZone or PersistZoneGrant effect lands per untrusted path (trust-all-or- nothing batch). 4. Zone Approved advances to verb stage if verb prompt is needed; else terminates with ApprovedOnce. 5. Verb response writes its grant similarly and terminates. 6. Deny / TimedOut at either stage terminates immediately; zone grants approved at the prior stage are NOT rolled back (user approved geography, only declined the verb). 19 new tests cover every transition: auto-allow, hard-deny, zone-only, verb-only, both-prompts, ApprovedOnce / Session / Always / Everywhere at each stage, Deny and TimedOut at each stage, and invariant guards (null callId, null gate, response on completed workflow). The actor-side dispatcher and wire-up land in §6.4. The engine itself ships green: 1572 Actors tests + 0 slopwatch + headers verified.
Tasks 6.4-6.6 of approval-policy-trust-zones. Connects the WorkflowEngine pure core (b23185f) to the actor's receive handlers and the v2 pipeline so the trust-zones two-prompt flow runs end-to-end when `GateEvaluator` produces a NeedsPrompt decision. Plumbing: - `ToolApprovalContext` carries `Gate: GateEvaluation?`. Populated by `ToolAccessPolicy.AuthorizeInvocation` when the GateEvaluator fast path runs and decides NeedsPrompt; null for v2-matcher-only calls (Windows shell, non-shell tools, or shell calls without an active GateEvaluator registration). - `ToolInteractionRequest.Gate` mirrors the same field. The pipeline copies it from the approval context when emitting the request. - `ToolExecutionContext` gains `SessionTrustedZones` and `SessionVerbPatterns` (live `IReadOnlyCollection<string>` views of the actor-owned `SessionScopeGrants` HashSets). The pipeline threads them through `BuildToolExecutionContext`; the policy reads them when composing `TrustState`. Live views matter: mid-workflow zone Session grants are visible to the verb stage's retry on the same call. - `ToolExecutionContext.WorkflowApprovedThisCall` is a per-retry bypass flag. The pipeline sets it after a workflow-driven approval; `DispatchingToolExecutor.AuthorizeAsync` returns early when set, so the immediate post-approval retry bypasses every gate. Cleared in the `finally` of `ExecuteToolAttemptAsync` (one-shot semantics). Actor side (`LlmSessionActor`): - New transient field `_activeWorkflows: Dictionary<string, ToolApprovalWorkflow>` lives next to `_pendingToolInteractions`. - New constructor dependency `AudienceTrustStore?` injected via `SessionToolServices`. Wired in `Program.cs`. - `Command<ToolInteractionRequest>` branches on `msg.Gate`: when populated, starts a workflow via `WorkflowEngine.Start` and dispatches the initial effects. Stores `PendingToolInteraction` for routing metadata in both branches; v2 emit is suppressed when the workflow takes over so the actor controls prompt rendering. - `CommandAsync<ToolInteractionResponse>` branches on `_activeWorkflows`: when active, runs `WorkflowEngine.OnResponse` and dispatches the resulting effects; otherwise falls through to the v2 persistence + complete path unchanged. - `DispatchWorkflowEffects` translates each effect: - `EmitZonePrompt` / `EmitVerbPrompt` → `EmitOutput` of a workflow-flavored `ToolInteractionRequest` (Kind=`approval_zone` / `approval_verb`, body text mentions the audience + paths / command). Adapters fall back to the v2 4-button rendering on unknown Kinds for now; §7-§8 adds distinct renderings. - `AddSessionZone` / `AddSessionVerbPattern` → write to the actor's `_sessionScopeGrants`. Visible to subsequent gate evaluations via the live-view collections on `ToolExecutionContext`. - `PersistZoneGrant` / `PersistVerbPatternGrant` → write to `_audienceTrustStore`. Logged-and-dropped when the store is null (transitional fail-safe; the trust-zones DI registration always provides it in the daemon). - `CompleteCall` → remove the pending interaction, resume the watchdog, signal `_approvalChannel.Complete(callId, decision)` so the pipeline's blocked `WaitForApprovalAsync` returns. The pipeline then sets `WorkflowApprovedThisCall = true` (because `ctx.Gate is not null`) and retries; the executor's bypass short-circuits and the tool runs. Watchdog: pause-for-approval-wait fires once at request arrival; resume runs from the terminal `CompleteCall` dispatcher. Two-prompt workflows hold the watchdog paused across both stages — the zone-stage response triggers `OnResponse → EmitVerbPrompt` without toggling watchdog state. Scope semantics on `CompleteCall.Decision`: the engine always emits `ApprovedOnce` on the success path because by that point the workflow has already applied the user's chosen scope via the preceding effects (AddSession* for Session clicks, Persist* for Always/Everywhere clicks). `ApprovedOnce` from the engine reaches the pipeline as a uniform "approved, please retry" signal; the pipeline's existing `OneTimeApproved*` setup runs harmlessly alongside the `WorkflowApprovedThisCall` bypass and gets cleared by the same finally block. v2 path (no `Gate`): completely untouched. Adapters keep rendering v2 prompts; persistence keeps flowing through `PersistApprovalCandidatesAsync`. The trust-zones path only activates when both the GateEvaluator and TrustStateComposer are registered AND the shell command produces a NeedsPrompt evaluation. 1572 Actors + 568 Security + 314 Configuration + 504 Daemon tests pass. Slopwatch clean. Headers verified. Tasks 6.1-6.6 marked done in tasks.md. Binary swap deferred per user direction — review and dogfood after operator-level smoke testing.
…ices Replaces the silent-fallback drop-with-warning in the workflow dispatcher: persistent grants now reach `AudienceTrustStore` unconditionally, and the store is required at SessionToolServices construction time. A missing registration crashes at DI build rather than swallowing an Always/Everywhere grant the user clicked. - `SessionToolServices.AudienceTrustStore` drops the `?` and becomes a required positional parameter (moved before the trailing optionals to keep the record's required-then-optional shape). - `Program.cs` uses `GetRequiredService<AudienceTrustStore>` — DI validation fails loudly if the trust-zones registration block was skipped. - `LlmSessionTestExtensions` registers a unique per-test temp-file-backed `AudienceTrustStore` so test hosts that wire `SessionToolServices` continue to construct cleanly. - `LlmSessionActor.DispatchWorkflowEffects` dereferences `_audienceTrustStore` directly on Persist* effects; the helper `RequireAudienceTrustStore()` is gone. A null deref here would mean an actor was somehow built without tool services AND yet received a workflow effect — that combination is structurally impossible given the receive-handler gate. CLAUDE.md "no silent fallbacks": a saved-Always grant that doesn't save is exactly the kind of security-relevant fail-soft the rule exists to prevent. 1572 Actors + 504 Daemon tests pass. Slopwatch / headers clean.
Branches `SlackApprovalBlockBuilder` on `request.Kind` to render three distinct prompt shapes: - `approval` (legacy v2) — unchanged. Existing 5-button row, single prompt per call. All existing tests pass. - `approval_zone` — trust-zones zone gate. Header asks "Trust this path?" / "Trust these N paths?"; body bullet-lists the untrusted paths; button row is `Once / This chat / Trust <path-or-count> / Deny`. ApproveEverywhere is omitted because a zone is always a concrete directory (no "anywhere" form). - `approval_verb` — trust-zones verb gate. Header asks "Approve this verb pattern?"; body shows the command and the proposed pattern; button row is `Once / This chat / Always <pattern> / Always anywhere / Deny`. ApproveEverywhere stays because verb patterns DO have a global form. Button label rendering: - The `ApproveAlways` button gets a dynamic label per Kind: "Trust /etc/nginx" / "Trust all 3 listed" for zone prompts, "Always git push origin main *" for verb prompts. - `TruncateButtonLabel` enforces the 76-char Slack cap with a trailing ellipsis. The full path/pattern is always preserved in the prompt body, so truncation is purely cosmetic — the operator can still see what they're approving before they click. - Static labels (`Once`, `This chat`, `Deny`, etc.) stay sourced from `ApprovalOptionKeys` constants and don't pass through the truncator; they're within the cap by construction. Resolution message rendering branches per Kind: - Zone Always → "Saved zone: <paths>" - Zone Session → "Saved zone for this chat: <paths>" - Verb Always → "Saved verb: <pattern>" - Verb Everywhere → "Saved verb (anywhere): <pattern>" - Verb Session → "Saved verb for this chat: <pattern>" - Once / Deny → existing "Approved (no save)" / "Denied" Response routing is unchanged: the existing interaction handler decodes by callId via the actor's `_activeWorkflows` dispatch, so no Kind-aware routing logic is needed at the handler level. Tests: 22 cases (10 legacy v2 still passing, 12 new for trust-zones). New cases cover single-path / multi-path zones, long path / long pattern truncation, four-button vs five-button rows, and all resolution-line variants. 1584 Actors tests pass. Discord (§8) lands after Slack dogfood validates the shape.
Windows CI runners on `pr_validation.yml` have been failing the trust-zones path-extraction tests since the BashParser integration landed. The failures aren't bugs — the v2 matcher gates the BashParser fast path behind `!OperatingSystem.IsWindows()`, so Windows falls through to the legacy ShellTokenizer which doesn't carry cwd attribution or `arg.Resolved` canonicalization. Tests pinning the trust-zones path semantics correctly fail to match POSIX-shaped inputs on a Windows host. Uses xunit.v3 structured skip (`[Fact(SkipUnless = nameof(IsPosix), Skip = "...")]`) so the affected tests surface as "Skipped" in the CI log rather than hiding behind an early-return — preserves the platform-gap signal and matches the convention used elsewhere in the suite (`ShellTokenizerTests.WindowsAnchoredPathCases` etc.). Affected (10 fixtures, all under `Netclaw.Security.Tests`): - ShellApprovalMatcherPathExtractionTests (8 new + 2 modified pre-existing tests that exercise BashParser cwd attribution and `arg.Resolved` canonicalization) - TrustStateComposerTests.Compose_uses_home_directory_override_for_tilde_expansion (Path.Combine produces backslash-mixed paths on Windows) - ShellTokenizerTests inline data for `cp /src/a.txt /dst/b.txt` / `cat /etc/hosts.conf` extracted into a POSIX-gated theory (Path.GetDirectoryName platform-aware separator) The trust-zones path is POSIX-first by design — ShellSyntaxTree is bash-only and the legacy v2 path is being retired. Once the rewrite finishes, Windows shell handling moves to whatever shell-parser shape the trust-zones spec lands on for cmd.exe / PowerShell. 568 Security tests pass on Linux. Windows CI should now show 568 - 10 skipped = 558 passed instead of 9 failed.
`SessionMemoryObserverActor.cs` was created `MemoriesDistilledV2`
events at runtime but the type was never wired into the protobuf
serializer, so every persistence attempt failed under strict
serialization:
[ERR] Rejected to persist event type [MemoriesDistilledV2]
due to [No serializer binding found for type MemoriesDistilledV2.
Configure a binding in 'akka.actor.serialization-bindings'
or set 'akka.actor.serialization-settings.allow-unregistered-types
= true' to use the default fallback.]
Three sessions hit this in a four-minute window today. The memory
observer's distillation events were being silently dropped from the
journal. Fix the four-step binding:
- Add `MemoriesDistilledV2Proto` and `ProposedMemoryContextProto`
to `netclaw_messages.proto` so Grpc.Tools generates the wire
types.
- Add `MemoriesDistilledV2 → MemoriesDistilledV2Proto` (and back)
mappings in `NetclawProtoMapper`. ProposedMemoryContext maps
through its three string fields; the parent type carries the
repeated anchors, the repeated proposals, and the timestamp.
- Register `MemoriesDistilledV2Manifest = "mdv2-v1"` in
`NetclawProtobufSerializer.TypeToManifest` and add the
`FromBinary` branch.
- Append `typeof(MemoriesDistilledV2)` to the bound types list in
`NetclawAkkaHostingExtensions.WithNetclawSerialization`.
Regression coverage in `SerializationRoundTripTests`: two new cases
exercise a populated event and an empty-collections edge case
through the real `Sys.Serialization` pipeline. The tests would have
caught this gap before production.
Baseline regen via `dotnet slopwatch init --force`: trust-zones
POSIX-only test skips (SW001) and upstream's `ConfigWatcherService`
delays (SW004) are now baselined with current line numbers.
Pre-existing entries retained with refreshed line numbers where the
underlying files moved.
1586 Actors tests pass. Slopwatch clean.
See also netclaw-dev#961 for the broader test-coverage gap that let this slip
through (Akka serialization verification not enabled in test
configurations).
Temporary diagnostic logs in ToolAccessPolicy.AuthorizeInvocation to pin down why the trust-zones workflow isn't firing in production despite the §6 wire-up shipping in 0718cb2. Two INFO log lines: - `gate_fastpath_eval` — emitted for every shell call before the fast-path block runs, with each precondition value as a structured field (isShell / isMessy / hasEvaluator / hasComposer / hasContext / hasSessionDir / hasCommand). Whichever condition is false explains why Gate stays null. - `gate_fastpath_result` — emitted only when the fast-path block ran, with the GateEvaluator's decision (Approved / NeedsPrompt / HardDenied), hard-deny reason, whether ZonePrompt/VerbPrompt are populated, and the unparseable flag. Reproduces the diagnostic data on a single approval prompt cycle. Revert after the root cause is fixed in a follow-up. Adds ILogger<ToolAccessPolicy> as an optional constructor parameter following the established DispatchingToolExecutor pattern (default NullLogger when DI doesn't provide one — tests that don't wire a logger keep compiling).
Construct ToolAccessPolicy twice — once locally (config-time use by ToolRegistry.WithFirstPartyTools, no logger needed) and once as a DI factory that resolves ILogger<ToolAccessPolicy>. Without the factory the manual instance from line 728 was registered as the singleton and DispatchingToolExecutor was resolving it WITHOUT a logger, which silently suppressed the gate_fastpath_* diagnostic from 64fc251. Temporary scaffolding for #962. Revert with the gate_fastpath logs.
…xecutor DispatchingToolExecutor's factory was capturing the local toolAccessPolicy variable (constructed without a logger), so it held the no-logger instance regardless of the DI-registered logger-equipped factory we added in 79c9fda. Result: gate_fastpath_* log lines never fired because _logger on the executor's policy was NullLogger. Switch to sp.GetRequiredService<ToolAccessPolicy>() so the DI container hands over the factory-produced (with-logger) instance. Temporary scaffolding alongside the rest of the diag commits; revert when #962 is resolved.
ToolAccessPolicy.ExtractShellCommand was using a direct `command is string text` pattern match that fails for the JsonElement values LLM-generated tool calls actually deserialize to. Real-world impact: - The hard-deny pre-check at AuthorizeInvocation line 152 was silently no-op for every real shell call — _shellCommandPolicy.Evaluate was gated on `shellCommand is not null`, so destructive-command rules weren't actually firing. - The trust-zones GateEvaluator fast-path in CheckApprovalGate was also gated on `!string.IsNullOrEmpty(shellCommandForGate)`, so it never entered. The actor fell back to v2 for every approval — exactly the symptom diagnosed via the gate_fastpath_eval log showing `hasCommand=False` despite all other preconditions True (issue #962). Route through ToolArgumentHelper.GetString (the same helper the matcher's GetCommand uses), which properly unwraps JsonElement via .GetString(). Both call sites benefit: - Hard-deny rules now evaluate against the real command text. - GateEvaluator fast-path runs on shell calls, populating Gate on ApprovalContext, which routes through the trust-zones workflow on LlmSessionActor instead of v2. 1586 Actors tests pass. Slopwatch clean. Worth a follow-up issue: three places extract "Command" from tool args (this one, ShellApprovalMatcher.GetCommand, ShellTool.cs). Consolidating to a single helper would prevent the JsonElement type-mismatch class of bug from recurring.
…truction Cleanup pass on the diagnostic scaffolding that surfaced the JsonElement-extraction bug (256b863): - ToolAccessPolicy.CheckApprovalGate: gate_fastpath_eval and gate_fastpath_result drop from LogInformation to LogDebug, gated by IsEnabled(LogLevel.Debug) so the structured-field argument allocations are skipped on the hot path when nobody is listening. Production at Info default emits nothing; an operator debugging trust-zones flow can set `Logging.LogLevel."Netclaw.Actors.Tools.ToolAccessPolicy": "Debug"` in netclaw.json (hot-reloads now per netclaw-dev#960) and the decision flow lights up for the next call. No code change, no swap required. - Program.cs dual-construction comment rewritten to document the pattern as permanent rather than scaffolding. The two ToolAccessPolicy instances are functionally identical except for the logger — local instance feeds the synchronous WithFirstPartyTools call (helper tools that don't need logging), DI factory feeds DispatchingToolExecutor (runtime gate path that does). Removing the duplication would require a Func<ToolAccessPolicy> refactor or mid-config service- provider build, neither of which justifies the complexity for two identical stateless objects. The runtime fixes from this session stay in place: - ExtractShellCommand uses ToolArgumentHelper.GetString so JsonElement-valued args unwrap correctly (real bug, not scaffolding). - DispatchingToolExecutor resolves ToolAccessPolicy from DI so the logger-equipped instance reaches AuthorizeInvocation (correct DI shape regardless of the diagnostic). 1586 Actors tests pass.
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
OpenSpec change proposing a breaking redesign of the persistent tool-approval store and the Slack/Discord approval prompt UX. No code changes in this PR — the proposal, design, delta specs, and tasks list.
/opsx-applywork follows in subsequent PRs.What's in scope
(verb, directory)ApprovalEntryreplaces the v1 flat string list. v1 file quarantines to.v1.bakon first read; no automatic migration (we have no production users).safe-verbs.linux.json,safe-verbs.windows.json) plus the agent's existingsession_dirand optionalproject_dirform a three-position policy: auto-run when both axes match; prompt otherwise; hard-deny list unchanged. ReusesToolAudienceProfileResolverand the symlink-segment guard fromScopedFileAccessPolicy.project_dirif set, elsesession_dir. Today it inherits the daemon-process cwd, which is a footgun.ShellTokenizerreturns no verb chains forfor/while/do/done/etc. or unbalanced quotes/brackets. Approval gate offers onlyOnceandDenyfor messy input. Stops the on-disk store from accumulating fragments likedone,for pid,awk {print $2}).Once/This chat/Always here/Always anywhere/Deny, with danger styling onAlways anywhereandDeny. Replaces the v1Patterns+Directory Rootsbody sections with a singleApprove in <cwd>?header + verb bullets. Resolution message collapses to one line.netclaw approvals trust-verb <verb>writes a global wildcard(verb, null)entry;listandrevokelabel entries by scope (<verb> in <dir>/<verb> anywhere).set_working_directoryearly, tool-description rewrite, shell failure-path hint pointing atset_working_directory. Three new eval cases (positive, negative, recovery) lock adoption in. Schedule-creation flow proactively suggests pre-approval for unattended verbs.Phasing
The
tasks.mdplans two implementation PRs under this single change:Validation
Test plan
D0AC6CKBK5K/1778238489.065719?trust-verbis the only path to(verb, null)and that we're not regressing anything inlist/revoke.