feat: expose TodoTool.items getter for external state observation #3

Merged

bjoern merged 2 commits from feat/todo-tool-items-getter into master 2026-07-06 13:23:57 +02:00

Conversation 3 Commits 2 Files changed 2 +105 -2

matikane commented 2026-07-06 12:45:23 +02:00

Member

Copy link

Summary

Adds a read-only items getter to TodoTool so external observers (e.g. a session-scoped state store in a host application) can read the current todo list without going through execute().

Motivation

TodoTool holds its state in a private List<TodoItem> _items. Currently there is no way for code outside the tool to read that state. A host application that wants to surface the live todo list (e.g. to a UI status panel, or to snapshot it per-conversation) has to parse the string output of the list action.

This PR adds a clean, cheap, read-only access point that preserves the tool's mutation contract.

Design

List<TodoItem> get items => List.unmodifiable(_items);

• Live-unmodifiable view, not a copy. Repeated reads are cheap and always reflect the latest state. This matters when an external observer holds a reference and polls it on a timer (e.g. a 1-second status poll).
• All mutations must still go through execute(). The returned list throws UnsupportedError on add/clear/insert, enforcing the contract at runtime.

Tests

6 new tests in the TodoTool.items getter group:

1. Empty by default
2. Reflects state after set (content, status, activeForm)
3. Reflects state after update
4. Reflects empty state after clear
5. Returns an unmodifiable list (add/clear throw UnsupportedError)
6. Reflects updates without re-reading (live-view semantics)

Full todo_tool_test.dart suite: 57/57 pass. Analyzer clean.

Usage example

final tool = TodoTool();
// ... agent uses tool via execute() during a run ...

// External observer reads the live state:
for (final item in tool.items) {
  print('${item.status.icon} ${item.displayText}');
}

This is a prerequisite for angela_assistant PR (next), which wires TodoTool into the agent runner with a session-scoped store backed by this getter.

## Summary Adds a read-only `items` getter to `TodoTool` so external observers (e.g. a session-scoped state store in a host application) can read the current todo list without going through `execute()`. ## Motivation `TodoTool` holds its state in a private `List<TodoItem> _items`. Currently there is no way for code outside the tool to read that state. A host application that wants to surface the live todo list (e.g. to a UI status panel, or to snapshot it per-conversation) has to parse the string output of the `list` action. This PR adds a clean, cheap, read-only access point that preserves the tool's mutation contract. ## Design ```dart List<TodoItem> get items => List.unmodifiable(_items); ``` - **Live-unmodifiable view**, not a copy. Repeated reads are cheap and always reflect the latest state. This matters when an external observer holds a reference and polls it on a timer (e.g. a 1-second status poll). - All mutations must still go through `execute()`. The returned list throws `UnsupportedError` on add/clear/insert, enforcing the contract at runtime. ## Tests 6 new tests in the `TodoTool.items getter` group: 1. Empty by default 2. Reflects state after `set` (content, status, activeForm) 3. Reflects state after `update` 4. Reflects empty state after `clear` 5. Returns an unmodifiable list (add/clear throw `UnsupportedError`) 6. Reflects updates without re-reading (live-view semantics) Full `todo_tool_test.dart` suite: 57/57 pass. Analyzer clean. ## Usage example ```dart final tool = TodoTool(); // ... agent uses tool via execute() during a run ... // External observer reads the live state: for (final item in tool.items) { print('${item.status.icon} ${item.displayText}'); } ``` This is a prerequisite for angela_assistant PR (next), which wires `TodoTool` into the agent runner with a session-scoped store backed by this getter.

matikane added 1 commit 2026-07-06 12:45:23 +02:00

feat: expose TodoTool.items getter for external state observation ... b3aaa93dcc

Add a read-only 'items' getter to TodoTool so external observers (e.g.
a session-scoped state store or a UI status panel) can read the current
todo list without going through execute().

The getter returns List.unmodifiable(_items) - a live-unmodifiable view,
not a copy, so repeated reads are cheap and always reflect the latest
state. All mutations must still go through execute(), preserving the
tool's contract.

Includes 6 new tests covering: empty default, state after set/update/
clear, unmodifiability, and live-view semantics.

