fix(flows): stop the builder authoring memory steps that fabricate#5150
Conversation
The standing workflow_builder prompt claimed an `agent` node "can also read and write the user's memory at run time". Both halves were false. A plain `agent` node (no `agent_ref`) is a single completion through `OpenHumanLlm::complete` — no tool loop, so it can neither read nor write memory. Told otherwise, the builder authored plain agent nodes prompted to "recall the user's preference", and the model INVENTED one. The step didn't fail; it fabricated context and the graph still looked correct, which is strictly worse than not working. Replace it with the two mechanisms that actually reach memory from a running flow: - a `tool_call` node with `oh:memory_recall` / `oh:memory_hybrid_search` for one deterministic read, including the correct downstream binding (`=nodes.<id>.item.json.content[0].text` — a native tool result is a `ToolResult`, so it dereferences `.content[0].text`, not `.<field>`) - `agent_ref = "context_scout"` when the step must DECIDE what to look up, or look up several things — that routes to the harness (tinyhumansai#5120, tinyhumansai#5121) and gets a real read-only tool loop Also state plainly that a workflow can never WRITE the user's memory, so the builder stops authoring remember/store steps and says so instead. Deliberately does NOT steer to `agent_memory`, despite it being a registered `read_only` builtin with memory tools: its `memory_tree` tool inherits the trait-default `PermissionLevel::ReadOnly` while dispatching an `ingest_document` WRITE mode, and the read-only tool filter in `session/builder/factory.rs` consults the argless `permission_level()`, so it survives that filter. Flows run on trigger data a third party can influence, so pointing them there would hand injected content a memory-write foothold — the hole `context_scout`'s own agent.toml documents refusing. Tracked separately. Tests: 3 regression guards pinning the banned wording, both working read paths + the binding, and the no-write/no-agent_memory contract.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 56 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
| Filename | Overview |
|---|---|
| src/openhuman/flows/agents/workflow_builder/prompt.md | Corrects a fabrication-causing prompt bug: removes the false "agent node can read and write memory" claim and replaces it with accurate guidance for two real runtime-memory paths (oh:memory_recall/oh:memory_hybrid_search tool_call nodes and context_scout agent_ref), including the correct ToolResult binding path; explicitly bans memory writes and avoids steering to agent_memory for the documented security reason. |
| src/openhuman/flows/agents/workflow_builder/builder_prompt.rs | Adds three regression tests that pin the prompt changes via include_str!: a negative guard for banned fabrication-inducing wording, a positive check for both working memory-read paths and the correct binding path, and a contract test that the prompt bans agent_memory. One positive assertion embeds a newline + specific indentation, making it brittle to future reformatting of prompt.md. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[User asks for memory-aware workflow] --> B{Which memory mechanism?}
B -->|Deterministic read at fixed point| C["tool_call node\nconfig.slug = oh:memory_recall\nor oh:memory_hybrid_search"]
B -->|Needs to decide what to look up| D["agent node\nconfig.agent_ref = context_scout\n(real read-only tool loop)"]
B -->|Write memory| E["Not possible\nWorkflow can never WRITE memory\nTell user plainly"]
C --> F["Bind output downstream:\n=nodes.id.item.json.content[0].text\n(ToolResult envelope — NOT .item.json.field)"]
D --> G["Returns context bundle\nFeed into following agent node\nvia input_context"]
subgraph OLD ["OLD (fabrication bug)"]
H["plain agent node\nprompted to recall preference"]
H --> I["Model INVENTS answer\nGraph looks correct\nSilent fabrication"]
end
subgraph NEW ["NEW (this PR)"]
C
D
E
F
G
end
Reviews (1): Last reviewed commit: "fix(flows): stop the builder authoring m..." | Re-trigger Greptile
| assert!( | ||
| STANDING_PROMPT.contains("A plain `agent` node has NO\n memory access"), | ||
| "standing prompt must state outright that a plain agent node has no \ | ||
| memory access, so the builder never authors a no-op recall step" | ||
| ); |
There was a problem hiding this comment.
The positive assertion embeds a literal
(newline + 3 spaces of indentation), tying the test to the exact current line-wrap position inside the Markdown list. If this paragraph is later reflowed, re-indented, or pulled out of the numbered list, the assertion fails even though the semantic constraint is still satisfied. The negative banned-phrase checks share the same fragility. Every other positive assertion in this test module uses a short, single-line substring that is robust to reformatting. Extracting just the key phrase — without the accidental line break — keeps the guard meaningful while surviving common edits to prompt.md.
| assert!( | |
| STANDING_PROMPT.contains("A plain `agent` node has NO\n memory access"), | |
| "standing prompt must state outright that a plain agent node has no \ | |
| memory access, so the builder never authors a no-op recall step" | |
| ); | |
| assert!( | |
| STANDING_PROMPT.contains("A plain `agent` node has NO"), | |
| "standing prompt must state outright that a plain agent node has no \ | |
| memory access, so the builder never authors a no-op recall step" | |
| ); |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 252ba79389
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| recall) or `oh:memory_hybrid_search` (keyword/lexical lookup). One | ||
| deterministic read at a fixed point in the graph. Its output is a native | ||
| tool result, so bind downstream off | ||
| `=nodes.<id>.item.json.content[0].text` — NOT `.item.json.<field>`. |
There was a problem hiding this comment.
Use a valid jq path for native tool output
This example includes an array index but still uses the bare nodes shorthand. The prompt's own expression rules say paths that continue into jq syntax like [...] must start from the jq root, so builders following this will author =nodes.<id>.item.json.content[0].text, which evaluates as invalid/null instead of reading the native tool result. For memory-recall flows this makes the newly recommended path fail to feed downstream nodes; use the jq form such as =.nodes["<id>"].item.json.content[0].text.
Useful? React with 👍 / 👎.
| **A workflow can never WRITE the user's memory.** There is no | ||
| remember/store step, and no `agent_ref` that grants one — a flow runs on | ||
| trigger data that a third party can influence (an inbound email, a webhook | ||
| payload), so writing that into the user's memory is deliberately not | ||
| possible. If the user asks for a workflow that "remembers" something | ||
| across runs, say plainly that memory writes are not available to workflows | ||
| yet and build the rest of the flow without that step. |
There was a problem hiding this comment.
Enforce the no-memory-write guarantee
This now states that workflows can never write memory, but that guarantee is not enforced by the runtime: a saved/imported flow can still use a native tool_call with slug: "oh:memory_store", which goes through OpenHumanTools::invoke to runtime_node::execute_tool, and the runtime registry includes MemoryStoreTool. In supervised/full autonomy, trigger-derived args can therefore still be persisted to the user's memory while the prompt/tests assume the surface is impossible; either block memory-write native tools for flows or weaken this to builder guidance.
Useful? React with 👍 / 👎.
Summary
workflow_builderstanding prompt told the builder anagentnode "can also read and write the user's memory at run time". Both halves are false, and the failure mode is fabrication rather than an error.agent_memory— see Impact for the injection reason.Problem
A plain
agentnode (noagent_ref) is a single completion throughOpenHumanLlm::complete— no tool loop. It cannot read memory and it cannot write memory.The prompt said otherwise, so when a user asked for a memory-aware workflow the builder authored a plain agent node prompted to "recall the user's preference". The model has no retrieval path, so it invents the value. The node succeeds, the graph validates, the run completes — and the workflow acts on fabricated context.
That is worse than a broken step: a failure is visible, a fabrication is not. It has also been self-contradictory since #5120, whose own text four lines below states a plain agent node "cannot run code, browse the web, or reach any domain-specific tool".
Solution
Two mechanisms genuinely reach memory at flow run time, and the prompt now teaches both:
tool_callnode withconfig.slug=oh:memory_recall/oh:memory_hybrid_search— one deterministic read at a fixed point. A native tool result is aToolResult({ content: [{ type, text }], is_error }), so the downstream binding is=nodes.<id>.item.json.content[0].text, not the bare.item.json.<field>an agent/http_requestoutput uses. Getting that path wrong reproduces the same silent-null class the=-binding rules exist to prevent, so it is spelled out.config.agent_ref = "context_scout"when the step must decide what to look up, or look up several things — that routes throughrun_via_harness(feat(flows): teach the workflow builder to pick specialist agents via agent_ref (B37) #5120/fix(agents): run custom registry agents with their real tools (flows + chat + tasks) #5121) to a real read-only tool loop with memory recall, transcript search, and thread reads.context_scoutwas also added to theagent_refworked examples, which previously listed onlycode_executorandresearcher.Submission Checklist
agent_memorycontractprompt.md(data,include_str!-asserted by all 3 tests) and the tests themselvesN/A: behaviour-only change to agent prompt guidance## Related—N/A: no matrix feature row covers builder prompt copyN/A: no release-cut surface touchedCloses #NNN— N/A: no issue filed; bug found while auditing the flows/memory integrationImpact
Behaviour-only change to builder guidance. No runtime, schema, or API change. Existing saved flows are untouched; newly authored ones stop containing no-op memory steps.
Security — why
agent_memoryis deliberately not recommended. It looks like the obvious pick: a registered builtin,sandbox_mode = "read_only", memory tools in its belt. But itsmemory_treetool does not overridepermission_level(), so it inherits the trait defaultPermissionLevel::ReadOnlywhile dispatching aningest_documentmode that writes to the tree. The read-only tool filter insession/builder/factory.rsconsults the arglesspermission_level(), somemory_treesurvives that filter.Flows run on trigger data a third party can influence (an inbound email, a webhook payload). Pointing the builder at
agent_memorywould therefore hand prompt-injected content a memory-write foothold — precisely the holecontext_scout's ownagent.tomldocuments refusing for the same reason. This PR routes around it; the underlying tool-permission gap is pre-existing, out of scope here, and worth its own issue.Related
Follows #5120 (specialist selection via
agent_ref) and #5121 (custom registry agents run with real tools), which together made the harness path the real mechanism this prompt now points at.