fix(subconscious): route text-only tasks through cloud when local AI disabled#1565
Conversation
…disabled
When local AI is disabled, text-only subconscious tasks would
fail every tick with 'local_ai.usage.subconscious not enabled'.
Route them through the cloud agentic analysis path instead.
Before: text-only tasks → execute_with_local_model() → Err
After: text-only tasks → local AI check → local (if enabled)
→ cloud agent_analysis (fallback)
Closes tinyhumansai#1374
|
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:
📝 WalkthroughWalkthroughexecute_task loads Config and routes tasks: write-intent → full agent loop; needs-agent → analysis-only; text-only → local model when enabled or cloud analysis otherwise, with cloud analysis mapping "RECOMMENDED ACTION:" to UnapprovedWrite when applicable. ChangesExecution routing, agent delegates, and tests
Sequence DiagramsequenceDiagram
participant Client as execute_task
participant Config
participant Local as execute_with_local_model
participant Analysis as execute_with_agent_analysis
participant Full as execute_with_agent_full
Client->>Config: load_or_init()
Config-->>Client: Config (mutable)
alt write-intent
Client->>Full: execute_with_agent_full(&mut Config, task)
Full-->>Client: ExecutionOutcome
else needs_agent
Client->>Analysis: execute_with_agent_analysis(&mut Config, task)
Analysis-->>Client: analysis output
else text-only
Client->>Config: check local_ai.use_local_for_subconscious()
alt local enabled
Client->>Local: execute_with_local_model(&Config, task)
Local-->>Client: ExecutionOutcome
else local disabled
Client->>Analysis: execute_with_agent_analysis(&mut Config, task)
Analysis-->>Client: analysis output (RECOMMENDED ACTION handling)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/openhuman/subconscious/executor.rs (2)
145-145:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove stale TODO comment.
The TODO about wiring a cloud fallback has been completed — the caller (
execute_task) now implements the cloud fallback at lines 93-114. This comment is now stale and should be removed to avoid confusion.🧹 Proposed fix
-/// TODO: wire a cloud fallback here when use_local_for_subconscious is false. async fn execute_with_local_model(🤖 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/subconscious/executor.rs` at line 145, Remove the stale TODO comment in src/openhuman/subconscious/executor.rs that says "TODO: wire a cloud fallback here when use_local_for_subconscious is false" because the cloud fallback is already implemented by the caller in execute_task; simply delete that comment to avoid confusion and keep the file clean (look for the TODO near the executor module where execute_task is used).
155-169:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUnreachable check — dead code.
Since the caller (
execute_taskline 79) now checksuse_local_for_subconscious()before callingexecute_with_local_model, this check at line 155 will always pass and the error path at lines 161-168 is unreachable dead code. Additionally, the error message "no cloud fallback configured" is now inaccurate because the cloud fallback exists in the caller.Remove this check or update the function's contract. If
execute_with_local_modelis intended to be a private function only called fromexecute_task, the check is redundant. If it's meant to be called from multiple places, consider whether those call sites also implement the routing logic.🧹 Proposed fix: Remove unreachable code
async fn execute_with_local_model( task: &SubconsciousTask, situation_report: &str, identity_context: &str, ) -> Result<String, String> { let config = crate::openhuman::config::Config::load_or_init() .await .map_err(|e| format!("config load: {e}"))?; - if !config.local_ai.use_local_for_subconscious() { - // Fail fast rather than returning Ok("") — upstream code uses an - // empty string as a normal "task ran but produced no output" - // signal, so a silent skip would mask a disabled subsystem as a - // completed action. Surface the gate state so callers can - // distinguish "skipped" from "succeeded with empty output". - tracing::info!( - "[subconscious:executor] local_ai.usage.subconscious not enabled — \ - refusing to execute task '{}' (no cloud fallback configured)", - task.title - ); - return Err( - "local_ai.usage.subconscious not enabled (no cloud fallback configured)".to_string(), - ); - } - let prompt_text = prompt::build_text_execution_prompt(task, situation_report, identity_context);Note: This assumes you also implement the fix from the previous comment to pass
configto avoid redundant loading.🤖 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/subconscious/executor.rs` around lines 155 - 169, The conditional guard in execute_with_local_model that checks config.local_ai.use_local_for_subconscious() and returns an error is now dead/unreachable because execute_task already performs that check; remove this redundant if-block and its Err return (or alternatively make execute_with_local_model private and document its precondition to require the caller has validated use_local_for_subconscious()), and ensure callers supply the config (as per the earlier change) so you don't re-load config inside execute_with_local_model.
🤖 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/subconscious/executor.rs`:
- Around line 75-92: The code redundantly loads Config and re-checks
use_local_for_subconscious() inside execute_with_local_model; refactor by
changing execute_with_local_model to accept a &crate::openhuman::config::Config
(or &Config) and the existing task, situation_report, identity_context
parameters, then remove the Config::load_or_init call and the
use_local_for_subconscious() check from inside execute_with_local_model; update
the caller that currently calls execute_with_local_model(...) to pass the
already-loaded config from Config::load_or_init(), ensuring only the caller
performs routing based on config::use_local_for_subconscious() and the callee
only performs local execution logic.
---
Outside diff comments:
In `@src/openhuman/subconscious/executor.rs`:
- Line 145: Remove the stale TODO comment in
src/openhuman/subconscious/executor.rs that says "TODO: wire a cloud fallback
here when use_local_for_subconscious is false" because the cloud fallback is
already implemented by the caller in execute_task; simply delete that comment to
avoid confusion and keep the file clean (look for the TODO near the executor
module where execute_task is used).
- Around line 155-169: The conditional guard in execute_with_local_model that
checks config.local_ai.use_local_for_subconscious() and returns an error is now
dead/unreachable because execute_task already performs that check; remove this
redundant if-block and its Err return (or alternatively make
execute_with_local_model private and document its precondition to require the
caller has validated use_local_for_subconscious()), and ensure callers supply
the config (as per the earlier change) so you don't re-load config inside
execute_with_local_model.
🪄 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: 3edefad9-03b5-4c96-81f6-6579478a822d
📒 Files selected for processing (1)
src/openhuman/subconscious/executor.rs
execute_with_local_model was reloading Config and re-checking use_local_for_subconscious(), duplicating work already done by the caller in execute_task(). Pass the already-loaded Config as a parameter instead. The gate check remains in the caller (execute_task) which routes to the cloud fallback when local AI is disabled.
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
Review — 2 actionable findings, 1 nitpick, 2 questions
The core fix is correct and valuable — cloud fallback for text-only tasks when local AI is disabled. Second commit cleaning up the config threading is good. See inline comments for details.
Verified / looks good:
- No bare
.unwrap(), proper[subconscious:executor]log prefix, no PII in logs UnapprovedWritehandling mirrors existingneeds_agentbranch exactlyexecute_with_local_modelconfig threading is correct- No new standalone
.rsfiles atsrc/openhuman/root
| task.title | ||
| ); | ||
| let output = | ||
| execute_with_agent_analysis(task, situation_report, identity_context).await?; |
There was a problem hiding this comment.
[major] Double config load on the cloud fallback path
execute_task loads Config at lines 75–77, then calls execute_with_agent_analysis here, which loads Config again internally. This is the same class of issue CodeRabbit caught on the local path (and you fixed in commit 2) — but on the cloud path.
Every cloud-fallback tick pays two async config reads, and there’s a TOCTOU window: if config changes between the two loads, the branch decision at line 79 was made with config_A but the cloud call runs with config_B.
Fix is the same pattern from commit 2 — pass &mut Config to execute_with_agent_analysis:
async fn execute_with_agent_analysis(
config: &mut crate::openhuman::config::Config,
task: &SubconsciousTask,
situation_report: &str,
identity_context: &str,
) -> Result<String, String> {
let prompt_text = prompt::build_analysis_only_prompt(task, situation_report, identity_context);
agent_chat_with_retry(config, &prompt_text).await
}Note: the needs_agent branch (line 56) also calls execute_with_agent_analysis without a pre-loaded config — that call site would need updating too.
| ExecutionOutcome::Completed(ExecutionResult { | ||
| .map_err(|e| format!("config load: {e}"))?; | ||
|
|
||
| if config.local_ai.use_local_for_subconscious() { |
There was a problem hiding this comment.
[major] No tests for the new routing branch
The if config.local_ai.use_local_for_subconscious() decision here (lines 79–114) is completely untested. The existing test suite covers needs_tools, needs_agent, and extract_recommended_action — all pure functions — but nothing exercises this new conditional routing.
Issue #1374 criterion 6 explicitly requires regression safety for the unavailable-provider and enabled-provider paths, and criterion 7 requires ≥ 80% diff coverage (enforced by the CI coverage gate).
Minimum bar: two #[tokio::test] cases — one with use_local_for_subconscious() = false asserting cloud routing, one with true asserting local routing. If Config::load_or_init is hard to mock at the unit level, an integration test in tests/ exercising the RPC method with the mock backend would also work.
| task.title | ||
| ); | ||
| execute_with_local_model(task, situation_report, identity_context) | ||
| // Simple text-only task. Use local model if available, otherwise |
There was a problem hiding this comment.
[nitpick] Comment says "if available" but the actual gate is use_local_for_subconscious() — a deliberate config flag, not availability. Tighter wording:
// Simple text-only task. Use local model if configured for subconscious
// tasks, otherwise fall back to the cloud agentic analysis path.| execute_with_agent_analysis(task, situation_report, identity_context).await?; | ||
| let duration_ms = started.elapsed().as_millis() as u64; | ||
|
|
||
| if let Some(recommendation) = extract_recommended_action(&output) { |
There was a problem hiding this comment.
[question] When the cloud-fallback path returns UnapprovedWrite, the recommendation surfaces in the escalation UI. For a task the user defined as text-only (passive verb like "Summarize emails"), is it intentional that it can produce a write escalation via RECOMMENDED ACTION:? The needs_agent branch already does this, but those tasks are explicitly flagged as complex. For a simple text-only task hitting this path, the UX consequence seems surprising. Worth documenting the intent even if the behavior is correct.
| duration_ms: started.elapsed().as_millis() as u64, | ||
| }) | ||
| }) | ||
| } else { |
There was a problem hiding this comment.
[question] Issue #1374 criteria 2 (both-unavailable UI state), 3 (evaluation gating), and 4 (recovery without restart) are not addressed by this PR, but the PR closes #1374. Are these tracked as follow-ups, or intentionally descoped? If descoped, consider partially closing the issue or filing a follow-up so the issue tracker reflects what’s actually done.
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
Review — 2 blockers, 3 suggestions
The core fix is correct and valuable — cloud fallback for text-only tasks when local AI is disabled. Config-hoisting refactor is clean. But two questions from the previous review remain unanswered (findings 1 & 2 below are blocking), plus three non-blocking suggestions.
Verified / looks good:
- Config hoisting correct — single load, threaded to all helpers
&Config(immutable) for local path,&mut Configfor agent paths — matches downstream signaturesEnvVarGuard+TEST_ENV_LOCKconsistent with existing config tests- No bare
.unwrap(), proper[subconscious:executor]prefix, no PII in logs - File stays well under 500 lines, no new standalone
.rsat root
| execute_with_agent_analysis(&mut config, task, situation_report, identity_context).await?; | ||
| let duration_ms = started.elapsed().as_millis() as u64; | ||
|
|
||
| if let Some(recommendation) = extract_recommended_action(&output) { |
There was a problem hiding this comment.
[major] UnapprovedWrite from text-only tasks via cloud fallback — unanswered question
A passive task like "Summarize emails" can now produce a write escalation if the cloud model includes RECOMMENDED ACTION: in its output. This silently changes the task's contract from read-only to write-capable — the needs_tools gate was supposed to be the write-intent signal, and this path bypasses it.
Two options:
- Suppress — force
Completedon the text-only cloud-fallback path (never produceUnapprovedWritefor passive tasks) - Keep and document — add a comment explaining that cloud analysis can surface recommendations even from read-only tasks, and that the approval gate still applies
Either is fine but the PR needs an explicit decision. This was flagged in the previous review and remains unanswered.
| duration_ms: started.elapsed().as_millis() as u64, | ||
| }) | ||
| }) | ||
| } else { |
There was a problem hiding this comment.
[major] PR closes #1374 but only criterion 1 of 4 is addressed
Issue #1374 acceptance criteria:
- ✅ Repro gone — cloud fallback routing (this PR)
- ❌ Unavailable-state UI — no UI changes
- ❌ Evaluation gating — tasks still scheduled when no provider available
- ❌ Recovery without restart — not addressed
`Closes #1374` in the PR body will auto-close the issue on merge, silently dropping criteria 2–4. Either:
- Change to `Partially addresses Subconscious intelligence repeatedly fails when local AI is disabled #1374` and file follow-up issues for 2–4, or
- Update issue Subconscious intelligence repeatedly fails when local AI is disabled #1374 to reflect the reduced scope before merging
| ); | ||
| let output = | ||
| execute_with_agent_analysis(&mut config, task, situation_report, identity_context).await?; | ||
| let duration_ms = started.elapsed().as_millis() as u64; |
There was a problem hiding this comment.
[minor] No exit/completion logging on cloud-fallback path
Entry logs at `info!` (line 84) but there's no log on completion, on the `UnapprovedWrite` branch, or on error from `execute_with_agent_analysis`. CLAUDE.md requires entry/exit logging on new flows.
Suggested additions after the `let output = ...` line:
```rust
debug!(
"[subconscious:executor] text task cloud fallback complete: {} — duration_ms={}",
task.title, duration_ms
);
```
And on the `UnapprovedWrite` arm:
```rust
info!(
"[subconscious:executor] text task cloud fallback produced write recommendation: {}",
task.title
);
```
| ExecutionOutcome::Completed(ExecutionResult { | ||
| // Simple text-only task. Use local model if configured for subconscious | ||
| // tasks, otherwise fall back to the cloud agentic analysis path. | ||
| if config.local_ai.use_local_for_subconscious() { |
There was a problem hiding this comment.
[minor] Test assertions too loose to verify actual routing
Both routing tests assert `is_err()` + error string contains `"agent execution"` or `"local model"`. If routing logic inverts (e.g., the `if` condition is flipped), both could still pass because connection failures produce similar error shapes.
Stronger approach: use distinct error-prefix strings in `execute_with_local_model` vs `execute_with_agent_analysis` so the test can assert the specific path, not just that something failed. Alternatively, a `cfg(test)` counter or mock seam.
| .map_err(|e| format!("config load: {e}"))?; | ||
| let output = execute_with_agent_full(&mut config, task, situation_report, identity_context).await?; | ||
| Ok(ExecutionResult { | ||
| output, |
There was a problem hiding this comment.
[nitpick] `execute_approved_write` loads config independently — TOCTOU window
This is a separate entry point so an independent `Config::load_or_init()` is reasonable. But it creates a TOCTOU window: the task was originally routed under config_A in `execute_task`, and now the approved write executes under config_B. If `use_local_for_subconscious()` toggled between the two calls, the approval was made under different assumptions.
Low risk, but worth a one-line comment documenting that this is an intentional independent load.
…s review feedback Graycyrus review findings addressed: - [blocker] Suppress UnapprovedWrite on text-only cloud-fallback path — passive tasks never escalate, preserves write-intent gate contract - [blocker] PR body updated: Closes tinyhumansai#1374 → Partially addresses tinyhumansai#1374 - [minor] Add debug! exit logging on cloud-fallback completion path - [minor] Distinct error prefix for cloud fallback routing tests - [nitpick] TOCTOU comment on execute_approved_write - [fix] Add #[derive(Debug)] to ExecutionOutcome for test unwrap_err()
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/subconscious/executor.rs`:
- Around line 405-443: The tests
execute_task_routes_to_cloud_when_local_disabled and
execute_task_routes_to_local_when_local_enabled are relying on real backend
failures; instead add a test seam so execute_task's backend calls can be faked:
modify execute_task to accept an optional dependency/injectable function or
trait (or use a cfg/test-only feature) for the two backends
execute_with_local_model and execute_with_agent_analysis, then in the tests
replace those with deterministic fakes that return distinct sentinel errors or
results so the assertions verify which path was chosen; update the tests to
inject the fake implementations (referencing execute_task,
execute_with_local_model, execute_with_agent_analysis, and the two test
functions) and assert against the sentinel values rather than expecting
incidental real-provider errors.
- Around line 80-109: The logs in the executor paths are printing user-authored
PII via task.title; update the logging calls around execute_with_local_model and
execute_with_agent_analysis to avoid emitting task.title and instead log task.id
and route metadata (e.g., local vs cloud fallback) as structured fields, e.g.
change the debug/info messages that reference task.title to include task.id and
a stable prefix like "[subconscious:executor]" plus the route ("local" or
"cloud-fallback"), and keep duration_ms and used_tools as before; ensure any
other debug line that currently interpolates task.title is similarly switched to
task.id to prevent PII leakage.
🪄 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: 38d78e37-c9b8-4e2b-809d-0532b758d5e0
📒 Files selected for processing (1)
src/openhuman/subconscious/executor.rs
…t mocks - Replace task.title with task.id in all 6 log statements to avoid logging user-authored content (PII). - Add #[cfg(test)] test_mocks module with atomic state so routing tests can force deterministic failures from execute_with_local_model / execute_with_agent_analysis without depending on real provider state (Ollama running, cloud credentials). - Both routing tests now exercise branch selection independently of infrastructure, per review feedback.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/openhuman/subconscious/executor.rs (1)
103-151: 💤 Low valueAlign log levels across text-task branches.
use_local_for_subconscious()=truelogs atdebug!(Line 107), while the cloud-fallback branch logs atinfo!(Line 121). Both fire on every tick for the same class of work (simple text-only tasks); when local AI is disabled, this becomes an info-level entry per task per tick. The completion log on Line 134 is alreadydebug!, so the entry log is the outlier. Recommenddebug!for symmetry and to match the per-tick cadence —info!is more appropriate for one-off state transitions, which the write-intent (Line 68) andneeds_agent(Line 83) branches arguably are.As per coding guidelines: "Use
log/tracingatdebug/tracelevel; include stable grep-friendly prefixes ([domain],[rpc]) and correlation fields".♻️ Suggested change
- info!( - "[subconscious:executor] text task: id={} — local AI disabled, using cloud fallback", - task.id - ); + debug!( + "[subconscious:executor] text task: id={} — local AI disabled, using cloud fallback", + task.id + );🤖 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/subconscious/executor.rs` around lines 103 - 151, The cloud-fallback branch logs the text-task start at info! while the local path uses debug! causing inconsistent per-tick noise; in the else branch that calls execute_with_agent_analysis (the branch guarded by config.local_ai.use_local_for_subconscious() == false) change the info! call to debug! and keep the same grep-friendly prefix and correlation field (the "[subconscious:executor] text task: id={}" message) so both simple text-only task entry logs use debug level and match the completion debug! log and the local-model branch.
🤖 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.
Nitpick comments:
In `@src/openhuman/subconscious/executor.rs`:
- Around line 103-151: The cloud-fallback branch logs the text-task start at
info! while the local path uses debug! causing inconsistent per-tick noise; in
the else branch that calls execute_with_agent_analysis (the branch guarded by
config.local_ai.use_local_for_subconscious() == false) change the info! call to
debug! and keep the same grep-friendly prefix and correlation field (the
"[subconscious:executor] text task: id={}" message) so both simple text-only
task entry logs use debug level and match the completion debug! log and the
local-model branch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 15453678-b17c-41a4-bf9d-7c41c9cc9bc0
📒 Files selected for processing (1)
src/openhuman/subconscious/executor.rs
3 participants
Summary
When local AI is disabled, text-only subconscious tasks (summarize, check, monitor, review, analyze, extract, classify) would fail every tick with
local_ai.usage.subconscious not enabled (no cloud fallback configured), filling the activity log with repeatedExecution failedentries.Changes
src/openhuman/subconscious/executor.rs— Modified the text-only task routing inexecute_task():use_local_for_subconscious()returnstrue): use the local model as beforeexecute_with_agent_analysis()(cloud agentic-v1), which was already working for read-only tasks needing deeper reasoningThis is the exact fix the TODO at line 117 was asking for: "wire a cloud fallback here when use_local_for_subconscious is false."
Impact
Execution failedentries in the activity log when local AI is disabledPartially addresses #1374
Summary by CodeRabbit
Bug Fixes
Improvements
Tests