Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
yh928 has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (26)
🚧 Files skipped from review as they are similar to previous changes (20)
Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughThe change adds a reversible cloud-summarization consent setting. The setting persists through the configuration RPC, appears in memory-tree status, controls summarization model resolution, and is available through a localized status-panel switch. ChangesCloud summarization consent
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to This change lets users enable or withdraw cloud summarization from the memory settings panel while preserving the stored consent state. No actionable merge-blocking risk remains beyond normal checks and review. Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Linked Issues checkExplanation The changes satisfy the coding objectives in [ Full details: Out of Scope Changes checkExplanation The changes remain within [ Full details: Docstring CoverageExplanation Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 36 functions across 26 files. (5 skipped: 5 too large.) Warning Your free Security trial is over. An organization admin can upgrade to Advanced for continuous pull request security review or dismiss this notice. Comment |
coderabbitai
Bot
added
feature
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 24db6a0521
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/openhuman/config/ops/model.rs (1)
309-311: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a diagnostic log line for the consent state transition.
The block sets
config.memory_tree.cloud_summarization_opt_inbut does not log the change. This flag controls whether workspace memory content leaves the device to a configured cloud provider. Add a grep-friendly log line for this transition so an operator can trace when consent changed, without logging any memory content.💡 Suggested diagnostic log
if let Some(opt_in) = update.cloud_summarization_opt_in { + tracing::info!( + opt_in, + "[config] cloud_summarization_opt_in updated" + ); config.memory_tree.cloud_summarization_opt_in = opt_in; }Do you want me to open a follow-up issue to add consistent state-transition logging across the other
MemorySettingsPatchfields as well?As per path instructions, "New or changed flows must include verbose, grep-friendly diagnostics for entry/exit, branches, external calls, retries, state transitions, and errors, while never logging secrets or full PII."
🤖 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/config/ops/model.rs` around lines 309 - 311, In the update flow containing the cloud_summarization_opt_in assignment, add a verbose, grep-friendly diagnostic log immediately before or after the state transition that records the previous and new consent states. Do not include memory content, secrets, or other PII, and keep the existing assignment behavior unchanged.Source: Path instructions
🤖 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/intelligence/MemoryTreeStatusPanel.tsx`:
- Around line 356-372: Update the cloud-summarization switch to use a stable
analytics identifier, and in handleCloudSummarizationToggle call
trackAnalyticsEvent from components/analytics only after
memoryTreeSetCloudSummarization succeeds. Keep the event dimensions free of
optIn and other user-preference data, and preserve the existing refresh and
error handling flow.
In `@app/src/lib/i18n/de.ts`:
- Around line 1342-1344: Update the German
`memoryTree.status.cloudSummarizationDescription` translation to use the formal
`Sie/Ihren` address consistently with the adjacent auto-sync text and explicitly
identify the configured cloud provider as the destination for summaries. Keep
the label unchanged.
In `@app/src/lib/i18n/ko.ts`:
- Around line 1283-1285: Add a dedicated cloud-consent toggle failure
translation alongside the existing memoryTree status keys in ko.ts and mirror
the same key across every locale. Update the cloud toggle error path in
MemoryTreeStatusPanel to use this new cloud-specific key instead of
memoryTree.status.toggleFailed, preserving the existing auto-sync failure
translation for its original handler.
In `@app/src/lib/i18n/pl.ts`:
- Around line 1314-1316: Update the cloud-toggle error handling in
MemoryTreeStatusPanel to use a dedicated cloud-toggle failure translation key
instead of memoryTree.status.toggleFailed. Add the corresponding Polish
translation near the existing cloudSummarization keys and define matching
translations for the same key in every supported locale, preserving the existing
generic toggle failure key for non-cloud errors.
---
Nitpick comments:
In `@src/openhuman/config/ops/model.rs`:
- Around line 309-311: In the update flow containing the
cloud_summarization_opt_in assignment, add a verbose, grep-friendly diagnostic
log immediately before or after the state transition that records the previous
and new consent states. Do not include memory content, secrets, or other PII,
and keep the existing assignment behavior unchanged.
🪄 Autofix
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 Plus
Run ID: 9f89c648-135f-4418-bd3a-9e84aadf296a
📒 Files selected for processing (23)
app/src/components/intelligence/MemoryTreeStatusPanel.test.tsxapp/src/components/intelligence/MemoryTreeStatusPanel.tsxapp/src/lib/i18n/ar.tsapp/src/lib/i18n/bn.tsapp/src/lib/i18n/de.tsapp/src/lib/i18n/en.tsapp/src/lib/i18n/es.tsapp/src/lib/i18n/fr.tsapp/src/lib/i18n/hi.tsapp/src/lib/i18n/id.tsapp/src/lib/i18n/it.tsapp/src/lib/i18n/ko.tsapp/src/lib/i18n/pl.tsapp/src/lib/i18n/pt.tsapp/src/lib/i18n/ru.tsapp/src/lib/i18n/zh-CN.tsapp/src/utils/tauriCommands/memoryTree.tssrc/openhuman/config/ops/model.rssrc/openhuman/config/ops_tests.rssrc/openhuman/config/schemas/controllers.rssrc/openhuman/config/schemas/helpers.rssrc/openhuman/config/schemas/schema_defs.rssrc/openhuman/memory/tree/tree/rpc.rs
24db6a0 to
849ca00
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
coderabbitai
Bot
added
the
memory
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/openhuman/memory/tinycortex/summariser.rs (1)
37-83: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the required debug or trace diagnostics.
The changed flow logs a reload error at
warnbut does not record the consent decision, unavailable branch, or provider call with a safe correlation field. Use debug or trace logs with the existing stable prefix and a non-sensitive correlation value.As per coding guidelines, use
logortracingat debug or trace levels with stable prefixes and correlation fields for changed flows.🤖 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/tinycortex/summariser.rs` around lines 37 - 83, The changed summarisation flow lacks diagnostic coverage for its key decisions. Add debug or trace logs in consent_config and call using the existing stable memory_tree::summarise prefix, recording a non-sensitive correlation field for the consent result, unavailable refusal branch, and provider invocation; keep sensitive inputs and content out of logs.Source: Coding guidelines
🤖 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/memory/tinycortex/summariser.rs`:
- Around line 191-213: Update the test setup around HostSummariser::new so its
initial Config has cloud_summarization_opt_in disabled, then persist enabled
consent to the config file before calling summarise. Keep the existing
availability assertion and provider-reachability check, ensuring the test
exercises the live reload value rather than the startup snapshot.
- Around line 34-50: Update the Err branch of consent_config so a failed reload
explicitly sets config.memory_tree.cloud_summarization_opt_in to false before
returning the cloned configuration, while preserving the existing warning and
other startup-snapshot values.
- Around line 160-164: Update refuses_to_summarise_without_cloud_consent to use
the same isolated temporary config fixture as the granted-consent test, setting
cloud_summarization_opt_in to false while retaining local_ai.runtime_enabled =
false. Avoid Config::default()’s user-scoped config_path so the test cannot read
ambient persisted consent.
- Around line 76-83: Update the summarise flow around summarizer_available and
build_chat_provider so cloud-selected providers are rejected unless the default
memory_tree.cloud_summarization_opt_in is enabled, even when local AI is
available. Fail closed before sending inputs to the cloud, while preserving
local-provider summarization and the existing unavailable error behavior.
---
Nitpick comments:
In `@src/openhuman/memory/tinycortex/summariser.rs`:
- Around line 37-83: The changed summarisation flow lacks diagnostic coverage
for its key decisions. Add debug or trace logs in consent_config and call using
the existing stable memory_tree::summarise prefix, recording a non-sensitive
correlation field for the consent result, unavailable refusal branch, and
provider invocation; keep sensitive inputs and content out of logs.
🪄 Autofix
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 Plus
Run ID: 8968b9d2-5912-4594-b609-855045c9b6c3
📒 Files selected for processing (25)
app/src/components/intelligence/MemoryTreeStatusPanel.test.tsxapp/src/components/intelligence/MemoryTreeStatusPanel.tsxapp/src/lib/i18n/ar.tsapp/src/lib/i18n/bn.tsapp/src/lib/i18n/de.tsapp/src/lib/i18n/en.tsapp/src/lib/i18n/es.tsapp/src/lib/i18n/fr.tsapp/src/lib/i18n/hi.tsapp/src/lib/i18n/id.tsapp/src/lib/i18n/it.tsapp/src/lib/i18n/ko.tsapp/src/lib/i18n/pl.tsapp/src/lib/i18n/pt.tsapp/src/lib/i18n/ru.tsapp/src/lib/i18n/zh-CN.tsapp/src/services/analytics.tsapp/src/utils/tauriCommands/memoryTree.tssrc/openhuman/config/ops/model.rssrc/openhuman/config/ops_tests.rssrc/openhuman/config/schemas/controllers.rssrc/openhuman/config/schemas/helpers.rssrc/openhuman/config/schemas/schema_defs.rssrc/openhuman/memory/tinycortex/summariser.rssrc/openhuman/memory/tree/tree/rpc.rs
🚧 Files skipped from review as they are similar to previous changes (23)
- src/openhuman/config/schemas/controllers.rs
- src/openhuman/config/ops_tests.rs
- src/openhuman/config/schemas/schema_defs.rs
- app/src/lib/i18n/ru.ts
- src/openhuman/config/ops/model.rs
- src/openhuman/config/schemas/helpers.rs
- app/src/components/intelligence/MemoryTreeStatusPanel.test.tsx
- src/openhuman/memory/tree/tree/rpc.rs
- app/src/lib/i18n/pl.ts
- app/src/lib/i18n/pt.ts
- app/src/lib/i18n/de.ts
- app/src/components/intelligence/MemoryTreeStatusPanel.tsx
- app/src/lib/i18n/ar.ts
- app/src/lib/i18n/id.ts
- app/src/lib/i18n/ko.ts
- app/src/lib/i18n/hi.ts
- app/src/lib/i18n/es.ts
- app/src/utils/tauriCommands/memoryTree.ts
- app/src/lib/i18n/bn.ts
- app/src/lib/i18n/en.ts
- app/src/lib/i18n/zh-CN.ts
- app/src/lib/i18n/fr.ts
- app/src/lib/i18n/it.ts
![tinysweeper[bot]](https://avatars.githubusercontent.com/in/4526585?s=60&v=4){kind=small}
There was a problem hiding this comment.
tinysweeper found nothing blocking. Approving.
$0.0137 · 153,657 in / 22,244 out · 54,071 cached (35%) · z-ai/glm-5.2, deepseek/deepseek-v4-pro
critique: $0.0070 · 55,170 in / 13,335 out · 16,785 cached (30%) · z-ai/glm-5.2, deepseek/deepseek-v4-pro
security: $0.0023 · 54,594 in / 2,684 out · 37,286 cached (68%) · z-ai/glm-5.2
tests: $0.0016 · 13,137 in / 3,256 out · 0 cached (0%) · z-ai/glm-5.2
description: $0.0015 · 13,939 in / 2,240 out · 0 cached (0%) · z-ai/glm-5.2
How this change flows1 changed behaviour across 3 relationships. 3 surrounding behaviours are shown (60 graph nodes walked). 40 further behaviours left out to keep the diagram readable. flowchart LR
n0["MemoryTreeStatusPanel<br/>changed"]:::changed
n1["tmp_config"]:::impacted
n2["format"]:::impacted
n3["join"]:::impacted
n0 -->|calls| n2
n1 -->|calls| n3
n3 -->|calls| n2
classDef changed fill:#0d4429,stroke:#238636,color:#e6edf3
classDef impacted fill:#161b22,stroke:#6e7681,color:#c9d1d9
classDef flagged fill:#5a1e02,stroke:#d93f0b,color:#ffffff
classDef blocking fill:#67060c,stroke:#f85149,color:#ffffff
Green: changed behaviour. Grey: surrounding behaviour. Arrows name the call, use, implementation, or test relationship. Orange: has findings. Red: has a finding that blocks the merge. |
tinysweeper
Bot
added
the
priority: p2
849ca00 to
1ed86a0
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
…losed
Review found the gate had a hole and the wrong failure direction.
**The hole.** It reused `summarizer_available`, which passes whenever local AI
is enabled. But `summarise` never reads `local_ai.runtime_enabled` — it resolves
the `summarization` role, which is `memory_provider` and falls back to the
managed cloud backend when that is unset. Local AI on with `memory_provider`
unset therefore passed the consent check while sending workspace memory to the
cloud: the exact bypass the gate exists to close.
The question is not "can we summarize" but "would this summary leave the
machine". `routes_to_cloud` asks `provider_for_role("summarization")` and checks
whether the answer is a local provider string, so the gate is on the destination
rather than on capability.
**The direction.** A failed consent re-read kept the startup snapshot. A config
read that errors is not evidence of consent, and a stale `true` would keep
sending after a withdrawal. It now refuses. The refusal is visible — the doctor
reports `summarizer_unavailable` and the panel renders the toggle beside it —
where continuing is silent.
Tests, all on a config anchored to its own temp file (`Config::default()` points
`config_path` at the real `~/.openhuman/config.toml`, so the gate's reload would
otherwise let the developer's own consent decide the assertion):
- a cloud route without consent refuses
- consent granted on disk after startup is honoured — constructed with consent
OFF so a gate that trusted its snapshot, or fell back to it, would fail
- consent withdrawn on disk after startup takes effect before the next summary
- local AI enabled does not excuse an un-consented cloud route
- a local route owes no cloud consent
summariser 5 pass; fmt clean.
Reported by CodeRabbit on tinyhumansai#5418.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SRSNnqQsokuGmkbpLoLCGy
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/openhuman/memory/tinycortex/summariser.rs (1)
190-203: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winProve that the grant test uses a cloud route.
The fixture does not assert that
grantedresolves to cloud. If it resolves local, the consent gate is bypassed and this test passes without testing the reload behavior. Setmemory_providerexplicitly if needed, then assertHostSummariser::routes_to_cloud(&granted)before the call.🤖 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/tinycortex/summariser.rs` around lines 190 - 203, Update the grant test around HostSummariser::new to explicitly configure granted.memory_provider for the cloud path, then assert HostSummariser::routes_to_cloud(&granted) before invoking summarise. Preserve the existing consent persistence and refusal assertion while ensuring the test exercises reload behavior for a cloud route.
🤖 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/memory/tinycortex/summariser.rs`:
- Around line 63-76: Add a trace-level diagnostic in `call` for the consent
decision, using the `[memory_tree]` prefix and including `context.tree_id`, the
result of `Self::routes_to_cloud(&self.config)`, and whether consent allows or
refuses the operation. Keep the record opaque and exclude summary content,
provider names, and credentials; preserve the existing refusal behavior.
---
Nitpick comments:
In `@src/openhuman/memory/tinycortex/summariser.rs`:
- Around line 190-203: Update the grant test around HostSummariser::new to
explicitly configure granted.memory_provider for the cloud path, then assert
HostSummariser::routes_to_cloud(&granted) before invoking summarise. Preserve
the existing consent persistence and refusal assertion while ensuring the test
exercises reload behavior for a cloud route.
🪄 Autofix
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 Plus
Run ID: 2f30d84a-3808-4eef-a21f-5821b1daa50b
📒 Files selected for processing (2)
src/openhuman/memory/tinycortex/summariser.rssrc/openhuman/web3/wallet/chains/btc.rs
|
Maintainer review pass (comment only — no approval, and I am not pushing to this branch). Verdict: the problem you found is still real and still unfixed on The user-facing problem is still liveI checked, because a month is a long time in this repo:
So the exact thing you filed #5417 about — a panel telling the user to set a flag that no control in the app can set — is unchanged. Nobody has overtaken you on the substance. What broke underneath you
This is the eviction wave: the summariser moved down into the loaded The rest of your files survived and should still apply:
In other words the frontend half — the toggle, the status payload, the locales — is probably close to still landing. It is the core-side consent gate that needs rethinking against the module seam. Ignore the red CI — it is not about your change
Neither file is yours, and On the design — worth preserving through the rewriteThree calls in your description are right and I would not want them lost when this is re-cut:
The one still-open review threadCodeRabbit asks for a Suggested path
Sorry this sat long enough to be overtaken by a refactor. The diagnosis in your description — the live instance frozen at 620 chunks with a 22-day-old newest chunk while the base namespace kept ingesting — is exactly the kind of evidence that makes a bug report actionable, and it is still worth acting on. |
f9c8cb6 to
fd45428
Compare
|
Rebased onto The Rust gate is half superseded, and the half that isn't is the one that matteredYou said the summariser's new home would need a different shape. It needs less than that, because
That is exactly what What is still open is the staleness half. struct ChatCallbacks(Arc<Config>);
// …
ChatCallbacks(Arc::clone(&config)) // memory_host.rs:480, onceand never refreshed. So the consent flag every summarization route is checked against is a boot-time snapshot. A user who opens the panel, unticks the box, and expects summaries to stop leaving the machine keeps having them sent to their cloud provider until the core restarts — the withdrawal is recorded on disk and ignored at the point of use. That is the same defect So the Rust half of this PR is now one small change in On the read-failure behaviour, which is a real design choice and not obvious: it fails closed only on the cloud route. A config that cannot be read is not evidence that consent still holds, so refusing is right — but blanket-refusing would break summarization for local-AI users, who never needed consent at all and whose route the ladder resolves without consulting the flag. So a failed re-read blocks the cloud path and leaves the local path alone. CodeRabbit's outstanding threadsBoth remaining ones were on The other (assert the grant test resolves to cloud) was specific to a fixture that no longer exists. The frontend half, which is the actual fix for #5417Unchanged and still needed — you confirmed it yourself: The |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
tinysweeper
Bot
added
priority: p3
coderabbitai
Bot
removed
feature
|
The diff-coverage gate came in at 70% (7 of 24 changed lines uncovered), and the gaps were in the two places this feature is least safe to leave unverified. Fixed rather than waived.
75 tests pass. The panel is at 96% line coverage with no uncovered lines, and |
`memory_tree.cloud_summarization_opt_in` is the flag the memory-health panel's own remediation text names when it reports `summarizer_unavailable` — and until now nothing in the app could set it. The user was told to flip a switch that did not exist, on any surface they had. Adds it end to end: - `MemorySettingsPatch.cloud_summarization_opt_in`, threaded through `config.update_memory_settings` and declared in its schema. Optional, so an older client posting a partial patch cannot silently revoke consent the user granted elsewhere. - `PipelineStatusResponse.cloud_summarization_opt_in`, so the control can render the *stored* value. A toggle showing a default would misreport whether memory summaries are allowed to leave the machine, which is the one thing this control must not get wrong. - The toggle itself in the memory-tree status panel, rendered unconditionally beside auto-sync rather than only while `summarizer_unavailable` is live: a control that appears with an error and vanishes when it clears cannot be used to *withdraw* the consent it granted. - Copy in all 14 locales, and a `memory_tree_cloud_summarization_changed` analytics event carrying only the boolean. Tests: both directions of the RPC patch (set, withdraw, and absent-means- unchanged) and three panel tests covering stored-value rendering, withdrawal, and the control's presence when the summarizer is healthy.
…t at boot
`main` already closed the other half of this hole: `resolve_chat_model`
routes the "summarization" role through the tree summarizer's ladder
instead of the blind role factory, so local AI being on no longer lets a
cloud-resolved fold slip past the opt-in. That is the check this branch
originally added as `HostSummariser::routes_to_cloud`, and it is dropped
rather than duplicated.
What is still open is *when* the flag is read. `ChatCallbacks` is built
once, at module-serve time, from an `Arc<Config>` that is never refreshed:
ChatCallbacks(Arc::clone(&config)) // memory_host.rs, once
So every summarization route is decided against a boot-time snapshot. A
user who unticks the box has their withdrawal written durably to disk and
ignored at the point of use — workspace memory keeps going to their cloud
provider until the core restarts. That is the worst shape a consent
control can have: it reports success and changes nothing.
The summarization role — and only it — now re-reads from disk before
resolving, the way `ComposioCallbacks::live_config` already does in this
file. Other roles carry no consent decision, and re-reading for all of
them would put a file read in front of every module-side model call.
A read failure refuses **only** the cloud route: a config that cannot be
read is not evidence that consent still holds, but blanket-refusing would
break summarization for local-AI users who never owed consent at all. So
the refusal is scoped to the case where the held snapshot says the route
would be the cloud one — exactly where the answer might have changed.
Also adds the consent-decision `debug` line CodeRabbit asked for on the
original gate: role, local-AI state, and the opt-in result. No summary
content, no provider strings, no credentials.
…C wrapper The diff-coverage gate came in at 70%, and the seven uncovered lines were the two places this feature is least safe to leave unverified: - `MemoryTreeStatusPanel.tsx:165-167` — the catch arm. The toggle is not optimistic; on failure the panel re-reads the stored value on the next poll. If the rejection were swallowed the switch would read "off" for a machine still summarising in the cloud, which is the one thing a consent control must never do. Now asserts the error toast *and* that the control re-enables, since a toggle stuck disabled after one failure cannot be used to withdraw consent later. - `memoryTree.ts:1013-1019` — `memoryTreeSetCloudSummarization` itself, which the panel tests mock away entirely. Now asserts the exact params (the shared memory-settings mutator applies only the fields it is given, so sending more would let this toggle rewrite the embedder), that an explicit `false` travels rather than being omitted (an absent field means "leave alone", which would make the off position a no-op), and that a transport failure propagates. 75 tests pass; the panel is at 96% line coverage with no uncovered lines, and `memoryTree.ts`'s remaining gaps are outside this diff.
b07a474 to
7f3b8fc
Compare
|
Rebased onto The red The failure is: That file is nowhere near this change. It gets pulled into the lane because Evidence it is not mine:
So it reads as a genuine flake in that test — a Everything this PR owns is green: |
|
we're moveing tree stuff so closing this pr |
3 participants
Closes #5417.
Summary
The memory-health panel tells the user to set
memory_tree.cloud_summarization_opt_in=truewhen "Build Summary Trees" has no summarizer. Nothing in the app could set it. This adds the toggle, on the surface that reports the problem.Problem
tree_runtime::ops::create_providerresolves the summarizer in three steps — local Ollama, else cloud when opted in, else error. A default headless install lands on the error, and the panel renders a remediation naming a flag with no control behind it:MemorySettingsPatchcarried six fields and not this one;config/tools.rsstates theconfig_update_*mutators are deliberately unexposed. The only routes wereOPENHUMAN_MEMORY_TREE_CLOUD_SUMMARIZATIONor hand-editingconfig.toml— both needing shell access to the host the app is supposed to manage.:11434, flipping it makessummarizer_availablereport true while every call fails at connect.Observed on a live instance: the tree's newest chunk 22 days old and frozen at 620 while the base namespace kept ingesting (a mail from 8 days ago was queryable),
summary_treereportingsummarizer_unavailable,local_ai.runtime_enabled = false,cloud_summarization_opt_in = false, and nothing for the user to press.Solution
MemorySettingsPatch+config_update_memory_settingscloud_summarization_opt_in;apply_memory_settingswrites itPipelineStatusResponseMemoryTreeStatusPanelThree decisions worth stating:
The status payload carries the value rather than the panel assuming a default. A switch that renders
falsewhen the stored value istruewould misreport whether memory summaries are allowed to leave the machine — the wrong thing to be casual about.The toggle renders unconditionally, not only while
summarizer_unavailableis live. A control that appears with the error and vanishes once it is fixed can grant consent but never withdraw it.The copy says what it does — summaries are sent to the configured provider. That is why the setting is opt-in rather than a default, and the UI should not lose that when it becomes a switch.
Out of scope: provisioning Ollama from the app. The local-AI path is untouched.
Acceptance criteria
config_update_memory_settingsaccepts and persists it.cloud_summarization_opt_inoff the status payload.config::ops99,memory_tree::tree::rpc17,MemoryTreeStatusPanel24;tsc --noEmitandpnpm i18n:checkclean (missing 0 / extra 0); fmt + clippy clean.Related
src/openhuman/memory/tree/tree_runtime/ops.rs—create_provider/summarizer_available.app/src/lib/i18n/*.ts—memory.health.remediation.summarizer_unavailable, the string that named the unreachable flag.Summary by CodeRabbit