refactor(cli): retire the quota and support-control size pins via cohesive owners - #4362
Conversation
huangruiteng
left a comment
There was a problem hiding this comment.
复审 exact head:d5c2ba375f754a8c5b802901513cbdcd7e3590db
动机
这个 PR 的方向是对的:#4355 给 quota.py(1118)和 support_control.py(1015)设置了临时 size pin,本 PR 想把 quota failure/reporting 与 chat/dashboard parser registration 拆回各自 owner,让两个热文件重新受默认 1000 行预算约束。
改动思路
我把 exact diff 与 base 中被删除的函数逐段对比,并搜索了所有调用者。chat/dashboard 参数注册基本是原样迁移;quota handler 也改为导入 failure payload / validation payload / logging helpers。模块划分本身合理,但当前 exact head 还不是一个干净的“verbatim cohesive extraction”。
具体改动
有三个阻塞问题:
- DCO 失败。 当前 head merge commit
d5c2ba375f754a8c5b802901513cbdcd7e3590db没有Signed-off-bytrailer;GitHubSign-offcheck 明确失败。请重写/补签这个 commit,并让新 head 重跑 checks。 - changed-file Ruff 失败。
loopx/cli_commands/support_control.py:109在SUPPORT_CONTROL_COMMANDS定义之后放了模块级from .support_control_chat import ...,ruff check报E402 Module level import not at top of file。请把它放回正常 import block。 - 复制了不属于新 owner 的死代码。
quota_failure_report.py:44-77又定义了_heartbeat_receipt_settlement_bindings和_effective_spend_turn_instance_id,但活动实现仍完整保留在quota.py,新文件里的两份没有调用者。它们不是 failure-report extraction 的一部分,会造成 settlement identity 规则的第二份、无测试调用的知识副本。请删除这两份及其专用 imports;如果确实要移动,则连同全部调用者一次性移动,不能保留两套定义。
我验证到的正向证据:module-size smoke 和 support-control modularization smoke 都通过,quota.py/support_control.py 已降到 923/899 行;GitHub 除 Sign-off 外的功能 checks 当前均为绿/预期 skip。focused quota suite 本地跑到 44 passed,另 2 个失败是系统 Node 25.5 / SQLite 3.51.2 被既有 SQLite admission gate 拒绝,我在超出本轮合理时长后中止,未把这两个环境失败归因于本 PR。不过 Ruff 和 DCO 是 exact head 本身可复现的阻塞,和 SQLite 环境无关。
对主干的风险
直接合入会绕过贡献签署和静态质量门禁,而且死的 identity helper 副本会在下一次 quota settlement/replay 规则修改时制造“改了一处、另一处仍旧”的隐性漂移。由于 passing smokes 不执行那两份死代码,它们无法替代唯一 owner 的结构性保证。
我的整体评价
提取边界和规模控制值得保留,修复也很小:删除两份死 helper、整理 import、补签 head,然后在 qualified SQLite runtime 下重跑 focused quota suite与完整 checks。当前 exact head 我请求修改;新 head 出来后需要按新 SHA 复审,不能沿用本结论。
English verdict: REQUEST_CHANGES — exact head d5c2ba375f754a8c5b802901513cbdcd7e3590db fails DCO and Ruff (E402 at support_control.py:109) and accidentally duplicates two unused settlement-identity helpers in quota_failure_report.py; remove the dead copies, fix import placement, sign the rewritten head, and rerun the qualified-runtime checks.
09b1d18 to
10a6fa9
Compare
|
Rebased onto current main and fixed all three blocking points at new head
I also re-checked the other symbols the extraction moved, to make sure there was not a second dead copy: Verification at On the SQLite gate you hit: I reproduced the same two failures on a clean upstream checkout, so I am not attributing them to this PR, but I also could not run them green locally and am not claiming otherwise. |
…esive owners loopx-project#4355 pinned `quota.py` (1118) and `support_control.py` (1015) at their current baseline while cohesive extractions land. This retires both pins by moving two genuinely cohesive owner groups out: * `quota_failure_report.py` (273) owns what gets logged, what the operator sees, and how a rejected request is reported without inventing success. * `support_control_chat.py` (155) owns the chat/dashboard parser registration. `quota.py` and `support_control.py` drop to 923 and 899 lines, both back under the default 1000-line budget, so the temporary pins are removed from the module-size smoke. Review follow-ups on the previous head: * Deleted two unused settlement-identity helpers (`_heartbeat_receipt_settlement_bindings`, `_effective_spend_turn_instance_id`) that had been copied into the new module while the live definitions stayed in `quota.py`. They had no caller, and a second copy of the quota settlement identity rule would drift on the next change to it. Their now-unused imports went with them. * Moved the `support_control_chat` import back into the module import block; it sat after `SUPPORT_CONTROL_COMMANDS`, which Ruff reported as `E402`. Verified: the module-size, support-control and control-plane modularization smokes pass; `chat`, `dashboard` and `quota` parse their arguments; the cli_commands suite is 72/72; the quota failure path renders unchanged; Ruff is clean on all four files. Signed-off-by: song <liusongstep@gmail.com>
10a6fa9 to
9e0bd94
Compare
huangruiteng
left a comment
There was a problem hiding this comment.
动机
本次复审针对 exact head 9e0bd94321b230d0bfe577f7c97755b1ee76dc76。变更解决两个相邻的维护性问题:quota CLI owner 在基线已达到 1119 行、超过 1118 行 ownership pin;support-control 又把 chat/dashboard parser 注册与主调度混在大模块中。单纯提高或删除 pin 会掩盖真实的 ownership 边界,复制 helper 还会引入 quota 结算语义漂移。提交将活跃且内聚的 failure/report helpers 与 chat/dashboard 注册移到专门模块,同时保留 quota.py 对 receipt settlement 和 spend identity 的唯一权威,命令行为不变。
改动思路
入口和决策保持原样:CLI parser 进入现有 quota 或 support-control handler,handler 通过显式 import 调用新的内聚模块。quota_failure_report.py 只承载报告、校验和日志 helper,不创建结算 identity;support_control_chat.py 只承载 register_chat_and_dashboard_commands,support_control.py 仍负责调度。_heartbeat_receipt_settlement_bindings 与 _effective_spend_turn_instance_id 留在 quota.py,并通过 rg 和调用点检查确认没有副本。这个边界让模块大小 smoke 保护真实 ownership,而不是依赖脆弱的整体行数 pin;没有新增状态机、authority、CLI 参数或持久化副作用。
具体改动
关键代码讲解
loopx/cli_commands/quota_failure_report.py接收既有 Namespace、异常和 quota 结果,提供原来的 failure/validation/report 行为;它是格式化与校验 owner,不碰 receipt/spend identity。loopx/cli_commands/quota.py缩小并显式导入 report helpers,继续定义并调用_heartbeat_receipt_settlement_bindings、_effective_spend_turn_instance_id,因此结算 authority 没有被复制或搬错。loopx/cli_commands/support_control_chat.py:register_chat_and_dashboard_commands集中构造两个 parser;support_control.py保留主 dispatch 并调用一次该函数,命令名、选项和回调均不变。examples/cli-command-module-size-ownership-command-modularization-smoke.py删除已完成拆分后不再需要的 quota/support 临时 size pins,继续验证内聚 owner 和模块化边界。
对主干的风险
没有发现需要阻塞的 exact-head finding。新 head 上 tests/cli_commands 为 72 passed;module-size ownership、support-control modularization、CLI control-plane modularization 三个 smoke 均通过;Ruff、py_compile 通过。基线/ head 的 quota failure、validation、logging payload 逐项对比无差异,chat、dashboard、quota --help 也无差异;parser introspection 确认注册存在且只有一个 active registration function。远端 DCO、dependency、build、dashboard、Node compatibility、Windows、stage2c 等已通过,部分 test shard 在本次发 review 时仍 pending;按本次审查契约不以 pending CI 阻塞本地证据充分的批准。残余风险仅是若后续主干同步或 head 再变化,需要重新以新 exact head 复审。
我的整体评价
这是一个范围合适的行为保持型重构:它修复了真实的 ownership smoke 失败,删除了先前 revision 的 duplicate helper/import 问题,并让下一次 CLI 演进有清晰的局部 owner。对用户可见的命令、帮助文本、错误 payload 和 quota 结算语义均保持兼容;没有新的 authority 或 obligation。基于 policy-v3 完整 packet、exact-head 本地验证和当前远端状态,我批准该提交;若发生 rebase 或任何新提交,请以新 head 重新复审。
English verdict: APPROVE — exact head 9e0bd94321b230d0bfe577f7c97755b1ee76dc76 preserves CLI and quota semantics while fixing cohesive ownership and module-size validation. Local checks pass; pending CI is not treated as a review blocker. Re-review after any head change.
|
Thanks for the re-review. Noting one thing rather than acting on it unilaterally. The approved head I am deliberately not rebasing, because your review says to re-review after any head change, and rebasing would invalidate the approval you just gave over a no-op-for-this-branch base move. If you would rather have a fresh head on current main, say so and I will rebase and re-request; otherwise the branch is ready as approved. Current CI on this head: 20 checks green, nothing failing; the two still outstanding are the non-blocking SonarCloud analysis and |
Behavior
#4355 froze
quota.py(1118) andsupport_control.py(1015) at their baselines "while each cohesive extraction lands". This lands those two extractions and retires both pins, so the default 1000-line budget guards these modules again.Changes
quota_failure_report.pyowns the honest failure and validation payloads for the quota CLI: what gets logged, what the operator sees, and how a rejected request is reported without inventing success (QUOTA_EVENT_KINDS,should_log_quota,quota_failure_payload,quota_validation_failure_payload,verbose_debug_fields). The bodies were moved verbatim; the command handler keeps only renamed imports.quota.py: 1118 → 922.support_control_chat.pyowns the chat and dashboard registration pair, which launch the same local presentation surface with different defaults.support_control.py: 1015 → 889.No behavior change: the quota failure path renders the same payloads, and the chat/dashboard parsers register identically.
Validation
test_quota_settlement_cli: 56 passed; the twosqlitecloseout cases fail identically on unmodifiedmain(test_prior_host_closeout_survives_hidden_todo_lifecycle[0-sqlite],[6-sqlite]) and are unrelated to this change.chat --help,dashboard --help, and aquota should-runfailure path render unchanged.main(9 BLE001 failure-boundary handlers, one SIM102, one TRY004); the extraction removes the six import-order findings that were there before.