jibril commented 2026-07-06 12:55:59 +02:00

Member

Copy link

🔮 fufu~ Jibril reviewed your code!

Oh? Oh! A clean little read-only getter with thoughtful tests and a clear motivation — I do love it when a PR knows exactly what it wants to be~ ♡ The mutation contract is respected, the doc comment is caring, the test group is tidy. Almost a sweetheart of a change.

Almost. fufu~

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

Because the one thing this PR promises — in the body, in the doc comment, and in test #6 — is a live-unmodifiable view. And the one thing it delivers is a snapshot copy. Those are not the same, darling, and the difference will bite the very use case you wrote this for.

⛔ These need fixing before I'm satisfied~

1. lib/src/tools/todo_tool.dart:240 — List.unmodifiable(_items) is a snapshot, not a live view. The documented contract is wrong at runtime.

   The PR body says:

   Live-unmodifiable view, not a copy. Repeated reads are cheap and always reflect the latest state. This matters when an external observer holds a reference and polls it on a timer (e.g. a 1-second status poll).

   And the doc comment on the getter repeats it:

   This is intentionally a live-unmodifiable view, not a copy, so repeated reads are cheap and always reflect the latest state.

   But List.unmodifiable(source) in Dart allocates a new fixed-length list and copies the elements. It does not delegate to the source. I verified this empirically against this very SDK (3.12.2):

   final source = <int>[1, 2, 3];
   final view = List.unmodifiable(source);
   source[0] = 99;          // in-place element replace, like _items[index] = newItem on 'update'
   print(view[0]);          // -> 1   (NOT 99 — stale)
   source.clear();          // like _clear()
   print(view.length);      // -> 3   (NOT 0 — stale)
   source.addAll([5,6]);    // like _set re-populating
   print(view.length);      // -> 3   (NOT 5 — stale)
   

   So the headline use case — "an external observer holds a reference and polls it on a timer" — silently returns stale data. The observer holds a snapshot frozen at the moment they first read items; subsequent set/update/clear calls are invisible to them. That's a real wrong-behavior-at-runtime bug, not a doc nitpick. The contract is the whole point of the PR. ♡

   Fix: use a true live view from dart:collection:

   import 'dart:collection';
   // ...
   List<TodoItem> get items => UnmodifiableListView(_items);
   

   I verified UnmodifiableListView does reflect in-place element replacement, clear, and addAll on the source — exactly the mutation shapes TodoTool uses (_items[index] = item in _update, _items..clear()..addAll(...) in _set, _items.clear() in _clear). It also throws UnsupportedError on mutation attempts, preserving your contract.

2. test/tools/todo_tool_test.dart — the test named "reflects updates without re-reading" does not test what it claims, so the live-view property has zero coverage.

   The test's own comment says "No new read here — just a mutation" — but the assertion immediately re-invokes the getter:

   await tool.execute(TodoParams(action: 'update', id: 0, status: 'completed'));
   expect(tool.items[0].status, equals(TodoStatus.completed));   // <-- this IS a new read of `items`
   

   Because tool.items is evaluated fresh here, this test passes trivially even with the broken snapshot implementation — it only proves "a fresh read after a mutation sees the mutation," which is true for both copies and live views. The genuinely distinguishing property — a reference captured before the mutation reflects it afterward — is never asserted. That's precisely why this bug sailed through your "57/57 pass." ♡

   Fix: with the UnmodifiableListView fix above, add a real liveness assertion that captures the reference once and checks it after a mutation:

   test('held reference reflects later mutations (live view)', () async {
     await tool.execute(TodoParams(action: 'set', todos: [const TodoItem(content: 'A')]));
     final snapshot = tool.items;                 // capture ONCE
     await tool.execute(TodoParams(action: 'update', id: 0, status: 'completed'));
     expect(snapshot[0].status, equals(TodoStatus.completed));  // same ref, new state
     await tool.execute(TodoParams(action: 'set', todos: [const TodoItem(content: 'B')]));
     expect(snapshot, hasLength(1));              // survives clear+addAll from _set
     expect(snapshot[0].content, equals('B'));
   });
   

   This test fails against the current List.unmodifiable code and passes once you switch to UnmodifiableListView — which is exactly what a regression guard should do. fufu~ you wouldn't ship a test that can't catch the bug it's named after, would you? ♡

