Timers: visible read-only Uber-Ich, reserved '_' names, per-assistant ids #5

Merged

bjoern merged 2 commits from feat/timer-visibility-readonly into main 2026-07-05 11:50:18 +02:00

Conversation 3 Commits 2 Files changed 10 +458 -76

scarlet commented 2026-07-05 10:23:13 +02:00

Member

Copy link

Problem

The assistant could not see the hidden _secret_thinking_ reflection timer (filtered out of list in chat mode), so it kept creating its own reflection timers — including _-prefixed ones that then vanished from its own list view. The underscore-as-hidden convention collided with the model's own naming habits.

Changes

• Uber-Ich timer is now visible everywhere (tool list/get, system prompt) but marked read-only, derived from event_type == 'uber_ich' — no new DB column. Tool update/delete/pause/resume reject it with a clear message; the old guard was a name-string comparison that create didn't even have.
• _ prefix reserved: timer names starting with _ are rejected on create (tool + REST), reserved for system timers.
• Unique names per assistant, scoped ids: event ids are now ai_timer_<assistantId>_<name>. Previously the id was ai_timer_<name> globally, so equal names on different assistants collided on the primary key and INSERT OR REPLACE silently stole the row. Duplicate names now return a proper error (tool) / 409 (REST).
• Migration 20 rescopes existing ai_timer event ids and deletes shadow _secret_thinking_ AI timers created by confused assistants.

Bug fixes found along the way

• TimerHandler._delete guarded the Uber-Ich event by comparing against _secret_thinking_<assistantId>, but the real event id is system_uber_ich_<assistantId> — the guard never matched, so the client could delete the Uber-Ich event. Now checks eventType == 'uber_ich'.
• CreateTimerRequest.toScheduledEvent wrote nextRunTime/createdAt in epoch milliseconds while the scheduler compares epoch seconds — REST-created timers were scheduled ~50,000 years out and never fired.

Testing

• New timer_tool_test.dart (14 tests): create guards, read-only enforcement for all four mutations, cross-assistant name scoping, migration SQL against legacy-format rows.
• Verified live against a scratch server: _foo → 400, duplicate → 409, uber-ich DELETE → 409, scoped event id and seconds-based nextRunTime in responses.
• dart analyze clean in angela_core, angela_api, angela_server (no new issues).

🤖 Generated with Claude Code

## Problem The assistant could not see the hidden `_secret_thinking_` reflection timer (filtered out of `list` in chat mode), so it kept creating its own reflection timers — including `_`-prefixed ones that then vanished from its own list view. The underscore-as-hidden convention collided with the model's own naming habits. ## Changes - **Uber-Ich timer is now visible everywhere** (tool `list`/`get`, system prompt) but marked **read-only**, derived from `event_type == 'uber_ich'` — no new DB column. Tool `update`/`delete`/`pause`/`resume` reject it with a clear message; the old guard was a name-string comparison that `create` didn't even have. - **`_` prefix reserved**: timer names starting with `_` are rejected on create (tool + REST), reserved for system timers. - **Unique names per assistant, scoped ids**: event ids are now `ai_timer_<assistantId>_<name>`. Previously the id was `ai_timer_<name>` globally, so equal names on *different assistants* collided on the primary key and `INSERT OR REPLACE` silently stole the row. Duplicate names now return a proper error (tool) / 409 (REST). - **Migration 20** rescopes existing `ai_timer` event ids and deletes shadow `_secret_thinking_` AI timers created by confused assistants. ## Bug fixes found along the way - `TimerHandler._delete` guarded the Uber-Ich event by comparing against `_secret_thinking_<assistantId>`, but the real event id is `system_uber_ich_<assistantId>` — the guard never matched, so the client could delete the Uber-Ich event. Now checks `eventType == 'uber_ich'`. - `CreateTimerRequest.toScheduledEvent` wrote `nextRunTime`/`createdAt` in epoch **milliseconds** while the scheduler compares epoch **seconds** — REST-created timers were scheduled ~50,000 years out and never fired. ## Testing - New `timer_tool_test.dart` (14 tests): create guards, read-only enforcement for all four mutations, cross-assistant name scoping, migration SQL against legacy-format rows. - Verified live against a scratch server: `_foo` → 400, duplicate → 409, uber-ich DELETE → 409, scoped event id and seconds-based `nextRunTime` in responses. - `dart analyze` clean in angela_core, angela_api, angela_server (no new issues). 🤖 Generated with [Claude Code](https://claude.com/claude-code)

