fix(state): resolve memory scopes most-specific-first when a recall widens - #438
Merged
Conversation
…idens
state.Scope documents the instance/project/workspace axis as one on which memory
is "resolved most-specific-first", but no store did that. Both MemoryStore
implementations compared the whole struct for equality - state/memory.go on
`it.Scope != q.Scope`, storage/sqlite on a three-column predicate - so a fact
written at {Project: X} was invisible to a reader at {Project: X, Workspace: W}.
An agent running workspace-under-project therefore recalled only what that exact
workspace had written, and the general facts worth keeping were unreachable.
The widening is opt-in via RecallQuery.IncludeAncestors rather than the new
default. Exact matching is a partitioning guarantee existing readers rely on
(memorytest already pinned it: a scoped recall must not see the enclosing
scope's items), so silently widening every read would trade one wrong behaviour
for another. A caller states at the call site that it wants to inherit.
The resolution rule lives in state and every backend resolves through it:
- Scope.Ancestors walks the enclosing chain, most-specific first, skipping a
level whose field is empty rather than inventing one, so {Project: "x"}
resolves straight to the global scope with no phantom instance in between.
- Scope.Depth ranks specificity by the innermost set field. Counting set fields
would tie two levels of a chain whose outer field is empty; reading the
innermost one keeps Depth a total order over any chain, which is what lets
SQLite express the same ranking as an argument-free ORDER BY CASE.
- RecallQuery.ScopeChain and SortRecall give the three implementations one
filter and one sort instead of three that drift apart.
Ordering by scope applies only when the read actually spans levels, so an
unfiltered or single-scope recall keeps the plain most-recent-first order it has
always returned. SQLite orders in SQL rather than after collection, because
LIMIT would otherwise truncate the wrong rows; it also falls off the prepared
statement for a widened read, since that statement binds exactly one scope
triple.
memorytest gains a ScopeResolution case covering the walk, the sibling scope it
must never reach, the empty-level skip, limit-after-resolution, and the zero
scope still meaning "unfiltered". It fails against exact matching in both
backends. state's own MemoryStore now runs that suite too: it was the one
implementation of the interface the conformance suite never covered, which is
how it could resolve scopes differently from the others unnoticed.
SkillStore.List has the same exact-match behaviour and is left alone, documented
on Scope rather than changed, since memory is the read that runs every turn.
Signed-off-by: Ion Alpha <contact@ionalpha.io>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Patch coverage on the scope-resolution change came in under the gate, and the uncovered lines were all in one shape: an assert-and-fail block repeated at every assertion, whose t.Fatalf branch never runs on a green build. Collapse them into four helpers (write, recall, wantOrder, wantCount) so each assertion in the suite is a single call and the failure branches live in one place. That is better to read as well as to measure - the suite now states what it expects rather than how it checks. Two real coverage gaps came out of it, both the same kind of gap the scope bug itself came from - an implementation nothing held to the shared contract: - The SQLite provider's own memory store (the FTS-backed one behind state.Provider, distinct from the resource-backed facade) never ran the MemoryStore suite. statetest exercises its CRUD but never a recall that resolves across scope levels, so the SQL that widens one had nothing checking it. It now runs memorytest.RunSuite like the other implementations. - The facade's widened read walks one List per ancestor, and a backend outage inside that walk was untested. TestRecallBackendErrorsPropagate now covers the widened path alongside the scoped and scope-spanning ones. Patch coverage 96.4%; what remains is the four helpers' own failure branches. Signed-off-by: Ion Alpha <contact@ionalpha.io>
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
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.
What
Memory recall can now resolve up the instance/project/workspace hierarchy instead of matching one exact scope.
RecallQuery.IncludeAncestorswidens a scoped recall to that scope's enclosing scopes, most-specific first.Scope.Ancestorsis the enclosing chain;Scope.Depthranks specificity.RecallQuery.ScopeChainandstate.SortRecallare the one filter and one sort all threeMemoryStoreimplementations now resolve through.state's own memory store joins thememorytestconformance suite.Why
Scope's doc comment says memory is "resolved most-specific-first". Nothing implemented that. Both stores compared the whole struct for equality (state/memory.goonit.Scope != q.Scope,storage/sqliteon a three-column predicate), so a fact written at{Project: X}was invisible to a reader at{Project: X, Workspace: W}. An agent running workspace-under-project recalled only what that exact workspace had written, which is to say nothing worth having: the durable general facts are exactly the ones written at the outer scope.Widening is opt-in rather than the new default. Exact matching is a partitioning guarantee readers rely on, and
memorytestalready pinned it: a scoped recall must not see the enclosing scope's items. Making every read hierarchical would trade one wrong behaviour for another, so a caller says at the call site that it wants to inherit.SkillStore.Listmatches scopes exactly for the same reason and is deliberately left alone; the divergence is now documented onScopeinstead of being an accident. Memory went first because it is the read that runs on every turn.How to verify
The new
memorytestScopeResolutioncase is the contract: it covers the ancestor walk, the sibling scope a widened read must never reach, the skipped empty level, limit applied after resolution, and the zero scope still meaning "unfiltered". It fails against exact matching in both backends, confirmed by forcing the chain back to a single scope, which redsTestConformance/ScopeResolutionandTestMemoryFacadeConformance/ScopeResolution.Three new property tests cover the chain invariants: starts at the scope, ends global, no repeats, strictly decreasing
Depth; ancestors only ever widen; a widened recall extends the narrow one without reordering it.Notes for reviewers
RanksByScopeis false for both, andSortRecallfalls straight through to the recency comparison.Depthreads the innermost set field rather than counting set fields. Counting ties two levels of a chain whose outer field is empty ({Project: "x"}and its global parent). Reading the innermost keepsDeptha total order over any chain, which is what lets SQLite express the identical ranking as an argument-freeORDER BY CASE.LIMITtruncates the wrong rows. A widened read also falls off the preparedmemoryRecallstatement, since that binds exactly one scope triple; the built query is the only shape that can express a variable-length chain.RecallQuery's zero value behaves exactly as before, so no existing caller changes.state'sMemoryStoreand the SQLite provider's own memory store were the two implementations of the interface the conformance suite never ran. That gap is how the same bug could sit in three places without a test noticing; both now run the same suite.