Escalation ladder: measure it, then skip only the futile case (2.13) - #187
Merged
Conversation
…2.13) The plan's instruction was "do not touch this without a large-owner arm", so the arm was the missing prerequisite and got built first rather than treated as a blocker: 400 crowding neighbours, a 20-row starved owner that loses the global top-K, and an owner holding nothing. Measured: the empty owner returns 0 rows at every rung, and the starved owner has 10 of its 20 rows recovered. So the middle rung stays. Removing it unconditionally would cost the starved owner its rescue, which is precisely the trade the note warned about and precisely what a plausible- sounding argument would have got wrong. What the measurement does license is a different optimisation. The distinction worth acting on is not "did the search return nothing" but "does this owner have anything to find" -- and only the second is answerable cheaply, because it is bounded by one owner's data instead of the corpus. For an owner holding nothing, the escalation pays a widened probe asking the global index for up to 2,000 candidates to find rows that do not exist, then scans. OwnerRowExistence.Any is that probe: LIMIT 1, not a count, because the question is existence and counting an owner's whole partition to learn it is non-zero reintroduces the cost being avoided. It mirrors the search's own scoping exactly, including the invalidated_at filter -- a probe judging the owner empty on stricter terms than the search would skip a rescue that would have worked, and a silent recall loss is not worth one avoided query. Applied to all four repositories that share the ladder -- fact, entity, preference and reasoning-trace -- so there is no setting some of them respect. Off by default, and the guard defaults to "escalate" whenever the owner is unknown. Results are identical either way, and that is asserted against a live database rather than argued: the empty owner finds nothing with or without the skip, and the starved owner's recovered rows are unchanged. 4,370 unit and 360 integration (+3) green. Release 0 warnings. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PgDgctPpbTziBNE2RT8VdE
There was a problem hiding this comment.
Pull request overview
Adds an opt-in optimization to the Neo4j “escalation ladder” recall path: before issuing the widened global-index probe + owner scan, cheaply detect whether the owner has any rows of the searched label and skip the ladder only for provably-empty owners. Includes an integration “large-owner arm” test suite to measure/validate the empty-vs-starved behavior described in PLAN 2.13.
Changes:
- Introduces
MemoryOptions.SkipEscalationWhenOwnerHasNoRows(default off) and applies it consistently across Fact/Entity/Preference/ReasoningTrace repositories. - Adds
OwnerRowExistence.Any(...)query builder used to probe per-owner existence (LIMIT 1, invalidated rows excluded). - Adds integration tests to measure ladder behavior under heavy neighbor crowding and assert equivalence of returned results when skipping is enabled.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/AgentMemory.Tests.Integration/Repositories/EscalationLadderArmIntegrationTests.cs | New integration test “arm” that seeds crowded neighbors + starved owner to measure ladder behavior and validate skip-option equivalence. |
| src/AgentMemory.Neo4j/Repositories/Neo4jReasoningTraceRepository.cs | Gates escalation with an owner-row existence probe when the new option is enabled. |
| src/AgentMemory.Neo4j/Repositories/Neo4jPreferenceRepository.cs | Same: skip futile escalation for provably-empty owners when enabled. |
| src/AgentMemory.Neo4j/Repositories/Neo4jFactRepository.cs | Same gating added; also introduces duplicated helper method implementation used across repos. |
| src/AgentMemory.Neo4j/Repositories/Neo4jEntityRepository.cs | Same gating added for entity recall. |
| src/AgentMemory.Neo4j/Queries/OwnerRowExistence.cs | New Cypher query builder for owner-scoped existence probing (LIMIT 1, shared-scope aware, excludes invalidated rows). |
| src/AgentMemory.Abstractions/Options/MemoryOptions.cs | Adds the new option and documents the measured rationale/tradeoff. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+39
to
+53
| public static string Any(string label, bool includeShared) | ||
| { | ||
| var owner = includeShared | ||
| ? "(n.owner_id = $ownerId OR n.owner_id IS NULL)" | ||
| : "n.owner_id = $ownerId"; | ||
|
|
||
| // invalidated_at mirrors the vector searches, which all exclude soft-invalidated rows. An | ||
| // owner holding nothing but tombstones has nothing the escalation could return. | ||
| return $""" | ||
| MATCH (n:{label}) | ||
| WHERE {owner} AND n.invalidated_at IS NULL | ||
| RETURN 1 AS present | ||
| LIMIT 1 | ||
| """; | ||
| } |
Comment on lines
+916
to
+922
| var present = await _tx.ReadAsync(async runner => | ||
| { | ||
| var cursor = await runner.RunAsync( | ||
| OwnerRowExistence.Any("Fact", includeShared), | ||
| new { ownerId = scope.OwnerId }).ConfigureAwait(false); | ||
| return (await cursor.ToListAsync().ConfigureAwait(false)).Count > 0; | ||
| }, cancellationToken).ConfigureAwait(false); |
Comment on lines
+646
to
+652
| var present = await _tx.ReadAsync(async runner => | ||
| { | ||
| var cursor = await runner.RunAsync( | ||
| OwnerRowExistence.Any("Preference", includeShared), | ||
| new { ownerId = scope.OwnerId }).ConfigureAwait(false); | ||
| return (await cursor.ToListAsync().ConfigureAwait(false)).Count > 0; | ||
| }, cancellationToken).ConfigureAwait(false); |
Comment on lines
+1032
to
+1038
| var present = await _tx.ReadAsync(async runner => | ||
| { | ||
| var cursor = await runner.RunAsync( | ||
| OwnerRowExistence.Any("Entity", includeShared), | ||
| new { ownerId = scope.OwnerId }).ConfigureAwait(false); | ||
| return (await cursor.ToListAsync().ConfigureAwait(false)).Count > 0; | ||
| }, cancellationToken).ConfigureAwait(false); |
Comment on lines
+568
to
+574
| var present = await _tx.ReadAsync(async runner => | ||
| { | ||
| var cursor = await runner.RunAsync( | ||
| OwnerRowExistence.Any("ReasoningTrace", includeShared), | ||
| new { ownerId = scope.OwnerId }).ConfigureAwait(false); | ||
| return (await cursor.ToListAsync().ConfigureAwait(false)).Count > 0; | ||
| }, cancellationToken).ConfigureAwait(false); |
Comment on lines
+902
to
+914
| /// <summary> | ||
| /// Whether the escalation ladder can possibly help this owner (2.13). | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// Returns <see langword="true"/> unless the option is on AND the owner provably holds no rows of | ||
| /// this label. Defaulting to "escalate" is the safe direction: a probe that wrongly reported empty | ||
| /// would skip a rescue that would have worked, and a silent recall loss costs far more than one | ||
| /// avoided query. | ||
| /// </remarks> | ||
| private async Task<bool> ShouldClimbLadderAsync( | ||
| MemoryScope? scope, bool includeShared, CancellationToken cancellationToken) | ||
| { | ||
| if (!_skipEscalationWhenOwnerHasNoRows || scope?.OwnerId is null) return true; |
This was referenced Aug 27, 2026
Open
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.
The plan said "do not touch this without a large-owner arm" — so the arm was the missing prerequisite and got built first rather than treated as a blocker.
Measured (
EscalationLadderArmIntegrationTests: 400 crowding neighbours, a 20-row starved owner, an empty owner):So the rung stays. Removing it unconditionally costs the starved owner its rescue — exactly the trade the note warned about, and exactly what a plausible-sounding argument would have got wrong.
What the measurement does license is a different optimisation: the useful distinction is not "did the search return nothing" but "does this owner have anything to find" — and only the second is cheap, being bounded by one owner's data rather than the corpus. For an empty owner the ladder currently asks the global index for up to 2,000 candidates to find rows that do not exist, then scans.
OwnerRowExistence.Anyis that probe —LIMIT 1, not a count, because counting an owner's whole partition to learn it is non-zero reintroduces the cost being avoided. It mirrors the search's own scoping exactly,invalidated_atincluded: a probe judging the owner empty on stricter terms would skip a rescue that would have worked, and a silent recall loss is not worth one avoided query.MemoryOptions.SkipEscalationWhenOwnerHasNoRows, default off, applied to all four repos sharing the ladder (fact/entity/preference/trace) — no partially-respected setting. The guard defaults to escalate whenever the owner is unknown.Equivalence asserted against a live database rather than argued: empty owner finds nothing either way, starved owner's recovered rows unchanged.
4,370 unit and 360 integration (+3) green. Release 0 warnings.
🤖 Generated with Claude Code
https://claude.ai/code/session_01PgDgctPpbTziBNE2RT8VdE