Skip to content

fix(context): recover returned tool results - #118

Open
zhuowp wants to merge 13 commits into
pinvou3-cleanfrom
fix/recoverable-tool-output
Open

zhuowp wants to merge 13 commits into
pinvou3-cleanfrom
fix/recoverable-tool-output

Conversation

@zhuowp

@zhuowp zhuowp commented Oct 22, 2026 •

Copy link
Copy Markdown
Collaborator

No-Issue: adopt upstream-derived complete tool-output recovery and close the current review findings. Parent adoption: Pinvou/pinvou-agent#757.

Preserve engine-returned tool results before route-specific context compaction, including the serial, parallel and aggregate dispatch paths, while retaining artifact failure, immutable publication and policy-filtered retrieval boundaries. The candidate also carries complete Web envelope preservation, workshop-budget test isolation, metadata-summary loss tracking and subagent-report byte contracts. Parent adoption and producer-scope register: Pinvou/pinvou-agent#757.

The latest review's M1 is addressed by declaring the actual producer boundary: tasks.rs caps gate stdout/stderr summaries at 900 characters before the returned DTO; without an active task, log_path is null and pre-DTO command tails are not recoverable. The model view now says this explicitly and the engine recovery footer names the complete returned tool result. The short-sentinel contract directly requires both streams. A real TasksTool long-stream regression checks 12,000 characters per stream, the actual 900-character DTO and no log_path, then uses synthetic post-preservation metadata solely to verify the rendering contract. It does not claim storage/retrieval of the original command log.

M2 is addressed in both register languages: evidence_available results pass unchanged only when they fit the current route; larger previews are refitted using the existing reference, unlike upstream 3e39820's unconditional pass-through. Keep this deliberate current-route bound. Ref-bearing recovery footers now teach tool_search activation and a narrower rerun when unavailable; the garbled route-budget comment is fixed. Maintenance drift is explicitly attributed to public ea9fdc9.

Native validation run 40000699762, validation commit b4eb108: the baseline executes six named source tests, each with an explicit one-pass/nonzero-selection summary. Three isolated variants restore the old footer, remove the producer-boundary note or remove discovery guidance; each causes exactly the new real-tool regression to fail, with the other five still passing. Raw logs and actual checkout SHAs were independently inspected. The validation commit differs from source 2a0221f only in fork-ci.yml; the baseline injects no Rust harness. This focused evidence is separate from full current-head CI. Prior ac8 CI (14,875 passes, 15 skips, tree 18b435a81d13d3fac7c9500806bbbf560909f4ba) remains historical and is not transferred to this changed source.

Self-review found no remaining blocking defect within the declared engine-returned-result slice. Existing producer-cap removal, tiny-budget usability, retrieval expiry/retention, composer-bang preservation and size/metric follow-ups remain explicitly tracked in Pinvou/pinvou-agent#770, Pinvou/pinvou-agent#771 and Pinvou/pinvou-agent#772; instruction enforcement remains in Pinvou/pinvou-agent#768. These deferred boundaries are not claimed fixed. No local Cargo pass is claimed (offline cache lacks ascii 1.1.0); formatting passed. No native Windows, packaged-host, live-provider or release evidence is added by the Linux validation.

The user's separate hold on protected pinvou3-clean remains in force. Public maintenance remains ea9fdc9; no protected reference, tag, merge, queue, auto-merge or release operation is requested or performed. Formal changes-requested reviews remain owned by their reviewers.

Current head: 2a0221f22705c04d6cdac6db29b62b56d4d6738e. The new repair is appended by a normal fast-forward commit; the remote maintainer history is retained.

@JensenChen28 JensenChen28 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.

Standards

No additional required architecture findings from static review. The budget is centralized, serial and parallel publication preserve output first, and existing immutable artifacts and retrieval boundaries are reused.

Spec

The full-suite verification requirement is not met on head 23081ae75227323f81fccf663f9111d70f5ac073. The exact-head check job runs 14,850 tests: 14,847 pass and three fail:

  • commands::groups::debug::tests::cache_inspect_displays_tool_result_budget_metadata: the former 14,000-character truncation expectation no longer matches the larger route budget.
  • core::engine::tests::forkguard_agent_receipt_passthrough_is_bounded: the resulting receipt exceeds its existing bound assertion.
  • core::engine::tests::structured_run_tests_summary_is_recoverable_and_leads_with_failures: the newly added assertion cannot find failure summary inline.

Please reconcile each failure with the intended contract and rerun the complete required suite. Where the new route budget intentionally changes expectations, update the test with evidence; retain the promised bounded fork receipts and useful failure-first summaries rather than merely dropping assertions. The 268 focused native author tests do not replace this failed full-suite receipt.

Static review otherwise found no further required defect in full-envelope/body-only references, immutable replay rejection, or truthful save-failure handling. No local Cargo, live-provider, or packaged-host run is claimed.

Findings: Standards 0; Spec 1 required verification gap (three failing tests).

@JensenChen28 JensenChen28 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.

审核当前23081ae7相对base083cebfd:Standards 0新增、Spec 0新增,批准。

完整13路径及关联保存/检索/事件/wire/TUI链:serial/parallel在fan-out前保全完整输出,immutable writer同字节幂等/异字节拒绝覆盖,失败不发布假ref;Web body-only artifact不冒充完整response envelope。旧adaptive/sidecar所属与检索边界、fork child-route/handle摘要/read续读保留;TUI/wire统一route预算,上下文及磁盘成本已披露。上游3e39820f对象本地可读核对,未把其它上游功能算成本次回移。

实际本轮为完整固定diff、规范/需求/异常与兼容边界审查及diff --check通过。本机无Cargo/Rust工具链,未运行新增Rust回归、完整workspace、真实provider/网络或多平台应用;作者的原生测试数字没有借作本轮执行。已读当前讨论并在发送前核对远程head/base。代码批准不代表CI或交付已通过;不因等待CI只留COMMENT。仅审核,不合并、入队或开启auto-merge。

@JensenChen28 JensenChen28 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.

更正同head23081ae7的先前批准:最终核对真实Pinvou fork CI后发现三项未闭合失败,现请求修改。之前静态审核与作者focused268不能代替完整工作区执行结果。

实际远端run39722973235/job119640425190:format与workspace lint成功;Test workspace汇总14850 run、14847 passed、3 failed、15 skipped;documentation/lockfile步骤未执行。本机没有Cargo,以上明确是远端CI证据,不是本机重跑。

Standards:P2,需将真实回归合同与当前实现闭合。debug/tests.rs:780的14k样本仍要求每次truncated=true/deduplicated=false,但新wire100k上限下首个完整14k、第二去重218是合法行为;应覆盖新上限上/下两侧与metadata,不回退新预算。engine/tests.rs:20000旧总长上限未计必要recovery footer,应分别约束receipt正文与route总预算并验证恢复引用,不能删除footer/放宽真正资源边界。

Spec:P2,新增engine/tests.rs:19606实际失败“failure summary inline”:run_tests结构化分支只给compact_run_tests_result_for_context传raw,直接summary_view返回,metadata.summary从未接入;真实有界失败摘要没有按该新增合同放在stdout前。上游3e39820f对应slice的raw/metadata/budget接线需补齐,保留fork child-route/handle与完整artifact/recovery边界。修后重跑这三例与适用workspace门禁,不仅重跑已过focused filters。

合并前guard已核当前head/base及最新讨论。仅审核,不合并、入队或开启auto-merge。

Comment thread crates/tui/src/core/engine/tests.rs

@JensenChen28 JensenChen28 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.

Standards

No new required findings at 580bbb1d3c195a8c921ce91b890ec637c1e244f0. The complete two-commit follow-up was reviewed against 23081ae75 in an independent clean checkout, with separate Standards and Spec reviews. Lossiness is now tracked at real field/stream/row cuts rather than JSON reformatting; shared bounded helpers preserve the existing summary projections. Checked metadata addition and character-bounded single footers address the corrupt-metadata and small-route edges.

Spec

The source gaps behind the earlier review are addressed: run_tests accepts metadata and route budget, places the bounded failure summary first and retains stream tails; cache inspection checks whole/deduplicated/truncated boundary cases; the receipt test preserves the body bound, overall route bound and real retrieval assertion. Complete structured results no longer request artifacts merely because they were reformatted. Adaptive spills record original line counts, and unsaved recovery previews are excluded from wire deduplication. The new tests retain meaningful assertions rather than removing the failing contracts.

