fix: remediate memory engine audit findings#74
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 41 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThe PR hardens TinyCortex across configuration, storage, ingestion, retrieval, queue execution, synchronization, tree processing, public contracts, and CI. It adds transactional flows, corruption recovery, validation, collision-resistant paths, safer parsing, expanded feature testing, and extensive regression coverage. ChangesCore correctness and infrastructure
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
![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: 95454a6995
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (11)
src/memory/chunks/connection.rs (1)
263-300: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winBreaker isn't re-checked after acquiring
init_lock, risking a thundering-herd of wasted retries once cooldown expires.The connection cache is double-checked under
init_lock(Lines 275-280), but the breaker'sis_open()is only checked once, before the lock (Lines 244-253). Once the cooldown lapses, every waiting caller that already passed that earlier check will proceed to callopen_and_initin turn — even if an earlier queued attempt just re-tripped the breaker (Line 39-40 inconnection_breaker.rsextendslast_tripon each failure while tripped) while they were queued. Each such attempt pays the full connect/timeout cost against a DB that's still down, defeating the "don't busy-loop" purpose of the breaker under sustained failure.Re-check
breaker.is_open()right after acquiring_init_guard, mirroring the existing cache double-check.🔒 Proposed fix: re-check breaker after acquiring the init lock
// Re-check the cache once we hold the init lock. { let guard = conn_cache().connections.lock(); if let Some(conn) = guard.get(&db_path) { return Ok(Arc::clone(conn)); } } + + // Re-check the breaker too — a queued attempt ahead of us may have + // just re-tripped it while we were waiting for the lock. + { + let breakers = conn_cache().breakers.lock(); + if let Some(breaker) = breakers.get(&db_path) { + if breaker.is_open() { + anyhow::bail!( + "[chunks] circuit breaker open for {}: too many consecutive init failures", + db_path.display() + ); + } + } + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/memory/chunks/connection.rs` around lines 263 - 300, After acquiring _init_guard in the connection initialization flow, re-check the relevant breaker’s is_open() before the cache re-check or any open_and_init call. Return the same breaker-open result used by the earlier pre-lock check, while preserving the existing cache double-check and retry behavior for closed breakers.src/memory/sync/dispatcher.rs (1)
122-328: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winTests are mixed into the implementation file instead of a sibling
dispatcher_tests.rs.This PR extends the existing inline
mod tests { ... }block indispatcher.rswith new coverage (init-failure propagation, blank-id rejection). As per coding guidelines, tests should live in a per-file sibling such asdispatcher_tests.rsrather than mixed into the implementation file. Moving this module out keepsdispatcher.rsfocused and gives headroom against the 500-line file-size guideline as coverage keeps growing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/memory/sync/dispatcher.rs` around lines 122 - 328, Move the inline `mod tests` block from `dispatcher.rs` into a sibling `dispatcher_tests.rs` module, preserving all existing test helpers and cases including `FakePipeline`, `NoopHost`, and the three test functions. Wire the external test module using the project’s established module declaration pattern, and keep implementation code in `dispatcher.rs` unchanged.Source: Coding guidelines
src/memory/tool_memory/store.rs (2)
180-225: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
rules_for_promptdoc still describes the old hard-truncate/drop behavior.Lines 182 and 186-189 say the result is "bounded by
TOOL_MEMORY_PROMPT_CAPentries total" and that excess Critical rules are "silently dropped ... there is no overflow signal." The new logic at lines 221-225 computescritical_countand truncates toTOOL_MEMORY_PROMPT_CAP.max(critical_count), which guarantees every Critical rule survives — the opposite of what's documented. Please update the docstring to state that the cap only bounds High-priority overflow and that Critical rules are never dropped (and unbounded, which may be worth calling out as a caveat given Critical rules can originate from less-trusted capture paths per the docstring a few lines above).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/memory/tool_memory/store.rs` around lines 180 - 225, Update the documentation for rules_for_prompt to match the critical_count and truncate behavior: state that all Critical rules are retained and never dropped, while TOOL_MEMORY_PROMPT_CAP only bounds the retained High-priority rules. Remove the outdated hard-cap and silent Critical-drop claims, and mention that the result can exceed the cap when Critical rules are numerous.
68-104: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the stale
put_ruleandrules_for_promptdocs.
- The
put_ruleNOTE about verbatimtool_nameand newline-forgery is out of date:tool_nameis lower-cased before storage, and the prompt renderer no longer treats embedded newlines as a section break.- The
rules_for_promptcomment still says excess Critical rules are silently dropped, but the new truncation logic keeps all Critical entries and only limits the non-critical remainder.- If you keep the lower-casing note, call out that older mixed-case rows remain unchanged until rewritten.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/memory/tool_memory/store.rs` around lines 68 - 104, The documentation around put_rule is stale: remove or revise the claims that tool_name is stored verbatim and that embedded newlines can forge prompt sections, and if retaining the casing note, mention that only newly written values are lower-cased while older mixed-case rows persist until rewritten. Update the rules_for_prompt documentation to state that truncation preserves all Critical rules and limits only the non-critical remainder.src/memory/score/mod.rs (1)
371-398: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMake
persist_scoreatomic across score and entity-index writespersist_scorestill commitsupsert_score,clear_entity_index_for_node, andindex_entitiesseparately, so a failure in the clear/reindex path can leave the score row and entity index out of sync. Use one transaction for the whole sequence, or route callers throughpersist_score_txwhen atomicity matters.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/memory/score/mod.rs` around lines 371 - 398, Make persist_score atomic by executing upsert_score, clear_entity_index_for_node, and the conditional index_entities operation within one database transaction. Reuse the existing persist_score_tx transaction path if available, ensuring any failure rolls back both the score row and entity-index changes together.src/memory/conversations/store_index.rs (1)
158-197: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy liftAvoid rescanning every thread file on each list/summary call
list_threads_unlockednow re-reads and deserializes every thread’s JSONL file on every call underCONVERSATION_STORE_LOCK, so a normal thread list becomes O(total messages) and one unreadable thread file fails the whole request.thread_summary_unlockeddoes the same per-thread reread on its path.Gating this to cold/recovery cases or memoizing an “already reconciled” marker per workspace would avoid turning these hot reads into workspace-wide scans.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/memory/conversations/store_index.rs` around lines 158 - 197, Change list_threads_unlocked and the corresponding thread_summary_unlocked path so normal reads do not call measure_messages_unlocked for every thread. Restrict message-file reconciliation to cold-start/recovery cases or reuse a workspace-level marker indicating reconciliation has already completed, while preserving repair behavior when reconciliation is required and preventing one unreadable thread file from failing routine list/summary requests.src/memory/queue/store_settle.rs (1)
1-1: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
mark_done_with_followupsdoesn't report whether it actually committed;worker.rsacts onfollow_upsas if it always did.
mark_done_with_followupsreturnsOk(())even when the optimistic claim-token check fails (updated == 0), silently skipping the follow-up inserts in that case.settle_planned_job's Done branch inworker.rscomputesarms_reembedfrom thefollow_upsslice and unconditionally callsset_backfill_in_progress(true)— it never checks whether the follow-up was actually persisted. Under a stale-claim race (lease already reclaimed elsewhere), theReembedBackfillfollow-up is silently dropped, yet the global flag is still set totruewith no corresponding queue row ever able to clear it, leavingbackfill_in_progressstuck. Contrast with the same function's own seal re-arm logic and the Err-branch's flag-clearing, both of which correctly re-verify viaget_jobbefore mutating state.
src/memory/queue/store_settle.rs#L32-62: changemark_done_with_followupsto returnResult<bool>(whether the row update actually matched), so callers can distinguish a real commit from a stale-claim no-op.src/memory/queue/worker.rs#L136-148: gateset_backfill_in_progress(true)on that returned success flag, mirroring the guard already used in the seal re-arm and Err-branch cleanup.🔧 Suggested fix
pub(crate) fn mark_done_with_followups( config: &MemoryConfig, job: &Job, follow_ups: &[NewJob], -) -> Result<()> { +) -> Result<bool> { let job_id = &job.id; let claim_attempts = job.attempts as i64; let claim_started_at = job.started_at_ms; with_connection(config, |conn| { let now_ms = Utc::now().timestamp_millis(); let tx = conn.unchecked_transaction()?; let updated = tx.execute( "UPDATE mem_tree_jobs ...", params![now_ms, job_id, claim_attempts, claim_started_at], )?; if updated != 0 { for follow_up in follow_ups { crate::memory::queue::store::enqueue_tx(&tx, follow_up)?; } } tx.commit()?; - Ok(()) + Ok(updated != 0) }) } pub fn mark_done(config: &MemoryConfig, job: &Job) -> Result<()> { - mark_done_with_followups(config, job, &[]) + mark_done_with_followups(config, job, &[]).map(|_| ()) }- mark_done_with_followups(config, job, &follow_ups)?; - if arms_reembed { + let committed = mark_done_with_followups(config, job, &follow_ups)?; + if committed && arms_reembed { set_backfill_in_progress(true); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/memory/queue/store_settle.rs` at line 1, Update mark_done_with_followups to return Result<bool>, with the boolean indicating whether the claim-token update matched a row; return false for stale-claim no-ops. In settle_planned_job’s Done branch, capture this result and only call set_backfill_in_progress(true) when the settlement succeeded, preventing follow-up state from being updated after an uncommitted completion.src/memory/store/vectors/store.rs (2)
198-218: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStale doc on
insert_with_vector— dimension check is now enforced.The doc says the vector's length "is never checked against the store's configured dimensions" and a wrong-dimension vector is "stored as-is" — but the
anyhow::ensure!added directly below now rejects mismatched lengths at insert time. Update the doc to describe the new fail-fast behavior instead of the old silent-corruption behavior.📝 Proposed doc fix
- /// NOTE: the vector's length is never checked against the store's - /// configured dimensions (`store_meta.embed_dims`). A wrong-dimension - /// vector is stored as-is; [`cosine_similarity`] then returns `0.0` for - /// any query against it (length mismatch), so the row silently never - /// surfaces in search results rather than erroring at insert time. + /// The vector's length is validated against the store's configured + /// dimensions (`store_meta.embed_dims`); a mismatch is rejected here + /// rather than being silently stored and later dropped from search + /// results.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/memory/store/vectors/store.rs` around lines 198 - 218, Update the documentation for insert_with_vector to reflect that it validates embedding.len() against self.backend.dimensions() and rejects mismatched vectors immediately with an error. Remove the outdated description of storing invalid vectors and silently excluding them from search results.
88-91: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStale "known gaps" documentation — the gaps described are already fixed by this diff.
The doc says the
embed_dimsread still "collapses toNonevia.ok()" and the dims string is still "parsed with.parse().unwrap_or(0)" — but the code below now uses.optional()?(Line 147) and.parse().with_context(...)?(Lines 164-168), which is exactly the fix for both described gaps. The same stale wording also appears inopen()'s doc (Lines 88-91). Leaving this uncorrected risks misleading future readers into thinking the fail-closed guard is still broken.📝 Proposed doc fix
- /// NOTE: two related gaps, both currently present: - /// - A failed read of the `embed_dims` row (e.g. a transient I/O error) - /// collapses to `None` via `.ok()` and is treated identically to a - /// genuinely first-ever open, so it silently *overwrites* - /// `embed_provider`/`embed_dims` with the current runtime backend - /// instead of surfacing the read failure. - /// - The stored dims string is parsed with `.parse().unwrap_or(0)`; a - /// corrupted value collapses to `0`, and the mismatch check is skipped - /// whenever either side is `0` — so a corrupt row disables the guard - /// entirely instead of failing closed. + /// A failed read of the `embed_dims` row now propagates as an error + /// (via `.optional()?`) instead of being treated as first-open, and an + /// unparsable stored dims string now fails closed via + /// `.parse().with_context(...)?` rather than silently defaulting to `0`.Also update the corresponding note in
open()'s doc comment (Lines 88-91).Also applies to: 124-176
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/memory/store/vectors/store.rs` around lines 88 - 91, Update the documentation for open() and the related check_or_store_meta helper to remove references to the already-fixed gaps involving .ok() collapsing errors and .parse().unwrap_or(0). Describe the current fail-closed behavior accurately, without changing the implementation.src/memory/conversations/inverted_index.rs (1)
337-346: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winStale doc:
candidates_for_term's doc comment no longer matches theNone-branch behavior.This doc block still says the
Nonecase "routinely exceedsLARGE_CANDIDATE_LIMITand trips the recency-only fallback," but per the updatedsearchlogic (lines 250-267), the pathological cap now only applies inside theSome(v)branch — theNonebranch always does an uncapped full-corpus scan and never triggersrecency_fallback. The doc should be updated to describe the current, intentional behavior (avoiding fabricated score-0.0 hits for short/CJK terms at the cost of an uncapped scan) instead of the old one.As per coding guidelines, "Document public APIs, module contracts, and non-obvious behavior thoroughly, preferring module-level docs and item docs."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/memory/conversations/inverted_index.rs` around lines 337 - 346, Update the doc comment for candidates_for_term to match the current search behavior: explain that the None branch intentionally performs an uncapped full-corpus scan and does not trigger recency_fallback, avoiding fabricated score-0.0 hits for short or single-CJK terms at the cost of scanning all live documents. Remove the obsolete LARGE_CANDIDATE_LIMIT and recency-only fallback claim while preserving the existing explanation of why this branch occurs.Source: Coding guidelines
src/memory/chunks/store.rs (1)
73-94: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftReclaim the previous staged blob on plain upsert
src/memory/chunks/store.rs:73-94clearscontent_path/content_sha256, but it never removes the file referenced by the oldcontent_path.store_delete.rsonly reclaims files on explicit deletes, so a staged→plain transition can strand the staged blob on disk.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/memory/chunks/store.rs` around lines 73 - 94, The UPSERT_SQL path clears the previous content_path without reclaiming its staged blob. Update the plain upsert flow around UPSERT_SQL to capture and remove the existing content_path file before or as part of replacing the row, while preserving the current reset of content_path and content_sha256; reuse the existing blob-reclamation mechanism from the store deletion flow rather than introducing separate cleanup behavior.
🧹 Nitpick comments (17)
src/memory/store/safety/pii/checks.rs (1)
1-232: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
src/memory/store/safety/pii/checks_tests.rsfor the checksum validators.pii_tests.rsonly covers a couple of cases indirectly; these CPF/CNPJ/CUIT/Luhn/IBAN/Verhoeff/SSN/NINO rules deserve direct edge-case tests beside the module.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/memory/store/safety/pii/checks.rs` around lines 1 - 232, Add a sibling checks_tests.rs module containing direct edge-case tests for valid_cpf, valid_cnpj, valid_cuit, valid_luhn, valid_iban, valid_verhoeff, valid_ssn, valid_dni_es, valid_nie_es, and valid_nino. Cover representative valid inputs plus invalid lengths, checksums, reserved values, prefixes, and boundary rules, and structure the tests so the private validators are exercised through the module’s test setup.Source: Coding guidelines
src/memory/store/vectors/store_tests_more.rs (1)
31-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReduce schema coupling in the corrupt-metadata test.
The raw INSERT re-specifies every column of
vectors. A normal insert followed by anUPDATE ... SET metadata = 'not-json'would isolate the test to only the behavior under test (invalid metadata handling) and survive future schema changes to unrelated columns.♻️ Proposed simplification
fn search_handles_invalid_metadata_json() { let store = fake_store(2); + store + .insert_with_vector("bad", "ns", "text", &[1.0, 0.0], json!({})) + .unwrap(); { let conn = store.conn.lock(); conn.execute( - "INSERT INTO vectors (id, namespace, text, embedding, metadata, created_at, updated_at) - VALUES ('bad', 'ns', 'text', ?1, 'not-json', 0.0, 0.0)", - rusqlite::params![vec_to_bytes(&[1.0, 0.0])], + "UPDATE vectors SET metadata = 'not-json' WHERE id = 'bad'", + [], ) .unwrap(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/memory/store/vectors/store_tests_more.rs` around lines 31 - 45, Update search_handles_invalid_metadata_json to create the vector using the store’s normal insertion path, then execute a targeted UPDATE that sets only its metadata to invalid JSON. Remove the raw INSERT’s unrelated column values while preserving the existing search and null-metadata assertions.src/memory/sources/registry.rs (1)
183-185: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDeduplicate the lock-acquisition boilerplate.
All seven mutating methods repeat the identical
REGISTRY_MUTATION_LOCK.lock().expect("source registry mutation lock poisoned")line. Extracting a small private helper centralizes the panic message and reduces the chance a future mutating method is added without acquiring the lock.♻️ Proposed helper
+ fn mutation_guard(&self) -> std::sync::MutexGuard<'static, ()> { + REGISTRY_MUTATION_LOCK + .lock() + .expect("source registry mutation lock poisoned") + } + pub fn add(&self, entry: MemorySourceEntry) -> Result<MemorySourceEntry> { - let _guard = REGISTRY_MUTATION_LOCK - .lock() - .expect("source registry mutation lock poisoned"); + let _guard = self.mutation_guard();(repeat for
update,remove,remove_composio_source_by_connection_id,upsert_composio_source,upsert_composio_sources_batch,apply_all_in)Also applies to: 199-208, 218-220, 235-237, 261-263, 276-278, 289-291
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/memory/sources/registry.rs` around lines 183 - 185, Extract a private lock-acquisition helper for REGISTRY_MUTATION_LOCK that performs the existing lock call and panic message, then replace the repeated boilerplate in all mutating methods, including update, remove, remove_composio_source_by_connection_id, upsert_composio_source, upsert_composio_sources_batch, apply_all_in, and the method shown. Keep each method’s guard held for its full mutation scope.src/memory/chunks/connection_breaker.rs (2)
11-49: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing item-level docs on public breaker API.
new,record_success,record_failure, andis_openhave no doc comments describing their contracts (e.g., what theboolreturn values mean, cooldown semantics). Only the module header documentsCircuitBreakerat a high level.As per path instructions, "Document public APIs, module contracts, and non-obvious behavior thoroughly, preferring module-level docs and item docs."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/memory/chunks/connection_breaker.rs` around lines 11 - 49, Add Rust doc comments to CircuitBreaker::new, record_success, record_failure, and is_open describing initialization, the meaning of each bool return value, failure-threshold behavior, and cooldown semantics. Keep the implementation unchanged and align the item documentation with the existing module-level CircuitBreaker contract.Source: Path instructions
32-49: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNarrow TOCTOU window between
trippedandlast_trip.
record_failureflipstrippedtotrue(Line 34) before writinglast_trip(Line 35). A concurrentis_open()call in that gap seestripped == truebutlast_tripstillNone, somatches!fails theSome(..)pattern andis_open()wrongly reportsfalse, letting a caller slip past the breaker mid-trip. Self-correcting on the next failure, but the state isn't updated as one atomic unit.Consider consolidating
tripped,consecutive_failures, andlast_tripinto a singleMutex-guarded struct so trip transitions are observed atomically.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/memory/chunks/connection_breaker.rs` around lines 32 - 49, Make the breaker state transition atomic by consolidating tripped, consecutive_failures, and last_trip into one Mutex-guarded state structure. Update record_failure and is_open to access and mutate this shared state under the same lock, ensuring is_open cannot observe tripped without its corresponding trip timestamp.src/memory/sources/readers/folder.rs (1)
117-125: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
DEFAULT_GLOBinstead of duplicating the literal.
list_itemsderives its default via theDEFAULT_GLOBconstant (Line 50);read_itemhardcodes the same string literal instead. Values match today, but this duplication risks the exact list/read glob divergence this change is meant to close ifDEFAULT_GLOBis ever updated in one place only.♻️ Proposed fix
- let pattern = source.glob.as_deref().unwrap_or("**/*.md"); + let pattern = source.glob.as_deref().unwrap_or(DEFAULT_GLOB);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/memory/sources/readers/folder.rs` around lines 117 - 125, Update the default pattern used in read_item to reference the existing DEFAULT_GLOB constant instead of hardcoding "**/*.md". Keep the explicit source.glob value and subsequent glob matching behavior unchanged.src/memory/sync/composio/providers/slack_parse.rs (1)
10-81: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd item docs for the extracted parsing helpers.
These
pub(super)functions encode several non-obvious JSON-pointer fallback paths (cursor/pagination/mention/timestamp parsing) now shared across Slack sync code paths, but none carry item-level doc comments.As per path instructions, "Document public APIs, module contracts, and non-obvious behavior thoroughly, preferring module-level docs and item docs."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/memory/sync/composio/providers/slack_parse.rs` around lines 10 - 81, Add item-level documentation comments to each pub(super) helper: mention_regex, replace_mentions, next_cursor, search_matches, search_total_pages, decode_cursors, and parse_ts. Document their purpose and the non-obvious fallback paths, mention resolution behavior, cursor defaults, and timestamp parsing contract without changing implementation behavior.Source: Path instructions
src/memory/score/store_query.rs (1)
1-92: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a dedicated
store_query_tests.rssibling.This new
store_query.rsmodule's functions are currently tested fromstore_tests.rsrather than a co-locatedstore_query_tests.rs. As per path instructions,src/**/*_tests.rs: "Add focused unit tests beside the module undersrc/." Transactional cross-module tests can stay instore_tests.rs, but plain diagnostics-query tests (e.g.lookup_entity_in_window,list_entity_ids_for_node) would fit the convention better in a sibling file.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/memory/score/store_query.rs` around lines 1 - 92, Move focused unit tests for the diagnostics-query functions in store_query, including lookup_entity_in_window and list_entity_ids_for_node, from store_tests.rs into a co-located store_query_tests.rs sibling under src. Keep transactional or cross-module integration tests in store_tests.rs, and preserve the existing test coverage and behavior.Source: Path instructions
src/memory/queue/store_settle.rs (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShare the defer-age default instead of duplicating it.
src/memory/queue/store_settle.rsandsrc/memory/config/policy.rsboth hardcode the same 7-daymax_defer_age_ms, andsrc/memory/queue/store_settle_tests.rsbackdates against the test-only copy. Pull the test bound fromMemoryConfig::default()(or a shared const) so the boundary can’t drift.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/memory/queue/store_settle.rs` at line 1, Share the 7-day max_defer_age_ms default instead of duplicating it across settlement and policy code. Update the defer-age boundary in store_settle.rs and store_settle_tests.rs to reuse MemoryConfig::default() or a shared constant from policy.rs, while preserving the existing settlement behavior and test boundary.src/memory/diff/ledger_helpers.rs (1)
13-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd doc comments to the contract-defining helpers.
checkpoint_message/checkpoint_from_messagedefine the JSON wire schema for checkpoint tag messages, andvalidate_source_id/build_commit_messageencode invariants (blank/control-char rejection, trailer layout) relied on elsewhere — but none of these have doc comments, unlike sibling helpers in the same file (parse_trailers,sanitize_trailer,encode_item_id, etc.).As per path instructions,
src/**/*.rsshould "Document public APIs, module contracts, and non-obvious behavior thoroughly, preferring module-level docs and item docs."Also applies to: 22-42, 69-76, 78-122
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/memory/diff/ledger_helpers.rs` around lines 13 - 16, Add Rust doc comments to checkpoint_message, checkpoint_from_message, validate_source_id, and build_commit_message documenting their wire-schema or validation/trailer-layout contracts, including relevant rejection and formatting behavior. Follow the existing documentation style used by parse_trailers, sanitize_trailer, and encode_item_id; do not change implementation behavior.Source: Path instructions
src/memory/config/policy.rs (1)
52-78: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a dedicated
policy_tests.rsfor these validators.
RetrievalLimits::validate,ScoringPolicyConfig::validate, andQueueConfig::validateare only exercised indirectly today, throughMemoryConfig::validate()inconfig_tests.rs. Per the path guideline forsrc/**/*.rs, tests should live in a<name>_tests.rssibling of the module under test.As per path instructions, "Keep tests in per-file
<name>_tests.rssiblings, such asstore.rsandstore_tests.rs, rather than mixing tests into implementation files."Also applies to: 106-119, 153-166
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/memory/config/policy.rs` around lines 52 - 78, Move direct validator coverage for RetrievalLimits::validate, ScoringPolicyConfig::validate, and QueueConfig::validate out of config_tests.rs into a dedicated policy_tests.rs sibling of the policy module. Add focused tests for each validator’s accepted and rejected bounds while retaining MemoryConfig::validate integration coverage where appropriate.Source: Path instructions
src/memory/store/entity_index/transaction.rs (1)
10-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDoc comment omits the non-obvious identity-preserving behavior.
index_entities_txsilently usesNoSelfIdentity(alwaysfalse) plusUPSERT_PRESERVE_USER_SQL, meaning on conflict the existingis_uservalue is preserved rather than being reset tofalse, but a brand-new row would be inserted withis_user = false. This distinction (preserve-on-conflict vs. insert-as-non-user) is exactly the kind of non-obvious behavior worth calling out inline so callers don't assumeNoSelfIdentitysimply "skips" identity classification. As per coding guidelines, "Document public APIs, module contracts, and non-obvious behavior thoroughly, preferring module-level docs and item docs."📝 Proposed doc clarification
-/// Index canonical entities inside the caller's transaction. +/// Index canonical entities inside the caller's transaction, without identity +/// classification. Existing rows keep their prior `is_user` flag on conflict +/// (via `UPSERT_PRESERVE_USER_SQL`); newly inserted rows are marked non-user. pub fn index_entities_tx(🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/memory/store/entity_index/transaction.rs` around lines 10 - 29, Expand the doc comment for the public function index_entities_tx to document its identity behavior: new rows are inserted with is_user = false, while conflicts preserve the existing is_user value via UPSERT_PRESERVE_USER_SQL; clarify that NoSelfIdentity does not merely skip identity classification.Source: Coding guidelines
src/memory/store/content/atomic.rs (1)
40-69: 🗄️ Data Integrity & Integration | 🔵 TrivialVerify workspace filesystem supports hard links.
The publish path now requires hard-link support; filesystems without it (e.g., FAT/exFAT) will make every
write_if_newpublish fail. Worth confirming the workspace is never expected to live on such a filesystem, sincerename(still portable) no longer backs this path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/memory/store/content/atomic.rs` around lines 40 - 69, Verify and enforce that the workspace filesystem supports hard links before relying on std::fs::hard_link in the publish path used by write_if_new. Add an appropriate capability check or documented validation at workspace initialization, and ensure unsupported filesystems produce a clear error rather than causing unexplained publish failures.src/memory/archivist/store.rs (1)
44-67: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared "sanitize name with collision-resistant digest suffix" helper. Both sites independently reimplement the same char-replace + conditional SHA-256-suffix pattern, and they've already diverged on the empty-input guard, risking further silent divergence as either evolves.
src/memory/archivist/store.rs#L44-L67: extractsanitize_session's logic (char whitelist + digest suffix + empty guard) into a shared helper (e.g. insrc/memory/fsutil.rs) parameterized by the allowed-character predicate.src/memory/tree/runtime/store/paths.rs#L28-L44: replacesanitizewith a call to the same shared helper, picking up the missing!sanitized.is_empty()guard in the process so an empty-after-trim namespace also gets a digest suffix rather than collapsing to"".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/memory/archivist/store.rs` around lines 44 - 67, The session and namespace sanitizers duplicate collision-resistant digest-suffix logic and diverge on empty results. In src/memory/archivist/store.rs:44-67, extract sanitize_session’s character replacement, empty guard, and SHA-256 suffix behavior into a shared helper in src/memory/fsutil.rs parameterized by the allowed-character predicate; in src/memory/tree/runtime/store/paths.rs:28-44, replace sanitize with that helper so empty-after-trim values also receive a digest suffix.src/memory/store/kv.rs (1)
229-257: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared JSON-parsing helper for the three row-iteration methods.
list_namespace,records_namespace, andrecords_globalnow all parsevalue_jsonand format an identical-shaped error (only the function name differs) on failure. Consider extracting a small private helper to avoid three copies of the same pattern.♻️ Proposed helper extraction
+ fn parse_stored_value(raw: &str, key: &str, context: &str) -> Result<Value, String> { + serde_json::from_str(raw).map_err(|e| format!("{context} JSON for key '{key}': {e}")) + } + pub fn list_namespace(&self, namespace: &str) -> Result<Vec<Value>, String> { ... - let value = serde_json::from_str::<Value>(&value_raw) - .map_err(|e| format!("list_namespace JSON for key '{key}': {e}"))?; + let value = Self::parse_stored_value(&value_raw, &key, "list_namespace")?;Apply analogously to
records_namespaceandrecords_global.Also applies to: 274-304, 308-334
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/memory/store/kv.rs` around lines 229 - 257, Extract the repeated value_json deserialization and contextual error formatting from list_namespace, records_namespace, and records_global into a shared private helper. Have each row-iteration method call the helper with the raw JSON and its function context, preserving the existing error messages and successful Value results.src/memory/store/store.rs (1)
83-127: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTerm-matching scoring logic duplicated between
MemoryStore::searchandMemory::recall. Both independently tokenize the query into lowercase whitespace-separated terms, count substring matches against lowercased content, and computematched/total(returning1.0/Nonefor empty query / zero matches) — same algorithm, two copies, drifting types (f32vsf64).
src/memory/store/store.rs#L83-L127: extract the term-tokenize + score computation into a shared helper (e.g. in a small scoring module) usable by bothsearchandrecall.src/memory/store/memory_trait.rs#L45-L59: replacequery_scorewith a call to the shared helper instead of re-implementing the same matching logic.src/memory/store/memory_trait.rs#L134-L179: updaterecall's call site to use the shared helper's result instead of the localquery_score.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/memory/store/store.rs` around lines 83 - 127, Extract the duplicated lowercase whitespace-term tokenization and matched/total scoring into a shared helper, using one consistent score type and preserving 1.0 for empty queries and no result for zero matches. In src/memory/store/store.rs lines 83-127, update MemoryStore::search to use the helper; in src/memory/store/memory_trait.rs lines 45-59, replace query_score with the helper; and in src/memory/store/memory_trait.rs lines 134-179, update recall to consume the helper’s result.src/memory/chunks/store.rs (1)
88-94: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a named constant instead of the bare
'admitted'literal.
CHUNK_STATUS_DROPPEDis already a named constant reused across modules (e.g.store_list.rs); hardcoding'admitted'directly in this SQL string risks silent drift/typos in a lifecycle-critical status value.♻️ Suggested fix
- created_at_ms = excluded.created_at_ms, - content_path = NULL, - content_sha256 = NULL, - lifecycle_status = 'admitted'"; + created_at_ms = excluded.created_at_ms, + content_path = NULL, + content_sha256 = NULL, + lifecycle_status = '" + CHUNK_STATUS_ADMITTED + "'"; // build via format!/const concat🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/memory/chunks/store.rs` around lines 88 - 94, Replace the hardcoded 'admitted' lifecycle status in the SQL assignment within the chunk store upsert flow with the existing named status constant, reusing the same symbol as CHUNK_STATUS_DROPPED and its usage in store_list.rs. Keep the SQL behavior unchanged while preventing the status value from being duplicated as a string literal.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/memory/chunks/migrations.rs`:
- Around line 166-168: Reorder the global/topic purge migration so filesystem
cleanup of wiki/summaries/global* and topic-* occurs before updating
user_version and committing the transaction. Ensure startup retries that cleanup
when it was interrupted, rather than skipping it solely because the migration
version is already current; apply the same fix to the corresponding second
completion path.
In `@src/memory/chunks/store_list.rs`:
- Around line 92-114: Update the allowlist SQL built in the surrounding
chunk-filtering logic so the prefixed source comparison is case-sensitive,
replacing the SQLite LIKE-based condition while preserving the existing
escaped-prefix matching and exact source_id equality checks. Anchor the change
to the SQL fragment containing source_id LIKE and keep the allowed-source
iteration and bindings behavior unchanged.
In `@src/memory/conversations/bus.rs`:
- Around line 352-375: Update persisted_channel_thread_id and its read path to
preserve compatibility with existing underscore-containing thread IDs: first
look up the legacy unsuffixed base key, then use the hashed key for new or
non-legacy entries, or provide an equivalent migration. Ensure future turns
append to an existing legacy conversation rather than creating a new one, while
retaining collision-safe IDs for newly persisted conversations.
In `@src/memory/queue/runtime/mod.rs`:
- Around line 11-14: Update the module documentation bullet for run_worker to
state that corruption is handled by recover_corrupt_db and the worker continues
polling, replacing the inaccurate claim that corruption is fatal and returned to
the host.
In `@src/memory/queue/worker_tests.rs`:
- Around line 114-133: Serialize
terminal_reembed_failure_clears_process_backfill_flag with the other queue tests
that access the process-wide backfill_in_progress AtomicBool. Reuse the existing
test synchronization mechanism rather than adding a new one, ensuring the flag
is not concurrently modified during this test.
In `@src/memory/retrieval/global.rs`:
- Around line 123-141: Update the documentation comment above resolve_topic_hits
to remove the claim that dropped leaf chunks can surface and state that chunks
are filtered using the fetched dropped_chunk_ids tombstone set. Keep the
existing notes about soft-deleted summaries and stale index entries accurate.
- Around line 78-81: Correct the duplicated “the” in the documentation comment
describing the entity-index timestamp window and configured topic lookup cap,
leaving the surrounding wording unchanged.
In `@src/memory/retrieval/source.rs`:
- Around line 64-68: Update the time_window_days mapping in the retrieval flow
to avoid unchecked subtraction for oversized u32 values. Use checked_sub_signed
(or the existing documented maximum, if one is defined) and propagate an
appropriate error through the function’s Result instead of allowing the
calculation to panic; preserve the current timestamp window behavior for valid
durations.
In `@src/memory/sources/types.rs`:
- Around line 233-271: Update MemorySourcePatch::validate_for_kind to reject
since_days unless kind is SourceKind::TwitterQuery and reject max_items unless
kind is SourceKind::RssFeed, using the existing reject helper and preserving all
current validations.
In `@src/memory/store/vectors/store.rs`:
- Around line 440-455: Update bytes_to_vec to replace usize::is_multiple_of with
the MSRV-compatible remainder check, preserving the existing error behavior for
byte lengths not divisible by four; do not change the decoding logic or require
a Rust version bump.
In `@src/memory/tree/document_seal.rs`:
- Around line 86-104: Update both document-seal transaction paths around
with_connection to query and require the tree status to be "active" before
calling store::update_tree_after_seal_tx. Apply the same archived-tree guard to
the transaction near the existing current_root/current_max check and the second
transaction near lines 265-296, preserving the existing commit and update
behavior for active trees.
In `@src/memory/tree/summarise.rs`:
- Around line 99-107: Update the configuration validation in the relevant
config-validation function in config.rs to require that output_token_budget plus
summary_overhead_reserve_tokens remains below input_token_budget, preserving
positive input headroom for per_input_cap. Keep the existing overhead-reserve
validation and ensure invalid configurations are rejected before summarisation
reaches prepare_summary_prompt.
In `@src/memory/types.rs`:
- Around line 129-178: Update MemoryCategory::from_str to retain the current
built-in and custom: parsing while also treating non-empty unrecognized bare
strings as MemoryCategory::Custom, preserving compatibility with legacy
serialized values. Keep Display and Serialize emitting the new custom:{name}
wire format.
---
Outside diff comments:
In `@src/memory/chunks/connection.rs`:
- Around line 263-300: After acquiring _init_guard in the connection
initialization flow, re-check the relevant breaker’s is_open() before the cache
re-check or any open_and_init call. Return the same breaker-open result used by
the earlier pre-lock check, while preserving the existing cache double-check and
retry behavior for closed breakers.
In `@src/memory/chunks/store.rs`:
- Around line 73-94: The UPSERT_SQL path clears the previous content_path
without reclaiming its staged blob. Update the plain upsert flow around
UPSERT_SQL to capture and remove the existing content_path file before or as
part of replacing the row, while preserving the current reset of content_path
and content_sha256; reuse the existing blob-reclamation mechanism from the store
deletion flow rather than introducing separate cleanup behavior.
In `@src/memory/conversations/inverted_index.rs`:
- Around line 337-346: Update the doc comment for candidates_for_term to match
the current search behavior: explain that the None branch intentionally performs
an uncapped full-corpus scan and does not trigger recency_fallback, avoiding
fabricated score-0.0 hits for short or single-CJK terms at the cost of scanning
all live documents. Remove the obsolete LARGE_CANDIDATE_LIMIT and recency-only
fallback claim while preserving the existing explanation of why this branch
occurs.
In `@src/memory/conversations/store_index.rs`:
- Around line 158-197: Change list_threads_unlocked and the corresponding
thread_summary_unlocked path so normal reads do not call
measure_messages_unlocked for every thread. Restrict message-file reconciliation
to cold-start/recovery cases or reuse a workspace-level marker indicating
reconciliation has already completed, while preserving repair behavior when
reconciliation is required and preventing one unreadable thread file from
failing routine list/summary requests.
In `@src/memory/queue/store_settle.rs`:
- Line 1: Update mark_done_with_followups to return Result<bool>, with the
boolean indicating whether the claim-token update matched a row; return false
for stale-claim no-ops. In settle_planned_job’s Done branch, capture this result
and only call set_backfill_in_progress(true) when the settlement succeeded,
preventing follow-up state from being updated after an uncommitted completion.
In `@src/memory/score/mod.rs`:
- Around line 371-398: Make persist_score atomic by executing upsert_score,
clear_entity_index_for_node, and the conditional index_entities operation within
one database transaction. Reuse the existing persist_score_tx transaction path
if available, ensuring any failure rolls back both the score row and
entity-index changes together.
In `@src/memory/store/vectors/store.rs`:
- Around line 198-218: Update the documentation for insert_with_vector to
reflect that it validates embedding.len() against self.backend.dimensions() and
rejects mismatched vectors immediately with an error. Remove the outdated
description of storing invalid vectors and silently excluding them from search
results.
- Around line 88-91: Update the documentation for open() and the related
check_or_store_meta helper to remove references to the already-fixed gaps
involving .ok() collapsing errors and .parse().unwrap_or(0). Describe the
current fail-closed behavior accurately, without changing the implementation.
In `@src/memory/sync/dispatcher.rs`:
- Around line 122-328: Move the inline `mod tests` block from `dispatcher.rs`
into a sibling `dispatcher_tests.rs` module, preserving all existing test
helpers and cases including `FakePipeline`, `NoopHost`, and the three test
functions. Wire the external test module using the project’s established module
declaration pattern, and keep implementation code in `dispatcher.rs` unchanged.
In `@src/memory/tool_memory/store.rs`:
- Around line 180-225: Update the documentation for rules_for_prompt to match
the critical_count and truncate behavior: state that all Critical rules are
retained and never dropped, while TOOL_MEMORY_PROMPT_CAP only bounds the
retained High-priority rules. Remove the outdated hard-cap and silent
Critical-drop claims, and mention that the result can exceed the cap when
Critical rules are numerous.
- Around line 68-104: The documentation around put_rule is stale: remove or
revise the claims that tool_name is stored verbatim and that embedded newlines
can forge prompt sections, and if retaining the casing note, mention that only
newly written values are lower-cased while older mixed-case rows persist until
rewritten. Update the rules_for_prompt documentation to state that truncation
preserves all Critical rules and limits only the non-critical remainder.
---
Nitpick comments:
In `@src/memory/archivist/store.rs`:
- Around line 44-67: The session and namespace sanitizers duplicate
collision-resistant digest-suffix logic and diverge on empty results. In
src/memory/archivist/store.rs:44-67, extract sanitize_session’s character
replacement, empty guard, and SHA-256 suffix behavior into a shared helper in
src/memory/fsutil.rs parameterized by the allowed-character predicate; in
src/memory/tree/runtime/store/paths.rs:28-44, replace sanitize with that helper
so empty-after-trim values also receive a digest suffix.
In `@src/memory/chunks/connection_breaker.rs`:
- Around line 11-49: Add Rust doc comments to CircuitBreaker::new,
record_success, record_failure, and is_open describing initialization, the
meaning of each bool return value, failure-threshold behavior, and cooldown
semantics. Keep the implementation unchanged and align the item documentation
with the existing module-level CircuitBreaker contract.
- Around line 32-49: Make the breaker state transition atomic by consolidating
tripped, consecutive_failures, and last_trip into one Mutex-guarded state
structure. Update record_failure and is_open to access and mutate this shared
state under the same lock, ensuring is_open cannot observe tripped without its
corresponding trip timestamp.
In `@src/memory/chunks/store.rs`:
- Around line 88-94: Replace the hardcoded 'admitted' lifecycle status in the
SQL assignment within the chunk store upsert flow with the existing named status
constant, reusing the same symbol as CHUNK_STATUS_DROPPED and its usage in
store_list.rs. Keep the SQL behavior unchanged while preventing the status value
from being duplicated as a string literal.
In `@src/memory/config/policy.rs`:
- Around line 52-78: Move direct validator coverage for
RetrievalLimits::validate, ScoringPolicyConfig::validate, and
QueueConfig::validate out of config_tests.rs into a dedicated policy_tests.rs
sibling of the policy module. Add focused tests for each validator’s accepted
and rejected bounds while retaining MemoryConfig::validate integration coverage
where appropriate.
In `@src/memory/diff/ledger_helpers.rs`:
- Around line 13-16: Add Rust doc comments to checkpoint_message,
checkpoint_from_message, validate_source_id, and build_commit_message
documenting their wire-schema or validation/trailer-layout contracts, including
relevant rejection and formatting behavior. Follow the existing documentation
style used by parse_trailers, sanitize_trailer, and encode_item_id; do not
change implementation behavior.
In `@src/memory/queue/store_settle.rs`:
- Line 1: Share the 7-day max_defer_age_ms default instead of duplicating it
across settlement and policy code. Update the defer-age boundary in
store_settle.rs and store_settle_tests.rs to reuse MemoryConfig::default() or a
shared constant from policy.rs, while preserving the existing settlement
behavior and test boundary.
In `@src/memory/score/store_query.rs`:
- Around line 1-92: Move focused unit tests for the diagnostics-query functions
in store_query, including lookup_entity_in_window and list_entity_ids_for_node,
from store_tests.rs into a co-located store_query_tests.rs sibling under src.
Keep transactional or cross-module integration tests in store_tests.rs, and
preserve the existing test coverage and behavior.
In `@src/memory/sources/readers/folder.rs`:
- Around line 117-125: Update the default pattern used in read_item to reference
the existing DEFAULT_GLOB constant instead of hardcoding "**/*.md". Keep the
explicit source.glob value and subsequent glob matching behavior unchanged.
In `@src/memory/sources/registry.rs`:
- Around line 183-185: Extract a private lock-acquisition helper for
REGISTRY_MUTATION_LOCK that performs the existing lock call and panic message,
then replace the repeated boilerplate in all mutating methods, including update,
remove, remove_composio_source_by_connection_id, upsert_composio_source,
upsert_composio_sources_batch, apply_all_in, and the method shown. Keep each
method’s guard held for its full mutation scope.
In `@src/memory/store/content/atomic.rs`:
- Around line 40-69: Verify and enforce that the workspace filesystem supports
hard links before relying on std::fs::hard_link in the publish path used by
write_if_new. Add an appropriate capability check or documented validation at
workspace initialization, and ensure unsupported filesystems produce a clear
error rather than causing unexplained publish failures.
In `@src/memory/store/entity_index/transaction.rs`:
- Around line 10-29: Expand the doc comment for the public function
index_entities_tx to document its identity behavior: new rows are inserted with
is_user = false, while conflicts preserve the existing is_user value via
UPSERT_PRESERVE_USER_SQL; clarify that NoSelfIdentity does not merely skip
identity classification.
In `@src/memory/store/kv.rs`:
- Around line 229-257: Extract the repeated value_json deserialization and
contextual error formatting from list_namespace, records_namespace, and
records_global into a shared private helper. Have each row-iteration method call
the helper with the raw JSON and its function context, preserving the existing
error messages and successful Value results.
In `@src/memory/store/safety/pii/checks.rs`:
- Around line 1-232: Add a sibling checks_tests.rs module containing direct
edge-case tests for valid_cpf, valid_cnpj, valid_cuit, valid_luhn, valid_iban,
valid_verhoeff, valid_ssn, valid_dni_es, valid_nie_es, and valid_nino. Cover
representative valid inputs plus invalid lengths, checksums, reserved values,
prefixes, and boundary rules, and structure the tests so the private validators
are exercised through the module’s test setup.
In `@src/memory/store/store.rs`:
- Around line 83-127: Extract the duplicated lowercase whitespace-term
tokenization and matched/total scoring into a shared helper, using one
consistent score type and preserving 1.0 for empty queries and no result for
zero matches. In src/memory/store/store.rs lines 83-127, update
MemoryStore::search to use the helper; in src/memory/store/memory_trait.rs lines
45-59, replace query_score with the helper; and in
src/memory/store/memory_trait.rs lines 134-179, update recall to consume the
helper’s result.
In `@src/memory/store/vectors/store_tests_more.rs`:
- Around line 31-45: Update search_handles_invalid_metadata_json to create the
vector using the store’s normal insertion path, then execute a targeted UPDATE
that sets only its metadata to invalid JSON. Remove the raw INSERT’s unrelated
column values while preserving the existing search and null-metadata assertions.
In `@src/memory/sync/composio/providers/slack_parse.rs`:
- Around line 10-81: Add item-level documentation comments to each pub(super)
helper: mention_regex, replace_mentions, next_cursor, search_matches,
search_total_pages, decode_cursors, and parse_ts. Document their purpose and the
non-obvious fallback paths, mention resolution behavior, cursor defaults, and
timestamp parsing contract without changing implementation behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
# Conflicts: # Cargo.toml # src/memory/mod.rs # src/memory/providers/mod.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e8ee978680
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/memory/mod.rs`:
- Around line 84-91: Update the documentation comment above the feature-gated
providers module in memory::providers to state that the providers-http feature
currently exposes the concrete OpenRouterProvider, removing the stale claim that
concrete providers will arrive with C3/M3. Preserve the existing feature-gating
and dependency descriptions.
In `@src/memory/persona/types.rs`:
- Around line 298-307: Update Persona’s source_id() to use an unambiguous
canonical encoding of kind, scope, session_id, and path, such as length-prefixed
fields or structured serialization, instead of joining values with "|". Preserve
deterministic output and ensure distinct provenance values remain distinct when
consumed by evidence_id().
- Around line 329-345: Update PersonaEvidence deserialization to route input
through PersonaEvidence::new, ensuring the excerpt is redacted and the
content-addressed id is recomputed from the source and excerpt rather than
trusting serialized id/excerpt values. Replace the derived Deserialize
implementation with a custom one or remove deserialization support if no longer
required, while preserving valid serialized fields such as timestamp, tier, and
facets.
In `@src/memory/providers/openrouter.rs`:
- Around line 312-330: Replace the shared usage snapshot subtraction around the
summary call with request-local usage returned by chat() or its accounting path.
Update the relevant chat() result and SummaryCall construction so input_tokens,
output_tokens, and charged_amount_usd reflect only this request, while
preserving the existing summary output handling.
- Around line 137-180: Make request accounting in post_json count and admit
every retry attempt atomically, independently of whether the response includes
optional usage data; adjust check_budget and the attempt-accounting flow so
limits are enforced per attempt without relying on record_usage. In
src/memory/providers/openrouter.rs lines 137-180, update the relevant budget and
usage methods while preserving token and cost accumulation. In
src/memory/providers/openrouter_tests.rs lines 81-85, change the
429-then-success expectation to count both attempts and add coverage for
responses without usage data.
- Around line 200-206: Update the response handling in post_json so
r.text().await propagates body-read errors instead of converting them to an
empty string with unwrap_or_default(). Ensure the propagated error exits through
the existing retry path, while successful body reads continue to use the current
status and JSON-decoding logic.
- Around line 369-395: Update the embeddings response handling around the `out`
vector and item loop to validate the payload strictly: require each embedding
value to be numeric, require every embedding to match the expected dimension,
reject duplicate or out-of-range `index` values, and ensure all expected input
positions are populated so short responses cannot succeed. Add wiremock
regression coverage for short responses, duplicate/out-of-range indices,
non-numeric values, and incorrect embedding lengths.
In `@src/memory/store/content/atomic.rs`:
- Around line 62-66: Update the hard-link error handling in the atomic publish
flow to preserve the original error for failures unrelated to unsupported
filesystem semantics. Only wrap the error as a capability issue when the failure
specifically indicates an unsupported hard-link operation; otherwise propagate
or retain the original `e`, including permission, ENOSPC, and invalid-path
errors.
In `@src/memory/tree/runtime/store/paths.rs`:
- Around line 33-38: The path resolution logic using
sanitize_component_with_digest in src/memory/tree/runtime/store/paths.rs lines
31-38 and the corresponding logic in src/memory/archivist/store.rs lines 44-46
must probe the legacy digest-suffixed directory before using the transformed
namespace/session location, preserving access to data written by older versions;
update both sites consistently, or migrate the legacy directory when
encountered.
🪄 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: CHILL
Plan: Pro
Run ID: 3d94bb28-23b0-40f4-9d08-28f1ba5c1fb6
📒 Files selected for processing (61)
Cargo.tomlsrc/memory/archivist/store.rssrc/memory/chunks/connection.rssrc/memory/chunks/connection_breaker.rssrc/memory/chunks/migrations.rssrc/memory/chunks/produce.rssrc/memory/chunks/store.rssrc/memory/chunks/store_delete.rssrc/memory/chunks/store_list.rssrc/memory/config.rssrc/memory/config/policy.rssrc/memory/config/policy_tests.rssrc/memory/conversations/bus.rssrc/memory/conversations/inverted_index.rssrc/memory/conversations/store_index.rssrc/memory/conversations/store_tests_more.rssrc/memory/diff/ledger_helpers.rssrc/memory/fsutil.rssrc/memory/ingest/pipeline.rssrc/memory/ingest/pipeline_tests.rssrc/memory/ingest/types.rssrc/memory/mod.rssrc/memory/persona/mod.rssrc/memory/persona/types.rssrc/memory/providers/openrouter.rssrc/memory/providers/openrouter_tests.rssrc/memory/queue/ops_tests.rssrc/memory/queue/runtime/mod.rssrc/memory/queue/store.rssrc/memory/queue/store_settle.rssrc/memory/queue/store_settle_tests.rssrc/memory/queue/test_support.rssrc/memory/queue/worker.rssrc/memory/queue/worker_tests.rssrc/memory/retrieval/global.rssrc/memory/retrieval/source.rssrc/memory/score/mod.rssrc/memory/score/store_query.rssrc/memory/score/store_query_tests.rssrc/memory/sources/readers/folder.rssrc/memory/sources/registry.rssrc/memory/sources/types.rssrc/memory/store/content/atomic.rssrc/memory/store/entity_index/transaction.rssrc/memory/store/kv.rssrc/memory/store/memory_trait.rssrc/memory/store/mod.rssrc/memory/store/safety/pii/checks.rssrc/memory/store/safety/pii/checks_tests.rssrc/memory/store/store.rssrc/memory/store/vectors/store.rssrc/memory/store/vectors/store_tests_more.rssrc/memory/sync/composio/providers/slack_parse.rssrc/memory/sync/dispatcher.rssrc/memory/sync/dispatcher_tests.rssrc/memory/tool_memory/store.rssrc/memory/tree/bucket_seal.rssrc/memory/tree/document_seal.rssrc/memory/tree/runtime/store/paths.rssrc/memory/types.rssrc/memory/types_tests.rs
🚧 Files skipped from review as they are similar to previous changes (34)
- src/memory/queue/store_settle_tests.rs
- src/memory/ingest/types.rs
- src/memory/queue/worker_tests.rs
- src/memory/store/kv.rs
- src/memory/config/policy.rs
- src/memory/sync/composio/providers/slack_parse.rs
- src/memory/tool_memory/store.rs
- src/memory/queue/worker.rs
- src/memory/chunks/produce.rs
- src/memory/retrieval/source.rs
- src/memory/chunks/store_list.rs
- src/memory/types_tests.rs
- src/memory/score/store_query.rs
- src/memory/store/vectors/store_tests_more.rs
- src/memory/tree/document_seal.rs
- src/memory/store/entity_index/transaction.rs
- src/memory/conversations/store_index.rs
- src/memory/ingest/pipeline_tests.rs
- src/memory/store/safety/pii/checks.rs
- src/memory/score/mod.rs
- src/memory/store/vectors/store.rs
- src/memory/tree/bucket_seal.rs
- src/memory/sources/readers/folder.rs
- src/memory/types.rs
- src/memory/chunks/store_delete.rs
- src/memory/ingest/pipeline.rs
- src/memory/retrieval/global.rs
- src/memory/conversations/store_tests_more.rs
- src/memory/sources/registry.rs
- Cargo.toml
- src/memory/config.rs
- src/memory/conversations/inverted_index.rs
- src/memory/diff/ledger_helpers.rs
- src/memory/chunks/connection.rs
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/memory/mod.rs`:
- Around line 84-91: Update the documentation comment above the feature-gated
providers module in memory::providers to state that the providers-http feature
currently exposes the concrete OpenRouterProvider, removing the stale claim that
concrete providers will arrive with C3/M3. Preserve the existing feature-gating
and dependency descriptions.
In `@src/memory/persona/types.rs`:
- Around line 298-307: Update Persona’s source_id() to use an unambiguous
canonical encoding of kind, scope, session_id, and path, such as length-prefixed
fields or structured serialization, instead of joining values with "|". Preserve
deterministic output and ensure distinct provenance values remain distinct when
consumed by evidence_id().
- Around line 329-345: Update PersonaEvidence deserialization to route input
through PersonaEvidence::new, ensuring the excerpt is redacted and the
content-addressed id is recomputed from the source and excerpt rather than
trusting serialized id/excerpt values. Replace the derived Deserialize
implementation with a custom one or remove deserialization support if no longer
required, while preserving valid serialized fields such as timestamp, tier, and
facets.
In `@src/memory/providers/openrouter.rs`:
- Around line 312-330: Replace the shared usage snapshot subtraction around the
summary call with request-local usage returned by chat() or its accounting path.
Update the relevant chat() result and SummaryCall construction so input_tokens,
output_tokens, and charged_amount_usd reflect only this request, while
preserving the existing summary output handling.
- Around line 137-180: Make request accounting in post_json count and admit
every retry attempt atomically, independently of whether the response includes
optional usage data; adjust check_budget and the attempt-accounting flow so
limits are enforced per attempt without relying on record_usage. In
src/memory/providers/openrouter.rs lines 137-180, update the relevant budget and
usage methods while preserving token and cost accumulation. In
src/memory/providers/openrouter_tests.rs lines 81-85, change the
429-then-success expectation to count both attempts and add coverage for
responses without usage data.
- Around line 200-206: Update the response handling in post_json so
r.text().await propagates body-read errors instead of converting them to an
empty string with unwrap_or_default(). Ensure the propagated error exits through
the existing retry path, while successful body reads continue to use the current
status and JSON-decoding logic.
- Around line 369-395: Update the embeddings response handling around the `out`
vector and item loop to validate the payload strictly: require each embedding
value to be numeric, require every embedding to match the expected dimension,
reject duplicate or out-of-range `index` values, and ensure all expected input
positions are populated so short responses cannot succeed. Add wiremock
regression coverage for short responses, duplicate/out-of-range indices,
non-numeric values, and incorrect embedding lengths.
In `@src/memory/store/content/atomic.rs`:
- Around line 62-66: Update the hard-link error handling in the atomic publish
flow to preserve the original error for failures unrelated to unsupported
filesystem semantics. Only wrap the error as a capability issue when the failure
specifically indicates an unsupported hard-link operation; otherwise propagate
or retain the original `e`, including permission, ENOSPC, and invalid-path
errors.
In `@src/memory/tree/runtime/store/paths.rs`:
- Around line 33-38: The path resolution logic using
sanitize_component_with_digest in src/memory/tree/runtime/store/paths.rs lines
31-38 and the corresponding logic in src/memory/archivist/store.rs lines 44-46
must probe the legacy digest-suffixed directory before using the transformed
namespace/session location, preserving access to data written by older versions;
update both sites consistently, or migrate the legacy directory when
encountered.
🪄 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: CHILL
Plan: Pro
Run ID: 3d94bb28-23b0-40f4-9d08-28f1ba5c1fb6
📒 Files selected for processing (61)
Cargo.tomlsrc/memory/archivist/store.rssrc/memory/chunks/connection.rssrc/memory/chunks/connection_breaker.rssrc/memory/chunks/migrations.rssrc/memory/chunks/produce.rssrc/memory/chunks/store.rssrc/memory/chunks/store_delete.rssrc/memory/chunks/store_list.rssrc/memory/config.rssrc/memory/config/policy.rssrc/memory/config/policy_tests.rssrc/memory/conversations/bus.rssrc/memory/conversations/inverted_index.rssrc/memory/conversations/store_index.rssrc/memory/conversations/store_tests_more.rssrc/memory/diff/ledger_helpers.rssrc/memory/fsutil.rssrc/memory/ingest/pipeline.rssrc/memory/ingest/pipeline_tests.rssrc/memory/ingest/types.rssrc/memory/mod.rssrc/memory/persona/mod.rssrc/memory/persona/types.rssrc/memory/providers/openrouter.rssrc/memory/providers/openrouter_tests.rssrc/memory/queue/ops_tests.rssrc/memory/queue/runtime/mod.rssrc/memory/queue/store.rssrc/memory/queue/store_settle.rssrc/memory/queue/store_settle_tests.rssrc/memory/queue/test_support.rssrc/memory/queue/worker.rssrc/memory/queue/worker_tests.rssrc/memory/retrieval/global.rssrc/memory/retrieval/source.rssrc/memory/score/mod.rssrc/memory/score/store_query.rssrc/memory/score/store_query_tests.rssrc/memory/sources/readers/folder.rssrc/memory/sources/registry.rssrc/memory/sources/types.rssrc/memory/store/content/atomic.rssrc/memory/store/entity_index/transaction.rssrc/memory/store/kv.rssrc/memory/store/memory_trait.rssrc/memory/store/mod.rssrc/memory/store/safety/pii/checks.rssrc/memory/store/safety/pii/checks_tests.rssrc/memory/store/store.rssrc/memory/store/vectors/store.rssrc/memory/store/vectors/store_tests_more.rssrc/memory/sync/composio/providers/slack_parse.rssrc/memory/sync/dispatcher.rssrc/memory/sync/dispatcher_tests.rssrc/memory/tool_memory/store.rssrc/memory/tree/bucket_seal.rssrc/memory/tree/document_seal.rssrc/memory/tree/runtime/store/paths.rssrc/memory/types.rssrc/memory/types_tests.rs
🚧 Files skipped from review as they are similar to previous changes (34)
- src/memory/queue/store_settle_tests.rs
- src/memory/ingest/types.rs
- src/memory/queue/worker_tests.rs
- src/memory/store/kv.rs
- src/memory/config/policy.rs
- src/memory/sync/composio/providers/slack_parse.rs
- src/memory/tool_memory/store.rs
- src/memory/queue/worker.rs
- src/memory/chunks/produce.rs
- src/memory/retrieval/source.rs
- src/memory/chunks/store_list.rs
- src/memory/types_tests.rs
- src/memory/score/store_query.rs
- src/memory/store/vectors/store_tests_more.rs
- src/memory/tree/document_seal.rs
- src/memory/store/entity_index/transaction.rs
- src/memory/conversations/store_index.rs
- src/memory/ingest/pipeline_tests.rs
- src/memory/store/safety/pii/checks.rs
- src/memory/score/mod.rs
- src/memory/store/vectors/store.rs
- src/memory/tree/bucket_seal.rs
- src/memory/sources/readers/folder.rs
- src/memory/types.rs
- src/memory/chunks/store_delete.rs
- src/memory/ingest/pipeline.rs
- src/memory/retrieval/global.rs
- src/memory/conversations/store_tests_more.rs
- src/memory/sources/registry.rs
- Cargo.toml
- src/memory/config.rs
- src/memory/conversations/inverted_index.rs
- src/memory/diff/ledger_helpers.rs
- src/memory/chunks/connection.rs
🛑 Comments failed to post (9)
src/memory/mod.rs (1)
84-91: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the providers feature documentation.
OpenRouterProvideris now a concrete provider undermemory::providers, but this text says concrete providers will land with C3/M3. Replace the stale wording so users understand what the feature currently provides.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/memory/mod.rs` around lines 84 - 91, Update the documentation comment above the feature-gated providers module in memory::providers to state that the providers-http feature currently exposes the concrete OpenRouterProvider, removing the stale claim that concrete providers will arrive with C3/M3. Preserve the existing feature-gating and dependency descriptions.src/memory/persona/types.rs (2)
298-307: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Make
source_id()unambiguous.Joining provenance fields with
|allows different(scope, session_id, path)values containing|to produce the same source ID. Because this value feedsevidence_id(), unrelated evidence can collide and deduplicate incorrectly. Use length-prefixed or canonical structured encoding.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/memory/persona/types.rs` around lines 298 - 307, Update Persona’s source_id() to use an unambiguous canonical encoding of kind, scope, session_id, and path, such as length-prefixed fields or structured serialization, instead of joining values with "|". Preserve deterministic output and ensure distinct provenance values remain distinct when consumed by evidence_id().
329-345: 🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail # Map the relevant file and locate PersonaEvidence-related items. ast-grep outline src/memory/persona/types.rs --view expanded || true printf '\n--- rg hits ---\n' rg -n "PersonaEvidence|redact_pii|Deserialize|serde|new\\(" src/memory/persona/types.rsRepository: tinyhumansai/tinycortex
Length of output: 4186
🏁 Script executed:
#!/bin/bash set -euo pipefail sed -n '320,390p' src/memory/persona/types.rs | cat -nRepository: tinyhumansai/tinycortex
Length of output: 3092
🏁 Script executed:
#!/bin/bash set -euo pipefail sed -n '1,40p' src/memory/persona/types.rs | cat -nRepository: tinyhumansai/tinycortex
Length of output: 2390
Make
PersonaEvidencedeserialization go through the redacting constructor
#[derive(Deserialize)]lets serde populate the privateexcerptand arbitraryiddirectly from JSON/on-disk data, bypassingPersonaEvidence::newand the redaction/content-addressing invariant. Deserialize throughnew(or removeDeserialize), and keepid/sourcefrom drifting out of sync with the stored excerpt.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/memory/persona/types.rs` around lines 329 - 345, Update PersonaEvidence deserialization to route input through PersonaEvidence::new, ensuring the excerpt is redacted and the content-addressed id is recomputed from the source and excerpt rather than trusting serialized id/excerpt values. Replace the derived Deserialize implementation with a custom one or remove deserialization support if no longer required, while preserving valid serialized fields such as timestamp, tier, and facets.src/memory/providers/openrouter.rs (4)
137-180: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use one consistent per-attempt request-accounting contract.
src/memory/providers/openrouter.rs#L137-L180: count and admit each retry attempt atomically, independently of optional token-usage data.src/memory/providers/openrouter_tests.rs#L81-L85: update the429-then-success expectation to include both attempts and cover usage-less responses.📍 Affects 2 files
src/memory/providers/openrouter.rs#L137-L180(this comment)src/memory/providers/openrouter_tests.rs#L81-L85🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/memory/providers/openrouter.rs` around lines 137 - 180, Make request accounting in post_json count and admit every retry attempt atomically, independently of whether the response includes optional usage data; adjust check_budget and the attempt-accounting flow so limits are enforced per attempt without relying on record_usage. In src/memory/providers/openrouter.rs lines 137-180, update the relevant budget and usage methods while preserving token and cost accumulation. In src/memory/providers/openrouter_tests.rs lines 81-85, change the 429-then-success expectation to count both attempts and add coverage for responses without usage data.
200-206: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
set -euo pipefail wc -l src/memory/providers/openrouter.rs sed -n '1,280p' src/memory/providers/openrouter.rs rg -n "retry|retri|transport|body-read|unwrap_or_default|text\(\)\.await|with_context" src/memory/providers/openrouter.rsRepository: tinyhumansai/tinycortex
Length of output: 12124
🏁 Script executed:
set -euo pipefail # Inspect the surrounding retry logic and any helper wrappers used by OpenRouter requests. ast-grep outline src/memory/providers/openrouter.rs --view expandedRepository: tinyhumansai/tinycortex
Length of output: 1973
Retry body-read failures.
r.text().await.unwrap_or_default()turns a transient response-body read error into an empty payload, so a200can fail at decode time and bypass the retry loop. Propagate that error throughpost_jsoninstead of defaulting to"".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/memory/providers/openrouter.rs` around lines 200 - 206, Update the response handling in post_json so r.text().await propagates body-read errors instead of converting them to an empty string with unwrap_or_default(). Ensure the propagated error exits through the existing retry path, while successful body reads continue to use the current status and JSON-decoding logic.
312-330: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not derive per-call usage from shared snapshots.
Concurrent summaries can overlap between
beforeandafter, causinginput_tokens,output_tokens, andcharged_amount_usdto include other calls. Return request-local usage fromchat()/accounting instead of subtracting global snapshots.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/memory/providers/openrouter.rs` around lines 312 - 330, Replace the shared usage snapshot subtraction around the summary call with request-local usage returned by chat() or its accounting path. Update the relevant chat() result and SummaryCall construction so input_tokens, output_tokens, and charged_amount_usd reflect only this request, while preserving the existing summary output handling.
369-395: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail printf 'FILE INFO\n' wc -l src/memory/providers/openrouter.rs printf '\nRELEVANT LINES\n' sed -n '330,430p' src/memory/providers/openrouter.rs printf '\nTEST SEARCH\n' rg -n "OpenRouter embeddings|embedding.*dimension|missing data\[\]|returned fewer vectors|index" src/memory -g '*rs'Repository: tinyhumansai/tinycortex
Length of output: 50379
🏁 Script executed:
#!/bin/bash set -euo pipefail printf 'FILE INFO\n' wc -l src/memory/providers/openrouter.rs printf '\nRELEVANT LINES\n' sed -n '330,430p' src/memory/providers/openrouter.rs printf '\nTEST SEARCH\n' rg -n "OpenRouter embeddings|embedding.*dimension|missing data\\[\\]|returned fewer vectors|index" src/memory -g '*rs'Repository: tinyhumansai/tinycortex
Length of output: 50379
🏁 Script executed:
#!/bin/bash set -euo pipefail sed -n '1,260p' src/memory/providers/openrouter_tests.rsRepository: tinyhumansai/tinycortex
Length of output: 6843
Validate the embeddings payload shape.
outis sized fromdata.len(), so a short response can returnOkwith fewer vectors than requested. Reject duplicate/out-of-range indices, non-numeric values, and wrong-length embeddings instead of silently accepting them; add wiremock regressions for those cases.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/memory/providers/openrouter.rs` around lines 369 - 395, Update the embeddings response handling around the `out` vector and item loop to validate the payload strictly: require each embedding value to be numeric, require every embedding to match the expected dimension, reject duplicate or out-of-range `index` values, and ensure all expected input positions are populated so short responses cannot succeed. Add wiremock regression coverage for short responses, duplicate/out-of-range indices, non-numeric values, and incorrect embedding lengths.src/memory/store/content/atomic.rs (1)
62-66: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail # Map the file shape first, then read the relevant slice. ast-grep outline src/memory/store/content/atomic.rs --view expanded || true printf '\n--- file excerpt ---\n' cat -n src/memory/store/content/atomic.rs | sed -n '1,220p'Repository: tinyhumansai/tinycortex
Length of output: 8386
Preserve the original hard-link error
hard_linkcan fail for reasons other than unsupported filesystem semantics (for example permissions, ENOSPC, or invalid paths). Ifabs_pathdoes not exist, this branch still rewrites every failure as a filesystem-capability issue, so keep the originaleor only special-case the unsupported-operation case.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/memory/store/content/atomic.rs` around lines 62 - 66, Update the hard-link error handling in the atomic publish flow to preserve the original error for failures unrelated to unsupported filesystem semantics. Only wrap the error as a capability issue when the failure specifically indicates an unsupported hard-link operation; otherwise propagate or retain the original `e`, including permission, ENOSPC, and invalid-path errors.src/memory/tree/runtime/store/paths.rs (1)
33-38: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n== File list ==\n' git ls-files src/memory/tree/runtime/store/paths.rs src/memory/archivist/store.rs src/memory/fsutil.rs printf '\n== Outlines ==\n' ast-grep outline src/memory/tree/runtime/store/paths.rs --view expanded || true ast-grep outline src/memory/archivist/store.rs --view expanded || true ast-grep outline src/memory/fsutil.rs --view expanded || true printf '\n== Relevant excerpts ==\n' for f in src/memory/tree/runtime/store/paths.rs src/memory/archivist/store.rs src/memory/fsutil.rs; do echo "--- $f ---" nl -ba "$f" | sed -n '1,220p' doneRepository: tinyhumansai/tinycortex
Length of output: 1676
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n== Search for legacy/compatibility/migration around these stores ==\n' rg -n "legacy|migrat|compat|fallback|sanitize_component_with_digest|digest-suffixed|namespace directory|session directory" src/memory -S printf '\n== Search for filesystem key/path helpers used by store paths ==\n' rg -n "sanitize_component_with_digest|sanitize_component|fsutil" src/memory -SRepository: tinyhumansai/tinycortex
Length of output: 31974
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n== Check surrounding tests or docs for persistence semantics ==\n' rg -n "tree/runtime/store/paths|archivist/store|sanitize_component_with_digest|path.*compat|legacy.*path|digest" src tests -S || trueRepository: tinyhumansai/tinycortex
Length of output: 18690
🏁 Script executed:
#!/bin/bash set -euo pipefail # Read the two target files with line numbers, plus the shared helper. for f in src/memory/tree/runtime/store/paths.rs src/memory/archivist/store.rs src/memory/fsutil.rs; do echo "--- $f ---" nl -ba "$f" | sed -n '1,220p' doneRepository: tinyhumansai/tinycortex
Length of output: 248
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "=== src/memory/tree/runtime/store/paths.rs ===" nl -ba src/memory/tree/runtime/store/paths.rs | sed -n '1,140p' echo echo "=== src/memory/archivist/store.rs ===" nl -ba src/memory/archivist/store.rs | sed -n '1,140p' echo echo "=== src/memory/fsutil.rs ===" nl -ba src/memory/fsutil.rs | sed -n '1,220p' echo echo "=== references to sanitize_component_with_digest ===" rg -n "sanitize_component_with_digest" src/memory -SRepository: tinyhumansai/tinycortex
Length of output: 248
🏁 Script executed:
#!/bin/bash set -euo pipefail show() { f="$1" start="$2" end="$3" echo "--- $f:$start-$end ---" awk -v s="$start" -v e="$end" 'NR>=s && NR<=e { printf "%4d %s\n", NR, $0 }' "$f" echo } show src/memory/tree/runtime/store/paths.rs 1 120 show src/memory/archivist/store.rs 1 220 show src/memory/fsutil.rs 1 140 printf '\n== search for alternative legacy path handling in nearby modules ==\n' rg -n "tree_dir\\(|buffer_dir\\(|node_file_path\\(|session_dir\\(|sanitize_session\\(|sanitize\\(" src/memory/tree src/memory/archivist -S printf '\n== search for migration/fallback around these stores ==\n' rg -n "legacy.*(path|dir)|fallback.*(path|dir)|migrat.*(path|dir)|compat.*(path|dir)" src/memory/tree src/memory/archivist -SRepository: tinyhumansai/tinycortex
Length of output: 20258
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '== tests referencing tree/store paths or archivist sessions ==\n' rg -n "tree_dir|buffer_dir|node_file_path|session_dir|session_entries|record_turn|sanitize_component_with_digest" src/memory/tree src/memory/archivist tests -S printf '\n== file inventory for relevant tests ==\n' git ls-files 'src/memory/tree/**/*tests*.rs' 'src/memory/archivist/**/*tests*.rs' 'tests/**/*.rs'Repository: tinyhumansai/tinycortex
Length of output: 7116
Add a legacy fallback for digest-suffixed directories. Transformed namespaces/sessions now map to a different on-disk directory, so previously written data can disappear on upgrade unless you probe or migrate the legacy location first.
src/memory/tree/runtime/store/paths.rs#L31-L38src/memory/archivist/store.rs#L43-L46📍 Affects 2 files
src/memory/tree/runtime/store/paths.rs#L33-L38(this comment)src/memory/archivist/store.rs#L44-L46🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/memory/tree/runtime/store/paths.rs` around lines 33 - 38, The path resolution logic using sanitize_component_with_digest in src/memory/tree/runtime/store/paths.rs lines 31-38 and the corresponding logic in src/memory/archivist/store.rs lines 44-46 must probe the legacy digest-suffixed directory before using the transformed namespace/session location, preserving access to data written by older versions; update both sites consistently, or migrate the legacy directory when encountered.
1 participant
Summary
Memorycontract for the reference storerpcandproviders-httpfeature surfaces, splits every Rust source file below 500 lines, and updates the spec index with an honest remediation recordNotable contract changes
StoreError, avoiding the duplicate publicMemoryErrornameMemoryConfignow includes scoring, ingestion, retrieval-limit, and queue policy roots with validation and partial TOML defaultsVerification
cargo fmt --all -- --checkcargo check --all-featurescargo clippy --all-targets --all-features -- -D warningsRUSTDOCFLAGS="-D warnings" cargo doc --all-features --no-depscargo test --all-features: 1300 unit tests, 15 functional tests, 4 doctests passed; one credentialed live test and one example ignoredtokio,git-diff, andsynccargo llvm-cov --all-features --workspace --summary-only: 90.60% line coveragecargo package --allow-dirty --no-verifySee
docs/spec/README.mdfor the current status and the retained pre-remediation audit rationale.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests