Skip to content

fix(periodic-report): report Goal evidence, not one lane's - #4729

Merged
huangruiteng merged 3 commits into
loopx-project:mainfrom
DJC1412:codex/goal-owned-periodic-progress
Sep 19, 2026
Merged

huangruiteng merged 3 commits into
loopx-project:mainfrom
DJC1412:codex/goal-owned-periodic-progress

Conversation

@DJC1412

@DJC1412 DJC1412 commented Sep 19, 2026 •

Copy link
Copy Markdown
Contributor

Goal And Delivered Outcome

Scope And Continuation

  • Completed scope and remaining work: Goal-level selection across Agent lanes plus reporter-first ranking of outcomes and the next action. Rows no Agent claimed stay out: an amendment comment records that the first revision admitted them, which broke two pending-intent regressions that pin an empty snapshot for an unowned frontier row. What is deliberately not claimed: the per-Agent publication cursor (incremental._cursor_path still keys goals/<goal>/periodic_reports/publication-cursors/<agent>.json) and per-item Agent provenance in the rendered body. A multi-Agent Goal therefore still keeps one incremental cursor per lane, and a report line says what was done without naming whose lane did it — attributing at that granularity needs the item contract, the editorial consumer that rejects consumer-assigned fields, and the renderer to carry one more field, which is its own slice.
  • Slice boundary / successor: the boundary is the selection rule in project_progress_snapshot.py, which is independently testable (builder-level and hook-level fixtures) and revertible without touching any persisted shape. Next owner/task: [Feature]: add generic machine configuration and Goal-owned weekly reporting #3859's remaining rows — per-Goal publication cursor and the observed_by / executed_by provenance the issue lists as Agent-layer evidence attributes. Per-item provenance is intentionally deferred rather than half-added: _normalize_item in adapters.py projects a fixed field set, so an unpaired new field would be dropped on the way to the report.

Validation

  • Tested revision: 321b9615f (rebased on 3d5fc5b46); the rows below were run on 2c1fefc10 and re-run focused on this head
  • Run state: finished for the focused rows below; an aggregate tests/ run is executing on this head and its outcome is added on the amendment comment rather than inferred from these rows.
  • Input classes: synthetic
Check kind Result Public-safe evidence / limitation
unit passed uv run --extra test python -m pytest -q tests/capabilities/test_periodic_report_incremental.py — 27 passed, including four added Goal-aggregation, ranking, outcome-cap and fallback cases.
unit passed uv run --extra test python -m pytest -q tests/control_plane/test_post_writeback_capability_hooks.py — 61 passed, including the added hook-level peer-progress case.
integration passed uv run --extra test python -m pytest -q tests/capabilities -k periodic_report tests/control_plane/test_post_writeback_capability_hooks.py — 270 passed, 0 failed, 0 skipped.
real_entrypoint passed build_periodic_report_post_writeback_projection, the shipped post-writeback producer, exercised with a synthetic registry, runtime root and active-state file; asserts the item sequence the report consumes rather than a mock's return.
regression_parity passed With the builder reverted to base and the new tests kept: the four builder cases and the hook case fail; with the change: all pass. The pre-existing test_snapshot_next_action_prefers_the_reporting_agent_within_the_stage_window passes on both, so the reporter-first rule is unchanged where it already applied.
static passed uv run --extra test python -m py_compile loopx/capabilities/periodic_report/project_progress_snapshot.py; examples/docs-governance-smoke.py → docs-governance-smoke ok; loopx check --scan-path docs/reference/protocols/periodic-report-v0.md --scan-path loopx/capabilities/periodic_report --scan-path tests/capabilities/test_periodic_report_incremental.py → public boundary scan clean (30 files).
manual passed Each shipped periodic-report smoke: examples/periodic-report-{adapters,bindings,html,profile,runtime-producer,smoke}.py → all ok.
  • Coverage and gaps: the changed path is one selection rule with one ranking helper; it is covered at both the builder level and through the production post-writeback producer, and the mutation run above shows the new assertions are load-bearing rather than derived from current output. Runtime behavior of the TypeScript boundary is unaffected (no state machine, effect or schema field changed), so npm run test:control-plane was not run; no real multi-Agent Goal was exercised, because that would mean reading or writing an active Goal's state, which is out of bounds here — the fixtures are synthetic. The full tests/ run was executed on this revision and its aggregate outcome is reported in the linked comment when it finishes.

