Closes #5 Add admin public agent pool support#82
Conversation
XiaoCooder
changed the title
XiaoCooder
changed the title
XiaoCooder
changed the title
keting
left a comment
keting
left a comment
There was a problem hiding this comment.
1. 总体结论
暂缓合并。
PR 在主体路径上确实落实了 issue #5 的可见性模型：管理员公共 Agent 池、普通用户私有 Agent 隔离、项目/计划流程对公共 Agent 的支持、用户升降级语义和文档更新。但当前分支存在两条 Blocker：已有测试被 PR 直接打挂；流程模板应用路径仍按 owner-only 校验，公共 Agent 在该路径下功能性
broken，未满足 issue 验收。再加上测试覆盖远未达到 issue 验收清单要求，建议先修两个 Blocker、补足关键路径测试、收敛新增设计文档，再来评审一轮。
2. 是否解决 issue #5
Issue #5 核心要求：引入“管理员公共 Agent + 普通用户私有 Agent”模型；公共 Agent 对所有登录用户可见可用、仅创建者管理员可维护；项目/计划/任务相关流程都能正确使用公共 Agent；删除/禁用、管理员升降级、冻结、共享状态语义都要一致。
PR 已完成：
- GET /agents 可见性模型：普通用户看自己 + 活跃公共；管理员看自己 + 全部公共含停用。
- 普通用户对公共 Agent 只读：前端按钮已 disable，覆盖编辑、删除、状态、重置、确认、拖拽。
- 项目创建/编辑可选活跃公共 Agent；编辑时历史引用的停用公共 Agent 可保留。
- 计划 prompt 生成、_resolve_assignee_agent_id 在项目已绑定 Agent 集合中优先解析，支持已停用公共 Agent。
- 公共 Agent 删除前做了跨用户引用扫描，但实现仍有可维护性和边界风险，见 Minor。
- 超级管理员 username == "admin" 不可降级；其他管理员降级时公共 Agent 迁移给超级管理员，名称冲突拒绝。
- 普通用户升级要求 confirm_publicize_agents 显式确认。
- 冻结管理员不影响其公共 Agent。
- SECURITY.md 和 docs/architecture.md 同步公共 Agent / 共享状态语义。
未完成或可疑：
- 流程模板应用 routers/process_templates.py 仍按 owner-only 校验，项目绑定的公共 Agent 在该路径会被拒绝，见 Blocker 2。
- 已有测试 test_plan_assignee_resolution.py 被 PR 的签名变更直接打挂，见 Blocker 1。
- 新增设计文档 docs/admin-public-agents-design.md 含过程性自述，更像方案草稿而非长期文档。
偏离 issue 范围的改动：
- .gitignore 加入 src/.env.debug、src/docker-compose.debug.yml、.codex-debug/、debug-*.log，与 issue 无关。
- 449 行 docs/admin-public-agents-design.md 体量过大且位置不合适。
3. 主要问题清单
Blocker
- 现有后端测试失败，PR 不能合并
src/backend/tests/test_plan_assignee_resolution.py 中 4 个测试仍按旧签名 _resolve_assignee_agent_id(self.db, "...", owner_user_id=1) 调用，但 PR 已把签名改为 (db, assignee, project, owner)，运行会抛：
TypeError: unexpected keyword argument 'owner_user_id'
修复方式：更新这些测试到新签名；同时补充三个解析场景的覆盖：项目已绑定公共 Agent、停用但项目历史绑定的公共 Agent、私有 Agent 与公共 Agent 同名时的优先级。
- 流程模板应用路径仍 owner-only，公共 Agent 在该路径不可用
src/backend/routers/process_templates.py:487-490 的 slot 映射查询仍是：
agents = db.query(Agent).filter(
Agent.id.in_(mapped_agent_ids),
Agent.created_by == user.id,
).all()
if len(agents) != len(mapped_agent_ids):
raise HTTPException(status_code=400, detail="Some mapped agents do not exist")
即使前面已经检查 mapped_agent_ids 属于 project_agent_ids，第二次查询仍会因为 created_by == user.id 把项目绑定的公共 Agent 滤掉，普通用户应用模板时报 Some mapped agents do not exist。这违反 issue 验收“项目/计划/任务相关流程可使用公共 Agent”。
修复方式：把这段查询替换为复用 load_usable_agents(db, mapped_agent_ids, user, allow_keep_ids=set(project_agent_ids)) 或同等可见性 + 项目绑定校验。请同时在该文件里 grep 一遍 created_by == user.id 的其它出现，避免遗漏同模式问题。
Major
- 测试覆盖不足以验证 issue 验收清单
新增 test_admin_public_agents.py 只覆盖了 4 个 case，可见性 + 部分升降级，未覆盖：
- 普通用户 / 非创建者管理员对公共 Agent 的修改、删除、reset、confirm 应被拒绝。
- 公共 Agent 被其他用户项目/任务/计划引用时禁止硬删除，并明确返回“请先禁用”。
- 已停用公共 Agent 在已有项目中保留、在新建项目中不可选。
- 项目/计划/流程模板三条路径都用公共 Agent 的最小 e2e。
- 降级时 (name, created_by) 命名冲突路径返回冲突列表。
- 冻结管理员后其公共 Agent 仍对其他用户可见可用。
设计文档 docs/admin-public-agents-design.md 自己已经把这份测试计划写得很清晰，建议落实到测试。
- docs/admin-public-agents-design.md 不应放在 docs/
- issue 描述里参考的是 proposals/admin-public-agents/plan.md，仓库提案文档惯例更像应在 proposals/ 而非 docs/。
- 文件中保留了“当前分支中未找到 proposals/admin-public-agents/plan.md 文件；本设计基于现有代码结构和本需求描述编写。”这是给 reviewer 的临时说明，不属于产品文档。
- 文件长达 449 行，结尾还有“实施顺序建议”等过程性内容。
- 大部分内容已经在 docs/architecture.md 和 SECURITY.md 用更精炼的方式表达。
建议：挪到 proposals/admin-public-agents/plan.md 作为提案存档，删掉过程性语句；或者直接删除，依赖 docs/architecture.md 已有段落即可。
4. Minor
- get_mutable_agent 对只读公共 Agent 返回 404
access.py::get_mutable_agent 直接复用 get_owned_agent。当普通用户尝试 PUT/PATCH/DELETE/reset/confirm 一个能在列表里看见的公共 Agent 时，会拿到 404 Agent not found，语义错位。
建议：先用 get_visible_agent 找资源；存在但 created_by != user.id 时返回 403，文案例如“公共 Agent 仅创建者可维护”。
- delete_agent 引用检查实现偏重，且存在边界漏判风险
当前删除检查会全表加载 Project / ProjectPlan，再在 Python 侧逐条解析 JSON。数据量上来后会有性能问题。
另外，selected_agent_ids_json 解析只识别 int 类型。如果历史数据里出现 "1" 这种数字字符串，可能漏判引用。建议至少兼容数字字符串，并考虑用粗筛或结构化关联表减少全表 JSON 扫描。
- 公共 Agent reset / confirm 影响共享状态，前端缺少提示
公共 Agent 的 subscription_expires_at、availability_status、短期/长期 reset 字段是所有使用者共享的一份状态。当创建者管理员点“重置”或“确认”时，所有正在使用该公共 Agent 的用户的倒计时/状态会同步刷新，但前端没有提示。
建议在创建者管理员的 reset/confirm 按钮 hover 加一句 title，或加一个 confirm 弹窗：“该操作会同步刷新所有使用该公共 Agent 用户的倒计时/状态，是否继续？”
- .gitignore 夹带了与本 PR 范围无关的本地调试条目
src/.env.debug、src/docker-compose.debug.yml、.codex-debug/、debug-*.log 与 issue #5 无关，属于作者本地开发环境痕迹。建议拆到独立 PR 或直接删除。.pytest_cache/ 与测试相关，可以保留。
Nit
无。
5. 文档与命名检查
- SECURITY.md 修改：必要、位置合理、内容准确。
- docs/architecture.md 修改：必要、位置合理、内容简洁，与 issue 已确认规则一致。
- docs/admin-public-agents-design.md 新增：不建议保留当前形态。原因是位置不合适、含过程性自述、与 docs/architecture.md 大量重复。建议挪到 proposals/admin-public-agents/plan.md 并删去过程性内容；或直接删除。
6. 测试与验证建议
已有测试不足，且当前相关测试失败。
后端单测建议：
- 修复 test_plan_assignee_resolution.py，覆盖项目已绑定公共 Agent、停用但历史绑定公共 Agent、私有 Agent 优先于公共 Agent 的解析顺序。
- 增加 process_templates 测试：项目绑定公共 Agent 后，可以应用模板并生成 assignee。
- 增加项目创建/编辑测试：活跃公共 Agent 可新增；停用公共 Agent 不可新增但历史引用可保留；移除后不可重新添加。
- 增加删除测试：公共 Agent 被其他用户项目、任务、计划引用时禁止硬删除并返回“请先禁用”；私有 Agent 被引用返回原错误；无引用时允许硬删除。
- 增加用户升降级测试：超级管理员重名冲突拒绝；普通用户升级未确认时返回将公共化 Agent 列表，确认后升级成功；冻结管理员后其公共 Agent 仍可被其他用户使用。
前端测试建议：
- 公共 Agent 只读按钮 disabled。
- 用户升级确认弹窗。
- 项目/Plan 页面公共/停用标识。
手工验证建议：
- 用 admin A 创建公共 Agent，用户 B 看到、可在新项目中选；admin A 禁用后，B 看不到，但 B 已有项目仍显示并标“已停用”；再应用流程模板、生成计划并 finalize。
- 用户 B 在已有项目里把已停用公共 Agent 移除后，再编辑该项目时它不再可选。
- 升级用户 B 为 admin，弹窗列出将公共化的 Agent，取消和确认两条路径分别测试。
- 降级 admin A，名下 Agent 与 super admin 重名时前端展示冲突列表。
- 冻结 admin A 后，用户 B 仍能在新建项目中看到 admin A 的公共 Agent。
7. 建议给 PR 作者的修改意见
整体方向和实现质量不错，访问控制 helper 抽得清晰，前端只读态也补到了拖拽和按钮粒度。但当前还不能合并，请先处理两个阻塞问题：
- 修复现有测试失败：src/backend/tests/test_plan_assignee_resolution.py 仍在用旧签名 _resolve_assignee_agent_id(..., owner_user_id=...) 调用，PR 已经把签名改为 (db, assignee, project, owner)，4 个测试 TypeError。请更新这些测试，并补充公共 Agent / 历史停用 Agent / 私有公共同名优
先级三种解析场景的覆盖。 - 修复流程模板应用路径：src/backend/routers/process_templates.py:487-490 的 slot 映射查询仍写 Agent.created_by == user.id，会拒绝项目中绑定的管理员公共 Agent。请复用新的 load_usable_agents(...) 或同等可见性 + 项目绑定校验，并在该文件里 grep 一遍 created_by == user.id 的其它出
现，避免遗漏同模式问题。 - 补足关键测试：按设计文档自己列的测试计划至少补齐普通用户/非创建者管理员对公共 Agent 的修改、删除、reset、confirm 应被拒；公共 Agent 跨用户引用时禁止硬删除、可禁用；项目/计划/流程模板三条路径都用公共 Agent 的最小 e2e；降级名称冲突路径；冻结管理员后其公共 Agent 仍可被其他用户使
用。 - 收敛文档：docs/admin-public-agents-design.md 不建议放在 docs/，且内容含“当前分支中未找到...”、“实施顺序建议”等过程性自述。请挪到 proposals/admin-public-agents/plan.md 并删掉过程性语句；或者直接删除，依赖已经更新的 docs/architecture.md 段落。
- 调整 get_mutable_agent 的 404 语义：普通用户对一个能看见的公共 Agent 发起修改时拿到 404 会让人误以为资源不存在。建议先 get_visible_agent，存在但 created_by != user.id 时返回 403 + 文案“公共 Agent 仅创建者可维护”。
- 改进 delete_agent 引用检查：当前全表加载 Project / ProjectPlan 并逐条 JSON 解析，性能曲线不好；selected_agent_ids_json 只识别 int，也可能漏掉历史数字字符串。建议至少兼容数字字符串，并考虑先粗筛或后续结构化关联表。
- 公共 Agent 共享状态的 UI 提示：创建者管理员点 reset/confirm 时建议加提示，告知该操作会同步刷新所有使用该公共 Agent 用户的倒计时/状态。
- 清理 .gitignore 越界改动：src/.env.debug、src/docker-compose.debug.yml、.codex-debug/、debug-*.log 跟本 issue 无关，请拆到独立 PR 或删除。.pytest_cache/ 可保留。
8. 最终建议
暂缓合并。修复两个 Blocker、补足关键路径测试、收敛新增设计文档之后，再来评审一轮。
|
提交pr前请先用这个模版进行review：https://github.com/keting/aicoding/blob/main/docs/prompt-templates/pr-review.md |
|
感谢详细评审，已按意见完成一轮修复和收敛。 本次主要处理了两个 blocker：
同时补充和调整了以下内容：
新增/补强测试覆盖：
已验证： python3 -m pytest src/backend/tests/test_admin_public_agents.py src/backend/tests/test_plan_assignee_resolution.py src/
backend/tests/test_process_templates.py -q
# 48 passed
npm test -- --run agents ProjectNewPage applyTemplatePlan contracts
# 91 passed
npm run build
# passed
git diff --check
# passed
已推送提交：
0f77d40 Fix public agent review blockers |
|
整体实现方向和测试设计都基本符合 issue #5，11 个新测试 + 已有测试全部通过。但有两条合并前必须修： Blocker 2（含产品决策变更）: 评审 owner 对 "停用公共 Agent 在已有项目里能否继续被新计划选用" 做了产品决策变更： Minor: Nit（可选）: access.py:18 get_owned_agent 和 access.py:133 list_owned_agents 重构后无任何调用方，且属于鉴权 helper，留着容易被未来新代码误用回 owner-only 模型；建议顺手删掉。 |
|
已按评审意见完成修改并推送。 主要修复：
验证：
提交： e1c29a9 Fix inactive public agent handling |
2 participants
Closes #5
Summary
Add support for an administrator-managed public Agent pool while keeping regular users' private Agents isolated.
This change introduces derived Agent visibility and permission semantics without adding a database visibility field:
retained for history and compatibility.
The frontend now displays public/private/disabled-public Agent badges and disables edit-only actions for read-only public
Agents. Documentation was updated to describe the new access model and shared public Agent state.
Closes #
Testing
Checklist