diff --git a/evals/run-evals.sh b/evals/run-evals.sh index 3f1c4a2df..eedcbf2f1 100755 --- a/evals/run-evals.sh +++ b/evals/run-evals.sh @@ -1030,10 +1030,11 @@ assert_tool_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. + # Spelling-tolerant meta keys: a near-miss timeout key (TimeoutSeconds, + # timeout_seconds, Timeout) now resolves onto _timeout_seconds and is + # consumed directly — no rejection round-trip needed. If the model instead + # emits the canonical key, that works too. Either way the command running is + # the proof the timeout hint was honored, not dropped. stdout_contains '\[tool:call\] shell_execute' \ && stdout_contains 'netclaw-timeout-eval-ok' } diff --git a/feeds/skills/.system/files/netclaw-operations/SKILL.md b/feeds/skills/.system/files/netclaw-operations/SKILL.md index 3750d9a27..d858aa728 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.14.0" + version: "2.15.0" --- # Netclaw Operations @@ -310,15 +310,24 @@ 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. +Prefer the canonical argument names exactly as a tool declares them, and the +canonical meta keys `_rationale`, `_timeout_seconds`, `_background` (leading +underscore, snake_case). Recognition is spelling-tolerant so a near-miss is +consumed rather than dropped: declared params fold case/punctuation, and the +meta keys also accept the underscore-dropped/cased/shortened forms +(`TimeoutSeconds`, `timeout_seconds`, `Timeout` → the timeout hint; `Rationale`, +`Background` likewise). The supplied value is always *used* — never silently +defaulted. + +Three things are still rejected loudly, and when rejected the tool did NOT run — +fix and re-issue once, do not retry the same shape: + +- **Unknown keys** — a key that matches no parameter and no meta field rejects + with a `did you mean ''?` suggestion and the list of valid names. +- **Invalid values** — a value that cannot parse as its type (`_timeout_seconds: + "1200ms"`, `_background: "yes"`) rejects instead of falling back to a default. +- **Ambiguous meta spelling** — supplying two keys that map to the same meta + field (e.g. both `_timeout_seconds` and `TimeoutSeconds`) rejects; send one. ## Large tool output diff --git a/src/Netclaw.Actors.Tests/Protocol/ChatMessageConverterTests.cs b/src/Netclaw.Actors.Tests/Protocol/ChatMessageConverterTests.cs index 2177b547c..e4594dd94 100644 --- a/src/Netclaw.Actors.Tests/Protocol/ChatMessageConverterTests.cs +++ b/src/Netclaw.Actors.Tests/Protocol/ChatMessageConverterTests.cs @@ -604,6 +604,68 @@ private static byte[] SmallPng(int size = 16) return data.ToArray(); } + [Fact] + public void FromAiMessage_with_interpreter_persists_schema_aware_meta() + { + var aiMsg = new AiChatMessage(AiChatRole.Assistant, new List + { + new FunctionCallContent("c1", "shell_execute", new Dictionary + { + ["Command"] = "ls", + ["TimeoutSeconds"] = 600 // ChatGPT-style near-miss + }) + }); + + // With the schema-aware interpreter (what LlmSessionActor passes), the + // near-miss is stripped from persisted args and captured in MetaJson — + // so recorded history matches what the runtime actually executes. + var withInterp = ChatMessageConverter.FromAiMessage(aiMsg, interpretToolCall: tc => + ToolCallMeta.ExtractFrom(tc.Arguments, ToolArgumentHelper.ResolveMetaField)); + var call = Assert.Single(withInterp.ToolCalls); + Assert.DoesNotContain("TimeoutSeconds", call.ArgumentsJson); + Assert.NotNull(call.MetaJson); + Assert.Contains("600", call.MetaJson!); + + // Without an interpreter (schema-blind default = exact), the near-miss is + // left raw and no meta is captured — the safe fallback for callers/replay. + var exact = ChatMessageConverter.FromAiMessage(aiMsg); + var exactCall = Assert.Single(exact.ToolCalls); + Assert.Contains("TimeoutSeconds", exactCall.ArgumentsJson); + Assert.Null(exactCall.MetaJson); + } + + [Fact] + public void ToAiMessage_reinjectMeta_restores_hint_for_redrive_but_not_outbound() + { + // Persist a near-miss call: the meta key is stripped into MetaJson. + var persisted = ChatMessageConverter.FromAiMessage( + new AiChatMessage(AiChatRole.Assistant, new List + { + new FunctionCallContent("c1", "shell_execute", new Dictionary + { + ["Command"] = "long-task", + ["TimeoutSeconds"] = 1800 + }) + }), + interpretToolCall: tc => ToolCallMeta.ExtractFrom(tc.Arguments, ToolArgumentHelper.ResolveMetaField)); + var stored = Assert.Single(persisted.ToolCalls); + Assert.DoesNotContain("TimeoutSeconds", stored.ArgumentsJson); + + // Re-drive reconstruction re-injects the canonical key, so extraction + // reapplies the 1800s hint on re-dispatch (regression guard for the + // previously-dropped hint on post-approval re-drive). + var redriven = ChatMessageConverter.ToAiMessage(persisted, reinjectMeta: true); + var redrivenCall = redriven.Contents.OfType().Single(); + Assert.True(redrivenCall.Arguments!.ContainsKey("_timeout_seconds")); + var (meta, _) = ToolCallMeta.ExtractFrom(redrivenCall.Arguments, ToolArgumentHelper.ResolveMetaField); + Assert.Equal(1800, meta?.TimeoutHintSeconds); + + // Outbound provider history (the default) must NOT carry meta keys. + var outbound = ChatMessageConverter.ToAiMessage(persisted); + var outboundCall = outbound.Contents.OfType().Single(); + Assert.False(outboundCall.Arguments?.ContainsKey("_timeout_seconds") ?? false); + } + private sealed class TempSessionDir : IDisposable { public string Path { get; } = System.IO.Path.Combine( diff --git a/src/Netclaw.Actors.Tests/Sessions/Pipelines/ToolCallMetaExtractorTests.cs b/src/Netclaw.Actors.Tests/Sessions/Pipelines/ToolCallMetaExtractorTests.cs index ef0d1b343..65b0bec60 100644 --- a/src/Netclaw.Actors.Tests/Sessions/Pipelines/ToolCallMetaExtractorTests.cs +++ b/src/Netclaw.Actors.Tests/Sessions/Pipelines/ToolCallMetaExtractorTests.cs @@ -25,7 +25,7 @@ public void Extract_WithAllMetaFields_ReturnsMetaAndCleanArgs() }; var tc = new FunctionCallContent("call-1", "shell_execute", args); - var (meta, cleaned) = ToolCallMetaExtractor.Extract(tc); + var (meta, cleaned) = ToolCallMetaExtractor.Extract(tc, ToolArgumentHelper.ResolveMetaField); Assert.NotNull(meta); Assert.Equal("running tests", meta!.Rationale); @@ -48,7 +48,7 @@ public void Extract_WithOnlyRationale_ReturnsMetaWithRationaleOnly() }; var tc = new FunctionCallContent("call-1", "web_search", args); - var (meta, cleaned) = ToolCallMetaExtractor.Extract(tc); + var (meta, cleaned) = ToolCallMetaExtractor.Extract(tc, ToolArgumentHelper.ResolveMetaField); Assert.NotNull(meta); Assert.Equal("searching docs", meta!.Rationale); @@ -65,7 +65,7 @@ public void Extract_WithNoMetaFields_ReturnsNullMeta() }; var tc = new FunctionCallContent("call-1", "shell_execute", args); - var (meta, cleaned) = ToolCallMetaExtractor.Extract(tc); + var (meta, cleaned) = ToolCallMetaExtractor.Extract(tc, ToolArgumentHelper.ResolveMetaField); Assert.Null(meta); Assert.Same(tc, cleaned); @@ -76,7 +76,7 @@ public void Extract_WithNullArguments_ReturnsNullMeta() { var tc = new FunctionCallContent("call-1", "shell_execute", null); - var (meta, cleaned) = ToolCallMetaExtractor.Extract(tc); + var (meta, cleaned) = ToolCallMetaExtractor.Extract(tc, ToolArgumentHelper.ResolveMetaField); Assert.Null(meta); Assert.Same(tc, cleaned); @@ -91,7 +91,7 @@ public void Extract_HandlesJsonElementValues() args[prop.Name] = prop.Value; var tc = new FunctionCallContent("call-1", "shell_execute", args); - var (meta, cleaned) = ToolCallMetaExtractor.Extract(tc); + var (meta, cleaned) = ToolCallMetaExtractor.Extract(tc, ToolArgumentHelper.ResolveMetaField); Assert.NotNull(meta); Assert.Equal("from json", meta!.Rationale); @@ -109,7 +109,7 @@ public void Extract_TimeoutSeconds_ZeroNotTreatedAsMeta() }; var tc = new FunctionCallContent("call-1", "shell_execute", args); - var (meta, _) = ToolCallMetaExtractor.Extract(tc); + var (meta, _) = ToolCallMetaExtractor.Extract(tc, ToolArgumentHelper.ResolveMetaField); // Zero timeout is not meaningful — should not produce meta Assert.Null(meta); @@ -133,7 +133,7 @@ public void Extract_TimeoutAlone_DoesNotTriggerBackground() }; var tc = new FunctionCallContent("call-1", "shell_execute", args); - var (meta, _) = ToolCallMetaExtractor.Extract(tc); + var (meta, _) = ToolCallMetaExtractor.Extract(tc, ToolArgumentHelper.ResolveMetaField); Assert.NotNull(meta); Assert.False(meta!.Background); @@ -150,9 +150,118 @@ public void Extract_BackgroundFalse_DoesNotSetBackground() }; var tc = new FunctionCallContent("call-1", "shell_execute", args); - var (meta, _) = ToolCallMetaExtractor.Extract(tc); + var (meta, _) = ToolCallMetaExtractor.Extract(tc, ToolArgumentHelper.ResolveMetaField); Assert.NotNull(meta); Assert.False(meta!.Background); } + + // ── ChatGPT-style meta naming (Qwen) is consumed, not dropped ── + + [Fact] + public void Extract_MisnamedMetaFields_ConsumedAndStripped() + { + // The names Qwen emits: underscore dropped, capitalized, and the + // shortened "Timeout". All resolve onto the canonical fields and are + // removed from the args the tool binder sees. + var args = new Dictionary + { + ["Command"] = "dotnet test", + ["Rationale"] = "running tests", + ["TimeoutSeconds"] = "1200", + ["Background"] = true + }; + var tc = new FunctionCallContent("call-1", "shell_execute", args); + + var (meta, cleaned) = ToolCallMetaExtractor.Extract(tc, ToolArgumentHelper.ResolveMetaField); + + Assert.NotNull(meta); + Assert.Equal("running tests", meta!.Rationale); + Assert.Equal(1200, meta.TimeoutHintSeconds); + Assert.True(meta.Background); + + var cleanArgs = (IDictionary)cleaned.Arguments!; + Assert.Contains("Command", cleanArgs); + Assert.DoesNotContain("Rationale", cleanArgs); + Assert.DoesNotContain("TimeoutSeconds", cleanArgs); + Assert.DoesNotContain("Background", cleanArgs); + } + + [Fact] + public void Extract_BareTimeout_ResolvesToTimeoutHint() + { + var args = new Dictionary + { + ["Command"] = "sleep 5", + ["Timeout"] = 600 + }; + var tc = new FunctionCallContent("call-1", "shell_execute", args); + + var (meta, cleaned) = ToolCallMetaExtractor.Extract(tc, ToolArgumentHelper.ResolveMetaField); + + Assert.NotNull(meta); + Assert.Equal(600, meta!.TimeoutHintSeconds); + Assert.DoesNotContain("Timeout", (IDictionary)cleaned.Arguments!); + } + + // ── Exact resolver (persistence / schema-blind default): near-misses are NOT meta ── + // The executor passes a schema-aware resolver (covered via the executor + MCP tests); + // the default exact resolver is what persistence and schema-blind callers use. + + [Fact] + public void Extract_ExactResolver_DoesNotConsumeNearMissNames() + { + var args = new Dictionary + { + ["Command"] = "ls", + ["TimeoutSeconds"] = 1200, + ["Timeout"] = 600 + }; + var tc = new FunctionCallContent("call-1", "shell_execute", args); + + var (meta, cleaned) = ToolCallMetaExtractor.Extract(tc); // default = exact resolver + + Assert.Null(meta); + Assert.Same(tc, cleaned); // near-miss keys retained, nothing stripped + } + + [Fact] + public void Extract_ExactResolver_StillConsumesCanonicalMetaKeys() + { + var args = new Dictionary + { + ["Command"] = "ls", + ["_timeout_seconds"] = 300 + }; + var tc = new FunctionCallContent("call-1", "shell_execute", args); + + var (meta, cleaned) = ToolCallMetaExtractor.Extract(tc); // default = exact resolver + + Assert.NotNull(meta); + Assert.Equal(300, meta!.TimeoutHintSeconds); + Assert.DoesNotContain("_timeout_seconds", (IDictionary)cleaned.Arguments!); + } + + [Fact] + public void Extract_BindsEveryCanonicalMetaField() + { + // Guards ExtractFrom's `default:` throw against drift: every name in + // MetaFieldNames must have a matching extraction case, so a future meta + // field added without one fails here at CI time, not at runtime. + foreach (var name in ToolCallMeta.MetaFieldNames) + { + object? value = name switch + { + "_timeout_seconds" => 30, + "_background" => true, + _ => "because" + }; + var args = new Dictionary { ["Command"] = "ls", [name] = value }; + var tc = new FunctionCallContent("call-1", "shell_execute", args); + + var (meta, _) = ToolCallMetaExtractor.Extract(tc); + + Assert.NotNull(meta); // recognized + bound, no default-throw + } + } } diff --git a/src/Netclaw.Actors.Tests/Sessions/SubAgentSpawnIntegrationTests.cs b/src/Netclaw.Actors.Tests/Sessions/SubAgentSpawnIntegrationTests.cs index 7cdc10f9c..2bc82aa8c 100644 --- a/src/Netclaw.Actors.Tests/Sessions/SubAgentSpawnIntegrationTests.cs +++ b/src/Netclaw.Actors.Tests/Sessions/SubAgentSpawnIntegrationTests.cs @@ -286,7 +286,12 @@ public async Task Spawn_agent_subagent_approval_uses_parent_authority_and_resume "shell_execute", new Dictionary { - ["Command"] = "git push origin main" + ["Command"] = "git push origin main", + // Per-call timeout hint on the sub-agent path: the sub-agent + // loop must extract this via the shared executor seam and apply + // it to the tool context (it previously skipped extraction and + // silently dropped the hint). + ["_timeout_seconds"] = 1800 }) ]; @@ -348,6 +353,10 @@ await sessionManager.Ask(new SendUserMessage Assert.NotNull(_recordingShellTool); Assert.True(_recordingShellTool!.WasCalled); Assert.Equal(TrustAudience.Personal, _recordingShellTool.LastContext?.Audience); + + // The sub-agent extracted the meta timeout hint and applied it to the + // tool context (regression guard for the previously-dropped hint). + Assert.Equal(1800, _recordingShellTool.LastContext?.RequestedTimeoutSeconds); } [Fact] diff --git a/src/Netclaw.Actors.Tests/Tools/MetaFieldResolutionTests.cs b/src/Netclaw.Actors.Tests/Tools/MetaFieldResolutionTests.cs new file mode 100644 index 000000000..2326da70a --- /dev/null +++ b/src/Netclaw.Actors.Tests/Tools/MetaFieldResolutionTests.cs @@ -0,0 +1,136 @@ +// ----------------------------------------------------------------------- +// +// Copyright (C) 2026 - 2026 Petabridge, LLC +// +// ----------------------------------------------------------------------- +using System.Text.Json; +using Microsoft.Extensions.AI; +using Netclaw.Actors.Tools; +using Netclaw.Configuration; +using Netclaw.Tools; +using Xunit; + +namespace Netclaw.Actors.Tests.Tools; + +/// +/// Spelling-tolerant resolution of per-call meta fields. ChatGPT-trained models +/// (Qwen) drop the leading underscore, capitalize, or shorten the meta names; +/// these must resolve onto the canonical fields so the value is consumed rather +/// than rejected — while genuinely unknown keys still resolve to null and are +/// rejected upstream. The collision guard proves the tool-agnostic resolution is +/// safe: no first-party tool declares a parameter that would be hijacked. +/// +public class MetaFieldResolutionTests +{ + [Theory] + [InlineData("_rationale", "_rationale")] + [InlineData("Rationale", "_rationale")] + [InlineData("rationale", "_rationale")] + [InlineData("_timeout_seconds", "_timeout_seconds")] + [InlineData("TimeoutSeconds", "_timeout_seconds")] + [InlineData("timeout_seconds", "_timeout_seconds")] + [InlineData("Timeout_seconds", "_timeout_seconds")] + [InlineData("Timeout", "_timeout_seconds")] + [InlineData("timeout", "_timeout_seconds")] + [InlineData("_background", "_background")] + [InlineData("Background", "_background")] + [InlineData("background", "_background")] + public void ResolveMetaField_recognizes_canonical_and_chatgpt_variants(string key, string expected) + => Assert.Equal(expected, ToolArgumentHelper.ResolveMetaField(key)); + + [Theory] + [InlineData("Command")] + [InlineData("Task")] // load_tool: model invented this — genuinely unknown + [InlineData("Context")] // load_tool: ditto + [InlineData("Cancel")] // set_reminder: ditto + [InlineData("reason")] // deliberately NOT aliased — only the observed names map + [InlineData("Url")] + [InlineData("")] + [InlineData(" ")] + public void ResolveMetaField_returns_null_for_non_meta_keys(string key) + => Assert.Null(ToolArgumentHelper.ResolveMetaField(key)); + + [Fact] + public void ResolveMetaField_returns_null_for_null_key() + => Assert.Null(ToolArgumentHelper.ResolveMetaField(null)); + + // Tool-agnostic resolution is only safe if no real (non-meta) parameter + // canonicalizes onto a meta field. If a future tool declares e.g. a + // "Background" or "Timeout" parameter, this fails and forces a deliberate + // decision rather than a silent hijack. + [Fact] + public void No_first_party_tool_parameter_collides_with_a_meta_field() + { + var registry = new ToolRegistry(); + registry.WithFirstPartyTools(new ToolConfig()); + + var collisions = new List(); + foreach (var registration in registry.GetAllRegistrations()) + { + var schema = registration.Tool.ParameterSchema; + if (!schema.TryGetProperty("properties", out var props) + || props.ValueKind != JsonValueKind.Object) + continue; + + foreach (var prop in props.EnumerateObject()) + { + // Meta fields are injected into every schema and legitimately + // resolve; only declared (non-_) parameters must stay clear. + if (prop.Name.StartsWith('_')) + continue; + + if (ToolArgumentHelper.ResolveMetaField(prop.Name) is { } canonical) + collisions.Add($"{registration.Tool.Name}.{prop.Name} -> {canonical}"); + } + } + + Assert.True( + collisions.Count == 0, + "Declared tool parameters collide with meta fields: " + string.Join(", ", collisions)); + } + + // A tool (e.g. an MCP server) that declares a REAL parameter colliding with a + // meta name. Schema-aware resolution must forward that parameter, not hijack it. + private sealed class TimeoutParamTool(string schema) : INetclawTool + { + public string Name => "fake_timeout_tool"; + public LlmFacingToolName LlmFacingName { get; } = LlmFacingToolName.FromCanonical("fake_timeout_tool"); + public string Description => ""; + public string GrantCategory => "test"; + public JsonElement ParameterSchema { get; } = JsonDocument.Parse(schema).RootElement.Clone(); + public Task ExecuteAsync(IDictionary? arguments, CancellationToken ct = default) + => Task.FromResult("ok"); + public AITool ToAITool() => AIFunctionFactory.Create(() => "ok", Name); + } + + [Fact] + public void ResolveMetaField_yields_to_a_declared_parameter_of_the_same_name() + { + // An MCP tool whose server declares a real "timeout" param, alongside the + // injected meta fields. + var tool = new TimeoutParamTool( + """{"type":"object","properties":{"url":{"type":"string"},"timeout":{"type":"integer"},"_rationale":{"type":"string"},"_timeout_seconds":{"type":"integer"}}}"""); + + // The server's own "timeout" is forwarded, NOT hijacked as the meta hint. + Assert.Null(ToolArgumentValidator.ResolveMetaField(tool, "timeout")); + + // The exact injected meta key is always meta. + Assert.Equal("_timeout_seconds", ToolArgumentValidator.ResolveMetaField(tool, "_timeout_seconds")); + + // A near-miss that is NOT a declared parameter still resolves to meta. + Assert.Equal("_timeout_seconds", ToolArgumentValidator.ResolveMetaField(tool, "TimeoutSeconds")); + Assert.Equal("_rationale", ToolArgumentValidator.ResolveMetaField(tool, "Rationale")); + } + + [Fact] + public void ResolveMetaField_resolves_near_miss_when_no_colliding_parameter() + { + // Same tool minus the real "timeout" param: bare "timeout" is now free to + // resolve to the meta hint. + var tool = new TimeoutParamTool( + """{"type":"object","properties":{"url":{"type":"string"},"_timeout_seconds":{"type":"integer"}}}"""); + + Assert.Equal("_timeout_seconds", ToolArgumentValidator.ResolveMetaField(tool, "timeout")); + Assert.Null(ToolArgumentValidator.ResolveMetaField(tool, "url")); + } +} diff --git a/src/Netclaw.Actors.Tests/Tools/ToolArgumentValidatorTests.cs b/src/Netclaw.Actors.Tests/Tools/ToolArgumentValidatorTests.cs index dd1d4d1f9..f6c9f1762 100644 --- a/src/Netclaw.Actors.Tests/Tools/ToolArgumentValidatorTests.cs +++ b/src/Netclaw.Actors.Tests/Tools/ToolArgumentValidatorTests.cs @@ -72,33 +72,83 @@ private async Task ExecuteShellAsync(IDictionary args) } [Fact] - public async Task TimeoutSeconds_rejected_with_meta_key_suggestion() + public async Task TimeoutSeconds_accepted_and_consumed_as_meta_field() { // The literal arg shape from production session - // D0AC6CKBK5K_1781115410_840529 that was silently dropped. + // D0AC6CKBK5K_1781115410_840529. ChatGPT-trained models (Qwen) emit the + // underscore-dropped name; rather than reject (which pushed the model off + // tools entirely — session D0AC6CKBK5K_1781746527), it now resolves onto + // _timeout_seconds and the call runs. Not a silent default: the value is + // consumed (see MetaFieldResolutionTests / ToolCallMetaExtractorTests). var result = await ExecuteShellAsync(new Dictionary { - ["Command"] = "echo should-not-run", + ["Command"] = "echo runs-now", ["TimeoutSeconds"] = "1200" }); - Assert.Contains("Unrecognized argument 'TimeoutSeconds'", result); - Assert.Contains("Did you mean '_timeout_seconds'?", result); + Assert.DoesNotContain("Unrecognized argument", result); + Assert.Contains("runs-now", result); + } + + [Fact] + public async Task Underscore_missing_timeout_seconds_accepted() + { + var result = await ExecuteShellAsync(new Dictionary + { + ["Command"] = "echo runs-now", + ["timeout_seconds"] = 300 + }); + + Assert.DoesNotContain("Unrecognized argument", result); + Assert.Contains("runs-now", result); + } + + [Fact] + public async Task Conflicting_timeout_spellings_rejected_as_ambiguous() + { + // Two distinct keys resolving to the same meta field would force a silent + // pick-one-drop-the-other — the no-silent-discard invariant rejects it. + var result = await ExecuteShellAsync(new Dictionary + { + ["Command"] = "echo should-not-run", + ["_timeout_seconds"] = 120, + ["TimeoutSeconds"] = 1200 + }); + + Assert.Contains("both map to the meta field '_timeout_seconds'", result); Assert.Contains("NOT executed", result); Assert.DoesNotContain("should-not-run", result); } + [Theory] + [InlineData("Rationale")] + [InlineData("rationale")] + public async Task Misnamed_rationale_accepted(string key) + { + var result = await ExecuteShellAsync(new Dictionary + { + ["Command"] = "echo runs-now", + [key] = "because" + }); + + Assert.DoesNotContain("Unrecognized argument", result); + Assert.Contains("runs-now", result); + } + [Fact] - public async Task Underscore_missing_timeout_seconds_rejected_never_bound() + public async Task Misnamed_timeout_with_invalid_value_rejected_loudly() { + // Spelling tolerance must not become a silent escape hatch: a resolved + // meta key with an unusable value is still rejected before dispatch, + // naming the model's own key spelling. var result = await ExecuteShellAsync(new Dictionary { ["Command"] = "echo should-not-run", - ["timeout_seconds"] = 300 + ["TimeoutSeconds"] = "not-a-number" }); - Assert.Contains("Unrecognized argument 'timeout_seconds'", result); - Assert.Contains("Did you mean '_timeout_seconds'?", result); + Assert.Contains("Meta argument 'TimeoutSeconds'", result); + Assert.Contains("not a valid positive integer", result); Assert.DoesNotContain("should-not-run", result); } @@ -238,4 +288,56 @@ public async Task Mcp_tools_exempt_from_native_validation() Assert.DoesNotContain("Unrecognized argument", result); } + + [Fact] + public void Mcp_conflicting_meta_spellings_rejected_as_ambiguous() + { + // MCP tools skip native key validation (above), but the no-silent-discard + // invariant must still hold: two distinct keys mapping to one meta field are + // rejected loudly. The guard lives in ValidateMetaValues (every tool), not + // the native-only ValidateArgumentKeys — this proves it covers the MCP path. + var fakeTool = AIFunctionFactory.Create(() => "mcp-result", "store"); + var registry = new ToolRegistry(); + registry.Register(new McpToolAdapter(fakeTool, "memorizer", "store")); + var executor = new DispatchingToolExecutor(registry); + + var rejection = executor.ValidateToolCall(new FunctionCallContent( + "call-mcp", "memorizer/store", new Dictionary + { + ["_timeout_seconds"] = 120, + ["TimeoutSeconds"] = 1200 + })); + + Assert.NotNull(rejection); + Assert.Contains("both map to the meta field '_timeout_seconds'", rejection!.Message); + } + + [Fact] + public void InterpretToolCall_valid_extracts_meta_and_strips_keys() + { + var interp = _executor.InterpretToolCall(new FunctionCallContent("c", "shell_execute", + new Dictionary { ["Command"] = "echo hi", ["TimeoutSeconds"] = 300 })); + + Assert.Null(interp.Rejection); + Assert.Equal(300, interp.Meta?.TimeoutHintSeconds); + Assert.DoesNotContain("TimeoutSeconds", (IDictionary)interp.Cleaned.Arguments!); + } + + [Fact] + public void InterpretToolCall_rejection_leaves_the_call_uncleaned() + { + var original = new FunctionCallContent("c", "shell_execute", new Dictionary + { + ["Command"] = "echo hi", + ["_timeout_seconds"] = 1, + ["TimeoutSeconds"] = 2 + }); + + var interp = _executor.InterpretToolCall(original); + + Assert.NotNull(interp.Rejection); + Assert.Contains("both map to the meta field", interp.Rejection!.Message); + Assert.Same(original, interp.Cleaned); // not cleaned when rejected + Assert.Null(interp.Meta); + } } diff --git a/src/Netclaw.Actors/Protocol/ChatMessageConverter.cs b/src/Netclaw.Actors/Protocol/ChatMessageConverter.cs index c9d23515f..9443bd96a 100644 --- a/src/Netclaw.Actors/Protocol/ChatMessageConverter.cs +++ b/src/Netclaw.Actors/Protocol/ChatMessageConverter.cs @@ -34,11 +34,20 @@ public static class ChatMessageConverter /// re-dispatch by canonical name through the registry's two-form /// lookup. /// + /// + /// Re-drive only. When true, the per-call meta hints persisted in + /// SerializableToolCall.MetaJson (stripped from ArgumentsJson at + /// persistence) are re-injected as their canonical keys into the reconstructed + /// tool-call arguments, so a re-dispatched call reapplies the timeout/background/ + /// rationale instead of silently falling back to defaults. Must stay false for + /// outbound provider history — the model must never receive meta keys. + /// public static AiChatMessage ToAiMessage( SerializableChatMessage msg, string? sessionDir = null, ILogger? logger = null, - Func? toolNameResolver = null) + Func? toolNameResolver = null, + bool reinjectMeta = false) { var role = msg.Role switch { @@ -73,6 +82,21 @@ public static AiChatMessage ToAiMessage( args = JsonSerializer.Deserialize>(tc.ArgumentsJson); } + // Re-drive only: re-inject the persisted meta hints (stripped from + // ArgumentsJson at persistence) as canonical keys, so the re-dispatched + // call's extraction reapplies the timeout/background/rationale. Outbound + // provider history leaves them out — the model must not see meta keys. + if (reinjectMeta && ToolCallMeta.Parse(tc.MetaJson) is { } meta) + { + args ??= new Dictionary(StringComparer.Ordinal); + if (meta.Rationale is not null) + args["_rationale"] = meta.Rationale; + if (meta.TimeoutHintSeconds is { } timeoutSeconds) + args["_timeout_seconds"] = timeoutSeconds; + if (meta.Background) + args["_background"] = true; + } + var wireName = toolNameResolver?.Invoke(tc.Name.Value) ?? tc.Name.Value; contents.Add(new FunctionCallContent(tc.CallId.Value, wireName, args)); } @@ -116,7 +140,18 @@ public static List ToAiMessages( return [.. messages.Select(m => ToAiMessage(m, sessionDir, logger, toolNameResolver))]; } - public static SerializableChatMessage FromAiMessage(AiChatMessage msg, string? sessionDir = null) + /// + /// Optional schema-aware interpreter (the executor's PrepareToolCall) used to + /// extract meta + strip meta keys per tool call. When supplied, persisted history + /// matches what the runtime actually executed (e.g. a near-miss TimeoutSeconds + /// is stripped and captured in MetaJson); when null, falls back to exact + /// — the safe, schema-blind default for callers without an + /// executor (tests, replay). + /// + public static SerializableChatMessage FromAiMessage( + AiChatMessage msg, + string? sessionDir = null, + Func? CleanArgs)>? interpretToolCall = null) { var role = msg.Role == AiChatRole.User ? ChatRole.User : msg.Role == AiChatRole.Assistant ? ChatRole.Assistant @@ -142,7 +177,9 @@ public static SerializableChatMessage FromAiMessage(AiChatMessage msg, string? s break; case FunctionCallContent toolCall: - var (meta, cleanArgs) = ExtractMeta(toolCall.Arguments); + var (meta, cleanArgs) = interpretToolCall is not null + ? interpretToolCall(toolCall) + : ExtractMeta(toolCall.Arguments); toolCalls.Add(new SerializableToolCall { CallId = new Netclaw.Tools.ToolCallId(toolCall.CallId), diff --git a/src/Netclaw.Actors/Sessions/LlmSessionActor.cs b/src/Netclaw.Actors/Sessions/LlmSessionActor.cs index 3a66f6593..81dbc4590 100644 --- a/src/Netclaw.Actors/Sessions/LlmSessionActor.cs +++ b/src/Netclaw.Actors/Sessions/LlmSessionActor.cs @@ -1790,7 +1790,18 @@ private void HandleToolCallResponse( CanonicalizeToolCallNames(lastMessage, toolCalls, _toolRegistry); } - var assistantMsg = ChatMessageConverter.FromAiMessage(lastMessage); + // Persist tool calls exactly as the executor will interpret them (schema-aware + // meta extraction), so recorded history matches what actually runs — a near-miss + // meta key is stripped + captured in MetaJson, not left raw with an empty meta. + var assistantMsg = ChatMessageConverter.FromAiMessage( + lastMessage, + interpretToolCall: _toolExecutor is { } toolExec + ? tc => + { + var (meta, cleaned) = toolExec.PrepareToolCall(tc); + return (meta, cleaned.Arguments); + } + : null); var userMsg = _state.FindLastUserMessage() ?? new SerializableChatMessage { Role = Protocol.ChatRole.User, @@ -4152,7 +4163,10 @@ private bool RedriveToolBatchForApproval( // Rebuild the FunctionCallContent batch from the persisted assistant // message — tool arguments are durably stored in SerializableToolCall. - var aiMessage = ChatMessageConverter.ToAiMessage(assistantMsg); + // reinjectMeta re-applies the persisted per-call hints (timeout/background/ + // rationale) that were stripped into MetaJson at persistence, so a re-driven + // call honors them instead of falling back to defaults. + var aiMessage = ChatMessageConverter.ToAiMessage(assistantMsg, reinjectMeta: true); var toolCalls = aiMessage.Contents .OfType() .Where(tc => !ParkedToolBatchHistory.HasToolResult(_state.History, tc.CallId) diff --git a/src/Netclaw.Actors/Sessions/Pipelines/SessionToolExecutionPipeline.cs b/src/Netclaw.Actors/Sessions/Pipelines/SessionToolExecutionPipeline.cs index 581f79539..d69a937bc 100644 --- a/src/Netclaw.Actors/Sessions/Pipelines/SessionToolExecutionPipeline.cs +++ b/src/Netclaw.Actors/Sessions/Pipelines/SessionToolExecutionPipeline.cs @@ -206,14 +206,14 @@ 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) + // Single execution-preflight seam, shared with the sub-agent path via + // IToolExecutor.InterpretToolCall: validate the ORIGINAL arguments (parse + // sentinel, invalid/ambiguous meta values, unrecognized keys) and, on + // success, extract meta + strip meta keys. Rejecting here — rather than + // letting ExecuteAsync return the rejection string — is what lets the denial + // be audited as Allowed=false instead of being misreported as executed. + var interpretation = executor.InterpretToolCall(tc); + if (interpretation.Rejection is { } rejection) { auditLogger?.Log(BuildAuditEntry(sessionId, tc, timeProvider, TimeSpan.Zero, meta: null) with { @@ -230,8 +230,8 @@ public static async Task ExecuteSingleToolAsync( }, [], [], [], []); } - var (meta, cleanedTc) = ToolCallMetaExtractor.Extract(tc); - tc = cleanedTc; + var meta = interpretation.Meta; + tc = interpretation.Cleaned; // The agent's per-call timeout hint is honored as requested; when absent // the inherited default (SessionConfig.ToolExecutionTimeout) applies. diff --git a/src/Netclaw.Actors/Sessions/Pipelines/ToolCallMetaExtractor.cs b/src/Netclaw.Actors/Sessions/Pipelines/ToolCallMetaExtractor.cs index 136e35583..0b9c607b3 100644 --- a/src/Netclaw.Actors/Sessions/Pipelines/ToolCallMetaExtractor.cs +++ b/src/Netclaw.Actors/Sessions/Pipelines/ToolCallMetaExtractor.cs @@ -15,9 +15,15 @@ namespace Netclaw.Actors.Sessions.Pipelines; /// internal static class ToolCallMetaExtractor { - public static (ToolCallMeta? Meta, FunctionCallContent CleanedToolCall) Extract(FunctionCallContent tc) + /// + /// Maps a key to its canonical meta field (schema-aware for the executor, + /// exact for persistence). Defaults to exact. See + /// . + /// + public static (ToolCallMeta? Meta, FunctionCallContent CleanedToolCall) Extract( + FunctionCallContent tc, Func? resolveMeta = null) { - var (meta, cleanArgs) = ToolCallMeta.ExtractFrom(tc.Arguments); + var (meta, cleanArgs) = ToolCallMeta.ExtractFrom(tc.Arguments, resolveMeta); if (meta is null) return (null, tc); @@ -26,36 +32,63 @@ public static (ToolCallMeta? Meta, FunctionCallContent CleanedToolCall) Extract( } /// - /// 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 . + /// Rejects an unusable meta surface before dispatch: two distinct keys that map + /// to the same meta field (ambiguous), or a present-but-invalid value. 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). Runs for EVERY tool via + /// DispatchingToolExecutor.ValidateArguments, so it — not the native-only + /// — is what enforces + /// the no-silent-discard invariant on the MCP path too. Key resolution mirrors + /// (the same ), + /// so a near-miss that extraction would consume is the same one checked here, and + /// errors name the model's own key spelling. /// - public static string? ValidateMetaValues(IDictionary? arguments) + public static string? ValidateMetaValues( + IDictionary? arguments, Func? resolveMeta = null) { if (arguments is null || arguments.Count == 0) return null; + resolveMeta ??= ToolCallMeta.ResolveExactMetaField; + + // One pass. Ambiguity (two distinct keys -> one meta field) is reported the + // moment the second key is seen, ahead of any value error, so the model is + // told to drop the duplicate rather than fix a value it must remove anyway. // 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)) + // TryCoerce* ToolCallMeta.ExtractFrom binds through — and a timeout must be + // positive, matching ExtractFrom's `> 0` guard. + Dictionary? seen = null; + string? valueError = null; + foreach (var kvp in arguments) { - return $"Error: Meta argument '_timeout_seconds' value '{ToolArgumentHelper.RenderValue(tVal)}' is not a valid positive integer. The tool was NOT executed."; - } + var canonical = resolveMeta(kvp.Key); + if (canonical is null) + continue; - 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."; + seen ??= new Dictionary(StringComparer.Ordinal); + if (seen.TryGetValue(canonical, out var firstKey) + && !string.Equals(firstKey, kvp.Key, StringComparison.Ordinal)) + { + return $"Error: Arguments '{firstKey}' and '{kvp.Key}' both map to the meta field '{canonical}'. Supply only one. The tool was NOT executed."; + } + + seen[canonical] = kvp.Key; + + if (valueError is not null + || kvp.Value is null or JsonElement { ValueKind: JsonValueKind.Null }) + continue; + + valueError = canonical switch + { + "_timeout_seconds" when !(ToolArgumentHelper.TryCoerceInt(kvp.Value, out var t) && t > 0) + => $"Error: Meta argument '{kvp.Key}' value '{ToolArgumentHelper.RenderValue(kvp.Value)}' is not a valid positive integer. The tool was NOT executed.", + "_background" when !ToolArgumentHelper.TryCoerceBool(kvp.Value, out _) + => $"Error: Meta argument '{kvp.Key}' value '{ToolArgumentHelper.RenderValue(kvp.Value)}' is not a valid boolean. The tool was NOT executed.", + _ => null + }; } - return null; + return valueError; } } diff --git a/src/Netclaw.Actors/SubAgents/SubAgentActor.cs b/src/Netclaw.Actors/SubAgents/SubAgentActor.cs index 83f1b7fdc..07c9ebd6b 100644 --- a/src/Netclaw.Actors/SubAgents/SubAgentActor.cs +++ b/src/Netclaw.Actors/SubAgents/SubAgentActor.cs @@ -1064,10 +1064,25 @@ private static async Task ExecuteToolsAsync( var tasks = toolCalls.Select(async tc => { var toolContext = CreatePerToolExecutionContext(executionContext); + + // Same execution-preflight seam as the main pipeline: validate + + // extract in one step (the sub-agent previously skipped extraction + // entirely, silently dropping timeout hints). meta.Background and + // meta.Rationale are intentionally not consumed here — sub-agents + // have no background-job manager or audit logger; only the timeout + // hint maps onto the per-tool context via ApplyMeta. + var interpretation = executor.InterpretToolCall(tc); + if (interpretation.Rejection is { } rejection) + return BuildToolResult(tc, rejection.Message, toolContext, modelInputBudget); + + var meta = interpretation.Meta; + var cleanedTc = interpretation.Cleaned; + toolContext.ApplyMeta(meta); + try { - var result = await executor.ExecuteAsync(tc, toolContext, ct); - return BuildToolResult(tc, result, toolContext, modelInputBudget); + var result = await executor.ExecuteAsync(cleanedTc, toolContext, ct); + return BuildToolResult(cleanedTc, result, toolContext, modelInputBudget); } catch (ToolApprovalRequiredException approvalEx) when (approvalBridge is not null) @@ -1119,9 +1134,10 @@ or ParentApprovalDecision.ApprovedAlways var retryContext = CreatePerToolExecutionContext(executionContext); retryContext.OneTimeApprovedToolName = tc.Name; retryContext.SetOneTimeApprovedPatterns(ctx.Patterns); + retryContext.ApplyMeta(meta); - var result = await executor.ExecuteAsync(tc, retryContext, ct); - return BuildToolResult(tc, result, retryContext, modelInputBudget); + var result = await executor.ExecuteAsync(cleanedTc, retryContext, ct); + return BuildToolResult(cleanedTc, result, retryContext, modelInputBudget); } var reason = decision == ParentApprovalDecision.TimedOut diff --git a/src/Netclaw.Actors/Tools/DispatchingToolExecutor.cs b/src/Netclaw.Actors/Tools/DispatchingToolExecutor.cs index 455edb392..d549bc4ab 100644 --- a/src/Netclaw.Actors/Tools/DispatchingToolExecutor.cs +++ b/src/Netclaw.Actors/Tools/DispatchingToolExecutor.cs @@ -53,16 +53,53 @@ public DispatchingToolExecutor(ToolRegistry registry, ToolAccessPolicy policy, /// public ToolArgumentRejection? ValidateToolCall(FunctionCallContent toolCall) + => _registry.GetByName(toolCall.Name) is { } registered + ? ValidateCore(toolCall, registered, MetaResolverFor(registered)) + : null; // unknown-tool is handled separately by the execute paths + + /// + public ToolCallInterpretation InterpretToolCall(FunctionCallContent toolCall) { + // The single execution-preflight seam: resolve the tool + build the resolver + // ONCE, then validate and (only on success) extract — so validation and + // extraction can never disagree, and a caller cannot extract without first + // validating (the silent-drop footgun). Both the main pipeline and the + // sub-agent loop route through this. if (_registry.GetByName(toolCall.Name) is not { } registered) - return null; // unknown-tool is handled separately by the execute paths + return new ToolCallInterpretation(null, null, toolCall); // unknown tool: execute path reports it + + var resolveMeta = MetaResolverFor(registered); + if (ValidateCore(toolCall, registered, resolveMeta) is { } rejection) + return new ToolCallInterpretation(rejection, null, toolCall); - // Registry-free checks first (parse sentinel, meta values). - if (ValidateArguments(toolCall.Arguments) is { } rejection) + var (meta, cleaned) = ToolCallMetaExtractor.Extract(toolCall, resolveMeta); + return new ToolCallInterpretation(null, meta, cleaned); + } + + /// + public (ToolCallMeta? Meta, FunctionCallContent Cleaned) PrepareToolCall(FunctionCallContent toolCall) + { + // Extraction only (no validation) — used by the persistence path, which must + // record the model's message regardless of whether it would be rejected. + // Schema-aware; unknown tool → exact-match default (no schema to consult). + return _registry.GetByName(toolCall.Name) is { } registered + ? ToolCallMetaExtractor.Extract(toolCall, MetaResolverFor(registered)) + : ToolCallMetaExtractor.Extract(toolCall); + } + + // Validate against a tool already resolved from the registry, using a resolver + // built once by the caller — so InterpretToolCall and ValidateToolCall share one + // definition and never drift. Schema-aware meta resolution (see MetaResolverFor): + // a key that binds to the tool's OWN declared parameter is forwarded, never + // hijacked as meta. Meta-value validity and ambiguous double-spellings are checked + // in ValidateArguments (every tool); unrecognized keys are native-only (MCP + // servers validate their own schema and reject observably). + private static ToolArgumentRejection? ValidateCore( + FunctionCallContent toolCall, INetclawTool registered, Func resolveMeta) + { + if (ValidateArguments(toolCall.Arguments, resolveMeta) 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"); @@ -70,6 +107,9 @@ public DispatchingToolExecutor(ToolRegistry registry, ToolAccessPolicy policy, return null; } + private static Func MetaResolverFor(INetclawTool tool) + => key => ToolArgumentValidator.ResolveMetaField(tool, key); + /// public ToolLivenessMode GetLivenessMode(FunctionCallContent toolCall) => _registry.GetByName(toolCall.Name)?.LivenessMode ?? ToolLivenessMode.Opaque; @@ -80,7 +120,8 @@ public ToolLivenessMode GetLivenessMode(FunctionCallContent toolCall) /// 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) + public static ToolArgumentRejection? ValidateArguments( + IDictionary? args, Func? resolveMeta = null) { if (args is null || args.Count == 0) return null; @@ -100,7 +141,7 @@ public ToolLivenessMode GetLivenessMode(FunctionCallContent toolCall) // 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) + if (ToolCallMetaExtractor.ValidateMetaValues(args, resolveMeta) is { } metaError) return new ToolArgumentRejection(metaError, "invalid_meta_value"); return null; diff --git a/src/Netclaw.Actors/Tools/IToolExecutor.cs b/src/Netclaw.Actors/Tools/IToolExecutor.cs index 963dfd74b..e2f1413c0 100644 --- a/src/Netclaw.Actors/Tools/IToolExecutor.cs +++ b/src/Netclaw.Actors/Tools/IToolExecutor.cs @@ -6,6 +6,7 @@ using System.Runtime.CompilerServices; using Microsoft.Extensions.AI; using Netclaw.Actors.Protocol; +using Netclaw.Actors.Sessions.Pipelines; using Netclaw.Tools; namespace Netclaw.Actors.Tools; @@ -32,6 +33,35 @@ public interface IToolExecutor /// ToolArgumentRejection? ValidateToolCall(FunctionCallContent toolCall) => null; + /// + /// Validate + extract a tool call in one step — the single execution-preflight + /// seam BOTH the main session pipeline and the sub-agent loop route through, so + /// neither can skip validation or drop the meta hints (the sub-agent previously + /// skipped extraction entirely and silently dropped timeout hints). Returns a + /// rejection (leaving the call uncleaned) when validation fails; otherwise the + /// extracted meta and a tool call with meta keys stripped. + /// resolves the tool once and runs the real + /// schema-aware checks; the default here validates nothing and extracts exact-match, + /// for test fakes. + /// + ToolCallInterpretation InterpretToolCall(FunctionCallContent toolCall) + { + var (meta, cleaned) = PrepareToolCall(toolCall); + return new ToolCallInterpretation(ValidateToolCall(toolCall), meta, cleaned); + } + + /// + /// Extracts per-call meta (_rationale/_timeout_seconds/_background) + /// and returns it alongside a tool call with the meta keys stripped — extraction + /// ONLY, no validation (used by the persistence path, which records the model's + /// message regardless). resolves meta names + /// schema-aware: a key that binds to the tool's own declared parameter is forwarded, + /// never hijacked as meta. The default here is exact-match so test fakes need not + /// implement it. + /// + (ToolCallMeta? Meta, FunctionCallContent Cleaned) PrepareToolCall(FunctionCallContent toolCall) + => ToolCallMetaExtractor.Extract(toolCall); + /// /// Return the liveness mode for the resolved tool. Unknown tools and test /// fakes default to opaque so callers keep the conservative wall-clock bound. @@ -60,6 +90,15 @@ async IAsyncEnumerable ExecuteStreamAsync( /// public sealed record ToolArgumentRejection(string Message, string DenyReason); +/// +/// Result of : either a +/// (the call must not run) or the extracted +/// plus the tool call with meta +/// keys stripped. On rejection, is the original call. +/// +public sealed record ToolCallInterpretation( + ToolArgumentRejection? Rejection, ToolCallMeta? Meta, FunctionCallContent Cleaned); + /// /// Audit entry for tool invocations. Logged regardless of allow/deny. /// diff --git a/src/Netclaw.Tools.Abstractions/ToolArgumentHelper.cs b/src/Netclaw.Tools.Abstractions/ToolArgumentHelper.cs index f3e5ab491..4a84ce4aa 100644 --- a/src/Netclaw.Tools.Abstractions/ToolArgumentHelper.cs +++ b/src/Netclaw.Tools.Abstractions/ToolArgumentHelper.cs @@ -100,6 +100,38 @@ public static IEnumerable NormalizedAliasesFor(string normalizedName) } } + // Normalized-form -> canonical meta field, precomputed once. NormalizeKey + // folds underscore/punctuation/case, so "_timeout_seconds", "TimeoutSeconds", + // and "timeout_seconds" all key as "timeoutseconds"; the near-miss entry + // "timeout" covers the shortened form models emit, which NormalizeKey alone + // does not fold onto a canonical. OrdinalIgnoreCase because NormalizeKey + // strips punctuation but preserves case. + private static readonly Dictionary NormalizedMetaFields = BuildNormalizedMetaFields(); + + private static Dictionary BuildNormalizedMetaFields() + { + var map = new Dictionary(StringComparer.OrdinalIgnoreCase); + foreach (var canonical in ToolCallMeta.MetaFieldNames) + map[NormalizeKey(canonical)] = canonical; + + map["timeout"] = "_timeout_seconds"; + return map; + } + + /// + /// Spelling-tolerant resolution of an argument key to its canonical meta field + /// (_rationale/_timeout_seconds/_background), or null. This + /// is tool-agnostic and does NOT account for a tool that declares a real + /// parameter of the same name — callers that handle untrusted schemas (MCP) + /// must go through , + /// which yields to a declared parameter. Safe to use directly only where + /// collisions are impossible (proven for native tools by MetaFieldResolutionTests). + /// + public static string? ResolveMetaField(string? key) + => string.IsNullOrWhiteSpace(key) + ? null + : NormalizedMetaFields.TryGetValue(NormalizeKey(key), out var canonical) ? canonical : null; + public static string? GetString(IDictionary? arguments, string key) { // Binding-side consumer of the text↔Message alias group above. diff --git a/src/Netclaw.Tools.Abstractions/ToolArgumentValidator.cs b/src/Netclaw.Tools.Abstractions/ToolArgumentValidator.cs index c6802bbd8..0d191a99f 100644 --- a/src/Netclaw.Tools.Abstractions/ToolArgumentValidator.cs +++ b/src/Netclaw.Tools.Abstractions/ToolArgumentValidator.cs @@ -3,32 +3,71 @@ // Copyright (C) 2026 - 2026 Petabridge, LLC // // ----------------------------------------------------------------------- -using System.Collections.Concurrent; +using System.Runtime.CompilerServices; 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. +/// Validates LLM-supplied argument keys against a 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 ), and meta names (TimeoutSeconds → +/// _timeout_seconds) resolve via , +/// which yields to a declared parameter of the same name. 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(); + // Keyed by tool INSTANCE, not Type: MCP tools all share the McpToolAdapter + // type but each carries a distinct server-defined schema, so a Type cache + // would conflate their declared parameters. ConditionalWeakTable lets the + // entries be collected with their tools. + private sealed class RecognizedHolder + { + public RecognizedKeys? Value; + } + + private static readonly ConditionalWeakTable Cache = new(); + + private static RecognizedKeys? GetRecognized(INetclawTool tool) + => Cache.GetValue(tool, static t => new RecognizedHolder { Value = BuildRecognizedKeys(t) }).Value; + + /// + /// Resolves to the canonical meta field it addresses + /// (_rationale/_timeout_seconds/_background), or null when + /// it is not meta. Schema-aware: if declares a real + /// parameter that binds to, the parameter wins and this + /// returns null — so a third-party MCP tool's own timeout argument is + /// forwarded to the server, never hijacked as a Netclaw meta hint. The exact + /// canonical names always resolve as meta (they are the injected underscore + /// forms no real parameter shares). + /// + public static string? ResolveMetaField(INetclawTool tool, string key) + { + if (ToolArgumentHelper.ResolveMetaField(key) is not { } canonical) + return null; + + // Exact canonical meta keys (_timeout_seconds) are always meta. + if (Array.IndexOf(ToolCallMeta.MetaFieldNames, key) >= 0) + return canonical; + + // Otherwise a declared (non-meta) parameter of this tool wins. + var recognized = GetRecognized(tool); + if (recognized is not null + && recognized.NormalizedDeclared.ContainsKey(ToolArgumentHelper.NormalizeKey(key))) + return null; + + return canonical; + } /// /// Validates the supplied argument keys for . @@ -40,26 +79,38 @@ private sealed record RecognizedKeys( if (arguments is null || arguments.Count == 0) return null; - var recognized = Cache.GetOrAdd(tool.GetType(), _ => BuildRecognizedKeys(tool)); + var recognized = GetRecognized(tool); if (recognized is null) return null; // schema exposes no property list — nothing to validate against + // Classify each key as declared param, near-miss meta, or unknown. Declared + // params are matched first; only then is a key tested as a near-miss meta + // name, so the tool-agnostic resolve is safe (a declared param can never be + // mistaken for meta here). Meta-value validity and ambiguous double-spellings + // are enforced upstream in ToolCallMetaExtractor.ValidateMetaValues (which + // runs for every tool, native and MCP — this method is native-only). List<(string Key, string? Suggestion)>? unknown = null; foreach (var key in arguments.Keys) { if (recognized.Exact.Contains(key)) - continue; + continue; // exact schema property (declared param or injected meta key) - // 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. + // NormalizeKey once, reused for the declared-param and meta checks. var normalized = ToolArgumentHelper.NormalizeKey(key); + + // Flexible recognition for declared params: binding consumes + // case/punctuation variants of declared names. if (recognized.NormalizedDeclared.ContainsKey(normalized)) continue; + // Not a declared param — recognize ChatGPT-style near-miss meta names + // (TimeoutSeconds, Rationale, bare Timeout) so they are not flagged + // unknown; extraction consumes them the same way. + if (ToolArgumentHelper.ResolveMetaField(key) is not null) + continue; + unknown ??= []; - unknown.Add((key, SuggestFor(key, normalized, recognized))); + unknown.Add((key, SuggestFor(normalized, recognized))); } if (unknown is null) @@ -96,7 +147,6 @@ private sealed record RecognizedKeys( 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()) @@ -105,9 +155,9 @@ private sealed record RecognizedKeys( exact.Add(name); names.Add(name); - if (name.StartsWith('_')) - meta.Add(name); - else + // Meta keys (_-prefixed) are recognized via ResolveMetaField, not the + // declared-param table, so they are not tracked here. + if (!name.StartsWith('_')) normalizedDeclared[ToolArgumentHelper.NormalizeKey(name)] = name; } @@ -122,21 +172,14 @@ private sealed record RecognizedKeys( normalizedDeclared.TryAdd(alias, normalizedDeclared[declared]); } - return new RecognizedKeys(exact, normalizedDeclared, [.. meta], [.. names]); + return new RecognizedKeys(exact, normalizedDeclared, [.. names]); } - private static string? SuggestFor(string key, string normalizedKey, RecognizedKeys recognized) + private static string? SuggestFor(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; - } - + // Near-miss meta keys never reach here — ValidateArgumentKeys recognizes + // anything ResolveMetaField maps to a meta field before falling through to + // the unknown-key suggestion path. Only declared-param typos remain. string? best = null; var bestDistance = 3; // suggest only within edit distance 2 foreach (var name in recognized.ValidNames) diff --git a/src/Netclaw.Tools.Abstractions/ToolCallMeta.cs b/src/Netclaw.Tools.Abstractions/ToolCallMeta.cs index 86706514e..5e0b5b02c 100644 --- a/src/Netclaw.Tools.Abstractions/ToolCallMeta.cs +++ b/src/Netclaw.Tools.Abstractions/ToolCallMeta.cs @@ -55,62 +55,107 @@ public sealed record ToolCallMeta /// and a cleaned dictionary with meta keys removed. Shared by persistence and /// pipeline extraction paths. /// + /// + /// Maps an argument key to the canonical meta field it addresses, or null if it + /// is not meta. When omitted, only the exact canonical names + /// () match — the safe, schema-blind behavior for + /// persistence. The executor passes a schema-aware resolver + /// () + /// that recognizes ChatGPT-style near-misses (TimeoutSeconds, + /// Rationale) but yields to a tool's own declared parameter of the same + /// name, so a third-party MCP argument is never hijacked as meta. + /// public static (ToolCallMeta? Meta, IDictionary? CleanArgs) ExtractFrom( - IDictionary? arguments) + IDictionary? arguments, Func? resolveMeta = null) { if (arguments is null || arguments.Count == 0) return (null, arguments); + resolveMeta ??= ResolveExactMetaField; + string? rationale = null; int? timeoutSeconds = null; bool background = false; var hasAnyMeta = false; - if (arguments.TryGetValue("_rationale", out var rVal) && rVal is not null) - { - rationale = rVal switch - { - string s => s, - JsonElement { ValueKind: JsonValueKind.String } je => je.GetString(), - _ => rVal.ToString() - }; - hasAnyMeta = true; - } + // Keys (in the model's own spelling) that resolve to a meta field; stripped + // from the args the tool binder sees. Conflicting double-spellings (e.g. + // both "_timeout_seconds" and "TimeoutSeconds") are rejected loudly upstream + // in ToolArgumentValidator before this runs on the native path. + HashSet? metaKeys = null; - if (arguments.TryGetValue("_timeout_seconds", out var tVal) && tVal is not null) + foreach (var kvp in arguments) { - // 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) - { - timeoutSeconds = parsedTimeout; - hasAnyMeta = true; - } - } + var canonical = resolveMeta(kvp.Key); + if (canonical is null) + continue; - if (arguments.TryGetValue("_background", out var bVal) && bVal is not null) - { - // Shares ToolArgumentHelper.TryCoerceBool with ValidateMetaValues. - if (ToolArgumentHelper.TryCoerceBool(bVal, out var parsedBackground)) + (metaKeys ??= new HashSet(StringComparer.Ordinal)).Add(kvp.Key); + + var value = kvp.Value; + if (value is null) + continue; + + switch (canonical) { - background = parsedBackground; - if (background) + case "_rationale": + rationale = value switch + { + string s => s, + JsonElement { ValueKind: JsonValueKind.String } je => je.GetString(), + _ => value.ToString() + }; hasAnyMeta = true; + break; + + case "_timeout_seconds": + // 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(value, out var parsedTimeout) && parsedTimeout > 0) + { + timeoutSeconds = parsedTimeout; + hasAnyMeta = true; + } + + break; + + case "_background": + // Shares ToolArgumentHelper.TryCoerceBool with ValidateMetaValues. + if (ToolArgumentHelper.TryCoerceBool(value, out var parsedBackground)) + { + background = parsedBackground; + if (background) + hasAnyMeta = true; + } + + break; + + default: + // A name was added to MetaFieldNames (or the near-miss table) + // without a matching extraction case here. Fail loudly rather + // than strip-but-silently-drop the value. + throw new InvalidOperationException( + $"ToolCallMeta.ExtractFrom: unhandled meta field '{canonical}'."); } } - if (!hasAnyMeta) + if (metaKeys is null) return (null, arguments); + // Strip every resolved meta key — including ones that carried no usable + // value — so the tool binder never receives a stray meta argument. var clean = new Dictionary(arguments.Count, StringComparer.Ordinal); foreach (var kvp in arguments) { - if (!MetaFieldNames.Contains(kvp.Key)) + if (!metaKeys.Contains(kvp.Key)) clean[kvp.Key] = kvp.Value; } + if (!hasAnyMeta) + return (null, clean); + var meta = new ToolCallMeta { Rationale = rationale, @@ -120,4 +165,11 @@ public static (ToolCallMeta? Meta, IDictionary? CleanArgs) Extr return (meta, clean); } + + /// + /// Exact (ordinal) canonical-name resolver — the schema-blind default used by + /// persistence and any caller that cannot prove a key is not a real parameter. + /// + public static string? ResolveExactMetaField(string key) + => Array.IndexOf(MetaFieldNames, key) >= 0 ? key : null; } diff --git a/src/Netclaw.Tools.Abstractions/ToolExecutionContext.cs b/src/Netclaw.Tools.Abstractions/ToolExecutionContext.cs index 5292eace0..3339e1aa6 100644 --- a/src/Netclaw.Tools.Abstractions/ToolExecutionContext.cs +++ b/src/Netclaw.Tools.Abstractions/ToolExecutionContext.cs @@ -92,6 +92,19 @@ public ToolExecutionContext(string? sessionId, string? sessionDirectory) /// public int? RequestedTimeoutSeconds { get; set; } + /// + /// Applies a per-call hint to this context — the one + /// definition of "meta hint → context" shared by the pipeline and sub-agent so the + /// timeout hint can't be dropped by a path that forgets to apply it. Currently + /// only the timeout hint maps onto the context; an absent hint leaves the + /// inherited default in place. + /// + public void ApplyMeta(ToolCallMeta? meta) + { + if (meta?.TimeoutHintSeconds is { } timeoutSeconds) + RequestedTimeoutSeconds = timeoutSeconds; + } + public string? ChannelType { get; set; }