mirror of
https://github.com/fabro-sh/fabro.git
synced 2026-10-10 03:30:59 +00:00
fix(agent): refuse to truncate history on a degenerate compaction summary
When the summarization LLM call returned an empty completion, compaction truncated the conversation anyway. `history.compact_from` discarded the summarized turns irreversibly, `CompactionCompleted` was emitted as if nothing had gone wrong, and the replacement system turn contained only the handoff preamble: "A different assistant began this task and produced the following summary" followed by nothing. The agent then continued with zero context while having been explicitly told a handoff summary existed. It presents to a user as the agent suddenly forgetting everything, and the only trace was a `debug!` line that is off by default, so there was nothing in production logs to correlate against. This is provider-independent. Any completion that comes back empty triggers it: a truncated stream, a reasoning model that spends its whole token budget on reasoning, or a rate-limit edge. Validate the summary before mutating history. A summary that is empty, whitespace-only, or shorter than 32 bytes after trimming is refused: the history is left fully intact and an error is returned instead. The threshold is deliberately far below any genuine summary — 32 bytes is shorter than a single source file path — because this guards against degenerate responses, not summary quality, and a false refusal would let the context keep growing. Structure is not validated, since a model may legitimately vary the requested section format. Returning `Err` is sufficient to surface the failure. `compact_if_needed` already converts it into an `AgentEvent::Error`, which lands in the run event stream and logs at ERROR via `AgentEvent::trace`, and the session continues rather than dying — behavior already covered by `compaction_failure_is_non_fatal`. The canned summary in `compaction_includes_structured_prompt_and_file_tracking` was 26 bytes, which the new guard rejects. That test verifies the summarization request prompt and file tracking, not minimum summary length, so its fixture is now a realistic summary. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
8921fc533f
commit
b17b8aeaed
2 changed files with 157 additions and 3 deletions
|
|
@ -13,6 +13,17 @@ use crate::types::{AgentEvent, Message};
|
|||
|
||||
const APPROX_CHARS_PER_TOKEN: usize = 4;
|
||||
|
||||
/// Minimum length, in bytes, of a usable compaction summary after trimming.
|
||||
///
|
||||
/// A summary is traded for many turns of conversation, so anything shorter
|
||||
/// than a single source file path
|
||||
/// (`lib/components/fabro-agent/src/compaction.rs` is 43 bytes) cannot be
|
||||
/// carrying that context forward. The bar is set far below any genuine summary
|
||||
/// on purpose: this exists to catch degenerate responses, not to judge summary
|
||||
/// quality. A false refusal leaves the context to keep growing, so the check
|
||||
/// must never fire on a real summary.
|
||||
const MIN_SUMMARY_LEN: usize = 32;
|
||||
|
||||
#[derive(Debug, Clone, Copy, PartialEq, Eq, strum::IntoStaticStr)]
|
||||
#[strum(serialize_all = "snake_case")]
|
||||
pub(crate) enum ContextEstimateMethod {
|
||||
|
|
@ -149,7 +160,25 @@ function names, error messages, and exact values. Omit pleasantries and conversa
|
|||
.await
|
||||
.map_err(Error::Llm)?;
|
||||
|
||||
let summary_text = response.text();
|
||||
let response_text = response.text();
|
||||
let summary_text = response_text.trim();
|
||||
|
||||
// Refuse to compact on a degenerate summary. `compact_from` discards the
|
||||
// summarized turns irreversibly, so an empty or near-empty summary must not
|
||||
// be traded for them: the preamble below would tell the model a handoff
|
||||
// summary exists while it actually runs with no history at all. Empty
|
||||
// completions are provider-independent — a truncated stream, a reasoning
|
||||
// model that spent its whole budget on reasoning, or a rate-limit edge all
|
||||
// produce one. Returning here leaves the history intact; the caller turns
|
||||
// this into an `AgentEvent::Error` and continues the session.
|
||||
if summary_text.len() < MIN_SUMMARY_LEN {
|
||||
return Err(Error::InvalidState(format!(
|
||||
"compaction summary was empty or too short to replace {preserve_start} turns \
|
||||
({} bytes, minimum {MIN_SUMMARY_LEN}); history left intact",
|
||||
summary_text.len()
|
||||
)));
|
||||
}
|
||||
|
||||
debug!(
|
||||
summary_len = summary_text.len(),
|
||||
"Compaction summary generated"
|
||||
|
|
@ -298,6 +327,7 @@ pub fn render_turns_for_summary(turns: &[Message]) -> String {
|
|||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use std::sync::Arc;
|
||||
use std::time::SystemTime;
|
||||
|
||||
use fabro_llm::types::{TokenCounts, ToolCall, ToolResult};
|
||||
|
|
@ -305,7 +335,7 @@ mod tests {
|
|||
use super::*;
|
||||
use crate::event::Emitter;
|
||||
use crate::history::History;
|
||||
use crate::test_support::TestProfile;
|
||||
use crate::test_support::{MockLlmProvider, TestProfile, make_client, text_response};
|
||||
use crate::tool_registry::ToolRegistry;
|
||||
use crate::types::Message;
|
||||
|
||||
|
|
@ -570,4 +600,126 @@ mod tests {
|
|||
assert!(matches!(event.event, AgentEvent::Warning { details, .. }
|
||||
if details["estimate_method"] == "local_estimate"));
|
||||
}
|
||||
|
||||
/// Run `compact_context` over a fixed four-turn history against a mock
|
||||
/// provider that returns `summary` from the summarization call.
|
||||
async fn compact_with_summary(summary: &str) -> (Result<(), Error>, History, Vec<AgentEvent>) {
|
||||
let mut history = History::default();
|
||||
for index in 0..4 {
|
||||
history.push(Message::User {
|
||||
content: format!("message {index}"),
|
||||
timestamp: SystemTime::now(),
|
||||
});
|
||||
}
|
||||
|
||||
let provider = Arc::new(MockLlmProvider::new(vec![text_response(summary)]));
|
||||
let client = make_client(provider).await;
|
||||
let profile = TestProfile::new();
|
||||
let file_tracker = FileTracker::default();
|
||||
let emitter = Emitter::new();
|
||||
let mut rx = emitter.subscribe();
|
||||
|
||||
let result = compact_context(
|
||||
&mut history,
|
||||
&client,
|
||||
&profile,
|
||||
&file_tracker,
|
||||
1,
|
||||
ContextEstimate {
|
||||
tokens: 1_000,
|
||||
method: ContextEstimateMethod::LocalEstimate,
|
||||
},
|
||||
&emitter,
|
||||
"sess",
|
||||
)
|
||||
.await;
|
||||
|
||||
let mut events = Vec::new();
|
||||
while let Ok(event) = rx.try_recv() {
|
||||
events.push(event.event);
|
||||
}
|
||||
|
||||
(result, history, events)
|
||||
}
|
||||
|
||||
fn assert_history_untouched(history: &History) {
|
||||
assert_eq!(
|
||||
history.turns().len(),
|
||||
4,
|
||||
"history must not be truncated when the summary is rejected"
|
||||
);
|
||||
assert!(
|
||||
history
|
||||
.turns()
|
||||
.iter()
|
||||
.all(|turn| matches!(turn, Message::User { .. })),
|
||||
"no summary turn should be inserted when the summary is rejected"
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn compaction_refuses_to_truncate_on_empty_summary() {
|
||||
let (result, history, events) = compact_with_summary("").await;
|
||||
|
||||
let err = result.expect_err("empty summary must not report success");
|
||||
assert!(
|
||||
matches!(&err, Error::InvalidState(message) if message.contains("empty or too short")),
|
||||
"unexpected error: {err}"
|
||||
);
|
||||
|
||||
assert_history_untouched(&history);
|
||||
assert!(
|
||||
!events
|
||||
.iter()
|
||||
.any(|event| matches!(event, AgentEvent::CompactionCompleted { .. })),
|
||||
"CompactionCompleted must not be emitted for a rejected summary"
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn compaction_refuses_to_truncate_on_whitespace_only_summary() {
|
||||
let (result, history, events) = compact_with_summary(" \n\t \n ").await;
|
||||
|
||||
let err = result.expect_err("whitespace-only summary must not report success");
|
||||
assert!(
|
||||
matches!(&err, Error::InvalidState(message) if message.contains("empty or too short")),
|
||||
"unexpected error: {err}"
|
||||
);
|
||||
|
||||
assert_history_untouched(&history);
|
||||
assert!(
|
||||
!events
|
||||
.iter()
|
||||
.any(|event| matches!(event, AgentEvent::CompactionCompleted { .. })),
|
||||
"CompactionCompleted must not be emitted for a rejected summary"
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn compaction_replaces_history_on_normal_summary() {
|
||||
let (result, history, events) = compact_with_summary(
|
||||
"## Goal\nAdd a compaction guard.\n\n## Next Steps\nRun the test suite.",
|
||||
)
|
||||
.await;
|
||||
|
||||
result.expect("a normal summary should compact");
|
||||
|
||||
let summary_turn = history
|
||||
.turns()
|
||||
.iter()
|
||||
.find_map(|turn| match turn {
|
||||
Message::System { content, .. } => Some(content),
|
||||
_ => None,
|
||||
})
|
||||
.expect("compacted history should contain a summary turn");
|
||||
assert!(summary_turn.contains("A different assistant began this task"));
|
||||
assert!(summary_turn.contains("Add a compaction guard."));
|
||||
|
||||
assert!(
|
||||
events
|
||||
.iter()
|
||||
.any(|event| matches!(event, AgentEvent::CompactionCompleted { .. })),
|
||||
"CompactionCompleted should be emitted on success"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -4690,7 +4690,9 @@ mod tests {
|
|||
|
||||
async fn complete(&self, request: &Request) -> Result<Response, LlmError> {
|
||||
*self.captured_complete.lock().unwrap() = Some(request.clone());
|
||||
Ok(text_response("## Goal\nSummary goes here."))
|
||||
Ok(text_response(
|
||||
"## Goal\nSummary goes here.\n\n## Progress\nRead /src/main.rs.",
|
||||
))
|
||||
}
|
||||
|
||||
async fn stream(&self, _request: &Request) -> Result<StreamEventStream, LlmError> {
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue