feat(agent-harness): memory read→dedupe→write→update-index protocol — rework of #4388 - #4444
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 12 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 (1)
📝 WalkthroughWalkthroughIntroduces a new memory-protocol enforcement module that classifies memory-related tool calls and tracks a read→write→update-index cycle. Wires a new ChangesMemory Protocol Enforcement
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Harness as Turn Harness
participant Middleware as MemoryProtocolMiddleware
participant Tracker as MemoryProtocolTracker
participant ToolResult as Tool Result
Harness->>Middleware: before_tool(call_id, tool_name, args)
Middleware->>Middleware: classify_memory_op, store pending
Harness->>ToolResult: execute tool
ToolResult-->>Harness: result
Harness->>Middleware: after_tool(call_id, result)
Middleware->>Tracker: observe(op) if no error
Tracker-->>Middleware: observation (missing_index_read, index_drift)
Middleware->>ToolResult: append guidance text
Harness->>Middleware: after_agent()
Middleware->>Tracker: pending_index_update()
Tracker-->>Middleware: true/false
Middleware-->>Harness: emit warning if stale
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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: fa69e393b1
ℹ️ 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".
| } | ||
| // The always-on closing-step reminder: keep the index in sync. | ||
| parts.push( | ||
| "After mutating memory, call `update_memory_md` to keep the MEMORY.md index in sync." |
There was a problem hiding this comment.
When a successful memory write occurs, this guidance tells the model to call update_memory_md, and that is the only operation that clears pending_index_update. In the current shared tool registry (all_tools_with_runtime), UpdateMemoryMdTool is only defined/re-exported and is never inserted into the tool vector, so normal agent turns are directed to an unavailable tool and will keep accumulating stale-index warnings after memory writes. Register the tool in the shared tool set or only emit this protocol when the tool is actually exposed.
Useful? React with 👍 / 👎.
| // update-index cycle and append a corrective note when a write skips the | ||
| // dedupe read or leaves the index stale. Pushed first / outermost so its | ||
| // `after_tool` runs *after* the byte-cap truncation, keeping the note. | ||
| harness.push_middleware(Arc::new(middleware::MemoryProtocolMiddleware::new())); |
There was a problem hiding this comment.
This middleware is pushed before context_mw.install(...), so its after_tool runs before the existing tool-output cap/summarization middleware in the same registration-order stack used by HandoffMiddleware and ToolOutcomeCaptureMiddleware. For memory writes with oversized results, ToolOutputMiddleware can then replace or truncate result.content after the memory-protocol note is appended, dropping the corrective note this comment says should survive; push this after the output-budget middleware (while still before outcome capture) or protect the note from capping.
Useful? React with 👍 / 👎.
…dex protocol (tinyhumansai#4116) Rework of tinyhumansai#4388 onto post-tinyhumansai#4399 code. tinyhumansai#4399 deleted the original home (agent/harness/memory_protocol.rs did not exist yet; the enforcement was to live in the turn engine); the turn loop now runs on the tinyagents crate, so this re-expresses the protocol as a crate middleware. - Ports the pure `MemoryProtocolTracker` state machine as src/openhuman/agent/harness/memory_protocol.rs: classifies each memory tool call (IndexRead / Write / IndexUpdate / Other) and, on a write, flags a missing dedupe read or a stale index (a prior write never followed by update_memory_md). - Adds `MemoryProtocolMiddleware` in the tinyagents adapter seam: before_tool captures the classified op by call id; after_tool advances the tracker on success and appends a `[memory-protocol]` corrective note to the tool result so the model is nudged back onto the cycle; after_agent warns when the run ends with a stale index. Registered outermost in assemble_turn_harness so its after_tool runs after the byte-cap truncation, preserving the note. - Fixes a stale classification from the original PR: `memory_search` is not a real tool — replaced with the actual dedupe-read search tools (memory_vector_search / memory_chunk_context / memory_hybrid_search). 9 unit tests (classification + tracker ladder) + 4 middleware integration tests (corrective note, SKILL.md doesn't close the cycle, consolidated memory_tree ingest = write, failed write doesn't advance). All green.
fa69e39 to
b17780c
Compare
coderabbitai
Bot
added
agent
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
…entory The memory-protocol enforcement middleware adds one lifecycle middleware (registered outermost), so the adapter-inventory counts move 12→13 (window present) and 10→11 (no window). Updates the registration-order comment too.
…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
…teering Six Rust test failures surfaced by clean CI (merged without test parity): - middleware (production regression, tinyhumansai#4473): the no-progress ladder's Nudge sent SteeringCommand::Redirect, which is NOT in the Interactive steering allowlist (InjectMessage + Pause only). Every interactive turn where a tool failed twice with identical args or four times with varied errors aborted the whole turn with a Steering error instead of nudging then halting gracefully. Switch the nudge to InjectMessage(system) — equivalent (append + Continue) but within the interactive policy. Fixes the three agent *_raw_coverage_e2e panics (turn_xml_failures…, bus_turn_halts_on_repeated_tool_error…, no_progress_guard_uses_default_iteration_fallback_when_zero). - config schema catalog test: privacy-mode controllers (config_get/set_privacy_mode, added by tinyhumansai#4435/tinyhumansai#4446) were registered but the hand-maintained golden list in config_auth_app_state_connectivity_e2e.rs wasn't updated. Add the two entries. - api::config backend_url test: tinyhumansai#4153 intentionally made a bare `/v1` base on an unknown host classify as an OpenAI-compatible inference base (with its own passing sibling test); the older contradictory assertion wasn't updated. Align it to expect the fallback. - tinyagents middleware inventory tests: tinyhumansai#4444 added MemoryProtocolMiddleware (+1) and tinyhumansai#4473 removed CacheAlignMiddleware (-1), but the count literals were left at 13/11. Correct to 12/10 and drop cache-align from the comment. Claude-Session: https://claude.ai/code/session_014RLnG2QbdL3n9TLtfomdhB
2 participants
Summary
Rework of #4388 onto post-#4399
main. Enforces the read-index → dedupe → write → update-index memory protocol: on each successful memory write the agent gets a[memory-protocol]corrective note if it skipped the dedupe read or leftMEMORY.mdstale, and the run warns at the end if an index update was never issued.Why a rework
The turn loop now runs on the
tinyagentscrate (#4399), so the protocol is re-expressed as a crate middleware in the adapter seam rather than in the turn engine.Changes
src/openhuman/agent/harness/memory_protocol.rs— pureMemoryProtocolTrackerstate machine:classify_memory_op(name, args)→IndexRead / Write / IndexUpdate / Other, and the ladder that flagsmissing_index_read/index_drifton writes. No harness deps → fully unit-testable.MemoryProtocolMiddleware(src/openhuman/tinyagents/middleware.rs) —before_toolcaptures the op by call id,after_tooladvances the tracker on success and appends the corrective note,after_agentwarns on a stale index. Registered outermost inassemble_turn_harnessso itsafter_toolruns after the byte-cap truncation (note survives).memory_searchis not a registered tool — replaced with the real dedupe-read search tools (memory_vector_search/memory_chunk_context/memory_hybrid_search).Tests
9 unit tests (classification + tracker) + 4 middleware integration tests (corrective note appears; SKILL.md update does not close the memory cycle; consolidated
memory_treeingest treated as a write; a failed write does not advance the protocol). All green.Related
main; merge fix(orchestration): repair TinyAgents-migration build break (main does not compile) #4442 first.Summary by CodeRabbit