feat(memory): implement dual-mode archival in memory consolidation#524
feat(memory): implement dual-mode archival in memory consolidation#524
Conversation
Add content-density-aware archival to the consolidation system: - Heuristic-based DensityClassifier (5 weighted signals: code patterns, structured data, identifiers, numeric density, line structure) - ExtractivePreserver for dense content (verbatim key facts + start/mid/end anchors instead of lossy summarization) - AbstractiveSummarizer for sparse content (LLM-based with truncation fallback) - DualModeConsolidationStrategy implementing ConsolidationStrategy protocol (per-entry classification, group-level majority vote, mode-aware processing) - Enriched ConsolidationResult with mode_assignments and archival_index for deterministic index-based restore of self-archived content - ArchivalEntry gains archival_mode field (None for legacy entries) - DualModeConfig nested under ArchivalConfig for density threshold, model, anchor length, and fact extraction limits Research basis: Memex (arXiv:2603.04257) dual-mode archival, KV Cache Attention Matching (arXiv:2602.16284) proving summarization is catastrophically lossy on dense content. Closes #418
- Use .value for StrEnum literal comparisons in tests (mypy comparison-overlap) - Add ArchivalMode, ContentDensity, DualModeConsolidationStrategy to expected exports in test_init.py
- Extract _archive_single_entry from _archive_entries (50-line limit) - Extract _process_group from consolidate (50-line limit) - Add conditional validator: DualModeConfig requires summarization_model when enabled (default changed to enabled=False for safe opt-in) - Add model param validation in AbstractiveSummarizer.__init__ - Add cross-field validator: ConsolidationResult archival_index <= count - Normalize _select_entries to use tuple params (consistency) - Add density weight sum-to-1.0 assertion guard - Fix _build_anchors: short text uses single anchor, not triple copy - Log empty-entries early return in dual-mode strategy - Document tie-breaking: 50/50 splits default to ABSTRACTIVE - Update docs/design/memory.md with dual-mode archival section - Update CLAUDE.md Package Structure consolidation description - Add API ref directives for 4 new modules in docs/api/memory.md - Update docs/architecture/tech-stack.md consolidation description Pre-reviewed by 6 agents, 13 findings addressed
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds density-aware dual-mode archival to memory consolidation: content is classified as SPARSE or DENSE, then archived via abstractive LLM summaries or extractive verbatim preservation. Introduces DensityClassifier, AbstractiveSummarizer, ExtractivePreserver, DualModeConsolidationStrategy, archival models/indexing, service indexing plumbing, observability events, docs, and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Service as MemoryConsolidationService
participant Strategy as DualModeConsolidationStrategy
participant Classifier as DensityClassifier
participant Summarizer as AbstractiveSummarizer
participant Extractor as ExtractivePreserver
participant Backend as MemoryBackend
participant ArchStore as ArchivalStore
Service->>Strategy: consolidate(entries, agent_id)
Strategy->>Classifier: classify_batch(group)
Classifier-->>Strategy: (entry, density) tuples
Strategy->>Strategy: determine group mode (majority vote)
Strategy->>Strategy: select keep_entry, removed_entries
alt mode == ABSTRACTIVE
Strategy->>Summarizer: summarize_batch(removed_entries)
Summarizer-->>Strategy: (entry_id, summary) pairs
Strategy->>Backend: store(abstractive_summary, agent_id)
else mode == EXTRACTIVE
Strategy->>Extractor: extract(concatenated_content)
Extractor-->>Strategy: facts + anchors
Strategy->>Backend: store(extractive_content, agent_id)
end
Backend-->>Strategy: archival_id
Strategy->>Backend: delete(removed_entry_ids, agent_id)
Backend-->>Strategy: deletion results
Strategy->>ArchStore: record archival_index(original_id, archival_id, mode)
Strategy-->>Service: ConsolidationResult(removed_ids, archived_count, mode_assignments, archival_index)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 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 unit tests (beta)
✨ Simplify code
📝 Coding Plan
Comment |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly upgrades the memory consolidation system by introducing an intelligent, dual-mode archival mechanism. Instead of a one-size-fits-all approach, the system now dynamically adapts its summarization strategy based on the density of the content, preserving critical information more effectively. This enhancement not only improves the accuracy and utility of archived memories but also provides a robust, index-based method for retrieving specific archived entries, streamlining future access and reducing reliance on potentially lossy semantic searches. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a significant new feature: dual-mode archival for memory consolidation. It adds a content density classifier to distinguish between sparse and dense content, applying abstractive summarization for the former and extractive preservation for the latter. This is a well-structured and comprehensive implementation, including new configuration options, data models, and a new consolidation strategy, all supported by thorough documentation and testing.
My review focuses on a critical syntax issue and a couple of key performance improvements in the asynchronous code. Specifically, I've pointed out an incorrect exception handling syntax and opportunities to parallelize network-bound async operations using asyncio.gather to improve efficiency when processing batches.
Overall, this is an excellent contribution that adds a sophisticated and valuable capability to the memory system.
| model=self._model, | ||
| ) | ||
| return response.content.strip() | ||
| except builtins.MemoryError, RecursionError: |
There was a problem hiding this comment.
This except block uses Python 2 syntax for catching multiple exceptions. In Python 3, this will raise a SyntaxError. To fix this, you should use a tuple of exception types.
| except builtins.MemoryError, RecursionError: | |
| except (builtins.MemoryError, RecursionError): |
| results: list[tuple[NotBlankStr, str]] = [] | ||
| for entry in entries: | ||
| summary = await self.summarize(entry.content) | ||
| results.append((entry.id, summary)) | ||
| return tuple(results) |
There was a problem hiding this comment.
The summarize_batch method currently awaits each summarize call sequentially within a loop. Since summarize is an async operation involving a network call, performance can be significantly improved by executing these calls concurrently. You can use asyncio.gather for this.
Note: You'll need to import asyncio at the top of the file.
| results: list[tuple[NotBlankStr, str]] = [] | |
| for entry in entries: | |
| summary = await self.summarize(entry.content) | |
| results.append((entry.id, summary)) | |
| return tuple(results) | |
| import asyncio | |
| tasks = [self.summarize(entry.content) for entry in entries] | |
| summaries = await asyncio.gather(*tasks) | |
| results = [(entry.id, summary) for entry, summary in zip(entries, summaries, strict=True)] | |
| return tuple(results) |
| parts: list[str] = [] | ||
| for entry in entries: | ||
| if mode == ArchivalMode.EXTRACTIVE: | ||
| parts.append(self._extractor.extract(entry.content)) | ||
| else: | ||
| summary = await self._summarizer.summarize(entry.content) | ||
| parts.append(summary) | ||
| return "\n---\n".join(parts) |
There was a problem hiding this comment.
In _build_content, when the mode is ABSTRACTIVE, the summarize calls are awaited sequentially in a loop. This can be inefficient for multiple entries. To improve performance, you can run these async calls concurrently using asyncio.gather.
Note: You'll need to import asyncio at the top of the file.
| parts: list[str] = [] | |
| for entry in entries: | |
| if mode == ArchivalMode.EXTRACTIVE: | |
| parts.append(self._extractor.extract(entry.content)) | |
| else: | |
| summary = await self._summarizer.summarize(entry.content) | |
| parts.append(summary) | |
| return "\n---\n".join(parts) | |
| if mode == ArchivalMode.EXTRACTIVE: | |
| parts = [self._extractor.extract(entry.content) for entry in entries] | |
| else: | |
| import asyncio | |
| tasks = [self._summarizer.summarize(entry.content) for entry in entries] | |
| parts = await asyncio.gather(*tasks) | |
| return "\n---\n".join(parts) |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #524 +/- ##
==========================================
+ Coverage 93.12% 93.15% +0.03%
==========================================
Files 513 517 +4
Lines 24644 24936 +292
Branches 2344 2369 +25
==========================================
+ Hits 22950 23230 +280
- Misses 1342 1351 +9
- Partials 352 355 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/synthorg/memory/consolidation/abstractive.py`:
- Around line 119-139: summarize_batch currently awaits each call to
self.summarize sequentially, causing serialized provider calls; change it to
fan-out/fan-in using asyncio.TaskGroup so each entry's self.summarize runs
concurrently but the final returned tuple preserves input order. Inside
summarize_batch create tasks for each entry (e.g., by iterating entries and
scheduling self.summarize(entry.content) into a TaskGroup), store the task
objects alongside entry.id, await the TaskGroup to run them, then collect
results in the original entries order to build and return the tuple of
(entry.id, summary) pairs; keep the existing failure fallback behavior per-entry
(handle exceptions per task and fall back to truncation if needed).
In `@src/synthorg/memory/consolidation/config.py`:
- Around line 83-86: Replace the blank-string sentinel for the
summarization_model field with an explicit optional NotBlankStr: change the
Field type to NotBlankStr | None (import NotBlankStr from core.types), set
default=None, and remove any manual strip()/whitespace checks that treated "" as
disabled; apply the same pattern to the other identifier fields referenced in
the file (the fields around the 106-112 region). Ensure Pydantic validation
handles whitespace-only rejection via NotBlankStr and that None represents the
disabled case.
In `@src/synthorg/memory/consolidation/extractive.py`:
- Around line 31-33: The extractor is lossy: _KEY_VALUE_PATTERN currently forces
a non-whitespace start for the value and the code in _extract_key_values
rewrites every match to use "="; change the regex to capture the separator and
allow empty values (e.g. r"^\s*([\w.-]+)\s*([:=])\s*(.*)$" with re.MULTILINE)
and update _extract_key_values to re-emit the original separator group instead
of always "=", returning key + separator + value verbatim (preserving empty
values and original ':' vs '='). Also apply the same change to the similar logic
referenced around lines 54-57 so both extraction paths preserve exact original
assignment form.
- Around line 153-156: The preservation block currently appends all extracted
facts as a single comma-joined string (see the list building around the variable
lines and the facts handling in extractive.py), which loses boundaries; instead,
when facts is truthy iterate over facts and append each fact as its own entry in
lines (e.g., call lines.append for each fact) so each extracted fact appears on
its own line while leaving the surrounding "[Extractive preservation]" and
"[START] {start}" lines unchanged.
In `@src/synthorg/memory/consolidation/models.py`:
- Around line 109-127: Update the _validate_index_count validator to enforce
consistency with consolidated IDs: in addition to the existing length check on
archival_index vs archived_count, first assert archived_count <=
consolidated_count, then verify every ArchivalIndexEntry.original_id in
archival_index is present in removed_ids and that those original_ids are unique
(no duplicates across archival_index). Use the existing symbols
_validate_index_count, archival_index, archived_count, consolidated_count,
removed_ids and ArchivalIndexEntry.original_id to locate the code; raise
ValueError with clear messages when any condition fails and return self when all
checks pass.
In `@tests/unit/memory/consolidation/test_dual_mode_strategy.py`:
- Around line 141-214: Tests depend on the real DensityClassifier; stub density
outputs so tests are deterministic by injecting fixed density values into the
entries or by passing a mocked classifier into _make_strategy used in
test_sparse_group_uses_abstractive_mode, test_sparse_calls_summarizer,
test_dense_group_uses_extractive_mode, and test_dense_calls_extractor: replace
reliance on DensityClassifier by setting a fixed .density (or similar attribute)
on entries returned by _make_sparse_entry/_make_dense_entry or by providing a
mock classifier via _make_strategy that returns predefined densities (e.g., low
values for sparse entries, high for dense), and add an explicit tie case (50/50
split) to assert that DualModeConsolidationStrategy chooses
ArchivalMode.ABSTRACTIVE in ties. Ensure references: _make_strategy,
_make_sparse_entry, _make_dense_entry, DualModeConsolidationStrategy, and the
test functions named above.
In `@tests/unit/memory/consolidation/test_extractive.py`:
- Around line 22-43: Update the two tests to assert the actual preserved facts,
not just the "Key facts:" header: in test_extracts_identifiers call
ExtractivePreserver().extract and assert that the identifier "abc-123-def" and
the full commit hash "a1b2c3d4e5f6a1b2c3d4e5f6a1b2c3d4e5f6" appear in the result
(in addition to the existing header checks), and in
test_extracts_key_value_pairs assert that the specific key-value strings "host:
192.168.1.100", "port: 5432" (and optionally "timeout: 30") are present in the
output from ExtractivePreserver.extract so the tests verify preservation of
actual values rather than only the section header.
In `@tests/unit/memory/consolidation/test_service.py`:
- Around line 339-383: Add a second variant to test_archival_builds_index that
verifies archival_index is keyed by ArchivalModeAssignment.original_id (not by
position): call MemoryConsolidationService.run_consolidation with a
ConsolidationResult whose mode_assignments are in a different order than
removed_ids (e.g., swap the two ArchivalModeAssignment entries) and make
archival.archive return IDs in an order that would fail a positional mapping
(also include one archival.archive side_effect that raises/returns a failure to
assert failed archival handling), then assert result.archival_index entries are
looked up by original_id (check archival_id and mode for each original_id) to
ensure the service maps archival IDs to original_id rather than by index.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e21f7b43-af53-4fb2-a437-5f2f402bda33
📒 Files selected for processing (22)
CLAUDE.mddocs/api/memory.mddocs/architecture/tech-stack.mddocs/design/memory.mdsrc/synthorg/memory/__init__.pysrc/synthorg/memory/consolidation/__init__.pysrc/synthorg/memory/consolidation/abstractive.pysrc/synthorg/memory/consolidation/config.pysrc/synthorg/memory/consolidation/density.pysrc/synthorg/memory/consolidation/dual_mode_strategy.pysrc/synthorg/memory/consolidation/extractive.pysrc/synthorg/memory/consolidation/models.pysrc/synthorg/memory/consolidation/service.pysrc/synthorg/observability/events/consolidation.pytests/unit/memory/consolidation/test_abstractive.pytests/unit/memory/consolidation/test_config.pytests/unit/memory/consolidation/test_density.pytests/unit/memory/consolidation/test_dual_mode_strategy.pytests/unit/memory/consolidation/test_extractive.pytests/unit/memory/consolidation/test_models.pytests/unit/memory/consolidation/test_service.pytests/unit/memory/test_init.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build Web
- GitHub Check: Build Backend
- GitHub Check: Build Sandbox
- GitHub Check: Test (Python 3.14)
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Nofrom __future__ import annotations— Python 3.14 has PEP 649 with native lazy annotations
Useexcept A, B:syntax (no parentheses) for exception handling in Python 3.14, as enforced by ruff PEP 758
All public functions and methods require type hints; mypy strict mode enforced
Docstrings: Google style, required on all public classes and functions, enforced by ruff D rules
Create new objects instead of mutating existing ones; never mutate. For non-Pydantic internal collections (registries,BaseTool), usecopy.deepcopy()at construction +MappingProxyTypewrapping for read-only enforcement.
Fordict/listfields in frozen Pydantic models, rely onfrozen=Truefor field reassignment prevention andcopy.deepcopy()at system boundaries (tool execution, LLM provider serialization, inter-agent delegation, serializing for persistence)
Use frozen Pydantic models for config/identity; use separate mutable-via-copy models (usingmodel_copy(update=...)) for runtime state that evolves. Never mix static config fields with mutable runtime fields in one model.
Use Pydantic v2 with adopted conventions:@computed_fieldfor derived values instead of storing + validating redundant fields; useNotBlankStrfromcore.typesfor all identifier/name fields (including optional and tuple variants) instead of manual whitespace validators
Preferasyncio.TaskGroupfor fan-out/fan-in parallel operations in new code (multiple tool invocations, parallel agent calls). Prefer structured concurrency over barecreate_task.
Line length: 88 characters (enforced by ruff)
Functions must be < 50 lines; files must be < 800 lines
Handle errors explicitly, never silently swallow them
Validate at system boundaries (user input, external APIs, config files)
Files:
src/synthorg/memory/consolidation/abstractive.pytests/unit/memory/consolidation/test_abstractive.pysrc/synthorg/observability/events/consolidation.pytests/unit/memory/consolidation/test_extractive.pysrc/synthorg/memory/consolidation/dual_mode_strategy.pysrc/synthorg/memory/consolidation/config.pysrc/synthorg/memory/consolidation/service.pysrc/synthorg/memory/consolidation/extractive.pytests/unit/memory/consolidation/test_density.pytests/unit/memory/consolidation/test_config.pytests/unit/memory/consolidation/test_dual_mode_strategy.pysrc/synthorg/memory/consolidation/density.pytests/unit/memory/test_init.pysrc/synthorg/memory/consolidation/models.pytests/unit/memory/consolidation/test_models.pytests/unit/memory/consolidation/test_service.pysrc/synthorg/memory/consolidation/__init__.pysrc/synthorg/memory/__init__.py
src/synthorg/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/synthorg/**/*.py: Every module with business logic MUST havefrom synthorg.observability import get_loggerthenlogger = get_logger(__name__); never useimport logging/logging.getLogger()/print()in application code
Logger variable name must always belogger(not_logger, notlog)
Event names must always use constants from domain-specific modules undersynthorg.observability.events(e.g.,API_REQUEST_STARTEDfromevents.api). Import directly:from synthorg.observability.events.<domain> import EVENT_CONSTANT
Always use structured logging:logger.info(EVENT, key=value)— neverlogger.info('msg %s', val)
All error paths must log at WARNING or ERROR with context before raising
All state transitions must log at INFO level
DEBUG logging for object creation, internal flow, and entry/exit of key functions
All provider calls go throughBaseCompletionProviderwhich applies retry + rate limiting automatically. Never implement retry logic in driver subclasses or calling code.
RetryConfig and RateLimiterConfig are set per-provider inProviderConfig; retryable errors (is_retryable=True) includeRateLimitError,ProviderTimeoutError,ProviderConnectionError,ProviderInternalError
Non-retryable errors raise immediately without retry;RetryExhaustedErrorsignals that all retries failed — engine layer catches this to trigger fallback chains
Rate limiter respectsRateLimitError.retry_afterfrom providers — automatically pauses future requests
Library reference: auto-generated from docstrings via mkdocstrings + Griffe (AST-based, no imports) indocs/api/
Files:
src/synthorg/memory/consolidation/abstractive.pysrc/synthorg/observability/events/consolidation.pysrc/synthorg/memory/consolidation/dual_mode_strategy.pysrc/synthorg/memory/consolidation/config.pysrc/synthorg/memory/consolidation/service.pysrc/synthorg/memory/consolidation/extractive.pysrc/synthorg/memory/consolidation/density.pysrc/synthorg/memory/consolidation/models.pysrc/synthorg/memory/consolidation/__init__.pysrc/synthorg/memory/__init__.py
docs/**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
Docs: Markdown in
docs/, built with Zensical; Design spec indocs/design/(7 pages: index, agents, organization, communication, engine, memory, operations); Architecture indocs/architecture/(overview, tech-stack, decision log); Roadmap indocs/roadmap/; Security indocs/security.md; Licensing indocs/licensing.md; Reference indocs/reference/
Files:
docs/api/memory.mddocs/design/memory.mddocs/architecture/tech-stack.md
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Use pytest markers:@pytest.mark.unit,@pytest.mark.integration,@pytest.mark.e2e,@pytest.mark.slowfor test categorization
Useasyncio_mode = 'auto'in pytest — no manual@pytest.mark.asyncioneeded
Per-test timeout: 30 seconds
Prefer@pytest.mark.parametrizefor testing similar cases
Usetest-provider,test-small-001, etc. in tests instead of real vendor names
Use Python Hypothesis for property-based testing with@given+@settingsdecorators
NEVER skip, dismiss, or ignore flaky tests — always fix them fully and fundamentally. For timing-sensitive tests, mocktime.monotonic()andasyncio.sleep()to make them deterministic
Files:
tests/unit/memory/consolidation/test_abstractive.pytests/unit/memory/consolidation/test_extractive.pytests/unit/memory/consolidation/test_density.pytests/unit/memory/consolidation/test_config.pytests/unit/memory/consolidation/test_dual_mode_strategy.pytests/unit/memory/test_init.pytests/unit/memory/consolidation/test_models.pytests/unit/memory/consolidation/test_service.py
🧠 Learnings (4)
📚 Learning: 2026-03-17T17:24:41.990Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T17:24:41.990Z
Learning: Applies to docs/**/*.md : Docs: Markdown in `docs/`, built with Zensical; Design spec in `docs/design/` (7 pages: index, agents, organization, communication, engine, memory, operations); Architecture in `docs/architecture/` (overview, tech-stack, decision log); Roadmap in `docs/roadmap/`; Security in `docs/security.md`; Licensing in `docs/licensing.md`; Reference in `docs/reference/`
Applied to files:
CLAUDE.mddocs/architecture/tech-stack.md
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to src/synthorg/**/*.py : Event names: always use constants from domain-specific modules under synthorg.observability.events (e.g., PROVIDER_CALL_START from events.provider, BUDGET_RECORD_ADDED from events.budget, etc.). Import directly: `from synthorg.observability.events.<domain> import EVENT_CONSTANT`.
Applied to files:
src/synthorg/observability/events/consolidation.py
📚 Learning: 2026-03-17T17:24:41.990Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T17:24:41.990Z
Learning: Applies to src/synthorg/**/*.py : Event names must always use constants from domain-specific modules under `synthorg.observability.events` (e.g., `API_REQUEST_STARTED` from `events.api`). Import directly: `from synthorg.observability.events.<domain> import EVENT_CONSTANT`
Applied to files:
src/synthorg/observability/events/consolidation.py
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Always read the relevant `docs/design/` page before implementing any feature or planning any issue. DESIGN_SPEC.md is a pointer file linking to the 7 design pages (index, agents, organization, communication, engine, memory, operations).
Applied to files:
docs/architecture/tech-stack.md
🧬 Code graph analysis (11)
tests/unit/memory/consolidation/test_abstractive.py (1)
src/synthorg/memory/consolidation/abstractive.py (3)
AbstractiveSummarizer(39-139)summarize(71-117)summarize_batch(119-139)
tests/unit/memory/consolidation/test_extractive.py (1)
src/synthorg/memory/consolidation/extractive.py (2)
ExtractivePreserver(96-171)extract(127-171)
src/synthorg/memory/consolidation/dual_mode_strategy.py (5)
src/synthorg/core/enums.py (1)
MemoryCategory(101-108)src/synthorg/memory/consolidation/abstractive.py (2)
AbstractiveSummarizer(39-139)summarize(71-117)src/synthorg/memory/consolidation/density.py (3)
ContentDensity(20-31)DensityClassifier(149-213)classify_batch(190-213)src/synthorg/memory/consolidation/extractive.py (2)
ExtractivePreserver(96-171)extract(127-171)src/synthorg/memory/consolidation/models.py (4)
ArchivalMode(24-34)ArchivalModeAssignment(37-52)ConsolidationResult(80-133)consolidated_count(131-133)
src/synthorg/memory/consolidation/service.py (1)
src/synthorg/memory/consolidation/models.py (4)
ArchivalIndexEntry(55-77)ArchivalMode(24-34)ArchivalModeAssignment(37-52)ConsolidationResult(80-133)
tests/unit/memory/consolidation/test_density.py (1)
src/synthorg/memory/consolidation/density.py (4)
ContentDensity(20-31)DensityClassifier(149-213)classify(170-188)classify_batch(190-213)
tests/unit/memory/consolidation/test_config.py (1)
src/synthorg/memory/consolidation/config.py (2)
DualModeConfig(51-112)ArchivalConfig(115-138)
tests/unit/memory/consolidation/test_dual_mode_strategy.py (4)
src/synthorg/core/enums.py (1)
MemoryCategory(101-108)src/synthorg/memory/consolidation/density.py (1)
DensityClassifier(149-213)src/synthorg/memory/consolidation/dual_mode_strategy.py (2)
DualModeConsolidationStrategy(43-283)consolidate(87-156)src/synthorg/memory/consolidation/models.py (2)
ArchivalMode(24-34)consolidated_count(131-133)
tests/unit/memory/consolidation/test_models.py (2)
src/synthorg/memory/consolidation/models.py (5)
ArchivalIndexEntry(55-77)ArchivalMode(24-34)ArchivalModeAssignment(37-52)ConsolidationResult(80-133)ArchivalEntry(136-177)src/synthorg/core/enums.py (1)
MemoryCategory(101-108)
tests/unit/memory/consolidation/test_service.py (2)
src/synthorg/memory/consolidation/models.py (3)
ArchivalMode(24-34)ArchivalModeAssignment(37-52)ConsolidationResult(80-133)src/synthorg/memory/consolidation/service.py (2)
MemoryConsolidationService(46-349)run_consolidation(75-136)
src/synthorg/memory/consolidation/__init__.py (5)
src/synthorg/memory/consolidation/abstractive.py (1)
AbstractiveSummarizer(39-139)src/synthorg/memory/consolidation/density.py (2)
ContentDensity(20-31)DensityClassifier(149-213)src/synthorg/memory/consolidation/dual_mode_strategy.py (1)
DualModeConsolidationStrategy(43-283)src/synthorg/memory/consolidation/extractive.py (1)
ExtractivePreserver(96-171)src/synthorg/memory/consolidation/models.py (4)
ArchivalEntry(136-177)ArchivalIndexEntry(55-77)ArchivalMode(24-34)ArchivalModeAssignment(37-52)
src/synthorg/memory/__init__.py (3)
src/synthorg/memory/consolidation/models.py (2)
ArchivalMode(24-34)ConsolidationResult(80-133)src/synthorg/memory/consolidation/density.py (1)
ContentDensity(20-31)src/synthorg/memory/consolidation/dual_mode_strategy.py (1)
DualModeConsolidationStrategy(43-283)
🪛 LanguageTool
docs/design/memory.md
[typographical] ~332-~332: The word ‘When’ starts a question. Add a question mark (“?”) at the end of the sentence.
Context: ...density before choosing an archival mode. This prevents catastrophic information ...
(WRB_QUESTION_MARK)
🔇 Additional comments (17)
docs/architecture/tech-stack.md (1)
121-121: LGTM!The documentation accurately reflects the new dual-mode consolidation capability, clearly distinguishing between the simple strategy (deduplication + summarization) and the density-aware strategy (abstractive for sparse, extractive for dense). The mention of deterministic index-based restore aligns with the PR objectives.
src/synthorg/observability/events/consolidation.py (1)
39-58: LGTM!New event constants follow the established
consolidation.<entity>.<action>naming convention and are properly organized into logical sections. The coverage is comprehensive for the dual-mode workflow: density classification, group classification, abstractive/extractive operations, and archival indexing.src/synthorg/memory/consolidation/__init__.py (1)
7-56: LGTM!The module correctly re-exports all new dual-mode components (density classifier, strategies, preservers, summarizer, configuration, and model types) for clean consumer imports. The
__all__list maintains alphabetical ordering, which aids maintainability.src/synthorg/memory/consolidation/density.py (5)
1-56: LGTM!The module structure is well-designed:
ContentDensityenum with clear docstrings explaining the classification impact- Weights with assertion guard ensuring they sum to 1.0 for threshold interpretability
- Clean separation of concerns between the enum and classifier class
57-147: LGTM!The heuristic scoring implementation is solid:
- Compiled regex patterns at module level optimize repeated classification calls
- Each scoring function is bounded to [0.0, 1.0] via
min(..., 1.0)- Division-by-zero protection with
max(len(...), 1)in all scoring functions- Clear inline comments explaining the patterns being matched
149-189: LGTM!The
DensityClassifierclass is well-implemented:
- Constructor validates threshold bounds with clear error message
classify()method correctly combines weighted scores- Threshold comparison uses
>=which aligns with the docstring ("DENSE if score >= threshold")
202-213: LGTM!The batch classification correctly:
- Preserves input order in results
- Only logs when results are non-empty (avoiding spurious debug logs)
- Uses structured logging with appropriate event constant and context keys
190-193: No issues found.The type annotation on line 192 is already correct:
entries: tuple[MemoryEntry, ...]. This matches the return type on line 193 and is consistent with the docstring. No changes are required.src/synthorg/memory/consolidation/service.py (4)
12-18: LGTM!The new model imports align with the dual-mode archival feature requirements. The import organization maintains consistency with existing imports.
107-120: LGTM!The
run_consolidationmethod correctly:
- Passes
mode_assignmentsfrom the strategy result to_archive_entries- Receives and propagates the
archival_indexto the finalConsolidationResult- Maintains backward compatibility (mode_assignments defaults to empty tuple in strategies that don't use dual-mode)
237-291: LGTM!The refactored
_archive_entriesmethod:
- Returns a tuple of
(archived_count, archival_index)for richer result propagation- Builds a mode map for O(1) lookup during per-entry archival
- Delegates to
_archive_single_entryfor cleaner separation of concerns- Logs
ARCHIVAL_INDEX_BUILTonly when index is non-empty
293-349: LGTM!The new
_archive_single_entryhelper:
- Properly handles the optional
archival_mode(None for legacy/non-dual-mode flows)- Returns
(success, index_entry | None)tuple for clean result aggregation- Logs failures at WARNING level without aborting the batch
- Only creates
ArchivalIndexEntrywhenarchival_modeis present, supporting the deterministic restore objectivesrc/synthorg/memory/consolidation/dual_mode_strategy.py (5)
1-41: LGTM!The module setup is clean with proper imports organized by source. The threshold constants
_DEFAULT_GROUP_THRESHOLD = 3and_MIN_GROUP_THRESHOLD = 2provide sensible defaults while allowing configuration.
43-86: LGTM!The
DualModeConsolidationStrategyclass:
- Has a comprehensive docstring explaining the grouping and majority-vote behavior
- Validates
group_threshold >= 2to ensure consolidation is meaningful- Accepts all required collaborators via constructor injection (good for testability)
87-156: LGTM!The
consolidate()method implementation:
- Handles empty input gracefully with early return and debug logging
- Uses
sorted()+groupby()correctly for category grouping- Only sets
summary_idfrom the first processed group, which aligns withConsolidationResult.summary_idbeing singular- Logs at INFO level for strategy start/complete (state transitions)
158-214: LGTM!The
_process_group()method:
- Correctly determines mode before processing
- The comment at lines 182-183 documents the tie-breaking behavior (50/50 → ABSTRACTIVE)
- Stores consolidated content before deleting originals (safe ordering)
- Creates
ArchivalModeAssignmentfor each removed entry
216-235: LGTM!The majority vote logic is correct:
- Uses strict
>comparison:dense_count > len(classified) / 2- This means 50/50 splits favor ABSTRACTIVE (the "safer" mode as noted in the comment)
- StrEnum comparison with
== ContentDensity.DENSEis correct
… reviewers Source fixes: - Use NotBlankStr | None for DualModeConfig.summarization_model (#1) - Parallelize LLM calls with asyncio.TaskGroup in summarize_batch and _build_content (#2) - Remove dead-code guards in _build_anchors (#3) - Narrow except Exception to re-raise non-retryable ProviderErrors (#4) - Fix double-logging on abstractive fallback (#9) - Remove unnecessary import builtins (#10) - Preserve key-value pairs verbatim in extractive mode (#5) - Emit extracted facts one per line (#6) - Strengthen ConsolidationResult validator with cross-field checks (#7) - Check _backend.delete() return value in _process_group (#8) - Fix mode_map type to dict[NotBlankStr, ArchivalMode] (#11) - Move tie-breaking comment to _determine_group_mode (#12) - Fix misleading DualModeConfig docstring (#13) - Add missing mkdocstrings entries for retention/archival/simple_strategy (#14) - Use O(M) lookup dict in _archive_entries (#15) - Document 1000-entry query limit in run_consolidation docstring (#16) - Add Raises section to AbstractiveSummarizer docstring (#17) Test fixes: - Fix imports in test_density.py to module level (#18) - Strengthen fallback assertion to verify exact content (#19) - Use exact call counts for summarizer/extractor (#20) - Add tests: blank model rejection, MemoryError/RecursionError propagation (#21, #22) - Add tests: validator rejects invalid archival state (#23) - Add tests: 50/50 tie-breaking, None relevance handling (#24, #25) - Assert actual preserved facts in extractive tests (#26) - Prove archival index keyed by original_id not position (#27) - Add test: empty string classifies as SPARSE (#28)
There was a problem hiding this comment.
Actionable comments posted: 10
♻️ Duplicate comments (1)
tests/unit/memory/consolidation/test_dual_mode_strategy.py (1)
75-87: 🧹 Nitpick | 🔵 TrivialStop defaulting the helper to the real classifier.
_make_strategy()still injectsDensityClassifier(), so the sparse/dense/tie suites are really integration tests fordensity.py. A weight tweak there can break this file even whenDualModeConsolidationStrategyis correct.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/memory/consolidation/test_dual_mode_strategy.py` around lines 75 - 87, The helper _make_strategy currently instantiates a real DensityClassifier() by default, coupling these unit tests to density.py; change the default to a test-double instead (e.g., a Mock or a simple stub implementing the same interface as DensityClassifier) and use that when classifier is None so the sparse/dense/tie suites can inject deterministic classifier behavior; update _make_strategy to accept and pass through the mock/stub to DualModeConsolidationStrategy and ensure tests explicitly provide the classifier they need rather than relying on the real DensityClassifier.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/synthorg/memory/consolidation/abstractive.py`:
- Around line 108-110: In the except ProviderError branch inside summarize(),
log the non-retryable error with context before re-raising: detect the path
where "if not exc.is_retryable" currently just raises, call the module/class
logger (the same logger used elsewhere in this file, e.g., logger or
self.logger) to emit a WARNING or ERROR including that this is a non-retryable
ProviderError, the exception message/stack (exc), and relevant context (e.g.,
model/provider info), then re-raise the exception as before; reference
ProviderError and summarize() to locate where to add the logging.
In `@src/synthorg/memory/consolidation/dual_mode_strategy.py`:
- Around line 88-157: consolidate() and _process_group() are over the 50-line
limit and mix grouping, persistence, deletion, and result-shaping; refactor by
extracting small helper methods (e.g., group_entries_by_category,
filter_small_groups, handle_group_processing, apply_persistence_and_deletion,
and build_consolidation_result) so each function does one responsibility and
stays under 50 lines. Move grouping logic from consolidate into
group_entries_by_category and filtering into filter_small_groups, move the
per-category orchestration inside _process_group into handle_group_processing
that returns (new_id, removed_ids, mode_assignments) without side effects, and
move any persistence/deletion calls into apply_persistence_and_deletion called
from consolidate (or a new orchestrator) so consolidate only iterates groups,
delegates work, collects results, and calls build_consolidation_result to
assemble the ConsolidationResult. Ensure signatures reference consolidate and
_process_group (and the new helpers) and preserve existing return types and
logging calls.
- Around line 129-147: The code only keeps the first new summary id in
summary_id, so later consolidated groups' new_ids are lost; change summary_id
from a scalar to a collection: initialize summary_ids = [] before the loop,
append each new_id returned by _process_group, and after the loop pass
tuple(summary_ids) into the ConsolidationResult (or add a new
summaries/summary_ids field to ConsolidationResult and update its
constructor/signature accordingly); ensure any downstream code that expected a
single summary_id is updated to handle the tuple (or to use the first element if
backwards compatibility is required).
In `@src/synthorg/memory/consolidation/extractive.py`:
- Around line 29-30: The current _VERSION_PATTERN also matches IPv4 addresses
(dot-quad), so update the regex used in _VERSION_PATTERN (and the same pattern
occurrences around the block referenced at lines 49-51) to explicitly exclude
4-part numeric dot-quad sequences by adding a negative lookahead for
\d+\.\d+\.\d+\.\d+ (for example:
re.compile(r"\b(?!\d+\.\d+\.\d+\.\d+\b)v?\d+\.\d+\.\d+(?:\.\d+)?\b")). Replace
the existing _VERSION_PATTERN and the other identical patterns at the referenced
code locations with this revised regex so IPs are not treated as versions.
In `@src/synthorg/memory/consolidation/models.py`:
- Around line 118-151: The _validate_archival_consistency model_validator
currently misses checks for duplicate/invalid removed_ids and only length-checks
mode_assignments; update ConsolidationResult._validate_archival_consistency to
(1) validate that removed_ids contains no duplicate IDs (raise ValueError if
len(removed_ids) != len(set(removed_ids))), (2) ensure every id referenced in
archival_index and in any mode_assignments mapping exists in removed_ids (reject
missing ids), and (3) ensure mode_assignments is one-to-one with removed_ids
(either require len(mode_assignments) == len(removed_ids) or validate that the
set of keys in mode_assignments equals set(removed_ids) depending on intended
semantics) so duplicates in removed_ids cannot double-count consolidation runs
and mode map construction cannot silently collapse assignments.
- Around line 55-77: The ArchivalIndexEntry currently requires mode which breaks
indexing for archives created with no archival_mode; update ArchivalIndexEntry
to accept a nullable mode by changing its type to Optional[ArchivalMode] and set
Field(default=None, description=...) so entries without a mode can be indexed,
and make the same change for the other model referenced (the second occurrence
around the alternate definition at lines 187-190) so both index models tolerate
a missing archival_mode.
In `@src/synthorg/memory/consolidation/service.py`:
- Around line 271-274: The loop over removed_ids silently skips IDs not found in
entry_map; add a warning log before the continue so missing removed_id values
are visible (e.g., logger.warning or self.logger.warning) including the
removed_id and context (e.g., the strategy name or relevant batch id), placed
where entry = entry_map.get(removed_id) is checked and entry is None; this
preserves current control flow but surfaces contract violations between the
strategy and service.
In `@tests/unit/memory/consolidation/test_abstractive.py`:
- Around line 151-180: The test's final assertion is too weak; in
test_batch_continues_on_individual_failure replace the non-empty check with an
exact expectation for the fallback payload—assert that
summarizer.summarize_batch returns the original entry text for the failed item
(i.e., assert result[1][1] == "Second") so the behavior of
AbstractiveSummarizer.summarize_batch and provider.complete fallback is
precisely validated (use the existing _make_entry and provider.complete setup to
identify the failing item).
In `@tests/unit/memory/consolidation/test_extractive.py`:
- Around line 39-46: The test test_extracts_key_value_pairs in
ExtractivePreserver misses a blank-value regression case that
_extract_key_values now accepts; update this test to include a key with an empty
assignment (e.g., append "API_KEY=" to the content string) and add an assertion
that the resulting extraction still contains the literal "API_KEY=" so empty
assignments survive extraction and prevent a lossy config regression.
In `@tests/unit/memory/consolidation/test_models.py`:
- Around line 210-315: Add two regression tests to cover the new ID invariants:
one that constructs ConsolidationResult with duplicate removed_ids (e.g.,
removed_ids=("m1","m1")) and asserts a ValidationError is raised, and another
that passes mode_assignments containing an ArchivalModeAssignment whose
original_id is not present in removed_ids (or duplicates not allowed) and
asserts a ValidationError is raised. Reference the ConsolidationResult
constructor, the removed_ids and mode_assignments parameters, and
ArchivalModeAssignment/ArchivalMode symbols when adding these tests to mirror
the existing test style and error-match patterns.
---
Duplicate comments:
In `@tests/unit/memory/consolidation/test_dual_mode_strategy.py`:
- Around line 75-87: The helper _make_strategy currently instantiates a real
DensityClassifier() by default, coupling these unit tests to density.py; change
the default to a test-double instead (e.g., a Mock or a simple stub implementing
the same interface as DensityClassifier) and use that when classifier is None so
the sparse/dense/tie suites can inject deterministic classifier behavior; update
_make_strategy to accept and pass through the mock/stub to
DualModeConsolidationStrategy and ensure tests explicitly provide the classifier
they need rather than relying on the real DensityClassifier.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3b9080b1-7d9d-49b1-bf08-7046106a3b3c
📒 Files selected for processing (14)
docs/api/memory.mdsrc/synthorg/memory/consolidation/abstractive.pysrc/synthorg/memory/consolidation/config.pysrc/synthorg/memory/consolidation/dual_mode_strategy.pysrc/synthorg/memory/consolidation/extractive.pysrc/synthorg/memory/consolidation/models.pysrc/synthorg/memory/consolidation/service.pytests/unit/memory/consolidation/test_abstractive.pytests/unit/memory/consolidation/test_config.pytests/unit/memory/consolidation/test_density.pytests/unit/memory/consolidation/test_dual_mode_strategy.pytests/unit/memory/consolidation/test_extractive.pytests/unit/memory/consolidation/test_models.pytests/unit/memory/consolidation/test_service.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Test (Python 3.14)
- GitHub Check: Build Backend
- GitHub Check: Build Web
- GitHub Check: Build Sandbox
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (4)
docs/**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
Docs: Markdown in
docs/, built with Zensical; Design spec indocs/design/(7 pages: index, agents, organization, communication, engine, memory, operations); Architecture indocs/architecture/(overview, tech-stack, decision log); Roadmap indocs/roadmap/; Security indocs/security.md; Licensing indocs/licensing.md; Reference indocs/reference/
Files:
docs/api/memory.md
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Nofrom __future__ import annotations— Python 3.14 has PEP 649 with native lazy annotations
Useexcept A, B:syntax (no parentheses) for exception handling in Python 3.14, as enforced by ruff PEP 758
All public functions and methods require type hints; mypy strict mode enforced
Docstrings: Google style, required on all public classes and functions, enforced by ruff D rules
Create new objects instead of mutating existing ones; never mutate. For non-Pydantic internal collections (registries,BaseTool), usecopy.deepcopy()at construction +MappingProxyTypewrapping for read-only enforcement.
Fordict/listfields in frozen Pydantic models, rely onfrozen=Truefor field reassignment prevention andcopy.deepcopy()at system boundaries (tool execution, LLM provider serialization, inter-agent delegation, serializing for persistence)
Use frozen Pydantic models for config/identity; use separate mutable-via-copy models (usingmodel_copy(update=...)) for runtime state that evolves. Never mix static config fields with mutable runtime fields in one model.
Use Pydantic v2 with adopted conventions:@computed_fieldfor derived values instead of storing + validating redundant fields; useNotBlankStrfromcore.typesfor all identifier/name fields (including optional and tuple variants) instead of manual whitespace validators
Preferasyncio.TaskGroupfor fan-out/fan-in parallel operations in new code (multiple tool invocations, parallel agent calls). Prefer structured concurrency over barecreate_task.
Line length: 88 characters (enforced by ruff)
Functions must be < 50 lines; files must be < 800 lines
Handle errors explicitly, never silently swallow them
Validate at system boundaries (user input, external APIs, config files)
Files:
tests/unit/memory/consolidation/test_config.pysrc/synthorg/memory/consolidation/config.pysrc/synthorg/memory/consolidation/extractive.pytests/unit/memory/consolidation/test_extractive.pysrc/synthorg/memory/consolidation/models.pytests/unit/memory/consolidation/test_density.pysrc/synthorg/memory/consolidation/service.pytests/unit/memory/consolidation/test_dual_mode_strategy.pysrc/synthorg/memory/consolidation/abstractive.pytests/unit/memory/consolidation/test_service.pysrc/synthorg/memory/consolidation/dual_mode_strategy.pytests/unit/memory/consolidation/test_abstractive.pytests/unit/memory/consolidation/test_models.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Use pytest markers:@pytest.mark.unit,@pytest.mark.integration,@pytest.mark.e2e,@pytest.mark.slowfor test categorization
Useasyncio_mode = 'auto'in pytest — no manual@pytest.mark.asyncioneeded
Per-test timeout: 30 seconds
Prefer@pytest.mark.parametrizefor testing similar cases
Usetest-provider,test-small-001, etc. in tests instead of real vendor names
Use Python Hypothesis for property-based testing with@given+@settingsdecorators
NEVER skip, dismiss, or ignore flaky tests — always fix them fully and fundamentally. For timing-sensitive tests, mocktime.monotonic()andasyncio.sleep()to make them deterministic
Files:
tests/unit/memory/consolidation/test_config.pytests/unit/memory/consolidation/test_extractive.pytests/unit/memory/consolidation/test_density.pytests/unit/memory/consolidation/test_dual_mode_strategy.pytests/unit/memory/consolidation/test_service.pytests/unit/memory/consolidation/test_abstractive.pytests/unit/memory/consolidation/test_models.py
src/synthorg/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/synthorg/**/*.py: Every module with business logic MUST havefrom synthorg.observability import get_loggerthenlogger = get_logger(__name__); never useimport logging/logging.getLogger()/print()in application code
Logger variable name must always belogger(not_logger, notlog)
Event names must always use constants from domain-specific modules undersynthorg.observability.events(e.g.,API_REQUEST_STARTEDfromevents.api). Import directly:from synthorg.observability.events.<domain> import EVENT_CONSTANT
Always use structured logging:logger.info(EVENT, key=value)— neverlogger.info('msg %s', val)
All error paths must log at WARNING or ERROR with context before raising
All state transitions must log at INFO level
DEBUG logging for object creation, internal flow, and entry/exit of key functions
All provider calls go throughBaseCompletionProviderwhich applies retry + rate limiting automatically. Never implement retry logic in driver subclasses or calling code.
RetryConfig and RateLimiterConfig are set per-provider inProviderConfig; retryable errors (is_retryable=True) includeRateLimitError,ProviderTimeoutError,ProviderConnectionError,ProviderInternalError
Non-retryable errors raise immediately without retry;RetryExhaustedErrorsignals that all retries failed — engine layer catches this to trigger fallback chains
Rate limiter respectsRateLimitError.retry_afterfrom providers — automatically pauses future requests
Library reference: auto-generated from docstrings via mkdocstrings + Griffe (AST-based, no imports) indocs/api/
Files:
src/synthorg/memory/consolidation/config.pysrc/synthorg/memory/consolidation/extractive.pysrc/synthorg/memory/consolidation/models.pysrc/synthorg/memory/consolidation/service.pysrc/synthorg/memory/consolidation/abstractive.pysrc/synthorg/memory/consolidation/dual_mode_strategy.py
🧠 Learnings (5)
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to **/*.py : Models: Pydantic v2 (BaseModel, model_validator, computed_field, ConfigDict). Use computed_field for derived values instead of storing + validating redundant fields. Use NotBlankStr (from core.types) for all identifier/name fields — including optional (NotBlankStr | None) and tuple (tuple[NotBlankStr, ...]) variants — instead of manual whitespace validators.
Applied to files:
src/synthorg/memory/consolidation/config.py
📚 Learning: 2026-03-17T17:24:41.990Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T17:24:41.990Z
Learning: Applies to **/*.py : Use Pydantic v2 with adopted conventions: `computed_field` for derived values instead of storing + validating redundant fields; use `NotBlankStr` from `core.types` for all identifier/name fields (including optional and tuple variants) instead of manual whitespace validators
Applied to files:
src/synthorg/memory/consolidation/config.py
📚 Learning: 2026-03-17T17:24:41.990Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T17:24:41.990Z
Learning: Applies to **/*.py : Use `except A, B:` syntax (no parentheses) for exception handling in Python 3.14, as enforced by ruff PEP 758
Applied to files:
src/synthorg/memory/consolidation/abstractive.py
📚 Learning: 2026-03-17T17:24:41.990Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T17:24:41.990Z
Learning: Applies to **/*.py : Prefer `asyncio.TaskGroup` for fan-out/fan-in parallel operations in new code (multiple tool invocations, parallel agent calls). Prefer structured concurrency over bare `create_task`.
Applied to files:
src/synthorg/memory/consolidation/abstractive.pysrc/synthorg/memory/consolidation/dual_mode_strategy.py
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to **/*.py : Async concurrency: prefer asyncio.TaskGroup for fan-out/fan-in parallel operations in new code (e.g. multiple tool invocations, parallel agent calls). Prefer structured concurrency over bare create_task.
Applied to files:
src/synthorg/memory/consolidation/abstractive.pysrc/synthorg/memory/consolidation/dual_mode_strategy.py
🧬 Code graph analysis (7)
tests/unit/memory/consolidation/test_config.py (1)
src/synthorg/memory/consolidation/config.py (2)
DualModeConfig(52-113)ArchivalConfig(116-139)
tests/unit/memory/consolidation/test_extractive.py (1)
src/synthorg/memory/consolidation/extractive.py (2)
ExtractivePreserver(91-167)extract(122-167)
tests/unit/memory/consolidation/test_density.py (2)
src/synthorg/core/enums.py (1)
MemoryCategory(101-108)src/synthorg/memory/consolidation/density.py (4)
ContentDensity(20-31)DensityClassifier(149-213)classify(170-188)classify_batch(190-213)
src/synthorg/memory/consolidation/service.py (1)
src/synthorg/memory/consolidation/models.py (5)
ArchivalEntry(160-201)ArchivalIndexEntry(55-77)ArchivalMode(24-34)ArchivalModeAssignment(37-52)ConsolidationResult(80-157)
tests/unit/memory/consolidation/test_service.py (1)
src/synthorg/memory/consolidation/models.py (3)
ArchivalMode(24-34)ArchivalModeAssignment(37-52)ConsolidationResult(80-157)
tests/unit/memory/consolidation/test_abstractive.py (2)
src/synthorg/core/enums.py (1)
MemoryCategory(101-108)tests/unit/memory/consolidation/test_service.py (1)
_make_entry(29-37)
tests/unit/memory/consolidation/test_models.py (2)
src/synthorg/memory/consolidation/models.py (4)
ArchivalIndexEntry(55-77)ArchivalMode(24-34)ArchivalModeAssignment(37-52)ConsolidationResult(80-157)src/synthorg/core/enums.py (1)
MemoryCategory(101-108)
🔇 Additional comments (4)
tests/unit/memory/consolidation/test_density.py (1)
14-162: Classifier test coverage looks solid.These cases pin down the new enum behavior, sparse/dense examples, threshold boundaries, and batch ordering well.
tests/unit/memory/consolidation/test_abstractive.py (1)
19-129: Single-entry summarizer coverage is strong.The helpers keep the tests readable, and the happy path, empty-response fallback, blank-model validation, and non-fallback exception cases are all pinned down cleanly.
tests/unit/memory/consolidation/test_models.py (1)
140-207: The new enum/model smoke tests are useful.The added checks around
ArchivalMode,ArchivalModeAssignment,ArchivalIndexEntry, andArchivalEntry.archival_modecover the basic construction and frozen-model behavior well.Also applies to: 318-344
src/synthorg/memory/consolidation/dual_mode_strategy.py (1)
272-295: Good use ofasyncio.TaskGroupin_build_content().This keeps the abstractive fan-out structured while still preserving input order via the
taskslist.Based on learnings, "Prefer
asyncio.TaskGroupfor fan-out/fan-in parallel operations in new code (multiple tool invocations, parallel agent calls). Prefer structured concurrency over barecreate_task."
| async def consolidate( | ||
| self, | ||
| entries: tuple[MemoryEntry, ...], | ||
| *, | ||
| agent_id: NotBlankStr, | ||
| ) -> ConsolidationResult: | ||
| """Consolidate entries using density-aware dual-mode approach. | ||
|
|
||
| Groups entries by category, classifies density, selects archival | ||
| mode by majority vote, then processes entries accordingly. | ||
|
|
||
| Args: | ||
| entries: Memory entries to consolidate. | ||
| agent_id: Owning agent identifier. | ||
|
|
||
| Returns: | ||
| Result describing what was consolidated. | ||
| """ | ||
| if not entries: | ||
| logger.debug( | ||
| STRATEGY_COMPLETE, | ||
| agent_id=agent_id, | ||
| consolidated_count=0, | ||
| strategy="dual_mode", | ||
| ) | ||
| return ConsolidationResult() | ||
|
|
||
| logger.info( | ||
| STRATEGY_START, | ||
| agent_id=agent_id, | ||
| entry_count=len(entries), | ||
| strategy="dual_mode", | ||
| ) | ||
|
|
||
| removed_ids: list[NotBlankStr] = [] | ||
| summary_id: NotBlankStr | None = None | ||
| mode_assignments: list[ArchivalModeAssignment] = [] | ||
|
|
||
| sorted_entries = sorted(entries, key=attrgetter("category")) | ||
| groups = groupby(sorted_entries, key=attrgetter("category")) | ||
|
|
||
| for category, group_iter in groups: | ||
| group = list(group_iter) | ||
| if len(group) < self._group_threshold: | ||
| continue | ||
| new_id, group_removed, group_modes = await self._process_group( | ||
| category, | ||
| group, | ||
| agent_id, | ||
| ) | ||
| if summary_id is None: | ||
| summary_id = new_id | ||
| removed_ids.extend(group_removed) | ||
| mode_assignments.extend(group_modes) | ||
|
|
||
| result = ConsolidationResult( | ||
| removed_ids=tuple(removed_ids), | ||
| summary_id=summary_id, | ||
| mode_assignments=tuple(mode_assignments), | ||
| ) | ||
|
|
||
| logger.info( | ||
| STRATEGY_COMPLETE, | ||
| agent_id=agent_id, | ||
| consolidated_count=result.consolidated_count, | ||
| summary_id=result.summary_id, | ||
| strategy="dual_mode", | ||
| ) | ||
|
|
||
| return result |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Split the orchestration methods.
consolidate() and _process_group() are already over the repo's 50-line limit, and each mixes grouping, persistence, deletion, and result shaping. Extracting helpers here will make the multi-step failure paths easier to reason about.
As per coding guidelines, "Functions must be < 50 lines; files must be < 800 lines."
Also applies to: 159-222
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/synthorg/memory/consolidation/dual_mode_strategy.py` around lines 88 -
157, consolidate() and _process_group() are over the 50-line limit and mix
grouping, persistence, deletion, and result-shaping; refactor by extracting
small helper methods (e.g., group_entries_by_category, filter_small_groups,
handle_group_processing, apply_persistence_and_deletion, and
build_consolidation_result) so each function does one responsibility and stays
under 50 lines. Move grouping logic from consolidate into
group_entries_by_category and filtering into filter_small_groups, move the
per-category orchestration inside _process_group into handle_group_processing
that returns (new_id, removed_ids, mode_assignments) without side effects, and
move any persistence/deletion calls into apply_persistence_and_deletion called
from consolidate (or a new orchestrator) so consolidate only iterates groups,
delegates work, collects results, and calls build_consolidation_result to
assemble the ConsolidationResult. Ensure signatures reference consolidate and
_process_group (and the new helpers) and preserve existing return types and
logging calls.
| for category, group_iter in groups: | ||
| group = list(group_iter) | ||
| if len(group) < self._group_threshold: | ||
| continue | ||
| new_id, group_removed, group_modes = await self._process_group( | ||
| category, | ||
| group, | ||
| agent_id, | ||
| ) | ||
| if summary_id is None: | ||
| summary_id = new_id | ||
| removed_ids.extend(group_removed) | ||
| mode_assignments.extend(group_modes) | ||
|
|
||
| result = ConsolidationResult( | ||
| removed_ids=tuple(removed_ids), | ||
| summary_id=summary_id, | ||
| mode_assignments=tuple(mode_assignments), | ||
| ) |
There was a problem hiding this comment.
summary_id loses later consolidated groups.
This strategy stores one new memory per qualifying category, but consolidate() only preserves the first new_id. If multiple groups consolidate in one run, the remaining stored summaries become unreachable from ConsolidationResult.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/synthorg/memory/consolidation/dual_mode_strategy.py` around lines 129 -
147, The code only keeps the first new summary id in summary_id, so later
consolidated groups' new_ids are lost; change summary_id from a scalar to a
collection: initialize summary_ids = [] before the loop, append each new_id
returned by _process_group, and after the loop pass tuple(summary_ids) into the
ConsolidationResult (or add a new summaries/summary_ids field to
ConsolidationResult and update its constructor/signature accordingly); ensure
any downstream code that expected a single summary_id is updated to handle the
tuple (or to use the first element if backwards compatibility is required).
| class ArchivalIndexEntry(BaseModel): | ||
| """Maps a removed memory entry to its archival store ID. | ||
|
|
||
| Enables deterministic index-based restore: agents can look up | ||
| their own archived entries by original ID without semantic search. | ||
|
|
||
| Attributes: | ||
| original_id: ID of the original memory entry. | ||
| archival_id: ID assigned by the archival store. | ||
| mode: Archival mode used for this entry. | ||
| """ | ||
|
|
||
| model_config = ConfigDict(frozen=True, allow_inf_nan=False) | ||
|
|
||
| original_id: NotBlankStr = Field( | ||
| description="ID of the original memory entry", | ||
| ) | ||
| archival_id: NotBlankStr = Field( | ||
| description="ID assigned by the archival store", | ||
| ) | ||
| mode: ArchivalMode = Field( | ||
| description="Archival mode used for this entry", | ||
| ) |
There was a problem hiding this comment.
Don't make restore indexing depend on archival_mode.
ArchivalEntry.archival_mode is nullable here, so archive records can legitimately exist without a mode. Requiring ArchivalIndexEntry.mode forces the service into a lossy branch where successfully archived entries from non-dual-mode strategies cannot be indexed at all, which makes restore-by-original_id unavailable for those runs.
Also applies to: 187-190
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/synthorg/memory/consolidation/models.py` around lines 55 - 77, The
ArchivalIndexEntry currently requires mode which breaks indexing for archives
created with no archival_mode; update ArchivalIndexEntry to accept a nullable
mode by changing its type to Optional[ArchivalMode] and set Field(default=None,
description=...) so entries without a mode can be indexed, and make the same
change for the other model referenced (the second occurrence around the
alternate definition at lines 187-190) so both index models tolerate a missing
archival_mode.
- Log non-retryable ProviderError before re-raising in summarize() - Exclude IPv4 literals from version extraction regex - Log warning when removed_id not found in retrieved entries - Add duplicate removed_ids validation to ConsolidationResult - Strengthen batch failure assertion to verify exact fallback content - Add blank-value key=value regression test for extractive mode
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
tests/unit/memory/consolidation/test_models.py (1)
220-324:⚠️ Potential issue | 🟡 MinorAdd regression cases for invalid
mode_assignments.This block exercises
removed_idsandarchival_indexconsistency, but it still doesn't assert that unknown or duplicatemode_assignments.original_idvalues are rejected. Without those cases, the validator gap can regress without a failing test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/memory/consolidation/test_models.py` around lines 220 - 324, Add tests that assert ConsolidationResult rejects mode_assignments with unknown or duplicate original_id values: create cases where mode_assignments contains an ArchivalModeAssignment whose original_id is not in removed_ids (e.g., original_id="m99") and where mode_assignments contains duplicate original_id entries (e.g., two ArchivalModeAssignment with original_id="m1"), each wrapped in pytest.raises(ValidationError, match=...) similar to existing tests; reference the ConsolidationResult constructor and the mode_assignments / ArchivalModeAssignment symbols when adding these new assertions to the test file.src/synthorg/memory/consolidation/models.py (1)
136-154:⚠️ Potential issue | 🟠 MajorValidate
mode_assignmentsby ID, not just by count.This validator still accepts duplicate assignments or assignments for IDs that were never removed. The service later materializes a
mode_mapfrom this tuple, so duplicates silently overwrite earlier modes and stray IDs are ignored. Requiremode_assignments.original_idvalues to be unique members ofremoved_ids(or an exact match when dual-mode classification is present).Suggested validator tightening
if len(self.mode_assignments) > len(self.removed_ids): msg = ( f"mode_assignments length ({len(self.mode_assignments)}) " f"must not exceed removed_ids length " f"({len(self.removed_ids)})" ) raise ValueError(msg) removed_set = set(self.removed_ids) + assignment_ids = [ + assignment.original_id for assignment in self.mode_assignments + ] + if len(assignment_ids) != len(set(assignment_ids)): + msg = "mode_assignments contains duplicate original_ids" + raise ValueError(msg) + if any( + assignment_id not in removed_set for assignment_id in assignment_ids + ): + msg = "mode_assignments contains original_ids not in removed_ids" + raise ValueError(msg) for idx_entry in self.archival_index:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/synthorg/memory/consolidation/models.py` around lines 136 - 154, The validator currently only checks counts; update it to validate each ModeAssignment's original_id: iterate self.mode_assignments, collect original_ids, ensure there are no duplicates (len(list) == len(set)), and verify every original_id is a member of self.removed_ids (or when dual-mode classification is used require an exact match policy as implemented elsewhere), raising ValueError with a clear message if a duplicate or a non-member is found; reference the self.mode_assignments entries' original_id, self.removed_ids, and the existing archival_index checks so the new checks run alongside those validations and prevent silent overwrites when building the mode_map.src/synthorg/memory/consolidation/service.py (1)
350-357:⚠️ Potential issue | 🟠 MajorDon't drop restore metadata when
archival_modeis missing.This branch omits
ArchivalIndexEntryfor successful archives whenevermode_assignmentsis empty. That leavesarchival_indexincomplete for legacy/non-dual-mode runs, so restore-by-original_idstops working even thougharchive()succeeded. MakeArchivalIndexEntry.modenullable and always emit the index row here.Build the index row unconditionally after making `mode` optional
- index_entry = ( - ArchivalIndexEntry( - original_id=entry.id, - archival_id=archival_id, - mode=archival_mode, - ) - if archival_mode is not None - else None - ) + index_entry = ArchivalIndexEntry( + original_id=entry.id, + archival_id=archival_id, + mode=archival_mode, + ) return True, index_entry🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/synthorg/memory/consolidation/service.py` around lines 350 - 357, The code currently skips creating an ArchivalIndexEntry when archival_mode is None which drops restore metadata; update the ArchivalIndexEntry model to allow mode to be nullable (make its mode/assignment field optional) and change the logic in the consolidation service so index_entry is constructed unconditionally (always create ArchivalIndexEntry(original_id=entry.id, archival_id=archival_id, mode=archival_mode) even when archival_mode is None) and ensure archival_index (or wherever entries are collected) always receives that index_entry so restore-by-original_id continues to work for legacy/non-dual-mode runs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/unit/memory/consolidation/test_abstractive.py`:
- Around line 151-181: The test test_batch_continues_on_individual_failure is
flaky because provider.complete uses a positional side_effect list and
summarize_batch (which uses asyncio.TaskGroup) races the calls; change
provider.complete to a deterministic side_effect callable that inspects the
incoming request (e.g., the user content or prompt from the call) and returns a
successful CompletionResponse for the entry with content "First" and raises
RuntimeError for the entry with content "Second" so the behavior no longer
depends on scheduling; update the mock setup in the test to use that callable
(targeting provider.complete) and, if needed for other timing sensitivity, mock
time.monotonic/asyncio.sleep to fixed values to ensure full determinism.
---
Duplicate comments:
In `@src/synthorg/memory/consolidation/models.py`:
- Around line 136-154: The validator currently only checks counts; update it to
validate each ModeAssignment's original_id: iterate self.mode_assignments,
collect original_ids, ensure there are no duplicates (len(list) == len(set)),
and verify every original_id is a member of self.removed_ids (or when dual-mode
classification is used require an exact match policy as implemented elsewhere),
raising ValueError with a clear message if a duplicate or a non-member is found;
reference the self.mode_assignments entries' original_id, self.removed_ids, and
the existing archival_index checks so the new checks run alongside those
validations and prevent silent overwrites when building the mode_map.
In `@src/synthorg/memory/consolidation/service.py`:
- Around line 350-357: The code currently skips creating an ArchivalIndexEntry
when archival_mode is None which drops restore metadata; update the
ArchivalIndexEntry model to allow mode to be nullable (make its mode/assignment
field optional) and change the logic in the consolidation service so index_entry
is constructed unconditionally (always create
ArchivalIndexEntry(original_id=entry.id, archival_id=archival_id,
mode=archival_mode) even when archival_mode is None) and ensure archival_index
(or wherever entries are collected) always receives that index_entry so
restore-by-original_id continues to work for legacy/non-dual-mode runs.
In `@tests/unit/memory/consolidation/test_models.py`:
- Around line 220-324: Add tests that assert ConsolidationResult rejects
mode_assignments with unknown or duplicate original_id values: create cases
where mode_assignments contains an ArchivalModeAssignment whose original_id is
not in removed_ids (e.g., original_id="m99") and where mode_assignments contains
duplicate original_id entries (e.g., two ArchivalModeAssignment with
original_id="m1"), each wrapped in pytest.raises(ValidationError, match=...)
similar to existing tests; reference the ConsolidationResult constructor and the
mode_assignments / ArchivalModeAssignment symbols when adding these new
assertions to the test file.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c8b0bce4-8a92-49bd-9c17-cf380623070e
📒 Files selected for processing (7)
src/synthorg/memory/consolidation/abstractive.pysrc/synthorg/memory/consolidation/extractive.pysrc/synthorg/memory/consolidation/models.pysrc/synthorg/memory/consolidation/service.pytests/unit/memory/consolidation/test_abstractive.pytests/unit/memory/consolidation/test_extractive.pytests/unit/memory/consolidation/test_models.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Test (Python 3.14)
- GitHub Check: Build Sandbox
- GitHub Check: Build Web
- GitHub Check: Build Backend
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Nofrom __future__ import annotations— Python 3.14 has PEP 649 with native lazy annotations
Useexcept A, B:syntax (no parentheses) for exception handling in Python 3.14, as enforced by ruff PEP 758
All public functions and methods require type hints; mypy strict mode enforced
Docstrings: Google style, required on all public classes and functions, enforced by ruff D rules
Create new objects instead of mutating existing ones; never mutate. For non-Pydantic internal collections (registries,BaseTool), usecopy.deepcopy()at construction +MappingProxyTypewrapping for read-only enforcement.
Fordict/listfields in frozen Pydantic models, rely onfrozen=Truefor field reassignment prevention andcopy.deepcopy()at system boundaries (tool execution, LLM provider serialization, inter-agent delegation, serializing for persistence)
Use frozen Pydantic models for config/identity; use separate mutable-via-copy models (usingmodel_copy(update=...)) for runtime state that evolves. Never mix static config fields with mutable runtime fields in one model.
Use Pydantic v2 with adopted conventions:@computed_fieldfor derived values instead of storing + validating redundant fields; useNotBlankStrfromcore.typesfor all identifier/name fields (including optional and tuple variants) instead of manual whitespace validators
Preferasyncio.TaskGroupfor fan-out/fan-in parallel operations in new code (multiple tool invocations, parallel agent calls). Prefer structured concurrency over barecreate_task.
Line length: 88 characters (enforced by ruff)
Functions must be < 50 lines; files must be < 800 lines
Handle errors explicitly, never silently swallow them
Validate at system boundaries (user input, external APIs, config files)
Files:
src/synthorg/memory/consolidation/models.pytests/unit/memory/consolidation/test_models.pysrc/synthorg/memory/consolidation/extractive.pytests/unit/memory/consolidation/test_extractive.pytests/unit/memory/consolidation/test_abstractive.pysrc/synthorg/memory/consolidation/service.pysrc/synthorg/memory/consolidation/abstractive.py
src/synthorg/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/synthorg/**/*.py: Every module with business logic MUST havefrom synthorg.observability import get_loggerthenlogger = get_logger(__name__); never useimport logging/logging.getLogger()/print()in application code
Logger variable name must always belogger(not_logger, notlog)
Event names must always use constants from domain-specific modules undersynthorg.observability.events(e.g.,API_REQUEST_STARTEDfromevents.api). Import directly:from synthorg.observability.events.<domain> import EVENT_CONSTANT
Always use structured logging:logger.info(EVENT, key=value)— neverlogger.info('msg %s', val)
All error paths must log at WARNING or ERROR with context before raising
All state transitions must log at INFO level
DEBUG logging for object creation, internal flow, and entry/exit of key functions
All provider calls go throughBaseCompletionProviderwhich applies retry + rate limiting automatically. Never implement retry logic in driver subclasses or calling code.
RetryConfig and RateLimiterConfig are set per-provider inProviderConfig; retryable errors (is_retryable=True) includeRateLimitError,ProviderTimeoutError,ProviderConnectionError,ProviderInternalError
Non-retryable errors raise immediately without retry;RetryExhaustedErrorsignals that all retries failed — engine layer catches this to trigger fallback chains
Rate limiter respectsRateLimitError.retry_afterfrom providers — automatically pauses future requests
Library reference: auto-generated from docstrings via mkdocstrings + Griffe (AST-based, no imports) indocs/api/
Files:
src/synthorg/memory/consolidation/models.pysrc/synthorg/memory/consolidation/extractive.pysrc/synthorg/memory/consolidation/service.pysrc/synthorg/memory/consolidation/abstractive.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Use pytest markers:@pytest.mark.unit,@pytest.mark.integration,@pytest.mark.e2e,@pytest.mark.slowfor test categorization
Useasyncio_mode = 'auto'in pytest — no manual@pytest.mark.asyncioneeded
Per-test timeout: 30 seconds
Prefer@pytest.mark.parametrizefor testing similar cases
Usetest-provider,test-small-001, etc. in tests instead of real vendor names
Use Python Hypothesis for property-based testing with@given+@settingsdecorators
NEVER skip, dismiss, or ignore flaky tests — always fix them fully and fundamentally. For timing-sensitive tests, mocktime.monotonic()andasyncio.sleep()to make them deterministic
Files:
tests/unit/memory/consolidation/test_models.pytests/unit/memory/consolidation/test_extractive.pytests/unit/memory/consolidation/test_abstractive.py
🧠 Learnings (7)
📚 Learning: 2026-03-17T17:24:41.990Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T17:24:41.990Z
Learning: Applies to **/*.py : Use `except A, B:` syntax (no parentheses) for exception handling in Python 3.14, as enforced by ruff PEP 758
Applied to files:
src/synthorg/memory/consolidation/abstractive.py
📚 Learning: 2026-03-17T17:24:41.990Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T17:24:41.990Z
Learning: Applies to **/*.py : Prefer `asyncio.TaskGroup` for fan-out/fan-in parallel operations in new code (multiple tool invocations, parallel agent calls). Prefer structured concurrency over bare `create_task`.
Applied to files:
src/synthorg/memory/consolidation/abstractive.py
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to **/*.py : Async concurrency: prefer asyncio.TaskGroup for fan-out/fan-in parallel operations in new code (e.g. multiple tool invocations, parallel agent calls). Prefer structured concurrency over bare create_task.
Applied to files:
src/synthorg/memory/consolidation/abstractive.py
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to src/synthorg/**/*.py : All error paths must log at WARNING or ERROR with context before raising.
Applied to files:
src/synthorg/memory/consolidation/abstractive.py
📚 Learning: 2026-03-17T17:24:41.990Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T17:24:41.990Z
Learning: Applies to src/synthorg/**/*.py : All error paths must log at WARNING or ERROR with context before raising
Applied to files:
src/synthorg/memory/consolidation/abstractive.py
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to src/synthorg/providers/**/*.py : Retryable errors (is_retryable=True): RateLimitError, ProviderTimeoutError, ProviderConnectionError, ProviderInternalError. Non-retryable errors raise immediately without retry. RetryExhaustedError signals that all retries failed — the engine layer catches this to trigger fallback chains.
Applied to files:
src/synthorg/memory/consolidation/abstractive.py
📚 Learning: 2026-03-17T17:24:41.990Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T17:24:41.990Z
Learning: Applies to src/synthorg/**/*.py : RetryConfig and RateLimiterConfig are set per-provider in `ProviderConfig`; retryable errors (`is_retryable=True`) include `RateLimitError`, `ProviderTimeoutError`, `ProviderConnectionError`, `ProviderInternalError`
Applied to files:
src/synthorg/memory/consolidation/abstractive.py
🧬 Code graph analysis (4)
tests/unit/memory/consolidation/test_models.py (2)
src/synthorg/memory/consolidation/models.py (5)
ArchivalIndexEntry(55-77)ArchivalMode(24-34)ArchivalModeAssignment(37-52)ConsolidationResult(80-160)ArchivalEntry(163-204)src/synthorg/core/enums.py (1)
MemoryCategory(101-108)
tests/unit/memory/consolidation/test_extractive.py (1)
src/synthorg/memory/consolidation/extractive.py (2)
ExtractivePreserver(95-171)extract(126-171)
tests/unit/memory/consolidation/test_abstractive.py (1)
src/synthorg/memory/consolidation/abstractive.py (3)
AbstractiveSummarizer(40-172)summarize(75-140)summarize_batch(142-172)
src/synthorg/memory/consolidation/service.py (1)
src/synthorg/memory/consolidation/models.py (4)
ArchivalIndexEntry(55-77)ArchivalMode(24-34)ArchivalModeAssignment(37-52)ConsolidationResult(80-160)
| async def test_batch_continues_on_individual_failure(self) -> None: | ||
| provider = AsyncMock() | ||
| provider.complete = AsyncMock( | ||
| side_effect=[ | ||
| CompletionResponse( | ||
| content="Good summary", | ||
| finish_reason=FinishReason.STOP, | ||
| usage=TokenUsage( | ||
| input_tokens=10, | ||
| output_tokens=5, | ||
| cost_usd=0.001, | ||
| ), | ||
| model="test-small-001", | ||
| ), | ||
| RuntimeError("LLM error"), | ||
| ], | ||
| ) | ||
| summarizer = AbstractiveSummarizer( | ||
| provider=provider, | ||
| model="test-small-001", | ||
| ) | ||
| entries = ( | ||
| _make_entry("m1", "First"), | ||
| _make_entry("m2", "Second"), | ||
| ) | ||
| result = await summarizer.summarize_batch(entries) | ||
| assert len(result) == 2 | ||
| assert result[0][1] == "Good summary" | ||
| # Second entry falls back to truncation; "Second" is shorter | ||
| # than _TRUNCATE_LENGTH so returned verbatim. | ||
| assert result[1][1] == "Second" |
There was a problem hiding this comment.
Make the concurrent failure case deterministic.
summarize_batch() fans out with asyncio.TaskGroup, so the two provider.complete() awaits race. A positional side_effect=[success, RuntimeError(...)] makes this test depend on scheduler order: whichever task reaches the mock first gets the success response. Key the mock behavior off the user content instead of call index.
Deterministic mock setup
- provider.complete = AsyncMock(
- side_effect=[
- CompletionResponse(
- content="Good summary",
- finish_reason=FinishReason.STOP,
- usage=TokenUsage(
- input_tokens=10,
- output_tokens=5,
- cost_usd=0.001,
- ),
- model="test-small-001",
- ),
- RuntimeError("LLM error"),
- ],
- )
+ async def _complete(messages, model, *, config):
+ user_content = messages[1].content
+ if user_content == "First":
+ return CompletionResponse(
+ content="Good summary",
+ finish_reason=FinishReason.STOP,
+ usage=TokenUsage(
+ input_tokens=10,
+ output_tokens=5,
+ cost_usd=0.001,
+ ),
+ model="test-small-001",
+ )
+ if user_content == "Second":
+ raise RuntimeError("LLM error")
+ raise AssertionError(f"Unexpected content: {user_content}")
+
+ provider.complete = AsyncMock(side_effect=_complete)As per coding guidelines, "NEVER skip, dismiss, or ignore flaky tests — always fix them fully and fundamentally. For timing-sensitive tests, mock time.monotonic() and asyncio.sleep() to make them deterministic."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/memory/consolidation/test_abstractive.py` around lines 151 - 181,
The test test_batch_continues_on_individual_failure is flaky because
provider.complete uses a positional side_effect list and summarize_batch (which
uses asyncio.TaskGroup) races the calls; change provider.complete to a
deterministic side_effect callable that inspects the incoming request (e.g., the
user content or prompt from the call) and returns a successful
CompletionResponse for the entry with content "First" and raises RuntimeError
for the entry with content "Second" so the behavior no longer depends on
scheduling; update the mock setup in the test to use that callable (targeting
provider.complete) and, if needed for other timing sensitivity, mock
time.monotonic/asyncio.sleep to fixed values to ensure full determinism.
Add duplicate and subset-of-removed_ids validation for mode_assignments.original_id values, preventing silent overwrites when building mode_map in the service layer.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/synthorg/memory/consolidation/models.py (1)
75-77:⚠️ Potential issue | 🟠 MajorMake
ArchivalIndexEntry.modenullable for backward-compatible restore indexing.Line 75 requires
ArchivalIndexEntry.mode, but Lines 197-200 explicitly allowArchivalEntry.archival_mode=Nonefor legacy entries. This can block indexing/restoring archival records that legitimately have no mode.💡 Proposed fix
class ArchivalIndexEntry(BaseModel): @@ - mode: ArchivalMode = Field( - description="Archival mode used for this entry", - ) + mode: ArchivalMode | None = Field( + default=None, + description="Archival mode used for this entry (None for legacy entries)", + )Also applies to: 197-200
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/synthorg/memory/consolidation/models.py` around lines 75 - 77, Make ArchivalIndexEntry.mode nullable to allow legacy entries with no mode: change the type annotation from ArchivalMode to Optional[ArchivalMode] on the ArchivalIndexEntry model and set the Field default to None (e.g., Field(default=None, description="...")). This mirrors ArchivalEntry.archival_mode's allowance for None and prevents indexing/restore failures for legacy records; update any related validation or typing references that assume a non-None mode.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/synthorg/memory/consolidation/models.py`:
- Around line 118-161: The validator _validate_archival_consistency raises
ValueError on multiple failure paths without logging; import get_logger from
synthorg.observability and create logger = get_logger(__name__), then before
each raise in _validate_archival_consistency log a WARNING or ERROR with
contextual details (e.g., include removed_ids,
archival_count/consolidated_count, lengths, problematic ids or duplicates, and
the constructed msg) so every error path emits an observability log entry
immediately prior to raising the ValueError.
---
Duplicate comments:
In `@src/synthorg/memory/consolidation/models.py`:
- Around line 75-77: Make ArchivalIndexEntry.mode nullable to allow legacy
entries with no mode: change the type annotation from ArchivalMode to
Optional[ArchivalMode] on the ArchivalIndexEntry model and set the Field default
to None (e.g., Field(default=None, description="...")). This mirrors
ArchivalEntry.archival_mode's allowance for None and prevents indexing/restore
failures for legacy records; update any related validation or typing references
that assume a non-None mode.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b09ecf5b-a3c6-4c11-a1ce-3544551035ca
📒 Files selected for processing (2)
src/synthorg/memory/consolidation/models.pytests/unit/memory/consolidation/test_models.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Test (Python 3.14)
- GitHub Check: Build Backend
- GitHub Check: Build Web
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Nofrom __future__ import annotations— Python 3.14 has PEP 649 with native lazy annotations
Useexcept A, B:syntax (no parentheses) for exception handling in Python 3.14, as enforced by ruff PEP 758
All public functions and methods require type hints; mypy strict mode enforced
Docstrings: Google style, required on all public classes and functions, enforced by ruff D rules
Create new objects instead of mutating existing ones; never mutate. For non-Pydantic internal collections (registries,BaseTool), usecopy.deepcopy()at construction +MappingProxyTypewrapping for read-only enforcement.
Fordict/listfields in frozen Pydantic models, rely onfrozen=Truefor field reassignment prevention andcopy.deepcopy()at system boundaries (tool execution, LLM provider serialization, inter-agent delegation, serializing for persistence)
Use frozen Pydantic models for config/identity; use separate mutable-via-copy models (usingmodel_copy(update=...)) for runtime state that evolves. Never mix static config fields with mutable runtime fields in one model.
Use Pydantic v2 with adopted conventions:@computed_fieldfor derived values instead of storing + validating redundant fields; useNotBlankStrfromcore.typesfor all identifier/name fields (including optional and tuple variants) instead of manual whitespace validators
Preferasyncio.TaskGroupfor fan-out/fan-in parallel operations in new code (multiple tool invocations, parallel agent calls). Prefer structured concurrency over barecreate_task.
Line length: 88 characters (enforced by ruff)
Functions must be < 50 lines; files must be < 800 lines
Handle errors explicitly, never silently swallow them
Validate at system boundaries (user input, external APIs, config files)
Files:
src/synthorg/memory/consolidation/models.pytests/unit/memory/consolidation/test_models.py
src/synthorg/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/synthorg/**/*.py: Every module with business logic MUST havefrom synthorg.observability import get_loggerthenlogger = get_logger(__name__); never useimport logging/logging.getLogger()/print()in application code
Logger variable name must always belogger(not_logger, notlog)
Event names must always use constants from domain-specific modules undersynthorg.observability.events(e.g.,API_REQUEST_STARTEDfromevents.api). Import directly:from synthorg.observability.events.<domain> import EVENT_CONSTANT
Always use structured logging:logger.info(EVENT, key=value)— neverlogger.info('msg %s', val)
All error paths must log at WARNING or ERROR with context before raising
All state transitions must log at INFO level
DEBUG logging for object creation, internal flow, and entry/exit of key functions
All provider calls go throughBaseCompletionProviderwhich applies retry + rate limiting automatically. Never implement retry logic in driver subclasses or calling code.
RetryConfig and RateLimiterConfig are set per-provider inProviderConfig; retryable errors (is_retryable=True) includeRateLimitError,ProviderTimeoutError,ProviderConnectionError,ProviderInternalError
Non-retryable errors raise immediately without retry;RetryExhaustedErrorsignals that all retries failed — engine layer catches this to trigger fallback chains
Rate limiter respectsRateLimitError.retry_afterfrom providers — automatically pauses future requests
Library reference: auto-generated from docstrings via mkdocstrings + Griffe (AST-based, no imports) indocs/api/
Files:
src/synthorg/memory/consolidation/models.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Use pytest markers:@pytest.mark.unit,@pytest.mark.integration,@pytest.mark.e2e,@pytest.mark.slowfor test categorization
Useasyncio_mode = 'auto'in pytest — no manual@pytest.mark.asyncioneeded
Per-test timeout: 30 seconds
Prefer@pytest.mark.parametrizefor testing similar cases
Usetest-provider,test-small-001, etc. in tests instead of real vendor names
Use Python Hypothesis for property-based testing with@given+@settingsdecorators
NEVER skip, dismiss, or ignore flaky tests — always fix them fully and fundamentally. For timing-sensitive tests, mocktime.monotonic()andasyncio.sleep()to make them deterministic
Files:
tests/unit/memory/consolidation/test_models.py
🧬 Code graph analysis (1)
tests/unit/memory/consolidation/test_models.py (1)
src/synthorg/memory/consolidation/models.py (5)
ArchivalIndexEntry(55-77)ArchivalMode(24-34)ArchivalModeAssignment(37-52)ConsolidationResult(80-167)ArchivalEntry(170-211)
🔇 Additional comments (3)
tests/unit/memory/consolidation/test_models.py (3)
140-208: Strong coverage for new archival-mode primitives.These tests validate enum values, immutability, and basic ID validation for assignment/index models in a clean, focused way.
238-343: Good regression coverage for newConsolidationResultID invariants.The new cases for duplicate
removed_ids, duplicate assignment IDs, and unknown assignment/index IDs directly protect the validator logic added in this PR.
365-387: Backward-compatibility test forArchivalEntry.archival_modeis well-targeted.Verifying both default
Noneand explicit mode assignment is exactly the right guardrail for dual-mode rollout.
| @model_validator(mode="after") | ||
| def _validate_archival_consistency(self) -> Self: | ||
| """Ensure archival fields are internally consistent.""" | ||
| if len(self.removed_ids) != len(set(self.removed_ids)): | ||
| msg = "removed_ids contains duplicates" | ||
| raise ValueError(msg) | ||
| if self.archived_count > self.consolidated_count: | ||
| msg = ( | ||
| f"archived_count ({self.archived_count}) must not exceed " | ||
| f"consolidated_count ({self.consolidated_count})" | ||
| ) | ||
| raise ValueError(msg) | ||
| if len(self.archival_index) > self.archived_count: | ||
| msg = ( | ||
| f"archival_index length ({len(self.archival_index)}) " | ||
| f"must not exceed archived_count ({self.archived_count})" | ||
| ) | ||
| raise ValueError(msg) | ||
| if len(self.mode_assignments) > len(self.removed_ids): | ||
| msg = ( | ||
| f"mode_assignments length ({len(self.mode_assignments)}) " | ||
| f"must not exceed removed_ids length " | ||
| f"({len(self.removed_ids)})" | ||
| ) | ||
| raise ValueError(msg) | ||
| removed_set = set(self.removed_ids) | ||
| assignment_ids = [a.original_id for a in self.mode_assignments] | ||
| if len(assignment_ids) != len(set(assignment_ids)): | ||
| msg = "mode_assignments contains duplicate original_ids" | ||
| raise ValueError(msg) | ||
| if any(aid not in removed_set for aid in assignment_ids): | ||
| msg = "mode_assignments contains original_ids not in removed_ids" | ||
| raise ValueError(msg) | ||
| for idx_entry in self.archival_index: | ||
| if idx_entry.original_id not in removed_set: | ||
| msg = ( | ||
| f"archival_index entry '{idx_entry.original_id}' not in removed_ids" | ||
| ) | ||
| raise ValueError(msg) | ||
| index_ids = [e.original_id for e in self.archival_index] | ||
| if len(index_ids) != len(set(index_ids)): | ||
| msg = "archival_index contains duplicate original_ids" | ||
| raise ValueError(msg) | ||
| return self |
There was a problem hiding this comment.
Log validator failure paths before raising.
_validate_archival_consistency raises multiple ValueErrors without a WARNING/ERROR log, which breaks observability requirements for business-logic modules under src/synthorg/.
As per coding guidelines, "Every module with business logic MUST have from synthorg.observability import get_logger then logger = get_logger(__name__)" and "All error paths must log at WARNING or ERROR with context before raising".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/synthorg/memory/consolidation/models.py` around lines 118 - 161, The
validator _validate_archival_consistency raises ValueError on multiple failure
paths without logging; import get_logger from synthorg.observability and create
logger = get_logger(__name__), then before each raise in
_validate_archival_consistency log a WARNING or ERROR with contextual details
(e.g., include removed_ids, archival_count/consolidated_count, lengths,
problematic ids or duplicates, and the constructed msg) so every error path
emits an observability log entry immediately prior to raising the ValueError.
🤖 I have created a release *beep* *boop* --- ## [0.3.1](v0.3.0...v0.3.1) (2026-03-17) ### Features * **api:** RFC 9457 Phase 2 — ProblemDetail and content negotiation ([#496](#496)) ([30f7c49](30f7c49)) * **cli:** verify container image signatures and SLSA provenance on pull ([#492](#492)) ([bef272d](bef272d)), closes [#491](#491) * **engine:** implement context budget management in execution loops ([#520](#520)) ([181eb8a](181eb8a)), closes [#416](#416) * implement settings persistence layer (DB-backed config) ([#495](#495)) ([4bd99f7](4bd99f7)), closes [#450](#450) * **memory:** implement dual-mode archival in memory consolidation ([#524](#524)) ([4603c9e](4603c9e)), closes [#418](#418) * migrate config consumers to read through SettingsService ([#510](#510)) ([32f553d](32f553d)) * **settings:** implement settings change subscriptions for service hot-reload ([#526](#526)) ([53f908e](53f908e)), closes [#503](#503) * **settings:** register API config in SettingsService with 2-phase init ([#518](#518)) ([29f7481](29f7481)) * **tools:** add SSRF prevention for git clone URLs ([#505](#505)) ([492dd0d](492dd0d)) * **tools:** wire RootConfig.git_clone to GitCloneTool instantiation ([#519](#519)) ([b7d8172](b7d8172)) ### Bug Fixes * **api:** replace JWT query parameter with one-time ticket for WebSocket auth ([#493](#493)) ([22a25f6](22a25f6)), closes [#343](#343) ### Documentation * add uv cache lock contention handling to worktree skill ([#500](#500)) ([bd85a8d](bd85a8d)) * document RFC 9457 dual response formats in OpenAPI schema ([#506](#506)) ([8dd2524](8dd2524)) ### Maintenance * upgrade jsdom from 28 to 29 ([#499](#499)) ([1ea2249](1ea2249)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Summary
DensityClassifier(5 weighted signals) classifies content as sparse or dense, then applies LLM abstractive summarization (AbstractiveSummarizer) or extractive key-fact preservation (ExtractivePreserver) accordinglyDualModeConsolidationStrategyimplements existingConsolidationStrategyprotocol with per-entry classification, group-level majority vote, and mode-aware processingConsolidationResultwithmode_assignmentsandarchival_indexfor deterministic index-based restore of self-archived content (no semantic search needed)ArchivalEntrygainsarchival_modefield;ArchivalConfiggains nestedDualModeConfigResearch basis
New files
consolidation/density.pyContentDensityenum +DensityClassifier(heuristic, no LLM)consolidation/extractive.pyExtractivePreserver— verbatim key facts + start/mid/end anchorsconsolidation/abstractive.pyAbstractiveSummarizer— LLM summary with truncation fallbackconsolidation/dual_mode_strategy.pyDualModeConsolidationStrategy— orchestrates classification + mode selectionTest plan
Closes #418
🤖 Generated with Claude Code