Repository navigation
fix(lifecycle): a resumed pre-turn leads with the thread in one pack #206
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -648,3 +648,144 @@ async fn core_build_consolidates_exactly_the_core_node() { | |
| assert_eq!(request.reach, Reach::exact(acme())); | ||
| assert!(request.kinds.is_empty()); | ||
| } | ||
|
|
||
| // ── a resumed pre-turn: one pack, one budget (openhuman#7023) ─────────────── | ||
|
|
||
| /// Logs `turns` exchanges on `thread`, each with distinct text. | ||
| async fn log_exchanges(memory: &AgentMemory, thread: &str, turns: u32) { | ||
| for turn in 0..turns { | ||
| memory | ||
| .pre_turn(PreTurn::new( | ||
| thread, | ||
| turn * 2, | ||
| format!("leg {turn}: how far is Porto"), | ||
| )) | ||
| .await | ||
| .unwrap(); | ||
| memory | ||
| .post_turn(PostTurn::new( | ||
| thread, | ||
| turn * 2 + 1, | ||
| format!("Leg {turn} is about 310 km."), | ||
| )) | ||
| .await | ||
| .unwrap(); | ||
| } | ||
| } | ||
|
|
||
| fn bullets(markdown: &str) -> Vec<&str> { | ||
| markdown | ||
| .lines() | ||
| .filter(|line| line.trim_start().starts_with("- ")) | ||
| .collect() | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn a_resumed_pre_turn_leads_with_the_thread_in_one_pack_and_repeats_nothing() { | ||
| let engine = Arc::new(ReferenceEngine::new()); | ||
| let memory = memory(&engine, "assistant"); | ||
| engine | ||
| .store(StoreItem::learning( | ||
| "The user prefers metric units", | ||
| LearningKind::Preference, | ||
| 0.9, | ||
| MemoryMeta::default(), | ||
| )) | ||
| .await | ||
| .unwrap(); | ||
| log_exchanges(&memory, "t1", 3).await; | ||
|
|
||
| let resumed = memory | ||
| .pre_turn_resumed(PreTurn { | ||
| in_prompt_from: 4, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Preserve compatibility for existing PreTurn literals
[RULE] api-compatibility ·
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. False positive: |
||
| ..PreTurn::new("t1", 6, "which units does the user prefer for the distance") | ||
| }) | ||
| .await | ||
| .unwrap() | ||
| .pack; | ||
| let markdown = &resumed.markdown; | ||
| assert!(markdown.contains(THREAD_HEADING), "{markdown}"); | ||
| assert!(markdown.contains("metric units"), "{markdown}"); | ||
| assert!( | ||
| markdown.contains("leg 0"), | ||
| "an older turn leads: {markdown}" | ||
| ); | ||
| assert!( | ||
| !markdown.contains("leg 2:"), | ||
| "turns in the prompt stay out: {markdown}" | ||
| ); | ||
| let lines = bullets(markdown); | ||
| let unique: std::collections::HashSet<&str> = lines.iter().copied().collect(); | ||
| assert_eq!( | ||
| lines.len(), | ||
| unique.len(), | ||
| "a line was injected twice:\n{markdown}" | ||
| ); | ||
| assert!(resumed.tokens <= RecallPolicy::default().budget_tokens); | ||
|
|
||
| // An ordinary pre-turn has no thread section, as before. | ||
| let plain = memory | ||
| .pre_turn(PreTurn { | ||
| in_prompt_from: 4, | ||
| ..PreTurn::new("t1", 8, "which units does the user prefer for the distance") | ||
| }) | ||
| .await | ||
| .unwrap() | ||
| .pack; | ||
| assert!( | ||
| !plain.markdown.contains(THREAD_HEADING), | ||
| "{}", | ||
| plain.markdown | ||
| ); | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn a_resumed_pre_turn_honours_a_zero_history_limit() { | ||
| let engine = Arc::new(ReferenceEngine::new()); | ||
| let memory = memory(&engine, "assistant"); | ||
| log_exchanges(&memory, "t1", 3).await; | ||
| let quiet = memory.clone().with_policy(RecallPolicy { | ||
| history_limit: 0, | ||
| ..memory.policy().clone() | ||
| }); | ||
|
|
||
| let pack = quiet | ||
| .pre_turn_resumed(PreTurn { | ||
| in_prompt_from: 4, | ||
| ..PreTurn::new("t1", 6, "how far is Porto") | ||
| }) | ||
| .await | ||
| .unwrap() | ||
| .pack; | ||
| assert!(!pack.markdown.contains(THREAD_HEADING), "{}", pack.markdown); | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn older_turns_survive_a_prompt_window_bigger_than_the_overfetch() { | ||
| // 30 exchanges = turns 0..59; the prompt holds turns 8..59 (52 turns), | ||
| // far more than the section's overfetch allowance. Before the fix the | ||
| // newest candidates were cut to that allowance first, all of them were | ||
| // in the window, and the thread section came back empty. | ||
| let engine = Arc::new(ReferenceEngine::new()); | ||
| let memory = memory(&engine, "assistant"); | ||
| log_exchanges(&memory, "t1", 30).await; | ||
|
|
||
| let pack = memory | ||
| .pre_turn_resumed(PreTurn { | ||
| in_prompt_from: 8, | ||
| ..PreTurn::new("t1", 60, "how far is Porto") | ||
| }) | ||
| .await | ||
| .unwrap() | ||
| .pack; | ||
| let markdown = &pack.markdown; | ||
| assert!(markdown.contains(THREAD_HEADING), "{markdown}"); | ||
| assert!( | ||
| markdown.contains("leg 3"), | ||
| "the newest turn before the window: {markdown}" | ||
| ); | ||
| assert!( | ||
| !markdown.contains("leg 4:"), | ||
| "turns in the window stay out: {markdown}" | ||
| ); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -80,6 +80,7 @@ pub(super) async fn section( | |
| section: &ScopeSection, | ||
| beliefs: usize, | ||
| ) -> Gathered { | ||
| let keep = |hit: &Hit| !request.excludes(hit); | ||
| let want = wanted(request, section); | ||
| let outcome = match §ion.query { | ||
| SectionQuery::Answer { | ||
|
|
@@ -94,7 +95,7 @@ pub(super) async fn section( | |
| "[recall] answer failed, fetching instead heading={:?} error={error}", | ||
| section.heading | ||
| ); | ||
| fetch(engine, §ion.filter, question, want, 0).await | ||
| fetch(engine, §ion.filter, question, want, 0, &keep).await | ||
| } | ||
| Err(error) => Err(error), | ||
| }, | ||
|
|
@@ -104,11 +105,11 @@ pub(super) async fn section( | |
| .or(request.query.as_deref()) | ||
| .filter(|query| !query.trim().is_empty()) | ||
| { | ||
| Some(query) => fetch(engine, §ion.filter, query, want, beliefs).await, | ||
| None => with_listed_beliefs(engine, section, want).await, | ||
| Some(query) => fetch(engine, §ion.filter, query, want, beliefs, &keep).await, | ||
| None => with_listed_beliefs(engine, section, want, &keep).await, | ||
| } | ||
| } | ||
| SectionQuery::Latest => with_listed_beliefs(engine, section, want).await, | ||
| SectionQuery::Latest => with_listed_beliefs(engine, section, want, &keep).await, | ||
| }; | ||
| match outcome { | ||
| Ok((hits, beliefs)) => Gathered::Hits { hits, beliefs }, | ||
|
|
@@ -235,17 +236,21 @@ async fn with_listed_beliefs( | |
| engine: &dyn MemoryEngine, | ||
| section: &ScopeSection, | ||
| want: usize, | ||
| keep: &dyn Fn(&Hit) -> bool, | ||
| ) -> tinymemory_api::Result<(Vec<Hit>, Vec<Hit>)> { | ||
| if !reads_learnings(section) { | ||
| return Ok((latest(engine, §ion.filter, want).await?, Vec::new())); | ||
| return Ok(( | ||
| latest(engine, §ion.filter, want, keep).await?, | ||
| Vec::new(), | ||
| )); | ||
| } | ||
| let reach = section | ||
| .filter | ||
| .reach | ||
| .clone() | ||
| .unwrap_or_else(|| Reach::subtree(Namespace::ROOT)); | ||
| let (hits, beliefs) = join( | ||
| latest(engine, §ion.filter, want), | ||
| latest(engine, §ion.filter, want, keep), | ||
| engine.beliefs(BeliefsRequest::new(reach, want)), | ||
| ) | ||
| .await; | ||
|
|
@@ -319,9 +324,10 @@ async fn fetch( | |
| query: &str, | ||
| limit: usize, | ||
| beliefs: usize, | ||
| keep: &dyn Fn(&Hit) -> bool, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift Apply exclusions before limiting fetched hits. When the engine supports a fetch mode, 🤖 Prompt for AI Agents
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 511571a: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
git show --stat 511571a660
ast-grep outline crates/tinymemory-tools/src/recall/gather.rs --match 'fetch|FETCH_MAX_PAGES' --view expanded
ast-grep run --lang rust --pattern 'async fn fetch($$$) { $$$ }' crates/tinymemory-tools/src/recall/gather.rs
rg -n -C 4 'FETCH_MAX_PAGES|a_ranked_section_reads_past_a_page_the_thread_window_empties' crates/tinymemory-tools
git show --format=short 511571a660 -- crates/tinymemory-tools/src/recall/gather.rs
rg -n -A 130 -B 5 'async fn a_ranked_section_reads_past_a_page_the_thread_window_empties\b' crates/tinymemory-toolsLength of output: 1128 🏁 Script executed: #!/bin/bash
ast-grep outline crates/tinymemory-tools/src/recall/gather.rs --match 'fetch|FETCH_MAX_PAGES' --view expanded
sed -n '310,415p' crates/tinymemory-tools/src/recall/gather.rs
rg -n -C 4 'FETCH_MAX_PAGES|a_ranked_section_reads_past_a_page_the_thread_window_empties' crates/tinymemory-tools
rg -n -A 130 -B 5 'async fn a_ranked_section_reads_past_a_page_the_thread_window_empties\b' crates/tinymemory-toolsLength of output: 4621
Please retry verification once the updated commit is available. I’m leaving this finding open because the inspected code predates your reported fix. You are interacting with an AI system. |
||
| ) -> tinymemory_api::Result<(Vec<Hit>, Vec<Hit>)> { | ||
| let Some(mode) = preferred_mode(engine) else { | ||
| return Ok((latest(engine, filter, limit).await?, Vec::new())); | ||
| return Ok((latest(engine, filter, limit, keep).await?, Vec::new())); | ||
| }; | ||
| let mut request = FetchRequest::new(query, mode, limit); | ||
| request.filter = filter.clone(); | ||
|
|
@@ -332,18 +338,24 @@ async fn fetch( | |
|
|
||
| /// The newest hits, then the most confident, then the latest turn; ties | ||
| /// keep the engine's order. | ||
| /// | ||
| /// Hits `keep` refuses (the request's exclusions: the prompt's own thread | ||
| /// window, ids already shown) are dropped *before* the cut to `limit`, so a | ||
| /// window holding more recent turns than the overfetch allowance cannot | ||
| /// crowd every older turn out of the section. | ||
| async fn latest( | ||
| engine: &dyn MemoryEngine, | ||
| filter: &MetaFilter, | ||
| limit: usize, | ||
| keep: &dyn Fn(&Hit) -> bool, | ||
| ) -> tinymemory_api::Result<Vec<Hit>> { | ||
| let mut all: Vec<Hit> = Vec::new(); | ||
| let mut cursor: Option<String> = None; | ||
| for _ in 0..LATEST_MAX_PAGES { | ||
| let mut request = ListRequest::new(filter.clone(), LATEST_PAGE); | ||
| request.cursor = cursor.take(); | ||
| let page = engine.list(request).await?; | ||
| all.extend(page.items); | ||
| all.extend(page.items.into_iter().filter(|hit| keep(hit))); | ||
| match page.next_cursor { | ||
| Some(next) => cursor = Some(next), | ||
| None => break, | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.