Loud tool-argument validation: eliminate silent discard/degradation of LLM tool arguments#1398
Merged
Aaronontheweb merged 8 commits intoJun 12, 2026
Conversation
Audit of the tool-call argument surface found 17 silent-discard/degrade sites sharing three mechanisms: unknown keys dropped without signal, present-but-invalid values coerced to defaults, and requested values silently clamped or overridden. Origin: production session D0AC6CKBK5K_1781115410_840529, where "TimeoutSeconds":"1200" was silently dropped (only _timeout_seconds is recognized), the shell tool fell back to its 90s default, and the agent's delegation call was killed mid-flight, feeding a stuck loop. The OpenSpec change (proposal, design, spec deltas, tasks) establishes a no-silent-discard invariant at the tool-arg seam: unknown keys reject with did-you-mean suggestions (fuzzy matching generates suggestion text only, never acceptance), invalid values reject instead of coercing, and every clamp/override emits a model-facing notice. Policy-layer findings are parked as open questions for a security owner.
Implements the loud-tool-arg-validation OpenSpec change: LLM-supplied
tool arguments are never silently discarded, coerced, or overridden.
- Unknown-key validation at DispatchingToolExecutor (sync + streaming):
unrecognized argument keys reject the call before execution with a
"did you mean" suggestion and the valid argument names. Recognition
mirrors actual consumption semantics — flexible (case/punctuation)
for declared parameters, exact-only for meta keys. Fuzzy matching
generates suggestion text only, never acceptance. MCP tools exempt
(server-side schema validation is authoritative).
- Strict value binding: new GetIntStrict/GetDoubleStrict/GetBoolStrict
helpers distinguish absent from present-but-invalid; the tool source
generator emits them so invalid values reject instead of coercing to
0/0.0/false. Non-integral numerics no longer silently truncate, and
non-integral/overflow JSON numbers no longer throw uncaught.
- Meta-value validation: malformed _timeout_seconds/_background values
reject pre-dispatch (pipeline-side; persisted ToolCallMeta unchanged).
- Override notices: timeout ceiling clamps and below-floor requests now
surface in the tool result (appended post-bounding so they cannot be
spilled away), steering to _background:true for longer work.
- Provider boundary: malformed tool-call arguments JSON travels as a
sentinel and is rejected per call id instead of dispatching the tool
with null arguments; deterministic on persistence re-drive.
- web_fetch: Format validated against {raw, text} (no silent raw
fallback); 5 MB response-cap truncation is surfaced in the summary.
- list_webhooks: the schema-advertised Filter parameter is honored
(active/all) instead of being a silent no-op.
- netclaw-operations skill v2.12.0: timeout-clamp and background-job
delegation guidance, strict argument naming rules.
- New eval case tool_timeout_arg_recovery (passed 3/3 against
Qwen3.6-35B on self-hosted vLLM); full suite 4,998 tests green.
Origin: production session D0AC6CKBK5K_1781115410_840529, where
"TimeoutSeconds":"1200" was silently dropped and the shell timeout
fell back to a 90s default, killing a delegation call mid-flight.
The test asserted a valid ShellTool call passed validation but never touched an McpToolAdapter, leaving the "MCP tool exempt from native key validation" spec scenario without real coverage. It now registers an McpToolAdapter, dispatches a call with an unknown argument key, and asserts the native "Unrecognized argument" rejection does not fire — the unknown-key gate runs before authorization, so any other outcome proves the exemption.
Sync delta specs to main specs: - New capability spec tool-arg-validation (unknown-key rejection with suggestion-only near-miss matching, present-but-invalid value rejection, malformed args-JSON handling, override notices) - tool-call-metadata: per-call timeout hint now surfaces clamp/floor overrides in the tool result; new malformed-meta-value rejection requirement - netclaw-tools: web_fetch format validation + response-cap truncation notice; list_webhooks filter honored Archive change to openspec/changes/archive/2026-06-11-loud-tool-arg-validation (26/26 tasks, verification clean, eval suite 7/7).
Correctness: - Accept integral JSON numbers written with a decimal point (300.0, 12.0) as integers; JsonElement.TryGetInt32 rejects that text, so coercion now falls back to an integral-double check (12.5 still rejected). Models commonly emit whole numbers as N.0, which previously hit a spurious "not a valid integer" rejection loop. - Parse string-typed numeric arguments with InvariantCulture. The daemon runs with InvariantGlobalization=false, so a comma-decimal host locale previously misparsed or falsely rejected values like "1.5". - Consolidate all pre-dispatch validation (provider args-parse sentinel, present-but-invalid meta values, unrecognized keys) into IToolExecutor.ValidateToolCall. The sub-agent loop calls the executor directly and previously skipped meta-value validation and the sentinel check — the silent-fallback gap this feature exists to close. - Audit unknown-argument rejections as Allowed=false: the pipeline now preflights via ValidateToolCall instead of running the executor and mislogging the rejected call as executed. - Accept the text<->Message parameter alias bidirectionally so a send_channel_message call using "Message" (which binding consumes) is no longer rejected. Alias defined once in ToolArgumentHelper. - Clamp background-job timeouts to MaxToolTimeoutSeconds so _background cannot bypass the ceiling (was an unbounded kill timer). - Bound the args-parse sentinel value (200 chars) so a forged/oversized value cannot flood the tool result. Cleanup: - One shared TryCoerceInt/Double/Bool + RenderValue across the strict binders, ToolCallMeta.ExtractFrom, and ValidateMetaValues (was three hand-mirrored ladders that had to stay in lockstep by hand). - Delete the now-unused GetNullableInt/Double/Bool helpers. - web_fetch streams into a pooled growing buffer: no eager 5 MB allocation per fetch, no 5 MB slice-copy on truncation. Full suite 5,004 tests green; slopwatch clean; headers verified.
…ad-write-build-loop # Conflicts: # feeds/skills/.system/files/netclaw-operations/SKILL.md
Aaronontheweb
commented
Jun 12, 2026
Aaronontheweb
left a comment
Collaborator
Author
There was a problem hiding this comment.
Found an immediate design issue that needs to be cleaned up - still working through the rest
| /// to <c>channelid</c>. Shared with <see cref="ToolArgumentValidator"/> so | ||
| /// key recognition mirrors the exact matching that binding performs. | ||
| /// </summary> | ||
| public static string NormalizeKey(string key) |
Collaborator
Author
There was a problem hiding this comment.
strip out special characters and whitespace - do this for both the tool name and the input
Per review: a per-call timeout is the agent's judgement, not something to override. Clamping a requested _timeout_seconds down to a ceiling (and flooring it up to a default) was friction with no clear benefit — the floor especially (a shorter timeout is strictly safer), and the silent version was the original fallback-rule violation this PR set out to fix. - _timeout_seconds is now honored exactly: no ceiling, no floor, no notice. Absent → the inherited SessionConfig.ToolExecutionTimeout default applies. - Background-job path honors the requested timeout too (was clamped). - Collapse the conflicting timeout sources: remove ToolConfig.MaxToolTimeoutSeconds (the ceiling) and ToolConfig.ShellTimeoutSeconds (a 60s default shadowed in production by the 90s pipeline default). Single source of truth is SessionConfig.ToolExecutionTimeout; ShellTool keeps a const fallback only for the no-pipeline-context path. Both properties removed from netclaw-config.v1.schema.json (doctor --fix strips them from existing configs via additionalProperties:false). - Delete the now-dead ToolExecutionContext.Notices mechanism (the clamp notice was its only producer) and ComputeEffectiveTimeout. - Update tool-call-metadata spec, background-jobs runbook, configuration doc, and netclaw-operations skill (v2.12.1) to honor-the-hint semantics. Tests updated: timeout hints honored exactly (high + low), background honors requested timeout, ShellTool timeout driven via context. Core suites green (Actors 2386, Configuration 416, Daemon 734, Cli 802); slopwatch + headers clean; specs validate.
NuGetAudit flags MessagePack 2.5.192 (GHSA-hv8m-jj95-wg3x — LZ4 decompression DoS), pulled in transitively only by the Aspire demo samples: Aspire.Hosting.AppHost → StreamJsonRpc 2.22.23 → MessagePack 2.5.192 Netclaw uses MessagePack in no production code, and the affected LZ4 path is not exercised by Aspire's local tooling RPC — but the advisory DB update turned it into a warning-as-error that fails pr_validation's solution build (dev is affected identically). Pin to the patched v2 (2.5.301) via central package management + a direct reference in the two sample projects to promote the transitive. Same minor branch, no API risk for StreamJsonRpc.
Aaronontheweb
commented
Jun 12, 2026
Aaronontheweb
left a comment
Collaborator
Author
There was a problem hiding this comment.
LGTM - we'll have to see how it performs during beta testing. Passed evals with flying colors.
| } | ||
|
|
||
| [Fact] | ||
| public async Task Integral_decimal_json_timeout_accepted_and_executes() |
Collaborator
Author
There was a problem hiding this comment.
LGTM - tolerant reader stuff
| 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_number_surfaces_parse_error_not_truncation() |
| } | ||
|
|
||
| [Fact] | ||
| public async Task Valid_string_number_still_binds() |
| [Theory] | ||
| [InlineData("yes")] | ||
| [InlineData("1")] | ||
| public void BoolStrict_colloquial_string_throws(string value) |
Collaborator
Author
There was a problem hiding this comment.
We require true or false only.
| if (arguments.TryGetValue("_timeout_seconds", out var tVal) && tVal is not null) | ||
| { | ||
| timeoutSeconds = tVal switch | ||
| // Shares ToolArgumentHelper.TryCoerceInt with ValidateMetaValues so |
| { | ||
| background = bVal switch | ||
| // Shares ToolArgumentHelper.TryCoerceBool with ValidateMetaValues. | ||
| if (ToolArgumentHelper.TryCoerceBool(bVal, out var parsedBackground)) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
LLM-supplied tool arguments could be silently discarded, coerced, or overridden with no signal to the agent. Origin: production session
D0AC6CKBK5K_1781115410_840529, where the agent passed"TimeoutSeconds":"1200"to extend a shell timeout, but Netclaw only recognizes the meta key_timeout_seconds. The key was silently dropped, the timeout fell back to a default, the agent's delegation call was killed mid-flight, and the agent — believing it had set a generous timeout — spun in a stuck loop.A follow-up audit (
SILENT_FALLBACK_AUDIT.md) found 17 sites across the tool-call surface sharing three mechanisms. The config surface already enforces this discipline (additionalProperties:false+ConfigSchemaDoctorCheck); the live tool-argument surface had no equivalent. This PR closes that gap.What changes
_timeout_seconds?" suggestion. Fuzzy matching generates the suggestion text only — it never accepts a near-miss key (the LLM re-issues explicitly).0/false/null. Absent optional params keep their documented defaults.web_fetchvalidatesFormat(no silent raw fallback) and surfaces 5 MB truncation;list_webhookshonors its previously-deadFilterparameter.Allowed=false.netclaw-operationssystem skill updated (timeout-clamp +_backgroundguidance, strict argument naming).Behavior change: calls that previously "succeeded" by silently dropping arguments now error. Well-formed calls are unaffected.
Out of scope (parked for a security owner)
Policy-layer audit findings: audience allowlist non-authority (
ToolAudienceProfileResolver), null path-token fail-closed in shell trust zones, safe-verb auto-grant audit logging. Also deferred: a typed out-of-band channel for the args-parse signal (currently an in-band sentinel key, blast radius bounded), and agent-loop / no-progress detection (separate workstream, same originating incident).Validation
loud-tool-arg-validation(proposal/design/specs/tasks), synced to main specs and archived.dotnet slopwatch analyzeclean; copyright headers verified.tool_timeout_arg_recoverycase (validated against Qwen3.6-35B on self-hosted vLLM — the same model class as the originating incident)./code-review highpass surfaced 10 findings; all are fixed in this branch (final commit), with regression tests.Traceability
PRD-001 (MVP tool surface), PRD-002 (gateway security envelope); constitution "No silent fallbacks". Origin incident: memorizer memory
e9a72b27-72d9-4e98-aad7-4d970ce52ecf.