feat(compaction): publish survival contract and keep last round (#4394) - #5782
feat(compaction): publish survival contract and keep last round (#4394)#5782Hmbown wants to merge 2 commits into
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Claude finished @Hmbown's task in 3m 42s —— View job Review complete
SummaryThis PR ports the compaction survival-contract work onto current Correctness bug (posted inline on Minor / non-blocking notes:
Testing note: I could not run |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // A trailing user/assistant pair with no tools still needs the previous | ||
| // tool-bearing round; otherwise the last results vanish behind the summary. | ||
| messages[..last_user] | ||
| .iter() | ||
| .enumerate() | ||
| .rev() | ||
| .find_map(|(idx, message)| { | ||
| if is_compaction_checkpoint_message(message) { | ||
| return None; | ||
| } | ||
| user_text_of(message).map(|_| idx) | ||
| }) | ||
| .unwrap_or(last_user) |
There was a problem hiding this comment.
Correctness bug: the toolless-tail fallback only steps back one user round, not to the nearest tool-bearing round.
The comment says this fallback exists so that "a trailing user/assistant pair with no tools still needs the previous tool-bearing round." But the code just finds the previous plain-user-text index — it never checks that round actually contains tool results. If there are two or more consecutive toolless user rounds after the last tool round, only one hop back is taken, and the real tool round is left outside the computed "last round" slice.
Trace, for
0 user "Run the failing test."
1 assistant "Running."
2 assistant tool_use("live", Bash …)
3 user tool_result("live", "...FAILED")
4 user "ok thanks"
5 assistant "you're welcome"
6 user "one more thing"
7 assistant "sure"
last_user = 6 ("one more thing"), tail (6..8) has no tools → fallback searches messages[..6] and finds idx 4 ("ok thanks") — not idx 0. last_round_start returns 4, so the slice [4..8] never includes the tool_use/tool_result at 2/3.
Because retained_user_messages (crates/tui/src/compaction.rs) only keeps plain user text from messages[..start] and drops tool_use/tool_result/assistant content entirely, the live tool result silently falls out of verbatim retention and is only as good as whatever the LLM summary captured.
Worse: validate_last_round_coverage / validate_survival_contract — the coverage floor this PR introduces specifically to prevent this class of loss — computes "last round" with this same last_round_start/last_round_range, so it's blind to its own gap and will report success even though the earlier tool round silently vanished behind the summary.
Suggest walking back until a round is found that actually contains a ToolResult block (or exhausting the message list), rather than a single hop:
// after the first `messages[..last_user]` search fails to find tools in tail:
let mut candidate = last_user;
loop {
let prev = messages[..candidate]
.iter().enumerate().rev()
.find_map(|(idx, m)| (!is_compaction_checkpoint_message(m))
.then(|| user_text_of(m).map(|_| idx)).flatten());
let Some(prev) = prev else { break candidate };
if messages[prev..].iter().any(|m| m.content.iter().any(|b| matches!(b, ContentBlock::ToolResult { .. }))) {
break prev;
}
candidate = prev;
}(or equivalent). No existing test exercises the tail_has_tools == false fallback branch at all — worth adding a regression test with ≥2 toolless rounds after the last tool round, asserting the tool result survives build_replacement_history.
|
Claude finished @Hmbown's task in 2m 0s —— View job Review complete
SummaryThis trigger fired on a Correctness bug (unresolved) — When the trailing round has no tool results, the fallback steps back exactly one user-round boundary ( Concrete trace:
Suggested fix: loop back over user-round boundaries until a round containing a let mut candidate = last_user;
loop {
let Some(prev) = messages[..candidate]
.iter().enumerate().rev()
.find_map(|(idx, m)| (!is_compaction_checkpoint_message(m))
.then(|| user_text_of(m).map(|_| idx)).flatten())
else { break candidate };
if messages[prev..].iter().any(|m| m.content.iter().any(|b| matches!(b, ContentBlock::ToolResult { .. }))) {
break prev;
}
candidate = prev;
}Add a regression test with ≥2 toolless rounds after the last tool round, asserting the tool result survives What's solid
Minor / non-blocking (repeated from prior review, still applicable)
Testing note: cargo ( |
Port the missing coverage floor onto origin/main: replacement history
keeps a bounded last user round (assistant + tool results) and refuses
the rewrite if that round, /anchor text, or the checkpoint receipt
would vanish. Failed compact still does not replace live history.
Publish the language-invariant schema next to the compaction module
and surface path + /anchor survival in /context. Billed parent prompt
tokens survive the turn boundary so last-turn pressure can still
auto-compact.
Evidence:
cargo test -p codewhale-tui --lib --offline --locked -- compaction context_inspector last_round
-> 125 passed; 0 failed
Refs: #4394
Signed-off-by: CodeWhale Bot <bot@codewhale.net>
Drop the unused validate_last_round_coverage re-export that failed Lint.
Walk last_round_start back to the last tool-bearing user round instead of
one hop, so two trailing toolless turns cannot drop a tool result behind
the summary. Sync the TUI changelog slice with root.
Add a language-invariant fixture matrix and a Node coverage validator
next to the survival contract. There is no Go runtime in this repository
yet.
Evidence:
cargo check -p codewhale-tui --locked
cargo clippy -p codewhale-tui --lib --locked -- -A clippy::all -D unused-imports
cargo test -p codewhale-tui --lib --offline --locked -- compaction context_inspector last_round
-> 128 passed; 0 failed
node crates/tui/src/compaction/validate_survival_contract.mjs
-> ok 9 survival-contract fixtures
Refs: #4394
Signed-off-by: CodeWhale Bot <bot@codewhale.net>
06017ae to
74aa1c9
Compare
|
Claude finished @Hmbown's task in 1m 42s —— View job Review complete
SummaryNew commit Previously-reported bug — fixed. let mut candidate = last_user;
loop {
let Some(prev) = last_plain_user_index(messages, candidate) else {
return last_user;
};
if slice_has_tool_result(messages, prev) {
return prev;
}
candidate = prev;
}I traced this by hand against the exact 8-message example from the earlier review ( Since Also cleaned up in this commit:
Remaining minor/non-blocking (from earlier review, unchanged, not required for merge):
Testing note: No new issues found. The correctness bug is resolved and covered by regression tests. |
![codewhale-agent[bot]](https://avatars.githubusercontent.com/u/101357273?s=60&v=4){kind=small}
There was a problem hiding this comment.
Codewhale review
PR 5782 ports the survival contract to main and keeps the last user round, anchors, checkpoint receipt, and billed parent prompt tokens. The Rust builder produces compliant history, but the enforcement validator is weaker than the contract states, and there are a few smaller documentation/i18n concerns.
Findings
- [WARNING] validate_last_round_coverage enforces a weaker floor than the survival contract (
crates/tui/src/compaction/last_round.rs:231)
The function only checks the first user text in last_round, not all user turns; it checks only that some assistant-like message exists, not that the assistant's last-round text or tool_use IDs survive; and it validates tool_result IDs but not tool_use IDs. A replacement that keeps the first user text and an unrelated assistant message while dropping the actual latest user turn or the assistant's tool_use call would pass validation. The JS validator has the same weakness. - [INFO] Placeholder-only checkpoint rejection is documented but not implemented (
crates/tui/src/compaction/last_round.rs:289)
SURVIVAL_CONTRACT.md says validate_survival_contract refuses a placeholder-only checkpoint, but the implementation only counts checkpoint messages and checks anchors; it never inspects the checkpoint text. If a placeholder checkpoint carries the standard marker, it would pass. - [INFO] Hardcoded English summary leaks into localized inspector text (
crates/tui/src/tui/context_inspector.rs:222)
build_context_inspector_text writes 'Last compaction: kept last round verbatim...' directly without a MessageId. This line will appear in English even for non-English locales, unlike the new CtxInspCompaction* rows. - [INFO] Trailing toolless tail can make replacement history unbounded (
crates/tui/src/compaction/last_round.rs:126)
last_round_start walks back to the previous tool-bearing round instead of bounding only the latest user round. In a long chat-only tail after the last tool result, build_replacement_history retains the entire tail verbatim outside the retained_user_messages token budget, which can defeat compaction pressure and grow the cacheable prefix.
Suggestions
-
crates/tui/src/compaction/last_round.rs:231— Validate every plain user text in the last round, not just the first, so a replacement cannot drop the actual latest user turn while keeping an earlier one.for text in last_round.iter().filter_map(user_text_of) { let kept = replacement.iter().any(|message| { user_text_of(message).is_some_and(|kept| { kept == text || text.starts_with(&kept) || kept.starts_with(&text) }) }); if !kept { anyhow::bail!( "Compaction coverage floor: the last user message was dropped; history was not replaced." ); } }
Assessment
Request changes. The main Rust path constructs compliant replacement history, but the coverage floor should be tightened to actually fail closed as the contract describes. The localized inspector also has a small i18n leak.
Advisory review by Codewhale (codewhale review --pr 5782 --post, head 74aa1c97154b0dea3d85643f91036e5f095398ce). Line-specific findings are also posted as inline review comments; mechanical fixes arrive as committable suggestions you can apply from the Files tab. CODEOWNERS approval still governs merge.
| ContentBlock::ToolResult { tool_use_id, .. } if tool_use_id == id | ||
| ) | ||
| }) | ||
| } |
There was a problem hiding this comment.
[WARNING] validate_last_round_coverage enforces a weaker floor than the survival contract
The function only checks the first user text in last_round, not all user turns; it checks only that some assistant-like message exists, not that the assistant's last-round text or tool_use IDs survive; and it validates tool_result IDs but not tool_use IDs. A replacement that keeps the first user text and an unrelated assistant message while dropping the actual latest user turn or the assistant's tool_use call would pass validation. The JS validator has the same weakness.
| let kept = replacement.iter().any(|message| { | ||
| message.content.iter().any(|block| match block { | ||
| ContentBlock::Text { text, .. } => text.contains(needle), | ||
| ContentBlock::ToolResult { content, .. } => content.contains(needle), |
There was a problem hiding this comment.
[INFO] Placeholder-only checkpoint rejection is documented but not implemented
SURVIVAL_CONTRACT.md says validate_survival_contract refuses a placeholder-only checkpoint, but the implementation only counts checkpoint messages and checks anchors; it never inspects the checkpoint text. If a placeholder checkpoint carries the standard marker, it would pass.
| if let Some(kept) = last_round_kept_count(&app.api_messages) { | ||
| let _ = writeln!( | ||
| out, | ||
| "Last compaction: kept last round verbatim ({kept} messages); earlier turns summarized." |
There was a problem hiding this comment.
[INFO] Hardcoded English summary leaks into localized inspector text
build_context_inspector_text writes 'Last compaction: kept last round verbatim...' directly without a MessageId. This line will appear in English even for non-English locales, unlike the new CtxInspCompaction* rows.
| } | ||
|
|
||
| #[must_use] | ||
| pub(crate) fn last_round_start(messages: &[Message]) -> usize { |
There was a problem hiding this comment.
[INFO] Trailing toolless tail can make replacement history unbounded
last_round_start walks back to the previous tool-bearing round instead of bounding only the latest user round. In a long chat-only tail after the last tool result, build_replacement_history retains the entire tail verbatim outside the retained_user_messages token budget, which can defeat compaction pressure and grow the cacheable prefix.
| } | ||
|
|
||
| pub(crate) fn validate_last_round_coverage( | ||
| original: &[Message], | ||
| replacement: &[Message], | ||
| ) -> Result<()> { | ||
| let last_round = last_round_slice(original); | ||
| if last_round.is_empty() { | ||
| return Ok(()); | ||
| } | ||
| if let Some(text) = last_round.iter().find_map(user_text_of) { | ||
| let kept = replacement.iter().any(|message| { |
There was a problem hiding this comment.
Validate every plain user text in the last round, not just the first, so a replacement cannot drop the actual latest user turn while keeping an earlier one.
| } | |
| pub(crate) fn validate_last_round_coverage( | |
| original: &[Message], | |
| replacement: &[Message], | |
| ) -> Result<()> { | |
| let last_round = last_round_slice(original); | |
| if last_round.is_empty() { | |
| return Ok(()); | |
| } | |
| if let Some(text) = last_round.iter().find_map(user_text_of) { | |
| let kept = replacement.iter().any(|message| { | |
| for text in last_round.iter().filter_map(user_text_of) { | |
| let kept = replacement.iter().any(|message| { | |
| user_text_of(message).is_some_and(|kept| { | |
| kept == text || text.starts_with(&kept) || kept.starts_with(&text) | |
| }) | |
| }); | |
| if !kept { | |
| anyhow::bail!( | |
| "Compaction coverage floor: the last user message was dropped; history was not replaced." | |
| ); | |
| } | |
| } |
1 participant
Summary
Closes #4394.
297bd808a(coverage floor) was not onorigin/main(git merge-base --is-ancestor 297bd808a origin/mainfailed; no#4394commits on main). This ports the contract onto current main rather than cherry-picking the stale integration commit.crates/tui/src/compaction/SURVIVAL_CONTRACT.mdplus a typed field table insurvival_contract.rs./anchortext, or the checkpoint receipt would vanish./contextnames the compaction path (summary/prune-only) and/anchorsurvival.Testing
cargo test -p codewhale-tui --lib --offline --locked -- compaction context_inspector last_round→ 125 passed; 0 failedmessage_id_list_english_pack_stay_in_exact_sync,shipped_complete_packs_have_raw_key_parity_with_englishcargo fmt --all -- --check(touched Rust files formatted withrustfmt --edition 2024)cargo clippy --workspace --all-targets --all-features --lockedcargo test --workspace --all-features --lockedChecklist
SURVIVAL_CONTRACT.md)