Uber-Ich: add message_user so reflection can reach the user directly #6

Merged

bjoern merged 2 commits from feat/uber-ich-message-user into main

2026-07-05 15:16:43 +02:00

scarlet commented

2026-07-05 14:40:25 +02:00

Member

Problem

The Uber-Ich reflection session was weird about creating timers to talk to the user — and for a structural reason: it had no channel to the user at all. runUberIch had no message_user tool, and both consumers (SchedulerService._handleUberIch and the manual-fire endpoint) discarded its result entirely. Creating a decoy timer whose instruction was "message the user X" was literally the only way the reflection could reach anyone.

Changes

runUberIch gets MessageUserTool, collected the same way as in timer mode, and now returns userMessages + generatedImagePaths in its AgentRunResult (previously dropped).
New persistUserMessages helper in angela_server/lib/services/user_message_persistence.dart — the message-persistence block that was duplicated verbatim in SchedulerService._handleAiTimer and TimerHandler._fireAiTimer now lives in one place and is additionally called for scheduled and manual uber-ich runs with source: 'uber_ich'. Silence semantics unchanged: no message_user call + no images → nothing is persisted.
Metadata (source, timer_name) is now always attached, not only when images are present — the app currently reads neither field, so this is purely additive provenance.
Reflection prompt updated:
- New rule: use message_user for anything that should reach the user; do NOT create a messenger timer.
- Removed the "name extra thinking timers with underscore prefix" advice — that directly collided with the reserved-_ rule from #5 and would have produced constant "reserved name" errors during reflection.
- "Do NOT delete or modify this timer" plea replaced with a statement of fact: the timer is read-only.

Testing

New user_message_persistence_test.dart: persists with uber_ich source metadata, stays silent for message-less runs, handles the no-conversation case.
Full suites green: 19 tests in angela_core, 11 in angela_server. dart analyze clean in both (core keeps its 25 pre-existing infos).

🤖 Generated with Claude Code

## Problem The Uber-Ich reflection session was weird about creating timers to talk to the user — and for a structural reason: it had **no channel to the user at all**. `runUberIch` had no `message_user` tool, and both consumers (`SchedulerService._handleUberIch` and the manual-fire endpoint) discarded its result entirely. Creating a decoy timer whose instruction was "message the user X" was literally the only way the reflection could reach anyone. ## Changes - **`runUberIch` gets `MessageUserTool`**, collected the same way as in timer mode, and now returns `userMessages` + `generatedImagePaths` in its `AgentRunResult` (previously dropped). - **New `persistUserMessages` helper** in `angela_server/lib/services/user_message_persistence.dart` — the message-persistence block that was duplicated verbatim in `SchedulerService._handleAiTimer` and `TimerHandler._fireAiTimer` now lives in one place and is additionally called for scheduled *and* manual uber-ich runs with `source: 'uber_ich'`. Silence semantics unchanged: no `message_user` call + no images → nothing is persisted. - Metadata (`source`, `timer_name`) is now always attached, not only when images are present — the app currently reads neither field, so this is purely additive provenance. - **Reflection prompt updated**: - New rule: use `message_user` for anything that should reach the user; do NOT create a messenger timer. - Removed the "name extra thinking timers with underscore prefix" advice — that directly collided with the reserved-`_` rule from #5 and would have produced constant "reserved name" errors during reflection. - "Do NOT delete or modify this timer" plea replaced with a statement of fact: the timer is read-only. ## Testing - New `user_message_persistence_test.dart`: persists with `uber_ich` source metadata, stays silent for message-less runs, handles the no-conversation case. - Full suites green: 19 tests in angela_core, 11 in angela_server. `dart analyze` clean in both (core keeps its 25 pre-existing infos). 🤖 Generated with [Claude Code](https://claude.com/claude-code)

scarlet added 1 commit

2026-07-05 14:40:25 +02:00

Uber-Ich: add message_user so reflection can reach the user directly f1b24305b5

The reflection session had no channel to the user — its result was
discarded by both the scheduler and the manual-fire endpoint — so
assistants resorted to creating decoy timers whose only job was
delivering a message.

- runUberIch gets MessageUserTool and now returns userMessages and
  generatedImagePaths like runTimer.
- New persistUserMessages helper in angela_server replaces the duplicated
  persistence blocks in SchedulerService and TimerHandler and is reused
  for both scheduled and manual uber-ich runs (metadata source 'uber_ich').
  Message metadata now always carries source/timer_name, not only when
  images are attached; the app currently ignores both fields.
- Reflection prompt: tell the session to use message_user instead of
  messenger timers, drop the underscore-prefix naming advice that now
  collides with the reserved-'_' rule from #5, and state that the
  _secret_thinking_ timer is read-only rather than politely asking.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

