From 7b904a1f8148b16984f6bec8472e707e49741cfb Mon Sep 17 00:00:00 2001 From: xgopilot Date: Wed, 15 Apr 2026 08:34:11 +0000 Subject: [PATCH 1/2] feat(tools): unify write approval and strict unknown-args handling - add executor-level write approval gate for destructive tools - default strict args to reject unknown fields and normalize schema defaults - add stable ToolExecError mapping contract tests - update architecture doc with real runner/registry/executor mapping Generated with [codeagent](https://github.com/qbox/codeagent) Co-authored-by: 520wheat <142715831+520wheat@users.noreply.github.com> --- docs/architecture.md | 3 +- internal/tools/errors_test.go | 87 ++++++++++++++++++++++++++++++++ internal/tools/executor.go | 76 +++++++++++++++++++++++++--- internal/tools/executor_test.go | 89 ++++++++++++++++++++++++++------- internal/tools/registry.go | 26 +++++++++- internal/tools/registry_test.go | 18 +++++++ 6 files changed, 274 insertions(+), 25 deletions(-) create mode 100644 internal/tools/errors_test.go diff --git a/docs/architecture.md b/docs/architecture.md index 3df935ea..ec6dce50 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -237,7 +237,8 @@ flowchart TD 9. 若模型返回文本答案，则保存 assistant 消息并结束本轮。 10. 若模型返回工具调用，则逐个执行 Tool： - 生成 `ExecutionContext` - - 执行 Tool + - 通过 `Runner` 调用 `Executor` 执行统一管线（参数校验、权限策略、超时、错误归一），当前实现在 `internal/agent/runner.go` + `internal/tools/executor.go` + - 工具定义与 mode 过滤由 `internal/tools/registry.go` 提供 - 记录结果 - 将 tool result 重新写回 Session 11. 进入下一轮模型调用，直到： diff --git a/internal/tools/errors_test.go b/internal/tools/errors_test.go new file mode 100644 index 00000000..292bac6e --- /dev/null +++ b/internal/tools/errors_test.go @@ -0,0 +1,87 @@ +package tools + +import ( + "context" + "errors" + "testing" +) + +func TestNormalizeToolErrorContractTimeout(t *testing.T) { + cause := context.DeadlineExceeded + err := normalizeToolError(cause) + execErr, ok := AsToolExecError(err) + if !ok { + t.Fatalf("expected ToolExecError, got %T", err) + } + if execErr.Code != ToolErrorTimeout { + t.Fatalf("unexpected code: %s", execErr.Code) + } + if !execErr.Retryable { + t.Fatal("timeout should be retryable") + } + if !errors.Is(execErr, cause) { + t.Fatal("timeout cause must be preserved") + } +} + +func TestNormalizeToolErrorContractPermissionDenied(t *testing.T) { + cause := errors.New("approval denied") + err := normalizeToolError(cause) + execErr, ok := AsToolExecError(err) + if !ok { + t.Fatalf("expected ToolExecError, got %T", err) + } + if execErr.Code != ToolErrorPermissionDenied { + t.Fatalf("unexpected code: %s", execErr.Code) + } + if execErr.Retryable { + t.Fatal("permission denied should not be retryable") + } + if !errors.Is(execErr, cause) { + t.Fatal("permission-denied cause must be preserved") + } +} + +func TestNormalizeToolErrorContractInvalidArgs(t *testing.T) { + cause := errors.New("unknown argument \"extra\"") + err := normalizeToolError(cause) + execErr, ok := AsToolExecError(err) + if !ok { + t.Fatalf("expected ToolExecError, got %T", err) + } + if execErr.Code != ToolErrorInvalidArgs { + t.Fatalf("unexpected code: %s", execErr.Code) + } + if execErr.Retryable { + t.Fatal("invalid args should not be retryable") + } + if !errors.Is(execErr, cause) { + t.Fatal("invalid-args cause must be preserved") + } +} + +func TestNormalizeToolErrorContractToolFailed(t *testing.T) { + cause := errors.New("tool crashed") + err := normalizeToolError(cause) + execErr, ok := AsToolExecError(err) + if !ok { + t.Fatalf("expected ToolExecError, got %T", err) + } + if execErr.Code != ToolErrorToolFailed { + t.Fatalf("unexpected code: %s", execErr.Code) + } + if !execErr.Retryable { + t.Fatal("tool failed should be retryable") + } + if !errors.Is(execErr, cause) { + t.Fatal("tool-failed cause must be preserved") + } +} + +func TestNormalizeToolErrorKeepsExistingToolExecError(t *testing.T) { + original := NewToolExecError(ToolErrorInvalidArgs, "bad input", false, errors.New("root")) + err := normalizeToolError(original) + if err != original { + t.Fatal("expected existing ToolExecError to be returned unchanged") + } +} diff --git a/internal/tools/executor.go b/internal/tools/executor.go index 64f54c1d..34d93772 100644 --- a/internal/tools/executor.go +++ b/internal/tools/executor.go @@ -1,10 +1,12 @@ package tools import ( + "bufio" "context" "encoding/json" "errors" "fmt" + "io" "strings" "time" "unicode/utf8" @@ -37,9 +39,14 @@ type OutputNormalizer interface { Normalize(string, ResolvedTool) string } +type WriteApprovalEngine interface { + Check(context.Context, ResolvedTool, *ExecutionContext) error +} + type Executor struct { registry *Registry permissionEngine PermissionEngine + writeApproval WriteApprovalEngine argumentDecoder ArgumentDecoder outputNormalizer OutputNormalizer } @@ -48,6 +55,7 @@ func NewExecutor(registry *Registry) *Executor { return &Executor{ registry: registry, permissionEngine: defaultPermissionEngine{}, + writeApproval: defaultWriteApprovalEngine{}, argumentDecoder: strictJSONArgumentDecoder{}, outputNormalizer: maxCharsOutputNormalizer{}, } @@ -90,6 +98,11 @@ func (e *Executor) ExecuteRequest(ctx context.Context, req ExecuteRequest) (Exec if err := e.permissionEngine.Check(ctx, resolved, req.Context); err != nil { return ExecuteResult{}, err } + if e.writeApproval != nil { + if err := e.writeApproval.Check(ctx, resolved, req.Context); err != nil { + return ExecuteResult{}, err + } + } execCtx := req.Context if execCtx == nil { @@ -121,6 +134,60 @@ func (defaultPermissionEngine) Check(_ context.Context, resolved ResolvedTool, e return nil } +type defaultWriteApprovalEngine struct{} + +func (defaultWriteApprovalEngine) Check(_ context.Context, resolved ResolvedTool, execCtx *ExecutionContext) error { + if !resolved.Spec.Destructive { + return nil + } + if execCtx == nil { + return NewToolExecError(ToolErrorPermissionDenied, fmt.Sprintf("tool %q requires approval context", resolved.Definition.Function.Name), false, nil) + } + + switch strings.TrimSpace(execCtx.ApprovalPolicy) { + case "never": + return nil + case "always", "on-request", "": + return promptForWriteApproval(resolved.Definition.Function.Name, execCtx) + default: + return promptForWriteApproval(resolved.Definition.Function.Name, execCtx) + } +} + +func promptForWriteApproval(toolName string, execCtx *ExecutionContext) error { + reason := "writes files in the workspace" + if execCtx.Approval != nil { + approved, err := execCtx.Approval(ApprovalRequest{ + Command: toolName, + Reason: reason, + }) + if err != nil { + return err + } + if !approved { + return NewToolExecError(ToolErrorPermissionDenied, fmt.Sprintf("tool %q was not run because approval was denied", toolName), false, nil) + } + return nil + } + + if execCtx.Stdin == nil { + return NewToolExecError(ToolErrorPermissionDenied, fmt.Sprintf("tool %q requires approval but no stdin is available", toolName), false, nil) + } + if execCtx.Stdout != nil { + fmt.Fprintf(execCtx.Stdout, "Approve tool %q (%s)? [y/N]: ", toolName, reason) + } + reader := bufio.NewReader(execCtx.Stdin) + line, err := reader.ReadString('\n') + if err != nil && !errors.Is(err, io.EOF) { + return NewToolExecError(ToolErrorPermissionDenied, err.Error(), false, err) + } + answer := strings.ToLower(strings.TrimSpace(line)) + if answer != "y" && answer != "yes" { + return NewToolExecError(ToolErrorPermissionDenied, fmt.Sprintf("tool %q was not run because approval was denied", toolName), false, nil) + } + return nil +} + type strictJSONArgumentDecoder struct{} func (strictJSONArgumentDecoder) Decode(rawArgs string, resolved ResolvedTool) (json.RawMessage, error) { @@ -143,14 +210,11 @@ func (strictJSONArgumentDecoder) Decode(rawArgs string, resolved ResolvedTool) ( return nil, NewToolExecError(ToolErrorInvalidArgs, "tool arguments must be a JSON object", false, nil) } - if !schemaRejectsUnknownFields(resolved.Definition.Function.Parameters) { + if schemaAllowsUnknownFields(resolved.Definition.Function.Parameters) { return json.RawMessage(rawArgs), nil } allowedFields := schemaPropertyNames(resolved.Definition.Function.Parameters) - if len(allowedFields) == 0 { - return json.RawMessage(rawArgs), nil - } for key := range objectPayload { if _, ok := allowedFields[key]; ok { continue @@ -227,13 +291,13 @@ func schemaPropertyNames(parameters map[string]any) map[string]struct{} { return names } -func schemaRejectsUnknownFields(parameters map[string]any) bool { +func schemaAllowsUnknownFields(parameters map[string]any) bool { value, ok := parameters["additionalProperties"] if !ok { return false } allowed, ok := value.(bool) - return ok && !allowed + return ok && allowed } func executionTimeout(raw json.RawMessage, spec ToolSpec) time.Duration { diff --git a/internal/tools/executor_test.go b/internal/tools/executor_test.go index db563b8f..c427b2a3 100644 --- a/internal/tools/executor_test.go +++ b/internal/tools/executor_test.go @@ -4,6 +4,8 @@ import ( "context" "encoding/json" "errors" + "os" + "path/filepath" "strings" "testing" "time" @@ -41,21 +43,25 @@ func (t executorTestTool) Run(ctx context.Context, raw json.RawMessage, execCtx return t.result, t.err } -func TestExecutorAllowsUnknownArgumentsUnlessSchemaForbidsThem(t *testing.T) { +func TestExecutorRejectsUnknownArgumentsByDefault(t *testing.T) { registry := &Registry{} registry.Add(executorTestTool{name: "strict_tool", result: `{"ok":true}`}) executor := NewExecutor(registry) - got, err := executor.Execute(context.Background(), "strict_tool", `{"path":"a.txt","extra":true}`, &ExecutionContext{}) - if err != nil { - t.Fatalf("expected extra field to be ignored, got %v", err) + _, err := executor.Execute(context.Background(), "strict_tool", `{"path":"a.txt","extra":true}`, &ExecutionContext{}) + if err == nil { + t.Fatal("expected argument validation error") } - if got != `{"ok":true}` { - t.Fatalf("unexpected result: %q", got) + execErr, ok := AsToolExecError(err) + if !ok { + t.Fatalf("expected ToolExecError, got %T", err) + } + if execErr.Code != ToolErrorInvalidArgs { + t.Fatalf("unexpected code: %s", execErr.Code) } } -func TestExecutorRejectsUnknownArgumentsWhenSchemaForbidsThem(t *testing.T) { +func TestExecutorAllowsUnknownArgumentsWhenSchemaAllowsThem(t *testing.T) { registry := &Registry{} registry.Add(executorTestTool{ name: "strict_tool", @@ -69,7 +75,7 @@ func TestExecutorRejectsUnknownArgumentsWhenSchemaForbidsThem(t *testing.T) { Name: "strict_tool", Parameters: map[string]any{ "type": "object", - "additionalProperties": false, + "additionalProperties": true, "properties": map[string]any{ "path": map[string]any{"type": "string"}, }, @@ -81,16 +87,12 @@ func TestExecutorRejectsUnknownArgumentsWhenSchemaForbidsThem(t *testing.T) { } executor := NewExecutor(registry) - _, err := executor.Execute(context.Background(), "strict_tool", `{"path":"a.txt","extra":true}`, &ExecutionContext{}) - if err == nil { - t.Fatal("expected argument validation error") - } - execErr, ok := AsToolExecError(err) - if !ok { - t.Fatalf("expected ToolExecError, got %T", err) + got, err := executor.Execute(context.Background(), "strict_tool", `{"path":"a.txt","extra":true}`, &ExecutionContext{}) + if err != nil { + t.Fatalf("expected extra field to be allowed, got %v", err) } - if execErr.Code != ToolErrorInvalidArgs { - t.Fatalf("unexpected code: %s", execErr.Code) + if got != `{"ok":true}` { + t.Fatalf("unexpected result: %q", got) } } @@ -111,6 +113,59 @@ func TestExecutorMapsPolicyFailuresToPermissionDenied(t *testing.T) { } } +func TestExecutorPromptsApprovalForDestructiveTools(t *testing.T) { + registry := DefaultRegistry() + executor := NewExecutor(registry) + workspace := t.TempDir() + var approvalCalled bool + + _, err := executor.Execute(context.Background(), "write_file", `{"path":"a.txt","content":"ok"}`, &ExecutionContext{ + Workspace: workspace, + ApprovalPolicy: "on-request", + Approval: func(req ApprovalRequest) (bool, error) { + approvalCalled = true + if req.Command != "write_file" { + t.Fatalf("unexpected approval command: %q", req.Command) + } + return false, nil + }, + }) + if err == nil { + t.Fatal("expected approval denial") + } + if !approvalCalled { + t.Fatal("expected approval callback to be called") + } + execErr, ok := AsToolExecError(err) + if !ok || execErr.Code != ToolErrorPermissionDenied { + t.Fatalf("unexpected error: %#v", err) + } + if _, statErr := os.Stat(filepath.Join(workspace, "a.txt")); !os.IsNotExist(statErr) { + t.Fatalf("expected file not to be created, got %v", statErr) + } +} + +func TestExecutorSkipsWriteApprovalWhenPolicyIsNever(t *testing.T) { + registry := DefaultRegistry() + executor := NewExecutor(registry) + workspace := t.TempDir() + + _, err := executor.Execute(context.Background(), "write_file", `{"path":"a.txt","content":"ok"}`, &ExecutionContext{ + Workspace: workspace, + ApprovalPolicy: "never", + }) + if err != nil { + t.Fatal(err) + } + data, readErr := os.ReadFile(filepath.Join(workspace, "a.txt")) + if readErr != nil { + t.Fatal(readErr) + } + if string(data) != "ok" { + t.Fatalf("unexpected file content: %q", string(data)) + } +} + func TestExecutorNormalizesToolFailure(t *testing.T) { registry := &Registry{} registry.Add(executorTestTool{name: "failing_tool", err: errors.New("command is required")}) diff --git a/internal/tools/registry.go b/internal/tools/registry.go index 0fa965f1..66221b31 100644 --- a/internal/tools/registry.go +++ b/internal/tools/registry.go @@ -59,7 +59,7 @@ func (r *Registry) Add(tool Tool) { if r.tools == nil { r.tools = map[string]ResolvedTool{} } - definition := tool.Definition() + definition := normalizeDefinitionSchema(tool.Definition()) spec := DefaultToolSpec(definition) if provider, ok := tool.(ToolSpecProvider); ok { spec = MergeToolSpec(spec, provider.Spec()) @@ -68,6 +68,7 @@ func (r *Registry) Add(tool Tool) { if err := ValidateToolSpec(spec); err != nil { panic(err) } + definition = applySpecToDefinitionSchema(definition, spec) r.tools[definition.Function.Name] = ResolvedTool{ Definition: definition, Spec: spec, @@ -209,3 +210,26 @@ func toolAllowedByPolicy(name string, execCtx *ExecutionContext) bool { } return true } + +func normalizeDefinitionSchema(def llm.ToolDefinition) llm.ToolDefinition { + if def.Function.Parameters == nil { + def.Function.Parameters = map[string]any{} + } + if _, ok := def.Function.Parameters["type"]; !ok { + def.Function.Parameters["type"] = "object" + } + if _, ok := def.Function.Parameters["properties"]; !ok { + def.Function.Parameters["properties"] = map[string]any{} + } + return def +} + +func applySpecToDefinitionSchema(def llm.ToolDefinition, spec ToolSpec) llm.ToolDefinition { + if !spec.StrictArgs { + return def + } + if _, ok := def.Function.Parameters["additionalProperties"]; !ok { + def.Function.Parameters["additionalProperties"] = false + } + return def +} diff --git a/internal/tools/registry_test.go b/internal/tools/registry_test.go index 82ed08c0..ae670d6d 100644 --- a/internal/tools/registry_test.go +++ b/internal/tools/registry_test.go @@ -132,3 +132,21 @@ func TestRegistryExecuteRespectsActiveSkillPolicy(t *testing.T) { t.Fatalf("unexpected error: %v", err) } } + +func TestRegistryAddDefaultsAdditionalPropertiesToFalseForStrictTools(t *testing.T) { + registry := &Registry{tools: map[string]ResolvedTool{}} + registry.Add(&recordingTool{name: "fake_tool"}) + + resolved, err := registry.ResolveForMode(planpkg.ModeBuild, "fake_tool") + if err != nil { + t.Fatal(err) + } + value, ok := resolved.Definition.Function.Parameters["additionalProperties"] + if !ok { + t.Fatal("expected additionalProperties to be defaulted") + } + allowed, ok := value.(bool) + if !ok || allowed { + t.Fatalf("expected additionalProperties=false, got %#v", value) + } +} From 360c01f37eeb03c136ba8a1319676e127d54bee2 Mon Sep 17 00:00:00 2001 From: xgopilot Date: Thu, 16 Apr 2026 07:01:35 +0000 Subject: [PATCH 2/2] fix(tools): normalize approval callback errors and honor schema additionalProperties object Generated with [codeagent](https://github.com/qbox/codeagent) Co-authored-by: Nomikfk1215 <229251720+Nomikfk1215@users.noreply.github.com> --- internal/tools/executor.go | 12 ++++-- internal/tools/executor_test.go | 70 +++++++++++++++++++++++++++++++++ 2 files changed, 79 insertions(+), 3 deletions(-) diff --git a/internal/tools/executor.go b/internal/tools/executor.go index 34d93772..ca69d7c5 100644 --- a/internal/tools/executor.go +++ b/internal/tools/executor.go @@ -162,7 +162,7 @@ func promptForWriteApproval(toolName string, execCtx *ExecutionContext) error { Reason: reason, }) if err != nil { - return err + return NewToolExecError(ToolErrorPermissionDenied, err.Error(), false, err) } if !approved { return NewToolExecError(ToolErrorPermissionDenied, fmt.Sprintf("tool %q was not run because approval was denied", toolName), false, nil) @@ -296,8 +296,14 @@ func schemaAllowsUnknownFields(parameters map[string]any) bool { if !ok { return false } - allowed, ok := value.(bool) - return ok && allowed + switch typed := value.(type) { + case bool: + return typed + case map[string]any: + return true + default: + return false + } } func executionTimeout(raw json.RawMessage, spec ToolSpec) time.Duration { diff --git a/internal/tools/executor_test.go b/internal/tools/executor_test.go index c427b2a3..f712615b 100644 --- a/internal/tools/executor_test.go +++ b/internal/tools/executor_test.go @@ -96,6 +96,42 @@ func TestExecutorAllowsUnknownArgumentsWhenSchemaAllowsThem(t *testing.T) { } } +func TestExecutorAllowsUnknownArgumentsWhenSchemaUsesAdditionalPropertiesObject(t *testing.T) { + registry := &Registry{} + registry.Add(executorTestTool{ + name: "strict_tool", + result: `{"ok":true}`, + }) + registry.tools["strict_tool"] = ResolvedTool{ + Definition: llm.ToolDefinition{ + Type: "function", + Function: llm.FunctionDefinition{ + Name: "strict_tool", + Parameters: map[string]any{ + "type": "object", + "additionalProperties": map[string]any{ + "type": "string", + }, + "properties": map[string]any{ + "path": map[string]any{"type": "string"}, + }, + }, + }, + }, + Spec: registry.tools["strict_tool"].Spec, + Tool: registry.tools["strict_tool"].Tool, + } + executor := NewExecutor(registry) + + got, err := executor.Execute(context.Background(), "strict_tool", `{"path":"a.txt","extra":"v"}`, &ExecutionContext{}) + if err != nil { + t.Fatalf("expected extra field to be allowed, got %v", err) + } + if got != `{"ok":true}` { + t.Fatalf("unexpected result: %q", got) + } +} + func TestExecutorMapsPolicyFailuresToPermissionDenied(t *testing.T) { registry := &Registry{} registry.Add(executorTestTool{name: "strict_tool", result: `{"ok":true}`}) @@ -145,6 +181,40 @@ func TestExecutorPromptsApprovalForDestructiveTools(t *testing.T) { } } +func TestExecutorWrapsApprovalCallbackError(t *testing.T) { + registry := DefaultRegistry() + executor := NewExecutor(registry) + workspace := t.TempDir() + cause := errors.New("approval backend failed") + + _, err := executor.Execute(context.Background(), "write_file", `{"path":"a.txt","content":"ok"}`, &ExecutionContext{ + Workspace: workspace, + ApprovalPolicy: "on-request", + Approval: func(req ApprovalRequest) (bool, error) { + if req.Command != "write_file" { + t.Fatalf("unexpected approval command: %q", req.Command) + } + return false, cause + }, + }) + if err == nil { + t.Fatal("expected approval callback failure") + } + execErr, ok := AsToolExecError(err) + if !ok { + t.Fatalf("expected ToolExecError, got %T", err) + } + if execErr.Code != ToolErrorPermissionDenied { + t.Fatalf("unexpected code: %s", execErr.Code) + } + if execErr.Retryable { + t.Fatal("approval callback failure should not be retryable") + } + if !errors.Is(err, cause) { + t.Fatal("expected wrapped approval callback cause") + } +} + func TestExecutorSkipsWriteApprovalWhenPolicyIsNever(t *testing.T) { registry := DefaultRegistry() executor := NewExecutor(registry)