feat(core): opt-in memory deletion on channel / composio disconnect#2546
Open
mengqiuzhen wants to merge 1 commit into
Open
feat(core): opt-in memory deletion on channel / composio disconnect#2546mengqiuzhen wants to merge 1 commit into
mengqiuzhen wants to merge 1 commit into
Conversation
Contributor
📝 WalkthroughWalkthroughAdds an opt-in "clear memory" checkbox to disconnect flows (channels and Composio), forwards the flag through updated frontend API clients to core RPCs, implements backend deletion of mem_tree_chunks by source pattern, and updates tests and i18n strings accordingly. ChangesMemory cleanup and disconnect flows
Frontend API and component implementations
Sequence DiagramsequenceDiagram
participant User
participant Frontend as App UI
participant APIClient as channelConnectionsApi/composioApi
participant Core as core RPC (openhuman)
participant Ops as backend ops (disconnect/composio_delete_connection)
participant Store as memory.tree.store
User->>Frontend: Click "Disconnect" -> open confirmation (clearMemory?)
Frontend->>APIClient: call disconnectChannel/deleteConnection(clearMemory)
APIClient->>Core: coreRpc call openhuman.channels_disconnect / composio_delete_connection with clear_memory
Core->>Ops: invoke disconnect/composio op with clear_memory
Ops->>Store: delete_chunks_by_source(kind, pattern) if clear_memory
Store-->>Ops: deleted_count / error
Ops-->>Core: rpc response including memory_clear
Core-->>APIClient: rpc result
APIClient-->>Frontend: resolve and update UI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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
mengqiuzhen
force-pushed
the
feat/clear-memory-on-disconnect
branch
from
a8982eb to
5353a75
Compare
… composio connections Add `clear_memory` parameter to `channels.disconnect` and `composio.delete_connection` RPCs. When set, all memory tree chunks indexed under the source are deleted in a single transaction before credentials are revoked. A confirmation checkbox in the UI (unchecked by default) makes the irreversibility explicit. - memory/tree/store: add `delete_chunks_by_source` helper - channels/composio ops: thread `clear_memory` through disconnect paths - frontend: add confirmation dialog with opt-in checkbox in DiscordConfig, TelegramConfig, and ComposioConnectModal Also translates the custom GIF mascot strings for zh-CN.
![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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/src/components/channels/DiscordConfig.tsx (1)
363-377:⚠️ Potential issue | 🟠 Major | ⚡ Quick winManaged DM disconnect cannot reach the confirmation flow
For
managed_dmin connected state, clicking Disconnect only setsconfirmingDisconnect, but thespec.mode === 'managed_dm' && status === 'connected'branch keeps rendering the same simple button. The confirm UI never appears, so the disconnect API path is never reached for this mode.Proposed minimal fix
- {spec.mode === 'managed_dm' && status === 'connected' ? ( + {spec.mode === 'managed_dm' && + status === 'connected' && + confirmingDisconnect !== spec.mode ? ( <div className="mt-3 flex items-center justify-between"> <p className="text-xs text-sage-700 dark:text-sage-300 font-medium"> {t('channels.discord.accountLinked')} </p> <buttonAlso applies to: 388-430
🤖 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/channels/DiscordConfig.tsx` around lines 363 - 377, The managed_dm branch never shows the confirm UI because it always renders the simple Disconnect button; change the JSX condition so that when confirmingDisconnect (the component state) is true for spec.mode === 'managed_dm' and status === 'connected' it renders the same confirmation UI used for other modes (Cancel button should call setConfirmingDisconnect(false), Confirm should invoke handleDisconnect to perform the actual disconnect), otherwise render the simple Disconnect button; in short, update the conditional around spec.mode === 'managed_dm' to consider confirmingDisconnect and reuse the existing confirmation buttons/handlers (confirmingDisconnect, setConfirmingDisconnect, handleDisconnect) so the API disconnect path can be reached.src/openhuman/composio/schemas.rs (1)
750-760:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUpdate the
delete_connectioncontroller schema to match the new runtime contract.Line 759 now forwards
clear_memory, but the schema fordelete_connectionstill only declaresconnection_idinput anddeletedoutput. This creates RPC contract drift for schema-driven callers/docs.💡 Suggested schema update
"delete_connection" => ControllerSchema { namespace: "composio", function: "delete_connection", description: "Delete a Composio connection owned by the caller.", - inputs: vec![FieldSchema { - name: "connection_id", - ty: TypeSchema::String, - comment: "Identifier of the connection to delete.", - required: true, - }], - outputs: vec![FieldSchema { - name: "deleted", - ty: TypeSchema::Bool, - comment: "True when the backend confirmed the deletion.", - required: true, - }], + inputs: vec![ + FieldSchema { + name: "connection_id", + ty: TypeSchema::String, + comment: "Identifier of the connection to delete.", + required: true, + }, + FieldSchema { + name: "clear_memory", + ty: TypeSchema::Option(Box::new(TypeSchema::Bool)), + comment: "When true, also attempts to clear ingested memory for this connection. Default false.", + required: false, + }, + ], + outputs: vec![ + FieldSchema { + name: "deleted", + ty: TypeSchema::Bool, + comment: "True when the backend confirmed the deletion.", + required: true, + }, + FieldSchema { + name: "memory_clear", + ty: TypeSchema::Option(Box::new(TypeSchema::Json)), + comment: "Optional memory-clear outcome payload when clear_memory is requested.", + required: 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 `@src/openhuman/composio/schemas.rs` around lines 750 - 760, The controller schema for delete_connection is out of sync: handle_delete_connection now forwards an optional clear_memory boolean to super::ops::composio_delete_connection but the schema still only lists connection_id input and deleted output. Update the delete_connection schema in schemas.rs to add an optional "clear_memory" input (boolean, default false) and adjust the response schema to match the actual return shape from composio_delete_connection (e.g., include both "deleted" and any "memory_cleared" or other fields it returns) so schema-driven callers/docs match handle_delete_connection and super::ops::composio_delete_connection.
🧹 Nitpick comments (2)
app/src/lib/composio/composioApi.ts (1)
114-123: ⚡ Quick winDocument the new
clearMemoryparameter in JSDoc for consistency.The
channelConnectionsApi.disconnectChannelfunction includes JSDoc documenting theclearMemoryparameter (lines 148-149 inchannelConnectionsApi.ts). For consistency and maintainability, this function should also document the new parameter's behavior.📝 Suggested JSDoc addition
/** * Delete an existing Composio connection. Backend verifies ownership * before forwarding to Composio. + * Set `clearMemory` to also delete ingested memory chunks for this connection. */ export async function deleteConnection(🤖 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/lib/composio/composioApi.ts` around lines 114 - 123, Add a JSDoc block to the deleteConnection function documenting the new clearMemory parameter: describe that clearMemory is an optional boolean that defaults to false and controls whether associated memory/state should be cleared when deleting the connection; include the parameter tag (`@param` clearMemory) and a short return description for Promise<ComposioDeleteResponse>, and place it immediately above the export async function deleteConnection declaration so it mirrors the documentation style used in channelConnectionsApi.disconnectChannel.app/src/components/channels/__tests__/DiscordConfig.test.tsx (1)
46-92: ⚡ Quick winAdd one test for
managed_dmdisconnect confirmation pathThis test only validates
bot_token. A connectedmanaged_dmcase should also assert that confirmation appears anddisconnectChannel('discord', 'managed_dm', ...)is invoked, so mode-specific regressions are caught.🤖 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/channels/__tests__/DiscordConfig.test.tsx` around lines 46 - 92, Add a new test case in DiscordConfig.test.tsx that mirrors the existing bot_token test but uses a connected managed_dm entry: render DiscordConfig with preloadedState containing connections.discord.managed_dm { channel: 'discord', authMode: 'managed_dm', status: 'connected', ... }, mock channelConnectionsApi.disconnectChannel, locate and click the enabled "Disconnect" for the managed_dm entry, assert the disconnect confirmation dialog appears (e.g., "Yes, disconnect") and any mode-specific text, optionally toggle the clear-memory checkbox, then verify channelConnectionsApi.disconnectChannel was called with ('discord', 'managed_dm', <expectedBoolean>) to ensure the managed_dm path invokes the API.
🤖 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/components/channels/__tests__/TelegramConfig.test.tsx`:
- Around line 127-131: The test currently grabs all "Disconnect" buttons via
screen.getAllByText and uses index-based selection (disconnectBtns[1] ->
botTokenBtn), which is fragile; instead locate the bot_token-specific section or
find the enabled Disconnect button directly (e.g., scope queries around the
element that contains the bot_token label or use screen.getByRole/getByText
within that container) and assert that that button is not disabled. Update the
assertions referencing disconnectBtns and botTokenBtn (also at the similar block
around lines 146-148) to use a scoped query or filter for the enabled Disconnect
element instead of an index.
In `@CLAUDE.md`:
- Around line 336-353: Remove the personal configuration block in CLAUDE.md (the
Chinese sections about the local wiki, memory paths, Clash proxy, and GitHub
username) — delete the lines described in the review (the four sections between
the existing English content) and replace with either nothing or a neutral,
repo-wide contribution note in English; if the author wants to keep these notes,
move them out of the repo into a local ignored file or a private fork/branch and
ensure no personal paths or usernames (e.g., the Windows username and local
memory paths) remain in the shared repository.
In `@src/openhuman/channels/controllers/ops.rs`:
- Around line 414-438: disconnect_channel is calling the synchronous rusqlite
helper delete_chunks_by_source directly, which can block the Tokio executor;
wrap the call in tokio::task::spawn_blocking and await its JoinHandle to move
the DB work off the async executor (mirror how the single-chunk delete RPC does
it). Specifically, replace the direct match on delete_chunks_by_source(...) with
a spawn_blocking(|| delete_chunks_by_source(config.clone(), SourceKind::Chat,
pattern)) (or equivalent closure capturing needed values), .await the handle,
propagate or map the JoinError and inner Result into the existing logging
branches, and preserve the same JSON Some(json!({..})) success/failure
construction and tracing::info/error behavior used currently in
disconnect_channel.
In `@src/openhuman/channels/controllers/schemas.rs`:
- Around line 39-40: The disconnect schema is missing the new clearMemory flag:
update the disconnect schema metadata used by schemas("disconnect") to include
the DisconnectParams field clear_memory (serde renamed to "clearMemory") and
mark it optional/defaulted (matching #[serde(default)]), so schema-driven
clients/docs advertise the same opt-in clear memory behavior that
handle_disconnect and DisconnectParams support. Ensure the schema entry mirrors
the existing channel and authMode entries (same type, description and default
behavior) and reference the DisconnectParams/clear_memory field name so
validation and generated docs pick it up.
In `@src/openhuman/composio/ops.rs`:
- Around line 419-424: When clear_memory is true you must always set the
response field memory_clear regardless of toolkit resolution or mapping
failures; update the delete_connection flow (where toolkit is checked and where
mapping -> None is handled) so that both the early-fail branch (when
toolkit.is_none()) and the mapping-none branch set memory_clear = false (or
appropriate status) into the returned response object before returning, and
likewise ensure the later response construction paths at the mapping and
deletion stages (the sections corresponding to the 452–475 and 515–517 logic)
always populate memory_clear with the correct boolean result to preserve the
partial-failure reporting contract.
- Around line 453-455: The current cleanup uses toolkit-wide prefixes returned
by composio_toolkit_source_kind(toolkit) and calls
delete_chunks_by_source(config, kind, &prefix), which can erase chunks from
other active connections; change the logic to build and pass a connection-scoped
source prefix (e.g., include the disconnected connection's unique id or
connector identifier) instead of the generic toolkit prefix so
delete_chunks_by_source only removes memory for that specific disconnected
connection; update the code paths around composio_toolkit_source_kind(toolkit),
the delete_chunks_by_source call, and any variables like memory_clear to derive
and use the connection-specific prefix.
In `@src/openhuman/composio/schemas.rs`:
- Around line 754-757: The current code silently defaults invalid or non-boolean
`clear_memory` values to false by using params.get("clear_memory").and_then(|v|
v.as_bool()).unwrap_or(false); change this to strictly validate the parameter:
if params contains "clear_memory" then verify the value is a boolean and return
an error (or propagate a parsing error) when it's not; only default to false
when the key is absent. Update the logic around the clear_memory extraction (the
params.get("clear_memory") handling) to match on Some(v) and error on
non-boolean types instead of using as_bool().unwrap_or(false).
In `@src/openhuman/memory/tree/store.rs`:
- Around line 1735-1765: delete_chunks_by_source currently removes only DB rows
so chunk files referenced by mem_tree_chunks.content_path remain on disk; before
executing the DELETE in delete_chunks_by_source (inside the with_connection
closure using tx), SELECT the content_path values from mem_tree_chunks WHERE
source_kind = ?1 AND source_id LIKE ?2, iterate those paths and remove each file
from the filesystem (use std::fs::remove_file), log and ignore individual remove
errors so DB deletion proceeds, then run the existing DELETE; mirror the
filesystem-cleanup behavior used in the single-chunk delete flow in read_rpc.rs
to ensure on-disk bodies are removed when chunks are bulk-deleted.
---
Outside diff comments:
In `@app/src/components/channels/DiscordConfig.tsx`:
- Around line 363-377: The managed_dm branch never shows the confirm UI because
it always renders the simple Disconnect button; change the JSX condition so that
when confirmingDisconnect (the component state) is true for spec.mode ===
'managed_dm' and status === 'connected' it renders the same confirmation UI used
for other modes (Cancel button should call setConfirmingDisconnect(false),
Confirm should invoke handleDisconnect to perform the actual disconnect),
otherwise render the simple Disconnect button; in short, update the conditional
around spec.mode === 'managed_dm' to consider confirmingDisconnect and reuse the
existing confirmation buttons/handlers (confirmingDisconnect,
setConfirmingDisconnect, handleDisconnect) so the API disconnect path can be
reached.
In `@src/openhuman/composio/schemas.rs`:
- Around line 750-760: The controller schema for delete_connection is out of
sync: handle_delete_connection now forwards an optional clear_memory boolean to
super::ops::composio_delete_connection but the schema still only lists
connection_id input and deleted output. Update the delete_connection schema in
schemas.rs to add an optional "clear_memory" input (boolean, default false) and
adjust the response schema to match the actual return shape from
composio_delete_connection (e.g., include both "deleted" and any
"memory_cleared" or other fields it returns) so schema-driven callers/docs match
handle_delete_connection and super::ops::composio_delete_connection.
---
Nitpick comments:
In `@app/src/components/channels/__tests__/DiscordConfig.test.tsx`:
- Around line 46-92: Add a new test case in DiscordConfig.test.tsx that mirrors
the existing bot_token test but uses a connected managed_dm entry: render
DiscordConfig with preloadedState containing connections.discord.managed_dm {
channel: 'discord', authMode: 'managed_dm', status: 'connected', ... }, mock
channelConnectionsApi.disconnectChannel, locate and click the enabled
"Disconnect" for the managed_dm entry, assert the disconnect confirmation dialog
appears (e.g., "Yes, disconnect") and any mode-specific text, optionally toggle
the clear-memory checkbox, then verify channelConnectionsApi.disconnectChannel
was called with ('discord', 'managed_dm', <expectedBoolean>) to ensure the
managed_dm path invokes the API.
In `@app/src/lib/composio/composioApi.ts`:
- Around line 114-123: Add a JSDoc block to the deleteConnection function
documenting the new clearMemory parameter: describe that clearMemory is an
optional boolean that defaults to false and controls whether associated
memory/state should be cleared when deleting the connection; include the
parameter tag (`@param` clearMemory) and a short return description for
Promise<ComposioDeleteResponse>, and place it immediately above the export async
function deleteConnection declaration so it mirrors the documentation style used
in channelConnectionsApi.disconnectChannel.
🪄 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: 2d175f75-a16d-4b49-b69b-de3d4308fc3d
📒 Files selected for processing (33)
CLAUDE.mdapp/src/components/channels/DiscordConfig.tsxapp/src/components/channels/TelegramConfig.tsxapp/src/components/channels/__tests__/DiscordConfig.test.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/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/services/api/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/composio/ops.rssrc/openhuman/composio/ops_test.rssrc/openhuman/composio/schemas.rssrc/openhuman/memory/tree/store.rssrc/openhuman/memory/tree/store_tests.rs
Comment on lines
+127
to
+131
| // "Disconnect" button should be enabled when connected. | ||
| const disconnectBtns = screen.getAllByText('Disconnect'); | ||
| // bot_token is the second Disconnect button (managed_dm is first, disconnected by default). | ||
| const botTokenBtn = disconnectBtns[1]; | ||
| expect(botTokenBtn).not.toBeDisabled(); |
Contributor
There was a problem hiding this comment.
Avoid index-based button targeting in the disconnect test
Using disconnectBtns[1] couples the test to auth mode render order and can produce fragile failures. Prefer selecting the enabled Disconnect button (or scope query to the bot_token section) to keep behavior assertions stable.
Suggested test tweak
- const disconnectBtns = screen.getAllByText('Disconnect');
- // bot_token is the second Disconnect button (managed_dm is first, disconnected by default).
- const botTokenBtn = disconnectBtns[1];
+ const disconnectBtns = screen.getAllByText('Disconnect');
+ const botTokenBtn = disconnectBtns.find(
+ btn => !(btn as HTMLButtonElement).disabled
+ ) as HTMLButtonElement | undefined;
+ if (!botTokenBtn) throw new Error('No enabled Disconnect button found');
expect(botTokenBtn).not.toBeDisabled();
@@
- fireEvent.click(screen.getAllByText('Disconnect')[1]);
+ fireEvent.click(screen.getAllByText('Disconnect').find(
+ btn => !(btn as HTMLButtonElement).disabled
+ ) as HTMLButtonElement);Also applies to: 146-148
🤖 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/channels/__tests__/TelegramConfig.test.tsx` around lines
127 - 131, The test currently grabs all "Disconnect" buttons via
screen.getAllByText and uses index-based selection (disconnectBtns[1] ->
botTokenBtn), which is fragile; instead locate the bot_token-specific section or
find the enabled Disconnect button directly (e.g., scope queries around the
element that contains the bot_token label or use screen.getByRole/getByText
within that container) and assert that that button is not disabled. Update the
assertions referencing disconnectBtns and botTokenBtn (also at the similar block
around lines 146-148) to use a scoped query or filter for the enabled Disconnect
element instead of an index.
Comment on lines
+336
to
+353
| ## 内存系统 | ||
|
|
||
| 个人 wiki 位于 E:\Projects\Vault\Test，使用 LLM Wiki 方法论管理。重要经验持久化到 C:\Users\12267\.claude\projects\e--Projects-Vault-Test\memory\。 | ||
|
|
||
| ## 网络代理 | ||
|
|
||
| Clash 代理 http://127.0.0.1:7890。网络操作（pip/curl/npm/git）报 ProxyError 时，第一步检查 Clash 进程（netstat -an | findstr 7890），不要先改 pip 参数。DeepSeek API 不走代理。 | ||
|
|
||
| ## GitHub | ||
|
|
||
| 用户名 mengqiuzhen。项目仓库见 https://github.com/mengqiuzhen。 | ||
|
|
||
| ## 开源贡献纪律 | ||
|
|
||
| - 一个 PR 一个 issue，不要混。 | ||
| - 改 scope/API 名字前，先搜项目已有 issue 和 PR 确认方向。 | ||
| - 验证以平台开发者后台实际能授予的为准，不以 metadata 接口为准。 | ||
| - 提交前搜已有 PR，避免重复劳动。 |
Contributor
There was a problem hiding this comment.
Remove personal configuration from shared repository documentation.
These four sections document personal development environment setup (local wiki paths, proxy settings, GitHub username, contribution reminders) in Chinese, which is inconsistent with the rest of this English-language shared repository file.
Key concerns:
- Personal paths:
E:\Projects\Vault\TestandC:\Users\12267\.claude\projects\...are environment-specific to one developer and will confuse other contributors. - Language inconsistency: The entire file is in English except these sections.
- Unrelated to PR scope: PR
#2546implements memory deletion for channel/Composio disconnect (SQLitemem_tree_chunkscleanup), but these sections document a personal wiki system and local proxy config—unrelated to the codebase's memory feature. - Privacy exposure: Windows username
12267and local configuration details are now in the shared repository.
Personal development notes belong in a local .md file outside version control, a personal fork's branch-specific docs, or a per-developer config ignored by .gitignore.
🗑️ Recommended action
Remove lines 336–353 entirely from this commit. If these notes are useful for your workflow, store them in a local file (e.g., ~/openhuman-personal-notes.md) or in your fork under a personal branch that never merges upstream.
---
-## 内存系统
-
-个人 wiki 位于 E:\Projects\Vault\Test,使用 LLM Wiki 方法论管理。重要经验持久化到 C:\Users\12267\.claude\projects\e--Projects-Vault-Test\memory\。
-
-## 网络代理
-
-Clash 代理 http://127.0.0.1:7890。网络操作(pip/curl/npm/git)报 ProxyError 时,第一步检查 Clash 进程(netstat -an | findstr 7890),不要先改 pip 参数。DeepSeek API 不走代理。
-
-## GitHub
-
-用户名 mengqiuzhen。项目仓库见 https://github.com/mengqiuzhen。
-
-## 开源贡献纪律
-
-- 一个 PR 一个 issue,不要混。
-- 改 scope/API 名字前,先搜项目已有 issue 和 PR 确认方向。
-- 验证以平台开发者后台实际能授予的为准,不以 metadata 接口为准。
-- 提交前搜已有 PR,避免重复劳动。
-🤖 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 `@CLAUDE.md` around lines 336 - 353, Remove the personal configuration block in
CLAUDE.md (the Chinese sections about the local wiki, memory paths, Clash proxy,
and GitHub username) — delete the lines described in the review (the four
sections between the existing English content) and replace with either nothing
or a neutral, repo-wide contribution note in English; if the author wants to
keep these notes, move them out of the repo into a local ignored file or a
private fork/branch and ensure no personal paths or usernames (e.g., the Windows
username and local memory paths) remain in the shared repository.
Comment on lines
+414
to
+438
| let memory_cleared: Option<serde_json::Value> = if clear_memory { | ||
| let pattern = format!("{channel_id}:%"); | ||
| match delete_chunks_by_source(config, SourceKind::Chat, &pattern) { | ||
| Ok(n) => { | ||
| tracing::info!( | ||
| target: "openhuman::channels", | ||
| channel_id = %channel_id, | ||
| deleted_chunks = n, | ||
| "[channels] disconnect_channel: cleared {n} chunks for source {channel_id}" | ||
| ); | ||
| Some(json!({"ok": true, "deleted_chunks": n})) | ||
| } | ||
| Err(e) => { | ||
| tracing::error!( | ||
| target: "openhuman::channels", | ||
| channel_id = %channel_id, | ||
| error = %e, | ||
| "[channels] disconnect_channel: memory clear failed (non-fatal)" | ||
| ); | ||
| Some(json!({"ok": false, "error": e.to_string()})) | ||
| } | ||
| } | ||
| } else { | ||
| None | ||
| }; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate disconnect_channel and delete_chunks_by_source definitions/usages
rg -n "disconnect_channel" -S src/openhuman/channels/controllers/ops.rs src/openhuman || true
rg -n "fn delete_chunks_by_source|delete_chunks_by_source\(" -S src/openhuman || true
rg -n "spawn_blocking" -S src/openhuman/memory/tree/read_rpc.rs src/openhuman || true
# Print the relevant sections around the line range mentioned in the review
sed -n '360,480p' src/openhuman/channels/controllers/ops.rs | cat -n
# Print delete_chunks_by_source definition area
# (Use fd to find likely file if not found by rg)
if rg -n "delete_chunks_by_source" -S src/openhuman >/tmp/rg_delete.txt; then
echo "---- delete_chunks_by_source matches ----"
head -n 50 /tmp/rg_delete.txt
fi
# Show full function definitions for delete_chunks_by_source if small enough
# We attempt to extract from the first match location.
first_match_file=$(rg -n "fn delete_chunks_by_source" -S src/openhuman -l | head -n 1 || true)
if [[ -n "${first_match_file:-}" ]]; then
echo "---- delete_chunks_by_source file: $first_match_file ----"
# Show around the definition
def_line=$(rg -n "fn delete_chunks_by_source" -S "$first_match_file" | head -n 1 | cut -d: -f1)
start=$((def_line-30))
end=$((def_line+120))
sed -n "${start},${end}p" "$first_match_file" | cat -n
fiRepository: tinyhumansai/openhuman
Length of output: 27900
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect delete_chunks_by_source implementation
sed -n '1710,1825p' src/openhuman/memory/tree/store.rs | cat -n
# Inspect the single-chunk delete RPC around the previously mentioned spawn_blocking range
sed -n '760,880p' src/openhuman/memory/tree/read_rpc.rs | cat -n
# Also inspect where delete_chunks_by_source is called from channels controller (disconnect_channel)
sed -n '330,460p' src/openhuman/channels/controllers/ops.rs | cat -nRepository: tinyhumansai/openhuman
Length of output: 16823
Run the bulk purge off the async executor.
delete_chunks_by_source is synchronous rusqlite work (transaction + multi-table DELETE) and is called directly from the async fn disconnect_channel RPC, so it can block a Tokio worker for the duration of the purge. The existing single-chunk delete RPC already wraps similar DB deletes in tokio::task::spawn_blocking; apply the same approach around delete_chunks_by_source in disconnect_channel.
🤖 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/channels/controllers/ops.rs` around lines 414 - 438,
disconnect_channel is calling the synchronous rusqlite helper
delete_chunks_by_source directly, which can block the Tokio executor; wrap the
call in tokio::task::spawn_blocking and await its JoinHandle to move the DB work
off the async executor (mirror how the single-chunk delete RPC does it).
Specifically, replace the direct match on delete_chunks_by_source(...) with a
spawn_blocking(|| delete_chunks_by_source(config.clone(), SourceKind::Chat,
pattern)) (or equivalent closure capturing needed values), .await the handle,
propagate or map the JoinError and inner Result into the existing logging
branches, and preserve the same JSON Some(json!({..})) success/failure
construction and tracing::info/error behavior used currently in
disconnect_channel.
Comment on lines
+39
to
+40
| #[serde(default)] | ||
| clear_memory: bool, |
Contributor
There was a problem hiding this comment.
Expose clearMemory in the disconnect controller schema too.
DisconnectParams accepts the flag and handle_disconnect forwards it, but schemas("disconnect") still advertises only channel and authMode. Schema-driven clients and docs will miss the new opt-in behavior unless the schema metadata is updated alongside the handler.
Also applies to: 485-485
🤖 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/channels/controllers/schemas.rs` around lines 39 - 40, The
disconnect schema is missing the new clearMemory flag: update the disconnect
schema metadata used by schemas("disconnect") to include the DisconnectParams
field clear_memory (serde renamed to "clearMemory") and mark it
optional/defaulted (matching #[serde(default)]), so schema-driven clients/docs
advertise the same opt-in clear memory behavior that handle_disconnect and
DisconnectParams support. Ensure the schema entry mirrors the existing channel
and authMode entries (same type, description and default behavior) and reference
the DisconnectParams/clear_memory field name so validation and generated docs
pick it up.
Comment on lines
+419
to
+424
| if toolkit.is_none() && clear_memory { | ||
| tracing::warn!( | ||
| connection_id = %connection_id, | ||
| "[composio] delete_connection: cannot clear memory — failed to resolve toolkit" | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
Always populate memory_clear when clear_memory=true, including skipped/failure paths.
If toolkit resolution fails (Line 419–424) or mapping returns None (Line 453), the response currently omits memory_clear. That breaks the partial-failure reporting contract and leaves callers unable to distinguish “not attempted” vs “succeeded”.
💡 Suggested patch
- if toolkit.is_none() && clear_memory {
+ if toolkit.is_none() && clear_memory {
tracing::warn!(
connection_id = %connection_id,
"[composio] delete_connection: cannot clear memory — failed to resolve toolkit"
);
+ memory_clear = Some(json!({
+ "ok": false,
+ "error": "toolkit_resolution_failed"
+ }));
}
@@
- if clear_memory {
+ if clear_memory {
if let Some((kind, prefix)) = composio_toolkit_source_kind(toolkit) {
memory_clear = match delete_chunks_by_source(config, kind, &prefix) {
@@
};
+ } else if memory_clear.is_none() {
+ memory_clear = Some(json!({
+ "ok": false,
+ "error": "unsupported_toolkit_for_memory_clear"
+ }));
}
}Also applies to: 452-475, 515-517
🤖 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.rs` around lines 419 - 424, When clear_memory is
true you must always set the response field memory_clear regardless of toolkit
resolution or mapping failures; update the delete_connection flow (where toolkit
is checked and where mapping -> None is handled) so that both the early-fail
branch (when toolkit.is_none()) and the mapping-none branch set memory_clear =
false (or appropriate status) into the returned response object before
returning, and likewise ensure the later response construction paths at the
mapping and deletion stages (the sections corresponding to the 452–475 and
515–517 logic) always populate memory_clear with the correct boolean result to
preserve the partial-failure reporting contract.
Comment on lines
+453
to
+455
| if let Some((kind, prefix)) = composio_toolkit_source_kind(toolkit) { | ||
| memory_clear = match delete_chunks_by_source(config, kind, &prefix) { | ||
| Ok(n) => { |
Contributor
There was a problem hiding this comment.
Scope memory deletion to the disconnected connection, not the whole toolkit.
At Line 453–455, cleanup uses toolkit-wide prefixes (gmail:%, notion:%, googledrive:%). That can delete chunks belonging to other active connections of the same toolkit in the same workspace, which is irreversible data loss outside user intent.
🤖 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.rs` around lines 453 - 455, The current cleanup
uses toolkit-wide prefixes returned by composio_toolkit_source_kind(toolkit) and
calls delete_chunks_by_source(config, kind, &prefix), which can erase chunks
from other active connections; change the logic to build and pass a
connection-scoped source prefix (e.g., include the disconnected connection's
unique id or connector identifier) instead of the generic toolkit prefix so
delete_chunks_by_source only removes memory for that specific disconnected
connection; update the code paths around composio_toolkit_source_kind(toolkit),
the delete_chunks_by_source call, and any variables like memory_clear to derive
and use the connection-specific prefix.
Comment on lines
+754
to
+757
| let clear_memory = params | ||
| .get("clear_memory") | ||
| .and_then(|v| v.as_bool()) | ||
| .unwrap_or(false); |
Contributor
There was a problem hiding this comment.
Reject invalid clear_memory types instead of silently defaulting to false.
Line 754–757 uses as_bool().unwrap_or(false), so malformed payloads (e.g. "true") are quietly accepted as false. Prefer strict parsing so client bugs are surfaced.
💡 Suggested patch
- let clear_memory = params
- .get("clear_memory")
- .and_then(|v| v.as_bool())
- .unwrap_or(false);
+ let clear_memory = read_optional::<bool>(¶ms, "clear_memory")?.unwrap_or(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 `@src/openhuman/composio/schemas.rs` around lines 754 - 757, The current code
silently defaults invalid or non-boolean `clear_memory` values to false by using
params.get("clear_memory").and_then(|v| v.as_bool()).unwrap_or(false); change
this to strictly validate the parameter: if params contains "clear_memory" then
verify the value is a boolean and return an error (or propagate a parsing error)
when it's not; only default to false when the key is absent. Update the logic
around the clear_memory extraction (the params.get("clear_memory") handling) to
match on Some(v) and error on non-boolean types instead of using
as_bool().unwrap_or(false).
Comment on lines
+1735
to
+1765
| pub fn delete_chunks_by_source( | ||
| config: &Config, | ||
| source_kind: SourceKind, | ||
| source_id_pattern: &str, | ||
| ) -> Result<usize> { | ||
| with_connection(config, |conn| { | ||
| let tx = conn.unchecked_transaction()?; | ||
| // Clean up rows that don't cascade from mem_tree_chunks. | ||
| for (table, col) in &[ | ||
| ("mem_tree_score", "chunk_id"), | ||
| ("mem_tree_entity_index", "node_id"), | ||
| ] { | ||
| let sql = format!( | ||
| "DELETE FROM {table} WHERE {col} IN \ | ||
| (SELECT id FROM mem_tree_chunks \ | ||
| WHERE source_kind = ?1 AND source_id LIKE ?2)" | ||
| ); | ||
| tx.execute(&sql, params![source_kind.as_str(), source_id_pattern])?; | ||
| } | ||
| let deleted = tx.execute( | ||
| "DELETE FROM mem_tree_chunks WHERE source_kind = ?1 AND source_id LIKE ?2", | ||
| params![source_kind.as_str(), source_id_pattern], | ||
| )?; | ||
| tx.commit()?; | ||
| log::info!( | ||
| "[memory_tree::store] delete_chunks_by_source kind={} pattern_len={} deleted={deleted}", | ||
| source_kind.as_str(), | ||
| source_id_pattern.len(), | ||
| ); | ||
| Ok(deleted) | ||
| }) |
Contributor
There was a problem hiding this comment.
Bulk source deletion leaves chunk bodies on disk.
This only removes SQLite rows. Chunks with a content_path will keep their .md body after clear_memory, so the disconnect flow can report success while the source content still exists on disk. The existing single-chunk delete path in src/openhuman/memory/tree/read_rpc.rs:798-871 already treats that filesystem cleanup as part of the delete contract.
🤖 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 1735 - 1765,
delete_chunks_by_source currently removes only DB rows so chunk files referenced
by mem_tree_chunks.content_path remain on disk; before executing the DELETE in
delete_chunks_by_source (inside the with_connection closure using tx), SELECT
the content_path values from mem_tree_chunks WHERE source_kind = ?1 AND
source_id LIKE ?2, iterate those paths and remove each file from the filesystem
(use std::fs::remove_file), log and ignore individual remove errors so DB
deletion proceeds, then run the existing DELETE; mirror the filesystem-cleanup
behavior used in the single-chunk delete flow in read_rpc.rs to ensure on-disk
bodies are removed when chunks are bulk-deleted.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
src/openhuman/channels/controllers/schemas.rs (1)
39-40:⚠️ Potential issue | 🟠 Major | ⚡ Quick winExpose
clearMemoryin the disconnect schema as well.
DisconnectParamsandhandle_disconnectaccept the flag, butschemas("disconnect")still advertises onlychannelandauthMode. Schema-driven clients and docs will miss the new opt-in clear-memory behavior until the input metadata is updated.Also applies to: 485-485
🤖 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/channels/controllers/schemas.rs` around lines 39 - 40, The disconnect JSON schema is missing the clearMemory flag even though DisconnectParams and handle_disconnect accept it; update the schema emitted by schemas("disconnect") to include the clear_memory field (exposed as clearMemory) with its serde/default semantics and a brief description and default false so schema-driven clients/docs see the opt-in clear-memory behavior; locate the match arm or builder for "disconnect" in schemas.rs and add the clear_memory property (serialized name clearMemory) consistent with the other properties (channel, authMode) so the generated schema reflects DisconnectParams and handle_disconnect.src/openhuman/channels/controllers/ops.rs (1)
414-438:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRun the purge in
spawn_blocking.
delete_chunks_by_sourceis synchronousrusqlitework inside thisasync fn. A largeclear_memorypurge can block the Tokio worker and stall unrelated RPCs; move just this delete path totokio::task::spawn_blockingand await the result before buildingmemory_clear.🤖 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/channels/controllers/ops.rs` around lines 414 - 438, The current synchronous call to delete_chunks_by_source inside the async disconnect flow can block the Tokio runtime when clear_memory is true; wrap the delete_chunks_by_source call in tokio::task::spawn_blocking and await its JoinHandle to run the rusqlite work off the async runtime, then map the Result to the same tracing and JSON creation logic used for memory_cleared. Specifically, when clear_memory is true, spawn_blocking should run the closure that calls delete_chunks_by_source(config, SourceKind::Chat, &pattern), return the Result<n, e>, and after awaiting the handle perform the existing tracing::info / tracing::error branches and build the Some(json!(...)) value for memory_cleared; leave the pattern, channel_id, and JSON shape unchanged.src/openhuman/memory/tree/store.rs (1)
1726-1765:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBulk clear still leaves chunk bodies on disk.
This helper only deletes SQLite rows. Any chunks with a
content_pathkeep their.mdbody, soclear_memorycan report success while recoverable source content is still present. Please reuse the same filesystem cleanup path as the single-chunk delete flow before removing the rows.Based on learnings: in the memory-tree pipeline, full chunk bodies live at
content_path, whilemem_tree_chunks.contentis only a preview.🤖 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 1726 - 1765, delete_chunks_by_source currently only removes DB rows and leaves chunk bodies referenced by mem_tree_chunks.content_path on disk; before deleting rows, SELECT content_path FROM mem_tree_chunks WHERE source_kind = ?1 AND source_id LIKE ?2, iterate over the result set and for each non-empty path attempt to remove the file (std::fs::remove_file) and log any removal errors (log::warn) but do not abort the whole operation for individual unlink failures; after cleaning files, continue with the existing cleanup and DELETE statements (the same tx logic that deletes mem_tree_score, mem_tree_entity_index, then mem_tree_chunks) so the filesystem cleanup mirrors the single-chunk delete flow that removes content_path.
🧹 Nitpick comments (3)
src/openhuman/channels/controllers/ops_tests.rs (1)
525-527: ⚡ Quick winAssert the new
memory_clearpayload too.Both tests only check the DB side effect. They won't catch regressions where
disconnect_channelstops returningmemory_clear, flipsok, or omitsdeleted_chunks. Since this PR changes the RPC contract, please assert the response body in both paths as well.Based on learnings: All PRs must meet ≥ 80% coverage on changed lines. Add tests for new/changed lines, not just happy paths.
Also applies to: 573-575
🤖 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/channels/controllers/ops_tests.rs` around lines 525 - 527, Update the tests that call disconnect_channel in ops_tests.rs (the calls using disconnect_channel(...) with ChannelAuthMode::BotToken at the two locations) to assert the returned RPC response body includes the new memory_clear payload fields; specifically, after awaiting disconnect_channel(...).await, inspect the returned value and assert memory_clear.ok is true (or false as appropriate for the path) and that memory_clear.deleted_chunks is present and matches the expected count (or is zero) so both the DB side-effect and the RPC contract are verified for both clear_memory=true and the other path.app/src/lib/composio/composioApi.ts (1)
110-117: ⚡ Quick winDocument
clearMemoryindeleteConnectionJSDoc.The function behavior changed, but the doc comment still describes only the old semantics. Please add one line documenting the optional memory deletion flag.
📝 Suggested doc update
/** * Delete an existing Composio connection. Backend verifies ownership * before forwarding to Composio. + * Set `clearMemory` to also delete ingested memory chunks for this connection. */Based on learnings: All code changes must include matching rustdoc comments (Rust) or JSDoc comments (TypeScript) documenting the change.
🤖 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/lib/composio/composioApi.ts` around lines 110 - 117, Update the JSDoc for the exported async function deleteConnection(connectionId: string, clearMemory?: boolean): Promise<ComposioDeleteResponse> to document the new optional clearMemory flag — add a brief line describing that when clearMemory is true the backend will also delete associated memory/state for the connection (default: false/optional) so the comment matches the implementation.app/src/lib/composio/composioApi.test.ts (1)
185-201: ⚡ Quick winAdd a return-value assertion for
deleteConnectionbehavior.These tests currently verify only RPC payload shape. Add at least one assertion on the returned value to lock the wrapper’s observable behavior (envelope unwrap path), not just implementation details.
✅ Suggested test tweak
it('calls composio_delete_connection with connection_id', async () => { mockCallCoreRpc.mockResolvedValue({ result: { deleted: true }, logs: [] }); - await deleteConnection('conn-abc'); + const out = await deleteConnection('conn-abc'); expect(mockCallCoreRpc).toHaveBeenCalledWith({ method: 'openhuman.composio_delete_connection', params: { connection_id: 'conn-abc', clear_memory: false }, }); + expect(out).toMatchObject({ deleted: true }); });As per coding guidelines: Prefer testing behavior over 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 `@app/src/lib/composio/composioApi.test.ts` around lines 185 - 201, The tests only assert the RPC call shape but not the wrapper's returned value; update the two tests that call deleteConnection to capture and assert its resolved return (e.g., const res = await deleteConnection('conn-abc') and expect(res).toEqual({ deleted: true })) so the envelope-unwrapping behavior is locked, and do the same for the clearMemory=true case; reference the deleteConnection function and mockCallCoreRpc mock when adding these assertions.
🤖 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/components/channels/DiscordConfig.tsx`:
- Around line 388-430: The managed_dm disconnect path never reaches the confirm
action because the confirmation UI is only shown in the alternate branch; update
the managed_dm UI to use the same confirmation flow by wiring its state and
handlers: ensure the managed_dm button sets confirmingDisconnect to spec.mode
(same state used by the conditional), and when confirmingDisconnect ===
spec.mode render the existing confirmation block that calls
handleConfirmDisconnect(spec.mode) (or change the managed_dm button's onClick to
call handleConfirmDisconnect directly if immediate confirmation is desired).
Touch the components referencing confirmingDisconnect, spec.mode,
handleConfirmDisconnect and handleDisconnect so both UI paths use the same
confirm UI/handler.
In `@app/src/services/api/channelConnectionsApi.ts`:
- Around line 150-158: The disconnectChannel wrapper currently discards the
callCoreRpc result; change the signature of disconnectChannel (and its return
type) to return the RPC payload from callCoreRpc so callers can inspect fields
like memory_clear; specifically, update the function signature
(disconnectChannel(channel: ChannelType, authMode: ChannelAuthMode,
clearMemory?: boolean): Promise<any|DisconnectResponse>) and return the result
of callCoreRpc({ method: 'openhuman.channels_disconnect', params: { channel,
authMode, clearMemory: clearMemory ?? false } }) instead of awaiting and
returning void, ensuring callers can observe memory_clear and any non-fatal
cleanup failures.
In `@src/openhuman/composio/ops.rs`:
- Around line 515-517: The code attempts to assign resp["memory_clear"] on a
typed ComposioDeleteResponse, causing E0608 and also the struct lacks that
field; add an optional field pub memory_clear: Option<YourType> (use the same
type as memory_clear and annotate with serde as needed) to the
ComposioDeleteResponse definition in types.rs, derive/allow serde as the other
fields do, then in ops.rs replace the JSON-indexing with direct assignment
(e.g., let mut resp = ...; resp.memory_clear = memory_clear;) so the typed
struct is used correctly.
In `@src/openhuman/composio/schemas.rs`:
- Around line 758-760: The schema metadata for delete_connection is out of sync
with the runtime: the call to super::ops::composio_delete_connection(&config,
&connection_id, clear_memory) forwards a clear_memory flag but
schemas("delete_connection") only declares connection_id input and deleted
output; update the schemas("delete_connection") definition to include the
clear_memory (or memory_clear) boolean input parameter and reflect any
additional outputs (e.g., memory_cleared) returned by composio_delete_connection
so the declared API matches the runtime contract.
---
Duplicate comments:
In `@src/openhuman/channels/controllers/ops.rs`:
- Around line 414-438: The current synchronous call to delete_chunks_by_source
inside the async disconnect flow can block the Tokio runtime when clear_memory
is true; wrap the delete_chunks_by_source call in tokio::task::spawn_blocking
and await its JoinHandle to run the rusqlite work off the async runtime, then
map the Result to the same tracing and JSON creation logic used for
memory_cleared. Specifically, when clear_memory is true, spawn_blocking should
run the closure that calls delete_chunks_by_source(config, SourceKind::Chat,
&pattern), return the Result<n, e>, and after awaiting the handle perform the
existing tracing::info / tracing::error branches and build the Some(json!(...))
value for memory_cleared; leave the pattern, channel_id, and JSON shape
unchanged.
In `@src/openhuman/channels/controllers/schemas.rs`:
- Around line 39-40: The disconnect JSON schema is missing the clearMemory flag
even though DisconnectParams and handle_disconnect accept it; update the schema
emitted by schemas("disconnect") to include the clear_memory field (exposed as
clearMemory) with its serde/default semantics and a brief description and
default false so schema-driven clients/docs see the opt-in clear-memory
behavior; locate the match arm or builder for "disconnect" in schemas.rs and add
the clear_memory property (serialized name clearMemory) consistent with the
other properties (channel, authMode) so the generated schema reflects
DisconnectParams and handle_disconnect.
In `@src/openhuman/memory/tree/store.rs`:
- Around line 1726-1765: delete_chunks_by_source currently only removes DB rows
and leaves chunk bodies referenced by mem_tree_chunks.content_path on disk;
before deleting rows, SELECT content_path FROM mem_tree_chunks WHERE source_kind
= ?1 AND source_id LIKE ?2, iterate over the result set and for each non-empty
path attempt to remove the file (std::fs::remove_file) and log any removal
errors (log::warn) but do not abort the whole operation for individual unlink
failures; after cleaning files, continue with the existing cleanup and DELETE
statements (the same tx logic that deletes mem_tree_score,
mem_tree_entity_index, then mem_tree_chunks) so the filesystem cleanup mirrors
the single-chunk delete flow that removes content_path.
---
Nitpick comments:
In `@app/src/lib/composio/composioApi.test.ts`:
- Around line 185-201: The tests only assert the RPC call shape but not the
wrapper's returned value; update the two tests that call deleteConnection to
capture and assert its resolved return (e.g., const res = await
deleteConnection('conn-abc') and expect(res).toEqual({ deleted: true })) so the
envelope-unwrapping behavior is locked, and do the same for the clearMemory=true
case; reference the deleteConnection function and mockCallCoreRpc mock when
adding these assertions.
In `@app/src/lib/composio/composioApi.ts`:
- Around line 110-117: Update the JSDoc for the exported async function
deleteConnection(connectionId: string, clearMemory?: boolean):
Promise<ComposioDeleteResponse> to document the new optional clearMemory flag —
add a brief line describing that when clearMemory is true the backend will also
delete associated memory/state for the connection (default: false/optional) so
the comment matches the implementation.
In `@src/openhuman/channels/controllers/ops_tests.rs`:
- Around line 525-527: Update the tests that call disconnect_channel in
ops_tests.rs (the calls using disconnect_channel(...) with
ChannelAuthMode::BotToken at the two locations) to assert the returned RPC
response body includes the new memory_clear payload fields; specifically, after
awaiting disconnect_channel(...).await, inspect the returned value and assert
memory_clear.ok is true (or false as appropriate for the path) and that
memory_clear.deleted_chunks is present and matches the expected count (or is
zero) so both the DB side-effect and the RPC contract are verified for both
clear_memory=true and the other path.
🪄 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: 063f40fa-d555-4a34-a449-242bd8547290
📒 Files selected for processing (33)
CLAUDE.mdapp/src/components/channels/DiscordConfig.tsxapp/src/components/channels/TelegramConfig.tsxapp/src/components/channels/__tests__/DiscordConfig.test.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/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/services/api/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/composio/ops.rssrc/openhuman/composio/ops_test.rssrc/openhuman/composio/schemas.rssrc/openhuman/memory/tree/store.rssrc/openhuman/memory/tree/store_tests.rs
✅ Files skipped from review due to trivial changes (9)
- app/src/lib/i18n/chunks/zh-CN-1.ts
- app/src/services/api/channelConnectionsApi.test.ts
- app/src/lib/i18n/chunks/de-1.ts
- app/src/lib/i18n/chunks/fr-1.ts
- CLAUDE.md
- app/src/lib/i18n/en.ts
- app/src/lib/i18n/chunks/it-1.ts
- app/src/lib/i18n/chunks/id-1.ts
- app/src/lib/i18n/chunks/ar-1.ts
Comment on lines
+388
to
+430
| {confirmingDisconnect === spec.mode ? ( | ||
| <div className="flex flex-col gap-2"> | ||
| <span className="text-xs text-coral-600 dark:text-coral-400 font-medium"> | ||
| {t('accounts.disconnectRevokeConfirm').replace( | ||
| '{name}', | ||
| definition.display_name | ||
| )} | ||
| </span> | ||
| <label className="flex items-center gap-2 text-xs text-stone-500 dark:text-neutral-400 cursor-pointer"> | ||
| <input | ||
| type="checkbox" | ||
| checked={clearMemory} | ||
| onChange={e => setClearMemory(e.target.checked)} | ||
| className="rounded border-stone-300 dark:border-neutral-600" | ||
| /> | ||
| {t('accounts.disconnectClearMemory')} | ||
| </label> | ||
| <div className="flex items-center gap-1.5"> | ||
| <button | ||
| type="button" | ||
| disabled={busy} | ||
| onClick={() => handleConfirmDisconnect(spec.mode)} | ||
| className="rounded-lg bg-coral-500 px-3 py-1.5 text-xs font-medium text-white hover:bg-coral-600 disabled:opacity-50"> | ||
| {t('accounts.disconnectYes')} | ||
| </button> | ||
| <button | ||
| type="button" | ||
| disabled={busy} | ||
| onClick={() => setConfirmingDisconnect(null)} | ||
| className="rounded-lg border border-stone-200 dark:border-neutral-700 px-3 py-1.5 text-xs font-medium text-stone-600 dark:text-neutral-300 hover:border-stone-300 disabled:opacity-50"> | ||
| {t('common.cancel')} | ||
| </button> | ||
| </div> | ||
| </div> | ||
| ) : ( | ||
| <button | ||
| type="button" | ||
| disabled={busy || status === 'disconnected'} | ||
| onClick={() => handleDisconnect(spec.mode)} | ||
| className="rounded-lg border border-stone-200 dark:border-neutral-800 px-3 py-1.5 text-xs font-medium text-stone-600 dark:text-neutral-300 hover:border-stone-300 dark:hover:border-neutral-700 disabled:opacity-50"> | ||
| {t('accounts.disconnect')} | ||
| </button> | ||
| )} |
Contributor
There was a problem hiding this comment.
managed_dm connected disconnect flow is now non-functional.
The new confirmation UI is only rendered in this branch, but managed_dm connected uses a different UI path. Clicking Disconnect there only sets confirmingDisconnect and never reaches the actual confirm/disconnect action.
🤖 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/channels/DiscordConfig.tsx` around lines 388 - 430, The
managed_dm disconnect path never reaches the confirm action because the
confirmation UI is only shown in the alternate branch; update the managed_dm UI
to use the same confirmation flow by wiring its state and handlers: ensure the
managed_dm button sets confirmingDisconnect to spec.mode (same state used by the
conditional), and when confirmingDisconnect === spec.mode render the existing
confirmation block that calls handleConfirmDisconnect(spec.mode) (or change the
managed_dm button's onClick to call handleConfirmDisconnect directly if
immediate confirmation is desired). Touch the components referencing
confirmingDisconnect, spec.mode, handleConfirmDisconnect and handleDisconnect so
both UI paths use the same confirm UI/handler.
Comment on lines
+150
to
+158
| disconnectChannel: async ( | ||
| channel: ChannelType, | ||
| authMode: ChannelAuthMode, | ||
| clearMemory?: boolean | ||
| ): Promise<void> => { | ||
| await callCoreRpc({ | ||
| method: 'openhuman.channels_disconnect', | ||
| params: { channel, authMode, clearMemory: clearMemory ?? false }, | ||
| }); |
Contributor
There was a problem hiding this comment.
Return the disconnect RPC payload instead of dropping it.
This wrapper returns Promise<void> and discards the response, so callers cannot inspect memory_clear when cleanup fails non-fatally. That makes the clear-memory path effectively silent from this API surface.
💡 Suggested change
+export interface DisconnectChannelResult {
+ channel: string;
+ auth_mode: string;
+ disconnected: boolean;
+ restart_required: boolean;
+ memory_clear?: {
+ ok: boolean;
+ deleted_chunks?: number;
+ error?: string;
+ };
+}
+
disconnectChannel: async (
channel: ChannelType,
authMode: ChannelAuthMode,
clearMemory?: boolean
- ): Promise<void> => {
- await callCoreRpc({
+ ): Promise<DisconnectChannelResult> => {
+ const result = await callCoreRpc<unknown>({
method: 'openhuman.channels_disconnect',
params: { channel, authMode, clearMemory: clearMemory ?? false },
});
+ return expectObject<DisconnectChannelResult>(result, 'Channel disconnect');
},🤖 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/services/api/channelConnectionsApi.ts` around lines 150 - 158, The
disconnectChannel wrapper currently discards the callCoreRpc result; change the
signature of disconnectChannel (and its return type) to return the RPC payload
from callCoreRpc so callers can inspect fields like memory_clear; specifically,
update the function signature (disconnectChannel(channel: ChannelType, authMode:
ChannelAuthMode, clearMemory?: boolean): Promise<any|DisconnectResponse>) and
return the result of callCoreRpc({ method: 'openhuman.channels_disconnect',
params: { channel, authMode, clearMemory: clearMemory ?? false } }) instead of
awaiting and returning void, ensuring callers can observe memory_clear and any
non-fatal cleanup failures.
Comment on lines
+515
to
+517
| if let Some(ref mc) = memory_clear { | ||
| resp["memory_clear"] = mc.clone(); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify response type shape and whether a typed `memory_clear` field exists.
fd -p 'types.rs' src/openhuman/composio | xargs -I{} rg -n "struct ComposioDeleteResponse|memory_clear|deleted" {}
# Verify write sites and the invalid index mutation site.
rg -n "composio_delete_connection\\(|\\[\"memory_clear\"\\]|memory_clear" \
src/openhuman/composio/ops.rs src/openhuman/composio/schemas.rs src/openhuman/composio/ops_test.rsRepository: tinyhumansai/openhuman
Length of output: 263
Fix memory_clear compile blocker by using a real ComposioDeleteResponse field (not JSON indexing).
src/openhuman/composio/ops.rs lines 515-517 indexes resp like resp["memory_clear"], but resp is a typed ComposioDeleteResponse (E0608). Also, ComposioDeleteResponse in src/openhuman/composio/types.rs only defines pub deleted: bool—there’s no memory_clear field to assign—so the correct fix is to add an optional memory_clear field to ComposioDeleteResponse (with the proper serde type) and then set it via let mut resp = ...; resp.memory_clear = memory_clear;.
🧰 Tools
🪛 GitHub Actions: Type Check / 0_Rust Quality (fmt + clippy).txt
[error] 516-516: error[E0608]: cannot index into a value of type openhuman::composio::types::ComposioDeleteResponse at resp["memory_clear"] = mc.clone();
🪛 GitHub Actions: Type Check / Rust Quality (fmt + clippy)
[error] 516-516: Clippy/cargo build failed with Rust error E0608: cannot index into a value of type openhuman::composio::types::ComposioDeleteResponse at resp["memory_clear"] = mc.clone();.
🤖 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.rs` around lines 515 - 517, The code attempts to
assign resp["memory_clear"] on a typed ComposioDeleteResponse, causing E0608 and
also the struct lacks that field; add an optional field pub memory_clear:
Option<YourType> (use the same type as memory_clear and annotate with serde as
needed) to the ComposioDeleteResponse definition in types.rs, derive/allow serde
as the other fields do, then in ops.rs replace the JSON-indexing with direct
assignment (e.g., let mut resp = ...; resp.memory_clear = memory_clear;) so the
typed struct is used correctly.
Comment on lines
+758
to
+760
| to_json( | ||
| super::ops::composio_delete_connection(&config, &connection_id, clear_memory).await?, | ||
| ) |
Contributor
There was a problem hiding this comment.
Align delete_connection schema metadata with the new runtime contract.
Line 759 forwards clear_memory, but schemas("delete_connection") still advertises only connection_id input and only deleted output. This breaks schema-driven clients/tooling and hides memory_clear from the declared API surface.
💡 Proposed schema update
"delete_connection" => ControllerSchema {
namespace: "composio",
function: "delete_connection",
description: "Delete a Composio connection owned by the caller.",
- inputs: vec![FieldSchema {
- name: "connection_id",
- ty: TypeSchema::String,
- comment: "Identifier of the connection to delete.",
- required: true,
- }],
- outputs: vec![FieldSchema {
- name: "deleted",
- ty: TypeSchema::Bool,
- comment: "True when the backend confirmed the deletion.",
- required: true,
- }],
+ inputs: vec![
+ FieldSchema {
+ name: "connection_id",
+ ty: TypeSchema::String,
+ comment: "Identifier of the connection to delete.",
+ required: true,
+ },
+ FieldSchema {
+ name: "clear_memory",
+ ty: TypeSchema::Option(Box::new(TypeSchema::Bool)),
+ comment: "When true, also delete ingested memory for this connection (default false).",
+ required: false,
+ },
+ ],
+ outputs: vec![
+ FieldSchema {
+ name: "deleted",
+ ty: TypeSchema::Bool,
+ comment: "True when the backend confirmed the deletion.",
+ required: true,
+ },
+ FieldSchema {
+ name: "memory_clear",
+ ty: TypeSchema::Option(Box::new(TypeSchema::Json)),
+ comment: "Memory cleanup result when clear_memory=true: { ok, deleted_chunks? | error? }.",
+ required: 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 `@src/openhuman/composio/schemas.rs` around lines 758 - 760, The schema
metadata for delete_connection is out of sync with the runtime: the call to
super::ops::composio_delete_connection(&config, &connection_id, clear_memory)
forwards a clear_memory flag but schemas("delete_connection") only declares
connection_id input and deleted output; update the schemas("delete_connection")
definition to include the clear_memory (or memory_clear) boolean input parameter
and reflect any additional outputs (e.g., memory_cleared) returned by
composio_delete_connection so the declared API matches the runtime contract.
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
1 participant
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
Add opt-in
clear_memoryparameter tochannels.disconnectandcomposio.delete_connectionRPCs. When set, all memory tree chunks indexed under the source are deleted in a single SQLite transaction. A confirmation checkbox in the UI (unchecked by default) makes the irreversibility explicit. The RPC response includes amemory_clearfield so callers can surface partial failures.Closes #2515.
Changes
delete_chunks_by_sourcehelper inmemory/tree/store.rs, transaction-safe with explicit cascade cleanupdisconnect_channel+composio_delete_connectionacceptclear_memory: bool(default false)Test plan
pnpm typecheck— cleanpnpm test --run— 342 files / 3332 tests passing (all new tests pass)cargo fmt --all -- --check— cleandelete_chunks_by_source,composio_toolkit_source_kind,disconnect_channelwithclear_memorydeleteConnection,disconnectChannel, disconnect confirmation UI (3 components)Summary by CodeRabbit
New Features
Internationalization
Documentation
Tests