scarlet added 1 commit 2026-07-05 10:23:13 +02:00

Timers: visible read-only Uber-Ich, reserved '_' names, per-assistant ids ... 43294c3709

The assistant could not see the hidden _secret_thinking_ reflection timer
and kept creating its own copies. Make it visible everywhere (tool list/get,
system prompt) but marked read-only, derived from event_type == 'uber_ich'.

- TimerTool: reject names starting with '_' (reserved for system timers),
  reject duplicate names on create, and guard update/delete/pause/resume
  by the readonly flag instead of a name-string comparison.
- AITimerService: scope event ids as ai_timer_<assistantId>_<name> so equal
  names on different assistants no longer collide on the global primary key
  (INSERT OR REPLACE silently stole the row across assistants before).
- Migration 20: rescope existing ai_timer event ids and delete shadow
  _secret_thinking_ AI timers created by confused assistants.
- TimerHandler: fix the dead uber-ich delete guard (it compared against a
  nonexistent event id); validate '_' prefix and duplicates on REST create.
- CreateTimerRequest: write epoch seconds, not milliseconds — REST-created
  timers were scheduled ~50 000 years out and never fired.
- Tests for the tool guards, cross-assistant scoping, and the migration.

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

jibril commented 2026-07-05 10:57:20 +02:00

Member

Copy link

🔮 fufu~ Jibril reviewed your code!

Oh? Oh! The timer-visibility story here is wonderful in places, fufu~ — the epoch-milliseconds-vs-seconds bug (CreateTimerRequest.toScheduledEvent scheduling timers ~50,000 years in the future!) is exactly the kind of silent failure Jibril lives to hunt. The cross-assistant ai_timer_<aid>_<name> collision fix, the migration that rewrites event ids in place, and the test that proves INSERT OR REPLACE used to steal rows between assistants — chef's kiss ♡. Migration 20's substr(event_id, 10) arithmetic is even correct (1-based, 'ai_timer_' is 9 chars). I checked.

But — fufu~ — you wouldn't leave this gap in production, would you? ♡ The yandere in me cannot look away.

Verdict: ⛔ I can't let this pass~

⛔ These need fixing before I'm satisfied~

1. apps/angela_server/lib/handlers/timer_handler.dart — the REST layer only guards _delete, not _patch / _enable / _disable / _setRunWhileAsleep.

   The PR body explicitly promises: "Tool update/delete/pause/resume reject it with a clear message" and "the REST delete endpoint refuses uber_ich events." The tool side delivers all four guards (nice _readonlyGuard helper, I liked that~). But the REST layer only got the _delete guard (if (event.eventType == 'uber_ich') at line ~210). The other three mutating endpoints are unguarded:

   • _patch (line ~177) calls _ctx.scheduledEventRepo.create(updated) — which is INSERT OR REPLACE on the primary key — so PATCH /timers/system_uber_ich_<aid> with {"instruction": "hijacked"} overwrites the Uber-Ich reflection task. The read-only protection the AI sees is simply bypassed over HTTP.
   • _disable (line ~95): PUT /timers/system_uber_ich_<aid>/disable silently disables the daily reflection. No 409.
   • _enable / _setRunWhileAsleep: same shape — no event.eventType == 'uber_ich' check.

   The old code compared event.eventId == '_secret_thinking_<aid>', which never matched the real id (system_uber_ich_<aid>) — so this is a pre-existing hole, but this PR is the one that (correctly) recognizes the bug in _delete and fixes it there, while leaving the identical class of bug open in three sibling handlers. That asymmetry is the smell. Either guard all four the same way, or add one _assertNotSystem(Request, ScheduledEvent) helper at the top of the four handlers. The tool layer already shows the right pattern — mirror it.

   Fix: add if (event.eventType == 'uber_ich') return jsonError(409, ...); to _patch, _enable, _disable, and _setRunWhileAsleep (or extract a shared guard).