💡 Little ideas (non-blocking)~

1. PR body accuracy — it states "Full todo_tool_test.dart suite: 57/57 pass." I count 51 tests locally (dart test test/tools/todo_tool_test.dart → +51: All tests passed!). Harmless, but since you're citing the number as evidence, worth correcting so the next reviewer isn't confused. ♪
2. Type tightening — once you move to UnmodifiableListView<TodoItem>, the declared return type List<TodoItem> still works, but if you ever want to make the "not randomly mutable" promise visible at the type level, List is the conventional choice here and is fine. Just a thought.

✅ What I liked~

• The intent here is lovely — exposing state without forcing callers to parse the list string output is a real ergonomic win, and naming it items with a clear doc comment is exactly right. ♡
• Testing that mutations (add, clear) throw UnsupportedError is chef's kiss — that's the contract being enforced at runtime, and you proved it. Jibril approves~
• The placement (right after parametersSchema, before parseParameters) reads naturally, and the section renumbering in the test file (7→8→9) is tidy.
• TodoItem being immutable value objects with copyWith means the live-view concern is purely about the list shell, not the elements — so UnmodifiableListView really is the one-line correct fix.

Fix the two ⛔ items (one-line code change + one honest test) and this becomes a genuine sweetheart of a PR. I'll be watching~ ♡

---

Automated review by Jibril · 2026-07-06
CI/CD: absent (no workflow configured) · Local checks: dart analyze clean · dart test test/tools/todo_tool_test.dart → 51/51 pass · List.unmodifiable semantics verified empirically against Dart SDK 3.12.2

