fix(context): respect isolation scopes during compaction - #830
Conversation
|
Hi @kalenkevich, I have checked the issue and validated the proposed changes. I was able to reproduce the issue. Could you please review this PR? |
AmaadMartin
left a comment
There was a problem hiding this comment.
The scope filter is correct where it runs, but it runs on one of the six paths that read or rewrite history. I checked the five compactors and the content request processor at the head SHA: three of them still merge or drop events across scopes, and the summary this compactor pushes is itself untagged, so every peer scope can read it. One note outside the diff: run_llm_agent_as_node.ts:94 guards with if (ctx.isolationScope), so a node declared with isolationScope: '' never tags its events. I did not run the test suite.
| const visibleEvents = | ||
| currentIsolationScope !== undefined | ||
| ? events.filter( | ||
| (event) => | ||
| event.isolationScope === undefined || | ||
| event.isolationScope === currentIsolationScope, | ||
| ) | ||
| : events; |
There was a problem hiding this comment.
Not a nit. An unscoped caller filters nothing, so scoped events still reach the summarizer.
currentIsolationScope !== undefined
? events.filter(...)
: events;The root agent runs with isolationScope === undefined. Its compactor then summarizes every isolated node's events into shared history. isOutsideIsolationScope (core/src/agents/processors/content_processor_utils.ts:239) withholds a tagged event from an unscoped reader; this copy keeps it. Always filter:
const visibleEvents = events.filter(
(event) =>
event.isolationScope === undefined ||
event.isolationScope === currentIsolationScope,
);Better: export isOutsideIsolationScope and call it, so the two rules cannot drift.
There was a problem hiding this comment.
Resolved. isEventVisibleInIsolationScope now returns false for a scoped event when the caller is unscoped (currentIsolationScope undefined), so an unscoped summarizer no longer ingests peer-scoped events. The comment documenting this semantic is a nice touch.
| export function getActiveEvents( | ||
| events: Event[], | ||
| currentIsolationScope?: string, | ||
| ): Event[] { |
There was a problem hiding this comment.
Not a nit. The parameter is optional, so the other history paths keep the old behaviour.
Callers that still pass no scope:
core/src/context/agent_controlled_context_compactor.ts:32core/src/agents/processors/content_request_processor.ts:45core/src/context/anchored_context_compactor.ts:70and:97, through a private copy of this helper at:44
AnchoredContextCompactor.compact is the worst case. It rewrites session.events in place at :148-149, so a peer scope's events merge into one shared scratchpad and the originals disappear.
Two more compactors index session.events directly and never see a scope: truncating_context_compactor.ts:49 splices peer events out, trajectory_thought_pruning_compactor.ts:61 rewrites them.
There was a problem hiding this comment.
Resolved. Every history path now threads the scope through: content_request_processor, the token/anchored/agent_controlled compactors, and the truncating/trajectory compactors use the visibility helper directly. The parameter stays optional but no production caller relies on the default anymore.
| const activeEvents = getActiveEvents( | ||
| events, | ||
| invocationContext.isolationScope, | ||
| ); |
There was a problem hiding this comment.
Not a nit. The summary that compact pushes carries no scope, so content leaks the other way.
Line 128 is invocationContext.session.events.push(compactedEvent). The summarizer output has no isolationScope. An untagged event is shared history, and every peer scope reads it (content_processor_utils.ts:243-246). Scope A's turns still reach scope B, now as the summary.
compactedEvent.isolationScope ??= invocationContext.isolationScope;
invocationContext.session.events.push(compactedEvent);There was a problem hiding this comment.
Resolved. compactedEvent.isolationScope ??= invocationContext.isolationScope now tags the pushed summary (same in agent_controlled), so a scoped compaction's summary is only visible within its scope and no longer leaks to peers or unscoped readers.
| expect(summarized).toHaveLength(1); | ||
| expect(summarized[0]).toEqual([current]); |
There was a problem hiding this comment.
Nit. The test fails without the fix, but not for the reason its name gives.
peer is the last event, so retention keeps it out of the summarizer either way. Without the fix rawEventsToCompact is [current, current2], and this assertion fails on current2, not on peer. Move peer into the middle:
const context = createMockInvocationContext([current, peer, current2]);The unfixed code then summarizes [current, peer], which is the real leak. I traced this by hand and did not run the suite.
There was a problem hiding this comment.
Thanks — the new does not summarize events from another isolation scope test targets the behavior precisely, asserting the peer event never reaches the summarizer and the resulting summary is scope-tagged.
Closes #824
Problem
TokenBasedContextCompactorselected events from the entire session before applying workflow isolation. This allowed a peer node's event to reach the summarizer and then the current node's model after compaction.Changes
isolationScopebefore token counting and compaction.Tests
npx vitest run --project unit:core core/test/context/token_based_context_compactor_test.ts(8 passed)npm run ts:check(passed)npx eslint core/src/context/compaction_utils.ts core/src/context/token_based_context_compactor.ts core/test/context/token_based_context_compactor_test.ts(passed)npx prettier core/src/context/compaction_utils.ts core/src/context/token_based_context_compactor.ts core/test/context/token_based_context_compactor_test.ts --check(passed)git diff --check(passed)Compatibility / Known limitations
Unscoped compaction remains unchanged. This PR does not add a live-model or end-to-end test; the regression is covered with deterministic unit-test fixtures and requires no credentials or external services.