feat: add hierarchical context manager [Story 447.1] - #706
Merged
Conversation
rafaelscosta
requested review from
Pedrovaleriolopez and
oalanicolas
as code owners
May 8, 2026 18:40
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThis PR implements HierarchicalContextManager: an event-emitting class that maintains bounded LLM-ready context by compacting older short-term messages into long-term system summaries within configurable token budgets. It supports injectable tokenizers and summarizers with a deterministic fallback, emits swap events, and includes docs, registry/manifest updates, and Jest tests. ChangesHierarchical Context Management Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Suggested labelsarea: core, area: synapse, type: test, area: docs Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
area: agents
Contributor
📊 Coverage ReportCoverage report not available
Generated by PR Automation (Story 6.1) |
Collaborator
Author
|
@Pedrovaleriolopez @oalanicolas PR pronto para code owner review.\n\nResumo de validação:\n- CI GitHub verde, incluindo ESLint, TypeScript, manifest, Story Checkbox, Dependency Validation, Brownfield Install, Installer Smoke, Semantic Lint e Jest Node 18/20/22/24/25.\n- CodeQL verde.\n- Vercel preview verde.\n- Validação local pré-PR também concluída: focused hierarchical tests, regressão adjacente SYNAPSE/context/memory/orchestration, lint, typecheck, validate:manifest e full Jest suite.\n\nÚnico bloqueio de merge restante: ruleset/CODEOWNERS exige aprovação de maintainer; tentativa de merge administrativo com --admin não teve bypass efetivo para a credencial atual. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
tests/synapse/hierarchical-context-manager.test.js (5)
76-85: 💤 Low valueConsider asserting on the
swap:completeevent payload, not just its presence.Line 76 only asserts
completeEvents.length > 0. The test already validates the storedcontextshape in detail (Lines 79–90), so asserting the event's payload shape (e.g.sourceMessageCount,summaryTokens, or similar contract fields) would independently catch regressions in the event emission contract without relying solely on context inspection.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/synapse/hierarchical-context-manager.test.js` around lines 76 - 85, The test currently only checks that completeEvents.length > 0 for the "swap:complete" event; update the assertion to verify the event payload shape and key contract fields (e.g., inspect completeEvents[0].payload and assert presence and types/values for fields like sourceMessageCount, summaryTokens, and any other contract fields you expect from the swap:complete emission), and ensure those values are consistent with stats and context (for example sourceMessageCount === context[0].metadata.aiox.sourceMessages.length and summaryTokens matches stats.longTermSummaries or stats.totalTokens as appropriate); reference completeEvents, swap:complete, context and stats to locate and validate the payload in the test.
76-85: 💤 Low valueAssert the
swap:completeevent payload structure, not just its presence.Line 76 only checks
completeEvents.length > 0. The test already verifies the resultingcontextshape in detail (Lines 79–90), so it would be consistent and more useful to also assert the event payload (e.g.,sourceMessageCount,summaryTokens, or whatever the contract specifies), catching regressions in the event emission contract independently of the stored context.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/synapse/hierarchical-context-manager.test.js` around lines 76 - 85, The test currently only checks that completeEvents.length > 0; update the assertions to validate the payload of the emitted 'swap:complete' event(s) (using completeEvents[0] or iterate completeEvents) against expected contract fields (e.g., sourceMessageCount, summaryTokens, summaryId or similar), and cross-check those fields with the already-verified values in stats/context (for example ensure payload.sourceMessageCount equals context[0].metadata.aiox.sourceMessages.length and payload.summaryTokens matches stats.longTermSummaries or stats.totalTokens as appropriate); locate the emission assertions around completeEvents and add explicit shape/value assertions rather than only checking length.
116-141: ⚡ Quick winSeparate the two error-notification listeners to avoid ambiguous assertions.
Both
onSwapError(Line 125) andmanager.on('swap:error', ...)(Line 128) push into the sameerrorEventsarray. If the implementation fires both paths (which is the expected contract per the AI summary), each compaction failure pushes two entries, makingerrorEvents.lengthan unreliable signal. It also makes it impossible to tell from a failing test which notification path broke.♻️ Proposed fix
- const errorEvents = []; + const callbackEvents = []; + const emitterEvents = []; const manager = new HierarchicalContextManager({ maxTokens: 30, summarizationThreshold: 0.5, tokenizer: wordTokenizer, summarizer: async () => { throw new Error('summarizer unavailable'); }, - onSwapError: event => errorEvents.push(event), + onSwapError: event => callbackEvents.push(event), }); - manager.on('swap:error', event => errorEvents.push(event)); + manager.on('swap:error', event => emitterEvents.push(event)); // ... - expect(errorEvents.length).toBeGreaterThan(0); + expect(callbackEvents.length).toBeGreaterThan(0); + expect(emitterEvents.length).toBeGreaterThan(0);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/synapse/hierarchical-context-manager.test.js` around lines 116 - 141, The test currently uses a single errorEvents array for both the onSwapError option and the manager.on('swap:error', ...) listener, making it ambiguous which notification path fired; split them into two distinct collectors (e.g., onSwapErrorEvents and emittedSwapErrorEvents) and attach the onSwapError option to push into onSwapErrorEvents and manager.on('swap:error', ...) to push into emittedSwapErrorEvents (referencing the HierarchicalContextManager constructor option onSwapError and the EventEmitter listener manager.on('swap:error')). Then update the assertions to verify each collector received the expected number of entries (or specific payloads) separately instead of asserting on a single combined errorEvents.length and leave the other expectations (getStats().lastError and fallbackUsed) unchanged.
116-141: ⚡ Quick winSeparate the two error-notification listeners to avoid ambiguous assertions.
Both
onSwapError(Line 125) andmanager.on('swap:error', ...)(Line 128) push into the sameerrorEventsarray. If the implementation fires both paths — which is the stated contract — each failure pushes two entries, makingerrorEvents.lengthan unreliable signal. It also makes a failing test undiagnosable: you can't tell from the count which notification path broke.♻️ Proposed fix
- const errorEvents = []; + const callbackEvents = []; + const emitterEvents = []; const manager = new HierarchicalContextManager({ maxTokens: 30, summarizationThreshold: 0.5, tokenizer: wordTokenizer, summarizer: async () => { throw new Error('summarizer unavailable'); }, - onSwapError: event => errorEvents.push(event), + onSwapError: event => callbackEvents.push(event), }); - manager.on('swap:error', event => errorEvents.push(event)); + manager.on('swap:error', event => emitterEvents.push(event)); // ... - expect(errorEvents.length).toBeGreaterThan(0); + expect(callbackEvents.length).toBeGreaterThan(0); + expect(emitterEvents.length).toBeGreaterThan(0);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/synapse/hierarchical-context-manager.test.js` around lines 116 - 141, The test registers two different notification paths (the onSwapError option and the manager.on('swap:error'...) listener) but pushes into the same errorEvents array, making the assertion ambiguous; update the test to use distinct collectors (e.g., errorEventsOption and errorEventsEmitter) or Jest spies (jest.fn()) for the two handlers, assert each one was invoked (and/or inspect their event payloads) separately, and then keep the existing assertions for getStats().lastError and fallback metadata to verify behavior; reference the onSwapError option and manager.on('swap:error', ...) listener in your changes to locate where to split the collectors.
1-4: 💤 Low valueConsider pinning to the latest Jest 30 patch (
30.3.0).The latest stable Jest version on npm is
30.3.0. The test suite references30.2.0. Jest30.2.0is a real, released version, so this is not a blocker, but staying on the latest patch picks up any interim bug fixes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/synapse/hierarchical-context-manager.test.js` around lines 1 - 4, The test suite references Jest 30.2.0; update the project's devDependency for jest to the latest patch 30.3.0 in package.json (where Jest is declared), reinstall dependencies (npm install or yarn install) to update the lockfile (package-lock.json or yarn.lock), and re-run the tests (including the test that imports HierarchicalContextManager and buildDefaultSummary) to ensure nothing broke.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.aiox-core/core/synapse/context/hierarchical-context-manager.js:
- Around line 134-138: The addMessage/_compactIfNeeded flow is racy: concurrent
addMessage calls can interleave in _swapShortTermMessages (which awaits
summarization then unconditionally splices), corrupting _shortTermMessages and
making _lastError/_lastSwap non-atomic. Fix by serializing mutations: introduce
a per-instance async mutex or promise chain and acquire it around
_compactIfNeeded/_swapShortTermMessages and the push into _shortTermMessages so
the sequence push → _compactIfNeeded → splice/update _lastSwap/_lastError is
atomic; ensure errors are caught and _lastError/_lastSwap are updated while
holding the lock so state remains consistent. Reference functions: addMessage,
_compactIfNeeded, _swapShortTermMessages, and fields _shortTermMessages,
_lastError, _lastSwap.
- Around line 240-244: The current fallback that replaces
this._longTermSummaries with only the truncated last summary can silently drop
earlier summaries; instead ensure we collapse/compact all existing long-term
summaries before truncation so nothing is discarded without being summarized.
Modify the branch that runs when this._getTotalTokens() > this.maxTokens and
this._longTermSummaries.length > 0 to first call the existing compaction routine
(e.g., this._compactLongTermSummaries or a new helper that merges all entries)
to collapse the array into a single combined summary, then apply
this._truncateSummaryMessage to that combined summary and set
this._longTermSummaries to the truncated result; reference methods/fields
_getTotalTokens, maxTokens, _longTermSummaries, _truncateSummaryMessage, and
_compactLongTermSummaries (or add a collapse helper) when making the change.
In `@tests/synapse/hierarchical-context-manager.test.js`:
- Around line 1-4: The test imports HierarchicalContextManager and
buildDefaultSummary using a relative path; update the require call so it uses
the project's configured absolute import alias/root (the same alias used by
Jest's moduleNameMapper or moduleDirectories) to import the module that exports
HierarchicalContextManager and buildDefaultSummary; locate the
require('../../.aiox-core/core/synapse/context') in
tests/synapse/hierarchical-context-manager.test.js and replace it with the
equivalent absolute path import that resolves in the test runner.
- Around line 1-4: Replace the relative
require('../../.aiox-core/core/synapse/context') in the test with the project's
configured absolute import for that module; update the import that provides
HierarchicalContextManager and buildDefaultSummary to use the project alias/root
used by Jest (moduleNameMapper/moduleDirectories) so the test resolves the same
module via absolute path.
---
Nitpick comments:
In `@tests/synapse/hierarchical-context-manager.test.js`:
- Around line 76-85: The test currently only checks that completeEvents.length >
0 for the "swap:complete" event; update the assertion to verify the event
payload shape and key contract fields (e.g., inspect completeEvents[0].payload
and assert presence and types/values for fields like sourceMessageCount,
summaryTokens, and any other contract fields you expect from the swap:complete
emission), and ensure those values are consistent with stats and context (for
example sourceMessageCount === context[0].metadata.aiox.sourceMessages.length
and summaryTokens matches stats.longTermSummaries or stats.totalTokens as
appropriate); reference completeEvents, swap:complete, context and stats to
locate and validate the payload in the test.
- Around line 76-85: The test currently only checks that completeEvents.length >
0; update the assertions to validate the payload of the emitted 'swap:complete'
event(s) (using completeEvents[0] or iterate completeEvents) against expected
contract fields (e.g., sourceMessageCount, summaryTokens, summaryId or similar),
and cross-check those fields with the already-verified values in stats/context
(for example ensure payload.sourceMessageCount equals
context[0].metadata.aiox.sourceMessages.length and payload.summaryTokens matches
stats.longTermSummaries or stats.totalTokens as appropriate); locate the
emission assertions around completeEvents and add explicit shape/value
assertions rather than only checking length.
- Around line 116-141: The test currently uses a single errorEvents array for
both the onSwapError option and the manager.on('swap:error', ...) listener,
making it ambiguous which notification path fired; split them into two distinct
collectors (e.g., onSwapErrorEvents and emittedSwapErrorEvents) and attach the
onSwapError option to push into onSwapErrorEvents and manager.on('swap:error',
...) to push into emittedSwapErrorEvents (referencing the
HierarchicalContextManager constructor option onSwapError and the EventEmitter
listener manager.on('swap:error')). Then update the assertions to verify each
collector received the expected number of entries (or specific payloads)
separately instead of asserting on a single combined errorEvents.length and
leave the other expectations (getStats().lastError and fallbackUsed) unchanged.
- Around line 116-141: The test registers two different notification paths (the
onSwapError option and the manager.on('swap:error'...) listener) but pushes into
the same errorEvents array, making the assertion ambiguous; update the test to
use distinct collectors (e.g., errorEventsOption and errorEventsEmitter) or Jest
spies (jest.fn()) for the two handlers, assert each one was invoked (and/or
inspect their event payloads) separately, and then keep the existing assertions
for getStats().lastError and fallback metadata to verify behavior; reference the
onSwapError option and manager.on('swap:error', ...) listener in your changes to
locate where to split the collectors.
- Around line 1-4: The test suite references Jest 30.2.0; update the project's
devDependency for jest to the latest patch 30.3.0 in package.json (where Jest is
declared), reinstall dependencies (npm install or yarn install) to update the
lockfile (package-lock.json or yarn.lock), and re-run the tests (including the
test that imports HierarchicalContextManager and buildDefaultSummary) to ensure
nothing broke.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e880c597-9013-40d8-b784-fc5b483c26b7
📒 Files selected for processing (8)
.aiox-core/core/synapse/context/README.md.aiox-core/core/synapse/context/hierarchical-context-manager.js.aiox-core/core/synapse/context/index.js.aiox-core/data/entity-registry.yaml.aiox-core/install-manifest.yamldocs/stories/epic-447-hierarchical-context-management/EPIC-447-HIERARCHICAL-CONTEXT-MANAGEMENT.mddocs/stories/epic-447-hierarchical-context-management/STORY-447.1-HIERARCHICAL-CONTEXT-MANAGER-CONTRACT.mdtests/synapse/hierarchical-context-manager.test.js
rafaelscosta
dismissed
coderabbitai[bot]’s stale review
May 8, 2026 19:20
Addressed in b45895a: serialized concurrent addMessage mutations, preserved long-term summary lineage before hard-limit truncation, updated absolute import and event assertions, and revalidated CI gates.
tuanmedeiros
pushed a commit
to tuanmedeiros/aios-core-synkraay
that referenced
this pull request
Jun 2, 2026
Marks Story 447.1 and Epic 447 as Done after PR SynkraAI#706 merged the hierarchical context manager. Records quality gate evidence and closes SynkraAI#447.
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
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.
Summary
.aiox-core/core/synapse/context/Validation
Summary by CodeRabbit
New Features
Documentation
Tests
Chores