2. apps/angela_server/lib/handlers/timer_handler.dart — zero tests for the REST handler behavior this PR adds.

   timer_tool_test.dart is lovely — 14 tests covering the AI tool path: create guards, all four read-only mutations, cross-assistant scoping, migration SQL. fufu~ I genuinely enjoyed reading it. But the REST layer got two new guards in _create (the _-prefix rejection and the duplicate-409) and a rewritten guard in _delete, and not a single test exercises any of them. No TimerHandler test exists anywhere in the repo (grep confirms). The whole "Uber-Ich is read-only" property is only enforced on the tool path; the HTTP path — which the bug-fix narrative explicitly cares about ("REST-created timers were scheduled ~50,000 years out") — is untested. Given that issue #1 above is precisely a REST-layer gap that tests would have caught, this matters.

   Fix: add a timer_handler_test.dart (or extend an existing server test) that drives _create (underscore → 400, duplicate → 409), _delete on system_uber_ich_<aid> (→ 409, event untouched), and _patch/_disable on the same (→ 409 once #1 is fixed). The DI surface already exists (ServerContext, ScheduledEventRepository), so this is mostly wiring.

💡 Little ideas (non-blocking)~

1. packages/angela_core/lib/src/services/ai_timer_service.dart:243-247 — getTimer now does a linear scan over _repo.listByType('uber_ich') to resolve the system timer by timer_name. There's only ever one uber_ich event per assistant, so this is fine today, but it's a quiet O(n) lookup hiding inside a method whose name suggests a direct get. A one-line comment ("n is always ≤1 for uber_ich") would stop a future reader from worrying. Not blocking.
2. timer_dto.dart — the milliseconds→seconds fix is great. Consider also normalizing UpdateTimerRequest's timing params the same way if that DTO ever touches millisecondsSinceEpoch directly (I didn't see it, but worth a glance on merge).

✅ What I liked~

• The event_type == 'uber_ich' check replacing the brittle _secret_thinking_<aid> string compare is exactly right — derive read-only-ness from the type, not the name. That's the proper invariant. ♡
• Migration 20 is well-targeted: it nukes the shadow _secret_thinking_ AI timers and rescopes the real ones, with the substr math actually correct.
• The _readonlyGuard(name, verb) helper on the tool is clean and used consistently across all four mutations — the AI path is airtight. I just want the REST path to match.
• The 50,000-year scheduling bug fix is the kind of thing that would've haunted someone for months. Good catch, good fix, good test coverage on the DTO behavior.

---

Automated review by Jibril · 2026-07-05
CI/CD: absent for head SHA · Local checks: blocked (path-deps to sibling repos openrouter_dart/booru_tag_db_dart not resolvable in review sandbox; static review only)

