fix(canary): enforce module ceiling settlement in the same diff as growth - #4619
Conversation
A PR that grew a module past its reviewed line ceiling, without raising that ceiling in the same diff, previously merged and turned main red until a separate reconciliation PR refreshed the ledger. That split settlement is the structural hole behind loopx-project#4587 -> loopx-project#4618/loopx-project#4619. Add diff_scoped_module_ceiling_violations to the maintainability ratchet and wire it into 'loopx canary premerge' as a direct gate check. The check flags exactly the diff that crossed an inherited ceiling while leaving the head ledger short; growth below the ceiling and in-diff ceiling settlements both stay silent. Signed-off-by: song <liusongstep@gmail.com>
15ea891 to
fc50428
Compare
…chat-actions-20260917 Signed-off-by: song <22676124+songoow@users.noreply.github.com>
`_git_show_text` reads a committed file through `subprocess.run(..., text=True)` with no codec, so on a non-UTF-8 locale it decodes the baseline JSON with the platform default and mangles or raises on any non-ASCII byte. That is the loopx-project#4155 bug `test_shipped_runtime_pins_utf8_for_every_text_mode_subprocess_call` exists to catch, and it caught this one: the check failed on this branch only. Match the form the same module already uses at line 211 and the rest of the runtime uses: `text=True, encoding="utf-8", errors="replace"`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: song <22676124+songoow@users.noreply.github.com>
exact-head 复核(
|
…chat-actions-20260917 Signed-off-by: song <22676124+songoow@users.noreply.github.com>
本 PR 在 #4447 计划中的位置issue #4447 现在有一节统一协调(中英双语),把这 13 个在开 PR 作为一个计划列出:各自修什么、为何必要、以及实测出的合并顺序。 冲突实测:对全部 78 对做了试合并,9 对冲突,分四簇,每一处都是文本相邻,没有一处是语义分歧。
建议顺序(代价从低到高):#4628 → #4625、#4626 → #4627 → #4619、#4621 → #4630 → #4614 → #4631 → #4629 → #4617 → #4606 → #4608。四个棘轮 PR 放最后,因为每落地一个,下一个的数字就从估算变成确定值。 全部 13 个 PR 现已同步到 |
…chat-actions-20260917 Signed-off-by: song <22676124+songoow@users.noreply.github.com>
huangruiteng
left a comment
There was a problem hiding this comment.
Reviewed exact head: e44f11938589e6950d91bb69f25a66e4d65f2b5e (codex/canary-ceiling-chat-actions-20260917).
动机
真正的洞不是「账没记对」,而是「reviewed ceiling 的增长和结算可以被拆进两个 diff」:#4587 让模块越过 ceiling 却不在同一个 diff 里结算,于是 main 变红、挡住所有并发 PR,直到 #4618 用单独的 reconciliation PR 补账。既有整树测试只能回答「现在这棵树是否干净」,无法指出是哪个 diff 越的界,因此失败要由所有人承担。
作者把原 PR 的一行 ledger bump 丢掉(#4618 已合并完全相同的改动),改为承载结构修法——这一点我按仓库规则单独确认过:新增规则回答的是「这个 diff 是否让模块越过它继承的 ceiling」,与既有的「整树是否零 debt」是不同问题,不是重复实现,PR 正文也把分工写清楚了。
改动思路
入口是 loopx canary premerge:build_premerge_validation_gate 已有 changed_files + base_ref,新增的 _module_ceiling_colocation_check 只在 diff 触碰 loopx/*.py 时挂检查,并把判定交给 loopx/canary/maintainability_ratchet.py:diff_scoped_module_ceiling_violations。判定条件三合一:base 处该模块在 ceiling 之内(was_within_budget)、head 行数超过「继承的 ceiling」、且 head ledger 仍没覆盖 head 行数——低于 ceiling 的增长、本 diff 内完成的结算、非 .py/已删除文件全部静默;新文件继承默认上限(1500),越界同样被算作本 diff 造成的越界。
复用上:规则组合了既有的 module_metric_baseline / module_metrics / MODULE_LINE_LIMIT,没有第二份 ledger 或第二个 metrics 提取器;base 处的 ceiling 直接来自 git show,而不是新建一份基线文件。
具体改动
3 个文件、+247/-0:loopx/canary/maintainability_ratchet.py +77(规则函数与两个私有 helper)、loopx/canary/premerge.py +58(检查与挂载)、tests/canary/test_maintainability_ratchet.py +112(三个新共置用例 + import)。
关键代码讲解
diff_scoped_module_ceiling_violations(loopx/canary/maintainability_ratchet.py:626):crossed = was_within_budget and head_lines > base_ceiling,并且仅当head_ceiling < head_lines才算违规——即「本 diff 造成的越界且没在本 diff 结算」。base 里已经超限的模块不会被算到当前 diff 头上(避免把历史债记给作者)。_module_ceiling_colocation_check(loopx/canary/premerge.py:505):无loopx/*.py时返回None(不挂检查,零开销);dry-run 只报ready/ok=True,与其它 direct check 的 dry-run 语义一致;违规时ok=False并提供精确到模块与行数的诊断。build_validation_summary(loopx/canary/premerge.py:656):ok is False计入direct_failures→failure_count/failed_commands,所以这条检查确实会让 premerge 失败,而不只是打印一行说明。
对主干的风险
我独立复现了违规路径:在本 checkout 上给 ledger 已登记的模块 loopx/canary/planner.py(ceiling 1992)临时追加 20 行,examples/control_plane/control-plane-maintainability-ratchet-smoke.py 以 rc=1 报 unreviewed finding: module_metric_budget:loopx/canary/planner.py,同一时刻新规则给出 base_lines 1987, base_ceiling 1992, head_lines 2007, head_ceiling 1992;随后我把文件还原并确认工作树干净。也就是说:检查里对外声明的命令确实能验证同一条件,失败信息不会把人指向一个会变绿的命令。测试 tests/canary/test_maintainability_ratchet.py 13 passed。头部 CI 8 项成功、0 失败,review 时 9 项仍在排队/进行中(无失败),merge_state=BLOCKED 是分支保护状态。
两条非阻塞问题:
- 检查没有异常兜底(P2)。它没有走仓库其它 direct check 都在用的
_run_gate_check,而是手写 check dict 并直接调用规则函数;而module_metric_baseline在 schema 不符时raise ValueError、base 侧是json.loads、subprocess.run(["git", ...])在无 git 时会抛FileNotFoundError。我在临时 git 仓库里探针时就撞上了这条:schema 不符时异常从build_premerge_validation_gate里抛出来,调用方拿不到任何 gate payload。由于这条检查在每个触碰loopx/*.py的 diff 上都会挂载,一次 ledger 异常会把「一个检查失败」升级成「整个 gate 崩掉」。最小修法:把求值包起来(或直接复用_run_gate_check),让内部错误变成ok=False的失败检查并附带命令。 - 检查的形状与其它 direct check 不一致(P3):
kind: "direct_import"是全仓库唯一(其它都是direct_command),且缺少argv/display_argv。目前没有消费者按kind分支,所以不影响行为;但若以后有「按 kind 分组」或「提供重跑命令」的渲染,这条会表现不同。建议统一形状,或至少补上 argv。
我的整体评价
结论 APPROVE。这是一条把「结算与增长拆 diff」这一类事故堵住的结构修法:判定条件保守且方向正确(只归责本 diff 造成的越界、本 diff 结算即静默、不触碰 loopx/*.py 的 PR 零开销),复用了既有 ledger 与 metrics,只新增一个只读的门禁检查;我用真实 checkout 复现了「越界被抓、结算即清除」,并确认失败确实会进入 gate 的失败汇总而不是仅打印。它没有新增 CLI、schema、持久化状态或授权面。
两条问题都不影响这个判断:P2 是异常路径的健壮性(当前需要 ledger 损坏或无 git 才会触发),P3 是形状一致性,最小修法都已写明。按仓库规则,本评审只给出 exact-head 结论;合并就绪度(CI 收敛、BLOCKED 状态)属于另一道关卡。
English verdict: APPROVE - exact head e44f119; the diff-scoped ceiling check correctly charges only the crossing a diff caused (verified on a real checkout: appending 20 lines to a ledger-listed module made the maintainability smoke fail and the new rule report base_lines 1987/base_ceiling 1992/head_lines 2007/head_ceiling 1992, with the checkout restored clean), settles silently when the ledger is updated in the same diff, mounts nothing for non-loopx diffs, and routes failures into the gate's direct-failure summary; 13 ratchet tests pass. Two non-blocking findings: the check bypasses the shared _run_gate_check helper so a malformed ledger (or missing git) raises out of the gate instead of reporting a failed check (P2), and its hand-rolled check dict uses a unique kind with no argv (P3).
动机与范围变更
原 PR 只做一行 ledger 结算(
chat_actions.py.lines1590→1604)。该一行修复已由 #4618 以完全相同的改动合并,upstream/main(3ca868193)现在已是lines: 1604,因此原一行改动冗余,已随 force-push 丢弃。本 PR 现在改为承载结构修法,堵住这一事故类的根:reviewed ceiling 的增长与结算被拆进两个 diff。#4587 让模块越过 ceiling 却没在同一个 diff 里结算,导致 main 变红、挡住所有 PR,直到 #4618/#4619 才用单独的 reconciliation PR 补账。真正的洞不是「账没记对」,而是「结算可以和增长分离」。
改动
loopx/canary/maintainability_ratchet.py(+76)、loopx/canary/premerge.py(+58)、tests/canary/test_maintainability_ratchet.py(+112)。新增
diff_scoped_module_ceiling_violations(repository_root, changed_files, base_ref):对 diff 里每个loopx/*.py,比较「继承的 ceiling(base_ref处的 ledger)」与「head 实测行数」,只有在本 diff 让模块越过其继承 ceiling、且 head ledger 仍没覆盖时才算违规。低于 ceiling 的增长、本 diff 内完成的结算、非.py/已删除文件,全部静默。接入
loopx canary premerge作为直接门禁检查(module_ceiling_colocation):build_premerge_validation_gate已有changed_files+base_ref,直接复用;只在该 diff 触碰loopx/*.py时才挂检查,失败时输出精确到模块与行数的诊断。与整树测试的分工
test_current_repository_debt_is_reviewed_without_line_count_pins是「整棵树零 debt」快照:任何 PR 一越界,所有 PR 的 pytest/merge-gate 一起红(fix(canary): enforce module ceiling settlement in the same diff as growth #4619 正文描述的正是这种全局挡人)。premerge(commit-time)就把失败指到「那个越界 diff」,要求结算与增长同 diff 落地,而不是事后由别人补账。两者不互斥:整树测试仍是 tripwire,共置检查把责任钉到引入 PR。
验证
pytest -q tests/canary/test_maintainability_ratchet.pypytest -q tests/capabilities/test_change_quality.pyruff check(3 个改动文件)base_ref=HEAD)ok: false、gate.status: failed、诊断「grew 5 -> 20 lines past its inherited ceiling 10; settle the ceiling in this diff」;同 diff 结算 →ok: true、status: passed对主干的风险
无运行时 / CLI / API / 权限 / 持久化行为变更。 只新增一个 premerge 直接门禁检查;它读取 checkout 的 ledger 与
base_ref处的 ledger 做 diff 级比较,不写任何状态。对不触碰loopx/*.py的 PR(例如现有app.py相关测试)不挂载、零开销。English verdict: The original one-line ledger bump is now redundant — #4618 merged the identical
chat_actions.py.lines1590→1604 change, so this branch was force-pushed ontoupstream/main(3ca868193) and repurposed to carry the structural fix. The real hole behind #4587 was that a reviewed ceiling's growth and its settlement were split across two diffs, so main went red and blocked every PR until a separate reconciliation PR landed. This PR addsdiff_scoped_module_ceiling_violations(growth past the inherited ceiling atbase_refmust be settled in the same diff) and wires it intoloopx canary premergeas amodule_ceiling_colocationdirect gate check that names the exact module and line counts. Growth below the ceiling and in-diff settlements stay silent; non-.py/deleted files are ignored. Focused tests pass 13/13 (3 new co-location cases) and 20/20 change-quality regressions; end-to-end premerge flipsok:false→ok:truebetween an unsettled and a settled crossing. No runtime/CLI/API/permission/persistence surface is touched.Refs #4587, supersedes the original one-line settlement intent of this PR.
🤖 Generated with pi