fix(runtime): bound malformed compaction retries - #3751
Conversation
435b957 to
480f9cf
Compare
xxhZs
left a comment
There was a problem hiding this comment.
Reviewed exact head 480f9cfaa9933f5d410a3589a8eae59ea9ad58cb.
The core liveness fix is sound: strict checkpoint validation remains intact, malformed summaries get one bounded repair attempt, the Turn latch prevents repeated calls in the same Turn, and the Session-backend fingerprint circuit stops unchanged bad input from being retried across Turns. The granular failure reason is also preserved through Runtime Event → Runtime Host failed-turn state → Desktop, with the compatibility epoch advanced to 49. Exact-head test is green. I found no P1 correctness issue.
I do see two contract gaps worth resolving before approval:
- [P2] Recovery behavior is only closed on Desktop. CLI/TUI still consume the generic
context_budget_exhaustedclass and do not usecontextBudgetExhaustedDetail. If #3743 is explicitly Desktop-only, please record CLI/TUI as follow-up scope; otherwise the cross-surface recovery contract is incomplete. - [P2] Fingerprint invalidation is not regression-locked. The implementation is intended to re-enable compaction when model, connection, context-window budget, or request shape changes, but tests only prove source-history invalidation. Please add a parameterized regression covering unchanged input plus at least model, connection, and context-window changes; this fingerprint is the core cross-Turn safety/liveness boundary.
Non-blocking follow-ups:
- [P3] Repair coverage only exercises
malformed_summary_missing_section;truncatedandtoo_small_for_foldshare the path but have no second-call regression, and the repair prompt is not reason-specific. - [P3] Desktop recovery is actionable copy rather than an actionable control:
deriveFailedTurnRecovery()returns an action, but the turn view model retains only the label, so there is no direct settings/model/new-task affordance.
Recommendation: this can proceed to maintainer review as the Desktop liveness fix, but I would clarify the CLI/TUI scope and add the configuration-change fingerprint tests before approval.
AI-assisted review disclosure: OpenAI Codex inspected the exact-head diff, protocol/runtime/host/Desktop flow, focused tests, and live CI state; I verified the findings against the source before posting.
简体中文
核心限流与熔断方向正确,没有发现 P1;当前主要缺口是 CLI/TUI 未消费细分失败原因,以及跨 Turn 指纹对 model/connection/context-window 变化的契约缺少回归测试。另有 repair 场景覆盖和 Desktop 一键恢复能力两个非阻断缺口。
480f9cf to
cbe797d
Compare
Review follow-upAddressed the fingerprint-invalidation P2 in The new table-driven regression verifies the complete cross-Turn circuit contract:
The existing source-history invalidation regression remains in place. Scope clarification for the other P2: #3743 and this PR are explicitly scoped to Desktop + Runtime Host. CLI/TUI currently retain the generic The branch was rebased onto This is ready for maintainer review after CI completes. AI assistance disclosure: Codex implemented the review follow-up, ran the verification, and drafted this response; I verified the result before posting. 简体中文已在 范围方面,#3743 和本 PR 明确限定为 Desktop + Runtime Host。CLI/TUI 目前仍展示通用的 分支已 rebase 到 CI 通过后即可请求 maintainer review。 |
Astro-Han
left a comment
There was a problem hiding this comment.
I reviewed this head and found a blocking issue.
[P2] Repair failure overwrites the initial malformed cause and retries indefinitely
Once initial.defect is malformed_summary_*, the repair budget is spent, but generateSummary(1) can itself throw output_length or be wrapped as provider_error. That final reason replaces the malformed fact, so neither the session-backend fingerprint circuit nor the Turn latch blocks the next identical compactHistory — leading to 4 physical calls across two compactions instead of 2.
Fix: arm the circuit with the original malformed reason whenever repair is entered and does not produce a checkpoint.
Checks on cbe797d811 are test: success.
简体中文
存在修复失败覆盖原始缺陷导致的重试环。
Review follow-upFixed the blocking repair-failure P2 in Once repair is entered, every path that fails to produce a valid checkpoint now reports the original malformed reason to the caller. This covers repair The end-to-end regression reproduced the review's exact failure on the prior head: two identical Verification on
AI assistance disclosure: Codex reproduced the finding, implemented the fix and regression coverage, ran verification, and drafted this response; I verified the result before posting. 简体中文已在 一旦进入 repair,只要最终没有生成有效 checkpoint,对外都会保留第一次的 malformed 原因;repair 阶段的 端到端回归在旧 head 上复现了 review 描述的问题:相同输入连续两次
|
3e9a3d3 to
943d344
Compare
There was a problem hiding this comment.
I reviewed this head and found no code blocking issues, but the change cannot be merged as is.
[P2] RUNTIME_HOST_COMPATIBILITY_EPOCH collision and merge conflict
Head 943d344af sets RUNTIME_HOST_COMPATIBILITY_EPOCH = 50 for compaction detail, but main already uses 50 for WorkHub coordination summaries (merged #3798). The two 50s have different meanings, so incompatible peers would accept or reject the wrong fields. Also packages/runtime-host/src/protocol/index.ts is CONFLICTING.
Fix: rebase onto current main and bump epoch to 51 (strictly greater than main's 50), keeping the comment load-bearing.
Hosted test: SUCCESS (32858535454) on code.
简体中文
epoch 与 main 上的 50 语义不同,需要重排并升到 51。Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
943d344 to
4c37dc5
Compare
b4e424c to
c72e047
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
The problem statement in #3743 is the right one: the validator is correct to refuse an incomplete summary, and the defect is that the same Session had no bounded repair and no escape. This fixes both at the level where they belong.
Three things I checked rather than took on faith:
The circuit really is Session-scoped. It lives on the AiSdkCompaction instance, and backends are cached per Session in runtime-kernel.ts:2686 and rebuilt only after disposal, so it does survive across Turns as the summary claims.
The repair refits its input budget. repairSummarizationSystemPrompt is longer than the original, and the refit passes repairInstructions.length as fixedInputChars rather than reusing the first fit. That is exactly the detail that would otherwise push the repair attempt over budget and turn a repairable defect into output_length.
The cost profile improves. A first malformed input now costs two provider calls instead of one, but every later dispatch of the same input costs zero. The incident spent eight.
Approving. One P3 inline and three notes below, none of them blocking — though the epoch collision has to be resolved before this can merge.
Epoch. The branch declares 51 and main reached 51 when #3818 merged a short while ago, so mergeable is already CONFLICTING. That is your third renumber (48 → 49 → 51); it is the merge-result guard doing its job rather than a fault in the PR. Take main's 51 as-is and move the context-budget note to 52.
The circuit is residency-scoped, not durable. A Host restart or backend disposal re-arms one more doomed dispatch plus its repair. That is a sound bound — the summary just reads more durable than it is.
The granular detail reaches one of three presentation sites. FailedTurnRecoveryInput carries only errorClass, so deriveFailedTurnRecovery cannot see contextBudgetExhaustedDetail and falls back to the coarse context_budget_exhausted label. Nothing is lost today, since both strings give the same advice — but the recovery affordance is the one place that extra precision was meant for.
AI use: Claude Code assisted with source investigation; the analysis and conclusions are my own.
简体中文
#3743 的问题定义是对的:校验器拒绝不完整的摘要没有错,缺陷在于同一个 Session 既没有有界修复也没有逃生路径。这个 PR 在正确的层次上同时解决了两者。
有三处我是实际核过而不是采信的:
熔断器确实是 Session 级的。 它挂在 AiSdkCompaction 实例上,而后端在 runtime-kernel.ts:2686 按 Session 缓存、只在释放后重建,所以它确实能跨 Turn 存活,与摘要的说法一致。
修复重试重新计算了输入预算。 repairSummarizationSystemPrompt 比原始 prompt 更长,重新 fit 时传的是 repairInstructions.length 而不是复用第一次的结果。正是这个细节决定了修复尝试会不会超预算、把一个本可修复的缺陷变成 output_length。
成本是改善的。 首次遇到非法输入从 1 次调用变成 2 次,但之后同一输入的每次派发都是 0 次。事故里花了 8 次。
Approve。行内一条 P3,下面三条说明,都不阻塞——但 epoch 冲突必须先解决才能合并。
Epoch。 分支声明 51,而 main 在不久前 #3818 合入时也到了 51,所以 mergeable 已经是 CONFLICTING。这是你第三次改号(48 → 49 → 51);这是 merge-result 守卫在起作用,不是 PR 的问题。原样保留 main 的 51,把 context-budget 那条说明挪到 52。
熔断器是驻留期作用域,不是持久化的。 Host 重启或后端释放会重新放行一次注定失败的派发及其修复。这个界限是合理的,只是摘要读起来比实际更持久。
细粒度信息只到达了三个展示点中的一个。 FailedTurnRecoveryInput 只带 errorClass,所以 deriveFailedTurnRecovery 看不到 contextBudgetExhaustedDetail,只能回落到粗粒度的 context_budget_exhausted 文案。今天没有实际损失,两段文案给的建议一样——但恢复操作提示恰恰是这份额外精度最该用上的地方。
e45dda2 to
a3be6b1
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Stops a Session from re-sending a compaction that can never succeed (#3743 saw eight in one Turn): one stricter repair, a per-Turn latch, a Session-scoped fingerprint circuit, and malformed_summary_* reasons carried to Desktop.
Correctly defined, and it refuses to weaken the #3029 validator — right call: the validator is a safety property, the spin is the bug. 589 of 859 lines are tests. Re-reviewing at a3be6b159; the only drift since my earlier approval is removing the duplicate validator call I flagged.
P2 — a user pressing Stop during repair arms the circuit and permanently fails compaction for that Session, with zero provider calls. The repair catch in history-compact-summarizer.ts rethrows anything that isn't a HistoryCompactSummarizerError — including AbortError — as initial.defect, and ai-sdk-compaction.ts:471-487 arms the Session circuit unconditionally. A cancelled Turn appends nothing, so the retry fingerprint is identical and the circuit hits every time. Treat abort as cancellation, not defect.
P2 — epoch 54 collides with #3390 and #3935 against main = 53. scripts/protocol-epoch-check.mjs only compares head against origin/main, so it structurally can't see sibling branches. First to land keeps 54.
The remaining non-minimality is representational: the same detail enum is hand-listed in five places.
AI-assisted review: a Claude Code subagent analysed the change; I verified the abort path and the epoch collision against main myself. No tests or typecheck run. AI review is not independent human review.
a3be6b1 to
106ff0f
Compare
|
Fixed on exact head
Verification:
简体中文已在 分支已 rebase 到当前 本地构建、聚焦回归、Runtime 全套、Runtime Host 协议测试、Biome、diff check 和 epoch guard 均通过;GitHub Windows recovery 已通过,主 test 仍处于 queued,尚未失败。 |
|
This is a synthesis of the independent blind review by @Luna-Deep-Qronos at exact head What I checked myself:
Findings from Luna-Deep's review (file:line anchored): Overall — CODE NO-GO — 1×P2 (worst P2).
What was proven green: summarizer 9/9 PASS, backend 24/24 PASS, Runtime Host protocol/session-projector 27/27 PASS; Core build and What I did not judge: a true 56 KiB-boundary end-to-end Host restart with the malformed detail was not executed beyond the byte-size reproduction — verification was by code inspection and the targeted suites noted above. Gate: one P2 open (
|
Preserve granular context-budget failure details through Runtime Host and show actionable Desktop recovery guidance. Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Keep AbortError out of the malformed-summary circuit, move the context budget detail contract to one Core authority, and advance the Runtime Host compatibility epoch past concurrent protocol changes.\n\nGenerated-by: Codex
106ff0f to
d06739d
Compare
Summary
Fixes #3743
Verification
npm run buildnpm run lintnpm run format:checknpm run typechecknpx knip --workspace apps/desktopnpx knip --workspace packages/uinode scripts/protocol-epoch-check.mjs --base origin/main --head HEAD(epoch 48 -> 49)npm testpassed the affected Runtime (3,052 tests) and Desktop (1,497 tests) workspaces, but the concurrent root run exited nonzero on three unrelated timing/process tests. Each passed when rerun in isolation:AI use
Select exactly one:
Tool(s) and scope: Codex inspected the incident evidence and repository, implemented the Runtime/Runtime Host/Desktop changes, added tests, and drafted this PR description. The commit includes a
Generated-by: Codextrailer.Checklist
Does this PR entail a change in behavior?