-
Notifications
You must be signed in to change notification settings - Fork 7
docs: align architecture entry with mainline implementation #223
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
8d61a67
20b478b
02bd10b
213e312
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 异常更直接。 | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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。 | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This section now reads as if the repository still has an existing Gateway boundary or protocol surface reserved in code, but this PR deletes
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 这里写成“已经为未来…预留了协议边界与设计空间”会让读者以为仓库里仍保留了现成的 Gateway 扩展接口,但本 PR 同时删除了
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This sentence now overstates the current implementation. In the same PR we remove |
||
|
|
||
| 当前约束如下: | ||
|
|
||
| - 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` | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This document is internally contradictory after the new progress note on line 18. It says the gateway/global-state/sandbox/event issues were fixed, but then immediately switches back to present-tense wording (
仍存在) and describes those same issues as current problems. Please either mark the rest of the file as historical review findings, or update the wording so readers do not come away thinking the fixes in this PR have not landed.