fix(agent-harness): state memory read→dedupe→write→update contract to the model (#4116)#4476
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 32 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)
📝 WalkthroughWalkthroughUpdates the description strings of the ChangesMemory Protocol Tool Description Updates
Estimated code review effort: 1 (Trivial) | ~3 minutes Possibly related issues
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
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: cae7444e2c
ℹ️ 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".
| "[tinyagents] appending memory-protocol run-end drift note to turn text" | ||
| ); | ||
| text.push_str("\n\n"); | ||
| text.push_str(¬e); |
There was a problem hiding this comment.
When this drift slot is set, the warning is appended only to the local text. The returned history/conversation are still derived from run.messages, and callers such as the chat/session path persist outcome.conversation into the transcript/history before returning outcome.text, so a normal turn that ends with an unreconciled memory write can show the note in the returned reply while storing the original assistant message. On the next turn the model will not see the stale-index warning and the persisted transcript can disagree with what was returned; fold the note into the final assistant message/conversation before conversion/persistence.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — resolved by removing the run-end surfacing from this PR. AgentRun::text() reads final_response while history/conversation derive from run.messages, so appending only to the local text diverged from the persisted transcript exactly as you noted. A consistent model-visible surfacing needs to mutate both final_response and the final assistant message in messages; I've scoped this PR down to the up-front prompt-scaffold (the genuinely-missing half of #4116) and left the run-end model-visible surfacing as a follow-up. The run-end case keeps its tracing::warn! for now.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/openhuman/memory/tools/forget.rs (1)
27-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a description-contract test for
memory_forget, mirroringstore.rs.Updated the
name_and_schematest to validate thattool.description()includes bothmemory_recallandupdate_memory_md, with a failure message describing the required read→dedupe→write→update contract.forget.rs's own tests weren't updated with an equivalent assertion that the description mentionsupdate_memory_md, leaving the new contract wording unguarded against future regressions here.✅ Suggested test addition
#[test] fn name_and_schema() { let (_tmp, mem) = test_mem(); let tool = MemoryForgetTool::new(mem, test_security()); assert_eq!(tool.name(), "memory_forget"); + let desc = tool.description(); + assert!( + desc.contains("update_memory_md"), + "memory_forget description must instruct calling update_memory_md: {desc}" + ); }🤖 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/memory/tools/forget.rs` around lines 27 - 31, Add a description-contract test for `MemoryForget` in the same style as `store.rs` so the tool’s `description()` is explicitly validated. Update the `memory_forget` tests to assert that `MemoryForget::description` includes `update_memory_md` (and the full contract wording if applicable), using the existing `name_and_schema`/description test pattern to guard against regressions in `MemoryForget`.
🤖 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.
Nitpick comments:
In `@src/openhuman/memory/tools/forget.rs`:
- Around line 27-31: Add a description-contract test for `MemoryForget` in the
same style as `store.rs` so the tool’s `description()` is explicitly validated.
Update the `memory_forget` tests to assert that `MemoryForget::description`
includes `update_memory_md` (and the full contract wording if applicable), using
the existing `name_and_schema`/description test pattern to guard against
regressions in `MemoryForget`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c47f477c-eecc-4397-a09f-f118262219a0
📒 Files selected for processing (4)
src/openhuman/memory/tools/forget.rssrc/openhuman/memory/tools/store.rssrc/openhuman/tinyagents/middleware.rssrc/openhuman/tinyagents/mod.rs
…t to the model (tinyhumansai#4116) The memory-protocol enforcement middleware (tinyhumansai#4444) corrects violations *after* they happen (a duplicate write, a drifted index), but the read→dedupe→write→update-index contract was never actually stated to the model in the live prompt — `memory_protocol.rs`'s "agents are instructed to follow…" doc was aspirational; no prompt text described the sequence. So dedupe was only ever nudged *after* a duplicate write landed, never prevented. State it up front in the two durable-write tools' descriptions: - `memory_store`: recall existing memory (e.g. `memory_recall`) to check for a near-duplicate before storing; call `update_memory_md` after. - `memory_forget`: call `update_memory_md` after removing an entry. Test: `memory_store`'s `name_and_schema` asserts the description carries the contract, so the up-front instruction can't silently regress. The run-end stale-index case remains a `tracing::warn!` (the issue's "correct or warn"); surfacing it model-visibly needs consistent `final_response` + `messages` mutation and is left as follow-up. Claude-Session: https://claude.ai/code/session_01KcmdqJVpjmnH31HqTHRLwG
senamakel
force-pushed
the
fix/GH-4116-memory-protocol-scaffold
branch
from
cae7444 to
6ea4d86
Compare
senamakel
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6ea4d86cb1
ℹ️ 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".
| Memory protocol: before storing, recall existing memory (e.g. `memory_recall`) to check for a \ | ||
| near-duplicate so you don't create one; after storing, call `update_memory_md` to keep the \ | ||
| MEMORY.md index in sync with the store." |
There was a problem hiding this comment.
When memory_store is exposed through a named tool scope, this unconditional protocol can point the model at tools that are not actually sent to the provider: visible_tool_specs_for_policy keeps only names in the allowlist (src/openhuman/agent/harness/session/builder/mod.rs:61-63), and current named agents include memory_store without memory_recall and/or update_memory_md (profile_memory_agent lines 21-23, trigger_reactor lines 39-42, tinyplace_agent lines 52-54, archivist line 23). In those runs the model cannot satisfy the new read/update instruction, so memory writes can still leave MEMORY.md stale or lead to attempts to use unavailable tools; please add the companion tools to those scopes or make the description conditional on visibility.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in 7cf5d5f. The wording is now conditional ("if you have a memory-recall tool…", "if update_memory_md is available…") so scopes that expose memory_store without the companion tools (archivist, profile_memory_agent, trigger_reactor, tinyplace_agent) aren't pointed at tools outside their allowlist. I kept it in the description rather than editing those scopes' tool sets, since giving them recall/update tools they were deliberately not granted is a bigger behavioural change.
Unrelated CI unblock: tinyhumansai#4446 (Privacy Mode) registered `config_get_privacy_mode` / `config_set_privacy_mode` unconditionally (src/openhuman/config/schemas/controllers.rs) but did not update the `worker_a_controller_schemas_are_fully_exposed` golden in tests/config_auth_app_state_connectivity_e2e.rs, so that test fails on main for every PR that doesn't carry the fix. Add the two methods at their sorted positions (same fix as tinyhumansai#4475) so this PR's Rust E2E / coverage jobs go green. Claude-Session: https://claude.ai/code/session_01KcmdqJVpjmnH31HqTHRLwG
|
Note on CI: the last red on |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 01ef1c6419
ℹ️ 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".
| Memory protocol: after removing an entry, call `update_memory_md` to keep the MEMORY.md \ | ||
| index in sync with the store." |
There was a problem hiding this comment.
In the profile-memory flow I checked, profile_memory_agent exposes memory_forget but not update_memory_md (src/openhuman/agent_registry/agents/profile_memory_agent/agent.toml:21-23), and the harness only sends tools whose names are in the named allowlist (visible_tool_specs_for_policy in src/openhuman/agent/harness/session/builder/mod.rs:54-63). This newly added guidance therefore tells the model to call a tool it cannot see after a normal forget request, leaving the index-sync step impossible or wasting a turn on unknown-tool recovery; either include the companion tool in that scope or make the description conditional on visibility.
Useful? React with 👍 / 👎.
…tinyhumansai#4116 review) Codex review (P2): some named agent scopes (archivist, profile_memory_agent, trigger_reactor, tinyplace_agent) advertise `memory_store` without `memory_recall` / `update_memory_md`, so an unconditional "recall first, then call update_memory_md" instruction pointed those agents at tools not in their scope — the very unavailable-tool problem tinyhumansai#4118 is about. Phrase the protocol conditionally ("if you have a memory-recall tool", "if `update_memory_md` is available") so it degrades to a no-op when the companion tools aren't present. Claude-Session: https://claude.ai/code/session_01KcmdqJVpjmnH31HqTHRLwG
…4116-memory-protocol-scaffold
1 participant
Summary
memory_storeandmemory_forgettool descriptions, so duplicate entries are prevented rather than only corrected after a duplicate write lands.memory_store'sname_and_schemaasserts the description carries the contract, so the instruction can't silently regress.Problem
Issue #4116: agents skip reading the index for dedupe before writing memory, and skip
update_memory_mdafterward, soMEMORY.mddrifts and duplicate entries accumulate.The prior rework (#4444) added the enforcement state machine + a per-write
after_toolcorrection. But the issue's core complaint — "MEMORY.md not read for dedupe" — was only ever addressed reactively: the protocol was never actually stated to the model.memory_protocol.rs's doc-comment claims "agents are instructed to follow…", yet there is zero instruction text in the live prompt describing the sequence. So dedupe was only nudged after a duplicate write already happened — never prevented.Solution
Add the contract to the two durable-write tools'
description()— model-visible up front, per-tool, with no user-facing output pollution:memory_store: recall existing memory (e.g.memory_recall) to check for a near-duplicate before storing; callupdate_memory_mdafter storing.memory_forget: callupdate_memory_mdafter removing an entry.Combined with the merged #4444 enforcement (which corrects mid-run drift) and the existing run-end
tracing::warn!, this closes the issue's "read-index → dedupe → write → update-index" scaffold + post-step-check requirement.Scope note: an earlier draft of this PR also surfaced the run-end stale-index case as model-visible text. That was dropped —
AgentRun::text()readsfinal_responsewhile the persistedhistory/conversationreadrun.messages, so a consistent surfacing needs to mutate both crate-internal structures (flagged by the codex bot as a text/transcript divergence). The run-end case stays atracing::warn!(the issue's "correct or warn"); a consistent model-visible surfacing is left as a follow-up.Submission Checklist
openhuman::memory::tools::store::tests::name_and_schemaasserts the description states thememory_recall+update_memory_mdcontract (guards against silent regression).N/A: behaviour-only changeto an existing feature (memory-protocol enforcement).## Related— N/A (no new feature row).N/A: agent tool-description text only, no release-cut UI surface.Closes #NNNin## Related.Impact
Related
final_response+messagesmutation); builds on the enforcement machinery merged in feat(agent-harness): memory read→dedupe→write→update-index protocol — rework of #4388 #4444.AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
fix/GH-4116-memory-protocol-scaffoldValidation Run
pnpm --filter openhuman-app format:check— N/A (no frontend changes; pre-existing repo JS format warnings are unrelated)pnpm typecheck— N/A (no TS changes)openhuman::memory::tools::store— passed in CI's Rust Core Coverage run (store::tests::name_and_schema ... ok)rustfmt --edition 2021 --checkclean on all touched filesValidation Blocked
command:N/Aerror:N/Aimpact:N/ABehavior Changes
Parity Contract
after_toolper-write correction and run-endtracing::warn!from feat(agent-harness): memory read→dedupe→write→update-index protocol — rework of #4388 #4444 are unchanged; this PR only augments two tool description strings and adds one test assertion.Duplicate / Superseded PR Handling
Summary by CodeRabbit
Documentation
Tests