feat(memory): port OpenHuman engine gaps#66
Conversation
Replace the lightweight PII screen (4 generic patterns → `[REDACTED_PII]`)
with OpenHuman's exhaustive checksum-gated module, so `sanitize_text` and the
`has_likely_pii` boundary match the host contract byte-for-byte (a prerequisite
for OpenHuman flipping its `memory_store::safety` onto this crate — the host's
parity guard rejects the lightweight screen).
- `store/safety.rs` → `store/safety/{mod,safety_tests}.rs`; add `store/safety/pii.rs`.
- `pii.rs`: formatted + checksum-gated national IDs (CPF/CNPJ Luhn-style, CUIT,
RFC, JP My Number, SSN range filter, credit-card Luhn, IBAN mod-97, Aadhaar
Verhoeff, India PAN, UK NINO, Spain DNI/NIE, Korea RRN) + E.164/NANP phone,
with a Unicode normalization pre-pass (zero-width strip, fullwidth/Arabic-Indic
digit folding) mapped back to original offsets, and overlap-safe priority
redaction. `once_cell::Lazy` → `std::LazyLock` to match crate style.
- `sanitize_text` now runs `pii::redact_pii` after secret redaction; `has_likely_pii`
is re-exported from `pii` (strict boundary set: formatted IDs only, bare-numeric
and phone excluded from write-rejection but still content-scrubbed).
- Updated the two mod-level PII tests to the strict-boundary + national-ID
contract (email is out of scope by design).
cargo test --lib safety: 56 passed.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 54 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 (37)
📝 WalkthroughWalkthroughThis change adds a feature-gated synchronization engine with Composio providers, persistent sync state, audit logging, graph retrieval, content hydration, expanded storage APIs, multilingual PII handling, and observable tree sealing. ChangesMemory storage and retrieval
Synchronization and tree processing
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant SyncDispatcher
participant ComposioClient
participant IncrementalSource
participant SyncStateStore
participant SkillDocSink
SyncDispatcher->>ComposioClient: execute provider action
ComposioClient->>IncrementalSource: return paginated response
IncrementalSource->>SyncStateStore: load and update cursor/budget
IncrementalSource->>SkillDocSink: store converted SkillDocument
IncrementalSource->>SyncStateStore: persist sync state
SyncDispatcher-->>SyncDispatcher: return SyncOutcome
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: d93452acc7
ℹ️ 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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 874e20afd9
ℹ️ 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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ccc26809c6
ℹ️ 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.
💡 Codex Review
tinycortex/src/memory/retrieval/cover.rs
Line 320 in 92d20fc
Keep all latest-version document summaries
When a document version seals into more than one summary node (for example, several L1 batches plus an L2 root), those nodes all share the same doc_id and version_ms. This line treats every same-version node after the first as superseded; because list_summaries_in_window returns lower levels before the root, the later root is marked as a loser and collect_subtree_ids removes the whole current-version subtree and suppresses its chunks, so cover_window_scoped can return no current content for larger documents. Dedupe superseded versions without dropping sibling/ancestor nodes from the winning version.
tinycortex/src/memory/retrieval/drill_down.rs
Lines 185 to 186 in 92d20fc
Do not dedupe same-version drill-down branches
When drilling into a document subtree with multiple child summaries for the same current doc_id/version_ms (or when max_depth > 1 reaches descendants of an already-emitted document root), this global emitted_docs set skips every later sibling/descendant from that same version. The result is that drill_down returns only one branch of a multi-batch document, or refuses to expand a document root to its leaves. Limit the dedupe to competing revision roots rather than all nodes sharing the document id.
ℹ️ 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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 64a276cbdd
ℹ️ 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: 1
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/memory/tree/runtime/engine.rs (1)
204-213: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winNotify observers when a rebuild completes as a no-op.
Both early returns bypass
rebuild_completed, so hosts waiting for a terminal notification can remain pending for empty trees or trees without hour leaves.Proposed correction
let status = store::get_tree_status(config, namespace)?; if status.total_nodes == 0 { + observer.rebuild_completed(namespace, status.total_nodes); return Ok(status); } let base = store::tree_dir(config, namespace); let mut hour_leaves: Vec<TreeNode> = Vec::new(); collect_hour_leaves_recursive(&base, namespace, "", &mut hour_leaves)?; if hour_leaves.is_empty() { - return store::get_tree_status(config, namespace); + let status = store::get_tree_status(config, namespace)?; + observer.rebuild_completed(namespace, status.total_nodes); + return Ok(status); }🤖 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/engine.rs` around lines 204 - 213, Update the rebuild flow around collect_hour_leaves_recursive so both no-op paths—status.total_nodes == 0 and hour_leaves.is_empty()—invoke rebuild_completed before returning. Preserve each path’s existing status result while ensuring observers receive the terminal notification.src/memory/tree/store/buffers.rs (1)
17-57: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winDocument transaction and fallback semantics.
The newly public adapters should state that missing buffers return an unsaved empty value, identify transaction expectations, and describe timestamp/upsert behavior.
As per coding guidelines, “Document public APIs, module contracts, and non-obvious behavior thoroughly.”
🤖 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/store/buffers.rs` around lines 17 - 57, Document the public APIs get_buffer_conn and upsert_buffer_tx with their fallback and transaction semantics: state that get_buffer_conn returns an unsaved empty Buffer when no row exists, and that upsert_buffer_tx must be called within the caller-managed transaction. Also describe that upsert_buffer_tx writes updated_at_ms using the current timestamp while preserving the buffer’s oldest_at value.Source: Coding guidelines
src/memory/retrieval/cover.rs (1)
143-155: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winApply
source_scopewith the same key aschunk_tree_scope.ListChunksQuery.source_scopefilters onmetadata.source_id, but this cover path groups and resolves trees bychunk_tree_scope()(path_scopewhen present). For path-scoped chunks, that mismatch can silently drop allowed rows before the cover is built. Filter afterchunk_tree_scopehere, or translate the allowlist before callinglist_chunks.🤖 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/retrieval/cover.rs` around lines 143 - 155, Align the source_scope filtering in the cover retrieval flow with chunk_tree_scope(), which uses path_scope when present. Update the list_chunks query or subsequent filtering so path-scoped chunks are evaluated against the same scope key before building the cover, without dropping allowed rows due to metadata.source_id mismatch.
🟠 Major comments (34)
src/memory/store/safety/pii.rs-103-106 (1)
103-106: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winMatch lowercase IBANs.
Both the detailed regex and pre-screen require uppercase country and account letters, so a lowercase IBAN bypasses financial-identifier redaction.
Proposed fix
-static IBAN_RE: LazyLock<Regex> = - LazyLock::new(|| Regex::new(r"\b[A-Z]{2}\d{2}(?:[\s]?[A-Z0-9]){11,30}\b").expect("iban")); +static IBAN_RE: LazyLock<Regex> = + LazyLock::new(|| Regex::new(r"(?i)\b[A-Z]{2}\d{2}(?:[\s]?[A-Z0-9]){11,30}\b").expect("iban"));- r"\b[A-Z]{2}\d{2}[A-Z0-9]", // IBAN prefix + r"(?i)\b[A-Z]{2}\d{2}[A-Z0-9]", // IBAN prefixAlso applies to: 150-150
🤖 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.rs` around lines 103 - 106, Update the IBAN detection in IBAN_RE and its associated pre-screen to accept lowercase country and account letters, while preserving the existing optional-spacing and length rules so lowercase IBANs are redacted identically to uppercase values.src/memory/store/safety/pii.rs-139-158 (1)
139-158: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDo not preserve the bare ten-digit NANP bypass.
PHONE_NANP_REcan redact2025551234, butSCREENreturns early before that regex runs. This exposes a common phone-number form. Adding it toSCREENdoes not broadenhas_likely_pii, because strict collection already excludes phone patterns.Proposed fix
r"\(?[2-9]\d{2}\)?[\s.\-]\d{3}[\s.\-]\d{4}", // Formatted NANP + r"\b[2-9]\d{2}[2-9]\d{6}\b", // Bare NANPReplace the sentinel test with an assertion that the number is redacted.
Also applies to: 174-190, 1090-1107
🤖 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.rs` around lines 139 - 158, Update the SCREEN-based safety tests to assert that the bare ten-digit NANP number is redacted instead of treating it as an intentional bypass. Apply this change consistently to the related cases at the other referenced test locations, while preserving strict collection behavior that excludes phone-only patterns.src/memory/store/safety/pii.rs-134-159 (1)
134-159: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winInclude email addresses in content redaction.
EMAIL_REis only used byhas_likely_email; it is absent fromSCREENandcollect_redactions. Consequently,sanitize_textleaves email addresses intact despite its documented contract.Proposed fix
+const PII_EMAIL: &str = "[REDACTED_PII_EMAIL]";r"\d{6}-[1-4]\d{6}", // RRN + r"(?i)\b[A-Z0-9._%+-]+@[A-Z0-9.-]+\.[A-Z]{2,}\b", // Emailif include_bare_numeric { + // Content-only PII; strict key boundaries continue using + // `has_likely_email` according to caller policy. + push_simple(&mut hits, norm, &EMAIL_RE, PII_EMAIL); + // Phones: E.164 first (more specific), then NANP.Also applies to: 248-309
🤖 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.rs` around lines 134 - 159, Update the PII screening and redaction flow so email addresses detected by EMAIL_RE are included in SCREEN or otherwise trigger collect_redactions and sanitize_text redaction. Reuse the existing EMAIL_RE matching logic and ensure has_likely_email, collect_redactions, and sanitize_text consistently redact detected email addresses.src/memory/store/kv.rs-118-118 (1)
118-118: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not reject scanner-owned JIDs as email addresses.
has_likely_emailmatches identifiers such as12025551234@g.us, so these checks makeset_namespacereject the WhatsApp keys thathas_likely_piiexplicitly permits. This breaks scanner and sync-state persistence. Keep the generic KV boundary on strict PII, or add an explicit policy for user-authored keys.Proposed fix
- if safety::has_likely_email(key) || safety::has_likely_pii(key) { + if safety::has_likely_pii(key) { return Err("kv key cannot contain personal identifiers".to_string()); }- if safety::has_likely_email(namespace) - || safety::has_likely_email(key) - || safety::has_likely_pii(namespace) + if safety::has_likely_pii(namespace) || safety::has_likely_pii(key)As per coding guidelines, preserve machine-readable ids and enum wire strings when porting contracts from OpenHuman.
Also applies to: 158-162
🤖 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` at line 118, Update the validation in set_namespace to stop treating scanner-owned JIDs as email addresses: use the strict PII policy for generic KV keys, or explicitly allow the machine-readable JID formats permitted by has_likely_pii before applying email detection. Apply the same policy to the corresponding checks around the additional referenced location, preserving scanner and sync-state persistence while retaining rejection of user-authored PII.Source: Coding guidelines
src/memory/store/safety/pii.rs-1-1113 (1)
1-1113: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftSplit this module before merge.
At 1,113 lines, this file exceeds the repository’s 500-line limit. Separate patterns, normalization, checksum validators, and tests into focused sibling modules.
As per coding guidelines, do not let any source file grow beyond 500 lines; split behavior into focused modules before reaching that point.
🤖 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.rs` around lines 1 - 1113, Split the PII implementation into focused sibling modules so no source file exceeds 500 lines. Move regex/constants and match collection from collect_redactions_inner, Unicode handling from NormalizedView, checksum helpers such as valid_cpf and valid_luhn, and the test module into appropriate files; keep redact_pii, has_likely_pii, and has_likely_email behavior and module visibility intact through re-exports or qualified references.Source: Coding guidelines
src/memory/chunks/embeddings.rs-377-400 (1)
377-400: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFilter chunk rows by lifecycle status here — the chunk branch has no equivalent to
s.deleted = 0. Pending/dropped chunks can keep this helper true even though they’ll never get an embedding or skip tombstone, which can keepreembed_backfillarmed indefinitely.🤖 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/embeddings.rs` around lines 377 - 400, Update has_uncovered_reembed_work so the mem_tree_chunks branch filters out non-live chunks using the existing chunk lifecycle-status field, matching the summaries branch’s s.deleted = 0 condition. Keep the embedding and reembed-skipped checks unchanged, and ensure pending or dropped chunks cannot make the helper report uncovered work.src/memory/sync/github.rs-14-29 (1)
14-29: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winDocument the new pipeline type.
GithubRepoSyncPipelineand its constructor have no doc comments describing the source requirements, generatedidformat, or the validation performed innew.As per coding guidelines, "Document public APIs, module contracts, and non-obvious behavior thoroughly; prefer clear module-level docs and item docs over relying on implementation details."
🤖 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/github.rs` around lines 14 - 29, Add documentation for the public `GithubRepoSyncPipeline` struct and its `new` constructor, describing the required `GithubRepo` source, the generated `workspace:github_repo:{source.id}` identifier format, and the validation performed before construction.Source: Coding guidelines
src/memory/sync/dispatcher.rs-11-118 (1)
11-118: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winDocument the new public dispatcher API.
SyncRunResult,SyncDispatcher, and its public methods (new,register,ids,init_all,tick,tick_all) have no item-level doc comments. Per the PR objectives this is an explicit integration seam host applications are meant to build on top of (registeringSyncPipelines, readingSyncRunResult), so documenting expected semantics (e.g., whatidnormalization/uniqueness guaranteesregistergives, whatoutcome: NonevsSomemeans forinit_allvstick_all) would materially help integrators.As per coding guidelines, "Document public APIs, module contracts, and non-obvious behavior thoroughly; prefer clear module-level docs and item docs over relying on implementation details."
🤖 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 11 - 118, Document the public API symbols SyncRunResult, SyncDispatcher, and methods new, register, ids, init_all, tick, and tick_all with item-level Rust doc comments. Describe registration’s trimmed, non-empty, unique pipeline ID guarantees, deterministic ID access, and each operation’s result semantics, including that init_all returns a default outcome on success while tick_all returns the pipeline’s tick outcome, with errors represented separately.Source: Coding guidelines
src/memory/sync/github.rs-59-108 (1)
59-108: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUse the shared
content_roothelper here.
config.workspace.join("memory_tree/content")bypassesMemoryConfig.content_root, so a custom override will write GitHub archives under a different tree than the rest of the memory pipeline reads from.🐛 Proposed fix
- let content_root = config.workspace.join("memory_tree/content"); + let content_root = crate::memory::chunks::content_root(config);🤖 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/github.rs` around lines 59 - 108, Update the content root initialization in tick to use the shared MemoryConfig::content_root helper instead of joining "memory_tree/content" directly. Keep the existing write_raw_items flow unchanged so GitHub archives honor custom content-root overrides.src/memory/retrieval/fast.rs-81-121 (1)
81-121: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winApply
time_window_daysto graph-local results.The option reaches only dense/global fallback paths. When graph candidates exist, nodes outside the requested window are returned. Pass a cutoff into local candidate resolution and filter occurrence timestamps before ranking.
🤖 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/retrieval/fast.rs` around lines 81 - 121, Update the graph-local retrieval flow around local_candidates and resolve_local to apply options.time_window_days. Derive and pass the requested cutoff into local candidate resolution, then filter out occurrences older than that cutoff before ranking and limiting results; preserve the existing global fallback behavior.src/memory/sync/rebuild.rs-105-111 (1)
105-111: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDo not treat coverage failures as “no rebuild needed.”
An I/O or SQLite failure returns
false, making stale archives appear complete. ReturnResult<bool>and let callers surface or retry the failure.🤖 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/rebuild.rs` around lines 105 - 111, Update needs_rebuild to return Result<bool> instead of bool, propagate raw_coverage errors rather than logging and returning false, and preserve the pending-coverage check on success. Update all callers to handle or propagate the new Result so coverage failures can be surfaced or retried.src/memory/retrieval/fast.rs-232-240 (1)
232-240: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winBound the dense fallback input at
src/memory/retrieval/fast.rs:232.
query_sourcegathers and reranks the full match set before truncation, so passingusize::MAXhere still scans every summary before the scope filter runs. Use a bounded/paginated limit instead of relying on the final 100-result cap.🤖 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/retrieval/fast.rs` around lines 232 - 240, Update the query_source call in the dense fallback path to use a bounded, paginated limit instead of usize::MAX. Choose the appropriate limit that caps the match set before scope filtering while preserving the existing final 100-result output cap.src/memory/sync/rebuild.rs-249-270 (1)
249-270: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMake the batch handoff replay-safe.
ingest_summarymints a newsummary_idon every attempt, so ifmark_raw_paths_ingestedfails or the process crashes after the summary commit, rerunning this batch writes a second summary for the same raw files. Move coverage marking into the same transaction or add a batch idempotency key.🤖 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/rebuild.rs` around lines 249 - 270, Make the batch flow around ingest_summary and mark_raw_paths_ingested replay-safe by ensuring summary creation and raw-path coverage marking commit atomically, or by applying a stable batch idempotency key that reuses the existing summary on retries. Preserve the current child path coverage and prevent reruns after partial failure from minting duplicate summaries.src/memory/sync/composio/providers/slack.rs-113-115 (1)
113-115: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPersist a resume point before truncating search pagination.
Clamping
total_pagestomax_pagesmakesmore_pendingfalse after page 50 even when the provider advertised more pages. Since every tick restarts at page 1, later pages are never ingested. Preserve the provider total and persist the next search page or time-window cursor.Also applies to: 156-167
🤖 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.rs` around lines 113 - 115, Update the pagination flow around the page-1 total calculation and the later `more_pending` handling to preserve the provider-reported total pages instead of clamping `total_pages` to `self.max_pages`. Before stopping at the configured page limit, persist a resume point for the next search page or time-window cursor so subsequent ticks continue ingestion beyond the limit.src/memory/tree/direct_ingest.rs-95-143 (1)
95-143: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake post-commit cascade failures retry-safe.
The summary transaction commits before
cascade_all_from(...).await?. If cascading fails, callers receive an error although the L1 summary already exists; retrying creates a new summary ID and duplicates persisted data. Use an idempotency key or durable pending-cascade state, or return the committed ingest separately from cascade status.🤖 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/direct_ingest.rs` around lines 95 - 143, The direct-ingest flow must make failures after the transaction commit retry-safe: update the logic around the committed insert and `cascade_all_from` so a cascade error cannot cause a retry to create duplicate persisted summaries. Use an idempotency key or durable pending-cascade state, or return the committed `SummaryIngestOutcome` separately from cascade status, while preserving the existing transaction and successful-cascade behavior.src/memory/tree/direct_ingest.rs-18-43 (1)
18-43: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winDocument direct-ingestion invariants.
Add item docs for the public inputs, outcome, and function, especially time-range validity, child label/basename correspondence, persistence semantics, and cascade behavior.
As per coding guidelines, “Document public APIs, module contracts, and non-obvious behavior thoroughly.”
🤖 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/direct_ingest.rs` around lines 18 - 43, Add Rust documentation comments for the public SummaryIngestInput, SummaryIngestOutcome, and ingest_summary APIs. Document valid time-range expectations, the one-to-one correspondence between child_labels and child_basenames, persistence semantics, and cascade behavior, including what the returned sealed_ids represent.Source: Coding guidelines
src/memory/sync/composio/providers/slack.rs-68-163 (1)
68-163: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPersist request accounting on failure paths.
Every
?after provider calls can return beforestate.save, losing consumed-budget accounting and causing retries to repeat work. Structure the tick so mutated state is saved on both success and failure.🤖 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.rs` around lines 68 - 163, Update the sync tick surrounding Slack provider calls, especially SlackSyncPipeline::scopes and checked_execute, so any mutated SyncState is persisted with state.save before returning an error. Preserve normal success behavior and ensure failures from provider requests or subsequent processing do not bypass request-budget accounting.src/memory/sync/composio/providers/slack.rs-23-50 (1)
23-50: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winDocument the public pipeline contracts.
Add item docs for both public pipeline types and constructors, including backfill boundaries, pagination caps, state sharing, and expected credentials.
As per coding guidelines, “Document public APIs, module contracts, and non-obvious behavior thoroughly.”
Also applies to: 176-185
🤖 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.rs` around lines 23 - 50, Add documentation comments for the public SlackSyncPipeline and SlackSearchBackfillPipeline structs and their public constructors, covering backfill-day minimum/limits, pagination caps, shared synchronization state, and required Slack/Composio credentials. Apply the same contract documentation to the constructor around SlackSyncPipeline as well, using only behavior and configuration symbols exposed by these pipelines.Source: Coding guidelines
src/memory/sync/composio/providers/slack.rs-1-520 (1)
1-520: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftSplit this source file before merge.
At 520 lines, the new file exceeds the repository limit. Move the search-backfill pipeline or directory/search helpers into focused Slack submodules.
As per coding guidelines, “Do not let any source file grow beyond 500 lines; split behavior into focused modules before reaching that point.”
🤖 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.rs` around lines 1 - 520, Split the 520-line Slack implementation before merge by moving either SlackSearchBackfillPipeline with its search helpers, or the directory/history helpers, into focused Slack submodules. Update module declarations and imports so SlackSyncPipeline, IncrementalSource, and shared helpers continue working, and ensure the original slack.rs remains under 500 lines without changing behavior.Source: Coding guidelines
src/memory/sync/state.rs-9-177 (1)
9-177: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winDocument the public persistence and budget contracts.
The public constants, store trait, state models, and mutation methods need item docs covering key semantics, UTC reset behavior, serialization, and request accounting.
As per coding guidelines, “Document public APIs, module contracts, and non-obvious behavior thoroughly.”
🤖 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/state.rs` around lines 9 - 177, Add Rust documentation comments to the public constants, SyncStateStore, DailyBudget, and SyncState APIs. Document the namespace/key semantics, persistence through JSON serialization, UTC-based daily budget reset behavior, and the distinction between persisted daily accounting and per-run request/cost accounting. Cover mutation methods such as remaining, record_requests, record_action, load, and save, including their observable state changes.Source: Coding guidelines
src/memory/sync/state.rs-154-162 (1)
154-162: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winCanonicalize identity after hydrating keyed state.
A value loaded under one key can contain different
toolkitorconnection_idfields; the nextsavethen writes it under that other identity, crossing connection boundaries. Reject mismatches or overwrite both fields from the load arguments.Proposed fix
Some(value) => { let mut state: Self = serde_json::from_value(value)?; + state.toolkit = toolkit.to_owned(); + state.connection_id = connection_id.to_owned(); if state.daily_budget.date != today() {🤖 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/state.rs` around lines 154 - 162, Canonicalize the hydrated state in the state-loading method after serde_json::from_value and before returning it: overwrite its toolkit and connection_id fields with the toolkit and connection_id arguments used to build the key, or reject mismatches. Ensure subsequent save operations cannot write the loaded state under a different identity.src/memory/tree/flush.rs-41-45 (1)
41-45: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winDocument the services-aware flush contract.
Specify observer error behavior, embedding behavior, stale-buffer selection, and whether this function schedules follow-up work.
As per coding guidelines, “Document public APIs, module contracts, and non-obvious behavior thoroughly.”
🤖 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/flush.rs` around lines 41 - 45, Add thorough API documentation to the public function flush_stale_buffers_with_services, describing observer error handling, embedding behavior, stale-buffer selection criteria, and whether the function schedules follow-up work.Source: Coding guidelines
src/memory/retrieval/source.rs-40-40 (1)
40-40: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy liftHydrate only the final selected hits.
collect_source_hitsnow reads every full summary body before time filtering, ranking, andlimittruncation. Large source trees therefore incur O(all summaries) content-store I/O even when returning ten hits. Keep previews during scoring and hydrate only the final result set, preferably in a batch.Also applies to: 131-131
🤖 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/retrieval/source.rs` at line 40, Update collect_source_hits to retain lightweight previews while applying time filtering, ranking, and limit truncation, rather than hydrating every summary body upfront. Hydrate only the final selected RetrievalHit set, using a batch content-store fetch if available, and preserve hydrated_summary_hit for constructing the returned hits.src/memory/tree/bucket_seal.rs-257-264 (1)
257-264: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winDocument the new public sealing contracts.
Add item documentation covering force behavior, follow-up enqueue semantics, persistence boundaries, observer ordering, and document-version behavior.
As per coding guidelines, “Document public APIs, module contracts, and non-obvious behavior thoroughly.”
Also applies to: 299-328, 561-569
🤖 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/bucket_seal.rs` around lines 257 - 264, Add comprehensive documentation to the public sealing APIs, especially cascade_all_from_with_services and the related public items near the referenced sections. Document force behavior, enqueue_follow_ups semantics, persistence boundaries, observer ordering, and document-version handling, using Rust doc comments without changing implementation behavior.Source: Coding guidelines
src/memory/tree/summarise.rs-71-84 (1)
71-84: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winDocument the provider-facing summary API.
Add field docs for usage units and charge semantics, explain
effective_budget, document finalization behavior, and state that the defaultsummarise_with_usagereports zero usage.As per coding guidelines, “Document public APIs, module contracts, and non-obvious behavior thoroughly.”
Also applies to: 137-145, 165-174
🤖 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/summarise.rs` around lines 71 - 84, Document the public summary API around SummaryCall, PreparedSummaryPrompt, and the related summarise_with_usage flow: add field-level docs defining input_tokens and output_tokens as provider-reported usage units, charged_amount_usd as an optional provider charge, explain effective_budget, describe finalization behavior, and state that the default summarise_with_usage implementation reports zero usage.Source: Coding guidelines
src/memory/tree/bucket_seal.rs-554-832 (1)
554-832: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftSplit document-subtree sealing into a focused module.
This addition grows
bucket_seal.rsbeyond 800 lines. Move document batching andseal_explicit_childreninto a dedicated module while retaining the public façade here.As per coding guidelines, “Do not let any source file grow beyond 500 lines; split behavior into focused modules before reaching that point.”
🤖 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/bucket_seal.rs` around lines 554 - 832, Split the document-subtree sealing implementation out of bucket_seal.rs into a focused module: move batch_leaves_by_token_budget, batch_by_count, and seal_explicit_children there, along with their private constants and required imports. Keep seal_document_subtree_with_services as the public façade in bucket_seal.rs, delegating to the new module while preserving its existing behavior and API.Source: Coding guidelines
src/memory/tree/bucket_seal.rs-50-60 (1)
50-60: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDo not report a committed seal as failed when the observer fails.
Both callbacks run after durable commits but propagate errors with
?. Callers can retry an already-committed seal, producing stale snapshots or duplicate staged artifacts. Make notifications best-effort, or use a durable outbox with a result that explicitly distinguishes commit success from notification failure.Also applies to: 547-549, 824-829
🤖 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/bucket_seal.rs` around lines 50 - 60, Update the seal flow around SealObserver::progress and SealObserver::summary_committed so observer errors no longer propagate as seal failures after durable commits. Make notifications best-effort by handling and recording/logging callback errors while preserving the committed success result; apply the same behavior at the additional call sites noted in the review.src/memory/tree/bucket_seal.rs-586-597 (1)
586-597: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPrevent partially committed document subtrees.
Each concurrent batch commits independently before
try_collectcompletes. If a later batch fails, earlier summaries and backlinks remain persisted without being attached to the merge buffer. A retry creates new summary IDs and can re-parent level-zero chunks, leaving duplicate retrievable nodes. Persist the subtree atomically, or add idempotent recovery/cleanup before returning an error.Also applies to: 792-823
🤖 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/bucket_seal.rs` around lines 586 - 597, Update the subtree sealing flow around seal_explicit_children and the buffered try_collect so all batch summaries and backlinks are persisted atomically, or are fully cleaned up when any batch fails. Ensure a failed batch cannot leave earlier batches committed without attaching them to the merge buffer, and retries do not create duplicate summary nodes or re-parent level-zero chunks.src/memory/sync/composio/client.rs-1-3 (1)
1-3: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winNo unit tests colocated with this module.
This file has no
#[cfg(test)]module; retry/backoff decision helpers (retryable_provider_error,retryable_transport_error) are pure functions that would benefit from direct unit tests rather than relying solely on HTTP-mocked integration coverage intests/composio_sync_mock.rs.As per coding guidelines, "Add focused unit tests beside the module under
src/."🤖 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/client.rs` around lines 1 - 3, Add a colocated #[cfg(test)] module in the Composio client module with focused unit tests for the pure retryable_provider_error and retryable_transport_error helpers. Cover representative retryable and non-retryable provider and transport errors, without replacing or expanding the existing integration coverage.Source: Coding guidelines
src/memory/sync/audit.rs-12-43 (1)
12-43: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winPublic API lacks doc comments for non-obvious behavior.
SyncAuditEntryandRealCostAccumulator(and their public methods, e.g.combined_cost_usd/effective_cost_usd) have no doc comments explaining the newest-first ordering contract, the real-vs-estimated cost fallback semantics, or malformed-line handling.As per coding guidelines, "Document public APIs, module contracts, and non-obvious behavior thoroughly; prefer clear module-level docs and item docs over relying on implementation details."
🤖 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/audit.rs` around lines 12 - 43, Add module-level and item-level Rust doc comments for SyncAuditEntry, RealCostAccumulator, and their public methods, including combined_cost_usd and effective_cost_usd. Document the newest-first ordering contract, that actual charged cost falls back to estimated cost before adding Composio cost, and how malformed audit lines are handled. Keep the implementation unchanged.Source: Coding guidelines
src/memory/sync/audit.rs-45-56 (1)
45-56: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winNon-atomic two-step write risks interleaved/corrupted JSONL under concurrent callers.
serde_json::to_writerandwriteln!are two separatewrite()calls on an O_APPEND file. Ifappend_audit_entryis invoked concurrently (e.g. by multiple sync pipelines ticking in parallel), the writes can interleave, producing a malformed line thatread_audit_logsilently drops — quietly losing audit/cost data.🔒 Proposed fix: single atomic write
- serde_json::to_writer(&mut file, entry)?; - writeln!(file)?; + let mut payload = serde_json::to_vec(entry)?; + payload.push(b'\n'); + file.write_all(&payload)?;🤖 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/audit.rs` around lines 45 - 56, Update append_audit_entry to serialize the complete JSONL record, including its trailing newline, into one buffer before performing a single append write. Replace the separate serde_json::to_writer and writeln! calls while preserving the existing file-opening, logging, and error propagation behavior.src/memory/sync/composio/gmail.rs-90-118 (1)
90-118: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winAvoid falling back to
after:0for an unparseable cursor
Ifcursor_to_seconds(cursor)returnsNone, this turns the Gmail query intoafter:0, which removes the incremental cutoff and can rescan the whole mailbox. Fall back to the depth-based cutoff or skip settingqueryin that 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/sync/composio/gmail.rs` around lines 90 - 118, Update the cursor branch in arguments so an unparseable cursor does not use unwrap_or_default or produce an after:0 query. Only set the cursor-based query when cursor_to_seconds succeeds; otherwise fall through to the configured sync_depth_days cutoff or leave query unset, while preserving query_override precedence.src/memory/store/content/read.rs-133-185 (1)
133-185: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not silently return partial raw-backed content.
Each failed reference is skipped, and all failures return
Ok(""); because raw refs take precedence, the staged body is never attempted. Retrieval and summarization can therefore consume truncated content without an error.Make raw hydration all-or-nothing, or fall back to the staged pointer only when a complete raw body cannot be produced.
🤖 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/read.rs` around lines 133 - 185, The raw-reference path in read_chunk_body_from_raw must not silently return partial content. Track rejected, unreadable, invalid-UTF-8, and unusable references, then either return an error for any failure or make read_chunk_body fall back to the staged pointer via get_chunk_content_pointers when complete raw hydration is unavailable; preserve raw precedence only when every reference succeeds.src/memory/sync/composio/orchestrator.rs-378-400 (1)
378-400: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPersist pagination progress before reporting
more_pending.When the page cap is reached,
more_pendingprevents cursor advancement, butpage_tokenis discarded. The next run restarts at page one, consumes the same page limit while skipping deduplicated items, and can never reach the remaining pages.Persist a continuation token/query window in
SyncState, and add a regression test where the source exposes more thanmax_pages()pages across multiple runs.🤖 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/orchestrator.rs` around lines 378 - 400, Update the pagination flow around page_token, more_pending, and SyncState so reaching the max-pages cap persists a continuation token or query window before setting more_pending. Resume from that persisted continuation on the next run instead of restarting at page one, while preserving existing cursor advancement when pagination completes. Add a regression test covering a source with more than max_pages() pages across multiple runs.
🟡 Minor comments (8)
src/memory/chunks/store.rs-214-240 (1)
214-240: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFail fast when the checksum row is missing.
conn.execute(...)drops the affected-row count, so a stalechunk_id/summary_idis treated as success and the checksum-repair path silently does nothing.set_chunk_lifecycle_status_connalready bails onchanged == 0; these helpers should do the same so a lost row surfaces instead of being swallowed.🤖 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 214 - 240, Update update_chunk_content_sha256 and update_summary_content_sha256 to retain the conn.execute affected-row count and return an error when changed == 0, matching the fail-fast behavior of set_chunk_lifecycle_status_conn; preserve successful updates for existing rows.src/memory/chunks/store.rs-242-258 (1)
242-258: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPropagate
query_maperrors inlist_source_ids_with_prefix
filter_map(Result::ok)can silently truncate the list on a row decode failure; collect the iterator asrusqlite::Result<Vec<_>>so the error bubbles up like the other store helpers. If this set can grow, push the prefix predicate into SQL instead of filtering every distinctsource_idin Rust.🤖 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 242 - 258, Update list_source_ids_with_prefix to propagate row-decoding errors from query_map by collecting into a rusqlite::Result<Vec<String>> instead of discarding failures with filter_map(Result::ok). Apply the prefix constraint in SQL with the appropriate LIKE parameter so filtering occurs in the database, while preserving DISTINCT ordering and the existing Result return behavior.src/memory/chunks/store_embed_tests.rs-85-148 (1)
85-148: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTest never verifies the tombstone was actually cleared.
The test calls
clear_summary_reembed_skippedtwice (lines 145-146) but, unlike the siblingclear_chunk_reembed_skipped_is_idempotenttest (lines 65-83) which re-queries and assertscount == 0, this test has no final assertion after the clear calls. The line-range change summary states the test "verifies it has been removed (count goes to zero)", but the code as written only checks.unwrap()on theResult— it would pass even ifclear_summary_reembed_skippedsilently failed to delete the row.🐛 Proposed fix to actually verify removal
clear_summary_reembed_skipped(&cfg, "summary-1", "model@3").unwrap(); clear_summary_reembed_skipped(&cfg, "summary-1", "model@3").unwrap(); + let count: i64 = with_connection(&cfg, |conn| { + Ok(conn.query_row( + "SELECT COUNT(*) FROM mem_tree_summary_reembed_skipped + WHERE summary_id='summary-1' AND model_signature='model@3'", + [], + |row| row.get(0), + )?) + }) + .unwrap(); + assert_eq!(count, 0); }🤖 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_embed_tests.rs` around lines 85 - 148, Update the test summary_reembed_tombstone_roundtrips_and_clears to query mem_tree_summary_reembed_skipped after both clear_summary_reembed_skipped calls and assert the matching row count is zero, mirroring clear_chunk_reembed_skipped_is_idempotent while retaining the existing pre-clear count assertion.src/memory/sync/rebuild.rs-183-187 (1)
183-187: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDo not count unreadable files as successfully read.
This branch is reached only when no file produced an input, yet reports every pending file in
files_read.Proposed correction
if inputs.is_empty() { return Ok(RebuildOutcome { - files_read: coverage.pending.len(), + files_read: 0, ..RebuildOutcome::default() }); }🤖 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/rebuild.rs` around lines 183 - 187, Update the empty-input branch in the rebuild flow to report zero successfully read files instead of using coverage.pending.len(). Preserve the existing RebuildOutcome default behavior and early return for inputs.is_empty().src/memory/config_tests.rs-18-20 (1)
18-20: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winAssert that secret fields are omitted, not merely redacted.
These assertions still pass if
api_keyandbearer_tokenbegin serializing as"[REDACTED]", despite the#[serde(skip)]contract insrc/memory/config.rsLines 301-313.Proposed test
- let json = serde_json::to_string(&config).unwrap(); + let value = serde_json::to_value(&config).unwrap(); + assert!(value.get("api_key").is_none()); + assert!(value.get("bearer_token").is_none()); + let json = value.to_string(); assert!(!json.contains("super-secret")); assert!(!json.contains("bearer-secret"));🤖 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_tests.rs` around lines 18 - 20, Strengthen the serialization assertions in the config test around serde serialization so they verify the secret field names and redacted placeholder values are absent, not just the original secret contents. Preserve the existing #[serde(skip)] contract for api_key and bearer_token defined in the configuration type.src/memory/tree/bucket_seal.rs-282-290 (1)
282-290: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAvoid enqueueing work that this cascade immediately completes.
With
enqueue_follow_ups = true, sealing enqueues a ready parent, then the cascade advances to that same level and seals it synchronously. The queued job is stale by construction. Either stop cascading after enqueueing or enqueue only when the synchronous cascade stops.Also applies to: 528-535
🤖 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/bucket_seal.rs` around lines 282 - 290, Update the cascade flow around seal_one_level_with_services so it does not enqueue follow-up work for a parent level that the same synchronous cascade will immediately seal. When enqueue_follow_ups is enabled, either stop the cascade after enqueueing or defer enqueueing until the cascade terminates; apply the same correction to the corresponding flow near the other seal_one_level_with_services call.src/memory/tree/summarise.rs-137-145 (1)
137-145: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winApply the canonical output cap during finalization.
prepare_summary_promptlimits output to 5,000 tokens, butfinish_provider_summarycan return more wheneverbudgetis larger. Clamp against both limits.Proposed fix
pub fn finish_provider_summary(text: &str, budget: u32) -> SummaryOutput { - let (content, token_count) = clamp_to_budget(text.trim(), budget); + let effective_budget = budget.min(MAX_SUMMARY_OUTPUT_TOKENS); + let (content, token_count) = clamp_to_budget(text.trim(), effective_budget);🤖 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/summarise.rs` around lines 137 - 145, Update finish_provider_summary to cap the effective budget at the canonical 5,000-token output limit before calling clamp_to_budget, ensuring larger budget values cannot produce more than 5,000 tokens while preserving lower caller-provided budgets.src/memory/sync/composio/orchestrator.rs-118-131 (1)
118-131: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winEmit a terminal event when the budget is already exhausted.
Line 118 emits
Fetching, but the early return sends neitherCompletednorFailed, leaving event-driven status consumers stuck in a fetching state.Proposed fix
if state.budget_exhausted() { + emit( + context, + toolkit, + connection_id, + SyncStage::Completed, + Some("daily request budget exhausted".into()), + ) + .await; return Ok(SyncOutcome {🤖 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/orchestrator.rs` around lines 118 - 131, Update the budget-exhausted early-return branch in the sync orchestrator after SyncState::load to emit a terminal SyncStage event, preferably Completed, before returning the existing SyncOutcome. Preserve the current note and default outcome behavior while ensuring consumers do not remain in Fetching.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5ef8677f-8725-4b1d-b38d-1a3d10a5992c
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (89)
Cargo.tomlsrc/memory/chunks/connection.rssrc/memory/chunks/embeddings.rssrc/memory/chunks/mod.rssrc/memory/chunks/recovery.rssrc/memory/chunks/store.rssrc/memory/chunks/store_delete.rssrc/memory/chunks/store_embed_tests.rssrc/memory/chunks/store_sources.rssrc/memory/chunks/store_tests.rssrc/memory/config.rssrc/memory/config_tests.rssrc/memory/goals/types.rssrc/memory/graph/bfs.rssrc/memory/graph/edge_store.rssrc/memory/graph/mod.rssrc/memory/ingest/extract/mod.rssrc/memory/ingest/extract/types.rssrc/memory/ingest/mod.rssrc/memory/ingest/types.rssrc/memory/mod.rssrc/memory/retrieval/cover.rssrc/memory/retrieval/cover_tests.rssrc/memory/retrieval/drill_down.rssrc/memory/retrieval/fast.rssrc/memory/retrieval/fetch.rssrc/memory/retrieval/fetch_tests.rssrc/memory/retrieval/mod.rssrc/memory/retrieval/source.rssrc/memory/retrieval/types.rssrc/memory/sources/mod.rssrc/memory/sources/registry.rssrc/memory/sources/types.rssrc/memory/store/content/atomic.rssrc/memory/store/content/content_tests.rssrc/memory/store/content/mod.rssrc/memory/store/content/raw.rssrc/memory/store/content/read.rssrc/memory/store/content/read_tests.rssrc/memory/store/entity_index/mod.rssrc/memory/store/entity_index/store.rssrc/memory/store/entity_index/store_tests.rssrc/memory/store/entity_index/types.rssrc/memory/store/kv.rssrc/memory/store/kv_tests.rssrc/memory/store/safety/mod.rssrc/memory/store/safety/pii.rssrc/memory/store/safety/safety_tests.rssrc/memory/store/vectors/embedding.rssrc/memory/sync/audit.rssrc/memory/sync/composio/client.rssrc/memory/sync/composio/gmail.rssrc/memory/sync/composio/mod.rssrc/memory/sync/composio/orchestrator.rssrc/memory/sync/composio/providers/clickup.rssrc/memory/sync/composio/providers/common.rssrc/memory/sync/composio/providers/github.rssrc/memory/sync/composio/providers/linear.rssrc/memory/sync/composio/providers/mod.rssrc/memory/sync/composio/providers/notion.rssrc/memory/sync/composio/providers/slack.rssrc/memory/sync/dispatcher.rssrc/memory/sync/github.rssrc/memory/sync/mod.rssrc/memory/sync/periodic.rssrc/memory/sync/rebuild.rssrc/memory/sync/state.rssrc/memory/sync/status.rssrc/memory/sync/traits.rssrc/memory/sync/workspace.rssrc/memory/tree/bucket_seal.rssrc/memory/tree/bucket_seal_label_tests.rssrc/memory/tree/bucket_seal_tests.rssrc/memory/tree/direct_ingest.rssrc/memory/tree/flush.rssrc/memory/tree/hydrate.rssrc/memory/tree/mod.rssrc/memory/tree/runtime/engine.rssrc/memory/tree/runtime/engine_tests.rssrc/memory/tree/runtime/mod.rssrc/memory/tree/store/buffers.rssrc/memory/tree/store/mod.rssrc/memory/tree/store/store_tests.rssrc/memory/tree/store/summaries.rssrc/memory/tree/store/trees.rssrc/memory/tree/summarise.rssrc/memory/tree/summarise_tests.rstests/composio_sync_live.rstests/composio_sync_mock.rs
There was a problem hiding this comment.
💡 Codex Review
Reapply the folder glob in direct reads
When a caller invokes SourceReader::read_item directly with an item id, this path only verifies containment under the folder root and never checks the source glob used by list_items. A folder source scoped to something like docs/**/*.md can therefore be made to read another contained file such as docs/.env that would never be listed; reject item ids that do not match the configured glob before reading the file.
ℹ️ 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".
| if config | ||
| .sync | ||
| .budget | ||
| .max_items | ||
| .is_some_and(|limit| ingested >= limit) |
There was a problem hiding this comment.
When Composio sources are created through SourceRegistry, the default max_items/sync_depth_days caps live on the MemorySourceEntry, but this limit check only consults config.sync.budget. If the host registers a source with the registry defaults and leaves the global budget unset, the per-toolkit caps are ignored and the provider can fetch up to the pipeline page limits instead of the configured source cap; resolve the effective budget from the source entry before running the incremental loop.
Useful? React with 👍 / 👎.
| #[async_trait] | ||
| impl SyncPipeline for GmailSyncPipeline { | ||
| fn id(&self) -> &str { | ||
| "composio:gmail" |
There was a problem hiding this comment.
For users with two Gmail Composio connections, both GmailSyncPipeline instances return the same id, and SyncDispatcher::register rejects duplicate pipeline ids even though the source registry keys Composio sources by connection_id. This prevents registering/syncing a second Gmail account; include the connection id in the pipeline id, as the workspace pipelines already do.
Useful? React with 👍 / 👎.
| ) | ||
| .await; | ||
| } | ||
| resolve_local(config, candidates, source_scope, limit) |
There was a problem hiding this comment.
When query entity ids produce graph pairs, this local branch bypasses the dense/global paths that pass options.time_window_days into query_source; resolve_local receives only scope and limit, so a request for a recent window can still return old co-occurrence summaries or leaves. Filter the resolved local hits by the requested window before computing totals/truncating.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e259125402
ℹ️ 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".
| [&tree.id], | ||
| |row| Ok((row.get(0)?, row.get(1)?)), | ||
| )?; | ||
| if current_root.as_deref() != Some(root_id.as_str()) || root_level > current_max { |
There was a problem hiding this comment.
Fresh evidence after the previous root-refresh thread: this condition still takes the update path whenever the new document root id differs, even if current_max is already a cross-document merge level. After a source has sealed a merge root at MERGE_LEVEL_BASE + 1, a later edit to one document seals a low-level document root, this branch calls update_tree_after_seal_tx with that lower root_level, downgrading max_level and root_id; query_source then only scans up to the downgraded level and skips the existing merge summaries.
Useful? React with 👍 / 👎.
| let pii = pii::redact_pii(&out); | ||
| report = report.merge(pii.report); | ||
| out = pii.value; |
There was a problem hiding this comment.
For KV/JSON values that contain ordinary email addresses, sanitize_json delegates string values to sanitize_text, but this now only runs the national-ID/phone scrubber and never applies the separate has_likely_email detector. A value such as { "contact": "alice@example.com" } is therefore stored unchanged even though the previous sanitizer redacted email PII and this module still documents content PII scrubbing.
Useful? React with 👍 / 👎.
1 participant
Summary
Complete the reusable memory-engine ports required by OpenHuman: shared storage/config primitives, source registry/local readers, deterministic ingestion, queue and sync engines, tree sealing/runtime/summarisation, graph and fast retrieval, and persistence-safe hydration.
The crate now owns the engine behavior while host applications inject credentials, providers, event sinks, scheduling, and product policy through explicit seams.
Depends on tinyagents PR #58 for the expanded embedding model API and providers. Until that merges/releases, Cargo pins the exact reviewed fork revision so this branch is independently reproducible.
API Or Behavior Changes
syncsupport and Composio pipeline seams.MemoryConfigwith content-root and sync configuration.has_likely_emailseparately from generic PII boundary detection so scanner identifiers remain compatible.Tests
cargo fmt --checkcargo clippy --all-targets -- -D warnings- not run locallycargo build --all-targets- covered by library tests and host all-target compilecargo test-cargo test --lib --quiet: 1,118 passedAdditional validation:
cargo test --features sync --test composio_sync_mock: 12 passedGGML_NATIVE=OFF cargo check --tests: passed across all host test targetsDocumentation
Public APIs and ownership seams are documented inline. The corresponding migration ownership and integration documentation is maintained in the OpenHuman PR.
Summary by CodeRabbit