Skip to content
76 changes: 72 additions & 4 deletions src/openhuman/agent/harness/archivist/recap.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ use super::types::ArchivistHook;
use crate::openhuman::memory::api::provider::{
ConversationSegment, EpisodicTurn, SummaryContext, SummaryInput,
};
use std::time::Duration;
// The fold itself is `MemoryTree::summarise` now, so the DTOs are the
// contract's owned ones and `tree_kind` is the wire string the driver
// validates rather than the engine's `TreeKind` enum (#5560).
Expand All @@ -28,6 +29,34 @@ const INPUT_TOKEN_BUDGET: u32 = 50_000;
/// See [`INPUT_TOKEN_BUDGET`] for why this is a copy.
const SUMMARY_OVERHEAD_RESERVE_TOKENS: u32 = 2_048;

/// How long a segment recap may take before the turn stops waiting for it.
///
/// This bound is **reasoned, not measured** — say so plainly, because the
/// number should be re-tuned the moment someone has real folds to look at.
/// The attempt to measure it (#6200) produced nothing usable: the workspace it
/// ran in had no summariser configured, so every fold short-circuited to the
/// heuristic without a network call.
///
/// Three things set it:
///
/// - **600 s is what it replaces.** `tinyinference`'s request default is the
/// only bound under this call today, and `ChatPrompt` carries no per-call
/// override, so a summariser that accepts a connection and then goes silent
/// holds the turn for ten minutes.
/// - **It must clear two attempts.** `tinymemory`'s retry gives up on a further
/// attempt once ~20 s have elapsed, so its realistic worst case is two folds
/// back to back. A deadline under that would cut the chain before the second
/// attempt and quietly turn the retry into dead code.
/// - **Overshooting is cheap now, undershooting is not.** Since #6156 a fold
/// that misses this writes nothing and leaves the segment `'closed'` for the
/// recovery pass, so a healthy-but-slow fold is deferred rather than lost.
/// Cutting a good fold short only churns.
///
/// Compare `AUTO_RECALL_BUDGET` (5 s), which bounds a *retrieval* on the same
/// turn path and was set from a field measurement. A fold is an LLM generation
/// over up to `INPUT_TOKEN_BUDGET` tokens, so it is not the same kind of wait.
const RECAP_DEADLINE: Duration = Duration::from_secs(90);

/// Fold one segment's corpus through the **guarded** driver's tree family.
///
/// The guard rather than the archivist's own provider handle, because
Expand Down Expand Up @@ -268,27 +297,66 @@ impl ArchivistHook {
"[archivist] summarize_entries: LLM recap segment={segment_id} entries={}",
entries.len()
);
let summary_result = fold_through_driver(&corpus_inputs, &summary_ctx).await;
// #6200: bounded because this await is on the turn path — the
// caller's `flush_open_segment` runs before a turn returns — and
// the only bound under it otherwise is tinyinference's 600 s
// request default. Wrapping here rather than passing a timeout
// down keeps it host-side: `ChatPrompt` has no timeout field, so
// the alternative is a `tinymemory` contract change, a release
// and a re-pin. It also bounds the whole retry chain rather than
// a single attempt, which is the thing the turn actually waits
// on. `elapsed_ms` rides every arm so the number above can be
// re-tuned from real folds.
let started = std::time::Instant::now();
let summary_result = match tokio::time::timeout(
RECAP_DEADLINE,
fold_through_driver(&corpus_inputs, &summary_ctx),
)
.await
{
Ok(result) => result,
Err(_) => {
let elapsed_ms = started.elapsed().as_millis();
// WARN, not debug: a fold that outlasts the deadline
// held the turn for the whole of it, and that is the
// symptom worth finding in a log. Falls through to
// the same `(bookend, false)` the error arm returns,
// so nothing is persisted and the segment keeps the
// marker a later pass selects on.
tracing::warn!(
"[archivist] summarize_entries: recap exceeded {:?} \
elapsed_ms={elapsed_ms} — segment={segment_id} left \
unsummarised for a later pass",
RECAP_DEADLINE
);
Comment thread
YellowSnnowmann marked this conversation as resolved.
return (
super::boundary::fallback_summary(first, last, turn_count),
false,
);
}
};
let elapsed_ms = started.elapsed().as_millis();

