-
Notifications
You must be signed in to change notification settings - Fork 1
feat: add pluggable MemoryBackend protocol with models, config, and events #180
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
1b82823
077dddd
884b7b5
377d4d2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -409,7 +409,7 @@ class MemoryCapabilities(Protocol): | |||||||||||||||
| """Capability discovery — what this backend supports.""" | ||||||||||||||||
|
|
||||||||||||||||
| @property | ||||||||||||||||
| def supported_types(self) -> frozenset[MemoryType]: ... | ||||||||||||||||
| def supported_types(self) -> frozenset[MemoryCategory]: ... | ||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's an inconsistency in the property name for the Additionally, the protocol definition in this ADR is incomplete compared to the final implementation. It's missing the
greptile-apps[bot] marked this conversation as resolved.
Outdated
|
||||||||||||||||
| def supported_types(self) -> frozenset[MemoryCategory]: ... | |
| def supported_categories(self) -> frozenset[MemoryCategory]: ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Property name mismatch between ADR and implementation.
The ADR shows supported_types but src/ai_company/memory/capabilities.py (line 29) defines supported_categories. Update the ADR to match the actual implementation.
📝 Suggested fix
`@property`
- def supported_types(self) -> frozenset[MemoryCategory]: ...
+ def supported_categories(self) -> frozenset[MemoryCategory]: ...📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @property | |
| def supported_types(self) -> frozenset[MemoryType]: ... | |
| def supported_types(self) -> frozenset[MemoryCategory]: ... | |
| @property | |
| `@property` | |
| def supported_categories(self) -> frozenset[MemoryCategory]: ... | |
| `@property` |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/decisions/ADR-001-memory-layer.md` around lines 411 - 413, The ADR
declares a property named supported_types but the implementation exposes
supported_categories; update the ADR to use the same property name
supported_categories (and keep the declared return type
frozenset[MemoryCategory]) so the design document matches the code (also scan
for any references in ADR-001-memory-layer.md to supported_types and rename them
to supported_categories).
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,6 +17,7 @@ | |
| from ai_company.core.enums import CompanyType, SeniorityLevel | ||
| from ai_company.core.role import CustomRole # noqa: TC001 | ||
| from ai_company.core.types import NotBlankStr # noqa: TC001 | ||
| from ai_company.memory.config import CompanyMemoryConfig | ||
| from ai_company.observability import get_logger | ||
| from ai_company.observability.config import LogConfig # noqa: TC001 | ||
| from ai_company.observability.events.config import CONFIG_VALIDATION_FAILED | ||
|
|
@@ -77,6 +78,14 @@ def _validate_delay_ordering(self) -> Self: | |
| f"base_delay ({self.base_delay}) must be" | ||
| f" <= max_delay ({self.max_delay})" | ||
| ) | ||
| logger.warning( | ||
| CONFIG_VALIDATION_FAILED, | ||
| model="RetryConfig", | ||
| field="base_delay/max_delay", | ||
| base_delay=self.base_delay, | ||
| max_delay=self.max_delay, | ||
| reason=msg, | ||
| ) | ||
| raise ValueError(msg) | ||
| return self | ||
|
|
||
|
|
@@ -459,6 +468,7 @@ class RootConfig(BaseModel): | |
| escalation_paths: Cross-department escalation paths. | ||
| coordination_metrics: Coordination metrics configuration. | ||
| task_assignment: Task assignment configuration. | ||
| memory: Memory backend configuration. | ||
| persistence: Persistence backend configuration. | ||
| """ | ||
|
|
||
|
|
@@ -527,6 +537,10 @@ class RootConfig(BaseModel): | |
| default_factory=TaskAssignmentConfig, | ||
| description="Task assignment configuration", | ||
| ) | ||
| memory: CompanyMemoryConfig = Field( | ||
| default_factory=CompanyMemoryConfig, | ||
| description="Memory backend configuration", | ||
| ) | ||
|
Comment on lines
+540
to
+543
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't default every root config to an unbuildable backend.
🤖 Prompt for AI Agents |
||
| persistence: PersistenceConfig = Field( | ||
| default_factory=PersistenceConfig, | ||
| description="Persistence backend configuration", | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -88,15 +88,38 @@ class CreativityLevel(StrEnum): | |
| HIGH = "high" | ||
|
|
||
|
|
||
| class MemoryType(StrEnum): | ||
| """Memory persistence type for an agent.""" | ||
| class MemoryLevel(StrEnum): | ||
| """Memory persistence level for an agent (§7.3). | ||
|
|
||
| Note: DESIGN_SPEC §7.3 uses ``"full"`` — renamed to ``PERSISTENT`` | ||
| for clarity (approved deviation). | ||
| """ | ||
|
|
||
| PERSISTENT = "persistent" | ||
| PROJECT = "project" | ||
| SESSION = "session" | ||
| NONE = "none" | ||
|
Comment on lines
+91
to
97
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add a compatibility path for the renamed memory level.
🤖 Prompt for AI Agents |
||
|
|
||
|
|
||
| class MemoryCategory(StrEnum): | ||
| """Memory type categories for agent memory (§7.2).""" | ||
|
|
||
| WORKING = "working" | ||
| EPISODIC = "episodic" | ||
| SEMANTIC = "semantic" | ||
| PROCEDURAL = "procedural" | ||
| SOCIAL = "social" | ||
|
|
||
|
|
||
| class ConsolidationInterval(StrEnum): | ||
| """Interval for memory consolidation.""" | ||
|
|
||
| HOURLY = "hourly" | ||
| DAILY = "daily" | ||
| WEEKLY = "weekly" | ||
| NEVER = "never" | ||
|
|
||
|
|
||
| class CostTier(StrEnum): | ||
| """Built-in cost tier identifiers. | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Table of Contents entry for section 7 lists 7.5 before 7.4; this is out of numeric order and doesn’t match the section numbering below. Please reorder the referenced subsections (7.4, 7.5, 7.6) so the TOC reflects the actual structure.