feat(quota): give the blocked priority transition a typed owner notice - #4692
huangruiteng merged 1 commit into
Conversation
huangruiteng
left a comment
There was a problem hiding this comment.
评审 head:c3800390026e1938990c9d10929a9b137cfbc3fd;base:main(PR base 02cc53bd5)。
交付判定(policy 6):justified_increment。已落地的部分我验证通过;阻塞项是范围(scope),不是正确性——这一点我在下面用 grep 证据说明。
动机
#4381 的要求是:一个任务进入 blocked 时,要让 owner 知道——什么被卡住、为什么、谁能解决、是否需要 owner 动作。@4543(已合并)交付的是准入(notify_user: True)而不是投递,并留下三个缺口:投递/回读的持久化、按 blocker 身份与 revision 去重、以及解决/被取代的对账。
baseline 的问题描述是准确的:owner 面对的是一句没有任务、原因、恢复条件与下一步的通用话术,而且每次轮询都重复同一句。这类信号会衰减成噪音,恰恰发生在最需要它的场景里。
改动思路
活的那条路径很清晰:should_run_prepare._blocked_priority_fallback 对每个 blocked item 调用 build_blocked_transition_notice,把最多 3 条 notice 放进 blocked_transition_notices;interaction_contract._blocked_priority_fallback_user_reason 优先用 notice 里的任务/原因/影响/下一步拼出 owner 可见的 reason,没有 notice 时保持原来的话术。
契约的形状我认为是对的:owner_must_know 与 owner_must_act 分开、可解析的恢复条件、投递状态用字面量集合而不是散落布尔;pending 的语义被明确写成"没有投递面,因此什么都没送达",而不是把 NOTIFY 意图当成投递成功。
具体改动
4 个文件、+1077/-0:新模块 661 行、should_run_prepare.py 10 行接线、interaction_contract.py 26 行渲染偏好、新测试 380 行。
关键内容讲解
一、我用 grep 而不是 PR 描述来确定哪些部分真的在生产路径上:全仓(排除测试与本模块)搜索每个公开函数——
build_blocked_transition_notice -> loopx/control_plane/quota/should_run_prepare.py
load/store_blocked_transition_notice_state -> (无)
record_blocked_transition_delivery -> (无)
reconcile_blocked_transition_notices -> (无)
pending_/limitation/owner_decision -> (无)
也就是说:notice 构建 + 渲染偏好是活的(owner 看到的内容真的变了,且有"无 notice 时回落到原话术"的等价性测试),而投递账本与去重状态机(约 300 行)目前只被测试调用。
二、后果要讲清楚:作者在 PR body 里说本 PR 关掉三个缺口,但生产路径上实际只关掉一个——"重复轮询仍然渲染同一句"和"没有任何投递状态被记录"这两件事,在今天的生产路径上仍然成立。作者自己是主动披露了这一点的("no runtime caller writes the ledger yet… wiring it before that decision would encode the wrong answer"),我认可这个诚实,但披露不等于接受了边界。
三、测试我没只看结论:tests/control_plane/test_blocked_transition_notice.py + test_blocked_priority_fallback_notice.py 在 exact head 上 19 passed,覆盖 issue 列出的五种场景、提醒窗口、无投递面时保持 pending、回读失败、已批准不再重复索要,以及一次持久化往返。契约本身是扎实的。
对主干的风险
P1(阻塞):loopx/control_plane/quota/blocked_transition_notice.py 中约 300 行(reconcile_blocked_transition_notices、record_blocked_transition_readback、record_blocked_transition_owner_decision、pending_blocked_transition_notices、blocked_transition_delivery_limitation、load/store_blocked_transition_notice_state)在测试之外没有任何调用点。AGENTS.md 的 scope-fit 规则明确要求:当一个新增生产结构的"价值只存在于未提交的未来 runner、设计说明或没有验证契约的假设扩展"时,把它留在 docs 或 todo 状态里,等真实调用点出现;同时"仅由测试覆盖的未使用模块不算已交付行为,除非记录了一个owner 接受的 coverage-only 边界"。本 PR 的边界是作者声明的,不是 owner 接受的。
最小修复(按推荐顺序):
- 拆:把未使用的账本/状态机拆到后继,本 PR 只发 notice 构建 + 渲染偏好(这两半可独立测试、独立回滚,而且已经验证通过);
- 接:给状态机接一个真实调用点——已经持久化 per-goal 状态的 quota/heartbeat writer 是自然的归属方——让 emission decision、投递记录与对账真的执行;
- 由维护者在 PR 上明确接受这个 coverage-only 边界(这是仓库规则下唯一允许未使用生产结构随 PR 发布的出口)。
P3(非阻塞):_blocked_priority_fallback_user_reason 只取第一条 dict 形状的 notice;当多条 notice 并存时,其余内容不会出现在 owner reason 里(payload 里仍在)。若这是有意的,建议在渲染注释里写清"payload 保留全部、reason 只呈现第一条"。
其余验证:exact head 上 19 项聚焦测试通过;本 head 的 CI 19 项中 4 成功、15 待跑、0 失败;PR 落后 main(base 早于 #4651 与 #4686 的合并),因此无论范围问题如何决定,同步后都需要在新 head 上重跑评审与 merge readiness。
我的整体评价
baseline(一句无信息量、每轮重复的 blocked 说明)与 head(owner reason 带上任务/原因/影响/下一步,且契约把"意图"与"投递"分开)对比:活着的那部分是一次真实、可验证、边界清晰的改进,契约设计(must_know / must_act 分离、pending 不等于 delivered、revision digest 去重)也符合本仓库对状态规则的要求。
但它以"关掉三个缺口"的说法提交,实际上生产路径只关掉一个;另外两个是只有测试调用的生产代码。按仓库规则,这类结构应当先在 docs/todo 里等待真实调用点,或由 owner 明确接受边界。所以结论是 REQUEST_CHANGES——请把它读成"缩小范围或接上调用点",不是"代码写错了":第 1 种修法(拆出 live 部分)我可以立即复核通过。
English verdict: REQUEST_CHANGES - exact head c380039; the live half is good and verified: should_run_prepare now attaches typed blocked-transition notices and the interaction contract renders that notice as the owner-facing reason, with a pinned parity test when no notice is present, and the 19 focused tests pass at the head. The blocker is scope, not correctness. A production-reachability grep shows build_blocked_transition_notice is the only public function with a caller outside tests, so roughly 300 of the new module's 661 lines - the emission decision, delivery/readback recording, reconciliation and the file-backed ledger - are exercised only by tests. Two of the three gaps the PR claims to close (suppression of unchanged repeats and persisted delivery state) are therefore not shipped, and AGENTS.md's scope-fit rule asks that structure with no real call site stay in docs or todo state until a caller exists, with any coverage-only boundary explicitly owner-accepted rather than author-declared. Minimum repair in order of preference: split the unexercised ledger into a successor and ship the notice plus renderer now, wire one real caller (the quota/heartbeat writer that already persists per-goal state), or have the maintainer accept the boundary on the PR. Fifteen of nineteen checks were still pending and the branch is behind main, so merge readiness needs a re-run after a sync either way.
loopx-project#4543 made the quota projection admit that a higher-priority Todo blocked ahead of an executable fallback should be told to the owner. That admission is an intent, not a delivery: the owner-facing reason was still one generic sentence with no task, cause, recovery condition or next step. This slice ships the notice itself and nothing that has no caller: * build_blocked_transition_notice turns every first material entry into blocked into a typed notice - task, cause, evidence, impact, responsible party, recovery condition and next action - keeping owner_must_know and owner_must_act as separate decisions, plus a blocker_revision digest over the cause, evidence, recovery condition and responsible party so a later ledger can dedup "same blocker, same cause" from "materially changed". * should_run_prepare attaches at most three notices to the projection. * blocked_priority_fallback_owner_reason renders the first notice as the owner-facing sentence and falls back to the previous prose when the payload carries none, so the projection stays a thin caller instead of growing a second copy of the rules. Deliberately out of scope: the emission decision, the persisted delivery and readback state and the reconciliation of resolved and superseded blockers. Those were part of an earlier revision of this branch (661-line module) and were removed after review: a production-reachability grep showed every one of those entry points was exercised only by tests, and AGENTS.md asks that a structure with no real call site wait in docs or todo until a caller exists. They arrive with the successor that binds the ledger to an authorized delivery surface. Until then every notice reports delivery.state == pending: no surface has been authorized, so nothing has reached anybody. Refs loopx-project#4381. Arrives with loopx-project#4543. Signed-off-by: YZJF <195568136+YZJF@users.noreply.github.com>
c380039 to
e35a97d
Compare
|
Addressed as your option 1 — the unexercised ledger is out of this PR. New head What is left, and what each public entry point now reaches No function in the module is exercised by tests only. Removed from the earlier head: What survives of the ledger contract is only the part the live half needs: P3: the intent is documented where it is decided, not left in a comment — Reviewer-visible ledger edit: Re-validation at the new head: focused suite 13 passed; related quota/heartbeat/interaction/presentation suites 162 at head versus 153 at a detached |
huangruiteng
left a comment
There was a problem hiding this comment.
动机
Issue #4381 的要求是:一个任务进入 blocked 时,要让 owner 知道——什么被卡住、为什么、谁能解决、是否需要 owner 动作。@4543(已合并)交付的是准入(notify_user: True)而不是投递:owner 看到的仍是一句没有任务、原因、恢复条件与下一步的通用话术。
本 PR 是 #4692 在我上一轮 REQUEST_CHANGES(c3800390,review 5245303037)之后的返工版。上一轮我验证了活的那部分(notice 构建 + 渲染偏好),但指出约 300 行投递账本/去重状态机只被测试调用,属于 coverage-only 结构。这一版把那段整体删掉,只留下有真实调用点的部分,并在 PR body 里明确写成 “Related to #4381(不是 Closes)”。判为 justified_increment:这是一个可独立验证、可回滚的完整增量,但 issue 的投递缺口仍然开着,作者也是这么写的。
改动思路
- 新增一个内聚的 owner:
loopx/control_plane/quota/blocked_transition_notice.py(322 行)负责 notice 词汇、身份/修订摘要推导,以及“这句话怎么给 owner 看”。它只处理已经被 quota lane 选中的 Todo,不新增持久化、不新增加速通道。 - 接到既有活路径上:
should_run_prepare._blocked_priority_fallback对每个status == blocked或resume_condition_pending的 item 调用build_blocked_transition_notice,把最多 3 条 notice 放进既有 payload 的新字段blocked_transition_notices。 - 渲染偏好收敛到同一 owner:
interaction_contract._blocked_priority_fallback_user_reason从“直接返回fallback['reason']”改为委托blocked_priority_fallback_owner_reason—— 有 notice 就用 notice 句(任务/原因/影响/下一步),没有就回到原来的通用话术,保持既有契约不变。 - 把“还没有的”明确留白:notice 里的
delivery块是state: "pending"+ 空 surface/时间戳,blocker_revision只是给后续账本用的去重键;模块 docstring 直接写明 emission/投递/对账属于后继切片。 - 同步棘轮:
interaction_contract.py因这次 +4 行(import + 委托)跨过其继承上限,按maintainability_ratchet.diff_scoped_module_ceiling_violations的规则在同一 diff 里把loopx/canary/module_metric_baseline.json的该模块上限 1544 → 1548 结清。
与上一版的比较:模块 661 → 322 行、测试 380 → 245 行,生产可见结果不变。
具体改动
loopx/control_plane/quota/blocked_transition_notice.py(新增 322 行):blocked_transition_notice_identity、build_blocked_transition_notice、blocked_transition_notice_owner_reason、blocked_priority_fallback_owner_reason四个公开函数(前两个分别被后两个内部使用)+ 6 个常量(schema、kind、NOTICE_DELIVERY_PENDING、三个RESPONSIBLE_*)。loopx/control_plane/quota/should_run_prepare.py(+9):导入 + 收集transition_notices+ payload 增加blocked_transition_notices[:3],并在注释里点明这是 #4381 要求的 typed contract。loopx/control_plane/work_items/interaction_contract.py(+4/-2):委托给新 owner,删除原来的三行 reason 直取逻辑。loopx/canary/module_metric_baseline.json(1 行):interaction_contract.py行数上限 1544 → 1548。tests/control_plane/test_blocked_transition_notice.py(新增 245 行,9 个用例):notice 携带任务/原因/影响/下一步、owner_must_know与owner_must_act分离、有 notice 时 owner 句来自 notice、无 notice 时回落到旧话术、payload 保留全部 notice 而 reason 只渲染一条等。
关键内容讲解
一、我先用 grep 确认“这一版真的接上了生产路径”(上一轮我就是这样发现问题的):
build_blocked_transition_notice -> should_run_prepare.py:278 (生产调用)
blocked_priority_fallback_owner_reason -> interaction_contract.py:1044 (生产调用)
blocked_transition_notice_identity -> 模块内部 :180 (被 builder 使用)
blocked_transition_notice_owner_reason -> 模块内部 :318 (被渲染器使用)
被删掉的九个名字(load_/store_blocked_transition_notice_state、record_blocked_transition_delivery、record_blocked_transition_readback、record_blocked_transition_owner_decision、reconcile_blocked_transition_notices、pending_blocked_transition_notices、blocked_transition_delivery_limitation、empty_blocked_transition_notice_state)在全仓(loopx/apps/packages/tests/examples/docs)引用数全部为 0,没有留下悬空引用。我也核对了新模块的常量表:上一版里那些属于账本的状态机常量(delivery 四态、decision/suppression、resolution、owner decision、提醒窗口、历史条数、state/emission schema)已经全部消失,只剩 6 个仍在用的常量——这一点我特意查了两遍,因为它正是上一轮问题的同类风险。
二、我用探针复现了 owner 实际看到什么:对 {"todo_id": "todo_demo1", "status": "blocked", "priority": "P1", "text": "Repair the flaky public smoke on main", "reason": "waiting on the upstream fixture release"} 得到
'Repair the flaky public smoke on main' is blocked: waiting on the upstream fixture release.
'Repair ...' will not advance until the recorded blocker must clear: ...
No owner action is required: the agent keeps independent work moving and reports again if this blocker changes.
同一探针确认:普通 open item 返回 None(不产生 notice)、{"reason": ...} 无 notice 时逐字回到旧话术、{} 返回 None;一个 resume_when 未就绪(resume_ready: False)的 Todo 会产生 notice(这正是“等待条件”的语义,而“未来才到期的监控窗口”因为没有 resume 条件仍不产生 notice)。
对主干的风险
本地验证(exact head e35a97d5d882027eea6428d13c3b7746a74fd593,base f5c95ff0):
tests/control_plane/test_blocked_transition_notice.py→ 9 passed。test_blocked_transition_notice.py + test_blocked_priority_fallback_notice.py + test_heartbeat_recommendation_refinement.py + test_interaction_contract_workspace_causality.py + test_quota_recommended_action_refinement.py + test_quota_decision_summary.py + tests/presentation/test_quota_markdown_boundary.py + test_quota_markdown_renderers.py→ 43 passed。其中test_blocked_priority_fallback_notice.py是 @4543 留下的既有契约测试、本 PR 未改动它,说明这次委托替换没有破坏原来的 reason 契约。- 托管 CI:本 head 26/26 已完成检查全绿(Sign-off、dependency-review、postgresql-authority、build、kernel-static-checks、node 最低/前瞻兼容、test-shard 1–4、stage2c e2e/mutants/installed、windows-powershell、checks、pytest、sonar 非阻塞、merge-gate;upload-release/publish-pypi/presentation 按条件是 SKIPPED)。
风险面很窄:只增加 payload 一个可选字段 + 一处渲染偏好,notify_user 等准入字段、优先级、Todo/锁/quota/受保护操作都没有变化;纯函数、无持久化,回滚即恢复旧句。棘轮上限的上调不是隐藏的预算放宽:maintainability_ratchet 的规则就是“本次 diff 使模块越过其继承上限时,必须在同一 diff 里结清该上限”,1548 与 head 实测行数一致。
需要诚实记录的边界(都写在 PR body / 模块 docstring 里,我也认同):
- 投递缺口仍未关闭:emission 决策、投递/回读账本、按
blocker_revision去重、resolved/superseded 对账都得等有授权投递面的后继切片;因此 #4381 不能被这个 PR 关掉,作者用的是 “Related to”。 - notice 里有一部分字段当前渲染器不打印(
evidence、responsible_party、recovery_condition、owner_must_act、blocker_revision,以及那个恒为 pending 的delivery块)。它们是给 host/agent 的契约数据(也是 #4381 要求“告诉 owner 什么”的载体),不是不可达的控制流,所以我没有把它当阻塞;但如果后继切片一直不落地,这些字段就是应当裁掉的部分——我把它记在残余风险里。 - 我没有跑真实 heartbeat/agent 端到端(本地只有模块与契约级验证),且本 head 相对 main 是 BEHIND,合并前需在同一 head 重读合并就绪。
我的整体评价
这是一次按要求收敛的返工:上一轮我指出“约 300 行只有测试调用”的范围问题,这一版没有辩解,而是直接把没有真实调用点的投递账本/状态机删掉(模块 661 → 322、测试 380 → 245),只保留两个有生产调用点、且互为生产者/消费者的函数对,并把“还没做的”明确写成后继切片而不是留在代码里。我把上一轮的方法又跑了一遍(逐函数 grep + 残留常量核对 + 直接探针),确认这次没有换来新的同类型问题:删掉的名字零引用、剩余常量都在用、owner 可见句子确实变成了任务特定内容、无 notice 时逐字保持旧话术,@4543 的既有契约测试也仍然通过;CI 在本 head 全绿。
我没有在本 head 上发现需要修改的问题。合并前只需更新分支后重读合并就绪;合并决定权仍在维护者。
English verdict: APPROVE - e35a97d resolves the scope defect I previously raised on #4692c380039 and delivers the live slice of #4381's owner notice: build_blocked_transition_notice has exactly one production caller (should_run_prepare.py:278) and blocked_priority_fallback_owner_reason exactly one (interaction_contract.py:1044), the emission/delivery/reconciliation ledger is removed with zero remaining references (module 661 -> 322 lines, suite 380 -> 245), and the owner-facing reason now renders task, cause, impact and next action while a payload without notices keeps the previous prose verbatim; verified locally with 9 notice tests, 43 tests across the notice/fallback/interaction/quota-rendering surfaces, a direct probe of the rendered sentence and all None/fallback cases, and green hosted CI (26/26). Not a blocker but recorded: several notice fields (evidence, responsible_party, recovery_condition, owner_must_act, blocker_revision, the pending delivery block) are contract data the current renderer does not print and are the fields to trim if the successor delivery slice never lands; delivery/readback/dedup remain unimplemented by design, so #4381 stays open, and the branch is BEHIND main, so merge readiness must be re-read on the updated head.
Goal And Delivered Outcome
blockedbe explained to the owner — what is blocked, why, who can resolve it, and whether owner action is required — instead of being discoverable only by opening Tasks. fix(quota): tell the owner a blocked priority todo while fallback continues #4543 (merged) delivered the admission:blocked_priority_fallbacksetsnotify_user: Truefor a blocked advancement Todo while an executable fallback continues. That is an intent, not a delivery._blocked_priority_fallbackcarriesblocked_transition_notices, andblocked_priority_fallback_owner_reasonrenders the typed notice as the owner-facing sentence — task text, cause, impact and next action (test_owner_facing_reason_carries_the_typed_notice). A payload without notices still renders the previous prose (test_owner_facing_reason_falls_back_to_the_generic_prose), and the payload keeps every notice while the reason renders one (test_the_owner_reason_renders_one_notice_while_the_payload_keeps_every_one).Closes: the delivery-surface work remains open, see below); intended basemain.Scope And Continuation
build_blocked_transition_noticecarries the task, cause, evidence, impact, responsible party, recovery condition and next action, keepsowner_must_knowseparate fromowner_must_act, and adds ablocker_revisiondigest over the cause, evidence, recovery condition, responsible party and supersession marker — the dedup key a ledger needs, without deciding emission here.c380039) also shipped the emission decision, the persisted delivery/readback ledger, the reconciliation of resolved and superseded blockers and the file-backed state helpers. All of it was reachable from tests only —build_blocked_transition_noticewas the sole entry point with a production caller — so per AGENTS.md's scope-fit rule it now waits for a real call site instead of being carried as coverage-only structure. The module shrank 661 → 322 lines and the suite 380 → 245.delivery.state == "pending": no surface, so nothing reached anybody, and a NOTIFY intent is not reported as a delivery.loopx/canary/module_metric_baseline.jsonraises the reviewed ceiling ofwork_items/interaction_contract.pyfrom 1544 to 1548 lines. Relocation was tried first (the rendering now lives in the notice module, which removed ~26 lines from the projection), but the file was already exactly at its frozen ceiling, so the ledger is the remedy the ratchet itself names for accepted growth:module metric debt: refresh the reviewed ceiling … for the growth this review accepts.any_countis unchanged.Validation
e35a97d5d(rebased ontoorigin/mainf5c95ff08)finishedsyntheticunitpassedpython3 -m pytest -q tests/control_plane/test_blocked_transition_notice.py tests/control_plane/test_blocked_priority_fallback_notice.py— 13 passed. Covers the three scenarios #4381 names that are live in this slice: agent-owned blocker, owner-action gate, blocked primary plus runnable fallback; plus the revision digest separating an unchanged cause from a materially changed one, a scheduled future monitor earning no notice, the owner reason carrying the notice, the parity fallback, and one-reason/many-notices rendering.unitpassedtests/control_plane/test_quota_run_decision.py test_quota_should_run_parity.py test_quota_decision_summary.py test_quota_action_portfolio.py test_quota_selection.py test_quota_planning_horizon.py test_quota_recommended_action_refinement.py test_heartbeat_recommendation_refinement.py test_heartbeat_recommendation_rules.py test_heartbeat_agent_input.py test_heartbeat_prompt_support.py test_interaction_contract_workspace_causality.py test_scheduler_interaction_arbitration.py test_agent_task_visibility.py tests/presentation— 162 passed at head, 153 at baseline, i.e. identical plus the 9 new cases.staticpassedpython -m ruff check --select E4,E7,E9,F tests loopx/control_plane— all checks passed.regression_paritypassedorigin/main, detached worktree) versus head on the related suites above: 153 versus 162, the difference being exactly the 9 new cases.regression_paritypassedtests/canary/test_maintainability_ratchet.py— 13 passed at head and at baseline; the ceiling refresh above is what keeps it green, and without it the ratchet reportsmodule_metric_budget:loopx/control_plane/work_items/interaction_contract.pyat 1548 lines.integrationblockedtests/control_planewas not completed locally: the canonical authority/lease arms need the managed Node 22.22.3 runtime, unavailable in this environment. Baseline and head behave identically there (both stop at the same point), so no signal is lost, but I am not claiming the full arm passed._blocked_priority_fallback's new notice list,_blocked_priority_fallback_user_reason's delegation, and the new module. Only four test files in the repository referenceblocked_priority_fallbackand all four are in the runs above. Untested paths: nothing persists the ledger yet (by design, see scope) and the Lark/owner-channel writer is untouched, so the end-to-end "notice reaches the owner" path remains [Feature]: Proactively explain blocked tasks and recovery actions without requiring dashboard inspection #4381's remaining work.Frontend / Visual Evidence
none. The change is confined to the control-plane projection and its tests; no dashboard, website or documentation chrome is touched.Signed-off-by: YZJF 195568136+YZJF@users.noreply.github.com