Frontend / Visual Evidence

  • UI impact: none
  • Before: N/A — no dashboard, chat or CLI rendering code changed; the projection already renders whatever items it receives.
  • After: N/A
  • States and viewports shown: N/A
  • Source data: synthetic

Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • Refactoring (no functional changes)
  • Documentation update
  • Test update

LoopX Area

  • Control plane (goals, todos, quota, scheduler, registry, runtime)
  • Benchmark boundary (adapters, runners, verifiers, scoring, evidence)
  • Capability or extension (providers, adapters, skills)
  • Public docs or presentation surface (README, protocols, dashboard)
  • Build, packaging, installer, or CI
  • Host or runtime integration

Technical Direction

Shared-authority RFC fixture impact

  • N/A: no TypeScript control-plane migration or shared Goal Authority claim; no production-scale fixture, provider conformance arm, or routing/promotion change is touched.

Boundary Checklist

  • Neither the diff nor this PR body/comments/attachments disclose private state, credentials, raw traces or verifier output, internal links, or local machine paths (including .loopx/, .codex/goals/, and live ACTIVE_GOAL_STATE.md).
  • I did not duplicate maintainer-owned benchmark work unless a maintainer split out a public issue for it.
  • I kept the change scoped to the linked issue/task.
  • I completed the visual evidence section for UI changes, or marked UI impact none.
  • Every commit includes a DCO Signed-off-by trailer (git commit -s).

@huangruiteng huangruiteng left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

评审对象:#4729 @ 2c1fefc1075b0b7ff594c84b7637cb31375df457(base main,作者 @DJC1412)。结论:REQUEST_CHANGES,一条 P1。

动机

#3859 要求周报由 Goal 拥有、Agent 只作证据生产者,它点名的 “Current mismatch” 正是这一步:project-progress 快照只包含 claimed_by == agent_id 的 Todo。base bdc1c1604 上确实如此——build_project_progress_snapshot_from_state 的 done 与 open 两个列表都带 claimed_by == agent_id 过滤,于是一个多车道 Goal 对每条车道只讲自己那一片,owner 读到的不是 Goal 的进展而是各车道的摘要。方向我认可:把选择域从“车道”提到“Goal”,再用排序保证报告人自己的证据不被截断,是这个能力该做的下一步。

改动思路

两步:(1) 去掉两个 claimed_by == agent_id 过滤,让满足阶段窗口、元动作、可行动性条件的 Goal 行都可选;(2) 新增 _reporting_agent_rank(自己 0 / 同伴 1 / 无主 2),并在“按 updated_at 倒序”之后用稳定排序再按 tier 排序,保证 tier 内仍是新到旧、6 条 outcome 上限不会挤掉报告人自己的行。测试在 builder 与真实 post-writeback hook 两层补齐,原先守“只看本车道”的用例被改名为 ..._prefers_the_reporting_agent_within_the_stage_window,协议文档同步更新——这些我都赞成,尤其“改名而不是悄悄改期望”这一点。

具体改动

  • loopx/capabilities/periodic_report/project_progress_snapshot.py:新增 _reporting_agent_rank;done/open 两处去掉 claimed_by == agent_id;done 在 recency 排序后再做 tier 排序,open_items 直接 tier 排序;docstring 说明“按 Goal 选择、按报告人排序”。
  • tests/capabilities/test_periodic_report_incremental.py:原 scope 用例改名;新增 4 个用例(同伴 outcome、ranking + 阶段窗口、6 条上限保住报告人自己的行、next action 回退到同伴/无主)。
  • tests/control_plane/test_post_writeback_capability_hooks.py:新增 hook 级用例,断言 [outcome(自己), outcome(同伴), next_action]。
  • docs/reference/protocols/periodic-report-v0.md:补一段 Goal 级范围 + reporter-first 排序说明。

我在 2c1fefc10 上重跑了作者的证据并一致:tests/capabilities/test_periodic_report_incremental.py 27 passed、tests/control_plane/test_post_writeback_capability_hooks.py 61 passed、tests/capabilities -k periodic_report 加该 hook 文件 271 passed、6 个 examples/periodic-report-*-smoke.py 全 ok、docs-governance-smoke ok、public boundary scan clean(30 files)。对照实验也成立:只把生产文件回退到 e5c13f39a^,恰好这 5 个新用例失败。

对主干的风险

P1(阻塞):同一能力既有的消费者契约被打红,本 PR 没有同步。 tests/cli_commands/test_periodic_report_intent_capability_chain.py 在 2c1fefc10 上 2 个用例全挂,在 base bdc1c1604(以及只回退生产文件后)2 passed:

python -m pytest -q tests/cli_commands/test_periodic_report_intent_capability_chain.py
# head: 2 failed
#   :104 assert "project_progress" not in payload
#   :121 assert "project_progress" not in sidecar["intent"]["payload"]

根因我做了最小复现,不是夹具问题:夹具 _write_unclaimed_frontier_state 的语义是“报告人自己那条 Todo 已被完成、时间戳落在冻结的阶段窗口之后,唯一还开着的是无主的 Goal Todo”,因此 base 的选择结果为空、快照为 None、intent payload 里没有 project_progress(夹具 docstring 明确写了 “stays empty”)。本 PR 去掉 open 侧的 claimed_by 过滤后,无主行以 tier 2 成为 next_action:

base : snapshot is None: True
head : snapshot is None: False  -> [{"source_ref": "todo:todo_unclaimed_frontier", "content_kind": "next_action"}]

也就是说,本 PR 的意图(“无主 Goal 工作不再对所有车道不可见”)在一个它没有提到的消费者里生效了,而那个消费者的 payload 形状曾被测试钉住。最小修复二选一:(a) 若 Goal 级选择是有意的,就把这两个用例(连同夹具 docstring 的 “stays empty”)改名/改期望,明确 payload 何时会带 project_progress;(b) 若 pending intent 仍应 fail-closed 保持为空,就把 next action 的回退限制回报告人自己的行。我倾向前者,因为它正是 #3859 的方向;但需要作者显式选择,而不是把这条红留在分支上。

另有两件非阻塞、但属同一批要说清的事:

  1. PR 正文承诺的聚合 tests/ 结果尚未出现(正文写 “the aggregate tests/ run is still executing on this revision at authoring time and its outcome is added as a comment on this PR”,而评论区目前为空)。仓库的合并前门禁要求该 revision 有聚合结果;上面的 P1 本应由它先拦住。
  2. Goal 级选择 + 每 Agent 游标的组合意味着同一条“同伴/无主”事实可能被多条车道各自发布一次(游标按 source_ref 记在 goals/<goal>/periodic_reports/publication-cursors/<agent>.json)。作者已把 per-Goal cursor 列为后继,这没问题,但建议在 PR/issue 里写明这条交付语义,因为它是选择域扩大的直接后果,而不是实现细节。

我的整体评价

选择域与排序的设计是对的,改动小而干净(4 个文件、+235/-8,无新 schema、flag 或持久化形状),测试在 builder 与真实 hook 两层都能反向证明(回退生产文件则 5 个新用例失败)。但这条 head 不能就这么合:同一能力的 test_periodic_report_intent_capability_chain.py 从 base 的 2 passed 变成 2 failed,正文承诺的聚合结果又恰好缺席。修复量很小(改两个用例,或收窄回退),但需要作者明确 payload 契约朝哪边走,并在同一 head 上把该文件跑绿、把聚合结果贴出来。改好后我再看这个 head。

English verdict: REQUEST_CHANGES - #4729 at 2c1fefc is the right direction (Goal-level evidence selection with reporter-first ranking, so a multi-lane Goal stops reporting only one lane's slice) and I reproduced the author's evidence at this head: 27 + 61 + 271 pytest cases passed, all six periodic-report example smokes ok, docs-governance ok, boundary scan clean, and reverting only the production file makes exactly the five new cases fail. It nevertheless cannot be accepted because a pre-existing consumer contract in the same capability is now red and the PR does not touch it: python -m pytest -q tests/cli_commands/test_periodic_report_intent_capability_chain.py is 2 passed at the merge base and 2 failed at the head (assert "project_progress" not in payload at :104 and :121). The cause is not the fixture: with the lane's own row done beyond the frozen stage window and the only open Agent Todo unclaimed, the removed claimed_by == agent_id filter makes that unclaimed row the emitted next action, so the snapshot stops being empty (base: snapshot is None: True; head: one next_action item) and the trigger-evaluation intent payload gains project_progress, contradicting the fixture's documented fail-closed "stays empty" behavior. Minimum repair is one of two explicit choices - update/rename those two chain tests (and the fixture docstring) to the new Goal-level rule, or restrict the next-action fallback to the caller's own rows - plus the aggregate tests/ outcome the PR body promised as a comment but has not posted. Non-blocking: Goal-level selection combined with per-Agent publication cursors lets the same peer or unclaimed fact be published by several lanes, which should be stated as delivery semantics rather than left implicit.

A project-progress snapshot selected only Todos whose claimed_by equalled
the requesting agent, so a Goal worked by several lanes reported each lane
its own slice and silently omitted peer progress. Selection is now Goal-level
and claimed_by only ranks the reporter's own outcomes and next action first,
which also keeps the bounded outcome cap from evicting them.

Part of loopx-project#3859: the machine-configuration base and the Goal-level delivery
identity have already landed; this closes the evidence-aggregation step and
leaves per-agent publication cursors and per-item provenance to the next cut.

Signed-off-by: DJC1412 <108855841+DJC1412@users.noreply.github.com>
Covers the aggregation through the shipped projection entry point rather than
the snapshot builder alone: a Goal whose lanes split their Todos now reaches
the report with both outcomes, ordered reporter-first.

Signed-off-by: DJC1412 <108855841+DJC1412@users.noreply.github.com>
Aggregating by Goal also admitted Todos no Agent claimed, which turned an
unowned frontier row into a next action. Two pending-intent regressions pin
the opposite contract on purpose: an unclaimed advancement row keeps successor
ownership in the frontier while the durable progress snapshot stays empty.
Peer lanes still report; a row with no producer has no provenance to report.

Signed-off-by: DJC1412 <108855841+DJC1412@users.noreply.github.com>
@DJC1412
DJC1412 force-pushed the codex/goal-owned-periodic-progress branch from 2c1fefc to 321b961 Compare September 19, 2026 09:54
@DJC1412

DJC1412 commented Sep 19, 2026

Copy link
Copy Markdown
Contributor Author

Amendment after a full-tree run

The revision 2c1fefc10 was pushed on the strength of the focused suites named in the description. Those were green, and they were not enough: an aggregate pytest -q tests/ run on that head surfaced two regressions in a suite the focused selection did not include.

Failed: tests/cli_commands/test_periodic_report_intent_capability_chain.py::test_pending_intent_fallback_uses_producer_capability_evidence and ::test_pending_intent_fallback_fails_closed_without_producer_evidence.

Cause: the first commit selected any Goal row, so a Todo with no claimed_by became a next_action. The fixture those two tests share states the opposite on purpose — "the unclaimed advancement Todo keeps the successor frontier owned … while the durable progress snapshot stays empty" — so an unowned frontier row became reportable progress and the pending intent gained a project_progress it must not carry.

Fix: fix(periodic-report): keep unclaimed rows out of report evidence narrows selection to rows an Agent produced; peer lanes still report, and a row with no producer has nothing to attribute. The two regressions pass again, and the same-shape rule now has a snapshot-level case of its own (test_snapshot_reports_peer_outcomes_and_keeps_unowned_rows_out, test_snapshot_next_action_prefers_the_peer_lane_over_an_unowned_row) instead of living only in the pending-intent chain.

The protocol sentence changed with it: a row no Agent claimed stays out because it has no provenance, which is a narrower claim than the first revision made.

Current head: 321b9615f, rebased onto 3d5fc5b46 (three commits). Focused re-run on this head: tests/capabilities/test_periodic_report_incremental.py, tests/control_plane/test_post_writeback_capability_hooks.py, tests/cli_commands/test_periodic_report_intent_capability_chain.py → 90 passed.

Parity on this head: with the builder reverted to main and the tests kept, all five added cases fail — including At index 1 diff: ('next_action', 'Next action') != ('outcome', "Land a peer lane's change.") at the hook entry point — and pass with the change; the pre-existing test_snapshot_next_action_prefers_the_reporting_agent_within_the_stage_window passes on both sides. An aggregate pytest -q tests/ run is executing on this head now; its outcome will be added to this comment thread rather than inferred from the focused rows.

@huangruiteng huangruiteng left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

复评对象:#4729 @ 321b9615f21de25d41e7d3fb5b0753537eea1de8(base main,作者 @DJC1412)。这是我上一轮 REQUEST_CHANGES 之后的新 head。结论:APPROVE。

动机

#3859 要求周报由 Goal 拥有、Agent 只作证据生产者,它点名的 “Current mismatch” 就是这一步:project-progress 快照只包含 claimed_by == agent_id 的 Todo,于是多车道 Goal 对每条车道只讲自己那一片。我上一轮认可这个方向,但要求先解决同一能力里被这条改动打红的消费者契约。这次作者没有改我的期望,而是改了实现:把“无主行”移出报告证据,并把这个选择过程写进了 PR 的一个 amendment 评论(含失败用例、根因、修法),这正是我要求的显式选择。

改动思路

与上一版相比,选择规则从“所有 Goal 行可选 + 三档 tier(自己/同伴/无主)”收窄为“有产出者的行才可选”:新增 produced_by(item)(即 claimed_by 非空),done 与 open 两处都用它做过滤,排序键退化成 produced_by(item) != agent_id(自己在前、同伴在后),依旧放在按 updated_at 倒序之后,所以 tier 内的时间顺序不变。无主行既不是 outcome 也不是 next action,于是“只开着一个无主 Todo”的状态重新得到空快照——pending intent 的 fail-closed 语义恢复。测试同步更新:原来的 peer/unowned 期望改为“peer 保留、unowned 排除”,并新增 test_snapshot_reports_peer_outcomes_and_keeps_unowned_rows_out、test_snapshot_next_action_prefers_the_peer_lane_over_an_unowned_row,让这条规则在快照层也有一条自己的用例,而不是只活在 pending-intent 链路里。

具体改动

  • loopx/capabilities/periodic_report/project_progress_snapshot.py:新增 produced_by;done/open 两处由 claimed_by == agent_id 改为 produced_by(item);排序键改为 produced_by(item) != agent_id;docstring 明确“无主行没有产出者,因此不进报告”。
  • tests/capabilities/test_periodic_report_incremental.py:改名原有 scope 用例 + 4 个 builder 用例(同伴 outcome、ranking 与阶段窗口、6 条上限保住报告人自己的行、next action 优先同伴且排除无主)。
  • tests/control_plane/test_post_writeback_capability_hooks.py:hook 级用例断言 [outcome(自己), outcome(同伴), next_action]。
  • docs/reference/protocols/periodic-report-v0.md:把 Goal 级范围、reporter-first 排序与“无产出者不进报告”写进协议段落。

我在 321b9615f 上重跑了证据(该 head 已 rebase 到 3d5fc5b46,所以下面的 diff/对照都以新 merge-base 为基准):

检查 结果
tests/capabilities/test_periodic_report_incremental.py + tests/control_plane/test_post_writeback_capability_hooks.py 88 passed
tests/cli_commands/test_periodic_report_intent_capability_chain.py(上一轮的阻塞项) 2 passed(上一版为 2 failed)
tests/cli_commands 整个目录 101 passed
22 个 periodic-report 相关模块 319 passed
tests/capabilities 整个目录 1456 passed / 21 failed,这 21 条在 merge-base 3d5fc5b46 上逐条同样失败(本地环境:capability show 报某个已装扩展引用未知 capability、以及一批 benchmark CLI 子进程用例),与本 PR 无关
反向对照:把生产文件换回 merge-base 版本、保留新测试 恰好 5 条新增用例失败,chain 两条在两种配置下都通过

对主干的风险

无阻塞项。上一轮的 P1(无主行成为 next_action 导致 pending intent 多出 project_progress)已被这次收窄修复,两条 chain 用例在 base 与 head 两种配置下都是我复跑过的绿灯。剩下两点属于记录与后续范围,不构成阻塞:

  1. amendment 评论里承诺的聚合 tests/ 结果仍未贴出;我用 tests/capabilities + tests/cli_commands 做了替代(并逐条比对了 base),但这仍应是合并前贴出的那一行。
  2. “Goal 级选择 + 每 Agent 游标”意味着同一条事实仍可能被多条车道各发布一次;作者已把 per-Goal cursor 列为后继,另一条 PR #4734 直接针对这一点(我已单独评审),两者在 project_progress_snapshot.py 的不同函数处重叠,后落地的那个 rebase 即可。

我的整体评价

上一轮我要求的最小修复是“要么更新那两条用例,要么收窄回退”,作者选了后者,并在同一 head 上用一条新测试把这条规则搬到快照层,还留下了说明失败与根因的 amendment 评论——这比我要求的更完整。方向(Goal 级报告)与边界(有产出者才是证据)都站得住,证据我逐条复跑且反向对照成立,rebase 后与当前 main 无文本冲突。建议合并前把聚合 tests/ 结果贴到那条评论上,然后由 maintainer 处理合并。

English verdict: APPROVE - re-review of #4729 at 321b961 (rebased onto 3d5fc5b). The P1 I raised is resolved: selection is narrowed to rows that have a producer (produced_by), so an unowned-only frontier again yields no snapshot and the pending-intent payload stays fail-closed; tests/cli_commands/test_periodic_report_intent_capability_chain.py is now 2 passed where it was 2 failed, and the same-shape rule gained snapshot-level cases instead of living only in that chain. I reproduced the evidence at this head: incremental + hooks 88 passed, chain 2 passed, tests/cli_commands 101 passed, the 22-module periodic-report set 319 passed, and the five added cases fail exactly when the merge-base builder is restored. tests/capabilities is 1456 passed with 21 failures that I reran node-for-node at merge base 3d5fc5b (local environment: an installed extension referencing an unknown capability, plus benchmark CLI subprocess cases), so they are not caused by this PR. Remaining non-blocking: the aggregate tests/ result promised in the amendment comment is still not posted, and cross-lane repeat announcements remain possible until the per-Goal cursor lands (proposed as #4734, which I reviewed separately).

@huangruiteng
huangruiteng merged commit 4077b87 into loopx-project:main Sep 19, 2026
@DJC1412

DJC1412 commented Sep 19, 2026

Copy link
Copy Markdown
Contributor Author

Aggregate pytest -q tests/ outcome promised on the previous comment, run on 321b9615f (the head merged as 4077b87a5):

90 failed, 10337 passed, 37 skipped in 35m36s on macOS/arm64 with Node 22.23.2.

Attribution, measured rather than assumed. A pristine worktree at 74b62ebb5 — main with neither #4728 nor #4729 — running the seven files that contained those 90 failures: 88 failed, 223 passed, and the failing test functions are the same 27 distinct names, all inside tests/architecture/test_semantic_{field_use,production,vocabulary_drift,producer_binding,incident_retrodiction}.py. Those are this machine's pre-existing failures, not this PR's.

The two remaining IDs (tests/extensions/test_extension_scaffold.py, tests/test_external_scheduler_worker.py::test_default_invocation_persists_backoff_state) pass in isolation on both trees (11 passed each), so they are load flakes inside the 10k-test run rather than a delta here. The real delta this run caught is the one the amendment comment documents: two tests/cli_commands/test_periodic_report_intent_capability_chain.py cases the focused selection missed, fixed by fix(periodic-report): keep unclaimed rows out of report evidence and green in the merged head.

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.

2 participants