feat(flows): general read-only flow memory-agent (#5204) - #5205
Merged
Conversation
…ry access General (not per-case) runtime-reasoned memory retrieval for automation flows: a flow `agent` node now routes to `flow_memory_agent` via `config.agent_ref` for ANY step that needs the user's context, style, history, or people, looping across as many retrievals as the step needs and returning a concise, source-attributed text answer. Read-only belt (exactly 8 tools, sandbox_mode = "read_only"): memory_recall, memory_hybrid_search, memory_flavour, people_list, transcript_search, thread_list, thread_read, thread_message_list. `memory_tree` is deliberately excluded — it declares ReadOnly but exposes an `ingest_document` write mode that survives the read-only sandbox filter, which would hand this auto-run, prompt-injectable agent a memory-write foothold. `context_scout` is retained for its narrower structured `[context_bundle]` output; `flow_memory_agent` is now the preferred general route in the workflow_builder prompt. Purely additive: new agent directory, +1 `pub mod` in agent_registry/agents/mod.rs, +1 BUILTINS entry and tests in agent_registry/agents/loader.rs, workflow_builder prompt/test updates, and one pinned test-snapshot line in agent/harness/definition_tests.rs (the exhaustive built-in effective_max_iterations() audit). No main-module business logic touched. Closes tinyhumansai#5204
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR adds the feature-gated ChangesFlow memory agent
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant WorkflowBuilder
participant flow_memory_agent
participant ReadOnlyMemoryTools
WorkflowBuilder->>flow_memory_agent: route general context or history need
flow_memory_agent->>ReadOnlyMemoryTools: retrieve grounded memory and conversation data
ReadOnlyMemoryTools-->>flow_memory_agent: return retrieved content
flow_memory_agent-->>WorkflowBuilder: return concise attributed context
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
graycyrus
marked this pull request as ready for review
July 27, 2026 07:29
coderabbitai
Bot
added
agent
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: de8727de3f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Comment on lines
+107
to
+112
| BuiltinAgent { | ||
| id: "flow_memory_agent", | ||
| toml: include_str!("flow_memory_agent/agent.toml"), | ||
| prompt_fn: super::flow_memory_agent::prompt::build, | ||
| graph_fn: None, | ||
| }, |
There was a problem hiding this comment.
In a --no-default-features build, this unconditional entry is still loaded by default_agents(), so flow_memory_agent remains advertised in the agent registry even though the flow engine and all other flow agents are compiled out. Gate both this entry and its module with #[cfg(feature = "flows")], matching workflow_builder and flow_discovery, so the slim build removes the complete flow-specific surface.
AGENTS.md reference: AGENTS.md:L282-L282
Useful? React with 👍 / 👎.
Comment on lines
+11
to
+14
| # Cap on the returned text. The runner truncates the final output to this many | ||
| # characters (char-safe) before handing it back to the calling flow node, so a | ||
| # flow's context budget only ever grows by a bounded amount. | ||
| max_result_chars = 4000 |
There was a problem hiding this comment.
When this agent is invoked through the newly documented config.agent_ref route and returns more than 4,000 characters, the cap is not applied: OpenHumanAgentRunner::run_via_harness calls Agent::run_single and passes its complete response to build_agent_result, while the only enforcement of max_result_chars is inside the separate subagent runner. Consequently a verbose retrieval can place far more than the promised 4,000 characters into the flow and its downstream context; the flow harness path needs to truncate using the selected definition's cap.
Useful? React with 👍 / 👎.
|
| Filename | Overview |
|---|---|
| src/openhuman/agent_registry/agents/flow_memory_agent/agent.toml | New agent definition: read-only worker tier, burst model, 8 curated tools, memory_tree exclusion well-documented, max_result_chars=4000 cap set correctly. |
| src/openhuman/agent_registry/agents/flow_memory_agent/prompt.md | New agent prompt: clearly delineates read-only contract, prompt-injection resistance, source attribution, and not-found honesty guidance. |
| src/openhuman/agent_registry/agents/flow_memory_agent/prompt.rs | Prompt builder follows established context_scout pattern; three unit tests cover non-empty output, read-only contract, and tool-name presence. |
| src/openhuman/agent_registry/agents/loader.rs | BUILTINS entry correctly feature-gated and structured; the comment in low_context_workers_use_burst_hint incorrectly claims array literals can't carry per-element cfg, contradicted by existing usages in definition_tests.rs. |
| src/openhuman/agent/harness/definition_tests.rs | Correctly adds flows-gated (flow_memory_agent, 50) to the iteration-cap audit list, consistent with the existing per-element cfg pattern already used for mcp_agent, flow_discovery, etc. |
| src/openhuman/flows/agents/workflow_builder/builder_prompt.rs | Three test additions/updates correctly guard the new routing guidance; the regression test for customer-history example routing is a direct response to the previous review finding. |
| src/openhuman/flows/agents/workflow_builder/prompt.md | Memory-reading section updated from two to three mechanisms; flow_memory_agent correctly positioned as the PREFERRED general route and context_scout demoted to its structured-bundle niche. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
FN["Flow agent node\n(config.agent_ref)"]
FN -->|"agent_ref = flow_memory_agent\n(PREFERRED — ANY context/style/history/people need)"| FMA
FN -->|"agent_ref = context_scout\n(narrower niche: structured context_bundle output only)"| CS
FN -->|"config.slug = oh:memory_recall\nor oh:memory_hybrid_search\n(single deterministic read)"| TC["tool_call node"]
subgraph FMA["flow_memory_agent (NEW)"]
direction LR
T1["memory_recall / memory_hybrid_search / memory_flavour"]
T2["people_list"]
T3["transcript_search / thread_list / thread_read / thread_message_list"]
T1 --- T2 --- T3
end
subgraph CS["context_scout"]
CB["context_bundle\nrecommended_tool_calls / recommended_skills"]
end
FMA -->|"plain text answer (source-attributed)"| OUT["Downstream flow node\n(via input_context)"]
CS -->|"structured bundle (via input_context)"| OUT
TC -->|"=nodes.id.item.json.content[0].text"| OUT
style FMA fill:#d4edda,stroke:#28a745
style CS fill:#fff3cd,stroke:#ffc107
style TC fill:#cce5ff,stroke:#004085
Reviews (4): Last reviewed commit: "fix(flows): route generic customer-histo..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/openhuman/agent_registry/agents/flow_memory_agent/agent.toml`:
- Around line 1-4: The new user-selectable flow_memory_agent capability lacks
integration coverage and user-facing documentation. Add Rust-level flow
execution coverage plus JSON-RPC and E2E tests that verify config.agent_ref
resolves correctly, produces bounded source-attributed output, and performs
read-only retrieval; then update the relevant content under about_app to
describe this capability.
In `@src/openhuman/flows/agents/workflow_builder/prompt.md`:
- Around line 381-383: Route the generic customer-history example in prompt.md
to flow_memory_agent, retaining context_scout only for requests explicitly
requiring a structured [context_bundle]. In builder_prompt.rs, update the
relevant assertions near the concrete routing examples to verify that “what this
customer has asked us before” selects flow_memory_agent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d59cb375-a421-4500-b93d-8430a91468a4
📒 Files selected for processing (9)
src/openhuman/agent/harness/definition_tests.rssrc/openhuman/agent_registry/agents/flow_memory_agent/agent.tomlsrc/openhuman/agent_registry/agents/flow_memory_agent/mod.rssrc/openhuman/agent_registry/agents/flow_memory_agent/prompt.mdsrc/openhuman/agent_registry/agents/flow_memory_agent/prompt.rssrc/openhuman/agent_registry/agents/loader.rssrc/openhuman/agent_registry/agents/mod.rssrc/openhuman/flows/agents/workflow_builder/builder_prompt.rssrc/openhuman/flows/agents/workflow_builder/prompt.md
Comment on lines
+1
to
+4
| id = "flow_memory_agent" | ||
| display_name = "Flow Memory Agent" | ||
| delegate_name = "retrieve_flow_context" | ||
| when_to_use = "General-purpose read-only context and memory retrieval specialist for automation flows. A flow `agent` node routes here via `config.agent_ref` for ANY step that needs the user's context, style, history, or people — not a fixed list of cases: drafting in the user's tone, resolving 'the customer I talked to last week', checking a preference before acting, looking up a contact, or any other run-time memory need a flow author can't fully predict at build time. It may loop across several retrievals in one turn to gather what the step needs, then returns a concise, source-attributed text answer — never a structured bundle, never an action. `context_scout` remains the right choice only when a step specifically needs the scout's structured `[context_bundle]` output; for everything else, prefer this agent." |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Add the required JSON-RPC/E2E coverage and About App update.
This new user-selectable config.agent_ref is covered only by definition/prompt unit tests in the supplied changes. Add a JSON-RPC and E2E flow run proving resolution, bounded output, and read-only retrieval behavior, and document the new user-facing capability in src/openhuman/about_app/.
As per coding guidelines, “Follow the workflow: specify, prove in Rust, prove over JSON-RPC, surface in the UI, and test at unit and E2E levels” and “Update src/openhuman/about_app/ when adding… user-facing features.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/openhuman/agent_registry/agents/flow_memory_agent/agent.toml` around
lines 1 - 4, The new user-selectable flow_memory_agent capability lacks
integration coverage and user-facing documentation. Add Rust-level flow
execution coverage plus JSON-RPC and E2E tests that verify config.agent_ref
resolves correctly, produces bounded source-attributed output, and performs
read-only retrieval; then update the relevant content under about_app to
describe this capability.
Source: Coding guidelines
…feature + bound its output Codex P2 (loader.rs): flow_memory_agent is flow-purposed (routed to only from a flow agent_ref), so gate it `#[cfg(feature = "flows")]` like workflow_builder/flow_discovery — a slim --no-default-features build now drops it instead of advertising dead flow surface. Gated in lockstep: the module decl, the BUILTINS entry, the dedicated loader test, and the definition_tests.rs pinned-audit line; removed from the plain-array low_context_workers_use_burst_hint list (can't per-element cfg an array literal — the gated dedicated test covers its burst hint). Codex P2 (agent.toml max_result_chars): enforcement of max_result_chars on the flow agent_ref path lives in run_via_harness (a main-module engine seam) and is missing for ALL agent_ref agents, not just this one — a pre-existing systemic gap, out of scope for this additive PR. Mitigated in-scope by tightening the agent's prompt to return a short, distilled answer (well under ~4000 chars, summarize-don't-dump). Systemic fix tracked separately.
Contributor
Author
|
Review addressed (commit
|
coderabbitai
Bot
removed
the
memory
…yhumansai#5209) `dedicated_profile_experience_recall_merges_shared_legacy_store` failed run-to-run under the diff-scoped command `cargo test -p openhuman --lib -- 'openhuman::agent' 'openhuman::agent_registry' 'openhuman::flows'`. The scope filter is a prefix match, so it also runs `agent_orchestration`, `agent_experience`, etc. Root-causing found TWO independent, real bugs — both reproduced deterministically — not the HashMap-ordering the symptom suggested. 1. Deep agent-turn tests overflow the default ~2 MiB libtest thread stack. `turn_dispatches_spawn_subagent_through_full_path` and `agent_turn_runs_long_parallel_subagent_flow_with_many_nested_tool_calls` drive the full (nested / parallel) agent-turn harness — a deep async state machine. In debug/coverage builds the stacked frames exceed 2 MiB, the thread overflows, and the process SIGABRTs. Because libtest runs tests concurrently, the abort tags whichever unrelated test was in flight as FAILED — most often the experience-recall test. This is why the "same commit" flipped pass↔fail: the overflow is deterministic, but *which* concurrent test gets tagged is not. CI hid it via `RUST_MIN_STACK=64MiB`; a raw `cargo test` has no such env. Fixed by running both tests on an explicit 64 MiB stack (mirroring production's `agent::bus::handle_agent_run_turn_on_large_stack`), so they never abort the process regardless of `RUST_MIN_STACK`. 2. A Luhn-valid millisecond timestamp corrupts the stored experience JSON. `AgentExperienceStore` serializes each experience to JSON and stores it as memory-document `content`. `Memory::store` runs content through the secret/PII sanitizer, whose credit-card pattern (`\b(?:\d[\s\-]?){13,19}\b`, Luhn-gated) matches any 13–19-digit run. `put` stamps `created_at_ms` / `updated_at_ms` with `now_ms()` — a bare 13-digit run. Exactly 10% of ms timestamps pass Luhn (measured), so ~1 write in 10 had its timestamp rewritten to a `[REDACTED_PII_*]` token, producing invalid JSON that failed to parse on read — the experience silently vanished from `list()`, recall returned nothing, and the assertion genuinely failed (~10%, matching the observed rate). This corrupted real agent experiences in production too, not just the test. Fixed by base64-encoding the structured payload before storage (sanitizer no-op over base64; lossless round-trip; the store still redacts the sensitive free-text fields itself before serialization), with a plain-JSON fallback on read so pre-existing records still decode. Added a deterministic regression test over the real `UnifiedMemory` using a Luhn-valid timestamp. Also made the parallel test's overlap assertion deterministic. It relied on a fixed 25 ms `sleep` to make two subagent provider calls overlap; that raced under load and flaked (~13% even in isolation once the stack fix let it run). Replaced with a timeout-guarded `Barrier(2)` rendezvous so `max_active >= 2` holds deterministically, failing (not hanging) if parallelism regresses. Verification (fresh `cargo test` process each run, new HashMap/clock seed): - target test: 10/10 pass under the full diff-scope; 0 stack overflows. - both formerly-overflowing tests: pass, including 20/20 for the parallel test alone (was ~13/15). - new regression test passes; fails without the base64 fix. - agent_orchestration (301), agent_experience (28), memory_store (277): green. Closes tinyhumansai#5209
…efore base64 (review P1) The tinyhumansai#5209 base64 fix stores the experience payload base64-encoded, which makes `Memory::store`'s content sanitizer a no-op over the payload. Previously that store-time scrub redacted secrets anywhere in the serialized JSON; base64 silently removed that protection. The store's own `redact_experience` was a NARROWER scrubber (only Bearer / OpenAI `sk-` / `key=value` secret patterns), so genuine secrets in captured free-text — Stripe `sk_live_` keys, phone numbers, private-key blocks, national-ID PII, arbitrary fields the `agent_experience. capture` RPC accepts (it takes the whole `AgentExperience`) — were being stored REVERSIBLY (base64 is trivially decodable) and returned verbatim on recall. Fix: `redact_experience` now runs the SAME full scrubber the memory layer uses — `memory_store::safety::sanitize_text` (private-key blocks, Bearer/`sk-`/Stripe/ npm/OAuth secrets, then the full national-ID / phone / credit-card PII set) — over every sensitive free-text field before serialization + base64: task_fingerprint, task_summary, lesson, reuse_hint, avoid_hint, error_class, agent_id, entrypoint, tools_used, tool_sequence, tags. Secrets are therefore redacted in BOTH the stored record and what recall returns. Only string fields are scrubbed. The numeric timestamp/confidence fields are left untouched — scrubbing structural numbers is exactly the Luhn-timestamp corruption tinyhumansai#5209 fixed, and base64 still shields those numbers from the store-time re-scrub. `id` (storage key; a secret key is rejected up front by the memory layer's `has_likely_secret` guard) and `profile_id` (hard partition- filter key) are deliberately left intact to avoid key/partition desync. Added `secrets_in_free_text_are_redacted_before_storage`: over the real `UnifiedMemory`, captures an experience whose free-text fields carry a Stripe key, a phone number and a private-key block, reads it back through the store, and asserts (a) each secret is redacted in the recalled content and (b) the Luhn-valid timestamp survives intact and the record still parses. It fails on the pre-fix code (the old `redact_text` matched `sk-` with a hyphen, not the `sk_live_` Stripe form, nor phone / private-key patterns). Verification: new secret-redaction test + the Luhn-timestamp test pass; agent_experience suite 29/29; target flaky test 10/10 under the full diff-scope with 0 stack overflows.
…' into feat/flow-memory-agent
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/openhuman/agent_experience/store.rs (3)
42-52: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a trace line on the legacy-JSON fallback path.
The layered guards (base64 → UTF-8 → JSON, else plain JSON) are correct and unambiguous. However, neither branch emits a diagnostic, so you can't tell from logs whether legacy plain-JSON records are still being read (useful for eventually dropping the fallback).
♻️ Suggested diagnostics
fn decode_experience_payload(stored: &str) -> Result<AgentExperience, String> { if let Ok(bytes) = base64::engine::general_purpose::STANDARD.decode(stored.trim()) { if let Ok(text) = std::str::from_utf8(&bytes) { if let Ok(experience) = serde_json::from_str::<AgentExperience>(text) { return Ok(experience); } } } + log::debug!( + "[agent-experience] decode falling back to legacy plain-JSON payload len={}", + stored.len() + ); serde_json::from_str::<AgentExperience>(stored) .map_err(|e| format!("parse agent experience: {e}")) }As per coding guidelines: "New or changed flows must include verbose, grep-friendly diagnostics covering entry/exit, branches, external calls, retries/timeouts, state transitions, and errors".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/agent_experience/store.rs` around lines 42 - 52, Add a verbose, grep-friendly trace diagnostic in decode_experience_payload immediately before the plain JSON serde_json::from_str fallback, indicating that a legacy plain-JSON record is being decoded. Leave the existing layered decoding and error handling unchanged.Source: Coding guidelines
518-630: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd coverage for the legacy plain-JSON decode path.
Both new tests exercise only base64 records written by
put. Thedecode_experience_payloadfallback is what keeps every pre-existing record readable after this change, and nothing tests it. AMockMemory-based test that stores raw JSON directly underexperience/<id>and assertslist()returns it would close the gap cheaply.💚 Suggested test
#[tokio::test] async fn legacy_plain_json_records_still_decode() { let (store, memory) = fresh_store(); let legacy = sample_experience("exp_legacy_json", "legacy task", vec![], vec![], 0.5); let json = serde_json::to_string(&legacy).unwrap(); memory .store( AGENT_EXPERIENCE_NAMESPACE, "experience/exp_legacy_json", &json, MemoryCategory::Custom(AGENT_EXPERIENCE_NAMESPACE.into()), None, ) .await .unwrap(); let listed = store.list().await.unwrap(); assert_eq!(listed.len(), 1); assert_eq!(listed[0].id, "exp_legacy_json"); }As per coding guidelines: "Add tests for new or changed behavior; untested code is incomplete."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/agent_experience/store.rs` around lines 518 - 630, Add a MockMemory-based regression test for the legacy plain-JSON path in decode_experience_payload. Store serialized sample_experience data directly under the experience/<id> key using the appropriate namespace and custom category, then call AgentExperienceStore::list and assert the record is returned with its expected ID.Source: Coding guidelines
338-353: 📐 Maintainability & Code Quality | 🔵 TrivialLog when
redact_experienceredacts a record
sanitize_textalready returns aSanitizationReport; emit a warning whenreport.changed()so redactions intask_fingerprint,task_summary,lesson,reuse_hint, etc. are visible in the write path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/agent_experience/store.rs` around lines 338 - 353, Update redact_experience and its local scrub helper to retain each sanitize_text SanitizationReport and emit a warning whenever report.changed() is true, including enough context to identify the affected field. Ensure all fields currently passed through scrub, including optional values and collections, use this reporting path while preserving their existing redacted values.Source: Coding guidelines
src/openhuman/agent/harness/session/tests.rs (1)
946-961: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the large-stack test-thread wrapper into a shared helper. Both files independently reimplement the identical
thread::Builder+ 64MiB stack + current-threadtokio::runtime+block_on+join().expect(...)boilerplate to route around the same libtest stack-overflow issue (#5209). A single shared helper would remove the duplication and keep the rationale comment in one place for future large-stack tests.
src/openhuman/agent/harness/session/tests.rs#L946-L961: replace the inline thread/runtime setup inturn_dispatches_spawn_subagent_through_full_pathwith a call to a sharedrun_on_large_stack(name, fut)helper.src/openhuman/agent_orchestration/tools/spawn_parallel_agents_tests.rs#L801-L818: replace the inline thread/runtime setup inagent_turn_runs_long_parallel_subagent_flow_with_many_nested_tool_callswith the same shared helper.♻️ Proposed shared helper
fn run_on_large_stack<F>(name: &str, fut: F) where F: std::future::Future<Output = ()> + Send + 'static, { std::thread::Builder::new() .name(name.to_string()) .stack_size(64 * 1024 * 1024) .spawn(move || { tokio::runtime::Builder::new_current_thread() .enable_all() .build() .expect("build large-stack test runtime") .block_on(fut); }) .expect("spawn large-stack test thread") .join() .expect("large-stack test thread panicked"); }Each call site then reduces to:
#[test] fn turn_dispatches_spawn_subagent_through_full_path() { - std::thread::Builder::new() - .name("spawn-subagent-full-path-test".to_string()) - .stack_size(64 * 1024 * 1024) - .spawn(|| { - tokio::runtime::Builder::new_current_thread() - .enable_all() - .build() - .expect("build large-stack test runtime") - .block_on(turn_dispatches_spawn_subagent_through_full_path_inner()); - }) - .expect("spawn large-stack test thread") - .join() - .expect("large-stack spawn_subagent test thread panicked"); + run_on_large_stack( + "spawn-subagent-full-path-test", + turn_dispatches_spawn_subagent_through_full_path_inner(), + ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/agent/harness/session/tests.rs` around lines 946 - 961, Extract the duplicated large-stack thread/runtime boilerplate into a shared run_on_large_stack(name, fut) helper, preserving the 64MiB stack, current-thread Tokio runtime, block_on, and join error handling. In src/openhuman/agent/harness/session/tests.rs lines 946-961, replace the wrapper in turn_dispatches_spawn_subagent_through_full_path with this helper; make the same replacement in src/openhuman/agent_orchestration/tools/spawn_parallel_agents_tests.rs lines 801-818 for agent_turn_runs_long_parallel_subagent_flow_with_many_nested_tool_calls, keeping the rationale comment with the shared helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/openhuman/agent_orchestration/tools/spawn_parallel_agents_tests.rs`:
- Around line 645-650: Update the barrier synchronization in the test harness
around self.state.overlap_barrier.wait() so timeout failures are not silently
discarded. Preserve the existing sequencing and delay behavior, but either
handle and log the timeout result explicitly or perform the barrier wait in a
spawned task to avoid cancelling Barrier::wait() directly.
---
Nitpick comments:
In `@src/openhuman/agent_experience/store.rs`:
- Around line 42-52: Add a verbose, grep-friendly trace diagnostic in
decode_experience_payload immediately before the plain JSON serde_json::from_str
fallback, indicating that a legacy plain-JSON record is being decoded. Leave the
existing layered decoding and error handling unchanged.
- Around line 518-630: Add a MockMemory-based regression test for the legacy
plain-JSON path in decode_experience_payload. Store serialized sample_experience
data directly under the experience/<id> key using the appropriate namespace and
custom category, then call AgentExperienceStore::list and assert the record is
returned with its expected ID.
- Around line 338-353: Update redact_experience and its local scrub helper to
retain each sanitize_text SanitizationReport and emit a warning whenever
report.changed() is true, including enough context to identify the affected
field. Ensure all fields currently passed through scrub, including optional
values and collections, use this reporting path while preserving their existing
redacted values.
In `@src/openhuman/agent/harness/session/tests.rs`:
- Around line 946-961: Extract the duplicated large-stack thread/runtime
boilerplate into a shared run_on_large_stack(name, fut) helper, preserving the
64MiB stack, current-thread Tokio runtime, block_on, and join error handling. In
src/openhuman/agent/harness/session/tests.rs lines 946-961, replace the wrapper
in turn_dispatches_spawn_subagent_through_full_path with this helper; make the
same replacement in
src/openhuman/agent_orchestration/tools/spawn_parallel_agents_tests.rs lines
801-818 for
agent_turn_runs_long_parallel_subagent_flow_with_many_nested_tool_calls, keeping
the rationale comment with the shared helper.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: aa994809-f0cc-44bc-826e-6427ac3c5b72
📒 Files selected for processing (3)
src/openhuman/agent/harness/session/tests.rssrc/openhuman/agent_experience/store.rssrc/openhuman/agent_orchestration/tools/spawn_parallel_agents_tests.rs
Comment on lines
+645
to
+650
| let seq = self.state.subagent_call_seq.fetch_add(1, Ordering::SeqCst); | ||
| if seq < 2 { | ||
| let _ = timeout(Duration::from_secs(5), self.state.overlap_barrier.wait()).await; | ||
| } else { | ||
| sleep(Duration::from_millis(5)).await; | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does tokio's Barrier::wait() leave its internal arrived counter incremented if the future is dropped before completing (e.g. via tokio::time::timeout)?
💡 Result:
Yes, tokio::sync::Barrier::wait does leave the internal arrived counter incremented if the future is dropped before completing. The implementation of Barrier::wait is not cancel-safe [1][2][3]. When you call wait, the future immediately increments the internal arrived counter [1][4]. If the future is subsequently dropped—for instance, because it was part of a tokio::select! block or wrapped in a timeout—that incremented count remains in the barrier's state [2]. Because the barrier has no mechanism to decrement the counter upon cancellation, this can lead to incorrect behavior where the barrier considers a task to have "arrived" when it has actually been dropped, potentially causing the barrier to trigger prematurely or behave unpredictably in subsequent uses [2]. Tokio documentation explicitly warns that Barrier::wait is not cancel-safe [5][3].
Citations:
- 1: https://github.com/tokio-rs/tokio/blob/master/tokio/src/sync/barrier.rs
- 2: Tokio select! Macro Issue with Barrier tokio-rs/tokio#6493
- 3: https://github.com/tokio-rs/tokio/blob/306ed1c3/tokio/src/sync/barrier.rs
- 4: https://github.com/tokio-rs/tokio/blob/c637f6e7/tokio/src/sync/barrier.rs
- 5: https://docs.rs/tokio/latest/tokio/sync/struct.Barrier.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant slice with line numbers.
sed -n '620,680p' src/openhuman/agent_orchestration/tools/spawn_parallel_agents_tests.rs
# Find the barrier definition and any other waits/timeouts in this file.
rg -n "overlap_barrier|Barrier::wait|timeout\\(|spawn\\(" src/openhuman/agent_orchestration/tools/spawn_parallel_agents_tests.rsRepository: tinyhumansai/openhuman
Length of output: 3331
Don't swallow the barrier timeout
timeout(...).await is dropped on the floor here, so a real rendezvous failure only shows up later as a flaky peak-count assertion. Barrier::wait() is also not cancel-safe, so a timed-out wait can skew the barrier state for the rest of this harness. Log the timeout, or run the barrier wait in a spawned task so cancellation stays outside the barrier.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/openhuman/agent_orchestration/tools/spawn_parallel_agents_tests.rs`
around lines 645 - 650, Update the barrier synchronization in the test harness
around self.state.overlap_barrier.wait() so timeout failures are not silently
discarded. Preserve the existing sequencing and delay behavior, but either
handle and log the timeout result explicitly or perform the barrier wait in a
spawned task to avoid cancelling Barrier::wait() directly.
Source: Coding guidelines
…nt (review) Greptile P1 + CodeRabbit: the agent_ref example 'work out what this customer has asked us before' → context_scout contradicted the new routing rule (general context/history → flow_memory_agent; context_scout only for structured [context_bundle]). Reroute the example to flow_memory_agent and add a regression guard asserting it routes there and NOT to context_scout, so the contradiction can't return.
Contributor
Author
|
Review addressed (commit
|
coderabbitai
Bot
added
the
memory
4 tasks
This was referenced Jul 27, 2026
12 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
flow_memory_agent, a new dedicated read-only built-in agent that any automation flowagentnode can route to viaconfig.agent_reffor general run-time context/memory retrieval (style, history, people, preferences) — not a fixed list of cases.memory_recall,memory_hybrid_search,memory_flavour,people_list,transcript_search,thread_list,thread_read,thread_message_list);memory_treeis deliberately excluded (see Problem).sandbox_mode = "read_only",agent_tier = "worker",iteration_policy = "extended"(loop across several retrievals in one turn), boundedmax_result_chars = 4000.workflow_builder's standing prompt to teachflow_memory_agentas the PREFERRED general route for context/style/history/people needs, keepingcontext_scoutfor its narrower structured[context_bundle]niche.pub mod+ 1BUILTINSentry + tests; no existing agent, the flow engine, or memory internals were modified.Problem
Today a flow
agentnode's only real-agent route into the user's memory/context iscontext_scout, which is designed to emit a structured pre-flight[context_bundle]for an orchestrator — not a plain-text answer for an arbitrary flow step ("draft a reply in the user's tone", "who did I email about X", "what does this customer prefer"). Without a general-purpose route, the workflow builder either mis-authors a plainagentnode that fabricates a memory lookup it can't actually perform, or force-fitscontext_scout's bundle format onto steps that just need a short grounded answer. Issue #5204 asks for a dedicated, general, read-only memory/context agent for this.Solution
flow_memory_agentmodeled oncontext_scout's file structure (agent.toml/prompt.md/prompt.rs/mod.rs), registered as one moreBuiltinAgententry inagent_registry/agents/loader.rs::BUILTINS(no branching, no new match arms — same additive pattern every built-in agent already follows).memory_treeis intentionally never added: it declaresPermissionLevel::ReadOnlyon its arglesspermission_level()but still dispatches aningest_documentwrite mode, so it survives the read-only sandbox filter despite being a write tool — a documented hazard already called out forcontext_scoutand now pinned for this agent too (both inagent.tomlcomments and a loader test).workflow_builder/prompt.md's "Reading the user's memory at run time" section now teaches three mechanisms instead of two (deterministictool_callreads, the newflow_memory_agentgeneral route, andcontext_scout's narrower bundle niche), plus one newagent_refexample.memory_tree/memory_store/shell/etc.), theflow_memory_agentid added to the existingburst-hint worker list and the pinnedeffective_max_iterations()audit snapshot, and threeworkflow_builderprompt-regression tests (general-route teaching, updated to "three" working memory read paths,flow_memory_agentadded to the specialist-selection guard).Submission Checklist
flow_memory_agent_is_read_only_worker_with_bounded_memory_belt(positive tool-belt + negative forbidden-tool assertions),flow_memory_agent/prompt.rsunit tests, and the threebuilder_prompt.rsregression tests listed above.cargo build/cargo test/cargo checkintentionally not run locally per this task's explicit constraint (CI validates); every field name, enum variant, tool name, and test helper referenced was confirmed by reading the live source first (see PR description / agent report).rustfmt --edition 2021was run on all changed.rsfiles.docs/TEST-COVERAGE-MATRIX.mdapplies (not a user-toggleable feature flag).Closes #5204in the## Relatedsection.Impact
src/openhuman/agent_registry/agents/**,src/openhuman/flows/agents/workflow_builder/**,src/openhuman/agent/harness/definition_tests.rs). No frontend/Tauri/mobile changes.config.agent_ref = "flow_memory_agent"on anagentnode. No existing agent, controller, or RPC schema was changed.sandbox_mode = "read_only"), no shell/file-write/delegation tools, andmemory_tree's write-mode hazard is explicitly excluded — this agent runs on flow trigger data a third party can influence, so every belt tool must be genuinely read-only (verified against each tool'spermission_level()implementation).flowsCargo feature, so it stays registered (harmlessly unreachable viaagent_ref, since there's noflows-gated caller) even in a slim build withoutflows.Related
AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
feat/flow-memory-agentValidation Run
pnpm --filter openhuman-app format:check— no frontend files changed.pnpm typecheck— no frontend files changed.flow_memory_agent_is_read_only_worker_with_bounded_memory_belt,low_context_workers_use_burst_hint(updated),all_builtin_agent_definitions_have_expected_effective_max_iterations(updated),flow_memory_agent::prompt::tests::{build_returns_nonempty_body, body_describes_the_read_only_contract, body_instructs_memory_and_people_and_thread_gathering},standing_prompt_teaches_specialist_agent_ref_selection(updated),standing_prompt_teaches_flow_memory_agent_as_general_context_route(new),standing_prompt_teaches_the_three_working_memory_read_paths(renamed/updated).rustfmt --edition 2021run on every changed.rsfile (clean, no diff).cargo check/cargo testintentionally not run per this task's constraint — CI validates.app/src-taurifiles changed.Validation Blocked
command:cargo check --manifest-path Cargo.toml/cargo test --lib agent_registry:: agent::harness:: flows::error:not run — blocked by this task's explicit "do not run cargo" constraint, not by a failure.impact:correctness rests on manual verification of every referenced field/enum/tool name against the live source (documented in the agent's final report) rather than a local compile; CI'scargo check/ coverage lanes are the first real compile of this diff.Behavior Changes
agentnode can now setconfig.agent_ref = "flow_memory_agent"to get a real, read-only, general-purpose memory/context retrieval turn instead of either fabricating a lookup (plainagentnode) or force-fittingcontext_scout's structured bundle format.list_agent_profiles/ the Flows builder;workflow_buildernow recommends it by default for context/style/history/people needs.Parity Contract
context_scoutand its callers are untouched.load_builtins()/list_agent_profilespath every other built-in already uses.Duplicate / Superseded PR Handling
Summary by CodeRabbit
New Features
Bug Fixes