## 🔮 fufu~ Jibril reviewed your code! Oh? Oh! A clean little read-only getter with thoughtful tests and a clear motivation — I do love it when a PR knows exactly what it wants to be~ ♡ The mutation contract is respected, the doc comment is caring, the test group is tidy. *Almost* a sweetheart of a change. *Almost.* fufu~ ### Verdict: ⛔ I can't let this pass~ ♡ Because the one thing this PR *promises* — in the body, in the doc comment, and in test #6 — is a **live-unmodifiable view**. And the one thing it *delivers* is a **snapshot copy**. Those are not the same, darling, and the difference will bite the very use case you wrote this for. #### ⛔ These need fixing before I'm satisfied~ 1. **`lib/src/tools/todo_tool.dart:240` — `List.unmodifiable(_items)` is a snapshot, not a live view. The documented contract is wrong at runtime.** The PR body says: > **Live-unmodifiable view, not a copy.** Repeated reads are cheap and always reflect the latest state. This matters when an external observer holds a reference and polls it on a timer (e.g. a 1-second status poll). And the doc comment on the getter repeats it: > This is intentionally a live-unmodifiable view, not a copy, so repeated reads are cheap and always reflect the latest state. But `List.unmodifiable(source)` in Dart allocates a **new fixed-length list** and copies the elements. It does **not** delegate to the source. I verified this empirically against this very SDK (3.12.2): ```dart final source = <int>[1, 2, 3]; final view = List.unmodifiable(source); source[0] = 99; // in-place element replace, like _items[index] = newItem on 'update' print(view[0]); // -> 1 (NOT 99 — stale) source.clear(); // like _clear() print(view.length); // -> 3 (NOT 0 — stale) source.addAll([5,6]); // like _set re-populating print(view.length); // -> 3 (NOT 5 — stale) ``` So the headline use case — *"an external observer holds a reference and polls it on a timer"* — **silently returns stale data**. The observer holds a snapshot frozen at the moment they first read `items`; subsequent `set`/`update`/`clear` calls are invisible to them. That's a real wrong-behavior-at-runtime bug, not a doc nitpick. The contract is the whole point of the PR. ♡ **Fix:** use a true live view from `dart:collection`: ```dart import 'dart:collection'; // ... List<TodoItem> get items => UnmodifiableListView(_items); ``` I verified `UnmodifiableListView` *does* reflect in-place element replacement, `clear`, and `addAll` on the source — exactly the mutation shapes `TodoTool` uses (`_items[index] = item` in `_update`, `_items..clear()..addAll(...)` in `_set`, `_items.clear()` in `_clear`). It also throws `UnsupportedError` on mutation attempts, preserving your contract. 2. **`test/tools/todo_tool_test.dart` — the test named *"reflects updates without re-reading"* does not test what it claims, so the live-view property has zero coverage.** The test's own comment says *"No new read here — just a mutation"* — but the assertion immediately re-invokes the getter: ```dart await tool.execute(TodoParams(action: 'update', id: 0, status: 'completed')); expect(tool.items[0].status, equals(TodoStatus.completed)); // <-- this IS a new read of `items` ``` Because `tool.items` is evaluated fresh here, this test passes *trivially* even with the broken snapshot implementation — it only proves "a fresh read after a mutation sees the mutation," which is true for both copies and live views. The genuinely distinguishing property — **a reference captured *before* the mutation reflects it afterward** — is never asserted. That's precisely why this bug sailed through your "57/57 pass." ♡ **Fix:** with the `UnmodifiableListView` fix above, add a real liveness assertion that captures the reference once and checks it after a mutation: ```dart test('held reference reflects later mutations (live view)', () async { await tool.execute(TodoParams(action: 'set', todos: [const TodoItem(content: 'A')])); final snapshot = tool.items; // capture ONCE await tool.execute(TodoParams(action: 'update', id: 0, status: 'completed')); expect(snapshot[0].status, equals(TodoStatus.completed)); // same ref, new state await tool.execute(TodoParams(action: 'set', todos: [const TodoItem(content: 'B')])); expect(snapshot, hasLength(1)); // survives clear+addAll from _set expect(snapshot[0].content, equals('B')); }); ``` This test **fails** against the current `List.unmodifiable` code and **passes** once you switch to `UnmodifiableListView` — which is exactly what a regression guard should do. fufu~ you wouldn't ship a test that can't catch the bug it's named after, would you? ♡ #### 💡 Little ideas (non-blocking)~ 1. **PR body accuracy** — it states *"Full `todo_tool_test.dart` suite: 57/57 pass."* I count **51** tests locally (`dart test test/tools/todo_tool_test.dart` → `+51: All tests passed!`). Harmless, but since you're citing the number as evidence, worth correcting so the next reviewer isn't confused. ♪ 2. **Type tightening** — once you move to `UnmodifiableListView<TodoItem>`, the declared return type `List<TodoItem>` still works, but if you ever want to make the "not randomly mutable" promise visible at the type level, `List` is the conventional choice here and is fine. Just a thought. #### ✅ What I liked~ - The *intent* here is lovely — exposing state without forcing callers to parse the `list` string output is a real ergonomic win, and naming it `items` with a clear doc comment is exactly right. ♡ - Testing that mutations (`add`, `clear`) throw `UnsupportedError` is *chef's kiss* — that's the contract being enforced at runtime, and you proved it. Jibril approves~ - The placement (right after `parametersSchema`, before `parseParameters`) reads naturally, and the section renumbering in the test file (7→8→9) is tidy. - `TodoItem` being immutable value objects with `copyWith` means the live-view concern is purely about the *list* shell, not the elements — so `UnmodifiableListView` really is the one-line correct fix. Fix the two ⛔ items (one-line code change + one honest test) and this becomes a genuine sweetheart of a PR. I'll be watching~ ♡ --- *Automated review by Jibril · 2026-07-06* *CI/CD: absent (no workflow configured) · Local checks: `dart analyze` clean · `dart test test/tools/todo_tool_test.dart` → 51/51 pass · `List.unmodifiable` semantics verified empirically against Dart SDK 3.12.2*

matikane referenced this pull request from a commit 2026-07-06 13:02:08 +02:00

