fix(runtime): validate compaction summaries before they replace history - #3039
Conversation
|
Follow-up c08bfa2 after an adversarial self-review pass:
Known trade-offs, deliberately kept to the issue's chosen semantics and left as follow-ups:
Full |
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for adding a checkpoint admission guard for the #3029 failure. Validating the generated summary before it replaces the covered history is a useful and appropriately scoped incident fix. I found one remaining structural bypass, plus a production-seam fixture that needs updating before CI can pass.
[P2] Required headings inside a closed code fence are accepted as real summary structure
The current regular expressions search every line, including lines inside fenced code blocks. As a result, this completion is accepted:
```markdown
## Goal
template goal
## Progress
template progress
## Next Steps
template next
```I reproduced this against the production buildLlmHistorySummarizer at the current head. This is a plausible degraded-model response: a weak model can quote or demonstrate the requested template instead of producing the actual checkpoint, and the fenced example would still be allowed to replace the history.
Could the validator scan the document once and only recognize required headings outside code fences? The same small scanner could:
- require the sections in the expected order with non-empty content;
- detect unclosed backtick and tilde fences;
- avoid maintaining separate heading and fence interpretations.
I do not think this needs a general Markdown parser.
There is also a directly related CI fixture failure. The real-provider fixture in execution-model-composition.test.ts still returns the shared one-section SUMMARY_TEXT, so the new validator correctly rejects it, no checkpoint is written, and the expected Memory boundary becomes undefined. A compaction-specific structured fixture would fix this without changing the shared summary used by recap, proposal, and other tests.
As a non-blocking follow-up, I think the longer-term contract should make a versioned structured summary object authoritative and treat Markdown as a deterministic rendering. Model-authored narrative can then be kept separate from facts owned by Runtime and the durable ledgers. A bounded repair retry would also avoid retrying the same deterministic format failure on every Turn. That larger design does not need to block this incident fix once the fenced-heading bypass and CI fixture are corrected.
The existing roll-forward coverage fix and focused validation tests look sound; I did not find tests that should be removed. The already documented legacy-checkpoint and token-calibration limitations are better tracked as explicit follow-ups rather than expanded into this PR.
One merge-hygiene note: because the PR discloses material Claude Code assistance, please preserve a Generated-by: Claude Code trailer in the final squash commit.
中文对照
感谢为 #3029 的事故增加 checkpoint admission gate。在摘要替换 covered history 之前进行校验,是一个有价值且范围合适的事故修复。目前还存在一个结构校验绕过,以及一个需要更新才能让 CI 通过的真实调用链 fixture。
[P2] 闭合代码块中的必需标题会被当成真实摘要结构
当前正则会搜索所有行,包括 fenced code block 内的内容。因此下面的输出会被接受:
```markdown
## Goal
template goal
## Progress
template progress
## Next Steps
template next
```我已在当前 head 的生产 buildLlmHistorySummarizer 上复现。这个退化形态并不牵强:较弱的模型可能只是引用或演示提示中的模板,而没有真正生成 checkpoint;当前实现仍会允许这个 fenced 示例替换历史。
建议对文档做一次按行扫描,只认可代码围栏之外的必需标题。同一个小型扫描器还可以:
- 要求 section 顺序正确且内容非空;
- 检测未闭合的 backtick 和 tilde fence;
- 避免标题校验和 fence 校验分别维护两套文本解释。
这里不需要引入通用 Markdown parser。
当前还有一个由本 PR 直接触发的 CI fixture 失败。execution-model-composition.test.ts 的真实 provider fixture 仍返回共享的单节 SUMMARY_TEXT,因此新 validator 正确拒绝它、不再写入 checkpoint,后续预期的 Memory boundary 变成了 undefined。建议增加一个 compaction 专用的结构化 fixture,而不是修改 recap、proposal 等其他测试共用的 summary。
作为非阻塞 follow-up,长期更合理的契约是让带版本的结构化 summary object 成为权威数据,Markdown 只作为确定性渲染。这样可以把模型生成的叙事与 Runtime、durable ledger 拥有的真实事实分开。再增加一次有上限的格式修复重试,也能避免同一个确定性格式错误在每个 Turn 上反复失败。修复当前 fenced-heading 绕过和 CI fixture 后,不需要让这套更大的终局设计阻塞本次事故修复。
现有 roll-forward coverage 修复和 focused validation tests 是有效的,没有发现应该删除的测试。作者已说明的旧 checkpoint 和 token 校准限制,更适合建立明确的 follow-up,而不是继续扩大本 PR。
另有一项合并规范:PR 已披露使用 Claude Code 产生了实质性内容,最终 squash commit 应保留 Generated-by: Claude Code trailer。
AI-assisted review disclosure: Codex assisted with inspecting the diff and reproducing the validation and CI cases. I verified the evidence, assessed the scope and severity, and made the final review judgment.
|
Both addressed in 0cc6ef7. Fenced-heading bypass: replaced the per-heading regexes with the single line scanner you suggested — one interpretation of the document for both checks. It recognizes the mandated sections only outside fenced code blocks, requires them in the expected order with non-empty content each (subheadings organize, non-heading lines and fenced lines carry content), and reports end-inside-open-fence from the same scan. Backtick and tilde fences are tracked by family, line-opening only, so a verbatim ``` inside a preserved error message stays content. Tests added: your fenced-template completion is rejected, a heading-only skeleton is rejected, and out-of-order sections are rejected; the inline-fence-marker acceptance test still passes. CI fixture: Agreed on the longer-term contract (versioned structured summary object with Markdown as rendering, plus a bounded repair retry for deterministic format failures) — that pairs naturally with the legacy-checkpoint quarantine follow-up already noted. Merge hygiene: added the |
|
One more hardening from a self-run adversarial probe pass (fb24f9d): a heading skeleton interleaved with horizontal rules ( Generated-by: Claude Code |
There was a problem hiding this comment.
The exact fenced-template case and the Runtime Host fixture from the previous review are fixed. I verified the focused summarizer suite (20/20) and Biome on the six changed files. One structural bypass remains.
[P2] Preserve the opening fence width when deciding whether a fence closes
scanSummaryStructure tracks only the fence character family. Any later run of three or more characters from the same family closes the fence, even when it is shorter than the opener. Markdown requires a closing delimiter to be at least as long as the opening delimiter.
I reproduced this against the production buildLlmHistorySummarizer at the current PR head. The following completion is accepted:
[
"````markdown",
"```",
"## Goal",
"template goal",
"## Progress",
"template progress",
"## Next Steps",
"template next",
].join("\n")Under Markdown semantics, the three-backtick line does not close the four-backtick opener, so every required heading remains inside one unclosed fenced block. The scanner closes it early, recognizes those headings as real structure, and allows the completion to replace the covered history. This is the same failure class as the earlier fenced-template bypass.
Please retain the opening fence character and width, and only close it with the same character, an equal-or-longer delimiter, and no non-whitespace trailing content. A regression case using the completion above should cover it.
The PR also currently conflicts with the latest main, which added provider-native history compaction. The rebase should preserve the distinction between text summaries, which need this validation, and opaque provider-native checkpoint state.
AI-assisted review disclosure: Codex inspected the diff, traced the compaction lifecycle, and reproduced the fence-width bypass against the PR head. I verified the evidence and made the final review judgment.
abbe164 to
11eac74
Compare
|
Rebased onto
Full Generated-by: Claude Code |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (3)
Included review availability: Your plan includes up to 3 reviews per rolling hour; 0 remain after this review. 📝 WalkthroughWhat problem this solvesThis PR prevents malformed history-compaction summaries from replacing conversation history. It rejects summaries that omit required sections, use invalid order, contain empty content, echo templates, contain truncated fenced content, or are too small for the complete folded history span. The pipeline fails open. It retains history, retries compaction later, and records typed validation reasons in diagnostics. Existing Design and scopeThis PR extends the existing history-compaction path. It does not create a parallel path. The shared The shared scanner is necessary to validate section order, meaningful content, fenced blocks, thematic breaks, template echoes, and truncation consistently. Fence-family and opening-width tracking prevents invalid closers from bypassing truncation checks. The complete fold span and session No production code or regression test can be deleted without weakening behavior or incident coverage. Updated fixtures are necessary because accepted summaries must use the checkpoint format. ValidationTests cover missing, malformed, empty, and out-of-order sections; invalid headings; horizontal rules; fenced templates; unclosed and mismatched-width fences; truncation markers; inline fence text; complete fold-size floors; prior checkpoints; character/token calibration; rolling checkpoints; provider-native checkpoints; manual compaction; capacity-triggered compaction; and the reported short-summary regression. The end-to-end regression verifies that malformed output fails open, writes no checkpoint, preserves the raw history span in the next prompt, and records The final status of required checks is unverified from the available evidence. Review-relevant risksThe PR changes user-visible compaction behavior. Previously accepted short or unstructured provider responses can now fail validation and retain conversation history. Material changes in this area require independent human review under repository policy. The PR adds No security, licensing, release, or governance effect was identified in the current diff. The person performing the merge reviews the final diff. A maintainer makes the final determination. WalkthroughHistory compaction now validates structured summaries for required sections, truncation indicators, Markdown fence integrity, and folded-history size. Invalid summaries fail open before checkpoint persistence. Tests and provider fixtures now use structured compaction responses. ChangesHistory Compaction Validation
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: 🟡 Moderate · up to Malformed summaries may still pass validation in two fence-handling cases and replace conversation history, potentially causing misleading continuations or lost context. Merge should wait for these cases to be fixed or explicitly accepted. Sequence Diagram(s)sequenceDiagram
participant RuntimeCompaction
participant HistoryCompactSummarizer
participant SummaryValidator
participant CheckpointStore
RuntimeCompaction->>HistoryCompactSummarizer: summarize folded runtime events
HistoryCompactSummarizer->>SummaryValidator: validate structured summary
SummaryValidator-->>HistoryCompactSummarizer: valid summary or failure reason
HistoryCompactSummarizer->>CheckpointStore: persist valid checkpoint
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e42843aa-201e-41e2-8ede-5b31f71bfb81
📒 Files selected for processing (6)
packages/runtime-host/src/__tests__/execution-model-composition.test.tspackages/runtime/src/__tests__/history-compact-summarizer.test.tspackages/runtime/src/__tests__/mid-turn-capacity-backend.test.tspackages/runtime/src/__tests__/session-manager.test.tspackages/runtime/src/history-compact-error.tspackages/runtime/src/history-compact-summarizer.ts
Included review availability: Your plan includes up to 3 reviews per rolling hour; 1 remains after this review.
11eac74 to
680fa1a
Compare
|
The Generated-by: Claude Code |
Astro-Han
left a comment
There was a problem hiding this comment.
The incident fragment, the closed fenced template, and the roll-forward size-floor bypass are covered. Two things still stand in the way of approve.
The fence-width case CodeRabbit already opened on scanSummaryStructure still reproduces on this head (a four-backtick opener closed by a three-backtick line, headings accepted). Same failure class as the earlier fenced-template bypass.
The other is layering, inline on the generate-time assert. #3040 is the parallel #3029 fix and already owns the write gates. Landing both as-is leaves two contracts.
AI-assisted review: Grok 4.6 inspected the current head and reproduced the four-backtick completion against scanSummaryStructure. I checked the write-path call sites on this branch and on #3040, and confirmed the generate-only assert. Unverified by me: I did not rerun the runtime suite.
|
Rebased again after #3038 and #3100 landed on Generated-by: Claude Code |
618d756 to
b65a087
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/runtime/src/history-compact-summarizer.ts (1)
204-210: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace the constructed
RegExpwith a literal prefix check.Line 206 builds a regex from
REQUIRED_SUMMARY_SECTIONS[matchedSections]on every line. The constants at line 41 contain no regex metacharacters today, so behavior is correct. If a future section constant gains a character such as(,+, or., the regex silently changes meaning or throws. A literal check removes that coupling, removes the per-line allocation, and clears the ast-grepregexp-from-variablewarning.♻️ Proposed refactor
+function matchesSectionHeading(line: string, heading: string): boolean { + if (!line.startsWith(heading)) return false; + const next = line[heading.length]; + // Word boundary: end of line, or a non-word character after the heading. + return next === undefined || !/[\w]/.test(next); +}if ( matchedSections < REQUIRED_SUMMARY_SECTIONS.length && - new RegExp(`^${REQUIRED_SUMMARY_SECTIONS[matchedSections]}\\b`).test(line) + matchesSectionHeading(line, REQUIRED_SUMMARY_SECTIONS[matchedSections]!) ) {Source: Linters/SAST tools
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7d5c845a-4b64-4e53-8cff-9c764c3b6f81
📒 Files selected for processing (3)
packages/runtime-host/src/__tests__/execution-model-composition.test.tspackages/runtime/src/__tests__/history-compact-summarizer.test.tspackages/runtime/src/history-compact-summarizer.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/runtime-host/src/tests/execution-model-composition.test.ts
- packages/runtime/src/tests/history-compact-summarizer.test.ts
Included review availability: Your plan includes up to 3 reviews per rolling hour; 2 remain after this review.
|
Thanks for digging into the incident — the incident fragment is indeed rejected (I verified the regression test reproduces #3029's exact shape), and the scanner design is clean. Two independent reviews converged on the same blockers, worth resolving before merge. Conclusion: needs work — one P1 (parallel implementation with #3040/#3046) plus a reproducible validation bypass. P1 — this PR and #3040/#3046 are uncoordinated parallel implementations of the same fix. P2-1 — the fence-tracking bypass (already reproduced by human reviewers, still open). The scanner tracks fence character family but not opening width: a 4-backtick opener closed by a 3-backtick line is misjudged as closed (Markdown requires the closer to be ≥ opener length). I reproduced: a template-echo output with P2-2 — validation still passes content that is meaningless. An unfenced echo of the prompt template (346 chars of placeholders) is ACCEPTED for folds ≤10k tokens (the size floor only guards >10k); a 3-section summary missing P2-3 — the claimed acceptance criterion ("rejection lands in Optional nits (P3): the trailing-colon truncation heuristic rejects legitimate summaries ending in AI-assisted review disclosure: this review was produced with AI assistance (two pi review subagents on 中文摘要(AI 辅助审查)结论:FAIL,需要返工。两个独立审查收敛到同一组阻塞项:① P1:#3039 与 #3040/#3046 是同一修复的未协调并行实现(15 分钟内先后创建,改同一批文件,两套 validator 语义不同且各有互补的 fence bug),违反仓库"不建并行实现"原则,须与 #3040/#3046 作者对齐二选一并显式关闭另一份;② P2-1 fence 关闭宽度绕过(4-backtick 开启被 3-backtick 行误判闭合,已被人类审查人两次复现),结构性校验可被模板回显穿透,需记录开启宽度并要求闭合宽度 ≥ 开启宽度;③ P2-2 验证仍放行无意义内容——unfenced 模板回显(346 chars 占位符)在 ≤10k tokens 折叠上通过(size floor 只保护 >10k),缺 Critical Context 的三节瘦摘要也通过,建议把 Critical Context 加入必需 section;④ P2-3 声称的验收准则(拒绝 reason 落账)无端到端测试,只有单元层。事故片段确实被拦住且有逐字回归测试 ✅。P3 可选:尾冒号截断误杀合法摘要导致压缩永久停摆、charsPerToken 校准延后、fixture 字符串四处硬编码重复。 |
|
Thanks for the thorough round — both reproduced bypasses were real. All four points addressed in 33c509d: P2-1 (fence width): the scanner now records the opening run's width; a closer must match the family AND be at least as long. Your exact `````markdown P2-2 (meaningless content): three changes.
P2-3 (end-to-end): added at the backend seam ( P1 (parallel implementations): the validation now lives in its own P3s: the per-line Full Generated-by: Claude Code |
There was a problem hiding this comment.
Pull request overview
This PR hardens runtime history compaction by validating LLM-produced checkpoint summaries before allowing them to replace folded history, preventing malformed/truncated/too-small summaries from becoming durable checkpoints.
Changes:
- Adds a single-pass Markdown-aware validator for checkpoint summary structure, truncation heuristics, and a size floor relative to the folded span.
- Integrates validation into
buildLlmHistorySummarizer, surfacing granular typed failure reasons viaHistoryCompactSummarizerError. - Updates/increases test coverage (unit + end-to-end fixture) and adjusts host/provider stubs to emit compaction-shaped summaries.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/runtime/src/history-compact-summary-validation.ts | New shared contract + validator for checkpoint summary structure/truncation/size floor. |
| packages/runtime/src/history-compact-summarizer.ts | Builds prompt from shared template and rejects invalid summaries with granular reasons. |
| packages/runtime/src/history-compact-error.ts | Extends failure-reason union with new malformed-summary reasons. |
| packages/runtime/src/tests/session-manager.test.ts | Updates summarizer stub output to satisfy new validation contract. |
| packages/runtime/src/tests/mid-turn-capacity-backend.test.ts | Threads summarizer input through fixture and adds end-to-end fail-open regression for malformed summaries. |
| packages/runtime/src/tests/history-compact-summarizer.test.ts | Adds comprehensive validator-focused regression tests, including incident-shaped fragments. |
| packages/runtime-host/src/tests/execution-model-composition.test.ts | Updates provider stub to return structured compaction summaries for compaction requests. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 22413445-953d-4ea9-bb2d-514ce7134efe
📒 Files selected for processing (6)
packages/runtime-host/src/__tests__/execution-model-composition.test.tspackages/runtime/src/__tests__/history-compact-summarizer.test.tspackages/runtime/src/__tests__/mid-turn-capacity-backend.test.tspackages/runtime/src/__tests__/session-manager.test.tspackages/runtime/src/history-compact-summarizer.tspackages/runtime/src/history-compact-summary-validation.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/runtime-host/src/tests/execution-model-composition.test.ts
- packages/runtime/src/history-compact-summarizer.ts
- packages/runtime/src/tests/session-manager.test.ts
- packages/runtime/src/tests/history-compact-summarizer.test.ts
Included review availability: Your plan includes up to 3 reviews per rolling hour; 2 remain after this review.
33c509d to
c95b68b
Compare
|
Rebased past #3113 (summarizer input bounding) and #3128. One rebase-only fixture pass, same pattern as the previous rounds: #3113's new bounding test stubs a free-form summary over a >10k-token span at Generated-by: Claude Code |
…tion Adversarial-review follow-up on the initial validation: - The size floor is now measured against the FULL covered span the checkpoint replaces (input.source.foldedRuntimeEvents via the shared estimateRuntimeEventsTokens), not the newly folded increment plus the prepended prior-summary text. Steady-state roll-forward compaction — a small increment updating a checkpoint that covers a large span — could previously slip a fragment past the floor entirely; the prior summary text also inflated the measure in the other direction. The estimate is only computed when the summary is short enough to matter. - Section detection is line-anchored, so a '### Goal' heading or an inline mention no longer satisfies the check via substring inclusion. The prompt is built from the same section constants, so the mandated format and the validation cannot drift apart. - Fence-truncation only counts fences that open a line, so a verbatim ``` inside a preserved error message is not read as truncation. - The flat malformed_summary reason is split into malformed_summary_missing_section / _truncated / _too_small_for_fold so compaction diagnostics can distinguish deterministic-rejection regimes without re-reading raw provider output. Generated-by: Claude Code
Review follow-up: the section regexes scanned every line, so a degraded model quoting the mandated template inside a fenced code block was accepted as real checkpoint structure. One line scan now holds a single interpretation of the document for both checks: the mandated sections must appear in order, each with non-empty content, and only outside fenced code blocks (backtick or tilde, line-opening only, so a verbatim fence marker inside a preserved error message stays content); the same scan reports whether the document ends inside an open fence. Also gives the hosted execution-model composition fixture a compaction-shaped completion: its shared one-section summary is now correctly rejected by the validator, which broke the Memory boundary expectation; history-compaction requests get their own structured text while recap, proposal and title keep the shared fixture. Generated-by: Claude Code
Adversarial-probe follow-up: a heading skeleton interleaved with
horizontal rules ('## Goal\n---\n## Progress\n---\n## Next Steps\n---')
passed the non-empty-content requirement because any non-heading,
non-blank line counted as content. Thematic breaks (---, ***, - - -)
are now excluded, so a separator-only skeleton is rejected like the
bare one; list items keep counting as content.
Generated-by: Claude Code
Generated-by: Claude Code
…recompression fixture The rebase onto apache#2993 brought a fixture that returns free-form text, which the validation this PR adds rejects by design. Same treatment as the other compaction fixtures: return the structured VALID_SUMMARY. Generated-by: Claude Code
… discussion) Generated-by: Claude Code
…xtures Same rebase-only treatment as the provider-native fixture: apache#3038's grouping tests stub free-form text this PR's validation rejects by design; they assert on the grouped messages, not the returned text. Generated-by: Claude Code
…ewed bypasses Review round (Astro-Han): - The scanner and defect predicate move to history-compact-summary-validation.ts, one module owning the mandated format: the prompt is built from its template and any other validation site (e.g. checkpoint load) can consume the same findCheckpointSummaryDefect predicate. - Fences track their opening width: a closer must match the family AND be at least as long, so a four-backtick template echo with a three-backtick line inside no longer reads as closed. - Verbatim template lines never count as section content, closing the unfenced template-echo acceptance. - '## Critical Context' joins the required sections — it carries exactly what the apache#3029 incident lost, and the template offers a '(none)' escape hatch. - The size floor uses the session's charsPerToken from the compaction input budget on both sides of the comparison. - End-to-end regression at the backend seam: a malformed completion from the real summarizer fails open, writes no checkpoint, keeps the raw span in the next prompt, and lands its granular reason in the compaction diagnostics. Generated-by: Claude Code
…ured summary Rebase past apache#3113: its new bounding test stubs a free-form summary and covers a >10k-token span at charsPerToken=1, so this PR's validation rejects it by design (missing sections, then the size floor). The stub now returns a structured summary proportionate to the fold; the test's own assertions (bounded input, preserved pairs) are unchanged. Generated-by: Claude Code
Review threads (Astro-Han, CodeRabbit, Copilot): - The write gates (pre-turn writer and the mid-turn engine) now call findCheckpointSummaryDefect before building a checkpoint, so the invariant — a malformed summary must not replace folded history — holds for any producer, not just the default summarizer's generate path. The generate-time throw stays for its typed reason. Regression tests at both gates feed a section-less fragment from a producer that skipped the summarizer and assert fail-open with the granular reason and no persist. - A fence closer must be a bare marker run: an opener may carry an info string, a closer may not (regression: trailing text is content). - charsPerToken is clamped to >= 1 so a zero/negative estimate cannot zero out the size floor. - Stub summaries across the compaction test files are now shaped like real checkpoints (sentinels preserved), padded proportionally where the covered span is large enough for the size floor to apply. Generated-by: Claude Code
…CRLF Copilot's second pass: - An unmatched H2 (e.g. the template's '## Key Decisions') opens its own section, so its content no longer satisfies the previous required section's non-empty requirement (regression: empty Progress backed only by Key Decisions bullets is rejected). - Lines split on /\r?\n/ so CRLF input cannot leave a trailing \r in any line-based check (the existing checks already tolerated it via \s* and trim, but normalization removes the class; CRLF acceptance and CRLF thematic-break rejection are now pinned by tests). Generated-by: Claude Code
Copilot's remaining suppressed observation: a section whose only lines are nested fence delimiters satisfied the non-empty requirement while carrying no checkpoint information. Bare marker runs are excluded from content counting (regression test with a nested shorter run). Generated-by: Claude Code
…apache#3041) Generated-by: Claude Code
…ment the summarizer contract Qodo review round: - The floor compares estimated tokens via the shared ceil-based estimateTokens instead of raw characters, so a summary of exactly the documented 200-token floor is accepted (boundary regression test). - HistoryCompactSummarizer's contract now states that a string result must satisfy the mandated checkpoint format owned by history-compact-summary-validation.ts — the write gates reject free-form plain text by design. Generated-by: Claude Code
The scanner comment read as if section CONTENT were also recognized only outside fences; the invariant is that headings are — fenced lines are literal content of their enclosing section by design. Generated-by: Claude Code
bef8aeb to
aa1e451
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
The latest update is a rebase; the PR-owned runtime files are unchanged from the tree reviewed. The shared scanner, both write gates, fail-open behavior, and load-time recovery are a strong improvement over the incident path. Two remaining boundaries keep me from approving this head.
First, the scanner accepts a four-space-indented fence marker as a delimiter, although Markdown treats it as indented code. That lets headings after a false closer escape an actually open fence. Second, every text checkpoint is still V2, so load/copy cannot distinguish a compatible legacy free-form summary from a newly produced section-contract summary; truncation-only quarantine therefore cannot enforce the new contract at the durable boundary. The smallest durable shape is an explicit summary-format marker/version: retain V2 compatibility, but apply the complete predicate to newly marked checkpoints across record, load, repair, and copy.
I am leaving a review comment rather than approving. AI-assisted review by Codex with three independent reviewer passes and OpenCode Go DeepSeek V4 Flash (high); I verified the rebased head, scanner probes, ledger/copy authority, focused test evidence, and live checks.
中文
最新更新只是 rebase,PR 自身的 runtime 文件与已审查 tree 相同。共享 scanner、两个写入 gate、fail-open 和 load-time recovery 都正确加强了事故路径。仍有两个边界未闭合:四空格缩进的 fence marker 被错误当作 delimiter;所有文本 checkpoint 仍共用 V2,load/copy 无法区分兼容的 legacy free-form summary 与新 section contract。
最小长期方案是增加明确的 summary format marker/version:V2 保留兼容,新格式在 record、load、repair、copy 全路径执行完整 predicate。当前先发 COMMENT,不 Approve。
本次由 Codex、三个独立 reviewer 和 OpenCode Go DeepSeek V4 Flash high 辅助;已核对 rebase 后 head、scanner probe、ledger/copy authority、focused tests 与实时检查。
…and reject indented pseudo-fences Review round (Astro-Han): - Fence delimiters are limited to at most three leading spaces per the Markdown rule: a four-space-indented marker run is indented code and can no longer falsely close (or open) a fence and let fenced headings count as structure. Regressions: four-space false closer rejected, three-space closer still closes. - Text checkpoints built under the sectioned contract carry a durable summaryFormat: 'sections_v1' marker. The load/repair authority holds marked checkpoints to the complete shared predicate (minus the size floor, whose covered-span estimate is not durable) while unmarked legacy V2 stays under the truncation-only compatibility policy; shape validation fails closed on unknown markers; conversation copy validates a marked source against the complete predicate and preserves the source's format identity in the target instead of promoting legacy free-form summaries to the new contract. - Test fixtures that model legacy data now build with 'legacy_freeform'; new tests cover the default stamping, unknown-marker rejection, and a marked malformed checkpoint quarantined at load with prior-valid recovery. Generated-by: Claude Code
|
Both remaining boundaries from the latest review are closed in c8d193e:
Full Generated-by: Claude Code |
Astro-Han
left a comment
There was a problem hiding this comment.
The latest head fixes both previously reported mechanisms: Markdown fences now follow the 0–3-space delimiter rule, and sections_v1 gives new section-contract summaries an explicit durable identity while preserving unmarked legacy free-form checkpoints. Unknown markers fail closed and load/copy preserve the marker.\n\nTwo admission gaps remain below. Both come from the same first-principles rule: sections_v1 should be proof that the full predicate was checked, so only the durable authority may assign/cache it, and every transfer seam with covered events available must re-run the size floor. That is smaller and safer than adding more downstream exceptions.\n\ntest_workspaces is still running; other reported checks currently pass.\n\nReviewed with Codex using two independent reviewer agents and OpenCode Go DeepSeek V4 Flash (high); I verified the exact delta, prior findings, malformed-summary paths, copy/record/load authority, compatibility, tests, and live CI.\n\n
中文
\n\n最新 head 已修复此前两个机制:Markdown fence 现在遵循 0–3 spaces delimiter 规则;sections_v1 为新的 section-contract summary 提供显式 durable identity,同时保留无 marker 的 legacy free-form checkpoint。未知 marker 会 fail closed,load/copy 也会保留 marker。\n\n下面仍有两个 admission 缺口,根因相同:sections_v1 应代表完整 predicate 已验证,因此只有 durable authority 能赋予/缓存它;任何能拿到 covered events 的 transfer seam 都应重新执行 size floor。这比继续添加下游例外更小、更安全。\n\ntest_workspaces 仍在运行;其余已报告检查当前通过。\n\n本次由 Codex 配合两个独立 reviewer agent,以及 OpenCode Go DeepSeek V4 Flash(high)审查;我核验了精确增量、既有 finding、malformed-summary 路径、copy/record/load authority、兼容性、测试和实时 CI。\n\n… after the complete predicate Review round (Astro-Han): sections_v1 must be proof the full predicate was checked, assigned only by the durable authority, with the size floor re-run at every seam that has the covered events. - buildHistoryCompactCheckpoint runs findCheckpointSummaryDefect — structure, truncation, AND the size floor over the covered span it is handed — before stamping, and throws on a defect. A direct recorder caller can no longer mint the trusted marker for unvalidated text; the coordinator only ever persists/caches marker-bearing checkpoints that earned it. legacy_freeform remains the explicit escape for preserving unmarked legacy summaries (copy). - Conversation copy re-runs the complete predicate with the MATCHED covered span, so a structurally valid but undersized marked summary cannot be copied over a >10k-token fold; the builder backstops the same rule during the rebuild. - The Desktop E2E compaction fixture now emits a sectioned summary. - Regressions: minting refused for free-form text, the builder size floor over a large covered span, and legacy-modeling fixtures across the suites declare legacy_freeform explicitly. Generated-by: Claude Code
|
Both admission gaps closed in 12f020f, by centralizing rather than adding downstream exceptions: Generated-by: Claude Code |
Astro-Han
left a comment
There was a problem hiding this comment.
Centralizing sections_v1 minting in buildHistoryCompactCheckpoint() closes the previous admission/copy gaps, and current CI is green. One structural parser bypass remains, so an information-free heading skeleton can still receive the trusted marker and replace history.
AI-assisted review disclosure: Codex reviewed exact head 12f020f, the latest builder/copy delta, all current checks, and thread state, with two independent reviewer passes.
中文说明
把 sections_v1 的签发集中到 checkpoint builder 是正确的最终状态,之前的 admission/copy 缺口已关闭,当前 CI 也已全绿。但结构扫描没有按 Markdown 规则识别 1–3 个前导空格或行尾结束的 ATX heading,纯 heading 骨架仍可能被当成有效内容并获得可信 marker,需要先修复。
…g content Review round (Astro-Han): CommonMark permits up to three leading spaces and lets the marker run end the line, so ' ### Done' or a bare '##' was counted as section content and an information-free heading skeleton could earn the sectioned marker. One heading interpretation now serves all three checks: required-section matching tolerates up to three leading spaces around the exact heading text, the non-required-H2 transition matches '## ...' and bare '##' (but not deeper levels), and content exclusion skips every ATX heading of any level including bare and indented runs. Regressions: an indented/bare heading skeleton is rejected; a required heading indented up to three spaces still matches its section. Generated-by: Claude Code
Astro-Han
left a comment
There was a problem hiding this comment.
Approved on exact head d810079.
The summary-validation contract is now coherent at its natural authority: the checkpoint builder alone mints sections_v1 after the complete predicate passes, write/load/copy paths preserve the durable format distinction, and malformed marked checkpoints fail open to canonical history. The latest delta closes the remaining indented and bare ATX-heading skeleton bypass without adding a parallel parser.
No remaining P0-P2 findings. All current checks pass, the PR is CLEAN and mergeable, and there are no unresolved review threads.
AI-assisted review disclosure: Codex reviewed the exact head, the full feedback history, the final parser delta, current CI, and thread state. Astro-Han authorized this approval and made the final merge decision.
中文说明
精确 head d810079 已通过审查。summary validation 已收敛到正确的单一权威:只有 checkpoint builder 在完整 predicate 通过后签发 sections_v1;write、load、copy 保留 durable format 边界;损坏的 marked checkpoint 会 fail open 并回放 canonical history。最新提交修复了最后的缩进与 bare ATX heading 骨架绕过,没有引入并行 parser。
当前没有剩余 P0-P2;CI 全绿、merge state CLEAN、无未解决 review thread。
|
Addressed the remaining ATX-heading P2 in a focused cross-fork follow-up because the #3039 head repository does not grant me direct push permission: UncertaintyDeterminesYou4ndMe#1 The patch uses one CommonMark-compatible matcher for both non-required-H2 transitions and heading content exclusion, with an indented/bare heading-only regression. Runtime build, focused summarizer suite (44/44), Biome, and diff check pass locally. |
|
@me2seeks Thanks — that follow-up raced with d810079, which landed in this PR's merged head minutes before the merge and covers the same gap (one heading interpretation across content exclusion, the H2 transition, and additionally the required-section matcher, which now tolerates up to three leading spaces around the exact heading text). I've replied on the fork PR with the comparison and closed it; your single captured Generated-by: Claude Code |
Summary
Fixes #3029.
History compaction accepted any non-empty string as a checkpoint summary. In the incident, a degraded provider completion — a 138-token free-form fragment that ignored the mandated sections and ended mid-sentence, delivered with a
stopfinish reason right after threeprovider_errorfail-opens — replaced ~235k estimated tokens of history, and the continuation model confabulated around the missing context.What changed — the validation lives in a dedicated
history-compact-summary-validation.ts, one module owning the mandated format: the summarization prompt is built from its template, and a writer-agnosticfindCheckpointSummaryDefect(summary, foldContext?)predicate is consumable by any validation site (e.g. checkpoint load, see #3046). It is enforced at every layer (proposals 1–3 of the issue):ai-sdk-compaction.ts) and the mid-turn engine (mid-turn-capacity-compact.ts) — call the predicate before building a checkpoint, so the invariant holds for any producer.buildLlmHistorySummarizeradditionally rejects at generate time for the typed reason on the summarizer path.buildHistoryCompactCheckpointitself is the only authority that assigns thesections_v1marker, and it re-runs the complete predicate (including the size floor over the covered span it is handed) before stamping — a direct recorder caller cannot mint the trusted marker for unvalidated text;legacy_freeformis the explicit escape for preserving unmarked legacy summaries across copy.summaryFormat: 'sections_v1'marker, and the ledger's load sites hold marked checkpoints to the complete predicate (minus the size floor, whose covered-span estimate is not durable) — so a malformed summary entering through a direct recorder or copy seam never becomes authoritative after restart. Unmarked legacy checkpoints keep loading when intact; only truncation is quarantined writer-agnostically, via the same shared fence scan and a broadened mid-thought tail heuristic. Rejection repairs the projection from the canonical ledger and recovers the prior valid checkpoint; shape validation fails closed on unknown markers; conversation copy validates marked sources and preserves the source's format identity in the target.## Goal,## Progress,## Next Steps, and## Critical Contextmust appear in order, each with non-empty content, recognized only outside fenced code blocks. Critical Context is required because it carries exactly what the incident lost (files, commands, errors); the template gives it an explicit"(none)"escape hatch. Verbatim template lines, thematic breaks, and bare fence-marker runs never count as content, and content under a non-required H2 (e.g.## Key Decisions) cannot satisfy a required section above it — so a template echo, fenced or not, cannot pass.stop— the incident's exact shape. Fences follow the Markdown closing rule: a closer must be a bare run of the same family at least as long as the opener, tracked line-opening only, so a verbatim ``` inside a preserved error message is not read as truncation and a four-backtick fence is not closed by a three-backtick line. Lines are split on/\r?\n/so CRLF output is handled identically.charsPerToken, clamped ≥ 1) into less than ~200 estimated tokens of summary is rejected.A failed validation surfaces a granular typed reason (
malformed_summary_missing_section/_truncated/_too_small_for_fold). The existing pipeline turns that into a fail-open — history kept, compaction retried next turn — and the reason lands inhistoryCompactWriteSkippedReasonCounts/failOpenReasondiagnostics. The empty case stays with the compaction layer'sempty_summarygate, andoutput_lengthstill wins when the provider reports an exhausted budget.Scope notes
Verification
mid_turnfailedOpendecision), plus write-gate regressions at both persist sites feeding a section-less fragment from a producer that bypasses the summarizer.@maka/runtimeand@maka/runtime-hostsuites pass locally.AI use
Select exactly one:
Tool(s) and scope: Claude Code authored the fix, the tests, and this description under human direction and review; commits carry
Generated-by: Claude Codetrailers.Checklist
Does this PR entail a change in behavior?