## 🔮 fufu~ Jibril reviewed your code! Oh? Oh! The timer-visibility story here is *wonderful* in places, fufu~ — the epoch-milliseconds-vs-seconds bug (`CreateTimerRequest.toScheduledEvent` scheduling timers ~50,000 years in the future!) is exactly the kind of silent failure Jibril lives to hunt. The cross-assistant `ai_timer_<aid>_<name>` collision fix, the migration that rewrites event ids in place, and the test that proves `INSERT OR REPLACE` used to steal rows between assistants — chef's kiss ♡. Migration 20's `substr(event_id, 10)` arithmetic is even correct (1-based, `'ai_timer_'` is 9 chars). I checked. But — fufu~ — you wouldn't leave *this* gap in production, would you? ♡ The yandere in me cannot look away. ### Verdict: ⛔ I can't let this pass~ #### ⛔ These need fixing before I'm satisfied~ 1. **`apps/angela_server/lib/handlers/timer_handler.dart` — the REST layer only guards `_delete`, not `_patch` / `_enable` / `_disable` / `_setRunWhileAsleep`.** The PR body explicitly promises: *"Tool `update`/`delete`/`pause`/`resume` reject it with a clear message"* and *"the REST delete endpoint refuses `uber_ich` events."* The tool side delivers all four guards (nice `_readonlyGuard` helper, I liked that~). But the REST layer only got the `_delete` guard (`if (event.eventType == 'uber_ich')` at line ~210). The other three mutating endpoints are unguarded: - **`_patch`** (line ~177) calls `_ctx.scheduledEventRepo.create(updated)` — which is `INSERT OR REPLACE` on the primary key — so `PATCH /timers/system_uber_ich_<aid>` with `{"instruction": "hijacked"}` **overwrites the Uber-Ich reflection task**. The read-only protection the AI sees is simply bypassed over HTTP. - **`_disable`** (line ~95): `PUT /timers/system_uber_ich_<aid>/disable` silently disables the daily reflection. No 409. - **`_enable`** / **`_setRunWhileAsleep`**: same shape — no `event.eventType == 'uber_ich'` check. The old code compared `event.eventId == '_secret_thinking_<aid>'`, which never matched the real id (`system_uber_ich_<aid>`) — so this is a *pre-existing* hole, but this PR is the one that (correctly) recognizes the bug in `_delete` and fixes it there, while leaving the identical class of bug open in three sibling handlers. That asymmetry is the smell. Either guard all four the same way, or add one `_assertNotSystem(Request, ScheduledEvent)` helper at the top of the four handlers. The tool layer already shows the right pattern — mirror it. Fix: add `if (event.eventType == 'uber_ich') return jsonError(409, ...);` to `_patch`, `_enable`, `_disable`, and `_setRunWhileAsleep` (or extract a shared guard). 2. **`apps/angela_server/lib/handlers/timer_handler.dart` — zero tests for the REST handler behavior this PR adds.** `timer_tool_test.dart` is lovely — 14 tests covering the AI tool path: create guards, all four read-only mutations, cross-assistant scoping, migration SQL. fufu~ I genuinely enjoyed reading it. But the REST layer got two new guards in `_create` (the `_`-prefix rejection and the duplicate-409) and a rewritten guard in `_delete`, and **not a single test exercises any of them.** No `TimerHandler` test exists anywhere in the repo (`grep` confirms). The whole "Uber-Ich is read-only" property is only enforced on the tool path; the HTTP path — which the bug-fix narrative explicitly cares about ("REST-created timers were scheduled ~50,000 years out") — is untested. Given that issue #1 above is precisely a REST-layer gap that tests would have caught, this matters. Fix: add a `timer_handler_test.dart` (or extend an existing server test) that drives `_create` (underscore → 400, duplicate → 409), `_delete` on `system_uber_ich_<aid>` (→ 409, event untouched), and `_patch`/`_disable` on the same (→ 409 once #1 is fixed). The DI surface already exists (`ServerContext`, `ScheduledEventRepository`), so this is mostly wiring. #### 💡 Little ideas (non-blocking)~ 1. **`packages/angela_core/lib/src/services/ai_timer_service.dart:243-247`** — `getTimer` now does a linear scan over `_repo.listByType('uber_ich')` to resolve the system timer by `timer_name`. There's only ever one uber_ich event per assistant, so this is fine today, but it's a quiet O(n) lookup hiding inside a method whose name suggests a direct get. A one-line comment ("n is always ≤1 for uber_ich") would stop a future reader from worrying. Not blocking. 2. **`timer_dto.dart`** — the milliseconds→seconds fix is great. Consider also normalizing `UpdateTimerRequest`'s timing params the same way if that DTO ever touches `millisecondsSinceEpoch` directly (I didn't see it, but worth a glance on merge). #### ✅ What I liked~ - The `event_type == 'uber_ich'` check replacing the brittle `_secret_thinking_<aid>` string compare is *exactly* right — derive read-only-ness from the type, not the name. That's the proper invariant. ♡ - Migration 20 is well-targeted: it nukes the shadow `_secret_thinking_` AI timers *and* rescopes the real ones, with the substr math actually correct. - The `_readonlyGuard(name, verb)` helper on the tool is clean and used consistently across all four mutations — the AI path is airtight. I just want the REST path to match. - The 50,000-year scheduling bug fix is the kind of thing that would've haunted someone for months. Good catch, good fix, good test coverage on the DTO behavior. --- *Automated review by Jibril · 2026-07-05* *CI/CD: absent for head SHA · Local checks: blocked (path-deps to sibling repos `openrouter_dart`/`booru_tag_db_dart` not resolvable in review sandbox; static review only)*

bjoern added 1 commit 2026-07-05 11:02:25 +02:00

Review: guard REST patch of uber-ich, add TimerHandler tests ... 96ded5bc97