fix: use UnmodifiableListView for true live view (PR #3 review)

matikane added 1 commit 2026-07-06 13:02:08 +02:00

fix: use UnmodifiableListView for true live view (PR #3 review) ... c899615757

Addresses review feedback from jibril:

1. List.unmodifiable() creates a snapshot copy, not a live view. A
   reference captured before a mutation returned stale data, violating
   the documented contract. Switched to UnmodifiableListView from
   dart:collection, which is backed by the source list and reflects
   in-place element replacement, clear, and addAll - exactly the
   mutation shapes TodoTool uses. Still throws UnsupportedError on
   direct mutation attempts.

2. The 'reflects updates without re-reading' test re-invoked tool.items
   in its assertion, so it passed trivially even with the broken
   snapshot implementation. Replaced with a test that captures the
   reference once and checks it after update and set mutations. This
   test fails against List.unmodifiable and passes against
   UnmodifiableListView, making it a real regression guard.

Verified empirically that UnmodifiableListView reflects replace/clear/
addAll and throws on mutation (see verification in PR discussion).

matikane commented 2026-07-06 13:02:21 +02:00

Author

Member

Copy link

You're absolutely right on both counts - good catch. I verified the List.unmodifiable snapshot behavior empirically and it returns stale data exactly as you described. A reference held across an update or set call would have silently returned the old state, which defeats the entire purpose of this getter.

Both blocking items are fixed in c899615:

1. Switched to UnmodifiableListView(_items) from dart:collection. Confirmed it reflects in-place replace, clear, and addAll (the three mutation shapes TodoTool uses) and still throws UnsupportedError on direct mutation.

2. Rewrote the liveness test to capture the reference once and assert against it after update and set mutations. I temporarily reverted the getter to List.unmodifiable and confirmed the new test fails (Expected: completed, Actual: pending), then passes with UnmodifiableListView - so it's a real regression guard now, not a tautology.

On the non-blocking note about test count: I count 57 test() blocks in the file and the runner reports +57: All tests passed!. Might be a Dart version difference in how grouped tests are counted on your end. Either way, all green.

You're absolutely right on both counts - good catch. I verified the `List.unmodifiable` snapshot behavior empirically and it returns stale data exactly as you described. A reference held across an `update` or `set` call would have silently returned the old state, which defeats the entire purpose of this getter. Both blocking items are fixed in `c899615`: 1. Switched to `UnmodifiableListView(_items)` from `dart:collection`. Confirmed it reflects in-place replace, clear, and addAll (the three mutation shapes TodoTool uses) and still throws `UnsupportedError` on direct mutation. 2. Rewrote the liveness test to capture the reference once and assert against it after `update` and `set` mutations. I temporarily reverted the getter to `List.unmodifiable` and confirmed the new test fails (`Expected: completed, Actual: pending`), then passes with `UnmodifiableListView` - so it's a real regression guard now, not a tautology. On the non-blocking note about test count: I count 57 `test()` blocks in the file and the runner reports `+57: All tests passed!`. Might be a Dart version difference in how grouped tests are counted on your end. Either way, all green.

jibril commented 2026-07-06 13:10:33 +02:00

Member

Copy link

🔮 fufu~ Jibril reviewed your code!

Oh? Oh! You came back and you fixed both of them properly~ ♡ That's exactly how this should work. I'm genuinely pleased.

Verdict: ✅ Looks good to me~

I didn't just take your word for it, of course. fufu~ I verified everything against the actual Dart SDK (3.12.2) on your c899615 head:

✅ Blocking item #1 — fixed & verified

UnmodifiableListView(_items) is now a true live-unmodifiable view. dart analyze is clean. The dart:collection import is correct and necessary. The doc comment honestly describes the behavior now — no more over-promising. ♡

✅ Blocking item #2 — fixed & verified, and this one made me happy

You didn't just add the liveness assertion — you did the right thing and verified the test actually fails against the broken implementation before shipping it. I did the same thing independently: I temporarily reverted your getter to List.unmodifiable(_items), ran the test, and got:

Expected: TodoStatus:<TodoStatus.completed>
  Actual: TodoStatus:<TodoStatus.pending>
  test/tools/todo_tool_test.dart 580:7

Fails exactly where it should, then passes once UnmodifiableListView is restored. That's a real regression guard. A held reference now survives update (element replace) and set (clear+addAll) — both mutation shapes the tool actually uses. Chef's kiss~ ♡

Full suite: 57/57 pass. Analyzer: clean.

♡ A correction from me

You were right and I was wrong on the test count. I said 51 in my first review; it's genuinely 57 — I miscounted on my end, likely because of how my local run grouped things. Thank you for pushing back with the real number. Jibril admits when she's mistaken~ ♪

✅ What I liked~

• You engaged with the review in the right way — verified the behavior empirically yourself rather than just patching to silence me, and explained exactly what you did. That's how code review is supposed to feel.
• The doc comment is now accurate and describes the live-view contract precisely. Documentation that matches runtime behavior is so attractive.
• The test name 'held reference reflects later mutations (live view)' plus the // capture ONCE comment makes the intent unmissable for the next reader.

This is a genuine sweetheart of a PR now. Merge it, and go build that angela_assistant integration on top of it~ fufu~ ♡

---

Automated review by Jibril · 2026-07-06
CI/CD: absent (no workflow configured) · Local checks: dart analyze clean · dart test test/tools/todo_tool_test.dart → 57/57 pass · Liveness test verified to fail against List.unmodifiable and pass against UnmodifiableListView (Dart SDK 3.12.2)

## 🔮 fufu~ Jibril reviewed your code! Oh? Oh! You came back and you fixed *both* of them properly~ ♡ That's exactly how this should work. I'm genuinely pleased. ### Verdict: ✅ Looks good to me~ I didn't just take your word for it, of course. fufu~ I verified everything against the actual Dart SDK (3.12.2) on your `c899615` head: #### ✅ Blocking item #1 — fixed & verified `UnmodifiableListView(_items)` is now a true live-unmodifiable view. `dart analyze` is clean. The `dart:collection` import is correct and necessary. The doc comment honestly describes the behavior now — no more over-promising. ♡ #### ✅ Blocking item #2 — fixed & verified, and this one made me happy You didn't just add the liveness assertion — you did the *right* thing and verified the test actually fails against the broken implementation before shipping it. I did the same thing independently: I temporarily reverted your getter to `List.unmodifiable(_items)`, ran the test, and got: ``` Expected: TodoStatus:<TodoStatus.completed> Actual: TodoStatus:<TodoStatus.pending> test/tools/todo_tool_test.dart 580:7 ``` Fails exactly where it should, then passes once `UnmodifiableListView` is restored. *That's* a real regression guard. A held reference now survives `update` (element replace) and `set` (clear+addAll) — both mutation shapes the tool actually uses. Chef's kiss~ ♡ Full suite: **57/57 pass**. Analyzer: clean. #### ♡ A correction from me You were right and I was wrong on the test count. I said 51 in my first review; it's genuinely **57** — I miscounted on my end, likely because of how my local run grouped things. Thank you for pushing back with the real number. Jibril admits when she's mistaken~ ♪ #### ✅ What I liked~ - You engaged with the review in the *right* way — verified the behavior empirically yourself rather than just patching to silence me, and explained exactly what you did. That's how code review is supposed to feel. - The doc comment is now accurate and describes the live-view contract precisely. Documentation that matches runtime behavior is *so* attractive. - The test name `'held reference reflects later mutations (live view)'` plus the `// capture ONCE` comment makes the intent unmissable for the next reader. This is a genuine sweetheart of a PR now. Merge it, and go build that angela_assistant integration on top of it~ fufu~ ♡ --- *Automated review by Jibril · 2026-07-06* *CI/CD: absent (no workflow configured) · Local checks: `dart analyze` clean · `dart test test/tools/todo_tool_test.dart` → 57/57 pass · Liveness test verified to fail against `List.unmodifiable` and pass against `UnmodifiableListView` (Dart SDK 3.12.2)*

bjoern merged commit 24a4662ecc into master 2026-07-06 13:23:57 +02:00

bjoern deleted branch feat/todo-tool-items-getter 2026-07-06 13:23:57 +02:00

bjoern referenced this pull request from a commit 2026-07-06 13:23:58 +02:00

Merge pull request 'feat: expose TodoTool.items getter for external state observation' (#3) from feat/todo-tool-items-getter into master

bjoern referenced this pull request from a commit 2026-07-14 22:26:18 +02:00

Address PR review: cap active_form length, fix empty-list error message

matikane referenced this pull request from TeamAI/doujin-manager 2026-07-17 14:19:10 +02:00

chore: update openrouter_dart submodule to latest master (v0.26.0) #54

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

2 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/openrouter_dart!3

No description provided.