match summary_result {
Ok(output) if !output.content.is_empty() => {
tracing::debug!(
"[archivist] summarize_entries: LLM recap ok segment={segment_id} \
chars={}",
chars={} elapsed_ms={elapsed_ms}",
output.content.len()
);
return (output.content, true);
}
Ok(_) => {
tracing::debug!(
"[archivist] summarize_entries: LLM returned empty — \
heuristic fallback segment={segment_id}"
heuristic fallback segment={segment_id} elapsed_ms={elapsed_ms}"
);
}
Err(e) => {
tracing::warn!(
"[archivist] summarize_entries: LLM recap failed (non-fatal) \
segment={segment_id}: {e} — heuristic fallback"
segment={segment_id} elapsed_ms={elapsed_ms}: {e} — \
heuristic fallback"
);
}
}
Expand Down
25 changes: 25 additions & 0 deletions src/openhuman/agent/harness/archivist/recap_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -59,3 +59,28 @@ fn segment_membership_falls_back_to_episodic_id() {
assert!(entry(None, Some(25), 101.0).is_in_segment(&segment));
assert!(!entry(None, Some(26), 100.001).is_in_segment(&segment));
}

/// The recap deadline clears two of `tinymemory`'s retry attempts.
///
/// Not a behavioural test — an arithmetic one, and it exists because the two
/// numbers live in different repositories and nothing else would catch them
/// drifting apart. `tinymemory`'s retry stops starting further attempts once
/// ~20 s have elapsed, so its realistic worst case is two folds back to back.
/// A `RECAP_DEADLINE` under that would cut the chain before the second attempt
/// and silently turn the retry into dead code — the failure this pins.
#[test]
fn the_recap_deadline_leaves_room_for_the_drivers_retry() {
const TINYMEMORY_RETRY_CEILING: Duration = Duration::from_secs(20);

assert!(
RECAP_DEADLINE > TINYMEMORY_RETRY_CEILING * 2,
"RECAP_DEADLINE ({RECAP_DEADLINE:?}) must clear two attempts of the \
driver's {TINYMEMORY_RETRY_CEILING:?} retry window, or the retry can \
never fire a second attempt"
);
assert!(

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

priority medium tests likely

Hardcode the constant reference in the upper-bound assertion

The test uses Duration::from_secs(600) as the reference for the upper bound, but the only reason 600 s is relevant is that tinyinference's default is 600 s. If that default ever changes, this test silently becomes a weaker guard. The same file's RECAP_DEADLINE doc comment already says '600 s is what it replaces' — export that constant or define a named constant alongside the deadline so a future reader can see the two are meant to track each other.

[RULE] brittle-assertion ·

RECAP_DEADLINE < Duration::from_secs(600),
"RECAP_DEADLINE must stay well under tinyinference's 600s request \
default, which is the bound it exists to replace"
);
}
28 changes: 21 additions & 7 deletions src/openhuman/agent/harness/session/builder/helpers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ pub(super) fn add_memory_prompt_sections(
) -> SystemPromptBuilder {
use crate::openhuman::agent::learning::{
any_tool_offered, MemoryAccessSection, MemoryWriteSection, MEMORY_READ_TOOLS,
MEMORY_STORE_TOOL, SAVE_PREFERENCE_TOOL,
MEMORY_STORE_TOOL, MEMORY_WRITE_DELEGATE_TOOL, SAVE_PREFERENCE_TOOL,
};
let mut prompt_builder = prompt_builder;
if any_tool_offered(&MEMORY_READ_TOOLS, tools, delegation_tools, visible) {
Expand All @@ -138,17 +138,31 @@ pub(super) fn add_memory_prompt_sections(
// about the other (review finding).
let preferences = any_tool_offered(&[SAVE_PREFERENCE_TOOL], tools, delegation_tools, visible);
let facts = any_tool_offered(&[MEMORY_STORE_TOOL], tools, delegation_tools, visible);
if preferences || facts {
prompt_builder =
prompt_builder.add_section(Box::new(MemoryWriteSection::new(preferences, facts)));
// #6200: asked for as well as the pair, not instead of it. An agent whose
// only write path is the delegate held the tool and no rule about using it
// — the write-side twin of the read-side gap #6183 closed.
let delegate = any_tool_offered(
&[MEMORY_WRITE_DELEGATE_TOOL],
tools,
delegation_tools,
visible,
);
if preferences || facts || delegate {
prompt_builder = prompt_builder.add_section(Box::new(MemoryWriteSection::new(
preferences,
facts,
delegate,
)));
log::debug!(
"[memory_write] prompt section registered for agent={agent_id} \
save_preference={preferences} memory_store={facts}"
save_preference={preferences} memory_store={facts} \
manage_profile_memory={delegate}"
);
} else {
log::debug!(
"[memory_write] skipping MemoryWriteSection — neither memory_store nor \
save_preference is registered+visible for agent={agent_id}"
"[memory_write] skipping MemoryWriteSection — none of memory_store, \
save_preference or manage_profile_memory is registered+visible for \
agent={agent_id}"
);
}
prompt_builder
Expand Down
27 changes: 20 additions & 7 deletions src/openhuman/agent/harness/session/turn/core_turn.rs
Original file line number Diff line number Diff line change
Expand Up @@ -673,13 +673,26 @@ impl Agent {
// archivist sub-agent that will distil durable facts into the
// workspace MEMORY.md file via the `update_memory_md` tool.
//
// The spawn is fire-and-forget: the main turn returns the
// user-visible response immediately, and the archivist runs
// asynchronously on the `agentic` tier. We optimistically mark
// the extraction complete right away — if it actually fails,
// we'll just retry on the next threshold window (a few turns
// later), which is the right amount of retry behaviour for a
// librarian task that's idempotent across reruns.
// The archivist sub-agent itself is spawned and runs asynchronously
// on the `agentic` tier. We optimistically mark the extraction
// complete right away — if it actually fails, we'll just retry on the
// next threshold window (a few turns later), which is the right amount
// of retry behaviour for a librarian task that's idempotent across
// reruns.
//
// This call is NOT fire-and-forget, despite spawning one (#6200). It
// is awaited, and before it spawns anything it awaits
// `flush_open_segment`, so the trailing segment's recap runs on this
// path — `result` below is returned only afterwards. The comment here
// used to claim the turn returned immediately; it did not, and with
// `tinyinference`'s 600 s request default underneath that was up to ten
// minutes of a held-open turn. `RECAP_DEADLINE` in `archivist::recap`
// is what bounds it now.
//
// Detaching the flush instead would match the old comment, but it would
// drop the `GUARANTEE:` documented at the flush site — that the
// trailing segment always receives its recap before wind-down. Bounding
// the wait keeps that promise and removes the hazard.
if result.is_ok() && self.context.should_extract_session_memory() {
self.spawn_session_memory_extraction(session_memory_parent_context)
.await;
Expand Down
3 changes: 2 additions & 1 deletion src/openhuman/agent/learning/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,8 @@ pub use profile_md_renderer::ProfileMdRenderer;
pub use prompt_sections::{
any_tool_offered, load_learned_from_cache, memory_write_instruction, LearnedContextSection,
MemoryAccessSection, MemoryWriteSection, UserProfileSection, MEMORY_ACCESS_INSTRUCTION,
MEMORY_READ_TOOLS, MEMORY_STORE_TOOL, MEMORY_WRITE_TOOLS, SAVE_PREFERENCE_TOOL,
MEMORY_READ_TOOLS, MEMORY_STORE_TOOL, MEMORY_WRITE_DELEGATE_TOOL, MEMORY_WRITE_TOOLS,
SAVE_PREFERENCE_TOOL,
};
pub use reflection::ReflectionHook;
pub use schemas::{
Expand Down
68 changes: 61 additions & 7 deletions src/openhuman/agent/learning/prompt_sections.rs
Original file line number Diff line number Diff line change
Expand Up @@ -155,14 +155,19 @@ impl PromptSection for MemoryAccessSection {
pub struct MemoryWriteSection {
preferences: bool,
facts: bool,
delegate: bool,
}

impl MemoryWriteSection {
/// `preferences` = `save_preference` is offered here, `facts` =
/// `memory_store` is.
/// `memory_store` is, `delegate` = [`MEMORY_WRITE_DELEGATE_TOOL`] is.
#[must_use]
pub fn new(preferences: bool, facts: bool) -> Self {
Self { preferences, facts }
pub fn new(preferences: bool, facts: bool, delegate: bool) -> Self {
Self {
preferences,
facts,
delegate,
}
}
}

Expand All @@ -173,11 +178,27 @@ impl MemoryWriteSection {
/// Empty when neither tool is offered — which is also when nothing registers
/// the section, so the empty string is a guard, not a path in normal use.
#[must_use]
pub fn memory_write_instruction(preferences: bool, facts: bool) -> String {
pub fn memory_write_instruction(preferences: bool, facts: bool, delegate: bool) -> String {
let route = match (preferences, facts) {
(true, true) => "— `save_preference` for preferences, `memory_store` for everything else",
(true, false) => "with `save_preference`",
(false, true) => "with `memory_store`",
// Only when neither direct tool is held, so an agent that has one
// renders exactly the text it rendered before this arm existed.
//
// `blocking: true` is not a stylistic detail, it is what makes the
// sentence above true (#6200 review). `ArchetypeDelegationTool`
// defaults an omitted `blocking` to `false` and dispatches
// `PreferAsync`, which hands back an immediate reference while the
// worker runs later — so a model told merely to "write with
// `manage_profile_memory`" would confirm a save that had not happened,
// which is the #6048 bug arriving by a new route. The argument is
// advertised on the tool's own schema, so this is a demand the model
// can actually satisfy.
(false, false) if delegate => {
"with `manage_profile_memory` (pass `blocking: true` so the write \
gates your reply)"
}
(false, false) => return String::new(),
};
format!(
Expand All @@ -195,7 +216,11 @@ impl PromptSection for MemoryWriteSection {
}

fn build(&self, _ctx: &PromptContext<'_>) -> Result<String> {
Ok(memory_write_instruction(self.preferences, self.facts))
Ok(memory_write_instruction(
self.preferences,
self.facts,
self.delegate,
))
}
}

Expand All @@ -220,8 +245,37 @@ pub const SAVE_PREFERENCE_TOOL: &str = "save_preference";
/// The tool every other remembered fact is written through.
pub const MEMORY_STORE_TOOL: &str = "memory_store";

/// The writing tools [`MemoryWriteSection`] is keyed on.
pub const MEMORY_WRITE_TOOLS: [&str; 2] = [MEMORY_STORE_TOOL, SAVE_PREFERENCE_TOOL];
/// The delegate an agent writes through when it holds neither direct write
/// tool.
///
/// Synthesised from `profile_memory_agent`'s `delegate_name`, and the write-side
/// counterpart of `retrieve_memory` in [`MEMORY_READ_TOOLS`]. The orchestrator
/// is configured this way: its visible set carries this delegate and neither
/// [`MEMORY_STORE_TOOL`] nor [`SAVE_PREFERENCE_TOOL`], so keying the section on
/// the direct pair alone dropped the rule for the agent that needed it most —
/// the #6048 case, "got it, saved" with no tool call behind it.
///
/// The section's promise survives the indirection **only under a blocking
/// delegation**. `profile_memory_agent` holds both direct tools, so a delegated
/// write reaches the same store — but `ArchetypeDelegationTool` defaults to an
/// async dispatch that returns before the worker runs, so the route text demands
/// `blocking: true`. Without that the parent could confirm a save that had not
/// happened yet, which is exactly the bug this section exists to prevent.
pub const MEMORY_WRITE_DELEGATE_TOOL: &str = "manage_profile_memory";

/// The writing routes [`MemoryWriteSection`] is keyed on, delegate included.
///
/// Mirrors [`MEMORY_READ_TOOLS`], which lists `retrieve_memory` beside its two
/// direct tools for the same reason. The live gate in `add_memory_prompt_sections`
/// asks about each of these separately rather than reading this array — the
/// section names the route it found, so it cannot treat them interchangeably —
/// but a reader reaching for "what does the write section care about" should get
/// the whole answer here (#6200 review).
pub const MEMORY_WRITE_TOOLS: [&str; 3] = [
MEMORY_STORE_TOOL,
SAVE_PREFERENCE_TOOL,
MEMORY_WRITE_DELEGATE_TOOL,
];

/// Whether any of `names` is registered on this session **and** survives tool
/// filtering.
Expand Down
Loading
Loading