Skip to content

fix(issue-fix): close grouped Monitor execution and recovery - #4862

Open
huangruiteng wants to merge 2 commits into
mainfrom
codex/authority-execution-closure
Open

huangruiteng wants to merge 2 commits into
mainfrom
codex/authority-execution-closure

Conversation

@huangruiteng

Copy link
Copy Markdown
Collaborator

Goal And Delivered Outcome

Close the retained issue-fix grouped Monitor execution caller under the shared-authority and TS migration roadmap (#4574, #3225, #3245).

Before, the public PR-lifecycle command could create a grouped Monitor but could not observe changed membership or stop an empty bucket under hard leases. The immutable main baseline fails the member-change step on File/SQLite and the empty-group step on legacy. The caller now completes create → observe → stop → reactivate through the existing typed Todo and lease owners, with recovery after interrupted execution.

Base: main. This closes one real L4 caller; it does not change provider defaults or complete whole-Goal migration.

Behavior And Refactor

  • Move complete bucket selection and reconciliation decisions into one TS capability planner. Remove the Python grouping/fingerprint/transition implementations; runtime Python shrinks by 76 lines. Reuse scheduler cadence, scope decoding, Unicode ordering, Todo transactions and projection delivery.
  • Acquire a bounded execution for hard-lease observations/stops, revalidate the plan, and release only that attempt. Same Agent identity does not authorize borrowing a different execution. Identical retry recovers its own active acquisition; after an observation commit, retry cleans its surviving lease without repeating the Todo write. Expired/released attempts acquire anew.
  • Explicit lease-backed legacy Monitor observation uses the existing native held fence under the Markdown writer lock. Ordinary legacy update admission remains unchanged.
  • Reject missing/malformed ledger evidence, ambiguous active targets and older empty observations before they can be interpreted as completed work. Archived/superseded history cannot shadow a current Monitor. Preserve existing Unicode membership digests.
  • Pass the existing --runtime-root override through every affected read/write and use the explicit priority parameter for creation. No new capability, provider, settings surface or raw-patch authority.

Buckets remain independent commits. A later failure does not undo earlier committed buckets; read back and retry the observation. Unchanged retry can perform lease cleanup and display delivery while reporting no Todo business write. This is not an external-tool fencing protocol or an all-bucket atomic transaction.

Validation

  • Tested revision: cd5bf1b19d9b5c3a6c9e47ffcbcdf9ee88184509.
  • Run state: finished.
  • Input classes: synthetic, public_fixture, authorized_private_read_only.
Check kind Result Public-safe evidence / limitation
unit / integration passed Final head: 94 Python tests covering the public CLI, explicit runtime-root routing, soft/hard modes, foreign execution rejection, retry, Monitor admission/configuration and lease compatibility. Includes real child-process exit after durable observation.
real_backend passed Final head: 24 TS tests, zero failures/skips. Native/imported complex fixtures run observation, stop and reactivation against File, SQLite and isolated PostgreSQL 16; non-target Todo/lease records remain unchanged.
regression_parity / real_entrypoint passed Read-only local-source capture with 1,012 records and 10 leases; all six CLI stages pass on three disposable legacy/File/SQLite runtimes, with frozen source and all original records unchanged. Immutable original main reproduces the failures above. This rehearsal preceded integration of unrelated ready-work changes; final affected tests were rerun afterward.
integration passed Broad TS run before that main integration: 2,525 passed, zero failures. Its one optional PostgreSQL service skip subsequently passed separately on another isolated database. An earlier standalone store command selected an incompatible system Python for four subprocess checks; the corrected full run uses the selected checkout environment and passes.
real_entrypoint passed Built wheel installed in an isolated environment; its packaged native reconciliation handler executes outside the checkout.
premerge passed Final-head risk-based canary: all 19 selected checks passed, zero failures; no manual holds.
static passed TS typecheck, configured mypy (22 sources), focused Ruff, diff hygiene, and all 16 paths classified/scanned for private material.

Cold CLI observations on the retained source took roughly 3–18 seconds per invocation under concurrent validation load. Acquiring and revalidating has a cost; this PR claims correctness and ownership convergence, not throughput improvement. Further batching belongs to the existing transaction-payoff workstream.

Exact-scope quality receipt: cqr_232a33f42dcbc0e8bfc4, verified valid; zero blockers/warnings, one disclosed efficiency advisory, no policy safe-fix pass.

Scope And Continuation

Complete for the grouped Monitor caller. Bilingual operator docs and RFC checkpoints describe the changed error/recovery behavior and retain the conditional 5–8 remaining delivery-package estimate: remaining execution callers/fences, D1 consumers, contributor-owned SQLite D2 (#4224), capture/whole-Goal qualification, and default onboarding/retirement. No active Goal was promoted or used as a mutation fixture. PostgreSQL deployment, capacity and elapsed soak remain separate.

The affected entrypoint is the existing issue-fix CLI and its shared command template. No frontend change is needed: no setting, action schema or visual surface is introduced; existing Todo rendering and projection recovery are reused. No first-screen changes. The bounded future-facing pass was applied at the planner/execution boundary; no speculative provider framework was added.

Two DCO-signed commits separate runtime behavior from durable validation and documentation. Maintainer review and merge are required for this control-plane change.

Signed-off-by: huangruiteng <14976749+huangruiteng@users.noreply.github.com>
Signed-off-by: huangruiteng <14976749+huangruiteng@users.noreply.github.com>

@huangruiteng huangruiteng left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Request changes conclusion (author-owned PR; GitHub blocks formal self-review)

Exact head reviewed: cd5bf1b19d9b5c3a6c9e47ffcbcdf9ee88184509

动机

这个 PR 的目标是对的:把 grouped issue-fix Monitor 的 eligibility/grouping/operation decision 收到 TypeScript typed planner,Python 只负责读取完整 ledger、执行 public Todo writers、持有并释放 lease。它解决的也不是表面问题——空 bucket 要真实完成,membership 变化要被观察,进程中断后同一执行还要能恢复,不能借用别人的 lease。

我没有只验证正常 observe/complete,而是把“中断后恢复”当作最强产品承诺来反证。结果发现当前 exact head 只在测试复用完全相同 generated_at 时可恢复;真实 CLI 重跑会生成新的时间戳,因此会把自己的旧 lease 当成另一次执行并被冲突拒绝。

改动思路

总体边界合理:

  • planIssueFixMonitorReconciliation 从完整 ledger 与 canonical Todo 状态生成 typed steps;
  • Python materializer 在 effect 前 acquire,拿到自己的 execution proof 后 re-plan,再调用 add_goal_todo / update_goal_todo / complete_goal_todo
  • _release_attempt 只允许相同 owner、相同 attempt prefix、相同 proof 释放,避免误放别人的执行;
  • legacy/file/SQLite 都走同一 lease/public-writer contract。

阻塞点不在这个分层,而在 attempt identity 的输入:

identity = json.dumps(
    [goal_id, step.get("todo_id"), claimed_by, generated_at, cadence, rows],
    sort_keys=True,
    separators=(",", ":"),
)
prefix = "issue-fix-monitor:" + sha256(identity) + ":"

generated_at 是每次命令的 observation time,rows 还是原始有序列表;它们不是同一业务执行的稳定身份。进程在 durable acquire 后退出时,下一次正常命令即使 Goal、Todo、actor、cadence、ledger 语义和 planned operation 全部相同,只要时间变化一秒,prefix 就不同。随后代码拒绝复用旧 key、生成新 key,再被现存 active lease 拒绝。

具体改动

本 PR 已经完成且验证有效的部分:

  1. TypeScript planner 正确接管 group construction、add/observe/reactivate/complete/unchanged 的决策;
  2. Python effect adapter 使用现有 public Todo/lease API,没有再保留第二套业务 transition owner;
  3. effect 前 acquire、acquire 后 re-plan、effect 后 exact-proof release 的顺序合理;
  4. complete ledger、explicit boolean、namespace、priority 和 projection 字段都采用 typed contract;
  5. 三个 provider 的正常 execution、lost response、wrong actor/lease 与 projection settlement 覆盖扎实。

但需要修复以下阻塞问题:

[P1] 真实重试的 generated_at 变化会破坏自己的 lease recovery

  • 位置:loopx/capabilities/issue_fix/pr_monitor_materialization.py:99-136
  • 触发:第一次执行在 acquire_task_lease 已持久化之后进程退出;相同命令重新运行,generated_at01:00:00Z 变为 01:00:01Z
  • 实际结果:legacy 报 todo already has an active lease,file/SQLite 报 todo_lease_conflict
  • 影响:Monitor bucket 被卡到 TTL 到期或人工释放,和 PR 的“interrupted execution recovery”目标直接相反;
  • 为什么现有测试没发现:test_retry_after_interrupted_acquisition_recovers_its_own_attempt 和 real-process retry 都把两次 generated_at 固定成完全相同的值,因此恰好得到同一 prefix。

最小修复建议:

  1. attempt prefix 由稳定的执行意图派生,例如 Goal/Todo/actor/typed target operation 与必要的稳定 source fingerprint;不要包含每次 observation 的时间戳或 incidental list order;
  2. source freshness 继续在 acquire 后的 typed re-plan 中单独校验,不能为了稳定 key 而复用陈旧业务决定;
  3. 保留“unrelated execution 即使同 actor 也不能借 lease”的现有负例;
  4. 增加三后端回归:第一次在 acquire 后退出,第二次使用更晚 generated_at,要求成功恢复并保持同一 lease key/epoch;最好再覆盖语义相同但 ledger 行顺序变化。

对主干的风险

exact head 的正常路径质量较高:

  • focused Python suite:37 passed(70.37s);
  • source-checkout TypeScript/authority suite:419 passed(189.71s);
  • ruff check:通过;
  • git diff --check:通过;
  • 对当前 origin/maingit merge-tree --write-tree:无冲突。

第一次直接调用 system node 时,子进程误用了系统 Python,出现 dataclass(slots=...) 环境错误;随后改用 uv run --extra test node ...,完整套件全绿。这一环境纠正不影响上面的 blocker。

我另外运行了独立的真实边界负例,只改变重试时间戳,legacy/file/SQLite 3/3 全部失败。临时测试文件已删除,两个 review worktree 都保持干净。这个失败比正常 suite 更接近实际 CLI 行为,因此不能用现有绿测覆盖掉。

我的整体评价

结论是 REQUEST_CHANGES。TypeScript 决策权、Python effect boundary、public writer/lease 复用以及 normal-path tests 都是正确方向;我不建议推翻这套设计。需要阻止合并的是一个很窄但直接命中核心承诺的语义错误:把 observation timestamp/raw row ordering 当成 recoverable execution identity。

修复后请不要只重跑现有同时间戳测试;要让 fresh-invocation counterexample 在三个 provider 上先红后绿,并重新检查 unrelated lease 仍然拒绝。完成这个最小修复后,PR 的整体结构有机会直接通过复审。

English verdict: REQUEST_CHANGES - exact head cd5bf1b19d9b5c3a6c9e47ffcbcdf9ee88184509 has a blocking recovery bug: the lease-attempt prefix includes generated_at, so a normal retry after a post-acquire crash cannot recognize its own active lease; a one-second timestamp change reproduced todo_lease_conflict on legacy, file, and SQLite despite the existing same-timestamp suites passing.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant