Return accepted managed reports to Goal Files and source manager conversation - #4978
Conversation
41d97c8 to
ddd89a1
Compare
huangruiteng
left a comment
There was a problem hiding this comment.
Request-changes conclusion — author-owned PR; GitHub does not allow formal self-review approval. Reviewed exact head ddd89a1502b58e0e63e9ea7fc12d49222b7c1f5b.
[P1, blocking] Plan-linked report is hidden by an unrelated Goal result. In apps/presentation/dashboard/src/features/personal-workspace/manager-team-result.tsx:163-181, collectManagedGoalIndex scans a Goal-wide paginated inventory and rejects the entire read when any page has unavailable_count > 0, before readManagedPlanResult narrows it to the confirmed plan's Todo IDs. A valid accepted report for this plan therefore disappears from the original manager conversation when an unrelated historical report becomes unreadable. The fixed eight-page bound similarly leaves a matching report after page eight undiscoverable. I reproduced the first case in the existing team-plan browser scenario: the matching row remained valid, but setting an unrelated unavailable count to one made the expected manager report never appear; the unchanged fixture passed. Please resolve/verify the plan's exact Todo IDs, or make incomplete inventory explicit without treating unrelated unavailable rows as failure, and add the stale-row and pagination regressions.
[P2] Interrupted content-object creation is not retryable. In loopx/control_plane/todos/completion_result.py:51-63, the final digest path is opened with O_EXCL and populated in place. If interrupted after creation but before the full write, the next normal completion attempt sees partial bytes and fails with content-addressed completion result changed. A disposable partial-object reproduction reached that error. Stage complete bytes in a same-directory temporary file and install atomically; define safe recovery for a pre-existing incomplete object and test that retry path.
动机
把独立验收的 Agent Todo 报告带回 Goal Files 和原始管理对话,是明确的用户闭环;此前完成状态没有可供这两个入口精确读取的报告正文。这个增量有价值,但原对话不能依赖整个 Goal 的历史报告始终健康。
改动思路
todo complete --result-file 把有界本地文本存成内容寻址对象;TS 终态规则在独立验收和生产者身份成立时绑定摘要/提供方/验收依据;读取端重新校验当前 Todo 与验收状态,再经 CLI、loopback API、Files 和管理对话呈现。字节存储从属于 canonical Todo/验收状态,不应形成第二个验收权威。当前的全 Goal 索引和最终对象直接写入,是这条链上的两个失效点。
具体改动
关键代码讲解
acceptedCompletionResult:限定只有已被独立验收、由原生产者完成的 Agent Todo 才能绑定结果描述符;错误分支由终态规则拒绝。store_completion_result/read_completion_result:前者持久化本地字节,后者核对当前 Todo、验收修订、生产者和摘要后才返回正文;读取失效时 CLI/API 不应继续提供旧内容。GoalManagedResults:Files 列表通过 exact read 打开选中报告,失效时清除已显示正文。collectManagedGoalIndex/readManagedPlanResult:原对话先扫描 Goal-wide 列表再匹配计划 Todo;这里需改成计划目标范围内的验证,不应由无关结果决定本计划的可用性。
对主干的风险
验证覆盖了 30 个 Python 测试、14 个 TS 终态测试、desktop/chat 构建、开发与打包态的 Files/管理对话浏览器正例,并检查了基线团队计划路径。上述 P1 负例失败、P2 中断态重试失败仍未解决;打包正例不能覆盖这两项。当前 PR 还显示与主干冲突,修复后需在更新后的 exact head 上重新验证。按本次 review 契约未等待远端 CI;没有远端真实协作/采用证明。默认入口新增 Files 展示,但不扩大外部权限或自动宣称采用;需要保留这些边界。
我的整体评价
方向和 canonical 状态归属合理,Files 正向路径可用;但原始管理对话的核心返回场景存在可复现阻断,故本 head 不宜合并。优先修计划范围内的读取,再补对象写入/恢复的负例。相邻的小范围重构应收敛重复的 Goal-wide 读取策略,而不是再加一层结果权威。
English verdict: REQUEST_CHANGES — the valid plan report is suppressed by unrelated stale Goal results; interrupted object creation also needs a safe retry path.
huangruiteng
left a comment
There was a problem hiding this comment.
Request changes conclusion (author-owned PR; GitHub blocks formal self-review). Reviewed exact head ddd89a1502b58e0e63e9ea7fc12d49222b7c1f5b. This review corrects the first line of my earlier same-head COMMENTED review so the recorded REQUEST_CHANGES verdict is machine-readable; the findings and conclusion are unchanged.
[P1, blocking] Plan-linked report is hidden by an unrelated Goal result. In apps/presentation/dashboard/src/features/personal-workspace/manager-team-result.tsx:163-181, collectManagedGoalIndex scans a Goal-wide paginated inventory and rejects the entire read when any page has unavailable_count > 0, before readManagedPlanResult narrows it to the confirmed plan's Todo IDs. A valid accepted report for this plan therefore disappears from the original manager conversation when an unrelated historical report becomes unreadable. The fixed eight-page bound similarly leaves a matching report after page eight undiscoverable. I reproduced the first case in the existing team-plan browser scenario: the matching row remained valid, but setting an unrelated unavailable count to one made the expected manager report never appear; the unchanged fixture passed. Please resolve/verify the plan's exact Todo IDs, or make incomplete inventory explicit without treating unrelated unavailable rows as failure, and add the stale-row and pagination regressions.
[P2] Interrupted content-object creation is not retryable. In loopx/control_plane/todos/completion_result.py:51-63, the final digest path is opened with O_EXCL and populated in place. If interrupted after creation but before the full write, the next normal completion attempt sees partial bytes and fails with content-addressed completion result changed. A disposable partial-object reproduction reached that error. Stage complete bytes in a same-directory temporary file and install atomically; define safe recovery for a pre-existing incomplete object and test that retry path.
动机
把独立验收的 Agent Todo 报告带回 Goal Files 和原始管理对话,是明确的用户闭环;此前完成状态没有可供这两个入口精确读取的报告正文。这个增量有价值,但原对话不能依赖整个 Goal 的历史报告始终健康。
改动思路
todo complete --result-file 把有界本地文本存成内容寻址对象;TS 终态规则在独立验收和生产者身份成立时绑定摘要/提供方/验收依据;读取端重新校验当前 Todo 与验收状态,再经 CLI、loopback API、Files 和管理对话呈现。字节存储从属于 canonical Todo/验收状态,不应形成第二个验收权威。当前的全 Goal 索引和最终对象直接写入,是这条链上的两个失效点。
具体改动
关键代码讲解
acceptedCompletionResult:限定只有已被独立验收、由原生产者完成的 Agent Todo 才能绑定结果描述符;错误分支由终态规则拒绝。store_completion_result/read_completion_result:前者持久化本地字节,后者核对当前 Todo、验收修订、生产者和摘要后才返回正文;读取失效时 CLI/API 不应继续提供旧内容。GoalManagedResults:Files 列表通过 exact read 打开选中报告,失效时清除已显示正文。collectManagedGoalIndex/readManagedPlanResult:原对话先扫描 Goal-wide 列表再匹配计划 Todo;这里需改成计划目标范围内的验证,不应由无关结果决定本计划的可用性。
对主干的风险
验证覆盖了 30 个 Python 测试、14 个 TS 终态测试、desktop/chat 构建、开发与打包态的 Files/管理对话浏览器正例,并检查了基线团队计划路径。上述 P1 负例失败、P2 中断态重试失败仍未解决;打包正例不能覆盖这两项。当前 PR 还显示与主干冲突,修复后需在更新后的 exact head 上重新验证。按本次 review 契约未等待远端 CI;没有远端真实协作/采用证明。默认入口新增 Files 展示,但不扩大外部权限或自动宣称采用;需要保留这些边界。
我的整体评价
方向和 canonical 状态归属合理,Files 正向路径可用;但原始管理对话的核心返回场景存在可复现阻断,故本 head 不宜合并。优先修计划范围内的读取,再补对象写入/恢复的负例。相邻的小范围重构应收敛重复的 Goal-wide 读取策略,而不是再加一层结果权威。
English verdict: REQUEST_CHANGES — the valid plan report is suppressed by unrelated stale Goal results; interrupted object creation also needs a safe retry path.
ddd89a1 to
ab8530f
Compare
Signed-off-by: huangruiteng <14976749+huangruiteng@users.noreply.github.com>
Signed-off-by: huangruiteng <14976749+huangruiteng@users.noreply.github.com>
Signed-off-by: huangruiteng <14976749+huangruiteng@users.noreply.github.com>
Signed-off-by: huangruiteng <14976749+huangruiteng@users.noreply.github.com>
Signed-off-by: huangruiteng <14976749+huangruiteng@users.noreply.github.com>
Signed-off-by: huangruiteng <14976749+huangruiteng@users.noreply.github.com>
Signed-off-by: huangruiteng <14976749+huangruiteng@users.noreply.github.com>
Signed-off-by: huangruiteng <14976749+huangruiteng@users.noreply.github.com>
Signed-off-by: huangruiteng <14976749+huangruiteng@users.noreply.github.com>
Signed-off-by: huangruiteng <14976749+huangruiteng@users.noreply.github.com>
Review follow-up. The Goal-wide report inventory previously rejected the whole read when any page carried an unreadable row, before the plan's own Todo ids were considered, and a fixed eight-page budget could stop before a matching report. A valid accepted report for the assignment therefore disappeared from its source manager conversation because of an unrelated historical row. The loopback inventory now names the exact Todo ids it could not verify (`unavailable_todo_ids`, with `unavailable_count` kept as `len(...)`), and the manager card pages to the end of the snapshot and withdraws the report only when the confirmed plan's own ids are unverifiable. Unrelated rows stay the Files view's and the Goal's concern. Regression coverage: a Todo whose report sorts past the previous eight-page budget still returns to the manager conversation, an unrelated unavailable row no longer hides the plan's report, and the server test asserts the named unavailable rows instead of a count-only read. Signed-off-by: huangruiteng <14976749+huangruiteng@users.noreply.github.com>
Review follow-up. The content-addressed digest path was created with O_EXCL and then filled in place, so an interrupted store left partial bytes and every later retry failed with `content-addressed completion result changed`; the bytes could never be repaired through the normal completion path. `_install_object` now stages the exact bytes in a same-directory temporary file, fsyncs, and installs them with one `os.replace`. A digest path can only hold the bytes it names, so a pre-existing regular file with matching bytes returns immediately, and a partial or corrupt entry is replaced rather than blocking the retry. `os.replace` installs the staged name without following a link, so an existing symlink at the digest path is replaced instead of written through. Tests cover the idempotent install, an interrupted partial object being repaired by the next store, a symlink not being written through, and the oversized/empty rejection path. Signed-off-by: huangruiteng <14976749+huangruiteng@users.noreply.github.com>
The Todo read-model validator derived the historical pre-extension manifest by filtering the *current* field list, so adding `completion_result` to the v0 manifest silently dropped the manifest that the previous release wrote: every persisted head from that release failed with `coordination Todo read-model field contract mismatch` and a Goal's Todo list could not be read after an upgrade. `TODO_CONTRACT_REVISION_FIELDS` now names one field group per released revision, and validation accepts the current manifest or an exact earlier released shape, still rejecting partial revision groups, reordered, duplicated and unknown fields. Records that carry a field their declared release had not added yet remain rejected. `source_projection.test.ts` also read the historical manifest by filtering the current list; it now reads the frozen `tests/fixtures/coordination/todo-pre-validator-revision-fields.json` instead, and `coordination_projection.test.ts` covers a head written by the previous release plus a current head that carries a completion result. Signed-off-by: huangruiteng <14976749+huangruiteng@users.noreply.github.com>
ab8530f to
c837315
Compare
huangruiteng
left a comment
There was a problem hiding this comment.
Approval conclusion (author-owned PR; GitHub blocks formal self-approval). Approved exact head c83731531baf05b1b9be02da78ba09688f09095c, rebased onto origin/main c088d6378f7152ebce7face677df949d7df3d0ef. This head resolves the two blocking findings from my review on ddd89a150 and one backward-compatibility regression that the rebase exposed; every claim below was re-verified on this exact head.
[P1 resolved] Plan-linked report is no longer hidden by an unrelated Goal row. The loopback inventory now returns unavailable_todo_ids with unavailable_count = len(...), and readManagedPlanResult withdraws the card only when the confirmed plan's own Todo ids are unverifiable, instead of failing the Goal-wide read. collectManagedGoalIndex pages to the snapshot end (the fixed eight-page budget is gone) and rejects a repeated cursor or a page that makes no progress. Verified by mutation: restoring either the whole-Goal rejection or the page budget makes the team-plan scenario time out waiting for 托管团队报告 · 已验收,采用尚未核验; the unmutated head passes, including the new unrelated-unavailable and paged-plan-report fixture states.
[P2 resolved] Interrupted content objects are retryable. _install_object stages the exact bytes in a same-directory temporary file, fsyncs, and installs them with one os.replace; identical bytes return immediately and a partial object at the digest path is replaced by the next attempt. os.replace installs the staged name without following a link, so a symlink at the digest path is replaced rather than written through. tests/control_plane/test_completion_result_objects.py covers the idempotent install, the interrupted object, the symlink and the size limits.
[Regression found and fixed while rebasing] Additive Todo fields must not orphan persisted heads. completion_result was added to the v0 Todo manifest, but the read-model validator derived the historical pre-extension manifest by filtering today's field list, so every head written by the previous release failed with coordination Todo read-model field contract mismatch once this PR added its own field — the class of failure the two historical … Todo head survives upgrade tests exist to catch, and they did. TODO_CONTRACT_REVISION_FIELDS now names one field group per released revision and validation accepts the current manifest or an exact earlier released shape; partial groups, reordered, duplicated and unknown fields still fail, and a record that carries a field its declared release had not added is still rejected. source_projection.test.ts read the historical manifest the same derived way and now reads the frozen tests/fixtures/coordination/todo-pre-validator-revision-fields.json instead. Verified by mutation: removing the completion_result group fails 4 of the 14 projection tests, including the two pre-existing historical ones.
动机
把独立验收通过的托管报告带回 Goal Files 和原始管理对话,是明确的用户闭环:此前完成记录只有校验摘要,没有可核对的正文,请求者只能回到执行者的对话里自行重建结论。这个增量本身有价值,但它的正确性依赖两件事——原对话只对自己计划内的 Todo 负责,以及本地字节必须能被安全地重试写入。上一轮 review 指出的正是这两处的失效点。
改动思路
字节归属不变:canonical Todo 的完成态仍是唯一的验收权威,本地运行时只负责按摘要保存正文,读取端每次都重新核对当前 Todo、验收依据、生产者与字节。本次修复不动这个边界,只修边界的执行方式:服务端把「无法核验的行」具体命名出来,客户端把可用性判断收窄到本计划自己的 Todo id,并把分页读到快照结尾;内容对象改为同目录暂存加一次原子替换。此外补上一条被本次字段新增触发的向后兼容修复:v0 manifest 不换 schema 版本而靠追加字段演进,因此历史形态必须按已发布修订逐组显式列出,不能由今天的字段表反推。
具体改动
关键代码讲解
_install_object(loopx/control_plane/todos/completion_result.py:41):摘要路径只可能承载它命名的字节,因此已存在的非常规内容(中断留下的半截对象、被替换成符号链接的路径)都是待替换的残留而不是竞争对象;匹配则直接返回,不匹配则以暂存文件加os.replace覆盖。_verify_goal_result_page(loopx/chat_completed_todos.py:43):逐行做 exact read 复核,逐行命名无法核验的 Todo id,unavailable_count由该数组长度导出,使读取方可以只关心自己范围内的 id,而不再让无关历史行决定整个 Goal 的可用性。readManagedPlanResult/collectManagedGoalIndex(apps/presentation/dashboard/src/features/personal-workspace/manager-team-result.tsx:211/:165):分页读到next_cursor为空,校验游标推进与页形状;只有本计划自己的 id unavailable 才撤回,exact read 还必须在 goal id、todo id、摘要、生产者、content type 上一致,否则一律撤回而不是沿用旧内容。TODO_CONTRACT_REVISION_FIELDS/validateCoordinationTodoReadModel(loopx/control_plane/coordination/coordination_projection.ts:90/:178):按已发布修订分组列出追加字段,校验要求声明等于当前清单或某个历史发布形态,保留 partial、乱序、重复、未知字段的拒绝,并要求历史声明下的记录不得携带当时尚未存在的字段。acceptedCompletionResult(loopx/control_plane/coordination/todo_terminal_lifecycle.ts:681):只有已完成、经独立验收、且由自身生产者的 Agent Todo 才能绑定结果描述符,畸形描述符以completion_result_rejected拒绝,不会落库。
对主干的风险
验证在 rebase 后的 exact head 上重跑:control-plane TypeScript 全量 3044 项(3016 通过、0 失败、28 跳过)、聚焦 Python 35 项、dashboard tsc --noEmit、build:chat(Chat bundle built and verified)、team-plan 与 managed-goal-results 的开发态与打包态浏览器场景,以及 goal 作用域的 premerge(19 项检查、0 失败、0 人工挂起);精确 diff 的变更质量回执 cqr_68488e2b60a5832c7d99 对该 fingerprint 有效。仍未验证的部分据实记录:本 goal 的 review 策略不等待远端 CI,因此该 head 的远端 lane 未取得结果(上一 head 的 node-forward-compatibility 在负载下 20 分钟超时,属未验证而非通过);仓库级 mypy 在 merge base 上同样失败(local_authority_shadow_projection.py:100,与 origin/main 逐字节相同,且不在 mypy 配置的文件清单内),因此它不能作为本次 diff 的判据。功能边界未扩大:读取仍限 loopback,不会启动 worker、不会分配 Todo、也不宣称请求者已采用;卡片文案明确写「采用尚未核验」。需要后续注意的是同一类风险本身——v0 manifest 今后每追加一个字段都必须登记到修订组,测试会因形态缺失而失败,这条约束已写进回执风险项。
我的整体评价
P1/P2 两项阻断已按最小修复解决并有可复现的反向验证;rebase 暴露的持久化兼容回归属于本 PR 自己引入的字段演进,必须在同一 PR 修完,现已用「按已发布修订显式列组 + 冻结 fixture」的方式修好,两个测试文件都不再依赖被评审实现自身推导历史形态。相邻的小重构也做了且方向正确:去掉了按今天字段表反推历史、去掉了固定八页预算、把历史形态收敛到一处规则与一份冻结 fixture。剩余风险是远端 lane 未等待、原生 Windows 未实测,以及本机仓库级 mypy 在 main 上就已红——这三项都属于记录在案的外部/既有条件,不构成对本 head 的阻断。可交维护者合并。
English verdict: APPROVE - the previous P1 (unrelated unreadable Goal rows and the page budget hiding a plan's accepted report) and P2 (interrupted content objects being unretryable) are fixed with mutation-verified regressions, and the additive-field compatibility regression that this PR itself introduced is repaired by naming one field group per released Todo contract revision so persisted heads from the previous release stay readable.
|
Merged as Changed surfaces: canonical Todo completion binding and the additive Checks run on the reviewed head: control-plane TypeScript suite (3044 tests: 3016 passed, 0 failed, 28 skipped); focused Python suites (35 passed); Mutation checks that fail as intended: restoring the whole-Goal inventory rejection, restoring the eight-page budget, and removing the Skips and holds: remote CI lanes for this head were not awaited under this goal's review policy; repository-wide Residual risk: the manager conversation returns an accepted report while stating that requester adoption is unverified; real mixed-team continuation, explicit adoption, scoped stop/recovery and packaged first-use qualification remain recorded roadmap items. Future additive Todo fields must be registered in |
A managed team could complete an independently accepted Todo while its report stayed absent from both Goal Files and the manager conversation that assigned the work. This PR binds declared report bytes to canonical Todo completion. Goal Files opens the exact accepted version; the original manager plan shows a readable report only when its confirmed receipt names that Todo and exactly one report matches. A stale, unreadable or cross-Goal result is withdrawn. Multiple matching reports require a choice in the Goal.
The existing Todo writer owns acceptance and the local runtime owns report bytes. The loopback-only API revalidates inventory pages and exact reads. The manager card reuses that API and artifact renderer, with a shared bounded inventory read for multiple plan cards. It labels a managed report as accepted while leaving requester adoption unverified. No result read launches a worker or assigns another Todo.
The Windows desktop build also no longer fingerprints the non-build
.gitkeepplaceholder, whose CRLF checkout differed from the committed LF bytes. A regression confirms real public assets still invalidate the bundle. This PR commits source and validation only; the production build generates the Chat bundle. The roadmap checkpoint records the remaining real mixed-team continuation, explicit adoption, scoped stop/recovery and packaged first-use qualification.Review follow-up
Review on
ddd89a150found two blocking defects, and rebuilding this branch on currentmainexposed a third that the same change owns. All three are repaired here.unavailable_todo_ids;unavailable_countstays its length), and the manager card pages the snapshot to its end and withdraws a report only when the confirmed plan's own ids are unverifiable. The fixed eight-page budget is gone. Newunrelated-unavailableandpaged-plan-reportfixture states reproduce both cases; restoring either defect makes theteam-planscenario fail._install_objectstages the exact bytes in a same-directory temporary file, fsyncs, and installs them with oneos.replace. Matching bytes return immediately, a partial or corrupt digest entry is replaced instead of failing every later retry, and a symlink at the digest path is replaced rather than written through.tests/control_plane/test_completion_result_objects.pycovers those paths plus the size limits.completion_resultto the v0 Todo manifest broke the historical manifest rule, which derived the pre-extension shape by filtering today's field list, so every head written by the previous release failed withfield contract mismatchafter an upgrade.TODO_CONTRACT_REVISION_FIELDSnow names one field group per released revision and validation accepts the current manifest or an exact earlier released shape, while partial groups, reordered, duplicated and unknown fields still fail.source_projection.test.tsnow reads the frozen pre-extension manifest instead of re-deriving it.Validation on head
c83731531rebased ontoorigin/mainc088d6378:build:chatreported the bundle built and verified.team-planandmanaged-goal-resultsbrowser scenarios passed in development and in the packaged bundle, including theunrelated-unavailableandpaged-plan-reportstates and the mobile layout check. Mutation checks (whole-Goal rejection, page budget, missing revision group) fail as intended.cqr_68488e2b60a5832c7d99is valid for 38 changed paths. Goal-scoped premerge passed: 19 selected checks, 0 failures, 0 manual holds.local_authority_shadow_projection.py:100).This changes runtime, control-plane and packaged UI behavior, so it remains for maintainer review on its exact head. The related refactor shares the managed content-type mapping and bounds repeated Goal inventory reads without creating a second acceptance authority.