fix(runtime): reject truncated history-compact summaries at load time (#3041) - #3046
fix(runtime): reject truncated history-compact summaries at load time (#3041)#3046me2seeks wants to merge 6 commits into
Conversation
… history summarizer (apache#3030) replayPlanItemsToModelMessages mapped every tool_call replay item to its own assistant message. Strict OpenAI-compatible providers reject that shape (DeepSeek: 400, "an assistant message with 'tool_calls' must be followed by tool messages responding to each 'tool_call_id'"), so the history summarizer failed with provider_error on any session whose folded history contains parallel tool calls — the 3 consecutive provider_error fail-opens in the apache#3029 incident ledger match this failure mode. The primary replay materializer already merges a step's tool calls into one provider message (model-history.ts); mirror that invariant here by folding consecutive tool_call items into a single assistant message. Verified live against DeepSeek with the incident session's real events: the request that previously 400'd is now accepted. Regression tests: parallel calls produce one assistant message with N tool-call parts followed by N tool messages; calls separated by other content stay unmerged.
… gates (apache#3029) Incident: a degraded summarizer response — a 138-token section-less fragment ending mid-sentence, produced right after 3 provider_error retries — was persisted as checkpoint hcheckpoint-981ceab8…, replacing 742 folded events (~235k estimated tokens). The continuation model then hallucinated implementation details the summary had lost. The only rejection gate was empty_summary; any non-empty string replaced the folded history. Add validateHistoryCompactSummary (history-compact-summary-validation.ts): - required section headers (## Goal / ## Progress / ## Next Steps — the load-bearing subset of the prompt contract), and - truncation detection (output ending on colon/comma/paren/backtick/em-dash, or stopping inside an unclosed code fence). Enforce it at both checkpoint write gates — writeHistoryCompactCheckpoint and planMidTurnCapacityCompaction — so every summarizer implementation, not just the LLM one, passes the same bar. Rejections fail open with the new malformed_summary reason: history is kept and compaction retries next turn. The section names are the single source of truth: the summarization prompt is now built from HISTORY_COMPACT_SUMMARY_SECTIONS, so the requested contract and the enforced contract cannot drift apart. Tests: - validation unit matrix (incident fragment verbatim, single word, raw DSML tool markup from a live weak-model reproduction, missing sections, unclosed fence, truncation punctuation), - mid-turn fail-open with diagnosticReason malformed_summary, - backend end-to-end: fragment summary records no checkpoint, the turn completes on the raw projection, diagnostics carry failOpenReason malformed_summary, - existing stub summaries updated to the conforming shape.
…int write gate (apache#3029) Review follow-up (first-principles + Occam): section headings are now matched line-anchored instead of via substring includes, so prose mentions or near-matches like `## Goals` no longer satisfy the contract; the required-section list is pinned to the full prompt contract at compile time. Adds a direct regression test for the writeHistoryCompactCheckpoint gate (previously only the mid-turn gate was exercised) and trims redundant tests.
…erseded by apache#3038) PR apache#3038 (fix/summarizer-parallel-tool-calls) fixes apache#3030 with a stronger step-based grouping that also handles interleaved results (call A, call B, result A, call C, result B, result C). Our adjacency-based merge missed that case, so drop the apache#3030 portion here and keep this PR focused on apache#3029 (write-gate summary validation) only.
…es (maka-agent#3041 review) The shared truncation heuristic counted a trailing backtick as truncation, but an even fence count already proves a fenced code block closed. Applying that tail check at load time therefore rejected complete legacy summaries that happened to end with a code fence, discarding their compaction benefit and forcing a re-compaction. Drop the backtick from the tail class (fence count still catches unclosed blocks), add the ellipsis signal, and rename the predicate to isHistoryCompactSummaryTruncated to match its boolean result.
875f7b0 to
621f1d5
Compare
📝 WalkthroughProblem solvedMalformed or truncated history-compact summaries could load during replay and replace valid folded history. This PR rejects them during both checkpoint load paths. Rejected checkpoints fall back to the previous valid checkpoint or raw events. Invalid bounded projections are repaired with Source of truthThe PR extends the existing checkpoint and canonical ledger paths. It does not create a parallel recovery path.
Scope and complexityThe solution is the smallest coherent change. It adds one shared validation module and calls it from the write path and both load paths. The load predicate checks fence counts, terminal punctuation, and ellipses. It does not require section headings, so legacy sectionless checkpoints remain valid. Closed code fences are accepted. Simplification opportunitiesNo code or tests can be removed without weakening behavior or regression coverage. Fixture updates convert test summaries to the structured format required by validation. Shared fixtures could reduce repetition, but the current tests clearly cover write-time fail-open behavior, load-time fallback, projection repair, legacy compatibility, and the closed-fence regression. Risks and validationThe main risk is rejecting valid summaries or legacy checkpoints. Tests cover valid punctuation, closed fences, sectionless legacy summaries, and truncated fragments. The test suite also covers WalkthroughHistory-compaction summaries now use a shared structured format. Write paths reject malformed output with fail-open behavior. Load paths reject truncated checkpoints and repair poisoned projections from valid ledger data. ChangesHistory Compaction Validation
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: 🟡 Moderate · up to The change can still accept and persist incomplete history summaries ending in Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Summarizer
participant SummaryValidator
participant Compaction
participant CheckpointLedger
Summarizer->>Compaction: produce structured summary
Compaction->>SummaryValidator: validate summary
SummaryValidator-->>Compaction: valid or malformed_summary
Compaction->>CheckpointLedger: persist valid checkpoint
CheckpointLedger-->>Compaction: reject truncated checkpoint during load
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: afd004ef-e2d1-424b-a738-d859b940c8aa
📒 Files selected for processing (13)
packages/runtime/src/__tests__/ai-sdk-backend.test.tspackages/runtime/src/__tests__/history-compact-checkpoint.test.tspackages/runtime/src/__tests__/history-compact-summary-validation.test.tspackages/runtime/src/__tests__/mid-turn-capacity-backend.test.tspackages/runtime/src/__tests__/mid-turn-capacity-compact.test.tspackages/runtime/src/__tests__/overflow-reactive-recovery.test.tspackages/runtime/src/__tests__/session-manager.test.tspackages/runtime/src/ai-sdk-compaction.tspackages/runtime/src/history-compact-error.tspackages/runtime/src/history-compact-ledger.tspackages/runtime/src/history-compact-summarizer.tspackages/runtime/src/history-compact-summary-validation.tspackages/runtime/src/mid-turn-capacity-compact.ts
| const REQUIRED_SECTION_HEADING_PATTERNS = HISTORY_COMPACT_REQUIRED_SECTIONS.map( | ||
| (section) => new RegExp(`^${escapeRegExp(section)}\\b`, 'm'), | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Require an exact required-section heading.
\\b accepts headings such as ## Goal: and ## Goal - details. These headings do not match the exact shared section contract. The write gate can persist summaries that the prompt did not request.
Use an end-of-line matcher instead.
Proposed fix
- (section) => new RegExp(`^${escapeRegExp(section)}\\b`, 'm'),
+ (section) => new RegExp(`^${escapeRegExp(section)}\\r?$`, 'm'),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const REQUIRED_SECTION_HEADING_PATTERNS = HISTORY_COMPACT_REQUIRED_SECTIONS.map( | |
| (section) => new RegExp(`^${escapeRegExp(section)}\\b`, 'm'), | |
| ); | |
| const REQUIRED_SECTION_HEADING_PATTERNS = HISTORY_COMPACT_REQUIRED_SECTIONS.map( | |
| (section) => new RegExp(`^${escapeRegExp(section)}\r?$`, 'm'), | |
| ); |
🧰 Tools
🪛 ast-grep (0.45.1)
[warning] 43-43: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^${escapeRegExp(section)}\\b, 'm')
Note: [CWE-1333] Inefficient Regular Expression Complexity
(regexp-from-variable)
| * here — it would also match a closed ``` fence, which is completion, not | ||
| * truncation. | ||
| */ | ||
| const TRUNCATED_TAIL_PATTERN = /[:：,，、;；…(（—]\s*$/u; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Detect ASCII ellipses as truncation.
The pattern detects … but not .... A sectioned summary ending in ... passes the write gate and remains loadable, although it has the truncation signal described by the PR objective. Add ... to this pattern and add it to the regression cases in packages/runtime/src/__tests__/history-compact-summary-validation.test.ts line 104.
Proposed fix
-const TRUNCATED_TAIL_PATTERN = /[:：,，、;；…(（—]\s*$/u;
+const TRUNCATED_TAIL_PATTERN = /(?:\.\.\.|[:：,，、;；…(（—])\s*$/u;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const TRUNCATED_TAIL_PATTERN = /[:：,，、;；…(（—]\s*$/u; | |
| const TRUNCATED_TAIL_PATTERN = /(?:\.\.\.|[:：,，、;；…(（—])\s*$/u; |
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for extending malformed-summary protection to restart/load and for fixing the false positive on a properly closed fence. I reviewed this head with two independent @reviewer passes plus a read-only ollama-cloud/deepseek-v4-flash:high pass.
The problem is correctly defined: a checkpoint that replaces RuntimeEvents must be safe both when written and when selected after restart. The current implementation still has two bypasses in the shared predicate: heading presence is not semantic structure, and the legacy fence detector recognizes only unindented backtick fences. That means the new load quarantine can still accept a title-only/fenced template or an unfinished tilde/indented block.
The first-principles simplification is one line scanner with two explicit policies: strict admission for new text checkpoints (ordered, non-empty required sections outside fences) and conservative truncation quarantine for legacy text checkpoints (backtick/tilde family, indentation, delimiter width). Record and load/replay should reuse it. When rebasing to current main, guard this behind the text-checkpoint discriminator so provider-native checkpoints keep their own schema/continuation authority. #3039 already contains the stronger structural scanner; the optimal result is to consolidate its scanner with this PR's restart quarantine, not land parallel validators.
Current CI is green. No local test suite was run during this review; conclusions are based on source, test, current-main, and CI inspection. Codex coordinated the independent passes and performed the final adjudication; external-model output was treated as unverified until checked against the code.
中文摘要
感谢把 malformed-summary 保护扩展到 restart/load，并修复合法闭合 fence 的误报。问题定义正确：替换 RuntimeEvents 的 checkpoint 在写入时和 restart 后被选择时都必须安全。
当前 shared predicate 仍有两个 bypass：heading presence 不是 semantic structure；legacy fence detector 只识别无缩进的 backtick fence。因此 title-only/fenced template，以及未闭合的 tilde/缩进 fence 仍可能通过新的 load quarantine。
最小方案是一个 line scanner、两套明确 policy：新 text checkpoint 严格要求 fence 外、按序、非空 required sections；legacy text checkpoint 保守识别 backtick/tilde、缩进和 delimiter width。record 与 load/replay 复用。rebase 到 current main 时必须先按 text/provider-native 分流。#3039 已有更强结构 scanner，最优解是与本 PR 的 restart quarantine 合并，而不是并行落两套 validator。
当前 CI 全绿。本次未运行本地测试套件；结论来自源码、测试、current-main 与 CI 检查。外部模型输出在核对代码前均视为未验证输入。
| export function validateHistoryCompactSummary( | ||
| summary: string, | ||
| ): HistoryCompactSummaryRejection | undefined { | ||
| if (REQUIRED_SECTION_HEADING_PATTERNS.some((pattern) => !pattern.test(summary))) { |
There was a problem hiding this comment.
P1 — Validate section structure and content, not independent heading presence. This accepts a title-only skeleton, required headings out of order, and all headings inside a completed code fence; each can then replace the folded RuntimeEvents with effectively no continuation facts. Please replace the independent regexes with one fence-aware scanner that requires the sections in order, outside fences, with non-placeholder content, and assert both writer paths do not record heading-only/fenced-template summaries.
| */ | ||
| export function isHistoryCompactSummaryTruncated(summary: string): boolean { | ||
| // An odd number of fences means the output stops inside a code block. | ||
| const fenceCount = (summary.match(/^```/gm) ?? []).length; |
There was a problem hiding this comment.
P2 — Quarantine legacy fences across Markdown delimiter forms. This count misses indented backtick fences and every ~~~ fence; after removing trailing backtick from the tail heuristic, an unfinished legacy summary using those forms is accepted by projection/ledger recovery. Please have the shared scanner track backtick/tilde family, indentation and opener width, then add projection and canonical-ledger recovery cases for valid and unterminated fences.
|
Closing in favor of #3039 as the single history-compaction summary-validation authority, following the maintainer coordination. The load/replay behavior from this PR is consolidated in reviewed commit 48de7eb, prepared for the #3039 author to cherry-pick, so this parallel validator should not remain open. |
Summary
Closes #3041. #3029 (PR #3040) added validation at the two checkpoint write gates, but a checkpoint whose truncated summary was persisted before that gate — incident checkpoint
hcheckpoint-981ceab8…in sessionfbdb3fd3, plus two more ind282f6ac— still loads and replays. This PR closes the load side.What changed
isHistoryCompactSummaryTruncatedfromvalidateHistoryCompactSummary(fence count + tail punctuation).loadLatestHistoryCompactCheckpointFromRunLedger: the bounded projection fast path and the canonical ledger recovery path.repairEventProjection. No data is lost: raw events are append-only, and the next high-water fold rewrites a fresh checkpoint.missing_sectionscheck at load: legacy checkpoints predate the sectioned summarizer contract and remain usable without sections. Only truncation is load-bearing regardless of writer era.Review fixes (first-principles + Occam double review)
…signal, and renamed the predicate to `isHistoryCompactSummaryTruncated`.Validation
Stacking
Stacked on #3040 (
maka/history-compact-summary-validation), which is not yet merged; this branch contains those 3 commits plus 2 new ones. Merge after #3040, at which point the diff reduces to the 2 new commits.