feat(tools): standardize executor approval and strict arg contract by fennoai[bot] · Pull Request #191 · 1024XEngineer/bytemind

fennoai · 2026-04-15T08:34:34Z

This PR implements the four requested follow-ups for issue #187 based on current runner.go / registry.go / executor.go reality.

Summary

Updated architecture docs with the real execution mapping:
- orchestration and tool result write-back: internal/agent/runner.go
- tool registration/mode filtering: internal/tools/registry.go
- unified execution pipeline: internal/tools/executor.go
Added a unified write-operation approval layer in Executor for destructive tools (write_file, replace_in_file, apply_patch class), instead of relying only on run_shell.
Changed strict unknown-args behavior to default-deny:
- decoder now rejects unknown fields by default
- registry auto-fills strict schemas with additionalProperties: false
- explicit additionalProperties: true remains opt-in to allow unknown args
Added stable contract tests in internal/tools/errors_test.go for Code/Retryable/Cause mapping.

Validation

go test ./internal/tools ./internal/agent -run 'Test(Executor|Registry|DefaultRegistry|NormalizeToolError|RunPromptStopsOnRepeatedToolPlan)' -v

- 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>

fennoai

Focused review completed across code quality, performance, security, and documentation. One actionable finding is included below.

fennoai · 2026-04-15T08:36:57Z

+			Reason:  reason,
+		})
+		if err != nil {
+			return err


Approval callback errors are returned directly here, which bypasses the ToolExecError contract used elsewhere by Executor. That can leak inconsistent error shapes/codes to callers for pre-run failures. Consider wrapping callback errors as ToolErrorPermissionDenied (or ToolErrorInternal as appropriate) with the callback error as cause, similar to other permission-path failures.

fennoai

One noteworthy issue found in the new write-approval path; details inline.

fennoai · 2026-04-15T08:37:12Z

+			Reason:  reason,
+		})
+		if err != nil {
+			return err


promptForWriteApproval returns callback errors directly (return err), which bypasses executor error normalization and can leak untyped errors out of ExecuteRequest. Since this new path is part of the unified executor pipeline, consider wrapping callback failures as ToolExecError (likely permission_denied or internal) so downstream handling remains consistent.

fennoai

Reviewed the diff across code quality, performance, security, and documentation/test alignment. I found 2 issues worth addressing.

fennoai · 2026-04-15T08:37:21Z

+			Reason:  reason,
+		})
+		if err != nil {
+			return err


promptForWriteApproval returns the raw callback error directly. Because this happens before Tool.Run, ExecuteRequest returns a non-ToolExecError on this path, which breaks the otherwise consistent executor error contract (Code/Retryable) and can leak backend-specific error text to the model. Consider wrapping callback failures as ToolErrorPermissionDenied (or ToolErrorInternal) with the original error as cause.

fennoai · 2026-04-15T08:37:21Z

 }