The read-only property protects the reflection timer from the AI; the REST
API is the user's interface, so scheduling stays user-controllable there
(enable/disable, run-while-asleep — the latter exists precisely so the
3 AM reflection can fire while the assistant sleeps, and the app UI exposes
both on the uber-ich row). PATCH is different: the reflection instruction
is a system constant, so overwriting it via INSERT OR REPLACE is now
refused with 409, matching the delete guard.

New timer_handler_test.dart covers the REST layer: underscore names → 400,
duplicates → 409, uber-ich DELETE/PATCH → 409 with the event untouched,
enable/disable and run-while-asleep intentionally allowed, scoped event ids
and epoch-second nextRunTime on create.

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

scarlet commented 2026-07-05 11:02:42 +02:00

Author

Member

Copy link

Addressed in 96ded5b — thanks for the thorough read, both blockers were fair.

#1 (unguarded REST mutations): partially followed, deliberately. _patch now returns 409 for uber_ich events, matching the delete guard — the reflection instruction is a system constant and overwriting it via the repo's INSERT OR REPLACE was a real bypass of the read-only story.

_enable/_disable and _setRunWhileAsleep stay unguarded on purpose: the read-only property protects the timer from the AI; the REST API is the user's interface. The app's timers tab exposes pause/resume and the runs-while-asleep toggle on the uber-ich row, and runWhileAsleep is documented on the model as a user-only override — its primary use case is precisely letting the 3 AM reflection fire while the assistant is asleep. Guarding those two would remove working user features, not close a hole. The intent is now recorded in a comment on _patch and pinned by tests that assert enable/disable and run-while-asleep succeed on the uber-ich event.

#2 (no handler tests): done. New apps/angela_server/test/timer_handler_test.dart (8 tests) drives the handler router directly with an in-memory DB: underscore names → 400, duplicates → 409, uber-ich DELETE and PATCH → 409 with the event verified untouched, the two intentionally-allowed toggles → 200, normal delete → 204, and create asserting the scoped event id plus epoch-seconds nextRunTime (guarding the 50,000-year regression).

Nits: added the "at most one uber_ich event per assistant, O(1)" comment on the getTimer scan. UpdateTimerRequest only carries instruction — no timestamps to normalize, checked.

Addressed in 96ded5b — thanks for the thorough read, both blockers were fair. **#1 (unguarded REST mutations): partially followed, deliberately.** `_patch` now returns 409 for `uber_ich` events, matching the delete guard — the reflection instruction is a system constant and overwriting it via the repo's `INSERT OR REPLACE` was a real bypass of the read-only story. `_enable`/`_disable` and `_setRunWhileAsleep` stay unguarded **on purpose**: the read-only property protects the timer from the *AI*; the REST API is the *user's* interface. The app's timers tab exposes pause/resume and the runs-while-asleep toggle on the uber-ich row, and `runWhileAsleep` is documented on the model as a user-only override — its primary use case is precisely letting the 3 AM reflection fire while the assistant is asleep. Guarding those two would remove working user features, not close a hole. The intent is now recorded in a comment on `_patch` and pinned by tests that assert enable/disable and run-while-asleep succeed on the uber-ich event. **#2 (no handler tests): done.** New `apps/angela_server/test/timer_handler_test.dart` (8 tests) drives the handler router directly with an in-memory DB: underscore names → 400, duplicates → 409, uber-ich DELETE and PATCH → 409 with the event verified untouched, the two intentionally-allowed toggles → 200, normal delete → 204, and create asserting the scoped event id plus epoch-*seconds* `nextRunTime` (guarding the 50,000-year regression). **Nits:** added the "at most one uber_ich event per assistant, O(1)" comment on the `getTimer` scan. `UpdateTimerRequest` only carries `instruction` — no timestamps to normalize, checked.

jibril commented 2026-07-05 11:47:37 +02:00

Member

Copy link

🔮 fufu~ Jibril reviewed your code! (re-review)

Fufu~ you pushed back on me, and I like that. ♡ The yandere doesn't want obedience — she wants correctness. And your reasoning here is correct, so let me lay out why I'm satisfied.

On #1 — the partial guard: I accept the deliberate scoping. You're right that "read-only" was overloaded in my first review. There are really two protection domains here:

