refactor(cli): own the refresh-state command in its own module - #4521
huangruiteng merged 1 commit into
Conversation
huangruiteng
left a comment
There was a problem hiding this comment.
动机
loopx/cli_commands/project_lifecycle.py 同时承载了 refresh-state 命令与其它 project lifecycle/archive 命令,refresh-state 因此没有本地 owner,每次改动都要落在一个已经过大的共享模块里。本 PR 把这条命令整体搬进自己的模块,属于"内部移动 + 删除旧入口",而不是新增抽象。
改动思路
register_refresh_state_command 与 handle_refresh_state_command 原样移入新的 project_lifecycle_refresh_state.py(715 行),父模块只保留薄接线:第 30-32 行导入、第 54 行注册、第 205 行派发;旧定义被删除,不保留兼容包装。新增 tests/test_project_lifecycle_refresh_state_ownership.py 固定所有权。
具体改动
loopx/cli_commands/project_lifecycle_refresh_state.py(+715):两个函数的新 owner,register_refresh_state_command在第 73 行、handle_refresh_state_command在第 359 行。loopx/cli_commands/project_lifecycle.py(-690 净减):只留 2 个顶层 def(注册与统一入口),其余改为 import 与派发。tests/test_project_lifecycle_refresh_state_ownership.py(+62):4 个用例固定"命令有自己的模块、父模块不得再定义同名函数"。
关键代码讲解
loopx/cli_commands/project_lifecycle_refresh_state.py:73—register_refresh_state_command:参数面(含--formatadder)随函数一起搬走,父模块通过第 30-32 行导入并在第 54 行调用,因此外部注册入口和命令行行为不变。loopx/cli_commands/project_lifecycle_refresh_state.py:359—handle_refresh_state_command:执行路径(writeback、vision、settlement 分支)整体搬迁;父模块第 205 行仍以同一签名派发,调用方无需知道模块被拆分。loopx/cli_commands/project_lifecycle.py:30— 仅剩 2 个顶层 def(我按rg -c '^def '核对:父模块 2、新模块 2),说明这是移动而不是复制,旧 owner 已退役;这一点是"删除旧入口"这条规则的直接证据。
对主干的风险
没有阻塞项。 我核对过:两个函数在全仓各自只有一处定义且都在新模块;父模块保留注册与派发入口;tests/test_state_refresh_agent_lane_action.py + tests/test_state_refresh_projections.py 共 12 passed,ownership 测试 4 passed;与 origin/main(merge base 4aaad69bd)merge-tree 干净。
残余风险(已写入 result,不是隐藏项):ownership 测试只能看"定义在哪里",看不出 715 行搬迁中的人为改动,因此我用 refresh-state 行为测试面来覆盖语义,而不是只跑 ownership 测试;我没有做一次对真实 goal state 的端到端 CLI 调用,这一层证据是缺的。这一条不构成阻塞(移动本身是机械的,且父模块入口与签名未变),但属于未来若要再动这条命令时应补的验证。
我的整体评价
APPROVE。这是"移动到正确 bounded context 并删除旧入口"的标准做法:命令有了自己的 owner,热文件缩小约 690 行,父模块只留接线,且没有留下兼容包装或第二份定义——仓库纪律(Capability And Extension Placement、Internals move 两条)都满足了。我用行为测试而不是只靠所有权测试来确认语义未变,这是这次评审里我实际投入的地方;没有发现需要作者再改一轮的问题。
English verdict: APPROVE — exact head db7db205191ca2e6a70fe1a9488278b117a7c46d of #4521. The refresh-state command now has a single owning module: both register_refresh_state_command (project_lifecycle_refresh_state.py:73) and handle_refresh_state_command (:359) are defined exactly once there, while project_lifecycle.py keeps the public registration entry point at :30-32/:54 and the dispatch at :205, so callers are unaffected and the old definitions are deleted rather than duplicated. Validation at this head: 12 refresh-state tests (agent-lane action and projections) and 4 ownership tests pass, and git merge-tree against current main is clean. No blocking findings; the residual risk is that an edit inside the 715 moved lines would not be visible to the ownership test, which the behaviour suites mitigate but a live end-to-end refresh run does not cover.
Refs GH-C06 cli_commands/project_lifecycle.py was 972 lines against the 1000-line default budget enforced by the module size/ownership smoke, and it is not one of the legacy owners frozen in STARTER_MODULE_LIMITS. It was the tightest non-frozen seam left. refresh-state is one command and one rule group, so its parser block and its dispatch branch move together into cli_commands/project_lifecycle_refresh_state.py. project_lifecycle.py delegates to both and drops to 314 lines; the new module is 715. No budget entry was added, so the limit reflects real extraction rather than a raised ceiling. No compatibility wrapper is left behind: nothing is re-exported for callers that do not exist, and the imports the moved code no longer needs are removed from project_lifecycle.py. Public invocation is unchanged. `loopx refresh-state --help` and `loopx --help` are byte-identical to a28562e, and the four real refresh-state smokes pass. Signed-off-by: YZJF <195568136+YZJF@users.noreply.github.com>
db7db20 to
75cad50
Compare
…tate (#4534) `examples/cli-project-lifecycle-command-modularization-smoke.py` fails on `main` with `project lifecycle module missing retry it before delivery`. The marker is not missing: #4521 moved the `refresh-state` command into `loopx/cli_commands/project_lifecycle_refresh_state.py`, which is where the string now lives, while the smoke still required it in the dispatcher module. The check now requires that marker, and the `refresh-state` command name, in the module that owns the command, and keeps requiring the dispatcher markers where they still are. Nothing is weakened: the marker is still required, and a regression that drops it from the refresh-state module still fails. This red blocked the pre-merge gate for any diff that selected the smoke, which is why it is worth fixing rather than working around. Verified: examples/cli-project-lifecycle-command-modularization-smoke.py ok (it was the failing check). Signed-off-by: huangruiteng <14976749+huangruiteng@users.noreply.github.com> Co-authored-by: huangruiteng <14976749+huangruiteng@users.noreply.github.com>
huangruiteng
left a comment
There was a problem hiding this comment.
动机
这行任务(GH-C06)要的是"再挑一个仍然过大的 CLI owner 接缝,只把一个内聚的命令或规则组搬进它自己的模块"。作者先做了度量再动手:在 a28562e97 上,cli_commands/project_lifecycle.py 是 972 行、对着 smoke 的 1000 行默认预算只剩 28 行余量,而且不在 STARTER_MODULE_LIMITS 的冻结名单里,所以它确实是最紧的非冻结 owner。这个判断是对的:再涨一次就会撞预算,而那时的选择只剩"抬预算"或"赶工搬代码",而 project_lifecycle_inputs.py / project_lifecycle_sinks.py 已经确立了这条拆分方向。
改动思路
搬的单位选得准:refresh-state 的注册块与 dispatch 分支是一对,一起搬才不会出现"parser 在新模块、分支在旧模块"的半拆状态;project_lifecycle.py 只 import 两个函数并委派,依赖方向单向(旧 → 新),没有反向 import。
两点我特别认可:
- 不留兼容外壳。 没有把
refresh_state_run之类再 export 回旧模块——这正是后来 #4565 拒绝用 re-export 修测试的理由(在旧模块上装的替身永远不会被调用,测试会"通过但什么都没验")。 - 不抬预算。 没有往
STARTER_MODULE_LIMITS加冻结项,而是让project_lifecycle.py真的回到 314 行。守卫保持诚实,比"把现状写进豁免表"有价值得多。
具体改动
新增 loopx/cli_commands/project_lifecycle_refresh_state.py(715 行,含 register_refresh_state_command 与 handle_refresh_state_command),project_lifecycle.py 从 972 降到 314 行并只保留委派与其余命令,新增 tests/test_project_lifecycle_refresh_state_ownership.py 固定"仍属于 project lifecycle / 只注册一次 / 其他命令原样穿透"。两文件 +793/-674,全部是搬移加一个 early-return。
我自己做了两遍独立的行为等价验证(都在独立 worktree 上,base 用 df47d07b):
loopx refresh-state --help两版逐字节相同(205 行,diff 为空)。- 同一 fixture 下
--dry-run的 JSON payload 去掉路径与时间戳后完全相同,唯一差异是global_sync.updated_at这个墙钟时间。 - 真跑一次(非 dry-run)
refresh-state --classification validated_progress ...:ok=true、appended=true、请求的 classification 生效、磁盘上正好一条 run。
守卫与静态检查:examples/cli-command-module-size-ownership-command-modularization-smoke.py → ok(新模块 715 行、旧模块 314 行,且没有命令被两个模块注册);ruff check → All checks passed;git diff --check 干净;新增的 ownership 测试与 tests/test_state_refresh_agent_lane_action.py 通过。
P2(非阻塞,但属实质发现):这次搬移把 refresh-state 的协作者(refresh_state_run、sync_human_gate_after_refresh、sync_explore_graph_after_material_refresh、read_heartbeat_settlement、settlement_result_payload、resolve_runtime_root)一起带走了,于是 project_lifecycle 不再持有这些名字;而 tests/cli_commands/test_project_lifecycle_goal_channel.py 仍然在旧模块上 monkeypatch.setattr,9 个用例在断言之前就 AttributeError。我在 head 与 base 各跑了一次同一个文件来归因:head 9 failed(module 'loopx.cli_commands.project_lifecycle' has no attribute 'refresh_state_run'),base 9 passed。main 的 Python Tests 因此变红,直到 #4565 把 19 处 patch 目标改到真正的调用模块才修好;那条修复的判断也是对的(不能用 re-export 让它"假通过")。生产行为不受影响(上面两遍等价验证),所以这是测试/CI 回归而非功能回归,且已在上游修复;PR 正文的验证清单里没有这个文件,这属于"搬 owner 时,patch 它的测试也是被搬动的表面"。
P3(非阻塞):新增 ownership 测试里的"只注册一次"扫描,与 size smoke 已有的 duplicate_registrations 检查(104-111 行,失败信息 "commands registered by multiple cli_commands modules")重复且更窄,容易与它漂移;测试里真正新增价值的部分是 PROJECT_LIFECYCLE_COMMANDS 归属、两个导出函数、以及其他命令的穿透。
对主干的风险
风险在搬移类改动里算低,而且我用两种独立方式验证过公开面等价:help 逐字节相同、dry-run payload 相同、真跑仍只追加一条 run;命令只注册一次、依赖方向单向、没有 re-export 造成的双 owner。没有新增 flag、默认值、状态或权限。
需要留意的就是上面那条 P2:私有模块路径变了,凡是按旧路径绑定协作者的测试/工具都会立刻炸——好在失败是响亮的 AttributeError 而不是静默失配,这也是搬移类改动里更好的失败模式。生产路径本身没有发现任何不等价之处。
我的整体评价
这是一次干净、可回滚的结构性搬移:单位是一个命令的 parser+dispatch 这一对,旧模块只委派、无反向依赖,不留兼容外壳、不抬预算,并且给出了 help 逐字节相同这类可核验的等价证据。我把这些说法逐条复跑过,结论一致;size smoke 与 ruff 也干净。
唯一实质问题是它把 patch 这些协作者的测试留在了旧路径上,导致 main 的 Python Tests 变红(9 个用例),随后由 #4565 正确修复;这不影响合并结论,但值得记在审查记录里:搬动命令 owner 时,按旧路径绑定协作者的测试属于同一次搬动的表面。
English verdict: APPROVE (exact head 75cad50)
Refs GH-C06 — contributor task board row: "Characterize one remaining oversized CLI ownership seam after the recent quota, status, todo, history, and scheduler command-plumbing extractions, then move only a cohesive command or rule group into its bounded module."
Which seam, and why this one
The board's focused issue for the previous cut (#3710, Goal Channel runtime CLI ownership) is closed, so this is the next seam.
examples/cli-command-module-size-ownership-command-modularization-smoke.pysets a default budget of 1000 lines, with a smallSTARTER_MODULE_LIMITStable that freezes a few legacy owners (turn.py1114,todo.py1098, and thestarter_*modules) at their current baseline. Ata28562e97the module closest to the default budget while not being in that freeze list was:cli_commands/project_lifecycle.pycli_commands/quota.pycli_commands/support_control.pyproject_lifecycle.pywas the tightest non-frozen seam. It had already shed helpers intoproject_lifecycle_inputs.pyandproject_lifecycle_sinks.py, so extracting a command owner follows the module's own established direction.What moved
refresh-state— one command, one rule group. Its parser registration and its dispatch branch are a matched pair, so they moved together intocli_commands/project_lifecycle_refresh_state.py:register_refresh_state_command(subparsers, add_subcommand_format)— the wholerefresh_state_parserblock.handle_refresh_state_command(args, ...)— the wholeif args.command == "refresh-state"branch, now guarded by an earlyreturn Nonefor other commands so the caller can fall through.project_lifecycle.pydelegates to both. Result:project_lifecycle.pyproject_lifecycle_refresh_state.pyBoth are now well inside the default budget. No new entry was added to
STARTER_MODULE_LIMITS— the budget stays honest rather than being raised to accommodate the code that was already there.Scope discipline
project_lifecycle.pyimports exactly the two functions it calls. Unused imports left behind by the move were removed rather than retained.project_lifecycle→project_lifecycle_refresh_state, with no import back.Verification
Public invocation is unchanged:
loopx refresh-state --helpandloopx --helpoutput are byte-identical before and after (diffed againsta28562e97).refresh-state-agent-lane-scope-smoke,refresh-state-shared-runtime-projection-smoke,refresh-state-unique-run-path-smoke,refresh-state-write-correctness-smoke.examples/cli-command-module-size-ownership-command-modularization-smoke.py→ ok (this is the guard the row names; it also re-checks that no command is registered by two modules).tests/test_project_lifecycle_refresh_state_ownership.pypins thatrefresh-statestays inPROJECT_LIFECYCLE_COMMANDS, is registered exactly once, and that the dispatch falls through for other commands. 16 passed with the neighbouring state-refresh suites.ruff checkclean on all changed files.regression/cli-command-module-contract.pyfails on this machine both before and after this change: it shells out toloopx doctor, which exits 1 in this sandbox. Unrelated to the move; noted for transparency.Base
a28562e97, Python 3.13.12.