diff --git a/Directory.Packages.props b/Directory.Packages.props index 29c3bfc9d..a7f323292 100644 --- a/Directory.Packages.props +++ b/Directory.Packages.props @@ -106,4 +106,11 @@ + + + + \ No newline at end of file diff --git a/SILENT_FALLBACK_AUDIT.md b/SILENT_FALLBACK_AUDIT.md new file mode 100644 index 000000000..feda1286a --- /dev/null +++ b/SILENT_FALLBACK_AUDIT.md @@ -0,0 +1,108 @@ +# Silent-Fallback / Silent-Discard Audit — Tool-Call Surface + +**Origin:** session `D0AC6CKBK5K_1781115410_840529` (memory `e9a72b27`). The agent passed +`"TimeoutSeconds":"1200"` to extend a shell timeout; Netclaw only recognizes the meta key +`_timeout_seconds`, so the value was silently dropped, the timeout fell back to a 90s default, +and the agent got **no signal** — forming a false belief that fed a stuck loop. + +**Constitution rule violated:** *"When something fails or is misconfigured, fail loudly — do +not silently degrade to a default… on security-relevant paths they can silently escalate +privileges."* The rule is enforced on the **config** surface (`additionalProperties:false` + +`ConfigSchemaDoctorCheck`) but **not** on the live **tool-call argument** surface. That +asymmetry is the gap. + +**Method:** 3 parallel auditors (arg/meta/pipeline layer; tool implementations; policy/security +layer), identical rubric. CRITICAL finding independently re-verified by hand and down-graded. + +**Resolution status (change `loud-tool-arg-validation`):** findings #1, #2, #4, #5, #6, #7, +#8, #9, #10 are **FIXED** by this change (unknown-key validator at the dispatcher, +strict value binding in the generator + `ToolArgumentHelper`, pipeline-side meta-value +rejection, `ComputeEffectiveTimeout` clamp/floor notices via `ToolExecutionContext.Notices`, +provider-boundary args-parse sentinel, `web_fetch` format validation + truncation notice, +`list_webhooks` Filter honored). The latent `GetInt32` uncaught-throw is also fixed +(`TryGetInt32`). Findings #3, #11, #15, #17 (policy layer) remain **OPEN — parked** for a +security owner, recorded as out-of-scope open questions in the change proposal. #12–#14, #16 +(BORDERLINE) remain open, unchanged. + +--- + +## The class unifies into 3 mechanisms + standalones + +Most of the 17 sites are not independent bugs — they are three repeated shapes: + +- **M1 — unknown / near-miss key silently dropped.** The original bug. Lives at *two* layers: + generated `ParseArguments` (all tools) and `ToolCallMeta.ExtractFrom` (meta keys, exact-match). +- **M2 — present-but-invalid value silently coerced to a default.** `_ => null`/`_ => false` + switch arms; malformed args JSON → null args. +- **M3 — requested value silently clamped/overridden.** timeout floor & ceiling; format + fallback; output/byte truncation with no marker. + +A loud-by-default tool-arg seam (reject unknown keys + "did you mean" + surface every +override) closes M1–M3 for all ~20 tools at ~3 chokepoints. + +--- + +## Findings (severity-ranked, de-duplicated) + +| # | file:line | mechanism | what's silently handled | verdict | sev | minimal loud fix | +|---|---|---|---|---|---|---| +| 1 | `Netclaw.Tools.Generators/NetclawToolGenerator.cs:234-296` + `DispatchingToolExecutor.cs:67` | M1 | **any** unknown/misspelled arg key, all tools (the exact `TimeoutSeconds` mechanism) | VIOLATION | HIGH | diff supplied keys vs schema props (+ meta keys); return `Error: unrecognized argument 'X'. Did you mean 'Y'?` | +| 2 | `Netclaw.Tools.Abstractions/ToolCallMeta.cs:69,80,95` | M1 | near-miss meta keys (`TimeoutSeconds`, `_timeoutSeconds`, `_timeout-seconds`) — exact-match `TryGetValue`, unlike normal args which use `ToolArgumentHelper.TryGetValueFlexible` | VIOLATION | HIGH | reuse the flexible matcher for meta keys; emit "did you mean `_timeout_seconds`?" notice on near-miss | +| 3 | `ToolAudienceProfileResolver.cs:70-71,175-192` | (policy) | audience `Allowlist` profile silently does **not** govern tools outside a hardcoded set (memory tools, `search_tools`, `load_tool`, `spawn_agent`, `check_background_job`) → always allowed | VIOLATION / maybe by-design | HIGH (re-classified from CRITICAL) | product decision: default-deny unmanaged first-party tools under Allowlist, **or** document + surface that the allowlist is non-authoritative. Approval gate still applies (mitigates). | +| 4 | `ToolArgumentHelper.cs:111,129,145` (`_ => null`) + generator `:260,274,288` (`?? 0/0.0/false`) | M2 | present-but-unparseable scalar (`"abc"` int, `"yes"` bool) → coerced to `0`/`false` | VIOLATION | HIGH | distinguish absent vs present-but-unparseable; throw/surface on the latter | +| 5 | `ToolCallMeta.cs:89,103` (`_ => null`/`_ => false`) | M2 | malformed `_timeout_seconds`/`_background` value → meta silently empty → default | VIOLATION | HIGH | surface "ignored `_timeout_seconds=\"1200ms\"` — expected positive int; used default" | +| 6 | `Netclaw.Providers/SelfHosted/OpenAiCompatibleChatClient.cs:779-792` (`TryDeserializeArguments`) | M2 | malformed/truncated tool-call args JSON → `return null`, call dispatched with **null args** | VIOLATION | HIGH | emit a tool-result error for that call id instead of an arg-less dispatch | +| 7 | `Sessions/Pipelines/ToolCallMetaExtractor.cs:37-39` & `:41` | M3 | requested timeout below floor → default; above ceiling → silent `Math.Min` clamp (the literal prod scenario) | VIOLATION | HIGH | append `[timeout clamped 1200s→600s max]` / `[requested 10s below 60s floor]` to result | +| 8 | `Tools/ListWebhooksTool.cs:31` | standalone | schema-advertised `Filter` arg is **never read** — `ListRouteFiles()` ignores it; complete no-op | VIOLATION | MEDIUM | honor filter (`definition.Enabled`) or reject unknown values; echo applied filter | +| 9 | `Tools/WebFetchTool.cs:111` | M3 | `Format` ≠ `"text"` (e.g. `"markdown"`, typo) → silent raw-HTML fallback | VIOLATION | MEDIUM | validate `Format ∈ {raw,text}`; error/notice otherwise | +| 10 | `Tools/WebFetchTool.cs:212-217` → `:107,149` | M3 | body > 5 MB cap → truncated, summary shows count but **no "truncated" marker** | VIOLATION | MEDIUM | append `[content truncated at 5 MB — N bytes not fetched]` | +| 11 | `ToolAccessPolicy.cs:204-205` (non-interactive shell trust-zone) | (policy) | path token that `NormalizePathToken` returns null for → `continue` (unchecked); working-dir branch fails closed — inconsistent, allow-leaning | BORDERLINE | MEDIUM | fail closed on null-normalized token (`shell_unresolvable_path_token`) | +| 12 | `Protocol/ChatMessageConverter.cs:94-97` | M2 | persisted media file missing at request-build → `log + continue`; attachment vanishes from LLM message (log-only, model blind) | BORDERLINE | MEDIUM | insert `[attachment unavailable: ]` placeholder into contents | +| 13 | `Providers/SelfHosted/TextToolCallParser.cs:40` | M2 | text/XML tool-call params all coerced to trimmed **string**; arrays/objects/types lost (the **Qwen text-format path** — relevant: orchestrator was Qwen) | BORDERLINE | MEDIUM | try `JsonDocument.Parse` per value; keep structured form; surface unreconcilable values | +| 14 | `OpenAiCompatibleChatClient.cs:749-752` | M2 | streamed tool call missing `function.name` → `?? string.Empty` → masquerades as "unknown tool", args lost | BORDERLINE | MEDIUM | treat nameless finished call as stream-assembly error (log + diagnostic) | +| 15 | `ToolAccessPolicy.cs:493-525` (`ResolveApprovalMode`) | (policy) | a future `/`-bearing matcher key would skip `McpServerDefaults` (latent, not currently reachable) | BORDERLINE | MEDIUM | guard: matcher keys must be first-party (no `/`) | +| 16 | `Tools/WebSearchTool.cs:40` / `Tools/FileReadTool.cs:84-85` | M3 | `MaxResults` clamp to 30 (documented); `StartLine`/`Limit` 0/neg treated as unspecified | BORDERLINE | LOW | optional `[capped at 30]` note; reject non-positive line numbers | +| 17 | `ToolAccessPolicy.cs:338-346` (safe-verb short-circuit) | (policy) | read-only-verb auto-ALLOW with no audit line (cf. `LogApprovalNearMisses` which does log) | BORDERLINE | LOW | emit info/audit log on safe-verb auto-grant | + +**Separate defect class (not silent-fallback, flagged for awareness):** +`ToolCallMeta.cs:87` — `je.GetInt32()` inside a `when` guard **throws** (uncaught) on a +non-integral/overflow JSON number (`_timeout_seconds: 12.5` or `1e12`). `ExtractFrom` has no +try/catch. Use `TryGetInt32`. + +--- + +## Reusable "loud" patterns already in-repo (reuse before adding) + +- **`ToolOutputSpill.Compose` (`ToolOutputSpill.cs:108-115`)** — gold standard model-facing + truncation notice: `[output truncated to X of Y; saved to — read a slice…]`. +- **`SessionToolExecutionPipeline.AppendModelInputHandoffWarning` (`:924`)** — in-band, + model-facing notice driven by a requested-vs-actual gap. Exact mechanism for M3 overrides. +- **`ToolArgumentHelper.TryGetValueFlexible` / `NormalizeKey` (`:17-68`)** — case/punct-insensitive + matcher; reuse inside `ToolCallMeta.ExtractFrom` to fix M1 meta-key drops *and* detect near-misses. +- **`ApprovalNearMiss` facility (`ApprovalPatternMatching.cs:192-311`, logged via + `ToolApprovalActor.LogApprovalNearMisses:163-185`)** — already classifies + "expected-match-but-missed" with a `Describe()`. A `ToolArgNearMiss` modeled on it gives the + "supplied X, recognized form is Y, here's why" surface for M1. Read-only diagnostics — safe. +- **`RedirectToken` throw-on-unknown-enum (`IToolApprovalMatcher.cs:447`)** — correct alternative + to `_ => default` on a security-relevant enum map. +- **`ShellTool` cwd handling (`:118-133`)** and **`RouteToBackgroundJobAsync` trust-context guard + (`:734-736`, `throw … "trust context cannot be defaulted"`)** — constitution-aligned exemplars. + +--- + +## Proposed spec spine (for the OpenSpec change) + +1. **No-silent-discard invariant at the tool-arg seam.** Unknown keys → loud, recoverable + tool-result error with near-miss "did you mean" (reuse `ApprovalNearMiss` shape + flexible + matcher). Covers M1 at `NetclawToolGenerator.ParseArguments` + `ToolCallMeta.ExtractFrom`. +2. **Absent vs present-but-invalid.** Present-but-unparseable values surface, never coerce to + `0`/`false`/`null`-args. Covers M2. +3. **Surface every override to the agent.** Clamp/floor/format-fallback/truncation emit a + model-facing note (reuse `ToolOutputSpill`/`AppendModelInputHandoffWarning`). Covers M3. +4. **Policy decisions, separately.** #3 (allowlist authority), #11 (null token fail-closed), + #17 (audit auto-grants) need a security-owner decision, not just a notice. +5. **Ergonomic fix alongside the safety net:** accept obvious aliases (`TimeoutSeconds` → + `_timeout_seconds`) so correct intent just works; the validator is the backstop. + +**Highest single leverage:** items 1 + 2 (the M1 seam) — closes the original bug's entire +class in ~2 places. diff --git a/docs/runbooks/background-jobs.md b/docs/runbooks/background-jobs.md index 7d69a83f9..896228a5b 100644 --- a/docs/runbooks/background-jobs.md +++ b/docs/runbooks/background-jobs.md @@ -83,9 +83,9 @@ category as `shell_execute`). ## Configuration -Background jobs inherit the session's tool timeout ceiling -(`MaxToolTimeoutSeconds`, default 600s). The `_timeout_seconds` metadata field -on the tool call controls per-job timeout within that ceiling. +The `_timeout_seconds` metadata field on the tool call sets the per-job timeout +and is honored as requested; when omitted, the session's default tool timeout +(`SessionConfig.ToolExecutionTimeout`) applies. No separate configuration surface exists — background jobs use the same approval policy and audience ACL as regular shell execution. diff --git a/docs/spec/configuration.md b/docs/spec/configuration.md index f1f575fc3..71e284271 100644 --- a/docs/spec/configuration.md +++ b/docs/spec/configuration.md @@ -189,7 +189,6 @@ shape, confirm that strict-default fallback is active, or verify that { "Tools": { "ShellMode": "HostAllowed", - "ShellTimeoutSeconds": 60, "MaxOutputChars": 32000, "AudienceProfiles": { "Public": { @@ -230,7 +229,6 @@ shape, confirm that strict-default fallback is active, or verify that | Field | Type | Default | Description | |-------|------|---------|-------------| | `ShellMode` | string? | `null` | Optional shell mode override (`Off`, `SandboxOnly`, `HostAllowed`). Falls back to security posture defaults when omitted. | -| `ShellTimeoutSeconds` | int | `60` | Timeout for shell command execution. | | `MaxOutputChars` | int | `32000` | Maximum characters captured from tool output. | | `AudienceProfiles` | object | built-in defaults | Per-audience tool, MCP server, and filesystem scopes. Default tool grants are monotonic — `public` ⊆ `team` ⊆ `personal`. `public` gets read-only file tools only (`file_read`, `file_list`, `attach_file`) — no file-mutation and no outbound web tools; `team` adds the file-mutation, web (`web_search`/`web_fetch`), scheduling, and skill tools but not `shell_execute`, the webhook tools, or any MCP server; `personal` defaults to unrestricted tool/file access and all MCP servers. `public` and `team` keep session-scoped file access until the operator opts in. | @@ -536,7 +534,6 @@ export NETCLAW_Session__MaxToolIterationsPerTurn="60" "ToolExecutionTimeoutSeconds": 90 }, "Tools": { - "ShellTimeoutSeconds": 60, "MaxOutputChars": 32000 } } diff --git a/evals/run-evals.sh b/evals/run-evals.sh index 4d058686a..cf58ca887 100755 --- a/evals/run-evals.sh +++ b/evals/run-evals.sh @@ -1017,6 +1017,15 @@ assert_tool_file_list() { stdout_contains '\[tool:call\] file_list' } +assert_tool_timeout_arg_recovery() { + # Loud arg validation: if the model emits a near-miss timeout key + # (TimeoutSeconds, timeout_seconds), the rejection's did-you-mean must + # steer it to the canonical _timeout_seconds within the turn — the + # command actually running is the proof of recovery. + stdout_contains '\[tool:call\] shell_execute' \ + && stdout_contains 'netclaw-timeout-eval-ok' +} + # Category 5: Grounding & Alignment assert_grounding_no_hallucinate_version() { stdout_contains '\[tool:call\]' @@ -1477,6 +1486,10 @@ run_all() { run_case tool_file_list "file_list called" \ "What files are in my session directory?" + run_case tool_timeout_arg_recovery "long-timeout shell call lands on _timeout_seconds" \ + "Run 'echo netclaw-timeout-eval-ok' in the shell with a 5 minute timeout." \ + "Use the shell to run: echo netclaw-timeout-eval-ok — give it a 300 second timeout since it might be slow." + end_category # ── Category 5: Grounding & Alignment ── diff --git a/feeds/skills/.system/files/netclaw-operations/SKILL.md b/feeds/skills/.system/files/netclaw-operations/SKILL.md index c063f77af..9bfdc3bb2 100644 --- a/feeds/skills/.system/files/netclaw-operations/SKILL.md +++ b/feeds/skills/.system/files/netclaw-operations/SKILL.md @@ -3,7 +3,7 @@ name: netclaw-operations description: "REQUIRED when the user asks about scheduling, reminders, cron jobs, timers, background jobs, diagnostics, troubleshooting, MCP tools, daemon health, identity updates, or Netclaw capabilities and self-maintenance." metadata: author: netclaw - version: "2.11.2" + version: "2.12.1" --- # Netclaw Operations @@ -258,6 +258,14 @@ Rules: - Only `shell_execute` supports background mode. Other tools ignore `_background`. - `_timeout_seconds` alone does NOT trigger background execution. +- `_timeout_seconds` is honored as you set it (there is no ceiling or floor) — + set it to however long the work genuinely needs. When omitted, the default + tool timeout applies. +- **Long-running delegation calls** (e.g. `curl` to a local coding-agent or + model server that takes minutes to respond) should run as background jobs and + carry a `_timeout_seconds` large enough for the work. A synchronous call set + to a short timeout (or left at the default) will be killed mid-flight while + the remote server is still working. - The user must approve the command before it starts running in the background. - Maximum 5 concurrent background jobs; overflow queues FIFO. - Job definitions persist to `~/.netclaw/jobs/{id}.json`. @@ -279,6 +287,18 @@ results proactively when the job completes. Active background jobs appear in the `[active-background-jobs]` section of the session context on every turn. +## Tool argument validation + +Tool argument names are validated strictly — unrecognized keys reject the call +before execution with a `did you mean ''?` suggestion and the list +of valid argument names. Meta keys are exact-match: `_timeout_seconds` and +`_background` (a leading underscore, snake_case). `TimeoutSeconds`, +`timeout_seconds`, or `_timeoutSeconds` are rejected, never silently dropped. +Values must parse as their declared type: `_timeout_seconds: "1200ms"` or +`_background: "yes"` rejects the call instead of silently using defaults. When +a call is rejected this way the tool did NOT run — fix the argument and +re-issue once; do not retry the same shape. + ## Large tool output Tool output is bounded to a small inline budget diff --git a/openspec/changes/archive/2026-06-11-loud-tool-arg-validation/.openspec.yaml b/openspec/changes/archive/2026-06-11-loud-tool-arg-validation/.openspec.yaml new file mode 100644 index 000000000..e0c0898ff --- /dev/null +++ b/openspec/changes/archive/2026-06-11-loud-tool-arg-validation/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-06-11 diff --git a/openspec/changes/archive/2026-06-11-loud-tool-arg-validation/design.md b/openspec/changes/archive/2026-06-11-loud-tool-arg-validation/design.md new file mode 100644 index 000000000..4c602b024 --- /dev/null +++ b/openspec/changes/archive/2026-06-11-loud-tool-arg-validation/design.md @@ -0,0 +1,253 @@ +# Design: Loud Tool-Argument Validation + +## Context + +LLM-supplied tool arguments flow through three seams today: + +1. **Provider boundary** — `OpenAiCompatibleChatClient.TryDeserializeArguments` + parses the model's arguments JSON; on `JsonException` it returns null and the + call is dispatched with **null arguments**. +2. **Pipeline meta extraction** — `ToolCallMetaExtractor.Extract` → + `ToolCallMeta.ExtractFrom` pulls `_rationale` / `_timeout_seconds` / + `_background` by **exact key match** and silently drops malformed values + (`_ => null` / `_ => false`). `ComputeEffectiveTimeout` silently clamps to the + ceiling and silently ignores below-floor hints. +3. **Tool binding** — generated `ParseArguments` (per-tool, from + `NetclawToolGenerator`) reads only declared parameters via + `ToolArgumentHelper.Get*` (which match **flexibly**: exact first, then + case/punctuation-normalized via `NormalizeKey`). Unknown keys are never + inspected; present-but-unparseable values coerce to `0`/`0.0`/`false` through + `GetNullable* … ?? default`. + +Two channels already exist that this design reuses rather than duplicates: + +- **Exception → error result**: `SessionToolExecutionPipeline` catches any tool + exception and converts it to `resultText = "Error executing tool: {message}"` + (`:553-556`) — the generated `ParseArguments` already uses this for + missing-required (`throw new ArgumentException`). +- **Post-bounding notice append**: `AppendModelInputHandoffWarning` (`:571-574`) + appends model-facing notices to `resultText` *after* `ToolOutputSpill` + bounding, so notices can never be windowed away. + +Origin incident and full site inventory: `SILENT_FALLBACK_AUDIT.md` (repo root, +this branch); proposal.md for scope. + +## Goals / Non-Goals + +**Goals:** + +- No LLM-supplied argument is ever silently discarded, coerced, or overridden: + every such event either rejects the call with a self-describing, recoverable + error (before execution) or surfaces a notice in the tool result. +- Rejection errors are actionable in one model round-trip ("did you mean + `_timeout_seconds`?"). +- Zero behavior change for well-formed calls. +- No new config knobs, no new persisted types, no new actors or messages. + +**Non-Goals:** + +- No fuzzy/alias *acceptance* of argument keys (explicitly decided against — + see D2). +- No change to MCP tool argument handling (`mcp-schema-coercion` + server-side + validation remain authoritative). +- No policy/ACL changes (parked in proposal). +- No stuck-loop/no-progress detection (separate workstream). +- No change to `TextToolCallParser` type fidelity (separate design needed). + +## Decisions + +### D1: Central unknown-key validation in `DispatchingToolExecutor`, driven by the tool's schema + +Unknown-key checking runs once, centrally, in `DispatchingToolExecutor.ExecuteAsync` +before `tool.ExecuteAsync`, for **native tools only** (skip `McpToolAdapter`). +The recognized-key set is derived from the tool's existing `ParameterSchema` +(which the generator already augments with the meta keys), computed lazily once +per tool type and cached — no generator changes needed for the set itself. + +**Recognition MUST mirror actual consumption semantics**, not an idealized rule: + +- A supplied key is recognized iff it would actually be consumed downstream: + - **declared parameters**: exact match OR `NormalizeKey`-equivalent — because + `ToolArgumentHelper.TryGetValueFlexible` already binds flexibly today; + - **meta keys** (`_`-prefixed): **exact match only** — because + `ToolCallMeta.ExtractFrom` extracts exactly. +- Anything else → reject with a tool-result error, before execution. + +If recognition were stricter than binding (exact-only for declared params), the +Qwen text-parser path — which emits lowercased keys that flexible binding +accepts today — would start failing on working calls. If it were looser than +extraction (flexible for meta keys), `TimeoutSeconds` would be "recognized" but +never consumed — recreating the original bug behind the validator. + +*Alternatives considered:* (a) emit the check inside generated `ParseArguments` +— rejected: N generated copies of one rule, and the executor seam also covers +direct callers; (b) validate in `SessionToolExecutionPipeline` — rejected: +sub-agent and non-pipeline dispatch paths also funnel through +`DispatchingToolExecutor`, making it the true chokepoint. + +### D2: Suggestions only — fuzzy matching never accepts + +**Locked decision (user):** the system never acts on a guessed key. The +dividing line is *who resolves ambiguity*: + +- **Deterministic canonicalization** (existing `NormalizeKey` case/punctuation + folding for declared params) is retained — it is existing, deterministic + consumption behavior, not guessing, and removing it would break working + callers. +- **Guess-based matching** (edit distance, near-miss against meta keys) is used + **only to generate the suggestion text** in the rejection error: + `Unrecognized argument 'TimeoutSeconds'. Did you mean '_timeout_seconds'? + The tool was NOT executed.` The LLM resolves the ambiguity by re-issuing + explicitly. + +Suggestion generation: `NormalizeKey`-equality against meta keys first (catches +the entire `TimeoutSeconds`/`_timeoutSeconds`/`timeout_seconds` family), then +edit-distance ≤ 2 against all recognized names. Modeled on the +`ApprovalNearMiss` shape (`ApprovalPatternMatching`): classify, describe, +never alter the decision. The error also lists the tool's valid argument names +(bounded — native tools have ≤ ~6 params + 3 meta keys). + +### D3: Present-but-invalid values reject via strict helper variants + +`ToolArgumentHelper` gains strict variants (`GetIntStrict`, `GetDoubleStrict`, +`GetBoolStrict`, and nullable counterparts) that distinguish three states: +**absent** (→ documented default, unchanged), **parsed** (→ value), and +**present-but-invalid** (→ `throw ArgumentException("Parameter 'Limit' value +'abc' is not a valid integer.")`). `NetclawToolGenerator` emits the strict +variants in `ParseArguments`; the existing `ArgumentException` → pipeline +catch → error-result channel surfaces it. Two latent value bugs are fixed in +the same pass: `double d => (int)d` silent truncation (12.7 → 12) becomes +invalid-unless-integral, and `JsonElement.GetInt32()` on non-integral/overflow +numbers (currently an **uncaught throw**) becomes `TryGetInt32` → +present-but-invalid. + +The non-strict `GetNullable*` helpers remain for callers that legitimately +treat unparseable as absent (none known in generated code after this change; +audit flagged the `?? 0/0.0/false` arms specifically). + +### D4: Malformed meta values reject the call — computed in the pipeline layer, not persisted + +`ToolCallMeta.ExtractFrom` keeps its signature and the persisted `ToolCallMeta` +type is **unchanged** (it is persistence-owned; adding transient validation +state to it would leak pipeline concerns into the serialization contract). +Instead, `ToolCallMetaExtractor.Extract` (pipeline-side) returns validation +errors alongside the meta: a present-but-invalid `_timeout_seconds` or +`_background` value produces a tool-result error **before dispatch** — the +agent expressed execution semantics we cannot honor, so we do not run the call +on different semantics. Same rejection channel as D1/D3. + +### D5: Override notices via a `Notices` list on `ToolExecutionContext`, appended post-bounding + +`ToolExecutionContext` (already flowing through every seam — per the +constitution, reuse what is already at the call site) gains a +`List Notices`. Producers: + +- `SessionToolExecutionPipeline` / `ToolCallMetaExtractor.ComputeEffectiveTimeout`: + ceiling clamp → `[timeout clamped: requested 1200s, maximum 600s — use + _background:true for longer work]`; below-floor → `[timeout request 10s is + below the 60s tool default; 60s applied]`. +- `WebFetchTool`: response-byte cap reached → `[content truncated at 5 MB — N + bytes not fetched]`. + +Notices are appended to `resultText` at the existing +`AppendModelInputHandoffWarning` seam — after `ToolOutputSpill` bounding, so a +notice can never be spilled or windowed away. Notices are additive text on the +existing result string: no persistence change (results are already persisted as +strings). + +*Alternative considered:* return notices through tool return values — rejected: +changes the `string`-returning tool contract for every tool; the context object +already traverses the exact path needed. + +### D6: Provider-boundary malformed args JSON → sentinel argument, rejected pre-dispatch + +When `TryDeserializeArguments` fails, instead of dispatching null arguments, +the client attaches a single sentinel entry +`__netclaw_args_parse_error: ""`. The pipeline detects the sentinel before meta extraction and emits +a tool-result error for that call id without dispatching: `Tool call arguments +were not valid JSON: …. The tool was NOT executed.` + +*Alternatives considered:* (a) custom `AIContent` subtype — rejected: invasive +across message conversion and persistence for one error path; +(b) drop the call silently — violates the invariant being established; +(c) keep null-args dispatch + detect downstream — rejected: the raw payload +(needed for a useful error) is only available at the client. +The sentinel never collides with validation: it is checked and consumed before +D1 runs, and if it ever leaked it is not in any schema → rejected loudly anyway. +On persistence re-drive the sentinel round-trips as an ordinary argument and the +pipeline rejects it again pre-dispatch — deterministic on replay. + +### D7: In-tool fixes — `web_fetch` format, `list_webhooks` filter + +- `WebFetchTool`: `Format` validated against `{null, "raw", "text"}`; + anything else → `ArgumentException` (same channel as D3). The silent + `useTextMode = Format == "text"` fallback is removed. +- `ListWebhooksTool`: `Filter` honored — `"active"` (default) filters on + `definition.Enabled`, `"all"` returns everything, any other value rejects; + the applied filter is echoed in the result header. + +### D8: No escape hatches + +No config knob disables validation (a toggle would be a sanctioned silent +fallback). If a future tool legitimately accepts free-form keys, it must opt +out explicitly in source (e.g. an `[AllowUnknownArguments]` attribute on the +tool class) where it is visible to review — not at runtime. + +## Actor Boundaries and Persistence Implications + +- **No new actors, messages, or protocols.** All changes live inside the + session actor's existing tool-execution pipeline (`LlmSessionActor` → + `SessionToolExecutionPipeline` → `DispatchingToolExecutor` → tool classes), + which is already transport-agnostic. Sub-agent dispatch funnels through the + same `DispatchingToolExecutor` and inherits validation unchanged. +- **No persisted-type changes.** `ToolCallMeta` / `SerializableToolCall` are + untouched (D4 keeps validation state pipeline-side). Rejection errors and + notices are ordinary tool-result strings, persisted through the existing + `SerializableChatMessage` path. Legacy persisted tool calls deserialize and + re-drive exactly as before; a re-driven call carrying a bad key is rejected + deterministically (same input → same error), which is the correct replay + semantic. + +## Failure Modes and Recovery + +- **Validator rejects a key a model insists on** → error is recoverable and + self-describing (valid-key list + suggestion); the model corrects in one + round-trip. If a model loops on the same rejection, that is the (separate) + stuck-loop workstream's domain; the error text is deterministic, so loop + detection sees identical failures — the easy case. +- **False rejection of a working call pattern** (biggest risk — e.g. a text- + parser key shape we did not anticipate) → recognition mirrors binding + semantics exactly (D1), and the eval suite + a replay of representative + session logs gate the release. Recovery: revert; no data migration involved. +- **A tool with intentionally dynamic args breaks** → contingency is the + explicit source-level opt-out (D8); audit during implementation confirms no + current native tool needs it. +- **Notice text inflates context** → notices are single bounded lines, appended + at most once per producer per call. +- **Sentinel arg persisted mid-rollout, processed post-rollback** → an unknown + `__netclaw_args_parse_error` arg under old code is dropped by old binding + (the old silent behavior) — degraded but not corrupt. + +## Migration Plan + +1. Land helpers + generator change + validator behind nothing (no flag — the + change is the behavior). +2. Run `dotnet slopwatch analyze`, full test suite, eval suite (tool-definition + change → required per constitution), and the light smoke tapes. +3. Release on the beta channel; watch session logs for `Unrecognized argument` + / `not valid` error rates and any new tool-error loops. +4. Stable release after beta soak. +5. Rollback: revert the release tag; no persisted-schema or config migration in + either direction. + +## Open Questions + +1. Should the rejection error enumerate valid keys always, or only when no + near-miss suggestion is found? (Leaning: always — the list is small and + removes a second failure round-trip.) +2. `skill_manage` / `set_webhook` accept structured sub-objects — confirm during + implementation that their generated param surface is flat (expected) so the + top-level key diff is sufficient; nested-object validation is out of scope. +3. Exact wording of the `_background` steer in the clamp notice — coordinate + with the `netclaw-operations` skill update so the two use identical phrasing. diff --git a/openspec/changes/archive/2026-06-11-loud-tool-arg-validation/proposal.md b/openspec/changes/archive/2026-06-11-loud-tool-arg-validation/proposal.md new file mode 100644 index 000000000..7dfbc67f1 --- /dev/null +++ b/openspec/changes/archive/2026-06-11-loud-tool-arg-validation/proposal.md @@ -0,0 +1,139 @@ +# Proposal: Loud Tool-Argument Validation + +## Why + +Netclaw silently discards or degrades LLM-supplied tool arguments: unknown keys are +dropped without signal, present-but-unparseable values coerce to defaults, and +requested values (timeouts) are clamped or ignored with no notice in the tool result. +In production session `D0AC6CKBK5K_1781115410_840529`, the agent passed +`"TimeoutSeconds":"1200"` (instead of the recognized `_timeout_seconds`), the key was +silently dropped, the shell timeout fell back to a 90s default, and the agent's call +to its code delegate was killed mid-flight — the agent formed a false belief ("I set a +generous timeout") that fed a multi-hour stuck loop. A follow-up audit +(`SILENT_FALLBACK_AUDIT.md`) found 17 sites sharing three mechanisms. This violates +the constitution's no-silent-fallbacks rule and the secure-by-default posture of +PRD-001 / PRD-002: the config surface already enforces strict validation +(`additionalProperties: false` + `ConfigSchemaDoctorCheck`), but the tool-call +argument surface — the agent's highest-frequency input path — has no equivalent. + +## What Changes + +- **Unknown-key rejection (M1).** A tool call carrying an argument key that matches + neither the tool's declared parameters nor the meta keys (`_rationale`, + `_timeout_seconds`, `_background`) is rejected with a recoverable tool-result error + before execution. The error names the unrecognized key and, when a near-miss is + detected, includes a "did you mean ``?" suggestion. **Fuzzy matching is + used ONLY to generate the suggestion text — never to accept a near-miss key.** + (Decided: no alias acceptance; system-side intent-guessing on a surface carrying + timeout/background/path semantics has unacceptable blast radius. The LLM resolves + the ambiguity explicitly by re-issuing.) +- **Present-but-invalid value rejection (M2).** A value that is present but + unparseable for its declared type (e.g. `"abc"` for an int parameter, + `_timeout_seconds: "1200ms"`, `_background: "yes"`) returns a tool-result error + naming the parameter, the supplied value, and the expected type — instead of + silently coercing to `0`/`false`/null. Absent optional parameters keep their + documented defaults (no intent expressed → no error). +- **Malformed tool-call JSON surfaces (M2, provider boundary).** A tool call whose + arguments JSON fails to deserialize produces a tool-result error for that call id + instead of dispatching the call with null arguments. +- **Override notices (M3).** Every silent override of an agent-expressed value emits + a model-facing notice appended to the tool result, reusing the existing + `ToolOutputSpill.Compose` / `AppendModelInputHandoffWarning` notice patterns: + - timeout hint clamped to `MaxToolTimeoutSeconds` ceiling → `[timeout clamped from + 1200s to 600s maximum; use _background:true for longer work]` + - timeout hint below the tool default floor → noted, not silently ignored + - `web_fetch` `Format` value outside `{raw, text}` → error (not silent raw fallback) + - `web_fetch` 5 MB response cap reached → truncation marker in the summary +- **Phantom argument fix.** `list_webhooks`' schema-advertised `Filter` parameter is + currently never read (complete no-op); it will be honored (filter on + `definition.Enabled`) with the applied filter echoed in the result. +- **Skill guidance.** `netclaw-operations` system skill updated: long-running + delegation calls (e.g. HTTP calls to a local coding-agent server) must use + `_background: true` rather than a synchronous shell call under the timeout ceiling. + +Not breaking for well-formed callers: tool calls using declared parameters and valid +values behave identically. Calls that previously "succeeded" by silently dropping +arguments will now error — that is the intended behavior change. + +## Capabilities + +### New Capabilities + +- `tool-arg-validation`: validation contract for LLM-supplied tool arguments at the + dispatch seam — unknown-key rejection with suggestion-only near-miss matching, + present-but-invalid value rejection, absent-vs-invalid distinction, malformed + args-JSON handling at the provider boundary, and the model-facing override-notice + mechanism. + +### Modified Capabilities + +- `tool-call-metadata`: the "Per-call timeout hint" requirement currently specifies + *silent* clamping to the ceiling and *silent* ignoring of below-floor hints + (scenarios "Timeout hint exceeds ceiling", "Timeout hint below tool default + ignored"). Both change: the effective value still clamps/floors, but the override + is surfaced in the tool result. Malformed meta values (`_timeout_seconds`, + `_background`) change from silent drop to a tool-result error. +- `netclaw-tools`: `web_fetch` gains explicit `Format` validation and a truncation + marker at the response-byte cap; `list_webhooks` gains honored `Filter` semantics. + +## Impact + +- **Affected code:** + - `Netclaw.Tools.Generators/NetclawToolGenerator.cs` (generated `ParseArguments` — + unknown-key diff + invalid-value errors; this is the seam covering all ~20 native + tools) + - `Netclaw.Tools.Abstractions/ToolCallMeta.cs`, `ToolArgumentHelper.cs` (meta + extraction, absent-vs-invalid distinction; also fix the latent uncaught-throw on + non-integral JSON numbers via `TryGetInt32`) + - `Netclaw.Actors/Sessions/Pipelines/ToolCallMetaExtractor.cs`, + `SessionToolExecutionPipeline.cs` (clamp/floor notices on the result path) + - `Netclaw.Providers/SelfHosted/OpenAiCompatibleChatClient.cs` + (`TryDeserializeArguments` null-args dispatch) + - `Netclaw.Actors/Tools/WebFetchTool.cs`, `ListWebhooksTool.cs` + - `feeds/skills/.system/files/netclaw-operations/SKILL.md` (+ version bump, per the + System Skills Sync Rule) +- **Reused constructs (no new parallel mechanisms):** near-miss suggestion modeled on + `ApprovalPatternMatching`'s `ApprovalNearMiss` shape; notices via the existing + result-append patterns; key normalization via `ToolArgumentHelper.NormalizeKey` + (for suggestion generation only). +- **MCP tools:** unchanged — MCP servers validate their own schemas and reject + observably through `McpToolAdapter`'s existing error surface (`mcp-schema-coercion` + remains authoritative). +- **Tests/evals:** tool-definition behavior changes → eval suite run required per the + constitution's Eval Suite rule; new unit coverage for the validation seam. + +### Security and Operational Impact + +- **Security:** net positive — closes a class the constitution flags as + privilege-escalation-adjacent (silently altered execution semantics). No policy or + ACL decision changes in this change. Rejection happens *before* execution, so no + partial side effects. The validator is fail-closed: ambiguity → error, never guess. +- **Operational:** a transient rise in tool-call errors is expected immediately after + deployment as resident models learn canonical keys from the error messages; errors + are self-describing and recoverable in one round-trip, so no operator action is + required. Override notices add small, bounded text to tool results. + +### In Scope (MVP) vs Out of Scope + +**In scope:** the four mechanisms above (M1, M2, M3, skill guidance) plus the +`list_webhooks` phantom-arg fix. + +**Out of scope — parked as open questions for a security owner** (from +`SILENT_FALLBACK_AUDIT.md`, policy layer): + +1. Audience allowlist non-authority: `ToolAudienceProfileResolver.IsProfileManagedTool` + silently exempts non-managed tools (memory tools, `search_tools`, `load_tool`, + `spawn_agent`, `check_background_job`) from `Allowlist` profiles — needs a + product decision (govern vs document). +2. Non-interactive shell trust-zone enforcement skips unnormalizable path tokens + (`continue`) while the working-directory branch fails closed — inconsistency + should be resolved fail-closed. +3. Safe-verb auto-allow short-circuit emits no audit line — observability decision. + +Also out of scope: stuck-loop / no-progress detection for agent sessions (separate +investigation, same originating incident); text-format tool-call parser type fidelity +(`TextToolCallParser` string flattening — borderline, needs its own design). + +**Traceability:** PRD-001 (MVP tool surface), PRD-002 (gateway security envelope / +fail-closed posture); constitution "No silent fallbacks" quality bar; origin incident +documented in memorizer memory `e9a72b27-72d9-4e98-aad7-4d970ce52ecf`. diff --git a/openspec/changes/archive/2026-06-11-loud-tool-arg-validation/specs/netclaw-tools/spec.md b/openspec/changes/archive/2026-06-11-loud-tool-arg-validation/specs/netclaw-tools/spec.md new file mode 100644 index 000000000..8ecdc3979 --- /dev/null +++ b/openspec/changes/archive/2026-06-11-loud-tool-arg-validation/specs/netclaw-tools/spec.md @@ -0,0 +1,72 @@ +# netclaw-tools — Delta Spec + +## ADDED Requirements + +### Requirement: Web fetch format is validated + +The `web_fetch` tool SHALL validate the `Format` argument against the supported +set (absent, `"raw"`, `"text"`). Any other value SHALL reject the call with a +tool-result error naming the supplied value and the supported set. The tool +SHALL NOT silently fall back to raw mode for an unsupported format value. + +#### Scenario: Unsupported format value rejects + +- **GIVEN** a `web_fetch` call with `"Format": "markdown"` +- **WHEN** arguments are validated +- **THEN** the call is rejected with an error naming `"markdown"` and the + supported values `raw` and `text` +- **AND** no HTTP request is made + +#### Scenario: Supported formats behave unchanged + +- **GIVEN** a `web_fetch` call with `"Format": "text"` (or `Format` absent) +- **WHEN** the fetch executes +- **THEN** behavior is identical to current behavior + +### Requirement: Web fetch response-cap truncation is surfaced + +The `web_fetch` result SHALL include a notice stating the content was +truncated at the cap whenever a fetched response body reaches the +response-byte cap. +The captured byte count alone SHALL NOT be the only signal. + +#### Scenario: Body larger than the cap carries a truncation notice + +- **GIVEN** a URL whose response body exceeds the 5 MB response cap +- **WHEN** `web_fetch` returns its summary +- **THEN** the result includes a notice that content was truncated at 5 MB + +#### Scenario: Body under the cap carries no truncation notice + +- **GIVEN** a URL whose response body is under the response cap +- **WHEN** `web_fetch` returns its summary +- **THEN** no truncation notice is present + +### Requirement: Webhook listing honors its filter argument + +The `list_webhooks` tool SHALL honor its schema-advertised `Filter` argument: +`"active"` (the default) SHALL return only enabled webhooks, `"all"` SHALL +return every webhook, and any other value SHALL reject the call naming the +supported values. The applied filter SHALL be echoed in the result. + +#### Scenario: Active filter excludes disabled webhooks + +- **GIVEN** two registered webhooks, one enabled and one disabled +- **AND** a `list_webhooks` call with `"Filter": "active"` (or `Filter` absent) +- **WHEN** the tool executes +- **THEN** only the enabled webhook is listed +- **AND** the result states the `active` filter was applied + +#### Scenario: All filter includes disabled webhooks + +- **GIVEN** two registered webhooks, one enabled and one disabled +- **AND** a `list_webhooks` call with `"Filter": "all"` +- **WHEN** the tool executes +- **THEN** both webhooks are listed with their enabled state +- **AND** the result states the `all` filter was applied + +#### Scenario: Unknown filter value rejects + +- **GIVEN** a `list_webhooks` call with `"Filter": "enabled"` +- **WHEN** arguments are validated +- **THEN** the call is rejected naming the supported values `active` and `all` diff --git a/openspec/changes/archive/2026-06-11-loud-tool-arg-validation/specs/tool-arg-validation/spec.md b/openspec/changes/archive/2026-06-11-loud-tool-arg-validation/specs/tool-arg-validation/spec.md new file mode 100644 index 000000000..40139a1c5 --- /dev/null +++ b/openspec/changes/archive/2026-06-11-loud-tool-arg-validation/specs/tool-arg-validation/spec.md @@ -0,0 +1,135 @@ +# tool-arg-validation — Delta Spec + +## ADDED Requirements + +### Requirement: Unknown argument keys reject the call before execution + +For native (first-party) tools, the dispatcher SHALL validate every supplied +argument key against the tool's recognized-key set before execution. A supplied +key is recognized if and only if it would be consumed downstream: + +- a declared tool parameter, matched exactly or by deterministic key + normalization (case/punctuation folding, mirroring existing flexible binding); +- a meta key (`_rationale`, `_timeout_seconds`, `_background`), matched + **exactly only**. + +A call carrying one or more unrecognized keys SHALL be rejected with a +tool-result error and the tool SHALL NOT execute. The error SHALL name each +unrecognized key, state that the tool was not executed, and list the tool's +valid argument names. MCP tools are exempt (server-side schema validation is +authoritative). + +#### Scenario: Near-miss meta key rejected with suggestion + +- **GIVEN** a `shell_execute` call with `"TimeoutSeconds": "1200"` +- **WHEN** the dispatcher validates argument keys +- **THEN** the call is rejected without executing the command +- **AND** the tool result contains `Unrecognized argument 'TimeoutSeconds'`, + a `did you mean '_timeout_seconds'` suggestion, and the valid argument names + +#### Scenario: Case-variant declared parameter still accepted + +- **GIVEN** a `shell_execute` call with `"command": "ls"` (lowercase) +- **WHEN** the dispatcher validates argument keys +- **THEN** the key is recognized via deterministic normalization +- **AND** the tool executes exactly as it does today + +#### Scenario: Exact meta key accepted + +- **GIVEN** a tool call with `"_timeout_seconds": 300` +- **WHEN** the dispatcher validates argument keys +- **THEN** the key is recognized and extraction consumes it + +#### Scenario: Wholly unknown key rejected without suggestion + +- **GIVEN** a `file_read` call with `"Banana": true` and a valid `Path` +- **WHEN** the dispatcher validates argument keys +- **THEN** the call is rejected naming `Banana` with no near-miss suggestion +- **AND** the valid argument names for `file_read` are listed + +#### Scenario: MCP tool exempt from native key validation + +- **GIVEN** a tool call targeting an MCP server tool with an extra key +- **WHEN** the dispatcher processes the call +- **THEN** native key validation is skipped +- **AND** the MCP server's own schema validation result is returned observably + +### Requirement: Fuzzy matching generates suggestions only — never acceptance + +Near-miss matching SHALL be used solely to generate "did you mean" suggestion +text inside rejection errors (normalization equivalence against meta keys, +edit distance against recognized names). The system SHALL NOT bind, alias, or +otherwise act on a guessed key. Ambiguity SHALL always be resolved by the LLM +re-issuing the call explicitly. + +#### Scenario: Near-miss key is never silently bound + +- **GIVEN** a tool call with `"timeout_seconds": 300` (missing the `_` prefix) +- **WHEN** the dispatcher validates argument keys +- **THEN** the call is rejected with a `did you mean '_timeout_seconds'` + suggestion +- **AND** no timeout override is applied from the near-miss key + +### Requirement: Present-but-invalid argument values reject the call + +For native tools, the system SHALL reject a call whose argument key is +recognized but whose value cannot be parsed as the declared type, with a +tool-result error naming the parameter, the supplied value, and the expected +type. The tool SHALL +NOT execute. An absent optional parameter SHALL continue to use its documented +default (absence expresses no intent; invalidity does). Numeric coercion SHALL +NOT silently truncate: a non-integral value supplied for an integer parameter +is invalid. + +#### Scenario: Unparseable integer rejects instead of coercing to zero + +- **GIVEN** a `file_read` call with `"Limit": "abc"` +- **WHEN** arguments are bound +- **THEN** the call is rejected with an error naming `Limit`, the value + `"abc"`, and the expected type integer +- **AND** the file is not read + +#### Scenario: Non-integral number for integer parameter is invalid + +- **GIVEN** a tool call supplying `12.7` for an integer parameter +- **WHEN** arguments are bound +- **THEN** the call is rejected (no silent truncation to 12) + +#### Scenario: Absent optional parameter keeps its default + +- **GIVEN** a `file_read` call that omits `Limit` +- **WHEN** arguments are bound +- **THEN** the documented default applies and no error is raised + +### Requirement: Malformed tool-call arguments JSON rejects before dispatch + +The pipeline SHALL produce a tool-result error for a tool call whose arguments +JSON the provider boundary fails to deserialize, stating the arguments were +not valid JSON; the tool SHALL NOT be dispatched with null or empty arguments. The error SHALL include the parse failure detail so the +model can correct its emission. + +#### Scenario: Truncated arguments JSON surfaces as an error result + +- **GIVEN** a streamed tool call whose accumulated arguments JSON is truncated + and fails to parse +- **WHEN** the pipeline processes the call +- **THEN** a tool-result error for that call id states the arguments were not + valid JSON and the tool was not executed +- **AND** no tool receives a null-argument invocation + +### Requirement: Overridden argument values are surfaced to the model + +The system SHALL append a model-facing notice to a call's tool result whenever +it honors the call but applies a value different from the one the LLM +requested (clamping, flooring, capping), describing the requested value, the +applied value, and the reason. Notices SHALL be appended after output +bounding so they cannot be truncated away. Log-only signaling SHALL NOT +satisfy this requirement: the notice MUST appear in the tool result the model +reads. + +#### Scenario: Notice survives output bounding + +- **GIVEN** a tool call whose result exceeds the inline output budget +- **AND** an override notice applies to the call +- **WHEN** the result is bounded and spilled +- **THEN** the notice is present in the inline result returned to the model diff --git a/openspec/changes/archive/2026-06-11-loud-tool-arg-validation/specs/tool-call-metadata/spec.md b/openspec/changes/archive/2026-06-11-loud-tool-arg-validation/specs/tool-call-metadata/spec.md new file mode 100644 index 000000000..f3a422129 --- /dev/null +++ b/openspec/changes/archive/2026-06-11-loud-tool-arg-validation/specs/tool-call-metadata/spec.md @@ -0,0 +1,93 @@ +# tool-call-metadata — Delta Spec + +## MODIFIED Requirements + +### Requirement: Per-call timeout hint + +The `_timeout_seconds` field SHALL allow the LLM to request a per-call timeout +override. The value SHALL be clamped to a configurable ceiling +(`ToolConfig.MaxToolTimeoutSeconds`, default 600). Values below the tool's +default timeout SHALL NOT lower the timeout (the default applies). The pipeline +SHALL use the effective value when creating the per-call +`CancellationTokenSource`. Whenever the effective value differs from the +requested value (ceiling clamp or below-floor request), the pipeline SHALL +append a model-facing notice to the tool result stating the requested value, +the applied value, and — for ceiling clamps — steering the model to +`_background: true` for longer work. Silent clamping or silent ignoring of the +hint SHALL NOT occur. + +#### Scenario: Timeout hint applied within ceiling + +- **GIVEN** `MaxToolTimeoutSeconds` is 600 +- **AND** the LLM requests `_timeout_seconds: 300` on a shell_execute call +- **WHEN** the pipeline creates the cancellation token +- **THEN** the timeout is set to 300 seconds +- **AND** no override notice is appended (requested value was honored) + +#### Scenario: Timeout hint exceeds ceiling + +- **GIVEN** `MaxToolTimeoutSeconds` is 600 +- **AND** the LLM requests `_timeout_seconds: 1200` +- **WHEN** the pipeline creates the cancellation token +- **THEN** the timeout is clamped to 600 seconds +- **AND** the tool result includes a notice stating 1200s was requested, 600s + was applied, and `_background: true` is available for longer work + +#### Scenario: Timeout hint below tool default surfaces a notice + +- **GIVEN** `ShellTimeoutSeconds` is 60 (shell tool default) +- **AND** the LLM requests `_timeout_seconds: 10` +- **WHEN** the pipeline creates the cancellation token +- **THEN** the timeout remains at 60 seconds (the tool default) +- **AND** the tool result includes a notice stating 10s was requested and the + 60s tool default was applied + +#### Scenario: No timeout hint uses default + +- **GIVEN** the LLM does not provide `_timeout_seconds` +- **WHEN** the pipeline creates the cancellation token +- **THEN** the existing default timeout applies (60s for shell, 90s for + general tool execution) +- **AND** no override notice is appended (no intent was expressed) + +## ADDED Requirements + +### Requirement: Malformed meta values reject the call + +The pipeline SHALL reject a tool call carrying a meta key whose value cannot +be parsed as its declared type (`_timeout_seconds` not a positive integer; +`_background` not a boolean) with a tool-result error before dispatch, naming +the meta key, the supplied value, and the expected type. The tool SHALL NOT execute +with default semantics in place of the expressed intent. Validation state SHALL +be computed pipeline-side; the persisted `ToolCallMeta` type SHALL remain +unchanged. + +#### Scenario: Unparseable timeout value rejects instead of silently defaulting + +- **GIVEN** a tool call with `"_timeout_seconds": "1200ms"` +- **WHEN** the pipeline extracts meta fields +- **THEN** the call is rejected with an error naming `_timeout_seconds`, the + value `"1200ms"`, and the expected type positive integer +- **AND** the tool does not execute under the default timeout + +#### Scenario: Non-boolean background value rejects + +- **GIVEN** a tool call with `"_background": "yes"` +- **WHEN** the pipeline extracts meta fields +- **THEN** the call is rejected with an error naming `_background` and the + expected type boolean +- **AND** the tool does not execute synchronously in place of the request + +#### Scenario: Non-integral JSON number for timeout is handled, not thrown + +- **GIVEN** a tool call with `"_timeout_seconds": 12.5` +- **WHEN** the pipeline extracts meta fields +- **THEN** extraction does not throw an uncaught exception +- **AND** the call is rejected as present-but-invalid + +#### Scenario: Legacy persisted tool call re-drives deterministically + +- **GIVEN** a persisted tool call carrying a malformed meta value is re-driven + after recovery +- **WHEN** the pipeline extracts meta fields +- **THEN** the same rejection error is produced (deterministic on replay) diff --git a/openspec/changes/archive/2026-06-11-loud-tool-arg-validation/tasks.md b/openspec/changes/archive/2026-06-11-loud-tool-arg-validation/tasks.md new file mode 100644 index 000000000..9f59fdc7f --- /dev/null +++ b/openspec/changes/archive/2026-06-11-loud-tool-arg-validation/tasks.md @@ -0,0 +1,120 @@ +# Tasks: loud-tool-arg-validation + +## 1. Value-parsing helpers (foundation — no behavior change yet) + +- [x] 1.1 Add strict variants to `ToolArgumentHelper` (`GetIntStrict`, + `GetDoubleStrict`, `GetBoolStrict` + nullable counterparts) that + distinguish absent / parsed / present-but-invalid; present-but-invalid + throws `ArgumentException` naming parameter, supplied value, and expected + type. Non-integral numeric for integer parameter is invalid (no `(int)d` + truncation); replace `JsonElement.GetInt32()` with `TryGetInt32` so + overflow/non-integral never throws uncaught. +- [x] 1.2 Unit tests for the strict helpers: absent → null/default, valid + parses (int/long/string-number/JsonElement), invalid string, non-integral + double, overflow JSON number, bool variants (`"yes"`, `1`, `"true"`). + +## 2. Generator: bind with strict helpers + +- [x] 2.1 Update `NetclawToolGenerator.ParseArguments` emission to call the + strict variants for integer/number/boolean parameters (required, + nullable, and optional-with-default arms) so present-but-invalid throws + instead of coercing to `0`/`0.0`/`false`. +- [x] 2.2 Snapshot/golden tests for the generator output covering each + parameter arm; verify a representative generated tool + (e.g. `file_read` `Limit: "abc"`) surfaces + `Error executing tool: Parameter 'Limit' value 'abc'…` through the + pipeline catch. + +## 3. Unknown-key validation in the dispatcher + +- [x] 3.1 Compute and cache the recognized-key set per native tool in + `DispatchingToolExecutor` (or `NetclawToolBase`): schema property names + from `ParameterSchema` (includes meta keys); recognition = exact OR + `NormalizeKey`-equal for declared params, exact-only for `_`-prefixed + meta keys. Skip `McpToolAdapter`. +- [x] 3.2 Implement rejection: unrecognized key(s) → return tool-result error + (do not execute) naming each key, stating the tool was NOT executed, and + listing valid argument names. +- [x] 3.3 Implement suggestion generation (suggestion text ONLY — never + acceptance): `NormalizeKey`-equality against meta keys first, then edit + distance ≤ 2 against recognized names; model the near-miss + classification on `ApprovalPatternMatching`'s `ApprovalNearMiss` shape. +- [x] 3.4 Unit tests: `TimeoutSeconds` → rejected with `_timeout_seconds` + suggestion; `timeout_seconds` → rejected with suggestion (never bound); + lowercase `command` → accepted (flexible binding preserved); exact + `_timeout_seconds` → accepted; wholly unknown key → rejected without + suggestion; MCP tool with extra key → not validated natively. +- [x] 3.5 Audit all native tools for intentionally free-form argument surfaces; + if any exists, add the explicit source-level opt-out + (`[AllowUnknownArguments]`) and a test proving it is honored — otherwise + record "none needed" in the PR description. + +## 4. Meta-value validation and override notices (pipeline) + +- [x] 4.1 Extend `ToolCallMetaExtractor.Extract` (pipeline-side; persisted + `ToolCallMeta` type unchanged) to report present-but-invalid + `_timeout_seconds` / `_background` values; pipeline rejects the call + pre-dispatch with an error naming key, value, expected type. +- [x] 4.2 Change `ComputeEffectiveTimeout` to report when the effective value + differs from the requested value (ceiling clamp, below-floor); plumb as + a notice, not a silent return. +- [x] 4.3 Add `Notices` accumulation to `ToolExecutionContext` and append + notices to `resultText` at the existing `AppendModelInputHandoffWarning` + seam (post-bounding, so notices cannot be spilled away). Clamp notice + text steers to `_background: true` for longer work. +- [x] 4.4 Unit tests: 1200s request with 600s ceiling → executes at 600s AND + result contains the clamp notice with `_background` steer; 10s request + with 60s floor → executes at 60s with notice; honored 300s request → no + notice; `_timeout_seconds: "1200ms"` → rejected pre-dispatch; + `_background: "yes"` → rejected; `_timeout_seconds: 12.5` → rejected + without uncaught throw; notice survives an over-budget result that + spills. + +## 5. Provider boundary: malformed arguments JSON + +- [x] 5.1 In `OpenAiCompatibleChatClient`, on `TryDeserializeArguments` + failure attach the `__netclaw_args_parse_error` sentinel (exception + message + first 200 chars of raw payload) instead of returning null + arguments. +- [x] 5.2 In `SessionToolExecutionPipeline`, detect the sentinel before meta + extraction and emit a tool-result error for that call id without + dispatching ("arguments were not valid JSON… The tool was NOT + executed."). +- [x] 5.3 Tests: truncated args JSON → error result for the call id, no tool + invocation; sentinel round-trips persistence and re-drives to the same + rejection deterministically. + +## 6. In-tool fixes + +- [x] 6.1 `WebFetchTool`: validate `Format ∈ {absent, "raw", "text"}`; reject + anything else (no silent raw fallback). Detect response-byte-cap hit in + `ReadBytesWithLimitAsync` and add the truncation notice to the summary. +- [x] 6.2 `ListWebhooksTool`: honor `Filter` — `"active"` (default) filters on + `definition.Enabled`, `"all"` returns everything, other values reject; + echo the applied filter in the result. +- [x] 6.3 Tests: unsupported format rejects with no HTTP request; >5 MB body + carries truncation notice, under-cap body does not; active/all/unknown + filter scenarios. + +## 7. Skill and documentation sync + +- [x] 7.1 Update `feeds/skills/.system/files/netclaw-operations/SKILL.md`: + long-running delegation calls must use `_background: true` (phrasing + identical to the clamp-notice steer); bump `metadata.version`. +- [x] 7.2 Verify no `Netclaw.Configuration` `*Config` property changed (no + schema sync needed) — confirm in PR description. + +## 8. Quality gates and verification + +- [x] 8.1 `dotnet slopwatch analyze` — no new violations. +- [x] 8.2 `./scripts/Add-FileHeaders.ps1 -Verify` — headers on any new files. +- [x] 8.3 Run the eval suite (`./evals/run-evals.sh`) — tool definitions + changed; add/adjust an eval case asserting the model recovers from an + unknown-key rejection in one round-trip. +- [x] 8.4 Replay regression: drive the recorded arg shapes from session + `D0AC6CKBK5K_1781115410_840529` (`"TimeoutSeconds":"1200"` on + shell_execute) against the validator and assert the rejection + + suggestion; confirm representative text-parser (lowercase-key) calls + from session logs still bind. +- [x] 8.5 Update `SILENT_FALLBACK_AUDIT.md` rows fixed by this change with + their resolution status. diff --git a/openspec/specs/netclaw-tools/spec.md b/openspec/specs/netclaw-tools/spec.md index 977fd7be7..1932f06bd 100644 --- a/openspec/specs/netclaw-tools/spec.md +++ b/openspec/specs/netclaw-tools/spec.md @@ -237,6 +237,74 @@ present in the media catalog. - **WHEN** `web_fetch` saves the response - **THEN** it chooses `.pdf` from the media catalog +### Requirement: Web fetch format is validated + +The `web_fetch` tool SHALL validate the `Format` argument against the supported +set (absent, `"raw"`, `"text"`). Any other value SHALL reject the call with a +tool-result error naming the supplied value and the supported set. The tool +SHALL NOT silently fall back to raw mode for an unsupported format value. + +#### Scenario: Unsupported format value rejects + +- **GIVEN** a `web_fetch` call with `"Format": "markdown"` +- **WHEN** arguments are validated +- **THEN** the call is rejected with an error naming `"markdown"` and the + supported values `raw` and `text` +- **AND** no HTTP request is made + +#### Scenario: Supported formats behave unchanged + +- **GIVEN** a `web_fetch` call with `"Format": "text"` (or `Format` absent) +- **WHEN** the fetch executes +- **THEN** behavior is identical to current behavior + +### Requirement: Web fetch response-cap truncation is surfaced + +The `web_fetch` result SHALL include a notice stating the content was +truncated at the cap whenever a fetched response body reaches the +response-byte cap. The captured byte count alone SHALL NOT be the only signal. + +#### Scenario: Body larger than the cap carries a truncation notice + +- **GIVEN** a URL whose response body exceeds the 5 MB response cap +- **WHEN** `web_fetch` returns its summary +- **THEN** the result includes a notice that content was truncated at 5 MB + +#### Scenario: Body under the cap carries no truncation notice + +- **GIVEN** a URL whose response body is under the response cap +- **WHEN** `web_fetch` returns its summary +- **THEN** no truncation notice is present + +### Requirement: Webhook listing honors its filter argument + +The `list_webhooks` tool SHALL honor its schema-advertised `Filter` argument: +`"active"` (the default) SHALL return only enabled webhooks, `"all"` SHALL +return every webhook, and any other value SHALL reject the call naming the +supported values. The applied filter SHALL be echoed in the result. + +#### Scenario: Active filter excludes disabled webhooks + +- **GIVEN** two registered webhooks, one enabled and one disabled +- **AND** a `list_webhooks` call with `"Filter": "active"` (or `Filter` absent) +- **WHEN** the tool executes +- **THEN** only the enabled webhook is listed +- **AND** the result states the `active` filter was applied + +#### Scenario: All filter includes disabled webhooks + +- **GIVEN** two registered webhooks, one enabled and one disabled +- **AND** a `list_webhooks` call with `"Filter": "all"` +- **WHEN** the tool executes +- **THEN** both webhooks are listed with their enabled state +- **AND** the result states the `all` filter was applied + +#### Scenario: Unknown filter value rejects + +- **GIVEN** a `list_webhooks` call with `"Filter": "enabled"` +- **WHEN** arguments are validated +- **THEN** the call is rejected naming the supported values `active` and `all` + ### Requirement: File read tool The system SHALL provide a `file_read` first-party tool that authorizes the diff --git a/openspec/specs/tool-arg-validation/spec.md b/openspec/specs/tool-arg-validation/spec.md new file mode 100644 index 000000000..974f63bcb --- /dev/null +++ b/openspec/specs/tool-arg-validation/spec.md @@ -0,0 +1,145 @@ +# tool-arg-validation Specification + +## Purpose + +Defines the validation contract for LLM-supplied tool arguments at the +dispatch seam. No argument the model expressed intent through is ever silently +discarded, coerced, or overridden: unknown keys and invalid values reject the +call with a recoverable, self-describing error before execution, and every +honored-but-overridden value is surfaced in the tool result. Originated from a +production incident where a near-miss timeout key (`TimeoutSeconds` instead of +`_timeout_seconds`) was silently dropped, the shell timeout silently fell back +to a default, and the agent's false belief fed a stuck loop. + +## Requirements + +### Requirement: Unknown argument keys reject the call before execution + +For native (first-party) tools, the dispatcher SHALL validate every supplied +argument key against the tool's recognized-key set before execution. A supplied +key is recognized if and only if it would be consumed downstream: + +- a declared tool parameter, matched exactly or by deterministic key + normalization (case/punctuation folding, mirroring existing flexible binding); +- a meta key (`_rationale`, `_timeout_seconds`, `_background`), matched + **exactly only**. + +A call carrying one or more unrecognized keys SHALL be rejected with a +tool-result error and the tool SHALL NOT execute. The error SHALL name each +unrecognized key, state that the tool was not executed, and list the tool's +valid argument names. MCP tools are exempt (server-side schema validation is +authoritative). + +#### Scenario: Near-miss meta key rejected with suggestion + +- **GIVEN** a `shell_execute` call with `"TimeoutSeconds": "1200"` +- **WHEN** the dispatcher validates argument keys +- **THEN** the call is rejected without executing the command +- **AND** the tool result contains `Unrecognized argument 'TimeoutSeconds'`, + a `did you mean '_timeout_seconds'` suggestion, and the valid argument names + +#### Scenario: Case-variant declared parameter still accepted + +- **GIVEN** a `shell_execute` call with `"command": "ls"` (lowercase) +- **WHEN** the dispatcher validates argument keys +- **THEN** the key is recognized via deterministic normalization +- **AND** the tool executes exactly as it does today + +#### Scenario: Exact meta key accepted + +- **GIVEN** a tool call with `"_timeout_seconds": 300` +- **WHEN** the dispatcher validates argument keys +- **THEN** the key is recognized and extraction consumes it + +#### Scenario: Wholly unknown key rejected without suggestion + +- **GIVEN** a `file_read` call with `"Banana": true` and a valid `Path` +- **WHEN** the dispatcher validates argument keys +- **THEN** the call is rejected naming `Banana` with no near-miss suggestion +- **AND** the valid argument names for `file_read` are listed + +#### Scenario: MCP tool exempt from native key validation + +- **GIVEN** a tool call targeting an MCP server tool with an extra key +- **WHEN** the dispatcher processes the call +- **THEN** native key validation is skipped +- **AND** the MCP server's own schema validation result is returned observably + +### Requirement: Fuzzy matching generates suggestions only — never acceptance + +Near-miss matching SHALL be used solely to generate "did you mean" suggestion +text inside rejection errors (normalization equivalence against meta keys, +edit distance against recognized names). The system SHALL NOT bind, alias, or +otherwise act on a guessed key. Ambiguity SHALL always be resolved by the LLM +re-issuing the call explicitly. + +#### Scenario: Near-miss key is never silently bound + +- **GIVEN** a tool call with `"timeout_seconds": 300` (missing the `_` prefix) +- **WHEN** the dispatcher validates argument keys +- **THEN** the call is rejected with a `did you mean '_timeout_seconds'` + suggestion +- **AND** no timeout override is applied from the near-miss key + +### Requirement: Present-but-invalid argument values reject the call + +For native tools, the system SHALL reject a call whose argument key is +recognized but whose value cannot be parsed as the declared type, with a +tool-result error naming the parameter, the supplied value, and the expected +type. The tool SHALL NOT execute. An absent optional parameter SHALL continue +to use its documented default (absence expresses no intent; invalidity does). +Numeric coercion SHALL NOT silently truncate: a non-integral value supplied +for an integer parameter is invalid. + +#### Scenario: Unparseable integer rejects instead of coercing to zero + +- **GIVEN** a `file_read` call with `"Limit": "abc"` +- **WHEN** arguments are bound +- **THEN** the call is rejected with an error naming `Limit`, the value + `"abc"`, and the expected type integer +- **AND** the file is not read + +#### Scenario: Non-integral number for integer parameter is invalid + +- **GIVEN** a tool call supplying `12.7` for an integer parameter +- **WHEN** arguments are bound +- **THEN** the call is rejected (no silent truncation to 12) + +#### Scenario: Absent optional parameter keeps its default + +- **GIVEN** a `file_read` call that omits `Limit` +- **WHEN** arguments are bound +- **THEN** the documented default applies and no error is raised + +### Requirement: Malformed tool-call arguments JSON rejects before dispatch + +The pipeline SHALL produce a tool-result error for a tool call whose arguments +JSON the provider boundary fails to deserialize, stating the arguments were +not valid JSON; the tool SHALL NOT be dispatched with null or empty arguments. +The error SHALL include the parse failure detail so the model can correct its +emission. + +#### Scenario: Truncated arguments JSON surfaces as an error result + +- **GIVEN** a streamed tool call whose accumulated arguments JSON is truncated + and fails to parse +- **WHEN** the pipeline processes the call +- **THEN** a tool-result error for that call id states the arguments were not + valid JSON and the tool was not executed +- **AND** no tool receives a null-argument invocation + +### Requirement: Overridden argument values are surfaced to the model + +The system SHALL append a model-facing notice to a call's tool result whenever +it honors the call but applies a value different from the one the LLM +requested (clamping, flooring, capping), describing the requested value, the +applied value, and the reason. Notices SHALL be appended after output bounding +so they cannot be truncated away. Log-only signaling SHALL NOT satisfy this +requirement: the notice MUST appear in the tool result the model reads. + +#### Scenario: Notice survives output bounding + +- **GIVEN** a tool call whose result exceeds the inline output budget +- **AND** an override notice applies to the call +- **WHEN** the result is bounded and spilled +- **THEN** the notice is present in the inline result returned to the model diff --git a/openspec/specs/tool-call-metadata/spec.md b/openspec/specs/tool-call-metadata/spec.md index 5dd826f61..876a13ec2 100644 --- a/openspec/specs/tool-call-metadata/spec.md +++ b/openspec/specs/tool-call-metadata/spec.md @@ -7,10 +7,10 @@ every tool's JSON schema and populated by the LLM as part of normal tool calling. The metadata captures the model's intent (`_rationale`), a per-call synchronous timeout hint (`_timeout_seconds`), and an explicit background execution signal (`_background`). The tool execution pipeline extracts these -fields before dispatch so tool implementations never receive them, clamps the -timeout hint to a configurable ceiling, persists the metadata on the tool call -for journal replay, and enriches audit entries with the rationale and timeout -hint. This capability defines the signaling and metadata mechanism only; +fields before dispatch so tool implementations never receive them, honors the +timeout hint as requested (rejecting only invalid values), persists the +metadata on the tool call for journal replay, and enriches audit entries with +the rationale and timeout hint. This capability defines the signaling and metadata mechanism only; actual background job execution is consumed by a follow-on change. ## Requirements @@ -88,39 +88,39 @@ call in one sentence — what are you trying to accomplish and why?" ### Requirement: Per-call timeout hint -The `_timeout_seconds` field SHALL allow the LLM to request a per-call timeout -override. The value SHALL be clamped to a configurable ceiling -(`ToolConfig.MaxToolTimeoutSeconds`, default 600). Values below the tool's -default timeout SHALL be ignored (the default applies). The pipeline SHALL use -the clamped value when creating the per-call `CancellationTokenSource`. +The `_timeout_seconds` field SHALL allow the LLM to set a per-call timeout. A +positive value SHALL be honored exactly — it SHALL NOT be clamped to a ceiling +nor floored to the tool default; the agent owns this judgement. When no hint is +provided, the inherited per-call default (`SessionConfig.ToolExecutionTimeout`) +SHALL apply. The pipeline SHALL use this value when creating the per-call +`CancellationTokenSource`, and the same value SHALL govern the background-job +path when `_background` is set. (A present-but-invalid value — non-positive or +unparseable — is rejected before dispatch; see "Malformed meta values".) -#### Scenario: Timeout hint applied within ceiling +#### Scenario: Timeout hint is honored exactly -- **GIVEN** `MaxToolTimeoutSeconds` is 600 -- **AND** the LLM requests `_timeout_seconds: 300` on a shell_execute call +- **GIVEN** the LLM requests `_timeout_seconds: 1200` on a shell_execute call - **WHEN** the pipeline creates the cancellation token -- **THEN** the timeout is set to 300 seconds +- **THEN** the timeout is set to 1200 seconds +- **AND** nothing is appended to the tool result -#### Scenario: Timeout hint exceeds ceiling +#### Scenario: A small timeout hint is honored, not floored -- **GIVEN** `MaxToolTimeoutSeconds` is 600 -- **AND** the LLM requests `_timeout_seconds: 1200` -- **WHEN** the pipeline creates the cancellation token -- **THEN** the timeout is clamped to 600 seconds - -#### Scenario: Timeout hint below tool default ignored - -- **GIVEN** `ShellTimeoutSeconds` is 60 (shell tool default) - **AND** the LLM requests `_timeout_seconds: 10` - **WHEN** the pipeline creates the cancellation token -- **THEN** the timeout remains at 60 seconds (the tool default) +- **THEN** the timeout is set to 10 seconds (no floor is imposed) -#### Scenario: No timeout hint uses default +#### Scenario: No timeout hint uses the inherited default - **GIVEN** the LLM does not provide `_timeout_seconds` - **WHEN** the pipeline creates the cancellation token -- **THEN** the existing default timeout applies (60s for shell, 90s for - general tool execution) +- **THEN** the `SessionConfig.ToolExecutionTimeout` default applies + +#### Scenario: Background path honors the same hint + +- **GIVEN** the LLM sets `_background: true` and `_timeout_seconds: 1800` +- **WHEN** the call is routed to a background job +- **THEN** the job's timeout is 1800 seconds (not clamped) ### Requirement: Background execution signal @@ -151,10 +151,50 @@ execution); this spec defines only the signaling mechanism. - **GIVEN** background job execution is not yet available - **AND** the LLM requests `_background: true` - **WHEN** the pipeline processes the tool call -- **THEN** the tool executes synchronously with the requested (clamped) timeout +- **THEN** the tool executes synchronously with the requested timeout - **AND** a log message indicates background execution was requested but is not yet available +### Requirement: Malformed meta values reject the call + +The pipeline SHALL reject a tool call carrying a meta key whose value cannot +be parsed as its declared type (`_timeout_seconds` not a positive integer; +`_background` not a boolean) with a tool-result error before dispatch, naming +the meta key, the supplied value, and the expected type. The tool SHALL NOT +execute with default semantics in place of the expressed intent. Validation +state SHALL be computed pipeline-side; the persisted `ToolCallMeta` type SHALL +remain unchanged. + +#### Scenario: Unparseable timeout value rejects instead of silently defaulting + +- **GIVEN** a tool call with `"_timeout_seconds": "1200ms"` +- **WHEN** the pipeline extracts meta fields +- **THEN** the call is rejected with an error naming `_timeout_seconds`, the + value `"1200ms"`, and the expected type positive integer +- **AND** the tool does not execute under the default timeout + +#### Scenario: Non-boolean background value rejects + +- **GIVEN** a tool call with `"_background": "yes"` +- **WHEN** the pipeline extracts meta fields +- **THEN** the call is rejected with an error naming `_background` and the + expected type boolean +- **AND** the tool does not execute synchronously in place of the request + +#### Scenario: Non-integral JSON number for timeout is handled, not thrown + +- **GIVEN** a tool call with `"_timeout_seconds": 12.5` +- **WHEN** the pipeline extracts meta fields +- **THEN** extraction does not throw an uncaught exception +- **AND** the call is rejected as present-but-invalid + +#### Scenario: Legacy persisted tool call re-drives deterministically + +- **GIVEN** a persisted tool call carrying a malformed meta value is re-driven + after recovery +- **WHEN** the pipeline extracts meta fields +- **THEN** the same rejection error is produced (deterministic on replay) + ### Requirement: ToolCallMeta persistence Extracted `ToolCallMeta` SHALL be persisted on `SerializableToolCall` as an @@ -194,31 +234,4 @@ allow/deny, duration, approval decision). - **GIVEN** the LLM provides a timeout hint on a tool call - **WHEN** the audit entry is logged -- **THEN** the entry includes the `TimeoutHintSeconds` value (pre-clamp, as - requested by the LLM) - -### Requirement: Configuration for timeout ceiling - -`ToolConfig` SHALL include `MaxToolTimeoutSeconds` (int, default 600). It SHALL -be validated in the config schema (`netclaw-config.v1.schema.json`) with -`minimum: 1`. The schema SHALL include a default value for migration-friendly -`netclaw doctor --fix` support. - -#### Scenario: Config properties parsed - -- **GIVEN** the config file includes `tools.MaxToolTimeoutSeconds: 900` -- **WHEN** the config is loaded -- **THEN** `ToolConfig.MaxToolTimeoutSeconds` is 900 - -#### Scenario: Config defaults applied - -- **GIVEN** the config file does not include timeout properties -- **WHEN** the config is loaded -- **THEN** `ToolConfig.MaxToolTimeoutSeconds` is 600 - -#### Scenario: Config schema validates new properties - -- **GIVEN** `netclaw-config.v1.schema.json` includes the new properties -- **WHEN** `netclaw doctor` validates a config with these properties -- **THEN** validation passes -- **AND** `SchemaFixResolver` can insert defaults for missing properties +- **THEN** the entry includes the `TimeoutHintSeconds` value as requested by the LLM diff --git a/samples/Netclaw.Demo.AppHost.IntegrationTests/Netclaw.Demo.AppHost.IntegrationTests.csproj b/samples/Netclaw.Demo.AppHost.IntegrationTests/Netclaw.Demo.AppHost.IntegrationTests.csproj index b85c5bed3..29bbcc73e 100644 --- a/samples/Netclaw.Demo.AppHost.IntegrationTests/Netclaw.Demo.AppHost.IntegrationTests.csproj +++ b/samples/Netclaw.Demo.AppHost.IntegrationTests/Netclaw.Demo.AppHost.IntegrationTests.csproj @@ -13,6 +13,9 @@ + + diff --git a/samples/Netclaw.Demo.AppHost/Netclaw.Demo.AppHost.csproj b/samples/Netclaw.Demo.AppHost/Netclaw.Demo.AppHost.csproj index b8bfca2a1..774919731 100644 --- a/samples/Netclaw.Demo.AppHost/Netclaw.Demo.AppHost.csproj +++ b/samples/Netclaw.Demo.AppHost/Netclaw.Demo.AppHost.csproj @@ -14,6 +14,9 @@ + + diff --git a/src/Netclaw.Actors.Tests/Sessions/Pipelines/BackgroundRoutingTests.cs b/src/Netclaw.Actors.Tests/Sessions/Pipelines/BackgroundRoutingTests.cs index d3db5ee01..cda6c5e1d 100644 --- a/src/Netclaw.Actors.Tests/Sessions/Pipelines/BackgroundRoutingTests.cs +++ b/src/Netclaw.Actors.Tests/Sessions/Pipelines/BackgroundRoutingTests.cs @@ -117,6 +117,52 @@ await SessionToolExecutionPipeline.ExecuteToolsAsync( Assert.Equal("long running build", received.Rationale); } + [Fact] + public async Task ExplicitBackground_HonorsRequestedTimeout() + { + // The agent's requested timeout is honored on the background path too — + // it is not clamped to a ceiling (the agent owns that judgement). + var executor = new EchoExecutor(); + var probe = CreateTestProbe("pipeline-probe-bg-timeout"); + var jobManagerProbe = CreateTestProbe("job-manager-bg-timeout"); + var fakeJobManager = Sys.ActorOf(Props.Create(() => new FakeJobManager(jobManagerProbe.Ref))); + + var toolCalls = new List + { + new("call-bg-timeout", "shell_execute", new Dictionary + { + ["command"] = "sleep 1200", + ["_background"] = true, + ["_timeout_seconds"] = 1800, + ["_rationale"] = "long job" + }) + }; + + await SessionToolExecutionPipeline.ExecuteToolsAsync( + executor, toolCalls, + new SessionId("test/background-timeout"), + source: TestMessageSource(), + auditLogger: null, + timeProvider: TimeProvider.System, + sessionDir: Path.GetTempPath(), + maxInlineToolResultChars: 4096, + timeout: TimeSpan.FromSeconds(5), + self: probe.Ref, + emitSubAgentOutput: _ => { }, + spawnChildActor: static (_, _, _) => Task.FromResult(new object()), + backgroundJobManager: fakeJobManager, + ct: TestContext.Current.CancellationToken); + + await probe.ExpectMsgAsync( + TimeSpan.FromSeconds(5), + cancellationToken: TestContext.Current.CancellationToken); + + var received = await jobManagerProbe.ExpectMsgAsync( + TimeSpan.FromSeconds(3), + cancellationToken: TestContext.Current.CancellationToken); + Assert.Equal(1800, received.TimeoutSeconds); + } + [Fact] public async Task ExplicitBackground_PreservesWorkingDirectory() { diff --git a/src/Netclaw.Actors.Tests/Sessions/Pipelines/MetaValidationAndNoticeTests.cs b/src/Netclaw.Actors.Tests/Sessions/Pipelines/MetaValidationAndNoticeTests.cs new file mode 100644 index 000000000..eae6e0172 --- /dev/null +++ b/src/Netclaw.Actors.Tests/Sessions/Pipelines/MetaValidationAndNoticeTests.cs @@ -0,0 +1,278 @@ +// ----------------------------------------------------------------------- +// +// Copyright (C) 2026 - 2026 Petabridge, LLC +// +// ----------------------------------------------------------------------- +using System.Text.Json; +using Akka.Hosting; +using Akka.Hosting.TestKit; +using Microsoft.Extensions.AI; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Hosting; +using Netclaw.Actors.Channels; +using Netclaw.Actors.Protocol; +using Netclaw.Actors.Sessions; +using Netclaw.Actors.Sessions.Pipelines; +using Netclaw.Actors.Tools; +using Netclaw.Configuration; +using Netclaw.Security; +using Netclaw.Tools; +using Xunit; + +namespace Netclaw.Actors.Tests.Sessions.Pipelines; + +/// +/// Meta-value validation rejects present-but-invalid meta values pre-dispatch, +/// and timeout overrides (clamp/floor) surface as model-facing notices in the +/// tool result — the silent clamp manufactured a false belief in production +/// (tool-call-metadata spec deltas). +/// +public sealed class MetaValidationAndNoticeTests(ITestOutputHelper output) : TestKit(output: output) +{ + protected override void ConfigureServices(HostBuilderContext context, IServiceCollection services) + { + } + + protected override void ConfigureAkka(AkkaConfigurationBuilder builder, IServiceProvider provider) + { + } + + private static TurnContext InteractiveTurnContext(SessionId sessionId) => new() + { + SessionId = sessionId, + TurnId = new TurnId("test-turn"), + Audience = TrustAudience.Personal, + Boundary = TrustBoundary.Personal, + ChannelType = ChannelType.SignalR, + RequesterSenderId = new SenderId("local-user"), + RequesterPrincipal = PrincipalClassification.Operator, + Provenance = new SourceProvenance(TransportAuthenticity.Verified, PayloadTaint.Trusted), + SupportsInteractiveApproval = true + }; + + private async Task RunPipelineAsync( + IToolExecutor executor, + Dictionary args, + TimeSpan? timeout = null) + { + var probe = CreateTestProbe(); + var sessionId = new SessionId("D1/meta-validation-test"); + var toolCalls = new List + { + new("call-1", "shell_execute", args) + }; + + var pipelineTask = SessionToolExecutionPipeline.ExecuteToolsAsync( + executor, + toolCalls, + sessionId, + source: null, + auditLogger: null, + timeProvider: TimeProvider.System, + sessionDir: Path.GetTempPath(), + maxInlineToolResultChars: 4096, + timeout: timeout ?? TimeSpan.FromSeconds(60), + self: probe.Ref, + emitSubAgentOutput: _ => { }, + spawnChildActor: static (_, _, _) => Task.FromResult(new object()), + turnContext: InteractiveTurnContext(sessionId), + ct: TestContext.Current.CancellationToken); + + var completed = await probe.ExpectMsgAsync( + TimeSpan.FromSeconds(5), + cancellationToken: TestContext.Current.CancellationToken); + await pipelineTask.WaitAsync(TimeSpan.FromSeconds(5), TestContext.Current.CancellationToken); + return completed; + } + + private sealed class EchoExecutor(string result = "ok") : IToolExecutor + { + public int Invocations; + public ToolExecutionContext? LastContext; + + public Task AuthorizeAsync(FunctionCallContent toolCall, ToolExecutionContext? context = null, CancellationToken ct = default) + => Task.CompletedTask; + + // Mirror the real executor's registry-free pre-dispatch validation + // (sentinel + meta values) so the pipeline rejects the same calls it + // would in production. The schema/unknown-key half needs a registry and + // is covered by ToolArgumentValidatorTests against the real executor. + public ToolArgumentRejection? ValidateToolCall(FunctionCallContent toolCall) + => DispatchingToolExecutor.ValidateArguments(toolCall.Arguments); + + public Task ExecuteAsync(FunctionCallContent toolCall, ToolExecutionContext? context = null, CancellationToken ct = default) + { + Invocations++; + LastContext = context; + return Task.FromResult(result); + } + } + + // ── Timeout hint is honored exactly (no clamp, no floor) ── + + [Fact] + public async Task High_timeout_request_honored_exactly() + { + // The agent's judgement governs: a large value is used as-is, not + // clamped to a ceiling, and nothing is appended to the result. + var executor = new EchoExecutor(); + var completed = await RunPipelineAsync(executor, new Dictionary + { + ["Command"] = "echo hi", + ["_timeout_seconds"] = 1200 + }); + + var content = completed.ToolResults[0].Content; + Assert.Equal(1, executor.Invocations); + Assert.Equal(1200, executor.LastContext?.RequestedTimeoutSeconds); + Assert.DoesNotContain("clamped", content); + Assert.DoesNotContain("[timeout", content); + Assert.Contains("ok", content); + } + + [Fact] + public async Task Low_timeout_request_honored_exactly() + { + // A value below the inherited default is used as-is — a shorter timeout + // is the agent's prerogative and is strictly safer; no floor is imposed. + var executor = new EchoExecutor(); + var completed = await RunPipelineAsync(executor, new Dictionary + { + ["Command"] = "echo hi", + ["_timeout_seconds"] = 10 + }); + + var content = completed.ToolResults[0].Content; + Assert.Equal(1, executor.Invocations); + Assert.Equal(10, executor.LastContext?.RequestedTimeoutSeconds); + Assert.DoesNotContain("[timeout", content); + } + + [Fact] + public async Task Integral_decimal_json_timeout_accepted_and_executes() + { + // {"_timeout_seconds": 300.0} — a common LLM emission — must be accepted + // (TryGetInt32 rejects "300.0"; the integral-double fallback rescues it), + // not rejected as invalid. + var executor = new EchoExecutor(); + var completed = await RunPipelineAsync(executor, new Dictionary + { + ["Command"] = "echo hi", + ["_timeout_seconds"] = JsonDocument.Parse("300.0").RootElement.Clone() + }); + + Assert.Equal(1, executor.Invocations); + Assert.Equal(300, executor.LastContext?.RequestedTimeoutSeconds); + Assert.DoesNotContain("NOT executed", completed.ToolResults[0].Content); + } + + [Fact] + public async Task Honored_request_executes_with_no_notice() + { + var executor = new EchoExecutor(); + var completed = await RunPipelineAsync(executor, new Dictionary + { + ["Command"] = "echo hi", + ["_timeout_seconds"] = 300 + }); + + var content = completed.ToolResults[0].Content; + Assert.Equal(1, executor.Invocations); + Assert.Equal(300, executor.LastContext?.RequestedTimeoutSeconds); + Assert.DoesNotContain("[timeout", content); + Assert.Equal("ok", content); + } + + // ── Malformed meta values reject pre-dispatch ── + + [Fact] + public async Task Unparseable_timeout_value_rejects_without_dispatch() + { + var executor = new EchoExecutor(); + var completed = await RunPipelineAsync(executor, new Dictionary + { + ["Command"] = "echo hi", + ["_timeout_seconds"] = "1200ms" + }); + + var content = completed.ToolResults[0].Content; + Assert.Equal(0, executor.Invocations); + Assert.Contains("'_timeout_seconds'", content); + Assert.Contains("'1200ms'", content); + Assert.Contains("positive integer", content); + Assert.Contains("NOT executed", content); + } + + [Fact] + public async Task Non_boolean_background_value_rejects_without_dispatch() + { + var executor = new EchoExecutor(); + var completed = await RunPipelineAsync(executor, new Dictionary + { + ["Command"] = "echo hi", + ["_background"] = "yes" + }); + + var content = completed.ToolResults[0].Content; + Assert.Equal(0, executor.Invocations); + Assert.Contains("'_background'", content); + Assert.Contains("boolean", content); + Assert.Contains("NOT executed", content); + } + + [Fact] + public async Task Non_integral_json_timeout_rejects_without_uncaught_throw() + { + var executor = new EchoExecutor(); + var completed = await RunPipelineAsync(executor, new Dictionary + { + ["Command"] = "echo hi", + ["_timeout_seconds"] = JsonDocument.Parse("12.5").RootElement.Clone() + }); + + var content = completed.ToolResults[0].Content; + Assert.Equal(0, executor.Invocations); + Assert.Contains("'_timeout_seconds'", content); + Assert.Contains("12.5", content); + } + + [Fact] + public async Task Negative_timeout_rejects_without_dispatch() + { + var executor = new EchoExecutor(); + var completed = await RunPipelineAsync(executor, new Dictionary + { + ["Command"] = "echo hi", + ["_timeout_seconds"] = -5 + }); + + Assert.Equal(0, executor.Invocations); + Assert.Contains("NOT executed", completed.ToolResults[0].Content); + } + + // ── Provider-boundary args parse failure ── + + [Fact] + public async Task Args_parse_error_sentinel_rejects_without_dispatch_and_is_deterministic() + { + var executor = new EchoExecutor(); + var args = new Dictionary + { + [ToolCallArgumentErrors.ArgsParseErrorKey] = + "Expected end of object. Raw arguments prefix: {\"Command\":\"ech" + }; + + var first = await RunPipelineAsync(executor, args); + // Same call re-driven (persistence recovery replays the same args) — + // the rejection must be deterministic. + var second = await RunPipelineAsync(executor, args); + + Assert.Equal(0, executor.Invocations); + var content = first.ToolResults[0].Content; + Assert.Contains("not valid JSON", content); + Assert.Contains("NOT executed", content); + Assert.Contains("Raw arguments prefix:", content); + Assert.Equal(content, second.ToolResults[0].Content); + } + +} diff --git a/src/Netclaw.Actors.Tests/Sessions/Pipelines/ToolCallMetaExtractorTests.cs b/src/Netclaw.Actors.Tests/Sessions/Pipelines/ToolCallMetaExtractorTests.cs index b9d4b9f80..ef0d1b343 100644 --- a/src/Netclaw.Actors.Tests/Sessions/Pipelines/ToolCallMetaExtractorTests.cs +++ b/src/Netclaw.Actors.Tests/Sessions/Pipelines/ToolCallMetaExtractorTests.cs @@ -115,52 +115,10 @@ public void Extract_TimeoutSeconds_ZeroNotTreatedAsMeta() Assert.Null(meta); } - // ── Timeout clamping tests ── - - [Fact] - public void ComputeEffectiveTimeout_WithinRange_UsesHint() - { - var result = ToolCallMetaExtractor.ComputeEffectiveTimeout( - 300, TimeSpan.FromSeconds(60), 600); - - Assert.Equal(TimeSpan.FromSeconds(300), result); - } - - [Fact] - public void ComputeEffectiveTimeout_AboveCeiling_ClampsToCeiling() - { - var result = ToolCallMetaExtractor.ComputeEffectiveTimeout( - 1200, TimeSpan.FromSeconds(60), 600); - - Assert.Equal(TimeSpan.FromSeconds(600), result); - } - - [Fact] - public void ComputeEffectiveTimeout_BelowFloor_UsesDefault() - { - var result = ToolCallMetaExtractor.ComputeEffectiveTimeout( - 10, TimeSpan.FromSeconds(60), 600); - - Assert.Equal(TimeSpan.FromSeconds(60), result); - } - - [Fact] - public void ComputeEffectiveTimeout_Absent_UsesDefault() - { - var result = ToolCallMetaExtractor.ComputeEffectiveTimeout( - null, TimeSpan.FromSeconds(90), 600); - - Assert.Equal(TimeSpan.FromSeconds(90), result); - } - - [Fact] - public void ComputeEffectiveTimeout_NegativeHint_UsesDefault() - { - var result = ToolCallMetaExtractor.ComputeEffectiveTimeout( - -5, TimeSpan.FromSeconds(60), 600); - - Assert.Equal(TimeSpan.FromSeconds(60), result); - } + // Timeout-hint application (honor-or-default; no clamp/floor) is exercised + // end-to-end in MetaValidationAndNoticeTests against the pipeline. ExtractFrom's + // own parsing of _timeout_seconds (positive int → hint; else null) is covered + // by the extraction tests above and ToolArgumentHelperStrictTests. // ── Background signaling tests ── diff --git a/src/Netclaw.Actors.Tests/Tools/GeneratedToolStrictBindingTests.cs b/src/Netclaw.Actors.Tests/Tools/GeneratedToolStrictBindingTests.cs new file mode 100644 index 000000000..92f29d929 --- /dev/null +++ b/src/Netclaw.Actors.Tests/Tools/GeneratedToolStrictBindingTests.cs @@ -0,0 +1,109 @@ +// ----------------------------------------------------------------------- +// +// Copyright (C) 2026 - 2026 Petabridge, LLC +// +// ----------------------------------------------------------------------- +using System.Text.Json; +using Netclaw.Actors.Tools; +using Netclaw.Configuration; +using Netclaw.Tools; +using Xunit; + +namespace Netclaw.Actors.Tests.Tools; + +/// +/// Generated ParseArguments must bind through the strict helpers: a +/// present-but-invalid value surfaces as a model-facing parse error (via +/// NetclawTool.TryParse) and the tool never executes — instead of the old +/// silent coercion to 0/0.0/false (tool-arg-validation spec). +/// +public class GeneratedToolStrictBindingTests +{ + private static FileReadTool NewFileReadTool() => new(new ToolConfig()); + + private static ToolExecutionContext PersonalContext() + => new("signalr/thread-1", Path.GetTempPath()) + { + Audience = TrustAudience.Personal, + Boundary = TrustBoundary.TrustedInstance, + ChannelType = "signalr" + }; + + [Fact] + public async Task Invalid_int_value_surfaces_parse_error_and_does_not_execute() + { + var tool = NewFileReadTool(); + var result = await tool.ExecuteAsync(new Dictionary + { + ["Path"] = "/tmp/does-not-matter.txt", + ["Limit"] = "abc" + }, TestContext.Current.CancellationToken); + + Assert.Contains("Error parsing arguments for tool 'file_read'", result); + Assert.Contains("'Limit'", result); + Assert.Contains("'abc'", result); + Assert.Contains("integer", result); + } + + [Fact] + public async Task Non_integral_json_number_surfaces_parse_error_not_truncation() + { + var tool = NewFileReadTool(); + var limit = JsonDocument.Parse("12.5").RootElement.Clone(); + var result = await tool.ExecuteAsync(new Dictionary + { + ["Path"] = "/tmp/does-not-matter.txt", + ["Limit"] = limit + }, TestContext.Current.CancellationToken); + + Assert.Contains("Error parsing arguments", result); + Assert.Contains("'Limit'", result); + } + + [Fact] + public async Task Absent_optional_int_keeps_default_and_executes() + { + var tool = NewFileReadTool(); + var path = Path.Combine(Path.GetTempPath(), $"strict-binding-{Guid.NewGuid():N}.txt"); + await File.WriteAllTextAsync(path, "line1\nline2\n", TestContext.Current.CancellationToken); + try + { + var result = await tool.ExecuteAsync(new Dictionary + { + ["Path"] = path + }, PersonalContext(), TestContext.Current.CancellationToken); + + Assert.DoesNotContain("Error parsing arguments", result); + Assert.Contains("line1", result); + } + finally + { + File.Delete(path); + } + } + + [Fact] + public async Task Valid_string_number_still_binds() + { + var tool = NewFileReadTool(); + var path = Path.Combine(Path.GetTempPath(), $"strict-binding-{Guid.NewGuid():N}.txt"); + await File.WriteAllTextAsync(path, "line1\nline2\nline3\n", TestContext.Current.CancellationToken); + try + { + var result = await tool.ExecuteAsync(new Dictionary + { + ["Path"] = path, + ["StartLine"] = "2", + ["Limit"] = 1 + }, PersonalContext(), TestContext.Current.CancellationToken); + + Assert.DoesNotContain("Error parsing arguments", result); + Assert.Contains("line2", result); + Assert.DoesNotContain("line3", result); + } + finally + { + File.Delete(path); + } + } +} diff --git a/src/Netclaw.Actors.Tests/Tools/ListWebhooksToolTests.cs b/src/Netclaw.Actors.Tests/Tools/ListWebhooksToolTests.cs new file mode 100644 index 000000000..e4d1b447a --- /dev/null +++ b/src/Netclaw.Actors.Tests/Tools/ListWebhooksToolTests.cs @@ -0,0 +1,94 @@ +// ----------------------------------------------------------------------- +// +// Copyright (C) 2026 - 2026 Petabridge, LLC +// +// ----------------------------------------------------------------------- +using Netclaw.Actors.Tools; +using Netclaw.Configuration; +using Netclaw.Tests.Utilities; +using Xunit; + +namespace Netclaw.Actors.Tests.Tools; + +/// +/// The schema-advertised Filter parameter was previously a complete no-op +/// (silently discarded). It must now be honored: 'active' (default) excludes +/// disabled routes, 'all' includes everything, anything else rejects +/// (netclaw-tools spec: webhook listing honors its filter argument). +/// +public sealed class ListWebhooksToolTests : IDisposable +{ + private readonly DisposableTempDir _dir = new(); + private readonly WebhookRouteStore _store; + private readonly ListWebhooksTool _tool; + + public ListWebhooksToolTests() + { + var paths = new NetclawPaths(_dir.Path); + paths.EnsureDirectoriesExist(); + _store = new WebhookRouteStore(paths); + _tool = new ListWebhooksTool(_store); + + _store.Save("enabled-route", new WebhookRouteConfig + { + Enabled = true, + Prompt = "Handle inbound delivery." + }); + _store.Save("disabled-route", new WebhookRouteConfig + { + Enabled = false, + Prompt = "Dormant route." + }); + } + + public void Dispose() => _dir.Dispose(); + + [Fact] + public async Task Active_filter_excludes_disabled_routes_and_echoes_filter() + { + var result = await _tool.ExecuteAsync( + ToolInput.Create("Filter", "active"), TestContext.Current.CancellationToken); + + Assert.Contains("enabled-route", result); + Assert.DoesNotContain("disabled-route", result); + Assert.Contains("filter: active", result); + Assert.Contains("1 of 2", result); + } + + [Fact] + public async Task Absent_filter_defaults_to_active() + { + var result = await _tool.ExecuteAsync( + new Dictionary(), TestContext.Current.CancellationToken); + + Assert.Contains("enabled-route", result); + Assert.DoesNotContain("disabled-route", result); + Assert.Contains("filter: active", result); + } + + [Fact] + public async Task All_filter_includes_disabled_routes_with_enabled_state() + { + var result = await _tool.ExecuteAsync( + ToolInput.Create("Filter", "all"), TestContext.Current.CancellationToken); + + Assert.Contains("enabled-route", result); + Assert.Contains("disabled-route", result); + Assert.Contains("Enabled: True", result); + Assert.Contains("Enabled: False", result); + Assert.Contains("filter: all", result); + Assert.Contains("2 of 2", result); + } + + [Fact] + public async Task Unknown_filter_value_rejects_naming_supported_values() + { + var result = await _tool.ExecuteAsync( + ToolInput.Create("Filter", "enabled"), TestContext.Current.CancellationToken); + + Assert.Contains("'Filter' value 'enabled' is not supported", result); + Assert.Contains("active, all", result); + Assert.Contains("NOT executed", result); + Assert.DoesNotContain("enabled-route", result); + } +} diff --git a/src/Netclaw.Actors.Tests/Tools/ShellToolTests.cs b/src/Netclaw.Actors.Tests/Tools/ShellToolTests.cs index 06f384156..dd82f443f 100644 --- a/src/Netclaw.Actors.Tests/Tools/ShellToolTests.cs +++ b/src/Netclaw.Actors.Tests/Tools/ShellToolTests.cs @@ -48,10 +48,15 @@ public async Task Execute_returns_nonzero_exit_code() [Fact] public async Task Timeout_kills_long_running_process() { - var tool = new ShellTool(new ToolConfig { ShellTimeoutSeconds = 1 }); + var tool = new ShellTool(new ToolConfig()); var args = ToolInput.Create("Command", "sleep 100"); + var context = new ToolExecutionContext("test/thread", Path.GetTempPath()) + { + Audience = TrustAudience.Personal, + RequestedTimeoutSeconds = 1 + }; - var result = await tool.ExecuteAsync(args, CancellationToken.None); + var result = await tool.ExecuteAsync(args, context, CancellationToken.None); Assert.Contains("timed out", result); } @@ -59,7 +64,7 @@ public async Task Timeout_kills_long_running_process() [Fact] public async Task Requested_timeout_overrides_default_timeout() { - var tool = new ShellTool(new ToolConfig { ShellTimeoutSeconds = 1 }); + var tool = new ShellTool(new ToolConfig()); var args = ToolInput.Create("Command", "sleep 1"); var context = new ToolExecutionContext("test/thread", Path.GetTempPath()) { @@ -82,7 +87,7 @@ public async Task Caller_cancellation_kills_child_process_tree_and_returns_grace // exception. On Unix the command also spawns a background child that // inherits stdout/stderr; if the tree kill regresses, that child keeps // the pipe write-ends open and the test never completes. - var tool = new ShellTool(new ToolConfig { ShellTimeoutSeconds = 100 }); + var tool = new ShellTool(new ToolConfig()); var command = OperatingSystem.IsWindows() ? "ping 127.0.0.1 -n 120 > nul" : "sleep 120 & wait"; diff --git a/src/Netclaw.Actors.Tests/Tools/ToolArgumentHelperStrictTests.cs b/src/Netclaw.Actors.Tests/Tools/ToolArgumentHelperStrictTests.cs new file mode 100644 index 000000000..2b3093d35 --- /dev/null +++ b/src/Netclaw.Actors.Tests/Tools/ToolArgumentHelperStrictTests.cs @@ -0,0 +1,230 @@ +// ----------------------------------------------------------------------- +// +// Copyright (C) 2026 - 2026 Petabridge, LLC +// +// ----------------------------------------------------------------------- +using System.Globalization; +using System.Text.Json; +using Netclaw.Tools; +using Xunit; + +namespace Netclaw.Actors.Tests.Tools; + +/// +/// Strict argument helpers must distinguish absent (→ null), parseable +/// (→ value), and present-but-invalid (→ ArgumentException) — the silent +/// coercion to 0/0.0/false that the non-strict helpers perform is the +/// defect class these exist to close (tool-arg-validation spec). +/// +public class ToolArgumentHelperStrictTests +{ + private static Dictionary Args(string key, object? value) + => new() { [key] = value }; + + private static JsonElement Json(string raw) + => JsonDocument.Parse(raw).RootElement.Clone(); + + // ── GetIntStrict: absent vs invalid ── + + [Fact] + public void IntStrict_absent_key_returns_null() + { + Assert.Null(ToolArgumentHelper.GetIntStrict(new Dictionary(), "Limit")); + Assert.Null(ToolArgumentHelper.GetIntStrict(null, "Limit")); + } + + [Fact] + public void IntStrict_json_null_value_treated_as_absent() + { + Assert.Null(ToolArgumentHelper.GetIntStrict(Args("Limit", Json("null")), "Limit")); + Assert.Null(ToolArgumentHelper.GetIntStrict(Args("Limit", null), "Limit")); + } + + [Theory] + [InlineData(42)] + [InlineData(0)] + [InlineData(-7)] + public void IntStrict_native_int_parses(int value) + { + Assert.Equal(value, ToolArgumentHelper.GetIntStrict(Args("Limit", value), "Limit")); + } + + [Fact] + public void IntStrict_long_in_range_parses() + { + Assert.Equal(500, ToolArgumentHelper.GetIntStrict(Args("Limit", 500L), "Limit")); + } + + [Fact] + public void IntStrict_long_out_of_range_throws() + { + var ex = Assert.Throws( + () => ToolArgumentHelper.GetIntStrict(Args("Limit", long.MaxValue), "Limit")); + Assert.Contains("Limit", ex.Message); + Assert.Contains("integer", ex.Message); + } + + [Fact] + public void IntStrict_integral_double_parses() + { + Assert.Equal(12, ToolArgumentHelper.GetIntStrict(Args("Limit", 12.0), "Limit")); + } + + [Fact] + public void IntStrict_non_integral_double_throws_no_silent_truncation() + { + var ex = Assert.Throws( + () => ToolArgumentHelper.GetIntStrict(Args("Limit", 12.7), "Limit")); + Assert.Contains("12.7", ex.Message); + } + + [Fact] + public void IntStrict_json_number_parses() + { + Assert.Equal(300, ToolArgumentHelper.GetIntStrict(Args("Limit", Json("300")), "Limit")); + } + + [Fact] + public void IntStrict_json_integral_with_decimal_point_accepted() + { + // Models commonly emit whole numbers as 300.0 — JsonElement.TryGetInt32 + // rejects that text, so it must fall through to the integral-double + // check rather than being rejected as invalid. + Assert.Equal(300, ToolArgumentHelper.GetIntStrict(Args("Limit", Json("300.0")), "Limit")); + Assert.Equal(12, ToolArgumentHelper.GetIntStrict(Args("Limit", Json("12.0")), "Limit")); + } + + [Fact] + public void IntStrict_json_fractional_still_rejected() + { + Assert.Throws( + () => ToolArgumentHelper.GetIntStrict(Args("Limit", Json("12.5")), "Limit")); + } + + [Fact] + public void IntStrict_json_non_integral_number_throws_not_uncaught() + { + // The non-strict path called JsonElement.GetInt32() which throws + // FormatException uncaught; strict must convert to ArgumentException. + var ex = Assert.Throws( + () => ToolArgumentHelper.GetIntStrict(Args("Limit", Json("12.5")), "Limit")); + Assert.Contains("Limit", ex.Message); + } + + [Fact] + public void IntStrict_json_overflow_number_throws_argument_exception() + { + var ex = Assert.Throws( + () => ToolArgumentHelper.GetIntStrict(Args("Limit", Json("99999999999")), "Limit")); + Assert.Contains("integer", ex.Message); + } + + [Fact] + public void IntStrict_numeric_string_parses() + { + Assert.Equal(1200, ToolArgumentHelper.GetIntStrict(Args("Limit", "1200"), "Limit")); + Assert.Equal(1200, ToolArgumentHelper.GetIntStrict(Args("Limit", Json("\"1200\"")), "Limit")); + } + + [Fact] + public void IntStrict_unparseable_string_throws_naming_param_value_type() + { + var ex = Assert.Throws( + () => ToolArgumentHelper.GetIntStrict(Args("Limit", "abc"), "Limit")); + Assert.Contains("'Limit'", ex.Message); + Assert.Contains("'abc'", ex.Message); + Assert.Contains("integer", ex.Message); + } + + [Fact] + public void IntStrict_flexible_key_matching_preserved() + { + // Existing deterministic canonicalization (case/punctuation) must keep + // working — strictness applies to values, not key matching. + Assert.Equal(5, ToolArgumentHelper.GetIntStrict(Args("limit", 5), "Limit")); + Assert.Equal(5, ToolArgumentHelper.GetIntStrict(Args("start_line", 5), "StartLine")); + } + + // ── GetDoubleStrict ── + + [Fact] + public void DoubleStrict_absent_returns_null_valid_parses_invalid_throws() + { + Assert.Null(ToolArgumentHelper.GetDoubleStrict(new Dictionary(), "Scale")); + Assert.Equal(2.5, ToolArgumentHelper.GetDoubleStrict(Args("Scale", 2.5), "Scale")); + Assert.Equal(2.5, ToolArgumentHelper.GetDoubleStrict(Args("Scale", Json("2.5")), "Scale")); + Assert.Equal(2.5, ToolArgumentHelper.GetDoubleStrict(Args("Scale", "2.5"), "Scale")); + Assert.Equal(3.0, ToolArgumentHelper.GetDoubleStrict(Args("Scale", 3), "Scale")); + + var ex = Assert.Throws( + () => ToolArgumentHelper.GetDoubleStrict(Args("Scale", "fast"), "Scale")); + Assert.Contains("'Scale'", ex.Message); + Assert.Contains("number", ex.Message); + } + + // ── GetBoolStrict ── + + [Fact] + public void BoolStrict_absent_returns_null() + { + Assert.Null(ToolArgumentHelper.GetBoolStrict(new Dictionary(), "Recursive")); + } + + [Fact] + public void BoolStrict_valid_forms_parse() + { + Assert.True(ToolArgumentHelper.GetBoolStrict(Args("Recursive", true), "Recursive")); + Assert.True(ToolArgumentHelper.GetBoolStrict(Args("Recursive", Json("true")), "Recursive")); + Assert.False(ToolArgumentHelper.GetBoolStrict(Args("Recursive", Json("false")), "Recursive")); + Assert.True(ToolArgumentHelper.GetBoolStrict(Args("Recursive", "true"), "Recursive")); + Assert.True(ToolArgumentHelper.GetBoolStrict(Args("Recursive", "True"), "Recursive")); + Assert.True(ToolArgumentHelper.GetBoolStrict(Args("Recursive", Json("\"true\"")), "Recursive")); + } + + [Theory] + [InlineData("yes")] + [InlineData("1")] + public void BoolStrict_colloquial_string_throws(string value) + { + var ex = Assert.Throws( + () => ToolArgumentHelper.GetBoolStrict(Args("Recursive", value), "Recursive")); + Assert.Contains("'Recursive'", ex.Message); + Assert.Contains("boolean", ex.Message); + } + + [Fact] + public void BoolStrict_numeric_one_throws() + { + Assert.Throws( + () => ToolArgumentHelper.GetBoolStrict(Args("Recursive", 1), "Recursive")); + } + + [Fact] + public void Strict_error_renders_long_values_bounded() + { + var huge = new string('x', 500); + var ex = Assert.Throws( + () => ToolArgumentHelper.GetIntStrict(Args("Limit", huge), "Limit")); + Assert.True(ex.Message.Length < 300); + } + + [Fact] + public void Strict_string_parsing_is_culture_invariant() + { + // The daemon does not run with InvariantGlobalization, so a comma-decimal + // host locale must not change how a string-typed numeric argument parses. + var original = CultureInfo.CurrentCulture; + try + { + CultureInfo.CurrentCulture = new CultureInfo("de-DE"); + Assert.Equal(1.5, ToolArgumentHelper.GetDoubleStrict(Args("Scale", "1.5"), "Scale")); + Assert.Equal(1500, ToolArgumentHelper.GetIntStrict(Args("Limit", "1500"), "Limit")); + // de-DE would read "1.5" as 15 via a group separator if culture leaked in. + Assert.NotEqual(15.0, ToolArgumentHelper.GetDoubleStrict(Args("Scale", "1.5"), "Scale")); + } + finally + { + CultureInfo.CurrentCulture = original; + } + } +} diff --git a/src/Netclaw.Actors.Tests/Tools/ToolArgumentValidatorTests.cs b/src/Netclaw.Actors.Tests/Tools/ToolArgumentValidatorTests.cs new file mode 100644 index 000000000..dd1d4d1f9 --- /dev/null +++ b/src/Netclaw.Actors.Tests/Tools/ToolArgumentValidatorTests.cs @@ -0,0 +1,241 @@ +// ----------------------------------------------------------------------- +// +// Copyright (C) 2026 - 2026 Petabridge, LLC +// +// ----------------------------------------------------------------------- +using Microsoft.Extensions.AI; +using Netclaw.Actors.Tools; +using Netclaw.Configuration; +using Netclaw.Security; +using Netclaw.Tests.Utilities; +using Netclaw.Tools; +using Xunit; + +namespace Netclaw.Actors.Tests.Tools; + +/// +/// Unknown-key validation at the dispatcher: near-miss keys are rejected with +/// a suggestion (never silently bound — the original production bug passed +/// "TimeoutSeconds" and got a silent 90s default), declared-param flexible +/// binding is preserved, and exact meta keys pass through. +/// +public class ToolArgumentValidatorTests +{ + private readonly DispatchingToolExecutor _executor; + + public ToolArgumentValidatorTests() + { + var config = new ToolConfig(); + config.AudienceProfiles.Personal.ApprovalPolicy = new ToolApprovalConfig + { + ToolOverrides = new Dictionary(StringComparer.Ordinal) + { + ["shell_execute"] = ToolApprovalMode.Auto + } + }; + + var registry = new ToolRegistry(); + registry.WithFirstPartyTools(config); + _executor = new DispatchingToolExecutor( + registry, + new ToolAccessPolicy( + config, + new EffectivePolicyDefaults( + DeploymentPosture.Personal, + TrustAudience.Personal, + ShellExecutionMode.HostAllowed, + UsedStrictFallback: false))); + } + + private static ToolExecutionContext PersonalContext(string sessionDir) + => new("signalr/thread-1", sessionDir) + { + Audience = TrustAudience.Personal, + Boundary = TrustBoundary.TrustedInstance, + ChannelType = "signalr" + }; + + private async Task ExecuteShellAsync(IDictionary args) + { + var sessionDir = Path.Combine(Path.GetTempPath(), "nc-val-" + Guid.NewGuid().ToString("N")); + Directory.CreateDirectory(sessionDir); + try + { + var toolCall = new FunctionCallContent("call-1", "shell_execute", args); + return await _executor.ExecuteAsync( + toolCall, PersonalContext(sessionDir), TestContext.Current.CancellationToken); + } + finally + { + Directory.Delete(sessionDir, recursive: true); + } + } + + [Fact] + public async Task TimeoutSeconds_rejected_with_meta_key_suggestion() + { + // The literal arg shape from production session + // D0AC6CKBK5K_1781115410_840529 that was silently dropped. + var result = await ExecuteShellAsync(new Dictionary + { + ["Command"] = "echo should-not-run", + ["TimeoutSeconds"] = "1200" + }); + + Assert.Contains("Unrecognized argument 'TimeoutSeconds'", result); + Assert.Contains("Did you mean '_timeout_seconds'?", result); + Assert.Contains("NOT executed", result); + Assert.DoesNotContain("should-not-run", result); + } + + [Fact] + public async Task Underscore_missing_timeout_seconds_rejected_never_bound() + { + var result = await ExecuteShellAsync(new Dictionary + { + ["Command"] = "echo should-not-run", + ["timeout_seconds"] = 300 + }); + + Assert.Contains("Unrecognized argument 'timeout_seconds'", result); + Assert.Contains("Did you mean '_timeout_seconds'?", result); + Assert.DoesNotContain("should-not-run", result); + } + + [Fact] + public async Task Lowercase_declared_param_still_accepted() + { + // Deterministic canonicalization for declared params is existing + // consumption behavior (Qwen text-parser path emits lowercase keys). + var result = await ExecuteShellAsync(new Dictionary + { + ["command"] = "echo flexible-ok" + }); + + Assert.DoesNotContain("Unrecognized argument", result); + Assert.Contains("flexible-ok", result); + } + + [Fact] + public async Task Exact_meta_key_accepted() + { + var result = await ExecuteShellAsync(new Dictionary + { + ["Command"] = "echo meta-ok", + ["_timeout_seconds"] = 120, + ["_rationale"] = "test" + }); + + Assert.DoesNotContain("Unrecognized argument", result); + Assert.Contains("meta-ok", result); + } + + [Fact] + public async Task Wholly_unknown_key_rejected_without_suggestion_lists_valid_args() + { + var result = await ExecuteShellAsync(new Dictionary + { + ["Command"] = "echo should-not-run", + ["Banana"] = true + }); + + Assert.Contains("Unrecognized argument 'Banana'", result); + Assert.DoesNotContain("Did you mean", result); + Assert.Contains("Valid arguments:", result); + Assert.Contains("Command", result); + Assert.Contains("_timeout_seconds", result); + Assert.DoesNotContain("should-not-run", result); + } + + [Fact] + public async Task Typo_in_declared_param_rejected_with_suggestion() + { + var result = await ExecuteShellAsync(new Dictionary + { + ["Comand"] = "echo should-not-run" + }); + + Assert.Contains("Unrecognized argument 'Comand'", result); + Assert.Contains("Did you mean 'Command'?", result); + Assert.DoesNotContain("should-not-run", result); + } + + // A native tool that declares 'text' (like send_channel_message) but binds + // an interchangeable 'Message' key at runtime — recognition must accept the + // alias bidirectionally or a previously-working call shape regresses. + private sealed class TextAliasTool : INetclawTool + { + public string Name => "fake_text_tool"; + public LlmFacingToolName LlmFacingName { get; } = LlmFacingToolName.FromCanonical("fake_text_tool"); + public string Description => ""; + public string GrantCategory => "test"; + public System.Text.Json.JsonElement ParameterSchema { get; } = + System.Text.Json.JsonDocument.Parse( + """{"type":"object","properties":{"text":{"type":"string"},"_rationale":{"type":"string"}}}""") + .RootElement.Clone(); + + public Task ExecuteAsync(IDictionary? arguments, CancellationToken ct = default) + => Task.FromResult("ok"); + + // Not exercised by key validation, which reads only Name + ParameterSchema. + public AITool ToAITool() => AIFunctionFactory.Create(() => "ok", Name); + } + + [Fact] + public void Declared_text_accepts_message_alias_and_vice_versa() + { + var tool = new TextAliasTool(); + + // 'Message' is consumed by binding's text↔Message fallback even though + // only 'text' is declared — it must not be rejected. + Assert.Null(ToolArgumentValidator.ValidateArgumentKeys(tool, new Dictionary + { + ["Message"] = "hi", + ["_rationale"] = "test" + })); + + // The declared key itself still works. + Assert.Null(ToolArgumentValidator.ValidateArgumentKeys(tool, new Dictionary + { + ["text"] = "hi" + })); + + // A genuinely unknown key is still rejected. + Assert.NotNull(ToolArgumentValidator.ValidateArgumentKeys(tool, new Dictionary + { + ["bogus"] = "x" + })); + } + + [Fact] + public async Task Mcp_tools_exempt_from_native_validation() + { + // McpToolAdapter is skipped at the dispatcher gate: an extra unknown + // key must NOT produce the native "Unrecognized argument" rejection — + // the MCP server's own schema validation is the authority + // (mcp-schema-coercion spec). The unknown-key gate runs BEFORE + // authorization, so reaching any other outcome proves the exemption. + var fakeTool = AIFunctionFactory.Create(() => "mcp-result", "store"); + var registry = new ToolRegistry(); + registry.Register(new McpToolAdapter(fakeTool, "memorizer", "store")); + var executor = new DispatchingToolExecutor(registry); + + string result; + try + { + result = await executor.ExecuteAsync( + new FunctionCallContent("call-mcp", "memorizer/store", new Dictionary + { + ["TotallyUnknownKey"] = "value" + }), + ct: TestContext.Current.CancellationToken); + } + catch (Exception ex) + { + // An authorization/invocation failure is still past the key gate. + result = ex.Message; + } + + Assert.DoesNotContain("Unrecognized argument", result); + } +} diff --git a/src/Netclaw.Actors.Tests/Tools/WebFetchToolTests.cs b/src/Netclaw.Actors.Tests/Tools/WebFetchToolTests.cs index fb78b6f3c..f49c04667 100644 --- a/src/Netclaw.Actors.Tests/Tools/WebFetchToolTests.cs +++ b/src/Netclaw.Actors.Tests/Tools/WebFetchToolTests.cs @@ -692,6 +692,71 @@ public void GetFallbackExtension_returns_correct_extension(string contentType, b /// /// Fake HTTP handler that returns a canned response (text or binary). /// + [Fact] + public async Task ExecuteAsync_unsupported_format_rejects_without_http_request() + { + var handler = new CountingHttpHandler("irrelevant", "text/html"); + var httpClient = new HttpClient(handler); + var tool = new WebFetchTool(httpClient: httpClient, fetchDirectory: _dir.Path); + + var result = await tool.ExecuteAsync( + ToolInput.Create("Url", "https://example.com/test", "Format", "markdown"), + CancellationToken.None); + + Assert.Contains("'Format' value 'markdown' is not supported", result); + Assert.Contains("raw, text", result); + Assert.Contains("NOT performed", result); + Assert.Equal(0, handler.Requests); + } + + [Fact] + public async Task ExecuteAsync_body_over_cap_carries_truncation_notice() + { + // 5 MB cap + 1 KB over: the notice must distinguish "truncated" from + // "exactly at the cap" — byte count alone is not a signal. + var oversized = new byte[5 * 1024 * 1024 + 1024]; + Array.Fill(oversized, (byte)'a'); + var handler = new FakeHttpHandler(oversized, "text/plain"); + var httpClient = new HttpClient(handler); + var tool = new WebFetchTool(httpClient: httpClient, fetchDirectory: _dir.Path); + + var result = await tool.ExecuteAsync( + ToolInput.Create("Url", "https://example.com/huge.txt"), + CancellationToken.None); + + Assert.Contains("[content truncated at 5 MB", result); + } + + [Fact] + public async Task ExecuteAsync_body_under_cap_has_no_truncation_notice() + { + var handler = new FakeHttpHandler("small body content", "text/plain"); + var httpClient = new HttpClient(handler); + var tool = new WebFetchTool(httpClient: httpClient, fetchDirectory: _dir.Path); + + var result = await tool.ExecuteAsync( + ToolInput.Create("Url", "https://example.com/small.txt"), + CancellationToken.None); + + Assert.DoesNotContain("content truncated", result); + } + + private sealed class CountingHttpHandler(string content, string contentType) : HttpMessageHandler + { + public int Requests; + + protected override Task SendAsync( + HttpRequestMessage request, CancellationToken ct) + { + Requests++; + var response = new HttpResponseMessage(System.Net.HttpStatusCode.OK) + { + Content = new StringContent(content, Encoding.UTF8, contentType) + }; + return Task.FromResult(response); + } + } + private sealed class FakeHttpHandler : HttpMessageHandler { private readonly byte[] _bytes; diff --git a/src/Netclaw.Actors/Sessions/LlmSessionActor.cs b/src/Netclaw.Actors/Sessions/LlmSessionActor.cs index 0700ef81a..3665305bf 100644 --- a/src/Netclaw.Actors/Sessions/LlmSessionActor.cs +++ b/src/Netclaw.Actors/Sessions/LlmSessionActor.cs @@ -1878,8 +1878,6 @@ await self.Ask( approvalChannel: _approvalChannel, emitApprovalRequest: request => self.Tell(request), approvalTimeout: Timeout.InfiniteTimeSpan, - maxToolTimeoutSeconds: _toolAccessPolicy?.MaxToolTimeoutSeconds ?? 600, - shellTimeoutSeconds: _toolAccessPolicy?.ShellTimeoutSeconds ?? 60, backgroundJobManager: bgJobManager, projectDirectory: _state.WorkingContext.ProjectDirectory, setWorkingDirectoryAvailable: setWorkingDirectoryAvailable, diff --git a/src/Netclaw.Actors/Sessions/Pipelines/SessionToolExecutionPipeline.cs b/src/Netclaw.Actors/Sessions/Pipelines/SessionToolExecutionPipeline.cs index d1f61b7dd..c16987cf3 100644 --- a/src/Netclaw.Actors/Sessions/Pipelines/SessionToolExecutionPipeline.cs +++ b/src/Netclaw.Actors/Sessions/Pipelines/SessionToolExecutionPipeline.cs @@ -83,9 +83,7 @@ public static async Task ExecuteToolsAsync( IApprovalChannel? approvalChannel = null, Action? emitApprovalRequest = null, TimeSpan? approvalTimeout = null, - int maxToolTimeoutSeconds = 600, ILogger? logger = null, - int shellTimeoutSeconds = 60, IActorRef? backgroundJobManager = null, string? projectDirectory = null, bool setWorkingDirectoryAvailable = false, @@ -121,9 +119,7 @@ public static async Task ExecuteToolsAsync( approvalChannel, emitApprovalRequest, approvalTimeout ?? Timeout.InfiniteTimeSpan, - maxToolTimeoutSeconds, logger, - shellTimeoutSeconds, backgroundJobManager, projectDirectory, setWorkingDirectoryAvailable, @@ -195,9 +191,7 @@ public static async Task ExecuteSingleToolAsync( IApprovalChannel? approvalChannel = null, Action? emitApprovalRequest = null, TimeSpan? approvalTimeout = null, - int maxToolTimeoutSeconds = 600, ILogger? logger = null, - int shellTimeoutSeconds = 60, IActorRef? backgroundJobManager = null, string? projectDirectory = null, bool setWorkingDirectoryAvailable = false, @@ -207,14 +201,38 @@ public static async Task ExecuteSingleToolAsync( TurnContext? turnContext = null, ModelInputBatchBudget? modelInputBudget = null) { + // Pre-dispatch validation, on the ORIGINAL (pre-extraction) arguments: + // provider args-parse sentinel, present-but-invalid meta values, and + // unrecognized argument keys. Shared with the executor (and thus the + // sub-agent path) via IToolExecutor.ValidateToolCall so the rules live + // in one place. Rejecting here — rather than letting the executor return + // the rejection string from ExecuteAsync — is what lets the denial be + // audited as Allowed=false instead of being misreported as executed. + if (executor.ValidateToolCall(tc) is { } rejection) + { + auditLogger?.Log(BuildAuditEntry(sessionId, tc, timeProvider, TimeSpan.Zero, meta: null) with + { + Allowed = false, + DenyReason = rejection.DenyReason + }); + + return new ToolCallResult(new SerializableChatMessage + { + Role = Protocol.ChatRole.Tool, + Content = rejection.Message, + ToolCallId = new ToolCallId(tc.CallId), + Name = tc.Name + }, [], [], [], []); + } + var (meta, cleanedTc) = ToolCallMetaExtractor.Extract(tc); tc = cleanedTc; - if (meta?.TimeoutHintSeconds is not null) - { - timeout = ToolCallMetaExtractor.ComputeEffectiveTimeout( - meta.TimeoutHintSeconds, timeout, maxToolTimeoutSeconds); - } + // The agent's per-call timeout hint is honored as requested; when absent + // the inherited default (SessionConfig.ToolExecutionTimeout) applies. + // ExtractFrom only yields a positive hint, so there is nothing to clamp. + if (meta?.TimeoutHintSeconds is { } hintSeconds) + timeout = TimeSpan.FromSeconds(hintSeconds); var sw = Stopwatch.StartNew(); string resultText; @@ -376,7 +394,10 @@ public static async Task ExecuteSingleToolAsync( tc, sessionId, source, auditLogger, timeProvider, turnContext, meta, backgroundJobManager, - meta.TimeoutHintSeconds ?? shellTimeoutSeconds, + // Honor the agent's requested timeout; when absent, the + // inherited per-call default applies (same source as the + // synchronous path). + meta.TimeoutHintSeconds ?? (int)timeout.TotalSeconds, sw.Elapsed, logger, context.AppliedApprovalDecision, context.AppliedApprovalPattern); @@ -476,7 +497,10 @@ or ApprovalDecision.ApprovedAlways tc, sessionId, source, auditLogger, timeProvider, turnContext, meta, backgroundJobManager, - meta.TimeoutHintSeconds ?? shellTimeoutSeconds, + // Honor the agent's requested timeout; when absent, the + // inherited per-call default applies (same source as the + // synchronous path). + meta.TimeoutHintSeconds ?? (int)timeout.TotalSeconds, sw.Elapsed, logger, decision.ToString(), string.Join(", ", ctx.Patterns)); diff --git a/src/Netclaw.Actors/Sessions/Pipelines/ToolCallMetaExtractor.cs b/src/Netclaw.Actors/Sessions/Pipelines/ToolCallMetaExtractor.cs index 99d866f3b..136e35583 100644 --- a/src/Netclaw.Actors/Sessions/Pipelines/ToolCallMetaExtractor.cs +++ b/src/Netclaw.Actors/Sessions/Pipelines/ToolCallMetaExtractor.cs @@ -3,6 +3,7 @@ // Copyright (C) 2026 - 2026 Petabridge, LLC // // ----------------------------------------------------------------------- +using System.Text.Json; using Microsoft.Extensions.AI; using Netclaw.Tools; @@ -25,20 +26,36 @@ public static (ToolCallMeta? Meta, FunctionCallContent CleanedToolCall) Extract( } /// - /// Computes the effective timeout by clamping the LLM's hint between the tool's - /// default floor and the config ceiling. + /// Rejects present-but-invalid meta values before dispatch. Returns null when + /// the meta surface is valid; otherwise a model-facing error (the call must + /// not execute — the agent expressed execution semantics we cannot honor, so + /// we do not run on defaults instead). Computed pipeline-side so the + /// persisted type stays unchanged. Exact key + /// lookup mirrors . /// - public static TimeSpan ComputeEffectiveTimeout( - int? hintSeconds, TimeSpan defaultTimeout, int maxToolTimeoutSeconds) + public static string? ValidateMetaValues(IDictionary? arguments) { - if (!hintSeconds.HasValue || hintSeconds.Value <= 0) - return defaultTimeout; + if (arguments is null || arguments.Count == 0) + return null; - var floorSeconds = (int)defaultTimeout.TotalSeconds; - if (hintSeconds.Value < floorSeconds) - return defaultTimeout; + // Validity is defined as "the shared coercion accepts it" — the same + // TryCoerce* ToolCallMeta.ExtractFrom binds through — so a value can + // never validate here yet extract to null (or vice versa). A timeout + // additionally must be positive, matching ExtractFrom's `> 0` guard. + if (arguments.TryGetValue("_timeout_seconds", out var tVal) + && tVal is not null and not JsonElement { ValueKind: JsonValueKind.Null } + && !(ToolArgumentHelper.TryCoerceInt(tVal, out var t) && t > 0)) + { + return $"Error: Meta argument '_timeout_seconds' value '{ToolArgumentHelper.RenderValue(tVal)}' is not a valid positive integer. The tool was NOT executed."; + } - var clamped = Math.Min(hintSeconds.Value, maxToolTimeoutSeconds); - return TimeSpan.FromSeconds(clamped); + if (arguments.TryGetValue("_background", out var bVal) + && bVal is not null and not JsonElement { ValueKind: JsonValueKind.Null } + && !ToolArgumentHelper.TryCoerceBool(bVal, out _)) + { + return $"Error: Meta argument '_background' value '{ToolArgumentHelper.RenderValue(bVal)}' is not a valid boolean. The tool was NOT executed."; + } + + return null; } } diff --git a/src/Netclaw.Actors/Tools/DispatchingToolExecutor.cs b/src/Netclaw.Actors/Tools/DispatchingToolExecutor.cs index 359cf39f1..1d2cb8535 100644 --- a/src/Netclaw.Actors/Tools/DispatchingToolExecutor.cs +++ b/src/Netclaw.Actors/Tools/DispatchingToolExecutor.cs @@ -8,6 +8,7 @@ using Microsoft.Extensions.AI; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Logging.Abstractions; +using Netclaw.Actors.Sessions.Pipelines; using Netclaw.Configuration; using Netclaw.Security; using Netclaw.Tools; @@ -50,6 +51,57 @@ public DispatchingToolExecutor(ToolRegistry registry, ToolAccessPolicy policy, _logger = logger ?? (ILogger)NullLogger.Instance; } + /// + public ToolArgumentRejection? ValidateToolCall(FunctionCallContent toolCall) + { + if (_registry.GetByName(toolCall.Name) is not { } registered) + return null; // unknown-tool is handled separately by the execute paths + + // Registry-free checks first (parse sentinel, meta values). + if (ValidateArguments(toolCall.Arguments) is { } rejection) + return rejection; + + // Unrecognized argument keys — native tools only; MCP servers validate + // their own schema and reject observably. + if (registered is not McpToolAdapter + && ToolArgumentValidator.ValidateArgumentKeys(registered, toolCall.Arguments) is { } keyError) + return new ToolArgumentRejection(keyError, "unrecognized_argument"); + + return null; + } + + /// + /// The schema-independent half of : provider + /// args-parse sentinel + present-but-invalid meta values. Static so it is + /// the single definition of these rules across the executor and any other + /// pre-dispatch caller, with no registry needed. + /// + public static ToolArgumentRejection? ValidateArguments(IDictionary? args) + { + if (args is null || args.Count == 0) + return null; + + // Provider args-parse failure rides as a sentinel key (set by the + // OpenAI-compatible client when the model's arguments JSON did not + // deserialize). Checked first so the sentinel key is not then reported + // as an "unrecognized argument", and the value is bounded so a + // forged/oversized value cannot flood the result. + if (args.TryGetValue(ToolCallArgumentErrors.ArgsParseErrorKey, out var parseFailure)) + { + return new ToolArgumentRejection( + $"Error: Tool call arguments were not valid JSON: {ToolArgumentHelper.RenderValue(parseFailure, maxLength: 200)} The tool was NOT executed.", + "args_parse_error"); + } + + // Present-but-invalid meta values (malformed _timeout_seconds / + // _background) — the agent expressed execution semantics we cannot + // honor, so reject rather than run on defaults. + if (ToolCallMetaExtractor.ValidateMetaValues(args) is { } metaError) + return new ToolArgumentRejection(metaError, "invalid_meta_value"); + + return null; + } + public async Task ExecuteAsync(FunctionCallContent toolCall, ToolExecutionContext? context = null, CancellationToken ct = default) { if (_registry.GetByName(toolCall.Name) is null) @@ -58,6 +110,19 @@ public async Task ExecuteAsync(FunctionCallContent toolCall, ToolExecuti return $"Unknown tool: {toolCall.Name}"; } + // Pre-dispatch validation runs before authorization so a doomed call + // never raises an approval prompt. This is the shared seam: callers that + // bypass the session pipeline (sub-agents, direct callers) get the same + // protection here. The pipeline preflights via ValidateToolCall too, so + // for that path this is a cheap idempotent re-check. + if (ValidateToolCall(toolCall) is { } rejection) + { + _logger.LogWarning( + "Rejected tool call ({Reason}): {ToolName} — {Error}", + rejection.DenyReason, toolCall.Name, rejection.Message); + return rejection.Message; + } + var tool = await AuthorizeCoreAsync(toolCall, context, ct); var sw = Stopwatch.StartNew(); @@ -126,6 +191,16 @@ public async IAsyncEnumerable ExecuteStreamAsync( yield break; } + // Same pre-authorization validation as the non-streaming path. + if (ValidateToolCall(toolCall) is { } rejection) + { + _logger.LogWarning( + "Rejected tool call ({Reason}): {ToolName} — {Error}", + rejection.DenyReason, toolCall.Name, rejection.Message); + yield return new ToolCompletedUpdate(rejection.Message); + yield break; + } + // Authorization throws (ToolApprovalRequiredException / ToolAccessDeniedException) // before the first item is produced; the tool-execution pipeline handles // those exactly as it does for the non-streaming path. diff --git a/src/Netclaw.Actors/Tools/IToolExecutor.cs b/src/Netclaw.Actors/Tools/IToolExecutor.cs index e11d74ff2..e42799090 100644 --- a/src/Netclaw.Actors/Tools/IToolExecutor.cs +++ b/src/Netclaw.Actors/Tools/IToolExecutor.cs @@ -20,6 +20,18 @@ public interface IToolExecutor Task AuthorizeAsync(FunctionCallContent toolCall, ToolExecutionContext? context = null, CancellationToken ct = default); + /// + /// Pre-dispatch argument validation shared by every caller (main session + /// pipeline AND sub-agent loop AND any direct caller): provider args-parse + /// failure, present-but-invalid meta values, and unrecognized argument keys. + /// Returns null when the call may proceed, otherwise a model-facing + /// rejection. Centralizing here is what keeps the no-silent-discard + /// invariant from holding only on the pipeline path. The default returns + /// null so test fakes need not implement it; + /// provides the real check. + /// + ToolArgumentRejection? ValidateToolCall(FunctionCallContent toolCall) => null; + /// /// Execute a tool call as a stream of items. The /// default implementation runs and yields its @@ -35,6 +47,13 @@ async IAsyncEnumerable ExecuteStreamAsync( } } +/// +/// A pre-dispatch tool-argument rejection: the model-facing message and a +/// stable audit reason. Carries the reason so callers audit the denial +/// accurately instead of misreporting a rejected call as executed. +/// +public sealed record ToolArgumentRejection(string Message, string DenyReason); + /// /// Audit entry for tool invocations. Logged regardless of allow/deny. /// diff --git a/src/Netclaw.Actors/Tools/ListWebhooksTool.cs b/src/Netclaw.Actors/Tools/ListWebhooksTool.cs index 74414f581..e99927acd 100644 --- a/src/Netclaw.Actors/Tools/ListWebhooksTool.cs +++ b/src/Netclaw.Actors/Tools/ListWebhooksTool.cs @@ -28,21 +28,37 @@ public ListWebhooksTool(WebhookRouteStore store) protected override Task ExecuteAsync(Params args, CancellationToken ct) { + var filter = args.Filter?.ToLowerInvariant() ?? "active"; + if (filter is not ("active" or "all")) + { + return Task.FromResult( + $"Error: Parameter 'Filter' value '{args.Filter}' is not supported. " + + "Valid values: active, all. The tool was NOT executed."); + } + var configured = _store.ListRouteFiles(); if (configured.Count == 0) return Task.FromResult("No webhook routes configured."); + // 'active' keeps unreadable routes visible: they cannot be classified + // as disabled, and hiding a broken route under the default filter + // would hide exactly the problem this tool exists to surface. + var routes = filter == "active" + ? configured.Where(r => r.Definition is null || r.Definition.Enabled).ToList() + : [.. configured]; + var sb = new StringBuilder(); - sb.AppendLine($"Webhook routes ({configured.Count}):"); + sb.AppendLine($"Webhook routes ({routes.Count} of {configured.Count}, filter: {filter}):"); sb.AppendLine(); - foreach (var (routeName, _, definition) in configured) + foreach (var (routeName, _, definition) in routes) { sb.AppendLine($" Route: {routeName}"); sb.AppendLine($" Status: {(definition is null ? "invalid_or_unreadable" : "configured")}"); if (definition is not null) { + sb.AppendLine($" Enabled: {definition.Enabled}"); sb.AppendLine($" Audience: {definition.Audience}"); sb.AppendLine($" Verification: {definition.Verification.Kind}"); sb.AppendLine($" DeliveryRequired: {definition.DeliveryRequired}"); diff --git a/src/Netclaw.Actors/Tools/ShellTool.cs b/src/Netclaw.Actors/Tools/ShellTool.cs index 4f598d501..cab56e08b 100644 --- a/src/Netclaw.Actors/Tools/ShellTool.cs +++ b/src/Netclaw.Actors/Tools/ShellTool.cs @@ -26,6 +26,14 @@ public sealed partial class ShellTool : NetclawTool { public const string ToolName = "shell_execute"; + // Fallback wall-clock timeout used only when ShellTool runs without a + // pipeline-provided context (direct/test calls). In the session pipeline + // the per-call timeout always arrives via ToolExecutionContext + // .RequestedTimeoutSeconds (SessionConfig.ToolExecutionTimeout, or the + // agent's honored _timeout_seconds hint), so this default is just a safety + // net against an unbounded process. + private const int DefaultTimeoutSeconds = 90; + // Shell output is mostly verbose noise the model skims, so bound it // aggressively: small inline head+tail, full output spilled to a session file // to grep. Content tools (file_read, web_fetch, MCP) keep the larger session @@ -137,7 +145,7 @@ or UnauthorizedAccessException var effectiveTimeoutSeconds = context.RequestedTimeoutSeconds is > 0 ? context.RequestedTimeoutSeconds.Value - : _config.ShellTimeoutSeconds; + : DefaultTimeoutSeconds; using var timeoutCts = new CancellationTokenSource(); using var linkedCts = CancellationTokenSource.CreateLinkedTokenSource(ct, timeoutCts.Token); @@ -379,7 +387,7 @@ or UnauthorizedAccessException var effectiveTimeoutSeconds = context.RequestedTimeoutSeconds is > 0 ? context.RequestedTimeoutSeconds.Value - : _config.ShellTimeoutSeconds; + : DefaultTimeoutSeconds; // Wall-clock ceiling: the watchdog's inactivity budget resets on // each activity item (keeping chatty commands alive), but a command diff --git a/src/Netclaw.Actors/Tools/ToolAccessPolicy.cs b/src/Netclaw.Actors/Tools/ToolAccessPolicy.cs index 34df1dad1..646a681d4 100644 --- a/src/Netclaw.Actors/Tools/ToolAccessPolicy.cs +++ b/src/Netclaw.Actors/Tools/ToolAccessPolicy.cs @@ -46,10 +46,6 @@ public ToolAccessPolicy( _safeVerbPolicy = safeVerbs is not null ? new ScopedShellSafeVerbPolicy(safeVerbs) : null; } - public int MaxToolTimeoutSeconds => _toolConfig.MaxToolTimeoutSeconds; - - public int ShellTimeoutSeconds => _toolConfig.ShellTimeoutSeconds; - public IReadOnlyList FilterExposedTools( IEnumerable tools, ToolRegistry registry, diff --git a/src/Netclaw.Actors/Tools/WebFetchTool.cs b/src/Netclaw.Actors/Tools/WebFetchTool.cs index 0eeba63e7..a4d2d4776 100644 --- a/src/Netclaw.Actors/Tools/WebFetchTool.cs +++ b/src/Netclaw.Actors/Tools/WebFetchTool.cs @@ -3,6 +3,7 @@ // Copyright (C) 2026 - 2026 Petabridge, LLC // // ----------------------------------------------------------------------- +using System.Buffers; using System.ComponentModel; using System.Net; using System.Text; @@ -29,6 +30,9 @@ public sealed partial class WebFetchTool : NetclawTool private const int PreviewLines = 10; private const int MaxResponseBytes = 5 * 1024 * 1024; // 5MB + private const string TruncationNotice = + "\n[content truncated at 5 MB — the response exceeded the cap; the remainder was not fetched]"; + private static readonly string[] UserAgents = [ "Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/120.0.0.0 Safari/537.36", @@ -70,6 +74,17 @@ protected override async Task ExecuteAsync(Params args, ToolExecutionCon if (string.IsNullOrWhiteSpace(args.Url)) return "Error: 'url' parameter is required."; + // An unsupported format must not silently fall back to raw mode — the + // agent asked for an output shape we don't produce (netclaw-tools spec: + // web fetch format is validated). + if (args.Format is not null + && !string.Equals(args.Format, "raw", StringComparison.OrdinalIgnoreCase) + && !string.Equals(args.Format, "text", StringComparison.OrdinalIgnoreCase)) + { + return $"Error: Parameter 'Format' value '{args.Format}' is not supported. " + + "Valid values: raw, text. The fetch was NOT performed."; + } + if (!Uri.TryCreate(args.Url, UriKind.Absolute, out var uri) || uri.Scheme is not ("http" or "https")) return "Error: Invalid URL. Must be an absolute HTTP or HTTPS URL."; @@ -96,7 +111,7 @@ protected override async Task ExecuteAsync(Params args, ToolExecutionCon if (IsBinaryContentType(contentType)) { - var bytes = await ReadBytesWithLimitAsync(response, ct); + var (bytes, binaryTruncated) = await ReadBytesWithLimitAsync(response, ct); if (bytes.Length == 0) return $"Fetched {args.Url} but the response was empty."; @@ -104,10 +119,11 @@ protected override async Task ExecuteAsync(Params args, ToolExecutionCon ?? GetFallbackExtension(contentType, isBinary: true); var filePath = SaveBytesToFile(bytes, uri, fetchDir, extension); - return FormatBinarySummary(uri.ToString(), filePath, bytes.Length, contentType); + var binarySummary = FormatBinarySummary(uri.ToString(), filePath, bytes.Length, contentType); + return binaryTruncated ? binarySummary + TruncationNotice : binarySummary; } - var content = await ReadTextWithLimitAsync(response, ct); + var (content, contentTruncated) = await ReadTextWithLimitAsync(response, ct); var useTextMode = string.Equals(args.Format, "text", StringComparison.OrdinalIgnoreCase); string savedContent; @@ -146,7 +162,8 @@ protected override async Task ExecuteAsync(Params args, ToolExecutionCon var textFilePath = SaveToFile(savedContent, uri, fetchDir, textExtension); var lineCount = savedContent.Count(c => c == '\n') + 1; - return FormatSummary(uri.ToString(), title, textFilePath, savedContent.Length, lineCount, previewText); + var summary = FormatSummary(uri.ToString(), title, textFilePath, savedContent.Length, lineCount, previewText); + return contentTruncated ? summary + TruncationNotice : summary; } catch (HttpRequestException ex) { @@ -206,14 +223,44 @@ internal static string GetFallbackExtension(string contentType, bool isBinary) : isBinary ? ".bin" : ".txt"; } - private static async Task ReadTextWithLimitAsync(HttpResponseMessage response, CancellationToken ct) - => Encoding.UTF8.GetString(await ReadBytesWithLimitAsync(response, ct)); + private static async Task<(string Content, bool Truncated)> ReadTextWithLimitAsync(HttpResponseMessage response, CancellationToken ct) + { + var (bytes, truncated) = await ReadBytesWithLimitAsync(response, ct); + return (Encoding.UTF8.GetString(bytes), truncated); + } - private static async Task ReadBytesWithLimitAsync(HttpResponseMessage response, CancellationToken ct) + private static async Task<(byte[] Bytes, bool Truncated)> ReadBytesWithLimitAsync(HttpResponseMessage response, CancellationToken ct) { var stream = await response.Content.ReadAsStreamAsync(ct); - using var limited = new BinaryReader(stream); - return limited.ReadBytes(MaxResponseBytes); + + // Stream into a buffer that grows to the actual content size, capped at + // MaxResponseBytes. Avoids eagerly allocating the full cap for every + // small fetch (the old BinaryReader.ReadBytes(cap) cost) and the 5 MB + // slice-copy a truncated response used to pay. Truncation is detected by + // reading one extra byte past the cap, so it is surfaced, not inferred. + using var buffer = new MemoryStream(); + var chunk = ArrayPool.Shared.Rent(81920); + try + { + int read; + while ((read = await stream.ReadAsync(chunk, ct)) > 0) + { + var remaining = MaxResponseBytes - (int)buffer.Length; + if (read >= remaining) + { + buffer.Write(chunk, 0, remaining); + return (buffer.ToArray(), Truncated: true); + } + + buffer.Write(chunk, 0, read); + } + } + finally + { + ArrayPool.Shared.Return(chunk); + } + + return (buffer.ToArray(), Truncated: false); } private string BuildFilePath(Uri uri, string directory, string extension) diff --git a/src/Netclaw.Configuration/Schemas/netclaw-config.v1.schema.json b/src/Netclaw.Configuration/Schemas/netclaw-config.v1.schema.json index cbafd32a9..2a65ed2d6 100644 --- a/src/Netclaw.Configuration/Schemas/netclaw-config.v1.schema.json +++ b/src/Netclaw.Configuration/Schemas/netclaw-config.v1.schema.json @@ -287,11 +287,6 @@ "type": ["string", "null"], "enum": ["Off", "SandboxOnly", "HostAllowed", null] }, - "ShellTimeoutSeconds": { - "type": "integer", - "minimum": 1, - "default": 60 - }, "MaxOutputChars": { "type": "integer", "minimum": 1, @@ -323,12 +318,6 @@ "items": { "type": "string" }, "default": [], "description": "Additional shell command patterns to add to the hard deny list. Verb-chain prefixes that are categorically blocked and cannot be approved." - }, - "MaxToolTimeoutSeconds": { - "type": "integer", - "minimum": 1, - "default": 600, - "description": "Maximum per-call timeout in seconds that the LLM can request via _timeout_seconds meta field. Values above this ceiling are clamped." } }, "additionalProperties": false diff --git a/src/Netclaw.Configuration/ToolConfig.cs b/src/Netclaw.Configuration/ToolConfig.cs index fd70b6214..f5dc9c2e5 100644 --- a/src/Netclaw.Configuration/ToolConfig.cs +++ b/src/Netclaw.Configuration/ToolConfig.cs @@ -11,7 +11,6 @@ namespace Netclaw.Configuration; public sealed class ToolConfig { public ShellExecutionMode? ShellMode { get; set; } - public int ShellTimeoutSeconds { get; set; } = 60; /// /// The capture ceiling: the maximum characters of tool output captured (in @@ -24,11 +23,6 @@ public sealed class ToolConfig /// public int MaxOutputChars { get; set; } = 256_000; - /// - /// Maximum per-call timeout in seconds that the LLM can request via _timeout_seconds. - /// Values above this ceiling are clamped. Default 600s (10 minutes). - /// - public int MaxToolTimeoutSeconds { get; set; } = 600; public ToolAudienceProfiles AudienceProfiles { get; set; } = new(); public WebFetchConfig WebFetch { get; set; } = new(); diff --git a/src/Netclaw.Daemon.Tests/Configuration/OpenAiCompatibleChatClientTests.cs b/src/Netclaw.Daemon.Tests/Configuration/OpenAiCompatibleChatClientTests.cs index f1fa01f10..ba41ca78a 100644 --- a/src/Netclaw.Daemon.Tests/Configuration/OpenAiCompatibleChatClientTests.cs +++ b/src/Netclaw.Daemon.Tests/Configuration/OpenAiCompatibleChatClientTests.cs @@ -162,6 +162,43 @@ public async Task BuffersFragmentedToolCallArguments_UntilFinishReason() Assert.Equal("what is TextForge", toolCall.Arguments!["Query"]?.ToString()); } + [Fact] + public async Task MalformedToolCallArguments_CarrySentinel_InsteadOfNullArgs() + { + // Truncated mid-stream arguments JSON must not dispatch a null-args + // call (silent intent discard) — the parse failure travels with the + // call via the sentinel and the pipeline rejects it pre-dispatch + // (tool-arg-validation spec). + const string sse = """ +data: {"id":"abc","model":"Qwen","object":"chat.completion.chunk","choices":[{"index":0,"delta":{"tool_calls":[{"index":0,"id":"call_1","type":"function","function":{"name":"shell_execute","arguments":"{\"Command\":\"ech"}}]}}]} + +data: {"id":"abc","model":"Qwen","object":"chat.completion.chunk","choices":[{"index":0,"delta":{},"finish_reason":"tool_calls"}]} + +data: [DONE] + +"""; + + using var handler = new RecordingHandler(_ => new HttpResponseMessage(HttpStatusCode.OK) + { + Content = new StringContent(sse, Encoding.UTF8, "text/event-stream") + }); + using var httpClient = new HttpClient(handler) { BaseAddress = new Uri("http://localhost:8000") }; + var endpoint = OpenAiCompatibleEndpoint.FromBaseUrl("http://localhost:8000/api/v1"); + var client = new OpenAiCompatibleChatClient(httpClient, endpoint, "test-model"); + + var updates = new List(); + await foreach (var update in client.GetStreamingResponseAsync([new ChatMessage(ChatRole.User, "hello")], cancellationToken: TestContext.Current.CancellationToken)) + updates.Add(update); + + var toolUpdate = Assert.Single(updates, u => u.FinishReason == ChatFinishReason.ToolCalls); + var toolCall = Assert.Single(toolUpdate.Contents.OfType()); + Assert.NotNull(toolCall.Arguments); + var sentinel = Assert.Contains( + Netclaw.Tools.ToolCallArgumentErrors.ArgsParseErrorKey, + (IDictionary)toolCall.Arguments!); + Assert.Contains("Raw arguments prefix:", sentinel?.ToString()); + } + [Fact] public async Task SerializesAssistantToolCalls_AndToolResults_InConversationHistory() { diff --git a/src/Netclaw.Providers/Netclaw.Providers.csproj b/src/Netclaw.Providers/Netclaw.Providers.csproj index a5aac93bd..dbce734ec 100644 --- a/src/Netclaw.Providers/Netclaw.Providers.csproj +++ b/src/Netclaw.Providers/Netclaw.Providers.csproj @@ -19,6 +19,7 @@ + diff --git a/src/Netclaw.Providers/SelfHosted/OpenAiCompatibleChatClient.cs b/src/Netclaw.Providers/SelfHosted/OpenAiCompatibleChatClient.cs index b2a579a0f..728ebbb9a 100644 --- a/src/Netclaw.Providers/SelfHosted/OpenAiCompatibleChatClient.cs +++ b/src/Netclaw.Providers/SelfHosted/OpenAiCompatibleChatClient.cs @@ -778,6 +778,8 @@ private static IEnumerable ParseStreamingUpdates( private static Dictionary? TryDeserializeArguments(string? argumentsJson) { + // Absent/empty arguments are legitimate (no-param tools) — null means + // "no intent expressed" and dispatch proceeds normally. if (string.IsNullOrWhiteSpace(argumentsJson)) return null; @@ -785,9 +787,22 @@ private static IEnumerable ParseStreamingUpdates( { return JsonSerializer.Deserialize>(argumentsJson, JsonOptions); } - catch (JsonException) - { - return null; + catch (JsonException ex) + { + // The model DID emit arguments but they don't parse (truncated + // stream, malformed emission). Dispatching null args here silently + // discards the model's intent — instead the failure travels with + // the call and the pipeline rejects the call id pre-dispatch with a + // model-facing error (tool-arg-validation spec). The raw prefix is + // only available at this boundary. + var rawPrefix = argumentsJson.Length > 200 + ? argumentsJson[..200] + "…" + : argumentsJson; + return new Dictionary + { + [Netclaw.Tools.ToolCallArgumentErrors.ArgsParseErrorKey] = + $"{ex.Message} Raw arguments prefix: {rawPrefix}" + }; } } diff --git a/src/Netclaw.Tools.Abstractions/ToolArgumentHelper.cs b/src/Netclaw.Tools.Abstractions/ToolArgumentHelper.cs index 613e413a1..f3e5ab491 100644 --- a/src/Netclaw.Tools.Abstractions/ToolArgumentHelper.cs +++ b/src/Netclaw.Tools.Abstractions/ToolArgumentHelper.cs @@ -3,6 +3,7 @@ // Copyright (C) 2026 - 2026 Petabridge, LLC // // ----------------------------------------------------------------------- +using System.Globalization; using System.Text.Json; namespace Netclaw.Tools; @@ -50,7 +51,13 @@ private static bool TryGetValueFlexible(IDictionary? arguments, return false; } - private static string NormalizeKey(string key) + /// + /// Deterministic key canonicalization (case + punctuation folding): + /// ChannelId, channel_id, and channel-id all normalize + /// to channelid. Shared with so + /// key recognition mirrors the exact matching that binding performs. + /// + public static string NormalizeKey(string key) { if (string.IsNullOrWhiteSpace(key)) return string.Empty; @@ -67,8 +74,35 @@ private static string NormalizeKey(string key) return count == 0 ? string.Empty : new string(buffer, 0, count); } + // Interchangeable parameter-name groups (normalized form). A tool may + // declare one member and accept another at binding time — e.g. GetString's + // Message↔text fallback below, and SendChannelMessageTool binding + // `text ?? Message`. This is the single definition both binding and + // ToolArgumentValidator's key recognition consume, so validation never + // rejects a key that binding would happily accept. + private static readonly string[][] AliasGroups = [["text", "message"]]; + + /// + /// The normalized names interchangeable with + /// (excluding itself). Empty when the name has no aliases. + /// + public static IEnumerable NormalizedAliasesFor(string normalizedName) + { + foreach (var group in AliasGroups) + { + if (Array.IndexOf(group, normalizedName) < 0) + continue; + foreach (var member in group) + { + if (!string.Equals(member, normalizedName, StringComparison.Ordinal)) + yield return member; + } + } + } + public static string? GetString(IDictionary? arguments, string key) { + // Binding-side consumer of the text↔Message alias group above. if (string.Equals(key, "Message", StringComparison.OrdinalIgnoreCase) && !TryGetValueFlexible(arguments, key, out _) && TryGetValueFlexible(arguments, "text", out var textAlias) @@ -95,54 +129,135 @@ private static string NormalizeKey(string key) }; } - public static int? GetNullableInt(IDictionary? arguments, string key) + // ── Coercion primitives ── + // Single source of truth for loose-value → typed coercion, shared by the + // strict argument binders below AND by ToolCallMeta.ExtractFrom / + // ToolCallMetaExtractor.ValidateMetaValues so the accept-set cannot drift + // between binding and validation (the drift would reintroduce the silent + // intent-drop these guards exist to prevent). All string parsing uses + // InvariantCulture: tool arguments are a machine protocol, not locale-aware + // user input, and the daemon does not run with InvariantGlobalization. + + /// + /// Coerces a loose argument value to . Integral numerics + /// written with a decimal point (e.g. 300.0 — a common LLM emission + /// shape) are accepted; genuinely fractional values (12.7) are not + /// (no silent truncation). Returns false for any other value. + /// + public static bool TryCoerceInt(object? value, out int result) { - if (!TryGetValueFlexible(arguments, key, out var value) || value is null) - return null; + switch (value) + { + case int i: result = i; return true; + case long l and >= int.MinValue and <= int.MaxValue: result = (int)l; return true; + case double d when double.IsInteger(d) && d is >= int.MinValue and <= int.MaxValue: + result = (int)d; return true; + case JsonElement { ValueKind: JsonValueKind.Number } je when je.TryGetInt32(out result): + return true; + // JSON integral written with a decimal point: TryGetInt32 rejects + // "300.0", so fall back to a double check that still bars fractions. + case JsonElement { ValueKind: JsonValueKind.Number } je + when je.TryGetDouble(out var jd) && double.IsInteger(jd) && jd is >= int.MinValue and <= int.MaxValue: + result = (int)jd; return true; + case string s when int.TryParse(s, NumberStyles.Integer, CultureInfo.InvariantCulture, out result): + return true; + case JsonElement { ValueKind: JsonValueKind.String } je + when int.TryParse(je.GetString(), NumberStyles.Integer, CultureInfo.InvariantCulture, out result): + return true; + default: result = 0; return false; + } + } - return value switch + /// Coerces a loose argument value to . + public static bool TryCoerceDouble(object? value, out double result) + { + switch (value) { - int i => i, - long l => (int)l, - double d => (int)d, - JsonElement { ValueKind: JsonValueKind.Number } je => je.GetInt32(), - string s when int.TryParse(s, out var parsed) => parsed, - JsonElement { ValueKind: JsonValueKind.String } je when int.TryParse(je.GetString(), out var parsed) => parsed, - _ => null - }; + case double d: result = d; return true; + case float f: result = f; return true; + case int i: result = i; return true; + case long l: result = l; return true; + case JsonElement { ValueKind: JsonValueKind.Number } je when je.TryGetDouble(out result): + return true; + case string s when double.TryParse(s, NumberStyles.Float, CultureInfo.InvariantCulture, out result): + return true; + case JsonElement { ValueKind: JsonValueKind.String } je + when double.TryParse(je.GetString(), NumberStyles.Float, CultureInfo.InvariantCulture, out result): + return true; + default: result = 0; return false; + } } - public static double? GetNullableDouble(IDictionary? arguments, string key) + /// Coerces a loose argument value to . + public static bool TryCoerceBool(object? value, out bool result) { - if (!TryGetValueFlexible(arguments, key, out var value) || value is null) + switch (value) + { + case bool b: result = b; return true; + case JsonElement { ValueKind: JsonValueKind.True }: result = true; return true; + case JsonElement { ValueKind: JsonValueKind.False }: result = false; return true; + case string s when bool.TryParse(s, out result): return true; + case JsonElement { ValueKind: JsonValueKind.String } je when bool.TryParse(je.GetString(), out result): + return true; + default: result = false; return false; + } + } + + /// + /// True when is "absent" for binding purposes: + /// a missing key (caller's responsibility) or an explicit JSON null. + /// + private static bool IsAbsent(object? value) + => value is null or JsonElement { ValueKind: JsonValueKind.Null }; + + // Strict variants distinguish three states: absent (or JSON null) → null; + // coercible → value; present-but-invalid → ArgumentException naming the + // parameter, supplied value, and expected type. Generated ParseArguments + // uses these so an invalid value rejects the call (surfaced via the + // pipeline's exception→error-result channel) instead of silently coercing + // to 0/0.0/false (tool-arg-validation spec). + + public static int? GetIntStrict(IDictionary? arguments, string key) + { + if (!TryGetValueFlexible(arguments, key, out var value) || IsAbsent(value)) return null; - return value switch - { - double d => d, - float f => f, - int i => i, - long l => l, - JsonElement { ValueKind: JsonValueKind.Number } je => je.GetDouble(), - string s when double.TryParse(s, out var parsed) => parsed, - JsonElement { ValueKind: JsonValueKind.String } je when double.TryParse(je.GetString(), out var parsed) => parsed, - _ => null - }; + return TryCoerceInt(value, out var result) ? result : throw InvalidValue(key, value!, "integer"); } - public static bool? GetNullableBool(IDictionary? arguments, string key) + public static double? GetDoubleStrict(IDictionary? arguments, string key) { - if (!TryGetValueFlexible(arguments, key, out var value) || value is null) + if (!TryGetValueFlexible(arguments, key, out var value) || IsAbsent(value)) return null; - return value switch + return TryCoerceDouble(value, out var result) ? result : throw InvalidValue(key, value!, "number"); + } + + public static bool? GetBoolStrict(IDictionary? arguments, string key) + { + if (!TryGetValueFlexible(arguments, key, out var value) || IsAbsent(value)) + return null; + + return TryCoerceBool(value, out var result) ? result : throw InvalidValue(key, value!, "boolean"); + } + + /// + /// Renders a loose argument value for a model-facing error message, + /// bounded so a pathological multi-KB value cannot flood the result. + /// Shared so every "invalid value" surface renders identically. + /// + public static string RenderValue(object? value, int maxLength = 100) + { + var rendered = value switch { - bool b => b, - JsonElement { ValueKind: JsonValueKind.True } => true, - JsonElement { ValueKind: JsonValueKind.False } => false, - string s when bool.TryParse(s, out var parsed) => parsed, - JsonElement { ValueKind: JsonValueKind.String } je when bool.TryParse(je.GetString(), out var parsed) => parsed, - _ => null + null => "null", + JsonElement je => je.GetRawText(), + _ => value.ToString() ?? string.Empty }; + + return rendered.Length > maxLength ? rendered[..maxLength] + "…" : rendered; } + + private static ArgumentException InvalidValue(string key, object value, string expectedType) + => new($"Parameter '{key}' value '{RenderValue(value)}' is not a valid {expectedType}. The tool was NOT executed."); } diff --git a/src/Netclaw.Tools.Abstractions/ToolArgumentValidator.cs b/src/Netclaw.Tools.Abstractions/ToolArgumentValidator.cs new file mode 100644 index 000000000..c6802bbd8 --- /dev/null +++ b/src/Netclaw.Tools.Abstractions/ToolArgumentValidator.cs @@ -0,0 +1,193 @@ +// ----------------------------------------------------------------------- +// +// Copyright (C) 2026 - 2026 Petabridge, LLC +// +// ----------------------------------------------------------------------- +using System.Collections.Concurrent; +using System.Text; +using System.Text.Json; + +namespace Netclaw.Tools; + +/// +/// Validates LLM-supplied argument keys against a native tool's declared +/// surface before execution (tool-arg-validation spec). A key is recognized +/// iff it would actually be consumed downstream: declared parameters match +/// exactly or via (mirroring +/// the flexible binding in ), while meta keys +/// (_-prefixed) match exactly only (mirroring exact extraction in +/// ToolCallMeta.ExtractFrom). Unrecognized keys reject the call with a +/// "did you mean" suggestion — fuzzy matching generates suggestion text ONLY, +/// never acceptance: the LLM resolves ambiguity by re-issuing explicitly. +/// +public static class ToolArgumentValidator +{ + private sealed record RecognizedKeys( + HashSet Exact, + Dictionary NormalizedDeclared, + string[] MetaKeys, + string[] ValidNames); + + private static readonly ConcurrentDictionary Cache = new(); + + /// + /// Validates the supplied argument keys for . + /// Returns null when all keys are recognized; otherwise a model-facing + /// error string (the call must not execute). + /// + public static string? ValidateArgumentKeys(INetclawTool tool, IDictionary? arguments) + { + if (arguments is null || arguments.Count == 0) + return null; + + var recognized = Cache.GetOrAdd(tool.GetType(), _ => BuildRecognizedKeys(tool)); + if (recognized is null) + return null; // schema exposes no property list — nothing to validate against + + List<(string Key, string? Suggestion)>? unknown = null; + foreach (var key in arguments.Keys) + { + if (recognized.Exact.Contains(key)) + continue; + + // Flexible recognition applies to declared params only: binding + // consumes case/punctuation variants of declared names, but meta + // extraction is exact-match, so a near-miss meta key would NOT be + // consumed and must be rejected here. + var normalized = ToolArgumentHelper.NormalizeKey(key); + if (recognized.NormalizedDeclared.ContainsKey(normalized)) + continue; + + unknown ??= []; + unknown.Add((key, SuggestFor(key, normalized, recognized))); + } + + if (unknown is null) + return null; + + var sb = new StringBuilder("Error: "); + foreach (var (key, suggestion) in unknown) + { + sb.Append($"Unrecognized argument '{key}' for tool '{tool.Name}'."); + if (suggestion is not null) + sb.Append($" Did you mean '{suggestion}'?"); + sb.Append(' '); + } + + sb.Append("The tool was NOT executed. Valid arguments: "); + sb.Append(string.Join(", ", recognized.ValidNames)); + sb.Append('.'); + return sb.ToString(); + } + + private static RecognizedKeys? BuildRecognizedKeys(INetclawTool tool) + { + JsonElement props; + try + { + if (!tool.ParameterSchema.TryGetProperty("properties", out props) + || props.ValueKind != JsonValueKind.Object) + return null; + } + catch (InvalidOperationException) + { + return null; // schema is not an object (defensive; native generated schemas always are) + } + + var exact = new HashSet(StringComparer.Ordinal); + var normalizedDeclared = new Dictionary(StringComparer.OrdinalIgnoreCase); + var meta = new List(); + var names = new List(); + + foreach (var prop in props.EnumerateObject()) + { + var name = prop.Name; + exact.Add(name); + names.Add(name); + + if (name.StartsWith('_')) + meta.Add(name); + else + normalizedDeclared[ToolArgumentHelper.NormalizeKey(name)] = name; + } + + // A key binding would consume via an interchangeable alias (text↔message) + // must be recognized too, or validation rejects calls binding accepts. + // The alias groups live in ToolArgumentHelper so binding and validation + // share one definition (bidirectional: declaring either member accepts + // the other). + foreach (var declared in normalizedDeclared.Keys.ToArray()) + { + foreach (var alias in ToolArgumentHelper.NormalizedAliasesFor(declared)) + normalizedDeclared.TryAdd(alias, normalizedDeclared[declared]); + } + + return new RecognizedKeys(exact, normalizedDeclared, [.. meta], [.. names]); + } + + private static string? SuggestFor(string key, string normalizedKey, RecognizedKeys recognized) + { + // A key that canonicalizes to a meta key (TimeoutSeconds, timeout_seconds, + // _timeoutSeconds → _timeout_seconds) is the highest-confidence near-miss. + foreach (var metaKey in recognized.MetaKeys) + { + if (string.Equals( + ToolArgumentHelper.NormalizeKey(metaKey), normalizedKey, + StringComparison.OrdinalIgnoreCase)) + return metaKey; + } + + string? best = null; + var bestDistance = 3; // suggest only within edit distance 2 + foreach (var name in recognized.ValidNames) + { + var distance = BoundedLevenshtein( + normalizedKey, + ToolArgumentHelper.NormalizeKey(name), + maxDistance: 2); + if (distance < bestDistance) + { + bestDistance = distance; + best = name; + } + } + + return best; + } + + /// + /// Levenshtein distance with early exit once + /// is exceeded (returns maxDistance + 1 in that case). + /// + private static int BoundedLevenshtein(string a, string b, int maxDistance) + { + if (Math.Abs(a.Length - b.Length) > maxDistance) + return maxDistance + 1; + + var previous = new int[b.Length + 1]; + var current = new int[b.Length + 1]; + for (var j = 0; j <= b.Length; j++) + previous[j] = j; + + for (var i = 1; i <= a.Length; i++) + { + current[0] = i; + var rowMin = current[0]; + for (var j = 1; j <= b.Length; j++) + { + var cost = char.ToLowerInvariant(a[i - 1]) == char.ToLowerInvariant(b[j - 1]) ? 0 : 1; + current[j] = Math.Min( + Math.Min(current[j - 1] + 1, previous[j] + 1), + previous[j - 1] + cost); + rowMin = Math.Min(rowMin, current[j]); + } + + if (rowMin > maxDistance) + return maxDistance + 1; + + (previous, current) = (current, previous); + } + + return previous[b.Length] <= maxDistance ? previous[b.Length] : maxDistance + 1; + } +} diff --git a/src/Netclaw.Tools.Abstractions/ToolCallArgumentErrors.cs b/src/Netclaw.Tools.Abstractions/ToolCallArgumentErrors.cs new file mode 100644 index 000000000..022561af8 --- /dev/null +++ b/src/Netclaw.Tools.Abstractions/ToolCallArgumentErrors.cs @@ -0,0 +1,26 @@ +// ----------------------------------------------------------------------- +// +// Copyright (C) 2026 - 2026 Petabridge, LLC +// +// ----------------------------------------------------------------------- +namespace Netclaw.Tools; + +/// +/// Wire-level markers for tool-call argument failures detected at the provider +/// boundary. When a model emits a tool call whose arguments JSON cannot be +/// deserialized, the provider attaches instead +/// of dispatching null arguments; the session pipeline detects the sentinel +/// before meta extraction and rejects the call id with a model-facing error +/// (tool-arg-validation spec). The raw payload needed for a useful error is +/// only available at the provider, which is why the failure travels with the +/// call rather than being reconstructed downstream. +/// +public static class ToolCallArgumentErrors +{ + /// + /// Sentinel argument key carrying the parse-failure detail. Never collides + /// with validation: the pipeline consumes it before the unknown-key gate, + /// and it appears in no tool schema, so a leak is rejected loudly anyway. + /// + public const string ArgsParseErrorKey = "__netclaw_args_parse_error"; +} diff --git a/src/Netclaw.Tools.Abstractions/ToolCallMeta.cs b/src/Netclaw.Tools.Abstractions/ToolCallMeta.cs index 947dd284b..86706514e 100644 --- a/src/Netclaw.Tools.Abstractions/ToolCallMeta.cs +++ b/src/Netclaw.Tools.Abstractions/ToolCallMeta.cs @@ -79,31 +79,26 @@ public static (ToolCallMeta? Meta, IDictionary? CleanArgs) Extr if (arguments.TryGetValue("_timeout_seconds", out var tVal) && tVal is not null) { - timeoutSeconds = tVal switch + // Shares ToolArgumentHelper.TryCoerceInt with ValidateMetaValues so + // acceptance here and rejection there cannot drift. A positive int + // is a valid hint; anything else (including 0/negative) is left null + // and validation rejects it loudly before dispatch. + if (ToolArgumentHelper.TryCoerceInt(tVal, out var parsedTimeout) && parsedTimeout > 0) { - int i when i > 0 => i, - long l when l > 0 => (int)l, - double d when d > 0 => (int)d, - JsonElement { ValueKind: JsonValueKind.Number } je when je.GetInt32() > 0 => je.GetInt32(), - string s when int.TryParse(s, out var parsed) && parsed > 0 => parsed, - _ => null - }; - if (timeoutSeconds.HasValue) + timeoutSeconds = parsedTimeout; hasAnyMeta = true; + } } if (arguments.TryGetValue("_background", out var bVal) && bVal is not null) { - background = bVal switch + // Shares ToolArgumentHelper.TryCoerceBool with ValidateMetaValues. + if (ToolArgumentHelper.TryCoerceBool(bVal, out var parsedBackground)) { - bool b => b, - JsonElement { ValueKind: JsonValueKind.True } => true, - JsonElement { ValueKind: JsonValueKind.False } => false, - string s when bool.TryParse(s, out var parsed) => parsed, - _ => false - }; - if (background) - hasAnyMeta = true; + background = parsedBackground; + if (background) + hasAnyMeta = true; + } } if (!hasAnyMeta) diff --git a/src/Netclaw.Tools.Abstractions/ToolExecutionContext.cs b/src/Netclaw.Tools.Abstractions/ToolExecutionContext.cs index 35430131c..5292eace0 100644 --- a/src/Netclaw.Tools.Abstractions/ToolExecutionContext.cs +++ b/src/Netclaw.Tools.Abstractions/ToolExecutionContext.cs @@ -92,6 +92,7 @@ public ToolExecutionContext(string? sessionId, string? sessionDirectory) /// public int? RequestedTimeoutSeconds { get; set; } + public string? ChannelType { get; set; } /// diff --git a/src/Netclaw.Tools.Generators/NetclawToolGenerator.cs b/src/Netclaw.Tools.Generators/NetclawToolGenerator.cs index 1b8f83d32..190a3c7b3 100644 --- a/src/Netclaw.Tools.Generators/NetclawToolGenerator.cs +++ b/src/Netclaw.Tools.Generators/NetclawToolGenerator.cs @@ -247,45 +247,48 @@ private static void GenerateSource(SourceProductionContext spc, ToolModel model) } else if (p.JsonType == "integer") { + // Strict variants throw on present-but-invalid values, so the + // null-coalesce arms below only apply a default for a genuinely + // absent parameter (tool-arg-validation spec). if (p.IsNullable) - sb.AppendLine($" var __{p.Name} = Netclaw.Tools.ToolArgumentHelper.GetNullableInt(arguments, \"{p.Name}\");"); + sb.AppendLine($" var __{p.Name} = Netclaw.Tools.ToolArgumentHelper.GetIntStrict(arguments, \"{p.Name}\");"); else if (p.IsRequired) { - sb.AppendLine($" var __{p.Name}_raw = Netclaw.Tools.ToolArgumentHelper.GetNullableInt(arguments, \"{p.Name}\");"); + sb.AppendLine($" var __{p.Name}_raw = Netclaw.Tools.ToolArgumentHelper.GetIntStrict(arguments, \"{p.Name}\");"); sb.AppendLine($" if (__{p.Name}_raw is null)"); sb.AppendLine($" throw new System.ArgumentException(\"Required parameter '{p.Name}' is missing.\");"); sb.AppendLine($" var __{p.Name} = __{p.Name}_raw.Value;"); } else - sb.AppendLine($" var __{p.Name} = Netclaw.Tools.ToolArgumentHelper.GetNullableInt(arguments, \"{p.Name}\") ?? 0;"); + sb.AppendLine($" var __{p.Name} = Netclaw.Tools.ToolArgumentHelper.GetIntStrict(arguments, \"{p.Name}\") ?? 0;"); } else if (p.JsonType == "number") { if (p.IsNullable) - sb.AppendLine($" var __{p.Name} = Netclaw.Tools.ToolArgumentHelper.GetNullableDouble(arguments, \"{p.Name}\");"); + sb.AppendLine($" var __{p.Name} = Netclaw.Tools.ToolArgumentHelper.GetDoubleStrict(arguments, \"{p.Name}\");"); else if (p.IsRequired) { - sb.AppendLine($" var __{p.Name}_raw = Netclaw.Tools.ToolArgumentHelper.GetNullableDouble(arguments, \"{p.Name}\");"); + sb.AppendLine($" var __{p.Name}_raw = Netclaw.Tools.ToolArgumentHelper.GetDoubleStrict(arguments, \"{p.Name}\");"); sb.AppendLine($" if (__{p.Name}_raw is null)"); sb.AppendLine($" throw new System.ArgumentException(\"Required parameter '{p.Name}' is missing.\");"); sb.AppendLine($" var __{p.Name} = __{p.Name}_raw.Value;"); } else - sb.AppendLine($" var __{p.Name} = Netclaw.Tools.ToolArgumentHelper.GetNullableDouble(arguments, \"{p.Name}\") ?? 0.0;"); + sb.AppendLine($" var __{p.Name} = Netclaw.Tools.ToolArgumentHelper.GetDoubleStrict(arguments, \"{p.Name}\") ?? 0.0;"); } else if (p.JsonType == "boolean") { if (p.IsNullable) - sb.AppendLine($" var __{p.Name} = Netclaw.Tools.ToolArgumentHelper.GetNullableBool(arguments, \"{p.Name}\");"); + sb.AppendLine($" var __{p.Name} = Netclaw.Tools.ToolArgumentHelper.GetBoolStrict(arguments, \"{p.Name}\");"); else if (p.IsRequired) { - sb.AppendLine($" var __{p.Name}_raw = Netclaw.Tools.ToolArgumentHelper.GetNullableBool(arguments, \"{p.Name}\");"); + sb.AppendLine($" var __{p.Name}_raw = Netclaw.Tools.ToolArgumentHelper.GetBoolStrict(arguments, \"{p.Name}\");"); sb.AppendLine($" if (__{p.Name}_raw is null)"); sb.AppendLine($" throw new System.ArgumentException(\"Required parameter '{p.Name}' is missing.\");"); sb.AppendLine($" var __{p.Name} = __{p.Name}_raw.Value;"); } else - sb.AppendLine($" var __{p.Name} = Netclaw.Tools.ToolArgumentHelper.GetNullableBool(arguments, \"{p.Name}\") ?? false;"); + sb.AppendLine($" var __{p.Name} = Netclaw.Tools.ToolArgumentHelper.GetBoolStrict(arguments, \"{p.Name}\") ?? false;"); } }