feat(todo): close native work requirement edits with shared typed validation - #4252
Conversation
huangruiteng
left a comment
There was a problem hiding this comment.
Request changes conclusion (author-owned PR; GitHub blocks formal self-review)
复审绑定 exact head:504895960b80f105ef440112c166bdb5cc318843。结论:REQUEST_CHANGES。
动机
这个 PR 补的是一个真实的 Todo 原生编辑缺口:action_kind、task_domain、task_repository、write scopes、capabilities 与 Explore refs 已经是现有状态,却没有完整地走 native planning → canonical CAS → receipt 的编辑路径。把 monitor successor 里的 repository/capability 校验抽到 Todo bounded context,也比在 Python adapter 再维护一份规则更符合现有架构。
关键语义边界是对的:只处理显式提供的字段;省略和空白 scalar 保持旧值,显式空列表用于清空;集合中任意非法成员都拒绝整个计划,不能悄悄丢弃;保留 lease 时不允许改 work requirements。requirements 仍只是工作声明,不会授予 capability、仓库权限、lease 或执行权。
改动思路
Python update_goal_todo 只做参数适配,TypeScript normalizeNativePlanningIntent 与 prepareUpdatedTodo 继续拥有校验、lease/revision rejection 和 canonical mutation。共享的 work_requirements.ts 让 native edit 与 monitor successor 使用同一套 token、scope、repository 和 Explore ref 规则,没有引入第二个状态 owner。
这个提取还必须带走原来保护该不变量的 mutation case。当前实现代码已经移动,但 mutation locator 没有同步移动,导致正常 focused tests 是绿的,刻意把“throw”改成“continue”的回归敏感性却不再能执行。
具体改动
真实 base 3aa05cc256b1d77e59576dc3e943f8d7e626afe3 到 head 为 17 个文件、+361/-66。核心是新的 loopx/control_plane/todos/work_requirements.ts、native planning/update 接线、Python adapter、文档和正负路径测试。
我在 exact head 上运行了 40 个 focused Python tests 和 21 个 focused TypeScript tests,共 61 passed;git diff --check 通过。省略/清空、混合非法成员、过量 refs、unsafe repository、metadata-only correction 与 retained lease 等路径都有直接覆盖。
我还做了 base/head mutation 对照:base 上 monitor_route_drops_invalid_capability 能定位原始代码、注入 mutation,并被独立 oracle kill;head 上同一 harness 直接报 ValueError: mutation locator drift。原因很明确:examples/shared-goal-authority-e2e/mutants.py:60 仍指向 monitor_successor.ts,而权威 throw 已移动到 work_requirements.ts:57。
对主干的风险
- [P1] mutation guard 没有随共享 codec 移动。 当前 required mutation、Stage2c correctness、aggregate 和 merge-gate 都因此为红。请把
monitor_route_drops_invalid_capability的 locator 改到work_requirements.ts中的权威分支,继续使用独立的 reject-all oracle,然后完整跑通 mutation/Stage2c/merge matrix。普通 focused tests 通过不能替代这个证据:它们在当前 head 上通过,但已经无法证明测试会杀掉“非法成员被静默丢弃”这个历史风险。
另有一个严格 browser locator 报重复元素;它不是本轮 runtime blocker 的根因,但在新 exact head 上也需要通过 rebase/修复让 required rollup 收敛。
Future-facing pass:共享 codec、typed planning 与 canonical CAS 已经是正确的最小边界;不需要再加 adapter-local validator 或新 capability。最高价值的小修就是让 mutation source mapping 跟随 owner 移动。
我的整体评价
实现方向、scope、原子语义和 authority boundary 都成立;当前阻断不是要求扩大功能,而是必须恢复这次重构意外丢失的 regression sensitivity。修正 mutation locator、让 exact-head required checks 全绿后,可以重新给 approval conclusion。
English verdict: REQUEST_CHANGES on exact head 504895960b80f105ef440112c166bdb5cc318843. The native Todo edit path, shared TypeScript ownership, atomic rejection, lease guard, and default-off semantics are sound, and 61 focused tests pass. However, the existing invalid-capability mutant still targets monitor_successor.ts after the guarded branch moved to work_requirements.ts; base locates and kills it, while this head fails with mutation locator drift, leaving Stage2c and the merge gate red. Retarget the mutant to the shared owner and obtain a green exact-head required rollup.
…idation Signed-off-by: huangruiteng <huangrt01@163.com>
…undaries Signed-off-by: huangruiteng <huangrt01@163.com>
Signed-off-by: huangruiteng <huangrt01@163.com>
5048959 to
004fab2
Compare
|
Addressed the requested changes on exact head
Exact-head evidence:
GitHub required checks on this new head are the remaining merge gate. After they are green, this is ready for the owner-authorized self-merge. |
|
Merged with owner-authorized admin bypass after the requested refinement. At merge time all completed exact-head checks were green, including the repaired mutant lane, Stage2C aggregate, kernel static checks, Dashboard acceptance and aggregate |
Summary
Close the existing public work-requirement edit path for promoted, non-Monitor Agent Todos without a retained lease. This advances T1 of the TypeScript migration and shared-authority RFCs without changing provider defaults or promotion holds.
Deliberate semantics and boundaries
Previously, the legacy writer could drop invalid members from capability/write-scope/reference lists or silently remove an invalid repository while committing the rest of an edit. Public updates now reject the entire invalid declaration. Excess Explore references also reject rather than truncate. Explicit empty collections still clear; omitted/blank scalar input preserves existing state. Copy edits do not revalidate unrelated historical declarations or re-arm waits.
Repository/capability aliases normalize before receipt identity. A retry after a later edit cannot restore old requirements. Password-bearing SCP userinfo is rejected, including Monitor successor routes; username-only Git transports remain supported.
Requirements are not authorization. Other-owner/excluded/bound-agent fences, raw text/note patch allowlist, decision authority, terminal operations, Monitor operations and retained-lease requirement edits remain restricted. No live Goal was promoted or changed.
Implementation and refactor economics
Existing CLI/Python API → public intent → shared TS requirements/public planner → canonical CAS and durable receipt → existing independent Markdown projection delivery.
Repository/capability rules move out of the scheduler-owned module and are reused for both update paths and receipt comparison. The existing write-scope validator remains authoritative. Product code is +136/-62 (net +74); moving a codec is not claimed as net deletion. Legacy reader/bootstrap codecs and the Markdown writer still have actual callers and remain. Remaining ownership/decision metadata and leased Monitor effects are explicitly not declared complete.
The affected entry points are existing CLI and Python update APIs. The current Dashboard edit adapter does not expose these seven metadata fields; no new frontend setting, action, or UI behavior is claimed.
Review refinement
The review blocker was valid:
monitor_route_drops_invalid_capabilitystill targetedmonitor_successor.tsafter the guarded branch moved towork_requirements.ts. Commit004fab2b1retargets the existing mutant to the shared owner. The mutant now executes once and is killed by the independent reject-all oracle. The branch was also rebased ontod7e47652bso the unrelated browser locator correction already present onmainis included.Validation
004fab2b1774386f893762253acb46af47b077a7d7e47652b99148376e12d37cf56fe67ae0aad7cenpm run typecheck:control-plane;python -m mypy(22 source files); focused Ruff;git diff --checknpm run test:control-plane: 1,189 passed, 0 failed; the single environment-gated PostgreSQL row was exercised separatelymonitor_route_drops_invalid_capability: selected 1, executed 1, killed 1 by the independent reject-all oracleloopx checkacrossloopx/,tests/,examples/anddocs/; no public/private or credential finding in the changed scopecqr_0e7ac7cdc0b1ab030290; fingerprint0e7ac7cdc0b1ab0302900d6804496d685b55eee71bd5f1c8e6c0a001a9790d18No paid model run, production promotion, live provider cutover, permission expansion or hosted deployment was needed for this transaction/validation change. At the owner-authorized admin merge, every completed exact-head check was green—including mutants, Stage2C, kernel static checks, Dashboard acceptance and their aggregate gates—and only the final pytest aggregation was still pending. The matching local full control-plane suite had already completed with 1,189 passed and 0 failed.
Shared-authority RFC fixture impact
loopx_coordination_production_scale_fixture_v0(unchanged).Boundary checklist
main; no stacked unrelated history.