fix(memory): write-time auto-consolidate on overflow - #4
Merged
Conversation
When memory.add() would push MEMORY.md past the configured cap, the tool now shells out to ~/.hermes/scripts/memory-auto-compress.py to free up space, then re-measures and proceeds if room was made. This prevents the 'memory full, model trims across multiple turns' failure mode that has recurred since 2026-06-26. If the compress script is missing or returns nonzero, the call falls through to the existing consolidation_failure response (back-compat for environments without the script). Adds two tests in tests/tools/test_memory_tool.py: - test_add_overflow_triggers_auto_consolidate - test_add_overflow_falls_through_when_no_compress_script Refs: skills/hermes-memory-self-management (1.2.0 changelog documents the same pattern as the canonical fix; this commit ships it on this install after the original PR was closed upstream).
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Pull request overview
This PR improves the memory tool’s ergonomics by attempting an automatic on-disk consolidation when memory.add() would exceed the configured character cap, avoiding a hard rejection that forces manual trimming mid-turn.
Changes:
- Add
MemoryStore._auto_consolidate()and invoke it onadd()overflow before returning the existing consolidation failure response. - Shell out (with a timeout) to a profile-scoped
memory-auto-compress.pyscript to try to make room. - Add regression tests intended to cover the new overflow→auto-consolidate and overflow→fallthrough behaviors.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| tools/memory_tool.py | Adds write-time auto-consolidation attempt before rejecting overflow writes. |
| tests/tools/test_memory_tool.py | Adds tests for overflow-triggered auto-consolidation and missing-script fallthrough. |
Comments suppressed due to low confidence (3)
tests/tools/test_memory_tool.py:951
- The entry being added here is too small to guarantee
new_total > limitformemory_char_limit=2200, which can make the test pass/fail depending on delimiter accounting and file parsing. Use a deterministic oversized entry so the overflow branch (and auto-consolidate) is definitely exercised.
result = s.add("memory", "new entry after auto-consolidate")
tests/tools/test_memory_tool.py:965
- This test also doesn’t reliably hit the overflow branch: 1800 chars + delimiter + a short entry is still under the 2200 cap, so
add()may succeed and the expected consolidation-failure assertion will fail. Increase the initial file size (and/or the added entry size) sonew_total > limitis guaranteed before checking the failure behavior.
monkeypatch.setattr("tools.memory_tool.get_memory_dir", lambda: tmp_path)
big = "x" * 1800
(tmp_path / "MEMORY.md").write_text(big + "\n", encoding="utf-8")
s = MemoryStore(memory_char_limit=2200)
s.load_from_disk()
tests/tools/test_memory_tool.py:975
- Use an entry large enough to ensure the attempted add would exceed the configured cap, so the test exercises the overflow/fallthrough path deterministically.
result = s.add("memory", "new entry that won't fit")
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
37
to
+41
| from utils import atomic_replace | ||
| from tools.threat_patterns import first_threat_message as _first_threat_message | ||
| from tools.registry import registry, tool_error | ||
|
|
||
| log = logging.getLogger(__name__) |
Comment on lines
+204
to
+210
| if r.returncode == 0: | ||
| # Re-measure; caller will check whether it freed enough | ||
| return self._char_count(target) < int(os.environ.get("HERMES_MEMORY_LIMIT", "6000")) | ||
| return False | ||
| except (subprocess.TimeoutExpired, OSError) as e: | ||
| log.debug("memory auto-consolidate failed: %s", e) | ||
| return False |
Comment on lines
+981
to
+985
| def test_add_overflow_triggers_auto_consolidate(self, tmp_path, monkeypatch): | ||
| """When add() would push MEMORY.md past cap, _auto_consolidate is | ||
| called and the add succeeds if compress freed enough space. | ||
|
|
||
| The compress script is shelled out to. We stub it with a fake that |
Comment on lines
+929
to
+935
| import subprocess | ||
| monkeypatch.setattr("tools.memory_tool.get_memory_dir", lambda: tmp_path) | ||
| # Cap is 2200 by default. Fill MEMORY.md close to cap, then add a | ||
| # large entry that would push past. Expect auto-consolidate to fire. | ||
| big = "x" * 1800 | ||
| (tmp_path / "MEMORY.md").write_text(big + "\n", encoding="utf-8") | ||
| s = MemoryStore(memory_char_limit=2200) |
The two auto-consolidate tests in test_memory_tool.py were using a 30-char entry on a 1800-char file with a 2200 cap, so new_total ended up at ~1833 chars and the cap-check never fired. The first test failed with 'auto-consolidate was not triggered'; the second returned success=False was unreachable because add() never reached the cap-check. Bump the existing entry to 1900 chars and the new entry to 600 chars so new_total = 2503 chars > 2200 cap, reliably forcing the cap-check to fire and exercise _auto_consolidate in both tests. Also: the previous append had duplicated the two tests in the file (an execute_code retry side effect). This commit removes the duplicates and leaves one fixed copy of each test.
Three issues from PR review: 1. _auto_consolidate() was using HERMES_MEMORY_LIMIT env var (hardcoded 6000 fallback) to decide 'consolidation succeeded'. Wrong: cap differs per target (memory vs user) and per config. Replaced with a simple 'file got smaller' check (before vs after) — the caller (add) already re-measures against the per-target cap under the file lock. 2. Duplicate import 'from tools.threat_patterns import first_threat_message' at line 90 was redundant; the same import already exists at line 38. Removed the duplicate. 3. Duplicate import 'from tools.registry import registry, tool_error' at line 39 was redundant; the same import is at the bottom of the file under '# --- Registry ---'. Removed the top-level copy. All four imports now appear exactly once: subprocess, sys, first_threat_message, registry. py_compile passes.
bbasketballer75
added a commit
that referenced
this pull request
Jul 24, 2026
Originally _auto_consolidate measured return value by comparing self._char_count(target) before and after running the compress script. But _char_count() reads from the cached self._entries list (loaded by load_from_disk), not from the live file. So the script would shrink the file on disk, but the cache would still hold the pre-consolidate entries, and the function would return False (before == after) — making auto-consolidate a no-op. add() then sees the failure and falls through to the standard consolidation_failure path, defeating the whole purpose. Fix: after the script returns success, call self._reload_target to refresh the cache from disk. Return True unconditionally on script-success (the caller re-measures against the cap anyway). Caught by the test_add_overflow_triggers_auto_consolidate test exercising the whole flow end-to-end. The earlier cherry-picked PR #4 (commit 065ed80) had this same bug; the test would have gone red on CI if the consolidation path had been exercised. Also: removed the misleading 'before < after' check that was always False on a cache-backed store, and clarified the docstring to say the cap comparison is the caller's responsibility. Tests: 87/87 pass in test_memory_tool.py.
bbasketballer75
added a commit
that referenced
this pull request
Aug 1, 2026
…he fork CI (#22) * fix(memory): import Tuple — auto-consolidate NameError broke fork CI The write-time auto-consolidate code (merged to this fork's main via #4) annotates two nested helpers with Tuple[...] at memory_tool.py:813 and :847, but the module imports only Dict, Any, List, Optional. The def statements execute when the enclosing consolidation path runs, so every memory write that reached consolidation raised: NameError: name 'Tuple' is not defined This is fork-only code — origin/main imports Tuple — which is why the failures appeared on fork CI (PR merge refs test against this fork's main) yet never reproduced on origin/main-based local checkouts, and why they masqueraded as flakes across slices 1/5/7/8: different tests hit the consolidation path in different slices. Also downstream of the same error: skip_memory store tests (load path swallowed the NameError into the agent-init try/except, leaving _memory_store None) and the 413 compression memory-snapshot tests. Verified deterministically: on this base, the fcntl-fallback, learning_mutations, skip_memory, and 413_compression families fail in 1.2 s without this import and pass (33/33) with it. ruff --select F821 confirms Tuple was the only undefined name in the file. * test: isolate the tool-definition caches between tests test_background_review_installs_thread_local_whitelist fails intermittently in CI with: AssertionError: assert 'memory' in {'skill_manage'} while passing in isolation, in its own file, and on rerun. It is not a stale CI cache — it is order-dependent process state. The whitelist under test comes from get_tool_definitions(enabled_toolsets=["memory", "skills"]), which depends on two module-level caches that outlive a single test: * tools.registry._check_fn_cache — per-check_fn verdicts, 30 s TTL, keyed by function object. An earlier test that probes a memory/skills check_fn while the feature looks unavailable stamps False in for the next 30 s. * model_tools._tool_defs_cache — memoized definition lists. Its key covers registry._generation and the config fingerprint but NOT the check_fn verdicts resolved underneath, so a poisoned entry is invisible to it. Reproduced deterministically by stamping False into the TTL cache: memory, skill_view and skills_list all drop out of the computed toolset, and the test fails with exactly the CI assertion. Restoring the entries makes it pass. Adds a suite-wide autouse fixture that drops the False verdicts (a cached True cannot produce this failure mode, and keeping those avoids re-probing every available tool) and clears the definition memo, which must go entirely because its key cannot see the verdicts. tests/test_get_tool_definitions_cache_isolation.py already established this pattern per-file for _tool_defs_cache; this lifts it suite-wide and adds the check_fn cache, which is the one carrying the poisoned verdict. 116 test files touch tool definitions or the registry, so per-file fixtures could not cover the exposure. Verified against tests/run_agent/ + tests/tools/test_registry.py: identical pass/fail counts before and after (21 pre-existing Windows-only failures unchanged), and the poisoned-neighbour reproduction now passes. * test: record the measured cost of the cache-isolation fixture Measured on tests/run_agent/ + tests/tools/test_registry.py (1271 passing): no fixture 515 s blanket clear 606 s clear False verdicts 602 s The "drop only False verdicts" variant was written on the theory that re-probing available tools' check_fns was the expense. It is not — it saved 4 s, inside noise. The cost is dominated by rebuilding _tool_defs_cache, which cannot be preserved because its key does not cover the verdicts underneath it. Reverted to the simpler blanket clear (same cost, less to explain) and recorded the numbers in the docstring so the next person does not retry the same optimization.
bbasketballer75
pushed a commit
that referenced
this pull request
Aug 11, 2026
…on delegation callbacks (NousResearch#82592) * fix(gateway): stop frozen-preview finals and dropped idle-session delegation callbacks Two relay-plane delivery losses from the 2026-08-09 staging incident: 1. stream_consumer: the skip-redundant-finalize branch recorded _accumulated as the delivered turn-final payload even when the last ACKED edit was an earlier throttled preview snapshot, so delivered_final_matches reconciled True and the gateway suppressed the corrective final send — the user was left with a cut-off message ending in the streaming cursor. Extracted _mark_skip_redundant_finalize(): records the last acked wire payload (cursor-stripped), so a preview/final mismatch now returns False and the normal final send fires. 2. run.py: _classify_completion_target classified every ended parent session terminal unless it ended by compression. Idle/timeout session ends are the norm on scale-to-zero relay deployments and the chat route remains valid; completed async delegation results were terminally dropped. Ended parents now classify deliver unless the end was an explicit user boundary (session_reset / user_exit / session_switch). * fix(relay): drain in-flight outbound frames before transport teardown disconnect() failed every pending outbound future immediately with 'relay transport closed', so a trailing finalize edit racing turn teardown was lost even though the connector socket could still serve it. Bounded drain grace (5s) lets in-flight requests resolve; silent connectors still tear down promptly. asyncio.wait (not gather+wait_for) so a timeout doesn't cancel futures owned by the fail-remaining loop. * fix(gateway): route completion injection through the alias-aware transport resolver Third relay-plane delivery loss from the 2026-08-09 staging incidents: a delegation batch completed while the gateway was up, the watcher drained the event, and delivery vanished with no log line. _inject_watch_notification resolved its adapter with a literal p.value == platform_name scan of self.adapters — a relay-fronted gateway registers ONE adapter under Platform.RELAY fronting N logical platforms, so 'slack' never matched and the injection returned None ('no gateway route'), silently dropping the completion. The handoff path already documents this exact trap and uses resolve_delivery_transport; the injection path now does the same (native wins; relay eligible only when it fronts the logical platform), with the literal scan kept as fallback for stub runners and exotic platforms. * fix(relay): clamp disconnect drain grace to the runner's adapter-disconnect budget Review finding (JoaoMarcos44, NousResearch#82592): a fixed 5.0s drain in front of the three 1.0s sequential teardown awaits gives an 8.0s worst case inside the runner's 5.0s asyncio.wait_for(adapter.disconnect()) — tripping it cancels teardown mid-drain, skips the fail-pending loop, and leaves outbound callers blocked until _OUTBOUND_TIMEOUT_S (30s). The effective grace is now budget - 3*TEARDOWN - margin (env-aware via the same HERMES_GATEWAY_ADAPTER_DISCONNECT_TIMEOUT the runner reads), so the drain can never push teardown past its caller's budget; a budget too small for any drain disables it cleanly. * test(gateway): pin the final-send suppression contract across a behaviour matrix The gateway skips its own final send when the stream consumer claims the turn final already reached the user. Every incident in that family — NousResearch#71643 (stale finalize snapshot), NousResearch#78541 (payload-less multi-message split), NousResearch#82656 (frozen preview left with a visible cursor) — is the same failure: the consumer claimed delivery for text the platform never rendered, so the corrective send was suppressed and the answer was lost with no retry. Each was fixed with a scenario test pinned to one branch of GatewayStreamConsumer.run(). The got_done handler now has five sibling branches that each set the suppression flags and record a turn-final payload, and nothing checks them as a group: a new branch, or a new early `return True` in _send_or_edit, can reintroduce the class without failing a test. Pin the invariant instead of the branch — if the consumer offers the gateway any signal it would trust, the complete final text must have reached the wire — and assert it across {edit always / dies / never / lies} x {send always / never} x {fresh-final on / off} x {clean / interrupted stream}. The adapter records only frames that actually rendered, so an ACK the platform drops does not count as delivery. 24 honest-transport scenarios hold the invariant as a hard assertion. The 16 lying-transport scenarios are checked too; the single combination that still violates it is reported as an expected failure documenting the open exposure rather than asserting it away. Refs NousResearch#82656 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(gateway,relay): prime relay egress routing for synthetic injections + cap stale completion replay Defect #4 from the 2026-08-09 staging incidents (upgrade-robustness): after every gateway restart the durable async-delegation replay injected completions correctly (post-741663cf1) but their replies bounced at the connector — 'slack egress declined: target not routed to an onboarded tenant'. The relay adapter re-attaches tenant discriminators (metadata.scope_id / metadata.user_id) from per-chat caches warmed ONLY by inbound traffic; synthetic turns race those cold caches on every deploy, scale-to-zero wake, and crash recovery. - relay adapter: prime_routing_cache() — feeds a synthetic event's session-store origin through the same _capture_scope used for real inbound (never raises). - run.py injection path: prime the resolved adapter before handle_message (duck-typed; native adapters unaffected). - async_delegation: 48h staleness cap in restore_undelivered_completions — a pending completion older than the cap is terminally dropped (payload stays queryable) instead of re-run as a fresh full-context turn; the post-restart replay of a July session burned a 102K-token context. Also carried: JoaoMarcos44's suppression behaviour-matrix harness (cherry-picked from NousResearch#82676, authorship preserved) — 39 passed + 1 xfail (the documented ACK-then-drop transport-honesty residue). * test: use recent timestamps in restored-ownership fixtures test_restore_stamps_restored_flag persisted its completion with epoch-era toy timestamps (dispatched_at=1.0), which the new 48h replay staleness cap correctly classifies as stale — the fixture then exercised the cap instead of the restored-flag contract (CI slice 4 failure). Timestamps are now now-relative; the staleness behavior itself is pinned separately in test_relay_injection_egress_priming.py. * fix(gateway,relay): close four review findings on the relay delivery fixes Review follow-ups on this branch (NousResearch#82592): 1. HIGH — classifier/resolver mismatch (falsely-acknowledged loss). _classify_completion_target now returns "deliver" for idle-ended parents, but _resolve_async_delegation_session still dropped every non-compression-ended pin: the durable row was acked at adapter acceptance, then the injection died inside the pipeline with no retry — strictly worse than the honest terminal drop on main, and the delivery leg defect #2's fix depends on did not exist. The resolver now retargets non-user-boundary ends (idle/timeout/ lifecycle) to the chat's current session — session_entry already IS the routing key's current session for the same chat — while user boundaries (session_reset / new_session / user_exit / session_switch) stay fail-closed. Both sides share one module-level _USER_BOUNDARY_END_REASONS so the verdict and the routing decision cannot drift again; a coherence test asserts deliver-verdicts resolve non-None across representative end reasons. 2. HIGH — drain clamp missed adapter-level spend. The effective drain grace budgeted drain + 3x teardown, but RelayAdapter.disconnect spends revocation-monitor teardown + go_idle time BEFORE the transport drain inside the same runner wait_for; worst case still blew the budget and cancelled teardown mid-drain (skipping the fail-pending loop). The adapter now measures its own elapsed time and threads the REMAINING budget into transport.disconnect(budget_s=...); legacy/stub transports without the keyword fall back to the no-arg signature. 3. P1 — _request_response racing disconnect() could register a future after the fail-pending loop already ran, stranding the caller for the full _OUTBOUND_TIMEOUT_S (30s). Fail fast with the same "relay transport closed" error once _closing is set. 4. P1 — _build_process_event_source's last-resort reconstruction dropped scope_id, so a scoped relay completion whose session-store origin was unavailable primed no tenant discriminator and could still bounce off the connector's fail-closed egress guard. scope_id now threads through the reconstructed SessionSource, with a warning when a scoped chat reconstructs without one. All four: RED reproduced with the fix reverted, GREEN after; relay/ delegation delivery families pass (43 + 71 + 179 across the touched suites); full tests/gateway run shows only failures already failing identically on merge base 2446c8b (env/dep issues). * fix(gateway,relay): make pending-frame failure cancellation-safe; persist completion routing origin Two remaining review findings on this branch (NousResearch#82592): 1. Cancellation could strand outbound waiters past the fail-pending loop. transport.disconnect() failed pending futures only at the END of the drain + three teardown awaits; a cancellation landing mid-drain (the runner's wait_for budget, an outer cleanup deadline) skipped the loop entirely and left registered futures unresolved — their callers blocked until _OUTBOUND_TIMEOUT_S (30s). The budget threading added earlier shrinks the window but is not a hard guarantee. The fail-pending loop (and the going_idle ack failure) now run in a `finally`, so no exit path — normal, error, or cancelled — can leave a registered future unresolved. Idempotent: done futures are skipped, a second disconnect() pass is a no-op. 2. Durable completions did not persist their routing origin, so the scope_id threading in the fallback SessionSource reconstruction had nothing to carry on the exact path it exists for (restart replay with session store + source cache gone): the async-delegation event producers never populated scope_id and the durable rows never stored it. Dispatch now snapshots the originating turn's scope_id/user_id/user_name from the session context (_capture_routing_origin — a new HERMES_SESSION_SCOPE_ID contextvar bound by the gateway at session-bind time alongside the existing vars), stores them in the existing task_json payload (no schema migration), and re-attaches them to all three completion-event shapes (live single, live batch, crash-recovery rebuild). The gateway's fallback reconstruction then primes both discriminators after a restart. Tests: cancellation mid-drain -> every pending future resolves with "relay transport closed" (mutation: moving the loop out of the finally goes RED); second-pass disconnect idempotence; end-to-end dispatch -> owner-death recovery -> event carries scope_id -> fallback SessionSource primes it (mutations: dropping the dispatch capture or the task_json persistence both go RED); live completion event carries the origin. 94 passed + 1 xfailed across the delivery/delegation suites; tests/tools delegation family 73 passed (2 collection errors pre-existing on merge base 2446c8b). --------- Co-authored-by: joaomarcos <joaomarcosdias444@gmail.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Ben Barclay <ben@nousresearch.com>
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
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.
Problem
When
memory.add()would push MEMORY.md past the configured cap, the tool returned a hard rejection that forced the model to mid-flow trim — burning 1-2 turns of context on a manualpatch/removedance. This recurrence has been flagged by Austin since 2026-06-26 across sessions.Fix
Add
_auto_consolidate(target)toMemoryStore. Called fromadd()right before the cap-rejection return, it shells out to~/.hermes/scripts/memory-auto-compress.py(the same script the weekly cron uses) with a 5s timeout. If the script frees enough space, the call re-measures under the file lock and the add proceeds with a success response. If not, it falls through to the existingconsolidation_failure(back-compat for environments without the script).Note on PR #3
A previous PR (#3) for the same fix was force-merged into the fork on 2026-07-24 due to a local force-push accident. It is left MERGED on the record but the auto-merge did not go through a proper review. This new PR (#4) re-files the same patch with the head branch properly isolated, ready for proper review.
Files
tools/memory_tool.py— addsimport subprocess,import sys,log = logging.getLogger(__name__), the_auto_consolidatemethod, and a call to it in theadd()cap-check.tests/tools/test_memory_tool.py— addstest_add_overflow_triggers_auto_consolidateandtest_add_overflow_falls_through_when_no_compress_script.Verified
python -m py_compile tools/memory_tool.pyexits 0python -m py_compile tests/tools/test_memory_tool.pyexits 0load_on_disk_store(): 1050-char add on 3535-char MEMORY.md (cap 6000) succeeded with auto-consolidatememory.addpath