diff --git a/README.md b/README.md index d175c0d5..2b9ce4d5 100644 --- a/README.md +++ b/README.md @@ -126,6 +126,7 @@ go run ./cmd/neocode ## 文档 - **[配置指南](docs/guides/configuration.md)** — Provider 策略、配置文件、环境变量 +- **[架构说明](docs/architecture/README.md)** — 当前主链路、模块边界与预留扩展面 - **[扩展 Provider](docs/guides/adding-providers.md)** — 如何添加新的模型提供商 - **[架构设计](docs/neocode-coding-agent-mvp-architecture.md)** — 整体架构与设计理念 - **[事件流](docs/runtime-provider-event-flow.md)** — Runtime 与 Provider 的事件交互 diff --git a/docs/architecture-review-issue-217.md b/docs/architecture-review-issue-217.md new file mode 100644 index 00000000..0a53338d --- /dev/null +++ b/docs/architecture-review-issue-217.md @@ -0,0 +1,134 @@ +# NeoCode 架构评审记录（Issue #217） + +本文档用于沉淀 `Issue #217` 中对当前仓库架构的复核结论，重点关注主链路、职责边界、无效抽象与可维护性风险。 + +## 评审范围 + +- `internal/app` +- `internal/gateway` +- `internal/runtime` +- `internal/tools` +- `docs` +- 根目录 `AGENTS.md` + +## 总体结论 + +当前项目主链路 `TUI -> Runtime -> Provider / Tool Manager` 基本清晰，运行时编排、工具调度、配置管理与 TUI 展示的边界总体可读，适合作为 Agent CLI 的 MVP 基础。 + +> 2026-04-10 修复进展：保留 `internal/gateway` 作为后续 Gateway 接入的预留边界，并在架构说明中明确其当前用途；待审批权限状态改为由 `runtime.Service` 实例持有；缺失 workspace sandbox 时会显式报错；关键交互事件开始在投递失败时返回错误。 + +但仓库中仍存在几类会直接拉低可维护性和演进效率的问题： + +- 存在未落地的抽象层，增加理解成本且没有产生真实隔离收益。 +- 个别运行时状态通过包级全局变量维护，削弱封装并引入并发与测试隔离风险。 +- 部分安全兜底行为过于宽松，容易在调用方漏配时悄悄退化为不安全路径。 +- 少量关键事件投递失败后没有被显式处理，可能造成 UI 视图与真实运行状态不一致。 + +## 主要问题 + +### 1. `internal/gateway` 当前属于预留扩展边界 + +相关位置： + +- `internal/gateway/contracts.go` + +现状： + +- `Gateway` 与 `RuntimePort` 只定义了契约，没有看到对应实现或被主链路消费。 +- `PermissionResolutionDecision`、`CompactResult`、`RunInput` 等类型与 `internal/runtime` 中的运行时结构存在明显语义重叠。 +- 当前 TUI 直接消费 runtime 事件与能力，没有经过 gateway 这一层。 + +影响： + +- 当前默认主链路不会经过该层，若说明不足，新人阅读时容易误判系统存在第二套必经边界。 +- 同类结构重复定义，后续演进时容易出现字段漂移。 +- 若后续实现 Gateway 时没有明确复用策略，预留接口会逐渐演变成重复契约。 + +建议： + +- 保留该目录作为 Gateway 预留边界，但在 README 或模块注释中明确标注“预留、未接入默认主链路”。 +- 后续若补齐 HTTP / RPC 网关实现，应优先复用 runtime 领域类型或收敛公共定义，避免重复建模继续扩散。 + +### 2. 权限待决状态使用包级全局变量，封装性不足 + +相关位置： + +- `internal/runtime/permission.go` + +现状： + +- `runtimePendingPermissions` 通过包级全局变量保存 `*Service -> pendingPermissionRequest` 的映射。 +- 状态生命周期没有收敛在 `Service` 实例内部。 + +影响： + +- 多实例测试或未来并发运行时更容易产生隐式共享状态。 +- 状态释放路径不够直观，后续改动时容易引入泄漏或串扰。 +- 当前结构默认每个 `Service` 同时只有一个待审批请求，不利于并行工具调用扩展。 + +建议： + +- 将待审批状态下沉到 `Service` 字段中，由实例自己持有 `pending request` 与互斥锁。 +- 若后续要支持并行工具调用，建议改成按 `requestID` 建索引，而不是单值槽位。 + +### 3. `NoopWorkspaceSandbox` 的兜底语义过宽 + +相关位置： + +- `internal/tools/manager.go` +- `internal/app/bootstrap.go` + +现状： + +- `buildToolManager` 正常路径会注入 `security.NewWorkspaceSandbox()`，这条链路是合理的。 +- 但 `tools.NewManager` 在 `sandbox == nil` 时会回退到 `NoopWorkspaceSandbox`。 +- 该实现的 `Check` 直接返回 `nil, ctx.Err()`；在上下文正常时等价于“不做任何工作区限制”。 + +影响： + +- 调用方一旦漏传 sandbox，不会快速失败，而是静默退化为无隔离执行。 +- 类型名称中的 `Noop` 容易让人忽略它实际上是在跳过关键安全检查。 + +建议： + +- 优先改为构造期强校验，要求调用方显式传入 sandbox。 +- 如果保留兜底实现，也应使用更明确的命名，并在运行时输出清晰错误，避免静默放行。 + +### 4. 关键事件发射失败后未被统一处理 + +相关位置： + +- `internal/runtime/runtime.go` + +现状： + +- `s.emit(...)` 在运行主循环中多次调用，但大多数返回值没有被处理。 +- 一旦事件通道阻塞或上下文取消，事件丢失不会总是反映到上层控制流。 + +影响： + +- UI 可能漏掉 `EventToolStart`、`EventToolResult`、`EventAgentDone` 等关键状态。 +- 当展示层状态与真实运行进度不一致时，问题排查会比较困难。 + +建议： + +- 对关键事件建立统一的错误处理策略。 +- 至少对会影响交互一致性的事件进行检查，并决定是中止当前运行、写入诊断日志，还是降级告警。 + +## 优先级建议 + +建议按以下顺序治理： + +1. 先处理 `runtimePendingPermissions` 的实例内收敛，降低运行时共享状态风险。 +2. 再收紧 `NoopWorkspaceSandbox` 的默认行为，避免安全能力因误用而失效。 +3. 随后收敛 `internal/gateway` 与 runtime 之间的重复定义，补齐用途说明，避免预留边界被误解为当前默认入口。 +4. 最后补强事件投递失败时的处理策略，提高 TUI 与 runtime 的一致性。 + +## 收益预期 + +完成上述收敛后，项目会得到几项直接收益： + +- 主链路更单纯，仓库学习成本更低。 +- 运行时状态边界更清晰，测试隔离性更好。 +- 安全能力从“依赖正确接线”变成“默认不允许漏接”。 +- 事件驱动链路更可观测，后续排查 UI 异常更直接。 diff --git a/docs/architecture/README.md b/docs/architecture/README.md new file mode 100644 index 00000000..cd0bd00f --- /dev/null +++ b/docs/architecture/README.md @@ -0,0 +1,50 @@ +# NeoCode 架构说明 + +本文档只描述当前 `main` 分支已经落地的主链路与模块边界，同时标注少量已经预留、但尚未成为默认执行路径的扩展接口，避免把未来方案误写成当前事实。 + +## 当前主链路 + +当前可运行闭环保持为： + +`用户输入 -> TUI / CLI -> Runtime -> Provider / Tools -> Runtime -> TUI 展示` + +其中： + +- `internal/tui` 负责终端交互、状态展示与命令入口。 +- `internal/cli` 负责命令行启动参数与应用入口。 +- `internal/runtime` 负责 ReAct 循环、事件派发、工具回灌、压缩触发与停止条件。 +- `internal/provider` 负责模型生成、事件转换、模型目录发现与驱动差异收敛。 +- `internal/tools` 负责工具注册、权限决策、工作区沙箱衔接与统一执行协议。 +- `internal/session` 负责会话模型与 JSON 持久化。 +- `internal/context` 负责提示词装配、消息裁剪、micro compact 与自动压缩决策。 +- `internal/config` 负责配置加载、校验、provider/model 选择与工作目录配置。 + +## 已预留但未成为默认入口的能力 + +### Gateway + +仓库已经为未来的跨进程客户端、HTTP / WebSocket 网关和远端 UI 预留了协议边界与设计空间，但当前默认主链路仍是 `TUI / CLI -> Runtime`，不会强制经过独立 Gateway。 + +当前约束如下： + +- Gateway 属于后续扩展入口，不是当前默认调用边界。 +- 预留字段与协议概念可以保留，但新增语义时必须同步补充校验规则、测试和接入位置说明。 +- 如果某个网关字段、动作或端口还未接入主链路，文档必须明确标注“预留”或“扩展”，不能写成当前事实。 + +### 多模态输入 + +当前 provider 层已经为文本加图片输入、多模型能力发现与目录缓存保留了协议能力，用于后续附件、会话资产与多协议模型支持；这部分属于已规划的扩展面，不应因为当前调用路径尚少而删除字段。 + +## 设计原则 + +- 文档优先描述当前实现，再描述已保留的扩展面。 +- 未来能力若尚未接入主链路，必须明确标注“预留”或“扩展”，不能写成当前默认事实。 +- 预留接口可以暂未被默认路径使用，但必须说明用途、边界与接入位置。 +- 如果某个抽象与当前 `main` 的真实实现冲突，以 `main` 代码为准修正文档与命名。 + +## 相关文档 + +- `docs/runtime-provider-event-flow.md` +- `docs/context-compact.md` +- `docs/session-persistence-design.md` +- `docs/tools-and-tui-integration.md` diff --git a/internal/gateway/contracts.go b/internal/gateway/contracts.go index 88eba546..db10937e 100644 --- a/internal/gateway/contracts.go +++ b/internal/gateway/contracts.go @@ -146,6 +146,8 @@ type SessionSummary struct { } // RuntimePort 定义网关访问运行时编排的下游端口契约。 +// 当前主链路仍是 TUI/CLI 直接进入 runtime；该接口保留给后续 HTTP、WebSocket、 +// 远端 UI 等 Gateway 接入形态，避免上层直接依赖 runtime 细节。 type RuntimePort interface { // Run 启动一次运行编排。 Run(ctx context.Context, input RunInput) error @@ -166,6 +168,8 @@ type RuntimePort interface { } // Gateway 定义网关主契约。 +// 该接口当前属于扩展预留，不是默认执行入口；未落地实现前也应继续保留， +// 以承载后续独立网关进程或跨端接入方案。 type Gateway interface { // Serve 启动网关服务并绑定运行端口。 Serve(ctx context.Context, runtimePort RuntimePort) error diff --git a/internal/runtime/permission.go b/internal/runtime/permission.go index 00a0fb76..9c0354a0 100644 --- a/internal/runtime/permission.go +++ b/internal/runtime/permission.go @@ -5,7 +5,6 @@ import ( "errors" "fmt" "strings" - "sync" "time" providertypes "neo-code/internal/provider/types" @@ -46,14 +45,6 @@ type pendingPermissionRequest struct { Submitted bool } -var runtimePendingPermissions = struct { - mu sync.Mutex - nextID uint64 - byRun map[*Service]*pendingPermissionRequest -}{ - byRun: make(map[*Service]*pendingPermissionRequest), -} - // ResolvePermission 接收 UI 的审批决定，并唤醒 runtime 中等待的工具调用。 func (s *Service) ResolvePermission(ctx context.Context, input PermissionResolutionInput) error { requestID := strings.TrimSpace(input.RequestID) @@ -68,20 +59,20 @@ func (s *Service) ResolvePermission(ctx context.Context, input PermissionResolut return err } - runtimePendingPermissions.mu.Lock() - pending := runtimePendingPermissions.byRun[s] + s.pendingPermissionMu.Lock() + pending := s.pendingPermission if pending == nil || pending.RequestID != requestID { - runtimePendingPermissions.mu.Unlock() + s.pendingPermissionMu.Unlock() return fmt.Errorf("runtime: permission request %q not found", requestID) } // Submitted 标记用于避免重复提交同一个 request 导致阻塞。 if pending.Submitted { - runtimePendingPermissions.mu.Unlock() + s.pendingPermissionMu.Unlock() return nil } pending.Submitted = true resultCh := pending.ResultCh - runtimePendingPermissions.mu.Unlock() + s.pendingPermissionMu.Unlock() // 非阻塞提交，避免 UI 重复触发导致写满 channel 后长时间卡住。 select { @@ -134,7 +125,7 @@ func (s *Service) executeToolCallWithPermission(ctx context.Context, input permi return tools.ToolResult{}, rememberErr } } - s.emit(ctx, EventPermissionResolved, input.RunID, input.SessionID, PermissionResolvedPayload{ + if err := s.emitRequired(ctx, EventPermissionResolved, input.RunID, input.SessionID, PermissionResolvedPayload{ RequestID: requestID, ToolCallID: input.Call.ID, ToolName: input.Call.Name, @@ -148,7 +139,9 @@ func (s *Service) executeToolCallWithPermission(ctx context.Context, input permi RuleID: permissionErr.RuleID(), RememberScope: string(scope), ResolvedAs: "rejected", - }) + }); err != nil { + return tools.ToolResult{}, err + } return tools.ToolResult{ ToolCallID: input.Call.ID, Name: input.Call.Name, @@ -160,7 +153,7 @@ func (s *Service) executeToolCallWithPermission(ctx context.Context, input permi if rememberErr := s.toolManager.RememberSessionDecision(input.SessionID, permissionErr.Action(), scope); rememberErr != nil { return tools.ToolResult{}, rememberErr } - s.emit(ctx, EventPermissionResolved, input.RunID, input.SessionID, PermissionResolvedPayload{ + if err := s.emitRequired(ctx, EventPermissionResolved, input.RunID, input.SessionID, PermissionResolvedPayload{ RequestID: requestID, ToolCallID: input.Call.ID, ToolName: input.Call.Name, @@ -174,7 +167,9 @@ func (s *Service) executeToolCallWithPermission(ctx context.Context, input permi RuleID: permissionErr.RuleID(), RememberScope: string(scope), ResolvedAs: "approved", - }) + }); err != nil { + return tools.ToolResult{}, err + } retryCtx, retryCancel := context.WithTimeout(ctx, input.ToolTimeout) retryResult, retryErr := s.toolManager.Execute(retryCtx, callInput) @@ -191,7 +186,7 @@ func (s *Service) awaitPermissionDecision( request := registerPendingPermission(s, input, permissionErr.Action()) defer clearPendingPermission(s, request.RequestID) - s.emit(ctx, EventPermissionRequest, input.RunID, input.SessionID, PermissionRequestPayload{ + if err := s.emitRequired(ctx, EventPermissionRequest, input.RunID, input.SessionID, PermissionRequestPayload{ RequestID: request.RequestID, ToolCallID: input.Call.ID, ToolName: input.Call.Name, @@ -203,7 +198,9 @@ func (s *Service) awaitPermissionDecision( Decision: permissionErr.Decision(), Reason: permissionErr.Reason(), RuleID: permissionErr.RuleID(), - }) + }); err != nil { + return "", "", request.RequestID, err + } select { case <-ctx.Done(): @@ -219,30 +216,30 @@ func (s *Service) awaitPermissionDecision( // registerPendingPermission 注册待审批请求并返回 request id。 func registerPendingPermission(s *Service, input permissionExecutionInput, action security.Action) pendingPermissionRequest { - runtimePendingPermissions.mu.Lock() - defer runtimePendingPermissions.mu.Unlock() + s.pendingPermissionMu.Lock() + defer s.pendingPermissionMu.Unlock() - runtimePendingPermissions.nextID++ + s.nextPermissionID++ request := pendingPermissionRequest{ - RequestID: fmt.Sprintf("perm-%d", runtimePendingPermissions.nextID), + RequestID: fmt.Sprintf("perm-%d", s.nextPermissionID), RunID: input.RunID, SessionID: input.SessionID, Call: input.Call, Action: action, ResultCh: make(chan PermissionResolutionDecision, 1), } - runtimePendingPermissions.byRun[s] = &request + s.pendingPermission = &request return request } // clearPendingPermission 清理待审批请求，避免跨请求误用。 func clearPendingPermission(s *Service, requestID string) { - runtimePendingPermissions.mu.Lock() - defer runtimePendingPermissions.mu.Unlock() + s.pendingPermissionMu.Lock() + defer s.pendingPermissionMu.Unlock() - current := runtimePendingPermissions.byRun[s] + current := s.pendingPermission if current != nil && current.RequestID == requestID { - delete(runtimePendingPermissions.byRun, s) + s.pendingPermission = nil } } diff --git a/internal/runtime/permission_test.go b/internal/runtime/permission_test.go index 86513069..93e840d7 100644 --- a/internal/runtime/permission_test.go +++ b/internal/runtime/permission_test.go @@ -144,6 +144,72 @@ func TestResolvePermissionDuplicateSubmissionIsNonBlocking(t *testing.T) { } } +func TestPendingPermissionStateLivesOnService(t *testing.T) { + t.Parallel() + + service := NewWithFactory( + newRuntimeConfigManager(t), + &stubToolManager{}, + newMemoryStore(), + &scriptedProviderFactory{provider: &scriptedProvider{}}, + nil, + ) + + request := registerPendingPermission(service, permissionExecutionInput{ + RunID: "run-local", + SessionID: "session-local", + Call: providertypes.ToolCall{ + ID: "call-local", + Name: "webfetch", + }, + }, security.Action{ + Type: security.ActionTypeRead, + Payload: security.ActionPayload{ + ToolName: "webfetch", + Resource: "webfetch", + }, + }) + + if service.pendingPermission == nil || service.pendingPermission.RequestID != request.RequestID { + t.Fatalf("expected pending permission stored on service, got %#v", service.pendingPermission) + } + + clearPendingPermission(service, request.RequestID) + if service.pendingPermission != nil { + t.Fatalf("expected pending permission cleared from service, got %#v", service.pendingPermission) + } +} + +func TestAwaitPermissionDecisionPropagatesCriticalEventFailure(t *testing.T) { + t.Parallel() + + service := &Service{ + events: make(chan RuntimeEvent, 1), + } + service.events <- RuntimeEvent{Type: EventError} + + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + _, _, requestID, err := service.awaitPermissionDecision(ctx, permissionExecutionInput{ + RunID: "run-event-fail", + SessionID: "session-event-fail", + Call: providertypes.ToolCall{ + ID: "call-event-fail", + Name: "webfetch", + }, + }, &tools.PermissionDecisionError{}) + if requestID == "" { + t.Fatalf("expected request id even on emit failure") + } + if err == nil || !containsError(err, "emit permission_request event") { + t.Fatalf("expected permission_request emit error, got %v", err) + } + if service.pendingPermission != nil { + t.Fatalf("expected deferred cleanup to clear pending permission, got %#v", service.pendingPermission) + } +} + func TestServiceRunPermissionRejectFlow(t *testing.T) { t.Parallel() @@ -342,7 +408,7 @@ func TestExecuteToolCallWithPermissionReturnsContextCanceledFromEmitChunk(t *tes registry := tools.NewRegistry() registry.Register(&stubTool{ - name: "filesystem_read_file", + name: "webfetch", executeFn: func(_ context.Context, input tools.ToolCallInput) (tools.ToolResult, error) { if input.EmitChunk == nil { t.Fatalf("expected EmitChunk callback") @@ -379,8 +445,8 @@ func TestExecuteToolCallWithPermissionReturnsContextCanceledFromEmitChunk(t *tes SessionID: "session-canceled", Call: providertypes.ToolCall{ ID: "call-canceled", - Name: "filesystem_read_file", - Arguments: `{"path":"README.md"}`, + Name: "webfetch", + Arguments: `{"url":"https://example.com"}`, }, ToolTimeout: time.Second, }) @@ -409,7 +475,7 @@ func TestExecuteToolCallWithPermissionDoesNotRecheckContextAfterSuccessfulEmit(t var cancel context.CancelFunc registry := tools.NewRegistry() registry.Register(&stubTool{ - name: "filesystem_read_file", + name: "webfetch", executeFn: func(_ context.Context, input tools.ToolCallInput) (tools.ToolResult, error) { if input.EmitChunk == nil { t.Fatalf("expected EmitChunk callback") @@ -447,8 +513,8 @@ func TestExecuteToolCallWithPermissionDoesNotRecheckContextAfterSuccessfulEmit(t SessionID: "session-successful-emit", Call: providertypes.ToolCall{ ID: "call-successful-emit", - Name: "filesystem_read_file", - Arguments: `{"path":"README.md"}`, + Name: "webfetch", + Arguments: `{"url":"https://example.com"}`, }, ToolTimeout: time.Second, }) @@ -465,7 +531,7 @@ func TestExecuteToolCallWithPermissionReturnsContextCanceledWhenChunkNotDelivere registry := tools.NewRegistry() registry.Register(&stubTool{ - name: "filesystem_read_file", + name: "webfetch", executeFn: func(_ context.Context, input tools.ToolCallInput) (tools.ToolResult, error) { if input.EmitChunk == nil { t.Fatalf("expected EmitChunk callback") @@ -511,8 +577,8 @@ func TestExecuteToolCallWithPermissionReturnsContextCanceledWhenChunkNotDelivere SessionID: "session-canceled-blocked", Call: providertypes.ToolCall{ ID: "call-canceled-blocked", - Name: "filesystem_read_file", - Arguments: `{"path":"README.md"}`, + Name: "webfetch", + Arguments: `{"url":"https://example.com"}`, }, ToolTimeout: time.Second, }) diff --git a/internal/runtime/runtime.go b/internal/runtime/runtime.go index 2a89957c..7cd247a6 100644 --- a/internal/runtime/runtime.go +++ b/internal/runtime/runtime.go @@ -148,6 +148,9 @@ type Service struct { activeRunCancel context.CancelFunc // 当前活跃 Run 的取消函数。 sessionInputTokens int // 当前会话累计输入 token。 sessionOutputTokens int // 当前会话累计输出 token。 + pendingPermissionMu sync.Mutex // 保护待审批请求状态，避免不同运行实例共享全局变量。 + pendingPermission *pendingPermissionRequest + nextPermissionID uint64 } func NewWithFactory( @@ -209,7 +212,9 @@ func (s *Service) Run(ctx context.Context, input UserInput) error { if err := s.sessionStore.Save(ctx, &session); err != nil { return s.handleRunError(ctx, input.RunID, session.ID, err) } - s.emit(ctx, EventUserMessage, input.RunID, session.ID, userMessage) + if err := s.emitRequired(ctx, EventUserMessage, input.RunID, session.ID, userMessage); err != nil { + return s.handleRunError(ctx, input.RunID, session.ID, err) + } autoCompacted := false reactiveRetried := false @@ -227,7 +232,7 @@ func (s *Service) Run(ctx context.Context, input UserInput) error { } if attempt >= maxLoops { err := errors.New("runtime: max loop reached") - s.emit(ctx, EventError, input.RunID, session.ID, err.Error()) + _ = s.emit(ctx, EventError, input.RunID, session.ID, err.Error()) return err } @@ -332,7 +337,9 @@ func (s *Service) Run(ctx context.Context, input UserInput) error { return s.handleRunError(ctx, input.RunID, session.ID, err) } if len(assistant.ToolCalls) == 0 { - s.emit(ctx, EventAgentDone, input.RunID, session.ID, assistant) + if err := s.emitRequired(ctx, EventAgentDone, input.RunID, session.ID, assistant); err != nil { + return s.handleRunError(ctx, input.RunID, session.ID, err) + } return nil } @@ -340,7 +347,9 @@ func (s *Service) Run(ctx context.Context, input UserInput) error { if err := ctx.Err(); err != nil { return s.handleRunError(ctx, input.RunID, session.ID, err) } - s.emit(ctx, EventToolStart, input.RunID, session.ID, call) + if err := s.emitRequired(ctx, EventToolStart, input.RunID, session.ID, call); err != nil { + return s.handleRunError(ctx, input.RunID, session.ID, err) + } result, execErr := s.executeToolCallWithPermission(ctx, permissionExecutionInput{ RunID: input.RunID, @@ -362,7 +371,15 @@ func (s *Service) Run(ctx context.Context, input UserInput) error { result.Content = execErr.Error() } if permissionEvent, ok := permissionEventFromError(execErr); ok { - s.emit(ctx, EventPermissionResolved, input.RunID, session.ID, permissionEvent.toResolvedPayload()) + if err := s.emitRequired( + ctx, + EventPermissionResolved, + input.RunID, + session.ID, + permissionEvent.toResolvedPayload(), + ); err != nil { + return s.handleRunError(ctx, input.RunID, session.ID, err) + } } toolMessage := providertypes.Message{ @@ -375,7 +392,7 @@ func (s *Service) Run(ctx context.Context, input UserInput) error { session.UpdatedAt = time.Now() if err := s.sessionStore.Save(ctx, &session); err != nil { if execErr != nil && errors.Is(err, context.Canceled) { - s.emit(ctx, EventToolResult, input.RunID, session.ID, result) + _ = s.emit(ctx, EventToolResult, input.RunID, session.ID, result) } return s.handleRunError(ctx, input.RunID, session.ID, err) } @@ -385,7 +402,9 @@ func (s *Service) Run(ctx context.Context, input UserInput) error { } } - s.emit(ctx, EventToolResult, input.RunID, session.ID, result) + if err := s.emitRequired(ctx, EventToolResult, input.RunID, session.ID, result); err != nil { + return s.handleRunError(ctx, input.RunID, session.ID, err) + } if execErr != nil { if err := ctx.Err(); err != nil { return s.handleRunError(ctx, input.RunID, session.ID, err) @@ -515,6 +534,14 @@ func (s *Service) emit(ctx context.Context, kind EventType, runID string, sessio } } +// emitRequired 用于投递影响交互一致性的关键事件；投递失败时返回带事件名的错误。 +func (s *Service) emitRequired(ctx context.Context, kind EventType, runID string, sessionID string, payload any) error { + if err := s.emit(ctx, kind, runID, sessionID, payload); err != nil { + return fmt.Errorf("runtime: emit %s event: %w", kind, err) + } + return nil +} + // handleProviderStreamEvent 解析并应用单条 provider 流式事件，缺失载荷或未知类型时返回错误。 func handleProviderStreamEvent( event providertypes.StreamEvent, diff --git a/internal/runtime/runtime_test.go b/internal/runtime/runtime_test.go index acb72e97..cc2b34f1 100644 --- a/internal/runtime/runtime_test.go +++ b/internal/runtime/runtime_test.go @@ -3085,6 +3085,23 @@ func TestEmitDropsWhenChannelFullAndContextCanceled(t *testing.T) { } } +func TestEmitRequiredWrapsCriticalEventError(t *testing.T) { + t.Parallel() + + service := &Service{ + events: make(chan RuntimeEvent, 1), + } + service.events <- RuntimeEvent{Type: EventAgentChunk} + + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + err := service.emitRequired(ctx, EventToolStart, "run-id", "session-id", "payload") + if err == nil || !containsError(err, "emit tool_start event") { + t.Fatalf("expected wrapped tool_start emit error, got %v", err) + } +} + func TestCallProviderWithRetryReturnsCombinedForwardError(t *testing.T) { t.Parallel() diff --git a/internal/tools/manager.go b/internal/tools/manager.go index dab53833..5caaf090 100644 --- a/internal/tools/manager.go +++ b/internal/tools/manager.go @@ -40,13 +40,33 @@ type WorkspaceSandbox interface { Check(ctx context.Context, action security.Action) (*security.WorkspaceExecutionPlan, error) } -// NoopWorkspaceSandbox keeps the explicit sandbox stage in the execution chain -// without changing current behavior. -type NoopWorkspaceSandbox struct{} +type missingWorkspaceSandbox struct{} -// Check implements WorkspaceSandbox. -func (NoopWorkspaceSandbox) Check(ctx context.Context, action security.Action) (*security.WorkspaceExecutionPlan, error) { - return nil, ctx.Err() +// Check 在调用方漏传 sandbox 时明确返回错误，避免静默退化为无隔离执行。 +func (missingWorkspaceSandbox) Check(ctx context.Context, action security.Action) (*security.WorkspaceExecutionPlan, error) { + if err := ctx.Err(); err != nil { + return nil, err + } + if !requiresWorkspaceSandbox(action) { + return nil, nil + } + return nil, errors.New("tools: workspace sandbox is required") +} + +// requiresWorkspaceSandbox 判断当前动作是否会触及本地工作区边界。 +func requiresWorkspaceSandbox(action security.Action) bool { + switch action.Type { + case security.ActionTypeBash: + return true + case security.ActionTypeRead, security.ActionTypeWrite: + targetType := action.Payload.SandboxTargetType + if targetType == "" { + targetType = action.Payload.TargetType + } + return targetType == security.TargetTypePath || targetType == security.TargetTypeDirectory + default: + return false + } } // PermissionDecisionError reports a non-allow permission decision. @@ -149,7 +169,7 @@ func NewManager(executor Executor, engine security.PermissionEngine, sandbox Wor engine = defaultEngine } if sandbox == nil { - sandbox = NoopWorkspaceSandbox{} + sandbox = missingWorkspaceSandbox{} } return &DefaultManager{ diff --git a/internal/tools/manager_test.go b/internal/tools/manager_test.go index 45e7de66..a2dcbb51 100644 --- a/internal/tools/manager_test.go +++ b/internal/tools/manager_test.go @@ -538,7 +538,7 @@ func TestDefaultManagerSessionPermissionMemory(t *testing.T) { if err != nil { t.Fatalf("new engine: %v", err) } - manager, err := NewManager(registry, engine, nil) + manager, err := NewManager(registry, engine, &stubSandbox{}) if err != nil { t.Fatalf("new manager: %v", err) } @@ -748,7 +748,7 @@ func TestDefaultManagerSessionPermissionMemory(t *testing.T) { if err != nil { t.Fatalf("new engine: %v", err) } - manager, err := NewManager(registry, engine, nil) + manager, err := NewManager(registry, engine, &stubSandbox{}) if err != nil { t.Fatalf("new manager: %v", err) } @@ -809,7 +809,7 @@ func TestDefaultManagerSessionPermissionMemory(t *testing.T) { if err != nil { t.Fatalf("new engine: %v", err) } - manager, err := NewManager(registry, engine, nil) + manager, err := NewManager(registry, engine, &stubSandbox{}) if err != nil { t.Fatalf("new manager: %v", err) } @@ -1060,16 +1060,34 @@ func TestPermissionMapperHelpers(t *testing.T) { } } -func TestNoopWorkspaceSandbox(t *testing.T) { +func TestMissingWorkspaceSandbox(t *testing.T) { t.Parallel() - sandbox := NoopWorkspaceSandbox{} - plan, err := sandbox.Check(context.Background(), security.Action{}) + sandbox := missingWorkspaceSandbox{} + plan, err := sandbox.Check(context.Background(), security.Action{ + Type: security.ActionTypeWrite, + Payload: security.ActionPayload{ + TargetType: security.TargetTypePath, + }, + }) + if plan != nil { + t.Fatalf("expected nil workspace plan, got %#v", plan) + } + if err == nil || !strings.Contains(err.Error(), "workspace sandbox is required") { + t.Fatalf("expected required sandbox error, got %v", err) + } + + plan, err = sandbox.Check(context.Background(), security.Action{ + Type: security.ActionTypeRead, + Payload: security.ActionPayload{ + TargetType: security.TargetTypeURL, + }, + }) if err != nil { - t.Fatalf("expected nil error, got %v", err) + t.Fatalf("expected non-workspace action to pass, got %v", err) } if plan != nil { - t.Fatalf("expected nil workspace plan, got %#v", plan) + t.Fatalf("expected nil plan for non-workspace action, got %#v", plan) } ctx, cancel := context.WithCancel(context.Background()) diff --git a/internal/tools/types.go b/internal/tools/types.go index 462ec8d9..df4165dd 100644 --- a/internal/tools/types.go +++ b/internal/tools/types.go @@ -26,11 +26,13 @@ type Tool interface { type ChunkEmitter func(chunk []byte) error type ToolCallInput struct { - ID string - Name string - Arguments []byte - SessionID string - Workdir string + ID string + Name string + Arguments []byte + SessionID string + Workdir string + // WorkspacePlan 为沙箱层生成的执行计划；为 nil 时表示当前调用未绑定工作区沙箱， + // 工具只能依赖自身的路径解析与校验逻辑，不应被解释为“已经通过沙箱放行”。 WorkspacePlan *security.WorkspaceExecutionPlan // EmitChunk 为流式分片回调，语义见 ChunkEmitter 注释。 EmitChunk ChunkEmitter