refactor(cli): own the promotion control commands in their own module - #4644
huangruiteng merged 1 commit into
Conversation
`support_control.py` registered seven unrelated top-level commands in one 898-line module. Move `promotion-gate`, `promotion-readiness` (with its `record` subcommand), and `upgrade-plan` into `cli_commands/support_control_promotion.py` so one cohesive canary-promotion and upgrade-propagation group has a bounded owner. Registration and dispatch move together, the three commands stay inside `SUPPORT_CONTROL_COMMANDS`, and no public invocation, flag, payload, or exit code changes: `promotion-gate` and `upgrade-plan` emit byte-identical JSON before and after, and `promotion-readiness record` differs only in its `generated_at` timestamp. Refs loopx-project#4639, GH-C06 Signed-off-by: YZJF <195568136+YZJF@users.noreply.github.com>
huangruiteng
left a comment
There was a problem hiding this comment.
Reviewed exact head: 29d243a3b6b209c5491c79ad269b877fee98a54f (refactor/support-control-promotion-ownership).
动机
GH-C06(#4639)要求把最后一个「过大且无主」的 CLI ownership 边界拆分掉:loopx/cli_commands/support_control.py 一个模块注册了七个互不相关的顶层命令,任何新增的 promotion/upgrade 参数都会长在这条没有主人的公共缝上,review 也得在无关的 parser 块之间来回读。
改动后:support_control.py 从 898 行降到 758 行(仍在仓库 1000 行默认预算内,且为剩余分组留出余量),三个命令归入新的 201 行 owner 模块。这不是「先拆再说」:仓库的 bounded refactoring 规则要求在相邻边界能让下一次改动更容易时做这类行为保持的搬迁,而 cli_commands 下已经有同样的按组合成模块先例(backup、chat endpoint、heartbeat registration、registry、supervisor 等),所以这是既有形状而不是新抽象。
改动思路
入口是 loopx/cli.py:264 的 register_support_control_commands 与 support_control.handle_support_control_command。新模块同时拥有「注册」与「分发」两半:register_promotion_control_commands(parser 与三个命令的参数)与 handle_promotion_control_command(三个分支及其原有 try/except 兜底 payload),并用 PROMOTION_CONTROL_COMMANDS 表达它拥有哪些命令;support_control.py 改为一次委托调用,位置正好是原来那三条内联分支所在处(supervisor 分发之后),所以分发顺序语义不变。
我核对了「复用还是新建」:PrintPayload 这个局部类型别名在 turn.py / version.py / workflow_skills.py / turn_inspection.py / worker_bridge.py 里都是各模块自己声明,所以新模块再声明一次符合既有约定,不是重复知识;真正需要留意的重复只有一处(见风险一节的 P2)。
具体改动
3 个文件、+284/-153:support_control.py 净减 140 行(只留一个注册委托与一个分发委托,并移除已随代码迁走的 promotion/upgrade 导入),新增 support_control_promotion.py(201 行,含模块 docstring 说明分组理由与 GH-C06 出处),新增 tests/test_support_control_promotion_ownership.py(70 行)。
我没有相信「搬了就是没变」:分别在 main 与 head 上把整棵 argparse 树导出(命令、option_strings、dest、default、required、choices、nargs)并比较——除默认值里嵌的 checkout 路径外完全相同(两侧各 655983 字节),promotion 分组切片也完全相同(2835 字节);四个 help 面(promotion-gate、promotion-readiness、promotion-readiness record、upgrade-plan)逐字节相同;promotion-gate 的 JSON 只在 age_seconds/freshness_reference_time 上不同,upgrade-plan 的 JSON 逐字节相同(35754 字节)。
关键代码讲解
register_promotion_control_commands:三个 parser 及其 flag/choices/help 原样搬迁,--dashboard-readiness的choices=("passed","skipped")、--mode的["thin","brief","compact"]、--goal-id的action="append"等全部与 main 一致(由 parser 树对比背书)。handle_promotion_control_command:先做args.command not in PROMOTION_CONTROL_COMMANDS的提前返回(新测试用args.command="update"钉住「非本组命令必须先原样落空」),再分派三个命令;三段 except 兜底 payload 逐字保留,所以缺 registry 等失败仍是同样的ok=false + recommended_action形态与退出码 1。support_control.handle_support_control_command:promotion_result = handle_promotion_control_command(...)后if promotion_result is not None: return promotion_result,与删除的内联分支等价。tests/test_support_control_promotion_ownership.py:把「仍在 SUPPORT_CONTROL_COMMANDS」「每个命令只注册一次」「模块暴露两半」「模块只拥有本组」「其它命令落空」这五件事固定下来。
对主干的风险
我跑了:新测试 5 passed;examples/cli-command-module-size-ownership-command-modularization-smoke.py → ok;ruff check 三个文件 → All checks passed。头部 CI 15 项成功、0 失败,review 时 4 项仍在进行/排队(test-shard 3/4、checks、stage2c-correctness-e2e),merge_state=BEHIND 是待更新分支而非冲突。
最强回归场景不是崩溃而是「CLI 契约被悄悄改变」:某个 flag 丢了 default/choices、某个子命令不再注册、或分发顺序变化导致处理到错误分支——用户只会在 promotion/upgrade 跑失败时才发现。我会用整棵 parser 树对比而不是只跑测试,正是因为新测试钉的是注册与落空,不会发现搬动文本里的 default 变化。
一条非阻塞 P2:命令「谁注册」这条规则现在有两处实现。既有的 ownership smoke(examples/cli-command-module-size-ownership-command-modularization-smoke.py)本来就会因为「同一命令被多个模块注册」而失败,并有 STARTER_COMMAND_OWNERS 这张「命令→模块」表;新测试又自己写了一份正则扫描(subparsers.add_parser("x")来做同类断言。差别在于那张表里没有这三个 promotion 命令——真正缺的只是把三行登记进去。建议把三条命令加进 STARTER_COMMAND_OWNERS(让既有唯一 owner 覆盖它们),新测试只保留 smoke 表达不了的部分:本组仍在 SUPPORT_CONTROL_COMMANDS、模块暴露注册与分发两半、PROMOTION_CONTROL_COMMANDS 恰为本组、非本组命令落空。这样规则只有一处实现,也不会在将来扫描口径变化时漂移。
另外两条我确认不是问题,记下来避免下次重复讨论:一是 PrintPayload 局部别名属于仓库既有每模块约定;二是「文件本来就 <1000 行」不构成拒绝理由——仓库的 bounded refactoring 规则正是为这种相邻边界搬迁而写,且本次搬迁零新抽象、单 commit 可回退。
我的整体评价
结论 APPROVE。这是一个可验证的行为保持搬迁:整棵 CLI parser 树、四个 help 面、以及两个命令的 JSON payload 我都独立对比过,结论与作者声明一致(promotion-gate 仅时间戳不同,upgrade-plan 逐字节相同);注册与分发两半一起搬、公共调用面与 SUPPORT_CONTROL_COMMANDS 未变、失败兜底 payload 逐字保留,回退成本是一个 commit。它让 remaining 分组重新获得预算空间,符合仓库「缩小模块 owner」而非「新增抽象层」的方向。
唯一的 P2 是测试侧的 ownership 规则重复实现(且既有 owner 的表里缺这三行),属于维护性问题,不影响本次判断。按仓库规则这是 CLI/生产代码改动,因此本评审只给出 exact-head 结论;合并就绪度(CI 收敛、BEHIND 更新)属于另一道关卡。
English verdict: APPROVE - exact head 29d243a; the promotion command group moves registration and dispatch together into its own module and I verified the surface independently (entire argparse tree identical apart from the embedded checkout path, four help texts identical, promotion-gate JSON differing only in its freshness timestamp, upgrade-plan JSON byte-identical, 5 new tests plus the ownership smoke and ruff clean). One non-blocking P2: the new test re-implements the command-ownership scan that the cli-command-module-size-ownership smoke already owns, so the three commands should be registered in its STARTER_COMMAND_OWNERS map instead.
Goal And Delivered Outcome
loopx/cli_commands/support_control.pyregistered seven unrelated top-level commands in one 898-line module, so any new promotion flag grew a shared seam with no owner.promotion-gate,promotion-readiness(with itsrecordsubcommand) andupgrade-planwere registered and dispatched fromsupport_control.py; after,loopx/cli_commands/support_control_promotion.pyowns both halves,support_control.pydelegates, and no public invocation, flag, payload, exit code or help text changes —promotion-gateandupgrade-planemit byte-identical JSON, andpromotion-readiness recorddiffers only in itsgenerated_attimestamp.main. This replaces refactor(cli): own the promotion control commands in their own module #4642, which carried the same change from a different fork and has been closed so upstream keeps one PR.Scope And Continuation
support_control.pystill owns its remaining groups; further extraction is a separate slice.SUPPORT_CONTROL_COMMANDS, and the extraction is reversible as a single commit.Validation
origin/maine1a97c2fdfinishedsyntheticunitpassedpython -m pytest tests/test_support_control_promotion_ownership.py— 5 passed: commands stay inSUPPORT_CONTROL_COMMANDS, each registered exactly once, owner module exposes registration and dispatch, owns exactly its group, and a non-promotion command falls through untouchedregression_paritypassedpython -m pytest tests/test_support_control_promotion_ownership.py tests/test_dashboard_command.py— 42 passed, unchanged before/afterreal_entrypointpassedpython examples/cli-command-module-size-ownership-command-modularization-smoke.py— okstaticpassedruff checkclean on the new test file; CI lintstestsand does not includeloopx/cli_commandssupport_control.py.Frontend / Visual Evidence
Type of Change
LoopX Area
Technical Direction