refactor(chat): remove inherited module-budget regressions - #4793
Conversation
…odes Signed-off-by: song <22676124+songoow@users.noreply.github.com>
Signed-off-by: song <22676124+songoow@users.noreply.github.com>
Signed-off-by: song <22676124+songoow@users.noreply.github.com>
…get-dedup Signed-off-by: song <22676124+songoow@users.noreply.github.com>
|
This PR is currently the only external dependency blocking #4790 from syncing with I locally combined #4790 ( All checks on this PR are green. Once this lands, I can merge the resulting |
Pull in the independently validated main-branch repairs for the module budget ratchet and time-dependent Lark context fixtures so this feature branch can exercise its own checks without duplicating the fixes. Signed-off-by: huangruiteng <14976749+huangruiteng@users.noreply.github.com>
huangruiteng
left a comment
There was a problem hiding this comment.
精确 head b28af169f271eb9997b07730c1f034d58cccb827 评审
动机
当前 main 的维护性 ratchet 会稳定报告两个未评审的 module-budget finding:loopx/chat_server.py 和 loopx/extensions/lark/goal_topic_connections.py。这不是需要放宽预算的新增产品能力,而是两个现有热模块积累了可删除的重复规则,并已经让继承该基线的 PR 无法通过完整验证。
这个 PR 完成了一个独立且有用的维护切片:保留 Chat action HTTP 与 Lark routing 的现有外部行为,通过收拢重复 owner 把模块重新降到既有预算内;同时把两个日期固定的 Lark 测试锚定到与 fixture 一致的测试时钟,不改变生产七天保留策略。
改动思路
权威关系保持不变。Chat action 是否存在仍由 ChatActionService / store 决定,handler 只统一渲染既有 404;Lark routing 的合法值和默认值仍由 CaptureScope、IngressMode、ReplyMode 与 _routing_value 决定,connection readback 和 event decision 共同调用一个私有 normalization helper。
最强的反对理由是:直接更新 module budget 会更小,而且没有任何 production diff。但那会保留两份 routing 默认/校验规则和多份 Chat missing-action 响应,下一次变更仍需同步修改。当前方案没有新增状态、schema、CLI、provider 或 public protocol,production 净减少 21 行,因此比提高预算更符合这个 regression budget 的用途。
具体改动
关键代码讲解
ChatRequestHandler._action_not_found集中既有404 / action_not_found响应。snapshot、apply、cancel、regenerate、reject、defer 仍各自拥有原来的 lookup/transition 分支,只有响应渲染被去重。_connection_routing_modes保留原来的 precedence:显式capture_scope优先;缺失时仅由 legacyincoming_mode=all推导configured_chat_all;ingress/reply 继续使用既有 enum 默认和 rejection。list_lark_connections与decide_lark_topic_event是两个真实 caller。参数化测试用同一份 file-backed binding 比较 readback 和 route decision,并覆盖默认值、legacy 兼容、大小写/空白 normalization,以及三个非法字段。manager_context_clock只在两个 dated test 内替换manager_context.datetime,让 synthetic event 留在现有 retention window 中;它没有关闭 compaction,也没有扩大生产 retention。
对主干的风险
最需要防的是 helper extraction 改变 legacy routing precedence,导致列表显示一个模式而 event router 使用另一个模式。为此我在 detached base 4eb6ea9eed3a2e374933b233b62bd46419046263 和 exact head 上运行了同一套 exact-head behavior tests:两边均为 199 passed。这些用例经过真实本地 Chat HTTP server、file action store、file-backed Lark binding,以及 production readback/decision 函数;invalid routing 还验证 binding bytes 不变,missing action 验证 store 没有新增记录。
Regression sensitivity 也成立:同一 maintainability ratchet 在 base 为 1 failed, 12 passed,精确指出上述两个 module-budget finding;在 exact head 为 13 passed。Exact-head focused 总验证为 212 passed,六个 changed files 的 Ruff 与 git diff --check 均通过。远端 checks 当前也为 green,但本结论依据 capability packet 要求的本地 exact-head evidence,并未把远端 CI 当成 approval 证明。
语义与 CI 对齐
本次复用现有 Chat error、Lark enum/default 和 module-budget vocabulary,没有新建或扩张 authority/actor semantics,也没有 default-off、guidance/obligation 或 goal-specific contract 变化。行为差异仅是模块尺寸从 1525/1542 降到 1516/1499,以及测试时钟与 synthetic fixture 对齐。
我的整体评价
没有 blocking finding。这个 diff 把确实重复的规则移到最近的既有 owner,保留完整 public behavior 和失败语义,并用 base/head real-path comparison 证明不是通过提高预算或缩小 scanner 来隐藏回归。范围与问题成比例,且可以无迁移回滚。
残余风险是 PR 当前落后于 main;若更新 base 或 head,必须重新执行 exact-head review。Future-facing pass 已应用在 routing normalization 的单一 owner;继续抽象 Chat action store 或 Lark runtime 会超出这次共同 change reason。
English verdict: APPROVE - exact head b28af169f271eb9997b07730c1f034d58cccb827; behavior parity passed on base and head (199 tests each), the maintainability regression failed on base and passed on head, and the exact-head focused suite passed 212 tests with no blocking finding.
songoow
left a comment
There was a problem hiding this comment.
Approval conclusion (author-owned PR; GitHub blocks formal self-approval)
动机
#4780 合入后,chat_server.py 与 goal_topic_connections.py 超过既有模块预算。exact head b28af169f271eb9997b07730c1f034d58cccb827 删除 Chat 404 响应和 persisted Lark routing defaults 的重复知识,使模块回到 1516/1499 行,没有提高预算或新增例外。
改动思路
Chat 保留 action service/store owner,仅收拢相同 404;Lark 把两条 active caller 已相同的 capture/ingress/reply 解析放到现有 routing owner。caller 继续负责 connector validation、health、route selection 和失败投影。
具体改动
_action_not_found保留相同 404 body/status/code,六种路径共享。_connection_routing_modes复用现有 enum,保留 legacyincoming_mode=all、默认、trim/lower 与非法值拒绝。- readback 与 event route 同时覆盖合法、legacy 与三类非法值,binding 文件不被改写。
- dated manager-context tests 使用 scoped clock,七天运行时 retention 不变。
对主干的风险
共享 helper 可能改变两个 caller 的默认或拒绝顺序;同一 12-case harness 在 base/head production 各通过,focused 157 +116 tests、三个 smoke、ratchet 和最终 CI 均通过。PR 正文先前自指 #4793 dependency 的 P2 文案已修正。没有新 vocabulary、authority、状态或 default-off 行为。
我的整体评价
APPROVE。 相比抬高预算,该 PR 删除真实重复规则,并有完整行为等价证据。
English verdict: APPROVE — exact head b28af16 restores module budgets by reusing existing owners, preserves Chat/Lark behavior under base/head characterization, and passes final CI.
The maintainability check inherited by #4772 fails on main
361347713: #4780 (21d2e4e7d) grewchat_server.pyfrom 1518 to 1525 lines andgoal_topic_connections.pyfrom 1471 to 1542, exceeding the existing 1518/1500 ceilings. This change removes repeated logic and restores the check without changing budgets, exceptions, or scanners.The Chat handler now owns one missing-action response; persisted Lark capture/ingress/reply defaults share a private helper in the existing routing module. Both live callers use it. Production code shrinks by 21 lines overall; module sizes become 1516 and 1499. Defaults, enum rejection, connector validation, API responses and durable writes are preserved. The related refactor pass is this shared ownership; no new capability or provider is introduced.
Validation (Python 3.12.3, Node 22.22.3, isolated checkout environment):
uv run --extra test python -m pytest tests/canary/test_maintainability_ratchet.py tests/test_chat_server_cors.py tests/extensions/test_lark_goal_topic_connections.py -q: 157 passed. Before the fix, ratchet alone was 1 failed, 12 passed, matching CI. Final ratchet-only readback: 13 passed.uv run --extra test python -m pytest tests/test_chat_lark_api_contract.py tests/extensions/test_lark_goal_topic_runtime.py tests/extensions/test_lark_event_collector_routing.py tests/test_chat_operation_actions.py tests/test_chat_team_plan_action.py -q: 114 passed, 2 failed. Both failures reproduce with unchanged main production code:test_manager_captures_unaddressed_context_without_granting_turn_authorityandtest_manager_authorized_turn_quietly_recovers_history_as_contextexpect one historical context item but see zero. Their September 13 fixtures encounter the existing seven-day retention rule; the two test cases now freeze the retention clock to their dated fixture. The runtime retention policy is unchanged; the final rerun below includes both cases.uv run --extra test python examples/<name>.pypassed:lark-goal-topic-connection-smoke,loopx-chat-actions-smoke,loopx-chat-server-smoke.git diff --check, and added-content public/private scans passed.The affected entrypoints are the existing Chat action API and Lark connection readback/event routing. No frontend change is needed: HTTP errors and routing projections are characterized as unchanged. Provider calls use synthetic fixtures; no live Lark service or active Goal is mutated. Full CI remains to run; the two inherited runtime failures are explicitly not claimed as fixed.
Current-head CI repair
Head
b28af169f271eb9997b07730c1f034d58cccb827is the module-budget repair itself and also includes the verified repair for date-dependent manager-context fixtures. The two previously reported manager-history failures are repaired with a scoped fixture clock; the seven-day runtime retention rule remains active. No budget or exception was raised.Focused current-head validation: 217 passed, no skips or failures in that run. Diff checks pass. This is local evidence; the newly triggered remote CI remains authoritative for readiness. No PR was merged and no new unrelated feature PR was opened.