feat(memory): clear source memory on disconnect#2528
Open
YOMXXX wants to merge 5 commits into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughAdds an opt-in "also delete memory" checkbox in disconnect UIs (Discord, Telegram, Composio), wires the flag through frontend APIs to new RPC parameters, and removes matching memory-tree chunks via new store deletion APIs; responses surface deleted-chunk counts and tests cover success/error flows. ChangesMemory deletion on channel/Composio disconnect
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
coderabbitai
Bot
added
feature
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/components/composio/ComposioConnectModal.tsx (1)
507-522:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReset
clearMemoryOnDisconnecton error recovery.If
deleteConnectionfails, the checkbox state persists when the user dismisses the error (line 787) and returns to the 'connected' phase. On retry, the checkbox will still appear checked, which may confuse users who expected the form to reset.🔄 Proposed fix
Add the state reset in the error handler:
} catch (err) { const msg = err instanceof Error ? err.message : String(err); setPhase('error'); setError(`${t('composio.connect.disconnectFailed')}: ${msg}`); + setClearMemoryOnDisconnect(false); }🤖 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 `@app/src/components/composio/ComposioConnectModal.tsx` around lines 507 - 522, The error handler in handleDisconnect does not reset the clearMemoryOnDisconnect checkbox state when deleteConnection fails, so after dismissing the error the checkbox remains checked; update the catch block in handleDisconnect to reset the checkbox by calling setClearMemoryOnDisconnect(false) (in addition to setting phase and error) and ensure any recovery path that sets phase back to 'connected' also clears this state; reference functions/vars: handleDisconnect, deleteConnection, setClearMemoryOnDisconnect, setPhase, setError, onChanged.
🧹 Nitpick comments (1)
app/src/components/composio/ComposioConnectModal.test.tsx (1)
226-243: 💤 Low valueConsider testing state reset behavior (optional).
The test verifies the API parameter is passed correctly, but doesn't verify that
clearMemoryOnDisconnectis reset tofalseafter a successful disconnect. While the current coverage is sufficient for the primary requirement, adding assertions for state reset would catch regressions if that logic changes.Optional test enhancement
You could extend this test or add a separate one to verify the checkbox becomes unchecked after disconnect completes:
await waitFor(() => { expect(composioApi.deleteConnection).toHaveBeenCalledWith('ca_xyz', { clearMemory: true }); }); // Verify checkbox resets (if modal remains open in a different state) // or that subsequent disconnect calls would default to clearMemory: false🤖 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 `@app/src/components/composio/ComposioConnectModal.test.tsx` around lines 226 - 243, Extend the test for ComposioConnectModal to assert that the component's clearMemoryOnDisconnect state resets after a successful disconnect: after the existing waitFor that checks composioApi.deleteConnection was called with ('ca_xyz', { clearMemory: true }), add a follow-up assertion that the "also delete memory" checkbox (or the clearMemoryOnDisconnect state) is false/unchecked, or trigger a second disconnect and assert composioApi.deleteConnection is called with clearMemory: false; reference ComposioConnectModal, the clearMemoryOnDisconnect state/checkbox label "also delete memory", and composioApi.deleteConnection to locate where to add the assertion.
🤖 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 `@app/src/lib/i18n/ko.ts`:
- Around line 345-347: Replace the English values for the locale keys
accounts.disconnectClearMemory and accounts.disconnectClearMemoryHint with
Korean text: set accounts.disconnectClearMemory to a concise label like "이 소스의
메모리도 삭제" and set accounts.disconnectClearMemoryHint to a descriptive hint like
"이 연결과 연관된 로컬 메모리 조각을 영구적으로 삭제합니다." Ensure you update the string values for
those exact keys in the ko.ts translations so the disconnect UI shows Korean
text.
In `@src/openhuman/composio/ops.rs`:
- Around line 521-524: The match branch for toolkit currently only returns
sources for "slack" and "gmail" (using gmail_memory_sources_for_connection) and
falls back to Vec::new(), causing clear_memory to be a no-op for other Composio
memory-backed toolkits; update the default branch so it derives or queries
actual memory source IDs for any toolkit instead of returning an empty Vec (for
example by adding a helper like memory_sources_for_toolkit(toolkit,
connection_id) or calling the Composio API to list sources for the
connection_id), ensure the helper returns tuples of (SourceKind, source_id)
consistent with the Slack/Gmail branches, and wire that in where clear_memory
uses the match so memory_chunks_deleted reflects actual deletions.
- Around line 436-444: The loop calling
memory_tree_store::delete_chunks_by_source currently uses ? which returns early
and skips identity-facet cleanup, cache invalidation, and publishing
ComposioConnectionDeleted; change it to capture or log errors instead of
returning: replace the ? with code that pushes any error (with context including
source_id) into a local Vec<Error> or logs it, and continue the loop to ensure
identity-facet cleanup and cache invalidation still run and
ComposioConnectionDeleted is published; after all cleanup steps complete, if the
Vec is non-empty return or aggregate the errors into a single Err so callers
still see failures while local cleanup always runs.
In `@src/openhuman/memory/tree/store.rs`:
- Around line 822-833: remove_chunk_content_files currently appends DB-sourced
content_paths directly to config.memory_tree_content_root and can be exploited
for path traversal; instead, for each rel in content_paths (and in function
remove_chunk_content_files) resolve the candidate path safely by joining rel
components with the root via Path/PathBuf, then canonicalize both the root
(config.memory_tree_content_root()) and the candidate and verify the
canonicalized candidate starts_with the canonicalized root; if it does not, skip
removing and log a warning mentioning the redacted rel; only call
std::fs::remove_file for candidates that pass this containment check and handle
NotFound as before.
---
Outside diff comments:
In `@app/src/components/composio/ComposioConnectModal.tsx`:
- Around line 507-522: The error handler in handleDisconnect does not reset the
clearMemoryOnDisconnect checkbox state when deleteConnection fails, so after
dismissing the error the checkbox remains checked; update the catch block in
handleDisconnect to reset the checkbox by calling
setClearMemoryOnDisconnect(false) (in addition to setting phase and error) and
ensure any recovery path that sets phase back to 'connected' also clears this
state; reference functions/vars: handleDisconnect, deleteConnection,
setClearMemoryOnDisconnect, setPhase, setError, onChanged.
---
Nitpick comments:
In `@app/src/components/composio/ComposioConnectModal.test.tsx`:
- Around line 226-243: Extend the test for ComposioConnectModal to assert that
the component's clearMemoryOnDisconnect state resets after a successful
disconnect: after the existing waitFor that checks composioApi.deleteConnection
was called with ('ca_xyz', { clearMemory: true }), add a follow-up assertion
that the "also delete memory" checkbox (or the clearMemoryOnDisconnect state) is
false/unchecked, or trigger a second disconnect and assert
composioApi.deleteConnection is called with clearMemory: false; reference
ComposioConnectModal, the clearMemoryOnDisconnect state/checkbox label "also
delete memory", and composioApi.deleteConnection to locate where to add the
assertion.
🪄 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: 313e1116-213d-4c44-a6b8-e5aac8a5f882
📒 Files selected for processing (36)
app/src/components/channels/DiscordConfig.tsxapp/src/components/channels/TelegramConfig.tsxapp/src/components/channels/__tests__/TelegramConfig.test.tsxapp/src/components/composio/ComposioConnectModal.test.tsxapp/src/components/composio/ComposioConnectModal.tsxapp/src/lib/composio/composioApi.test.tsapp/src/lib/composio/composioApi.tsapp/src/lib/composio/types.tsapp/src/lib/i18n/chunks/ar-1.tsapp/src/lib/i18n/chunks/bn-1.tsapp/src/lib/i18n/chunks/de-1.tsapp/src/lib/i18n/chunks/en-1.tsapp/src/lib/i18n/chunks/es-1.tsapp/src/lib/i18n/chunks/fr-1.tsapp/src/lib/i18n/chunks/hi-1.tsapp/src/lib/i18n/chunks/id-1.tsapp/src/lib/i18n/chunks/it-1.tsapp/src/lib/i18n/chunks/ko-1.tsapp/src/lib/i18n/chunks/pt-1.tsapp/src/lib/i18n/chunks/ru-1.tsapp/src/lib/i18n/chunks/zh-CN-1.tsapp/src/lib/i18n/en.tsapp/src/lib/i18n/ko.tsapp/src/services/api/__tests__/channelConnectionsApi.test.tsapp/src/services/api/channelConnectionsApi.tssrc/openhuman/channels/controllers/ops.rssrc/openhuman/channels/controllers/ops_tests.rssrc/openhuman/channels/controllers/schemas.rssrc/openhuman/channels/controllers/schemas_tests.rssrc/openhuman/composio/ops.rssrc/openhuman/composio/ops_test.rssrc/openhuman/composio/providers/profile.rssrc/openhuman/composio/schemas.rssrc/openhuman/composio/types.rssrc/openhuman/memory/tree/store.rssrc/openhuman/memory/tree/store_tests.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/openhuman/composio/ops_test.rs (1)
470-564: ⚡ Quick winConsider adding error/failure case tests for memory cleanup.
The new memory cleanup tests cover happy paths well, but per the PR objectives ("Error handling: if clear_memory=true and deletion fails...surface an actionable error"), error scenarios should also be tested. Consider adding tests for:
- Memory cleanup failure when
clear_memory=true(e.g., simulating store errors during chunk deletion)- Partial cleanup failures (some targets succeed, others fail)
- Error message format when cleanup fails
Suggested test scenario
#[tokio::test] async fn composio_delete_connection_reports_memory_cleanup_failures() { // Set up config with invalid/broken memory store path // or seed chunks that trigger deletion errors // Call composio_delete_connection(&config, "c1", true) // Assert that the error message surfaces actionable detail // about the memory cleanup failure (not just connection deletion) }🤖 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/openhuman/composio/ops_test.rs` around lines 470 - 564, Add a test named composio_delete_connection_reports_memory_cleanup_failures that calls composio_delete_connection(&config, "c1", true) but arranges the memory store to fail (for example create a temp workspace, seed chunks via memory_tree_store::upsert_chunks or SyncState::save, then make the workspace path unreadable/removed or use a MemoryClient pointing at a corrupted/nonexistent dir so deletions error); assert the returned Result is Err and the error string includes actionable details mentioning memory cleanup and the failing target(s) (referencing composio_delete_connection, composio_memory_targets_for_connection, MemoryCleanupTarget, memory_tree_store::upsert_chunks/list_chunks, SyncState and MemoryClient to locate relevant setup/seed code).
🤖 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/openhuman/composio/ops.rs`:
- Around line 605-649: The helper notion_memory_targets_for_connection currently
swallows errors from MemoryClient::from_workspace_dir and SyncState::load and
returns a best-effort Vec, causing callers to miss hard failures; change
notion_memory_targets_for_connection to return a
Result<Vec<MemoryCleanupTarget>, E> (propagate the error returned by
MemoryClient::from_workspace_dir and SyncState::load back to the caller instead
of tracing::warn and continuing), only treat the specific benign "no sync state
yet" SyncState::load error as a no-op, and keep dedupe_memory_targets(targets)
on the success path so callers (e.g., the delete/clear flow) can surface and
handle failures rather than silently leaving Notion page-scoped targets behind.
In `@src/openhuman/memory/tree/store.rs`:
- Around line 855-876: The code currently canonicalizes root.join(rel_path) into
path and then calls std::fs::remove_file(&path), which removes the canonical
target (following symlinks); instead, keep the original entry and only use the
canonicalized path for the containment check: compute original_path =
root.join(rel_path), canonicalize(&original_path) for the NotFound/containment
checks, and after confirming it starts_with(&canonical_root) call
std::fs::remove_file(&original_path) (not &path); update the log/error handling
to reference original_path/rel as needed and preserve the existing warnings.
---
Nitpick comments:
In `@src/openhuman/composio/ops_test.rs`:
- Around line 470-564: Add a test named
composio_delete_connection_reports_memory_cleanup_failures that calls
composio_delete_connection(&config, "c1", true) but arranges the memory store to
fail (for example create a temp workspace, seed chunks via
memory_tree_store::upsert_chunks or SyncState::save, then make the workspace
path unreadable/removed or use a MemoryClient pointing at a
corrupted/nonexistent dir so deletions error); assert the returned Result is Err
and the error string includes actionable details mentioning memory cleanup and
the failing target(s) (referencing composio_delete_connection,
composio_memory_targets_for_connection, MemoryCleanupTarget,
memory_tree_store::upsert_chunks/list_chunks, SyncState and MemoryClient to
locate relevant setup/seed code).
🪄 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: c572c3ff-e807-4daa-85fe-1f13cac6e552
📒 Files selected for processing (8)
app/src/components/composio/ComposioConnectModal.test.tsxapp/src/components/composio/ComposioConnectModal.tsxapp/src/lib/i18n/chunks/ko-1.tsapp/src/lib/i18n/ko.tssrc/openhuman/composio/ops.rssrc/openhuman/composio/ops_test.rssrc/openhuman/memory/tree/store.rssrc/openhuman/memory/tree/store_tests.rs
✅ Files skipped from review due to trivial changes (1)
- app/src/lib/i18n/chunks/ko-1.ts
Contributor
|
placeholder |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/openhuman/memory/tree/store.rs (1)
719-726:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject empty prefixes before deleting.
starts_with("")matches everysource_id, so an accidentally emptysource_id_prefixwill delete all chunks and ingest-gate rows for thatSourceKind. This helper is public and sits on a destructive path, so it should fail closed.🔧 Suggested fix
pub fn delete_chunks_by_source_prefix( config: &Config, source_kind: SourceKind, source_id_prefix: &str, ) -> Result<usize> { + if source_id_prefix.is_empty() { + anyhow::bail!("source_id_prefix must not be empty"); + } delete_chunks_by_source_filter(config, source_kind, |candidate| { candidate.starts_with(source_id_prefix) }) }🤖 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/openhuman/memory/tree/store.rs` around lines 719 - 726, The public helper delete_chunks_by_source_prefix currently allows an empty source_id_prefix (since starts_with("") matches everything); modify delete_chunks_by_source_prefix to validate source_id_prefix is non-empty and return a failing Result (Err) when it's empty instead of calling delete_chunks_by_source_filter, so accidental empty prefixes cannot delete all rows; reference the function delete_chunks_by_source_prefix and propagate a clear error message via the crate Result type before invoking delete_chunks_by_source_filter.
🧹 Nitpick comments (1)
src/openhuman/memory/tree/store.rs (1)
707-724: ⚡ Quick winPush exact-source deletes into SQL.
The exact path currently scans every row for the
source_kindin bothmem_tree_chunksandmem_tree_ingested_sources, then filterssource_idin Rust. That bypasses the existing(source_kind, source_id)index/PK on a user-facing disconnect path. Splitting exact and prefix handling would keep the Rust-side filter only where it is actually needed.Also applies to: 739-743, 788-793
🤖 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/openhuman/memory/tree/store.rs` around lines 707 - 724, The current delete_chunks_by_source implementation scans rows and filters source_id in Rust, bypassing the (source_kind, source_id) index; change it so delete_chunks_by_source performs direct SQL deletes (DELETE ... WHERE source_kind = ? AND source_id = ?) against mem_tree_chunks and mem_tree_ingested_sources to use the PK/index, and reserve delete_chunks_by_source_filter/delete_chunks_by_source_prefix for true prefix logic only (keep the Rust-side predicate there). Update the paths that currently call delete_chunks_by_source_filter for exact matches (e.g., callers around delete_chunks_by_source, delete_chunks_by_source_prefix, and any code affecting mem_tree_ingested_sources) to call the new exact-SQL deletion code so exact-source deletes hit the DB index. Ensure the SQL deletes return the affected-row counts to match the current Result<usize> behavior.
🤖 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.
Outside diff comments:
In `@src/openhuman/memory/tree/store.rs`:
- Around line 719-726: The public helper delete_chunks_by_source_prefix
currently allows an empty source_id_prefix (since starts_with("") matches
everything); modify delete_chunks_by_source_prefix to validate source_id_prefix
is non-empty and return a failing Result (Err) when it's empty instead of
calling delete_chunks_by_source_filter, so accidental empty prefixes cannot
delete all rows; reference the function delete_chunks_by_source_prefix and
propagate a clear error message via the crate Result type before invoking
delete_chunks_by_source_filter.
---
Nitpick comments:
In `@src/openhuman/memory/tree/store.rs`:
- Around line 707-724: The current delete_chunks_by_source implementation scans
rows and filters source_id in Rust, bypassing the (source_kind, source_id)
index; change it so delete_chunks_by_source performs direct SQL deletes (DELETE
... WHERE source_kind = ? AND source_id = ?) against mem_tree_chunks and
mem_tree_ingested_sources to use the PK/index, and reserve
delete_chunks_by_source_filter/delete_chunks_by_source_prefix for true prefix
logic only (keep the Rust-side predicate there). Update the paths that currently
call delete_chunks_by_source_filter for exact matches (e.g., callers around
delete_chunks_by_source, delete_chunks_by_source_prefix, and any code affecting
mem_tree_ingested_sources) to call the new exact-SQL deletion code so
exact-source deletes hit the DB index. Ensure the SQL deletes return the
affected-row counts to match the current Result<usize> behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6403beb9-05b8-4cc6-a131-a93c2b56fb80
📒 Files selected for processing (5)
app/src/components/channels/__tests__/DiscordConfig.test.tsxsrc/openhuman/composio/ops.rssrc/openhuman/composio/ops_test.rssrc/openhuman/memory/tree/store.rssrc/openhuman/memory/tree/store_tests.rs
Contributor
Author
|
@graycyrus @senamakel Ready for review. Latest state for #2528 (feat(memory): clear source memory on disconnect):
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
clearMemory/clear_memorythrough channel and Composio disconnect flows, returningmemory_chunks_deletedcounts.Related Issue
Closes #2515
Type of Change
How Tested
pnpm --filter openhuman-app test:unit src/services/api/__tests__/channelConnectionsApi.test.ts src/lib/composio/composioApi.test.ts src/components/composio/ComposioConnectModal.test.tsx src/components/channels/__tests__/TelegramConfig.test.tsxpnpm --filter openhuman-app test:unit src/components/channels/__tests__/DiscordConfig.test.tsxpnpm --filter openhuman-app exec vitest run --coverage --config test/vitest.config.ts src/components/channels/__tests__/DiscordConfig.test.tsx src/components/channels/__tests__/TelegramConfig.test.tsx src/components/composio/ComposioConnectModal.test.tsx src/services/api/__tests__/channelConnectionsApi.test.ts src/lib/composio/composioApi.test.tsGGML_NATIVE=OFF cargo test --manifest-path Cargo.toml clear_memory --libGGML_NATIVE=OFF cargo test --manifest-path Cargo.toml cleanup_targets --libGGML_NATIVE=OFF cargo test --manifest-path Cargo.toml composio::ops --libGGML_NATIVE=OFF cargo test --manifest-path Cargo.toml delete_chunks_by_source --libGGML_NATIVE=OFF cargo test --manifest-path Cargo.toml memory::tree::store --libpnpm i18n:checkpnpm typecheckGGML_NATIVE=OFF cargo check --manifest-path Cargo.tomlpnpm lint(passes with existing warnings)pnpm format:checkgit diff --checkGGML_NATIVE=OFF pnpm rust:checkGGML_NATIVE=OFF git push(pre-push hook passed)Screenshots / Recordings
Not applicable.
Checklist
AI Authorship Checklist
6e1eaa2632d7ea010dd54b4cb1a797deabd527de.Summary by CodeRabbit
New Features
Localization