fix(agent): give the orchestrator direct memory tools so recall/store don't over-delegate#4764
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 40 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 (3)
📝 WalkthroughWalkthroughThis PR adds memory_recall, memory_store, and save_preference to the orchestrator agent's direct tool allowlist in agent.toml, updates prompt.md guidance to clarify these are direct-tool operations rather than delegated sub-agent work, and extends the loader.rs test to assert these tools are present. ChangesDirect Memory Tool Allowlisting
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related issues
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
![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: de744a3195
ℹ️ 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".
… don't over-delegate
Memory recall and store were reachable from the chat orchestrator ONLY via
sub-agent delegates (retrieve_memory -> agent_memory, manage_profile_memory ->
profile_memory_agent). So a trivial recall ("what is my Q3 OKR") spawned a
whole worker that hung (over-delegation, tinyhumansai#4744), and a simple write ("store my
Q3 OKR") spawned profile_memory_agent which — per the eval — made 0 tool calls
and returned empty, so the delegate surfaced outcome.output verbatim as a
0-char result with the write's persistence unconfirmed. The orchestrator prompt
even called the retrieve_memory DELEGATE a "direct tool", reinforcing the
wrong route.
Memory is the product's core; recall/store must be first-class DIRECT tools:
- agent.toml: add memory_recall, memory_store, save_preference to the direct
[tools].named surface (already registered in the shared pool, tools/ops.rs).
memory_recall reads the same store memory_store writes, so store->recall
closes the loop inline with a real confirmation and confirmed persistence.
These are memory-store ops, not file/shell mutation, so the chat agent's
"no write tools" rule (files/commands) is unaffected. Deep tree walks,
ingest/reconcile, people-graph, and persona edits still delegate to
retrieve_memory / manage_profile_memory.
- prompt.md: correct the mislabeled "direct" retrieve_memory -> memory_recall
and add a bullet — recall/store are direct; reserve the delegates for deep
work; never spawn a sub-agent for a single recall or "remember this".
- loader test asserts the three direct memory tools are wired.
Closes tinyhumansai#4762
M3gA-Mind
force-pushed
the
fix/GH-4762-direct-memory-tools
branch
from
de744a3 to
781563a
Compare
coderabbitai
Bot
added
agent
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
…t-store memory protocol loop A successful `memory_store` is a durable write, so the memory-protocol middleware appends a "call `update_memory_md` to keep the MEMORY.md index in sync" note to the tool result and warns at run end when the index is never reconciled. Now that the orchestrator takes the direct-store path itself (this PR), it must own that closing step — otherwise the guidance is unsatisfiable and MEMORY.md (which the orchestrator loads, `omit_memory_md = false`) drifts from the store. `update_memory_md` is locked to the MEMORY.md / SKILL.md workspace index files (enum-gated + symlink-hardened), not a general file-editor — it is the index-sync counterpart of `memory_store`, so it sits in the same memory bucket as the writes already allowlisted. The "no write tools" rule (edit / file_write / apply_patch / shell) is unaffected. - agent.toml: allowlist `update_memory_md` with rationale. - prompt.md: instruct the reconcile after a `memory_store` write (save_preference needs none — it writes the preference store). - loader.rs: assert the closing step is wired. Addresses @chatgpt-codex-connector (P2) on agent.toml — "Add index syncing for direct memory stores".
Verdict: Approve — feature satisfiedDelivers what #4762 asks.
Codex P2 thread (agent.toml:239) — addressedThe concern was valid: a direct CI is green (Rust Quality, Rust Core Coverage, all PR gates); frontend/E2E lanes correctly skipped for this Rust-config-only change. |
YellowSnnowmann
left a comment
YellowSnnowmann
left a comment
There was a problem hiding this comment.
Approving — feature satisfied, no blocker/major issues, review comments addressed in code, CI green. Detailed review in the PR comment above.
3 participants
Problem (issue #4762)
Memory ops via the agent path over-delegate and hang, and the one profile write that returned gave a 0-char result with persistence unconfirmed:
manage_profile_memory@12s, tool_result{"elapsed_ms":0,"output_chars":0}(redundantly spawned a sub-agent that did 0 sub tool calls) — write persistence unconfirmed.Root cause — openhuman agent-registry config (not tinyagents/harness)
I checked whether this was a harness matter: it isn't. The delegation machinery works; the problem is routing. The chat orchestrator has a direct read surface for files/web ("answer small lookups itself instead of spawning a sub-agent") but no direct memory tools. Memory recall/store are reachable only via sub-agent delegates:
retrieve_memory→ spawnsagent_memorymanage_profile_memory→ spawnsprofile_memory_agentSo a trivial recall spawns a whole worker (over-delegation, #4744) that hangs, and a simple write spawns
profile_memory_agentwhich (per the eval) made 0 tool calls and returned empty — the delegate then surfacesoutcome.outputverbatim (agent_orchestration/tools/dispatch.rs:247), hence theoutput_chars:0result with persistence unconfirmed. The orchestrator prompt even labels theretrieve_memorydelegate a "direct tool", reinforcing the wrong route.Fix (minimal: config + prompt + test)
Memory is the product's core — recall/store must be first-class direct tools:
orchestrator/agent.toml— addmemory_recall,memory_store,save_preferenceto the direct[tools].namedsurface. They're already registered in the shared tool pool (tools/ops.rs:337-377), andmemory_recallreads the same storememory_storewrites, so store → recall closes the loop inline with a real confirmation and confirmed persistence. These are memory-store ops, not file/shell mutation, so the chat agent's "no write tools" rule (which is about editing files / running commands) is unaffected. Deep memory-tree walks, ingest/reconcile, people-graph, and persona edits still delegate toretrieve_memory/manage_profile_memory.orchestrator/prompt.md— correct the mislabeled "direct"retrieve_memory→memory_recall, and add a bullet: recall/store are direct; reserve the delegates for deep work; never spawn a sub-agent for a single recall or "remember this".agents/loader.rs— theorchestrator_has_chat_hint_and_named_toolstest now asserts the three direct memory tools are wired (fails before, passes after).Why this resolves both asks
memory_store/save_preferencecall that runs synchronously, returns a real confirmation (non-zero output), and persists before returning. Direct-tool persistence itself is already covered by the existing unit tests inmemory/tools/store.rs+recall.rs.The
manage_profile_memorydelegate remains for deep-only requests; its empty-output edge is no longer on the hot path for trivial store/recall.Checks
Per the active no-local-builds directive (disk pressure), CI verifies — no
cargo/pnpmrun locally.Closes #4762
Summary by CodeRabbit
New Features
Bug Fixes