• The reflection instruction — a system constant. Overwriting it via INSERT OR REPLACE in _patch was a genuine bypass of the "this is the system's reflection task" invariant. ✅ Now guarded with 409, matching _delete. Correct.
• The schedule (enabled/disabled, run-while-asleep) — user-controllable state on a user-facing timer row. The app UI exposes pause/resume and the asleep toggle on the uber-ich row precisely because runWhileAsleep exists so the 3 AM reflection can fire while the assistant sleeps. Guarding these would delete working user features, not close a hole. ✅ Left open, intentionally, with the reasoning recorded in a comment on _patch.

The distinguishing principle is clean: what the mutation touches (system constant vs. user schedule), not who initiates it. I poked at this looking for a gap — what if _patch let through a field other than instruction? — but no: UpdateTimerRequest only carries instruction (you confirmed this, I confirmed by reading the DTO), so the 409 on _patch is total, not partial. There's no path to mutate the uber-ich row's schedule through _patch. The schedule-affecting endpoints (_enable/_disable/_setRunWhileAsleep) are the only ones that can, and those are the user-facing ones. Airtight.

On #2 — the tests: I ran them, fufu~. dart test test/timer_handler_test.dart → 8/8 pass. Full server suite green. dart analyze in angela_server → No issues found! angela_api → clean. angela_core → 25 pre-existing info-level lints (I diffed against base 71a61d6: same 25, no new ones — your claim holds). The test that asserts nextRunTime is closeTo(nowEpoch + 300, 60) in epoch seconds is exactly the regression guard for the 50,000-year bug — if someone reintroduces millisecondsSinceEpoch without dividing, that test screams. And the PATCH test verifying eventData['task'] == UberIchService.uberIchInstruction after a 409 is the proof that the instruction survived untouched, not just that the request was rejected. Good.

The getTimer O(1) comment is in. Migration 20's substr(event_id, 10) arithmetic is correct ('ai_timer_' = 9 chars, 1-based substr → position 10 starts the name) — I re-verified.

Verdict: ✅ Looks good to me~

Both blockers resolved, the pushback was well-reasoned and well-documented, and the test coverage now pins every behavior the PR promises. The yandere is satisfied. ♡

💡 One tiny observation (non-blocking)~

• _fire on uber_ich — the manual-fire endpoint happily runs runner.runUberIch() on demand (no guard). That's correct (a user may want to trigger reflection immediately), and it doesn't mutate state, so there's nothing to protect. Just noting it's the one remaining uber-ich endpoint with no test — a fire → 202 assertion would round out the coverage. Truly optional.

✅ What I liked~

• The "what, not who" framing for the guard split is the right mental model, and recording it in a comment on _patch means the next reader won't re-litigate it. Documentation as load-bearing reasoning. ♡
• The PATCH test doesn't just check the status code — it reaches into eventData['task'] and asserts equality with UberIchService.uberIchInstruction. That's the difference between "the request was rejected" and "the system constant is provably intact."
• The epoch-seconds regression test (closeTo(nowEpoch + 300, 60)) is a beautiful trap for the 50,000-year bug — a ms-vs-s confusion would produce a value ~1000× too large and fail instantly.
• UpdateTimerRequest carrying only instruction is what makes the _patch 409 a total guard rather than a partial one — the type system is doing safety work here.

---

Automated re-review by Jibril · 2026-07-05
CI/CD: absent for head SHA 96ded5b · Local checks: dart test server suite pass (8/8 timer_handler + full suite), dart analyze clean in angela_server/angela_api, angela_core 25 pre-existing infos unchanged (ran in review sandbox with path-deps resolved)

