Closed
pangtong-fujunshi
wants to merge 6 commits from
docs/merge-19-into-13 into main
pull from: docs/merge-19-into-13
merge into: sanguo:main
sanguo:main
sanguo:docs/20-issue-centric-orchestration
sanguo:impl/17-issue-assigned-git-steps
sanguo:docs/17-issue-assigned-git-steps
sanguo:impl/17-ci-deploy-steps-branching
sanguo:docs/19-fix-delivery-mode
sanguo:docs/17-ci-deploy-steps-branching
sanguo:fix/91-ci-lint-ensurepip
sanguo:impl/19-s6-deprecated-cleanup
sanguo:docs/19-cron-config-design
sanguo:fix/spawner-get-task-info-must-haves
sanguo:docs/skill-lifecycle-design
sanguo:fix/spawner-event-type-missing
sanguo:design/mail-verify-prompt-fix
sanguo:feat/runaway-guard
sanguo:fix/mention-duplicate-mail-race-doc-sync
sanguo:feat/l0-l2-prompt-improvements
sanguo:feat/toolchain-from-to-filter
sanguo:fix/task-sort-desc
sanguo:refactor/toolchain-to-settings
sanguo:fix/cd-push-trigger-yaml
sanguo:ci/add-frontend
sanguo:feat/toolchain-tab
sanguo:refactor/api-split-expand
sanguo:docs/18-api-refactor-design
sanguo:docs/rewrite-s26-conventions
sanguo:feat/gitea-conventions
sanguo:impl/16-knowledge-injection
sanguo:docs/16-knowledge-injection-v2
sanguo:docs/16-knowledge-injection
sanguo:fix/ci-pip-upgrade
sanguo:feat/17-toolchain-handler-impl
sanguo:fix/17-toolchain-mail-separation
sanguo:fix/17-on-failure-redesign
sanguo:feat/17-toolchain-handler-enforcement
sanguo:feat/17-action-mail-type
sanguo:fix/docs-path-ref-15
sanguo:fix/pr-synchronize-dispatch
sanguo:docs/design-renumber
sanguo:fix/g2-agent-error-reason-map
sanguo:feat/mail-notify-v2
sanguo:chore/docs-merge-mail-failure
sanguo:chore/docs-rename-watchdog
sanguo:chore/docs-merge-3-batch
sanguo:chore/docs-merge-2-3
sanguo:chore/docs-merge-1-review-to-archive
sanguo:chore/docs-cleanup
sanguo:chore/g1-comment-fix
sanguo:fix/is-pr-detection
sanguo:docs/25-sync-help-keywords
sanguo:feat/25-gitea-mention-toolchain
sanguo:fix/auto-deploy-self-kill
sanguo:feat/43-auto-deploy-on-merge
sanguo:fix/39-review-merge-reminder
sanguo:docs/38-toolchain-design-update
sanguo:fix/toolchain-event-type-registration
sanguo:fix/toolchain-synchronize-fallback-and-merge-notify
sanguo:fix/24-compact-detection-v4
sanguo:fix/compact-detection-v3
sanguo:feat/pr-lifecycle-events
sanguo:fix/lint-cleanup
sanguo:fix/sync-to-gitea
sanguo:feat/step5-engine-integration
sanguo:feat/task-type-handlers-step2-4
sanguo:docs/s-fixes
sanguo:docs/task-type-architecture
sanguo:fix/frontend-null-vs-undefined
sanguo:fix/e2e-collection-crash
sanguo:fix/deploy-workflow
sanguo:fix/lint-regression
sanguo:fix/ci-dedup-and-notify-fix
sanguo:docs/19-toolchain-context-layers-v2
sanguo:docs/19-toolchain-context-layers-only
sanguo:fix/toolchain-review-dedup-ci-cooldown
sanguo:docs/19-toolchain-context-layers
sanguo:feat/webhook-test
No Reviewers
Dismiss Review
Are you sure you want to dismiss this review?
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: sanguo/sanguo_moziplus_v2#19
Reference in New Issue
Block a user
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Delete Branch "docs/merge-19-into-13"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
已过时:文档内容已被 PR #25/26 合入覆盖,有冲突无法合并。CI run URL 改动后续单独 PR 处理。
[CI] 失败
分支: 19
触发 commit:
a72e08403fb4f750e3b8bd5febfebbddc76526e5失败 Job: test
请检查 CI 日志并修复。
审查结论:APPROVED
改动范围
仅 2 个文件:
docs/design/13-toolchain-and-dev-workflow.md:新增 376 行(#19 内容合并入)docs/design/19-toolchain-context-layers.md:删除 382 行(原文件移除)✅ 确认项
---分隔),保持原 13.md 结构不被打乱风险级别确认
仅文档合并,无代码改动 → standard(与 PR 标注一致)
[CI] 失败
分支: 19
触发 commit:
c4f615ce7f06fa0381bc599a4ae3de1a2336d2c3失败 Job: lint
请检查 CI 日志并修复。
审查结论:APPROVED(附 3 条必修 + 4 条建议)
改动范围确认
PR #19 共 5 个文件:
docs/design/13-toolchain-and-dev-workflow.md:+376 行(#19 上下文四层改造合并,已审查通过)docs/design/19-toolchain-context-layers.md:-382 行(删除,已审查通过)docs/design/20-task-type-architecture.md:+637 行(本次审查重点)src/api/toolchain_routes.py:+22 行(CI 日志链接查询)templates/toolchain/ci_failure.md:+2 行(CI 日志链接展示)✅ 设计审查确认项
问题诊断准确(§1):grep 验证 dispatcher.py 有 15+ 处
_mail/is_mail分支、spawner.py 有 6 处、ticker.py 有 9 处。分散度确实高,Handler 模式有必要。Handler 接口定义合理(§3):8 方法 + 2 属性,每个方法在现有代码中有明确对应点。Protocol 比 ABC 更轻量,适合只定义接口不强制继承。
TaskTypeRegistry 简洁(§4):dict 查询 O(1),启动时加载运行时只读。
virtual_projects()用于 ticker 自动发现,避免硬编码虚拟项目列表。兼容期过渡策略正确(§3):handler 优先 + fallback 到 if/else,同 project_id 只走一条路径不重复执行。这降低了迁移风险。
实施顺序合理(§7):Step 1-3 风险极低/低(纯新增),Step 4 MailHandler 迁移风险中但可逐步验证,Step 5 清理放最后。
PromptSection 模式有意义(§11-§12):验证了 BootstrapBuilder 确实是 4 段拼装 +
---分隔符 + token 预算警告,PromptComposer 的 compose() 逻辑与现有\n\n---\n\n.join(sections) 一致。优先级约定(10-69 分段)清晰可扩展。引擎改动量评估(§6):dispatcher ~20 行、spawner ~15 行、ticker ~10 行——与实际 if/else 分支密度匹配,评估合理。
toolchain_routes.py CI 日志改动:从 commit sha 查 CI run URL 的逻辑正确,有 try/except 容错,ci_failure.md 补了 ci_url 变量。
⚠️ 必修(M)
M1:§2 toolchain 行为表"完成标准"描述模糊
这个描述不够精确。当前 CI failure 通知发给 Agent,Agent 的闭环是什么?修代码 push?回复 Mail?还是 CI 自动重跑成功?
建议补充具体的闭环判定条件(类似 mail 的"回复邮件 / inform done"),否则 ToolchainHandler 的
check_completion返回False意味着完全依赖 outcome 判定,需明确说明。M2:§3
post_complete签名缺少outcome的使用说明post_complete接收outcome参数,但 §5 三个 handler 的实现描述中只有 MailHandler 提到"幻觉门控 + auto_done + 失败通知"。ToolchainHandler 的post_complete描述为"auto-done + 可选 escalate"——什么条件触发 escalate?建议补充。M3:§6 spawner.py 改动遗漏了
classify_outcome中的 mail 幻觉门控spawner.py 第 1102 行
is_mail分支在classify_outcome的完成处理中触发幻觉门控。§6 的 spawner 改动只提到_build_prompt、_build_api_section、retry,没有提到classify_outcome。如果幻觉门控搬到 MailHandler.post_complete,需要在 §6 明确说明 classify_outcome 中的调用点也要改。💡 建议(S)
S1:PromptContext 字段膨胀风险
PromptContext 有
from_agent/mail_type(mail 专用)和event_type/event_data(toolchain 专用)。每新增一种 task type 就要加字段,会越来越臃肿。建议用metadata: Dict = field(default_factory=dict)存放 type 专用字段,或让 section 从 task dict 里直接取自己需要的字段。S2:BaseApiSection 复用可以更激进
§13 复用分析中 ApiSection "基础框架可复用",但三个 handler 各自声明
*ApiSection。既然 success_status 是唯一差异,可以直接用一个ApiSection(success_status="review"|"done")参数化实例,不用三个类。S3:ticker.py
_general的处理方式未说明§6 ticker 改动提到"保留 _general 硬编码作为 fallback",但 §4
virtual_projects()只返回显式注册的虚拟项目。_general不注册为 handler,它的扫描逻辑是否会和virtual_projects()自动发现冲突?建议在 §4 或 §6 补充说明。S4:toolchain_routes.py 的 CI URL 查询用了
_GITEA_BASE但未定义在 diff 中toolchain_routes.py 新增代码引用了
_GITEA_BASE和_GITEA_TOKEN。需确认这些变量已在文件其他地方定义(不在 diff 中是因为它们在 hunk 上下文外)。如果是未定义变量,运行时会报 NameError。风险级别
设计文档 + 24 行代码改动 → standard(代码部分低风险,但设计变更影响大)
M1-M3 修复确认通过:
All M-items resolved.
Pull request closed