fix(runtime): validate history-compact summaries at the write gates (#3029) - #3040
fix(runtime): validate history-compact summaries at the write gates (#3029)#3040me2seeks wants to merge 4 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.
me2seeks
changed the title
📝 WalkthroughWhat problem this solvesHistory compaction could replace folded runtime history with truncated or unstructured summaries. This PR validates summaries before persistence. Validation requires the Design and source of truthThis PR extends the existing compaction and checkpoint write paths. It does not create a parallel persistence path. The canonical section list is shared by the validator and summarization prompt. Both checkpoint write gates apply the same validation result. This keeps the prompt contract and runtime enforcement aligned. Scope and complexityThe solution is the smallest coherent change for the stated failure mode:
The added constants and rejection type are necessary to prevent drift between prompt generation and validation. The reverted parallel tool-call merge is not part of the remaining implementation and should not be reintroduced here because Possible simplificationNo required production code deletion is evident. The test fixture updates are necessary because valid summaries must now use the required structure. If test helpers exist elsewhere for repeated structured summaries, they could centralize fixture creation. This would reduce repetition without reducing coverage. The existing regression cases should remain. Risks and validationMain risks are false rejection of valid summaries and false acceptance of degraded summaries. The validator uses line-anchored required headings, so near-match headings and raw tool-call markup receive explicit coverage. It also checks code-fence balance and truncation punctuation. The tests cover:
The PR does not implement the broader issue proposals for size bounds, deterministic facts, retries, or model changes. WalkthroughHistory compaction now requires structured summaries. Validation rejects missing sections, unbalanced code fences, and truncation indicators. Malformed output fails open, avoids checkpoint creation, and reports ChangesHistory compaction summary validation
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: 🟡 Moderate · up to The change can still accept certain malformed summaries, allowing degraded compaction output to replace conversation history and cause incorrect continuation behavior. Merge should wait until the heading and code-fence validation cases are fixed or explicitly accepted by the owner. Sequence Diagram(s)sequenceDiagram
participant Summarizer
participant Validator
participant CheckpointWriter
participant CompactionFlow
Summarizer->>Validator: Generate structured summary
Validator->>CheckpointWriter: Accept or reject summary
CheckpointWriter->>CompactionFlow: Create checkpoint or return malformed_summary
CompactionFlow->>CompactionFlow: Retain history on fail-open
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/runtime/src/__tests__/mid-turn-capacity-backend.test.ts (1)
301-306: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the duplicated default summary fixture.
Lines 304 and 432 define the same valid summary. Define one test-local constant and use it in both paths. This prevents fixture drift when the summary contract changes.
As per path instructions, “Flag concrete cases where code can be deleted or simplified.”
Also applies to: 430-432
Source: Path instructions
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 158830e2-838e-4d2f-aa33-01ab064a453c
📒 Files selected for processing (11)
packages/runtime/src/__tests__/ai-sdk-backend.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-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 | 🟠 Major | ⚡ Quick win
Require an exact heading line.
Line 44 accepts ## Goal continued because \b only rejects a word-character suffix. This violates the exact section contract and lets a near-match heading satisfy the gate.
Match only optional horizontal whitespace after the canonical heading. Add a regression case for ## Goal continued.
Proposed fix
- (section) => new RegExp(`^${escapeRegExp(section)}\\b`, 'm'),
+ (section) => new RegExp(`^${escapeRegExp(section)}[\\t ]*$`, 'm'),As per path instructions, this review checks the validation gate against the claimed malformed-summary prevention objective.
📝 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)}[\t ]*$`, '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)
Source: Path instructions
| // An odd number of fences means the output stops inside a code block. | ||
| const fenceCount = (summary.match(/^```/gm) ?? []).length; | ||
| if (fenceCount % 2 === 1) return 'truncated'; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Parse Markdown fence delimiters before accepting the summary.
Line 68 only counts lines that start with three backticks. An unclosed ~~~ fence receives no count and passes validation. A four-backtick opener followed by a three-backtick line also produces an even count, although Markdown keeps the fence open.
Track the opening fence character and width. Close it only with the same character and an equal-or-longer delimiter. Add regression cases for both forms.
As per path instructions, this review checks the validation gate against the claimed malformed-summary prevention objective.
Source: Path instructions
|
@me2seeks Following up on Astro-Han's review of #3039, which flagged this PR and #3039 as uncoordinated parallel implementations of the #3029 fix (and #3046 stacking on this one) — proposing an explicit split so one validation contract survives:
Happy to adjust the predicate's shape if #3046 needs something different at load time — deferring to you and the maintainers on the final call. Generated-by: Claude Code |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for addressing a real irreversible boundary: a history-compaction summary can become the only replay survivor for a large event prefix, so malformed output must fail open. I reviewed this head with two independent @reviewer passes plus a read-only ollama-cloud/deepseek-v4-flash:high pass.
The write-gate direction is correct, but the current implementation does not yet make semantic admission authoritative. The validator accepts headings inside a completed fence and accepts empty sections, while restart/load still admits older shape-valid malformed checkpoints without the new policy. Current required checks are also failing.
The simpler first-principles design is one scanner and one admission policy: parse ordered, non-empty required sections outside backtick/tilde fences; invoke it at the durable checkpoint authority; and reuse the same policy when loading, repairing projections, or copying checkpoints. On rejection, replay the canonical events. #3039 changes the same authority and already pursues a stronger scanner, so please consolidate these efforts rather than maintain parallel validators.
No local test suite was run during this review; conclusions are based on source, test, and current CI inspection. Codex coordinated the independent passes and performed the final adjudication; external-model output was treated as unverified until checked against the code.
中文摘要
感谢修复这个不可逆边界：history compaction summary 会成为大段事件历史的唯一 replay survivor，因此 malformed output 必须 fail-open。
当前 write gate 方向正确，但 semantic admission 还不是权威：validator 会接受完整 fence 内的 headings 和空 section，restart/load 也仍会接纳旧的、shape 合法但语义损坏的 checkpoint；当前 required checks 也在失败。
更符合第一性原理和奥卡姆剃刀的方案是只保留一个 scanner 与一套 admission policy：只接受 fence 外、顺序正确且非空的 required sections；在 durable checkpoint authority 执行；load、projection repair 和 copy 都复用同一 policy；拒绝时回放 canonical events。#3039 正在修改同一权威且已有更强 scanner，建议合并方案，不要维护两套 validator。
本次未在本地运行测试套件；结论来自源码、测试和当前 CI 检查。Codex 协调了独立审查并完成最终判断，外部模型输出在核对代码前均视为未验证输入。
| 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 — Do not admit empty or fenced template summaries. These independent global heading regexes accept `## Goal
Progress
Next Steps` with no content, and they also accept all three headings inside a completed fenced block because an even fence count is considered valid. Either result can replace the folded RuntimeEvents with effectively no continuation facts. Please use one fence-aware scanner that requires the sections in order, outside backtick/tilde fences, with non-empty content; add focused title-only and fenced-template tests that assert no checkpoint is recorded.
|
Closing in favor of #3039 as the single history-compaction summary-validation authority, following the maintainer coordination. Keeping a second write-gate validator would recreate the overlapping contract. The remaining #3039 load/replay review fix is prepared at me2seeks commit 48de7eb for its author to cherry-pick. |
|
Thanks for the clean handoff — 48de7eb cherry-picked into #3039 as 4b05575 with your authorship preserved. The legacy-tolerant load policy (quarantine truncation only, keep section-less-but-intact legacy summaries loadable) is exactly the right version-aware line, and the shared-scanner reuse means write admission and load quarantine cannot disagree about fence semantics. #3039 now carries the single validation contract end to end. Generated-by: Claude Code |
3 participants
Summary
History compaction folds hundreds of RuntimeEvents into one checkpoint summary. Before this change the only gate on that summary was
empty_summary, so a degraded provider response — a section-less fragment ending mid-sentence, a single word, or raw tool-call markup — replaced the folded history and made the continuation model hallucinate implementation details (see #3029).Root cause (reproduced)
Incident session
fbdb3fd3: compaction folded 742 events / ~235,503 tokens into a 138-token section-less fragment, persisted because every non-empty string passed the gate. Threeprovider_errorfail-opens preceded the degraded write.The model returned ~160 tokens of raw tool-call markup (no summary sections). The old gate accepted it. (The other failure layer — the summarizer emitting parallel tool calls as separate assistant messages, triggering the
provider_errorretries — is tracked as #3030 and fixed independently by #3038.)Changes
history-compact-summary-validation.ts(new):validateHistoryCompactSummary()enforces the summarizer's own structural contract — the required section headings (## Goal/## Progress/## Next Steps) matched line-anchored, plus truncation signals (unclosed code fence, trailing continuation punctuation). Section names are the single source of truth for both the prompt and the validator.writeHistoryCompactCheckpoint→ throwsHistoryCompactSummarizerError('malformed_summary'), mapped tofailOpenReason: 'malformed_summary'.planMidTurnCapacityCompaction→ returnsfail_openwithdiagnosticReason: 'malformed_summary'.'malformed_summary'added to the failure-reason union.## Goals), a directwriteHistoryCompactCheckpointgate test, and a backend-level fail-open regression test. Fixtures updated to the conforming shape.Verification
npm --workspace @maka/runtime run test→ 2805 pass / 0 failnpm run typecheck→ clean (all packages)npx biome check→ clean (2298 files)malformed_summary; the non-compliant output is rejected withmissing_sections.Review notes
Related