Skip to content

Remove dead rolling recap helper #4572

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ahfoysal wants to merge 2 commits into
base: main
Choose a base branch
from
+3 −128
Open

Remove dead rolling recap helper #4572

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ba5a8d5
Remove dead rolling recap helper
ahfoysal Jul 5, 2026
3820f07
test: remove stale rolling recap assertions
ahfoysal Jul 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
129 changes: 3 additions & 126 deletions src/openhuman/agent/harness/archivist/recap.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
//! Summarization and rolling recap logic for `ArchivistHook`.
//! Segment summarization logic for `ArchivistHook`.

use super::types::ArchivistHook;
use crate::openhuman::memory_store::fts5::{self, EpisodicEntry};
Expand Down Expand Up @@ -51,9 +51,7 @@ impl ArchivistHook {
fts5::episodic_session_entries(conn, session_id).unwrap_or_default()
}

/// Shared summarize helper — the **single LLM summarizer** used by both
/// the finalize path (`on_segment_closed`) and the rolling-recap path
/// (`rolling_segment_recap`).
/// Shared summarize helper used by the finalize path (`on_segment_closed`).

@chatgpt-codex-connector chatgpt-codex-connector Bot Jul 5, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Restore the rolling recap API or update its tests

Deleting rolling_segment_recap leaves integration tests that still call it (tests/agent_archivist_debug_round21_raw_coverage_e2e.rs:321 and :371), so cargo test --test agent_archivist_debug_round21_raw_coverage_e2e will fail to compile with “no method named rolling_segment_recap”. If the helper is truly dead, remove or rewrite those assertions in this same change; otherwise keep the API.

Useful? React with 👍 / 👎.

///
/// Builds a prose corpus from `entries`, calls the `LlmSummariser` when a
/// `chat_provider` is configured, and falls back to the heuristic
Expand All @@ -69,10 +67,7 @@ impl ArchivistHook {
/// Returns `(text, produced_by_llm)`. `produced_by_llm == false` means the
/// LLM was unavailable / failed / returned empty and `text` is the shallow
/// heuristic `fallback_summary` bookend stub. That stub is an acceptable
/// durable last-resort on the *finalize* path, but callers driving the
/// **live prompt** (rolling recap → compaction) must treat
/// `produced_by_llm == false` as "no real recap" and fall back to their
/// own strategy — the stub must never become live compaction text.
/// durable last-resort on the finalize path.
pub(super) async fn summarize_entries(
&self,
entries: &[&EpisodicEntry],
Expand Down Expand Up @@ -177,122 +172,4 @@ impl ArchivistHook {
}
(segments::fallback_summary(first, last, turn_count), false)
}

@M3gA-Mind M3gA-Mind Jul 9, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocker: build break. Removing rolling_segment_recap leaves two dangling callers in tests/agent_archivist_debug_round21_raw_coverage_e2e.rs (L321 and L371), both assert_eq!(...rolling_segment_recap(...).await, None). cargo test will fail to compile the archivist e2e test crate.

Delete those two assertions along with the method (or port them to a surviving API), then run cargo test --test agent_archivist_debug_round21_raw_coverage_e2e once submodules are initialized to confirm green. A repo-wide rg -n rolling_segment_recap (not scoped to src/) surfaces both call sites.


/// Produce a rolling recap of the **currently-open** segment for
/// `session_id` WITHOUT closing it, writing `segment_set_summary`, or
/// embedding.
///
/// This is the Phase 1.5 "one summarizer" entry point. Both
/// `on_segment_closed` (finalize) and this function delegate to the same
/// [`Self::summarize_entries`] helper so the same LLM path is used in both
/// cases. The distinction is purely in what happens *after* the summary
/// string is produced:
///
/// - **Finalize** (`on_segment_closed`): persists the summary via
/// `segment_set_summary`, embeds it, extracts events, pipes tree ingest.
/// - **Rolling** (this function): returns the summary string and does
/// nothing else — segment stays open, DB is untouched.
///
/// Returns `None` when:
/// - The archivist is disabled or has no connection.
/// - There is no open segment for `session_id`.
/// - The open segment has no episodic entries.
/// - No real LLM recap was produced (LLM unavailable / failed / empty, so
/// only the heuristic bookend stub is available). The shallow stub is
/// deliberately NOT used as live compaction text.
///
/// Callers must treat `None` as "recap unavailable" and fall back to
/// their own compaction strategy (e.g. `ProviderSummarizer`).
pub async fn rolling_segment_recap(&self, session_id: &str) -> Option<String> {
if !self.enabled {
tracing::debug!(
"[archivist] rolling_segment_recap: archivist disabled \
session={session_id} — returning None"
);
return None;
}
let conn = self.conn.as_ref()?;

// Find the currently-open segment for this session.
let open_segment = match crate::openhuman::memory_store::segments::open_segment_for_session(
conn, session_id,
) {
Ok(Some(seg)) => seg,
Ok(None) => {
tracing::debug!(
"[archivist] rolling_segment_recap: no open segment for \
session={session_id} — returning None"
);
return None;
}
Err(e) => {
tracing::warn!(
"[archivist] rolling_segment_recap: failed to query open segment \
session={session_id}: {e} — returning None"
);
return None;
}
};

// Gather the episodic entries for this session so far.
let all_entries = self.read_session_entries(conn, session_id);

// Keep only entries within the open segment's time window (start →
// now, inclusive). An open segment has `end_timestamp = None`.
let segment_entries: Vec<&EpisodicEntry> = all_entries
.iter()
.filter(|e| e.timestamp >= open_segment.start_timestamp)
.collect();

if segment_entries.is_empty() {
tracing::debug!(
"[archivist] rolling_segment_recap: no entries in open segment={} \
session={session_id} — returning None",
open_segment.segment_id
);
return None;
}

tracing::debug!(
"[archivist] rolling_segment_recap: summarizing open segment={} \
entries={} session={session_id}",
open_segment.segment_id,
segment_entries.len()
);

let (recap, from_llm) = self
.summarize_entries(
&segment_entries,
&open_segment.segment_id,
open_segment.turn_count,
)
.await;

if !from_llm {
tracing::debug!(
"[archivist] rolling_segment_recap: only heuristic bookend stub \
available (no real LLM recap) session={session_id} segment={} — \
returning None",
open_segment.segment_id
);
return None;
}

if recap.is_empty() {
tracing::debug!(
"[archivist] rolling_segment_recap: summarize_entries returned empty \
session={session_id} segment={} — returning None",
open_segment.segment_id
);
return None;
}

tracing::debug!(
"[archivist] rolling_segment_recap: produced LLM recap chars={} \
session={session_id} segment={}",
recap.len(),
open_segment.segment_id
);
Some(recap)
}
}
2 changes: 0 additions & 2 deletions tests/agent_archivist_debug_round21_raw_coverage_e2e.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -318,7 +318,6 @@ async fn archivist_flush_finalizes_open_segment_and_extracts_profile_events() ->

let open_before = segments::open_segment_for_session(&conn, session)?;
assert!(open_before.is_some());
assert_eq!(hook.rolling_segment_recap(session).await, None);

hook.flush_open_segment(session).await;

Expand Down Expand Up @@ -368,7 +367,6 @@ async fn archivist_disabled_and_unknown_session_paths_are_noops() -> Result<()>
assert!(fts5::episodic_session_entries(&conn, "unknown")?.is_empty());
let enabled = ArchivistHook::new(conn, true);
enabled.flush_open_segment("missing-session").await;
assert_eq!(enabled.rolling_segment_recap("missing-session").await, None);
Ok(())
}

Expand Down