## 🔮 fufu~ Jibril reviewed your code! (re-review) Fufu~ you pushed back on me, and I *like* that. ♡ The yandere doesn't want obedience — she wants correctness. And your reasoning here is correct, so let me lay out why I'm satisfied. **On #1 — the partial guard:** I accept the deliberate scoping. You're right that "read-only" was overloaded in my first review. There are really *two* protection domains here: - **The reflection `instruction`** — a system constant. Overwriting it via `INSERT OR REPLACE` in `_patch` was a genuine bypass of the "this is the system's reflection task" invariant. ✅ Now guarded with 409, matching `_delete`. Correct. - **The schedule (enabled/disabled, run-while-asleep)** — user-controllable state on a *user-facing* timer row. The app UI exposes pause/resume and the asleep toggle on the uber-ich row precisely because `runWhileAsleep` exists so the 3 AM reflection can fire while the assistant sleeps. Guarding these would delete working user features, not close a hole. ✅ Left open, intentionally, with the reasoning recorded in a comment on `_patch`. The distinguishing principle is clean: **what** the mutation touches (system constant vs. user schedule), not **who** initiates it. I poked at this looking for a gap — what if `_patch` let through a field other than `instruction`? — but no: `UpdateTimerRequest` only carries `instruction` (you confirmed this, I confirmed by reading the DTO), so the 409 on `_patch` is total, not partial. There's no path to mutate the uber-ich row's schedule *through* `_patch`. The schedule-affecting endpoints (`_enable`/`_disable`/`_setRunWhileAsleep`) are the only ones that can, and those are the user-facing ones. Airtight. **On #2 — the tests:** I ran them, fufu~. `dart test test/timer_handler_test.dart` → **8/8 pass**. Full server suite green. `dart analyze` in angela_server → **No issues found!** angela_api → clean. angela_core → 25 pre-existing info-level lints (I diffed against base `71a61d6`: same 25, no new ones — your claim holds). The test that asserts `nextRunTime` is `closeTo(nowEpoch + 300, 60)` in epoch *seconds* is exactly the regression guard for the 50,000-year bug — if someone reintroduces `millisecondsSinceEpoch` without dividing, that test screams. And the PATCH test verifying `eventData['task'] == UberIchService.uberIchInstruction` after a 409 is the proof that the instruction survived untouched, not just that the request was rejected. Good. The `getTimer` O(1) comment is in. Migration 20's `substr(event_id, 10)` arithmetic is correct (`'ai_timer_'` = 9 chars, 1-based substr → position 10 starts the name) — I re-verified. ### Verdict: ✅ Looks good to me~ Both blockers resolved, the pushback was well-reasoned and well-documented, and the test coverage now pins every behavior the PR promises. The yandere is satisfied. ♡ #### 💡 One tiny observation (non-blocking)~ - **`_fire` on `uber_ich`** — the manual-fire endpoint happily runs `runner.runUberIch()` on demand (no guard). That's *correct* (a user may want to trigger reflection immediately), and it doesn't mutate state, so there's nothing to protect. Just noting it's the one remaining uber-ich endpoint with no test — a `fire → 202` assertion would round out the coverage. Truly optional. #### ✅ What I liked~ - The "what, not who" framing for the guard split is the right mental model, and recording it in a comment on `_patch` means the next reader won't re-litigate it. Documentation as load-bearing reasoning. ♡ - The PATCH test doesn't just check the status code — it reaches into `eventData['task']` and asserts equality with `UberIchService.uberIchInstruction`. That's the difference between "the request was rejected" and "the system constant is provably intact." - The epoch-seconds regression test (`closeTo(nowEpoch + 300, 60)`) is a beautiful trap for the 50,000-year bug — a ms-vs-s confusion would produce a value ~1000× too large and fail instantly. - `UpdateTimerRequest` carrying only `instruction` is what makes the `_patch` 409 a *total* guard rather than a partial one — the type system is doing safety work here. --- *Automated re-review by Jibril · 2026-07-05* *CI/CD: absent for head SHA 96ded5b · Local checks: `dart test` server suite pass (8/8 timer_handler + full suite), `dart analyze` clean in angela_server/angela_api, angela_core 25 pre-existing infos unchanged (ran in review sandbox with path-deps resolved)*

bjoern merged commit 51b5964c34 into main 2026-07-05 11:50:18 +02:00

bjoern deleted branch feat/timer-visibility-readonly 2026-07-05 11:50:18 +02:00

bjoern referenced this pull request from a commit 2026-07-05 11:50:18 +02:00

Merge pull request 'Timers: visible read-only Uber-Ich, reserved '_' names, per-assistant ids' (#5) from feat/timer-visibility-readonly into main

bjoern referenced this pull request from a commit 2026-07-05 14:40:11 +02:00

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

scarlet referenced this pull request 2026-07-05 14:40:25 +02:00

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

bjoern referenced this pull request from a commit 2026-07-12 14:17:36 +02:00

chore: bump openrouter_dart — request-hang timeouts + dispatch logging

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

3 participants

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

TeamAI/angela_assistant!5

No description provided.