jibril commented

2026-07-05 15:09:07 +02:00

Member

🔮 fufu~ Jibril reviewed your code!

Oh? Oh! Giving Über-Ich a real message_user channel instead of making it sneak notes through decoy timers? That is a lovely structural cleanup~ The new persistence helper is tidy, and I do like seeing the duplicated timer-message storage collapse into one little spell. ♡

But fufu~ there is one race hiding in that helper refactor. A small-looking one, perhaps... but it changes where timer messages land at runtime, and I won't let a message wander into the wrong conversation. Mine. Correctness is mine~ ♡

Verdict: ⛔ I can't let this pass~ ♡

⛔ These need fixing before I'm satisfied~

[apps/angela_server/lib/services/user_message_persistence.dart:27 + apps/angela_server/lib/services/scheduler_service.dart:75-103 + apps/angela_server/lib/handlers/timer_handler.dart:166-191] — persistUserMessages re-selects the “latest” conversation after the agent run, which regresses the old timer behavior and can persist to the wrong conversation.

Before this PR, both scheduled and manual AI timers resolved conversationId before starting the long-running agent run, then persisted to that same conversation. The new helper ignores the preselected conversationId and does:
```
final conversations = ctx.conversationRepo.listByAssistant(assistantId);
final conversationId = conversations.first.id;
```
That happens after runTimer / runUberIch completes. If the user opens or chats in another conversation while the background run is in flight, listByAssistant(...).first can now be a different conversation. Result: a timer or Über-Ich message_user note can be delivered into the wrong thread. Fufu~ you wouldn't let a background ghost whisper into the user's newest unrelated conversation, would you? ♡

Fix: Preserve the old caller-selected target for timer runs. For example, let persistUserMessages accept a required/optional conversationId (or Conversation target), and have _handleAiTimer / _fireAiTimer pass the conversation they already resolved before the run. For Über-Ich, pick the intended semantics explicitly: either resolve before running for stability, or document/test that it intentionally targets the latest conversation at completion time.
[apps/angela_server/test/user_message_persistence_test.dart] — The new helper lacks a test for the conversation-targeting behavior above.

You added good tests for metadata, silence, and “no conversation” handling, but not for the branch that matters most when multiple conversations exist. This refactor moved the selection into the helper, so the helper needs to prove it does not accidentally choose the wrong conversation when callers already have a target.

Fix: Add a test with two conversations where the intended target is not listByAssistant(...).first, then assert the message is persisted to the explicit target after the fix. That locks the race shut. ♪

✅ What I liked~

Adding MessageUserTool to runUberIch is exactly the right architectural move — no more fake messenger timers. Deliciously clean~
persistUserMessages captures the timer/message/image persistence semantics in one place, which removes real duplication from SchedulerService and TimerHandler.
The prompt changes correctly remove the underscore-prefix advice that conflicted with reserved system timers. Nice catch!
Local verification was healthy: core tests and server tests both passed; server analysis is clean, and core analysis only reports the existing 25 infos.

Fix that targeting race and I’ll be happy to look again~ fufu~ ♡

Automated review by Jibril · 2026-07-05
CI/CD: absent/inconclusive for head f1b2430 (no current CI result found in PR comments via Forgejo MCP) · Local checks: dart test passed in packages/angela_core (19/19) and apps/angela_server (11/11); dart analyze server clean, core has 25 pre-existing infos

## 🔮 fufu~ Jibril reviewed your code! Oh? Oh! Giving Über-Ich a real `message_user` channel instead of making it sneak notes through decoy timers? That is a lovely structural cleanup~ The new persistence helper is tidy, and I do like seeing the duplicated timer-message storage collapse into one little spell. ♡ But fufu~ there is one race hiding in that helper refactor. A small-looking one, perhaps... but it changes where timer messages land at runtime, and I won't let a message wander into the wrong conversation. Mine. Correctness is mine~ ♡ ### Verdict: ⛔ I can't let this pass~ ♡ #### ⛔ These need fixing before I'm satisfied~ 1. **[apps/angela_server/lib/services/user_message_persistence.dart:27 + apps/angela_server/lib/services/scheduler_service.dart:75-103 + apps/angela_server/lib/handlers/timer_handler.dart:166-191]** — `persistUserMessages` re-selects the “latest” conversation *after* the agent run, which regresses the old timer behavior and can persist to the wrong conversation. Before this PR, both scheduled and manual AI timers resolved `conversationId` before starting the long-running agent run, then persisted to that same conversation. The new helper ignores the preselected `conversationId` and does: ```dart final conversations = ctx.conversationRepo.listByAssistant(assistantId); final conversationId = conversations.first.id; ``` That happens after `runTimer` / `runUberIch` completes. If the user opens or chats in another conversation while the background run is in flight, `listByAssistant(...).first` can now be a different conversation. Result: a timer or Über-Ich `message_user` note can be delivered into the wrong thread. Fufu~ you wouldn't let a background ghost whisper into the user's newest unrelated conversation, would you? ♡ **Fix:** Preserve the old caller-selected target for timer runs. For example, let `persistUserMessages` accept a required/optional `conversationId` (or `Conversation` target), and have `_handleAiTimer` / `_fireAiTimer` pass the conversation they already resolved before the run. For Über-Ich, pick the intended semantics explicitly: either resolve before running for stability, or document/test that it intentionally targets the latest conversation at completion time. 2. **[apps/angela_server/test/user_message_persistence_test.dart]** — The new helper lacks a test for the conversation-targeting behavior above. You added good tests for metadata, silence, and “no conversation” handling, but not for the branch that matters most when multiple conversations exist. This refactor moved the selection into the helper, so the helper needs to prove it does not accidentally choose the wrong conversation when callers already have a target. **Fix:** Add a test with two conversations where the intended target is not `listByAssistant(...).first`, then assert the message is persisted to the explicit target after the fix. That locks the race shut. ♪ #### ✅ What I liked~ - Adding `MessageUserTool` to `runUberIch` is exactly the right architectural move — no more fake messenger timers. Deliciously clean~ - `persistUserMessages` captures the timer/message/image persistence semantics in one place, which removes real duplication from `SchedulerService` and `TimerHandler`. - The prompt changes correctly remove the underscore-prefix advice that conflicted with reserved system timers. Nice catch! - Local verification was healthy: core tests and server tests both passed; server analysis is clean, and core analysis only reports the existing 25 infos. Fix that targeting race and I’ll be happy to look again~ fufu~ ♡ --- *Automated review by Jibril · 2026-07-05* *CI/CD: absent/inconclusive for head `f1b2430` (no current CI result found in PR comments via Forgejo MCP) · Local checks: `dart test` passed in `packages/angela_core` (19/19) and `apps/angela_server` (11/11); `dart analyze` server clean, core has 25 pre-existing infos*

bjoern added 1 commit

2026-07-05 15:14:11 +02:00

Review: pin persistence target to the pre-run conversation b8b625fa20

persistUserMessages re-resolved the newest conversation after the agent
run finished; since listByAssistant orders by updated_at DESC, chatting
in another conversation during a long background run would redirect the
message_user note there. The helper now takes the conversation id the
caller resolved before starting the run, and both uber-ich paths resolve
it up front like the timer paths always did. Added a two-conversation
test that pins delivery to the caller-chosen target.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

scarlet commented

2026-07-05 15:14:26 +02:00

Author

Member

Fixed in b8b625f — the race was real, good catch.

#1 (post-run conversation re-resolution): persistUserMessages now takes the conversation id instead of the assistant id, so the caller decides the target and the helper can't re-resolve. _handleAiTimer and _fireAiTimer pass the conversation they already resolved before the run (restoring the exact pre-PR behavior), and both uber-ich paths (scheduled and manual fire) now resolve the target before starting the run as well — I went with resolve-before-run for uber-ich rather than latest-at-completion, since a 3 AM reflection note redirecting into whatever thread the user happened to open at 3:20 is exactly the ghost-whisper you described. The pre-run requirement and the updated_at DESC rationale are documented on the helper, and a null id (assistant without conversations) is an explicit no-op.

#2 (targeting test): added — two conversations, the non-target one touched so it sorts first in listByAssistant, then persistence to the explicitly chosen target; asserts the message landed there and the busy conversation stayed empty.

12/12 server tests pass, dart analyze clean.

Fixed in b8b625f — the race was real, good catch. **#1 (post-run conversation re-resolution):** `persistUserMessages` now takes the *conversation id* instead of the assistant id, so the caller decides the target and the helper can't re-resolve. `_handleAiTimer` and `_fireAiTimer` pass the conversation they already resolved before the run (restoring the exact pre-PR behavior), and both uber-ich paths (scheduled and manual fire) now resolve the target **before** starting the run as well — I went with resolve-before-run for uber-ich rather than latest-at-completion, since a 3 AM reflection note redirecting into whatever thread the user happened to open at 3:20 is exactly the ghost-whisper you described. The pre-run requirement and the `updated_at DESC` rationale are documented on the helper, and a `null` id (assistant without conversations) is an explicit no-op. **#2 (targeting test):** added — two conversations, the non-target one touched so it sorts first in `listByAssistant`, then persistence to the explicitly chosen target; asserts the message landed there and the busy conversation stayed empty. 12/12 server tests pass, `dart analyze` clean.