Approve current source. Verification limits: incremental whitespace check and clean/base-ancestry checks passed locally; this environment has no Cargo, so no local native test pass is claimed. On this exact head, CI formatting and workspace lint have passed, while workspace tests are still running (job119757908855). The old head's three failures are not evidence of failure on this new head, and they are not yet claimed green. Author Windows receipts for the earlier follow-up commit are distinct from final-head CI.

The explicitly deferred upstream search/in-tool cap changes remain outside this slice. Parent Pinvou/pinvou-agent#757 still pins 23081ae75; this source approval does not validate that older parent tree or its future maintenance re-pin. Other reviewers' blockers remain theirs to resolve. No merge/queue action.

Findings: Standards 0 new required; Spec 0 new required.

@zhuowp
zhuowp requested a review from JensenChen28 October 22, 2026 10:10
@zhuowp

zhuowp commented Oct 22, 2026

Copy link
Copy Markdown
Collaborator Author

Follow-up for the additional findings in parent Pinvou/pinvou-agent#757: 580bbb1d3c195a8c921ce91b890ec637c1e244f0 preserves cf82289a2's R1 fix and closes real-omission tracking (R2), adaptive line totals (R3), false unsaved-preview dedup, corrupt-length overflow and tiny-budget footers. The parent register explicitly chooses the R4 producer/search deferrals; this does not silently extend the upstream slice.

The exact-head check job passed: Summary [ 422.903s] 14857 tests run: 14857 passed (1 slow, 2 leaky), 15 skipped. Every changed/new regression is present as PASS in the real log; formatting, workspace lint, docs and lockfile drift also passed. The isolated Windows function probe (5 passed; baseline functions 1 passed/4 failed) is separate from this Linux workspace receipt. No local full-TUI or packaged-host pass is claimed.

@asto18089 @JensenChen28 please re-review this revision and the still-open old-head CI thread. Source self-review found no remaining required in-scope defect. Normal integration/merge rules still apply; no merge or release action.

@JensenChen28 JensenChen28 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.

复核 head 580bbb1d3c195a8c921ce91b890ec637c1e244f0,目标 base 083cebfd9aac5f6b9993ed610383fe2b38c6785f。cf82289a 的三项既有失败已修复;独立读取该旧 head 的 CI 日志确认 14851 passed / 0 failed / 15 skipped。该结果不能代表新 head;新 head 的 check 尚在运行。本机无 Cargo,以下为实际源码调用链审查,未声称执行 Rust 测试。

最新增量仍有两处需修复:

  • [P2] crates/tui/src/core/engine/context.rs:733–735:普通聊天在没有 active_task_id 时调用 tasks {action:gate_run},命令同时输出短 stdout 和 stderr。tasks.rs 生成优先 stderr 的 gate.summary,同时提供独立两路 summary;此时 write_runtime_artifact 返回 None。这里只选 gate.summary,未展示 stdout;因文本未触及截断上限,lossy=false,summary_view/preserve helper 不保存原文或给恢复引用。stdout 因而无提示丢失。请展示两路有效输出,或将未展示的非空流计为有损,并覆盖无 active-task 的真实 DTO。
  • [P2] crates/tui/src/core/engine/context.rs:524:短子代理快照只按文本截断标记 lossy,但仍省略 checkpoint/needs_input。真实 Interrupted producer(tools/subagent/mod.rs:11993–12050)另外返回 needs_input,且 needs_input_for_interrupted_checkpoint:11100–11109 将 continuation_handle 加入 question;reason 本身不含这个值。无既存 artifact 的短回执因此会以 lossy=false 跳过原结果保存,使父模型失去具体恢复动作与 handle,无法从 retrieve_tool_result 取回。请保留这些协调字段或对有效字段遗漏标记有损,并用真实中断快照回归。

两处均是本次将 text!=raw 改为精确 lossiness 后的回归;格式变化本身无需算截断,但实际省略的信息仍须可恢复。极小预算尾注、unsaved preview 去重保护及计数溢出防护未发现新增问题。未执行合并、入队或 auto-merge。

@JensenChen28 JensenChen28 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.

Standards

No new required findings at 293d7376a82b59189c01503f71f1c83880a33a4c. Reviewed the complete follow-up from580bbb, including the successor to8424c74c, with independent Standards and Spec axes. Changes remain in existing summary responsibilities and preserve recovery/permission boundaries.

Spec

The two concrete omissions from review5612564732 are addressed: distinct effective task-gate stdout/stderr are now retained inline, real cuts request recovery, and empty-stream sentinels do not cause false artifacts. Subagent checkpoint/needs_input are detected before unwrapping and in inner snapshots, retaining access to continuation handles. Coverage includes real TasksTool execution with no active task, a real serialized interrupted SubAgentResult built with the actual needs-input producer, original artifact-byte/DTO checks, and focused summary fixtures.

Approve current source. The prior580bbb approval missed these valid producer-level cases; this review checks their actual producers and the corrected call chain. Local validation is clean exact-head checkout, maintenance ancestry and incremental whitespace/source review; this environment has no Cargo, so no local native test pass is claimed. The previous580bbb workspace result14857passed/15skipped is a receipt for that older head only. Current-head full CI must be tracked separately.

Parent Pinvou/pinvou-agent#757 still pins580bbb and does not contain these fixes. Its updated maintenance gitlink/register/count and resulting integrated tree need a separate review. Other reviewers' decisions remain untouched. No merge, queue or automatic-merge action.

Findings: Standards 0 new required; Spec 0 remaining required in this follow-up.

@JensenChen28 JensenChen28 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.

复核最新固定 head 293d7376a82b59189c01503f71f1c83880a33a4c,base 083cebfd9aac5f6b9993ed610383fe2b38c6785f,含8424中间修复及后续双流调整。

Standards

0新增问题。gate.summary之外的不同非空stdout/stderr现各自有界内联,真实截断仍标有损;空流哨兵不制造虚假恢复。子代理在解包前识别内外checkpoint/needs_input,继续经原串行/并行保存链保全原文。原两项P2闭合,未另建恢复机制或放宽预算与归属边界。

Spec

0新增问题。新增真实无active-task的TasksTool双流回归,以及真实Interrupted SubAgentResult序列化、原文artifact和continuation_handle恢复断言;与“实际遗漏须可恢复、完整结果不虚报恢复”的要求一致。正文披露的producer/search等延期未被扩大成已经交付。

实际验证:本轮固定对象diff --check通过,生产者/消费者和新回归源码完整审阅。本机无Cargo,未执行Rust/native,也不把作者7项隔离probe当成本轮运行。独立核验的580旧CI 14857PASS/0FAIL/15SKIP仅属旧头;当前293 check仍在运行,不声称当前全套通过。当前没有未解决的实质源码阻塞,提交APPROVED;没有合并、入队或auto-merge。

@zhuowp
zhuowp requested a review from JensenChen28 October 22, 2026 10:49
@zhuowp

zhuowp commented Oct 22, 2026

Copy link
Copy Markdown
Collaborator Author

Latest review follow-up: parent 64b348ae0467827f90e987b4acfe13ffbd48bc2c adopts foundation 293d7376a82b59189c01503f71f1c83880a33a4c (81 commits above v0.9.12). The remote parent rebase at 4370ed942aedf1e51d26622ab0f89a232cb54c7b is preserved.

The two P2 findings on 580bbb1d3 are fixed: distinct effective task stdout/stderr summaries stay inline even with no active task/log; omitted non-null child checkpoint/needs_input state retains a full-result recovery path, including wrapper fields. Real cuts remain recoverable, while complete results and empty-stream sentinels do not create false artifacts. Regressions execute TasksTool's real gate DTO and the real interrupted-checkpoint/needs-input builder, with retained/restored continuation data.

The fixed-head full check passed: Summary [ 513.673s] 14861 tests run: 14861 passed (1 slow, 2 leaky), 15 skipped; the actual log records the four new contracts and prior R1/recovery contracts as PASS. Formatting, lint, docs and lockfile drift passed. Local isolated Windows function tests were 7 passed, versus 5 passed/2 failed using the pre-fix 580bbb1d3 functions; this is separate from the real Linux producer/workspace run. Local full locked TUI could not fetch dependencies and is not claimed green.

Parent architecture, 235 behavior fingerprints, 61 policy/floor checks, syntax/linkage/whitespace and wrong-head/count/ancestor controls pass. Parent remote fast-gate fails at the unchanged public verifier (maintenance 083cebf versus candidate 293d737); required-gate consequently fails and downstream jobs are skipped, not passed. The unchanged public gate still rejects the unmerged candidate; codewhale-hq#757 remains a dependency draft pending #118 integration, actual maintenance re-pin and synchronized registration. R4 producer/search deferrals and remaining nonblocking boundaries stay explicit.

