feat: support per-agent memory retention overrides (#209)#951
feat: support per-agent memory retention overrides (#209)#951
Conversation
Add AgentRetentionRule model and retention_overrides field to MemoryConfig, enabling individual agents to override company-level retention policies per category. Extend RetentionEnforcer with _resolve_categories() for merging agent overrides with company defaults using a 5-level resolution order. Pass through override params in MemoryConsolidationService. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Fix critical resolution priority bug where _build_categories_to_check baked company default_retention_days into base, making agent default unreachable. Pass only explicit company rules to _resolve_categories. Also: add WARNING log in uniqueness validator, extract _resolve_for_agent helper (50-line limit), rename log kwarg to resolved_category_count, update design spec table, add parity/log/cutoff tests. Pre-reviewed by 6 agents, 8 findings addressed Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📜 Recent 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). (6)
🧰 Additional context used📓 Path-based instructions (2)**/*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
tests/**/*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (16)📚 Learning: 2026-03-20T21:44:04.528ZApplied to files:
📚 Learning: 2026-03-17T22:08:13.456ZApplied to files:
📚 Learning: 2026-03-15T18:28:13.207ZApplied to files:
📚 Learning: 2026-03-31T14:17:24.182ZApplied to files:
📚 Learning: 2026-03-31T14:40:41.736ZApplied to files:
📚 Learning: 2026-03-31T14:17:24.182ZApplied to files:
📚 Learning: 2026-03-16T06:24:56.341ZApplied to files:
📚 Learning: 2026-03-31T14:40:41.736ZApplied to files:
📚 Learning: 2026-03-20T21:44:04.528ZApplied to files:
📚 Learning: 2026-03-15T18:38:44.202ZApplied to files:
📚 Learning: 2026-03-18T21:23:23.586ZApplied to files:
📚 Learning: 2026-03-19T11:33:01.580ZApplied to files:
📚 Learning: 2026-03-20T11:18:48.128ZApplied to files:
📚 Learning: 2026-03-15T18:28:13.207ZApplied to files:
📚 Learning: 2026-03-15T19:14:27.144ZApplied to files:
📚 Learning: 2026-03-31T14:40:41.736ZApplied to files:
🔇 Additional comments (4)
WalkthroughAdds per-agent memory retention overrides: a frozen Pydantic model 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Snapshot WarningsEnsure that dependencies are being submitted on PR branches. Re-running this action after a short time may resolve the issue. See the documentation for more information and troubleshooting advice. Scanned FilesNone |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/design/memory.md`:
- Around line 357-363: The admonition currently uses an indented code block
style that triggers MD046; update the "Per-Agent Retention Overrides" admonition
so it uses lint-friendly formatting by removing any leading 4-space indentation
and replacing the indented snippet with either inline `code` or a fenced code
block (```...```) for the references to MemoryConfig.retention_overrides and
MemoryConfig.retention_days, and rewrite the resolution order line as plain text
(or a fenced code line) so it reads: agent per-category rule > company
per-category rule > agent global default > company global default > keep
forever, ensuring no indented code blocks remain.
In `@src/synthorg/memory/consolidation/retention.py`:
- Around line 146-162: The code currently treats an empty dict as an applied
override because has_overrides uses "agent_category_overrides is not None";
change the check to detect actual content by using a truthiness check for the
dict (e.g., bool(agent_category_overrides)) so has_overrides becomes True only
when agent_category_overrides has entries or agent_default_retention_days is
provided, then leave the call to _resolve_categories and the
RETENTION_AGENT_OVERRIDE_APPLIED log intact (still reference
agent_category_overrides, agent_default_retention_days, _resolve_categories,
has_overrides and RETENTION_AGENT_OVERRIDE_APPLIED) so you only suppress the log
when there is no real override.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ad8140a2-6702-49cd-8597-610420d74b5c
📒 Files selected for processing (10)
docs/design/memory.mdsrc/synthorg/core/__init__.pysrc/synthorg/core/agent.pysrc/synthorg/memory/consolidation/config.pysrc/synthorg/memory/consolidation/retention.pysrc/synthorg/memory/consolidation/service.pysrc/synthorg/observability/events/consolidation.pytests/unit/core/test_agent.pytests/unit/memory/consolidation/test_retention.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). (6)
- GitHub Check: Test (Python 3.14)
- GitHub Check: Build Web
- GitHub Check: Build Backend
- GitHub Check: Build Sandbox
- GitHub Check: Dependency Review
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Python version 3.14+ with PEP 649 native lazy annotations
Do NOT usefrom __future__ import annotations-- Python 3.14 has PEP 649
PEP 758 except syntax: useexcept A, B:(no parentheses) -- ruff enforces this on Python 3.14
Line length: 88 characters (ruff enforced)
Files:
src/synthorg/observability/events/consolidation.pysrc/synthorg/core/__init__.pysrc/synthorg/memory/consolidation/config.pytests/unit/memory/consolidation/test_service.pytests/unit/core/test_agent.pysrc/synthorg/core/agent.pysrc/synthorg/memory/consolidation/service.pytests/unit/memory/consolidation/test_retention.pysrc/synthorg/memory/consolidation/retention.py
src/synthorg/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/synthorg/**/*.py: Type hints: all public functions, mypy strict mode
Docstrings: Google style, required on public classes/functions (enforced by ruff D rules)
Use Pydantic v2 (BaseModel,model_validator,computed_field,ConfigDict)
Useallow_inf_nan=Falsein allConfigDictdeclarations to rejectNaN/Infin numeric fields at validation time
Use@computed_fieldfor derived values instead of storing + validating redundant fields (e.g.TokenUsage.total_tokens)
UseNotBlankStr(fromcore.types) for all identifier/name fields -- including optional (NotBlankStr | None) and tuple (tuple[NotBlankStr, ...]) variants -- instead of manual whitespace validators
Async concurrency: preferasyncio.TaskGroupfor fan-out/fan-in parallel operations in new code (e.g. multiple tool invocations, parallel agent calls); prefer structured concurrency over barecreate_task
Functions: < 50 lines, files < 800 lines
Errors: handle explicitly, never silently swallow
Every module with business logic MUST have:from synthorg.observability import get_loggerthenlogger = get_logger(__name__)
Never useimport logging/logging.getLogger()/print()in application code (exception:observability/setup.pyandobservability/sinks.pymay use stdlibloggingandprint(..., file=sys.stderr)for bootstrap and handler-cleanup code)
Variable name for logger: alwayslogger(not_logger, notlog)
Event names: always use constants from the domain-specific module undersynthorg.observability.events(e.g.,API_REQUEST_STARTEDfromevents.api,TOOL_INVOKE_STARTfromevents.tool); import directly:from synthorg.observability.events.<domain> import EVENT_CONSTANT
Structured kwargs for logging: alwayslogger.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
DEBUG logging for object creation, internal flow, entry/exit of key...
Files:
src/synthorg/observability/events/consolidation.pysrc/synthorg/core/__init__.pysrc/synthorg/memory/consolidation/config.pysrc/synthorg/core/agent.pysrc/synthorg/memory/consolidation/service.pysrc/synthorg/memory/consolidation/retention.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Test markers:@pytest.mark.unit,@pytest.mark.integration,@pytest.mark.e2e,@pytest.mark.slow
Async tests:asyncio_mode = "auto"-- no manual@pytest.mark.asyncioneeded
Test timeout: 30 seconds per test (global inpyproject.toml-- do not add per-filepytest.mark.timeout(30)markers; non-default overrides liketimeout(60)are allowed)
Parametrize: Prefer@pytest.mark.parametrizefor testing similar cases
Tests must usetest-provider,test-small-001, etc. instead of real vendor names
Property-based testing in Python: use Hypothesis (@given+@settings); run withHYPOTHESIS_PROFILE=dev uv run python -m pytest tests/ -m unit -n auto -k properties(dev profile: 1000 examples)
Hypothesis profiles:ci(50 examples, default) anddev(1000 examples), controlled viaHYPOTHESIS_PROFILEenv var
For timing-sensitive tests, mocktime.monotonic()andasyncio.sleep()to make them deterministic instead of widening timing margins
For tasks that must block indefinitely until cancelled, useasyncio.Event().wait()instead ofasyncio.sleep(large_number)-- it is cancellation-safe and carries no timing assumptions
Files:
tests/unit/memory/consolidation/test_service.pytests/unit/core/test_agent.pytests/unit/memory/consolidation/test_retention.py
🧠 Learnings (20)
📚 Learning: 2026-03-19T07:12:14.508Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T07:12:14.508Z
Learning: Applies to src/synthorg/observability/**/*.py : Observability package (observability/): structured logging, correlation tracking, log sinks; event constants organized by domain under observability/events/ (e.g., events.api, events.tool, events.git, events.context_budget, events.backup)
Applied to files:
src/synthorg/observability/events/consolidation.pysrc/synthorg/memory/consolidation/retention.py
📚 Learning: 2026-03-31T14:17:24.182Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-31T14:17:24.182Z
Learning: Applies to src/synthorg/**/*.py : Use event name constants from `synthorg.observability.events.<domain>` modules (e.g., `API_REQUEST_STARTED` from `events.api`, `TOOL_INVOKE_START` from `events.tool`); import directly and use in structured logging
Applied to files:
src/synthorg/observability/events/consolidation.pysrc/synthorg/memory/consolidation/retention.py
📚 Learning: 2026-03-16T06:24:56.341Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T06:24:56.341Z
Learning: Applies to src/synthorg/**/*.py : Always use event name constants from the domain-specific module under `synthorg.observability.events` in logging calls
Applied to files:
src/synthorg/observability/events/consolidation.pysrc/synthorg/memory/consolidation/retention.py
📚 Learning: 2026-03-19T11:33:01.580Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T11:33:01.580Z
Learning: Applies to src/synthorg/**/*.py : Use event constants from `synthorg.observability.events.<domain>` (e.g., `API_REQUEST_STARTED` from `events.api`); import directly and log with structured kwargs: `logger.info(EVENT, key=value)`, never interpolated strings
Applied to files:
src/synthorg/observability/events/consolidation.pysrc/synthorg/memory/consolidation/retention.py
📚 Learning: 2026-03-16T06:24:56.341Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T06:24:56.341Z
Learning: Applies to src/synthorg/core/**/*.py : Core module must contain shared domain models, base classes, resilience config (RetryConfig, RateLimiterConfig)
Applied to files:
src/synthorg/core/__init__.py
📚 Learning: 2026-03-19T07:12:14.508Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T07:12:14.508Z
Learning: Applies to src/synthorg/security/**/*.py : Security package (security/): SecOps agent, rule engine (soft-allow/hard-deny, fail-closed), audit log, output scanner, output scan response policies (redact/withhold/log-only/autonomy-tiered), risk classifier, risk tier classifier, action type registry, ToolInvoker security integration, progressive trust (4 strategies), autonomy levels (presets, resolver, change strategy), timeout policies (park/resume)
Applied to files:
src/synthorg/core/__init__.pytests/unit/core/test_agent.py
📚 Learning: 2026-03-17T06:30:14.180Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T06:30:14.180Z
Learning: Applies to src/synthorg/security/**/*.py : Security module includes SecOps agent, rule engine (soft-allow/hard-deny), audit log, output scanner, risk classifier, autonomy levels (4 strategies), timeout policies.
Applied to files:
src/synthorg/core/__init__.pytests/unit/core/test_agent.py
📚 Learning: 2026-03-19T07:12:14.508Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T07:12:14.508Z
Learning: Applies to src/synthorg/**/*.py : Package structure: src/synthorg/ organized as: api/ (REST+WebSocket, Litestar), auth/ (auth subpackage), backup/ (scheduled/manual backups), budget/ (cost tracking, CFO), cli/ (superseded by Go CLI), communication/ (message bus, meetings), config/ (YAML loading), core/ (domain models, resilience config), engine/ (orchestration, task state, coordination, approval gates, stagnation detection, context budget, compaction), hr/ (hiring, performance, promotion), memory/ (pluggable backend, Mem0, retrieval, consolidation), persistence/ (operational data, SQLite, settings), observability/ (logging, correlation, sinks), providers/ (LLM abstraction, LiteLLM, auth types, presets, runtime CRUD), settings/ (runtime-editable, typed definitions, encryption, config bridge), security/ (SecOps, rule engine, output scanning, progressive trust, autonomy levels), templates/ (company templates, personalities), tools/ (registry, built-in tools, git, sandbox, code_runner, MCP...
Applied to files:
src/synthorg/core/__init__.py
📚 Learning: 2026-03-19T07:13:44.964Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T07:13:44.964Z
Learning: Applies to src/synthorg/hr/**/*.py : HR package (hr/): hiring, firing, onboarding, offboarding, agent registry, performance tracking (task metrics, collaboration scoring, LLM calibration, collaboration overrides, trend detection), promotion/demotion (criteria evaluation, approval strategies, model mapping)
Applied to files:
src/synthorg/core/__init__.py
📚 Learning: 2026-03-16T06:24:56.341Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T06:24:56.341Z
Learning: Applies to src/synthorg/memory/**/*.py : Use MemoryBackend protocol with pluggable backends (Mem0 adapter available at backends/mem0/) for persistent agent memory
Applied to files:
src/synthorg/core/__init__.pytests/unit/core/test_agent.pysrc/synthorg/core/agent.pysrc/synthorg/memory/consolidation/retention.py
📚 Learning: 2026-03-20T21:44:04.528Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-20T21:44:04.528Z
Learning: Applies to src/synthorg/**/*.py : Use frozen Pydantic models for config/identity; use separate mutable-via-copy models (via `model_copy(update=...)`) for runtime state that evolves
Applied to files:
src/synthorg/core/__init__.pytests/unit/core/test_agent.pysrc/synthorg/core/agent.py
📚 Learning: 2026-03-15T18:38:44.202Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T18:38:44.202Z
Learning: Applies to src/synthorg/**/*.py : Use frozen Pydantic models for config/identity; separate mutable-via-copy models (using `model_copy(update=...)`) for runtime state
Applied to files:
src/synthorg/core/__init__.pytests/unit/core/test_agent.py
📚 Learning: 2026-03-31T14:40:41.736Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-31T14:40:41.736Z
Learning: Required dependencies: `mem0ai` (Mem0 memory backend), `cryptography` (Fernet encryption), `faker` (multi-locale agent name generation)
Applied to files:
src/synthorg/core/__init__.pytests/unit/core/test_agent.py
📚 Learning: 2026-03-15T19:14:27.144Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T19:14:27.144Z
Learning: Applies to src/synthorg/**/*.py : Use frozen Pydantic models for config/identity; use separate mutable-via-copy models (using model_copy(update=...)) for runtime state that evolves. Never mix static config fields with mutable runtime fields in one model.
Applied to files:
src/synthorg/core/__init__.pytests/unit/core/test_agent.pysrc/synthorg/core/agent.py
📚 Learning: 2026-03-19T07:12:14.508Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T07:12:14.508Z
Learning: Applies to src/synthorg/memory/**/*.py : Memory package (memory/): pluggable MemoryBackend protocol, backends/ (Mem0 adapter), retrieval pipeline (ranking, RRF fusion, injection, formatting, non-inferable filtering), shared org memory (org/), consolidation/archival (density-aware: DensityClassifier, AbstractiveSummarizer, ExtractivePreserver, DualModeConsolidationStrategy)
Applied to files:
docs/design/memory.mdsrc/synthorg/core/agent.pysrc/synthorg/memory/consolidation/service.pysrc/synthorg/memory/consolidation/retention.py
📚 Learning: 2026-03-31T14:40:41.736Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-31T14:40:41.736Z
Learning: Applies to tests/**/*.py : For timing-sensitive tests, mock `time.monotonic()` and `asyncio.sleep()` to make them deterministic instead of widening timing margins
Applied to files:
tests/unit/memory/consolidation/test_retention.py
📚 Learning: 2026-03-17T06:30:14.180Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T06:30:14.180Z
Learning: Applies to src/synthorg/observability/**/*.py : Observability includes structured logging via `get_logger(__name__)`, correlation tracking, and log sinks.
Applied to files:
src/synthorg/memory/consolidation/retention.py
📚 Learning: 2026-03-15T19:14:27.144Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T19:14:27.144Z
Learning: Applies to src/synthorg/**/*.py : Use event name constants from synthorg.observability.events domain-specific modules (e.g., PROVIDER_CALL_START from events.provider). Import directly: from synthorg.observability.events.<domain> import EVENT_CONSTANT.
Applied to files:
src/synthorg/memory/consolidation/retention.py
📚 Learning: 2026-03-18T21:23:23.586Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-18T21:23:23.586Z
Learning: Applies to src/synthorg/**/*.py : Event names: always use constants from the domain-specific module under synthorg.observability.events (e.g., API_REQUEST_STARTED from events.api, TOOL_INVOKE_START from events.tool). Import directly from synthorg.observability.events.<domain>.
Applied to files:
src/synthorg/memory/consolidation/retention.py
📚 Learning: 2026-03-15T18:28:13.207Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T18:28:13.207Z
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/memory/consolidation/retention.py
🪛 markdownlint-cli2 (0.22.0)
docs/design/memory.md
[warning] 359-359: Code block style
Expected: fenced; Actual: indented
(MD046, code-block-style)
🔇 Additional comments (12)
src/synthorg/observability/events/consolidation.py (1)
32-34: Good addition of a domain-scoped event constant.
RETENTION_AGENT_OVERRIDE_APPLIEDis consistent with the existing consolidation event taxonomy.src/synthorg/memory/consolidation/config.py (1)
17-29: Doc update is aligned with runtime precedence semantics.The precedence order is now explicit and unambiguous for consumers of
RetentionConfig.src/synthorg/core/agent.py (1)
183-277: Strong validation surface for per-agent retention overrides.The new model +
MemoryConfigvalidators enforce the key invariants cleanly (NONE-type guard and duplicate-category rejection) with structured validation logs.src/synthorg/core/__init__.py (1)
5-5: Public API export is correctly wired.Re-exporting
AgentRetentionRulehere keeps the core package surface consistent.Also applies to: 84-84
tests/unit/core/test_agent.py (1)
412-527: Great coverage for override invariants.These tests exercise the critical acceptance/rejection paths and immutability for the new retention override model.
tests/unit/memory/consolidation/test_service.py (1)
470-532: Nice pass-through coverage for service-level override plumbing.These tests protect both the new override flow and the no-override backward-compatible behavior.
src/synthorg/memory/consolidation/retention.py (1)
77-128: Resolution chain implementation looks correct and readable.The 5-level precedence is encoded clearly and matches the intended behavior.
src/synthorg/memory/consolidation/service.py (2)
194-217: Override pass-through incleanup_retentionis clean and backward-compatible.Keyword-only optional parameters and direct forwarding to
RetentionEnforcer.cleanup_expired(...)are correctly implemented.
219-246:run_maintenancecorrectly threads agent retention overrides into cleanup.The maintenance orchestration now propagates agent-level override inputs without altering the existing consolidation/max-enforcement flow.
tests/unit/memory/consolidation/test_retention.py (3)
172-337: Resolution-chain tests are comprehensive and well targeted.This suite exercises override precedence and fallback semantics thoroughly, including empty/no-default paths and full-chain behavior.
339-483: Agent-override cleanup tests validate behavior and observability effectively.The added cases verify category selection, cutoff propagation, and override-event emission with expected structured fields.
485-500: Field-parity guard is a strong regression test.Validating field names and annotations between
AgentRetentionRuleandRetentionRuleis a solid safety net for future schema changes.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #951 +/- ##
=======================================
Coverage 91.98% 91.99%
=======================================
Files 605 605
Lines 32543 32583 +40
Branches 3141 3147 +6
=======================================
+ Hits 29935 29975 +40
Misses 2071 2071
Partials 537 537 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Code Review
This pull request introduces per-agent retention overrides, allowing individual agents to define custom memory retention periods that supersede company-level defaults. The implementation includes the AgentRetentionRule model, updated MemoryConfig validation, and a prioritized resolution logic in RetentionEnforcer. Feedback focuses on the inability to explicitly set a 'keep forever' override due to type constraints and resolution logic in the per-category settings. Additionally, minor suggestions were provided to remove unnecessary parentheses in type hints for consistency with the codebase.
| category: MemoryCategory = Field( | ||
| description="Memory category this override applies to", | ||
| ) | ||
| retention_days: int = Field( |
There was a problem hiding this comment.
The retention_days field is currently restricted to integers greater than or equal to 1. This prevents agents from explicitly overriding a company-level retention rule to "keep forever" (which is represented by None in other parts of the memory configuration). To support full override capabilities, this field should allow None values.
| retention_days: int = Field( | |
| retention_days: int | None = Field( |
| agent_days = agent_overrides.get(category) | ||
| if agent_days is not None: | ||
| result.append((category, agent_days)) | ||
| continue |
There was a problem hiding this comment.
The current implementation uses .get(category) and checks is not None. This logic fails to distinguish between a missing override and an explicit "keep forever" override (if retention_days were allowed to be None). Using an in check would correctly handle explicit None overrides by stopping the resolution chain for that category.
| agent_days = agent_overrides.get(category) | |
| if agent_days is not None: | |
| result.append((category, agent_days)) | |
| continue | |
| if category in agent_overrides: | |
| agent_days = agent_overrides[category] | |
| if agent_days is not None: | |
| result.append((category, agent_days)) | |
| continue |
| self, | ||
| agent_id: NotBlankStr, | ||
| *, | ||
| agent_category_overrides: (Mapping[MemoryCategory, int] | None) = None, |
There was a problem hiding this comment.
The parentheses around the type hint are unnecessary and inconsistent with standard Python type hinting practices used elsewhere in the codebase.
| agent_category_overrides: (Mapping[MemoryCategory, int] | None) = None, | |
| agent_category_overrides: Mapping[MemoryCategory, int] | None = None, |
| self, | ||
| agent_id: NotBlankStr, | ||
| *, | ||
| agent_category_overrides: (Mapping[MemoryCategory, int] | None) = None, |
There was a problem hiding this comment.
The parentheses around the type hint are unnecessary and inconsistent with standard Python type hinting practices used elsewhere in the codebase.
| agent_category_overrides: (Mapping[MemoryCategory, int] | None) = None, | |
| agent_category_overrides: Mapping[MemoryCategory, int] | None = None, |
…mini - Add logging to RetentionConfig and DualModeConfig validators (CLAUDE.md rule) - Remove unnecessary parentheses in service.py type annotations - Update stale _validate_retention_consistency docstring - Fix O(n^2) duplicate detection with set-based approach - Add runtime validation for agent override retention days (>= 1) - Standardize level 5 wording to "Keep forever (no expiry)" across all locations - Add clarifying comment on _explicit_rules contract - Fix empty dict triggering override path unnecessarily - Replace Sphinx :meth: ref to private method with plain backticks - Fix MD046 markdownlint warning in memory design page - Add retention_overrides to agent YAML example in design spec - Add 2 new tests: fast-path identity return + combined override params Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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_retention.py`:
- Around line 527-529: The test assumes call_args_list[0] is the WORKING
category which is order-dependent; instead, locate the call for
MemoryCategory.WORKING by scanning backend.retrieve.call_args_list for the entry
whose MemoryQuery (second positional arg) has category ==
MemoryCategory.WORKING, then assert that its query.until == _NOW -
timedelta(days=7); update any similar assertions to find calls by matching
query.category rather than relying on the list order (references:
backend.retrieve.call_args_list, MemoryQuery, MemoryCategory).
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 01f53f87-4aeb-4a6f-bc1b-bd3f25d0b4ed
📒 Files selected for processing (7)
docs/design/agents.mddocs/design/memory.mdsrc/synthorg/core/agent.pysrc/synthorg/memory/consolidation/config.pysrc/synthorg/memory/consolidation/retention.pysrc/synthorg/memory/consolidation/service.pytests/unit/memory/consolidation/test_retention.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). (6)
- GitHub Check: Test (Python 3.14)
- GitHub Check: Build Sandbox
- GitHub Check: Build Web
- GitHub Check: Build Backend
- GitHub Check: Dependency Review
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Python version 3.14+ with PEP 649 native lazy annotations
Do NOT usefrom __future__ import annotations-- Python 3.14 has PEP 649
PEP 758 except syntax: useexcept A, B:(no parentheses) -- ruff enforces this on Python 3.14
Line length: 88 characters (ruff enforced)
Files:
src/synthorg/memory/consolidation/config.pysrc/synthorg/core/agent.pysrc/synthorg/memory/consolidation/retention.pysrc/synthorg/memory/consolidation/service.pytests/unit/memory/consolidation/test_retention.py
src/synthorg/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/synthorg/**/*.py: Type hints: all public functions, mypy strict mode
Docstrings: Google style, required on public classes/functions (enforced by ruff D rules)
Use Pydantic v2 (BaseModel,model_validator,computed_field,ConfigDict)
Useallow_inf_nan=Falsein allConfigDictdeclarations to rejectNaN/Infin numeric fields at validation time
Use@computed_fieldfor derived values instead of storing + validating redundant fields (e.g.TokenUsage.total_tokens)
UseNotBlankStr(fromcore.types) for all identifier/name fields -- including optional (NotBlankStr | None) and tuple (tuple[NotBlankStr, ...]) variants -- instead of manual whitespace validators
Async concurrency: preferasyncio.TaskGroupfor fan-out/fan-in parallel operations in new code (e.g. multiple tool invocations, parallel agent calls); prefer structured concurrency over barecreate_task
Functions: < 50 lines, files < 800 lines
Errors: handle explicitly, never silently swallow
Every module with business logic MUST have:from synthorg.observability import get_loggerthenlogger = get_logger(__name__)
Never useimport logging/logging.getLogger()/print()in application code (exception:observability/setup.pyandobservability/sinks.pymay use stdlibloggingandprint(..., file=sys.stderr)for bootstrap and handler-cleanup code)
Variable name for logger: alwayslogger(not_logger, notlog)
Event names: always use constants from the domain-specific module undersynthorg.observability.events(e.g.,API_REQUEST_STARTEDfromevents.api,TOOL_INVOKE_STARTfromevents.tool); import directly:from synthorg.observability.events.<domain> import EVENT_CONSTANT
Structured kwargs for logging: alwayslogger.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
DEBUG logging for object creation, internal flow, entry/exit of key...
Files:
src/synthorg/memory/consolidation/config.pysrc/synthorg/core/agent.pysrc/synthorg/memory/consolidation/retention.pysrc/synthorg/memory/consolidation/service.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Test markers:@pytest.mark.unit,@pytest.mark.integration,@pytest.mark.e2e,@pytest.mark.slow
Async tests:asyncio_mode = "auto"-- no manual@pytest.mark.asyncioneeded
Test timeout: 30 seconds per test (global inpyproject.toml-- do not add per-filepytest.mark.timeout(30)markers; non-default overrides liketimeout(60)are allowed)
Parametrize: Prefer@pytest.mark.parametrizefor testing similar cases
Tests must usetest-provider,test-small-001, etc. instead of real vendor names
Property-based testing in Python: use Hypothesis (@given+@settings); run withHYPOTHESIS_PROFILE=dev uv run python -m pytest tests/ -m unit -n auto -k properties(dev profile: 1000 examples)
Hypothesis profiles:ci(50 examples, default) anddev(1000 examples), controlled viaHYPOTHESIS_PROFILEenv var
For timing-sensitive tests, mocktime.monotonic()andasyncio.sleep()to make them deterministic instead of widening timing margins
For tasks that must block indefinitely until cancelled, useasyncio.Event().wait()instead ofasyncio.sleep(large_number)-- it is cancellation-safe and carries no timing assumptions
Files:
tests/unit/memory/consolidation/test_retention.py
🧠 Learnings (15)
📚 Learning: 2026-03-19T07:12:14.508Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T07:12:14.508Z
Learning: Applies to src/synthorg/memory/**/*.py : Memory package (memory/): pluggable MemoryBackend protocol, backends/ (Mem0 adapter), retrieval pipeline (ranking, RRF fusion, injection, formatting, non-inferable filtering), shared org memory (org/), consolidation/archival (density-aware: DensityClassifier, AbstractiveSummarizer, ExtractivePreserver, DualModeConsolidationStrategy)
Applied to files:
docs/design/memory.mdsrc/synthorg/core/agent.pysrc/synthorg/memory/consolidation/retention.pysrc/synthorg/memory/consolidation/service.py
📚 Learning: 2026-03-15T19:14:27.144Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T19:14:27.144Z
Learning: Applies to src/synthorg/**/*.py : Use frozen Pydantic models for config/identity; use separate mutable-via-copy models (using model_copy(update=...)) for runtime state that evolves. Never mix static config fields with mutable runtime fields in one model.
Applied to files:
src/synthorg/memory/consolidation/config.pysrc/synthorg/core/agent.py
📚 Learning: 2026-03-20T21:44:04.528Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-20T21:44:04.528Z
Learning: Applies to src/synthorg/**/*.py : Use frozen Pydantic models for config/identity; use separate mutable-via-copy models (via `model_copy(update=...)`) for runtime state that evolves
Applied to files:
src/synthorg/memory/consolidation/config.pysrc/synthorg/core/agent.py
📚 Learning: 2026-03-16T06:24:56.341Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T06:24:56.341Z
Learning: Applies to src/synthorg/memory/**/*.py : Use MemoryBackend protocol with pluggable backends (Mem0 adapter available at backends/mem0/) for persistent agent memory
Applied to files:
src/synthorg/core/agent.pysrc/synthorg/memory/consolidation/retention.py
📚 Learning: 2026-03-15T19:14:27.144Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T19:14:27.144Z
Learning: Applies to src/synthorg/**/*.py : For dict/list fields in frozen Pydantic models, rely on frozen=True for field reassignment prevention and copy.deepcopy() at system boundaries (tool execution, LLM provider serialization, inter-agent delegation, serializing for persistence).
Applied to files:
src/synthorg/core/agent.py
📚 Learning: 2026-03-15T18:38:44.202Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T18:38:44.202Z
Learning: Applies to src/synthorg/**/*.py : Use frozen Pydantic models for config/identity; separate mutable-via-copy models (using `model_copy(update=...)`) for runtime state
Applied to files:
src/synthorg/core/agent.py
📚 Learning: 2026-03-19T07:12:14.508Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T07:12:14.508Z
Learning: Applies to src/synthorg/observability/**/*.py : Observability package (observability/): structured logging, correlation tracking, log sinks; event constants organized by domain under observability/events/ (e.g., events.api, events.tool, events.git, events.context_budget, events.backup)
Applied to files:
src/synthorg/memory/consolidation/retention.py
📚 Learning: 2026-03-31T14:17:24.182Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-31T14:17:24.182Z
Learning: Applies to src/synthorg/**/*.py : Use event name constants from `synthorg.observability.events.<domain>` modules (e.g., `API_REQUEST_STARTED` from `events.api`, `TOOL_INVOKE_START` from `events.tool`); import directly and use in structured logging
Applied to files:
src/synthorg/memory/consolidation/retention.py
📚 Learning: 2026-03-17T06:30:14.180Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T06:30:14.180Z
Learning: Applies to src/synthorg/observability/**/*.py : Observability includes structured logging via `get_logger(__name__)`, correlation tracking, and log sinks.
Applied to files:
src/synthorg/memory/consolidation/retention.py
📚 Learning: 2026-03-16T06:24:56.341Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T06:24:56.341Z
Learning: Applies to src/synthorg/**/*.py : Always use event name constants from the domain-specific module under `synthorg.observability.events` in logging calls
Applied to files:
src/synthorg/memory/consolidation/retention.py
📚 Learning: 2026-03-15T19:14:27.144Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T19:14:27.144Z
Learning: Applies to src/synthorg/**/*.py : Use event name constants from synthorg.observability.events domain-specific modules (e.g., PROVIDER_CALL_START from events.provider). Import directly: from synthorg.observability.events.<domain> import EVENT_CONSTANT.
Applied to files:
src/synthorg/memory/consolidation/retention.py
📚 Learning: 2026-03-19T11:33:01.580Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T11:33:01.580Z
Learning: Applies to src/synthorg/**/*.py : Use event constants from `synthorg.observability.events.<domain>` (e.g., `API_REQUEST_STARTED` from `events.api`); import directly and log with structured kwargs: `logger.info(EVENT, key=value)`, never interpolated strings
Applied to files:
src/synthorg/memory/consolidation/retention.py
📚 Learning: 2026-03-18T21:23:23.586Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-18T21:23:23.586Z
Learning: Applies to src/synthorg/**/*.py : Event names: always use constants from the domain-specific module under synthorg.observability.events (e.g., API_REQUEST_STARTED from events.api, TOOL_INVOKE_START from events.tool). Import directly from synthorg.observability.events.<domain>.
Applied to files:
src/synthorg/memory/consolidation/retention.py
📚 Learning: 2026-03-15T18:28:13.207Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T18:28:13.207Z
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/memory/consolidation/retention.py
📚 Learning: 2026-03-31T14:40:41.736Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-31T14:40:41.736Z
Learning: Applies to tests/**/*.py : For timing-sensitive tests, mock `time.monotonic()` and `asyncio.sleep()` to make them deterministic instead of widening timing margins
Applied to files:
tests/unit/memory/consolidation/test_retention.py
🔇 Additional comments (15)
src/synthorg/core/agent.py (3)
183-204: LGTM! Well-structured frozen model for agent retention overrides.The
AgentRetentionRulemodel correctly mirrorsRetentionRulein the memory package while avoiding acore -> memoryimport dependency. Thege=1constraint onretention_daysandfrozen=Trueconfig are appropriate.
231-234: LGTM! Per-category retention overrides field.Using a tuple of
AgentRetentionRulerather than a dict preserves field ordering and immutability consistent with other frozen model patterns in this codebase.
262-283: LGTM! Efficient duplicate detection with proper observability.The O(n)
seenset approach (addressed from earlier O(n²) finding per commit messages) correctly identifies duplicates. Structured logging withCONFIG_VALIDATION_FAILEDfollows guidelines.src/synthorg/memory/consolidation/config.py (2)
21-39: LGTM! Clear documentation of the resolution chain.The docstring accurately documents the 5-level precedence order which aligns with the implementation in
retention.py. This helps maintainers understand the override behavior.
53-73: LGTM! Consistent validation pattern with observability.The duplicate detection mirrors the implementation in
MemoryConfig._validate_unique_override_categories, using the efficient O(n)seen/dupesapproach with proper structured logging.src/synthorg/memory/consolidation/retention.py (3)
48-53: LGTM! Critical distinction between explicit rules and default-filled entries.Storing only explicit per-category rules (without default-filled entries) is essential for correct resolution. The docstring clearly explains this constraint and its importance for
_resolve_categories.
80-138: LGTM! Well-implemented 5-level resolution chain.The implementation correctly follows the documented priority:
- Agent per-category override
- Company per-category rule (explicit only)
- Agent global default
- Company global default
- Keep forever (skip)
Input validation for
retention_days >= 1at lines 109-115 ensures invalid values are rejected early.
156-172: Past review finding addressed: empty dict no longer triggers override log.The
bool(agent_category_overrides)check correctly treats{}as "no overrides" rather than "overrides applied", preventing spurious observability events.docs/design/memory.md (1)
357-369: LGTM! Clear documentation of per-agent retention overrides.The section accurately documents the resolution order and uses lint-friendly markdown formatting (addressing the prior MD046 concern). The numbered list clearly communicates the precedence chain.
docs/design/agents.md (1)
121-122: LGTM! Agent YAML example updated with retention override fields.The example correctly documents both
retention_daysas the agent-level global default andretention_overridesfor per-category rules.src/synthorg/memory/consolidation/service.py (2)
194-217: LGTM! Clean pass-through of retention override parameters.The service layer correctly forwards
agent_category_overridesandagent_default_retention_daysto the underlyingRetentionEnforcer. The backward-compatibleNonedefaults preserve existing behavior.
219-246: Note: No production callers currently populate override parameters.Per the method signatures,
run_maintenanceandcleanup_retentionaccept the new override parameters (agent_category_overridesandagent_default_retention_days), which are threaded through the call chain. However, no external callers currently populate these parameters—they're only used internally within the maintenance cycle. This is likely intentional as a future integration point for custom retention rules (e.g., extracting values fromAgentIdentity.memory.retention_overrides), but worth tracking.tests/unit/memory/consolidation/test_retention.py (3)
172-336: LGTM! Comprehensive tests for the 5-level resolution chain.
TestResolveCategoriesthoroughly covers all priority levels:
- Agent override replacing/adding categories
- Agent default filling gaps
- Company rules beating agent defaults
- Company defaults as fallback
- Full chain integration test
459-482: LGTM! Structured log event assertion.Using
structlog.testing.capture_logs()to validate theRETENTION_AGENT_OVERRIDE_APPLIEDevent is the correct approach for testing observability.
532-546: LGTM! Clever parity test to prevent model drift.This test ensures
AgentRetentionRuleandRetentionRulemaintain identical field names and types, catching future divergence between the duplicate models incoreandmemorypackages.
Find the WORKING category call by scanning query.categories instead of relying on call_args_list ordering, making the test resilient to iteration order changes. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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_retention.py`:
- Around line 473-477: Replace the hard-coded event string in the test filter
with the event constant: import RETENTION_AGENT_OVERRIDE_APPLIED from
src/synthorg/observability/events/consolidation.py and use it in the list
comprehension that defines override_logs (the comprehension currently
referencing log.get("event") ==
"consolidation.retention.agent_override_applied"); this makes the assertion
resilient to renames and ensures the test references the canonical event symbol.
- Around line 176-337: Multiple near-duplicate tests exercise
RetentionEnforcer._resolve_categories with different inputs; replace them with a
single parametrized pytest test to remove duplication. Create one test function
(e.g., test_resolve_categories_parametrized) decorated with
`@pytest.mark.parametrize` that supplies tuples of (base, agent_overrides,
agent_default_days, company_default_days, expected_dict) covering each of the 10
scenarios currently implemented (including empty case), call
RetentionEnforcer._resolve_categories for each row, convert result to dict and
assert equality/expectations, and remove the original individual test_*
functions; keep the same scenario names as param ids for readability. Ensure you
import pytest and preserve assertions for per-category checks (use expected_dict
to drive them) so coverage of agent vs company vs defaults remains identical to
the originals.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 814696a7-08cf-4123-8224-ed583f44298e
📒 Files selected for processing (1)
tests/unit/memory/consolidation/test_retention.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). (6)
- GitHub Check: Test (Python 3.14)
- GitHub Check: Build Backend
- GitHub Check: Build Sandbox
- GitHub Check: Build Web
- GitHub Check: Dependency Review
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Python version 3.14+ with PEP 649 native lazy annotations
Do NOT usefrom __future__ import annotations-- Python 3.14 has PEP 649
PEP 758 except syntax: useexcept A, B:(no parentheses) -- ruff enforces this on Python 3.14
Line length: 88 characters (ruff enforced)
Files:
tests/unit/memory/consolidation/test_retention.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Test markers:@pytest.mark.unit,@pytest.mark.integration,@pytest.mark.e2e,@pytest.mark.slow
Async tests:asyncio_mode = "auto"-- no manual@pytest.mark.asyncioneeded
Test timeout: 30 seconds per test (global inpyproject.toml-- do not add per-filepytest.mark.timeout(30)markers; non-default overrides liketimeout(60)are allowed)
Parametrize: Prefer@pytest.mark.parametrizefor testing similar cases
Tests must usetest-provider,test-small-001, etc. instead of real vendor names
Property-based testing in Python: use Hypothesis (@given+@settings); run withHYPOTHESIS_PROFILE=dev uv run python -m pytest tests/ -m unit -n auto -k properties(dev profile: 1000 examples)
Hypothesis profiles:ci(50 examples, default) anddev(1000 examples), controlled viaHYPOTHESIS_PROFILEenv var
For timing-sensitive tests, mocktime.monotonic()andasyncio.sleep()to make them deterministic instead of widening timing margins
For tasks that must block indefinitely until cancelled, useasyncio.Event().wait()instead ofasyncio.sleep(large_number)-- it is cancellation-safe and carries no timing assumptions
Files:
tests/unit/memory/consolidation/test_retention.py
🧠 Learnings (2)
📚 Learning: 2026-03-20T21:44:04.528Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-20T21:44:04.528Z
Learning: Applies to tests/**/*.py : Fix flaky tests completely and fundamentally; for timing-sensitive tests, mock `time.monotonic()` and `asyncio.sleep()` to make them deterministic instead of widening timing margins
Applied to files:
tests/unit/memory/consolidation/test_retention.py
📚 Learning: 2026-03-31T14:40:41.736Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-31T14:40:41.736Z
Learning: Applies to tests/**/*.py : For timing-sensitive tests, mock `time.monotonic()` and `asyncio.sleep()` to make them deterministic instead of widening timing margins
Applied to files:
tests/unit/memory/consolidation/test_retention.py
🔇 Additional comments (1)
tests/unit/memory/consolidation/test_retention.py (1)
527-534: Good fix: WORKING-category assertion is now order-independent.Matching the retrieve call by
MemoryCategory.WORKINGremoves call-order coupling and makes this test substantially less brittle.
| def test_agent_override_replaces_company_rule(self) -> None: | ||
| """Agent per-category rule overrides company per-category rule.""" | ||
| base = ((MemoryCategory.WORKING, 30),) | ||
| agent_overrides: dict[MemoryCategory, int] = { | ||
| MemoryCategory.WORKING: 90, | ||
| } | ||
| result = RetentionEnforcer._resolve_categories( | ||
| base, | ||
| agent_overrides=agent_overrides, | ||
| agent_default_days=None, | ||
| company_default_days=None, | ||
| ) | ||
| result_dict = dict(result) | ||
| assert result_dict[MemoryCategory.WORKING] == 90 | ||
|
|
||
| def test_agent_override_adds_new_category(self) -> None: | ||
| """Agent can add a rule for a category not in company config.""" | ||
| base = ((MemoryCategory.WORKING, 30),) | ||
| agent_overrides: dict[MemoryCategory, int] = { | ||
| MemoryCategory.SEMANTIC: 365, | ||
| } | ||
| result = RetentionEnforcer._resolve_categories( | ||
| base, | ||
| agent_overrides=agent_overrides, | ||
| agent_default_days=None, | ||
| company_default_days=None, | ||
| ) | ||
| result_dict = dict(result) | ||
| assert result_dict[MemoryCategory.WORKING] == 30 | ||
| assert result_dict[MemoryCategory.SEMANTIC] == 365 | ||
|
|
||
| def test_agent_default_fills_gaps(self) -> None: | ||
| """Agent default_retention_days fills categories without rules.""" | ||
| base = () # no company rules | ||
| result = RetentionEnforcer._resolve_categories( | ||
| base, | ||
| agent_overrides={}, | ||
| agent_default_days=60, | ||
| company_default_days=None, | ||
| ) | ||
| result_dict = dict(result) | ||
| # All categories should get the agent default | ||
| for cat in MemoryCategory: | ||
| assert result_dict[cat] == 60 | ||
|
|
||
| def test_agent_rule_beats_agent_default(self) -> None: | ||
| """Agent per-category rule takes priority over agent default.""" | ||
| base = () | ||
| agent_overrides: dict[MemoryCategory, int] = { | ||
| MemoryCategory.EPISODIC: 180, | ||
| } | ||
| result = RetentionEnforcer._resolve_categories( | ||
| base, | ||
| agent_overrides=agent_overrides, | ||
| agent_default_days=60, | ||
| company_default_days=None, | ||
| ) | ||
| result_dict = dict(result) | ||
| assert result_dict[MemoryCategory.EPISODIC] == 180 | ||
| # Other categories get agent default | ||
| assert result_dict[MemoryCategory.WORKING] == 60 | ||
|
|
||
| def test_company_rule_beats_agent_default(self) -> None: | ||
| """Company per-category rule beats agent global default.""" | ||
| base = ((MemoryCategory.WORKING, 7),) | ||
| result = RetentionEnforcer._resolve_categories( | ||
| base, | ||
| agent_overrides={}, | ||
| agent_default_days=365, | ||
| company_default_days=None, | ||
| ) | ||
| result_dict = dict(result) | ||
| # Company rule for WORKING wins over agent default | ||
| assert result_dict[MemoryCategory.WORKING] == 7 | ||
| # Agent default fills other categories | ||
| assert result_dict[MemoryCategory.SEMANTIC] == 365 | ||
|
|
||
| def test_company_default_used_when_no_agent_default(self) -> None: | ||
| """Company default fills gaps when no agent default is set.""" | ||
| base = ((MemoryCategory.WORKING, 7),) | ||
| result = RetentionEnforcer._resolve_categories( | ||
| base, | ||
| agent_overrides={}, | ||
| agent_default_days=None, | ||
| company_default_days=30, | ||
| ) | ||
| result_dict = dict(result) | ||
| assert result_dict[MemoryCategory.WORKING] == 7 | ||
| assert result_dict[MemoryCategory.SEMANTIC] == 30 | ||
|
|
||
| def test_no_overrides_returns_base(self) -> None: | ||
| """When no agent overrides, result matches base plus defaults.""" | ||
| base = ( | ||
| (MemoryCategory.WORKING, 30), | ||
| (MemoryCategory.EPISODIC, 90), | ||
| ) | ||
| result = RetentionEnforcer._resolve_categories( | ||
| base, | ||
| agent_overrides={}, | ||
| agent_default_days=None, | ||
| company_default_days=None, | ||
| ) | ||
| result_dict = dict(result) | ||
| assert result_dict[MemoryCategory.WORKING] == 30 | ||
| assert result_dict[MemoryCategory.EPISODIC] == 90 | ||
| # Categories with no rule at all are not included | ||
| assert MemoryCategory.SOCIAL not in result_dict | ||
|
|
||
| def test_full_resolution_chain(self) -> None: | ||
| """Test all 5 resolution levels in a single scenario. | ||
|
|
||
| - WORKING: agent per-category (7) beats company per-category (30) | ||
| - EPISODIC: company per-category (90) beats agent default (60) | ||
| - SEMANTIC: agent per-category (365) -- no company rule | ||
| - PROCEDURAL: agent default (60) -- no rules, no company rule | ||
| - SOCIAL: agent default (60) -- no rules, agent default > co default | ||
| """ | ||
| # Only explicit company per-category rules (not company default) | ||
| explicit_rules = ( | ||
| (MemoryCategory.WORKING, 30), | ||
| (MemoryCategory.EPISODIC, 90), | ||
| ) | ||
| agent_overrides: dict[MemoryCategory, int] = { | ||
| MemoryCategory.WORKING: 7, | ||
| MemoryCategory.SEMANTIC: 365, | ||
| } | ||
| result = RetentionEnforcer._resolve_categories( | ||
| explicit_rules, | ||
| agent_overrides=agent_overrides, | ||
| agent_default_days=60, | ||
| company_default_days=45, | ||
| ) | ||
| result_dict = dict(result) | ||
| assert result_dict[MemoryCategory.WORKING] == 7 # 1. agent rule | ||
| assert result_dict[MemoryCategory.EPISODIC] == 90 # 2. company rule | ||
| assert result_dict[MemoryCategory.SEMANTIC] == 365 # 1. agent rule | ||
| assert result_dict[MemoryCategory.PROCEDURAL] == 60 # 3. agent default | ||
| assert result_dict[MemoryCategory.SOCIAL] == 60 # 3. agent default | ||
|
|
||
| def test_company_default_used_when_agent_default_absent(self) -> None: | ||
| """Priority 4 (company default) used when no agent default.""" | ||
| explicit_rules = ((MemoryCategory.WORKING, 30),) | ||
| result = RetentionEnforcer._resolve_categories( | ||
| explicit_rules, | ||
| agent_overrides={}, | ||
| agent_default_days=None, | ||
| company_default_days=45, | ||
| ) | ||
| result_dict = dict(result) | ||
| assert result_dict[MemoryCategory.WORKING] == 30 # 2. company rule | ||
| assert result_dict[MemoryCategory.SOCIAL] == 45 # 4. company default | ||
|
|
||
| def test_empty_everything_returns_empty(self) -> None: | ||
| """No rules, no defaults -- returns empty tuple.""" | ||
| result = RetentionEnforcer._resolve_categories( | ||
| (), | ||
| agent_overrides={}, | ||
| agent_default_days=None, | ||
| company_default_days=None, | ||
| ) | ||
| assert result == () | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify repetition hotspots around _resolve_categories tests
rg -n "def test_.*|_resolve_categories\(" tests/unit/memory/consolidation/test_retention.pyRepository: Aureliolo/synthorg
Length of output: 2388
🏁 Script executed:
# Check if tests have docstrings explaining distinct scenarios
cat tests/unit/memory/consolidation/test_retention.py | sed -n '176,337p' | head -60Repository: Aureliolo/synthorg
Length of output: 2418
🏁 Script executed:
# Verify the test methods exist at the specified lines and count assertions
sed -n '176,337p' tests/unit/memory/consolidation/test_retention.py | grep -E "^\s*(def test_|assert |for )" | head -50Repository: Aureliolo/synthorg
Length of output: 1999
Consolidate _resolve_categories tests using @pytest.mark.parametrize.
These 10 test methods (lines 176–337) all follow the same pattern: define base/agent_overrides, call _resolve_categories, and assert on the result. Converting to parametrized test cases would reduce duplication and make the priority resolution scenarios easier to extend. Per coding guidelines: "Prefer @pytest.mark.parametrize for testing similar cases."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/memory/consolidation/test_retention.py` around lines 176 - 337,
Multiple near-duplicate tests exercise RetentionEnforcer._resolve_categories
with different inputs; replace them with a single parametrized pytest test to
remove duplication. Create one test function (e.g.,
test_resolve_categories_parametrized) decorated with `@pytest.mark.parametrize`
that supplies tuples of (base, agent_overrides, agent_default_days,
company_default_days, expected_dict) covering each of the 10 scenarios currently
implemented (including empty case), call RetentionEnforcer._resolve_categories
for each row, convert result to dict and assert equality/expectations, and
remove the original individual test_* functions; keep the same scenario names as
param ids for readability. Ensure you import pytest and preserve assertions for
per-category checks (use expected_dict to drive them) so coverage of agent vs
company vs defaults remains identical to the originals.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
🤖 I have created a release *beep* *boop* --- ## [0.5.4](v0.5.3...v0.5.4) (2026-04-01) ### Features * artifact and project management UI in web dashboard ([#954](#954)) ([00a0430](00a0430)) * embed MkDocs build output in React web dashboard at /docs ([#948](#948)) ([f229fc2](f229fc2)) * personality preset discovery API and user-defined preset CRUD ([#952](#952)) ([497848a](497848a)) * support multi-provider model resolution with budget-based selection ([#953](#953)) ([146b782](146b782)) * support per-agent memory retention overrides ([#209](#209)) ([#951](#951)) ([020c610](020c610)) ### Documentation * write user guides and tutorials ([#949](#949)) ([1367225](1367225)) --- 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
AgentRetentionRulemodel andMemoryConfig.retention_overridesfield for per-agent, per-category retention overridesRetentionEnforcerwith_resolve_categories()implementing a 5-level resolution chain: agent per-category > company per-category > agent global default > company global default > keep foreverMemoryConsolidationService.cleanup_retention()andrun_maintenance()RETENTION_AGENT_OVERRIDE_APPLIEDstructured log eventRetentionConfigdocstring to document override mechanismTest plan
AgentRetentionRulemodel validation (frozen, ge=1, invalid values)MemoryConfig.retention_overrides(defaults, duplicates, NONE type guard, coexistence withretention_days)_resolve_categoriesresolution chain (all 5 priority levels individually + combined)cleanup_expiredwith agent overrides (cutoff correctness, category expansion, log event)AgentRetentionRule/RetentionRulefield parity testCloses #209
🤖 Generated with Claude Code