feat(subconscious): stabilize heartbeat + subconscious loop (#392)#437
Merged
senamakel merged 6 commits intotinyhumansai:mainfrom Apr 8, 2026
Merged
Conversation
Contributor
|
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:
📝 WalkthroughWalkthroughReplaces the in-memory decision-log subconscious with a SQLite-backed task pipeline: persistent scheduling, per-task evaluation and execution, escalation lifecycle with approval gating, new executor/store/prompt/types, expanded RPC/tauri surfaces, frontend hook/UI, docs, and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Intelligence Page
participant Hook as useSubconscious Hook
participant Tauri as Tauri Commands
participant Engine as SubconsciousEngine
participant Store as SQLite Store
participant Model as Local Ollama Model
participant Agent as Agentic API
UI->>Hook: mount / start poll
Hook->>Tauri: fetch {tasks, escalations, logs, status}
Tauri->>Store: query tables
Store-->>Tauri: rows
Tauri-->>Hook: data
Hook-->>UI: render
UI->>Hook: Run Now / triggerTick()
Hook->>Tauri: subconsciousTrigger()
Tauri->>Engine: spawn tick (background)
Engine->>Store: load due_tasks()
Store-->>Engine: tasks
Engine->>Store: insert in_progress logs
Engine->>Model: evaluate tasks (batch prompt)
Model-->>Engine: evaluations[]
alt task needs tools
Engine->>Agent: execute_with_agent_full()
Agent-->>Engine: result
else task needs agent analysis
Engine->>Agent: execute_with_agent_analysis()
Agent-->>Engine: result (may include recommendation)
alt recommendation produced
Engine->>Store: create escalation
end
else text-only
Engine->>Model: execute_with_local_model()
Model-->>Engine: result
end
Engine->>Store: update logs / task run_times / create escalations
Engine-->>Tauri: done (background)
Tauri-->>Hook: eventual refresh via poll
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/openhuman/heartbeat/engine.rs (1)
206-210:⚠️ Potential issue | 🟠 MajorTest will hang: default config now has
enabled: true.
HeartbeatConfig::default()now setsenabled: true, so this test will enter the infinite loop instead of returning immediately. The test needs an explicit disabled config.🐛 Proposed fix
#[tokio::test] async fn run_returns_immediately_when_disabled() { - let engine = HeartbeatEngine::new(HeartbeatConfig::default(), std::env::temp_dir()); + let config = HeartbeatConfig { + enabled: false, + ..HeartbeatConfig::default() + }; + let engine = HeartbeatEngine::new(config, std::env::temp_dir()); let result = engine.run().await; assert!(result.is_ok()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openhuman/heartbeat/engine.rs` around lines 206 - 210, The test run_returns_immediately_when_disabled uses HeartbeatConfig::default(), but default now has enabled: true so the test hangs; update the test to pass an explicit disabled config to HeartbeatEngine::new (e.g. construct a HeartbeatConfig with enabled: false while preserving other defaults) before calling engine.run() so the engine returns immediately.
🧹 Nitpick comments (15)
src/openhuman/config/schema/heartbeat_cron.rs (1)
33-35: Consider documenting the behavioral change for existing users.These new defaults (
enabled: true,inference_enabled: true, 5-minute interval) will silently activate for existing installations that have no explicit[heartbeat]section in theirconfig.toml, due to#[serde(default)]on the heartbeat field. Users upgrading may be surprised by the new background inference activity.Consider adding a note to the PR description or release notes about this change, and possibly a one-time log message on first activation to inform users.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openhuman/config/schema/heartbeat_cron.rs` around lines 33 - 35, The new defaults in heartbeat_cron.rs (enabled: true, inference_enabled: true, interval_minutes: 5) will activate for existing installs because the parent config uses #[serde(default)] for the heartbeat field; update the PR/release notes to call out this behavioral change and add a one-time informational log when the HeartbeatConfig (or struct in heartbeat_cron.rs) is implicitly defaulted/first activated so users are notified; locate the HeartbeatConfig/default impl and the code path that reads the config (where serde(default) is applied) and emit a clear log message on first activation indicating the defaulted settings and how to opt out.docs/memory-sync-functions.md (2)
244-286: Line numbers in API reference may drift.The hard-coded line numbers (e.g.,
put_docat line 105) will become stale asclient.rsevolves. Consider removing them or linking to source via GitHub blob URLs with commit SHAs for stability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/memory-sync-functions.md` around lines 244 - 286, The API reference uses hard-coded line numbers that will drift; update docs/memory-sync-functions.md by removing the numeric line references (e.g., entries for put_doc, put_doc_light, ingest_doc, list_documents, list_namespaces, query_namespace, recall_namespace, kv_set, graph_upsert, graph_query, etc.) or replace them with stable GitHub blob links including commit SHAs that point to the exact symbol in source; ensure each table row references the function name/signature only (or a SHA-based URL) and remove the inline line number column so the doc stays accurate as client.rs changes.
13-13: Add language specifier to fenced code block.The decision tree code block lacks a language specifier. Use
textorplaintextto satisfy markdownlint (MD040).📝 Proposed fix
-``` +```text Need to store data?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/memory-sync-functions.md` at line 13, The fenced code block containing the decision-tree text "Need to store data?" is missing a language specifier; update the opening fence to include a language like ```text or ```plaintext (e.g., change ``` to ```text) so the markdownlint rule MD040 is satisfied and the block is explicitly marked as plain text.app/src/hooks/useSubconscious.ts (1)
88-98: Stale closure:triggeringin dependency array causes unnecessary recreation.Including
triggeringin the dependency array meanstriggerTickis recreated on every state change, but the guardif (triggering)at line 89 still works because it reads the currenttriggeringvalue. However, this pattern is fragile—the callback is recreated unnecessarily.Consider using a ref to track the triggering state, similar to
fetchingRef:♻️ Proposed fix using ref
+ const triggeringRef = useRef(false); + const triggerTick = useCallback(async () => { - if (!isTauri() || triggering) return; + if (!isTauri() || triggeringRef.current) return; + triggeringRef.current = true; setTriggering(true); try { await subconsciousTrigger(); } catch (err) { console.warn('[subconscious] trigger failed:', err); } finally { setTriggering(false); + triggeringRef.current = false; } - }, [triggering]); + }, []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/hooks/useSubconscious.ts` around lines 88 - 98, The triggerTick callback is being recreated because it depends on the triggering state; replace that dependency with a ref so the callback is stable: create a triggeringRef (similar to fetchingRef), update triggeringRef.current whenever setTriggering(true/false) is called (or wrap setTriggering to also set the ref), use triggeringRef.current in the guard inside triggerTick rather than the triggering state, and remove triggering from triggerTick's dependency array so triggerTick remains stable; keep function name triggerTick and state setter setTriggering referenced so you can locate and update the code.app/src/pages/Intelligence.tsx (2)
647-657: Toggle switch accessibility: Add aria-label.The custom toggle switch button lacks an accessible label for screen readers. Consider adding an
aria-labeloraria-labelledbyattribute.<button onClick={() => toggleSubconsciousTask(task.id, !task.enabled)} + aria-label={`${task.enabled ? 'Disable' : 'Enable'} task: ${task.title}`} className={`relative w-7 h-4 rounded-full flex-shrink-0 transition-colors ${🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/pages/Intelligence.tsx` around lines 647 - 657, The custom toggle button for subconscious tasks lacks an accessible label; update the <button> rendering inside the Intelligence.tsx component where toggleSubconsciousTask(task.id, !task.enabled) is called to add an aria-label or aria-labelledby that describes the action (e.g., "Enable subconscious task" / "Disable subconscious task" based on task.enabled). Use task.id or task.name to generate a unique label if needed and ensure the aria text reflects the current state so screen readers understand the toggle's purpose and state.
721-775: Complex nested ternary for log entry display.This deeply nested ternary chain for determining log entry text based on decision type is functional but challenging to read. Consider extracting to a helper function for maintainability.
♻️ Optional refactor to improve readability
const getLogEntryText = (entry: SubconsciousLogEntry): string => { if (entry.result) return entry.result; const decisionTextMap: Record<string, string> = { noop: 'Nothing new', act: 'Completed', in_progress: 'Evaluating...', escalate: 'Waiting for approval', failed: 'Failed', cancelled: 'Cancelled', dismissed: 'Skipped', }; return decisionTextMap[entry.decision] ?? entry.decision; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/pages/Intelligence.tsx` around lines 721 - 775, The nested ternary that builds the visible log text should be extracted into a small helper (e.g. getLogEntryText(entry: SubconsciousLogEntry): string) to improve readability and maintainability; implement getLogEntryText to return entry.result if present, otherwise map entry.decision to the human-readable strings (noop -> "Nothing new", act -> "Completed", in_progress -> "Evaluating...", escalate -> "Waiting for approval", failed -> "Failed", cancelled -> "Cancelled", dismissed -> "Skipped") and fallback to entry.decision, then replace the large ternary in the JSX with a call to this helper and keep the existing expandedLogIds/truncation logic (use getLogEntryText(entry) for length checks and display).docs/subconscious.md (3)
63-63: Minor: Fix spacing in inline code span.The markdownlint tool flagged spaces inside the code span. The
-marker should not have trailing space inside the backticks or the backticks should be removed.-Additional system tasks are imported from `HEARTBEAT.md` in the workspace directory (one task per `- ` line). +Additional system tasks are imported from `HEARTBEAT.md` in the workspace directory (one task per `- ` list item).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/subconscious.md` at line 63, Update the inline code span that contains the list marker: change the backticked "`- ` " to either "`-`" (no trailing space inside the backticks) or remove the backticks and leave a plain "- " so markdownlint no longer flags spacing inside the code span; the affected text references "HEARTBEAT.md" and the "(one task per `- ` line)" phrase.
9-31: Minor: Add language specifier to ASCII diagram code block.The markdownlint tool flagged this fenced code block as missing a language specifier. For ASCII art diagrams, use
textorplaintextto silence the warning.-``` +```text ┌─────────────────────────────────────────────────────────┐🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/subconscious.md` around lines 9 - 31, The fenced ASCII diagram block is missing a language tag which triggers markdownlint; update the opening fence from ``` to ```text (or ```plaintext) for the diagram that begins with "┌─────────────────────────────────────────────────────────┐" and contains "Heartbeat Engine" and "Subconscious Engine" so the markdown linter recognizes it as plain text and the warning is silenced.
106-128: Minor: Add language specifier to flow diagram code block.Similar to the architecture diagram, this fenced code block should have a language specifier for linting compliance.
-``` +```text Decision: noop🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/subconscious.md` around lines 106 - 128, The fenced flow-diagram block that begins with "Decision: noop" (and contains "Decision: act" / "Decision: escalate") is missing a language specifier; update the opening fence from ``` to ```text (or another appropriate language) so the block starts with ```text to satisfy the linter and match the architecture diagram style.src/openhuman/subconscious/schemas.rs (1)
446-471: Remove duplicate helper function.
fieldandfield_reqare identical. Remove one to reduce duplication.♻️ Suggested fix
-fn field(name: &'static str, ty: TypeSchema, comment: &'static str) -> FieldSchema { - FieldSchema { - name, - ty, - comment, - required: true, - } -} - fn field_req(name: &'static str, ty: TypeSchema, comment: &'static str) -> FieldSchema { FieldSchema { name, ty, comment, required: true, } }Then update usages of
field(...)tofield_req(...)in the schema definitions (lines 79, 86, 97, 115, 131, 142, 152, 163-166, 178, 189, 196).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openhuman/subconscious/schemas.rs` around lines 446 - 471, Remove the duplicate helper by deleting one of the identical functions (either remove fn field(...) or fn field_req(...)) and keep a single canonical helper (e.g., fn field_req(name: &'static str, ty: TypeSchema, comment: &'static str) -> FieldSchema) that constructs FieldSchema { name, ty, comment, required: true }. Then update all call sites that used the removed name to call the remaining helper (replace field(...) usages with field_req(...) if you kept field_req), ensuring references to FieldSchema, TypeSchema and the field_opt helper remain unchanged.src/openhuman/subconscious/executor.rs (1)
213-234: Simplifyextract_recommended_actionloop structure.The current implementation uses
filter_mapinside aforloop but returns on the first match, making thefilter_mapunnecessary. The logic can be simplified.♻️ Suggested simplification
fn extract_recommended_action(output: &str) -> Option<String> { - // Look for "RECOMMENDED ACTION:" marker in the output - for line_idx in output.lines().enumerate().filter_map(|(i, l)| { - if l.trim().starts_with("RECOMMENDED ACTION:") { - Some(i) - } else { - None - } - }) { - let recommendation: String = output + let line_idx = output + .lines() + .position(|l| l.trim().starts_with("RECOMMENDED ACTION:"))?; + + let recommendation: String = output .lines() .skip(line_idx) .collect::<Vec<_>>() .join("\n") .trim() .to_string(); - if !recommendation.is_empty() { - return Some(recommendation); - } + + if recommendation.is_empty() { + None + } else { + Some(recommendation) } - None }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openhuman/subconscious/executor.rs` around lines 213 - 234, The loop in extract_recommended_action is overcomplicated: replace the for + filter_map pattern with a single iterator that finds the first line starting with "RECOMMENDED ACTION:" and returns the joined remainder; for example, use output.lines().enumerate().find_map(|(i, l)| if l.trim().starts_with("RECOMMENDED ACTION:") { let rec = output.lines().skip(i).collect::<Vec<_>>().join("\n").trim().to_string(); if rec.is_empty() { None } else { Some(rec) } } else { None }) to simplify and return the first non-empty recommendation.src/openhuman/subconscious/engine.rs (1)
139-146: Race condition incancel_stale_in_progress: unconditionally cancels all in-progress entries.The function at
store.rs:276-283updates ALL rows withdecision='in_progress'without checking generation. Whiletick_generation(an in-memory AtomicU64) tracks which tick is active, this value is never stored in the log table or checked during cancellation. If two ticks start nearly simultaneously, the second tick'scancel_stale_in_progresswill wipe out in-progress entries created by the first tick.Though unlikely in practice (scheduled ticks sleep first, manual triggers spawn async), this is fragile. Adding a generation column to
subconscious_logand filtering by generation incancel_stale_in_progresswould make this more robust and eliminate timing assumptions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openhuman/subconscious/engine.rs` around lines 139 - 146, The cancel_stale_in_progress path currently wipes all rows with decision='in_progress' regardless of which tick created them; add a generation column to the subconscious_log table, persist the current tick_generation (the AtomicU64 used by tick_generation) into that column when creating in_progress log entries, and change store::cancel_stale_in_progress to accept the current generation and only update rows where decision='in_progress' AND generation != provided_generation (so only previous-tick in_progress entries are cancelled); update the callsite in engine.rs (where store::cancel_stale_in_progress is invoked via store::with_connection) to pass the current tick_generation value.src/openhuman/subconscious/store.rs (3)
36-83: Missing foreign key constraints ontask_idcolumns.While
PRAGMA foreign_keys = ONis set,subconscious_log.task_idandsubconscious_escalations.task_idlack foreign key constraints. Deleting a user task will leave orphaned log entries and escalations.Consider adding FK constraints with
ON DELETE CASCADEor implementing cascade deletion inremove_task:🛡️ Proposed schema fix
CREATE TABLE IF NOT EXISTS subconscious_log ( id TEXT PRIMARY KEY, - task_id TEXT NOT NULL, + task_id TEXT NOT NULL REFERENCES subconscious_tasks(id) ON DELETE CASCADE, tick_at REAL NOT NULL, ... ); CREATE TABLE IF NOT EXISTS subconscious_escalations ( id TEXT PRIMARY KEY, - task_id TEXT NOT NULL, + task_id TEXT NOT NULL REFERENCES subconscious_tasks(id) ON DELETE CASCADE, log_id TEXT, ... );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openhuman/subconscious/store.rs` around lines 36 - 83, The schema in SCHEMA_DDL is missing foreign key constraints linking subconscious_log.task_id and subconscious_escalations.task_id to subconscious_tasks.id; add FOREIGN KEY (task_id) REFERENCES subconscious_tasks(id) ON DELETE CASCADE to both CREATE TABLE statements (or if you prefer code-side handling, update the remove_task implementation to explicitly delete rows from subconscious_log and subconscious_escalations where task_id = id before deleting the task) so orphaned logs/escalations cannot remain when a subconscious_tasks row is removed.
146-166: Non-atomic update with multiple statements.Each
UPDATEis a separate transaction. If one succeeds and the next fails, the task is left in a partially updated state. Consider combining into a single statement:♻️ Proposed atomic update
pub fn update_task(conn: &Connection, task_id: &str, patch: &TaskPatch) -> Result<()> { - if let Some(ref title) = patch.title { - conn.execute( - "UPDATE subconscious_tasks SET title = ?1 WHERE id = ?2", - rusqlite::params![title, task_id], - )?; - } - if let Some(ref recurrence) = patch.recurrence { - conn.execute( - "UPDATE subconscious_tasks SET recurrence = ?1 WHERE id = ?2", - rusqlite::params![recurrence_to_string(recurrence), task_id], - )?; - } - if let Some(enabled) = patch.enabled { - conn.execute( - "UPDATE subconscious_tasks SET enabled = ?1 WHERE id = ?2", - rusqlite::params![enabled, task_id], - )?; - } + conn.execute( + "UPDATE subconscious_tasks SET + title = COALESCE(?1, title), + recurrence = COALESCE(?2, recurrence), + enabled = COALESCE(?3, enabled) + WHERE id = ?4", + rusqlite::params![ + patch.title.as_deref(), + patch.recurrence.as_ref().map(recurrence_to_string), + patch.enabled, + task_id + ], + )?; Ok(()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openhuman/subconscious/store.rs` around lines 146 - 166, The update_task function currently runs multiple independent UPDATEs which can leave the row partially modified; change update_task to perform the update atomically by building one UPDATE statement (or wrapping in a single transaction) that sets only the provided fields from TaskPatch in one execution and uses bound parameters; reference update_task, TaskPatch, recurrence_to_string, and conn to locate where to construct a dynamic SET clause (title, recurrence converted via recurrence_to_string, enabled) and execute a single conn.execute (or start a rusqlite::Transaction and commit after all updates) so the operation is all-or-nothing.
87-119: Consider simplifying enum-to-string conversion.Using
serde_json::to_value(&source).unwrap_or_default().as_str().unwrap_or("user")is fragile and indirect. A simple match is clearer and avoids JSON serialization overhead:♻️ Proposed simplification
+fn source_to_string(source: &TaskSource) -> &'static str { + match source { + TaskSource::System => "system", + TaskSource::User => "user", + } +} + pub fn add_task( conn: &Connection, title: &str, source: TaskSource, recurrence: TaskRecurrence, ) -> Result<SubconsciousTask> { let id = Uuid::new_v4().to_string(); let now = now_secs(); - let source_str = serde_json::to_value(&source) - .unwrap_or_default() - .as_str() - .unwrap_or("user") - .to_string(); + let source_str = source_to_string(&source); let recurrence_str = recurrence_to_string(&recurrence);The same pattern applies to
add_escalation(lines 332-336) andlist_escalations(lines 362-366).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openhuman/subconscious/store.rs` around lines 87 - 119, The code in add_task (and similarly in add_escalation and list_escalations) converts TaskSource to a string via serde_json::to_value(...).as_str() which is indirect, fragile and allocates; replace that with an explicit match on the TaskSource enum that returns the correct &str or String for DB storage (e.g., match source { TaskSource::User => "user", TaskSource::System => "system", ... }) and use that matched string in the SQL params and when constructing SubconsciousTask; update add_escalation and list_escalations the same way to remove the JSON serialization, unwraps, and improve clarity and safety.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/hooks/useSubconscious.ts`:
- Line 1: Prettier formatting errors exist in the useSubconscious hook file; run
the formatter and re-save the file (e.g., run prettier --write on the file) or
apply the project's Prettier rules so the file containing the useSubconscious
hook conforms to style rules; after formatting, re-run ESLint/CI to ensure all
style issues are resolved before merging.
In `@app/src/pages/Intelligence.tsx`:
- Around line 1-2: The Intelligence.tsx file has Prettier formatting issues
(affecting imports like useCallback, useEffect, useMemo, useState and
useNavigate); fix by running the project's formatter and linter on the app code
(e.g. run the Prettier write command for app/src/pages/Intelligence.tsx and then
run ESLint with --fix or the repo's lint script against app/) and commit the
reformatted file so CI no longer flags formatting errors.
In `@app/src/utils/tauriCommands/subconscious.ts`:
- Around line 1-5: Run Prettier and ESLint autofix on the modified subconscious
module to resolve the CI formatting errors: run npx prettier --write on the file
containing the Subconscious engine code and then npx eslint --fix for the app
sources, then re-stage the resulting changes; specifically ensure imports and
comment formatting around the Subconscious module and its symbols (callCoreRpc,
isTauri, CommandResponse) adhere to the project's Prettier/ESLint rules before
merging.
In `@src/openhuman/subconscious/store.rs`:
- Around line 701-729: The tests assert wrong counts because seed_from_heartbeat
always seeds DEFAULT_SYSTEM_TASKS (3 items) before importing HEARTBEAT.md;
update the two tests (seed_from_heartbeat_imports_tasks and
seed_from_missing_heartbeat_returns_zero) to expect the default tasks plus
imports: in seed_from_heartbeat_imports_tasks assert count == 5 (3 defaults + 2
imported), tasks.len() == 5 and verify their source is TaskSource::System; in
seed_from_missing_heartbeat_returns_zero assert count == 3 to reflect the seeded
DEFAULT_SYSTEM_TASKS.
---
Outside diff comments:
In `@src/openhuman/heartbeat/engine.rs`:
- Around line 206-210: The test run_returns_immediately_when_disabled uses
HeartbeatConfig::default(), but default now has enabled: true so the test hangs;
update the test to pass an explicit disabled config to HeartbeatEngine::new
(e.g. construct a HeartbeatConfig with enabled: false while preserving other
defaults) before calling engine.run() so the engine returns immediately.
---
Nitpick comments:
In `@app/src/hooks/useSubconscious.ts`:
- Around line 88-98: The triggerTick callback is being recreated because it
depends on the triggering state; replace that dependency with a ref so the
callback is stable: create a triggeringRef (similar to fetchingRef), update
triggeringRef.current whenever setTriggering(true/false) is called (or wrap
setTriggering to also set the ref), use triggeringRef.current in the guard
inside triggerTick rather than the triggering state, and remove triggering from
triggerTick's dependency array so triggerTick remains stable; keep function name
triggerTick and state setter setTriggering referenced so you can locate and
update the code.
In `@app/src/pages/Intelligence.tsx`:
- Around line 647-657: The custom toggle button for subconscious tasks lacks an
accessible label; update the <button> rendering inside the Intelligence.tsx
component where toggleSubconsciousTask(task.id, !task.enabled) is called to add
an aria-label or aria-labelledby that describes the action (e.g., "Enable
subconscious task" / "Disable subconscious task" based on task.enabled). Use
task.id or task.name to generate a unique label if needed and ensure the aria
text reflects the current state so screen readers understand the toggle's
purpose and state.
- Around line 721-775: The nested ternary that builds the visible log text
should be extracted into a small helper (e.g. getLogEntryText(entry:
SubconsciousLogEntry): string) to improve readability and maintainability;
implement getLogEntryText to return entry.result if present, otherwise map
entry.decision to the human-readable strings (noop -> "Nothing new", act ->
"Completed", in_progress -> "Evaluating...", escalate -> "Waiting for approval",
failed -> "Failed", cancelled -> "Cancelled", dismissed -> "Skipped") and
fallback to entry.decision, then replace the large ternary in the JSX with a
call to this helper and keep the existing expandedLogIds/truncation logic (use
getLogEntryText(entry) for length checks and display).
In `@docs/memory-sync-functions.md`:
- Around line 244-286: The API reference uses hard-coded line numbers that will
drift; update docs/memory-sync-functions.md by removing the numeric line
references (e.g., entries for put_doc, put_doc_light, ingest_doc,
list_documents, list_namespaces, query_namespace, recall_namespace, kv_set,
graph_upsert, graph_query, etc.) or replace them with stable GitHub blob links
including commit SHAs that point to the exact symbol in source; ensure each
table row references the function name/signature only (or a SHA-based URL) and
remove the inline line number column so the doc stays accurate as client.rs
changes.
- Line 13: The fenced code block containing the decision-tree text "Need to
store data?" is missing a language specifier; update the opening fence to
include a language like ```text or ```plaintext (e.g., change ``` to ```text) so
the markdownlint rule MD040 is satisfied and the block is explicitly marked as
plain text.
In `@docs/subconscious.md`:
- Line 63: Update the inline code span that contains the list marker: change the
backticked "`- ` " to either "`-`" (no trailing space inside the backticks) or
remove the backticks and leave a plain "- " so markdownlint no longer flags
spacing inside the code span; the affected text references "HEARTBEAT.md" and
the "(one task per `- ` line)" phrase.
- Around line 9-31: The fenced ASCII diagram block is missing a language tag
which triggers markdownlint; update the opening fence from ``` to ```text (or
```plaintext) for the diagram that begins with
"┌─────────────────────────────────────────────────────────┐" and contains
"Heartbeat Engine" and "Subconscious Engine" so the markdown linter recognizes
it as plain text and the warning is silenced.
- Around line 106-128: The fenced flow-diagram block that begins with "Decision:
noop" (and contains "Decision: act" / "Decision: escalate") is missing a
language specifier; update the opening fence from ``` to ```text (or another
appropriate language) so the block starts with ```text to satisfy the linter and
match the architecture diagram style.
In `@src/openhuman/config/schema/heartbeat_cron.rs`:
- Around line 33-35: The new defaults in heartbeat_cron.rs (enabled: true,
inference_enabled: true, interval_minutes: 5) will activate for existing
installs because the parent config uses #[serde(default)] for the heartbeat
field; update the PR/release notes to call out this behavioral change and add a
one-time informational log when the HeartbeatConfig (or struct in
heartbeat_cron.rs) is implicitly defaulted/first activated so users are
notified; locate the HeartbeatConfig/default impl and the code path that reads
the config (where serde(default) is applied) and emit a clear log message on
first activation indicating the defaulted settings and how to opt out.
In `@src/openhuman/subconscious/engine.rs`:
- Around line 139-146: The cancel_stale_in_progress path currently wipes all
rows with decision='in_progress' regardless of which tick created them; add a
generation column to the subconscious_log table, persist the current
tick_generation (the AtomicU64 used by tick_generation) into that column when
creating in_progress log entries, and change store::cancel_stale_in_progress to
accept the current generation and only update rows where decision='in_progress'
AND generation != provided_generation (so only previous-tick in_progress entries
are cancelled); update the callsite in engine.rs (where
store::cancel_stale_in_progress is invoked via store::with_connection) to pass
the current tick_generation value.
In `@src/openhuman/subconscious/executor.rs`:
- Around line 213-234: The loop in extract_recommended_action is
overcomplicated: replace the for + filter_map pattern with a single iterator
that finds the first line starting with "RECOMMENDED ACTION:" and returns the
joined remainder; for example, use output.lines().enumerate().find_map(|(i, l)|
if l.trim().starts_with("RECOMMENDED ACTION:") { let rec =
output.lines().skip(i).collect::<Vec<_>>().join("\n").trim().to_string(); if
rec.is_empty() { None } else { Some(rec) } } else { None }) to simplify and
return the first non-empty recommendation.
In `@src/openhuman/subconscious/schemas.rs`:
- Around line 446-471: Remove the duplicate helper by deleting one of the
identical functions (either remove fn field(...) or fn field_req(...)) and keep
a single canonical helper (e.g., fn field_req(name: &'static str, ty:
TypeSchema, comment: &'static str) -> FieldSchema) that constructs FieldSchema {
name, ty, comment, required: true }. Then update all call sites that used the
removed name to call the remaining helper (replace field(...) usages with
field_req(...) if you kept field_req), ensuring references to FieldSchema,
TypeSchema and the field_opt helper remain unchanged.
In `@src/openhuman/subconscious/store.rs`:
- Around line 36-83: The schema in SCHEMA_DDL is missing foreign key constraints
linking subconscious_log.task_id and subconscious_escalations.task_id to
subconscious_tasks.id; add FOREIGN KEY (task_id) REFERENCES
subconscious_tasks(id) ON DELETE CASCADE to both CREATE TABLE statements (or if
you prefer code-side handling, update the remove_task implementation to
explicitly delete rows from subconscious_log and subconscious_escalations where
task_id = id before deleting the task) so orphaned logs/escalations cannot
remain when a subconscious_tasks row is removed.
- Around line 146-166: The update_task function currently runs multiple
independent UPDATEs which can leave the row partially modified; change
update_task to perform the update atomically by building one UPDATE statement
(or wrapping in a single transaction) that sets only the provided fields from
TaskPatch in one execution and uses bound parameters; reference update_task,
TaskPatch, recurrence_to_string, and conn to locate where to construct a dynamic
SET clause (title, recurrence converted via recurrence_to_string, enabled) and
execute a single conn.execute (or start a rusqlite::Transaction and commit after
all updates) so the operation is all-or-nothing.
- Around line 87-119: The code in add_task (and similarly in add_escalation and
list_escalations) converts TaskSource to a string via
serde_json::to_value(...).as_str() which is indirect, fragile and allocates;
replace that with an explicit match on the TaskSource enum that returns the
correct &str or String for DB storage (e.g., match source { TaskSource::User =>
"user", TaskSource::System => "system", ... }) and use that matched string in
the SQL params and when constructing SubconsciousTask; update add_escalation and
list_escalations the same way to remove the JSON serialization, unwraps, and
improve clarity and safety.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: d5a4f687-6b65-4c60-a10a-e3f4e6bceacc
⛔ Files ignored due to path filters (1)
app/src-tauri/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
app/src/hooks/useSubconscious.tsapp/src/pages/Intelligence.tsxapp/src/utils/tauriCommands/index.tsapp/src/utils/tauriCommands/subconscious.tsdocs/memory-sync-functions.mddocs/subconscious.mdsrc/openhuman/config/schema/heartbeat_cron.rssrc/openhuman/heartbeat/engine.rssrc/openhuman/subconscious/decision_log.rssrc/openhuman/subconscious/engine.rssrc/openhuman/subconscious/executor.rssrc/openhuman/subconscious/integration_test.rssrc/openhuman/subconscious/mod.rssrc/openhuman/subconscious/prompt.rssrc/openhuman/subconscious/schemas.rssrc/openhuman/subconscious/store.rssrc/openhuman/subconscious/types.rstests/subconscious_e2e.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/openhuman/config/schema/heartbeat_cron.rs (1)
14-17:⚠️ Potential issue | 🟡 MinorSerde default for
inference_enableddoesn't matchDefaultimpl.The
#[serde(default)]attribute uses serde's bool default (false), but theDefaultimpl on line 35 setsinference_enabled: true. If a user'sconfig.tomlhas a[heartbeat]section but omitsinference_enabled, serde will deserialize it asfalse— contrary to the intended default.Either add a helper function like
default_inference_enabled() -> bool { true }and use#[serde(default = "default_inference_enabled")], or accept that partial configs will disable inference.🛠️ Proposed fix
+fn default_inference_enabled() -> bool { + true +} + /// Enable subconscious inference (local model evaluation). /// When false, the heartbeat only counts tasks without reasoning. -#[serde(default)] +#[serde(default = "default_inference_enabled")] pub inference_enabled: bool,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openhuman/config/schema/heartbeat_cron.rs` around lines 14 - 17, The serde default for the struct field inference_enabled currently falls back to false but the type's Default impl sets inference_enabled: true, causing inconsistent behavior; fix by adding a helper like default_inference_enabled() -> bool that returns true and change the field attribute to #[serde(default = "default_inference_enabled")], or alternatively update the Default impl to match serde's false—make sure to update the field attribute and the Default impl for the struct so both use the same canonical default for inference_enabled.src/openhuman/heartbeat/engine.rs (1)
206-210:⚠️ Potential issue | 🔴 CriticalTest will hang indefinitely after default change.
HeartbeatConfig::default()now hasenabled: true, sorun()enters the infinite loop instead of returning early. This test will never complete.🐛 Proposed fix
#[tokio::test] async fn run_returns_immediately_when_disabled() { - let engine = HeartbeatEngine::new(HeartbeatConfig::default(), std::env::temp_dir()); + let config = HeartbeatConfig { + enabled: false, + ..HeartbeatConfig::default() + }; + let engine = HeartbeatEngine::new(config, std::env::temp_dir()); let result = engine.run().await; assert!(result.is_ok()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openhuman/heartbeat/engine.rs` around lines 206 - 210, The test run_returns_immediately_when_disabled() relies on HeartbeatConfig being disabled but uses HeartbeatConfig::default(), which now sets enabled: true and causes run() to block; update the test to construct a config with enabled: false (e.g. HeartbeatConfig { enabled: false, ..Default::default() } or use an existing helper like HeartbeatConfig::disabled()) and pass that to HeartbeatEngine::new before calling run() so run() returns immediately.
🧹 Nitpick comments (8)
docs/memory-sync-functions.md (1)
208-217: Mark thistokio::spawnpattern as best-effort only (or prefer awaiting).This example detaches writes and loses direct error propagation; during shutdown, queued ingestion can be dropped. For default guidance, prefer awaiting
put_doc()and reserve detached spawn for explicitly best-effort telemetry/event writes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/memory-sync-functions.md` around lines 208 - 217, Update the example to explicitly label the tokio::spawn + memory_client.clone() + client.put_doc(input).await pattern as "best-effort only" and advise preferring awaiting put_doc() in normal code paths; mention that detached tasks can be dropped during shutdown and recommend alternate approaches (await the future, propagate the Result, or use a managed background worker with shutdown/join) and reserve tokio::spawn detachment only for non-critical telemetry/event writes.app/src/pages/Intelligence.tsx (3)
32-34: Use arrow function per coding guidelines.As per coding guidelines: "Prefer arrow functions over function declarations".
-function isSkillRelated(title: string, description: string): boolean { - return SKILL_KEYWORDS.test(title) || SKILL_KEYWORDS.test(description); -} +const isSkillRelated = (title: string, description: string): boolean => + SKILL_KEYWORDS.test(title) || SKILL_KEYWORDS.test(description);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/pages/Intelligence.tsx` around lines 32 - 34, The function is declared using a function declaration instead of an arrow function; convert the function isSkillRelated(title: string, description: string): boolean { ... } into a const isSkillRelated = (title: string, description: string): boolean => { return SKILL_KEYWORDS.test(title) || SKILL_KEYWORDS.test(description); } so it follows the arrow-function guideline and still uses SKILL_KEYWORDS to check title and description.
587-594: Potential silent failure on dismiss before navigation.
dismissEscalation(esc.id)is called without awaiting or handling errors beforenavigate('/skills'). If the dismissal fails, the user navigates away without feedback. Consider awaiting and handling errors, or accept that navigation takes priority.<button - onClick={() => { - dismissEscalation(esc.id); - navigate('/skills'); + onClick={async () => { + try { + await dismissEscalation(esc.id); + } catch { + // Best-effort dismiss; navigate regardless + } + navigate('/skills'); }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/pages/Intelligence.tsx` around lines 587 - 594, The Fix in Skills button currently calls dismissEscalation(esc.id) and immediately calls navigate('/skills'), risking silent failures; update the onClick handler in the component rendering that button to await dismissEscalation(esc.id) and handle errors (e.g., try/catch) before calling navigate('/skills')—on success navigate, on failure show a user-visible error or toast and avoid navigating so the user can retry or see the failure; keep references to isSkillRelated, dismissEscalation, navigate, and esc.id when locating and fixing the code.
721-775: Consider extracting log entry display logic.The 8-level nested ternary is hard to read and maintain. A lookup object or helper function would improve clarity.
♻️ Suggested refactor
const LOG_DECISION_DEFAULTS: Record<string, string> = { noop: 'Nothing new', act: 'Completed', in_progress: 'Evaluating...', escalate: 'Waiting for approval', failed: 'Failed', cancelled: 'Cancelled', dismissed: 'Skipped', }; // Then in the render: {entry.result ? expandedLogIds.has(entry.id) ? entry.result : entry.result.length > 120 ? `${entry.result.substring(0, 120)}...` : entry.result : LOG_DECISION_DEFAULTS[entry.decision] ?? entry.decision}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/pages/Intelligence.tsx` around lines 721 - 775, Replace the deep nested ternaries that compute the display string for a log entry in Intelligence.tsx (currently using entry.result, expandedLogIds, entry.id and entry.decision) with a concise helper or lookup map (e.g., LOG_DECISION_DEFAULTS) that maps decision keys to default messages and a small function (e.g., formatLogResult(entry, expandedLogIds)) that returns either the full/trimmed entry.result or the lookup value (or fallback to entry.decision); update the JSX span to call that helper and remove the 8-level ternary so the logic is readable and maintainable.docs/subconscious.md (1)
63-63: Minor: Trailing space inside code span.The markdown linter flagged
-(line 63) as having a space inside the code span. Consider removing the trailing space or using a different formatting approach.-Additional system tasks are imported from `HEARTBEAT.md` in the workspace directory (one task per `- ` line). +Additional system tasks are imported from `HEARTBEAT.md` in the workspace directory (one task per `- ` (dash-space) line).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/subconscious.md` at line 63, The inline code span contains a trailing space (`- `) which triggers the markdown linter; edit the sentence in subconscious.md to remove the trailing space inside the backticks (change "`- ` line" to "`-` line" or rephrase to use a non-code span format), ensuring the code span content is exactly "-" with no trailing whitespace.src/openhuman/heartbeat/engine.rs (1)
42-45: Sleep-first means delayed first tick.The first tick now occurs after
interval_minutes(default 5 min). Previously withinterval().tick(), the first tick could fire immediately. If immediate task seeding/evaluation on startup is desired, consider triggering an initial tick before entering the loop, or document this as intentional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openhuman/heartbeat/engine.rs` around lines 42 - 45, Sleep-first behavior delays the initial heartbeat; call the same tick/processing path once immediately before entering the loop instead of waiting on time::sleep. Locate the loop using sleep_secs (variables interval_mins, sleep_secs, time::sleep(Duration::from_secs(sleep_secs)).await) in engine.rs and invoke the existing tick handler or seed/evaluate routine once (the same code executed inside the loop) right before the loop; keep the periodic sleep loop unchanged so subsequent ticks still occur every interval.src/openhuman/subconscious/store.rs (2)
146-166: Non-atomic multi-field update risks partial writes.Each field is updated in a separate SQL statement. If an error occurs midway (or between statements), partial updates are committed. Consider wrapping in a transaction or building a single dynamic UPDATE.
♻️ Proposed fix: Single UPDATE statement
pub fn update_task(conn: &Connection, task_id: &str, patch: &TaskPatch) -> Result<()> { - if let Some(ref title) = patch.title { - conn.execute( - "UPDATE subconscious_tasks SET title = ?1 WHERE id = ?2", - rusqlite::params![title, task_id], - )?; - } - if let Some(ref recurrence) = patch.recurrence { - conn.execute( - "UPDATE subconscious_tasks SET recurrence = ?1 WHERE id = ?2", - rusqlite::params![recurrence_to_string(recurrence), task_id], - )?; - } - if let Some(enabled) = patch.enabled { - conn.execute( - "UPDATE subconscious_tasks SET enabled = ?1 WHERE id = ?2", - rusqlite::params![enabled, task_id], - )?; - } + let mut sets = Vec::new(); + let mut params: Vec<Box<dyn rusqlite::types::ToSql>> = Vec::new(); + + if let Some(ref title) = patch.title { + sets.push(format!("title = ?{}", params.len() + 1)); + params.push(Box::new(title.clone())); + } + if let Some(ref recurrence) = patch.recurrence { + sets.push(format!("recurrence = ?{}", params.len() + 1)); + params.push(Box::new(recurrence_to_string(recurrence))); + } + if let Some(enabled) = patch.enabled { + sets.push(format!("enabled = ?{}", params.len() + 1)); + params.push(Box::new(enabled)); + } + + if sets.is_empty() { + return Ok(()); + } + + params.push(Box::new(task_id.to_string())); + let sql = format!( + "UPDATE subconscious_tasks SET {} WHERE id = ?{}", + sets.join(", "), + params.len() + ); + conn.execute(&sql, rusqlite::params_from_iter(params.iter()))?; Ok(()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openhuman/subconscious/store.rs` around lines 146 - 166, The update_task function performs separate UPDATE statements per field which can lead to partial commits; modify update_task (and use Connection/TaskPatch symbols) to perform the update atomically by starting a transaction (conn.transaction()) or by building a single dynamic UPDATE SQL that only sets provided fields (title, recurrence via recurrence_to_string, enabled) and executes once with the assembled params against the subconscious_tasks row for id; ensure the transaction is committed or rolled back on error so no partial writes occur.
95-99: Consider a safer serialization helper to avoid silent defaults.This pattern relies on
serde_json::to_valuereturning a JSON string. IfTaskSource's serde representation changes (e.g., to an object), this silently falls back to"user". The same pattern appears forEscalationPriority(lines 332-336) andEscalationStatus(lines 362-366, 392-396).A dedicated helper or explicit
matchwould be more robust:♻️ Example: explicit match instead of serde fallback
fn source_to_string(s: &TaskSource) -> &'static str { match s { TaskSource::System => "system", TaskSource::User => "user", } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openhuman/subconscious/store.rs` around lines 95 - 99, The current use of serde_json::to_value(...).as_str().unwrap_or("user") for source_str (and analogous uses for EscalationPriority and EscalationStatus) can silently default if the serde representation changes; replace these with explicit helper functions that match each enum variant (e.g., implement source_to_string(&TaskSource) -> &'static str, priority_to_string(&EscalationPriority) -> &'static str, status_to_string(&EscalationStatus) -> &'static str) and call those helpers to produce the String values instead of relying on serde_json::to_value; update the places using source_str and the priority/status variables to use the new helpers so serialization is deterministic and fails loudly if enums change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/hooks/useSubconscious.ts`:
- Around line 69-82: The fetch logic in useSubconscious (the block calling
subconsciousTasksList, subconsciousEscalationsList, subconsciousLogList,
subconsciousStatus and then setTasks/setEscalations/setLogEntries/setStatus)
swallows individual RPC failures because each promise is caught to null before
Promise.all, so the outer catch never runs; change to Promise.allSettled for
those four calls, inspect each result and apply only the fulfilled values (using
unwrap(...) where currently used) to preserve partial updates, and if any result
is rejected call setError with the error message (or a combined message) so
refresh failures are surfaced instead of being silently ignored.
In `@app/src/utils/tauriCommands/subconscious.ts`:
- Around line 74-79: The subconsciousTrigger wrapper returns the old TickResult
type but the RPC now returns { triggered: true }; update the function signature
and its RPC call generic to match the new contract: change subconsciousTrigger's
Promise<CommandResponse<TickResult>> to Promise<CommandResponse<{ triggered:
boolean }>> (or a named interface like SubconsciousTriggerResponse) and ensure
any callers that expect evaluations/duration_ms are updated to handle the new {
triggered: boolean } shape; modify the callCoreRpc generic in
subconsciousTrigger accordingly and remove or refactor any dependencies on
TickResult for this path.
In `@docs/memory-sync-functions.md`:
- Around line 70-77: The example uses Option wrappers and ..Default::default()
but the actual struct NamespaceDocumentInput expects required owned fields
(e.g., key: String, source_type: String, metadata: Value) and does not implement
Default; update the examples to construct NamespaceDocumentInput with the
required fields as concrete values (remove Some(...) wrappers and the struct
update shorthand), supply a JSON Value for metadata (e.g., serde_json::json!
macro) and ensure all required fields (key, source_type, metadata, namespace,
title, content) are provided explicitly when calling client.put_doc so the
snippet matches the types defined in NamespaceDocumentInput.
- Line 143: The documentation incorrectly states that NamespaceRetrievalContext
exposes top-level entities, relations, and chunks; update the wording to reflect
the actual structure returned by the API: NamespaceRetrievalContext yields a
collection of hits (type namespace/query/context_text/hits) where each hit
contains its own entities, relations, chunks, and score breakdowns. Modify the
description to say "returns hits with per-hit entities, relations, chunks, and
score breakdowns" (or list the exact field names as defined on the hit type) and
remove any claims of top-level entities/relations/chunks to prevent API misuse;
apply the same correction to the other affected occurrences.
- Line 13: The fenced code block used for the decision-tree is missing a
language tag; update the triple-backtick fence (the decision-tree fence) to
include a language specifier such as ```text so markdownlint MD040 is satisfied
and the block is explicitly annotated.
- Line 201: The table row and related note currently imply per-integration
isolation for the clear_skill_memory API; update the wording to state that the
skill namespace is shared across integrations as `skill-{skill}` and that the
`integration_id` parameter controls targeted revocation behavior (not namespace
partitioning). Modify the `clear_skill_memory(skill_id, integration_id)`
description and the related note at the second mention (line ~229) to explicitly
say: "Skill namespaces use a shared `skill-{skill}` namespace across
integrations; `integration_id` is only used to target revocation logic for that
integration's tokens/permissions." Reference the `clear_skill_memory` function
name, the `skill-{skill}` namespace format, and the `integration_id` parameter
when making the edits.
In `@docs/subconscious.md`:
- Line 305: Update the documentation text that currently reads "Auto-polls every
2s" to match the implementation's 3-second polling; specifically change that
phrase to "Auto-polls every 3s" (the implementation uses setInterval(refresh,
3000)), so the docs align with the actual behavior.
In `@src/openhuman/config/schema/heartbeat_cron.rs`:
- Around line 33-35: The new heartbeat defaults (enabled: true,
inference_enabled: true, interval_minutes: 5) cause heartbeat/inference to run
for users without an explicit [heartbeat] section; change the schema defaults so
users are opted out by default — e.g., set enabled: false and inference_enabled:
false (or remove #[serde(default)] so missing configs don't enable the feature),
and keep interval_minutes as-is; update the default values in the HeartbeatCron
(enabled, inference_enabled, interval_minutes) definition accordingly so only
explicit configs opt-in.
In `@src/openhuman/subconscious/engine.rs`:
- Around line 133-136: The code sets state.seeded = true unconditionally after
calling seed_tasks(), which swallows errors, so a failed fallback seed prevents
future retries; update seed_tasks() (or add a new seed_tasks_fallback() wrapper)
to return a Result/boolean indicating success and only set state.seeded = true
when that call succeeds (i.e., check the returned Result/ok or boolean from
seed_tasks()/seed_tasks_fallback() before flipping state.seeded), ensuring
seed_tasks() errors are propagated/logged and retried on subsequent ticks if it
failed.
- Around line 371-390: The code currently resolves the escalation and
advances/completes the task even when execute_approved_write returned Err;
change the control flow so only successful executions (Ok from
execute_approved_write) call store::resolve_escalation,
store::mark_task_completed, or self.advance_task_schedule_in_conn; always write
the "act" log via store::add_log_entry with the error text on Err, but do NOT
call store::resolve_escalation, store::mark_task_completed, or
advance_task_schedule_in_conn when result is Err. Locate execute_approved_write,
the match that sets result_text/duration, and the calls to store::add_log_entry,
store::resolve_escalation, store::mark_task_completed, and
self.advance_task_schedule_in_conn to implement this conditional behavior.
- Around line 235-240: The current evaluation_failed check (in engine.rs around
the evaluation loop) only looks for TickDecision::Noop with a reason starting
with "Evaluation failed:", but evaluate_tasks() also treats config-load failures
and unparseable model responses as failing evaluations; update this so those
cases stop last_tick_at from advancing and increment consecutive_failures. Fix
by adding an explicit error flag on the Evaluation struct (e.g., bool errored or
enum EvaluationOutcome) and have evaluate_tasks() set that flag for config-load
failures, parse/unparseable responses, and existing "Evaluation failed:" paths;
then change the evaluation_failed computation to e.errored (or match the enum)
instead of string-matching the reason. Ensure references: Evaluation struct,
evaluate_tasks(), evaluation_failed variable, TickDecision::Noop, last_tick_at,
and consecutive_failures are updated accordingly.
- Around line 270-287: The fallback that marks tasks as noop when the model
omits them only updates the log (using store::update_log_entry) but does not
call the same scheduling advancement used by the normal noop branch, so omitted
tasks remain due and get retried; update the omitted-task branch (the loop over
due_tasks that builds evaluated_task_ids and checks log_ids) to call
advance_task_schedule with the same arguments and behavior as the normal noop
path immediately after updating the log entry (or invoke the same helper used by
the noop branch), ensuring the task schedule is advanced and cron/pending state
is updated like a normal noop; reference symbols: evaluated_task_ids, due_tasks,
log_ids, store::update_log_entry, and advance_task_schedule.
In `@src/openhuman/subconscious/executor.rs`:
- Around line 182-201: The retry loop uses MAX_RETRIES = 3 but because you
increment attempt before checking and use the condition if is_rate_limit &&
attempt < MAX_RETRIES you only sleep/retry for attempts 1 and 2; change the
check to if is_rate_limit && attempt <= MAX_RETRIES (or rename MAX_RETRIES to
MAX_ATTEMPTS and adjust accordingly) so the third backoff (8s) is allowed;
update the condition in the loop around the call to
crate::openhuman::local_ai::ops::agent_chat and keep the existing backoff
calculation (backoff_secs = 2u64 << (attempt - 1)).
- Around line 33-50: The current write-authorization uses free-text matching via
needs_tools(&task.title) before calling execute_with_agent_full, which is
unsafe; change the gating to require an explicit task capability/approval flag
(e.g., task.approved_for_write or task.capabilities.contains("write")) instead
of substring heuristics on task.title, and only call execute_with_agent_full
when that explicit flag is true; update the same check locations (the block
around needs_tools & execute_with_agent_full and the other occurrence mentioned)
to reference the explicit approval field, and keep needs_tools for a
non-authoritative hint only if desired.
In `@src/openhuman/subconscious/schemas.rs`:
- Around line 211-234: The handler currently derives last_tick_at and
total_ticks from the subconscious_log and sets consecutive_failures to 0, which
loses empty ticks and failure streaks; modify the code path (the
store::with_connection block that queries subconscious_log and the construction
of super::types::SubconsciousStatus) to read these values from a persisted stats
row (add a new store accessor like store::get_subconscious_stats or extend the
existing store API to return persisted last_tick_at, total_ticks, and
consecutive_failures) that engine.tick() updates when it advances, or
alternatively read from a cached snapshot API exposed by the engine instead of
querying subconscious_log; update the DB schema and store write paths to persist
these fields, and ensure the handler falls back to sensible defaults if the
persisted row is missing for backward compatibility.
In `@src/openhuman/subconscious/store.rs`:
- Around line 702-729: The test assertions are wrong because
seed_from_heartbeat(&conn, dir.path()) inserts DEFAULT_SYSTEM_TASKS first (3)
and then heartbeat tasks; update the expectations in
seed_from_heartbeat_imports_tasks to expect count == 5 (3 defaults + 2
heartbeat), keep count2 == 0 (no duplicates on second run), and assert
tasks.len() == 5 with all Task.source == TaskSource::System; also update
seed_from_missing_heartbeat_returns_zero to expect count == 3 (the
DEFAULT_SYSTEM_TASKS) when HEARTBEAT.md is missing.
---
Outside diff comments:
In `@src/openhuman/config/schema/heartbeat_cron.rs`:
- Around line 14-17: The serde default for the struct field inference_enabled
currently falls back to false but the type's Default impl sets
inference_enabled: true, causing inconsistent behavior; fix by adding a helper
like default_inference_enabled() -> bool that returns true and change the field
attribute to #[serde(default = "default_inference_enabled")], or alternatively
update the Default impl to match serde's false—make sure to update the field
attribute and the Default impl for the struct so both use the same canonical
default for inference_enabled.
In `@src/openhuman/heartbeat/engine.rs`:
- Around line 206-210: The test run_returns_immediately_when_disabled() relies
on HeartbeatConfig being disabled but uses HeartbeatConfig::default(), which now
sets enabled: true and causes run() to block; update the test to construct a
config with enabled: false (e.g. HeartbeatConfig { enabled: false,
..Default::default() } or use an existing helper like
HeartbeatConfig::disabled()) and pass that to HeartbeatEngine::new before
calling run() so run() returns immediately.
---
Nitpick comments:
In `@app/src/pages/Intelligence.tsx`:
- Around line 32-34: The function is declared using a function declaration
instead of an arrow function; convert the function isSkillRelated(title: string,
description: string): boolean { ... } into a const isSkillRelated = (title:
string, description: string): boolean => { return SKILL_KEYWORDS.test(title) ||
SKILL_KEYWORDS.test(description); } so it follows the arrow-function guideline
and still uses SKILL_KEYWORDS to check title and description.
- Around line 587-594: The Fix in Skills button currently calls
dismissEscalation(esc.id) and immediately calls navigate('/skills'), risking
silent failures; update the onClick handler in the component rendering that
button to await dismissEscalation(esc.id) and handle errors (e.g., try/catch)
before calling navigate('/skills')—on success navigate, on failure show a
user-visible error or toast and avoid navigating so the user can retry or see
the failure; keep references to isSkillRelated, dismissEscalation, navigate, and
esc.id when locating and fixing the code.
- Around line 721-775: Replace the deep nested ternaries that compute the
display string for a log entry in Intelligence.tsx (currently using
entry.result, expandedLogIds, entry.id and entry.decision) with a concise helper
or lookup map (e.g., LOG_DECISION_DEFAULTS) that maps decision keys to default
messages and a small function (e.g., formatLogResult(entry, expandedLogIds))
that returns either the full/trimmed entry.result or the lookup value (or
fallback to entry.decision); update the JSX span to call that helper and remove
the 8-level ternary so the logic is readable and maintainable.
In `@docs/memory-sync-functions.md`:
- Around line 208-217: Update the example to explicitly label the tokio::spawn +
memory_client.clone() + client.put_doc(input).await pattern as "best-effort
only" and advise preferring awaiting put_doc() in normal code paths; mention
that detached tasks can be dropped during shutdown and recommend alternate
approaches (await the future, propagate the Result, or use a managed background
worker with shutdown/join) and reserve tokio::spawn detachment only for
non-critical telemetry/event writes.
In `@docs/subconscious.md`:
- Line 63: The inline code span contains a trailing space (`- `) which triggers
the markdown linter; edit the sentence in subconscious.md to remove the trailing
space inside the backticks (change "`- ` line" to "`-` line" or rephrase to use
a non-code span format), ensuring the code span content is exactly "-" with no
trailing whitespace.
In `@src/openhuman/heartbeat/engine.rs`:
- Around line 42-45: Sleep-first behavior delays the initial heartbeat; call the
same tick/processing path once immediately before entering the loop instead of
waiting on time::sleep. Locate the loop using sleep_secs (variables
interval_mins, sleep_secs, time::sleep(Duration::from_secs(sleep_secs)).await)
in engine.rs and invoke the existing tick handler or seed/evaluate routine once
(the same code executed inside the loop) right before the loop; keep the
periodic sleep loop unchanged so subsequent ticks still occur every interval.
In `@src/openhuman/subconscious/store.rs`:
- Around line 146-166: The update_task function performs separate UPDATE
statements per field which can lead to partial commits; modify update_task (and
use Connection/TaskPatch symbols) to perform the update atomically by starting a
transaction (conn.transaction()) or by building a single dynamic UPDATE SQL that
only sets provided fields (title, recurrence via recurrence_to_string, enabled)
and executes once with the assembled params against the subconscious_tasks row
for id; ensure the transaction is committed or rolled back on error so no
partial writes occur.
- Around line 95-99: The current use of
serde_json::to_value(...).as_str().unwrap_or("user") for source_str (and
analogous uses for EscalationPriority and EscalationStatus) can silently default
if the serde representation changes; replace these with explicit helper
functions that match each enum variant (e.g., implement
source_to_string(&TaskSource) -> &'static str,
priority_to_string(&EscalationPriority) -> &'static str,
status_to_string(&EscalationStatus) -> &'static str) and call those helpers to
produce the String values instead of relying on serde_json::to_value; update the
places using source_str and the priority/status variables to use the new helpers
so serialization is deterministic and fails loudly if enums change.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 926d6c65-9200-41e0-8c2a-9efe2c6314dc
⛔ Files ignored due to path filters (1)
app/src-tauri/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
app/src/hooks/useSubconscious.tsapp/src/pages/Intelligence.tsxapp/src/utils/tauriCommands/index.tsapp/src/utils/tauriCommands/subconscious.tsdocs/memory-sync-functions.mddocs/subconscious.mdsrc/openhuman/config/schema/heartbeat_cron.rssrc/openhuman/heartbeat/engine.rssrc/openhuman/subconscious/decision_log.rssrc/openhuman/subconscious/engine.rssrc/openhuman/subconscious/executor.rssrc/openhuman/subconscious/integration_test.rssrc/openhuman/subconscious/mod.rssrc/openhuman/subconscious/prompt.rssrc/openhuman/subconscious/schemas.rssrc/openhuman/subconscious/store.rssrc/openhuman/subconscious/types.rstests/subconscious_e2e.rs
Contributor
|
Caution Review failedAn error occurred during the review process. Please try again later. 📝 WalkthroughWalkthroughThis PR introduces a comprehensive redesign of the subconscious loop system, migrating from a decision-log model to a SQLite-backed task pipeline. It adds persistent task scheduling with recurrence support, escalation handling, execution tracking, and a UI layer for task management. The refactor includes a new executor module for task dispatch, prompt builders for distinct execution modes, and frontend components for status monitoring and user interaction. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Intelligence Page
participant Hook as useSubconscious Hook
participant Tauri as Tauri Commands
participant Engine as SubconsciousEngine
participant Store as SQLite Store
participant Model as Local Ollama Model
participant Agent as Agentic API
UI->>Hook: Mount / Poll (3s interval)
Hook->>Tauri: Fetch tasks, escalations, logs, status
Tauri->>Store: Query tables
Store-->>Tauri: Rows
Tauri-->>Hook: Data
Hook-->>UI: Render state
UI->>Hook: triggerTick()
Hook->>Tauri: subconsciousTrigger()
Tauri->>Engine: tick()
Engine->>Store: Load due_tasks()
Store-->>Engine: Tasks
Engine->>Store: Insert in_progress log entries
Store-->>Engine: Log IDs
Engine->>Model: Evaluate all tasks (batch prompt)
Model-->>Engine: TaskEvaluation[]
par Task Execution
Engine->>Engine: Route by task intent (needs_tools, needs_agent)
alt Tools Required
Engine->>Agent: execute_with_agent_full()
Agent-->>Engine: ExecutionResult
else Read-Only Analysis
Engine->>Model: execute_with_agent_analysis()
Model-->>Engine: Result + optional recommendation
alt Recommendation Detected
Engine->>Store: Create escalation (unapproved_write)
end
else Text-Only
Engine->>Model: execute_with_local_model()
Model-->>Engine: ExecutionResult
end
end
par Persistence
Engine->>Store: Update log entries (decision, result, duration)
Engine->>Store: Update task run_times
Engine->>Store: Create escalations
end
Engine-->>Tauri: TickResult { evaluations, executed, escalated }
Tauri-->>Hook: Async confirmation
UI->>Hook: approveEscalation(id)
Hook->>Tauri: escalationsApprove()
Tauri->>Engine: approve_escalation()
Engine->>Agent: execute_approved_write()
Agent-->>Engine: ExecutionResult
Engine->>Store: Update escalation status, log execution
Store-->>Engine: Done
Engine-->>Tauri: Confirmation
Tauri-->>Hook: Done
Hook->>Tauri: Refresh data
Tauri->>Store: Query updated state
Store-->>Hook: New data
Hook-->>UI: Re-render
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Member
|
bro @sanil-23 one test is taking ages.. see actions |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
app/src/pages/Intelligence.tsx (1)
33-35: Use an arrow declaration for the new helper.
isSkillRelatedis introduced as a function declaration; this part of the app is standardized on const-arrow helpers.As per coding guidelines: "Prefer arrow functions over function declarations."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/pages/Intelligence.tsx` around lines 33 - 35, isSkillRelated is implemented as a function declaration but the codebase standard uses const arrow helpers; replace the declaration with a const arrow function (e.g., const isSkillRelated = (title: string, description: string): boolean => ...) keeping the same logic and references to SKILL_KEYWORDS so callers of isSkillRelated remain unchanged.app/src/hooks/useSubconscious.ts (1)
53-53: Use arrow declarations for the new hook utilities.
useSubconsciousandunwrapare introduced as function declarations, butapp/srcis standardized on const-arrow declarations.As per coding guidelines: "Prefer arrow functions over function declarations."
Also applies to: 197-205
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/hooks/useSubconscious.ts` at line 53, Convert the function declarations useSubconscious and unwrap into const arrow-function declarations to match project style; locate the function declaration named useSubconscious and the helper function unwrap (also referenced around the block covering the other occurrence) and change their signatures from "function useSubconscious(...)" / "function unwrap(...)" to "const useSubconscious = (...) => { ... }" and "const unwrap = (...) => { ... }", preserving exports, types (UseSubconsciousResult), and internal logic/closures exactly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/hooks/useSubconscious.ts`:
- Around line 76-79: The current use of unwrap() in useSubconscious.ts (calls
around setTasks, setEscalations, setLogEntries, setStatus and the similar block
at lines ~194-205) assumes RPC responses are { result: ... } and thus can clear
state when the core RPC returns bare values or keyed payloads; update the
normalization so unwrap() (or a small helper, e.g.,
normalizeSubconsciousResponse) accepts and returns either res.result, or keyed
payloads like res.tasks / res.entries / res.escalations, or the raw value
itself, and then pass that normalized value into setTasks, setEscalations,
setLogEntries, and setStatus instead of blindly calling unwrap; ensure the
helper is used in both places mentioned so keyed and bare RPC responses populate
state correctly.
In `@app/src/pages/Intelligence.tsx`:
- Around line 59-72: The subconscious poller is mounted regardless of which tab
is active because useSubconscious() is called unconditionally in
Intelligence.tsx; change the call to pass an enabled flag (e.g., const
subconscious = useSubconscious(activeTab === 'subconscious') or pass activeTab
=== 'subconscious' into useSubconscious) and update the hook implementation in
useSubconscious to accept an enabled parameter and early-return from its
useEffect when enabled is false, so the immediate refresh and the 3s interval
are only started when enabled is true.
- Around line 676-706: The toggle and delete buttons lack accessible names/state
and the delete button is hidden from keyboard users; update the toggle button
(calling toggleSubconsciousTask with task.id) to include
aria-pressed={task.enabled} and an aria-label like `Toggle task: ${task.title}`
(and add type="button"), and update the delete button (calling
removeSubconsciousTask) to include an explicit aria-label such as `Delete task:
${task.title}`, type="button", and a title for sighted users; make the delete
control focusable by changing its visibility rules from only hover (e.g., remove
"opacity-0 group-hover:opacity-100") to also show on keyboard focus (add classes
like "opacity-0 group-hover:opacity-100 focus:opacity-100
focus-visible:opacity-100 group-focus-within:opacity-100" or equivalent) so
keyboard/screen-reader users can see and activate it.
- Around line 622-624: approveEscalation(esc.id) can rethrow and currently
causes an unhandled rejection from the button onClick; wrap the call in an async
handler that awaits approveEscalation and catches errors, then handle the
failure by logging and surfacing a user-facing message (e.g., call your
toast/error handler or set an error state) so the UI shows the failure instead
of leaving an unhandled promise; update the button's onClick to call this
wrapper rather than calling approveEscalation directly.
---
Nitpick comments:
In `@app/src/hooks/useSubconscious.ts`:
- Line 53: Convert the function declarations useSubconscious and unwrap into
const arrow-function declarations to match project style; locate the function
declaration named useSubconscious and the helper function unwrap (also
referenced around the block covering the other occurrence) and change their
signatures from "function useSubconscious(...)" / "function unwrap(...)" to
"const useSubconscious = (...) => { ... }" and "const unwrap = (...) => { ...
}", preserving exports, types (UseSubconsciousResult), and internal
logic/closures exactly.
In `@app/src/pages/Intelligence.tsx`:
- Around line 33-35: isSkillRelated is implemented as a function declaration but
the codebase standard uses const arrow helpers; replace the declaration with a
const arrow function (e.g., const isSkillRelated = (title: string, description:
string): boolean => ...) keeping the same logic and references to SKILL_KEYWORDS
so callers of isSkillRelated remain unchanged.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: b71c8d43-680d-4d65-b027-82b660b2d099
📒 Files selected for processing (3)
app/src/hooks/useSubconscious.tsapp/src/pages/Intelligence.tsxapp/src/utils/tauriCommands/subconscious.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/utils/tauriCommands/subconscious.ts
Comment on lines
+76
to
+79
| if (tasksRes) setTasks(unwrap(tasksRes) ?? []); | ||
| if (escalationsRes) setEscalations(unwrap(escalationsRes) ?? []); | ||
| if (logRes) setLogEntries(unwrap(logRes) ?? []); | ||
| if (statusRes) setStatus(unwrap(statusRes) ?? null); |
Contributor
There was a problem hiding this comment.
Broaden unwrap() to the actual RPC response shapes.
unwrap() only accepts { result: ... }, but the core RPC serializer can return bare values when no logs are emitted, and the subconscious list endpoints use keyed payloads (tasks, entries, escalations). A successful refresh can therefore clear state instead of populating it.
💡 One way to normalize wrapped and raw responses
- if (tasksRes) setTasks(unwrap(tasksRes) ?? []);
- if (escalationsRes) setEscalations(unwrap(escalationsRes) ?? []);
- if (logRes) setLogEntries(unwrap(logRes) ?? []);
+ if (tasksRes) setTasks(unwrap(tasksRes, 'tasks') ?? []);
+ if (escalationsRes) setEscalations(unwrap(escalationsRes, 'escalations') ?? []);
+ if (logRes) setLogEntries(unwrap(logRes, 'entries') ?? []);
if (statusRes) setStatus(unwrap(statusRes) ?? null);
...
-function unwrap<T>(response: unknown): T | null {
- if (!response || typeof response !== 'object') return null;
- const r = response as Record<string, unknown>;
- // CommandResponse shape: { result: T, logs: string[] }
- if ('result' in r) {
- return r.result as T;
- }
- return null;
+function unwrap<T>(response: unknown, key?: string): T | null {
+ if (response == null) return null;
+ if (typeof response !== 'object') return response as T;
+ const record = response as Record<string, unknown>;
+ if ('result' in record) return record.result as T;
+ if (key && key in record) return record[key] as T;
+ return response as T;
}Also applies to: 194-205
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/hooks/useSubconscious.ts` around lines 76 - 79, The current use of
unwrap() in useSubconscious.ts (calls around setTasks, setEscalations,
setLogEntries, setStatus and the similar block at lines ~194-205) assumes RPC
responses are { result: ... } and thus can clear state when the core RPC returns
bare values or keyed payloads; update the normalization so unwrap() (or a small
helper, e.g., normalizeSubconsciousResponse) accepts and returns either
res.result, or keyed payloads like res.tasks / res.entries / res.escalations, or
the raw value itself, and then pass that normalized value into setTasks,
setEscalations, setLogEntries, and setStatus instead of blindly calling unwrap;
ensure the helper is used in both places mentioned so keyed and bare RPC
responses populate state correctly.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/openhuman/subconscious/engine.rs`:
- Around line 215-233: The current one-time generation check (comparing
self.tick_generation to my_generation) lets a tick be superseded after Line 216
but still perform side effects; fix by re-checking the generation immediately
before every side-effecting operation (e.g., before calling execute_task(),
before any store::with_connection(...) calls that cancel/update rows, and before
mutating state in the block that advances last_tick_at or updates logs)
or—preferably—serialize tick execution with a short-lived async guard so a
superseded tick cannot run side effects: implement either repeated checks that
bail the tick the same way the initial branch does (cancel_stale_in_progress,
return TickResult without advancing last_tick_at) or acquire a per-tick
execution lock at the start of run_tick()/the tick handler so only the newest
generation proceeds to execute_task(), DB updates, and state.lock() mutations
(apply the same fix in the other similar regions noted around the
execute_task/store::with_connection/state.lock() patterns).
- Around line 48-87: The file fails cargo fmt --check; run cargo fmt to fix
formatting and then cargo check to ensure there are no compilation issues before
merging; specifically reformat the functions new and from_heartbeat_config and
surrounding struct/initializer usage (e.g., Engine::new,
Engine::from_heartbeat_config, EngineState initialization, and tick_generation
AtomicU64) so the Rust formatter's expectations are met, then run cargo check in
the crate(s) containing these symbols (and update any minor syntax/formatting
issues found) and commit the formatted changes.
- Around line 667-689: The parser currently accepts any non-empty
Vec<TaskEvaluation>, which can include duplicates or evaluations for wrong
tasks; update the handling after deserializing EvaluationResponse or
Vec<TaskEvaluation> to: compute the set of due task ids from the incoming tasks
iterator, filter parsed evaluations to only those whose task_id is in that
due-task set, deduplicate by task_id (e.g., keep first occurrence), and only
return the filtered/deduped list if it contains exactly one evaluation per due
task; otherwise treat the response as malformed and fall back to constructing
the noop TaskEvaluation list (the existing fallback using TaskEvaluation and
TickDecision::Noop), ensuring this validation is applied for both the
EvaluationResponse.evaluations and the bare Vec<TaskEvaluation] branches so
tick() cannot receive duplicated or irrelevant task_ids.
- Around line 348-391: The current approve_escalation flow fetches the
escalation and then executes the task before atomically claiming the escalation,
allowing duplicate approvals; change approve_escalation so it first performs an
atomic DB claim (e.g., add/use a store function like store::claim_escalation or
extend store::resolve_escalation to do WHERE status = 'Pending' and return
number of rows affected) inside store::with_connection, verify exactly one row
was updated (rows_affected == 1) and only then proceed to build the report and
call executor::execute_approved_write; if the claim fails (0 rows affected)
abort early (return Ok(()) or an appropriate error) so the
execution/logging/mark-complete steps (store::add_log_entry,
store::resolve_escalation/mark_task_completed/advance_task_schedule_in_conn)
only run for the single successful claim. Ensure the claim/update and subsequent
final status change happen in the correct places (claim before execution; final
resolve/mark completed after execution) and reference approve_escalation,
store::with_connection, executor::execute_approved_write, store::add_log_entry,
and advance_task_schedule_in_conn when making the changes.
- Around line 253-266: Escalated tasks are never removed from the due queue
because handle_escalate creates an escalation row but does not update scheduling
or check for existing escalations; update handle_escalate (and any other
escalation path used in the TickDecision match) to either call
advance_task_schedule(task, tick_at) after creating the escalation or to first
query/suppress if an open escalation already exists for the given task_id before
creating a new one so duplicate escalations/logs are not emitted on every tick;
ensure the same logic is applied in the other escalation handling sites
referenced (around lines 525-565 and 582-618) so Pending/cron tasks have
next_run_at advanced or suppressed when an escalation is active.
In `@src/openhuman/subconscious/situation_report.rs`:
- Around line 77-91: build_tasks_section currently uses the full tasks vector
returned by super::store::list_tasks and renders completed/disabled items;
change it to filter that list to only active tasks (e.g., let active_tasks:
Vec<_> = tasks.into_iter().filter(|t| t.enabled && !t.completed).collect()),
then use active_tasks for the empty check and the loop that writes each "- {}"
line; update the early-return messages to reflect "No active pending tasks" if
needed. Ensure you reference build_tasks_section, super::store::list_tasks, and
the task fields enabled and completed when making the change.
In `@src/openhuman/subconscious/store.rs`:
- Around line 168-183: remove_task currently deletes a user task without
checking for related escalation rows, which breaks approve_escalation (it
reloads by esc.task_id); update remove_task to first query the escalations table
(the table read by approve_escalation for esc.task_id) for any non-final/pending
escalations referencing the task_id and return an error if any exist, or
alternatively enforce DB-level referential integrity (foreign key ON DELETE
RESTRICT) so deletes are prevented; reference the remove_task function and
approve_escalation/esc.task_id when implementing the check or adding the
constraint.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: b5b104fd-c6b4-4496-add2-db0cd4c8d7a8
📒 Files selected for processing (5)
src/openhuman/heartbeat/engine.rssrc/openhuman/subconscious/engine.rssrc/openhuman/subconscious/integration_test.rssrc/openhuman/subconscious/situation_report.rssrc/openhuman/subconscious/store.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/openhuman/heartbeat/engine.rs
Comment on lines
+348
to
+391
| pub async fn approve_escalation(&self, escalation_id: &str) -> Result<()> { | ||
| let (escalation, task) = store::with_connection(&self.workspace_dir, |conn| { | ||
| let esc = store::get_escalation(conn, escalation_id)?; | ||
| let task = store::get_task(conn, &esc.task_id)?; | ||
| Ok((esc, task)) | ||
| })?; | ||
|
|
||
| info!( | ||
| "[subconscious] approved escalation '{}' for task '{}'", | ||
| escalation.title, task.title | ||
| ); | ||
|
|
||
| // Execute the task | ||
| let identity = prompt::load_identity_context(&self.workspace_dir); | ||
| let memory_ref = self.memory.as_ref().map(|m| m.as_ref()); | ||
| let report = build_situation_report( | ||
| memory_ref, | ||
| &self.workspace_dir, | ||
| 0.0, // fresh report for execution | ||
| self.context_budget_tokens, | ||
| ) | ||
| .await; | ||
|
|
||
| let tick_at = now_secs(); | ||
| let result = executor::execute_approved_write(&task, &report, &identity).await; | ||
| let (result_text, duration) = match &result { | ||
| Ok(r) => (r.output.clone(), Some(r.duration_ms as i64)), | ||
| Err(e) => (format!("Execution failed: {e}"), None), | ||
| }; | ||
|
|
||
| store::with_connection(&self.workspace_dir, |conn| { | ||
| store::add_log_entry(conn, &task.id, tick_at, "act", Some(&result_text), duration)?; | ||
| store::resolve_escalation( | ||
| conn, | ||
| escalation_id, | ||
| &super::types::EscalationStatus::Approved, | ||
| )?; | ||
| if task.recurrence == TaskRecurrence::Once { | ||
| store::mark_task_completed(conn, &task.id)?; | ||
| } else { | ||
| self.advance_task_schedule_in_conn(conn, &task, tick_at); | ||
| } | ||
| Ok(()) | ||
| })?; |
Contributor
There was a problem hiding this comment.
Make approval a pending-only, single-use transition.
approve_escalation() executes the write before it ever claims or validates the escalation state. A retry, double-click, or stale tab can approve the same escalation again after it is already approved or dismissed, and the task will execute twice. Gate this behind an atomic pending -> approved claim and only proceed when exactly one row was claimed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/openhuman/subconscious/engine.rs` around lines 348 - 391, The current
approve_escalation flow fetches the escalation and then executes the task before
atomically claiming the escalation, allowing duplicate approvals; change
approve_escalation so it first performs an atomic DB claim (e.g., add/use a
store function like store::claim_escalation or extend store::resolve_escalation
to do WHERE status = 'Pending' and return number of rows affected) inside
store::with_connection, verify exactly one row was updated (rows_affected == 1)
and only then proceed to build the report and call
executor::execute_approved_write; if the claim fails (0 rows affected) abort
early (return Ok(()) or an appropriate error) so the
execution/logging/mark-complete steps (store::add_log_entry,
store::resolve_escalation/mark_task_completed/advance_task_schedule_in_conn)
only run for the single successful claim. Ensure the claim/update and subsequent
final status change happen in the correct places (claim before execution; final
resolve/mark completed after execution) and reference approve_escalation,
store::with_connection, executor::execute_approved_write, store::add_log_entry,
and advance_task_schedule_in_conn when making the changes.
Comment on lines
+667
to
+689
| // Try parsing as EvaluationResponse | ||
| if let Ok(response) = serde_json::from_str::<EvaluationResponse>(json_text) { | ||
| if !response.evaluations.is_empty() { | ||
| return response.evaluations; | ||
| } | ||
| } | ||
|
|
||
| // Fallback: wrap the raw response as a single notify action | ||
| vec![super::types::RecommendedAction { | ||
| action_type: super::types::ActionType::EscalateToAgent, | ||
| description: format!( | ||
| "Escalation resolved: {}", | ||
| &agent_response[..agent_response.len().min(300)] | ||
| ), | ||
| priority: super::types::Priority::High, | ||
| task: Some(reason.to_string()), | ||
| }] | ||
| } | ||
|
|
||
| /// Generate a short random suffix for KV key uniqueness. | ||
| fn rand_suffix() -> u32 { | ||
| use std::collections::hash_map::DefaultHasher; | ||
| use std::hash::{Hash, Hasher}; | ||
| let mut hasher = DefaultHasher::new(); | ||
| std::time::SystemTime::now().hash(&mut hasher); | ||
| std::thread::current().id().hash(&mut hasher); | ||
| (hasher.finish() % 10000) as u32 | ||
| } | ||
|
|
||
| /// Wrapper around build_situation_report that also returns new doc IDs. | ||
| async fn build_situation_report_with_doc_ids( | ||
| memory: Option<&super::super::memory::MemoryClient>, | ||
| workspace_dir: &std::path::Path, | ||
| last_tick_at: f64, | ||
| token_budget: u32, | ||
| ) -> (String, Vec<String>) { | ||
| let report = build_situation_report(memory, workspace_dir, last_tick_at, token_budget).await; | ||
|
|
||
| // Extract doc IDs from memory if available | ||
| let mut doc_ids = Vec::new(); | ||
| if let Some(client) = memory { | ||
| if let Ok(docs) = client.list_documents(None).await { | ||
| let is_cold_start = last_tick_at <= 0.0; | ||
| if let Some(arr) = docs | ||
| .as_array() | ||
| .or_else(|| docs.get("documents").and_then(|v| v.as_array())) | ||
| { | ||
| for doc in arr { | ||
| let updated_at = doc | ||
| .get("updated_at") | ||
| .or_else(|| doc.get("updatedAt")) | ||
| .and_then(|v| v.as_f64()) | ||
| .unwrap_or(0.0); | ||
| if is_cold_start || updated_at > last_tick_at { | ||
| if let Some(id) = doc | ||
| .get("document_id") | ||
| .or_else(|| doc.get("documentId")) | ||
| .and_then(|v| v.as_str()) | ||
| { | ||
| doc_ids.push(id.to_string()); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| // Try parsing as a bare array of evaluations | ||
| if let Ok(evals) = serde_json::from_str::<Vec<TaskEvaluation>>(json_text) { | ||
| if !evals.is_empty() { | ||
| return evals; | ||
| } | ||
| } | ||
|
|
||
| (report, doc_ids) | ||
| warn!("[subconscious] could not parse evaluation response, defaulting all to noop"); | ||
| tasks | ||
| .iter() | ||
| .map(|t| TaskEvaluation { | ||
| task_id: t.id.clone(), | ||
| decision: TickDecision::Noop, | ||
| reason: "Unparseable evaluation response".to_string(), | ||
| }) | ||
| .collect() |
Contributor
There was a problem hiding this comment.
Validate one evaluation per due task before returning.
This accepts any non-empty list from the model. If the response repeats the same task_id, tick() iterates that vector verbatim and can execute/escalate the same task multiple times in one tick. Filter to the due-task id set and dedupe by task_id here, or treat duplicates as malformed and fall back.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/openhuman/subconscious/engine.rs` around lines 667 - 689, The parser
currently accepts any non-empty Vec<TaskEvaluation>, which can include
duplicates or evaluations for wrong tasks; update the handling after
deserializing EvaluationResponse or Vec<TaskEvaluation> to: compute the set of
due task ids from the incoming tasks iterator, filter parsed evaluations to only
those whose task_id is in that due-task set, deduplicate by task_id (e.g., keep
first occurrence), and only return the filtered/deduped list if it contains
exactly one evaluation per due task; otherwise treat the response as malformed
and fall back to constructing the noop TaskEvaluation list (the existing
fallback using TaskEvaluation and TickDecision::Noop), ensuring this validation
is applied for both the EvaluationResponse.evaluations and the bare
Vec<TaskEvaluation] branches so tick() cannot receive duplicated or irrelevant
task_ids.
Comment on lines
+77
to
+91
| fn build_tasks_section(workspace_dir: &Path) -> String { | ||
| let tasks = match super::store::with_connection(workspace_dir, |conn| { | ||
| super::store::list_tasks(conn, false) | ||
| }) { | ||
| Ok(tasks) => tasks, | ||
| Err(_) => return "## Pending Tasks\n\nFailed to read tasks.\n".to_string(), | ||
| }; | ||
|
|
||
| let tasks: Vec<&str> = content | ||
| .lines() | ||
| .filter_map(|line| line.trim().strip_prefix("- ")) | ||
| .collect(); | ||
|
|
||
| if tasks.is_empty() { | ||
| return "## Pending Tasks\n\nNo tasks defined.\n".to_string(); | ||
| } | ||
|
|
||
| let mut section = String::from("## Pending Tasks\n\n"); | ||
| for task in &tasks { | ||
| let _ = writeln!(section, "- {task}"); | ||
| let _ = writeln!(section, "- {}", task.title); |
Contributor
There was a problem hiding this comment.
Filter inactive rows out of the “Pending Tasks” section.
This now pulls list_tasks(conn, false), so completed/disabled tasks are rendered ahead of memory and skills in a capped report. On longer-lived workspaces that burns budget on historical items and can crowd out the fresh context the evaluator actually needs. Please limit this section to active tasks (enabled && !completed) or the current due set.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/openhuman/subconscious/situation_report.rs` around lines 77 - 91,
build_tasks_section currently uses the full tasks vector returned by
super::store::list_tasks and renders completed/disabled items; change it to
filter that list to only active tasks (e.g., let active_tasks: Vec<_> =
tasks.into_iter().filter(|t| t.enabled && !t.completed).collect()), then use
active_tasks for the empty check and the loop that writes each "- {}" line;
update the early-return messages to reflect "No active pending tasks" if needed.
Ensure you reference build_tasks_section, super::store::list_tasks, and the task
fields enabled and completed when making the change.
Comment on lines
+168
to
+183
| /// Remove a task. System tasks cannot be deleted — only disabled. | ||
| pub fn remove_task(conn: &Connection, task_id: &str) -> Result<()> { | ||
| let source: String = conn | ||
| .query_row( | ||
| "SELECT source FROM subconscious_tasks WHERE id = ?1", | ||
| [task_id], | ||
| |row| row.get(0), | ||
| ) | ||
| .with_context(|| format!("task not found: {task_id}"))?; | ||
|
|
||
| if source == "system" { | ||
| anyhow::bail!("System tasks cannot be deleted. Disable them instead."); | ||
| } | ||
|
|
||
| conn.execute("DELETE FROM subconscious_tasks WHERE id = ?1", [task_id])?; | ||
| Ok(()) |
Contributor
There was a problem hiding this comment.
Block deletes while a task still has unresolved escalations.
remove_task() only checks source, so a user task can be deleted while its escalation row remains in SQLite. approve_escalation() later reloads the task by esc.task_id, which means that approval flow can no longer execute cleanly once the task row is gone. Reject the delete while pending escalations exist, or enforce it with a DB constraint.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/openhuman/subconscious/store.rs` around lines 168 - 183, remove_task
currently deletes a user task without checking for related escalation rows,
which breaks approve_escalation (it reloads by esc.task_id); update remove_task
to first query the escalations table (the table read by approve_escalation for
esc.task_id) for any non-final/pending escalations referencing the task_id and
return an error if any exist, or alternatively enforce DB-level referential
integrity (foreign key ON DELETE RESTRICT) so deletes are prevented; reference
the remove_task function and approve_escalation/esc.task_id when implementing
the check or adding the constraint.
…nsai#392) - Enable heartbeat by default (enabled=true, inference_enabled=true, 5min interval) - Seed system tasks on engine init, not first tick - SQLite-backed task/log/escalation persistence - Overlap guard with generation counter — stale ticks are cancelled - Single log entry per task per tick, updated in place (in_progress → act/noop/escalate/failed/cancelled) - Rate-limit retry (429 only) for agentic-v1 cloud model calls - Approval gate: unsolicited write actions on read-only tasks require user approval - Analysis-only mode for agentic-v1 on read-only escalations - Non-blocking status RPC — reads from DB, never blocks on engine mutex - Frontend: system vs user task distinction, toggle switches, expandable activity log - Frontend: 3s auto-poll on Subconscious tab, skill-related escalation navigation - Consecutive failure counter in status (resets on success) - last_tick_at only advances on successful evaluation - Missing LLM evaluation fallback — unevaluated tasks default to noop - Docs: subconscious.md architecture guide, memory-sync-functions.md reference Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…isabled test HeartbeatConfig::default() has enabled: true, so run() entered the infinite loop and never returned — hanging the test (and CI) indefinitely. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…e task source Tasks are now managed exclusively in SQLite via the Subconscious UI. HEARTBEAT.md is retained for instructions/context only, not as a task list. Situation report now reads pending tasks from SQLite instead of HEARTBEAT.md. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
sanil-23
force-pushed
the
feat/392-subconscious-loop
branch
from
4afa33d to
3762203
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (9)
app/src/utils/tauriCommands/subconscious.ts (1)
70-75:⚠️ Potential issue | 🟡 MinorAlign the trigger wrapper with the new RPC payload.
openhuman.subconscious_triggernow returns{ triggered: true }, but this wrapper still advertisesTickResult. It's harmless while callers ignore the value, but the first consumer that readsevaluationsorduration_mswill getundefined.💡 Minimal fix
+export interface SubconsciousTriggerResult { + triggered: boolean; +} + -export async function subconsciousTrigger(): Promise<CommandResponse<TickResult>> { +export async function subconsciousTrigger(): Promise<CommandResponse<SubconsciousTriggerResult>> { if (!isTauri()) throw new Error('Not running in Tauri'); - return await callCoreRpc<CommandResponse<TickResult>>({ + return await callCoreRpc<CommandResponse<SubconsciousTriggerResult>>({ method: 'openhuman.subconscious_trigger', }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/utils/tauriCommands/subconscious.ts` around lines 70 - 75, The wrapper subconsciousTrigger advertises CommandResponse<TickResult> but the RPC now returns { triggered: true }; update the function signature and call to reflect the new payload by replacing TickResult with a small interface/type representing { triggered: boolean } (or use inline { triggered: boolean }) in the generic for callCoreRpc and the function Promise return type, and remove any references to evaluations/duration_ms in callers or update them to handle the new shape; ensure the function name subconsciousTrigger and the callCoreRpc invocation remain intact so locating and changing the generic is straightforward.src/openhuman/subconscious/store.rs (1)
168-183:⚠️ Potential issue | 🟠 MajorDon't allow deletes while a task still has a pending escalation.
That leaves
subconscious_escalations.task_idorphaned, andapprove_escalation()later fails when it reloads the task row. Reject the delete while pending escalations exist, or enforce the invariant with a foreign key /ON DELETE RESTRICT.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openhuman/subconscious/store.rs` around lines 168 - 183, The remove_task function currently deletes a task unconditionally causing orphaned rows in subconscious_escalations and later failures in approve_escalation; update remove_task to first check for any pending escalations referencing the task (query subconscious_escalations WHERE task_id = ? AND status = 'pending' or NULL as appropriate) and return an error if any exist, or alternatively add/ensure a foreign key on subconscious_escalations.task_id → subconscious_tasks.id with ON DELETE RESTRICT so deletes are prevented at the DB level; reference the remove_task function, subconscious_escalations.task_id, and approve_escalation when making the change.src/openhuman/subconscious/situation_report.rs (1)
78-91:⚠️ Potential issue | 🟠 MajorOnly include active tasks in this prompt section.
list_tasks(conn, false)still feeds completed/disabled rows into a capped report, so stale tasks can consume context budget and crowd out work that is actually pending. Filter locally toenabled && !completedbefore the empty check and render loop.💡 Minimal fix
fn build_tasks_section(workspace_dir: &Path) -> String { let tasks = match super::store::with_connection(workspace_dir, |conn| { super::store::list_tasks(conn, false) }) { Ok(tasks) => tasks, Err(_) => return "## Pending Tasks\n\nFailed to read tasks.\n".to_string(), }; + let active_tasks: Vec<_> = tasks + .into_iter() + .filter(|task| task.enabled && !task.completed) + .collect(); - if tasks.is_empty() { - return "## Pending Tasks\n\nNo tasks defined.\n".to_string(); + if active_tasks.is_empty() { + return "## Pending Tasks\n\nNo active pending tasks.\n".to_string(); } let mut section = String::from("## Pending Tasks\n\n"); - for task in &tasks { + for task in &active_tasks { let _ = writeln!(section, "- {}", task.title); } section }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openhuman/subconscious/situation_report.rs` around lines 78 - 91, The Pending Tasks block currently uses super::store::list_tasks(conn, false) and then displays all returned rows; update the logic in situation_report.rs to filter the retrieved tasks collection to only those with enabled == true and completed == false (e.g., tasks.retain(|t| t.enabled && !t.completed) or an iterator filter) before performing the is_empty() check and the for loop that writes lines; keep the rest of the flow (error handling from with_connection, variable names tasks and section, and writeln! loop) intact so only active, not completed/disabled, tasks are counted and rendered.src/openhuman/subconscious/engine.rs (5)
664-675:⚠️ Potential issue | 🔴 CriticalDeduplicate parsed evaluations by
task_idbefore returning them.Any non-empty array is accepted here. If the model repeats a due
task_id, the loop below will execute or escalate the same task multiple times in one tick. Filtering to known due-task IDs and keeping only the first occurrence preserves the later omitted-task fallback without allowing duplicate side effects.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openhuman/subconscious/engine.rs` around lines 664 - 675, When parsing JSON into EvaluationResponse or Vec<TaskEvaluation> in engine.rs (the block using serde_json::from_str::<EvaluationResponse>(json_text) and from_str::<Vec<TaskEvaluation>>(json_text)), deduplicate the resulting evaluations by TaskEvaluation.task_id and filter to only known due-task IDs before returning; for both response.evaluations and evals produce a new Vec<TaskEvaluation> that keeps the first occurrence of each task_id (preserving original order) and excludes any task_ids not present in the current due-task set, then return that filtered/deduped vector instead of the raw evaluations.
522-563:⚠️ Potential issue | 🟠 MajorEscalation paths should suppress duplicates while one is already pending.
Both branches unconditionally create a new escalation row for the task. Without checking for an existing pending escalation,
pendingand cron tasks can emit identical approval cards on every tick until the user responds.Also applies to: 591-615
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openhuman/subconscious/engine.rs` around lines 522 - 563, The UnapprovedWrite handling currently always calls store::add_escalation (and creates/upserts a log via store::add_log_entry / store::update_log_entry), causing duplicate pending escalations; before adding a new escalation in the ExecutionOutcome::UnapprovedWrite branch, query whether a pending escalation already exists for the task (e.g., via a store method like has_pending_escalation or a new DB lookup) and if one exists, avoid calling store::add_escalation — instead update the existing escalation and its associated log (use update_log_entry when log_id is present or update the existing escalation's log reference) so only one pending approval row remains; apply the same check/update logic to the identical code path around the EscalationPriority::Important handling at the other block (lines ~591-615).
347-390:⚠️ Potential issue | 🔴 CriticalClaim the escalation before executing it.
approve_escalation()fetches the row, performs the side effect, and only then marks it approved. A retry, double-click, or stale tab can therefore execute the same escalation twice. Use an atomicpending -> approvedclaim (orpending -> in_progress) and only continue when exactly one row was claimed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openhuman/subconscious/engine.rs` around lines 347 - 390, approve_escalation currently reads the escalation and executes its task before marking it approved, allowing duplicate executions on retries; change the flow so you atomically claim the escalation in the DB first (e.g. implement/use a store method like store::claim_escalation(conn, escalation_id, from_status, to_status) or perform an UPDATE ... WHERE status = 'pending' and check affected rows) inside the initial store::with_connection call, returning an error if no row was claimed, and only after a successful single-row claim proceed to build the report, call executor::execute_approved_write, and finally call store::resolve_escalation/store::mark_task_completed within the same or a new connection as appropriate; reference functions: approve_escalation, store::with_connection, store::resolve_escalation, and executor::execute_approved_write to locate where to insert the atomic claim and the subsequent guarded execution.
132-136:⚠️ Potential issue | 🟠 MajorOnly flip
state.seededafter the fallback seed succeeds.
seed_tasks()logs and swallows errors, but this branch marks the engine as seeded unconditionally. If the first fallback attempt fails, later ticks never retry and the default system tasks stay missing until restart.💡 Minimal fix
// Seed default tasks on first tick (fallback if init seeding failed) if !state.seeded { - self.seed_tasks(); - state.seeded = true; + state.seeded = self.seed_tasks(); } @@ - fn seed_tasks(&self) { + fn seed_tasks(&self) -> bool { match store::with_connection(&self.workspace_dir, |conn| store::seed_default_tasks(conn)) { Ok(count) => { if count > 0 { info!("[subconscious] seeded {count} default tasks"); } + true } - Err(e) => warn!("[subconscious] seed failed: {e}"), + Err(e) => { + warn!("[subconscious] seed failed: {e}"); + false + } } }Also applies to: 418-426
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openhuman/subconscious/engine.rs` around lines 132 - 136, The fallback seeding branch in engine.rs currently calls self.seed_tasks() and unconditionally sets state.seeded = true even if seeding failed; change seed_tasks() to return a Result (or propagate errors) and only set state.seeded = true when that Result is Ok — i.e., call self.seed_tasks(), check the return value, log/handle Err but do not flip state.seeded until success. Apply the same fix to the other similar block (the second occurrence referenced around lines 418-426) so both places only mark the engine seeded after a successful seed_tasks() result.
214-232:⚠️ Potential issue | 🔴 CriticalThe supersession check stops protecting the tick after evaluation.
Once this one-shot generation check passes, a newer tick can still supersede the current one while the loop below is executing. The stale tick can then run tools, rewrite cancelled log rows, and reschedule/complete tasks anyway. Re-check generation immediately before each side-effecting branch, or serialize the whole execution phase.
Also applies to: 245-266
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openhuman/subconscious/engine.rs` around lines 214 - 232, The current one-shot check comparing tick_generation to my_generation only protects the early return but allows a newer tick to supersede while the later side-effecting code runs; fix by re-checking tick_generation.load(Ordering::SeqCst) == my_generation immediately before every side-effecting branch (e.g., before calling store::with_connection, before running tools, before rewriting/cancelling log rows, and before rescheduling/completing tasks), and if it no longer matches, perform the same cancel-stale_in_progress logic, increment state.total_ticks and return an empty TickResult; alternatively, serialize the whole execution phase by acquiring a dedicated execution lock (or reuse state.lock()) for the duration of the side-effecting section so no newer generation can interleave (apply the same change around the later block referenced by the reviewer near the 245-266 region).docs/subconscious.md (1)
303-305:⚠️ Potential issue | 🟡 MinorUpdate this polling interval to 3s.
This still says
2s, but the current Subconscious UI polls every 3 seconds while any log entry remainsin_progress.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/subconscious.md` around lines 303 - 305, Update the Activity Log documentation text that currently says "Auto-polls every 2s while any entries are `in_progress`" to instead say "Auto-polls every 3s while any entries are `in_progress`" so it matches the actual Subconscious UI; edit the paragraph under the "Activity Log" heading accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/subconscious.md`:
- Around line 156-163: Update the retry/backoff description for agentic-v1 to
reflect the actual behavior of agent_chat_with_retry: it performs 3 total
attempts (1 initial + 2 retries) with exponential backoff delays of 2s and 4s on
429 errors, so replace "2s, 4s, 8s" with "2s, 4s" and note the 3 total attempts.
- Around line 56-64: The docs incorrectly claim the runtime imports additional
system tasks from HEARTBEAT.md; update the documentation to reflect actual
behavior by removing or replacing that sentence and stating that only
DEFAULT_SYSTEM_TASKS (seeded in DEFAULT_SYSTEM_TASKS in
src/openhuman/subconscious/store.rs) are used — do not mention imports from
src/openhuman/subconscious/engine.rs since engine.rs does not read HEARTBEAT.md;
ensure the doc references DEFAULT_SYSTEM_TASKS as the source of seeded system
tasks.
In `@src/openhuman/heartbeat/engine.rs`:
- Around line 117-122: The multiline string assigned to let default in engine.rs
contains the function's source indentation as literal leading spaces; replace it
with a dedented/raw string literal so the generated HEARTBEAT.md does not
include indentation. Locate the let default = ... assignment in engine.rs
(symbol: default) and convert the value to a raw string (or otherwise build the
lines without the surrounding code indentation, e.g., an r#"..."# placed at
column 1 or using a dedent helper) so each line starts without the ~27 leading
spaces.
In `@src/openhuman/subconscious/engine.rs`:
- Around line 252-257: The code increments executed in the Tick loop immediately
on TickDecision::Act, but handle_act() can fail or return
ExecutionOutcome::UnapprovedWrite which should not be counted as executed;
change handle_act() to return a small outcome enum (e.g., ExecutionOutcome with
variants Success, UnapprovedWrite, Error) and in the match arm for
TickDecision::Act call handle_act(...).await, inspect its returned outcome,
increment executed only for Success, and increment the appropriate
escalation/error counters (or update TickResult) for UnapprovedWrite/Error so
TickResult reflects actual completed work rather than model intent; update any
callers/structs (TickResult, types that consume executed) to account for the new
outcome.
In `@src/openhuman/subconscious/schemas.rs`:
- Around line 322-330: The current parsing for recurrence in the block that
reads params.get("recurrence") coerces any unrecognized string into
TaskRecurrence::Pending (e.g., typos like "cron:bad" or "daily"), which should
instead be rejected; update the parser so it only accepts the exact supported
shapes: "once" -> TaskRecurrence::Once and "cron:<expr>" -> TaskRecurrence::Cron
only if the cron expression validates, otherwise return a validation error from
the RPC path (do not map to TaskRecurrence::Pending). Specifically, change the
closure that produces recurrence to return a Result and perform cron syntax
validation for the expr before constructing TaskRecurrence::Cron, and
propagate/convert failures into an appropriate RPC/validation error instead of
falling back to TaskRecurrence::Pending.
In `@src/openhuman/subconscious/store.rs`:
- Around line 146-165: The update_task function currently applies patches
blindly; first SELECT the existing row for the given task_id (e.g. fetch fields
id, source, enabled, title, recurrence) inside update_task and return an error
if no row is found, then enforce invariants: if the existing task has source ==
"system" reject any patch that modifies title or recurrence (only allow changing
enabled) and return an Err in that case; finally apply updates only after those
checks (use the same rusqlite::params and recurrence_to_string for recurrence)
so callers cannot rename or reschedule seeded system tasks and invalid task_ids
produce an error.
---
Duplicate comments:
In `@app/src/utils/tauriCommands/subconscious.ts`:
- Around line 70-75: The wrapper subconsciousTrigger advertises
CommandResponse<TickResult> but the RPC now returns { triggered: true }; update
the function signature and call to reflect the new payload by replacing
TickResult with a small interface/type representing { triggered: boolean } (or
use inline { triggered: boolean }) in the generic for callCoreRpc and the
function Promise return type, and remove any references to
evaluations/duration_ms in callers or update them to handle the new shape;
ensure the function name subconsciousTrigger and the callCoreRpc invocation
remain intact so locating and changing the generic is straightforward.
In `@docs/subconscious.md`:
- Around line 303-305: Update the Activity Log documentation text that currently
says "Auto-polls every 2s while any entries are `in_progress`" to instead say
"Auto-polls every 3s while any entries are `in_progress`" so it matches the
actual Subconscious UI; edit the paragraph under the "Activity Log" heading
accordingly.
In `@src/openhuman/subconscious/engine.rs`:
- Around line 664-675: When parsing JSON into EvaluationResponse or
Vec<TaskEvaluation> in engine.rs (the block using
serde_json::from_str::<EvaluationResponse>(json_text) and
from_str::<Vec<TaskEvaluation>>(json_text)), deduplicate the resulting
evaluations by TaskEvaluation.task_id and filter to only known due-task IDs
before returning; for both response.evaluations and evals produce a new
Vec<TaskEvaluation> that keeps the first occurrence of each task_id (preserving
original order) and excludes any task_ids not present in the current due-task
set, then return that filtered/deduped vector instead of the raw evaluations.
- Around line 522-563: The UnapprovedWrite handling currently always calls
store::add_escalation (and creates/upserts a log via store::add_log_entry /
store::update_log_entry), causing duplicate pending escalations; before adding a
new escalation in the ExecutionOutcome::UnapprovedWrite branch, query whether a
pending escalation already exists for the task (e.g., via a store method like
has_pending_escalation or a new DB lookup) and if one exists, avoid calling
store::add_escalation — instead update the existing escalation and its
associated log (use update_log_entry when log_id is present or update the
existing escalation's log reference) so only one pending approval row remains;
apply the same check/update logic to the identical code path around the
EscalationPriority::Important handling at the other block (lines ~591-615).
- Around line 347-390: approve_escalation currently reads the escalation and
executes its task before marking it approved, allowing duplicate executions on
retries; change the flow so you atomically claim the escalation in the DB first
(e.g. implement/use a store method like store::claim_escalation(conn,
escalation_id, from_status, to_status) or perform an UPDATE ... WHERE status =
'pending' and check affected rows) inside the initial store::with_connection
call, returning an error if no row was claimed, and only after a successful
single-row claim proceed to build the report, call
executor::execute_approved_write, and finally call
store::resolve_escalation/store::mark_task_completed within the same or a new
connection as appropriate; reference functions: approve_escalation,
store::with_connection, store::resolve_escalation, and
executor::execute_approved_write to locate where to insert the atomic claim and
the subsequent guarded execution.
- Around line 132-136: The fallback seeding branch in engine.rs currently calls
self.seed_tasks() and unconditionally sets state.seeded = true even if seeding
failed; change seed_tasks() to return a Result (or propagate errors) and only
set state.seeded = true when that Result is Ok — i.e., call self.seed_tasks(),
check the return value, log/handle Err but do not flip state.seeded until
success. Apply the same fix to the other similar block (the second occurrence
referenced around lines 418-426) so both places only mark the engine seeded
after a successful seed_tasks() result.
- Around line 214-232: The current one-shot check comparing tick_generation to
my_generation only protects the early return but allows a newer tick to
supersede while the later side-effecting code runs; fix by re-checking
tick_generation.load(Ordering::SeqCst) == my_generation immediately before every
side-effecting branch (e.g., before calling store::with_connection, before
running tools, before rewriting/cancelling log rows, and before
rescheduling/completing tasks), and if it no longer matches, perform the same
cancel-stale_in_progress logic, increment state.total_ticks and return an empty
TickResult; alternatively, serialize the whole execution phase by acquiring a
dedicated execution lock (or reuse state.lock()) for the duration of the
side-effecting section so no newer generation can interleave (apply the same
change around the later block referenced by the reviewer near the 245-266
region).
In `@src/openhuman/subconscious/situation_report.rs`:
- Around line 78-91: The Pending Tasks block currently uses
super::store::list_tasks(conn, false) and then displays all returned rows;
update the logic in situation_report.rs to filter the retrieved tasks collection
to only those with enabled == true and completed == false (e.g.,
tasks.retain(|t| t.enabled && !t.completed) or an iterator filter) before
performing the is_empty() check and the for loop that writes lines; keep the
rest of the flow (error handling from with_connection, variable names tasks and
section, and writeln! loop) intact so only active, not completed/disabled, tasks
are counted and rendered.
In `@src/openhuman/subconscious/store.rs`:
- Around line 168-183: The remove_task function currently deletes a task
unconditionally causing orphaned rows in subconscious_escalations and later
failures in approve_escalation; update remove_task to first check for any
pending escalations referencing the task (query subconscious_escalations WHERE
task_id = ? AND status = 'pending' or NULL as appropriate) and return an error
if any exist, or alternatively add/ensure a foreign key on
subconscious_escalations.task_id → subconscious_tasks.id with ON DELETE RESTRICT
so deletes are prevented at the DB level; reference the remove_task function,
subconscious_escalations.task_id, and approve_escalation when making the change.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4f01bc5e-fa9e-4e36-940b-b8ff0675ecbe
📒 Files selected for processing (19)
app/src/hooks/useSubconscious.tsapp/src/pages/Intelligence.tsxapp/src/utils/tauriCommands/index.tsapp/src/utils/tauriCommands/subconscious.tsdocs/memory-sync-functions.mddocs/subconscious.mdsrc/openhuman/config/schema/heartbeat_cron.rssrc/openhuman/heartbeat/engine.rssrc/openhuman/subconscious/decision_log.rssrc/openhuman/subconscious/engine.rssrc/openhuman/subconscious/executor.rssrc/openhuman/subconscious/integration_test.rssrc/openhuman/subconscious/mod.rssrc/openhuman/subconscious/prompt.rssrc/openhuman/subconscious/schemas.rssrc/openhuman/subconscious/situation_report.rssrc/openhuman/subconscious/store.rssrc/openhuman/subconscious/types.rstests/subconscious_e2e.rs
✅ Files skipped from review due to trivial changes (3)
- app/src/utils/tauriCommands/index.ts
- src/openhuman/config/schema/heartbeat_cron.rs
- app/src/pages/Intelligence.tsx
🚧 Files skipped from review as they are similar to previous changes (6)
- docs/memory-sync-functions.md
- src/openhuman/subconscious/decision_log.rs
- src/openhuman/subconscious/mod.rs
- app/src/hooks/useSubconscious.ts
- tests/subconscious_e2e.rs
- src/openhuman/subconscious/executor.rs
Comment on lines
+56
to
+64
| Seeded automatically on engine initialization. Cannot be deleted, only disabled. | ||
|
|
||
| Default system tasks: | ||
| - Check connected skills for errors or disconnections | ||
| - Review new memory updates for actionable items | ||
| - Monitor system health (Ollama, memory, connections) | ||
|
|
||
| Additional system tasks are imported from `HEARTBEAT.md` in the workspace directory (one task per `- ` line). | ||
|
|
Contributor
There was a problem hiding this comment.
The docs still describe a HEARTBEAT.md import path that no longer exists.
The current runtime only seeds DEFAULT_SYSTEM_TASKS in src/openhuman/subconscious/store.rs; src/openhuman/subconscious/engine.rs does not import additional workspace tasks from HEARTBEAT.md. Leaving this here will send users to a configuration file the engine ignores.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/subconscious.md` around lines 56 - 64, The docs incorrectly claim the
runtime imports additional system tasks from HEARTBEAT.md; update the
documentation to reflect actual behavior by removing or replacing that sentence
and stating that only DEFAULT_SYSTEM_TASKS (seeded in DEFAULT_SYSTEM_TASKS in
src/openhuman/subconscious/store.rs) are used — do not mention imports from
src/openhuman/subconscious/engine.rs since engine.rs does not read HEARTBEAT.md;
ensure the doc references DEFAULT_SYSTEM_TASKS as the source of seeded system
tasks.
Comment on lines
+156
to
+163
| ### agentic-v1 (cloud) | ||
|
|
||
| Used for: | ||
| - Tool-required task execution (send, post, delete, create) | ||
| - Analysis-only mode for read-only tasks escalated by the local model | ||
|
|
||
| Rate-limit retry: up to 3 attempts with exponential backoff (2s, 4s, 8s) on 429 errors. | ||
|
|
Contributor
There was a problem hiding this comment.
The retry/backoff description overstates one extra retry.
The executor only performs the initial call plus two 429 retries, so the documented backoff should stop at 2s, 4s, not 2s, 4s, 8s. Based on learnings agent_chat_with_retry intentionally performs 3 total attempts (1 initial + 2 retries) with 2s and 4s exponential backoff.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/subconscious.md` around lines 156 - 163, Update the retry/backoff
description for agentic-v1 to reflect the actual behavior of
agent_chat_with_retry: it performs 3 total attempts (1 initial + 2 retries) with
exponential backoff delays of 2s and 4s on 429 errors, so replace "2s, 4s, 8s"
with "2s, 4s" and note the 3 total attempts.
Comment on lines
+117
to
+122
| let default = "# Subconscious Instructions\n\ | ||
| #\n\ | ||
| # The subconscious loop checks these tasks periodically against\n\ | ||
| # The subconscious loop evaluates pending tasks periodically against\n\ | ||
| # your workspace state (memory, skills, email, etc.)\n\ | ||
| # Add or remove tasks — one per line starting with `- `\n\n\ | ||
| - Check for new emails that need attention\n\ | ||
| - Review upcoming deadlines and calendar events\n\ | ||
| - Monitor connected skills for errors or disconnections\n"; | ||
| # Tasks are managed in the Subconscious UI — this file provides\n\ | ||
| # additional context and instructions for task evaluation.\n"; |
Contributor
There was a problem hiding this comment.
Leading whitespace bleeds into the file content.
The string continuation here includes the code indentation as literal whitespace in the output. Lines 119-122 will have ~27 leading spaces in the generated HEARTBEAT.md:
# Subconscious Instructions
#
# The subconscious loop evaluates...
🔧 Proposed fix using a raw string or dedent
if !path.exists() {
- let default = "# Subconscious Instructions\n\
- #\n\
- # The subconscious loop evaluates pending tasks periodically against\n\
- # your workspace state (memory, skills, email, etc.)\n\
- # Tasks are managed in the Subconscious UI — this file provides\n\
- # additional context and instructions for task evaluation.\n";
+ let default = "\
+# Subconscious Instructions
+#
+# The subconscious loop evaluates pending tasks periodically against
+# your workspace state (memory, skills, email, etc.)
+# Tasks are managed in the Subconscious UI — this file provides
+# additional context and instructions for task evaluation.
+";
tokio::fs::write(&path, default).await?;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let default = "# Subconscious Instructions\n\ | |
| #\n\ | |
| # The subconscious loop checks these tasks periodically against\n\ | |
| # The subconscious loop evaluates pending tasks periodically against\n\ | |
| # your workspace state (memory, skills, email, etc.)\n\ | |
| # Add or remove tasks — one per line starting with `- `\n\n\ | |
| - Check for new emails that need attention\n\ | |
| - Review upcoming deadlines and calendar events\n\ | |
| - Monitor connected skills for errors or disconnections\n"; | |
| # Tasks are managed in the Subconscious UI — this file provides\n\ | |
| # additional context and instructions for task evaluation.\n"; | |
| if !path.exists() { | |
| let default = "\ | |
| # Subconscious Instructions | |
| # | |
| # The subconscious loop evaluates pending tasks periodically against | |
| # your workspace state (memory, skills, email, etc.) | |
| # Tasks are managed in the Subconscious UI — this file provides | |
| # additional context and instructions for task evaluation. | |
| "; | |
| tokio::fs::write(&path, default).await?; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/openhuman/heartbeat/engine.rs` around lines 117 - 122, The multiline
string assigned to let default in engine.rs contains the function's source
indentation as literal leading spaces; replace it with a dedented/raw string
literal so the generated HEARTBEAT.md does not include indentation. Locate the
let default = ... assignment in engine.rs (symbol: default) and convert the
value to a raw string (or otherwise build the lines without the surrounding code
indentation, e.g., an r#"..."# placed at column 1 or using a dedent helper) so
each line starts without the ~27 leading spaces.
Comment on lines
+252
to
+257
| match eval.decision { | ||
| TickDecision::Act => { | ||
| self.handle_act(task, &report, &identity, tick_at, eval, log_id) | ||
| .await; | ||
| executed += 1; | ||
| } |
Contributor
There was a problem hiding this comment.
executed is incremented before you know whether anything actually executed.
The Act branch bumps executed even when handle_act() later fails or returns ExecutionOutcome::UnapprovedWrite, which should count as an escalation instead. Have handle_act() return a small outcome enum so TickResult reflects completed work instead of just model intent.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/openhuman/subconscious/engine.rs` around lines 252 - 257, The code
increments executed in the Tick loop immediately on TickDecision::Act, but
handle_act() can fail or return ExecutionOutcome::UnapprovedWrite which should
not be counted as executed; change handle_act() to return a small outcome enum
(e.g., ExecutionOutcome with variants Success, UnapprovedWrite, Error) and in
the match arm for TickDecision::Act call handle_act(...).await, inspect its
returned outcome, increment executed only for Success, and increment the
appropriate escalation/error counters (or update TickResult) for
UnapprovedWrite/Error so TickResult reflects actual completed work rather than
model intent; update any callers/structs (TickResult, types that consume
executed) to account for the new outcome.
Comment on lines
+322
to
+330
| recurrence: params.get("recurrence").and_then(|v| v.as_str()).map(|s| { | ||
| if s == "once" { | ||
| TaskRecurrence::Once | ||
| } else if let Some(expr) = s.strip_prefix("cron:") { | ||
| TaskRecurrence::Cron(expr.to_string()) | ||
| } else { | ||
| TaskRecurrence::Pending | ||
| } | ||
| }), |
Contributor
There was a problem hiding this comment.
Reject invalid recurrence strings instead of coercing them to pending.
Any unknown value currently falls through to TaskRecurrence::Pending. A typo like cron:bad or daily therefore mutates the task into an always-due pending task instead of returning a validation error. Parse only the supported shapes and fail the RPC on invalid cron syntax.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/openhuman/subconscious/schemas.rs` around lines 322 - 330, The current
parsing for recurrence in the block that reads params.get("recurrence") coerces
any unrecognized string into TaskRecurrence::Pending (e.g., typos like
"cron:bad" or "daily"), which should instead be rejected; update the parser so
it only accepts the exact supported shapes: "once" -> TaskRecurrence::Once and
"cron:<expr>" -> TaskRecurrence::Cron only if the cron expression validates,
otherwise return a validation error from the RPC path (do not map to
TaskRecurrence::Pending). Specifically, change the closure that produces
recurrence to return a Result and perform cron syntax validation for the expr
before constructing TaskRecurrence::Cron, and propagate/convert failures into an
appropriate RPC/validation error instead of falling back to
TaskRecurrence::Pending.
Comment on lines
+146
to
+165
| pub fn update_task(conn: &Connection, task_id: &str, patch: &TaskPatch) -> Result<()> { | ||
| if let Some(ref title) = patch.title { | ||
| conn.execute( | ||
| "UPDATE subconscious_tasks SET title = ?1 WHERE id = ?2", | ||
| rusqlite::params![title, task_id], | ||
| )?; | ||
| } | ||
| if let Some(ref recurrence) = patch.recurrence { | ||
| conn.execute( | ||
| "UPDATE subconscious_tasks SET recurrence = ?1 WHERE id = ?2", | ||
| rusqlite::params![recurrence_to_string(recurrence), task_id], | ||
| )?; | ||
| } | ||
| if let Some(enabled) = patch.enabled { | ||
| conn.execute( | ||
| "UPDATE subconscious_tasks SET enabled = ?1 WHERE id = ?2", | ||
| rusqlite::params![enabled, task_id], | ||
| )?; | ||
| } | ||
| Ok(()) |
Contributor
There was a problem hiding this comment.
Harden update_task() before applying arbitrary patches.
This path neither checks that a row exists nor protects system-task invariants. Today a bad task_id still returns success, and callers can rename or re-schedule seeded system tasks even though they are supposed to be disable-only. Load the task first, reject non-enabled patches on source == "system", and error when no row matches.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/openhuman/subconscious/store.rs` around lines 146 - 165, The update_task
function currently applies patches blindly; first SELECT the existing row for
the given task_id (e.g. fetch fields id, source, enabled, title, recurrence)
inside update_task and return an error if no row is found, then enforce
invariants: if the existing task has source == "system" reject any patch that
modifies title or recurrence (only allow changing enabled) and return an Err in
that case; finally apply updates only after those checks (use the same
rusqlite::params and recurrence_to_string for recurrence) so callers cannot
rename or reschedule seeded system tasks and invalid task_ids produce an error.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Stabilizes and extends the heartbeat + subconscious loop per #392. The subconscious engine now runs reliably with proper overlap protection, lifecycle logging, and an approval gate for unsolicited write actions.
Key changes
enabled=true,inference_enabled=true, 5-min intervalin_progress→act/noop/escalate/failed/cancelled)last_tick_atcorrectness: only advances on successful evaluation, not on failure/canceldocs/subconscious.mdarchitecture guide,docs/memory-sync-functions.mdreferenceFiles changed
engine.rs,executor.rs,store.rs,types.rs,schemas.rs,prompt.rs,mod.rsheartbeat/engine.rs,heartbeat_cron.rsIntelligence.tsx,useSubconscious.ts,subconscious.ts,index.tsintegration_test.rs,decision_log.rs,subconscious_e2e.rssubconscious.md,memory-sync-functions.mdCloses #392
Test plan
cargo checkpassescargo fmt --checkpassesyarn typecheckpassesyarn lintpasses (0 errors, pre-existing warnings only)in_progressentries appear → transition to final state🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Chores
Tests