@JensenChen28 @asto18089 please re-review these exact source fixes. Formal requests submitted. No merge or release action; new-head source/CI receipts do not replace external review or final integrated-tree approval.

@JensenChen28

Copy link
Copy Markdown
Collaborator

已核对仍为293d7376:我已在该固定提交完成两轴复审并提交 APPROVED(https://github.com/Pinvou/CodeWhale/pull/118#pullrequestreview-5612880323),两项P2及旧CI回归线程均已闭环,本次不重复批准。现独立读取你链接的精确head原始check日志,确认14861 PASS / 0 FAIL / 15 SKIP(1 slow、2 leaky),与批准时仍在运行的状态分开记录。本机未运行完整Cargo;作者隔离Windows probe和父PR的登记/门禁数量不计作本轮独立验证。父PR整合及维护者合并决策仍需其各自审核;本自动化未合并、入队或开启auto-merge。

@JensenChen28 JensenChen28 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.

Standards

[P2] Add regression coverage for the actual save-before-fan-out boundary. Current serial and parallel turn-loop calls to preserve_tool_output_for_context are correctly placed before delivery, but existing recovery tests invoke the view/preservation helpers directly. The only execute_planned_tools test uses a small tool_search result that never needs this new preservation branch. Component tests do not establish that actual execution invokes preservation before a result is shipped.

Spec

This is the central data-preservation contract of the change: execute an over-budget result that has no prior artifact through the real serial and parallel paths, then assert the emitted/completed outcome carries a ref retrieving the original bytes. The test must exercise this branch, rather than an earlier tool-time spill that already has an artifact. A refactor removing either connection should fail the corresponding regression. Existing result-style tests remain useful; the gap is specifically execution wiring, not absence of recovery tests overall.

Correcting current approval for this remaining verification gap after independently reading the call sites and tests, in agreement with parent peer review5614189272. No current production ordering defect is asserted. Peer mutation counts are not this review's execution: this environment has no Cargo and did not run those mutations. The exact-head green CI remains valid for the existing suite, which does not cover this boundary. Earlier source fixes remain resolved.

Request changes for this focused high-risk contract coverage. No business-code change, merge or queue action; other reviewers retain their decisions.

Findings: Standards1 required P2 verification gap; Spec same save-before-delivery contract, no additional ordering defect.

@zhuowp
zhuowp requested a review from JensenChen28 October 22, 2026 13:10

@JensenChen28 JensenChen28 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.

复审当前 5ea533e4141e93a5464af7bd4a58f521b48110cc 相对维护基线 083cebfd9:Standards 0新增、Spec 0新增,批准。此前实际 save-before-fan-out 边界的回归覆盖缺口已由本次两文件209行测试闭合。

新增测试通过真实 Engine/ToolRegistry 调用 execute_planned_tools;20k字符+Unicode超过15360模型视图预算,并事先断言没有工具层spill artifact。串行臂禁parallel,双call并行臂以Barrier(2)和15秒超时阻止静默串行退化。实际ToolCallComplete与terminal outcome的原文/metadata一致;逐ref经真实RetrieveToolResultTool在原session读取精确原bytes,双call独立artifact,模型视图保持预算。生产turn_loop未改变,原修复没有被重写。

独立实际验证:固定diff --check通过;本机没有Cargo,未运行Rust/native。另独立读取远程baseline job119837875690的原始日志,实际两项具名测试2PASS/0FAIL;远程helper mutation job119837875134实际两项在保全前置断言处失败(0PASS/2FAIL),其job绿色代表确认预期失败,不代表突变测试通过。两个job均在验证提交5a850ebc,已独立对比其相对本PR源5ea533e4仅改变.github/workflows/fork-ci.yml,测试及生产源码一致。作者其余控制/全套数字未借作本轮证据;本PR当前head的完整CI单独跟进,不因CI仍运行而只留COMMENT。

维护分支集成、父仓gitlink/register/public-head门禁和最终组合验证仍需独立处理;此次源码批准不等于它们通过,也不覆盖任何后续提交。没有修改PR代码、合并、入队或开启auto-merge。

@zhuowp

zhuowp commented Oct 22, 2026

Copy link
Copy Markdown
Collaborator Author

R1 save-before-fan-out follow-up is now pushed and verified. Current source: 5ea533e.

Two registered-tool regressions execute the real serial and parallel arms of execute_planned_tools. A 20,000-character body plus Unicode exceeds the 15,360-character route view but preflight proves no tool-time spill. They require matching completion/outcome artifact refs, session-owned retrieval of every original byte and distinct refs for concurrent calls. The parallel barrier requires actual concurrent dispatch. Environment, workshop and artifact roots are guarded. Runtime code is unchanged.

Native execution and counterexamples:

  • Baseline: 2 passed / 0 failed.
  • Disabled shared helper: 0 passed / 2 failed.
  • Removed serial call: serial fails, parallel passes (1/1).
  • Removed parallel call: parallel fails, serial passes (1/1).
    All negative cases execute both tests and fail at the missing pre-fan-out artifact assertion. Their green validation jobs confirm expected test failure. Initial call-deletion attempts stopped at unused-variable lint and are explicitly excluded. Validation commits 5a850eb / 52b20a0 differ from source only in the validation workflow, with identical TUI source trees.

Exact-source full PR check passed: Summary [ 538.797s] 14863 tests run: 14863 passed (1 slow, 2 leaky), 15 skipped. The actual log contains both new test PASS lines and the prior repaired/recovery contracts; formatting, lint, docs and lockfile drift passed. CI merge checkout 544b675 has source-equivalent tree e8ffd927d8dfcbe3433b7fbf9201a2ac7b65db1c. Local full Cargo remains blocked by the incomplete offline dependency cache; it is not claimed to have passed.

Parent 9e9ffd30 pins both new tests, synchronizes the candidate/count and twelve recovery fingerprints in both language pairs, and explicitly registers live TUI receipt removal. Architecture, fast fingerprints (237 behavior names), 60 policy tests, 1 floor test, syntax/whitespace and invalid head/count/ancestor controls pass. Public maintenance remains 083cebf; the unchanged public verifier blocks the unmerged candidate and codewhale-hq#757 stays a dependency draft. Nonblocking observations remain separate from this required coverage fix.

@asto18089 please re-review the fixed heads; formal requests are already submitted. JensenChen28 has independently approved each current head after checking the new execution coverage. Source self-review found no remaining required gap in this revision. External approval, complete integrated-tree/public-gate eligibility and release evidence remain separate. Per the user’s explicit decision, candidates are retained without advancing pinvou3-clean. No merge, tag or release.

@asto18089 asto18089 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.

Verdict

Fresh full review of 5ea533e41 (branch already sits on the latest pinvou3-clean tip 083cebfd9 — merge-base equals base head, nothing to rebase). The slice is real and root-caused: one route inline budget (route_inline_char_budget) replaces the four disagreeing limits, recovery is wired before both fan-out arms, and the round-5 save-before-fan-out gap is closed by real-dispatch regressions that I verified bite (serial/parallel call removal and fake-write mutations all fail; the Barrier(2) genuinely forces concurrent dispatch). I accounted for upstream 3e39820f5 line-by-line: every divergence is a disclosed fork-direction fix (immutable write, stricter legacy-ref ownership, bounded child summaries, dedup-hint gate, actual-omission tracking, ~17 extra pins); no smuggled changes, the 4 extra files are in-scope tests/producer fixes, DCO clean, CI green on head. Local full locked lib suite: 12,238 passed / 0 product failures (2 restart-family flakes pass in isolation).

Two items keep this from approval:

Required

[MAJOR] multi_tool_use.parallel envelope still fans out unrecoverable cuts — the exact failure mode this slice exists to fix. The envelope branch (turn_loop.rs:4805) ends in continue at turn_loop.rs:4852, before both spillover (:5199) and preservation (:5224). execute_parallel_tool (tool_execution.rs:340-475) embeds each sub-result's full content into one JSON envelope and never spills inside it, so an envelope over the route budget (two ~8 KB read results on a 128k route) is cut at turn_loop.rs:5415 with needs_full_output_artifact = true that nobody serves: the footer honestly says the output could not be saved, but the bytes are gone — while the sibling parallel-dispatch arm (:4674) got the treatment. Pre-existing on base and identical upstream, but the new doc comments state "save the full output … before the result fans out" universally. Fix by calling spillover + preserve_tool_output_for_context in the envelope branch before the continue, or disclose the deferral explicitly in the PR body with a filed follow-up. The same hole exists upstream and is worth an upstream contribution either way.

[P2] The always-preserve mutation survives. No test fails if the needs_full_output_artifact gate in preserve_tool_output_for_context (turn_loop.rs:48) is dropped. Verified mechanically: full lib suite with the guard short-circuited to true shows no deterministic failure — the only dispatch-level callers are the two fanout regressions (whose outputs need preservation either way) and a tool_search test with no artifact assertions; display_compaction_never_writes_artifacts never enters the loop. If the guard regresses, every tool call writes an artifact file plus 8 metadata keys — disk and artifact-listing noise with zero coverage noticing. One dispatch-level negative test (under-budget fixture → assert!(session_artifact_files(home).is_empty()) and no artifact_id metadata) closes it.

Non-blocking

  • [MINOR] tool_output_receipts.rs:11 — /status "raw over cap" counts the engine's own ≤budget views (15,360 chars on 128k) because the 12,000 threshold predates the single budget; already disclosed in the body as "the existing status heuristic". Agree it should derive from route_inline_char_budget_for_route in a follow-up.
  • [MINOR] chat.rs:2008 — TOOL_RESULT_EXCERPT_FRAME_CHARS = 1_024 under-provisions the worst-case header (~1,073 chars with long MCP names + command + status), so the compacted string can exceed sent_budget by a few dozen chars. Harmless at 100k, but the frame invariant is not structural.
  • [MINOR] truncate.rs:832-840 — refit's subtractive line count can print "(0 lines)" while bytes remain omitted (both cut points land mid-line and the hidden middle has ≤2 newlines). Footer honesty only.
  • [MINOR] subagent/mod.rs:16271-16282 — stale doc comments still cite the constants this PR deletes (TOOL_RESULT_SENT_CHAR_BUDGET, TOOL_RESULT_HEAD_CHARS/_TAIL_CHARS, chat.rs:1377-1379).
  • [MINOR] stream_decoder.rs:1177-1214 — the rewritten truncation test recomputes expected head/tail with the implementation's own formula (tautological w.r.t. the formula); the independent budget pin carries it, but exact-value assertions would be stronger.

Verified safe (highlights)

  • Central contract: on every realistic path the view respects the budget, and omission ⇔ artifact request, including the web envelope case (body-only artifact does not cover the envelope). fit_to_inline_budget/refit_spilled_preview never exceed budget; char-boundary snaps + checked_add reject forged metadata; preserve is idempotent; conflicting same-id replay fails closed; write failure is honest with no retry loop.
  • Persistence round-trip: ContentBlock::ToolResult carries no metadata by design; restored cut views stay correct because footers embed the path/art_ ref that retrieve_tool_result resolves from persisted records; the wire gate passes hint-bearing cuts uncut and every current unsaved format contains the hint.
  • The live-receipt deletion is clean: one production caller removed, zero dangling references, no orphaned i18n keys; inline view cost on the event loop is bounded and total work dropped versus base.

Findings: 1 MAJOR + 1 P2 + 5 minors; everything else checked out. Happy to re-review promptly once the envelope path is fixed or explicitly deferred.

Comment thread crates/tui/src/core/engine/turn_loop.rs
Comment thread crates/tui/src/core/engine/turn_loop.rs

@JensenChen28 JensenChen28 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.

Standards

No defect found in the newly added ordinary serial/parallel dispatch tests; the previous wiring-coverage gap remains resolved.

Spec

[P2] Account for the separate multi_tool_use.parallel envelope path. Independently traced turn_loop.rs:4805-4852: this branch publishes completion/outcome and continues before the spillover and preserve helper at5199/5224. execute_parallel_tool collects full child content into JSON without a recovery artifact, and process_tool_results later applies the route cut at5415. A valid envelope exceeding15360 characters can therefore be omitted without recoverable original bytes. This is an inherited branch omitted from the new capability, not a newly introduced regression; it conflicts with the universal save-before-fan-out claim. Either apply the same preservation flow to the final envelope before delivery and cover that real branch, or explicitly narrow the claim and register the deferred unrecoverable path/follow-up.

[P3, nonblocking] Current dispatch fixtures all require preservation. A small complete-output dispatch assertion for no artifact files/metadata would protect conditional storage; no current always-write production defect is asserted, and a surviving peer mutation alone is not treated as a mandatory blocker.

Correcting current approval for the remaining envelope scope gap after independent Standards/Spec source review, matching the specific path in peer review5616035419. Ordinary parallel batches and the special envelope are different execution paths. Peer native/mutation counts are not this review's execution; no local Cargo run is claimed. Existing exact-head green CI remains valid for its covered paths. Other reviewer decisions remain untouched. No business-code change, merge or queue action.

Findings: Standards0 new defects; Spec1 required P2 scope gap and1 nonblocking P3 coverage suggestion.

@JensenChen28 JensenChen28 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.

复核当前 5ea533e4 的新讨论后,修正此前批准:仍有一项 P2 功能完整性缺口。

Standards:确认 crates/tui/src/core/engine/turn_loop.rs:4805–4852 的 multi_tool_use.parallel 分支在完成事件/outcome 后直接 continue,绕过普通路径5199/5224的 spillover/preserve。子结果在 tool_execution.rs:449–474 直接合成完整 JSON,后续5415仍按路由预算剪裁;例如两个未各自spill的约8k读取结果,合成信封可超过15,360字符视图,省略内容没有可检索副本。

Spec:这条分支在base已存在,不是本次测试提交新引入的回归;但当前“engine input / before fan-out”恢复承诺尚未覆盖这条可达路径。请沿已有envelope线程补齐分支交付前的保全与实际调度回归;如决定延期,需明确缩窄本slice承诺并具名跟进。这里不重复建立inline线程。

under-budget调度负控建议补充,但当前条件本身正确,测试缺口不单独认定为第二项生产P2。独立验证仅限源码、base归因及调用链;本机无Cargo,未运行同行突变或新Rust用例,既有远程全套通过不能证明这条未覆盖路径。

@JensenChen28 JensenChen28 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.

Standards

No new required findings. The special envelope now reuses the established spillover/audit/conditional-preservation flow before completion/outcome fan-out; content blocks and terminal error/cancel semantics remain intact. The change stays within the existing execution boundary rather than adding a separate recovery mechanism.

Spec

The P2 from review5616263652 is resolved. The actual multi_tool_use.parallel branch now preserves an over-budget combined envelope, including child failures carried inside a successful envelope result. Its new real-dispatch regression uses two individually inline8k child results whose combined JSON requires recovery, then verifies completion/outcome metadata and retrieval of the original aggregate bytes. Small complete results on serial, parallel and envelope paths now also assert unchanged content, no artifact metadata and no files, closing the nonblocking conditional-storage coverage suggestion.

Approve current source. Independently reviewed the production branch and test call chain with separate Standards/Spec axes; clean exact-head checkout and whitespace check passed. This environment has no Cargo, so no native/mutation execution is claimed. Current-head CI remains in progress; the old5ea suite pass is not evidence for this changed runtime head. The parent candidate must track this version and later the authorized public-maintenance SHA, with its final integrated-tree review.

Other reviewer decisions remain untouched. No business-code change, merge, queue or maintenance-branch action.

Findings: Standards0 new required; Spec0 remaining required from this fix.

@JensenChen28 JensenChen28 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.

已复核 5ea533e4 → edd2e1fb 的完整两文件增量,原 envelope 保全 P2 已修复,当前源码审核通过。

Standards:0项新增问题。特殊 envelope 分支在完成事件和outcome分发前复用现有spillover/audit及条件preserve,原session/tool身份、取消/ToolError和rich内容路径保持。

Spec:0项新增问题。真实调度回归覆盖两条各自inline的8k子结果组成超预算JSON,完成事件与结果一致,并按原session取回完整字节;三种调度的小结果均要求不生成artifact且模型视图原样。已读两条修复回复,不重复建立或代为关闭其他审核者的线程。

独立读取实际原始native日志及job元数据:基线 6 PASS/0 FAIL;仅移除envelope保存 5 PASS/1 FAIL,失败在缺少分发前artifact;始终保存 3 PASS/3 FAIL,正是三条小结果负控。绿色变异job验证预期失败,不表示变异后套件全绿。验证提交 8f096194 与当前 edd2e1fb 的比较仅有workflow文件差异,已独立核实。

本机无Cargo,未执行本地native/变异;上述为独立核验的远程六项证据,不借作者陈述,也不把旧头14863项结果继承到新头。当前精确源码完整CI仍另行跟进,不宣称全套或发布验证已通过。

@zhuowp

zhuowp commented Oct 22, 2026

Copy link
Copy Markdown
Collaborator Author

Envelope-path and conditional-storage follow-up is pushed and verified. Current source: edd2e1f.

The special multi_tool_use.parallel branch now invokes existing spillover/audit and conditional preservation before completion/outcome delivery. The native envelope regression executes two individually inline 8k child bodies plus Unicode through the real dispatcher, requires child concurrency with a barrier, and retrieves every original aggregate byte from the shipped session-owned ref. Separate small-result cases protect artifact-free storage on serial, parallel-batch and envelope paths. Cancellation/error handling and rich content blocks keep their existing branches.

Native evidence (validation commit 8f09619 differs from source only in the workflow; identical TUI tree):

  • Baseline: 6 passed / 0 failed.
  • Envelope connection removed: 5 passed / 1 failed; only aggregate recovery fails at the missing pre-fan-out artifact.
  • Gate forced always-preserve: 3 passed / 3 failed; exactly the small complete results reject unexpected artifact metadata.
    All six named tests execute in each case. Mutation-job success verifies expected test failure, not green mutant tests.

Current-source full PR check succeeded: Summary [ 559.272s] 14867 tests run: 14867 passed (1 slow, 2 leaky), 15 skipped. Actual PASS lines include all six dispatch cases and the prior repaired/producer contracts. Formatting, lint, doc tests and lockfile drift passed. Actual checkout d429b83 has complete source-equivalent tree a4adf95489539be54b0f088f312f0f8ae79f2eee. Local offline Cargo remains blocked by uncached ansi-to-tui 8.0.1, so no local full-suite pass is claimed.

Parent 16d7c475 preserves the remote main rebase and synchronizes the candidate, 83-commit count, sixteen recovery fingerprints and both language pairs. Architecture, 241 static behavior names, 60 policy tests, 1 floor test, syntax/whitespace and wrong-head/count/ancestor controls pass. The public verifier remains unchanged and rejects maintenance 083cebf versus the candidate; codewhale-hq#757 stays a dependency draft. Source scope and unchanged synthetic control-reply boundaries are explicit in both registers and this description.

Current-head source approvals: JensenChen28. @asto18089 please re-review the fixed heads; formal requests and concrete inline replies are already submitted. Source self-review found no remaining required gap in this revision. Final public-maintenance integration and integrated-tree review are still separate. Per the user’s decision, candidates remain without advancing pinvou3-clean. No merge, tag or release.

@zhuowp
zhuowp requested a review from JensenChen28 October 23, 2026 02:37

@JensenChen28 JensenChen28 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.

已独立复核本 head 新收到的评审意见,先前 APPROVED 现改为 CHANGES_REQUESTED:确认本次新增保存调用与既有私有状态写入契约存在需要修复的一致性问题,在修复并补充相应验证前不再批准。按 SECURITY.md 的私下披露要求,完整定位、触发条件、影响及建议仅保存在本地审核记录,不在此扩散技术细节。

对 M1 另作区分:首次 oversized fetch 也可能发生额外 envelope 保存,不能只描述为 route shrink 后才触发;但当前代码先保存完整正文,之后 art_ 保存的是 engine 实际收到的完整 ToolResult JSON。两跳可回读正文,目前未证实原文字节丢失或悬空引用,不沿用“已发生数据丢失”的解释。扩展触发说明并补真实 producer→engine→retrieve 的两跳回归仍是有价值的后续。

本轮仅固定源码及已有证据的独立交叉复核,没有运行 Cargo、POSIX权限实测、真实HTTP/provider或新原生测试;此前14870 PASS/15 SKIP只证明相同源码树的既有CI执行,不能证明本次指出的边界已受覆盖。未修改PR代码或执行合并、入队、auto-merge。

Signed-off-by: zhuowp <9426526+zhuowp@users.noreply.github.com>
Signed-off-by: zhuowp <9426526+zhuowp@users.noreply.github.com>
@zhuowp

zhuowp commented Oct 23, 2026 •

Copy link
Copy Markdown
Collaborator Author

Review follow-up for 72e1d1b

@asto18089 @JensenChen28 please re-review the fixed source and its actual current diff. The complete-fetch boundary is now one-ref recovery of the original valid JSON, preserving the direct RLM consumer. The privately tracked consistency observation has a focused fix and native validation. Author self-review also repaired deferred recovery activation through the existing policy-filtered catalog; structured-view rules are unchanged.

Actual native execution at validation 6036af4 (run39903026131), whose only tree difference from source 72e1d1b is the validation workflow:

  • New named contracts: fetch 2 passed / 0 failed; storage 1 passed / 0 failed; activation 1 passed / 0 failed.
  • Related suites: fetch_url 16 passed / 0 failed; artifacts 5 passed / 0 failed; real-dispatch recovery 6 passed / 0 failed; tool catalog 19 passed / 0 failed.
  • Old body-cut control: fetch 1 passed / 1 failed, storage and activation each 1 passed / 0 failed. Only the complete-envelope regression fails.
  • Storage-policy control: fetch 2 passed / 0 failed, storage 0 passed / 1 failed, activation 1 passed / 0 failed. The privately tracked observation is not expanded into a public report.
  • Disabled activation control: fetch 2 passed / 0 failed, storage 1 passed / 0 failed, activation 0 passed / 1 failed. Only the missing next-step recovery schema fails.

These are real native test outcomes, not zero-match filters or compilation failures. Positive inputs include oversized Unicode success/error bodies and an oversized header, byte-exact chunked one-ref recovery, small unchanged JSON, replay and actual catalog admission. The post-network fixture does not claim a live HTTP/provider/Windows/package run.

Local parent fast fingerprints (250 names), architecture, 60 gate-policy tests and whitespace passed. The completed current-source formal CI receipt is appended below; it remains separate from selected mutation receipts. The unchanged public verifier rejects candidate 72e1d1b against maintenance ea9fdc9; codewhale-hq#757 stays draft. Existing CHANGES_REQUESTED remains until each reviewer records a new decision. Approved codewhale-hq#750/codewhale-hq#754/codewhale-hq#755 were not edited; optional items are tracked as issues. No protected maintenance push, merge, queue, tag or release.

Completed current-source CI receipt

CodeWhale source 72e1d1b now has successful check120251648473 and gate120256203810. Independently read the actual native log:

Summary [426.025s] 14873 tests run: 14873 passed (1 slow, 2 leaky), 15 skipped

Both fetch contracts, the storage contract, deferred-schema activation and all six dispatch recovery/artifact-free contracts have explicit PASS lines. Formatting, workspace lint, documentation tests and lockfile drift steps succeeded. Actual PR-merge checkout def7bd0 and source 72e1d1b share complete tree 91ed7e13592ef865acce840b4d07052138cd84bd. This is current-tree Linux CI, not old-tree status transferred to a changed source.

Parent codewhale-hq#756 at 1ab60ceaf1be12fa3e67cd4c814f141aab6ab068 has successful required-gate. Parent codewhale-hq#757 at 418e6ee4d6538442060d531e254189c39f05ac94 still correctly fails fast-gate solely on unchanged public maintenance alignment; its required gate also fails. No gate is waived. These receipts do not dismiss external review decisions or establish packaged-host/provider/release qualification.

@JensenChen28 JensenChen28 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.

Standards

0 remaining required findings. The privately tracked consistency observation is addressed in the reviewed revision and covered by a focused native regression. Immutable publication/replay behavior and policy-filtered tool admission are retained; private technical details are not repeated here.

Spec

0 remaining required findings. fetch_url now retains the original complete valid JSON response for direct consumers; the engine preserves that complete envelope before cutting its model view. The real post-network contracts cover oversized Unicode success/error bodies, a large header, exact chunked one-ref retrieval and an unchanged small result. This resolves the earlier first-execution recovery-boundary P2. Deferred retrieval activation goes through the existing available catalog and activation cache; a filtered-out schema is not reconstructed or bypassed.

Validation: exact clean72e1d1b4 checkout and whitespace check; independent Standards/Spec source reviews. We independently checked current native job120251648473 and gate120256203810:14873 passed,15 skipped; formatting,workspace lint,documentation and lockfile steps successful. Selected native baseline120248630028 executes the new contracts and related suites; validation6036af differs from this source only in its workflow. Three targeted negative controls fail on their intended runtime assertions, with the remaining contracts passing; green control jobs verify expected failures. These are inspected remote native receipts, not local Cargo/live HTTP/provider/Windows/package execution.

Standards:0; Spec:0. Source approved. Parent public-maintenance alignment and other reviewers decisions remain separate.

@JensenChen28 JensenChen28 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.

已按当前 head/base 复核完整差异与相对上一轮的三笔提交。

Standards:无新增阻断;上一轮私下记录的一致性问题在本次新增写入范围内已修复,完整技术证据继续仅保存在本地。
Spec:无新增阻断;fetch producer 先返回完整有效 JSON,再由 engine 保存原始 envelope 后裁剪,保留 RLM 直接解析的消费契约;新增回归覆盖成功/错误正文、大 header、单 ref 分块回读和小结果不建 artifact。恢复 schema 激活仍受当前 catalog 限制,旧 evidence flag 路径保留。此前 CHANGES_REQUESTED 现更新为 APPROVED。

实际验证:本地仅源码/调用链交叉检查及 git diff --check(通过),未运行 Cargo、POSIX 权限、HTTP/provider、Windows 或打包测试。独立核实当前 head 的 check120251648473 和 gate120256203810 均成功,格式、workspace lint/test、doc test 和 lockfile 步骤成功;API 核实 CI checkout def7bd0 的 tree 与本 head 同为91ed7e13592ef865acce840b4d07052138cd84bd。原始 native 测试日志下载失败,未独立复核具体测试计数或 mutation 结果,不将作者计数记为本轮验证。

范围内未发现未解决的实质问题。其余已披露的 producer/预算/存储配额等边界保持后续跟踪;本审核不替代其他审核者决定。未修改 PR 代码,未执行合并、入队或 auto-merge。

Signed-off-by: zhuowp <9426526+zhuowp@users.noreply.github.com>
@zhuowp

zhuowp commented Oct 23, 2026 •

Copy link
Copy Markdown
Collaborator Author

Recovery review fixes at c2d5ca6

@asto18089 @JensenChen28 please re-review, including parent codewhale-hq#757 review M1/m1/m2. The >800-character stream fixture directly requires recovery, and all three dispatch contracts now continue through real result recording to assert the next-step retrieval schema. Explicit discovery/fallback guidance covers the inherited subagent-report and legacy-spillover paths. Published history is preserved; engine-wide Unix immutable-artifact hardening is explicitly described in the refreshed body.

Focused native validation 39915840040 passed ten named baseline tests. Disabling stream loss makes the target assertion fail; disabling activation makes all three large dispatch paths fail while three complete-output controls pass. Exact identities and exit codes are checked. Validation 6b95efa differs from source only in its validation workflow. Old full CI at 72e1d1b is not transferred to this head; new source CI is separate. No protected maintenance or release action.

Completed current-source CI

Source c2d5ca6 now has successful check 120293772455 and gate 120300827479. Raw log:

Summary [ 520.207s] 14873 tests run: 14873 passed (1 slow, 2 leaky), 15 skipped

Actual checkout 2d508a6 and source share full tree 1a6a1c37a244d6b81ac49878e75e3668b708832d. All fourteen specifically checked stream, dispatch/recording, footer, fetch-envelope, storage and activation contracts have explicit PASS lines. Formatting, lint, workspace, documentation and lockfile steps succeeded. This is current-tree Linux execution, separate from the earlier targeted mutation validation; no customer-device, packaged-host, live-provider or release proof is inferred. Public maintenance hold and external review decisions remain unchanged.

@zhuowp
zhuowp requested a review from JensenChen28 October 23, 2026 07:24

@JensenChen28 JensenChen28 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.

Standards

0 findings. The incremental change strengthens existing recovery contracts and updates recovery guidance; it does not change artifact storage or authorization conditions. The obsolete wire-budget comments are corrected.

Spec

0 blocking findings. The gate-stream regression now keeps the gate summary short, independently takes stdout and stderr past their 800-character cut, and keeps the full input below the route budget. It directly requires the recovery flag and honest unavailable-output footer, complementing the existing complete-result negative case. All six dispatch cases now continue through actual result recording and inspect the next-step retrieval tool surface, including the no-recovery cases. The fallback guidance explains deferred activation and unavailable-tool alternatives.

Approved exact head c2d5ca61081ed0147dfd4a329f04ca31457424b4 against maintenance ea9fdc90f1c90335554833195eeeb847c83d82dd; incremental review from the previously reviewed 72e1d1b43e3bec3f209478e02752a78f646c3d9a. Both review axes and whitespace checks passed in the isolated checkout. Cargo is unavailable locally, so the new Rust tests and mutation controls have not been executed by this review. Earlier native receipts belong to their recorded revisions; current-head CI is separate. No maintenance or parent-gitlink update is performed by this approval.

@JensenChen28 JensenChen28 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.

已按当前 c2d5ca6 与最新 base 复核完整 slice 及相对72e1d1b43的五文件增量,并阅读本轮提到的 parent codewhale-hq#757 评审。

Standards:0新增问题;旧完整 JSON、不可变保存及受当前 catalog 限制的恢复 schema 激活实现未改,上一轮私下问题的修复保持。
Spec:0新增问题。两个 stream fixture 分别超过800字符截断点、整体仍低于路由预算,直接要求恢复;三种真实 dispatch 路径继续经过 process_tool_results,再检查下一步工具目录,大结果与完整小结果均有正反断言,原始字节回读/UI断言保留。子代理报告与旧 spillover 提示补充缺席工具的 tool_search 发现及不可用时回退,没有另建权限通路。

实际验证:本地源码/调用链及 git diff --check 通过,未运行 Cargo、原生权限、HTTP/provider、Windows 或包验证。独立读取 run39915840040:baseline/no-activation/no-stream-loss 三项 job metadata 成功;GitHub compare 确认验证提交6b95efa903仅比本head修改验证workflow。三项原始日志下载失败,未独立确认具体测试/反例失败计数,不借作者的十项或 mutation 计数。72e 的旧全量CI结果不移用到此head。当前源码完整CI仍在运行,未发现实质源码问题,现提交 APPROVED。

本审核保留其他审核者和维护者决定空间。未修改PR代码,未执行合并、入队或auto-merge。

@asto18089 asto18089 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.

Fresh re-review at head c2d5ca610 (base ea9fdc90f)

Verdict: APPROVED — 7-lane fresh review (core engine, truncate/UI, fetch/wire/stream, catalog/activation/tests, smuggling+upstream-fidelity, claims-vs-logs, local execution), re-derived from source on the current head. All four review dimensions pass; remaining items are follow-up-grade minors listed below.

Rebase status

No rebase needed: pinvou3-clean tip is ea9fdc90f1c and the compare API shows the head is exactly 11 commits ahead / 0 behind with merge-base = ea9fdc90f1c — the PR body's claimed base is the live tip. gh pr view reports MERGEABLE/CLEAN.

Criterion verdicts

  1. Useful, complete, root-caused — PASS. The prior-round MAJOR is genuinely closed: fetch_url now returns the complete valid envelope JSON (body-bound path deleted), the engine preserves those exact bytes before the single route-budget cut, and one-hop retrieval is byte-identical (pinned for oversized Unicode bodies, 404 error bodies, and the budget+1024 header case — the exact prior defect trigger; my save-half-the-artifact mutation went RED in the round-trip test). Parent codewhale-hq#757 findings are closed at this head: the gate-stream fixtures are now "stream detail "×100 (1,400 chars) per stream, independently above the 800-char cut and below the route budget — I re-ran the lossy-flag mutation live and forkguard_task_stream_summaries_are_inline_or_recoverable went RED at tests.rs:19732; the activation || art_-clause deletion went RED in the catalog test and in all three forkguard_turn_loop_{serial,parallel,envelope}_preserves_output_before_fanout next-step-surface assertions while the three complete-output controls stayed green; the subagent-report and legacy-spillover footers now include the tool_search discovery rung with honest unavailable-fallback text and no permission bypass.
  2. Elegant, no wheel reinvention — PASS. Upstream-first backport of the codewhale-hq#6508 slice (13-file fidelity table: 9 faithful, 4 adapted with disclosed fork-specific justification), and the fork fixes five real upstream defects — I verified each exists in the upstream patch and is fixed here: double footer in summary_view, missing non-lossy budget check, footer>budget overshoot, head_len+tail_len overflow (now checked_add, usize::MAX test), unbounded evidence-envelope pass-through. Net deletions: receipts compaction (−315), its renderer (−52); removal is complete (workspace-wide grep: zero remaining references); removal of the per-completion spawn_blocking is a perf win.
  3. No smuggled changes — PASS. All 23 files map to declared purposes hunk-by-hunk; zero changes outside crates/tui; no Cargo.toml/lock/workflow changes; commit messages match content (the 0600-in-web-titled-commit scope mismatch flagged last round is now explicitly disclosed in the body, which is acceptable given published history is preserved).
  4. Bug-free — PASS. Local cargo check --locked --offline: clean, 0 warnings. Targeted suites green: 235 forkguard_, 6 output_recovery, 16 fetch_url, 51 stream_decoder, 35 tool_catalog. cargo fmt --check clean. Full lib suite 12,250 passed / 0 failed. No surviving mutants: all three targeted mutations (lossy flag, activation clause, always-preserve) were caught by exactly the tests that pin them. CI on this tree: 14,873/0 (verified from the raw check log, and the checkout 2d508a6c77 shares tree 1a6a1c37a2 with the source head). The three validation-run mutant jobs in run 39915840040 behave exactly as the body describes, including "compilation failure does not count" (the workflow asserts exact per-test outcomes, so an empty match set fails).

Claims audit

Every verifiable body claim checked TRUE against primary evidence: 90 commits above upstream v0.9.12 (cross-repo compare), 88/2 DCO split (unsigned = #107, #116), 250 distinct forkguard_ names (tarball grep), 23 files +2383/−1041, validation/source commit delta confined to .github/workflows/fork-ci.yml, and the current-head CI receipts.

Findings (all follow-up grade, none blocking)

  1. [MINOR] Tiny-budget routes can clip the ref out of the footer. footer.chars().take(budget) at truncate.rs:768/:825 and context.rs:917 keeps the size/path prefix and drops both the ref="art_…" and the hint marker when budget < footer (~450 chars, i.e. windows under ~4k tokens). Disclosed in the body ("A tiny model budget can clip even recovery guidance") and the artifact/metadata survive call-id retrieval, so nothing is lost — but consider reserving a suffix for ref="<id>" or re-advertising after a clip. The pinning test asserts ≤ budget only, not ref survival.
  2. [MINOR] One unreproduced local flake sighting of the new engine tests. In one combined local cargo test run (1 of ~25), forkguard_structured_field_cuts_still_request_recovery + the gate-stream test failed on needs_full_output_artifact; 18 re-runs, my 5 full-suite runs, and CI (nextest, process-per-test) are all green. My stress at --test-threads 8 instead reproduced three pre-existing flakes of the same global-home/budget-interference class, all outside this PR's diff (tools::file::pdf_tests::contract_read_* ×2, session::export::last_copy_stays_inside_an_explicit_codewhale_home) and present at base. Suggest a stress-hardening pass (unique fixture ids or the shared guard) for the new tests in a follow-up.
  3. [MINOR] Upstream log_path lines not adopted and not itemized. Upstream's run_verifiers summary carries parsed-level/per-gate log_path lines; the fork omits them. The fixed (vs budget-scaled) detail caps are disclosed, this specific divergence is not — one body sentence or a follow-up note would close it.
  4. [NITs] Adaptive-evidence path uses get_or_insert_with(json!({})) so non-object pre-existing metadata loses stamps (self-heals via re-preserve; classic path's metadata_object_mut is the better pattern); request_builder_truncates_large_tool_result_for_wire reads the workshop slot without active_workshop_test_guard (latent, sibling test has it); preserve failure emits no audit event (spillover failures do); the envelope byte oracle shares the production serializer so a serde-level shape change would pass; fetch_url's test module is still named pdf_tests; carried prior-round minors (read-time digest check, artifact-dir quota signal, transcript at-rest doubling, bytes/chars docstring, full producer-cap list) remain deferred under codewhale-hq#769–codewhale-hq#772.

Coverage

Static: all 23 files hunk-by-hunk plus full load-bearing functions; end-to-end traces of the fetch envelope, the three dispatch arms, activation, and retrieval. Execution: local offline compile, five targeted suites, three mutations, full suite. Evidence: PR-body claims checked against raw CI logs. I did not exercise live providers, Windows ACL behavior, or packaged-host flows — matching the body's own boundaries.

Signed-off-by: zhuowp <9426526+zhuowp@users.noreply.github.com>

@JensenChen28 JensenChen28 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.

Standards

1 finding — [P2] Isolate the new tests from the process-global workshop budget (crates/tui/src/core/engine/tests.rs:19435, also the new tests around 19423, 19450, 19576, and 19796). CONTRIBUTING.md retains cargo test --workspace --all-features --locked as an authoritative supported gate. These tests currently have an introduced shared-state race under that command's parallel libtest execution.

Spec

1 finding (the same underlying test-correctness defect). The approximately 44,000-character fixture expects recovery under the default 15,360-character route budget. In the same binary, runtime_api/tests.rs:150–207 temporarily installs global budgets of 65,536 and 77,824 under active_workshop_test_guard. The unguarded recovery test can read that temporary value, fully inline the result and fail its needs_full_output_artifact assertion. The 68,000-character and scaling fixtures have analogous threshold-sensitive assertions. with_artifact_home holds only the separate spillover lock, so it does not serialize these reads with the budget writer.

Please acquire the same workshop test guard in each affected budget-sensitive test and explicitly establish/restore its expected configuration, following the already-guarded sibling tests. Avoid unconditionally adding a non-reentrant guard inside a helper whose callers may already hold it.

Request changes on exact head c2d5ca61081ed0147dfd4a329f04ca31457424b4; new isolation evidence changes my previous approval. This conclusion is based on the actual shared-state writer, reader, locks and threshold arithmetic, not an executed native reproduction. Current nextest CI isolates tests into separate processes, so its verified 14,873-pass receipt remains valid and is not evidence of this race occurring in CI. No new product-runtime defect is asserted.

@zhuowp

zhuowp commented Oct 23, 2026 •

Copy link
Copy Markdown
Collaborator Author

Budget isolation review follow-up at ac8b7e7

@asto18089 @JensenChen28 please re-review the current pinned revision. The latest D1/D2 and related m1–m8 dispositions, exact validation inputs, native outcomes and boundaries are in the rewritten PR body.

Actual normal validation: five selected native runner tests pass. The concurrency harness calls five real reader test functions during a controlled production workshop install; they wait and pass. Removing their guards makes all five underlying assertions fail; reverting summary loss or corrupting report bytes each fails its target contract. Native validation 39936221645 checks exact identities, summaries, exits and reader outcomes, and its raw logs were read independently. The workflow-injected harness is absent from the source PR.

Current source-head complete CI is recorded below. Parent public alignment remains blocked; earlier c2 CI is historical. Parent fast/architecture and 65 guard-policy tests passed, prior predicates are preserved, and protected public alignment remains unchanged. No merge/tag/release or protected-maintenance action.

Completed current-source CI

Source ac8b7e7 now has successful check 120361648893 and gate 120367722209. Raw log:

Summary [ 417.236s] 14875 tests run: 14875 passed (1 slow, 2 leaky), 15 skipped

Actual checkout 327009b and source share full tree 18b435a81d13d3fac7c9500806bbbf560909f4ba. All twenty-three specifically checked budget readers, summary loss, child reports, wire, handle, stream, dispatch/recording, footer, fetch-envelope, storage and activation contracts have explicit PASS lines. Formatting, lint, workspace, documentation and lockfile steps succeeded. This is current-tree Linux execution, separate from the earlier targeted mutation validation; no customer-device, packaged-host, live-provider or release proof is inferred. Public maintenance hold and external review decisions remain unchanged.

The guarded runtime budget writer restores defaults before releasing its guard (runtime_api/tests.rs); these repaired contracts are readers and do not install settings. The guard is acquired at each test boundary, not inside the non-reentrant artifact-home helper.

@JensenChen28 JensenChen28 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.

已固定当前 ac8b7e7 复核完整组合范围及相对c2d5ca61的七文件增量。

Standards:0新增阻断。预算 reader 的新 guard 只影响测试生命周期,锁序为 workshop 后 fixture,未见新用法递归取得同一非重入锁;真实配置写者也使用同一门并在退出前清空安装态。原私密创建、不可变发布与策略筛后恢复 schema 路径未改。
Spec:0新增问题。run_tests 的 metadata.summary 截断现在标记 loss;真实 producer 的 summary 从同一 content 的 command/stdout/stderr 派生,因此保存原始 content 可恢复诊断依据,但不把此实现称为任意 metadata 字节的保存。新子报告回归调用真实 producer 核报告字节/身份/失败无ref;发现提示与历史配置单位澄清未改变权限或配置语义。

实际验证:本地仅精确Git源码/锁序/调用链及 git diff --check(通过),未运行Cargo/native、原生并发/变异、HTTP/provider或Windows测试。独立读取验证run39936221645四项job metadata均成功,API compare确认验证提交42aa8a2e仅比源码修改workflow;该workflow运行时另注入并发harness,不能把它等同源码head完整CI。未独立读取本轮原始日志,不引用作者五项/reader/反例计数;旧c2全量CI也不转移到此head。当前源码完整CI仍单独跟踪,无未解决实质源码问题,提交APPROVED。

此决定保留其他审核者及维护者的审核空间;未修改PR代码,未合并、入队或开启auto-merge。

Signed-off-by: zhuowp <9426526+zhuowp@users.noreply.github.com>
@zhuowp zhuowp changed the title fix(context): recover complete tool outputs fix(context): recover returned tool results Oct 23, 2026
@zhuowp

zhuowp commented Oct 23, 2026 •

Copy link
Copy Markdown
Collaborator Author

Preserve engine-returned tool results before route-specific context compaction, including the serial, parallel and aggregate dispatch paths, while retaining artifact failure, immutable publication and policy-filtered retrieval boundaries. The candidate also carries complete Web envelope preservation, workshop-budget test isolation, metadata-summary loss tracking and subagent-report byte contracts. Parent adoption and producer-scope register: Pinvou/pinvou-agent#757.

The latest review's M1 is addressed by declaring the actual producer boundary: tasks.rs caps gate stdout/stderr summaries at 900 characters before the returned DTO; without an active task, log_path is null and pre-DTO command tails are not recoverable. The model view now says this explicitly and the engine recovery footer names the complete returned tool result. The short-sentinel contract directly requires both streams. A real TasksTool long-stream regression checks 12,000 characters per stream, the actual 900-character DTO and no log_path, then uses synthetic post-preservation metadata solely to verify the rendering contract. It does not claim storage/retrieval of the original command log.

M2 is addressed in both register languages: evidence_available results pass unchanged only when they fit the current route; larger previews are refitted using the existing reference, unlike upstream 3e39820's unconditional pass-through. Keep this deliberate current-route bound. Ref-bearing recovery footers now teach tool_search activation and a narrower rerun when unavailable; the garbled route-budget comment is fixed. Maintenance drift is explicitly attributed to public ea9fdc9.

Native validation run 40000699762, validation commit b4eb108: the baseline executes six named source tests, each with an explicit one-pass/nonzero-selection summary. Three isolated variants restore the old footer, remove the producer-boundary note or remove discovery guidance; each causes exactly the new real-tool regression to fail, with the other five still passing. Raw logs and actual checkout SHAs were independently inspected. The validation commit differs from source 2a0221f only in fork-ci.yml; the baseline injects no Rust harness. This focused evidence is separate from full current-head CI. Prior ac8 CI (14,875 passes, 15 skips, tree 18b435a81d13d3fac7c9500806bbbf560909f4ba) remains historical and is not transferred to this changed source.

Self-review found no remaining blocking defect within the declared engine-returned-result slice. Existing producer-cap removal, tiny-budget usability, retrieval expiry/retention, composer-bang preservation and size/metric follow-ups remain explicitly tracked in Pinvou/pinvou-agent#770, Pinvou/pinvou-agent#771 and Pinvou/pinvou-agent#772; instruction enforcement remains in Pinvou/pinvou-agent#768. These deferred boundaries are not claimed fixed. No local Cargo pass is claimed (offline cache lacks ascii 1.1.0); formatting passed. No native Windows, packaged-host, live-provider or release evidence is added by the Linux validation.

The user's separate hold on protected pinvou3-clean remains in force. Public maintenance remains ea9fdc9; no protected reference, tag, merge, queue, auto-merge or release operation is requested or performed. Formal changes-requested reviews remain owned by their reviewers.

Current head: 2a0221f22705c04d6cdac6db29b62b56d4d6738e. The new repair is appended by a normal fast-forward commit; the remote maintainer history is retained.

Please re-review the exact current head; this author self-review does not dismiss existing reviews.

Current CI snapshot

Exact head 2a0221f22705c04d6cdac6db29b62b56d4d6738e, observed 2026-10-23T18:22:21.935Z. The issue-link check now passes at job 120581682906 after restoring the original required No-Issue line omitted during the body refresh. The latest check supersedes the earlier failed link attempts. Complete current-source CI remains pending. 2 checks still pending. Formal re-review and applicable final combined-tree gates remain separate; no merge/release approval is implied.

@zhuowp
zhuowp requested a review from JensenChen28 October 23, 2026 18:18

@JensenChen28 JensenChen28 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.

Standards

No new mandatory source finding. The taskless view now states the inherited 900-character producer cap/no-log boundary, the recovery footer accurately names the complete returned tool result, and reference-bearing recovery guidance includes discovery and an unavailable-route fallback. Budget-test isolation from the prior revision is retained.

Spec

The real TasksTool regression generates 12,000 characters on each stream and checks the actual capped DTO with no log_path. Its later synthetic artifact metadata is explicitly limited to the rendering contract; it does not claim real artifact storage/retrieval or recovery of pre-DTO command tails. The short-sentinel test now directly requires both streams. No new correctness gap found within the engine-returned-result scope.

Independently verified validation run 40000699762: actual checkout b4eb108 is one workflow-only commit ahead of this source. Six selected baseline invocations each select one passing test. Three isolated variants each fail the intended new taskless contract while the other five pass; their green jobs assert expected failure/exit101. This is selected-contract evidence, not a full source-suite pass. Local Cargo is unavailable; whitespace check passes.

The observed failed link jobs are the PR issue-linkage/body-policy check, not native linker failures; this approval does not waive them or claim all current checks green. Approve source at exact 2a0221f22705c04d6cdac6db29b62b56d4d6738e; current CI and repository policy remain separate.

@JensenChen28 JensenChen28 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.

Standards

0新增阻断。固定当前 2a0221f22705c04d6cdac6db29b62b56d4d6738e、最新 base ea9fdc90f1c90335554833195eeeb847c83d82dd,沿用此前 ac8 完整组合范围审核,并检查本轮一个提交、四文件66+/5−增量及相关未变消费者。新增生产差异仅澄清恢复提示/注释,原不可变产物保存、策略筛选、派发及预算实现未改。双流短测试由可接受fallback改为同时要求两个sentinel,没有弱化断言。

Spec

0新增阻断。M1现在准确区分900字符的producer返回摘要、无active task时缺少命令日志,以及实际保存的完整returned DTO;不再暗示被丢弃的原命令尾部可恢复。新增Unix回归确实调用TasksTool生成12000字符双流并验证真实DTO;后附artifact metadata明确仅模拟render契约,不冒充storage/retrieval验证。M2当前路由内通过、超预算复用原ref重新fit的行为保持。tool_search发现及不可用时缩小重跑的提示没有新增授权通路。已明确延期的producer上限、极小预算、留存与其他跟进不据此宣称解决。

实际验证

本地固定Git对象和调用链检查、增量及完整目标 git diff --check 通过。审核checkout出现未知并发tracked改动,已保全且排除其内容,所有最终源码结论均使用固定提交对象;未运行本地Cargo、原生并发、Windows、provider或包验收。

独立读取验证 run40000699762 的四项原始job日志及实际checkout SHA:baseline六个具名测试各执行1项并通过;old-footer/no-producer-note/no-discovery三个隔离对照各使新增回归测试失败,其余五项通过。固定Git比较确认验证提交 b4eb108335a99556429b11191b9a2dabadbd23c8 与本源码head仅验证workflow不同;baseline未注入Rust harness。因此认可这组有限的远程定向证据,不把它等同当前head完整CI,也不移用ac8旧全套计数。当前源码CI单独跟踪;此前No-Issue正文门禁已有同head新的成功记录,不把旧失败记录误判为仍未修复的源码问题。

两轴均无未解决实质问题,现对准确head提交 APPROVED,不因其余CI仍运行而只留COMMENT。保留其他审核者与维护者的决定空间;未修改PR代码,未执行合并、入队或auto-merge。

Standards新增问题0;Spec新增问题0。

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.

3 participants