feat(subconscious): memory-context-aware reflection threads + dedupe gates (#623)#1344
Merged
senamakel merged 15 commits intotinyhumansai:mainfrom May 7, 2026
Merged
Conversation
Adds the proactive subconscious layer's persistence foundation (tinyhumansai#623): - `reflection.rs`: `Reflection` / `ReflectionKind` / `Disposition` types, `ReflectionDraft` LLM-side wire shape, `MAX_REFLECTIONS_PER_TICK = 5` per-tick cap, dedup key helper. Pure types; no I/O. - `reflection_store.rs`: SQLite CRUD over `subconscious_reflections` and `subconscious_hotness_snapshots`. `add_reflection` is idempotent on id; `mark_surfaced` is one-shot; `replace_hotness_snapshots` is atomic. - DDL appended to `subconscious::store::SCHEMA_DDL` so a single migration runs per `with_connection` lifecycle. - 17 unit tests cover the wire round-trip, hydration, dedup stability, cap behaviour, idempotency, ordering, and snapshot lifecycle. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…nal sections Replaces the legacy unified-store sections (tinyhumansai#623) with sections derived from the memory tree: - `mod.rs`: orchestrates priority-ordered append + token-budget truncation. Drops `MemoryClient`-backed sections (`list_documents`, `graph_query`) and the local-skills placeholder. - `hotness.rs`: top-K movers from `mem_tree_entity_hotness`, joined against `subconscious_hotness_snapshots` for since-last-tick deltas. Refreshes the snapshot table at the end of each render. - `summaries.rs`: rows from `mem_tree_summaries` sealed since `last_tick_at`, grouped by tree scope. - `digest.rs`: most recent global L0 daily digest body. - `query_window.rs`: wraps `tree::retrieval::global::query_global` with a cold-start / since-last-tick window calculator. - `reflections.rs`: previous-tick reflections (last 8) — the LLM's anti-double-emit signal so the model decays/strengthens/skips rather than re-emitting. `engine.rs` updated to load the `Config` and recent reflections before calling `build_situation_report`. Failure to load either is non-fatal — sections degrade to "unavailable" stubs so the tick still proceeds. Old `situation_report.rs` deleted in favour of the directory. 11 unit tests cover the env/append/truncation logic plus the reflections formatter and the window-day calculator. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…thread Bridges the proactive subconscious layer to the conversation surface (tinyhumansai#623): - Stable thread id `system:subconscious`, title "OpenHuman — Proactive", labels `["system", "subconscious"]` so the frontend can filter the system thread out of the regular conversation list. - `ensure_subconscious_thread` — idempotent locate-or-create wrapping `conversations::ensure_thread`. Repeated calls produce the same row. - `post_reflection` — appends a Notify reflection's body (with rendered `proposed_action` paragraph) as an assistant-role message and embeds `{reflection_id, kind, disposition, proposed_action, source_refs}` in `extra_metadata` so the renderer can wire the action button. - Refuses to post Observe-disposition reflections — those persist only. 7 unit tests cover body rendering with/without action, blank-action ignore, metadata shape, thread idempotency on restart, and the Observe-disposition refusal. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Wires the proactive subconscious layer into the tick loop (tinyhumansai#623): - `types.rs`: extended `EvaluationResponse` with additive `reflections: Vec<ReflectionDraft>` (default empty for back-compat). - `prompt.rs`: extended `build_evaluation_prompt` to teach the LLM the reflection schema (kind/body/disposition/proposed_action/source_refs), the anti-double-emit rule, and the no-side-effect rule. Added `format_recent_reflections_for_prompt` so the wire format matches what the situation report emits. - `engine.rs`: - Switched the tick LLM call from `local_ai::ops::local_ai_chat` to `memory::tree::chat::build_chat_provider(_, ChatConsumer::Summarise)`. No fallback to local — per tinyhumansai#623 the subconscious must never contend on the local LLM (`feedback_local_llm_load.md`). - On cloud unreachable, log warn and return empty results so the tick is treated as a skip and `last_tick_at` is NOT advanced. - Renamed `parse_evaluations` → `parse_response`, returning `(evaluations, reflection_drafts)`. - Added `persist_and_surface_reflections` that caps drafts to `MAX_REFLECTIONS_PER_TICK = 5`, hydrates them with fresh UUIDs, persists via `reflection_store::add_reflection`, and for Notify disposition: ensures the `system:subconscious` thread, posts the rendered body via `conversation_post::post_reflection`, and stamps `surfaced_at`. - Skipped on `evaluation_failed` so we never persist mock-shaped output as if it were real. Two new engine tests cover the reflection-extraction and only-reflections-no-evaluations cases. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds three new JSON-RPC controllers (tinyhumansai#623), namespaced under `subconscious.reflections_*`: - `reflections_list { limit?, since_ts? }` — read recent reflections, newest-first, with optional cursor + cap. - `reflections_act { reflection_id, target_thread_id }` — drives `channels::providers::web::start_chat` against the user's active orchestrator thread (NOT the subconscious feed) with a primer composed from the reflection's body + `proposed_action`. Stamps `acted_on_at` on success and returns `{request_id, reflection_id}`. - `reflections_dismiss { reflection_id }` — stamps `dismissed_at`. The `target_thread_id` design choice is deliberate: tapping a proposed action moves the conversation into the user's normal chat surface, leaving the subconscious thread observation-only. Schemas auto-pick-up via `all_subconscious_*` exporters (already wired in `core/all.rs`). Test count bumped from 10 → 13. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Surfaces the proactive reflection layer in the Intelligence tab (tinyhumansai#623): - `tauriCommands/subconscious.ts`: adds `Reflection`, `ReflectionKind`, `ReflectionDisposition` types + `listReflections`, `actOnReflection`, `dismissReflection` wrappers over the new core RPCs. - `SubconsciousReflectionCards.tsx`: self-contained component that polls `listReflections`, renders a card per visible reflection (Observe + Notify) with a kind chip + disposition badge, an Act button (only for Notify reflections with `proposed_action`), and a Dismiss button. Optimistic dismiss hides the card immediately. Acting routes via `actOnReflection` to the user's active orchestrator thread (passed via prop), NOT the subconscious feed — taps move the conversation into the user's normal chat surface. - `IntelligenceSubconsciousTab.tsx`: wires the cards into the tab via a new optional `activeThreadId` prop (defaults to `null`, which disables the Act button). - 8 Vitest cases cover empty state, Notify/Observe rendering, Act button visibility & wiring, optimistic dismiss + rollback, the no-active-thread disabled state, and the mount-time fetch. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Auto-formatting fix-up across the subconscious modules added in the preceding commits. No functional changes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Clippy fix-ups across the new tinyhumansai#623 modules: - Remove `.map_err(anyhow::Error::from)` calls where the inner result is already an `anyhow::Result` — clippy's `useless_conversion`. - Switch `&PathBuf` parameter on `persist_and_surface_reflections` to `&Path` — clippy's `ptr_arg`. Adjust the callers to pass `.to_path_buf()` only at the FFI boundary into the JSONL store. No functional changes; all 94 subconscious tests still pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…gates (tinyhumansai#623) Core feature: every subconscious reflection now snapshots its source chunks at tick time. Acting on a reflection spawns a fresh chat thread with the reflection as the assistant's first message, and the orchestrator's system prompt for that thread carries the same memory chunks the reflection-LLM saw — follow-up answers stay grounded without polluting the visible conversation. Disposition-based auto-posting is removed entirely. Dedupe gates: persist `last_tick_at` in `subconscious_state`, gate `digest::build_section` and `query_window::build_section` on it. The digest section was previously rendering the same global L0 verbatim every tick, which the LLM kept echoing into duplicate reflections. Resolver fix: source-chunk lookups now query against the full prefixed ref string (`summary:L0:<uuid>` is the actual DB primary key), and route `artifact|person|place|tool|topic` kinds through the entity resolver. Bundled Windows-dev-env fixes that this branch needed for local boot: allow `start_core_process` invoke (tinyhumansai#1316 follow-up), Vite loopback bind, inline redux-persist storage adapter (CJS/ESM interop), MSVC + sccache fixes in `run-dev-win.sh`, pnpm bash pin in `package.json`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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:
📝 WalkthroughWalkthroughAdds a proactive reflection layer: LLM-produced reflection drafts are parsed, hydrated, deduplicated/capped, persisted, exposed via three RPCs, surfaced in a new React UI with optimistic actions that spawn threads, and injected into agent prompts and situation reports. ChangesProactive Reflection Intelligence
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/lib/bootCheck/index.ts (1)
67-89:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDocstring references outdated method name.
Line 67 still documents "Poll
openhuman.ping" but line 89 now correctly callscore.ping. Update the docstring to match the implementation.📝 Proposed fix
/** - * Poll `openhuman.ping` with exponential back-off until the core responds or + * Poll `core.ping` with exponential back-off until the core responds or * we exhaust the budget.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/lib/bootCheck/index.ts` around lines 67 - 89, The docstring above waitForCore incorrectly says it polls `openhuman.ping`; update that comment to state it polls `core.ping` to match the implementation and avoid confusion — locate the waitForCore function and change the text "Poll `openhuman.ping`" (and any other occurrences in that block) to "Poll `core.ping`", and ensure the return description and examples (if any) reflect the `core.ping` method used by callRpc.
🧹 Nitpick comments (4)
app/src/store/index.ts (1)
13-32: ⚡ Quick winKeep the import block contiguous.
The new
defaultStoragedeclaration splits the module’s static imports, which breaks theapp/srcconvention here. Move the later imports above this adapter.As per coding guidelines,
Prefer static imports at the top of the module; do not use dynamic imports (async import(), React.lazy(), await import()) for production app code.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/store/index.ts` around lines 13 - 32, The new inline localStorage adapter (const defaultStorage) is placed before other static imports, breaking the module import ordering; move the adapter declaration below the existing top-of-file static import block so all imports remain contiguous. Locate the defaultStorage object in index.ts and cut/paste it to after the final static import statements (keeping the adapter exactly as-is), ensuring no dynamic imports are introduced and that the module's import block remains contiguous at the top.src/openhuman/channels/providers/web.rs (1)
1136-1170: 💤 Low valueConsider adding trace-level logging for silent failures.
The function intentionally returns
Noneon any error to avoid breaking the chat path, but this makes debugging difficult when chunks unexpectedly don't load. A trace-level log before returningNonein error cases would help diagnose issues without affecting the happy path.♻️ Optional: Add trace logs for silent failure paths
fn load_reflection_chunks_for_thread( workspace_dir: &std::path::Path, thread_id: &str, ) -> Option<Vec<crate::openhuman::subconscious::SourceChunk>> { let messages = crate::openhuman::memory::conversations::get_messages( workspace_dir.to_path_buf(), thread_id, ) - .ok()?; + .map_err(|e| { + log::trace!("[web-channel] load_reflection_chunks: get_messages failed thread={} err={}", thread_id, e); + e + }) + .ok()?; let first = messages.first()?; // ... rest unchanged }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/channels/providers/web.rs` around lines 1136 - 1170, The function load_reflection_chunks_for_thread silently returns None on several failure paths; add trace-level logs at each early-return to aid debugging: log when get_messages fails (include workspace_dir and thread_id and the error), when messages.first() is None, when origin is missing or not "subconscious_reflection" (log the extracted origin and thread_id), when reflection_id is missing, and when with_connection/get_reflection returns None or Err (include reflection_id and any error). Use the existing tracing/log facility in the codebase (e.g., trace! or process_logger) and reference load_reflection_chunks_for_thread, get_messages, extra_metadata origin/reflection_id, and subconscious::reflection_store::get_reflection to place the logs.src/openhuman/subconscious/situation_report/hotness.rs (2)
130-145: 💤 Low valueDocument the ordering requirement more defensively.
The direct
Connection::openbypasseswith_connection's schema migration. While the comment notes that "callers always go throughwith_connectionfirst", this creates an implicit ordering dependency that could cause confusingSQLITE_ERROR: no such tablefailures if the call order changes in future refactors.Consider adding a debug assertion or returning a clearer error if the table doesn't exist, rather than propagating a raw SQLite error.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/subconscious/situation_report/hotness.rs` around lines 130 - 145, The function update_snapshots_with_now currently opens a rusqlite::Connection directly and can surface a confusing raw SQLite error if the required table is missing; add a defensive check right after opening the DB (e.g. query sqlite_master or attempt a harmless SELECT 1 FROM hotness_snapshots LIMIT 1) and if it fails return an anyhow::Error with a clear message suggesting the caller should run with_connection first (or run migrations), and/or keep a debug_assert!(table_exists) to catch ordering problems in tests; ensure the improved error is returned before calling reflection_store::replace_hotness_snapshots so callers get a descriptive error instead of SQLITE_ERROR: no such table.
22-22: 💤 Low valueUnused parameter
_last_tick_at— intentional?The parameter is marked unused with the underscore prefix, but the function signature accepts it. If this is for future use or API consistency, consider adding a brief doc comment explaining when it will be used. Otherwise, if it's dead code, it could be removed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/subconscious/situation_report/hotness.rs` at line 22, build_section currently takes an unused parameter `_last_tick_at`; either remove it or document its intentional reservation: if it's dead, remove `_last_tick_at` from the build_section signature and update all callers (and any trait/impl signatures) to stop passing that argument; if it is reserved for future API consistency, add a brief doc comment above pub async fn build_section(config: &Config, workspace_dir: &Path, _last_tick_at: f64) explaining why `_last_tick_at` is intentionally unused (e.g., "reserved for future use / API compatibility") so the intent is clear to reviewers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/vite.config.ts`:
- Around line 134-141: Update the inline comment above the Vite server config
where "host: host || true" is set to also warn that using true binds 0.0.0.0 and
therefore exposes the dev server (HMR/websocket and served assets) to other
machines on the network; mention that this is the chosen workaround for Windows
dual-stack loopback issues but advise developers to avoid or override it on
shared/corporate networks (e.g., set host to 'localhost' or false when not
debugging cross-stack loopback). Ensure the comment references the config
expression "host: host || true" so readers know which setting carries the
exposure tradeoff.
In `@src/openhuman/subconscious/reflection.rs`:
- Around line 157-162: The dedupe key in dedup_key is too sensitive to LLM
noise: canonicalize source_refs by trimming each ref, dropping empties,
deduplicating (e.g., via a HashSet) and then sorting the unique refs before
joining; canonicalize body by trimming and collapsing consecutive whitespace
into single spaces (and optionally normalizing case or unicode) before taking
the 80-char prefix; keep the final format using ReflectionKind::as_str() so
dedup_key(kind, source_refs, body) produces stable keys across whitespace-only
or duplicate-id variations.
In `@src/openhuman/subconscious/schemas.rs`:
- Around line 616-618: The current call to store::with_connection(...
reflection_store::mark_acted(...)) ignores errors, allowing duplicate threads if
stamping fails; change it to capture the Result returned by
store::with_connection and, on Err, emit a warning (e.g., warn! or the project's
logging facility) that includes the reflection_id and the error details (and
optionally now) so failures are observable while keeping behavior non-fatal;
locate the invocation of store::with_connection and reflection_store::mark_acted
to implement this check-and-log.
In `@src/openhuman/subconscious/source_chunk.rs`:
- Around line 115-132: The match only routes six hardcoded prefixes to
resolve_entity and then normalizes resolved results to kind="entity", which
drops other valid prefixed kinds and loses the original LLM-provided kind;
change the logic so any non-empty, non-"summary" prefixed ref is routed to
resolve_entity (i.e., treat all non-summary kinds as entity lookups) and ensure
the original kind string is preserved in the resulting SourceChunk (either by
passing the original kind into resolve_entity or by having the caller set
SourceChunk.kind to the original kind after resolution); update the other
analogous match at the 276-301 region too.
---
Outside diff comments:
In `@app/src/lib/bootCheck/index.ts`:
- Around line 67-89: The docstring above waitForCore incorrectly says it polls
`openhuman.ping`; update that comment to state it polls `core.ping` to match the
implementation and avoid confusion — locate the waitForCore function and change
the text "Poll `openhuman.ping`" (and any other occurrences in that block) to
"Poll `core.ping`", and ensure the return description and examples (if any)
reflect the `core.ping` method used by callRpc.
---
Nitpick comments:
In `@app/src/store/index.ts`:
- Around line 13-32: The new inline localStorage adapter (const defaultStorage)
is placed before other static imports, breaking the module import ordering; move
the adapter declaration below the existing top-of-file static import block so
all imports remain contiguous. Locate the defaultStorage object in index.ts and
cut/paste it to after the final static import statements (keeping the adapter
exactly as-is), ensuring no dynamic imports are introduced and that the module's
import block remains contiguous at the top.
In `@src/openhuman/channels/providers/web.rs`:
- Around line 1136-1170: The function load_reflection_chunks_for_thread silently
returns None on several failure paths; add trace-level logs at each early-return
to aid debugging: log when get_messages fails (include workspace_dir and
thread_id and the error), when messages.first() is None, when origin is missing
or not "subconscious_reflection" (log the extracted origin and thread_id), when
reflection_id is missing, and when with_connection/get_reflection returns None
or Err (include reflection_id and any error). Use the existing tracing/log
facility in the codebase (e.g., trace! or process_logger) and reference
load_reflection_chunks_for_thread, get_messages, extra_metadata
origin/reflection_id, and subconscious::reflection_store::get_reflection to
place the logs.
In `@src/openhuman/subconscious/situation_report/hotness.rs`:
- Around line 130-145: The function update_snapshots_with_now currently opens a
rusqlite::Connection directly and can surface a confusing raw SQLite error if
the required table is missing; add a defensive check right after opening the DB
(e.g. query sqlite_master or attempt a harmless SELECT 1 FROM hotness_snapshots
LIMIT 1) and if it fails return an anyhow::Error with a clear message suggesting
the caller should run with_connection first (or run migrations), and/or keep a
debug_assert!(table_exists) to catch ordering problems in tests; ensure the
improved error is returned before calling
reflection_store::replace_hotness_snapshots so callers get a descriptive error
instead of SQLITE_ERROR: no such table.
- Line 22: build_section currently takes an unused parameter `_last_tick_at`;
either remove it or document its intentional reservation: if it's dead, remove
`_last_tick_at` from the build_section signature and update all callers (and any
trait/impl signatures) to stop passing that argument; if it is reserved for
future API consistency, add a brief doc comment above pub async fn
build_section(config: &Config, workspace_dir: &Path, _last_tick_at: f64)
explaining why `_last_tick_at` is intentionally unused (e.g., "reserved for
future use / API compatibility") so the intent is clear to reviewers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f5a4e37d-8b97-48fd-83f0-67c937e9821a
📒 Files selected for processing (34)
app/package.jsonapp/src-tauri/permissions/allow-core-process.tomlapp/src-tauri/src/core_process.rsapp/src/components/intelligence/IntelligenceSubconsciousTab.tsxapp/src/components/intelligence/SubconsciousReflectionCards.tsxapp/src/components/intelligence/__tests__/SubconsciousReflectionCards.test.tsxapp/src/lib/bootCheck/index.tsapp/src/pages/Conversations.tsxapp/src/store/index.tsapp/src/utils/tauriCommands/subconscious.tsapp/vite.config.tssrc/openhuman/agent/harness/session/builder.rssrc/openhuman/agent/prompts/mod.rssrc/openhuman/channels/providers/web.rssrc/openhuman/subconscious/engine.rssrc/openhuman/subconscious/engine_tests.rssrc/openhuman/subconscious/mod.rssrc/openhuman/subconscious/prompt.rssrc/openhuman/subconscious/reflection.rssrc/openhuman/subconscious/reflection_store.rssrc/openhuman/subconscious/reflection_store_tests.rssrc/openhuman/subconscious/reflection_tests.rssrc/openhuman/subconscious/schemas.rssrc/openhuman/subconscious/schemas_tests.rssrc/openhuman/subconscious/situation_report.rssrc/openhuman/subconscious/situation_report/digest.rssrc/openhuman/subconscious/situation_report/hotness.rssrc/openhuman/subconscious/situation_report/mod.rssrc/openhuman/subconscious/situation_report/query_window.rssrc/openhuman/subconscious/situation_report/reflections.rssrc/openhuman/subconscious/situation_report/summaries.rssrc/openhuman/subconscious/source_chunk.rssrc/openhuman/subconscious/store.rssrc/openhuman/subconscious/types.rs
💤 Files with no reviewable changes (1)
- src/openhuman/subconscious/situation_report.rs
- `dedup_key` now trims/dedupes/sorts source_refs and collapses runs of whitespace in the body before truncating, so LLM noise like duplicate ids or stray newlines no longer slip past the dedupe gate. - `resolve_entity` preserves the LLM's original kind prefix (`artifact:`, `person:`, `tool:`, …) instead of flattening every resolved entity to `kind: "entity"`. Routing also accepts any non-summary non-empty kind so we don't silently drop refs outside an arbitrary allowlist — the SQL miss decides whether the row exists. - `handle_reflections_act` logs a warning when the post-thread `mark_acted` stamp write fails. Without the log, a silent failure here leaves the reflection card visible and a re-Act would spawn a duplicate thread. - Vite `host: true` comment now calls out the 0.0.0.0 / LAN exposure side effect and points at `host: 'localhost'` as the override on shared / corporate networks. - `bootCheck::waitForCore` docstring corrected to reference `core.ping` (matched the implementation since the tinyhumansai#1316 fix; comment was stale). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/openhuman/subconscious/source_chunk.rs (1)
290-297:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winInconsistent
kindassignment onnot_foundpath.The
Ok(None)branch hardcodeskind: "entity"while the success path (line 285) and error path (line 304) both preserveoriginal_kind. This means aperson:fooref that doesn't exist in the index will be labeled"entity"instead of"person", breaking downstream UI/prompt rendering that expects the original prefix.🐛 Proposed fix
Ok(None) => SourceChunk { ref_id: raw.to_string(), - kind: "entity".to_string(), + kind: original_kind, content: String::new(), metadata: serde_json::json!({ "resolver_status": "not_found", }), },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/subconscious/source_chunk.rs` around lines 290 - 297, The Ok(None) branch constructs a SourceChunk with kind hardcoded to "entity" which should preserve the original_kind like the other branches; update the Ok(None) SourceChunk to set kind: original_kind.to_string() (or equivalent) instead of "entity" so refs like "person:foo" keep their original prefix; keep the rest of the fields and metadata (resolver_status: "not_found") unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@src/openhuman/subconscious/source_chunk.rs`:
- Around line 290-297: The Ok(None) branch constructs a SourceChunk with kind
hardcoded to "entity" which should preserve the original_kind like the other
branches; update the Ok(None) SourceChunk to set kind: original_kind.to_string()
(or equivalent) instead of "entity" so refs like "person:foo" keep their
original prefix; keep the rest of the fields and metadata (resolver_status:
"not_found") unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f953570f-f966-494d-9adb-3bf657b6e707
📒 Files selected for processing (5)
app/src/lib/bootCheck/index.tsapp/vite.config.tssrc/openhuman/subconscious/reflection.rssrc/openhuman/subconscious/schemas.rssrc/openhuman/subconscious/source_chunk.rs
…us-memory-tree # Conflicts: # app/src-tauri/permissions/allow-core-process.toml # app/src/lib/bootCheck/index.ts # app/src/store/index.ts
`cargo fmt` reordered some imports + collapsed multi-line log macros that went over the column limit; prettier touched two tinyhumansai#623 frontend files that had drifted past the column rule. No behaviour change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app/src-tauri/src/core_process.rs (2)
129-151: 🏗️ Heavy liftSplit listener/takeover concerns out of this module soon.
This change adds more flow control into an already large file (>500 lines). Extracting listener identification/takeover logic into a dedicated submodule would keep
CoreProcessHandlefocused and easier to reason about.As per coding guidelines: "
**/*.rs: Prefer modules ≤ ~500 lines; split growing modules."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src-tauri/src/core_process.rs` around lines 129 - 151, The file has grown past the recommended size and the listener identification/takeover logic is mixed into CoreProcessHandle; extract that logic into a new submodule (e.g., core_process/listener.rs) and move functions related to this behavior—identify_listener, takeover_stale_listener, and any helper types—into it, keeping CoreProcessHandle and its ensure_running method focused: have ensure_running call into the new listener module's public API for identifying and handling stale listeners, update imports/visibility accordingly, and add tests for the moved functions to preserve behavior.
129-151: ⚡ Quick winConsolidate to a single idempotent guard in
ensure_running.This newly added block overlaps with the existing fast-path at Line 107-126. Keeping both increases behavioral drift risk over time because the checks are not exactly the same.
Suggested cleanup
- // Idempotent fast-path: if we already own a running embedded - // task, the listener on this port is us — not a stale external - // process. Without this short-circuit, a second `ensure_running` - // call (from BootCheckGate re-render, React StrictMode mount, or - // any double-invoke of `start_core_process`) hits the - // `identify_listener` path, identifies the listener as - // OpenHuman, calls `takeover_stale_listener`, and aborts with - // "stale-listener pid <self> matches the Tauri host pid; - // refusing to self-terminate". (`#1316` introduced the - // frontend-driven `start_core_process` invoke without - // hardening `ensure_running` against double-invoke.) - { - let guard = self.task.lock().await; - if let Some(task) = guard.as_ref() { - if !task.is_finished() { - log::debug!( - "[core] ensure_running: embedded task already running on port {}, returning Ok (idempotent)", - self.port - ); - return Ok(()); - } - } - }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src-tauri/src/core_process.rs` around lines 129 - 151, There are two overlapping idempotent fast-path checks inside ensure_running; consolidate them by keeping a single guard that acquires self.task.lock().await, checks if guard.as_ref() is Some and !task.is_finished(), logs the idempotent return and returns Ok(()), and remove the duplicate block (the newly added brace-wrapped guard) so ensure_running uses only the canonical self.task check to avoid behavioral drift; update the remaining log message/context to match the removed duplicate if needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@app/src-tauri/src/core_process.rs`:
- Around line 129-151: The file has grown past the recommended size and the
listener identification/takeover logic is mixed into CoreProcessHandle; extract
that logic into a new submodule (e.g., core_process/listener.rs) and move
functions related to this behavior—identify_listener, takeover_stale_listener,
and any helper types—into it, keeping CoreProcessHandle and its ensure_running
method focused: have ensure_running call into the new listener module's public
API for identifying and handling stale listeners, update imports/visibility
accordingly, and add tests for the moved functions to preserve behavior.
- Around line 129-151: There are two overlapping idempotent fast-path checks
inside ensure_running; consolidate them by keeping a single guard that acquires
self.task.lock().await, checks if guard.as_ref() is Some and
!task.is_finished(), logs the idempotent return and returns Ok(()), and remove
the duplicate block (the newly added brace-wrapped guard) so ensure_running uses
only the canonical self.task check to avoid behavioral drift; update the
remaining log message/context to match the removed duplicate if needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d3dcb6e2-2850-4a6d-be83-f1d46ad11cb7
📒 Files selected for processing (2)
app/package.jsonapp/src-tauri/src/core_process.rs
✅ Files skipped from review due to trivial changes (1)
- app/package.json
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
src/openhuman/subconscious/source_chunk.rs (1)
279-285:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep the original ref kind on entity lookup misses.
Line 281 still flattens every unresolved ref to
"entity", so a missingartifact:...orperson:...snapshot loses the exact category the LLM cited even though the success and db-error paths preserve it. That leaves the UI/prompt with a weaker “source unavailable” snapshot than intended.Suggested fix
Ok(None) => SourceChunk { ref_id: raw.to_string(), - kind: "entity".to_string(), + kind: original_kind, content: String::new(), metadata: serde_json::json!({ "resolver_status": "not_found", }), },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/subconscious/source_chunk.rs` around lines 279 - 285, The Ok(None) branch currently hardcodes kind: "entity", losing the original ref category; change it to extract and preserve the original ref kind from raw (e.g., let kind = raw.splitn(2, ':').next().unwrap_or("entity").to_string()) and use that variable for the SourceChunk.kind field so unresolved refs keep their original category (keep ref_id as raw.to_string() and metadata resolver_status as "not_found").
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/openhuman/subconscious/engine.rs`:
- Around line 783-830: The fallback path in parse_response returns Noop
evaluations with a reason that doesn't trigger tick()'s failure handling; update
parse_response (function parse_response) so that any
synthetic/unparseable-evaluation reasons use the same "Evaluation failed:"
prefix (e.g., "Evaluation failed: Unparseable evaluation response") so tick()
treats these as failures; ensure both the full-envelope branch where evaluations
are empty and the final unparseable branch use this prefixed reason for
TaskEvaluation::reason.
- Around line 881-890: The current batch persist swallows write errors in
store::with_connection when calling reflection_store::add_reflection, allowing
tick() to advance last_tick_at and permanently lose reflections; change the
logic so any reflection_store::add_reflection error causes
store::with_connection to return Err (or begin a DB transaction and rollback on
any failure) and propagate that error out of tick() so last_tick_at is not
advanced on failure; alternatively implement idempotent/deduped inserts in
reflection_store::add_reflection (deterministic unique key or upsert) so partial
failures can be retried safely—update tick(), store::with_connection usage, and
reflection_store::add_reflection to reflect the chosen approach.
- Around line 180-184: The current branch for if due_tasks.is_empty() updates
state.last_tick_at and calls persist_last_tick_at(&self.workspace_dir, tick_at),
advancing the dedupe cursor even though nothing was evaluated; remove those two
lines from the empty-case so the cursor is not advanced when no tasks are
processed. Instead, ensure state.last_tick_at = tick_at and
persist_last_tick_at(...) are executed only in the code path where due_tasks are
actually evaluated (the branch that processes tasks), leaving state.total_ticks
handling as appropriate; look for symbols due_tasks, state.last_tick_at,
persist_last_tick_at, tick_at and move the persist/update into the non-empty
evaluation flow.
In `@src/openhuman/subconscious/situation_report/digest.rs`:
- Around line 65-86: read_latest_global_l0 currently logs entry/no-row/error but
doesn't log successful fetch/exit; add an explicit success/exit diagnostic
immediately before returning a DigestRow so production triage can see cutoff and
row details. Update read_latest_global_l0 to emit a grep-friendly log (e.g.,
"digest:read_latest_global_l0:exit=success") including correlation fields like
cutoff_ms, id and sealed_at_ms (from the returned DigestRow) and/or a short
content hash, using the existing logging facility in scope (e.g., tracing::info!
or process_logger) so the success path is recorded alongside the existing
entry/no-row/error logs.
- Around line 68-85: The query_row call that constructs a DigestRow is currently
followed by .ok(), which masks real SQLite errors as None; import
rusqlite::OptionalExtension and replace the .ok() call on the query_row result
with .optional()? so actual DB errors propagate while QueryReturnedNoRows
becomes Ok(None); locate the query that builds DigestRow (fields id, content,
sealed_at_ms) and make this change there.
---
Duplicate comments:
In `@src/openhuman/subconscious/source_chunk.rs`:
- Around line 279-285: The Ok(None) branch currently hardcodes kind: "entity",
losing the original ref category; change it to extract and preserve the original
ref kind from raw (e.g., let kind = raw.splitn(2,
':').next().unwrap_or("entity").to_string()) and use that variable for the
SourceChunk.kind field so unresolved refs keep their original category (keep
ref_id as raw.to_string() and metadata resolver_status as "not_found").
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b7b74313-5cab-429b-aff8-01ff1abc921c
📒 Files selected for processing (8)
app/src/components/intelligence/SubconsciousReflectionCards.tsxapp/src/utils/tauriCommands/subconscious.tssrc/openhuman/channels/providers/web.rssrc/openhuman/subconscious/engine.rssrc/openhuman/subconscious/mod.rssrc/openhuman/subconscious/reflection_store_tests.rssrc/openhuman/subconscious/situation_report/digest.rssrc/openhuman/subconscious/source_chunk.rs
✅ Files skipped from review due to trivial changes (1)
- src/openhuman/subconscious/reflection_store_tests.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- src/openhuman/channels/providers/web.rs
- app/src/components/intelligence/SubconsciousReflectionCards.tsx
- src/openhuman/subconscious/mod.rs
Comment on lines
180
to
184
| if due_tasks.is_empty() { | ||
| debug!("[subconscious] no due tasks"); | ||
| state.last_tick_at = tick_at; | ||
| persist_last_tick_at(&self.workspace_dir, tick_at); | ||
| state.total_ticks += 1; |
Contributor
There was a problem hiding this comment.
Don't advance last_tick_at when no task was evaluated.
This path now persists the dedupe cursor even though no situation report was built and no reflection window was consumed. If memory-tree rows arrived since the previous real evaluation, they will be filtered out on the next non-empty tick as if they were already processed.
Suggested fix
if due_tasks.is_empty() {
debug!("[subconscious] no due tasks");
- state.last_tick_at = tick_at;
- persist_last_tick_at(&self.workspace_dir, tick_at);
state.total_ticks += 1;
return Ok(TickResult {
tick_at,
evaluations: vec![],📝 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
| if due_tasks.is_empty() { | |
| debug!("[subconscious] no due tasks"); | |
| state.last_tick_at = tick_at; | |
| persist_last_tick_at(&self.workspace_dir, tick_at); | |
| state.total_ticks += 1; | |
| if due_tasks.is_empty() { | |
| debug!("[subconscious] no due tasks"); | |
| state.total_ticks += 1; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/openhuman/subconscious/engine.rs` around lines 180 - 184, The current
branch for if due_tasks.is_empty() updates state.last_tick_at and calls
persist_last_tick_at(&self.workspace_dir, tick_at), advancing the dedupe cursor
even though nothing was evaluated; remove those two lines from the empty-case so
the cursor is not advanced when no tasks are processed. Instead, ensure
state.last_tick_at = tick_at and persist_last_tick_at(...) are executed only in
the code path where due_tasks are actually evaluated (the branch that processes
tasks), leaving state.total_ticks handling as appropriate; look for symbols
due_tasks, state.last_tick_at, persist_last_tick_at, tick_at and move the
persist/update into the non-empty evaluation flow.
Comment on lines
+783
to
+830
| /// Parse the per-tick LLM response into evaluations + reflection drafts. | ||
| /// | ||
| /// Best-effort: if the JSON has only `evaluations`, `reflections` is | ||
| /// empty; if it's a bare evaluations array, `reflections` is empty. If | ||
| /// nothing parses, all tasks default to Noop (with a parse-failure | ||
| /// reason) and `reflections` is empty. | ||
| fn parse_response( | ||
| text: &str, | ||
| tasks: &[SubconsciousTask], | ||
| ) -> (Vec<TaskEvaluation>, Vec<ReflectionDraft>) { | ||
| let json_text = extract_json(text); | ||
|
|
||
| // Try parsing as EvaluationResponse | ||
| // 1. Full envelope (preferred). | ||
| if let Ok(response) = serde_json::from_str::<EvaluationResponse>(json_text) { | ||
| if !response.evaluations.is_empty() { | ||
| return response.evaluations; | ||
| } | ||
| let evals = if response.evaluations.is_empty() { | ||
| // The LLM returned only reflections — fall through to the | ||
| // default-noop branch for tasks but keep reflections. | ||
| tasks | ||
| .iter() | ||
| .map(|t| TaskEvaluation { | ||
| task_id: t.id.clone(), | ||
| decision: TickDecision::Noop, | ||
| reason: "No evaluation returned by model".to_string(), | ||
| }) | ||
| .collect() | ||
| } else { | ||
| response.evaluations | ||
| }; | ||
| return (evals, response.reflections); | ||
| } | ||
|
|
||
| // Try parsing as a bare array of evaluations | ||
| // 2. Bare evaluations array (legacy shape pre-#623). | ||
| if let Ok(evals) = serde_json::from_str::<Vec<TaskEvaluation>>(json_text) { | ||
| if !evals.is_empty() { | ||
| return evals; | ||
| return (evals, vec![]); | ||
| } | ||
| } | ||
|
|
||
| warn!("[subconscious] could not parse evaluation response, defaulting all to noop"); | ||
| tasks | ||
| warn!("[subconscious] could not parse LLM response, defaulting all tasks to noop"); | ||
| let evals = tasks | ||
| .iter() | ||
| .map(|t| TaskEvaluation { | ||
| task_id: t.id.clone(), | ||
| decision: TickDecision::Noop, | ||
| reason: "Unparseable evaluation response".to_string(), | ||
| }) | ||
| .collect() | ||
| .collect(); | ||
| (evals, vec![]) |
Contributor
There was a problem hiding this comment.
Unparseable model output still looks like a successful tick.
The fallback here returns ordinary Noop evaluations with reason "Unparseable evaluation response". tick() only treats reasons prefixed with "Evaluation failed:" as failures, so this branch still advances last_tick_at later and drops the same digest/query-window input on the next run even though nothing usable was produced.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/openhuman/subconscious/engine.rs` around lines 783 - 830, The fallback
path in parse_response returns Noop evaluations with a reason that doesn't
trigger tick()'s failure handling; update parse_response (function
parse_response) so that any synthetic/unparseable-evaluation reasons use the
same "Evaluation failed:" prefix (e.g., "Evaluation failed: Unparseable
evaluation response") so tick() treats these as failures; ensure both the
full-envelope branch where evaluations are empty and the final unparseable
branch use this prefixed reason for TaskEvaluation::reason.
Comment on lines
+881
to
+890
| if let Err(e) = store::with_connection(workspace_dir, |conn| { | ||
| for r in &reflections { | ||
| if let Err(e) = reflection_store::add_reflection(conn, r) { | ||
| warn!("[subconscious] reflection persist failed id={}: {e}", r.id); | ||
| } | ||
| } | ||
| Ok(()) | ||
| }) { | ||
| warn!("[subconscious] reflection batch persist failed: {e}"); | ||
| } |
Contributor
There was a problem hiding this comment.
Reflection-store write failures are currently lossy.
If this batch write fails, tick() still proceeds as success and advances last_tick_at. That permanently loses every reflection for this window because the next tick filters out the same source rows. This needs to bubble up as a failure/partial-failure signal, ideally with transactional persistence or deterministic dedupe so retries don't double-insert.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/openhuman/subconscious/engine.rs` around lines 881 - 890, The current
batch persist swallows write errors in store::with_connection when calling
reflection_store::add_reflection, allowing tick() to advance last_tick_at and
permanently lose reflections; change the logic so any
reflection_store::add_reflection error causes store::with_connection to return
Err (or begin a DB transaction and rollback on any failure) and propagate that
error out of tick() so last_tick_at is not advanced on failure; alternatively
implement idempotent/deduped inserts in reflection_store::add_reflection
(deterministic unique key or upsert) so partial failures can be retried
safely—update tick(), store::with_connection usage, and
reflection_store::add_reflection to reflect the chosen approach.
Comment on lines
+65
to
+86
| fn read_latest_global_l0(config: &Config, cutoff_ms: i64) -> anyhow::Result<Option<DigestRow>> { | ||
| crate::openhuman::memory::tree::store::with_connection(config, |conn| { | ||
| let row = conn | ||
| .query_row( | ||
| "SELECT s.id, s.content, s.sealed_at_ms | ||
| FROM mem_tree_summaries s | ||
| JOIN mem_tree_trees t ON t.id = s.tree_id | ||
| WHERE t.kind = ?1 AND s.level = 0 AND s.deleted = 0 | ||
| AND s.sealed_at_ms > ?2 | ||
| ORDER BY s.sealed_at_ms DESC LIMIT 1", | ||
| rusqlite::params![tree_kind_global_str(), cutoff_ms], | ||
| |row| { | ||
| Ok(DigestRow { | ||
| id: row.get(0)?, | ||
| content: row.get(1)?, | ||
| sealed_at_ms: row.get(2)?, | ||
| }) | ||
| }, | ||
| ) | ||
| .ok(); | ||
| Ok(row) | ||
| }) |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add success/exit diagnostics for the DB read helper.
This new flow logs entry/no-row/error, but not successful fetch/return from read_latest_global_l0, which makes cutoff/debug validation harder in production triage.
As per coding guidelines: “Default to verbose diagnostics on new/changed flows with entry/exit logging, branches, external calls, retries/timeouts, state transitions, and errors; use stable grep-friendly prefixes and correlation fields”.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/openhuman/subconscious/situation_report/digest.rs` around lines 65 - 86,
read_latest_global_l0 currently logs entry/no-row/error but doesn't log
successful fetch/exit; add an explicit success/exit diagnostic immediately
before returning a DigestRow so production triage can see cutoff and row
details. Update read_latest_global_l0 to emit a grep-friendly log (e.g.,
"digest:read_latest_global_l0:exit=success") including correlation fields like
cutoff_ms, id and sealed_at_ms (from the returned DigestRow) and/or a short
content hash, using the existing logging facility in scope (e.g., tracing::info!
or process_logger) so the success path is recorded alongside the existing
entry/no-row/error logs.
Comment on lines
+68
to
+85
| .query_row( | ||
| "SELECT s.id, s.content, s.sealed_at_ms | ||
| FROM mem_tree_summaries s | ||
| JOIN mem_tree_trees t ON t.id = s.tree_id | ||
| WHERE t.kind = ?1 AND s.level = 0 AND s.deleted = 0 | ||
| AND s.sealed_at_ms > ?2 | ||
| ORDER BY s.sealed_at_ms DESC LIMIT 1", | ||
| rusqlite::params![tree_kind_global_str(), cutoff_ms], | ||
| |row| { | ||
| Ok(DigestRow { | ||
| id: row.get(0)?, | ||
| content: row.get(1)?, | ||
| sealed_at_ms: row.get(2)?, | ||
| }) | ||
| }, | ||
| ) | ||
| .ok(); | ||
| Ok(row) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check the file exists and read the relevant section
cat -n src/openhuman/subconscious/situation_report/digest.rs | head -100Repository: tinyhumansai/openhuman
Length of output: 4723
🏁 Script executed:
# Also check the imports at the top of the file
head -30 src/openhuman/subconscious/situation_report/digest.rsRepository: tinyhumansai/openhuman
Length of output: 1371
🏁 Script executed:
# Look for any existing use of OptionalExtension or similar patterns
rg "OptionalExtension|optional\(\)" src/openhuman/subconscious/situation_report/ -A 2 -B 2Repository: tinyhumansai/openhuman
Length of output: 48
🏁 Script executed:
# Check what methods rusqlite::Statement and Result provide
rg "query_row|\.ok\(\)" src/openhuman/subconscious/situation_report/ -B 2 -A 2Repository: tinyhumansai/openhuman
Length of output: 906
🏁 Script executed:
# Verify logging statements in the file
rg "log::|tracing::|debug!|error!" src/openhuman/subconscious/situation_report/digest.rsRepository: tinyhumansai/openhuman
Length of output: 193
Replace .ok() with .optional()? to preserve SQLite errors instead of masking them as "no rows."
On Line 84, .ok() converts every query_row error—including schema, type, and SQL failures—into None, causing real database failures to be silently misreported as "No new global digest sealed since last tick." Use .optional()? instead, which distinguishes QueryReturnedNoRows (mapped to Ok(None)) from actual errors (propagated as Err).
Required change:
- Add
use rusqlite::OptionalExtension;to imports - Replace
.ok();with.optional()?;on line 84
Suggested fix
+use rusqlite::OptionalExtension;
+
pub async fn build_section(config: &Config, last_tick_at: f64) -> String {
@@
- .ok();
+ .optional()?;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/openhuman/subconscious/situation_report/digest.rs` around lines 68 - 85,
The query_row call that constructs a DigestRow is currently followed by .ok(),
which masks real SQLite errors as None; import rusqlite::OptionalExtension and
replace the .ok() call on the query_row result with .optional()? so actual DB
errors propagate while QueryReturnedNoRows becomes Ok(None); locate the query
that builds DigestRow (fields id, content, sealed_at_ms) and make this change
there.
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
The diff-coverage gate failed at 66% — three tinyhumansai#623 surfaces had no behaviour-level tests: * `tauriCommands/subconscious.ts` (was 0%): new test file mirrors the pattern in `config.test.ts` / `core.test.ts` — mocks `isTauri` and `callCoreRpc`, asserts each wrapper throws outside Tauri and forwards the right method name + params on the happy path. * `IntelligenceSubconsciousTab.tsx` (was 0% on the act callback): new test stubs out `SubconsciousReflectionCards` and triggers `onNavigateToThread` directly, asserting `setSelectedThread` dispatch and `navigate('/chat')` (not `/conversations`, which redirects to `/home`). * `SubconsciousReflectionCards.tsx` (was 77.5%): three error-path cases added — `listReflections` reject surfaces the error banner; `dismissReflection` reject rolls the optimistic hide back; `actOnReflection` reject leaves the card visible and skips `onNavigateToThread`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Tightens the mock declarations onto single lines and groups the imports above the local `mockDispatch` / `mockNavigate` consts. No behaviour change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
source_chunkresolver freezes memory-tree chunks at tick time so each reflection carries the evidence the LLM citedlast_tick_atacross restarts; gatedigest::build_sectionandquery_window::build_sectionso already-seen content stops re-feeding the LLM each tickDispositionenum and the auto-post-into-existing-thread path are removed entirelystart_core_processallow entry, etc.)Problem
digesthad no cutoff andquery_windowrounded any sub-day gap up to 24h, producing duplicate reflections about the same contentlast_tick_atwas held in-memory only, so dev restarts cold-started the cutoff to0.0and re-emitted reflections about long-sealed contentSolution
subconscious::source_chunk: resolvessource_refsinto{ ref_id, kind, content, metadata }snapshots. Queries against the full prefixed ref string becausemem_tree_summaries.idandmem_tree_entity_index.entity_idboth store the kind prefix as part of the PK. Routesartifact|person|place|tool|topickinds through the entity resolverSystemPromptBuilder::with_reflection_contextadds aReflectionMemoryContextSection. Builder is byte-stable for the session so KV-cache prefix stays warmweb.rs::build_session_agentdetects reflection-origin threads via the seed message'sextra_metadata.origin == "subconscious_reflection"and routes through the chunks-aware constructorsubconscious_stateKV table storeslast_tick_at; loaded on engine init, persisted on every successful tick advancedigest::build_sectionfiltersWHERE sealed_at_ms > last_tick_at*1000; emits a "no new digest since last tick" stub when nothing freshquery_window::build_sectionpost-filters returned hits bytime_range_end > last_tick_at; same empty-tick stubselectedThreadIdarrives so navigating from "Act" lands ready-to-type; reflection cards cap atmax-h-[70vh]with internal scrollSubmission Checklist
docs/TESTING-STRATEGY.mddiff-cover) meet the gate enforced by.github/workflows/coverage.yml. Runpnpm test:coverageandpnpm test:rustlocally; PRs below 80% on changed lines will not merge.## Relateddocs/TESTING-STRATEGY.md)Closes #NNNin the## RelatedsectionImpact
source_chunksJSON column,subconscious_statetable); existing rows back-fill at next opensubconscious_reflections.source_chunks(workspace-scoped SQLite, same isolation level as the rest of the subconscious data). No new external endpointsmigrate_add_source_chunks_columnandmigrate_drop_legacy_columnsswallow duplicate-column / missing-column errors so re-running on already-migrated DBs is a no-opRelated
source_refssignature (within-tick duplicates from a single LLM response still get through; the dedupe gates in this PR only catch cross-tick duplication)AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
feat/623-subconscious-memory-treec6b6942dd5c4cde44b75baceca68c029bfda3c9cValidation Run
pnpm --filter openhuman-app format:checkpnpm typecheckpnpm test:unitandcargo testfor subconscious modulecargo check --manifest-path Cargo.tomlcleanapp/src-tauri/srcRust changes beyondcore_process.rs(which has no fmt drift)Validation Blocked
command:N/Aerror:N/Aimpact:N/ABehavior Changes
Parity Contract
Disposition::Notifyauto-post path is intentionally removed (per Surface proactive intelligence (heartbeat, etc.) in agent responses #623 scope clarification — see commit body)from_config_for_agentstill works for non-reflection threads (no chunks → normal path);load_reflection_chunks_for_threadreturnsNonefor any thread without the reflection-origin metadataDuplicate / Superseded PR Handling
Summary by CodeRabbit
New Features
Improvements