-func schemaRejectsUnknownFields(parameters map[string]any) bool {
+func schemaAllowsUnknownFields(parameters map[string]any) bool {


schemaAllowsUnknownFields only treats additionalProperties: true as allowing unknown keys. In JSON Schema, additionalProperties may also be an object schema; that should still allow unknown keys (with validation constraints). This implementation now rejects all unknown keys in that valid schema form, which is a behavior regression for tools that rely on schema-object additionalProperties.

fennoai

No blocking issues found across code quality, performance, security, and documentation accuracy in the changed files.

Validation run:
go test ./internal/tools ./internal/agent -run 'Test(Executor|Registry|DefaultRegistry|NormalizeToolError|RunPromptStopsOnRepeatedToolPlan)' -v

fennoai · 2026-04-15T08:37:26Z

+	"testing"
+)
+
+func TestNormalizeToolErrorContractTimeout(t *testing.T) {


Good contract coverage here: these tests assert Code, Retryable, and wrapped cause behavior, which protects error mapping stability.

fennoai

Focused review completed on executor/registry argument-validation and approval-path changes.

One actionable issue was found in error-contract consistency for the write-approval callback path.

fennoai · 2026-04-15T08:37:48Z

+			Reason:  reason,
+		})
+		if err != nil {
+			return err


execCtx.Approval(...) errors are returned directly, which bypasses ToolExecError normalization used elsewhere in the executor pipeline. This can leak untyped errors from ExecuteRequest and break callers that rely on Code/Retryable consistency. Consider wrapping callback failures with NewToolExecError(...) before returning.

Nomikfk1215 · 2026-04-16T06:29:29Z

@xgopilot 这个issue里最大的不能不修复的问题是什么

fennoai · 2026-04-16T06:29:38Z

结论

这个 PR 里最大且必须修复的问题是：write 审批回调报错时会直接透传原始错误，破坏 Executor 的统一错误契约。
位置：internal/tools/executor.go:165（return err）+ internal/tools/executor.go:101-104（ExecuteRequest 直接返回）。

为什么这是最高优先级

这条路径绕过了 ToolExecError（Code/Retryable/Cause）规范，和其余执行路径不一致。
上游如果依赖 Code/Retryable 做重试/分类，会在这条路径失效。
会把后端/回调的原始错误文本直接暴露给模型或调用方，带来不稳定和潜在信息泄露风险。

建议修复

在 promptForWriteApproval 回调错误分支将 err 包装为 NewToolExecError(...)（建议 permission_denied 或按语义用 internal），并保留 cause。

说明

已完成代码与测试上下文核对；本次仅做分析答复，未提交代码改动。

View job run

Nomikfk1215 · 2026-04-16T06:38:53Z

@xgopilot 简单再解释一下刚刚你说的问题，并且告诉我这个内容是不是总体架构文档里的内容 #162

fennoai · 2026-04-16T06:38:58Z

简单解释刚刚说的问题

问题是这条路径：internal/tools/executor.go:157-166。
在写操作审批里，如果 execCtx.Approval(...) 回调本身报错，当前直接 return err，没有包装成统一的 ToolExecError。

这会带来两点影响：

Executor 其他路径都在走统一错误契约（Code / Retryable / Cause），这条路径会“跳出去”，上层拿到不一致错误。
可能把底层原始错误文本直接透出，分类和安全性都变差。

这个是不是 #162 总体架构文档里的内容

结论：算是架构流程的一部分，但你问的这个具体问题本身是实现细节/缺陷，不是总体架构层面的新模块设计。

对应关系是：

架构设计 #162 的总体架构文档在 docs/architecture.md:238-241 已写了“工具执行走 Runner + Executor 统一管线，并做错误归一”。
你这次问的点是该管线里一个具体分支没有遵守这个契约（executor.go 的审批回调错误分支）。

所以可以理解为：文档覆盖了“应该怎么工作”的架构层描述；当前问题是代码没有完全对齐该架构约束。

View job run

Nomikfk1215 · 2026-04-16T06:53:35Z

@xgopilot 修复这个bug 的最小改动方案是什么

fennoai · 2026-04-16T06:53:41Z

最小改动方案

针对你说的这个 bug（internal/tools/executor.go:165 在审批回调失败时直接 return err），最小修复可以只做两步：

改 1 处返回逻辑（核心修复）

位置：internal/tools/executor.go:165
现在：return err
建议改为统一错误包装，例如：

return NewToolExecError(
    ToolErrorPermissionDenied,
    err.Error(),
    false,
    err,
)

这样 ExecuteRequest 这条分支也会保持 Code/Retryable/Cause 契约一致。

补 1 条回归测试（防止回退）

文件：internal/tools/executor_test.go
新增用例：Approval 回调返回 errors.New("approval backend failed") 时，断言：
- AsToolExecError(err) 为 true
- Code == ToolErrorPermissionDenied（或你们最终选定的 code）
- errors.Is(err, cause) 为 true

说明

这是“最小改动”版本：不改接口、不改流程，只修正错误归一化缺口。
本次按你的问题做了分析说明，未直接提交代码变更。

View job run

Nomikfk1215 · 2026-04-16T06:59:46Z

@xgopilot 进行修复

fennoai · 2026-04-16T06:59:52Z

完成情况

已按 @xgopilot 进行修复 完成并推送到当前 PR 分支。

修复内容

修复 internal/tools/executor.go：Approval 回调报错时不再直接透传，统一包装为 ToolExecError（permission_denied，保留原始 cause）。
修复 schemaAllowsUnknownFields：支持 JSON Schema 的 additionalProperties 为对象（schema object）时允许未知字段，避免误判。
新增回归测试：
- TestExecutorWrapsApprovalCallbackError
- TestExecutorAllowsUnknownArgumentsWhenSchemaUsesAdditionalPropertiesObject

验证

go test ./internal/tools -run 'Test(Executor|Registry|DefaultRegistry|NormalizeToolError)' -v
go test ./internal/agent -run 'TestRunPromptStopsOnRepeatedToolPlan' -v

提交

360c01f fix(tools): normalize approval callback errors and honor schema additionalProperties object

未执行内容：无。

View job run

…ionalProperties object Generated with [codeagent](https://github.com/qbox/codeagent) Co-authored-by: Nomikfk1215 <229251720+Nomikfk1215@users.noreply.github.com>

fennoai Bot mentioned this pull request Apr 15, 2026

Tools 模块设计 #187

Closed

fennoai Bot commented Apr 15, 2026

View reviewed changes

fix(tools): normalize approval callback errors and honor schema addit…

360c01f

…ionalProperties object Generated with [codeagent](https://github.com/qbox/codeagent) Co-authored-by: Nomikfk1215 <229251720+Nomikfk1215@users.noreply.github.com>

Nomikfk1215 merged commit 89ef480 into main Apr 16, 2026
0 of 2 checks passed

fennoai Bot deleted the xgopilot/issue-187-1776239652 branch April 16, 2026 07:06

Conversation

fennoai Bot commented Apr 15, 2026

Summary

Validation

Uh oh!

fennoai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

fennoai Bot Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

fennoai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

fennoai Bot Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

fennoai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

fennoai Bot Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

fennoai Bot Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

fennoai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

fennoai Bot Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

fennoai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

fennoai Bot Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

Nomikfk1215 commented Apr 16, 2026

Uh oh!

fennoai Bot commented Apr 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

结论

为什么这是最高优先级

建议修复

说明

Uh oh!

Nomikfk1215 commented Apr 16, 2026

Uh oh!

fennoai Bot commented Apr 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

简单解释刚刚说的问题

这个是不是 #162 总体架构文档里的内容

Uh oh!

Nomikfk1215 commented Apr 16, 2026

Uh oh!

fennoai Bot commented Apr 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

最小改动方案

说明

Uh oh!

Nomikfk1215 commented Apr 16, 2026

Uh oh!

fennoai Bot commented Apr 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

完成情况

修复内容

验证

提交

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

fennoai Bot commented Apr 16, 2026 •

edited

Loading

fennoai Bot commented Apr 16, 2026 •

edited

Loading

fennoai Bot commented Apr 16, 2026 •

edited

Loading

fennoai Bot commented Apr 16, 2026 •

edited

Loading