feat(domain): add F3 episodic semantic-memory contract - #216
Conversation
🤖 CodeAnt AI — Review Status
|
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
|
Tick the box to add this pull request to the merge queue (same as
|
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| async fn recall( | ||
| &self, | ||
| query: SemanticMemoryQuery, | ||
| budget: SemanticMemoryBudget, | ||
| ) -> Result<Vec<SemanticMemoryRecord>, SemanticMemoryError>; |
There was a problem hiding this comment.
Suggestion: The validated budget is never applied to returned records, so recall can exceed the documented hard content cap before prompt construction. [incomplete implementation]
Assessment: 🟠 Major · 🔁 Occurrence: Sometimes
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/forge_domain/src/semantic_memory.rs
**Line:** 350:354
**Comment:**
*Incomplete Implementation: The validated budget is never applied to returned records, so recall can exceed the documented hard content cap before prompt construction.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
CodeAnt Nitpicks1 code suggestion1. A zero limit is reported as
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c4c7b5c595
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| async fn recall( | ||
| &self, | ||
| query: SemanticMemoryQuery, | ||
| budget: SemanticMemoryBudget, | ||
| ) -> Result<Vec<SemanticMemoryRecord>, SemanticMemoryError>; |
There was a problem hiding this comment.
When one semantic-memory backend serves multiple workspaces, this request contains no workspace or conversation filter, so an adapter cannot constrain the provider query before records are retrieved; it must either search globally—risking cross-workspace conversation disclosure—or rely on an undocumented per-workspace port instance. Include the owning WorkspaceId (and any intended conversation boundary) in the recall request so tenant isolation is enforceable.
Useful? React with 👍 / 👎.
| #[async_trait] | ||
| pub trait SemanticMemoryPort: Send + Sync { | ||
| /// Stores one Episodic memory and returns its provider-assigned identifier. | ||
| async fn store(&self, record: SemanticMemoryRecord) -> Result<String, SemanticMemoryError>; |
There was a problem hiding this comment.
When callers store a new episodic memory, they must construct a SemanticMemoryRecord, even though that type requires an adapter-provided relevance score and represents a recalled result; no meaningful score exists before a query, so every caller must fabricate one and may accidentally persist query-specific metadata. Accept a score-free write type here and reserve SemanticMemoryRecord for recall output.
Useful? React with 👍 / 👎.
|
|
||
| impl SemanticMemoryProvenance { | ||
| /// Creates provenance for a record derived from one conversation. | ||
| pub fn new( |
There was a problem hiding this comment.
This public constructor takes three parameters but omits the required # Arguments section, and the same omission recurs on the other new constructors and port methods. Add parameter documentation to each parameterized public API so the domain contract follows the repository's LLM-oriented documentation requirements.
AGENTS.md reference: AGENTS.md:L113-L117
Useful? React with 👍 / 👎.
| #[derive(Debug, Clone, PartialEq, Eq)] | ||
| pub struct SemanticMemoryProvenance { |
There was a problem hiding this comment.
The newly introduced domain structs, beginning with SemanticMemoryProvenance, omit derive_setters::Setters and the required into/strip_option configuration. Add the repository-standard setter derivation to the new domain types while preserving constructor validation for constrained fields.
AGENTS.md reference: AGENTS.md:L109-L111
Useful? React with 👍 / 👎.
| let actual = SemanticMemoryScope::try_from(MemoryScope::Identity); | ||
| assert_eq!( | ||
| actual, | ||
| Err(SemanticMemoryError::UnsupportedScope(MemoryScope::Identity)) | ||
| ); |
There was a problem hiding this comment.
After the first case, this test directly embeds the expected error in the assertion instead of keeping discrete fixture/setup, actual, and handwritten expected steps; similar patterns occur throughout the new test module. Restructure each case using the mandated three-step form so failures and intended outputs remain explicit.
AGENTS.md reference: AGENTS.md:L13-L23
Useful? React with 👍 / 👎.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| @@ -0,0 +1,400 @@ | |||
| #[cfg(test)] | |||
There was a problem hiding this comment.
WARNING: #[cfg(test)] mod tests is declared before every production item in the file
Every other module in forge_domain puts the test module last (e.g. conversation.rs:310). Here all use declarations, types, the trait and the error enum come after a #[cfg(test)] module, which is exactly the shape clippy's warn-by-default clippy::items_after_test_module lint targets. When the lib is linted as a test target (cargo clippy --workspace --all-targets -- -D warnings) that turns into a hard CI failure, so please confirm CI runs --all-targets and move the module to the bottom of the file, after SemanticMemoryError.
Related nit: use crate::{ConversationId, MemoryScope, WorkspaceId}; (line 5) is redundant — use super::*; (line 7) already pulls those names in from the parent module, so the explicit import only shadows the glob.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| } | ||
|
|
||
| #[test] | ||
| fn query_rejects_empty_text_excessive_limit_and_non_finite_min_score() { |
There was a problem hiding this comment.
SUGGESTION: Validation tests miss the boundary cases most likely to regress
Currently uncovered branches: limit == 0 (line 208), whitespace-only query text (" ", the trim() branch), Some(f32::INFINITY)/NEG_INFINITY (only NAN is exercised), SemanticMemoryRecord::try_new rejecting a non-finite score, exact-boundary acceptance for MAX_LIMIT and MAX_BYTES, and the third ranked tiebreaker on provenance.source_key (line 312) which no fixture currently varies. Each is a one-line addition and each maps to a branch that is unverified today.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| fn try_from(scope: MemoryScope) -> Result<Self, Self::Error> { | ||
| match scope { | ||
| MemoryScope::Episodic => Ok(Self::Episodic), | ||
| scope => Err(SemanticMemoryError::UnsupportedScope(scope)), |
There was a problem hiding this comment.
SUGGESTION: Wildcard arm silently absorbs future MemoryScope variants
MemoryScope is a local enum without #[non_exhaustive], so listing each variant explicitly would make the compiler flag this conversion the moment a scope is added. With the catch-all, a newly added scope silently becomes a runtime UnsupportedScope error instead of a compile-time reminder to revisit the deliberately narrow F3 boundary — the opposite of what an intentionally bounded slice wants.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
|
||
| impl SemanticMemoryQuery { | ||
| /// Default number of records requested when callers do not need a wider recall. | ||
| pub const DEFAULT_LIMIT: usize = 10; |
There was a problem hiding this comment.
SUGGESTION: DEFAULT_LIMIT and DEFAULT_BYTES (line 242) are exported but unreachable through any constructor
Nothing uses them and neither type offers a default constructor, so callers are pushed into SemanticMemoryQuery::new(text, SemanticMemoryQuery::DEFAULT_LIMIT, None).unwrap() — an unwrap() on a path that is statically infallible. Consider SemanticMemoryQuery::with_defaults(text) and an infallible impl Default for SemanticMemoryBudget (both constants are in range by construction) so the default path cannot return an error at all.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| if text.trim().is_empty() { | ||
| return Err(SemanticMemoryError::EmptyQuery); | ||
| } | ||
| if limit == 0 || limit > Self::MAX_LIMIT { |
There was a problem hiding this comment.
WARNING: limit == 0 reports QueryLimitExceeded, producing a factually wrong error message
SemanticMemoryQuery::new(text, 0, None) returns QueryLimitExceeded { requested: 0, maximum: 100 }, which renders as semantic-memory query limit 0 exceeds maximum 100. That statement is untrue and will mislead whoever debugs the caller or parses the error. SemanticMemoryBudget already models this correctly with a separate InvalidBudget for zero — mirror it with an InvalidLimit variant and keep QueryLimitExceeded for limit > MAX_LIMIT only. Note no test covers limit == 0 today, so the wrong message is invisible.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| score: f32, | ||
| provenance: SemanticMemoryProvenance, | ||
| ) -> Result<Self, SemanticMemoryError> { | ||
| if !score.is_finite() { |
There was a problem hiding this comment.
WARNING: try_new validates score finiteness only — key, content size and score magnitude are unchecked
Three gaps in a constructor whose stated job is to be the safe conversion point for untrusted provider responses:
- An empty or whitespace
keyis accepted and then flows intoSemanticMemoryPort::forget(&str)and provider identity, whileSemanticMemoryQueryrejects blank text for the same reason. contenthas no ceiling, so a single record can exceedSemanticMemoryBudget::MAX_BYTESon its own — the budget type exists specifically to bound this.- A "finite" score of
1e30/-1e30passes and then breaks everymin_scorecomparison and score-based ranking downstream.
Reject blank keys and out-of-range scores here so no adapter has to re-derive the rules.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| async fn store(&self, record: SemanticMemoryRecord) -> Result<String, SemanticMemoryError>; | ||
|
|
||
| /// Recalls records matching a validated query. | ||
| async fn recall( |
There was a problem hiding this comment.
WARNING: The port contract never states (or enforces) what limit, min_score and budget obligate an implementation to do
recall takes a validated query and a hard budget, yet nothing in the domain truncates to query.limit(), filters by query.min_score(), applies budget.bytes(), or requires the returned Vec to be ranked — SemanticMemoryRecord::ranked is a free-standing helper an adapter can simply forget to call, and the trait docs do not mention it. Every future provider plus the FTS fallback will therefore re-implement these three rules slightly differently, and a single missed budget check means unbounded recalled content entering prompt context, which is the exact failure this budget type was introduced to prevent.
Add a domain-side enforcement helper (e.g. SemanticMemoryBudget::apply(records) that ranks, filters by score, truncates to the limit and cuts content past the byte budget) and state in the trait docs that implementations must return its output.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
|
||
| /// Errors produced at the provider-agnostic semantic-memory boundary. | ||
| #[derive(Debug, Clone, PartialEq, Eq, Error)] | ||
| pub enum SemanticMemoryError { |
There was a problem hiding this comment.
SUGGESTION: Consider #[non_exhaustive] on this public error enum
The PR description commits to follow-up slices for providers, transport, storage and configuration, each of which will add variants. Without #[non_exhaustive], any downstream match on SemanticMemoryError in other crates compiles today and breaks on the next variant, turning a purely additive change into a breaking one. Adding it now costs nothing because no external matcher exists yet.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| Unavailable(String), | ||
| /// The provider returned a non-success status and body. | ||
| #[error("semantic-memory backend returned status {status}: {body}")] | ||
| Backend { status: u16, body: String }, |
There was a problem hiding this comment.
WARNING: Raw provider response body is embedded verbatim into a Display error
Backend { status, body } renders the whole provider body into logs and user-facing error chains. Provider error bodies routinely echo request metadata and headers, and for a memory provider the body can also contain the recalled conversation text (user PII) or an echoed auth header. Bound and sanitize before constructing this variant — e.g. store a truncated prefix and document on the variant that adapters must redact credentials — otherwise the first HTTP adapter will pipe upstream payloads straight into tracing output.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| impl SemanticMemoryError { | ||
| /// Returns whether this error is eligible for a semantic-to-FTS fallback. | ||
| pub fn allows_fts_fallback(&self) -> bool { | ||
| matches!(self, Self::Unavailable(_)) |
There was a problem hiding this comment.
WARNING: Transient provider failures classified as Backend are not fallback-eligible
allows_fts_fallback matches Unavailable only, but the natural mapping in an HTTP adapter puts 429, 500, 502, 503, 504 and timeouts into Backend { status, .. }. Those are precisely the "provider temporarily unavailable" conditions the FTS fallback exists for, so a 503 will hard-fail recall instead of degrading gracefully — while a permanent 401 and a transient 503 are treated identically. Either classify transient statuses here (e.g. Backend { status, .. } if *status == 429 || *status >= 500) or document on Unavailable/Backend that adapters must map transport errors and 5xx/429 to Unavailable. Nothing in the contract says that today, and the test at line 116 pins only the current stricter behaviour.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 12 Issues Found | Recommendation: Address before merge — Request Changes Overview
What this PR doesAdds The design intent is good and the scope discipline is real. The blocking concerns are that the file's organization risks a Issue Details (click to expand)WARNING
SUGGESTION
Constraint checklist
Files Reviewed (3 files)
RecommendationRequest Changes. None of the findings are compilation-breaking, but the line-1 organization issue can fail the zero-warnings gate, and the boundary types should not ship advertising validation guarantees they do not enforce (lines 208, 214, 291, 350) before the first provider adapter is written against them. Auto-fix: reply Fix these issues in Kilo Cloud Previous Review Summaries (2 snapshots, latest commit a9929f2)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit a9929f2)Status: 12 Issues Found | Recommendation: Address before merge — Request Changes Overview
What this PR doesAdds The design intent is good and the scope discipline is real. The blocking concerns are that the file's organization risks a Issue Details (click to expand)WARNING
SUGGESTION
Constraint checklist
Files Reviewed (3 files)
RecommendationRequest Changes. None of the findings are compilation-breaking, but the line-1 organization issue can fail the zero-warnings gate, and the boundary types should not ship advertising validation guarantees they do not enforce (lines 208, 214, 291, 350) before the first provider adapter is written against them. Auto-fix: reply Fix these issues in Kilo Cloud Previous review (commit c4c7b5c)Status: 12 Issues Found | Recommendation: Address before merge — Request Changes Overview
What this PR doesAdds The design intent is good and the scope discipline is real. The blocking concerns are that the file's organization risks a Issue Details (click to expand)WARNING
SUGGESTION
Constraint checklist
Files Reviewed (3 files)
RecommendationRequest Changes. None of the findings are compilation-breaking, but the line-1 organization issue can fail the zero-warnings gate, and the boundary types should not ship advertising validation guarantees they do not enforce (lines 208, 214, 291, 350) before the first provider adapter is written against them. Auto-fix: reply Reviewed by free · Input: 40.7K · Output: 11.3K · Cached: 123K |
|
Dependency limit exceeded — report not shown. This pull request scan exceeded the 10,000-dependency limit applied to this scan, so the results are incomplete and may be inaccurate. To avoid reporting false positives, Socket has not posted a report. Upgrade your plan to raise the dependency limit and get complete reports, or view the partial scan in the dashboard. Socket is always free for open source. If this is a non-commercial open source project, contact us to request a free Team account. |
Co-Authored-By: ForgeCode <noreply@forgecode.dev>
Co-Authored-By: ForgeCode <noreply@forgecode.dev>
a9929f2 to
a513985
Compare
User description
Summary
F3 previously had a research and ADR surface but no provider-agnostic domain contract for semantic memory. This slice adds the deliberately narrow Episodic-only boundary required before any provider, database, network, or prompt integration can be introduced.
It exports validated query and hard-budget types, provenance-bearing records with deterministic ranking, a provider-agnostic
SemanticMemoryPort, and an error taxonomy that permits FTS fallback only for provider unavailability. Identity and project-knowledge scopes are explicitly rejected in this slice.Cargo.lockis included as a pre-existing workspace consistency repair discovered by Cargo: the lockfile lacked the already-declaredforge_e2e,forge_sandbox, andhelios-botmembers and the existingforge_app -> forge_sandboxedge. No F3 dependency was added.Validation
cargo test -p forge_domain semantic_memory --lib --locked(5 passed)cargo test -p forge_domain --lib --locked --quiet(679 passed)cargo clippy -p forge_domain --all-targets --locked -- -D warningscargo fmt --all -- --checkgit diff --checkScope
This is only the domain contract. It intentionally does not add Thegent imports, remote calls, embedding providers, storage/migrations, configuration, or orchestration prompt injection.
CodeAnt-AI Description
Add a workspace-scoped contract for episodic semantic-memory recall
What Changed
Impact
✅ Workspace-isolated memory recall✅ Bounded prompt context from recalled memories✅ Stable, provenance-aware search results💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.